Re: [PATCH rfcv2 00/20] intel_iommu: Enable stage-1 translation for passthrough device

[Donald Dutile <ddutile@redhat.com>]

Hey Zhenzhong,
Thanks for feedback. replies below.
- Don

On 5/19/25 4:37 AM, Duan, Zhenzhong wrote:
> Hi Donald,
> 
>> -----Original Message-----
>> From: Donald Dutile <ddutile@redhat.com>
>> Subject: Re: [PATCH rfcv2 00/20] intel_iommu: Enable stage-1 translation for
>> passthrough device
>>
>> Zhenzhong,
>>
>> Hi!
>> Eric asked me to review this series.
>> Since it's rather late since you posted will summarize review feedback
>> below/bottom.
>>
>> - Don
>>
>> On 2/19/25 3:22 AM, Zhenzhong Duan wrote:
>>> Hi,
>>>
>>> Per Jason Wang's suggestion, iommufd nesting series[1] is split into
>>> "Enable stage-1 translation for emulated device" series and
>>> "Enable stage-1 translation for passthrough device" series.
>>>
>>> This series is 2nd part focusing on passthrough device. We don't do
>>> shadowing of guest page table for passthrough device but pass stage-1
>>> page table to host side to construct a nested domain. There was some
>>> effort to enable this feature in old days, see [2] for details.
>>>
>>> The key design is to utilize the dual-stage IOMMU translation
>>> (also known as IOMMU nested translation) capability in host IOMMU.
>>> As the below diagram shows, guest I/O page table pointer in GPA
>>> (guest physical address) is passed to host and be used to perform
>>> the stage-1 address translation. Along with it, modifications to
>>> present mappings in the guest I/O page table should be followed
>>> with an IOTLB invalidation.
>>>
>>>           .-------------.  .---------------------------.
>>>           |   vIOMMU    |  | Guest I/O page table      |
>>>           |             |  '---------------------------'
>>>           .----------------/
>>>           | PASID Entry |--- PASID cache flush --+
>>>           '-------------'                        |
>>>           |             |                        V
>>>           |             |           I/O page table pointer in GPA
>>>           '-------------'
>>>       Guest
>>>       ------| Shadow |---------------------------|--------
>>>             v        v                           v
>>>       Host
>>>           .-------------.  .------------------------.
>>>           |   pIOMMU    |  |  FS for GIOVA->GPA     |
>>>           |             |  '------------------------'
>>>           .----------------/  |
>>>           | PASID Entry |     V (Nested xlate)
>>>           '----------------\.----------------------------------.
>>>           |             |   | SS for GPA->HPA, unmanaged domain|
>>>           |             |   '----------------------------------'
>>>           '-------------'
>>> Where:
>>>    - FS = First stage page tables
>>>    - SS = Second stage page tables
>>> <Intel VT-d Nested translation>
>>>
>> I'd prefer the use of 's1' for stage1/First stage, and 's2' for stage2/second stage.
>> We don't need different terms for the same technology in the iommu/iommufd
>> space(s).
> 
> OK, then I'd like to use stage1 and stage2 everywhere which is more verbose.
> 
your choice; in other kernel & qemu code I've seen, 's1' and 's2' are used quite frequently,
that's why I recommended it -- call it plagerizing! ;-)

>>
>>> There are some interactions between VFIO and vIOMMU
>>> * vIOMMU registers PCIIOMMUOps [set|unset]_iommu_device to PCI
>>>     subsystem. VFIO calls them to register/unregister HostIOMMUDevice
>>>     instance to vIOMMU at vfio device realize stage.
>>> * vIOMMU calls HostIOMMUDeviceIOMMUFD interface [at|de]tach_hwpt
>>>     to bind/unbind device to IOMMUFD backed domains, either nested
>>>     domain or not.
>>>
>>> See below diagram:
>>>
>>>           VFIO Device                                 Intel IOMMU
>>>       .-----------------.                         .-------------------.
>>>       |                 |                         |                   |
>>>       |       .---------|PCIIOMMUOps              |.-------------.    |
>>>       |       | IOMMUFD |(set_iommu_device)       || Host IOMMU  |    |
>>>       |       | Device  |------------------------>|| Device list |    |
>>>       |       .---------|(unset_iommu_device)     |.-------------.    |
>>>       |                 |                         |       |           |
>>>       |                 |                         |       V           |
>>>       |       .---------|  HostIOMMUDeviceIOMMUFD |  .-------------.  |
>>>       |       | IOMMUFD |            (attach_hwpt)|  | Host IOMMU  |  |
>>>       |       | link    |<------------------------|  |   Device    |  |
>>>       |       .---------|            (detach_hwpt)|  .-------------.  |
>>>       |                 |                         |       |           |
>>>       |                 |                         |       ...         |
>>>       .-----------------.                         .-------------------.
>>>
>>> Based on Yi's suggestion, this design is optimal in sharing ioas/hwpt
>>> whenever possible and create new one on demand, also supports multiple
>>> iommufd objects and ERRATA_772415.
>>>
>>> E.g., Stage-2 page table could be shared by different devices if there
>>> is no conflict and devices link to same iommufd object, i.e. devices
>>> under same host IOMMU can share same stage-2 page table. If there is
>> and 'devices under the same guest'.
>> Different guests cant be sharing the same stage-2 page tables.
> 
> Yes, will update.
> 
Thanks.

>>
>>> conflict, i.e. there is one device under non cache coherency mode
>>> which is different from others, it requires a separate stage-2 page
>>> table in non-CC mode.
>>>
>>> SPR platform has ERRATA_772415 which requires no readonly mappings
>>> in stage-2 page table. This series supports creating VTDIOASContainer
>>> with no readonly mappings. If there is a rare case that some IOMMUs
>>> on a multiple IOMMU host have ERRATA_772415 and others not, this
>>> design can still survive.
>>>
>>> See below example diagram for a full view:
>>>
>>>         IntelIOMMUState
>>>                |
>>>                V
>>>       .------------------.    .------------------.    .-------------------.
>>>       | VTDIOASContainer |--->| VTDIOASContainer |--->| VTDIOASContainer  |--
>>> ...
>>>       | (iommufd0,RW&RO) |    | (iommufd1,RW&RO) |    | (iommufd0,RW only)|
>>>       .------------------.    .------------------.    .-------------------.
>>>                |                       |                              |
>>>                |                       .-->...                        |
>>>                V                                                      V
>>>         .-------------------.    .-------------------.          .---------------.
>>>         |   VTDS2Hwpt(CC)   |--->| VTDS2Hwpt(non-CC) |-->...    | VTDS2Hwpt(CC) |-
>> ->...
>>>         .-------------------.    .-------------------.          .---------------.
>>>             |            |               |                            |
>>>             |            |               |                            |
>>>       .-----------.  .-----------.  .------------.              .------------.
>>>       | IOMMUFD   |  | IOMMUFD   |  | IOMMUFD    |              | IOMMUFD    |
>>>       | Device(CC)|  | Device(CC)|  | Device     |              | Device(CC) |
>>>       | (iommufd0)|  | (iommufd0)|  | (non-CC)   |              | (errata)   |
>>>       |           |  |           |  | (iommufd0) |              | (iommufd0) |
>>>       .-----------.  .-----------.  .------------.              .------------.
>>>
>>> This series is also a prerequisite work for vSVA, i.e. Sharing
>>> guest application address space with passthrough devices.
>>>
>>> To enable stage-1 translation, only need to add "x-scalable-mode=on,x-flts=on".
>>> i.e. -device intel-iommu,x-scalable-mode=on,x-flts=on...
>>>
>>> Passthrough device should use iommufd backend to work with stage-1
>> translation.
>>> i.e. -object iommufd,id=iommufd0 -device vfio-pci,iommufd=iommufd0,...
>>>
>>> If host doesn't support nested translation, qemu will fail with an unsupported
>>> report.
>>>
>>> Test done:
>>> - VFIO devices hotplug/unplug
>>> - different VFIO devices linked to different iommufds
>>> - vhost net device ping test
>>>
>>> PATCH1-8:  Add HWPT-based nesting infrastructure support
>>> PATCH9-10: Some cleanup work
>>> PATCH11:   cap/ecap related compatibility check between vIOMMU and Host
>> IOMMU
>>> PATCH12-19:Implement stage-1 page table for passthrough device
>>> PATCH20:   Enable stage-1 translation for passthrough device
>>>
>>> Qemu code can be found at [3]
>>>
>>> TODO:
>>> - RAM discard
>>> - dirty tracking on stage-2 page table
>>>
>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02740.html
>>> [2] https://patchwork.kernel.org/project/kvm/cover/20210302203827.437645-
>> 1-yi.l.liu@intel.com/
>>> [3]
>> https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_rfcv2
>>>
>>> Thanks
>>> Zhenzhong
>>>
>>> Changelog:
>>> rfcv2:
>>> - Drop VTDPASIDAddressSpace and use VTDAddressSpace (Eric, Liuyi)
>>> - Move HWPT uAPI patches ahead(patch1-8) so arm nesting could easily rebase
>>> - add two cleanup patches(patch9-10)
>>> - VFIO passes iommufd/devid/hwpt_id to vIOMMU instead of
>> iommufd/devid/ioas_id
>>> - add vtd_as_[from|to]_iommu_pasid() helper to translate between vtd_as and
>>>     iommu pasid, this is important for dropping VTDPASIDAddressSpace
>>>
>>> Yi Liu (3):
>>>     intel_iommu: Replay pasid binds after context cache invalidation
>>>     intel_iommu: Propagate PASID-based iotlb invalidation to host
>>>     intel_iommu: Refresh pasid bind when either SRTP or TE bit is changed
>>>
>>> Zhenzhong Duan (17):
>>>     backends/iommufd: Add helpers for invalidating user-managed HWPT
>>>     vfio/iommufd: Add properties and handlers to
>>>       TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>>>     HostIOMMUDevice: Introduce realize_late callback
>>>     vfio/iommufd: Implement HostIOMMUDeviceClass::realize_late() handler
>>>     vfio/iommufd: Implement [at|de]tach_hwpt handlers
>>>     host_iommu_device: Define two new capabilities
>>>       HOST_IOMMU_DEVICE_CAP_[NESTING|FS1GP]
>>>     iommufd: Implement query of HOST_IOMMU_DEVICE_CAP_[NESTING|FS1GP]
>>>     iommufd: Implement query of HOST_IOMMU_DEVICE_CAP_ERRATA
>>>     intel_iommu: Rename vtd_ce_get_rid2pasid_entry to
>>>       vtd_ce_get_pasid_entry
>>>     intel_iommu: Optimize context entry cache utilization
>>>     intel_iommu: Check for compatibility with IOMMUFD backed device when
>>>       x-flts=on
>>>     intel_iommu: Introduce a new structure VTDHostIOMMUDevice
>>>     intel_iommu: Add PASID cache management infrastructure
>>>     intel_iommu: Bind/unbind guest page table to host
>>>     intel_iommu: ERRATA_772415 workaround
>>>     intel_iommu: Bypass replay in stage-1 page table mode
>>>     intel_iommu: Enable host device when x-flts=on in scalable mode
>>>
>>>    hw/i386/intel_iommu_internal.h     |   56 +
>>>    include/hw/i386/intel_iommu.h      |   33 +-
>>>    include/system/host_iommu_device.h |   40 +
>>>    include/system/iommufd.h           |   53 +
>>>    backends/iommufd.c                 |   58 +
>>>    hw/i386/intel_iommu.c              | 1660 ++++++++++++++++++++++++----
>>>    hw/vfio/common.c                   |   17 +-
>>>    hw/vfio/iommufd.c                  |   48 +
>>>    backends/trace-events              |    1 +
>>>    hw/i386/trace-events               |   13 +
>>>    10 files changed, 1776 insertions(+), 203 deletions(-)
>>>
>> Relative to the patches:
>> Patch 1: As Eric eluded to, a proper description for the patch should be written,
>> and the title should change 'helpers' to 'helper'
> 
> This is addressed in [1]
> 
> [1] https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv3.wip/
> 
ok.
>>
>> Patch 2:
>> (1) Introduce 'realize_late()' interface, but leave the reader wondering 'ah, why?
>> what?' ... after reading farther down the series, I learn more about realize_late(),
>> but more on that later...
> 
> realize_late() has been removed in [1]
> 
Yes, I saw. :)

>> (2) For my education, can you provide ptrs to VFIO & VPDA code paths that
>> demonstrate the need for different [at|de]tach_<>_hwpt()
> 
> Google help me find this link https://lkml.iu.edu/2309.2/08079.html
> specially https://gitlab.com/lulu6/gitlabqemutmp/-/commit/354870ff6bd9bac80c9fc5c7f944331cb24b0331
> 
thanks for vdpa ptrs; I have my diff work cut out for me, but heh, I asked for it! :)

>>
>> Patch 3: Why can't the realize() be moved to after attach?  isn't realize() suppose
>> to indicate 'all is setup and object can now be used' -- apologies for what could
>> be a dumb question, as that's my understanding of realize().  If the argument is
>> such that there needs to be two steps, how does the first realize() that put the
>> object into a used state <somehow> wait until realize_late()?
>>
>> Patch 4: Shouldn't the current/existing realize callback just be overwritten with
>> the later one, when this is needed?
> 
> realize_late() has been removed in [1]
> 
+1

>>
>> Patch 5: no issues.
>>
>> Patch 6: ewww -- fs1gp ... we use underlines all over the place for multi-word
>> elements; so how about 's1_1g_pg'
>>            -- how many places is that really used that multiple underlines is an issue?
> 
> This is vtd specific, so to follow VTD spec's naming so that we can easily find its definition by searching fs1gp.
> 
>>
ok.  You had mentioned earlier that you used VTD nomenclature, so I should have concluded the same here. /my bad.

