[Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[Bharata B Rao]

When drmgr is run in the guest to add a device for which device_add
hasn't been issued in QEMU, configure-connector call fails.
When configure-connector call fails, the guest would release (*)
the previously acquired DRC by setting back the DRC isolation state
to ISOLATED and allocation state to UNUSABLE. These calls will be issued
only if get-sensor-state call returns PRESENT state. However currently for
a logical DR, entity_sense() would unconditinally return UNUSABLE
state only. This prevents any subsequent hotplug of the device with
that DRC.

Fix this by returning the right state in entity_sense() by checking
the allocation_state of DRC.

(*) https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/133430.html

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_drc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9ce844a..2586065 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -186,7 +186,11 @@ static sPAPRDREntitySense entity_sense(sPAPRDRConnector *drc)
              */
             state = SPAPR_DR_ENTITY_SENSE_EMPTY;
         } else {
-            state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
+            if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
+                state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
+            } else {
+                state = SPAPR_DR_ENTITY_SENSE_PRESENT;
+            }
         }
     }
 
-- 
2.1.0

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> When drmgr is run in the guest to add a device for which device_add
> hasn't been issued in QEMU, configure-connector call fails.
> When configure-connector call fails, the guest would release (*)
> the previously acquired DRC by setting back the DRC isolation state
> to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> only if get-sensor-state call returns PRESENT state. However currently for
> a logical DR, entity_sense() would unconditinally return UNUSABLE
> state only. This prevents any subsequent hotplug of the device with
> that DRC.
> 
> Fix this by returning the right state in entity_sense() by checking
> the allocation_state of DRC.
> 
> (*) https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/133430.html
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

and applied to my tree.

> ---
>  hw/ppc/spapr_drc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9ce844a..2586065 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -186,7 +186,11 @@ static sPAPRDREntitySense entity_sense(sPAPRDRConnector *drc)
>               */
>              state = SPAPR_DR_ENTITY_SENSE_EMPTY;
>          } else {
> -            state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> +            if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> +                state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> +            } else {
> +                state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> +            }
>          }
>      }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[Michael Roth]

Quoting David Gibson (2015-09-07 20:22:50)
> On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > When drmgr is run in the guest to add a device for which device_add
> > hasn't been issued in QEMU, configure-connector call fails.
> > When configure-connector call fails, the guest would release (*)
> > the previously acquired DRC by setting back the DRC isolation state
> > to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> > only if get-sensor-state call returns PRESENT state. However currently for
> > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > state only. This prevents any subsequent hotplug of the device with
> > that DRC.

This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
for logical DR, and it's up the guest to transition to USABLE, which
probably happens prior to the configure-connector calls. So I think the
net effect of this fix is that guest will see these unallocated/unattached
resources the same way they would a resource that was actually attached
via device_add, and all we're really doing is working around the
eventual configuration failure that that will lead to by pretending a
resource was actually there.

According to PAPR+ 2.7:

13.7.3.1 Acquire Logical Resource from Resource Pool:

  If the state is “unusable” the OS issues set-indicator (allocation-state, usable) to attempt to allocate the re-
  source. Similarly, if the state is “available for exchange” the OS issues set-indicator (allocation-state, ex-
  change) to attempt to allocate the resource, and if the state is “available for recovery” the OS issues
  set-indicator (allocation-state, recover) to attempt to allocate the resource.

and

13.7 Logical Resource Dynamic Reconfiguration (LRDR):

  The OS may use the get-sensor-state RTAS call with the dr-entity-sense token to deter-
mine if a given drc-index refers to a connector that is currently usable for DR operations. If the connector is not
currently usable the return state is “DR entity unusable” (2). A set-indicator (isolation state) RTAS call to an unusable
connector or (dr-indicator) to any logical resource connector results in a “No such indicator implemented” return sta-
tus.  

So I think maybe the proper fix is to make sure that
drc->set_indicator_state() fails with an error that indicates to RTAS to
return NO_SENSOR (-3) for cases where we haven't attached a resource
to the DRC via device_add.

Which also kind of re-opens the discussion of whether or not
drc->set_indicator_state() should return RTAS errors directly. I'd
still stray away from that for now but maybe if we get more cases
like this it'll start becoming more practical.

