| Message ID | 20181203050438.6034-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen/pt: Fix a xen passthrough failure | expand |
Hello, Thanks for the patch. The subject should be more descriptive, "Fix a xen passthrough failure" is too generic. How about: "allow passthrough of devices with bogus interrupt pin" or something similar. On Mon, Dec 03, 2018 at 12:04:38AM -0500, Zhao Yan wrote: > For some pci device, even its PCI_INTERRUPT_PIN is not 0, it actually > doesn't support INTx mode, so its machine irq read from host sysfs is 0. > In that case, report PCI_INTERRUPT_PIN as 0 to guest and let passthrough > continue. > > v2: fix some coding style issue The changelog between versions should be below the '---'. > > Cc: Roger Pau Monné <roger.pau@citrix.com> > Cc: Jan Beulich <JBeulich@suse.com> > Signed-off-by: Zhao Yan <yan.y.zhao@intel.com> > --- > hw/xen/xen_pt.c | 5 +++++ > hw/xen/xen_pt_config_init.c | 8 +++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index f1f3a3727c..d601c9979c 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -847,6 +847,11 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) > } > > machine_irq = s->real_device.irq; > + if (machine_irq == 0) { > + XEN_PT_LOG(d, "machine irq is 0\n"); I would maybe consider disabling INTX assertion here on the command register. > + goto out; > + } > + > rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); > if (rc < 0) { > error_setg_errno(errp, errno, "Mapping machine irq %u to" > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c > index 47f9010c75..1007b6c977 100644 > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -300,7 +300,13 @@ static int xen_pt_irqpin_reg_init(XenPCIPassthroughState *s, > XenPTRegInfo *reg, uint32_t real_offset, > uint32_t *data) > { > - *data = xen_pt_pci_read_intx(s); > + if (s->real_device.irq) > + *data = xen_pt_pci_read_intx(s); > + else { > + XEN_PT_LOG(&s->dev, > + "machine irq is 0, init guest PCI_INTERRUPT_PIN to 0\n"); > + *data = 0; The default value for the register is already zero, so you could drop the else branch AFAICT. Thanks, Roger.
On 03/12/2018 05:04, Zhao Yan wrote: > For some pci device, even its PCI_INTERRUPT_PIN is not 0, it actually > doesn't support INTx mode, so its machine irq read from host sysfs is 0. > In that case, report PCI_INTERRUPT_PIN as 0 to guest and let passthrough > continue. What causes this problem? It it a non-PCI compliant device? Is it a device which has legacy lines prohibited? ~Andrew
On Mon, Dec 03, 2018 at 12:20:30PM +0100, Roger Pau Monné wrote: > Hello, > > Thanks for the patch. > > The subject should be more descriptive, "Fix a xen passthrough > failure" is too generic. How about: "allow passthrough of devices with > bogus interrupt pin" or something similar. right, thanks for your suggestion. I'll change the subject to this one. > > On Mon, Dec 03, 2018 at 12:04:38AM -0500, Zhao Yan wrote: > > For some pci device, even its PCI_INTERRUPT_PIN is not 0, it actually > > doesn't support INTx mode, so its machine irq read from host sysfs is 0. > > In that case, report PCI_INTERRUPT_PIN as 0 to guest and let passthrough > > continue. > > > > v2: fix some coding style issue > > The changelog between versions should be below the '---'. > got it. thanks :) > > > > Cc: Roger Pau Monné <roger.pau@citrix.com> > > Cc: Jan Beulich <JBeulich@suse.com> > > Signed-off-by: Zhao Yan <yan.y.zhao@intel.com> > > --- > > hw/xen/xen_pt.c | 5 +++++ > > hw/xen/xen_pt_config_init.c | 8 +++++++- > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > > index f1f3a3727c..d601c9979c 100644 > > --- a/hw/xen/xen_pt.c > > +++ b/hw/xen/xen_pt.c > > @@ -847,6 +847,11 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) > > } > > > > machine_irq = s->real_device.irq; > > + if (machine_irq == 0) { > > + XEN_PT_LOG(d, "machine irq is 0\n"); > > I would maybe consider disabling INTX assertion here on the command > register. > ok, I'll add that. Actually I was vacillating on whether to add the disabling of INTx assertion, as I thought PCI_INTERRUPT_PIN was already reported 0. But adding the disabling is better. > > + goto out; > > + } > > + > > rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); > > if (rc < 0) { > > error_setg_errno(errp, errno, "Mapping machine irq %u to" > > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c > > index 47f9010c75..1007b6c977 100644 > > --- a/hw/xen/xen_pt_config_init.c > > +++ b/hw/xen/xen_pt_config_init.c > > @@ -300,7 +300,13 @@ static int xen_pt_irqpin_reg_init(XenPCIPassthroughState *s, > > XenPTRegInfo *reg, uint32_t real_offset, > > uint32_t *data) > > { > > - *data = xen_pt_pci_read_intx(s); > > + if (s->real_device.irq) > > + *data = xen_pt_pci_read_intx(s); > > + else { > > + XEN_PT_LOG(&s->dev, > > + "machine irq is 0, init guest PCI_INTERRUPT_PIN to 0\n"); > > + *data = 0; > > The default value for the register is already zero, so you could drop > the else branch AFAICT. ok, I'll drop the else branch. thanks for your review :) > Thanks, Roger.
hi Andrew, It's a pci device that does not support legacy intx mode, but it accidently reports PCI_INTERRUPT_PIN as 1, which should be 0 instead. So, in dom0, the machine irq is 0, which will cause later xc_physdev_map_pirq() fail and passthrough failure. Therefore, we treat this case as PCI_INTERRUPT_PIN is 0 and report to guest the right value (0) of PCI_INTERRUPT_PIN. Then in guest, it's able to use msi mode and function normally. On Mon, Dec 03, 2018 at 02:12:58PM +0000, Andrew Cooper wrote: > On 03/12/2018 05:04, Zhao Yan wrote: > > For some pci device, even its PCI_INTERRUPT_PIN is not 0, it actually > > doesn't support INTx mode, so its machine irq read from host sysfs is 0. > > In that case, report PCI_INTERRUPT_PIN as 0 to guest and let passthrough > > continue. > > What causes this problem? It it a non-PCI compliant device? Is it a > device which has legacy lines prohibited? > > ~Andrew
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index f1f3a3727c..d601c9979c 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -847,6 +847,11 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) } machine_irq = s->real_device.irq; + if (machine_irq == 0) { + XEN_PT_LOG(d, "machine irq is 0\n"); + goto out; + } + rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq); if (rc < 0) { error_setg_errno(errp, errno, "Mapping machine irq %u to" diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c index 47f9010c75..1007b6c977 100644 --- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -300,7 +300,13 @@ static int xen_pt_irqpin_reg_init(XenPCIPassthroughState *s, XenPTRegInfo *reg, uint32_t real_offset, uint32_t *data) { - *data = xen_pt_pci_read_intx(s); + if (s->real_device.irq) + *data = xen_pt_pci_read_intx(s); + else { + XEN_PT_LOG(&s->dev, + "machine irq is 0, init guest PCI_INTERRUPT_PIN to 0\n"); + *data = 0; + } return 0; }
For some pci device, even its PCI_INTERRUPT_PIN is not 0, it actually doesn't support INTx mode, so its machine irq read from host sysfs is 0. In that case, report PCI_INTERRUPT_PIN as 0 to guest and let passthrough continue. v2: fix some coding style issue Cc: Roger Pau Monné <roger.pau@citrix.com> Cc: Jan Beulich <JBeulich@suse.com> Signed-off-by: Zhao Yan <yan.y.zhao@intel.com> --- hw/xen/xen_pt.c | 5 +++++ hw/xen/xen_pt_config_init.c | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-)