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

login
register
mail settings
Submitter Tiejun Chen
Date April 28, 2013, 10:30 a.m.
Message ID <1367145009-14512-1-git-send-email-tiejun.chen@windriver.com>
Download mbox | patch
Permalink /patch/240257/
State New
Headers show

Comments

Tiejun Chen - April 28, 2013, 10:30 a.m.
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(+)
Scott Wood - April 29, 2013, 7:18 p.m.
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
Alexander Graf - April 30, 2013, 9:53 a.m.
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
Andreas Färber - April 30, 2013, 12:04 p.m.
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
Tiejun Chen - April 30, 2013, 11:03 p.m.
> -----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
Scott Wood - April 30, 2013, 11:08 p.m.
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
Tiejun Chen - April 30, 2013, 11:31 p.m.
> -----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
>
Scott Wood - April 30, 2013, 11:36 p.m.
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
Tiejun Chen - April 30, 2013, 11:46 p.m.
> -----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
>
Tiejun Chen - April 30, 2013, 11:54 p.m.
> -----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
Alexander Graf - May 1, 2013, 12:29 a.m.
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

Patch

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);