>> Patch 7: intel-iommu-specific callbacks in the common vfio & iommufd-backend
>> code; nack. This won't compile w/intel-iommu included with iommufd... I think
>> backend, intel-iommu hw-caps should provide the generic 'caps' boolean-type
>> values/states; ... and maybe they should be extracted via vfio? .... like
>>       case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>>           return vfio_device_get_aw_bits(hiod->agent);
>>
>> Patch 8: Again, VTD-specific code in IOMMUFD is a nack; again, maybe via vfio,
>> or a direct call into an iommu-device-cap api.
> 
> Patch 7&8 addressed in [1]
> 
+1
>>
>> Patch 9: no issues.
>>
>> Patch 10: "except it's stale"  likely "except when it's entry is stale" ?
> 
> addressed in [1]
> 
+1
>>            Did you ever put some tracing in to capture avg hits in cache? ... if so, add
>> as a comment.
>>            Otherwise, looks good.
>>
>> Patch 11: Apologies, I don't know what 'flts' stands for, and why it is relative to 2-
>> stage mapping, or SIOV.  Could you add verbage to explain the use of it, as the
>> rest of this patch doesn't make any sense to me without the background.
>> The patch introduces hw-info-type (none or intel), and then goes on to add a
>> large set of checks; seems like the caps & this checking should go together (split
>> for each cap; put all caps together & the check...).
> 
> OK, will do. There are some explanations in cover-letter.
> For history reason, old vtd spec define stage-1 as first level then switch to first stage.
> 
So 'flts' is 'first level then switch' .

>>
>> Patch 12: Why isn't HostIOMMUDevice extended to have another iommu-specif
>> element, opaque in HostIOMMUDevice, but set to specific IOMMU in use?   e.g.
>> void *hostiommustate;
> 
> Yes, that's possible, but we want to make a generic interface between VFIO/VDPA and vIOMMU.
> 
ok. I don't understand how VFIO & VPDA complicate that add.

>>
>> Patch 13: Isn't PASID just an extension/addition of BDF id? and doesn't each
>> PASID have its own address space?
> 
> Yes, it is.
> 
>> So, why isn't it handle via a uniqe AS cache like 'any other device'?  Maybe I'm
>> thinking too SMMU-StreamID, which can be varying length, depending on
>> subsystem support.  I see what appears to be sid+pasid calls to do the AS lookups;
>> hmm, so maybe this is the generalized BDF+pasid AS lookup?  if so, maybe a
>> better description stating this transition to a wider stream-id would set the code
>> context better.
> 
> Not quite get..
> 
I'm looking for a better description that states the AS cache lookup is broadened from bdf
to bdf+pasid.

>> As for the rest of the (400 intel-iommu) code, I'm not that in-depth in intel-iommu
>> to determine if its all correct or not.
>>
>> Patch 14: Define PGTT; the second paragraph seem self-contradicting -- it says it
>> uses a 2-stage page table in each case, but it implies it should be different.  At 580
>> lines of code changes, you win! ;-)
> 
> The host side's using nested or only stage-2 page table depends on PGTT's setting in guest.
> 
Thanks for clarification.

>>
>> Patch 15: Read-only and Read/write areas have different IOMMUFDs?  is that an
>> intel-iommu requriement?
>>            At least this intel-iommu-errata code is only in hw/i386/<> modules.
> 
> No, if ERRATA_772415, read-only areas should not be mapped, so we allocate a new VTDIOASContainer to hold only read/write areas mapping.
> We can use same IOMMUFDs for different VTDIOASContainer.
> 
ah yes; I got hung-up on different mappings, and didn't back up to AS-container split & same IOMMUFD.

>>
>> Patch 16: Looks reasonable.  What does the 'SI' mean after "CACHE_DEV",
>> "CACHE_DOM" & "CACHE_PASID" ? -- stream-invalidation?
> 
> VTD_PASID_CACHE_DEVSI stands for 'pasid cache device selective invalidation',
> VTD_PASID_CACHE_DOMSI means 'pasid cache domain selective invalidation'.
> 
That explanation helps. :)  maybe put a short blurb in the commit log, or code,
so one doesn't have to be a ninja-VTD spec consumer to comprehend those (important) diffs.

> Thanks
> Zhenzhong
> 
Again, thanks for the reply.
Looking fwd to the rfcv3 (on list) or move to v1-POST.
- Don