diff mbox

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

Message ID 1454005340-15682-1-git-send-email-wei@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Huang Jan. 28, 2016, 6:22 p.m. UTC
When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
request will succeed; but the following shutdown/reboot requests
fail to trigger VMs to react. Notice that in mach-virt machine
model GPIO is defined as edge-triggered and active-high in ACPI.
This patch changes the behavior of powerdown notifier from PULLUP
to PULSE. It solves the problem described above (i.e. reboot
continues to work).

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Shannon Zhao Jan. 29, 2016, 10:10 a.m. UTC | #1
Hi?

This makes ACPI work well but makes DT not work. The reason is systemd or
acpid open /dev/input/event0 failed. So the interrupt could be injected and
could see under /proc/interrupts but guest doesn't have any action. I'll
investigate why it opens failed later.

2016?1?29?????Wei Huang <wei@redhat.com> ???

> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
> request will succeed; but the following shutdown/reboot requests
> fail to trigger VMs to react. Notice that in mach-virt machine
> model GPIO is defined as edge-triggered and active-high in ACPI.
> This patch changes the behavior of powerdown notifier from PULLUP
> to PULSE. It solves the problem described above (i.e. reboot
> continues to work).
>
> Signed-off-by: Wei Huang <wei@redhat.com <javascript:;>>
> ---
>  hw/arm/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 05f9087..b5468a9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -546,7 +546,7 @@ static DeviceState *pl061_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(pl061_dev, 3), 1);
> +    qemu_irq_pulse(qdev_get_gpio_in(pl061_dev, 3));
>  }
>
>  static Notifier virt_system_powerdown_notifier = {
> --
> 1.8.3.1
>
>
Wei Huang Jan. 29, 2016, 2:35 p.m. UTC | #2
On 01/29/2016 04:10 AM, Shannon Zhao wrote:
> Hi?
> 
> This makes ACPI work well but makes DT not work. The reason is systemd or
> acpid open /dev/input/event0 failed. So the interrupt could be injected and
> could see under /proc/interrupts but guest doesn't have any action. I'll
> investigate why it opens failed later.

That is interesting. Could you try it with the following? This reverses
the order to down-up and worked on ACPI case.

qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);

Thanks,
-Wei

