[PATCH RFC 0/2] add SHPC hotplug event

[PATCH RFC 0/2] add SHPC hotplug event

[Vladimir Sementsov-Ogievskiy]

Hi all! Please look at 02 for the details.

Vladimir Sementsov-Ogievskiy (2):
  hw/pci/shpc: introduce FOR_EACH_DEVICE_IN_SLOT
  qapi: introduce DEVICE_POWER_ON for SHPC hotplug

 hw/pci/shpc.c  | 53 +++++++++++++++++++++++++++++++++++++++++---------
 qapi/qdev.json | 23 ++++++++++++++++++++++
 2 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/pci/shpc: introduce FOR_EACH_DEVICE_IN_SLOT

[Vladimir Sementsov-Ogievskiy]

Introduce a macro to loop through devices like in
shpc_free_devices_in_slot(), as we are going to add one more similar
function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/shpc.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index e71f3a7483..ba241e2818 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -236,22 +236,41 @@ static void shpc_invalid_command(SHPCDevice *shpc)
                                SHPC_CMD_STATUS_INVALID_CMD);
 }
 
-static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
+static PCIDevice *shpc_next_device_in_slot(SHPCDevice *shpc, int slot,
+                                           int *start_devfn)
 {
-    HotplugHandler *hotplug_ctrl;
     int devfn;
     int pci_slot = SHPC_IDX_TO_PCI(slot);
-    for (devfn = PCI_DEVFN(pci_slot, 0);
+
+    for (devfn = *start_devfn ?: PCI_DEVFN(pci_slot, 0);
          devfn <= PCI_DEVFN(pci_slot, PCI_FUNC_MAX - 1);
          ++devfn) {
-        PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
-        if (affected_dev) {
-            hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(affected_dev));
-            hotplug_handler_unplug(hotplug_ctrl, DEVICE(affected_dev),
-                                   &error_abort);
-            object_unparent(OBJECT(affected_dev));
+        PCIDevice *dev = shpc->sec_bus->devices[devfn];
+        if (dev) {
+            *start_devfn = devfn + 1; /* for next iteration */
+            return dev;
         }
     }
+
+    return NULL;
+}
+
+#define FOR_EACH_DEVICE_IN_SLOT(shpc, slot, dev, devfn) \
+    for ((devfn) = 0, \
+         (dev) = shpc_next_device_in_slot((shpc), (slot), &(devfn)); \
+         (dev); (dev) = shpc_next_device_in_slot((shpc), (slot), &(devfn)))
+
+static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
+{
+    HotplugHandler *hotplug_ctrl;
+    int devfn;
+    PCIDevice *dev;
+
+    FOR_EACH_DEVICE_IN_SLOT(shpc, slot, dev, devfn) {
+        hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(dev));
+        hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort);
+        object_unparent(OBJECT(dev));
+    }
 }
 
 static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
-- 
2.34.1

[PATCH 2/2] qapi: introduce DEVICE_POWER_ON for SHPC hotplug

[Vladimir Sementsov-Ogievskiy]

Hi all! That's an RFC patch.

The problem is that SHPC protocol says that power-led is blinking for 5
seconds before actual turning-on the device. If we call device-del
during this time the attention button press is ignored and we never get
DEVICE_DELETED event, which is unexpected for the user.

I suggest add a pair for DEVICE_DELETED: DEVICE_POWER_ON. So user
should wait for DEVICE_POWER_ON after device-add before making any
other operations with the device (incluing device-del).

What I'm unsure is what about other types of hotplug - PCIE and
ACPI.. Do they suffer from similar problems? Seems not.. Should we sent
for them this event at some moment of should the user be aware of which
kind of hotplug is in use to determine to wait for the DEVICE_POWER_ON
or not to wait.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/shpc.c  | 16 ++++++++++++++++
 qapi/qdev.json | 23 +++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index ba241e2818..7c53971c1c 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -1,5 +1,6 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-qdev.h"
 #include "qemu/host-utils.h"
 #include "qemu/range.h"
 #include "qemu/error-report.h"
