GuestPhysBlockList is currently used to produce dumps. Given the size
and the typical usage of NVDIMM for storage, they are not a good idea
to have in the dumps. We may want to have an extra dump option to
include them. For now, skip non-volatile regions.

The TCG memory clear function is going to use the GuestPhysBlockList
as well, and will thus skip NVDIMM for similar reasons.

Cc: lersek@redhat.com
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 memory_mapping.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/memory_mapping.c b/memory_mapping.c
index 775466f3a8..724dd0b417 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
 
     /* we only care about RAM */
     if (!memory_region_is_ram(section->mr) ||
-        memory_region_is_ram_device(section->mr)) {
+        memory_region_is_ram_device(section->mr) ||
+        memory_region_is_nonvolatile(section->mr)) {
         return;
     }
 
-- 
2.19.0.271.gfe8321ec05

On 10/03/18 13:44, Marc-André Lureau wrote:
> GuestPhysBlockList is currently used to produce dumps. Given the size
> and the typical usage of NVDIMM for storage, they are not a good idea
> to have in the dumps. We may want to have an extra dump option to
> include them. For now, skip non-volatile regions.
> 
> The TCG memory clear function is going to use the GuestPhysBlockList
> as well, and will thus skip NVDIMM for similar reasons.
> 
> Cc: lersek@redhat.com
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  memory_mapping.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 775466f3a8..724dd0b417 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
>  
>      /* we only care about RAM */
>      if (!memory_region_is_ram(section->mr) ||
> -        memory_region_is_ram_device(section->mr)) {
> +        memory_region_is_ram_device(section->mr) ||
> +        memory_region_is_nonvolatile(section->mr)) {
>          return;
>      }
>  
> 

I've peeked at the first two patches as well. Seems OK to me. (Famous
last words?)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 10/03/18 13:44, Marc-André Lureau wrote:
> > GuestPhysBlockList is currently used to produce dumps. Given the size
> > and the typical usage of NVDIMM for storage, they are not a good idea
> > to have in the dumps. We may want to have an extra dump option to
> > include them. For now, skip non-volatile regions.
> > 
> > The TCG memory clear function is going to use the GuestPhysBlockList
> > as well, and will thus skip NVDIMM for similar reasons.
> > 
> > Cc: lersek@redhat.com
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  memory_mapping.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/memory_mapping.c b/memory_mapping.c
> > index 775466f3a8..724dd0b417 100644
> > --- a/memory_mapping.c
> > +++ b/memory_mapping.c
> > @@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
> >  
> >      /* we only care about RAM */
> >      if (!memory_region_is_ram(section->mr) ||
> > -        memory_region_is_ram_device(section->mr)) {
> > +        memory_region_is_ram_device(section->mr) ||
> > +        memory_region_is_nonvolatile(section->mr)) {
> >          return;
> >      }
> >  
> > 
> 
> I've peeked at the first two patches as well. Seems OK to me. (Famous
> last words?)
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

This also looks good to me; just cc'ing in David H as well though.

Dave

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

On 10/10/2018 11:44, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
>> On 10/03/18 13:44, Marc-André Lureau wrote:
>>> GuestPhysBlockList is currently used to produce dumps. Given the size
>>> and the typical usage of NVDIMM for storage, they are not a good idea
>>> to have in the dumps. We may want to have an extra dump option to
>>> include them. For now, skip non-volatile regions.
>>>
>>> The TCG memory clear function is going to use the GuestPhysBlockList
>>> as well, and will thus skip NVDIMM for similar reasons.
>>>
>>> Cc: lersek@redhat.com
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  memory_mapping.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/memory_mapping.c b/memory_mapping.c
>>> index 775466f3a8..724dd0b417 100644
>>> --- a/memory_mapping.c
>>> +++ b/memory_mapping.c
>>> @@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
>>>  
>>>      /* we only care about RAM */
>>>      if (!memory_region_is_ram(section->mr) ||
>>> -        memory_region_is_ram_device(section->mr)) {
>>> +        memory_region_is_ram_device(section->mr) ||
>>> +        memory_region_is_nonvolatile(section->mr)) {
>>>          return;
>>>      }
>>>  
>>>
>>
>> I've peeked at the first two patches as well. Seems OK to me. (Famous
>> last words?)
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> This also looks good to me; just cc'ing in David H as well though.
> 

Thanks Dave. Yes, just like the guest will exclude NVDIMMs from dumps,
so should we. (if somebody ever want to have this e.g. because the
NVDIMM is based on RAM in the host, we can introduce what you describe -
extra dump option).

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb

On 03/10/2018 13:44, Marc-André Lureau wrote:
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 775466f3a8..724dd0b417 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
>  
>      /* we only care about RAM */
>      if (!memory_region_is_ram(section->mr) ||
> -        memory_region_is_ram_device(section->mr)) {
> +        memory_region_is_ram_device(section->mr) ||
> +        memory_region_is_nonvolatile(section->mr)) {
>          return;
>      }
>  

We should also have

diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index 5a857cebcf..dd180b531c 100644
--- a/scripts/dump-guest-memory.py
+++ b/scripts/dump-guest-memory.py
@@ -417,7 +417,9 @@ def get_guest_phys_blocks():
         memory_region = flat_range["mr"].dereference()

         # we only care about RAM
-        if not memory_region["ram"]:
+        if not memory_region["ram"] \
+           or memory_region["ram_device"] \
+           or memory_region["nonvolatile"]:
             continue

         section_size = int128_get64(flat_range["addr"]["size"])

here.  I queued the patches and will post this soon as a separate patch.

Paolo

On 10/29/18 10:50, Paolo Bonzini wrote:
> On 03/10/2018 13:44, Marc-André Lureau wrote:
>> diff --git a/memory_mapping.c b/memory_mapping.c
>> index 775466f3a8..724dd0b417 100644
>> --- a/memory_mapping.c
>> +++ b/memory_mapping.c
>> @@ -206,7 +206,8 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
>>  
>>      /* we only care about RAM */
>>      if (!memory_region_is_ram(section->mr) ||
>> -        memory_region_is_ram_device(section->mr)) {
>> +        memory_region_is_ram_device(section->mr) ||
>> +        memory_region_is_nonvolatile(section->mr)) {
>>          return;
>>      }
>>  
> 
> We should also have
> 
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index 5a857cebcf..dd180b531c 100644
> --- a/scripts/dump-guest-memory.py
> +++ b/scripts/dump-guest-memory.py
> @@ -417,7 +417,9 @@ def get_guest_phys_blocks():
>          memory_region = flat_range["mr"].dereference()
> 
>          # we only care about RAM
> -        if not memory_region["ram"]:
> +        if not memory_region["ram"] \
> +           or memory_region["ram_device"] \
> +           or memory_region["nonvolatile"]:
>              continue
> 
>          section_size = int128_get64(flat_range["addr"]["size"])
> 
> here.  I queued the patches and will post this soon as a separate patch.

Thanks. I keep forgetting that this logic is duplicated.

Laszlo