Re: [PATCH RFC] pci/ats: do not allow broken devices to be assigned

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2022 13:43, Roger Pau Monne wrote:
> Introduce a new field to mark devices as broken: having it set
> prevents the device from being assigned to domains. Use the field in
> order to mark ATS devices that have failed a flush as broken, thus
> preventing them to be assigned to any guest.
> 
> This allows the device IOMMU context entry to be cleaned up properly,
> as calling _pci_hide_device will just change the ownership of the
> device, but the IOMMU context entry of the device would be left as-is.
> It would also leak a Domain ID, as removing the device from it's
> previous owner will allow releasing the DID used by the device without
> having cleaned up the context entry.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> RFC: I haven't tested the code path, as I have no ATS devices on the
> box I'm currently testing on. In any case, ATS is not supported, and
> removing the call to _pci_hide_device in iommu_dev_iotlb_flush_timeout
> should allow to remove the dependency on recursive pcidevs lock.

No objection in principle. Whether this is the only dependency on
recursive pcidevs lock isn't really know though, is it?

> TBD: it's unclear whether we still need the pcidevs_lock in
> iommu_dev_iotlb_flush_timeout. The caller of
> iommu_dev_iotlb_flush_timeout is already bogus as it iterates over a
> list of pdevs without holding the pcidevs_lock.

Analysis of whether / where recursive uses are needed should imo
include cases where the lock ought to be held, but currently isn't
(like apparently this case).

> @@ -1487,6 +1487,11 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>      ASSERT(pdev && (pdev->domain == hardware_domain ||
>                      pdev->domain == dom_io));
>  
> +    /* Do not allow broken devices to be assigned. */
> +    rc = -EBADF;
> +    if ( pdev->broken )
> +        goto done;

I think this wants exceptions for Dom0 and DomIO. An admin may be
able to fix things in Dom0, e.g. by updating device firmware.

Jan