On Wed, Nov 12, 2014 at 11:41:54AM -0800, Jordan Justen wrote: > 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.
I gave a quick look, and it's look like Xen use the same Host Bridge. And running a guest under Xen gave the same device id, which is 0x1237. > 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. -- Anthony PERARD ------------------------------------------------------------------------------ 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
