Re: [PATCH V9 6/6] hw/mips: Add Loongson-3 machine support

[Huacai Chen]

Hi, Philippe,

On Thu, Sep 24, 2020 at 11:40 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 9/16/20 11:49 AM, Huacai Chen wrote:
> > Hi, Philippe,
> >
> > On Wed, Sep 16, 2020 at 3:56 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > wrote:
> >>
> >> Hi Huacai,
> >>
> >> On 9/16/20 4:12 AM, Huacai Chen wrote:
> ...
> >> hw/mips/loongson3_virt.c:373:15: note: each undeclared identifier is
> >> reported only once for each function it appears in
> >> hw/mips/loongson3_virt.c:373:15: error: excess elements in struct
> >> initializer [-Werror]
> >> hw/mips/loongson3_virt.c:373:15: note: (near initialization for 'freq_reg')
> >> hw/mips/loongson3_virt.c:374:9: error: unknown field 'addr' specified in
> >> initializer
> >>          .addr = (uintptr_t)(&freq)
> >>          ^
> >> hw/mips/loongson3_virt.c:374:17: error: excess elements in struct
> >> initializer [-Werror]
> >>          .addr = (uintptr_t)(&freq)
> >>                  ^
> >> hw/mips/loongson3_virt.c:374:17: note: (near initialization for 'freq_reg')
> >> hw/mips/loongson3_virt.c:372:24: error: storage size of 'freq_reg' isn't
> >> known
> >>      struct kvm_one_reg freq_reg = {
> >>                         ^
> >> hw/mips/loongson3_virt.c:380:41: error: 'KVM_GET_ONE_REG' undeclared
> >> (first use in this function)
> >>          ret = kvm_vcpu_ioctl(first_cpu, KVM_GET_ONE_REG, &freq_reg);
> >>                                          ^
> >> hw/mips/loongson3_virt.c:372:24: error: unused variable 'freq_reg'
> >> [-Werror=unused-variable]
> >>      struct kvm_one_reg freq_reg = {
> >>                         ^
> >> hw/mips/loongson3_virt.c: In function 'init_loongson_params':
> >> hw/mips/loongson3_virt.c:467:25: error: cast from pointer to integer of
> >> different size [-Werror=pointer-to-int-cast]
> >>      lp->memory_offset = (unsigned long long)init_memory_map(p)
> >>                          ^
> > I guess this happens on a 32bit platform where pointer is 32bit, and
> > could you please suggest a best solution for this? Maybe use uintptr_t
> > instead of unsigned long long?
>
> Since the machine doesn't have to know the EFI structures
> layout, I'd change your method to fill EFI structures as i.e.:
>
> /*
>  * @ptr: Pointer to fill
>  * @size: Buffer size available at @ptr
>  * Returns: Structure size filled on success, -1 on error.
>  */
> size_t fill_efi_memory_map_loongson(char *ptr, size_t size);
>
> And move that to hw/mips/loongson_efi.{c,h}.
>
> Then you don't need to worry about host pointer size, you just
> exchange buffer/size, then caller can round up and increment an
> offset.
All boot parameters are located in a small region, so a uint32_t seems
enough for xxx_offset, so uintptr_t is just OK, I think.

>
> BTW the EFI helpers are not endian safe.
>
> You should use the helpers described in docs/devel/loads-stores.rst:
>
> stw_le_p, stl_le_p(), ... (as I don't expect big-endian guests).
This seems like a very big project, but I will do it in the next version.

Huacai
>
> Regards,
>
> Phil.