> > 
> > Fix this by returning the right state in entity_sense() by checking
> > the allocation_state of DRC.
> > 
> > (*) https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/133430.html
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> and applied to my tree.
> 
> > ---
> >  hw/ppc/spapr_drc.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 9ce844a..2586065 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -186,7 +186,11 @@ static sPAPRDREntitySense entity_sense(sPAPRDRConnector *drc)
> >               */
> >              state = SPAPR_DR_ENTITY_SENSE_EMPTY;
> >          } else {
> > -            state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> > +            if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > +                state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> > +            } else {
> > +                state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> > +            }
> >          }
> >      }
> >  
> 
> -- 
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[Michael Roth]

Quoting Michael Roth (2015-09-08 16:03:56)
> Quoting David Gibson (2015-09-07 20:22:50)
> > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > > When drmgr is run in the guest to add a device for which device_add
> > > hasn't been issued in QEMU, configure-connector call fails.
> > > When configure-connector call fails, the guest would release (*)
> > > the previously acquired DRC by setting back the DRC isolation state
> > > to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> > > only if get-sensor-state call returns PRESENT state. However currently for
> > > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > > state only. This prevents any subsequent hotplug of the device with
> > > that DRC.
> 
> This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
> for logical DR, and it's up the guest to transition to USABLE, which
> probably happens prior to the configure-connector calls. So I think the
> net effect of this fix is that guest will see these unallocated/unattached
> resources the same way they would a resource that was actually attached
> via device_add, and all we're really doing is working around the
> eventual configuration failure that that will lead to by pretending a
> resource was actually there.
> 
> According to PAPR+ 2.7:
> 
> 13.7.3.1 Acquire Logical Resource from Resource Pool:
> 
>   If the state is “unusable” the OS issues set-indicator (allocation-state, usable) to attempt to allocate the re-
>   source. Similarly, if the state is “available for exchange” the OS issues set-indicator (allocation-state, ex-
>   change) to attempt to allocate the resource, and if the state is “available for recovery” the OS issues
>   set-indicator (allocation-state, recover) to attempt to allocate the resource.
> 
> and
> 
> 13.7 Logical Resource Dynamic Reconfiguration (LRDR):
> 
>   The OS may use the get-sensor-state RTAS call with the dr-entity-sense token to deter-
> mine if a given drc-index refers to a connector that is currently usable for DR operations. If the connector is not
> currently usable the return state is “DR entity unusable” (2). A set-indicator (isolation state) RTAS call to an unusable
> connector or (dr-indicator) to any logical resource connector results in a “No such indicator implemented” return sta-
> tus.  
> 
> So I think maybe the proper fix is to make sure that
> drc->set_indicator_state() fails with an error that indicates to RTAS to

drc->set_isolation_state(), I mean.

> return NO_SENSOR (-3) for cases where we haven't attached a resource
> to the DRC via device_add.
> 
> Which also kind of re-opens the discussion of whether or not
> drc->set_indicator_state() should return RTAS errors directly. I'd

and again.

> still stray away from that for now but maybe if we get more cases
> like this it'll start becoming more practical.
> 
> > > 
> > > Fix this by returning the right state in entity_sense() by checking
> > > the allocation_state of DRC.
> > > 
> > > (*) https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/133430.html
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > and applied to my tree.
> > 
> > > ---
> > >  hw/ppc/spapr_drc.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 9ce844a..2586065 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -186,7 +186,11 @@ static sPAPRDREntitySense entity_sense(sPAPRDRConnector *drc)
> > >               */
> > >              state = SPAPR_DR_ENTITY_SENSE_EMPTY;
> > >          } else {
> > > -            state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> > > +            if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > > +                state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> > > +            } else {
> > > +                state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> > > +            }
> > >          }
> > >      }
> > >  
> > 
> > -- 
> > David Gibson                    | I'll have my music baroque, and my code
> > david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
> >                                 | _way_ _around_!
> > http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[Michael Roth]

