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

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

[Jan Beulich]

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@suse.com>

--- 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
+
 /* 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))
+
 #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);

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

[Jan Beulich]

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@suse.com>

--- 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
+
 /* 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))
+
 #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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Roger Pau Monné]

On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
defines), I guess it doesn't matter much because we only care for
stable releases.

I would probably be fine without the interface check and the error,
but I don't know the approach we usually take regarding those.

Thanks, Roger.

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

[Roger Pau Monné]

On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
defines), I guess it doesn't matter much because we only care for
stable releases.

I would probably be fine without the interface check and the error,
but I don't know the approach we usually take regarding those.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 01.09.17 at 20:20, <roger.pau@citrix.com> wrote:
> On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- 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
> 
> XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
> defines), I guess it doesn't matter much because we only care for
> stable releases.

Well, that's if you build qemu against master Xen. What about
people building against older Xen versions/headers?

Jan

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

[Jan Beulich]

>>> On 01.09.17 at 20:20, <roger.pau@citrix.com> wrote:
> On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- 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
> 
> XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
> defines), I guess it doesn't matter much because we only care for
> stable releases.

Well, that's if you build qemu against master Xen. What about
people building against older Xen versions/headers?

Jan

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

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

[Roger Pau Monné]

On Mon, Sep 04, 2017 at 03:04:41AM -0600, Jan Beulich wrote:
> >>> On 01.09.17 at 20:20, <roger.pau@citrix.com> wrote:
> > On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> --- 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
> > 
> > XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
> > defines), I guess it doesn't matter much because we only care for
> > stable releases.
> 
> Well, that's if you build qemu against master Xen. What about
> people building against older Xen versions/headers?

Sorry, I think I haven't explained myself clearly. What I mean is
that if this change gets committed before the Xen side one, QEMU would
not compile against current Xen. As said, this is only a transitory
issue, and it's never going to be a problem in stable branches. This
is because of the "#error" that you add.

Roger.

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

[Roger Pau Monné]

On Mon, Sep 04, 2017 at 03:04:41AM -0600, Jan Beulich wrote:
> >>> On 01.09.17 at 20:20, <roger.pau@citrix.com> wrote:
> > On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> --- 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
> > 
> > XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
> > defines), I guess it doesn't matter much because we only care for
> > stable releases.
> 
> Well, that's if you build qemu against master Xen. What about
> people building against older Xen versions/headers?

Sorry, I think I haven't explained myself clearly. What I mean is
that if this change gets committed before the Xen side one, QEMU would
not compile against current Xen. As said, this is only a transitory
issue, and it's never going to be a problem in stable branches. This
is because of the "#error" that you add.

Roger.

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

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

[Jan Beulich]

>>> On 04.09.17 at 11:23, <roger.pau@citrix.com> wrote:
> On Mon, Sep 04, 2017 at 03:04:41AM -0600, Jan Beulich wrote:
>> >>> On 01.09.17 at 20:20, <roger.pau@citrix.com> wrote:
>> > On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>
>> > 
>> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> Thanks.
>> 
>> >> --- 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
>> > 
>> > XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
>> > defines), I guess it doesn't matter much because we only care for
>> > stable releases.
>> 
>> Well, that's if you build qemu against master Xen. What about
>> people building against older Xen versions/headers?
> 
> Sorry, I think I haven't explained myself clearly. What I mean is
> that if this change gets committed before the Xen side one, QEMU would
> not compile against current Xen. As said, this is only a transitory
> issue, and it's never going to be a problem in stable branches. This
> is because of the "#error" that you add.

Oh, yes, that's the case. I can't see an easy way around it, but I'm
pretty sure the qemu side of it will see some further delay anyway.

Jan

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

[Jan Beulich]

>>> On 04.09.17 at 11:23, <roger.pau@citrix.com> wrote:
> On Mon, Sep 04, 2017 at 03:04:41AM -0600, Jan Beulich wrote:
>> >>> On 01.09.17 at 20:20, <roger.pau@citrix.com> wrote:
>> > On Fri, Sep 01, 2017 at 10:25:42AM -0600, 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@suse.com>
>> > 
>> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> Thanks.
>> 
>> >> --- 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
>> > 
>> > XEN_DOMCTL_INTERFACE_VERSION is already 0xe (without you added
>> > defines), I guess it doesn't matter much because we only care for
>> > stable releases.
>> 
>> Well, that's if you build qemu against master Xen. What about
>> people building against older Xen versions/headers?
> 
> Sorry, I think I haven't explained myself clearly. What I mean is
> that if this change gets committed before the Xen side one, QEMU would
> not compile against current Xen. As said, this is only a transitory
> issue, and it's never going to be a problem in stable branches. This
> is because of the "#error" that you add.

Oh, yes, that's the case. I can't see an easy way around it, but I'm
pretty sure the qemu side of it will see some further delay anyway.

Jan

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

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

[Stefano Stabellini]

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@suse.com>

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

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

[Stefano Stabellini]

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@suse.com>

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 21.09.17 at 03:12, <sstabellini@kernel.org> wrote:
> On Fri, 1 Sep 2017, Jan Beulich wrote:
>> --- 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.

I know there's a central place, but moving there stuff that's
needed only in this file seemed rather counterproductive to
me - why would you want all files including that shared one
have to see these definitions? If there was a remote chance
that some other file may need to make use of it, I might
agree, but I don't see any such chance at all.

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

Hmm, I can drop the #error, but to be honest I'm hesitant to do
so - I've put it there intentionally.

Jan

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

[Jan Beulich]

>>> On 21.09.17 at 03:12, <sstabellini@kernel.org> wrote:
> On Fri, 1 Sep 2017, Jan Beulich wrote:
>> --- 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.

I know there's a central place, but moving there stuff that's
needed only in this file seemed rather counterproductive to
me - why would you want all files including that shared one
have to see these definitions? If there was a remote chance
that some other file may need to make use of it, I might
agree, but I don't see any such chance at all.

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

Hmm, I can drop the #error, but to be honest I'm hesitant to do
so - I've put it there intentionally.

Jan


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

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

[Stefano Stabellini]

On Thu, 21 Sep 2017, Jan Beulich wrote:
> >>> On 21.09.17 at 03:12, <sstabellini@kernel.org> wrote:
> > On Fri, 1 Sep 2017, Jan Beulich wrote:
> >> --- 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.
> 
> I know there's a central place, but moving there stuff that's
> needed only in this file seemed rather counterproductive to
> me - why would you want all files including that shared one
> have to see these definitions? If there was a remote chance
> that some other file may need to make use of it, I might
> agree, but I don't see any such chance at all.

Basically, xen_common.h is meant to supply all definitions and
declarations that should already be part of the Xen interface, but
because of versioning, they are not.

Xen headers + xen_common.h = all declarations and definitions

So far, we haven't distinguished based on how many users of a given
missing functions exist in QEMU. Simply, xen_common.h fills all gaps.
One day, xen_common.h could become a set of header files, so that in
cases such as this, one can only import the compat header file that she
needs. But today we only have one compat header in QEMU.


> > We usually assume that the Xen version we are building against is
> > "sane", so we don't do #error's typically.
> 
> Hmm, I can drop the #error, but to be honest I'm hesitant to do
> so - I've put it there intentionally.

It is difficult to draw the line when an #error is needed and when it is
not. I am just trying to be consistent. Why do you think we should have
it in this specific case? Do you think we cannot make correct
assumptions based on the XEN_DOMCTL_INTERFACE_VERSION only or the Xen
version? If so, please explain.

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

[Stefano Stabellini]

On Thu, 21 Sep 2017, Jan Beulich wrote:
> >>> On 21.09.17 at 03:12, <sstabellini@kernel.org> wrote:
> > On Fri, 1 Sep 2017, Jan Beulich wrote:
> >> --- 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.
> 
> I know there's a central place, but moving there stuff that's
> needed only in this file seemed rather counterproductive to
> me - why would you want all files including that shared one
> have to see these definitions? If there was a remote chance
> that some other file may need to make use of it, I might
> agree, but I don't see any such chance at all.

Basically, xen_common.h is meant to supply all definitions and
declarations that should already be part of the Xen interface, but
because of versioning, they are not.

Xen headers + xen_common.h = all declarations and definitions

So far, we haven't distinguished based on how many users of a given
missing functions exist in QEMU. Simply, xen_common.h fills all gaps.
One day, xen_common.h could become a set of header files, so that in
cases such as this, one can only import the compat header file that she
needs. But today we only have one compat header in QEMU.


> > We usually assume that the Xen version we are building against is
> > "sane", so we don't do #error's typically.
> 
> Hmm, I can drop the #error, but to be honest I'm hesitant to do
> so - I've put it there intentionally.

It is difficult to draw the line when an #error is needed and when it is
not. I am just trying to be consistent. Why do you think we should have
it in this specific case? Do you think we cannot make correct
assumptions based on the XEN_DOMCTL_INTERFACE_VERSION only or the Xen
version? If so, please explain.

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