Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Kevin O'Connor]

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 as well as why it would want support for
pointers smaller than 8 bytes in size.  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.  I'm okay with either format.

-Kevin