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

[Stefano Garzarella]

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