Re: [Xen-devel] [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group...

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 08, 2019 at 02:24:03PM +0100, Paul Durrant wrote:
> ... using the new iommu_group infrastructure.
> 
> Because 'sibling' devices are now members of the same iommu_group,
> implement the domctl by looking up the relevant iommu_group and walking
> the membership list.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/drivers/passthrough/iommu.c | 65 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c   | 47 -----------------------------
>  xen/include/xen/iommu.h         |  2 ++
>  3 files changed, 67 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 11319fbaae..49140c652e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -729,6 +729,71 @@ int iommu_group_assign(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static struct iommu_group *iommu_group_lookup(uint16_t seg, uint8_t bus,
> +                                              uint8_t devfn)

Could you use pci_sbdf_t to pass the SBDF?

> +{
> +    unsigned int id = 0;
> +    struct iommu_group *grp;
> +
> +    while ( radix_tree_gang_lookup(&iommu_groups, (void **)&grp, id, 1) )
> +    {
> +        struct pci_dev *pdev;
> +
> +        list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> +            if ( pdev->seg == seg && pdev->bus == bus &&
> +                 pdev->devfn == devfn )
> +                return grp;
> +
> +        id = grp->id + 1;
> +    }
> +
> +    return NULL;
> +}
> +
> +int iommu_get_device_group(struct domain *d, u16 seg, u8 bus, u8 devfn,

Using pci_sbdf_t would be better here to pass the SBDF IMO, or
uint<size>_t, or just plain unsigned int.

Also, I wonder about the usefulness of the domain parameter, shouldn't
you do the ownership check somewhere else (if required) and have this
function just check the IOMMU group of a given PCI  device?

(Note you probably want to constify the domain parameter if it needs to
stay).

> +                           XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
> +{
> +    struct iommu_group *grp;
> +    struct pci_dev *pdev;
> +    int i = 0;

It seems like this should be unsigned int?

> +
> +    pcidevs_lock();
> +
> +    grp = iommu_group_lookup(seg, bus, devfn);
> +    if ( !grp )
> +    {
> +        pcidevs_unlock();
> +        return 0;
> +    }
> +
> +    list_for_each_entry ( pdev, &grp->devs_list, grpdevs_list )
> +    {
> +        uint32_t sbdf;
> +
> +        if ( i >= max_sdevs )
> +            break;
> +
> +        if ( pdev->domain != d )
> +            continue;
> +
> +        sbdf = PCI_SBDF3(pdev->seg, pdev->bus, pdev->devfn);
> +
> +        if ( xsm_get_device_group(XSM_HOOK, sbdf) )
> +            continue;
> +
> +        if ( unlikely(copy_to_guest_offset(buf, i, &sbdf, 1)) )
> +        {
> +            pcidevs_unlock();
> +            return -1;

-EFAULT?

> +        }
> +        i++;
> +    }
> +
> +    pcidevs_unlock();
> +
> +    return i;
> +}
> +
>  #endif /* CONFIG_HAS_PCI */
>  
>  /*
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 6210409741..68b2883ed6 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1554,53 +1554,6 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, 
> u8 devfn)
>      return ret;
>  }
>  
> -static int iommu_get_device_group(
> -    struct domain *d, u16 seg, u8 bus, u8 devfn,
> -    XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)

Oh, I see this is code movement and changes to an existing function,
hence my comments above might be stale, or will require to fixup the
callers which might be cumbersome.

Thanks, Roger.

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