Re: [PATCH v4 2/7] system/physmem: poisoned memory discard on reboot

[William Roche <william.roche@oracle.com>]

On 1/8/25 22:44, David Hildenbrand wrote:
> On 14.12.24 14:45, “William Roche wrote:
>> +/* Try to simply remap the given location */
>> +static void qemu_ram_remap_mmap(RAMBlock *block, void* vaddr, size_t 
>> size,
>> +                                ram_addr_t offset)
> 
> Can you make the parameters match the ones of ram_block_discard_range() 
> so the invocation gets easier to read? You can calculate vaddr easily 
> internally.
> 
> Something like
> 
> static void qemu_ram_remap_mmap(RAMBlock *rb, uint64_t start,
>                  size_t length)

I used the same arguments as ram_block_discard_range() as you asked.

> 
>> +{
>> +    int flags, prot;
>> +    void *area;
>> +
>> +    flags = MAP_FIXED;
>> +    flags |= block->flags & RAM_SHARED ? MAP_SHARED : MAP_PRIVATE;
>> +    flags |= block->flags & RAM_NORESERVE ? MAP_NORESERVE : 0;
>> +    prot = PROT_READ;
>> +    prot |= block->flags & RAM_READONLY ? 0 : PROT_WRITE;
>> +    if (block->fd >= 0) {
> 
> Heh, that case can no longer happen!

I also removed the used case of remapping a file in the 
qemu_ram_remap_mmap() function.

> 
> assert(block->fs < 0);

And added the assert() you suggested.


>> +                if (ram_block_discard_range(block, offset + block->fd_offset,
>> +                                            page_size) != 0) {


Studying some more the arguments used by ram_block_discard_range() and 
the need to fallocate/Punch the underlying file, I think that I should 
simply provide the 'offset' here and that block->fd_offset is missing in 
the ram_block_discard_range() function where we have to punch a hole in 
the file. Don't you agree ?

If we can get the current set of fixes integrated, I'll submit another 
fix proposal to take the fd_offset into account in a second time. (Not 
enlarging the current set)

But here is what I'm thinking about. That we can discuss later if you want:

@@ -3730,11 +3724,12 @@ int ram_block_discard_range(RAMBlock *rb, 
uint64_t start, size_t length)
              }

              ret = fallocate(rb->fd, FALLOC_FL_PUNCH_HOLE | 
FALLOC_FL_KEEP_SIZE,
-                            start, length);
+                            start + rb->fd_offset, length);
              if (ret) {
                  ret = -errno;
                  error_report("%s: Failed to fallocate %s:%" PRIx64 " 
+%zx (%d)",
-                             __func__, rb->idstr, start, length, ret);
+                             __func__, rb->idstr, start + rb->fd_offset,
+                            length, ret);
                  goto err;
              }


Or I can integrate that as an addition patch if you prefer.




>> +                    /*
>> +                     * Fold back to using mmap() only for anonymous mapping,
> 
> s/Fold/Fall/

typo fixed



>>                   }
>>                   memory_try_enable_merging(vaddr, page_size);
>>                   qemu_ram_setup_dump(vaddr, page_size);
> 
> These two can be moved into qemu_ram_remap_mmap(). They are not required 
> if we didn't actually mess with mmap().


These functions will be replaced by the ram_block_notify_remap() of 
patch 7 which is called no matter the ram_block_discard_range() 
succeeded or not.
So we should leave these 2 function calls here for now as they mimic an 
aspect of what the notifier code will do.