Re: [Xen-devel] [PATCH v5 1/3] xen/pt: fix some pass-thru devices don't work across reboot

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

On Wed, Jan 16, 2019 at 07:59:44PM +0800, Chao Gao wrote:
> On Wed, Jan 16, 2019 at 11:38:23AM +0100, Roger Pau Monné wrote:
> >On Wed, Jan 16, 2019 at 04:17:30PM +0800, Chao Gao wrote:
> >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> >> index 93c20b9..4f2be02 100644
> >> --- a/xen/drivers/passthrough/pci.c
> >> +++ b/xen/drivers/passthrough/pci.c
> >> @@ -1514,6 +1514,68 @@ static int assign_device(struct domain *d, u16 seg, 
> >> u8 bus, u8 devfn, u32 flag)
> >>      return rc;
> >>  }
> >>  
> >> +/*
> >> + * Unmap established mappings between domain's pirq and device's MSI.
> >> + * These mappings were set up by qemu/guest and are expected to be
> >> + * destroyed when changing the device's ownership.
> >> + */
> >> +static void pci_unmap_msi(struct pci_dev *pdev)
> >> +{
> >> +    struct msi_desc *entry, *tmp;
> >> +    struct domain *d = pdev->domain;
> >> +
> >> +    ASSERT(pcidevs_locked());
> >> +    ASSERT(d);
> >> +
> >> +    spin_lock(&d->event_lock);
> >> +    list_for_each_entry_safe(entry, tmp, &pdev->msi_list, list)
> >> +    {
> >> +        struct pirq *info;
> >> +        int ret, pirq = 0;
> >> +        unsigned int nr = entry->msi_attrib.type != PCI_CAP_ID_MSIX
> >> +                          ? entry->msi.nvec : 1;
> >
> >I think you should mask the entry, like it's done in
> >pt_irq_destroy_bind, see the call to guest_mask_msi_irq. That gives a
> >consistent state between bind and unbind.
> 
> I don't think it is necessary considering that we are to unmap pirq.
> The reason of keeping state consistent is that we might try to bind
> the same pirq to another guest interrupt.

Even taking into account that the pirq will be unmapped afterwards I'm
not sure the state is going to be the same. unmap_domain_pirq doesn't
seem to mask the MSI entries, and so I wonder whether we could run
into issues (state not being the expected) when later re-assigning the
device to another guest.

Maybe I'm missing something, but I would like to make sure the device
state stays consistent between assignations, at the end of day the
problem this patch aims to solve is a state inconsistency between
device assignations.

> >> +        }
> >> +    }
> >> +    /*
> >> +     * All pirq-s should have been unmapped and corresponding msi_desc
> >> +     * entries should have been removed in the above loop.
> >> +     */
> >> +    ASSERT(list_empty(&pdev->msi_list));
> >> +
> >> +    spin_unlock(&d->event_lock);
> >> +}
> >> +
> >>  /* caller should hold the pcidevs_lock */
> >>  int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
> >>  {
> >> @@ -1529,6 +1591,8 @@ int deassign_device(struct domain *d, u16 seg, u8 
> >> bus, u8 devfn)
> >>      if ( !pdev )
> >>          return -ENODEV;
> >>  
> >> +    pci_unmap_msi(pdev);
> >
> >Just want to make sure, since deassign_device will be called for both
> >PV and HVM domains. AFAICT pci_unmap_msi is safe to call when the
> >device is assigned to a PV guest, but would like your confirmation.
> 
> TBH, I don't know how device pass-thru is implemented for PV guest.
> If PV guest also uses the same structures and APIs to manage the mapping
> between msi, pirq and guest interrupt, I think pci_unmap_msi() should also
> work for PV guest case.

No, PV guest uses a completely different mechanism. I think
pci_unmap_msi is safe to be used against PV guests, but it would be
nice to have some confirmation. The more that there are no
pci-passthorugh tests in osstest, so such error would go unnoticed.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel