Re: [PATCH] hw/pci-host/pam: Replace magic number by PAM_REGIONS_COUNT definition

2020-12-13 Thread Laurent Vivier 
Le 02/12/2020 à 14:20, Philippe Mathieu-Daudé a écrit :
> While this change helps triskaidekaphobic developers, it
> is a good practice to avoid magic values and using constant
> definitions instead.
> 
> Introduce the PAM_REGIONS_COUNT and use it. No logical change.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/pci-host/i440fx.h | 2 +-
>  include/hw/pci-host/pam.h| 2 ++
>  include/hw/pci-host/q35.h| 2 +-
>  hw/pci-host/pam.c| 2 +-
>  hw/pci-host/q35.c| 2 +-
>  5 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
> index 6c16eaf876d..24fd53942ca 100644
> --- a/include/hw/pci-host/i440fx.h
> +++ b/include/hw/pci-host/i440fx.h
> @@ -29,7 +29,7 @@ struct PCII440FXState {
>  MemoryRegion *system_memory;
>  MemoryRegion *pci_address_space;
>  MemoryRegion *ram_memory;
> -PAMMemoryRegion pam_regions[13];
> +PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
>  MemoryRegion smram_region;
>  MemoryRegion smram, low_smram;
>  };
> diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
> index fec5cd35d60..c1fd06ba2ae 100644
> --- a/include/hw/pci-host/pam.h
> +++ b/include/hw/pci-host/pam.h
> @@ -80,6 +80,8 @@
>  #define SMRAM_C_BASE_SEG_MASK  ((uint8_t)0x7)
>  #define SMRAM_C_BASE_SEG   ((uint8_t)0x2)  /* hardwired to b010 */
>  
> +#define PAM_REGIONS_COUNT   13
> +
>  typedef struct PAMMemoryRegion {
>  MemoryRegion alias[4];  /* index = PAM value */
>  unsigned current;
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index bbb95817656..ab989698ef8 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -44,7 +44,7 @@ struct MCHPCIState {
>  MemoryRegion *pci_address_space;
>  MemoryRegion *system_memory;
>  MemoryRegion *address_space_io;
> -PAMMemoryRegion pam_regions[13];
> +PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
>  MemoryRegion smram_region, open_high_smram;
>  MemoryRegion smram, low_smram, high_smram;
>  MemoryRegion tseg_blackhole, tseg_window;
> diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
> index a4962057833..454dd120db9 100644
> --- a/hw/pci-host/pam.c
> +++ b/hw/pci-host/pam.c
> @@ -62,7 +62,7 @@ void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
>  
>  void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
>  {
> -assert(0 <= idx && idx <= 12);
> +assert(0 <= idx && idx < PAM_REGIONS_COUNT);
>  
>  memory_region_set_enabled(>alias[pam->current], false);
>  pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b67cb9c29f8..2eb729dff58 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -641,7 +641,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
>  init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>   mch->pci_address_space, >pam_regions[0],
>   PAM_BIOS_BASE, PAM_BIOS_SIZE);
> -for (i = 0; i < 12; ++i) {
> +for (i = 0; i < ARRAY_SIZE(mch->pam_regions) - 1; ++i) {
>  init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>   mch->pci_address_space, >pam_regions[i+1],
>   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] hw/pci-host/pam: Replace magic number by PAM_REGIONS_COUNT definition

2020-12-02 Thread Julia Suvorova 
On Wed, Dec 2, 2020 at 2:24 PM Philippe Mathieu-Daudé  wrote:
>
> While this change helps triskaidekaphobic developers, it
> is a good practice to avoid magic values and using constant
> definitions instead.
>
> Introduce the PAM_REGIONS_COUNT and use it. No logical change.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/pci-host/i440fx.h | 2 +-
>  include/hw/pci-host/pam.h| 2 ++
>  include/hw/pci-host/q35.h| 2 +-
>  hw/pci-host/pam.c| 2 +-
>  hw/pci-host/q35.c| 2 +-
>  5 files changed, 6 insertions(+), 4 deletions(-)

:D

Reviewed-by: Julia Suvorova

[PATCH] hw/pci-host/pam: Replace magic number by PAM_REGIONS_COUNT definition

2020-12-02 Thread Philippe Mathieu-Daudé 
While this change helps triskaidekaphobic developers, it
is a good practice to avoid magic values and using constant
definitions instead.

Introduce the PAM_REGIONS_COUNT and use it. No logical change.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/pci-host/i440fx.h | 2 +-
 include/hw/pci-host/pam.h| 2 ++
 include/hw/pci-host/q35.h| 2 +-
 hw/pci-host/pam.c| 2 +-
 hw/pci-host/q35.c| 2 +-
 5 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index 6c16eaf876d..24fd53942ca 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -29,7 +29,7 @@ struct PCII440FXState {
 MemoryRegion *system_memory;
 MemoryRegion *pci_address_space;
 MemoryRegion *ram_memory;
-PAMMemoryRegion pam_regions[13];
+PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
 MemoryRegion smram_region;
 MemoryRegion smram, low_smram;
 };
diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
index fec5cd35d60..c1fd06ba2ae 100644
--- a/include/hw/pci-host/pam.h
+++ b/include/hw/pci-host/pam.h
@@ -80,6 +80,8 @@
 #define SMRAM_C_BASE_SEG_MASK  ((uint8_t)0x7)
 #define SMRAM_C_BASE_SEG   ((uint8_t)0x2)  /* hardwired to b010 */
 
+#define PAM_REGIONS_COUNT   13
+
 typedef struct PAMMemoryRegion {
 MemoryRegion alias[4];  /* index = PAM value */
 unsigned current;
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index bbb95817656..ab989698ef8 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -44,7 +44,7 @@ struct MCHPCIState {
 MemoryRegion *pci_address_space;
 MemoryRegion *system_memory;
 MemoryRegion *address_space_io;
-PAMMemoryRegion pam_regions[13];
+PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
 MemoryRegion smram_region, open_high_smram;
 MemoryRegion smram, low_smram, high_smram;
 MemoryRegion tseg_blackhole, tseg_window;
diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
index a4962057833..454dd120db9 100644
--- a/hw/pci-host/pam.c
+++ b/hw/pci-host/pam.c
@@ -62,7 +62,7 @@ void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
 
 void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
 {
-assert(0 <= idx && idx <= 12);
+assert(0 <= idx && idx < PAM_REGIONS_COUNT);
 
 memory_region_set_enabled(>alias[pam->current], false);
 pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b67cb9c29f8..2eb729dff58 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -641,7 +641,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
 init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
  mch->pci_address_space, >pam_regions[0],
  PAM_BIOS_BASE, PAM_BIOS_SIZE);
-for (i = 0; i < 12; ++i) {
+for (i = 0; i < ARRAY_SIZE(mch->pam_regions) - 1; ++i) {
 init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
  mch->pci_address_space, >pam_regions[i+1],
  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
-- 
2.26.2