[Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Wolfgang Mauerer]

The configuration register for the onboard LEDs is
emulated, but the state is not exported, which makes
the feature not particularly useful. Create a character
device to make status changes accessible to the host.

For example, use the command line argument

-chardev socket,id=leds,host=localhost,port=12345,server,nowait

to observe status changes via a socket.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
---
 hw/arm_sysctl.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
index b733617..7cb8559 100644
--- a/hw/arm_sysctl.c
+++ b/hw/arm_sysctl.c
@@ -19,6 +19,7 @@ typedef struct {
     SysBusDevice busdev;
     MemoryRegion iomem;
     qemu_irq pl110_mux_ctrl;
+    CharDriverState *display;
 
     uint32_t sys_id;
     uint32_t leds;
@@ -92,6 +93,24 @@ static void arm_sysctl_reset(DeviceState *d)
     }
 }
 
+static void notify_led_change(CharDriverState *chr, uint32_t old, uint32_t new)
+{
+    uint32_t changed;
+    unsigned int i;
+
+    if (chr == NULL) {
+        return;
+    }
+
+    changed = old ^ new;
+    for (i = 0; i < 8; i++) {
+        if (changed & (1 << i)) {
+            qemu_chr_fe_printf(chr, "%u:%s\r\n", i,
+                               new & (1 << i) ? "on" : "off");
+        }
+    }
+}
+
 static uint64_t arm_sysctl_read(void *opaque, hwaddr offset,
                                 unsigned size)
 {
@@ -198,6 +217,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
 
     switch (offset) {
     case 0x08: /* LED */
+        notify_led_change(s->display, s->leds, (uint32_t)val);
         s->leds = val;
     case 0x0c: /* OSC0 */
     case 0x10: /* OSC1 */
@@ -386,6 +406,10 @@ static int arm_sysctl_init(SysBusDevice *dev)
 {
     arm_sysctl_state *s = FROM_SYSBUS(arm_sysctl_state, dev);
 
+    s->display = qemu_chr_new("leds0", "chardev:leds", NULL);
+    if (s->display) {
+        qemu_chr_fe_printf(s->display, "8 LEDs available\r\n");
+    }
     memory_region_init_io(&s->iomem, &arm_sysctl_ops, s, "arm-sysctl", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
     qdev_init_gpio_in(&s->busdev.qdev, arm_sysctl_gpio_set, 2);
-- 
1.7.1

Re: [Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Andreas Färber]

Hello,

Am 11.01.2013 14:42, schrieb Wolfgang Mauerer:
> The configuration register for the onboard LEDs is
> emulated, but the state is not exported, which makes
> the feature not particularly useful. Create a character
> device to make status changes accessible to the host.
> 
> For example, use the command line argument
> 
> -chardev socket,id=leds,host=localhost,port=12345,server,nowait
> 
> to observe status changes via a socket.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
> ---
>  hw/arm_sysctl.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
> index b733617..7cb8559 100644
> --- a/hw/arm_sysctl.c
> +++ b/hw/arm_sysctl.c
> @@ -19,6 +19,7 @@ typedef struct {
>      SysBusDevice busdev;
>      MemoryRegion iomem;
>      qemu_irq pl110_mux_ctrl;
> +    CharDriverState *display;
>  
>      uint32_t sys_id;
>      uint32_t leds;
> @@ -92,6 +93,24 @@ static void arm_sysctl_reset(DeviceState *d)
>      }
>  }
>  
> +static void notify_led_change(CharDriverState *chr, uint32_t old, uint32_t new)
> +{
> +    uint32_t changed;
> +    unsigned int i;
> +
> +    if (chr == NULL) {
> +        return;
> +    }
> +
> +    changed = old ^ new;
> +    for (i = 0; i < 8; i++) {
> +        if (changed & (1 << i)) {
> +            qemu_chr_fe_printf(chr, "%u:%s\r\n", i,
> +                               new & (1 << i) ? "on" : "off");

Instead of inventing a custom text-based protocol, would exposing a bool
QOM property per LED be an option? What exactly are you trying to do
with the state?

> +        }
> +    }
> +}
> +
>  static uint64_t arm_sysctl_read(void *opaque, hwaddr offset,
>                                  unsigned size)
>  {
> @@ -198,6 +217,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
>  
>      switch (offset) {
>      case 0x08: /* LED */
> +        notify_led_change(s->display, s->leds, (uint32_t)val);
>          s->leds = val;
>      case 0x0c: /* OSC0 */
>      case 0x10: /* OSC1 */
> @@ -386,6 +406,10 @@ static int arm_sysctl_init(SysBusDevice *dev)
>  {
>      arm_sysctl_state *s = FROM_SYSBUS(arm_sysctl_state, dev);
>  
> +    s->display = qemu_chr_new("leds0", "chardev:leds", NULL);
> +    if (s->display) {

Can this ever be NULL?

> +        qemu_chr_fe_printf(s->display, "8 LEDs available\r\n");
> +    }
>      memory_region_init_io(&s->iomem, &arm_sysctl_ops, s, "arm-sysctl", 0x1000);
>      sysbus_init_mmio(dev, &s->iomem);
>      qdev_init_gpio_in(&s->busdev.qdev, arm_sysctl_gpio_set, 2);

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Peter Maydell]

On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote:
> The configuration register for the onboard LEDs is
> emulated, but the state is not exported, which makes
> the feature not particularly useful. Create a character
> device to make status changes accessible to the host.
>
> For example, use the command line argument
>
> -chardev socket,id=leds,host=localhost,port=12345,server,nowait
>
> to observe status changes via a socket.

This isn't the only board we emulate which has LEDs. I'd
rather see this problem tackled with a general plan for "this
is how we handle LEDs in QEMU boards" rather than a
versatile board specific patch.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Andreas Färber]

Am 11.01.2013 16:00, schrieb Peter Maydell:
> On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote:
>> The configuration register for the onboard LEDs is
>> emulated, but the state is not exported, which makes
>> the feature not particularly useful. Create a character
>> device to make status changes accessible to the host.
>>
>> For example, use the command line argument
>>
>> -chardev socket,id=leds,host=localhost,port=12345,server,nowait
>>
>> to observe status changes via a socket.
> 
> This isn't the only board we emulate which has LEDs. I'd
> rather see this problem tackled with a general plan for "this
> is how we handle LEDs in QEMU boards" rather than a
> versatile board specific patch.

Hm, mips_malta.c does use a CharDriverState... just a more "structured"
one using ASCII art.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Wolfgang Mauerer]

On 11/01/13 16:23, Andreas Färber wrote:
> Am 11.01.2013 16:00, schrieb Peter Maydell:
>> On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote:
>>> The configuration register for the onboard LEDs is
>>> emulated, but the state is not exported, which makes
>>> the feature not particularly useful. Create a character
>>> device to make status changes accessible to the host.
>>>
>>> For example, use the command line argument
>>>
>>> -chardev socket,id=leds,host=localhost,port=12345,server,nowait
>>>
>>> to observe status changes via a socket.
>>
>> This isn't the only board we emulate which has LEDs. I'd
>> rather see this problem tackled with a general plan for "this
>> is how we handle LEDs in QEMU boards" rather than a
>> versatile board specific patch.
> 
> Hm, mips_malta.c does use a CharDriverState... just a more "structured"
> one using ASCII art.

that's one of the reaons why I used a character device to export the
status information.

However, a simple custom protocol (or maybe the same information in
JSON) seemed more appripriate since I assume that graphical frontends
will eventually visualise the LED status. Having to parse some ASCII
art output for this seems suboptimal.

Exporting a binary QOM property (as you suggested in another email)
might be an option, although I'm not really sure if LED status changes
should be communicated this way -- to me, using a chardev feels nicer.
Do the QMP maintainers have an opinion?

Regards, Wolfgang

Re: [Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Wolfgang Mauerer]

Hello,

On 11/01/13 15:58, Andreas Färber wrote:
>> @@ -386,6 +406,10 @@ static int arm_sysctl_init(SysBusDevice *dev)
>> >  {
>> >      arm_sysctl_state *s = FROM_SYSBUS(arm_sysctl_state, dev);
>> >  
>> > +    s->display = qemu_chr_new("leds0", "chardev:leds", NULL);
>> > +    if (s->display) {
> Can this ever be NULL?

if can be NULL if no leds chardev is available.

Best, Wolfgang

Re: [Qemu-devel] [PATCH] Realview/Versatile: Export LED state

[Luiz Capitulino]

On Fri, 11 Jan 2013 16:44:49 +0100
Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote:

> On 11/01/13 16:23, Andreas Färber wrote:
> > Am 11.01.2013 16:00, schrieb Peter Maydell:
> >> On 11 January 2013 13:42, Wolfgang Mauerer <wolfgang.mauerer@siemens.com> wrote:
> >>> The configuration register for the onboard LEDs is
> >>> emulated, but the state is not exported, which makes
> >>> the feature not particularly useful. Create a character
> >>> device to make status changes accessible to the host.
> >>>
> >>> For example, use the command line argument
> >>>
> >>> -chardev socket,id=leds,host=localhost,port=12345,server,nowait
> >>>
> >>> to observe status changes via a socket.
> >>
> >> This isn't the only board we emulate which has LEDs. I'd
> >> rather see this problem tackled with a general plan for "this
> >> is how we handle LEDs in QEMU boards" rather than a
> >> versatile board specific patch.
> > 
> > Hm, mips_malta.c does use a CharDriverState... just a more "structured"
> > one using ASCII art.
> 
> that's one of the reaons why I used a character device to export the
> status information.
> 
> However, a simple custom protocol (or maybe the same information in
> JSON) seemed more appripriate since I assume that graphical frontends
> will eventually visualise the LED status. Having to parse some ASCII
> art output for this seems suboptimal.
> 
> Exporting a binary QOM property (as you suggested in another email)
> might be an option, although I'm not really sure if LED status changes
> should be communicated this way -- to me, using a chardev feels nicer.
> Do the QMP maintainers have an opinion?

I'm not completely sure I understand the problem here, but if this is
some kind of status to be queried then exporting it through a QOM
property seems the right thing to do.

If you go for a custom protocol, then using QMP syntax might be a good
idea, as you can use our infra and clients which already know QMP will
also appreciate this (this is what qemu-ga did, btw).