On Thu, 13 Aug 2020 17:38:59 -0300 Gustavo Romero <grom...@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 <gr...@kaod.org> > > --- > > Follow-up on "ppc/xive: Rework setup of XiveSource::esb_mmio" > > http://patchwork.ozlabs.org/project/qemu-devel/patch/159679992680.876294.7520540158586170894.st...@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