[PATCH 0/2] Make PAMMemoryRegion less Intel-specific

[PATCH 0/2] Make PAMMemoryRegion less Intel-specific

[Bernhard Beschow]

This series makes pam_update() more self-contained by removing some Intel
assumptions. As a result, the purpose of PAMMemoryRegion becomes clearer and, as
a side effect, makes it reusable for northbridges other than Intel, e.g. VIA.

Testing done:
* `make check`
* `make check-avocado`
* This series is sent from a VM implementing this series.

Bernhard Beschow (2):
  hw/pci-host/pam: Free PAMMemoryRegion from Intel-specific bit handling
  hw/pci-host/pam: Remove northbridge-specific PAM_REGIONS_COUNT

 include/hw/pci-host/i440fx.h |  4 +++-
 include/hw/pci-host/pam.h    |  9 +++------
 include/hw/pci-host/q35.h    |  4 +++-
 hw/pci-host/i440fx.c         |  7 +++++--
 hw/pci-host/pam.c            | 14 +++++++-------
 hw/pci-host/q35.c            |  7 ++++---
 6 files changed, 25 insertions(+), 20 deletions(-)

-- 
2.44.0

[PATCH 1/2] hw/pci-host/pam: Free PAMMemoryRegion from Intel-specific bit handling

[Bernhard Beschow]

The PAM bit extraction is currently spread across pam.c and the northbridge
device models, making the extraction logic harder to comprehend. Also note how
pam_update() deals with PAM_REGIONS_COUNT, even though it handles exactly one
region. Fix this (at the cost of minor code duplication) by moving the bit
extraction into the northbridge device models. As a side effect, pam_update()
becomes less Intel-specific which would allow it to be reused e.g. in VIA
northbridges.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/pci-host/pam.h |  7 +++----
 hw/pci-host/i440fx.c      |  7 +++++--
 hw/pci-host/pam.c         | 14 +++++++-------
 hw/pci-host/q35.c         |  5 +++--
 4 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
index 005916f826..b9b33aecc8 100644
--- a/include/hw/pci-host/pam.h
+++ b/include/hw/pci-host/pam.h
@@ -70,7 +70,6 @@
 /* PAM registers: log nibble and high nibble*/
 #define PAM_ATTR_WE     ((uint8_t)2)
 #define PAM_ATTR_RE     ((uint8_t)1)
-#define PAM_ATTR_MASK   ((uint8_t)3)
 
 /* SMRAM register */
 #define SMRAM_D_OPEN           ((uint8_t)(1 << 6))
@@ -83,13 +82,13 @@
 #define PAM_REGIONS_COUNT       13
 
 typedef struct PAMMemoryRegion {
-    MemoryRegion alias[4];  /* index = PAM value */
-    unsigned current;
+    MemoryRegion alias[4];  /* index = mode value */
+    uint8_t mode;
 } PAMMemoryRegion;
 
 void init_pam(PAMMemoryRegion *mem, Object *owner, MemoryRegion *ram,
               MemoryRegion *system, MemoryRegion *pci,
               uint32_t start, uint32_t size);
-void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
+void pam_update(PAMMemoryRegion *mem, uint8_t mode);
 
 #endif /* QEMU_PAM_H */
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 4f0a0438d7..cddd506ab0 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -64,6 +64,8 @@ struct I440FXState {
 #define I440FX_PAM_SIZE 7
 #define I440FX_SMRAM    0x72
 
+#define I440FX_PAM_ATTR_MASK ((uint8_t)3)
+
 /* Keep it 2G to comply with older win32 guests */
 #define I440FX_PCI_HOST_HOLE64_SIZE_DEFAULT (1ULL << 31)
 
@@ -88,8 +90,9 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
 
     memory_region_transaction_begin();
     for (i = 0; i < ARRAY_SIZE(d->pam_regions); i++) {
-        pam_update(&d->pam_regions[i], i,
-                   pd->config[I440FX_PAM + DIV_ROUND_UP(i, 2)]);
+        uint8_t reg = pd->config[I440FX_PAM + DIV_ROUND_UP(i, 2)];
+        pam_update(&d->pam_regions[i],
+                   (reg >> ((!(i & 1)) * 4)) & I440FX_PAM_ATTR_MASK);
     }
     memory_region_set_enabled(&d->smram_region,
                               !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
index 68e9884d27..29c0db097a 100644
--- a/hw/pci-host/pam.c
+++ b/hw/pci-host/pam.c
@@ -51,20 +51,20 @@ void init_pam(PAMMemoryRegion *mem, Object *owner, 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);
     }
     memory_region_transaction_commit();
-    mem->current = 0;
+    mem->mode = 0;
 }
 
