Hi Marc, > -----Original Message----- > From: kvmarm-bounces@xxxxxxxxxxxxxxxxxxxxx > [mailto:kvmarm-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Marc Zyngier > Sent: 13 March 2019 11:59 > To: Tangnianyao (ICT) <tangnianyao@xxxxxxxxxx>; Christoffer Dall > <christoffer.dall@xxxxxxx>; james.morse@xxxxxxx; julien.thierry@xxxxxxx; > suzuki.poulose@xxxxxxx; catalin.marinas@xxxxxxx; will.deacon@xxxxxxx; > alex.bennee@xxxxxxxxxx; mark.rutland@xxxxxxx; andre.przywara@xxxxxxx; > Zhangshaokun <zhangshaokun@xxxxxxxxxxxxx>; keescook@xxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when > using gicv4 > > On 12/03/2019 15:48, Marc Zyngier wrote: > > 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@xxxxxxxxxx> > >> > >> --- > >> > >> 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... > > Can you please give the following patch a go? I can't test it, but hopefully > you can. Thanks for your quick response. I just did a quick test on one of our platforms with VHE+GICv4 and it seems to fix the performance issue we were seeing when GICv4 is enabled. Test setup: Host connected to a PC over a 10G port. Launch Guest with an assigned vf dev. Run iperf from Guest, 5.0 kernel: [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec 1.30 GBytes 1.12 Gbits/sec +Patch: [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec 10.9 GBytes 9.39 Gbits/sec Cheers, Shameer > Thanks, > > M. > > diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c > index 9652c453480f..3c3f7cda95c7 100644 > --- a/virt/kvm/arm/hyp/vgic-v3-sr.c > +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c > @@ -222,7 +222,7 @@ void __hyp_text __vgic_v3_save_state(struct > kvm_vcpu *vcpu) > } > } > > - if (used_lrs) { > + if (used_lrs || cpu_if->its_vpe.its_vm) { > int i; > u32 elrsr; > > @@ -247,7 +247,7 @@ void __hyp_text __vgic_v3_restore_state(struct > kvm_vcpu *vcpu) > u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs; > int i; > > - if (used_lrs) { > + if (used_lrs || cpu_if->its_vpe.its_vm) { > write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2); > > for (i = 0; i < used_lrs; i++) > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c > index abd9c7352677..3af69f2a3866 100644 > --- a/virt/kvm/arm/vgic/vgic.c > +++ b/virt/kvm/arm/vgic/vgic.c > @@ -867,15 +867,21 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu > *vcpu) > * either observe the new interrupt before or after doing this check, > * and introducing additional synchronization mechanism doesn't change > * this. > + * > + * Note that we still need to go through the whole thing if anything > + * can be directly injected (GICv4). > */ > - if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head)) > + if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head) && > + !vgic_supports_direct_msis(vcpu->kvm)) > return; > > DEBUG_SPINLOCK_BUG_ON(!irqs_disabled()); > > - raw_spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock); > - vgic_flush_lr_state(vcpu); > - raw_spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock); > + if (!list_empty(&vcpu->arch.vgic_cpu.ap_list_head)) { > + raw_spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock); > + vgic_flush_lr_state(vcpu); > + raw_spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock); > + } > > if (can_access_vgic_from_kernel()) > vgic_restore_state(vcpu); > > > -- > Jazz is not dead. It just smells funny... > _______________________________________________ > kvmarm mailing list > kvmarm@xxxxxxxxxxxxxxxxxxxxx > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
