>-----Original Message-----
>From: Donald Dutile <ddut...@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 <ddut...@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'.
>
>>>
>>> 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
>
>>>
>>> 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().
>
>>> 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
