[Linaro-uefi,Linaro-uefi,v1,06/21] Hisilicon: Fix ACPI/DSDT table checksum error

Message ID 1490015485-53685-7-git-send-email-chenhui.sun@linaro.org
State Superseded
Headers show
Series
  • D02/D03 platforms bug fix
Related show

Commit Message

Chenhui Sun March 20, 2017, 1:11 p.m.
From: Chenhui Sun <sunchenhui@huawei.com>

Refresh checksum after changing DSDT table.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: wanglijun <wanglijun@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
---
 Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Graeme Gregory March 21, 2017, 11:35 a.m. | #1
On Mon, Mar 20, 2017 at 09:11:10PM +0800, Chenhui Sun wrote:
> From: Chenhui Sun <sunchenhui@huawei.com>

> 

> Refresh checksum after changing DSDT table.

> 

This probably indicates we miss a test in ERP QA as we didn't notice
this.

Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>


> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: wanglijun <wanglijun@huawei.com>

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> ---

>  Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++

>  1 file changed, 22 insertions(+)

>

> diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c

> index 41f5692..98be4dc 100644

> --- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c

> +++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c

> @@ -442,6 +442,27 @@ static EFI_STATUS ProcessDSDT(

>    return EFI_SUCCESS;

>  }

>

> +VOID

> +AcpiPlatformChecksum (

> +  IN UINT8      *Buffer,

> +  IN UINTN      Size

> +  )

> +{

> +  UINTN ChecksumOffset;

> +

> +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);

> +

> +  //

> +  // set checksum to 0 first

> +  //

> +  Buffer[ChecksumOffset] = 0;

> +

> +  //

> +  // Update checksum value

> +  //

> +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);

> +}

> +

>  EFI_STATUS EthMacInit(void)

>  {

>    EFI_STATUS              Status;

> @@ -478,6 +499,7 @@ EFI_STATUS EthMacInit(void)

>      ProcessDSDT(AcpiTableProtocol, TableHandle);

>  

>      AcpiTableProtocol->Close(TableHandle);

> +    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);

>    }

>  

>    return EFI_SUCCESS;

> -- 

> 1.9.1

>
Leif Lindholm March 21, 2017, 2:48 p.m. | #2
On Mon, Mar 20, 2017 at 09:11:10PM +0800, Chenhui Sun wrote:
> From: Chenhui Sun <sunchenhui@huawei.com>
> 
> Refresh checksum after changing DSDT table.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: wanglijun <wanglijun@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> ---
>  Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> index 41f5692..98be4dc 100644
> --- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> +++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> @@ -442,6 +442,27 @@ static EFI_STATUS ProcessDSDT(
>    return EFI_SUCCESS;
>  }
>  

STATIC

> +VOID
> +AcpiPlatformChecksum (

EDK2 in general seems to camel-case also the final S in CheckSum, so
for consistency, please to the same (and also in variables below).

I do not see anything "Platform" about this. Since it is only a local
function, name is not very important, "AcpiCheckSum" would be
sufficient.

> +  IN UINT8      *Buffer,

It would be nicer to accept an EFI_ACPI_SDT_HEADER *, so that the
caller did not always need to cast when calling. And then add a local
UINT8 * to operate on. We would also then not need to pass the Size
separately.

Also, this is IN OUT. The buffer is being modified.

> +  IN UINTN      Size
> +  )
> +{
> +  UINTN ChecksumOffset;
> +
> +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
> +
> +  //
> +  // set checksum to 0 first
> +  //
> +  Buffer[ChecksumOffset] = 0;

Could this not be skipped if ...

> +
> +  //
> +  // Update checksum value
> +  //
> +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);

... we passed in "Size - 1" here?

> +}
> +
>  EFI_STATUS EthMacInit(void)
>  {
>    EFI_STATUS              Status;
> @@ -478,6 +499,7 @@ EFI_STATUS EthMacInit(void)
>      ProcessDSDT(AcpiTableProtocol, TableHandle);
>  
>      AcpiTableProtocol->Close(TableHandle);
> +    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
>    }
>  
>    return EFI_SUCCESS;
> -- 
> 1.9.1
>
Chenhui Sun March 31, 2017, 9 a.m. | #3
Hi Leif,


在 2017/3/21 22:48, Leif Lindholm 写道:
> On Mon, Mar 20, 2017 at 09:11:10PM +0800, Chenhui Sun wrote:
>> From: Chenhui Sun <sunchenhui@huawei.com>
>>
>> Refresh checksum after changing DSDT table.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: wanglijun <wanglijun@huawei.com>
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
>> ---
>>   Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
>> index 41f5692..98be4dc 100644
>> --- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
>> +++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
>> @@ -442,6 +442,27 @@ static EFI_STATUS ProcessDSDT(
>>     return EFI_SUCCESS;
>>   }
>>   
> STATIC
>
>> +VOID
>> +AcpiPlatformChecksum (
> EDK2 in general seems to camel-case also the final S in CheckSum, so
> for consistency, please to the same (and also in variables below).
Checksum is a word I think, not "Check" + "Sum", it is spelled as 
Checksum in Edk2 base code.
Could you check it again please? :)

> I do not see anything "Platform" about this. Since it is only a local
> function, name is not very important, "AcpiCheckSum" would be
> sufficient.
Ok
>> +  IN UINT8      *Buffer,
> It would be nicer to accept an EFI_ACPI_SDT_HEADER *, so that the
> caller did not always need to cast when calling. And then add a local
> UINT8 * to operate on. We would also then not need to pass the Size
> separately.
It should calculate all the table data checksum, not only the header.

Chenhui,
Thanks and Regards

