RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[JeeHeng Sia]

> -----Original Message-----
> From: Sunil V L <sunilvl@ventanamicro.com>
> Sent: Monday, January 8, 2024 7:35 PM
> To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Cc: JeeHeng Sia <jeeheng.sia@starfivetech.com>; qemu-arm@nongnu.org; 
> qemu-devel@nongnu.org; qemu-riscv@nongnu.org;
> mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; 
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; 
> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR 
> creation to common location
> 
> On Fri, Jan 05, 2024 at 09:19:14AM -0300, Daniel Henrique Barboza wrote:
> >
> >
> > On 1/5/24 06:06, Sia Jee Heng wrote:
> > > RISC-V should also generate the SPCR in a manner similar to ARM.
> > > Therefore, instead of replicating the code, relocate this function
> > > to the common AML build.
> > >
> > > Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> > > ---
> > >   hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
> > >   hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
> > >   include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
> > >   include/hw/acpi/aml-build.h |  4 +++
> > >   4 files changed, 115 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index af66bde0f5..1efa534aa8 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray 
> > > *tbl, uint32_t flags,
> > >       }
> > >   }
> > > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > > +                const AcpiSpcrData *f, const char *oem_id,
> > > +                const char *oem_table_id)
> > > +{
> > > +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> > > +                        .oem_table_id = oem_table_id };
> > > +
> > > +    acpi_table_begin(&table, table_data);
> > > +    /* Interface type */
> > > +    build_append_int_noprefix(table_data, f->interface_type, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data, 0, 3);
> > > +    /* Base Address */
> > > +    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> > > +                     f->base_addr.offset, f->base_addr.size,
> > > +                     f->base_addr.addr);
> > > +    /* Interrupt type */
> > > +    build_append_int_noprefix(table_data, f->interrupt_type, 1);
> > > +    /* IRQ */
> > > +    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
> > > +    /* Global System Interrupt */
> > > +    build_append_int_noprefix(table_data, f->interrupt, 4);
> > > +    /* Baud Rate */
> > > +    build_append_int_noprefix(table_data, f->baud_rate, 1);
> > > +    /* Parity */
> > > +    build_append_int_noprefix(table_data, f->parity, 1);
> > > +    /* Stop Bits */
> > > +    build_append_int_noprefix(table_data, f->stop_bits, 1);
> > > +    /* Flow Control */
> > > +    build_append_int_noprefix(table_data, f->flow_control, 1);
> > > +    /* Terminal Type */
> > > +    build_append_int_noprefix(table_data, f->terminal_type, 1);
> > > +    /* PCI Device ID  */
> > > +    build_append_int_noprefix(table_data, f->pci_device_id, 2);
> > > +    /* PCI Vendor ID */
> > > +    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> > > +    /* PCI Bus Number */
> > > +    build_append_int_noprefix(table_data, f->pci_bus, 1);
> > > +    /* PCI Device Number */
> > > +    build_append_int_noprefix(table_data, f->pci_device, 1);
> > > +    /* PCI Function Number */
> > > +    build_append_int_noprefix(table_data, f->pci_function, 1);
> > > +    /* PCI Flags */
> > > +    build_append_int_noprefix(table_data, f->pci_flags, 4);
> > > +    /* PCI Segment */
> > > +    build_append_int_noprefix(table_data, f->pci_segment, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data, 0, 4);
> > > +
> > > +    acpi_table_end(linker, &table);
> > > +}
> > >   /*
> > >    * ACPI spec, Revision 6.3
> > >    * 5.2.29 Processor Properties Topology Table (PPTT)
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 510ab0dcca..a31f736d1a 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> > > VirtMachineState *vms)
> > >    * Rev: 1.07
> > >    */
> > >   static void
> > > -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > +build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState 
> > > *vms)
> >
> > Nit: I don't understand the '_v2' in the name of this function. Is it just 
> > to not collide
> > with the now public build_spcr()? Or does it have to do with the SPCR table 
> > being
> > '.rev = 2'? Because if it's the latter, you can name the common helper 
> > 'build_spcr_rev2'
> > (since both ARM and RISC-V use SPCR rev 2), keep this local build_spcr() 
> > initializing
> > the AcpiSpcrData struct with ARM attributes and then call the common 
> > build_spcr_rev2().
> >
> My suggestion is, keep the build_spcr() generic and take version as the
> parameter.
Alright. I will give them a generic name for both the arch build_spcr() and
the common build_spcr(). The revision info should be passed to the common
build_spcr().
> 
> Thanks,
> Sunil