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

[Gustavo Romero]

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


Cheers,
Gustavo

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