Re: [PATCH 1/2] irqchip/gic-v4.1: Add support for VPENDBASER's Dirty+Valid signaling

[Zenghui Yu]

Hi Marc,

On 2020/4/14 17:50, Marc Zyngier wrote:
> When a vPE is made resident, the GIC starts parsing the virtual pending
> table to deliver pending interrupts. This takes place asynchronously,
> and can at times take a long while. Long enough that the vcpu enters
> the guest and hits WFI before any interrupt has been signaled yet.
> The vcpu then exits, blocks, and now gets a doorbell. Rince, repeat.
>
> In order to avoid the above, a (optional on GICv4, mandatory on v4.1)
> feature allows the GIC to feedback to the hypervisor whether it is
> done parsing the VPT by clearing the GICR_VENPENDBASER.Dirty bit.

GICR_VPENDBASER

> The hypervisor can then wait until the GIC is ready before actually
> running the vPE.
>
> Plug the detection code as well as polling on vPE schedule. While
> at it, tidy-up the kernel message that displays the GICv4 optional
> features.
>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
>   drivers/irqchip/irq-gic-v3-its.c   | 19 +++++++++++++++++++
>   drivers/irqchip/irq-gic-v3.c       | 11 +++++++----
>   include/linux/irqchip/arm-gic-v3.h |  2 ++
>   3 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 54d142ccc63a..e24a145f17f6 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -14,6 +14,7 @@
>   #include <linux/dma-iommu.h>
>   #include <linux/efi.h>
>   #include <linux/interrupt.h>
> +#include <linux/iopoll.h>
>   #include <linux/irqdomain.h>
>   #include <linux/list.h>
>   #include <linux/log2.h>
> @@ -3672,6 +3673,20 @@ static int its_vpe_set_affinity(struct irq_data *d,
>   	return IRQ_SET_MASK_OK_DONE;
>   }
>   
> +static void its_wait_vpend_dirty_clear(void)

I'm not sure if we can give it a better name. As we will also wait the
Dirty bit to be cleared in a de-scheduling operation, while now this
function is only used in a vPE schedule. To me, it should be something
like 'its_wait_vpt_parse_complete' (but someone may argue that in a vPE
de-schedule, there is also a VPT parsing to calculate the PendingLast).

Either way,
Reviewed-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>


Thanks.

> +{
> +	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> +	u64 val;
> +
> +	if (!gic_rdists->has_vpend_valid_dirty)
> +		return;
> +
> +	WARN_ON_ONCE(readq_relaxed_poll_timeout(vlpi_base + GICR_VPENDBASER,
> +						val,
> +						!(val & GICR_VPENDBASER_Dirty),
> +						10, 500));
> +}
> +
>   static void its_vpe_schedule(struct its_vpe *vpe)
>   {
>   	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> @@ -3702,6 +3717,8 @@ static void its_vpe_schedule(struct its_vpe *vpe)
>   	val |= vpe->idai ? GICR_VPENDBASER_IDAI : 0;
>   	val |= GICR_VPENDBASER_Valid;
>   	gicr_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
> +
> +	its_wait_vpend_dirty_clear();
>   }
>   
>   static void its_vpe_deschedule(struct its_vpe *vpe)
> @@ -3910,6 +3927,8 @@ static void its_vpe_4_1_schedule(struct its_vpe *vpe,
>   	val |= FIELD_PREP(GICR_VPENDBASER_4_1_VPEID, vpe->vpe_id);
>   
>   	gicr_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
> +
> +	its_wait_vpend_dirty_clear();
>   }
>   
>   static void its_vpe_4_1_deschedule(struct its_vpe *vpe,
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 9dbc81b6f62e..d7006ef18a0d 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -873,6 +873,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
>   	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
>   	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
>   					   gic_data.rdists.has_rvpeid);
> +	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
>   
>   	/* Detect non-sensical configurations */
>   	if (WARN_ON_ONCE(gic_data.rdists.has_rvpeid && !gic_data.rdists.has_vlpis)) {
> @@ -893,10 +894,11 @@ static void gic_update_rdist_properties(void)
>   	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
>   		gic_data.ppi_nr = 0;
>   	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
> -	pr_info("%sVLPI support, %sdirect LPI support, %sRVPEID support\n",
> -		!gic_data.rdists.has_vlpis ? "no " : "",
> -		!gic_data.rdists.has_direct_lpi ? "no " : "",
> -		!gic_data.rdists.has_rvpeid ? "no " : "");
> +	if (gic_data.rdists.has_vlpis)
> +		pr_info("GICv4 features: %s%s%s\n",
> +			gic_data.rdists.has_direct_lpi ? "DirectLPI " : "",
> +			gic_data.rdists.has_rvpeid ? "RVPEID " : "",
> +			gic_data.rdists.has_vpend_valid_dirty ? "Valid+Dirty " : "");
>   }
>   
>   /* Check whether it's single security state view */
> @@ -1620,6 +1622,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
>   	gic_data.rdists.has_rvpeid = true;
>   	gic_data.rdists.has_vlpis = true;
>   	gic_data.rdists.has_direct_lpi = true;
> +	gic_data.rdists.has_vpend_valid_dirty = true;
>   
>   	if (WARN_ON(!gic_data.domain) || WARN_ON(!gic_data.rdists.rdist)) {
>   		err = -ENOMEM;
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 765d9b769b69..6c36b6cc3edf 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -243,6 +243,7 @@
>   
>   #define GICR_TYPER_PLPIS		(1U << 0)
>   #define GICR_TYPER_VLPIS		(1U << 1)
> +#define GICR_TYPER_DIRTY		(1U << 2)
>   #define GICR_TYPER_DirectLPIS		(1U << 3)
>   #define GICR_TYPER_LAST			(1U << 4)
>   #define GICR_TYPER_RVPEID		(1U << 7)
> @@ -686,6 +687,7 @@ struct rdists {
>   	bool			has_vlpis;
>   	bool			has_rvpeid;
>   	bool			has_direct_lpi;
> +	bool			has_vpend_valid_dirty;
>   };
>   
>   struct irq_domain;