RE: [RESEND v2 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 29, 2024 1:13 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; 
> 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
> 
> Hi Jee Heng,
> 
> 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);
> > +
> I think either there should be a comment that this supports only v2 of
> SPCR spec or it should be able to create SPCR of any version. IMO, I
> think it is better to add support till v4 (latest). Since consumers like
> Linux probably doesn't support v4 yet, ARM/RISC-V can continue to create
> v2 itself for the time being but the generic build_spcr() should be able
> to create v4 also if the arch requires it.
A v4 table depends on the updated acpica. I am not aware if there is a
request from ARM to update to v4. Anyway, RISC-V BRS Spec did mentioned
on poll-based sbi console. I can check with acpica community if updating
table to v4 is the go otherwise I would suggest we cont stick to v2 because
there is no compatible ACPI guest to test the code.
> 
> Thanks,
> Sunil
> > +    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,
> > +    };
> >
> > -    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
> >