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