[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 19 Apr 2023 12:11:09 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=OwhbEs/gsN5KpCp38BYdSeIDQo3sgtftXc5pm0kvaWc=; b=kWzlMFY4ou1h99H4DrTFHn+x3zVXLtU+iyCFk5etU2PcgGjM8XMDWcyN8beatnhofmYmnghND1qwzhJN69xsm7nqCryTdvN2UjMwx90/rTdKqL1JwoOCmUv8wlg06M2tikH06dh2cdUD2Ow4ikLyOV18F4v5tEJVnsvXDAvj/fZf1Fwh/Cll+H5+TSQAUkdCKxKVzS8FsFIib6+prDHKarE+zSv9IHLVgp/75B5JX1CehW8Ami65KU+mDs69qvwqKJWaIUvZuckCDWCFECm1VGsIJ6SbNPQxdPMCE2YKDdW4vwOJktjitmpXUk2sUYqHnKLVUL/nMizGu4KNDaUDTg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bFwjJqE8enGa6aP2p5bg2ibJKhNRIWFHDmYePl9Clugx5e/NVtY1Tubvyaf6ztcBUS6oJN3RiV87GfoRqgTjAdYSPFVS30bpu60hc7QA7Z8gJOiQDKUvGrLZesWA3OrODmk2b39Dqjn2rK01JXpoGCUPrMpzTamNEILo6Uq2p++qZuG7bA76Ec3v9QCAbz1XB/2aQOz8CgCfQieS8F0ABy6/oOOsgkqHhN9OtIhlPci4bynUbr+BWPD3Unk9sXQ0DfxzuTu6HzNT2f+9yFj/RwXGcQBxlGn8eeH7w+JWmNVdfGfd/LNJgiFsDGQhay3iyGHUyq8G83wA7GBhAWplhw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Henry Wang <Henry.Wang@xxxxxxx>, Community Manager <community.manager@xxxxxxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Delivery-date: Wed, 19 Apr 2023 10:11:21 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.