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

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

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.
Currently Xen does copy past the original (3.0) structure and likely
copy rubbish from the guest stack from those guests, that might
contain the VCPU_SSHOTTMR_future bit set and end up returning -ENOTIME
unexpectedly.

Overall I don't see the benefit of keeping the flags field, even if
technically it's been so long the ABI was broken that it doesn't
matter anymore.  But still keeping an empty flags field is kind of
useless, the more that removing it allows removing the compat code
handling for VCPUOP_set_singleshot_timer.

Thanks, Roger.