Re: [Xen-devel] [PATCH v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.17 at 17:42, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_destroy_p2m:
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_mem_access:
> +    case HVMOP_altp2m_set_mem_access_multi:

Was it agreed that this, just like others (many wrongly, I think) is
supposed to be invokable by the affected domain itself? Its non-
altp2m counterpart is a DM_PRIV operation. If the one here is
meant to be different, I think the commit message should say why.

> @@ -4568,6 +4571,30 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>  
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        if ( a.u.set_mem_access_multi.pad ||
> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> +                                      a.u.set_mem_access_multi.access_list,
> +                                      a.u.set_mem_access_multi.nr,
> +                                      a.u.set_mem_access_multi.opaque,
> +                                      MEMOP_CMD_MASK,
> +                                      a.u.set_mem_access_multi.view);
> +        if ( rc > 0 )
> +        {
> +            a.u.set_mem_access_multi.opaque = rc;
> +            if ( __copy_to_guest(arg, &a, 1) )

__copy_field_to_guest() would suffice here afaict.

> @@ -4586,6 +4613,53 @@ static int do_altp2m_op(
>      return rc;
>  }
>  
> +static int compat_altp2m_op(
> +    XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct compat_hvm_altp2m_op a;
> +    union
> +    {
> +        XEN_GUEST_HANDLE_PARAM(void) hnd;
> +        struct xen_hvm_altp2m_op *altp2m_op;
> +    } nat;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return -EOPNOTSUPP;
> +
> +    if ( copy_from_guest(&a, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( a.pad1 || a.pad2 ||
> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> +        return -EINVAL;

Why doesn't suffice what do_altp2m_op() does?

> +    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> +    switch ( a.cmd )
> +    {
> +        case HVMOP_altp2m_set_mem_access_multi:
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +            
> XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi,
> +                    &a.u.set_mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> +            break;
> +        default:
> +            return do_altp2m_op(arg);
> +    }
> +
> +    nat.altp2m_op->version  = a.version;
> +    nat.altp2m_op->cmd      = a.cmd;
> +    nat.altp2m_op->domain   = a.domain;
> +    nat.altp2m_op->pad1     = a.pad1;
> +    nat.altp2m_op->pad2     = a.pad2;

Why do you do this by hand, rather than using XLAT_*() macros
which at the same time check that the field sizes match?

> @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>  
>      case HVMOP_altp2m:
> -        rc = do_altp2m_op(arg);
> +        rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) : 
> do_altp2m_op(arg);

Pointless parentheses and spaces. Plus, to be honest, I'm not really
happy about this ad hoc compat handling, but at the same time I
can't suggest a truly better alternative.

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -73,6 +73,7 @@
>  ?    vcpu_hvm_context                hvm/hvm_vcpu.h
>  ?    vcpu_hvm_x86_32                 hvm/hvm_vcpu.h
>  ?    vcpu_hvm_x86_64                 hvm/hvm_vcpu.h
> +!    hvm_altp2m_set_mem_access_multi hvm/hvm_op.h

Please insert alphabetically, sorted by filename (and then structure
name).

> --- a/xen/tools/compat-build-header.py
> +++ b/xen/tools/compat-build-header.py
> @@ -16,6 +16,7 @@ pats = [
>   [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
>   [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ],
>   [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
> + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],

What is this needed for? I can't find any instance of HVMMEM_*
elsewhere in the patch. As you can see, so far there are only
pretty generic tokens being replaced here.

Jan

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