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 10:52:48PM +0800, Chao Gao wrote:
> On Wed, Jan 16, 2019 at 01:34:28PM +0100, Roger Pau Monné wrote:
> >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.
> 
> A valid call trace (in this patch's description) is:
> 
> ->unmap_domain_pirq (if pirq isn't unmapped by qemu)
>     ->pirq_guest_force_unbind
>       ->__pirq_guest_unbind
>           ->mask_msi_irq(=desc->handler->disable())
>               ->the warning in msi_set_mask_bit()

Ack, sorry for not realizing this.

Thanks, Roger.

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