Re: [PATCH v17 04/20] KVM: arm64: Refactor the vcpu_load to allow for VM specific callbacks

[Gavin Shan <gshan@redhat.com>]

Hi Suzuki,

On 9/9/26 2:22 AM, Suzuki K Poulose wrote:
> Based on a work by Marc Zyngier
> 
> To keep the VCPU load/put handling cleaner with the different kinds of VM types,
> we are about to introduce VM specific callbacks to do just the right thing.
> In preparation for that, make some refactoring to add the change easier.
> 
> No functional changes intended.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>   arch/arm64/kvm/arm.c | 46 +++++++++++++++++++++++++++-----------------
>   1 file changed, 28 insertions(+), 18 deletions(-)
> 

Nitpicks below, none of them are introduced by this patch itself. In either way:

Reviewed-by: Gavin Shan <gshan@redhat.com>

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 67f1ff9bc4fbe..51fc651267157 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -683,14 +683,11 @@ static bool kvm_vcpu_should_clear_twe(struct kvm_vcpu *vcpu)
>   	return single_task_running();
>   }
>   
> -void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> +static void vcpu_prepare_mmu(struct kvm_vcpu *vcpu)
>   {
>   	struct kvm_s2_mmu *mmu;
>   	int *last_ran;
>   
> -	if (is_protected_kvm_enabled())
> -		goto nommu;
> -
>   	if (vcpu_has_nv(vcpu))
>   		kvm_vcpu_load_hw_mmu(vcpu);
>   
> @@ -720,10 +717,33 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   		kvm_call_hyp(__kvm_flush_cpu_context, mmu);
>   		*last_ran = vcpu->vcpu_idx;
>   	}
> +}
> +
> +static void vcpu_set_wfx_traps(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_vcpu_should_clear_twe(vcpu))
> +		vcpu->arch.hcr_el2 &= ~HCR_TWE;
> +	else
> +		vcpu->arch.hcr_el2 |= HCR_TWE;
> +
> +	if (kvm_vcpu_should_clear_twi(vcpu))
> +		vcpu->arch.hcr_el2 &= ~HCR_TWI;
> +	else
> +		vcpu->arch.hcr_el2 |= HCR_TWI;
> +}
> +

It seems FIELD_MODIFY() can be applied here if no visible constraints exist:

	FIELD_MODIFY(HCR_TWE, vcpu->arch.hcr_el2, !kvm_vcpu_should_clear_twe(vcpu));
	FIELD_MODIFY(HCR_TWI, vcpu->arch.hcr_el2, !kvm_vcpu_should_clear_twi(vcpu));

> +static void vcpu_load_pvtime(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> +		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> +}
> +
> +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> +{
> +	if (!is_protected_kvm_enabled())
> +		vcpu_prepare_mmu(vcpu);
>  

I think we probably try to depend on kvm->arch.vm_flavor instead
of !is_protected_kvm_enabled().

	struct kvm *kvm = vcpu->kvm;

	if (kvm_vm_is_protected(kvm) || kvm_vm_flavor_eq(kvm, VM_PKVM))
		vcpu_prepare_mmu(vcpu);
  
> -nommu:
>   	vcpu->cpu = cpu;
> -
>   	/*
>   	 * The timer must be loaded before the vgic to correctly set up physical
>   	 * interrupt deactivation in nested state (e.g. timer interrupt).
> @@ -736,19 +756,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   		kvm_vcpu_load_vhe(vcpu);
>   	kvm_arch_vcpu_load_fp(vcpu);
>   	kvm_vcpu_pmu_restore_guest(vcpu);
> -	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> -		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> -
> -	if (kvm_vcpu_should_clear_twe(vcpu))
> -		vcpu->arch.hcr_el2 &= ~HCR_TWE;
> -	else
> -		vcpu->arch.hcr_el2 |= HCR_TWE;
> -
> -	if (kvm_vcpu_should_clear_twi(vcpu))
> -		vcpu->arch.hcr_el2 &= ~HCR_TWI;
> -	else
> -		vcpu->arch.hcr_el2 |= HCR_TWI;
>   
> +	vcpu_load_pvtime(vcpu);
> +	vcpu_set_wfx_traps(vcpu);
>   	vcpu_set_pauth_traps(vcpu);
>   
>   	if (is_protected_kvm_enabled()) {

Thanks,
Gavin