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.