Re: [SeaBIOS] [PATCH v3 1/2] QEMU DMA: Add DMA write capability

[Laszlo Ersek]

On 02/05/17 18:09, [e-mail] wrote:
> From: Ben Warren
>
> This allows BIOS to write data back to QEMU using the DMA interface and
> provides a higher-level abstraction to write to a fw_cfg file
> Signed-off-by: Ben Warren
>
> src/fw/paravirt.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/fw/paravirt.h |  3 +++
>  2 files changed, 52 insertions(+)
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 6de70f6..75cb992 100644
> --- 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 {
>    warn_internalerror();
> }
>
> +}
>
> +static void
>  qemu_cfg_skip(int len)
>  {
>      if (len == 0) {
> @@ -280,6 +294,18 @@ 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 {
>    warn_internalerror();
> }
>
> +}
>
> struct qemu_romfile_s {
>      struct romfile_s file;
>      int select, skip;
> @@ -303,6 +329,29 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
>      return file->size;
>  }
>
> +int
> +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
> +{
> if ((file->size + offset) < len)
>    return -1;

I'm confused about this; mathematically, we want to require

offset + len <= size

If we want to avoid any possibility for overflow in the addition, we can
subtract "len":

offset <= size - len

However, in this case the subtraction needs to be protected, and we end
up with the following condition to proceed:

size >= len && offset <= size - len

If we want a condition for bailing out early, we got to negate the above:

size < len || offset > size - len

The rest looks good to me.

Thanks
Laszlo
> if (!qemu_cfg_dma_enabled() || (file->copy != qemu_cfg_read_file)) {
>    warn_internalerror();
>    return -1;
> }
> struct qemu_romfile_s *qfile;
> qfile = container_of(file, struct qemu_romfile_s, file);
> if (offset == 0) {
>    /* Do it in one transfer */
>    qemu_cfg_write_entry(src, qfile->select, len);
> } else {
>    qemu_cfg_select(qfile->select);
>    qemu_cfg_skip(offset);
>    qemu_cfg_write(src, len);
> }
> return len;
>
> +}
>
> static void
>  qemu_romfile_add(char *name, int select, int skip, int size)
>  {
> diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> index d8eb7c4..fb220d8 100644
> --- a/src/fw/paravirt.h
> +++ b/src/fw/paravirt.h
> @@ -3,6 +3,7 @@
>
> #include "config.h" // CONFIG_*
>  #include "biosvar.h" // GET_GLOBAL
> +#include "romfile.h" // struct romfile_s
>
> // Types of paravirtualized platforms.
>  #define PF_QEMU     (1<<0)
> @@ -43,6 +44,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
> @@ -53,5 +55,6 @@ void qemu_platform_setup(void);
>  void qemu_cfg_init(void);
>
> u16 qemu_get_present_cpus_count(void);
> +int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len);
>
> #endif

[PATCH v3 1/2] QEMU DMA: Add DMA write capability

[Ben Warren]

> On Feb 8, 2017, at 4:13 AM, Laszlo Ersek  wrote:
>
> On 02/05/17 18:09, [e-mail]  wrote:
> > From: Ben Warren
> >
> > This allows BIOS to write data back to QEMU using the DMA interface and
> > provides a higher-level abstraction to write to a fw_cfg file
> > Signed-off-by: Ben Warren
> >
> > src/fw/paravirt.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> > src/fw/paravirt.h |  3 +++
> > 2 files changed, 52 insertions(+)
> >
> > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> > index 6de70f6..75cb992 100644
> > --- 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 {
> >    warn_internalerror();
> > }
> >
> > +}
> >
> > +static void
> > qemu_cfg_skip(int len)
> > {
> >     if (len == 0) {
> > @@ -280,6 +294,18 @@ 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 {
> >    warn_internalerror();
> > }
> >
> > +}
> >
> > struct qemu_romfile_s {
> >     struct romfile_s file;
> >     int select, skip;
> > @@ -303,6 +329,29 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
> >     return file->size;
> > }
> >
> > +int
> > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len)
> > +{
> > if ((file->size + offset) < len)
> >    return -1;
>
> I'm confused about this; mathematically, we want to require
>
> offset + len <= size
>
> If we want to avoid any possibility for overflow in the addition, we can
> subtract "len":
>
> offset <= size - len
>
> However, in this case the subtraction needs to be protected, and we end
> up with the following condition to proceed:
>
> size >= len && offset <= size - len
>
> If we want a condition for bailing out early, we got to negate the above:
>
> size < len || offset > size - len
>
> The rest looks good to me.

This was dumb.  I meant to do:

if ((offset + len) > file->size) {
    return -1;
}

but got mixed up.  Will fix.
> Thanks
> Laszlo
> > if (!qemu_cfg_dma_enabled() || (file->copy != qemu_cfg_read_file)) {
> >    warn_internalerror();
> >    return -1;
> > }
> > struct qemu_romfile_s *qfile;
> > qfile = container_of(file, struct qemu_romfile_s, file);
> > if (offset == 0) {
> >    /* Do it in one transfer */
> >    qemu_cfg_write_entry(src, qfile->select, len);
> > } else {
> >    qemu_cfg_select(qfile->select);
> >    qemu_cfg_skip(offset);
> >    qemu_cfg_write(src, len);
> > }
> > return len;
> >
> > +}
> >
> > static void
> > qemu_romfile_add(char *name, int select, int skip, int size)
> > {
> > diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
> > index d8eb7c4..fb220d8 100644
> > --- a/src/fw/paravirt.h
> > +++ b/src/fw/paravirt.h
> > @@ -3,6 +3,7 @@
> >
> > #include "config.h" // CONFIG_*
> > #include "biosvar.h" // GET_GLOBAL
> > +#include "romfile.h" // struct romfile_s
> >
> > // Types of paravirtualized platforms.
> > #define PF_QEMU     (1<<0)
> > @@ -43,6 +44,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
> > @@ -53,5 +55,6 @@ void qemu_platform_setup(void);
> > void qemu_cfg_init(void);
> >
> > u16 qemu_get_present_cpus_count(void);
> > +int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len);
> >
> > #endif