qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] memory-device: reintroduce memory region size check
@ 2024-01-17 13:55 David Hildenbrand
  2024-01-17 13:55 ` [PATCH v1 1/2] hv-balloon: use get_min_alignment() to express 32 GiB alignment David Hildenbrand
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: David Hildenbrand @ 2024-01-17 13:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: David Hildenbrand, Maciej S. Szmigiero, Mario Casquero,
	Igor Mammedov, Xiao Guangrong

Reintroduce a modified region size check, after we would now allow some
configurations that don't make any sense (e.g., partial hugetlb pages,
1G+1byte DIMMs).

We have to take care of hv-balloon first, which was the case why we
remove that check in the first place.

Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
Cc: Mario Casquero <mcasquer@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>

David Hildenbrand (2):
  hv-balloon: use get_min_alignment() to express 32 GiB alignment
  memory-device: reintroduce memory region size check

 hw/hyperv/hv-balloon.c | 37 +++++++++++++++++++++----------------
 hw/mem/memory-device.c | 14 ++++++++++++++
 2 files changed, 35 insertions(+), 16 deletions(-)

-- 
2.43.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v1 1/2] hv-balloon: use get_min_alignment() to express 32 GiB alignment
  2024-01-17 13:55 [PATCH v1 0/2] memory-device: reintroduce memory region size check David Hildenbrand
@ 2024-01-17 13:55 ` David Hildenbrand
  2024-01-17 13:55 ` [PATCH v1 2/2] memory-device: reintroduce memory region size check David Hildenbrand
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: David Hildenbrand @ 2024-01-17 13:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: David Hildenbrand, Maciej S. Szmigiero, Mario Casquero,
	Igor Mammedov, Xiao Guangrong

Let's implement the get_min_alignment() callback for memory devices, and
copy for the device memory region the alignment of the host memory
region. This mimics what virtio-mem does, and allows for re-introducing
proper alignment checks for the memory region size (where we don't care
about additional device requirements) in memory device core.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/hyperv/hv-balloon.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/hyperv/hv-balloon.c b/hw/hyperv/hv-balloon.c
index 66f297c1d7..0829c495b0 100644
--- a/hw/hyperv/hv-balloon.c
+++ b/hw/hyperv/hv-balloon.c
@@ -1476,22 +1476,7 @@ static void hv_balloon_ensure_mr(HvBalloon *balloon)
     balloon->mr = g_new0(MemoryRegion, 1);
     memory_region_init(balloon->mr, OBJECT(balloon), TYPE_HV_BALLOON,
                        memory_region_size(hostmem_mr));
-
-    /*
-     * The VM can indicate an alignment up to 32 GiB. Memory device core can
-     * usually only handle/guarantee 1 GiB alignment. The user will have to
-     * specify a larger maxmem eventually.
-     *
-     * The memory device core will warn the user in case maxmem might have to be
-     * increased and will fail plugging the device if there is not sufficient
-     * space after alignment.
-     *
-     * TODO: we could do the alignment ourselves in a slightly bigger region.
-     * But this feels better, although the warning might be annoying. Maybe
-     * we can optimize that in the future (e.g., with such a device on the
-     * cmdline place/size the device memory region differently.
-     */
-    balloon->mr->align = MAX(32 * GiB, memory_region_get_alignment(hostmem_mr));
+    balloon->mr->align = memory_region_get_alignment(hostmem_mr);
 }
 
 static void hv_balloon_free_mr(HvBalloon *balloon)
@@ -1653,6 +1638,25 @@ static MemoryRegion *hv_balloon_md_get_memory_region(MemoryDeviceState *md,
     return balloon->mr;
 }
 
+static uint64_t hv_balloon_md_get_min_alignment(const MemoryDeviceState *md)
+{
+    /*
+     * The VM can indicate an alignment up to 32 GiB. Memory device core can
+     * usually only handle/guarantee 1 GiB alignment. The user will have to
+     * specify a larger maxmem eventually.
+     *
+     * The memory device core will warn the user in case maxmem might have to be
+     * increased and will fail plugging the device if there is not sufficient
+     * space after alignment.
+     *
+     * TODO: we could do the alignment ourselves in a slightly bigger region.
+     * But this feels better, although the warning might be annoying. Maybe
+     * we can optimize that in the future (e.g., with such a device on the
+     * cmdline place/size the device memory region differently.
+     */
+    return 32 * GiB;
+}
+
 static void hv_balloon_md_fill_device_info(const MemoryDeviceState *md,
                                            MemoryDeviceInfo *info)
 {
@@ -1765,5 +1769,6 @@ static void hv_balloon_class_init(ObjectClass *klass, void *data)
     mdc->get_memory_region = hv_balloon_md_get_memory_region;
     mdc->decide_memslots = hv_balloon_decide_memslots;
     mdc->get_memslots = hv_balloon_get_memslots;
+    mdc->get_min_alignment = hv_balloon_md_get_min_alignment;
     mdc->fill_device_info = hv_balloon_md_fill_device_info;
 }
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v1 2/2] memory-device: reintroduce memory region size check
  2024-01-17 13:55 [PATCH v1 0/2] memory-device: reintroduce memory region size check David Hildenbrand
  2024-01-17 13:55 ` [PATCH v1 1/2] hv-balloon: use get_min_alignment() to express 32 GiB alignment David Hildenbrand
@ 2024-01-17 13:55 ` David Hildenbrand
  2024-01-18  3:08   ` Zhenyu Zhang
  2024-01-22 14:34 ` [PATCH v1 0/2] " Maciej S. Szmigiero
  2024-01-22 16:49 ` David Hildenbrand
  3 siblings, 1 reply; 8+ messages in thread
From: David Hildenbrand @ 2024-01-17 13:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: David Hildenbrand, Maciej S. Szmigiero, Mario Casquero,
	Igor Mammedov, Xiao Guangrong, Zhenyu Zhang, Michal Privoznik

We used to check that the memory region size is multiples of the overall
requested address alignment for the device memory address.

We removed that check, because there are cases (i.e., hv-balloon) where
devices unconditionally request an address alignment that has a very large
alignment (i.e., 32 GiB), but the actual memory device size might not be
multiples of that alignment.

However, this change:

(a) allows for some practically impossible DIMM sizes, like "1GB+1 byte".
(b) allows for DIMMs that partially cover hugetlb pages, previously
    reported in [1].

Both scenarios don't make any sense: we might even waste memory.

So let's reintroduce that check, but only check that the
memory region size is multiples of the memory region alignment (i.e.,
page size, huge page size), but not any additional memory device
requirements communicated using md->get_min_alignment().

The following examples now fail again as expected:

(a) 1M with 2M THP
 qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
                     -object memory-backend-ram,id=mem1,size=1M \
                     -device pc-dimm,id=dimm1,memdev=mem1
 -> backend memory size must be multiple of 0x200000

(b) 1G+1byte

 qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
                   -object memory-backend-ram,id=mem1,size=1073741825B \
                   -device pc-dimm,id=dimm1,memdev=mem1
 -> backend memory size must be multiple of 0x200000

(c) Unliagned hugetlb size (2M)

 qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
                   -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=511M \
                   -device pc-dimm,id=dimm1,memdev=mem1
 backend memory size must be multiple of 0x200000

(d) Unliagned hugetlb size (1G)

 qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
                    -object memory-backend-file,id=mem1,mem-path=/dev/hugepages1G/tmp,size=2047M \
                    -device pc-dimm,id=dimm1,memdev=mem1
 -> backend memory size must be multiple of 0x40000000

Note that this fix depends on a hv-balloon change to communicate its
additional alignment requirements using get_min_alignment() instead of
through the memory region.

[1] https://lkml.kernel.org/r/f77d641d500324525ac036fe1827b3070de75fc1.1701088320.git.mprivozn@redhat.com

Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
Reported-by: Michal Privoznik <mprivozn@redhat.com>
Fixes: eb1b7c4bd413 ("memory-device: Drop size alignment check")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index a1b1af26bc..e098585cda 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -374,6 +374,20 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
         goto out;
     }
 
+    /*
+     * We always want the memory region size to be multiples of the memory
+     * region alignment: for example, DIMMs with 1G+1byte size don't make
+     * any sense. Note that we don't check that the size is multiples
+     * of any additional alignment requirements the memory device might
+     * have when it comes to the address in physical address space.
+     */
+    if (!QEMU_IS_ALIGNED(memory_region_size(mr),
+                         memory_region_get_alignment(mr))) {
+        error_setg(errp, "backend memory size must be multiple of 0x%"
+                   PRIx64, memory_region_get_alignment(mr));
+        return;
+    }
+
     if (legacy_align) {
         align = *legacy_align;
     } else {
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] memory-device: reintroduce memory region size check
  2024-01-17 13:55 ` [PATCH v1 2/2] memory-device: reintroduce memory region size check David Hildenbrand
@ 2024-01-18  3:08   ` Zhenyu Zhang
  2024-01-19  8:35     ` Mario Casquero
  0 siblings, 1 reply; 8+ messages in thread
From: Zhenyu Zhang @ 2024-01-18  3:08 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: qemu-devel, Maciej S. Szmigiero, Mario Casquero, Igor Mammedov,
	Xiao Guangrong, Michal Privoznik, shan.gavin, Guowen Shan

[PATCH v1 2/2] memory-device: reintroduce memory region size check

Test on 64k basic page size aarch64
The patches work well on my Ampere host.
The test results are as expected.

(a) 1M with 512M THP
/home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
-name 'avocado-vt-vm1'  \
-sandbox on \
:
-nodefaults \
-m 4096,maxmem=32G,slots=4 \
-object memory-backend-ram,id=mem1,size=1M \
-device pc-dimm,id=dimm1,memdev=mem1 \
-smp 4  \
-cpu 'host' \
-accel kvm \
-monitor stdio
-> backend memory size must be multiple of 0x200000

(b) 1G+1byte
/home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
-name 'avocado-vt-vm1'  \
-sandbox on \
:
-nodefaults \
-m 4096,maxmem=32G,slots=4 \
-object memory-backend-ram,id=mem1,size=1073741825B \
-device pc-dimm,id=dimm1,memdev=mem1 \
-smp 4  \
-cpu 'host' \
-accel kvm \
-monitor stdio
-> backend memory size must be multiple of 0x200000

(c) Unliagned hugetlb size (2M)
/home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
-name 'avocado-vt-vm1'  \
-sandbox on \
:
-nodefaults \
-m 4096,maxmem=32G,slots=4 \
-object memory-backend-file,id=mem1,prealloc=yes,mem-path=/mnt/kvm_hugepage,size=2047M
\
-device pc-dimm,id=dimm1,memdev=mem1 \
-smp 4  \
-cpu 'host' \
-accel kvm \
-monitor stdio
-> backend memory size must be multiple of 0x200000

(d)  2M with 512M hugetlb size
/home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
-name 'avocado-vt-vm1'  \
-sandbox on \
:
-nodefaults \
-m 4096,maxmem=32G,slots=4 \
-object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=2M \
-device pc-dimm,id=dimm1,memdev=mem1 \
-smp 4  \
-cpu 'host' \
-accel kvm \
-monitor stdio
-> memory size 0x200000 must be equal to or larger than page size 0x20000000

(e)  511M with 512M hugetlb size
/home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
-name 'avocado-vt-vm1'  \
-sandbox on \
:
-nodefaults \
-m 4096,maxmem=32G,slots=4 \
-object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=511M \
-device pc-dimm,id=dimm1,memdev=mem1 \
-smp 4  \
-cpu 'host' \
-accel kvm \
-monitor stdio
-> memory size 0x1ff00000 must be equal to or larger than page size 0x20000000

Tested-by: Zhenyu Zhang <zhenzha@redhat.com>



On Wed, Jan 17, 2024 at 9:56 PM David Hildenbrand <david@redhat.com> wrote:
>
> We used to check that the memory region size is multiples of the overall
> requested address alignment for the device memory address.
>
> We removed that check, because there are cases (i.e., hv-balloon) where
> devices unconditionally request an address alignment that has a very large
> alignment (i.e., 32 GiB), but the actual memory device size might not be
> multiples of that alignment.
>
> However, this change:
>
> (a) allows for some practically impossible DIMM sizes, like "1GB+1 byte".
> (b) allows for DIMMs that partially cover hugetlb pages, previously
>     reported in [1].
>
> Both scenarios don't make any sense: we might even waste memory.
>
> So let's reintroduce that check, but only check that the
> memory region size is multiples of the memory region alignment (i.e.,
> page size, huge page size), but not any additional memory device
> requirements communicated using md->get_min_alignment().
>
> The following examples now fail again as expected:
>
> (a) 1M with 2M THP
>  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
>                      -object memory-backend-ram,id=mem1,size=1M \
>                      -device pc-dimm,id=dimm1,memdev=mem1
>  -> backend memory size must be multiple of 0x200000
>
> (b) 1G+1byte
>
>  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
>                    -object memory-backend-ram,id=mem1,size=1073741825B \
>                    -device pc-dimm,id=dimm1,memdev=mem1
>  -> backend memory size must be multiple of 0x200000
>
> (c) Unliagned hugetlb size (2M)
>
>  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
>                    -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=511M \
>                    -device pc-dimm,id=dimm1,memdev=mem1
>  backend memory size must be multiple of 0x200000
>
> (d) Unliagned hugetlb size (1G)
>
>  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
>                     -object memory-backend-file,id=mem1,mem-path=/dev/hugepages1G/tmp,size=2047M \
>                     -device pc-dimm,id=dimm1,memdev=mem1
>  -> backend memory size must be multiple of 0x40000000
>
> Note that this fix depends on a hv-balloon change to communicate its
> additional alignment requirements using get_min_alignment() instead of
> through the memory region.
>
> [1] https://lkml.kernel.org/r/f77d641d500324525ac036fe1827b3070de75fc1.1701088320.git.mprivozn@redhat.com
>
> Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
> Reported-by: Michal Privoznik <mprivozn@redhat.com>
> Fixes: eb1b7c4bd413 ("memory-device: Drop size alignment check")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index a1b1af26bc..e098585cda 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -374,6 +374,20 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
>          goto out;
>      }
>
> +    /*
> +     * We always want the memory region size to be multiples of the memory
> +     * region alignment: for example, DIMMs with 1G+1byte size don't make
> +     * any sense. Note that we don't check that the size is multiples
> +     * of any additional alignment requirements the memory device might
> +     * have when it comes to the address in physical address space.
> +     */
> +    if (!QEMU_IS_ALIGNED(memory_region_size(mr),
> +                         memory_region_get_alignment(mr))) {
> +        error_setg(errp, "backend memory size must be multiple of 0x%"
> +                   PRIx64, memory_region_get_alignment(mr));
> +        return;
> +    }
> +
>      if (legacy_align) {
>          align = *legacy_align;
>      } else {
> --
> 2.43.0
>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] memory-device: reintroduce memory region size check
  2024-01-18  3:08   ` Zhenyu Zhang