> 
> 2016?1?29?????Wei Huang <wei@redhat.com> ???
> 
>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>> request will succeed; but the following shutdown/reboot requests
>> fail to trigger VMs to react. Notice that in mach-virt machine
>> model GPIO is defined as edge-triggered and active-high in ACPI.
>> This patch changes the behavior of powerdown notifier from PULLUP
>> to PULSE. It solves the problem described above (i.e. reboot
>> continues to work).
>>
>> Signed-off-by: Wei Huang <wei@redhat.com <javascript:;>>
>> ---
>>  hw/arm/virt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 05f9087..b5468a9 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -546,7 +546,7 @@ static DeviceState *pl061_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(pl061_dev, 3), 1);
>> +    qemu_irq_pulse(qdev_get_gpio_in(pl061_dev, 3));
>>  }
>>
>>  static Notifier virt_system_powerdown_notifier = {
>> --
>> 1.8.3.1
>>
>>
>
Shannon Zhao Jan. 29, 2016, 2:46 p.m. UTC | #3
On 2016/1/29 22:35, Wei Huang wrote:
>
>
> On 01/29/2016 04:10 AM, Shannon Zhao wrote:
>> Hi?
>>
>> This makes ACPI work well but makes DT not work. The reason is systemd or
>> acpid open /dev/input/event0 failed. So the interrupt could be injected and
>> could see under /proc/interrupts but guest doesn't have any action. I'll
>> investigate why it opens failed later.
>
> That is interesting. Could you try it with the following? This reverses
> the order to down-up and worked on ACPI case.
>
Yeah, that's very weird.

> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>
I'll try this tomorrow. But even if this works, it's still weird.

> Thanks,
> -Wei
>
>>
>> 2016?1?29?????Wei Huang <wei@redhat.com> ???
>>
>>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>>> request will succeed; but the following shutdown/reboot requests
>>> fail to trigger VMs to react. Notice that in mach-virt machine
>>> model GPIO is defined as edge-triggered and active-high in ACPI.
>>> This patch changes the behavior of powerdown notifier from PULLUP
>>> to PULSE. It solves the problem described above (i.e. reboot
>>> continues to work).
>>>
>>> Signed-off-by: Wei Huang <wei@redhat.com <javascript:;>>
>>> ---
>>>   hw/arm/virt.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 05f9087..b5468a9 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -546,7 +546,7 @@ static DeviceState *pl061_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(pl061_dev, 3), 1);
>>> +    qemu_irq_pulse(qdev_get_gpio_in(pl061_dev, 3));
>>>   }
>>>
>>>   static Notifier virt_system_powerdown_notifier = {
>>> --
>>> 1.8.3.1
>>>
>>>
>>
Wei Huang Jan. 29, 2016, 2:50 p.m. UTC | #4
On 01/29/2016 08:46 AM, Shannon Zhao wrote:
> 
> 
> On 2016/1/29 22:35, Wei Huang wrote:
>>
>>
>> On 01/29/2016 04:10 AM, Shannon Zhao wrote:
>>> Hi?
>>>
>>> This makes ACPI work well but makes DT not work. The reason is
>>> systemd or
>>> acpid open /dev/input/event0 failed. So the interrupt could be
>>> injected and
>>> could see under /proc/interrupts but guest doesn't have any action. I'll
>>> investigate why it opens failed later.
>>
>> That is interesting. Could you try it with the following? This reverses
>> the order to down-up and worked on ACPI case.
>>
> Yeah, that's very weird.
> 
>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>>
> I'll try this tomorrow. But even if this works, it's still weird.

To reproduce this case, do the following steps using current upstream
qemu: create vm => reboot vm (succeed) => reboot or shutdown vm (fail).
Apparently the last interrupt wasn't received correctly.

-Wei


> 
>> Thanks,
>> -Wei
>>
>>>
>>> 2016?1?29?????Wei Huang <wei@redhat.com> ???
>>>
>>>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>>>> request will succeed; but the following shutdown/reboot requests
>>>> fail to trigger VMs to react. Notice that in mach-virt machine
>>>> model GPIO is defined as edge-triggered and active-high in ACPI.
>>>> This patch changes the behavior of powerdown notifier from PULLUP
>>>> to PULSE. It solves the problem described above (i.e. reboot
>>>> continues to work).
>>>>
>>>> Signed-off-by: Wei Huang <wei@redhat.com <javascript:;>>
>>>> ---
>>>>   hw/arm/virt.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index 05f9087..b5468a9 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -546,7 +546,7 @@ static DeviceState *pl061_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(pl061_dev, 3), 1);
>>>> +    qemu_irq_pulse(qdev_get_gpio_in(pl061_dev, 3));
>>>>   }
>>>>
>>>>   static Notifier virt_system_powerdown_notifier = {
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>>
>>>
>
Peter Maydell Jan. 29, 2016, 2:50 p.m. UTC | #5
On 29 January 2016 at 14:46, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> On 2016/1/29 22:35, Wei Huang wrote:
>> On 01/29/2016 04:10 AM, Shannon Zhao wrote:
>>> This makes ACPI work well but makes DT not work. The reason is systemd or
>>> acpid open /dev/input/event0 failed. So the interrupt could be injected
>>> and
>>> could see under /proc/interrupts but guest doesn't have any action. I'll
>>> investigate why it opens failed later.
>>
>>
>> That is interesting. Could you try it with the following? This reverses
>> the order to down-up and worked on ACPI case.
>>
> Yeah, that's very weird.
>
>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>>
> I'll try this tomorrow. But even if this works, it's still weird.

I wonder if we should be asserting the GPIO pin in the powerdown-request
hook and then deasserting it on system reset somewhere...

thanks
-- PMM
Wei Huang Jan. 29, 2016, 3:13 p.m. UTC | #6
On 01/29/2016 08:50 AM, Peter Maydell wrote:
> On 29 January 2016 at 14:46, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> On 2016/1/29 22:35, Wei Huang wrote:
>>> On 01/29/2016 04:10 AM, Shannon Zhao wrote:
>>>> This makes ACPI work well but makes DT not work. The reason is systemd or
>>>> acpid open /dev/input/event0 failed. So the interrupt could be injected
>>>> and
>>>> could see under /proc/interrupts but guest doesn't have any action. I'll
>>>> investigate why it opens failed later.
>>>
>>>
>>> That is interesting. Could you try it with the following? This reverses
>>> the order to down-up and worked on ACPI case.
>>>
>> Yeah, that's very weird.
>>
>>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
>>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>>>
>> I'll try this tomorrow. But even if this works, it's still weird.
> 
> I wonder if we should be asserting the GPIO pin in the powerdown-request
> hook and then deasserting it on system reset somewhere...

This is another possibility. We can try to reset the pl061 state by
hooking up with dc->reset and see what happens.

> 
> thanks
> -- PMM
>
Shannon Zhao Jan. 29, 2016, 3:22 p.m. UTC | #7
On 2016/1/29 22:50, Wei Huang wrote:
>
> On 01/29/2016 08:46 AM, Shannon Zhao wrote:
>> >
>> >
>> >On 2016/1/29 22:35, Wei Huang wrote:
>>> >>
>>> >>
>>> >>On 01/29/2016 04:10 AM, Shannon Zhao wrote:
>>>> >>>Hi?
>>>> >>>
>>>> >>>This makes ACPI work well but makes DT not work. The reason is
>>>> >>>systemd or
>>>> >>>acpid open /dev/input/event0 failed. So the interrupt could be
>>>> >>>injected and
>>>> >>>could see under /proc/interrupts but guest doesn't have any action. I'll
>>>> >>>investigate why it opens failed later.
>>> >>
>>> >>That is interesting. Could you try it with the following? This reverses
>>> >>the order to down-up and worked on ACPI case.
>>> >>
>> >Yeah, that's very weird.
>> >
>>> >>qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
>>> >>qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>>> >>
>> >I'll try this tomorrow. But even if this works, it's still weird.
> To reproduce this case, do the following steps using current upstream
> qemu: create vm => reboot vm (succeed) => reboot or shutdown vm (fail).
> Apparently the last interrupt wasn't received correctly.

Yes, I reproduce this today. Let's clarify current state.
Firstly, for ACPI it should use qemu_irq_pulse since we make the GPIO 
pin edge-triggered. And for DT, it uses gpio-key which is also 
edge-triggered that we could get from output of guest /proc/interrupts.

Secondly, current upstream qemu with your patch makes second reboot 
works when using ACPI. But first shutdown/reboot doesn't works when 
using DT since the systemd or acpid open /dev/input/event0 failed. This 
is what I'm surprised.

Wei, what userspace program your guest uses? systemd or acpid? Could you 
please try to use DT to test your patch? And see if there is a same 
result with me.(I know Redhat kernel uses ACPI by default, so you could 
append acpi=off to switch to DT)

Thanks,
Peter Maydell Jan. 29, 2016, 3:29 p.m. UTC | #8
On 29 January 2016 at 15:13, Wei Huang <wei@redhat.com> wrote:
>
>
> On 01/29/2016 08:50 AM, Peter Maydell wrote:
>> I wonder if we should be asserting the GPIO pin in the powerdown-request
>> hook and then deasserting it on system reset somewhere...
>
> This is another possibility. We can try to reset the pl061 state by
> hooking up with dc->reset and see what happens.

Ah, yes, PL061 hasn't been updated to implement reset. That is almost
certainly your problem.

thanks
-- PMM
Shannon Zhao Jan. 30, 2016, 8:18 a.m. UTC | #9
On 2016/1/29 22:35, Wei Huang wrote:
> 
> On 01/29/2016 04:10 AM, Shannon Zhao wrote:
>> > Hi?
>> > 
>> > This makes ACPI work well but makes DT not work. The reason is systemd or
>> > acpid open /dev/input/event0 failed.
To correct, systemd or acpid open /dev/input/event0 successfully but it
waits for events and when we input "system_powerdown", it doesn't get
the event.

> So the interrupt could be injected and
>> > could see under /proc/interrupts but guest doesn't have any action. I'll
>> > investigate why it opens failed later.
> That is interesting. Could you try it with the following? This reverses
> the order to down-up and worked on ACPI case.
> 
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
If we don't input "system_powerdown" during guest booting(before device
driver load), current QEMU with above codes could work for DT. But if we
input "system_powerdown" during guest booting, guest has no action when
we input "system_powerdown" again after guest booting completely.
I add dc->reset for pl061 but get the same result.

