Re: [PATCH v2 1/1] spapr_drc.c: handle hotunplug errors in drc_unisolate_logical()

[David Gibson]

On Tue, Apr 20, 2021 at 01:51:00PM -0300, Daniel Henrique Barboza wrote:
> At this moment, PAPR does not provide a way to report errors during a
> device removal operation. This led the pSeries machine to implement
> extra mechanisms to try to fallback and recover from an error that might
> have happened during the hotunplug in the guest side. This started to
> change a bit with commit fe1831eff8a4 ("spapr_drc.c: use DRC
> reconfiguration to cleanup DIMM unplug state"), where one way to
> fallback from a memory removal error was introduced.
> 
> Around the same time, in [1], the idea of using RTAS set-indicator for
> this role was first introduced. The RTAS set-indicator call, when
> attempting to UNISOLATE a DRC that is already UNISOLATED or CONFIGURED,
> returns RTAS_OK and does nothing else for both QEMU and phyp. This gives
> us an opportunity to use this behavior to signal the hypervisor layer
> when a device removal errir happens, allowing QEMU/phyp to do a proper
> error handling. Using set-indicator to report HP errors isn't strange to
> PAPR, as per R1-13.5.3.4-4. of table 13.7 of current PAPR [2]:
> 
> "For all DR options: If this is a DR operation that involves the user
> insert- ing a DR entity, then if the firmware can determine that the
> inserted entity would cause a system disturbance, then the set-indicator
> RTAS call must not unisolate the entity and must return an error status
> which is unique to the particular error."
> 
> A change was proposed to the pSeries Linux kernel to call set-indicator
> to move a DRC to 'unisolate' in the case of a hotunplug error in the
> guest side [3]. Setting a DRC that is already unisolated or configured to
> 'unisolate' is a no-op (returns RTAS_OK) for QEMU and also for phyp.
> Being a benign change for hypervisors that doesn't care about handling
> such errors, we expect the kernel to accept this change at some point.
> 
> This patch prepares the pSeries machine for this new kernel feature by
> changing drc_unisolate_logical() to handle guest side hotunplug errors.
> For CPUs it's a simple matter of setting drc->unplug_requested to 'false',
> while for LMBs the process is similar to the rollback that is done in
> rtas_ibm_configure_connector().
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg06395.html
> [2] 
> https://openpowerfoundation.org/wp-content/uploads/2020/07/LoPAR-20200611.pdf
> [3] 
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210416210216.380291-3-danielhb413@gmail.com/
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Applied to ppc-for-6.1, thanks.

> ---
>  hw/ppc/spapr_drc.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9e16505fa1..6918e0c9d1 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -151,9 +151,32 @@ static uint32_t drc_isolate_logical(SpaprDrc *drc)
>  
>  static uint32_t drc_unisolate_logical(SpaprDrc *drc)
>  {
> +    SpaprMachineState *spapr = NULL;
> +
>      switch (drc->state) {
>      case SPAPR_DRC_STATE_LOGICAL_UNISOLATE:
>      case SPAPR_DRC_STATE_LOGICAL_CONFIGURED:
> +        /*
> +         * Unisolating a logical DRC that was marked for unplug
> +         * means that the kernel is refusing the removal.
> +         */
> +        if (drc->unplug_requested && drc->dev) {
> +            if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> +                spapr = SPAPR_MACHINE(qdev_get_machine());
> +
> +                spapr_memory_unplug_rollback(spapr, drc->dev);
> +            }
> +
> +            drc->unplug_requested = false;
> +            error_report("Device hotunplug rejected by the guest "
> +                         "for device %s", drc->dev->id);
> +
> +            /*
> +             * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
> +             * it is implemented.
> +             */
> +        }
> +
>          return RTAS_OUT_SUCCESS; /* Nothing to do */
>      case SPAPR_DRC_STATE_LOGICAL_AVAILABLE:
>          break; /* see below */

-- 
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

signature.asc
Description: PGP signature