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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Feb 12, 2018 at 03:16:25PM +0000, Roger Pau Monné wrote:
>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.

Will do.

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

As for compatibility, How about introducing an new API for binding
interrupt using an unfiltered message? QEMU maintainer thinks changing
an API between QEMU and Xen is not good.

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

It seems yes. Will try to remove 'gvec' field.

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

Yes. When we were rebasing, we found a conflict here. And after some
study about the new flag, It is no easy to remove this flag.

Thanks
Chao

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