Re: [PATCH V2 04/10] hw/riscv/virt: virt-acpi-build.c: Add basic ACPI tables

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 2/14/23 00:43, Sunil V L wrote:
> On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
>> Sunil,
>>
>> This patch is a bit confusing to me. You're using functions that doesn't exist
>> in the code base yet (build_madt and build_rhct) because they are introduced
>> in later patches. This also means that this patch is not being compiled tested,
>> because otherwise it would throw a compile error. And the build of the file only
>> happens after patch 8.
>>
> My intention was to add the caller also in the same patch where the
> function is added. I think I missed it when I split. Thanks!
> 
>> Now, there is no hard rule in QEMU that dictates that every patch must be compile
>> tested, but nevertheless this is a good rule to follow that makes our lives easier
>> when bisecting and cherry-pick individual patches.
>>
>> My suggestion for this patch is:
>>
>> - squash both patches 7 and 8 into this patch to allow the file to be built;
>>
> Sure.
> 
>> - remove the code that is referring to stuff that you haven't add yet:
>>
>> $ git diff
>> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
>> index 3c4da6c385..eb17029b64 100644
>> --- a/hw/riscv/virt-acpi-build.c
>> +++ b/hw/riscv/virt-acpi-build.c
>> @@ -156,12 +156,6 @@ virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
>> -    acpi_add_table(table_offsets, tables_blob);
>> -    build_madt(tables_blob, tables->linker, s);
>> -
>> -    acpi_add_table(table_offsets, tables_blob);
>> -    build_rhct(tables_blob, tables->linker, s);
>> -
>>       /* XSDT is pointed to by RSDP */
>>       xsdt = tables_blob->len;
>>       build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
>>
>>
>> - in patch 5, add back the lines in virt_acpi_build() that uses build_madt();
>>
>> - in patch 6, add back the lines in virt_acpi_build() that uses build_rhct();
>>
>>
>> This will make this patch to be compiled and built right away without interfering with
>> the end result of the series.
>>
> Thanks!
>   
>>
>> One more suggestion:
>>
>>
>> On 2/13/23 11:40, Sunil V L wrote:
>>> Add few basic ACPI tables and DSDT with few devices in a
>>> new file virt-acpi-build.c.
>>>
>>> These are mostly leveraged from arm64.
>>
>> Quick rant that you've already heard: I don't really understand why there is so
>> much ACPI code duplication everywhere. I really don't. E.g. acpi_align_size() is
>> copied verbatim in hw/arm/virt-acpi-build.c, hw/i386/acpi-build.c and
>> hw/loongarch/acpi-build.c. I don't see why we can't have a common file in hw/acpi
>> with helpers and so on that every ACPI architecture can use, and then the
>> individual drivers for each arch can have its own magic sauce.
>>
> I completely agree that we better avoid duplication But I am bit
> hesitant in this case because,
> 1) Low level functions which help in creating the namespace/static
> tables are already common (ex: aml_append)
> 
> 2) Using these basic routines, individual platforms can create the
> namespace with appropriate devices and the methods.
> 
> ACPI name space is tightly coupled with a platform. While there may be
> common devices like processors, uart etc, there can be difference in the
> ACPI methods for each of those devices. For ex: CPU objects for one
> platform may support _LPI method. uart may support _DSD for one platform
> and other may use totally different UART. If we have to create common routines,
> we will have to decide on all parameters the common function would need for
> different platforms. Same concern with fw_cfg/virtio etc which look same
> now but in future one platform can add a new method for these devices.
> 
> IMHO, even though it looks like we have the same function in each architecture
> currently, this model allows us to have platforms with different devices with
> different methods/features. Creating common routines now would probably make
> them difficult to use in future.
> 
> acpi_align_size() is a simple wrapper. We don't need it if we directly
> use the common function.
> 
> Since I see insistence let me try moving few functions like fw_cfg (virtio, pci in
> future) to a common file in hw/acpi.

Nah. Doing that now will make this series rely on acks for every other ACPI arch to
push the RISC-V side.

Let's make this happen as is now to get ACPI in RISC-V working. We can think about
reducing overall ACPI duplication later. IMO it's enough for now to, mention in this
commit msg, which bits of the arm64 virt-acpi-build.c you changed for this RISC-V
version.


Thanks,

Daniel

>   
>> All this said, instead of mentioning "this is mostly leveraged from arm64", you
>> can make a brief summary of the changes you've done from the arm64 version. This
>> will help guide the review into focusing on the novel code you're adding and
>> ignore the copied bits.
>>
> Sure.
> 
> Thanks!
> Sunil