| Message ID | 56BA6F6C.5000301@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wei, On 2016/2/10 6:59, Wei Huang wrote: > > On 02/04/2016 12:51 AM, Shannon Zhao wrote: >> >> >> On 2016/2/4 14:10, Wei Huang wrote: >>> >>> On 02/03/2016 07:44 PM, Shannon Zhao wrote: > > <snip> > >>> I reversed the order of edge pulling. The state is 1 according to printk >>> inside gpio_keys driver. However the reboot still failed with two >>> reboots (1 very early, 1 later). >>> >> Because to make the input work, it should call input_event twice I think. >> >> input_event(input, type, button->code, 1) means the button pressed >> input_event(input, type, button->code, 0) means the button released >> >> But We only see guest entering gpio_keys_gpio_report_event once. >> >> My original purpose is like below: >> >> call qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1) to make guest >> execute input_event(input, type, button->code, 1) >> call qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0) to make guest >> execute input_event(input, type, button->code, 0). >> >> But even though it calls qemu_set_irq twice, it only calls pl061_update >> once in qemu. > > Hi Shannon, > > Assuming that we are talking about the special case you found (i.e. send > reboot very early and then send another one after guest VM fully > booted). Dug into the code further, here are my findings: > > === Why ACPI case failed? === > QEMU pl061.c checks the current request against the new request (see > s->old_in_data ^ s->data in pl061.c file). If no change, nothing will > happen. > > So two consecutive reboots will cause the following state change; > apparently the second request won't trigger VM reboot because > pl01_update() decides _not_ to inject IRQ into guest VM. > > (PL061State fields) data old_in_data istate > VM boot 0 0 0 > 1st ACPI reboot request 8 0 0 > After VM PL061 driver ACK 8 8 0 > 2nd ACPI reboot request 8 no-change, no IRQ <== > > To solve this problem, we have to invert PL061State->data at least once > to trigger IRQ inside VM. Two solutions: > > S1: "Pulse" > static void virt_powerdown_req(Notifier *n, void *opaque) > { > qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0); > qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1); > } > > S2: "Invert" > static int old_gpio_level = 0; > static void virt_powerdown_req(Notifier *n, void *opaque) > { > /* use gpio Pin 3 for power button event */ > qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), !old_gpio_level); > old_gpio_level = !old_gpio_level; > } > The S2 still doesn't work. After sending the early first reboot, whne guest boots well, it doesn't react to the second reboot while it reacts to the third one. > Both S1 and S2 works for ACPI. But S1 has problem with DT, which is > explained below. > > === Why DT fails with S1 === > DT approach requires both PL061 and gpio-keys to work together. Looking > into guest kernel gpio-keys/input code, you will find that it only > reacts to both LEVEL-HI and input changes. Here is the code snip from > drivers/input/input.c file: > > /* auto-repeat bypasses state updates */ > if (value == 2) { > disposition = INPUT_PASS_TO_HANDLERS; > break; > } > > if (!!test_bit(code, dev->key) != !!value) { > __change_bit(code, dev->key); > disposition = INPUT_PASS_TO_HANDLERS; > } > > Unless we adds gpio-keys DT property to "autorepeat", the > "!!test_bit(code, dev->key) != !!value" is always FALSE with S1. Thus > systemd won't receive any input event; and no power-switch will be > triggered. > > In comparison S2 will work because value is changed very time. > > === Summary === > 1. Cleaning PL061 state is required. That means "[PATCH V2 1/2] ARM: > PL061: Clear PL061 device state after reset" is necessary. > 2. S2 is better. To support S2, we needs to change GPIO IRQ polarity to > AML_ACTIVE_BOTH in ACPI description. > Thanks. But we don't have the AML_ACTIVE_BOTH since there is only one bit for "Interrupt Polarity" in ACPI table. See ACPI SPEC 6.0 "Table 6-216 Extended Interrupt Descriptor Definition" Bit [2] Interrupt Polarity, _LL 0 Active-High: This interrupt is sampled when the signal is high, or true. 1 Active-Low: This interrupt is sampled when the signal is low, or false. > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -326,7 +326,7 @@ 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}; > - aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, > + aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, 3, What's the meaning of 3 here? > AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1, > "GPO0", NULL, 0)); > aml_append(dev, aml_name_decl("_AEI", aei)); Thanks,
On 02/20/2016 04:53 AM, Shannon Zhao wrote: > Hi Wei, > > On 2016/2/10 6:59, Wei Huang wrote: >> >> On 02/04/2016 12:51 AM, Shannon Zhao wrote: >>> >>> >>> On 2016/2/4 14:10, Wei Huang wrote: >>>> >>>> On 02/03/2016 07:44 PM, Shannon Zhao wrote: >> >> <snip> >> >>>> I reversed the order of edge pulling. The state is 1 according to printk >>>> inside gpio_keys driver. However the reboot still failed with two >>>> reboots (1 very early, 1 later). >>>> >>> Because to make the input work, it should call input_event twice I think. >>> >>> input_event(input, type, button->code, 1) means the button pressed >>> input_event(input, type, button->code, 0) means the button released >>> >>> But We only see guest entering gpio_keys_gpio_report_event once. >>> >>> My original purpose is like below: >>> >>> call qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1) to make guest >>> execute input_event(input, type, button->code, 1) >>> call qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0) to make guest >>> execute input_event(input, type, button->code, 0). >>> >>> But even though it calls qemu_set_irq twice, it only calls pl061_update >>> once in qemu. >> >> Hi Shannon, >> >> Assuming that we are talking about the special case you found (i.e. send >> reboot very early and then send another one after guest VM fully >> booted). Dug into the code further, here are my findings: >> >> === Why ACPI case failed? === >> QEMU pl061.c checks the current request against the new request (see >> s->old_in_data ^ s->data in pl061.c file). If no change, nothing will >> happen. >> >> So two consecutive reboots will cause the following state change; >> apparently the second request won't trigger VM reboot because >> pl01_update() decides _not_ to inject IRQ into guest VM. >> >> (PL061State fields) data old_in_data istate >> VM boot 0 0 0 >> 1st ACPI reboot request 8 0 0 >> After VM PL061 driver ACK 8 8 0 >> 2nd ACPI reboot request 8 no-change, no IRQ <== >> >> To solve this problem, we have to invert PL061State->data at least once >> to trigger IRQ inside VM. Two solutions: >> >> S1: "Pulse" >> static void virt_powerdown_req(Notifier *n, void *opaque) >> { >> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0); >> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1); >> } >> >> S2: "Invert" >> static int old_gpio_level = 0; >> static void virt_powerdown_req(Notifier *n, void *opaque) >> { >> /* use gpio Pin 3 for power button event */ >> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), !old_gpio_level); >> old_gpio_level = !old_gpio_level; >> } >> > The S2 still doesn't work. After sending the early first reboot, whne > guest boots well, it doesn't react to the second reboot while it reacts > to the third one. I can reproduce it. The problem is that gpio-keys only handles GPIO_ACTIVE_LOW or GPIO_ACTIVE_HIGH. It can't react to ACTIVE_BOTH. That explains why it reacts to the 3rd request: HIGH (ingored, too early, keyboard driver not loaded) ==> LOW (ignored, ACTIVE_HIGH only) ==> HIGH (received). This problem is full of dilemma, across different components in QEMU or guest VM: * For qemu/pl011.c to generate IRQ, we need to have level transition. That means qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1) in virt_powerdown_req() isn't enough. * If we do "invert" (i.e. S2 above), gpio-keys inside VM isn't happy with it. * If we do pulse (i.e. S1 above), DT fails because of the reason explained below. Plus the GPIO seems to receive the same state due to non-preemptive (you mentioned it long time ago). Not sure what to do next. Some wild ideas can be: 1) set up a worker thread to pull down GPIO after a fix time. This emulates real world scenario. 2) enable PL061 to support auto-ACK after receiving ACK from guest VM. Thanks, -Wei > >> Both S1 and S2 works for ACPI. But S1 has problem with DT, which is >> explained below. >> >> === Why DT fails with S1 === >> DT approach requires both PL061 and gpio-keys to work together. Looking >> into guest kernel gpio-keys/input code, you will find that it only >> reacts to both LEVEL-HI and input changes. Here is the code snip from >> drivers/input/input.c file: >> >> /* auto-repeat bypasses state updates */ >> if (value == 2) { >> disposition = INPUT_PASS_TO_HANDLERS; >> break; >> } >> >> if (!!test_bit(code, dev->key) != !!value) { >> __change_bit(code, dev->key); >> disposition = INPUT_PASS_TO_HANDLERS; >> } >> >> Unless we adds gpio-keys DT property to "autorepeat", the >> "!!test_bit(code, dev->key) != !!value" is always FALSE with S1. Thus >> systemd won't receive any input event; and no power-switch will be >> triggered. >> >> In comparison S2 will work because value is changed very time. >> >> === Summary === >> 1. Cleaning PL061 state is required. That means "[PATCH V2 1/2] ARM: >> PL061: Clear PL061 device state after reset" is necessary. >> 2. S2 is better. To support S2, we needs to change GPIO IRQ polarity to >> AML_ACTIVE_BOTH in ACPI description. >> > Thanks. But we don't have the AML_ACTIVE_BOTH since there is only one > bit for "Interrupt Polarity" in ACPI table. > See ACPI SPEC 6.0 "Table 6-216 Extended Interrupt Descriptor Definition" > Bit [2] Interrupt Polarity, _LL > 0 Active-High: This interrupt is sampled when the signal is high, > or true. > 1 Active-Low: This interrupt is sampled when the signal is low, or > false. > >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -326,7 +326,7 @@ 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}; >> - aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, >> + aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, 3, > What's the meaning of 3 here? > >> AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1, >> "GPO0", NULL, 0)); >> aml_append(dev, aml_name_decl("_AEI", aei)); > > Thanks, >
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 7b124f6..7b1dc5e 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -326,7 +326,7 @@ 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}; - aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, + aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, 3, AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1, "GPO0", NULL, 0)); aml_append(dev, aml_name_decl("_AEI", aei)); diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 5695e72..ed02a6c 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -547,10 +547,12 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic) } static DeviceState *pl061_dev; +static int old_gpio_level = 0; static void virt_powerdown_req(Notifier *n, void *opaque) { /* use gpio Pin 3 for power button event */ - qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1); + qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), !old_gpio_level); + old_gpio_level = !old_gpio_level; } static Notifier virt_system_powerdown_notifier = {