Re: [PATCH v4 1/5] x86: allow reading MSR_IA32_TSC with XENPF_resource_op

[Chao Peng <chao.p.peng@linux.intel.com>]

On Tue, Jan 20, 2015 at 10:02:32AM +0000, Jan Beulich wrote:
> >>> On 20.01.15 at 10:45, <chao.p.peng@linux.intel.com> wrote:
> > Memory bandwidth monitoring requires timestamp returned along with the
> > monitoring counter to verify the correctness of the counter value.
> > Allow MSR_IA32_TSC to be read and increase RESOURCE_ACCESS_MAX_ENTRIES
> > to 3.
> 
> Do you really need an MSR access here, i.e. is RDTSC not
> sufficient?

RDTSC is enough. The purpose of using MSR is to reuse the existed
XEN_RESOURCE_OP_MSR_READ command. A new command XEN_RESOURCE_OP_TSC_READ
(looks like not so generic) is needed If using RDTSC. If it’s not the
issue, then RDTSC can be used instead.

> 
> > --- a/xen/arch/x86/platform_hypercall.c
> > +++ b/xen/arch/x86/platform_hypercall.c
> > @@ -61,14 +61,14 @@ long cpu_down_helper(void *data);
> >  long core_parking_helper(void *data);
> >  uint32_t get_cur_idle_nums(void);
> >  
> > -#define RESOURCE_ACCESS_MAX_ENTRIES 2
> > +#define RESOURCE_ACCESS_MAX_ENTRIES 3
> 
> No - the limit on the number of consecutive entries doesn't change
> because of your addition of another MSR. Actual batching is to be
> done via multicalls, the multi-entry requests here are meant to be
> used for successive operations that _must_ be issued without
> preemption (e.g. an MSR write needed for a successive read).
> 
You remind me that the requirement here is more than "without
preemption", ideally the exact timestamp the moment that the counter
being read should be returned, which is used by used space to calculate
the time elapsed between two samplings.  So the MSR read and TSC
read should be atomic, which means, 1) preemption should not happen,
2) interrupt should be disabled. Sounds more complex as we have no existed
way to disable interrupt, using back-to-back flag again?

> >  struct xen_resource_access {
> >      unsigned int nr_done;
> >      unsigned int nr_entries;
> >      xenpf_resource_entry_t *entries;
> >  };
> >  
> > -static bool_t allow_access_msr(unsigned int msr)
> > +static bool_t allow_access_msr(unsigned int msr, unsigned int cmd)
> 
> Since I'm going to ask you (below) to special case MSR_IA32_TSC on
> the read path, I think the write denial should also be handled specially,
> and also ideally with a distinguishable (from the -EACCES) error code
> (perhaps -EPERM). I.e. this change should be dropped.
> 
> > @@ -102,7 +104,7 @@ static void check_resource_access(struct xen_resource_access *ra)
> >          case XEN_RESOURCE_OP_MSR_WRITE:
> >              if ( entry->idx >> 32 )
> >                  ret = -EINVAL;
> > -            else if ( !allow_access_msr(entry->idx) )
> > +            else if ( !allow_access_msr(entry->idx, entry->u.cmd) )
> >                  ret = -EACCES;
> >              break;
> 
> Unless you have a reason for the read to be done through RDMSR,
> I'd really prefer you to special case the TSC and use RDTSC on the
> read side (and, as said above, deny the write in resource_access()
> rather than in check_resource_access()).

Sure, I agree to use RDTSC(with a new command XEN_RESOURCE_OP_TSC_READ
added) and the write path will not be supported(e.g. no
XEN_RESOURCE_OP_TSC_WRITE defined).

Chao

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