Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On Wed, 15 May 2019 16:52:46 +0100 Jean-Philippe Brucker wrote: > On 14/05/2019 18:55, Jacob Pan wrote: > > Yes, I agree to replace the standalone __64 pasid with this struct. > > Looks more inline with address selective info., Just to double > > confirm the new struct. > > > > Jean, will you put this in your sva/api repo? > > Yes, I pushed it along with some documentation fixes (mainly getting > rid of scripts/kernel-doc warnings and outputting valid rst) > Just pulled, I am rebasing on top of this branch. If you could also include our api for bind guest pasid, then we have a complete set of common APIs in one place. https://lkml.org/lkml/2019/5/3/775 I just need to add a small tweak for supporting non-identity guest-host PASID mapping for the next version. > Thanks, > Jean [Jacob Pan] ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On 14/05/2019 18:55, Jacob Pan wrote: > Yes, I agree to replace the standalone __64 pasid with this struct. > Looks more inline with address selective info., Just to double confirm > the new struct. > > Jean, will you put this in your sva/api repo? Yes, I pushed it along with some documentation fixes (mainly getting rid of scripts/kernel-doc warnings and outputting valid rst) Thanks, Jean
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On 15/05/2019 15:47, Tian, Kevin wrote: >> From: Jean-Philippe Brucker >> Sent: Wednesday, May 15, 2019 7:04 PM >> >> On 14/05/2019 18:44, Jacob Pan wrote: >>> Hi Thank you both for the explanation. >>> >>> On Tue, 14 May 2019 11:41:24 +0100 >>> Jean-Philippe Brucker wrote: >>> On 14/05/2019 08:36, Auger Eric wrote: > Hi Jacob, > > On 5/14/19 12:16 AM, Jacob Pan wrote: >> On Mon, 13 May 2019 18:09:48 +0100 >> Jean-Philippe Brucker wrote: >> >>> On 13/05/2019 17:50, Auger Eric wrote: > struct iommu_inv_pasid_info { > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > __u32 flags; > __u32 archid; > __u64 pasid; > }; I agree it does the job now. However it looks a bit strange to do a PASID based invalidation in my case - SMMUv3 nested stage - where I don't have any PASID involved. Couldn't we call it context based invalidation then? A context can be tagged by a PASID or/and an ARCHID. >>> >>> I think calling it "context" would be confusing as well (I >>> shouldn't have used it earlier), since VT-d uses that name for >>> device table entries (=STE on Arm SMMU). Maybe "addr_space"? >>> >> I am still struggling to understand what ARCHID is after scanning >> through SMMUv3.1 spec. It seems to be a constant for a given SMMU. >> Why do you need to pass it down every time? Could you point to me >> the document or explain a little more on ARCHID use cases. >> We have three fileds called pasid under this struct >> iommu_cache_invalidate_info{} >> Gets confusing :) > archid is a generic term. That's why you did not find it in the > spec ;-) > > On ARM SMMU the archid is called the ASID (Address Space ID, up to > 16 bits. The ASID is stored in the Context Descriptor Entry (your > PASID entry) and thus characterizes a given stage 1 translation > "context"/"adress space". Yes, another way to look at it is, for a given address space: * PASID tags device-IOTLB (ATC) entries. * ASID (here called archid) tags IOTLB entries. They could have the same value, but it depends on the guest's allocation policy which isn't in our control. With my PASID patches for SMMUv3, they have different values. So we need both fields if we intend to invalidate both ATC and IOTLB with a single call. >>> For ASID invalidation, there is also page/address selective within an >>> ASID, right? I guess it is CMD_TLBI_NH_VA? >>> So the single call to invalidate both ATC & IOTLB should share the same >>> address information. i.e. >>> struct iommu_inv_addr_info {} >>> >>> Just out of curiosity, what is the advantage of having guest tag its >>> ATC with its own PASID? I thought you were planning to use custom >>> ioasid allocator to get PASID from host. >> >> Hm, for the moment I mostly considered the custom ioasid allocator for >> Intel platforms. On Arm platforms the SR-IOV model where each VM has its >> own PASID space is still very much on the table. This would be the only >> model supported by a vSMMU emulation for example, since the SMMU >> doesn't >> have PASID allocation commands. >> > > I didn't get how ATS works in such case, if device ATC PASID is different > from IOTLB ASID. Who will be responsible for translation in-between? ATS with the SMMU works like this: * The PCI function sends a Translation Request with PASID. * The SMMU walks the PASID table (which we call context descriptor table), finds the context descriptor indexed by PASID. This context descriptor has an ASID field, and a page directory pointer. * After successfully walking the page tables, the SMMU may add an IOTLB entry tagged by ASID and address, then returns a Translation Completion. * The PCI function adds an ATC entry tagged by PASID and address. I think the ASID on Arm CPUs is roughly equivalent to Intel PCID. One reason we use ASIDs for IOTLBs is that with SVA, the ASID of an address space is the same on the CPU side. And when the CPU executes a TLB invalidation instructions, it also invalidates the corresponding IOTLB entries. It's nice for vSVA because you don't need to context-switch to the host to send an IOTLB invalidation. But only non-PCI devices that implement SVA benefit from this at the moment, because ATC invalidations still have to go through the SMMU command queue. Thanks, Jean
RE: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
> From: Jean-Philippe Brucker > Sent: Wednesday, May 15, 2019 7:04 PM > > On 14/05/2019 18:44, Jacob Pan wrote: > > Hi Thank you both for the explanation. > > > > On Tue, 14 May 2019 11:41:24 +0100 > > Jean-Philippe Brucker wrote: > > > >> On 14/05/2019 08:36, Auger Eric wrote: > >>> Hi Jacob, > >>> > >>> On 5/14/19 12:16 AM, Jacob Pan wrote: > On Mon, 13 May 2019 18:09:48 +0100 > Jean-Philippe Brucker wrote: > > > On 13/05/2019 17:50, Auger Eric wrote: > >>> struct iommu_inv_pasid_info { > >>> #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > >>> #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > >>> __u32 flags; > >>> __u32 archid; > >>> __u64 pasid; > >>> }; > >> I agree it does the job now. However it looks a bit strange to > >> do a PASID based invalidation in my case - SMMUv3 nested stage - > >> where I don't have any PASID involved. > >> > >> Couldn't we call it context based invalidation then? A context > >> can be tagged by a PASID or/and an ARCHID. > > > > I think calling it "context" would be confusing as well (I > > shouldn't have used it earlier), since VT-d uses that name for > > device table entries (=STE on Arm SMMU). Maybe "addr_space"? > > > I am still struggling to understand what ARCHID is after scanning > through SMMUv3.1 spec. It seems to be a constant for a given SMMU. > Why do you need to pass it down every time? Could you point to me > the document or explain a little more on ARCHID use cases. > We have three fileds called pasid under this struct > iommu_cache_invalidate_info{} > Gets confusing :) > >>> archid is a generic term. That's why you did not find it in the > >>> spec ;-) > >>> > >>> On ARM SMMU the archid is called the ASID (Address Space ID, up to > >>> 16 bits. The ASID is stored in the Context Descriptor Entry (your > >>> PASID entry) and thus characterizes a given stage 1 translation > >>> "context"/"adress space". > >> > >> Yes, another way to look at it is, for a given address space: > >> * PASID tags device-IOTLB (ATC) entries. > >> * ASID (here called archid) tags IOTLB entries. > >> > >> They could have the same value, but it depends on the guest's > >> allocation policy which isn't in our control. With my PASID patches > >> for SMMUv3, they have different values. So we need both fields if we > >> intend to invalidate both ATC and IOTLB with a single call. > >> > > For ASID invalidation, there is also page/address selective within an > > ASID, right? I guess it is CMD_TLBI_NH_VA? > > So the single call to invalidate both ATC & IOTLB should share the same > > address information. i.e. > > struct iommu_inv_addr_info {} > > > > Just out of curiosity, what is the advantage of having guest tag its > > ATC with its own PASID? I thought you were planning to use custom > > ioasid allocator to get PASID from host. > > Hm, for the moment I mostly considered the custom ioasid allocator for > Intel platforms. On Arm platforms the SR-IOV model where each VM has its > own PASID space is still very much on the table. This would be the only > model supported by a vSMMU emulation for example, since the SMMU > doesn't > have PASID allocation commands. > I didn't get how ATS works in such case, if device ATC PASID is different from IOTLB ASID. Who will be responsible for translation in-between? Thanks Kevin
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On 14/05/2019 18:44, Jacob Pan wrote: > Hi Thank you both for the explanation. > > On Tue, 14 May 2019 11:41:24 +0100 > Jean-Philippe Brucker wrote: > >> On 14/05/2019 08:36, Auger Eric wrote: >>> Hi Jacob, >>> >>> On 5/14/19 12:16 AM, Jacob Pan wrote: On Mon, 13 May 2019 18:09:48 +0100 Jean-Philippe Brucker wrote: > On 13/05/2019 17:50, Auger Eric wrote: >>> struct iommu_inv_pasid_info { >>> #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) >>> #define IOMMU_INV_PASID_FLAGS_ARCHID(1 << 1) >>> __u32 flags; >>> __u32 archid; >>> __u64 pasid; >>> }; >> I agree it does the job now. However it looks a bit strange to >> do a PASID based invalidation in my case - SMMUv3 nested stage - >> where I don't have any PASID involved. >> >> Couldn't we call it context based invalidation then? A context >> can be tagged by a PASID or/and an ARCHID. > > I think calling it "context" would be confusing as well (I > shouldn't have used it earlier), since VT-d uses that name for > device table entries (=STE on Arm SMMU). Maybe "addr_space"? > I am still struggling to understand what ARCHID is after scanning through SMMUv3.1 spec. It seems to be a constant for a given SMMU. Why do you need to pass it down every time? Could you point to me the document or explain a little more on ARCHID use cases. We have three fileds called pasid under this struct iommu_cache_invalidate_info{} Gets confusing :) >>> archid is a generic term. That's why you did not find it in the >>> spec ;-) >>> >>> On ARM SMMU the archid is called the ASID (Address Space ID, up to >>> 16 bits. The ASID is stored in the Context Descriptor Entry (your >>> PASID entry) and thus characterizes a given stage 1 translation >>> "context"/"adress space". >> >> Yes, another way to look at it is, for a given address space: >> * PASID tags device-IOTLB (ATC) entries. >> * ASID (here called archid) tags IOTLB entries. >> >> They could have the same value, but it depends on the guest's >> allocation policy which isn't in our control. With my PASID patches >> for SMMUv3, they have different values. So we need both fields if we >> intend to invalidate both ATC and IOTLB with a single call. >> > For ASID invalidation, there is also page/address selective within an > ASID, right? I guess it is CMD_TLBI_NH_VA? > So the single call to invalidate both ATC & IOTLB should share the same > address information. i.e. > struct iommu_inv_addr_info {} > > Just out of curiosity, what is the advantage of having guest tag its > ATC with its own PASID? I thought you were planning to use custom > ioasid allocator to get PASID from host. Hm, for the moment I mostly considered the custom ioasid allocator for Intel platforms. On Arm platforms the SR-IOV model where each VM has its own PASID space is still very much on the table. This would be the only model supported by a vSMMU emulation for example, since the SMMU doesn't have PASID allocation commands. > Also ASID is 16 bit as Eric said and PASID (substreamID?) is 20 bit, > right? Yes. Some implementations have 8-bit ASIDs, but I think those would be on embedded rather than server class platforms. And yes, if it wasn't confusing enough, the Arm SMMU uses "SubstreamID" (SSID) for PASIDs :) Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On Tue, 14 May 2019 10:44:01 -0700 Jacob Pan wrote: > Hi Thank you both for the explanation. > > On Tue, 14 May 2019 11:41:24 +0100 > Jean-Philippe Brucker wrote: > > > On 14/05/2019 08:36, Auger Eric wrote: > > > Hi Jacob, > > > > > > On 5/14/19 12:16 AM, Jacob Pan wrote: > > >> On Mon, 13 May 2019 18:09:48 +0100 > > >> Jean-Philippe Brucker wrote: > > >> > > >>> On 13/05/2019 17:50, Auger Eric wrote: > > > struct iommu_inv_pasid_info { > > > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > > > #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > > > __u32 flags; > > > __u32 archid; > > > __u64 pasid; > > > }; > > I agree it does the job now. However it looks a bit strange to > > do a PASID based invalidation in my case - SMMUv3 nested stage > > - where I don't have any PASID involved. > > > > Couldn't we call it context based invalidation then? A context > > can be tagged by a PASID or/and an ARCHID. > > >>> > > >>> I think calling it "context" would be confusing as well (I > > >>> shouldn't have used it earlier), since VT-d uses that name for > > >>> device table entries (=STE on Arm SMMU). Maybe "addr_space"? > > >>> > > >> I am still struggling to understand what ARCHID is after scanning > > >> through SMMUv3.1 spec. It seems to be a constant for a given > > >> SMMU. Why do you need to pass it down every time? Could you > > >> point to me the document or explain a little more on ARCHID use > > >> cases. We have three fileds called pasid under this struct > > >> iommu_cache_invalidate_info{} > > >> Gets confusing :) > > > archid is a generic term. That's why you did not find it in the > > > spec ;-) > > > > > > On ARM SMMU the archid is called the ASID (Address Space ID, up to > > > 16 bits. The ASID is stored in the Context Descriptor Entry (your > > > PASID entry) and thus characterizes a given stage 1 translation > > > "context"/"adress space". > > > > Yes, another way to look at it is, for a given address space: > > * PASID tags device-IOTLB (ATC) entries. > > * ASID (here called archid) tags IOTLB entries. > > > > They could have the same value, but it depends on the guest's > > allocation policy which isn't in our control. With my PASID patches > > for SMMUv3, they have different values. So we need both fields if we > > intend to invalidate both ATC and IOTLB with a single call. > > > For ASID invalidation, there is also page/address selective within an > ASID, right? I guess it is CMD_TLBI_NH_VA? > So the single call to invalidate both ATC & IOTLB should share the > same address information. i.e. > struct iommu_inv_addr_info {} > Nevermind for this question. archid field is already in the addr_info. Sorry. > Just out of curiosity, what is the advantage of having guest tag its > ATC with its own PASID? I thought you were planning to use custom > ioasid allocator to get PASID from host. > > Also ASID is 16 bit as Eric said and PASID (substreamID?) is 20 bit, > right? > > > Thanks, > > Jean > > > > > > > > At the moment the ASID is allocated per iommu domain. With aux > > > domains we should have one ASID per aux domain, Jean-Philippe > > > said. > > > > > > ASID tags IOTLB S1 entries. As the ASID is part of the "context > > > descriptor" which is owned by the guest, the API must pass it > > > somehow. > > > > > > 4.4.1.2 CMD_TLBI_NH_ASID(VMID, ASID) invalidation command allows > > > to invalidate all IOTLB S1 entries for a given VMID/ASID and this > > > is the functionality which is currently missing in the API. This > > > is not an address based invalidation or a "pure" PASID based > > > invalidation. At the moment we don't support PASIDs on ARM and I > > > need this capability. > > > > Got it. > > > Thanks > > > > > > Eric > > > > > > > > > > > >>> Thanks, > > >>> Jean > > >>> > > > > Domain invalidation would invalidate all the contexts belonging > > to that domain. > > > > Thanks > > > > Eric > > >> > > >> [Jacob Pan] > > >> > > > > [Jacob Pan] [Jacob Pan]
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On Tue, 14 May 2019 13:02:47 +0200 Auger Eric wrote: > Hi Jean, > > On 5/14/19 12:42 PM, Jean-Philippe Brucker wrote: > > On 14/05/2019 08:46, Auger Eric wrote: > >> Hi Jean, > >> > >> On 5/13/19 7:09 PM, Jean-Philippe Brucker wrote: > >>> On 13/05/2019 17:50, Auger Eric wrote: > > struct iommu_inv_pasid_info { > > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > > #define IOMMU_INV_PASID_FLAGS_ARCHID(1 << 1) > > __u32 flags; > > __u32 archid; > > __u64 pasid; > > }; > I agree it does the job now. However it looks a bit strange to > do a PASID based invalidation in my case - SMMUv3 nested stage - > where I don't have any PASID involved. > > Couldn't we call it context based invalidation then? A context > can be tagged by a PASID or/and an ARCHID. > >>> > >>> I think calling it "context" would be confusing as well (I > >>> shouldn't have used it earlier), since VT-d uses that name for > >>> device table entries (=STE on Arm SMMU). Maybe "addr_space"? > >> yes you're right. Well we already pasid table table terminology so > >> we can use it here as well - as long as we understand what purpose > >> it serves ;-) - So OK for iommu_inv_pasid_info. > >> > >> I think Jean understood we would keep pasid standalone field in > I meant Jacob here. > >> iommu_cache_invalidate_info's union. I understand the struct > >> iommu_inv_pasid_info now would replace it, correct? > > Thank you for the confirmation. > Yes, I agree to replace the standalone __64 pasid with this struct. Looks more inline with address selective info., Just to double confirm the new struct. Jean, will you put this in your sva/api repo? struct iommu_cache_invalidate_info { #define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1 __u32 version; /* IOMMU paging structure cache */ #define IOMMU_CACHE_INV_TYPE_IOTLB (1 << 0) /* IOMMU IOTLB */ #define IOMMU_CACHE_INV_TYPE_DEV_IOTLB (1 << 1) /* Device IOTLB */ #define IOMMU_CACHE_INV_TYPE_PASID (1 << 2) /* PASID cache */ #define IOMMU_CACHE_TYPE_NR (3) __u8cache; __u8granularity; __u8padding[2]; union { struct iommu_inv_pasid_info pasid_info; struct iommu_inv_addr_info addr_info; }; };
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Thank you both for the explanation. On Tue, 14 May 2019 11:41:24 +0100 Jean-Philippe Brucker wrote: > On 14/05/2019 08:36, Auger Eric wrote: > > Hi Jacob, > > > > On 5/14/19 12:16 AM, Jacob Pan wrote: > >> On Mon, 13 May 2019 18:09:48 +0100 > >> Jean-Philippe Brucker wrote: > >> > >>> On 13/05/2019 17:50, Auger Eric wrote: > > struct iommu_inv_pasid_info { > > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > > #define IOMMU_INV_PASID_FLAGS_ARCHID(1 << 1) > > __u32 flags; > > __u32 archid; > > __u64 pasid; > > }; > I agree it does the job now. However it looks a bit strange to > do a PASID based invalidation in my case - SMMUv3 nested stage - > where I don't have any PASID involved. > > Couldn't we call it context based invalidation then? A context > can be tagged by a PASID or/and an ARCHID. > >>> > >>> I think calling it "context" would be confusing as well (I > >>> shouldn't have used it earlier), since VT-d uses that name for > >>> device table entries (=STE on Arm SMMU). Maybe "addr_space"? > >>> > >> I am still struggling to understand what ARCHID is after scanning > >> through SMMUv3.1 spec. It seems to be a constant for a given SMMU. > >> Why do you need to pass it down every time? Could you point to me > >> the document or explain a little more on ARCHID use cases. > >> We have three fileds called pasid under this struct > >> iommu_cache_invalidate_info{} > >> Gets confusing :) > > archid is a generic term. That's why you did not find it in the > > spec ;-) > > > > On ARM SMMU the archid is called the ASID (Address Space ID, up to > > 16 bits. The ASID is stored in the Context Descriptor Entry (your > > PASID entry) and thus characterizes a given stage 1 translation > > "context"/"adress space". > > Yes, another way to look at it is, for a given address space: > * PASID tags device-IOTLB (ATC) entries. > * ASID (here called archid) tags IOTLB entries. > > They could have the same value, but it depends on the guest's > allocation policy which isn't in our control. With my PASID patches > for SMMUv3, they have different values. So we need both fields if we > intend to invalidate both ATC and IOTLB with a single call. > For ASID invalidation, there is also page/address selective within an ASID, right? I guess it is CMD_TLBI_NH_VA? So the single call to invalidate both ATC & IOTLB should share the same address information. i.e. struct iommu_inv_addr_info {} Just out of curiosity, what is the advantage of having guest tag its ATC with its own PASID? I thought you were planning to use custom ioasid allocator to get PASID from host. Also ASID is 16 bit as Eric said and PASID (substreamID?) is 20 bit, right? > Thanks, > Jean > > > > > At the moment the ASID is allocated per iommu domain. With aux > > domains we should have one ASID per aux domain, Jean-Philippe said. > > > > ASID tags IOTLB S1 entries. As the ASID is part of the "context > > descriptor" which is owned by the guest, the API must pass it > > somehow. > > > > 4.4.1.2 CMD_TLBI_NH_ASID(VMID, ASID) invalidation command allows to > > invalidate all IOTLB S1 entries for a given VMID/ASID and this is > > the functionality which is currently missing in the API. This is > > not an address based invalidation or a "pure" PASID based > > invalidation. At the moment we don't support PASIDs on ARM and I > > need this capability. > > Got it. > > Thanks > > > > Eric > > > > > > > >>> Thanks, > >>> Jean > >>> > > Domain invalidation would invalidate all the contexts belonging > to that domain. > > Thanks > > Eric > >> > >> [Jacob Pan] > >> > [Jacob Pan] ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Jean, On 5/14/19 12:42 PM, Jean-Philippe Brucker wrote: > On 14/05/2019 08:46, Auger Eric wrote: >> Hi Jean, >> >> On 5/13/19 7:09 PM, Jean-Philippe Brucker wrote: >>> On 13/05/2019 17:50, Auger Eric wrote: > struct iommu_inv_pasid_info { > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > __u32 flags; > __u32 archid; > __u64 pasid; > }; I agree it does the job now. However it looks a bit strange to do a PASID based invalidation in my case - SMMUv3 nested stage - where I don't have any PASID involved. Couldn't we call it context based invalidation then? A context can be tagged by a PASID or/and an ARCHID. >>> >>> I think calling it "context" would be confusing as well (I shouldn't >>> have used it earlier), since VT-d uses that name for device table >>> entries (=STE on Arm SMMU). Maybe "addr_space"? >> yes you're right. Well we already pasid table table terminology so we >> can use it here as well - as long as we understand what purpose it >> serves ;-) - So OK for iommu_inv_pasid_info. >> >> I think Jean understood we would keep pasid standalone field in I meant Jacob here. >> iommu_cache_invalidate_info's union. I understand the struct >> iommu_inv_pasid_info now would replace it, correct? Thank you for the confirmation. Eric > > Yes > > Thanks, > Jean > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On 14/05/2019 08:36, Auger Eric wrote: > Hi Jacob, > > On 5/14/19 12:16 AM, Jacob Pan wrote: >> On Mon, 13 May 2019 18:09:48 +0100 >> Jean-Philippe Brucker wrote: >> >>> On 13/05/2019 17:50, Auger Eric wrote: > struct iommu_inv_pasid_info { > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > __u32 flags; > __u32 archid; > __u64 pasid; > }; I agree it does the job now. However it looks a bit strange to do a PASID based invalidation in my case - SMMUv3 nested stage - where I don't have any PASID involved. Couldn't we call it context based invalidation then? A context can be tagged by a PASID or/and an ARCHID. >>> >>> I think calling it "context" would be confusing as well (I shouldn't >>> have used it earlier), since VT-d uses that name for device table >>> entries (=STE on Arm SMMU). Maybe "addr_space"? >>> >> I am still struggling to understand what ARCHID is after scanning >> through SMMUv3.1 spec. It seems to be a constant for a given SMMU. Why >> do you need to pass it down every time? Could you point to me the >> document or explain a little more on ARCHID use cases. >> We have three fileds called pasid under this struct >> iommu_cache_invalidate_info{} >> Gets confusing :) > archid is a generic term. That's why you did not find it in the spec ;-) > > On ARM SMMU the archid is called the ASID (Address Space ID, up to 16 > bits. The ASID is stored in the Context Descriptor Entry (your PASID > entry) and thus characterizes a given stage 1 translation > "context"/"adress space". Yes, another way to look at it is, for a given address space: * PASID tags device-IOTLB (ATC) entries. * ASID (here called archid) tags IOTLB entries. They could have the same value, but it depends on the guest's allocation policy which isn't in our control. With my PASID patches for SMMUv3, they have different values. So we need both fields if we intend to invalidate both ATC and IOTLB with a single call. Thanks, Jean > > At the moment the ASID is allocated per iommu domain. With aux domains > we should have one ASID per aux domain, Jean-Philippe said. > > ASID tags IOTLB S1 entries. As the ASID is part of the "context > descriptor" which is owned by the guest, the API must pass it somehow. > > 4.4.1.2 CMD_TLBI_NH_ASID(VMID, ASID) invalidation command allows to > invalidate all IOTLB S1 entries for a given VMID/ASID and this is the > functionality which is currently missing in the API. This is not an > address based invalidation or a "pure" PASID based invalidation. At the > moment we don't support PASIDs on ARM and I need this capability. > > Thanks > > Eric > > > >>> Thanks, >>> Jean >>> Domain invalidation would invalidate all the contexts belonging to that domain. Thanks Eric >> >> [Jacob Pan] >>
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Jean, On 5/13/19 7:09 PM, Jean-Philippe Brucker wrote: > On 13/05/2019 17:50, Auger Eric wrote: >>> struct iommu_inv_pasid_info { >>> #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) >>> #define IOMMU_INV_PASID_FLAGS_ARCHID(1 << 1) >>> __u32 flags; >>> __u32 archid; >>> __u64 pasid; >>> }; >> I agree it does the job now. However it looks a bit strange to do a >> PASID based invalidation in my case - SMMUv3 nested stage - where I >> don't have any PASID involved. >> >> Couldn't we call it context based invalidation then? A context can be >> tagged by a PASID or/and an ARCHID. > > I think calling it "context" would be confusing as well (I shouldn't > have used it earlier), since VT-d uses that name for device table > entries (=STE on Arm SMMU). Maybe "addr_space"? yes you're right. Well we already pasid table table terminology so we can use it here as well - as long as we understand what purpose it serves ;-) - So OK for iommu_inv_pasid_info. I think Jean understood we would keep pasid standalone field in iommu_cache_invalidate_info's union. I understand the struct iommu_inv_pasid_info now would replace it, correct? Thanks Eric > > Thanks, > Jean > >> >> Domain invalidation would invalidate all the contexts belonging to that >> domain. >> >> Thanks >> >> Eric
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Jacob, On 5/14/19 12:16 AM, Jacob Pan wrote: > On Mon, 13 May 2019 18:09:48 +0100 > Jean-Philippe Brucker wrote: > >> On 13/05/2019 17:50, Auger Eric wrote: struct iommu_inv_pasid_info { #define IOMMU_INV_PASID_FLAGS_PASID(1 << 0) #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) __u32 flags; __u32 archid; __u64 pasid; }; >>> I agree it does the job now. However it looks a bit strange to do a >>> PASID based invalidation in my case - SMMUv3 nested stage - where I >>> don't have any PASID involved. >>> >>> Couldn't we call it context based invalidation then? A context can >>> be tagged by a PASID or/and an ARCHID. >> >> I think calling it "context" would be confusing as well (I shouldn't >> have used it earlier), since VT-d uses that name for device table >> entries (=STE on Arm SMMU). Maybe "addr_space"? >> > I am still struggling to understand what ARCHID is after scanning > through SMMUv3.1 spec. It seems to be a constant for a given SMMU. Why > do you need to pass it down every time? Could you point to me the > document or explain a little more on ARCHID use cases. > We have three fileds called pasid under this struct > iommu_cache_invalidate_info{} > Gets confusing :) archid is a generic term. That's why you did not find it in the spec ;-) On ARM SMMU the archid is called the ASID (Address Space ID, up to 16 bits. The ASID is stored in the Context Descriptor Entry (your PASID entry) and thus characterizes a given stage 1 translation "context"/"adress space". At the moment the ASID is allocated per iommu domain. With aux domains we should have one ASID per aux domain, Jean-Philippe said. ASID tags IOTLB S1 entries. As the ASID is part of the "context descriptor" which is owned by the guest, the API must pass it somehow. 4.4.1.2 CMD_TLBI_NH_ASID(VMID, ASID) invalidation command allows to invalidate all IOTLB S1 entries for a given VMID/ASID and this is the functionality which is currently missing in the API. This is not an address based invalidation or a "pure" PASID based invalidation. At the moment we don't support PASIDs on ARM and I need this capability. Thanks Eric >> Thanks, >> Jean >> >>> >>> Domain invalidation would invalidate all the contexts belonging to >>> that domain. >>> >>> Thanks >>> >>> Eric > > [Jacob Pan] >
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On Mon, 13 May 2019 18:09:48 +0100 Jean-Philippe Brucker wrote: > On 13/05/2019 17:50, Auger Eric wrote: > >> struct iommu_inv_pasid_info { > >> #define IOMMU_INV_PASID_FLAGS_PASID(1 << 0) > >> #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > >>__u32 flags; > >>__u32 archid; > >>__u64 pasid; > >> }; > > I agree it does the job now. However it looks a bit strange to do a > > PASID based invalidation in my case - SMMUv3 nested stage - where I > > don't have any PASID involved. > > > > Couldn't we call it context based invalidation then? A context can > > be tagged by a PASID or/and an ARCHID. > > I think calling it "context" would be confusing as well (I shouldn't > have used it earlier), since VT-d uses that name for device table > entries (=STE on Arm SMMU). Maybe "addr_space"? > I am still struggling to understand what ARCHID is after scanning through SMMUv3.1 spec. It seems to be a constant for a given SMMU. Why do you need to pass it down every time? Could you point to me the document or explain a little more on ARCHID use cases. We have three fileds called pasid under this struct iommu_cache_invalidate_info{} Gets confusing :) > Thanks, > Jean > > > > > Domain invalidation would invalidate all the contexts belonging to > > that domain. > > > > Thanks > > > > Eric [Jacob Pan] ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
On 13/05/2019 17:50, Auger Eric wrote: >> struct iommu_inv_pasid_info { >> #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) >> #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) >> __u32 flags; >> __u32 archid; >> __u64 pasid; >> }; > I agree it does the job now. However it looks a bit strange to do a > PASID based invalidation in my case - SMMUv3 nested stage - where I > don't have any PASID involved. > > Couldn't we call it context based invalidation then? A context can be > tagged by a PASID or/and an ARCHID. I think calling it "context" would be confusing as well (I shouldn't have used it earlier), since VT-d uses that name for device table entries (=STE on Arm SMMU). Maybe "addr_space"? Thanks, Jean > > Domain invalidation would invalidate all the contexts belonging to that > domain. > > Thanks > > Eric
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Jean-Philippe, On 5/13/19 1:20 PM, Jean-Philippe Brucker wrote: > Hi Eric, > > On 13/05/2019 10:14, Auger Eric wrote: >> I noticed my qemu integration was currently incorrectly using PASID >> invalidation for ASID based invalidation (SMMUV3 Stage1 CMD_TLBI_NH_ASID >> invalidation command). So I think we also need ARCHID invalidation. >> Sorry for the late notice. >>> >>> +/* defines the granularity of the invalidation */ >>> +enum iommu_inv_granularity { >>> + IOMMU_INV_GRANU_DOMAIN, /* domain-selective invalidation */ >> IOMMU_INV_GRANU_ARCHID, /* archid-selective invalidation */ >>> + IOMMU_INV_GRANU_PASID, /* pasid-selective invalidation */ > > In terms of granularity, these values have the same meaning: invalidate > the whole address space of a context. Then you can communicate two > things using the same struct: > * If ATS is enables an Arm host needs to invalidate all ATC entries > using PASID. > * If BTM isn't used by the guest, the host needs to invalidate all TLB > entries using ARCHID. > > Rather than introducing a new granule here, could we just add an archid > field to the struct associated with IOMMU_INV_GRANU_PASID? Something like... > >>> + IOMMU_INV_GRANU_ADDR, /* page-selective invalidation */ >>> + IOMMU_INVAL_GRANU_NR, /* number of invalidation granularities */ >>> +}; >>> + >>> +/** >>> + * Address Selective Invalidation Structure >>> + * >>> + * @flags indicates the granularity of the address-selective invalidation >>> + * - if PASID bit is set, @pasid field is populated and the invalidation >>> + * relates to cache entries tagged with this PASID and matching the >>> + * address range. >>> + * - if ARCHID bit is set, @archid is populated and the invalidation >>> relates >>> + * to cache entries tagged with this architecture specific id and >>> matching >>> + * the address range. >>> + * - Both PASID and ARCHID can be set as they may tag different caches. >>> + * - if neither PASID or ARCHID is set, global addr invalidation applies >>> + * - LEAF flag indicates whether only the leaf PTE caching needs to be >>> + * invalidated and other paging structure caches can be preserved. >>> + * @pasid: process address space id >>> + * @archid: architecture-specific id >>> + * @addr: first stage/level input address >>> + * @granule_size: page/block size of the mapping in bytes >>> + * @nb_granules: number of contiguous granules to be invalidated >>> + */ >>> +struct iommu_inv_addr_info { >>> +#define IOMMU_INV_ADDR_FLAGS_PASID (1 << 0) >>> +#define IOMMU_INV_ADDR_FLAGS_ARCHID(1 << 1) >>> +#define IOMMU_INV_ADDR_FLAGS_LEAF (1 << 2) >>> + __u32 flags; >>> + __u32 archid; >>> + __u64 pasid; >>> + __u64 addr; >>> + __u64 granule_size; >>> + __u64 nb_granules; >>> +}; > > struct iommu_inv_pasid_info { > #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) > #define IOMMU_INV_PASID_FLAGS_ARCHID (1 << 1) > __u32 flags; > __u32 archid; > __u64 pasid; > }; I agree it does the job now. However it looks a bit strange to do a PASID based invalidation in my case - SMMUv3 nested stage - where I don't have any PASID involved. Couldn't we call it context based invalidation then? A context can be tagged by a PASID or/and an ARCHID. Domain invalidation would invalidate all the contexts belonging to that domain. Thanks Eric > >>> + >>> +/** >>> + * First level/stage invalidation information >>> + * @cache: bitfield that allows to select which caches to invalidate >>> + * @granularity: defines the lowest granularity used for the invalidation: >>> + * domain > pasid > addr >>> + * >>> + * Not all the combinations of cache/granularity make sense: >>> + * >>> + * type | DEV_IOTLB | IOTLB | PASID| >>> + * granularity | | | cache| >>> + * -+---+---+---+ >>> + * DOMAIN | N/A | Y | Y | >> * ARCHID | N/A | Y | N/A | >> >>> + * PASID | Y | Y | Y | >>> + * ADDR| Y | Y | N/A | >>> + * >>> + * Invalidations by %IOMMU_INV_GRANU_ADDR use field @addr_info. >> * Invalidations by %IOMMU_INV_GRANU_ARCHID use field @archid. >>> + * Invalidations by %IOMMU_INV_GRANU_PASID use field @pasid. >>> + * Invalidations by %IOMMU_INV_GRANU_DOMAIN don't take any argument. >>> + * >>> + * If multiple cache types are invalidated simultaneously, they all >>> + * must support the used granularity. >>> + */ >>> +struct iommu_cache_invalidate_info { >>> +#define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1 >>> + __u32 version; >>> +/* IOMMU paging structure cache */ >>> +#define IOMMU_CACHE_INV_TYPE_IOTLB (1 << 0) /* IOMMU IOTLB */ >>> +#define IOMMU_CACHE_INV_TYPE_DEV_IOTLB (1 << 1) /* Device IOTLB */ >>> +#define IOMMU_CACHE_INV_TYPE_PASID (1 << 2) /* PASID cache */ >>>
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Eric, On 13/05/2019 10:14, Auger Eric wrote: > I noticed my qemu integration was currently incorrectly using PASID > invalidation for ASID based invalidation (SMMUV3 Stage1 CMD_TLBI_NH_ASID > invalidation command). So I think we also need ARCHID invalidation. > Sorry for the late notice. >> >> +/* defines the granularity of the invalidation */ >> +enum iommu_inv_granularity { >> +IOMMU_INV_GRANU_DOMAIN, /* domain-selective invalidation */ > IOMMU_INV_GRANU_ARCHID, /* archid-selective invalidation */ >> +IOMMU_INV_GRANU_PASID, /* pasid-selective invalidation */ In terms of granularity, these values have the same meaning: invalidate the whole address space of a context. Then you can communicate two things using the same struct: * If ATS is enables an Arm host needs to invalidate all ATC entries using PASID. * If BTM isn't used by the guest, the host needs to invalidate all TLB entries using ARCHID. Rather than introducing a new granule here, could we just add an archid field to the struct associated with IOMMU_INV_GRANU_PASID? Something like... >> +IOMMU_INV_GRANU_ADDR, /* page-selective invalidation */ >> +IOMMU_INVAL_GRANU_NR, /* number of invalidation granularities */ >> +}; >> + >> +/** >> + * Address Selective Invalidation Structure >> + * >> + * @flags indicates the granularity of the address-selective invalidation >> + * - if PASID bit is set, @pasid field is populated and the invalidation >> + * relates to cache entries tagged with this PASID and matching the >> + * address range. >> + * - if ARCHID bit is set, @archid is populated and the invalidation relates >> + * to cache entries tagged with this architecture specific id and matching >> + * the address range. >> + * - Both PASID and ARCHID can be set as they may tag different caches. >> + * - if neither PASID or ARCHID is set, global addr invalidation applies >> + * - LEAF flag indicates whether only the leaf PTE caching needs to be >> + * invalidated and other paging structure caches can be preserved. >> + * @pasid: process address space id >> + * @archid: architecture-specific id >> + * @addr: first stage/level input address >> + * @granule_size: page/block size of the mapping in bytes >> + * @nb_granules: number of contiguous granules to be invalidated >> + */ >> +struct iommu_inv_addr_info { >> +#define IOMMU_INV_ADDR_FLAGS_PASID (1 << 0) >> +#define IOMMU_INV_ADDR_FLAGS_ARCHID (1 << 1) >> +#define IOMMU_INV_ADDR_FLAGS_LEAF (1 << 2) >> +__u32 flags; >> +__u32 archid; >> +__u64 pasid; >> +__u64 addr; >> +__u64 granule_size; >> +__u64 nb_granules; >> +}; struct iommu_inv_pasid_info { #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0) #define IOMMU_INV_PASID_FLAGS_ARCHID(1 << 1) __u32 flags; __u32 archid; __u64 pasid; }; >> + >> +/** >> + * First level/stage invalidation information >> + * @cache: bitfield that allows to select which caches to invalidate >> + * @granularity: defines the lowest granularity used for the invalidation: >> + * domain > pasid > addr >> + * >> + * Not all the combinations of cache/granularity make sense: >> + * >> + * type | DEV_IOTLB | IOTLB | PASID| >> + * granularity | | | cache| >> + * -+---+---+---+ >> + * DOMAIN | N/A | Y | Y | > * ARCHID | N/A | Y | N/A | > >> + * PASID| Y | Y | Y | >> + * ADDR | Y | Y | N/A | >> + * >> + * Invalidations by %IOMMU_INV_GRANU_ADDR use field @addr_info. > * Invalidations by %IOMMU_INV_GRANU_ARCHID use field @archid. >> + * Invalidations by %IOMMU_INV_GRANU_PASID use field @pasid. >> + * Invalidations by %IOMMU_INV_GRANU_DOMAIN don't take any argument. >> + * >> + * If multiple cache types are invalidated simultaneously, they all >> + * must support the used granularity. >> + */ >> +struct iommu_cache_invalidate_info { >> +#define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1 >> +__u32 version; >> +/* IOMMU paging structure cache */ >> +#define IOMMU_CACHE_INV_TYPE_IOTLB (1 << 0) /* IOMMU IOTLB */ >> +#define IOMMU_CACHE_INV_TYPE_DEV_IOTLB (1 << 1) /* Device IOTLB */ >> +#define IOMMU_CACHE_INV_TYPE_PASID (1 << 2) /* PASID cache */ >> +#define IOMMU_CACHE_TYPE_NR (3) >> +__u8cache; >> +__u8granularity; >> +__u8padding[2]; >> +union { >> +__u64 pasid; > __u32 archid; struct iommu_inv_pasid_info pasid_info; Thanks, Jean > > Thanks > > Eric >> +struct iommu_inv_addr_info addr_info; >> +}; >> +}; >> + >> + >> #endif /* _UAPI_IOMMU_H */ >> ___ iommu mailing list iommu@lists.linux-foundation.org
Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API
Hi Jacob, Jean-Philippe, On 5/4/19 12:32 AM, Jacob Pan wrote: > From: "Liu, Yi L" > > In any virtualization use case, when the first translation stage > is "owned" by the guest OS, the host IOMMU driver has no knowledge > of caching structure updates unless the guest invalidation activities > are trapped by the virtualizer and passed down to the host. > > Since the invalidation data are obtained from user space and will be > written into physical IOMMU, we must allow security check at various > layers. Therefore, generic invalidation data format are proposed here, > model specific IOMMU drivers need to convert them into their own format. > > Signed-off-by: Liu, Yi L > Signed-off-by: Jean-Philippe Brucker > Signed-off-by: Jacob Pan > Signed-off-by: Ashok Raj > Signed-off-by: Eric Auger > > --- > v6 -> v7: > - detail which fields are used for each invalidation type > - add a comment about multiple cache invalidation > > v5 -> v6: > - fix merge issue > > v3 -> v4: > - full reshape of the API following Alex' comments > > v1 -> v2: > - add arch_id field > - renamed tlb_invalidate into cache_invalidate as this API allows > to invalidate context caches on top of IOTLBs > > v1: > renamed sva_invalidate into tlb_invalidate and add iommu_ prefix in > header. Commit message reworded. > --- > drivers/iommu/iommu.c | 14 > include/linux/iommu.h | 15 - > include/uapi/linux/iommu.h | 80 > ++ > 3 files changed, 108 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 8df9d34..a2f6f3e 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1645,6 +1645,20 @@ void iommu_detach_pasid_table(struct iommu_domain > *domain) > } > EXPORT_SYMBOL_GPL(iommu_detach_pasid_table); > > +int iommu_cache_invalidate(struct iommu_domain *domain, struct device *dev, > +struct iommu_cache_invalidate_info *inv_info) > +{ > + int ret = 0; > + > + if (unlikely(!domain->ops->cache_invalidate)) > + return -ENODEV; > + > + ret = domain->ops->cache_invalidate(domain, dev, inv_info); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_cache_invalidate); > + > static void __iommu_detach_device(struct iommu_domain *domain, > struct device *dev) > { > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index ab4d922..d182525 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -266,6 +266,7 @@ struct page_response_msg { > * @page_response: handle page request response > * @attach_pasid_table: attach a pasid table > * @detach_pasid_table: detach the pasid table > + * @cache_invalidate: invalidate translation caches > * @pgsize_bitmap: bitmap of all possible supported page sizes > */ > struct iommu_ops { > @@ -328,8 +329,9 @@ struct iommu_ops { > int (*attach_pasid_table)(struct iommu_domain *domain, > struct iommu_pasid_table_config *cfg); > void (*detach_pasid_table)(struct iommu_domain *domain); > - > int (*page_response)(struct device *dev, struct page_response_msg *msg); > + int (*cache_invalidate)(struct iommu_domain *domain, struct device *dev, > + struct iommu_cache_invalidate_info *inv_info); > > unsigned long pgsize_bitmap; > }; > @@ -442,6 +444,9 @@ extern void iommu_detach_device(struct iommu_domain > *domain, > extern int iommu_attach_pasid_table(struct iommu_domain *domain, > struct iommu_pasid_table_config *cfg); > extern void iommu_detach_pasid_table(struct iommu_domain *domain); > +extern int iommu_cache_invalidate(struct iommu_domain *domain, > + struct device *dev, > + struct iommu_cache_invalidate_info *inv_info); > extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev); > extern struct iommu_domain *iommu_get_dma_domain(struct device *dev); > extern int iommu_map(struct iommu_domain *domain, unsigned long iova, > @@ -982,6 +987,14 @@ static inline int iommu_sva_get_pasid(struct iommu_sva > *handle) > static inline > void iommu_detach_pasid_table(struct iommu_domain *domain) {} > > +static inline int > +iommu_cache_invalidate(struct iommu_domain *domain, > +struct device *dev, > +struct iommu_cache_invalidate_info *inv_info) > +{ > + return -ENODEV; > +} > + > #endif /* CONFIG_IOMMU_API */ > > #ifdef CONFIG_IOMMU_DEBUGFS > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h > index 8848514..fa96ecb 100644 > --- a/include/uapi/linux/iommu.h > +++ b/include/uapi/linux/iommu.h > @@ -162,4 +162,84 @@ struct iommu_pasid_table_config { > }; > }; I noticed my qemu integration was currently incorrectly using PASID invalidation for ASID based invalidation (SMMUV3 Stage1