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

[Daniel Henrique Barboza]

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 <address@hidden>
>
> 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