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 :|