Re: [Xen-devel] [PATCH v4 7/9] vpci/msi: add MSI handlers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 02:21:33AM -0600, Jan Beulich wrote:
> >>> On 08.08.17 at 17:44, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, Aug 02, 2017 at 07:34:28AM -0600, Jan Beulich wrote:
> >> >>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/30/17 5:01 PM >>>
> >> >+    /* Get a PIRQ. */
> >> >+    rc = allocate_and_map_msi_pirq(pdev->domain, -1, &arch->pirq,
> >> >+                                   MAP_PIRQ_TYPE_MULTI_MSI, &msi_info);
> >> >+    if ( rc )
> >> >+    {
> >> >+        dprintk(XENLOG_ERR, "%04x:%02x:%02x.%u: failed to map PIRQ: 
> >> >%d\n",
> >> >+                pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> >> >+                PCI_FUNC(pdev->devfn), rc);
> >> >+        return rc;
> >> >+    }
> >> >+
> >> >+    for ( i = 0; i < vectors; i++ )
> >> >+    {
> >> >+        xen_domctl_bind_pt_irq_t bind = {
> >> >+            .machine_irq = arch->pirq + i,
> >> >+            .irq_type = PT_IRQ_TYPE_MSI,
> >> >+            .u.msi.gvec = msi_vector(data) + i,
> >> >+            .u.msi.gflags = msi_flags(data, address),
> >> >+        };
> >> >+
> >> >+        pcidevs_lock();
> >> >+        rc = pt_irq_create_bind(pdev->domain, &bind);
> >> >+        if ( rc )
> >> >+        {
> >> >+            dprintk(XENLOG_ERR,
> >> >+                    "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n",
> >> >+                    pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> >> >+                    PCI_FUNC(pdev->devfn), arch->pirq + i, rc);
> >> >+            spin_lock(&pdev->domain->event_lock);
> >> >+            unmap_domain_pirq(pdev->domain, arch->pirq);
> >> 
> >> Don't you also need to undo the pt_irq_create_bind() calls here for all 
> >> prior
> >> successful iterations?
> > 
> > Yes, unmap_domain_pirq calls pirq_guest_force_unbind but better not
> > resort to that.
> 
> I don't understand.

I've added a calls to pt_irq_destroy_bind before calling
unmap_domain_pirq.

> >> >+int vpci_msi_arch_disable(struct vpci_arch_msi *arch, struct pci_dev 
> >> >*pdev,
> >> >+                          unsigned int vectors)
> >> >+{
> >> >+    unsigned int i;
> >> >+
> >> >+    ASSERT(arch->pirq != -1);
> >> >+
> >> >+    for ( i = 0; i < vectors; i++ )
> >> >+    {
> >> >+        xen_domctl_bind_pt_irq_t bind = {
> >> >+            .machine_irq = arch->pirq + i,
> >> >+            .irq_type = PT_IRQ_TYPE_MSI,
> >> >+        };
> >> >+
> >> >+        pcidevs_lock();
> >> >+        pt_irq_destroy_bind(pdev->domain, &bind);
> >> 
> >> While I agree that the loop should continue of this fails, I'm not 
> >> convinced
> >> you should entirely ignore the return value here.
> > 
> > I've added a printk in order to aid debug.
> 
> I've actually tried to hint at you wanting to run the loop to
> completion while returning to the caller the first error you've
> encountered.

Hm, I'm not sure of the best way to proceed here.

If vpci_msi_arch_disable returns once one of the pt_irq_destroy_bind
calls fail, further calls to vpci_msi_arch_disable are also likely to
fail if the previous call managed to destroy some of the bindings but
not all of them.

But then trying to call unmap_domain_pirq without having destroyed all
of the bindings seems likely to fail anyway...

Thanks, Roger.

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