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

[Huacai Chen]

Hi, Philippe,

On Mon, Dec 14, 2020 at 7:09 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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 {

This seems not allowed. In include/qemu/bswap.h it says:
 * Do an in-place conversion of the value pointed to by @v from the
 * native endianness of the host CPU to the specified format.
 *
 * Both X_to_cpu() and cpu_to_X() perform the same operation; you
 * should use whichever one is better documenting of the function your
 * code is performing.
 *
 * Do not use these functions for conversion of values which are in guest
 * memory, since the data may not be sufficiently aligned for the host CPU's
 * load and store instructions. Instead you should use the ld*_p() and
 * st*_p() functions, which perform loads and stores of data of any
 * required size and endianness and handle possible misalignment.

And there is a very strange problem, nearly all 32bit members are
after a 16bit vers member, why only processor_id is special? Compiler
bug?

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