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

[CLEMENT MATHIEU--DRIF]

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