[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/3] x86/vmsi: add support for extended destination ID in address field
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |