[Pdbg] [PATCH 26/29] libpdbg/htm: Add POWER8 HTM (both Core and Nest)

[Cyril Bur]

On Mon, 2018-02-12 at 14:21 +1100, Alistair Popple wrote:
> > +static int do_chtm_reset(struct htm *htm, uint64_t *r_base, uint64_t *r_size)
> > +{
> > +	struct htm_status status;
> > +
> > +	if (HTM_ERR(get_status(htm, &status)))
> > +		return -1;
> > +
> > +	if (!is_resetable(&status) || !is_configured(htm)) {
> > +		if (HTM_ERR(configure_chtm(htm))) {
> > +			PR_ERROR("Couldn't setup Core HTM during reset\n");
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	if (HTM_ERR(configure_memory(htm, r_base, r_size)))
> > +		return -1;
> > +
> > +	return 1;
> > +}
> > +
> > +static int do_nhtm_reset(struct htm *htm, uint64_t *r_base, uint64_t *r_size)
> > +{
> > +	struct htm_status status;
> > +
> > +	if (HTM_ERR(get_status(htm, &status)))
> > +		return -1;
> > +
> > +	if (!is_resetable(&status) || !is_configured(htm)) {
> > +		if (HTM_ERR(configure_nhtm(htm))) {
> > +			PR_ERROR("Couldn't setup Nest HTM during reset\n");
> > +			return -1;
> > +		}
> 
> The majority of this code is the same as do_chtm_reset() above. Personally I
> think it might be nicer to have `if (pdbg_target_class_name(target) == "nhtm")
> ...` rather than duplicate the code, but I'll admit this is mostly a personal
> preference.
> 
> > +	}
> > +
> > +	if (HTM_ERR(configure_memory(htm, r_base, r_size)))
> > +		return -1;
> > +
> >  	return 1;
> >  }
> >  
> > @@ -834,28 +915,59 @@ static int do_htm_dump(struct htm *htm, uint64_t size, const char *basename)
> >  	return 1;
> >  }
> >  
> > -static int nhtm_probe(struct pdbg_target *target)
> > +static bool is_debugfs_memtrace_ok(void)
> >  {
> > -	uint64_t val;
> > -
> >  	if (access(DEBUGFS_MEMTRACE, F_OK) != 0) {
> >  		PR_DEBUG("Couldn't open debugfs\n");
> 
> Previously I think PR_DEBUG was defined in the HTM code, is that still the case?
> 

Looks like its declared in target.h now. It might be a bit verbose now?

> Printing messages in the probe functions is problematic as they are called on
> all platforms and are expected to fail on at least some. 

Now that there's a common place that all the htm commands go through in
pdbg I'll move all these kinds of checks to pdbg and not libpdbg. That
makes sense to me.

> Although printing at
> debug level is ok, we just need to make sure it's not enabled by default. And I
> will admit our error logging could use some improvement :-)
> 
> > +struct htm p9nhtm = {
> > +	.target = {
> > +		.name =	"POWER9 Nest HTM",
> > +		.compatible = "ibm,power9-nhtm",
> > +		.class = "nhtm",
> > +		.probe = nhtm_probe,
> > +	},
> > +	.start = do_htm_start,
> > +	.stop = do_htm_stop,
> > +	.reset = do_nhtm_reset,
> > +	.pause = do_htm_pause,
> > +	.status = do_htm_status,
> > +	.dump = do_htm_dump,
> > +};
> > +DECLARE_HW_UNIT(p9nhtm);
> > +
> > +struct htm p8nhtm = {
> >  	.target = {
> > -		.name =	"Nest HTM",
> > -		.compatible = "ibm,nhtm",
> > +		.name = "POWER8 Nest HTM",
> > +		.compatible = "ibm,power8-nhtm",
> >  		.class = "nhtm",
> >  		.probe = nhtm_probe,
> >  	},
> > @@ -866,4 +978,20 @@ struct htm nhtm = {
> >  	.status = do_htm_status,
> >  	.dump = do_htm_dump,
> >  };
> > -DECLARE_HW_UNIT(nhtm);
> > +DECLARE_HW_UNIT(p8nhtm);
> 
> Is there any reason at present to have a POWER8 vs. POWER9 version of these
> classes? I would be quite happy to have a single nhtm class with `compatible =
> "ibm,nhtm", "ibm,power?-nhtm"` to differentiate between the two flavours of
> almost identical hardware interfaces.
> 
> > +
> > +struct htm chtm = {
> > +	.target = {
> > +		.name = "POWER8 Core HTM",
> > +		.compatible = "ibm,power8-chtm",
> > +		.class = "chtm",
> > +		.probe = chtm_probe,
> > +	},
> > +	.start = do_htm_start,
> > +	.stop = do_htm_stop,
> > +	.reset = do_chtm_reset,
> > +	.pause = do_htm_pause,
> > +	.status = do_htm_status,
> > +	.dump = do_htm_dump,
> > +};
> > +DECLARE_HW_UNIT(chtm);
> 
> In fact if you merge the do_?htm_reset() functions this could also be merged as
> another flavour of the hardware with `compatible = "ibm,htm",
> "ibm,power8-html"`. I guess this could make the command parsing harder though,
> so I'm not going to get too hung up on this if you don't think it makes sense.
> 
> - Alistair
> 
> > diff --git a/p8-pib.dts.m4 b/p8-pib.dts.m4
> > index de1d3bf..bdc02c5 100644
> > --- a/p8-pib.dts.m4
> > +++ b/p8-pib.dts.m4
> > @@ -7,6 +7,11 @@ define(`CORE', `core at CORE_BASE($1) {
> >  	compatible = "ibm,power8-core";
> >  	reg = <0x0 HEX(CORE_BASE($1)) 0xfffff>;
> >  	index = <0x$2>;
> > +	chtm at 11000 {
> > +		compatible = "ibm,power8-chtm";
> > +		reg = <0x0 0x11000 0xB>;
> > +		index = <0x0>;
> > +	};
> >  
> >  	THREAD(0);
> >  	THREAD(1);
> > @@ -42,4 +47,10 @@ adu at 2020000 {
> >  	reg = <0x0 0x2020000 0x4>;
> >  };
> >  
> > +nhtm at 2010880 {
> > +	compatible = "ibm,power8-nhtm";
> > +	reg = <0x0 0x2010880 0x8>;
> > +	index = <0x0>;
> > +};
> > +
> >  PROC_CORES;
> > 
> 
>