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

[Chao Gao <chao.gao@xxxxxxxxx>]

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()

>
>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.

I will do some tests for PV guest.

Thanks
Chao

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