| Message ID | 20201011194918.3219195-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Replace some magic by definitions | expand |
On Sun, 11 Oct 2020 at 20:49, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Use self-explicit WINDOW_COUNT definition instead of a magic value. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/pci-host/versatile.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c > index b4951023f4e..4d9565de4b1 100644 > --- a/hw/pci-host/versatile.c > +++ b/hw/pci-host/versatile.c > @@ -72,6 +72,8 @@ enum { > PCI_VPB_IRQMAP_FORCE_OK, > }; > > +#define WINDOW_COUNT 3 > + > struct PCIVPBState { > PCIHostState parent_obj; > > @@ -86,18 +88,18 @@ struct PCIVPBState { > * The offsets into pci_mem_space are controlled by the imap registers. > */ > MemoryRegion pci_io_window; > - MemoryRegion pci_mem_window[3]; > + MemoryRegion pci_mem_window[WINDOW_COUNT]; > PCIBus pci_bus; > PCIDevice pci_dev; > > /* Constant for life of device: */ > int realview; > - uint32_t mem_win_size[3]; > + uint32_t mem_win_size[WINDOW_COUNT]; > uint8_t irq_mapping_prop; > > /* Variable state: */ > - uint32_t imap[3]; > - uint32_t smap[3]; > + uint32_t imap[WINDOW_COUNT]; > + uint32_t smap[WINDOW_COUNT]; Strictly speaking, this is conflating two separate things which both happen to be equal to three. The versatile/realview PCI controller has: * three memory windows in the system address space - those are represented by the pci_mem_window[] array - mem_win_size[] holds the size of each window (which is fixed but varies between the different implementations of this controller on different boards) - the device IMAPn registers allow the guest to configure the mapping from "a CPU access to an address in window n" to "a PCI address on the PCI bus, and our imap[] array holds those register values * three PCI BARs which represent memory windows on the PCI bus which bus-master PCI devices can use to write back into the system address space - the device SMAPn registers allow the guest to configure the mapping from "a bus-master access to an address on the PCI bus wherever the guest mapped BAR n" to "a system address", and the smap[] array holds those register values There's no inherent reason why the number of PCI BARs needs to be the same as the number of system address space memory windows, so they shouldn't really share the same constant. (We don't actually implement the behaviour of the SMAP registers and the BARs, because Linux always configures the PCI controller to a 1:1 mapping of PCI space to system address space. So we get away with just having our implementation be "always do direct accesses".) thanks -- PMM
On 10/11/20 10:46 PM, Peter Maydell wrote: > On Sun, 11 Oct 2020 at 20:49, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> Use self-explicit WINDOW_COUNT definition instead of a magic value. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/pci-host/versatile.c | 28 ++++++++++++++-------------- >> 1 file changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c >> index b4951023f4e..4d9565de4b1 100644 >> --- a/hw/pci-host/versatile.c >> +++ b/hw/pci-host/versatile.c >> @@ -72,6 +72,8 @@ enum { >> PCI_VPB_IRQMAP_FORCE_OK, >> }; >> >> +#define WINDOW_COUNT 3 >> + >> struct PCIVPBState { >> PCIHostState parent_obj; >> >> @@ -86,18 +88,18 @@ struct PCIVPBState { >> * The offsets into pci_mem_space are controlled by the imap registers. >> */ >> MemoryRegion pci_io_window; >> - MemoryRegion pci_mem_window[3]; >> + MemoryRegion pci_mem_window[WINDOW_COUNT]; >> PCIBus pci_bus; >> PCIDevice pci_dev; >> >> /* Constant for life of device: */ >> int realview; >> - uint32_t mem_win_size[3]; >> + uint32_t mem_win_size[WINDOW_COUNT]; >> uint8_t irq_mapping_prop; >> >> /* Variable state: */ >> - uint32_t imap[3]; >> - uint32_t smap[3]; >> + uint32_t imap[WINDOW_COUNT]; >> + uint32_t smap[WINDOW_COUNT]; > > Strictly speaking, this is conflating two separate > things which both happen to be equal to three. > > The versatile/realview PCI controller has: > * three memory windows in the system address space > - those are represented by the pci_mem_window[] array > - mem_win_size[] holds the size of each window > (which is fixed but varies between the different > implementations of this controller on different boards) > - the device IMAPn registers allow the guest to > configure the mapping from "a CPU access to an > address in window n" to "a PCI address on the PCI bus, > and our imap[] array holds those register values > * three PCI BARs which represent memory windows on the > PCI bus which bus-master PCI devices can use to > write back into the system address space > - the device SMAPn registers allow the guest to configure > the mapping from "a bus-master access to an address > on the PCI bus wherever the guest mapped BAR n" > to "a system address", and the smap[] array holds > those register values > There's no inherent reason why the number of PCI BARs > needs to be the same as the number of system address > space memory windows, so they shouldn't really share > the same constant. Thanks for the detailed explanation, I'll update. > > (We don't actually implement the behaviour of the SMAP > registers and the BARs, because Linux always configures > the PCI controller to a 1:1 mapping of PCI space to > system address space. So we get away with just having > our implementation be "always do direct accesses".) > > thanks > -- PMM >
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c index b4951023f4e..4d9565de4b1 100644 --- a/hw/pci-host/versatile.c +++ b/hw/pci-host/versatile.c @@ -72,6 +72,8 @@ enum { PCI_VPB_IRQMAP_FORCE_OK, }; +#define WINDOW_COUNT 3 + struct PCIVPBState { PCIHostState parent_obj; @@ -86,18 +88,18 @@ struct PCIVPBState { * The offsets into pci_mem_space are controlled by the imap registers. */ MemoryRegion pci_io_window; - MemoryRegion pci_mem_window[3]; + MemoryRegion pci_mem_window[WINDOW_COUNT]; PCIBus pci_bus; PCIDevice pci_dev; /* Constant for life of device: */ int realview; - uint32_t mem_win_size[3]; + uint32_t mem_win_size[WINDOW_COUNT]; uint8_t irq_mapping_prop; /* Variable state: */ - uint32_t imap[3]; - uint32_t smap[3]; + uint32_t imap[WINDOW_COUNT]; + uint32_t smap[WINDOW_COUNT]; uint32_t selfid; uint32_t flags; uint8_t irq_mapping; @@ -130,7 +132,7 @@ static void pci_vpb_update_all_windows(PCIVPBState *s) /* Update all alias windows based on the current register state */ int i; - for (i = 0; i < 3; i++) { + for (i = 0; i < WINDOW_COUNT; i++) { pci_vpb_update_window(s, i); } } @@ -148,8 +150,8 @@ static const VMStateDescription pci_vpb_vmstate = { .minimum_version_id = 1, .post_load = pci_vpb_post_load, .fields = (VMStateField[]) { - VMSTATE_UINT32_ARRAY(imap, PCIVPBState, 3), - VMSTATE_UINT32_ARRAY(smap, PCIVPBState, 3), + VMSTATE_UINT32_ARRAY(imap, PCIVPBState, WINDOW_COUNT), + VMSTATE_UINT32_ARRAY(smap, PCIVPBState, WINDOW_COUNT), VMSTATE_UINT32(selfid, PCIVPBState), VMSTATE_UINT32(flags, PCIVPBState), VMSTATE_UINT8(irq_mapping, PCIVPBState), @@ -371,12 +373,10 @@ static void pci_vpb_reset(DeviceState *d) { PCIVPBState *s = PCI_VPB(d); - s->imap[0] = 0; - s->imap[1] = 0; - s->imap[2] = 0; - s->smap[0] = 0; - s->smap[1] = 0; - s->smap[2] = 0; + for (int i = 0; i < WINDOW_COUNT; i++) { + s->imap[i] = 0; + s->smap[i] = 0; + } s->selfid = 0; s->flags = 0; s->irq_mapping = s->irq_mapping_prop; @@ -453,7 +453,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp) * PCI memory space. The sizes vary from board to board; the base * offsets are guest controllable via the IMAP registers. */ - for (i = 0; i < 3; i++) { + for (i = 0; i < WINDOW_COUNT; i++) { memory_region_init_alias(&s->pci_mem_window[i], OBJECT(s), "pci-vbp-window", &s->pci_mem_space, 0, s->mem_win_size[i]); sysbus_init_mmio(sbd, &s->pci_mem_window[i]);
Use self-explicit WINDOW_COUNT definition instead of a magic value. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/pci-host/versatile.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)