Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers

[Philippe Mathieu-Daudé]

On 12/13/20 11:17 PM, Philippe Mathieu-Daudé wrote:
> On 12/11/20 12:32 PM, Philippe Mathieu-Daudé wrote:
>> On 12/11/20 3:46 AM, Huacai Chen wrote:
>>> Hi, Rechard and Peter,
>>>
>>> On Wed, Dec 2, 2020 at 5:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
>>> wrote:
>>>>
>>>> On 12/2/20 2:14 AM, Huacai Chen wrote:
>>>>> Hi, Phillippe,
>>>>>
>>>>> On Tue, Nov 24, 2020 at 6:25 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
>>>>> wrote:
>>>>>>
>>>>>> On 11/6/20 5:21 AM, Huacai Chen wrote:
>>>>>>> Preparing to add Loongson-3 machine support, add Loongson-3's LEFI (a
>>>>>>> UEFI-like interface for BIOS-Kernel boot parameters) helpers first.
>>>>>>>
>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>>>>>>> Co-developed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>>>> ---
>>>>>>>  hw/mips/loongson3_bootp.c | 165 +++++++++++++++++++++++++++++++
>>>>>>>  hw/mips/loongson3_bootp.h | 241 
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  hw/mips/meson.build       |   1 +
>>>>>>>  3 files changed, 407 insertions(+)
>>>>>>>  create mode 100644 hw/mips/loongson3_bootp.c
>>>>>>>  create mode 100644 hw/mips/loongson3_bootp.h
>>>>>>>
>>>>>>> diff --git a/hw/mips/loongson3_bootp.c b/hw/mips/loongson3_bootp.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3a16081
>>>>>>> --- /dev/null
>>>>>>> +++ b/hw/mips/loongson3_bootp.c
>>>>>>> @@ -0,0 +1,165 @@
>>>>>>> +/*
>>>>>>> + * LEFI (a UEFI-like interface for BIOS-Kernel boot parameters) helpers
>>>>>>> + *
>>>>>>> + * Copyright (c) 2018-2020 Huacai Chen (chenhc@lemote.com)
>>>>>>> + * Copyright (c) 2018-2020 Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>>>> + *
>>>>>>> + * This program is free software: you can redistribute it and/or modify
>>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>>> + * the Free Software Foundation, either version 2 of the License, or
>>>>>>> + * (at your option) any later version.
>>>>>>> + *
>>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>>>> + * GNU General Public License for more details.
>>>>>>> + *
>>>>>>> + * You should have received a copy of the GNU General Public License
>>>>>>> + * along with this program. If not, see 
>>>>>>> <https://www.gnu.org/licenses/>.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include "qemu/osdep.h"
>>>>>>> +#include "qemu/units.h"
>>>>>>> +#include "qemu/cutils.h"
>>>>>>> +#include "cpu.h"
>>>>>>> +#include "hw/boards.h"
>>>>>>> +#include "hw/mips/loongson3_bootp.h"
>>>>>>> +
>>>>>>> +#define LOONGSON3_CORE_PER_NODE 4
>>>>>>> +
>>>>>>> +static struct efi_cpuinfo_loongson *init_cpu_info(void *g_cpuinfo, 
>>>>>>> uint64_t cpu_freq)
>>>>>>> +{
>>>>>>> +    struct efi_cpuinfo_loongson *c = g_cpuinfo;
>>>>>>> +
>>>>>>> +    stl_le_p(&c->cputype, Loongson_3A);
>>>>>>> +    stl_le_p(&c->processor_id, MIPS_CPU(first_cpu)->env.CP0_PRid);
>>>>>>
>>>>>> Build failing with Clang:
>>>>>>
>>>>>> FAILED: libqemu-mips64el-softmmu.fa.p/hw_mips_loongson3_bootp.c.o
>>>>>> hw/mips/loongson3_bootp.c:35:15: error: taking address of packed member
>>>>>> 'processor_id' of class or structure 'efi_cpuinfo_loongson' may result
>>>>>> in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
>>>>>>     stl_le_p(&c->processor_id, MIPS_CPU(first_cpu)->env.CP0_PRid);
>>>>>>               ^~~~~~~~~~~~~~~
>>>>>> 1 error generated.
>>>>> We cannot reproduce it on X86/MIPS with clang...
>>>>
>>>> You can reproduce running the Clang job on Gitlab-CI:
>>>>
>>>> https://wiki.qemu.org/Testing/CI/GitLabCI
>>>>
>>>>> And I found that
>>>>> stl_le_p() will be __builtin_memcpy(), I don't think memcpy() will
>>>>> cause unaligned access. So, any suggestions?
>>
>> My understanding is the compiler is complaining for the argument
>> passed to the caller, with no knowledge of the callee implementation.
>>
>> Which makes me wonder if these functions are really inlined...
>>
>> Do we need to use QEMU_ALWAYS_INLINE for these LDST helpers?
> 
> No, this doesn't work neither.

Well, this works:

-- >8 --
@@ -32,7 +32,7 @@ static struct efi_cpuinfo_loongson *init_cpu_info(void
*g_cpuinfo, uint64_t cpu_
     struct efi_cpuinfo_loongson *c = g_cpuinfo;

     stl_le_p(&c->cputype, Loongson_3A);
-    stl_le_p(&c->processor_id, MIPS_CPU(first_cpu)->env.CP0_PRid);
+    c->processor_id = cpu_to_le32(MIPS_CPU(first_cpu)->env.CP0_PRid);
     if (cpu_freq > UINT_MAX) {
         stl_le_p(&c->cpu_clock_freq, UINT_MAX);
     } else {
---

> 
>>
>> I see Richard used it in commit 80d9d1c6785 ("cputlb: Split out
>> load/store_memop").
>>
>>>>
>>>> I'll defer this question to Richard/Peter who have deeper understanding.
>>> Any sugguestions? Other patches are updated, except this one.
>>
>> Searching on the list, I see Marc-André resolved that by
>> using a copy on the stack:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg614482.html
>>
>>>
>>> Huacai
>>>>
>>>>>
>>>>> Huacai
>>>
>>
>