| Message ID | 1514440458-10515-8-git-send-email-gengdongjiu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 28 Dec 2017 13:54:16 +0800 Dongjiu Geng <gengdongjiu@huawei.com> 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 <gengdongjiu@huawei.com> > --- > 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 */ > + 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 > + 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. > static bool main_loop_should_exit(void) > { > RunState r;
On 2017/12/28 22:53, Igor Mammedov wrote: > On Thu, 28 Dec 2017 13:54:16 +0800 > Dongjiu Geng <gengdongjiu@huawei.com> 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 <gengdongjiu@huawei.com> >> --- >> 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; > > > . >
On Wed, 3 Jan 2018 11:48:30 +0800 gengdongjiu <gengdongjiu@huawei.com> wrote: > On 2017/12/28 22:53, Igor Mammedov wrote: > > On Thu, 28 Dec 2017 13:54:16 +0800 > > Dongjiu Geng <gengdongjiu@huawei.com> wrote: [...] > >> +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))); compared to host, yours method does nothing, read ACPI6.2 "6.3.7 _STA (Status)" one more time > [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); > >> +} > >> + [...]
On 2018/1/3 21:36, Igor Mammedov wrote: > On Wed, 3 Jan 2018 11:48:30 +0800 > gengdongjiu <gengdongjiu@huawei.com> wrote: > >> On 2017/12/28 22:53, Igor Mammedov wrote: >>> On Thu, 28 Dec 2017 13:54:16 +0800 >>> Dongjiu Geng <gengdongjiu@huawei.com> wrote: > [...] >>>> +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))); > compared to host, yours method does nothing, > read ACPI6.2 "6.3.7 _STA (Status)" one more time Thanks for the pointing out. yes, you are right. As the spec statement[1], Device is assumed to be present if there is no _STA. [1]: ACPI6.2 "6.3.7 _STA (Status), Return Value Information" If a device object (including the processor object) does not have an _STA object, then OSPM assumes that all of the above bits are set (i.e., the device is present, enabled, shown in the UI, and functioning). > >> [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); >>>> +} >>>> + > [...] > > > . >
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_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))); + 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); +} + static bool main_loop_should_exit(void) { RunState r;
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 <gengdongjiu@huawei.com> --- 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(-)