> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: 15 December 2014 12:01 > To: [email protected]; [email protected]; > [email protected]; [email protected] > Subject: Re: [edk2] [PATCH v3 01/13] ArmVirtualizationPkg: VirtFdtDxe: > forward FwCfg addresses from DTB to PCDs > > On 12/15/14 12:40, Olivier Martin wrote: > > > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:[email protected]] > >> Sent: 11 December 2014 02:46 > >> To: [email protected]; [email protected]; > >> [email protected]; [email protected] > >> Subject: [edk2] [PATCH v3 01/13] ArmVirtualizationPkg: VirtFdtDxe: > >> forward FwCfg addresses from DTB to PCDs > >> > >> Qemu's firmware configuration interface for ARM consists of two MMIO > >> registers, a 16-bit selector, and a 64-bit data register that allows > >> the > >> guest to transfer data with 8, 16, 32, and 64-bit wide accesses. > Parse > >> the > >> base address from the DTB, and expose the registers to the rest of > DXE > >> via > >> dynamic PCDs. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek <[email protected]> > >> --- > >> > >> Notes: > >> v3: > >> - updated to the layout with 64-bit wide data register > >> > >> v2: > >> - use "qemu,fw-cfg-mmio" as compatible property > >> - the DTB node specifies a single contiguous region > >> > >> ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf | 2 > ++ > >> ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c | 24 > >> ++++++++++++++++++++++++ > >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec | 3 > +++ > >> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc | 3 > +++ > >> 4 files changed, 32 insertions(+) > >> > >> diff --git > >> a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf > >> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf > >> index 1c9dd20..daafe6c 100644 > >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf > >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf > >> @@ -47,6 +47,8 @@ > >> [Pcd] > >> gArmVirtualizationTokenSpaceGuid.PcdDeviceTreeBaseAddress > >> gArmVirtualizationTokenSpaceGuid.PcdArmPsciMethod > >> + gArmVirtualizationTokenSpaceGuid.PcdFwCfgSelectorAddress > >> + gArmVirtualizationTokenSpaceGuid.PcdFwCfgDataAddress > >> gArmTokenSpaceGuid.PcdGicDistributorBase > >> gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase > >> gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum > >> diff --git > >> a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c > >> b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c > >> index d002e66..7050196 100644 > >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c > >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c > >> @@ -44,6 +44,7 @@ typedef enum { > >> PropertyTypeUart, > >> PropertyTypeTimer, > >> PropertyTypePsci, > >> + PropertyTypeFwCfg, > >> } PROPERTY_TYPE; > >> > >> typedef struct { > >> @@ -59,6 +60,7 @@ STATIC CONST PROPERTY CompatibleProperties[] = { > >> { PropertyTypeTimer, "arm,armv7-timer" }, > >> { PropertyTypeTimer, "arm,armv8-timer" }, > >> { PropertyTypePsci, "arm,psci-0.2" }, > >> + { PropertyTypeFwCfg, "qemu,fw-cfg-mmio" }, > >> { PropertyTypeUnknown, "" } > >> }; > >> > >> @@ -115,6 +117,10 @@ InitializeVirtFdtDxe ( > >> CONST INTERRUPT_PROPERTY *InterruptProp; > >> INT32 SecIntrNum, IntrNum, VirtIntrNum, > >> HypIntrNum; > >> CONST CHAR8 *PsciMethod; > >> + UINT64 FwCfgSelectorAddress; > >> + UINT64 FwCfgSelectorSize; > >> + UINT64 FwCfgDataAddress; > >> + UINT64 FwCfgDataSize; > >> > >> DeviceTreeBase = (VOID *)(UINTN)PcdGet64 > (PcdDeviceTreeBaseAddress); > >> ASSERT (DeviceTreeBase != NULL); > >> @@ -160,6 +166,24 @@ InitializeVirtFdtDxe ( > >> (PropType == PropertyTypePsci)); > >> > >> switch (PropType) { > >> + case PropertyTypeFwCfg: > >> + ASSERT (Len == 2 * sizeof (UINT64)); > >> + > >> + FwCfgDataAddress = fdt64_to_cpu (((UINT64 *)RegProp)[0]); > >> + FwCfgDataSize = 8; > >> + FwCfgSelectorAddress = FwCfgDataAddress + FwCfgDataSize; > >> + FwCfgSelectorSize = 2; > >> + > >> + ASSERT (FwCfgSelectorAddress <= MAX_UINTN - FwCfgSelectorSize > + > >> 1); > >> + ASSERT (FwCfgDataAddress <= MAX_UINTN - FwCfgDataSize > + > >> 1); > > > > I am not sure to understand these ASSERT(). > > > > Should it not be: > > ASSERT (FwCfgSelectorAddress < (1 << (FwCfgSelectorSize - 1))); > > ASSERT (FwCfgDataAddress < (1 << (FwCfgDataSize - 1)); > > > > Or > > > > ASSERT (FwCfgSelectorAddress <= MAX_UINT64); // This one is likely to > always > > be true. > > ASSERT (FwCfgDataAddress <= MAX_UINT16); > > > > If 'Address' means something else than base address of the MMIO > registers > > maybe a comment should be added to clarify these two assertions. > > The ASSERT()s express that the last byte in each MMIO range is > expressible as a MAX_UINTN. Mathematically, > > Address + Size - 1 <= MAX_UINTN > > However, in C, if the left hand side (which is performed in UINT64) > contained an unsigned overflow -- because Address were too high -- then > a check of the form > > ASSERT (Address + Size - 1 <= MAX_UINTN) > > would precisely miss it -- the overflow would happen before the > comparison. > > For this reason, the equivalent mathematical form > > Address <= ((MAX_UINTN - Size) + 1) > > (which emerges by subtracting (Size - 1) from both sides) is coded, > which is safe. > > Namely, the Size variables are set to very small, but still positive, > constant values. Hence, while the above expression is mathematically > identical, it has no chance to overflow or underflow in C either > (unlike > the more obvious coding). > > Thanks > Laszlo >
I am happy with the explanation. Could you add it to the source code? As you said, it does not follow the obvious way to code to it might be confusing. > > > >> + > >> + PcdSet64 (PcdFwCfgSelectorAddress, FwCfgSelectorAddress); > >> + PcdSet64 (PcdFwCfgDataAddress, FwCfgDataAddress); > >> + > >> + DEBUG ((EFI_D_INFO, "Found FwCfg @ 0x%Lx/0x%Lx\n", > >> FwCfgSelectorAddress, > >> + FwCfgDataAddress)); > >> + break; > >> + > >> case PropertyTypeVirtio: > >> ASSERT (Len == 16); > >> // > >> diff --git > >> a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec > >> b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec > >> index b581add..9941154 100644 > >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec > >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec > >> @@ -53,3 +53,6 @@ > >> # PcdArmPsciMethod == 2 : use SMC > >> # > >> > >> > gArmVirtualizationTokenSpaceGuid.PcdArmPsciMethod|0|UINT32|0x00000003 > >> + > >> + > >> > gArmVirtualizationTokenSpaceGuid.PcdFwCfgSelectorAddress|0x0|UINT64|0x0 > >> 0000004 > >> + > >> > gArmVirtualizationTokenSpaceGuid.PcdFwCfgDataAddress|0x0|UINT64|0x00000 > >> 005 > >> diff --git > >> a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> index 61689b7..1f3ddea 100644 > >> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationQemu.dsc > >> @@ -173,6 +173,9 @@ > >> > >> gArmVirtualizationTokenSpaceGuid.PcdArmPsciMethod|0 > >> > >> + gArmVirtualizationTokenSpaceGuid.PcdFwCfgSelectorAddress|0x0 > >> + gArmVirtualizationTokenSpaceGuid.PcdFwCfgDataAddress|0x0 > >> + > >> > >> > ####################################################################### > >> ######### > >> # > >> # Components Section - list of all EDK II Modules needed by this > >> Platform > >> -- > >> 1.8.3.1 > >> > >> > >> > >> -------------------------------------------------------------------- > --- > >> ------- > >> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server > >> from Actuate! Instantly Supercharge Your Business Reports and > >> Dashboards > >> with Interactivity, Sharing, Native Excel Exports, App Integration & > >> more > >> Get technology previously reserved for billion-dollar corporations, > >> FREE > >> > http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.c > >> lktrk > >> _______________________________________________ > >> edk2-devel mailing list > >> [email protected] > >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > > > > > > > > --------------------------------------------------------------------- > --------- > > Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server > > from Actuate! Instantly Supercharge Your Business Reports and > Dashboards > > with Interactivity, Sharing, Native Excel Exports, App Integration & > more > > Get technology previously reserved for billion-dollar corporations, > FREE > > > http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.c > lktrk > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > ----------------------------------------------------------------------- > ------- > Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server > from Actuate! Instantly Supercharge Your Business Reports and > Dashboards > with Interactivity, Sharing, Native Excel Exports, App Integration & > more > Get technology previously reserved for billion-dollar corporations, > FREE > http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.c > lktrk > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
