| Message ID | 20210517123239.8025-7-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MTE support for KVM guest | expand |
On Mon, 17 May 2021 13:32:37 +0100, Steven Price <steven.price@arm.com> wrote: > > It's now safe for the VMM to enable MTE in a guest, so expose the > capability to user space. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > arch/arm64/kvm/arm.c | 9 +++++++++ > arch/arm64/kvm/sys_regs.c | 3 +++ > 2 files changed, 12 insertions(+) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 1cb39c0803a4..e89a5e275e25 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -93,6 +93,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > r = 0; > kvm->arch.return_nisv_io_abort_to_user = true; > break; > + case KVM_CAP_ARM_MTE: > + if (!system_supports_mte() || kvm->created_vcpus) > + return -EINVAL; > + r = 0; > + kvm->arch.mte_enabled = true; As far as I can tell from the architecture, this isn't valid for a 32bit guest. M.
On 17/05/2021 18:40, Marc Zyngier wrote: > On Mon, 17 May 2021 13:32:37 +0100, > Steven Price <steven.price@arm.com> wrote: >> >> It's now safe for the VMM to enable MTE in a guest, so expose the >> capability to user space. >> >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> arch/arm64/kvm/arm.c | 9 +++++++++ >> arch/arm64/kvm/sys_regs.c | 3 +++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index 1cb39c0803a4..e89a5e275e25 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -93,6 +93,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, >> r = 0; >> kvm->arch.return_nisv_io_abort_to_user = true; >> break; >> + case KVM_CAP_ARM_MTE: >> + if (!system_supports_mte() || kvm->created_vcpus) >> + return -EINVAL; >> + r = 0; >> + kvm->arch.mte_enabled = true; > > As far as I can tell from the architecture, this isn't valid for a > 32bit guest. Indeed, however the MTE flag is a property of the VM not of the vCPU. And, unless I'm mistaken, it's technically possible to create a VM where some CPUs are 32 bit and some 64 bit. Not that I can see much use of a configuration like that. Steve
On Wed, 19 May 2021 14:26:31 +0100, Steven Price <steven.price@arm.com> wrote: > > On 17/05/2021 18:40, Marc Zyngier wrote: > > On Mon, 17 May 2021 13:32:37 +0100, > > Steven Price <steven.price@arm.com> wrote: > >> > >> It's now safe for the VMM to enable MTE in a guest, so expose the > >> capability to user space. > >> > >> Signed-off-by: Steven Price <steven.price@arm.com> > >> --- > >> arch/arm64/kvm/arm.c | 9 +++++++++ > >> arch/arm64/kvm/sys_regs.c | 3 +++ > >> 2 files changed, 12 insertions(+) > >> > >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > >> index 1cb39c0803a4..e89a5e275e25 100644 > >> --- a/arch/arm64/kvm/arm.c > >> +++ b/arch/arm64/kvm/arm.c > >> @@ -93,6 +93,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > >> r = 0; > >> kvm->arch.return_nisv_io_abort_to_user = true; > >> break; > >> + case KVM_CAP_ARM_MTE: > >> + if (!system_supports_mte() || kvm->created_vcpus) > >> + return -EINVAL; > >> + r = 0; > >> + kvm->arch.mte_enabled = true; > > > > As far as I can tell from the architecture, this isn't valid for a > > 32bit guest. > > Indeed, however the MTE flag is a property of the VM not of the vCPU. > And, unless I'm mistaken, it's technically possible to create a VM where > some CPUs are 32 bit and some 64 bit. Not that I can see much use of a > configuration like that. It looks that this is indeed a bug, and I'm on my way to squash it. Can't believe we allowed that for so long... But the architecture clearly states: <quote> These features are supported in AArch64 state only. </quote> So I'd expect something like: diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c index 956cdc240148..50635eacfa43 100644 --- a/arch/arm64/kvm/reset.c +++ b/arch/arm64/kvm/reset.c @@ -220,7 +220,8 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu) switch (vcpu->arch.target) { default: if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) { - if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1)) { + if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) || + vcpu->kvm->arch.mte_enabled) { ret = -EINVAL; goto out; } that makes it completely impossible to create 32bit CPUs within a MTE-enabled guest. Thanks, M.
On 20/05/2021 11:09, Marc Zyngier wrote: > On Wed, 19 May 2021 14:26:31 +0100, > Steven Price <steven.price@arm.com> wrote: >> >> On 17/05/2021 18:40, Marc Zyngier wrote: >>> On Mon, 17 May 2021 13:32:37 +0100, >>> Steven Price <steven.price@arm.com> wrote: >>>> >>>> It's now safe for the VMM to enable MTE in a guest, so expose the >>>> capability to user space. >>>> >>>> Signed-off-by: Steven Price <steven.price@arm.com> >>>> --- >>>> arch/arm64/kvm/arm.c | 9 +++++++++ >>>> arch/arm64/kvm/sys_regs.c | 3 +++ >>>> 2 files changed, 12 insertions(+) >>>> >>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >>>> index 1cb39c0803a4..e89a5e275e25 100644 >>>> --- a/arch/arm64/kvm/arm.c >>>> +++ b/arch/arm64/kvm/arm.c >>>> @@ -93,6 +93,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, >>>> r = 0; >>>> kvm->arch.return_nisv_io_abort_to_user = true; >>>> break; >>>> + case KVM_CAP_ARM_MTE: >>>> + if (!system_supports_mte() || kvm->created_vcpus) >>>> + return -EINVAL; >>>> + r = 0; >>>> + kvm->arch.mte_enabled = true; >>> >>> As far as I can tell from the architecture, this isn't valid for a >>> 32bit guest. >> >> Indeed, however the MTE flag is a property of the VM not of the vCPU. >> And, unless I'm mistaken, it's technically possible to create a VM where >> some CPUs are 32 bit and some 64 bit. Not that I can see much use of a >> configuration like that. > > It looks that this is indeed a bug, and I'm on my way to squash it. > Can't believe we allowed that for so long... Ah, well if you're going to kill off mixed 32bit/64bit VMs then... > But the architecture clearly states: > > <quote> > These features are supported in AArch64 state only. > </quote> > > So I'd expect something like: > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > index 956cdc240148..50635eacfa43 100644 > --- a/arch/arm64/kvm/reset.c > +++ b/arch/arm64/kvm/reset.c > @@ -220,7 +220,8 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu) > switch (vcpu->arch.target) { > default: > if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) { > - if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1)) { > + if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) || > + vcpu->kvm->arch.mte_enabled) { > ret = -EINVAL; > goto out; > } > > that makes it completely impossible to create 32bit CPUs within a > MTE-enabled guest. ... that makes complete sense, and I'll include this hunk in my next posting. Thanks, Steve
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 1cb39c0803a4..e89a5e275e25 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -93,6 +93,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, r = 0; kvm->arch.return_nisv_io_abort_to_user = true; break; + case KVM_CAP_ARM_MTE: + if (!system_supports_mte() || kvm->created_vcpus) + return -EINVAL; + r = 0; + kvm->arch.mte_enabled = true; + break; default: r = -EINVAL; break; @@ -237,6 +243,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) */ r = 1; break; + case KVM_CAP_ARM_MTE: + r = system_supports_mte(); + break; case KVM_CAP_STEAL_TIME: r = kvm_arm_pvtime_supported(); break; diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 88adbc2286f2..3a749fa0779b 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1308,6 +1308,9 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) { + if (kvm_has_mte(vcpu->kvm)) + return 0; + return REG_HIDDEN; }
It's now safe for the VMM to enable MTE in a guest, so expose the capability to user space. Signed-off-by: Steven Price <steven.price@arm.com> --- arch/arm64/kvm/arm.c | 9 +++++++++ arch/arm64/kvm/sys_regs.c | 3 +++ 2 files changed, 12 insertions(+)