[RFC PATCH 0/5] hw/misc: Add LED device

[RFC PATCH 0/5] hw/misc: Add LED device

[Philippe Mathieu-Daudé]

Hello,

These patches are part of the GSoC unselected 'QEMU visualizer'
project.  As the AVR port is not merged, I switched to microbit
to keep working on it.

This series presents a proof of concept of LED device that can
be easily connected to a GPIO.
The LED emit QMP events, so an external visualizer can display
the LED events.

If there is no negative opinion on this series, next step will
be add Zephyr test for the microbit, then we'll work on LED
array/matrix.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw/misc: Add a LED device
  hw/misc/led: Add LED_STATUS_CHANGED QAPI event
  hw/misc/led: Add create_led_by_gpio_id() helper
  hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr
  hw/arm/tosa: Use LED device for the Bluetooth led

 qapi/led.json         |  47 ++++++++++++++++++
 qapi/qapi-schema.json |   1 +
 include/hw/misc/led.h |  44 +++++++++++++++++
 hw/arm/microbit.c     |   3 ++
 hw/arm/tosa.c         |   7 ++-
 hw/misc/led.c         | 108 ++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS           |   7 +++
 hw/arm/Kconfig        |   2 +
 hw/misc/Kconfig       |   3 ++
 hw/misc/Makefile.objs |   1 +
 hw/misc/trace-events  |   3 ++
 qapi/Makefile.objs    |   2 +-
 12 files changed, 223 insertions(+), 5 deletions(-)
 create mode 100644 qapi/led.json
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

-- 
2.21.3

[RFC PATCH 1/5] hw/misc: Add a LED device

[Philippe Mathieu-Daudé]

A LED device can be connected to a GPIO output.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h | 30 ++++++++++++++++
 hw/misc/led.c         | 84 +++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS           |  6 ++++
 hw/misc/Kconfig       |  3 ++
 hw/misc/Makefile.objs |  1 +
 hw/misc/trace-events  |  3 ++
 6 files changed, 127 insertions(+)
 create mode 100644 include/hw/misc/led.h
 create mode 100644 hw/misc/led.c

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
new file mode 100644
index 0000000000..427ca1418e
--- /dev/null
+++ b/include/hw/misc/led.h
@@ -0,0 +1,30 @@
+/*
+ * QEMU single LED device
+ *
+ * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_MISC_LED_H
+#define HW_MISC_LED_H
+
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h" /* FIXME remove */
+
+#define TYPE_LED "led"
+#define LED(obj) OBJECT_CHECK(LEDState, (obj), TYPE_LED)
+
+typedef struct LEDState {
+    /* Private */
+    SysBusDevice parent_obj; /* FIXME DeviceState */
+    /* Public */
+
+    qemu_irq irq;
+    uint8_t current_state;
+
+    /* Properties */
+    char *name;
+    uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
+} LEDState;
+
+#endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
new file mode 100644
index 0000000000..dc61b11017
--- /dev/null
+++ b/hw/misc/led.c
@@ -0,0 +1,84 @@
+/*
+ * QEMU single LED device
+ *
+ * Copyright (C) 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/led.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "trace.h"
+
+static void led_set(void *opaque, int line, int new_state)
+{
+    LEDState *s = LED(opaque);
+
+    trace_led_set(s->name, s->current_state, new_state);
+
+    s->current_state = new_state;
+}
+
+static void led_reset(DeviceState *dev)
+{
+    LEDState *s = LED(dev);
+
+    s->current_state = s->reset_state;
+}
+
+static const VMStateDescription vmstate_led = {
+    .name = TYPE_LED,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(reset_state, LEDState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void led_realize(DeviceState *dev, Error **errp)
+{
+    LEDState *s = LED(dev);
+
+    if (s->name == NULL) {
+        error_setg(errp, "property 'name' not specified");
+        return;
+    }
+
+    qdev_init_gpio_in(DEVICE(s), led_set, 1);
+}
+
+static Property led_properties[] = {
+    DEFINE_PROP_STRING("name", LEDState, name),
+    DEFINE_PROP_UINT8("reset_state", LEDState, reset_state, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void led_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "LED";
+    dc->vmsd = &vmstate_led;
+    dc->reset = led_reset;
+    dc->realize = led_realize;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    device_class_set_props(dc, led_properties);
+}
+
+static const TypeInfo led_info = {
+    .name = TYPE_LED,
+    .parent = TYPE_SYS_BUS_DEVICE, /* FIXME TYPE_DEVICE */
+    .instance_size = sizeof(LEDState),
+    .class_init = led_class_init
+};
+
+static void led_register_types(void)
+{
+    type_register_static(&led_info);
+}
+
+type_init(led_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e7890ce82..f873f0b94a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1864,6 +1864,12 @@ S: Maintained
 F: include/hw/misc/unimp.h
 F: hw/misc/unimp.c
 
+LED
+M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+S: Maintained
+F: include/hw/misc/led.h
+F: hw/misc/led.c
+
 Standard VGA
 M: Gerd Hoffmann <kraxel@redhat.com>
 S: Maintained
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index bdd77d8020..f60dce694d 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -126,6 +126,9 @@ config AUX
 config UNIMP
     bool
 
+config LED
+    bool
+
 config MAC_VIA
     bool
     select MOS6522
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 60a9d80b74..aae07033f9 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -90,3 +90,4 @@ common-obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
 obj-$(CONFIG_MAC_VIA) += mac_via.o
 
 common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
+common-obj-$(CONFIG_LED) += led.o
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index a5862b2bed..55d7143f23 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -198,3 +198,6 @@ via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
 via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
 via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
 via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
+
+# led.c
+led_set(const char *name, uint8_t old_state, uint8_t new_state) "led name:'%s' state %d -> %d"
-- 
2.21.3

[RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Philippe Mathieu-Daudé]

Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.

QMP event examples:

{
    "timestamp": {
        "seconds": 1591704274,
        "microseconds": 520850
    },
    "event": "LED_STATUS_CHANGED",
    "data": {
        "name": "Green LED #0",
        "status": "on"
    }
}
{
    "timestamp": {
        "seconds": 1591704275,
        "microseconds": 530912
    },
    "event": "LED_STATUS_CHANGED",
    "data": {
        "name": "Green LED #0",
        "status": "off"
    }
}

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 qapi/led.json         | 47 +++++++++++++++++++++++++++++++++++++++++++
 qapi/qapi-schema.json |  1 +
 hw/misc/led.c         |  4 ++++
 MAINTAINERS           |  1 +
 qapi/Makefile.objs    |  2 +-
 5 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 qapi/led.json

diff --git a/qapi/led.json b/qapi/led.json
new file mode 100644
index 0000000000..b6cef8a5dd
--- /dev/null
+++ b/qapi/led.json
@@ -0,0 +1,47 @@
+# -*- Mode: Python -*-
+#
+
+##
+# = LED device
+##
+
+##
+# @LedState:
+#
+# Status of a LED
+#
+# @on: device is emitting
+#
+# @off: device is off
+#
+# Since: 5.1
+##
+{ 'enum': 'LedState', 'data': [ 'on', 'off' ] }
+
+##
+# @LED_STATUS_CHANGED:
+#
+# Emitted when LED status changed
+#
+# @name: LED description
+#
+# @status: New status
+#
+# Since: 5.1
+#
+# Example:
+#
+# <- {"timestamp": {"seconds": 1541579657, "microseconds": 986760},
+#     "event": "LED_STATUS_CHANGED",
+#     "data":
+#         {"name": "Blue LED #3",
+#          "status": "on"
+#         }
+#    }
+#
+##
+{ 'event': 'LED_STATUS_CHANGED',
+  'data': { 'name'      : 'str',
+            'status'    : 'LedState'
+          }
+}
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 43b0ba0dea..6f3ffc0ae1 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -84,3 +84,4 @@
 { 'include': 'misc.json' }
 { 'include': 'misc-target.json' }
 { 'include': 'audio.json' }
+{ 'include': 'led.json' }
diff --git a/hw/misc/led.c b/hw/misc/led.c
index dc61b11017..233843f581 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -7,6 +7,7 @@
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-led.h"
 #include "hw/qdev-properties.h"
 #include "hw/misc/led.h"
 #include "hw/irq.h"
@@ -19,6 +20,9 @@ static void led_set(void *opaque, int line, int new_state)
 
     trace_led_set(s->name, s->current_state, new_state);
 
+    /* FIXME QMP rate limite? */
+    qapi_event_send_led_status_changed(s->name, new_state
+                                                ? LED_STATE_ON : LED_STATE_OFF);
     s->current_state = new_state;
 }
 
diff --git a/MAINTAINERS b/MAINTAINERS
index f873f0b94a..9ff84498fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1867,6 +1867,7 @@ F: hw/misc/unimp.c
 LED
 M: Philippe Mathieu-Daudé <f4bug@amsat.org>
 S: Maintained
+F: qapi/led.json
 F: include/hw/misc/led.h
 F: hw/misc/led.c
 
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 4673ab7490..e9f6570c32 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -6,7 +6,7 @@ util-obj-y += qmp-event.o
 util-obj-y += qapi-util.o
 
 QAPI_COMMON_MODULES = audio authz block-core block char common control crypto
-QAPI_COMMON_MODULES += dump error introspect job machine migration misc
+QAPI_COMMON_MODULES += dump error introspect job led machine migration misc
 QAPI_COMMON_MODULES += net pragma qdev qom rdma rocker run-state sockets tpm
 QAPI_COMMON_MODULES += trace transaction ui
 QAPI_TARGET_MODULES = machine-target misc-target
-- 
2.21.3

[RFC PATCH 3/5] hw/misc/led: Add create_led_by_gpio_id() helper

[Philippe Mathieu-Daudé]

Add create_led_by_gpio_id() to easily connect a LED to
a GPIO output.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h | 14 ++++++++++++++
 hw/misc/led.c         | 20 ++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index 427ca1418e..58393c16de 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -27,4 +27,18 @@ typedef struct LEDState {
     uint8_t reset_state; /* TODO [GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH] */
 } LEDState;
 
+/**
+ * create_led_by_gpio_id: create and LED device
+ * @parent: the parent object
+ * @gpio_dev: device exporting GPIOs
+ * @gpio_id: GPIO ID of this LED
+ * @name: name of the LED
+ *
+ * This utility function creates a LED and connects it to a
+ * GPIO exported by another device.
+ */
+DeviceState *create_led_by_gpio_id(Object *parentobj,
+                                   DeviceState *gpio_dev, unsigned gpio_id,
+                                   const char *led_name);
+
 #endif /* HW_MISC_LED_H */
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 233843f581..27882e2f9d 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -86,3 +86,23 @@ static void led_register_types(void)
 }
 
 type_init(led_register_types)
+
+DeviceState *create_led_by_gpio_id(Object *parentobj,
+                                   DeviceState *gpio_dev, unsigned gpio_id,
+                                   const char *led_name)
+{
+    DeviceState *dev;
+    char *name;
+
+    dev = qdev_create(NULL, TYPE_LED);
+    /* TODO set "reset_state" */
+    qdev_prop_set_string(dev, "name", led_name);
+    name = g_ascii_strdown(led_name, -1);
+    name = g_strdelimit(name, " #", '-');
+    object_property_add_child(parentobj, name, OBJECT(dev));
+    g_free(name);
+    qdev_init_nofail(dev);
+    qdev_connect_gpio_out(gpio_dev, gpio_id, qdev_get_gpio_in(dev, 0));
+
+    return dev;
+}
-- 
2.21.3

[RFC PATCH 4/5] hw/arm/microbit: Add a fake LED to use as proof-of-concept with Zephyr

[Philippe Mathieu-Daudé]

We were using an AVR based Arduino to use this device, but since
the port is not merged, the microbit is the easiest board to use
with Zephyr.
Note the microbit doesn't have a such LED, this is simply a proof
of concept.

