On Fri, 2020-12-04 at 13:50 -0300, Daniel Henrique Barboza wrote:
> On 12/4/20 12:13 PM, Andrea Bolognani wrote:
> > I suggest you instead do a pseudo-revert, where you rename the
> > function (without otherwise altering its signature) and move it to
> > the part of qemu_domain.c where you are ultimately going to need it;
> > in the commit message, you can still mention the commit that such a
> > change is a "spiritual revert" of, but this way we avoid muddying the
> > waters more than necessary.
>
> The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def),
> because that would be another function that would need to be moved up. I forgot to
> mention that in patch 04.
Why would you want to avoid using it? It's exactly what that function
is intended to do. Moving it around is no big deal, and by using it
you can avoid open-coding the same value twice. See attached diff.
> IIUC what you're suggesting can be implemented as follows:
>
> - go back to the first revert I was doing in v2 (remove the code from PostParse).
> Single revert of d3f3c2c97f9b92;
>
> - apply the other 2 patches;
>
> - do an extra patch to rename/move the function that is now being used only
> in QEMU files, but without a straight up revert of ace5931553c8. I can also
> simplify the signature in this step.
Yes, except of course the other two patches should still include the
changes that were implemented after my review, eg. they should be as
seen in
https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign
and not how they showed up initially on the list.
> The order here is intentional - the double revert in patch 3 done in this series
> needed to be followed up by changes in patches 4 and 5 (given that the function was
> renamed in patch 3). In this order, I can pick the patches from your local branch
> directly and just bother with the renaming in a single follow-up patch.
>
>
> Sounds good?
Yeah, it sounds like a plan. You can consider all patches from the
branch linked above
Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx>
and push them at your leisure.
--
Andrea Bolognani / Red Hat / Virtualization
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e7f82d42ed..f3404b0895 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5341,8 +5341,39 @@ qemuDomainTPMDefPostParse(virDomainTPMDefPtr tpm,
}
+static unsigned long long
+qemuDomainGetMemorySizeAlignment(const virDomainDef *def)
+{
+ /* PPC requires the memory sizes to be rounded to 256MiB increments, so
+ * round them to the size always. */
+ if (ARCH_IS_PPC64(def->os.arch))
+ return 256 * 1024;
+
+ /* Align memory size. QEMU requires rounding to next 4KiB block.
+ * We'll take the "traditional" path and round it to 1MiB */
+
+ return 1024;
+}
+
+
+static unsigned long long
+qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def,
+ const virDomainMemoryDef *mem G_GNUC_UNUSED)
+{
+ /* PPC requires the memory sizes to be rounded to 256MiB increments, so
+ * round them to the size always. */
+ if (ARCH_IS_PPC64(def->os.arch))
+ return 256 * 1024;
+
+ /* dimm memory modules require 2MiB alignment rather than the 1MiB we are
+ * using elsewhere. */
+ return 2048;
+}
+
+
static int
-qemuDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem)
+qemuDomainNVDimmAlignSizePseries(const virDomainDef *def,
+ virDomainMemoryDefPtr mem)
{
/* For NVDIMMs in ppc64 in we want to align down the guest
* visible space, instead of align up, to avoid writing
@@ -5358,7 +5389,7 @@ qemuDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem)
*
* target_size = AlignDown(target_size - label_size) + label_size
*/
- unsigned long long ppc64AlignSize = 256 * 1024;
+ unsigned long long ppc64AlignSize = qemuDomainGetMemorySizeAlignment(def);
unsigned long long guestArea = mem->size - mem->labelsize;
/* Align down guest_area. 256MiB is the minimum size. Error
@@ -5379,7 +5410,8 @@ qemuDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem)
static int
-qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, virArch arch,
+qemuDomainMemoryDefPostParse(const virDomainDef *def,
+ virDomainMemoryDefPtr mem, virArch arch,
unsigned int parseFlags)
{
/* Memory alignment can't be done for migration or snapshot
@@ -5396,10 +5428,10 @@ qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, virArch arch,
* guests as well. */
if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
if (ARCH_IS_PPC64(arch)) {
- unsigned long long ppc64MemModuleAlign = 256 * 1024;
+ unsigned long long ppc64MemModuleAlign = qemuDomainGetMemorySizeAlignment(def);
if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
- if (qemuDomainNVDimmAlignSizePseries(mem) < 0)
+ if (qemuDomainNVDimmAlignSizePseries(def, mem) < 0)
return -1;
} else {
mem->size = VIR_ROUND_UP(mem->size, ppc64MemModuleAlign);
@@ -5469,7 +5501,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
break;
case VIR_DOMAIN_DEVICE_MEMORY:
- ret = qemuDomainMemoryDefPostParse(dev->data.memory, def->os.arch,
+ ret = qemuDomainMemoryDefPostParse(def, dev->data.memory, def->os.arch,
parseFlags);
break;
@@ -8088,36 +8120,6 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
}
-static unsigned long long
-qemuDomainGetMemorySizeAlignment(const virDomainDef *def)
-{
- /* PPC requires the memory sizes to be rounded to 256MiB increments, so
- * round them to the size always. */
- if (ARCH_IS_PPC64(def->os.arch))
- return 256 * 1024;
-
- /* Align memory size. QEMU requires rounding to next 4KiB block.
- * We'll take the "traditional" path and round it to 1MiB */
-
- return 1024;
-}
-
-
-static unsigned long long
-qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def,
- const virDomainMemoryDef *mem G_GNUC_UNUSED)
-{
- /* PPC requires the memory sizes to be rounded to 256MiB increments, so
- * round them to the size always. */
- if (ARCH_IS_PPC64(def->os.arch))
- return 256 * 1024;
-
- /* dimm memory modules require 2MiB alignment rather than the 1MiB we are
- * using elsewhere. */
- return 2048;
-}
-
-
int
qemuDomainAlignMemorySizes(virDomainDefPtr def)
{
@@ -8166,7 +8168,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
for (i = 0; i < def->nmems; i++) {
if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
ARCH_IS_PPC64(def->os.arch)) {
- if (qemuDomainNVDimmAlignSizePseries(def->mems[i]) < 0)
+ if (qemuDomainNVDimmAlignSizePseries(def, def->mems[i]) < 0)
return -1;
} else {
align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
@@ -8203,7 +8205,7 @@ qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
{
if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
ARCH_IS_PPC64(def->os.arch)) {
- return qemuDomainNVDimmAlignSizePseries(mem);
+ return qemuDomainNVDimmAlignSizePseries(def, mem);
} else {
mem->size = VIR_ROUND_UP(mem->size,
qemuDomainGetMemorySizeAlignment(def));
