Add support for NoTagAccess memory attribute
cover Add support for NoTagAccess memory attribute
A VMM allows assigning different types of memory regions to the guest and not all memory regions support storing allocation tags. Currently, the kernel doesn't allow enabling the MTE feature in the guest if any of the assigned memory regions don't allow MTE. This prevents the usage of MTE in the guest even though the guest will never use these memory regions as allocation tagged memory.
This patch series provides a way to enable MTE in such configs. Translations from non-MTE-allowed memory regions are installed in stage-2 with NoTagAccess memory attributes. Guest access of allocation tags with these memory regions will result in a VM Exit.
Note: We could use the existing KVM_EXIT_MEMORY_FAULT for this. I chose to add a new EXIT type because this is an arm64-specific exit type and I was not sure whether KVM_EXIT_MEMORY_FAULT needs a NoTagAccess flag.
Aneesh Kumar K.V (Arm) (4):
arm64: Update the values to binary from hex
arm64: cpufeature: add Allocation Tag Access Permission (MTE_PERM)
feature
arm64: mte: update code comments
arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
arch/arm64/include/asm/cpufeature.h | 5 ++++ arch/arm64/include/asm/kvm_emulate.h | 5 ++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/include/asm/memory.h | 14 +++++----- arch/arm64/kernel/cpufeature.c | 9 +++++++ arch/arm64/kvm/hyp/pgtable.c | 16 ++++++++--- arch/arm64/kvm/mmu.c | 40 +++++++++++++++++++--------- arch/arm64/tools/cpucaps | 1 + include/uapi/linux/kvm.h | 7 +++++ 9 files changed, 77 insertions(+), 21 deletions(-)
base-commit: c964ced7726294d40913f2127c3f185a92cb4a41
Signature · 1 line
2.43.0
PATCH 1/4 arm64: Update the values to binary from hex
This matches the ARM ARM representation. No functional change in this patch.
--- arch/arm64/include/asm/memory.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
arch/arm64/include/asm/memory.h +6 -6
@@ -178,17 +178,17 @@/* * Memory types for Stage-2 translation */ -#define MT_S2_NORMAL 0xf -#define MT_S2_NORMAL_NC 0x5 -#define MT_S2_DEVICE_nGnRE 0x1 +#define MT_S2_NORMAL 0b1111 +#define MT_S2_NORMAL_NC 0b0101 +#define MT_S2_DEVICE_nGnRE 0b0001/* * Memory types for Stage-2 translation when ID_AA64MMFR2_EL1.FWB is 0001 * Stage-2 enforces Normal-WB and Device-nGnRE */ -#define MT_S2_FWB_NORMAL 6 -#define MT_S2_FWB_NORMAL_NC 5 -#define MT_S2_FWB_DEVICE_nGnRE 1 +#define MT_S2_FWB_NORMAL 0b0110 +#define MT_S2_FWB_NORMAL_NC 0b0101 +#define MT_S2_FWB_DEVICE_nGnRE 0b0001#ifdef CONFIG_ARM64_4K_PAGES #define IOREMAP_MAX_ORDER (PUD_SHIFT)
Signature · 1 line
2.43.0
PATCH 2/4 arm64: cpufeature: add Allocation Tag Access Permission (MTE_PERM) feature
In reply to Aneesh Kumar K.V (Arm)This indicates if the system supports MTE_PERM. This will be used by KVM for stage 2 mapping. This is a CPUCAP_SYSTEM feature because if we enable the feature all cpus must have it.
--- arch/arm64/include/asm/cpufeature.h | 5 +++++ arch/arm64/include/asm/memory.h | 2 ++ arch/arm64/kernel/cpufeature.c | 9 +++++++++ arch/arm64/tools/cpucaps | 1 + 4 files changed, 17 insertions(+)
arch/arm64/include/asm/cpufeature.h +5 -0
@@ -805,6 +805,11 @@ static inline bool system_supports_mte(void)return alternative_has_cap_unlikely (ARM64_MTE ); } +static inline bool system_supports_notagaccess (void ) +{ +return alternative_has_cap_unlikely (ARM64_MTE_PERM ); +} +static inline bool system_has_prio_mask_debugging (void ) { return IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
arch/arm64/include/asm/memory.h +2 -0
@@ -179,6 +179,7 @@* Memory types for Stage-2 translation */ #define MT_S2_NORMAL 0b1111 +#define MT_S2_NORMAL_NOTAGACCESS 0b0100#define MT_S2_NORMAL_NC 0b0101#define MT_S2_DEVICE_nGnRE 0b0001
@@ -187,6 +188,7 @@* Stage-2 enforces Normal-WB and Device-nGnRE */ #define MT_S2_FWB_NORMAL 0b0110 +#define MT_S2_FWB_NORMAL_NOTAGACCESS 0b1110#define MT_S2_FWB_NORMAL_NC 0b0101#define MT_S2_FWB_DEVICE_nGnRE 0b0001
arch/arm64/kernel/cpufeature.c +9 -0
@@ -305,6 +305,7 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr1[] = {static const struct arm64_ftr_bits ftr_id_aa64pfr2[] = {ARM64_FTR_BITS (FTR_VISIBLE ,FTR_STRICT ,FTR_LOWER_SAFE ,ID_AA64PFR2_EL1_FPMR_SHIFT ,4 ,0 ), +ARM64_FTR_BITS (FTR_HIDDEN ,FTR_NONSTRICT ,FTR_LOWER_SAFE ,ID_AA64PFR2_EL1_MTEPERM_SHIFT ,4 ,0 ),ARM64_FTR_END , };
@@ -2742,6 +2743,14 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .matches = has_cpuid_feature, ARM64_CPUID_FIELDS(ID_AA64PFR1_EL1, MTE, MTE3) }, + { + .desc = "MTE Allocation Tag Access Permission", + .capability = ARM64_MTE_PERM, + .type = ARM64_CPUCAP_SYSTEM_FEATURE, + .matches = has_cpuid_feature, + ARM64_CPUID_FIELDS(ID_AA64PFR2_EL1, MTEPERM, IMP) + }, + #endif /* CONFIG_ARM64_MTE */ { .desc = "RCpc load-acquire (LDAPR)",
arch/arm64/tools/cpucaps +1 -0
@@ -62,6 +62,7 @@ KVM_PROTECTED_MODE MISMATCHED_CACHE_TYPE MTE MTE_ASYMM +MTE_PERM SME SME_FA64 SME2
Signature · 1 line
2.43.0
PATCH 3/4 arm64: mte: update code comments
In reply to Aneesh Kumar K.V (Arm)commit d77e59a8fccd ("arm64: mte: Lock a page for MTE tag initialisation") updated the locking such the kernel now allows VM_SHARED mapping with MTE. Update the code comment to reflect this.
--- arch/arm64/kvm/mmu.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
arch/arm64/kvm/mmu.c +6 -6
@@ -1390,11 +1390,8 @@ static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva)* able to see the page's tags and therefore they must be initialised first. If * PG_mte_tagged is set, tags have already been initialised. * -* The race in the test/set of the PG_mte_tagged flag is handled by: -* - preventing VM_SHARED mappings in a memslot with MTE preventing two VMs -* racing to santise the same page -* - mmap_lock protects between a VM faulting a page in and the VMM performing -* an mprotect() to add VM_MTE +* The race in the test/set of the PG_mte_tagged flag is handled by +* using PG_mte_lock and PG_mte_tagged together. */ static void sanitise_mte_tags (struct kvm *kvm,kvm_pfn_t pfn,unsigned long size)
@@ -1646,7 +1643,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, }if (!fault_is_perm && !device &&kvm_has_mte (kvm)) { -/* Check the VMM hasn't introduced a new disallowed VMA */ +/* +* not a permission fault implies a translation fault which +* means mapping the page for the first time +*/ if (mte_allowed) {sanitise_mte_tags (kvm, pfn, vma_pagesize); }else {
Signature · 1 line
2.43.0
Marc Zyngier
maintainerRe: [PATCH 3/4] arm64: mte: update code comments
Quoted 9 lines · Aneesh Kumar K.V (Arm)
commit
d77e59a8fccd("arm64: mte: Lock a page for MTE tag initialisation") updated the locking such the kernel now allowsVM_SHAREDmapping with MTE. Update the code comment to reflect this.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/kvm/mmu.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
This is a KVM patch. Please make sure you write the subject accordingly, matching the existing conventions (in this case, this should read something like: "KVM: arm64: MTE: Update...").
Quoted 6 lines
arch/arm64/kvm/mmu.c +0
-0
arch/arm64/kvm/mmu.c static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva) · line 1394
Quoted 10 lines
arch/arm64/kvm/mmu.c +2
-5@@ -1390,11 +1390,8 @@ static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva)* able to see the page's tags and therefore they must be initialised first. If * PG_mte_tagged is set, tags have already been initialised. * -* The race in the test/set of the PG_mte_tagged flag is handled by: -* - preventing VM_SHARED mappings in a memslot with MTE preventing two VMs -* racing to santise the same page -* - mmap_lock protects between a VM faulting a page in and the VMM performing -* an mprotect() to add VM_MTE +* The race in the test/set of the PG_mte_tagged flag is handled by +* using PG_mte_lock and PG_mte_tagged together.
How? This comment is pretty content-free. TO be useful, you should elaborate on *how* these two are used together.
arch/arm64/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 1648
Quoted 11 lines
arch/arm64/kvm/mmu.c +3
-1*/ static void sanitise_mte_tags (struct kvm *kvm,kvm_pfn_t pfn,unsigned long size)@@ -1646,7 +1643,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, }if (!fault_is_perm && !device &&kvm_has_mte (kvm)) { -/* Check the VMM hasn't introduced a new disallowed VMA */ +/* +* not a permission fault implies a translation fault which +* means mapping the page for the first time
How about an Access fault due to page ageing?
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 3/4] arm64: mte: update code comments
Hi Marc,
Thanks for reviewing the changes.
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Aneesh Kumar K.V (Arm)
commit
d77e59a8fccd("arm64: mte: Lock a page for MTE tag initialisation") updated the locking such the kernel now allowsVM_SHAREDmapping with MTE. Update the code comment to reflect this.
Quoted 9 lines
Quoted 4 lines
Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/kvm/mmu.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)This is a KVM patch. Please make sure you write the subject accordingly, matching the existing conventions (in this case, this should read something like: "KVM: arm64: MTE: Update...").
Will update
Quoted 12 lines
Quoted 12 lines
arch/arm64/kvm/mmu.c +0
-2@@ -1390,11 +1390,8 @@ static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva)* able to see the page's tags and therefore they must be initialised first. If * PG_mte_tagged is set, tags have already been initialised. * -* The race in the test/set of the PG_mte_tagged flag is handled by: -* - preventing VM_SHARED mappings in a memslot with MTE preventing two VMs
Quoted 9 lines
arch/arm64/kvm/mmu.c static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva) · line 1394
Quoted 5 lines
arch/arm64/kvm/mmu.c +2
-3-* racing to santise the same page -* - mmap_lock protects between a VM faulting a page in and the VMM performing -* an mprotect() to add VM_MTE +* The race in the test/set of the PG_mte_tagged flag is handled by +* using PG_mte_lock and PG_mte_tagged together. How? This comment is pretty content-free. TO be useful, you should elaborate on *how* these two are used together.
I will add more details described in commit d77e59a8fccde7fb5dd8c57594ed147b4291c970 Should i quote the commit there in the comment?
Quoted 7 lines
Quoted 7 lines
arch/arm64/kvm/mmu.c +0
-0*/ static void sanitise_mte_tags (struct kvm *kvm,kvm_pfn_t pfn,unsigned long size)@@ -1646,7 +1643,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, }
Quoted 8 lines
arch/arm64/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 1648
Quoted 5 lines
arch/arm64/kvm/mmu.c +3
-1if (!fault_is_perm && !device &&kvm_has_mte (kvm)) { -/* Check the VMM hasn't introduced a new disallowed VMA */ +/* +* not a permission fault implies a translation fault which +* means mapping the page for the first time How about an Access fault due to page ageing?
IIUC access fault is already handled by the caller kvm_handle_guest_abort? I can add that as part of the updated comments?
-aneesh
Marc Zyngier
maintainerRe: [PATCH 3/4] arm64: mte: update code comments
Quoted 21 lines · Aneesh Kumar K.V
Quoted 11 lines
Quoted 11 lines
arch/arm64/kvm/mmu.c +0
-2@@ -1390,11 +1390,8 @@ static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva)* able to see the page's tags and therefore they must be initialised first. If * PG_mte_tagged is set, tags have already been initialised. * -* The race in the test/set of the PG_mte_tagged flag is handled by: -* - preventing VM_SHARED mappings in a memslot with MTE preventing two VMs Quoted 9 lines
arch/arm64/kvm/mmu.c static int get_vma_page_shift(struct vm_area_struct *vma, unsigned long hva) · line 1394
Quoted 5 lines
arch/arm64/kvm/mmu.c +2
-3-* racing to santise the same page -* - mmap_lock protects between a VM faulting a page in and the VMM performing -* an mprotect() to add VM_MTE +* The race in the test/set of the PG_mte_tagged flag is handled by +* using PG_mte_lock and PG_mte_tagged together. How? This comment is pretty content-free. TO be useful, you should elaborate on *how* these two are used together.
Quoted 2 lines
I will add more details described in commit
d77e59a8fccde7fb5dd8c57594ed147b4291c970Should i quote the commit there in the comment?
The commit is not relevant. What is important is an indication of how the race is resolved if that's important. A reference to try_page_mte_tagging() would probably be the right thing to do.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
arch/arm64/kvm/mmu.c +0
-0*/ static void sanitise_mte_tags (struct kvm *kvm,kvm_pfn_t pfn,unsigned long size)@@ -1646,7 +1643,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, }Quoted 8 lines
arch/arm64/kvm/mmu.c static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, · line 1648
Quoted 5 lines
arch/arm64/kvm/mmu.c +3
-1if (!fault_is_perm && !device &&kvm_has_mte (kvm)) { -/* Check the VMM hasn't introduced a new disallowed VMA */ +/* +* not a permission fault implies a translation fault which +* means mapping the page for the first time How about an Access fault due to page ageing?
Quoted 2 lines
IIUC access fault is already handled by the caller
kvm_handle_guest_abort? I can add that as part of the updated comments?
Maybe. The thing is, you are removing a pretty essential comment for no good reason, and now there is no rational left behind the -EFAULT that is returned.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
PATCH 4/4 arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
In reply to Aneesh Kumar K.V (Arm)Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
NOTE: We could also use KVM_EXIT_MEMORY_FAULT for this. I chose to add a new EXIT type because this is arm64 specific exit type.
--- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/kvm/hyp/pgtable.c | 16 +++++++++++++--- arch/arm64/kvm/mmu.c | 28 ++++++++++++++++++++++------ include/uapi/linux/kvm.h | 7 +++++++ 5 files changed, 48 insertions(+), 9 deletions(-)
arch/arm64/include/asm/kvm_emulate.h +5 -0
@@ -373,6 +373,11 @@ static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)return kvm_vcpu_trap_is_iabt (vcpu) && !kvm_vcpu_abt_iss1tw (vcpu); } +static inline bool kvm_vcpu_trap_is_tagaccess (const struct kvm_vcpu *vcpu) +{ +return !!(ESR_ELx_ISS2 (kvm_vcpu_get_esr (vcpu)) & ESR_ELx_TagAccess); +} + static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) {return kvm_vcpu_get_esr (vcpu) & ESR_ELx_FSC;
arch/arm64/include/asm/kvm_pgtable.h +1 -0
@@ -252,6 +252,7 @@ enum kvm_pgtable_prot {KVM_PGTABLE_PROT_DEVICE =BIT (3 ),KVM_PGTABLE_PROT_NORMAL_NC =BIT (4 ), +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS =BIT (5 ),KVM_PGTABLE_PROT_SW0 =BIT (55 ), KVM_PGTABLE_PROT_SW1 = BIT(56),
arch/arm64/kvm/hyp/pgtable.c +13 -3
@@ -677,9 +677,11 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p {kvm_pte_t attr;u32 sh =KVM_PTE_LEAF_ATTR_LO_S2_SH_IS ; +unsigned long prot_mask =KVM_PGTABLE_PROT_DEVICE | +KVM_PGTABLE_PROT_NORMAL_NC | +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; -switch (prot & (KVM_PGTABLE_PROT_DEVICE | -KVM_PGTABLE_PROT_NORMAL_NC )) { +switch (prot & prot_mask) {case KVM_PGTABLE_PROT_DEVICE |KVM_PGTABLE_PROT_NORMAL_NC :return -EINVAL ;case KVM_PGTABLE_PROT_DEVICE :
@@ -692,6 +694,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot preturn -EINVAL ; attr =KVM_S2_MEMATTR (pgt,NORMAL_NC );break ; +case KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS : +if (system_supports_notagaccess ()) + attr =KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS ); +else +return -EINVAL ; +break ;default : attr =KVM_S2_MEMATTR (pgt,NORMAL ); }
@@ -872,7 +880,9 @@ static void stage2_unmap_put_pte(const struct kvm_pgtable_visit_ctx *ctx,static bool stage2_pte_cacheable (struct kvm_pgtable *pgt,kvm_pte_t pte) {u64 memattr = pte &KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR ; -return kvm_pte_valid (pte) && memattr ==KVM_S2_MEMATTR (pgt,NORMAL ); +return kvm_pte_valid (pte) && + ((memattr ==KVM_S2_MEMATTR (pgt,NORMAL )) || + (memattr ==KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS ))); }static bool stage2_pte_executable (kvm_pte_t pte)
arch/arm64/kvm/mmu.c +22 -6
@@ -1647,12 +1647,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,* not a permission fault implies a translation fault which * means mapping the page for the first time */ -if (mte_allowed) { +if (mte_allowed)sanitise_mte_tags (kvm, pfn, vma_pagesize); - }else { - ret = -EFAULT ; - goto out_unlock; - } +else + prot |=KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; }if (writable)
@@ -1721,6 +1719,15 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)kvm_set_pfn_accessed (kvm_pte_to_pfn (pte)); } +static inline void kvm_prepare_notagaccess_exit (struct kvm_vcpu *vcpu, +gpa_t gpa,gpa_t size) +{ + vcpu->run->exit_reason =KVM_EXIT_ARM_NOTAG_ACCESS ; + vcpu->run->notag_access.flags =0 ; + vcpu->run->notag_access.gpa = gpa; + vcpu->run->notag_access.size = size; +} +/** * kvm_handle_guest_abort - handles all 2nd stage aborts * @vcpu: the VCPU pointer
@@ -1833,6 +1840,14 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) gfn = ipa >>PAGE_SHIFT ; memslot =gfn_to_memslot (vcpu->kvm, gfn); + +if (kvm_vcpu_trap_is_tagaccess (vcpu)) { +/* exit to host and handle the error */ +kvm_prepare_notagaccess_exit (vcpu, gfn <<PAGE_SHIFT ,PAGE_SIZE ); + ret =0 ; + goto out; + } + hva =gfn_to_hva_memslot_prot (memslot, gfn, &writable); write_fault =kvm_is_write_fault (vcpu);if (kvm_is_error_hva (hva) || (write_fault && !writable)) {
@@ -2145,7 +2160,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,if (!vma)break ; -if (kvm_has_mte (kvm) && !kvm_vma_mte_allowed (vma)) { +if (kvm_has_mte (kvm) && !system_supports_notagaccess () && + !kvm_vma_mte_allowed (vma)) { ret = -EINVAL ;break ; }
include/uapi/linux/kvm.h +7 -0
@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */
@@ -446,6 +447,12 @@ struct kvm_run {__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };
Signature · 1 line
2.43.0
Marc Zyngier
maintainerRe: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Quoted 8 lines · Aneesh Kumar K.V (Arm)
Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
I only have questions here:
- what is the benefit of such approach? why shouldn't that be the kernel's job to fix it?
- where is the documentation for this new userspace ABI?
- are you expecting the VMM to create a new memslot for this?
- where is the example of a VMM using this?
Quoted 34 lines
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/kvm/hyp/pgtable.c | 16 +++++++++++++--- arch/arm64/kvm/mmu.c | 28 ++++++++++++++++++++++------ include/uapi/linux/kvm.h | 7 +++++++ 5 files changed, 48 insertions(+), 9 deletions(-)arch/arm64/include/asm/kvm_emulate.h +5
-0@@ -373,6 +373,11 @@ static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)return kvm_vcpu_trap_is_iabt (vcpu) && !kvm_vcpu_abt_iss1tw (vcpu); } +static inline bool kvm_vcpu_trap_is_tagaccess (const struct kvm_vcpu *vcpu) +{ +return !!(ESR_ELx_ISS2 (kvm_vcpu_get_esr (vcpu)) & ESR_ELx_TagAccess); +} + static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) {return kvm_vcpu_get_esr (vcpu) & ESR_ELx_FSC;arch/arm64/include/asm/kvm_pgtable.h +0
-0
arch/arm64/include/asm/kvm_pgtable.h enum kvm_pgtable_prot { · line 255
Quoted 4 lines
arch/arm64/include/asm/kvm_pgtable.h +1
-0@@ -252,6 +252,7 @@ enum kvm_pgtable_prot {KVM_PGTABLE_PROT_DEVICE =BIT (3 ),KVM_PGTABLE_PROT_NORMAL_NC =BIT (4 ), + KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS = BIT(5),
This seems wrong. NOTAGACCESS is a *permission*, not a memory type.
Quoted 22 lines
arch/arm64/include/asm/kvm_pgtable.h +0
-0KVM_PGTABLE_PROT_SW0 =BIT (55 ), KVM_PGTABLE_PROT_SW1 = BIT(56),arch/arm64/kvm/hyp/pgtable.c +4
-2@@ -677,9 +677,11 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p {kvm_pte_t attr;u32 sh =KVM_PTE_LEAF_ATTR_LO_S2_SH_IS ; +unsigned long prot_mask =KVM_PGTABLE_PROT_DEVICE | +KVM_PGTABLE_PROT_NORMAL_NC | +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; -switch (prot & (KVM_PGTABLE_PROT_DEVICE | -KVM_PGTABLE_PROT_NORMAL_NC )) { +switch (prot & prot_mask) {case KVM_PGTABLE_PROT_DEVICE |KVM_PGTABLE_PROT_NORMAL_NC :return -EINVAL ;case KVM_PGTABLE_PROT_DEVICE :
arch/arm64/kvm/hyp/pgtable.c static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p · line 702
Quoted 9 lines
arch/arm64/kvm/hyp/pgtable.c +6
-0@@ -692,6 +694,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot preturn -EINVAL ; attr =KVM_S2_MEMATTR (pgt,NORMAL_NC );break ; +case KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS : +if (system_supports_notagaccess ()) + attr =KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS ); +else +return -EINVAL ; +break ;
How do you see this working when migrating a VM from one host to another, one that supports FEAT_MTE_PERM and one that doesn't? The current assumptions are that the VMM will replay the *exact same* setup on the target host, and this obviously doesn't work.
Quoted 35 lines
arch/arm64/kvm/hyp/pgtable.c +3
-1default : attr =KVM_S2_MEMATTR (pgt,NORMAL ); }@@ -872,7 +880,9 @@ static void stage2_unmap_put_pte(const struct kvm_pgtable_visit_ctx *ctx,static bool stage2_pte_cacheable (struct kvm_pgtable *pgt,kvm_pte_t pte) {u64 memattr = pte &KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR ; -return kvm_pte_valid (pte) && memattr ==KVM_S2_MEMATTR (pgt,NORMAL ); +return kvm_pte_valid (pte) && + ((memattr ==KVM_S2_MEMATTR (pgt,NORMAL )) || + (memattr ==KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS ))); }static bool stage2_pte_executable (kvm_pte_t pte)arch/arm64/kvm/mmu.c +3
-5@@ -1647,12 +1647,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,* not a permission fault implies a translation fault which * means mapping the page for the first time */ -if (mte_allowed) { +if (mte_allowed)sanitise_mte_tags (kvm, pfn, vma_pagesize); - }else { - ret = -EFAULT ; - goto out_unlock; - } +else + prot |=KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; }if (writable)
arch/arm64/kvm/mmu.c static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa) · line 1728
Quoted 10 lines
arch/arm64/kvm/mmu.c +7
-0@@ -1721,6 +1719,15 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)kvm_set_pfn_accessed (kvm_pte_to_pfn (pte)); } +static inline void kvm_prepare_notagaccess_exit (struct kvm_vcpu *vcpu, +gpa_t gpa,gpa_t size) +{ + vcpu->run->exit_reason =KVM_EXIT_ARM_NOTAG_ACCESS ; + vcpu->run->notag_access.flags =0 ; + vcpu->run->notag_access.gpa = gpa; + vcpu->run->notag_access.size = size;
Why does size matter here? It seems pretty pointless.
Quoted 45 lines
arch/arm64/kvm/mmu.c +12
-1+} +/** * kvm_handle_guest_abort - handles all 2nd stage aborts * @vcpu: the VCPU pointer @@ -1833,6 +1840,14 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) gfn = ipa >>PAGE_SHIFT ; memslot =gfn_to_memslot (vcpu->kvm, gfn); + +if (kvm_vcpu_trap_is_tagaccess (vcpu)) { +/* exit to host and handle the error */ +kvm_prepare_notagaccess_exit (vcpu, gfn <<PAGE_SHIFT ,PAGE_SIZE ); + ret =0 ; + goto out; + } + hva =gfn_to_hva_memslot_prot (memslot, gfn, &writable); write_fault =kvm_is_write_fault (vcpu);if (kvm_is_error_hva (hva) || (write_fault && !writable)) {@@ -2145,7 +2160,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,if (!vma)break ; -if (kvm_has_mte (kvm) && !kvm_vma_mte_allowed (vma)) { +if (kvm_has_mte (kvm) && !system_supports_notagaccess () && + !kvm_vma_mte_allowed (vma)) { ret = -EINVAL ;break ; }include/uapi/linux/kvm.h +1
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -446,6 +447,12 @@ struct kvm_run {__u64 gpa;__u64 size;
include/uapi/linux/kvm.h struct kvm_run { · line 458
Quoted 10 lines
include/uapi/linux/kvm.h +6
-0} memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };
How do you plan to handle the same thing for NV?
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Quoted 11 lines · Marc Zyngier
Quoted 8 lines · Aneesh Kumar K.V (Arm)
Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
Quoted 5 lines
I only have questions here:
- what is the benefit of such approach? why shouldn't that be the kernel's job to fix it?
IMHO leaving that policy decision to VMM makes the kernel changes simpler. In most cases, VMM will kill the guest, because these restrictions of MTE_ALLOWED are applied at the memslot/vma.
Quoted 3 lines
- where is the documentation for this new userspace ABI?
I will add the details if we agree that this should be a separate EXIT as outlined in this patch.
Quoted 3 lines
- are you expecting the VMM to create a new memslot for this?
I guess there are examples of configs where some memory regions are backed by page cache where we don't directly use those memory regions as allocatable memory in the guest. This change allows us to enable MTE in such configs.
Quoted 2 lines
- where is the example of a VMM using this?
I do have changes to kvmtool which won't do any fixup on receiving that VM exit. I expect other VMM to do the same by default when they get a VM exit with an unknown exit_reason. So unless VMM wants to do any special handling, we don't need any change in the VMM.
arm/kvm-cpu.c +13 -0
@@ -239,6 +239,17 @@ static bool handle_memoryfault(struct kvm_cpu *vcpu)return true; } +static bool handle_notag_access (struct kvm_cpu *vcpu) +{ +u64 gpa = vcpu->kvm_run->memory_fault.gpa; +u64 size = vcpu->kvm_run->memory_fault.size; + +/* For now VMM just panic */ +pr_err ("Tag Access to a wrong memory region 0x%lx size 0x%lx\n" , + (unsigned long )gpa, (unsigned long )size); +return false; +} +bool kvm_cpu__handle_exit (struct kvm_cpu *vcpu) {switch (vcpu->kvm_run->exit_reason) {
@@ -246,6 +257,8 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)return handle_hypercall (vcpu);case KVM_EXIT_MEMORY_FAULT :return handle_memoryfault (vcpu); +case KVM_EXIT_ARM_NOTAG_ACCESS : +return handle_notag_access (vcpu); }return false;
include/linux/kvm.h +8 -0
@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */
@@ -429,10 +430,17 @@ struct kvm_run {/* KVM_EXIT_MEMORY_FAULT */ struct {#define KVM_MEMORY_EXIT_FLAG_PRIVATE (1ULL << 3) +#define KVM_MEMORY_EXIT_FLAG_NOTAGACCESS (1ULL << 4)__u64 flags;__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };
Quoted 35 lines
Quoted 35 lines
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/kvm/hyp/pgtable.c | 16 +++++++++++++--- arch/arm64/kvm/mmu.c | 28 ++++++++++++++++++++++------ include/uapi/linux/kvm.h | 7 +++++++ 5 files changed, 48 insertions(+), 9 deletions(-)arch/arm64/include/asm/kvm_emulate.h +5
-0@@ -373,6 +373,11 @@ static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)return kvm_vcpu_trap_is_iabt (vcpu) && !kvm_vcpu_abt_iss1tw (vcpu); } +static inline bool kvm_vcpu_trap_is_tagaccess (const struct kvm_vcpu *vcpu) +{ +return !!(ESR_ELx_ISS2 (kvm_vcpu_get_esr (vcpu)) & ESR_ELx_TagAccess); +} + static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) {return kvm_vcpu_get_esr (vcpu) & ESR_ELx_FSC;arch/arm64/include/asm/kvm_pgtable.h +0
-0@@ -252,6 +252,7 @@ enum kvm_pgtable_prot {
Quoted 6 lines
arch/arm64/include/asm/kvm_pgtable.h enum kvm_pgtable_prot { · line 255
Quoted 3 lines
arch/arm64/include/asm/kvm_pgtable.h +1
-0KVM_PGTABLE_PROT_DEVICE =BIT (3 ),KVM_PGTABLE_PROT_NORMAL_NC =BIT (4 ), + KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS = BIT(5),This seems wrong. NOTAGACCESS is a *permission*, not a memory type.
Are you suggesting the name is wrong? The memory attribute value I wanted to use is
MemAttr[3:0] = 0b0100 which is Normal, NoTagAccess, writeback cacheable.
I am following the changes similar to KVM_PGTABLE_PROT_NORMAL_NC.
Quoted 29 lines
Quoted 29 lines
arch/arm64/include/asm/kvm_pgtable.h +0
-0KVM_PGTABLE_PROT_SW0 =BIT (55 ), KVM_PGTABLE_PROT_SW1 = BIT(56),arch/arm64/kvm/hyp/pgtable.c +7
-2@@ -677,9 +677,11 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p {kvm_pte_t attr;u32 sh =KVM_PTE_LEAF_ATTR_LO_S2_SH_IS ; +unsigned long prot_mask =KVM_PGTABLE_PROT_DEVICE | +KVM_PGTABLE_PROT_NORMAL_NC | +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; -switch (prot & (KVM_PGTABLE_PROT_DEVICE | -KVM_PGTABLE_PROT_NORMAL_NC )) { +switch (prot & prot_mask) {case KVM_PGTABLE_PROT_DEVICE |KVM_PGTABLE_PROT_NORMAL_NC :return -EINVAL ;case KVM_PGTABLE_PROT_DEVICE :@@ -692,6 +694,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot preturn -EINVAL ; attr =KVM_S2_MEMATTR (pgt,NORMAL_NC );break ; +case KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS : +if (system_supports_notagaccess ()) + attr =KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS );
Quoted 9 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p · line 702
Quoted 3 lines
arch/arm64/kvm/hyp/pgtable.c +3
-0+else +return -EINVAL ; +break ;How do you see this working when migrating a VM from one host to another, one that supports
FEAT_MTE_PERMand one that doesn't? The current assumptions are that the VMM will replay the *exact same* setup on the target host, and this obviously doesn't work.
I missed looking at kvm migration. I guess I will have to expose this as a capability and only allow migration if the target also supports the same capability?
Quoted 39 lines
Quoted 39 lines
arch/arm64/kvm/hyp/pgtable.c +3
-1default : attr =KVM_S2_MEMATTR (pgt,NORMAL ); }@@ -872,7 +880,9 @@ static void stage2_unmap_put_pte(const struct kvm_pgtable_visit_ctx *ctx,static bool stage2_pte_cacheable (struct kvm_pgtable *pgt,kvm_pte_t pte) {u64 memattr = pte &KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR ; -return kvm_pte_valid (pte) && memattr ==KVM_S2_MEMATTR (pgt,NORMAL ); +return kvm_pte_valid (pte) && + ((memattr ==KVM_S2_MEMATTR (pgt,NORMAL )) || + (memattr ==KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS ))); }static bool stage2_pte_executable (kvm_pte_t pte)arch/arm64/kvm/mmu.c +3
-5@@ -1647,12 +1647,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,* not a permission fault implies a translation fault which * means mapping the page for the first time */ -if (mte_allowed) { +if (mte_allowed)sanitise_mte_tags (kvm, pfn, vma_pagesize); - }else { - ret = -EFAULT ; - goto out_unlock; - } +else + prot |=KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; }if (writable)@@ -1721,6 +1719,15 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)kvm_set_pfn_accessed (kvm_pte_to_pfn (pte)); }
Quoted 10 lines
arch/arm64/kvm/mmu.c static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa) · line 1728
Quoted 7 lines
arch/arm64/kvm/mmu.c +7
-0+static inline void kvm_prepare_notagaccess_exit (struct kvm_vcpu *vcpu, +gpa_t gpa,gpa_t size) +{ + vcpu->run->exit_reason =KVM_EXIT_ARM_NOTAG_ACCESS ; + vcpu->run->notag_access.flags =0 ; + vcpu->run->notag_access.gpa = gpa; + vcpu->run->notag_access.size = size;Why does size matter here? It seems pretty pointless.
I agree that since the exit is only generated on fault and size will always be PAGE_SIZE, size field is not required. I will remove that in the next update.
Quoted 50 lines
Quoted 50 lines
arch/arm64/kvm/mmu.c +12
-1+} +/** * kvm_handle_guest_abort - handles all 2nd stage aborts * @vcpu: the VCPU pointer @@ -1833,6 +1840,14 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) gfn = ipa >>PAGE_SHIFT ; memslot =gfn_to_memslot (vcpu->kvm, gfn); + +if (kvm_vcpu_trap_is_tagaccess (vcpu)) { +/* exit to host and handle the error */ +kvm_prepare_notagaccess_exit (vcpu, gfn <<PAGE_SHIFT ,PAGE_SIZE ); + ret =0 ; + goto out; + } + hva =gfn_to_hva_memslot_prot (memslot, gfn, &writable); write_fault =kvm_is_write_fault (vcpu);if (kvm_is_error_hva (hva) || (write_fault && !writable)) {@@ -2145,7 +2160,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,if (!vma)break ; -if (kvm_has_mte (kvm) && !kvm_vma_mte_allowed (vma)) { +if (kvm_has_mte (kvm) && !system_supports_notagaccess () && + !kvm_vma_mte_allowed (vma)) { ret = -EINVAL ;break ; }include/uapi/linux/kvm.h +4
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -446,6 +447,12 @@ struct kvm_run {__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags;
Quoted 9 lines
include/uapi/linux/kvm.h struct kvm_run { · line 458
Quoted 6 lines
include/uapi/linux/kvm.h +3
-0+__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };How do you plan to handle the same thing for NV?
I will have to admit that I have not looked at nested virtualization. I will update that as part of the next update.
-aneesh
Marc Zyngier
maintainerRe: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Quoted 16 lines · Aneesh Kumar K.V
Quoted 13 lines · Marc Zyngier
Quoted 8 lines · Aneesh Kumar K.V (Arm)
Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
I only have questions here:
Quoted 7 lines
Quoted 3 lines
- what is the benefit of such approach? why shouldn't that be the kernel's job to fix it?
IMHO leaving that policy decision to VMM makes the kernel changes simpler. In most cases, VMM will kill the guest, because these restrictions of
MTE_ALLOWEDare applied at the memslot/vma.
Where is that captured? The whole idea behind FEAT_MTE_PERM was that it would be the hypervisor's task to lazily allocate MTE-capable memory as tagged-access would occur.
Quoted 7 lines
Quoted 4 lines
- where is the documentation for this new userspace ABI?
I will add the details if we agree that this should be a separate EXIT as outlined in this patch.
Woooot??? This isn't a *DETAIL*. This is the very first thing you should write. Everything *else* is an implementation detail.
Quoted 9 lines
Quoted 4 lines
- are you expecting the VMM to create a new memslot for this?
I guess there are examples of configs where some memory regions are backed by page cache where we don't directly use those memory regions as allocatable memory in the guest. This change allows us to enable MTE in such configs.
This doesn't answer my question. What is expected sequence a VMM should apply to provide tagged memory to the guest?
Quoted 8 lines
Quoted 3 lines
- where is the example of a VMM using this?
I do have changes to kvmtool which won't do any fixup on receiving that VM exit. I expect other VMM to do the same by default when they get a VM exit with an unknown
exit_reason. So unless VMM wants to do any special handling, we don't need any change in the VMM.
OK, so this has never been tested. I'm sorry, but for something of this magnitude, with expected userspace interactions, and requiring VMM handling, I want to see the full end-to-end thing.
Quoted 106 lines
arm/kvm-cpu.c +13
-0@@ -239,6 +239,17 @@ static bool handle_memoryfault(struct kvm_cpu *vcpu)return true; } +static bool handle_notag_access (struct kvm_cpu *vcpu) +{ +u64 gpa = vcpu->kvm_run->memory_fault.gpa; +u64 size = vcpu->kvm_run->memory_fault.size; + +/* For now VMM just panic */ +pr_err ("Tag Access to a wrong memory region 0x%lx size 0x%lx\n" , + (unsigned long )gpa, (unsigned long )size); +return false; +} +bool kvm_cpu__handle_exit (struct kvm_cpu *vcpu) {switch (vcpu->kvm_run->exit_reason) {@@ -246,6 +257,8 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)return handle_hypercall (vcpu);case KVM_EXIT_MEMORY_FAULT :return handle_memoryfault (vcpu); +case KVM_EXIT_ARM_NOTAG_ACCESS : +return handle_notag_access (vcpu); }return false;include/linux/kvm.h +8
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -429,10 +430,17 @@ struct kvm_run {/* KVM_EXIT_MEMORY_FAULT */ struct {#define KVM_MEMORY_EXIT_FLAG_PRIVATE (1ULL << 3) +#define KVM_MEMORY_EXIT_FLAG_NOTAGACCESS (1ULL << 4)__u64 flags;__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };Quoted 36 lines
Quoted 36 lines
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/kvm/hyp/pgtable.c | 16 +++++++++++++--- arch/arm64/kvm/mmu.c | 28 ++++++++++++++++++++++------ include/uapi/linux/kvm.h | 7 +++++++ 5 files changed, 48 insertions(+), 9 deletions(-)arch/arm64/include/asm/kvm_emulate.h +5
-0@@ -373,6 +373,11 @@ static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)return kvm_vcpu_trap_is_iabt (vcpu) && !kvm_vcpu_abt_iss1tw (vcpu); } +static inline bool kvm_vcpu_trap_is_tagaccess (const struct kvm_vcpu *vcpu) +{ +return !!(ESR_ELx_ISS2 (kvm_vcpu_get_esr (vcpu)) & ESR_ELx_TagAccess); +} + static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) {return kvm_vcpu_get_esr (vcpu) & ESR_ELx_FSC;arch/arm64/include/asm/kvm_pgtable.h +0
-0@@ -252,6 +252,7 @@ enum kvm_pgtable_prot {Quoted 6 lines
arch/arm64/include/asm/kvm_pgtable.h enum kvm_pgtable_prot { · line 255
Quoted 3 lines
arch/arm64/include/asm/kvm_pgtable.h +1
-0KVM_PGTABLE_PROT_DEVICE =BIT (3 ),KVM_PGTABLE_PROT_NORMAL_NC =BIT (4 ), + KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS = BIT(5),This seems wrong. NOTAGACCESS is a *permission*, not a memory type.
Quoted 6 lines
Are you suggesting the name is wrong? The memory attribute value I wanted to use is
MemAttr[3:0] = 0b0100 which is Normal, NoTagAccess, writeback cacheable.
I am following the changes similar to
KVM_PGTABLE_PROT_NORMAL_NC.
But that's entirely different. This really isn't a memory type. Quite the opposite, actually. This is a stage-2 permission setting, which you are conflating with the actual memory type.
Also, I don't see why that should be incompatible with something other than Normal memory.
Quoted 40 lines
Quoted 30 lines
Quoted 30 lines
arch/arm64/include/asm/kvm_pgtable.h +0
-0KVM_PGTABLE_PROT_SW0 =BIT (55 ), KVM_PGTABLE_PROT_SW1 = BIT(56),arch/arm64/kvm/hyp/pgtable.c +7
-2@@ -677,9 +677,11 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p {kvm_pte_t attr;u32 sh =KVM_PTE_LEAF_ATTR_LO_S2_SH_IS ; +unsigned long prot_mask =KVM_PGTABLE_PROT_DEVICE | +KVM_PGTABLE_PROT_NORMAL_NC | +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; -switch (prot & (KVM_PGTABLE_PROT_DEVICE | -KVM_PGTABLE_PROT_NORMAL_NC )) { +switch (prot & prot_mask) {case KVM_PGTABLE_PROT_DEVICE |KVM_PGTABLE_PROT_NORMAL_NC :return -EINVAL ;case KVM_PGTABLE_PROT_DEVICE :@@ -692,6 +694,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot preturn -EINVAL ; attr =KVM_S2_MEMATTR (pgt,NORMAL_NC );break ; +case KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS : +if (system_supports_notagaccess ()) + attr =KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS );Quoted 9 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p · line 702
Quoted 3 lines
arch/arm64/kvm/hyp/pgtable.c +3
-0+else +return -EINVAL ; +break ;How do you see this working when migrating a VM from one host to another, one that supports
FEAT_MTE_PERMand one that doesn't? The current assumptions are that the VMM will replay the *exact same* setup on the target host, and this obviously doesn't work.
Quoted 3 lines
I missed looking at kvm migration. I guess I will have to expose this as a capability and only allow migration if the target also supports the same capability?
I don't think so. This doesn't affect the guest at all, as the guest doesn't know anything about S2, unless you decide to expose FEAT_MTE_PERM to NV.
It affects the VMM though, and that's because you are making a difference in handling between having FEAT_MTE_PERM or not. Given that this is invisible to userspace, that's not a great design.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Hi Marc,
Sorry for the delay in response, I was looking at nested virt and migration changes so that I can get your feedback on those.
Quoted 22 lines · Marc Zyngier
Quoted 20 lines · Aneesh Kumar K.V
Quoted 15 lines · Marc Zyngier
Quoted 8 lines · Aneesh Kumar K.V (Arm)
Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
I only have questions here:
- what is the benefit of such approach? why shouldn't that be the kernel's job to fix it?
Quoted 8 lines
Quoted 3 lines
IMHO leaving that policy decision to VMM makes the kernel changes simpler. In most cases, VMM will kill the guest, because these restrictions of
MTE_ALLOWEDare applied at the memslot/vma.Where is that captured? The whole idea behind
FEAT_MTE_PERMwas that it would be the hypervisor's task to lazily allocate MTE-capable memory as tagged-access would occur.
Lazily allocating MTE-capable memory requires changes to different kernel subsystems and previous attempts got dropped [1] because it was not clear whether the benefit of saving 3% memory overhead was worth the complexity we add to the kernel.
This patchset is not looking at that feature. Instead, it can be used to enable MTE in configurations that currently won't allow MTE. One such example is libkrun which includes linux kernel as firmware in a dynamically linked library (libkrunfw). libkrun can insert the kernel region which got mmaped as part of the library load, directly into the guest memory map instead of copying the kernel. Such a guest config can't enable MTE currently even though we will never use the newly inserted memory regions as tag access memory.
Similarly, virtiofs dax support can use a page cache region as virtio-shm region. We can use MTE_PERM to enable MTE in this config.
[1] https://lore.kernel.org/all/[email protected]/
Quoted 148 lines
Quoted 7 lines
Quoted 4 lines
- where is the documentation for this new userspace ABI?
I will add the details if we agree that this should be a separate EXIT as outlined in this patch.
Woooot??? This isn't a *DETAIL*. This is the very first thing you should write. Everything *else* is an implementation detail.
Quoted 9 lines
Quoted 4 lines
- are you expecting the VMM to create a new memslot for this?
I guess there are examples of configs where some memory regions are backed by page cache where we don't directly use those memory regions as allocatable memory in the guest. This change allows us to enable MTE in such configs.
This doesn't answer my question. What is expected sequence a VMM should apply to provide tagged memory to the guest?
Quoted 8 lines
Quoted 3 lines
- where is the example of a VMM using this?
I do have changes to kvmtool which won't do any fixup on receiving that VM exit. I expect other VMM to do the same by default when they get a VM exit with an unknown
exit_reason. So unless VMM wants to do any special handling, we don't need any change in the VMM.OK, so this has never been tested. I'm sorry, but for something of this magnitude, with expected userspace interactions, and requiring VMM handling, I want to see the full end-to-end thing.
Quoted 111 lines
arm/kvm-cpu.c +13
-0@@ -239,6 +239,17 @@ static bool handle_memoryfault(struct kvm_cpu *vcpu)return true; } +static bool handle_notag_access (struct kvm_cpu *vcpu) +{ +u64 gpa = vcpu->kvm_run->memory_fault.gpa; +u64 size = vcpu->kvm_run->memory_fault.size; + +/* For now VMM just panic */ +pr_err ("Tag Access to a wrong memory region 0x%lx size 0x%lx\n" , + (unsigned long )gpa, (unsigned long )size); +return false; +} +bool kvm_cpu__handle_exit (struct kvm_cpu *vcpu) {switch (vcpu->kvm_run->exit_reason) {@@ -246,6 +257,8 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)return handle_hypercall (vcpu);case KVM_EXIT_MEMORY_FAULT :return handle_memoryfault (vcpu); +case KVM_EXIT_ARM_NOTAG_ACCESS : +return handle_notag_access (vcpu); }return false;include/linux/kvm.h +8
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -429,10 +430,17 @@ struct kvm_run {/* KVM_EXIT_MEMORY_FAULT */ struct {#define KVM_MEMORY_EXIT_FLAG_PRIVATE (1ULL << 3) +#define KVM_MEMORY_EXIT_FLAG_NOTAGACCESS (1ULL << 4)__u64 flags;__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };Quoted 36 lines
Quoted 36 lines
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/kvm/hyp/pgtable.c | 16 +++++++++++++--- arch/arm64/kvm/mmu.c | 28 ++++++++++++++++++++++------ include/uapi/linux/kvm.h | 7 +++++++ 5 files changed, 48 insertions(+), 9 deletions(-)arch/arm64/include/asm/kvm_emulate.h +5
-0@@ -373,6 +373,11 @@ static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)return kvm_vcpu_trap_is_iabt (vcpu) && !kvm_vcpu_abt_iss1tw (vcpu); } +static inline bool kvm_vcpu_trap_is_tagaccess (const struct kvm_vcpu *vcpu) +{ +return !!(ESR_ELx_ISS2 (kvm_vcpu_get_esr (vcpu)) & ESR_ELx_TagAccess); +} + static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) {return kvm_vcpu_get_esr (vcpu) & ESR_ELx_FSC;arch/arm64/include/asm/kvm_pgtable.h +0
-0@@ -252,6 +252,7 @@ enum kvm_pgtable_prot {Quoted 6 lines
arch/arm64/include/asm/kvm_pgtable.h enum kvm_pgtable_prot { · line 255
Quoted 3 lines
arch/arm64/include/asm/kvm_pgtable.h +1
-0KVM_PGTABLE_PROT_DEVICE =BIT (3 ),KVM_PGTABLE_PROT_NORMAL_NC =BIT (4 ), + KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS = BIT(5),This seems wrong. NOTAGACCESS is a *permission*, not a memory type.
Are you suggesting the name is wrong? The memory attribute value I wanted to use is
MemAttr[3:0] = 0b0100 which is Normal, NoTagAccess, writeback cacheable.
Quoted 9 lines
Quoted 1 line
I am following the changes similar to
KVM_PGTABLE_PROT_NORMAL_NC.But that's entirely different. This really isn't a memory type. Quite the opposite, actually. This is a stage-2 permission setting, which you are conflating with the actual memory type.
Also, I don't see why that should be incompatible with something other than Normal memory.
I am sorry that i am not able to follow you here. I am confused whether your feedback is related to the use of MemAttr[3:0] values or w.r.t the usage of KVM_PGTABLE_PROT_NORMAL_NOTAG_ACCESS. If you can explain more on how you would like to see the changes I can incorporate them in the next update.
Quoted 44 lines
Quoted 40 lines
Quoted 30 lines
Quoted 30 lines
arch/arm64/include/asm/kvm_pgtable.h +0
-0KVM_PGTABLE_PROT_SW0 =BIT (55 ), KVM_PGTABLE_PROT_SW1 = BIT(56),arch/arm64/kvm/hyp/pgtable.c +7
-2@@ -677,9 +677,11 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p {kvm_pte_t attr;u32 sh =KVM_PTE_LEAF_ATTR_LO_S2_SH_IS ; +unsigned long prot_mask =KVM_PGTABLE_PROT_DEVICE | +KVM_PGTABLE_PROT_NORMAL_NC | +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; -switch (prot & (KVM_PGTABLE_PROT_DEVICE | -KVM_PGTABLE_PROT_NORMAL_NC )) { +switch (prot & prot_mask) {case KVM_PGTABLE_PROT_DEVICE |KVM_PGTABLE_PROT_NORMAL_NC :return -EINVAL ;case KVM_PGTABLE_PROT_DEVICE :@@ -692,6 +694,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot preturn -EINVAL ; attr =KVM_S2_MEMATTR (pgt,NORMAL_NC );break ; +case KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS : +if (system_supports_notagaccess ()) + attr =KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS );Quoted 9 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p · line 702
Quoted 3 lines
arch/arm64/kvm/hyp/pgtable.c +3
-0+else +return -EINVAL ; +break ;How do you see this working when migrating a VM from one host to another, one that supports
FEAT_MTE_PERMand one that doesn't? The current assumptions are that the VMM will replay the *exact same* setup on the target host, and this obviously doesn't work.Quoted 3 lines
I missed looking at kvm migration. I guess I will have to expose this as a capability and only allow migration if the target also supports the same capability?
Quoted 8 lines
I don't think so. This doesn't affect the guest at all, as the guest doesn't know anything about S2, unless you decide to expose
FEAT_MTE_PERMto NV.It affects the VMM though, and that's because you are making a difference in handling between having
FEAT_MTE_PERMor not. Given that this is invisible to userspace, that's not a great design.
With migration, I guess i can prevent migration to a target that doesn't support FEAT_MTE_PERM by updating set_one_reg to check for the feature support. Something like this
modified arch/arm64/kvm/sys_regs.c
@@ -1514,7 +1514,7 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
const struct sys_reg_desc *r)
{
u32 id = reg_to_encoding(r);
- u64 val;
+ u64 val, mask;
if (sysreg_visible_as_raz(vcpu, r))
return 0;
@@ -1529,8 +1529,10 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME);
break;
case SYS_ID_AA64PFR2_EL1:
- /* We only expose FPMR */
- val &= ID_AA64PFR2_EL1_FPMR;
+ mask = ID_AA64PFR2_EL1_FPMR;
+ if (system_supports_notagaccess())
+ mask |= ID_AA64PFR2_EL1_MTEPERM;
+ val &= mask;
break;
case SYS_ID_AA64ISAR1_EL1:
if (!vcpu_has_ptrauth(vcpu))
@@ -2382,7 +2384,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
ID_AA64PFR0_EL1_AdvSIMD |
ID_AA64PFR0_EL1_FP), },
ID_SANITISED(ID_AA64PFR1_EL1),
- ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR),
+ ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR |
+ ID_AA64PFR2_EL1_MTEPERM),
ID_UNALLOCATED(4,3),
ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0), ID_HIDDEN(ID_AA64SMFR0_EL1),
For nested virtualization, I guess it is useful to support this for nested guest. This implies L2 stage2 in L1 will have to support Normal,NoTagAccess attribute. ie,
in L0 kvm, if the guest abort is on a nested smmu, then we walk the L2 stage2 in L1 and see if the memory attribute is NoTagAccess. If yes we inject a s2 fault to L1. Do you agree this is a good design?
-aneesh
Marc Zyngier
maintainerRe: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Quoted 40 lines · Aneesh Kumar K.V
Hi Marc,
Sorry for the delay in response, I was looking at nested virt and migration changes so that I can get your feedback on those.
Quoted 22 lines · Marc Zyngier
Quoted 20 lines · Aneesh Kumar K.V
Quoted 15 lines · Marc Zyngier
Quoted 8 lines · Aneesh Kumar K.V (Arm)
Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
I only have questions here:
- what is the benefit of such approach? why shouldn't that be the kernel's job to fix it?
Quoted 8 lines
Quoted 3 lines
IMHO leaving that policy decision to VMM makes the kernel changes simpler. In most cases, VMM will kill the guest, because these restrictions of
MTE_ALLOWEDare applied at the memslot/vma.Where is that captured? The whole idea behind
FEAT_MTE_PERMwas that it would be the hypervisor's task to lazily allocate MTE-capable memory as tagged-access would occur.
Quoted 4 lines
Lazily allocating MTE-capable memory requires changes to different kernel subsystems and previous attempts got dropped [1] because it was not clear whether the benefit of saving 3% memory overhead was worth the complexity we add to the kernel.
That's not the point. Tagged memory doesn't have to cover the whole of physical memory, and it can be statically allocated. The architecture doesn't mandate that all of the memory is MTE-capable.
Quoted 11 lines
This patchset is not looking at that feature. Instead, it can be used to enable MTE in configurations that currently won't allow MTE. One such example is libkrun which includes linux kernel as firmware in a dynamically linked library (libkrunfw). libkrun can insert the kernel region which got mmaped as part of the library load, directly into the guest memory map instead of copying the kernel. Such a guest config can't enable MTE currently even though we will never use the newly inserted memory regions as tag access memory.
Similarly, virtiofs dax support can use a page cache region as virtio-shm region. We can use
MTE_PERMto enable MTE in this config.
And this use case doesn't contradict what I am stating above. But it definitely contradicts what you wrote: "In most cases, VMM will kill the guest".
So which one is it?
Quoted 161 lines
[1] https://lore.kernel.org/all/[email protected]/
Quoted 148 lines
Quoted 7 lines
Quoted 4 lines
- where is the documentation for this new userspace ABI?
I will add the details if we agree that this should be a separate EXIT as outlined in this patch.
Woooot??? This isn't a *DETAIL*. This is the very first thing you should write. Everything *else* is an implementation detail.
Quoted 9 lines
Quoted 4 lines
- are you expecting the VMM to create a new memslot for this?
I guess there are examples of configs where some memory regions are backed by page cache where we don't directly use those memory regions as allocatable memory in the guest. This change allows us to enable MTE in such configs.
This doesn't answer my question. What is expected sequence a VMM should apply to provide tagged memory to the guest?
Quoted 8 lines
Quoted 3 lines
- where is the example of a VMM using this?
I do have changes to kvmtool which won't do any fixup on receiving that VM exit. I expect other VMM to do the same by default when they get a VM exit with an unknown
exit_reason. So unless VMM wants to do any special handling, we don't need any change in the VMM.OK, so this has never been tested. I'm sorry, but for something of this magnitude, with expected userspace interactions, and requiring VMM handling, I want to see the full end-to-end thing.
Quoted 111 lines
arm/kvm-cpu.c +13
-0@@ -239,6 +239,17 @@ static bool handle_memoryfault(struct kvm_cpu *vcpu)return true; } +static bool handle_notag_access (struct kvm_cpu *vcpu) +{ +u64 gpa = vcpu->kvm_run->memory_fault.gpa; +u64 size = vcpu->kvm_run->memory_fault.size; + +/* For now VMM just panic */ +pr_err ("Tag Access to a wrong memory region 0x%lx size 0x%lx\n" , + (unsigned long )gpa, (unsigned long )size); +return false; +} +bool kvm_cpu__handle_exit (struct kvm_cpu *vcpu) {switch (vcpu->kvm_run->exit_reason) {@@ -246,6 +257,8 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)return handle_hypercall (vcpu);case KVM_EXIT_MEMORY_FAULT :return handle_memoryfault (vcpu); +case KVM_EXIT_ARM_NOTAG_ACCESS : +return handle_notag_access (vcpu); }return false;include/linux/kvm.h +8
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -429,10 +430,17 @@ struct kvm_run {/* KVM_EXIT_MEMORY_FAULT */ struct {#define KVM_MEMORY_EXIT_FLAG_PRIVATE (1ULL << 3) +#define KVM_MEMORY_EXIT_FLAG_NOTAGACCESS (1ULL << 4)__u64 flags;__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;/* Fix the size of the union. */ char padding[256 ]; };Quoted 36 lines
Quoted 36 lines
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>--- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_pgtable.h | 1 + arch/arm64/kvm/hyp/pgtable.c | 16 +++++++++++++--- arch/arm64/kvm/mmu.c | 28 ++++++++++++++++++++++------ include/uapi/linux/kvm.h | 7 +++++++ 5 files changed, 48 insertions(+), 9 deletions(-)arch/arm64/include/asm/kvm_emulate.h +5
-0@@ -373,6 +373,11 @@ static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)return kvm_vcpu_trap_is_iabt (vcpu) && !kvm_vcpu_abt_iss1tw (vcpu); } +static inline bool kvm_vcpu_trap_is_tagaccess (const struct kvm_vcpu *vcpu) +{ +return !!(ESR_ELx_ISS2 (kvm_vcpu_get_esr (vcpu)) & ESR_ELx_TagAccess); +} + static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu) {return kvm_vcpu_get_esr (vcpu) & ESR_ELx_FSC;arch/arm64/include/asm/kvm_pgtable.h +0
-0@@ -252,6 +252,7 @@ enum kvm_pgtable_prot {Quoted 6 lines
arch/arm64/include/asm/kvm_pgtable.h enum kvm_pgtable_prot { · line 255
Quoted 3 lines
arch/arm64/include/asm/kvm_pgtable.h +1
-0KVM_PGTABLE_PROT_DEVICE =BIT (3 ),KVM_PGTABLE_PROT_NORMAL_NC =BIT (4 ), + KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS = BIT(5),This seems wrong. NOTAGACCESS is a *permission*, not a memory type.
Are you suggesting the name is wrong? The memory attribute value I wanted to use is
MemAttr[3:0] = 0b0100 which is Normal, NoTagAccess, writeback cacheable.
Quoted 9 lines
Quoted 1 line
I am following the changes similar to
KVM_PGTABLE_PROT_NORMAL_NC.But that's entirely different. This really isn't a memory type. Quite the opposite, actually. This is a stage-2 permission setting, which you are conflating with the actual memory type.
Also, I don't see why that should be incompatible with something other than Normal memory.
Quoted 5 lines
I am sorry that i am not able to follow you here. I am confused whether your feedback is related to the use of MemAttr[3:0] values or w.r.t the usage of
KVM_PGTABLE_PROT_NORMAL_NOTAG_ACCESS. If you can explain more on how you would like to see the changes I can incorporate them in the next update.
There is a clear difference between permission and memory types. What if I want to have non-cacheable memory being tagged?
Quoted 69 lines
Quoted 45 lines
Quoted 41 lines
Quoted 31 lines
Quoted 31 lines
arch/arm64/include/asm/kvm_pgtable.h +0
-0KVM_PGTABLE_PROT_SW0 =BIT (55 ), KVM_PGTABLE_PROT_SW1 = BIT(56),arch/arm64/kvm/hyp/pgtable.c +7
-2@@ -677,9 +677,11 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p {kvm_pte_t attr;u32 sh =KVM_PTE_LEAF_ATTR_LO_S2_SH_IS ; +unsigned long prot_mask =KVM_PGTABLE_PROT_DEVICE | +KVM_PGTABLE_PROT_NORMAL_NC | +KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS ; -switch (prot & (KVM_PGTABLE_PROT_DEVICE | -KVM_PGTABLE_PROT_NORMAL_NC )) { +switch (prot & prot_mask) {case KVM_PGTABLE_PROT_DEVICE |KVM_PGTABLE_PROT_NORMAL_NC :return -EINVAL ;case KVM_PGTABLE_PROT_DEVICE :@@ -692,6 +694,12 @@ static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot preturn -EINVAL ; attr =KVM_S2_MEMATTR (pgt,NORMAL_NC );break ; +case KVM_PGTABLE_PROT_NORMAL_NOTAGACCESS : +if (system_supports_notagaccess ()) + attr =KVM_S2_MEMATTR (pgt,NORMAL_NOTAGACCESS );Quoted 9 lines
arch/arm64/kvm/hyp/pgtable.c static int stage2_set_prot_attr(struct kvm_pgtable *pgt, enum kvm_pgtable_prot p · line 702
Quoted 3 lines
arch/arm64/kvm/hyp/pgtable.c +3
-0+else +return -EINVAL ; +break ;How do you see this working when migrating a VM from one host to another, one that supports
FEAT_MTE_PERMand one that doesn't? The current assumptions are that the VMM will replay the *exact same* setup on the target host, and this obviously doesn't work.Quoted 3 lines
I missed looking at kvm migration. I guess I will have to expose this as a capability and only allow migration if the target also supports the same capability?
Quoted 8 lines
I don't think so. This doesn't affect the guest at all, as the guest doesn't know anything about S2, unless you decide to expose
FEAT_MTE_PERMto NV.It affects the VMM though, and that's because you are making a difference in handling between having
FEAT_MTE_PERMor not. Given that this is invisible to userspace, that's not a great design.With migration, I guess i can prevent migration to a target that doesn't support
FEAT_MTE_PERMby updatingset_one_regto check for the feature support. Something like thismodified
arch/arm64/kvm/sys_regs.c(quoted diff) +1
-1@@ -1514,7 +1514,7 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) { u32 id = reg_to_encoding(r); - u64 val; + u64 val, mask; if (sysreg_visible_as_raz(vcpu, r)) return 0;
Review static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu, · line 1534
Quoted 8 lines
(quoted diff) +3
-2@@ -1529,8 +1529,10 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu, val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME); break; case SYS_ID_AA64PFR2_EL1: - /* We only expose FPMR */ - val &= ID_AA64PFR2_EL1_FPMR; + mask = ID_AA64PFR2_EL1_FPMR; + if (system_supports_notagaccess()) + mask |= ID_AA64PFR2_EL1_MTEPERM;
But this doesn't matter for an EL1 guest. Only for an EL2 guest.For an EL1 guest you need to validate the support at the API level.
Quoted 16 lines
(quoted diff) +3
-1+ val &= mask; break; case SYS_ID_AA64ISAR1_EL1: if (!vcpu_has_ptrauth(vcpu))@@ -2382,7 +2384,8 @@ static const struct sys_reg_desc sys_reg_descs[] = { ID_AA64PFR0_EL1_AdvSIMD | ID_AA64PFR0_EL1_FP), }, ID_SANITISED(ID_AA64PFR1_EL1), - ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR), + ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR | + ID_AA64PFR2_EL1_MTEPERM), ID_UNALLOCATED(4,3), ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0), ID_HIDDEN(ID_AA64SMFR0_EL1),
Quoted 7 lines
For nested virtualization, I guess it is useful to support this for nested guest. This implies L2 stage2 in L1 will have to support Normal,NoTagAccess attribute. ie,
in L0 kvm, if the guest abort is on a nested smmu, then we walk the L2 stage2 in L1 and see if the memory attribute is NoTagAccess. If yes we inject a s2 fault to L1. Do you agree this is a good design?
Yes. But you need to add that support.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Catalin Marinas
maintainerRe: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
(joining the thread as well, though not sure I'm bringing anything new)
Quoted 22 lines · Marc Zyngier
Quoted 21 lines · Aneesh Kumar K.V
Quoted 20 lines · Marc Zyngier
Quoted 18 lines · Aneesh Kumar K.V
Quoted 9 lines · Marc Zyngier
Quoted 7 lines · Aneesh Kumar K.V (Arm)
Currently, the kernel won't start a guest if the MTE feature is enabled and the guest RAM is backed by memory which doesn't support access tags. Update this such that the kernel uses the NoTagAccess memory attribute while mapping pages from VMAs for which MTE is not allowed. The fault from accessing the access tags with such pages is forwarded to VMM so that VMM can decide to kill the guest or remap the pages so that access tag storage is allowed.
Quoted 4 lines
I only have questions here:
- what is the benefit of such approach? why shouldn't that be the kernel's job to fix it?
IMHO leaving that policy decision to VMM makes the kernel changes simpler. In most cases, VMM will kill the guest, because these restrictions of
MTE_ALLOWEDare applied at the memslot/vma.
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Where is that captured? The whole idea behind
FEAT_MTE_PERMwas that it would be the hypervisor's task to lazily allocate MTE-capable memory as tagged-access would occur.Lazily allocating MTE-capable memory requires changes to different kernel subsystems and previous attempts got dropped [1] because it was not clear whether the benefit of saving 3% memory overhead was worth the complexity we add to the kernel.
I'd say the most complex part in Alex's approach was the need to reuse the tag storage for classic data and kick the pages around when some other page needs to store tags in there. That approach is pretty much dead.
In theory, having MTE and non-MTE memory (heterogeneous) without a carveout reuse would be a bit more manageable - not that far from the NUMA migration and at least you only migrate the page being accessed rather than unrelated ones where the tags need to go. But this was not Alex's goal for Android since people were asking for the reuse of the 3% carveout rather than a smaller carveout.
Other future deployments, CXL-attached memory etc. may benefit from a new scheme but I wouldn't rush in implementing anything in the kernel for now.
The VMM may be in a better position to manage such heterogeneous memory for the guest if it knows the capabilities of the slots (e.g. some DAX mmap() vs anonymous mmap()). This would require the VMM replacing a page within a slot from one memory type to another (while preserving the data). I don't think we have a concrete use-case yet to be worth the hassle.
Quoted 3 lines
That's not the point. Tagged memory doesn't have to cover the whole of physical memory, and it can be statically allocated. The architecture doesn't mandate that all of the memory is MTE-capable.
There's some vague wording that general purpose memory should be MTE capable if FEAT_MTE2 or later is advertised. But that's not well defined and one can have other types of memory in the physical space (e.g. CXL) that don't support tags. Last time I looked we still haven't got a way to describe memory capabilities in firmware.
For the time being, I think a real use-case for FEAT_MTE_PERM is in the context of cacheable MMIO (there are some patches around from Nvidia to do this with VFIO). That memory, if exposed to guest as WB and the guest enables MTE, may trigger some SErrors. With FEAT_MTE_PERM KVM could trap and inject a fault back into the guest - maybe SEA. Is it easier to do this from KVM itself or we would rather exit to the VMM and let it handle? The latter allows room for other fancier things in the future but the former may be quicker, in the absence of other strong use-cases.
Quoted 8 lines
Quoted 8 lines
This patchset is not looking at that feature. Instead, it can be used to enable MTE in configurations that currently won't allow MTE. One such example is libkrun which includes linux kernel as firmware in a dynamically linked library (libkrunfw). libkrun can insert the kernel region which got mmaped as part of the library load, directly into the guest memory map instead of copying the kernel. Such a guest config can't enable MTE currently even though we will never use the newly inserted memory regions as tag access memory.
I've never played with libkrunfw. Does it handle inserting a Linux kernel? Such approach may not work well with MTE. The kernel frees the init text/data sections back into the page allocator. With MTE advertised as present, the guest will try to reuse that memory, potentially as tagged. However, since the VMM mmap'ed the guest kernel from a file, VM_MTE is not supported in the VMM address space. Even if the mapping is MAP_PRIVATE and the page copied on write, the vma remains the original one associated with the file, so VM_MTE_ALLOWED not set.
We could revisit this and allow mprotect(PROT_MTE) to force the CoW on private file mappings but we'd need some strong requirement for this (MTE+libkrunfw could be such thing if people need this combination).
Quoted 6 lines
Quoted 2 lines
Similarly, virtiofs dax support can use a page cache region as virtio-shm region. We can use
MTE_PERMto enable MTE in this config.And this use case doesn't contradict what I am stating above. But it definitely contradicts what you wrote: "In most cases, VMM will kill the guest".
To simplify things, I think whatever is presented to the VM as standard RAM (typically present at the VM boot) should either support MTE or MTE will be disabled for the guest. For other types of memory, whether WB MMIO or RAM presented as virtio-(pmem, shm etc.) backed by files in the VMM, the VM should be aware it is not standard RAM and should not attempt to enable MTE on it. If it does (either by mistake or malice), FEAT_MTE_PERM should trap and inject a fault back into the guest (or kill it altogether but for debugging, I'd rather inject a fault if possible).
Signature · 1 line
Catalin
Re: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
In reply to Aneesh Kumar K.V (Arm)Quoted 13 lines · Aneesh Kumar K.V (Arm)
include/uapi/linux/kvm.h +1
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */
include/uapi/linux/kvm.h struct kvm_run { · line 455
Quoted 9 lines
include/uapi/linux/kvm.h +6
-0@@ -446,6 +447,12 @@ struct kvm_run {__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;
Can you please look into reusing the memory fault exit infrastructure?
The entire point of that is for KVM to tell the VMM it cannot make forward progress because of ${SOMETHING} unexpected at the specified GPA. You can add a new flag that describes tag access.
Signature · 2 lines
Thanks, Oliver
Re: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Hi Oliver,
Thanks for reviewing the changes.
Quoted 8 lines · Oliver Upton
On Mon, Oct 28, 2024 at 03:10:14PM +0530, Aneesh Kumar K.V (Arm) wrote:
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>---
I have used KVM_EXIT_MEMORY_FAULT as part of the initial prototype.
Quoted 25 lines
Quoted 13 lines
include/uapi/linux/kvm.h +1
-0@@ -178,6 +178,7 @@ struct kvm_xen_exit {#define KVM_EXIT_NOTIFY 37#define KVM_EXIT_LOONGARCH_IOCSR 38#define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_ARM_NOTAG_ACCESS 40/* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ include/uapi/linux/kvm.h struct kvm_run { · line 455
Quoted 9 lines
include/uapi/linux/kvm.h +6
-0@@ -446,6 +447,12 @@ struct kvm_run {__u64 gpa;__u64 size; } memory_fault; +/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;Can you please look into reusing the memory fault exit infrastructure?
Quoted 4 lines
The entire point of that is for KVM to tell the VMM it cannot make forward progress because of ${SOMETHING} unexpected at the specified GPA. You can add a new flag that describes tag access.
The only reason I dropped the change was because the flag will be very much arm64 specific. Based on your feedback, I will switch to KVM_EXIT_MEMORY_FAULT in the next update.
-aneesh
Re: [PATCH 4/4] arm64: mte: Use stage-2 NoTagAccess memory attribute if supported
Quoted 11 lines · Aneesh Kumar K.V
Hi Oliver,
Thanks for reviewing the changes.
Quoted 2 lines · Oliver Upton
On Mon, Oct 28, 2024 at 03:10:14PM +0530, Aneesh Kumar K.V (Arm) wrote:
Quoted 7 lines
Quoted 5 lines
NOTE: We could also use
KVM_EXIT_MEMORY_FAULTfor this. I chose to add a new EXIT type because this is arm64 specific exit type.Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>---
I have used
KVM_EXIT_MEMORY_FAULTas part of the initial prototype.
Ah, apologies, I clearly didn't read the changelog.
Quoted 9 lines
Quoted 9 lines
include/uapi/linux/kvm.h struct kvm_run { · line 455
Quoted 6 lines
include/uapi/linux/kvm.h +6
-0+/* KVM_EXIT_ARM_NOTAG_ACCESS */ +struct { +__u64 flags; +__u64 gpa; +__u64 size; + } notag_access;Can you please look into reusing the memory fault exit infrastructure?
Quoted 7 lines
Quoted 4 lines
The entire point of that is for KVM to tell the VMM it cannot make forward progress because of ${SOMETHING} unexpected at the specified GPA. You can add a new flag that describes tag access.
The only reason I dropped the change was because the flag will be very much arm64 specific.
Eh, making it arm64-specific is very much the right call. There's no shortage of available bits in that structure, and we should make no attempt to provide a generic description of what is otherwise a very architecture-specific thing.
Quoted 2 lines
Based on your feedback, I will switch to
KVM_EXIT_MEMORY_FAULTin the next update.
Excellent, thanks!
Signature · 2 lines
Thanks, Oliver