[PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Russell King (Oracle)]

Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
changed their value on the same Neoverse N1 r3p1 hardware which makes
migrating between these kernels on the host problematical.

We already permit changing HPDS in AA64MMFR1_EL1 and CnP in
AA64MMFR2_EL1. We also allow LSM as we allow that in AA64MMFR2_EL1,
so this patch includes support for that even though it isn't required.

Discussing with Marc Zygnier, AC2 should also be fine to be writable,
even though we don't inject an UNDEF if the guest accesses those
registers.

The only questionable one is XNX, execute-never control distinction,
which is also in AA64MMFR1_EL1 but we don't allow to be changed there.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 arch/arm64/kvm/sys_regs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0cc289b17665..f306b38ec341 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2116,6 +2116,16 @@ static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
 	.val = mask,				\
 }
 
+/* sys_reg_desc initialiser for writable AA32 ID registers */
+#define AA32_ID_WRITABLE(name, mask) {		\
+	ID_DESC(name),				\
+	.set_user = set_id_reg,			\
+	.visibility = aa32_id_visibility,	\
+	.reset = kvm_read_sanitised_id_reg,	\
+	.val = mask,				\
+}
+
+
 /*
  * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
  * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
@@ -2270,7 +2280,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	AA32_ID_SANITISED(ID_ISAR3_EL1),
 	AA32_ID_SANITISED(ID_ISAR4_EL1),
 	AA32_ID_SANITISED(ID_ISAR5_EL1),
-	AA32_ID_SANITISED(ID_MMFR4_EL1),
+	AA32_ID_WRITABLE(ID_MMFR4_EL1, ID_MMFR4_EL1_LSM |
+				       ID_MMFR4_EL1_HPDS |
+				       ID_MMFR4_EL1_CnP |
+				       ID_MMFR4_EL1_XNX |
+				       ID_MMFR4_EL1_AC2),
 	AA32_ID_SANITISED(ID_ISAR6_EL1),
 
 	/* CRm=3 */
-- 
2.30.2

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Oliver Upton]

Hi Russell,

On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> changed their value on the same Neoverse N1 r3p1 hardware which makes
> migrating between these kernels on the host problematical.

It'd be helpful to expand a bit more on how these fields changed, better
yet if we can blame it back to a commit. I'm guessing the only direction
of migration you care about is old -> new then?

> We already permit changing HPDS in AA64MMFR1_EL1 and CnP in
> AA64MMFR2_EL1. We also allow LSM as we allow that in AA64MMFR2_EL1,
> so this patch includes support for that even though it isn't required.
> 
> Discussing with Marc Zygnier, AC2 should also be fine to be writable,

typo: Zyngier

> even though we don't inject an UNDEF if the guest accesses those
> registers.
> 
> The only questionable one is XNX, execute-never control distinction,
> which is also in AA64MMFR1_EL1 but we don't allow to be changed there.

It is quite odd for us to expose this field to non-nested VMs in the
first place, though I suppose we will apply an additional set of
restrictions for nested VMs when they come along.

-- 
Thanks,
Oliver

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Russell King (Oracle)]

On Wed, May 01, 2024 at 05:57:20PM +0000, Oliver Upton wrote:
> Hi Russell,
> 
> On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> > Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> > changed their value on the same Neoverse N1 r3p1 hardware which makes
> > migrating between these kernels on the host problematical.
> 
> It'd be helpful to expand a bit more on how these fields changed, better
> yet if we can blame it back to a commit. I'm guessing the only direction
> of migration you care about is old -> new then?

Yes. For MMFR4_EL1, we see 0 with our 5.4 based kernel, and 0x21110
with our 5.15 kernel. I've been looking at tracking down which commit
is responsible but I've come up with nothing that fits.

The only change I can see is the FTR definition for MMFR4, but this
always included 4:7 (AC2) which changed 0 -> 1. So... no idea what
commit caused the change.

There are a load of other registers that we need sorting, but this
is just a test forray into attempting to solve this.

> 
> > We already permit changing HPDS in AA64MMFR1_EL1 and CnP in
> > AA64MMFR2_EL1. We also allow LSM as we allow that in AA64MMFR2_EL1,
> > so this patch includes support for that even though it isn't required.
> > 
> > Discussing with Marc Zygnier, AC2 should also be fine to be writable,
> 
> typo: Zyngier
> 
> > even though we don't inject an UNDEF if the guest accesses those
> > registers.
> > 
> > The only questionable one is XNX, execute-never control distinction,
> > which is also in AA64MMFR1_EL1 but we don't allow to be changed there.
> 
> It is quite odd for us to expose this field to non-nested VMs in the
> first place, though I suppose we will apply an additional set of
> restrictions for nested VMs when they come along.

Yes, it did strike me as odd, since the description seems to imply that
XNX affects EL2, which the VM wouldn't have access to. So I'm not sure
why we don't just force it to zero.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Oliver Upton]

