Hi David, Thanks for looking into this patch David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> writes: > On Thu, Apr 01, 2021 at 06:35:19AM +0530, Vaibhav Jain wrote: >> Add support for H_SCM_HEALTH hcall described at [1] for spapr >> nvdimms. This enables guest to detect the 'unarmed' status of a >> specific spapr nvdimm identified by its DRC and if its unarmed, mark >> the region backed by the nvdimm as read-only. >> >> The patch adds h_scm_health() to handle the H_SCM_HEALTH hcall which >> returns two 64-bit bitmaps (health bitmap, health bitmap mask) derived >> from 'struct nvdimm->unarmed' member. >> >> Linux kernel side changes to enable handling of 'unarmed' nvdimms for >> ppc64 are proposed at [2]. >> >> References: >> [1] "Hypercall Op-codes (hcalls)" >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/powerpc/papr_hcalls.rst#n220 >> [2] "powerpc/papr_scm: Mark nvdimm as unarmed if needed during probe" >> https://lore.kernel.org/linux-nvdimm/20210329113103.476760-1-vaibhav@xxxxxxxxxxxxx/ >> >> Signed-off-by: Vaibhav Jain <vaibhav@xxxxxxxxxxxxx> > > As well as the handful of comments below, this will definitely need to > wait for ppc-6.1 at this point. > Sure >> --- >> Changelog >> >> v2: >> * Added a check for drc->dev to ensure that the dimm is plugged in >> when servicing H_SCM_HEALTH. [ Shiva ] >> * Instead of accessing the 'nvdimm->unarmed' member directly use the >> object_property_get_bool accessor to fetch it. [ Shiva ] >> * Update the usage of PAPR_PMEM_UNARMED* macros [ Greg ] >> * Updated patch description reference#1 to point appropriate section >> in the documentation. [ Greg ] >> --- >> hw/ppc/spapr_nvdimm.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr.h | 3 ++- >> 2 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c >> index b46c36917c..34096e4718 100644 >> --- a/hw/ppc/spapr_nvdimm.c >> +++ b/hw/ppc/spapr_nvdimm.c >> @@ -31,6 +31,13 @@ >> #include "qemu/range.h" >> #include "hw/ppc/spapr_numa.h" >> >> +/* DIMM health bitmap bitmap indicators. Taken from kernel's papr_scm.c */ >> +/* SCM device is unable to persist memory contents */ >> +#define PAPR_PMEM_UNARMED (1ULL << (63 - 0)) > > You can use PPC_BIT() for more clarity here. > Sure, will address this in v3 >> +/* Bits status indicators for health bitmap indicating unarmed dimm */ >> +#define PAPR_PMEM_UNARMED_MASK (PAPR_PMEM_UNARMED) > > I'm not sure why you want two equal #defines here. > Will address this in v3. Switched to a single define. >> + >> bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, >> uint64_t size, Error **errp) >> { >> @@ -467,6 +474,36 @@ static target_ulong h_scm_unbind_all(PowerPCCPU *cpu, SpaprMachineState *spapr, >> return H_SUCCESS; >> } >> >> +static target_ulong h_scm_health(PowerPCCPU *cpu, SpaprMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + uint32_t drc_index = args[0]; >> + SpaprDrc *drc = spapr_drc_by_index(drc_index); >> + NVDIMMDevice *nvdimm; >> + >> + if (drc && spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) { > > This will fail badly if !drc (given index is way out of bounds). I'm > pretty sure you want > if (!drc || spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) { > > Thanks for catching this. I have fixed this in v3 >> + return H_PARAMETER; >> + } >> + >> + /* Ensure that the dimm is plugged in */ >> + if (!drc->dev) { >> + return H_HARDWARE; > > H_HARDWARE doesn't seem right - it's the guest that has chosen to > attempt this on an unplugged LMB, not the (virtual) hardware's fault. > Agree, addressed this in v3 >> + } >> + >> + nvdimm = NVDIMM(drc->dev); >> + >> + args[0] = 0; >> + /* Check if the nvdimm is unarmed and send its status via health bitmaps */ >> + if (object_property_get_bool(OBJECT(nvdimm), NVDIMM_UNARMED_PROP, NULL)) { >> + args[0] |= PAPR_PMEM_UNARMED; >> + } >> + >> + /* Update the health bitmap with the applicable mask */ >> + args[1] = PAPR_PMEM_UNARMED_MASK; >> + >> + return H_SUCCESS; >> +} >> + >> static void spapr_scm_register_types(void) >> { >> /* qemu/scm specific hcalls */ >> @@ -475,6 +512,7 @@ static void spapr_scm_register_types(void) >> spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem); >> spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem); >> spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all); >> + spapr_register_hypercall(H_SCM_HEALTH, h_scm_health); >> } >> >> type_init(spapr_scm_register_types) >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index 47cebaf3ac..6e1eafb05d 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -538,8 +538,9 @@ struct SpaprMachineState { >> #define H_SCM_BIND_MEM 0x3EC >> #define H_SCM_UNBIND_MEM 0x3F0 >> #define H_SCM_UNBIND_ALL 0x3FC >> +#define H_SCM_HEALTH 0x400 >> >> -#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL >> +#define MAX_HCALL_OPCODE H_SCM_HEALTH >> >> /* The hcalls above are standardized in PAPR and implemented by pHyp >> * as well. > > -- > 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 -- Cheers ~ Vaibhav
