diff mbox

[1/5] spapr: Leave DR-indicator management to the guest

Message ID 20170620015332.13874-2-david@gibson.dropbear.id.au
State New, archived
Headers show

Commit Message

David Gibson June 20, 2017, 1:53 a.m. UTC
The DR-indicator is essentially a "virtual LED" attached to a hotpluggable
device, which the guest can set to various states for the attention of
the operator or management layers.

It's mostly guest managed, except that we once-off set it to
ACTIVE/INACTIVE in the attach/detach path.  While that makes certain sense,
there's no indication in PAPR that the hypervisor should do this, and the
drmgr code on the guest side doesn't appear to need it (it will already set
the indicator to ACTIVE on hotplug, and INACTIVE on remove).

So, leave the DR-indicator entirely to the guest; the only thing we need
to do is ensure it's in a sane state on reset.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Laurent Vivier June 20, 2017, 4:05 p.m. UTC | #1
On 20/06/2017 03:53, David Gibson wrote:
> The DR-indicator is essentially a "virtual LED" attached to a hotpluggable
> device, which the guest can set to various states for the attention of
> the operator or management layers.
> 
> It's mostly guest managed, except that we once-off set it to
> ACTIVE/INACTIVE in the attach/detach path.  While that makes certain sense,
> there's no indication in PAPR that the hypervisor should do this, and the
> drmgr code on the guest side doesn't appear to need it (it will already set
> the indicator to ACTIVE on hotplug, and INACTIVE on remove).
> 
> So, leave the DR-indicator entirely to the guest; the only thing we need
> to do is ensure it's in a sane state on reset.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fd9e07d..0bc9046 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -353,8 +353,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      }
>      g_assert(fdt || coldplug);
>  
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> -
>      drc->dev = d;
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
> @@ -372,8 +370,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void spapr_drc_release(sPAPRDRConnector *drc)
>  {
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> -
>      /* Calling release callbacks based on spapr_drc_type(drc). */
>      switch (spapr_drc_type(drc)) {
>      case SPAPR_DR_CONNECTOR_TYPE_CPU:
> @@ -452,12 +448,14 @@ static void reset(DeviceState *d)
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>      } else {
>          /* Otherwise device is absent, but might be hotplugged */
>          drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
>      }
>  }
>  
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Greg Kurz June 20, 2017, 4:12 p.m. UTC | #2
On Tue, 20 Jun 2017 09:53:28 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> The DR-indicator is essentially a "virtual LED" attached to a hotpluggable
> device, which the guest can set to various states for the attention of
> the operator or management layers.
> 
> It's mostly guest managed, except that we once-off set it to
> ACTIVE/INACTIVE in the attach/detach path.  While that makes certain sense,
> there's no indication in PAPR that the hypervisor should do this, and the
> drmgr code on the guest side doesn't appear to need it (it will already set
> the indicator to ACTIVE on hotplug, and INACTIVE on remove).
> 
> So, leave the DR-indicator entirely to the guest; the only thing we need
> to do is ensure it's in a sane state on reset.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_drc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fd9e07d..0bc9046 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -353,8 +353,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      }
>      g_assert(fdt || coldplug);
>  
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> -
>      drc->dev = d;
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
> @@ -372,8 +370,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void spapr_drc_release(sPAPRDRConnector *drc)
>  {
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> -
>      /* Calling release callbacks based on spapr_drc_type(drc). */
>      switch (spapr_drc_type(drc)) {
>      case SPAPR_DR_CONNECTOR_TYPE_CPU:
> @@ -452,12 +448,14 @@ static void reset(DeviceState *d)
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>      } else {
>          /* Otherwise device is absent, but might be hotplugged */
>          drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
>      }
>  }
>
diff mbox

Patch

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd9e07d..0bc9046 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -353,8 +353,6 @@  void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     }
     g_assert(fdt || coldplug);
 
-    drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
-
     drc->dev = d;
     drc->fdt = fdt;
     drc->fdt_start_offset = fdt_start_offset;
@@ -372,8 +370,6 @@  void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
 
 static void spapr_drc_release(sPAPRDRConnector *drc)
 {
-    drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
-
     /* Calling release callbacks based on spapr_drc_type(drc). */
     switch (spapr_drc_type(drc)) {
     case SPAPR_DR_CONNECTOR_TYPE_CPU:
@@ -452,12 +448,14 @@  static void reset(DeviceState *d)
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
             drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
         }
+        drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
     } else {
         /* Otherwise device is absent, but might be hotplugged */
         drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
             drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
         }
+        drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
     }
 }