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