Re: [Qemu-arm] [PATCH v14 7/9] ARM: ACPI: Add GPIO notification type for hardware RAS error

[gengdongjiu]

On 2017/12/28 22:53, Igor Mammedov wrote:
> On Thu, 28 Dec 2017 13:54:16 +0800
> Dongjiu Geng <address@hidden> wrote:
> 
>> In ARM platform we implements a notification of error events
>> via a GPIO pin. For this GPIO-signaled events, we choose GPIO
>> pin 4 for hardware error device (PNP0C33), So _E04 should be
>> added to ACPI DSDT table. When GPIO-pin 4 signaled a events,
>> the guest ACPI driver will receive this notification and
>> handing the error.
>>
>> In order to better trigger the GPIO IRQ, we defined a notifier
>> hardware_error_notifiers. If Qemu wants to deliver a GPIO-Signal
>> notification, will call it.
>>
>> Signed-off-by: Dongjiu Geng <address@hidden>
>> ---
>> 1. Address discussion result about guest APEI notification type for 
>> SIGBUS_MCEERR_AO SIGBUS in [1],
>>    the discussion conclusion is using GPIO-Signal
>>
>> [1]:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03397.html
>> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03467.html
>> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03601.html
>> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03775.html
>>
>> 2. The ASL dump for the GPIO and hardware error device
>>
>> ................
>> Device (GPO0)
>> {
>>     Name (_AEI, ResourceTemplate ()  // _AEI: ACPI Event Interrupts
>>     {
>>         .............
>>         GpioInt (Edge, ActiveHigh, Exclusive, PullUp, 0x0000,
>>             "GPO0", 0x00, ResourceConsumer, ,
>>             )
>>             {   // Pin list
>>                 0x0004
>>             }
>>     })
>>     Method (_E04, 0, NotSerialized)  // _Exx: Edge-Triggered GPE
>>     {
>>         Notify (ERRD, 0x80) // Status Change
>>     }
>> }
>> Device (ERRD)
>> {
>>     Name (_HID, EisaId ("PNP0C33") /* Error Device */)  // _HID: Hardware ID
>>     Name (_UID, Zero)  // _UID: Unique ID
>>     Method (_STA, 0, NotSerialized)  // _STA: Status
>>     {
>>         Return (0x0F)
>>     }
>> }
>>
>> 3. Below is the guest log when Qemu notifies guest using GPIO-signal after 
>> record a CPER
>> [  504.164899] {1}[Hardware Error]: Hardware error from APEI Generic 
>> Hardware Error Source: 7
>> [  504.166970] {1}[Hardware Error]: event severity: recoverable
>> [  504.251650] {1}[Hardware Error]:  Error 0, type: recoverable
>> [  504.252974] {1}[Hardware Error]:   section_type: memory error
>> [  504.254380] {1}[Hardware Error]:   physical_address: 0x00000000000003ec
>> [  504.255879] {1}[Hardware Error]:   error_type: 3, multi-bit ECC
>> ---
>>  hw/arm/virt-acpi-build.c | 31 ++++++++++++++++++++++++++++++-
>>  hw/arm/virt.c            | 18 ++++++++++++++++++
>>  include/sysemu/sysemu.h  |  3 +++
>>  vl.c                     | 12 ++++++++++++
>>  4 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index b7d45cd..06c14b3 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -49,6 +49,7 @@
>>  
>>  #define ARM_SPI_BASE 32
>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>> +#define ACPI_HARDWARE_ERROR_DEVICE "ERRD"
>>  
>>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>  {
>> @@ -340,7 +341,13 @@ static void acpi_dsdt_add_gpio(Aml *scope, const 
>> MemMapEntry *gpio_memmap,
>>  
>>      Aml *aei = aml_resource_template();
>>      /* Pin 3 for power button */
>> -    const uint32_t pin_list[1] = {3};
>> +    uint32_t pin_list[1] = {3};
>> +    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
>> +                                 AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
>> +                                 "GPO0", NULL, 0));
>> +
>> +    /* Pin 4 for hardware error device */
>> +    pin_list[0] = 4;
>>      aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
>>                                   AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
>>                                   "GPO0", NULL, 0));
>> @@ -351,6 +358,13 @@ static void acpi_dsdt_add_gpio(Aml *scope, const 
>> MemMapEntry *gpio_memmap,
>>      aml_append(method, aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
>>                                    aml_int(0x80)));
>>      aml_append(dev, method);
>> +
>> +    /* _E04 is handle for hardware error */
>> +    method = aml_method("_E04", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_notify(aml_name(ACPI_HARDWARE_ERROR_DEVICE),
>> +                                  aml_int(0x80)));
> aml_int(0x80) /* ACPI ... : Notification For Generic Error Sources */
sure. thanks

> 
>> +    aml_append(dev, method);
>> +
>>      aml_append(scope, dev);
>>  }
>>  
>> @@ -363,6 +377,20 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>>      aml_append(scope, dev);
>>  }
>>  
>> +static void acpi_dsdt_add_error_device(Aml *scope)
>> +{
>> +    Aml *dev = aml_device(ACPI_HARDWARE_ERROR_DEVICE);
>> +    Aml *method;
>> +
>> +    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C33")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>> +
>> +    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_return(aml_int(0x0f)));
> no need for dummy _STA method, device is assumed to be present if there is no 
> _STA 
Igor,
  do you mean remove above two line code as shown in [1]?
