Message ID | 20210322114834.166798-1-sgarzare@redhat.com |
---|---|
State | New |
Headers | show |
Series | optionrom/pvh: use memcmp() to find the RSDP signature | expand |
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; > } > } >
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
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; } }
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(-)