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...@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)
>
