Re: [kvm-unit-tests PATCH] arm64: Add mmio_addr arg to arm/micro-bench

[Ricardo Koller <ricarkol@google.com>]

On Fri, Oct 08, 2021 at 08:28:55AM +0200, Andrew Jones wrote:
> On Thu, Oct 07, 2021 at 11:32:30AM -0700, Ricardo Koller wrote:
> > Add a command line arg to arm/micro-bench to set the mmio_addr to other
> > values besides the default QEMU one. Default to the QEMU value if no arg
> > is passed. Also, the <NUM> in mmio_addr=<NUM> can't be a hex.
> 
> I'll send this patch
> 
> diff --git a/lib/util.c b/lib/util.c
> index a90554138952..682ca2db09e6 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -4,6 +4,7 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  #include <libcflat.h>
> +#include <stdlib.h>
>  #include "util.h"
>  
>  int parse_keyval(char *s, long *val)
> @@ -14,6 +15,6 @@ int parse_keyval(char *s, long *val)
>         if (!p)
>                 return -1;
>  
> -       *val = atol(p+1);
> +       *val = strtol(p+1, NULL, 0);
>         return p - s;
>  }
> 
> 
> which ought to improve that.
> 

Nice! thanks, just sent a V2 that uses it.

> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/micro-bench.c | 33 +++++++++++++++++++++++++++------
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> > index 8e1d4ab..2c08813 100644
> > --- a/arm/micro-bench.c
> > +++ b/arm/micro-bench.c
> > @@ -19,16 +19,19 @@
> >   * This work is licensed under the terms of the GNU LGPL, version 2.
> >   */
> >  #include <libcflat.h>
> > +#include <util.h>
> >  #include <asm/gic.h>
> >  #include <asm/gic-v3-its.h>
> >  #include <asm/timer.h>
> >  
> > -#define NS_5_SECONDS (5 * 1000 * 1000 * 1000UL)
> > +#define NS_5_SECONDS		(5 * 1000 * 1000 * 1000UL)
> > +#define QEMU_MMIO_ADDR		0x0a000008
> >  
> >  static u32 cntfrq;
> >  
> >  static volatile bool irq_ready, irq_received;
> >  static int nr_ipi_received;
> > +static unsigned long mmio_addr = QEMU_MMIO_ADDR;
> >  
> >  static void *vgic_dist_base;
> >  static void (*write_eoir)(u32 irqstat);
> > @@ -278,12 +281,10 @@ static void *userspace_emulated_addr;
> >  static bool mmio_read_user_prep(void)
> >  {
> >  	/*
> > -	 * FIXME: Read device-id in virtio mmio here in order to
> > -	 * force an exit to userspace. This address needs to be
> > -	 * updated in the future if any relevant changes in QEMU
> > -	 * test-dev are made.
> 
> I think the FIXME is still relavent, even with the user given control over
> the address. The FIXME text could be improved though, because it's not
> clear what it's trying to say, which is
> 
>  /*
>   * FIXME: We need an MMIO address that we can safely read to test
>   * exits to userspace. Ideally, the test-dev would provide us this
>   * address (and one we could write to too), but until it does we
>   * use a virtio-mmio transport address. FIXME2: We should be getting
>   * this address (and the future test-dev address) from the devicetree,
>   * but so far we lazily hardcode it.
>   */
>

ACK

> > +	 * Read device-id in virtio mmio here in order to
> > +	 * force an exit to userspace.
> >  	 */
> > -	userspace_emulated_addr = (void*)ioremap(0x0a000008, sizeof(u32));
> > +	userspace_emulated_addr = (void *)ioremap(mmio_addr, sizeof(u32));
> >  	return true;
> >  }
> >  
> > @@ -378,10 +379,30 @@ static void loop_test(struct exit_test *test)
> >  		test->name, total_ns.ns, total_ns.ns_frac, avg_ns.ns, avg_ns.ns_frac);
> >  }
> >  
> > +static void parse_args(int argc, char **argv)
> > +{
> > +	int i, len;
> > +	long val;
> > +
> > +	for (i = 0; i < argc; ++i) {
>                  ^ should be 1, since we know argv[0] is prognam
> 

ACK

> > +		len = parse_keyval(argv[i], &val);
> > +		if (len == -1)
> > +			continue;
> > +
> > +		argv[i][len] = '\0';
> 
> Not nice to write to the global args, especially when we're not yet sure
> if this arg is the one we care about.
> 

ACK, fixing that.

> > +		if (strcmp(argv[i], "mmio-addr") == 0) {
> 
> Can use strncmp to avoid the not nice args write,
> 
>  strncmp(argv[i], "mmio-addr", len)
> 

ACK, will use strncmp instead.

> > +			mmio_addr = val;
> > +			report_info("found mmio_addr=0x%lx", mmio_addr);
> > +		}
> > +	}
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >  	int i;
> >  
> > +	parse_args(argc, argv);
> > +
> >  	if (!test_init())
> >  		return 1;
> >  
> > -- 
> > 2.33.0.882.g93a45727a2-goog
> >
> 
> Thanks,
> drew 
> 

Thanks,
Ricardo