Patchwork qemu: define a TOM register to report the base of PCI

login
register
mail settings
Submitter Hao, Xudong
Date Feb. 22, 2013, 3:23 a.m.
Message ID <1361503393-10726-1-git-send-email-xudong.hao@intel.com>
Download mbox | patch
Permalink /patch/222449/
State New
Headers show

Comments

Hao, Xudong - Feb. 22, 2013, 3:23 a.m.
Define a TOM(top of memory) register to report the base of PCI memory, update
memory region dynamically.

The use case of this register defination is for Xen till now.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 hw/pc.h       |  4 ---
 hw/pc_piix.c  |  6 -----
 hw/piix_pci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 59 insertions(+), 30 deletions(-)
Jan Beulich - Feb. 22, 2013, 9:03 a.m.
>>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote:
> @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev)
>  
>      d->dev.config[I440FX_SMRAM] = 0x02;
>  
> +    /* Emulate top of memory, here use 0xe0000000 as default val*/
> +    if (xen_enabled()) {
> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
> +    } else {
> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
> +    }
> +    d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
> +    d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
> +    d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
> +
>      cpu_smm_register(&i440fx_set_smm, d);
>      return 0;
>  }

Isn't this the wrong way round (big endian, when it needs to be
little)?

Or else, this read and calculation

>+    pci_hole_start = pci_default_read_config(&f->dev, I440FX_PCI_HOLE_BASE, 4);
>+    pci_hole_size = 0x100000000ULL - pci_hole_start;

would seem wrong (e.g. if the granularity is meant to be 16M).

And then again, wasting 4 bytes of config space on something that
one ought to be allowed to expect to be at least 64k-aligned seems
questionable too. hvmloader surely could align the value up to the
next 64k boundary before writing the - then only word size - field.
That would come closer to how real chipsets normally implement
things like this.

Jan
Stefano Stabellini - Feb. 22, 2013, 12:35 p.m.
On Fri, 22 Feb 2013, Xudong Hao wrote:
> Define a TOM(top of memory) register to report the base of PCI memory, update
> memory region dynamically.
> 
> The use case of this register defination is for Xen till now.
> 
> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>

Thanks for the patch!

Aside from Jan's comment on the pci config endianness, I would also like
to see an #define somewhere in QEMU to specific what's the default TOM.
In particular I wouldn't want 0xe0000000 (and 0xf0000000) to be
hardcoded in more than one place.


>  hw/pc.h       |  4 ---
>  hw/pc_piix.c  |  6 -----
>  hw/piix_pci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  3 files changed, 59 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/pc.h b/hw/pc.h
> index fbcf43d..2a60490 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -134,10 +134,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>                      MemoryRegion *address_space_mem,
>                      MemoryRegion *address_space_io,
>                      ram_addr_t ram_size,
> -                    hwaddr pci_hole_start,
> -                    hwaddr pci_hole_size,
> -                    hwaddr pci_hole64_start,
> -                    hwaddr pci_hole64_size,
>                      MemoryRegion *pci_memory,
>                      MemoryRegion *ram_memory);
>  
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0a6923d..98cf467 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -130,12 +130,6 @@ static void pc_init1(MemoryRegion *system_memory,
>      if (pci_enabled) {
>          pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
>                                system_memory, system_io, ram_size,
> -                              below_4g_mem_size,
> -                              0x100000000ULL - below_4g_mem_size,
> -                              0x100000000ULL + above_4g_mem_size,
> -                              (sizeof(hwaddr) == 4
> -                               ? 0
> -                               : ((uint64_t)1 << 62)),
>                                pci_memory, ram_memory);
>      } else {
>          pci_bus = NULL;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 3d79c73..88bd688 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -86,6 +86,14 @@ struct PCII440FXState {
>  #define I440FX_PAM_SIZE 7
>  #define I440FX_SMRAM    0x72
>  
> +/* The maximum vaule of TOM(top of memory) register in I440FX
> + * is 1G, so it doesn't meet any popular virutal machines, so
> + * define another register to report the base of PCI memory.
> + * Use four bytes 0xb0-0xb3 for this purpose, they are originally
> + * resevered for host bridge.
> + * */
> +#define I440FX_PCI_HOLE_BASE 0xb0
> +
>  static void piix3_set_irq(void *opaque, int pirq, int level);
>  static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pci_intx);
>  static void piix3_write_config_xen(PCIDevice *dev,
> @@ -101,6 +109,43 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
>      return (pci_intx + slot_addend) & 3;
>  }
>  
> +
> +static void i440fx_update_pci_mem_hole(PCII440FXState *f, bool del)
> +{
> +    ram_addr_t above_4g_mem_size;
> +    hwaddr pci_hole_start, pci_hole_size, pci_hole64_start, pci_hole64_size;
> +
> +    pci_hole_start = pci_default_read_config(&f->dev, I440FX_PCI_HOLE_BASE, 4);
> +    pci_hole_size = 0x100000000ULL - pci_hole_start;
> +
> +    if (ram_size >= pci_hole_start) {
> +        above_4g_mem_size = ram_size - pci_hole_start;
> +    } else {
> +        above_4g_mem_size = 0;
> +    }
> +    pci_hole64_start = 0x100000000ULL + above_4g_mem_size;
> +    pci_hole64_size = sizeof(hwaddr) == 4 ? 0 : ((uint64_t)1 << 62);
> +
> +    if (del) {
> +        memory_region_del_subregion(f->system_memory, &f->pci_hole);
> +        if (pci_hole64_size) {
> +            memory_region_del_subregion(f->system_memory, &f->pci_hole_64bit);
> +        }
> +    }
> +
> +    memory_region_init_alias(&f->pci_hole, "pci-hole", f->pci_address_space,
> +                             pci_hole_start, pci_hole_size);
> +    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> +    memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
> +                             f->pci_address_space,
> +                             pci_hole64_start, pci_hole64_size);
> +    if (pci_hole64_size) {
> +        memory_region_add_subregion(f->system_memory, pci_hole64_start,
> +                                    &f->pci_hole_64bit);
> +    }
> +}
> +
> +
>  static void i440fx_update_memory_mappings(PCII440FXState *d)
>  {
>      int i;
> @@ -136,6 +181,9 @@ static void i440fx_write_config(PCIDevice *dev,
>          range_covers_byte(address, len, I440FX_SMRAM)) {
>          i440fx_update_memory_mappings(d);
>      }
> +    if (range_covers_byte(address, len, I440FX_PCI_HOLE_BASE)) {
> +        i440fx_update_pci_mem_hole(d, true);
> +    }
>  }
>  
>  static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
> @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev)
>  
>      d->dev.config[I440FX_SMRAM] = 0x02;
>  
> +    /* Emulate top of memory, here use 0xe0000000 as default val*/
> +    if (xen_enabled()) {
> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
> +    } else {
> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
> +    }
> +    d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
> +    d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
> +    d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
> +
>      cpu_smm_register(&i440fx_set_smm, d);
>      return 0;
>  }
> @@ -214,10 +272,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
>                                    MemoryRegion *address_space_mem,
>                                    MemoryRegion *address_space_io,
>                                    ram_addr_t ram_size,
> -                                  hwaddr pci_hole_start,
> -                                  hwaddr pci_hole_size,
> -                                  hwaddr pci_hole64_start,
> -                                  hwaddr pci_hole64_size,
>                                    MemoryRegion *pci_address_space,
>                                    MemoryRegion *ram_memory)
>  {
> @@ -244,16 +298,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
>      f->system_memory = address_space_mem;
>      f->pci_address_space = pci_address_space;
>      f->ram_memory = ram_memory;
> -    memory_region_init_alias(&f->pci_hole, "pci-hole", f->pci_address_space,
> -                             pci_hole_start, pci_hole_size);
> -    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> -    memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
> -                             f->pci_address_space,
> -                             pci_hole64_start, pci_hole64_size);
> -    if (pci_hole64_size) {
> -        memory_region_add_subregion(f->system_memory, pci_hole64_start,
> -                                    &f->pci_hole_64bit);
> -    }
>      memory_region_init_alias(&f->smram_region, "smram-region",
>                               f->pci_address_space, 0xa0000, 0x20000);
>      memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
> @@ -295,6 +339,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
>      (*pi440fx_state)->dev.config[0x57]=ram_size;
>  
>      i440fx_update_memory_mappings(f);
> +    i440fx_update_pci_mem_hole(f, false);
>  
>      return b;
>  }
> @@ -304,10 +349,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>                      MemoryRegion *address_space_mem,
>                      MemoryRegion *address_space_io,
>                      ram_addr_t ram_size,
> -                    hwaddr pci_hole_start,
> -                    hwaddr pci_hole_size,
> -                    hwaddr pci_hole64_start,
> -                    hwaddr pci_hole64_size,
>                      MemoryRegion *pci_memory, MemoryRegion *ram_memory)
>  
>  {
> @@ -315,8 +356,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>  
>      b = i440fx_common_init("i440FX", pi440fx_state, piix3_devfn, isa_bus, pic,
>                             address_space_mem, address_space_io, ram_size,
> -                           pci_hole_start, pci_hole_size,
> -                           pci_hole64_start, pci_hole64_size,
>                             pci_memory, ram_memory);
>      return b;
>  }
> -- 
> 1.7.12.1
>
Hao, Xudong - Feb. 22, 2013, 3:37 p.m.
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, February 22, 2013 5:04 PM
> To: Hao, Xudong
> Cc: stefano.stabellini@eu.citrix.com; Zhang, Xiantao; xen-devel@lists.xen.org;
> qemu-devel@nongnu.org; mst@redhat.com; aliguori@us.ibm.com
> Subject: Re: [Xen-devel] [PATCH] qemu: define a TOM register to report the
> base of PCI
> 
> >>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote:
> > @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev)
> >
> >      d->dev.config[I440FX_SMRAM] = 0x02;
> >
> > +    /* Emulate top of memory, here use 0xe0000000 as default val*/
> > +    if (xen_enabled()) {
> > +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
> > +    } else {
> > +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
> > +    }
> > +    d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
> > +    d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
> > +    d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
> > +
> >      cpu_smm_register(&i440fx_set_smm, d);
> >      return 0;
> >  }
> 
> Isn't this the wrong way round (big endian, when it needs to be
> little)?
> 
Jan, 

Here it should be little endian since the following config reading is little endian, I'll correct it. Thanks your comments.

> Or else, this read and calculation
> 
> >+    pci_hole_start = pci_default_read_config(&f->dev,
> I440FX_PCI_HOLE_BASE, 4);
> >+    pci_hole_size = 0x100000000ULL - pci_hole_start;
> 
> would seem wrong (e.g. if the granularity is meant to be 16M).
> 
> And then again, wasting 4 bytes of config space on something that
> one ought to be allowed to expect to be at least 64k-aligned seems
> questionable too. hvmloader surely could align the value up to the
> next 64k boundary before writing the - then only word size - field.
Hvmloader did 64k-aligned, I'm not quite understand, could you help to point out?

If considering wasting of config space, actually hvmloader only write 4 values in this register: 3.75G(0xf0000000), 3.5G(0xe0000000), 3G(0xc0000000), 2G(0x80000000), then 1 byte is enough for xen. 

> That would come closer to how real chipsets normally implement
> things like this.
> 
> Jan
Hao, Xudong - Feb. 22, 2013, 3:42 p.m.
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Friday, February 22, 2013 8:35 PM
> To: Hao, Xudong
> Cc: aliguori@us.ibm.com; mst@redhat.com; qemu-devel@nongnu.org;
> Stefano Stabellini; xen-devel@lists.xen.org; Zhang, Xiantao
> Subject: Re: [PATCH] qemu: define a TOM register to report the base of PCI
> 
> On Fri, 22 Feb 2013, Xudong Hao wrote:
> > Define a TOM(top of memory) register to report the base of PCI memory,
> update
> > memory region dynamically.
> >
> > The use case of this register defination is for Xen till now.
> >
> > Signed-off-by: Xudong Hao <xudong.hao@intel.com>
> > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
> 
> Thanks for the patch!
> 
> Aside from Jan's comment on the pci config endianness, I would also like
> to see an #define somewhere in QEMU to specific what's the default TOM.
> In particular I wouldn't want 0xe0000000 (and 0xf0000000) to be
> hardcoded in more than one place.
> 
> 
Good comments, I'll define the default TOM out and replace the hardcode.

