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

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

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.

Thanks, Roger.