Message-ID: <e8d8d08c-8327-b0ad-6bdd-ef10cd34e212@intel.com>
Date:   Tue, 12 Apr 2022 14:34:02 +0800
From:   Yi Liu <yi.l.liu@...el.com>
To:     Lu Baolu <baolu.lu@...ux.intel.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        Joerg Roedel <joro@...tes.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        "Christoph Hellwig" <hch@...radead.org>,
        "Raj, Ashok" <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>
CC:     Eric Auger <eric.auger@...hat.com>,
        "Pan, Jacob jun" <jacob.jun.pan@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v3 02/12] iommu: Add a flag to indicate immutable
 singleton group

Hi Baolu,

On 2022/4/12 13:08, Lu Baolu wrote:
> Hi Kevin,
> 
> Thanks for your time.
> 
> On 2022/4/12 11:15, Tian, Kevin wrote:
>>> From: Lu Baolu <baolu.lu@...ux.intel.com>
>>> Sent: Sunday, April 10, 2022 6:25 PM
>>>
>>> Some features require that a single device must be immutably isolated,
>>> even when hot plug is supported.
>>
>> This reads confusing, as hotplug cannot be allowed in a singleton group.
>> What you actually meant suppose to be:
>>
>> "Enabling certain device features require the device in a singleton iommu
>> group which is immutable in fabric i.e. not affected by hotplug"
> 
> Yours is better. Thank you.
> 
>>
>>> For example, the SVA bind()/unbind()
>>> interfaces require that the device exists in a singleton group. If we
>>> have a singleton group that doesn't have ACS (or similar technologies)
>>> and someone hotplugs in another device on a bridge, then our SVA is
>>> completely broken and we get data corruption.
>>
>> this needs the background that PASID doesn't join PCI packet routing
>> thus the dma address (CPU VA) may hit a p2p range.
> 
> Sure.
> 
>>
>>>
>>> This adds a flag in the iommu_group struct to indicate an immutable
>>> singleton group, and uses standard PCI bus topology, isolation features,
>>> and DMA alias quirks to set the flag. If the device came from DT, assume
>>> it is static and then the singleton attribute can know from the device
>>> count in the group.
>>
>> where does the assumption come from?
> 
> Hotplug is the only factor that can dynamically affect the
> characteristics of IOMMU group singleton as far as I can see. If a
> device node was created from the DT, it could be treated as static,
> hence we can judge the singleton in iommu probe phase during boot.

not sure if hotplug is the only factor. Is it possible that admin modifies
the ACS configuration on the bridge?

>>
>>>
>>> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
>>> Suggested-by: Kevin Tian <kevin.tian@...el.com>
>>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>>> ---
>>>   drivers/iommu/iommu.c | 67 ++++++++++++++++++++++++++++++++++++----
>>> ---
>>>   1 file changed, 57 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 0c42ece25854..56ffbf5fdc18 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -48,6 +48,7 @@ struct iommu_group {
>>>       struct list_head entry;
>>>       unsigned int owner_cnt;
>>>       void *owner;
>>> +    bool immutable_singleton;
>>
>> Just call it 'singleton' with a comment to explain it must be immutable?
> 
> I was just trying to distinguish "singleton" and "immutable singleton".
> A group is singleton if it only contains a single device in the device
> list, while a group is immutable singleton only if the fabric doesn't
> allow to change the state.
> 
>>
>>>   };
>>>
>>>   struct group_device {
>>> @@ -74,6 +75,16 @@ static const char * const
>>> iommu_group_resv_type_string[] = {
>>>   #define IOMMU_CMD_LINE_DMA_API        BIT(0)
>>>   #define IOMMU_CMD_LINE_STRICT        BIT(1)
>>>
>>> +/*
>>> + * To consider a PCI device isolated, we require ACS to support Source
>>> + * Validation, Request Redirection, Completer Redirection, and Upstream
>>> + * Forwarding.  This effectively means that devices cannot spoof their
>>> + * requester ID, requests and completions cannot be redirected, and all
>>> + * transactions are forwarded upstream, even as it passes through a
>>> + * bridge where the target device is downstream.
>>> + */
>>> +#define REQ_ACS_FLAGS   (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
>>> PCI_ACS_UF)
>>> +
>>>   static int iommu_alloc_default_domain(struct iommu_group *group,
>>>                         struct device *dev);
>>>   static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>>> @@ -89,6 +100,7 @@ static int
>>> iommu_create_device_direct_mappings(struct iommu_group *group,
>>>   static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>>>   static ssize_t iommu_group_store_type(struct iommu_group *group,
>>>                         const char *buf, size_t count);
>>> +static int iommu_group_device_count(struct iommu_group *group);
>>>
>>>   #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)
>>>     \
>>>   struct iommu_group_attribute iommu_group_attr_##_name =        \
>>> @@ -844,6 +856,37 @@ static bool iommu_is_attach_deferred(struct device
>>> *dev)
>>>       return false;
>>>   }
>>>
>>> +static int has_pci_alias(struct pci_dev *pdev, u16 alias, void *opaque)
>>> +{
>>> +    return -EEXIST;
>>> +}
>>> +
>>> +static bool pci_immutably_isolated(struct pci_dev *pdev)
>>> +{
>>> +    /* Skip the bridges. */
>>> +    if (pci_is_bridge(pdev))
>>> +        return false;
>>> +
>>> +    /*
>>> +     * The device could be considered to be fully isolated if
>>> +     * all devices on the path from the parent to the host-PCI
>>> +     * bridge are protected from peer-to-peer DMA by ACS.
>>> +     */
>>> +    if (!pci_is_root_bus(pdev->bus) &&
>>> +        !pci_acs_path_enabled(pdev->bus->self, NULL, REQ_ACS_FLAGS))
>>> +        return false;
>>> +
>>> +    /* Multi-function devices should have ACS enabled. */
>>> +    if (pdev->multifunction && !pci_acs_enabled(pdev, REQ_ACS_FLAGS))
>>> +        return false;
>>
>> Looks my earlier comment was lost, i.e. you can just use
>> pci_acs_path_enabled(pdev) to cover above two checks.
> 
> If a device is directly connected to the root bridge and it is not an
> MFD, do we still need ACS on it? The Intel idxd device seems to be such
> a device. I had a quick check with lspci, it has no ACS support.
> 
> I probably missed anything.

seems not necessary per my knowledge.

> 
>>
>>> +
>>> +    /* Filter out devices which has any alias device. */
>>> +    if (pci_for_each_dma_alias(pdev, has_pci_alias, NULL))
>>> +        return false;
>>> +
>>> +    return true;
>>> +}
>>> +
>>>   /**
>>>    * iommu_group_add_device - add a device to an iommu group
>>>    * @group: the group into which to add the device (reference should be 
>>> held)
>>> @@ -898,6 +941,20 @@ int iommu_group_add_device(struct iommu_group
>>> *group, struct device *dev)
>>>       list_add_tail(&device->list, &group->devices);
>>>       if (group->domain  && !iommu_is_attach_deferred(dev))
>>>           ret = __iommu_attach_device(group->domain, dev);
>>> +
>>> +    /*
>>> +     * Use standard PCI bus topology, isolation features, and DMA
>>> +     * alias quirks to set the immutable singleton attribute. If
>>> +     * the device came from DT, assume it is static and then
>>> +     * singleton can know from the device count in the group.
>>> +     */
>>> +    if (dev_is_pci(dev))
>>> +        group->immutable_singleton =
>>> +                pci_immutably_isolated(to_pci_dev(dev));
>>> +    else if (is_of_node(dev_fwnode(dev)))
>>> +        group->immutable_singleton =
>>> +                (iommu_group_device_count(group) == 1);
>>> +
>>>       mutex_unlock(&group->mutex);
>>>       if (ret)
>>>           goto err_put_group;
>>> @@ -1290,16 +1347,6 @@ EXPORT_SYMBOL_GPL(iommu_group_id);
>>>   static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev,
>>>                              unsigned long *devfns);
>>>
>>> -/*
>>> - * To consider a PCI device isolated, we require ACS to support Source
>>> - * Validation, Request Redirection, Completer Redirection, and Upstream
>>> - * Forwarding.  This effectively means that devices cannot spoof their
>>> - * requester ID, requests and completions cannot be redirected, and all
>>> - * transactions are forwarded upstream, even as it passes through a
>>> - * bridge where the target device is downstream.
>>> - */
>>> -#define REQ_ACS_FLAGS   (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
>>> PCI_ACS_UF)
>>> -
>>>   /*
>>>    * For multifunction devices which are not isolated from each other, find
>>>    * all the other non-isolated functions and look for existing groups.  
>>> For
>>> -- 
>>> 2.25.1
>>
> 
> Best regards,
> baolu

-- 
Regards,
Yi Liu

Powered by blists - more mailing lists