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

[Bjorn Helgaas]

On Thu, Nov 7, 2013 at 8:40 PM, Yijing Wang <wangyijing@xxxxxxxxxx> wrote:
> 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.

Yep, that would be a fix for a separate issue and should be a separate patch.

>>>  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.

I don't know the IOMMU drivers well either, but it seems like they
rely on notifications of device addition and removal (see
iommu_bus_notifier()).  It doesn't seem right for them to also use the
generic PCI interfaces like pci_get_domain_bus_and_slot() because the
IOMMU driver should already know what devices exist and their
lifetimes.  It seems like confusion to mix the two.  But I don't have
a concrete suggestion.

Bjorn
--
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/