Re: [PATCH 19/27] KVM: arm64: nv: Add trap forwarding for CNTHCTL_EL2

[Marc Zyngier <maz@kernel.org>]

On 2023-07-25 18:37, Eric Auger wrote:
> Hi Marc,
> 
> On 7/12/23 16:58, Marc Zyngier wrote:
>> Describe the CNTHCTL_EL2 register, and associate it with all the 
>> sysregs
>> it allows to trap.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/emulate-nested.c | 37 
>> ++++++++++++++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/kvm/emulate-nested.c 
>> b/arch/arm64/kvm/emulate-nested.c
>> index 25e4842ac334..c07c0f3361d7 100644
>> --- a/arch/arm64/kvm/emulate-nested.c
>> +++ b/arch/arm64/kvm/emulate-nested.c
>> @@ -98,9 +98,11 @@ enum coarse_grain_trap_id {
>> 
>>  	/*
>>  	 * Anything after this point requires a callback evaluating a
>> -	 * complex trap condition. Hopefully we'll never need this...
>> +	 * complex trap condition. Ugly stuff.
>>  	 */
>>  	__COMPLEX_CONDITIONS__,
>> +	CGT_CNTHCTL_EL1PCTEN = __COMPLEX_CONDITIONS__,
>> +	CGT_CNTHCTL_EL1PTEN,
>>  };
>> 
>>  static const struct trap_bits coarse_trap_bits[] = {
>> @@ -358,9 +360,37 @@ static const enum coarse_grain_trap_id 
>> *coarse_control_combo[] = {
>> 
>>  typedef enum trap_behaviour (*complex_condition_check)(struct 
>> kvm_vcpu *);
>> 
>> +static u64 get_sanitized_cnthctl(struct kvm_vcpu *vcpu)
>> +{
>> +	u64 val = __vcpu_sys_reg(vcpu, CNTHCTL_EL2);
>> +
>> +	if (!vcpu_el2_e2h_is_set(vcpu))
>> +		val = (val & (CNTHCTL_EL1PCEN | CNTHCTL_EL1PCTEN)) << 10;
>> +
>> +	return val;
> don't you want to return only bits 10 & 11 to match the other 
> condition?
> 
> I would add a comment saying that When FEAT_VHE is implemented and
> HCR_EL2.E2H == 1:
> 
> sanitized_cnthctl[11:10] = [EL1PTEN, EL1PCTEN]
> otherwise
> sanitized_cnthctl[11:10] = [EL1PCEN, EL1PCTEN]
> 
>> +}
>> +
>> +static enum trap_behaviour check_cnthctl_el1pcten(struct kvm_vcpu 
>> *vcpu)
>> +{
>> +	if (get_sanitized_cnthctl(vcpu) & (CNTHCTL_EL1PCTEN << 10))
>> +		return BEHAVE_HANDLE_LOCALLY;
>> +
>> +	return BEHAVE_FORWARD_ANY;
>> +}
>> +
>> +static enum trap_behaviour check_cnthctl_el1pten(struct kvm_vcpu 
>> *vcpu)
> or pcen. This is a bit confusing to see EL1PCEN below. But this is due
> to above sanitized CNTHCTL. Worth a comment to me.

I'm adding the following:

/*
  * Warning, maximum confusion ahead.
  *
  * When E2H=0, CNTHCTL_EL2[1:0] are defined as EL1PCEN:EL1PCTEN
  * When E2H=1, CNTHCTL_EL2[11:10] are defined as EL1PTEN:EL1PCTEN
  *
  * Note the single letter difference? Yet, the bits have the same
  * function despite a different layout and a different name.
  *
  * We don't try to reconcile this mess. We just use the E2H=0 bits
  * to generate something that is in the E2H=1 format, and live with
  * it. You're welcome.
  */

Hopefully this will make things clearer. Not completely sure though.

>> +{
>> +	if (get_sanitized_cnthctl(vcpu) & (CNTHCTL_EL1PCEN << 10))
>> +		return BEHAVE_HANDLE_LOCALLY;
>> +
>> +	return BEHAVE_FORWARD_ANY;
>> +}
>> +
>>  #define CCC(id, fn)	[id - __COMPLEX_CONDITIONS__] = fn
>> 
>>  static const complex_condition_check ccc[] = {
>> +	CCC(CGT_CNTHCTL_EL1PCTEN, check_cnthctl_el1pcten),
>> +	CCC(CGT_CNTHCTL_EL1PTEN, check_cnthctl_el1pten),
>>  };
>> 
>>  /*
>> @@ -855,6 +885,11 @@ static const struct encoding_to_trap_config 
>> encoding_to_cgt[] __initdata = {
>>  	SR_TRAP(SYS_TRBPTR_EL1, 	CGT_MDCR_E2TB),
>>  	SR_TRAP(SYS_TRBSR_EL1, 		CGT_MDCR_E2TB),
>>  	SR_TRAP(SYS_TRBTRG_EL1,		CGT_MDCR_E2TB),
>> +	SR_TRAP(SYS_CNTP_TVAL_EL0,	CGT_CNTHCTL_EL1PTEN),
>> +	SR_TRAP(SYS_CNTP_CVAL_EL0,	CGT_CNTHCTL_EL1PTEN),
>> +	SR_TRAP(SYS_CNTP_CTL_EL0,	CGT_CNTHCTL_EL1PTEN),
>> +	SR_TRAP(SYS_CNTPCT_EL0,		CGT_CNTHCTL_EL1PCTEN),
>> +	SR_TRAP(SYS_CNTPCTSS_EL0,	CGT_CNTHCTL_EL1PCTEN),
>>  };
>> 
>>  static DEFINE_XARRAY(sr_forward_xa);
> Otherwise looks good to me
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks!

         M.
-- 
Jazz is not dead. It just smells funny...