How to test:

- Apply this patch on zephyr-v2.3.0

  diff --git a/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts b/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
  index a1b3044275..61b39506b1 100644
  --- a/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
  +++ b/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
  @@ -21,6 +21,18 @@
                  zephyr,flash = &flash0;
                  zephyr,code-partition = &slot0_partition;
          };
  +
  +       leds {
  +               compatible = "gpio-leds";
  +               led0: led_0 {
  +                       gpios = <&gpio0 21 GPIO_ACTIVE_LOW>;
  +                       label = "Green LED 0";
  +               };
  +       };
  +
  +       aliases {
  +               led0 = &led0;
  +       };
   };

   &gpiote {

- Build Zephyr blinky:

  $ west build -b qemu_cortex_m0 samples/basic/blinky

- Run QEMU

  $ qemu-system-arm -M microbit -trace led\* \
      -kernel ~/zephyrproject/zephyr/build/zephyr/zephyr.elf -trace led\*
  2953@1591704866.319665:led_set led name:'Green LED #0' state 0 -> 0
  2953@1591704867.329143:led_set led name:'Green LED #0' state 0 -> 1
  2953@1591704868.332590:led_set led name:'Green LED #0' state 1 -> 0

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/microbit.c | 3 +++
 hw/arm/Kconfig    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index ef213695bd..102661b66a 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -18,6 +18,7 @@
 #include "hw/arm/nrf51_soc.h"
 #include "hw/i2c/microbit_i2c.h"
 #include "hw/qdev-properties.h"
+#include "hw/misc/led.h"
 
 typedef struct {
     MachineState parent;
@@ -58,6 +59,8 @@ static void microbit_init(MachineState *machine)
     memory_region_add_subregion_overlap(&s->nrf51.container, NRF51_TWI_BASE,
                                         mr, -1);
 
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(soc), 21, "Green LED #0");
+
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
                        NRF51_SOC(soc)->flash_size);
 }
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 9afa6eee79..2afaa7c8e9 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -436,6 +436,7 @@ config FSL_IMX6UL
 config MICROBIT
     bool
     select NRF51_SOC
+    select LED
 
 config NRF51_SOC
     bool
-- 
2.21.3

[RFC PATCH 5/5] hw/arm/tosa: Use LED device for the Bluetooth led

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/tosa.c  | 7 +++----
 hw/arm/Kconfig | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 5dee2d76c6..86d7e0283a 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -24,6 +24,7 @@
 #include "hw/irq.h"
 #include "hw/ssi/ssi.h"
 #include "hw/sysbus.h"
+#include "hw/misc/led.h"
 #include "exec/address-spaces.h"
 
 #define TOSA_RAM    0x04000000
@@ -68,9 +69,6 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
 static void tosa_out_switch(void *opaque, int line, int level)
 {
     switch (line) {
-        case 0:
-            fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
-            break;
         case 1:
             fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
             break;
@@ -119,7 +117,6 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
                         qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ),
                         NULL);
 
-    qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]);
     qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]);
     qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]);
     qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]);
@@ -234,6 +231,8 @@ static void tosa_init(MachineState *machine)
 
     scp0 = sysbus_create_simple("scoop", 0x08800000, NULL);
     scp1 = sysbus_create_simple("scoop", 0x14800040, NULL);
+    create_led_by_gpio_id(OBJECT(machine), DEVICE(scp1),
+                          TOSA_GPIO_BT_LED, "blue LED");
 
     tosa_gpio_setup(mpu, scp0, scp1, tmio);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 2afaa7c8e9..009336cac8 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -150,6 +150,7 @@ config TOSA
     select ZAURUS  # scoop
     select MICRODRIVE
     select PXA2XX
+    select LED
 
 config SPITZ
     bool
-- 
2.21.3

Re: [RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Eric Blake]

