Re: [PATCH 2/2] IntelSiliconPkg\Include\IndustryStandard: Update IGD_OPREGION_MBOX3 Structure

["Ni, Ray" <ray.ni@Intel.com>]

On 1/31/2019 2:55 PM, Ni, Ray wrote:
> On 1/31/2019 1:00 PM, Digant H Solanki wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1454
>> Based on latest IGD OpRegion Spec, IGD_OPREGION_MBOX3 needs to be 
>> updated with two new members : Physical Address of Raw VBT Data (RVDA) 
>> and Size of Raw VBT Data (RVDS)
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Digant H Solanki <digant.h.solanki@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
>> Cc: Ni, Ray <ray.ni@intel.com>
>> ---
>>   IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h 
>> b/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h
>> index 5ce80a5be8..1c83efee40 100644
>> --- a/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h
>> +++ b/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion.h
>> @@ -4,9 +4,7 @@
>>     
>> https://01.org/sites/default/files/documentation/skl_opregion_rev0p5.pdf
>> -  @note Fixed bug in the spec Mailbox3 - RM31 size from 0x45(69) to 
>> 0x46(70)
>> -
>> -  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2016 - 2019, Intel Corporation. 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
>> @@ -118,7 +116,9 @@ typedef struct {
>>     UINT64 FDSS;          ///< Offset 0x3AA DSS Buffer address 
>> allocated for IFFS feature
>>     UINT32 FDSP;          ///< Offset 0x3B2 Size of DSS buffer
>>     UINT32 STAT;          ///< Offset 0x3B6 State Indicator
>> -  UINT8  RM31[0x46];    ///< Offset 0x3BA - 0x3FF  Reserved Must be 
>> zero. Bug in spec 0x45(69)
>> +  UINT64 RVDA;          ///< Offset 0x3BA Physical address of Raw VBT 
>> data. Added from Spec Version 0.90 to support VBT greater than 6KB.
>> +  UINT32 RVDS;          ///< Offset 0x3C2 Size of Raw VBT data. Added 
>> from Spec Version 0.90 to support VBT greater than 6KB.
>> +  UINT8  RM31[0x3A];    ///< Offset 0x3C6 - 0x3FF  Reserved Must be 
>> zero.
>>   } IGD_OPREGION_MBOX3;
>>   ///
>>
> 
> 

Sorry seems my comment was missed in previous mail.
Is it possible that an existing consumer is already using
RM31 to access RVDA and RVDS? For example,
using " * (UINT64 *) RM31[0]" to access RVDA.
If it's possible, I suggest we rename "RM31" to a different name.
Otherwise, the original code that modifies RVDA using RM may
incorrectly modifies bytes at offset 0x3C6.
A build failure instead of a silent failure can be seen with the rename.

-- 
Thanks,
Ray