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

[Vaibhav Jain]

Hi Shiva,

Thanks for reviweing this patch. My responses inline below;


Shivaprasad G Bhat <sbhat@linux.ibm.com> writes:

<snip>

>>   
>> +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) {
>> +        return H_PARAMETER;
>> +    }
>> +
>
>
> Please check if drc->dev is not NULL too. DRCs are created in advance
>
> and drc->dev may not be assigned if the device is not plugged yet.
>
>
Sure, will address that in v2

>> +    nvdimm = NVDIMM(drc->dev);
>> +
>> +    /* Check if the nvdimm is unarmed and send its status via health 
>> bitmaps */
>> +    args[0] = nvdimm->unarmed ? PAPR_PMEM_UNARMED_MASK : 0;
>
>
> Please use object_property_get_bool to fetch the unarmed value.
>
>
Sure I will switch to object_property_get_bool in v2. However I see
nvdimm->unarmed being accessed in similar manner in
nvdimm_build_structure_memdev() which probably needs an update too.

<snip>

-- 
Cheers
~ Vaibhav