From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:59811)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <benh@kernel.crashing.org>) id 1ZhOBu-0005n8-5U
	for qemu-devel@nongnu.org; Wed, 30 Sep 2015 16:41:31 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <benh@kernel.crashing.org>) id 1ZhOBq-0005Yr-42
	for qemu-devel@nongnu.org; Wed, 30 Sep 2015 16:41:30 -0400
Message-ID: <1443645669.2828.19.camel@kernel.crashing.org>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 01 Oct 2015 06:41:09 +1000
In-Reply-To: <CAFEAcA_T0p2DkUdpB-L1vyWhJgsnVu-YFKqt9UUifCFqWtMgAg@mail.gmail.com>
References: <1443335476.4610.2.camel@kernel.crashing.org>
	<CAFEAcA_T0p2DkUdpB-L1vyWhJgsnVu-YFKqt9UUifCFqWtMgAg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC/PATCH] monitor/ppc: Access all SPRs from the
	monitor
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org>, Alexander Graf <agraf@suse.de>

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