@ 2024-01-19  8:35     ` Mario Casquero
  0 siblings, 0 replies; 8+ messages in thread
From: Mario Casquero @ 2024-01-19  8:35 UTC (permalink / raw)
  To: Zhenyu Zhang
  Cc: David Hildenbrand, qemu-devel, Maciej S. Szmigiero, Igor Mammedov,
	Xiao Guangrong, Michal Privoznik, shan.gavin, Guowen Shan

This series has also been successfully tested in x86_64.

Tested-by: Mario Casquero <mcasquer@redhat.com>

On Thu, Jan 18, 2024 at 4:08 AM Zhenyu Zhang <zhenyzha@redhat.com> wrote:
>
> [PATCH v1 2/2] memory-device: reintroduce memory region size check
>
> Test on 64k basic page size aarch64
> The patches work well on my Ampere host.
> The test results are as expected.
>
> (a) 1M with 512M THP
> /home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
> -name 'avocado-vt-vm1'  \
> -sandbox on \
> :
> -nodefaults \
> -m 4096,maxmem=32G,slots=4 \
> -object memory-backend-ram,id=mem1,size=1M \
> -device pc-dimm,id=dimm1,memdev=mem1 \
> -smp 4  \
> -cpu 'host' \
> -accel kvm \
> -monitor stdio
> -> backend memory size must be multiple of 0x200000
>
> (b) 1G+1byte
> /home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
> -name 'avocado-vt-vm1'  \
> -sandbox on \
> :
> -nodefaults \
> -m 4096,maxmem=32G,slots=4 \
> -object memory-backend-ram,id=mem1,size=1073741825B \
> -device pc-dimm,id=dimm1,memdev=mem1 \
> -smp 4  \
> -cpu 'host' \
> -accel kvm \
> -monitor stdio
> -> backend memory size must be multiple of 0x200000
>
> (c) Unliagned hugetlb size (2M)
> /home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
> -name 'avocado-vt-vm1'  \
> -sandbox on \
> :
> -nodefaults \
> -m 4096,maxmem=32G,slots=4 \
> -object memory-backend-file,id=mem1,prealloc=yes,mem-path=/mnt/kvm_hugepage,size=2047M
> \
> -device pc-dimm,id=dimm1,memdev=mem1 \
> -smp 4  \
> -cpu 'host' \
> -accel kvm \
> -monitor stdio
> -> backend memory size must be multiple of 0x200000
>
> (d)  2M with 512M hugetlb size
> /home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
> -name 'avocado-vt-vm1'  \
> -sandbox on \
> :
> -nodefaults \
> -m 4096,maxmem=32G,slots=4 \
> -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=2M \
> -device pc-dimm,id=dimm1,memdev=mem1 \
> -smp 4  \
> -cpu 'host' \
> -accel kvm \
> -monitor stdio
> -> memory size 0x200000 must be equal to or larger than page size 0x20000000
>
> (e)  511M with 512M hugetlb size
> /home/zhenyzha/sandbox/qemu.main/build/qemu-system-aarch64 \
> -name 'avocado-vt-vm1'  \
> -sandbox on \
> :
> -nodefaults \
> -m 4096,maxmem=32G,slots=4 \
> -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=511M \
> -device pc-dimm,id=dimm1,memdev=mem1 \
> -smp 4  \
> -cpu 'host' \
> -accel kvm \
> -monitor stdio
> -> memory size 0x1ff00000 must be equal to or larger than page size 0x20000000
>
> Tested-by: Zhenyu Zhang <zhenzha@redhat.com>
>
>
>
> On Wed, Jan 17, 2024 at 9:56 PM David Hildenbrand <david@redhat.com> wrote:
> >
> > We used to check that the memory region size is multiples of the overall
> > requested address alignment for the device memory address.
> >
> > We removed that check, because there are cases (i.e., hv-balloon) where
> > devices unconditionally request an address alignment that has a very large
> > alignment (i.e., 32 GiB), but the actual memory device size might not be
> > multiples of that alignment.
> >
> > However, this change:
> >
> > (a) allows for some practically impossible DIMM sizes, like "1GB+1 byte".
> > (b) allows for DIMMs that partially cover hugetlb pages, previously
> >     reported in [1].
> >
> > Both scenarios don't make any sense: we might even waste memory.
> >
> > So let's reintroduce that check, but only check that the
> > memory region size is multiples of the memory region alignment (i.e.,
> > page size, huge page size), but not any additional memory device
> > requirements communicated using md->get_min_alignment().
> >
> > The following examples now fail again as expected:
> >
> > (a) 1M with 2M THP
> >  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
> >                      -object memory-backend-ram,id=mem1,size=1M \
> >                      -device pc-dimm,id=dimm1,memdev=mem1
> >  -> backend memory size must be multiple of 0x200000
> >
> > (b) 1G+1byte
> >
> >  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
> >                    -object memory-backend-ram,id=mem1,size=1073741825B \
> >                    -device pc-dimm,id=dimm1,memdev=mem1
> >  -> backend memory size must be multiple of 0x200000
> >
> > (c) Unliagned hugetlb size (2M)
> >
> >  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
> >                    -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=511M \
> >                    -device pc-dimm,id=dimm1,memdev=mem1
> >  backend memory size must be multiple of 0x200000
> >
> > (d) Unliagned hugetlb size (1G)
> >
> >  qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
> >                     -object memory-backend-file,id=mem1,mem-path=/dev/hugepages1G/tmp,size=2047M \
> >                     -device pc-dimm,id=dimm1,memdev=mem1
> >  -> backend memory size must be multiple of 0x40000000
> >
> > Note that this fix depends on a hv-balloon change to communicate its
> > additional alignment requirements using get_min_alignment() instead of
> > through the memory region.
> >
> > [1] https://lkml.kernel.org/r/f77d641d500324525ac036fe1827b3070de75fc1.1701088320.git.mprivozn@redhat.com
> >
> > Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
> > Reported-by: Michal Privoznik <mprivozn@redhat.com>
> > Fixes: eb1b7c4bd413 ("memory-device: Drop size alignment check")
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  hw/mem/memory-device.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> > index a1b1af26bc..e098585cda 100644
> > --- a/hw/mem/memory-device.c
> > +++ b/hw/mem/memory-device.c
> > @@ -374,6 +374,20 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
> >          goto out;
> >      }
> >
> > +    /*
> > +     * We always want the memory region size to be multiples of the memory
> > +     * region alignment: for example, DIMMs with 1G+1byte size don't make
> > +     * any sense. Note that we don't check that the size is multiples
> > +     * of any additional alignment requirements the memory device might
> > +     * have when it comes to the address in physical address space.
> > +     */
> > +    if (!QEMU_IS_ALIGNED(memory_region_size(mr),
> > +                         memory_region_get_alignment(mr))) {
> > +        error_setg(errp, "backend memory size must be multiple of 0x%"
> > +                   PRIx64, memory_region_get_alignment(mr));
> > +        return;
> > +    }
> > +
> >      if (legacy_align) {
> >          align = *legacy_align;
> >      } else {
> > --
> > 2.43.0
> >
>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 0/2] memory-device: reintroduce memory region size check
  2024-01-17 13:55 [PATCH v1 0/2] memory-device: reintroduce memory region size check David Hildenbrand
  2024-01-17 13:55 ` [PATCH v1 1/2] hv-balloon: use get_min_alignment() to express 32 GiB alignment David Hildenbrand
  2024-01-17 13:55 ` [PATCH v1 2/2] memory-device: reintroduce memory region size check David Hildenbrand
@ 2024-01-22 14:34 ` Maciej S. Szmigiero
  2024-01-22 16:49 ` David Hildenbrand
  3 siblings, 0 replies; 8+ messages in thread
From: Maciej S. Szmigiero @ 2024-01-22 14:34 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Mario Casquero, Igor Mammedov, Xiao Guangrong, qemu-devel

Hi David,

On 17.01.2024 14:55, David Hildenbrand wrote:
> Reintroduce a modified region size check, after we would now allow some
> configurations that don't make any sense (e.g., partial hugetlb pages,
> 1G+1byte DIMMs).
> 
> We have to take care of hv-balloon first, which was the case why we
> remove that check in the first place.
> 
> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> Cc: Mario Casquero <mcasquer@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
> 
> David Hildenbrand (2):
>    hv-balloon: use get_min_alignment() to express 32 GiB alignment
>    memory-device: reintroduce memory region size check
> 
>   hw/hyperv/hv-balloon.c | 37 +++++++++++++++++++++----------------
>   hw/mem/memory-device.c | 14 ++++++++++++++
>   2 files changed, 35 insertions(+), 16 deletions(-)
> 

Looked at the changes, tested hv-balloon with a small memory
backend and it seem to work fine, so for the whole series:

Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 0/2] memory-device: reintroduce memory region size check
  2024-01-17 13:55 [PATCH v1 0/2] memory-device: reintroduce memory region size check David Hildenbrand
                   ` (2 preceding siblings ...)
  2024-01-22 14:34 ` [PATCH v1 0/2] " Maciej S. Szmigiero
@ 2024-01-22 16:49 ` David Hildenbrand
  2024-01-22 16:50   ` David Hildenbrand
  3 siblings, 1 reply; 8+ messages in thread
From: David Hildenbrand @ 2024-01-22 16:49 UTC (permalink / raw)
  To: qemu-devel
  Cc: Maciej S. Szmigiero, Mario Casquero, Igor Mammedov,
	Xiao Guangrong

On 17.01.24 14:55, David Hildenbrand wrote:
> Reintroduce a modified region size check, after we would now allow some
> configurations that don't make any sense (e.g., partial hugetlb pages,
> 1G+1byte DIMMs).
> 
> We have to take care of hv-balloon first, which was the case why we
> remove that check in the first place.
> 
> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> Cc: Mario Casquero <mcasquer@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>

Thanks all for resting+review. Queued to

https://github.com/davidhildenbrand/qemu.git mem-next

-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 0/2] memory-device: reintroduce memory region size check
  2024-01-22 16:49 ` David Hildenbrand
@ 2024-01-22 16:50   ` David Hildenbrand
  0 siblings, 0 replies; 8+ messages in thread
From: David Hildenbrand @ 2024-01-22 16:50 UTC (permalink / raw)
  To: qemu-devel
  Cc: Maciej S. Szmigiero, Mario Casquero, Igor Mammedov,
	Xiao Guangrong

On 22.01.24 17:49, David Hildenbrand wrote:
> On 17.01.24 14:55, David Hildenbrand wrote:
>> Reintroduce a modified region size check, after we would now allow some
>> configurations that don't make any sense (e.g., partial hugetlb pages,
>> 1G+1byte DIMMs).
>>
>> We have to take care of hv-balloon first, which was the case why we
>> remove that check in the first place.
>>
>> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>> Cc: Mario Casquero <mcasquer@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
> 
> Thanks all for resting+review. Queued to

Haha, "testing" :)

-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-01-22 16:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-17 13:55 [PATCH v1 0/2] memory-device: reintroduce memory region size check David Hildenbrand
2024-01-17 13:55 ` [PATCH v1 1/2] hv-balloon: use get_min_alignment() to express 32 GiB alignment David Hildenbrand
2024-01-17 13:55 ` [PATCH v1 2/2] memory-device: reintroduce memory region size check David Hildenbrand
2024-01-18  3:08   ` Zhenyu Zhang
2024-01-19  8:35     ` Mario Casquero
2024-01-22 14:34 ` [PATCH v1 0/2] " Maciej S. Szmigiero
2024-01-22 16:49 ` David Hildenbrand
2024-01-22 16:50   ` David Hildenbrand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).