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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 19, 2023 at 12:11:09PM +0200, Jan Beulich wrote:
> On 19.04.2023 11:41, Roger Pau Monné wrote:
> > On Tue, Apr 18, 2023 at 05:12:07PM +0100, Andrew Cooper wrote:
> >> On 18/04/2023 5:02 pm, Roger Pau Monné wrote:
> >>> On Tue, Apr 18, 2023 at 04:54:49PM +0100, 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.
> >>> I agree, but didn't think it was necessary to get into debating
> >>> whether it's useful or not, since its introduction was bogus anyway.
> >>
> >> Well - the reason to actually make a change is that (older) guests are
> >> really exploding on that BUG_ON() for reasons outside of their own control.
> >>
> >> And the reason to fix it by ignoring VCPU_SSHOTTMR_future is because the
> >> entire concept is broken and should never have existed.
> >>
> >> The ABI argument just adds to why the patch ought to have been rejected
> >> at the time.  But it was done, and the fact it has been like this for 16
> >> years means that the ABI shouldn't change further, even if it done in
> >> error in the first place.
> >>
> >>>
> >>>> It can literally only be used to shoot yourself in the foot with, and
> >>>> more recent Linuxes have dropped their use of it.
> >>>>
> >>>> The structure needs to stay it's current shape, and while it's fine to
> >>>> hide the VCPU_SSHOTTMR_future behind an interface version macro, we do
> >>>> need to say that it is explicitly ignored.
> >>> Oh, I think I've dropped the comment I had added next to
> >>> VCPU_SSHOTTMR_future that contained /* Ignored. */ (just like for the 
> >>> whole
> >>> flags field).
> >>>
> >>> I can elaborate a bit on why VCPU_SSHOTTMR_future is not useful in the
> >>> commit log, and add that Ignored comment to the flag.
> >>
> >> The important thing is to not actually change the size of the structure,
> >> and to change the commit message to explain the real reason why we need
> >> to make the change.
> > 
> > Why not revert back to the previous (smaller) size of the structure?
> > 
> > That would work for guests that have been built with Xen 3.0 headers.
> 
> Are there any such guests known to still be in active use? Linux iirc
> requires 4.0 as a minimum ...

That would be something from the pre-pvops Linux days.

I looked forward a bit, and I don't think the introduction of the
field was an ABI breakage, because it was done a day after introducing
the original hypercall, so no released version of the hypervisor
headers contained the structure without the flags field:

commit 505ef3ea86870bb8a35533ec9d446f98a6b61ea6
Author: kfraser@localhost.localdomain <kfraser@localhost.localdomain>
Date:   Sat Mar 10 16:58:11 2007 +0000

    Add flags field to VCPUOP_set_singlsehot_timer.
    Flag 'future' causes Xen to check if the timeout is in the past and
    return -ETIME if so.
    From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
    Signed-off-by: Keir Fraser <keir@xxxxxxxxxxxxx>

commit eb1a565927c0fdcd89be41f6d063c458539cca8d
Author: kfraser@localhost.localdomain <kfraser@localhost.localdomain>
Date:   Fri Mar 9 18:26:47 2007 +0000

    xen: New vcpu_op commands for setting periodic and single-shot timers.
    Signed-off-by: Keir Fraser <keir@xxxxxxxxxxxxx>

So I think my proposal is to declare the flag as deprecated (and
effectively ignored in the hypervisor) due to the bogus usage of it in
Linux.

Thanks, Roger.