Below is the output of acpid:

./acpid -l -n -d
input layer /dev/input/event0 (gpio-keys) opened successfully, fd 4
inotify fd: 5
inotify wd: 1
RTNETLINK1 answers: No such file or directory
acpid: error talking to the kernel via netlink
netlink opened successfully
acpid: starting up with netlink and the input layer
parsing conf file /etc/acpi/events/powerbtn
acpid: 1 rule loaded
acpid: waiting for events: event logging is on

It will stuck here even if we input "system_powerdown".

Below is the right output when we input "system_powerdown".

./acpid -l -n -d
input layer /dev/input/event0 (gpio-keys) opened successfully, fd 4
inotify fd: 5
inotify wd: 1
RTNETLINK1 answers: No such file or directory
acpid: error talking to the kernel via netlink
netlink opened successfully
acpid: starting up with netlink and the input layer
parsing conf file /etc/acpi/events/powerbtn
acpid: 1 rule loaded
acpid: waiting for events: event logging is on
acpid: received input layer event "button/power PBTN 00000080 00000000"
acpid: rule from /etc/acpi/events/powerbtn matched
acpid: executing action "/etc/acpi/powerbtn.sh"

Thanks,
Igor Mammedov Feb. 1, 2016, 10:17 a.m. UTC | #10
On Fri, 29 Jan 2016 09:13:15 -0600
Wei Huang <wei@redhat.com> wrote:

> On 01/29/2016 08:50 AM, Peter Maydell wrote:
> > On 29 January 2016 at 14:46, Shannon Zhao <shannon.zhao@linaro.org> wrote:  
> >> On 2016/1/29 22:35, Wei Huang wrote:  
> >>> On 01/29/2016 04:10 AM, Shannon Zhao wrote:  
> >>>> This makes ACPI work well but makes DT not work. The reason is systemd or
> >>>> acpid open /dev/input/event0 failed. So the interrupt could be injected
> >>>> and
> >>>> could see under /proc/interrupts but guest doesn't have any action. I'll
> >>>> investigate why it opens failed later.  
> >>>
> >>>
> >>> That is interesting. Could you try it with the following? This reverses
> >>> the order to down-up and worked on ACPI case.
> >>>  
> >> Yeah, that's very weird.
> >>  
> >>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
> >>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
> >>>  
> >> I'll try this tomorrow. But even if this works, it's still weird.  
> > 
> > I wonder if we should be asserting the GPIO pin in the powerdown-request
> > hook and then deasserting it on system reset somewhere...  
> 
> This is another possibility. We can try to reset the pl061 state by
> hooking up with dc->reset and see what happens.
I think that's what we do on x86.

> 
> > 
> > thanks
> > -- PMM
> >   
>
Wei Huang Feb. 1, 2016, 5:24 p.m. UTC | #11
On 02/01/2016 04:17 AM, Igor Mammedov wrote:
> On Fri, 29 Jan 2016 09:13:15 -0600
> Wei Huang <wei@redhat.com> wrote:
> 
>> On 01/29/2016 08:50 AM, Peter Maydell wrote:
>>> On 29 January 2016 at 14:46, Shannon Zhao <shannon.zhao@linaro.org> wrote:  
>>>> On 2016/1/29 22:35, Wei Huang wrote:  
>>>>> On 01/29/2016 04:10 AM, Shannon Zhao wrote:  
>>>>>> This makes ACPI work well but makes DT not work. The reason is systemd or
>>>>>> acpid open /dev/input/event0 failed. So the interrupt could be injected
>>>>>> and
>>>>>> could see under /proc/interrupts but guest doesn't have any action. I'll
>>>>>> investigate why it opens failed later.  
>>>>>
>>>>>
>>>>> That is interesting. Could you try it with the following? This reverses
>>>>> the order to down-up and worked on ACPI case.
>>>>>  
>>>> Yeah, that's very weird.
>>>>  
>>>>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);
>>>>> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>>>>>  
>>>> I'll try this tomorrow. But even if this works, it's still weird.  
>>>
>>> I wonder if we should be asserting the GPIO pin in the powerdown-request
>>> hook and then deasserting it on system reset somewhere...  
>>
>> This is another possibility. We can try to reset the pl061 state by
>> hooking up with dc->reset and see what happens.
> I think that's what we do on x86.

So the device state is wrong after reset. I just sent out a fix for it,
which was verified on ACPI code path. Basically the old_in_data is stale
after reboot, causing QEMU to believe that it isn't necessary to raise
up IRQ again for the second reboot/shutdown (see s->old_in_data ^
s->data in pl061.c file).

Shannon: could you double check from your side with my patch?

-Wei

> 
>>
>>>
>>> thanks
>>> -- PMM
>>>   
>>
>
Michael Tokarev Feb. 3, 2016, 7:15 a.m. UTC | #12
28.01.2016 21:22, Wei Huang wrote:
> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
> request will succeed; but the following shutdown/reboot requests
> fail to trigger VMs to react. Notice that in mach-virt machine
> model GPIO is defined as edge-triggered and active-high in ACPI.
> This patch changes the behavior of powerdown notifier from PULLUP
> to PULSE. It solves the problem described above (i.e. reboot
> continues to work).

