[RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Shaoqin Huang]

Hi guys,

I'm trying to enable migration from MtCollins(Ampere Altra, ARMv8.2+) to
AmpereOne(AmpereOne, ARMv8.6+), the migration always fails when migration from
MtCollins to AmpereOne due to some register fields differing between the
two machines.

In this patch series, we try to make more register fields writable like
ID_AA64PFR1_EL1.BT. This is first step towards making the migration possible.
Some other hurdles need to be overcome. This is not sufficient to make the
migration successful from MtCollins to AmpereOne.

Shaoqin Huang (2):
  KVM: arm64: Allow BT field in ID_AA64PFR1_EL1 writable
  KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1

 arch/arm64/kvm/sys_regs.c                         | 2 +-
 tools/testing/selftests/kvm/aarch64/set_id_regs.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.40.1

[RFC PATCH v1 1/2] KVM: arm64: Allow BT field in ID_AA64PFR1_EL1 writable

[Shaoqin Huang]

When migrating from MtCollins to AmpereOne, the BT field value in
ID_AA64PFR1_EL1 register is different and not writable. This causes
the migration to fail.

The BT field means Branch Target Identification mechanism support in
AArch64 state. The value 0 means BT is not implemented, the value 1
means BT is implemented.

On MtCollins(Migration Src), the BT value is 0.
On AmpereOne(Migration Dst), the BT value is 1.

As it defined in the ftr_id_aa64dfr0, the samller value is safe. So if
we make the BT field writable, on the AmpereOne(Migration Dst) the BT
field will be overrided with value 0.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
But there is a question, the ARM DDI mentions from Armv8.5, the only
permitted value is 0b01. Do you guys know if there are any consequence
if the userspace write value 0b0 into this field? Or we should restrict
that at some level, like in VMM or kernel level?
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c9f4f387155f..8e0ea62e14e1 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2292,7 +2292,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 		   ID_AA64PFR0_EL1_GIC |
 		   ID_AA64PFR0_EL1_AdvSIMD |
 		   ID_AA64PFR0_EL1_FP), },
-	ID_SANITISED(ID_AA64PFR1_EL1),
+	ID_WRITABLE(SYS_ID_AA64PFR1_EL1, ID_AA64PFR1_EL1_BT),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
-- 
2.40.1

Re: [RFC PATCH v1 1/2] KVM: arm64: Allow BT field in ID_AA64PFR1_EL1 writable

[Oliver Upton]

On Tue, Jun 11, 2024 at 10:35:51PM -0400, Shaoqin Huang wrote:
> When migrating from MtCollins to AmpereOne, the BT field value in
> ID_AA64PFR1_EL1 register is different and not writable. This causes
> the migration to fail.
> 
> The BT field means Branch Target Identification mechanism support in
> AArch64 state. The value 0 means BT is not implemented, the value 1
> means BT is implemented.
> 
> On MtCollins(Migration Src), the BT value is 0.
> On AmpereOne(Migration Dst), the BT value is 1.
> 
> As it defined in the ftr_id_aa64dfr0, the samller value is safe. So if

typo: smaller

> we make the BT field writable, on the AmpereOne(Migration Dst) the BT
> field will be overrided with value 0.
> 
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
> But there is a question, the ARM DDI mentions from Armv8.5, the only
> permitted value is 0b01. Do you guys know if there are any consequence
> if the userspace write value 0b0 into this field? Or we should restrict
> that at some level, like in VMM or kernel level?

There's no directly visible attribute in the CPU registers to determine
what level of the architecture the implementation supports, and I
don't really want KVM to go about policing this.

The general guidance for ID register fields is that we allow userspace
to select a subset of CPU features supported by KVM / the
implementation, which in this case would include the _NI encoding for
the field. This has been slightly opinionated so far, leaving features
that userspace selects via a separate mechanism (e.g. KVM_ARM_VCPU_INIT)
read only.

Userspace can (and should) come up with its own heuristics for
determining the feature set for the vCPU.

> ---
>  arch/arm64/kvm/sys_regs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c9f4f387155f..8e0ea62e14e1 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2292,7 +2292,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  		   ID_AA64PFR0_EL1_GIC |
>  		   ID_AA64PFR0_EL1_AdvSIMD |
>  		   ID_AA64PFR0_EL1_FP), },
> -	ID_SANITISED(ID_AA64PFR1_EL1),
> +	ID_WRITABLE(SYS_ID_AA64PFR1_EL1, ID_AA64PFR1_EL1_BT),

