All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-03 18:05 ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-03 18:05 UTC (permalink / raw)
  To: qemu-devel
  Cc: kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones,
	Richard Henderson, Peter Maydell

Add basic support for Pointer Authentication when running a KVM
guest and that the host supports it, loosely based on the SVE
support.

Although the feature is enabled by default when the host advertises
it, it is possible to disable it by setting the 'pauth=off' CPU
property. The 'pauth' comment is removed from cpu-features.rst,
as it is now common to both TCG and KVM.

Tested on an Apple M1 running 5.16-rc6.

Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
* From v1:
  - Drop 'pauth' documentation
  - Make the TCG path common to both TCG and KVM
  - Some tidying up

 docs/system/arm/cpu-features.rst |  4 ----
 target/arm/cpu.c                 | 14 ++++----------
 target/arm/cpu.h                 |  1 +
 target/arm/cpu64.c               | 33 ++++++++++++++++++++++++++++----
 target/arm/kvm64.c               | 21 ++++++++++++++++++++
 5 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 584eb17097..3e626c4b68 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -217,10 +217,6 @@ TCG VCPU Features
 TCG VCPU features are CPU features that are specific to TCG.
 Below is the list of TCG VCPU features and their descriptions.
 
-  pauth                    Enable or disable ``FEAT_Pauth``, pointer
-                           authentication.  By default, the feature is
-                           enabled with ``-cpu max``.
-
   pauth-impdef             When ``FEAT_Pauth`` is enabled, either the
                            *impdef* (Implementation Defined) algorithm
                            is enabled or the *architected* QARMA algorithm
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a211804fd3..d96cc4ef18 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1380,18 +1380,11 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
             return;
         }
 
-        /*
-         * KVM does not support modifications to this feature.
-         * We have not registered the cpu properties when KVM
-         * is in use, so the user will not be able to set them.
-         */
-        if (!kvm_enabled()) {
-            arm_cpu_pauth_finalize(cpu, &local_err);
-            if (local_err != NULL) {
+	arm_cpu_pauth_finalize(cpu, &local_err);
+	if (local_err != NULL) {
                 error_propagate(errp, local_err);
                 return;
-            }
-        }
+	}
     }
 
     if (kvm_enabled()) {
@@ -2091,6 +2084,7 @@ static void arm_host_initfn(Object *obj)
     kvm_arm_set_cpu_features_from_host(cpu);
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         aarch64_add_sve_properties(obj);
+        aarch64_add_pauth_properties(obj);
     }
 #else
     hvf_arm_set_cpu_features_from_host(cpu);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e33f37b70a..c6a4d50e82 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1076,6 +1076,7 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 void aarch64_sve_change_el(CPUARMState *env, int old_el,
                            int new_el, bool el0_a64);
 void aarch64_add_sve_properties(Object *obj);
+void aarch64_add_pauth_properties(Object *obj);
 
 /*
  * SVE registers are encoded in KVM's memory in an endianness-invariant format.
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 15245a60a8..d5c0bce1c4 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -630,6 +630,17 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
     int arch_val = 0, impdef_val = 0;
     uint64_t t;
 
+    if (kvm_enabled()) {
+        if (cpu->prop_pauth) {
+            if (!cpu_isar_feature(aa64_pauth, cpu)) {
+                error_setg(errp, "'pauth' feature not supported by KVM on this host");
+            }
+
+            return;
+        }
+        /* Fall through to disable PAuth */
+    }
+
     /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
     if (cpu->prop_pauth) {
         if (cpu->prop_pauth_impdef) {
@@ -655,6 +666,23 @@ static Property arm_cpu_pauth_property =
 static Property arm_cpu_pauth_impdef_property =
     DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
 
+void aarch64_add_pauth_properties(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    /* Default to PAUTH on, with the architected algorithm on TCG. */
+    qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
+    if (kvm_enabled()) {
+        /*
+         * Mirror PAuth support from the probed sysregs back into the
+         * property for KVM. Is it just a bit backward? Yes it is!
+         */
+        cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
+    } else {
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+    }
+}
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -829,13 +857,10 @@ static void aarch64_max_initfn(Object *obj)
         cpu->dcz_blocksize = 7; /*  512 bytes */
 #endif
 
-        /* Default to PAUTH on, with the architected algorithm. */
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
-
         bitmap_fill(cpu->sve_vq_supported, ARM_MAX_VQ);
     }
 
+    aarch64_add_pauth_properties(obj);
     aarch64_add_sve_properties(obj);
     object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
                         cpu_max_set_sve_max_vq, NULL, NULL);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e790d6c9a5..5c425bc074 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -491,6 +491,12 @@ static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
     return ioctl(fd, KVM_GET_ONE_REG, &idreg);
 }
 
+static bool kvm_arm_pauth_supported(void)
+{
+    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     struct kvm_vcpu_init init = { .target = -1, };
 
+    /*
+     * Ask for Pointer Authentication if supported. We can't play the
+     * SVE trick of synthetising the ID reg as KVM won't tell us
+     * whether we have the architected or IMPDEF version of PAuth, so
+     * we have to use the actual ID regs.
+     */
+    if (kvm_arm_pauth_supported()) {
+        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
+    }
+
     if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
         return false;
     }
@@ -865,6 +882,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
         assert(kvm_arm_sve_supported());
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
     }
+    if (cpu_isar_feature(aa64_pauth, cpu)) {
+	    cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+					  1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-03 18:05 ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-03 18:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kvm, Richard Henderson, kernel-team, kvmarm

Add basic support for Pointer Authentication when running a KVM
guest and that the host supports it, loosely based on the SVE
support.

Although the feature is enabled by default when the host advertises
it, it is possible to disable it by setting the 'pauth=off' CPU
property. The 'pauth' comment is removed from cpu-features.rst,
as it is now common to both TCG and KVM.

Tested on an Apple M1 running 5.16-rc6.

Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
* From v1:
  - Drop 'pauth' documentation
  - Make the TCG path common to both TCG and KVM
  - Some tidying up

 docs/system/arm/cpu-features.rst |  4 ----
 target/arm/cpu.c                 | 14 ++++----------
 target/arm/cpu.h                 |  1 +
 target/arm/cpu64.c               | 33 ++++++++++++++++++++++++++++----
 target/arm/kvm64.c               | 21 ++++++++++++++++++++
 5 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 584eb17097..3e626c4b68 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -217,10 +217,6 @@ TCG VCPU Features
 TCG VCPU features are CPU features that are specific to TCG.
 Below is the list of TCG VCPU features and their descriptions.
 
-  pauth                    Enable or disable ``FEAT_Pauth``, pointer
-                           authentication.  By default, the feature is
-                           enabled with ``-cpu max``.
-
   pauth-impdef             When ``FEAT_Pauth`` is enabled, either the
                            *impdef* (Implementation Defined) algorithm
                            is enabled or the *architected* QARMA algorithm
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a211804fd3..d96cc4ef18 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1380,18 +1380,11 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
             return;
         }
 
