LinuxLists.cc - [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:

> From: Jiang Liu <[email protected]>
>
> Introduce callback irq_chip.irq_write_msi_msg, so we can share common
> code among MSI alike interrupt controllers, such as HPET and DMAR.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Yingjoe Chen <[email protected]>
> Cc: Yijing Wang <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> include/linux/irq.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> Index: tip/include/linux/irq.h
> ===================================================================
> --- tip.orig/include/linux/irq.h
> +++ tip/include/linux/irq.h
> @@ -322,6 +322,7 @@ static inline irq_hw_number_t irqd_to_hw
> * @irq_release_resources: optional to release resources acquired with
> * irq_request_resources
> * @irq_compose_msi_msg: optional to compose message content for MSI
> + * @irq_write_msi_msg: optional to write message content for MSI
> * @flags: chip specific flags
> */
> struct irq_chip {
> @@ -359,6 +360,7 @@ struct irq_chip {
> void (*irq_release_resources)(struct irq_data *data);
>
> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);

Hmm... It's really weird.
I don't think it's the interrupt controllers' responsibility to write messages
for all the endpoint devices since the methods of configuring message registers
may different between these devices. And theoretically, the endpoint devices
themselves should take the responsibility to configure their message registers.
To say the least, the write_msg callback here still need to call some certain
interfaces provided by the corresponding device.
There would be lots of ARM new devices capable of sending message based interrupts
to interrupt controllers, does all the drivers of the devices need to expose a
write_msg callback to interrupt controllers?

Regards,
Abel

>
> unsigned long flags;
> };
> @@ -453,6 +455,12 @@ extern void irq_chip_ack_parent(struct i
> extern int irq_chip_retrigger_hierarchy(struct irq_data *data);
> #endif
>
> +static inline void irq_chip_write_msi_msg(struct irq_data *data,
> + struct msi_msg *msg)
> +{
> + data->chip->irq_write_msi_msg(data, msg);
> +}
> +
> /* Handling of unhandled and spurious interrupts: */
> extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
> irqreturn_t action_ret);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> .
>
>

2014-11-18 10:19:13

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
> On 2014/11/12 21:43, Thomas Gleixner wrote:
> > struct irq_chip {
> > @@ -359,6 +360,7 @@ struct irq_chip {
> > void (*irq_release_resources)(struct irq_data *data);
> >
> > void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
> > + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>
> Hmm... It's really weird.
> I don't think it's the interrupt controllers' responsibility to write messages
> for all the endpoint devices since the methods of configuring message registers
> may different between these devices. And theoretically, the endpoint devices
> themselves should take the responsibility to configure their message registers.
> To say the least, the write_msg callback here still need to call some certain
> interfaces provided by the corresponding device.
>
> There would be lots of ARM new devices capable of sending message
> based interrupts to interrupt controllers, does all the drivers of
> the devices need to expose a write_msg callback to interrupt
> controllers?

Well, writing the message _IS_ part of the interrupt controller.

So in order to enable non PCI based MSI we want to have generic
infrastructure with minimal per device/device class callbacks and of
course you need to provide that callback for your special device.

We already have non PCI based MSI controllers in x86 today and we need
to handle the whole stuff with tons of copied coded extra for each of
those. So consolidating it into common infrastructure allows us to get
rid of the pointless copied code and reduce the per device effort to
the relevant hardware specific callbacks. irq_write_msi_msg being one
of those.

Thanks,

tglx

2014-11-18 13:33:35

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 18:19, Thomas Gleixner wrote:

> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>> struct irq_chip {
>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>> void (*irq_release_resources)(struct irq_data *data);
>>>
>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>
>> Hmm... It's really weird.
>> I don't think it's the interrupt controllers' responsibility to write messages
>> for all the endpoint devices since the methods of configuring message registers
>> may different between these devices. And theoretically, the endpoint devices
>> themselves should take the responsibility to configure their message registers.
>> To say the least, the write_msg callback here still need to call some certain
>> interfaces provided by the corresponding device.
>>
>> There would be lots of ARM new devices capable of sending message
>> based interrupts to interrupt controllers, does all the drivers of
>> the devices need to expose a write_msg callback to interrupt
>> controllers?
>
> Well, writing the message _IS_ part of the interrupt controller.
>
> So in order to enable non PCI based MSI we want to have generic
> infrastructure with minimal per device/device class callbacks and of
> course you need to provide that callback for your special device.
>
> We already have non PCI based MSI controllers in x86 today and we need
> to handle the whole stuff with tons of copied coded extra for each of
> those. So consolidating it into common infrastructure allows us to get
> rid of the pointless copied code and reduce the per device effort to
> the relevant hardware specific callbacks. irq_write_msi_msg being one
> of those.
>

At least, we have the same goal.
I will illustrate my thoughts by an example.
The current code is something like:

Device A
========
void A_write_msg() { ... }

Group B
(a group of devices behave same on writing messages, i.e. PCI)
=======
void B_write_msg() { ... }

Controller
==========
irq_chip.irq_write_msi_msg () {
if (A)
A_write_msg();
if (B)
B_write_msg();
}

It's horrible when new devices come out, since we need to modify the
controller part for each new device.
What I suggested is:

MSI Core
========
struct msi_ops { .write_msg, };
struct msi_desc { .msi_ops, };

write_msg() {
X = get_dev();
irq_chip.compose_msg(X); // IRQ chips' responsibility
X_msi_ops.write_msg(); // nothing to do with IRQ chips
}

Device A
========
void A_write_msg() { ... }
A_msi_ops.write_msg = A_write_msg;

Group B
=======
void B_write_msg() { ... }
B_msi_ops.write_msg = B_write_msg;

Please correct me if I misunderstood anything.

Thanks,
Abel

2014-11-18 13:43:53

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 21:33, Yun Wu (Abel) wrote:
> On 2014/11/18 18:19, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>> struct irq_chip {
>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>
>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>
>>> Hmm... It's really weird.
>>> I don't think it's the interrupt controllers' responsibility to write messages
>>> for all the endpoint devices since the methods of configuring message registers
>>> may different between these devices. And theoretically, the endpoint devices
>>> themselves should take the responsibility to configure their message registers.
>>> To say the least, the write_msg callback here still need to call some certain
>>> interfaces provided by the corresponding device.
>>>
>>> There would be lots of ARM new devices capable of sending message
>>> based interrupts to interrupt controllers, does all the drivers of
>>> the devices need to expose a write_msg callback to interrupt
>>> controllers?
>>
>> Well, writing the message _IS_ part of the interrupt controller.
>>
>> So in order to enable non PCI based MSI we want to have generic
>> infrastructure with minimal per device/device class callbacks and of
>> course you need to provide that callback for your special device.
>>
>> We already have non PCI based MSI controllers in x86 today and we need
>> to handle the whole stuff with tons of copied coded extra for each of
>> those. So consolidating it into common infrastructure allows us to get
>> rid of the pointless copied code and reduce the per device effort to
>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>> of those.
>>
>
> At least, we have the same goal.
> I will illustrate my thoughts by an example.
> The current code is something like:
>
> Device A
> ========
> void A_write_msg() { ... }
>
> Group B
> (a group of devices behave same on writing messages, i.e. PCI)
> =======
> void B_write_msg() { ... }
>
> Controller
> ==========
> irq_chip.irq_write_msi_msg () {
> if (A)
> A_write_msg();
> if (B)
> B_write_msg();
> }
>
> It's horrible when new devices come out, since we need to modify the
> controller part for each new device.
> What I suggested is:
>
> MSI Core
> ========
> struct msi_ops { .write_msg, };
> struct msi_desc { .msi_ops, };
>
> write_msg() {
> X = get_dev();
> irq_chip.compose_msg(X); // IRQ chips' responsibility
> X_msi_ops.write_msg(); // nothing to do with IRQ chips
> }
>
> Device A
> ========
> void A_write_msg() { ... }
> A_msi_ops.write_msg = A_write_msg;
>
> Group B
> =======
> void B_write_msg() { ... }
> B_msi_ops.write_msg = B_write_msg;
>
> Please correct me if I misunderstood anything.
Hi Yun,
We provide an irq_chip for each type of interrupt controller
instead of devices. For the example mentioned above, if device A
and Group B has different interrupt controllers, we just need to
implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
to suitable callbacks.
The framework already achieves what you you want:)
Regards!
Gerry
>
> Thanks,
> Abel
>

2014-11-18 13:51:46

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 21:33, Yun Wu (Abel) wrote:
> On 2014/11/18 18:19, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>> struct irq_chip {
>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>
>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>
>>> Hmm... It's really weird.
>>> I don't think it's the interrupt controllers' responsibility to write messages
>>> for all the endpoint devices since the methods of configuring message registers
>>> may different between these devices. And theoretically, the endpoint devices
>>> themselves should take the responsibility to configure their message registers.
>>> To say the least, the write_msg callback here still need to call some certain
>>> interfaces provided by the corresponding device.
>>>
>>> There would be lots of ARM new devices capable of sending message
>>> based interrupts to interrupt controllers, does all the drivers of
>>> the devices need to expose a write_msg callback to interrupt
>>> controllers?
>>
>> Well, writing the message _IS_ part of the interrupt controller.
>>
>> So in order to enable non PCI based MSI we want to have generic
>> infrastructure with minimal per device/device class callbacks and of
>> course you need to provide that callback for your special device.
>>
>> We already have non PCI based MSI controllers in x86 today and we need
>> to handle the whole stuff with tons of copied coded extra for each of
>> those. So consolidating it into common infrastructure allows us to get
>> rid of the pointless copied code and reduce the per device effort to
>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>> of those.
>>
>
> At least, we have the same goal.
> I will illustrate my thoughts by an example.
> The current code is something like:
>
> Device A
> ========
> void A_write_msg() { ... }
>
> Group B
> (a group of devices behave same on writing messages, i.e. PCI)
> =======
> void B_write_msg() { ... }
>
> Controller
> ==========
> irq_chip.irq_write_msi_msg () {
> if (A)
> A_write_msg();
> if (B)
> B_write_msg();
> }
>
> It's horrible when new devices come out, since we need to modify the
> controller part for each new device.
> What I suggested is:
>
> MSI Core
> ========
> struct msi_ops { .write_msg, };
> struct msi_desc { .msi_ops, };
>
> write_msg() {
> X = get_dev();
> irq_chip.compose_msg(X); // IRQ chips' responsibility
> X_msi_ops.write_msg(); // nothing to do with IRQ chips
> }
>
> Device A
> ========
> void A_write_msg() { ... }
> A_msi_ops.write_msg = A_write_msg;
>
> Group B
> =======
> void B_write_msg() { ... }
> B_msi_ops.write_msg = B_write_msg;
>
> Please correct me if I misunderstood anything.
Please take a look at following file, which uses the common MSI
framework to support PCI, DMAR and HPET interrupt on x86.
https://github.com/jiangliu/linux/blob/irqdomain/p2v7/arch/x86/kernel/apic/msi.c
Regards!
Gerry

>
> Thanks,
> Abel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2014-11-18 13:53:11

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 21:43, Jiang Liu wrote:

> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>> struct irq_chip {
>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>>
>>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>
>>>> Hmm... It's really weird.
>>>> I don't think it's the interrupt controllers' responsibility to write messages
>>>> for all the endpoint devices since the methods of configuring message registers
>>>> may different between these devices. And theoretically, the endpoint devices
>>>> themselves should take the responsibility to configure their message registers.
>>>> To say the least, the write_msg callback here still need to call some certain
>>>> interfaces provided by the corresponding device.
>>>>
>>>> There would be lots of ARM new devices capable of sending message
>>>> based interrupts to interrupt controllers, does all the drivers of
>>>> the devices need to expose a write_msg callback to interrupt
>>>> controllers?
>>>
>>> Well, writing the message _IS_ part of the interrupt controller.
>>>
>>> So in order to enable non PCI based MSI we want to have generic
>>> infrastructure with minimal per device/device class callbacks and of
>>> course you need to provide that callback for your special device.
>>>
>>> We already have non PCI based MSI controllers in x86 today and we need
>>> to handle the whole stuff with tons of copied coded extra for each of
>>> those. So consolidating it into common infrastructure allows us to get
>>> rid of the pointless copied code and reduce the per device effort to
>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>> of those.
>>>
>>
>> At least, we have the same goal.
>> I will illustrate my thoughts by an example.
>> The current code is something like:
>>
>> Device A
>> ========
>> void A_write_msg() { ... }
>>
>> Group B
>> (a group of devices behave same on writing messages, i.e. PCI)
>> =======
>> void B_write_msg() { ... }
>>
>> Controller
>> ==========
>> irq_chip.irq_write_msi_msg () {
>> if (A)
>> A_write_msg();
>> if (B)
>> B_write_msg();
>> }
>>
>> It's horrible when new devices come out, since we need to modify the
>> controller part for each new device.
>> What I suggested is:
>>
>> MSI Core
>> ========
>> struct msi_ops { .write_msg, };
>> struct msi_desc { .msi_ops, };
>>
>> write_msg() {
>> X = get_dev();
>> irq_chip.compose_msg(X); // IRQ chips' responsibility
>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>> }
>>
>> Device A
>> ========
>> void A_write_msg() { ... }
>> A_msi_ops.write_msg = A_write_msg;
>>
>> Group B
>> =======
>> void B_write_msg() { ... }
>> B_msi_ops.write_msg = B_write_msg;
>>
>> Please correct me if I misunderstood anything.
> Hi Yun,
> We provide an irq_chip for each type of interrupt controller
> instead of devices. For the example mentioned above, if device A
> and Group B has different interrupt controllers, we just need to
> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
> to suitable callbacks.
> The framework already achieves what you you want:)

What if device A and group B have the same interrupt controller?

Regards,
Abel

2014-11-18 14:04:12

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 21:52, Yun Wu (Abel) wrote:
> On 2014/11/18 21:43, Jiang Liu wrote:
>
>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>
>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>> struct irq_chip {
>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>>>
>>>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>
>>>>> Hmm... It's really weird.
>>>>> I don't think it's the interrupt controllers' responsibility to write messages
>>>>> for all the endpoint devices since the methods of configuring message registers
>>>>> may different between these devices. And theoretically, the endpoint devices
>>>>> themselves should take the responsibility to configure their message registers.
>>>>> To say the least, the write_msg callback here still need to call some certain
>>>>> interfaces provided by the corresponding device.
>>>>>
>>>>> There would be lots of ARM new devices capable of sending message
>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>> the devices need to expose a write_msg callback to interrupt
>>>>> controllers?
>>>>
>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>
>>>> So in order to enable non PCI based MSI we want to have generic
>>>> infrastructure with minimal per device/device class callbacks and of
>>>> course you need to provide that callback for your special device.
>>>>
>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>> those. So consolidating it into common infrastructure allows us to get
>>>> rid of the pointless copied code and reduce the per device effort to
>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>> of those.
>>>>
>>>
>>> At least, we have the same goal.
>>> I will illustrate my thoughts by an example.
>>> The current code is something like:
>>>
>>> Device A
>>> ========
>>> void A_write_msg() { ... }
>>>
>>> Group B
>>> (a group of devices behave same on writing messages, i.e. PCI)
>>> =======
>>> void B_write_msg() { ... }
>>>
>>> Controller
>>> ==========
>>> irq_chip.irq_write_msi_msg () {
>>> if (A)
>>> A_write_msg();
>>> if (B)
>>> B_write_msg();
>>> }
>>>
>>> It's horrible when new devices come out, since we need to modify the
>>> controller part for each new device.
>>> What I suggested is:
>>>
>>> MSI Core
>>> ========
>>> struct msi_ops { .write_msg, };
>>> struct msi_desc { .msi_ops, };
>>>
>>> write_msg() {
>>> X = get_dev();
>>> irq_chip.compose_msg(X); // IRQ chips' responsibility
>>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>>> }
>>>
>>> Device A
>>> ========
>>> void A_write_msg() { ... }
>>> A_msi_ops.write_msg = A_write_msg;
>>>
>>> Group B
>>> =======
>>> void B_write_msg() { ... }
>>> B_msi_ops.write_msg = B_write_msg;
>>>
>>> Please correct me if I misunderstood anything.
>> Hi Yun,
>> We provide an irq_chip for each type of interrupt controller
>> instead of devices. For the example mentioned above, if device A
>> and Group B has different interrupt controllers, we just need to
>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>> to suitable callbacks.
>> The framework already achieves what you you want:)
>
> What if device A and group B have the same interrupt controller?
Device doesn't care about interrupt controllers, it just cares about
interrupts used by itself. It's the interrupt source (controller)
enumerators' responsibility to discover interrupt source, associate
methods to control the interrupt source and assign irq numbers for
interrupt sources.
There are two ways to associate irq numbers with device:
1) arch code/bus drivers statically assigns irq number for devices
when creating device objects, such as PCI legacy interrupt
(INTA-INTD), IOAPIC interrupts.
2) device drivers ask interrupt source enumerators to dynamically
create irq numbers, such pci_enable_msix_range() and friends.
So device driver definitely doesn't need to known about irq_chip
methods to control interrupt sources.
Regards!
Gerry

2014-11-18 14:15:54

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:03, Jiang Liu wrote:
> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>> On 2014/11/18 21:43, Jiang Liu wrote:
>>
>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>> Hi Yun,
>>> We provide an irq_chip for each type of interrupt controller
>>> instead of devices. For the example mentioned above, if device A
>>> and Group B has different interrupt controllers, we just need to
>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>> to suitable callbacks.
>>> The framework already achieves what you you want:)
>>
>> What if device A and group B have the same interrupt controller?
> Device doesn't care about interrupt controllers, it just cares about
> interrupts used by itself. It's the interrupt source (controller)
> enumerators' responsibility to discover interrupt source, associate
> methods to control the interrupt source and assign irq numbers for
> interrupt sources.
> There are two ways to associate irq numbers with device:
> 1) arch code/bus drivers statically assigns irq number for devices
> when creating device objects, such as PCI legacy interrupt
> (INTA-INTD), IOAPIC interrupts.
> 2) device drivers ask interrupt source enumerators to dynamically
> create irq numbers, such pci_enable_msix_range() and friends.
> So device driver definitely doesn't need to known about irq_chip
> methods to control interrupt sources.
Please refer to
http://www.xisanqi.net/year/2014/pdf/2014-pdf-09-reconstruct-x86-interrupt-architecture.pdf
for more information
about the hierarchy irqdomain background:)

> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2014-11-18 14:19:56

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
> On 2014/11/18 21:43, Jiang Liu wrote:
> > We provide an irq_chip for each type of interrupt controller
> > instead of devices. For the example mentioned above, if device A
> > and Group B has different interrupt controllers, we just need to
> > implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
> > to suitable callbacks.
> > The framework already achieves what you you want:)
>
> What if device A and group B have the same interrupt controller?

Well, if write_msg() is different they are hardly the same.

Thanks,

tglx

2014-11-18 14:23:31

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:03, Jiang Liu wrote:

> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>> On 2014/11/18 21:43, Jiang Liu wrote:
>>
>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>>
>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>>> struct irq_chip {
>>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>>>>
>>>>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>
>>>>>> Hmm... It's really weird.
>>>>>> I don't think it's the interrupt controllers' responsibility to write messages
>>>>>> for all the endpoint devices since the methods of configuring message registers
>>>>>> may different between these devices. And theoretically, the endpoint devices
>>>>>> themselves should take the responsibility to configure their message registers.
>>>>>> To say the least, the write_msg callback here still need to call some certain
>>>>>> interfaces provided by the corresponding device.
>>>>>>
>>>>>> There would be lots of ARM new devices capable of sending message
>>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>>> the devices need to expose a write_msg callback to interrupt
>>>>>> controllers?
>>>>>
>>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>>
>>>>> So in order to enable non PCI based MSI we want to have generic
>>>>> infrastructure with minimal per device/device class callbacks and of
>>>>> course you need to provide that callback for your special device.
>>>>>
>>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>>> those. So consolidating it into common infrastructure allows us to get
>>>>> rid of the pointless copied code and reduce the per device effort to
>>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>>> of those.
>>>>>
>>>>
>>>> At least, we have the same goal.
>>>> I will illustrate my thoughts by an example.
>>>> The current code is something like:
>>>>
>>>> Device A
>>>> ========
>>>> void A_write_msg() { ... }
>>>>
>>>> Group B
>>>> (a group of devices behave same on writing messages, i.e. PCI)
>>>> =======
>>>> void B_write_msg() { ... }
>>>>
>>>> Controller
>>>> ==========
>>>> irq_chip.irq_write_msi_msg () {
>>>> if (A)
>>>> A_write_msg();
>>>> if (B)
>>>> B_write_msg();
>>>> }
>>>>
>>>> It's horrible when new devices come out, since we need to modify the
>>>> controller part for each new device.
>>>> What I suggested is:
>>>>
>>>> MSI Core
>>>> ========
>>>> struct msi_ops { .write_msg, };
>>>> struct msi_desc { .msi_ops, };
>>>>
>>>> write_msg() {
>>>> X = get_dev();
>>>> irq_chip.compose_msg(X); // IRQ chips' responsibility
>>>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>>>> }
>>>>
>>>> Device A
>>>> ========
>>>> void A_write_msg() { ... }
>>>> A_msi_ops.write_msg = A_write_msg;
>>>>
>>>> Group B
>>>> =======
>>>> void B_write_msg() { ... }
>>>> B_msi_ops.write_msg = B_write_msg;
>>>>
>>>> Please correct me if I misunderstood anything.
>>> Hi Yun,
>>> We provide an irq_chip for each type of interrupt controller
>>> instead of devices. For the example mentioned above, if device A
>>> and Group B has different interrupt controllers, we just need to
>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>> to suitable callbacks.
>>> The framework already achieves what you you want:)
>>
>> What if device A and group B have the same interrupt controller?
> Device doesn't care about interrupt controllers, it just cares about
> interrupts used by itself. It's the interrupt source (controller)
> enumerators' responsibility to discover interrupt source, associate
> methods to control the interrupt source and assign irq numbers for
> interrupt sources.

Yes, indeed.

> There are two ways to associate irq numbers with device:
> 1) arch code/bus drivers statically assigns irq number for devices
> when creating device objects, such as PCI legacy interrupt
> (INTA-INTD), IOAPIC interrupts.

And OF interfaces.

> 2) device drivers ask interrupt source enumerators to dynamically
> create irq numbers, such pci_enable_msix_range() and friends.
> So device driver definitely doesn't need to known about irq_chip
> methods to control interrupt sources.
>

The above you described is absolutely right, but not the things I want
to know. :)
Take GICv3 ITS for example, it deals with both PCI and non PCI message
interrupts. IIUC, several irq_chips need to be implemented in the ITS
driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
do to the ITS driver if new MSI-capable devices come out?

Regards,
Abel

2014-11-18 14:30:06

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:22, Yun Wu (Abel) wrote:
> On 2014/11/18 22:03, Jiang Liu wrote:
>
>> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>
>>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>>>
>>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>>>> struct irq_chip {
>>>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>>>>>
>>>>>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>>
>>>>>>> Hmm... It's really weird.
>>>>>>> I don't think it's the interrupt controllers' responsibility to write messages
>>>>>>> for all the endpoint devices since the methods of configuring message registers
>>>>>>> may different between these devices. And theoretically, the endpoint devices
>>>>>>> themselves should take the responsibility to configure their message registers.
>>>>>>> To say the least, the write_msg callback here still need to call some certain
>>>>>>> interfaces provided by the corresponding device.
>>>>>>>
>>>>>>> There would be lots of ARM new devices capable of sending message
>>>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>>>> the devices need to expose a write_msg callback to interrupt
>>>>>>> controllers?
>>>>>>
>>>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>>>
>>>>>> So in order to enable non PCI based MSI we want to have generic
>>>>>> infrastructure with minimal per device/device class callbacks and of
>>>>>> course you need to provide that callback for your special device.
>>>>>>
>>>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>>>> those. So consolidating it into common infrastructure allows us to get
>>>>>> rid of the pointless copied code and reduce the per device effort to
>>>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>>>> of those.
>>>>>>
>>>>>
>>>>> At least, we have the same goal.
>>>>> I will illustrate my thoughts by an example.
>>>>> The current code is something like:
>>>>>
>>>>> Device A
>>>>> ========
>>>>> void A_write_msg() { ... }
>>>>>
>>>>> Group B
>>>>> (a group of devices behave same on writing messages, i.e. PCI)
>>>>> =======
>>>>> void B_write_msg() { ... }
>>>>>
>>>>> Controller
>>>>> ==========
>>>>> irq_chip.irq_write_msi_msg () {
>>>>> if (A)
>>>>> A_write_msg();
>>>>> if (B)
>>>>> B_write_msg();
>>>>> }
>>>>>
>>>>> It's horrible when new devices come out, since we need to modify the
>>>>> controller part for each new device.
>>>>> What I suggested is:
>>>>>
>>>>> MSI Core
>>>>> ========
>>>>> struct msi_ops { .write_msg, };
>>>>> struct msi_desc { .msi_ops, };
>>>>>
>>>>> write_msg() {
>>>>> X = get_dev();
>>>>> irq_chip.compose_msg(X); // IRQ chips' responsibility
>>>>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>>>>> }
>>>>>
>>>>> Device A
>>>>> ========
>>>>> void A_write_msg() { ... }
>>>>> A_msi_ops.write_msg = A_write_msg;
>>>>>
>>>>> Group B
>>>>> =======
>>>>> void B_write_msg() { ... }
>>>>> B_msi_ops.write_msg = B_write_msg;
>>>>>
>>>>> Please correct me if I misunderstood anything.
>>>> Hi Yun,
>>>> We provide an irq_chip for each type of interrupt controller
>>>> instead of devices. For the example mentioned above, if device A
>>>> and Group B has different interrupt controllers, we just need to
>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>> to suitable callbacks.
>>>> The framework already achieves what you you want:)
>>>
>>> What if device A and group B have the same interrupt controller?
>> Device doesn't care about interrupt controllers, it just cares about
>> interrupts used by itself. It's the interrupt source (controller)
>> enumerators' responsibility to discover interrupt source, associate
>> methods to control the interrupt source and assign irq numbers for
>> interrupt sources.
>
> Yes, indeed.
>
>> There are two ways to associate irq numbers with device:
>> 1) arch code/bus drivers statically assigns irq number for devices
>> when creating device objects, such as PCI legacy interrupt
>> (INTA-INTD), IOAPIC interrupts.
>
> And OF interfaces.
>
>> 2) device drivers ask interrupt source enumerators to dynamically
>> create irq numbers, such pci_enable_msix_range() and friends.
>> So device driver definitely doesn't need to known about irq_chip
>> methods to control interrupt sources.
>>
>
> The above you described is absolutely right, but not the things I want
> to know. :)
> Take GICv3 ITS for example, it deals with both PCI and non PCI message
> interrupts. IIUC, several irq_chips need to be implemented in the ITS
> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
> do to the ITS driver if new MSI-capable devices come out?
Marc has posted a patchset to enable ITS based on the hierarchy
irqdomain framework, please refer to "[PATCH 00/15] arm64: PCI/MSI:
GICv3 ITS support (stacked domain edition)" at
https://lkml.org/lkml/2014/11/11/620

>
> Regards,
> Abel
>

2014-11-18 14:33:08

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:

Can you please trim the messages when you're replying?

> The above you described is absolutely right, but not the things I want
> to know. :)
> Take GICv3 ITS for example, it deals with both PCI and non PCI message
> interrupts. IIUC, several irq_chips need to be implemented in the ITS
> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
> do to the ITS driver if new MSI-capable devices come out?

You seem to miss the stacking here

PCI-MSI ->
A-MSI -> ITS -> GIC
B-MSI ->

So each of the device types has its own MSI controller. Each of them
will have their own callbacks and are backed by the underlying ITS/GIC
implementation.

And that's the only sensible solution.

Thanks,

tglx

2014-11-18 14:35:56

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:19, Thomas Gleixner wrote:

> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/18 21:43, Jiang Liu wrote:
>>> We provide an irq_chip for each type of interrupt controller
>>> instead of devices. For the example mentioned above, if device A
>>> and Group B has different interrupt controllers, we just need to
>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>> to suitable callbacks.
>>> The framework already achieves what you you want:)
>>
>> What if device A and group B have the same interrupt controller?
>
> Well, if write_msg() is different they are hardly the same.
>

The GICv3 ITS now deals with both PCI and non PCI message interrupts.
We can't require the new devices behave writing message in a same way.
What we can do is to abstract all the endpoints' behavior, and I
provided one abstraction in an earlier reply.

Thanks,
Abel

2014-11-18 14:46:59

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:29, Jiang Liu wrote:

>
>
> On 2014/11/18 22:22, Yun Wu (Abel) wrote:
>> On 2014/11/18 22:03, Jiang Liu wrote:
>>
>>> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>>
>>>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>>>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>>>>
>>>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>>>>> struct irq_chip {
>>>>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>>>>>>
>>>>>>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>>>
>>>>>>>> Hmm... It's really weird.
>>>>>>>> I don't think it's the interrupt controllers' responsibility to write messages
>>>>>>>> for all the endpoint devices since the methods of configuring message registers
>>>>>>>> may different between these devices. And theoretically, the endpoint devices
>>>>>>>> themselves should take the responsibility to configure their message registers.
>>>>>>>> To say the least, the write_msg callback here still need to call some certain
>>>>>>>> interfaces provided by the corresponding device.
>>>>>>>>
>>>>>>>> There would be lots of ARM new devices capable of sending message
>>>>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>>>>> the devices need to expose a write_msg callback to interrupt
>>>>>>>> controllers?
>>>>>>>
>>>>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>>>>
>>>>>>> So in order to enable non PCI based MSI we want to have generic
>>>>>>> infrastructure with minimal per device/device class callbacks and of
>>>>>>> course you need to provide that callback for your special device.
>>>>>>>
>>>>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>>>>> those. So consolidating it into common infrastructure allows us to get
>>>>>>> rid of the pointless copied code and reduce the per device effort to
>>>>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>>>>> of those.
>>>>>>>
>>>>>>
>>>>>> At least, we have the same goal.
>>>>>> I will illustrate my thoughts by an example.
>>>>>> The current code is something like:
>>>>>>
>>>>>> Device A
>>>>>> ========
>>>>>> void A_write_msg() { ... }
>>>>>>
>>>>>> Group B
>>>>>> (a group of devices behave same on writing messages, i.e. PCI)
>>>>>> =======
>>>>>> void B_write_msg() { ... }
>>>>>>
>>>>>> Controller
>>>>>> ==========
>>>>>> irq_chip.irq_write_msi_msg () {
>>>>>> if (A)
>>>>>> A_write_msg();
>>>>>> if (B)
>>>>>> B_write_msg();
>>>>>> }
>>>>>>
>>>>>> It's horrible when new devices come out, since we need to modify the
>>>>>> controller part for each new device.
>>>>>> What I suggested is:
>>>>>>
>>>>>> MSI Core
>>>>>> ========
>>>>>> struct msi_ops { .write_msg, };
>>>>>> struct msi_desc { .msi_ops, };
>>>>>>
>>>>>> write_msg() {
>>>>>> X = get_dev();
>>>>>> irq_chip.compose_msg(X); // IRQ chips' responsibility
>>>>>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>>>>>> }
>>>>>>
>>>>>> Device A
>>>>>> ========
>>>>>> void A_write_msg() { ... }
>>>>>> A_msi_ops.write_msg = A_write_msg;
>>>>>>
>>>>>> Group B
>>>>>> =======
>>>>>> void B_write_msg() { ... }
>>>>>> B_msi_ops.write_msg = B_write_msg;
>>>>>>
>>>>>> Please correct me if I misunderstood anything.
>>>>> Hi Yun,
>>>>> We provide an irq_chip for each type of interrupt controller
>>>>> instead of devices. For the example mentioned above, if device A
>>>>> and Group B has different interrupt controllers, we just need to
>>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>>> to suitable callbacks.
>>>>> The framework already achieves what you you want:)
>>>>
>>>> What if device A and group B have the same interrupt controller?
>>> Device doesn't care about interrupt controllers, it just cares about
>>> interrupts used by itself. It's the interrupt source (controller)
>>> enumerators' responsibility to discover interrupt source, associate
>>> methods to control the interrupt source and assign irq numbers for
>>> interrupt sources.
>>
>> Yes, indeed.
>>
>>> There are two ways to associate irq numbers with device:
>>> 1) arch code/bus drivers statically assigns irq number for devices
>>> when creating device objects, such as PCI legacy interrupt
>>> (INTA-INTD), IOAPIC interrupts.
>>
>> And OF interfaces.
>>
>>> 2) device drivers ask interrupt source enumerators to dynamically
>>> create irq numbers, such pci_enable_msix_range() and friends.
>>> So device driver definitely doesn't need to known about irq_chip
>>> methods to control interrupt sources.
>>>
>>
>> The above you described is absolutely right, but not the things I want
>> to know. :)
>> Take GICv3 ITS for example, it deals with both PCI and non PCI message
>> interrupts. IIUC, several irq_chips need to be implemented in the ITS
>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
>> do to the ITS driver if new MSI-capable devices come out?
> Marc has posted a patchset to enable ITS based on the hierarchy
> irqdomain framework, please refer to "[PATCH 00/15] arm64: PCI/MSI:
> GICv3 ITS support (stacked domain edition)" at
> https://lkml.org/lkml/2014/11/11/620
>

IIUC, Marc's patch now only supports PCI MSI/MSI-X...

Thanks,
Abel

2014-11-18 14:52:53

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:34, Yun Wu (Abel) wrote:
> On 2014/11/18 22:19, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>> We provide an irq_chip for each type of interrupt controller
>>>> instead of devices. For the example mentioned above, if device A
>>>> and Group B has different interrupt controllers, we just need to
>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>> to suitable callbacks.
>>>> The framework already achieves what you you want:)
>>>
>>> What if device A and group B have the same interrupt controller?
>>
>> Well, if write_msg() is different they are hardly the same.
>>
>
> The GICv3 ITS now deals with both PCI and non PCI message interrupts.
> We can't require the new devices behave writing message in a same way.
> What we can do is to abstract all the endpoints' behavior, and I
> provided one abstraction in an earlier reply.
It should be easy to extend:)
Actually, x86 interrupt remapping drivers already support two types of
MSIs, one is PCI MSI/MSIX, another is HPET interrupt.
>
> Thanks,
> Abel
>

2014-11-18 17:14:24

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Tue, Nov 18 2014 at 2:46:02 pm GMT, "Yun Wu (Abel)" <[email protected]> wrote:
> On 2014/11/18 22:29, Jiang Liu wrote:
>
>>
>>
>> On 2014/11/18 22:22, Yun Wu (Abel) wrote:
>>> On 2014/11/18 22:03, Jiang Liu wrote:
>>>
>>>> On 2014/11/18 21:52, Yun Wu (Abel) wrote:
>>>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>>>
>>>>>> On 2014/11/18 21:33, Yun Wu (Abel) wrote:
>>>>>>> On 2014/11/18 18:19, Thomas Gleixner wrote:
>>>>>>>
>>>>>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>>>>>> struct irq_chip {
>>>>>>>>>> @@ -359,6 +360,7 @@ struct irq_chip {
>>>>>>>>>> void (*irq_release_resources)(struct irq_data *data);
>>>>>>>>>>
>>>>>>>>>> void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>>>>> + void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
>>>>>>>>>
>>>>>>>>> Hmm... It's really weird.
>>>>>>>>> I don't think it's the interrupt controllers' responsibility
>>>>>>>>> to write messages
>>>>>>>>> for all the endpoint devices since the methods of configuring
>>>>>>>>> message registers
>>>>>>>>> may different between these devices. And theoretically, the
>>>>>>>>> endpoint devices
>>>>>>>>> themselves should take the responsibility to configure their
>>>>>>>>> message registers.
>>>>>>>>> To say the least, the write_msg callback here still need to
>>>>>>>>> call some certain
>>>>>>>>> interfaces provided by the corresponding device.
>>>>>>>>>
>>>>>>>>> There would be lots of ARM new devices capable of sending message
>>>>>>>>> based interrupts to interrupt controllers, does all the drivers of
>>>>>>>>> the devices need to expose a write_msg callback to interrupt
>>>>>>>>> controllers?
>>>>>>>>
>>>>>>>> Well, writing the message _IS_ part of the interrupt controller.
>>>>>>>>
>>>>>>>> So in order to enable non PCI based MSI we want to have generic
>>>>>>>> infrastructure with minimal per device/device class callbacks and of
>>>>>>>> course you need to provide that callback for your special device.
>>>>>>>>
>>>>>>>> We already have non PCI based MSI controllers in x86 today and we need
>>>>>>>> to handle the whole stuff with tons of copied coded extra for each of
>>>>>>>> those. So consolidating it into common infrastructure allows us to get
>>>>>>>> rid of the pointless copied code and reduce the per device effort to
>>>>>>>> the relevant hardware specific callbacks. irq_write_msi_msg being one
>>>>>>>> of those.
>>>>>>>>
>>>>>>>
>>>>>>> At least, we have the same goal.
>>>>>>> I will illustrate my thoughts by an example.
>>>>>>> The current code is something like:
>>>>>>>
>>>>>>> Device A
>>>>>>> ========
>>>>>>> void A_write_msg() { ... }
>>>>>>>
>>>>>>> Group B
>>>>>>> (a group of devices behave same on writing messages, i.e. PCI)
>>>>>>> =======
>>>>>>> void B_write_msg() { ... }
>>>>>>>
>>>>>>> Controller
>>>>>>> ==========
>>>>>>> irq_chip.irq_write_msi_msg () {
>>>>>>> if (A)
>>>>>>> A_write_msg();
>>>>>>> if (B)
>>>>>>> B_write_msg();
>>>>>>> }
>>>>>>>
>>>>>>> It's horrible when new devices come out, since we need to modify the
>>>>>>> controller part for each new device.
>>>>>>> What I suggested is:
>>>>>>>
>>>>>>> MSI Core
>>>>>>> ========
>>>>>>> struct msi_ops { .write_msg, };
>>>>>>> struct msi_desc { .msi_ops, };
>>>>>>>
>>>>>>> write_msg() {
>>>>>>> X = get_dev();
>>>>>>> irq_chip.compose_msg(X); // IRQ chips' responsibility
>>>>>>> X_msi_ops.write_msg(); // nothing to do with IRQ chips
>>>>>>> }
>>>>>>>
>>>>>>> Device A
>>>>>>> ========
>>>>>>> void A_write_msg() { ... }
>>>>>>> A_msi_ops.write_msg = A_write_msg;
>>>>>>>
>>>>>>> Group B
>>>>>>> =======
>>>>>>> void B_write_msg() { ... }
>>>>>>> B_msi_ops.write_msg = B_write_msg;
>>>>>>>
>>>>>>> Please correct me if I misunderstood anything.
>>>>>> Hi Yun,
>>>>>> We provide an irq_chip for each type of interrupt controller
>>>>>> instead of devices. For the example mentioned above, if device A
>>>>>> and Group B has different interrupt controllers, we just need to
>>>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>>>> to suitable callbacks.
>>>>>> The framework already achieves what you you want:)
>>>>>
>>>>> What if device A and group B have the same interrupt controller?
>>>> Device doesn't care about interrupt controllers, it just cares about
>>>> interrupts used by itself. It's the interrupt source (controller)
>>>> enumerators' responsibility to discover interrupt source, associate
>>>> methods to control the interrupt source and assign irq numbers for
>>>> interrupt sources.
>>>
>>> Yes, indeed.
>>>
>>>> There are two ways to associate irq numbers with device:
>>>> 1) arch code/bus drivers statically assigns irq number for devices
>>>> when creating device objects, such as PCI legacy interrupt
>>>> (INTA-INTD), IOAPIC interrupts.
>>>
>>> And OF interfaces.
>>>
>>>> 2) device drivers ask interrupt source enumerators to dynamically
>>>> create irq numbers, such pci_enable_msix_range() and friends.
>>>> So device driver definitely doesn't need to known about irq_chip
>>>> methods to control interrupt sources.
>>>>
>>>
>>> The above you described is absolutely right, but not the things I want
>>> to know. :)
>>> Take GICv3 ITS for example, it deals with both PCI and non PCI message
>>> interrupts. IIUC, several irq_chips need to be implemented in the ITS
>>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
>>> do to the ITS driver if new MSI-capable devices come out?
>> Marc has posted a patchset to enable ITS based on the hierarchy
>> irqdomain framework, please refer to "[PATCH 00/15] arm64: PCI/MSI:
>> GICv3 ITS support (stacked domain edition)" at
>> https://lkml.org/lkml/2014/11/11/620
>>
>
> IIUC, Marc's patch now only supports PCI MSI/MSI-X...

Indeed, and the current solution makes is relatively easy to plug in
non-PCI MSI. Just don't plug the ITS into the *PCI* MSI framework when
you encounter such a thing.

Thanks,

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

2014-11-18 17:22:03

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Tue, Nov 18 2014 at 2:34:44 pm GMT, "Yun Wu (Abel)" <[email protected]> wrote:
> On 2014/11/18 22:19, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>> We provide an irq_chip for each type of interrupt controller
>>>> instead of devices. For the example mentioned above, if device A
>>>> and Group B has different interrupt controllers, we just need to
>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>> to suitable callbacks.
>>>> The framework already achieves what you you want:)
>>>
>>> What if device A and group B have the same interrupt controller?
>>
>> Well, if write_msg() is different they are hardly the same.
>>
>
> The GICv3 ITS now deals with both PCI and non PCI message interrupts.
> We can't require the new devices behave writing message in a same way.
> What we can do is to abstract all the endpoints' behavior, and I
> provided one abstraction in an earlier reply.

This is why the framework gives you the opportunity to provide methods
that:
- compose the message
- program the message into the device

None of that has to be PCI specific, and gives you a clean
abstraction. The framework only gives you a number of shortcuts for PCI
MSI, because that's what most people care about.

Thanks,

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

2014-11-19 03:38:44

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/19 1:14, Marc Zyngier wrote:

> On Tue, Nov 18 2014 at 2:46:02 pm GMT, "Yun Wu (Abel)" <[email protected]> wrote:
[...]
>> IIUC, Marc's patch now only supports PCI MSI/MSI-X...
>
> Indeed, and the current solution makes is relatively easy to plug in
> non-PCI MSI. Just don't plug the ITS into the *PCI* MSI framework when
> you encounter such a thing.
>

I am looking forward to that. :)
I guess the main reason you haven't plugged in non-PCI MSI is the way
of obtaining device ids used in searching DT table.

Thanks,
Abel

2014-11-19 03:41:24

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/19 1:21, Marc Zyngier wrote:

> On Tue, Nov 18 2014 at 2:34:44 pm GMT, "Yun Wu (Abel)" <[email protected]> wrote:
>> On 2014/11/18 22:19, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>>> We provide an irq_chip for each type of interrupt controller
>>>>> instead of devices. For the example mentioned above, if device A
>>>>> and Group B has different interrupt controllers, we just need to
>>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>>> to suitable callbacks.
>>>>> The framework already achieves what you you want:)
>>>>
>>>> What if device A and group B have the same interrupt controller?
>>>
>>> Well, if write_msg() is different they are hardly the same.
>>>
>>
>> The GICv3 ITS now deals with both PCI and non PCI message interrupts.
>> We can't require the new devices behave writing message in a same way.
>> What we can do is to abstract all the endpoints' behavior, and I
>> provided one abstraction in an earlier reply.
>
> This is why the framework gives you the opportunity to provide methods
> that:
> - compose the message
> - program the message into the device
>
> None of that has to be PCI specific, and gives you a clean
> abstraction. The framework only gives you a number of shortcuts for PCI
> MSI, because that's what most people care about.
>

Indeed, and I never said Jiang's patches don't work, I was just thinking
that they were not that perfect.

Thanks,
Abel

2014-11-19 03:54:49

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:52, Jiang Liu wrote:

> On 2014/11/18 22:34, Yun Wu (Abel) wrote:
>> On 2014/11/18 22:19, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>> On 2014/11/18 21:43, Jiang Liu wrote:
>>>>> We provide an irq_chip for each type of interrupt controller
>>>>> instead of devices. For the example mentioned above, if device A
>>>>> and Group B has different interrupt controllers, we just need to
>>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
>>>>> to suitable callbacks.
>>>>> The framework already achieves what you you want:)
>>>>
>>>> What if device A and group B have the same interrupt controller?
>>>
>>> Well, if write_msg() is different they are hardly the same.
>>>
>>
>> The GICv3 ITS now deals with both PCI and non PCI message interrupts.
>> We can't require the new devices behave writing message in a same way.
>> What we can do is to abstract all the endpoints' behavior, and I
>> provided one abstraction in an earlier reply.
> It should be easy to extend:)
> Actually, x86 interrupt remapping drivers already support two types of
> MSIs, one is PCI MSI/MSIX, another is HPET interrupt.

Well, if there are one hundred types, I don't think it's as easy as you
thought to extend. Of course we can doubt the possibility of being hundred,
but tens or twenties is reasonably possible lying under the fact we have
already startet to integrate the MSI registers (or some other form to store
information) into the individual devices.

Thanks,
Abel

2014-11-19 07:04:13

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/18 22:32, Thomas Gleixner wrote:

> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>
> Can you please trim the messages when you're replying?
>
>> The above you described is absolutely right, but not the things I want
>> to know. :)
>> Take GICv3 ITS for example, it deals with both PCI and non PCI message
>> interrupts. IIUC, several irq_chips need to be implemented in the ITS
>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
>> do to the ITS driver if new MSI-capable devices come out?
>
> You seem to miss the stacking here
>
> PCI-MSI ->
> A-MSI -> ITS -> GIC
> B-MSI ->
>
> So each of the device types has its own MSI controller. Each of them
> will have their own callbacks and are backed by the underlying ITS/GIC
> implementation.

Yes, this hits the key point. Once a new device type becomes available,
we need to add pieces of code outside the new device's driver to make
it work, which in my opinion is due to lack of core infrastructure.
More specifically, the core infrastructure needs to support mechanism
of MSI, not the various types of devices.

>
> And that's the only sensible solution.
>

It's sensible, but not perfect.
What I suggested is to add a generic layer:

PCI-MSI ->
A-MSI -> (generic layer) -> ITS -> GICR
B-MSI ->

The PCI/A/B/... passes its hardware properties to the generic layer which
gets configurations ready by calling ITS's domain/chip callbacks. When
a new device type arrives, the only thing need to do is to implement the
driver of that device, with nothing to do with the generic layer or ITS.

Thanks,
Abel

2014-11-19 08:02:40

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/19 14:57, Yun Wu (Abel) wrote:
> On 2014/11/18 22:32, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>
>> Can you please trim the messages when you're replying?
>>
>>> The above you described is absolutely right, but not the things I want
>>> to know. :)
>>> Take GICv3 ITS for example, it deals with both PCI and non PCI message
>>> interrupts. IIUC, several irq_chips need to be implemented in the ITS
>>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
>>> do to the ITS driver if new MSI-capable devices come out?
>>
>> You seem to miss the stacking here
>>
>> PCI-MSI ->
>> A-MSI -> ITS -> GIC
>> B-MSI ->
>>
>> So each of the device types has its own MSI controller. Each of them
>> will have their own callbacks and are backed by the underlying ITS/GIC
>> implementation.
>
> Yes, this hits the key point. Once a new device type becomes available,
> we need to add pieces of code outside the new device's driver to make
> it work, which in my opinion is due to lack of core infrastructure.
> More specifically, the core infrastructure needs to support mechanism
> of MSI, not the various types of devices.
It depends on the implementation details whether we need to change ITS
to support new devices. One possible way to avoid changing ITS to
support new devices is:
1) ITS only allocates and manages interrupt translation tables and pass
some type of identifier back to MSI controller drivers.
2) Each MSI controller driver implements its own compose_msi_msg()
and write_msi_msg() according to hardware specifications.
By this way, there is no need to change ITS to support new types of
MSI controllers.

On x86 systems, interrupt remapping is optional and we also want to hide
implementation details, so we chose another solution:
1) interrupt remapping driver allocates and manages remapping resources,
it also implements different compose_msi_msg() to support different
MSI controllers.
2) MSI controller implement write_msi_msg(), it also implements
compose_msi_msg() if interrupt remapping is disabled.
By this way, we do need to change interrupt remapping driver(ITS on ARM)
to support new type of MSI controllers.

So it totally depends on how you implement the hierarchy irqdomain,
there are no limitations from the framework side.

For the proposal to add some type of ops structure into struct msi_desc,
actually I have a serial of patches to support device specific
mask/unmask/write/compose/startup/shutdown operations by adding
msi_desc.chip_ops. But there's no actual users of those interfaces
currently and we don't want to over-engineering, so I abandoned
that patch set. I may rework on it when enabling generic MSI
support later.
Regards!
Gerry
>
>>
>> And that's the only sensible solution.
>>
>
> It's sensible, but not perfect.
> What I suggested is to add a generic layer:
>
> PCI-MSI ->
> A-MSI -> (generic layer) -> ITS -> GICR
> B-MSI ->
>
> The PCI/A/B/... passes its hardware properties to the generic layer which
> gets configurations ready by calling ITS's domain/chip callbacks. When
> a new device type arrives, the only thing need to do is to implement the
> driver of that device, with nothing to do with the generic layer or ITS.
>
> Thanks,
> Abel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2014-11-19 08:55:32

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Wed, Nov 19 2014 at 3:38:09 am GMT, "Yun Wu (Abel)" <[email protected]> wrote:
> On 2014/11/19 1:14, Marc Zyngier wrote:
>
>> On Tue, Nov 18 2014 at 2:46:02 pm GMT, "Yun Wu (Abel)" <[email protected]> wrote:
> [...]
>>> IIUC, Marc's patch now only supports PCI MSI/MSI-X...
>>
>> Indeed, and the current solution makes is relatively easy to plug in
>> non-PCI MSI. Just don't plug the ITS into the *PCI* MSI framework when
>> you encounter such a thing.
>>
>
> I am looking forward to that. :)

I really don't.

> I guess the main reason you haven't plugged in non-PCI MSI is the way
> of obtaining device ids used in searching DT table.

That, and also:
- No way to carve out LPI ranges for non-PCI allocations, should some
ITS mandate specific ranges
- No description of how to program the generating device, it required
- No platform with such requirements aiming for upstream support

To put it mildly, non-PCI support on the ITS is a rather long way
away, and it has nothing to do with the code. It has everything to do
with specifying what we want to support, and how we describe it in
DT. Until then, not much is going to happen.

Thanks,

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

2014-11-19 09:21:02

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Wed, Nov 19 2014 at 6:57:25 am GMT, "Yun Wu (Abel)" <[email protected]> wrote:
> On 2014/11/18 22:32, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>
>> Can you please trim the messages when you're replying?
>>
>>> The above you described is absolutely right, but not the things I want
>>> to know. :)
>>> Take GICv3 ITS for example, it deals with both PCI and non PCI message
>>> interrupts. IIUC, several irq_chips need to be implemented in the ITS
>>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
>>> do to the ITS driver if new MSI-capable devices come out?
>>
>> You seem to miss the stacking here
>>
>> PCI-MSI ->
>> A-MSI -> ITS -> GIC
>> B-MSI ->
>>
>> So each of the device types has its own MSI controller. Each of them
>> will have their own callbacks and are backed by the underlying ITS/GIC
>> implementation.
>
> Yes, this hits the key point. Once a new device type becomes available,
> we need to add pieces of code outside the new device's driver to make
> it work, which in my opinion is due to lack of core infrastructure.
> More specifically, the core infrastructure needs to support mechanism
> of MSI, not the various types of devices.
>
>>
>> And that's the only sensible solution.
>>
>
> It's sensible, but not perfect.
> What I suggested is to add a generic layer:
>
> PCI-MSI ->
> A-MSI -> (generic layer) -> ITS -> GICR
> B-MSI ->
>
> The PCI/A/B/... passes its hardware properties to the generic layer which
> gets configurations ready by calling ITS's domain/chip callbacks. When
> a new device type arrives, the only thing need to do is to implement the
> driver of that device, with nothing to do with the generic layer or ITS.

I really don't get your "generic layer" story. To me, it looks like a
glorified set of function pointers. And that's exactly what stacked
domains are giving you:

A-MSI -> ITS -> GIC

This "A-MSI" is responsible for:
- implementing the "prepare" callback, which allocates the ITT
- implementing the "irq_compose_msi_msg"

Hardly anything to change in the ITS driver, and I can probably make it
so that you don't even have to write a single line of code in the ITS
driver.

If the generic MSI layer we now have is not enough for you, then please
submit detailed use cases.

Thanks,

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

2014-11-19 11:09:28

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Wed, 19 Nov 2014, Yun Wu (Abel) wrote:
> On 2014/11/18 22:52, Jiang Liu wrote:
>
> > On 2014/11/18 22:34, Yun Wu (Abel) wrote:
> >> On 2014/11/18 22:19, Thomas Gleixner wrote:
> >>
> >>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
> >>>> On 2014/11/18 21:43, Jiang Liu wrote:
> >>>>> We provide an irq_chip for each type of interrupt controller
> >>>>> instead of devices. For the example mentioned above, if device A
> >>>>> and Group B has different interrupt controllers, we just need to
> >>>>> implement irq_chip_A and irq_chip_B and set irq_chip.irq_write_msi_msg()
> >>>>> to suitable callbacks.
> >>>>> The framework already achieves what you you want:)
> >>>>
> >>>> What if device A and group B have the same interrupt controller?
> >>>
> >>> Well, if write_msg() is different they are hardly the same.
> >>>
> >>
> >> The GICv3 ITS now deals with both PCI and non PCI message interrupts.
> >> We can't require the new devices behave writing message in a same way.
> >> What we can do is to abstract all the endpoints' behavior, and I
> >> provided one abstraction in an earlier reply.
> > It should be easy to extend:)
> > Actually, x86 interrupt remapping drivers already support two types of
> > MSIs, one is PCI MSI/MSIX, another is HPET interrupt.
>
>
> Well, if there are one hundred types, I don't think it's as easy as you
> thought to extend. Of course we can doubt the possibility of being hundred,
> but tens or twenties is reasonably possible lying under the fact we have
> already startet to integrate the MSI registers (or some other form to store
> information) into the individual devices.

If your hardware designers decided to come up with 20+ different ways
to implement MSI support, then it's not a reason to inflict completely
non-sensical crap into the core infrastucture.

The infrastructure we provided is optimized for sane use cases, while
it allows you to deal with totally brainwrecked hardware designs.

If you have 20 different ways, you need 20 different controllers for
it, whether you like it or not. No magic 'generic' abstraction will
solve that for you.

Thanks,

tglx

2014-11-19 11:11:21

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Wed, 19 Nov 2014, Yun Wu (Abel) wrote:
> On 2014/11/19 1:21, Marc Zyngier wrote:
> > This is why the framework gives you the opportunity to provide methods
> > that:
> > - compose the message
> > - program the message into the device
> >
> > None of that has to be PCI specific, and gives you a clean
> > abstraction. The framework only gives you a number of shortcuts for PCI
> > MSI, because that's what most people care about.
> >
>
> Indeed, and I never said Jiang's patches don't work, I was just thinking
> that they were not that perfect.

But your magic extra layer of indirection is perfect? It's not, it
just violates sane layering in order to support braindead hardware
designs.

Thanks,

tglx

2014-12-10 09:12:51

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/19 19:11, Thomas Gleixner wrote:

> On Wed, 19 Nov 2014, Yun Wu (Abel) wrote:
>> On 2014/11/19 1:21, Marc Zyngier wrote:
>>> This is why the framework gives you the opportunity to provide methods
>>> that:
>>> - compose the message
>>> - program the message into the device
>>>
>>> None of that has to be PCI specific, and gives you a clean
>>> abstraction. The framework only gives you a number of shortcuts for PCI
>>> MSI, because that's what most people care about.
>>>
>>
>> Indeed, and I never said Jiang's patches don't work, I was just thinking
>> that they were not that perfect.
>
> But your magic extra layer of indirection is perfect? It's not, it
> just violates sane layering in order to support braindead hardware
> designs.
>

Hi Thomas, Gerry and Marc,

I spent last two weeks implementing and testing my original idea about making
the sub domains generic, based on stacked domain feature. Now it comes real,
please see the attached patch.

With this patch applied, I think things will get easier.

For drivers of interrupt controllers, they need to implement:
a) struct irq_chip, gets associated in domain's map/alloc callback
b) struct irq_domain, with corresponding operations
c) create sub generic MBI domain of IRQ domain to deal with all MBI types.
This changes almost nothing of the current code.

For drivers of MBI-capable devices, they need to implement:
a) MBI operations, like mask/unmask or setting message.
This will remove current ugly arch-specific code by organizing the device
behavior into a generic structure used in generic MBI layer.

The MBI generic code will build the bridge between the two groups. So when a
new driver need to implement, either controller's or endpoint's, just code
the corresponding 'need' described above with no more work to do.

This patch (also with several other patches) is tested on Hisilicon ARM64 SoC,
with non PCI devices capable of message based interrupts. The PCI part is not
tested because it needs large refactoring work to do. So yes, the testing work
is not sufficient, but I think the patch is enough to present what I really
wanted to express. :)

A new term introduced by the patch named Message Based Interrupt (MBI) is used
for presenting the generic MSIs (which does help me avoid conflicting with the
existing code). Actually the new name is proposed by Marc several months ago,
suggesting that MSI implies too much about PCI. I think it's a good idea to use
MBI in generic code and make the MSI/MSI-x a wrapper of MBI inside the PCI core.
Anyway, naming is not the key point yet.

Finally, yes, my thoughts is not perfect, but I am just trying to make it better.

Best regards and thanks,
Abel

Attachments:

0001-MBI-initial-support-for-message-based-interrupts.patch (16.45 kB)

2014-12-10 09:27:22

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On 2014/11/19 17:20, Marc Zyngier wrote:

> On Wed, Nov 19 2014 at 6:57:25 am GMT, "Yun Wu (Abel)" <[email protected]> wrote:
>> On 2014/11/18 22:32, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>
>>> Can you please trim the messages when you're replying?
>>>
>>>> The above you described is absolutely right, but not the things I want
>>>> to know. :)
>>>> Take GICv3 ITS for example, it deals with both PCI and non PCI message
>>>> interrupts. IIUC, several irq_chips need to be implemented in the ITS
>>>> driver (i.e. pci_msi_chip, A_msi_chip and B_msi_chip). What should we
>>>> do to the ITS driver if new MSI-capable devices come out?
>>>
>>> You seem to miss the stacking here
>>>
>>> PCI-MSI ->
>>> A-MSI -> ITS -> GIC
>>> B-MSI ->
>>>
>>> So each of the device types has its own MSI controller. Each of them
>>> will have their own callbacks and are backed by the underlying ITS/GIC
>>> implementation.
>>
>> Yes, this hits the key point. Once a new device type becomes available,
>> we need to add pieces of code outside the new device's driver to make
>> it work, which in my opinion is due to lack of core infrastructure.
>> More specifically, the core infrastructure needs to support mechanism
>> of MSI, not the various types of devices.
>>
>>>
>>> And that's the only sensible solution.
>>>
>>
>> It's sensible, but not perfect.
>> What I suggested is to add a generic layer:
>>
>> PCI-MSI ->
>> A-MSI -> (generic layer) -> ITS -> GICR
>> B-MSI ->
>>
>> The PCI/A/B/... passes its hardware properties to the generic layer which
>> gets configurations ready by calling ITS's domain/chip callbacks. When
>> a new device type arrives, the only thing need to do is to implement the
>> driver of that device, with nothing to do with the generic layer or ITS.
>
> I really don't get your "generic layer" story. To me, it looks like a
> glorified set of function pointers. And that's exactly what stacked
> domains are giving you:
>
> A-MSI -> ITS -> GIC
>
> This "A-MSI" is responsible for:
> - implementing the "prepare" callback, which allocates the ITT
> - implementing the "irq_compose_msi_msg"
>
> Hardly anything to change in the ITS driver, and I can probably make it
> so that you don't even have to write a single line of code in the ITS
> driver.
>
> If the generic MSI layer we now have is not enough for you, then please
> submit detailed use cases.
>

Hi Marc,

As I said, I never thought Gerry's patch don't work, I am just trying to
make it better. :)

As to the "generic layer" story, please check the following URL:
https://lkml.org/lkml/2014/12/10/93

Thanks,
Abel

2014-12-10 10:25:22

[permalink] [raw]

Subject: Re: [patch 08/16] genirq: Introduce callback irq_chip.irq_write_msi_msg

On Wed, 10 Dec 2014, Yun Wu (Abel) wrote:
> On 2014/11/19 19:11, Thomas Gleixner wrote:
> I spent last two weeks implementing and testing my original idea about making
> the sub domains generic, based on stacked domain feature. Now it comes real,
> please see the attached patch.

Can you please send patches inline? Attached is a pain to reply to.

> With this patch applied, I think things will get easier.

I don't see what gets easier. It's just another infrastructure which
is painfully similar to MSI.

> This patch (also with several other patches) is tested on Hisilicon ARM64 SoC,
> with non PCI devices capable of message based interrupts. The PCI part is not
> tested because it needs large refactoring work to do. So yes, the testing work
> is not sufficient, but I think the patch is enough to present what I really
> wanted to express. :)

Not really.

If you provide proper patches which make use of it and most important
a proper refactoring of the PCI/MSI side then we can discuss that, but
for now it's just handwaving.

Thanks,

tglx

2014-12-11 03:02:20