[PATCH v2 0/5] hw: Replace some magic by definitions

[PATCH v2 0/5] hw: Replace some magic by definitions

[Philippe Mathieu-Daudé]

A bunch of trivial cleanups, replacing magic
values by definitions to make the code easier
to review.

Since v1:
- Addressed Peter review comment on versatile PCI controller
- Added Thomas R-b tag

Expected to be merged via qemu-trivial@.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw: Replace magic value by PCI_NUM_PINS definition
  hw/pci-host/pam: Use ARRAY_SIZE() instead of magic value
  hw/pci-host/versatile: Add the MEMORY_WINDOW_COUNT definition
  hw/pci-host/versatile: Add the PCI_BAR_COUNT definition
  tests/qtest: Replace magic value by NANOSECONDS_PER_SECOND definition

 hw/arm/virt.c           |  4 ++--
 hw/mips/gt64xxx_pci.c   |  2 +-
 hw/pci-host/pam.c       |  2 +-
 hw/pci-host/versatile.c | 46 ++++++++++++++++++++++-------------------
 tests/qtest/rtc-test.c  |  2 +-
 5 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.26.2

[PATCH v2 1/5] hw: Replace magic value by PCI_NUM_PINS definition

[Philippe Mathieu-Daudé]

Use self-explicit PCI_NUM_PINS definition instead of magic value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/virt.c           | 4 ++--
 hw/mips/gt64xxx_pci.c   | 2 +-
 hw/pci-host/versatile.c | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e465a988d68..ddad9621f79 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1117,11 +1117,11 @@ static void create_pcie_irq_map(const VirtMachineState *vms,
                                 int first_irq, const char *nodename)
 {
     int devfn, pin;
-    uint32_t full_irq_map[4 * 4 * 10] = { 0 };
+    uint32_t full_irq_map[4 * PCI_NUM_PINS * 10] = { 0 };
     uint32_t *irq_map = full_irq_map;
 
     for (devfn = 0; devfn <= 0x18; devfn += 0x8) {
-        for (pin = 0; pin < 4; pin++) {
+        for (pin = 0; pin < PCI_NUM_PINS; pin++) {
             int irq_type = GIC_FDT_IRQ_TYPE_SPI;
             int irq_nr = first_irq + ((pin + PCI_SLOT(devfn)) % PCI_NUM_PINS);
             int irq_level = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index e091bc4ed55..ff1a35755f6 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1018,7 +1018,7 @@ static void gt64120_pci_set_irq(void *opaque, int irq_num, int level)
     if (pic_irq < 16) {
         /* The pic level is the logical OR of all the PCI irqs mapped to it. */
         pic_level = 0;
-        for (i = 0; i < 4; i++) {
+        for (i = 0; i < PCI_NUM_PINS; i++) {
             if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
                 pic_level |= pci_irq_levels[i];
             }
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 3553277f941..b4951023f4e 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -75,7 +75,7 @@ enum {
 struct PCIVPBState {
     PCIHostState parent_obj;
 
-    qemu_irq irq[4];
+    qemu_irq irq[PCI_NUM_PINS];
     MemoryRegion controlregs;
     MemoryRegion mem_config;
     MemoryRegion mem_config2;
@@ -412,7 +412,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
 
     object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_VERSATILE_PCI_HOST);
 
-    for (i = 0; i < 4; i++) {
+    for (i = 0; i < PCI_NUM_PINS; i++) {
         sysbus_init_irq(sbd, &s->irq[i]);
     }
 
@@ -422,7 +422,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
         mapfn = pci_vpb_map_irq;
     }
 
-    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
+    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, PCI_NUM_PINS);
 
     /* Our memory regions are:
      * 0 : our control registers
-- 
2.26.2

[PATCH v2 2/5] hw/pci-host/pam: Use ARRAY_SIZE() instead of magic value

[Philippe Mathieu-Daudé]

Replace the magic '4' by ARRAY_SIZE(mem->alias) which is more explicit.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/pam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
index a4962057833..4712260025a 100644
--- a/hw/pci-host/pam.c
+++ b/hw/pci-host/pam.c
@@ -51,7 +51,7 @@ void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
                              start, size);
 
     memory_region_transaction_begin();
-    for (i = 0; i < 4; ++i) {
+    for (i = 0; i < ARRAY_SIZE(mem->alias); ++i) {
         memory_region_set_enabled(&mem->alias[i], false);
         memory_region_add_subregion_overlap(system_memory, start,
                                             &mem->alias[i], 1);
-- 
2.26.2

[PATCH v2 3/5] hw/pci-host/versatile: Add the MEMORY_WINDOW_COUNT definition

[Philippe Mathieu-Daudé]

Note from Peter Maydell:

 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

Use self-explicit MEMORY_WINDOW_COUNT definition instead of
a magic value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/versatile.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index b4951023f4e..61465bbbb65 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -72,6 +72,8 @@ enum {
     PCI_VPB_IRQMAP_FORCE_OK,
 };
 
+#define MEMORY_WINDOW_COUNT 3
+
 struct PCIVPBState {
     PCIHostState parent_obj;
 
@@ -86,17 +88,17 @@ 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[MEMORY_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[MEMORY_WINDOW_COUNT];
     uint8_t irq_mapping_prop;
 
     /* Variable state: */
-    uint32_t imap[3];
+    uint32_t imap[MEMORY_WINDOW_COUNT];
     uint32_t smap[3];
     uint32_t selfid;
     uint32_t flags;
@@ -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 < MEMORY_WINDOW_COUNT; i++) {
         pci_vpb_update_window(s, i);
     }
 }
@@ -148,7 +150,7 @@ 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(imap, PCIVPBState, MEMORY_WINDOW_COUNT),
         VMSTATE_UINT32_ARRAY(smap, PCIVPBState, 3),
         VMSTATE_UINT32(selfid, PCIVPBState),
         VMSTATE_UINT32(flags, PCIVPBState),
@@ -370,10 +372,11 @@ static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
 static void pci_vpb_reset(DeviceState *d)
 {
     PCIVPBState *s = PCI_VPB(d);
+    int i;
 
-    s->imap[0] = 0;
-    s->imap[1] = 0;
-    s->imap[2] = 0;
+    for (i = 0; i < MEMORY_WINDOW_COUNT; i++) {
+        s->imap[i] = 0;
+    }
     s->smap[0] = 0;
     s->smap[1] = 0;
     s->smap[2] = 0;
@@ -453,7 +456,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 < MEMORY_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]);
-- 
2.26.2

[PATCH v2 4/5] hw/pci-host/versatile: Add the PCI_BAR_COUNT definition

[Philippe Mathieu-Daudé]

Note from Peter Maydell:

 The versatile/realview PCI controller has:
   * 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

Use self-explicit PCI_BAR_COUNT definition instead of a magic
value, and rename some misnamed variables indexing the smap[]
array from 'win' to 'bar'.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/versatile.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 61465bbbb65..f5846b803c0 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -73,6 +73,7 @@ enum {
 };
 
 #define MEMORY_WINDOW_COUNT 3
+#define PCI_BAR_COUNT 3
 
 struct PCIVPBState {
     PCIHostState parent_obj;
@@ -99,7 +100,7 @@ struct PCIVPBState {
 
     /* Variable state: */
     uint32_t imap[MEMORY_WINDOW_COUNT];
-    uint32_t smap[3];
+    uint32_t smap[PCI_BAR_COUNT];
     uint32_t selfid;
     uint32_t flags;
     uint8_t irq_mapping;
@@ -151,7 +152,7 @@ static const VMStateDescription pci_vpb_vmstate = {
     .post_load = pci_vpb_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(imap, PCIVPBState, MEMORY_WINDOW_COUNT),
-        VMSTATE_UINT32_ARRAY(smap, PCIVPBState, 3),
+        VMSTATE_UINT32_ARRAY(smap, PCIVPBState, PCI_BAR_COUNT),
         VMSTATE_UINT32(selfid, PCIVPBState),
         VMSTATE_UINT32(flags, PCIVPBState),
         VMSTATE_UINT8(irq_mapping, PCIVPBState),
@@ -203,8 +204,8 @@ static void pci_vpb_reg_write(void *opaque, hwaddr addr,
     case PCI_SMAP1:
     case PCI_SMAP2:
     {
-        int win = (addr - PCI_SMAP0) >> 2;
-        s->smap[win] = val;
+        int bar = (addr - PCI_SMAP0) >> 2;
+        s->smap[bar] = val;
         break;
     }
     default:
@@ -235,8 +236,8 @@ static uint64_t pci_vpb_reg_read(void *opaque, hwaddr addr,
     case PCI_SMAP1:
     case PCI_SMAP2:
     {
-        int win = (addr - PCI_SMAP0) >> 2;
-        return s->smap[win];
+        int bar = (addr - PCI_SMAP0) >> 2;
+        return s->smap[bar];
     }
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -377,9 +378,9 @@ static void pci_vpb_reset(DeviceState *d)
     for (i = 0; i < MEMORY_WINDOW_COUNT; i++) {
         s->imap[i] = 0;
     }
-    s->smap[0] = 0;
-    s->smap[1] = 0;
-    s->smap[2] = 0;
+    for (i = 0; i < PCI_BAR_COUNT; i++) {
+        s->smap[i] = 0;
+    }
     s->selfid = 0;
     s->flags = 0;
     s->irq_mapping = s->irq_mapping_prop;
-- 
2.26.2

[PATCH v2 5/5] tests/qtest: Replace magic value by NANOSECONDS_PER_SECOND definition

[Philippe Mathieu-Daudé]

Use self-explicit NANOSECONDS_PER_SECOND definition instead
of a magic value.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/rtc-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/rtc-test.c b/tests/qtest/rtc-test.c
index c7af34f6b1b..402ce2c6090 100644
--- a/tests/qtest/rtc-test.c
+++ b/tests/qtest/rtc-test.c
@@ -292,7 +292,7 @@ static void alarm_time(void)
             break;
         }
 
-        clock_step(1000000000);
+        clock_step(NANOSECONDS_PER_SECOND);
     }
 
     g_assert(get_irq(RTC_ISA_IRQ));
-- 
2.26.2

Re: [PATCH v2 3/5] hw/pci-host/versatile: Add the MEMORY_WINDOW_COUNT definition

[Peter Maydell]

On Mon, 12 Oct 2020 at 14:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Note from Peter Maydell:
>
>  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
>
> Use self-explicit MEMORY_WINDOW_COUNT definition instead of
> a magic value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 4/5] hw/pci-host/versatile: Add the PCI_BAR_COUNT definition

[Peter Maydell]

On Mon, 12 Oct 2020 at 14:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Note from Peter Maydell:
>
>  The versatile/realview PCI controller has:
>    * 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
>
> Use self-explicit PCI_BAR_COUNT definition instead of a magic
> value, and rename some misnamed variables indexing the smap[]
> array from 'win' to 'bar'.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 0/5] hw: Replace some magic by definitions

[Richard Henderson]

On 10/12/20 6:20 AM, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (5):
>   hw: Replace magic value by PCI_NUM_PINS definition
>   hw/pci-host/pam: Use ARRAY_SIZE() instead of magic value
>   hw/pci-host/versatile: Add the MEMORY_WINDOW_COUNT definition
>   hw/pci-host/versatile: Add the PCI_BAR_COUNT definition
>   tests/qtest: Replace magic value by NANOSECONDS_PER_SECOND definition

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~