Thanks,
-Xudong
Jan Beulich - Feb. 22, 2013, 4:44 p.m.
>>> On 22.02.13 at 16:37, "Hao, Xudong" <xudong.hao@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> And then again, wasting 4 bytes of config space on something that
>> one ought to be allowed to expect to be at least 64k-aligned seems
>> questionable too. hvmloader surely could align the value up to the
>> next 64k boundary before writing the - then only word size - field.
> Hvmloader did 64k-aligned, I'm not quite understand, could you help to point 
> out?
> 
> If considering wasting of config space, actually hvmloader only write 4 
> values in this register: 3.75G(0xf0000000), 3.5G(0xe0000000), 3G(0xc0000000), 
> 2G(0x80000000), then 1 byte is enough for xen. 

I'd be fine with that too, but whether use 16M granularity here is
acceptable going forward I don't know.

Jan
Andreas Färber - Feb. 22, 2013, 6:52 p.m.
Am 22.02.2013 16:37, schrieb Hao, Xudong:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, February 22, 2013 5:04 PM
>> To: Hao, Xudong
>> Cc: stefano.stabellini@eu.citrix.com; Zhang, Xiantao; xen-devel@lists.xen.org;
>> qemu-devel@nongnu.org; mst@redhat.com; aliguori@us.ibm.com
>> Subject: Re: [Xen-devel] [PATCH] qemu: define a TOM register to report the
>> base of PCI
>>
>>>>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote:
>>> @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev)
>>>
>>>      d->dev.config[I440FX_SMRAM] = 0x02;
>>>
>>> +    /* Emulate top of memory, here use 0xe0000000 as default val*/
>>> +    if (xen_enabled()) {
>>> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
>>> +    } else {
>>> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
>>> +    }
>>> +    d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
>>> +    d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
>>> +    d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
>>> +
>>>      cpu_smm_register(&i440fx_set_smm, d);
>>>      return 0;
>>>  }
>>
>> Isn't this the wrong way round (big endian, when it needs to be
>> little)?
>>
> Jan, 
> 
> Here it should be little endian since the following config reading is little endian, I'll correct it. Thanks your comments.

Your colleague David Woodhouse has just prepared some i440fx cleanups.
Please use dev->config instead of the indirect d->dev.config.

Given Jan's endianness comment, would alignment allow you to simply
write as follows?

uint32_t addr = xen_enabled() ? 0xe0000000 : 0xf0000000;
*(uint32_t *)&dev->config[I440FX_PCI_HOLE_BASE] = cpu_to_le32(addr);

Then the byte-swapping would be explicit and the address in one piece
grep'able. Alternatively:

cpu_to_le32s(&addr);
for (i = 0; i < 4; i++) {
    dev->config[... + i] = ((uint8_t *)&addr)[i];
}

Anyway, please use "piix: " in the subject rather than "qemu: " if this
is supposed to go into upstream qemu.git.

