Linux Virtualization - Aug 2014 - [RFC PATCH 00/11] Refactor MSI to support Non-PCI device

On 2014/7/29 22:08, Arnd Bergmann wrote:
> On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
>>         The series is a draft of generic MSI driver that supports PCI
>> and Non-PCI device which have MSI capability. If you're not
interested
>> it, sorry for the noise.
> 
> I've finally managed to take some time to look at the series. Overall,
> the concept looks good to me, and the patches look very well implemented.
> 
> The part I'm not sure about is the interface we want to end up with
> at the end of the series. More on that below
Hi Arnd,
   Thanks for your review and comments very much!
Please refer the inline comments.
> 
>> The series is based on Linux-3.16-rc1.
>>
>> MSI was introduced in PCI Spec 2.2. Currently, kernel MSI 
>> driver codes are bonding with PCI device. Because MSI has a lot
>> advantages in design. More and more non-PCI devices want to
>> use MSI as their default interrupt. The existing MSI device
>> include HPET. HPET driver provide its own MSI code to initialize
>> and process MSI interrupts. In the latest GIC v3 spec, legacy device
>> can deliver MSI by the help of a relay device named consolidator.
>> Consolidator can translate the legacy interrupts connected to it
>> to MSI/MSI-X. And new non-PCI device will be designed to 
>> support MSI in future. So make the MSI driver code be generic will 
>> help the non-PCI device use MSI more simply.
>>
>> The new data struct for generic MSI driver.
>> struct msi_irqs {
>>         u8 msi_enabled:1; /* Enable flag */
>>         u8 msix_enabled:1;
>>         struct list_head msi_list; /* MSI desc list */
>>         void *data;     /* help to find the MSI device */
>>         struct msi_ops *ops; /* MSI device specific hook */
>> };
>> struct msi_irqs is used to manage MSI related informations. Every
device supports
>> MSI should contain this data struct and allocate it.
> 
> I think you should have a stronger association with the 'struct
> device' here. Can you replace the 'void *data' with 'struct
device *dev'?
Actually, I used the struct device *dev in my first draft, finally, I replaced
it with void *data, because some MSI devices don't have a struct device
*dev,
like the existing hpet device, dmar msi device, and OF device, like the ARM
consolidator.

Of course, we can make the MSI devices have their own struct device, and
register to
device tree, eg, add a class device named MSI_DEV. But I'm not sure whether
it is appropriate.
> 
> The other part I'm not completely sure about is how you want to
> have MSIs map into normal IRQ descriptors. At the moment, all
> MSI users are based on IRQ numbers, but this has known scalability
problems.
Hmmm, I still use the IRQ number to map the MSIs to IRQ description.
I'm sorry, I don't understand you meaning.
What are the scalability problems you mentioned ?
For device drivers, they always process interrupt in two steps.
If irq is the legacy interrupt, drivers will first
use the irq_of_parse_and_map() or pci_enable_device() to parse and get the IRQ
number.
Then drivers will call the request_irq() to register the interrupt handler.
If irq is MSIs, first call pci_enable_msi/x() to get the IRQ number and then
call
request_irq() to register interrupt handler.
> I wonder if we can do the interface in a way that
> hides the interrupt number from generic device drivers and just
> passes a 'struct irq_desc'. Note that there are long-term plans to
> get rid of IRQ numbers entirely, but those plans have existed for
> a long time already without anybody seriously addressing the device
> driver interfaces so far, so it might never really happen.
> 
Maybe this is a huge work, now hundreds drivers use the IRQ number, so maybe we
can consider
this in a separate title.
>> struct msi_ops {
>>         struct msi_desc *(*msi_setup_entry)(struct msi_irqs *msi,
struct msi_desc *entry);
>>         int msix_setup_entries(struct msi_irqs *msi, struct msix_entry
*entries);
>>         u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask, u32 flag);
>>         u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
>>         void (*msi_read_message)(struct msi_desc *desc, struct msi_msg
*msg);
>>         void (*msi_write_message)(struct msi_desc *desc, struct msi_msg
*msg);
>>         void (*msi_set_intx)(struct msi_irqs *msi, int enable);
>> };
>> struct msi_ops provides several hook functions, generic MSI driver will
call
>> the hook functions to access device specific registers. PCI devices
will share
>> the same msi_ops, because they have the same way to access MSI hardware
registers.
>>
>> Generic MSI layer export msi_capability_init() and
msix_capability_init() functions
>> to drivers. msi/x_capability_init() will initialize MSI capability data
struct msi_desc
>> and alloc the irq, then write the msi address/data value to hardware
registers.
>>
>> This series only did compile test, we will test it in x86 and arm
platform later.
> 
> For the generic drivers, I don't see much point in differentiating
between
> MSI and MSI-X, as I believe the difference is something internal to the PCI
> implementation.
Yes, we can integrate them, and use a generic ops, add a type in hook function
to
differentiate them.
> 
> With the other operations, I think they should all take a 'struct
device *'
> as the first argument for convenience and consistency. I don't think
you actually
> need msi_read_message(), and we could avoid msi_write_message() by doing it
> the other way round.
> 
There only two functions use the read_msi_msg(), because every msi_desc has
a struct msi_msg, and it caches the msi address and data. I will consider to
retrieve the msg from cached msi_msg, then we can avoid the msi_read_message().
But msi_write_message() maybe necessary, some xxx_set_affinity() functions and
restore functions need the msi_write_message() to rewrite the address and data.
> What I'd envision as the API from the device driver perspective is
something
> as simple like this:
> 
> struct msi_desc *msi_request(struct msi_chip *chip, irq_handler_t handler,
> 			unsigned long flags, const char *name, struct device *dev);
> 
> which would get an msi descriptor that is valid for this device (dev)
> connected to a particular msi_chip, and associate a handler function
> with it. The device driver can call that function and retrieve the
> address/message pair from the msi_desc in order to store it in its own
> device specific registers. The request_irq() can be handled internally
> to msi_request().
This is a huge change for device drivers, and some device drivers don't know
which msi_chip
their MSI irq deliver to. I'm reworking the msi_chip, and try to use
msi_chip to eliminate
all arch_msi_xxx() under every arch in kernel. And the important point is how to
create the
binding for the MSI device to the target msi_chip.

For PCI device, some arm platform already bound the msi_chip to the pci
hostbridge, then all
pci devices under the pci hostbridge deliver their MSI irqs to the target
msi_chip.
And other platform create the binding in DTS file, then the MSI device can find
their msi_chip
by device_node.
I don't know whether there are other situations, we should provide a generic
interface that
every MSI device under every platform can use it to find its msi_chip exactly.


Thanks!
Yijing.
> 
> .
> 

-- 
Thanks!
Yijing

On 2014/7/30 10:45, Yijing Wang wrote:
> On 2014/7/29 22:08, Arnd Bergmann wrote:
>> On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
>>>         The series is a draft of generic MSI driver that supports
PCI
>>> and Non-PCI device which have MSI capability. If you're not
interested
>>> it, sorry for the noise.
>>
>> I've finally managed to take some time to look at the series.
Overall,
>> the concept looks good to me, and the patches look very well
implemented.
>>
>> The part I'm not sure about is the interface we want to end up with
>> at the end of the series. More on that below
> 
> Hi Arnd,
>    Thanks for your review and comments very much!
> Please refer the inline comments.
> 
>>
>>> The series is based on Linux-3.16-rc1.
>>>
>>> MSI was introduced in PCI Spec 2.2. Currently, kernel MSI 
>>> driver codes are bonding with PCI device. Because MSI has a lot
>>> advantages in design. More and more non-PCI devices want to
>>> use MSI as their default interrupt. The existing MSI device
>>> include HPET. HPET driver provide its own MSI code to initialize
>>> and process MSI interrupts. In the latest GIC v3 spec, legacy
device
>>> can deliver MSI by the help of a relay device named consolidator.
>>> Consolidator can translate the legacy interrupts connected to it
>>> to MSI/MSI-X. And new non-PCI device will be designed to 
>>> support MSI in future. So make the MSI driver code be generic will 
>>> help the non-PCI device use MSI more simply.
>>>
>>> The new data struct for generic MSI driver.
>>> struct msi_irqs {
>>>         u8 msi_enabled:1; /* Enable flag */
>>>         u8 msix_enabled:1;
>>>         struct list_head msi_list; /* MSI desc list */
>>>         void *data;     /* help to find the MSI device */
>>>         struct msi_ops *ops; /* MSI device specific hook */
>>> };
>>> struct msi_irqs is used to manage MSI related informations. Every
device supports
>>> MSI should contain this data struct and allocate it.
>>
>> I think you should have a stronger association with the 'struct
>> device' here. Can you replace the 'void *data' with
'struct device *dev'?
> 
> Actually, I used the struct device *dev in my first draft, finally, I
replaced
> it with void *data, because some MSI devices don't have a struct device
*dev,
> like the existing hpet device, dmar msi device, and OF device, like the ARM
consolidator.
> 
> Of course, we can make the MSI devices have their own struct device, and
register to
> device tree, eg, add a class device named MSI_DEV. But I'm not sure
whether it is appropriate.
> 
>>
>> The other part I'm not completely sure about is how you want to
>> have MSIs map into normal IRQ descriptors. At the moment, all
>> MSI users are based on IRQ numbers, but this has known scalability
problems.
> 
> Hmmm, I still use the IRQ number to map the MSIs to IRQ description.
> I'm sorry, I don't understand you meaning.
> What are the scalability problems you mentioned ?
We have soft limitation of nr_irqs or hard limitation NR_IRQS,
we couldn't allocate as much irq number as we need in some cases,
such as to support MSI-x.
> For device drivers, they always process interrupt in two steps.
> If irq is the legacy interrupt, drivers will first
> use the irq_of_parse_and_map() or pci_enable_device() to parse and get the
IRQ number.
> Then drivers will call the request_irq() to register the interrupt handler.
> If irq is MSIs, first call pci_enable_msi/x() to get the IRQ number and
then call
> request_irq() to register interrupt handler.
> 
>> I wonder if we can do the interface in a way that
>> hides the interrupt number from generic device drivers and just
>> passes a 'struct irq_desc'. Note that there are long-term plans
to
>> get rid of IRQ numbers entirely, but those plans have existed for
>> a long time already without anybody seriously addressing the device
>> driver interfaces so far, so it might never really happen.
>>
> 
> Maybe this is a huge work, now hundreds drivers use the IRQ number, so
maybe we can consider
> this in a separate title.
> 
>>> struct msi_ops {
>>>         struct msi_desc *(*msi_setup_entry)(struct msi_irqs *msi,
struct msi_desc *entry);
>>>         int msix_setup_entries(struct msi_irqs *msi, struct
msix_entry *entries);
>>>         u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask, u32
flag);
>>>         u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
>>>         void (*msi_read_message)(struct msi_desc *desc, struct
msi_msg *msg);
>>>         void (*msi_write_message)(struct msi_desc *desc, struct
msi_msg *msg);
>>>         void (*msi_set_intx)(struct msi_irqs *msi, int enable);
>>> };
>>> struct msi_ops provides several hook functions, generic MSI driver
will call
>>> the hook functions to access device specific registers. PCI devices
will share
>>> the same msi_ops, because they have the same way to access MSI
hardware registers.
>>>
>>> Generic MSI layer export msi_capability_init() and
msix_capability_init() functions
>>> to drivers. msi/x_capability_init() will initialize MSI capability
data struct msi_desc
>>> and alloc the irq, then write the msi address/data value to
hardware registers.
>>>
>>> This series only did compile test, we will test it in x86 and arm
platform later.
>>
>> For the generic drivers, I don't see much point in differentiating
between
>> MSI and MSI-X, as I believe the difference is something internal to the
PCI
>> implementation.
> 
> Yes, we can integrate them, and use a generic ops, add a type in hook
function to
> differentiate them.
> 
>>
>> With the other operations, I think they should all take a 'struct
device *'
>> as the first argument for convenience and consistency. I don't
think you actually
>> need msi_read_message(), and we could avoid msi_write_message() by
doing it
>> the other way round.
>>
> 
> There only two functions use the read_msi_msg(), because every msi_desc has
> a struct msi_msg, and it caches the msi address and data. I will consider
to
> retrieve the msg from cached msi_msg, then we can avoid the
msi_read_message().
> But msi_write_message() maybe necessary, some xxx_set_affinity() functions
and
> restore functions need the msi_write_message() to rewrite the address and
data.
> 
>> What I'd envision as the API from the device driver perspective is
something
>> as simple like this:
>>
>> struct msi_desc *msi_request(struct msi_chip *chip, irq_handler_t
handler,
>> 			unsigned long flags, const char *name, struct device *dev);
>>
>> which would get an msi descriptor that is valid for this device (dev)
>> connected to a particular msi_chip, and associate a handler function
>> with it. The device driver can call that function and retrieve the
>> address/message pair from the msi_desc in order to store it in its own
>> device specific registers. The request_irq() can be handled internally
>> to msi_request().
> 
> This is a huge change for device drivers, and some device drivers don't
know which msi_chip
> their MSI irq deliver to. I'm reworking the msi_chip, and try to use
msi_chip to eliminate
> all arch_msi_xxx() under every arch in kernel. And the important point is
how to create the
> binding for the MSI device to the target msi_chip.
> 
> For PCI device, some arm platform already bound the msi_chip to the pci
hostbridge, then all
> pci devices under the pci hostbridge deliver their MSI irqs to the target
msi_chip.
> And other platform create the binding in DTS file, then the MSI device can
find their msi_chip
> by device_node.
> I don't know whether there are other situations, we should provide a
generic interface that
> every MSI device under every platform can use it to find its msi_chip
exactly.
> 
> 
> Thanks!
> Yijing.
> 
>>
>> .
>>
> 
>

On 2014/7/30 14:47, Jiang Liu wrote:
> 
> 
> On 2014/7/30 10:45, Yijing Wang wrote:
>> On 2014/7/29 22:08, Arnd Bergmann wrote:
>>> On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
>>>>         The series is a draft of generic MSI driver that
supports PCI
>>>> and Non-PCI device which have MSI capability. If you're not
interested
>>>> it, sorry for the noise.
>>>
>>> I've finally managed to take some time to look at the series.
Overall,
>>> the concept looks good to me, and the patches look very well
implemented.
>>>
>>> The part I'm not sure about is the interface we want to end up
with
>>> at the end of the series. More on that below
>>
>> Hi Arnd,
>>    Thanks for your review and comments very much!
>> Please refer the inline comments.
>>
>>>
>>>> The series is based on Linux-3.16-rc1.
>>>>
>>>> MSI was introduced in PCI Spec 2.2. Currently, kernel MSI 
>>>> driver codes are bonding with PCI device. Because MSI has a lot
>>>> advantages in design. More and more non-PCI devices want to
>>>> use MSI as their default interrupt. The existing MSI device
>>>> include HPET. HPET driver provide its own MSI code to
initialize
>>>> and process MSI interrupts. In the latest GIC v3 spec, legacy
device
>>>> can deliver MSI by the help of a relay device named
consolidator.
>>>> Consolidator can translate the legacy interrupts connected to
it
>>>> to MSI/MSI-X. And new non-PCI device will be designed to 
>>>> support MSI in future. So make the MSI driver code be generic
will
>>>> help the non-PCI device use MSI more simply.
>>>>
>>>> The new data struct for generic MSI driver.
>>>> struct msi_irqs {
>>>>         u8 msi_enabled:1; /* Enable flag */
>>>>         u8 msix_enabled:1;
>>>>         struct list_head msi_list; /* MSI desc list */
>>>>         void *data;     /* help to find the MSI device */
>>>>         struct msi_ops *ops; /* MSI device specific hook */
>>>> };
>>>> struct msi_irqs is used to manage MSI related informations.
Every device supports
>>>> MSI should contain this data struct and allocate it.
>>>
>>> I think you should have a stronger association with the 'struct
>>> device' here. Can you replace the 'void *data' with
'struct device *dev'?
>>
>> Actually, I used the struct device *dev in my first draft, finally, I
replaced
>> it with void *data, because some MSI devices don't have a struct
device *dev,
>> like the existing hpet device, dmar msi device, and OF device, like the
ARM consolidator.
>>
>> Of course, we can make the MSI devices have their own struct device,
and register to
>> device tree, eg, add a class device named MSI_DEV. But I'm not sure
whether it is appropriate.
>>
>>>
>>> The other part I'm not completely sure about is how you want to
>>> have MSIs map into normal IRQ descriptors. At the moment, all
>>> MSI users are based on IRQ numbers, but this has known scalability
problems.
>>
>> Hmmm, I still use the IRQ number to map the MSIs to IRQ description.
>> I'm sorry, I don't understand you meaning.
>> What are the scalability problems you mentioned ?
> We have soft limitation of nr_irqs or hard limitation NR_IRQS,
> we couldn't allocate as much irq number as we need in some cases,
> such as to support MSI-x.
Oh, yes, this is a potential issue. Gerry, thanks for you explanation. :)
> 
>> For device drivers, they always process interrupt in two steps.
>> If irq is the legacy interrupt, drivers will first
>> use the irq_of_parse_and_map() or pci_enable_device() to parse and get
the IRQ number.
>> Then drivers will call the request_irq() to register the interrupt
handler.
>> If irq is MSIs, first call pci_enable_msi/x() to get the IRQ number and
then call
>> request_irq() to register interrupt handler.
>>
>>> I wonder if we can do the interface in a way that
>>> hides the interrupt number from generic device drivers and just
>>> passes a 'struct irq_desc'. Note that there are long-term
plans to
>>> get rid of IRQ numbers entirely, but those plans have existed for
>>> a long time already without anybody seriously addressing the device
>>> driver interfaces so far, so it might never really happen.
>>>
>>
>> Maybe this is a huge work, now hundreds drivers use the IRQ number, so
maybe we can consider
>> this in a separate title.
>>
>>>> struct msi_ops {
>>>>         struct msi_desc *(*msi_setup_entry)(struct msi_irqs
*msi, struct msi_desc *entry);
>>>>         int msix_setup_entries(struct msi_irqs *msi, struct
msix_entry *entries);
>>>>         u32 (*msi_mask_irq)(struct msi_desc *desc, u32 mask,
u32 flag);
>>>>         u32 (*msix_mask_irq)(struct msi_desc *desc, u32 flag);
>>>>         void (*msi_read_message)(struct msi_desc *desc, struct
msi_msg *msg);
>>>>         void (*msi_write_message)(struct msi_desc *desc, struct
msi_msg *msg);
>>>>         void (*msi_set_intx)(struct msi_irqs *msi, int enable);
>>>> };
>>>> struct msi_ops provides several hook functions, generic MSI
driver will call
>>>> the hook functions to access device specific registers. PCI
devices will share
>>>> the same msi_ops, because they have the same way to access MSI
hardware registers.
>>>>
>>>> Generic MSI layer export msi_capability_init() and
msix_capability_init() functions
>>>> to drivers. msi/x_capability_init() will initialize MSI
capability data struct msi_desc
>>>> and alloc the irq, then write the msi address/data value to
hardware registers.
>>>>
>>>> This series only did compile test, we will test it in x86 and
arm platform later.
>>>
>>> For the generic drivers, I don't see much point in
differentiating between
>>> MSI and MSI-X, as I believe the difference is something internal to
the PCI
>>> implementation.
>>
>> Yes, we can integrate them, and use a generic ops, add a type in hook
function to
>> differentiate them.
>>
>>>
>>> With the other operations, I think they should all take a
'struct device *'
>>> as the first argument for convenience and consistency. I don't
think you actually
>>> need msi_read_message(), and we could avoid msi_write_message() by
doing it
>>> the other way round.
>>>
>>
>> There only two functions use the read_msi_msg(), because every msi_desc
has
>> a struct msi_msg, and it caches the msi address and data. I will
consider to
>> retrieve the msg from cached msi_msg, then we can avoid the
msi_read_message().
>> But msi_write_message() maybe necessary, some xxx_set_affinity()
functions and
>> restore functions need the msi_write_message() to rewrite the address
and data.
>>
>>> What I'd envision as the API from the device driver perspective
is something
>>> as simple like this:
>>>
>>> struct msi_desc *msi_request(struct msi_chip *chip, irq_handler_t
handler,
>>> 			unsigned long flags, const char *name, struct device *dev);
>>>
>>> which would get an msi descriptor that is valid for this device
(dev)
>>> connected to a particular msi_chip, and associate a handler
function
>>> with it. The device driver can call that function and retrieve the
>>> address/message pair from the msi_desc in order to store it in its
own
>>> device specific registers. The request_irq() can be handled
internally
>>> to msi_request().
>>
>> This is a huge change for device drivers, and some device drivers
don't know which msi_chip
>> their MSI irq deliver to. I'm reworking the msi_chip, and try to
use msi_chip to eliminate
>> all arch_msi_xxx() under every arch in kernel. And the important point
is how to create the
>> binding for the MSI device to the target msi_chip.
>>
>> For PCI device, some arm platform already bound the msi_chip to the pci
hostbridge, then all
>> pci devices under the pci hostbridge deliver their MSI irqs to the
target msi_chip.
>> And other platform create the binding in DTS file, then the MSI device
can find their msi_chip
>> by device_node.
>> I don't know whether there are other situations, we should provide
a generic interface that
>> every MSI device under every platform can use it to find its msi_chip
exactly.
>>
>>
>> Thanks!
>> Yijing.
>>
>>>
>>> .
>>>
>>
>>
> 
> .
> 

-- 
Thanks!
Yijing

On Wednesday 30 July 2014, Yijing Wang wrote:
> On 2014/7/29 22:08, Arnd Bergmann wrote:
> > On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
> >>
> >> The new data struct for generic MSI driver.
> >> struct msi_irqs {
> >>         u8 msi_enabled:1; /* Enable flag */
> >>         u8 msix_enabled:1;
> >>         struct list_head msi_list; /* MSI desc list */
> >>         void *data;     /* help to find the MSI device */
> >>         struct msi_ops *ops; /* MSI device specific hook */
> >> };
> >> struct msi_irqs is used to manage MSI related informations. Every
device supports
> >> MSI should contain this data struct and allocate it.
> > 
> > I think you should have a stronger association with the 'struct
> > device' here. Can you replace the 'void *data' with
'struct device *dev'?
> 
> Actually, I used the struct device *dev in my first draft, finally, I
replaced
> it with void *data, because some MSI devices don't have a struct device
*dev,
> like the existing hpet device, dmar msi device, and OF device, like the ARM
consolidator.
> 
> Of course, we can make the MSI devices have their own struct device, and
register to
> device tree, eg, add a class device named MSI_DEV. But I'm not sure
whether it is appropriate.
It doesn't have to be in the (OF) device tree, but I think it absolutely
makes
sense to use the 'struct device' infrastructure here, as almost
everything uses
a device, and the ones that don't do that today can be easily changed.
> > The other part I'm not completely sure about is how you want to
> > have MSIs map into normal IRQ descriptors. At the moment, all
> > MSI users are based on IRQ numbers, but this has known scalability
problems.
> 
> Hmmm, I still use the IRQ number to map the MSIs to IRQ description.
> I'm sorry, I don't understand you meaning.
> What are the scalability problems you mentioned ?
> For device drivers, they always process interrupt in two steps.
> If irq is the legacy interrupt, drivers will first
> use the irq_of_parse_and_map() or pci_enable_device() to parse and get the
IRQ number.
> Then drivers will call the request_irq() to register the interrupt handler.
> If irq is MSIs, first call pci_enable_msi/x() to get the IRQ number and
then call
> request_irq() to register interrupt handler.
The method you describe here makes sense for PCI devices that are required to
support
legacy interrupts and may or may not support MSI on a given system, but not so
much
for platform devices for which we know exactly whether we want to use MSI
or legacy interrupts.

In particular if you have a device that can only do MSI, the entire
pci_enable_msi
step is pointless: all we need to do is program the correct MSI target
address/message
pair into the device and register the handler.
> > I wonder if we can do the interface in a way that
> > hides the interrupt number from generic device drivers and just
> > passes a 'struct irq_desc'. Note that there are long-term
plans to
> > get rid of IRQ numbers entirely, but those plans have existed for
> > a long time already without anybody seriously addressing the device
> > driver interfaces so far, so it might never really happen.
> > 
> 
> Maybe this is a huge work, now hundreds drivers use the IRQ number, so
maybe we can consider
> this in a separate title.
Sorry for being unclear here: I did suggest changing all drivers now. What I
meant
is that we use a different API for non-PCI devices that works without IRQ
numbers.
I don't think we should touch the PCI interfaces at this point.
> > With the other operations, I think they should all take a 'struct
device *'
> > as the first argument for convenience and consistency. I don't
think you actually
> > need msi_read_message(), and we could avoid msi_write_message() by
doing it
> > the other way round.
> > 
> 
> There only two functions use the read_msi_msg(), because every msi_desc has
> a struct msi_msg, and it caches the msi address and data. I will consider
to
> retrieve the msg from cached msi_msg, then we can avoid the
msi_read_message().
> But msi_write_message() maybe necessary, some xxx_set_affinity() functions
and
> restore functions need the msi_write_message() to rewrite the address and
data.
Makes sense. I'd have to think about it more, but I think you are right
about the affinity APIs needing this.
> > What I'd envision as the API from the device driver perspective is
something
> > as simple like this:
> > 
> > struct msi_desc *msi_request(struct msi_chip *chip, irq_handler_t
handler,
> > 			unsigned long flags, const char *name, struct device *dev);
> > 
> > which would get an msi descriptor that is valid for this device (dev)
> > connected to a particular msi_chip, and associate a handler function
> > with it. The device driver can call that function and retrieve the
> > address/message pair from the msi_desc in order to store it in its own
> > device specific registers. The request_irq() can be handled internally
> > to msi_request().
> 
> This is a huge change for device drivers, and some device drivers don't
know which msi_chip
> their MSI irq deliver to. I'm reworking the msi_chip, and try to use
msi_chip to eliminate
> all arch_msi_xxx() under every arch in kernel. And the important point is
how to create the
> binding for the MSI device to the target msi_chip.
Which drivers are you thinking of? Again, I wouldn't expect to change any
PCI drivers,
but only platform drivers that do native MSI, so we only have to change drivers
that
do not support any MSI at all yet and that need to be changed anyway in order to
add
support.
> For PCI device, some arm platform already bound the msi_chip to the pci
hostbridge, then all
> pci devices under the pci hostbridge deliver their MSI irqs to the target
msi_chip.
> And other platform create the binding in DTS file, then the MSI device can
find their msi_chip
> by device_node.
> I don't know whether there are other situations, we should provide a
generic interface that
> every MSI device under every platform can use it to find its msi_chip
exactly.
We have introduced the "msi-parent" property to mirror the
"interrupt-parent" property.
For the PCI case, this property is only needed in the PCI host controller, and
there
can be a system-wide default, by putting the "msi-parent" property
into the root device
node or the node of a bus that is parent to all devices supporting MSI.

