Re: [Qemu-devel] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2015-09-30 at 12:47 +0100, Peter Maydell wrote:

> >  const MonitorDef *target_monitor_defs(void);
> > +int target_extra_monitor_def(uint64_t *pval, const char *name);
> 
> This would be a good place to put a doc comment documenting
> the semantics of this new hook.
> 
> MonitorDef structs treat the value to be obtained as
> a target_long, but this uses uint64_t, which is a bit
> inconsistent.

I couldn't get the definition of target_ulong in the stubs.

[ Note: Alexey has a different approach which completely replaces
monitor_defs() with a CPU specific one via a method in the CPU class.
I don't care which way you prefer as long as the functionality is there
]

> It might be better to:
>  (a) fix the core monitor code to deal in int64_t rather
>  than target_long
>  (b) consider whether it would be better to have the ppc
>  code generate a bunch of MonitorDef structs to return for the
>  SPRs rather than having an extra hook function

That sounds like the most bloated way to handle this :)

> > --- /dev/null
> > +++ b/stubs/target-extra-monitor-def.c
> > @@ -0,0 +1,10 @@
> > +#include "stddef.h"
> > +#include "qemu/typedefs.h"
> > +#include 
> > +
> > +int target_extra_monitor_def(uint64_t *pval, const char *name);
> > +
> > +int target_extra_monitor_def(uint64_t *pval, const char *name)
> > +{
> > +    return -1;
> > +}
> 
> It would be better to put the prototype for the hook somewhere
> the stub file can include it rather than having it just rewritten
> here.

I just copied the existing practice in get_monitor_defs()... but yes, I
agree.

Ben.

> thanks
> -- PMM