Re: [SeaBIOS] [PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Kevin O'Connor]

On Thu, Jan 19, 2017 at 10:20:50PM -0800, [e-mail] wrote:
> From: Ben Warren
>
> This adds a new command to the DMA protocol.  The command allows the
> memory allocation of a fw_cfg file by BIOS and subsequent return of
> the allocated address to QEMU.  The initial use case is for Windows VM
> Generation ID, where QEMU needs to change the contents of fw_cfg
> data at runtime, while still having BIOS allocate and manage the memory.

Thanks - see my comments below.

[...]
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
>  }
>
> static void
> +qemu_cfg_write(void *buf, int len)
> +{
> if (len == 0) {
>    return;
> }
> if (qemu_cfg_dma_enabled()) {
>    qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
> } else {
>    outsb(PORT_QEMU_CFG_DATA, buf, len);
> }
>
> +}
>
> +static void
>  qemu_cfg_skip(int len)
>  {
>      if (len == 0) {
> @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
>      }
>  }
>
> +static void
> +qemu_cfg_write_entry(void *buf, int e, int len)
> +{
> if (qemu_cfg_dma_enabled()) {
>    u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
>                    | QEMU_CFG_DMA_CTL_WRITE;
>    qemu_cfg_dma_transfer(buf, len, control);
> } else {
>    qemu_cfg_select(e);
>    qemu_cfg_write(buf, len);
> }
>
> +}
>
> struct qemu_romfile_s {
>      struct romfile_s file;
>      int select, skip;
> @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
>      return file->size;
>  }
>
> +static int
> +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
> +{
> if (file->size < maxlen)
>    return -1;
> struct qemu_romfile_s *qfile;
> qfile = container_of(file, struct qemu_romfile_s, file);
> if (qfile->skip == 0) {
>    /* Do it in one transfer */
>    qemu_cfg_write_entry(src, qfile->select, file->size);
> } else {
>    qemu_cfg_select(qfile->select);
>    qemu_cfg_skip(qfile->skip);
>    qemu_cfg_write(src, file->size);
> }
> return file->size;
>
> +}

I'd prefer if we could break this patch into two parts - one part to
add the write interface to fw_cfg and one part to add
ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
still need to be committed prior to committing to SeaBIOS.  Having the
patch in separate parts will make it both easier to review and commit
at their appropriate times.
> static void
>  qemu_romfile_add(char *name, int select, int skip, int size)
>  {
> @@ -317,6 +362,7 @@ qemu_romfile_add(char *name, int select, int skip, int size)
>      qfile->select = select;
>      qfile->skip = skip;
>      qfile->file.copy = qemu_cfg_read_file;
> qfile->file.write_back = qemu_cfg_write_file;
> romfile_add(&qfile->file);
>
> }

I don't think it is worthwhile to extend the romfile mechanism with a
write_back() method.  Instead, I'd export qemu_cfg_write_file(),
extend it to verify the passed in romfile is a fw_cfg file (file->copy
== qemu_cfg_read_file), and change the callers to call it directly.
> diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> index d8eb7c4..479eaf9 100644
> --- a/src/fw/paravirt.h
> +++ b/src/fw/paravirt.h
> @@ -43,6 +43,7 @@ static inline int runningOnKVM(void) {
>  #define QEMU_CFG_DMA_CTL_READ    0x02
>  #define QEMU_CFG_DMA_CTL_SKIP    0x04
>  #define QEMU_CFG_DMA_CTL_SELECT  0x08
> +#define QEMU_CFG_DMA_CTL_WRITE   0x10
>
> // QEMU_CFG_DMA ID bit
>  #define QEMU_CFG_VERSION_DMA    2
> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> index f4b17ff..945572f 100644
> --- a/src/fw/romfile_loader.c
> +++ b/src/fw/romfile_loader.c
> @@ -127,6 +127,38 @@ err:
>      warn_internalerror();
>  }
>
> +static void romfile_loader_return_addr(struct romfile_loader_entry_s *entry,
>                                    struct romfile_loader_files *files)
>
> +{
> struct romfile_loader_file *alloc_file;
> struct romfile_loader_file *addr_file;
> u64 addr;
> int ret;
> alloc_file = romfile_loader_find(entry->alloc_ret_file, files);
> addr_file = romfile_loader_find(entry->alloc_ret_addr_file, files);
> if (!alloc_file || !addr_file || !alloc_file->data || !addr_file->data ||
>    addr_file->file->size < sizeof(addr))
>    goto err;
> /* Get the address of the just-allocated file
> * and stuff it in the address file */
> memcpy(&addr, &alloc_file->data, sizeof(addr));
> addr = cpu_to_le64(addr);
> memcpy(addr_file->data, &addr, sizeof(addr));
> if (!addr_file->file->write_back)
>    goto err;
> ret = addr_file->file->write_back(&addr, addr_file->file, sizeof(addr));
> if (ret != sizeof(addr))
>    goto err;
> return;
>
> +err:
> warn_internalerror();
>
> +}
>
> int romfile_loader_execute(const char *name)
>  {
>      struct romfile_loader_entry_s *entry;
> @@ -161,6 +193,11 @@ int romfile_loader_execute(const char *name)
>                          break;
>                  case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
>                          romfile_loader_add_checksum(entry, files);
>                    break;
>            case ROMFILE_LOADER_COMMAND_ALLOCATE_RET_ADDR:
>                    romfile_loader_allocate(entry, files);
>                    romfile_loader_return_addr(entry, files);
>                    break;
>            default:
>                    /* Skip commands that we don't recognize. */
>                    break;
>
> diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
> index 15eab2a..ea161e8 100644
> --- a/src/fw/romfile_loader.h
> +++ b/src/fw/romfile_loader.h
> @@ -51,15 +51,32 @@ struct romfile_loader_entry_s {
>              u32 cksum_length;
>          };
>    /*
>     * COMMAND_ALLOCATE_RETURN_ADDR - allocate a table from @alloc_file
>     * subject to @alloc_ret_align alignment (must be power of 2)
>     * and @alloc_ret_zone (can be HIGH or FSEG) requirements.
>     * Additionally, return the address of the allocation in
>     * @addr_file.
>     *
> * This may be used instead of COMMAND_ALLOCATE

Minor nit - a tab snuck in here.
>     */
>    struct {
>        char alloc_ret_file[ROMFILE_LOADER_FILESZ];
>        u32 alloc_ret_align;
>        u8 alloc_ret_zone;
>        char alloc_ret_addr_file[ROMFILE_LOADER_FILESZ];
>    };
>    /* padding */
>    char pad[124];
> };
>
> };
>
> enum {
> ROMFILE_LOADER_COMMAND_ALLOCATE     = 0x1,
> ROMFILE_LOADER_COMMAND_ADD_POINTER  = 0x2,
> ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> ROMFILE_LOADER_COMMAND_ALLOCATE          = 0x1,
> ROMFILE_LOADER_COMMAND_ADD_POINTER       = 0x2,
> ROMFILE_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
> ROMFILE_LOADER_COMMAND_ALLOCATE_RET_ADDR = 0x4,
>
> };
>
> enum {
> diff --git a/src/romfile.h b/src/romfile.h
> index c6d62a1..8694cc1 100644
> --- a/src/romfile.h
> +++ b/src/romfile.h
> @@ -9,6 +9,7 @@ struct romfile_s {
>      char name[128];
>      u32 size;
>      int (*copy)(struct romfile_s *file, void *dest, u32 maxlen);
> int (*write_back)(void *src, struct romfile_s *file, u32 maxlen);
>
> };
>  void romfile_add(struct romfile_s *file);
>  struct romfile_s *romfile_findprefix(const char *prefix, struct romfile_s *prev);

-Kevin

[PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Laszlo Ersek]

On 01/20/17 17:08, Kevin O'Connor wrote:
> On Thu, Jan 19, 2017 at 10:20:50PM -0800, [e-mail] wrote:
> > From: Ben Warren
> >
> > This adds a new command to the DMA protocol.  The command allows the
> > memory allocation of a fw_cfg file by BIOS and subsequent return of
> > the allocated address to QEMU.  The initial use case is for Windows VM
> > Generation ID, where QEMU needs to change the contents of fw_cfg
> > data at runtime, while still having BIOS allocate and manage the memory.
>
> Thanks - see my comments below.
>
> [...]
> > --- a/src/fw/paravirt.c
> > +++ b/src/fw/paravirt.c
> > @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
> >  }
> >
> > static void
> > +qemu_cfg_write(void *buf, int len)
> > +{
> > if (len == 0) {
> >    return;
> > }
> > if (qemu_cfg_dma_enabled()) {
> >    qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
> > } else {
> >    outsb(PORT_QEMU_CFG_DATA, buf, len);
> > }
> >
> > +}
> >
> > +static void
> >  qemu_cfg_skip(int len)
> >  {
> >      if (len == 0) {
> > @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
> >      }
> >  }
> >
> > +static void
> > +qemu_cfg_write_entry(void *buf, int e, int len)
> > +{
> > if (qemu_cfg_dma_enabled()) {
> >    u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
> >                    | QEMU_CFG_DMA_CTL_WRITE;
> >    qemu_cfg_dma_transfer(buf, len, control);
> > } else {
> >    qemu_cfg_select(e);
> >    qemu_cfg_write(buf, len);
> > }
> >
> > +}
> >
> > struct qemu_romfile_s {
> >      struct romfile_s file;
> >      int select, skip;
> > @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
> >      return file->size;
> >  }
> >
> > +static int
> > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
> > +{
> > if (file->size < maxlen)
> >    return -1;
> > struct qemu_romfile_s *qfile;
> > qfile = container_of(file, struct qemu_romfile_s, file);
> > if (qfile->skip == 0) {
> >    /* Do it in one transfer */
> >    qemu_cfg_write_entry(src, qfile->select, file->size);
> > } else {
> >    qemu_cfg_select(qfile->select);
> >    qemu_cfg_skip(qfile->skip);
> >    qemu_cfg_write(src, file->size);
> > }
> > return file->size;
> >
> > +}
>
> I'd prefer if we could break this patch into two parts - one part to
> add the write interface to fw_cfg and one part to add
> ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
> still need to be committed prior to committing to SeaBIOS.  Having the
> patch in separate parts will make it both easier to review and commit
> at their appropriate times.

The fw_cfg write support is now upstream (QEMU commit
baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
blobs").

Thanks
Laszlo
> > static void
> >  qemu_romfile_add(char *name, int select, int skip, int size)
> >  {
> > @@ -317,6 +362,7 @@ qemu_romfile_add(char *name, int select, int skip, int size)
> >      qfile->select = select;
> >      qfile->skip = skip;
> >      qfile->file.copy = qemu_cfg_read_file;
> > qfile->file.write_back = qemu_cfg_write_file;
> > romfile_add(&qfile->file);
> >
> > }
>
> I don't think it is worthwhile to extend the romfile mechanism with a
> write_back() method.  Instead, I'd export qemu_cfg_write_file(),
> extend it to verify the passed in romfile is a fw_cfg file (file->copy
> == qemu_cfg_read_file), and change the callers to call it directly.
> > diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> > index d8eb7c4..479eaf9 100644
> > --- a/src/fw/paravirt.h
> > +++ b/src/fw/paravirt.h
> > @@ -43,6 +43,7 @@ static inline int runningOnKVM(void) {
> >  #define QEMU_CFG_DMA_CTL_READ    0x02
> >  #define QEMU_CFG_DMA_CTL_SKIP    0x04
> >  #define QEMU_CFG_DMA_CTL_SELECT  0x08
> > +#define QEMU_CFG_DMA_CTL_WRITE   0x10
> >
> > // QEMU_CFG_DMA ID bit
> >  #define QEMU_CFG_VERSION_DMA    2
> > diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
> > index f4b17ff..945572f 100644
> > --- a/src/fw/romfile_loader.c
> > +++ b/src/fw/romfile_loader.c
> > @@ -127,6 +127,38 @@ err:
> >      warn_internalerror();
> >  }
> >
> > +static void romfile_loader_return_addr(struct romfile_loader_entry_s *entry,
> >                                    struct romfile_loader_files *files)
> >
> > +{
> > struct romfile_loader_file *alloc_file;
> > struct romfile_loader_file *addr_file;
> > u64 addr;
> > int ret;
> > alloc_file = romfile_loader_find(entry->alloc_ret_file, files);
> > addr_file = romfile_loader_find(entry->alloc_ret_addr_file, files);
> > if (!alloc_file || !addr_file || !alloc_file->data || !addr_file->data ||
> >    addr_file->file->size < sizeof(addr))
> >    goto err;
> > /* Get the address of the just-allocated file
> > * and stuff it in the address file */
> > memcpy(&addr, &alloc_file->data, sizeof(addr));
> > addr = cpu_to_le64(addr);
> > memcpy(addr_file->data, &addr, sizeof(addr));
> > if (!addr_file->file->write_back)
> >    goto err;
> > ret = addr_file->file->write_back(&addr, addr_file->file, sizeof(addr));
> > if (ret != sizeof(addr))
> >    goto err;
> > return;
> >
> > +err:
> > warn_internalerror();
> >
> > +}
> >
> > int romfile_loader_execute(const char *name)
> >  {
> >      struct romfile_loader_entry_s *entry;
> > @@ -161,6 +193,11 @@ int romfile_loader_execute(const char *name)
> >                          break;
> >                  case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
> >                          romfile_loader_add_checksum(entry, files);
> >                    break;
> >            case ROMFILE_LOADER_COMMAND_ALLOCATE_RET_ADDR:
> >                    romfile_loader_allocate(entry, files);
> >                    romfile_loader_return_addr(entry, files);
> >                    break;
> >            default:
> >                    /* Skip commands that we don't recognize. */
> >                    break;
> >
> > diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
> > index 15eab2a..ea161e8 100644
> > --- a/src/fw/romfile_loader.h
> > +++ b/src/fw/romfile_loader.h
> > @@ -51,15 +51,32 @@ struct romfile_loader_entry_s {
> >              u32 cksum_length;
> >          };
> >    /*
> >     * COMMAND_ALLOCATE_RETURN_ADDR - allocate a table from @alloc_file
> >     * subject to @alloc_ret_align alignment (must be power of 2)
> >     * and @alloc_ret_zone (can be HIGH or FSEG) requirements.
> >     * Additionally, return the address of the allocation in
> >     * @addr_file.
> >     *
> > * This may be used instead of COMMAND_ALLOCATE
>
> Minor nit - a tab snuck in here.
> >     */
> >    struct {
> >        char alloc_ret_file[ROMFILE_LOADER_FILESZ];
> >        u32 alloc_ret_align;
> >        u8 alloc_ret_zone;
> >        char alloc_ret_addr_file[ROMFILE_LOADER_FILESZ];
> >    };
> >    /* padding */
> >    char pad[124];
> > };
> >
> > };
> >
> > enum {
> > ROMFILE_LOADER_COMMAND_ALLOCATE     = 0x1,
> > ROMFILE_LOADER_COMMAND_ADD_POINTER  = 0x2,
> > ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> > ROMFILE_LOADER_COMMAND_ALLOCATE          = 0x1,
> > ROMFILE_LOADER_COMMAND_ADD_POINTER       = 0x2,
> > ROMFILE_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
> > ROMFILE_LOADER_COMMAND_ALLOCATE_RET_ADDR = 0x4,
> >
> > };
> >
> > enum {
> > diff --git a/src/romfile.h b/src/romfile.h
> > index c6d62a1..8694cc1 100644
> > --- a/src/romfile.h
> > +++ b/src/romfile.h
> > @@ -9,6 +9,7 @@ struct romfile_s {
> >      char name[128];
> >      u32 size;
> >      int (*copy)(struct romfile_s *file, void *dest, u32 maxlen);
> > int (*write_back)(void *src, struct romfile_s *file, u32 maxlen);
> >
> > };
> >  void romfile_add(struct romfile_s *file);
> >  struct romfile_s *romfile_findprefix(const char *prefix, struct romfile_s *prev);
>
> -Kevin

[PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Kevin O'Connor]

On Fri, Jan 20, 2017 at 05:39:58PM +0100, Laszlo Ersek wrote:
> On 01/20/17 17:08, Kevin O'Connor wrote:
> > On Thu, Jan 19, 2017 at 10:20:50PM -0800, [e-mail] wrote:
> > > From: Ben Warren
> > >
> > > This adds a new command to the DMA protocol.  The command allows the
> > > memory allocation of a fw_cfg file by BIOS and subsequent return of
> > > the allocated address to QEMU.  The initial use case is for Windows VM
> > > Generation ID, where QEMU needs to change the contents of fw_cfg
> > > data at runtime, while still having BIOS allocate and manage the memory.
> >
> > Thanks - see my comments below.
> >
> > [...]
> > > --- a/src/fw/paravirt.c
> > > +++ b/src/fw/paravirt.c
> > > @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
> > >  }
> > >
> > > static void
> > > +qemu_cfg_write(void *buf, int len)
> > > +{
> > > if (len == 0) {
> > >    return;
> > > }
> > > if (qemu_cfg_dma_enabled()) {
> > >    qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
> > > } else {
> > >    outsb(PORT_QEMU_CFG_DATA, buf, len);
> > > }
> > >
> > > +}
> > >
> > > +static void
> > >  qemu_cfg_skip(int len)
> > >  {
> > >      if (len == 0) {
> > > @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
> > >      }
> > >  }
> > >
> > > +static void
> > > +qemu_cfg_write_entry(void *buf, int e, int len)
> > > +{
> > > if (qemu_cfg_dma_enabled()) {
> > >    u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
> > >                    | QEMU_CFG_DMA_CTL_WRITE;
> > >    qemu_cfg_dma_transfer(buf, len, control);
> > > } else {
> > >    qemu_cfg_select(e);
> > >    qemu_cfg_write(buf, len);
> > > }
> > >
> > > +}
> > >
> > > struct qemu_romfile_s {
> > >      struct romfile_s file;
> > >      int select, skip;
> > > @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
> > >      return file->size;
> > >  }
> > >
> > > +static int
> > > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
> > > +{
> > > if (file->size < maxlen)
> > >    return -1;
> > > struct qemu_romfile_s *qfile;
> > > qfile = container_of(file, struct qemu_romfile_s, file);
> > > if (qfile->skip == 0) {
> > >    /* Do it in one transfer */
> > >    qemu_cfg_write_entry(src, qfile->select, file->size);
> > > } else {
> > >    qemu_cfg_select(qfile->select);
> > >    qemu_cfg_skip(qfile->skip);
> > >    qemu_cfg_write(src, file->size);
> > > }
> > > return file->size;
> > >
> > > +}
> >
> > I'd prefer if we could break this patch into two parts - one part to
> > add the write interface to fw_cfg and one part to add
> > ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
> > still need to be committed prior to committing to SeaBIOS.  Having the
> > patch in separate parts will make it both easier to review and commit
> > at their appropriate times.
>
> The fw_cfg write support is now upstream (QEMU commit
> baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
> blobs").

Thanks.  Is it correct that the QEMU write support is only possible
with the DMA interface?  If so, I'd request that Ben's patch also be
updated to not issue an outsb().  Preferably it should just fail at
the start of qemu_cfg_write_file() if the DMA interface isn't
available.

-Kevin

[PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Laszlo Ersek]

On 01/20/17 17:58, Kevin O'Connor wrote:
> On Fri, Jan 20, 2017 at 05:39:58PM +0100, Laszlo Ersek wrote:
> > On 01/20/17 17:08, Kevin O'Connor wrote:
> > > On Thu, Jan 19, 2017 at 10:20:50PM -0800, [e-mail] wrote:
> > > > From: Ben Warren
> > > >
> > > > This adds a new command to the DMA protocol.  The command allows the
> > > > memory allocation of a fw_cfg file by BIOS and subsequent return of
> > > > the allocated address to QEMU.  The initial use case is for Windows VM
> > > > Generation ID, where QEMU needs to change the contents of fw_cfg
> > > > data at runtime, while still having BIOS allocate and manage the memory.
> > >
> > > Thanks - see my comments below.
> > >
> > > [...]
> > > > --- a/src/fw/paravirt.c
> > > > +++ b/src/fw/paravirt.c
> > > > @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
> > > >  }
> > > >
> > > > static void
> > > > +qemu_cfg_write(void *buf, int len)
> > > > +{
> > > > if (len == 0) {
> > > >    return;
> > > > }
> > > > if (qemu_cfg_dma_enabled()) {
> > > >    qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
> > > > } else {
> > > >    outsb(PORT_QEMU_CFG_DATA, buf, len);
> > > > }
> > > >
> > > > +}
> > > >
> > > > +static void
> > > >  qemu_cfg_skip(int len)
> > > >  {
> > > >      if (len == 0) {
> > > > @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
> > > >      }
> > > >  }
> > > >
> > > > +static void
> > > > +qemu_cfg_write_entry(void *buf, int e, int len)
> > > > +{
> > > > if (qemu_cfg_dma_enabled()) {
> > > >    u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
> > > >                    | QEMU_CFG_DMA_CTL_WRITE;
> > > >    qemu_cfg_dma_transfer(buf, len, control);
> > > > } else {
> > > >    qemu_cfg_select(e);
> > > >    qemu_cfg_write(buf, len);
> > > > }
> > > >
> > > > +}
> > > >
> > > > struct qemu_romfile_s {
> > > >      struct romfile_s file;
> > > >      int select, skip;
> > > > @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
> > > >      return file->size;
> > > >  }
> > > >
> > > > +static int
> > > > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
> > > > +{
> > > > if (file->size < maxlen)
> > > >    return -1;
> > > > struct qemu_romfile_s *qfile;
> > > > qfile = container_of(file, struct qemu_romfile_s, file);
> > > > if (qfile->skip == 0) {
> > > >    /* Do it in one transfer */
> > > >    qemu_cfg_write_entry(src, qfile->select, file->size);
> > > > } else {
> > > >    qemu_cfg_select(qfile->select);
> > > >    qemu_cfg_skip(qfile->skip);
> > > >    qemu_cfg_write(src, file->size);
> > > > }
> > > > return file->size;
> > > >
> > > > +}
> > >
> > > I'd prefer if we could break this patch into two parts - one part to
> > > add the write interface to fw_cfg and one part to add
> > > ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
> > > still need to be committed prior to committing to SeaBIOS.  Having the
> > > patch in separate parts will make it both easier to review and commit
> > > at their appropriate times.
> >
> > The fw_cfg write support is now upstream (QEMU commit
> > baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
> > blobs").
>
> Thanks.  Is it correct that the QEMU write support is only possible
> with the DMA interface?  If so, I'd request that Ben's patch also be
> updated to not issue an outsb().  Preferably it should just fail at
> the start of qemu_cfg_write_file() if the DMA interface isn't
> available.

It varies across QEMU versions. Up to and including QEMU 2.3, IO port
writes were supported. In the [2.4, 2.8] range, inclusive, there was no
write support. Starting with 2.9, only the DMA interface supports writes
(and writeability is tracked independently of the numerical value of the
given fw_cfg key).

Today the idea is basically, if you have found an fw_cfg file by name
that you know is writeable, as opposed to a predefined numerical
selector value, then you can write to it with the DMA interface. The
case where the writeable, named file in question is found, but the DMA
interface is missing (so you'd have to revert to IO port write), is
nonexistent.

So I agree that the IO port-based write is practically dead code here.

Thanks
Laszlo

[PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Ben Warren]

> On Jan 20, 2017, at 8:58 AM, Kevin O'Connor  wrote:
>
> On Fri, Jan 20, 2017 at 05:39:58PM +0100, Laszlo Ersek wrote:
> > On 01/20/17 17:08, Kevin O'Connor wrote:
> > > On Thu, Jan 19, 2017 at 10:20:50PM -0800, [e-mail] wrote:
> > > > From: Ben Warren
> > > >
> > > > This adds a new command to the DMA protocol.  The command allows the
> > > > memory allocation of a fw_cfg file by BIOS and subsequent return of
> > > > the allocated address to QEMU.  The initial use case is for Windows VM
> > > > Generation ID, where QEMU needs to change the contents of fw_cfg
> > > > data at runtime, while still having BIOS allocate and manage the memory.
> > >
> > > Thanks - see my comments below.
> > >
> > > [...]
> > > > --- a/src/fw/paravirt.c
> > > > +++ b/src/fw/paravirt.c
> > > > @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
> > > > }
> > > >
> > > > static void
> > > > +qemu_cfg_write(void *buf, int len)
> > > > +{
> > > > if (len == 0) {
> > > >    return;
> > > > }
> > > > if (qemu_cfg_dma_enabled()) {
> > > >    qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
> > > > } else {
> > > >    outsb(PORT_QEMU_CFG_DATA, buf, len);
> > > > }
> > > >
> > > > +}
> > > >
> > > > +static void
> > > > qemu_cfg_skip(int len)
> > > > {
> > > >     if (len == 0) {
> > > > @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
> > > >     }
> > > > }
> > > >
> > > > +static void
> > > > +qemu_cfg_write_entry(void *buf, int e, int len)
> > > > +{
> > > > if (qemu_cfg_dma_enabled()) {
> > > >    u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
> > > >                    | QEMU_CFG_DMA_CTL_WRITE;
> > > >    qemu_cfg_dma_transfer(buf, len, control);
> > > > } else {
> > > >    qemu_cfg_select(e);
> > > >    qemu_cfg_write(buf, len);
> > > > }
> > > >
> > > > +}
> > > >
> > > > struct qemu_romfile_s {
> > > >     struct romfile_s file;
> > > >     int select, skip;
> > > > @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
> > > >     return file->size;
> > > > }
> > > >
> > > > +static int
> > > > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
> > > > +{
> > > > if (file->size < maxlen)
> > > >    return -1;
> > > > struct qemu_romfile_s *qfile;
> > > > qfile = container_of(file, struct qemu_romfile_s, file);
> > > > if (qfile->skip == 0) {
> > > >    /* Do it in one transfer */
> > > >    qemu_cfg_write_entry(src, qfile->select, file->size);
> > > > } else {
> > > >    qemu_cfg_select(qfile->select);
> > > >    qemu_cfg_skip(qfile->skip);
> > > >    qemu_cfg_write(src, file->size);
> > > > }
> > > > return file->size;
> > > >
> > > > +}
> > >
> > > I'd prefer if we could break this patch into two parts - one part to
> > > add the write interface to fw_cfg and one part to add
> > > ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
> > > still need to be committed prior to committing to SeaBIOS.  Having the
> > > patch in separate parts will make it both easier to review and commit
> > > at their appropriate times.
> >
> > The fw_cfg write support is now upstream (QEMU commit
> > baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
> > blobs").
>
> Thanks.  Is it correct that the QEMU write support is only possible
> with the DMA interface?  If so, I'd request that Ben's patch also be
> updated to not issue an outsb().  Preferably it should just fail at
> the start of qemu_cfg_write_file() if the DMA interface isn't
> available.

Hopefully I’ve addressed all of your concerns.  I’m curious now, how is this kind of thing typically phased with QEMU changes?  I’m working on getting the QEMU side of things submitted, but for it to work standalone, the embedded binary SeaBIOS image needs to be updated.  Who typically builds and inserts the new binary in to the QEMU code tree?
> -Kevin

[PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Laszlo Ersek]

On 01/20/17 23:41, Ben Warren wrote:
> > On Jan 20, 2017, at 8:58 AM, Kevin O'Connor <[e-mail]
> > > wrote:
> >
> > On Fri, Jan 20, 2017 at 05:39:58PM +0100, Laszlo Ersek wrote:
> > > On 01/20/17 17:08, Kevin O'Connor wrote:
> > > > On Thu, Jan 19, 2017 at 10:20:50PM -0800, [e-mail]
> > > >  wrote:
> > > > > From: Ben Warren <[e-mail]
> > > > > >
> > > > >
> > > > > This adds a new command to the DMA protocol.  The command allows the
> > > > > memory allocation of a fw_cfg file by BIOS and subsequent return of
> > > > > the allocated address to QEMU.  The initial use case is for Windows VM
> > > > > Generation ID, where QEMU needs to change the contents of fw_cfg
> > > > > data at runtime, while still having BIOS allocate and manage the
> > > > > memory.
> > > >
> > > > Thanks - see my comments below.
> > > >
> > > > [...]
> > > > > --- a/src/fw/paravirt.c
> > > > > +++ b/src/fw/paravirt.c
> > > > > @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
> > > > > }
> > > > >
> > > > > static void
> > > > > +qemu_cfg_write(void *buf, int len)
> > > > > +{
> > > > > if (len == 0) {
> > > > >    return;
> > > > > }
> > > > > if (qemu_cfg_dma_enabled()) {
> > > > >    qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
> > > > > } else {
> > > > >    outsb(PORT_QEMU_CFG_DATA, buf, len);
> > > > > }
> > > > >
> > > > > +}
> > > > >
> > > > > +static void
> > > > > qemu_cfg_skip(int len)
> > > > > {
> > > > >     if (len == 0) {
> > > > > @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
> > > > >     }
> > > > > }
> > > > >
> > > > > +static void
> > > > > +qemu_cfg_write_entry(void *buf, int e, int len)
> > > > > +{
> > > > > if (qemu_cfg_dma_enabled()) {
> > > > >    u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
> > > > >                    | QEMU_CFG_DMA_CTL_WRITE;
> > > > >    qemu_cfg_dma_transfer(buf, len, control);
> > > > > } else {
> > > > >    qemu_cfg_select(e);
> > > > >    qemu_cfg_write(buf, len);
> > > > > }
> > > > >
> > > > > +}
> > > > >
> > > > > struct qemu_romfile_s {
> > > > >     struct romfile_s file;
> > > > >     int select, skip;
> > > > > @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file,
> > > > > void *dst, u32 maxlen)
> > > > >     return file->size;
> > > > > }
> > > > >
> > > > > +static int
> > > > > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
> > > > > +{
> > > > > if (file->size < maxlen)
> > > > >    return -1;
> > > > > struct qemu_romfile_s *qfile;
> > > > > qfile = container_of(file, struct qemu_romfile_s, file);
> > > > > if (qfile->skip == 0) {
> > > > >    /* Do it in one transfer */
> > > > >    qemu_cfg_write_entry(src, qfile->select, file->size);
> > > > > } else {
> > > > >    qemu_cfg_select(qfile->select);
> > > > >    qemu_cfg_skip(qfile->skip);
> > > > >    qemu_cfg_write(src, file->size);
> > > > > }
> > > > > return file->size;
> > > > >
> > > > > +}
> > > >
> > > > I'd prefer if we could break this patch into two parts - one part to
> > > > add the write interface to fw_cfg and one part to add
> > > > ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
> > > > still need to be committed prior to committing to SeaBIOS.  Having the
> > > > patch in separate parts will make it both easier to review and commit
> > > > at their appropriate times.
> > >
> > > The fw_cfg write support is now upstream (QEMU commit
> > > baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
> > > blobs").
> >
> > Thanks.  Is it correct that the QEMU write support is only possible
> > with the DMA interface?  If so, I'd request that Ben's patch also be
> > updated to not issue an outsb().  Preferably it should just fail at
> > the start of qemu_cfg_write_file() if the DMA interface isn't
> > available.
>
> Hopefully I’ve addressed all of your concerns.  I’m curious now, how is
> this kind of thing typically phased with QEMU changes?  I’m working on
> getting the QEMU side of things submitted, but for it to work
> standalone, the embedded binary SeaBIOS image needs to be updated.  Who
> typically builds and inserts the new binary in to the QEMU code tree?

Usually the QEMU side of the changes is committed first. The matching
SeaBIOS code is committed second, with the SeaBIOS commit message
referencing the QEMU commit. The next SeaBIOS release is usually planned
so it can make the next QEMU soft freeze. Gerd rebuilds the bundled
SeaBIOS binaries for QEMU, and they go into the QEMU release like any
other commit.

Run

git log -- pc-bios/bios-256k.bin

in the QEMU tree, for example.

You can also search the SeaBIOS mailing list archive for subjects that
contain the word "release"; past releases were coordinated in those threads.

(CC'ing Gerd so he can correct me if necessary).

Thanks
Laszlo
> > -Kevin

[PATCH] QEMU DMA: Add ability to write back to fw_cfg file

[Gerd Hoffmann]

Hi,
> > Hopefully I’ve addressed all of your concerns.  I’m curious now, how is
> > this kind of thing typically phased with QEMU changes?  I’m working on
> > getting the QEMU side of things submitted, but for it to work
> > standalone, the embedded binary SeaBIOS image needs to be updated.  Who
> > typically builds and inserts the new binary in to the QEMU code tree?
>
> Usually the QEMU side of the changes is committed first. The matching
> SeaBIOS code is committed second, with the SeaBIOS commit message
> referencing the QEMU commit. The next SeaBIOS release is usually planned
> so it can make the next QEMU soft freeze. Gerd rebuilds the bundled
> SeaBIOS binaries for QEMU, and they go into the QEMU release like any
> other commit.
> (CC'ing Gerd so he can correct me if necessary).

Fully correct.  I guess in this specific case we'll go cherry-pick this
change from master into 1.10-stable and cut a 1.10.2 release from it for
qemu 2.9.

FYI: you can update the roms/seabios submodule, then run "make -C roms
bios" to rebuild seabios locally for testing purposes.

cheers,
  Gerd