| Message ID | 20200227204023.22125-10-grimm@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Ultravisor support in skiboot | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (82aed17a5468aff6b600ee1694a10a60f942c018) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On 28/02/2020 07:40, Ryan Grimm wrote: > The secure memory ranges are provided by the hostboot through HDAT. > Skiboot parses HDAT and creates secure-memory@ device tree nodes. > > Check bit 15 when checking for reserves that are too big so we reserve > regions from HB that are in secure memory. > > Signed-off-by: Ryan Grimm <grimm@linux.ibm.com> > --- > hdata/memory.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/hdata/memory.c b/hdata/memory.c > index 9c588ff6..c25bd9c0 100644 > --- a/hdata/memory.c > +++ b/hdata/memory.c > @@ -32,7 +32,7 @@ struct HDIF_ms_area_address_range { > __be64 start; > __be64 end; > __be32 chip; > - __be32 mirror_attr; > + __be32 memory_attr; Why is this renamed? > __be64 mirror_start; and this one is not? > __be32 controller_id; > __be32 phys_attr; > @@ -59,6 +59,9 @@ struct HDIF_ms_area_address_range { > #define MS_CONTROLLER_MCS_ID(id) GETFIELD(PPC_BITMASK32(4, 7), id) > #define MS_CONTROLLER_MCA_ID(id) GETFIELD(PPC_BITMASK32(8, 15), id) > > +#define MS_ATTR_PEF PPC_BIT32(23) > +#define UV_SECURE_MEM_BIT PPC_BIT(15) > + > struct HDIF_ms_area_id { > __be16 id; > #define MS_PTYPE_RISER_CARD 0x8000 > @@ -129,10 +132,10 @@ static bool add_address_range(struct dt_node *root, > chip_id = pcid_to_chip_id(be32_to_cpu(arange->chip)); > > prlog(PR_DEBUG, " Range: 0x%016llx..0x%016llx " > - "on Chip 0x%x mattr: 0x%x pattr: 0x%x status:0x%x\n", > + "on Chip 0x%x memattr: 0x%08x pattr: 0x%x status:0x%x\n", > (long long)be64_to_cpu(arange->start), > (long long)be64_to_cpu(arange->end), > - chip_id, be32_to_cpu(arange->mirror_attr), > + chip_id, be32_to_cpu(arange->memory_attr), > mem_type, mem_status); > > /* reg contains start and length */ > @@ -161,6 +164,13 @@ static bool add_address_range(struct dt_node *root, > return false; > } > > + if (be32_to_cpu(arange->memory_attr) & MS_ATTR_PEF) { > + prlog(PR_DEBUG, "HDAT: Found secure memory\n"); > + name = "secure-memory"; > + dev_type = "secure-memory"; > + compat = "ibm,secure-memory"; > + } > + > if (be16_to_cpu(id->flags) & MS_AREA_SHARED) { > mem = dt_find_by_name_addr(dt_root, name, reg[0]); > if (mem) { > @@ -674,9 +684,10 @@ static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd) > > /* > * Workaround broken HDAT reserve regions which are > - * bigger than 512MB > + * bigger than 512MB and not secure memory > */ > - if ((end_addr - start_addr) > 0x20000000) { > + if (((end_addr - start_addr) > 0x20000000) && > + !(start_addr & UV_SECURE_MEM_BIT)) { Is this a hostboot bug? Where does this broken HDAT come from and can we fix it there? Anyway this bit seems unrelated to the other half of the patch about "secure-memory". > prlog(PR_ERR, "MEM: Ignoring Bad HDAT reserve: too big\n"); > continue; > } >
On Wed, 2020-03-04 at 18:25 +1100, Alexey Kardashevskiy wrote: > > On 28/02/2020 07:40, Ryan Grimm wrote: > > The secure memory ranges are provided by the hostboot through HDAT. > > Skiboot parses HDAT and creates secure-memory@ device tree nodes. > > > > Check bit 15 when checking for reserves that are too big so we > > reserve > > regions from HB that are in secure memory. > > > > Signed-off-by: Ryan Grimm <grimm@linux.ibm.com> > > --- > > hdata/memory.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/hdata/memory.c b/hdata/memory.c > > index 9c588ff6..c25bd9c0 100644 > > --- a/hdata/memory.c > > +++ b/hdata/memory.c > > @@ -32,7 +32,7 @@ struct HDIF_ms_area_address_range { > > __be64 start; > > __be64 end; > > __be32 chip; > > - __be32 mirror_attr; > > + __be32 memory_attr; > > Why is this renamed? > > > __be64 mirror_start; > > and this one is not? The latest version of the HDAT spec renames mirror attributes to memory attributes including mirroring bits like before and also SMF. mirror_start is unchanged, so, I'll try to document this better. -Ryan > > > __be32 controller_id; > > __be32 phys_attr; > > @@ -59,6 +59,9 @@ struct HDIF_ms_area_address_range { > > #define MS_CONTROLLER_MCS_ID(id) GETFIELD(PPC_BITMASK32(4, 7), > > id) > > #define MS_CONTROLLER_MCA_ID(id) GETFIELD(PPC_BITMASK32(8, 15), > > id) > > > > +#define MS_ATTR_PEF PPC_BIT32(23) > > +#define UV_SECURE_MEM_BIT PPC_BIT(15) > > + > > struct HDIF_ms_area_id { > > __be16 id; > > #define MS_PTYPE_RISER_CARD 0x8000 > > @@ -129,10 +132,10 @@ static bool add_address_range(struct dt_node > > *root, > > chip_id = pcid_to_chip_id(be32_to_cpu(arange->chip)); > > > > prlog(PR_DEBUG, " Range: 0x%016llx..0x%016llx " > > - "on Chip 0x%x mattr: 0x%x pattr: 0x%x status:0x%x\n", > > + "on Chip 0x%x memattr: 0x%08x pattr: 0x%x status:0x%x\n", > > (long long)be64_to_cpu(arange->start), > > (long long)be64_to_cpu(arange->end), > > - chip_id, be32_to_cpu(arange->mirror_attr), > > + chip_id, be32_to_cpu(arange->memory_attr), > > mem_type, mem_status); > > > > /* reg contains start and length */ > > @@ -161,6 +164,13 @@ static bool add_address_range(struct dt_node > > *root, > > return false; > > } > > > > + if (be32_to_cpu(arange->memory_attr) & MS_ATTR_PEF) { > > + prlog(PR_DEBUG, "HDAT: Found secure memory\n"); > > + name = "secure-memory"; > > + dev_type = "secure-memory"; > > + compat = "ibm,secure-memory"; > > + } > > + > > if (be16_to_cpu(id->flags) & MS_AREA_SHARED) { > > mem = dt_find_by_name_addr(dt_root, name, reg[0]); > > if (mem) { > > @@ -674,9 +684,10 @@ static void get_hb_reserved_mem(struct > > HDIF_common_hdr *ms_vpd) > > > > /* > > * Workaround broken HDAT reserve regions which are > > - * bigger than 512MB > > + * bigger than 512MB and not secure memory > > */ > > - if ((end_addr - start_addr) > 0x20000000) { > > + if (((end_addr - start_addr) > 0x20000000) && > > + !(start_addr & UV_SECURE_MEM_BIT)) { > > > Is this a hostboot bug? Where does this broken HDAT come from and can > we > fix it there? Anyway this bit seems unrelated to the other half of > the > patch about "secure-memory". > I don't know about broken HDAT, we just piggybacked on this code. Let me check with HB team and make this look better. -Ryan > > > prlog(PR_ERR, "MEM: Ignoring Bad HDAT reserve: > > too big\n"); > > continue; > > } > > > >
On Wed, Mar 4, 2020 at 6:25 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 28/02/2020 07:40, Ryan Grimm wrote: > > The secure memory ranges are provided by the hostboot through HDAT. > > Skiboot parses HDAT and creates secure-memory@ device tree nodes. > > > > Check bit 15 when checking for reserves that are too big so we reserve > > regions from HB that are in secure memory. > > > > Signed-off-by: Ryan Grimm <grimm@linux.ibm.com> > > --- > > hdata/memory.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/hdata/memory.c b/hdata/memory.c > > index 9c588ff6..c25bd9c0 100644 > > --- a/hdata/memory.c > > +++ b/hdata/memory.c > > @@ -32,7 +32,7 @@ struct HDIF_ms_area_address_range { > > __be64 start; > > __be64 end; > > __be32 chip; > > - __be32 mirror_attr; > > + __be32 memory_attr; > > Why is this renamed? > > > __be64 mirror_start; > > and this one is not? > > > __be32 controller_id; > > __be32 phys_attr; > > @@ -59,6 +59,9 @@ struct HDIF_ms_area_address_range { > > #define MS_CONTROLLER_MCS_ID(id) GETFIELD(PPC_BITMASK32(4, 7), id) > > #define MS_CONTROLLER_MCA_ID(id) GETFIELD(PPC_BITMASK32(8, 15), id) > > > > +#define MS_ATTR_PEF PPC_BIT32(23) > > +#define UV_SECURE_MEM_BIT PPC_BIT(15) > > + > > struct HDIF_ms_area_id { > > __be16 id; > > #define MS_PTYPE_RISER_CARD 0x8000 > > @@ -129,10 +132,10 @@ static bool add_address_range(struct dt_node *root, > > chip_id = pcid_to_chip_id(be32_to_cpu(arange->chip)); > > > > prlog(PR_DEBUG, " Range: 0x%016llx..0x%016llx " > > - "on Chip 0x%x mattr: 0x%x pattr: 0x%x status:0x%x\n", > > + "on Chip 0x%x memattr: 0x%08x pattr: 0x%x status:0x%x\n", > > (long long)be64_to_cpu(arange->start), > > (long long)be64_to_cpu(arange->end), > > - chip_id, be32_to_cpu(arange->mirror_attr), > > + chip_id, be32_to_cpu(arange->memory_attr), > > mem_type, mem_status); > > > > /* reg contains start and length */ > > @@ -161,6 +164,13 @@ static bool add_address_range(struct dt_node *root, > > return false; > > } > > > > + if (be32_to_cpu(arange->memory_attr) & MS_ATTR_PEF) { > > + prlog(PR_DEBUG, "HDAT: Found secure memory\n"); > > + name = "secure-memory"; > > + dev_type = "secure-memory"; > > + compat = "ibm,secure-memory"; > > + } > > + > > if (be16_to_cpu(id->flags) & MS_AREA_SHARED) { > > mem = dt_find_by_name_addr(dt_root, name, reg[0]); > > if (mem) { > > @@ -674,9 +684,10 @@ static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd) > > > > /* > > * Workaround broken HDAT reserve regions which are > > - * bigger than 512MB > > + * bigger than 512MB and not secure memory > > */ > > - if ((end_addr - start_addr) > 0x20000000) { > > + if (((end_addr - start_addr) > 0x20000000) && > > + !(start_addr & UV_SECURE_MEM_BIT)) { > > > Is this a hostboot bug? Where does this broken HDAT come from and can we > fix it there? It was a hostboot bug that turned up some time early in p9 bringup that never seemed to get fixed so I worked around it instead. Maybe it's been fixed by now, but honestly I don't really care. > Anyway this bit seems unrelated to the other half of the > patch about "secure-memory". I think it's a carry over from earlier versions of this patchset which put the secure memory ranges into /reserved-memory/ rather than having actual memory nodes. I don't think there's any real need to support giant reservations in secure memory so this should probably be dropped. > > prlog(PR_ERR, "MEM: Ignoring Bad HDAT reserve: too big\n"); > > continue; > > } > > > > -- > Alexey
On Tue, 2020-03-24 at 10:14 +1100, Oliver O'Halloran wrote: > /* > > > * Workaround broken HDAT reserve regions which are > > > - * bigger than 512MB > > > + * bigger than 512MB and not secure memory > > > */ > > > - if ((end_addr - start_addr) > 0x20000000) { > > > + if (((end_addr - start_addr) > 0x20000000) && > > > + !(start_addr & UV_SECURE_MEM_BIT)) { > > > > > > Is this a hostboot bug? Where does this broken HDAT come from and > > can we > > fix it there? > > It was a hostboot bug that turned up some time early in p9 bringup > that never seemed to get fixed so I worked around it instead. Maybe > it's been fixed by now, but honestly I don't really care. > > > Anyway this bit seems unrelated to the other half of the > > patch about "secure-memory". > > I think it's a carry over from earlier versions of this patchset > which > put the secure memory ranges into /reserved-memory/ rather than > having > actual memory nodes. I don't think there's any real need to support > giant reservations in secure memory so this should probably be > dropped. > OK, I thought we needed this but I was reading the workaround incorrectly. I thought it was to workaround regions with starting address beyond 512MB but it's working around regions bigger than 512MB. FYI HB reserves in secure memory are within a 64MB now, not sure how official that value is. So, yeah, this will be dropped. -Ryan > > > prlog(PR_ERR, "MEM: Ignoring Bad HDAT > > > reserve: too big\n"); > > > continue; > > > } > > > > > > > -- > > Alexey
diff --git a/hdata/memory.c b/hdata/memory.c index 9c588ff6..c25bd9c0 100644 --- a/hdata/memory.c +++ b/hdata/memory.c @@ -32,7 +32,7 @@ struct HDIF_ms_area_address_range { __be64 start; __be64 end; __be32 chip; - __be32 mirror_attr; + __be32 memory_attr; __be64 mirror_start; __be32 controller_id; __be32 phys_attr; @@ -59,6 +59,9 @@ struct HDIF_ms_area_address_range { #define MS_CONTROLLER_MCS_ID(id) GETFIELD(PPC_BITMASK32(4, 7), id) #define MS_CONTROLLER_MCA_ID(id) GETFIELD(PPC_BITMASK32(8, 15), id) +#define MS_ATTR_PEF PPC_BIT32(23) +#define UV_SECURE_MEM_BIT PPC_BIT(15) + struct HDIF_ms_area_id { __be16 id; #define MS_PTYPE_RISER_CARD 0x8000 @@ -129,10 +132,10 @@ static bool add_address_range(struct dt_node *root, chip_id = pcid_to_chip_id(be32_to_cpu(arange->chip)); prlog(PR_DEBUG, " Range: 0x%016llx..0x%016llx " - "on Chip 0x%x mattr: 0x%x pattr: 0x%x status:0x%x\n", + "on Chip 0x%x memattr: 0x%08x pattr: 0x%x status:0x%x\n", (long long)be64_to_cpu(arange->start), (long long)be64_to_cpu(arange->end), - chip_id, be32_to_cpu(arange->mirror_attr), + chip_id, be32_to_cpu(arange->memory_attr), mem_type, mem_status); /* reg contains start and length */ @@ -161,6 +164,13 @@ static bool add_address_range(struct dt_node *root, return false; } + if (be32_to_cpu(arange->memory_attr) & MS_ATTR_PEF) { + prlog(PR_DEBUG, "HDAT: Found secure memory\n"); + name = "secure-memory"; + dev_type = "secure-memory"; + compat = "ibm,secure-memory"; + } + if (be16_to_cpu(id->flags) & MS_AREA_SHARED) { mem = dt_find_by_name_addr(dt_root, name, reg[0]); if (mem) { @@ -674,9 +684,10 @@ static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd) /* * Workaround broken HDAT reserve regions which are - * bigger than 512MB + * bigger than 512MB and not secure memory */ - if ((end_addr - start_addr) > 0x20000000) { + if (((end_addr - start_addr) > 0x20000000) && + !(start_addr & UV_SECURE_MEM_BIT)) { prlog(PR_ERR, "MEM: Ignoring Bad HDAT reserve: too big\n"); continue; }
The secure memory ranges are provided by the hostboot through HDAT. Skiboot parses HDAT and creates secure-memory@ device tree nodes. Check bit 15 when checking for reserves that are too big so we reserve regions from HB that are in secure memory. Signed-off-by: Ryan Grimm <grimm@linux.ibm.com> --- hdata/memory.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)