This doesn't compile. The macro prefixes "SYS_" to the register name.

-- 
Thanks,
Oliver

[RFC PATCH v1 2/2] KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1

[Shaoqin Huang]

Add test for the BT field in the ID_AA64PFR1_EL1 register.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 tools/testing/selftests/kvm/aarch64/set_id_regs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
index 16e2338686c1..5381b8ec5562 100644
--- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -133,6 +133,11 @@ static const struct reg_ftr_bits ftr_id_aa64pfr0_el1[] = {
 	REG_FTR_END,
 };
 
+static const struct reg_ftr_bits ftr_id_aa64pfr1_el1[] = {
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, BT, 0),
+	REG_FTR_END,
+};
+
 static const struct reg_ftr_bits ftr_id_aa64mmfr0_el1[] = {
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, ECV, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, EXS, 0),
@@ -199,6 +204,7 @@ static struct test_feature_reg test_regs[] = {
 	TEST_REG(SYS_ID_AA64ISAR1_EL1, ftr_id_aa64isar1_el1),
 	TEST_REG(SYS_ID_AA64ISAR2_EL1, ftr_id_aa64isar2_el1),
 	TEST_REG(SYS_ID_AA64PFR0_EL1, ftr_id_aa64pfr0_el1),
+	TEST_REG(SYS_ID_AA64PFR1_EL1, ftr_id_aa64pfr1_el1),
 	TEST_REG(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0_el1),
 	TEST_REG(SYS_ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1_el1),
 	TEST_REG(SYS_ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2_el1),
-- 
2.40.1

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Oliver Upton]

Hi Shaoqin,

On Tue, Jun 11, 2024 at 10:35:50PM -0400, Shaoqin Huang wrote:
> Hi guys,
> 
> I'm trying to enable migration from MtCollins(Ampere Altra, ARMv8.2+) to
> AmpereOne(AmpereOne, ARMv8.6+), the migration always fails when migration from
> MtCollins to AmpereOne due to some register fields differing between the
> two machines.
> 
> In this patch series, we try to make more register fields writable like
> ID_AA64PFR1_EL1.BT. This is first step towards making the migration possible.
> Some other hurdles need to be overcome. This is not sufficient to make the
> migration successful from MtCollins to AmpereOne.

It isn't possible to transparently migrate between these systems. The
former has a cntfrq of 25MHz, and the latter has a cntfrq of 1GHz. There
isn't a mechanism for scaling the counter frequency, and I have zero
appetite for a paravirt interface.

On top of that, erratum AC03_CPU_38 seems to make a migration from
Neoverse-N1 to AmpereOne quite perilous, unless you hide FEAT_HAFDBS on
the source.

These issues are separate, though, from any possible changes to the
writability of ID_AA64PFR1_EL1, which still may be useful to userspace.

-- 
Thanks,
Oliver

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Shaoqin Huang]

Hi Oliver,

On 6/12/24 13:30, Oliver Upton wrote:
> Hi Shaoqin,
> 
> On Tue, Jun 11, 2024 at 10:35:50PM -0400, Shaoqin Huang wrote:
>> Hi guys,
>>
>> I'm trying to enable migration from MtCollins(Ampere Altra, ARMv8.2+) to
>> AmpereOne(AmpereOne, ARMv8.6+), the migration always fails when migration from
>> MtCollins to AmpereOne due to some register fields differing between the
>> two machines.
>>
>> In this patch series, we try to make more register fields writable like
>> ID_AA64PFR1_EL1.BT. This is first step towards making the migration possible.
>> Some other hurdles need to be overcome. This is not sufficient to make the
>> migration successful from MtCollins to AmpereOne.
> 
> It isn't possible to transparently migrate between these systems. The
> former has a cntfrq of 25MHz, and the latter has a cntfrq of 1GHz. There
> isn't a mechanism for scaling the counter frequency, and I have zero
> appetite for a paravirt interface.

Thanks for letting me know the cntfrq will block the migration between 
the two machine. And we don't have the solution for it, which means it's 
impossible to migrate between these systems which have different cntfrq.

> 
> On top of that, erratum AC03_CPU_38 seems to make a migration from
> Neoverse-N1 to AmpereOne quite perilous, unless you hide FEAT_HAFDBS on
> the source.
> 
> These issues are separate, though, from any possible changes to the
> writability of ID_AA64PFR1_EL1, which still may be useful to userspace.
> 
I think I can still making the ID_AA64PFR1_EL1 writable to enable 
migration on some other machines.

Thanks,
Shaoqin

-- 
Shaoqin

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Marc Zyngier]

On Wed, 12 Jun 2024 06:30:51 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Shaoqin,
> 
> On Tue, Jun 11, 2024 at 10:35:50PM -0400, Shaoqin Huang wrote:
> > Hi guys,
> > 
> > I'm trying to enable migration from MtCollins(Ampere Altra, ARMv8.2+) to
> > AmpereOne(AmpereOne, ARMv8.6+), the migration always fails when migration from
> > MtCollins to AmpereOne due to some register fields differing between the
> > two machines.
> > 
> > In this patch series, we try to make more register fields writable like
> > ID_AA64PFR1_EL1.BT. This is first step towards making the migration possible.
> > Some other hurdles need to be overcome. This is not sufficient to make the
> > migration successful from MtCollins to AmpereOne.
> 
> It isn't possible to transparently migrate between these systems. The
> former has a cntfrq of 25MHz, and the latter has a cntfrq of 1GHz. There
> isn't a mechanism for scaling the counter frequency, and I have zero
> appetite for a paravirt interface.

Note that there *is* an architectural workaround in the form of
FEAT_CNTSC. But of course:

- it is optional (and likely not implemented)
- it is global (hence affecting all SW running on the machine)
- it invalidates the requirements of ARMv8.6 (who cares?)
- KVM has nothing to do with it (yay!)

So if the two systems (from the same manufacturer) were ever designed
to allow migration between the two, they would have at least baked
some of that in.

As for the paravirt interface, I agree that this is a non-starter
(been there, done that, dumped it in the bin).

The patch itself is interesting and may be of use once it has been put
to a compiler and not just dumped on the list without any testing.

	M.

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

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Shaoqin Huang]

Hi Marc,

On 6/12/24 18:07, Marc Zyngier wrote:
> On Wed, 12 Jun 2024 06:30:51 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
>>
>> Hi Shaoqin,
>>
>> On Tue, Jun 11, 2024 at 10:35:50PM -0400, Shaoqin Huang wrote:
>>> Hi guys,
>>>
>>> I'm trying to enable migration from MtCollins(Ampere Altra, ARMv8.2+) to
>>> AmpereOne(AmpereOne, ARMv8.6+), the migration always fails when migration from
>>> MtCollins to AmpereOne due to some register fields differing between the
>>> two machines.
>>>
>>> In this patch series, we try to make more register fields writable like
>>> ID_AA64PFR1_EL1.BT. This is first step towards making the migration possible.
>>> Some other hurdles need to be overcome. This is not sufficient to make the
>>> migration successful from MtCollins to AmpereOne.
>>
>> It isn't possible to transparently migrate between these systems. The
>> former has a cntfrq of 25MHz, and the latter has a cntfrq of 1GHz. There
>> isn't a mechanism for scaling the counter frequency, and I have zero
>> appetite for a paravirt interface.
> 
> Note that there *is* an architectural workaround in the form of
> FEAT_CNTSC. But of course:
> 
> - it is optional (and likely not implemented)
> - it is global (hence affecting all SW running on the machine)
> - it invalidates the requirements of ARMv8.6 (who cares?)
> - KVM has nothing to do with it (yay!)
> 
> So if the two systems (from the same manufacturer) were ever designed
> to allow migration between the two, they would have at least baked
> some of that in.
> 
> As for the paravirt interface, I agree that this is a non-starter
> (been there, done that, dumped it in the bin).
> 
> The patch itself is interesting and may be of use once it has been put
> to a compiler and not just dumped on the list without any testing.
> 
> 	M.

Thanks for putting your comments here.

If we don't care about the FEAT_CNTSC right now. Could I fix the compile 
issue and respin this again without the background of enabling migration 
between MtCollins and AmpereOne, and just keep the information of the 
different BT field between different machine?

Thanks,
Shaoqin

> 

-- 
Shaoqin

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Oliver Upton]

On Thu, Jun 13, 2024 at 04:31:45PM +0800, Shaoqin Huang wrote:
> Hi Marc,
> 
> On 6/12/24 18:07, Marc Zyngier wrote:
> > On Wed, 12 Jun 2024 06:30:51 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > Hi Shaoqin,
> > > 
> > > On Tue, Jun 11, 2024 at 10:35:50PM -0400, Shaoqin Huang wrote:
> > > > Hi guys,
> > > > 
> > > > I'm trying to enable migration from MtCollins(Ampere Altra, ARMv8.2+) to
> > > > AmpereOne(AmpereOne, ARMv8.6+), the migration always fails when migration from
> > > > MtCollins to AmpereOne due to some register fields differing between the
> > > > two machines.
> > > > 
> > > > In this patch series, we try to make more register fields writable like
> > > > ID_AA64PFR1_EL1.BT. This is first step towards making the migration possible.
> > > > Some other hurdles need to be overcome. This is not sufficient to make the
> > > > migration successful from MtCollins to AmpereOne.
> > > 
> > > It isn't possible to transparently migrate between these systems. The
> > > former has a cntfrq of 25MHz, and the latter has a cntfrq of 1GHz. There
> > > isn't a mechanism for scaling the counter frequency, and I have zero
> > > appetite for a paravirt interface.
> > 
> > Note that there *is* an architectural workaround in the form of
> > FEAT_CNTSC. But of course:

Heh, I should've further specified a per-CPU mechanism :)

> > - it is optional (and likely not implemented)
> > - it is global (hence affecting all SW running on the machine)
> > - it invalidates the requirements of ARMv8.6 (who cares?)
> > - KVM has nothing to do with it (yay!)
> > 
> > So if the two systems (from the same manufacturer) were ever designed
> > to allow migration between the two, they would have at least baked
> > some of that in.
> > 
> > As for the paravirt interface, I agree that this is a non-starter
> > (been there, done that, dumped it in the bin).
> > 
> > The patch itself is interesting and may be of use once it has been put
> > to a compiler and not just dumped on the list without any testing.
> > 
> > 	M.
> 
> Thanks for putting your comments here.
> 
> If we don't care about the FEAT_CNTSC right now. Could I fix the compile
> issue and respin this again without the background of enabling migration
> between MtCollins and AmpereOne, and just keep the information of the
> different BT field between different machine?

I don't think cross-platform migration is relevant for the KVM UAPI
you're trying to augment. We want to give userspace the ability to control
the visible feature set for a VM, which *could* be used by userspace in such
a way to transparently migrate VMs.

So if you could focus the changelog purely as an improvement upon the
existing controls we expose to the VMM then the patch is more obviously
justified.

Please do respin.

-- 
Thanks,
Oliver

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Marc Zyngier]

On Thu, 13 Jun 2024 09:31:45 +0100,
Shaoqin Huang <shahuang@redhat.com> wrote:
> 
> If we don't care about the FEAT_CNTSC right now. Could I fix the
> compile issue and respin this again without the background of enabling
> migration between MtCollins and AmpereOne, and just keep the
> information of the different BT field between different machine?

As I said, I think this patch is valuable. But maybe you should
consider tackling the full register, rather than only addressing a
single field.

Thanks,

	M.

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

Re: [RFC PATCH v1 0/2] KVM: arm64: Making BT Field in ID_AA64PFR1_EL1 writable

[Shaoqin Huang]

Hi Marc,

On 6/13/24 16:42, Marc Zyngier wrote:
> On Thu, 13 Jun 2024 09:31:45 +0100,
> Shaoqin Huang <shahuang@redhat.com> wrote:
>>
>> If we don't care about the FEAT_CNTSC right now. Could I fix the
>> compile issue and respin this again without the background of enabling
>> migration between MtCollins and AmpereOne, and just keep the
>> information of the different BT field between different machine?
> 
> As I said, I think this patch is valuable. But maybe you should
> consider tackling the full register, rather than only addressing a
> single field.

Yes, it would be better to tackling the full register. I will put more 
time on other fields in the register and try to making more field to be 
writable. But currently I just respin the series with deleting the 
machine specific information and fixing the compilation issue.

Thanks,
Shaoqin

> 
> Thanks,
> 
> 	M.
> 

-- 
Shaoqin