Allow userspace to downgrade HCX in ID_AA64MMFR1_EL1. Userspace can
only change this value from high to low.

Signed-off-by: Jinqian Yang <yangjinqian1@huawei.com>
---
 arch/arm64/kvm/sys_regs.c                       | 1 -
 tools/testing/selftests/kvm/arm64/set_id_regs.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 82ffb3b3b3cf..f2267db1c44c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -3002,7 +3002,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 				      ~(ID_AA64MMFR0_EL1_RES0 |
 					ID_AA64MMFR0_EL1_ASIDBITS)),
 	ID_WRITABLE(ID_AA64MMFR1_EL1, ~(ID_AA64MMFR1_EL1_RES0 |
-					ID_AA64MMFR1_EL1_HCX |
 					ID_AA64MMFR1_EL1_TWED |
 					ID_AA64MMFR1_EL1_XNX |
 					ID_AA64MMFR1_EL1_VH |
diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c
index d3bf9204409c..a744dee30cd2 100644
--- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
@@ -165,6 +165,7 @@ static const struct reg_ftr_bits ftr_id_aa64mmfr0_el1[] = {
 static const struct reg_ftr_bits ftr_id_aa64mmfr1_el1[] = {
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR1_EL1, TIDCP1, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR1_EL1, AFP, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR1_EL1, HCX, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR1_EL1, ETS, 0),
 	REG_FTR_BITS(FTR_HIGHER_SAFE, ID_AA64MMFR1_EL1, SpecSEI, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR1_EL1, PAN, 0),
-- 
2.33.0

On Tue, Sep 09, 2025 at 11:44:13AM +0800, Jinqian Yang wrote:
> Allow userspace to downgrade HCX in ID_AA64MMFR1_EL1. Userspace can
> only change this value from high to low.
> 
> Signed-off-by: Jinqian Yang <yangjinqian1@huawei.com>

I'm not sure our quality of emulation is that great in this case. We
have no way of trapping the register and it is always stateful. Better
yet, our RESx infrastructure doesn't account for the presence of
FEAT_HCX and we happily merge the contents with the host's HCRX.

We should make a reasonable attempt at upholding the architecture before
allowing userspace to de-feature FEAT_HCX.

Thanks,
Oliver

On Tue, 09 Sep 2025 08:07:15 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Sep 09, 2025 at 11:44:13AM +0800, Jinqian Yang wrote:
> > Allow userspace to downgrade HCX in ID_AA64MMFR1_EL1. Userspace can
> > only change this value from high to low.
> > 
> > Signed-off-by: Jinqian Yang <yangjinqian1@huawei.com>
> 
> I'm not sure our quality of emulation is that great in this case. We
> have no way of trapping the register and it is always stateful. Better
> yet, our RESx infrastructure doesn't account for the presence of
> FEAT_HCX and we happily merge the contents with the host's HCRX.

Yeah, that's not good, and definitely deserves a fix.

> We should make a reasonable attempt at upholding the architecture before
> allowing userspace to de-feature FEAT_HCX.

My concern here is the transitive implications of FEAT_HCX being
disabled: a quick look shows about 20 features that depend on
FEAT_HCX, and we don't really track this. I can probably generate the
dependency graph, but that's not going to be small. Or very useful.

However, we should be able to let FEAT_HCX being disabled without
problem if the downgrading is limited to non-EL2 VMs. Same thing for
FEAT_VHE.

What do you think?

	M.

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

On Tue, Sep 09, 2025 at 11:10:28AM +0100, Marc Zyngier wrote:
> On Tue, 09 Sep 2025 08:07:15 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Sep 09, 2025 at 11:44:13AM +0800, Jinqian Yang wrote:
> > > Allow userspace to downgrade HCX in ID_AA64MMFR1_EL1. Userspace can
> > > only change this value from high to low.
> > > 
> > > Signed-off-by: Jinqian Yang <yangjinqian1@huawei.com>
> > 
> > I'm not sure our quality of emulation is that great in this case. We
> > have no way of trapping the register and it is always stateful. Better
> > yet, our RESx infrastructure doesn't account for the presence of
> > FEAT_HCX and we happily merge the contents with the host's HCRX.
> 
> Yeah, that's not good, and definitely deserves a fix.
> 
> > We should make a reasonable attempt at upholding the architecture before
> > allowing userspace to de-feature FEAT_HCX.
> 
> My concern here is the transitive implications of FEAT_HCX being
> disabled: a quick look shows about 20 features that depend on
> FEAT_HCX, and we don't really track this. I can probably generate the
> dependency graph, but that's not going to be small. Or very useful.
> 
> However, we should be able to let FEAT_HCX being disabled without
> problem if the downgrading is limited to non-EL2 VMs. Same thing for
> FEAT_VHE.
> 
> What do you think?

So I'm a bit worried about making fields sometimes-writable, it creates
a very confusing UAPI behavior. On top of that, our writable masks are
currently static.

What if we treat the entire register as RES0 in this case? It seems to
be consistent with all the underlying bits / features being NI. A
mis-described VM isn't long for this world anyway (e.g. FEAT_SCTLR2 && !FEAT_HCX)
and in that case I'd prefer an approach that keeps the KVM code as
simple as possible.

We do, after all, expect some level of sanity from userspace of feature
dependencies as we do not enforce the dependency graph at the moment.
Hell, this could be useful for someone cross-migrating a nested VM from
a machine w/o FEAT_HCX to one that has it.

Thanks,
Oliver

On 2025/9/10 5:38, Oliver Upton wrote:
> On Tue, Sep 09, 2025 at 11:10:28AM +0100, Marc Zyngier wrote:
>> On Tue, 09 Sep 2025 08:07:15 +0100,
>> Oliver Upton <oliver.upton@linux.dev> wrote:
>>>
>>> On Tue, Sep 09, 2025 at 11:44:13AM +0800, Jinqian Yang wrote:
>>>> Allow userspace to downgrade HCX in ID_AA64MMFR1_EL1. Userspace can
>>>> only change this value from high to low.
>>>>
>>>> Signed-off-by: Jinqian Yang <yangjinqian1@huawei.com>
>>>
>>> I'm not sure our quality of emulation is that great in this case. We
>>> have no way of trapping the register and it is always stateful. Better
>>> yet, our RESx infrastructure doesn't account for the presence of
>>> FEAT_HCX and we happily merge the contents with the host's HCRX.
>>
>> Yeah, that's not good, and definitely deserves a fix.
>>
>>> We should make a reasonable attempt at upholding the architecture before
>>> allowing userspace to de-feature FEAT_HCX.
>>
>> My concern here is the transitive implications of FEAT_HCX being
>> disabled: a quick look shows about 20 features that depend on
>> FEAT_HCX, and we don't really track this. I can probably generate the
>> dependency graph, but that's not going to be small. Or very useful.
>>
>> However, we should be able to let FEAT_HCX being disabled without
>> problem if the downgrading is limited to non-EL2 VMs. Same thing for
>> FEAT_VHE.
>>
>> What do you think?
> 
> So I'm a bit worried about making fields sometimes-writable, it creates
> a very confusing UAPI behavior. On top of that, our writable masks are
> currently static.
> 
> What if we treat the entire register as RES0 in this case? It seems to
> be consistent with all the underlying bits / features being NI. A
> mis-described VM isn't long for this world anyway (e.g. FEAT_SCTLR2 && !FEAT_HCX)
> and in that case I'd prefer an approach that keeps the KVM code as
> simple as possible.
> 
> We do, after all, expect some level of sanity from userspace of feature
> dependencies as we do not enforce the dependency graph at the moment.
> Hell, this could be useful for someone cross-migrating a nested VM from
> a machine w/o FEAT_HCX to one that has it.
> 

Making HCX writable is very useful, as it allows VMs to migrate between 
machines that support and do not support FEAT_HCX. For non-EL2 VMs, we 
have tested that migration works without issues :)

Thanks,
Jinqian