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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> 
> > --- a/xen/drivers/passthrough/x86/hvm.c
> > +++ b/xen/drivers/passthrough/x86/hvm.c
> > @@ -269,7 +269,7 @@ int pt_irq_create_bind(
> >      {
> >      case PT_IRQ_TYPE_MSI:
> >      {
> > -        uint8_t dest, delivery_mode;
> > +        unsigned int dest, delivery_mode;
> >          bool dest_mode;
> 
> If you touch delivery_mode's type, wouldn't that better become bool?
> 
> > --- 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.

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.