* [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature
@ 2021-03-22 11:48 Stefano Garzarella
2021-03-22 13:15 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Stefano Garzarella @ 2021-03-22 11:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Philippe Mathieu-Daudé, qemu-devel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature
2021-03-22 11:48 [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature Stefano Garzarella
@ 2021-03-22 13:15 ` Philippe Mathieu-Daudé
2021-03-22 13:52 ` Stefano Garzarella
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-22 13:15 UTC (permalink / raw)
To: Stefano Garzarella, Paolo Bonzini; +Cc: qemu-devel
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;
> }
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature
2021-03-22 13:15 ` Philippe Mathieu-Daudé
@ 2021-03-22 13:52 ` Stefano Garzarella
0 siblings, 0 replies; 3+ messages in thread
From: Stefano Garzarella @ 2021-03-22 13:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-22 14:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-22 11:48 [PATCH] optionrom/pvh: use memcmp() to find the RSDP signature Stefano Garzarella
2021-03-22 13:15 ` Philippe Mathieu-Daudé
2021-03-22 13:52 ` Stefano Garzarella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).