Re: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when using gicv4

* Re: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when using gicv4
       [not found] <F348A9E21A573744858FF7E5D32E29D81FEFA462@DGGEMI529-MBX.china.huawei.com>
@ 2019-03-12 15:48 ` Marc Zyngier
  2019-03-13 11:59   ` Marc Zyngier
  0 siblings, 1 reply; 5+ messages in thread
From: Marc Zyngier @ 2019-03-12 15:48 UTC (permalink / raw)
  To: Tangnianyao (ICT),
	Christoffer Dall, james.morse, julien.thierry, suzuki.poulose,
	catalin.marinas, will.deacon, alex.bennee, mark.rutland,
	andre.przywara, Zhangshaokun, keescook, linux-arm-kernel, kvmarm,
	linux-kernel

Nianyao,

Please do not send patches as HTML. Or any email as HTML. Please make
sure that you only send plain text emails on any mailing list (see point
#6 in Documentation/process/submitting-patches.rst).

On 12/03/2019 12:22, Tangnianyao (ICT) wrote:
> In gicv4, direct vlpi may be forward to PE without using LR or ap_list. If
> 
> ICH_HCR_EL2.En is 0 in guest, direct vlpi cannot be accepted by PE.
> 
> It will take long time for direct vlpi to be forwarded in some cases.
> 
> Direct vlpi has to wait for ICH_HCR_EL2.En=1 where some other interrupts
> 
> using LR need to be forwarded, because in kvm_vgic_flush_hwstate,
> 
> if ap_list is empty, it will return quickly not setting ICH_HCR_EL2.En.
> 
> To fix this, set ICH_HCR_EL2.En to 1 before returning to guest when
> 
> using GICv4.
> 
>  
> 
> Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>
> 
> ---
> 
> arch/arm64/include/asm/kvm_asm.h |  1 +
> 
> virt/kvm/arm/hyp/vgic-v3-sr.c    | 10 ++++++++++
> 
> virt/kvm/arm/vgic/vgic-v4.c      |  8 ++++++++
> 
> 3 files changed, 19 insertions(+)
> 
>  
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h
> b/arch/arm64/include/asm/kvm_asm.h
> 
> index f5b79e9..0581c4d 100644
> 
> --- a/arch/arm64/include/asm/kvm_asm.h
> 
> +++ b/arch/arm64/include/asm/kvm_asm.h
> 
> @@ -79,6 +79,7 @@
> 
> extern void __vgic_v3_init_lrs(void);
> 
>  extern u32 __kvm_get_mdcr_el2(void);
> 
> +extern void __vgic_v3_write_hcr(u32 vmcr);
> 
>  /* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */
> 
> #define
> __hyp_this_cpu_ptr(sym)                                                       
> \
> 
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> 
> index 264d92d..12027af 100644
> 
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> 
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> 
> @@ -434,6 +434,16 @@ void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
> 
>        write_gicreg(vmcr, ICH_VMCR_EL2);
> 
> }
> 
> +u64 __hyp_text __vgic_v3_read_hcr(void)
> 
> +{
> 
> +       return read_gicreg(ICH_HCR_EL2);
> 
> +}
> 
> +
> 
> +void __hyp_text __vgic_v3_write_hcr(u32 vmcr)
> 
> +{
> 
> +       write_gicreg(vmcr, ICH_HCR_EL2);
> 
> +}

This is HYP code...

> 
> +
> 
> #ifdef CONFIG_ARM64
> 
>  static int __hyp_text __vgic_v3_bpr_min(void)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> 
> index 1ed5f22..515171a 100644
> 
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> 
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> 
> @@ -208,6 +208,8 @@ int vgic_v4_sync_hwstate(struct kvm_vcpu *vcpu)
> 
>        if (!vgic_supports_direct_msis(vcpu->kvm))
> 
>                 return 0;
> 
> +       __vgic_v3_write_hcr(vcpu->arch.vgic_cpu.vgic_v3.vgic_hcr &
> ~ICH_HCR_EN);

And you've now crashed your non-VHE system by branching directly to code
that cannot be executed at EL1.

> 
> +
> 
>        return its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, false);
> 
> }
> 
> @@ -220,6 +222,12 @@ int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu)
> 
>                 return 0;
> 
>         /*
> 
> +       * Enable ICH_HCR_EL.En so that guest can accpet and handle direct
> 
> +       * vlpi.
> 
> +       */
> 
> +       __vgic_v3_write_hcr(vcpu->arch.vgic_cpu.vgic_v3.vgic_hcr);
> 
> +
> 
> +       /*
> 
>         * Before making the VPE resident, make sure the redistributor
> 
>         * corresponding to our current CPU expects us here. See the
> 
>         * doc in drivers/irqchip/irq-gic-v4.c to understand how this
> 
> -- 
> 
> 1.9.1
> 
>  
> 
>  
> 

Overall, this looks like the wrong approach. It is not the GICv4 code's
job to do this, but the low-level code (either the load/put code for VHE
or the save/restore code for non-VHE).

It would certainly help if you could describe which context you're in
(VHE, non-VHE). I suppose the former...

Thanks,

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

^ permalink raw reply	[flat|nested] 5+ messages in thread