-        /*
-         * KVM does not support modifications to this feature.
-         * We have not registered the cpu properties when KVM
-         * is in use, so the user will not be able to set them.
-         */
-        if (!kvm_enabled()) {
-            arm_cpu_pauth_finalize(cpu, &local_err);
-            if (local_err != NULL) {
+	arm_cpu_pauth_finalize(cpu, &local_err);
+	if (local_err != NULL) {
                 error_propagate(errp, local_err);
                 return;
-            }
-        }
+	}
     }
 
     if (kvm_enabled()) {
@@ -2091,6 +2084,7 @@ static void arm_host_initfn(Object *obj)
     kvm_arm_set_cpu_features_from_host(cpu);
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         aarch64_add_sve_properties(obj);
+        aarch64_add_pauth_properties(obj);
     }
 #else
     hvf_arm_set_cpu_features_from_host(cpu);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e33f37b70a..c6a4d50e82 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1076,6 +1076,7 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 void aarch64_sve_change_el(CPUARMState *env, int old_el,
                            int new_el, bool el0_a64);
 void aarch64_add_sve_properties(Object *obj);
+void aarch64_add_pauth_properties(Object *obj);
 
 /*
  * SVE registers are encoded in KVM's memory in an endianness-invariant format.
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 15245a60a8..d5c0bce1c4 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -630,6 +630,17 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
     int arch_val = 0, impdef_val = 0;
     uint64_t t;
 
+    if (kvm_enabled()) {
+        if (cpu->prop_pauth) {
+            if (!cpu_isar_feature(aa64_pauth, cpu)) {
+                error_setg(errp, "'pauth' feature not supported by KVM on this host");
+            }
+
+            return;
+        }
+        /* Fall through to disable PAuth */
+    }
+
     /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
     if (cpu->prop_pauth) {
         if (cpu->prop_pauth_impdef) {
@@ -655,6 +666,23 @@ static Property arm_cpu_pauth_property =
 static Property arm_cpu_pauth_impdef_property =
     DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
 
+void aarch64_add_pauth_properties(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    /* Default to PAUTH on, with the architected algorithm on TCG. */
+    qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
+    if (kvm_enabled()) {
+        /*
+         * Mirror PAuth support from the probed sysregs back into the
+         * property for KVM. Is it just a bit backward? Yes it is!
+         */
+        cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
+    } else {
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+    }
+}
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -829,13 +857,10 @@ static void aarch64_max_initfn(Object *obj)
         cpu->dcz_blocksize = 7; /*  512 bytes */
 #endif
 
-        /* Default to PAUTH on, with the architected algorithm. */
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
-
         bitmap_fill(cpu->sve_vq_supported, ARM_MAX_VQ);
     }
 
+    aarch64_add_pauth_properties(obj);
     aarch64_add_sve_properties(obj);
     object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
                         cpu_max_set_sve_max_vq, NULL, NULL);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e790d6c9a5..5c425bc074 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -491,6 +491,12 @@ static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
     return ioctl(fd, KVM_GET_ONE_REG, &idreg);
 }
 
+static bool kvm_arm_pauth_supported(void)
+{
+    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     struct kvm_vcpu_init init = { .target = -1, };
 
+    /*
+     * Ask for Pointer Authentication if supported. We can't play the
+     * SVE trick of synthetising the ID reg as KVM won't tell us
+     * whether we have the architected or IMPDEF version of PAuth, so
+     * we have to use the actual ID regs.
+     */
+    if (kvm_arm_pauth_supported()) {
+        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
+    }
+
     if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
         return false;
     }
@@ -865,6 +882,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
         assert(kvm_arm_sve_supported());
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
     }
+    if (cpu_isar_feature(aa64_pauth, cpu)) {
+	    cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+					  1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
-- 
2.30.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-03 18:05 ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-03 18:05 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Andrew Jones, kvm, Richard Henderson, Eric Auger,
	kernel-team, kvmarm

Add basic support for Pointer Authentication when running a KVM
guest and that the host supports it, loosely based on the SVE
support.

Although the feature is enabled by default when the host advertises
it, it is possible to disable it by setting the 'pauth=off' CPU
property. The 'pauth' comment is removed from cpu-features.rst,
as it is now common to both TCG and KVM.

Tested on an Apple M1 running 5.16-rc6.

Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
* From v1:
  - Drop 'pauth' documentation
  - Make the TCG path common to both TCG and KVM
  - Some tidying up

 docs/system/arm/cpu-features.rst |  4 ----
 target/arm/cpu.c                 | 14 ++++----------
 target/arm/cpu.h                 |  1 +
 target/arm/cpu64.c               | 33 ++++++++++++++++++++++++++++----
 target/arm/kvm64.c               | 21 ++++++++++++++++++++
 5 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 584eb17097..3e626c4b68 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -217,10 +217,6 @@ TCG VCPU Features
 TCG VCPU features are CPU features that are specific to TCG.
 Below is the list of TCG VCPU features and their descriptions.
 
-  pauth                    Enable or disable ``FEAT_Pauth``, pointer
-                           authentication.  By default, the feature is
-                           enabled with ``-cpu max``.
-
   pauth-impdef             When ``FEAT_Pauth`` is enabled, either the
                            *impdef* (Implementation Defined) algorithm
                            is enabled or the *architected* QARMA algorithm
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a211804fd3..d96cc4ef18 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1380,18 +1380,11 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
             return;
         }
 
