[RFC,v5,09/16] hdata/memory.c: Parse HDAT for secure memory
diff mbox series

Message ID 20200227204023.22125-10-grimm@linux.ibm.com
State New
Headers show
Series
  • Ultravisor support in skiboot
Related show

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (82aed17a5468aff6b600ee1694a10a60f942c018)

Commit Message

Ryan Grimm Feb. 27, 2020, 8:40 p.m. UTC
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(-)

Comments

Alexey Kardashevskiy March 4, 2020, 7:25 a.m. UTC | #1
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;
>  		}
>
Ryan Grimm March 23, 2020, 3:38 p.m. UTC | #2
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;
> >  		}
> > 
> 
>
Oliver O'Halloran March 23, 2020, 11:14 p.m. UTC | #3
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
Ryan Grimm March 30, 2020, 4:22 p.m. UTC | #4
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

Patch
diff mbox series

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;
 		}