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

[PATCH 0/4] 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.

Expected to be merged via qemu-trivial@.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  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 WINDOW_COUNT definition to replace magic
    '3'
  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 | 34 +++++++++++++++++-----------------
 tests/qtest/rtc-test.c  |  2 +-
 5 files changed, 22 insertions(+), 22 deletions(-)

-- 
2.26.2

[PATCH 1/4] 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 2/4] 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 3/4] hw/pci-host/versatile: Add WINDOW_COUNT definition to replace magic '3'

[Philippe Mathieu-Daudé]

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];
     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]);
-- 
2.26.2

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

[Peter Maydell]

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

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

[Philippe Mathieu-Daudé]

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
> 

[PATCH 4/4] 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.

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 4/4] tests/qtest: Replace magic value by NANOSECONDS_PER_SECOND definition

[Thomas Huth]

On 11/10/2020 21.49, Philippe Mathieu-Daudé wrote:
> Use self-explicit NANOSECONDS_PER_SECOND definition instead
> of a magic value.
> 
> 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));
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>