Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1382724 > unrolled thread

[PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:20 +0200
    Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and  related callback Marc Zyngier <marc.zyngier@arm.com> - 2016-04-20 11:20 +0200
      Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and  related callback Eric Auger <eric.auger@linaro.org> - 2016-04-20 11:40 +0200

#1382724 — [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback

The purpose is to be able to retrieve the MSI doorbells of an irqchip.
This is now needed since on some platforms those doorbells must be
iommu mapped (in case the MSIs transit through an IOMMU that do not
bypass those transactions).

The assumption is there is a maximum of one doorbell region per cpu.
The number of doorbells for the whole irqchip is stored in nb_doorbells.

A doorbell region is characterized by its physical address base, size and
IOMMU protection flag.

irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
the irqchip.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v7: creation
---
 include/linux/irq.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de623..fdad8c1 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
-/**
- * struct irq_chip - hardware interrupt chip descriptor
- *
+/* MSI doorbell region */
+struct irq_chip_msi_doorbell {
+	phys_addr_t base;
+	size_t size;
+	int prot; /* iommu protection flag */
+};
+
+/*
+ * Describe all the MSI doorbell regions for an irqchip.
+ * A single doorbell region per cpu is assumed.
+ * In case a single doorbell is supported for the whole irqchip,
+ * the region is described in as cpu #0's one
+ */
+struct irq_chip_msi_doorbell_info {
+	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
+	int nb_doorbells; /* overall number of doorbells */
+};
+
+/** * struct irq_chip - hardware interrupt chip descriptor *
  * @name:		name for /proc/interrupts
  * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
  * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
@@ -349,6 +365,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @irq_get_irqchip_state:	return the internal state of an interrupt
  * @irq_set_irqchip_state:	set the internal state of a interrupt
  * @irq_set_vcpu_affinity:	optional to target a vCPU in a virtual machine
+ * @msi_doorbell_info:	return the MSI doorbell info
  * @ipi_send_single:	send a single IPI to destination cpus
  * @ipi_send_mask:	send an IPI to destination cpus in cpumask
  * @flags:		chip specific flags
@@ -394,7 +411,8 @@ struct irq_chip {
 	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
 
 	int		(*irq_set_vcpu_affinity)(struct irq_data *data, void *vcpu_info);
-
+	const struct irq_chip_msi_doorbell_info *(*msi_doorbell_info)(
+							struct irq_data *data);
 	void		(*ipi_send_single)(struct irq_data *data, unsigned int cpu);
 	void		(*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest);
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1383187 — Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback

On 19/04/16 18:13, Eric Auger wrote:
> The purpose is to be able to retrieve the MSI doorbells of an irqchip.
> This is now needed since on some platforms those doorbells must be
> iommu mapped (in case the MSIs transit through an IOMMU that do not
> bypass those transactions).
> 
> The assumption is there is a maximum of one doorbell region per cpu.
> The number of doorbells for the whole irqchip is stored in nb_doorbells.
> 
> A doorbell region is characterized by its physical address base, size and
> IOMMU protection flag.
> 
> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
> the irqchip.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v7: creation
> ---
>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de623..fdad8c1 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  	return d->hwirq;
>  }
>  
> -/**
> - * struct irq_chip - hardware interrupt chip descriptor
> - *
> +/* MSI doorbell region */
> +struct irq_chip_msi_doorbell {
> +	phys_addr_t base;
> +	size_t size;
> +	int prot; /* iommu protection flag */

I find this one a bit scary. "int" is a probably not the right type if
it is a set of flags (it should describe both the protection and the
memory attributes - in this case, probably something like Device +
Writeable). You should probably use the same type the IOMMU code uses
(and if it is actually an int, then I'll shut up...).

> +};
> +
> +/*
> + * Describe all the MSI doorbell regions for an irqchip.
> + * A single doorbell region per cpu is assumed.
> + * In case a single doorbell is supported for the whole irqchip,
> + * the region is described in as cpu #0's one
> + */
> +struct irq_chip_msi_doorbell_info {
> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
> +	int nb_doorbells; /* overall number of doorbells */
> +};

How can size and prot be different from one CPU to another? It really
feels like they should be common. Can I suggest something like this?

struct irq_chip_msi_doorbell_info {
	phys_addr_t __percpu	*doorbells;
	size_t			size;
	u32			prot;
};

and get rid of struct irq_chip_msi_doorbell altogether?

> +
> +/** * struct irq_chip - hardware interrupt chip descriptor *
>   * @name:		name for /proc/interrupts
>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
> @@ -349,6 +365,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @irq_get_irqchip_state:	return the internal state of an interrupt
>   * @irq_set_irqchip_state:	set the internal state of a interrupt
>   * @irq_set_vcpu_affinity:	optional to target a vCPU in a virtual machine
> + * @msi_doorbell_info:	return the MSI doorbell info
>   * @ipi_send_single:	send a single IPI to destination cpus
>   * @ipi_send_mask:	send an IPI to destination cpus in cpumask
>   * @flags:		chip specific flags
> @@ -394,7 +411,8 @@ struct irq_chip {
>  	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
>  
>  	int		(*irq_set_vcpu_affinity)(struct irq_data *data, void *vcpu_info);
> -
> +	const struct irq_chip_msi_doorbell_info *(*msi_doorbell_info)(
> +							struct irq_data *data);
>  	void		(*ipi_send_single)(struct irq_data *data, unsigned int cpu);
>  	void		(*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest);
>  
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1383204 — Re: [PATCH v7 5/8] genirq/irq: introduce msi_doorbell's structs and related callback

Hi Marc,
On 04/20/2016 11:16 AM, Marc Zyngier wrote:
> On 19/04/16 18:13, Eric Auger wrote:
>> The purpose is to be able to retrieve the MSI doorbells of an irqchip.
>> This is now needed since on some platforms those doorbells must be
>> iommu mapped (in case the MSIs transit through an IOMMU that do not
>> bypass those transactions).
>>
>> The assumption is there is a maximum of one doorbell region per cpu.
>> The number of doorbells for the whole irqchip is stored in nb_doorbells.
>>
>> A doorbell region is characterized by its physical address base, size and
>> IOMMU protection flag.
>>
>> irq_chip msi_doorbell_info callback enables to retrieve the doorbells of
>> the irqchip.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v7: creation
>> ---
>>  include/linux/irq.h | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index c4de623..fdad8c1 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -312,9 +312,25 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  	return d->hwirq;
>>  }
>>  
>> -/**
>> - * struct irq_chip - hardware interrupt chip descriptor
>> - *
>> +/* MSI doorbell region */
>> +struct irq_chip_msi_doorbell {
>> +	phys_addr_t base;
>> +	size_t size;
>> +	int prot; /* iommu protection flag */
> 
> I find this one a bit scary. "int" is a probably not the right type if
> it is a set of flags (it should describe both the protection and the
> memory attributes - in this case, probably something like Device +
> Writeable). You should probably use the same type the IOMMU code uses
> (and if it is actually an int, then I'll shut up...).
Hum yes iommu also uses an int ;-)
> 
>> +};
>> +
>> +/*
>> + * Describe all the MSI doorbell regions for an irqchip.
>> + * A single doorbell region per cpu is assumed.
>> + * In case a single doorbell is supported for the whole irqchip,
>> + * the region is described in as cpu #0's one
>> + */
>> +struct irq_chip_msi_doorbell_info {
>> +	struct irq_chip_msi_doorbell __percpu *percpu_doorbells;
>> +	int nb_doorbells; /* overall number of doorbells */
>> +};
> 
> How can size and prot be different from one CPU to another? It really
> feels like they should be common. Can I suggest something like this?
> 
> struct irq_chip_msi_doorbell_info {
> 	phys_addr_t __percpu	*doorbells;
> 	size_t			size;
> 	u32			prot;
> };
> 
> and get rid of struct irq_chip_msi_doorbell altogether?
I'am definitively fine with your proposal.

Thanks

Eric
> 
>> +
>> +/** * struct irq_chip - hardware interrupt chip descriptor *
>>   * @name:		name for /proc/interrupts
>>   * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
>> @@ -349,6 +365,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   * @irq_get_irqchip_state:	return the internal state of an interrupt
>>   * @irq_set_irqchip_state:	set the internal state of a interrupt
>>   * @irq_set_vcpu_affinity:	optional to target a vCPU in a virtual machine
>> + * @msi_doorbell_info:	return the MSI doorbell info
>>   * @ipi_send_single:	send a single IPI to destination cpus
>>   * @ipi_send_mask:	send an IPI to destination cpus in cpumask
>>   * @flags:		chip specific flags
>> @@ -394,7 +411,8 @@ struct irq_chip {
>>  	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
>>  
>>  	int		(*irq_set_vcpu_affinity)(struct irq_data *data, void *vcpu_info);
>> -
>> +	const struct irq_chip_msi_doorbell_info *(*msi_doorbell_info)(
>> +							struct irq_data *data);
>>  	void		(*ipi_send_single)(struct irq_data *data, unsigned int cpu);
>>  	void		(*ipi_send_mask)(struct irq_data *data, const struct cpumask *dest);
>>  
>>
> 
> Thanks,
> 
> 	M.
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web