Re: [PATCH 3/4] iommu/vt-d: Handle RMRR with PCI bridge device scopes

[Auger Eric <eric.auger@redhat.com>]

Hi Jacob,

On 5/13/19 6:41 PM, Jacob Pan wrote:
> On Mon, 13 May 2019 09:13:01 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> When reading the vtd specification and especially the
>> Reserved Memory Region Reporting Structure chapter,
>> it is not obvious a device scope element cannot be a
>> PCI-PCI bridge, in which case all downstream ports are
>> likely to access the reserved memory region. Let's handle
>> this case in device_has_rmrr.
>>
>> Fixes: ea2447f700ca ("intel-iommu: Prevent devices with RMRRs from
>> being placed into SI Domain")
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  drivers/iommu/intel-iommu.c | 32 +++++++++++++++++++++++---------
>>  1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index e2134b13c9ae..89d82a1d50b1 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -736,12 +736,31 @@ static int iommu_dummy(struct device *dev)
>>  	return dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>>  }
>>  
>> +static bool
>> +is_downstream_to_pci_bridge(struct device *deva, struct device *devb)
>> +{
>> +	struct pci_dev *pdeva, *pdevb;
>> +
> is there a more illustrative name for these. i guess deva is is the
> bridge dev?
deva is the candidate PCI device likely to belong to the PCI
sub-hierarchy of devb (the candidate bridge).

My concern about the naming was that they are not necessarily a pci
device or a bridge. But at least I can add a comment or rename according
to your suggestion ;-)


>> +	if (!dev_is_pci(deva) || !dev_is_pci(devb))
>> +		return false;
>> +
>> +	pdeva = to_pci_dev(deva);
>> +	pdevb = to_pci_dev(devb);
>> +
>> +	if (pdevb->subordinate &&
>> +	    pdevb->subordinate->number <= pdeva->bus->number &&
>> +	    pdevb->subordinate->busn_res.end >= pdeva->bus->number)
>> +		return true;
>> +
>> +	return false;
>> +>> +
> this seems to be a separate cleanup patch.
I can split into 2 patches: introduction of this helper and its usage in
device_to_iommu and second patch using it in iommu_has_rmrr.

>>  static struct intel_iommu *device_to_iommu(struct device *dev, u8
>> *bus, u8 *devfn) {
>>  	struct dmar_drhd_unit *drhd = NULL;
>>  	struct intel_iommu *iommu;
>>  	struct device *tmp;
>> -	struct pci_dev *ptmp, *pdev = NULL;
>> +	struct pci_dev *pdev = NULL;
>>  	u16 segment = 0;
>>  	int i;
>>  
>> @@ -787,13 +806,7 @@ static struct intel_iommu
>> *device_to_iommu(struct device *dev, u8 *bus, u8 *devf goto out;
>>  			}
>>  
>> -			if (!pdev || !dev_is_pci(tmp))
>> -				continue;
>> -
>> -			ptmp = to_pci_dev(tmp);
>> -			if (ptmp->subordinate &&
>> -			    ptmp->subordinate->number <=
>> pdev->bus->number &&
>> -			    ptmp->subordinate->busn_res.end >=
>> pdev->bus->number)
>> +			if (is_downstream_to_pci_bridge(dev, tmp))
>>  				goto got_pdev;
>>  		}
>>  
>> @@ -2886,7 +2899,8 @@ static bool device_has_rmrr(struct device *dev)
>>  		 */
>>  		for_each_active_dev_scope(rmrr->devices,
>>  					  rmrr->devices_cnt, i, tmp)
>> -			if (tmp == dev) {
>> +			if (tmp == dev ||
>> +			    is_downstream_to_pci_bridge(dev, tmp)) {
>>  				rcu_read_unlock();
>>  				return true;
>>  			}
> 
> [Jacob Pan]
> 
Thanks

Eric