On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> On Wed, May 01, 2024 at 05:57:20PM +0000, Oliver Upton wrote:
> > Hi Russell,
> > 
> > On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> > > Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> > > changed their value on the same Neoverse N1 r3p1 hardware which makes
> > > migrating between these kernels on the host problematical.
> > 
> > It'd be helpful to expand a bit more on how these fields changed, better
> > yet if we can blame it back to a commit. I'm guessing the only direction
> > of migration you care about is old -> new then?
> 
> Yes. For MMFR4_EL1, we see 0 with our 5.4 based kernel, and 0x21110
> with our 5.15 kernel. I've been looking at tracking down which commit
> is responsible but I've come up with nothing that fits.
> 
> The only change I can see is the FTR definition for MMFR4, but this
> always included 4:7 (AC2) which changed 0 -> 1. So... no idea what
> commit caused the change.
> 
> There are a load of other registers that we need sorting, but this
> is just a test forray into attempting to solve this.

Got it, let me see if I can find it then. Do share that list of
problematic registers when you have it, hopefully this isn't the tip of
the iceberg...

> > 
> > > We already permit changing HPDS in AA64MMFR1_EL1 and CnP in
> > > AA64MMFR2_EL1. We also allow LSM as we allow that in AA64MMFR2_EL1,
> > > so this patch includes support for that even though it isn't required.
> > > 
> > > Discussing with Marc Zygnier, AC2 should also be fine to be writable,
> > 
> > typo: Zyngier
> > 
> > > even though we don't inject an UNDEF if the guest accesses those
> > > registers.
> > > 
> > > The only questionable one is XNX, execute-never control distinction,
> > > which is also in AA64MMFR1_EL1 but we don't allow to be changed there.
> > 
> > It is quite odd for us to expose this field to non-nested VMs in the
> > first place, though I suppose we will apply an additional set of
> > restrictions for nested VMs when they come along.
> 
> Yes, it did strike me as odd, since the description seems to imply that
> XNX affects EL2, which the VM wouldn't have access to. So I'm not sure
> why we don't just force it to zero.

Probably because we failed to catch it in the first place and setting to
0 now would be even more UAPI breakage. Meh :-/ I don't see any immediate
issues with the patch, especially since it is fixing a genuine UAPI
breakage in KVM.

-- 
Thanks,
Oliver

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Russell King (Oracle)]

On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
> On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> > On Wed, May 01, 2024 at 05:57:20PM +0000, Oliver Upton wrote:
> > > Hi Russell,
> > > 
> > > On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> > > > Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> > > > changed their value on the same Neoverse N1 r3p1 hardware which makes
> > > > migrating between these kernels on the host problematical.
> > > 
> > > It'd be helpful to expand a bit more on how these fields changed, better
> > > yet if we can blame it back to a commit. I'm guessing the only direction
> > > of migration you care about is old -> new then?
> > 
> > Yes. For MMFR4_EL1, we see 0 with our 5.4 based kernel, and 0x21110
> > with our 5.15 kernel. I've been looking at tracking down which commit
> > is responsible but I've come up with nothing that fits.
> > 
> > The only change I can see is the FTR definition for MMFR4, but this
> > always included 4:7 (AC2) which changed 0 -> 1. So... no idea what
> > commit caused the change.
> > 
> > There are a load of other registers that we need sorting, but this
> > is just a test forray into attempting to solve this.
> 
> Got it, let me see if I can find it then. Do share that list of
> problematic registers when you have it, hopefully this isn't the tip of
> the iceberg...

There unfortunately is an iceberg, but hopefully it isn't big enough to
sink a ship!

Besides ID_MMFR4_EL1, here are the other differences we've identified.
Note that these are Oracle's UEK kernels, so based on stable kernel
branches.

Register		Field		5.4.x	5.15.x
ID_PFR0_EL1		CSV2		0	1
ID_ISAR6_EL1		DP		0	1
ID_PFR2_EL1		SSBS		0	1
			CSV3		0	1
ID_AA64DFR0_EL1		PMSVer		1	0
			DebugVer	8	6
ID_AA64MMFR1_EL1	XNX		0	1
ID_AA64MMFR2_EL1	EVT		0	1
KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2
					0x12	0

I think some of these differences are due to Marc's removal of the
WA2 code in commit 29e8910a566a ("KVM: arm64: Simplify handling of
ARCH_WORKAROUND_2"). The WA2 register for example has changed from
avail/enabled to not_avail.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Russell King (Oracle)]

