[Qemu-devel] [RFC PATCH 0/2] spapr: changes for kvm-unit-tests

[Qemu-devel] [RFC PATCH 0/2] spapr: changes for kvm-unit-tests

[Andrew Jones]

I've started playing with adding ppc support to kvm-unit-tests,
using spapr for the machine model. Here are a couple patches
to spapr I've made.

Thanks,
drew

Andrew Jones (2):
  spapr: add dumpdtb support
  spapr: -kernel: allow linking with specified addr

 hw/ppc/spapr.c | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.4.3

[Qemu-devel] [RFC PATCH 1/2] spapr: add dumpdtb support

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/ppc/spapr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a6f19473cf278..c1cbf3387ae0c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -30,6 +30,7 @@
 #include "hw/fw-path-provider.h"
 #include "elf.h"
 #include "net/net.h"
+#include "sysemu/device_tree.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
@@ -822,6 +823,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
         exit(1);
     }
 
+    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
 
     g_free(bootlist);
-- 
2.4.3

[Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[Andrew Jones]

I've started playing with adding ppc support to kvm-unit-tests,
using spapr for the machine model. I wanted to link the unit test
at 0x400000 to match qemu's load address, making the unit test
startup code simpler, but ended up with 0x800000 instead, due to
how translate_kernel_address works. The translation makes sense
for how Linux kernels are linked (always at 0xc0000000 or
0xc000000000000000), but for the unit test case we need to avoid
adding the offset.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
Big RFC because I don't know if the "always at 0xc..." statement
is 100% true for Linux, nor if this patch would break other stuff...

 hw/ppc/spapr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c1cbf3387ae0c..4f1548f5168db 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -832,6 +832,9 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
 
 static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
 {
+    if ((addr & 0x0fffffff) == addr) {
+        return addr;
+    }
     return (addr & 0x0fffffff) + KERNEL_LOAD_ADDR;
 }
 
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH 1/2] spapr: add dumpdtb support

[David Gibson]

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

On Fri, Jul 17, 2015 at 01:56:39PM +0200, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Looks good to me, but I'd like an actual commit message: what's
dumpdtb, how and why would you use it.

> ---
>  hw/ppc/spapr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a6f19473cf278..c1cbf3387ae0c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -30,6 +30,7 @@
>  #include "hw/fw-path-provider.h"
>  #include "elf.h"
>  #include "net/net.h"
> +#include "sysemu/device_tree.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/kvm.h"
> @@ -822,6 +823,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>          exit(1);
>      }
>  
> +    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>      cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
>  
>      g_free(bootlist);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/2] spapr: changes for kvm-unit-tests

[David Gibson]

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

On Fri, Jul 17, 2015 at 01:56:38PM +0200, Andrew Jones wrote:
> I've started playing with adding ppc support to kvm-unit-tests,
> using spapr for the machine model. Here are a couple patches
> to spapr I've made.

For future reference, I prefer if patches intended for upstream are
sent to my external address <david@gibson.dropbear.id.au>.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[David Gibson]

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

On Fri, Jul 17, 2015 at 01:56:40PM +0200, Andrew Jones wrote:
> I've started playing with adding ppc support to kvm-unit-tests,
> using spapr for the machine model. I wanted to link the unit test
> at 0x400000 to match qemu's load address, making the unit test
> startup code simpler, but ended up with 0x800000 instead, due to
> how translate_kernel_address works. The translation makes sense
> for how Linux kernels are linked (always at 0xc0000000 or
> 0xc000000000000000), but for the unit test case we need to avoid
> adding the offset.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> Big RFC because I don't know if the "always at 0xc..." statement
> is 100% true for Linux, nor if this patch would break other stuff...

Yeah, I'm pretty dubious about this too, especially since I don't
entirely grasp what the load_elf() translation function is all about
anyway.

At least 32-bit Linux on powerpc can have a different link offset -
it's actually a config option.  I can't see any practical purpose for
a link address that has any bits in the 0xfffffff mask though.

That said, I suspect making your unit test assume a fixed load address
may not be the best idea - qemu or SLOF could change in future to move
things about, so it might be more robust to have your test copy itself
to address it wants to be at before executing.