> Also, this is IN OUT. The buffer is being modified.
>
>> +  IN UINTN      Size
>> +  )
>> +{
>> +  UINTN ChecksumOffset;
>> +
>> +  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
>> +
>> +  //
>> +  // set checksum to 0 first
>> +  //
>> +  Buffer[ChecksumOffset] = 0;
> Could this not be skipped if ...
>
>> +
>> +  //
>> +  // Update checksum value
>> +  //
>> +  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);
> ... we passed in "Size - 1" here?
>
>> +}
>> +
>>   EFI_STATUS EthMacInit(void)
>>   {
>>     EFI_STATUS              Status;
>> @@ -478,6 +499,7 @@ EFI_STATUS EthMacInit(void)
>>       ProcessDSDT(AcpiTableProtocol, TableHandle);
>>   
>>       AcpiTableProtocol->Close(TableHandle);
>> +    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
>>     }
>>   
>>     return EFI_SUCCESS;
>> -- 
>> 1.9.1
>>
Leif Lindholm April 3, 2017, 1:08 p.m. | #4
On Fri, Mar 31, 2017 at 05:00:38PM +0800, Chenhui Sun wrote:
> Hi Leif,
> 
> 
> 在 2017/3/21 22:48, Leif Lindholm 写道:
> >On Mon, Mar 20, 2017 at 09:11:10PM +0800, Chenhui Sun wrote:
> >>From: Chenhui Sun <sunchenhui@huawei.com>
> >>
> >>Refresh checksum after changing DSDT table.
> >>
> >>Contributed-under: TianoCore Contribution Agreement 1.0
> >>Signed-off-by: wanglijun <wanglijun@huawei.com>
> >>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> >>---
> >>  Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >>diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> >>index 41f5692..98be4dc 100644
> >>--- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> >>+++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
> >>@@ -442,6 +442,27 @@ static EFI_STATUS ProcessDSDT(
> >>    return EFI_SUCCESS;
> >>  }
> >STATIC
> >
> >>+VOID
> >>+AcpiPlatformChecksum (
> >EDK2 in general seems to camel-case also the final S in CheckSum, so
> >for consistency, please to the same (and also in variables below).
> Checksum is a word I think, not "Check" + "Sum", it is spelled as Checksum
> in Edk2 base code.
> Could you check it again please? :)

So, I actually agree with your argument. Which was why I went to look.
And MdePkg/Include/Library/BaseLib.h uses "CheckSum" consistently.

And it sticks out in this file because it calls those functions, with
the other form.

> 
> >I do not see anything "Platform" about this. Since it is only a local
> >function, name is not very important, "AcpiCheckSum" would be
> >sufficient.
> Ok
> >>+  IN UINT8      *Buffer,
> >It would be nicer to accept an EFI_ACPI_SDT_HEADER *, so that the
> >caller did not always need to cast when calling. And then add a local
> >UINT8 * to operate on. We would also then not need to pass the Size
> >separately.
> It should calculate all the table data checksum, not only the header.

Yes. But the current code does
  AcpiPlatformChecksum ((UINT8*)Table, Table->Length);

Where it could do
  AcpiPlatformChecksum (Table);

and the function could do:
  UINT8 *Buffer;
  Buffer = (UINT8 *)Table;
  Buffer[ChecksumOffset] = CalculateCheckSum8 (Buffer, Table->Length);

Regards,

Leif

> 
> Chenhui,
> Thanks and Regards
> 
> >Also, this is IN OUT. The buffer is being modified.
> >
> >>+  IN UINTN      Size
> >>+  )
> >>+{
> >>+  UINTN ChecksumOffset;
> >>+
> >>+  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
> >>+
> >>+  //
> >>+  // set checksum to 0 first
> >>+  //
> >>+  Buffer[ChecksumOffset] = 0;
> >Could this not be skipped if ...
> >
> >>+
> >>+  //
> >>+  // Update checksum value
> >>+  //
> >>+  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);
> >... we passed in "Size - 1" here?
> >
> >>+}
> >>+
> >>  EFI_STATUS EthMacInit(void)
> >>  {
> >>    EFI_STATUS              Status;
> >>@@ -478,6 +499,7 @@ EFI_STATUS EthMacInit(void)
> >>      ProcessDSDT(AcpiTableProtocol, TableHandle);
> >>      AcpiTableProtocol->Close(TableHandle);
> >>+    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
> >>    }
> >>    return EFI_SUCCESS;
> >>-- 
> >>1.9.1
> >>
>

Patch hide | download patch | download mbox

diff --git a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
index 41f5692..98be4dc 100644
--- a/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
+++ b/Chips/Hisilicon/Drivers/AcpiPlatformDxe/EthMac.c
@@ -442,6 +442,27 @@  static EFI_STATUS ProcessDSDT(
   return EFI_SUCCESS;
 }
 
+VOID
+AcpiPlatformChecksum (
+  IN UINT8      *Buffer,
+  IN UINTN      Size
+  )
+{
+  UINTN ChecksumOffset;
+
+  ChecksumOffset = OFFSET_OF (EFI_ACPI_DESCRIPTION_HEADER, Checksum);
+
+  //
+  // set checksum to 0 first
+  //
+  Buffer[ChecksumOffset] = 0;
+
+  //
+  // Update checksum value
+  //
+  Buffer[ChecksumOffset] = CalculateCheckSum8(Buffer, Size);
+}
+
 EFI_STATUS EthMacInit(void)
 {
   EFI_STATUS              Status;
@@ -478,6 +499,7 @@  EFI_STATUS EthMacInit(void)
     ProcessDSDT(AcpiTableProtocol, TableHandle);
 
     AcpiTableProtocol->Close(TableHandle);
+    AcpiPlatformChecksum ((UINT8*)Table, Table->Length);
   }
 
   return EFI_SUCCESS;