[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/1] spapr_drc.c: handle hotunplug errors in drc_unisolate
From: |
David Gibson |
Subject: |
Re: [PATCH v2 1/1] spapr_drc.c: handle hotunplug errors in drc_unisolate_logical() |
Date: |
Wed, 21 Apr 2021 14:09:56 +1000 |
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