Re: [PATCH v3] acpi: Permit OEM ID and OEM table ID fields to be changed
On Mon, Jan 11, 2021 at 04:59:54PM +0200, Marian Posteuca wrote:
> Igor Mammedov writes:
>
> > overall looks good.
> > Please add a test case for it, see
> > tests/qtest/bios-tables-test.c for description how to do it
> > an/or at
> > "[PATCH v3 08/12] tests/acpi: allow updates for expected data files"
> > and follow up patches on the list.
> When you say add a test case, do you mean only updating the binary
> files in tests/data/acpi/{microvm,pc,q35,virt} according to the steps
> at the start of the file bios-tables-test.c? Or do you also mean an actual
> test case to be added in bios-tables-test.c?
>
> Also the step 6 described in bios-tables-test.c mentions that the diff of
> the ACPI table must be added to the commit log, but my change touches
> all the tables for all architectures so that would mean that I would
> have to create a huge commit log. How should I approach this?
If the changes are the same, you can just write:
the change is the same across all architectures,
and show it.
Something I just tripped over: make sure not to
include "---" lines in the diff. Otherwise git am
can not apply the resulting patch.
--
MST
Re: [PATCH v3] acpi: Permit OEM ID and OEM table ID fields to be changed
On Mon, 11 Jan 2021 16:59:54 +0200
Marian Posteuca wrote:
> Igor Mammedov writes:
>
> > overall looks good.
> > Please add a test case for it, see
> > tests/qtest/bios-tables-test.c for description how to do it
> > an/or at
> > "[PATCH v3 08/12] tests/acpi: allow updates for expected data files"
> > and follow up patches on the list.
> When you say add a test case, do you mean only updating the binary
> files in tests/data/acpi/{microvm,pc,q35,virt} according to the steps
> at the start of the file bios-tables-test.c? Or do you also mean an actual
> test case to be added in bios-tables-test.c?
an new test in bios-tables-test.c, which will test that new option works as
expected.
> Also the step 6 described in bios-tables-test.c mentions that the diff of
> the ACPI table must be added to the commit log, but my change touches
> all the tables for all architectures so that would mean that I would
> have to create a huge commit log. How should I approach this?
I don't think that large commit message is problem, it helps reviewer
to see expected changes (if|before one actually tests)
Re: [PATCH v3] acpi: Permit OEM ID and OEM table ID fields to be changed
Igor Mammedov writes:
> overall looks good.
> Please add a test case for it, see
> tests/qtest/bios-tables-test.c for description how to do it
> an/or at
> "[PATCH v3 08/12] tests/acpi: allow updates for expected data files"
> and follow up patches on the list.
When you say add a test case, do you mean only updating the binary
files in tests/data/acpi/{microvm,pc,q35,virt} according to the steps
at the start of the file bios-tables-test.c? Or do you also mean an actual
test case to be added in bios-tables-test.c?
Also the step 6 described in bios-tables-test.c mentions that the diff of
the ACPI table must be added to the commit log, but my change touches
all the tables for all architectures so that would mean that I would
have to create a huge commit log. How should I approach this?
Re: [PATCH v3] acpi: Permit OEM ID and OEM table ID fields to be changed
On Thu, 31 Dec 2020 00:13:01 +0200
Marian Posteuca wrote:
> Qemu's ACPI table generation sets the fields OEM ID and OEM table ID
> to "BOCHS " and "BXPC" where "" is replaced by the ACPI
> table name.
>
> Some games like Red Dead Redemption 2 seem to check the ACPI OEM ID
> and OEM table ID for the strings "BOCHS" and "BXPC" and if they are
> found, the game crashes(this may be an intentional detection
> mechanism to prevent playing the game in a virtualized environment).
>
> This patch allows you to override these default values.
>
> The feature can be used in this manner:
> qemu -machine oem-id=ABCDEF,oem-table-id=GHIJKLMN
>
> The oem-id string can be up to 6 bytes in size, and the
> oem-table-id string can be up to 8 bytes in size. If the string are
> smaller than their respective sizes they will be padded with space.
> If either of these parameters is not set, the current default values
> will be used for the one missing.
>
> Note that the the OEM Table ID field will not be extended with the
> name of the table, but will use either the default name or the user
> provided one.
>
> This does not affect the -acpitable option (for user-defined ACPI
> tables), which has precedence over -machine option.
overall looks good.
Please add a test case for it, see
tests/qtest/bios-tables-test.c for description how to do it
an/or at
"[PATCH v3 08/12] tests/acpi: allow updates for expected data files"
and follow up patches on the list.
Other than that, only my nitpicking below remains.
>
> Signed-off-by: Marian Posteuca
PS:
here under ---
should be changelog if it's not v1
> ---
> hw/acpi/hmat.h | 3 +-
> hw/i386/acpi-common.h | 3 +-
> include/hw/acpi/acpi-defs.h | 2 +-
> include/hw/acpi/aml-build.h | 8 ++--
> include/hw/acpi/ghes.h | 3 +-
> include/hw/acpi/pci.h | 3 +-
> include/hw/acpi/vmgenid.h | 2 +-
> include/hw/arm/virt.h | 2 +
> include/hw/i386/microvm.h | 4 ++
> include/hw/i386/pc.h| 5 ++-
> include/hw/mem/nvdimm.h | 3 +-
> hw/acpi/aml-build.c | 43 +++
> hw/acpi/ghes.c | 5 ++-
> hw/acpi/hmat.c | 5 ++-
> hw/acpi/nvdimm.c| 18 +---
> hw/acpi/pci.c | 5 ++-
> hw/acpi/vmgenid.c | 4 +-
> hw/arm/virt-acpi-build.c| 40 +++--
> hw/arm/virt.c | 57
> hw/i386/acpi-build.c| 86 +
> hw/i386/acpi-common.c | 5 ++-
> hw/i386/acpi-microvm.c | 13 +++---
> hw/i386/microvm.c | 60 ++
> hw/i386/pc.c| 58 +
> 24 files changed, 344 insertions(+), 93 deletions(-)
>
> diff --git a/hw/acpi/hmat.h b/hw/acpi/hmat.h
> index e9031cac01..b57f0e7e80 100644
> --- a/hw/acpi/hmat.h
> +++ b/hw/acpi/hmat.h
> @@ -37,6 +37,7 @@
> */
> #define HMAT_PROXIMITY_INITIATOR_VALID 0x1
>
> -void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState
> *numa_state);
> +void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState
> *numa_state,
> +const char *oem_id, const char *oem_table_id);
>
> #endif
> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> index c30e461f18..b12cd73ea5 100644
> --- a/hw/i386/acpi-common.h
> +++ b/hw/i386/acpi-common.h
> @@ -9,6 +9,7 @@
> #define ACPI_BUILD_IOAPIC_ID 0x0
>
> void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> - X86MachineState *x86ms, AcpiDeviceIf *adev);
> + X86MachineState *x86ms, AcpiDeviceIf *adev,
> + const char *oem_id, const char *oem_table_id);
>
> #endif
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 38a42f409a..cf9f44299c 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -41,7 +41,7 @@ enum {
> };
>
> typedef struct AcpiRsdpData {
> -uint8_t oem_id[6] QEMU_NONSTRING; /* OEM identification */
> +char *oem_id; /* OEM identification */
> uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */
>
> unsigned *rsdt_tbl_offset;
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index e727bea1bc..e22983bba1 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -8,7 +8,7 @@
> #define ACPI_BUILD_TABLE_MAX_SIZE 0x20
>
> #define ACPI_BUILD_APPNAME6 "BOCHS "
> -#define ACPI_BUILD_APPNAME4 "BXPC"
> +#define ACPI_BUILD_APPNAME8 "BXPC"
>
> #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
> #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
> @@ -457,10 +457,12 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet
> *range_set);
> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> uint64_t len, int node, MemoryAffinityFlags flags);
>
> -void build_slit(GArray *table_data,
[PATCH v3] acpi: Permit OEM ID and OEM table ID fields to be changed
Qemu's ACPI table generation sets the fields OEM ID and OEM table ID
to "BOCHS " and "BXPC" where "" is replaced by the ACPI
table name.
Some games like Red Dead Redemption 2 seem to check the ACPI OEM ID
and OEM table ID for the strings "BOCHS" and "BXPC" and if they are
found, the game crashes(this may be an intentional detection
mechanism to prevent playing the game in a virtualized environment).
This patch allows you to override these default values.
The feature can be used in this manner:
qemu -machine oem-id=ABCDEF,oem-table-id=GHIJKLMN
The oem-id string can be up to 6 bytes in size, and the
oem-table-id string can be up to 8 bytes in size. If the string are
smaller than their respective sizes they will be padded with space.
If either of these parameters is not set, the current default values
will be used for the one missing.
Note that the the OEM Table ID field will not be extended with the
name of the table, but will use either the default name or the user
provided one.
This does not affect the -acpitable option (for user-defined ACPI
tables), which has precedence over -machine option.
Signed-off-by: Marian Posteuca
---
hw/acpi/hmat.h | 3 +-
hw/i386/acpi-common.h | 3 +-
include/hw/acpi/acpi-defs.h | 2 +-
include/hw/acpi/aml-build.h | 8 ++--
include/hw/acpi/ghes.h | 3 +-
include/hw/acpi/pci.h | 3 +-
include/hw/acpi/vmgenid.h | 2 +-
include/hw/arm/virt.h | 2 +
include/hw/i386/microvm.h | 4 ++
include/hw/i386/pc.h| 5 ++-
include/hw/mem/nvdimm.h | 3 +-
hw/acpi/aml-build.c | 43 +++
hw/acpi/ghes.c | 5 ++-
hw/acpi/hmat.c | 5 ++-
hw/acpi/nvdimm.c| 18 +---
hw/acpi/pci.c | 5 ++-
hw/acpi/vmgenid.c | 4 +-
hw/arm/virt-acpi-build.c| 40 +++--
hw/arm/virt.c | 57
hw/i386/acpi-build.c| 86 +
hw/i386/acpi-common.c | 5 ++-
hw/i386/acpi-microvm.c | 13 +++---
hw/i386/microvm.c | 60 ++
hw/i386/pc.c| 58 +
24 files changed, 344 insertions(+), 93 deletions(-)
diff --git a/hw/acpi/hmat.h b/hw/acpi/hmat.h
index e9031cac01..b57f0e7e80 100644
--- a/hw/acpi/hmat.h
+++ b/hw/acpi/hmat.h
@@ -37,6 +37,7 @@
*/
#define HMAT_PROXIMITY_INITIATOR_VALID 0x1
-void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *numa_state);
+void build_hmat(GArray *table_data, BIOSLinker *linker, NumaState *numa_state,
+const char *oem_id, const char *oem_table_id);
#endif
diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
index c30e461f18..b12cd73ea5 100644
--- a/hw/i386/acpi-common.h
+++ b/hw/i386/acpi-common.h
@@ -9,6 +9,7 @@
#define ACPI_BUILD_IOAPIC_ID 0x0
void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
- X86MachineState *x86ms, AcpiDeviceIf *adev);
+ X86MachineState *x86ms, AcpiDeviceIf *adev,
+ const char *oem_id, const char *oem_table_id);
#endif
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 38a42f409a..cf9f44299c 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -41,7 +41,7 @@ enum {
};
typedef struct AcpiRsdpData {
-uint8_t oem_id[6] QEMU_NONSTRING; /* OEM identification */
+char *oem_id; /* OEM identification */
uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */
unsigned *rsdt_tbl_offset;
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index e727bea1bc..e22983bba1 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -8,7 +8,7 @@
#define ACPI_BUILD_TABLE_MAX_SIZE 0x20
#define ACPI_BUILD_APPNAME6 "BOCHS "
-#define ACPI_BUILD_APPNAME4 "BXPC"
+#define ACPI_BUILD_APPNAME8 "BXPC"
#define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
#define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
@@ -457,10 +457,12 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet
*range_set);
void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
uint64_t len, int node, MemoryAffinityFlags flags);
-void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
+void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
+const char *oem_id, const char *oem_table_id);
void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
const char *oem_id, const char *oem_table_id);
-void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog);
+void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
+const char *oem_id, const char *oem_table_id);
#endif
diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
index 4ad025e09a..2ae8bc1ded 100644
