1. Patchew
  2. QEMU
  3. View series

[PATCH] optionrom/pvh: use memcmp() to find the RSDP signature

Stefano Garzarella posted 1 patch 4 years, 10 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210322114834.166798-1-sgarzare@redhat.com
There is a newer version of this series
pc-bios/optionrom/pvh_main.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
[PATCH] optionrom/pvh: use memcmp() to find the RSDP signature
Posted by Stefano Garzarella 4 years, 10 months ago 
New versions of gcc report a potential error and there may be alignment
issues using uint64_t pointer to check the RSDP signature:

    gcc 10.2.1 "cc (Alpine 10.2.1_pre2) 10.2.1 20210313" reports:

    pc-bios/optionrom/pvh_main.c: In function 'search_rsdp':
    pc-bios/optionrom/pvh_main.c:61:21: warning: comparison is always false
    due to limited range of data type [-Wtype-limits]
       61 |         if (*rsdp_p == RSDP_SIGNATURE) {
          |                     ^~

Let's use memcmp() to get more readable code and avoid these issues.
Since pvh optionrom is baremetal, we use the compiler's __builtin_memcmp.

Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 pc-bios/optionrom/pvh_main.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/pc-bios/optionrom/pvh_main.c b/pc-bios/optionrom/pvh_main.c
index 28e79d7fc4..12202e38c2 100644
--- a/pc-bios/optionrom/pvh_main.c
+++ b/pc-bios/optionrom/pvh_main.c
@@ -27,7 +27,8 @@ asm (".code32"); /* this code will be executed in protected mode */
 #include "optrom_fw_cfg.h"
 #include "../../include/hw/xen/start_info.h"
 
-#define RSDP_SIGNATURE          0x2052545020445352LL /* "RSD PTR " */
+#define RSDP_SIGNATURE          "RSD PTR "
+#define RSDP_SIGNATURE_SIZE     8
 #define RSDP_AREA_ADDR          0x000E0000
 #define RSDP_AREA_SIZE          0x00020000
 #define EBDA_BASE_ADDR          0x0000040E
@@ -53,12 +54,14 @@ static uint8_t cmdline_buffer[CMDLINE_BUFSIZE];
 /* Search RSDP signature. */
 static uintptr_t search_rsdp(uint32_t start_addr, uint32_t end_addr)
 {
-    uint64_t *rsdp_p;
+    char rsdp_signature[RSDP_SIGNATURE_SIZE] = RSDP_SIGNATURE;
+    char *rsdp_p;
 
     /* RSDP signature is always on a 16 byte boundary */
-    for (rsdp_p = (uint64_t *)start_addr; rsdp_p < (uint64_t *)end_addr;
-         rsdp_p += 2) {
-        if (*rsdp_p == RSDP_SIGNATURE) {
+    for (rsdp_p = (char *)start_addr; rsdp_p < (char *)end_addr;
+         rsdp_p += 16) {
+        if (__builtin_memcmp(rsdp_p, rsdp_signature,
+                             RSDP_SIGNATURE_SIZE) == 0) {
             return (uintptr_t)rsdp_p;
         }
     }
-- 
2.30.2
Re: [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature
Posted by Philippe Mathieu-Daudé 4 years, 10 months ago 
On 3/22/21 12:48 PM, Stefano Garzarella wrote:
> New versions of gcc report a potential error and there may be alignment
> issues using uint64_t pointer to check the RSDP signature:
> 
>     gcc 10.2.1 "cc (Alpine 10.2.1_pre2) 10.2.1 20210313" reports:
> 
>     pc-bios/optionrom/pvh_main.c: In function 'search_rsdp':
>     pc-bios/optionrom/pvh_main.c:61:21: warning: comparison is always false
>     due to limited range of data type [-Wtype-limits]
>        61 |         if (*rsdp_p == RSDP_SIGNATURE) {
>           |                     ^~
> 
> Let's use memcmp() to get more readable code and avoid these issues.
> Since pvh optionrom is baremetal, we use the compiler's __builtin_memcmp.
> 
> Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  pc-bios/optionrom/pvh_main.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/optionrom/pvh_main.c b/pc-bios/optionrom/pvh_main.c
> index 28e79d7fc4..12202e38c2 100644
> --- a/pc-bios/optionrom/pvh_main.c
> +++ b/pc-bios/optionrom/pvh_main.c
> @@ -27,7 +27,8 @@ asm (".code32"); /* this code will be executed in protected mode */
>  #include "optrom_fw_cfg.h"
>  #include "../../include/hw/xen/start_info.h"
>  
> -#define RSDP_SIGNATURE          0x2052545020445352LL /* "RSD PTR " */
> +#define RSDP_SIGNATURE          "RSD PTR "
> +#define RSDP_SIGNATURE_SIZE     8
>  #define RSDP_AREA_ADDR          0x000E0000
>  #define RSDP_AREA_SIZE          0x00020000
>  #define EBDA_BASE_ADDR          0x0000040E
> @@ -53,12 +54,14 @@ static uint8_t cmdline_buffer[CMDLINE_BUFSIZE];
>  /* Search RSDP signature. */
>  static uintptr_t search_rsdp(uint32_t start_addr, uint32_t end_addr)
>  {
> -    uint64_t *rsdp_p;
> +    char rsdp_signature[RSDP_SIGNATURE_SIZE] = RSDP_SIGNATURE;

static const?

> +    char *rsdp_p;
>  
>      /* RSDP signature is always on a 16 byte boundary */
> -    for (rsdp_p = (uint64_t *)start_addr; rsdp_p < (uint64_t *)end_addr;
> -         rsdp_p += 2) {
> -        if (*rsdp_p == RSDP_SIGNATURE) {
> +    for (rsdp_p = (char *)start_addr; rsdp_p < (char *)end_addr;
> +         rsdp_p += 16) {
> +        if (__builtin_memcmp(rsdp_p, rsdp_signature,
> +                             RSDP_SIGNATURE_SIZE) == 0) {
>              return (uintptr_t)rsdp_p;
>          }
>      }
>
Re: [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature
Posted by Stefano Garzarella 4 years, 10 months ago 
On Mon, Mar 22, 2021 at 02:15:26PM +0100, Philippe Mathieu-Daudé wrote:
>On 3/22/21 12:48 PM, Stefano Garzarella wrote:
>> New versions of gcc report a potential error and there may be alignment
>> issues using uint64_t pointer to check the RSDP signature:
>>
>>     gcc 10.2.1 "cc (Alpine 10.2.1_pre2) 10.2.1 20210313" reports:
>>
>>     pc-bios/optionrom/pvh_main.c: In function 'search_rsdp':
>>     pc-bios/optionrom/pvh_main.c:61:21: warning: comparison is always false
>>     due to limited range of data type [-Wtype-limits]
>>        61 |         if (*rsdp_p == RSDP_SIGNATURE) {
>>           |                     ^~
>>
>> Let's use memcmp() to get more readable code and avoid these issues.
>> Since pvh optionrom is baremetal, we use the compiler's __builtin_memcmp.
>>
>> Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  pc-bios/optionrom/pvh_main.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/pc-bios/optionrom/pvh_main.c b/pc-bios/optionrom/pvh_main.c
>> index 28e79d7fc4..12202e38c2 100644
>> --- a/pc-bios/optionrom/pvh_main.c
>> +++ b/pc-bios/optionrom/pvh_main.c
>> @@ -27,7 +27,8 @@ asm (".code32"); /* this code will be executed in protected mode */
>>  #include "optrom_fw_cfg.h"
>>  #include "../../include/hw/xen/start_info.h"
>>
>> -#define RSDP_SIGNATURE          0x2052545020445352LL /* "RSD PTR " */
>> +#define RSDP_SIGNATURE          "RSD PTR "
>> +#define RSDP_SIGNATURE_SIZE     8
>>  #define RSDP_AREA_ADDR          0x000E0000
>>  #define RSDP_AREA_SIZE          0x00020000
>>  #define EBDA_BASE_ADDR          0x0000040E
>> @@ -53,12 +54,14 @@ static uint8_t cmdline_buffer[CMDLINE_BUFSIZE];
>>  /* Search RSDP signature. */
>>  static uintptr_t search_rsdp(uint32_t start_addr, uint32_t end_addr)
>>  {
>> -    uint64_t *rsdp_p;
>> +    char rsdp_signature[RSDP_SIGNATURE_SIZE] = RSDP_SIGNATURE;
>
>static const?

I'll fix in v2.

And I'll check better clang because gitlab CI [1] is failing:

     ld: pvh_main.o: in function `search_rsdp':
     /builds/sgarzarella/qemu/pc-bios/optionrom/pvh_main.c:63: undefined 
     reference to `memcmp'
     ld: /builds/sgarzarella/qemu/pc-bios/optionrom/pvh_main.c:63: 
     undefined reference to `memcmp'


Thanks,
Stefano

[1] https://gitlab.com/sgarzarella/qemu/-/jobs/1117036403#L385

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.