| Message ID | 20240823230100.1581448-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ARM/vgic: Use for_each_set_bit() in vgic_to_sgi() | expand |
On 24/08/2024 01:01, Andrew Cooper wrote: > > > The existing expression is just a very complicated way of expressing a loop > over all bits of target->list. Simplify the expression. > > While here, fix the two gprintk()'s. Because of a quotes vs line continuation > issue, there's a long string of spaces in the middle of the format string. > > $ strings xen-syms-arm32 | grep -e VGIC -e GICD_SGIR > <G><1>%pv VGIC: write r=%08x target->list=%hx, wrong CPUTargetList > <G><1>%pv vGICD:unhandled GICD_SGIR write %08x with wrong mode > > not to mention trailing whitespace too. > > Rewrite them to be more consise and more useful. Use 0x prefixes for hex, s/consise/concise > rather than ambigous, and identify the problem target vCPU / mode, rather than s/ambigous/ambiguous > simply saying somethign was wrong. s/somethign/something/ > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Michal Orzel <michal.orzel@amd.com> > CC: Jan Beulich <JBeulich@suse.com> > > In a fun twist, we can't use target->list directly in the expresion, because > the typeof() picks up constness from the pointer, and we get: > > In file included from arch/arm/vgic.c:11: > arch/arm/vgic.c: In function ‘vgic_to_sgi’: > ./include/xen/bitops.h:305:19: error: assignment of read-only variable ‘__v’ > 305 | __v &= __v - 1 ) > | ^~ > arch/arm/vgic.c:483:9: note: in expansion of macro ‘for_each_set_bit’ > 483 | for_each_set_bit ( i, target->list ) > | ^~~~~~~~~~~~~~~~ > > Sadly we need -std=c23 before we can use typeof_unqual() which is what we > actually want here. > --- > xen/arch/arm/vgic.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 7b54ccc7cbfa..081cbb67fb52 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -470,8 +470,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, > struct domain *d = v->domain; > int vcpuid; > int i; > - unsigned int base; > - unsigned long int bitmap; > + unsigned int base, bitmap; > > ASSERT( virq < 16 ); > > @@ -481,15 +480,16 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, > perfc_incr(vgic_sgi_list); > base = target->aff1 << 4; > bitmap = target->list; > - bitmap_for_each ( i, &bitmap, sizeof(target->list) * 8 ) > + > + for_each_set_bit ( i, bitmap ) > { > vcpuid = base + i; > if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL || > !is_vcpu_online(d->vcpu[vcpuid]) ) > { > - gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \ > - target->list=%hx, wrong CPUTargetList \n", > - sgir, target->list); > + gprintk(XENLOG_WARNING, > + "vGIC: write %#"PRIregister", target->list=%#x, bad target v%d\n", Sth like "bad target v2" where the word vcpu does not occur anywhere in the msg can be ambiguous. Can you add the word vcpu e.g. "bad vcpu target v%d" or "bad target vcpu %d" > + sgir, target->list, vcpuid); > continue; > } > vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); > @@ -510,8 +510,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, > break; > default: > gprintk(XENLOG_WARNING, > - "vGICD:unhandled GICD_SGIR write %"PRIregister" \ > - with wrong mode\n", sgir); > + "vGICD: GICD_SGIR write %#"PRIregister" with unhangled mode %d\n", s/unhangled/unhandled/ > + sgir, irqmode); > return false; > } > > -- > 2.39.2 > Otherwise: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
On 27/08/2024 1:13 pm, Michal Orzel wrote: > > On 24/08/2024 01:01, Andrew Cooper wrote: >> >> The existing expression is just a very complicated way of expressing a loop >> over all bits of target->list. Simplify the expression. >> >> While here, fix the two gprintk()'s. Because of a quotes vs line continuation >> issue, there's a long string of spaces in the middle of the format string. >> >> $ strings xen-syms-arm32 | grep -e VGIC -e GICD_SGIR >> <G><1>%pv VGIC: write r=%08x target->list=%hx, wrong CPUTargetList >> <G><1>%pv vGICD:unhandled GICD_SGIR write %08x with wrong mode >> >> not to mention trailing whitespace too. >> >> Rewrite them to be more consise and more useful. Use 0x prefixes for hex, > s/consise/concise > >> rather than ambigous, and identify the problem target vCPU / mode, rather than > s/ambigous/ambiguous > >> simply saying somethign was wrong. > s/somethign/something/ > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien@xen.org> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> CC: Bertrand Marquis <bertrand.marquis@arm.com> >> CC: Michal Orzel <michal.orzel@amd.com> >> CC: Jan Beulich <JBeulich@suse.com> >> >> In a fun twist, we can't use target->list directly in the expresion, because >> the typeof() picks up constness from the pointer, and we get: >> >> In file included from arch/arm/vgic.c:11: >> arch/arm/vgic.c: In function ‘vgic_to_sgi’: >> ./include/xen/bitops.h:305:19: error: assignment of read-only variable ‘__v’ >> 305 | __v &= __v - 1 ) >> | ^~ >> arch/arm/vgic.c:483:9: note: in expansion of macro ‘for_each_set_bit’ >> 483 | for_each_set_bit ( i, target->list ) >> | ^~~~~~~~~~~~~~~~ >> >> Sadly we need -std=c23 before we can use typeof_unqual() which is what we >> actually want here. >> --- >> xen/arch/arm/vgic.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 7b54ccc7cbfa..081cbb67fb52 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -470,8 +470,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, >> struct domain *d = v->domain; >> int vcpuid; >> int i; >> - unsigned int base; >> - unsigned long int bitmap; >> + unsigned int base, bitmap; >> >> ASSERT( virq < 16 ); >> >> @@ -481,15 +480,16 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, >> perfc_incr(vgic_sgi_list); >> base = target->aff1 << 4; >> bitmap = target->list; >> - bitmap_for_each ( i, &bitmap, sizeof(target->list) * 8 ) >> + >> + for_each_set_bit ( i, bitmap ) >> { >> vcpuid = base + i; >> if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL || >> !is_vcpu_online(d->vcpu[vcpuid]) ) >> { >> - gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \ >> - target->list=%hx, wrong CPUTargetList \n", >> - sgir, target->list); >> + gprintk(XENLOG_WARNING, >> + "vGIC: write %#"PRIregister", target->list=%#x, bad target v%d\n", > Sth like "bad target v2" where the word vcpu does not occur anywhere in the msg can be ambiguous. > Can you add the word vcpu e.g. "bad vcpu target v%d" or "bad target vcpu %d" Hmm yeah, v%d doesn't work quite so well when it's not prefixed with d%d. Would you be happy with d%dv%d? It's marginally more informative and shorter. > >> + sgir, target->list, vcpuid); >> continue; >> } >> vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); >> @@ -510,8 +510,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, >> break; >> default: >> gprintk(XENLOG_WARNING, >> - "vGICD:unhandled GICD_SGIR write %"PRIregister" \ >> - with wrong mode\n", sgir); >> + "vGICD: GICD_SGIR write %#"PRIregister" with unhangled mode %d\n", > s/unhangled/unhandled/ > >> + sgir, irqmode); >> return false; >> } >> >> -- >> 2.39.2 >> > Otherwise: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Bah, I really was asleep when writing this. I spotted 2 of the typos, but not all of them. I'll fix them all up. Thanks. ~Andrew
On 27/08/2024 14:20, Andrew Cooper wrote: > > > On 27/08/2024 1:13 pm, Michal Orzel wrote: >> >> On 24/08/2024 01:01, Andrew Cooper wrote: >>> >>> The existing expression is just a very complicated way of expressing a loop >>> over all bits of target->list. Simplify the expression. >>> >>> While here, fix the two gprintk()'s. Because of a quotes vs line continuation >>> issue, there's a long string of spaces in the middle of the format string. >>> >>> $ strings xen-syms-arm32 | grep -e VGIC -e GICD_SGIR >>> <G><1>%pv VGIC: write r=%08x target->list=%hx, wrong CPUTargetList >>> <G><1>%pv vGICD:unhandled GICD_SGIR write %08x with wrong mode >>> >>> not to mention trailing whitespace too. >>> >>> Rewrite them to be more consise and more useful. Use 0x prefixes for hex, >> s/consise/concise >> >>> rather than ambigous, and identify the problem target vCPU / mode, rather than >> s/ambigous/ambiguous >> >>> simply saying somethign was wrong. >> s/somethign/something/ >> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Stefano Stabellini <sstabellini@kernel.org> >>> CC: Julien Grall <julien@xen.org> >>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>> CC: Michal Orzel <michal.orzel@amd.com> >>> CC: Jan Beulich <JBeulich@suse.com> >>> >>> In a fun twist, we can't use target->list directly in the expresion, because >>> the typeof() picks up constness from the pointer, and we get: >>> >>> In file included from arch/arm/vgic.c:11: >>> arch/arm/vgic.c: In function ‘vgic_to_sgi’: >>> ./include/xen/bitops.h:305:19: error: assignment of read-only variable ‘__v’ >>> 305 | __v &= __v - 1 ) >>> | ^~ >>> arch/arm/vgic.c:483:9: note: in expansion of macro ‘for_each_set_bit’ >>> 483 | for_each_set_bit ( i, target->list ) >>> | ^~~~~~~~~~~~~~~~ >>> >>> Sadly we need -std=c23 before we can use typeof_unqual() which is what we >>> actually want here. >>> --- >>> xen/arch/arm/vgic.c | 16 ++++++++-------- >>> 1 file changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index 7b54ccc7cbfa..081cbb67fb52 100644 >>> --- a/xen/arch/arm/vgic.c >>> +++ b/xen/arch/arm/vgic.c >>> @@ -470,8 +470,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, >>> struct domain *d = v->domain; >>> int vcpuid; >>> int i; >>> - unsigned int base; >>> - unsigned long int bitmap; >>> + unsigned int base, bitmap; >>> >>> ASSERT( virq < 16 ); >>> >>> @@ -481,15 +480,16 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, >>> perfc_incr(vgic_sgi_list); >>> base = target->aff1 << 4; >>> bitmap = target->list; >>> - bitmap_for_each ( i, &bitmap, sizeof(target->list) * 8 ) >>> + >>> + for_each_set_bit ( i, bitmap ) >>> { >>> vcpuid = base + i; >>> if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL || >>> !is_vcpu_online(d->vcpu[vcpuid]) ) >>> { >>> - gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \ >>> - target->list=%hx, wrong CPUTargetList \n", >>> - sgir, target->list); >>> + gprintk(XENLOG_WARNING, >>> + "vGIC: write %#"PRIregister", target->list=%#x, bad target v%d\n", >> Sth like "bad target v2" where the word vcpu does not occur anywhere in the msg can be ambiguous. >> Can you add the word vcpu e.g. "bad vcpu target v%d" or "bad target vcpu %d" > > Hmm yeah, v%d doesn't work quite so well when it's not prefixed with d%d. > > Would you be happy with d%dv%d? It's marginally more informative and > shorter. I don't think we can target a different domain with SGIs, so it does not make much sense to print domain id if it always stays the same as the leading %pv from gprintk. ~Michal
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 7b54ccc7cbfa..081cbb67fb52 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -470,8 +470,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, struct domain *d = v->domain; int vcpuid; int i; - unsigned int base; - unsigned long int bitmap; + unsigned int base, bitmap; ASSERT( virq < 16 ); @@ -481,15 +480,16 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, perfc_incr(vgic_sgi_list); base = target->aff1 << 4; bitmap = target->list; - bitmap_for_each ( i, &bitmap, sizeof(target->list) * 8 ) + + for_each_set_bit ( i, bitmap ) { vcpuid = base + i; if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL || !is_vcpu_online(d->vcpu[vcpuid]) ) { - gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \ - target->list=%hx, wrong CPUTargetList \n", - sgir, target->list); + gprintk(XENLOG_WARNING, + "vGIC: write %#"PRIregister", target->list=%#x, bad target v%d\n", + sgir, target->list, vcpuid); continue; } vgic_inject_irq(d, d->vcpu[vcpuid], virq, true); @@ -510,8 +510,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, break; default: gprintk(XENLOG_WARNING, - "vGICD:unhandled GICD_SGIR write %"PRIregister" \ - with wrong mode\n", sgir); + "vGICD: GICD_SGIR write %#"PRIregister" with unhangled mode %d\n", + sgir, irqmode); return false; }
The existing expression is just a very complicated way of expressing a loop over all bits of target->list. Simplify the expression. While here, fix the two gprintk()'s. Because of a quotes vs line continuation issue, there's a long string of spaces in the middle of the format string. $ strings xen-syms-arm32 | grep -e VGIC -e GICD_SGIR <G><1>%pv VGIC: write r=%08x target->list=%hx, wrong CPUTargetList <G><1>%pv vGICD:unhandled GICD_SGIR write %08x with wrong mode not to mention trailing whitespace too. Rewrite them to be more consise and more useful. Use 0x prefixes for hex, rather than ambigous, and identify the problem target vCPU / mode, rather than simply saying somethign was wrong. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Jan Beulich <JBeulich@suse.com> In a fun twist, we can't use target->list directly in the expresion, because the typeof() picks up constness from the pointer, and we get: In file included from arch/arm/vgic.c:11: arch/arm/vgic.c: In function ‘vgic_to_sgi’: ./include/xen/bitops.h:305:19: error: assignment of read-only variable ‘__v’ 305 | __v &= __v - 1 ) | ^~ arch/arm/vgic.c:483:9: note: in expansion of macro ‘for_each_set_bit’ 483 | for_each_set_bit ( i, target->list ) | ^~~~~~~~~~~~~~~~ Sadly we need -std=c23 before we can use typeof_unqual() which is what we actually want here. --- xen/arch/arm/vgic.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)