| Message ID | 20180305160415.16760-21-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 05/03/18 16:03, Andre Przywara wrote: > The bit definition for the CPUID mask in the GICv2 LR register was > wrong, fortunately the current implementation does not use that bit. > Fix it up (it's starting at bit 10, not bit 9) and clean up some > nearby definitions on the way. > This will be used by the new VGIC shortly. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> Reviewed-by: Julien Grall <julien.grall@arm.com> > --- > Changelog RFC ... v1: > - new patch > xen/arch/arm/gic-v2.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 618dd94120..031be920cc 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -57,10 +57,11 @@ > #define GICH_V2_LR_HW_MASK 0x1 > #define GICH_V2_LR_GRP_SHIFT 30 > #define GICH_V2_LR_GRP_MASK 0x1 > -#define GICH_V2_LR_MAINTENANCE_IRQ (1<<19) > -#define GICH_V2_LR_GRP1 (1<<30) > -#define GICH_V2_LR_HW (1<<31) > -#define GICH_V2_LR_CPUID_SHIFT 9 > +#define GICH_V2_LR_MAINTENANCE_IRQ (1U << 19) > +#define GICH_V2_LR_GRP1 (1U << 30) > +#define GICH_V2_LR_HW (1U << GICH_V2_LR_HW_SHIFT) I think we want this patch to get backported as 1 << 31 is an undefined behavior. > +#define GICH_V2_LR_CPUID_SHIFT 10 > +#define GICH_V2_LR_CPUID_MASK 0x7 > #define GICH_V2_VTR_NRLRGS 0x3f > > #define GICH_V2_VMCR_PRIORITY_MASK 0x1f > Cheers,
Hi, On 06/03/18 15:46, Julien Grall wrote: > Hi Andre, > > On 05/03/18 16:03, Andre Przywara wrote: >> The bit definition for the CPUID mask in the GICv2 LR register was >> wrong, fortunately the current implementation does not use that bit. >> Fix it up (it's starting at bit 10, not bit 9) and clean up some >> nearby definitions on the way. >> This will be used by the new VGIC shortly. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > > Reviewed-by: Julien Grall <julien.grall@arm.com> Thanks! > >> --- >> Changelog RFC ... v1: >> - new patch >> xen/arch/arm/gic-v2.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index 618dd94120..031be920cc 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -57,10 +57,11 @@ >> #define GICH_V2_LR_HW_MASK 0x1 >> #define GICH_V2_LR_GRP_SHIFT 30 >> #define GICH_V2_LR_GRP_MASK 0x1 >> -#define GICH_V2_LR_MAINTENANCE_IRQ (1<<19) >> -#define GICH_V2_LR_GRP1 (1<<30) >> -#define GICH_V2_LR_HW (1<<31) >> -#define GICH_V2_LR_CPUID_SHIFT 9 >> +#define GICH_V2_LR_MAINTENANCE_IRQ (1U << 19) >> +#define GICH_V2_LR_GRP1 (1U << 30) >> +#define GICH_V2_LR_HW (1U << GICH_V2_LR_HW_SHIFT) > > I think we want this patch to get backported as 1 << 31 is an undefined > behavior. I don't think this is necessary. While I agree that 1<<31 is bad (thus the fix), there is only one user of that macro (down below in that very file), and the result type is unsigned. If I understand this issue correctly, the undefined behaviour is about a signed *result* type. Cheers, Andre. >> +#define GICH_V2_LR_CPUID_SHIFT 10 >> +#define GICH_V2_LR_CPUID_MASK 0x7 >> #define GICH_V2_VTR_NRLRGS 0x3f >> #define GICH_V2_VMCR_PRIORITY_MASK 0x1f >> > > Cheers, >
Hi, On 06/03/18 15:58, Andre Przywara wrote: > Hi, > > On 06/03/18 15:46, Julien Grall wrote: >> Hi Andre, >> >> On 05/03/18 16:03, Andre Przywara wrote: >>> The bit definition for the CPUID mask in the GICv2 LR register was >>> wrong, fortunately the current implementation does not use that bit. >>> Fix it up (it's starting at bit 10, not bit 9) and clean up some >>> nearby definitions on the way. >>> This will be used by the new VGIC shortly. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> >> Reviewed-by: Julien Grall <julien.grall@arm.com> > > Thanks! > >> >>> --- >>> Changelog RFC ... v1: >>> - new patch >>> xen/arch/arm/gic-v2.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >>> index 618dd94120..031be920cc 100644 >>> --- a/xen/arch/arm/gic-v2.c >>> +++ b/xen/arch/arm/gic-v2.c >>> @@ -57,10 +57,11 @@ >>> #define GICH_V2_LR_HW_MASK 0x1 >>> #define GICH_V2_LR_GRP_SHIFT 30 >>> #define GICH_V2_LR_GRP_MASK 0x1 >>> -#define GICH_V2_LR_MAINTENANCE_IRQ (1<<19) >>> -#define GICH_V2_LR_GRP1 (1<<30) >>> -#define GICH_V2_LR_HW (1<<31) >>> -#define GICH_V2_LR_CPUID_SHIFT 9 >>> +#define GICH_V2_LR_MAINTENANCE_IRQ (1U << 19) >>> +#define GICH_V2_LR_GRP1 (1U << 30) >>> +#define GICH_V2_LR_HW (1U << GICH_V2_LR_HW_SHIFT) >> >> I think we want this patch to get backported as 1 << 31 is an undefined >> behavior. > > I don't think this is necessary. While I agree that 1<<31 is bad (thus > the fix), there is only one user of that macro (down below in that very > file), and the result type is unsigned. If I understand this issue > correctly, the undefined behaviour is about a signed *result* type. Here a small example showing the problem: #include <stdio.h> #include <stdint.h> int foo(uint32_t foo) { foo &= 1 << 31; } int main(void) { foo(-1); return 0; } 42sh> ./a.out test.c:6:14: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' I also remember ubsan complaining on some of the GICv2 shift. I still need to send those patches :/. Cheers,
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 618dd94120..031be920cc 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -57,10 +57,11 @@ #define GICH_V2_LR_HW_MASK 0x1 #define GICH_V2_LR_GRP_SHIFT 30 #define GICH_V2_LR_GRP_MASK 0x1 -#define GICH_V2_LR_MAINTENANCE_IRQ (1<<19) -#define GICH_V2_LR_GRP1 (1<<30) -#define GICH_V2_LR_HW (1<<31) -#define GICH_V2_LR_CPUID_SHIFT 9 +#define GICH_V2_LR_MAINTENANCE_IRQ (1U << 19) +#define GICH_V2_LR_GRP1 (1U << 30) +#define GICH_V2_LR_HW (1U << GICH_V2_LR_HW_SHIFT) +#define GICH_V2_LR_CPUID_SHIFT 10 +#define GICH_V2_LR_CPUID_MASK 0x7 #define GICH_V2_VTR_NRLRGS 0x3f #define GICH_V2_VMCR_PRIORITY_MASK 0x1f
The bit definition for the CPUID mask in the GICv2 LR register was wrong, fortunately the current implementation does not use that bit. Fix it up (it's starting at bit 10, not bit 9) and clean up some nearby definitions on the way. This will be used by the new VGIC shortly. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - new patch xen/arch/arm/gic-v2.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)