> 
>  hw/ppc/spapr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c1cbf3387ae0c..4f1548f5168db 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -832,6 +832,9 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>  
>  static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
>  {
> +    if ((addr & 0x0fffffff) == addr) {
> +        return addr;
> +    }
>      return (addr & 0x0fffffff) + KERNEL_LOAD_ADDR;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[Thomas Huth]

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

On 20/07/15 07:01, David Gibson wrote:
> On Fri, Jul 17, 2015 at 01:56:40PM +0200, Andrew Jones wrote:
>> I've started playing with adding ppc support to kvm-unit-tests,
>> using spapr for the machine model. I wanted to link the unit test
>> at 0x400000 to match qemu's load address, making the unit test
>> startup code simpler, but ended up with 0x800000 instead, due to
>> how translate_kernel_address works. The translation makes sense
>> for how Linux kernels are linked (always at 0xc0000000 or
>> 0xc000000000000000), but for the unit test case we need to avoid
>> adding the offset.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> ---
>> Big RFC because I don't know if the "always at 0xc..." statement
>> is 100% true for Linux, nor if this patch would break other stuff...
> 
> Yeah, I'm pretty dubious about this too, especially since I don't
> entirely grasp what the load_elf() translation function is all about
> anyway.

Well, AFAIK it's used to modify the addresses before the ELF loader uses
the address for loading. For example if your ELF binary is linked at
address 0x1000, the translate function would move your binary to
0x401000 instead so that it does not interfere with the SLOF firmware
(which is loaded to address 0 IIRC).

So I also think your fix here is wrong, Andrew. E.g. when you have a
binary that is linked to address 0x1000, you don't want to bypass the
translation step here since it then would clash with the firmware.

> That said, I suspect making your unit test assume a fixed load address
> may not be the best idea - qemu or SLOF could change in future to move
> things about, so it might be more robust to have your test copy itself
> to address it wants to be at before executing.

+1

... or you could try to get the elf_reloc code working for POWER, too
(see include/hw/elf_ops.h). That way QEMU would take care of relocating
your program. (you can peek at elf_apply_rela64() in
 https://github.com/aik/SLOF/blob/master/lib/libelf/elf64.c
if you want to know what basically has to be done for POWER relocations).

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[Andrew Jones]

On Mon, Jul 20, 2015 at 08:47:53AM +0200, Thomas Huth wrote:
> On 20/07/15 07:01, David Gibson wrote:
> > On Fri, Jul 17, 2015 at 01:56:40PM +0200, Andrew Jones wrote:
> >> I've started playing with adding ppc support to kvm-unit-tests,
> >> using spapr for the machine model. I wanted to link the unit test
> >> at 0x400000 to match qemu's load address, making the unit test
> >> startup code simpler, but ended up with 0x800000 instead, due to
> >> how translate_kernel_address works. The translation makes sense
> >> for how Linux kernels are linked (always at 0xc0000000 or
> >> 0xc000000000000000), but for the unit test case we need to avoid
> >> adding the offset.
> >>
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> ---
> >> Big RFC because I don't know if the "always at 0xc..." statement
> >> is 100% true for Linux, nor if this patch would break other stuff...
> > 
> > Yeah, I'm pretty dubious about this too, especially since I don't
> > entirely grasp what the load_elf() translation function is all about
> > anyway.
> 
> Well, AFAIK it's used to modify the addresses before the ELF loader uses
> the address for loading. For example if your ELF binary is linked at
> address 0x1000, the translate function would move your binary to
> 0x401000 instead so that it does not interfere with the SLOF firmware
> (which is loaded to address 0 IIRC).

This is correct, but the move isn't just to make sure we don't interfere
with SLOF, it's also to make sure we can load the kernel into main
memory. When the link address is 0xc...,  then we can't use vaddr ==
paddr. The Linux ppc64 kernel, for example, is linked at 0xc000000000000000.
So it gets pulled down with the mask, and then the 0x400000 offset is added
to get it above SLOF.

> 
> So I also think your fix here is wrong, Andrew. E.g. when you have a
> binary that is linked to address 0x1000, you don't want to bypass the
> translation step here since it then would clash with the firmware.

I set the unit test's text segment start at 0x400000, so QEMU still
loads it there with this patch, and thus it wouldn't clash with SLOF.
But, anyway, there's no need for SLOF in kvm-unit-tests, and I've
replaced it with a four byte boot loader, b 0x400000.

> 
> > That said, I suspect making your unit test assume a fixed load address
> > may not be the best idea - qemu or SLOF could change in future to move
> > things about, so it might be more robust to have your test copy itself
> > to address it wants to be at before executing.
> 

Well, the reason I want vaddr == paddr is to be able to run C code
without the MMU enabled, not to mention to avoid needing to do any sort
of reloc dance in what's supposed to be super simple code. I don't need
to worry about SLOF changing things, since I don't use it. If QEMU changes
the load address, then things will indeed break, but it would be a one line
Makefile fix in kvm-unit-tests to point the text segment to the new offset.

> +1
> 
> ... or you could try to get the elf_reloc code working for POWER, too
> (see include/hw/elf_ops.h). That way QEMU would take care of relocating
> your program. (you can peek at elf_apply_rela64() in
>  https://github.com/aik/SLOF/blob/master/lib/libelf/elf64.c
> if you want to know what basically has to be done for POWER relocations).
> 
>  Thomas
> 
> 

kvm-unit-tests doesn't load the unit test elf itself. It relies on QEMU's
-kernel parameter to get the "kernel" (the unit test) into memory.

Thanks,
drew

Re: [Qemu-devel] [RFC PATCH 1/2] spapr: add dumpdtb support

[Andrew Jones]

On Mon, Jul 20, 2015 at 02:02:33PM +1000, David Gibson wrote:
> On Fri, Jul 17, 2015 at 01:56:39PM +0200, Andrew Jones wrote:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> Looks good to me, but I'd like an actual commit message: what's
> dumpdtb, how and why would you use it.

Ok, just sent it separately with a commit message and the RFC dropped.

Thanks,
drew
> 
> > ---
> >  hw/ppc/spapr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index a6f19473cf278..c1cbf3387ae0c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -30,6 +30,7 @@
> >  #include "hw/fw-path-provider.h"
> >  #include "elf.h"
> >  #include "net/net.h"
> > +#include "sysemu/device_tree.h"
> >  #include "sysemu/block-backend.h"
> >  #include "sysemu/cpus.h"
> >  #include "sysemu/kvm.h"
> > @@ -822,6 +823,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> >          exit(1);
> >      }
> >  
> > +    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> >      cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> >  
> >      g_free(bootlist);
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[Andrew Jones]

