| Message ID | 1495085580-10631-2-git-send-email-tianyu.lan@intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 18, 2017 at 01:32:59AM -0400, 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 | 50 ++++++++++++++++++++++++++++++++----------- > include/hw/i386/apic-msidef.h | 3 ++- > 2 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c > index 62add06..5fab95e 100644 > --- a/hw/xen/xen_pt_msi.c > +++ b/hw/xen/xen_pt_msi.c > @@ -163,16 +163,24 @@ static int msi_msix_update(XenPCIPassthroughState *s, > int rc = 0; > uint64_t table_addr = 0; > > - XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x" > - " (entry: %#x)\n", > - is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry); > - > if (is_msix) { > table_addr = s->msix->mmio_base_addr; > } > > - rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec, > - pirq, gflags, table_addr); > + if (addr & MSI_ADDR_IF_MASK) { > + XEN_PT_LOG(d, "Updating MSI%s with addr %#" PRIx64 "data %#x\n", With a space before "data", I think it will be easier to read the debug log. > + is_msix ? "-X": "", addr, data); > + rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq, > + d->devfn, data, addr, table_addr); We are going to need a stub function for xc_domain_update_msi_irq_remapping(), when Xen does not have support for it, so QEMU can compile in any case. (same for unbind version.) I think the stub can just return -ENOSYS. That going to require changes in configure to detect newer xen version and the stub can be in xen_common.h. > + } > + else { > + XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x" > + " (entry: %#x)\n", > + is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry); > + > + 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 +212,29 @@ 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_IF_MASK ) { > + XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, " > + "addr: %#" PRIx64 ")\n", > + 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: %#" PRIx64 ")\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..2c450f9 100644 > --- a/include/hw/i386/apic-msidef.h > +++ b/include/hw/i386/apic-msidef.h > @@ -26,6 +26,7 @@ > > #define MSI_ADDR_DEST_ID_SHIFT 12 > #define MSI_ADDR_DEST_IDX_SHIFT 4 > -#define MSI_ADDR_DEST_ID_MASK 0x00ffff0 > +#define MSI_ADDR_DEST_ID_MASK 0x000fff00 The value of MSI_ADDR_DEST_ID_MASK is changed here. I think the patch should be: +#define MSI_ADDR_DEST_ID_MASK 0x000ffff0 > +#define MSI_ADDR_IF_MASK 0x00000010 > > #endif /* HW_APIC_MSIDEF_H */ Thanks,
>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote: > On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote: >> --- a/include/hw/i386/apic-msidef.h >> +++ b/include/hw/i386/apic-msidef.h >> @@ -26,6 +26,7 @@ >> >> #define MSI_ADDR_DEST_ID_SHIFT 12 >> #define MSI_ADDR_DEST_IDX_SHIFT 4 >> -#define MSI_ADDR_DEST_ID_MASK 0x00ffff0 >> +#define MSI_ADDR_DEST_ID_MASK 0x000fff00 > > The value of MSI_ADDR_DEST_ID_MASK is changed here. I think the patch > should be: > +#define MSI_ADDR_DEST_ID_MASK 0x000ffff0 Judging from other sources, rather the other way around - the mask needs to have further bits removed (should be 0x000ff000 afaict). Xen sources confirm this, and while Linux has the value you suggest, that contradicts #define MSI_ADDR_DEST_ID_SHIFT 12 #define MSI_ADDR_DEST_ID(dest) (((dest) << MSI_ADDR_DEST_ID_SHIFT) & \ MSI_ADDR_DEST_ID_MASK) as well as #define MSI_ADDR_EXT_DEST_ID(dest) ((dest) & 0xffffff00) chopping off just the low 8 bits. Jan
On 2017年05月19日 20:04, Jan Beulich wrote: >>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote: >> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote: >>> --- a/include/hw/i386/apic-msidef.h >>> +++ b/include/hw/i386/apic-msidef.h >>> @@ -26,6 +26,7 @@ >>> >>> #define MSI_ADDR_DEST_ID_SHIFT 12 >>> #define MSI_ADDR_DEST_IDX_SHIFT 4 >>> -#define MSI_ADDR_DEST_ID_MASK 0x00ffff0 >>> +#define MSI_ADDR_DEST_ID_MASK 0x000fff00 >> The value of MSI_ADDR_DEST_ID_MASK is changed here. I think the patch >> should be: >> +#define MSI_ADDR_DEST_ID_MASK 0x000ffff0 > Judging from other sources, rather the other way around - the > mask needs to have further bits removed (should be 0x000ff000 > afaict). Xen sources confirm this, and while Linux has the value > you suggest, that contradicts Agree. Defining the mask as "0x000ff000" makes more sense. Just check Qemu source code. Only apic_send_msi() and msi_dest_id() use the mask to get dest apic id. They mask MSI address field with MSI_ADDR_DEST_ID_MASK and then right-shift 12bit. The low 12bit won't be used. Anthony, does this make sense?
On Tue, May 23, 2017 at 08:16:25PM +0800, Lan Tianyu wrote: > On 2017年05月19日 20:04, Jan Beulich wrote: > >>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote: > >> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote: > >>> --- a/include/hw/i386/apic-msidef.h > >>> +++ b/include/hw/i386/apic-msidef.h > >>> @@ -26,6 +26,7 @@ > >>> > >>> #define MSI_ADDR_DEST_ID_SHIFT 12 > >>> #define MSI_ADDR_DEST_IDX_SHIFT 4 > >>> -#define MSI_ADDR_DEST_ID_MASK 0x00ffff0 > >>> +#define MSI_ADDR_DEST_ID_MASK 0x000fff00 > >> The value of MSI_ADDR_DEST_ID_MASK is changed here. I think the patch > >> should be: > >> +#define MSI_ADDR_DEST_ID_MASK 0x000ffff0 > > Judging from other sources, rather the other way around - the > > mask needs to have further bits removed (should be 0x000ff000 > > afaict). Xen sources confirm this, and while Linux has the value > > you suggest, that contradicts > Agree. Defining the mask as "0x000ff000" makes more sense. > Just check Qemu source code. Only apic_send_msi() and msi_dest_id() use > the mask > to get dest apic id. They mask MSI address field with > MSI_ADDR_DEST_ID_MASK and > then right-shift 12bit. The low 12bit won't be used. > > Anthony, does this make sense? Yes, it does. The change to MSI_ADDR_DEST_ID_MASK should probably go in its own patch.
On 2017年05月24日 01:06, Anthony PERARD wrote: > On Tue, May 23, 2017 at 08:16:25PM +0800, Lan Tianyu wrote: >> On 2017年05月19日 20:04, Jan Beulich wrote: >>>>>> On 19.05.17 at 13:16, <anthony.perard@citrix.com> wrote: >>>> On Thu, May 18, 2017 at 01:32:59AM -0400, Lan Tianyu wrote: >>>>> --- a/include/hw/i386/apic-msidef.h >>>>> +++ b/include/hw/i386/apic-msidef.h >>>>> @@ -26,6 +26,7 @@ >>>>> >>>>> #define MSI_ADDR_DEST_ID_SHIFT 12 >>>>> #define MSI_ADDR_DEST_IDX_SHIFT 4 >>>>> -#define MSI_ADDR_DEST_ID_MASK 0x00ffff0 >>>>> +#define MSI_ADDR_DEST_ID_MASK 0x000fff00 >>>> The value of MSI_ADDR_DEST_ID_MASK is changed here. I think the patch >>>> should be: >>>> +#define MSI_ADDR_DEST_ID_MASK 0x000ffff0 >>> Judging from other sources, rather the other way around - the >>> mask needs to have further bits removed (should be 0x000ff000 >>> afaict). Xen sources confirm this, and while Linux has the value >>> you suggest, that contradicts >> Agree. Defining the mask as "0x000ff000" makes more sense. >> Just check Qemu source code. Only apic_send_msi() and msi_dest_id() use >> the mask >> to get dest apic id. They mask MSI address field with >> MSI_ADDR_DEST_ID_MASK and >> then right-shift 12bit. The low 12bit won't be used. >> >> Anthony, does this make sense? > Yes, it does. > The change to MSI_ADDR_DEST_ID_MASK should probably go in its own patch. > OK. Will update.
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c index 62add06..5fab95e 100644 --- a/hw/xen/xen_pt_msi.c +++ b/hw/xen/xen_pt_msi.c @@ -163,16 +163,24 @@ static int msi_msix_update(XenPCIPassthroughState *s, int rc = 0; uint64_t table_addr = 0; - XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x" - " (entry: %#x)\n", - is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry); - if (is_msix) { table_addr = s->msix->mmio_base_addr; } - rc = xc_domain_update_msi_irq(xen_xc, xen_domid, gvec, - pirq, gflags, table_addr); + if (addr & MSI_ADDR_IF_MASK) { + XEN_PT_LOG(d, "Updating MSI%s with addr %#" PRIx64 "data %#x\n", + is_msix ? "-X": "", addr, data); + rc = xc_domain_update_msi_irq_remapping(xen_xc, xen_domid, pirq, + d->devfn, data, addr, table_addr); + } + else { + XEN_PT_LOG(d, "Updating MSI%s with pirq %d gvec %#x gflags %#x" + " (entry: %#x)\n", + is_msix ? "-X" : "", pirq, gvec, gflags, msix_entry); + + 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 +212,29 @@ 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_IF_MASK ) { + XEN_PT_LOG(d, "Unbinding of MSI%s . ( pirq: %d, data: %x, " + "addr: %#" PRIx64 ")\n", + 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: %#" PRIx64 ")\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..2c450f9 100644 --- a/include/hw/i386/apic-msidef.h +++ b/include/hw/i386/apic-msidef.h @@ -26,6 +26,7 @@ #define MSI_ADDR_DEST_ID_SHIFT 12 #define MSI_ADDR_DEST_IDX_SHIFT 4 -#define MSI_ADDR_DEST_ID_MASK 0x00ffff0 +#define MSI_ADDR_DEST_ID_MASK 0x000fff00 +#define MSI_ADDR_IF_MASK 0x00000010 #endif /* HW_APIC_MSIDEF_H */