Re: [PATCH 2/5] qemu_validate.c: add pSeries NVDIMM size alignment validation
On 8/24/20 1:44 PM, Michal Privoznik wrote:
On 7/30/20 9:48 PM, Daniel Henrique Barboza wrote:
> The existing auto-align behavior for pSeries has the idea to
> alleviate user configuration of the NVDIMM size, given that the
> alignment calculation is not trivial to do (256MiB alignment
> of mem->size - mem->label_size value, a.k.a guest area). We
> align mem->size down to avoid end of file problems.
>
> The end result is not ideal though. We do not touch the domain
> XML, meaning that the XML can report a NVDIMM size 255MiB smaller
> than the actual size the guest is seeing. It also adds one more
> thing to consider in case the guest is reporting less memory
> than declared, since the auto-align is transparent to the
> user.
>
> Following Andrea's suggestion in [1], let's instead do an
> size alignment validation. If the NVDIMM is unaligned, error out
> and suggest a rounded up value. This can be bothersome to users,
> but will bring consistency of NVDIMM size between the domain XML
> and the guest.
>
> This approach will force existing non-running pSeries guests to
> readjust the NVDIMM value in their XMLs, if necessary. No changes
> were made for x86 NVDIMM support.
>
> [1] https://www.redhat.com/archives/libvir-list/2020-July/msg01471.html
>
> Suggested-by: Andrea Bolognani <abologna(a)redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> src/qemu/qemu_validate.c | 43 ++++++++++++++++---
> ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args | 2 +-
> .../memory-hotplug-nvdimm-ppc64.xml | 2 +-
> .../memory-hotplug-nvdimm-ppc64.xml | 2 +-
> 4 files changed, 40 insertions(+), 9 deletions(-)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 488f258d00..241139e268 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -4012,15 +4012,46 @@ qemuValidateDomainDeviceDefHub(virDomainHubDefPtr hub,
> }
> +static unsigned long long
> +qemuValidateGetNVDIMMAlignedSizePseries(virDomainMemoryDefPtr mem,
> + const virDomainDef *def)
> +{
> + unsigned long long ppc64AlignSize =
> + qemuDomainGetMemorySizeAlignment((virDomainDefPtr)def);
Ouch. This is not nice. We need a trivial patch that makes
qemuDomainGetMemorySizeAlignment() accept a const virDomainDef. I'm placing it at the
beginning before 1/5.
> + unsigned long long guestArea = mem->size - mem->labelsize;
> +
> + // NVDIMM is already aligned
We're old style, we use c89 style of comments :-)
LOL this went completely unnoticed. This kind of stuff reveals my true
age :P
DHB
> + if (guestArea % ppc64AlignSize == 0)
> + return mem->size;
> +
> + // Suggested aligned size is rounded up
> + guestArea = (guestArea/ppc64AlignSize + 1) * ppc64AlignSize;
> + return guestArea + mem->labelsize;
> +}
Michal