So, what's the outcome of this? :)

Thanks,

/mjt

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 05f9087..b5468a9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -546,7 +546,7 @@ static DeviceState *pl061_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(pl061_dev, 3), 1);
> +    qemu_irq_pulse(qdev_get_gpio_in(pl061_dev, 3));
>  }
>  
>  static Notifier virt_system_powerdown_notifier = {
>
Peter Maydell Feb. 3, 2016, 10:46 a.m. UTC | #13
On 3 February 2016 at 07:15, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 28.01.2016 21:22, Wei Huang wrote:
>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>> request will succeed; but the following shutdown/reboot requests
>> fail to trigger VMs to react. Notice that in mach-virt machine
>> model GPIO is defined as edge-triggered and active-high in ACPI.
>> This patch changes the behavior of powerdown notifier from PULLUP
>> to PULSE. It solves the problem described above (i.e. reboot
>> continues to work).
>
> So, what's the outcome of this? :)

This patch is definitely wrong. The patch to fix up the
gpio reset stuff is definitely the right idea. Whether it
fixes the reported failure or some further change is also
needed is currently unclear.

thanks
-- PMM
Wei Huang Feb. 3, 2016, 4:01 p.m. UTC | #14
On 2/3/16 04:46, Peter Maydell wrote:
> On 3 February 2016 at 07:15, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 28.01.2016 21:22, Wei Huang wrote:
>>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>>> request will succeed; but the following shutdown/reboot requests
>>> fail to trigger VMs to react. Notice that in mach-virt machine
>>> model GPIO is defined as edge-triggered and active-high in ACPI.
>>> This patch changes the behavior of powerdown notifier from PULLUP
>>> to PULSE. It solves the problem described above (i.e. reboot
>>> continues to work).
>>
>> So, what's the outcome of this? :)
> 
> This patch is definitely wrong. The patch to fix up the
> gpio reset stuff is definitely the right idea. Whether it
> fixes the reported failure or some further change is also
> needed is currently unclear.

I will NAK this one for now. Please see V2 patch, which is necessary. In
the meanwhile, I think there is a problem with pulling-up only in
current implementation. Let me debug Shannon's DT problem first.

> 
> thanks
> -- PMM
>
Shannon Zhao Feb. 4, 2016, 1:44 a.m. UTC | #15
On 2016/2/4 0:01, Wei Huang wrote:
> 
> On 2/3/16 04:46, Peter Maydell wrote:
>> > On 3 February 2016 at 07:15, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>> >> 28.01.2016 21:22, Wei Huang wrote:
>>>> >>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>>>> >>> request will succeed; but the following shutdown/reboot requests
>>>> >>> fail to trigger VMs to react. Notice that in mach-virt machine
>>>> >>> model GPIO is defined as edge-triggered and active-high in ACPI.
>>>> >>> This patch changes the behavior of powerdown notifier from PULLUP
>>>> >>> to PULSE. It solves the problem described above (i.e. reboot
>>>> >>> continues to work).
>>> >>
>>> >> So, what's the outcome of this? :)
>> > 
>> > This patch is definitely wrong. The patch to fix up the
>> > gpio reset stuff is definitely the right idea. Whether it
>> > fixes the reported failure or some further change is also
>> > needed is currently unclear.
> I will NAK this one for now. Please see V2 patch, which is necessary. In
> the meanwhile, I think there is a problem with pulling-up only in
> current implementation. Let me debug Shannon's DT problem first.
> 
Hi Wei,

The reason of DT problem is that when we use qemu_irq_pulse(i.e
qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);), it will inject the
GPIO interrupt until it executes
qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0) because the qemu main
thread is serialized and then guest will get the button value as zero,
so it's failed to report the input event.

See gpio_keys_gpio_report_event
 in drivers/input/keyboard/gpio_keys.c
int state = gpio_get_value_cansleep(button->gpio);

The state is always zero.

The solution I think would be making the each
qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1 or 0) cloud inject an
interrupt to guest.

