Re: [Xen-devel] [PATCH] xen: use vMSI related #define-s from public interface

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 1 Sep 2017, Jan Beulich wrote:
> Xen and qemu having identical #define-s (with different names) is a
> strong hint that these should have been part of the public interface
> from the very start. Use them if they're available, falling back to
> privately defined values only when using older headers.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Hi Jan,

Thanks for the patch and sorry for the delay in reviewing it.


> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -18,6 +18,11 @@
>  
>  #define XEN_PT_AUTO_ASSIGN -1
>  
> +#ifndef XEN_DOMCTL_VMSI_X86_DEST_ID_MASK
> +#if XEN_DOMCTL_INTERFACE_VERSION >= 0x0000000e
> +#error vMSI defines missing from domctl.h
> +#endif

All the version compatibility stuff goes to
include/hw/xen/xen_common.h. Please move it there.

We usually assume that the Xen version we are building against is
"sane", so we don't do #error's typically.


> +
>  /* shift count for gflags */
>  #define XEN_PT_GFLAGS_SHIFT_DEST_ID        0
>  #define XEN_PT_GFLAGS_SHIFT_RH             8
> @@ -26,6 +31,16 @@
>  #define XEN_PT_GFLAGSSHIFT_TRG_MODE       15
>  #define XEN_PT_GFLAGSSHIFT_UNMASKED       16
>  
> +#define XEN_DOMCTL_VMSI_X86_DEST_ID_MASK (0xffU << 
> XEN_PT_GFLAGS_SHIFT_DEST_ID)
> +#define XEN_DOMCTL_VMSI_X86_RH_MASK      (1U << XEN_PT_GFLAGS_SHIFT_RH)
> +#define XEN_DOMCTL_VMSI_X86_DM_MASK      (1U << XEN_PT_GFLAGS_SHIFT_DM)
> +#define XEN_DOMCTL_VMSI_X86_DELIV_MASK   (7U << 
> XEN_PT_GFLAGSSHIFT_DELIV_MODE)
> +#define XEN_DOMCTL_VMSI_X86_TRIG_MASK    (1U << XEN_PT_GFLAGSSHIFT_TRG_MODE)
> +#define XEN_DOMCTL_VMSI_X86_UNMASKED     (1U << XEN_PT_GFLAGSSHIFT_UNMASKED)
> +#endif
> +
> +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))

MASK_INSR can stay in this file.


>  #define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)]
>  
>  /*
> @@ -49,21 +64,18 @@ static inline uint32_t msi_ext_dest_id(u
>  
>  static uint32_t msi_gflags(uint32_t data, uint64_t addr)
>  {
> -    uint32_t result = 0;
> -    int rh, dm, dest_id, deliv_mode, trig_mode;
> +    int rh, dm, deliv_mode, trig_mode;
>  
>      rh = (addr >> MSI_ADDR_REDIRECTION_SHIFT) & 0x1;
>      dm = (addr >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1;
> -    dest_id = msi_dest_id(addr);
>      deliv_mode = (data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x7;
>      trig_mode = (data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
>  
> -    result = dest_id | (rh << XEN_PT_GFLAGS_SHIFT_RH)
> -        | (dm << XEN_PT_GFLAGS_SHIFT_DM)
> -        | (deliv_mode << XEN_PT_GFLAGSSHIFT_DELIV_MODE)
> -        | (trig_mode << XEN_PT_GFLAGSSHIFT_TRG_MODE);
> -
> -    return result;
> +    return MASK_INSR(msi_dest_id(addr), XEN_DOMCTL_VMSI_X86_DEST_ID_MASK) |
> +           MASK_INSR(rh, XEN_DOMCTL_VMSI_X86_RH_MASK) |
> +           MASK_INSR(dm, XEN_DOMCTL_VMSI_X86_DM_MASK) |
> +           MASK_INSR(deliv_mode, XEN_DOMCTL_VMSI_X86_DELIV_MASK) |
> +           MASK_INSR(trig_mode, XEN_DOMCTL_VMSI_X86_TRIG_MASK);
>  }
>  
>  static inline uint64_t msi_addr64(XenPTMSI *msi)
> @@ -173,7 +185,7 @@ static int msi_msix_update(XenPCIPassthr
>          table_addr = s->msix->mmio_base_addr;
>      }
>  
> -    gflags |= masked ? 0 : (1u << XEN_PT_GFLAGSSHIFT_UNMASKED);
> +    gflags |= masked ? 0 : XEN_DOMCTL_VMSI_X86_UNMASKED;
>  
>      rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec,
>                                    pirq, gflags, table_addr);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel