Re: [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:address@hidden
> > Sent: Tuesday, April 30, 2013 3:19 AM
> > To: Chen, Tiejun
> > Cc: address@hidden; address@hidden; address@hidden
> > 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 <address@hidden>
> > > ---
> > >  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