Re: [PATCH RFCv2 4/9] kvm/arm64: Detach ESR operator from vCPU struct

[Gavin Shan]

Hi Mark,

On 5/26/20 8:51 PM, Mark Rutland wrote:
> On Fri, May 08, 2020 at 01:29:14PM +1000, Gavin Shan wrote:
> > There are a set of inline functions defined in kvm_emulate.h. Those
> > functions reads ESR from vCPU fault information struct and then operate
> > on it. So it's tied with vCPU fault information and vCPU struct. It
> > limits their usage scope.
> >
> > This detaches these functions from the vCPU struct. With this, the
> > caller has flexibility on where the ESR is read. It shouldn't cause
> > any functional changes.
> >
> > Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
> > ---
> >   arch/arm64/include/asm/kvm_emulate.h     | 83 +++++++++++-------------
> >   arch/arm64/kvm/handle_exit.c             | 20 ++++--
> >   arch/arm64/kvm/hyp/switch.c              | 24 ++++---
> >   arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c |  7 +-
> >   arch/arm64/kvm/inject_fault.c            |  4 +-
> >   arch/arm64/kvm/sys_regs.c                | 12 ++--
> >   virt/kvm/arm/arm.c                       |  4 +-
> >   virt/kvm/arm/hyp/aarch32.c               |  2 +-
> >   virt/kvm/arm/hyp/vgic-v3-sr.c            |  5 +-
> >   virt/kvm/arm/mmio.c                      | 27 ++++----
> >   virt/kvm/arm/mmu.c                       | 22 ++++---
> >   11 files changed, 112 insertions(+), 98 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index bd1a69e7c104..2873bf6dc85e 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -270,10 +270,8 @@ static __always_inline u32 kvm_vcpu_get_esr(const struct kvm_vcpu *vcpu)
> >   	return vcpu->arch.fault.esr_el2;
> >   }
> >   
> > -static __always_inline int kvm_vcpu_get_condition(const struct kvm_vcpu *vcpu)
> > +static __always_inline int kvm_vcpu_get_condition(u32 esr)
>
> Given the `vcpu` argument has been removed, it's odd to keep `vcpu` in the
> name, rather than `esr`.
>
> e.g. this would make more sense as something like esr_get_condition().
>
> ... and if we did something like that, we could move most of the
> extraction functions into <asm/esr.h>, and share them with non-KVM code.
>
> Otherwise, do you need to extract all of these for your use-case, or do
> you only need a few of the helpers? If you only need a few, it might be
> better to only factor those out for now, and keep the existing API in
> place with wrappers, e.g. have:
>
> | esr_get_condition(u32 esr) {
> | 	...
> | }
> |
> | kvm_vcpu_get_condition(const struct kvm_vcpu *vcpu)
> | {
> | 	return esr_get_condition(kvm_vcpu_get_esr(vcpu));
> | }

Sure, I'll follow approach#1, to move these helper functions to asm/esr.h
and with "vcpu" dropped in their names. I don't think it makes sense to
maintain two sets of helper functions for the simple logic. So the helper
function will be called where they should be, as below:

   esr_get_condition(u32 esr) { ... }
   
   bool __hyp_text kvm_condition_valid32(const struct kvm_vcpu *vcpu)
   {
        int cond = esr_get_condition(kvm_vcpu_get_esr(vcpu));
        :
   }

Thanks,
Gavin