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

[Donald Dutile <ddutile@redhat.com>]

On 5/20/25 5:13 AM, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Donald Dutile <ddutile@redhat.com>
>> Subject: Re: [PATCH rfcv2 00/20] intel_iommu: Enable stage-1 translation for
>> passthrough device
>>
>> 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:
> ...
> 
>>>>             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' .
> 
> Sorry for confusion, it stands for 'first level translation support'.
> 
Thanks.

>>
>>>>
>>>> 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.
> 
> IIUC, the hostiommustate provided by VFIO and VDPA may be different format.
> By using a general interface like .get_cap(), we hide the resolving under VFIO and
> VDPA backend. This is like the KVM extension checking between QEMU and KVM.
> 
> FYI, there was some discuss on the interface before,
> see https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg02658.html
> 
Good analogy, thanks. I'll reach out to Cedric on the above discussion as well.

>>
>>>>
>>>> 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.
> 
> Guess you mean vtd_as_from_iommu_pasid(), it's a variant of vtd_find_add_as().
> We support AS cache lookup by bdf+pasid for a long time, see vtd_find_add_as().
> 
Thanks for clarification.

>>
>>>> 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.
> 
> Good idea, will do.
> 
>>
>>> Thanks
>>> Zhenzhong
>>>
>> Again, thanks for the reply.
>> Looking fwd to the rfcv3 (on list) or move to v1-POST.
> 
> Thanks for your comments!
> 
> BRs,
> Zhenzhong
> 
Thanks for the added clarifications.
- Don