| Message ID | 20180124181058.6157-9-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: VGIC/GIC separation cleanups | expand |
Hi Andre, On 24/01/18 18:10, Andre Przywara wrote: > On ARM the maximum number of IRQs is a constant, but we share it being > a variable to match x86. Since we are not supposed to alter it, let's > mark it as "const" to avoid accidental change. > > Suggested-by: Julien Grall <julien.grall@linaro.org> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> Acked-by: Julien Grall <julien.grall@arm.com> Cheers, > --- > xen/arch/arm/irq.c | 2 +- > xen/include/asm-arm/irq.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 62103a20e3..d229cb6871 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -27,7 +27,7 @@ > #include <asm/gic.h> > #include <asm/vgic.h> > > -unsigned int __read_mostly nr_irqs = NR_IRQS; > +const unsigned int __read_mostly nr_irqs = NR_IRQS; > > static unsigned int local_irqs_type[NR_LOCAL_IRQS]; > static DEFINE_SPINLOCK(local_irqs_type_lock); > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index 0d110ecb08..9d55e9b122 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -34,7 +34,7 @@ struct arch_irq_desc { > /* This is a spurious interrupt ID which never makes it into the GIC code. */ > #define INVALID_IRQ 1023 > > -extern unsigned int nr_irqs; > +extern const unsigned int nr_irqs; > #define nr_static_irqs NR_IRQS > #define arch_hwdom_irqs(domid) NR_IRQS > >
On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: > On ARM the maximum number of IRQs is a constant, but we share it being > a variable to match x86. Since we are not supposed to alter it, let's > mark it as "const" to avoid accidental change. > > Suggested-by: Julien Grall <julien.grall@linaro.org> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/irq.c | 2 +- > xen/include/asm-arm/irq.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 62103a20e3..d229cb6871 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -27,7 +27,7 @@ > #include <asm/gic.h> > #include <asm/vgic.h> > > -unsigned int __read_mostly nr_irqs = NR_IRQS; > +const unsigned int __read_mostly nr_irqs = NR_IRQS; Shouldn't you remove the __read_mostly attribute, so the symbol it's placed at the .rodata section by the compiler? Roger.
Hi, On 30/01/18 14:36, Roger Pau Monné wrote: > On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: >> On ARM the maximum number of IRQs is a constant, but we share it being >> a variable to match x86. Since we are not supposed to alter it, let's >> mark it as "const" to avoid accidental change. >> >> Suggested-by: Julien Grall <julien.grall@linaro.org> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/irq.c | 2 +- >> xen/include/asm-arm/irq.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 62103a20e3..d229cb6871 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -27,7 +27,7 @@ >> #include <asm/gic.h> >> #include <asm/vgic.h> >> >> -unsigned int __read_mostly nr_irqs = NR_IRQS; >> +const unsigned int __read_mostly nr_irqs = NR_IRQS; > > Shouldn't you remove the __read_mostly attribute, so the symbol it's > placed at the .rodata section by the compiler? Yes, makes sense, thanks for pointing this out! const ... __read_mostly sounds somewhat redundant. It looks like the compiler does the right thing anyway, as I can't find nr_irqs in the ELF in any case. Both with and without __read_mostly it results into the very same binary, actually even without the const. But I will include the change anyway. Cheers, Andre.
Hi Andre, On 01/02/18 13:43, Andre Przywara wrote: > On 30/01/18 14:36, Roger Pau Monné wrote: >> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: >>> On ARM the maximum number of IRQs is a constant, but we share it being >>> a variable to match x86. Since we are not supposed to alter it, let's >>> mark it as "const" to avoid accidental change. >>> >>> Suggested-by: Julien Grall <julien.grall@linaro.org> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>> --- >>> xen/arch/arm/irq.c | 2 +- >>> xen/include/asm-arm/irq.h | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >>> index 62103a20e3..d229cb6871 100644 >>> --- a/xen/arch/arm/irq.c >>> +++ b/xen/arch/arm/irq.c >>> @@ -27,7 +27,7 @@ >>> #include <asm/gic.h> >>> #include <asm/vgic.h> >>> >>> -unsigned int __read_mostly nr_irqs = NR_IRQS; >>> +const unsigned int __read_mostly nr_irqs = NR_IRQS; >> >> Shouldn't you remove the __read_mostly attribute, so the symbol it's >> placed at the .rodata section by the compiler? > > Yes, makes sense, thanks for pointing this out! > const ... __read_mostly sounds somewhat redundant. > > It looks like the compiler does the right thing anyway, as I can't find > nr_irqs in the ELF in any case. Both with and without __read_mostly it > results into the very same binary, actually even without the const. Really? I was expecting const data to be in the section.rodata. Are you suggesting this is landing in the section .data instead? Cheers,
On Thu, Feb 01, 2018 at 01:43:09PM +0000, Andre Przywara wrote: > Hi, > > On 30/01/18 14:36, Roger Pau Monné wrote: > > On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: > >> On ARM the maximum number of IRQs is a constant, but we share it being > >> a variable to match x86. Since we are not supposed to alter it, let's > >> mark it as "const" to avoid accidental change. > >> > >> Suggested-by: Julien Grall <julien.grall@linaro.org> > >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > >> --- > >> xen/arch/arm/irq.c | 2 +- > >> xen/include/asm-arm/irq.h | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > >> index 62103a20e3..d229cb6871 100644 > >> --- a/xen/arch/arm/irq.c > >> +++ b/xen/arch/arm/irq.c > >> @@ -27,7 +27,7 @@ > >> #include <asm/gic.h> > >> #include <asm/vgic.h> > >> > >> -unsigned int __read_mostly nr_irqs = NR_IRQS; > >> +const unsigned int __read_mostly nr_irqs = NR_IRQS; > > > > Shouldn't you remove the __read_mostly attribute, so the symbol it's > > placed at the .rodata section by the compiler? > > Yes, makes sense, thanks for pointing this out! > const ... __read_mostly sounds somewhat redundant. > > It looks like the compiler does the right thing anyway, as I can't find > nr_irqs in the ELF in any case. Both with and without __read_mostly it > results into the very same binary, actually even without the const. > But I will include the change anyway. Hm, that's kind of weird. nr_irqs seems to be used in ARM code. How did you assert that the symbol is not there? This is what I do on x86: # nm xen/xen-syms | grep ' nr_irqs$' ffff82d0804324f0 D nr_irqs Which matches what I would expect from the x86 build. Roger.
Hi, On 01/02/18 13:47, Julien Grall wrote: > Hi Andre, > > On 01/02/18 13:43, Andre Przywara wrote: >> On 30/01/18 14:36, Roger Pau Monné wrote: >>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: >>>> On ARM the maximum number of IRQs is a constant, but we share it being >>>> a variable to match x86. Since we are not supposed to alter it, let's >>>> mark it as "const" to avoid accidental change. >>>> >>>> Suggested-by: Julien Grall <julien.grall@linaro.org> >>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>> --- >>>> xen/arch/arm/irq.c | 2 +- >>>> xen/include/asm-arm/irq.h | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >>>> index 62103a20e3..d229cb6871 100644 >>>> --- a/xen/arch/arm/irq.c >>>> +++ b/xen/arch/arm/irq.c >>>> @@ -27,7 +27,7 @@ >>>> #include <asm/gic.h> >>>> #include <asm/vgic.h> >>>> -unsigned int __read_mostly nr_irqs = NR_IRQS; >>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS; >>> >>> Shouldn't you remove the __read_mostly attribute, so the symbol it's >>> placed at the .rodata section by the compiler? >> >> Yes, makes sense, thanks for pointing this out! >> const ... __read_mostly sounds somewhat redundant. >> >> It looks like the compiler does the right thing anyway, as I can't find >> nr_irqs in the ELF in any case. Both with and without __read_mostly it >> results into the very same binary, actually even without the const. > > Really? I was expecting const data to be in the section.rodata. Are you > suggesting this is landing in the section .data instead? Well, for the local case (arm/irq.c) the compiler just does the right thing and eliminates the variable completely : 00000000000005dc <request_irq>: 5dc: eb1f005f cmp x2, xzr 5e0: 52807fe5 mov w5, #0x3ff // #1023 5e4: 7a451002 ccmp w0, w5, #0x2, ne 5e8: 540003e8 b.hi 664 <request_irq+0x88> // EINVAL For common/domain.o it is as you guessed: in .data.read_mostly, with or without this (original) patch. So __read_mostly trumps const. Removing __read_mostly indeed puts it in .rodata. Don't know why the resulting xen/xen.axf don't show any difference, but the respective object files do. Cheers, Andre.
On 01/02/18 14:34, Andre Przywara wrote: > Hi, Hi, > On 01/02/18 13:47, Julien Grall wrote: >> Hi Andre, >> >> On 01/02/18 13:43, Andre Przywara wrote: >>> On 30/01/18 14:36, Roger Pau Monné wrote: >>>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: >>>>> On ARM the maximum number of IRQs is a constant, but we share it being >>>>> a variable to match x86. Since we are not supposed to alter it, let's >>>>> mark it as "const" to avoid accidental change. >>>>> >>>>> Suggested-by: Julien Grall <julien.grall@linaro.org> >>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>>> --- >>>>> xen/arch/arm/irq.c | 2 +- >>>>> xen/include/asm-arm/irq.h | 2 +- >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >>>>> index 62103a20e3..d229cb6871 100644 >>>>> --- a/xen/arch/arm/irq.c >>>>> +++ b/xen/arch/arm/irq.c >>>>> @@ -27,7 +27,7 @@ >>>>> #include <asm/gic.h> >>>>> #include <asm/vgic.h> >>>>> -unsigned int __read_mostly nr_irqs = NR_IRQS; >>>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS; >>>> >>>> Shouldn't you remove the __read_mostly attribute, so the symbol it's >>>> placed at the .rodata section by the compiler? >>> >>> Yes, makes sense, thanks for pointing this out! >>> const ... __read_mostly sounds somewhat redundant. >>> >>> It looks like the compiler does the right thing anyway, as I can't find >>> nr_irqs in the ELF in any case. Both with and without __read_mostly it >>> results into the very same binary, actually even without the const. >> >> Really? I was expecting const data to be in the section.rodata. Are you >> suggesting this is landing in the section .data instead? > > Well, for the local case (arm/irq.c) the compiler just does the right > thing and eliminates the variable completely : > > 00000000000005dc <request_irq>: > 5dc: eb1f005f cmp x2, xzr > 5e0: 52807fe5 mov w5, #0x3ff // #1023 > 5e4: 7a451002 ccmp w0, w5, #0x2, ne > 5e8: 540003e8 b.hi 664 <request_irq+0x88> // EINVAL > > For common/domain.o it is as you guessed: in .data.read_mostly, with or > without this (original) patch. So __read_mostly trumps const. > Removing __read_mostly indeed puts it in .rodata. > > Don't know why the resulting xen/xen.axf don't show any difference, but > the respective object files do. xen-syms is an ELF binary. xen is not an ELF. xen.axf is not built anymore since Xen 4.9. That might explain why you are not able to find it. Cheers,
Hi, On 01/02/18 13:57, Roger Pau Monné wrote: > On Thu, Feb 01, 2018 at 01:43:09PM +0000, Andre Przywara wrote: >> Hi, >> >> On 30/01/18 14:36, Roger Pau Monné wrote: >>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: >>>> On ARM the maximum number of IRQs is a constant, but we share it being >>>> a variable to match x86. Since we are not supposed to alter it, let's >>>> mark it as "const" to avoid accidental change. >>>> >>>> Suggested-by: Julien Grall <julien.grall@linaro.org> >>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>> --- >>>> xen/arch/arm/irq.c | 2 +- >>>> xen/include/asm-arm/irq.h | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >>>> index 62103a20e3..d229cb6871 100644 >>>> --- a/xen/arch/arm/irq.c >>>> +++ b/xen/arch/arm/irq.c >>>> @@ -27,7 +27,7 @@ >>>> #include <asm/gic.h> >>>> #include <asm/vgic.h> >>>> >>>> -unsigned int __read_mostly nr_irqs = NR_IRQS; >>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS; >>> >>> Shouldn't you remove the __read_mostly attribute, so the symbol it's >>> placed at the .rodata section by the compiler? >> >> Yes, makes sense, thanks for pointing this out! >> const ... __read_mostly sounds somewhat redundant. >> >> It looks like the compiler does the right thing anyway, as I can't find >> nr_irqs in the ELF in any case. Both with and without __read_mostly it >> results into the very same binary, actually even without the const. >> But I will include the change anyway. > > Hm, that's kind of weird. nr_irqs seems to be used in ARM code. How > did you assert that the symbol is not there? Well, I was looking at xen/xen.axf, which is obviously an artefact from some experiments last summer, as it didn't get rebuild ;-) So ignore me, the differences are there in xen-syms. And __read_mostly overrides const, so it *is* helpful to remove it. Thanks! Andre. > > This is what I do on x86: > > # nm xen/xen-syms | grep ' nr_irqs$' > ffff82d0804324f0 D nr_irqs > > Which matches what I would expect from the x86 build. > > Roger. >
Hi, On 01/02/18 14:39, Julien Grall wrote: > > > On 01/02/18 14:34, Andre Przywara wrote: >> Hi, > > Hi, > >> On 01/02/18 13:47, Julien Grall wrote: >>> Hi Andre, >>> >>> On 01/02/18 13:43, Andre Przywara wrote: >>>> On 30/01/18 14:36, Roger Pau Monné wrote: >>>>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote: >>>>>> On ARM the maximum number of IRQs is a constant, but we share it >>>>>> being >>>>>> a variable to match x86. Since we are not supposed to alter it, let's >>>>>> mark it as "const" to avoid accidental change. >>>>>> >>>>>> Suggested-by: Julien Grall <julien.grall@linaro.org> >>>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>>>> --- >>>>>> xen/arch/arm/irq.c | 2 +- >>>>>> xen/include/asm-arm/irq.h | 2 +- >>>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >>>>>> index 62103a20e3..d229cb6871 100644 >>>>>> --- a/xen/arch/arm/irq.c >>>>>> +++ b/xen/arch/arm/irq.c >>>>>> @@ -27,7 +27,7 @@ >>>>>> #include <asm/gic.h> >>>>>> #include <asm/vgic.h> >>>>>> -unsigned int __read_mostly nr_irqs = NR_IRQS; >>>>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS; >>>>> >>>>> Shouldn't you remove the __read_mostly attribute, so the symbol it's >>>>> placed at the .rodata section by the compiler? >>>> >>>> Yes, makes sense, thanks for pointing this out! >>>> const ... __read_mostly sounds somewhat redundant. >>>> >>>> It looks like the compiler does the right thing anyway, as I can't find >>>> nr_irqs in the ELF in any case. Both with and without __read_mostly it >>>> results into the very same binary, actually even without the const. >>> >>> Really? I was expecting const data to be in the section.rodata. Are you >>> suggesting this is landing in the section .data instead? >> >> Well, for the local case (arm/irq.c) the compiler just does the right >> thing and eliminates the variable completely : >> >> 00000000000005dc <request_irq>: >> 5dc: eb1f005f cmp x2, xzr >> 5e0: 52807fe5 mov w5, #0x3ff // #1023 >> 5e4: 7a451002 ccmp w0, w5, #0x2, ne >> 5e8: 540003e8 b.hi 664 <request_irq+0x88> // EINVAL >> >> For common/domain.o it is as you guessed: in .data.read_mostly, with or >> without this (original) patch. So __read_mostly trumps const. >> Removing __read_mostly indeed puts it in .rodata. >> >> Don't know why the resulting xen/xen.axf don't show any difference, but >> the respective object files do. > xen-syms is an ELF binary. > xen is not an ELF. > xen.axf is not built anymore since Xen 4.9. Indeed, I missed that part. The date stayed always at "Jun 7 2017" ;-) So on real surprise that it didn't change. Cheers, Andre. > That might explain why you are not able to find it.
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 62103a20e3..d229cb6871 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -27,7 +27,7 @@ #include <asm/gic.h> #include <asm/vgic.h> -unsigned int __read_mostly nr_irqs = NR_IRQS; +const unsigned int __read_mostly nr_irqs = NR_IRQS; static unsigned int local_irqs_type[NR_LOCAL_IRQS]; static DEFINE_SPINLOCK(local_irqs_type_lock); diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index 0d110ecb08..9d55e9b122 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -34,7 +34,7 @@ struct arch_irq_desc { /* This is a spurious interrupt ID which never makes it into the GIC code. */ #define INVALID_IRQ 1023 -extern unsigned int nr_irqs; +extern const unsigned int nr_irqs; #define nr_static_irqs NR_IRQS #define arch_hwdom_irqs(domid) NR_IRQS
On ARM the maximum number of IRQs is a constant, but we share it being a variable to match x86. Since we are not supposed to alter it, let's mark it as "const" to avoid accidental change. Suggested-by: Julien Grall <julien.grall@linaro.org> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/irq.c | 2 +- xen/include/asm-arm/irq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)