Sent from my iPhone
> On Nov 13, 2014, at 11:14 AM, Jordan Justen <jordan.l.jus...@intel.com> wrote: > >> On 2014-11-13 09:46:29, Laszlo Ersek wrote: >>> On 11/13/14 18:40, Gabriel Somlo wrote: >>>> On Thu, 13 Nov 2014 09:27:09 -0600, Scott Duplichan wrote: >>>> Gabriel Somlo [mailto:gso...@gmail.com] wrote: >>>> ]On Sun, Nov 09, 2014 at 05:00:38PM -0800, Jordan Justen wrote: >>>> ]> On 2014-11-08 14:01:56, Gabriel L. Somlo wrote: >>>> ]> > @@ -228,6 +229,11 @@ MiscInitialization ( >>>> ]> > VOID >>>> ]> > ) >>>> ]> > { >>>> ]> > + UINT16 HostBridgeDevId; >>>> ]> > + UINTN PmCmd; >>>> ]> > + UINTN Pmba; >>>> ]> > + UINTN PmRegMisc; >>>> ]> > + >>>> ]> > // >>>> ]> > // Disable A20 Mask >>>> ]> > // >>>> ]> > @@ -239,33 +245,47 @@ MiscInitialization ( >>>> ]> > BuildCpuHob (36, 16); >>>> ]> > >>>> ]> > // >>>> ]> > + // Query Host Bridge DID to determine platform type >>>> ]> > + // >>>> ]> > + HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID); >>>> ]> > + switch (HostBridgeDevId) { >>>> ]> > + case INTEL_82441_DEVICE_ID: >>>> ]> > + PmCmd = POWER_MGMT_REGISTER_PIIX4 (PCI_COMMAND_OFFSET); >>>> ]> > + Pmba = POWER_MGMT_REGISTER_PIIX4 (0x40); >>>> ]> > + PmRegMisc = POWER_MGMT_REGISTER_PIIX4 (0x80); >>>> ]> > + break; >>>> ]> > + case INTEL_Q35_MCH_DEVICE_ID: >>>> ]> > + PmCmd = POWER_MGMT_REGISTER_Q35 (PCI_COMMAND_OFFSET); >>>> ]> > + Pmba = POWER_MGMT_REGISTER_Q35 (0x40); >>>> ]> > + PmRegMisc = POWER_MGMT_REGISTER_Q35 (0x80); >>>> ]> > + break; >>>> ]> > + default: >>>> ]> > + DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: >>>> 0x%04x\n", >>>> ]> > + __FUNCTION__, HostBridgeDevId)); >>>> ]> > + ASSERT (FALSE); >>>> ]> >>>> ]> When I build IA32 (OvmfPkg/build.sh -a IA32) I get warnings about >>>> ]> possibly uninitialized variables. >>>> ]> >>>> ]> It seems to happen basically everwhere that you switch on >>>> ]> HostBridgeDevId. >>>> ] >>>> ]I wonder why it's only warning on IA32, not like its different on X64 :) >>>> >>>> Because GCC49 IA32 is an optimized build (-Os) and GCC49 X64 is not. >>> >>> OK, thanks, that makes sense :) >>> >>>> ]Assuming we're still on the same page, the question becomes how to >>>> ]shut up gcc. >>>> >>>> Using a phrase like 'shut up gcc' sort of implies the warning is in >>>> the bogus category, like so many from the Microsoft compilers. But >>>> this warning is valid, right? For example, if I run Ovmf in some >>>> environment where PIIX4 is not present, then the code will fail >>>> to find the ACPI timer I/O address. Why not return an error if >>>> no supported device ID is found. That will eliminate the warning, >>>> and prevent the code from running on the unsupported chipset. >>> >>> I'm using this type of switch statement from several places, some >>> of which return VOID. Right now, that's >>> >>> OvmfPkg/PlatformPei/Platform.c MiscInitialization () >>> OvmfPkg/Csm/CsmSupportLib/LegacyInterrupt.c LegacyInterruptInstall () >>> OvmfPkg/Library/AcpiTimerLib/*AcpiTimerLib.c AcpiTimerLibConstructor () >>> OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c PciAcpiInitialization () >>> >>> I could add a "return EFI_UNSUPPORTED" for those of them which return >>> a status code, or simply a "return" for those of type VOID, however: >>> >>> I'd still basically be just "shutting up gcc", because I'd place these >>> return statements after the ASSERT (FALSE). >>> >>> Once neither host bridge type we expect has been detected, the world >>> ends and returning "for real" makes no sense, as we're in an >>> unexpected/undefined state. Hence the ASSERT. >>> >>> Jordan, what do you think ? Adding a "return [EFI_UNSUPPORTED]" after >>> the ASSERT (FALSE), is that stylistically preferable over zeroing >>> out the values before the switch() ? >> >> (chiming in:) both have been employed in edk2. I *slightly* prefer the >> return statement after the ASSERT(), because that one makes the purpose >> obvious even without comments. > > Setting the defaults to zero is going to mean 'bad things happen' if > another devid is seen in a release build. So, I don't think it matters > too much. > > I already mentioned my (slight) preference would be to fallback to > assuming PIIX4, since qemu and related projects have used that for a > long time. But, with Anthony's Xen info, it seems like this is not > likely to be an issue. > > Regarding the assert, one thing I've seen (in other code bases) as an > alternative to > ASSERT (FALSE); > is something like > ASSERT (!"Unsupported host bridge"); > Most edk2 code has a comment before the ASSERT explaining it, or a DEBUG print. > I'm assuming EDK II's strict warnings would make this infeasible. :) > > -Jordan > > ------------------------------------------------------------------------------ > Comprehensive Server Monitoring with Site24x7. > Monitor 10 servers for $9/Month. > Get alerted through email, SMS, voice calls or mobile push notifications. > Take corrective actions from your mobile device. > http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
