[Qemu-devel] [v1][Qemu-ppc][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Qemu-devel] [v1][Qemu-ppc][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Tiejun Chen]

We should sync params->ram_size after we fixup memory size on
a alignment boundary. Otherwise Guest would exceed the actual
memory region.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 hw/ppc/e500.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index c1bdb6b..145da0e 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
 
     /* Fixup Memory size on a alignment boundary */
     ram_size &= ~(RAM_SIZES_ALIGN - 1);
+    /* Sync this for the system. */
+    params->ram_size = ram_size;
 
     /* Register Memory */
     memory_region_init_ram(ram, "mpc8544ds.ram", ram_size);
-- 
1.7.9.5

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Scott Wood]

On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
> We should sync params->ram_size after we fixup memory size on
> a alignment boundary. Otherwise Guest would exceed the actual
> memory region.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  hw/ppc/e500.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index c1bdb6b..145da0e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
> 
>      /* Fixup Memory size on a alignment boundary */
>      ram_size &= ~(RAM_SIZES_ALIGN - 1);
> +    /* Sync this for the system. */
> +    params->ram_size = ram_size;

Could you explain this further?  When does params->ram_size ever get  
used after this point?

-Scott

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Alexander Graf]

On 29.04.2013, at 21:18, Scott Wood wrote:

> On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
>> We should sync params->ram_size after we fixup memory size on
>> a alignment boundary. Otherwise Guest would exceed the actual
>> memory region.
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>> hw/ppc/e500.c |    2 ++
>> 1 file changed, 2 insertions(+)
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index c1bdb6b..145da0e 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
>>     /* Fixup Memory size on a alignment boundary */
>>     ram_size &= ~(RAM_SIZES_ALIGN - 1);
>> +    /* Sync this for the system. */
>> +    params->ram_size = ram_size;
> 
> Could you explain this further?  When does params->ram_size ever get used after this point?

This is the respective code in vl.c:

>     QEMUMachineInitArgs args = { .ram_size = ram_size,
>                                  .boot_device = (boot_devices[0] == '\0') ?
>                                                 machine->boot_order :
>                                                 boot_devices,
>                                  .kernel_filename = kernel_filename,
>                                  .kernel_cmdline = kernel_cmdline,
>                                  .initrd_filename = initrd_filename,
>                                  .cpu_model = cpu_model };
>     machine->init(&args);


After this, args in unused. So as you can see, params->ram_size never gets used after it's read once.

What bug are you trying to fix? Maybe there really is something broken here, that this patch doesn't fix either.


Alex

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Andreas Färber]

Am 30.04.2013 11:53, schrieb Alexander Graf:
> 
> On 29.04.2013, at 21:18, Scott Wood wrote:
> 
>> On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
>>> We should sync params->ram_size after we fixup memory size on
>>> a alignment boundary. Otherwise Guest would exceed the actual
>>> memory region.
>>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>>> ---
>>> hw/ppc/e500.c |    2 ++
>>> 1 file changed, 2 insertions(+)
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index c1bdb6b..145da0e 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
>>>     /* Fixup Memory size on a alignment boundary */
>>>     ram_size &= ~(RAM_SIZES_ALIGN - 1);
>>> +    /* Sync this for the system. */
>>> +    params->ram_size = ram_size;
>>
>> Could you explain this further?  When does params->ram_size ever get used after this point?
> 
> This is the respective code in vl.c:
> 
>>     QEMUMachineInitArgs args = { .ram_size = ram_size,
>>                                  .boot_device = (boot_devices[0] == '\0') ?
>>                                                 machine->boot_order :
>>                                                 boot_devices,
>>                                  .kernel_filename = kernel_filename,
>>                                  .kernel_cmdline = kernel_cmdline,
>>                                  .initrd_filename = initrd_filename,
>>                                  .cpu_model = cpu_model };
>>     machine->init(&args);
> 
> 
> After this, args in unused. So as you can see, params->ram_size never gets used after it's read once.

While that is true, it was being discussed to update cpu_model inside
QEMUMachineInitArgs FWIW. That's independent of PPCE500Params though.

Andreas

> What bug are you trying to fix? Maybe there really is something broken here, that this patch doesn't fix either.
> 
> 
> Alex

-- 
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] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Chen, Tiejun]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com] 
> Sent: Tuesday, April 30, 2013 3:19 AM
> To: Chen, Tiejun
> Cc: agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct 
> params->ram_size with ram_size
> 
> On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
> > We should sync params->ram_size after we fixup memory size on a 
> > alignment boundary. Otherwise Guest would exceed the actual memory 
> > region.
> > 
> > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> > ---
> >  hw/ppc/e500.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index c1bdb6b..145da0e 
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
> > 
> >      /* Fixup Memory size on a alignment boundary */
> >      ram_size &= ~(RAM_SIZES_ALIGN - 1);
> > +    /* Sync this for the system. */
> > +    params->ram_size = ram_size;
> 
> Could you explain this further?  When does params->ram_size 
> ever get used after this point?

In that case we have to create a dtb without passing an extra dtb, we always use params->ram_size inside ppce500_load_device_tree(),

ppce500_load_device_tree()
{
...
uint64_t mem_reg_property[] = { 0, cpu_to_be64(params->ram_size) };
...
qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property, sizeof(mem_reg_property));

So occasionally if we don't pass a aligned memory size with "-m" like "-m 500", then Guest still think it owns 500M but we register memory with ram_size actually,

ppce500_init()
{
...
/* Fixup Memory size on a alignment boundary */
ram_size &= ~(RAM_SIZES_ALIGN - 1); ==> 448M

/* Register Memory */
memory_region_init_ram(ram, "mpc8544ds.ram", ram_size);

Guest would trigger TLB error when Guest allocate to access somewhere between 448M and 500M, its always failed since KVM hasn't that memory slot at all.

So we need to sync params->ram_size with ram_size.

Tiejun

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Scott Wood]

On 04/30/2013 06:03:54 PM, Chen, Tiejun wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:scottwood@freescale.com]
> > Sent: Tuesday, April 30, 2013 3:19 AM
> > To: Chen, Tiejun
> > Cc: agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> > Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct
> > params->ram_size with ram_size
> >
> > On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
> > > We should sync params->ram_size after we fixup memory size on a
> > > alignment boundary. Otherwise Guest would exceed the actual memory
> > > region.
> > >
> > > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> > > ---
> > >  hw/ppc/e500.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index c1bdb6b..145da0e
> > > 100644
> > > --- a/hw/ppc/e500.c
> > > +++ b/hw/ppc/e500.c
> > > @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
> > >
> > >      /* Fixup Memory size on a alignment boundary */
> > >      ram_size &= ~(RAM_SIZES_ALIGN - 1);
> > > +    /* Sync this for the system. */
> > > +    params->ram_size = ram_size;
> >
> > Could you explain this further?  When does params->ram_size
> > ever get used after this point?
> 
> In that case we have to create a dtb without passing an extra dtb, we  
> always use params->ram_size inside ppce500_load_device_tree(),
> 
> ppce500_load_device_tree()
> {
> ...
> uint64_t mem_reg_property[] = { 0, cpu_to_be64(params->ram_size) };

OK, from reading the patch it looked like this was happening before you  
modify params->ram_size, but it's a separate function that gets called  
later.  The comment doesn't make much sense to me -- it's not passing  
any information back to "the system" (which I'd interpret as generic  
QEMU code, if anything, which is why I thought you were trying to do  
this for the benefit of the caller), just making sure later e500  
platform code does the right thing when generating the device tree.

-Scott

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Chen, Tiejun]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com] 
> Sent: Wednesday, May 01, 2013 7:09 AM
> To: Chen, Tiejun
> Cc: agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct 
> params->ram_size with ram_size
> 
> On 04/30/2013 06:03:54 PM, Chen, Tiejun wrote:
> > > -----Original Message-----
> > > From: Scott Wood [mailto:scottwood@freescale.com]
> > > Sent: Tuesday, April 30, 2013 3:19 AM
> > > To: Chen, Tiejun
> > > Cc: agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> > > Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct
> > > params->ram_size with ram_size
> > >
> > > On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
> > > > We should sync params->ram_size after we fixup memory size on a 
> > > > alignment boundary. Otherwise Guest would exceed the 
> actual memory 
> > > > region.
> > > >
> > > > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> > > > ---
> > > >  hw/ppc/e500.c |    2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 
> c1bdb6b..145da0e
> > > > 100644
> > > > --- a/hw/ppc/e500.c
> > > > +++ b/hw/ppc/e500.c
> > > > @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
> > > >
> > > >      /* Fixup Memory size on a alignment boundary */
> > > >      ram_size &= ~(RAM_SIZES_ALIGN - 1);
> > > > +    /* Sync this for the system. */
> > > > +    params->ram_size = ram_size;
> > >
> > > Could you explain this further?  When does 
> params->ram_size ever get 
> > > used after this point?
> > 
> > In that case we have to create a dtb without passing an 
> extra dtb, we 
> > always use params->ram_size inside ppce500_load_device_tree(),
> > 
> > ppce500_load_device_tree()
> > {
> > ...
> > uint64_t mem_reg_property[] = { 0, cpu_to_be64(params->ram_size) };
> 
> OK, from reading the patch it looked like this was happening 
> before you modify params->ram_size, but it's a separate 

Yes.

> function that gets called later.  The comment doesn't make 

Are you saying I should replace cpu_to_be64(params->ram_size) with cpu_to_be64(ram_size) directly in ppce500_load_device_tree()?

But as I understand we should sync this global argument after we fixup something associated to that since params->ram_size may impact on something else in the future.

Tiejun

> much sense to me -- it's not passing any information back to 
> "the system" (which I'd interpret as generic QEMU code, if 
> anything, which is why I thought you were trying to do this 
> for the benefit of the caller), just making sure later e500 
> platform code does the right thing when generating the device tree.
> 
> -Scott
> 

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Scott Wood]

On 04/30/2013 06:31:41 PM, Chen, Tiejun wrote:
> > function that gets called later.  The comment doesn't make
> 
> Are you saying I should replace cpu_to_be64(params->ram_size) with  
> cpu_to_be64(ram_size) directly in ppce500_load_device_tree()?

No, I'm saying to reword (or eliminate) the confusing comment.

> But as I understand we should sync this global argument after we  
> fixup something associated to that since params->ram_size may impact  
> on something else in the future.

...and we just hope that nothing uses it for anything important before  
the fixup. :-P

-Scott

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Chen, Tiejun]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com] 
> Sent: Wednesday, May 01, 2013 7:36 AM
> To: Chen, Tiejun
> Cc: agraf@suse.de; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct 
> params->ram_size with ram_size
> 
> On 04/30/2013 06:31:41 PM, Chen, Tiejun wrote:
> > > function that gets called later.  The comment doesn't make
> > 
> > Are you saying I should replace cpu_to_be64(params->ram_size) with
> > cpu_to_be64(ram_size) directly in ppce500_load_device_tree()?
> 
> No, I'm saying to reword (or eliminate) the confusing comment.

Okay, I prefer to eliminate that comment since the code itself is clear enough.

Thanks,

Tiejun

