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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.12.18 at 16:21, <chao.gao@xxxxxxxxx> wrote:
> On Fri, Dec 21, 2018 at 03:13:50AM -0700, Jan Beulich wrote:
>>>>> On 20.12.18 at 16:29, <chao.gao@xxxxxxxxx> wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1514,6 +1514,55 @@ 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;
>>> +    struct domain *d = pdev->domain;
>>> +
>>> +    ASSERT(pcidevs_locked());
>>> +
>>> +    if ( !d )
>>> +        return;
>>
>>Why? deassign_device() (the only caller) ought to guarantee this,
>>due to its use of pci_get_pdev_by_domain(). I think this simply
>>wants to be another ASSERT(), if anything at all.
>>
>>> +    spin_lock(&d->event_lock);
>>> +    while ( (entry = list_first_entry_or_null(&pdev->msi_list,
>>> +                                              struct msi_desc, list)) != 
> NULL )
>>> +    {
>>> +        struct pirq *info;
>>> +        int pirq = 0;
>>> +        unsigned int nr = entry->msi_attrib.type != PCI_CAP_ID_MSIX
>>> +                          ? entry->msi.nvec : 1;
>>> +
>>> +        while ( nr -- )
>>
>>Stray blank.
>>
>>> +        {
>>> +            struct hvm_pirq_dpci *pirq_dpci;
>>> +
>>> +            pirq = domain_irq_to_pirq(d, entry[nr].irq);
>>> +            WARN_ON(pirq < 0);
>>> +            if ( pirq <= 0 )
>>> +                continue;
>>> +
>>> +            info = pirq_info(d, pirq);
>>> +            if ( !info )
>>> +                continue;
>>> +
>>> +            pirq_dpci = pirq_dpci(info);
>>> +            if ( pirq_dpci &&
>>> +                 (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) &&
>>> +                 (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) )
>>> +                pt_irq_destroy_bind_msi(d, info);
>>> +        }
>>> +        if ( pirq > 0 )
>>> +            unmap_domain_pirq(d, pirq);
>>
>>Can you guarantee that this function won't fail? Because if it
>>does, I think you might end up in an infinite loop, because the
> 
> Considering current code doesn't deal with remaining pirq, if we
> failed to unmap some pirq here (remove all entries from the msi_list
> here), it wouldn't be a big issue. Hence the real issue here is a
> potential infinite loop. Then we can just use
> list_for_each_entry_safe(...) to traverse msi_list to avoid infinite
> loop.
> 
>>entry doesn't always get removed from the list in error cases.
>>Maybe unmap_domain_pirq() needs a "force" mode added,
>>perhaps indirectly by way of passing "entry" into it (all other
>>callers would pass NULL).
> 
> Yes, it is viable. However, for this call site, unmap_domain_pirq()
> would fail to remove an entry only if xsm_unmap_domain_irq() in
> unmap_domain_pirq() failed. Can we expect that xsm_unmap_domain_irq()
> would always succeed there?

It would probably be a buggy policy, but we shouldn't crash/hang
the hypervisor in such a case.

> If the answer is yes, what needed is
> another assertion rather than the "force" mode. Maybe we can
> forcibly remove all entries still on the list after the loop.

That's indeed a possible option.

Jan



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