From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: Igor Mammedov <imammedo@redhat.com>,
qemu-ppc@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM device hotplug sanity checks at pre-plug only
Date: Mon, 23 Nov 2020 15:55:48 +1100 [thread overview]
Message-ID: <20201123045548.GJ521467@yekko.fritz.box> (raw)
In-Reply-To: <20201120234208.683521-3-groug@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 6715 bytes --]
On Sat, Nov 21, 2020 at 12:42:01AM +0100, Greg Kurz wrote:
> Pre-plug of a memory device, be it an NVDIMM or a PC-DIMM, ensures
> that the memory slot is available and that addresses don't overlap
> with existing memory regions. The corresponding DRCs in the LMB
> and PMEM namespaces are thus necessarily attachable at plug time.
>
> Pass &error_abort to spapr_drc_attach() in spapr_add_lmbs() and
> spapr_add_nvdimm(). This allows to greatly simplify error handling
> on the plug path.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-6.0, thanks.
> ---
> include/hw/ppc/spapr_nvdimm.h | 2 +-
> hw/ppc/spapr.c | 40 ++++++++++++-----------------------
> hw/ppc/spapr_nvdimm.c | 11 +++++-----
> 3 files changed, 20 insertions(+), 33 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> index 344582d2f5f7..73be250e2ac9 100644
> --- a/include/hw/ppc/spapr_nvdimm.h
> +++ b/include/hw/ppc/spapr_nvdimm.h
> @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
> bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> uint64_t size, Error **errp);
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot);
>
> #endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12a012d9dd09..394d28d9e081 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3382,8 +3382,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> return 0;
> }
>
> -static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> - bool dedicated_hp_event_source, Error **errp)
> +static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> + bool dedicated_hp_event_source)
> {
> SpaprDrc *drc;
> uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> @@ -3396,15 +3396,12 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
>
> - if (!spapr_drc_attach(drc, dev, errp)) {
> - while (addr > addr_start) {
> - addr -= SPAPR_MEMORY_BLOCK_SIZE;
> - drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> - addr / SPAPR_MEMORY_BLOCK_SIZE);
> - spapr_drc_detach(drc);
> - }
> - return false;
> - }
> + /*
> + * memory_device_get_free_addr() provided a range of free addresses
> + * that doesn't overlap with any existing mapping at pre-plug. The
> + * corresponding LMB DRCs are thus assumed to be all attachable.
> + */
> + spapr_drc_attach(drc, dev, &error_abort);
> if (!hotplugged) {
> spapr_drc_reset(drc);
> }
> @@ -3425,11 +3422,9 @@ static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> nr_lmbs);
> }
> }
> - return true;
> }
>
> -static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> - Error **errp)
> +static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
> {
> SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
> PCDIMMDevice *dimm = PC_DIMM(dev);
> @@ -3444,24 +3439,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> if (!is_nvdimm) {
> addr = object_property_get_uint(OBJECT(dimm),
> PC_DIMM_ADDR_PROP, &error_abort);
> - if (!spapr_add_lmbs(dev, addr, size,
> - spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
> - goto out_unplug;
> - }
> + spapr_add_lmbs(dev, addr, size,
> + spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT));
> } else {
> slot = object_property_get_int(OBJECT(dimm),
> PC_DIMM_SLOT_PROP, &error_abort);
> /* We should have valid slot number at this point */
> g_assert(slot >= 0);
> - if (!spapr_add_nvdimm(dev, slot, errp)) {
> - goto out_unplug;
> - }
> + spapr_add_nvdimm(dev, slot);
> }
> -
> - return;
> -
> -out_unplug:
> - pc_dimm_unplug(dimm, MACHINE(ms));
> }
>
> static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -4009,7 +3995,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> - spapr_memory_plug(hotplug_dev, dev, errp);
> + spapr_memory_plug(hotplug_dev, dev);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> spapr_core_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5ed3..2f1c196e1b76 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
> }
>
>
> -bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> +void spapr_add_nvdimm(DeviceState *dev, uint64_t slot)
> {
> SpaprDrc *drc;
> bool hotplugged = spapr_drc_hotplugged(dev);
> @@ -97,14 +97,15 @@ bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
> g_assert(drc);
>
> - if (!spapr_drc_attach(drc, dev, errp)) {
> - return false;
> - }
> + /*
> + * pc_dimm_get_free_slot() provided a free slot at pre-plug. The
> + * corresponding DRC is thus assumed to be attachable.
> + */
> + spapr_drc_attach(drc, dev, &error_abort);
>
> if (hotplugged) {
> spapr_hotplug_req_add_by_index(drc);
> }
> - return true;
> }
>
> static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
--
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-11-23 5:10 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-20 23:41 [PATCH for-6.0 0/9] spapr: Perform hotplug sanity checks at pre-plug Greg Kurz
2020-11-20 23:42 ` [PATCH for-6.0 1/9] spapr: Do PCI device hotplug sanity checks at pre-plug only Greg Kurz
2020-11-23 4:50 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 2/9] spapr: Do NVDIMM/PC-DIMM " Greg Kurz
2020-11-23 4:55 ` David Gibson [this message]
2020-11-20 23:42 ` [PATCH for-6.0 3/9] spapr: Fix pre-2.10 dummy ICP hack Greg Kurz
2020-11-23 5:03 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 4/9] spapr: Set compat mode in spapr_reset_vcpu() Greg Kurz
2020-11-23 5:11 ` David Gibson
2020-11-23 11:51 ` Greg Kurz
2020-11-25 2:39 ` David Gibson
2020-11-25 9:51 ` Greg Kurz
2020-11-26 4:57 ` David Gibson
2020-11-26 9:10 ` Greg Kurz
2020-11-26 16:23 ` Igor Mammedov
2020-11-26 20:53 ` Igor Mammedov
2020-11-27 4:59 ` David Gibson
2020-11-27 13:25 ` Greg Kurz
2020-11-20 23:42 ` [PATCH for-6.0 5/9] spapr: Simplify error path of spapr_core_plug() Greg Kurz
2020-11-23 5:13 ` David Gibson
2020-11-24 13:07 ` Greg Kurz
2020-11-25 2:40 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 6/9] spapr: Make PHB placement functions and spapr_pre_plug_phb() return status Greg Kurz
2020-11-23 5:14 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 7/9] spapr: Do PHB hoplug sanity check at pre-plug Greg Kurz
2020-11-23 5:26 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 8/9] spapr: Do TPM proxy hotplug sanity checks " Greg Kurz
2020-11-23 5:32 ` David Gibson
2020-11-20 23:42 ` [PATCH for-6.0 9/9] spapr: spapr_drc_attach() cannot fail Greg Kurz
2020-11-23 5:34 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201123045548.GJ521467@yekko.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=groug@kaod.org \
--cc=imammedo@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).