| Message ID | 20210126171059.307867-4-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/pnv: Misc cleanups | expand |
Bonjour, Cédric. On Tuesday, January 26, 2021 2:10:55 PM -03 Cédric Le Goater wrote: > The current settings are useful to load large kernels (with debug) but > it moves the initrd image in a memory region not protected by > skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will > corrupt the initrd. > > Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > If we want to increase the kernel size limit as commit b45b56baeecd > ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I > think we should add a machine option. Is this a problem on bare-metal as well? I'm wondering if we should address this the other way around by increasing KERNEL_LOAD_SIZE and INITRAMFS_LOAD_SIZE in skiboot to accomodate large kernel and initramfs images. I think Linux debuginfo images won't get smaller with time and, assuming this also happens on bare-metal (I haven't verified), updating skiboot looks more appropriate. Bear in mind that I'm not an skiboot expert, I'm just considering the possibilities. > > hw/ppc/pnv.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 14fc9758a973..e500c2e2437e 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -65,9 +65,9 @@ > #define FW_MAX_SIZE (16 * MiB) > > #define KERNEL_LOAD_ADDR 0x20000000 > -#define KERNEL_MAX_SIZE (256 * MiB) > -#define INITRD_LOAD_ADDR 0x60000000 > -#define INITRD_MAX_SIZE (256 * MiB) > +#define KERNEL_MAX_SIZE (128 * MiB) > +#define INITRD_LOAD_ADDR 0x28000000 > +#define INITRD_MAX_SIZE (128 * MiB) > > static const char *pnv_chip_core_typename(const PnvChip *o) > { Cheers!
On 1/27/21 2:27 AM, Murilo Opsfelder Araújo wrote: > Bonjour, Cédric. > > On Tuesday, January 26, 2021 2:10:55 PM -03 Cédric Le Goater wrote: >> The current settings are useful to load large kernels (with debug) but >> it moves the initrd image in a memory region not protected by >> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will >> corrupt the initrd. >> >> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> >> If we want to increase the kernel size limit as commit b45b56baeecd >> ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I >> think we should add a machine option. > > Is this a problem on bare-metal as well? > > I'm wondering if we should address this the other way around by increasing > KERNEL_LOAD_SIZE and INITRAMFS_LOAD_SIZE in skiboot to accomodate large kernel > and initramfs images. The different memory areas are all strictly defined here : https://github.com/open-power/skiboot/blob/master/include/mem-map.h C. > I think Linux debuginfo images won't get smaller with time and, assuming this > also happens on bare-metal (I haven't verified), updating skiboot looks more > appropriate. > > Bear in mind that I'm not an skiboot expert, I'm just considering the > possibilities. > >> >> hw/ppc/pnv.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index 14fc9758a973..e500c2e2437e 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -65,9 +65,9 @@ >> #define FW_MAX_SIZE (16 * MiB) >> >> #define KERNEL_LOAD_ADDR 0x20000000 >> -#define KERNEL_MAX_SIZE (256 * MiB) >> -#define INITRD_LOAD_ADDR 0x60000000 >> -#define INITRD_MAX_SIZE (256 * MiB) >> +#define KERNEL_MAX_SIZE (128 * MiB) >> +#define INITRD_LOAD_ADDR 0x28000000 >> +#define INITRD_MAX_SIZE (128 * MiB) >> >> static const char *pnv_chip_core_typename(const PnvChip *o) >> { > > Cheers! >
On Tuesday, January 26, 2021 2:10:55 PM -03 Cédric Le Goater wrote: > The current settings are useful to load large kernels (with debug) but > it moves the initrd image in a memory region not protected by > skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will > corrupt the initrd. > > Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > --- > > If we want to increase the kernel size limit as commit b45b56baeecd > ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I > think we should add a machine option. > > hw/ppc/pnv.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 14fc9758a973..e500c2e2437e 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -65,9 +65,9 @@ > #define FW_MAX_SIZE (16 * MiB) > > #define KERNEL_LOAD_ADDR 0x20000000 > -#define KERNEL_MAX_SIZE (256 * MiB) > -#define INITRD_LOAD_ADDR 0x60000000 > -#define INITRD_MAX_SIZE (256 * MiB) > +#define KERNEL_MAX_SIZE (128 * MiB) > +#define INITRD_LOAD_ADDR 0x28000000 > +#define INITRD_MAX_SIZE (128 * MiB) > > static const char *pnv_chip_core_typename(const PnvChip *o) > { -- Murilo
On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote: > > The current settings are useful to load large kernels (with debug) but > it moves the initrd image in a memory region not protected by > skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will > corrupt the initrd. > > Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Joel Stanley <joel@jms.id.au> Note that the machine's default ram size will change with this patch: mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; So we will go from 1.75GB to 768MB. Does anything break when the machine has less than 1GB of ram? > --- > > If we want to increase the kernel size limit as commit b45b56baeecd > ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I > think we should add a machine option. > > hw/ppc/pnv.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 14fc9758a973..e500c2e2437e 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -65,9 +65,9 @@ > #define FW_MAX_SIZE (16 * MiB) > > #define KERNEL_LOAD_ADDR 0x20000000 > -#define KERNEL_MAX_SIZE (256 * MiB) > -#define INITRD_LOAD_ADDR 0x60000000 > -#define INITRD_MAX_SIZE (256 * MiB) > +#define KERNEL_MAX_SIZE (128 * MiB) > +#define INITRD_LOAD_ADDR 0x28000000 > +#define INITRD_MAX_SIZE (128 * MiB) > > static const char *pnv_chip_core_typename(const PnvChip *o) > { > -- > 2.26.2 > >
On Tue, Jan 26, 2021 at 06:10:55PM +0100, Cédric Le Goater wrote: > The current settings are useful to load large kernels (with debug) but > it moves the initrd image in a memory region not protected by > skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will > corrupt the initrd. > > Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > Signed-off-by: Cédric Le Goater <clg@kaod.org> Applied to ppc-for-6.0, thanks. > --- > > If we want to increase the kernel size limit as commit b45b56baeecd > ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I > think we should add a machine option. > > hw/ppc/pnv.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 14fc9758a973..e500c2e2437e 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -65,9 +65,9 @@ > #define FW_MAX_SIZE (16 * MiB) > > #define KERNEL_LOAD_ADDR 0x20000000 > -#define KERNEL_MAX_SIZE (256 * MiB) > -#define INITRD_LOAD_ADDR 0x60000000 > -#define INITRD_MAX_SIZE (256 * MiB) > +#define KERNEL_MAX_SIZE (128 * MiB) > +#define INITRD_LOAD_ADDR 0x28000000 > +#define INITRD_MAX_SIZE (128 * MiB) > > static const char *pnv_chip_core_typename(const PnvChip *o) > {
On 1/28/21 1:45 AM, Joel Stanley wrote: > On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote: >> >> The current settings are useful to load large kernels (with debug) but >> it moves the initrd image in a memory region not protected by >> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will >> corrupt the initrd. >> >> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > Note that the machine's default ram size will change with this patch: > > mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; Ah yes. I missed that. > So we will go from 1.75GB to 768MB. Does anything break when the > machine has less than 1GB of ram? There is a warning if the machine has less than 1GB but we should also change the default RAM size to 1G to be on the safe side. Thanks, C. > >> --- >> >> If we want to increase the kernel size limit as commit b45b56baeecd >> ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I >> think we should add a machine option. >> >> hw/ppc/pnv.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index 14fc9758a973..e500c2e2437e 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -65,9 +65,9 @@ >> #define FW_MAX_SIZE (16 * MiB) >> >> #define KERNEL_LOAD_ADDR 0x20000000 >> -#define KERNEL_MAX_SIZE (256 * MiB) >> -#define INITRD_LOAD_ADDR 0x60000000 >> -#define INITRD_MAX_SIZE (256 * MiB) >> +#define KERNEL_MAX_SIZE (128 * MiB) >> +#define INITRD_LOAD_ADDR 0x28000000 >> +#define INITRD_MAX_SIZE (128 * MiB) >> >> static const char *pnv_chip_core_typename(const PnvChip *o) >> { >> -- >> 2.26.2 >> >>
On Thu, Jan 28, 2021 at 08:02:41AM +0100, Cédric Le Goater wrote: > On 1/28/21 1:45 AM, Joel Stanley wrote: > > On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote: > >> > >> The current settings are useful to load large kernels (with debug) but > >> it moves the initrd image in a memory region not protected by > >> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will > >> corrupt the initrd. > >> > >> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > > > Note that the machine's default ram size will change with this patch: > > > > mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; > > Ah yes. I missed that. > > > So we will go from 1.75GB to 768MB. Does anything break when the > > machine has less than 1GB of ram? > > There is a warning if the machine has less than 1GB but we should > also change the default RAM size to 1G to be on the safe side. I've merged the patch, but I'm happy to replace it with an updated version, or fold in a change, if that helps. > > Thanks, > > C. > > > > >> --- > >> > >> If we want to increase the kernel size limit as commit b45b56baeecd > >> ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I > >> think we should add a machine option. > >> > >> hw/ppc/pnv.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > >> index 14fc9758a973..e500c2e2437e 100644 > >> --- a/hw/ppc/pnv.c > >> +++ b/hw/ppc/pnv.c > >> @@ -65,9 +65,9 @@ > >> #define FW_MAX_SIZE (16 * MiB) > >> > >> #define KERNEL_LOAD_ADDR 0x20000000 > >> -#define KERNEL_MAX_SIZE (256 * MiB) > >> -#define INITRD_LOAD_ADDR 0x60000000 > >> -#define INITRD_MAX_SIZE (256 * MiB) > >> +#define KERNEL_MAX_SIZE (128 * MiB) > >> +#define INITRD_LOAD_ADDR 0x28000000 > >> +#define INITRD_MAX_SIZE (128 * MiB) > >> > >> static const char *pnv_chip_core_typename(const PnvChip *o) > >> { > >> > >> >
On 1/28/21 11:36 PM, David Gibson wrote: > On Thu, Jan 28, 2021 at 08:02:41AM +0100, Cédric Le Goater wrote: >> On 1/28/21 1:45 AM, Joel Stanley wrote: >>> On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater <clg@kaod.org> wrote: >>>> >>>> The current settings are useful to load large kernels (with debug) but >>>> it moves the initrd image in a memory region not protected by >>>> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will >>>> corrupt the initrd. >>>> >>>> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> >>> Reviewed-by: Joel Stanley <joel@jms.id.au> >>> >>> Note that the machine's default ram size will change with this patch: >>> >>> mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE; >> >> Ah yes. I missed that. >> >>> So we will go from 1.75GB to 768MB. Does anything break when the >>> machine has less than 1GB of ram? >> >> There is a warning if the machine has less than 1GB but we should >> also change the default RAM size to 1G to be on the safe side. > > I've merged the patch, but I'm happy to replace it with an updated > version, or fold in a change, if that helps. I will send a little fix to set the default RAM size of the machine. Thanks, C.
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 14fc9758a973..e500c2e2437e 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -65,9 +65,9 @@ #define FW_MAX_SIZE (16 * MiB) #define KERNEL_LOAD_ADDR 0x20000000 -#define KERNEL_MAX_SIZE (256 * MiB) -#define INITRD_LOAD_ADDR 0x60000000 -#define INITRD_MAX_SIZE (256 * MiB) +#define KERNEL_MAX_SIZE (128 * MiB) +#define INITRD_LOAD_ADDR 0x28000000 +#define INITRD_MAX_SIZE (128 * MiB) static const char *pnv_chip_core_typename(const PnvChip *o) {
The current settings are useful to load large kernels (with debug) but it moves the initrd image in a memory region not protected by skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will corrupt the initrd. Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> Signed-off-by: Cédric Le Goater <clg@kaod.org> --- If we want to increase the kernel size limit as commit b45b56baeecd ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I think we should add a machine option. hw/ppc/pnv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)