| Message ID | 20190616171024.22799-10-hegdevasant@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MPIPL support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (dbf27b6c4af84addb36bd3be34f96580aba9c873) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | fail | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
All the previous firmware related patches I don't know enough about, so hopefully someone else can review them. Vasant Hegde's on June 17, 2019 3:10 am: > We use MPIPL system parameter to detect whether MPIPL is supported or not. > If its supported create new device tree node (/ibm,opal/dump) to pass all > dump related information to kernel. This patch creates new node and populates > below properties: > - compatible - dump version (ibm,opal-dump) I guess this seems like a reasonable way to advertise it to the OS. > - fw-load-area - Memory used by OPAL to load kernel/initrd from PNOR > (KERNEL_LOAD_BASE & INITRAMFS_LOAD_BASE) > This is the temporary memory used by OPAL during boot. > Later Linux kernel is free to use this memory. We will > pass this information to Linux. If Linux kernel is using > these memory, it will make sure to create destination > memory to preserve content during MPIPL. This is difficult to understand what to do, let alone why. Can you re-word it? If you have documented the dt changes anywhere, you could re-use your documentation here (or point to the documentation in the changelog). When you write the documentation, you should think of some poor OS developer who has no idea about what hostboot or FSP or spira means -- could a FreeBSD developer read your docs and add fadump support for their OS? Am I understanding it correctly, the OS is not allowed to overwrite some of this memory if it wants to use MPIPL? And if it does then it should somehow preserve it? Can you just mandate that the memory doesn't get used in that case? And add it to OPAL's general firmware memory range code? > > Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> > --- > hdata/spira.c | 24 ++++++++++++++++++++++++ > hdata/spira.h | 1 + > 2 files changed, 25 insertions(+) > > diff --git a/hdata/spira.c b/hdata/spira.c > index b5ec20db9..d2ad8e168 100644 > --- a/hdata/spira.c > +++ b/hdata/spira.c > @@ -934,6 +934,26 @@ static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) > dt_add_property_cells(node, "hw-key-hash-size", hw_key_hash_size); > } > > +static void add_opal_dump_node(void) > +{ > + u64 fw_load_area[4]; > + struct dt_node *node; > + > + if (proc_gen < proc_gen_p9) > + return; Already have a test for SYS_ATTR_MPIPL_SUPPORTED so this shouldn't be necessary. > + > + opal_node = dt_new_check(dt_root, "ibm,opal"); > + node = dt_new(opal_node, "dump"); > + assert(node); > + dt_add_property_string(node, "compatible", "ibm,opal-dump"); > + > + fw_load_area[0] = cpu_to_be64((u64)KERNEL_LOAD_BASE); > + fw_load_area[1] = cpu_to_be64(KERNEL_LOAD_SIZE); > + fw_load_area[2] = cpu_to_be64((u64)INITRAMFS_LOAD_BASE); > + fw_load_area[3] = cpu_to_be64(INITRAMFS_LOAD_SIZE); > + dt_add_property(node, "fw-load-area", fw_load_area, sizeof(fw_load_area)); This is a nasty opaque kind of data structure that's best to be avoided if possible (or well documented, and documentation should be added with the patch that adds the dt IMO). reserved-memory device tree on the other hand is self descriptive and well used and understood. Thanks, Nick
On 06/28/2019 06:54 AM, Nicholas Piggin wrote: > All the previous firmware related patches I don't know enough about, > so hopefully someone else can review them. > > Vasant Hegde's on June 17, 2019 3:10 am: >> We use MPIPL system parameter to detect whether MPIPL is supported or not. >> If its supported create new device tree node (/ibm,opal/dump) to pass all >> dump related information to kernel. This patch creates new node and populates >> below properties: >> - compatible - dump version (ibm,opal-dump) > > I guess this seems like a reasonable way to advertise it to the OS. > >> - fw-load-area - Memory used by OPAL to load kernel/initrd from PNOR >> (KERNEL_LOAD_BASE & INITRAMFS_LOAD_BASE) >> This is the temporary memory used by OPAL during boot. >> Later Linux kernel is free to use this memory. We will >> pass this information to Linux. If Linux kernel is using >> these memory, it will make sure to create destination >> memory to preserve content during MPIPL. > > This is difficult to understand what to do, let alone why. > > Can you re-word it? If you have documented the dt changes anywhere, Sure. I will try to rephrase. > you could re-use your documentation here (or point to the documentation > in the changelog). When you write the documentation, you should think of > some poor OS developer who has no idea about what hostboot or FSP or > spira means -- could a FreeBSD developer read your docs and add fadump > support for their OS? OS developer shouldn't be worried about internal behaviour of OPAL -OR- how it interacts with FSP/BMC etc. All he should care about is device tree details and OPAL APIs. And we have to make sure we have enough documentation around that. > > Am I understanding it correctly, the OS is not allowed to overwrite some > of this memory if it wants to use MPIPL? And if it does then it should > somehow preserve it? Yes. OPAL tells Linux that these are the memory area we may stomp during boot process. If you care about these memory to generate proper dump then you have to preserve it (basically kernel has to allocate destination memory to preserve it and add entry to MPIPL table). > > Can you just mandate that the memory doesn't get used in that case? And > add it to OPAL's general firmware memory range code? So these are the memory OPAL uses to load petitboot kernel/initrd from BMC. Its one time activity during boot. Hence I don't think we have to add/reserve this memory. We will publish the range. Let kernel deal with that. > >> >> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> >> --- >> hdata/spira.c | 24 ++++++++++++++++++++++++ >> hdata/spira.h | 1 + >> 2 files changed, 25 insertions(+) >> >> diff --git a/hdata/spira.c b/hdata/spira.c >> index b5ec20db9..d2ad8e168 100644 >> --- a/hdata/spira.c >> +++ b/hdata/spira.c >> @@ -934,6 +934,26 @@ static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) >> dt_add_property_cells(node, "hw-key-hash-size", hw_key_hash_size); >> } >> >> +static void add_opal_dump_node(void) >> +{ >> + u64 fw_load_area[4]; >> + struct dt_node *node; >> + >> + if (proc_gen < proc_gen_p9) >> + return; > > Already have a test for SYS_ATTR_MPIPL_SUPPORTED so this shouldn't be > necessary. Probably. But then we are trusting HDAT too much ;-) > >> + >> + opal_node = dt_new_check(dt_root, "ibm,opal"); >> + node = dt_new(opal_node, "dump"); >> + assert(node); >> + dt_add_property_string(node, "compatible", "ibm,opal-dump"); >> + >> + fw_load_area[0] = cpu_to_be64((u64)KERNEL_LOAD_BASE); >> + fw_load_area[1] = cpu_to_be64(KERNEL_LOAD_SIZE); >> + fw_load_area[2] = cpu_to_be64((u64)INITRAMFS_LOAD_BASE); >> + fw_load_area[3] = cpu_to_be64(INITRAMFS_LOAD_SIZE); >> + dt_add_property(node, "fw-load-area", fw_load_area, sizeof(fw_load_area)); > > This is a nasty opaque kind of data structure that's best to be avoided > if possible (or well documented, and documentation should be added with > the patch that adds the dt IMO). I do have documentation at the end. I will update it here as well. -Vasant
diff --git a/hdata/spira.c b/hdata/spira.c index b5ec20db9..d2ad8e168 100644 --- a/hdata/spira.c +++ b/hdata/spira.c @@ -934,6 +934,26 @@ static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams) dt_add_property_cells(node, "hw-key-hash-size", hw_key_hash_size); } +static void add_opal_dump_node(void) +{ + u64 fw_load_area[4]; + struct dt_node *node; + + if (proc_gen < proc_gen_p9) + return; + + opal_node = dt_new_check(dt_root, "ibm,opal"); + node = dt_new(opal_node, "dump"); + assert(node); + dt_add_property_string(node, "compatible", "ibm,opal-dump"); + + fw_load_area[0] = cpu_to_be64((u64)KERNEL_LOAD_BASE); + fw_load_area[1] = cpu_to_be64(KERNEL_LOAD_SIZE); + fw_load_area[2] = cpu_to_be64((u64)INITRAMFS_LOAD_BASE); + fw_load_area[3] = cpu_to_be64(INITRAMFS_LOAD_SIZE); + dt_add_property(node, "fw-load-area", fw_load_area, sizeof(fw_load_area)); +} + static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) { const struct iplparams_sysparams *p; @@ -1021,6 +1041,10 @@ static void add_iplparams_sys_params(const void *iplp, struct dt_node *node) if (sys_attributes & SYS_ATTR_RISK_LEVEL) dt_add_property(node, "elevated-risk-level", NULL, 0); + /* Populate OPAL dump node */ + if (sys_attributes & SYS_ATTR_MPIPL_SUPPORTED) + add_opal_dump_node(); + if (version >= 0x60 && proc_gen >= proc_gen_p9) dt_init_secureboot_node(p); } diff --git a/hdata/spira.h b/hdata/spira.h index 84bbcfee0..d29632821 100644 --- a/hdata/spira.h +++ b/hdata/spira.h @@ -364,6 +364,7 @@ struct iplparams_sysparams { __be32 sys_eco_mode; #define SYS_ATTR_MULTIPLE_TPM PPC_BIT32(0) #define SYS_ATTR_RISK_LEVEL PPC_BIT32(3) +#define SYS_ATTR_MPIPL_SUPPORTED PPC_BIT32(4) __be32 sys_attributes; __be32 mem_scrubbing; __be16 cur_spl_value;
We use MPIPL system parameter to detect whether MPIPL is supported or not. If its supported create new device tree node (/ibm,opal/dump) to pass all dump related information to kernel. This patch creates new node and populates below properties: - compatible - dump version (ibm,opal-dump) - fw-load-area - Memory used by OPAL to load kernel/initrd from PNOR (KERNEL_LOAD_BASE & INITRAMFS_LOAD_BASE) This is the temporary memory used by OPAL during boot. Later Linux kernel is free to use this memory. We will pass this information to Linux. If Linux kernel is using these memory, it will make sure to create destination memory to preserve content during MPIPL. Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> --- hdata/spira.c | 24 ++++++++++++++++++++++++ hdata/spira.h | 1 + 2 files changed, 25 insertions(+)