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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

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?

> ---
>  hw/ppc/spapr_nvdimm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 25be8082d7..9abcdcc26b 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -37,6 +37,12 @@ void spapr_nvdimm_validate_opts(NVDIMMDevice *nvdimm, uint64_t size,
>      QemuUUID uuid;
>      int ret;
>  
> +    if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
> +                                &error_abort) == 0) {
> +        error_setg(errp, "NVDIMM device requires label-size to be set");
> +        return;
> +    }
> +
>      if (size % SPAPR_MINIMUM_SCM_BLOCK_SIZE) {
>          error_setg(errp, "NVDIMM memory size excluding the label area"
>                     " must be a multiple of %" PRIu64 "MB",

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]