[Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

The commit:

Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
Author: Marcel Apfelbaum <marcel.a@redhat.com>
Date:   Mon Sep 16 11:21:16 2013 +0300

    hw/pci: partially handle pci master abort

introduced a regression on make check:

qemu-system-mips64el: /home/andreas/QEMU/qemu-cpu/exec.c:802:
register_subpage: Assertion `existing->mr->subpage || existing->mr ==
&io_mem_unassigned' failed.

The problem appears when a root memory region within an
address space with size < UINT64_MAX has overlapping children
with the same size. If the size of the root memory region is UINT64_MAX
everyting is ok.

Solved the regression by making the system-memory region
of size UINT64_MAX instead of INT64_MAX.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
In the mean time I am investigating why the
root memory region has to be UINT64_MAX size in order
to have overlapping children


 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index b453713..8715cf4 100644
--- a/exec.c
+++ b/exec.c
@@ -1741,7 +1741,7 @@ void address_space_destroy_dispatch(AddressSpace *as)
 static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
-    memory_region_init(system_memory, NULL, "system", INT64_MAX);
+    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
     address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Peter Maydell]

On 3 November 2013 20:48, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> The problem appears when a root memory region within an
> address space with size < UINT64_MAX has overlapping children
> with the same size. If the size of the root memory region is UINT64_MAX
> everyting is ok.
>
> Solved the regression by making the system-memory region
> of size UINT64_MAX instead of INT64_MAX.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> In the mean time I am investigating why the
> root memory region has to be UINT64_MAX size in order
> to have overlapping children

>      system_memory = g_malloc(sizeof(*system_memory));
> -    memory_region_init(system_memory, NULL, "system", INT64_MAX);
> +    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
>      address_space_init(&address_space_memory, system_memory, "memory");

As you say above we should investigate why this caused a
problem, but I was surprised the system memory space isn't
already maximum size. It turns out that that change was
introduced in commit 8417cebf in an attempt to avoid overflow
issues by sticking to signed 64 bit arithmetic. This approach was
subsequently ditched in favour of using proper 128 bit arithmetic
in commit 08dafab4, but we never changed the init call for
the system memory back to UINT64_MAX. So I think this is
a good change in itself.

-- PMM

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Michael S. Tsirkin]

On Sun, Nov 03, 2013 at 09:26:06PM +0000, Peter Maydell wrote:
> On 3 November 2013 20:48, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > The problem appears when a root memory region within an
> > address space with size < UINT64_MAX has overlapping children
> > with the same size. If the size of the root memory region is UINT64_MAX
> > everyting is ok.
> >
> > Solved the regression by making the system-memory region
> > of size UINT64_MAX instead of INT64_MAX.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > In the mean time I am investigating why the
> > root memory region has to be UINT64_MAX size in order
> > to have overlapping children
> 
> >      system_memory = g_malloc(sizeof(*system_memory));
> > -    memory_region_init(system_memory, NULL, "system", INT64_MAX);
> > +    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> >      address_space_init(&address_space_memory, system_memory, "memory");
> 
> As you say above we should investigate why this caused a
> problem, but I was surprised the system memory space isn't
> already maximum size. It turns out that that change was
> introduced in commit 8417cebf in an attempt to avoid overflow
> issues by sticking to signed 64 bit arithmetic. This approach was
> subsequently ditched in favour of using proper 128 bit arithmetic
> in commit 08dafab4, but we never changed the init call for
> the system memory back to UINT64_MAX. So I think this is
> a good change in itself.
> 
> -- PMM

I think I debugged it.

So this patch seems to help simply because we only have
sanity checking asserts in the subpage path. UINT64_MAX will make
the region a number of full pages and avoid
hitting the checks.


I think I see what the issue is: exec.c
assumes that TARGET_PHYS_ADDR_SPACE_BITS is enough
to render any section in system memory:
number of page table levels is calculated from that:

#define P_L2_LEVELS \
	(((TARGET_PHYS_ADDR_SPACE_BITS - TARGET_PAGE_BITS - 1) / L2_BITS) + 1)

any other bits are simply ignored:

    for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
        if (lp.ptr == PHYS_MAP_NODE_NIL) {
            return &sections[PHYS_SECTION_UNASSIGNED];
        }
        p = nodes[lp.ptr];
        lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
    }

so mask by L2_SIZE - 1 means that each round looks at L2_BITS bits,
and there are at most P_L2_LEVELS.

Any other bits are simply ignored.
This is very wrong and can break in a number of other ways,
for example I think we will also hit this assert
if we have a non aligned 64 bit BAR of a PCI device.

I think the fastest solution is to just limit
system memory size of TARGET_PAGE_BITS.
I sent a patch like this.



-- 
MST

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Mon, 2013-11-04 at 08:18 +0200, Michael S. Tsirkin wrote:
> On Sun, Nov 03, 2013 at 09:26:06PM +0000, Peter Maydell wrote:
> > On 3 November 2013 20:48, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > > The problem appears when a root memory region within an
> > > address space with size < UINT64_MAX has overlapping children
> > > with the same size. If the size of the root memory region is UINT64_MAX
> > > everyting is ok.
> > >
> > > Solved the regression by making the system-memory region
> > > of size UINT64_MAX instead of INT64_MAX.
> > >
> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > > In the mean time I am investigating why the
> > > root memory region has to be UINT64_MAX size in order
> > > to have overlapping children
> > 
> > >      system_memory = g_malloc(sizeof(*system_memory));
> > > -    memory_region_init(system_memory, NULL, "system", INT64_MAX);
> > > +    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> > >      address_space_init(&address_space_memory, system_memory, "memory");
> > 
> > As you say above we should investigate why this caused a
> > problem, but I was surprised the system memory space isn't
> > already maximum size. It turns out that that change was
> > introduced in commit 8417cebf in an attempt to avoid overflow
> > issues by sticking to signed 64 bit arithmetic. This approach was
> > subsequently ditched in favour of using proper 128 bit arithmetic
> > in commit 08dafab4, but we never changed the init call for
> > the system memory back to UINT64_MAX. So I think this is
> > a good change in itself.
> > 
> > -- PMM
> 
> I think I debugged it.
> 
> So this patch seems to help simply because we only have
> sanity checking asserts in the subpage path. UINT64_MAX will make
> the region a number of full pages and avoid
> hitting the checks.
> 
> 
> I think I see what the issue is: exec.c
> assumes that TARGET_PHYS_ADDR_SPACE_BITS is enough
> to render any section in system memory:
> number of page table levels is calculated from that:
> 
> #define P_L2_LEVELS \
> 	(((TARGET_PHYS_ADDR_SPACE_BITS - TARGET_PAGE_BITS - 1) / L2_BITS) + 1)
> 
> any other bits are simply ignored:
> 
>     for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
>         if (lp.ptr == PHYS_MAP_NODE_NIL) {
>             return &sections[PHYS_SECTION_UNASSIGNED];
>         }
>         p = nodes[lp.ptr];
>         lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
>     }
> 
> so mask by L2_SIZE - 1 means that each round looks at L2_BITS bits,
> and there are at most P_L2_LEVELS.
> 
> Any other bits are simply ignored.

Michael, thanks for helping to debug this issue.
Let me see if I got it right:
If the system memory size is INT64_MAX (0x7fffffffffffffff), the address of the
last page (0x7ffffffffffff) has more bits (55) that TARGET_PHYS_ADDR_SPACE_BITS (52)
and cannot be correctly mapped into page levels?

Thanks,
Marcel

> This is very wrong and can break in a number of other ways,
> for example I think we will also hit this assert
> if we have a non aligned 64 bit BAR of a PCI device.
> 
> I think the fastest solution is to just limit
> system memory size of TARGET_PAGE_BITS.
> I sent a patch like this.
> 
> 
> 

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Michael S. Tsirkin]

On Mon, Nov 04, 2013 at 11:33:56AM +0200, Marcel Apfelbaum wrote:
> On Mon, 2013-11-04 at 08:18 +0200, Michael S. Tsirkin wrote:
> > On Sun, Nov 03, 2013 at 09:26:06PM +0000, Peter Maydell wrote:
> > > On 3 November 2013 20:48, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > > > The problem appears when a root memory region within an
> > > > address space with size < UINT64_MAX has overlapping children
> > > > with the same size. If the size of the root memory region is UINT64_MAX
> > > > everyting is ok.
> > > >
> > > > Solved the regression by making the system-memory region
> > > > of size UINT64_MAX instead of INT64_MAX.
> > > >
> > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > > > ---
> > > > In the mean time I am investigating why the
> > > > root memory region has to be UINT64_MAX size in order
> > > > to have overlapping children
> > > 
> > > >      system_memory = g_malloc(sizeof(*system_memory));
> > > > -    memory_region_init(system_memory, NULL, "system", INT64_MAX);
> > > > +    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> > > >      address_space_init(&address_space_memory, system_memory, "memory");
> > > 
> > > As you say above we should investigate why this caused a
> > > problem, but I was surprised the system memory space isn't
> > > already maximum size. It turns out that that change was
> > > introduced in commit 8417cebf in an attempt to avoid overflow
> > > issues by sticking to signed 64 bit arithmetic. This approach was
> > > subsequently ditched in favour of using proper 128 bit arithmetic
> > > in commit 08dafab4, but we never changed the init call for
> > > the system memory back to UINT64_MAX. So I think this is
> > > a good change in itself.
> > > 
> > > -- PMM
> > 
> > I think I debugged it.
> > 
> > So this patch seems to help simply because we only have
> > sanity checking asserts in the subpage path. UINT64_MAX will make
> > the region a number of full pages and avoid
> > hitting the checks.
> > 
> > 
> > I think I see what the issue is: exec.c
> > assumes that TARGET_PHYS_ADDR_SPACE_BITS is enough
> > to render any section in system memory:
> > number of page table levels is calculated from that:
> > 
> > #define P_L2_LEVELS \
> > 	(((TARGET_PHYS_ADDR_SPACE_BITS - TARGET_PAGE_BITS - 1) / L2_BITS) + 1)
> > 
> > any other bits are simply ignored:
> > 
> >     for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
> >         if (lp.ptr == PHYS_MAP_NODE_NIL) {
> >             return &sections[PHYS_SECTION_UNASSIGNED];
> >         }
> >         p = nodes[lp.ptr];
> >         lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
> >     }
> > 
> > so mask by L2_SIZE - 1 means that each round looks at L2_BITS bits,
> > and there are at most P_L2_LEVELS.
> > 
> > Any other bits are simply ignored.
> 
> Michael, thanks for helping to debug this issue.
> Let me see if I got it right:
> If the system memory size is INT64_MAX (0x7fffffffffffffff), the address of the
> last page (0x7ffffffffffff) has more bits (55) that TARGET_PHYS_ADDR_SPACE_BITS (52)
> and cannot be correctly mapped into page levels?
> 
> Thanks,
> Marcel

Yes, I think that's it.

> > This is very wrong and can break in a number of other ways,
> > for example I think we will also hit this assert
> > if we have a non aligned 64 bit BAR of a PCI device.
> > 
> > I think the fastest solution is to just limit
> > system memory size of TARGET_PAGE_BITS.
> > I sent a patch like this.
> > 
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Jordan Justen]

On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> The commit:
>
> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
> Author: Marcel Apfelbaum <marcel.a@redhat.com>
> Date:   Mon Sep 16 11:21:16 2013 +0300
>
>     hw/pci: partially handle pci master abort
>
> introduced a regression on make check:

Laszlo pointed out that my OVMF flash support series was not working
with QEMU master. It was working with QEMU 1.6.0. I then bisected the
issue to this commit. It seems this commit regresses -pflash support
for both KVM and non-KVM modes.

Can you reproduce the issue this with command?
x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin
(with or without adding -enable-kvm)

I tried adding this patch ("exec: fix regression by making
system-memory region UINT64_MAX size") and it did not help the -pflash
regression.

-Jordan

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Thu, 2013-11-07 at 12:27 -0800, Jordan Justen wrote:
> On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > The commit:
> >
> > Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
> > Author: Marcel Apfelbaum <marcel.a@redhat.com>
> > Date:   Mon Sep 16 11:21:16 2013 +0300
> >
> >     hw/pci: partially handle pci master abort
> >
> > introduced a regression on make check:
> 
> Laszlo pointed out that my OVMF flash support series was not working
> with QEMU master. It was working with QEMU 1.6.0. I then bisected the
> issue to this commit. It seems this commit regresses -pflash support
> for both KVM and non-KVM modes.
> 
> Can you reproduce the issue this with command?
> x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin
> (with or without adding -enable-kvm)
Thanks Jordan for pointing this out.
This patch revealed a lot of hidden issues inside qemu ...
I succeeded to reproduce the issue.

I saw that get_page_addr_code associated address 0xfffffff0
(after calling io_tlb_to_region) with the new master abort region.
It breaks because memory_region_is_unassigned returns true for the new region.
It is interesting what happened before this region was added.

I will investigate this issue,
Thanks,
Marcel

 

> I tried adding this patch ("exec: fix regression by making
> system-memory region UINT64_MAX size") and it did not help the -pflash
> regression.
> 
> -Jordan

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 12358 bytes --]

This is a QEMU bug report, only disguised as an edk2-devel followup.

The problem in a nutshell is that the OVMF binary, placed into pflash
(read-only KVM memslot) used to run in qemu-1.6, but it fails to start
in qemu-1.7. The OVMF reset vector reads as 0xFF bytes.

(Jordan and myself started writing these emails in parallel.)

On 11/07/13 21:27, Jordan Justen wrote:
> On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum
> <marcel.a@redhat.com> wrote:
>> The commit:
>>
>> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
>> Author: Marcel Apfelbaum <marcel.a@redhat.com>
>> Date:   Mon Sep 16 11:21:16 2013 +0300
>>
>>     hw/pci: partially handle pci master abort
>>
>> introduced a regression on make check:
>
> Laszlo pointed out that my OVMF flash support series was not working
> with QEMU master. It was working with QEMU 1.6.0. I then bisected the
> issue to this commit. It seems this commit regresses -pflash support
> for both KVM and non-KVM modes.
>
> Can you reproduce the issue this with command?
> x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin
> (with or without adding -enable-kvm)
>
> I tried adding this patch ("exec: fix regression by making
> system-memory region UINT64_MAX size") and it did not help the -pflash
> regression.


>From the edk2-devel discussion:
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4787/focus=4812>

On 11/07/13 19:07, Laszlo Ersek wrote:
> On 11/07/13 17:28, Laszlo Ersek wrote:
>> On 11/06/13 23:29, Jordan Justen wrote:
>>> https://github.com/jljusten/edk2.git ovmf-nvvars-v2
>>>
>>> This series implements support for QEMU's emulated
>>> system flash.
>>>
>>> This allows for persistent UEFI non-volatile variables.
>>>
>>> Previously we attempted to emulate non-volatile
>>> variables in a few ways, but each of them would fail
>>> in particular situations.
>>>
>>> To support flash based variable storage, we:
>>>  * Reserve space in the firmware device image
>>>  * Add a new flash firmware volume block driver
>>>  * Disable EmuVariableFvbRuntimeDxe (at runtime) if QEMU
>>>    flash is available.
>>>
>>> To use:
>>>  * QEMU version 1.1 or newer is required without KVM
>>>  * KVM support requires Linux 3.7 and QEMU 1.6
>>>  * Run QEMU with -pflash OVMF.fd instead of -L or -bios
>>>    or use OvmfPkg/build.sh --enable-flash qemu ...
>>>  * If QEMU is 1.6 or newer, then OvmfPkg/build.sh will
>>>    automatically enable flash when running QEMU, so in
>>>    that case --enable-flash is not required.
>>>
>>> See also:
>>>  * http://wiki.qemu.org/Features/PC_System_Flash
>>>
>>> v2:
>>>  * Replace
>>>      "OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions"
>>>    with
>>>      "OvmfPkg/AcpiPlatformDxe/Qemu: Decrease upper limit for PCI window 32"
>>>  * Separate portions of
>>>      "OvmfPkg/build.sh: Support --enable-flash switch"
>>>    out into a new patch
>>>      "OvmfPkg/build.sh: Enable flash for QEMU >= 1.6"
>>>  * Add "OvmfPkg/README: Add information about OVMF flash layout"
>>>  * Update "OvmfPkg: Add NV Variable storage within FD" to also change the
>>>    size of PcdVariableStoreSize
>>>  * Update commit messages on a couple patches for better clarity
>>
>> Tested in the following configurations:
>>
>> (1) RHEL-6 host (no KVM support, no qemu support -- that is,
>> regression test): RHEL-6, Fedora 19, Windows 2008 R2 (needs CSM),
>> Windows 2012 R2 boot tests work OK.
>>
>> (2) 3.10-based host kernel, qemu v1.7.0-rc0, Xeon W3550 host CPU:
>> Unfortunately qemu dies with the following KVM trace:
>>
>>   KVM internal error. Suberror: 1
>>   emulation failure
>>   EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000623
>>   ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
>>   EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>>   ES =0000 00000000 0000ffff 00009300
>>   CS =f000 ffff0000 0000ffff 00009b00
>>   SS =0000 00000000 0000ffff 00009300
>>   DS =0000 00000000 0000ffff 00009300
>>   FS =0000 00000000 0000ffff 00009300
>>   GS =0000 00000000 0000ffff 00009300
>>   LDT=0000 00000000 0000ffff 00008200
>>   TR =0000 00000000 0000ffff 00008b00
>>   GDT=     00000000 0000ffff
>>   IDT=     00000000 0000ffff
>>   CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
>>   DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>>   DR6=00000000ffff0ff0 DR7=0000000000000400
>>   EFER=0000000000000000
>>   Code=ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff <ff> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>>
>> I'm quite unsure, but the CS:IP value seems to point at the reset
>> vector, no? However, the Code=... log only shows 0xFF bytes.
>>
>> (3) 3.10.17 host kernel, qemu v1.7.0-rc0, Athlon II X2 B22 host CPU:
>> almost identical KVM error, with the following difference:
>>
>>   --- vmx 2013-11-07 17:23:45.612973935 +0100
>>   +++ svm 2013-11-07 17:24:17.237973059 +0100
>>   @@ -1,6 +1,6 @@
>>    KVM internal error. Suberror: 1
>>    emulation failure
>>   -EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000623
>>   +EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000663
>>    ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
>>    EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>>    ES =0000 00000000 0000ffff 00009300
>>
>> Any ideas?
>
> I.
> This is a QEMU regression (somewhere between v1.6.0 and v1.7.0-rc0),
> I'll have to bisect it.

This bug is "caused" by the following commit:

  a53ae8e934cd54686875b5bcfc2f434244ee55d6 is the first bad commit
  commit a53ae8e934cd54686875b5bcfc2f434244ee55d6
  Author: Marcel Apfelbaum <marcel.a@redhat.com>
  Date:   Mon Sep 16 11:21:16 2013 +0300

      hw/pci: partially handle pci master abort

      A MemoryRegion with negative priority was created and
      it spans over all the pci address space.
      It "intercepts" the accesses to unassigned pci
      address space and will follow the pci spec:
       1. returns -1 on read
       2. does nothing on write

      Note: setting the RECEIVED MASTER ABORT bit in the STATUS register
            of the device that initiated the transaction will be
            implemented in another series

      Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
      Acked-by: Michael S. Tsirkin <mst@redhat.com>
      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

The patch was posted as patch 3/3 of the series

  [Qemu-devel] [PATCH v5 0/3] pci: partially implement master abort
  protocol

  http://thread.gmane.org/gmane.comp.emulators.qemu/233798/focus=233801

Basically, the series adds a "background" memory region that covers all
"unassigned" PCI addresses, and patch 3/3 specifically makes sure that
writes to this region are dropped, and reads all return -1 (0xFFFFFFFF).

The read implementation (master_abort_mem_read(), -1) is consistent with
the KVM dump in the quoted part above.

For some reason, this "background" region pushes into the "foreground"
when it comes to the pflash region just below 4GB (in guest-phys address
space).

Or, hm, supposing we start to run in real mode at FFFF:0000, the problem
could be with the "isa-bios" region too.

So I think we have a bug in qemu, which is likely one of the three
below:

(1) This commit is wrong. Or,
(2) the pflash implementation is wrong, because it doesn't register a
    memory region (with appropriate priority) that would catch the
    access.
(3) Both this commit and the pflash implementations are right, but this
    is an unusual situation that the memory infrastructure doesn't
    handle well.

(I doubt that the problem is in KVM.)

When the bug hits, the "info qtree" command has the following to say
about the flash device:

  dev: cfi.pflash01, id ""
    drive = pflash0
    num-blocks = 512
    sector-length = 4096
    width = 1
    big-endian = 0
    id0 = 0
    id1 = 0
    id2 = 0
    id3 = 0
    name = "system.flash"
    irq 0
    mmio 00000000ffe00000/0000000000200000

The "info mtree" command lists:

memory
0000000000000000-7ffffffffffffffe (prio 0, RW): system
  [...]
  0000000060000000-00000000ffffffff (prio 0, RW): alias pci-hole @pci 0000000060000000-00000000ffffffff
  [...]
  00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash
[...]
pci <-------- referred to as "rom_memory" in the source (pci is enabled)
0000000000000000-7ffffffffffffffe (prio 0, RW): pci
  0000000000000000-7ffffffffffffffe (prio -2147483648, RW): pci-master-abort
  [...]
  00000000000e0000-00000000000fffff (prio 1, R-): isa-bios

For some reason, the range called "pci-master-abort" takes priority over
"isa-bios" (and/or "system.flash").

I wrote the attached debug patch (for v1.7.0-rc0). It produces quite a
bit of output, but grepping it for

  isa-bios|system\.flash|pci-master-abort|pci-hole

results in the following messages:

  adding subregion 'system.flash' to region 'system' at offset ffe00000
  subregion 'system.flash' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
  subregion 'system.flash' (prio: 0) wins over sibling subregion 'ram-below-4g' (prio: 0)

  adding subregion 'isa-bios' to region 'pci' at offset e0000
  subregion 'isa-bios' (prio: 1) inserted at tail
  subregion 'pc.rom' (prio: 1) wins over sibling subregion 'isa-bios' (prio: 1)

  adding subregion 'pci-master-abort' to region 'pci' at offset 0
  subregion 'pci-master-abort' (prio: -2147483648) loses to sibling subregion 'pc.rom' (prio: 1)
  subregion 'pci-master-abort' (prio: -2147483648) loses to sibling subregion 'isa-bios' (prio: 1)
  subregion 'pci-master-abort' (prio: -2147483648) inserted at tail

  adding subregion 'pci-hole' to region 'system' at offset 60000000
  warning: subregion collision 60000000/a0000000 (pci-hole) vs ffe00000/200000 (system.flash)
  subregion 'pci-hole' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
  subregion 'pci-hole' (prio: 0) wins over sibling subregion 'system.flash' (prio: 0)

  adding subregion 'pci-hole64' to region 'system' at offset 100000000
  subregion 'pci-hole64' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
  subregion 'pci-hole64' (prio: 0) wins over sibling subregion 'pci-hole' (prio: 0)
  subregion 'smram-region' (prio: 1) wins over sibling subregion 'pci-hole64' (prio: 0)
  warning: subregion collision fec00000/1000 (kvm-ioapic) vs 60000000/a0000000 (pci-hole)
  subregion 'kvm-ioapic' (prio: 0) wins over sibling subregion 'pci-hole64' (prio: 0)
  warning: subregion collision fed00000/400 (hpet) vs 60000000/a0000000 (pci-hole)

I think I know what's going on... There's even a warning above, printed
by original (albeit disabled) qemu source code.

The "pci-hole" subregion, which is an alias, takes priority over
"system.flash". And, unfortunately, "pci-hole" provides a window into
"pci-master-abort".

I think this should be fixable by raising the priority of "system.flash"
to 2048. This way the relationship between "system.flash" and any other
region will not change, except it's going to be reversed with
"pci-hole".

The 2nd attached patch seems to solve the problem for me. I'll resubmit
it as a standalone patch if it is deemed good.

With it in place, I can run OVMF just fine. And:

  @@ -28,170 +28,224 @@
   adding subregion 'pci-conf-data' to region 'io' at offset cfc
   subregion 'pci-conf-data' (prio: 0) wins over sibling subregion 'pci-conf-idx' (prio: 0)
   adding subregion 'pci-hole' to region 'system' at offset 60000000
  -warning: subregion collision 60000000/a0000000 (pci-hole) vs ffe00000/200000 (system.flash)
   subregion 'pci-hole' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
  -subregion 'pci-hole' (prio: 0) wins over sibling subregion 'system.flash' (prio: 0)
  +subregion 'pci-hole' (prio: 0) loses to sibling subregion 'system.flash' (prio: 2048)
  +subregion 'pci-hole' (prio: 0) wins over sibling subregion 'ram-below-4g' (prio: 0)

According to the debug patch, the flash region starts to win over quite
a few unrelated other regions as well. But in practice I could see no
adverse effects -- the priority matters only when the addresses actually
overlap, and "system.flash" should not overlap with anything but
"pci-hole".

I'm attaching the following files as well:
- the "info mtree" output before and after the patch,
- the full output of the debug patch (before and after).

Thanks,
Laszlo

[-- Attachment #2: 0001-debug-messages-for-memory_region_add_subregion_commo.patch --]
[-- Type: text/plain, Size: 2485 bytes --]

>From 44854609c3134ec174593a890dfb1a9340b714f7 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 7 Nov 2013 21:19:07 +0100
Subject: [PATCH 1/2] debug messages for memory_region_add_subregion_common()

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 memory.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 28f6449..5e60a6c 100644
--- a/memory.c
+++ b/memory.c
@@ -1427,6 +1427,10 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
     memory_region_ref(subregion);
     subregion->parent = mr;
     subregion->addr = offset;
+
+    fprintf(stderr, "adding subregion '%s' to region '%s' at offset %llx\n",
+            subregion->name, mr->name, (unsigned long long)offset);
+
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->may_overlap || other->may_overlap) {
             continue;
@@ -1437,8 +1441,8 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
                          int128_make64(other->addr))) {
             continue;
         }
-#if 0
-        printf("warning: subregion collision %llx/%llx (%s) "
+#if 1
+        fprintf(stderr, "warning: subregion collision %llx/%llx (%s) "
                "vs %llx/%llx (%s)\n",
                (unsigned long long)offset,
                (unsigned long long)int128_get64(subregion->size),
@@ -1448,12 +1452,21 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
                other->name);
 #endif
     }
+
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
+            fprintf(stderr, "subregion '%s' (prio: %d) wins over sibling "
+                    "subregion '%s' (prio: %d)\n", subregion->name,
+                    subregion->priority, other->name, other->priority);
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
             goto done;
         }
+        fprintf(stderr, "subregion '%s' (prio: %d) loses to sibling "
+                "subregion '%s' (prio: %d)\n", subregion->name,
+                subregion->priority, other->name, other->priority);
     }
+    fprintf(stderr, "subregion '%s' (prio: %d) inserted at tail\n",
+            subregion->name, subregion->priority);
     QTAILQ_INSERT_TAIL(&mr->subregions, subregion, subregions_link);
 done:
     memory_region_update_pending |= mr->enabled && subregion->enabled;
-- 
1.8.3.1


[-- Attachment #3: 0002-pflash_cfi01-flash-memory-should-take-priority-over-.patch --]
[-- Type: text/plain, Size: 1746 bytes --]

>From c3f1150bb3b0618e08ff21bf302363b36aaea863 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Thu, 7 Nov 2013 21:36:59 +0100
Subject: [PATCH 2/2] pflash_cfi01: flash memory should take priority over
 "pci-hole"

After commit a53ae8e9 ("hw/pci: partially handle pci master abort"), the
"pci-hole" alias provides a window into the "pci-master-abort" memory
range. This range throws away writes and returns 0xFFFFFFFF for all reads.

Unfortunately, "pci-hole" has been in conflict with (and incidentally
taking priority over) "system.flash" for some time. This problem is
exposed now that commit a53ae8e9 made "pci-master-abort" appear in
"pci-hole" -- the flash has become invisible.

The problem can be triggered by running OVMF from flash (with the -pflash
option instead of -bios) on KVM 3.7+.

Fix it by raising the priority of flash, so that its relationship with
"pci-hole" is reversed. Its relationship with some other overlapping
ranges seems to change as well, but in my testing this had no adverse
effects.

The neighbor "pflash_cfi02" is not touched (although the fix should apply
the same), due to lack of a test case.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/block/pflash_cfi01.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 018a967..ab39ac2 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -765,7 +765,7 @@ pflash_t *pflash_cfi01_register(hwaddr base,
     qdev_prop_set_string(dev, "name", name);
     qdev_init_nofail(dev);
 
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(dev), 0, base, 2048);
     return CFI_PFLASH01(dev);
 }
 
-- 
1.8.3.1


[-- Attachment #4: info-mtree-unpatched.txt.gz --]
[-- Type: application/x-gzip, Size: 970 bytes --]

[-- Attachment #5: info-mtree-patched.txt.gz --]
[-- Type: application/x-gzip, Size: 1396 bytes --]

[-- Attachment #6: region-prios-unpatched.txt.gz --]
[-- Type: application/x-gzip, Size: 3259 bytes --]

[-- Attachment #7: region-prios-patched.txt.gz --]
[-- Type: application/x-gzip, Size: 3377 bytes --]

Re: [Qemu-devel] [edk2] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 07/11/2013 22:12, Laszlo Ersek ha scritto:
> 0000000000000000-7ffffffffffffffe (prio 0, RW): system
>   [...]
>   0000000060000000-00000000ffffffff (prio 0, RW): alias pci-hole @pci 0000000060000000-00000000ffffffff
>   [...]
>   00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash
> [...]

Priorities are not "transitive" across aliases; once you use an alias to
map a region, the alias's priority counts, not the target region's
priority.  So the INT_MIN priority for pci-master-abort counts *within
the alias*, but the choice between pci-hole and system.flash is only
affected by the priorities of pci-hole and system.flash.

You could give a smaller priority (-1 or INT_MIN) to pci-hole and just
let it occupy the whole address space, from 0 to UINT64_MAX.  Or perhaps
the pci-hole alias is too large and it should end before the system
flash area.  Both solutions should work.

Paolo

Re: [Qemu-devel] [edk2] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Laszlo Ersek]

On 11/07/13 22:21, Paolo Bonzini wrote:
> Il 07/11/2013 22:12, Laszlo Ersek ha scritto:
>> 0000000000000000-7ffffffffffffffe (prio 0, RW): system
>>   [...]
>>   0000000060000000-00000000ffffffff (prio 0, RW): alias pci-hole @pci 0000000060000000-00000000ffffffff
>>   [...]
>>   00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash
>> [...]
> 
> Priorities are not "transitive" across aliases; once you use an alias to
> map a region, the alias's priority counts, not the target region's
> priority.  So the INT_MIN priority for pci-master-abort counts *within
> the alias*, but the choice between pci-hole and system.flash is only
> affected by the priorities of pci-hole and system.flash.

Right. It's also documented in docs/memory.txt -- Peter's recent
addition I think?

> You could give a smaller priority (-1 or INT_MIN) to pci-hole and just
> let it occupy the whole address space, from 0 to UINT64_MAX.  Or perhaps
> the pci-hole alias is too large and it should end before the system
> flash area.  Both solutions should work.

I did reorder pci-hole and system.flash, but rather than lowering
pci-hole, I raised system.flash. I have no preference.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Thu, 2013-11-07 at 22:12 +0100, Laszlo Ersek wrote:
> This is a QEMU bug report, only disguised as an edk2-devel followup.
> 
> The problem in a nutshell is that the OVMF binary, placed into pflash
> (read-only KVM memslot) used to run in qemu-1.6, but it fails to start
> in qemu-1.7. The OVMF reset vector reads as 0xFF bytes.
> 
> (Jordan and myself started writing these emails in parallel.)
> 
> On 11/07/13 21:27, Jordan Justen wrote:
> > On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum
> > <marcel.a@redhat.com> wrote:
> >> The commit:
> >>
> >> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
> >> Author: Marcel Apfelbaum <marcel.a@redhat.com>
> >> Date:   Mon Sep 16 11:21:16 2013 +0300
> >>
> >>     hw/pci: partially handle pci master abort
> >>
> >> introduced a regression on make check:
> >
> > Laszlo pointed out that my OVMF flash support series was not working
> > with QEMU master. It was working with QEMU 1.6.0. I then bisected the
> > issue to this commit. It seems this commit regresses -pflash support
> > for both KVM and non-KVM modes.
> >
> > Can you reproduce the issue this with command?
> > x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin
> > (with or without adding -enable-kvm)
> >
> > I tried adding this patch ("exec: fix regression by making
> > system-memory region UINT64_MAX size") and it did not help the -pflash
> > regression.
> 
> 
> From the edk2-devel discussion:
> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4787/focus=4812>
> 
> On 11/07/13 19:07, Laszlo Ersek wrote:
> > On 11/07/13 17:28, Laszlo Ersek wrote:
> >> On 11/06/13 23:29, Jordan Justen wrote:
> >>> https://github.com/jljusten/edk2.git ovmf-nvvars-v2
> >>>
> >>> This series implements support for QEMU's emulated
> >>> system flash.
> >>>
> >>> This allows for persistent UEFI non-volatile variables.
> >>>
> >>> Previously we attempted to emulate non-volatile
> >>> variables in a few ways, but each of them would fail
> >>> in particular situations.
> >>>
> >>> To support flash based variable storage, we:
> >>>  * Reserve space in the firmware device image
> >>>  * Add a new flash firmware volume block driver
> >>>  * Disable EmuVariableFvbRuntimeDxe (at runtime) if QEMU
> >>>    flash is available.
> >>>
> >>> To use:
> >>>  * QEMU version 1.1 or newer is required without KVM
> >>>  * KVM support requires Linux 3.7 and QEMU 1.6
> >>>  * Run QEMU with -pflash OVMF.fd instead of -L or -bios
> >>>    or use OvmfPkg/build.sh --enable-flash qemu ...
> >>>  * If QEMU is 1.6 or newer, then OvmfPkg/build.sh will
> >>>    automatically enable flash when running QEMU, so in
> >>>    that case --enable-flash is not required.
> >>>
> >>> See also:
> >>>  * http://wiki.qemu.org/Features/PC_System_Flash
> >>>
> >>> v2:
> >>>  * Replace
> >>>      "OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions"
> >>>    with
> >>>      "OvmfPkg/AcpiPlatformDxe/Qemu: Decrease upper limit for PCI window 32"
> >>>  * Separate portions of
> >>>      "OvmfPkg/build.sh: Support --enable-flash switch"
> >>>    out into a new patch
> >>>      "OvmfPkg/build.sh: Enable flash for QEMU >= 1.6"
> >>>  * Add "OvmfPkg/README: Add information about OVMF flash layout"
> >>>  * Update "OvmfPkg: Add NV Variable storage within FD" to also change the
> >>>    size of PcdVariableStoreSize
> >>>  * Update commit messages on a couple patches for better clarity
> >>
> >> Tested in the following configurations:
> >>
> >> (1) RHEL-6 host (no KVM support, no qemu support -- that is,
> >> regression test): RHEL-6, Fedora 19, Windows 2008 R2 (needs CSM),
> >> Windows 2012 R2 boot tests work OK.
> >>
> >> (2) 3.10-based host kernel, qemu v1.7.0-rc0, Xeon W3550 host CPU:
> >> Unfortunately qemu dies with the following KVM trace:
> >>
> >>   KVM internal error. Suberror: 1
> >>   emulation failure
> >>   EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000623
> >>   ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
> >>   EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> >>   ES =0000 00000000 0000ffff 00009300
> >>   CS =f000 ffff0000 0000ffff 00009b00
> >>   SS =0000 00000000 0000ffff 00009300
> >>   DS =0000 00000000 0000ffff 00009300
> >>   FS =0000 00000000 0000ffff 00009300
> >>   GS =0000 00000000 0000ffff 00009300
> >>   LDT=0000 00000000 0000ffff 00008200
> >>   TR =0000 00000000 0000ffff 00008b00
> >>   GDT=     00000000 0000ffff
> >>   IDT=     00000000 0000ffff
> >>   CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
> >>   DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> >>   DR6=00000000ffff0ff0 DR7=0000000000000400
> >>   EFER=0000000000000000
> >>   Code=ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff <ff> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> >>
> >> I'm quite unsure, but the CS:IP value seems to point at the reset
> >> vector, no? However, the Code=... log only shows 0xFF bytes.
> >>
> >> (3) 3.10.17 host kernel, qemu v1.7.0-rc0, Athlon II X2 B22 host CPU:
> >> almost identical KVM error, with the following difference:
> >>
> >>   --- vmx 2013-11-07 17:23:45.612973935 +0100
> >>   +++ svm 2013-11-07 17:24:17.237973059 +0100
> >>   @@ -1,6 +1,6 @@
> >>    KVM internal error. Suberror: 1
> >>    emulation failure
> >>   -EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000623
> >>   +EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000663
> >>    ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
> >>    EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> >>    ES =0000 00000000 0000ffff 00009300
> >>
> >> Any ideas?
> >
> > I.
> > This is a QEMU regression (somewhere between v1.6.0 and v1.7.0-rc0),
> > I'll have to bisect it.
> 
> This bug is "caused" by the following commit:
> 
>   a53ae8e934cd54686875b5bcfc2f434244ee55d6 is the first bad commit
>   commit a53ae8e934cd54686875b5bcfc2f434244ee55d6
>   Author: Marcel Apfelbaum <marcel.a@redhat.com>
>   Date:   Mon Sep 16 11:21:16 2013 +0300
> 
>       hw/pci: partially handle pci master abort
> 
>       A MemoryRegion with negative priority was created and
>       it spans over all the pci address space.
>       It "intercepts" the accesses to unassigned pci
>       address space and will follow the pci spec:
>        1. returns -1 on read
>        2. does nothing on write
> 
>       Note: setting the RECEIVED MASTER ABORT bit in the STATUS register
>             of the device that initiated the transaction will be
>             implemented in another series
> 
>       Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>       Acked-by: Michael S. Tsirkin <mst@redhat.com>
>       Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> The patch was posted as patch 3/3 of the series
> 
>   [Qemu-devel] [PATCH v5 0/3] pci: partially implement master abort
>   protocol
> 
>   http://thread.gmane.org/gmane.comp.emulators.qemu/233798/focus=233801
> 
> Basically, the series adds a "background" memory region that covers all
> "unassigned" PCI addresses, and patch 3/3 specifically makes sure that
> writes to this region are dropped, and reads all return -1 (0xFFFFFFFF).
> 
> The read implementation (master_abort_mem_read(), -1) is consistent with
> the KVM dump in the quoted part above.
> 
> For some reason, this "background" region pushes into the "foreground"
> when it comes to the pflash region just below 4GB (in guest-phys address
> space).
> 
> Or, hm, supposing we start to run in real mode at FFFF:0000, the problem
> could be with the "isa-bios" region too.
> 
> So I think we have a bug in qemu, which is likely one of the three
> below:
> 
> (1) This commit is wrong. Or,
> (2) the pflash implementation is wrong, because it doesn't register a
>     memory region (with appropriate priority) that would catch the
>     access.
> (3) Both this commit and the pflash implementations are right, but this
>     is an unusual situation that the memory infrastructure doesn't
>     handle well.
> 
> (I doubt that the problem is in KVM.)
> 
> When the bug hits, the "info qtree" command has the following to say
> about the flash device:
> 
>   dev: cfi.pflash01, id ""
>     drive = pflash0
>     num-blocks = 512
>     sector-length = 4096
>     width = 1
>     big-endian = 0
>     id0 = 0
>     id1 = 0
>     id2 = 0
>     id3 = 0
>     name = "system.flash"
>     irq 0
>     mmio 00000000ffe00000/0000000000200000
> 
> The "info mtree" command lists:
> 
> memory
> 0000000000000000-7ffffffffffffffe (prio 0, RW): system
>   [...]
>   0000000060000000-00000000ffffffff (prio 0, RW): alias pci-hole @pci 0000000060000000-00000000ffffffff
>   [...]
>   00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash
> [...]
> pci <-------- referred to as "rom_memory" in the source (pci is enabled)
> 0000000000000000-7ffffffffffffffe (prio 0, RW): pci
>   0000000000000000-7ffffffffffffffe (prio -2147483648, RW): pci-master-abort
>   [...]
>   00000000000e0000-00000000000fffff (prio 1, R-): isa-bios
> 
> For some reason, the range called "pci-master-abort" takes priority over
> "isa-bios" (and/or "system.flash").
> 
> I wrote the attached debug patch (for v1.7.0-rc0). It produces quite a
> bit of output, but grepping it for
> 
>   isa-bios|system\.flash|pci-master-abort|pci-hole
> 
> results in the following messages:
> 
>   adding subregion 'system.flash' to region 'system' at offset ffe00000
>   subregion 'system.flash' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
>   subregion 'system.flash' (prio: 0) wins over sibling subregion 'ram-below-4g' (prio: 0)
> 
>   adding subregion 'isa-bios' to region 'pci' at offset e0000
>   subregion 'isa-bios' (prio: 1) inserted at tail
>   subregion 'pc.rom' (prio: 1) wins over sibling subregion 'isa-bios' (prio: 1)
> 
>   adding subregion 'pci-master-abort' to region 'pci' at offset 0
>   subregion 'pci-master-abort' (prio: -2147483648) loses to sibling subregion 'pc.rom' (prio: 1)
>   subregion 'pci-master-abort' (prio: -2147483648) loses to sibling subregion 'isa-bios' (prio: 1)
>   subregion 'pci-master-abort' (prio: -2147483648) inserted at tail
> 
>   adding subregion 'pci-hole' to region 'system' at offset 60000000
>   warning: subregion collision 60000000/a0000000 (pci-hole) vs ffe00000/200000 (system.flash)
Thank you Laszlo for the detailed info!
I think the problem is right above. Why pci-hole and system.flash collide?
IMHO we should not play with priorities here, better solve the collision.

Thanks,
Marcel 

>   subregion 'pci-hole' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
>   subregion 'pci-hole' (prio: 0) wins over sibling subregion 'system.flash' (prio: 0)
> 
>   adding subregion 'pci-hole64' to region 'system' at offset 100000000
>   subregion 'pci-hole64' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
>   subregion 'pci-hole64' (prio: 0) wins over sibling subregion 'pci-hole' (prio: 0)
>   subregion 'smram-region' (prio: 1) wins over sibling subregion 'pci-hole64' (prio: 0)
>   warning: subregion collision fec00000/1000 (kvm-ioapic) vs 60000000/a0000000 (pci-hole)
>   subregion 'kvm-ioapic' (prio: 0) wins over sibling subregion 'pci-hole64' (prio: 0)
>   warning: subregion collision fed00000/400 (hpet) vs 60000000/a0000000 (pci-hole)
> 
> I think I know what's going on... There's even a warning above, printed
> by original (albeit disabled) qemu source code.
> 
> The "pci-hole" subregion, which is an alias, takes priority over
> "system.flash". And, unfortunately, "pci-hole" provides a window into
> "pci-master-abort".
> 
> I think this should be fixable by raising the priority of "system.flash"
> to 2048. This way the relationship between "system.flash" and any other
> region will not change, except it's going to be reversed with
> "pci-hole".
> 
> The 2nd attached patch seems to solve the problem for me. I'll resubmit
> it as a standalone patch if it is deemed good.
> 
> With it in place, I can run OVMF just fine. And:
> 
>   @@ -28,170 +28,224 @@
>    adding subregion 'pci-conf-data' to region 'io' at offset cfc
>    subregion 'pci-conf-data' (prio: 0) wins over sibling subregion 'pci-conf-idx' (prio: 0)
>    adding subregion 'pci-hole' to region 'system' at offset 60000000
>   -warning: subregion collision 60000000/a0000000 (pci-hole) vs ffe00000/200000 (system.flash)
>    subregion 'pci-hole' (prio: 0) loses to sibling subregion 'icc-apic-container' (prio: 4096)
>   -subregion 'pci-hole' (prio: 0) wins over sibling subregion 'system.flash' (prio: 0)
>   +subregion 'pci-hole' (prio: 0) loses to sibling subregion 'system.flash' (prio: 2048)
>   +subregion 'pci-hole' (prio: 0) wins over sibling subregion 'ram-below-4g' (prio: 0)
> 
> According to the debug patch, the flash region starts to win over quite
> a few unrelated other regions as well. But in practice I could see no
> adverse effects -- the priority matters only when the addresses actually
> overlap, and "system.flash" should not overlap with anything but
> "pci-hole".
> 
> I'm attaching the following files as well:
> - the "info mtree" output before and after the patch,
> - the full output of the debug patch (before and after).
> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 07/11/2013 22:24, Marcel Apfelbaum ha scritto:
> Thank you Laszlo for the detailed info!
> I think the problem is right above. Why pci-hole and system.flash collide?
> IMHO we should not play with priorities here, better solve the collision.

We need to audit all the other boards that support PCI...  I'll take a
look tomorrow since you guys are off.

Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Thu, 2013-11-07 at 22:31 +0100, Paolo Bonzini wrote:
> Il 07/11/2013 22:24, Marcel Apfelbaum ha scritto:
> > Thank you Laszlo for the detailed info!
> > I think the problem is right above. Why pci-hole and system.flash collide?
> > IMHO we should not play with priorities here, better solve the collision.
> 
> We need to audit all the other boards that support PCI...  I'll take a
> look tomorrow since you guys are off.
Thanks Paolo,
Let me just point out what I know (or I think I know):
1. Not all architectures have the behavior: "Address space that is not RAM(and friends) is for sure PCI".
   Only x86 behaves like this (I think). That means that you cannot have a 64bit wide pci-hole
   with lower priority that catches all accesses that are not for RAM(and firends).
2. If the above is right, and making pci-hole 64 bit wide is not an option,
   playing with pci-holes/other-region priorities it would be just wrong,
   it would be only to "fight" with the locality of the memory region's priority.

That being said, I am looking forward for your findings.
Thanks!
Marcel
  
> 
> Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Peter Maydell]

On 7 November 2013 21:38, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> Thanks Paolo,
> Let me just point out what I know (or I think I know):
> 1. Not all architectures have the behavior: "Address space that is not RAM(and friends) is for sure PCI".
>    Only x86 behaves like this (I think).

More specifically, the x86 pc behaves like this. Other
x86 based systems could in theory behave differently
(not that we actually model any, I think).

> That means that you cannot have a 64bit wide pci-hole
>    with lower priority that catches all accesses that are not for RAM(and firends).

...but this conclusion is wrong, because the pci-hole
region is created by the pc model. So we should create
it at the correct priority and size to give the behaviour
relative to other devices in the pc model that is required
(ie that the hardware has). This doesn't affect any other
target architecture or board.

That said, I don't know enough about the PC to know what
the exact details of the pci-hole are, so I'm not making a
statement about what the correct model is. I'm just saying
that what you do with the pci-hole and the container it lives
in and the other devices in that container is not going to
change the behaviour of any other target board.

> 2. If the above is right, and making pci-hole 64 bit wide is not an option,
>    playing with pci-holes/other-region priorities it would be just wrong,
>    it would be only to "fight" with the locality of the memory region's priority.

I have no idea what you mean by "fighting" here. MR priorities
apply only within a specific container region[*], to set which of
that container's children appears 'above' another. They're
totally local to the container (which in this case is part of the
PC model, not the generic PCI code) and so the PC model
can freely set them to whatever makes most sense.

[*] if you didn't already know this, see the recently committed
updates to doc/memory.txt for a more detailed explanation.

-- PMM

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Thu, 2013-11-07 at 21:51 +0000, Peter Maydell wrote:
> On 7 November 2013 21:38, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > Thanks Paolo,
> > Let me just point out what I know (or I think I know):
> > 1. Not all architectures have the behavior: "Address space that is not RAM(and friends) is for sure PCI".
> >    Only x86 behaves like this (I think).
> 
> More specifically, the x86 pc behaves like this. Other
> x86 based systems could in theory behave differently
> (not that we actually model any, I think).
> 
> > That means that you cannot have a 64bit wide pci-hole
> >    with lower priority that catches all accesses that are not for RAM(and firends).
> 
> ...but this conclusion is wrong, because the pci-hole
> region is created by the pc model. So we should create
Yes... It think you are right. I was thinking for some
reason of master-abort region belonging to the the pci bus
that is not specific to x86 pc...

> it at the correct priority and size to give the behaviour
> relative to other devices in the pc model that is required
> (ie that the hardware has). This doesn't affect any other
> target architecture or board.
> 
> That said, I don't know enough about the PC to know what
> the exact details of the pci-hole are, so I'm not making a
> statement about what the correct model is. I'm just saying
> that what you do with the pci-hole and the container it lives
> in and the other devices in that container is not going to
> change the behaviour of any other target board.
> 
> > 2. If the above is right, and making pci-hole 64 bit wide is not an option,
> >    playing with pci-holes/other-region priorities it would be just wrong,
> >    it would be only to "fight" with the locality of the memory region's priority.
> 
> I have no idea what you mean by "fighting" here. MR priorities
By "fighting" I was referring exactly to the fact that because 
priorities are container specific we need to use them twice to
get the wanted behavior (master abort being with the lowest priority)
1. master-abort region priority for pci-address-space
2. pci-hole priority for system-address-space
But this is as designed...

Thanks,
Marcel

> apply only within a specific container region[*], to set which of
> that container's children appears 'above' another. They're
> totally local to the container (which in this case is part of the
> PC model, not the generic PCI code) and so the PC model
> can freely set them to whatever makes most sense.
> 
> [*] if you didn't already know this, see the recently committed
> updates to doc/memory.txt for a more detailed explanation.
> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 07/11/2013 22:51, Peter Maydell ha scritto:
>> > 1. Not all architectures have the behavior: "Address space that is not RAM(and friends)
>> >     is for sure PCI". Only x86 behaves like this (I think).
> 
> More specifically, the x86 pc behaves like this. Other
> x86 based systems could in theory behave differently
> (not that we actually model any, I think).

After Marcel's patch, we have changed behavior for at least all boards
that pass get_system_memory() to pci_register_bus or pci_bus_new:

* mips/gt64xxx_pci.c

* pci-host/bonito.c

* pci-host/ppce500.c

* ppc/ppc4xx_pci.c

* sh4/sh_pci.c

These now will not go anymore through unassigned_mem_ops, which is a
behavioral change for MIPS boards (gt64xxx_pci and bonito) at least.

Furthermore, the default behavior of the memory API _is_ read
all-ones/ignore writes, so I'm not sure what's the benefit of adding a
separate region for master abort...

Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Peter Maydell]

On 8 November 2013 08:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/11/2013 22:51, Peter Maydell ha scritto:
>>> > 1. Not all architectures have the behavior: "Address space that is not RAM(and friends)
>>> >     is for sure PCI". Only x86 behaves like this (I think).
>>
>> More specifically, the x86 pc behaves like this. Other
>> x86 based systems could in theory behave differently
>> (not that we actually model any, I think).
>
> After Marcel's patch, we have changed behavior for at least all boards
> that pass get_system_memory() to pci_register_bus or pci_bus_new:
>
> * mips/gt64xxx_pci.c
>
> * pci-host/bonito.c
>
> * pci-host/ppce500.c
>
> * ppc/ppc4xx_pci.c
>
> * sh4/sh_pci.c

Oh, right. Ideally those boards should not do that (I fixed
the versatile pci controller not to do that a while back) but
it's a long standing behaviour so it would be better had we
not broken it.

I think it should in general be fixable by just having those
pci controllers create an empty memory region to pass in,
like versatile:

    memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
    memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);

    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
                        &s->pci_mem_space, &s->pci_io_space,
                        PCI_DEVFN(11, 0), TYPE_PCI_BUS);

That doesn't affect where PCI DMA goes. It might also
require mapping an alias into that region at wherever
the pci hole is on those boards.

Kind of awkward to do and test at this point in the release cycle though.

> These now will not go anymore through unassigned_mem_ops, which is a
> behavioral change for MIPS boards (gt64xxx_pci and bonito) at least.
>
> Furthermore, the default behavior of the memory API _is_ read
> all-ones/ignore writes, so I'm not sure what's the benefit of adding a
> separate region for master abort...

It gives you a place set the appropriate PCI controller or device
register status bits on abort by a PCI device access, IIRC.

-- PMM

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 08/11/2013 11:44, Peter Maydell ha scritto:
> On 8 November 2013 08:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 07/11/2013 22:51, Peter Maydell ha scritto:
>>>>> 1. Not all architectures have the behavior: "Address space that is not RAM(and friends)
>>>>>     is for sure PCI". Only x86 behaves like this (I think).
>>>
>>> More specifically, the x86 pc behaves like this. Other
>>> x86 based systems could in theory behave differently
>>> (not that we actually model any, I think).
>>
>> After Marcel's patch, we have changed behavior for at least all boards
>> that pass get_system_memory() to pci_register_bus or pci_bus_new:
>>
>> * mips/gt64xxx_pci.c
>>
>> * pci-host/bonito.c
>>
>> * pci-host/ppce500.c
>>
>> * ppc/ppc4xx_pci.c
>>
>> * sh4/sh_pci.c
> 
> Oh, right. Ideally those boards should not do that (I fixed
> the versatile pci controller not to do that a while back) but
> it's a long standing behaviour so it would be better had we
> not broken it.
> 
> I think it should in general be fixable by just having those
> pci controllers create an empty memory region to pass in,
> like versatile:
> 
>     memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
>     memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
> 
>     pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
>                         &s->pci_mem_space, &s->pci_io_space,
>                         PCI_DEVFN(11, 0), TYPE_PCI_BUS);
> 
> That doesn't affect where PCI DMA goes. It might also
> require mapping an alias into that region at wherever
> the pci hole is on those boards.
> 
> Kind of awkward to do and test at this point in the release cycle though.

Yes, for now we should just revert the patch.

BTW I tried optimizing MMIO dispatch and managed to save ~30 cycles so I
don't think we should be afraid of increasing the depth of the radix
tree.  All stuff for 1.8 though.

Paolo

Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Fri, 2013-11-08 at 10:44 +0000, Peter Maydell wrote:
> On 8 November 2013 08:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 07/11/2013 22:51, Peter Maydell ha scritto:
> >>> > 1. Not all architectures have the behavior: "Address space that is not RAM(and friends)
> >>> >     is for sure PCI". Only x86 behaves like this (I think).
> >>
> >> More specifically, the x86 pc behaves like this. Other
> >> x86 based systems could in theory behave differently
> >> (not that we actually model any, I think).
> >
> > After Marcel's patch, we have changed behavior for at least all boards
> > that pass get_system_memory() to pci_register_bus or pci_bus_new:
> >
> > * mips/gt64xxx_pci.c
> >
> > * pci-host/bonito.c
> >
> > * pci-host/ppce500.c
> >
> > * ppc/ppc4xx_pci.c
> >
> > * sh4/sh_pci.c
> 
> Oh, right. Ideally those boards should not do that (I fixed
> the versatile pci controller not to do that a while back) but
> it's a long standing behaviour so it would be better had we
> not broken it.
> 
> I think it should in general be fixable by just having those
> pci controllers create an empty memory region to pass in,
> like versatile:
> 
>     memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);
>     memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
> 
>     pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
>                         &s->pci_mem_space, &s->pci_io_space,
>                         PCI_DEVFN(11, 0), TYPE_PCI_BUS);
> 
> That doesn't affect where PCI DMA goes. It might also
> require mapping an alias into that region at wherever
> the pci hole is on those boards.
> 
> Kind of awkward to do and test at this point in the release cycle though.
> 
> > These now will not go anymore through unassigned_mem_ops, which is a
> > behavioral change for MIPS boards (gt64xxx_pci and bonito) at least.
> >
> > Furthermore, the default behavior of the memory API _is_ read
> > all-ones/ignore writes, so I'm not sure what's the benefit of adding a
> > separate region for master abort...
> 
> It gives you a place set the appropriate PCI controller or device
> register status bits on abort by a PCI device access, IIRC.

Right, thanks for pointing this out. This was indeed the patches direction.

But even the first patch has meaning by itself and not just a preparation.
As I mentioned in other mail in this thread "read all-ones/ignore writes"
is not implemented yet asbeacuse unassigned_mem_ops has no read/write methods
and valid.accepts returns false.

Thanks,
Marcel

> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Fri, 2013-11-08 at 09:05 +0100, Paolo Bonzini wrote:
> Il 07/11/2013 22:51, Peter Maydell ha scritto:
> >> > 1. Not all architectures have the behavior: "Address space that is not RAM(and friends)
> >> >     is for sure PCI". Only x86 behaves like this (I think).
> > 
> > More specifically, the x86 pc behaves like this. Other
> > x86 based systems could in theory behave differently
> > (not that we actually model any, I think).
> 
> After Marcel's patch, we have changed behavior for at least all boards
> that pass get_system_memory() to pci_register_bus or pci_bus_new:
> 
> * mips/gt64xxx_pci.c
> 
> * pci-host/bonito.c
> 
> * pci-host/ppce500.c
> 
> * ppc/ppc4xx_pci.c
> 
> * sh4/sh_pci.c
> 
> These now will not go anymore through unassigned_mem_ops, which is a
> behavioral change for MIPS boards (gt64xxx_pci and bonito) at least.
> 
> Furthermore, the default behavior of the memory API _is_ read
> all-ones/ignore writes, so I'm not sure what's the benefit of adding a
> separate region for master abort...
Actually, as I see, the default behavior of "system" memory region
is to use unassigned_mem_ops that has valid.accepts method returning
false (no read/write methods). I don't see that read all-ones/ignore
writes is implemented.
This was the main reason I submitted this patch. I had *no* clue that
it would impact so much the system...
I still think the patch is needed ans the guests will benefit from
more accurate PCI spec emulation.

Thanks,
Marcel

> 
> Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 08/11/2013 16:08, Marcel Apfelbaum ha scritto:
> Actually, as I see, the default behavior of "system" memory region
> is to use unassigned_mem_ops that has valid.accepts method returning
> false (no read/write methods). I don't see that read all-ones/ignore
> writes is implemented.

Right, it's read-all-zeroes (see unassigned_mem_read).  It was
read-all-ones for a brief time, then it got changed back to read-all-zeroes.

> This was the main reason I submitted this patch. I had *no* clue that
> it would impact so much the system...

Yeah, it's not your fault.  But it should have been held back until 1.8.
 Do you agree with reverting it?

> I still think the patch is needed ans the guests will benefit from
> more accurate PCI spec emulation.

Only buggy guests :) but yes, I agree it's a good thing to have.

Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Fri, 2013-11-08 at 17:12 +0100, Paolo Bonzini wrote:
> Il 08/11/2013 16:08, Marcel Apfelbaum ha scritto:
> > Actually, as I see, the default behavior of "system" memory region
> > is to use unassigned_mem_ops that has valid.accepts method returning
> > false (no read/write methods). I don't see that read all-ones/ignore
> > writes is implemented.
> 
> Right, it's read-all-zeroes (see unassigned_mem_read).  It was
> read-all-ones for a brief time, then it got changed back to read-all-zeroes.
I remember something, Jan's patches got reverted because the modification
was system wide and not only for pci address space.
> 
> > This was the main reason I submitted this patch. I had *no* clue that
> > it would impact so much the system...
> 
> Yeah, it's not your fault.  But it should have been held back until 1.8.
>  Do you agree with reverting it?
Given the actual impact on version's stability, I agree
it is the right thing to do.

> 
> > I still think the patch is needed ans the guests will benefit from
> > more accurate PCI spec emulation.
> 
> Only buggy guests :) but yes, I agree it's a good thing to have.
Yes, it may be a driver there making some a wrong decision and
"reading-all-ones" may give it a clue. (the same for writes)

Thanks,
Marcel

> 
> Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Laszlo Ersek]

On 11/07/13 22:24, Marcel Apfelbaum wrote:
> On Thu, 2013-11-07 at 22:12 +0100, Laszlo Ersek wrote:

>>   adding subregion 'pci-hole' to region 'system' at offset 60000000
>>   warning: subregion collision 60000000/a0000000 (pci-hole) vs ffe00000/200000 (system.flash)
> Thank you Laszlo for the detailed info!
> I think the problem is right above. Why pci-hole and system.flash collide?
> IMHO we should not play with priorities here, better solve the collision.

pc_init1()
  pc_memory_init()
    pc_system_firmware_init()
      pc_system_flash_init() <---- sets base address to
                                   0x100000000ULL - flash_size
        pflash_cfi01_register()
          sysbus_mmio_map()
            sysbus_mmio_map_common()
              memory_region_add_subregion()
  i440fx_init()
    memory_region_init_alias("pci-hole")

pc_init1() passes

    0x100000000ULL - below_4g_mem_size

to i440fx_init() as "pci_hole_size", which is then used as the size of
the "pci-hole" alias.

We should probably subtract the size of the flash from this, but I don't
know how to do that "elegantly". Yet another (output) parameter for
pc_memory_init()? Blech.

Or look up the end address of "system.flash" by name?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

On Thu, 2013-11-07 at 22:48 +0100, Laszlo Ersek wrote:
> On 11/07/13 22:24, Marcel Apfelbaum wrote:
> > On Thu, 2013-11-07 at 22:12 +0100, Laszlo Ersek wrote:
> 
> >>   adding subregion 'pci-hole' to region 'system' at offset 60000000
> >>   warning: subregion collision 60000000/a0000000 (pci-hole) vs ffe00000/200000 (system.flash)
> > Thank you Laszlo for the detailed info!
> > I think the problem is right above. Why pci-hole and system.flash collide?
> > IMHO we should not play with priorities here, better solve the collision.
> 
> pc_init1()
>   pc_memory_init()
>     pc_system_firmware_init()
>       pc_system_flash_init() <---- sets base address to
>                                    0x100000000ULL - flash_size
>         pflash_cfi01_register()
>           sysbus_mmio_map()
>             sysbus_mmio_map_common()
>               memory_region_add_subregion()
>   i440fx_init()
>     memory_region_init_alias("pci-hole")
> 
> pc_init1() passes
> 
>     0x100000000ULL - below_4g_mem_size
> 
> to i440fx_init() as "pci_hole_size", which is then used as the size of
> the "pci-hole" alias.
> 
> We should probably subtract the size of the flash from this, but I don't
> know how to do that "elegantly". Yet another (output) parameter for
> pc_memory_init()? Blech.
> 
> Or look up the end address of "system.flash" by name?
Both seems ugly to me...
As Peter Maydell pointed out, the pci-hole belongs to the pc model,
so we can actually change its priority without affecting other
architectures.
 
> 
> Thanks
> Laszlo
> 

[Qemu-devel] [PATCH 0/2] Re: exec: fix regression by making system-memory region UINT64_MAX size

[Laszlo Ersek]

On 11/07/13 22:24, Marcel Apfelbaum wrote:

> Why pci-hole and system.flash collide? IMHO we should not play with
> priorities here, better solve the collision.

What about this "beautiful" series? It produces

memory
0000000000000000-000fffffffffffff (prio 0, RW): system
  [...]
  0000000060000000-00000000ffdfffff (prio 0, RW): alias pci-hole @pci
  0000000060000000-00000000ffdfffff
  [...]
  00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash

and I can run OVMF with it. It also stays within i386/pc.

Re 2/2, note that "below_4g_mem_size" never exceeds 0xe0000000 in
pc_init1(), so the subtraction is safe.

Thanks
Laszlo

Laszlo Ersek (2):
  i386/pc: propagate flash size from pc_system_flash_init() to
    pc_init1()
  i386/pc_piix: the pci-hole should end where the system flash starts

 include/hw/i386/pc.h |  6 ++++--
 hw/i386/pc.c         |  5 +++--
 hw/i386/pc_piix.c    |  5 +++--
 hw/i386/pc_q35.c     |  3 ++-
 hw/i386/pc_sysfw.c   | 10 +++++++---
 5 files changed, 19 insertions(+), 10 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Laszlo Ersek]

... upwards through the following call chain:

  pc_init1() | pc_q35_init()
    pc_memory_init()
      pc_system_firmware_init()
        pc_system_flash_init()

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/i386/pc.h |  6 ++++--
 hw/i386/pc.c         |  5 +++--
 hw/i386/pc_piix.c    |  3 ++-
 hw/i386/pc_q35.c     |  3 ++-
 hw/i386/pc_sysfw.c   | 10 +++++++---
 5 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 03cc0ba..a9b938e 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -148,7 +148,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            ram_addr_t above_4g_mem_size,
                            MemoryRegion *rom_memory,
                            MemoryRegion **ram_memory,
-                           PcGuestInfo *guest_info);
+                           PcGuestInfo *guest_info,
+                           int64_t **flash_size);
 qemu_irq *pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
@@ -232,7 +233,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 
 /* pc_sysfw.c */
 void pc_system_firmware_init(MemoryRegion *rom_memory,
-                             bool isapc_ram_fw);
+                             bool isapc_ram_fw,
+                             int64_t **flash_size);
 
 /* pvpanic.c */
 void pvpanic_init(ISABus *bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 12c436e..3ec18aa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1152,7 +1152,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            ram_addr_t above_4g_mem_size,
                            MemoryRegion *rom_memory,
                            MemoryRegion **ram_memory,
-                           PcGuestInfo *guest_info)
+                           PcGuestInfo *guest_info,
+                           int64_t **flash_size)
 {
     int linux_boot, i;
     MemoryRegion *ram, *option_rom_mr;
@@ -1186,7 +1187,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
 
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
+    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw, flash_size);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4fdb7b6..6e2c027 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -89,6 +89,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
     DeviceState *icc_bridge;
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
+    int64_t flash_size = 0;
 
     if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
         fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
@@ -135,7 +136,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
                        args->kernel_filename, args->kernel_cmdline,
                        args->initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory, guest_info);
+                       rom_memory, &ram_memory, guest_info, &flash_size);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4c191d3..90f29e9 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -76,6 +76,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     PCIDevice *ahci;
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
+    int64_t flash_size = 0;
 
     if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
         fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
@@ -120,7 +121,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
                        args->kernel_filename, args->kernel_cmdline,
                        args->initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory, guest_info);
+                       rom_memory, &ram_memory, guest_info, &flash_size);
     }
 
     /* irq lines */
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index e917c83..0d05dec 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -73,7 +73,8 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
 }
 
 static void pc_system_flash_init(MemoryRegion *rom_memory,
-                                 DriveInfo *pflash_drv)
+                                 DriveInfo *pflash_drv,
+                                 int64_t **flash_size)
 {
     BlockDriverState *bdrv;
     int64_t size;
@@ -101,6 +102,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
     flash_mem = pflash_cfi01_get_memory(system_flash);
 
     pc_isa_bios_init(rom_memory, flash_mem, size);
+    *flash_size = size;
 }
 
 static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
@@ -162,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
                                 bios);
 }
 
-void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
+void pc_system_firmware_init(MemoryRegion *rom_memory,
+                             bool isapc_ram_fw,
+                             int64_t **flash_size)
 {
     DriveInfo *pflash_drv;
 
@@ -181,5 +185,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
         exit(1);
     }
 
-    pc_system_flash_init(rom_memory, pflash_drv);
+    pc_system_flash_init(rom_memory, pflash_drv, flash_size);
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Jordan Justen]

On Thu, Nov 7, 2013 at 2:23 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> ... upwards through the following call chain:
>
>   pc_init1() | pc_q35_init()
>     pc_memory_init()
>       pc_system_firmware_init()
>         pc_system_flash_init()
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/i386/pc.h |  6 ++++--
>  hw/i386/pc.c         |  5 +++--
>  hw/i386/pc_piix.c    |  3 ++-
>  hw/i386/pc_q35.c     |  3 ++-
>  hw/i386/pc_sysfw.c   | 10 +++++++---
>  5 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 03cc0ba..a9b938e 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -148,7 +148,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>                             ram_addr_t above_4g_mem_size,
>                             MemoryRegion *rom_memory,
>                             MemoryRegion **ram_memory,
> -                           PcGuestInfo *guest_info);
> +                           PcGuestInfo *guest_info,
> +                           int64_t **flash_size);

Do you want *flash_size here, and at various other points? After
changing that I could build, and it seemed to fix the symptom.

Also, should we call it firmware_size?

int64_t? :)

-Jordan

>  qemu_irq *pc_allocate_cpu_irq(void);
>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> @@ -232,7 +233,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>
>  /* pc_sysfw.c */
>  void pc_system_firmware_init(MemoryRegion *rom_memory,
> -                             bool isapc_ram_fw);
> +                             bool isapc_ram_fw,
> +                             int64_t **flash_size);
>
>  /* pvpanic.c */
>  void pvpanic_init(ISABus *bus);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 12c436e..3ec18aa 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1152,7 +1152,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>                             ram_addr_t above_4g_mem_size,
>                             MemoryRegion *rom_memory,
>                             MemoryRegion **ram_memory,
> -                           PcGuestInfo *guest_info)
> +                           PcGuestInfo *guest_info,
> +                           int64_t **flash_size)
>  {
>      int linux_boot, i;
>      MemoryRegion *ram, *option_rom_mr;
> @@ -1186,7 +1187,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>
>
>      /* Initialize PC system firmware */
> -    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
> +    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw, flash_size);
>
>      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>      memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 4fdb7b6..6e2c027 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -89,6 +89,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>      DeviceState *icc_bridge;
>      FWCfgState *fw_cfg = NULL;
>      PcGuestInfo *guest_info;
> +    int64_t flash_size = 0;
>
>      if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> @@ -135,7 +136,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>                         args->kernel_filename, args->kernel_cmdline,
>                         args->initrd_filename,
>                         below_4g_mem_size, above_4g_mem_size,
> -                       rom_memory, &ram_memory, guest_info);
> +                       rom_memory, &ram_memory, guest_info, &flash_size);
>      }
>
>      gsi_state = g_malloc0(sizeof(*gsi_state));
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 4c191d3..90f29e9 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -76,6 +76,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      PCIDevice *ahci;
>      DeviceState *icc_bridge;
>      PcGuestInfo *guest_info;
> +    int64_t flash_size = 0;
>
>      if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> @@ -120,7 +121,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>                         args->kernel_filename, args->kernel_cmdline,
>                         args->initrd_filename,
>                         below_4g_mem_size, above_4g_mem_size,
> -                       rom_memory, &ram_memory, guest_info);
> +                       rom_memory, &ram_memory, guest_info, &flash_size);
>      }
>
>      /* irq lines */
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index e917c83..0d05dec 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -73,7 +73,8 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>  }
>
>  static void pc_system_flash_init(MemoryRegion *rom_memory,
> -                                 DriveInfo *pflash_drv)
> +                                 DriveInfo *pflash_drv,
> +                                 int64_t **flash_size)
>  {
>      BlockDriverState *bdrv;
>      int64_t size;
> @@ -101,6 +102,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
>      flash_mem = pflash_cfi01_get_memory(system_flash);
>
>      pc_isa_bios_init(rom_memory, flash_mem, size);
> +    *flash_size = size;
>  }
>
>  static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
> @@ -162,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>                                  bios);
>  }
>
> -void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
> +void pc_system_firmware_init(MemoryRegion *rom_memory,
> +                             bool isapc_ram_fw,
> +                             int64_t **flash_size)
>  {
>      DriveInfo *pflash_drv;
>
> @@ -181,5 +185,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>          exit(1);
>      }
>
> -    pc_system_flash_init(rom_memory, pflash_drv);
> +    pc_system_flash_init(rom_memory, pflash_drv, flash_size);
>  }
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Laszlo Ersek]

On 11/08/13 07:09, Jordan Justen wrote:
> On Thu, Nov 7, 2013 at 2:23 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> ... upwards through the following call chain:
>>
>>   pc_init1() | pc_q35_init()
>>     pc_memory_init()
>>       pc_system_firmware_init()
>>         pc_system_flash_init()
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  include/hw/i386/pc.h |  6 ++++--
>>  hw/i386/pc.c         |  5 +++--
>>  hw/i386/pc_piix.c    |  3 ++-
>>  hw/i386/pc_q35.c     |  3 ++-
>>  hw/i386/pc_sysfw.c   | 10 +++++++---
>>  5 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 03cc0ba..a9b938e 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -148,7 +148,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>>                             ram_addr_t above_4g_mem_size,
>>                             MemoryRegion *rom_memory,
>>                             MemoryRegion **ram_memory,
>> -                           PcGuestInfo *guest_info);
>> +                           PcGuestInfo *guest_info,
>> +                           int64_t **flash_size);
> 
> Do you want *flash_size here, and at various other points? After
> changing that I could build, and it seemed to fix the symptom.

Aaargh. I hacked this up at night in a rush, trying to respond with it
"interactively" in the thread, and I was apparently confused by the
double indirection just a bit higher in the param list (at ram_memory).
What a horrible mistake. Of course you're right.

> Also, should we call it firmware_size?

No preference, I don't mind :)

> int64_t? :)

Heh, yes, I did cringe when I wrote that, but if you check the
bottom-most function, where the assignment happens,
pc_system_flash_init(), it declares the local "size" variable as
int64_t. I've given up on arguing for sensible unsigned types so I just
went with the flow.

(Hm, actually, now I think the int64_t caused me so much pain and
suffering that I can justifiedly blame my fsckup with the pointers on
it! :))

Anyway, I think Paolo wants to revert Marcel's patch for 1.7. I'm not
sure about the direction for 1.8; the pci-master-abort change has many
more tentacles than I anticipated.

I expect OVMF (incl. your new flash series) will run fine on v1.7.0 once
it's released (but we should certainly test it soon after), so neither
of my qemu patches (prio changing or resizing) should be necessary.

Thanks!
Laszlo

> 
> -Jordan
> 
>>  qemu_irq *pc_allocate_cpu_irq(void);
>>  DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>> @@ -232,7 +233,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>>
>>  /* pc_sysfw.c */
>>  void pc_system_firmware_init(MemoryRegion *rom_memory,
>> -                             bool isapc_ram_fw);
>> +                             bool isapc_ram_fw,
>> +                             int64_t **flash_size);
>>
>>  /* pvpanic.c */
>>  void pvpanic_init(ISABus *bus);
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 12c436e..3ec18aa 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1152,7 +1152,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>>                             ram_addr_t above_4g_mem_size,
>>                             MemoryRegion *rom_memory,
>>                             MemoryRegion **ram_memory,
>> -                           PcGuestInfo *guest_info)
>> +                           PcGuestInfo *guest_info,
>> +                           int64_t **flash_size)
>>  {
>>      int linux_boot, i;
>>      MemoryRegion *ram, *option_rom_mr;
>> @@ -1186,7 +1187,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
>>
>>
>>      /* Initialize PC system firmware */
>> -    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
>> +    pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw, flash_size);
>>
>>      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
>>      memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 4fdb7b6..6e2c027 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -89,6 +89,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>      DeviceState *icc_bridge;
>>      FWCfgState *fw_cfg = NULL;
>>      PcGuestInfo *guest_info;
>> +    int64_t flash_size = 0;
>>
>>      if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
>>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>> @@ -135,7 +136,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>                         args->kernel_filename, args->kernel_cmdline,
>>                         args->initrd_filename,
>>                         below_4g_mem_size, above_4g_mem_size,
>> -                       rom_memory, &ram_memory, guest_info);
>> +                       rom_memory, &ram_memory, guest_info, &flash_size);
>>      }
>>
>>      gsi_state = g_malloc0(sizeof(*gsi_state));
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 4c191d3..90f29e9 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -76,6 +76,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>      PCIDevice *ahci;
>>      DeviceState *icc_bridge;
>>      PcGuestInfo *guest_info;
>> +    int64_t flash_size = 0;
>>
>>      if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
>>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>> @@ -120,7 +121,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>                         args->kernel_filename, args->kernel_cmdline,
>>                         args->initrd_filename,
>>                         below_4g_mem_size, above_4g_mem_size,
>> -                       rom_memory, &ram_memory, guest_info);
>> +                       rom_memory, &ram_memory, guest_info, &flash_size);
>>      }
>>
>>      /* irq lines */
>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>> index e917c83..0d05dec 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -73,7 +73,8 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>  }
>>
>>  static void pc_system_flash_init(MemoryRegion *rom_memory,
>> -                                 DriveInfo *pflash_drv)
>> +                                 DriveInfo *pflash_drv,
>> +                                 int64_t **flash_size)
>>  {
>>      BlockDriverState *bdrv;
>>      int64_t size;
>> @@ -101,6 +102,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
>>      flash_mem = pflash_cfi01_get_memory(system_flash);
>>
>>      pc_isa_bios_init(rom_memory, flash_mem, size);
>> +    *flash_size = size;
>>  }
>>
>>  static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>> @@ -162,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>                                  bios);
>>  }
>>
>> -void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>> +void pc_system_firmware_init(MemoryRegion *rom_memory,
>> +                             bool isapc_ram_fw,
>> +                             int64_t **flash_size)
>>  {
>>      DriveInfo *pflash_drv;
>>
>> @@ -181,5 +185,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>>          exit(1);
>>      }
>>
>> -    pc_system_flash_init(rom_memory, pflash_drv);
>> +    pc_system_flash_init(rom_memory, pflash_drv, flash_size);
>>  }
>> --
>> 1.8.3.1
>>
>>

Re: [Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Peter Maydell]

On 8 November 2013 15:07, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/08/13 07:09, Jordan Justen wrote:
>> int64_t? :)
>
> Heh, yes, I did cringe when I wrote that, but if you check the
> bottom-most function, where the assignment happens,
> pc_system_flash_init(), it declares the local "size" variable as
> int64_t. I've given up on arguing for sensible unsigned types so I just
> went with the flow

That's a bug in that function which should be fixed.
This is a memory region size and those are uint64_t.

That said, having to pass the size of a sub-sub-region
all the way back up the call stack is very odd and makes
me wonder if it's really the right way to do it...
The top level shouldn't have to care like that about
details of the bottom of the callstack.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Laszlo Ersek]

On 11/08/13 16:16, Peter Maydell wrote:
> On 8 November 2013 15:07, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 11/08/13 07:09, Jordan Justen wrote:
>>> int64_t? :)
>>
>> Heh, yes, I did cringe when I wrote that, but if you check the
>> bottom-most function, where the assignment happens,
>> pc_system_flash_init(), it declares the local "size" variable as
>> int64_t. I've given up on arguing for sensible unsigned types so I just
>> went with the flow
> 
> That's a bug in that function which should be fixed.
> This is a memory region size and those are uint64_t.
> 
> That said, having to pass the size of a sub-sub-region
> all the way back up the call stack is very odd and makes
> me wonder if it's really the right way to do it...
> The top level shouldn't have to care like that about
> details of the bottom of the callstack.

I agree. It's just that system.flash and pci-hole are siblings in the
same container (they shouldn't overlap, *or* they should have clearly
different priorities between them). We have two call chains rooted in
pc_init1(), and the "ends" of those chains need to coordinate with each
other (they set up the two regions, respectively, and both need the
boundary between them). We could introduce a new global, but that's not
exactly a step forward :)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Peter Maydell]

On 8 November 2013 15:27, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/08/13 16:16, Peter Maydell wrote:
>> That said, having to pass the size of a sub-sub-region
>> all the way back up the call stack is very odd and makes
>> me wonder if it's really the right way to do it...
>> The top level shouldn't have to care like that about
>> details of the bottom of the callstack.
>
> I agree. It's just that system.flash and pci-hole are siblings in the
> same container (they shouldn't overlap, *or* they should have clearly
> different priorities between them). We have two call chains rooted in
> pc_init1(), and the "ends" of those chains need to coordinate with each
> other (they set up the two regions, respectively, and both need the
> boundary between them). We could introduce a new global, but that's not
> exactly a step forward :)

I suspect the correct approach for the pci-hole is to
use priorities. (Also possibly to refactor the code so
that the things creating memory regions directly in the
top level container aren't four levels down the callstack.
The natural thing would be for the callgraph to be vaguely
parallel to memory region hierarchy, so all the code
managing a particular container region is in the same
place in the source rather than distributed.)

-- PMM

[Qemu-devel] [PATCH 2/2] i386/pc_piix: the pci-hole should end where the system flash starts

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc_piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6e2c027..9bda20a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -152,7 +152,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
                               system_memory, system_io, args->ram_size,
                               below_4g_mem_size,
-                              0x100000000ULL - below_4g_mem_size,
+                              (0x100000000ULL - flash_size) - below_4g_mem_size,
                               above_4g_mem_size,
                               pci_memory, ram_memory);
     } else {
-- 
1.8.3.1

[Qemu-devel] reverting commit a53ae8e934cd54686875b5bcfc2f434244ee55d6 Re: [edk2] [PATCH 0/2] Re: exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 07/11/2013 23:23, Laszlo Ersek ha scritto:
> On 11/07/13 22:24, Marcel Apfelbaum wrote:
>> Why pci-hole and system.flash collide? IMHO we should not play with
>> priorities here, better solve the collision.
> 
> What about this "beautiful" series? It produces
> 
> memory
> 0000000000000000-000fffffffffffff (prio 0, RW): system
>   [...]
>   0000000060000000-00000000ffdfffff (prio 0, RW): alias pci-hole @pci
>   0000000060000000-00000000ffdfffff
>   [...]
>   00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash
> 
> and I can run OVMF with it. It also stays within i386/pc.

This definitely works, and make sense.

But I think for 1.7 we should just revert the commit.  It can be done
later in 1.8.  And it should have a qtest testcase that shows the effect
of the patch.

Other patches can still be applied to 1.7, but the change definitely had
much bigger ramifications than anticipated.  The patches are

[PATCH for-1.7 v2 5/8] pci: fix address space size for bridge
[PATCH for-1.7 v2 7/8] pc: s/INT64_MAX/UINT64_MAX/
[PATCH for-1.7 v2 8/8] spapr_pci: s/INT64_MAX/UINT64_MAX/

There's also

[PATCH 1/2] split definitions for exec.c and translate-all.c radix trees
[PATCH 2/2] exec: make address spaces 64-bit wide

which however has a 2% perf hit for TCG.  In 1.8 we can apply it and
recover the hit with other optimizations.

Paolo

Re: [Qemu-devel] [PATCH 0/2] Re: exec: fix regression by making system-memory region UINT64_MAX size

[Igor Mammedov]

On Thu,  7 Nov 2013 23:23:57 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/07/13 22:24, Marcel Apfelbaum wrote:
> 
> > Why pci-hole and system.flash collide? IMHO we should not play with
> > priorities here, better solve the collision.
> 
> What about this "beautiful" series? It produces
Laszlo,
 there is patch that removes PCI-hole aliases at all
 mapping PCI address space with -1 priority in system address space.
 so all access flash region range will go to it sice it's priority 0 

> 
> memory
> 0000000000000000-000fffffffffffff (prio 0, RW): system
>   [...]
>   0000000060000000-00000000ffdfffff (prio 0, RW): alias pci-hole @pci
>   0000000060000000-00000000ffdfffff
>   [...]
>   00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash
> 
> and I can run OVMF with it. It also stays within i386/pc.
> 
> Re 2/2, note that "below_4g_mem_size" never exceeds 0xe0000000 in
> pc_init1(), so the subtraction is safe.
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (2):
>   i386/pc: propagate flash size from pc_system_flash_init() to
>     pc_init1()
>   i386/pc_piix: the pci-hole should end where the system flash starts
> 
>  include/hw/i386/pc.h |  6 ++++--
>  hw/i386/pc.c         |  5 +++--
>  hw/i386/pc_piix.c    |  5 +++--
>  hw/i386/pc_q35.c     |  3 ++-
>  hw/i386/pc_sysfw.c   | 10 +++++++---
>  5 files changed, 19 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Andreas Färber]

Am 07.11.2013 21:27, schrieb Jordan Justen:
> On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>> The commit:
>>
>> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
>> Author: Marcel Apfelbaum <marcel.a@redhat.com>
>> Date:   Mon Sep 16 11:21:16 2013 +0300
>>
>>     hw/pci: partially handle pci master abort
>>
>> introduced a regression on make check:
> 
> Laszlo pointed out that my OVMF flash support series was not working
> with QEMU master. It was working with QEMU 1.6.0. I then bisected the
> issue to this commit. It seems this commit regresses -pflash support
> for both KVM and non-KVM modes.
> 
> Can you reproduce the issue this with command?
> x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin
> (with or without adding -enable-kvm)

Jordan or Laszlo,

Can either of you please add a small test case to i440fx-test using
-pflash and doing a read in the PCI hole (or wherever exactly) so that
we can avoid regressing yet again? :)

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

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Laszlo Ersek]

On 11/08/13 16:42, Andreas Färber wrote:
> Am 07.11.2013 21:27, schrieb Jordan Justen:
>> On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>>> The commit:
>>>
>>> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
>>> Author: Marcel Apfelbaum <marcel.a@redhat.com>
>>> Date:   Mon Sep 16 11:21:16 2013 +0300
>>>
>>>     hw/pci: partially handle pci master abort
>>>
>>> introduced a regression on make check:
>>
>> Laszlo pointed out that my OVMF flash support series was not working
>> with QEMU master. It was working with QEMU 1.6.0. I then bisected the
>> issue to this commit. It seems this commit regresses -pflash support
>> for both KVM and non-KVM modes.
>>
>> Can you reproduce the issue this with command?
>> x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin
>> (with or without adding -enable-kvm)
> 
> Jordan or Laszlo,
> 
> Can either of you please add a small test case to i440fx-test using
> -pflash and doing a read in the PCI hole (or wherever exactly) so that
> we can avoid regressing yet again? :)

For -pflash we need a small test file. I'm thinking about creating a 512
byte (1 sector) big file, and modifying the qemu command line in
"tests/i440fx-test.c".

I'm not very familiar with external files in tests though. Can I model
it on "qdict-test-data.txt"?

"qdict-test-data.txt" is located in the root source directory. When
configure runs outside the root source directory (= separate build dir),
it symlinks it. And, the "check-qdict.c" test program opens it (with
fopen()) simply by basename (no path prefix). Can I follow that?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 08/11/2013 17:19, Laszlo Ersek ha scritto:
> I'm not very familiar with external files in tests though. Can I model
> it on "qdict-test-data.txt"?
> 
> "qdict-test-data.txt" is located in the root source directory. When
> configure runs outside the root source directory (= separate build dir),
> it symlinks it. And, the "check-qdict.c" test program opens it (with
> fopen()) simply by basename (no path prefix). Can I follow that?

Yes, tests are always run from the root build directory.  Symlinking
files is not particularly awesome, but it works.

A patch to move qdict-test-data.txt to tests/ is welcome. :)

You can create a temporary file as well from the qtest.  That's probably
easier; just write 64 kB of 0x00 0x01 0x02 0x03...0xFF 0x00 0x01...
Also, please test both -bios and -pflash.

Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Andreas Färber]

Am 08.11.2013 17:19, schrieb Laszlo Ersek:
> On 11/08/13 16:42, Andreas Färber wrote:
>> Jordan or Laszlo,
>>
>> Can either of you please add a small test case to i440fx-test using
>> -pflash and doing a read in the PCI hole (or wherever exactly) so that
>> we can avoid regressing yet again? :)
> 
> For -pflash we need a small test file. I'm thinking about creating a 512
> byte (1 sector) big file, and modifying the qemu command line in
> "tests/i440fx-test.c".
> 
> I'm not very familiar with external files in tests though. Can I model
> it on "qdict-test-data.txt"?
> 
> "qdict-test-data.txt" is located in the root source directory. When
> configure runs outside the root source directory (= separate build dir),
> it symlinks it. And, the "check-qdict.c" test program opens it (with
> fopen()) simply by basename (no path prefix). Can I follow that?

I don't have personal experience with using external files there yet,
but I was hoping that using -pflash pc-bios/bios.bin would just work
since that'll be symlinked for execution from build directory iiuc.

My thinking was the test could then verify that the BIOS does not read
as all 0xff, whereas Paolo's suggestion sounds more elaborate, ruling
out actual 0xff within SeaBIOS by having a positive pattern to check for.

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

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Paolo Bonzini]

Il 08/11/2013 18:09, Andreas Färber ha scritto:
> I don't have personal experience with using external files there yet,
> but I was hoping that using -pflash pc-bios/bios.bin would just work
> since that'll be symlinked for execution from build directory iiuc.
> 
> My thinking was the test could then verify that the BIOS does not read
> as all 0xff, whereas Paolo's suggestion sounds more elaborate, ruling
> out actual 0xff within SeaBIOS by having a positive pattern to check for.

Yeah, that's also a good test and easier!

Paolo

Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Laszlo Ersek]

On 11/08/13 18:15, Paolo Bonzini wrote:
> Il 08/11/2013 18:09, Andreas Färber ha scritto:
>> I don't have personal experience with using external files there yet,
>> but I was hoping that using -pflash pc-bios/bios.bin would just work
>> since that'll be symlinked for execution from build directory iiuc.
>>
>> My thinking was the test could then verify that the BIOS does not read
>> as all 0xff, whereas Paolo's suggestion sounds more elaborate, ruling
>> out actual 0xff within SeaBIOS by having a positive pattern to check for.
> 
> Yeah, that's also a good test and easier!

Believe it or not, I did think of both 0x00..0xFF (from a small static
file) and using an actual bios image :) I think I'd prefer 0x00..0xFF
(with a temporary file as Paolo suggested) because "what is it" looks
more attractive than "what is it not" to me.

Also, opening the packaged bios binary from the build dir would need
extra symlinking again; mkstemp() seems cleaner.

Thanks!
Laszlo