Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse

[Wei Huang]

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;
}

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.

I attach a small patch in this email. It should work for your DT case.
Please test it and let me know your comments.

Thanks,
-Wei


>

gpio-fix.txt
Description: Text document