Re: [Qemu-devel] [PATCH 1/4] pam:refactor PAM related code

2017-04-10 Thread Xu, Anthony 
> 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

Re: [Qemu-devel] [PATCH 1/4] pam:refactor PAM related code

2017-04-08 Thread Paolo Bonzini 


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 
> ---
>  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(>pam_regions[i], i,
> -   pd->config[I440FX_PAM + ((i + 1) / 2)]);
> -}
>  memory_region_set_enabled(>smram_region,
>!(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
>  memory_region_set_enabled(>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(>pam_regions[0], 0,
> +   pd->config[I440FX_PAM]);
> +for (i = 1; i < 13; i++) {
> +pam_update(>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, >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, >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(>smram), _abort);
>  
> -init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
> - >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,
> - >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

[Qemu-devel] [PATCH 1/4] pam:refactor PAM related code

2017-04-07 Thread Anthony Xu 
split PAM SMRAM functions in piix.c
create mch_init_pam in q35.c

Signed-off-by: Anthony Xu 
---
 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(>pam_regions[i], i,
-   pd->config[I440FX_PAM + ((i + 1) / 2)]);
-}
 memory_region_set_enabled(>smram_region,
   !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
 memory_region_set_enabled(>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(>pam_regions[0], 0,
+   pd->config[I440FX_PAM]);
+for (i = 1; i < 13; i++) {
+pam_update(>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, >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, >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(>smram), _abort);
 
-init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
- >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,
- >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, >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, >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(>smram), _abort);
 
-init_pam(DEVICE(mch), mch->ram_memory,