Re: [RFC PATCH] iommu/xen: Add Xen PV-IOMMU driver

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Hello Jason,

Le 19/06/2024 à 18:30, Jason Gunthorpe a écrit :
> On Thu, Jun 13, 2024 at 01:50:22PM +0000, Teddy Astie wrote:
>
>> +struct iommu_domain *xen_iommu_domain_alloc(unsigned type)
>> +{
>> +    struct xen_iommu_domain *domain;
>> +    u16 ctx_no;
>> +    int ret;
>> +
>> +    if (type & IOMMU_DOMAIN_IDENTITY) {
>> +            /* use default domain */
>> +            ctx_no = 0;
>
> Please use the new ops, domain_alloc_paging and the static identity domain.

Yes, in the v2, I will use this newer interface.

I have a question on this new interface : is it valid to not have a
identity domain (and "default domain" being blocking); well in the
current implementation it doesn't really matter, but at some point, we
may want to allow not having it (thus making this driver mandatory).

>
>> +static struct iommu_group *xen_iommu_device_group(struct device *dev)
>> +{
>> +    if (!dev_is_pci(dev))
>> +            return ERR_PTR(-ENODEV);
>> +
>
> device_group is only called after probe_device, since you already
> exclude !pci during probe there is no need for this wrapper, just set
> the op directly to pci_device_group.
>
>> +    if (!dev_is_pci(dev))
>> +            return;
>
> No op is ever called on a non-probed device, remove all these checks.
>
>
> A paging domain should be the only domain ops that have a populated
> map so this should be made impossible by construction
Makes sense, will remove these redundant checks in v2.

>
>> +static void xen_iommu_release_device(struct device *dev)
>> +{
>> +    int ret;
>> +    struct pci_dev *pdev;
>> +    struct pv_iommu_op op = {
>> +            .subop_id = IOMMUOP_reattach_device,
>> +            .flags = 0,
>> +            .ctx_no = 0 /* reattach device back to default context */
>> +    };
>
> Consider if you can use release_domain for this, I think this is
> probably a BLOCKED domain behavior.

The goal is to put back all devices where they were at the beginning
(the default "context"), which is what release_domain looks like it is
doing. Will use it for v2.

>
> Jason

Teddy


Teddy Astie | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech