Re: [PATCH v3 5/7] ACPI ERST: create ERST device for pc/x86 machines.

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 28 May 2021 14:14:17 -0400
Eric DeVolder <eric.devolder@oracle.com> wrote:

> This change enables ERST support for x86 guests.
> 
> ERST can be disabled at run-time, for example, with:
> 
>  -machine q35,erst=off

1st: not everyone needs this feature so I'd turn it off by default
2nd: it looks to me that patch breaks cross version migration
     (one more reason to disable ERST by default for simplicity sake),
     or it needs to be disabled on old machine types at least.
3rd: patch should not be necessary at all, if PCI device is used as
     a template for your device (see comments on 3/7)
4th: try to avoid using "ifdefs"
     (if you follow trhough #3 as suggested this patch including
      ifdefs can be dropped), on top of that in 3/7 I'd split out
     ACPI table related code into a separate object file so  that
     the device itself could be easily compiled out or made dynamically
     loadable as a module.

> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
>  hw/i386/acpi-build.c |  7 +++++++
>  hw/i386/pc.c         | 31 +++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h |  1 +
>  3 files changed, 39 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index de98750..6ba79db 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -43,6 +43,7 @@
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
>  #include "hw/acpi/vmgenid.h"
> +#include "hw/acpi/erst.h"
>  #include "hw/boards.h"
>  #include "sysemu/tpm_backend.h"
>  #include "hw/rtc/mc146818rtc_regs.h"
> @@ -2388,6 +2389,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>                      ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
>                      x86ms->oem_table_id);
>  
> +    if (pcms->erst_enabled) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_erst(tables_blob, tables->linker,
> +                   x86ms->oem_id, x86ms->oem_table_id);
> +    }
> +
>      vmgenid_dev = find_vmgenid_dev();
>      if (vmgenid_dev) {
>          acpi_add_table(table_offsets, tables_blob);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8a84b25..b7b4cc4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -74,6 +74,7 @@
>  #include "qemu/cutils.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu_hotplug.h"
> +#include "hw/acpi/erst.h"
>  #include "hw/boards.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> @@ -1111,6 +1112,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>      ISADevice *pit = NULL;
>      MemoryRegion *ioport80_io = g_new(MemoryRegion, 1);
>      MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1);
> +    const X86MachineState *x86ms = X86_MACHINE(pcms);
>  
>      memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1);
>      memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io);
> @@ -1153,6 +1155,11 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>      }
>      *rtc_state = mc146818_rtc_init(isa_bus, 2000, rtc_irq);
>  
> +    if (pcms->erst_enabled && x86_machine_is_acpi_enabled(x86ms)) {
> +        hwaddr base = HPET_BASE + 0x10000UL;
> +        setup_erst_dev(base, error_fatal);
> +    }
> +
>      qemu_register_boot_set(pc_boot_set, *rtc_state);
>  
>      if (!xen_enabled() && pcms->pit_enabled) {
> @@ -1529,6 +1536,22 @@ static void pc_machine_set_hpet(Object *obj, bool value, Error **errp)
>      pcms->hpet_enabled = value;
>  }
>  
> +#ifdef CONFIG_ACPI
> +static bool pc_machine_get_erst(Object *obj, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    return pcms->erst_enabled;
> +}
> +
> +static void pc_machine_set_erst(Object *obj, bool value, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    pcms->erst_enabled = value;
> +}
> +#endif
> +
>  static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
>                                              const char *name, void *opaque,
>                                              Error **errp)
> @@ -1628,6 +1651,9 @@ static void pc_machine_initfn(Object *obj)
>  #ifdef CONFIG_HPET
>      pcms->hpet_enabled = true;
>  #endif
> +#ifdef CONFIG_ACPI
> +    pcms->erst_enabled = true;
> +#endif
>  
>      pc_system_flash_create(pcms);
>      pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> @@ -1752,6 +1778,11 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_bool(oc, "hpet",
>          pc_machine_get_hpet, pc_machine_set_hpet);
>  
> +#ifdef CONFIG_ACPI
> +    object_class_property_add_bool(oc, "erst",
> +        pc_machine_get_erst, pc_machine_set_erst);
> +#endif
> +
>      object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size",
>          pc_machine_get_max_fw_size, pc_machine_set_max_fw_size,
>          NULL, NULL);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index dcf060b..4458c8f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -45,6 +45,7 @@ typedef struct PCMachineState {
>      bool sata_enabled;
>      bool pit_enabled;
>      bool hpet_enabled;
> +    bool erst_enabled;
>      uint64_t max_fw_size;
>  
>      /* NUMA information: */