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.