I dump the DSDT table in my host Ubuntu PC for the error device (PNP0C33), it 
has the _STA, as shown in [2].
do we not want to add the _STA for guest?

[1]
+    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
+    aml_append(method, aml_return(aml_int(0x0f)));

[2]:
        Device (WERR)
        {
            Name (_HID, EisaId ("PNP0C33"))  // _HID: Hardware ID
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                If (LGreaterEqual (OSYS, 0x07D9))
                {
                    Return (0x0F)
                }
                Else
                {
                    Return (Zero)
                }
            }
        }
> 
>> +    aml_append(dev, method);
>> +    aml_append(scope, dev);
>> +}
>> +
>>  /* RSDP */
>>  static GArray *
>>  build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>> @@ -716,6 +744,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>>      acpi_dsdt_add_power_button(scope);
>> +    acpi_dsdt_add_error_device(scope);
>>  
>>      aml_append(dsdt, scope);
>>  
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 6b7a0fe..68495c2 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -701,16 +701,27 @@ static void create_rtc(const VirtMachineState *vms, 
>> qemu_irq *pic)
>>  }
>>  
>>  static DeviceState *gpio_key_dev;
>> +static DeviceState *gpio_err_dev;
>>  static void virt_powerdown_req(Notifier *n, void *opaque)
>>  {
>>      /* use gpio Pin 3 for power button event */
>>      qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
>>  }
>>  
>> +static void virt_error_notify_req(Notifier *n, void *opaque)
>> +{
>> +    /* use gpio Pin 4 for hardware error event */
>> +    qemu_set_irq(qdev_get_gpio_in(gpio_err_dev, 0), 1);
>> +}
>> +
>>  static Notifier virt_system_powerdown_notifier = {
>>      .notify = virt_powerdown_req
>>  };
>>  
>> +static Notifier virt_hardware_error_notifier = {
>> +    .notify = virt_error_notify_req
>> +};
>> +
>>  static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
>>  {
>>      char *nodename;
>> @@ -739,6 +750,10 @@ static void create_gpio(const VirtMachineState *vms, 
>> qemu_irq *pic)
>>  
>>      gpio_key_dev = sysbus_create_simple("gpio-key", -1,
>>                                          qdev_get_gpio_in(pl061_dev, 3));
>> +
>> +    gpio_err_dev = sysbus_create_simple("gpio-key", -1,
>> +                                        qdev_get_gpio_in(pl061_dev, 4));
>> +
>>      qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
>>      qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", 
>> "gpio-keys");
>>      qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
>> @@ -755,6 +770,9 @@ static void create_gpio(const VirtMachineState *vms, 
>> qemu_irq *pic)
>>      /* connect powerdown request */
>>      qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);
>>  
>> +    /* connect hardware error notify request */
>> +    qemu_register_hardware_error_notifier(&virt_hardware_error_notifier);
>> +
>>      g_free(nodename);
>>  }
>>  
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index b213696..86931cf 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -75,6 +75,7 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
>>  void qemu_system_shutdown_request(ShutdownCause reason);
>>  void qemu_system_powerdown_request(void);
>>  void qemu_register_powerdown_notifier(Notifier *notifier);
>> +void qemu_register_hardware_error_notifier(Notifier *notifier);
>>  void qemu_system_debug_request(void);
>>  void qemu_system_vmstop_request(RunState reason);
>>  void qemu_system_vmstop_request_prepare(void);
>> @@ -93,6 +94,8 @@ void qemu_remove_machine_init_done_notifier(Notifier 
>> *notify);
>>  
>>  void qemu_announce_self(void);
>>  
>> +void qemu_hardware_error_notify(void);
>> +
>>  extern int autostart;
>>  
>>  typedef enum {
>> diff --git a/vl.c b/vl.c
>> index d632693..3552f7d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1614,6 +1614,8 @@ static int suspend_requested;
>>  static WakeupReason wakeup_reason;
>>  static NotifierList powerdown_notifiers =
>>      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
>> +static NotifierList hardware_error_notifiers =
>> +    NOTIFIER_LIST_INITIALIZER(hardware_error_notifiers);
>>  static NotifierList suspend_notifiers =
>>      NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
>>  static NotifierList wakeup_notifiers =
>> @@ -1850,12 +1852,22 @@ void qemu_register_powerdown_notifier(Notifier 
>> *notifier)
>>      notifier_list_add(&powerdown_notifiers, notifier);
>>  }
>>  
>> +void qemu_register_hardware_error_notifier(Notifier *notifier)
>> +{
>> +    notifier_list_add(&hardware_error_notifiers, notifier);
>> +}
>> +
>>  void qemu_system_debug_request(void)
>>  {
>>      debug_requested = 1;
>>      qemu_notify_event();
>>  }
>>  
>> +void qemu_hardware_error_notify(void)
>> +{
>> +    notifier_list_notify(&hardware_error_notifiers, NULL);
>> +}
> 
> I'd prefer if you'd replace all this Notifier code with a machine callback
> and call it when needed.
Ok, thanks for this suggestion. I will check the code and see how to add a 
machine callback.


> 
>>  static bool main_loop_should_exit(void)
>>  {
>>      RunState r;
> 
> 
> .
>