Re: [PATCH v3 04/17] KVM: arm64: Describe RES0/RES1 bits of MDCR_EL2

[Marc Zyngier <maz@kernel.org>]

On Mon, 07 Oct 2024 18:45:46 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Add support for sanitising MDCR_EL2 and describe the RES0/RES1 bits
> according to the feature set exposed to the VM.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 +-
>  arch/arm64/kvm/nested.c           | 35 +++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7764ca71ac6e..98ae79f7e0bc 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -464,7 +464,6 @@ enum vcpu_sysreg {
>  	/* EL2 registers */
>  	SCTLR_EL2,	/* System Control Register (EL2) */
>  	ACTLR_EL2,	/* Auxiliary Control Register (EL2) */
> -	MDCR_EL2,	/* Monitor Debug Configuration Register (EL2) */
>  	CPTR_EL2,	/* Architectural Feature Trap Register (EL2) */
>  	HACR_EL2,	/* Hypervisor Auxiliary Control Register */
>  	ZCR_EL2,	/* SVE Control Register (EL2) */
> @@ -492,6 +491,7 @@ enum vcpu_sysreg {
>  
>  	/* Anything from this can be RES0/RES1 sanitised */
>  	MARKER(__SANITISED_REG_START__),
> +	MDCR_EL2,	/* Monitor Debug Configuration Register (EL2) */
>  
>  	/* Any VNCR-capable reg goes after this point */
>  	MARKER(__VNCR_START__),
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index b20b3bfb9cae..33d85db860f3 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1186,5 +1186,40 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
>  		res0 |= SCTLR_EL1_EPAN;
>  	set_sysreg_masks(kvm, SCTLR_EL1, res0, res1);
>  
> +	/* MDCR_EL2 */
> +	res0 = MDCR_EL2_RES0;
> +	res1 = MDCR_EL2_RES1;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP))
> +		res0 |= (MDCR_EL2_HPMN | MDCR_EL2_TPMCR |
> +			 MDCR_EL2_TPM | MDCR_EL2_HPME);
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, IMP))
> +		res0 |= MDCR_EL2_E2PB | MDCR_EL2_TPMS;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, SPMU, IMP))
> +		res0 |= MDCR_EL2_EnSPM;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P1))
> +		res0 |= MDCR_EL2_HPMD;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, TraceFilt, IMP))
> +		res0 |= MDCR_EL2_TTRF;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P5))
> +		res0 |= MDCR_EL2_HCCD | MDCR_EL2_HLP;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, TraceBuffer, IMP))
> +		res0 |= MDCR_EL2_E2TB;
> +	if (!kvm_has_feat(kvm, ID_AA64MMFR0_EL1, FGT, IMP))
> +		res0 |= MDCR_EL2_TDCC;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, MTPMU, IMP) &&
> +	    !kvm_has_feat(kvm, ID_AA64PFR0_EL1, EL3, IMP))
> +		res0 |= MDCR_EL2_MTPME;

This doesn't seem to match the spec:

<quote>
When FEAT_MTPMU is implemented and EL3 is not implemented:
[...]
Otherwise:
Reserved, RES0.
</quote>

I would have expected something like:

	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, MTPMU, IMP) ||
	    kvm_has_feat(kvm, ID_AA64PFR0_EL1, EL3, IMP))
		res0 |= MDCR_EL2_MTPME;

making it forever RES0, since NV always exposes an EL3 so that SMC
appears as a valid instruction, even in situations where EL3 isn't
implemented.

> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P7))
> +		res0 |= MDCR_EL2_HPMFZO;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSS, IMP))
> +		res0 |= MDCR_EL2_PMSSE;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P2))
> +		res0 |= MDCR_EL2_HPMFZS;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR1_EL1, EBEP, IMP))
> +		res0 |= MDCR_EL2_PMEE;
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
> +		res0 |= MDCR_EL2_EBWE;
> +	set_sysreg_masks(kvm, MDCR_EL2, res0, res1);
> +
>  	return 0;
>  }

Thanks,

	M.

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