On Wed, May 01, 2024 at 08:51:15PM +0100, Russell King (Oracle) wrote:
> On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
> > On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> > > On Wed, May 01, 2024 at 05:57:20PM +0000, Oliver Upton wrote:
> > > > Hi Russell,
> > > > 
> > > > On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> > > > > Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> > > > > changed their value on the same Neoverse N1 r3p1 hardware which makes
> > > > > migrating between these kernels on the host problematical.
> > > > 
> > > > It'd be helpful to expand a bit more on how these fields changed, better
> > > > yet if we can blame it back to a commit. I'm guessing the only direction
> > > > of migration you care about is old -> new then?
> > > 
> > > Yes. For MMFR4_EL1, we see 0 with our 5.4 based kernel, and 0x21110
> > > with our 5.15 kernel. I've been looking at tracking down which commit
> > > is responsible but I've come up with nothing that fits.
> > > 
> > > The only change I can see is the FTR definition for MMFR4, but this
> > > always included 4:7 (AC2) which changed 0 -> 1. So... no idea what
> > > commit caused the change.
> > > 
> > > There are a load of other registers that we need sorting, but this
> > > is just a test forray into attempting to solve this.
> > 
> > Got it, let me see if I can find it then. Do share that list of
> > problematic registers when you have it, hopefully this isn't the tip of
> > the iceberg...
> 
> There unfortunately is an iceberg, but hopefully it isn't big enough to
> sink a ship!
> 
> Besides ID_MMFR4_EL1, here are the other differences we've identified.
> Note that these are Oracle's UEK kernels, so based on stable kernel
> branches.
> 
> Register		Field		5.4.x	5.15.x
> ID_PFR0_EL1		CSV2		0	1
> ID_ISAR6_EL1		DP		0	1
> ID_PFR2_EL1		SSBS		0	1
> 			CSV3		0	1
> ID_AA64DFR0_EL1		PMSVer		1	0
> 			DebugVer	8	6
> ID_AA64MMFR1_EL1	XNX		0	1
> ID_AA64MMFR2_EL1	EVT		0	1
> KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2
> 					0x12	0

I'm finding sys_regs.c very unintuitive for working out what we allow
to be written, because it's all coded in negative-logic. By that I mean
the mask values are all ~(what-we-don't-allow) rather than a positive
this-is-what-we-allow. So I've ended up creating a table, looking up
the registers and working out what's read-only and what's read-write.

From that, I can see (for example) that from the ISAR6_EL1 register,
the field names appear in the AA64ISAR0_EL1 and AA64ISAR1_EL1
registers, and all non-res0 fields are writable. It is therefore my
intention to submit a patch doing this:

-       AA32_ID_SANITISED(ID_ISAR6_EL1),
+       AA32_ID_WRITABLE(ID_ISAR6_EL1, ID_ISAR6_EL1_I8MM |
+                                      ID_ISAR6_EL1_BF16 |
+                                      ID_ISAR6_EL1_SPECRES |
+                                      ID_ISAR6_EL1_SB |
+                                      ID_ISAR6_EL1_FHM |
+                                      ID_ISAR6_EL1_DP |
+                                      ID_ISAR6_EL1_JSCVT),

which, like the MMFR4 patch, uses positive logic for what we allow
to be changed, even though this is equivalent to ~ID_ISAR6_EL1_RES0
which tells us nothing without either looking up in the spec, or
looking at the generated sysreg-defs.h to figure it out.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Russell King (Oracle)]

On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
> On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> > Yes, it did strike me as odd, since the description seems to imply that
> > XNX affects EL2, which the VM wouldn't have access to. So I'm not sure
> > why we don't just force it to zero.
> 
> Probably because we failed to catch it in the first place and setting to
> 0 now would be even more UAPI breakage. Meh :-/ I don't see any immediate
> issues with the patch, especially since it is fixing a genuine UAPI
> breakage in KVM.

I think the only two ways around this would be to:

1) teach QEMU about the contents of these registers, with which fields
   in these registers can be ignored when reloading a VMs context.

2) allow userspace to write to the XNX field such that it can be set
   to values seen with previous kernels (thus allowing at least one-
   way migration.)

(1) has the advantage that reloading a VM state on older vs newer
kernels can work in either direction, whereas (2) would only work
for state saved on an older kernel loaded onto a newer kernel.

I've been bitten before with KVM differences between kernel versions
in the past - where the number of registers that userspace sees has
changed despite being on the same hardware. I'm now wondering what
testing goes on to validate this part of the UAPI across different
kernel versions on the same hardware.

Surely, given the impact of these changing value (a failure of VMs),
we should be aware whenever any of these registers are different from
one kernel to another on the same hardware?

It's fine if all one cares about is starting fresh VMs on each host
kernel reboot, but any use case beyond that seems to be fragile.

I did knock up a test program that dumped the list of registers so
that one could trivially diff the output between various kernels.
Maybe I need to extend that to dump the register values themselves,
and then maybe we need to find a way to get some kind of automated
testing setup to highlight differences. (something maybe kernelci
could add?)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Marc Zyngier]

