diff mbox

[1/1] arm: virt: change GPIO trigger interrupt to pulse

Message ID 56D04596.7010205@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shannon Zhao Feb. 26, 2016, 12:31 p.m. UTC
On 2016/2/25 6:22, Wei Huang wrote:
> 
> 
> 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.
Yeah, right! But other than a worker thread, I'd like to use a timer.
Then set a latency between pull up and down like we press a button in
real world.

So how about below patch? I've tested it and it works both for ACPI and
DT. Could you help verify if it works for you? Thanks.

 static Notifier virt_system_powerdown_notifier = {
@@ -609,6 +618,8 @@ static void create_gpio(const VirtBoardInfo *vbi,
qemu_irq *pic)

     /* connect powerdown request */
     qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);
+    gpio_key_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
gpio_key_timer_expired,
+                                  NULL);

     g_free(nodename);
 }

Thanks,

Comments

Peter Maydell Feb. 26, 2016, 12:53 p.m. UTC | #1
On 26 February 2016 at 12:31, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> So how about below patch? I've tested it and it works both for ACPI and
> DT. Could you help verify if it works for you? Thanks.
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 15658f4..4d45ea2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -558,11 +558,20 @@ static void create_rtc(const VirtBoardInfo *vbi,
> qemu_irq *pic)
>      g_free(nodename);
>  }
>
> +#define GPIO_KEY_LATENCY 500 /* 500ms */

I don't understand why a 500ms pulse is better than a short one.

> +static QEMUTimer *gpio_key_timer;

This is state, and must be migrated somehow.

thanks
-- PMM
Shannon Zhao Feb. 26, 2016, 2:54 p.m. UTC | #2
On 2016/2/26 20:53, Peter Maydell wrote:
> On 26 February 2016 at 12:31, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>> So how about below patch? I've tested it and it works both for ACPI and
>> DT. Could you help verify if it works for you? Thanks.
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 15658f4..4d45ea2 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -558,11 +558,20 @@ static void create_rtc(const VirtBoardInfo *vbi,
>> qemu_irq *pic)
>>       g_free(nodename);
>>   }
>>
>> +#define GPIO_KEY_LATENCY 500 /* 500ms */
>
> I don't understand why a 500ms pulse is better than a short one.
>
Oh, I just pick a value which seems like a real latency for pressing a 
button. What's your suggestion?

>> +static QEMUTimer *gpio_key_timer;
>
> This is state, and must be migrated somehow.
>
Ah, yes. So is it fine if we move this timer and the notifier 
virt_system_powerdown_notifier to VirtMachineState?

Thanks,
Peter Maydell Feb. 26, 2016, 3:06 p.m. UTC | #3
On 26 February 2016 at 14:54, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> On 2016/2/26 20:53, Peter Maydell wrote:
>> I don't understand why a 500ms pulse is better than a short one.
>>
> Oh, I just pick a value which seems like a real latency for pressing a
> button. What's your suggestion?

I would prefer to avoid the pain of having a timer whose state
needs to be migrated. It's unclear to me why a 500ms pulse
will solve anything that an instantaneous pulse does not,
so I'd like to better understand the problem first.

thanks
-- PMM
Wei Huang Feb. 26, 2016, 3:28 p.m. UTC | #4
On 02/26/2016 09:06 AM, Peter Maydell wrote:
> On 26 February 2016 at 14:54, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> On 2016/2/26 20:53, Peter Maydell wrote:
>>> I don't understand why a 500ms pulse is better than a short one.
>>>
>> Oh, I just pick a value which seems like a real latency for pressing a
>> button. What's your suggestion?
> 
> I would prefer to avoid the pain of having a timer whose state
> needs to be migrated. It's unclear to me why a 500ms pulse
> will solve anything that an instantaneous pulse does not,
> so I'd like to better understand the problem first.

The problem we found with pulse was: only the last state change in GPIO
is received by guest VM. In other words, with 0(L)->1(H)->0(L) or
1(H)->0(L)->1(0), PL061 only sees the last state (0 and 1). I guess this
is because QEMU is non-preemptive. The solution is to have the following
steps:
  * qemu_set_irq(gpio_in, 1)
  * yeild to guest VM
  * qemu_set_irq(gpio_in, 0)

Is there any way to do so in QEMU without using timer?