On Fri, Jul 17, 2015 at 01:56:40PM +0200, Andrew Jones wrote:
> I've started playing with adding ppc support to kvm-unit-tests,
> using spapr for the machine model. I wanted to link the unit test
> at 0x400000 to match qemu's load address, making the unit test
> startup code simpler, but ended up with 0x800000 instead, due to
> how translate_kernel_address works. The translation makes sense
> for how Linux kernels are linked (always at 0xc0000000 or
> 0xc000000000000000), but for the unit test case we need to avoid
> adding the offset.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> Big RFC because I don't know if the "always at 0xc..." statement
> is 100% true for Linux, nor if this patch would break other stuff...
> 
>  hw/ppc/spapr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c1cbf3387ae0c..4f1548f5168db 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -832,6 +832,9 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>  
>  static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
>  {
> +    if ((addr & 0x0fffffff) == addr) {
> +        return addr;
> +    }
>      return (addr & 0x0fffffff) + KERNEL_LOAD_ADDR;
>  }
>

OK, while I'm still not sure if this is a bad idea or not, and I do think
it would be nice to give users more control over the kernel's linked VMAs,
I've decided it would probably be harder to prove this patch is safe, than
to add reloc support to kvm-unit-tests/powerpc. So I added the support
yesterday, at least enough for now, and this patch is no longer necessary.

Thanks,
drew

Re: [Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[Thomas Huth]

On 20/07/15 15:09, Andrew Jones wrote:
> On Mon, Jul 20, 2015 at 08:47:53AM +0200, Thomas Huth wrote:
...
>> ... or you could try to get the elf_reloc code working for POWER, too
>> (see include/hw/elf_ops.h). That way QEMU would take care of relocating
>> your program. (you can peek at elf_apply_rela64() in
>>  https://github.com/aik/SLOF/blob/master/lib/libelf/elf64.c
>> if you want to know what basically has to be done for POWER relocations).
> 
> kvm-unit-tests doesn't load the unit test elf itself. It relies on QEMU's
> -kernel parameter to get the "kernel" (the unit test) into memory.

I was talking about the -kernel parameter of QEMU. That triggers the
load_elf() function of QEMU - and this function can provide ELF
relocation, too. It is used for s390x already to relocate the firmware
there, so I think it could be done for ppc64, too.

 Thomas

Re: [Qemu-devel] [RFC PATCH 2/2] spapr: -kernel: allow linking with specified addr

[Andrew Jones]

On Tue, Jul 21, 2015 at 09:10:44AM +0200, Thomas Huth wrote:
> On 20/07/15 15:09, Andrew Jones wrote:
> > On Mon, Jul 20, 2015 at 08:47:53AM +0200, Thomas Huth wrote:
> ...
> >> ... or you could try to get the elf_reloc code working for POWER, too
> >> (see include/hw/elf_ops.h). That way QEMU would take care of relocating
> >> your program. (you can peek at elf_apply_rela64() in
> >>  https://github.com/aik/SLOF/blob/master/lib/libelf/elf64.c
> >> if you want to know what basically has to be done for POWER relocations).
> > 
> > kvm-unit-tests doesn't load the unit test elf itself. It relies on QEMU's
> > -kernel parameter to get the "kernel" (the unit test) into memory.
> 
> I was talking about the -kernel parameter of QEMU. That triggers the
> load_elf() function of QEMU - and this function can provide ELF
> relocation, too. It is used for s390x already to relocate the firmware
> there, so I think it could be done for ppc64, too.

Ah, I think I see what you mean now, extend QEMU's elf loader to do the
relocating. Then, when the relocatable kernel starts, it has already
been pre-relocated. That would certainly be do-able, but I think any
kernel that expects to be relocatable will already have it's own code,
and we'd end up doing the relocation twice. Kernels that want to avoid
relocation would typically depend on the linker load addresses (but the
problem here is that spapr has it's own opinion for those...)

I took a quick peek at what s390x does right now for its firmware. afaict
it's similar to what spapr is doing, just overriding the elf's LMA.

drew