Re: [Xen-devel] [PATCH 20/57] ARM: GICv2: fix GICH_V2_LR definitions

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
> > Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel