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

[Greg Kurz]

On Thu, 13 Aug 2020 17:38:59 -0300
Gustavo Romero <gromero@linux.vnet.ibm.com> wrote:

> Hi Greg,
> 

Hi Gustavo,

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

Heh, XIVE is tricky enough that only a few people will dare
to touch this code and they'd better already know about the
one IRQ per ESB page thingy ;-)

More seriously, this is a matter of taste, but since you're likely
to be involved in XIVE a bit more than me, I'm perfectly fine with
keeping this open-coded.

> 
> Cheers,
> Gustavo
> 
> PS: It seems something messed up with the commit message. It
> can be that the ML did that tho...

It's more laziness on my side... I should have come up with a
proper changelog like "We already have an helper that provides
the length of the ESB mapping. No need to open-code this again."
instead of pasting a code snippet. Time to go on vacation I guess :)

Cheers,

--
Greg