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

[Gustavo Romero]

On 8/13/20 7:00 PM, Greg Kurz wrote:
> 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 ;-)

Yea, maybe someday we get a cool documentation on it.

Luckily I came after Benh and Cédric and took some good info from
OPAL and QEMU code. I found out recently that XIVE support was
merged into FreeBSD and with that it also came some good comments
about xive...


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

Thanks, Greg.

Hope there is still some time to enjoy the summer over there :)


Cheers,
Gustavo