On Thu, 02 May 2024 11:50:10 +0100,
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> On Wed, May 01, 2024 at 08:51:15PM +0100, Russell King (Oracle) wrote:
> > On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
> > > On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> > > > On Wed, May 01, 2024 at 05:57:20PM +0000, Oliver Upton wrote:
> > > > > Hi Russell,
> > > > > 
> > > > > On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> > > > > > Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> > > > > > changed their value on the same Neoverse N1 r3p1 hardware which makes
> > > > > > migrating between these kernels on the host problematical.
> > > > > 
> > > > > It'd be helpful to expand a bit more on how these fields changed, better
> > > > > yet if we can blame it back to a commit. I'm guessing the only direction
> > > > > of migration you care about is old -> new then?
> > > > 
> > > > Yes. For MMFR4_EL1, we see 0 with our 5.4 based kernel, and 0x21110
> > > > with our 5.15 kernel. I've been looking at tracking down which commit
> > > > is responsible but I've come up with nothing that fits.
> > > > 
> > > > The only change I can see is the FTR definition for MMFR4, but this
> > > > always included 4:7 (AC2) which changed 0 -> 1. So... no idea what
> > > > commit caused the change.
> > > > 
> > > > There are a load of other registers that we need sorting, but this
> > > > is just a test forray into attempting to solve this.
> > > 
> > > Got it, let me see if I can find it then. Do share that list of
> > > problematic registers when you have it, hopefully this isn't the tip of
> > > the iceberg...
> > 
> > There unfortunately is an iceberg, but hopefully it isn't big enough to
> > sink a ship!
> > 
> > Besides ID_MMFR4_EL1, here are the other differences we've identified.
> > Note that these are Oracle's UEK kernels, so based on stable kernel
> > branches.
> > 
> > Register		Field		5.4.x	5.15.x
> > ID_PFR0_EL1		CSV2		0	1
> > ID_ISAR6_EL1		DP		0	1
> > ID_PFR2_EL1		SSBS		0	1
> > 			CSV3		0	1
> > ID_AA64DFR0_EL1		PMSVer		1	0
> > 			DebugVer	8	6
> > ID_AA64MMFR1_EL1	XNX		0	1
> > ID_AA64MMFR2_EL1	EVT		0	1
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2
> > 					0x12	0
> 
> I'm finding sys_regs.c very unintuitive for working out what we allow
> to be written, because it's all coded in negative-logic. By that I mean
> the mask values are all ~(what-we-don't-allow) rather than a positive
> this-is-what-we-allow. So I've ended up creating a table, looking up
> the registers and working out what's read-only and what's read-write.

[...]

Using positive or negative logic doesn't really have any impact on the
result. It often is a matter of concisely expressing what is allowed
or not.

There is also the fact that a lot of the KVM code now checks for
"feature downgrade" and enforces it. Construct such as:

	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS))
		kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_TLBIRVAALE1OS|
						HFGITR_EL2_TLBIRVALE1OS	|
						HFGITR_EL2_TLBIRVAAE1OS	|
						HFGITR_EL2_TLBIRVAE1OS	|
						HFGITR_EL2_TLBIVAALE1OS	|
						HFGITR_EL2_TLBIVALE1OS	|
						HFGITR_EL2_TLBIVAAE1OS	|
						HFGITR_EL2_TLBIASIDE1OS	|
						HFGITR_EL2_TLBIVAE1OS	|
						HFGITR_EL2_TLBIVMALLE1OS);

use negative logic by expressing what we don't want to enable.

In the end, consistency matters.

	M.

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

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Oliver Upton]

On Thu, May 02, 2024 at 03:40:38PM +0100, Russell King (Oracle) wrote:
> On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
> > On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> > > Yes, it did strike me as odd, since the description seems to imply that
> > > XNX affects EL2, which the VM wouldn't have access to. So I'm not sure
> > > why we don't just force it to zero.
> > 
> > Probably because we failed to catch it in the first place and setting to
> > 0 now would be even more UAPI breakage. Meh :-/ I don't see any immediate
> > issues with the patch, especially since it is fixing a genuine UAPI
> > breakage in KVM.
> 
> I think the only two ways around this would be to:
> 
> 1) teach QEMU about the contents of these registers, with which fields
>    in these registers can be ignored when reloading a VMs context.
> 
> 2) allow userspace to write to the XNX field such that it can be set
>    to values seen with previous kernels (thus allowing at least one-
>    way migration.)
> 
> (1) has the advantage that reloading a VM state on older vs newer
> kernels can work in either direction, whereas (2) would only work
> for state saved on an older kernel loaded onto a newer kernel.

Yeah, so this is something that has affected my employer as well.

I think (1) should only be expected of VMMs that want rollback safety,
i.e. the ability to migrate state back to an older kernel. Our userspace
initializes vCPUs from a fixed set of feature ID register values that
prevents VMs on new kernels from picking up new CPU features.

