Re: [Xen-devel] [PATCH 3/5] Implement code to read coverage informations

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Wed, 2013-02-06 at 17:12 +0000, Ian Campbell wrote:
> > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> > index 3225b2a..5e80400 100644
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -596,6 +596,42 @@ struct xen_sysctl_scheduler_op {
> >  typedef struct xen_sysctl_scheduler_op xen_sysctl_scheduler_op_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_scheduler_op_t);
> >  
> > +/* XEN_SYSCTL_coverage_op */
> > +/*
> > + * Check if coverage informations are available
> > + * return just success or error, no parameters
> > + */
> > +#define XEN_SYSCTL_COVERAGE_enabled        0
> 
> You can detect this by getting -ENOSYS from the hypercall, which is what
> you would get running the tool on an older hypervisor, so you need to
> handle it anyway.
> 

Yes, I think so. I'll change it. Was here cause on the separate
hypercall was the only operation available to non privileged domains but
since sysctl already check for it there is no clue.

> > +
> > +/*
> > + * Get total size of information, to help allocate
> > + * the buffer. The pointer points to a 32 bit value.
> > + */
> > +#define XEN_SYSCTL_COVERAGE_get_total_size 1
> > +
> > +/*
> > + * Read coverage information in a single run
> > + * You must use a tool to split them
> > + */
> > +#define XEN_SYSCTL_COVERAGE_read           2
> > +
> > +/*
> > + * Reset all the coverage counters to 0
> > + * No parameters.
> > + */
> > +#define XEN_SYSCTL_COVERAGE_reset          3
> 
> Any need for simultaneous read+reset?
> 

Mmmm... possibly could help, I'll add it!

> > +struct xen_sysctl_coverage_op {
> > +    uint32_t cmd;        /* XEN_SYSCTL_COVERAGE_* */
> > +    union {
> > +        XEN_GUEST_HANDLE_64(uint32) total_size; /* OUT */
> 
> This one can just be a uint32_t I think, no need for it to be a
> pointer/handle.
> 

I could, this require to set copyback to 1 in do_sysctl. Yes, it's
probably only a single instruction. It also complicate a bit the program
that extract the information but it's not a problem. And performance are
not a problem too. So, do you prefer I change it to uint32_t directly ?

I don't like the idea to pass a pointer to copyback to
sysctl_coverage_op as this could lead to sub optimization in do_sysctl
(passing a pointer lead to a stack allocation for the variable,
currently compiler can allocate just a register).

> > +        XEN_GUEST_HANDLE_64(uint8)  raw_info;   /* OUT */
> > +    } u;
> > +};
> > +typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
> > +
> > +
> >  struct xen_sysctl {
> >      uint32_t cmd;
> >  #define XEN_SYSCTL_readconsole                    1
> > @@ -616,6 +652,7 @@ struct xen_sysctl {
> >  #define XEN_SYSCTL_numainfo                      17
> >  #define XEN_SYSCTL_cpupool_op                    18
> >  #define XEN_SYSCTL_scheduler_op                  19
> > +#define XEN_SYSCTL_coverage_op                   20
> >      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
> >      union {
> >          struct xen_sysctl_readconsole       readconsole;
> > @@ -636,6 +673,7 @@ struct xen_sysctl {
> >          struct xen_sysctl_lockprof_op       lockprof_op;
> >          struct xen_sysctl_cpupool_op        cpupool_op;
> >          struct xen_sysctl_scheduler_op      scheduler_op;
> > +        struct xen_sysctl_coverage_op       coverage_op;
> >          uint8_t                             pad[128];
> >      } u;
> >  };
> 
> 

Frediano

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel