[Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend

Cédric Le Goater posted 3 patches 7 years, 5 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
include/hw/ppc/spapr_irq.h |  2 ++
include/hw/ppc/xics.h      |  2 --
hw/ppc/spapr.c             |  1 +
hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
hw/ppc/spapr_pci.c         |  5 +++--
5 files changed, 26 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend
Posted by Cédric Le Goater 7 years, 5 months ago
Hello,

This series adds a new sPAPRIrq backend increasing the number of
available IRQ numbers in pseries-3.1 machines. This change is an
opportunity to also fix the "ibm,pe-total-#msi" and remove the old
XICS_IRQS_SPAPR definition.

Thanks,

C.

Cédric Le Goater (3):
  spapr: introduce a spapr_irq class 'nr_msis' attribute
  spapr: increase the size of the IRQ number space
  spapr_pci: fix "ibm,pe-total-#msi" value

 include/hw/ppc/spapr_irq.h |  2 ++
 include/hw/ppc/xics.h      |  2 --
 hw/ppc/spapr.c             |  1 +
 hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
 hw/ppc/spapr_pci.c         |  5 +++--
 5 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.17.1


Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend
Posted by Greg Kurz 7 years, 5 months ago
On Mon, 10 Sep 2018 13:02:19 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Hello,
> 
> This series adds a new sPAPRIrq backend increasing the number of
> available IRQ numbers in pseries-3.1 machines. This change is an
> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
> XICS_IRQS_SPAPR definition.
> 

This cleanup looks sane but I still have a concern with the semantics of
"ibm,pe-total-#msi".

According to LoPAPR:

"ibm,pe-total-#msi"

property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
RTAS call.
prop-encoded-array: Maximum number of interrupts encoded as with encode-int.

IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.

But we currently have only one global allocator in the machine, so having
each PHB advertising the full range of the allocator still looks weird.

Shouldn't this be divided by the number of PHBs ? Or should we have one
separate allocator for each PHB ?

> Thanks,
> 
> C.
> 
> Cédric Le Goater (3):
>   spapr: introduce a spapr_irq class 'nr_msis' attribute
>   spapr: increase the size of the IRQ number space
>   spapr_pci: fix "ibm,pe-total-#msi" value
> 
>  include/hw/ppc/spapr_irq.h |  2 ++
>  include/hw/ppc/xics.h      |  2 --
>  hw/ppc/spapr.c             |  1 +
>  hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
>  hw/ppc/spapr_pci.c         |  5 +++--
>  5 files changed, 26 insertions(+), 6 deletions(-)
> 


Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend
Posted by Cédric Le Goater 7 years, 5 months ago
On 09/10/2018 05:02 PM, Greg Kurz wrote:
> On Mon, 10 Sep 2018 13:02:19 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> Hello,
>>
>> This series adds a new sPAPRIrq backend increasing the number of
>> available IRQ numbers in pseries-3.1 machines. This change is an
>> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
>> XICS_IRQS_SPAPR definition.
>>
> 
> This cleanup looks sane but I still have a concern with the semantics of
> "ibm,pe-total-#msi".
> 
> According to LoPAPR:
> 
> "ibm,pe-total-#msi"
> 
> property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
> to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
> ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
> RTAS call.
> prop-encoded-array: Maximum number of interrupts encoded as with encode-int.
> 
> IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.
> 
> But we currently have only one global allocator in the machine, so having
> each PHB advertising the full range of the allocator still looks weird.

yes. Multiple PHBs share the same IRQ number space and in this
case the advertised number "ibm,pe-total-#msi" does not reflect 
the maximum number of allocatable interrupts per PHB.

The patch improves only the value for one PHB and, as of today, 
it is still wrong when Multiple PHBs are involved.

> Shouldn't this be divided by the number of PHBs ? Or should we have one
> separate allocator for each PHB ?

That would mean segmenting the IRQ number space and I am not 
fond of this solution because we have plenty of space to share:

	0xd00 MSIs

When we find a scenario reaching this limit, I think what we 
should do is to dynamically extend the IRQ number space in QEMU 
and in KVM. It should not be a problem.

We could also downsize "ibm,pe-total-#msi". It is quite big
today. some controllers do have a lot of IRQs but no more 
that a hundred. I might be wrong. 

C.

>> Thanks,
>>
>> C.
>>
>> Cédric Le Goater (3):
>>   spapr: introduce a spapr_irq class 'nr_msis' attribute
>>   spapr: increase the size of the IRQ number space
>>   spapr_pci: fix "ibm,pe-total-#msi" value
>>
>>  include/hw/ppc/spapr_irq.h |  2 ++
>>  include/hw/ppc/xics.h      |  2 --
>>  hw/ppc/spapr.c             |  1 +
>>  hw/ppc/spapr_irq.c         | 22 ++++++++++++++++++++--
>>  hw/ppc/spapr_pci.c         |  5 +++--
>>  5 files changed, 26 insertions(+), 6 deletions(-)
>>
> 


Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend
Posted by David Gibson 7 years, 5 months ago
On Mon, Sep 10, 2018 at 07:24:47PM +0200, Cédric Le Goater wrote:
> On 09/10/2018 05:02 PM, Greg Kurz wrote:
> > On Mon, 10 Sep 2018 13:02:19 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> Hello,
> >>
> >> This series adds a new sPAPRIrq backend increasing the number of
> >> available IRQ numbers in pseries-3.1 machines. This change is an
> >> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
> >> XICS_IRQS_SPAPR definition.
> >>
> > 
> > This cleanup looks sane but I still have a concern with the semantics of
> > "ibm,pe-total-#msi".
> > 
> > According to LoPAPR:
> > 
> > "ibm,pe-total-#msi"
> > 
> > property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
> > to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
> > ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
> > RTAS call.
> > prop-encoded-array: Maximum number of interrupts encoded as with encode-int.
> > 
> > IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.
> > 
> > But we currently have only one global allocator in the machine, so having
> > each PHB advertising the full range of the allocator still looks weird.
> 
> yes. Multiple PHBs share the same IRQ number space and in this
> case the advertised number "ibm,pe-total-#msi" does not reflect 
> the maximum number of allocatable interrupts per PHB.
> 
> The patch improves only the value for one PHB and, as of today, 
> it is still wrong when Multiple PHBs are involved.
> 
> > Shouldn't this be divided by the number of PHBs ? Or should we have one
> > separate allocator for each PHB ?
> 
> That would mean segmenting the IRQ number space and I am not 
> fond of this solution because we have plenty of space to share:
> 
> 	0xd00 MSIs
> 
> When we find a scenario reaching this limit, I think what we 
> should do is to dynamically extend the IRQ number space in QEMU 
> and in KVM. It should not be a problem.
> 
> We could also downsize "ibm,pe-total-#msi". It is quite big
> today. some controllers do have a lot of IRQs but no more 
> that a hundred. I might be wrong.

Yeah, this is my take as well.  The spec sort of implies, but doesn't
explicitly state a per-PHB allocation of MSIs.  But there's not really
a good reason to partition the irqs that way.  So it may be a bit fast
and loose w.r.t. PAPR, but as long as we have plenty of MSIs available
I think using a shared pool is unlikely to cause problems in practice.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH 0/3] spapr: introduce a new sPAPRIrq backend
Posted by Greg Kurz 7 years, 5 months ago
On Tue, 11 Sep 2018 11:52:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Sep 10, 2018 at 07:24:47PM +0200, Cédric Le Goater wrote:
> > On 09/10/2018 05:02 PM, Greg Kurz wrote:  
> > > On Mon, 10 Sep 2018 13:02:19 +0200
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > >> Hello,
> > >>
> > >> This series adds a new sPAPRIrq backend increasing the number of
> > >> available IRQ numbers in pseries-3.1 machines. This change is an
> > >> opportunity to also fix the "ibm,pe-total-#msi" and remove the old
> > >> XICS_IRQS_SPAPR definition.
> > >>  
> > > 
> > > This cleanup looks sane but I still have a concern with the semantics of
> > > "ibm,pe-total-#msi".
> > > 
> > > According to LoPAPR:
> > > 
> > > "ibm,pe-total-#msi"
> > > 
> > > property name defines the maximum number of Message Signaled Interrupts (MSI plus MSI-X) that are available
> > > to the PE below this device tree node. This number only indicates the number of available interrupts, not the num-
> > > ber assigned. The number assigned for an IOA may be obtained by Function 0 (Query only) of the ibm,change-msi
> > > RTAS call.
> > > prop-encoded-array: Maximum number of interrupts encoded as with encode-int.
> > > 
> > > IIUC, the PHB is given ibm,pe-total-#msi MSIs that it can assign to devices.
> > > 
> > > But we currently have only one global allocator in the machine, so having
> > > each PHB advertising the full range of the allocator still looks weird.  
> > 
> > yes. Multiple PHBs share the same IRQ number space and in this
> > case the advertised number "ibm,pe-total-#msi" does not reflect 
> > the maximum number of allocatable interrupts per PHB.
> > 
> > The patch improves only the value for one PHB and, as of today, 
> > it is still wrong when Multiple PHBs are involved.
> >   
> > > Shouldn't this be divided by the number of PHBs ? Or should we have one
> > > separate allocator for each PHB ?  
> > 
> > That would mean segmenting the IRQ number space and I am not 
> > fond of this solution because we have plenty of space to share:
> > 
> > 	0xd00 MSIs
> > 
> > When we find a scenario reaching this limit, I think what we 
> > should do is to dynamically extend the IRQ number space in QEMU 
> > and in KVM. It should not be a problem.
> > 
> > We could also downsize "ibm,pe-total-#msi". It is quite big
> > today. some controllers do have a lot of IRQs but no more 
> > that a hundred. I might be wrong.  
> 
> Yeah, this is my take as well.  The spec sort of implies, but doesn't
> explicitly state a per-PHB allocation of MSIs.  But there's not really
> a good reason to partition the irqs that way.  So it may be a bit fast
> and loose w.r.t. PAPR, but as long as we have plenty of MSIs available
> I think using a shared pool is unlikely to cause problems in practice.
> 

Fair enough. Thanks for the clarification.

Cheers,

--
Greg