| Message ID | 1500025208-14827-4-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote: > By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility > for the internal MemoryRegion fields to be mapped by name for boards that wish > to wire up the fw_cfg device themselves. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> [...] > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index b980cba..b77ea48 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -1,8 +1,19 @@ > #ifndef FW_CFG_H > #define FW_CFG_H > > +#include "qemu/typedefs.h" > #include "exec/hwaddr.h" > #include "hw/nvram/fw_cfg_keys.h" > +#include "hw/sysbus.h" > +#include "sysemu/dma.h" > + > +#define TYPE_FW_CFG "fw_cfg" > +#define TYPE_FW_CFG_IO "fw_cfg_io" > +#define TYPE_FW_CFG_MEM "fw_cfg_mem" > + > +#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) > +#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) > +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) > > typedef struct FWCfgFile { > uint32_t size; /* file size */ > @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess { > > typedef void (*FWCfgReadCallback)(void *opaque); > > +struct FWCfgState { > + /*< private >*/ > + SysBusDevice parent_obj; > + /*< public >*/ > + > + uint16_t file_slots; > + FWCfgEntry *entries[2]; > + int *entry_order; > + FWCfgFiles *files; > + uint16_t cur_entry; > + uint32_t cur_offset; > + Notifier machine_ready; > + > + int fw_cfg_order_override; > + > + bool dma_enabled; > + dma_addr_t dma_addr; > + AddressSpace *dma_as; > + MemoryRegion dma_iomem; > +}; > + > +struct FWCfgIoState { > + /*< private >*/ > + FWCfgState parent_obj; > + /*< public >*/ > + > + MemoryRegion comb_iomem; > +}; > + > +struct FWCfgMemState { > + /*< private >*/ > + FWCfgState parent_obj; > + /*< public >*/ > + > + MemoryRegion ctl_iomem, data_iomem; > + uint32_t data_width; > + MemoryRegionOps wide_data_ops; > +}; Why do you need the full struct declaration to be exposed in the header? The memory regions are supposed to be visible as QOM children to the fw_cfg device, already.
On 07/14/17 20:09, Eduardo Habkost wrote: > On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote: >> By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility >> for the internal MemoryRegion fields to be mapped by name for boards that wish >> to wire up the fw_cfg device themselves. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > [...] >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >> index b980cba..b77ea48 100644 >> --- a/include/hw/nvram/fw_cfg.h >> +++ b/include/hw/nvram/fw_cfg.h >> @@ -1,8 +1,19 @@ >> #ifndef FW_CFG_H >> #define FW_CFG_H >> >> +#include "qemu/typedefs.h" >> #include "exec/hwaddr.h" >> #include "hw/nvram/fw_cfg_keys.h" >> +#include "hw/sysbus.h" >> +#include "sysemu/dma.h" >> + >> +#define TYPE_FW_CFG "fw_cfg" >> +#define TYPE_FW_CFG_IO "fw_cfg_io" >> +#define TYPE_FW_CFG_MEM "fw_cfg_mem" >> + >> +#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) >> +#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) >> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) >> >> typedef struct FWCfgFile { >> uint32_t size; /* file size */ >> @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess { >> >> typedef void (*FWCfgReadCallback)(void *opaque); >> >> +struct FWCfgState { >> + /*< private >*/ >> + SysBusDevice parent_obj; >> + /*< public >*/ >> + >> + uint16_t file_slots; >> + FWCfgEntry *entries[2]; >> + int *entry_order; >> + FWCfgFiles *files; >> + uint16_t cur_entry; >> + uint32_t cur_offset; >> + Notifier machine_ready; >> + >> + int fw_cfg_order_override; >> + >> + bool dma_enabled; >> + dma_addr_t dma_addr; >> + AddressSpace *dma_as; >> + MemoryRegion dma_iomem; >> +}; >> + >> +struct FWCfgIoState { >> + /*< private >*/ >> + FWCfgState parent_obj; >> + /*< public >*/ >> + >> + MemoryRegion comb_iomem; >> +}; >> + >> +struct FWCfgMemState { >> + /*< private >*/ >> + FWCfgState parent_obj; >> + /*< public >*/ >> + >> + MemoryRegion ctl_iomem, data_iomem; >> + uint32_t data_width; >> + MemoryRegionOps wide_data_ops; >> +}; > > Why do you need the full struct declaration to be exposed in the > header? Different board code wants to hook up "comb_iomem" manually to different address spaces, so they need to access the field directly. This is the ultimate goal of the entire exercise, IIRC. > The memory regions are supposed to be visible as QOM > children to the fw_cfg device, already. I don't understand this. How else can board code work with "comb_iomem" than described above? If there is a way, I agree it would be preferable. Thanks Laszlo
On Fri, Jul 14, 2017 at 08:28:37PM +0200, Laszlo Ersek wrote: > On 07/14/17 20:09, Eduardo Habkost wrote: > > On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote: > >> By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility > >> for the internal MemoryRegion fields to be mapped by name for boards that wish > >> to wire up the fw_cfg device themselves. > >> > >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > [...] > >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > >> index b980cba..b77ea48 100644 > >> --- a/include/hw/nvram/fw_cfg.h > >> +++ b/include/hw/nvram/fw_cfg.h > >> @@ -1,8 +1,19 @@ > >> #ifndef FW_CFG_H > >> #define FW_CFG_H > >> > >> +#include "qemu/typedefs.h" > >> #include "exec/hwaddr.h" > >> #include "hw/nvram/fw_cfg_keys.h" > >> +#include "hw/sysbus.h" > >> +#include "sysemu/dma.h" > >> + > >> +#define TYPE_FW_CFG "fw_cfg" > >> +#define TYPE_FW_CFG_IO "fw_cfg_io" > >> +#define TYPE_FW_CFG_MEM "fw_cfg_mem" > >> + > >> +#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) > >> +#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) > >> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) > >> > >> typedef struct FWCfgFile { > >> uint32_t size; /* file size */ > >> @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess { > >> > >> typedef void (*FWCfgReadCallback)(void *opaque); > >> > >> +struct FWCfgState { > >> + /*< private >*/ > >> + SysBusDevice parent_obj; > >> + /*< public >*/ > >> + > >> + uint16_t file_slots; > >> + FWCfgEntry *entries[2]; > >> + int *entry_order; > >> + FWCfgFiles *files; > >> + uint16_t cur_entry; > >> + uint32_t cur_offset; > >> + Notifier machine_ready; > >> + > >> + int fw_cfg_order_override; > >> + > >> + bool dma_enabled; > >> + dma_addr_t dma_addr; > >> + AddressSpace *dma_as; > >> + MemoryRegion dma_iomem; > >> +}; > >> + > >> +struct FWCfgIoState { > >> + /*< private >*/ > >> + FWCfgState parent_obj; > >> + /*< public >*/ > >> + > >> + MemoryRegion comb_iomem; > >> +}; > >> + > >> +struct FWCfgMemState { > >> + /*< private >*/ > >> + FWCfgState parent_obj; > >> + /*< public >*/ > >> + > >> + MemoryRegion ctl_iomem, data_iomem; > >> + uint32_t data_width; > >> + MemoryRegionOps wide_data_ops; > >> +}; > > > > Why do you need the full struct declaration to be exposed in the > > header? > > Different board code wants to hook up "comb_iomem" manually to different > address spaces, so they need to access the field directly. This is the > ultimate goal of the entire exercise, IIRC. > > > The memory regions are supposed to be visible as QOM > > children to the fw_cfg device, already. > > I don't understand this. How else can board code work with "comb_iomem" > than described above? If there is a way, I agree it would be preferable. object_resolve_path_component(fw_cfg, "fwcfg[0]") and object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively. I don't know why those names were chosen, though. Probably it's a good idea to call object_property_add_child() manually with more appropriate names inside the fw_cfg code instead of letting memory_region_init() pick the child name.
On 14/07/17 19:56, Eduardo Habkost wrote: >>> Why do you need the full struct declaration to be exposed in the >>> header? >> >> Different board code wants to hook up "comb_iomem" manually to different >> address spaces, so they need to access the field directly. This is the >> ultimate goal of the entire exercise, IIRC. Yes, that is correct (and I believe is mentioned in the cover letter, too). >>> The memory regions are supposed to be visible as QOM >>> children to the fw_cfg device, already. >> >> I don't understand this. How else can board code work with "comb_iomem" >> than described above? If there is a way, I agree it would be preferable. > > object_resolve_path_component(fw_cfg, "fwcfg[0]") and > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively. > > I don't know why those names were chosen, though. Probably it's > a good idea to call object_property_add_child() manually with > more appropriate names inside the fw_cfg code instead of letting > memory_region_init() pick the child name. That's interesting. I did a grep of the codebase for object_resolve_path_component and struggled to find an instance where it was being used to provide access to a MemoryRegion. Even if it's an available feature, it's certainly not one that is widely known about. In terms of the patch, is the v9 revision suitable for commit or does this constitute a NACK? ATB, Mark.
On Sun, 16 Jul 2017 20:12:13 +0100 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 14/07/17 19:56, Eduardo Habkost wrote: > > >>> Why do you need the full struct declaration to be exposed in the > >>> header? > >> > >> Different board code wants to hook up "comb_iomem" manually to different > >> address spaces, so they need to access the field directly. This is the > >> ultimate goal of the entire exercise, IIRC. > > Yes, that is correct (and I believe is mentioned in the cover letter, too). > > >>> The memory regions are supposed to be visible as QOM > >>> children to the fw_cfg device, already. > >> > >> I don't understand this. How else can board code work with "comb_iomem" > >> than described above? If there is a way, I agree it would be preferable. > > > > object_resolve_path_component(fw_cfg, "fwcfg[0]") and > > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should > > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively. > > > > I don't know why those names were chosen, though. Probably it's > > a good idea to call object_property_add_child() manually with > > more appropriate names inside the fw_cfg code instead of letting > > memory_region_init() pick the child name. > > That's interesting. I did a grep of the codebase for > object_resolve_path_component and struggled to find an instance where it > was being used to provide access to a MemoryRegion. Even if it's an > available feature, it's certainly not one that is widely known about. Agreed, we haven't used suggested approach before and I don't see benefits it will bring in fwcfg case. Eduardo, Could you just apply v9, which seems to be good enough and does the intended job before 2.10 goes into soft-freeze? we always could do object_resolve_path_component() on top if it makes sense. > > In terms of the patch, is the v9 revision suitable for commit or does > this constitute a NACK? > > > ATB, > > Mark.
On Sun, Jul 16, 2017 at 08:12:13PM +0100, Mark Cave-Ayland wrote: > On 14/07/17 19:56, Eduardo Habkost wrote: > > >>> Why do you need the full struct declaration to be exposed in the > >>> header? > >> > >> Different board code wants to hook up "comb_iomem" manually to different > >> address spaces, so they need to access the field directly. This is the > >> ultimate goal of the entire exercise, IIRC. > > Yes, that is correct (and I believe is mentioned in the cover letter, too). > > >>> The memory regions are supposed to be visible as QOM > >>> children to the fw_cfg device, already. > >> > >> I don't understand this. How else can board code work with "comb_iomem" > >> than described above? If there is a way, I agree it would be preferable. > > > > object_resolve_path_component(fw_cfg, "fwcfg[0]") and > > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should > > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively. > > > > I don't know why those names were chosen, though. Probably it's > > a good idea to call object_property_add_child() manually with > > more appropriate names inside the fw_cfg code instead of letting > > memory_region_init() pick the child name. > > That's interesting. I did a grep of the codebase for > object_resolve_path_component and struggled to find an instance where it > was being used to provide access to a MemoryRegion. Even if it's an > available feature, it's certainly not one that is widely known about. > > In terms of the patch, is the v9 revision suitable for commit or does > this constitute a NACK? It's not a NACK. Using QOM properties instead of direct struct access may be nice (I'm not 100% sure, either), but not required.
On Mon, Jul 17, 2017 at 01:03:11PM +0200, Igor Mammedov wrote: > On Sun, 16 Jul 2017 20:12:13 +0100 > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > > On 14/07/17 19:56, Eduardo Habkost wrote: > > > > >>> Why do you need the full struct declaration to be exposed in the > > >>> header? > > >> > > >> Different board code wants to hook up "comb_iomem" manually to different > > >> address spaces, so they need to access the field directly. This is the > > >> ultimate goal of the entire exercise, IIRC. > > > > Yes, that is correct (and I believe is mentioned in the cover letter, too). > > > > >>> The memory regions are supposed to be visible as QOM > > >>> children to the fw_cfg device, already. > > >> > > >> I don't understand this. How else can board code work with "comb_iomem" > > >> than described above? If there is a way, I agree it would be preferable. > > > > > > object_resolve_path_component(fw_cfg, "fwcfg[0]") and > > > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should > > > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively. > > > > > > I don't know why those names were chosen, though. Probably it's > > > a good idea to call object_property_add_child() manually with > > > more appropriate names inside the fw_cfg code instead of letting > > > memory_region_init() pick the child name. > > > > That's interesting. I did a grep of the codebase for > > object_resolve_path_component and struggled to find an instance where it > > was being used to provide access to a MemoryRegion. Even if it's an > > available feature, it's certainly not one that is widely known about. > Agreed, > > we haven't used suggested approach before and I don't see > benefits it will bring in fwcfg case. > > Eduardo, > > Could you just apply v9, which seems to be good enough and > does the intended job before 2.10 goes into soft-freeze? > we always could do object_resolve_path_component() on top > if it makes sense. I was hoping Michael would apply it (as fw_cfg is currently closer to PC code than to i386 or machine core). But I can apply it if he didn't queue it yet.
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index b850ac3..2375f21 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -37,14 +37,6 @@ #define FW_CFG_FILE_SLOTS_DFLT 0x20 -#define TYPE_FW_CFG "fw_cfg" -#define TYPE_FW_CFG_IO "fw_cfg_io" -#define TYPE_FW_CFG_MEM "fw_cfg_mem" - -#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) -#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) -#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) - /* FW_CFG_VERSION bits */ #define FW_CFG_VERSION 0x01 #define FW_CFG_VERSION_DMA 0x02 @@ -58,51 +50,12 @@ #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */ -typedef struct FWCfgEntry { +struct FWCfgEntry { uint32_t len; bool allow_write; uint8_t *data; void *callback_opaque; FWCfgReadCallback read_callback; -} FWCfgEntry; - -struct FWCfgState { - /*< private >*/ - SysBusDevice parent_obj; - /*< public >*/ - - uint16_t file_slots; - FWCfgEntry *entries[2]; - int *entry_order; - FWCfgFiles *files; - uint16_t cur_entry; - uint32_t cur_offset; - Notifier machine_ready; - - int fw_cfg_order_override; - - bool dma_enabled; - dma_addr_t dma_addr; - AddressSpace *dma_as; - MemoryRegion dma_iomem; -}; - -struct FWCfgIoState { - /*< private >*/ - FWCfgState parent_obj; - /*< public >*/ - - MemoryRegion comb_iomem; -}; - -struct FWCfgMemState { - /*< private >*/ - FWCfgState parent_obj; - /*< public >*/ - - MemoryRegion ctl_iomem, data_iomem; - uint32_t data_width; - MemoryRegionOps wide_data_ops; }; #define JPG_FILE 0 diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h index b980cba..b77ea48 100644 --- a/include/hw/nvram/fw_cfg.h +++ b/include/hw/nvram/fw_cfg.h @@ -1,8 +1,19 @@ #ifndef FW_CFG_H #define FW_CFG_H +#include "qemu/typedefs.h" #include "exec/hwaddr.h" #include "hw/nvram/fw_cfg_keys.h" +#include "hw/sysbus.h" +#include "sysemu/dma.h" + +#define TYPE_FW_CFG "fw_cfg" +#define TYPE_FW_CFG_IO "fw_cfg_io" +#define TYPE_FW_CFG_MEM "fw_cfg_mem" + +#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG) +#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO) +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM) typedef struct FWCfgFile { uint32_t size; /* file size */ @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess { typedef void (*FWCfgReadCallback)(void *opaque); +struct FWCfgState { + /*< private >*/ + SysBusDevice parent_obj; + /*< public >*/ + + uint16_t file_slots; + FWCfgEntry *entries[2]; + int *entry_order; + FWCfgFiles *files; + uint16_t cur_entry; + uint32_t cur_offset; + Notifier machine_ready; + + int fw_cfg_order_override; + + bool dma_enabled; + dma_addr_t dma_addr; + AddressSpace *dma_as; + MemoryRegion dma_iomem; +}; + +struct FWCfgIoState { + /*< private >*/ + FWCfgState parent_obj; + /*< public >*/ + + MemoryRegion comb_iomem; +}; + +struct FWCfgMemState { + /*< private >*/ + FWCfgState parent_obj; + /*< public >*/ + + MemoryRegion ctl_iomem, data_iomem; + uint32_t data_width; + MemoryRegionOps wide_data_ops; +}; + /** * fw_cfg_add_bytes: * @s: fw_cfg device being modified diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index 2706aab..0a23020 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -30,6 +30,7 @@ typedef struct DisplaySurface DisplaySurface; typedef struct DriveInfo DriveInfo; typedef struct Error Error; typedef struct EventNotifier EventNotifier; +typedef struct FWCfgEntry FWCfgEntry; typedef struct FWCfgIoState FWCfgIoState; typedef struct FWCfgMemState FWCfgMemState; typedef struct FWCfgState FWCfgState;