On 2014-11-11 19:10:20, Gabriel Somlo 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 :)
>
> > 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
So, officially, it would need to be:
UINTN PmCmd;
UINTN Pmba;
UINTN PmRegMisc;
PmCmd = 0;
Pmba = 0;
PmRegMisc = 0;
(I think this rule is frequently broken though. :)
> 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;
}
> I can retrofit this on top of each patch in which I introduced a
> switch on HostBridgeDevId, unless you really resent the way it looks :)
>
> Please let me know what you think.
>
> Thanks,
> --Gabriel
>
>
> PS. Even once I fixed all of these, IA32 build is still busted right now
> (for a reason unrelated to q35):
>
> "/usr/bin/gcc" -g -fshort-wchar -fno-strict-aliasing -Wall -Werror
> -Wno-array-bounds -ffunction-sections -fdata-sections -c -include
> AutoGen.h -DSTRING_ARRAY_NAME=XenPvBlkDxeStrings -m32 -malign-double
> -fno-stack-protector -D EFI32 -Wno-address
> -Wno-unused-but-set-variable -Os -mno-mmx -mno-sse -o
> /home/somlo/KVM-OSX/SCRATCH/edk2/Build/OvmfIa32/DEBUG_GCC48/IA32/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe/OUTPUT/./BlockFront.obj
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/OvmfPkg/XenPvBlkDxe
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/Build/OvmfIa32/DEBUG_GCC48/IA32/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe/DEBUG
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/MdePkg
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/MdePkg/Include
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/MdePkg/Include/Ia32
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/OvmfPkg
> -I/home/somlo/KVM-OSX/SCRATCH/edk2/OvmfPkg/Include
> /home/somlo/KVM-OSX/SCRATCH/edk2/OvmfPkg/XenPvBlkDxe/BlockFront.c
> In file included from /usr/include/features.h:388:0,
> from /usr/include/inttypes.h:25,
> from
> /home/somlo/KVM-OSX/SCRATCH/edk2/OvmfPkg/XenPvBlkDxe/BlockFront.c:40:
> /usr/include/gnu/stubs.h:7:27: fatal error: gnu/stubs-32.h: No such
> file or directory
> # include <gnu/stubs-32.h>
> ^
> compilation terminated.
Try my [PATCH v2] OvmfPkg/XenPvBlkDxe: Don't include system inttypes.h
-Jordan
> make: ***
> [/home/somlo/KVM-OSX/SCRATCH/edk2/Build/OvmfIa32/DEBUG_GCC48/IA32/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe/OUTPUT/BlockFront.obj]
> Error 1
>
>
> build.py...
> : error 7000: Failed to execute command
> make tbuild
> [/home/somlo/KVM-OSX/SCRATCH/edk2/Build/OvmfIa32/DEBUG_GCC48/IA32/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe]
>
>
> build.py...
> : error F002: Failed to build module
> /home/somlo/KVM-OSX/SCRATCH/edk2/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> [IA32, GCC48, DEBUG]
>
> - Failed -
>
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
