Re: [Xen-devel] [PATCH v11 1/9] x86: add generic MSR access hypercall

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, June 23, 2014 2:45 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> George.Dunlap@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: RE: [PATCH v11 1/9] x86: add generic MSR access hypercall
> 
> >>> On 23.06.14 at 08:27, <dongxiao.xu@xxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Friday, June 20, 2014 11:01 PM
> >> To: Xu, Dongxiao
> >> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> >> George.Dunlap@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> >> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> >> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> >> Subject: Re: [PATCH v11 1/9] x86: add generic MSR access hypercall
> >>
> >> >>> On 20.06.14 at 16:31, <dongxiao.xu@xxxxxxxxx> wrote:
> >> > Add a generic MSR access hypercall for tool stack or other components to
> >> > read or write certain system MSRs.
> >>
> >> If you want this to be usable by components other than the tool stack
> >> then this can't be a sysctl (would e.g. need to be a platform-op then).
> >
> > Per my understanding, the platform-op related hypercalls are used by Dom0
> > kernel.
> > While in this CQM scenario, we want libvirt/libxl/libxc to call such
> > hypercall to access the MSR, however I didn't see any usage of platform_op 
> > in
> > libxl/libxc side.
> >
> > Could you explain a bit more for it?
> 
> Platform ops are for use by Dom0, yes, but note the explicit omission
> of "kernel" in my reply. The fact that libxc currently doesn't use any
> doesn't mean it mustn't do so. The only question here that matter is
> what audience we see for the new functionality: If this is to be a
> purely tools interface, then a sysctl is fine. If the kernel is intended to
> also be able to use it (as I suggested), something other than a sysctl
> needs to be used (and I was merely _hinting_ at platform-op).

I got such concept in the file descriptor, and that's why I was thinking of 
platform-op is for dom0-kernel only...
Looking through all the current hypercalls, sysctl and platform-op are the most 
appropriate ones.

According to your word, if that's is not the limitation for Dom0 kernel only, I 
am okay to change it to platform-op wise.
BTW, do we need to modify the file header for platform_hypercall.c/h?

/******************************************************************************
 * platform_hypercall.c
 *
 * Hardware platform operations. Intended for use by domain-0 kernel.
 *
 * Copyright (c) 2002-2006, K Fraser
 */

> 
> >> > --- a/xen/arch/x86/sysctl.c
> >> > +++ b/xen/arch/x86/sysctl.c
> >> > @@ -66,10 +66,27 @@ void arch_do_physinfo(xen_sysctl_physinfo_t *pi)
> >> >          pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
> >> >  }
> >> >
> >> > +static void msr_access_helper(void *data)
> >> > +{
> >> > +    struct msr_access_info *info = data;
> >> > +    xen_sysctl_msr_data_t *msr_data;
> >> > +    unsigned int i;
> >> > +
> >> > +    for ( i = 0; i < info->nr_ops; i++ )
> >> > +    {
> >> > +        msr_data = &info->msr_data[i];
> >> > +        if ( msr_data->cmd == XEN_SYSCTL_MSR_read )
> >> > +            rdmsrl(msr_data->msr, msr_data->val);
> >> > +        else if ( msr_data->cmd == XEN_SYSCTL_MSR_write )
> >> > +            wrmsrl(msr_data->msr, msr_data->val);
> >> > +    }
> >>
> >> Missing preemption check.
> >
> > Do you mean hypercall_preemption_check() here? Or the preemption
> between the
> > MSR accesses?
> 
> Not sure what distinction you make between the two (or what the
> second question is supposed to refer to). The list of operations can
> be arbitrarily long, and individual operations may also take arbitrary
> time (especially if this later gets extended to I/O port accesses,
> which may incur SMM invocation). Hence preemption checks between
> operations are needed (except in well-defined cases where the
> caller may request such check to be skipped).

Okay.

> 
> >> Anyway, together with the comments on the interface itself (further
> >> down) I think you'd be much better off basing this on
> >> continue_hypercall_on_cpu().
> >
> > continue_hypercall_on_cpu() function schedules a tasklet to run the specific
> > function on certain CPU.
> > However in our CQM case, the libxl/libxc functions want to get the result
> > immediately when the function returns, so that's why I didn't use
> > continue_hypercall_on_cpu().
> 
> The use of a tasklet doesn't mean the result would only be available in
> a deferred manner: The tasklet runs _before_ control transfers back
> to the caller.

Okay.

> 
> >> > --- a/xen/include/public/sysctl.h
> >> > +++ b/xen/include/public/sysctl.h
> >> > @@ -636,6 +636,29 @@ struct xen_sysctl_coverage_op {
> >> >  typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
> >> >  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
> >> >
> >> > +#define XEN_SYSCTL_MSR_read                0
> >> > +#define XEN_SYSCTL_MSR_write               1
> >> > +
> >> > +struct xen_sysctl_msr_data {
> >> > +    uint32_t cmd;       /* XEN_SYSCTL_MSR_* */
> >> > +    uint32_t msr;
> >> > +    uint64_t val;
> >> > +};
> >> > +typedef struct xen_sysctl_msr_data xen_sysctl_msr_data_t;
> >> > +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_msr_data_t);
> >> > +struct xen_sysctl_msr_op {
> >> > +    uint32_t nr_ops;
> >>
> >> Jusr "nr" would suffice.
> >
> > Okay.
> >
> >>
> >> > +    uint32_t cpu;
> >>
> >> This is rather limiting - you should be able to specify a CPU for
> >> each operation. That way you'll have another 32-bit field available
> >> for flags: You indicated earlier on that you'd want certain
> >> operations (like a write followed by a read) pairable without
> >> potentially getting preempted in between. The field then freed up
> >> here should then be reserved for use as flags field too (i.e. you'd
> >> need to check that it's zero).
> >
> > Do you mean the following case?
> >
> > Op1: CPU0, write MSR 0x100. Flag: 1 (indicating paring)
> > Op2: CPU1, read MSR 0x200. Flag: 0.
> > Op3: CPU0, read MSR 0x101. Flag: 1 (paring the above op1)
> 
> No - CPU changes alway imply preemption between individual
> sub-ops.

Okay. Let's change the example a bit.

Op1: CPU0, write MSR 0x100.
Op2: CPU0, read MSR 0x101.
Op3: CPU1, read MSR 0x200.

Can we just follow the above example, if the two operations are "continuous" 
towards a target CPU, then we will pack them together via a single IPI?
Or do we really need the flag to indicate no-preemption there?

Thanks,
Dongxiao

> 
> > To avoid the preempt between certain operations, my previous solution is to
> > pack all the operations towards a certain CPU via a single IPI. When the
> > target CPU receives the IPI, it will execute the operations one by one
> > (suppose during the execution for loop, no preemption will happen).
> > Now if we specify a CPU for each operation, can we pack the operations
> > according to the target CPU, and finally issue one IPI per-target-CPU to 
> > avoid
> > preemption? That will get rid of such flag usage.
> 
> No, I don't think re-ordering operations can be permitted here.
> 
> Jan

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