RE: [PATCH RESEND v4 0/3] Upgrade ACPI SPCR table to support SPCR table revision 4 format
> -Original Message- > From: Sunil V L > Sent: Monday, August 26, 2024 3:14 PM > To: JeeHeng Sia > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; > anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com; > pal...@dabbelt.com; alistair.fran...@wdc.com; > bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com; > zhiwei_...@linux.alibaba.com > Subject: Re: [PATCH RESEND v4 0/3] Upgrade ACPI SPCR table to support SPCR > table revision 4 format > > Hi Jee Heng, > > On Mon, Aug 26, 2024 at 07:04:11AM +, JeeHeng Sia wrote: > > > > > > > -Original Message- > > > From: Sunil V L > > > Sent: Friday, August 23, 2024 10:29 PM > > > To: JeeHeng Sia > > > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > > > m...@redhat.com; imamm...@redhat.com; > > > anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com; > > > pal...@dabbelt.com; alistair.fran...@wdc.com; > > > bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com; > > > zhiwei_...@linux.alibaba.com > > > Subject: Re: [PATCH RESEND v4 0/3] Upgrade ACPI SPCR table to support > > > SPCR table revision 4 format > > > > > > Hi Jee Heng, > > > On Fri, Aug 23, 2024 at 04:31:39AM -0700, Sia Jee Heng wrote: > > > > Update the SPCR table to accommodate the SPCR Table revision 4 [1]. > > > > The SPCR table has been modified to adhere to the revision 4 format [2]. > > > > > > > > Meanwhile, the virt SPCR golden reference file for RISC-V have been > > > > updated to > > > > accommodate the SPCR Table revision 4. > > > > > > > > [1]: > > > > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table > > > > [2]: https://github.com/acpica/acpica/pull/931 > > > > > > > Just curious - whether this needs changes in linux side as well? Does > > > current linux code work fine with version 4 of SPCR table on RISC-V? > > The current Linux ACPI Table has not yet been updated to support SPCR v4. > > However, Linux RISC-V will be able to use the information in the QEMU's > > SPCR table to boot with behaviour similar to ARM. > > > > So, it means even if qemu risc-v SPCR is upgraded to version 4, > current linux continues to work. It may be just able to decode SPCR v2 > fields only until enhanced to understand v4 but but doesn't break > anything or crash. Is that correct? Yes, correct. > > Just wanted to confirm qemu changes can get merged without any > dependency. > > Thanks! > Sunil
RE: [PATCH RESEND v4 0/3] Upgrade ACPI SPCR table to support SPCR table revision 4 format
> -Original Message- > From: Sunil V L > Sent: Friday, August 23, 2024 10:29 PM > To: JeeHeng Sia > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; > anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com; > pal...@dabbelt.com; alistair.fran...@wdc.com; > bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com; > zhiwei_...@linux.alibaba.com > Subject: Re: [PATCH RESEND v4 0/3] Upgrade ACPI SPCR table to support SPCR > table revision 4 format > > Hi Jee Heng, > On Fri, Aug 23, 2024 at 04:31:39AM -0700, Sia Jee Heng wrote: > > Update the SPCR table to accommodate the SPCR Table revision 4 [1]. > > The SPCR table has been modified to adhere to the revision 4 format [2]. > > > > Meanwhile, the virt SPCR golden reference file for RISC-V have been updated > > to > > accommodate the SPCR Table revision 4. > > > > [1]: > > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table > > [2]: https://github.com/acpica/acpica/pull/931 > > > Just curious - whether this needs changes in linux side as well? Does > current linux code work fine with version 4 of SPCR table on RISC-V? The current Linux ACPI Table has not yet been updated to support SPCR v4. However, Linux RISC-V will be able to use the information in the QEMU's SPCR table to boot with behaviour similar to ARM. > > Thanks, > Sunil
RE: [PATCH RESEND v4 3/3] tests/qtest/bios-tables-test: Update virt SPCR golden reference for RISC-V
> -Original Message- > From: Sunil V L > Sent: Friday, August 23, 2024 9:20 PM > To: JeeHeng Sia > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; > anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com; > pal...@dabbelt.com; alistair.fran...@wdc.com; > bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com; > zhiwei_...@linux.alibaba.com > Subject: Re: [PATCH RESEND v4 3/3] tests/qtest/bios-tables-test: Update virt > SPCR golden reference for RISC-V > > On Fri, Aug 23, 2024 at 04:31:42AM -0700, Sia Jee Heng wrote: > > Update the virt SPCR golden reference file for RISC-V to accommodate the > > SPCR Table revision 4 [1], utilizing the iasl binary compiled from the > > latest ACPICA repository. The SPCR table has been modified to > > adhere to the revision 4 format [2]. > > > > [1]: > > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table > > [2]: https://github.com/acpica/acpica/pull/931 > > > > Diffs from iasl: > > /* > > * Intel ACPI Component Architecture > > * AML/ASL+ Disassembler version 20200925 (64-bit version) > > * Copyright (c) 2000 - 2020 Intel Corporation > > * > > - * Disassembly of tests/data/acpi/riscv64/virt/SPCR, Fri Aug 23 02:07:47 > > 2024 > > + * Disassembly of /tmp/aml-Y8JPS2, Fri Aug 23 02:07:47 2024 > > * > > * ACPI Data Table [SPCR] > > * > > * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue > > */ > > > > [000h 4]Signature : "SPCR"[Serial Port > > Console Redirection table] > > -[004h 0004 4] Table Length : 0050 > > -[008h 0008 1] Revision : 02 > > -[009h 0009 1] Checksum : B9 > > +[004h 0004 4] Table Length : 005A > > +[008h 0008 1] Revision : 04 > > +[009h 0009 1] Checksum : 13 > > [00Ah 0010 6] Oem ID : "BOCHS " > > [010h 0016 8] Oem Table ID : "BXPC" > > [018h 0024 4] Oem Revision : 0001 > > [01Ch 0028 4] Asl Compiler ID : "BXPC" > > [020h 0032 4]Asl Compiler Revision : 0001 > > > > -[024h 0036 1] Interface Type : 00 > > +[024h 0036 1] Interface Type : 12 > > [025h 0037 3] Reserved : 00 > > > > [028h 0040 12] Serial Port Register : [Generic Address Structure] > > [028h 0040 1] Space ID : 00 [SystemMemory] > > [029h 0041 1]Bit Width : 20 > > [02Ah 0042 1] Bit Offset : 00 > > [02Bh 0043 1] Encoded Access Width : 01 [Byte Access:8] > > [02Ch 0044 8] Address : 1000 > > > > [034h 0052 1] Interrupt Type : 10 > > [035h 0053 1] PCAT-compatible IRQ : 00 > > [036h 0054 4]Interrupt : 000A > > [03Ah 0058 1]Baud Rate : 07 > > [03Bh 0059 1] Parity : 00 > > [03Ch 0060 1]Stop Bits : 01 > > [03Dh 0061 1] Flow Control : 00 > > [03Eh 0062 1]Terminal Type : 00 > > [04Ch 0076 1] Reserved : 00 > > [040h 0064 2]PCI Device ID : > > [042h 0066 2]PCI Vendor ID : > > [044h 0068 1] PCI Bus : 00 > > [045h 0069 1] PCI Device : 00 > > [046h 0070 1] PCI Function : 00 > > [047h 0071 4]PCI Flags : > > [04Bh 0075 1] PCI Segment : 00 > > [04Ch 0076 4] Reserved : > > > Shouldn't iasl print additional fields added in version 4? You are right. It should print info for Revision 4. > > Thanks, > Sunil > > -Raw Table Data: Length 80 (0x50) > > +Raw Table Data: Length 90 (0x5A) > > > > -: 53 50 43 52 50 00 00 00 02 B9 42 4F 43 48 53 20 // > > SPCRP.BOCHS > > +: 53 50 43 52 5A 00 00 00 04 13 42 4F 43 48 53 20 // > > SPCRZ.BOCHS > > 0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC > > BXPC > > -0020: 01 00 00 00 00 00 00 00 00 20 00 01 00 00 00 10 // . > > .. > &
RE: [PATCH RESEND v4 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR table revision 4 format
> -Original Message-
> From: Sunil V L
> Sent: Friday, August 23, 2024 9:15 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [PATCH RESEND v4 2/3] hw/acpi: Upgrade ACPI SPCR table to
> support SPCR table revision 4 format
>
> On Fri, Aug 23, 2024 at 04:31:41AM -0700, Sia Jee Heng wrote:
> > Update the SPCR table to accommodate the SPCR Table revision 4 [1].
> > The SPCR table has been modified to adhere to the revision 4 format [2].
> >
> > [1]:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
> > [2]: https://github.com/acpica/acpica/pull/931
> >
> > Signed-off-by: Sia Jee Heng
> > Acked-by: Alistair Francis
> > ---
> > hw/acpi/aml-build.c | 20
> > hw/arm/virt-acpi-build.c| 10 +++---
> > hw/riscv/virt-acpi-build.c | 12 +---
> > include/hw/acpi/acpi-defs.h | 7 +--
> > include/hw/acpi/aml-build.h | 2 +-
> > 5 files changed, 38 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 6d4517cfbe..ad0a306db1 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1996,7 +1996,7 @@ 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)
> > +const char *oem_id, const char *oem_table_id, const char
> > *name)
> > {
> > AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> > .oem_table_id = oem_table_id };
> > @@ -2042,9 +2042,21 @@ void build_spcr(GArray *table_data, BIOSLinker
> > *linker,
> > 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);
> > -
> > +if (rev < 4) {
> > +/* Reserved */
> > +build_append_int_noprefix(table_data, 0, 4);
> > +} else {
> > +/* UartClkFreq */
> > +build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
> > +/* PreciseBaudrate */
> > +build_append_int_noprefix(table_data, f->precise_baudrate, 4);
> > +/* NameSpaceStringLength */
> > +build_append_int_noprefix(table_data, f->namespace_string_length,
> > 2);
> > +/* NameSpaceStringOffset */
> > +build_append_int_noprefix(table_data, f->namespace_string_offset,
> > 2);
> > +/* NamespaceString[] */
> > +g_array_append_vals(table_data, name, f->namespace_string_length);
> > +}
> > acpi_table_end(linker, &table);
> > }
> > /*
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f76fb117ad..cc27ba7daf 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -435,7 +435,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> >
> > /*
> > * Serial Port Console Redirection Table (SPCR)
> > - * Rev: 1.07
> > + * Rev: 1.10
> Why should this comment be updated? Since revision 2 of SPCR table ARM
> uses corresponds to only 1.07 right?
You are right. I will revert this.
>
> > */
> > static void
> > spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > @@ -464,8 +464,12 @@ spcr_setup(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > .pci_flags = 0,
> > .pci_segment = 0,
> > };
> > -
> > -build_spcr(table_data, linker, &serial, 2, vms->oem_id,
> > vms->oem_table_id);
> > +/*
> > + * Passing NULL as the SPCR Table for Revision 2 doesn't support
> > + * NameSpaceString.
> > + */
> > +build_spcr(table_data, linker, &serial, 2, vms->oem_id,
> > vms->oem_table_id,
> > + NULL);
> > }
> >
> > /*
> >
RE: [PATCH v3 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR table version 4 format
> -Original Message-
> From: Sunil V L
> Sent: Monday, August 19, 2024 12:10 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [PATCH v3 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR
> table version 4 format
>
> Hi Jee Heng,
>
> On Mon, Aug 12, 2024 at 10:22:22PM -0700, Sia Jee Heng wrote:
> > Update the SPCR table to accommodate the SPCR Table version 4 [1].
> > The SPCR table has been modified to adhere to the version 4 format [2].
> >
> > [1]:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
> > [2]: https://github.com/acpica/acpica/pull/931
> >
> > Signed-off-by: Sia Jee Heng
> > Acked-by: Alistair Francis
> > ---
> > hw/acpi/aml-build.c | 14 +++---
> > hw/arm/virt-acpi-build.c| 10 --
> > hw/riscv/virt-acpi-build.c | 12 +---
> > include/hw/acpi/acpi-defs.h | 7 +--
> > include/hw/acpi/aml-build.h | 2 +-
> > 5 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 6d4517cfbe..7c43573eef 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1996,7 +1996,7 @@ 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)
> > +const char *oem_id, const char *oem_table_id, const char
> > *name)
> > {
> > AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> > .oem_table_id = oem_table_id };
> > @@ -2042,8 +2042,16 @@ void build_spcr(GArray *table_data, BIOSLinker
> > *linker,
> > 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);
> > +/* UartClkFreq */
> > +build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
> > +/* PreciseBaudrate */
> > +build_append_int_noprefix(table_data, f->precise_baudrate, 4);
> > +/* NameSpaceStringLength */
> > +build_append_int_noprefix(table_data, f->namespace_string_length, 2);
> > +/* NameSpaceStringOffset */
> > +build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
> > +/* NamespaceString[] */
> > +g_array_append_vals(table_data, name, f->namespace_string_length);
> >
> > acpi_table_end(linker, &table);
> > }
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index e10cad86dd..ae075dc9fd 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -435,11 +435,12 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> >
> > /*
> > * Serial Port Console Redirection Table (SPCR)
> > - * Rev: 1.07
> > + * Rev: 1.10
> > */
> > static void
> > spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > {
> > +const char name[] = ".";
> > AcpiSpcrData serial = {
> > .interface_type = 3, /* ARM PL011 UART */
> > .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > @@ -463,9 +464,14 @@ spcr_setup(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > .pci_function = 0,
> > .pci_flags = 0,
> > .pci_segment = 0,
> > +.uart_clk_freq = 0,
> > +.precise_baudrate = 0,
> > +.namespace_string_length = sizeof(name),
> > +.namespace_string_offset = 88,
> > };
> >
> > -build_spcr(table_data, linker, &serial, 2, vms->oem_id,
> > vms->oem_table_id);
> > +build_spcr(table_data, linker, &serial, 4, vms->oem_id,
> > vms->oem_table_id,
> > + name);
> I request the same which I had asked earlier. Please keep SPCR for other
> architectures like ARM intact. The latest revision is primarily required
> for RISC-V. So, restrict the series only for RISC-V. The common
> build_spcr() should create SPCR based on the revision parameter.
Since there is no disagreement from other arch owner, I will proceed to remove
the changes. thanks
>
> Thanks,
> Sunil
RE: [PATCH v2 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR table version 4 format
> -Original Message-
> From: Michael S. Tsirkin
> Sent: Monday, May 13, 2024 2:31 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> imamm...@redhat.com; anisi...@redhat.com;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com; suni...@ventanamicro.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [PATCH v2 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR
> table version 4 format
>
> On Mon, May 06, 2024 at 10:22:11PM -0700, Sia Jee Heng wrote:
> > Update the SPCR table to accommodate the SPCR Table version 4 [1].
> > The SPCR table has been modified to adhere to the version 4 format [2].
> >
> > [1]:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
> > [2]: https://github.com/acpica/acpica/pull/931
> >
> > Signed-off-by: Sia Jee Heng
>
> What does this achieve? We don't normally change unless it gets
> us some useful feature.
The changes in Table 4 primarily include the addition of RISC-V support
and modifications to the Interrupt Type field. Additionally, new fields
added are Precise Baud Rate, NamespaceStringLength, NamespaceStringOffset,
and NamespaceString[].
>
>
> > ---
> > hw/acpi/aml-build.c | 14 +++---
> > hw/arm/virt-acpi-build.c| 10 --
> > hw/riscv/virt-acpi-build.c | 12 +---
> > include/hw/acpi/acpi-defs.h | 7 +--
> > include/hw/acpi/aml-build.h | 2 +-
> > 5 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 6d4517cfbe..7c43573eef 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1996,7 +1996,7 @@ 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)
> > +const char *oem_id, const char *oem_table_id, const char
> > *name)
> > {
> > AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> > .oem_table_id = oem_table_id };
> > @@ -2042,8 +2042,16 @@ void build_spcr(GArray *table_data, BIOSLinker
> > *linker,
> > 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);
> > +/* UartClkFreq */
> > +build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
> > +/* PreciseBaudrate */
> > +build_append_int_noprefix(table_data, f->precise_baudrate, 4);
> > +/* NameSpaceStringLength */
> > +build_append_int_noprefix(table_data, f->namespace_string_length, 2);
> > +/* NameSpaceStringOffset */
> > +build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
> > +/* NamespaceString[] */
> > +g_array_append_vals(table_data, name, f->namespace_string_length);
> >
> > acpi_table_end(linker, &table);
> > }
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 6a1bde61ce..cb345e8659 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -428,11 +428,12 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> >
> > /*
> > * Serial Port Console Redirection Table (SPCR)
> > - * Rev: 1.07
> > + * Rev: 1.10
> > */
> > static void
> > spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > {
> > +const char name[] = ".";
> > AcpiSpcrData serial = {
> > .interface_type = 3, /* ARM PL011 UART */
> > .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > @@ -456,9 +457,14 @@ spcr_setup(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > .pci_function = 0,
> > .pci_flags = 0,
> > .pci_segment = 0,
> > +.uart_clk_freq = 0,
> > +.precise_baudrate = 0,
> > +.namespace_string_length = sizeof(name),
> > +.namespace_string_offset = 88,
> > };
> >
> > -build_spcr(table_data, linker, &serial, 2, vms->oem_id,
> > v
RE: [PATCH v2 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR table version 4 format
> -Original Message-
> From: Sunil V L
> Sent: Monday, May 13, 2024 1:16 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [PATCH v2 2/3] hw/acpi: Upgrade ACPI SPCR table to support SPCR
> table version 4 format
>
> Hi Sia Jee Heng,
>
> On Mon, May 06, 2024 at 10:22:11PM -0700, Sia Jee Heng wrote:
> > Update the SPCR table to accommodate the SPCR Table version 4 [1].
> > The SPCR table has been modified to adhere to the version 4 format [2].
> >
> > [1]:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table
> > [2]: https://github.com/acpica/acpica/pull/931
> >
> > Signed-off-by: Sia Jee Heng
> > ---
> > hw/acpi/aml-build.c | 14 +++---
> > hw/arm/virt-acpi-build.c| 10 --
> > hw/riscv/virt-acpi-build.c | 12 +---
> > include/hw/acpi/acpi-defs.h | 7 +--
> > include/hw/acpi/aml-build.h | 2 +-
> > 5 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 6d4517cfbe..7c43573eef 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1996,7 +1996,7 @@ 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)
> > +const char *oem_id, const char *oem_table_id, const char
> > *name)
> > {
> > AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> > .oem_table_id = oem_table_id };
> > @@ -2042,8 +2042,16 @@ void build_spcr(GArray *table_data, BIOSLinker
> > *linker,
> > 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);
> > +/* UartClkFreq */
> > +build_append_int_noprefix(table_data, f->uart_clk_freq, 4);
> > +/* PreciseBaudrate */
> > +build_append_int_noprefix(table_data, f->precise_baudrate, 4);
> > +/* NameSpaceStringLength */
> > +build_append_int_noprefix(table_data, f->namespace_string_length, 2);
> > +/* NameSpaceStringOffset */
> > +build_append_int_noprefix(table_data, f->namespace_string_offset, 2);
> > +/* NamespaceString[] */
> > +g_array_append_vals(table_data, name, f->namespace_string_length);
> >
> Is it possible to check the revision here and add new fields only if the
> revision supports it? ARM maintainers are better to comment but IMO, we
> better keep ARM's SPCR in the same current version since I don't know
> how consumers like linux (and other OSs) react to the change.
Hi Sunil, there is only one prebuilt SPCR binary. By doing this, the qtest
will fail. I think I will let ARM maintainer to comment.
>
> Thanks!
> Sunil
> > acpi_table_end(linker, &table);
> > }
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 6a1bde61ce..cb345e8659 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -428,11 +428,12 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> >
> > /*
> > * Serial Port Console Redirection Table (SPCR)
> > - * Rev: 1.07
> > + * Rev: 1.10
> > */
> > static void
> > spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > {
> > +const char name[] = ".";
> > AcpiSpcrData serial = {
> > .interface_type = 3, /* ARM PL011 UART */
> > .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > @@ -456,9 +457,14 @@ spcr_setup(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > .pci_function = 0,
> > .pci_flags = 0,
> > .pci_segment = 0,
> > +.uart_clk_freq = 0,
> > +.precise_baudrate = 0,
> > +.namespace_string_length = sizeof(name),
> > +.namespace_string_offset = 88,
RE: [PATCH v1 0/2] Upgrade ACPI SPCR table to support SPCR table version 4 format
> -Original Message- > From: Peter Maydell > Sent: Thursday, May 2, 2024 5:19 PM > To: JeeHeng Sia > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; > anisi...@redhat.com; shannon.zha...@gmail.com; suni...@ventanamicro.com; > pal...@dabbelt.com; alistair.fran...@wdc.com; > bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com; > zhiwei_...@linux.alibaba.com > Subject: Re: [PATCH v1 0/2] Upgrade ACPI SPCR table to support SPCR table > version 4 format > > On Thu, 2 May 2024 at 06:12, Sia Jee Heng > wrote: > > > > Update the SPCR table to accommodate the SPCR Table version 4 [1]. > > The SPCR table has been modified to adhere to the version 4 format [2]. > > > > Meanwhile, the virt SPCR golden reference files have been updated to > > accommodate the SPCR Table version 4. > > > > [1]: > > https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table > > [2]: https://github.com/acpica/acpica/pull/931 > > > > Sia Jee Heng (2): > > tests/qtest/bios-tables-test: Update virt SPCR golden references > > hw/acpi: Upgrade ACPI SPCR table to support SPCR table version 4 > > format > > This isn't the right way to make a change that requires > updates to the bios-tables-test reference files, because > "make check" will fail after patch 1 but before patch 2. > > You need a three-patch approach. How to do that is documented > in the comment at the top of bios-tables-test.c. The resulting > three patches should look like: > * patch 1 updates bios-tables-test-allowed-diff.h to >mark the affected test or tests as "OK to fail" > * patch 2 makes the changes to QEMU that alter the >required table output > * patch 3 updates the reference files and removes the >tests from the allowed-diff file > > See for instance commits 6c1c2e912fcf9, 1ec896fe7ca938, > ea2fde5bccc514 as an example. Thank you for the guidance. I will improve the patch by referring to the example provided. > > Side note: if riscv virt has APCI tables now, maybe we > should add testing of them to the bios-tables-test ? Sure. I will make modifications based on the RISC-V test. > > thanks > -- PMM
RE: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message- > From: Alistair Francis > Sent: Thursday, March 7, 2024 9:33 AM > To: Daniel Henrique Barboza > Cc: JeeHeng Sia ; qemu-...@nongnu.org; > qemu-devel@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com; > peter.mayd...@linaro.org; shannon.zha...@gmail.com; > suni...@ventanamicro.com; pal...@dabbelt.com; alistair.fran...@wdc.com; > bin.m...@windriver.com; liwei1...@gmail.com; > zhiwei_...@linux.alibaba.com > Subject: Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation > to common location > > On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza > wrote: > > > > Hi, > > > > This patch break check-qtest, most specifically 'bios-table'test', for > > aarch64. > > I found this while running riscv-to-apply.next in the Gitlab pipeline. > > > > > > Here's the output: > > > > $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 > > ./tests/qtest/bios-tables-test > > TAP version 13 > > # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > > unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control > -display none -audio none -machine none -accel > qtest > > 1..8 > > # Start of aarch64 tests > > # Start of acpi tests > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > > unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control > -display none -audio none -machine virt -accel tcg > -nodefaults -nographic -drive > if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive > if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom > tests/data/uefi-boot-images/bios-tables- > test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios > type=4,max-speed=2900,current-speed=2700 -accel qtest > > acpi-test: Warning! SPCR binary file mismatch. Actual > > [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR]. > > See source file tests/qtest/bios-tables-test.c for instructions on how to > > update expected files. > > acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, > > aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, > aml:tests/data/acpi/virt/SPCR]. > > > > The diff is here: > > > > --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300 > > +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300 > > @@ -1,57 +1,49 @@ > > /* > >* Intel ACPI Component Architecture > >* AML/ASL+ Disassembler version 20220331 (64-bit version) > >* Copyright (c) 2000 - 2022 Intel Corporation > > > > (...) > > > > [000h 4]Signature : "SPCR"[Serial Port > > Console Redirection Table] > > -[004h 0004 4] Table Length : 0050 > > +[004h 0004 4] Table Length : 004F > > [008h 0008 1] Revision : 02 > > -[009h 0009 1] Checksum : B1 > > +[009h 0009 1] Checksum : B2 > > [00Ah 0010 6] Oem ID : "BOCHS " > > > > (...) > > > > -[042h 0066 2]PCI Vendor ID : > > +[042h 0066 2]PCI Vendor ID : 00FF > > > > > > After inspecting the common helper and what the original ARM code was doing > > I found out that we're missing something down there: > > > > > > On 1/15/24 22:09, 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 > > > --- > > > 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, > > >
RE: [RFC 2/8] hw/core: Move CPU topology enumeration into arch-agnostic file
> -Original Message-
> From: Zhao Liu
> Sent: Tuesday, February 20, 2024 5:25 PM
> To: Daniel P . Berrangé ; Eduardo Habkost
> ; Marcel Apfelbaum
> ; Philippe Mathieu-Daudé ;
> Yanan Wang ;
> Michael S . Tsirkin ; Paolo Bonzini ;
> Richard Henderson ;
> Eric Blake ; Markus Armbruster ;
> Marcelo Tosatti ; Alex Bennée
> ; Peter Maydell ; Jonathan
> Cameron ;
> JeeHeng Sia
> Cc: qemu-devel@nongnu.org; k...@vger.kernel.org; qemu-ri...@nongnu.org;
> qemu-...@nongnu.org; Zhenyu Wang
> ; Dapeng Mi ; Yongwei Ma
> ; Zhao Liu
>
> Subject: [RFC 2/8] hw/core: Move CPU topology enumeration into arch-agnostic
> file
>
> From: Zhao Liu
>
> Cache topology needs to be defined based on CPU topology levels. Thus,
> move CPU topology enumeration into a common header.
>
> To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
> and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
> CPU_TOPO_LEVEL_SOCKET.
>
> Also, enumerate additional topology levels for non-i386 arches, and add
> helpers for topology enumeration and string conversion.
>
> Signed-off-by: Zhao Liu
> ---
> MAINTAINERS| 2 ++
> hw/core/cpu-topology.c | 56 ++
> hw/core/meson.build| 1 +
> include/hw/core/cpu-topology.h | 40
> include/hw/i386/topology.h | 18 +--
> target/i386/cpu.c | 24 +++
> target/i386/cpu.h | 2 +-
> tests/unit/meson.build | 3 +-
> 8 files changed, 115 insertions(+), 31 deletions(-)
> create mode 100644 hw/core/cpu-topology.c
> create mode 100644 include/hw/core/cpu-topology.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7d61fb93194b..4b1cce938915 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1871,6 +1871,7 @@ R: Yanan Wang
> S: Supported
> F: hw/core/cpu-common.c
> F: hw/core/cpu-sysemu.c
> +F: hw/core/cpu-topology.c
> F: hw/core/machine-qmp-cmds.c
> F: hw/core/machine.c
> F: hw/core/machine-smp.c
> @@ -1882,6 +1883,7 @@ F: qapi/machine-common.json
> F: qapi/machine-target.json
> F: include/hw/boards.h
> F: include/hw/core/cpu.h
> +F: include/hw/core/cpu-topology.h
> F: include/hw/cpu/cluster.h
> F: include/sysemu/numa.h
> F: tests/unit/test-smp-parse.c
> diff --git a/hw/core/cpu-topology.c b/hw/core/cpu-topology.c
> new file mode 100644
> index ..ca1361d13c16
> --- /dev/null
> +++ b/hw/core/cpu-topology.c
> @@ -0,0 +1,56 @@
> +/*
> + * QEMU CPU Topology Representation
> + *
> + * Copyright (c) 2024 Intel Corporation
> + * Author: Zhao Liu
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License,
> + * or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/cpu-topology.h"
> +
> +typedef struct CPUTopoInfo {
> +const char *name;
> +} CPUTopoInfo;
> +
> +CPUTopoInfo cpu_topo_descriptors[] = {
> +[CPU_TOPO_LEVEL_INVALID] = { .name = "invalid", },
> +[CPU_TOPO_LEVEL_THREAD] = { .name = "thread", },
> +[CPU_TOPO_LEVEL_CORE]= { .name = "core",},
> +[CPU_TOPO_LEVEL_MODULE] = { .name = "module", },
> +[CPU_TOPO_LEVEL_CLUSTER] = { .name = "cluster", },
> +[CPU_TOPO_LEVEL_DIE] = { .name = "die", },
> +[CPU_TOPO_LEVEL_SOCKET] = { .name = "socket", },
> +[CPU_TOPO_LEVEL_BOOK]= { .name = "book",},
> +[CPU_TOPO_LEVEL_DRAWER] = { .name = "drawer", },
> +[CPU_TOPO_LEVEL_MAX] = { .name = NULL, },
> +};
> +
> +const char *cpu_topo_to_string(CPUTopoLevel topo)
> +{
> +return cpu_topo_descriptors[topo].name;
> +}
> +
> +CPUTopoLevel string_to_cpu_topo(char *str)
Can use const char *str.
> +{
> +for (int i = 0; i < ARRAY_SIZE(cpu_topo_descriptors); i++) {
> +CPUTopoInfo *info = &cpu_topo_descriptors[i];
> +
> +if (!strcmp(info->name, str)) {
Suggest to use strncmp instead.
> +
RE: [RFC 6/8] i386/cpu: Update cache topology with machine's configuration
> -Original Message-
> From: Zhao Liu
> Sent: Tuesday, February 20, 2024 5:25 PM
> To: Daniel P . Berrangé ; Eduardo Habkost
> ; Marcel Apfelbaum
> ; Philippe Mathieu-Daudé ;
> Yanan Wang ;
> Michael S . Tsirkin ; Paolo Bonzini ;
> Richard Henderson ;
> Eric Blake ; Markus Armbruster ;
> Marcelo Tosatti ; Alex Bennée
> ; Peter Maydell ; Jonathan
> Cameron ;
> JeeHeng Sia
> Cc: qemu-devel@nongnu.org; k...@vger.kernel.org; qemu-ri...@nongnu.org;
> qemu-...@nongnu.org; Zhenyu Wang
> ; Dapeng Mi ; Yongwei Ma
> ; Zhao Liu
>
> Subject: [RFC 6/8] i386/cpu: Update cache topology with machine's
> configuration
>
> From: Zhao Liu
>
> User will configure SMP cache topology via -smp.
>
> For this case, update the x86 CPUs' cache topology with user's
> configuration in MachineState.
>
> Signed-off-by: Zhao Liu
> ---
> target/i386/cpu.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d7cb0f1e49b4..4b5c551fe7f0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7582,6 +7582,27 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
>
> #ifndef CONFIG_USER_ONLY
> MachineState *ms = MACHINE(qdev_get_machine());
> +
> +if (ms->smp_cache.l1d != CPU_TOPO_LEVEL_INVALID) {
> +env->cache_info_cpuid4.l1d_cache->share_level = ms->smp_cache.l1d;
> +env->cache_info_amd.l1d_cache->share_level = ms->smp_cache.l1d;
> +}
> +
> +if (ms->smp_cache.l1i != CPU_TOPO_LEVEL_INVALID) {
> +env->cache_info_cpuid4.l1i_cache->share_level = ms->smp_cache.l1i;
> +env->cache_info_amd.l1i_cache->share_level = ms->smp_cache.l1i;
> +}
> +
> +if (ms->smp_cache.l2 != CPU_TOPO_LEVEL_INVALID) {
> +env->cache_info_cpuid4.l2_cache->share_level = ms->smp_cache.l2;
> +env->cache_info_amd.l2_cache->share_level = ms->smp_cache.l2;
> +}
> +
> +if (ms->smp_cache.l3 != CPU_TOPO_LEVEL_INVALID) {
> +env->cache_info_cpuid4.l3_cache->share_level = ms->smp_cache.l3;
> +env->cache_info_amd.l3_cache->share_level = ms->smp_cache.l3;
> +}
> +
I think this block of code can be further optimized. Maybe we can create
a function called updateCacheShareLevel() that takes a cache pointer and
a share level as arguments. This function encapsulates the common
pattern of updating cache share levels for different caches. You can define
it like this:
void updateCacheShareLevel(XxxCacheInfo *cache, int shareLevel) {
if (shareLevel != CPU_TOPO_LEVEL_INVALID) {
cache->share_level = shareLevel;
}
}
> qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>
> if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
> --
> 2.34.1
RE: [RFC 4/8] hw/core: Add cache topology options in -smp
> -Original Message-
> From: Zhao Liu
> Sent: Tuesday, February 20, 2024 5:25 PM
> To: Daniel P . Berrangé ; Eduardo Habkost
> ; Marcel Apfelbaum
> ; Philippe Mathieu-Daudé ;
> Yanan Wang ;
> Michael S . Tsirkin ; Paolo Bonzini ;
> Richard Henderson ;
> Eric Blake ; Markus Armbruster ;
> Marcelo Tosatti ; Alex Bennée
> ; Peter Maydell ; Jonathan
> Cameron ;
> JeeHeng Sia
> Cc: qemu-devel@nongnu.org; k...@vger.kernel.org; qemu-ri...@nongnu.org;
> qemu-...@nongnu.org; Zhenyu Wang
> ; Dapeng Mi ; Yongwei Ma
> ; Zhao Liu
>
> Subject: [RFC 4/8] hw/core: Add cache topology options in -smp
>
> From: Zhao Liu
>
> Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
> -smp to define the cache topology for SMP system.
>
> Signed-off-by: Zhao Liu
> ---
> hw/core/machine-smp.c | 128 ++
> hw/core/machine.c | 4 ++
> qapi/machine.json | 14 -
> system/vl.c | 15 +
> 4 files changed, 160 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 8a8296b0d05b..2cbd19f4aa57 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -61,6 +61,132 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
> return g_string_free(s, false);
> }
>
> +static bool machine_check_topo_support(MachineState *ms,
> + CPUTopoLevel topo)
> +{
> +MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +if (topo == CPU_TOPO_LEVEL_MODULE && !mc->smp_props.modules_supported) {
> +return false;
> +}
> +
> +if (topo == CPU_TOPO_LEVEL_CLUSTER && !mc->smp_props.clusters_supported)
> {
> +return false;
> +}
> +
> +if (topo == CPU_TOPO_LEVEL_DIE && !mc->smp_props.dies_supported) {
> +return false;
> +}
> +
> +if (topo == CPU_TOPO_LEVEL_BOOK && !mc->smp_props.books_supported) {
> +return false;
> +}
> +
> +if (topo == CPU_TOPO_LEVEL_DRAWER && !mc->smp_props.drawers_supported) {
> +return false;
> +}
> +
> +return true;
> +}
> +
> +static int smp_cache_string_to_topology(MachineState *ms,
> +char *topo_str,
> +CPUTopoLevel *topo,
> +Error **errp)
> +{
> +*topo = string_to_cpu_topo(topo_str);
> +
> +if (*topo == CPU_TOPO_LEVEL_MAX || *topo == CPU_TOPO_LEVEL_INVALID) {
> +error_setg(errp, "Invalid cache topology level: %s. The cache "
> + "topology should match the CPU topology level", topo_str);
> +return -1;
> +}
> +
> +if (!machine_check_topo_support(ms, *topo)) {
> +error_setg(errp, "Invalid cache topology level: %s. The topology "
> + "level is not supported by this machine", topo_str);
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +static void machine_parse_smp_cache_config(MachineState *ms,
> + const SMPConfiguration *config,
> + Error **errp)
> +{
> +MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +if (config->l1d_cache) {
> +if (!mc->smp_props.l1_separated_cache_supported) {
> +error_setg(errp, "L1 D-cache topology not "
> + "supported by this machine");
> +return;
> +}
> +
> +if (smp_cache_string_to_topology(ms, config->l1d_cache,
> +&ms->smp_cache.l1d, errp)) {
> +return;
> +}
> +}
> +
> +if (config->l1i_cache) {
> +if (!mc->smp_props.l1_separated_cache_supported) {
> +error_setg(errp, "L1 I-cache topology not "
> + "supported by this machine");
> +return;
> +}
> +
> +if (smp_cache_string_to_topology(ms, config->l1i_cache,
> +&ms->smp_cache.l1i, errp)) {
> +return;
> +}
> +}
> +
> +if (config->l2_cache) {
> +if (!mc->smp_props.l2_unified_cache_supported) {
> +error_setg(errp, "L2 cache topology not "
> + "supported by this machine");
> +return;
> +}
> +
> +if (smp_cache_string_to_topology(ms, config->l2_cache,
> +&m
RE: [RFC v1 3/3] hw/arm/virt-acpi-build.c: Enable CPU cache topology
> -Original Message-
> From: Jonathan Cameron
> Sent: Monday, January 29, 2024 7:08 PM
> To: JeeHeng Sia
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; shannon.zha...@gmail.com; peter.mayd...@linaro.org;
> suni...@ventanamicro.com; pal...@dabbelt.com;
> alistair.fran...@wdc.com; bin.m...@windriver.com; liwei1...@gmail.com;
> dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [RFC v1 3/3] hw/arm/virt-acpi-build.c: Enable CPU cache topology
>
> On Mon, 29 Jan 2024 00:14:23 -0800
> Sia Jee Heng wrote:
>
> > Introduced a 3-layer cache for the ARM virtual machine.
> >
> > Signed-off-by: Sia Jee Heng
>
> There are a bunch of CPU registers that also need updating to reflect the
> described cache.
> https://lore.kernel.org/qemu-devel/20230808115713.2613-3-jonathan.came...@huawei.com/
> It's called HACK for a reason ;)
> But there is some discussion about this issue in the thread.
>
> The l1 etc also needs to reflect the CPU model. This stuff needs to match.
> Wrong information being passed to a VM is probably worse than no information.
>
> Whilst I plan to circle back to the MPAM support (perhaps next month) there
> is a lot more to be done here before we have useful cache descriptions for
> guests.
Thanks for the info. I will spend time to look into.
>
> Jonathan
>
> > ---
> > hw/arm/virt-acpi-build.c | 44 +++-
> > 1 file changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 17aeec7a6f..c57067cd63 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -426,6 +426,48 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > g_array_free(its_idmaps, true);
> > }
> >
> > +static void pptt_setup(GArray *table_data, BIOSLinker *linker,
> > MachineState *ms,
> > + const char *oem_id, const char *oem_table_id)
> > +{
> > +CPUCaches default_cache_info = {
> > +.l1d_cache = &(CPUCacheInfo) {
> > +.type = DATA_CACHE,
> > +.size = 64 * KiB,
> > +.line_size = 64,
> > +.associativity = 4,
> > +.sets = 256,
> > +.attributes = 0x02,
> > +},
> > +.l1i_cache = &(CPUCacheInfo) {
> > +.type = INSTRUCTION_CACHE,
> > +.size = 64 * KiB,
> > +.line_size = 64,
> > +.associativity = 4,
> > +.sets = 256,
> > +.attributes = 0x04,
>
> This is the duplication I commented on in patch 1.
> The bit set there is the one to indicate it's an instruction
> cache and we have type doing that as well.
But this gives a great readability, no?
>
>
> > +},
> > +.l2_cache = &(CPUCacheInfo) {
> > +.type = UNIFIED_CACHE,
> > +.size = 2048 * KiB,
> > +.line_size = 64,
> > +.associativity = 8,
> > +.sets = 4096,
> > +.attributes = 0x0a,
> > +},
> > +.l3_cache = &(CPUCacheInfo) {
> > +.type = UNIFIED_CACHE,
> > +.size = 4096 * KiB,
> > +.line_size = 64,
> > +.associativity = 8,
> > +.sets = 8192,
> > +.attributes = 0x0a,
> > +},
> > +};
> > +
> > +build_pptt(table_data, linker, ms, oem_id, oem_table_id,
> > + &default_cache_info);
> > +}
> > +
> > /*
> > * Serial Port Console Redirection Table (SPCR)
> > * Rev: 1.07
> > @@ -912,7 +954,7 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> >
> > if (!vmc->no_cpu_topology) {
> > acpi_add_table(table_offsets, tables_blob);
> > -build_pptt(tables_blob, tables->linker, ms,
> > +pptt_setup(tables_blob, tables->linker, ms,
> > vms->oem_id, vms->oem_table_id);
> > }
> >
RE: [RFC v1 1/3] hw/acpi/aml-build: Add cache structure table creation for PPTT table
> -Original Message- > From: Jonathan Cameron > Sent: Monday, January 29, 2024 7:03 PM > To: JeeHeng Sia > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; qemu-ri...@nongnu.org; > m...@redhat.com; imamm...@redhat.com; > anisi...@redhat.com; shannon.zha...@gmail.com; peter.mayd...@linaro.org; > suni...@ventanamicro.com; pal...@dabbelt.com; > alistair.fran...@wdc.com; bin.m...@windriver.com; liwei1...@gmail.com; > dbarb...@ventanamicro.com; > zhiwei_...@linux.alibaba.com > Subject: Re: [RFC v1 1/3] hw/acpi/aml-build: Add cache structure table > creation for PPTT table > > On Mon, 29 Jan 2024 00:14:21 -0800 > Sia Jee Heng wrote: > > > Adds cache structure table generation for the Processor Properties > > Topology Table (PPTT) to describe cache hierarchy information for > > ACPI guests. > > > > A 3-level cache topology is employed here, referring to the type 1 cache > > structure according to ACPI spec v6.3. The L1 cache and L2 cache are > > private resources for the core, while the L3 cache is the private > > resource for the cluster. > > > > In the absence of cluster values in the QEMU command, a 2-layer cache is > > expected. The default cache value should be passed in from the > > architecture code. > > > > Examples: > > 3-layer: -smp 4,sockets=1,clusters=2,cores=2,threads=1 > > 2-layer: -smp 4,sockets=1,cores=2,threads=2 > > > > Signed-off-by: Sia Jee Heng > > Hi, > > I'm not keen on the topology assumptions this is making. > If were to use this on our Kunpeng 920 for guests then the description would > be wrong as we only share the l3 tags at the cluster level, the > L3 is die level (NUMA node). So for the physical machine we present > a cluster with no associated caches. For other platforms this would be > even further from the truth. Should you consider a file like kunpeng920.c and then pass the necessary value to the build_pptt() function? > > If we are presenting caches in PPTT (which I do want to see) then > we need additional controls to specify the levels at which the > appropriate caches are found. I understood and I'm wonder if adding default value meet your needs? > > There have been various proposals for how to do that description: > https://lore.kernel.org/qemu-devel/20230808115713.2613-2-jonathan.came...@huawei.com/ > was my brief go at this (and had PPTT cache descriptions). I can spend time to try out your patches, but it will be good for a short command. Btw, it seems you stop for many months, do you plan for a v2 or I will continue by update with your v2? > > Maybe it's acceptable to have some defaults. I would suggest to have some default value. > > A few other review comments inline. > > Give an example of the disassembled PPTT so we can see what is being > built. Need to clear if you are sharing descriptions across multiple > instances of a given cache (which is allowed if no cache IDs). > Looks like you do separate entries which is good because that's needed > in latest definition (but wasn't in 6.3 and people built systems that > didn't do separate entries). Sure, here is the example output with clusters=2,core=2,thread=1 [000h 004h] Signature : "PPTT"[Processor Properties Topology Table] [004h 0004 004h]Table Length : 0208 [008h 0008 001h]Revision : 02 [009h 0009 001h]Checksum : 88 [00Ah 0010 006h] Oem ID : "BOCHS " [010h 0016 008h]Oem Table ID : "BXPC" [018h 0024 004h]Oem Revision : 0001 [01Ch 0028 004h] Asl Compiler ID : "BXPC" [020h 0032 004h] Asl Compiler Revision : 0001 [024h 0036 001h] Subtable Type : 00 [Processor Hierarchy Node] [025h 0037 001h] Length : 14 [026h 0038 002h]Reserved : [028h 0040 004h] Flags (decoded below) : 0001 Physical package : 1 ACPI Processor ID valid : 0 Processor is a thread : 0 Node is a leaf : 0 Identical Implementation : 0 [02Ch 0044 004h] Parent : [030h 0048 004h] ACPI Processor ID : [034h 0052 004h] Private Resource Number : [038h 0056 001h] Subtable Type : 01 [Cache Type] [039h 0057 001h] Length : 18 [03Ah 0058 002h]Reserved : [03Ch 0060 004h] Flags (decoded below) : 007F Size valid : 1 Number of Sets valid : 1 Associativity valid : 1
RE: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: Andrew Jones
> Sent: Monday, January 29, 2024 5:19 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> suni...@ventanamicro.com; pal...@dabbelt.com;
> alistair.fran...@wdc.com; bin.m...@windriver.com; liwei1...@gmail.com;
> dbarb...@ventanamicro.com;
> zhiwei_...@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
> > ---
> > 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 *l
RE: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: Sunil V L
> Sent: Monday, January 29, 2024 1:13 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com;
> zhiwei_...@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
> > ---
> > 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 con
RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: JeeHeng Sia
> Sent: Wednesday, January 10, 2024 4:02 PM
> To: Daniel Henrique Barboza ; qemu-...@nongnu.org;
> qemu-devel@nongnu.org; qemu-
> ri...@nongnu.org
> Cc: m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> suni...@ventanamicro.com; pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com;
> zhiwei_...@linux.alibaba.com
> Subject: RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR
> creation to common location
>
>
>
> > -Original Message-
> > From: Daniel Henrique Barboza
> > Sent: Friday, January 5, 2024 8:19 PM
> > To: JeeHeng Sia ; qemu-...@nongnu.org;
> > qemu-devel@nongnu.org; qemu-ri...@nongnu.org
> > Cc: m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com;
> > peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> > suni...@ventanamicro.com; pal...@dabbelt.com; alistair.fran...@wdc.com;
> > bin.m...@windriver.com; liwei1...@gmail.com;
> > zhiwei_...@linux.alibaba.com
> > Subject: Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR
> > creation to common location
> >
> >
> >
> > 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
> > > ---
> > > 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 */
> >
RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: Sunil V L
> Sent: Monday, January 8, 2024 7:35 PM
> To: Daniel Henrique Barboza
> Cc: JeeHeng Sia ; qemu-...@nongnu.org;
> qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com; bin.m...@windriver.com;
> liwei1...@gmail.com; zhiwei_...@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
> > > ---
> > > 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
> &
RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: Daniel Henrique Barboza
> Sent: Friday, January 5, 2024 8:19 PM
> To: JeeHeng Sia ; qemu-...@nongnu.org;
> qemu-devel@nongnu.org; qemu-ri...@nongnu.org
> Cc: m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> suni...@ventanamicro.com; pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR
> creation to common location
>
>
>
> 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
> > ---
> > 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)
>
RE: [RESEND RFC v1 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table
> -Original Message-
> From: Daniel Henrique Barboza
> Sent: Friday, January 5, 2024 8:27 PM
> To: JeeHeng Sia ; qemu-...@nongnu.org;
> qemu-devel@nongnu.org; qemu-ri...@nongnu.org
> Cc: m...@redhat.com; imamm...@redhat.com; anisi...@redhat.com;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> suni...@ventanamicro.com; pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [RESEND RFC v1 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR
> table
>
>
>
> On 1/5/24 06:06, Sia Jee Heng wrote:
> > Generate Serial Port Console Redirection Table (SPCR) for RISC-V
> > virtual machine.
> >
> > Signed-off-by: Sia Jee Heng
> > ---
> > hw/riscv/virt-acpi-build.c | 39 ++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> > index d4a02579d6..388b3d1a84 100644
> > --- a/hw/riscv/virt-acpi-build.c
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -174,6 +174,42 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry
> > *uart_memmap,
> > aml_append(scope, dev);
> > }
> >
> > +/*
> > + * Serial Port Console Redirection Table (SPCR)
> > + * Rev: 1.07
>
> Shouldn't it be "Rev: 2.0"? The function is calling the common build_spcr()
> that
> specifies
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().
>
> +AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> +.oem_table_id = oem_table_id };
>
>
>
> Code LGTM regardless of the "Rev: " comment value.
>
>
> Reviewed-by: Daniel Henrique Barboza
>
>
>
>
>
> > + */
> > +
> > +static void
> > +build_spcr_rev2(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
> > +{
> > +AcpiSpcrData serial = {
> > +.interface_type = 0, /* 16550 compatible */
> > +.base_addr.id = AML_AS_SYSTEM_MEMORY,
> > +.base_addr.width = 32,
> > +.base_addr.offset = 0,
> > +.base_addr.size = 1,
> > +.base_addr.addr = s->memmap[VIRT_UART0].base,
> > +.interrupt_type = (1 << 4),/* Bit[4] RISC-V PLIC/APLIC */
> > +.pc_interrupt = 0,
> > +.interrupt = UART0_IRQ,
> > +.baud_rate = 7,/* 15200 */
> > +.parity = 0,
> > +.stop_bits = 1,
> > +.flow_control = 0,
> > +.terminal_type = 3,/* ANSI */
> > +.language = 0, /* Language */
> > +.pci_device_id = 0x, /* not a PCI device*/
> > +.pci_vendor_id = 0x, /* not a PCI device*/
> > +.pci_bus = 0,
> > +.pci_device = 0,
> > +.pci_function = 0,
> > +.pci_flags = 0,
> > +.pci_segment = 0,
> > +};
> > +
> > +build_spcr(table_data, linker, &serial, s->oem_id, s->oem_table_id);
> > +}
> > +
> > /* RHCT Node[N] starts at offset 56 */
> > #define RHCT_NODE_ARRAY_OFFSET 56
> >
> > @@ -555,6 +591,9 @@ static void virt_acpi_build(RISCVVirtState *s,
> > AcpiBuildTables *tables)
> > acpi_add_table(table_offsets, tables_blob);
> > build_rhct(tables_blob, tables->linker, s);
> >
> > +acpi_add_table(table_offsets, tables_blob);
> > +build_spcr_rev2(tables_blob, tables->linker, s);
> > +
> > acpi_add_table(table_offsets, tables_blob);
> > {
> > AcpiMcfgInfo mcfg = {
