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