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"); 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