> 
> > But as I understand we should sync this global argument 
> after we fixup 
> > something associated to that since params->ram_size may impact on 
> > something else in the future.
> 
> ...and we just hope that nothing uses it for anything 
> important before the fixup. :-P
> 
> -Scott
> 

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Chen, Tiejun]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de] 
> Sent: Tuesday, April 30, 2013 5:53 PM
> To: Scott Wood
> Cc: Chen, Tiejun; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct 
> params->ram_size with ram_size
> 
> 
> On 29.04.2013, at 21:18, Scott Wood wrote:
> 
> > On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
> >> We should sync params->ram_size after we fixup memory size on a 
> >> alignment boundary. Otherwise Guest would exceed the actual memory 
> >> region.
> >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> >> ---
> >> hw/ppc/e500.c |    2 ++
> >> 1 file changed, 2 insertions(+)
> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index c1bdb6b..145da0e 
> >> 100644
> >> --- a/hw/ppc/e500.c
> >> +++ b/hw/ppc/e500.c
> >> @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
> >>     /* Fixup Memory size on a alignment boundary */
> >>     ram_size &= ~(RAM_SIZES_ALIGN - 1);
> >> +    /* Sync this for the system. */
> >> +    params->ram_size = ram_size;
> > 
> > Could you explain this further?  When does params->ram_size 
> ever get used after this point?
> 
> This is the respective code in vl.c:
> 
> >     QEMUMachineInitArgs args = { .ram_size = ram_size,
> >                                  .boot_device = 
> (boot_devices[0] == '\0') ?
> >                                                 
> machine->boot_order :
> >                                                 boot_devices,
> >                                  .kernel_filename = kernel_filename,
> >                                  .kernel_cmdline = kernel_cmdline,
> >                                  .initrd_filename = initrd_filename,
> >                                  .cpu_model = cpu_model };
> >     machine->init(&args);
> 
> 
> After this, args in unused. So as you can see, 
> params->ram_size never gets used after it's read once.
> 
> What bug are you trying to fix? Maybe there really is 

I had an explanation when I reply to Scott, could you take a look at that?

If you have no further objection, I'd like to send next version with withdrawing that comment as Scott suggest.

Tiejun

Re: [Qemu-devel] [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct params->ram_size with ram_size

[Alexander Graf]

On 01.05.2013, at 01:54, Chen, Tiejun wrote:

>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de] 
>> Sent: Tuesday, April 30, 2013 5:53 PM
>> To: Scott Wood
>> Cc: Chen, Tiejun; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [Qemu-ppc] [v1][PATCH 1/1] PPC: e500: correct 
>> params->ram_size with ram_size
>> 
>> 
>> On 29.04.2013, at 21:18, Scott Wood wrote:
>> 
>>> On 04/28/2013 05:30:09 AM, Tiejun Chen wrote:
>>>> We should sync params->ram_size after we fixup memory size on a 
>>>> alignment boundary. Otherwise Guest would exceed the actual memory 
>>>> region.
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>>>> ---
>>>> hw/ppc/e500.c |    2 ++
>>>> 1 file changed, 2 insertions(+)
>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index c1bdb6b..145da0e 
>>>> 100644
>>>> --- a/hw/ppc/e500.c
>>>> +++ b/hw/ppc/e500.c
>>>> @@ -523,6 +523,8 @@ void ppce500_init(PPCE500Params *params)
>>>>    /* Fixup Memory size on a alignment boundary */
>>>>    ram_size &= ~(RAM_SIZES_ALIGN - 1);
>>>> +    /* Sync this for the system. */
>>>> +    params->ram_size = ram_size;
>>> 
>>> Could you explain this further?  When does params->ram_size 
>> ever get used after this point?
>> 
>> This is the respective code in vl.c:
>> 
>>>    QEMUMachineInitArgs args = { .ram_size = ram_size,
>>>                                 .boot_device = 
>> (boot_devices[0] == '\0') ?
>>> 
>> machine->boot_order :
>>>                                                boot_devices,
>>>                                 .kernel_filename = kernel_filename,
>>>                                 .kernel_cmdline = kernel_cmdline,
>>>                                 .initrd_filename = initrd_filename,
>>>                                 .cpu_model = cpu_model };
>>>    machine->init(&args);
>> 
>> 
>> After this, args in unused. So as you can see, 
>> params->ram_size never gets used after it's read once.
>> 
>> What bug are you trying to fix? Maybe there really is 
> 
> I had an explanation when I reply to Scott, could you take a look at that?

I think it makes sense.

> If you have no further objection, I'd like to send next version with withdrawing that comment as Scott suggest.

Yes, please :)


Alex