On 6/9/20 7:34 AM, Philippe Mathieu-Daudé wrote:
> Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.
> 
> QMP event examples:
> 
> {
>      "timestamp": {
>          "seconds": 1591704274,
>          "microseconds": 520850
>      },
>      "event": "LED_STATUS_CHANGED",
>      "data": {
>          "name": "Green LED #0",
>          "status": "on"
>      }
> }
> {
>      "timestamp": {
>          "seconds": 1591704275,
>          "microseconds": 530912
>      },
>      "event": "LED_STATUS_CHANGED",
>      "data": {
>          "name": "Green LED #0",
>          "status": "off"
>      }
> }
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

The QAPI addition looks reasonable, however,

> +++ b/hw/misc/led.c
> @@ -7,6 +7,7 @@
>    */
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
> +#include "qapi/qapi-events-led.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/misc/led.h"
>   #include "hw/irq.h"
> @@ -19,6 +20,9 @@ static void led_set(void *opaque, int line, int new_state)
>   
>       trace_led_set(s->name, s->current_state, new_state);
>   
> +    /* FIXME QMP rate limite? */

s/limite/limit/

Yes, this is under guest control, so you MUST rate limit to avoid the 
guest being able to DoS qemu by changing the LED so frequently as to 
overwhelm the QMP connection with events.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Philippe Mathieu-Daudé]

Hi Eric,

On 6/9/20 4:29 PM, Eric Blake wrote:
> On 6/9/20 7:34 AM, Philippe Mathieu-Daudé wrote:
>> Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.
>>
>> QMP event examples:
>>
>> {
>>      "timestamp": {
>>          "seconds": 1591704274,
>>          "microseconds": 520850
>>      },
>>      "event": "LED_STATUS_CHANGED",
>>      "data": {
>>          "name": "Green LED #0",
>>          "status": "on"
>>      }
>> }
>> {
>>      "timestamp": {
>>          "seconds": 1591704275,
>>          "microseconds": 530912
>>      },
>>      "event": "LED_STATUS_CHANGED",
>>      "data": {
>>          "name": "Green LED #0",
>>          "status": "off"
>>      }
>> }
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
> The QAPI addition looks reasonable, however,
> 
>> +++ b/hw/misc/led.c
>> @@ -7,6 +7,7 @@
>>    */
>>   #include "qemu/osdep.h"
>>   #include "qapi/error.h"
>> +#include "qapi/qapi-events-led.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/misc/led.h"
>>   #include "hw/irq.h"
>> @@ -19,6 +20,9 @@ static void led_set(void *opaque, int line, int
>> new_state)
>>         trace_led_set(s->name, s->current_state, new_state);
>>   +    /* FIXME QMP rate limite? */
> 
> s/limite/limit/
> 
> Yes, this is under guest control, so you MUST rate limit to avoid the
> guest being able to DoS qemu by changing the LED so frequently as to
> overwhelm the QMP connection with events.

Commits f544d174dfc and 7f1e7b23d5 refers to the qmp-events.txt
for documentation on rate-limiting QMP events, but I can't find
it in the codebase. Two files matches 'qmp-events' but don't have
documentation: qapi/qmp-event.c and include/qapi/qmp-event.h.

Last trace of it is in commit 231aaf3a8217. Apparently it was
somehow split qapi/event.json, then later c09656f1d392 move it
to qapi-schema.json, finally eb815e248f50 moved it to qapi/.

Is the referred documentation now in docs/devel/qapi-code-gen.txt?
There is only one occurence of 'limit' but it is unrelated to
rate-limit.

Thanks,

Phil.

