On 2014-11-12 05:38:28, Gabriel Somlo wrote: > On Tue, Nov 11, 2014 at 07:54:30PM -0800, Jordan Justen wrote: > > > > 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 :) > > > > > > > We should default to setting variables to the PIIX4 values in this > > > > case. > > > > > > > > I'm not sure if we should just skip the assert and use the PIIX4 > > > > values. > > > > > > With the understanding that I will obviously defer to your judgment > > > regarding style, I actually *like* the switch statement, complete with > > > ASSERT(FALSE) if something "surprising" happens that causes the world > > > to end... :) > > > > > > Assuming we're still on the same page, the question becomes how to > > > shut up gcc. We could initialize PmCmd, Pmba, and PmRegMisc to 0 when > > > we declare them: > > > > > > UINTN PmCmd = 0; > > > UINTN Pmba = 0; > > > UINTN PmRegMisc = 0; > > > > Unfortunately, it looks like the Coding Style document states: > > "Initializing a variable as part of its declaration is illegal." > > http://sourceforge.net/projects/edk2/files/General%20Documentation/EDK%20II%20C%20Coding%20Standards%20Specification.pdf/download > > > > > or right before the switch statement, but that feels cheesy because > > > it's gratuitous, so I don't like either alternative. > > > > > > Right now, I'm very tempted to initialize them to 0 *after* the > > > ASSERT(FALSE) with a comment: > > > > > > 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: x%04x\n", > > > __FUNCTION__, HostBridgeDevId)); > > > ASSERT (FALSE); > > > + PmCmd = Pmba = PmRegMisc = 0; // suppress uninitialized use gcc > > > warning > > > > But, in this case, I think I prefer to allow PIIX4 to be used as a > > default. Well, only for release builds, but... how about this? : > > > > switch (HostBridgeDevId) { > > 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: x%04x\n", > > __FUNCTION__, HostBridgeDevId)); > > ASSERT (FALSE); > > // > > // Fall through to use PIIX4 locations by default > > // > > 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; > > } > > > > Hmmm, if I looked at this a few years down the road, I'll most likely > end up wondering if there was a clever reason to word it like that, > and simply shutting up compiler warnings would be low on my list of > suspects :) > > I think I'll just go for this instead: > > > UINTN PmCmd; > > UINTN Pmba; > > UINTN PmRegMisc; > > > // > // suppress uninitialized use gcc warning > // > > PmCmd = 0; > > Pmba = 0; > > PmRegMisc = 0; > > > > if it's still an option :)
Yeah, it is fine. Personally I wanted to see the code fallback to PIIX4 mode, but I don't feel that strongly about it. At least in more modern chipsets there can be multiple device ids mapping to the same basic chipset family. I'm not sure if that is the case with 440FX, so I have a slight concern that perhaps Xen's 440FX emulation might use a different id. Thus, it might be valuable to 'fallback' to the older path of assuming PIIX4 is present. But, it would also be good to rout out all the used ids and cover them in the switch. -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
