Re: [PATCH 3/4] hw/pci-host/versatile: Add WINDOW_COUNT definition to replace magic '3'

[Peter Maydell <peter.maydell@linaro.org>]

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