Re: [Qemu-devel] [PATCH v2 1/1] riscv: Ensure the kernel start address is correctly cast

2019-01-24 Thread Alistair Francis 
On Wed, Jan 23, 2019 at 6:00 PM Palmer Dabbelt  wrote:
>
> On Tue, 15 Jan 2019 13:09:28 PST (-0800), alistai...@gmail.com wrote:
> > On Mon, Jan 14, 2019 at 2:58 AM Philippe Mathieu-Daudé
> >  wrote:
> >>
> >> Hi Alistair,
> >>
> >> On 1/12/19 2:17 AM, Alistair Francis wrote:
> >> > Cast the kernel start address to the target bit length.
> >> >
> >> > This ensures that we calculate the initrd offset to a valid address for
> >> > the architecture.
> >>
> >> Can you add an example of the failure symptoms?
> >
> > I can.
>
> Should I be waiting for a v3?

You should be, I thought I sent it but I must have forgotten. I'll send it now.

Alistair

>
> >
> >>
> >> >
> >> > Signed-off-by: Alistair Francis 
> >> > Suggested-by: Alexander Graf 
> >> > Reported-by: Alexander Graf 
> >> > ---
> >> > v2:
> >> >  - Remove old comment
> >> >  hw/riscv/sifive_e.c | 2 +-
> >> >  hw/riscv/sifive_u.c | 2 +-
> >> >  hw/riscv/spike.c| 2 +-
> >> >  hw/riscv/virt.c | 2 +-
> >> >  4 files changed, 4 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >> > index 5d9d65ff29..e5d7fc548e 100644
> >> > --- a/hw/riscv/sifive_e.c
> >> > +++ b/hw/riscv/sifive_e.c
> >> > @@ -74,7 +74,7 @@ static const struct MemmapEntry {
> >> >  [SIFIVE_E_DTIM] = { 0x8000, 0x4000 }
> >> >  };
> >> >
> >> > -static uint64_t load_kernel(const char *kernel_filename)
> >> > +static target_ulong load_kernel(const char *kernel_filename)
> >> >  {
> >> >  uint64_t kernel_entry, kernel_high;
> >>
> >> Shouldn't you update load_elf() and co now to take target_ulong
> >> arguments? This would fix this error generically for all archs.
> >
> > That is an option, but as load_elf() is called by every other machine
> > I don't want to break them. It's entirely possible that other machines
> > rely on this behaviour and changing it will break them.
> >
> > Alistair
> >
> >>
> >> >
> >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> >> > index 3bd3b67507..3b3acec377 100644
> >> > --- a/hw/riscv/sifive_u.c
> >> > +++ b/hw/riscv/sifive_u.c
> >> > @@ -65,7 +65,7 @@ static const struct MemmapEntry {
> >> >
> >> >  #define GEM_REVISION0x10070109
> >> >
> >> > -static uint64_t load_kernel(const char *kernel_filename)
> >> > +static target_ulong load_kernel(const char *kernel_filename)
> >> >  {
> >> >  uint64_t kernel_entry, kernel_high;
> >> >
> >> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> >> > index 268df04c3c..79cb4c1282 100644
> >> > --- a/hw/riscv/spike.c
> >> > +++ b/hw/riscv/spike.c
> >> > @@ -53,7 +53,7 @@ static const struct MemmapEntry {
> >> >  [SPIKE_DRAM] = { 0x8000,0x0 },
> >> >  };
> >> >
> >> > -static uint64_t load_kernel(const char *kernel_filename)
> >> > +static target_ulong load_kernel(const char *kernel_filename)
> >> >  {
> >> >  uint64_t kernel_entry, kernel_high;
> >> >
> >> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> > index e7f0716fb6..648462b18c 100644
> >> > --- a/hw/riscv/virt.c
> >> > +++ b/hw/riscv/virt.c
> >> > @@ -62,7 +62,7 @@ static const struct MemmapEntry {
> >> >  [VIRT_PCIE_ECAM] =   { 0x3000,0x1000 },
> >> >  };
> >> >
> >> > -static uint64_t load_kernel(const char *kernel_filename)
> >> > +static target_ulong load_kernel(const char *kernel_filename)
> >> >  {
> >> >  uint64_t kernel_entry, kernel_high;
> >> >
> >> >

Re: [Qemu-devel] [PATCH v2 1/1] riscv: Ensure the kernel start address is correctly cast

2019-01-23 Thread Palmer Dabbelt 

On Tue, 15 Jan 2019 13:09:28 PST (-0800), alistai...@gmail.com wrote:

On Mon, Jan 14, 2019 at 2:58 AM Philippe Mathieu-Daudé
 wrote:


Hi Alistair,

On 1/12/19 2:17 AM, Alistair Francis wrote:
> Cast the kernel start address to the target bit length.
>
> This ensures that we calculate the initrd offset to a valid address for
> the architecture.

Can you add an example of the failure symptoms?


I can.


Should I be waiting for a v3?





>
> Signed-off-by: Alistair Francis 
> Suggested-by: Alexander Graf 
> Reported-by: Alexander Graf 
> ---
> v2:
>  - Remove old comment
>  hw/riscv/sifive_e.c | 2 +-
>  hw/riscv/sifive_u.c | 2 +-
>  hw/riscv/spike.c| 2 +-
>  hw/riscv/virt.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 5d9d65ff29..e5d7fc548e 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -74,7 +74,7 @@ static const struct MemmapEntry {
>  [SIFIVE_E_DTIM] = { 0x8000, 0x4000 }
>  };
>
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;

Shouldn't you update load_elf() and co now to take target_ulong
arguments? This would fix this error generically for all archs.


That is an option, but as load_elf() is called by every other machine
I don't want to break them. It's entirely possible that other machines
rely on this behaviour and changing it will break them.

Alistair



>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 3bd3b67507..3b3acec377 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -65,7 +65,7 @@ static const struct MemmapEntry {
>
>  #define GEM_REVISION0x10070109
>
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;
>
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 268df04c3c..79cb4c1282 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -53,7 +53,7 @@ static const struct MemmapEntry {
>  [SPIKE_DRAM] = { 0x8000,0x0 },
>  };
>
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index e7f0716fb6..648462b18c 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -62,7 +62,7 @@ static const struct MemmapEntry {
>  [VIRT_PCIE_ECAM] =   { 0x3000,0x1000 },
>  };
>
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;
>
>

Re: [Qemu-devel] [PATCH v2 1/1] riscv: Ensure the kernel start address is correctly cast

2019-01-15 Thread Alistair Francis 
On Mon, Jan 14, 2019 at 2:58 AM Philippe Mathieu-Daudé
 wrote:
>
> Hi Alistair,
>
> On 1/12/19 2:17 AM, Alistair Francis wrote:
> > Cast the kernel start address to the target bit length.
> >
> > This ensures that we calculate the initrd offset to a valid address for
> > the architecture.
>
> Can you add an example of the failure symptoms?

I can.

>
> >
> > Signed-off-by: Alistair Francis 
> > Suggested-by: Alexander Graf 
> > Reported-by: Alexander Graf 
> > ---
> > v2:
> >  - Remove old comment
> >  hw/riscv/sifive_e.c | 2 +-
> >  hw/riscv/sifive_u.c | 2 +-
> >  hw/riscv/spike.c| 2 +-
> >  hw/riscv/virt.c | 2 +-
> >  4 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > index 5d9d65ff29..e5d7fc548e 100644
> > --- a/hw/riscv/sifive_e.c
> > +++ b/hw/riscv/sifive_e.c
> > @@ -74,7 +74,7 @@ static const struct MemmapEntry {
> >  [SIFIVE_E_DTIM] = { 0x8000, 0x4000 }
> >  };
> >
> > -static uint64_t load_kernel(const char *kernel_filename)
> > +static target_ulong load_kernel(const char *kernel_filename)
> >  {
> >  uint64_t kernel_entry, kernel_high;
>
> Shouldn't you update load_elf() and co now to take target_ulong
> arguments? This would fix this error generically for all archs.

That is an option, but as load_elf() is called by every other machine
I don't want to break them. It's entirely possible that other machines
rely on this behaviour and changing it will break them.

Alistair

>
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 3bd3b67507..3b3acec377 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -65,7 +65,7 @@ static const struct MemmapEntry {
> >
> >  #define GEM_REVISION0x10070109
> >
> > -static uint64_t load_kernel(const char *kernel_filename)
> > +static target_ulong load_kernel(const char *kernel_filename)
> >  {
> >  uint64_t kernel_entry, kernel_high;
> >
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index 268df04c3c..79cb4c1282 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -53,7 +53,7 @@ static const struct MemmapEntry {
> >  [SPIKE_DRAM] = { 0x8000,0x0 },
> >  };
> >
> > -static uint64_t load_kernel(const char *kernel_filename)
> > +static target_ulong load_kernel(const char *kernel_filename)
> >  {
> >  uint64_t kernel_entry, kernel_high;
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index e7f0716fb6..648462b18c 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -62,7 +62,7 @@ static const struct MemmapEntry {
> >  [VIRT_PCIE_ECAM] =   { 0x3000,0x1000 },
> >  };
> >
> > -static uint64_t load_kernel(const char *kernel_filename)
> > +static target_ulong load_kernel(const char *kernel_filename)
> >  {
> >  uint64_t kernel_entry, kernel_high;
> >
> >

Re: [Qemu-devel] [PATCH v2 1/1] riscv: Ensure the kernel start address is correctly cast

2019-01-14 Thread Philippe Mathieu-Daudé 
Hi Alistair,

On 1/12/19 2:17 AM, Alistair Francis wrote:
> Cast the kernel start address to the target bit length.
> 
> This ensures that we calculate the initrd offset to a valid address for
> the architecture.

Can you add an example of the failure symptoms?

> 
> Signed-off-by: Alistair Francis 
> Suggested-by: Alexander Graf 
> Reported-by: Alexander Graf 
> ---
> v2:
>  - Remove old comment
>  hw/riscv/sifive_e.c | 2 +-
>  hw/riscv/sifive_u.c | 2 +-
>  hw/riscv/spike.c| 2 +-
>  hw/riscv/virt.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 5d9d65ff29..e5d7fc548e 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -74,7 +74,7 @@ static const struct MemmapEntry {
>  [SIFIVE_E_DTIM] = { 0x8000, 0x4000 }
>  };
>  
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;

Shouldn't you update load_elf() and co now to take target_ulong
arguments? This would fix this error generically for all archs.

>  
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 3bd3b67507..3b3acec377 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -65,7 +65,7 @@ static const struct MemmapEntry {
>  
>  #define GEM_REVISION0x10070109
>  
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;
>  
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 268df04c3c..79cb4c1282 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -53,7 +53,7 @@ static const struct MemmapEntry {
>  [SPIKE_DRAM] = { 0x8000,0x0 },
>  };
>  
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;
>  
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index e7f0716fb6..648462b18c 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -62,7 +62,7 @@ static const struct MemmapEntry {
>  [VIRT_PCIE_ECAM] =   { 0x3000,0x1000 },
>  };
>  
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
>  {
>  uint64_t kernel_entry, kernel_high;
>  
>

[Qemu-devel] [PATCH v2 1/1] riscv: Ensure the kernel start address is correctly cast

2019-01-11 Thread Alistair Francis 
Cast the kernel start address to the target bit length.

This ensures that we calculate the initrd offset to a valid address for
the architecture.

Signed-off-by: Alistair Francis 
Suggested-by: Alexander Graf 
Reported-by: Alexander Graf 
---
v2:
 - Remove old comment
 hw/riscv/sifive_e.c | 2 +-
 hw/riscv/sifive_u.c | 2 +-
 hw/riscv/spike.c| 2 +-
 hw/riscv/virt.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 5d9d65ff29..e5d7fc548e 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -74,7 +74,7 @@ static const struct MemmapEntry {
 [SIFIVE_E_DTIM] = { 0x8000, 0x4000 }
 };
 
-static uint64_t load_kernel(const char *kernel_filename)
+static target_ulong load_kernel(const char *kernel_filename)
 {
 uint64_t kernel_entry, kernel_high;
 
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 3bd3b67507..3b3acec377 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -65,7 +65,7 @@ static const struct MemmapEntry {
 
 #define GEM_REVISION0x10070109
 
-static uint64_t load_kernel(const char *kernel_filename)
+static target_ulong load_kernel(const char *kernel_filename)
 {
 uint64_t kernel_entry, kernel_high;
 
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 268df04c3c..79cb4c1282 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -53,7 +53,7 @@ static const struct MemmapEntry {
 [SPIKE_DRAM] = { 0x8000,0x0 },
 };
 
-static uint64_t load_kernel(const char *kernel_filename)
+static target_ulong load_kernel(const char *kernel_filename)
 {
 uint64_t kernel_entry, kernel_high;
 
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e7f0716fb6..648462b18c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -62,7 +62,7 @@ static const struct MemmapEntry {
 [VIRT_PCIE_ECAM] =   { 0x3000,0x1000 },
 };
 
-static uint64_t load_kernel(const char *kernel_filename)
+static target_ulong load_kernel(const char *kernel_filename)
 {
 uint64_t kernel_entry, kernel_high;
 
-- 
2.19.1