| Message ID | 20240309134056.1605-2-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make PAMMemoryRegion less Intel-specific | expand |
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.
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.
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(); }
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(-)