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

[William Roche]

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.