> 
> thanks
> -- PMM
>
Peter Maydell Feb. 26, 2016, 3:42 p.m. UTC | #5
On 26 February 2016 at 15:28, Wei Huang <wei@redhat.com> wrote:
>
>
> On 02/26/2016 09:06 AM, Peter Maydell wrote:
>> On 26 February 2016 at 14:54, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>>> On 2016/2/26 20:53, Peter Maydell wrote:
>>>> I don't understand why a 500ms pulse is better than a short one.
>>>>
>>> Oh, I just pick a value which seems like a real latency for pressing a
>>> button. What's your suggestion?
>>
>> I would prefer to avoid the pain of having a timer whose state
>> needs to be migrated. It's unclear to me why a 500ms pulse
>> will solve anything that an instantaneous pulse does not,
>> so I'd like to better understand the problem first.
>
> The problem we found with pulse was: only the last state change in GPIO
> is received by guest VM. In other words, with 0(L)->1(H)->0(L) or
> 1(H)->0(L)->1(0), PL061 only sees the last state (0 and 1). I guess this
> is because QEMU is non-preemptive. The solution is to have the following
> steps:
>   * qemu_set_irq(gpio_in, 1)
>   * yeild to guest VM
>   * qemu_set_irq(gpio_in, 0)
>
> Is there any way to do so in QEMU without using timer?

You have no guarantee that the guest VM will necessarily
make any progress even with a 500ms pulse length, so I don't
think increasing the length of the pulse is going to solidly
fix this.

As usual with any kind of interrupt you either need to
trigger on an edge (and latch the trigger in the interrupt
controller until the guest picks it up), or trigger on a
level, and keep the level high until the guest acknowledges
by writing back to the original device to tell it to drop
the level.

thanks
-- PMM
Shannon Zhao Feb. 27, 2016, 1:55 a.m. UTC | #6
On 2016/2/26 23:42, Peter Maydell wrote:
> On 26 February 2016 at 15:28, Wei Huang <wei@redhat.com> wrote:
>>
>>
>> On 02/26/2016 09:06 AM, Peter Maydell wrote:
>>> On 26 February 2016 at 14:54, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>>>> On 2016/2/26 20:53, Peter Maydell wrote:
>>>>> I don't understand why a 500ms pulse is better than a short one.
>>>>>
>>>> Oh, I just pick a value which seems like a real latency for pressing a
>>>> button. What's your suggestion?
>>>
>>> I would prefer to avoid the pain of having a timer whose state
>>> needs to be migrated. It's unclear to me why a 500ms pulse
>>> will solve anything that an instantaneous pulse does not,
>>> so I'd like to better understand the problem first.
>>
>> The problem we found with pulse was: only the last state change in GPIO
>> is received by guest VM. In other words, with 0(L)->1(H)->0(L) or
>> 1(H)->0(L)->1(0), PL061 only sees the last state (0 and 1). I guess this
>> is because QEMU is non-preemptive. The solution is to have the following
>> steps:
>>   * qemu_set_irq(gpio_in, 1)
>>   * yeild to guest VM
>>   * qemu_set_irq(gpio_in, 0)
>>
>> Is there any way to do so in QEMU without using timer?
> 
> You have no guarantee that the guest VM will necessarily
> make any progress even with a 500ms pulse length, 
Yeah, if the guest is very busy! But it's true in the real world like
that we press a GPIO_KEY button, but guest is too busy that it can't
react to this then the guest can't shutdown.

> so I don't
> think increasing the length of the pulse is going to solidly
> fix this.
> 
This is not simply increasing the length. It just makes the guest not
blocked after the first qemu_set_irq so that the guest can detect the
button is pressed(i.e. get the value 1). Then after any time, call the
second qemu_set_irq then guest can detect the button is released.

You must be aware that here the purpose of qemu_set_irq is only to set
the value of gpio pin not directly set the interrupt controller. As for
injecting the interrupt, it's what the PL061 device does which detects
the pin value changed.

And for GPIO_KEY, it must detect 1 and 0 then it realizes it as a valid
input.

Have a look at the below function in drivers/input/keyboard/gpio_keys.c.

static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
{
	const struct gpio_keys_button *button = bdata->button;
	struct input_dev *input = bdata->input;
	unsigned int type = button->type ?: EV_KEY;
	int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^
button->active_low;

	if (type == EV_ABS) {
		if (state)
			input_event(input, type, button->code, button->value);
	} else {
		input_event(input, type, button->code, !!state);
	}
	input_sync(input);
}


> As usual with any kind of interrupt you either need to
> trigger on an edge (and latch the trigger in the interrupt
> controller until the guest picks it up), or trigger on a
> level, and keep the level high until the guest acknowledges
> by writing back to the original device to tell it to drop
> the level.
> 
As said, here we only want change the some pin value and want guest to
get the value. The interrupt is triggered by PL061 device which just
notifies the guest that pin value is changed, please read.

Thanks,
diff mbox

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 15658f4..4d45ea2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -558,11 +558,20 @@  static void create_rtc(const VirtBoardInfo *vbi,
qemu_irq *pic)
     g_free(nodename);
 }

+#define GPIO_KEY_LATENCY 500 /* 500ms */
 static DeviceState *pl061_dev;
+static QEMUTimer *gpio_key_timer;
+static void gpio_key_timer_expired(void *vp)
+{
+    qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 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);
+    timer_mod(gpio_key_timer,
+              qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + GPIO_KEY_LATENCY);
 }