Re: [PATCH v4 3/4] spapr: Add NVDIMM device support

[Shivaprasad G Bhat]

On 01/03/2020 06:50 AM, David Gibson wrote:
> On Tue, Dec 17, 2019 at 02:49:14AM -0600, Shivaprasad G Bhat wrote:
> > Add support for NVDIMM devices for sPAPR. Piggyback on existing nvdimm
> > device interface in QEMU to support virtual NVDIMM devices for Power.
> > Create the required DT entries for the device (some entries have
> > dummy values right now).
> >
> > The patch creates the required DT node and sends a hotplug
> > interrupt to the guest. Guest is expected to undertake the normal
> > DR resource add path in response and start issuing PAPR SCM hcalls.
> >
> > The device support is verified based on the machine version unlike x86.
> >
> > This is how it can be used ..
> > Ex :
> > For coldplug, the device to be added in qemu command line as shown below
> > -object 
> > memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm0,share=yes,size=1073872896
> > -device 
> > nvdimm,label-size=128k,uuid=75a3cdd7-6a2f-4791-8d15-fe0a920e8e9e,memdev=memnvdimm0,id=nvdimm0,slot=0
> >
> > For hotplug, the device to be added from monitor as below
> > object_add 
> > memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm0,share=yes,size=1073872896
> > device_add 
> > nvdimm,label-size=128k,uuid=75a3cdd7-6a2f-4791-8d15-fe0a920e8e9e,memdev=memnvdimm0,id=nvdimm0,slot=0
> >
> > Signed-off-by: Shivaprasad G Bhat <address@hidden>
> > Signed-off-by: Bharata B Rao <address@hidden>
> >                 [Early implementation]
> > ---
> >   default-configs/ppc64-softmmu.mak |    1
> >   hw/mem/Kconfig                    |    2
> >   hw/ppc/spapr.c                    |  216 ++++++++++++++++++++++++++++++++++---
> >   hw/ppc/spapr_drc.c                |   18 +++
> >   hw/ppc/spapr_events.c             |    4 +
> >   include/hw/ppc/spapr.h            |   11 ++
> >   include/hw/ppc/spapr_drc.h        |    9 ++
> >   7 files changed, 245 insertions(+), 16 deletions(-)
> >
> > diff --git a/default-configs/ppc64-softmmu.mak 
> > b/default-configs/ppc64-softmmu.mak
> > index cca52665d9..ae0841fa3a 100644
> > --- a/default-configs/ppc64-softmmu.mak
> > +++ b/default-configs/ppc64-softmmu.mak
> > @@ -8,3 +8,4 @@ CONFIG_POWERNV=y
> >   
> >   # For pSeries
> >   CONFIG_PSERIES=y
> > +CONFIG_NVDIMM=y
> > diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
> > index 620fd4cb59..2ad052a536 100644
> > --- a/hw/mem/Kconfig
> > +++ b/hw/mem/Kconfig
> > @@ -8,4 +8,4 @@ config MEM_DEVICE
> >   config NVDIMM
> >       bool
> >       default y
> > -    depends on PC
> > +    depends on (PC || PSERIES)
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 3ae7db1563..921d8d7c8e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -80,6 +80,8 @@
> >   #include "hw/ppc/spapr_cpu_core.h"
> >   #include "hw/mem/memory-device.h"
> >   #include "hw/ppc/spapr_tpm_proxy.h"
> > +#include "hw/mem/nvdimm.h"
> > +#include "qemu/nvdimm-utils.h"
> >   
> >   #include "monitor/monitor.h"
> >   
> > @@ -685,12 +687,22 @@ static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
> >               nr_entries++;
> >           }
> >   
> > -        /* Entry for DIMM */
> > -        drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, addr / lmb_size);
> > -        g_assert(drc);
> > -        elem = spapr_get_drconf_cell(size / lmb_size, addr,
> > -                                     spapr_drc_index(drc), node,
> > -                                     SPAPR_LMB_FLAGS_ASSIGNED);
> > +        if (info->value->type == MEMORY_DEVICE_INFO_KIND_DIMM) {
> > +            /* Entry for DIMM */
> > +            drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, addr / lmb_size);
> > +            g_assert(drc);
> > +            elem = spapr_get_drconf_cell(size / lmb_size, addr,
> > +                                         spapr_drc_index(drc), node,
> > +                                         SPAPR_LMB_FLAGS_ASSIGNED);
> > +        } else if (info->value->type == MEMORY_DEVICE_INFO_KIND_NVDIMM) {
> > +            /*
> > +             * NVDIMM sits here, let the DIMM LMBs be unusable here in the
> > +             * whole range
> > +             */
> > +            elem = spapr_get_drconf_cell(size / lmb_size, addr, 0, -1,
> > +                                         SPAPR_LMB_FLAGS_RESERVED |
> > +                                         SPAPR_LMB_FLAGS_DRC_INVALID);
> > +        }
> As discussed in reply to an earlier thread, this whole scheme
> basically breaks down in the presence of hotplug - it relies on which
> GPAs are DIMMs and which are NVDIMMs not changing.
>
> Other than that significant problem, the rest of this looks
> reasonable.

As discussed, I verified not marking the NVDIMM occupied area as reserved,
and it seems to work fine. The malicious attempts to claim
the drcs on the those areas fail as there wont be a valid dimm devices
asscociated with those drcs.

Sending the next version fixing this part.