Quoting Michael Roth (2015-09-08 16:03:56)
> Quoting David Gibson (2015-09-07 20:22:50)
> > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > > When drmgr is run in the guest to add a device for which device_add
> > > hasn't been issued in QEMU, configure-connector call fails.
> > > When configure-connector call fails, the guest would release (*)
> > > the previously acquired DRC by setting back the DRC isolation state
> > > to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> > > only if get-sensor-state call returns PRESENT state. However currently for
> > > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > > state only. This prevents any subsequent hotplug of the device with
> > > that DRC.
> 
> This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
> for logical DR, and it's up the guest to transition to USABLE, which
> probably happens prior to the configure-connector calls. So I think the
> net effect of this fix is that guest will see these unallocated/unattached
> resources the same way they would a resource that was actually attached
> via device_add, and all we're really doing is working around the
> eventual configuration failure that that will lead to by pretending a
> resource was actually there.
> 
> According to PAPR+ 2.7:
> 
> 13.7.3.1 Acquire Logical Resource from Resource Pool:
> 
>   If the state is “unusable” the OS issues set-indicator (allocation-state, usable) to attempt to allocate the re-
>   source. Similarly, if the state is “available for exchange” the OS issues set-indicator (allocation-state, ex-
>   change) to attempt to allocate the resource, and if the state is “available for recovery” the OS issues
>   set-indicator (allocation-state, recover) to attempt to allocate the resource.
> 
> and
> 
> 13.7 Logical Resource Dynamic Reconfiguration (LRDR):
> 
>   The OS may use the get-sensor-state RTAS call with the dr-entity-sense token to deter-
> mine if a given drc-index refers to a connector that is currently usable for DR operations. If the connector is not
> currently usable the return state is “DR entity unusable” (2). A set-indicator (isolation state) RTAS call to an unusable
> connector or (dr-indicator) to any logical resource connector results in a “No such indicator implemented” return sta-
> tus.  
> 
> So I think maybe the proper fix is to make sure that
> drc->set_indicator_state() fails with an error that indicates to RTAS to
> return NO_SENSOR (-3) for cases where we haven't attached a resource
> to the DRC via device_add.

Patch incoming:

  spapr_drc: don't allow 'empty' DRCs to be unisolated

applies to spapr-next but requires revert of this patch.

Bharata, can you give it a spin with CPU hotplug and see if it fixes the
issue you hit?

> 
> Which also kind of re-opens the discussion of whether or not
> drc->set_indicator_state() should return RTAS errors directly. I'd
> still stray away from that for now but maybe if we get more cases
> like this it'll start becoming more practical.

I'm starting to second-guess myself on this. I'm trying to maintain
separation between RTAS/DRC but result is a bit pathological. Feel free
to comment in the above patch.

Just FYI: original author names appear to have gotten lost in recent
spapr-next rebase.

> 
> > > 
> > > Fix this by returning the right state in entity_sense() by checking
> > > the allocation_state of DRC.
> > > 
> > > (*) https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/133430.html
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > and applied to my tree.
> > 
> > > ---
> > >  hw/ppc/spapr_drc.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 9ce844a..2586065 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -186,7 +186,11 @@ static sPAPRDREntitySense entity_sense(sPAPRDRConnector *drc)
> > >               */
> > >              state = SPAPR_DR_ENTITY_SENSE_EMPTY;
> > >          } else {
> > > -            state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> > > +            if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > > +                state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> > > +            } else {
> > > +                state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> > > +            }
> > >          }
> > >      }
> > >  
> > 
> > -- 
> > David Gibson                    | I'll have my music baroque, and my code
> > david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
> >                                 | _way_ _around_!
> > http://www.ozlabs.org/~dgibson
> 
> 

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[Bharata B Rao]

