Re: [RFC PATCH v2 20/44] i386/tdx: Parse tdx metadata and store the result into TdxGuestState

[Gerd Hoffmann]

  Hi,

> +        /*
> +         * If TDVF temp memory describe in TDVF metadata lays in RAM, reserve
> +         * the region property.
> +         */
> +        if (entry->address >= 4 * GiB + x86ms->above_4g_mem_size ||
> +            entry->address + entry->size >= 4 * GiB + 
> x86ms->above_4g_mem_size) {
> +            error_report("TDVF type %u address 0x%" PRIx64 " size 0x%" PRIx64
> +                         " above high memory",
> +                         entry->type, entry->address, entry->size);
> +            exit(1);
> +        }

I think you can simply use dma_memory_map() API, then just work with
guest physical addresses and drop the messy and error-prone memory
region offset calculations.

> +    entry->mem_ptr = memory_region_get_ram_ptr(entry->mr);
> +    if (entry->data_len) {
> +        /*
> +         * The memory_region api doesn't allow partial file mapping, create
> +         * ram and copy the contents
> +         */
> +        if (lseek(fd, entry->data_offset, SEEK_SET) != entry->data_offset) {
> +            error_report("can't seek to 0x%x %s", entry->data_offset, 
> filename);
> +            exit(1);
> +        }
> +        if (read(fd, entry->mem_ptr, entry->data_len) != entry->data_len) {
> +            error_report("can't read 0x%x %s", entry->data_len, filename);
> +            exit(1);
> +        }
> +    }

Wouldn't a simple rom_add_blob work here?

> +int load_tdvf(const char *filename)
> +{

> +    for_each_fw_entry(fw, entry) {
> +        if (entry->address < x86ms->below_4g_mem_size ||
> +            entry->address > 4 * GiB) {
> +            tdvf_init_ram_memory(ms, entry);
> +        } else {
> +            tdvf_init_bios_memory(fd, filename, entry);
> +        }
> +    }

Why there are two different ways to load the firmware?

Also: why is all this firmware volume parsing needed?  The normal ovmf
firmware can simply be mapped just below 4G, why can't tdvf work the
same way?

thanks,
  Gerd