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

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Huacai Chen <chenhuacai@gmail.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	"David Hildenbrand" <david@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Aleksandar Markovic" <aleksandar.qemu.devel@gmail.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Huacai Chen" <zltjiangshi@gmail.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers
Date: Tue, 15 Dec 2020 11:21:04 +0100	[thread overview]
Message-ID: <CAAdtpL7HFWFe+fP6h5v4mw8_gbJaAFNS+sMSoxro2kn9cJ0i-w@mail.gmail.com> (raw)
In-Reply-To: <CAAhV-H7B3+nGOmxiibUDf7-v1eU=aqrK5YBM6wVac=nDz+U4PQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 8216 bytes --]

Le mar. 15 déc. 2020 06:34, Huacai Chen <chenhuacai@gmail.com> a écrit :

> Hi, Philippe,
>
> On Mon, Dec 14, 2020 at 9:49 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> >
> > On 12/14/20 3:37 AM, Huacai Chen wrote:
> > > 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?
> >
> > This is what I wonder since some time but I don't have the knowledge
> > to confirm.
> >
> > Indeed I commented the "stl_le_p(&c->processor_id, ...);" line,
> > and there is no error for the following 32-bit values, which are
> > similarly unlikely 32-bit aligned.
> >
> > FWIW I am using Fedora release 32 (Thirty Two), and 'cc -v':
> >
> >  clang version 10.0.1 (Fedora 10.0.1-3.fc32)
> >  Target: x86_64-unknown-linux-gnu
> >  clang -cc1 version 10.0.1 based upon LLVM 10.0.1 default target
> > x86_64-unknown-linux-gnu
> >
> Since cpu_to_le32() "solve" the problem here, I want to use
> cpu_to_lexx() for all members, do you agree?
>

Fine by me. We can switch back to the recommended ldst helpers later, when
someone figure out the problem.

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

[-- Attachment #2: Type: text/html, Size: 13413 bytes --]