Peter Xu <pet...@redhat.com> writes:
> On Mon, Nov 27, 2023 at 05:25:51PM -0300, Fabiano Rosas wrote:
>> Add the necessary code to parse the format changes for the 'fixed-ram'
>> capability.
>>
>> One of the more notable changes in behavior is that in the 'fixed-ram'
>> case ram pages are restored in one go rather than constantly looping
>> through the migration stream.
>>
>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> - added sanity check for pages_offset alignment
>> - s/parsing/reading
>> - used Error
>> - fixed buffer size computation, now allowing an arbitrary limit
>> - fixed dereference of pointer to packed struct member in endianness
>> conversion
>> ---
>> migration/ram.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 119 insertions(+)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 4a0ab8105f..08604222f2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -106,6 +106,12 @@
>> */
>> #define FIXED_RAM_FILE_OFFSET_ALIGNMENT 0x100000
>>
>> +/*
>> + * When doing fixed-ram migration, this is the amount we read from the
>> + * pages region in the migration file at a time.
>> + */
>> +#define FIXED_RAM_LOAD_BUF_SIZE 0x100000
>> +
>> XBZRLECacheStats xbzrle_counters;
>>
>> /* used by the search for pages to send */
>> @@ -2996,6 +3002,35 @@ static void fixed_ram_insert_header(QEMUFile *file,
>> RAMBlock *block)
>> qemu_put_buffer(file, (uint8_t *) header, header_size);
>> }
>>
>> +static bool fixed_ram_read_header(QEMUFile *file, FixedRamHeader *header,
>> + Error **errp)
>> +{
>> + size_t ret, header_size = sizeof(FixedRamHeader);
>> +
>> + ret = qemu_get_buffer(file, (uint8_t *)header, header_size);
>> + if (ret != header_size) {
>> + error_setg(errp, "Could not read whole fixed-ram migration header "
>> + "(expected %zd, got %zd bytes)", header_size, ret);
>> + return false;
>> + }
>> +
>> + /* migration stream is big-endian */
>> + header->version = be32_to_cpu(header->version);
>> +
>> + if (header->version > FIXED_RAM_HDR_VERSION) {
>> + error_setg(errp, "Migration fixed-ram capability version mismatch "
>> + "(expected %d, got %d)", FIXED_RAM_HDR_VERSION,
>> + header->version);
>> + return false;
>> + }
>> +
>> + header->page_size = be64_to_cpu(header->page_size);
>> + header->bitmap_offset = be64_to_cpu(header->bitmap_offset);
>> + header->pages_offset = be64_to_cpu(header->pages_offset);
>> +
>> + return true;
>> +}
>> +
>> /*
>> * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>> * long-running RCU critical section. When rcu-reclaims in the code
>> @@ -3892,6 +3927,80 @@ void colo_flush_ram_cache(void)
>> trace_colo_flush_ram_cache_end();
>> }
>>
>> +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
>> + long num_pages, unsigned long *bitmap)
>> +{
>> + unsigned long set_bit_idx, clear_bit_idx;
>> + ram_addr_t offset;
>> + void *host;
>> + size_t read, unread, size, buf_size = FIXED_RAM_LOAD_BUF_SIZE;
>> +
>> + for (set_bit_idx = find_first_bit(bitmap, num_pages);
>> + set_bit_idx < num_pages;
>> + set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1))
>> {
>> +
>> + clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx +
>> 1);
>> +
>> + unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx);
>> + offset = set_bit_idx << TARGET_PAGE_BITS;
>> +
>> + while (unread > 0) {
>> + host = host_from_ram_block_offset(block, offset);
>> + size = MIN(unread, buf_size);
>
> Use the macro directly? buf_size can be dropped then.
>
Ok. We only need it later when multifd support is added to this
function.
>> +
>> + read = qemu_get_buffer_at(f, host, size,
>> + block->pages_offset + offset);
>
> Error detection missing? qemu_get_buffer_at() returns 0 if error, then it
> dead loops.
>
Ah right, I was expecting we'd have a direction on how to improve the
qemu-file error handling before I sent this version and ended up
forgetting to do something about it.
>> + offset += read;
>> + unread -= read;
>> + }
>> + }
>> +}
>> +
>> +static int parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
>> + ram_addr_t length, Error **errp)
>
> For new code, start to keep using boolean as retval when Error** exists?
>
Yep.
>> +{
>> + g_autofree unsigned long *bitmap = NULL;
>> + FixedRamHeader header;
>> + size_t bitmap_size;
>> + long num_pages;
>> +
>> + if (!fixed_ram_read_header(f, &header, errp)) {
>> + return -EINVAL;
>> + }
>> +
>> + block->pages_offset = header.pages_offset;
>> +
>> + /*
>> + * Check the alignment of the file region that contains pages. We
>> + * don't enforce FIXED_RAM_FILE_OFFSET_ALIGNMENT to allow that
>> + * value to change in the future. Do only a sanity check with page
>> + * size alignment.
>> + */
>> + if (!QEMU_IS_ALIGNED(block->pages_offset, TARGET_PAGE_SIZE)) {
>> + error_setg(errp,
>> + "Error reading ramblock %s pages, region has bad
>> alignment",
>> + block->idstr);
>> + return -EINVAL;
>> + }
>> +
>> + num_pages = length / header.page_size;
>> + bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long);
>> +
>> + bitmap = g_malloc0(bitmap_size);
>> + if (qemu_get_buffer_at(f, (uint8_t *)bitmap, bitmap_size,
>> + header.bitmap_offset) != bitmap_size) {
>> + error_setg(errp, "Error reading dirty bitmap");
>> + return -EINVAL;
>> + }
>> +
>> + read_ramblock_fixed_ram(f, block, num_pages, bitmap);
>
> Detect error and fail properly?
>
Ok.
>> +
>> + /* Skip pages array */
>> + qemu_set_offset(f, block->pages_offset + length, SEEK_SET);
>> +
>> + return 0;
>> +}
>> +
>> static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>> {
>> int ret = 0;
>> @@ -3900,6 +4009,16 @@ static int parse_ramblock(QEMUFile *f, RAMBlock
>> *block, ram_addr_t length)
>>
>> assert(block);
>>
>> + if (migrate_fixed_ram()) {
>> + Error *local_err = NULL;
>> +
>> + ret = parse_ramblock_fixed_ram(f, block, length, &local_err);
>> + if (local_err) {
>> + error_report_err(local_err);
>> + }
>> + return ret;
>
> We can optionally add one pre-requisite patch to convert parse_ramblock()
> to return boolean too. I remember it was done somewhere before, but maybe
> not merged.
>
I don't think we changed the return type. There was only a refactoring
at commit 2f5ced5b. I'll change to boolean if possible.
>> + }
>> +
>> if (!qemu_ram_is_migratable(block)) {
>> error_report("block %s should not be migrated !", block->idstr);
>> return -EINVAL;
>> --
>> 2.35.3
>>
