split PAM SMRAM functions in piix.c
create mch_init_pam in q35.c
Signed-off-by: Anthony Xu <anthony.xu@intel.com>
---
hw/pci-host/piix.c | 58 ++++++++++++++++++++++++++++++++++++++----------------
hw/pci-host/q35.c | 23 +++++++++++++---------
2 files changed, 55 insertions(+), 26 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index f9218aa..ff4e8b5 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -138,16 +138,11 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
return (pci_intx + slot_addend) & 3;
}
-static void i440fx_update_memory_mappings(PCII440FXState *d)
+static void i440fx_update_smram(PCII440FXState *d)
{
- int i;
PCIDevice *pd = PCI_DEVICE(d);
memory_region_transaction_begin();
- for (i = 0; i < 13; i++) {
- pam_update(&d->pam_regions[i], i,
- pd->config[I440FX_PAM + ((i + 1) / 2)]);
- }
memory_region_set_enabled(&d->smram_region,
!(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
memory_region_set_enabled(&d->smram,
@@ -155,6 +150,39 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
memory_region_transaction_commit();
}
+static void i440fx_update_pam(PCII440FXState *d)
+{
+ int i;
+ PCIDevice *pd = PCI_DEVICE(d);
+ memory_region_transaction_begin();
+ pam_update(&d->pam_regions[0], 0,
+ pd->config[I440FX_PAM]);
+ for (i = 1; i < 13; i++) {
+ pam_update(&d->pam_regions[i], i,
+ pd->config[I440FX_PAM + ((i + 1) / 2)]);
+ }
+ memory_region_transaction_commit();
+}
+
+static void i440fx_update_memory_mappings(PCII440FXState *d)
+{
+ i440fx_update_pam(d);
+ i440fx_update_smram(d);
+}
+
+
+static void i440fx_init_pam(PCII440FXState *d)
+{
+ int i;
+ init_pam(DEVICE(d), d->ram_memory, d->system_memory,
+ d->pci_address_space, &d->pam_regions[0],
+ PAM_BIOS_BASE, PAM_BIOS_SIZE);
+ for (i = 0; i < 12; ++i) {
+ init_pam(DEVICE(d), d->ram_memory, d->system_memory,
+ d->pci_address_space, &d->pam_regions[i + 1],
+ PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+ }
+}
static void i440fx_write_config(PCIDevice *dev,
uint32_t address, uint32_t val, int len)
@@ -163,9 +191,12 @@ static void i440fx_write_config(PCIDevice *dev,
/* XXX: implement SMRAM.D_LOCK */
pci_default_write_config(dev, address, val, len);
- if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE) ||
- range_covers_byte(address, len, I440FX_SMRAM)) {
- i440fx_update_memory_mappings(d);
+ if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE)) {
+ i440fx_update_pam(d);
+ }
+
+ if (range_covers_byte(address, len, I440FX_SMRAM)) {
+ i440fx_update_smram(d);
}
}
@@ -335,7 +366,6 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
PCIHostState *s;
PIIX3State *piix3;
PCII440FXState *f;
- unsigned i;
I440FXState *i440fx;
dev = qdev_create(NULL, host_type);
@@ -378,13 +408,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
object_property_add_const_link(qdev_get_machine(), "smram",
OBJECT(&f->smram), &error_abort);
- init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
- &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
- for (i = 0; i < 12; ++i) {
- init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
- &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
- PAM_EXPAN_SIZE);
- }
+ i440fx_init_pam(f);
/* Xen supports additional interrupt routes from the PCI devices to
* the IOAPIC: the four pins of each PCI device on the bus are also
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 344f77b..8866357 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -460,9 +460,21 @@ static void mch_reset(DeviceState *qdev)
mch_update(mch);
}
-static void mch_realize(PCIDevice *d, Error **errp)
+static void mch_init_pam(MCHPCIState *mch)
{
int i;
+ init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
+ mch->pci_address_space, &mch->pam_regions[0],
+ PAM_BIOS_BASE, PAM_BIOS_SIZE);
+ for (i = 0; i < 12; ++i) {
+ init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
+ mch->pci_address_space, &mch->pam_regions[i + 1],
+ PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
+ }
+}
+
+static void mch_realize(PCIDevice *d, Error **errp)
+{
MCHPCIState *mch = MCH_PCI_DEVICE(d);
/* setup pci memory mapping */
@@ -510,14 +522,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
object_property_add_const_link(qdev_get_machine(), "smram",
OBJECT(&mch->smram), &error_abort);
- init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
- mch->pci_address_space, &mch->pam_regions[0],
- PAM_BIOS_BASE, PAM_BIOS_SIZE);
- for (i = 0; i < 12; ++i) {
- init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
- mch->pci_address_space, &mch->pam_regions[i+1],
- PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
- }
+ mch_init_pam(mch);
}
uint64_t mch_mcfg_base(void)
--
1.8.3.1
On 08/04/2017 08:45, Anthony Xu wrote:
> split PAM SMRAM functions in piix.c
> create mch_init_pam in q35.c
Could you further move
MemoryRegion *ram_memory;
MemoryRegion *system_memory;
MemoryRegion *pci_address_space;
PAMMemoryRegion pam_regions[13];
from the northbridge devices up to PCMachineState, and move the common
code for PIIX and Q35 to hw/pci-host/pam.c?
It looks like you can define a better API than what pam.c currently
provides:
void pc_init_pam(PCMachineState *pc_machine, DeviceState *d);
void pc_update_pam(PCMachineState *pc_machine, uint8_t *pam_config);
or even remove "DeviceState *d" because the owner of the memory regions
can be the PCMachineState.
Thanks,
Paolo
> Signed-off-by: Anthony Xu <anthony.xu@intel.com>
> ---
> hw/pci-host/piix.c | 58 ++++++++++++++++++++++++++++++++++++++----------------
> hw/pci-host/q35.c | 23 +++++++++++++---------
> 2 files changed, 55 insertions(+), 26 deletions(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9218aa..ff4e8b5 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -138,16 +138,11 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
> return (pci_intx + slot_addend) & 3;
> }
>
> -static void i440fx_update_memory_mappings(PCII440FXState *d)
> +static void i440fx_update_smram(PCII440FXState *d)
> {
> - int i;
> PCIDevice *pd = PCI_DEVICE(d);
>
> memory_region_transaction_begin();
> - for (i = 0; i < 13; i++) {
> - pam_update(&d->pam_regions[i], i,
> - pd->config[I440FX_PAM + ((i + 1) / 2)]);
> - }
> memory_region_set_enabled(&d->smram_region,
> !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
> memory_region_set_enabled(&d->smram,
> @@ -155,6 +150,39 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
> memory_region_transaction_commit();
> }
>
> +static void i440fx_update_pam(PCII440FXState *d)
> +{
> + int i;
> + PCIDevice *pd = PCI_DEVICE(d);
> + memory_region_transaction_begin();
> + pam_update(&d->pam_regions[0], 0,
> + pd->config[I440FX_PAM]);
> + for (i = 1; i < 13; i++) {
> + pam_update(&d->pam_regions[i], i,
> + pd->config[I440FX_PAM + ((i + 1) / 2)]);
> + }
> + memory_region_transaction_commit();
> +}
> +
> +static void i440fx_update_memory_mappings(PCII440FXState *d)
> +{
> + i440fx_update_pam(d);
> + i440fx_update_smram(d);
> +}
> +
> +
> +static void i440fx_init_pam(PCII440FXState *d)
> +{
> + int i;
> + init_pam(DEVICE(d), d->ram_memory, d->system_memory,
> + d->pci_address_space, &d->pam_regions[0],
> + PAM_BIOS_BASE, PAM_BIOS_SIZE);
> + for (i = 0; i < 12; ++i) {
> + init_pam(DEVICE(d), d->ram_memory, d->system_memory,
> + d->pci_address_space, &d->pam_regions[i + 1],
> + PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> + }
> +}
>
> static void i440fx_write_config(PCIDevice *dev,
> uint32_t address, uint32_t val, int len)
> @@ -163,9 +191,12 @@ static void i440fx_write_config(PCIDevice *dev,
>
> /* XXX: implement SMRAM.D_LOCK */
> pci_default_write_config(dev, address, val, len);
> - if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE) ||
> - range_covers_byte(address, len, I440FX_SMRAM)) {
> - i440fx_update_memory_mappings(d);
> + if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE)) {
> + i440fx_update_pam(d);
> + }
> +
> + if (range_covers_byte(address, len, I440FX_SMRAM)) {
> + i440fx_update_smram(d);
> }
> }
>
> @@ -335,7 +366,6 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> PCIHostState *s;
> PIIX3State *piix3;
> PCII440FXState *f;
> - unsigned i;
> I440FXState *i440fx;
>
> dev = qdev_create(NULL, host_type);
> @@ -378,13 +408,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> object_property_add_const_link(qdev_get_machine(), "smram",
> OBJECT(&f->smram), &error_abort);
>
> - init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
> - &f->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
> - for (i = 0; i < 12; ++i) {
> - init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
> - &f->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE,
> - PAM_EXPAN_SIZE);
> - }
> + i440fx_init_pam(f);
>
> /* Xen supports additional interrupt routes from the PCI devices to
> * the IOAPIC: the four pins of each PCI device on the bus are also
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 344f77b..8866357 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -460,9 +460,21 @@ static void mch_reset(DeviceState *qdev)
> mch_update(mch);
> }
>
> -static void mch_realize(PCIDevice *d, Error **errp)
> +static void mch_init_pam(MCHPCIState *mch)
> {
> int i;
> + init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> + mch->pci_address_space, &mch->pam_regions[0],
> + PAM_BIOS_BASE, PAM_BIOS_SIZE);
> + for (i = 0; i < 12; ++i) {
> + init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> + mch->pci_address_space, &mch->pam_regions[i + 1],
> + PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> + }
> +}
> +
> +static void mch_realize(PCIDevice *d, Error **errp)
> +{
> MCHPCIState *mch = MCH_PCI_DEVICE(d);
>
> /* setup pci memory mapping */
> @@ -510,14 +522,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
> object_property_add_const_link(qdev_get_machine(), "smram",
> OBJECT(&mch->smram), &error_abort);
>
> - init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> - mch->pci_address_space, &mch->pam_regions[0],
> - PAM_BIOS_BASE, PAM_BIOS_SIZE);
> - for (i = 0; i < 12; ++i) {
> - init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
> - mch->pci_address_space, &mch->pam_regions[i+1],
> - PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
> - }
> + mch_init_pam(mch);
> }
>
> uint64_t mch_mcfg_base(void)
>
> On 08/04/2017 08:45, Anthony Xu wrote: > > split PAM SMRAM functions in piix.c > > create mch_init_pam in q35.c > > Could you further move > > MemoryRegion *ram_memory; > MemoryRegion *system_memory; > MemoryRegion *pci_address_space; > PAMMemoryRegion pam_regions[13]; > > from the northbridge devices up to PCMachineState, and move the common > code for PIIX and Q35 to hw/pci-host/pam.c? > > It looks like you can define a better API than what pam.c currently > provides: > > void pc_init_pam(PCMachineState *pc_machine, DeviceState *d); > void pc_update_pam(PCMachineState *pc_machine, uint8_t *pam_config); > > or even remove "DeviceState *d" because the owner of the memory regions > can be the PCMachineState. > Good idea! Will do. Anthony
© 2016 - 2026 Red Hat, Inc.