On Wed, Jan 08, 2025 at 06:47:25PM +0530, Ani Sinha wrote: > > > > On 8 Jan 2025, at 6:38 PM, Daniel P. Berrangé <[email protected]> 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 <[email protected]> > >> --- > >> 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 :|
