[PATCH] hw/core/machine: diagnose wrapping of maxmem

[PATCH] hw/core/machine: diagnose wrapping of maxmem

[Daniel P. Berrangé]

The 'maxmem' parameter parsed on the command line is held in uint64_t
and then assigned to the MachineState field that is 'ram_addr_t'. This
assignment will wrap on 32-bit hosts, silently changing the user's
config request if it were over-sized.

Improve the existing diagnositics for validating 'size', and add the
same diagnostics for 'maxmem'

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/machine.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a35c4a8fae..f29fe95964 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -598,11 +598,19 @@ static void machine_set_mem(Object *obj, Visitor *v, const char *name,
         mem->size = mc->fixup_ram_size(mem->size);
     }
     if ((ram_addr_t)mem->size != mem->size) {
-        error_setg(errp, "ram size too large");
+        error_setg(errp, "ram size %llu exceeds permitted maximum %llu",
+                   (unsigned long long)mem->size,
+                   (unsigned long long)RAM_ADDR_MAX);
         goto out_free;
     }
 
     if (mem->has_max_size) {
+        if ((ram_addr_t)mem->max_size != mem->max_size) {
+            error_setg(errp, "ram size %llu exceeds permitted maximum %llu",
+                       (unsigned long long)mem->max_size,
+                       (unsigned long long)RAM_ADDR_MAX);
+            goto out_free;
+        }
         if (mem->max_size < mem->size) {
             error_setg(errp, "invalid value of maxmem: "
                        "maximum memory size (0x%" PRIx64 ") must be at least "
-- 
2.46.0

Re: [PATCH] hw/core/machine: diagnose wrapping of maxmem

[Ani Sinha]

On Wed, Nov 27, 2024 at 5:11 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The 'maxmem' parameter parsed on the command line is held in uint64_t
> and then assigned to the MachineState field that is 'ram_addr_t'. This
> assignment will wrap on 32-bit hosts, silently changing the user's
> config request if it were over-sized.

With this patch, for the failing test_mem_addr_space test in 32-bit, we get:

Traceback (most recent call last):
  File "/builds/qemu/tests/functional/test_mem_addr_space.py", line
256, in test_phybits_low_tcg_q35_71_amd_41bits
    self.assertRegex(self.vm.get_log(), r'phys-bits too low')
AssertionError: Regex didn't match: 'phys-bits too low' not found in
'qemu-system-x86_64: ram size 1065151889408 exceeds permitted maximum
4294967295\n'

Traceback (most recent call last):
  File "/builds/qemu/tests/functional/test_mem_addr_space.py", line
294, in test_phybits_low_tcg_q35_intel_cxl
    self.assertRegex(self.vm.get_log(), r'phys-bits too low')
AssertionError: Regex didn't match: 'phys-bits too low' not found in
'qemu-system-x86_64: ram size 1059783180288 exceeds permitted maximum
4294967295\n'

etc.

>
> Improve the existing diagnositics for validating 'size', and add the
> same diagnostics for 'maxmem'
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Tested by: Ani Sinha <anisinha@redhat.com>
Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
>  hw/core/machine.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a35c4a8fae..f29fe95964 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -598,11 +598,19 @@ static void machine_set_mem(Object *obj, Visitor *v, const char *name,
>          mem->size = mc->fixup_ram_size(mem->size);
>      }
>      if ((ram_addr_t)mem->size != mem->size) {
> -        error_setg(errp, "ram size too large");
> +        error_setg(errp, "ram size %llu exceeds permitted maximum %llu",
> +                   (unsigned long long)mem->size,
> +                   (unsigned long long)RAM_ADDR_MAX);
>          goto out_free;
>      }
>
>      if (mem->has_max_size) {
> +        if ((ram_addr_t)mem->max_size != mem->max_size) {
> +            error_setg(errp, "ram size %llu exceeds permitted maximum %llu",
> +                       (unsigned long long)mem->max_size,
> +                       (unsigned long long)RAM_ADDR_MAX);
> +            goto out_free;
> +        }
>          if (mem->max_size < mem->size) {
>              error_setg(errp, "invalid value of maxmem: "
>                         "maximum memory size (0x%" PRIx64 ") must be at least "
> --
> 2.46.0
>

Re: [PATCH] hw/core/machine: diagnose wrapping of maxmem

[Philippe Mathieu-Daudé]

On 27/11/24 12:40, Daniel P. Berrangé wrote:
> The 'maxmem' parameter parsed on the command line is held in uint64_t
> and then assigned to the MachineState field that is 'ram_addr_t'. This
> assignment will wrap on 32-bit hosts, silently changing the user's
> config request if it were over-sized.
> 
> Improve the existing diagnositics for validating 'size', and add the
> same diagnostics for 'maxmem'
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   hw/core/machine.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)

Patch queued, thanks.