-void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
+void pam_update(PAMMemoryRegion *pam, uint8_t mode)
 {
-    assert(0 <= idx && idx < PAM_REGIONS_COUNT);
+    g_assert(mode < ARRAY_SIZE(pam->alias));
 
-    memory_region_set_enabled(&pam->alias[pam->current], false);
-    pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
-    memory_region_set_enabled(&pam->alias[pam->current], true);
+    memory_region_set_enabled(&pam->alias[pam->mode], false);
+    pam->mode = mode;
+    memory_region_set_enabled(&pam->alias[pam->mode], true);
 }
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 0d7d4e3f08..947d9aa9c4 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -330,8 +330,9 @@ static void mch_update_pam(MCHPCIState *mch)
 
     memory_region_transaction_begin();
     for (i = 0; i < 13; i++) {
-        pam_update(&mch->pam_regions[i], i,
-                   pd->config[MCH_HOST_BRIDGE_PAM0 + DIV_ROUND_UP(i, 2)]);
+        uint8_t reg = pd->config[MCH_HOST_BRIDGE_PAM0 + DIV_ROUND_UP(i, 2)];
+        pam_update(&mch->pam_regions[i],
+                   (reg >> ((!(i & 1)) * 4)) & MCH_HOST_BRIDGE_PAM_MASK);
     }
     memory_region_transaction_commit();
 }
-- 
2.44.0

[PATCH 2/2] hw/pci-host/pam: Remove northbridge-specific PAM_REGIONS_COUNT

[Bernhard Beschow]

PAM_REGIONS_COUNT being 13 seems to be Intel-specific. There are VIA 82cXX
northbridges having only 10, for example. Communicate this by having
northbridge-specific constants.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>

--

Do we need the constants or can we omit them, given they're just used once?
---
 include/hw/pci-host/i440fx.h | 4 +++-
 include/hw/pci-host/pam.h    | 2 --
 include/hw/pci-host/q35.h    | 4 +++-
 hw/pci-host/q35.c            | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index c988f70890..9e9b252660 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -22,12 +22,14 @@
 
 OBJECT_DECLARE_SIMPLE_TYPE(PCII440FXState, I440FX_PCI_DEVICE)
 
+#define I440FX_HOST_PAM_REGIONS_COUNT 13
+
 struct PCII440FXState {
     /*< private >*/
     PCIDevice parent_obj;
     /*< public >*/
 
-    PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
+    PAMMemoryRegion pam_regions[I440FX_HOST_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 b9b33aecc8..25dbe6feaf 100644
--- a/include/hw/pci-host/pam.h
+++ b/include/hw/pci-host/pam.h
@@ -79,8 +79,6 @@
 #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 = mode value */
     uint8_t mode;
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index bafcbe6752..618ecf05f4 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -35,6 +35,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(Q35PCIHost, Q35_HOST_DEVICE)
 #define TYPE_MCH_PCI_DEVICE "mch"
 OBJECT_DECLARE_SIMPLE_TYPE(MCHPCIState, MCH_PCI_DEVICE)
 
+#define MCH_HOST_PAM_REGIONS_COUNT 13
+
 struct MCHPCIState {
     /*< private >*/
     PCIDevice parent_obj;
@@ -44,7 +46,7 @@ struct MCHPCIState {
     MemoryRegion *pci_address_space;
     MemoryRegion *system_memory;
     MemoryRegion *address_space_io;
-    PAMMemoryRegion pam_regions[PAM_REGIONS_COUNT];
+    PAMMemoryRegion pam_regions[MCH_HOST_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/q35.c b/hw/pci-host/q35.c
index 947d9aa9c4..6782bf4216 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -329,7 +329,7 @@ static void mch_update_pam(MCHPCIState *mch)
     int i;
 
     memory_region_transaction_begin();
-    for (i = 0; i < 13; i++) {
+    for (i = 0; i < ARRAY_SIZE(mch->pam_regions); i++) {
         uint8_t reg = pd->config[MCH_HOST_BRIDGE_PAM0 + DIV_ROUND_UP(i, 2)];
         pam_update(&mch->pam_regions[i],
                    (reg >> ((!(i & 1)) * 4)) & MCH_HOST_BRIDGE_PAM_MASK);
-- 
2.44.0

Re: [PATCH 2/2] hw/pci-host/pam: Remove northbridge-specific PAM_REGIONS_COUNT

[Philippe Mathieu-Daudé]

On 9/3/24 14:40, Bernhard Beschow wrote:
> PAM_REGIONS_COUNT being 13 seems to be Intel-specific. There are VIA 82cXX
> northbridges having only 10, for example. Communicate this by having
> northbridge-specific constants.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> 
> --
> 
> Do we need the constants or can we omit them, given they're just used once?

We don't need but prefer, since the definition is self-explicit
and we don't have to lookup datasheet for magic value.

> ---
>   include/hw/pci-host/i440fx.h | 4 +++-
>   include/hw/pci-host/pam.h    | 2 --
>   include/hw/pci-host/q35.h    | 4 +++-
>   hw/pci-host/q35.c            | 2 +-
>   4 files changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] hw/pci-host/pam: Free PAMMemoryRegion from Intel-specific bit handling

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 9/3/24 14:40, Bernhard Beschow wrote:
> The PAM bit extraction is currently spread across pam.c and the northbridge
> device models, making the extraction logic harder to comprehend. Also note how
> pam_update() deals with PAM_REGIONS_COUNT, even though it handles exactly one
> region. Fix this (at the cost of minor code duplication) by moving the bit
> extraction into the northbridge device models. As a side effect, pam_update()
> becomes less Intel-specific which would allow it to be reused e.g. in VIA
> northbridges.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/pci-host/pam.h |  7 +++----
>   hw/pci-host/i440fx.c      |  7 +++++--
>   hw/pci-host/pam.c         | 14 +++++++-------
>   hw/pci-host/q35.c         |  5 +++--
>   4 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
> index 005916f826..b9b33aecc8 100644
> --- a/include/hw/pci-host/pam.h
> +++ b/include/hw/pci-host/pam.h
> @@ -70,7 +70,6 @@
>   /* PAM registers: log nibble and high nibble*/
>   #define PAM_ATTR_WE     ((uint8_t)2)
>   #define PAM_ATTR_RE     ((uint8_t)1)
> -#define PAM_ATTR_MASK   ((uint8_t)3)

Why not use PAM_ATTR_foo instead of MCH_HOST_BRIDGE_PAM_foo?

>   /* SMRAM register */
>   #define SMRAM_D_OPEN           ((uint8_t)(1 << 6))
> @@ -83,13 +82,13 @@
>   #define PAM_REGIONS_COUNT       13
>   
>   typedef struct PAMMemoryRegion {
> -    MemoryRegion alias[4];  /* index = PAM value */
> -    unsigned current;
> +    MemoryRegion alias[4];  /* index = mode value */
> +    uint8_t mode;
>   } PAMMemoryRegion;
>   
>   void init_pam(PAMMemoryRegion *mem, Object *owner, MemoryRegion *ram,
>                 MemoryRegion *system, MemoryRegion *pci,
>                 uint32_t start, uint32_t size);
> -void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
> +void pam_update(PAMMemoryRegion *mem, uint8_t mode);
>   
>   #endif /* QEMU_PAM_H */
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 4f0a0438d7..cddd506ab0 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -64,6 +64,8 @@ struct I440FXState {
>   #define I440FX_PAM_SIZE 7
>   #define I440FX_SMRAM    0x72
>   
> +#define I440FX_PAM_ATTR_MASK ((uint8_t)3)

or (PAM_ATTR_RE|PAM_ATTR_WE)?

It is odd to have I440FX_PAM_ATTR_MASK disconnected
from the values it masks.

> -void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
> +void pam_update(PAMMemoryRegion *pam, uint8_t mode)
>   {
> -    assert(0 <= idx && idx < PAM_REGIONS_COUNT);
> +    g_assert(mode < ARRAY_SIZE(pam->alias));
>   
> -    memory_region_set_enabled(&pam->alias[pam->current], false);
> -    pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;

Can we pass the mask by argument instead?

> -    memory_region_set_enabled(&pam->alias[pam->current], true);
> +    memory_region_set_enabled(&pam->alias[pam->mode], false);
> +    pam->mode = mode;
> +    memory_region_set_enabled(&pam->alias[pam->mode], true);
>   }

Are the VIA values different of the PAM_ATTR_foo ones?

I'm not sure this is an helpful change, I'd rather
remove the MCH_HOST_BRIDGE_PAM_foo definitions and
use the PAM generic ones.

Regards,

Phil.

Re: [PATCH 1/2] hw/pci-host/pam: Free PAMMemoryRegion from Intel-specific bit handling

[Bernhard Beschow]

Am 9. März 2024 16:29:23 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Hi Bernhard,
>
>On 9/3/24 14:40, Bernhard Beschow wrote:
>> The PAM bit extraction is currently spread across pam.c and the northbridge
>> device models, making the extraction logic harder to comprehend. Also note how
>> pam_update() deals with PAM_REGIONS_COUNT, even though it handles exactly one
>> region. Fix this (at the cost of minor code duplication) by moving the bit
>> extraction into the northbridge device models. As a side effect, pam_update()
>> becomes less Intel-specific which would allow it to be reused e.g. in VIA
>> northbridges.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/pci-host/pam.h |  7 +++----
>>   hw/pci-host/i440fx.c      |  7 +++++--
>>   hw/pci-host/pam.c         | 14 +++++++-------
>>   hw/pci-host/q35.c         |  5 +++--
>>   4 files changed, 18 insertions(+), 15 deletions(-)
>> 
>> diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
>> index 005916f826..b9b33aecc8 100644
>> --- a/include/hw/pci-host/pam.h
>> +++ b/include/hw/pci-host/pam.h
>> @@ -70,7 +70,6 @@
>>   /* PAM registers: log nibble and high nibble*/
>>   #define PAM_ATTR_WE     ((uint8_t)2)
>>   #define PAM_ATTR_RE     ((uint8_t)1)
>> -#define PAM_ATTR_MASK   ((uint8_t)3)
>
>Why not use PAM_ATTR_foo instead of MCH_HOST_BRIDGE_PAM_foo?

Could be used indeed. See also below.

>
>>   /* SMRAM register */
>>   #define SMRAM_D_OPEN           ((uint8_t)(1 << 6))
>> @@ -83,13 +82,13 @@
>>   #define PAM_REGIONS_COUNT       13
>>     typedef struct PAMMemoryRegion {
>> -    MemoryRegion alias[4];  /* index = PAM value */
>> -    unsigned current;
>> +    MemoryRegion alias[4];  /* index = mode value */
>> +    uint8_t mode;
>>   } PAMMemoryRegion;
>>     void init_pam(PAMMemoryRegion *mem, Object *owner, MemoryRegion *ram,
>>                 MemoryRegion *system, MemoryRegion *pci,
>>                 uint32_t start, uint32_t size);
>> -void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
>> +void pam_update(PAMMemoryRegion *mem, uint8_t mode);
>>     #endif /* QEMU_PAM_H */
>> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>> index 4f0a0438d7..cddd506ab0 100644
>> --- a/hw/pci-host/i440fx.c
>> +++ b/hw/pci-host/i440fx.c
>> @@ -64,6 +64,8 @@ struct I440FXState {
>>   #define I440FX_PAM_SIZE 7
>>   #define I440FX_SMRAM    0x72
>>   +#define I440FX_PAM_ATTR_MASK ((uint8_t)3)
>
>or (PAM_ATTR_RE|PAM_ATTR_WE)?
>
>It is odd to have I440FX_PAM_ATTR_MASK disconnected
>from the values it masks.

PAM_ATTR_RE and PAM_ATTR_WE are swapped in case of VIA. I didn't bother about these constants since both are currently not used at all. Shall I remove them in case of a respin?

>
>> -void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
>> +void pam_update(PAMMemoryRegion *pam, uint8_t mode)
>>   {
>> -    assert(0 <= idx && idx < PAM_REGIONS_COUNT);
>> +    g_assert(mode < ARRAY_SIZE(pam->alias));
>>   -    memory_region_set_enabled(&pam->alias[pam->current], false);
>> -    pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
>
>Can we pass the mask by argument instead?

For VIA, each PAM region is defined by just two bits (rather than four as for Intel). So a byte contains attributes for four regions instead of two. Therefore, passing a mask alone doesn't help, one needed to pass a shift value as well. Furthermore, since PAM_ATTR_RE and PAM_ATTR_WE are swapped, it seems cleaner to just pass the final mode value.

Do we consider avoiding the redundancies more worthwhile than having the bit extraction logic together? If so, I'm fine with dropping the series until a VIA northbridge gets accepted. Perhaps what's missing is a bit extraction API which spans multiple bytes. Please let me know.

>
>> -    memory_region_set_enabled(&pam->alias[pam->current], true);
>> +    memory_region_set_enabled(&pam->alias[pam->mode], false);
>> +    pam->mode = mode;
>> +    memory_region_set_enabled(&pam->alias[pam->mode], true);
>>   }
>
>Are the VIA values different of the PAM_ATTR_foo ones?

They are different except for PAM_ATTR_MASK.

>
>I'm not sure this is an helpful change, I'd rather
>remove the MCH_HOST_BRIDGE_PAM_foo definitions and
>use the PAM generic ones.

PAM_ATTR_MASK could indeed be reused for VIA. I'd respin if this series made sense in its own right.

Best regsrds,
Bernhard

>
>Regards,
>
>Phil.