[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/vcpu: remove vcpu_set_singleshot_timer flags field
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 ... Jan > 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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |