Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated

[Cédric Le Goater]

On 06/13/2018 06:27 AM, David Gibson wrote:
> On Tue, Jun 05, 2018 at 08:31:49AM +0200, Cédric Le Goater wrote:
>> On 06/05/2018 05:44 AM, David Gibson wrote:
>>> On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
 On Fri, 18 May 2018 18:44:03 +0200
 Cédric Le Goater  wrote:

> PCI LSIs are today allocated one by one using the IRQ alloc_block
> routine. Change the code sequence to first allocate a PCI_NUM_PINS
> block. It will help us providing a generic IRQ framework to the
> machine.
>
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/ppc/spapr_pci.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 39a14980d397..4fd97ffe4c6e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, 
> Error **errp)
>  sPAPRTCETable *tcet;
>  const unsigned windows_supported =
>  sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> +uint32_t irq;
> +Error *local_err = NULL;
>  
>  if (!spapr) {
>  error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
> machine");
> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, 
> Error **errp)
>  QLIST_INSERT_HEAD(>phbs, sphb, list);
>  
>  /* Initialize the LSI table */
> -for (i = 0; i < PCI_NUM_PINS; i++) {
> -uint32_t irq;
> -Error *local_err = NULL;
> -
> -irq = spapr_irq_alloc_block(spapr, 1, true, false, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -error_prepend(errp, "can't allocate LSIs: ");
> -return;
> -}
> +irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, 
> _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "can't allocate LSIs: ");
> +return;
> +}
>  

 It isn't strictly equivalent. The current code would be happy with
 sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
 cannot happen since we don't have PHB hotplug.
>>>
>>> This makes me pretty nervous, because it's not obvious it will come up
>>> with the same numbers in all circumstances, which we have to do for
>>> existing machine types.
>>
>> Given that : 
>>
>>  - irq_hint is "unused"
>>  - all IRQs are allocated sequentially at machine init,  
>>  - spapr_pci is the only model using the block allocation for MSIs, 
>>potentially fragmenting more the IRQ number space but done at 
>>guest runtime. 
>>  - the PHB LSI are the allocated at realize time doing the loop above, 
>>  - we don't support PHB hotplug 
>>  - we do support PHB coldplug but then the IRQ allocation is done
>>at machine time,
>>
>> it seems highly improbable that the IRQ number space is fragmented
>> to a point which would not allow the loop above to return four 
>> contiguous IRQ numbers, always.
> 
> Well, assuming irq_hint really is unused, that's right.  But we can't
> assume that - that's the whole point of the deprecation thing.
> 
> Given that, AIUI, just one vio device with irq= set to a value that
> would be within an LSI block allocated under the old scheme would
> result in the new scheme returning a non-contiguous set of LSIs -
> i.e. a different result from what we used to have.
> 
>> That is why I felt confident changing the loop to a single block 
>> allocation. 
>>
>>> It's also not obvious to me why it's useful
>>> to go via this step before going straight to static allocation of the
>>> irq numbers.
>>
>> It pollutes the new sPAPR IRQ interface API with an extra parameter 
>> to support both underlying backend and it complexifies the code 
>> to handle block allocation of a single IRQ (like above) within an 
>> IRQ range (the PCI LSIs).
>>
>> So you end up having a family, a device index, a count, an alignment,
>> and an index within the range. pffut.
>>
>> Also, could we kill the alignment ?
> 
> Since we sometimes pass 'true', no, we can't, without changing the
> existing pattern of allocations, which we can't do.

To be honest, this is very much discouraging. 

C. 

Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated

[David Gibson]

On Tue, Jun 05, 2018 at 08:31:49AM +0200, Cédric Le Goater wrote:
> On 06/05/2018 05:44 AM, David Gibson wrote:
> > On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
> >> On Fri, 18 May 2018 18:44:03 +0200
> >> Cédric Le Goater  wrote:
> >>
> >>> PCI LSIs are today allocated one by one using the IRQ alloc_block
> >>> routine. Change the code sequence to first allocate a PCI_NUM_PINS
> >>> block. It will help us providing a generic IRQ framework to the
> >>> machine.
> >>>
> >>> Signed-off-by: Cédric Le Goater 
> >>> ---
> >>>  hw/ppc/spapr_pci.c | 21 ++---
> >>>  1 file changed, 10 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >>> index 39a14980d397..4fd97ffe4c6e 100644
> >>> --- a/hw/ppc/spapr_pci.c
> >>> +++ b/hw/ppc/spapr_pci.c
> >>> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, 
> >>> Error **errp)
> >>>  sPAPRTCETable *tcet;
> >>>  const unsigned windows_supported =
> >>>  sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> >>> +uint32_t irq;
> >>> +Error *local_err = NULL;
> >>>  
> >>>  if (!spapr) {
> >>>  error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
> >>> machine");
> >>> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, 
> >>> Error **errp)
> >>>  QLIST_INSERT_HEAD(>phbs, sphb, list);
> >>>  
> >>>  /* Initialize the LSI table */
> >>> -for (i = 0; i < PCI_NUM_PINS; i++) {
> >>> -uint32_t irq;
> >>> -Error *local_err = NULL;
> >>> -
> >>> -irq = spapr_irq_alloc_block(spapr, 1, true, false, _err);
> >>> -if (local_err) {
> >>> -error_propagate(errp, local_err);
> >>> -error_prepend(errp, "can't allocate LSIs: ");
> >>> -return;
> >>> -}
> >>> +irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, 
> >>> _err);
> >>> +if (local_err) {
> >>> +error_propagate(errp, local_err);
> >>> +error_prepend(errp, "can't allocate LSIs: ");
> >>> +return;
> >>> +}
> >>>  
> >>
> >> It isn't strictly equivalent. The current code would be happy with
> >> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
> >> cannot happen since we don't have PHB hotplug.
> > 
> > This makes me pretty nervous, because it's not obvious it will come up
> > with the same numbers in all circumstances, which we have to do for
> > existing machine types.
> 
> Given that : 
> 
>  - irq_hint is "unused"
>  - all IRQs are allocated sequentially at machine init,  
>  - spapr_pci is the only model using the block allocation for MSIs, 
>potentially fragmenting more the IRQ number space but done at 
>guest runtime. 
>  - the PHB LSI are the allocated at realize time doing the loop above, 
>  - we don't support PHB hotplug 
>  - we do support PHB coldplug but then the IRQ allocation is done
>at machine time,
> 
> it seems highly improbable that the IRQ number space is fragmented
> to a point which would not allow the loop above to return four 
> contiguous IRQ numbers, always.

Well, assuming irq_hint really is unused, that's right.  But we can't
assume that - that's the whole point of the deprecation thing.

Given that, AIUI, just one vio device with irq= set to a value that
would be within an LSI block allocated under the old scheme would
result in the new scheme returning a non-contiguous set of LSIs -
i.e. a different result from what we used to have.

> That is why I felt confident changing the loop to a single block 
> allocation. 
> 
> > It's also not obvious to me why it's useful
> > to go via this step before going straight to static allocation of the
> > irq numbers.
> 
> It pollutes the new sPAPR IRQ interface API with an extra parameter 
> to support both underlying backend and it complexifies the code 
> to handle block allocation of a single IRQ (like above) within an 
> IRQ range (the PCI LSIs).
> 
> So you end up having a family, a device index, a count, an alignment,
> and an index within the range. pffut.
> 
> Also, could we kill the alignment ?

Since we sometimes pass 'true', no, we can't, without changing the
existing pattern of allocations, which we can't do.
> 

-- 
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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated

[Cédric Le Goater]

On 06/05/2018 05:44 AM, David Gibson wrote:
> On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
>> On Fri, 18 May 2018 18:44:03 +0200
>> Cédric Le Goater  wrote:
>>
>>> PCI LSIs are today allocated one by one using the IRQ alloc_block
>>> routine. Change the code sequence to first allocate a PCI_NUM_PINS
>>> block. It will help us providing a generic IRQ framework to the
>>> machine.
>>>
>>> Signed-off-by: Cédric Le Goater 
>>> ---
>>>  hw/ppc/spapr_pci.c | 21 ++---
>>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 39a14980d397..4fd97ffe4c6e 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error 
>>> **errp)
>>>  sPAPRTCETable *tcet;
>>>  const unsigned windows_supported =
>>>  sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
>>> +uint32_t irq;
>>> +Error *local_err = NULL;
>>>  
>>>  if (!spapr) {
>>>  error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
>>> machine");
>>> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, 
>>> Error **errp)
>>>  QLIST_INSERT_HEAD(>phbs, sphb, list);
>>>  
>>>  /* Initialize the LSI table */
>>> -for (i = 0; i < PCI_NUM_PINS; i++) {
>>> -uint32_t irq;
>>> -Error *local_err = NULL;
>>> -
>>> -irq = spapr_irq_alloc_block(spapr, 1, true, false, _err);
>>> -if (local_err) {
>>> -error_propagate(errp, local_err);
>>> -error_prepend(errp, "can't allocate LSIs: ");
>>> -return;
>>> -}
>>> +irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, 
>>> _err);
>>> +if (local_err) {
>>> +error_propagate(errp, local_err);
>>> +error_prepend(errp, "can't allocate LSIs: ");
>>> +return;
>>> +}
>>>  
>>
>> It isn't strictly equivalent. The current code would be happy with
>> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
>> cannot happen since we don't have PHB hotplug.
> 
> This makes me pretty nervous, because it's not obvious it will come up
> with the same numbers in all circumstances, which we have to do for
> existing machine types.

Given that : 

 - irq_hint is "unused"
 - all IRQs are allocated sequentially at machine init,  
 - spapr_pci is the only model using the block allocation for MSIs, 
   potentially fragmenting more the IRQ number space but done at 
   guest runtime. 
 - the PHB LSI are the allocated at realize time doing the loop above, 
 - we don't support PHB hotplug 
 - we do support PHB coldplug but then the IRQ allocation is done
   at machine time,

it seems highly improbable that the IRQ number space is fragmented
to a point which would not allow the loop above to return four 
contiguous IRQ numbers, always. 

That is why I felt confident changing the loop to a single block 
allocation. 

> It's also not obvious to me why it's useful
> to go via this step before going straight to static allocation of the
> irq numbers.

It pollutes the new sPAPR IRQ interface API with an extra parameter 
to support both underlying backend and it complexifies the code 
to handle block allocation of a single IRQ (like above) within an 
IRQ range (the PCI LSIs).

So you end up having a family, a device index, a count, an alignment,
and an index within the range. pffut.

Also, could we kill the alignment ?

C.  

> If you can convince me this will (in practice) return the same numbers
> as the existing code for all valid setups, and that it's a useful
> intermediate step, then I'll apply it.
> 
>>
>>> -sphb->lsi_table[i].irq = irq;
>>> +for (i = 0; i < PCI_NUM_PINS; i++) {
>>> +sphb->lsi_table[i].irq = irq + i;
>>>  }
>>>  
>>>  /* allocate connectors for child PCI devices */
>>
> 

Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated

[David Gibson]

On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
> On Fri, 18 May 2018 18:44:03 +0200
> Cédric Le Goater  wrote:
> 
> > PCI LSIs are today allocated one by one using the IRQ alloc_block
> > routine. Change the code sequence to first allocate a PCI_NUM_PINS
> > block. It will help us providing a generic IRQ framework to the
> > machine.
> > 
> > Signed-off-by: Cédric Le Goater 
> > ---
> >  hw/ppc/spapr_pci.c | 21 ++---
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 39a14980d397..4fd97ffe4c6e 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> > **errp)
> >  sPAPRTCETable *tcet;
> >  const unsigned windows_supported =
> >  sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> > +uint32_t irq;
> > +Error *local_err = NULL;
> >  
> >  if (!spapr) {
> >  error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
> > machine");
> > @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, 
> > Error **errp)
> >  QLIST_INSERT_HEAD(>phbs, sphb, list);
> >  
> >  /* Initialize the LSI table */
> > -for (i = 0; i < PCI_NUM_PINS; i++) {
> > -uint32_t irq;
> > -Error *local_err = NULL;
> > -
> > -irq = spapr_irq_alloc_block(spapr, 1, true, false, _err);
> > -if (local_err) {
> > -error_propagate(errp, local_err);
> > -error_prepend(errp, "can't allocate LSIs: ");
> > -return;
> > -}
> > +irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, 
> > _err);
> > +if (local_err) {
> > +error_propagate(errp, local_err);
> > +error_prepend(errp, "can't allocate LSIs: ");
> > +return;
> > +}
> >  
> 
> It isn't strictly equivalent. The current code would be happy with
> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
> cannot happen since we don't have PHB hotplug.

This makes me pretty nervous, because it's not obvious it will come up
with the same numbers in all circumstances, which we have to do for
existing machine types.  It's also not obvious to me why it's useful
to go via this step before going straight to static allocation of the
irq numbers.

If you can convince me this will (in practice) return the same numbers
as the existing code for all valid setups, and that it's a useful
intermediate step, then I'll apply it.

> 
> > -sphb->lsi_table[i].irq = irq;
> > +for (i = 0; i < PCI_NUM_PINS; i++) {
> > +sphb->lsi_table[i].irq = irq + i;
> >  }
> >  
> >  /* allocate connectors for child PCI devices */
> 

-- 
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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated

[Greg Kurz]

On Fri, 18 May 2018 18:44:03 +0200
Cédric Le Goater  wrote:

> PCI LSIs are today allocated one by one using the IRQ alloc_block
> routine. Change the code sequence to first allocate a PCI_NUM_PINS
> block. It will help us providing a generic IRQ framework to the
> machine.
> 
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/ppc/spapr_pci.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 39a14980d397..4fd97ffe4c6e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  sPAPRTCETable *tcet;
>  const unsigned windows_supported =
>  sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> +uint32_t irq;
> +Error *local_err = NULL;
>  
>  if (!spapr) {
>  error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
> machine");
> @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>  QLIST_INSERT_HEAD(>phbs, sphb, list);
>  
>  /* Initialize the LSI table */
> -for (i = 0; i < PCI_NUM_PINS; i++) {
> -uint32_t irq;
> -Error *local_err = NULL;
> -
> -irq = spapr_irq_alloc_block(spapr, 1, true, false, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -error_prepend(errp, "can't allocate LSIs: ");
> -return;
> -}
> +irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, 
> _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +error_prepend(errp, "can't allocate LSIs: ");
> +return;
> +}
>  

It isn't strictly equivalent. The current code would be happy with
sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
cannot happen since we don't have PHB hotplug.

Reviewed-by: Greg Kurz 

> -sphb->lsi_table[i].irq = irq;
> +for (i = 0; i < PCI_NUM_PINS; i++) {
> +sphb->lsi_table[i].irq = irq + i;
>  }
>  
>  /* allocate connectors for child PCI devices */