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

[Bin Meng]

On Wed, Feb 8, 2023 at 2:15 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Wed, Feb 08, 2023 at 12:10:55AM +0800, Bin Meng wrote:
> > On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> > >
> > > On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> > > > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 
> > > > 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.
> > > >
> > > > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > > > some refactoring work is needed before ACPI support fully lands on
> > > > RISC-V.
> > > > For example, we can extract the common part among x86/arm/riscv into a
> > > > separate file, like hw/acpi/acpi-build.c?
> > > >
> > >
> > > While it appears like there is same code in multiple places, those
> > > functions take architecture specific MachineState parameter. Some tables
> > > like MADT though with same name, will have different contents for
> > > different architectures.
> > >
> > > Only one function which Daniel also pointed is the wrapper
> > > acpi_align_size() which can be made common. I am not
> > > sure whether it is worth of refactoring.
> > >
> >
> > It's more than that. For example,
> >
> > With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
> > of cpus, so there is no need to pass the architecture specific
> > MachineState as the parameter.
> >
> I would not make this function common. The reason is, an architecture may
> choose to attach different ACPI objects under the CPU node.
>

Is it possible to insert architect specific CPU nodes after this
common API builds the basic CPU node in DSDT?

> > Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.
> >
> The issue is, these things are not exactly common across all architectures.
> x86 has bit different data in these objects. While today it appears they
> are same for riscv and arm, in future things may change for an architecture.
> It doesn't look like it is a standard practice to build files under
> hw/acpi for specific architectures. Hence, IMO, it is better to keep these
> things in architecture specific folders to allow them to do differently in
> future.
>

It looks like hw/acpi/ghes.c is for Arm from MAINTAINERS.

> But I look forward for the feedback from other architecture maintainers on
> this topic. My experience in qemu is very limited. So, I need help from
> experts.

+ Michael and Igor

Regards,
Bin