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

[Stefano Garzarella <sgarzare@redhat.com>]

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
>>
>>