It is quite tedious, but necessary as rollback safety is very much a
non-goal of the KVM UAPI.

OTOH, in cases where KVM screws up and breaks UAPI, the kernel needs to
do something special to accept the previously-advertised state even if
it were nonsensical.

For example, there was a bug where KVM advertised an IMP DEF PMU to VMs
even though the only thing KVM virtualizes is PMUv3. We fixed it in
commit f90f9360c3d7 ("KVM: arm64: Rewrite IMPDEF PMU version as NI") by
accepting the old value in the ioctl and changing the field to NI
internally.

I dislike these sort of hacks, but when we're caught between upholding
UAPI and the architecture it seems to be the best option. I wonder if an
approach similar to this would be sufficient to address the SPE change
that you noticed.

> I've been bitten before with KVM differences between kernel versions
> in the past - where the number of registers that userspace sees has
> changed despite being on the same hardware.

This is intended behavior, as VMs are initialized to the maximum feature
set KVM is able to support. Forward-compatibility for the set of exposed
registers is tested, see the get-reg-list selftest.

> I'm now wondering what
> testing goes on to validate this part of the UAPI across different
> kernel versions on the same hardware.

We may've been a bit more relaxed in the past with this, but in recent
history we've been careful about preserving UAPI. On top of that, we now
have some generalized infrastructure for dealing with these things by
way of the 'writable' / mutable ID register work.

Although it isn't precisely what you're looking for, the set_id_regs
selftest ensures we at least accept features that are valid for the
underlying HW platform and explicitly tests downgrades.

> I did knock up a test program that dumped the list of registers so
> that one could trivially diff the output between various kernels.
> Maybe I need to extend that to dump the register values themselves,
> and then maybe we need to find a way to get some kind of automated
> testing setup to highlight differences. (something maybe kernelci
> could add?)

Given the absolutely massive test matrix of implementations and kernel
versions I question our ability to support this. Additionally the only
thing we'd care about upstream is the unsafe removal of a feature.

Nevertheless, a starting point could be to drop some pr_info() into
set_id_regs.c to print the starting value of the ID registers which
could be diffed.

-- 
Thanks,
Oliver

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Russell King (Oracle)]

On Thu, May 02, 2024 at 04:23:10PM +0100, Marc Zyngier wrote:
> On Thu, 02 May 2024 11:50:10 +0100,
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > 
> > On Wed, May 01, 2024 at 08:51:15PM +0100, Russell King (Oracle) wrote:
> > > On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
> > > > On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
> > > > > On Wed, May 01, 2024 at 05:57:20PM +0000, Oliver Upton wrote:
> > > > > > Hi Russell,
> > > > > > 
> > > > > > On Wed, May 01, 2024 at 06:06:51PM +0100, Russell King (Oracle) wrote:
> > > > > > > Between 5.4 and 5.15, the guests view of HPDS, CnP, XNX and AC2
> > > > > > > changed their value on the same Neoverse N1 r3p1 hardware which makes
> > > > > > > migrating between these kernels on the host problematical.
> > > > > > 
> > > > > > It'd be helpful to expand a bit more on how these fields changed, better
> > > > > > yet if we can blame it back to a commit. I'm guessing the only direction
> > > > > > of migration you care about is old -> new then?
> > > > > 
> > > > > Yes. For MMFR4_EL1, we see 0 with our 5.4 based kernel, and 0x21110
> > > > > with our 5.15 kernel. I've been looking at tracking down which commit
> > > > > is responsible but I've come up with nothing that fits.
> > > > > 
> > > > > The only change I can see is the FTR definition for MMFR4, but this
> > > > > always included 4:7 (AC2) which changed 0 -> 1. So... no idea what
> > > > > commit caused the change.
> > > > > 
> > > > > There are a load of other registers that we need sorting, but this
> > > > > is just a test forray into attempting to solve this.
> > > > 
> > > > Got it, let me see if I can find it then. Do share that list of
> > > > problematic registers when you have it, hopefully this isn't the tip of
> > > > the iceberg...
> > > 
> > > There unfortunately is an iceberg, but hopefully it isn't big enough to
> > > sink a ship!
> > > 
> > > Besides ID_MMFR4_EL1, here are the other differences we've identified.
> > > Note that these are Oracle's UEK kernels, so based on stable kernel
> > > branches.
> > > 
> > > Register		Field		5.4.x	5.15.x
> > > ID_PFR0_EL1		CSV2		0	1
> > > ID_ISAR6_EL1		DP		0	1
> > > ID_PFR2_EL1		SSBS		0	1
> > > 			CSV3		0	1
> > > ID_AA64DFR0_EL1		PMSVer		1	0
> > > 			DebugVer	8	6
> > > ID_AA64MMFR1_EL1	XNX		0	1
> > > ID_AA64MMFR2_EL1	EVT		0	1
> > > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2
> > > 					0x12	0
> > 
> > I'm finding sys_regs.c very unintuitive for working out what we allow
> > to be written, because it's all coded in negative-logic. By that I mean
> > the mask values are all ~(what-we-don't-allow) rather than a positive
> > this-is-what-we-allow. So I've ended up creating a table, looking up
> > the registers and working out what's read-only and what's read-write.
> 
> [...]
> 
> Using positive or negative logic doesn't really have any impact on the
> result. It often is a matter of concisely expressing what is allowed
> or not.
> 
> There is also the fact that a lot of the KVM code now checks for
> "feature downgrade" and enforces it. Construct such as:
> 
> 	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS))
> 		kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_TLBIRVAALE1OS|
> 						HFGITR_EL2_TLBIRVALE1OS	|
> 						HFGITR_EL2_TLBIRVAAE1OS	|
> 						HFGITR_EL2_TLBIRVAE1OS	|
> 						HFGITR_EL2_TLBIVAALE1OS	|
> 						HFGITR_EL2_TLBIVALE1OS	|
> 						HFGITR_EL2_TLBIVAAE1OS	|
> 						HFGITR_EL2_TLBIASIDE1OS	|
> 						HFGITR_EL2_TLBIVAE1OS	|
> 						HFGITR_EL2_TLBIVMALLE1OS);
> 
> use negative logic by expressing what we don't want to enable.
> 
> In the end, consistency matters.

