On 5/20/25 5:13 AM, Duan, Zhenzhong wrote:
-----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'.
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
