Re: [PATCH RFC 3/7] xen/arm: Enable the compilation of mem_access and mem_event on ARM.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.08.14 at 11:30, <tamas.lengyel@zentific.com> wrote:
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -11,10 +11,17 @@
>  #include <xen/sched.h>
>  #include <xen/hypercall.h>
>  #include <public/domctl.h>
> +#include <asm/guest_access.h>
> +#include <xen/mem_event.h>
> +#include <public/mem_event.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> +
> +    long ret;
> +    bool_t copyback = 0;
> +
>      switch ( domctl->cmd )
>      {
>      case XEN_DOMCTL_cacheflush:
> @@ -23,17 +30,38 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          unsigned long e = s + domctl->u.cacheflush.nr_pfns;
>  
>          if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
> -            return -EINVAL;
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
>  
>          if ( e < s )
> -            return -EINVAL;
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
>  
> -        return p2m_cache_flush(d, s, e);
> +        ret = p2m_cache_flush(d, s, e);
>      }
> +    break;

What's wrong with the code above?

> +
> +    case XEN_DOMCTL_mem_event_op:
> +    {
> +        ret = mem_event_domctl(d, &domctl->u.mem_event_op,
> +                              guest_handle_cast(u_domctl, void));
> +        copyback = 1;
> +    }

Pointless curly braces. Furthermore this already goes beyond what
the subject says, so you may want to adjust the title.

> +    break;
>  
>      default:
> -        return subarch_do_domctl(domctl, d, u_domctl);
> +        ret = subarch_do_domctl(domctl, d, u_domctl);
> +        break;

Again an unnecessary change.

> @@ -1111,18 +1114,27 @@ int xenmem_add_to_physmap_one(
>  
>  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> -    switch ( op )
> +
> +    long rc;
> +
> +    switch ( op & MEMOP_CMD_MASK )
>      {
>      /* XXX: memsharing not working yet */
>      case XENMEM_get_sharing_shared_pages:
>      case XENMEM_get_sharing_freed_pages:
>          return 0;
> +    case XENMEM_access_op:

Missing blank line before new case.

> +    {
> +        rc = mem_access_memop(op, guest_handle_cast(arg, xen_mem_access_op_t));
> +        break;
> +    }

Pointless curly braces again.

>      default:
> -        return -ENOSYS;
> +        rc = -ENOSYS;
> +        break;

Unnecessary change again.

> --- a/xen/common/mem_access.c
> +++ b/xen/common/mem_access.c
> @@ -29,8 +29,6 @@
>  #include <xen/mem_event.h>
>  #include <xsm/xsm.h>
>  
> -#ifdef CONFIG_X86
> -
>  int mem_access_memop(unsigned long cmd,
>                       XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg)
>  {
> @@ -45,9 +43,11 @@ int mem_access_memop(unsigned long cmd,
>      if ( rc )
>          return rc;
>  
> +#ifdef CONFIG_X86
>      rc = -EINVAL;
>      if ( !is_hvm_domain(d) )
>          goto out;
> +#endif

Ugly, but well, I don't see a nice alternative.

> --- a/xen/common/mem_event.c
> +++ b/xen/common/mem_event.c
> @@ -20,16 +20,19 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>  
> -#ifdef CONFIG_X86
> -
> +#include <xen/sched.h>
>  #include <asm/domain.h>
>  #include <xen/event.h>
>  #include <xen/wait.h>
>  #include <asm/p2m.h>
>  #include <xen/mem_event.h>
>  #include <xen/mem_access.h>
> +
> +#ifdef CONFIG_X86
>  #include <asm/mem_paging.h>
>  #include <asm/mem_sharing.h>
> +#endif

Wouldn't that warrant introduction of HAVE_MEM_SHARING and
HAVE_MEM_PAGING?

> @@ -538,6 +543,8 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
>          {
>          case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE:
>          {
> +
> +#ifdef CONFIG_X86

Bogus blank line above the #ifdef.

>              rc = -ENODEV;
>              /* Only HAP is supported */
>              if ( !hap_enabled(d) )
> @@ -546,6 +553,7 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
>              /* Currently only EPT is supported */
>              if ( !cpu_has_vmx )
>                  break;
> +#endif

Code like what's getting enclosed in the #ifdef here should really
get abstracted out up front.

> +typedef enum {
> +    p2m_access_n     = 0, /* No access permissions allowed */
> +    p2m_access_r     = 1,
> +    p2m_access_w     = 2, 
> +    p2m_access_rw    = 3,
> +    p2m_access_x     = 4, 
> +    p2m_access_rx    = 5,
> +    p2m_access_wx    = 6, 
> +    p2m_access_rwx   = 7
> +
> +    /* NOTE: Assumed to be only 4 bits right now */

The comment seems bogus (i.e. blindly copied from x86). Furthermore
I think all the types above are really generic, i.e. may warrant placing
in a common header (with a per-arch define of extra types to be
added).

Jan