Is that a request to change my patch?

I'm unclear whether anyone wants changes to it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Cornelia Huck]

On Thu, May 02 2024, Oliver Upton <oliver.upton@linux.dev> wrote:

> On Thu, May 02, 2024 at 03:40:38PM +0100, Russell King (Oracle) wrote:
>> On Wed, May 01, 2024 at 06:59:17PM +0000, Oliver Upton wrote:
>> > On Wed, May 01, 2024 at 07:08:05PM +0100, Russell King (Oracle) wrote:
>> > > Yes, it did strike me as odd, since the description seems to imply that
>> > > XNX affects EL2, which the VM wouldn't have access to. So I'm not sure
>> > > why we don't just force it to zero.
>> > 
>> > Probably because we failed to catch it in the first place and setting to
>> > 0 now would be even more UAPI breakage. Meh :-/ I don't see any immediate
>> > issues with the patch, especially since it is fixing a genuine UAPI
>> > breakage in KVM.
>> 
>> I think the only two ways around this would be to:
>> 
>> 1) teach QEMU about the contents of these registers, with which fields
>>    in these registers can be ignored when reloading a VMs context.
>> 
>> 2) allow userspace to write to the XNX field such that it can be set
>>    to values seen with previous kernels (thus allowing at least one-
>>    way migration.)
>> 
>> (1) has the advantage that reloading a VM state on older vs newer
>> kernels can work in either direction, whereas (2) would only work
>> for state saved on an older kernel loaded onto a newer kernel.
>
> Yeah, so this is something that has affected my employer as well.
>
> I think (1) should only be expected of VMMs that want rollback safety,
> i.e. the ability to migrate state back to an older kernel. Our userspace
> initializes vCPUs from a fixed set of feature ID register values that
> prevents VMs on new kernels from picking up new CPU features.
>
> It is quite tedious, but necessary as rollback safety is very much a
> non-goal of the KVM UAPI.

Depending on your use case, rollback safety might be quite
important... have we ever stated exactly which guarantees the KVM UAPI
is giving? IOW, can someone implementing a VMM look at a doc and see
"oh, if I want to be able to go backwards, I need to be able to deal
with x, y, and z coming up on the new kernel"?

>
> OTOH, in cases where KVM screws up and breaks UAPI, the kernel needs to
> do something special to accept the previously-advertised state even if
> it were nonsensical.
>
> For example, there was a bug where KVM advertised an IMP DEF PMU to VMs
> even though the only thing KVM virtualizes is PMUv3. We fixed it in
> commit f90f9360c3d7 ("KVM: arm64: Rewrite IMPDEF PMU version as NI") by
> accepting the old value in the ioctl and changing the field to NI
> internally.
>
> I dislike these sort of hacks, but when we're caught between upholding
> UAPI and the architecture it seems to be the best option. I wonder if an
> approach similar to this would be sufficient to address the SPE change
> that you noticed.
>
>> I've been bitten before with KVM differences between kernel versions
>> in the past - where the number of registers that userspace sees has
>> changed despite being on the same hardware.
>
> This is intended behavior, as VMs are initialized to the maximum feature
> set KVM is able to support. Forward-compatibility for the set of exposed
> registers is tested, see the get-reg-list selftest.

I've seen this problem come up as well; if it is clear that this is
something that KVM expects the VMM to handle if needed, that is fine;
should we consider "it's tested in a selftest" as a canonical indicator
of "this is what KVM supports compatibility wise"?

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Oliver Upton]

