On 01/27/17 19:19, Ben Warren wrote: > >> On Jan 27, 2017, at 8:12 AM, Laszlo Ersek <ler...@redhat.com >> <mailto:ler...@redhat.com>> wrote: >> >> On 01/27/17 16:43, Kevin O'Connor wrote: >>> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote: >>>> On 01/27/17 15:18, Kevin O'Connor wrote: >>>>> If an offset is going to be added, shouldn't both a source offset and >>>>> destination offset be used? >>>>> >>>>> /* >>>>> * COMMAND_WRITE_POINTER - update a writeable file named >>>>> * @pointer.dest_file at @pointer.dest_offset, by writing >>>>> pointer >>>>> * plus @pointer.src_offset to the blob originating from >>>>> * @src_file. 1,2,4 or 8 byte unsigned write is used depending >>>>> * on @pointer.size. >>>>> */ >>>>> struct { >>>>> char dest_file[BIOS_LINKER_LOADER_FILESZ]; >>>>> char src_file[BIOS_LINKER_LOADER_FILESZ]; >>>>> uint32_t src_offset, dest_offset; >>>>> uint8_t size; >>>>> } pointer; >>>>> >>>>> I doubt the offsets or size is really all that important though. >>>> >>>> The offset into the fw_cfg file that receives the allocation address is >>>> important, that allows the same file to receive several different >>>> addresses (for different downloaded blobs), at different offsets. >>>> >>>> OTOH, asking the firmware to add a constant to the address value before >>>> writing it to the fw_cfg file is not necessary, in my opinion. The blob >>>> that the firmware allocated and downloaded originates from QEMU to begin >>>> with, so QEMU knows its internal structure. >>> >>> I guess I'm missing why QEMU would want to use the same writable file >>> for multiple pointers >> >> I know no specific reason; I just thought this possible generalization >> was one of the advantages in Michael's suggestion. >> >>> as well as why it would want support for >>> pointers smaller than 8 bytes in size. >> >> Hm, right, good point. >> >>> If it's because it may be >>> easier to support an internal QEMU blob of a particular format, then >>> adding a src_offset would facilitate that. >>> >>> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then >>> that's fine too. >> >> That might be the main reason I guess; reading back a bit, Michael wrote >> "... a variant of ADD_POINTER". >> >>> I'm okay with either format. >> >> I'd say let's go ahead with Michael's proposal then. Ben, are you okay >> with that? >> > Yes, this seems reasonable. If I’m interpreting this properly, it’s > like ADD_POINTER, except that we extend the write path back to QEMU over > DMA. Is that about right?
Yes. Basically the command differs from ADD_POINTER in that (a) the pointer field to patch lives in "fw_cfg space", not in guest RAM, so rather than updating guest RAM, the firmware has to select a writeable fw_cfg file, seek ("skip") to the specified offset, and rewrite the field, (b) the nature of the patching is not "increment", just a plain "overwrite". Thanks! Laszlo