Re: [PATCH] spapr/xive: Use xive_source_esb_len()

[David Gibson]

On Thu, Aug 13, 2020 at 05:38:59PM -0300, Gustavo Romero wrote:
> Hi Greg,
> 
> On 8/13/20 2:28 PM, Greg Kurz wrote:
> > static inline size_t xive_source_esb_len(XiveSource *xsrc)
> > {
> >      return (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> > }
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > Follow-up on "ppc/xive: Rework setup of XiveSource::esb_mmio"
> > 159679992680.876294.7520540158586170894.stgit@bahia.lan/">http://patchwork.ozlabs.org/project/qemu-devel/patch/159679992680.876294.7520540158586170894.stgit@bahia.lan/
> > ---
> >   hw/intc/spapr_xive.c     |    2 +-
> >   hw/intc/spapr_xive_kvm.c |    2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 3c84f64dc464..4bd0d606ba17 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -336,7 +336,7 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> > **errp)
> >       sysbus_init_mmio(SYS_BUS_DEVICE(xive), &end_xsrc->esb_mmio);
> >       /* Set the mapping address of the END ESB pages after the source ESBs 
> > */
> > -    xive->end_base = xive->vc_base + (1ull << xsrc->esb_shift) * 
> > xsrc->nr_irqs;
> > +    xive->end_base = xive->vc_base + xive_source_esb_len(xsrc);
> >       /*
> >        * Allocate the routing tables
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 82a6f99f022d..3263b982239a 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -831,7 +831,7 @@ void kvmppc_xive_disconnect(SpaprInterruptController 
> > *intc)
> >       /* Clear the KVM mapping */
> >       xsrc = &xive->source;
> > -    esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> > +    esb_len = xive_source_esb_len(xsrc);
> 
> hrm I'd like to not add another level of indirection here.
> In this specific case I think it's more clear to read just
> 
> 1ull << xsrc->esb_shift) * xsrc->nr_irqs
> 
> and get the idea of one IRQ per ESB page (or pair of pages,
> for trigger and management), than one having to look at
> what is inside "a box" called xive_source_esb_len().
> 
> Wrapping it under another function doesn't help more when
> reading the code, XIVE is already tricky enough :)

Given that we're already using it in some places, I'd prefer to use it
in all the places that it's correct to do so.

Applied to ppc-for-5.2.

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