Re: [PATCH] spapr_nvdimm.c: make 'label-size' mandatory

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 4/13/20 11:01 PM, David Gibson wrote:
> CCing Xiao, Michael and Igor for generic NVDIMM perspective.
> 
> On Mon, Apr 13, 2020 at 05:36:28PM -0300, Daniel Henrique Barboza wrote:
>> The pseries machine does not support NVDIMM modules without label.
>> Attempting to do so, even if the overall block size is aligned with
>> 256MB, will seg fault the guest kernel during NVDIMM probe. This
>> can be avoided by forcing 'label-size' to always be present for
>> sPAPR NVDIMMs.
>>
>> The verification was put before the alignment check because the
>> presence of label-size affects the alignment calculation, so
>> it's not optimal to warn the user about an alignment error,
>> then about the lack of label-size, then about a new alignment
>> error when the user sets a label-size.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> So, this would get the job done, but it seems a bit inelegant compared
> to having the device default to working settings.  I'm looking at how
> this interacts with the generic constraints on label-size.
> 
> The generic nvdimm code has a MIN_NAMESPACE_LABEL_SIZE of 128 kiB, and
> values of label-size less than that are rejected.  Except that if
> label-size is not set at all, it is left as 0.
> 
> Is that intended behaviour?  Do x86 (or whatever) NVDIMMs have a label
> of at least 128kiB, unless they have no label at all?  Or could we
> make the default label-size 128kiB generically?

My limited understanding on how NVDIMM works in x86 is that x86 NVDIMMs can
work with and without label, but the label has a minimum size of 128kiB.

The current NVDIMM init code always adds NVDIMM_LABEL_SIZE_PROP regardless
of user settings, as you can see in hw/mem/nvdimm.c, nvdimm_init(). I found
this out in my first attempt in this patch when doing


object_property_find(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
                      &error_abort) == 0) == NULL


inside spapr_nvdimm.c and finding out that this property is always present. This
means that the way to distinguish between label and label-less NVDIMMs is to
check label_size != 0, as we can see being done in hw/acpi/nvdimm.c. What I did
then was to use this same approach in spapr_nvdimm.c.




Thanks,


DHB