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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.15 at 10:45, <chao.p.peng@xxxxxxxxxxxxxxx> 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?

> --- 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).

>  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()).

Jan


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