Regards,
Andreas
Michael S. Tsirkin - Feb. 23, 2013, 8:41 p.m.
On Fri, Feb 22, 2013 at 07:52:26PM +0100, Andreas Färber wrote:
> Am 22.02.2013 16:37, schrieb Hao, Xudong:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, February 22, 2013 5:04 PM
> >> To: Hao, Xudong
> >> Cc: stefano.stabellini@eu.citrix.com; Zhang, Xiantao; xen-devel@lists.xen.org;
> >> qemu-devel@nongnu.org; mst@redhat.com; aliguori@us.ibm.com
> >> Subject: Re: [Xen-devel] [PATCH] qemu: define a TOM register to report the
> >> base of PCI
> >>
> >>>>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote:
> >>> @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev)
> >>>
> >>>      d->dev.config[I440FX_SMRAM] = 0x02;
> >>>
> >>> +    /* Emulate top of memory, here use 0xe0000000 as default val*/
> >>> +    if (xen_enabled()) {
> >>> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
> >>> +    } else {
> >>> +        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
> >>> +    }
> >>> +    d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
> >>> +    d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
> >>> +    d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
> >>> +
> >>>      cpu_smm_register(&i440fx_set_smm, d);
> >>>      return 0;
> >>>  }
> >>
> >> Isn't this the wrong way round (big endian, when it needs to be
> >> little)?
> >>
> > Jan, 
> > 
> > Here it should be little endian since the following config reading is little endian, I'll correct it. Thanks your comments.
> 
> Your colleague David Woodhouse has just prepared some i440fx cleanups.
> Please use dev->config instead of the indirect d->dev.config.
> 
> Given Jan's endianness comment, would alignment allow you to simply
> write as follows?
> 
> uint32_t addr = xen_enabled() ? 0xe0000000 : 0xf0000000;
> *(uint32_t *)&dev->config[I440FX_PCI_HOLE_BASE] = cpu_to_le32(addr);
> Then the byte-swapping would be explicit and the address in one piece
> grep'able. Alternatively:
> 
> cpu_to_le32s(&addr);
> for (i = 0; i < 4; i++) {
>     dev->config[... + i] = ((uint8_t *)&addr)[i];
> }

Please don't do either of these things, do not hack endian-ness manually,
we have APIs for pci endian-ness.  Please use pci_set_long
and friends for pci config accesses.

> 
> Anyway, please use "piix: " in the subject rather than "qemu: " if this
> is supposed to go into upstream qemu.git.
> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/hw/pc.h b/hw/pc.h
index fbcf43d..2a60490 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -134,10 +134,6 @@  PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
                     ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
-                    hwaddr pci_hole64_start,
-                    hwaddr pci_hole64_size,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
 
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0a6923d..98cf467 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -130,12 +130,6 @@  static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, ram_size,
-                              below_4g_mem_size,
-                              0x100000000ULL - below_4g_mem_size,
-                              0x100000000ULL + above_4g_mem_size,
-                              (sizeof(hwaddr) == 4
-                               ? 0
-                               : ((uint64_t)1 << 62)),
                               pci_memory, ram_memory);
     } else {
         pci_bus = NULL;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 3d79c73..88bd688 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -86,6 +86,14 @@  struct PCII440FXState {
 #define I440FX_PAM_SIZE 7
 #define I440FX_SMRAM    0x72
 
+/* The maximum vaule of TOM(top of memory) register in I440FX
+ * is 1G, so it doesn't meet any popular virutal machines, so
+ * define another register to report the base of PCI memory.
+ * Use four bytes 0xb0-0xb3 for this purpose, they are originally
+ * resevered for host bridge.
+ * */
+#define I440FX_PCI_HOLE_BASE 0xb0
+
 static void piix3_set_irq(void *opaque, int pirq, int level);
 static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pci_intx);
 static void piix3_write_config_xen(PCIDevice *dev,
@@ -101,6 +109,43 @@  static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
     return (pci_intx + slot_addend) & 3;
 }
 
+
+static void i440fx_update_pci_mem_hole(PCII440FXState *f, bool del)
+{
+    ram_addr_t above_4g_mem_size;
+    hwaddr pci_hole_start, pci_hole_size, pci_hole64_start, pci_hole64_size;
+
+    pci_hole_start = pci_default_read_config(&f->dev, I440FX_PCI_HOLE_BASE, 4);
+    pci_hole_size = 0x100000000ULL - pci_hole_start;
+
+    if (ram_size >= pci_hole_start) {
+        above_4g_mem_size = ram_size - pci_hole_start;
+    } else {
+        above_4g_mem_size = 0;
+    }
+    pci_hole64_start = 0x100000000ULL + above_4g_mem_size;
+    pci_hole64_size = sizeof(hwaddr) == 4 ? 0 : ((uint64_t)1 << 62);
+
+    if (del) {
+        memory_region_del_subregion(f->system_memory, &f->pci_hole);
+        if (pci_hole64_size) {
+            memory_region_del_subregion(f->system_memory, &f->pci_hole_64bit);
+        }
+    }
+
+    memory_region_init_alias(&f->pci_hole, "pci-hole", f->pci_address_space,
+                             pci_hole_start, pci_hole_size);
+    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
+    memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
+                             f->pci_address_space,
+                             pci_hole64_start, pci_hole64_size);
+    if (pci_hole64_size) {
+        memory_region_add_subregion(f->system_memory, pci_hole64_start,
+                                    &f->pci_hole_64bit);
+    }
+}
+
+
 static void i440fx_update_memory_mappings(PCII440FXState *d)
 {
     int i;
@@ -136,6 +181,9 @@  static void i440fx_write_config(PCIDevice *dev,
         range_covers_byte(address, len, I440FX_SMRAM)) {
         i440fx_update_memory_mappings(d);
     }
+    if (range_covers_byte(address, len, I440FX_PCI_HOLE_BASE)) {
+        i440fx_update_pci_mem_hole(d, true);
+    }
 }
 
 static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