Hi Cornelia,

On Wed, May 08, 2024 at 02:06:36PM +0200, Cornelia Huck wrote:
> On Thu, May 02 2024, Oliver Upton <oliver.upton@linux.dev> wrote:
> > I think (1) should only be expected of VMMs that want rollback safety,
> > i.e. the ability to migrate state back to an older kernel. Our userspace
> > initializes vCPUs from a fixed set of feature ID register values that
> > prevents VMs on new kernels from picking up new CPU features.
> >
> > It is quite tedious, but necessary as rollback safety is very much a
> > non-goal of the KVM UAPI.
> 
> Depending on your use case, rollback safety might be quite
> important... have we ever stated exactly which guarantees the KVM UAPI
> is giving? IOW, can someone implementing a VMM look at a doc and see
> "oh, if I want to be able to go backwards, I need to be able to deal
> with x, y, and z coming up on the new kernel"?

The behavior of KVM/arm64 has always been that new VMs get the maximum
set of vCPU features supported by KVM / hardware modulo the ones we
require explicit opt-in from userspace (e.g. SVE, vPMU). This behavior
is described in the arm64 vCPU feature documentation [1].

The biggest benefit of this approach is that new vCPU features can be
added without a VMM change, as userspace can just treat the registers in
KVM_GET_REG_LIST as an opaque blob of data that needs to be migrated.

I'm willing to wager that the set of users who want to migrate state
from kernel N -> (N - 1) know the exact CPU definition they want to
expose to the guest, and in that case should be using a static set of
feature register values matching their intent.

Beyond the CPU architecture, KVM presents hypercall features to the VM
which userspace can _opt-out_ of on a per-feature basis using the
feature bitmap registers described in [2]. Like the feature ID
registers, we've preallocated a range of indices to be used for
hypercall bitmaps. So if an unexpected bitmap register appears on a new
kernel, userspace should write 0 to it to prevent new features from
silently creeping in.

Prescribing the exact combination of these UAPIs to achieve a
rollback-safe feature set is beyond the scope of the KVM documentation
and should be determined based on the minimum kernel version that needs
to work.

[1]: https://docs.kernel.org/virt/kvm/arm/vcpu-features.html#the-id-registers
[2]: https://docs.kernel.org/virt/kvm/arm/hypercalls.html

> >> I've been bitten before with KVM differences between kernel versions
> >> in the past - where the number of registers that userspace sees has
> >> changed despite being on the same hardware.
> >
> > This is intended behavior, as VMs are initialized to the maximum feature
> > set KVM is able to support. Forward-compatibility for the set of exposed
> > registers is tested, see the get-reg-list selftest.
> 
> I've seen this problem come up as well; if it is clear that this is
> something that KVM expects the VMM to handle if needed, that is fine;
> should we consider "it's tested in a selftest" as a canonical indicator
> of "this is what KVM supports compatibility wise"?

It certainly is the level of compatibility that gets actively tested :)

The canonical reason for this behavior, though, is that KVM/arm64
defaults to the maximum-possible feature set as discussed above.

-- 
Thanks,
Oliver

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Cornelia Huck]

On Wed, May 08 2024, Oliver Upton <oliver.upton@linux.dev> wrote:

> Hi Cornelia,
>
> On Wed, May 08, 2024 at 02:06:36PM +0200, Cornelia Huck wrote:
>> On Thu, May 02 2024, Oliver Upton <oliver.upton@linux.dev> wrote:
>> > I think (1) should only be expected of VMMs that want rollback safety,
>> > i.e. the ability to migrate state back to an older kernel. Our userspace
>> > initializes vCPUs from a fixed set of feature ID register values that
>> > prevents VMs on new kernels from picking up new CPU features.
>> >
>> > It is quite tedious, but necessary as rollback safety is very much a
>> > non-goal of the KVM UAPI.
>> 
>> Depending on your use case, rollback safety might be quite
>> important... have we ever stated exactly which guarantees the KVM UAPI
>> is giving? IOW, can someone implementing a VMM look at a doc and see
>> "oh, if I want to be able to go backwards, I need to be able to deal
>> with x, y, and z coming up on the new kernel"?
>
> The behavior of KVM/arm64 has always been that new VMs get the maximum
> set of vCPU features supported by KVM / hardware modulo the ones we
> require explicit opt-in from userspace (e.g. SVE, vPMU). This behavior
> is described in the arm64 vCPU feature documentation [1].
>
> The biggest benefit of this approach is that new vCPU features can be
> added without a VMM change, as userspace can just treat the registers in
> KVM_GET_REG_LIST as an opaque blob of data that needs to be migrated.

It also needs to actively turn off everything it does not know how to
handle, if migration between different hardware is supposed to be
supported.

>
> I'm willing to wager that the set of users who want to migrate state
> from kernel N -> (N - 1) know the exact CPU definition they want to
> expose to the guest, and in that case should be using a static set of
> feature register values matching their intent.

