Re: [Qemu-devel] [PULL 18/76] optionrom: add new PVH option rom

[Stefano Garzarella]

On Fri, Mar 19, 2021 at 06:52:39PM +0100, Paolo Bonzini wrote:
> It's likely that the compiler will online it. But indeed it's better to add
> -minline-all-stringops to the compiler command line.

Cool, I didn't know that one!
I tried but I did something wrong because the linker is not happy, next 
week I'll check better:
ld: pvh_main.o: in function `search_rsdp':
/home/stefano/repos/qemu/pc-bios/optionrom/pvh_main.c:62: undefined reference 
to `memcmp'
ld: /home/stefano/repos/qemu/pc-bios/optionrom/pvh_main.c:62: undefined 
reference to `memcmp'


In the mean time, I simply tried to assign the RSDP_SIGNATURE macro to 
an uint64_t variable and I have this new warning, using gcc 10.2.1 "cc 
(Alpine 10.2.1_pre2) 10.2.1 20210313":

In file included from /tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c:25:
/tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c: In function 'search_rsdp':
/tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c:30:42: warning: conversion from 
'long long unsigned int' to 'uint64_t' {aka 'long unsigned int'} changes value 
from '2329016660419433298' to '541348690' [-Woverflow]
   30 | #define RSDP_SIGNATURE          UINT64_C(0x2052545020445352) /* "RSD PTR 
" */
      |                                          ^~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c:69:31: note: in expansion of 
macro 'RSDP_SIGNATURE'
   69 |     uint64_t rsdp_signature = RSDP_SIGNATURE;
      |

Using gcc (GCC) 10.2.1 20201125 (Red Hat 10.2.1-9) I don't have it.

It seems a bit strange, I don't know if it's related to the fact that we 
compile with -m16, I'll check better.

Thanks,
Stefano

> Paolo
>
> Il ven 19 mar 2021, 18:35 Stefano Garzarella <sgarzare@redhat.com> ha
> scritto:
>
> > On Fri, Mar 19, 2021 at 06:03:59PM +0100, Paolo Bonzini wrote:
> > >On 19/03/21 15:06, Philippe Mathieu-Daudé wrote:
> > >>>+
> > >>>+/* Search RSDP signature. */
> > >>>+static uintptr_t search_rsdp(uint32_t start_addr, uint32_t end_addr)
> > >>>+{
> > >>>+    uint64_t *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) {
> > >>>+            return (uintptr_t)rsdp_p;
> > >>>+        }
> > >>>+    }
> > >>>+
> > >>>+    return 0;
> > >>>+}
> > >>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) {
> > >>       |                     ^~
> > >
> > >This is probably a different bug, but I'll also add that uint64_t is
> > >supposed to be aligned to 64 bits, so you need either
> > >__attribute__((packed)), or use char* and memcmp.  If you go for the
> > >latter, it would fix the issue that Philippe is reporting.
> >
> > Yes, memcmp maybe is also more readable, but being baremetal, I have to
> > implement it right?
> >
> > Thanks,
> > Stefano