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

[Jason Gunthorpe <jgg@xxxxxxxx>]

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.

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

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

> +     if (!dev_is_pci(dev))
> +             return;

No op is ever called on a non-probed device, remove all these checks.

> +static int xen_iommu_map_pages(struct iommu_domain *domain, unsigned long 
> iova,
> +                                                        phys_addr_t paddr, 
> size_t pgsize, size_t pgcount,
> +                                                        int prot, gfp_t gfp, 
> size_t *mapped)
> +{
> +     size_t xen_pg_count = (pgsize / XEN_PAGE_SIZE) * pgcount;
> +     struct xen_iommu_domain *dom = to_xen_iommu_domain(domain);
> +     struct pv_iommu_op op = {
> +             .subop_id = IOMMUOP_map_pages,
> +             .flags = 0,
> +             .ctx_no = dom->ctx_no
> +     };
> +     /* NOTE: paddr is actually bound to pfn, not gfn */
> +     uint64_t pfn = addr_to_pfn(paddr);
> +     uint64_t dfn = addr_to_pfn(iova);
> +     int ret = 0;
> +
> +     if (WARN(!dom->ctx_no, "Tried to map page to default context"))
> +             return -EINVAL;

A paging domain should be the only domain ops that have a populated
map so this should be made impossible by construction.

Jason