I think the trouble starts when we introduce additional ranges of
registers that can be controled via that interface -- old userspace will
be able to figure out that there are more ranges available than what it
is aware of, but will have no idea how to handle those additional ranges
to get into a defined state (what is the actual range, for example?)

>
> Beyond the CPU architecture, KVM presents hypercall features to the VM
> which userspace can _opt-out_ of on a per-feature basis using the
> feature bitmap registers described in [2]. Like the feature ID
> registers, we've preallocated a range of indices to be used for
> hypercall bitmaps. So if an unexpected bitmap register appears on a new
> kernel, userspace should write 0 to it to prevent new features from
> silently creeping in.

Hm, the doc says: "The features for the registers that are untouched,
probably because userspace isn’t aware of them, will be exposed as is to
the guest." Seems to indicate that it is not too hard to get this wrong :)

>
> Prescribing the exact combination of these UAPIs to achieve a
> rollback-safe feature set is beyond the scope of the KVM documentation
> and should be determined based on the minimum kernel version that needs
> to work.
>
> [1]: https://docs.kernel.org/virt/kvm/arm/vcpu-features.html#the-id-registers
> [2]: https://docs.kernel.org/virt/kvm/arm/hypercalls.html
>
>> >> I've been bitten before with KVM differences between kernel versions
>> >> in the past - where the number of registers that userspace sees has
>> >> changed despite being on the same hardware.
>> >
>> > This is intended behavior, as VMs are initialized to the maximum feature
>> > set KVM is able to support. Forward-compatibility for the set of exposed
>> > registers is tested, see the get-reg-list selftest.
>> 
>> I've seen this problem come up as well; if it is clear that this is
>> something that KVM expects the VMM to handle if needed, that is fine;
>> should we consider "it's tested in a selftest" as a canonical indicator
>> of "this is what KVM supports compatibility wise"?
>
> It certainly is the level of compatibility that gets actively tested :)

:)

>
> The canonical reason for this behavior, though, is that KVM/arm64
> defaults to the maximum-possible feature set as discussed above.

/me contemplating "reverse" features, but too tired to think this
through on a Friday afternoon.

Re: [PATCH RFC] KVM: arm64: allow ID_MMFR4_EL1 to be writable

[Oliver Upton]

On Fri, May 10, 2024 at 05:11:09PM +0200, Cornelia Huck wrote:
> On Wed, May 08 2024, Oliver Upton <oliver.upton@linux.dev> wrote:
> > I'm willing to wager that the set of users who want to migrate state
> > from kernel N -> (N - 1) know the exact CPU definition they want to
> > expose to the guest, and in that case should be using a static set of
> > feature register values matching their intent.
> 
> I think the trouble starts when we introduce additional ranges of
> registers that can be controled via that interface -- old userspace will
> be able to figure out that there are more ranges available than what it
> is aware of, but will have no idea how to handle those additional ranges
> to get into a defined state (what is the actual range, for example?)

Even though the UAPI was designed around supporting multiple ranges, I
do not see this being an issue for quite some time. We already fully
describe the Feature ID register range from the architecture.

> >
> > Beyond the CPU architecture, KVM presents hypercall features to the VM
> > which userspace can _opt-out_ of on a per-feature basis using the
> > feature bitmap registers described in [2]. Like the feature ID
> > registers, we've preallocated a range of indices to be used for
> > hypercall bitmaps. So if an unexpected bitmap register appears on a new
> > kernel, userspace should write 0 to it to prevent new features from
> > silently creeping in.
> 
> Hm, the doc says: "The features for the registers that are untouched,
> probably because userspace isn’t aware of them, will be exposed as is to
> the guest." Seems to indicate that it is not too hard to get this wrong :)

Sure, but keep in mind the full range of possible Feature ID registers is
already known to userspace. Many of these register encodings are reserved,
RAZ to allow for future expansion of the architecture [*]. New registers
will come from one of these RAZ encodings.

If userspace wants to assert complete control over the CPU feature set
exposed to the VM it should use a pre-baked value for every register in
the range Op0=3, Op1=0, CRn=0, CRm={1-8}, Op2={0-8}.

[*] DDI0487J.a Table D18-2

> >
> > The canonical reason for this behavior, though, is that KVM/arm64
> > defaults to the maximum-possible feature set as discussed above.
> 
> /me contemplating "reverse" features, but too tired to think this
> through on a Friday afternoon.
> 

Reverse as in a negative / removed feature?

These tend to appear as negative ID register fields, to imply that the
feature set is less than what's provided for with a value of 0x0. KVM
probably couldn't start deprecating features in the default ID register
values but userspace could probe for features it can opt-out of using
the writable mask + trying to write -1 to a field.

DDI0487J.a D19.1.3 covers this.

-- 
Thanks,
Oliver