Re: [PATCH v2 2/4] Memory: Enable writeback for given memory region

[Beata Michalska]

On Wed, 6 Nov 2019 at 12:20, Richard Henderson
<address@hidden> wrote:
>
> On 11/6/19 12:40 AM, Beata Michalska wrote:
> > +void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t 
> > length)
> > +{
> > +    void *addr = ramblock_ptr(block, start);
> > +
> > +    /*
> > +     * The requested range might spread up to the very end of the block
> > +     */
> > +    if ((start + length) > block->used_length) {
> > +        qemu_log("%s: sync range outside the block boundaries: "
> > +                     "start: " RAM_ADDR_FMT " length: " RAM_ADDR_FMT
> > +                     " block length: " RAM_ADDR_FMT " Narrowing down ..." ,
> > +                     __func__, start, length, block->used_length);
> > +        length = block->used_length - start;
> > +    }
>
> qemu_log_mask w/ GUEST_ERROR?  How do we expect the length to overflow?

In theory it shouldn't, at least with current usage.
I guess the probe_access will make sure of that.
This was more of a precaution to enable catching potential/future misuses
aka debugging purpose. I can get rid of that it that's playing too safe.

>
> > +#ifdef CONFIG_LIBPMEM
> > +    /* The lack of support for pmem should not block the sync */
> > +    if (ramblock_is_pmem(block)) {
> > +        pmem_persist(addr, length);
> > +    } else
> > +#endif
>
> Perhaps better to return out of that if block than have the dangling else.

Good idea
>
> > +/**
> > + * Sync changes made to the memory mapped file back to the backing
> > + * storage. For POSIX compliant systems this will simply fallback
> > + * to regular msync call (thus the required alignment). Otherwise
> > + * it will trigger whole file sync (including the metadata case
> > + * there is no support to skip that otherwise)
> > + *
> > + * @addr   - start of the memory area to be synced
> > + * @length - length of the are to be synced
> > + * @align  - alignment (expected to be PAGE_SIZE)
> > + * @fd     - file descriptor for the file to be synced
> > + *           (mandatory only for POSIX non-compliant systems)
> > + */
> > +int qemu_msync(void *addr, size_t length, size_t align, int fd)
> > +{
> > +#ifdef CONFIG_POSIX
> > +    size_t align_mask;
> > +
> > +    /* Bare minimum of sanity checks on the alignment */
> > +    /* The start address needs to be a multiple of PAGE_SIZE */
> > +    align = MAX(align, qemu_real_host_page_size);
> > +    align_mask = ~(qemu_real_host_page_size - 1);
> > +    align = (align + ~align_mask) & align_mask;
> > +
> > +    align_mask = ~(align - 1);
>
> I don't understand what you're trying to do with align.
>
> You pass in qemu_host_page_size from the one caller, and then adjust it for
> qemu_real_host_page_size?
>
> Why pass in anything at all, and just use qemu_real_host_page_mask?

The qemu_msync was supposed to be generic and not tied to current use case
without any assumptions on the alignment and whether that would  be an actual
host page size. So that was just to make sure it will be a multiple of that.
I can get rid of that with assumption all will be using the same alignment.

BR
Beata
>
> > +    /**
> > +     * There are no strict reqs as per the length of mapping
> > +     * to be synced. Still the length needs to follow the address
> > +     * alignment changes. Additionally - round the size to the multiple
> > +     * of requested alignment (expected as PAGE_SIZE)
> > +     */
> > +    length += ((uintptr_t)addr & (align - 1));
> > +    length = (length + ~align_mask) & align_mask;
> > +
> > +    addr = (void *)((uintptr_t)addr & align_mask);
> > +
> > +    return msync(addr, length, MS_SYNC);
> > +#else /* CONFIG_POSIX */
> > +    /**
> > +     * Perform the sync based on the file descriptor
> > +     * The sync range will most probably be wider than the one
> > +     * requested - but it will still get the job done
> > +     */
> > +    return qemu_fdatasync(fd);
> > +#endif /* CONFIG_POSIX */
> > +}
>
>
> r~
>