[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers |
Date: |
Mon, 14 Dec 2020 14:49:23 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 |
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
>
> 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
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Huacai Chen, 2020/12/01
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Philippe Mathieu-Daudé, 2020/12/02
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Huacai Chen, 2020/12/10
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Philippe Mathieu-Daudé, 2020/12/11
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Philippe Mathieu-Daudé, 2020/12/13
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Philippe Mathieu-Daudé, 2020/12/13
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Huacai Chen, 2020/12/13
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers,
Philippe Mathieu-Daudé <=
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Huacai Chen, 2020/12/15
- Re: [PATCH V17 4/6] hw/mips: Add Loongson-3 boot parameter helpers, Philippe Mathieu-Daudé, 2020/12/15