[PATCH 1/1] OvmfPkg/QemuFwCfgLib: Suppress GCC49 IA32 build failure

[PATCH 1/1] OvmfPkg/QemuFwCfgLib: Suppress GCC49 IA32 build failure

[Brijesh Singh]

NumPages variable was introduced in commit 66c548be509d. In this commit
we allocate an intermediate buffer when SEV is enabled. The 'BounceBuffer'
variable points to the intermediate buffer pointer and NumPages variables
stores the number of pages. Later in the code, 'BounceBuffer' variable is
checked to see if we need to free the intermediate buffers. The code looks
correct, suppress the warning.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index dbebd36b1853..1b21ef094dc5 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -72,7 +72,7 @@ InternalQemuFwCfgDmaBytes (
   volatile FW_CFG_DMA_ACCESS *Access;
   UINT32                     AccessHigh, AccessLow;
   UINT32                     Status;
-  UINT32                     NumPages;
+  UINT32                     NumPages = 0;
   VOID                       *DmaBuffer, *BounceBuffer;
 
   ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
-- 
2.7.4

Re: [PATCH 1/1] OvmfPkg/QemuFwCfgLib: Suppress GCC49 IA32 build failure

[Laszlo Ersek]

Hi Brijesh,

On 07/11/17 19:08, Brijesh Singh wrote:
> NumPages variable was introduced in commit 66c548be509d. In this commit
> we allocate an intermediate buffer when SEV is enabled. The 'BounceBuffer'
> variable points to the intermediate buffer pointer and NumPages variables
> stores the number of pages. Later in the code, 'BounceBuffer' variable is
> checked to see if we need to free the intermediate buffers. The code looks
> correct, suppress the warning.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index dbebd36b1853..1b21ef094dc5 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -72,7 +72,7 @@ InternalQemuFwCfgDmaBytes (
>    volatile FW_CFG_DMA_ACCESS *Access;
>    UINT32                     AccessHigh, AccessLow;
>    UINT32                     Status;
> -  UINT32                     NumPages;
> +  UINT32                     NumPages = 0;
>    VOID                       *DmaBuffer, *BounceBuffer;
>  
>    ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
> 

In edk2 we don't initialize local variables in the ISO C sense of
"initialization" -- please use a separate assignment, plus add a code
comment like mentioned here:

https://bugzilla.tianocore.org/show_bug.cgi?id=607

Other than that, I agree that NumPages cannot be used without setting it
first. The only such path through the code would be if
InternalQemuFwCfgSevIsEnabled() returned FALSE. However, in that case,
BounceBuffer is set to NULL, and the final use of NumPages (on line 168)
is never reached.

Thank you,
Laszlo

Re: [PATCH 1/1] OvmfPkg/QemuFwCfgLib: Suppress GCC49 IA32 build failure

[Brijesh Singh]

On 07/11/2017 12:34 PM, Laszlo Ersek wrote:
> Hi Brijesh,
> 
> On 07/11/17 19:08, Brijesh Singh wrote:
>> NumPages variable was introduced in commit 66c548be509d. In this commit
>> we allocate an intermediate buffer when SEV is enabled. The 'BounceBuffer'
>> variable points to the intermediate buffer pointer and NumPages variables
>> stores the number of pages. Later in the code, 'BounceBuffer' variable is
>> checked to see if we need to free the intermediate buffers. The code looks
>> correct, suppress the warning.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> index dbebd36b1853..1b21ef094dc5 100644
>> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> @@ -72,7 +72,7 @@ InternalQemuFwCfgDmaBytes (
>>     volatile FW_CFG_DMA_ACCESS *Access;
>>     UINT32                     AccessHigh, AccessLow;
>>     UINT32                     Status;
>> -  UINT32                     NumPages;
>> +  UINT32                     NumPages = 0;
>>     VOID                       *DmaBuffer, *BounceBuffer;
>>   
>>     ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ ||
>>
> 
> In edk2 we don't initialize local variables in the ISO C sense of
> "initialization" -- please use a separate assignment, plus add a code
> comment like mentioned here:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=607
> 

Thanks for quick review Laszlo, I will update the patch with those comments.

-Brijesh