Re: [PATCH 2/2] iommu: add warning when sharing groups

[okaya-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

On 2017-02-14 18:51, Alex Williamson wrote:
> On Tue, 14 Feb 2017 16:25:22 -0500
> Sinan Kaya <okaya-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 
>> The ACS requirement has been obscured in the current code and is only
>> known by certain individuals who happen to read the code. Print out a
>> warning with ACS path failure when ACS requirement is not met.
>> 
>> Signed-off-by: Sinan Kaya <okaya-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>  drivers/iommu/iommu.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index dbe7f65..049ee0a 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -811,6 +811,9 @@ struct iommu_group *pci_device_group(struct device 
>> *dev)
>>  	if (IS_ERR(group))
>>  		return NULL;
>> 
>> +	if (pci_is_root_bus(bus))
>> +		dev_warn_once(&pdev->dev, "using shared group due to ACS path 
>> failure\n");
>> +
>>  	return group;
>>  }
>> 
> 
> The premise here is flawed.  An IOMMU group based at the root bus
> doesn't necessarily imply a lack of ACS.  There are devices on root
> buses, integrated endpoints and root ports.  Naturally an IOMMU group
> for these devices needs to be based at the root bus.  Additionally,
> there can be IOMMU groups developed around a lack of ACS that don't
> intersect with the root bus.  Since this is a warn_once, the false
> positives for root bus devices are going to be enumerated first.  On an
> Intel system there's typically a device as 00.0 that will always be
> pointlessly listed first.  Also, it's not clear that grouping devices
> together is always wrong, as Robin pointed out in the EHCI/OHCI
> example.  Lack of ACS on downtream ports is likely to cause problems,
> especially if that downstream port exposes a slot.  Maybe that would be
> a good place to start.  Also, what is someone supposed to do when they
> see this error?  If we can hope they'll look for the error in the code
> (unlikely) a big comment with useful external links might be
> necessary.  Based on how easily vendors ignore kernel warnings, I'm
> dubious there's any value to this path though.  Thanks,

Maybe, a better solution would be to add some sentences into vfio.txt 
documentation.

I'm ready to drop this patch. I just don't want ACS requirement to be 
hidden between the source code.

Would you be willing to do that?

I know I read all pci and vfio documents in the past. I could have 
captured this requirement if it was there.

> 
> Alex