| Message ID | 1367145009-14512-1-git-send-email-tiejun.chen@windriver.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
> -----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
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
> -----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 >
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
> -----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 >
> -----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
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
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);
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(+)