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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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.

Jan



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