For non-PCI devices, it should be possible to override the
"msi-parent" property per
device, but those can also use the global property.

The main use case that I see are PCI host controllers that have their own MSI
catcher
included, so meaning that any PCI device can either send its MSIs there, or to a
system-wide GICv3 instance, and we need a way to select which one.

A degenerate case of this would be a system where a PCI device sends its MSI
into
the host controller, that generates a legacy interrupt and that in turn gets 
sent to an irqchip which turns it back into an MSI for the GICv3. This would of
course be very inefficient, but I think we should be able to express this with
both the binding and the in-kernel framework just to be on the safe side.


	Arnd

On 2014/8/1 21:52, Arnd Bergmann wrote:
> On Wednesday 30 July 2014, Yijing Wang wrote:
>> On 2014/7/29 22:08, Arnd Bergmann wrote:
>>> On Saturday 26 July 2014 11:08:37 Yijing Wang wrote:
>>>>
>>>> The new data struct for generic MSI driver.
>>>> struct msi_irqs {
>>>>         u8 msi_enabled:1; /* Enable flag */
>>>>         u8 msix_enabled:1;
>>>>         struct list_head msi_list; /* MSI desc list */
>>>>         void *data;     /* help to find the MSI device */
>>>>         struct msi_ops *ops; /* MSI device specific hook */
>>>> };
>>>> struct msi_irqs is used to manage MSI related informations.
Every device supports
>>>> MSI should contain this data struct and allocate it.
>>>
>>> I think you should have a stronger association with the 'struct
>>> device' here. Can you replace the 'void *data' with
'struct device *dev'?
>>
>> Actually, I used the struct device *dev in my first draft, finally, I
replaced
>> it with void *data, because some MSI devices don't have a struct
device *dev,
>> like the existing hpet device, dmar msi device, and OF device, like the
ARM consolidator.
>>
>> Of course, we can make the MSI devices have their own struct device,
and register to
>> device tree, eg, add a class device named MSI_DEV. But I'm not sure
whether it is appropriate.
> 
> It doesn't have to be in the (OF) device tree, but I think it
absolutely makes
> sense to use the 'struct device' infrastructure here, as almost
everything uses
> a device, and the ones that don't do that today can be easily changed.
I will try to use "struct device" infrastructure, thanks for your
suggestion. :)
> 
>>> The other part I'm not completely sure about is how you want to
>>> have MSIs map into normal IRQ descriptors. At the moment, all
>>> MSI users are based on IRQ numbers, but this has known scalability
problems.
>>
>> Hmmm, I still use the IRQ number to map the MSIs to IRQ description.
>> I'm sorry, I don't understand you meaning.
>> What are the scalability problems you mentioned ?
>> For device drivers, they always process interrupt in two steps.
>> If irq is the legacy interrupt, drivers will first
>> use the irq_of_parse_and_map() or pci_enable_device() to parse and get
the IRQ number.
>> Then drivers will call the request_irq() to register the interrupt
handler.
>> If irq is MSIs, first call pci_enable_msi/x() to get the IRQ number and
then call
>> request_irq() to register interrupt handler.
> 
> The method you describe here makes sense for PCI devices that are required
to support
> legacy interrupts and may or may not support MSI on a given system, but not
so much
> for platform devices for which we know exactly whether we want to use MSI
> or legacy interrupts.
> 
> In particular if you have a device that can only do MSI, the entire
pci_enable_msi
> step is pointless: all we need to do is program the correct MSI target
address/message
> pair into the device and register the handler.
Yes, I almost agree if we won't change the existing hundreds of drivers,
what
I worried about is some drivers may want to know the IRQ numbers, and use the
IRQ
number to process different things, as I mentioned in another reply.
But we can also provide the interface which integrate MSI configuration and
request_irq(),
if most drivers don't care the IRQ number.
> 
>>> I wonder if we can do the interface in a way that
>>> hides the interrupt number from generic device drivers and just
>>> passes a 'struct irq_desc'. Note that there are long-term
plans to
>>> get rid of IRQ numbers entirely, but those plans have existed for
>>> a long time already without anybody seriously addressing the device
>>> driver interfaces so far, so it might never really happen.
>>>
>>
>> Maybe this is a huge work, now hundreds drivers use the IRQ number, so
maybe we can consider
>> this in a separate title.
> 
> Sorry for being unclear here: I did suggest changing all drivers now. What
I meant
> is that we use a different API for non-PCI devices that works without IRQ
numbers.
> I don't think we should touch the PCI interfaces at this point.
OK, I got it.
>>> What I'd envision as the API from the device driver perspective
is something
>>> as simple like this:
>>>
>>> struct msi_desc *msi_request(struct msi_chip *chip, irq_handler_t
handler,
>>> 			unsigned long flags, const char *name, struct device *dev);
>>>
>>> which would get an msi descriptor that is valid for this device
(dev)
>>> connected to a particular msi_chip, and associate a handler
function
>>> with it. The device driver can call that function and retrieve the
>>> address/message pair from the msi_desc in order to store it in its
own
>>> device specific registers. The request_irq() can be handled
internally
>>> to msi_request().
>>
>> This is a huge change for device drivers, and some device drivers
don't know which msi_chip
>> their MSI irq deliver to. I'm reworking the msi_chip, and try to
use msi_chip to eliminate
>> all arch_msi_xxx() under every arch in kernel. And the important point
is how to create the
>> binding for the MSI device to the target msi_chip.
> 
> Which drivers are you thinking of? Again, I wouldn't expect to change
any PCI drivers,
> but only platform drivers that do native MSI, so we only have to change
drivers that
> do not support any MSI at all yet and that need to be changed anyway in
order to add
> support.
I mean platform device drivers, because we can find the target msi_chip by some
platform
interfaces(like the existing of_pci_find_msi_chip_by_node()). So we no need to
explicitly provide
the msi_chip as the function argument.
> 
>> For PCI device, some arm platform already bound the msi_chip to the pci
hostbridge, then all
>> pci devices under the pci hostbridge deliver their MSI irqs to the
target msi_chip.
>> And other platform create the binding in DTS file, then the MSI device
can find their msi_chip
>> by device_node.
>> I don't know whether there are other situations, we should provide
a generic interface that
>> every MSI device under every platform can use it to find its msi_chip
exactly.
> 
> We have introduced the "msi-parent" property to mirror the
"interrupt-parent" property.
> For the PCI case, this property is only needed in the PCI host controller,
and there
> can be a system-wide default, by putting the "msi-parent"
property into the root device
> node or the node of a bus that is parent to all devices supporting MSI.
> 
> For non-PCI devices, it should be possible to override the
"msi-parent" property per
> device, but those can also use the global property.
> 
> The main use case that I see are PCI host controllers that have their own
MSI catcher
> included, so meaning that any PCI device can either send its MSIs there, or
to a
> system-wide GICv3 instance, and we need a way to select which one.
Yes, agree.
> 
> A degenerate case of this would be a system where a PCI device sends its
MSI into
> the host controller, that generates a legacy interrupt and that in turn
gets
> sent to an irqchip which turns it back into an MSI for the GICv3. This
would of
> course be very inefficient, but I think we should be able to express this
with
> both the binding and the in-kernel framework just to be on the safe side.
Yes, the best way to tell the kernel which msi_chip should deliver to is
describe
the binding in DTS file. If a real degenerate case found, we can update the
platform
interface which is responsible for getting the match msi_chip in future.
> 
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> .
> 

-- 
Thanks!
Yijing