Re: [Xen-devel] [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.16 at 12:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1836,6 +1836,15 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>      return 0;
>  }
>  
> +long p2m_set_mem_access_multi(struct domain *d,
> +                              const XEN_GUEST_HANDLE(const_uint64) pfn_list,
> +                              const XEN_GUEST_HANDLE(const_uint8) 
> access_list,
> +                              uint32_t nr, uint32_t start, uint32_t mask,
> +                              unsigned int altp2m_idx)
> +{
> +    return -ENOTSUP;
> +}

If this indeed fixes the build problem on ARM, then I'd like to have an
explanation (by you or the ARM maintainers) where ENOTSUP gets
defined. I can't find any single instance of this throughout the tree,
and headers outside of the tree aren't supposed to get included in
the hypervisor build.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -28,6 +28,7 @@
>  #include <xen/event.h>
>  #include <public/vm_event.h>
>  #include <asm/domain.h>
> +#include <xen/guest_access.h> /* copy_from_guest() */
>  #include <asm/page.h>

I thought we've been through this before: Why does this xen/
include get added in the middle of asm/ ones?

> +static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
> +                          struct p2m_domain *ap2m, p2m_access_t a,
> +                          unsigned long gfn_l)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
> -    p2m_access_t a, _a;
> -    p2m_type_t t;
> -    mfn_t mfn;
> -    unsigned long gfn_l;
> -    long rc = 0;
> +    int rc = 0;
>  
> +    if ( ap2m )
> +    {
> +        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l));

With this it becomes pretty clear that, following our general direction,
set_mem_access()'s last parameter should itself be gfn_t.

> +static bool_t p2m_xenmem_access_to_p2m_access(struct p2m_domain *p2m,

The p2m_ prefix is kind of redundant here as long as the function is
static.

Also plain bool please in new code, and ...

> @@ -1823,6 +1839,34 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>  #undef ACCESS
>      };
>  
> +    switch ( xaccess )
> +    {
> +    case 0 ... ARRAY_SIZE(memaccess) - 1:
> +        *paccess = memaccess[xaccess];
> +        break;
> +    case XENMEM_access_default:
> +        *paccess = p2m->default_access;
> +        break;
> +    default:
> +        return 0;

... false and ...

> +    }
> +
> +    return 1;

... true respectively then.

> @@ -520,6 +534,9 @@ int compat_memory_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) compat)
>                  rc = -EFAULT;
>              break;
>  
> +        case XENMEM_access_op:
> +            break;

There's a group of several other XENMEM_* which also require no
action (right above the XENMEM_get_vnumainfo handling). Please
simply add this new case there.

Jan


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