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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> --- 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.

Jan

> I've been wondering about this interface though
> (xen_domctl_bind_pt_irq), and it would seem better to just pass the
> raw MSI address and data fields from the guest and let Xen do the
> decoding. This however is not trivial to do as we would need to keep
> the previous interface anyway as it's used by QEMU. Maybe we could
> have some kind of union between a pair of address and data fields and
> a gflags one that would match the native layout, but as said not
> trivial (and would require using anonymous unions which I'm not sure
> are accepted even for domctls in the public headers).
> 
> Thanks, Roger.
>