Re: [Xen-devel] [PATCH v12 1/9] x86: add generic resource (e.g. MSR) access hypercall

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, July 04, 2014 6:44 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 v12 1/9] x86: add generic resource (e.g. MSR) access
> hypercall
> 
> >>> On 04.07.14 at 10:34, <dongxiao.xu@xxxxxxxxx> wrote:
> > Add a generic resource access hypercall for tool stack or other
> > components, e.g., accessing MSR, port I/O, etc.
> 
> Sigh - you're still allocating an unbounded amount of memory for
> passing around the input arguments, despite it being possible (and
> having been suggested) to read these from the original buffer on
> each iteration. You're still not properly checking for preemption
> between iterations. And you're still not making use of
> continue_hypercall_on_cpu(). Plus you now silently ignore the
> upper 32-bits of the passing in "idx" value as well as not
> understood XEN_RESOURCE_OP_* values.

continue_hypercall_on_cpu() is asynchronized, which requires the "data" field 
always points to the right place before the hypercall returns.
However in our function, we have a "for" loop to cover multiple operations, so 
the "data" field will be modified in each iteration, which cannot meet the 
continue_hypercall_on_cpu() requirements...

For the preemption check, what about the following? Here the preemption is 
checked within each resource_access_one() function.

static int resource_access_one(uint32_t type, uint32_t cmd,
                                uint64_t idx, uint64_t *val)
{
    int ret;

    if ( !is_idle_vcpu(current) && hypercall_preempt_check() )
    {
        ret = -ERESTART;
        break;
    }

    /* Handle the resource access code. */
    ...

    return ret;
}

int resource_access_helper(struct xenpf_resource_op *op)
{
    ...
    for ( i = 0; i < op->nr; i++ )
    {
        ...
        if ( data.cpu == smp_processor_id() )
            resource_access_one(&data);
        else
            on_selected_cpus(cpumask_of(last_cpu),
                             resource_access_one, &data, 1); 
    }

    ...
}

> 
> I also doubt that this warrants a new source file to be introduced,
> the helper functions (if indeed needed) can very well live in
> platform_hypercall.c.

Okay, I will put the resource related function in platform_hypercall.c.

Thanks,
Dongxiao

> 
> Jan


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