| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d97bd073-8775-f0cf-637a-ea1b04afbd8f@arm.com>
Date: Wed, 13 Mar 2019 11:59:21 +0000
From: Marc Zyngier <marc.zyngier@....com>
To: "Tangnianyao (ICT)" <tangnianyao@...wei.com>,
Christoffer Dall <christoffer.dall@....com>,
"james.morse@....com" <james.morse@....com>,
"julien.thierry@....com" <julien.thierry@....com>,
"suzuki.poulose@....com" <suzuki.poulose@....com>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will.deacon@....com" <will.deacon@....com>,
"alex.bennee@...aro.org" <alex.bennee@...aro.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"andre.przywara@....com" <andre.przywara@....com>,
Zhangshaokun <zhangshaokun@...ilicon.com>,
"keescook@...omium.org" <keescook@...omium.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.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 <tangnianyao@...wei.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...
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 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...
Powered by blists - more mailing lists