@@ -273,6 +274,18 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
     }
 }
 
+static void shpc_devices_power_on_in_slot(SHPCDevice *shpc, int slot)
+{
+    int devfn;
+    PCIDevice *dev;
+
+    FOR_EACH_DEVICE_IN_SLOT(shpc, slot, dev, devfn) {
+        DeviceState *ds = DEVICE(dev);
+
+        qapi_event_send_device_power_on(!!ds->id, ds->id, ds->canonical_path);
+    }
+}
+
 static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
                               uint8_t state, uint8_t power, uint8_t attn)
 {
@@ -291,6 +304,9 @@ static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
     switch (power) {
     case SHPC_LED_NO:
         break;
+    case SHPC_LED_ON:
+        shpc_devices_power_on_in_slot(shpc, slot);
+        __attribute__ ((fallthrough));
     default:
         /* TODO: send event to monitor */
         shpc_set_status(shpc, slot, power, SHPC_SLOT_PWR_LED_MASK);
diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2708fb4e99..360dcf8ba6 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -158,3 +158,26 @@
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @DEVICE_POWER_ON:
+#
+# Emitted whenever power is on for the devices plugged into pci slot.
+# At this point it's safe to remove the device.
+#
+# @device: the device's ID if it has one
+#
+# @path: the device's QOM path
+#
+# Since: 7.2
+#
+# Example:
+#
+# <- { "event": "DEVICE_POWER_ON",
+#      "data": { "device": "virtio-disk-0",
+#                "path": "/machine/peripheral/virtio-disk-0" },
+#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+#
+##
+{ 'event': 'DEVICE_POWER_ON',
+  'data': { '*device': 'str', 'path': 'str' } }
-- 
2.34.1

Re: [PATCH 2/2] qapi: introduce DEVICE_POWER_ON for SHPC hotplug

[Michael S. Tsirkin]

On Wed, Nov 16, 2022 at 07:12:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! That's an RFC patch.
> 
> The problem is that SHPC protocol says that power-led is blinking for 5
> seconds before actual turning-on the device. If we call device-del
> during this time the attention button press is ignored and we never get
> DEVICE_DELETED event, which is unexpected for the user.
> 
> I suggest add a pair for DEVICE_DELETED: DEVICE_POWER_ON. So user
> should wait for DEVICE_POWER_ON after device-add before making any
> other operations with the device (incluing device-del).
> 
> What I'm unsure is what about other types of hotplug - PCIE and
> ACPI.. Do they suffer from similar problems?

I didn't yet look at this patchset deeply (we are in freeze anyway)
but PCIE is substancially same as SHPC.

Take a look at Gerd's "improve native hotplug for pcie root ports"
same kind of approach probably works for SHPC.

> Seems not.. Should we sent
> for them this event at some moment of should the user be aware of which
> kind of hotplug is in use to determine to wait for the DEVICE_POWER_ON
> or not to wait.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/pci/shpc.c  | 16 ++++++++++++++++
>  qapi/qdev.json | 23 +++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index ba241e2818..7c53971c1c 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -1,5 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/range.h"
>  #include "qemu/error-report.h"
> @@ -273,6 +274,18 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
>      }
>  }
>  
> +static void shpc_devices_power_on_in_slot(SHPCDevice *shpc, int slot)
> +{
> +    int devfn;
> +    PCIDevice *dev;
> +
> +    FOR_EACH_DEVICE_IN_SLOT(shpc, slot, dev, devfn) {
> +        DeviceState *ds = DEVICE(dev);
> +
> +        qapi_event_send_device_power_on(!!ds->id, ds->id, ds->canonical_path);
> +    }
> +}
> +
>  static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
>                                uint8_t state, uint8_t power, uint8_t attn)
>  {
> @@ -291,6 +304,9 @@ static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
>      switch (power) {
>      case SHPC_LED_NO:
>          break;
> +    case SHPC_LED_ON:
> +        shpc_devices_power_on_in_slot(shpc, slot);
> +        __attribute__ ((fallthrough));
>      default:
>          /* TODO: send event to monitor */
>          shpc_set_status(shpc, slot, power, SHPC_SLOT_PWR_LED_MASK);
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..360dcf8ba6 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,26 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_POWER_ON:
> +#
> +# Emitted whenever power is on for the devices plugged into pci slot.
> +# At this point it's safe to remove the device.
> +#
> +# @device: the device's ID if it has one
> +#
> +# @path: the device's QOM path
> +#
> +# Since: 7.2
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_POWER_ON",
> +#      "data": { "device": "virtio-disk-0",
> +#                "path": "/machine/peripheral/virtio-disk-0" },
> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'DEVICE_POWER_ON',
> +  'data': { '*device': 'str', 'path': 'str' } }
> -- 
> 2.34.1

Re: [PATCH 2/2] qapi: introduce DEVICE_POWER_ON for SHPC hotplug

[Vladimir Sementsov-Ogievskiy]

On 11/16/22 19:26, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2022 at 07:12:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all! That's an RFC patch.
>>
>> The problem is that SHPC protocol says that power-led is blinking for 5
>> seconds before actual turning-on the device. If we call device-del
>> during this time the attention button press is ignored and we never get
>> DEVICE_DELETED event, which is unexpected for the user.
>>
>> I suggest add a pair for DEVICE_DELETED: DEVICE_POWER_ON. So user
>> should wait for DEVICE_POWER_ON after device-add before making any
>> other operations with the device (incluing device-del).
>>
>> What I'm unsure is what about other types of hotplug - PCIE and
>> ACPI.. Do they suffer from similar problems?
> I didn't yet look at this patchset deeply (we are in freeze anyway)
> but PCIE is substancially same as SHPC.
> 
> Take a look at Gerd's "improve native hotplug for pcie root ports"
> same kind of approach probably works for SHPC.

Looking at it. Yes, I think this approach is OK, thanks for the link.

I doubt now that we really need new event. Instead I can update SHPC to return an error when trying to unplug with blinking power indicator (like 81124b3c7a5dae "pcie: add power indicator blink check").

-- 
Best regards,
Vladimir

Re: [PATCH RFC 0/2] add SHPC hotplug event

[Michael S. Tsirkin]

On Wed, Nov 16, 2022 at 07:12:32PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! Please look at 02 for the details.


I got 2 copies donnu which to reply to.

> Vladimir Sementsov-Ogievskiy (2):
>   hw/pci/shpc: introduce FOR_EACH_DEVICE_IN_SLOT
>   qapi: introduce DEVICE_POWER_ON for SHPC hotplug
> 
>  hw/pci/shpc.c  | 53 +++++++++++++++++++++++++++++++++++++++++---------
>  qapi/qdev.json | 23 ++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 9 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH RFC 0/2] add SHPC hotplug event

[Michael S. Tsirkin]

On Wed, Nov 16, 2022 at 07:12:32PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! Please look at 02 for the details.


In the future, pls use --subject-prefix='PATCH RFC' with git-format-patch to
add same prefix to all patches.
If you are resending, add 'resend' in the subject, or increase the
version #.

> Vladimir Sementsov-Ogievskiy (2):
>   hw/pci/shpc: introduce FOR_EACH_DEVICE_IN_SLOT
>   qapi: introduce DEVICE_POWER_ON for SHPC hotplug
> 
>  hw/pci/shpc.c  | 53 +++++++++++++++++++++++++++++++++++++++++---------
>  qapi/qdev.json | 23 ++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 9 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH RFC 0/2] add SHPC hotplug event

[Vladimir Sementsov-Ogievskiy]

On 11/16/22 19:23, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2022 at 07:12:32PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all! Please look at 02 for the details.
> 
> In the future, pls use --subject-prefix='PATCH RFC' with git-format-patch to
> add same prefix to all patches.
> If you are resending, add 'resend' in the subject, or increase the
> version #.
> 

OK, yes, will do next time. Sorry for the inconvenience :/

-- 
Best regards,
Vladimir