[Qemu-devel] [PATCH] piix: fix 32bit pci hole

[Qemu-devel] [PATCH] piix: fix 32bit pci hole

[Gerd Hoffmann]

Make the 32bit pci hole start at end of ram, so all possible address
space is covered.  Of course the firmware can use less than that.
Leaving space unused is no problem, mapping pci bars outside the
hole causes problems though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/piix.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index edc974e..1414a2b 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     f->ram_memory = ram_memory;
 
     i440fx = I440FX_PCI_HOST_BRIDGE(dev);
-    /* Set PCI window size the way seabios has always done it. */
-    /* Power of 2 so bios can cover it with a single MTRR */
-    if (ram_size <= 0x80000000) {
-        i440fx->pci_info.w32.begin = 0x80000000;
-    } else if (ram_size <= 0xc0000000) {
-        i440fx->pci_info.w32.begin = 0xc0000000;
-    } else {
-        i440fx->pci_info.w32.begin = 0xe0000000;
-    }
+    i440fx->pci_info.w32.begin = ram_size;
 
     memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
                              pci_hole_start, pci_hole_size);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] piix: fix 32bit pci hole

[Laszlo Ersek]

On 11/26/13 17:16, Gerd Hoffmann wrote:
> Make the 32bit pci hole start at end of ram, so all possible address
> space is covered.  Of course the firmware can use less than that.
> Leaving space unused is no problem, mapping pci bars outside the
> hole causes problems though.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/piix.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index edc974e..1414a2b 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      f->ram_memory = ram_memory;
>  
>      i440fx = I440FX_PCI_HOST_BRIDGE(dev);
> -    /* Set PCI window size the way seabios has always done it. */
> -    /* Power of 2 so bios can cover it with a single MTRR */
> -    if (ram_size <= 0x80000000) {
> -        i440fx->pci_info.w32.begin = 0x80000000;
> -    } else if (ram_size <= 0xc0000000) {
> -        i440fx->pci_info.w32.begin = 0xc0000000;
> -    } else {
> -        i440fx->pci_info.w32.begin = 0xe0000000;
> -    }
> +    i440fx->pci_info.w32.begin = ram_size;
>  
>      memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
>                               pci_hole_start, pci_hole_size);
> 

You've convinced me in the other thread that we can lower w32.begin as
far as we want, as long as it doesn't end up below the top of RAM.

But this patch also obliterates the high bound, 0xe0000000, which can
lead to:
- w32.end - w32.begin <= 512M, or
- a special case of the former, w32.end < w32.begin.

w32.end is set to IO_APIC_DEFAULT_ADDRESS==0xfec00000. (Which is BTW
fine for OVMF too.) What will happen in a 6G guest, for example?

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] piix: fix 32bit pci hole

[Jordan Justen]

On Tue, Nov 26, 2013 at 8:16 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Make the 32bit pci hole start at end of ram, so all possible address
> space is covered.  Of course the firmware can use less than that.
> Leaving space unused is no problem, mapping pci bars outside the
> hole causes problems though.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/piix.c | 10 +---------

It seems like q35 should do something similar, but I'm not sure it
even looks at the ram size.

-Jordan

>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index edc974e..1414a2b 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      f->ram_memory = ram_memory;
>
>      i440fx = I440FX_PCI_HOST_BRIDGE(dev);
> -    /* Set PCI window size the way seabios has always done it. */
> -    /* Power of 2 so bios can cover it with a single MTRR */
> -    if (ram_size <= 0x80000000) {
> -        i440fx->pci_info.w32.begin = 0x80000000;
> -    } else if (ram_size <= 0xc0000000) {
> -        i440fx->pci_info.w32.begin = 0xc0000000;
> -    } else {
> -        i440fx->pci_info.w32.begin = 0xe0000000;
> -    }
> +    i440fx->pci_info.w32.begin = ram_size;
>
>      memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
>                               pci_hole_start, pci_hole_size);
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH] piix: fix 32bit pci hole

[Gerd Hoffmann]

> > +    i440fx->pci_info.w32.begin = ram_size;

> But this patch also obliterates the high bound, 0xe0000000, which can
> lead to:
> - w32.end - w32.begin <= 512M, or
> - a special case of the former, w32.end < w32.begin.

ram_size is not the total amount of memory, it is low memory only.
There is another parameter to i440fx_init (above_4g_mem_size) which
holds the amount of memory which is going to be mapped above 4G.

Maximum possible value of ram_size is 0xe0000000.
For completeness:  On q35 the maximum amount of low mem is 0xb0000000.

> w32.end is set to IO_APIC_DEFAULT_ADDRESS==0xfec00000. (Which is BTW
> fine for OVMF too.) What will happen in a 6G guest, for example?

ram_size = 0xe0000000
above_4g_mem_size = 0xa0000000

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] piix: fix 32bit pci hole

[Laszlo Ersek]

On 11/27/13 07:46, Gerd Hoffmann wrote:
> 
>>> +    i440fx->pci_info.w32.begin = ram_size;
> 
>> But this patch also obliterates the high bound, 0xe0000000, which can
>> lead to:
>> - w32.end - w32.begin <= 512M, or
>> - a special case of the former, w32.end < w32.begin.
> 
> ram_size is not the total amount of memory, it is low memory only.

I tried to check for that when I sent the email, and I couldn't see
where this is enforced. See:

pc_init1() [hw/i386/pc_piix.c]
  i440fx_init() [hw/pci-host/piix.c]

pc_init1() receives the full RAM size in "args->ram_size". It uses this
value to compute "above_4g_mem_size" and "below_4g_mem_size", for example.

pc_init1() passes "args->ram_size" unchanged to i440fx_init(), as 7th
argument.

The 7th parameter of i440fx_init() is called "ram_size". Therefore it
will hold the whole RAM size.

Thanks
Laszlo