Re: [edk2] [PATCH] ArmPkg ArmVirtPkg: fix the GIC base address variables as 64-bit
On Mon, Oct 17, 2016 at 10:20:30AM +0100, Ard Biesheuvel wrote: > On 17 October 2016 at 09:33, Leif Lindholmwrote: > > On Mon, Oct 17, 2016 at 08:28:50AM +0100, Ard Biesheuvel wrote: > >> > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > >> > b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > >> > index 64afc4d..16683ef 100644 > >> > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > >> > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > >> > @@ -79,11 +79,11 @@ ArmVirtGicArchLibConstructor ( > >> > > >> > // RegProp[0..1] == { GICD base, GICD size } > >> > DistBase = SwapBytes64 (Reg[0]); > >> > -ASSERT (DistBase < MAX_UINT32); > >> > +ASSERT (DistBase < MAX_UINT64); > >> > > >> > >> This becomes equivalent to 'DistBase != MAX_UINT64' given that a > >> UINT64 cannot exceed MAX_UINT64. That is a nonsensical thing to > >> assert, so it is better to simply drop it > > > > Random thought: > > Could we keep the assert(s) and change the test to MAX_UINTN, to have > > a sanity test over whether a 32-bit plaform ends up with a duff > > address? > > That seems like a useful thing in general, but given that we don't do > that anywhere else, I'd rather we just remove them. I won't argue with that. / Leif ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg ArmVirtPkg: fix the GIC base address variables as 64-bit
On 17 October 2016 at 09:33, Leif Lindholmwrote: > On Mon, Oct 17, 2016 at 08:28:50AM +0100, Ard Biesheuvel wrote: >> > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> > b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> > index 64afc4d..16683ef 100644 >> > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> > @@ -79,11 +79,11 @@ ArmVirtGicArchLibConstructor ( >> > >> > // RegProp[0..1] == { GICD base, GICD size } >> > DistBase = SwapBytes64 (Reg[0]); >> > -ASSERT (DistBase < MAX_UINT32); >> > +ASSERT (DistBase < MAX_UINT64); >> > >> >> This becomes equivalent to 'DistBase != MAX_UINT64' given that a >> UINT64 cannot exceed MAX_UINT64. That is a nonsensical thing to >> assert, so it is better to simply drop it > > Random thought: > Could we keep the assert(s) and change the test to MAX_UINTN, to have > a sanity test over whether a 32-bit plaform ends up with a duff > address? > That seems like a useful thing in general, but given that we don't do that anywhere else, I'd rather we just remove them. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg ArmVirtPkg: fix the GIC base address variables as 64-bit
On Mon, Oct 17, 2016 at 08:28:50AM +0100, Ard Biesheuvel wrote: > > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > index 64afc4d..16683ef 100644 > > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > @@ -79,11 +79,11 @@ ArmVirtGicArchLibConstructor ( > > > > // RegProp[0..1] == { GICD base, GICD size } > > DistBase = SwapBytes64 (Reg[0]); > > -ASSERT (DistBase < MAX_UINT32); > > +ASSERT (DistBase < MAX_UINT64); > > > > This becomes equivalent to 'DistBase != MAX_UINT64' given that a > UINT64 cannot exceed MAX_UINT64. That is a nonsensical thing to > assert, so it is better to simply drop it Random thought: Could we keep the assert(s) and change the test to MAX_UINTN, to have a sanity test over whether a 32-bit plaform ends up with a duff address? / Leif > > // RegProp[2..3] == { GICR base, GICR size } > > RedistBase = SwapBytes64 (Reg[2]); > > -ASSERT (RedistBase < MAX_UINT32); > > +ASSERT (RedistBase < MAX_UINT64); > > > > Likewise > > > PcdSet64 (PcdGicDistributorBase, DistBase); > > PcdSet64 (PcdGicRedistributorsBase, RedistBase); > > @@ -117,8 +117,8 @@ ArmVirtGicArchLibConstructor ( > > > > DistBase = SwapBytes64 (Reg[0]); > > CpuBase = SwapBytes64 (Reg[2]); > > -ASSERT (DistBase < MAX_UINT32); > > -ASSERT (CpuBase < MAX_UINT32); > > +ASSERT (DistBase < MAX_UINT64); > > +ASSERT (CpuBase < MAX_UINT64); > > > > Likewise > > > PcdSet64 (PcdGicDistributorBase, DistBase); > > PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase); > > -- > > 2.7.4 > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg ArmVirtPkg: fix the GIC base address variables as 64-bit
Hello Ard, Thanks for the comments! I will split this patch into 2 and for ArmVirtPkg patch, we just need to simply drop the original ASSERT() since it's nonsensical any more. Thanks, Dennis On Mon, Oct 17, 2016 at 08:28:50AM +0100, Ard Biesheuvel wrote: > Hi Dennis, > > On 17 October 2016 at 06:03, Dennis Chenwrote: > > Since ACPI spec defines the GIC base addresses (CPU interface, > > Distributor and Redistributor*GICv3 only*) as 64-bit, so we > > should define these corresponding base address variables as 64-bit > > instead of 32-bit. This patch redefines them according to the > > ACPI spec. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Cc: Ard Biesheuvel > > Cc: Leif Lindholm > > Signed-off-by: Dennis Chen > > --- > > ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c| 4 ++-- > > ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 8 > > Could you split this patch in 2 please, and put Laszlo Ersek on cc for > the ArmVirtPkg patch? > > > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > > b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > > index b9ecd55..a4ba5cf 100644 > > --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > > +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > > @@ -30,8 +30,8 @@ Abstract: > > > > extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol; > > > > -STATIC UINT32 mGicInterruptInterfaceBase; > > -STATIC UINT32 mGicDistributorBase; > > +STATIC UINTN mGicInterruptInterfaceBase; > > +STATIC UINTN mGicDistributorBase; > > > > This should be UINT64 not UINTN > > > /** > >Enable interrupt source Source. > > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > index 64afc4d..16683ef 100644 > > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > > @@ -79,11 +79,11 @@ ArmVirtGicArchLibConstructor ( > > > > // RegProp[0..1] == { GICD base, GICD size } > > DistBase = SwapBytes64 (Reg[0]); > > -ASSERT (DistBase < MAX_UINT32); > > +ASSERT (DistBase < MAX_UINT64); > > > > This becomes equivalent to 'DistBase != MAX_UINT64' given that a > UINT64 cannot exceed MAX_UINT64. That is a nonsensical thing to > assert, so it is better to simply drop it > > > // RegProp[2..3] == { GICR base, GICR size } > > RedistBase = SwapBytes64 (Reg[2]); > > -ASSERT (RedistBase < MAX_UINT32); > > +ASSERT (RedistBase < MAX_UINT64); > > > > Likewise > > > PcdSet64 (PcdGicDistributorBase, DistBase); > > PcdSet64 (PcdGicRedistributorsBase, RedistBase); > > @@ -117,8 +117,8 @@ ArmVirtGicArchLibConstructor ( > > > > DistBase = SwapBytes64 (Reg[0]); > > CpuBase = SwapBytes64 (Reg[2]); > > -ASSERT (DistBase < MAX_UINT32); > > -ASSERT (CpuBase < MAX_UINT32); > > +ASSERT (DistBase < MAX_UINT64); > > +ASSERT (CpuBase < MAX_UINT64); > > > > Likewise > > > PcdSet64 (PcdGicDistributorBase, DistBase); > > PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase); > > -- > > 2.7.4 > > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg ArmVirtPkg: fix the GIC base address variables as 64-bit
Hi Dennis, On 17 October 2016 at 06:03, Dennis Chenwrote: > Since ACPI spec defines the GIC base addresses (CPU interface, > Distributor and Redistributor*GICv3 only*) as 64-bit, so we > should define these corresponding base address variables as 64-bit > instead of 32-bit. This patch redefines them according to the > ACPI spec. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Signed-off-by: Dennis Chen > --- > ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c| 4 ++-- > ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 8 Could you split this patch in 2 please, and put Laszlo Ersek on cc for the ArmVirtPkg patch? > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > index b9ecd55..a4ba5cf 100644 > --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c > @@ -30,8 +30,8 @@ Abstract: > > extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol; > > -STATIC UINT32 mGicInterruptInterfaceBase; > -STATIC UINT32 mGicDistributorBase; > +STATIC UINTN mGicInterruptInterfaceBase; > +STATIC UINTN mGicDistributorBase; > This should be UINT64 not UINTN > /** >Enable interrupt source Source. > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > index 64afc4d..16683ef 100644 > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c > @@ -79,11 +79,11 @@ ArmVirtGicArchLibConstructor ( > > // RegProp[0..1] == { GICD base, GICD size } > DistBase = SwapBytes64 (Reg[0]); > -ASSERT (DistBase < MAX_UINT32); > +ASSERT (DistBase < MAX_UINT64); > This becomes equivalent to 'DistBase != MAX_UINT64' given that a UINT64 cannot exceed MAX_UINT64. That is a nonsensical thing to assert, so it is better to simply drop it > // RegProp[2..3] == { GICR base, GICR size } > RedistBase = SwapBytes64 (Reg[2]); > -ASSERT (RedistBase < MAX_UINT32); > +ASSERT (RedistBase < MAX_UINT64); > Likewise > PcdSet64 (PcdGicDistributorBase, DistBase); > PcdSet64 (PcdGicRedistributorsBase, RedistBase); > @@ -117,8 +117,8 @@ ArmVirtGicArchLibConstructor ( > > DistBase = SwapBytes64 (Reg[0]); > CpuBase = SwapBytes64 (Reg[2]); > -ASSERT (DistBase < MAX_UINT32); > -ASSERT (CpuBase < MAX_UINT32); > +ASSERT (DistBase < MAX_UINT64); > +ASSERT (CpuBase < MAX_UINT64); > Likewise > PcdSet64 (PcdGicDistributorBase, DistBase); > PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase); > -- > 2.7.4 > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] ArmPkg ArmVirtPkg: fix the GIC base address variables as 64-bit
Since ACPI spec defines the GIC base addresses (CPU interface, Distributor and Redistributor*GICv3 only*) as 64-bit, so we should define these corresponding base address variables as 64-bit instead of 32-bit. This patch redefines them according to the ACPI spec. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Ard BiesheuvelCc: Leif Lindholm Signed-off-by: Dennis Chen --- ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c| 4 ++-- ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 8 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c index b9ecd55..a4ba5cf 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -30,8 +30,8 @@ Abstract: extern EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptV2Protocol; -STATIC UINT32 mGicInterruptInterfaceBase; -STATIC UINT32 mGicDistributorBase; +STATIC UINTN mGicInterruptInterfaceBase; +STATIC UINTN mGicDistributorBase; /** Enable interrupt source Source. diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c index 64afc4d..16683ef 100644 --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c @@ -79,11 +79,11 @@ ArmVirtGicArchLibConstructor ( // RegProp[0..1] == { GICD base, GICD size } DistBase = SwapBytes64 (Reg[0]); -ASSERT (DistBase < MAX_UINT32); +ASSERT (DistBase < MAX_UINT64); // RegProp[2..3] == { GICR base, GICR size } RedistBase = SwapBytes64 (Reg[2]); -ASSERT (RedistBase < MAX_UINT32); +ASSERT (RedistBase < MAX_UINT64); PcdSet64 (PcdGicDistributorBase, DistBase); PcdSet64 (PcdGicRedistributorsBase, RedistBase); @@ -117,8 +117,8 @@ ArmVirtGicArchLibConstructor ( DistBase = SwapBytes64 (Reg[0]); CpuBase = SwapBytes64 (Reg[2]); -ASSERT (DistBase < MAX_UINT32); -ASSERT (CpuBase < MAX_UINT32); +ASSERT (DistBase < MAX_UINT64); +ASSERT (CpuBase < MAX_UINT64); PcdSet64 (PcdGicDistributorBase, DistBase); PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase); -- 2.7.4 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel