On 01/19/17 18:47, Ben Warren wrote: > Thanks Laszlo! >> On Jan 19, 2017, at 1:25 AM, Laszlo Ersek <ler...@redhat.com >> <mailto:ler...@redhat.com>> wrote: >> >> On 01/19/17 08:09, Ben Warren wrote: >>> >>>> On Jan 18, 2017, at 4:02 PM, Ben Warren <b...@skyportsystems.com >>>> <mailto:b...@skyportsystems.com>> >>>> wrote: >>>> >>>> Hi Michael, >>>>> On Jan 17, 2017, at 9:45 AM, Michael S. Tsirkin <m...@redhat.com >>>>> <mailto:m...@redhat.com>> >>>>> wrote: >>>>> >>>>> On Mon, Jan 16, 2017 at 10:57:42AM -0800, Ben Warren wrote: >>>>>> I think we have a misunderstanding here. I'm storing the VM >>>>>> Generation ID __data__ (a GUID) in a fw_cfg blob, not the address. >>>>> >>>>> Yes, I think I gathered this much from the discussion. This is what >>>>> I'm saying - don't. Have guest loader reserve guest memory and write >>>>> the address into a fw cfg blob, and have qemu write the id at that >>>>> address. This way you can update guest memory at any time. >>>>> >>>> So I've gone down the path of creating a writeable fw_cfg blob to >>>> hold the VGID address, but it doesn't seem to be getting updated. >>>> >>>> Here's the code I've added: >>>> >>>> #define VMGENID_FW_CFG_FILE "etc/vmgenid" >>>> #define VMGENID_FW_CFG_ADDR_FILE "etc/vmgenid_addr" >>>> >>>> // Create writeable fw_cfg blob, vas->vgia is a GArray of size 8 and >>>> element size 1 >>>> fw_cfg_add_file_callback(s, VMGENID_FW_CFG_ADDR_FILE, NULL, NULL, >>>> vms->vgia->data, 8, false); >>>> >>>> // Request BIOS to allocate memory for the read-only DATA file: >>>> bios_linker_loader_alloc(linker, VMGENID_FW_CFG_FILE, guid, 0,false); >>>> >>>> // Request BIOS to allocate memory for the writeable ADDRESS file: >>>> bios_linker_loader_alloc(linker, VMGENID_FW_CFG_ADDR_FILE, s->vgia, >>>> 0, false); >>>> >>>> // Request BIOS to write the address of the DATA file into the >>>> ADDRESS file: >>>> bios_linker_loader_add_pointer(linker, VMGENID_FW_CFG_ADDR_FILE, 0, >>>> sizeof(uint64_t), VMGENID_FW_CFG_FILE, 0); >>>> >>>> I've instrumented SeaBIOS and see the requests being made and memcpy >>>> to the file happening, but don't see any changes in QEMU in the >>>> memory pointed to by VMGENID_FW_CFG_ADDR_FILE. Is this how writeable >>>> fw_cfg is supposed to work? >>>> >>> Ed explained it to me and I think I get it now. I realize that you >>> and Igor have explained this throughout the e-mail chain, but I'm a >>> little bit slower. >>> >>> Anyway, is this understanding correct? BIOS is in fact patching >>> fw_cfg data, but it's in a copy of the fw_cfg file that is in guest >>> space. In order to get this to work we would need to add a new >>> command to the linker/loader protocol to write back changes to QEMU >>> after patching, and of course implement the change in BIOS and UEFI >>> projects. >> >> (1) It's not enough to create just the "etc/vmgenid_addr" file; the >> "etc/vmgenid" file must exist too, so that the firmware can download it. >> > I do have that file too, just didn’t show it. >> (2) I don't understand why you need the ADD_POINTER command here. I >> think it's unnecessary. >> >> (3) The ALLOCATE command for "etc/vmgenid_addr" is also unnecessary. >> > For both of these, I was working within the confines of the existing > API, which we now know is inadequate. >> (4) A new ALLOCATE_RETURN_ADDR command type should be introduced, with >> the following contents: >> >> struct { >> char file[BIOS_LINKER_LOADER_FILESZ]; >> uint32_t align; >> uint8_t zone; >> char addr_file[BIOS_LINKER_LOADER_FILESZ]; >> } alloc_return_addr; >> >> The first three fields have identical meanings to those of the current >> ALLOCATE command. The last field instructs the firmware as to what >> fw_cfg file to write the 8-byte physical address back to, in little >> endian byte order, of the actual allocation. >> >> (5) The linker/loader script should then use one command in total, >> namely ALLOCATE_RETURN_ADDR, with >> >> file = etc/vmgenid >> addr_file = etc/vmgenid_addr >> >> This will allow both SeaBIOS and OVMF to do the right thing. >> >> In summary: >> - create the read-only "etc/vmgenid" fw_cfg file >> - create the writeable "etc/vmgenid_addr" fw_cfg file >> - use one ALLOCATE_RETURN_ADDR command, and nothing else. >> >> I hereby volunteer to write the OVMF patch for the ALLOCATE_RETURN_ADDR >> command type. >> > Sounds great. We use SeaBIOS so I’ll try to do the same there. > > So, just to make sure everything’s covered: this new mechanism will > allow QEMU to have the guest allocate an arbitrary blob of memory (in > the form of a fw_cfg file),
Yes. > and will get an offset within guest memory > in return (via another fw_cfg file). Yes. The information that QEMU will receive is the LE-encoded 8-byte GPA (guest-physical address) of the allocation carried out by the firmware. > It can then translate the guest > offset into a host address Yes. I'm a bit rusty on the exact QEMU memory APIs here, but I think that ram_addr_t is *not* the right source address type to convert from (to HVA, host virtual address). I.e., what the firmware returns should *not* be considered a ram_addr_t; IIRC, ram_addr_t is a linear offset into RAMBlocks that does not take, for example, the 32-bit PCI MMIO hole into account. Instead, the value written by the firmware should be considered a hwaddr. (See "include/exec/hwaddr.h".) Therefore you'll have to find a translation from hwaddr to HVA. (I'm sure seasoned QEMU developers will correct me on the APIs / src address type if necessary.) > and update the blob at will. Yes. (And then Igor said something about dirtying that memory after the QEMU write...) > Is this > correct, because that’s what we need for VM Generation ID? Yes, it seems correct to me. Thanks Laszlo > >> If someone is interested, I'll remind us that EFI_ACPI_TABLE_PROTOCOL >> installs *copies* of ACPI tables, out of the fw_cfg blobs that were >> downloaded. Therefore OVMF tracks all ADD_POINTER commands that point >> into fw_cfg blobs, and if an fw_cfg blob is not pointed-to by *any* >> ADD_POINTER command, or else *all* ADD_POINTER commands that point into >> it point to ACPI table headers, then OVMF will ultimately free the >> originally downloaded fw_cfg blob. If there is at least one referring >> ADD_POINTER command that points to non-ACPI-table data within the blob, >> then OVMF marks the blob to be preserved permanently, in AcpiNVS type >> memory. >> >> By introducing the above ALLOCATE_RETURN_ADDR command type, OVMF can do >> two things: >> >> (a) immediately mark the blob for permanent preservation (i.e., it won't >> be released after the script processing is complete), >> (b) write the actual allocation address back to the appropriate fw_cfg >> file. >> >> For SeaBIOS, only (b) matters -- because it doesn't install *copies* of >> ACPI tables; it rather keeps everything in place, as originally >> allocated, and it just links things together --, but >> ALLOCATE_RETURN_ADDR as proposed above should be suitable for SeaBIOS >> just as well. >> >> Thanks >> Laszlo > —Ben >