Re: [PATCH 1/1] IOMMU: Save pci device id instead of pci_dev* pointerfor DMAR devices

[Yijing Wang]

HI Bjorn,
   Thanks for your review and comments very much!

>> +		list_for_each_entry(dmar_dev, head, list)
>> +		    if (dmar_dev->segment == pci_domain_nr(dev->bus)
>> +			    && dmar_dev->bus == dev->bus->number
>> +			    && dmar_dev->devfn == dev->devfn)
>> +			return 1;
>> +		
>>  		/* Check our parent */
>>  		dev = dev->bus->self;
> 
> You didn't change this, but it looks like this may have the same problem
> we've been talking about here:
> 
> http://lkml.kernel.org/r/20131105232903.3790.8738.stgit@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> Namely, if "dev" is a VF on a virtual bus, "dev->bus->self == NULL", so
> we won't search for any of the bridges leading to the VF.  I proposed a
> pci_upstream_bridge() interface that could be used like this:
> 
> 	/* Check our parent */
> 	dev = pci_upstream_bridge(dev);
>

It looks good to me, because pci_upstream_bridge() is still in your next branch, I think maybe
I can split this changes in a separate patch after 3.13-rc1.


>>  static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
>>  {
>>  	struct dmar_drhd_unit *drhd = NULL;
>> -	int i;
>> +	struct dmar_device *dmar_dev;
>> +	struct pci_dev *pdev;
>>  
>>  	for_each_drhd_unit(drhd) {
>>  		if (drhd->ignored)
>> @@ -658,16 +659,22 @@ static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
>>  		if (segment != drhd->segment)
>>  			continue;
>>  
>> -		for (i = 0; i < drhd->devices_cnt; i++) {
>> -			if (drhd->devices[i] &&
>> -			    drhd->devices[i]->bus->number == bus &&
>> -			    drhd->devices[i]->devfn == devfn)
>> -				return drhd->iommu;
>> -			if (drhd->devices[i] &&
>> -			    drhd->devices[i]->subordinate &&
>> -			    drhd->devices[i]->subordinate->number <= bus &&
>> -			    drhd->devices[i]->subordinate->busn_res.end >= bus)
>> -				return drhd->iommu;
>> +		list_for_each_entry(dmar_dev, &drhd->head, list) {
>> +		    if (dmar_dev->bus == bus && 
>> +			    dmar_dev->devfn == devfn)
>> +			return drhd->iommu;
>> +
>> +		    pdev = pci_get_domain_bus_and_slot(dmar_dev->segment, 
>> +			    dmar_dev->bus, dmar_dev->devfn);
>> +		    if (pdev->subordinate && 
>> +			    pdev->subordinate->number <= bus &&
>> +			    pdev->subordinate->busn_res.end >= bus) {
>> +			pci_dev_put(pdev);
>> +			return drhd->iommu;
> 
> I don't know the details of how device_to_iommu() is used, but this
> style (acquire ref to pci_dev, match it to some other object, drop
> pci_dev ref, return object) makes me nervous.  How do we know the
> caller isn't depending on pci_dev to remain attached to the object?
> What happens if the pci_dev disappears when we do the pci_dev_put()
> here?

Hmmm, this is the thing I am most worried about. If we just only use
(pci_dev *) poninter in drhd->devices array as a identification. Change
(pci_dev *) pointer instead of pci device id segment:bus:devfn is safe.
Or, this is a wrong way to fix this issue. I don't know IOMMU driver much now,
so IOMMU guys any comments on this issue is welcome.

If this is not safe, what about we both save pci device id and (pci_dev *) pointer
in drhd. So we can put pci_dev ref and set pci_dev * = NULL during device removed by bus notify, and
update (pci_dev *)pointer during device add.

like this:
struct dmar_device {
    struct list_head list;
    u16 segment;
    u8 bus;
    u8 devfn;
    struct pci_dev *dev;
};

>>  	for_each_drhd_unit(drhd) {
>> -		int i;
>>  		if (drhd->ignored || drhd->include_all)
>>  			continue;
>>  
>> -		for (i = 0; i < drhd->devices_cnt; i++)
>> -			if (drhd->devices[i] &&
>> -			    !IS_GFX_DEVICE(drhd->devices[i]))
>> +		list_for_each_entry(dmar_dev, &drhd->head, list) {
>> +			pdev = pci_get_domain_bus_and_slot(dmar_dev->segment,
>> +				dmar_dev->bus, dmar_dev->devfn);
>> +			if (!IS_GFX_DEVICE(pdev)) {
>> +				pci_dev_put(pdev);
>>  				break;
>> +			}
>> +			pci_dev_put(pdev);
>> +		}
>>  
>> -		if (i < drhd->devices_cnt)
>> +		if (!IS_GFX_DEVICE(pdev))
> 
> I think this is clearly wrong.  You acquire a pdev reference, drop the
> reference, then look at pdev again after dropping the reference.  But
> as soon as you do the pci_dev_put(), you have to assume pdev is no
> longer valid.
>

You are right, should move pci_dev_put() after if (!IS_GFX_DEVICE(pdev)).



>>  
>> +struct dmar_device {
>> +	struct list_head list;
>> +	u8 segment;
> 
> I think this should be u16.  I didn't chase down how you're using it,
> but Table 8.3 in the Intel VT-d spec shows Segment Number in a DRHD
> structure as 16 bits.

Yes, it's my mistake, thanks!

> 
>> +	u8 bus;
>> +	u8 devfn;
>> +};
>> +
>>  struct intel_iommu;
>>  #ifdef CONFIG_DMAR_TABLE
>>  extern struct acpi_table_header *dmar_tbl;
>> @@ -39,8 +46,7 @@ struct dmar_drhd_unit {
>>  	struct list_head list;		/* list of drhd units	*/
>>  	struct  acpi_dmar_header *hdr;	/* ACPI header		*/
>>  	u64	reg_base_addr;		/* register base address*/
>> -	struct	pci_dev **devices; 	/* target device array	*/
>> -	int	devices_cnt;		/* target device count	*/
>> +	struct list_head head;	/* target devices' list */
> 
> s/devices'/device/ (also below).  This is not a contraction or a
> possessive construct, so no apostrophe is needed.
> 
>>  	u16	segment;		/* PCI domain		*/
>>  	u8	ignored:1; 		/* ignore drhd		*/
>>  	u8	include_all:1;
>> @@ -139,8 +145,7 @@ struct dmar_rmrr_unit {
>>  	struct acpi_dmar_header *hdr;	/* ACPI header		*/
>>  	u64	base_address;		/* reserved base address*/
>>  	u64	end_address;		/* reserved end address */
>> -	struct pci_dev **devices;	/* target devices */
>> -	int	devices_cnt;		/* target device count */
>> +	struct list_head head;	/* target devices' list */
>>  };
>>  
>>  #define for_each_rmrr_units(rmrr) \
>> @@ -149,16 +154,15 @@ struct dmar_rmrr_unit {
>>  struct dmar_atsr_unit {
>>  	struct list_head list;		/* list of ATSR units */
>>  	struct acpi_dmar_header *hdr;	/* ACPI header */
>> -	struct pci_dev **devices;	/* target devices */
>> -	int devices_cnt;		/* target device count */
>>  	u8 include_all:1;		/* include all ports */
>> +	struct list_head head;	/* target devices' list */
>>  };
>>  
>>  int dmar_parse_rmrr_atsr_dev(void);
>>  extern int dmar_parse_one_rmrr(struct acpi_dmar_header *header);
>>  extern int dmar_parse_one_atsr(struct acpi_dmar_header *header);
>> -extern int dmar_parse_dev_scope(void *start, void *end, int *cnt,
>> -				struct pci_dev ***devices, u16 segment);
>> +extern int dmar_parse_dev_scope(void *start, void *end, u16 segment, 
>> +				struct list_head *head);
>>  extern int intel_iommu_init(void);
>>  #else /* !CONFIG_INTEL_IOMMU: */
>>  static inline int intel_iommu_init(void) { return -ENODEV; }
>> -- 
>> 1.7.1
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> .
> 


-- 
Thanks!
Yijing

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/