Re: [Xen-devel] [PATCH v4 20/28] xen/pt: when binding guest msi, accept the whole msi message

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

On Fri, Nov 17, 2017 at 02:22:27PM +0800, Chao Gao wrote:
> ... rather than a filtered one. Previously, some fields (reserved or
> unalterable) are filtered by QEMU. These fields are useless for the
> legacy interrupt format (i.e. non remappable format). However, these
> fields are meaningful to remappable format. Accepting the whole msi
> message will significantly reduce the efforts to support binding
> remappable format msi.

This should be sent as a separate patch series, together with the
required QEMU change. Batching it in this series it's going to make it
harder to commit IMO.

Also note that the QEMU side needs to be committed and backported to
the qemu-xen tree before applying the Xen side.

> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
> v4:
>  - new
> ---
>  tools/libxc/include/xenctrl.h |  7 ++++---
>  tools/libxc/xc_domain.c       | 14 ++++++++------
>  xen/arch/x86/hvm/vmsi.c       | 12 ++++++------
>  xen/drivers/passthrough/io.c  | 36 +++++++++++++++++-------------------
>  xen/include/asm-x86/hvm/irq.h |  5 +++--
>  xen/include/public/domctl.h   |  8 ++------
>  6 files changed, 40 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 666db0b..8ade90c 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1756,16 +1756,17 @@ int xc_domain_ioport_mapping(xc_interface *xch,
>  int xc_domain_update_msi_irq(
>      xc_interface *xch,
>      uint32_t domid,
> -    uint32_t gvec,
>      uint32_t pirq,
> +    uint64_t addr,
> +    uint32_t data,
>      uint32_t gflags,

If you pass addr and data, do you really need to also pass gflags?

> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 7126de7..5edb0e7 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -101,12 +101,12 @@ int vmsi_deliver(
>  
>  void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci 
> *pirq_dpci)
>  {
> -    uint32_t flags = pirq_dpci->gmsi.gflags;
> -    int vector = pirq_dpci->gmsi.gvec;
> -    uint8_t dest = (uint8_t)flags;
> -    bool dest_mode = flags & XEN_DOMCTL_VMSI_X86_DM_MASK;
> -    uint8_t delivery_mode = MASK_EXTR(flags, XEN_DOMCTL_VMSI_X86_DELIV_MASK);
> -    bool trig_mode = flags & XEN_DOMCTL_VMSI_X86_TRIG_MASK;
> +    uint8_t vector = pirq_dpci->gmsi.data & MSI_DATA_VECTOR_MASK;

MASK_EXTR please (here and elsewhere).

> +    uint8_t dest = MASK_EXTR(pirq_dpci->gmsi.addr, MSI_ADDR_DEST_ID_MASK);
> +    bool dest_mode = pirq_dpci->gmsi.addr & MSI_ADDR_DESTMODE_MASK;
> +    uint8_t delivery_mode = MASK_EXTR(pirq_dpci->gmsi.data,
> +                                      MSI_DATA_DELIVERY_MODE_MASK);
> +    bool trig_mode = pirq_dpci->gmsi.data & MSI_DATA_TRIGGER_MASK;
>  
>      HVM_DBG_LOG(DBG_LEVEL_IOAPIC,
>                  "msi: dest=%x dest_mode=%x delivery_mode=%x "
> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
> index 8f16e6c..d8c66bf 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -339,19 +339,17 @@ int pt_irq_create_bind(
>      {
>      case PT_IRQ_TYPE_MSI:
>      {
> -        uint8_t dest, delivery_mode;
> +        uint8_t dest, delivery_mode, gvec;

I'm not sure you really need the gvec local variable, AFAICT it's used
only once.

> diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
> index 3b6b4bd..3a8832c 100644
> --- a/xen/include/asm-x86/hvm/irq.h
> +++ b/xen/include/asm-x86/hvm/irq.h
> @@ -132,9 +132,10 @@ struct dev_intx_gsi_link {
>  #define HVM_IRQ_DPCI_TRANSLATE       (1u << _HVM_IRQ_DPCI_TRANSLATE_SHIFT)
>  
>  struct hvm_gmsi_info {
> -    uint32_t gvec;
> -    uint32_t gflags;
> +    uint32_t data;
>      int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */
> +    uint64_t addr;
> +    uint8_t gvec;

Can't you just obtain the guest vector from addr and flags?

>      bool posted; /* directly deliver to guest via VT-d PI? */
>  };
>  
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 9f6f0aa..2717c68 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -536,15 +536,11 @@ struct xen_domctl_bind_pt_irq {
>              uint8_t intx;
>          } pci;
>          struct {
> -            uint8_t gvec;
>              uint32_t gflags;
> -#define XEN_DOMCTL_VMSI_X86_DEST_ID_MASK 0x0000ff
> -#define XEN_DOMCTL_VMSI_X86_RH_MASK      0x000100
> -#define XEN_DOMCTL_VMSI_X86_DM_MASK      0x000200
> -#define XEN_DOMCTL_VMSI_X86_DELIV_MASK   0x007000
> -#define XEN_DOMCTL_VMSI_X86_TRIG_MASK    0x008000
>  #define XEN_DOMCTL_VMSI_X86_UNMASKED     0x010000

Oh, I see, you need gflags for the unmask thing only.

>  
> +            uint32_t data;
> +            uint64_t addr;
>              uint64_aligned_t gtable;
>          } msi;
>          struct {
> -- 
> 1.8.3.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel