linux-kernel - Re: [PATCH v10 04/12] iommu: Add attach/detach_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 24 Jul 2022 15:03:16 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Christoph Hellwig <hch@...radead.org>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Vinod Koul <vkoul@...nel.org>,
        Eric Auger <eric.auger@...hat.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        Jacob jun Pan <jacob.jun.pan@...el.com>,
        Zhangfei Gao <zhangfei.gao@...aro.org>,
        Zhu Tony <tony.zhu@...el.com>, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Jean-Philippe Brucker <jean-philippe@...aro.org>
Subject: Re: [PATCH v10 04/12] iommu: Add attach/detach_dev_pasid iommu
 interface

Hi Jason,

Thank you for reviewing this series.

On 2022/7/23 22:11, Jason Gunthorpe wrote:
> On Tue, Jul 05, 2022 at 01:07:02PM +0800, Lu Baolu wrote:
>> Attaching an IOMMU domain to a PASID of a device is a generic operation
>> for modern IOMMU drivers which support PASID-granular DMA address
>> translation. Currently visible usage scenarios include (but not limited):
>>
>>   - SVA (Shared Virtual Address)
>>   - kernel DMA with PASID
>>   - hardware-assist mediated device
>>
>> This adds a pair of domain ops for this purpose and adds the interfaces
>> for device drivers to attach/detach a domain to/from a {device, PASID}.
>> Some buses, like PCI, route packets without considering the PASID
>> value.
> Below the comments touch on ACS, so this is a bit out of date
> 
>> +static bool iommu_group_immutable_singleton(struct iommu_group *group,
>> +					    struct device *dev)
>> +{
>> +	int count;
>> +
>> +	mutex_lock(&group->mutex);
>> +	count = iommu_group_device_count(group);
>> +	mutex_unlock(&group->mutex);
>> +
>> +	if (count != 1)
>> +		return false;
>> +
>> +	/*
>> +	 * The PCI device could be considered to be fully isolated if all
>> +	 * devices on the path from the device to the host-PCI bridge are
>> +	 * protected from peer-to-peer DMA by ACS.
>> +	 */
>> +	if (dev_is_pci(dev))
>> +		return pci_acs_path_enabled(to_pci_dev(dev), NULL,
>> +					    REQ_ACS_FLAGS);
> You might want to explain what condition causes ACS isolated devices
> to share a group in the first place..
> 

How about rephrasing this part of commit message like below:

Some buses, like PCI, route packets without considering the PASID value.
Thus a DMA target address with PASID might be treated as P2P if the
address falls into the MMIO BAR of other devices in the group. To make
things simple, these interfaces only apply to devices belonging to the
singleton groups.

Considering that the PCI bus supports hot-plug, even a device boots with
a singleton group, a later hot-added device is still possible to share
the group, which breaks the singleton group assumption. In order to
avoid this situation, this interface requires that the ACS is enabled on
all devices on the path from the device to the host-PCI bridge.

Best regards,
baolu