[PATCH 0/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[PATCH 0/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

The flash device is exclusively for the host-controlled firmware, so
we should not expose it to the OS. Exposing it risks the OS messing
with it, which could break firmware runtime services and surprise the
OS when all its changes disappear after reboot.

This change was suggested by Ard and Laszlo.

Patch 3/4 is the meat. The other patches deal with updating qtest.

Thanks,
drew

Andrew Jones (4):
  tests/acpi: remove stale allowed tables
  tests/acpi: virt: allow DSDT acpi table changes
  hw/arm/virt-acpi-build: Only expose flash on older machine types
  tests/acpi: virt: update golden masters for DSDT

 hw/arm/virt-acpi-build.c                    |   5 ++++-
 hw/arm/virt.c                               |   3 +++
 include/hw/arm/virt.h                       |   1 +
 tests/data/acpi/virt/DSDT                   | Bin 5307 -> 5205 bytes
 tests/data/acpi/virt/DSDT.memhp             | Bin 6668 -> 6566 bytes
 tests/data/acpi/virt/DSDT.numamem           | Bin 5307 -> 5205 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |  18 ------------------
 7 files changed, 8 insertions(+), 19 deletions(-)

-- 
2.25.4

[PATCH 1/4] tests/acpi: remove stale allowed tables

[Andrew Jones]

Fixes: 93dd625f8bf7 ("tests/acpi: update expected data files")
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 8992f1f12b77..dfb8523c8bf4 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,19 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/pc/DSDT",
-"tests/data/acpi/pc/DSDT.acpihmat",
-"tests/data/acpi/pc/DSDT.bridge",
-"tests/data/acpi/pc/DSDT.cphp",
-"tests/data/acpi/pc/DSDT.dimmpxm",
-"tests/data/acpi/pc/DSDT.ipmikcs",
-"tests/data/acpi/pc/DSDT.memhp",
-"tests/data/acpi/pc/DSDT.numamem",
-"tests/data/acpi/q35/DSDT",
-"tests/data/acpi/q35/DSDT.acpihmat",
-"tests/data/acpi/q35/DSDT.bridge",
-"tests/data/acpi/q35/DSDT.cphp",
-"tests/data/acpi/q35/DSDT.dimmpxm",
-"tests/data/acpi/q35/DSDT.ipmibt",
-"tests/data/acpi/q35/DSDT.memhp",
-"tests/data/acpi/q35/DSDT.mmio64",
-"tests/data/acpi/q35/DSDT.numamem",
-"tests/data/acpi/q35/DSDT.tis",
-- 
2.25.4

[PATCH 2/4] tests/acpi: virt: allow DSDT acpi table changes

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf4..32a401ae35fa 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,4 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/DSDT",
+"tests/data/acpi/virt/DSDT.memhp",
+"tests/data/acpi/virt/DSDT.numamem",
-- 
2.25.4

[PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

The flash device is exclusively for the host-controlled firmware, so
we should not expose it to the OS. Exposing it risks the OS messing
with it, which could break firmware runtime services and surprise the
OS when all its changes disappear after reboot.

As firmware needs the device and uses DT, we leave the device exposed
there. It's up to firmware to remove the nodes from DT before sending
it on to the OS. However, there's no need to force firmware to remove
tables from ACPI (which it doesn't know how to do anyway), so we
simply don't add the tables in the first place. But, as we've been
adding the tables for quite some time and don't want to change the
default hardware exposed to versioned machines, then we only stop
exposing the flash device tables for 5.1 and later machine types.

Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt-acpi-build.c | 5 ++++-
 hw/arm/virt.c            | 3 +++
 include/hw/arm/virt.h    | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1384a2cf2ab4..91f0df7b13a3 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     Aml *scope, *dsdt;
     MachineState *ms = MACHINE(vms);
     const MemMapEntry *memmap = vms->memmap;
@@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     acpi_dsdt_add_cpus(scope, vms->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
-    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
+    if (vmc->acpi_expose_flash) {
+        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
+    }
     acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index cd0834ce7faf..5adc9ff799ef 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
 
 static void virt_machine_5_0_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_5_1_options(mc);
     compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
     mc->numa_mem_supported = true;
+    vmc->acpi_expose_flash = true;
 }
 DEFINE_VIRT_MACHINE(5, 0)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 31878ddc7223..c65be5fe0bb6 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -119,6 +119,7 @@ typedef struct {
     bool no_highmem_ecam;
     bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
     bool kvm_no_adjvtime;
+    bool acpi_expose_flash;
 } VirtMachineClass;
 
 typedef struct {
-- 
2.25.4

[PATCH 4/4] tests/acpi: virt: update golden masters for DSDT

[Andrew Jones]

Differences between disassembled ASL files for DSDT:

@@ -5,13 +5,13 @@
  *
  * Disassembling to symbolic ASL+ operators
  *
- * Disassembly of a, Mon Jun 29 09:50:01 2020
+ * Disassembly of b, Mon Jun 29 09:50:03 2020
  *
  * Original Table Header:
  *     Signature        "DSDT"
- *     Length           0x000014BB (5307)
+ *     Length           0x00001455 (5205)
  *     Revision         0x02
- *     Checksum         0xD1
+ *     Checksum         0xE1
  *     OEM ID           "BOCHS "
  *     OEM Table ID     "BXPCDSDT"
  *     OEM Revision     0x00000001 (1)
@@ -45,32 +45,6 @@
             })
         }

-        Device (FLS0)
-        {
-            Name (_HID, "LNRO0015")  // _HID: Hardware ID
-            Name (_UID, Zero)  // _UID: Unique ID
-            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
-            {
-                Memory32Fixed (ReadWrite,
-                    0x00000000,         // Address Base
-                    0x04000000,         // Address Length
-                    )
-            })
-        }
-
-        Device (FLS1)
-        {
-            Name (_HID, "LNRO0015")  // _HID: Hardware ID
-            Name (_UID, One)  // _UID: Unique ID
-            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
-            {
-                Memory32Fixed (ReadWrite,
-                    0x04000000,         // Address Base
-                    0x04000000,         // Address Length
-                    )
-            })
-        }
-
         Device (FWCF)
         {
             Name (_HID, "QEMU0002")  // _HID: Hardware ID

The other two binaries have the same changes (the removal of the
flash devices).

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tests/data/acpi/virt/DSDT                   | Bin 5307 -> 5205 bytes
 tests/data/acpi/virt/DSDT.memhp             | Bin 6668 -> 6566 bytes
 tests/data/acpi/virt/DSDT.numamem           | Bin 5307 -> 5205 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   3 ---
 4 files changed, 3 deletions(-)

diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index d6f5c617881c4247f55d4dcd06581f9693916b2f..e669508d175f1e3ddf355f8a9b0d419266cac8aa 100644
GIT binary patch
delta 28
kcmdn3c~yhUCD<h-RD^+n>ET2!X{H9}iRuX(-<}f&0DgxFc>n+a

delta 156
zcmcbrv0IbNCD<iow+I6R)5VEg(oAih6V(&y4c&Z#4LIUGJY9Hw{DS-q3=B;fIO0P+
zU4W!>P_UpN7hfAE10w?juv9WcH-WSmV$;Hiu7w4t3#`S$E!^1+q9xGPH`KtuzzAr5
LaERl^1zUvy_;n(J

diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index 730e95a46d2cce0af011ffc051d7342beb8f1328..4cb81f692d73526542493a0c4da9c9793cc8366e 100644
GIT binary patch
delta 28
kcmeA%S!T@T66_MPOp<|tiD@F2G*jb@iRuX(-^xn@0CHUjRR910

delta 156
zcmZ2x++)J!66_MfBgMeL^l>7WG*kP$iRuaUhHgH=1|0Doo-VvTenI{Q28N~#9Py!^
zE<n;bC|FRCi?5B7fsp|MSSlH!n?PC&v1wsM*TMqS1=eEW7Vhi@(GuwD8){%+U<5Qj
LIK*+|0yaqism~!^

diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
index d6f5c617881c4247f55d4dcd06581f9693916b2f..e669508d175f1e3ddf355f8a9b0d419266cac8aa 100644
GIT binary patch
delta 28
kcmdn3c~yhUCD<h-RD^+n>ET2!X{H9}iRuX(-<}f&0DgxFc>n+a

delta 156
zcmcbrv0IbNCD<iow+I6R)5VEg(oAih6V(&y4c&Z#4LIUGJY9Hw{DS-q3=B;fIO0P+
zU4W!>P_UpN7hfAE10w?juv9WcH-WSmV$;Hiu7w4t3#`S$E!^1+q9xGPH`KtuzzAr5
LaERl^1zUvy_;n(J

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 32a401ae35fa..dfb8523c8bf4 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,4 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/DSDT",
-"tests/data/acpi/virt/DSDT.memhp",
-"tests/data/acpi/virt/DSDT.numamem",
-- 
2.25.4

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Philippe Mathieu-Daudé]

On 6/29/20 4:09 PM, Andrew Jones wrote:
> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> As firmware needs the device and uses DT, we leave the device exposed
> there. It's up to firmware to remove the nodes from DT before sending
> it on to the OS. However, there's no need to force firmware to remove
> tables from ACPI (which it doesn't know how to do anyway), so we
> simply don't add the tables in the first place. But, as we've been
> adding the tables for quite some time and don't want to change the
> default hardware exposed to versioned machines, then we only stop
> exposing the flash device tables for 5.1 and later machine types.
> 
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 5 ++++-
>  hw/arm/virt.c            | 3 +++
>  include/hw/arm/virt.h    | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1384a2cf2ab4..91f0df7b13a3 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      Aml *scope, *dsdt;
>      MachineState *ms = MACHINE(vms);
>      const MemMapEntry *memmap = vms->memmap;
> @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    if (vmc->acpi_expose_flash) {
> +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    }
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cd0834ce7faf..5adc9ff799ef 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
>  
>  static void virt_machine_5_0_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_5_1_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
>      mc->numa_mem_supported = true;
> +    vmc->acpi_expose_flash = true;
>  }
>  DEFINE_VIRT_MACHINE(5, 0)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 31878ddc7223..c65be5fe0bb6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -119,6 +119,7 @@ typedef struct {
>      bool no_highmem_ecam;
>      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>      bool kvm_no_adjvtime;
> +    bool acpi_expose_flash;
>  } VirtMachineClass;
>  
>  typedef struct {
> 

Re: [PATCH 1/4] tests/acpi: remove stale allowed tables

[Michael S. Tsirkin]

On Mon, Jun 29, 2020 at 04:09:35PM +0200, Andrew Jones wrote:
> Fixes: 93dd625f8bf7 ("tests/acpi: update expected data files")
> Signed-off-by: Andrew Jones <drjones@redhat.com>


Ah yes. Thanks for fixing this! I will add something to the pull request
script to catch this in the future.

> ---
>  tests/qtest/bios-tables-test-allowed-diff.h | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 8992f1f12b77..dfb8523c8bf4 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,19 +1 @@
>  /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/pc/DSDT",
> -"tests/data/acpi/pc/DSDT.acpihmat",
> -"tests/data/acpi/pc/DSDT.bridge",
> -"tests/data/acpi/pc/DSDT.cphp",
> -"tests/data/acpi/pc/DSDT.dimmpxm",
> -"tests/data/acpi/pc/DSDT.ipmikcs",
> -"tests/data/acpi/pc/DSDT.memhp",
> -"tests/data/acpi/pc/DSDT.numamem",
> -"tests/data/acpi/q35/DSDT",
> -"tests/data/acpi/q35/DSDT.acpihmat",
> -"tests/data/acpi/q35/DSDT.bridge",
> -"tests/data/acpi/q35/DSDT.cphp",
> -"tests/data/acpi/q35/DSDT.dimmpxm",
> -"tests/data/acpi/q35/DSDT.ipmibt",
> -"tests/data/acpi/q35/DSDT.memhp",
> -"tests/data/acpi/q35/DSDT.mmio64",
> -"tests/data/acpi/q35/DSDT.numamem",
> -"tests/data/acpi/q35/DSDT.tis",
> -- 
> 2.25.4

Re: [PATCH 0/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Michael S. Tsirkin]

On Mon, Jun 29, 2020 at 04:09:34PM +0200, Andrew Jones wrote:
> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> This change was suggested by Ard and Laszlo.
> 
> Patch 3/4 is the meat. The other patches deal with updating qtest.


acpi things:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> Thanks,
> drew
> 
> Andrew Jones (4):
>   tests/acpi: remove stale allowed tables
>   tests/acpi: virt: allow DSDT acpi table changes
>   hw/arm/virt-acpi-build: Only expose flash on older machine types
>   tests/acpi: virt: update golden masters for DSDT
> 
>  hw/arm/virt-acpi-build.c                    |   5 ++++-
>  hw/arm/virt.c                               |   3 +++
>  include/hw/arm/virt.h                       |   1 +
>  tests/data/acpi/virt/DSDT                   | Bin 5307 -> 5205 bytes
>  tests/data/acpi/virt/DSDT.memhp             | Bin 6668 -> 6566 bytes
>  tests/data/acpi/virt/DSDT.numamem           | Bin 5307 -> 5205 bytes
>  tests/qtest/bios-tables-test-allowed-diff.h |  18 ------------------
>  7 files changed, 8 insertions(+), 19 deletions(-)
> 
> -- 
> 2.25.4

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Laszlo Ersek]

On 06/29/20 16:09, Andrew Jones wrote:
> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> As firmware needs the device and uses DT, we leave the device exposed
> there. It's up to firmware to remove the nodes from DT before sending
> it on to the OS. However, there's no need to force firmware to remove
> tables from ACPI (which it doesn't know how to do anyway), so we
> simply don't add the tables in the first place. But, as we've been
> adding the tables for quite some time and don't want to change the
> default hardware exposed to versioned machines, then we only stop
> exposing the flash device tables for 5.1 and later machine types.
> 
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 5 ++++-
>  hw/arm/virt.c            | 3 +++
>  include/hw/arm/virt.h    | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1384a2cf2ab4..91f0df7b13a3 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      Aml *scope, *dsdt;
>      MachineState *ms = MACHINE(vms);
>      const MemMapEntry *memmap = vms->memmap;
> @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    if (vmc->acpi_expose_flash) {
> +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    }
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cd0834ce7faf..5adc9ff799ef 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
>  
>  static void virt_machine_5_0_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_5_1_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
>      mc->numa_mem_supported = true;
> +    vmc->acpi_expose_flash = true;
>  }
>  DEFINE_VIRT_MACHINE(5, 0)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 31878ddc7223..c65be5fe0bb6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -119,6 +119,7 @@ typedef struct {
>      bool no_highmem_ecam;
>      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>      bool kvm_no_adjvtime;
> +    bool acpi_expose_flash;
>  } VirtMachineClass;
>  
>  typedef struct {
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thank you!
Laszlo

Re: [PATCH 4/4] tests/acpi: virt: update golden masters for DSDT

[Laszlo Ersek]

On 06/29/20 16:09, Andrew Jones wrote:
> Differences between disassembled ASL files for DSDT:
> 
> @@ -5,13 +5,13 @@
>   *
>   * Disassembling to symbolic ASL+ operators
>   *
> - * Disassembly of a, Mon Jun 29 09:50:01 2020
> + * Disassembly of b, Mon Jun 29 09:50:03 2020
>   *
>   * Original Table Header:
>   *     Signature        "DSDT"
> - *     Length           0x000014BB (5307)
> + *     Length           0x00001455 (5205)
>   *     Revision         0x02
> - *     Checksum         0xD1
> + *     Checksum         0xE1
>   *     OEM ID           "BOCHS "
>   *     OEM Table ID     "BXPCDSDT"
>   *     OEM Revision     0x00000001 (1)
> @@ -45,32 +45,6 @@
>              })
>          }
> 
> -        Device (FLS0)
> -        {
> -            Name (_HID, "LNRO0015")  // _HID: Hardware ID
> -            Name (_UID, Zero)  // _UID: Unique ID
> -            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
> -            {
> -                Memory32Fixed (ReadWrite,
> -                    0x00000000,         // Address Base
> -                    0x04000000,         // Address Length
> -                    )
> -            })
> -        }
> -
> -        Device (FLS1)
> -        {
> -            Name (_HID, "LNRO0015")  // _HID: Hardware ID
> -            Name (_UID, One)  // _UID: Unique ID
> -            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
> -            {
> -                Memory32Fixed (ReadWrite,
> -                    0x04000000,         // Address Base
> -                    0x04000000,         // Address Length
> -                    )
> -            })
> -        }
> -
>          Device (FWCF)
>          {
>              Name (_HID, "QEMU0002")  // _HID: Hardware ID
> 
> The other two binaries have the same changes (the removal of the
> flash devices).
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  tests/data/acpi/virt/DSDT                   | Bin 5307 -> 5205 bytes
>  tests/data/acpi/virt/DSDT.memhp             | Bin 6668 -> 6566 bytes
>  tests/data/acpi/virt/DSDT.numamem           | Bin 5307 -> 5205 bytes
>  tests/qtest/bios-tables-test-allowed-diff.h |   3 ---
>  4 files changed, 3 deletions(-)
> 
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> index d6f5c617881c4247f55d4dcd06581f9693916b2f..e669508d175f1e3ddf355f8a9b0d419266cac8aa 100644
> GIT binary patch
> delta 28
> kcmdn3c~yhUCD<h-RD^+n>ET2!X{H9}iRuX(-<}f&0DgxFc>n+a
> 
> delta 156
> zcmcbrv0IbNCD<iow+I6R)5VEg(oAih6V(&y4c&Z#4LIUGJY9Hw{DS-q3=B;fIO0P+
> zU4W!>P_UpN7hfAE10w?juv9WcH-WSmV$;Hiu7w4t3#`S$E!^1+q9xGPH`KtuzzAr5
> LaERl^1zUvy_;n(J
> 
> diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
> index 730e95a46d2cce0af011ffc051d7342beb8f1328..4cb81f692d73526542493a0c4da9c9793cc8366e 100644
> GIT binary patch
> delta 28
> kcmeA%S!T@T66_MPOp<|tiD@F2G*jb@iRuX(-^xn@0CHUjRR910
> 
> delta 156
> zcmZ2x++)J!66_MfBgMeL^l>7WG*kP$iRuaUhHgH=1|0Doo-VvTenI{Q28N~#9Py!^
> zE<n;bC|FRCi?5B7fsp|MSSlH!n?PC&v1wsM*TMqS1=eEW7Vhi@(GuwD8){%+U<5Qj
> LIK*+|0yaqism~!^
> 
> diff --git a/tests/data/acpi/virt/DSDT.numamem b/tests/data/acpi/virt/DSDT.numamem
> index d6f5c617881c4247f55d4dcd06581f9693916b2f..e669508d175f1e3ddf355f8a9b0d419266cac8aa 100644
> GIT binary patch
> delta 28
> kcmdn3c~yhUCD<h-RD^+n>ET2!X{H9}iRuX(-<}f&0DgxFc>n+a
> 
> delta 156
> zcmcbrv0IbNCD<iow+I6R)5VEg(oAih6V(&y4c&Z#4LIUGJY9Hw{DS-q3=B;fIO0P+
> zU4W!>P_UpN7hfAE10w?juv9WcH-WSmV$;Hiu7w4t3#`S$E!^1+q9xGPH`KtuzzAr5
> LaERl^1zUvy_;n(J
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 32a401ae35fa..dfb8523c8bf4 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,4 +1 @@
>  /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/virt/DSDT",
> -"tests/data/acpi/virt/DSDT.memhp",
> -"tests/data/acpi/virt/DSDT.numamem",
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Michael S. Tsirkin]

On Mon, Jun 29, 2020 at 04:09:37PM +0200, Andrew Jones wrote:
> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> As firmware needs the device and uses DT, we leave the device exposed
> there. It's up to firmware to remove the nodes from DT before sending
> it on to the OS. However, there's no need to force firmware to remove
> tables from ACPI (which it doesn't know how to do anyway), so we
> simply don't add the tables in the first place. But, as we've been
> adding the tables for quite some time and don't want to change the
> default hardware exposed to versioned machines, then we only stop
> exposing the flash device tables for 5.1 and later machine types.
> 
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

So who's merging this? Mostly ACPI things so I guess my tree?
If so can I get acks from ARM maintainers pls?

Thanks!

> ---
>  hw/arm/virt-acpi-build.c | 5 ++++-
>  hw/arm/virt.c            | 3 +++
>  include/hw/arm/virt.h    | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1384a2cf2ab4..91f0df7b13a3 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      Aml *scope, *dsdt;
>      MachineState *ms = MACHINE(vms);
>      const MemMapEntry *memmap = vms->memmap;
> @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    if (vmc->acpi_expose_flash) {
> +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    }
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cd0834ce7faf..5adc9ff799ef 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
>  
>  static void virt_machine_5_0_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_5_1_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
>      mc->numa_mem_supported = true;
> +    vmc->acpi_expose_flash = true;
>  }
>  DEFINE_VIRT_MACHINE(5, 0)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 31878ddc7223..c65be5fe0bb6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -119,6 +119,7 @@ typedef struct {
>      bool no_highmem_ecam;
>      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>      bool kvm_no_adjvtime;
> +    bool acpi_expose_flash;
>  } VirtMachineClass;
>  
>  typedef struct {
> -- 
> 2.25.4

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Peter Maydell]

On Thu, 2 Jul 2020 at 10:53, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 29, 2020 at 04:09:37PM +0200, Andrew Jones wrote:
> > The flash device is exclusively for the host-controlled firmware, so
> > we should not expose it to the OS. Exposing it risks the OS messing
> > with it, which could break firmware runtime services and surprise the
> > OS when all its changes disappear after reboot.
> >
> > As firmware needs the device and uses DT, we leave the device exposed
> > there. It's up to firmware to remove the nodes from DT before sending
> > it on to the OS. However, there's no need to force firmware to remove
> > tables from ACPI (which it doesn't know how to do anyway), so we
> > simply don't add the tables in the first place. But, as we've been
> > adding the tables for quite some time and don't want to change the
> > default hardware exposed to versioned machines, then we only stop
> > exposing the flash device tables for 5.1 and later machine types.
> >
> > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>
> So who's merging this? Mostly ACPI things so I guess my tree?
> If so can I get acks from ARM maintainers pls?

This is on my to-look-at queue but in theory I'm on holiday this week :-)

-- PMM

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Michael S. Tsirkin]

On Thu, Jul 02, 2020 at 11:16:03AM +0100, Peter Maydell wrote:
> On Thu, 2 Jul 2020 at 10:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 29, 2020 at 04:09:37PM +0200, Andrew Jones wrote:
> > > The flash device is exclusively for the host-controlled firmware, so
> > > we should not expose it to the OS. Exposing it risks the OS messing
> > > with it, which could break firmware runtime services and surprise the
> > > OS when all its changes disappear after reboot.
> > >
> > > As firmware needs the device and uses DT, we leave the device exposed
> > > there. It's up to firmware to remove the nodes from DT before sending
> > > it on to the OS. However, there's no need to force firmware to remove
> > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > simply don't add the tables in the first place. But, as we've been
> > > adding the tables for quite some time and don't want to change the
> > > default hardware exposed to versioned machines, then we only stop
> > > exposing the flash device tables for 5.1 and later machine types.
> > >
> > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> >
> > So who's merging this? Mostly ACPI things so I guess my tree?
> > If so can I get acks from ARM maintainers pls?
> 
> This is on my to-look-at queue but in theory I'm on holiday this week :-)
> 
> -- PMM

I picked up patch 1 for now :)

-- 
MST

Re: [PATCH 0/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Auger Eric]

Hi Drew,

On 6/29/20 4:09 PM, Andrew Jones wrote:
> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> This change was suggested by Ard and Laszlo.
> 
> Patch 3/4 is the meat. The other patches deal with updating qtest.
> 
> Thanks,
> drew
> 
> Andrew Jones (4):
>   tests/acpi: remove stale allowed tables
>   tests/acpi: virt: allow DSDT acpi table changes
>   hw/arm/virt-acpi-build: Only expose flash on older machine types
>   tests/acpi: virt: update golden masters for DSDT

SERIES:
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> 
>  hw/arm/virt-acpi-build.c                    |   5 ++++-
>  hw/arm/virt.c                               |   3 +++
>  include/hw/arm/virt.h                       |   1 +
>  tests/data/acpi/virt/DSDT                   | Bin 5307 -> 5205 bytes
>  tests/data/acpi/virt/DSDT.memhp             | Bin 6668 -> 6566 bytes
>  tests/data/acpi/virt/DSDT.numamem           | Bin 5307 -> 5205 bytes
>  tests/qtest/bios-tables-test-allowed-diff.h |  18 ------------------
>  7 files changed, 8 insertions(+), 19 deletions(-)
> 

Re: [PATCH 0/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Peter Maydell]

On Mon, 29 Jun 2020 at 15:09, Andrew Jones <drjones@redhat.com> wrote:
>
> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
>
> This change was suggested by Ard and Laszlo.



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Igor Mammedov]

On Mon, 29 Jun 2020 16:09:37 +0200
Andrew Jones <drjones@redhat.com> wrote:

> The flash device is exclusively for the host-controlled firmware, so
> we should not expose it to the OS. Exposing it risks the OS messing
> with it, which could break firmware runtime services and surprise the
> OS when all its changes disappear after reboot.
> 
> As firmware needs the device and uses DT, we leave the device exposed
> there. It's up to firmware to remove the nodes from DT before sending
> it on to the OS. However, there's no need to force firmware to remove
> tables from ACPI (which it doesn't know how to do anyway), so we
> simply don't add the tables in the first place. But, as we've been
> adding the tables for quite some time and don't want to change the
> default hardware exposed to versioned machines, then we only stop
> exposing the flash device tables for 5.1 and later machine types.
> 
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 5 ++++-
>  hw/arm/virt.c            | 3 +++
>  include/hw/arm/virt.h    | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1384a2cf2ab4..91f0df7b13a3 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      Aml *scope, *dsdt;
>      MachineState *ms = MACHINE(vms);
>      const MemMapEntry *memmap = vms->memmap;
> @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    if (vmc->acpi_expose_flash) {
> +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    }
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cd0834ce7faf..5adc9ff799ef 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
>  
>  static void virt_machine_5_0_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_5_1_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
>      mc->numa_mem_supported = true;
> +    vmc->acpi_expose_flash = true;

we usually do not version ACPI tables changes
(unless we have a good reason to do so)

>  }
>  DEFINE_VIRT_MACHINE(5, 0)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 31878ddc7223..c65be5fe0bb6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -119,6 +119,7 @@ typedef struct {
>      bool no_highmem_ecam;
>      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>      bool kvm_no_adjvtime;
> +    bool acpi_expose_flash;
>  } VirtMachineClass;
>  
>  typedef struct {

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:
> On Mon, 29 Jun 2020 16:09:37 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > The flash device is exclusively for the host-controlled firmware, so
> > we should not expose it to the OS. Exposing it risks the OS messing
> > with it, which could break firmware runtime services and surprise the
> > OS when all its changes disappear after reboot.
> > 
> > As firmware needs the device and uses DT, we leave the device exposed
> > there. It's up to firmware to remove the nodes from DT before sending
> > it on to the OS. However, there's no need to force firmware to remove
> > tables from ACPI (which it doesn't know how to do anyway), so we
> > simply don't add the tables in the first place. But, as we've been
> > adding the tables for quite some time and don't want to change the
> > default hardware exposed to versioned machines, then we only stop
> > exposing the flash device tables for 5.1 and later machine types.
> > 
> > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 5 ++++-
> >  hw/arm/virt.c            | 3 +++
> >  include/hw/arm/virt.h    | 1 +
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 1384a2cf2ab4..91f0df7b13a3 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> >  static void
> >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >  {
> > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> >      Aml *scope, *dsdt;
> >      MachineState *ms = MACHINE(vms);
> >      const MemMapEntry *memmap = vms->memmap;
> > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > +    if (vmc->acpi_expose_flash) {
> > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > +    }
> >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index cd0834ce7faf..5adc9ff799ef 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> >  
> >  static void virt_machine_5_0_options(MachineClass *mc)
> >  {
> > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > +
> >      virt_machine_5_1_options(mc);
> >      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> >      mc->numa_mem_supported = true;
> > +    vmc->acpi_expose_flash = true;
> 
> we usually do not version ACPI tables changes
> (unless we have a good reason to do so)

Even when the change is to remove the exposure of hardware from the guest?
Before this change, if a guest looked, it had a flash, after this change,
if a guest looks, it doesn't. I'd feel much better versioning a change
like that, than not.

Thanks,
drew

> 
> >  }
> >  DEFINE_VIRT_MACHINE(5, 0)
> >  
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 31878ddc7223..c65be5fe0bb6 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -119,6 +119,7 @@ typedef struct {
> >      bool no_highmem_ecam;
> >      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
> >      bool kvm_no_adjvtime;
> > +    bool acpi_expose_flash;
> >  } VirtMachineClass;
> >  
> >  typedef struct {
> 
> 

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Michael S. Tsirkin]

On Tue, Jul 14, 2020 at 07:51:09AM +0200, Andrew Jones wrote:
> On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:
> > On Mon, 29 Jun 2020 16:09:37 +0200
> > Andrew Jones <drjones@redhat.com> wrote:
> > 
> > > The flash device is exclusively for the host-controlled firmware, so
> > > we should not expose it to the OS. Exposing it risks the OS messing
> > > with it, which could break firmware runtime services and surprise the
> > > OS when all its changes disappear after reboot.
> > > 
> > > As firmware needs the device and uses DT, we leave the device exposed
> > > there. It's up to firmware to remove the nodes from DT before sending
> > > it on to the OS. However, there's no need to force firmware to remove
> > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > simply don't add the tables in the first place. But, as we've been
> > > adding the tables for quite some time and don't want to change the
> > > default hardware exposed to versioned machines, then we only stop
> > > exposing the flash device tables for 5.1 and later machine types.
> > > 
> > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 5 ++++-
> > >  hw/arm/virt.c            | 3 +++
> > >  include/hw/arm/virt.h    | 1 +
> > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 1384a2cf2ab4..91f0df7b13a3 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> > >  static void
> > >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >  {
> > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > >      Aml *scope, *dsdt;
> > >      MachineState *ms = MACHINE(vms);
> > >      const MemMapEntry *memmap = vms->memmap;
> > > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > +    if (vmc->acpi_expose_flash) {
> > > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > +    }
> > >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> > >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index cd0834ce7faf..5adc9ff799ef 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> > >  
> > >  static void virt_machine_5_0_options(MachineClass *mc)
> > >  {
> > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > +
> > >      virt_machine_5_1_options(mc);
> > >      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> > >      mc->numa_mem_supported = true;
> > > +    vmc->acpi_expose_flash = true;
> > 
> > we usually do not version ACPI tables changes
> > (unless we have a good reason to do so)
> 
> Even when the change is to remove the exposure of hardware from the guest?
> Before this change, if a guest looked, it had a flash, after this change,
> if a guest looks, it doesn't.

It's up to the relevant maintainers who know what the semantics are.
FYI ACPI tables only change across a reset though.
So it's a question of whether guests get confused even if this
changes after a reboot.
Versioning is generally safer, but it's a good idea to document
the motivation for it.


> I'd feel much better versioning a change
> like that, than not.
> 
> Thanks,
> drew
> 
> > 
> > >  }
> > >  DEFINE_VIRT_MACHINE(5, 0)
> > >  
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index 31878ddc7223..c65be5fe0bb6 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -119,6 +119,7 @@ typedef struct {
> > >      bool no_highmem_ecam;
> > >      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
> > >      bool kvm_no_adjvtime;
> > > +    bool acpi_expose_flash;
> > >  } VirtMachineClass;
> > >  
> > >  typedef struct {
> > 
> > 

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

On Tue, Jul 14, 2020 at 04:57:50AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jul 14, 2020 at 07:51:09AM +0200, Andrew Jones wrote:
> > On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:
> > > On Mon, 29 Jun 2020 16:09:37 +0200
> > > Andrew Jones <drjones@redhat.com> wrote:
> > > 
> > > > The flash device is exclusively for the host-controlled firmware, so
> > > > we should not expose it to the OS. Exposing it risks the OS messing
> > > > with it, which could break firmware runtime services and surprise the
> > > > OS when all its changes disappear after reboot.
> > > > 
> > > > As firmware needs the device and uses DT, we leave the device exposed
> > > > there. It's up to firmware to remove the nodes from DT before sending
> > > > it on to the OS. However, there's no need to force firmware to remove
> > > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > > simply don't add the tables in the first place. But, as we've been
> > > > adding the tables for quite some time and don't want to change the
> > > > default hardware exposed to versioned machines, then we only stop
> > > > exposing the flash device tables for 5.1 and later machine types.
> > > > 
> > > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  hw/arm/virt-acpi-build.c | 5 ++++-
> > > >  hw/arm/virt.c            | 3 +++
> > > >  include/hw/arm/virt.h    | 1 +
> > > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 1384a2cf2ab4..91f0df7b13a3 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> > > >  static void
> > > >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > >  {
> > > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > > >      Aml *scope, *dsdt;
> > > >      MachineState *ms = MACHINE(vms);
> > > >      const MemMapEntry *memmap = vms->memmap;
> > > > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > > >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > > >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > > > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > +    if (vmc->acpi_expose_flash) {
> > > > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > +    }
> > > >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > > >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> > > >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index cd0834ce7faf..5adc9ff799ef 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> > > >  
> > > >  static void virt_machine_5_0_options(MachineClass *mc)
> > > >  {
> > > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > +
> > > >      virt_machine_5_1_options(mc);
> > > >      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> > > >      mc->numa_mem_supported = true;
> > > > +    vmc->acpi_expose_flash = true;
> > > 
> > > we usually do not version ACPI tables changes
> > > (unless we have a good reason to do so)
> > 
> > Even when the change is to remove the exposure of hardware from the guest?
> > Before this change, if a guest looked, it had a flash, after this change,
> > if a guest looks, it doesn't.
> 
> It's up to the relevant maintainers who know what the semantics are.
> FYI ACPI tables only change across a reset though.
> So it's a question of whether guests get confused even if this
> changes after a reboot.

Yup, but it's still the same "machine", so a user may wonder why it
changed.

> Versioning is generally safer, but it's a good idea to document
> the motivation for it.
>

Well, in this case, we could probably push this change to old machine
types and nobody would notice. If a guest is using ACPI, then it must
be using firmware, and if they're using firmware, then they can't be
using the flash. So the user shouldn't care if it's there or not. The
only justification for the versioning is because "it's safer". If
people feel strongly about avoiding versioning when it's not obviously
necessary, then I can respin without it.

Thanks,
drew

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Michael S. Tsirkin]

On Tue, Jul 14, 2020 at 11:23:25AM +0200, Andrew Jones wrote:
> On Tue, Jul 14, 2020 at 04:57:50AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jul 14, 2020 at 07:51:09AM +0200, Andrew Jones wrote:
> > > On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:
> > > > On Mon, 29 Jun 2020 16:09:37 +0200
> > > > Andrew Jones <drjones@redhat.com> wrote:
> > > > 
> > > > > The flash device is exclusively for the host-controlled firmware, so
> > > > > we should not expose it to the OS. Exposing it risks the OS messing
> > > > > with it, which could break firmware runtime services and surprise the
> > > > > OS when all its changes disappear after reboot.
> > > > > 
> > > > > As firmware needs the device and uses DT, we leave the device exposed
> > > > > there. It's up to firmware to remove the nodes from DT before sending
> > > > > it on to the OS. However, there's no need to force firmware to remove
> > > > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > > > simply don't add the tables in the first place. But, as we've been
> > > > > adding the tables for quite some time and don't want to change the
> > > > > default hardware exposed to versioned machines, then we only stop
> > > > > exposing the flash device tables for 5.1 and later machine types.
> > > > > 
> > > > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  hw/arm/virt-acpi-build.c | 5 ++++-
> > > > >  hw/arm/virt.c            | 3 +++
> > > > >  include/hw/arm/virt.h    | 1 +
> > > > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > > index 1384a2cf2ab4..91f0df7b13a3 100644
> > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> > > > >  static void
> > > > >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > >  {
> > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > > > >      Aml *scope, *dsdt;
> > > > >      MachineState *ms = MACHINE(vms);
> > > > >      const MemMapEntry *memmap = vms->memmap;
> > > > > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > > > >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > > > >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > > > > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > +    if (vmc->acpi_expose_flash) {
> > > > > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > +    }
> > > > >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > > > >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> > > > >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > index cd0834ce7faf..5adc9ff799ef 100644
> > > > > --- a/hw/arm/virt.c
> > > > > +++ b/hw/arm/virt.c
> > > > > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> > > > >  
> > > > >  static void virt_machine_5_0_options(MachineClass *mc)
> > > > >  {
> > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > > +
> > > > >      virt_machine_5_1_options(mc);
> > > > >      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> > > > >      mc->numa_mem_supported = true;
> > > > > +    vmc->acpi_expose_flash = true;
> > > > 
> > > > we usually do not version ACPI tables changes
> > > > (unless we have a good reason to do so)
> > > 
> > > Even when the change is to remove the exposure of hardware from the guest?
> > > Before this change, if a guest looked, it had a flash, after this change,
> > > if a guest looks, it doesn't.
> > 
> > It's up to the relevant maintainers who know what the semantics are.
> > FYI ACPI tables only change across a reset though.
> > So it's a question of whether guests get confused even if this
> > changes after a reboot.
> 
> Yup, but it's still the same "machine", so a user may wonder why it
> changed.
> 
> > Versioning is generally safer, but it's a good idea to document
> > the motivation for it.
> >
> 
> Well, in this case, we could probably push this change to old machine
> types and nobody would notice. If a guest is using ACPI, then it must
> be using firmware, and if they're using firmware, then they can't be
> using the flash. So the user shouldn't care if it's there or not. The
> only justification for the versioning is because "it's safer". If
> people feel strongly about avoiding versioning when it's not obviously
> necessary, then I can respin without it.
> 
> Thanks,
> drew

It's up to maintainers either way, but please do tweak the motivation in the commit log
to include the above.

-- 
MST

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Igor Mammedov]

On Tue, 14 Jul 2020 11:23:25 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Tue, Jul 14, 2020 at 04:57:50AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jul 14, 2020 at 07:51:09AM +0200, Andrew Jones wrote:  
> > > On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:  
> > > > On Mon, 29 Jun 2020 16:09:37 +0200
> > > > Andrew Jones <drjones@redhat.com> wrote:
> > > >   
> > > > > The flash device is exclusively for the host-controlled firmware, so
> > > > > we should not expose it to the OS. Exposing it risks the OS messing
> > > > > with it, which could break firmware runtime services and surprise the
> > > > > OS when all its changes disappear after reboot.
> > > > > 
> > > > > As firmware needs the device and uses DT, we leave the device exposed
> > > > > there. It's up to firmware to remove the nodes from DT before sending
> > > > > it on to the OS. However, there's no need to force firmware to remove
> > > > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > > > simply don't add the tables in the first place. But, as we've been
> > > > > adding the tables for quite some time and don't want to change the
> > > > > default hardware exposed to versioned machines, then we only stop
> > > > > exposing the flash device tables for 5.1 and later machine types.
> > > > > 
> > > > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  hw/arm/virt-acpi-build.c | 5 ++++-
> > > > >  hw/arm/virt.c            | 3 +++
> > > > >  include/hw/arm/virt.h    | 1 +
> > > > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > > index 1384a2cf2ab4..91f0df7b13a3 100644
> > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> > > > >  static void
> > > > >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > >  {
> > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > > > >      Aml *scope, *dsdt;
> > > > >      MachineState *ms = MACHINE(vms);
> > > > >      const MemMapEntry *memmap = vms->memmap;
> > > > > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > > > >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > > > >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > > > > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > +    if (vmc->acpi_expose_flash) {
> > > > > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > +    }
> > > > >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > > > >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> > > > >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > index cd0834ce7faf..5adc9ff799ef 100644
> > > > > --- a/hw/arm/virt.c
> > > > > +++ b/hw/arm/virt.c
> > > > > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> > > > >  
> > > > >  static void virt_machine_5_0_options(MachineClass *mc)
> > > > >  {
> > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > > +
> > > > >      virt_machine_5_1_options(mc);
> > > > >      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> > > > >      mc->numa_mem_supported = true;
> > > > > +    vmc->acpi_expose_flash = true;  
> > > > 
> > > > we usually do not version ACPI tables changes
> > > > (unless we have a good reason to do so)  
> > > 
> > > Even when the change is to remove the exposure of hardware from the guest?
> > > Before this change, if a guest looked, it had a flash, after this change,
> > > if a guest looks, it doesn't.  
> > 
> > It's up to the relevant maintainers who know what the semantics are.
> > FYI ACPI tables only change across a reset though.
> > So it's a question of whether guests get confused even if this
> > changes after a reboot.  
> 
> Yup, but it's still the same "machine", so a user may wonder why it
> changed.

you can have a different firmware with the same machine type either
and it might look differently to guest OS but don't bother versioning
FW. APCI tables are also part of FW (but generated by QEMU), so the same
usually rule applies.
 
> > Versioning is generally safer, but it's a good idea to document
> > the motivation for it.
> >  
> 
> Well, in this case, we could probably push this change to old machine
> types and nobody would notice. If a guest is using ACPI, then it must
> be using firmware, and if they're using firmware, then they can't be
> using the flash. So the user shouldn't care if it's there or not. The
> only justification for the versioning is because "it's safer". If
> people feel strongly about avoiding versioning when it's not obviously
> necessary, then I can respin without it.

From my pov if it doesn't break anything don't version it, since versioning
adds complexity which cost time during review, so it would be nicer to reviewers
and to future yourself if you can help to keep it as simple as possible.

In this particular case I'd drop versioning.

> Thanks,
> drew
> 
> 

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

On Tue, Jul 14, 2020 at 04:41:41PM +0200, Igor Mammedov wrote:
> On Tue, 14 Jul 2020 11:23:25 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Tue, Jul 14, 2020 at 04:57:50AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Jul 14, 2020 at 07:51:09AM +0200, Andrew Jones wrote:  
> > > > On Mon, Jul 13, 2020 at 10:49:07AM +0200, Igor Mammedov wrote:  
> > > > > On Mon, 29 Jun 2020 16:09:37 +0200
> > > > > Andrew Jones <drjones@redhat.com> wrote:
> > > > >   
> > > > > > The flash device is exclusively for the host-controlled firmware, so
> > > > > > we should not expose it to the OS. Exposing it risks the OS messing
> > > > > > with it, which could break firmware runtime services and surprise the
> > > > > > OS when all its changes disappear after reboot.
> > > > > > 
> > > > > > As firmware needs the device and uses DT, we leave the device exposed
> > > > > > there. It's up to firmware to remove the nodes from DT before sending
> > > > > > it on to the OS. However, there's no need to force firmware to remove
> > > > > > tables from ACPI (which it doesn't know how to do anyway), so we
> > > > > > simply don't add the tables in the first place. But, as we've been
> > > > > > adding the tables for quite some time and don't want to change the
> > > > > > default hardware exposed to versioned machines, then we only stop
> > > > > > exposing the flash device tables for 5.1 and later machine types.
> > > > > > 
> > > > > > Suggested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > > > > Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > > ---
> > > > > >  hw/arm/virt-acpi-build.c | 5 ++++-
> > > > > >  hw/arm/virt.c            | 3 +++
> > > > > >  include/hw/arm/virt.h    | 1 +
> > > > > >  3 files changed, 8 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > > > index 1384a2cf2ab4..91f0df7b13a3 100644
> > > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > > @@ -749,6 +749,7 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> > > > > >  static void
> > > > > >  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > > >  {
> > > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > > > > >      Aml *scope, *dsdt;
> > > > > >      MachineState *ms = MACHINE(vms);
> > > > > >      const MemMapEntry *memmap = vms->memmap;
> > > > > > @@ -767,7 +768,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > > > >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> > > > > >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> > > > > >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > > > > > -    acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > > +    if (vmc->acpi_expose_flash) {
> > > > > > +        acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> > > > > > +    }
> > > > > >      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > > > > >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> > > > > >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > > index cd0834ce7faf..5adc9ff799ef 100644
> > > > > > --- a/hw/arm/virt.c
> > > > > > +++ b/hw/arm/virt.c
> > > > > > @@ -2482,9 +2482,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> > > > > >  
> > > > > >  static void virt_machine_5_0_options(MachineClass *mc)
> > > > > >  {
> > > > > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > > > +
> > > > > >      virt_machine_5_1_options(mc);
> > > > > >      compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
> > > > > >      mc->numa_mem_supported = true;
> > > > > > +    vmc->acpi_expose_flash = true;  
> > > > > 
> > > > > we usually do not version ACPI tables changes
> > > > > (unless we have a good reason to do so)  
> > > > 
> > > > Even when the change is to remove the exposure of hardware from the guest?
> > > > Before this change, if a guest looked, it had a flash, after this change,
> > > > if a guest looks, it doesn't.  
> > > 
> > > It's up to the relevant maintainers who know what the semantics are.
> > > FYI ACPI tables only change across a reset though.
> > > So it's a question of whether guests get confused even if this
> > > changes after a reboot.  
> > 
> > Yup, but it's still the same "machine", so a user may wonder why it
> > changed.
> 
> you can have a different firmware with the same machine type either
> and it might look differently to guest OS but don't bother versioning
> FW. APCI tables are also part of FW (but generated by QEMU), so the same
> usually rule applies.

That makes sense. However, while users of real machines agree to update
their firmware after determining what will change, or at least expect
there could be a change that they'll need to adapt to, users of virtual
machines simply reboot, getting new firmware and ACPI tables, and then
possibly new surprises. Indeed, they may have opted to use a virtual
machine precisely so they could keep the environment stable across
updates of the real machine. IOW, since we can maintain a versioned
machine, then maybe we should?

>  
> > > Versioning is generally safer, but it's a good idea to document
> > > the motivation for it.
> > >  
> > 
> > Well, in this case, we could probably push this change to old machine
> > types and nobody would notice. If a guest is using ACPI, then it must
> > be using firmware, and if they're using firmware, then they can't be
> > using the flash. So the user shouldn't care if it's there or not. The
> > only justification for the versioning is because "it's safer". If
> > people feel strongly about avoiding versioning when it's not obviously
> > necessary, then I can respin without it.
> 
> From my pov if it doesn't break anything don't version it,

I don't see how we can be sure that we won't break anything. Although,
in this case, we *probably* won't.

> since versioning
> adds complexity which cost time during review, so it would be nicer to reviewers
> and to future yourself if you can help to keep it as simple as possible.

I agree with all of that.

> 
> In this particular case I'd drop versioning.
>

So it sounds to me like we have some flexibility in our versioned machine
maintenance. We can choose to forgo the usual compat code when the risk is
deemed low enough. And, if somebody screams, we can always fix it later.
I can live with that. I'll go ahead and respin without the versioning.

Thanks,
drew

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

On Wed, Jul 15, 2020 at 08:36:48AM +0200, Andrew Jones wrote:
> On Tue, Jul 14, 2020 at 04:41:41PM +0200, Igor Mammedov wrote:
> > 
> > In this particular case I'd drop versioning.
> >
> 
> So it sounds to me like we have some flexibility in our versioned machine
> maintenance. We can choose to forgo the usual compat code when the risk is
> deemed low enough. And, if somebody screams, we can always fix it later.
> I can live with that. I'll go ahead and respin without the versioning.
>

Actually this patch was already merged

  2c1fb4d5c011 hw/arm/virt-acpi-build: Only expose flash on older machine types

and I don't see much value in posting a patch to remove the compat code.

Regarding the result of this discussion, my take is that unless the
policy is to always or to never use versioning, then I think we should
have guidelines documented which we can follow. Would one of the ACPI
maintainers like to submit a document with the reasoning for when and
when not to use versioning? And also for what's expected of the
commit message justification when versioning is necessary?

Thanks,
drew

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Laszlo Ersek]

Hi Drew,

On 07/15/20 08:36, Andrew Jones wrote:

> So it sounds to me like we have some flexibility in our versioned machine
> maintenance. We can choose to forgo the usual compat code when the risk is
> deemed low enough. And, if somebody screams, we can always fix it later.
> I can live with that. I'll go ahead and respin without the versioning.

In that case, please don't simply remove the acpi_dsdt_add_flash() call
from build_dsdt(), because then "git blame" won't be able to help later.
Can you please replace the call with a comment instead, similar to the
RTC comment from commit 67736a25f865 ("ARM: virt: Don't generate RTC
ACPI device when using UEFI", 2016-01-15)?

Thanks!
Laszlo

Re: [PATCH 3/4] hw/arm/virt-acpi-build: Only expose flash on older machine types

[Andrew Jones]

On Wed, Jul 15, 2020 at 02:26:19PM +0200, Laszlo Ersek wrote:
> Hi Drew,
> 
> On 07/15/20 08:36, Andrew Jones wrote:
> 
> > So it sounds to me like we have some flexibility in our versioned machine
> > maintenance. We can choose to forgo the usual compat code when the risk is
> > deemed low enough. And, if somebody screams, we can always fix it later.
> > I can live with that. I'll go ahead and respin without the versioning.
> 
> In that case, please don't simply remove the acpi_dsdt_add_flash() call
> from build_dsdt(), because then "git blame" won't be able to help later.
> Can you please replace the call with a comment instead, similar to the
> RTC comment from commit 67736a25f865 ("ARM: virt: Don't generate RTC
> ACPI device when using UEFI", 2016-01-15)?
>

In the end I won't be respinning, as this patch is already merged. And,
unless Igor twists my arm, then I don't plan to write another patch
that removes the compat code. If I did remove it, I'd put a comment
in there for git-blame to find. And, in the comment I'd write "Igor
said to remove this", because git-blame would otherwise blame me :-)

Thanks,
drew