Subject: Re: [PATCH v5 11/69] KVM: arm64: nv: Add nested virt VCPU primitives for vEL2 VCPU state

From: Marc Zyngier <[email-masked]>
Date: Sat, 15 Jan 2022 12:19:16 +0000

On Fri, 14 Jan 2022 17:42:04 +0000,
Alexandru Elisei <[email-masked]> wrote:
> 
> Hi Marc,
> 
> Bunch of bikeshedding regarding names below, which can be safely ignored.
> 
> On Mon, Nov 29, 2021 at 08:00:52PM +0000, Marc Zyngier wrote:
> > From: Christoffer Dall <[email-masked]>
> > 
> > When running a nested hypervisor we commonly have to figure out if
> > the VCPU mode is running in the context of a guest hypervisor or guest
> > guest, or just a normal guest.
> > 
> > Add convenient primitives for this.
> > 
> > Signed-off-by: Christoffer Dall <[email-masked]>
> > Signed-off-by: Marc Zyngier <[email-masked]>
> > ---
> >  arch/arm64/include/asm/kvm_emulate.h | 55 ++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index f4871e47b2d0..f4b079945d0f 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -176,6 +176,61 @@ static __always_inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num,
> >  		vcpu_gp_regs(vcpu)->regs[reg_num] = val;
> >  }
> >  
> > +static inline bool vcpu_mode_el2_ctxt(const struct kvm_cpu_context *ctxt)
> 
> "The Armv8-A architecture defines a set of Exception levels, EL0 to EL3" (ARM
> DDI 0487G.a, page G1-5941).
> 
> "AArch64 state does not support modes. Modes are a concept that is specific to
> AArch32 state." (ARM DDI 0487G.a, page G1-5944).
> 
> Wouldn't it be better to use the same terminology as the architecture?

Probably. I'll see how invasive it is to repaint this. It still
remains that the 'mode' term is used all over the shop (for example,
PSR_MODE_*).

> > +{
> > +	unsigned long cpsr = ctxt->regs.pstate;
> 
> CPSR is an AArch32 register. Why not name the variable pstate?

Because we have *a ton* of existing CPSR references all over the shop
(more than references to pstate, actually), owing to the AArch32
heritage of KVM/arm64. Yes, I can change this locally without any
damage. But repainting the whole of KVM would be fairly pointless
(same with hsr/esr, hfar/far_el2...).

> 
> > +
> > +	switch (cpsr & (PSR_MODE32_BIT | PSR_MODE_MASK)) {
> > +	case PSR_MODE_EL2h:
> > +	case PSR_MODE_EL2t:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static inline bool vcpu_mode_el2(const struct kvm_vcpu *vcpu)
> > +{
> > +	return vcpu_mode_el2_ctxt(&vcpu->arch.ctxt);
> > +}
> > +
> > +static inline bool __vcpu_el2_e2h_is_set(const struct kvm_cpu_context *ctxt)
> > +{
> > +	return ctxt_sys_reg(ctxt, HCR_EL2) & HCR_E2H;
> > +}
> > +
> > +static inline bool vcpu_el2_e2h_is_set(const struct kvm_vcpu *vcpu)
> > +{
> > +	return __vcpu_el2_e2h_is_set(&vcpu->arch.ctxt);
> > +}
> > +
> > +static inline bool __vcpu_el2_tge_is_set(const struct kvm_cpu_context *ctxt)
> > +{
> > +	return ctxt_sys_reg(ctxt, HCR_EL2) & HCR_TGE;
> > +}
> > +
> > +static inline bool vcpu_el2_tge_is_set(const struct kvm_vcpu *vcpu)
> 
> This is confusing. What does the exception level have to do with the
> HCR_EL2.TGE bit being set? Wouldn't vcpu_hcr_tge_is_set() be better?

Sure, why not. Again, I'll see how invasive such a repainting is
across 70 patches.

> 
> > +{
> > +	return __vcpu_el2_tge_is_set(&vcpu->arch.ctxt);
> > +}
> > +
> > +static inline bool __is_hyp_ctxt(const struct kvm_cpu_context *ctxt)
> > +{
> > +	/*
> > +	 * We are in a hypervisor context if the vcpu mode is EL2 or
> > +	 * E2H and TGE bits are set. The latter means we are in the user space
> > +	 * of the VHE kernel. ARMv8.1 ARM describes this as 'InHost'
> 
> So why not call the function vcpu_is_inhost() or something along
> those lines to match the architecture?

Because this is not the architectural 'InHost' primitive, which
returns 'false' if HCR_EL2.E2H==0. The second term of the expression
could be written in terms of an InHost primitive, but that's about it.

> 
> Thanks,
> Alex
> 
> > +	 */
> > +	return vcpu_mode_el2_ctxt(ctxt) ||
> > +		(__vcpu_el2_e2h_is_set(ctxt) && __vcpu_el2_tge_is_set(ctxt)) ||
> > +		WARN_ON(__vcpu_el2_tge_is_set(ctxt));
> > +}
> > +
> > +static inline bool is_hyp_ctxt(const struct kvm_vcpu *vcpu)
> > +{
> > +	return __is_hyp_ctxt(&vcpu->arch.ctxt);
> > +}
> > +
> >  /*
> >   * The layout of SPSR for an AArch32 state is different when observed from an
> >   * AArch64 SPSR_ELx or an AArch32 SPSR_*. This function generates the AArch32

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel