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

[Sunil V L <sunilvl@ventanamicro.com>]

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