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.coop...@citrix.com>
> >>>>> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> >>>> 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 <jer...@goop.org>
Signed-off-by: Keir Fraser <k...@xensource.com>
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 <k...@xensource.com>
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.
