| Message ID | 59A9A6260200007800176A6A@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
>>> 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
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.
>>> 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
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);
>>> 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
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.
--- 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 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>