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

[Richard Henderson]

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?

> +#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.

> +/**
> + * 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?

> +    /**
> +     * 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~