* [patch] OvmfPkg:Fix VS2012 build failure
@ 2018-03-23 5:12 Dandan Bi
2018-03-23 10:10 ` Laszlo Ersek
0 siblings, 1 reply; 2+ messages in thread
From: Dandan Bi @ 2018-03-23 5:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Laszlo Ersek, Ard Biesheuvel
Initialize local variable to suppress warning C4701/C4703:
potentially uninitialized local variable/pointer variable.
1.In VirtualMemory.c:
Read of "PageMapLevel4Entry" in SetMemoryEncDe() is only
reached when "PageMapLevel4Entry" is got correctly.
2.In VirtioBlk.c:
Reads (dereferences) of "BufferMapping" and "BufferDeviceAddress"
in SynchronousRequest() are only reached if "BufferSize > 0" *and*
we map the data buffer successfully.
3.In VirtioScsi.c:
Reads (dereferences) of "InDataMapping" and "InDataDeviceAddress",
in VirtioScsiPassThru() are only reached if
"Packet->InTransferLength > 0", *and* we map the input buffer
successfully. The similar reason for "OutDataMapping" and
"OutDataDeviceAddress".
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 7 ++++++-
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 8 +++++++-
OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 10 +++++++++-
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index c1bfa35d7a..1f1225dd13 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -1,10 +1,10 @@
/** @file
Virtual Memory Management Services to set or clear the memory encryption bit
- Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
distribution. The full text of the license may be found at
@@ -594,10 +594,15 @@ SetMemoryEncDec (
UINT64 PgTableMask;
UINT64 AddressEncMask;
BOOLEAN IsWpEnabled;
RETURN_STATUS Status;
+ //
+ // Set LocalVariable to suppress incorrect compiler/analyzer warnigns.
+ //
+ PageMapLevel4Entry = NULL;
+
DEBUG ((
DEBUG_VERBOSE,
"%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
gEfiCallerBaseName,
__FUNCTION__,
diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 5559884345..653715fb7b 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -9,11 +9,11 @@
- Although the non-blocking interfaces of EFI_BLOCK_IO2_PROTOCOL could be a
good match for multiple in-flight virtio-blk requests, we stick to
synchronous requests and EFI_BLOCK_IO_PROTOCOL for now.
Copyright (C) 2012, Red Hat, Inc.
- Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
distribution. The full text of the license may be found at
@@ -262,10 +262,16 @@ SynchronousRequest (
EFI_STATUS Status;
EFI_STATUS UnmapStatus;
BlockSize = Dev->BlockIoMedia.BlockSize;
+ //
+ // Set LocalVariable to suppress incorrect compiler/analyzer warnigns.
+ //
+ BufferMapping = NULL;
+ BufferDeviceAddress = 0;
+
//
// ensured by VirtioBlkInit()
//
ASSERT (BlockSize > 0);
ASSERT (BlockSize % 512 == 0);
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 1a68f06210..81aa89e6d9 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -24,11 +24,11 @@
VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET could be a good match. That would
however require client code for the control queue, which is deemed
unreasonable for now.
Copyright (C) 2012, Red Hat, Inc.
- Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
distribution. The full text of the license may be found at
@@ -453,10 +453,18 @@ VirtioScsiPassThru (
EFI_PHYSICAL_ADDRESS OutDataDeviceAddress;
VOID *InDataBuffer;
UINTN InDataNumPages;
BOOLEAN OutDataBufferIsMapped;
+ //
+ // Set LocalVariable to suppress incorrect compiler/analyzer warnigns.
+ //
+ InDataMapping = NULL;
+ OutDataMapping = NULL;
+ InDataDeviceAddress = 0;
+ OutDataDeviceAddress = 0;
+
ZeroMem ((VOID*) &Request, sizeof (Request));
Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
CopyMem (&TargetValue, Target, sizeof TargetValue);
--
2.14.3.windows.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [patch] OvmfPkg:Fix VS2012 build failure
2018-03-23 5:12 [patch] OvmfPkg:Fix VS2012 build failure Dandan Bi
@ 2018-03-23 10:10 ` Laszlo Ersek
0 siblings, 0 replies; 2+ messages in thread
From: Laszlo Ersek @ 2018-03-23 10:10 UTC (permalink / raw)
To: Dandan Bi, edk2-devel; +Cc: Ard Biesheuvel
On 03/23/18 06:12, Dandan Bi wrote:
> Initialize local variable to suppress warning C4701/C4703:
> potentially uninitialized local variable/pointer variable.
>
> 1.In VirtualMemory.c:
> Read of "PageMapLevel4Entry" in SetMemoryEncDe() is only
> reached when "PageMapLevel4Entry" is got correctly.
>
> 2.In VirtioBlk.c:
> Reads (dereferences) of "BufferMapping" and "BufferDeviceAddress"
> in SynchronousRequest() are only reached if "BufferSize > 0" *and*
> we map the data buffer successfully.
>
> 3.In VirtioScsi.c:
> Reads (dereferences) of "InDataMapping" and "InDataDeviceAddress",
> in VirtioScsiPassThru() are only reached if
> "Packet->InTransferLength > 0"
(1) Please insert the following words right here: "on input".
>, *and* we map the input buffer
> successfully. The similar reason for "OutDataMapping" and
> "OutDataDeviceAddress".
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
> OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 7 ++++++-
> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 8 +++++++-
> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 10 +++++++++-
> 3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> index c1bfa35d7a..1f1225dd13 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> @@ -1,10 +1,10 @@
> /** @file
>
> Virtual Memory Management Services to set or clear the memory encryption bit
>
> - Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> distribution. The full text of the license may be found at
> @@ -594,10 +594,15 @@ SetMemoryEncDec (
> UINT64 PgTableMask;
> UINT64 AddressEncMask;
> BOOLEAN IsWpEnabled;
> RETURN_STATUS Status;
>
> + //
> + // Set LocalVariable to suppress incorrect compiler/analyzer warnigns.
> + //
(2) Can you replace "LocalVariable" with "PageMapLevel4Entry"?
(3) Please replace "warnigns" with "warnings". (This is my fault, I made
the typo in the TianoCore BZ that suggests this kind of comment. Sorry
about that.)
> + PageMapLevel4Entry = NULL;
> +
> DEBUG ((
> DEBUG_VERBOSE,
> "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
> gEfiCallerBaseName,
> __FUNCTION__,
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 5559884345..653715fb7b 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -9,11 +9,11 @@
> - Although the non-blocking interfaces of EFI_BLOCK_IO2_PROTOCOL could be a
> good match for multiple in-flight virtio-blk requests, we stick to
> synchronous requests and EFI_BLOCK_IO_PROTOCOL for now.
>
> Copyright (C) 2012, Red Hat, Inc.
> - Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> distribution. The full text of the license may be found at
> @@ -262,10 +262,16 @@ SynchronousRequest (
> EFI_STATUS Status;
> EFI_STATUS UnmapStatus;
>
> BlockSize = Dev->BlockIoMedia.BlockSize;
>
> + //
> + // Set LocalVariable to suppress incorrect compiler/analyzer warnigns.
> + //
(4) Please replace "LocalVariable" with "BufferMapping and
BufferDeviceAddress", and please also make sure that the line length
stays <= 79 characters.
(5) Same as (3).
> + BufferMapping = NULL;
> + BufferDeviceAddress = 0;
> +
> //
> // ensured by VirtioBlkInit()
> //
> ASSERT (BlockSize > 0);
> ASSERT (BlockSize % 512 == 0);
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 1a68f06210..81aa89e6d9 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -24,11 +24,11 @@
> VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET could be a good match. That would
> however require client code for the control queue, which is deemed
> unreasonable for now.
>
> Copyright (C) 2012, Red Hat, Inc.
> - Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> distribution. The full text of the license may be found at
> @@ -453,10 +453,18 @@ VirtioScsiPassThru (
> EFI_PHYSICAL_ADDRESS OutDataDeviceAddress;
> VOID *InDataBuffer;
> UINTN InDataNumPages;
> BOOLEAN OutDataBufferIsMapped;
>
> + //
> + // Set LocalVariable to suppress incorrect compiler/analyzer warnigns.
> + //
(6) Please replace "LocalVariable" with "InDataMapping, OutDataMapping,
InDataDeviceAddress and OutDataDeviceAddress", and please also make sure
that the line length stays <= 79 characters.
(7) Same as (3).
> + InDataMapping = NULL;
> + OutDataMapping = NULL;
> + InDataDeviceAddress = 0;
> + OutDataDeviceAddress = 0;
> +
> ZeroMem ((VOID*) &Request, sizeof (Request));
>
> Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
> CopyMem (&TargetValue, Target, sizeof TargetValue);
>
>
Thank you for this patch. With the above changes:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
In the future, can you please split similar patches into one patch per
module, or even function? While the code changes are not large,
verifying the statements 1., 2., and 3. in the commit message (against
the code) does take some time. To me it's easier to review such
reasoning if the arguments (and the code) are separated to different,
small patches.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-03-23 10:03 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-23 5:12 [patch] OvmfPkg:Fix VS2012 build failure Dandan Bi
2018-03-23 10:10 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox