* [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
@ 2021-01-08 17:12 ` Marc Zyngier
0 siblings, 0 replies; 16+ messages in thread
From: Marc Zyngier @ 2021-01-08 17:12 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm
Cc: Suzuki K Poulose, Shameerali Kolothum Thodi, James Morse,
kernel-team, Ard Biesheuvel, Julien Thierry
It looks like we have broken firmware out there that wrongly advertises
a GICv2 compatibility interface, despite the CPUs not being able to deal
with it.
To work around this, check that the CPU initialising KVM is actually able
to switch to MMIO instead of system registers, and use that as a
precondition to enable GICv2 compatibility in KVM.
Note that the detection happens on a single CPU. If the firmware is
lying *and* that the CPUs are asymetric, all hope is lost anyway.
Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/hyp/vgic-v3-sr.c | 34 +++++++++++++++++++++++++++++++--
arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 005daa0c9dd7..d504499ab917 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
/*
* Return the GIC CPU configuration:
* - [31:0] ICH_VTR_EL2
- * - [63:32] RES0
+ * - [62:32] RES0
+ * - [63] MMIO (GICv2) capable
*/
u64 __vgic_v3_get_gic_config(void)
{
- return read_gicreg(ICH_VTR_EL2);
+ u64 sre = read_gicreg(ICC_SRE_EL1);
+ unsigned long flags = 0;
+ bool v2_capable;
+
+ /*
+ * To check whether we have a MMIO-based (GICv2 compatible)
+ * CPU interface, we need to disable the system register
+ * view. To do that safely, we have to prevent any interrupt
+ * from firing (which would be deadly).
+ *
+ * Note that this only makes sense on VHE, as interrupts are
+ * already masked for nVHE as part of the exception entry to
+ * EL2.
+ */
+ if (has_vhe())
+ flags = local_daif_save();
+
+ write_gicreg(0, ICC_SRE_EL1);
+ isb();
+
+ v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
+
+ write_gicreg(sre, ICC_SRE_EL1);
+ isb();
+
+ if (has_vhe())
+ local_daif_restore(flags);
+
+ return (read_gicreg(ICH_VTR_EL2) |
+ v2_capable ? (1ULL << 63) : 0);
}
u64 __vgic_v3_read_vmcr(void)
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 8e7bf3151057..67b27b47312b 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -584,8 +584,10 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
int vgic_v3_probe(const struct gic_kvm_info *info)
{
u64 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_gic_config);
+ bool has_v2;
int ret;
+ has_v2 = ich_vtr_el2 >> 63;
ich_vtr_el2 = (u32)ich_vtr_el2;
/*
@@ -605,13 +607,15 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
gicv4_enable ? "en" : "dis");
}
+ kvm_vgic_global_state.vcpu_base = 0;
+
if (!info->vcpu.start) {
kvm_info("GICv3: no GICV resource entry\n");
- kvm_vgic_global_state.vcpu_base = 0;
+ } else if (!has_v2) {
+ pr_warn("CPU interface incapable of MMIO access\n");
} else if (!PAGE_ALIGNED(info->vcpu.start)) {
pr_warn("GICV physical address 0x%llx not page aligned\n",
(unsigned long long)info->vcpu.start);
- kvm_vgic_global_state.vcpu_base = 0;
} else {
kvm_vgic_global_state.vcpu_base = info->vcpu.start;
kvm_vgic_global_state.can_emulate_gicv2 = true;
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
2021-01-08 17:12 ` Marc Zyngier
@ 2021-01-08 17:59 ` Ard Biesheuvel
-1 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2021-01-08 17:59 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, Android Kernel Team, kvmarm
On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
>
> It looks like we have broken firmware out there that wrongly advertises
> a GICv2 compatibility interface, despite the CPUs not being able to deal
> with it.
>
> To work around this, check that the CPU initialising KVM is actually able
> to switch to MMIO instead of system registers, and use that as a
> precondition to enable GICv2 compatibility in KVM.
>
> Note that the detection happens on a single CPU. If the firmware is
> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34 +++++++++++++++++++++++++++++++--
> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
> 2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 005daa0c9dd7..d504499ab917 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
> /*
> * Return the GIC CPU configuration:
> * - [31:0] ICH_VTR_EL2
> - * - [63:32] RES0
> + * - [62:32] RES0
> + * - [63] MMIO (GICv2) capable
> */
> u64 __vgic_v3_get_gic_config(void)
> {
> - return read_gicreg(ICH_VTR_EL2);
> + u64 sre = read_gicreg(ICC_SRE_EL1);
> + unsigned long flags = 0;
> + bool v2_capable;
> +
> + /*
> + * To check whether we have a MMIO-based (GICv2 compatible)
> + * CPU interface, we need to disable the system register
> + * view. To do that safely, we have to prevent any interrupt
> + * from firing (which would be deadly).
> + *
> + * Note that this only makes sense on VHE, as interrupts are
> + * already masked for nVHE as part of the exception entry to
> + * EL2.
> + */
> + if (has_vhe())
> + flags = local_daif_save();
> +
> + write_gicreg(0, ICC_SRE_EL1);
> + isb();
> +
> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
> +
> + write_gicreg(sre, ICC_SRE_EL1);
> + isb();
> +
> + if (has_vhe())
> + local_daif_restore(flags);
> +
> + return (read_gicreg(ICH_VTR_EL2) |
> + v2_capable ? (1ULL << 63) : 0);
> }
>
Is it necessary to perform this check unconditionally? We only care
about this if the firmware claims v2 compat support.
> u64 __vgic_v3_read_vmcr(void)
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 8e7bf3151057..67b27b47312b 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -584,8 +584,10 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
> int vgic_v3_probe(const struct gic_kvm_info *info)
> {
> u64 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_gic_config);
> + bool has_v2;
> int ret;
>
> + has_v2 = ich_vtr_el2 >> 63;
> ich_vtr_el2 = (u32)ich_vtr_el2;
>
> /*
> @@ -605,13 +607,15 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
> gicv4_enable ? "en" : "dis");
> }
>
> + kvm_vgic_global_state.vcpu_base = 0;
> +
> if (!info->vcpu.start) {
> kvm_info("GICv3: no GICV resource entry\n");
> - kvm_vgic_global_state.vcpu_base = 0;
> + } else if (!has_v2) {
> + pr_warn("CPU interface incapable of MMIO access\n");
> } else if (!PAGE_ALIGNED(info->vcpu.start)) {
> pr_warn("GICV physical address 0x%llx not page aligned\n",
> (unsigned long long)info->vcpu.start);
> - kvm_vgic_global_state.vcpu_base = 0;
> } else {
> kvm_vgic_global_state.vcpu_base = info->vcpu.start;
> kvm_vgic_global_state.can_emulate_gicv2 = true;
> --
> 2.29.2
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
@ 2021-01-08 17:59 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2021-01-08 17:59 UTC (permalink / raw)
To: Marc Zyngier
Cc: Suzuki K Poulose, Shameerali Kolothum Thodi, James Morse,
Linux ARM, Android Kernel Team, kvmarm, Julien Thierry
On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
>
> It looks like we have broken firmware out there that wrongly advertises
> a GICv2 compatibility interface, despite the CPUs not being able to deal
> with it.
>
> To work around this, check that the CPU initialising KVM is actually able
> to switch to MMIO instead of system registers, and use that as a
> precondition to enable GICv2 compatibility in KVM.
>
> Note that the detection happens on a single CPU. If the firmware is
> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34 +++++++++++++++++++++++++++++++--
> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
> 2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 005daa0c9dd7..d504499ab917 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
> /*
> * Return the GIC CPU configuration:
> * - [31:0] ICH_VTR_EL2
> - * - [63:32] RES0
> + * - [62:32] RES0
> + * - [63] MMIO (GICv2) capable
> */
> u64 __vgic_v3_get_gic_config(void)
> {
> - return read_gicreg(ICH_VTR_EL2);
> + u64 sre = read_gicreg(ICC_SRE_EL1);
> + unsigned long flags = 0;
> + bool v2_capable;
> +
> + /*
> + * To check whether we have a MMIO-based (GICv2 compatible)
> + * CPU interface, we need to disable the system register
> + * view. To do that safely, we have to prevent any interrupt
> + * from firing (which would be deadly).
> + *
> + * Note that this only makes sense on VHE, as interrupts are
> + * already masked for nVHE as part of the exception entry to
> + * EL2.
> + */
> + if (has_vhe())
> + flags = local_daif_save();
> +
> + write_gicreg(0, ICC_SRE_EL1);
> + isb();
> +
> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
> +
> + write_gicreg(sre, ICC_SRE_EL1);
> + isb();
> +
> + if (has_vhe())
> + local_daif_restore(flags);
> +
> + return (read_gicreg(ICH_VTR_EL2) |
> + v2_capable ? (1ULL << 63) : 0);
> }
>
Is it necessary to perform this check unconditionally? We only care
about this if the firmware claims v2 compat support.
> u64 __vgic_v3_read_vmcr(void)
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 8e7bf3151057..67b27b47312b 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -584,8 +584,10 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
> int vgic_v3_probe(const struct gic_kvm_info *info)
> {
> u64 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_gic_config);
> + bool has_v2;
> int ret;
>
> + has_v2 = ich_vtr_el2 >> 63;
> ich_vtr_el2 = (u32)ich_vtr_el2;
>
> /*
> @@ -605,13 +607,15 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
> gicv4_enable ? "en" : "dis");
> }
>
> + kvm_vgic_global_state.vcpu_base = 0;
> +
> if (!info->vcpu.start) {
> kvm_info("GICv3: no GICV resource entry\n");
> - kvm_vgic_global_state.vcpu_base = 0;
> + } else if (!has_v2) {
> + pr_warn("CPU interface incapable of MMIO access\n");
> } else if (!PAGE_ALIGNED(info->vcpu.start)) {
> pr_warn("GICV physical address 0x%llx not page aligned\n",
> (unsigned long long)info->vcpu.start);
> - kvm_vgic_global_state.vcpu_base = 0;
> } else {
> kvm_vgic_global_state.vcpu_base = info->vcpu.start;
> kvm_vgic_global_state.can_emulate_gicv2 = true;
> --
> 2.29.2
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
2021-01-08 17:59 ` Ard Biesheuvel
@ 2021-01-08 18:12 ` Marc Zyngier
-1 siblings, 0 replies; 16+ messages in thread
From: Marc Zyngier @ 2021-01-08 18:12 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Linux ARM, Android Kernel Team, kvmarm
On 2021-01-08 17:59, Ard Biesheuvel wrote:
> On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
>>
>> It looks like we have broken firmware out there that wrongly
>> advertises
>> a GICv2 compatibility interface, despite the CPUs not being able to
>> deal
>> with it.
>>
>> To work around this, check that the CPU initialising KVM is actually
>> able
>> to switch to MMIO instead of system registers, and use that as a
>> precondition to enable GICv2 compatibility in KVM.
>>
>> Note that the detection happens on a single CPU. If the firmware is
>> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>>
>> Reported-by: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
>> +++++++++++++++++++++++++++++++--
>> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
>> 2 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 005daa0c9dd7..d504499ab917 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
>> /*
>> * Return the GIC CPU configuration:
>> * - [31:0] ICH_VTR_EL2
>> - * - [63:32] RES0
>> + * - [62:32] RES0
>> + * - [63] MMIO (GICv2) capable
>> */
>> u64 __vgic_v3_get_gic_config(void)
>> {
>> - return read_gicreg(ICH_VTR_EL2);
>> + u64 sre = read_gicreg(ICC_SRE_EL1);
>> + unsigned long flags = 0;
>> + bool v2_capable;
>> +
>> + /*
>> + * To check whether we have a MMIO-based (GICv2 compatible)
>> + * CPU interface, we need to disable the system register
>> + * view. To do that safely, we have to prevent any interrupt
>> + * from firing (which would be deadly).
>> + *
>> + * Note that this only makes sense on VHE, as interrupts are
>> + * already masked for nVHE as part of the exception entry to
>> + * EL2.
>> + */
>> + if (has_vhe())
>> + flags = local_daif_save();
>> +
>> + write_gicreg(0, ICC_SRE_EL1);
>> + isb();
>> +
>> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
>> +
>> + write_gicreg(sre, ICC_SRE_EL1);
>> + isb();
>> +
>> + if (has_vhe())
>> + local_daif_restore(flags);
>> +
>> + return (read_gicreg(ICH_VTR_EL2) |
>> + v2_capable ? (1ULL << 63) : 0);
>> }
>>
>
> Is it necessary to perform this check unconditionally? We only care
> about this if the firmware claims v2 compat support.
Indeed. But this is done exactly once per boot, and I see it as
a way to extract the CPU configuration more than anything else.
Extracting it *only* when we have some v2 compat info would mean
sharing that information with EL2 (in the nVHE case), and it felt
more hassle than it is worth.
Do you foresee any issue with this, other than the whole thing
being disgusting (which I wilfully admit)?
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
@ 2021-01-08 18:12 ` Marc Zyngier
0 siblings, 0 replies; 16+ messages in thread
From: Marc Zyngier @ 2021-01-08 18:12 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Suzuki K Poulose, Shameerali Kolothum Thodi, James Morse,
Linux ARM, Android Kernel Team, kvmarm, Julien Thierry
On 2021-01-08 17:59, Ard Biesheuvel wrote:
> On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
>>
>> It looks like we have broken firmware out there that wrongly
>> advertises
>> a GICv2 compatibility interface, despite the CPUs not being able to
>> deal
>> with it.
>>
>> To work around this, check that the CPU initialising KVM is actually
>> able
>> to switch to MMIO instead of system registers, and use that as a
>> precondition to enable GICv2 compatibility in KVM.
>>
>> Note that the detection happens on a single CPU. If the firmware is
>> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>>
>> Reported-by: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
>> +++++++++++++++++++++++++++++++--
>> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
>> 2 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 005daa0c9dd7..d504499ab917 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
>> /*
>> * Return the GIC CPU configuration:
>> * - [31:0] ICH_VTR_EL2
>> - * - [63:32] RES0
>> + * - [62:32] RES0
>> + * - [63] MMIO (GICv2) capable
>> */
>> u64 __vgic_v3_get_gic_config(void)
>> {
>> - return read_gicreg(ICH_VTR_EL2);
>> + u64 sre = read_gicreg(ICC_SRE_EL1);
>> + unsigned long flags = 0;
>> + bool v2_capable;
>> +
>> + /*
>> + * To check whether we have a MMIO-based (GICv2 compatible)
>> + * CPU interface, we need to disable the system register
>> + * view. To do that safely, we have to prevent any interrupt
>> + * from firing (which would be deadly).
>> + *
>> + * Note that this only makes sense on VHE, as interrupts are
>> + * already masked for nVHE as part of the exception entry to
>> + * EL2.
>> + */
>> + if (has_vhe())
>> + flags = local_daif_save();
>> +
>> + write_gicreg(0, ICC_SRE_EL1);
>> + isb();
>> +
>> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
>> +
>> + write_gicreg(sre, ICC_SRE_EL1);
>> + isb();
>> +
>> + if (has_vhe())
>> + local_daif_restore(flags);
>> +
>> + return (read_gicreg(ICH_VTR_EL2) |
>> + v2_capable ? (1ULL << 63) : 0);
>> }
>>
>
> Is it necessary to perform this check unconditionally? We only care
> about this if the firmware claims v2 compat support.
Indeed. But this is done exactly once per boot, and I see it as
a way to extract the CPU configuration more than anything else.
Extracting it *only* when we have some v2 compat info would mean
sharing that information with EL2 (in the nVHE case), and it felt
more hassle than it is worth.
Do you foresee any issue with this, other than the whole thing
being disgusting (which I wilfully admit)?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
2021-01-08 18:12 ` Marc Zyngier
@ 2021-01-08 18:19 ` Ard Biesheuvel
-1 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2021-01-08 18:19 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, Android Kernel Team, kvmarm
On Fri, 8 Jan 2021 at 19:13, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2021-01-08 17:59, Ard Biesheuvel wrote:
> > On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> It looks like we have broken firmware out there that wrongly
> >> advertises
> >> a GICv2 compatibility interface, despite the CPUs not being able to
> >> deal
> >> with it.
> >>
> >> To work around this, check that the CPU initialising KVM is actually
> >> able
> >> to switch to MMIO instead of system registers, and use that as a
> >> precondition to enable GICv2 compatibility in KVM.
> >>
> >> Note that the detection happens on a single CPU. If the firmware is
> >> lying *and* that the CPUs are asymetric, all hope is lost anyway.
> >>
> >> Reported-by: Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>
> >> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >> ---
> >> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
> >> +++++++++++++++++++++++++++++++--
> >> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
> >> 2 files changed, 38 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> index 005daa0c9dd7..d504499ab917 100644
> >> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
> >> /*
> >> * Return the GIC CPU configuration:
> >> * - [31:0] ICH_VTR_EL2
> >> - * - [63:32] RES0
> >> + * - [62:32] RES0
> >> + * - [63] MMIO (GICv2) capable
> >> */
> >> u64 __vgic_v3_get_gic_config(void)
> >> {
> >> - return read_gicreg(ICH_VTR_EL2);
> >> + u64 sre = read_gicreg(ICC_SRE_EL1);
> >> + unsigned long flags = 0;
> >> + bool v2_capable;
> >> +
> >> + /*
> >> + * To check whether we have a MMIO-based (GICv2 compatible)
> >> + * CPU interface, we need to disable the system register
> >> + * view. To do that safely, we have to prevent any interrupt
> >> + * from firing (which would be deadly).
> >> + *
> >> + * Note that this only makes sense on VHE, as interrupts are
> >> + * already masked for nVHE as part of the exception entry to
> >> + * EL2.
> >> + */
> >> + if (has_vhe())
> >> + flags = local_daif_save();
> >> +
> >> + write_gicreg(0, ICC_SRE_EL1);
> >> + isb();
> >> +
> >> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
> >> +
> >> + write_gicreg(sre, ICC_SRE_EL1);
> >> + isb();
> >> +
> >> + if (has_vhe())
> >> + local_daif_restore(flags);
> >> +
> >> + return (read_gicreg(ICH_VTR_EL2) |
> >> + v2_capable ? (1ULL << 63) : 0);
> >> }
> >>
> >
> > Is it necessary to perform this check unconditionally? We only care
> > about this if the firmware claims v2 compat support.
>
> Indeed. But this is done exactly once per boot, and I see it as
> a way to extract the CPU configuration more than anything else.
>
> Extracting it *only* when we have some v2 compat info would mean
> sharing that information with EL2 (in the nVHE case), and it felt
> more hassle than it is worth.
>
> Do you foresee any issue with this, other than the whole thing
> being disgusting (which I wilfully admit)?
>
No I don't think it's a problem per se. Just a bit disappointing that
every system will be burdened with this for as long as the last v2
compat capable system is still being supported.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
@ 2021-01-08 18:19 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2021-01-08 18:19 UTC (permalink / raw)
To: Marc Zyngier
Cc: Suzuki K Poulose, Shameerali Kolothum Thodi, James Morse,
Linux ARM, Android Kernel Team, kvmarm, Julien Thierry
On Fri, 8 Jan 2021 at 19:13, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2021-01-08 17:59, Ard Biesheuvel wrote:
> > On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> It looks like we have broken firmware out there that wrongly
> >> advertises
> >> a GICv2 compatibility interface, despite the CPUs not being able to
> >> deal
> >> with it.
> >>
> >> To work around this, check that the CPU initialising KVM is actually
> >> able
> >> to switch to MMIO instead of system registers, and use that as a
> >> precondition to enable GICv2 compatibility in KVM.
> >>
> >> Note that the detection happens on a single CPU. If the firmware is
> >> lying *and* that the CPUs are asymetric, all hope is lost anyway.
> >>
> >> Reported-by: Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>
> >> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >> ---
> >> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
> >> +++++++++++++++++++++++++++++++--
> >> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
> >> 2 files changed, 38 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> index 005daa0c9dd7..d504499ab917 100644
> >> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> >> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
> >> /*
> >> * Return the GIC CPU configuration:
> >> * - [31:0] ICH_VTR_EL2
> >> - * - [63:32] RES0
> >> + * - [62:32] RES0
> >> + * - [63] MMIO (GICv2) capable
> >> */
> >> u64 __vgic_v3_get_gic_config(void)
> >> {
> >> - return read_gicreg(ICH_VTR_EL2);
> >> + u64 sre = read_gicreg(ICC_SRE_EL1);
> >> + unsigned long flags = 0;
> >> + bool v2_capable;
> >> +
> >> + /*
> >> + * To check whether we have a MMIO-based (GICv2 compatible)
> >> + * CPU interface, we need to disable the system register
> >> + * view. To do that safely, we have to prevent any interrupt
> >> + * from firing (which would be deadly).
> >> + *
> >> + * Note that this only makes sense on VHE, as interrupts are
> >> + * already masked for nVHE as part of the exception entry to
> >> + * EL2.
> >> + */
> >> + if (has_vhe())
> >> + flags = local_daif_save();
> >> +
> >> + write_gicreg(0, ICC_SRE_EL1);
> >> + isb();
> >> +
> >> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
> >> +
> >> + write_gicreg(sre, ICC_SRE_EL1);
> >> + isb();
> >> +
> >> + if (has_vhe())
> >> + local_daif_restore(flags);
> >> +
> >> + return (read_gicreg(ICH_VTR_EL2) |
> >> + v2_capable ? (1ULL << 63) : 0);
> >> }
> >>
> >
> > Is it necessary to perform this check unconditionally? We only care
> > about this if the firmware claims v2 compat support.
>
> Indeed. But this is done exactly once per boot, and I see it as
> a way to extract the CPU configuration more than anything else.
>
> Extracting it *only* when we have some v2 compat info would mean
> sharing that information with EL2 (in the nVHE case), and it felt
> more hassle than it is worth.
>
> Do you foresee any issue with this, other than the whole thing
> being disgusting (which I wilfully admit)?
>
No I don't think it's a problem per se. Just a bit disappointing that
every system will be burdened with this for as long as the last v2
compat capable system is still being supported.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
2021-01-08 17:12 ` Marc Zyngier
@ 2021-01-11 12:21 ` Shameerali Kolothum Thodi
-1 siblings, 0 replies; 16+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-01-11 12:21 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu
Cc: linuxarm@openeuler.org, kernel-team@android.com, Ard Biesheuvel
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier [mailto:maz@kernel.org]
> Sent: 08 January 2021 17:12
> To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> James Morse <james.morse@arm.com>; Julien Thierry
> <julien.thierry.kdev@gmail.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Ard Biesheuvel <ardb@kernel.org>;
> kernel-team@android.com
> Subject: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising
> GICv2-on-v3 compatibility
>
> It looks like we have broken firmware out there that wrongly advertises
> a GICv2 compatibility interface, despite the CPUs not being able to deal
> with it.
>
> To work around this, check that the CPU initialising KVM is actually able
> to switch to MMIO instead of system registers, and use that as a
> precondition to enable GICv2 compatibility in KVM.
>
> Note that the detection happens on a single CPU. If the firmware is
> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>
> Reported-by: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34 +++++++++++++++++++++++++++++++--
> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
> 2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 005daa0c9dd7..d504499ab917 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
> /*
> * Return the GIC CPU configuration:
> * - [31:0] ICH_VTR_EL2
> - * - [63:32] RES0
> + * - [62:32] RES0
> + * - [63] MMIO (GICv2) capable
> */
> u64 __vgic_v3_get_gic_config(void)
> {
> - return read_gicreg(ICH_VTR_EL2);
> + u64 sre = read_gicreg(ICC_SRE_EL1);
> + unsigned long flags = 0;
> + bool v2_capable;
> +
> + /*
> + * To check whether we have a MMIO-based (GICv2 compatible)
> + * CPU interface, we need to disable the system register
> + * view. To do that safely, we have to prevent any interrupt
> + * from firing (which would be deadly).
> + *
> + * Note that this only makes sense on VHE, as interrupts are
> + * already masked for nVHE as part of the exception entry to
> + * EL2.
> + */
> + if (has_vhe())
> + flags = local_daif_save();
> +
> + write_gicreg(0, ICC_SRE_EL1);
> + isb();
> +
> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
> +
> + write_gicreg(sre, ICC_SRE_EL1);
> + isb();
> +
> + if (has_vhe())
> + local_daif_restore(flags);
> +
> + return (read_gicreg(ICH_VTR_EL2) |
> + v2_capable ? (1ULL << 63) : 0);
> }
Thanks for sending this out. I had a go with this series and unfortunately
it didn't work on a system with faulty BIOS. It looks like the culprit here is
the ?: operator. There seems to be an operator precedence at play here
and it returns,
vgic_v3_probe: ich_vtr_el2 0x8000000000000000
And with the below change,
return (read_gicreg(ICH_VTR_EL2) |
- v2_capable ? (1ULL << 63) : 0);
+ (v2_capable ? (1ULL << 63) : 0));
}
It returns,
vgic_v3_probe: ich_vtr_el2 0x90080003
and works correctly.
[ 18.918738] kvm [1]: disabling GICv2 emulation
Thanks,
Shameer
> u64 __vgic_v3_read_vmcr(void)
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 8e7bf3151057..67b27b47312b 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -584,8 +584,10 @@ early_param("kvm-arm.vgic_v4_enable",
> early_gicv4_enable);
> int vgic_v3_probe(const struct gic_kvm_info *info)
> {
> u64 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_gic_config);
> + bool has_v2;
> int ret;
>
> + has_v2 = ich_vtr_el2 >> 63;
> ich_vtr_el2 = (u32)ich_vtr_el2;
>
> /*
> @@ -605,13 +607,15 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
> gicv4_enable ? "en" : "dis");
> }
>
> + kvm_vgic_global_state.vcpu_base = 0;
> +
> if (!info->vcpu.start) {
> kvm_info("GICv3: no GICV resource entry\n");
> - kvm_vgic_global_state.vcpu_base = 0;
> + } else if (!has_v2) {
> + pr_warn("CPU interface incapable of MMIO access\n");
> } else if (!PAGE_ALIGNED(info->vcpu.start)) {
> pr_warn("GICV physical address 0x%llx not page aligned\n",
> (unsigned long long)info->vcpu.start);
> - kvm_vgic_global_state.vcpu_base = 0;
> } else {
> kvm_vgic_global_state.vcpu_base = info->vcpu.start;
> kvm_vgic_global_state.can_emulate_gicv2 = true;
> --
> 2.29.2
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
@ 2021-01-11 12:21 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 16+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-01-11 12:21 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu
Cc: Suzuki K Poulose, linuxarm@openeuler.org, James Morse,
kernel-team@android.com, Ard Biesheuvel, Julien Thierry
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier [mailto:maz@kernel.org]
> Sent: 08 January 2021 17:12
> To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> James Morse <james.morse@arm.com>; Julien Thierry
> <julien.thierry.kdev@gmail.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Ard Biesheuvel <ardb@kernel.org>;
> kernel-team@android.com
> Subject: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising
> GICv2-on-v3 compatibility
>
> It looks like we have broken firmware out there that wrongly advertises
> a GICv2 compatibility interface, despite the CPUs not being able to deal
> with it.
>
> To work around this, check that the CPU initialising KVM is actually able
> to switch to MMIO instead of system registers, and use that as a
> precondition to enable GICv2 compatibility in KVM.
>
> Note that the detection happens on a single CPU. If the firmware is
> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>
> Reported-by: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34 +++++++++++++++++++++++++++++++--
> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
> 2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> index 005daa0c9dd7..d504499ab917 100644
> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
> /*
> * Return the GIC CPU configuration:
> * - [31:0] ICH_VTR_EL2
> - * - [63:32] RES0
> + * - [62:32] RES0
> + * - [63] MMIO (GICv2) capable
> */
> u64 __vgic_v3_get_gic_config(void)
> {
> - return read_gicreg(ICH_VTR_EL2);
> + u64 sre = read_gicreg(ICC_SRE_EL1);
> + unsigned long flags = 0;
> + bool v2_capable;
> +
> + /*
> + * To check whether we have a MMIO-based (GICv2 compatible)
> + * CPU interface, we need to disable the system register
> + * view. To do that safely, we have to prevent any interrupt
> + * from firing (which would be deadly).
> + *
> + * Note that this only makes sense on VHE, as interrupts are
> + * already masked for nVHE as part of the exception entry to
> + * EL2.
> + */
> + if (has_vhe())
> + flags = local_daif_save();
> +
> + write_gicreg(0, ICC_SRE_EL1);
> + isb();
> +
> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
> +
> + write_gicreg(sre, ICC_SRE_EL1);
> + isb();
> +
> + if (has_vhe())
> + local_daif_restore(flags);
> +
> + return (read_gicreg(ICH_VTR_EL2) |
> + v2_capable ? (1ULL << 63) : 0);
> }
Thanks for sending this out. I had a go with this series and unfortunately
it didn't work on a system with faulty BIOS. It looks like the culprit here is
the ?: operator. There seems to be an operator precedence at play here
and it returns,
vgic_v3_probe: ich_vtr_el2 0x8000000000000000
And with the below change,
return (read_gicreg(ICH_VTR_EL2) |
- v2_capable ? (1ULL << 63) : 0);
+ (v2_capable ? (1ULL << 63) : 0));
}
It returns,
vgic_v3_probe: ich_vtr_el2 0x90080003
and works correctly.
[ 18.918738] kvm [1]: disabling GICv2 emulation
Thanks,
Shameer
> u64 __vgic_v3_read_vmcr(void)
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 8e7bf3151057..67b27b47312b 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -584,8 +584,10 @@ early_param("kvm-arm.vgic_v4_enable",
> early_gicv4_enable);
> int vgic_v3_probe(const struct gic_kvm_info *info)
> {
> u64 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_gic_config);
> + bool has_v2;
> int ret;
>
> + has_v2 = ich_vtr_el2 >> 63;
> ich_vtr_el2 = (u32)ich_vtr_el2;
>
> /*
> @@ -605,13 +607,15 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
> gicv4_enable ? "en" : "dis");
> }
>
> + kvm_vgic_global_state.vcpu_base = 0;
> +
> if (!info->vcpu.start) {
> kvm_info("GICv3: no GICV resource entry\n");
> - kvm_vgic_global_state.vcpu_base = 0;
> + } else if (!has_v2) {
> + pr_warn("CPU interface incapable of MMIO access\n");
> } else if (!PAGE_ALIGNED(info->vcpu.start)) {
> pr_warn("GICV physical address 0x%llx not page aligned\n",
> (unsigned long long)info->vcpu.start);
> - kvm_vgic_global_state.vcpu_base = 0;
> } else {
> kvm_vgic_global_state.vcpu_base = info->vcpu.start;
> kvm_vgic_global_state.can_emulate_gicv2 = true;
> --
> 2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
2021-01-11 12:21 ` Shameerali Kolothum Thodi
@ 2021-01-11 13:20 ` Marc Zyngier
-1 siblings, 0 replies; 16+ messages in thread
From: Marc Zyngier @ 2021-01-11 13:20 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: linuxarm, kvmarm, linux-arm-kernel, kernel-team, Ard Biesheuvel
On 2021-01-11 12:21, Shameerali Kolothum Thodi wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Zyngier [mailto:maz@kernel.org]
>> Sent: 08 January 2021 17:12
>> To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> James Morse <james.morse@arm.com>; Julien Thierry
>> <julien.thierry.kdev@gmail.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>; Ard Biesheuvel <ardb@kernel.org>;
>> kernel-team@android.com
>> Subject: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly
>> advertising
>> GICv2-on-v3 compatibility
>>
>> It looks like we have broken firmware out there that wrongly
>> advertises
>> a GICv2 compatibility interface, despite the CPUs not being able to
>> deal
>> with it.
>>
>> To work around this, check that the CPU initialising KVM is actually
>> able
>> to switch to MMIO instead of system registers, and use that as a
>> precondition to enable GICv2 compatibility in KVM.
>>
>> Note that the detection happens on a single CPU. If the firmware is
>> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>>
>> Reported-by: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
>> +++++++++++++++++++++++++++++++--
>> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
>> 2 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 005daa0c9dd7..d504499ab917 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
>> /*
>> * Return the GIC CPU configuration:
>> * - [31:0] ICH_VTR_EL2
>> - * - [63:32] RES0
>> + * - [62:32] RES0
>> + * - [63] MMIO (GICv2) capable
>> */
>> u64 __vgic_v3_get_gic_config(void)
>> {
>> - return read_gicreg(ICH_VTR_EL2);
>> + u64 sre = read_gicreg(ICC_SRE_EL1);
>> + unsigned long flags = 0;
>> + bool v2_capable;
>> +
>> + /*
>> + * To check whether we have a MMIO-based (GICv2 compatible)
>> + * CPU interface, we need to disable the system register
>> + * view. To do that safely, we have to prevent any interrupt
>> + * from firing (which would be deadly).
>> + *
>> + * Note that this only makes sense on VHE, as interrupts are
>> + * already masked for nVHE as part of the exception entry to
>> + * EL2.
>> + */
>> + if (has_vhe())
>> + flags = local_daif_save();
>> +
>> + write_gicreg(0, ICC_SRE_EL1);
>> + isb();
>> +
>> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
>> +
>> + write_gicreg(sre, ICC_SRE_EL1);
>> + isb();
>> +
>> + if (has_vhe())
>> + local_daif_restore(flags);
>> +
>> + return (read_gicreg(ICH_VTR_EL2) |
>> + v2_capable ? (1ULL << 63) : 0);
>> }
>
> Thanks for sending this out. I had a go with this series and
> unfortunately
> it didn't work on a system with faulty BIOS. It looks like the culprit
> here is
> the ?: operator. There seems to be an operator precedence at play here
> and it returns,
> vgic_v3_probe: ich_vtr_el2 0x8000000000000000
>
> And with the below change,
>
> return (read_gicreg(ICH_VTR_EL2) |
> - v2_capable ? (1ULL << 63) : 0);
> + (v2_capable ? (1ULL << 63) : 0));
Gaahh. Well caught! Each time I use this operator, I end-up screwing
up one way or another. Thanks for the heads up, and for testing.
I'll respin the series shortly.
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility
@ 2021-01-11 13:20 ` Marc Zyngier
0 siblings, 0 replies; 16+ messages in thread
From: Marc Zyngier @ 2021-01-11 13:20 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: Suzuki K Poulose, linuxarm, kvmarm, James Morse, linux-arm-kernel,
kernel-team, Ard Biesheuvel, Julien Thierry
On 2021-01-11 12:21, Shameerali Kolothum Thodi wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Zyngier [mailto:maz@kernel.org]
>> Sent: 08 January 2021 17:12
>> To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> James Morse <james.morse@arm.com>; Julien Thierry
>> <julien.thierry.kdev@gmail.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>; Ard Biesheuvel <ardb@kernel.org>;
>> kernel-team@android.com
>> Subject: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly
>> advertising
>> GICv2-on-v3 compatibility
>>
>> It looks like we have broken firmware out there that wrongly
>> advertises
>> a GICv2 compatibility interface, despite the CPUs not being able to
>> deal
>> with it.
>>
>> To work around this, check that the CPU initialising KVM is actually
>> able
>> to switch to MMIO instead of system registers, and use that as a
>> precondition to enable GICv2 compatibility in KVM.
>>
>> Note that the detection happens on a single CPU. If the firmware is
>> lying *and* that the CPUs are asymetric, all hope is lost anyway.
>>
>> Reported-by: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
>> +++++++++++++++++++++++++++++++--
>> arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
>> 2 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 005daa0c9dd7..d504499ab917 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
>> /*
>> * Return the GIC CPU configuration:
>> * - [31:0] ICH_VTR_EL2
>> - * - [63:32] RES0
>> + * - [62:32] RES0
>> + * - [63] MMIO (GICv2) capable
>> */
>> u64 __vgic_v3_get_gic_config(void)
>> {
>> - return read_gicreg(ICH_VTR_EL2);
>> + u64 sre = read_gicreg(ICC_SRE_EL1);
>> + unsigned long flags = 0;
>> + bool v2_capable;
>> +
>> + /*
>> + * To check whether we have a MMIO-based (GICv2 compatible)
>> + * CPU interface, we need to disable the system register
>> + * view. To do that safely, we have to prevent any interrupt
>> + * from firing (which would be deadly).
>> + *
>> + * Note that this only makes sense on VHE, as interrupts are
>> + * already masked for nVHE as part of the exception entry to
>> + * EL2.
>> + */
>> + if (has_vhe())
>> + flags = local_daif_save();
>> +
>> + write_gicreg(0, ICC_SRE_EL1);
>> + isb();
>> +
>> + v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
>> +
>> + write_gicreg(sre, ICC_SRE_EL1);
>> + isb();
>> +
>> + if (has_vhe())
>> + local_daif_restore(flags);
>> +
>> + return (read_gicreg(ICH_VTR_EL2) |
>> + v2_capable ? (1ULL << 63) : 0);
>> }
>
> Thanks for sending this out. I had a go with this series and
> unfortunately
> it didn't work on a system with faulty BIOS. It looks like the culprit
> here is
> the ?: operator. There seems to be an operator precedence at play here
> and it returns,
> vgic_v3_probe: ich_vtr_el2 0x8000000000000000
>
> And with the below change,
>
> return (read_gicreg(ICH_VTR_EL2) |
> - v2_capable ? (1ULL << 63) : 0);
> + (v2_capable ? (1ULL << 63) : 0));
Gaahh. Well caught! Each time I use this operator, I end-up screwing
up one way or another. Thanks for the heads up, and for testing.
I'll respin the series shortly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread