[PATCH v3] tests/qtest/libqos: add DMA support for writing and reading fw_cfg files

[PATCH v3] tests/qtest/libqos: add DMA support for writing and reading fw_cfg files

[Ani Sinha]

At present, the libqos/fw_cfg.c library does not support the modern DMA
interface which is required to write to the fw_cfg files. It only uses the IO
interface. Implement read and write methods based on DMA. This will enable
developers to write tests that writes to the fw_cfg file(s). The structure of
the code is taken from edk2 fw_cfg implementation. It has been tested by
writing a qtest that writes to a fw_cfg file. This test will be part of a
future patch series.

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 tests/qtest/libqos/fw_cfg.c | 204 ++++++++++++++++++++++++++++++++----
 tests/qtest/libqos/fw_cfg.h |   5 +
 2 files changed, 186 insertions(+), 23 deletions(-)

changelog:
v3: fix bugs and code reorg. More testing to make sure this actually
works.
v2: refactor common code into a helper.
diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
index 89f053ccac..02d16b098c 100644
--- a/tests/qtest/libqos/fw_cfg.c
+++ b/tests/qtest/libqos/fw_cfg.c
@@ -17,6 +17,8 @@
 #include "../libqtest.h"
 #include "qemu/bswap.h"
 #include "hw/nvram/fw_cfg.h"
+#include "malloc-pc.h"
+#include "libqos-malloc.h"
 
 void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
 {
@@ -60,6 +62,99 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
     qtest_writew(fw_cfg->qts, fw_cfg->base, key);
 }
 
+static void
+qfw_cfg_dma_transfer(QFWCFG *fw_cfg, QOSState *qs, void *address,
+                     uint32_t length, uint32_t control)
+{
+    FWCfgDmaAccess access;
+    uint32_t addr;
+    uint64_t guest_access_addr;
+    uint64_t gaddr;
+
+    /* create a data buffer in guest memory */
+    gaddr = guest_alloc(&qs->alloc, length);
+    g_assert(gaddr);
+
+    if (control & FW_CFG_DMA_CTL_WRITE) {
+        qtest_bufwrite(fw_cfg->qts, gaddr, address, length);
+    }
+    access.address = cpu_to_be64(gaddr);
+    access.length = cpu_to_be32(length);
+    access.control = cpu_to_be32(control);
+
+    /* now create a separate buffer in guest memory for 'access' */
+    guest_access_addr = guest_alloc(&qs->alloc, sizeof(access));
+    g_assert(guest_access_addr);
+    qtest_bufwrite(fw_cfg->qts, guest_access_addr, &access, sizeof(access));
+
+    /* write lower 32 bits of address */
+    addr = cpu_to_be32((uint32_t)(uintptr_t)guest_access_addr);
+    qtest_outl(fw_cfg->qts, fw_cfg->base + 8, addr);
+
+    /* write upper 32 bits of address */
+    addr = cpu_to_be32((uint32_t)(uintptr_t)(guest_access_addr >> 32));
+    qtest_outl(fw_cfg->qts, fw_cfg->base + 4, addr);
+
+    g_assert(!(be32_to_cpu(access.control) & FW_CFG_DMA_CTL_ERROR));
+
+    if (control & FW_CFG_DMA_CTL_READ) {
+        qtest_bufread(fw_cfg->qts, gaddr, address, length);
+    }
+
+    guest_free(&qs->alloc, guest_access_addr);
+    guest_free(&qs->alloc, gaddr);
+}
+
+static void
+qfw_cfg_write_entry(QFWCFG *fw_cfg, QOSState *qs, uint16_t key,
+                    void *buf, uint32_t len)
+{
+    qfw_cfg_select(fw_cfg, key);
+    qfw_cfg_dma_transfer(fw_cfg, qs, buf, len, FW_CFG_DMA_CTL_WRITE);
+}
+
+static void
+qfw_cfg_read_entry(QFWCFG *fw_cfg, QOSState *qs, uint16_t key,
+                   void *buf, uint32_t len)
+{
+    qfw_cfg_select(fw_cfg, key);
+    qfw_cfg_dma_transfer(fw_cfg, qs, buf, len, FW_CFG_DMA_CTL_READ);
+}
+
+static bool
+find_pdir_entry(QFWCFG *fw_cfg, const char *filename,
+                uint16_t *sel, uint32_t *size)
+{
+    unsigned char *filesbuf = NULL;
+    uint32_t count;
+    size_t dsize;
+    FWCfgFile *pdir_entry;
+    uint32_t i;
+    bool found = false;
+
+    *size = 0;
+    *sel = 0;
+
+    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
+    count = be32_to_cpu(count);
+    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
+    filesbuf = g_malloc(dsize);
+    g_assert(filesbuf);
+    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
+    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
+    for (i = 0; i < count; ++i, ++pdir_entry) {
+        if (!strcmp(pdir_entry->name, filename)) {
+            *size = be32_to_cpu(pdir_entry->size);
+            *sel = be16_to_cpu(pdir_entry->select);
+            found = true;
+            break;
+        }
+    }
+
+    g_free(filesbuf);
+    return found;
+}
+
 /*
  * The caller need check the return value. When the return value is
  * nonzero, it means that some bytes have been transferred.
@@ -73,37 +168,100 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
  * populated, it has received only a starting slice of the fw_cfg file.
  */
 size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
-                      void *data, size_t buflen)
+                        void *data, size_t buflen)
 {
-    uint32_t count;
-    uint32_t i;
-    unsigned char *filesbuf = NULL;
-    size_t dsize;
-    FWCfgFile *pdir_entry;
     size_t filesize = 0;
+    uint32_t len;
+    uint16_t sel;
 
-    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
-    count = be32_to_cpu(count);
-    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
-    filesbuf = g_malloc(dsize);
-    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
-    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
-    for (i = 0; i < count; ++i, ++pdir_entry) {
-        if (!strcmp(pdir_entry->name, filename)) {
-            uint32_t len = be32_to_cpu(pdir_entry->size);
-            uint16_t sel = be16_to_cpu(pdir_entry->select);
-            filesize = len;
-            if (len > buflen) {
-                len = buflen;
-            }
-            qfw_cfg_get(fw_cfg, sel, data, len);
-            break;
+    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
+        filesize = len;
+        if (len > buflen) {
+            len = buflen;
         }
+        qfw_cfg_get(fw_cfg, sel, data, len);
     }
-    g_free(filesbuf);
+
     return filesize;
 }
 
+/*
+ * The caller need check the return value. When the return value is
+ * nonzero, it means that some bytes have been transferred.
+ *
+ * If the fw_cfg file in question is smaller than the allocated & passed-in
+ * buffer, then the first len bytes were read.
+ *
+ * If the fw_cfg file in question is larger than the passed-in
+ * buffer, then the return value explains how much was actually read.
+ *
+ * It is illegal to call this function if fw_cfg does not support DMA
+ * interface. The caller should ensure that DMA is supported before
+ * calling this function.
+ *
+ * Passed QOSState pointer qs must be initialized. qs->alloc must also be
+ * properly initialized.
+ */
+size_t qfw_cfg_read_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
+                         void *data, size_t buflen)
+{
+    uint32_t len = 0;
+    uint16_t sel;
+    uint32_t id;
+
+    g_assert(qs);
+    /* check if DMA is supported since we use DMA for read */
+    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
+    g_assert(id & FW_CFG_VERSION_DMA);
+
+    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
+        if (len > buflen) {
+            len = buflen;
+        }
+        qfw_cfg_read_entry(fw_cfg, qs, sel, data, len);
+    }
+
+    return (size_t) len;
+}
+
+/*
+ * The caller need check the return value. When the return value is
+ * nonzero, it means that some bytes have been transferred.
+ *
+ * If the fw_cfg file in question is smaller than the allocated & passed-in
+ * buffer, then the buffer has been partially written.
+ *
+ * If the fw_cfg file in question is larger than the passed-in
+ * buffer, then the return value explains how much was actually written.
+ *
+ * It is illegal to call this function if fw_cfg does not support DMA
+ * interface. The caller should ensure that DMA is supported before
+ * calling this function.
+ *
+ * Passed QOSState pointer qs must be initialized. qs->alloc must also be
+ * properly initialized.
+ */
+size_t qfw_cfg_write_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
+                          void *data, size_t buflen)
+{
+    uint32_t len = 0;
+    uint16_t sel;
+    uint32_t id;
+
+    g_assert(qs);
+    /* write operation is only valid if DMA is supported */
+    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
+    g_assert(id & FW_CFG_VERSION_DMA);
+
+    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
+        if (len > buflen) {
+            len = buflen;
+        }
+        qfw_cfg_write_entry(fw_cfg, qs, sel, data, len);
+    }
+    return (size_t) len;
+}
+
 static void mm_fw_cfg_read(QFWCFG *fw_cfg, void *data, size_t len)
 {
     uint8_t *ptr = data;
diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
index b0456a15df..63c1ac59d6 100644
--- a/tests/qtest/libqos/fw_cfg.h
+++ b/tests/qtest/libqos/fw_cfg.h
@@ -14,6 +14,7 @@
 #define LIBQOS_FW_CFG_H
 
 #include "../libqtest.h"
+#include "libqos.h"
 
 typedef struct QFWCFG QFWCFG;
 
@@ -33,6 +34,10 @@ uint32_t qfw_cfg_get_u32(QFWCFG *fw_cfg, uint16_t key);
 uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key);
 size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
                         void *data, size_t buflen);
+size_t qfw_cfg_write_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
+                          void *data, size_t buflen);
+size_t qfw_cfg_read_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
+                         void *data, size_t buflen);
 
 QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
 void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
-- 
2.45.2

Re: [PATCH v3] tests/qtest/libqos: add DMA support for writing and reading fw_cfg files

[Daniel P. Berrangé]

On Wed, Jan 08, 2025 at 06:27:50PM +0530, Ani Sinha wrote:
> At present, the libqos/fw_cfg.c library does not support the modern DMA
> interface which is required to write to the fw_cfg files. It only uses the IO
> interface. Implement read and write methods based on DMA. This will enable
> developers to write tests that writes to the fw_cfg file(s). The structure of
> the code is taken from edk2 fw_cfg implementation. It has been tested by
> writing a qtest that writes to a fw_cfg file. This test will be part of a
> future patch series.
> 
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  tests/qtest/libqos/fw_cfg.c | 204 ++++++++++++++++++++++++++++++++----
>  tests/qtest/libqos/fw_cfg.h |   5 +
>  2 files changed, 186 insertions(+), 23 deletions(-)

> +static bool
> +find_pdir_entry(QFWCFG *fw_cfg, const char *filename,
> +                uint16_t *sel, uint32_t *size)
> +{
> +    unsigned char *filesbuf = NULL;

Use g_autofree here instead of later g_free.

> +    uint32_t count;
> +    size_t dsize;
> +    FWCfgFile *pdir_entry;
> +    uint32_t i;
> +    bool found = false;
> +
> +    *size = 0;
> +    *sel = 0;
> +
> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
> +    count = be32_to_cpu(count);
> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
> +    filesbuf = g_malloc(dsize);
> +    g_assert(filesbuf);
> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
> +    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));

I'm not familiar with fwcfg data format, but I'm wondering
what the initial 'uint32_t' data field is that you're skipping
over, and whether its value should be validated before this
loop ?

> +    for (i = 0; i < count; ++i, ++pdir_entry) {
> +        if (!strcmp(pdir_entry->name, filename)) {
> +            *size = be32_to_cpu(pdir_entry->size);
> +            *sel = be16_to_cpu(pdir_entry->select);
> +            found = true;
> +            break;
> +        }
> +    }
> +
> +    g_free(filesbuf);
> +    return found;
> +}

> +
>  /*
>   * The caller need check the return value. When the return value is
>   * nonzero, it means that some bytes have been transferred.
> @@ -73,37 +168,100 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>   * populated, it has received only a starting slice of the fw_cfg file.
>   */
>  size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
> -                      void *data, size_t buflen)
> +                        void *data, size_t buflen)
>  {
> -    uint32_t count;
> -    uint32_t i;
> -    unsigned char *filesbuf = NULL;
> -    size_t dsize;
> -    FWCfgFile *pdir_entry;
>      size_t filesize = 0;
> +    uint32_t len;
> +    uint16_t sel;
>  
> -    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
> -    count = be32_to_cpu(count);
> -    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
> -    filesbuf = g_malloc(dsize);
> -    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
> -    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
> -    for (i = 0; i < count; ++i, ++pdir_entry) {
> -        if (!strcmp(pdir_entry->name, filename)) {
> -            uint32_t len = be32_to_cpu(pdir_entry->size);
> -            uint16_t sel = be16_to_cpu(pdir_entry->select);
> -            filesize = len;
> -            if (len > buflen) {
> -                len = buflen;
> -            }
> -            qfw_cfg_get(fw_cfg, sel, data, len);
> -            break;
> +    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
> +        filesize = len;
> +        if (len > buflen) {
> +            len = buflen;
>          }
> +        qfw_cfg_get(fw_cfg, sel, data, len);
>      }
> -    g_free(filesbuf);
> +
>      return filesize;
>  }

I'd recommend refactoring of existnig code, be a separate commit
from the newly added functionality.

>  
> +/*
> + * The caller need check the return value. When the return value is
> + * nonzero, it means that some bytes have been transferred.
> + *
> + * If the fw_cfg file in question is smaller than the allocated & passed-in
> + * buffer, then the first len bytes were read.
> + *
> + * If the fw_cfg file in question is larger than the passed-in
> + * buffer, then the return value explains how much was actually read.
> + *
> + * It is illegal to call this function if fw_cfg does not support DMA
> + * interface. The caller should ensure that DMA is supported before
> + * calling this function.
> + *
> + * Passed QOSState pointer qs must be initialized. qs->alloc must also be
> + * properly initialized.
> + */
> +size_t qfw_cfg_read_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
> +                         void *data, size_t buflen)
> +{
> +    uint32_t len = 0;
> +    uint16_t sel;
> +    uint32_t id;
> +
> +    g_assert(qs);
> +    /* check if DMA is supported since we use DMA for read */
> +    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
> +    g_assert(id & FW_CFG_VERSION_DMA);
> +
> +    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
> +        if (len > buflen) {
> +            len = buflen;
> +        }
> +        qfw_cfg_read_entry(fw_cfg, qs, sel, data, len);
> +    }
> +
> +    return (size_t) len;

The size_t cast is redundant, since we know sizeof(size_t)
will be >= sizeof(uint32_t) on all platforms

> +}
> +
> +/*
> + * The caller need check the return value. When the return value is
> + * nonzero, it means that some bytes have been transferred.
> + *
> + * If the fw_cfg file in question is smaller than the allocated & passed-in
> + * buffer, then the buffer has been partially written.
> + *
> + * If the fw_cfg file in question is larger than the passed-in
> + * buffer, then the return value explains how much was actually written.
> + *
> + * It is illegal to call this function if fw_cfg does not support DMA
> + * interface. The caller should ensure that DMA is supported before
> + * calling this function.
> + *
> + * Passed QOSState pointer qs must be initialized. qs->alloc must also be
> + * properly initialized.
> + */
> +size_t qfw_cfg_write_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
> +                          void *data, size_t buflen)
> +{
> +    uint32_t len = 0;
> +    uint16_t sel;
> +    uint32_t id;
> +
> +    g_assert(qs);
> +    /* write operation is only valid if DMA is supported */
> +    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
> +    g_assert(id & FW_CFG_VERSION_DMA);
> +
> +    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
> +        if (len > buflen) {
> +            len = buflen;
> +        }
> +        qfw_cfg_write_entry(fw_cfg, qs, sel, data, len);
> +    }
> +    return (size_t) len;

Another redundant cast


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3] tests/qtest/libqos: add DMA support for writing and reading fw_cfg files

[Ani Sinha]

