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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 15 May 2019 10:07
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 5/5] iommu / pci: re-implement XEN_DOMCTL_get_device_group...
> 
> 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@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > ---
> >  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?
> 

Probably, I'd not noticed its existence so I'll use it when I can.

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

Yes and no. This is the implementation of an existing domctl so it's semantics are baked in. I think I can use pci_sbdf_t but the domain parameter 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?
> 

Yes, I guess it could be.

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

Yes... then I can get rid of the override of the ret value in the calling code.

  Paul

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