[v2] memory-device: complete refactoring + virtio-pmem

[Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small

Posted by David Hildenbrand 7 years, 5 months ago

The "at" should actually be a "before".
    if (new_addr < address_space_start)
     -> "can't add memory ... before... $address_space_start"

So it looks similar to the other check
    } else if ((new_addr + size) > address_space_end)
     -> "can't add memory ... beyond..."

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 6de4f70bb4..efacbc2a7d 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
         new_addr = *hint;
         if (new_addr < address_space_start) {
             error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
-                       "] at 0x%" PRIx64, new_addr, size, address_space_start);
+                       "] before 0x%" PRIx64, new_addr, size,
+                       address_space_start);
             return 0;
         } else if ((new_addr + size) > address_space_end) {
             error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small

Posted by Dr. David Alan Gilbert 7 years, 5 months ago

* David Hildenbrand (david@redhat.com) wrote:
> The "at" should actually be a "before".
>     if (new_addr < address_space_start)
>      -> "can't add memory ... before... $address_space_start"
> 
> So it looks similar to the other check
>     } else if ((new_addr + size) > address_space_end)
>      -> "can't add memory ... beyond..."
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/memory-device.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 6de4f70bb4..efacbc2a7d 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
>          new_addr = *hint;
>          if (new_addr < address_space_start) {
>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
> -                       "] at 0x%" PRIx64, new_addr, size, address_space_start);
> +                       "] before 0x%" PRIx64, new_addr, size,
> +                       address_space_start);

OK, so


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>              return 0;
>          } else if ((new_addr + size) > address_space_end) {

That comparison (that's already there) doesn't look wrap-safe?

Dave
>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64

> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small

Posted by David Hildenbrand 7 years, 4 months ago

Am 30.08.18 um 10:53 schrieb Dr. David Alan Gilbert:
> * David Hildenbrand (david@redhat.com) wrote:
>> The "at" should actually be a "before".
>>     if (new_addr < address_space_start)
>>      -> "can't add memory ... before... $address_space_start"
>>
>> So it looks similar to the other check
>>     } else if ((new_addr + size) > address_space_end)
>>      -> "can't add memory ... beyond..."
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/memory-device.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index 6de4f70bb4..efacbc2a7d 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
>>          new_addr = *hint;
>>          if (new_addr < address_space_start) {
>>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
>> -                       "] at 0x%" PRIx64, new_addr, size, address_space_start);
>> +                       "] before 0x%" PRIx64, new_addr, size,
>> +                       address_space_start);
> 
> OK, so
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
>>              return 0;
>>          } else if ((new_addr + size) > address_space_end) {
> 
> That comparison (that's already there) doesn't look wrap-safe?
> 

Indeed, but it is not the only one in this file. I'll add a patch for
these. Thanks!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small

Posted by Igor Mammedov 7 years, 5 months ago

On Wed, 29 Aug 2018 17:36:05 +0200
David Hildenbrand <david@redhat.com> wrote:

> The "at" should actually be a "before".
>     if (new_addr < address_space_start)
>      -> "can't add memory ... before... $address_space_start"  
> 
> So it looks similar to the other check
>     } else if ((new_addr + size) > address_space_end)
>      -> "can't add memory ... beyond..."  
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/memory-device.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 6de4f70bb4..efacbc2a7d 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
>          new_addr = *hint;
>          if (new_addr < address_space_start) {
>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
> -                       "] at 0x%" PRIx64, new_addr, size, address_space_start);
> +                       "] before 0x%" PRIx64, new_addr, size,
> +                       address_space_start);
>              return 0;
>          } else if ((new_addr + size) > address_space_end) {
>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64