On Tue, Sep 08, 2015 at 05:03:25PM -0500, Michael Roth wrote:
> Quoting Michael Roth (2015-09-08 16:03:56)
> > Quoting David Gibson (2015-09-07 20:22:50)
> > > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > > > When drmgr is run in the guest to add a device for which device_add
> > > > hasn't been issued in QEMU, configure-connector call fails.
> > > > When configure-connector call fails, the guest would release (*)
> > > > the previously acquired DRC by setting back the DRC isolation state
> > > > to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> > > > only if get-sensor-state call returns PRESENT state. However currently for
> > > > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > > > state only. This prevents any subsequent hotplug of the device with
> > > > that DRC.
> > 
> > This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
> > for logical DR, and it's up the guest to transition to USABLE, which
> > probably happens prior to the configure-connector calls. So I think the
> > net effect of this fix is that guest will see these unallocated/unattached
> > resources the same way they would a resource that was actually attached
> > via device_add, and all we're really doing is working around the
> > eventual configuration failure that that will lead to by pretending a
> > resource was actually there.
> > 
> > According to PAPR+ 2.7:
> > 
> > 13.7.3.1 Acquire Logical Resource from Resource Pool:
> > 
> >   If the state is “unusable” the OS issues set-indicator (allocation-state, usable) to attempt to allocate the re-
> >   source. Similarly, if the state is “available for exchange” the OS issues set-indicator (allocation-state, ex-
> >   change) to attempt to allocate the resource, and if the state is “available for recovery” the OS issues
> >   set-indicator (allocation-state, recover) to attempt to allocate the resource.
> > 
> > and
> > 
> > 13.7 Logical Resource Dynamic Reconfiguration (LRDR):
> > 
> >   The OS may use the get-sensor-state RTAS call with the dr-entity-sense token to deter-
> > mine if a given drc-index refers to a connector that is currently usable for DR operations. If the connector is not
> > currently usable the return state is “DR entity unusable” (2). A set-indicator (isolation state) RTAS call to an unusable
> > connector or (dr-indicator) to any logical resource connector results in a “No such indicator implemented” return sta-
> > tus.  
> > 
> > So I think maybe the proper fix is to make sure that
> > drc->set_indicator_state() fails with an error that indicates to RTAS to
> > return NO_SENSOR (-3) for cases where we haven't attached a resource
> > to the DRC via device_add.
> 
> Patch incoming:
> 
>   spapr_drc: don't allow 'empty' DRCs to be unisolated
> 
> applies to spapr-next but requires revert of this patch.

Yes, please revert this patch in favour of Michael's.

> 
> Bharata, can you give it a spin with CPU hotplug and see if it fixes the
> issue you hit?

Yes it does, thanks.

Regards,
Bharata.

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3388 bytes --]

On Wed, Sep 09, 2015 at 09:32:32AM +0530, Bharata B Rao wrote:
> On Tue, Sep 08, 2015 at 05:03:25PM -0500, Michael Roth wrote:
> > Quoting Michael Roth (2015-09-08 16:03:56)
> > > Quoting David Gibson (2015-09-07 20:22:50)
> > > > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > > > > When drmgr is run in the guest to add a device for which device_add
> > > > > hasn't been issued in QEMU, configure-connector call fails.
> > > > > When configure-connector call fails, the guest would release (*)
> > > > > the previously acquired DRC by setting back the DRC isolation state
> > > > > to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> > > > > only if get-sensor-state call returns PRESENT state. However currently for
> > > > > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > > > > state only. This prevents any subsequent hotplug of the device with
> > > > > that DRC.
> > > 
> > > This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
> > > for logical DR, and it's up the guest to transition to USABLE, which
> > > probably happens prior to the configure-connector calls. So I think the
> > > net effect of this fix is that guest will see these unallocated/unattached
> > > resources the same way they would a resource that was actually attached
> > > via device_add, and all we're really doing is working around the
> > > eventual configuration failure that that will lead to by pretending a
> > > resource was actually there.
> > > 
> > > According to PAPR+ 2.7:
> > > 
> > > 13.7.3.1 Acquire Logical Resource from Resource Pool:
> > > 
> > >   If the state is “unusable” the OS issues set-indicator (allocation-state, usable) to attempt to allocate the re-
> > >   source. Similarly, if the state is “available for exchange” the OS issues set-indicator (allocation-state, ex-
> > >   change) to attempt to allocate the resource, and if the state is “available for recovery” the OS issues
> > >   set-indicator (allocation-state, recover) to attempt to allocate the resource.
> > > 
> > > and
> > > 
> > > 13.7 Logical Resource Dynamic Reconfiguration (LRDR):
> > > 
> > >   The OS may use the get-sensor-state RTAS call with the dr-entity-sense token to deter-
> > > mine if a given drc-index refers to a connector that is currently usable for DR operations. If the connector is not
> > > currently usable the return state is “DR entity unusable” (2). A set-indicator (isolation state) RTAS call to an unusable
> > > connector or (dr-indicator) to any logical resource connector results in a “No such indicator implemented” return sta-
> > > tus.  
> > > 
> > > So I think maybe the proper fix is to make sure that
> > > drc->set_indicator_state() fails with an error that indicates to RTAS to
> > > return NO_SENSOR (-3) for cases where we haven't attached a resource
> > > to the DRC via device_add.
> > 
> > Patch incoming:
> > 
> >   spapr_drc: don't allow 'empty' DRCs to be unisolated
> > 
> > applies to spapr-next but requires revert of this patch.
> 
> Yes, please revert this patch in favour of Michael's.

