* [PATCH] spapr/xive: Use xive_source_esb_len()
@ 2020-08-13 17:28 Greg Kurz
2020-08-13 20:38 ` Gustavo Romero
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2020-08-13 17:28 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
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);
if (xsrc->esb_mmap) {
memory_region_del_subregion(&xsrc->esb_mmio, &xsrc->esb_mmio_kvm);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr/xive: Use xive_source_esb_len()
2020-08-13 17:28 [PATCH] spapr/xive: Use xive_source_esb_len() Greg Kurz
@ 2020-08-13 20:38 ` Gustavo Romero
2020-08-13 22:00 ` Greg Kurz
2020-08-14 3:36 ` David Gibson
0 siblings, 2 replies; 6+ messages in thread
From: Gustavo Romero @ 2020-08-13 20:38 UTC (permalink / raw)
To: Greg Kurz, David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel
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"
> 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...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr/xive: Use xive_source_esb_len()
2020-08-13 20:38 ` Gustavo Romero
@ 2020-08-13 22:00 ` Greg Kurz
2020-08-13 23:01 ` Gustavo Romero
2020-08-14 3:36 ` David Gibson
1 sibling, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2020-08-13 22:00 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, qemu-ppc, Cédric Le Goater, David Gibson
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 ;-)
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr/xive: Use xive_source_esb_len()
2020-08-13 22:00 ` Greg Kurz
@ 2020-08-13 23:01 ` Gustavo Romero
2020-08-14 17:00 ` Cédric Le Goater
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo Romero @ 2020-08-13 23:01 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr/xive: Use xive_source_esb_len()
2020-08-13 20:38 ` Gustavo Romero
2020-08-13 22:00 ` Greg Kurz
@ 2020-08-14 3:36 ` David Gibson
1 sibling, 0 replies; 6+ messages in thread
From: David Gibson @ 2020-08-14 3:36 UTC (permalink / raw)
To: Gustavo Romero; +Cc: qemu-devel, qemu-ppc, Greg Kurz, Cédric Le Goater
[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]
On Thu, Aug 13, 2020 at 05:38:59PM -0300, Gustavo Romero wrote:
> 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"
> > 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 :)
Given that we're already using it in some places, I'd prefer to use it
in all the places that it's correct to do so.
Applied to ppc-for-5.2.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spapr/xive: Use xive_source_esb_len()
2020-08-13 23:01 ` Gustavo Romero
@ 2020-08-14 17:00 ` Cédric Le Goater
0 siblings, 0 replies; 6+ messages in thread
From: Cédric Le Goater @ 2020-08-14 17:00 UTC (permalink / raw)
To: Gustavo Romero, Greg Kurz; +Cc: qemu-ppc, qemu-devel
> I found out recently that XIVE support was
> merged into FreeBSD and with that it also came some good comments
> about xive...
cool ! Does it run in a QEMU PowerNV machine ?
C.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-14 17:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-13 17:28 [PATCH] spapr/xive: Use xive_source_esb_len() Greg Kurz
2020-08-13 20:38 ` Gustavo Romero
2020-08-13 22:00 ` Greg Kurz
2020-08-13 23:01 ` Gustavo Romero
2020-08-14 17:00 ` Cédric Le Goater
2020-08-14 3:36 ` David Gibson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).