Re: [PATCH 2/3] x86/vmsi: add support for extended destination ID in address field

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2022 10:54, Roger Pau Monné wrote:
> On Fri, Feb 04, 2022 at 10:30:54AM +0100, Jan Beulich wrote:
>> On 04.02.2022 10:23, Roger Pau Monné wrote:
>>> On Mon, Jan 24, 2022 at 02:47:58PM +0100, Jan Beulich wrote:
>>>> On 20.01.2022 16:23, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/include/asm/msi.h
>>>>> +++ b/xen/arch/x86/include/asm/msi.h
>>>>> @@ -54,6 +54,7 @@
>>>>>  #define MSI_ADDR_DEST_ID_SHIFT           12
>>>>>  #define   MSI_ADDR_DEST_ID_MASK          0x00ff000
>>>>>  #define  MSI_ADDR_DEST_ID(dest)          (((dest) << 
>>>>> MSI_ADDR_DEST_ID_SHIFT) & MSI_ADDR_DEST_ID_MASK)
>>>>> +#define   MSI_ADDR_EXT_DEST_ID_MASK      0x0000fe0
>>>>
>>>> Especially the immediately preceding macro now becomes kind of stale.
>>>
>>> Hm, I'm not so sure about that. We could expand the macro to place the
>>> high bits in dest at bits 11:4 of the resulting address. However that
>>> macro (MSI_ADDR_DEST_ID) is only used by Xen to compose its own
>>> messages, so until we add support for the hypervisor itself to use the
>>> extended destination ID mode there's not much point in modifying the
>>> macro IMO.
>>
>> Well, this is all unhelpful considering the different form of extended
>> ID in Intel's doc. At least by way of a comment things need clarifying
>> and potential pitfalls need pointing out imo.
> 
> Sure, will add some comments there.
> 
>>>>> --- a/xen/include/public/domctl.h
>>>>> +++ b/xen/include/public/domctl.h
>>>>> @@ -588,6 +588,7 @@ struct xen_domctl_bind_pt_irq {
>>>>>  #define XEN_DOMCTL_VMSI_X86_DELIV_MASK   0x007000
>>>>>  #define XEN_DOMCTL_VMSI_X86_TRIG_MASK    0x008000
>>>>>  #define XEN_DOMCTL_VMSI_X86_UNMASKED     0x010000
>>>>> +#define XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK 0xfe0000
>>>>
>>>> I'm not convinced it is a good idea to limit the overall destination
>>>> ID width to 15 bits here - at the interface level we could as well
>>>> permit more bits right away; the implementation would reject too high
>>>> a value, of course. Not only with this I further wonder whether the
>>>> field shouldn't be unsplit while extending it. You won't get away
>>>> without bumping XEN_DOMCTL_INTERFACE_VERSION anyway (unless it was
>>>> bumped already for 4.17) since afaics the unused bits of this field
>>>> previously weren't checked for being zero. We could easily have 8
>>>> bits vector, 16 bits flags, and 32 bits destination ID in the struct.
>>>> And there would then still be 8 unused bits (which from now on we
>>>> ought to check for being zero).
>>>
>>> So I've made gflags a 64bit field, used the high 32bits for the
>>> destination ID, and the low ones for flags. I've left gvec as a
>>> separate field in the struct, as I don't want to require a
>>> modification to QEMU, just a rebuild against the updated headers will
>>> be enough.
>>
>> Hmm, wait - if qemu uses this without going through a suitable
>> abstraction in at least libxc, then we cannot _ever_ change this
>> interface: If a rebuild was required, old qemu binaries would
>> stop working with newer Xen. If such a dependency indeed exists,
>> we'll need a prominent warning comment in the public header.
> 
> Hm, it's bad. The xc_domain_update_msi_irq interface uses a gflags
> parameter that's the gflags parameter of xen_domctl_bind_pt_irq. Which
> is even worse because it's not using the mask definitions from
> domctl.h, but rather a copy of them named XEN_PT_GFLAGS_* that are
> hardcoded in xen_pt_msi.c in QEMU code.
> 
> So we can likely expand the layout of gflags, but moving fields is not
> an option. I think my original proposal of adding a
> XEN_DOMCTL_VMSI_X86_EXT_DEST_ID_MASK mask is the less bad option until
> we add a new stable interface for device passthrough for QEMU.

Given the observations - yeah, not much of a choice left.

Jan