-        /*
-         * KVM does not support modifications to this feature.
-         * We have not registered the cpu properties when KVM
-         * is in use, so the user will not be able to set them.
-         */
-        if (!kvm_enabled()) {
-            arm_cpu_pauth_finalize(cpu, &local_err);
-            if (local_err != NULL) {
+	arm_cpu_pauth_finalize(cpu, &local_err);
+	if (local_err != NULL) {
                 error_propagate(errp, local_err);
                 return;
-            }
-        }
+	}
     }
 
     if (kvm_enabled()) {
@@ -2091,6 +2084,7 @@ static void arm_host_initfn(Object *obj)
     kvm_arm_set_cpu_features_from_host(cpu);
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         aarch64_add_sve_properties(obj);
+        aarch64_add_pauth_properties(obj);
     }
 #else
     hvf_arm_set_cpu_features_from_host(cpu);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e33f37b70a..c6a4d50e82 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1076,6 +1076,7 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 void aarch64_sve_change_el(CPUARMState *env, int old_el,
                            int new_el, bool el0_a64);
 void aarch64_add_sve_properties(Object *obj);
+void aarch64_add_pauth_properties(Object *obj);
 
 /*
  * SVE registers are encoded in KVM's memory in an endianness-invariant format.
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 15245a60a8..d5c0bce1c4 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -630,6 +630,17 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
     int arch_val = 0, impdef_val = 0;
     uint64_t t;
 
+    if (kvm_enabled()) {
+        if (cpu->prop_pauth) {
+            if (!cpu_isar_feature(aa64_pauth, cpu)) {
+                error_setg(errp, "'pauth' feature not supported by KVM on this host");
+            }
+
+            return;
+        }
+        /* Fall through to disable PAuth */
+    }
+
     /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
     if (cpu->prop_pauth) {
         if (cpu->prop_pauth_impdef) {
@@ -655,6 +666,23 @@ static Property arm_cpu_pauth_property =
 static Property arm_cpu_pauth_impdef_property =
     DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
 
+void aarch64_add_pauth_properties(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    /* Default to PAUTH on, with the architected algorithm on TCG. */
+    qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
+    if (kvm_enabled()) {
+        /*
+         * Mirror PAuth support from the probed sysregs back into the
+         * property for KVM. Is it just a bit backward? Yes it is!
+         */
+        cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
+    } else {
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+    }
+}
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -829,13 +857,10 @@ static void aarch64_max_initfn(Object *obj)
         cpu->dcz_blocksize = 7; /*  512 bytes */
 #endif
 
-        /* Default to PAUTH on, with the architected algorithm. */
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
-
         bitmap_fill(cpu->sve_vq_supported, ARM_MAX_VQ);
     }
 
+    aarch64_add_pauth_properties(obj);
     aarch64_add_sve_properties(obj);
     object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
                         cpu_max_set_sve_max_vq, NULL, NULL);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e790d6c9a5..5c425bc074 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -491,6 +491,12 @@ static int read_sys_reg64(int fd, uint64_t *pret, uint64_t id)
     return ioctl(fd, KVM_GET_ONE_REG, &idreg);
 }
 
+static bool kvm_arm_pauth_supported(void)
+{
+    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     struct kvm_vcpu_init init = { .target = -1, };
 
+    /*
+     * Ask for Pointer Authentication if supported. We can't play the
+     * SVE trick of synthetising the ID reg as KVM won't tell us
+     * whether we have the architected or IMPDEF version of PAuth, so
+     * we have to use the actual ID regs.
+     */
+    if (kvm_arm_pauth_supported()) {
+        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
+    }
+
     if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
         return false;
     }
@@ -865,6 +882,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
         assert(kvm_arm_sve_supported());
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
     }
+    if (cpu_isar_feature(aa64_pauth, cpu)) {
+	    cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+					  1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-03 18:05 ` Marc Zyngier
  (?)
@ 2022-01-05 14:58   ` Andrew Jones
  -1 siblings, 0 replies; 24+ messages in thread
From: Andrew Jones @ 2022-01-05 14:58 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: qemu-devel, kvmarm, kvm, kernel-team, Eric Auger,
	Richard Henderson, Peter Maydell

On Mon, Jan 03, 2022 at 06:05:07PM +0000, Marc Zyngier wrote:
> Add basic support for Pointer Authentication when running a KVM
> guest and that the host supports it, loosely based on the SVE
> support.
> 
> Although the feature is enabled by default when the host advertises
> it, it is possible to disable it by setting the 'pauth=off' CPU
> property. The 'pauth' comment is removed from cpu-features.rst,
> as it is now common to both TCG and KVM.
> 
> Tested on an Apple M1 running 5.16-rc6.
> 
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> * From v1:
>   - Drop 'pauth' documentation
>   - Make the TCG path common to both TCG and KVM
>   - Some tidying up
> 
>  docs/system/arm/cpu-features.rst |  4 ----
>  target/arm/cpu.c                 | 14 ++++----------
>  target/arm/cpu.h                 |  1 +
>  target/arm/cpu64.c               | 33 ++++++++++++++++++++++++++++----
>  target/arm/kvm64.c               | 21 ++++++++++++++++++++
>  5 files changed, 55 insertions(+), 18 deletions(-)
>

 
Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-05 14:58   ` Andrew Jones
  0 siblings, 0 replies; 24+ messages in thread
From: Andrew Jones @ 2022-01-05 14:58 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, Richard Henderson, qemu-devel, kernel-team, kvmarm

On Mon, Jan 03, 2022 at 06:05:07PM +0000, Marc Zyngier wrote:
> Add basic support for Pointer Authentication when running a KVM
> guest and that the host supports it, loosely based on the SVE
> support.
> 
> Although the feature is enabled by default when the host advertises
> it, it is possible to disable it by setting the 'pauth=off' CPU
> property. The 'pauth' comment is removed from cpu-features.rst,
> as it is now common to both TCG and KVM.
> 
> Tested on an Apple M1 running 5.16-rc6.
> 
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> * From v1:
>   - Drop 'pauth' documentation
>   - Make the TCG path common to both TCG and KVM
>   - Some tidying up
> 
>  docs/system/arm/cpu-features.rst |  4 ----
>  target/arm/cpu.c                 | 14 ++++----------
>  target/arm/cpu.h                 |  1 +
>  target/arm/cpu64.c               | 33 ++++++++++++++++++++++++++++----
>  target/arm/kvm64.c               | 21 ++++++++++++++++++++
>  5 files changed, 55 insertions(+), 18 deletions(-)
>

 
Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-05 14:58   ` Andrew Jones
  0 siblings, 0 replies; 24+ messages in thread
From: Andrew Jones @ 2022-01-05 14:58 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Peter Maydell, kvm, Richard Henderson, qemu-devel, Eric Auger,
	kernel-team, kvmarm

On Mon, Jan 03, 2022 at 06:05:07PM +0000, Marc Zyngier wrote:
> Add basic support for Pointer Authentication when running a KVM
> guest and that the host supports it, loosely based on the SVE
> support.
> 
> Although the feature is enabled by default when the host advertises
> it, it is possible to disable it by setting the 'pauth=off' CPU
> property. The 'pauth' comment is removed from cpu-features.rst,
> as it is now common to both TCG and KVM.
> 
> Tested on an Apple M1 running 5.16-rc6.
> 
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> * From v1:
>   - Drop 'pauth' documentation
>   - Make the TCG path common to both TCG and KVM
>   - Some tidying up
> 
>  docs/system/arm/cpu-features.rst |  4 ----
>  target/arm/cpu.c                 | 14 ++++----------
>  target/arm/cpu.h                 |  1 +
>  target/arm/cpu64.c               | 33 ++++++++++++++++++++++++++++----
>  target/arm/kvm64.c               | 21 ++++++++++++++++++++
>  5 files changed, 55 insertions(+), 18 deletions(-)
>

 
Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-03 18:05 ` Marc Zyngier
  (?)
@ 2022-01-05 21:36   ` Richard Henderson
  -1 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-05 21:36 UTC (permalink / raw)
  To: Marc Zyngier, qemu-devel
  Cc: kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones, Peter Maydell

On 1/3/22 10:05 AM, Marc Zyngier wrote:
> -        /*
> -         * KVM does not support modifications to this feature.
> -         * We have not registered the cpu properties when KVM
> -         * is in use, so the user will not be able to set them.
> -         */
> -        if (!kvm_enabled()) {
> -            arm_cpu_pauth_finalize(cpu, &local_err);
> -            if (local_err != NULL) {
> +	arm_cpu_pauth_finalize(cpu, &local_err);
> +	if (local_err != NULL) {
>                   error_propagate(errp, local_err);
>                   return;
> -            }
> -        }
> +	}

Looks like the indentation is off?

> +static bool kvm_arm_pauth_supported(void)
> +{
> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> +}

Do we really need to have them both set to play the game?  Given that the only thing that 
happens is that we disable whatever host support exists, can we have "pauth enabled" mean 
whatever subset the host has?


> @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>        */
>       struct kvm_vcpu_init init = { .target = -1, };
>   
> +    /*
> +     * Ask for Pointer Authentication if supported. We can't play the
> +     * SVE trick of synthetising the ID reg as KVM won't tell us

synthesizing

> +     * whether we have the architected or IMPDEF version of PAuth, so
> +     * we have to use the actual ID regs.
> +     */
> +    if (kvm_arm_pauth_supported()) {
> +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);

Align the two 1's.

Otherwise, it looks good.


r~

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-05 21:36   ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-05 21:36 UTC (permalink / raw)
  To: Marc Zyngier, qemu-devel; +Cc: kvm, kernel-team, kvmarm

On 1/3/22 10:05 AM, Marc Zyngier wrote:
> -        /*
> -         * KVM does not support modifications to this feature.
> -         * We have not registered the cpu properties when KVM
> -         * is in use, so the user will not be able to set them.
> -         */
> -        if (!kvm_enabled()) {
> -            arm_cpu_pauth_finalize(cpu, &local_err);
> -            if (local_err != NULL) {
> +	arm_cpu_pauth_finalize(cpu, &local_err);
> +	if (local_err != NULL) {
>                   error_propagate(errp, local_err);
>                   return;
> -            }
> -        }
> +	}

Looks like the indentation is off?

> +static bool kvm_arm_pauth_supported(void)
> +{
> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> +}

Do we really need to have them both set to play the game?  Given that the only thing that 
happens is that we disable whatever host support exists, can we have "pauth enabled" mean 
whatever subset the host has?


> @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>        */
>       struct kvm_vcpu_init init = { .target = -1, };
>   
> +    /*
> +     * Ask for Pointer Authentication if supported. We can't play the
> +     * SVE trick of synthetising the ID reg as KVM won't tell us

synthesizing

> +     * whether we have the architected or IMPDEF version of PAuth, so
> +     * we have to use the actual ID regs.
> +     */
> +    if (kvm_arm_pauth_supported()) {
> +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);

Align the two 1's.

Otherwise, it looks good.


r~
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-05 21:36   ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-05 21:36 UTC (permalink / raw)
  To: Marc Zyngier, qemu-devel
  Cc: Peter Maydell, Andrew Jones, kvm, Eric Auger, kernel-team, kvmarm

On 1/3/22 10:05 AM, Marc Zyngier wrote:
> -        /*
> -         * KVM does not support modifications to this feature.
> -         * We have not registered the cpu properties when KVM
> -         * is in use, so the user will not be able to set them.
> -         */
> -        if (!kvm_enabled()) {
> -            arm_cpu_pauth_finalize(cpu, &local_err);
> -            if (local_err != NULL) {
> +	arm_cpu_pauth_finalize(cpu, &local_err);
> +	if (local_err != NULL) {
>                   error_propagate(errp, local_err);
>                   return;
> -            }
> -        }
> +	}

Looks like the indentation is off?

> +static bool kvm_arm_pauth_supported(void)
> +{
> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> +}

Do we really need to have them both set to play the game?  Given that the only thing that 
happens is that we disable whatever host support exists, can we have "pauth enabled" mean 
whatever subset the host has?


> @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>        */
>       struct kvm_vcpu_init init = { .target = -1, };
>   
> +    /*
> +     * Ask for Pointer Authentication if supported. We can't play the
> +     * SVE trick of synthetising the ID reg as KVM won't tell us

synthesizing

> +     * whether we have the architected or IMPDEF version of PAuth, so
> +     * we have to use the actual ID regs.
> +     */
> +    if (kvm_arm_pauth_supported()) {
> +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);

Align the two 1's.

Otherwise, it looks good.


r~


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-05 21:36   ` Richard Henderson
  (?)
@ 2022-01-06  9:16     ` Marc Zyngier
  -1 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06  9:16 UTC (permalink / raw)
  To: Richard Henderson
  Cc: qemu-devel, kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones,
	Peter Maydell

Hi Richard,

On Wed, 05 Jan 2022 21:36:55 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/3/22 10:05 AM, Marc Zyngier wrote:
> > -        /*
> > -         * KVM does not support modifications to this feature.
> > -         * We have not registered the cpu properties when KVM
> > -         * is in use, so the user will not be able to set them.
> > -         */
> > -        if (!kvm_enabled()) {
> > -            arm_cpu_pauth_finalize(cpu, &local_err);
> > -            if (local_err != NULL) {
> > +	arm_cpu_pauth_finalize(cpu, &local_err);
> > +	if (local_err != NULL) {
> >                   error_propagate(errp, local_err);
> >                   return;
> > -            }
> > -        }
> > +	}
> 
> Looks like the indentation is off?

Most probably. I only just discovered how to use the QEMU style for
Emacs, and was indenting things by hand before that (yes, pretty
painful and likely to lead to issues (there is a TAB instead of a set
of spaces there...).

> 
> > +static bool kvm_arm_pauth_supported(void)
> > +{
> > +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> > +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> > +}
> 
> Do we really need to have them both set to play the game?  Given that
> the only thing that happens is that we disable whatever host support
> exists, can we have "pauth enabled" mean whatever subset the host has?

The host will always expose either both features or none, and that's
part of the ABI. From the bit of kernel documentation located in
Documentation/virt/kvm/api.rst:

<quote>
4.82 KVM_ARM_VCPU_INIT
----------------------
[...]
        - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.

        - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.
</quote>

KVM will reject the initialisation if only one of the features is
requested, so checking and enabling both makes sense to me.

> 
> > @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> >        */
> >       struct kvm_vcpu_init init = { .target = -1, };
> >   +    /*
> > +     * Ask for Pointer Authentication if supported. We can't play the
> > +     * SVE trick of synthetising the ID reg as KVM won't tell us
> 
> synthesizing

Yup.

> 
> > +     * whether we have the architected or IMPDEF version of PAuth, so
> > +     * we have to use the actual ID regs.
> > +     */
> > +    if (kvm_arm_pauth_supported()) {
> > +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> > +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
> 
> Align the two 1's.

Gah, another of these... Will fix.

> 
> Otherwise, it looks good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06  9:16     ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06  9:16 UTC (permalink / raw)
  To: Richard Henderson; +Cc: kvm, qemu-devel, kernel-team, kvmarm

Hi Richard,

On Wed, 05 Jan 2022 21:36:55 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/3/22 10:05 AM, Marc Zyngier wrote:
> > -        /*
> > -         * KVM does not support modifications to this feature.
> > -         * We have not registered the cpu properties when KVM
> > -         * is in use, so the user will not be able to set them.
> > -         */
> > -        if (!kvm_enabled()) {
> > -            arm_cpu_pauth_finalize(cpu, &local_err);
> > -            if (local_err != NULL) {
> > +	arm_cpu_pauth_finalize(cpu, &local_err);
> > +	if (local_err != NULL) {
> >                   error_propagate(errp, local_err);
> >                   return;
> > -            }
> > -        }
> > +	}
> 
> Looks like the indentation is off?

Most probably. I only just discovered how to use the QEMU style for
Emacs, and was indenting things by hand before that (yes, pretty
painful and likely to lead to issues (there is a TAB instead of a set
of spaces there...).

> 
> > +static bool kvm_arm_pauth_supported(void)
> > +{
> > +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> > +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> > +}
> 
> Do we really need to have them both set to play the game?  Given that
> the only thing that happens is that we disable whatever host support
> exists, can we have "pauth enabled" mean whatever subset the host has?

The host will always expose either both features or none, and that's
part of the ABI. From the bit of kernel documentation located in
Documentation/virt/kvm/api.rst:

<quote>
4.82 KVM_ARM_VCPU_INIT
----------------------
[...]
        - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.

        - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.
</quote>

KVM will reject the initialisation if only one of the features is
requested, so checking and enabling both makes sense to me.

> 
> > @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> >        */
> >       struct kvm_vcpu_init init = { .target = -1, };
> >   +    /*
> > +     * Ask for Pointer Authentication if supported. We can't play the
> > +     * SVE trick of synthetising the ID reg as KVM won't tell us
> 
> synthesizing

Yup.

> 
> > +     * whether we have the architected or IMPDEF version of PAuth, so
> > +     * we have to use the actual ID regs.
> > +     */
> > +    if (kvm_arm_pauth_supported()) {
> > +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> > +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
> 
> Align the two 1's.

Gah, another of these... Will fix.

> 
> Otherwise, it looks good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06  9:16     ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06  9:16 UTC (permalink / raw)
  To: Richard Henderson
  Cc: Peter Maydell, Andrew Jones, kvm, qemu-devel, Eric Auger,
	kernel-team, kvmarm

Hi Richard,

On Wed, 05 Jan 2022 21:36:55 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/3/22 10:05 AM, Marc Zyngier wrote:
> > -        /*
> > -         * KVM does not support modifications to this feature.
> > -         * We have not registered the cpu properties when KVM
> > -         * is in use, so the user will not be able to set them.
> > -         */
> > -        if (!kvm_enabled()) {
> > -            arm_cpu_pauth_finalize(cpu, &local_err);
> > -            if (local_err != NULL) {
> > +	arm_cpu_pauth_finalize(cpu, &local_err);
> > +	if (local_err != NULL) {
> >                   error_propagate(errp, local_err);
> >                   return;
> > -            }
> > -        }
> > +	}
> 
> Looks like the indentation is off?

Most probably. I only just discovered how to use the QEMU style for
Emacs, and was indenting things by hand before that (yes, pretty
painful and likely to lead to issues (there is a TAB instead of a set
of spaces there...).

> 
> > +static bool kvm_arm_pauth_supported(void)
> > +{
> > +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> > +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> > +}
> 
> Do we really need to have them both set to play the game?  Given that
> the only thing that happens is that we disable whatever host support
> exists, can we have "pauth enabled" mean whatever subset the host has?

The host will always expose either both features or none, and that's
part of the ABI. From the bit of kernel documentation located in
Documentation/virt/kvm/api.rst:

<quote>
4.82 KVM_ARM_VCPU_INIT
----------------------
[...]
        - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.

        - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.
</quote>

KVM will reject the initialisation if only one of the features is
requested, so checking and enabling both makes sense to me.

> 
> > @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> >        */
> >       struct kvm_vcpu_init init = { .target = -1, };
> >   +    /*
> > +     * Ask for Pointer Authentication if supported. We can't play the
> > +     * SVE trick of synthetising the ID reg as KVM won't tell us
> 
> synthesizing

Yup.

> 
> > +     * whether we have the architected or IMPDEF version of PAuth, so
> > +     * we have to use the actual ID regs.
> > +     */
> > +    if (kvm_arm_pauth_supported()) {
> > +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> > +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
> 
> Align the two 1's.

Gah, another of these... Will fix.

> 
> Otherwise, it looks good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-06  9:16     ` Marc Zyngier
  (?)
@ 2022-01-06 17:20       ` Richard Henderson
  -1 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-06 17:20 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: qemu-devel, kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones,
	Peter Maydell

On 1/6/22 1:16 AM, Marc Zyngier wrote:
>>> +static bool kvm_arm_pauth_supported(void)
>>> +{
>>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>> +}
>>
>> Do we really need to have them both set to play the game?  Given that
>> the only thing that happens is that we disable whatever host support
>> exists, can we have "pauth enabled" mean whatever subset the host has?
> 
> The host will always expose either both features or none, and that's
> part of the ABI. From the bit of kernel documentation located in
> Documentation/virt/kvm/api.rst:
> 
> <quote>
> 4.82 KVM_ARM_VCPU_INIT
> ----------------------
> [...]
>          - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>            for arm64 only.
>            Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
>            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>            requested.
> 
>          - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>            for arm64 only.
>            Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
>            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>            requested.
> </quote>
> 
> KVM will reject the initialisation if only one of the features is
> requested, so checking and enabling both makes sense to me.

Well, no, that's not what that says.  It says that *if* both host flags are set, then both 
guest flags must be set or both unset.

It's probably all academic anyway, because I can't actually imagine a vendor implementing 
ADDR and not GENERIC, but in theory we ought to be able to support a host with only ADDR.


r~

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 17:20       ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-06 17:20 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, qemu-devel, kernel-team, kvmarm

On 1/6/22 1:16 AM, Marc Zyngier wrote:
>>> +static bool kvm_arm_pauth_supported(void)
>>> +{
>>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>> +}
>>
>> Do we really need to have them both set to play the game?  Given that
>> the only thing that happens is that we disable whatever host support
>> exists, can we have "pauth enabled" mean whatever subset the host has?
> 
> The host will always expose either both features or none, and that's
> part of the ABI. From the bit of kernel documentation located in
> Documentation/virt/kvm/api.rst:
> 
> <quote>
> 4.82 KVM_ARM_VCPU_INIT
> ----------------------
> [...]
>          - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>            for arm64 only.
>            Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
>            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>            requested.
> 
>          - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>            for arm64 only.
>            Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
>            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>            requested.
> </quote>
> 
> KVM will reject the initialisation if only one of the features is
> requested, so checking and enabling both makes sense to me.

Well, no, that's not what that says.  It says that *if* both host flags are set, then both 
guest flags must be set or both unset.

It's probably all academic anyway, because I can't actually imagine a vendor implementing 
ADDR and not GENERIC, but in theory we ought to be able to support a host with only ADDR.


r~
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 17:20       ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-06 17:20 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Peter Maydell, Andrew Jones, kvm, qemu-devel, Eric Auger,
	kernel-team, kvmarm

On 1/6/22 1:16 AM, Marc Zyngier wrote:
>>> +static bool kvm_arm_pauth_supported(void)
>>> +{
>>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>> +}
>>
>> Do we really need to have them both set to play the game?  Given that
>> the only thing that happens is that we disable whatever host support
>> exists, can we have "pauth enabled" mean whatever subset the host has?
> 
> The host will always expose either both features or none, and that's
> part of the ABI. From the bit of kernel documentation located in
> Documentation/virt/kvm/api.rst:
> 
> <quote>
> 4.82 KVM_ARM_VCPU_INIT
> ----------------------
> [...]
>          - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>            for arm64 only.
>            Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
>            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>            requested.
> 
>          - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>            for arm64 only.
>            Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
>            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>            requested.
> </quote>
> 
> KVM will reject the initialisation if only one of the features is
> requested, so checking and enabling both makes sense to me.

Well, no, that's not what that says.  It says that *if* both host flags are set, then both 
guest flags must be set or both unset.

It's probably all academic anyway, because I can't actually imagine a vendor implementing 
ADDR and not GENERIC, but in theory we ought to be able to support a host with only ADDR.


r~


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-06 17:20       ` Richard Henderson
  (?)
@ 2022-01-06 17:29         ` Marc Zyngier
  -1 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06 17:29 UTC (permalink / raw)
  To: Richard Henderson
  Cc: qemu-devel, kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones,
	Peter Maydell

On Thu, 06 Jan 2022 17:20:33 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/6/22 1:16 AM, Marc Zyngier wrote:
> >>> +static bool kvm_arm_pauth_supported(void)
> >>> +{
> >>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> >>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> >>> +}
> >> 
> >> Do we really need to have them both set to play the game?  Given that
> >> the only thing that happens is that we disable whatever host support
> >> exists, can we have "pauth enabled" mean whatever subset the host has?
> > 
> > The host will always expose either both features or none, and that's
> > part of the ABI. From the bit of kernel documentation located in
> > Documentation/virt/kvm/api.rst:
> > 
> > <quote>
> > 4.82 KVM_ARM_VCPU_INIT
> > ----------------------
> > [...]
> >          - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
> >            for arm64 only.
> >            Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
> >            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> >            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> >            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> >            requested.
> > 
> >          - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
> >            for arm64 only.
> >            Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
> >            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> >            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> >            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> >            requested.
> > </quote>
> > 
> > KVM will reject the initialisation if only one of the features is
> > requested, so checking and enabling both makes sense to me.
> 
> Well, no, that's not what that says.  It says that *if* both host
> flags are set, then both guest flags must be set or both unset.

Indeed. But KVM never returns just one flag. It only exposes both or
none.

> It's probably all academic anyway, because I can't actually imagine a
> vendor implementing ADDR and not GENERIC, but in theory we ought to be
> able to support a host with only ADDR.

We explicitly decided against supporting such a configuration. If
someone comes up with such a contraption, guests won't be able to see
it.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 17:29         ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06 17:29 UTC (permalink / raw)
  To: Richard Henderson; +Cc: kvm, qemu-devel, kernel-team, kvmarm

On Thu, 06 Jan 2022 17:20:33 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/6/22 1:16 AM, Marc Zyngier wrote:
> >>> +static bool kvm_arm_pauth_supported(void)
> >>> +{
> >>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> >>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> >>> +}
> >> 
> >> Do we really need to have them both set to play the game?  Given that
> >> the only thing that happens is that we disable whatever host support
> >> exists, can we have "pauth enabled" mean whatever subset the host has?
> > 
> > The host will always expose either both features or none, and that's
> > part of the ABI. From the bit of kernel documentation located in
> > Documentation/virt/kvm/api.rst:
> > 
> > <quote>
> > 4.82 KVM_ARM_VCPU_INIT
> > ----------------------
> > [...]
> >          - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
> >            for arm64 only.
> >            Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
> >            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> >            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> >            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> >            requested.
> > 
> >          - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
> >            for arm64 only.
> >            Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
> >            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> >            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> >            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> >            requested.
> > </quote>
> > 
> > KVM will reject the initialisation if only one of the features is
> > requested, so checking and enabling both makes sense to me.
> 
> Well, no, that's not what that says.  It says that *if* both host
> flags are set, then both guest flags must be set or both unset.

Indeed. But KVM never returns just one flag. It only exposes both or
none.

> It's probably all academic anyway, because I can't actually imagine a
> vendor implementing ADDR and not GENERIC, but in theory we ought to be
> able to support a host with only ADDR.

We explicitly decided against supporting such a configuration. If
someone comes up with such a contraption, guests won't be able to see
it.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 17:29         ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06 17:29 UTC (permalink / raw)
  To: Richard Henderson
  Cc: Peter Maydell, Andrew Jones, kvm, qemu-devel, Eric Auger,
	kernel-team, kvmarm

On Thu, 06 Jan 2022 17:20:33 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/6/22 1:16 AM, Marc Zyngier wrote:
> >>> +static bool kvm_arm_pauth_supported(void)
> >>> +{
> >>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> >>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> >>> +}
> >> 
> >> Do we really need to have them both set to play the game?  Given that
> >> the only thing that happens is that we disable whatever host support
> >> exists, can we have "pauth enabled" mean whatever subset the host has?
> > 
> > The host will always expose either both features or none, and that's
> > part of the ABI. From the bit of kernel documentation located in
> > Documentation/virt/kvm/api.rst:
> > 
> > <quote>
> > 4.82 KVM_ARM_VCPU_INIT
> > ----------------------
> > [...]
> >          - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
> >            for arm64 only.
> >            Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
> >            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> >            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> >            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> >            requested.
> > 
> >          - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
> >            for arm64 only.
> >            Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
> >            If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
> >            both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
> >            KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
> >            requested.
> > </quote>
> > 
> > KVM will reject the initialisation if only one of the features is
> > requested, so checking and enabling both makes sense to me.
> 
> Well, no, that's not what that says.  It says that *if* both host
> flags are set, then both guest flags must be set or both unset.

Indeed. But KVM never returns just one flag. It only exposes both or
none.

> It's probably all academic anyway, because I can't actually imagine a
> vendor implementing ADDR and not GENERIC, but in theory we ought to be
> able to support a host with only ADDR.

We explicitly decided against supporting such a configuration. If
someone comes up with such a contraption, guests won't be able to see
it.

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-06 17:29         ` Marc Zyngier
  (?)
@ 2022-01-06 18:26           ` Richard Henderson
  -1 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-06 18:26 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, qemu-devel, kernel-team, kvmarm

On 1/6/22 9:29 AM, Marc Zyngier wrote:
> On Thu, 06 Jan 2022 17:20:33 +0000,
> Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> On 1/6/22 1:16 AM, Marc Zyngier wrote:
>>>>> +static bool kvm_arm_pauth_supported(void)
>>>>> +{
>>>>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>>>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>>>> +}
>>>>
>>>> Do we really need to have them both set to play the game?  Given that
>>>> the only thing that happens is that we disable whatever host support
>>>> exists, can we have "pauth enabled" mean whatever subset the host has?
>>>
>>> The host will always expose either both features or none, and that's
>>> part of the ABI. From the bit of kernel documentation located in
>>> Documentation/virt/kvm/api.rst:
>>>
>>> <quote>
>>> 4.82 KVM_ARM_VCPU_INIT
>>> ----------------------
>>> [...]
>>>           - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>>>             for arm64 only.
>>>             Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
>>>             If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>>>             both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>>>             KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>>>             requested.
>>>
>>>           - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>>>             for arm64 only.
>>>             Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
>>>             If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>>>             both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>>>             KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>>>             requested.
>>> </quote>
>>>
>>> KVM will reject the initialisation if only one of the features is
>>> requested, so checking and enabling both makes sense to me.
>>
>> Well, no, that's not what that says.  It says that *if* both host
>> flags are set, then both guest flags must be set or both unset.
> 
> Indeed. But KVM never returns just one flag. It only exposes both or
> none.

Mm.  It does beg the question of why KVM exposes multiple bits.  If they must be tied, 
then it only serves to make the interface more complicated than necessary.  We would be 
better served to have a single bit to control all of PAuth.


r~
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 18:26           ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-06 18:26 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: qemu-devel, kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones,
	Peter Maydell

On 1/6/22 9:29 AM, Marc Zyngier wrote:
> On Thu, 06 Jan 2022 17:20:33 +0000,
> Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> On 1/6/22 1:16 AM, Marc Zyngier wrote:
>>>>> +static bool kvm_arm_pauth_supported(void)
>>>>> +{
>>>>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>>>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>>>> +}
>>>>
>>>> Do we really need to have them both set to play the game?  Given that
>>>> the only thing that happens is that we disable whatever host support
>>>> exists, can we have "pauth enabled" mean whatever subset the host has?
>>>
>>> The host will always expose either both features or none, and that's
>>> part of the ABI. From the bit of kernel documentation located in
>>> Documentation/virt/kvm/api.rst:
>>>
>>> <quote>
>>> 4.82 KVM_ARM_VCPU_INIT
>>> ----------------------
>>> [...]
>>>           - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>>>             for arm64 only.
>>>             Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
>>>             If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>>>             both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>>>             KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>>>             requested.
>>>
>>>           - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>>>             for arm64 only.
>>>             Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
>>>             If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>>>             both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>>>             KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>>>             requested.
>>> </quote>
>>>
>>> KVM will reject the initialisation if only one of the features is
>>> requested, so checking and enabling both makes sense to me.
>>
>> Well, no, that's not what that says.  It says that *if* both host
>> flags are set, then both guest flags must be set or both unset.
> 
> Indeed. But KVM never returns just one flag. It only exposes both or
> none.

Mm.  It does beg the question of why KVM exposes multiple bits.  If they must be tied, 
then it only serves to make the interface more complicated than necessary.  We would be 
better served to have a single bit to control all of PAuth.


r~

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 18:26           ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2022-01-06 18:26 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Peter Maydell, Andrew Jones, kvm, qemu-devel, Eric Auger,
	kernel-team, kvmarm

On 1/6/22 9:29 AM, Marc Zyngier wrote:
> On Thu, 06 Jan 2022 17:20:33 +0000,
> Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> On 1/6/22 1:16 AM, Marc Zyngier wrote:
>>>>> +static bool kvm_arm_pauth_supported(void)
>>>>> +{
>>>>> +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
>>>>> +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>>>> +}
>>>>
>>>> Do we really need to have them both set to play the game?  Given that
>>>> the only thing that happens is that we disable whatever host support
>>>> exists, can we have "pauth enabled" mean whatever subset the host has?
>>>
>>> The host will always expose either both features or none, and that's
>>> part of the ABI. From the bit of kernel documentation located in
>>> Documentation/virt/kvm/api.rst:
>>>
>>> <quote>
>>> 4.82 KVM_ARM_VCPU_INIT
>>> ----------------------
>>> [...]
>>>           - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
>>>             for arm64 only.
>>>             Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
>>>             If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>>>             both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>>>             KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>>>             requested.
>>>
>>>           - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
>>>             for arm64 only.
>>>             Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
>>>             If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
>>>             both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
>>>             KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
>>>             requested.
>>> </quote>
>>>
>>> KVM will reject the initialisation if only one of the features is
>>> requested, so checking and enabling both makes sense to me.
>>
>> Well, no, that's not what that says.  It says that *if* both host
>> flags are set, then both guest flags must be set or both unset.
> 
> Indeed. But KVM never returns just one flag. It only exposes both or
> none.

Mm.  It does beg the question of why KVM exposes multiple bits.  If they must be tied, 
then it only serves to make the interface more complicated than necessary.  We would be 
better served to have a single bit to control all of PAuth.


r~


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
  2022-01-06 18:26           ` Richard Henderson
  (?)
@ 2022-01-06 19:25             ` Marc Zyngier
  -1 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06 19:25 UTC (permalink / raw)
  To: Richard Henderson
  Cc: qemu-devel, kvmarm, kvm, kernel-team, Eric Auger, Andrew Jones,
	Peter Maydell

On Thu, 06 Jan 2022 18:26:29 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> Mm.  It does beg the question of why KVM exposes multiple bits.  If
> they must be tied, then it only serves to make the interface more
> complicated than necessary.  We would be better served to have a
> single bit to control all of PAuth.

In hindsight, there is a lot I would change in the KVM userspace ABI,
and a lot I should have pushed back on. Unfortunately, there is little
we can do now to fix it (userspace expecting this behaviour has been
in the wild for almost 3 years already).

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 19:25             ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06 19:25 UTC (permalink / raw)
  To: Richard Henderson; +Cc: kvm, qemu-devel, kernel-team, kvmarm

On Thu, 06 Jan 2022 18:26:29 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> Mm.  It does beg the question of why KVM exposes multiple bits.  If
> they must be tied, then it only serves to make the interface more
> complicated than necessary.  We would be better served to have a
> single bit to control all of PAuth.

In hindsight, there is a lot I would change in the KVM userspace ABI,
and a lot I should have pushed back on. Unfortunately, there is little
we can do now to fix it (userspace expecting this behaviour has been
in the wild for almost 3 years already).

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host
@ 2022-01-06 19:25             ` Marc Zyngier
  0 siblings, 0 replies; 24+ messages in thread
From: Marc Zyngier @ 2022-01-06 19:25 UTC (permalink / raw)
  To: Richard Henderson
  Cc: Peter Maydell, Andrew Jones, kvm, qemu-devel, Eric Auger,
	kernel-team, kvmarm

On Thu, 06 Jan 2022 18:26:29 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> Mm.  It does beg the question of why KVM exposes multiple bits.  If
> they must be tied, then it only serves to make the interface more
> complicated than necessary.  We would be better served to have a
> single bit to control all of PAuth.

In hindsight, there is a lot I would change in the KVM userspace ABI,
and a lot I should have pushed back on. Unfortunately, there is little
we can do now to fix it (userspace expecting this behaviour has been
in the wild for almost 3 years already).

	M.

-- 
Without deviation from the norm, progress is not possible.


^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2022-01-06 19:28 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-03 18:05 [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host Marc Zyngier
2022-01-03 18:05 ` Marc Zyngier
2022-01-03 18:05 ` Marc Zyngier
2022-01-05 14:58 ` Andrew Jones
2022-01-05 14:58   ` Andrew Jones
2022-01-05 14:58   ` Andrew Jones
2022-01-05 21:36 ` Richard Henderson
2022-01-05 21:36   ` Richard Henderson
2022-01-05 21:36   ` Richard Henderson
2022-01-06  9:16   ` Marc Zyngier
2022-01-06  9:16     ` Marc Zyngier
2022-01-06  9:16     ` Marc Zyngier
2022-01-06 17:20     ` Richard Henderson
2022-01-06 17:20       ` Richard Henderson
2022-01-06 17:20       ` Richard Henderson
2022-01-06 17:29       ` Marc Zyngier
2022-01-06 17:29         ` Marc Zyngier
2022-01-06 17:29         ` Marc Zyngier
2022-01-06 18:26         ` Richard Henderson
2022-01-06 18:26           ` Richard Henderson
2022-01-06 18:26           ` Richard Henderson
2022-01-06 19:25           ` Marc Zyngier
2022-01-06 19:25             ` Marc Zyngier
2022-01-06 19:25             ` Marc Zyngier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.