[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


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 24 Jan 2022 14:47:58 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=OEvldL7r+gvkyG7UlEiOP9ytDWewVxZDREslfjHbfro=; b=CCQcqGyp5mKvrJU6/4EHsI2WcuiJ03n7Ki3QIXxftq/lgrRUzhwU3soi9c2t0/Ghm1AoyWiN19S/4EDJl51hnTw47rbiUkSDka9hvQIK68gLCcJ9U0X9dOFX5AkWV0lmIJ9wrBNZaQ3zz1f0EMd0+jxCtHS++9qmDM/6ONgYty/CYltkb+EPBi8+MJNXgRs/JLM8pbaCRLUAqCYxyRj2KDsmt3tPdy9Hh2FH0zhB3HAN99ICm7jvGz4pSQDThgqkdypt1HSglbHXIjJ/cfPOom4mrAO14yQsIOeofAnFLXstVOeyglJzC/sWtbE63hxU9jdb7HnRgMIrJeaL+7vDBQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Ufe/MQPOXmf4QAj/sr6WyidtOesw9hOcXUPICg4nGICrsPb+IYcuYMcJJ2omMO4P09ky02HzLcPuaLOS6t5OhPhMdAMIp2iI8bajxdpXLUmWolD41CaJldSkRO4/5SZRgJLWRNdj4eoR93rPrwfC49L5CbodJKzs5vJ6DuiOJ1fxhreeok7HD8ivdoJHBU4RTqa4VcWODxR93UXOlbqvIMrX6IptsTKHXKvUKXE+al3R1QSWZnnj5Q3sx1e00kPTGZh3BIpIlCr6yHVM88jxN2FDjEqWX4AGw/x8TOM6p84p3T3nUUlrPdQt7Iq3gdSCXCaZcDjA0H/hUqMggebtAQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 24 Jan 2022 13:48:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 20.01.2022 16:23, Roger Pau Monne wrote:
> Both QEMU/KVM and HyperV support using bits 11:5 from the MSI address
> field in order to store the high part of the target APIC ID. This
> allows expanding the maximum APID ID usable without interrupt
> remapping support from 255 to 32768.
> 
> Note the interface used by QEMU for emulated devices (via the
> XEN_DMOP_inject_msi hypercall) already passes both the address and
> data fields into Xen for processing, so there's no need for any change
> to QEMU there.
> 
> However for PCI passthrough devices QEMU uses the
> XEN_DOMCTL_bind_pt_irq hypercall which does need an addition to the
> gflags field in order to pass the high bits of the APIC destination
> ID.
> 
> Introduce a new CPUID flag to signal the support for the feature. The
> introduced flag covers both the support for extended ID for the
> IO-APIC RTE and the MSI address registers. Such flag is currently only
> exposed when the domain is using vPCI (ie: a PVH dom0).

Because of also covering the IO-APIC side, I think the CPUID aspect of
this really wants splitting into a 3rd patch. That way the MSI and
IO-APIC parts could in principle go in independently, and only the
CPUID one needs to remain at the tail.

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -66,7 +66,7 @@ static void vmsi_inj_irq(
>  
>  int vmsi_deliver(
>      struct domain *d, int vector,
> -    uint8_t dest, uint8_t dest_mode,
> +    unsigned int dest, unsigned int dest_mode,

If you change the type of dest_mode, then to "bool" please - see its
only call site.

> @@ -123,7 +125,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct 
> hvm_pirq_dpci *pirq_dpci)
>  }
>  
>  /* Return value, -1 : multi-dests, non-negative value: dest_vcpu_id */
> -int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t 
> dest_mode)
> +int hvm_girq_dest_2_vcpu_id(struct domain *d, unsigned int dest,
> +                            unsigned int dest_mode)

Same here then.

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

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

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.