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

[Chao Gao <chao.gao@xxxxxxxxx>]

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? 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.
The benefit is we needn't change unmap_domain_pirq() and its
existing call sites.

>
>But then again I'm still not fully convinced that a hypervisor
>change is the right course of action here in the first place. It
>would be better if the hypervisor had to just verify that all
>IRQ mappings are gone, or else fail the de-assignment of the
>device.

Then in another place, we need the "force" mode. I don't think
it would bring great benefit if there is no other case (except device
hot-remove and guest shutdown) where we want to unmap all pirqs
related to a device.  

Thanks
Chao


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