* [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse @ 2016-01-28 18:22 Wei Huang 2016-01-29 10:10 ` Shannon Zhao 2016-02-03 7:15 ` Michael Tokarev 0 siblings, 2 replies; 28+ messages in thread From: Wei Huang @ 2016-01-28 18:22 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-trivial, peter.maydell, shannon.zhao, zhaoshenglong 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(-) 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-28 18:22 [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse Wei Huang @ 2016-01-29 10:10 ` Shannon Zhao 2016-01-29 14:35 ` Wei Huang 2016-02-03 7:15 ` Michael Tokarev 1 sibling, 1 reply; 28+ messages in thread From: Shannon Zhao @ 2016-01-29 10:10 UTC (permalink / raw) To: Wei Huang; +Cc: qemu-trivial, peter.maydell, qemu-devel, zhaoshenglong [-- Attachment #1: Type: text/plain, Size: 1419 bytes --] 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 > > [-- Attachment #2: Type: text/html, Size: 1780 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 10:10 ` Shannon Zhao @ 2016-01-29 14:35 ` Wei Huang 2016-01-29 14:46 ` Shannon Zhao 2016-01-30 8:18 ` Shannon Zhao 0 siblings, 2 replies; 28+ messages in thread From: Wei Huang @ 2016-01-29 14:35 UTC (permalink / raw) To: Shannon Zhao; +Cc: qemu-trivial, peter.maydell, qemu-devel, zhaoshenglong 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 >> >> > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 14:35 ` Wei Huang @ 2016-01-29 14:46 ` Shannon Zhao 2016-01-29 14:50 ` Wei Huang 2016-01-29 14:50 ` Peter Maydell 2016-01-30 8:18 ` Shannon Zhao 1 sibling, 2 replies; 28+ messages in thread From: Shannon Zhao @ 2016-01-29 14:46 UTC (permalink / raw) To: Wei Huang; +Cc: qemu-trivial, peter.maydell, qemu-devel, zhaoshenglong 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 >>> >>> >> -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 14:46 ` Shannon Zhao @ 2016-01-29 14:50 ` Wei Huang 2016-01-29 15:22 ` Shannon Zhao 2016-01-29 14:50 ` Peter Maydell 1 sibling, 1 reply; 28+ messages in thread From: Wei Huang @ 2016-01-29 14:50 UTC (permalink / raw) To: Shannon Zhao; +Cc: qemu-trivial, peter.maydell, qemu-devel, zhaoshenglong 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 >>>> >>>> >>> > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 14:50 ` Wei Huang @ 2016-01-29 15:22 ` Shannon Zhao 0 siblings, 0 replies; 28+ messages in thread From: Shannon Zhao @ 2016-01-29 15:22 UTC (permalink / raw) To: Wei Huang; +Cc: qemu-trivial, peter.maydell, qemu-devel, zhaoshenglong 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, -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 14:46 ` Shannon Zhao 2016-01-29 14:50 ` Wei Huang @ 2016-01-29 14:50 ` Peter Maydell 2016-01-29 15:13 ` Wei Huang 1 sibling, 1 reply; 28+ messages in thread From: Peter Maydell @ 2016-01-29 14:50 UTC (permalink / raw) To: Shannon Zhao; +Cc: Wei Huang, qemu-trivial, qemu-devel, zhaoshenglong 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 14:50 ` Peter Maydell @ 2016-01-29 15:13 ` Wei Huang 2016-01-29 15:29 ` Peter Maydell 2016-02-01 10:17 ` Igor Mammedov 0 siblings, 2 replies; 28+ messages in thread From: Wei Huang @ 2016-01-29 15:13 UTC (permalink / raw) To: Peter Maydell, Shannon Zhao; +Cc: qemu-trivial, qemu-devel, zhaoshenglong 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 15:13 ` Wei Huang @ 2016-01-29 15:29 ` Peter Maydell 2016-02-01 10:17 ` Igor Mammedov 1 sibling, 0 replies; 28+ messages in thread From: Peter Maydell @ 2016-01-29 15:29 UTC (permalink / raw) To: Wei Huang; +Cc: qemu-trivial, zhaoshenglong, Shannon Zhao, qemu-devel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 15:13 ` Wei Huang 2016-01-29 15:29 ` Peter Maydell @ 2016-02-01 10:17 ` Igor Mammedov 2016-02-01 17:24 ` Wei Huang 1 sibling, 1 reply; 28+ messages in thread From: Igor Mammedov @ 2016-02-01 10:17 UTC (permalink / raw) To: Wei Huang Cc: qemu-trivial, Peter Maydell, zhaoshenglong, Shannon Zhao, qemu-devel 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 > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-01 10:17 ` Igor Mammedov @ 2016-02-01 17:24 ` Wei Huang 0 siblings, 0 replies; 28+ messages in thread From: Wei Huang @ 2016-02-01 17:24 UTC (permalink / raw) To: Igor Mammedov Cc: qemu-trivial, Peter Maydell, zhaoshenglong, Shannon Zhao, qemu-devel 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 >>> >> > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-29 14:35 ` Wei Huang 2016-01-29 14:46 ` Shannon Zhao @ 2016-01-30 8:18 ` Shannon Zhao 1 sibling, 0 replies; 28+ messages in thread From: Shannon Zhao @ 2016-01-30 8:18 UTC (permalink / raw) To: Wei Huang, Shannon Zhao; +Cc: qemu-trivial, peter.maydell, qemu-devel 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, -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-01-28 18:22 [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse Wei Huang 2016-01-29 10:10 ` Shannon Zhao @ 2016-02-03 7:15 ` Michael Tokarev 2016-02-03 10:46 ` Peter Maydell 1 sibling, 1 reply; 28+ messages in thread From: Michael Tokarev @ 2016-02-03 7:15 UTC (permalink / raw) To: Wei Huang, qemu-devel Cc: qemu-trivial, peter.maydell, shannon.zhao, zhaoshenglong 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 = { > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-03 7:15 ` Michael Tokarev @ 2016-02-03 10:46 ` Peter Maydell 2016-02-03 16:01 ` Wei Huang 0 siblings, 1 reply; 28+ messages in thread From: Peter Maydell @ 2016-02-03 10:46 UTC (permalink / raw) To: Michael Tokarev Cc: Wei Huang, QEMU Trivial, Shannon Zhao, QEMU Developers, Shannon Zhao 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-03 10:46 ` Peter Maydell @ 2016-02-03 16:01 ` Wei Huang 2016-02-04 1:44 ` Shannon Zhao 0 siblings, 1 reply; 28+ messages in thread From: Wei Huang @ 2016-02-03 16:01 UTC (permalink / raw) To: Peter Maydell, Michael Tokarev Cc: QEMU Trivial, Shannon Zhao, QEMU Developers, Shannon Zhao 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-03 16:01 ` Wei Huang @ 2016-02-04 1:44 ` Shannon Zhao 2016-02-04 6:10 ` Wei Huang 0 siblings, 1 reply; 28+ messages in thread From: Shannon Zhao @ 2016-02-04 1:44 UTC (permalink / raw) To: Wei Huang, Peter Maydell, Michael Tokarev Cc: QEMU Trivial, QEMU Developers, Shannon Zhao 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, -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-04 1:44 ` Shannon Zhao @ 2016-02-04 6:10 ` Wei Huang 2016-02-04 6:51 ` Shannon Zhao 0 siblings, 1 reply; 28+ messages in thread From: Wei Huang @ 2016-02-04 6:10 UTC (permalink / raw) To: Shannon Zhao, Peter Maydell, Michael Tokarev Cc: QEMU Trivial, QEMU Developers, Shannon Zhao 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, > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-04 6:10 ` Wei Huang @ 2016-02-04 6:51 ` Shannon Zhao 2016-02-09 22:59 ` Wei Huang 0 siblings, 1 reply; 28+ messages in thread From: Shannon Zhao @ 2016-02-04 6:51 UTC (permalink / raw) To: Wei Huang, Peter Maydell, Michael Tokarev Cc: QEMU Trivial, QEMU Developers, Shannon Zhao 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. -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-04 6:51 ` Shannon Zhao @ 2016-02-09 22:59 ` Wei Huang 2016-02-20 10:53 ` Shannon Zhao 0 siblings, 1 reply; 28+ messages in thread From: Wei Huang @ 2016-02-09 22:59 UTC (permalink / raw) To: Shannon Zhao, Peter Maydell, Michael Tokarev Cc: QEMU Trivial, QEMU Developers, Shannon Zhao [-- Attachment #1: Type: text/plain, Size: 3720 bytes --] 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 > [-- Attachment #2: gpio-fix.txt --] [-- Type: text/plain, Size: 1280 bytes --] 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 = { ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-09 22:59 ` Wei Huang @ 2016-02-20 10:53 ` Shannon Zhao 2016-02-24 22:22 ` Wei Huang 0 siblings, 1 reply; 28+ messages in thread From: Shannon Zhao @ 2016-02-20 10:53 UTC (permalink / raw) To: Wei Huang, Peter Maydell, Michael Tokarev Cc: QEMU Trivial, QEMU Developers, Shannon Zhao 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, -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-20 10:53 ` Shannon Zhao @ 2016-02-24 22:22 ` Wei Huang 2016-02-26 12:31 ` Shannon Zhao 0 siblings, 1 reply; 28+ messages in thread From: Wei Huang @ 2016-02-24 22:22 UTC (permalink / raw) To: Shannon Zhao, Peter Maydell, Michael Tokarev Cc: QEMU Trivial, QEMU Developers, Shannon Zhao 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, > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-24 22:22 ` Wei Huang @ 2016-02-26 12:31 ` Shannon Zhao 2016-02-26 12:53 ` Peter Maydell 0 siblings, 1 reply; 28+ messages in thread From: Shannon Zhao @ 2016-02-26 12:31 UTC (permalink / raw) To: Wei Huang, Peter Maydell Cc: QEMU Trivial, Michael Tokarev, QEMU Developers, Shannon Zhao 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. 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); } 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, -- Shannon ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-26 12:31 ` Shannon Zhao @ 2016-02-26 12:53 ` Peter Maydell 2016-02-26 14:54 ` Shannon Zhao 0 siblings, 1 reply; 28+ messages in thread From: Peter Maydell @ 2016-02-26 12:53 UTC (permalink / raw) To: Shannon Zhao Cc: Wei Huang, QEMU Trivial, Michael Tokarev, QEMU Developers, Shannon Zhao 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-26 12:53 ` Peter Maydell @ 2016-02-26 14:54 ` Shannon Zhao 2016-02-26 15:06 ` Peter Maydell 0 siblings, 1 reply; 28+ messages in thread From: Shannon Zhao @ 2016-02-26 14:54 UTC (permalink / raw) To: Peter Maydell, Shannon Zhao Cc: Wei Huang, QEMU Trivial, Michael Tokarev, QEMU Developers 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, -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-26 14:54 ` Shannon Zhao @ 2016-02-26 15:06 ` Peter Maydell 2016-02-26 15:28 ` Wei Huang 0 siblings, 1 reply; 28+ messages in thread From: Peter Maydell @ 2016-02-26 15:06 UTC (permalink / raw) To: Shannon Zhao Cc: Wei Huang, QEMU Trivial, Michael Tokarev, QEMU Developers, Shannon Zhao 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-26 15:06 ` Peter Maydell @ 2016-02-26 15:28 ` Wei Huang 2016-02-26 15:42 ` Peter Maydell 0 siblings, 1 reply; 28+ messages in thread From: Wei Huang @ 2016-02-26 15:28 UTC (permalink / raw) To: Peter Maydell, Shannon Zhao Cc: QEMU Trivial, Michael Tokarev, QEMU Developers, Shannon Zhao 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 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-26 15:28 ` Wei Huang @ 2016-02-26 15:42 ` Peter Maydell 2016-02-27 1:55 ` Shannon Zhao 0 siblings, 1 reply; 28+ messages in thread From: Peter Maydell @ 2016-02-26 15:42 UTC (permalink / raw) To: Wei Huang Cc: QEMU Trivial, QEMU Developers, Michael Tokarev, Shannon Zhao, Shannon Zhao 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse 2016-02-26 15:42 ` Peter Maydell @ 2016-02-27 1:55 ` Shannon Zhao 0 siblings, 0 replies; 28+ messages in thread From: Shannon Zhao @ 2016-02-27 1:55 UTC (permalink / raw) To: Peter Maydell, Wei Huang Cc: QEMU Trivial, Michael Tokarev, Shannon Zhao, QEMU Developers 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, -- Shannon ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2016-02-27 1:56 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-28 18:22 [Qemu-devel] [PATCH 1/1] arm: virt: change GPIO trigger interrupt to pulse Wei Huang 2016-01-29 10:10 ` Shannon Zhao 2016-01-29 14:35 ` Wei Huang 2016-01-29 14:46 ` Shannon Zhao 2016-01-29 14:50 ` Wei Huang 2016-01-29 15:22 ` Shannon Zhao 2016-01-29 14:50 ` Peter Maydell 2016-01-29 15:13 ` Wei Huang 2016-01-29 15:29 ` Peter Maydell 2016-02-01 10:17 ` Igor Mammedov 2016-02-01 17:24 ` Wei Huang 2016-01-30 8:18 ` Shannon Zhao 2016-02-03 7:15 ` Michael Tokarev 2016-02-03 10:46 ` Peter Maydell 2016-02-03 16:01 ` Wei Huang 2016-02-04 1:44 ` Shannon Zhao 2016-02-04 6:10 ` Wei Huang 2016-02-04 6:51 ` Shannon Zhao 2016-02-09 22:59 ` Wei Huang 2016-02-20 10:53 ` Shannon Zhao 2016-02-24 22:22 ` Wei Huang 2016-02-26 12:31 ` Shannon Zhao 2016-02-26 12:53 ` Peter Maydell 2016-02-26 14:54 ` Shannon Zhao 2016-02-26 15:06 ` Peter Maydell 2016-02-26 15:28 ` Wei Huang 2016-02-26 15:42 ` Peter Maydell 2016-02-27 1:55 ` Shannon Zhao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).