Re: [PATCH] xen/vcpu: remove vcpu_set_singleshot_timer flags field

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2023 17:54, Andrew Cooper wrote:
> On 18/04/2023 4:42 pm, Roger Pau Monne wrote:
>> The addition of the flags field in the vcpu_set_singleshot_timer in
>> 505ef3ea8687 is an ABI breakage, as the size of the structure is
>> increased.
>>
>> Remove such field addition and drop the implementation of the
>> VCPU_SSHOTTMR_future flag.  If a timer provides an expired timeout
>> value just inject the timer interrupt.
>>
>> Bump the Xen interface version, and keep the flags field and
>> VCPU_SSHOTTMR_future available for guests using the old interface.
>>
>> Note the removal of the field from the vcpu_set_singleshot_timer
>> struct allows removing the compat translation of the struct.
>>
>> Fixes: 505ef3ea8687 ('Add flags field to VCPUOP_set_singlsehot_timer.')
>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> While everything said is true, this isn't the reason to to get rid of
> VCPU_SSHOTTMR_future
> 
> It 505ef3ea8687 does appear to have been an ABI break, that's
> incidental.  It dates from 2007 so whatever we have now is the de-facto
> ABI, whether we like it or not.
> 
> The reason to delete this is because it is a monumentality and entirely
> stupid idea which should have been rejected outright at the time, and
> the only guest we can find which uses it also BUG_ON()'s in response to
> -ETIME.

The instance in Linux (up to 4.6) that I could find was

        BUG_ON(ret != 0 && ret != -ETIME);

i.e. not really dying when getting back -ETIME. (And if there really was
a BUG_ON(ret) somewhere despite setting the flag, it would be a bug there,
not something to "fix" in Xen.) I'm afraid I also disagree on "stupid
idea" as well as ...

> It can literally only be used to shoot yourself in the foot with, and
> more recent Linuxes have dropped their use of it.

... this: If used correctly, it can avoid injection of a pointless event.
Clearly the Linux change dropping use of the flag indicates that its use
wasn't correct (anymore?), likely because of not properly dealing with
-ETIME up the call stack. I'm willing to trust Jeremy / Keir that at the
time of its introduction such a problem didn't exist.

Jan