LinuxLists.cc - [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

[permalink] [raw]

Subject: RE: [PATCH v6 1/6] iommu: Add cache_invalidate_user op

> From: Yi Liu <[email protected]>
> Sent: Friday, November 17, 2023 9:07 PM
>
> +/**
> + * struct iommu_user_data_array - iommu driver specific user space data
> array
> + * @type: The data type of all the entries in the user buffer array
> + * @uptr: Pointer to the user buffer array for copy_from_user()

remove "for copy_from_user()"

> + * @entry_len: The fixed-width length of a entry in the array, in bytes

s/a/an/

> + * @entry_num: The number of total entries in the array
> + *
> + * A array having a @entry_num number of @entry_len sized entries, each
> entry is
> + * user space data, an uAPI defined in include/uapi/linux/iommufd.h where
> @type
> + * is also defined as enum iommu_xyz_data_type.

* The user buffer array has @entry_num entries, each with a fixed size
* @entry_len. The entry format and related type are defined in
* include/uapi/linux/iommufd.h

2023-11-20 08:18:08

[permalink] [raw]

Subject: RE: [PATCH v6 3/6] iommu: Add iommu_copy_struct_from_user_array helper

> From: Liu, Yi L <[email protected]>
> Sent: Friday, November 17, 2023 9:07 PM
>
> +/**
> + * __iommu_copy_struct_from_user_array - Copy iommu driver specific

__iommu_copy_entry_from_user_array?

2023-11-20 17:27:04

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] iommu: Add iommu_copy_struct_from_user_array helper

On Mon, Nov 20, 2023 at 08:17:47AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <[email protected]>
> > Sent: Friday, November 17, 2023 9:07 PM
> >
> > +/**
> > + * __iommu_copy_struct_from_user_array - Copy iommu driver specific
>
> __iommu_copy_entry_from_user_array?

I think "struct" and "entry" are interchangeable. Yet, aligning
with the {__}iommu_copy_struct_from_user seems to be nicer?

2023-11-21 02:49:02

[permalink] [raw]

Subject: RE: [PATCH v6 3/6] iommu: Add iommu_copy_struct_from_user_array helper

> From: Nicolin Chen <[email protected]>
> Sent: Tuesday, November 21, 2023 1:25 AM
>
> On Mon, Nov 20, 2023 at 08:17:47AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <[email protected]>
> > > Sent: Friday, November 17, 2023 9:07 PM
> > >
> > > +/**
> > > + * __iommu_copy_struct_from_user_array - Copy iommu driver specific
> >
> > __iommu_copy_entry_from_user_array?
>
> I think "struct" and "entry" are interchangeable. Yet, aligning
> with the {__}iommu_copy_struct_from_user seems to be nicer?

ok

2023-12-06 18:32:46

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] iommu: Add cache_invalidate_user op

On Fri, Nov 17, 2023 at 05:07:12AM -0800, Yi Liu wrote:
> +/**
> + * struct iommu_user_data_array - iommu driver specific user space data array
> + * @type: The data type of all the entries in the user buffer array
> + * @uptr: Pointer to the user buffer array for copy_from_user()
> + * @entry_len: The fixed-width length of a entry in the array, in bytes
> + * @entry_num: The number of total entries in the array
> + *
> + * A array having a @entry_num number of @entry_len sized entries, each entry is
> + * user space data, an uAPI defined in include/uapi/linux/iommufd.h where @type
> + * is also defined as enum iommu_xyz_data_type.
> + */
> +struct iommu_user_data_array {
> + unsigned int type;
> + void __user *uptr;
> + size_t entry_len;
> + int entry_num;

These are u32 in the uapi, they should probably be u32 here
too. Otherwise we have to worry about truncation.

> @@ -465,6 +492,9 @@ struct iommu_domain_ops {
> size_t size);
> void (*iotlb_sync)(struct iommu_domain *domain,
> struct iommu_iotlb_gather *iotlb_gather);
> + int (*cache_invalidate_user)(struct iommu_domain *domain,
> + struct iommu_user_data_array *array,
> + u32 *error_code);

Regarding the other conversation I worry a u32 error_code is too small.

Unfortunately there is no obvious place to put something better so if
we reach it we will have to add more error_code space via normal
extension.

Maybe expand this to u64? That is 64 bits of error register data and
the consumer index. It should do for SMMUv3 at least?

Jason

2023-12-06 18:44:25

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] iommu: Add cache_invalidate_user op

On Wed, Dec 06, 2023 at 02:32:09PM -0400, Jason Gunthorpe wrote:
> On Fri, Nov 17, 2023 at 05:07:12AM -0800, Yi Liu wrote:

> > @@ -465,6 +492,9 @@ struct iommu_domain_ops {
> > size_t size);
> > void (*iotlb_sync)(struct iommu_domain *domain,
> > struct iommu_iotlb_gather *iotlb_gather);
> > + int (*cache_invalidate_user)(struct iommu_domain *domain,
> > + struct iommu_user_data_array *array,
> > + u32 *error_code);
>
> Regarding the other conversation I worry a u32 error_code is too small.
>
> Unfortunately there is no obvious place to put something better so if
> we reach it we will have to add more error_code space via normal
> extension.
>
> Maybe expand this to u64? That is 64 bits of error register data and
> the consumer index. It should do for SMMUv3 at least?

I think Yi is moving the error_code to the entry data structure,
where we can even define a list of error_codes as a driver data
needs. So, I assume this u32 pointer would be gone too.

Thanks
Nicolin

2023-12-06 18:53:03

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] iommu: Add cache_invalidate_user op

On Wed, Dec 06, 2023 at 10:43:34AM -0800, Nicolin Chen wrote:
> On Wed, Dec 06, 2023 at 02:32:09PM -0400, Jason Gunthorpe wrote:
> > On Fri, Nov 17, 2023 at 05:07:12AM -0800, Yi Liu wrote:
>
> > > @@ -465,6 +492,9 @@ struct iommu_domain_ops {
> > > size_t size);
> > > void (*iotlb_sync)(struct iommu_domain *domain,
> > > struct iommu_iotlb_gather *iotlb_gather);
> > > + int (*cache_invalidate_user)(struct iommu_domain *domain,
> > > + struct iommu_user_data_array *array,
> > > + u32 *error_code);
> >
> > Regarding the other conversation I worry a u32 error_code is too small.
> >
> > Unfortunately there is no obvious place to put something better so if
> > we reach it we will have to add more error_code space via normal
> > extension.
> >
> > Maybe expand this to u64? That is 64 bits of error register data and
> > the consumer index. It should do for SMMUv3 at least?
>
> I think Yi is moving the error_code to the entry data structure,
> where we can even define a list of error_codes as a driver data
> needs. So, I assume this u32 pointer would be gone too.

Oh, lets see that then..

Jason

2023-12-07 06:51:31

by Yi Liu

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] iommu: Add cache_invalidate_user op

On 2023/12/7 02:50, Jason Gunthorpe wrote:
> On Wed, Dec 06, 2023 at 10:43:34AM -0800, Nicolin Chen wrote:
>> On Wed, Dec 06, 2023 at 02:32:09PM -0400, Jason Gunthorpe wrote:
>>> On Fri, Nov 17, 2023 at 05:07:12AM -0800, Yi Liu wrote:
>>
>>>> @@ -465,6 +492,9 @@ struct iommu_domain_ops {
>>>> size_t size);
>>>> void (*iotlb_sync)(struct iommu_domain *domain,
>>>> struct iommu_iotlb_gather *iotlb_gather);
>>>> + int (*cache_invalidate_user)(struct iommu_domain *domain,
>>>> + struct iommu_user_data_array *array,
>>>> + u32 *error_code);
>>>
>>> Regarding the other conversation I worry a u32 error_code is too small.
>>>
>>> Unfortunately there is no obvious place to put something better so if
>>> we reach it we will have to add more error_code space via normal
>>> extension.
>>>
>>> Maybe expand this to u64? That is 64 bits of error register data and
>>> the consumer index. It should do for SMMUv3 at least?
>>
>> I think Yi is moving the error_code to the entry data structure,
>> where we can even define a list of error_codes as a driver data
>> needs. So, I assume this u32 pointer would be gone too.
>
> Oh, lets see that then..

yes, I'm going to move it.

--
Regards,
Yi Liu

2023-12-09 01:47:53

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Fri, Nov 17, 2023 at 05:07:11AM -0800, Yi Liu wrote:

> Take Intel VT-d as an example, the stage-1 translation table is I/O page
> table. 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.

I've been looking at what the three HW's need for invalidation, it is
a bit messy.. Here is my thinking. Please let me know if I got it right

What is the starting point of the guest memory walks:
Intel: Single Scalable Mode PASID table entry indexed by a RID & PASID
AMD: GCR3 table (a table of PASIDs) indexed by RID
ARM: CD table (a table of PASIDs) indexed by RID

What key does the physical HW use for invalidation:
Intel: Domain-ID (stored in hypervisor, per PASID), PASID
AMD: Domain-ID (stored in hypervisor, per RID), PASID
ARM: VMID (stored in hypervisor, per RID), ASID (stored in guest)

Why key does the VM use for invalidation:
Intel: vDomain-ID (per PASID), PASID
AMD: vDomain-ID (per RID), PASID
ARM: ASID

