Le mar. 15 déc. 2020 06:34, Huacai Chen <chenhua...@gmail.com> a écrit :
> Hi, Philippe, > > On Mon, Dec 14, 2020 at 9:49 PM Philippe Mathieu-Daudé <f4...@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é < > f4...@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é < > f4...@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é < > f4...@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é <f4...@amsat.org> > > >>>>>>>>> Signed-off-by: Huacai Chen <che...@lemote.com> > > >>>>>>>>> Co-developed-by: Jiaxun Yang <jiaxun.y...@flygoat.com> > > >>>>>>>>> Signed-off-by: Jiaxun Yang <jiaxun.y...@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 (che...@lemote.com) > > >>>>>>>>> + * Copyright (c) 2018-2020 Jiaxun Yang < > jiaxun.y...@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 >
