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

[Philippe Mathieu-Daudé]

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?

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
>