Re: [PATCH ats_vtd v5 00/22] ATS support for VT-d

[Yi Liu <yi.l.liu@intel.com>]

On 2024/7/4 12:36, CLEMENT MATHIEU--DRIF wrote:
> 
> On 03/07/2024 14:32, Yi Liu wrote:
>> Caution: External email. Do not open attachments or click links,
>> unless this email comes from a known sender and you know the content
>> is safe.
>>
> Hi, thanks for your review! very efficient!
>>
>> Hi CMD,
>>
>> I've went through the series. Some general suggestions on the series.
>>
>> 1) Patch 01, 02, 04 can be sent separately as they are fixes.
> Will do
>> 2) This series mixed the ATS and PASID capability a bit. Actually,
>>     they don't have dependency. I'd suggest you split the series into
>>        - support ATS for the requests without PASID
>>        - support ATS for requests with PASID
>>     The second part should be an incremental change based on the first
>>     part. If you can make use of the existing translate() callback, then
>>     it is possible to remove the dependency on Zhenzhong's stage-1 series.
> The final purpose is to support SVM, consequently, we only add support
> for ATS with PASID here

yes. but no need to put all of them in one series. Just like you sent
the PRI series separately.

>> 3) Some commits do not have commit message. It would be good to have
>>     it.
> Ok, I will be more verbose ;)
>> 4) Some helpers look to be used by device model, if possible, it's better
>>     to submit them with a demo device.
> The demo device is already in my GitHub repo
> (https://github.com/BullSequana/qemu/tree/master)
> It will be sent in a future series that adds the last features required
> for SVM (splitting the series to make reviews less painful)
>> 5) A design description in the cover-letter would be helpful.
> Ok, I will elaborate
>>
>> On 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote:
>>> From: Clement Mathieu--Drif <cmdetu@gmail.com>
>>>
>>> This series belongs to a list of series that add SVM support for VT-d.
>>>
>>> As a starting point, we use the series called 'intel_iommu: Enable
>>> stage-1 translation' (rfc2) by Zhenzhong Duan and Yi Liu.
>>>
>>> Here we focus on the implementation of ATS support in the IOMMU and
>>> on a PCI-level
>>> API for ATS to be used by virtual devices.
>>>
>>> This work is based on the VT-d specification version 4.1 (March 2023).
>>> Here is a link to a GitHub repository where you can find the
>>> following elements :
>>>       - Qemu with all the patches for SVM
>>>           - ATS
>>>           - PRI
>>>           - Device IOTLB invalidations
>>>           - Requests with already translated addresses
>>>       - A demo device
>>>       - A simple driver for the demo device
>>>       - A userspace program (for testing and demonstration purposes)
>>>
>>> https://github.com/BullSequana/Qemu-in-guest-SVM-demo
>>>
>>>
>>> v2
>>>       - handle huge pages better by detecting the page table level at
>>> which the translation errors occur
>>>       - Changes after review by ZhenZhong Duan :
>>>        - Set the access bit after checking permissions
>>>        - helper for PASID and ATS : make the commit message more
>>> accurate ('present' replaced with 'enabled')
>>>        - pcie_pasid_init: add PCI_PASID_CAP_WIDTH_SHIFT and use it
>>> instead of PCI_EXT_CAP_PASID_SIZEOF for shifting the pasid width when
>>> preparing the capability register
>>>        - pci: do not check pci_bus_bypass_iommu after calling
>>> pci_device_get_iommu_bus_devfn
>>>        - do not alter formatting of IOMMUTLBEntry declaration
>>>        - vtd_iova_fl_check_canonical : directly use s->aw_bits instead
>>> of aw for the sake of clarity
>>>
>>> v3
>>>       - rebase on new version of Zhenzhong's flts implementation
>>>       - fix the atc lookup operation (check the mask before returning
>>> an entry)
>>>       - add a unit test for the ATC
>>>       - store a user pointer in the iommu notifiers to simplify the
>>> implementation of svm devices
>>>       Changes after review by Zhenzhong :
>>>        - store the input pasid instead of rid2pasid when returning an
>>> entry after a translation
>>>        - split the ATC implementation and its unit tests
>>>
>>> v4
>>>       Changes after internal review
>>>        - Fix the nowrite optimization, an ATS translation without the
>>> nowrite flag should not fail when the write permission is not set
>>
>> It's strange to list internal review here.
>>
>>> v5
>>>       Changes after review by Philippe :
>>>        - change the type of 'level' to unsigned in vtd_lookup_iotlb
>>
>> list change log from latest to the earliest would be nice too. Look
>> forward
>> to your next version. :)
>>
>> Regards,
>> Yi Liu
>>
>>> Clément Mathieu--Drif (22):
>>>     intel_iommu: fix FRCD construction macro.
>>>     intel_iommu: make types match
>>>     intel_iommu: return page walk level even when the translation fails
>>>     intel_iommu: do not consider wait_desc as an invalid descriptor
>>>     memory: add permissions in IOMMUAccessFlags
>>>     pcie: add helper to declare PASID capability for a pcie device
>>>     pcie: helper functions to check if PASID and ATS are enabled
>>>     intel_iommu: declare supported PASID size
>>>     pci: cache the bus mastering status in the device
>>>     pci: add IOMMU operations to get address spaces and memory regions
>>>       with PASID
>>>     memory: store user data pointer in the IOMMU notifiers
>>>     pci: add a pci-level initialization function for iommu notifiers
>>>     intel_iommu: implement the get_address_space_pasid iommu operation
>>>     intel_iommu: implement the get_memory_region_pasid iommu operation
>>>     memory: Allow to store the PASID in IOMMUTLBEntry
>>>     intel_iommu: fill the PASID field when creating an instance of
>>>       IOMMUTLBEntry
>>>     atc: generic ATC that can be used by PCIe devices that support SVM
>>>     atc: add unit tests
>>>     memory: add an API for ATS support
>>>     pci: add a pci-level API for ATS
>>>     intel_iommu: set the address mask even when a translation fails
>>>     intel_iommu: add support for ATS
>>>
>>>    hw/i386/intel_iommu.c                     | 146 +++++-
>>>    hw/i386/intel_iommu_internal.h            |   6 +-
>>>    hw/pci/pci.c                              | 127 +++++-
>>>    hw/pci/pcie.c                             |  42 ++
>>>    include/exec/memory.h                     |  51 ++-
>>>    include/hw/i386/intel_iommu.h             |   2 +-
>>>    include/hw/pci/pci.h                      | 101 +++++
>>>    include/hw/pci/pci_device.h               |   1 +
>>>    include/hw/pci/pcie.h                     |   9 +-
>>>    include/hw/pci/pcie_regs.h                |   3 +
>>>    include/standard-headers/linux/pci_regs.h |   1 +
>>>    system/memory.c                           |  20 +
>>>    tests/unit/meson.build                    |   1 +
>>>    tests/unit/test-atc.c                     | 527 ++++++++++++++++++++++
>>>    util/atc.c                                | 211 +++++++++
>>>    util/atc.h                                | 117 +++++
>>>    util/meson.build                          |   1 +
>>>    17 files changed, 1332 insertions(+), 34 deletions(-)
>>>    create mode 100644 tests/unit/test-atc.c
>>>    create mode 100644 util/atc.c
>>>    create mode 100644 util/atc.h
>>>
>>
>>

-- 
Regards,
Yi Liu