Done.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4315 bytes --]

On Tue, Sep 08, 2015 at 05:03:25PM -0500, Michael Roth wrote:
> Quoting Michael Roth (2015-09-08 16:03:56)
> > Quoting David Gibson (2015-09-07 20:22:50)
> > > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > > > When drmgr is run in the guest to add a device for which device_add
> > > > hasn't been issued in QEMU, configure-connector call fails.
> > > > When configure-connector call fails, the guest would release (*)
> > > > the previously acquired DRC by setting back the DRC isolation state
> > > > to ISOLATED and allocation state to UNUSABLE. These calls will be issued
> > > > only if get-sensor-state call returns PRESENT state. However currently for
> > > > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > > > state only. This prevents any subsequent hotplug of the device with
> > > > that DRC.
> > 
> > This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
> > for logical DR, and it's up the guest to transition to USABLE, which
> > probably happens prior to the configure-connector calls. So I think the
> > net effect of this fix is that guest will see these unallocated/unattached
> > resources the same way they would a resource that was actually attached
> > via device_add, and all we're really doing is working around the
> > eventual configuration failure that that will lead to by pretending a
> > resource was actually there.
> > 
> > According to PAPR+ 2.7:
> > 
> > 13.7.3.1 Acquire Logical Resource from Resource Pool:
> > 
> >   If the state is “unusable” the OS issues set-indicator (allocation-state, usable) to attempt to allocate the re-
> >   source. Similarly, if the state is “available for exchange” the OS issues set-indicator (allocation-state, ex-
> >   change) to attempt to allocate the resource, and if the state is “available for recovery” the OS issues
> >   set-indicator (allocation-state, recover) to attempt to allocate the resource.
> > 
> > and
> > 
> > 13.7 Logical Resource Dynamic Reconfiguration (LRDR):
> > 
> >   The OS may use the get-sensor-state RTAS call with the dr-entity-sense token to deter-
> > mine if a given drc-index refers to a connector that is currently usable for DR operations. If the connector is not
> > currently usable the return state is “DR entity unusable” (2). A set-indicator (isolation state) RTAS call to an unusable
> > connector or (dr-indicator) to any logical resource connector results in a “No such indicator implemented” return sta-
> > tus.  
> > 
> > So I think maybe the proper fix is to make sure that
> > drc->set_indicator_state() fails with an error that indicates to RTAS to
> > return NO_SENSOR (-3) for cases where we haven't attached a resource
> > to the DRC via device_add.
> 
> Patch incoming:
> 
>   spapr_drc: don't allow 'empty' DRCs to be unisolated
> 
> applies to spapr-next but requires revert of this patch.
> 
> Bharata, can you give it a spin with CPU hotplug and see if it fixes the
> issue you hit?
> 
> > 
> > Which also kind of re-opens the discussion of whether or not
> > drc->set_indicator_state() should return RTAS errors directly. I'd
> > still stray away from that for now but maybe if we get more cases
> > like this it'll start becoming more practical.
> 
> I'm starting to second-guess myself on this. I'm trying to maintain
> separation between RTAS/DRC but result is a bit pathological. Feel free
> to comment in the above patch.

To my mind the set_indicator path is pretty much inherently specific
to PAPR's weird way of doing things.  For that reason I think it's
just going to be simplest for it to return PAPR defined errors -
i.e. RTAS errors.

The whole thing is so tied to PAPR, I don't think it makes sense to
try to use generic error codes for it.  Trying to separate "DRC" error
codes from the RTAS codes they result in sounds like a translation
layer for no real gain.

> Just FYI: original author names appear to have gotten lost in recent
> spapr-next rebase.

Ah, thanks for catching that.  Should be fixed now.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]