@@ -203,6 +251,16 @@  static int i440fx_initfn(PCIDevice *dev)
 
     d->dev.config[I440FX_SMRAM] = 0x02;
 
+    /* Emulate top of memory, here use 0xe0000000 as default val*/
+    if (xen_enabled()) {
+        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
+    } else {
+        d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
+    }
+    d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
+    d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
+    d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
+
     cpu_smm_register(&i440fx_set_smm, d);
     return 0;
 }
@@ -214,10 +272,6 @@  static PCIBus *i440fx_common_init(const char *device_name,
                                   MemoryRegion *address_space_mem,
                                   MemoryRegion *address_space_io,
                                   ram_addr_t ram_size,
-                                  hwaddr pci_hole_start,
-                                  hwaddr pci_hole_size,
-                                  hwaddr pci_hole64_start,
-                                  hwaddr pci_hole64_size,
                                   MemoryRegion *pci_address_space,
                                   MemoryRegion *ram_memory)
 {
@@ -244,16 +298,6 @@  static PCIBus *i440fx_common_init(const char *device_name,
     f->system_memory = address_space_mem;
     f->pci_address_space = pci_address_space;
     f->ram_memory = ram_memory;
-    memory_region_init_alias(&f->pci_hole, "pci-hole", f->pci_address_space,
-                             pci_hole_start, pci_hole_size);
-    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
-    memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
-                             f->pci_address_space,
-                             pci_hole64_start, pci_hole64_size);
-    if (pci_hole64_size) {
-        memory_region_add_subregion(f->system_memory, pci_hole64_start,
-                                    &f->pci_hole_64bit);
-    }
     memory_region_init_alias(&f->smram_region, "smram-region",
                              f->pci_address_space, 0xa0000, 0x20000);
     memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
@@ -295,6 +339,7 @@  static PCIBus *i440fx_common_init(const char *device_name,
     (*pi440fx_state)->dev.config[0x57]=ram_size;
 
     i440fx_update_memory_mappings(f);
+    i440fx_update_pci_mem_hole(f, false);
 
     return b;
 }
@@ -304,10 +349,6 @@  PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
                     ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
-                    hwaddr pci_hole64_start,
-                    hwaddr pci_hole64_size,
                     MemoryRegion *pci_memory, MemoryRegion *ram_memory)
 
 {
@@ -315,8 +356,6 @@  PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
 
     b = i440fx_common_init("i440FX", pi440fx_state, piix3_devfn, isa_bus, pic,
                            address_space_mem, address_space_io, ram_size,
-                           pci_hole_start, pci_hole_size,
-                           pci_hole64_start, pci_hole64_size,
                            pci_memory, ram_memory);
     return b;
 }