lkml.org 
[lkml]   [2021]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [PATCH v2 1/1] irqchip/gic-v4.1: Disable vSGI upon (GIC CPUIF < v4.1) detection
    Hi Lorenzo,

    On Tue, 02 Mar 2021 10:27:44 +0000,
    Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
    >
    > GIC CPU interfaces versions predating GIC v4.1 were not built to
    > accommodate vINTID within the vSGI range; as reported in the GIC
    > specifications (8.2 "Changes to the CPU interface"), it is
    > CONSTRAINED UNPREDICTABLE to deliver a vSGI to a PE with
    > ID_AA64PFR0_EL1.GIC < b0011.
    >
    > Check the GIC CPUIF version by reading the SYS_ID_AA64_PFR0_EL1.
    >
    > Disable vSGIs if a CPUIF version < 4.1 is detected to prevent using
    > vSGIs on systems where they may misbehave.
    >
    > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
    > Cc: Marc Zyngier <maz@kernel.org>
    > ---
    > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 4 ++--
    > arch/arm64/kvm/vgic/vgic-v3.c | 3 ++-
    > drivers/irqchip/irq-gic-v3-its.c | 6 +++++-
    > drivers/irqchip/irq-gic-v3.c | 22 ++++++++++++++++++++++
    > include/kvm/arm_vgic.h | 1 +
    > include/linux/irqchip/arm-gic-common.h | 2 ++
    > include/linux/irqchip/arm-gic-v3.h | 1 +
    > 7 files changed, 35 insertions(+), 4 deletions(-)
    >
    > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
    > index 15a6c98ee92f..66548cd2a715 100644
    > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
    > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
    > @@ -86,7 +86,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
    > }
    > break;
    > case GICD_TYPER2:
    > - if (kvm_vgic_global_state.has_gicv4_1)
    > + if (kvm_vgic_global_state.has_gicv4_1_vsgi)
    > value = GICD_TYPER2_nASSGIcap;
    > break;
    > case GICD_IIDR:
    > @@ -119,7 +119,7 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
    > dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
    >
    > /* Not a GICv4.1? No HW SGIs */
    > - if (!kvm_vgic_global_state.has_gicv4_1)
    > + if (!kvm_vgic_global_state.has_gicv4_1_vsgi)
    > val &= ~GICD_CTLR_nASSGIreq;
    >
    > /* Dist stays enabled? nASSGIreq is RO */
    > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
    > index 52915b342351..57b73100e8cc 100644
    > --- a/arch/arm64/kvm/vgic/vgic-v3.c
    > +++ b/arch/arm64/kvm/vgic/vgic-v3.c
    > @@ -533,7 +533,7 @@ int vgic_v3_map_resources(struct kvm *kvm)
    > return ret;
    > }
    >
    > - if (kvm_vgic_global_state.has_gicv4_1)
    > + if (kvm_vgic_global_state.has_gicv4_1_vsgi)
    > vgic_v4_configure_vsgis(kvm);
    >
    > return 0;
    > @@ -589,6 +589,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
    > if (info->has_v4) {
    > kvm_vgic_global_state.has_gicv4 = gicv4_enable;
    > kvm_vgic_global_state.has_gicv4_1 = info->has_v4_1 && gicv4_enable;
    > + kvm_vgic_global_state.has_gicv4_1_vsgi = info->has_v4_1_vsgi && gicv4_enable;
    > kvm_info("GICv4%s support %sabled\n",
    > kvm_vgic_global_state.has_gicv4_1 ? ".1" : "",
    > gicv4_enable ? "en" : "dis");
    > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
    > index ed46e6057e33..ee2a2ca27d5c 100644
    > --- a/drivers/irqchip/irq-gic-v3-its.c
    > +++ b/drivers/irqchip/irq-gic-v3-its.c
    > @@ -5412,7 +5412,11 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
    > if (has_v4 & rdists->has_vlpis) {
    > const struct irq_domain_ops *sgi_ops;
    >
    > - if (has_v4_1)
    > + /*
    > + * Enable vSGIs only if the ITS and the
    > + * GIC CPUIF support them.
    > + */
    > + if (has_v4_1 && rdists->has_vsgi_cpuif)
    > sgi_ops = &its_sgi_domain_ops;
    > else
    > sgi_ops = NULL;

    This doesn't seem right. If you pass NULL for the SGI ops, you also
    lose the per-VPE doorbells and stick to the terrible GICv4.0 behaviour
    (see the use of has_v4_1() in irq-gic-v4.c). I don't think that is
    what you really want.

    > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
    > index eb0ee356a629..fd6cd9a5de34 100644
    > --- a/drivers/irqchip/irq-gic-v3.c
    > +++ b/drivers/irqchip/irq-gic-v3.c
    > @@ -31,6 +31,21 @@
    >
    > #include "irq-gic-common.h"
    >
    > +#ifdef CONFIG_ARM64
    > +#include <asm/cpufeature.h>
    > +
    > +static inline bool gic_cpuif_has_vsgi(void)
    > +{
    > + unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
    > +
    > + fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_GIC_SHIFT);
    > +
    > + return fld >= 0x3;
    > +}
    > +#else
    > +static inline bool gic_cpuif_has_vsgi(void) { return false; }
    > +#endif

    Why do we need to expose this in the GICv3 driver instead of the GICv4
    code? At the moment, you track this state:

    - in gic_data.rdists.has_vsgi_cpuif
    - indirectly in gic_v3_kvm_info.has_v4_1_vsgi
    - indirectly in kvm_vgic_global_state.has_gicv4_1_vsgi

    Can't we simplify the logic and track it *once*? Or even better, just
    evaluate it when required? I hacked the following stuff based on your
    patch (untested). What do you think?
    Thanks,

    M.

    diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
    index 15a6c98ee92f..2f1b156021a6 100644
    --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
    +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
    @@ -86,7 +86,7 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
    }
    break;
    case GICD_TYPER2:
    - if (kvm_vgic_global_state.has_gicv4_1)
    + if (kvm_vgic_global_state.has_gicv4_1 && gic_cpuif_has_vsgi())
    value = GICD_TYPER2_nASSGIcap;
    break;
    case GICD_IIDR:
    @@ -119,7 +119,7 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
    dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;

    /* Not a GICv4.1? No HW SGIs */
    - if (!kvm_vgic_global_state.has_gicv4_1)
    + if (!kvm_vgic_global_state.has_gicv4_1 || !gic_cpuif_has_vsgi())
    val &= ~GICD_CTLR_nASSGIreq;

    /* Dist stays enabled? nASSGIreq is RO */
    diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
    index 5d1dc9915272..864fa9bbda4c 100644
    --- a/drivers/irqchip/irq-gic-v4.c
    +++ b/drivers/irqchip/irq-gic-v4.c
    @@ -87,17 +87,37 @@ static struct irq_domain *gic_domain;
    static const struct irq_domain_ops *vpe_domain_ops;
    static const struct irq_domain_ops *sgi_domain_ops;

    +#ifdef CONFIG_ARM64
    +#include <asm/cpufeature.h>
    +
    +bool gic_cpuif_has_vsgi(void)
    +{
    + unsigned long fld, reg = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
    +
    + fld = cpuid_feature_extract_unsigned_field(reg, ID_AA64PFR0_GIC_SHIFT);
    +
    + return fld >= 0x3;
    +}
    +#else
    +bool gic_cpuif_has_vsgi(void) { }
    +#endif
    +
    static bool has_v4_1(void)
    {
    return !!sgi_domain_ops;
    }

    +static bool has_v4_1_sgi(void)
    +{
    + return has_v4_1() && gic_cpuif_has_vsgi();
    +}
    +
    static int its_alloc_vcpu_sgis(struct its_vpe *vpe, int idx)
    {
    char *name;
    int sgi_base;

    - if (!has_v4_1())
    + if (!has_v4_1_sgi())
    return 0;

    name = kasprintf(GFP_KERNEL, "GICv4-sgi-%d", task_pid_nr(current));
    @@ -182,7 +202,7 @@ static void its_free_sgi_irqs(struct its_vm *vm)
    {
    int i;

    - if (!has_v4_1())
    + if (!has_v4_1_sgi())
    return;

    for (i = 0; i < vm->nr_vpes; i++) {
    diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
    index 943c3411ca10..2c63375bbd43 100644
    --- a/include/linux/irqchip/arm-gic-v4.h
    +++ b/include/linux/irqchip/arm-gic-v4.h
    @@ -145,4 +145,6 @@ int its_init_v4(struct irq_domain *domain,
    const struct irq_domain_ops *vpe_ops,
    const struct irq_domain_ops *sgi_ops);

    +bool gic_cpuif_has_vsgi(void);
    +
    #endif
    --
    Without deviation from the norm, progress is not possible.

    \
     
     \ /
      Last update: 2021-03-08 20:24    [W:6.060 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site