Thanks,
Wei Huang Feb. 4, 2016, 6:10 a.m. UTC | #16
On 02/03/2016 07:44 PM, Shannon Zhao wrote:
> 
> 
> On 2016/2/4 0:01, Wei Huang wrote:
>>
>> On 2/3/16 04:46, Peter Maydell wrote:
>>>> On 3 February 2016 at 07:15, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>>>> 28.01.2016 21:22, Wei Huang wrote:
>>>>>>>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>>>>>>>> request will succeed; but the following shutdown/reboot requests
>>>>>>>> fail to trigger VMs to react. Notice that in mach-virt machine
>>>>>>>> model GPIO is defined as edge-triggered and active-high in ACPI.
>>>>>>>> This patch changes the behavior of powerdown notifier from PULLUP
>>>>>>>> to PULSE. It solves the problem described above (i.e. reboot
>>>>>>>> continues to work).
>>>>>>
>>>>>> So, what's the outcome of this? :)
>>>>
>>>> This patch is definitely wrong. The patch to fix up the
>>>> gpio reset stuff is definitely the right idea. Whether it
>>>> fixes the reported failure or some further change is also
>>>> needed is currently unclear.
>> I will NAK this one for now. Please see V2 patch, which is necessary. In
>> the meanwhile, I think there is a problem with pulling-up only in
>> current implementation. Let me debug Shannon's DT problem first.
>>
> Hi Wei,
> 
> The reason of DT problem is that when we use qemu_irq_pulse(i.e
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);), it will inject the
> GPIO interrupt until it executes
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0) because the qemu main
> thread is serialized and then guest will get the button value as zero,
> so it's failed to report the input event.
> 
> See gpio_keys_gpio_report_event
>  in drivers/input/keyboard/gpio_keys.c
> int state = gpio_get_value_cansleep(button->gpio);
> 
> The state is always zero.

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).

> 
> The solution I think would be making the each
> qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1 or 0) cloud inject an
> interrupt to guest.
> 
> Thanks,
>
Shannon Zhao Feb. 4, 2016, 6:51 a.m. UTC | #17
On 2016/2/4 14:10, Wei Huang wrote:
> 
> On 02/03/2016 07:44 PM, Shannon Zhao wrote:
>> > 
>> > 
>> > On 2016/2/4 0:01, Wei Huang wrote:
>>> >>
>>> >> On 2/3/16 04:46, Peter Maydell wrote:
>>>>> >>>> On 3 February 2016 at 07:15, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>>>>>> >>>>>> 28.01.2016 21:22, Wei Huang wrote:
>>>>>>>>> >>>>>>>> When QEMU is hook'ed up with libvirt/virsh, the first ACPI reboot
>>>>>>>>> >>>>>>>> request will succeed; but the following shutdown/reboot requests
>>>>>>>>> >>>>>>>> fail to trigger VMs to react. Notice that in mach-virt machine
>>>>>>>>> >>>>>>>> model GPIO is defined as edge-triggered and active-high in ACPI.
>>>>>>>>> >>>>>>>> This patch changes the behavior of powerdown notifier from PULLUP
>>>>>>>>> >>>>>>>> to PULSE. It solves the problem described above (i.e. reboot
>>>>>>>>> >>>>>>>> continues to work).
>>>>>>> >>>>>>
>>>>>>> >>>>>> So, what's the outcome of this? :)
>>>>> >>>>
>>>>> >>>> This patch is definitely wrong. The patch to fix up the
>>>>> >>>> gpio reset stuff is definitely the right idea. Whether it
>>>>> >>>> fixes the reported failure or some further change is also
>>>>> >>>> needed is currently unclear.
>>> >> I will NAK this one for now. Please see V2 patch, which is necessary. In
>>> >> the meanwhile, I think there is a problem with pulling-up only in
>>> >> current implementation. Let me debug Shannon's DT problem first.
>>> >>
>> > Hi Wei,
>> > 
>> > The reason of DT problem is that when we use qemu_irq_pulse(i.e
>> > qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
>> > qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0);), it will inject the
>> > GPIO interrupt until it executes
>> > qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 0) because the qemu main
>> > thread is serialized and then guest will get the button value as zero,
>> > so it's failed to report the input event.
>> > 
>> > See gpio_keys_gpio_report_event
>> >  in drivers/input/keyboard/gpio_keys.c
>> > int state = gpio_get_value_cansleep(button->gpio);
>> > 
>> > The state is always zero.
> 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.
diff mbox

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 05f9087..b5468a9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -546,7 +546,7 @@  static DeviceState *pl061_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(pl061_dev, 3), 1);
+    qemu_irq_pulse(qdev_get_gpio_in(pl061_dev, 3));
 }
 
 static Notifier virt_system_powerdown_notifier = {