What is in a Nested domain:
Intel: A single IO page table refereed to by a PASID entry
Each vDomain-ID,PASID allocates a unique nesting domain
AMD: A GCR3 table pointer
Nesting domains are created for every unique GCR3 pointer.
vDomain-ID can possibly refer to multiple Nesting domains :(
ARM: A CD table pointer
Nesting domains are created for every unique CD table top pointer.

How does the hypervisor compute it's cache tag:
Intel: Each time a nesting domain is attached to a (RID,PASID) the
hypervisor driver will try to find a (DID,PASID) that is
already used by this domain, or allocate a new DID.
AMD: When creating the Nesting Domain the vDomain-ID should be passed
in. The hypervisor driver will allocate a unique pDomain-ID for
each vDomain-ID (hand wave). Several Nesting Domains will share
the same p/vDomain-ID.
ARM: The VMID is uniquely assigned to the Nesting Parent when it
is allocated, in some configurations it has to be shared with
KVM's VMID so allocating the Nesting Parent will require a KVM FD.

Will ATC be forwarded or synthesized:
Intel: The (vDomain-ID,PASID) is a unique nesting domain so
the hypervisor knows exactly which RIDs this nesting domain is
linked to and can generate an ATC invalidation. Plan is to
supress/discard the ATC invalidations from the VM and generate
them in the hypervisor.
AMD: (vDomain-ID,PASID) is ambiguous, it can refer to multiple GCR3
tables. We know which maximal set of RIDs it represents, but not
the actual set. I expect AMD will forward the ATC invalidation
to avoid over invalidation.
ARM: ASID is ambiguous. We have no idea which Nesting Domain/CD table
the ASID is contained in. ARM must forward the ATC invalidation
from the guest.

What iommufd object should receive the IOTLB invalidation command list:
Intel: The Nesting domain. The command list has to be broken up per
(vDomain-ID,PASID) and that batch delivered to the single
nesting domain. Kernel ignores vDomain-ID/PASID and just
invalidates whatever the nesting domain is actually attached to
AMD: Any Nesting Domain in the vDomain-ID group. The command list has
to be broken up per (vDomain-ID). Kernel replaces
vDomain-ID with pDomain-ID from the nesting domain and executes
the invalidation.
ARM: The Nesting Parent domain. Kernel forces the VMID from the
Nesting Parent and executes the invalidation.

In all cases the VM issues an ATC invalidation with (vRID, PASID) as
the tag. The VMM must translate vRID -> dev_id -> pRID

For a pure SW flow the vRID can be mapped to the dev_id and the ATC
invalidation delivered to the device object (eg IOMMUFD_DEV_INVALIDATE)

Finally, we have the HW driven invalidation DMA queues that can be
directly assigned to the guest. AMD and SMMUv3+vCMDQ support this. In
this case the HW is directly processing invalidation commands without
a hypervisor trap.

To make this work the iommu needs to be programmed with:
AMD: A vDomain-ID -> pDomain-ID table
A vRID -> pRID table
This is all bound to some "virtual function"
ARM: A vRID -> pRID table
The vCMDQ is bound to a VM_ID, so to the Nesting Parent

For AMD, as above, I suggest the vDomain-ID be passed when creating
the nesting domain.

The AMD "virtual function".. It is probably best to create a new iommufd
object for this and it can be passed in to a few places

The vRID->pRID table should be some mostly common
IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. AMD will need to pass in the virtual
function ID and ARM will need to pass in the Nesting Parent ID.

For the HW path some function will create the command queue and
DMA/mmap it. Taking in the virtual function/nesting parent as the
handle to associate it with.

For a SW path:
AMD: All invalidations can be delivered to the virtual function
and the kernel can use the vDomainID/vRID tables to translate
them fully
ARM: All invalidations can be delivered to the nesting parent

In many ways the nesting parent/virtual function are very similar
things. Perhaps ARM should also create a virtual function object which
is just welded to the nesting parent for API consistency.

So.. In short.. Invalidation is a PITA. The idea is the same but
annoying little details interfere with actually having a compltely
common API here. IMHO the uAPI in this series is fine. It will support
Intel invalidation and non-ATC invalidation on AMD/ARM. It should be
setup to allow that the target domain object can be any HWPT.

ARM will be able to do IOTLB invalidation using this API.

IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
(and just force the stream ID, userspace must direct the vRID to the
correct dev_id).

Then in yet another series we can tackle the entire "virtual function"
vRID/pRID translation stuff when the mmapable queue thing is
introduced.

Thus next steps:
- Respin this and lets focus on Intel only (this will be tough for
the holidays, but if it is available I will try)
- Get an ARM patch that just does IOTLB invalidation and add it to my
part 3
- Start working on IOMMUFD_DEV_INVALIDATE along with an ARM
implementation of it
- Reorganize the AMD RFC broadly along these lines and lets see it
freshened up in the next months as well. I would like to see the
AMD support structured to implement the SW paths in first steps and
later add in the "virtual function" acceleration stuff. The latter
is going to be complex.

Jason

2023-12-11 02:30:19

[permalink] [raw]

Subject: RE: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

> From: Jason Gunthorpe <[email protected]>
> Sent: Saturday, December 9, 2023 9:47 AM
>
> What is in a Nested domain:
> Intel: A single IO page table refereed to by a PASID entry
> Each vDomain-ID,PASID allocates a unique nesting domain
> AMD: A GCR3 table pointer
> Nesting domains are created for every unique GCR3 pointer.
> vDomain-ID can possibly refer to multiple Nesting domains :(
> ARM: A CD table pointer
> Nesting domains are created for every unique CD table top pointer.

this AMD/ARM difference is not very clear to me.

How could a vDomain-ID refer to multiple GCR3 pointers? Wouldn't it
lead to cache tag conflict when a same PASID entry in multiple GCR3 tables
points to different I/O page tables?

2023-12-11 12:33:35

by Yi Liu

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On 2023/12/9 09:47, Jason Gunthorpe wrote:
> On Fri, Nov 17, 2023 at 05:07:11AM -0800, Yi Liu wrote:
>
>> Take Intel VT-d as an example, the stage-1 translation table is I/O page
>> table. 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.
>
> I've been looking at what the three HW's need for invalidation, it is
> a bit messy.. Here is my thinking. Please let me know if I got it right
>
> What is the starting point of the guest memory walks:
> Intel: Single Scalable Mode PASID table entry indexed by a RID & PASID
> AMD: GCR3 table (a table of PASIDs) indexed by RID
> ARM: CD table (a table of PASIDs) indexed by RID
>
> What key does the physical HW use for invalidation:
> Intel: Domain-ID (stored in hypervisor, per PASID), PASID
> AMD: Domain-ID (stored in hypervisor, per RID), PASID
> ARM: VMID (stored in hypervisor, per RID), ASID (stored in guest)
>
> Why key does the VM use for invalidation:
> Intel: vDomain-ID (per PASID), PASID
> AMD: vDomain-ID (per RID), PASID
> ARM: ASID
>
> What is in a Nested domain:
> Intel: A single IO page table refereed to by a PASID entry
> Each vDomain-ID,PASID allocates a unique nesting domain
> AMD: A GCR3 table pointer
> Nesting domains are created for every unique GCR3 pointer.
> vDomain-ID can possibly refer to multiple Nesting domains :(

Per section '2.2.6.3 Guest CR3 Table' in below spec, DTE has domainID,
and it points to a guest CR3 Table (it should be a set of guest CRs3)
which is indexed by PASID. So it looks like the PASID is per-DommainID.
HW should use domainID+PASID to tag the cache, otherwise there would be
cache conflict...

https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf

> ARM: A CD table pointer
> Nesting domains are created for every unique CD table top pointer.
>
> How does the hypervisor compute it's cache tag:
> Intel: Each time a nesting domain is attached to a (RID,PASID) the
> hypervisor driver will try to find a (DID,PASID) that is
> already used by this domain, or allocate a new DID.
> AMD: When creating the Nesting Domain the vDomain-ID should be passed
> in. The hypervisor driver will allocate a unique pDomain-ID for
> each vDomain-ID (hand wave). Several Nesting Domains will share
> the same p/vDomain-ID.
> ARM: The VMID is uniquely assigned to the Nesting Parent when it
> is allocated, in some configurations it has to be shared with
> KVM's VMID so allocating the Nesting Parent will require a KVM FD.
>
> Will ATC be forwarded or synthesized:
> Intel: The (vDomain-ID,PASID) is a unique nesting domain so
> the hypervisor knows exactly which RIDs this nesting domain is
> linked to and can generate an ATC invalidation. Plan is to
> supress/discard the ATC invalidations from the VM and generate
> them in the hypervisor.
> AMD: (vDomain-ID,PASID) is ambiguous, it can refer to multiple GCR3
> tables. We know which maximal set of RIDs it represents, but not
> the actual set. I expect AMD will forward the ATC invalidation
> to avoid over invalidation.
> ARM: ASID is ambiguous. We have no idea which Nesting Domain/CD table
> the ASID is contained in. ARM must forward the ATC invalidation
> from the guest.
>
> What iommufd object should receive the IOTLB invalidation command list:
> Intel: The Nesting domain. The command list has to be broken up per
> (vDomain-ID,PASID) and that batch delivered to the single
> nesting domain. Kernel ignores vDomain-ID/PASID and just
> invalidates whatever the nesting domain is actually attached to

this is what we are doing in current series[1]. is it? Guest needs to
issue invalidation request with vDomain-ID and PASID (if it is enabled),
and affected pages for sure. But in hypervisor side, use vDomainID and
PASID info to figure out the target HWPT, then invoke a cache invalidation
on the HWPT with the invalidation range is enough. Kernel can figure out
what device/pasid this HWPT has been attached and invalidate the caches.

[1]
https://lore.kernel.org/linux-iommu/[email protected]/

> AMD: Any Nesting Domain in the vDomain-ID group. The command list has
> to be broken up per (vDomain-ID). Kernel replaces
> vDomain-ID with pDomain-ID from the nesting domain and executes
> the invalidation.
> ARM: The Nesting Parent domain. Kernel forces the VMID from the
> Nesting Parent and executes the invalidation.
>
> In all cases the VM issues an ATC invalidation with (vRID, PASID) as
> the tag. The VMM must translate vRID -> dev_id -> pRID
>
> For a pure SW flow the vRID can be mapped to the dev_id and the ATC
> invalidation delivered to the device object (eg IOMMUFD_DEV_INVALIDATE)
>
> Finally, we have the HW driven invalidation DMA queues that can be
> directly assigned to the guest. AMD and SMMUv3+vCMDQ support this. In
> this case the HW is directly processing invalidation commands without
> a hypervisor trap.
>
> To make this work the iommu needs to be programmed with:
> AMD: A vDomain-ID -> pDomain-ID table
> A vRID -> pRID table
> This is all bound to some "virtual function"
> ARM: A vRID -> pRID table
> The vCMDQ is bound to a VM_ID, so to the Nesting Parent
>
> For AMD, as above, I suggest the vDomain-ID be passed when creating
> the nesting domain.
>
> The AMD "virtual function".. It is probably best to create a new iommufd
> object for this and it can be passed in to a few places
>
> The vRID->pRID table should be some mostly common
> IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. AMD will need to pass in the virtual
> function ID and ARM will need to pass in the Nesting Parent ID.
>
> For the HW path some function will create the command queue and
> DMA/mmap it. Taking in the virtual function/nesting parent as the
> handle to associate it with.
>
> For a SW path:
> AMD: All invalidations can be delivered to the virtual function
> and the kernel can use the vDomainID/vRID tables to translate
> them fully
> ARM: All invalidations can be delivered to the nesting parent
>
> In many ways the nesting parent/virtual function are very similar
> things. Perhaps ARM should also create a virtual function object which
> is just welded to the nesting parent for API consistency.
>
> So.. In short.. Invalidation is a PITA. The idea is the same but
> annoying little details interfere with actually having a compltely
> common API here. IMHO the uAPI in this series is fine. It will support
> Intel invalidation and non-ATC invalidation on AMD/ARM. It should be
> setup to allow that the target domain object can be any HWPT.

This HWPT is still nested domain. Is it? But it can represent a guest I/O
page table (VT-d), guest CD table (ARM), guest CR3 Table (AMD, it seems to
be a set of guest CR3 table pointers). May ARM and AMD guys keep me honest
here.

The Intel guest I/O page table case may be the simplest as userspace only
needs to provide the HWPT ID and the affected ranges for invalidating. As
mentioned above, kernel will find out the attached device/pasid and
invalidating cache with the device/pasid. For ARM and AMD case, extra
information is needed. Am I getting you correct?

>
> ARM will be able to do IOTLB invalidation using this API.
>
> IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
> HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
> (and just force the stream ID, userspace must direct the vRID to the
> correct dev_id).
>
> Then in yet another series we can tackle the entire "virtual function"
> vRID/pRID translation stuff when the mmapable queue thing is
> introduced.
>
> Thus next steps:
> - Respin this and lets focus on Intel only (this will be tough for
> the holidays, but if it is available I will try)

I've respinned the iommufd cache invalidation part with the change to
report error_code/error_data per invalidation entry. yet still busy on
making Intel VTd part to report the error_code. Besides, I didn't see
other respin needed for Intel VT-d invalidation. If I missed thing, please
do let me know.:)

> - Get an ARM patch that just does IOTLB invalidation and add it to my
> part 3
> - Start working on IOMMUFD_DEV_INVALIDATE along with an ARM
> implementation of it
> - Reorganize the AMD RFC broadly along these lines and lets see it
> freshened up in the next months as well. I would like to see the
> AMD support structured to implement the SW paths in first steps and
> later add in the "virtual function" acceleration stuff. The latter
> is going to be complex.
>
> Jason

--
Regards,
Yi Liu

2023-12-11 12:34:31

by Yi Liu

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On 2023/12/11 10:29, Tian, Kevin wrote:
>> From: Jason Gunthorpe <[email protected]>
>> Sent: Saturday, December 9, 2023 9:47 AM
>>
>> What is in a Nested domain:
>> Intel: A single IO page table refereed to by a PASID entry
>> Each vDomain-ID,PASID allocates a unique nesting domain
>> AMD: A GCR3 table pointer
>> Nesting domains are created for every unique GCR3 pointer.
>> vDomain-ID can possibly refer to multiple Nesting domains :(
>> ARM: A CD table pointer
>> Nesting domains are created for every unique CD table top pointer.
>
> this AMD/ARM difference is not very clear to me.
>
> How could a vDomain-ID refer to multiple GCR3 pointers? Wouldn't it
> lead to cache tag conflict when a same PASID entry in multiple GCR3 tables
> points to different I/O page tables?

Perhaps due to only one DomainID in the DTE table indexed by BDF? Actually,
the vDomainID will not be used to tag cache, the host DomainId would be
used instead. @Jason?

--
Regards,
Yi Liu

2023-12-11 13:06:12

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 08:36:46PM +0800, Yi Liu wrote:
> On 2023/12/11 10:29, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <[email protected]>
> > > Sent: Saturday, December 9, 2023 9:47 AM
> > >
> > > What is in a Nested domain:
> > > Intel: A single IO page table refereed to by a PASID entry
> > > Each vDomain-ID,PASID allocates a unique nesting domain
> > > AMD: A GCR3 table pointer
> > > Nesting domains are created for every unique GCR3 pointer.
> > > vDomain-ID can possibly refer to multiple Nesting domains :(
> > > ARM: A CD table pointer
> > > Nesting domains are created for every unique CD table top pointer.
> >
> > this AMD/ARM difference is not very clear to me.
> >
> > How could a vDomain-ID refer to multiple GCR3 pointers? Wouldn't it
> > lead to cache tag conflict when a same PASID entry in multiple GCR3 tables
> > points to different I/O page tables?
>
> Perhaps due to only one DomainID in the DTE table indexed by BDF? Actually,
> the vDomainID will not be used to tag cache, the host DomainId would be
> used instead. @Jason?

The DomainID comes from the DTE table which is indexed by the RID, and
the DTE entry points to the GCR3 table. So the VM certainly can setup
a DTE table with multiple entires having the same vDomainID but
pointing to different GCR3's. So the VMM has to do *something* with
this.

Most likely this is not a useful thing to do. However what should the
VMM do when it sees this? Block a random DTE or push the duplication
down to real HW would be my options. I'd probably try to do the latter
just on the basis of better emulation.

Jason

2023-12-11 13:21:18

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 08:35:09PM +0800, Yi Liu wrote:

> > What iommufd object should receive the IOTLB invalidation command list:
> > Intel: The Nesting domain. The command list has to be broken up per
> > (vDomain-ID,PASID) and that batch delivered to the single
> > nesting domain. Kernel ignores vDomain-ID/PASID and just
> > invalidates whatever the nesting domain is actually attached to
>
> this is what we are doing in current series[1]. is it?

I think so

> > So.. In short.. Invalidation is a PITA. The idea is the same but
> > annoying little details interfere with actually having a compltely
> > common API here. IMHO the uAPI in this series is fine. It will support
> > Intel invalidation and non-ATC invalidation on AMD/ARM. It should be
> > setup to allow that the target domain object can be any HWPT.
>
> This HWPT is still nested domain. Is it? But it can represent a guest I/O
> page table (VT-d), guest CD table (ARM), guest CR3 Table (AMD, it seems to
> be a set of guest CR3 table pointers). May ARM and AMD guys keep me honest
> here.

I was thinking ARM would not want to use a nested domain because
really the invalidation is global to the entire nesting parent.

But, there is an issue with that - the nesting parent could be
attached to multiple iommu instances but we only want to invalidate a
single instance.

However, specifying the nesting domain instead, and restricting the
nesting domain to be single-instance would provide the kernel enough
information without providing weird new APIs. So maybe it is best to
just make everything use the nesting domain even if it is somewhat
semantically weird on arm.

> The Intel guest I/O page table case may be the simplest as userspace only
> needs to provide the HWPT ID and the affected ranges for invalidating. As
> mentioned above, kernel will find out the attached device/pasid and
> invalidating cache with the device/pasid. For ARM and AMD case, extra
> information is needed. Am I getting you correct?

Yes

> > Thus next steps:
> > - Respin this and lets focus on Intel only (this will be tough for
> > the holidays, but if it is available I will try)
>
> I've respinned the iommufd cache invalidation part with the change to
> report error_code/error_data per invalidation entry. yet still busy on
> making Intel VTd part to report the error_code. Besides, I didn't see
> other respin needed for Intel VT-d invalidation. If I missed thing, please
> do let me know.:)

Lets see

Jason

2023-12-11 15:34:39

by Suthikulpanit, Suravee

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On 12/11/2023 8:05 PM, Jason Gunthorpe wrote:
> On Mon, Dec 11, 2023 at 08:36:46PM +0800, Yi Liu wrote:
>> On 2023/12/11 10:29, Tian, Kevin wrote:
>>>> From: Jason Gunthorpe <[email protected]>
>>>> Sent: Saturday, December 9, 2023 9:47 AM
>>>>
>>>> What is in a Nested domain:
>>>> Intel: A single IO page table refereed to by a PASID entry
>>>> Each vDomain-ID,PASID allocates a unique nesting domain
>>>> AMD: A GCR3 table pointer
>>>> Nesting domains are created for every unique GCR3 pointer.
>>>> vDomain-ID can possibly refer to multiple Nesting domains :(
>>>> ARM: A CD table pointer
>>>> Nesting domains are created for every unique CD table top pointer.
>>>
>>> this AMD/ARM difference is not very clear to me.
>>>
>>> How could a vDomain-ID refer to multiple GCR3 pointers? Wouldn't it
>>> lead to cache tag conflict when a same PASID entry in multiple GCR3 tables
>>> points to different I/O page tables?
>>
>> Perhaps due to only one DomainID in the DTE table indexed by BDF? Actually,
>> the vDomainID will not be used to tag cache, the host DomainId would be
>> used instead. @Jason?
>
> The DomainID comes from the DTE table which is indexed by the RID, and
> the DTE entry points to the GCR3 table. So the VM certainly can setup
> a DTE table with multiple entires having the same vDomainID but
> pointing to different GCR3's. So the VMM has to do *something* with
> this.
>
> Most likely this is not a useful thing to do. However what should the
> VMM do when it sees this? Block a random DTE or push the duplication
> down to real HW would be my options. I'd probably try to do the latter
> just on the basis of better emulation.
>
> Jason

For AMD, the hardware uses host DomainID (hDomainId) and PASID to tag
the IOMMU TLB.

The VM can setup vDomainID independently from device (RID) and
hDomainID. The vDomainId->hDomainId mapping would be managed by the host
IOMMU driver (since this is also needed by the HW when enabling the
HW-vIOMMU support a.k.a virtual function).

Currently, the AMD IOMMU driver allocates a DomainId per IOMMU group.
One issue with this is when we have nested translation where we could
end up with multiple devices (RIDs) sharing same PASID and the same
hDomainID.

For example:

- Host view
Device1 (RID 1) w/ hDomainId 1
Device2 (RID 2) w/ hDomainId 1
- Guest view
Pass-through Device1 (vRID 3) w/ vDomainID A + PASID 0
Pass-through Device2 (vRID 4) w/ vDomainID B + PASID 0

We should be able to workaround this by changing the way we assign
hDomainId to be per-device for VFIO pass-through devices although
sharing the same v1 (stage-2) page table. This would look like.

- Host view
Device1 (RID 1) w/ hDomainId 1
Device2 (RID 2) w/ hDomainId 2
- Guest view
Pass-through Device1 (vRID 3) w/ vDomainID A + PASID 0
Pass-through Device2 (vRID 4) w/ vDomainID B + PASID 0

This should avoid the IOMMU TLB conflict. However, the invalidation
would need to be done for both DomainId 1 and 2 when updating the v1
(stage-2) page table.

Thanks,
Suravee

2023-12-11 16:08:22

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 10:34:09PM +0700, Suthikulpanit, Suravee wrote:

> Currently, the AMD IOMMU driver allocates a DomainId per IOMMU group.
> One issue with this is when we have nested translation where we could end up
> with multiple devices (RIDs) sharing same PASID and the same hDomainID.

Which means you also create multiple GCR3 tables since those are
(soon) per-device and we end up with the situation I described for a
functional legitimate reason :( It is just wasting memory by
duplicating GCR3 tables.

> For example:
>
> - Host view
> Device1 (RID 1) w/ hDomainId 1
> Device2 (RID 2) w/ hDomainId 1

So.. Groups are another ugly mess that we may have to do something
more robust about.

The group infrastructure assumes that all devices in the group have
the same translation. This is not how the VM communicates, each member
of the group gets to have its own DTE and there are legitimate cases
where the DTEs will be different (even if just temporarily)

How to mesh this is not yet solved (most likely we need to allow group
members to have temporarily different translation). But in the long
run the group should definately not be providing the cache tag, the
driver has to be smarter than this.

I think we talked about this before.. For the AMD driver the v1 page
table should store the domainid in the iommu_domain and that value
should be used everywhere

For modes with a GCR3 table the best you can do is to de-duplicate the
GCR3 tables and assign identical GCR3 tables to identical domain ids.
Ie all devices in a group will eventually share GCR3 tables so they
can converge on the same domain id.

> - Guest view
> Pass-through Device1 (vRID 3) w/ vDomainID A + PASID 0
> Pass-through Device2 (vRID 4) w/ vDomainID B + PASID 0
>
> We should be able to workaround this by changing the way we assign hDomainId
> to be per-device for VFIO pass-through devices although sharing the same v1
> (stage-2) page table. This would look like.

As I said, this doesn't quite work since the VM could do other
things. The kernel must be aware of the vDomainID and must select an
appropriate hDomainID with that knowledge in mind, otherwise
multi-device-groups in guests are fully broken.

> - Guest view
> Pass-through Device1 (vRID 3) w/ vDomainID A + PASID 0
> Pass-through Device2 (vRID 4) w/ vDomainID B + PASID 0
>
> This should avoid the IOMMU TLB conflict. However, the invalidation would
> need to be done for both DomainId 1 and 2 when updating the v1 (stage-2)
> page table.

Which is the key problem, if the VM thinks it has only one vDomainID
the VMM can't split that into two hDomainID's and expect the viommu
acceleration will work - so we shouldn't try to make it work in SW
either, IMHO.

Jason

2023-12-11 17:36:03

by Suthikulpanit, Suravee

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On 12/9/2023 8:47 AM, Jason Gunthorpe wrote:
> On Fri, Nov 17, 2023 at 05:07:11AM -0800, Yi Liu wrote:
>
>> Take Intel VT-d as an example, the stage-1 translation table is I/O page
>> table. 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.
>
> I've been looking at what the three HW's need for invalidation, it is
> a bit messy.. Here is my thinking. Please let me know if I got it right
>
> What is the starting point of the guest memory walks:
> Intel: Single Scalable Mode PASID table entry indexed by a RID & PASID
> AMD: GCR3 table (a table of PASIDs) indexed by RID

GCR3 table is indexed by PASID.
Device Table (DTE) is indexted by DeviceID (RID)

> ...
> Will ATC be forwarded or synthesized:
> Intel: The (vDomain-ID,PASID) is a unique nesting domain so
> the hypervisor knows exactly which RIDs this nesting domain is
> linked to and can generate an ATC invalidation. Plan is to
> supress/discard the ATC invalidations from the VM and generate
> them in the hypervisor.
> AMD: (vDomain-ID,PASID) is ambiguous, it can refer to multiple GCR3
> tables. We know which maximal set of RIDs it represents, but not
> the actual set. I expect AMD will forward the ATC invalidation
> to avoid over invalidation.

Not sure I understand your description here.

For the AMD IOMMU INVALIDE_IOMMU_PAGES (i.e. invalidate the IOMMU TLB),
the hypervisor needs to map gDomainId->hDomainId and issue the command
on behalf of the VM along with the PASID and GVA (or GVA range) provided
by the guest.

For the AMD IOMMU INVALIDE_IOTLB_PAGES (i.e. invalidate the ATC on the
device), the hypervisor needs to map gDeviceId->hDeviceId and issue the
command on behalf of the VM along with the PASID and GVA (or GVA range)
provided by the guest.

> ARM: ASID is ambiguous. We have no idea which Nesting Domain/CD table
> the ASID is contained in. ARM must forward the ATC invalidation
> from the guest.
>
> What iommufd object should receive the IOTLB invalidation command list:
> Intel: The Nesting domain. The command list has to be broken up per
> (vDomain-ID,PASID) and that batch delivered to the single
> nesting domain. Kernel ignores vDomain-ID/PASID and just
> invalidates whatever the nesting domain is actually attached to
> AMD: Any Nesting Domain in the vDomain-ID group. The command list has
> to be broken up per (vDomain-ID). Kernel replaces
> vDomain-ID with pDomain-ID from the nesting domain and executes
> the invalidation.
> ARM: The Nesting Parent domain. Kernel forces the VMID from the
> Nesting Parent and executes the invalidation.
>
> In all cases the VM issues an ATC invalidation with (vRID, PASID) as
> the tag. The VMM must translate vRID -> dev_id -> pRID
>
> For a pure SW flow the vRID can be mapped to the dev_id and the ATC
> invalidation delivered to the device object (eg IOMMUFD_DEV_INVALIDATE)
>
> Finally, we have the HW driven invalidation DMA queues that can be
> directly assigned to the guest. AMD and SMMUv3+vCMDQ support this. In
> this case the HW is directly processing invalidation commands without
> a hypervisor trap.
>
> To make this work the iommu needs to be programmed with:
> AMD: A vDomain-ID -> pDomain-ID table
> A vRID -> pRID table
> This is all bound to some "virtual function"

By "virtual function", I assume you are referring to the AMD vIOMMU
instance in the guest?

> ARM: A vRID -> pRID table
> The vCMDQ is bound to a VM_ID, so to the Nesting Parent
>
> For AMD, as above, I suggest the vDomain-ID be passed when creating
> the nesting domain
Sure, we can do this part.

> The AMD "virtual function".. It is probably best to create a new iommufd
> object for this and it can be passed in to a few places

Something like IOMMUFD_OBJ_VIOMMU? Then operation would include
something like:
* Init
* Destroy
* ...

> The vRID->pRID table should be some mostly common
> IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. AMD will need to pass in the virtual
> function ID and ARM will need to pass in the Nesting Parent ID.

Ok.

> ...
> Thus next steps:
> - Respin this and lets focus on Intel only (this will be tough for
> the holidays, but if it is available I will try)
> - Get an ARM patch that just does IOTLB invalidation and add it to my
> part 3
> - Start working on IOMMUFD_DEV_INVALIDATE along with an ARM
> implementation of it
> - Reorganize the AMD RFC broadly along these lines and lets see it
> freshened up in the next months as well. I would like to see the
> AMD support structured to implement the SW paths in first steps and
> later add in the "virtual function" acceleration stuff. The latter
> is going to be complex.

Working on refining the part 1 to add HW info reporting and nested
translation (minus the invalidation stuff). Should be sending out soon.

Suravee

2023-12-11 17:46:39

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Tue, Dec 12, 2023 at 12:35:26AM +0700, Suthikulpanit, Suravee wrote:
>
>
> On 12/9/2023 8:47 AM, Jason Gunthorpe wrote:
> > On Fri, Nov 17, 2023 at 05:07:11AM -0800, Yi Liu wrote:
> >
> > > Take Intel VT-d as an example, the stage-1 translation table is I/O page
> > > table. 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.
> >
> > I've been looking at what the three HW's need for invalidation, it is
> > a bit messy.. Here is my thinking. Please let me know if I got it right
> >
> > What is the starting point of the guest memory walks:
> > Intel: Single Scalable Mode PASID table entry indexed by a RID & PASID
> > AMD: GCR3 table (a table of PASIDs) indexed by RID
>
> GCR3 table is indexed by PASID.
> Device Table (DTE) is indexted by DeviceID (RID)

Yes, this is what I was trying to say

> > Will ATC be forwarded or synthesized:
> > Intel: The (vDomain-ID,PASID) is a unique nesting domain so
> > the hypervisor knows exactly which RIDs this nesting domain is
> > linked to and can generate an ATC invalidation. Plan is to
> > supress/discard the ATC invalidations from the VM and generate
> > them in the hypervisor.
> > AMD: (vDomain-ID,PASID) is ambiguous, it can refer to multiple GCR3
> > tables. We know which maximal set of RIDs it represents, but not
> > the actual set. I expect AMD will forward the ATC invalidation
> > to avoid over invalidation.
>
> Not sure I understand your description here.
>
> For the AMD IOMMU INVALIDE_IOMMU_PAGES (i.e. invalidate the IOMMU TLB), the
> hypervisor needs to map gDomainId->hDomainId and issue the command on behalf
> of the VM along with the PASID and GVA (or GVA range) provided by the guest.

Yes, that is the "forwarding" approach. Contrast this to the Intel
approach where the ATC is synthesized by the kernel emulating the
INVALIDE_IOMMU_PAGES

> > To make this work the iommu needs to be programmed with:
> > AMD: A vDomain-ID -> pDomain-ID table
> > A vRID -> pRID table
> > This is all bound to some "virtual function"
>
> By "virtual function", I assume you are referring to the AMD vIOMMU instance
> in the guest?

Yes, but it is not in the guest, it has to be some concrete iommufd
object.

> Something like IOMMUFD_OBJ_VIOMMU? Then operation would include something
> like:
> * Init
> * Destroy
> * ...

Yes, something like that. It needs to be able to work for ARM vCMDQ
stuff too. I don't know what the name should be. Maybe viommu is OK
for now.

- Alloc viommu (against a single iommu instance?)
- Assign a virtual ID to an iommufd device within the same instance
- Setup a submission and completion queue in userspace memory
- mmap the doorbell page (both need this?)
- Route back completion interrupts via eventfd

When you get here you and Nicolin should work out something along
those lines that works for both

But I'd like to keep things in steps, so if we can get info, nesting
parent, nesting domain and SW IOTLB and ATC invalidation as the first
(two?) steps that would be great

> > Thus next steps:
> > - Respin this and lets focus on Intel only (this will be tough for
> > the holidays, but if it is available I will try)
> > - Get an ARM patch that just does IOTLB invalidation and add it to my
> > part 3
> > - Start working on IOMMUFD_DEV_INVALIDATE along with an ARM
> > implementation of it
> > - Reorganize the AMD RFC broadly along these lines and lets see it
> > freshened up in the next months as well. I would like to see the
> > AMD support structured to implement the SW paths in first steps and
> > later add in the "virtual function" acceleration stuff. The latter
> > is going to be complex.
>
> Working on refining the part 1 to add HW info reporting and nested
> translation (minus the invalidation stuff). Should be sending out soon.

Nice!

Jason

2023-12-11 20:12:03

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 09:20:41AM -0400, Jason Gunthorpe wrote:
> On Mon, Dec 11, 2023 at 08:35:09PM +0800, Yi Liu wrote:
> > > So.. In short.. Invalidation is a PITA. The idea is the same but
> > > annoying little details interfere with actually having a compltely
> > > common API here. IMHO the uAPI in this series is fine. It will support
> > > Intel invalidation and non-ATC invalidation on AMD/ARM. It should be
> > > setup to allow that the target domain object can be any HWPT.
> >
> > This HWPT is still nested domain. Is it? But it can represent a guest I/O
> > page table (VT-d), guest CD table (ARM), guest CR3 Table (AMD, it seems to
> > be a set of guest CR3 table pointers). May ARM and AMD guys keep me honest
> > here.
>
> I was thinking ARM would not want to use a nested domain because
> really the invalidation is global to the entire nesting parent.
>
> But, there is an issue with that - the nesting parent could be
> attached to multiple iommu instances but we only want to invalidate a
> single instance.

I am still not sure about attaching an S2 domain to multiple
SMMUs. An S2 domain is created per SMMU, and we have such a
rejection in arm_smmu_attach_dev():
} else if (smmu_domain->smmu != smmu)
ret = -EINVAL;

I understand that it would be probably ideal to share the S2
iopt among the SMMUs. But in the driver the objects (domain)
holding a shared S2 iopt must be different to allocate their
own VMIDs, right?

Thanks
Nicolin

2023-12-11 21:28:11

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Fri, Dec 08, 2023 at 09:47:26PM -0400, Jason Gunthorpe wrote:
> What is in a Nested domain:
> ARM: A CD table pointer
> Nesting domains are created for every unique CD table top pointer.

I think we basically implemented in a way of syncing STE, i,e,
vSTE.Config must be "S1 Translate" besides a CD table pointer,
and a nested domain is freed when vSTE.Config=BYPASS even if a
CD table pointer is present, right?

> To make this work the iommu needs to be programmed with:
> AMD: A vDomain-ID -> pDomain-ID table
> A vRID -> pRID table
> This is all bound to some "virtual function"
> ARM: A vRID -> pRID table
> The vCMDQ is bound to a VM_ID, so to the Nesting Parent

VCMDQ also has something called "virtual interface" that holds
a VMID and a list of CMDQ queues, which might be a bit similar
to AMD's "virtual function".

> For AMD, as above, I suggest the vDomain-ID be passed when creating
> the nesting domain.
>
> The AMD "virtual function".. It is probably best to create a new iommufd
> object for this and it can be passed in to a few places
>
> The vRID->pRID table should be some mostly common
> IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. AMD will need to pass in the virtual
> function ID and ARM will need to pass in the Nesting Parent ID.

It sounds like our previous IOMMUFD_SET/UNSET_IDEV_DATA. I'm
wondering if we need to make it exclusive to the ID assigning?
Maybe set_idev_data could be reused for other potential cases?

If we do implement an IOMMUFD_DEV_ASSIGN_VIRTUAL_ID, do we need
an IOMMUFD_DEV_RESIGN_VIRTUAL_ID? (or better word than resign).

Could the structure just look like this?
struct iommu_dev_assign_virtual_id {
__u32 size;
__u32 dev_id;
__u32 id_type;
__u32 id;
};

> In many ways the nesting parent/virtual function are very similar
> things. Perhaps ARM should also create a virtual function object which
> is just welded to the nesting parent for API consistency.

A virtual function that holds an S2 domain/iopt + a VMID? If
this is for VCMDQ, the VMCDQ extension driver has that kinda
object holding an S2 domain: I implemented as the extension
function at the end of arm_smmu_finalise_s2() previously.

> So.. In short.. Invalidation is a PITA. The idea is the same but
> annoying little details interfere with actually having a compltely
> common API here. IMHO the uAPI in this series is fine. It will support
> Intel invalidation and non-ATC invalidation on AMD/ARM. It should be
> setup to allow that the target domain object can be any HWPT.
>
> ARM will be able to do IOTLB invalidation using this API.
>
> IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
> HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
> (and just force the stream ID, userspace must direct the vRID to the
> correct dev_id).

SMMU's CD invalidations could fall into this category too.

> Then in yet another series we can tackle the entire "virtual function"
> vRID/pRID translation stuff when the mmapable queue thing is
> introduced.

VCMDQ is also a mmapable queue. I feel that there could be
more common stuff between "virtual function" and "virtual
interface", I'll need to take a look at AMD's stuff though.

I previously drafted something to test it out with iommufd.
Basically it needs the pairing of vRID/pRID in attach_dev()
and another ioctl to mmap/config user queue(s):
+struct iommu_hwpt_cache_config_tegra241_vcmdq {
+ __u32 vcmdq_id; // queue id
+ __u32 vcmdq_log2size; // queue size
+ __aligned_u64 vcmdq_base; // queue guest PA
+};

> Thus next steps:
> - Get an ARM patch that just does IOTLB invalidation and add it to my
> part 3
> - Start working on IOMMUFD_DEV_INVALIDATE along with an ARM
> implementation of it.

I will work on these two, presumably including the new
IOMMUFD_DEV_ASSIGN_VIRTUAL_ID or so.

Thanks
Nicolin

2023-12-11 21:49:08

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 12:11:25PM -0800, Nicolin Chen wrote:
> On Mon, Dec 11, 2023 at 09:20:41AM -0400, Jason Gunthorpe wrote:
> > On Mon, Dec 11, 2023 at 08:35:09PM +0800, Yi Liu wrote:
> > > > So.. In short.. Invalidation is a PITA. The idea is the same but
> > > > annoying little details interfere with actually having a compltely
> > > > common API here. IMHO the uAPI in this series is fine. It will support
> > > > Intel invalidation and non-ATC invalidation on AMD/ARM. It should be
> > > > setup to allow that the target domain object can be any HWPT.
> > >
> > > This HWPT is still nested domain. Is it? But it can represent a guest I/O
> > > page table (VT-d), guest CD table (ARM), guest CR3 Table (AMD, it seems to
> > > be a set of guest CR3 table pointers). May ARM and AMD guys keep me honest
> > > here.
> >
> > I was thinking ARM would not want to use a nested domain because
> > really the invalidation is global to the entire nesting parent.
> >
> > But, there is an issue with that - the nesting parent could be
> > attached to multiple iommu instances but we only want to invalidate a
> > single instance.
>
> I am still not sure about attaching an S2 domain to multiple
> SMMUs. An S2 domain is created per SMMU, and we have such a
> rejection in arm_smmu_attach_dev():
> } else if (smmu_domain->smmu != smmu)
> ret = -EINVAL;

I intend to remove that eventually

> I understand that it would be probably ideal to share the S2
> iopt among the SMMUs. But in the driver the objects (domain)
> holding a shared S2 iopt must be different to allocate their
> own VMIDs, right?

No, the vmid will be moved into the struct arm_smmu_master_domain

Jason

2023-12-11 21:57:55

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 01:27:49PM -0800, Nicolin Chen wrote:
> On Fri, Dec 08, 2023 at 09:47:26PM -0400, Jason Gunthorpe wrote:
> > What is in a Nested domain:
> > ARM: A CD table pointer
> > Nesting domains are created for every unique CD table top pointer.
>
> I think we basically implemented in a way of syncing STE, i,e,
> vSTE.Config must be "S1 Translate" besides a CD table pointer,
> and a nested domain is freed when vSTE.Config=BYPASS even if a
> CD table pointer is present, right?

Yes, but you can also de-duplicate the nested domains based on the CD
table pointer. It is not as critical for ARM as others, but may
still be worth doing.

> > To make this work the iommu needs to be programmed with:
> > AMD: A vDomain-ID -> pDomain-ID table
> > A vRID -> pRID table
> > This is all bound to some "virtual function"
> > ARM: A vRID -> pRID table
> > The vCMDQ is bound to a VM_ID, so to the Nesting Parent
>
> VCMDQ also has something called "virtual interface" that holds
> a VMID and a list of CMDQ queues, which might be a bit similar
> to AMD's "virtual function".

Yeah, there must be some kind of logical grouping of HW objects to
build that kind of stuff.

> > The vRID->pRID table should be some mostly common
> > IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. AMD will need to pass in the virtual
> > function ID and ARM will need to pass in the Nesting Parent ID.
>
> It sounds like our previous IOMMUFD_SET/UNSET_IDEV_DATA. I'm
> wondering if we need to make it exclusive to the ID assigning?
> Maybe set_idev_data could be reused for other potential cases?

No, it should be an API only for the ID

> If we do implement an IOMMUFD_DEV_ASSIGN_VIRTUAL_ID, do we need
> an IOMMUFD_DEV_RESIGN_VIRTUAL_ID? (or better word than resign).

I don't think so.. The vRID is basically fixed, if it needs to be
changed then the device can be destroyed (or assign can just change it)

> Could the structure just look like this?
> struct iommu_dev_assign_virtual_id {
> __u32 size;
> __u32 dev_id;
> __u32 id_type;
> __u32 id;
> };

It needs to take in the viommu_id also, and I'd make the id 64 bits
just for good luck.

> > In many ways the nesting parent/virtual function are very similar
> > things. Perhaps ARM should also create a virtual function object which
> > is just welded to the nesting parent for API consistency.
>
> A virtual function that holds an S2 domain/iopt + a VMID? If
> this is for VCMDQ, the VMCDQ extension driver has that kinda
> object holding an S2 domain: I implemented as the extension
> function at the end of arm_smmu_finalise_s2() previously.

Not so much hold a S2, but that the VMID would be forced to be shared
amung them somehow.

> > IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
> > HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
> > (and just force the stream ID, userspace must direct the vRID to the
> > correct dev_id).
>
> SMMU's CD invalidations could fall into this category too.

Yes, I forgot to look closely at the CD/GCR3 table invalidations :(
I actually can't tell how AMD invalidates any GCR3 cache, maybe
INVALIDATE_DEVTAB_ENTRY?

> > Then in yet another series we can tackle the entire "virtual function"
> > vRID/pRID translation stuff when the mmapable queue thing is
> > introduced.
>
> VCMDQ is also a mmapable queue. I feel that there could be
> more common stuff between "virtual function" and "virtual
> interface", I'll need to take a look at AMD's stuff though.

I'm not thinking of two things right now at least..

> I previously drafted something to test it out with iommufd.
> Basically it needs the pairing of vRID/pRID in attach_dev()
> and another ioctl to mmap/config user queue(s):
> +struct iommu_hwpt_cache_config_tegra241_vcmdq {
> + __u32 vcmdq_id; // queue id
> + __u32 vcmdq_log2size; // queue size
> + __aligned_u64 vcmdq_base; // queue guest PA
> +};

vRID/pRID pairing should come from IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. When
a HWPT is allocated it would be connected to the viommu_id and then it
would all be bundled together in the HW somehow

From there you can ask the viommu_id to setup a queue.

Jason

2023-12-12 07:30:25

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 05:57:38PM -0400, Jason Gunthorpe wrote:
> On Mon, Dec 11, 2023 at 01:27:49PM -0800, Nicolin Chen wrote:
> > On Fri, Dec 08, 2023 at 09:47:26PM -0400, Jason Gunthorpe wrote:
> > > What is in a Nested domain:
> > > ARM: A CD table pointer
> > > Nesting domains are created for every unique CD table top pointer.
> >
> > I think we basically implemented in a way of syncing STE, i,e,
> > vSTE.Config must be "S1 Translate" besides a CD table pointer,
> > and a nested domain is freed when vSTE.Config=BYPASS even if a
> > CD table pointer is present, right?
>
> Yes, but you can also de-duplicate the nested domains based on the CD
> table pointer. It is not as critical for ARM as others, but may
> still be worth doing.

Ah right! Should do that.

> > If we do implement an IOMMUFD_DEV_ASSIGN_VIRTUAL_ID, do we need
> > an IOMMUFD_DEV_RESIGN_VIRTUAL_ID? (or better word than resign).
>
> I don't think so.. The vRID is basically fixed, if it needs to be
> changed then the device can be destroyed (or assign can just change it)

OK. Will insert the cleanup piece to the unbind()/destroy().

> > Could the structure just look like this?
> > struct iommu_dev_assign_virtual_id {
> > __u32 size;
> > __u32 dev_id;
> > __u32 id_type;
> > __u32 id;
> > };
>
> It needs to take in the viommu_id also, and I'd make the id 64 bits
> just for good luck.

What is viommu_id required for in this context? I thought we
already know which SMMU instance to issue commands via dev_id?

> > > IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
> > > HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
> > > (and just force the stream ID, userspace must direct the vRID to the
> > > correct dev_id).
> >
> > SMMU's CD invalidations could fall into this category too.

Do we need a different iommu API for this ioctl? We currently
have the cache_invalidate_user as a domain op. The new device
op will be an iommu op?

And should we rename the "cache_invalidate_user"? Would VT-d
still uses it for device cache?

> > I previously drafted something to test it out with iommufd.
> > Basically it needs the pairing of vRID/pRID in attach_dev()
> > and another ioctl to mmap/config user queue(s):
> > +struct iommu_hwpt_cache_config_tegra241_vcmdq {
> > + __u32 vcmdq_id; // queue id
> > + __u32 vcmdq_log2size; // queue size
> > + __aligned_u64 vcmdq_base; // queue guest PA
> > +};
>
> vRID/pRID pairing should come from IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. When
> a HWPT is allocated it would be connected to the viommu_id and then it
> would all be bundled together in the HW somehow

Since we were talking about sharing stage-2 domain, the HWPT
to the stage-2 domain will be shared among the vIOMMU/pIOMMU
instances too? I think I am not quite getting the viommu_id
part yet. From the other side of this thread, viommu object
is created per vIOMMU instance and each one actually tied to
a pIOMMU by nature?

Thanks
Nicolin

2023-12-12 14:46:19

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Mon, Dec 11, 2023 at 11:30:00PM -0800, Nicolin Chen wrote:

> > > Could the structure just look like this?
> > > struct iommu_dev_assign_virtual_id {
> > > __u32 size;
> > > __u32 dev_id;
> > > __u32 id_type;
> > > __u32 id;
> > > };
> >
> > It needs to take in the viommu_id also, and I'd make the id 64 bits
> > just for good luck.
>
> What is viommu_id required for in this context? I thought we
> already know which SMMU instance to issue commands via dev_id?

The viommu_id would be the container that holds the xarray that maps
the vRID to pRID

Logically we could have multiple mappings per iommufd as we could have
multiple iommu instances working here.

> > > > IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
> > > > HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
> > > > (and just force the stream ID, userspace must direct the vRID to the
> > > > correct dev_id).
> > >
> > > SMMU's CD invalidations could fall into this category too.
>
> Do we need a different iommu API for this ioctl? We currently
> have the cache_invalidate_user as a domain op. The new device
> op will be an iommu op?

Yes

> And should we rename the "cache_invalidate_user"? Would VT-d
> still uses it for device cache?

I think vt-d will not implement it

> > > I previously drafted something to test it out with iommufd.
> > > Basically it needs the pairing of vRID/pRID in attach_dev()
> > > and another ioctl to mmap/config user queue(s):
> > > +struct iommu_hwpt_cache_config_tegra241_vcmdq {
> > > + __u32 vcmdq_id; // queue id
> > > + __u32 vcmdq_log2size; // queue size
> > > + __aligned_u64 vcmdq_base; // queue guest PA
> > > +};
> >
> > vRID/pRID pairing should come from IOMMUFD_DEV_ASSIGN_VIRTUAL_ID. When
> > a HWPT is allocated it would be connected to the viommu_id and then it
> > would all be bundled together in the HW somehow
>
> Since we were talking about sharing stage-2 domain, the HWPT
> to the stage-2 domain will be shared among the vIOMMU/pIOMMU
> instances too?

The HWPT for the stage 2 should be shared

> I think I am not quite getting the viommu_id
> part yet. From the other side of this thread, viommu object
> is created per vIOMMU instance and each one actually tied to
> a pIOMMU by nature?

Yes

Jason

2023-12-12 19:14:20

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Tue, Dec 12, 2023 at 10:44:21AM -0400, Jason Gunthorpe wrote:
> On Mon, Dec 11, 2023 at 11:30:00PM -0800, Nicolin Chen wrote:
>
> > > > Could the structure just look like this?
> > > > struct iommu_dev_assign_virtual_id {
> > > > __u32 size;
> > > > __u32 dev_id;
> > > > __u32 id_type;
> > > > __u32 id;
> > > > };
> > >
> > > It needs to take in the viommu_id also, and I'd make the id 64 bits
> > > just for good luck.
> >
> > What is viommu_id required for in this context? I thought we
> > already know which SMMU instance to issue commands via dev_id?
>
> The viommu_id would be the container that holds the xarray that maps
> the vRID to pRID
>
> Logically we could have multiple mappings per iommufd as we could have
> multiple iommu instances working here.

I see. This is the object to hold a shared stage-2 HWPT/domain then.

// iommufd_private.h

enum iommufd_object_type {
...
+ IOMMUFD_OBJ_VIOMMU,
...
};

+struct iommufd_viommu {
+ struct iommufd_object obj;
+ struct iommufd_hwpt_paging *hwpt;
+ struct xarray devices;
+};

struct iommufd_hwpt_paging hwpt {
...
+ struct list_head viommu_list;
...
};

struct iommufd_group {
...
+ struct iommufd_viommu *viommu; // should we attach to viommu instead of hwpt?
...
};

Question to finalize how we maps vRID-pRID in the xarray:
how should IOMMUFD_DEV_INVALIDATE work? The ioctl structure has
a dev_id and a list of commands that belongs to the device. So,
it forwards the struct device pointer to the driver along with
the commands. Then, doesn't the driver already know the pRID
from the dev pointer without looking up a vRID-pRID table?

// uapi/linux/iommufd.h

struct iommu_hwpt_alloc {
...
+ __u32 viommu_id;
};

+enum iommu_dev_virtual_id_type {
+ IOMMU_DEV_VIRTUAL_ID_TYPE_AMD_VIOMMU_DID, // not sure how this fits the xarray in viommu obj.
+ IOMMU_DEV_VIRTUAL_ID_TYPE_AMD_VIOMMU_RID,
+ IOMMU_DEV_VIRTUAL_ID_TYPE_ARM_SMMU_SID,
+};
+
+struct iommu_dev_assign_virtual_id {
+ __u32 size;
+ __u32 dev_id;
+ __u32 viommu_id;
+ __u32 id_type;
+ __aligned_u64 id;
+};

Then, I think that we also need an iommu_viommu_alloc structure
and ioctl to allocate an object, and that VMM should know if it
needs to allocate multiple viommu objects -- this probably needs
the hw_info ioctl to return a piommu_id so VMM gets the list of
piommus from the attached devices?

Another question, introducing the viommu obj complicates things
a lot. Do we want it to come with iommu_dev_assign_virtual_id,
or maybe put in a later series? We could stage the xarray in the
iommu_hwpt_paging struct for now, so a single-IOMMU system could
still work with that.

> > > > > IOMMUFD_DEV_INVALIDATE should be introduced with the same design as
> > > > > HWPT invalidate. This would be used for AMD/ARM's ATC invalidation
> > > > > (and just force the stream ID, userspace must direct the vRID to the
> > > > > correct dev_id).
> > > >
> > > > SMMU's CD invalidations could fall into this category too.
> >
> > Do we need a different iommu API for this ioctl? We currently
> > have the cache_invalidate_user as a domain op. The new device
> > op will be an iommu op?
>
> Yes

OK. FWIW, I think either VMM or iommufd core knows which nested
HWPT the device is attached to. If we ever wanted to keep it in
the domain op list, we could still do that by passing in extra
domain pointer to the driver.

> > And should we rename the "cache_invalidate_user"? Would VT-d
> > still uses it for device cache?
>
> I think vt-d will not implement it

Then should we "s/cache_invalidate_user/iotlb_sync_user"?

Thanks
Nicolin

2023-12-12 19:21:18

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Tue, Dec 12, 2023 at 11:13:37AM -0800, Nicolin Chen wrote:
> On Tue, Dec 12, 2023 at 10:44:21AM -0400, Jason Gunthorpe wrote:
> > On Mon, Dec 11, 2023 at 11:30:00PM -0800, Nicolin Chen wrote:
> >
> > > > > Could the structure just look like this?
> > > > > struct iommu_dev_assign_virtual_id {
> > > > > __u32 size;
> > > > > __u32 dev_id;
> > > > > __u32 id_type;
> > > > > __u32 id;
> > > > > };
> > > >
> > > > It needs to take in the viommu_id also, and I'd make the id 64 bits
> > > > just for good luck.
> > >
> > > What is viommu_id required for in this context? I thought we
> > > already know which SMMU instance to issue commands via dev_id?
> >
> > The viommu_id would be the container that holds the xarray that maps
> > the vRID to pRID
> >
> > Logically we could have multiple mappings per iommufd as we could have
> > multiple iommu instances working here.
>
> I see. This is the object to hold a shared stage-2 HWPT/domain then.

It could be done like that, yes. I wasn't thinking about linking the
stage two so tightly but perhaps? If we can avoid putting the hwpt
here that might be more general.

> // iommufd_private.h
>
> enum iommufd_object_type {
> ...
> + IOMMUFD_OBJ_VIOMMU,
> ...
> };
>
> +struct iommufd_viommu {
> + struct iommufd_object obj;
> + struct iommufd_hwpt_paging *hwpt;
> + struct xarray devices;
> +};
>
> struct iommufd_hwpt_paging hwpt {
> ...
> + struct list_head viommu_list;
> ...
> };

I'd probably first try to go backwards and link the hwpt to the
viommu.

> struct iommufd_group {
> ...
> + struct iommufd_viommu *viommu; // should we attach to viommu instead of hwpt?
> ...
> };

No. Attach is a statement of translation so you still attach to the HWPT.

> Question to finalize how we maps vRID-pRID in the xarray:
> how should IOMMUFD_DEV_INVALIDATE work? The ioctl structure has
> a dev_id and a list of commands that belongs to the device. So,
> it forwards the struct device pointer to the driver along with
> the commands. Then, doesn't the driver already know the pRID
> from the dev pointer without looking up a vRID-pRID table?

The first version of DEV_INVALIDATE should have no xarray. The
invalidate commands are stripped of the SID and executed on the given
dev_id period. VMM splits up the invalidate command list.

The second version maybe we have the xarray, or maybe we just push the
xarray to the eventual viommu series.

> struct iommu_hwpt_alloc {
> ...
> + __u32 viommu_id;
> };
>
> +enum iommu_dev_virtual_id_type {
> + IOMMU_DEV_VIRTUAL_ID_TYPE_AMD_VIOMMU_DID, // not sure how this fits the xarray in viommu obj.
> + IOMMU_DEV_VIRTUAL_ID_TYPE_AMD_VIOMMU_RID,

It is just DID. In both cases the ID is the index to the "STE" radix
tree, whatever the driver happens to call it.

> Then, I think that we also need an iommu_viommu_alloc structure
> and ioctl to allocate an object, and that VMM should know if it
> needs to allocate multiple viommu objects -- this probably needs
> the hw_info ioctl to return a piommu_id so VMM gets the list of
> piommus from the attached devices?

Yes and yes

> Another question, introducing the viommu obj complicates things
> a lot. Do we want it to come with iommu_dev_assign_virtual_id,
> or maybe put in a later series? We could stage the xarray in the
> iommu_hwpt_paging struct for now, so a single-IOMMU system could
> still work with that.

All this would be in its own series to enable HW accelerated viommu
support on ARM & AMD as we've been doing so far.

I imagine it after we get the basic invalidation done

> > > And should we rename the "cache_invalidate_user"? Would VT-d
> > > still uses it for device cache?
> >
> > I think vt-d will not implement it
>
> Then should we "s/cache_invalidate_user/iotlb_sync_user"?

I think cache_invalidate is still a fine name.. vt-d will generate ATC
invalidations under that function too.

Jason

2023-12-12 20:06:24

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Tue, Dec 12, 2023 at 03:21:00PM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 11:13:37AM -0800, Nicolin Chen wrote:
> > On Tue, Dec 12, 2023 at 10:44:21AM -0400, Jason Gunthorpe wrote:
> > > On Mon, Dec 11, 2023 at 11:30:00PM -0800, Nicolin Chen wrote:
> > >
> > > > > > Could the structure just look like this?
> > > > > > struct iommu_dev_assign_virtual_id {
> > > > > > __u32 size;
> > > > > > __u32 dev_id;
> > > > > > __u32 id_type;
> > > > > > __u32 id;
> > > > > > };
> > > > >
> > > > > It needs to take in the viommu_id also, and I'd make the id 64 bits
> > > > > just for good luck.
> > > >
> > > > What is viommu_id required for in this context? I thought we
> > > > already know which SMMU instance to issue commands via dev_id?
> > >
> > > The viommu_id would be the container that holds the xarray that maps
> > > the vRID to pRID
> > >
> > > Logically we could have multiple mappings per iommufd as we could have
> > > multiple iommu instances working here.
> >
> > I see. This is the object to hold a shared stage-2 HWPT/domain then.
>
> It could be done like that, yes. I wasn't thinking about linking the
> stage two so tightly but perhaps? If we can avoid putting the hwpt
> here that might be more general.
>
> > // iommufd_private.h
> >
> > enum iommufd_object_type {
> > ...
> > + IOMMUFD_OBJ_VIOMMU,
> > ...
> > };
> >
> > +struct iommufd_viommu {
> > + struct iommufd_object obj;
> > + struct iommufd_hwpt_paging *hwpt;
> > + struct xarray devices;
> > +};
> >
> > struct iommufd_hwpt_paging hwpt {
> > ...
> > + struct list_head viommu_list;
> > ...
> > };
>
> I'd probably first try to go backwards and link the hwpt to the
> viommu.

I think a VM should have only one hwpt_paging object while one
or more viommu objects, so we could do only viommu->hwpt_paging
and hwpt_paging->viommu_list. How to go backwards?

> > struct iommufd_group {
> > ...
> > + struct iommufd_viommu *viommu; // should we attach to viommu instead of hwpt?
> > ...
> > };
>
> No. Attach is a statement of translation so you still attach to the HWPT.

OK. It's probably not necessary since we know which piommu the
device is behind. And we only need to link viommu and piommu,
right?

> > Question to finalize how we maps vRID-pRID in the xarray:
> > how should IOMMUFD_DEV_INVALIDATE work? The ioctl structure has
> > a dev_id and a list of commands that belongs to the device. So,
> > it forwards the struct device pointer to the driver along with
> > the commands. Then, doesn't the driver already know the pRID
> > from the dev pointer without looking up a vRID-pRID table?
>
> The first version of DEV_INVALIDATE should have no xarray. The
> invalidate commands are stripped of the SID and executed on the given
> dev_id period. VMM splits up the invalidate command list.

Yes. This makes sense to me. VMM knows which device to issue
an IOMMUFD_DEV_INVALIDATE from the vSID/vRID in the commands.

> The second version maybe we have the xarray, or maybe we just push the
> xarray to the eventual viommu series.

I think that I still don't get the purpose of the xarray here.
It was needed previously because a cache invalidate per hwpt
doesn't know which device. Now IOMMUFD_DEV_INVALIDATE knows.

Maybe it's related to that narrative "logically we could have
multiple mappings per iommufd" that you mentioned above. Mind
elaborating a bit?

In my mind, viommu is allocated by VMM per piommu, by detecting
the piommu_id via hw_info. In that case, viommu can only have
one unique device list. If IOMMUFD_DEV_INVALIDATE passes in the
dev_id, we don't really need a mapping of vRID-pRID in a multi-
viommu case either? In another word, VMM already has a mapping
from vRID to dev_id, so it could call the DEV_INVALIDATE ioctl
in the first place?

Thanks
Nicolin

2023-12-13 12:41:20

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] iommufd: Add nesting infrastructure (part 2/2)

On Tue, Dec 12, 2023 at 12:05:41PM -0800, Nicolin Chen wrote:
> > > // iommufd_private.h
> > >
> > > enum iommufd_object_type {
> > > ...
> > > + IOMMUFD_OBJ_VIOMMU,
> > > ...
> > > };
> > >
> > > +struct iommufd_viommu {
> > > + struct iommufd_object obj;
> > > + struct iommufd_hwpt_paging *hwpt;
> > > + struct xarray devices;
> > > +};
> > >
> > > struct iommufd_hwpt_paging hwpt {
> > > ...
> > > + struct list_head viommu_list;
> > > ...
> > > };
> >
> > I'd probably first try to go backwards and link the hwpt to the
> > viommu.
>
> I think a VM should have only one hwpt_paging object while one
> or more viommu objects, so we could do only viommu->hwpt_paging
> and hwpt_paging->viommu_list. How to go backwards?

That is probably how things would work but I don't know if it makes
sense to enforce it in the kernel logic..

Point the S2 to a list of viommu objects it is linked to

> > > struct iommufd_group {
> > > ...
> > > + struct iommufd_viommu *viommu; // should we attach to viommu instead of hwpt?
> > > ...
> > > };
> >
> > No. Attach is a statement of translation so you still attach to the HWPT.
>
> OK. It's probably not necessary since we know which piommu the
> device is behind. And we only need to link viommu and piommu,
> right?

Yes

> > The second version maybe we have the xarray, or maybe we just push the
> > xarray to the eventual viommu series.
>
> I think that I still don't get the purpose of the xarray here.
> It was needed previously because a cache invalidate per hwpt
> doesn't know which device. Now IOMMUFD_DEV_INVALIDATE knows.
>
> Maybe it's related to that narrative "logically we could have
> multiple mappings per iommufd" that you mentioned above. Mind
> elaborating a bit?
>
> In my mind, viommu is allocated by VMM per piommu, by detecting
> the piommu_id via hw_info. In that case, viommu can only have
> one unique device list. If IOMMUFD_DEV_INVALIDATE passes in the
> dev_id, we don't really need a mapping of vRID-pRID in a multi-
> viommu case either? In another word, VMM already has a mapping
> from vRID to dev_id, so it could call the DEV_INVALIDATE ioctl
> in the first place?

The xarray exists to optimize the invalidation flow.

For SW you can imagine issuing an invalidation against the viommu
itself and *all* commands, be they ASID or ATC invalidations can be
processed in one shot. The xarray allows converting the vSID to pSID
to process ATC invalidations, and the viommu object forces a single
VMID to handle the ATC invalidations. If we want to do this, I don't
know.

For HW, the xarray holds the vSID to pSID mapping that must be
programmed into the HW operating the dedicated invalidation queue.

Jason

2023-12-13 20:02:54