Re: [PATCH v2] ppc/spapr: Add support for implement support for H_SCM_HEALTH

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi David,

Thanks for looking into this patch

David Gibson <david@gibson.dropbear.id.au> 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@linux.ibm.com/
>> 
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>
> 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