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.