Re: [Xen-devel] [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

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

On Thu, Nov 15, 2018 at 08:34:44AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -993,6 +993,16 @@ static void * __init allocate_ppr_log(struct amd_iommu 
> > *iommu)
> >  
> >  static int __init amd_iommu_init_one(struct amd_iommu *iommu)
> >  {
> > +    struct pci_dev *pdev;
> > +
> > +    pcidevs_lock();
> > +    pdev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
> > +                        PCI_DEVFN2(iommu->bdf));
> > +    if ( pdev )
> > +        /* Assign the IOMMU PCI device to Xen  */
> > +        pdev->domain = dom_xen;
> > +    pcidevs_unlock();
> 
> Why do you kind of open-code pci_hide_device()? It would need
> extending to cope with a non-zero segment number, but I'd much
> prefer if there could be one central place where the logic lives.
> That way list addition would also not be omitted, like you do.

Sure, expanding pci_hide_device is better, I just didn't realize this
function existed in the first place.

> As to the hiding in general, also considering Andrew's objection:
> Are these devices representing the IOMMU and nothing else? As
> mentioned by Andrew something similar would be needed on the
> VT-d side, but iirc there's less clear of a relationship there in any
> event (which causes me to wonder about the situation on the
> AMD side). I'm asking not the least because iirc at the time
> pci_hide_device() was introduced I think it was considered to
> hide the AMD IOMMU devices; I don't recall why we didn't in the
> end, though.

I think this would be easier if I give more context about the issue
I'm hitting here, which is very similar to the issue patch 5/5
attempts to address.

The problem is that the IOMMU PCI device itself is obviously not
behind an IOMMU, and update_paging_mode will return an error if it
finds any such device in the domain list when attempting to expand the
IOMMU page tables:

...
iommu = find_iommu_for_device(pdev->seg, bdf);
if ( !iommu )
{
    AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
    return -ENODEV;
}
...

Another option is to allow the hardware domain to have assigned
devices that a not behind an IOMMU, but I would consider this more
like a workaround rather than a real fix.

I'm not sure whether the IOMMU AMD PCI device could also represent
something else, Maybe Brian can provide more insight on whether there
might be other platform devices encompassed in the same PCI device as
the IOMMU.

Thanks, Roger.

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