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

[Gustavo Romero <gromero@linux.vnet.ibm.com>]

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