[RFC,3/4] xen-pt: bind/unbind interrupt remapping format MSI

From: Chao Gao <chao.gao@intel.com>

If a vIOMMU is exposed to guest, guest will configure the msi to remapping
format. The original code isn't suitable to the new format. A new pair
bind/unbind interfaces are added for this usage. This patch recognizes
this case and use new interfaces to bind/unbind msi.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
 include/hw/i386/apic-msidef.h |  1 +
 2 files changed, 29 insertions(+), 8 deletions(-)

On Fri, Mar 17, 2017 at 07:29:16PM +0800, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> If a vIOMMU is exposed to guest, guest will configure the msi to remapping
> format. The original code isn't suitable to the new format. A new pair
> bind/unbind interfaces are added for this usage. This patch recognizes
> this case and use new interfaces to bind/unbind msi.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
>  include/hw/i386/apic-msidef.h |  1 +
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 62add06..8b0d7fc 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>      uint8_t gvec = msi_vector(data);
>      uint32_t gflags = msi_gflags(data, addr);
>      int rc = 0;
> +    bool ir = !!(addr & MSI_ADDR_IM_MASK);
>      uint64_t table_addr = 0;
>  
>      XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
> @@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>          table_addr = s->msix->mmio_base_addr;
>      }
>  
> -    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
> +    if (ir) {

You could maybe use add&MSI_ADDR_IM_MASK instead of going through a
variable.

> +        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
> +                                    d->devfn, data, addr, table_addr);

Do you also want to update the XEN_PT_LOG above? Since it does not
always reflect the update_msi call anymore.

> +    }
> +    else {
> +        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>                                    pirq, gflags, table_addr);
> +    }
>  
>      if (rc) {
>          XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
> @@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>      }
>  
>      if (is_binded) {
> -        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
> -                   is_msix ? "-X" : "", pirq, gvec);
> -        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> -        if (rc) {
> -            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> -                       is_msix ? "-X" : "", errno, pirq, gvec);
> -            return rc;
> +        if ( addr & MSI_ADDR_IM_MASK ) {
> +            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",

For addr, it should be PRIx64 instead of %lx.

> +                       is_msix ? "-X" : "", pirq, data, addr);
> +            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
> +                                                    d->devfn, data, addr);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
> +                           is_msix ? "-X" : "", rc, pirq, data, addr);
> +                return rc;
> +            }
> +
> +        } else {
> +            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
> +                       is_msix ? "-X" : "", pirq, gvec);
> +            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> +                           is_msix ? "-X" : "", errno, pirq, gvec);
> +                return rc;
> +            }
>          }
>      }
>  
> diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
> index 8b4d4cc..08b584f 100644
> --- a/include/hw/i386/apic-msidef.h
> +++ b/include/hw/i386/apic-msidef.h
> @@ -27,5 +27,6 @@
>  #define MSI_ADDR_DEST_ID_SHIFT          12
>  #define MSI_ADDR_DEST_IDX_SHIFT         4
>  #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0

Could you add a 0 to dest_id here? So their will be 8 digit and it those
not look weird when compared to the next define.

> +#define  MSI_ADDR_IM_MASK               0x00000010

Is the definition of MSI_ADDR_IM_MASK available somewhere? In the Intel
SDM I've only found this bit to be reserved.

Thanks,

On Thu, Mar 30, 2017 at 05:51:45PM +0100, Anthony PERARD wrote:
>On Fri, Mar 17, 2017 at 07:29:16PM +0800, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> If a vIOMMU is exposed to guest, guest will configure the msi to remapping
>> format. The original code isn't suitable to the new format. A new pair
>> bind/unbind interfaces are added for this usage. This patch recognizes
>> this case and use new interfaces to bind/unbind msi.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  hw/xen/xen_pt_msi.c           | 36 ++++++++++++++++++++++++++++--------
>>  include/hw/i386/apic-msidef.h |  1 +
>>  2 files changed, 29 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
>> index 62add06..8b0d7fc 100644
>> --- a/hw/xen/xen_pt_msi.c
>> +++ b/hw/xen/xen_pt_msi.c
>> @@ -161,6 +161,7 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>>      uint8_t gvec = msi_vector(data);
>>      uint32_t gflags = msi_gflags(data, addr);
>>      int rc = 0;
>> +    bool ir = !!(addr & MSI_ADDR_IM_MASK);
>>      uint64_t table_addr = 0;
>>  
>>      XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x"
>> @@ -171,8 +172,14 @@ static int msi_msix_update(XenPCIPassthroughState *s,
>>          table_addr = s->msix->mmio_base_addr;
>>      }
>>  
>> -    rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>> +    if (ir) {
>
>You could maybe use add&MSI_ADDR_IM_MASK instead of going through a
>variable.
>
>> +        rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq,
>> +                                    d->devfn, data, addr, table_addr);
>
>Do you also want to update the XEN_PT_LOG above? Since it does not
>always reflect the update_msi call anymore.

Yes. I adjust the output.

>
>> +    }
>> +    else {
>> +        rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>>                                    pirq, gflags, table_addr);
>> +    }
>>  
>>      if (rc) {
>>          XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
>> @@ -204,13 +211,26 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>>      }
>>  
>>      if (is_binded) {
>> -        XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
>> -                   is_msix ? "-X" : "", pirq, gvec);
>> -        rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
>> -        if (rc) {
>> -            XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
>> -                       is_msix ? "-X" : "", errno, pirq, gvec);
>> -            return rc;
>> +        if ( addr & MSI_ADDR_IM_MASK ) {
>> +            XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, addr: %lx)\n",
>
>For addr, it should be PRIx64 instead of %lx.
>
>> +                       is_msix ? "-X" : "", pirq, data, addr);
>> +            rc = xc_domain_unbind_msi_irq_remapping(xen_xc, xen_domid, pirq,
>> +                                                    d->devfn, data, addr);
>> +            if (rc) {
>> +                XEN_PT_ERR(d, "Unbinding of MSI%s . (error: %d, pirq: %d, data: %x, addr: %lx)\n",
>> +                           is_msix ? "-X" : "", rc, pirq, data, addr);
>> +                return rc;
>> +            }
>> +
>> +        } else {
>> +            XEN_PT_LOG(d, "Unbind MSI%s with pirq %d, gvec %#x\n",
>> +                       is_msix ? "-X" : "", pirq, gvec);
>> +            rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
>> +            if (rc) {
>> +                XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
>> +                           is_msix ? "-X" : "", errno, pirq, gvec);
>> +                return rc;
>> +            }
>>          }
>>      }
>>  
>> diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
>> index 8b4d4cc..08b584f 100644
>> --- a/include/hw/i386/apic-msidef.h
>> +++ b/include/hw/i386/apic-msidef.h
>> @@ -27,5 +27,6 @@
>>  #define MSI_ADDR_DEST_ID_SHIFT          12
>>  #define MSI_ADDR_DEST_IDX_SHIFT         4
>>  #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0
>
>Could you add a 0 to dest_id here? So their will be 8 digit and it those
>not look weird when compared to the next define.
>

Will do.

>> +#define  MSI_ADDR_IM_MASK               0x00000010
>
>Is the definition of MSI_ADDR_IM_MASK available somewhere? In the Intel
>SDM I've only found this bit to be reserved.

Yes, it is defined in VT-d spec 5.1.5.2 MSI and MSI-X Register Programming.
I made a mistake here. I should use MSI_ADDR_IF_MASK. 

Thanks
Chao

>
>Thanks,
>
>-- 
>Anthony PERARD