Re: [RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi Eric,
>
> On 6/9/20 4:29 PM, Eric Blake wrote:
>> On 6/9/20 7:34 AM, Philippe Mathieu-Daudé wrote:
>>> Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.
>>>
>>> QMP event examples:
>>>
>>> {
>>>      "timestamp": {
>>>          "seconds": 1591704274,
>>>          "microseconds": 520850
>>>      },
>>>      "event": "LED_STATUS_CHANGED",
>>>      "data": {
>>>          "name": "Green LED #0",
>>>          "status": "on"
>>>      }
>>> }
>>> {
>>>      "timestamp": {
>>>          "seconds": 1591704275,
>>>          "microseconds": 530912
>>>      },
>>>      "event": "LED_STATUS_CHANGED",
>>>      "data": {
>>>          "name": "Green LED #0",
>>>          "status": "off"
>>>      }
>>> }
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>> 
>> The QAPI addition looks reasonable, however,
>> 
>>> +++ b/hw/misc/led.c
>>> @@ -7,6 +7,7 @@
>>>    */
>>>   #include "qemu/osdep.h"
>>>   #include "qapi/error.h"
>>> +#include "qapi/qapi-events-led.h"
>>>   #include "hw/qdev-properties.h"
>>>   #include "hw/misc/led.h"
>>>   #include "hw/irq.h"
>>> @@ -19,6 +20,9 @@ static void led_set(void *opaque, int line, int
>>> new_state)
>>>         trace_led_set(s->name, s->current_state, new_state);
>>>   +    /* FIXME QMP rate limite? */
>> 
>> s/limite/limit/
>> 
>> Yes, this is under guest control, so you MUST rate limit to avoid the
>> guest being able to DoS qemu by changing the LED so frequently as to
>> overwhelm the QMP connection with events.
>
> Commits f544d174dfc and 7f1e7b23d5 refers to the qmp-events.txt
> for documentation on rate-limiting QMP events, but I can't find
> it in the codebase. Two files matches 'qmp-events' but don't have
> documentation: qapi/qmp-event.c and include/qapi/qmp-event.h.
>
> Last trace of it is in commit 231aaf3a8217. Apparently it was
> somehow split qapi/event.json, then later c09656f1d392 move it
> to qapi-schema.json, finally eb815e248f50 moved it to qapi/.
>
> Is the referred documentation now in docs/devel/qapi-code-gen.txt?
> There is only one occurence of 'limit' but it is unrelated to
> rate-limit.

Commit 231aaf3a8217 is part of Marc-André's herculean QAPI/QMP doc
reorganization: use only schema doc comments instead of spreading the
knowledge over schema and several other files, with duplicated contents,
confused readers, and annoyed writers.

Before the reorganization, docs/qmp-events.txt listed the QMP events,
and rate-limited events carried a

    Note: this event is rate-limited.

The reorganization moved this note into its event's doc comment.
Example:

    ##
    # @WATCHDOG:
    #
    # Emitted when the watchdog device's timer is expired
    #
    # @action: action that has been taken
    #
    # Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
    #       followed respectively by the RESET, SHUTDOWN, or STOP events
    #
--> # Note: This event is rate-limited.
    #
    [...]
    ##
    { 'event': 'WATCHDOG',
      'data': { 'action': 'WatchdogAction' } }

The QMP *protocol* is still documented in docs/interop/qmp-spec.txt.
Relevant part:

    2.5 Asynchronous events
    -----------------------

    As a result of state changes, the Server may send messages unilaterally
    to the Client at any time, when not in the middle of any other
    response. They are called "asynchronous events".

    The format of asynchronous events is:

    { "event": json-string, "data": json-object,
      "timestamp": { "seconds": json-number, "microseconds": json-number } }

     Where,

    - The "event" member contains the event's name
    - The "data" member contains event specific data, which is defined in a
      per-event basis, it is optional
    - The "timestamp" member contains the exact time of when the event
      occurred in the Server. It is a fixed json-object with time in
      seconds and microseconds relative to the Unix Epoch (1 Jan 1970); if
      there is a failure to retrieve host time, both members of the
      timestamp will be set to -1.

    For a listing of supported asynchronous events, please, refer to the
    qmp-events.txt file.

    Some events are rate-limited to at most one per second.  If additional
    "similar" events arrive within one second, all but the last one are
    dropped, and the last one is delayed.  "Similar" normally means same
    event type.  See qmp-events.txt for details.

The reorganization neglected to update it for the removal of
qmp-events.txt.  Should point to
docs/interop/qemu-qmp-ref.{7,html,info,pdf,txt} now.

Event rate-limiting is defined in monitor_qapi_event_conf[].  To
rate-limit an event, add it to monitor_qapi_event_conf[], and also add
the "Note: This event is rate-limited." to its schema doc comment.

Doc bug: commit e2ae6159de "virtio-serial: report frontend connection
state via monitor" neglected to add the note.

Patches welcome!

Re: [RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
[...]
>> Commits f544d174dfc and 7f1e7b23d5 refers to the qmp-events.txt
>> for documentation on rate-limiting QMP events, but I can't find
>> it in the codebase. Two files matches 'qmp-events' but don't have
>> documentation: qapi/qmp-event.c and include/qapi/qmp-event.h.
>>
>> Last trace of it is in commit 231aaf3a8217. Apparently it was
>> somehow split qapi/event.json, then later c09656f1d392 move it
>> to qapi-schema.json, finally eb815e248f50 moved it to qapi/.
>>
>> Is the referred documentation now in docs/devel/qapi-code-gen.txt?
>> There is only one occurence of 'limit' but it is unrelated to
>> rate-limit.
>
> Commit 231aaf3a8217 is part of Marc-André's herculean QAPI/QMP doc
> reorganization: use only schema doc comments instead of spreading the
> knowledge over schema and several other files, with duplicated contents,
> confused readers, and annoyed writers.
>
> Before the reorganization, docs/qmp-events.txt listed the QMP events,
> and rate-limited events carried a
>
>     Note: this event is rate-limited.
>
> The reorganization moved this note into its event's doc comment.
> Example:
>
>     ##
>     # @WATCHDOG:
>     #
>     # Emitted when the watchdog device's timer is expired
>     #
>     # @action: action that has been taken
>     #
>     # Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
>     #       followed respectively by the RESET, SHUTDOWN, or STOP events
>     #
> --> # Note: This event is rate-limited.
>     #
>     [...]
>     ##
>     { 'event': 'WATCHDOG',
>       'data': { 'action': 'WatchdogAction' } }
>
> The QMP *protocol* is still documented in docs/interop/qmp-spec.txt.
> Relevant part:
>
>     2.5 Asynchronous events
>     -----------------------
>
>     As a result of state changes, the Server may send messages unilaterally
>     to the Client at any time, when not in the middle of any other
>     response. They are called "asynchronous events".
>
>     The format of asynchronous events is:
>
>     { "event": json-string, "data": json-object,
>       "timestamp": { "seconds": json-number, "microseconds": json-number } }
>
>      Where,
>
>     - The "event" member contains the event's name
>     - The "data" member contains event specific data, which is defined in a
>       per-event basis, it is optional
>     - The "timestamp" member contains the exact time of when the event
>       occurred in the Server. It is a fixed json-object with time in
>       seconds and microseconds relative to the Unix Epoch (1 Jan 1970); if
>       there is a failure to retrieve host time, both members of the
>       timestamp will be set to -1.
>
>     For a listing of supported asynchronous events, please, refer to the
>     qmp-events.txt file.
>
>     Some events are rate-limited to at most one per second.  If additional
>     "similar" events arrive within one second, all but the last one are
>     dropped, and the last one is delayed.  "Similar" normally means same
>     event type.  See qmp-events.txt for details.
>
> The reorganization neglected to update it for the removal of
> qmp-events.txt.  Should point to
> docs/interop/qemu-qmp-ref.{7,html,info,pdf,txt} now.
>
> Event rate-limiting is defined in monitor_qapi_event_conf[].  To
> rate-limit an event, add it to monitor_qapi_event_conf[], and also add
> the "Note: This event is rate-limited." to its schema doc comment.
>
> Doc bug: commit e2ae6159de "virtio-serial: report frontend connection
> state via monitor" neglected to add the note.
>
> Patches welcome!

Subject: [PATCH 1/3] docs/interop/qmp-spec: Point to the QEMU QMP reference manual
Message-Id: <20200806081147.3123652-2-armbru@redhat.com>

Subject: [PATCH 2/3] qapi: Document event VSERPORT_CHANGE is rate-limited
Message-Id: <20200806081147.3123652-3-armbru@redhat.com>