Re: [Xen-devel] [PATCH v17 01/10] x86: add generic resource (e.g. MSR) access hypercall

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/14 11:40, Chao Peng wrote:
> Add a generic resource access hypercall for tool stack or other
> components, e.g., accessing MSR, port I/O, etc.
>
> The resource is abstracted as a resource address/value pair.
> The resource access can be any type of XEN_RESOURCE_OP_*(current
> only support MSR and it's white-listed). The resource operations
> are always runs on cpu that caller specified. If caller does not
> care this, it should use current cpu to eliminate the IPI overhead.
>
> Batch resource operations in one call are also supported but the
> max number currently is limited to 2. The operations in a batch are
> non-preemptible and execute in their original order. If preemptible
> batch is desirable, then multicall mechanism can be used.
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>

Looking much better.

However, I have noticed something which I missed in previous reviews
(appologies for not spotting it earlier), and a couple of comments.

> ---
>  xen/arch/x86/platform_hypercall.c        |  140 
> ++++++++++++++++++++++++++++++
>  xen/arch/x86/x86_64/platform_hypercall.c |    4 +
>  xen/include/public/platform.h            |   28 ++++++
>  xen/include/xlat.lst                     |    1 +
>  4 files changed, 173 insertions(+)
>
> diff --git a/xen/arch/x86/platform_hypercall.c 
> b/xen/arch/x86/platform_hypercall.c
> index 2162811..4b92b56 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -61,6 +61,77 @@ long cpu_down_helper(void *data);
>  long core_parking_helper(void *data);
>  uint32_t get_cur_idle_nums(void);
>  
> +struct xen_resource_access {
> +    int32_t ret;
> +    uint32_t nr_entries;

This is private to Xen.  It is perfectly fine to use int and unsigned
int respectively.

> +    xenpf_resource_entry_t *entries;
> +};
> +
> +static bool_t allow_access_msr(unsigned int msr)
> +{
> +    return 0;
> +}
> +
> +static int check_resource_access(struct xen_resource_access *ra)
> +{
> +    xenpf_resource_entry_t *entry;
> +    unsigned int i;
> +
> +    for ( i = 0; i < ra->nr_entries; i++ )
> +    {
> +        entry = ra->entries + i;
> +
> +        if ( entry->rsvd )
> +            return -EINVAL;
> +
> +        switch ( entry->cmd )
> +        {
> +        case XEN_RESOURCE_OP_MSR_READ:
> +        case XEN_RESOURCE_OP_MSR_WRITE:
> +            if ( entry->idx >> 32 )
> +                return -EINVAL;
> +            else if ( !allow_access_msr(entry->idx) )
> +                return -EACCES;
> +        default:
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void resource_access(void *info)
> +{
> +    struct xen_resource_access *ra = info;
> +    xenpf_resource_entry_t *entry;
> +    int ret = 0;
> +    unsigned int i;
> +
> +    for ( i = 0; i < ra->nr_entries; i++ )
> +    {
> +        entry = ra->entries + i;
> +
> +        switch ( entry->cmd )
> +        {
> +        case XEN_RESOURCE_OP_MSR_READ:
> +        case XEN_RESOURCE_OP_MSR_WRITE:
> +            if ( entry->cmd == XEN_RESOURCE_OP_MSR_READ )
> +                ret = rdmsr_safe(entry->idx, entry->val);
> +            else
> +                ret = wrmsr_safe(entry->idx, entry->val);
> +            break;

This is a little bit weird to read.  Only the break statement is common
to the two case statements.  It would be better just to split the two
case statements properly and do away with the if().

> +        default:
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        if ( ret )
> +            break;
> +    }
> +
> +    ra->ret = ret;
> +}
> +
>  ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>  {
>      ret_t ret = 0;
> @@ -601,6 +672,75 @@ ret_t 
> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>      }
>      break;
>  
> +    case XENPF_resource_op:
> +    {
> +        struct xen_resource_access ra;
> +        uint32_t cpu;
> +        XEN_GUEST_HANDLE(xenpf_resource_entry_t) guest_entries;
> +
> +#define MAX_SUPPORTED_ENTRIES   2

This name is far too generic.  At the least it should be
RESOURCE_ACCESS_MAX_ENTRIES, and the definition should probably live
beside struct xen_resource_access.

> +
> +        ra.nr_entries = op->u.resource_op.nr_entries;
> +        if ( ra.nr_entries == 0 || ra.nr_entries > MAX_SUPPORTED_ENTRIES )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        ra.entries = xmalloc_array(xenpf_resource_entry_t, ra.nr_entries);
> +        if ( !ra.entries )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        guest_from_compat_handle(guest_entries, op->u.resource_op.entries);
> +
> +        if ( copy_from_guest(ra.entries, guest_entries, ra.nr_entries) )
> +        {
> +            xfree(ra.entries);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        /* Do sanity check earlier to omit the potential IPI overhead. */
> +        ret = check_resource_access(&ra);
> +        if ( ret )
> +        {
> +            xfree(ra.entries);
> +            break;
> +        }
> +
> +        cpu = op->u.resource_op.cpu;
> +        if ( cpu == smp_processor_id() )
> +            resource_access(&ra);
> +        else if ( cpu_online(cpu) )
> +            on_selected_cpus(cpumask_of(cpu), resource_access, &ra, 1);
> +        else
> +        {
> +            xfree(ra.entries);
> +            ret = -ENODEV;
> +            break;
> +        }
> +
> +        if ( ra.ret )
> +        {
> +            xfree(ra.entries);
> +            ret = ra.ret;
> +            break;
> +        }
> +
> +        if ( __copy_to_guest(guest_entries, ra.entries, ra.nr_entries) )
> +        {
> +            xfree(ra.entries);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        xfree(ra.entries);
> +    }
> +    break;
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/arch/x86/x86_64/platform_hypercall.c 
> b/xen/arch/x86/x86_64/platform_hypercall.c
> index b6f380e..ccfd30d 100644
> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -32,6 +32,10 @@ CHECK_pf_pcpu_version;
>  CHECK_pf_enter_acpi_sleep;
>  #undef xen_pf_enter_acpi_sleep
>  
> +#define xen_pf_resource_entry xenpf_resource_entry
> +CHECK_pf_resource_entry;
> +#undef xen_pf_resource_entry
> +
>  #define COMPAT
>  #define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
>  #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
> diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> index 053b9fa..5951848 100644
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -528,6 +528,33 @@ typedef struct xenpf_core_parking xenpf_core_parking_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>  
>  /*
> + * Access generic platform resources(e.g., accessing MSR, port I/O, etc)
> + * in unified way. Batch resource operations in one call are supported and
> + * thay are always non-preemptible and execute in their original order.
> + */
> +#define XENPF_resource_op   61
> +
> +#define XEN_RESOURCE_OP_MSR_READ  0
> +#define XEN_RESOURCE_OP_MSR_WRITE 1
> +
> +struct xenpf_resource_entry {
> +    uint32_t cmd;       /* IN: XEN_RESOURCE_OP_* */
> +    uint32_t rsvd;      /* IN: padding and must be zero */

This rsvd should be int32_t ret, and an out parameter.

When issuing two back to back operations, it is helpful to identify
which one failed, rather than just "something failed".  Putting a
per-resource return value allows userspace to unambiguously identify
which operation failed, and why (EACCES, EFAULT etc).

In situations like this, it is also common for the main hypercall to
return -E??? for general errors, or a positive integer for the number of
successful operations.

Therefore, for complete success, the XENPF_resource_op hypercall would
return 2.  If the read succeeded but the write failed, it would return
1, and if the read failed, it would return 0.

~Andrew

> +    uint64_t idx;       /* IN: resource address to access */
> +    uint64_t val;       /* IN/OUT: resource value to set/get */
> +};
> +typedef struct xenpf_resource_entry xenpf_resource_entry_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_entry_t);
> +
> +struct xenpf_resource_op {
> +    uint32_t nr_entries;    /* number of resource entry */
> +    uint32_t cpu;           /* which cpu to run */
> +    XEN_GUEST_HANDLE(xenpf_resource_entry_t) entries;
> +};
> +typedef struct xenpf_resource_op xenpf_resource_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_op_t);
> +
> +/*
>   * ` enum neg_errnoval
>   * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
>   */
> @@ -553,6 +580,7 @@ struct xen_platform_op {
>          struct xenpf_cpu_hotadd        cpu_add;
>          struct xenpf_mem_hotadd        mem_add;
>          struct xenpf_core_parking      core_parking;
> +        struct xenpf_resource_op       resource_op;
>          uint8_t                        pad[128];
>      } u;
>  };
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 9a35dd7..234b668 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -88,6 +88,7 @@
>  ?    xenpf_enter_acpi_sleep          platform.h
>  ?    xenpf_pcpuinfo                  platform.h
>  ?    xenpf_pcpu_version              platform.h
> +?    xenpf_resource_entry            platform.h
>  !    sched_poll                      sched.h
>  ?    sched_remote_shutdown           sched.h
>  ?    sched_shutdown                  sched.h



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