[Skiboot] [RESEND PATCH 01/11]ibm-fsp/firenze: nest data structure definitions

[Stewart Smith]

Madhavan Srinivasan <maddy at linux.vnet.ibm.com> writes:
> Also, I missing your review comments mail on patch 2 of this series,
> I could see it in the mailing list archives, can you forward it?

Sure, here it is:

  Madhavan Srinivasan <maddy at linux.vnet.ibm.com> writes:

  > Patch adds nest init function and Nest meta-data load functions.
  > Nest events supported by HW are passed as lid (meta-data), called as
  [ 79 more citation lines. Click/Enter to show. ]
  > "catalog"
  > "Catalog" lid is loaded to detect chip nest instrumentation support.
  > New file call "nest.c" added to hw/ to contain nest support code.
  >
  > Signed-off-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
  > ---
  >  hw/fsp/fsp.c       |   3 ++
  >  hw/nest.c          | 107
  > +++++++++++++++++++++++++++++++++++++++++++++++++++++
  >  include/mem-map.h  |   2 +
  >  include/nest.h     |  10 +++++
  >  include/platform.h |   1 +
  >  include/types.h    |   1 +
  >  6 files changed, 124 insertions(+)
  >  create mode 100644 hw/nest.c
  >
  > diff --git a/hw/fsp/fsp.c b/hw/fsp/fsp.c
  > index 12e162d..6b5d87c 100644
  > --- a/hw/fsp/fsp.c
  > +++ b/hw/fsp/fsp.c
  > @@ -38,6 +38,7 @@
  >  #include <opal.h>
  >  #include <opal-msg.h>
  >  #include <ccan/list/list.h>
  > +#include <chip.h>
  >
  >  DEFINE_LOG_ENTRY(OPAL_RC_FSP_POLL_TIMEOUT, OPAL_PLATFORM_ERR_EVT,
  > OPAL_FSP,
  >  		 OPAL_PLATFORM_FIRMWARE, OPAL_ERROR_PANIC, OPAL_NA, NULL);
  > @@ -106,6 +107,7 @@ static u64 fsp_hir_timeout;
  >  #define KERNEL_LID_PHYP			0x80a00701
  >  #define KERNEL_LID_OPAL			0x80f00101
  >  #define INITRAMFS_LID_OPAL		0x80f00102
  > +#define NEST_CATALOGUE_LID		0x81e00610
  >
  >  /*
  >   * We keep track on last logged values for some things to print only on
  > @@ -2222,6 +2224,7 @@ static struct {
  >  	{ RESOURCE_ID_CAPP,	CAPP_IDX_MURANO_DD21,	0x80a02001 },
  >  	{ RESOURCE_ID_CAPP,	CAPP_IDX_VENICE_DD10,	0x80a02003 },
  >  	{ RESOURCE_ID_CAPP,	CAPP_IDX_VENICE_DD20,	0x80a02004 },
  > +	{ RESOURCE_ID_CATALOGUE,RESOURCE_SUBID_NONE,	NEST_CATALOGUE_LID},
  >  };
  >
  >  static void fsp_start_fetching_next_lid(void);
  > diff --git a/hw/nest.c b/hw/nest.c
  > new file mode 100644
  > index 0000000..fd22a0e
  > --- /dev/null
  > +++ b/hw/nest.c
  > @@ -0,0 +1,107 @@
  > +/* Copyright 2015 IBM Corp.
  > + *
  > + * Licensed under the Apache License, Version 2.0 (the "License");
  > + * you may not use this file except in compliance with the License.
  > + * You may obtain a copy of the License at
  > + *
  > + *	http://www.apache.org/licenses/LICENSE-2.0
  > + *
  > + * Unless required by applicable law or agreed to in writing, software
  > + * distributed under the License is distributed on an "AS IS" BASIS,
  > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
  > + * implied.
  > + * See the License for the specific language governing permissions and
  > + * limitations under the License.
  > + * Locking notes:
  > + */
  > +
  > +#include <skiboot.h>
  > +#include <device.h>
  > +#include <nest.h>
  > +#include <chip.h>
  > +#include <cpu.h>
  > +#include <xscom.h>
  > +#include <timebase.h>
  > +#include <mem-map.h>
  > +#include <opal-api.h>
  > +#include <io.h>
  > +
  > +/*
  > + * Optimize different array/index
  > + */

  I don't understand what this comment means.

  > +struct ima_catalog_page_0 *page0_ptr;
  > +struct page0_offsets *pg0_offsets;

  what does page0 mean?

  is ima a well known abbreviation?

  > +
  > +int load_catalogue_lid()
  [ 21 more citation lines. Click/Enter to show. ]
  > +{
  > +	struct proc_chip *chip = get_chip(pir_to_chip_id(this_cpu()->pir));
  > +	size_t size = NEST_CATALOGUE_SIZE;
  > +	int rc=0, loaded;
  > +
  > +	pg0_offsets = (struct page0_offsets *)malloc(
  > +					sizeof(struct page0_offsets));
  > +	if (!pg0_offsets) {
  > +		prerror("Nest_IMA: No mem for pg0_offsets structure\n");;
  > +		rc = OPAL_NO_MEM;
  > +		return rc;
  > +	}
  > +
  > +	pg0_offsets->page0 = (char *)malloc(NEST_CATALOGUE_SIZE);
  > +	if (!pg0_offsets->page0) {
  > +		prerror("Nest_IMA: No mem for catalogue lid \n");
  > +		rc = OPAL_NO_MEM;
  > +		return rc;
  > +	}
  > +
  > +	loaded = start_preload_resource (RESOURCE_ID_CATALOGUE,
  > +						RESOURCE_SUBID_NONE,
  > +						pg0_offsets->page0,
  > &size);

  No space after function name.

  > +	if (loaded == OPAL_SUCCESS)
  > +		loaded = wait_for_resource_loaded(RESOURCE_ID_CATALOGUE,
  > +
  > RESOURCE_SUBID_NONE);

  Doing an immediate wait_for after start_preload means you're doing a
  synchronous load rather than an asynchronous one.

  This is generally frowned upon as it increases boot time.

  Instead, start preload early on and then wait_for only when you
  absolutely cannot wait any longer.

  You may also want to check the order in which things are queued to load,
  as this can affect boot time (e.g. if you register your preload after
  kernel and initramfs, you may be waiting longer than you like).

  > +
  > +/*
  > + * powerpc Nest instrumentation support
  > + */

  don't need the comment

  > +void nest_ima_init(void)
  > +{
  > +	if (load_catalogue_lid()) {
  > +		printf("IMA Catalog lid failed to load, Exiting \n");
  > +		return;

  We're not exiting though, just skipping nest instrumentation.

  > diff --git a/include/mem-map.h b/include/mem-map.h
  > index 1258d87..8bd8054 100644
  [ 5 more citation lines. Click/Enter to show. ]
  > --- a/include/mem-map.h
  > +++ b/include/mem-map.h
  > @@ -121,5 +121,7 @@
  >  /* Size allocated to build the device-tree */
  >  #define	DEVICE_TREE_MAX_SIZE	0x80000
  >
  > +/*Size of IMA Catalogue LID. 256KBytes. Fixed */
  > +#define NEST_CATALOGUE_SIZE		0x40000

  I don't see why this should be in mem-map.h rather than in nest.c