Re: [PATCH 09/31] powerpc/pseries/pci: Add a msi_free() handler to clear XIVE data

["Cédric Le Goater" <clg@kaod.org>]

Adding Marc.

On 4/30/21 10:03 AM, Cédric Le Goater wrote:
> The MSI domain clears the IRQ with msi_domain_free(), which calls
> irq_domain_free_irqs_top(), which clears the handler data. This is a
> problem for the XIVE controller since we need to unmap MMIO pages and
> free a specific XIVE structure.
> 
> The 'msi_free()' handler is called before irq_domain_free_irqs_top()
> when the handler data is still available. Use that to clear the XIVE
> controller data.
This feels like a clumsy way of doing so. 

irq_domain_free_irqs_parent() would be my preferred way to clear the 
lowlevel handler data but we can't today. Could there be a smarter way ?

Thanks,

C.


> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> ---
>  arch/powerpc/include/asm/xive.h      |  1 +
>  arch/powerpc/platforms/pseries/msi.c | 16 +++++++++++++++-
>  arch/powerpc/sysdev/xive/common.c    |  5 ++++-
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index aa094a8655b0..20ae50ab083c 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -111,6 +111,7 @@ void xive_native_free_vp_block(u32 vp_base);
>  int xive_native_populate_irq_data(u32 hw_irq,
>  				  struct xive_irq_data *data);
>  void xive_cleanup_irq_data(struct xive_irq_data *xd);
> +void xive_irq_free_data(unsigned int virq);
>  void xive_native_free_irq(u32 irq);
>  int xive_native_configure_irq(u32 hw_irq, u32 target, u8 prio, u32 sw_irq);
>  
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index a41c448520d4..da9d63a088bb 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -529,6 +529,19 @@ static int pseries_msi_ops_prepare(struct irq_domain *domain, struct device *dev
>  	return rtas_prepare_msi_irqs(pdev, nvec, type, arg);
>  }
>  
> +/*
> + * ->msi_free() is called before irq_domain_free_irqs_top() when the
> + * handler data is still available. Use that to clear the XIVE
> + * controller data.
> + */
> +static void pseries_msi_ops_msi_free(struct irq_domain *domain,
> +				     struct msi_domain_info *info,
> +				     unsigned int irq)
> +{
> +	if (xive_enabled())
> +		xive_irq_free_data(irq);
> +}
> +
>  /*
>   * RTAS can not disable one MSI at a time. It's all or nothing. Do it
>   * at the end after all IRQs have been freed.
> @@ -546,6 +559,7 @@ static void pseries_msi_domain_free_irqs(struct irq_domain *domain,
>  
>  static struct msi_domain_ops pseries_pci_msi_domain_ops = {
>  	.msi_prepare	= pseries_msi_ops_prepare,
> +	.msi_free	= pseries_msi_ops_msi_free,
>  	.domain_free_irqs = pseries_msi_domain_free_irqs,
>  };
>  
> @@ -660,7 +674,7 @@ static void pseries_irq_domain_free(struct irq_domain *domain, unsigned int virq
>  
>  	pr_debug("%s bridge %pOF %d #%d\n", __func__, phb->dn, virq, nr_irqs);
>  
> -	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +	/* XIVE domain data is cleared through ->msi_free() */
>  }
>  
>  static const struct irq_domain_ops pseries_irq_domain_ops = {
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 3485baf9ec8c..191cd80ec534 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -980,6 +980,8 @@ EXPORT_SYMBOL_GPL(is_xive_irq);
>  
>  void xive_cleanup_irq_data(struct xive_irq_data *xd)
>  {
> +	pr_debug("%s for HW %x\n", __func__, xd->hw_irq);
> +
>  	if (xd->eoi_mmio) {
>  		unmap_kernel_range((unsigned long)xd->eoi_mmio,
>  				   1u << xd->esb_shift);
> @@ -1025,7 +1027,7 @@ static int xive_irq_alloc_data(unsigned int virq, irq_hw_number_t hw)
>  	return 0;
>  }
>  
> -static void xive_irq_free_data(unsigned int virq)
> +void xive_irq_free_data(unsigned int virq)
>  {
>  	struct xive_irq_data *xd = irq_get_handler_data(virq);
>  
> @@ -1035,6 +1037,7 @@ static void xive_irq_free_data(unsigned int virq)
>  	xive_cleanup_irq_data(xd);
>  	kfree(xd);
>  }
> +EXPORT_SYMBOL_GPL(xive_irq_free_data);
>  
>  #ifdef CONFIG_SMP
>  
>