| Message ID | 1559229194-3036-8-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Pointer Authentication and SVE support | expand |
On Thu, 30 May 2019 16:13:12 +0100 Dave Martin <Dave.Martin@arm.com> wrote: > To help the user understand what is going on, amend ptrauth > configuration diagnostic messages to refer to command line options > by the exact name used on the command line. > > Also, provide a clean diagnostic when ptrauth is requested, but not > availble. The generic "Unable to initialise vcpu" message is > rather cryptic for this case. Again I don't see much value in having this as a separate patch, as it basically just touches code introduced two patches earlier. I think it should be merged into 5/9. > Since we now don't attempt to enable ptrauth at all unless KVM > reports the relevant capabilities, remove the error message for > that case too: in any case, we can't diagnose precisely why > KVM_ARM_VCPU_INIT failed, so the message may be misleading. So this leaves the only point where we use .enable_ptrauth to that error message about the host not supporting it. Not sure if that's worth this separate option? Cheers, Andre. > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > arm/aarch64/include/kvm/kvm-config-arch.h | 4 ++-- > arm/aarch64/kvm-cpu.c | 15 +++++++++++---- > arm/kvm-cpu.c | 8 ++------ > 3 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h > index 0279b13..fe1699d 100644 > --- a/arm/aarch64/include/kvm/kvm-config-arch.h > +++ b/arm/aarch64/include/kvm/kvm-config-arch.h > @@ -10,9 +10,9 @@ > "Specify random seed for Kernel Address Space " \ > "Layout Randomization (KASLR)"), \ > OPT_BOOLEAN('\0', "enable-ptrauth", &(cfg)->enable_ptrauth, \ > - "Enables pointer authentication"), \ > + "Enable pointer authentication for the guest"), \ > OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth, \ > - "Disables pointer authentication"), > + "Disable pointer authentication for the guest"), > > #include "arm-common/kvm-config-arch.h" > > diff --git a/arm/aarch64/kvm-cpu.c b/arm/aarch64/kvm-cpu.c > index d3c32e0..08e4fd5 100644 > --- a/arm/aarch64/kvm-cpu.c > +++ b/arm/aarch64/kvm-cpu.c > @@ -130,16 +130,23 @@ static void reset_vcpu_aarch64(struct kvm_cpu *vcpu) > > static void select_ptrauth_feature(struct kvm *kvm, struct kvm_vcpu_init *init) > { > + bool supported; > + > /* Check Pointer Authentication command line arguments. */ > if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth) > - die("Both enable-ptrauth and disable-ptrauth option cannot be present"); > + die("--enable-ptrauth conflicts with --disable-ptrauth"); > + > + supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && > + kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC); > + > + if (kvm->cfg.arch.enable_ptrauth && !supported) > + die("--enable-ptrauth not supported on this host"); > + > /* > * Always enable Pointer Authentication if system supports > * this extension unless disable-ptrauth option is present. > */ > - if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && > - kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) && > - !kvm->cfg.arch.disable_ptrauth) { > + if (supported && !kvm->cfg.arch.disable_ptrauth) { > init->features[0] |= 1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS; > init->features[0] |= 1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC; > } > diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c > index 764fb05..1652f6f 100644 > --- a/arm/kvm-cpu.c > +++ b/arm/kvm-cpu.c > @@ -108,12 +108,8 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) > die("Unable to find matching target"); > } > > - if (err || target->init(vcpu)) { > - if (kvm->cfg.arch.enable_ptrauth) > - die("Unable to initialise vcpu with pointer authentication feature"); > - else > - die("Unable to initialise vcpu"); > - } > + if (err || target->init(vcpu)) > + die("Unable to initialise vcpu"); > > coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, > KVM_CAP_COALESCED_MMIO);
On Fri, May 31, 2019 at 06:05:01PM +0100, Andre Przywara wrote: > On Thu, 30 May 2019 16:13:12 +0100 > Dave Martin <Dave.Martin@arm.com> wrote: > > > To help the user understand what is going on, amend ptrauth > > configuration diagnostic messages to refer to command line options > > by the exact name used on the command line. > > > > Also, provide a clean diagnostic when ptrauth is requested, but not > > availble. The generic "Unable to initialise vcpu" message is > > rather cryptic for this case. > > Again I don't see much value in having this as a separate patch, as it > basically just touches code introduced two patches earlier. I think it > should be merged into 5/9. Same as with the previous patch, I though it was better to keep it separate for review purposes for now, since it makes changes on top of Amit's existing patch. > > Since we now don't attempt to enable ptrauth at all unless KVM > > reports the relevant capabilities, remove the error message for > > that case too: in any case, we can't diagnose precisely why > > KVM_ARM_VCPU_INIT failed, so the message may be misleading. > > So this leaves the only point where we use .enable_ptrauth to that error > message about the host not supporting it. Not sure if that's worth this > separate option? There is indeed a question to be resolved here. See my response to the penultimate patch. Cheers ---Dave
diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h index 0279b13..fe1699d 100644 --- a/arm/aarch64/include/kvm/kvm-config-arch.h +++ b/arm/aarch64/include/kvm/kvm-config-arch.h @@ -10,9 +10,9 @@ "Specify random seed for Kernel Address Space " \ "Layout Randomization (KASLR)"), \ OPT_BOOLEAN('\0', "enable-ptrauth", &(cfg)->enable_ptrauth, \ - "Enables pointer authentication"), \ + "Enable pointer authentication for the guest"), \ OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth, \ - "Disables pointer authentication"), + "Disable pointer authentication for the guest"), #include "arm-common/kvm-config-arch.h" diff --git a/arm/aarch64/kvm-cpu.c b/arm/aarch64/kvm-cpu.c index d3c32e0..08e4fd5 100644 --- a/arm/aarch64/kvm-cpu.c +++ b/arm/aarch64/kvm-cpu.c @@ -130,16 +130,23 @@ static void reset_vcpu_aarch64(struct kvm_cpu *vcpu) static void select_ptrauth_feature(struct kvm *kvm, struct kvm_vcpu_init *init) { + bool supported; + /* Check Pointer Authentication command line arguments. */ if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth) - die("Both enable-ptrauth and disable-ptrauth option cannot be present"); + die("--enable-ptrauth conflicts with --disable-ptrauth"); + + supported = kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && + kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC); + + if (kvm->cfg.arch.enable_ptrauth && !supported) + die("--enable-ptrauth not supported on this host"); + /* * Always enable Pointer Authentication if system supports * this extension unless disable-ptrauth option is present. */ - if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && - kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) && - !kvm->cfg.arch.disable_ptrauth) { + if (supported && !kvm->cfg.arch.disable_ptrauth) { init->features[0] |= 1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS; init->features[0] |= 1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC; } diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c index 764fb05..1652f6f 100644 --- a/arm/kvm-cpu.c +++ b/arm/kvm-cpu.c @@ -108,12 +108,8 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) die("Unable to find matching target"); } - if (err || target->init(vcpu)) { - if (kvm->cfg.arch.enable_ptrauth) - die("Unable to initialise vcpu with pointer authentication feature"); - else - die("Unable to initialise vcpu"); - } + if (err || target->init(vcpu)) + die("Unable to initialise vcpu"); coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);
To help the user understand what is going on, amend ptrauth configuration diagnostic messages to refer to command line options by the exact name used on the command line. Also, provide a clean diagnostic when ptrauth is requested, but not availble. The generic "Unable to initialise vcpu" message is rather cryptic for this case. Since we now don't attempt to enable ptrauth at all unless KVM reports the relevant capabilities, remove the error message for that case too: in any case, we can't diagnose precisely why KVM_ARM_VCPU_INIT failed, so the message may be misleading. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- arm/aarch64/include/kvm/kvm-config-arch.h | 4 ++-- arm/aarch64/kvm-cpu.c | 15 +++++++++++---- arm/kvm-cpu.c | 8 ++------ 3 files changed, 15 insertions(+), 12 deletions(-)