RE: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when using gicv4
Hi, Marc, Shameer > -Original Message- > From: Marc Zyngier [mailto:marc.zyng...@arm.com] > Sent: Thursday, March 14, 2019 12:31 AM > To: Shameerali Kolothum Thodi ; > Tangnianyao (ICT) ; Christoffer Dall > ; james.mo...@arm.com; julien.thie...@arm.com; > suzuki.poul...@arm.com; catalin.mari...@arm.com; will.dea...@arm.com; > alex.ben...@linaro.org; mark.rutl...@arm.com; andre.przyw...@arm.com; > Zhangshaokun ; keesc...@chromium.org; > linux-arm-ker...@lists.infradead.org; kvmarm@lists.cs.columbia.edu; > linux-ker...@vger.kernel.org > Cc: Linuxarm > Subject: Re: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when > using gicv4 > > On 13/03/2019 15:59, Shameerali Kolothum Thodi wrote: > > Hi Shameer, > > >> 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 > > Ah, that looks much better! I'll try to write it properly, as I think what we > have > today is slightly bizarre (we write ICH_HCR_EL2 from too many paths, and I > need to remember how the whole thing works). > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... Test setup: VHE enable. Connected to an intel 8180 server over 10G port. Launch guest with an assigned vf dev. Intel server as server. Arm guest vm as client. Run netperf in guest, package length 512byte: 5.0.0-rc3 kernel: without patch: 2600 Mbits/s with patch: 5800 Mbits/s Thanks, -Nianyao Tang ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when using gicv4
On 13/03/2019 15:59, Shameerali Kolothum Thodi wrote: Hi Shameer, >> 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 Ah, that looks much better! I'll try to write it properly, as I think what we have today is slightly bizarre (we write ICH_HCR_EL2 from too many paths, and I need to remember how the whole thing works). Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
RE: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when using gicv4
Hi Marc,
> -Original Message-
> From: kvmarm-boun...@lists.cs.columbia.edu
> [mailto:kvmarm-boun...@lists.cs.columbia.edu] On Behalf Of Marc Zyngier
> Sent: 13 March 2019 11:59
> To: Tangnianyao (ICT) ; Christoffer Dall
> ; james.mo...@arm.com; julien.thie...@arm.com;
> suzuki.poul...@arm.com; catalin.mari...@arm.com; will.dea...@arm.com;
> alex.ben...@linaro.org; mark.rutl...@arm.com; andre.przyw...@arm.com;
> Zhangshaokun ; keesc...@chromium.org;
> linux-arm-ker...@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> linux-ker...@vger.kernel.org
> 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
> >>
> >> ---
> >>
> >> 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(>arch.vgic_cpu.vgi
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
>>
>> ---
>>
>> 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(>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,
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
Re: [PATCH] KVM: arm/arm64: Set ICH_HCR_EN in guest anyway when using gicv4
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
>
> ---
>
> 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(>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...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
