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

[Kevin O'Connor <kevin@koconnor.net>]

On Thu, Jan 26, 2017 at 08:59:04PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote:
> > On 01/26/17 19:15, Michael S. Tsirkin wrote:
> > > On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
> > >> On 01/26/17 16:20, Michael S. Tsirkin wrote:
> > >>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
> > >>
> > >>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
> > >>>
> > >>>
> > >>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
> > >>> COMMAND_ALLOCATE.
> > >>
> > >> It's a new command being introduced in this series, at my suggestion. It
> > >> does the exact same thing as COMMAND_ALLOCATE, except once the
> > >> allocation / download is carried out by the firmware, the firmware
> > >> writes back the allocation address to the fw_cfg file that is named in
> > >> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
> > >> is how QEMU learns where the blob in GPA space was placed by the
> > >> firmware.) The format for this address-receiving fw_cfg file is supposed
> > >> to be 8-byte, little endian.
> > > 
> > > I see. I really think it's better as a separate command though.
> > > E.g. COMMAND_WRITE_PTR?
> > 
> > Sure, but please provide specifics, otherwise Ben & myself will have a
> > hard time divining & implementing your intent :)
> > 
> > Thanks,
> > Laszlo
> 
> I would say a variant of ADD_POINTER:
> 
>         /*
> 	 * COMMAND_WRITE_POINTER - update a writeable file named
> 	 * @pointer.dest_file at @pointer.offset, by writing pointer to
> 	 * the table 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 offset;
>             uint8_t size;
>         } pointer;
> 

I'm okay with this approach.

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.

-Kevin