Re: [kvm-unit-tests PATCH 11/14] powerpc/ppc64: add rtas_power_off

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5278 bytes --]

On Tue, Aug 04, 2015 at 09:54:44AM +0200, Andrew Jones wrote:
> On Tue, Aug 04, 2015 at 02:11:30PM +1000, David Gibson wrote:
> > On Mon, Aug 03, 2015 at 04:41:28PM +0200, Andrew Jones wrote:
> > > Add enough RTAS support to support power-off, and apply it to
> > > exit().
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  lib/powerpc/asm/rtas.h  | 27 ++++++++++++++++
> > >  lib/powerpc/io.c        |  3 ++
> > >  lib/powerpc/rtas.c      | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/ppc64/asm/rtas.h    |  1 +
> > >  powerpc/Makefile.common |  1 +
> > >  powerpc/cstart64.S      |  9 ++++++
> > >  6 files changed, 125 insertions(+)
> > >  create mode 100644 lib/powerpc/asm/rtas.h
> > >  create mode 100644 lib/powerpc/rtas.c
> > >  create mode 100644 lib/ppc64/asm/rtas.h
> > > 
> > > diff --git a/lib/powerpc/asm/rtas.h b/lib/powerpc/asm/rtas.h
> > > new file mode 100644
> > > index 0000000000000..53ce126c69a42
> > > --- /dev/null
> > > +++ b/lib/powerpc/asm/rtas.h
> > > @@ -0,0 +1,27 @@
> > > +#ifndef _ASMPOWERPC_RTAS_H_
> > > +#define _ASMPOWERPC_RTAS_H_
> > > +/*
> > > + * Copyright (C) 2015, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > > + */
> > > +#include <libcflat.h>
> > > +
> > > +#define RTAS_UNKNOWN_SERVICE	(-1)
> > > +
> > > +struct rtas_args {
> > > +	u32 token;
> > > +	u32 nargs;
> > > +	u32 nret;
> > > +	u32 args[16];
> > > +	u32 *rets;
> > > +};
> > > +
> > > +extern void rtas_init(void);
> > > +extern void enter_rtas(unsigned long args);
> > > +extern int rtas_token(const char *service);
> > > +extern int rtas_call(int token, int nargs, int nret, int *outputs, ...);
> > > +
> > > +extern void rtas_power_off(void);
> > > +
> > > +#endif /* _ASMPOWERPC_RTAS_H_ */
> > > diff --git a/lib/powerpc/io.c b/lib/powerpc/io.c
> > > index a7eaafeca9205..25cdd0ad961ee 100644
> > > --- a/lib/powerpc/io.c
> > > +++ b/lib/powerpc/io.c
> > > @@ -7,6 +7,7 @@
> > >   */
> > >  #include <libcflat.h>
> > >  #include <asm/spinlock.h>
> > > +#include <asm/rtas.h>
> > >  
> > >  extern void halt(int code);
> > >  extern void putchar(int c);
> > > @@ -15,6 +16,7 @@ static struct spinlock uart_lock;
> > >  
> > >  void io_init(void)
> > >  {
> > > +	rtas_init();
> > >  }
> > >  
> > >  void puts(const char *s)
> > > @@ -27,5 +29,6 @@ void puts(const char *s)
> > >  
> > >  void exit(int code)
> > >  {
> > > +	rtas_power_off();
> > >  	halt(code);
> > >  }
> > > diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c
> > > new file mode 100644
> > > index 0000000000000..39f7d4428ee77
> > > --- /dev/null
> > > +++ b/lib/powerpc/rtas.c
> > > @@ -0,0 +1,84 @@
> > > +/*
> > > + * powerpc RTAS
> > > + *
> > > + * Copyright (C) 2015, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > > + */
> > > +#include <libcflat.h>
> > > +#include <libfdt/libfdt.h>
> > > +#include <devicetree.h>
> > > +#include <asm/spinlock.h>
> > > +#include <asm/io.h>
> > > +#include <asm/rtas.h>
> > > +
> > > +static struct spinlock lock;
> > > +struct rtas_args rtas_args;
> > > +static int rtas_node;
> > > +
> > > +void rtas_init(void)
> > > +{
> > > +	if (!dt_available()) {
> > > +		printf("%s: No device tree!\n", __func__);
> > > +		abort();
> > > +	}
> > > +
> > > +	rtas_node = fdt_path_offset(dt_fdt(), "/rtas");
> > > +	if (rtas_node < 0) {
> > > +		printf("%s: /rtas node: %s\n", __func__,
> > > +			fdt_strerror(rtas_node));
> > > +		abort();
> > > +	}
> > > +}
> > > +
> > > +int rtas_token(const char *service)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	u32 *token;
> > > +
> > > +	prop = fdt_get_property(dt_fdt(), rtas_node, service, NULL);
> > 
> > Oh.. one other thing here.
> > 
> > This is only safe if you never alter the device tree blob you're given
> > (or at least, not after rtas_init()).  That may well be the case, but
> > I don't know your code well enough to be sure.
> > 
> > Otherwise, the rtas node could get moved by other dt changes, meaning
> > the offset stored in rtas_node is no longer valid.
> 
> I hadn't planned on modifying the DT, but if you can conceive of a test
> they may want to, then we should do this right.

That's probably ok then, as long as it's made clear that you can't
mess with the DT.  Or if tests might want to modify it, you could give
them their own copy of it.

>I guess doing it right
> just means to hunt down rtas_node each time, that's easy.

That's right.

Not having persistent handles is a bit of a pain, but it's the
necessary tradeoff to work with the tree in flattened form without
having some complicated pile of "context" state to go with it.
(Attempting to remind people that these aren't persistent handles is,
incidentally, why so many of the libfdt functions explicitly mention
"offset" in their names).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]