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

[JeeHeng Sia]

> -----Original Message-----
> From: Andrew Jones <ajones@ventanamicro.com>
> Sent: Monday, January 29, 2024 5:19 PM
> To: JeeHeng Sia <jeeheng.sia@starfivetech.com>
> Cc: 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; 
> sunilvl@ventanamicro.com; palmer@dabbelt.com;
> alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com; 
> dbarboza@ventanamicro.com;
> zhiwei_liu@linux.alibaba.com
> Subject: Re: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation 
> to common location
> 
> On Sun, Jan 28, 2024 at 06:14:39PM -0800, 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..f3904650e4 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 uint8_t rev,
> > +                const char *oem_id, const char *oem_table_id)
> > +{
> > +    AcpiTable table = { .sig = "SPCR", .rev = rev, .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 a22a2f43a5..195767c0f0 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)
> > +spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >  {
> > -    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
> > -                        .oem_table_id = vms->oem_table_id };
> > -
> > -    acpi_table_begin(&table, table_data);
> > -
> > -    /* Interface Type */
> > -    build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
> > -    build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> > -    /* Base Address */
> > -    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 3,
> > -                     vms->memmap[VIRT_UART].base);
> > -    /* Interrupt Type */
> > -    build_append_int_noprefix(table_data,
> > -        (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
> > -    build_append_int_noprefix(table_data, 0, 1); /* IRQ */
> > -    /* Global System Interrupt */
> > -    build_append_int_noprefix(table_data,
> > -                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
> > -    build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
> > -    build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity 
> > */
> > -    /* Stop Bits */
> > -    build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
> > -    /* Flow Control */
> > -    build_append_int_noprefix(table_data,
> > -        (1 << 1) /* RTS/CTS hardware flow control */, 1);
> > -    /* Terminal Type */
> > -    build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
> > -    build_append_int_noprefix(table_data, 0, 1); /* Language */
> > -    /* PCI Device ID  */
> > -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> > -    /* PCI Vendor ID */
> > -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */
> > -    build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */
> > -    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> > +    AcpiSpcrData serial = {
> > +        .interface_type = 3,       /* ARM PL011 UART */
> > +        .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > +        .base_addr.width = 32,
> > +        .base_addr.offset = 0,
> > +        .base_addr.size = 3,
> > +        .base_addr.addr = vms->memmap[VIRT_UART].base,
> > +        .interrupt_type = (1 << 3),/* Bit[3] ARMH GIC interrupt*/
> > +        .pc_interrupt = 0,         /* IRQ */
> > +        .interrupt = (vms->irqmap[VIRT_UART] + ARM_SPI_BASE),
> > +        .baud_rate = 3,            /* 9600 */
> > +        .parity = 0,               /* No Parity */
> > +        .stop_bits = 1,            /* 1 Stop bit */
> > +        .flow_control = 1 << 1,    /* RTS/CTS hardware flow control */
> > +        .terminal_type = 0,        /* VT100 */
> > +        .language = 0,             /* Language */
> > +        .pci_device_id = 0xffff,   /* not a PCI device*/
> > +        .pci_vendor_id = 0xffff,   /* not a PCI device*/
> > +        .pci_bus = 0,
> > +        .pci_device = 0,
> > +        .pci_function = 0,
> > +        .pci_flags = 0,
> > +        .pci_segment = 0,
> 
> Sharing code is good, but if we have to parametrize the entire table, then
> we might as well keep Arm and RISCV separate. Building the table first
> with this struct, just to have it built again with the build_append API,
> doesn't make much sense to me. Do Arm and riscv really diverge on all
> these parameters? If not, then just add the parameters which do diverge
> build_scpr's arguments.
It is kind of chicken and egg thing, I would suggest let the arch code to
fill in the value. It doesn't make sense to change again in the future when
both riscv and arm realized the parameters were different.
Can arm confirm that these values wouldn't change in the future?
> 
> Thanks,
> drew
> 
> 
> > +    };
> >
> > -    acpi_table_end(linker, &table);
> > +    build_spcr(table_data, linker, &serial, 2, vms->oem_id, 
> > vms->oem_table_id);
> >  }
> >
> >  /*
> > @@ -930,7 +916,7 @@ void virt_acpi_build(VirtMachineState *vms, 
> > AcpiBuildTables *tables)
> >      }
> >
> >      acpi_add_table(table_offsets, tables_blob);
> > -    build_spcr(tables_blob, tables->linker, vms);
> > +    spcr_setup(tables_blob, tables->linker, vms);
> >
> >      acpi_add_table(table_offsets, tables_blob);
> >      build_dbg2(tables_blob, tables->linker, vms);
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 2b42e4192b..0e6e82b339 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -90,6 +90,39 @@ typedef struct AcpiFadtData {
> >      unsigned *xdsdt_tbl_offset;
> >  } AcpiFadtData;
> >
> > +typedef struct AcpiGas {
> > +    uint8_t id;                /* Address space ID */
> > +    uint8_t width;             /* Register bit width */
> > +    uint8_t offset;            /* Register bit offset */
> > +    uint8_t size;              /* Access size */
> > +    uint64_t addr;             /* Address */
> > +} AcpiGas;
> > +
> > +/* SPCR (Serial Port Console Redirection table) */
> > +typedef struct AcpiSpcrData {
> > +    uint8_t interface_type;
> > +    uint8_t reserved[3];
> > +    struct AcpiGas base_addr;
> > +    uint8_t interrupt_type;
> > +    uint8_t pc_interrupt;
> > +    uint32_t interrupt;        /* Global system interrupt */
> > +    uint8_t baud_rate;
> > +    uint8_t parity;
> > +    uint8_t stop_bits;
> > +    uint8_t flow_control;
> > +    uint8_t terminal_type;
> > +    uint8_t language;
> > +    uint8_t reserved1;
> > +    uint16_t pci_device_id;    /* Must be 0xffff if not PCI device */
> > +    uint16_t pci_vendor_id;    /* Must be 0xffff if not PCI device */
> > +    uint8_t pci_bus;
> > +    uint8_t pci_device;
> > +    uint8_t pci_function;
> > +    uint32_t pci_flags;
> > +    uint8_t pci_segment;
> > +    uint32_t reserved2;
> > +} AcpiSpcrData;
> > +
> >  #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
> >  #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
> >
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index ff2a310270..a3784155cb 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -497,4 +497,8 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const 
> > AcpiFadtData *f,
> >
> >  void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
> >                  const char *oem_id, const char *oem_table_id);
> > +
> > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > +                const AcpiSpcrData *f, const uint8_t rev,
> > +                const char *oem_id, const char *oem_table_id);
> >  #endif
> > --
> > 2.34.1
> >
> >