> On 8 Jan 2025, at 6:38 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Wed, Jan 08, 2025 at 06:27:50PM +0530, Ani Sinha wrote:
>> At present, the libqos/fw_cfg.c library does not support the modern DMA
>> interface which is required to write to the fw_cfg files. It only uses the IO
>> interface. Implement read and write methods based on DMA. This will enable
>> developers to write tests that writes to the fw_cfg file(s). The structure of
>> the code is taken from edk2 fw_cfg implementation. It has been tested by
>> writing a qtest that writes to a fw_cfg file. This test will be part of a
>> future patch series.
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>> tests/qtest/libqos/fw_cfg.c | 204 ++++++++++++++++++++++++++++++++----
>> tests/qtest/libqos/fw_cfg.h |   5 +
>> 2 files changed, 186 insertions(+), 23 deletions(-)
> 
>> +static bool
>> +find_pdir_entry(QFWCFG *fw_cfg, const char *filename,
>> +                uint16_t *sel, uint32_t *size)
>> +{
>> +    unsigned char *filesbuf = NULL;
> 
> Use g_autofree here instead of later g_free.

OK will send just a refactoring patch with this change.

> 
>> +    uint32_t count;
>> +    size_t dsize;
>> +    FWCfgFile *pdir_entry;
>> +    uint32_t i;
>> +    bool found = false;
>> +
>> +    *size = 0;
>> +    *sel = 0;
>> +
>> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
>> +    count = be32_to_cpu(count);
>> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
>> +    filesbuf = g_malloc(dsize);
>> +    g_assert(filesbuf);
>> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
>> +    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
> 
> I'm not familiar with fwcfg data format, but I'm wondering
> what the initial 'uint32_t' data field is that you're skipping
> over, and whether its value should be validated before this
> loop ?

This part I left as is from previous code. From https://www.qemu.org/docs/master/specs/fw_cfg.html

struct FWCfgFiles {         /* the entire file directory fw_cfg item */
     uint32_t count;        /* number of entries, in big-endian format */
     struct FWCfgFile f[]; /* array of file entries, see below */
};

struct FWCfgFile {       /* an individual file entry, 64 bytes total */
    uint32_t size;       /* size of referenced fw_cfg item, big-endian */
    uint16_t select;     /* selector key of fw_cfg item, big-endian */
    uint16_t reserved;
    char name[56];       /* fw_cfg item name, NUL-terminated ascii */
};

So the code first reads the count and then allocates ‘count' entries for ‘count' files.

> 
>> +    for (i = 0; i < count; ++i, ++pdir_entry) {
>> +        if (!strcmp(pdir_entry->name, filename)) {
>> +            *size = be32_to_cpu(pdir_entry->size);
>> +            *sel = be16_to_cpu(pdir_entry->select);
>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    g_free(filesbuf);
>> +    return found;
>> +}
> 
>> +
>> /*
>>  * The caller need check the return value. When the return value is
>>  * nonzero, it means that some bytes have been transferred.
>> @@ -73,37 +168,100 @@ static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>>  * populated, it has received only a starting slice of the fw_cfg file.
>>  */
>> size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
>> -                      void *data, size_t buflen)
>> +                        void *data, size_t buflen)
>> {
>> -    uint32_t count;
>> -    uint32_t i;
>> -    unsigned char *filesbuf = NULL;
>> -    size_t dsize;
>> -    FWCfgFile *pdir_entry;
>>     size_t filesize = 0;
>> +    uint32_t len;
>> +    uint16_t sel;
>> 
>> -    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
>> -    count = be32_to_cpu(count);
>> -    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
>> -    filesbuf = g_malloc(dsize);
>> -    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
>> -    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
>> -    for (i = 0; i < count; ++i, ++pdir_entry) {
>> -        if (!strcmp(pdir_entry->name, filename)) {
>> -            uint32_t len = be32_to_cpu(pdir_entry->size);
>> -            uint16_t sel = be16_to_cpu(pdir_entry->select);
>> -            filesize = len;
>> -            if (len > buflen) {
>> -                len = buflen;
>> -            }
>> -            qfw_cfg_get(fw_cfg, sel, data, len);
>> -            break;
>> +    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
>> +        filesize = len;
>> +        if (len > buflen) {
>> +            len = buflen;
>>         }
>> +        qfw_cfg_get(fw_cfg, sel, data, len);
>>     }
>> -    g_free(filesbuf);
>> +
>>     return filesize;
>> }
> 
> I'd recommend refactoring of existnig code, be a separate commit
> from the newly added functionality.

Yes will do that.

> 
>> 
>> +/*
>> + * The caller need check the return value. When the return value is
>> + * nonzero, it means that some bytes have been transferred.
>> + *
>> + * If the fw_cfg file in question is smaller than the allocated & passed-in
>> + * buffer, then the first len bytes were read.
>> + *
>> + * If the fw_cfg file in question is larger than the passed-in
>> + * buffer, then the return value explains how much was actually read.
>> + *
>> + * It is illegal to call this function if fw_cfg does not support DMA
>> + * interface. The caller should ensure that DMA is supported before
>> + * calling this function.
>> + *
>> + * Passed QOSState pointer qs must be initialized. qs->alloc must also be
>> + * properly initialized.
>> + */
>> +size_t qfw_cfg_read_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
>> +                         void *data, size_t buflen)
>> +{
>> +    uint32_t len = 0;
>> +    uint16_t sel;
>> +    uint32_t id;
>> +
>> +    g_assert(qs);
>> +    /* check if DMA is supported since we use DMA for read */
>> +    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
>> +    g_assert(id & FW_CFG_VERSION_DMA);
>> +
>> +    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
>> +        if (len > buflen) {
>> +            len = buflen;
>> +        }
>> +        qfw_cfg_read_entry(fw_cfg, qs, sel, data, len);
>> +    }
>> +
>> +    return (size_t) len;
> 
> The size_t cast is redundant, since we know sizeof(size_t)
> will be >= sizeof(uint32_t) on all platforms
> 
>> +}
>> +
>> +/*
>> + * The caller need check the return value. When the return value is
>> + * nonzero, it means that some bytes have been transferred.
>> + *
>> + * If the fw_cfg file in question is smaller than the allocated & passed-in
>> + * buffer, then the buffer has been partially written.
>> + *
>> + * If the fw_cfg file in question is larger than the passed-in
>> + * buffer, then the return value explains how much was actually written.
>> + *
>> + * It is illegal to call this function if fw_cfg does not support DMA
>> + * interface. The caller should ensure that DMA is supported before
>> + * calling this function.
>> + *
>> + * Passed QOSState pointer qs must be initialized. qs->alloc must also be
>> + * properly initialized.
>> + */
>> +size_t qfw_cfg_write_file(QFWCFG *fw_cfg, QOSState *qs, const char *filename,
>> +                          void *data, size_t buflen)
>> +{
>> +    uint32_t len = 0;
>> +    uint16_t sel;
>> +    uint32_t id;
>> +
>> +    g_assert(qs);
>> +    /* write operation is only valid if DMA is supported */
>> +    id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
>> +    g_assert(id & FW_CFG_VERSION_DMA);
>> +
>> +    if (find_pdir_entry(fw_cfg, filename, &sel, &len)) {
>> +        if (len > buflen) {
>> +            len = buflen;
>> +        }
>> +        qfw_cfg_write_entry(fw_cfg, qs, sel, data, len);
>> +    }
>> +    return (size_t) len;
> 
> Another redundant cast
> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3] tests/qtest/libqos: add DMA support for writing and reading fw_cfg files

[Daniel P. Berrangé]

On Wed, Jan 08, 2025 at 06:47:25PM +0530, Ani Sinha wrote:
> 
> 
> > On 8 Jan 2025, at 6:38 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > 
> > On Wed, Jan 08, 2025 at 06:27:50PM +0530, Ani Sinha wrote:
> >> At present, the libqos/fw_cfg.c library does not support the modern DMA
> >> interface which is required to write to the fw_cfg files. It only uses the IO
> >> interface. Implement read and write methods based on DMA. This will enable
> >> developers to write tests that writes to the fw_cfg file(s). The structure of
> >> the code is taken from edk2 fw_cfg implementation. It has been tested by
> >> writing a qtest that writes to a fw_cfg file. This test will be part of a
> >> future patch series.
> >> 
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >> ---
> >> tests/qtest/libqos/fw_cfg.c | 204 ++++++++++++++++++++++++++++++++----
> >> tests/qtest/libqos/fw_cfg.h |   5 +
> >> 2 files changed, 186 insertions(+), 23 deletions(-)
> > 
> >> +static bool
> >> +find_pdir_entry(QFWCFG *fw_cfg, const char *filename,
> >> +                uint16_t *sel, uint32_t *size)
> >> +{
> >> +    unsigned char *filesbuf = NULL;
> > 
> > Use g_autofree here instead of later g_free.
> 
> OK will send just a refactoring patch with this change.
> 
> > 
> >> +    uint32_t count;
> >> +    size_t dsize;
> >> +    FWCfgFile *pdir_entry;
> >> +    uint32_t i;
> >> +    bool found = false;
> >> +
> >> +    *size = 0;
> >> +    *sel = 0;
> >> +
> >> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
> >> +    count = be32_to_cpu(count);
> >> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
> >> +    filesbuf = g_malloc(dsize);
> >> +    g_assert(filesbuf);
> >> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
> >> +    pdir_entry = (FWCfgFile *)(filesbuf + sizeof(uint32_t));
> > 
> > I'm not familiar with fwcfg data format, but I'm wondering
> > what the initial 'uint32_t' data field is that you're skipping
> > over, and whether its value should be validated before this
> > loop ?
> 
> This part I left as is from previous code. From https://www.qemu.org/docs/master/specs/fw_cfg.html
> 
> struct FWCfgFiles {         /* the entire file directory fw_cfg item */
>      uint32_t count;        /* number of entries, in big-endian format */
>      struct FWCfgFile f[]; /* array of file entries, see below */
> };
> 
> struct FWCfgFile {       /* an individual file entry, 64 bytes total */
>     uint32_t size;       /* size of referenced fw_cfg item, big-endian */
>     uint16_t select;     /* selector key of fw_cfg item, big-endian */
>     uint16_t reserved;
>     char name[56];       /* fw_cfg item name, NUL-terminated ascii */
> };
> 
> So the code first reads the count and then allocates ‘count' entries for ‘count' files.

Ah right, so the first qfw_cfg_get already read count,
and the second qfw_cfg_get reads it again, followed by
the entries, so we can ignore that first field.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|