Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-30 Thread Paul Durrant 
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@linaro.org]
> Sent: 30 October 2017 12:09
> To: Paul Durrant <paul.durr...@citrix.com>; Jan Beulich
> <jbeul...@suse.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George
> Dunlap <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Stefano Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> <dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> Hi Paul,
> 
> On 27/10/17 16:19, Paul Durrant wrote:
> >> -Original Message-
> >> From: Julien Grall [mailto:julien.gr...@linaro.org]
> >> Sent: 27 October 2017 12:46
> >> To: Jan Beulich <jbeul...@suse.com>; Paul Durrant
> >> <paul.durr...@citrix.com>
> >> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> >> <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George
> >> Dunlap <george.dun...@citrix.com>; Ian Jackson
> <ian.jack...@citrix.com>;
> >> Stefano Stabellini <sstabell...@kernel.org>; xen-
> de...@lists.xenproject.org;
> >> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> >> <dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi,
> >>
> >> On 26/10/17 16:39, Jan Beulich wrote:
> >>>>>> On 26.10.17 at 17:32, <julien.gr...@linaro.org> wrote:
> >>>> On 26/10/17 16:26, Jan Beulich wrote:
> >>>>>>>> On 17.10.17 at 15:24, <paul.durr...@citrix.com> wrote:
> >>>>>> +/* IN/OUT - If the tools domain is PV then, upon return,
> frame_list
> >>>>>> + *  will be populated with the MFNs of the resource.
> >>>>>> + *  If the tools domain is HVM then it is expected that, 
> >>>>>> on
> >>>>>> + *  entry, frame_list will be populated with a list of 
> >>>>>> GFNs
> >>>>>> + *  that will be mapped to the MFNs of the resource.
> >>>>>> + *  If -EIO is returned then the frame_list has only been
> >>>>>> + *  partially mapped and it is up to the caller to unmap 
> >>>>>> all
> >>>>>> + *  the GFNs.
> >>>>>> + *  This parameter may be NULL if nr_frames is 0.
> >>>>>> + */
> >>>>>> +XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> >>>>>
> >>>>> This is still xen_ulong_t, which I can live with, but then you shouldn't
> >>>>> copy into / out of arrays of other types in acquire_resource() (the
> >>>>> more that this is common code, and iirc xen_ulong_t and
> >>>>> unsigned long aren't the same thing on ARM32).
> >>>>
> >>>> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
> >>>> we use xen_pfn_t here?
> >>>
> >>> I had put this question up earlier, but iirc Paul didn't like it.
> >>
> >> I'd like to understand why Paul doesn't like it. We should never assume
> >> that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
> >> that purpose.
> >
> > My reservation is whether xen_pfn_t is intended to hold either gfns or
> mfns, since this hypercall uses the same array for both. If it suitable then 
> I am
> happy to change it, but Andrew led me to believe otherwise.
> 
> Looking at the public hearders, xen_pfn_t is been used for both MFN (see
> xenpf_add_memtype) and GFN (see gnttab_setup_table).
> 
> So I think it would be fine to do the same here.

Yes, I'm going to change it in the next version.

  Cheers,

Paul

> Cheers,
> 
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-30 Thread Julien Grall 

Hi Paul,

On 27/10/17 16:19, Paul Durrant wrote:

-Original Message-
From: Julien Grall [mailto:julien.gr...@linaro.org]
Sent: 27 October 2017 12:46
To: Jan Beulich <jbeul...@suse.com>; Paul Durrant
<paul.durr...@citrix.com>
Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
<andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George
Dunlap <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
Stefano Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org;
Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
<dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources

Hi,

On 26/10/17 16:39, Jan Beulich wrote:

On 26.10.17 at 17:32, <julien.gr...@linaro.org> wrote:

On 26/10/17 16:26, Jan Beulich wrote:

On 17.10.17 at 15:24, <paul.durr...@citrix.com> wrote:

+/* IN/OUT - If the tools domain is PV then, upon return, frame_list
+ *  will be populated with the MFNs of the resource.
+ *  If the tools domain is HVM then it is expected that, on
+ *  entry, frame_list will be populated with a list of GFNs
+ *  that will be mapped to the MFNs of the resource.
+ *  If -EIO is returned then the frame_list has only been
+ *  partially mapped and it is up to the caller to unmap all
+ *  the GFNs.
+ *  This parameter may be NULL if nr_frames is 0.
+ */
+XEN_GUEST_HANDLE(xen_ulong_t) frame_list;


This is still xen_ulong_t, which I can live with, but then you shouldn't
copy into / out of arrays of other types in acquire_resource() (the
more that this is common code, and iirc xen_ulong_t and
unsigned long aren't the same thing on ARM32).


xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
we use xen_pfn_t here?


I had put this question up earlier, but iirc Paul didn't like it.


I'd like to understand why Paul doesn't like it. We should never assume
that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
that purpose.


My reservation is whether xen_pfn_t is intended to hold either gfns or mfns, 
since this hypercall uses the same array for both. If it suitable then I am 
happy to change it, but Andrew led me to believe otherwise.


Looking at the public hearders, xen_pfn_t is been used for both MFN (see 
xenpf_add_memtype) and GFN (see gnttab_setup_table).


So I think it would be fine to do the same here.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-30 Thread Paul Durrant 
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 26 October 2017 16:27
> To: Paul Durrant 
> Cc: Julien Grall ; Andrew Cooper
> ; Wei Liu ; George
> Dunlap ; Ian Jackson ;
> Stefano Stabellini ; xen-de...@lists.xenproject.org;
> Konrad Rzeszutek Wilk ; Daniel De Graaf
> ; Tim (Xen.org) 
> Subject: Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 17.10.17 at 15:24,  wrote:
> > @@ -535,6 +588,48 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> >  rc = -EFAULT;
> >  break;
> >
> > +case XENMEM_acquire_resource:
> > +{
> > +const xen_ulong_t *xen_frame_list =
> > +(xen_ulong_t *)(nat.mar + 1);
> > +compat_ulong_t *compat_frame_list =
> > +(compat_ulong_t *)(nat.mar + 1);
> > +
> > +if ( cmp.mar.nr_frames == 0 )
> 
> Doesn't this need to be compat_handle_is_null(cmp.mar.frame_list), or
> a combination of both?

Sorry, yes this was a hang-over from the old scheme.

> 
> > +{
> > +
> DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
> > +
> > +if ( __copy_field_to_guest(
> > + guest_handle_cast(compat,
> > +   compat_mem_acquire_resource_t),
> > + , nr_frames) )
> > +return -EFAULT;
> > +}
> > +else
> > +{
> > +/*
> > + * NOTE: the smaller compat array overwrites the native
> > + *   array.
> > + */
> 
> I think I had already asked for a respective BUILD_BUG_ON().

You asked for the comment. I can't find where you asked for a BUILD_BUG_ON() 
but I can certainly add one.

> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
> >  return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> >  }
> >
> > +static int acquire_resource(
> > +XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> > +{
> > +struct domain *d, *currd = current->domain;
> > +xen_mem_acquire_resource_t xmar;
> > +unsigned long mfn_list[2];
> > +int rc;
> > +
> > +if ( copy_from_guest(, arg, 1) )
> > +return -EFAULT;
> > +
> > +if ( xmar.pad != 0 )
> > +return -EINVAL;
> > +
> > +if ( guest_handle_is_null(xmar.frame_list) )
> > +{
> > +/* Special case for querying implementation limit */
> > +if ( xmar.nr_frames == 0 )
> 
> Perhaps invert the condition to reduce ...
> 
> > +{
> > +xmar.nr_frames = ARRAY_SIZE(mfn_list);
> > +
> > +if ( __copy_field_to_guest(arg, , nr_frames) )
> > +return -EFAULT;
> > +
> > +return 0;
> > +}
> 
> ... overall indentation?
> 
> > +return -EINVAL;
> > +}
> > +
> > +if ( xmar.nr_frames == 0 )
> > +return -EINVAL;
> 
> Why? (Almost?) everywhere else zero counts are simply no-ops, which
> result in success returns.

Ok, I'll drop the check.

> 
> > +if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> > +return -E2BIG;
> > +
> > +d = rcu_lock_domain_by_any_id(xmar.domid);
> 
> This being a tools only interface, why "by_any_id" instead of
> "remote_domain_by_id"? In particular ...
> 
> > +if ( d == NULL )
> > +return -ESRCH;
> > +
> > +rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
> 
> ... an unprivileged dm domain should probably not be permitted to
> invoke this on itself.

True.

> 
> > +if ( rc )
> > +goto out;
> > +
> > +switch ( xmar.type )
> > +{
> > +default:
> > +rc = -EOPNOTSUPP;
> > +break;
> > +}
> > +
> > +if ( rc )
> > +goto out;
> > +
> > +if ( !paging_mode_translate(currd) )
> > +{
> > +if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> > +rc = -EFAULT;
> > +}
> > +else
> > +{
> > +xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > +unsigned int i;
> > +
> > +rc = -EFAULT;
> > +if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> > +goto out;
> > +
> > +for ( i = 0; i < xmar.nr_frames; i++ )
> > +{
> > +rc = set_foreign_p2m_entry(currd, gfn_list[i],
> > +   _mfn(mfn_list[i]));
> > +if ( rc )
> > +{
> > +/*
> > + * Make sure rc is -EIO for any interation other than
> > + * the first.
> 
>

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-27 Thread Paul Durrant 
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@linaro.org]
> Sent: 27 October 2017 12:46
> To: Jan Beulich <jbeul...@suse.com>; Paul Durrant
> <paul.durr...@citrix.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George
> Dunlap <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Stefano Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> <dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> Hi,
> 
> On 26/10/17 16:39, Jan Beulich wrote:
> >>>> On 26.10.17 at 17:32, <julien.gr...@linaro.org> wrote:
> >> On 26/10/17 16:26, Jan Beulich wrote:
> >>>>>> On 17.10.17 at 15:24, <paul.durr...@citrix.com> wrote:
> >>>> +/* IN/OUT - If the tools domain is PV then, upon return, frame_list
> >>>> + *  will be populated with the MFNs of the resource.
> >>>> + *  If the tools domain is HVM then it is expected that, on
> >>>> + *  entry, frame_list will be populated with a list of GFNs
> >>>> + *  that will be mapped to the MFNs of the resource.
> >>>> + *  If -EIO is returned then the frame_list has only been
> >>>> + *  partially mapped and it is up to the caller to unmap all
> >>>> + *  the GFNs.
> >>>> + *  This parameter may be NULL if nr_frames is 0.
> >>>> + */
> >>>> +XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> >>>
> >>> This is still xen_ulong_t, which I can live with, but then you shouldn't
> >>> copy into / out of arrays of other types in acquire_resource() (the
> >>> more that this is common code, and iirc xen_ulong_t and
> >>> unsigned long aren't the same thing on ARM32).
> >>
> >> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
> >> we use xen_pfn_t here?
> >
> > I had put this question up earlier, but iirc Paul didn't like it.
> 
> I'd like to understand why Paul doesn't like it. We should never assume
> that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
> that purpose.

My reservation is whether xen_pfn_t is intended to hold either gfns or mfns, 
since this hypercall uses the same array for both. If it suitable then I am 
happy to change it, but Andrew led me to believe otherwise.

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-27 Thread Julien Grall 

Hi,

On 26/10/17 16:39, Jan Beulich wrote:

On 26.10.17 at 17:32,  wrote:

On 26/10/17 16:26, Jan Beulich wrote:

On 17.10.17 at 15:24,  wrote:

+/* IN/OUT - If the tools domain is PV then, upon return, frame_list
+ *  will be populated with the MFNs of the resource.
+ *  If the tools domain is HVM then it is expected that, on
+ *  entry, frame_list will be populated with a list of GFNs
+ *  that will be mapped to the MFNs of the resource.
+ *  If -EIO is returned then the frame_list has only been
+ *  partially mapped and it is up to the caller to unmap all
+ *  the GFNs.
+ *  This parameter may be NULL if nr_frames is 0.
+ */
+XEN_GUEST_HANDLE(xen_ulong_t) frame_list;


This is still xen_ulong_t, which I can live with, but then you shouldn't
copy into / out of arrays of other types in acquire_resource() (the
more that this is common code, and iirc xen_ulong_t and
unsigned long aren't the same thing on ARM32).


xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
we use xen_pfn_t here?


I had put this question up earlier, but iirc Paul didn't like it.


I'd like to understand why Paul doesn't like it. We should never assume 
that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for 
that purpose.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-26 Thread Jan Beulich 
>>> On 26.10.17 at 17:32,  wrote:
> On 26/10/17 16:26, Jan Beulich wrote:
> On 17.10.17 at 15:24,  wrote:
>>> +/* IN/OUT - If the tools domain is PV then, upon return, frame_list
>>> + *  will be populated with the MFNs of the resource.
>>> + *  If the tools domain is HVM then it is expected that, on
>>> + *  entry, frame_list will be populated with a list of GFNs
>>> + *  that will be mapped to the MFNs of the resource.
>>> + *  If -EIO is returned then the frame_list has only been
>>> + *  partially mapped and it is up to the caller to unmap all
>>> + *  the GFNs.
>>> + *  This parameter may be NULL if nr_frames is 0.
>>> + */
>>> +XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>> 
>> This is still xen_ulong_t, which I can live with, but then you shouldn't
>> copy into / out of arrays of other types in acquire_resource() (the
>> more that this is common code, and iirc xen_ulong_t and
>> unsigned long aren't the same thing on ARM32).
> 
> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't 
> we use xen_pfn_t here?

I had put this question up earlier, but iirc Paul didn't like it.

Jan


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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-26 Thread Julien Grall 



On 26/10/17 16:26, Jan Beulich wrote:

On 17.10.17 at 15:24,  wrote:

+/* IN/OUT - If the tools domain is PV then, upon return, frame_list
+ *  will be populated with the MFNs of the resource.
+ *  If the tools domain is HVM then it is expected that, on
+ *  entry, frame_list will be populated with a list of GFNs
+ *  that will be mapped to the MFNs of the resource.
+ *  If -EIO is returned then the frame_list has only been
+ *  partially mapped and it is up to the caller to unmap all
+ *  the GFNs.
+ *  This parameter may be NULL if nr_frames is 0.
+ */
+XEN_GUEST_HANDLE(xen_ulong_t) frame_list;


This is still xen_ulong_t, which I can live with, but then you shouldn't
copy into / out of arrays of other types in acquire_resource() (the
more that this is common code, and iirc xen_ulong_t and
unsigned long aren't the same thing on ARM32).


xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't 
we use xen_pfn_t here?


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-26 Thread Jan Beulich 
>>> On 17.10.17 at 15:24,  wrote:
> @@ -535,6 +588,48 @@ int compat_memory_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) compat)
>  rc = -EFAULT;
>  break;
>  
> +case XENMEM_acquire_resource:
> +{
> +const xen_ulong_t *xen_frame_list =
> +(xen_ulong_t *)(nat.mar + 1);
> +compat_ulong_t *compat_frame_list =
> +(compat_ulong_t *)(nat.mar + 1);
> +
> +if ( cmp.mar.nr_frames == 0 )

Doesn't this need to be compat_handle_is_null(cmp.mar.frame_list), or
a combination of both?

> +{
> +DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
> +
> +if ( __copy_field_to_guest(
> + guest_handle_cast(compat,
> +   compat_mem_acquire_resource_t),
> + , nr_frames) )
> +return -EFAULT;
> +}
> +else
> +{
> +/*
> + * NOTE: the smaller compat array overwrites the native
> + *   array.
> + */

I think I had already asked for a respective BUILD_BUG_ON().

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>  return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +static int acquire_resource(
> +XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +{
> +struct domain *d, *currd = current->domain;
> +xen_mem_acquire_resource_t xmar;
> +unsigned long mfn_list[2];
> +int rc;
> +
> +if ( copy_from_guest(, arg, 1) )
> +return -EFAULT;
> +
> +if ( xmar.pad != 0 )
> +return -EINVAL;
> +
> +if ( guest_handle_is_null(xmar.frame_list) )
> +{
> +/* Special case for querying implementation limit */
> +if ( xmar.nr_frames == 0 )

Perhaps invert the condition to reduce ...

> +{
> +xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +
> +if ( __copy_field_to_guest(arg, , nr_frames) )
> +return -EFAULT;
> +
> +return 0;
> +}

... overall indentation?

> +return -EINVAL;
> +}
> +
> +if ( xmar.nr_frames == 0 )
> +return -EINVAL;

Why? (Almost?) everywhere else zero counts are simply no-ops, which
result in success returns.

> +if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> +return -E2BIG;
> +
> +d = rcu_lock_domain_by_any_id(xmar.domid);

This being a tools only interface, why "by_any_id" instead of
"remote_domain_by_id"? In particular ...

> +if ( d == NULL )
> +return -ESRCH;
> +
> +rc = xsm_domain_resource_map(XSM_DM_PRIV, d);

... an unprivileged dm domain should probably not be permitted to
invoke this on itself.

> +if ( rc )
> +goto out;
> +
> +switch ( xmar.type )
> +{
> +default:
> +rc = -EOPNOTSUPP;
> +break;
> +}
> +
> +if ( rc )
> +goto out;
> +
> +if ( !paging_mode_translate(currd) )
> +{
> +if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> +rc = -EFAULT;
> +}
> +else
> +{
> +xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> +unsigned int i;
> +
> +rc = -EFAULT;
> +if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> +goto out;
> +
> +for ( i = 0; i < xmar.nr_frames; i++ )
> +{
> +rc = set_foreign_p2m_entry(currd, gfn_list[i],
> +   _mfn(mfn_list[i]));
> +if ( rc )
> +{
> +/*
> + * Make sure rc is -EIO for any interation other than
> + * the first.

"iteration", but why is this important in the first place?

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
>  typedef struct xen_reserved_device_memory_map 
> xen_reserved_device_memory_map_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>  
> +/*
> + * Get the pages for a particular guest resource, so that they can be
> + * mapped directly by a tools domain.
> + */
> +#define XENMEM_acquire_resource 28
> +struct xen_mem_acquire_resource {
> +/* IN - the domain whose resource is to be mapped */
> +domid_t domid;
> +/* IN - the type of resource */
> +uint16_t type;
> +/*
> + * IN - a type-specific resource identifier, which must be zero
> + *  unless stated otherwise.
> + */
> +uint32_t id;
> +/* IN/OUT - As an IN parameter number of frames of the resource
> + *  to be mapped. However, if the specified value is 0 and
> + *  frame_list is NULL then this field will be set to the
> + *  maximum value supported by

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-25 Thread Paul Durrant 
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@linaro.org]
> Sent: 23 October 2017 20:04
> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich'
> <jbeul...@suse.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger
> Pau Monne <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano
> Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad
> Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> <dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> 
> 
> On 20/10/17 11:10, Paul Durrant wrote:
> >> -Original Message-
> >> From: Julien Grall [mailto:julien.gr...@linaro.org]
> >> Sent: 20 October 2017 11:00
> >> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich'
> >> <jbeul...@suse.com>
> >> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> >> <andrew.coop...@citrix.com>; George Dunlap
> >> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Roger
> >> Pau Monne <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>;
> Stefano
> >> Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org;
> Konrad
> >> Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> >> <dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi Paul,
> >>
> >> On 20/10/17 09:26, Paul Durrant wrote:
> >>>> -Original Message-
> >>>> From: Jan Beulich [mailto:jbeul...@suse.com]
> >>>> Sent: 20 October 2017 07:25
> >>>> To: Julien Grall <julien.gr...@linaro.org>
> >>>> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> >>>> <andrew.coop...@citrix.com>; George Dunlap
> >>>> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Paul
> >>>> Durrant <paul.durr...@citrix.com>; Roger Pau Monne
> >>>> <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano
> Stabellini
> >>>> <sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad
> >> Rzeszutek
> >>>> Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> >> <dgde...@tycho.nsa.gov>;
> >>>> Tim (Xen.org) <t...@xen.org>
> >>>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >>>> HYPERVISOR_memory_op to acquire guest resources
> >>>>
> >>>>>>> On 19.10.17 at 18:21, <julien.gr...@linaro.org> wrote:
> >>>>> Looking a bit more at the resource you can acquire from this hypercall.
> >>>>> Some of them are allocated using alloc_xenheap_page() so not
> assigned
> >> to
> >>>>> a domain.
> >>>>>
> >>>>> So I am not sure how you can expect a function
> set_foreign_p2m_entry
> >> to
> >>>>> take reference in that case.
> >>>>
> >>>> Hmm, with the domain parameter added, DOMID_XEN there (for
> >>>> Xen heap pages) could identify no references to be taken, if that
> >>>> was really the intended behavior in that case. However, even for
> >>>> Xen heap pages life time tracking ought to be done - it is for a
> >>>> reason that share_xen_page_with_guest() assigns the target
> >>>> domain as the owner of such pages, as that allows get_page() to
> >>>> succeed for them.
> >>>>
> >>>
> >
> > Hi Julien,
> >
> >>> So, nothing I'm doing here is making anything worse, right? Grant tables
> are
> >> assigned to the guest, and IOREQ server pages are allocated with
> >> alloc_domheap_page() so nothing is anonymous.
> >>
> >> I don't think grant tables is assigned to the guest today. They are
> >> allocated using xenheap_pages() and I can't find
> >> share_xen_page_with_guest().
> >
> > The guest would not be able to map them if they were not assigned in
> some way!
> 
> Do you mean for PV? For HVM/PVH, we don't check whether

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-23 Thread Julien Grall 



On 20/10/17 11:10, Paul Durrant wrote:

-Original Message-
From: Julien Grall [mailto:julien.gr...@linaro.org]
Sent: 20 October 2017 11:00
To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich'
<jbeul...@suse.com>
Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
<andrew.coop...@citrix.com>; George Dunlap
<george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger
Pau Monne <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano
Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad
Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
<dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources

Hi Paul,

On 20/10/17 09:26, Paul Durrant wrote:

-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: 20 October 2017 07:25
To: Julien Grall <julien.gr...@linaro.org>
Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
<andrew.coop...@citrix.com>; George Dunlap
<george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Paul
Durrant <paul.durr...@citrix.com>; Roger Pau Monne
<roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini
<sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad

Rzeszutek

Wilk <konrad.w...@oracle.com>; Daniel De Graaf

<dgde...@tycho.nsa.gov>;

Tim (Xen.org) <t...@xen.org>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources


On 19.10.17 at 18:21, <julien.gr...@linaro.org> wrote:

Looking a bit more at the resource you can acquire from this hypercall.
Some of them are allocated using alloc_xenheap_page() so not assigned

to

a domain.

So I am not sure how you can expect a function set_foreign_p2m_entry

to

take reference in that case.


Hmm, with the domain parameter added, DOMID_XEN there (for
Xen heap pages) could identify no references to be taken, if that
was really the intended behavior in that case. However, even for
Xen heap pages life time tracking ought to be done - it is for a
reason that share_xen_page_with_guest() assigns the target
domain as the owner of such pages, as that allows get_page() to
succeed for them.





Hi Julien,


So, nothing I'm doing here is making anything worse, right? Grant tables are

assigned to the guest, and IOREQ server pages are allocated with
alloc_domheap_page() so nothing is anonymous.

I don't think grant tables is assigned to the guest today. They are
allocated using xenheap_pages() and I can't find
share_xen_page_with_guest().


The guest would not be able to map them if they were not assigned in some way!


Do you mean for PV? For HVM/PVH, we don't check whether the page is 
assigned (see gnttab_map_frame).



See the code block at 
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/grant_table.c;hb=HEAD#l1716
It calls gnttab_create_shared_page() which is what calls through to 
share_xen_page_with_guest().


Thank you for the link, I will have a look.





Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
going to be left unimplemented on Arm until someone as time to implement
correctly the function.



That makes sense. Do you still have any issues with this patch apart from the 
cosmetic ones you spotted in the header?


No. Although, may I request to add a comment in the ARM helpers about 
the reference counting?


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-20 Thread Paul Durrant 
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@linaro.org]
> Sent: 20 October 2017 11:00
> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich'
> <jbeul...@suse.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger
> Pau Monne <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano
> Stabellini <sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad
> Rzeszutek Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> <dgde...@tycho.nsa.gov>; Tim (Xen.org) <t...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> Hi Paul,
> 
> On 20/10/17 09:26, Paul Durrant wrote:
> >> -Original Message-
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 20 October 2017 07:25
> >> To: Julien Grall <julien.gr...@linaro.org>
> >> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> >> <andrew.coop...@citrix.com>; George Dunlap
> >> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Paul
> >> Durrant <paul.durr...@citrix.com>; Roger Pau Monne
> >> <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini
> >> <sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad
> Rzeszutek
> >> Wilk <konrad.w...@oracle.com>; Daniel De Graaf
> <dgde...@tycho.nsa.gov>;
> >> Tim (Xen.org) <t...@xen.org>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >>>>> On 19.10.17 at 18:21, <julien.gr...@linaro.org> wrote:
> >>> Looking a bit more at the resource you can acquire from this hypercall.
> >>> Some of them are allocated using alloc_xenheap_page() so not assigned
> to
> >>> a domain.
> >>>
> >>> So I am not sure how you can expect a function set_foreign_p2m_entry
> to
> >>> take reference in that case.
> >>
> >> Hmm, with the domain parameter added, DOMID_XEN there (for
> >> Xen heap pages) could identify no references to be taken, if that
> >> was really the intended behavior in that case. However, even for
> >> Xen heap pages life time tracking ought to be done - it is for a
> >> reason that share_xen_page_with_guest() assigns the target
> >> domain as the owner of such pages, as that allows get_page() to
> >> succeed for them.
> >>
> >

Hi Julien,

> > So, nothing I'm doing here is making anything worse, right? Grant tables are
> assigned to the guest, and IOREQ server pages are allocated with
> alloc_domheap_page() so nothing is anonymous.
> 
> I don't think grant tables is assigned to the guest today. They are
> allocated using xenheap_pages() and I can't find
> share_xen_page_with_guest().

The guest would not be able to map them if they were not assigned in some way!
See the code block at 
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/grant_table.c;hb=HEAD#l1716
It calls gnttab_create_shared_page() which is what calls through to 
share_xen_page_with_guest().

> 
> Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is
> going to be left unimplemented on Arm until someone as time to implement
> correctly the function.
> 

That makes sense. Do you still have any issues with this patch apart from the 
cosmetic ones you spotted in the header?

Cheers,

  Paul

> Cheers,
> 
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-20 Thread Julien Grall 

Hi Paul,

On 20/10/17 09:26, Paul Durrant wrote:

-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: 20 October 2017 07:25
To: Julien Grall <julien.gr...@linaro.org>
Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
<andrew.coop...@citrix.com>; George Dunlap
<george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Paul
Durrant <paul.durr...@citrix.com>; Roger Pau Monne
<roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini
<sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad Rzeszutek
Wilk <konrad.w...@oracle.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>;
Tim (Xen.org) <t...@xen.org>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources


On 19.10.17 at 18:21, <julien.gr...@linaro.org> wrote:

Looking a bit more at the resource you can acquire from this hypercall.
Some of them are allocated using alloc_xenheap_page() so not assigned to
a domain.

So I am not sure how you can expect a function set_foreign_p2m_entry to
take reference in that case.


Hmm, with the domain parameter added, DOMID_XEN there (for
Xen heap pages) could identify no references to be taken, if that
was really the intended behavior in that case. However, even for
Xen heap pages life time tracking ought to be done - it is for a
reason that share_xen_page_with_guest() assigns the target
domain as the owner of such pages, as that allows get_page() to
succeed for them.



So, nothing I'm doing here is making anything worse, right? Grant tables are 
assigned to the guest, and IOREQ server pages are allocated with 
alloc_domheap_page() so nothing is anonymous.


I don't think grant tables is assigned to the guest today. They are 
allocated using xenheap_pages() and I can't find 
share_xen_page_with_guest().


Anyway, I discussed with Stefano about it. set_foreign_p2m_entry is 
going to be left unimplemented on Arm until someone as time to implement 
correctly the function.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-20 Thread Paul Durrant 
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 20 October 2017 07:25
> To: Julien Grall <julien.gr...@linaro.org>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Paul
> Durrant <paul.durr...@citrix.com>; Roger Pau Monne
> <roger@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini
> <sstabell...@kernel.org>; xen-de...@lists.xenproject.org; Konrad Rzeszutek
> Wilk <konrad.w...@oracle.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>;
> Tim (Xen.org) <t...@xen.org>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> >>> On 19.10.17 at 18:21, <julien.gr...@linaro.org> wrote:
> > Looking a bit more at the resource you can acquire from this hypercall.
> > Some of them are allocated using alloc_xenheap_page() so not assigned to
> > a domain.
> >
> > So I am not sure how you can expect a function set_foreign_p2m_entry to
> > take reference in that case.
> 
> Hmm, with the domain parameter added, DOMID_XEN there (for
> Xen heap pages) could identify no references to be taken, if that
> was really the intended behavior in that case. However, even for
> Xen heap pages life time tracking ought to be done - it is for a
> reason that share_xen_page_with_guest() assigns the target
> domain as the owner of such pages, as that allows get_page() to
> succeed for them.
> 

So, nothing I'm doing here is making anything worse, right? Grant tables are 
assigned to the guest, and IOREQ server pages are allocated with 
alloc_domheap_page() so nothing is anonymous.

  Paul

> Jan


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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-20 Thread Jan Beulich 
>>> On 19.10.17 at 18:21,  wrote:
> Looking a bit more at the resource you can acquire from this hypercall. 
> Some of them are allocated using alloc_xenheap_page() so not assigned to 
> a domain.
> 
> So I am not sure how you can expect a function set_foreign_p2m_entry to 
> take reference in that case.

Hmm, with the domain parameter added, DOMID_XEN there (for
Xen heap pages) could identify no references to be taken, if that
was really the intended behavior in that case. However, even for
Xen heap pages life time tracking ought to be done - it is for a
reason that share_xen_page_with_guest() assigns the target
domain as the owner of such pages, as that allows get_page() to
succeed for them.

Jan


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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-20 Thread Jan Beulich 
>>> On 19.10.17 at 18:06,  wrote:
> On 19/10/17 16:47, Jan Beulich wrote:
>> I don't understand: The refcounting is to be done by ARM-specific
>> code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
>> not its caller. At least that's what I would have expected.
> 
> I thought I said it before, but it looks like not. Assuming the MFN is 
> always baked by a domain, the prototype would likely need to be extended 
> and take the foreign domain.
> 
> If it is not the case, we would need to find another way to do refcounting.

Well, adding another parameter can't be that bad of a problem to solve.

Jan


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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Julien Grall 

Hi,

On 19/10/17 17:06, Julien Grall wrote:

On 19/10/17 16:47, Jan Beulich wrote:

On 19.10.17 at 17:37,  wrote:

Hi,

On 19/10/17 16:11, Jan Beulich wrote:

On 19.10.17 at 16:49,  wrote:
I'd prefer to make the whole thing x86-only since that's the only 
platform

on which I can test it, and indeed the code used to be x86-only. Jan

objected
to this so all I'm trying to achieve is that it builds for ARM. 
Please can

you and
Jan reach agreement on where the code should live and how, if at 
all, it

should be #ifdef-ed?

I am quite surprised of "it is tools-only" so it is fine to not 
protect
it even if it is x86 only. That's probably going to bite us in the 
future.




So, this appears to have reached an impasse. I don't know how to 
proceed
without having to also fix priv mapping for x86, which is a yak far 
too

large

for me to shave at the moment.


Julien,

why is it that you are making refcounting on p2m insertion / removal
a requirement for this series? We all know it's not there right now
(and similarly also not for the IOMMU, which might affect ARM as well
unless you always use shared page tables), and it's - as Paul validly
says - unrelated to his series.


Well, we do at least have refcounting for foreign mapping on Arm. So if
a foreign domain remove the mapping, the page will stay allocated until
the last mapping is removed. For IOMMU, page tables are for the moment
always shared.

If you don't want to fix x86 now, then it is fine. But I would
appreciate if you don't spread that on Arm.

To give you a bit more context, I was ready to implement the Arm version
of set_foreign_p2m_entry (it is quite trivial) to append at the end of
this series. But given that refcounting is not done, I am more reluctant
to do that.


I don't understand: The refcounting is to be done by ARM-specific
code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
not its caller. At least that's what I would have expected.


I thought I said it before, but it looks like not. Assuming the MFN is 
always baked by a domain, the prototype would likely need to be extended 
and take the foreign domain.


If it is not the case, we would need to find another way to do refcounting.


Looking a bit more at the resource you can acquire from this hypercall. 
Some of them are allocated using alloc_xenheap_page() so not assigned to 
a domain.


So I am not sure how you can expect a function set_foreign_p2m_entry to 
take reference in that case.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Julien Grall 

Hi,

On 19/10/17 16:47, Jan Beulich wrote:

On 19.10.17 at 17:37,  wrote:

Hi,

On 19/10/17 16:11, Jan Beulich wrote:

On 19.10.17 at 16:49,  wrote:
I'd prefer to make the whole thing x86-only since that's the only platform

on which I can test it, and indeed the code used to be x86-only. Jan

objected

to this so all I'm trying to achieve is that it builds for ARM. Please can

you and

Jan reach agreement on where the code should live and how, if at all, it
should be #ifdef-ed?

I am quite surprised of "it is tools-only" so it is fine to not protect
it even if it is x86 only. That's probably going to bite us in the future.



So, this appears to have reached an impasse. I don't know how to proceed
without having to also fix priv mapping for x86, which is a yak far too

large

for me to shave at the moment.


Julien,

why is it that you are making refcounting on p2m insertion / removal
a requirement for this series? We all know it's not there right now
(and similarly also not for the IOMMU, which might affect ARM as well
unless you always use shared page tables), and it's - as Paul validly
says - unrelated to his series.


Well, we do at least have refcounting for foreign mapping on Arm. So if
a foreign domain remove the mapping, the page will stay allocated until
the last mapping is removed. For IOMMU, page tables are for the moment
always shared.

If you don't want to fix x86 now, then it is fine. But I would
appreciate if you don't spread that on Arm.

To give you a bit more context, I was ready to implement the Arm version
of set_foreign_p2m_entry (it is quite trivial) to append at the end of
this series. But given that refcounting is not done, I am more reluctant
to do that.


I don't understand: The refcounting is to be done by ARM-specific
code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
not its caller. At least that's what I would have expected.


I thought I said it before, but it looks like not. Assuming the MFN is 
always baked by a domain, the prototype would likely need to be extended 
and take the foreign domain.


If it is not the case, we would need to find another way to do refcounting.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Jan Beulich 
>>> On 19.10.17 at 17:37,  wrote:
> Hi,
> 
> On 19/10/17 16:11, Jan Beulich wrote:
> On 19.10.17 at 16:49,  wrote:
> I'd prefer to make the whole thing x86-only since that's the only platform
 on which I can test it, and indeed the code used to be x86-only. Jan 
> objected
 to this so all I'm trying to achieve is that it builds for ARM. Please can 
> you and
 Jan reach agreement on where the code should live and how, if at all, it
 should be #ifdef-ed?

 I am quite surprised of "it is tools-only" so it is fine to not protect
 it even if it is x86 only. That's probably going to bite us in the future.

>>>
>>> So, this appears to have reached an impasse. I don't know how to proceed
>>> without having to also fix priv mapping for x86, which is a yak far too 
> large
>>> for me to shave at the moment.
>> 
>> Julien,
>> 
>> why is it that you are making refcounting on p2m insertion / removal
>> a requirement for this series? We all know it's not there right now
>> (and similarly also not for the IOMMU, which might affect ARM as well
>> unless you always use shared page tables), and it's - as Paul validly
>> says - unrelated to his series.
> 
> Well, we do at least have refcounting for foreign mapping on Arm. So if 
> a foreign domain remove the mapping, the page will stay allocated until 
> the last mapping is removed. For IOMMU, page tables are for the moment 
> always shared.
> 
> If you don't want to fix x86 now, then it is fine. But I would 
> appreciate if you don't spread that on Arm.
> 
> To give you a bit more context, I was ready to implement the Arm version 
> of set_foreign_p2m_entry (it is quite trivial) to append at the end of 
> this series. But given that refcounting is not done, I am more reluctant 
> to do that.

I don't understand: The refcounting is to be done by ARM-specific
code anyway, i.e. by the implementation of set_foreign_p2m_entry(),
not its caller. At least that's what I would have expected.

Jan


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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Julien Grall 

Hi,

On 19/10/17 16:11, Jan Beulich wrote:

On 19.10.17 at 16:49,  wrote:
I'd prefer to make the whole thing x86-only since that's the only platform

on which I can test it, and indeed the code used to be x86-only. Jan objected
to this so all I'm trying to achieve is that it builds for ARM. Please can you 
and
Jan reach agreement on where the code should live and how, if at all, it
should be #ifdef-ed?

I am quite surprised of "it is tools-only" so it is fine to not protect
it even if it is x86 only. That's probably going to bite us in the future.



So, this appears to have reached an impasse. I don't know how to proceed
without having to also fix priv mapping for x86, which is a yak far too large
for me to shave at the moment.


Julien,

why is it that you are making refcounting on p2m insertion / removal
a requirement for this series? We all know it's not there right now
(and similarly also not for the IOMMU, which might affect ARM as well
unless you always use shared page tables), and it's - as Paul validly
says - unrelated to his series.


Well, we do at least have refcounting for foreign mapping on Arm. So if 
a foreign domain remove the mapping, the page will stay allocated until 
the last mapping is removed. For IOMMU, page tables are for the moment 
always shared.


If you don't want to fix x86 now, then it is fine. But I would 
appreciate if you don't spread that on Arm.


To give you a bit more context, I was ready to implement the Arm version 
of set_foreign_p2m_entry (it is quite trivial) to append at the end of 
this series. But given that refcounting is not done, I am more reluctant 
to do that.


Anyway, I don't plan to block common code. But I will block any 
implementation of set_foreign_p2m_entry (other than returning not 
implemented) on Arm until someone step up and fix the refcounting.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Jan Beulich 
>>> On 19.10.17 at 16:49,  wrote:
>> > I'd prefer to make the whole thing x86-only since that's the only platform
>> on which I can test it, and indeed the code used to be x86-only. Jan objected
>> to this so all I'm trying to achieve is that it builds for ARM. Please can 
>> you and
>> Jan reach agreement on where the code should live and how, if at all, it
>> should be #ifdef-ed?
>> 
>> I am quite surprised of "it is tools-only" so it is fine to not protect
>> it even if it is x86 only. That's probably going to bite us in the future.
>> 
> 
> So, this appears to have reached an impasse. I don't know how to proceed 
> without having to also fix priv mapping for x86, which is a yak far too large 
> for me to shave at the moment.

Julien,

why is it that you are making refcounting on p2m insertion / removal
a requirement for this series? We all know it's not there right now
(and similarly also not for the IOMMU, which might affect ARM as well
unless you always use shared page tables), and it's - as Paul validly
says - unrelated to his series.

Jan


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

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Paul Durrant 
> -Original Message-
[snip]
> >
> > I'd prefer to make the whole thing x86-only since that's the only platform
> on which I can test it, and indeed the code used to be x86-only. Jan objected
> to this so all I'm trying to achieve is that it builds for ARM. Please can 
> you and
> Jan reach agreement on where the code should live and how, if at all, it
> should be #ifdef-ed?
> 
> I am quite surprised of "it is tools-only" so it is fine to not protect
> it even if it is x86 only. That's probably going to bite us in the future.
> 

So, this appears to have reached an impasse. I don't know how to proceed 
without having to also fix priv mapping for x86, which is a yak far too large 
for me to shave at the moment.

  Paul
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Julien Grall 

Hi,

On 19/10/17 14:35, Paul Durrant wrote:

-Original Message-
From: Julien Grall [mailto:julien.gr...@linaro.org]
Sent: 19 October 2017 14:29
To: Paul Durrant <paul.durr...@citrix.com>; xen-de...@lists.xenproject.org
Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
<wei.l...@citrix.com>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
George Dunlap <george.dun...@citrix.com>; Andrew Cooper
<andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
(Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan Beulich
<jbeul...@suse.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>; Roger
Pau Monne <roger....@citrix.com>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources

Hi,

On 10/19/2017 01:57 PM, Paul Durrant wrote:

-Original Message-
From: Julien Grall [mailto:julien.gr...@linaro.org]
Sent: 19 October 2017 13:23
To: Paul Durrant <paul.durr...@citrix.com>; xen-

de...@lists.xenproject.org

Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
<wei.l...@citrix.com>; Konrad Rzeszutek Wilk

<konrad.w...@oracle.com>;

George Dunlap <george.dun...@citrix.com>; Andrew Cooper
<andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;

Tim

(Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan

Beulich

<jbeul...@suse.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>; Roger
Pau Monne <roger@citrix.com>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources

Hi,

On 17/10/17 14:24, Paul Durrant wrote:

Certain memory resources associated with a guest are not necessarily
present in the guest P2M.

This patch adds the boilerplate for new memory op to allow such a

resource

to be priv-mapped directly, by either a PV or HVM tools domain.

NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
 I have no means to test it on an ARM platform and so cannot verify
 that it functions correctly.

Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
---


[...]


diff --git a/xen/common/memory.c b/xen/common/memory.c
index ad987e0f29..cdd2e030cf 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -965,6 +965,95 @@ static long xatp_permission_check(struct

domain

*d, unsigned int space)

[...]


+if ( rc )
+goto out;
+
+if ( !paging_mode_translate(currd) )
+{
+if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
+rc = -EFAULT;
+}
+else
+{
+xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
+unsigned int i;
+
+rc = -EFAULT;
+if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
+goto out;
+
+for ( i = 0; i < xmar.nr_frames; i++ )
+{
+rc = set_foreign_p2m_entry(currd, gfn_list[i],
+   _mfn(mfn_list[i]));


Something looks a bit odd to me here. When I read foreign mapping, I
directly associate to mapping from a foreign domain.

On Arm, we will always get a reference on that page to prevent it
disappearing if the foreign domain is destroyed but the mapping is still
present.

This reference will either be put with an unmapped hypercall or while
teardown the domain.

Per my understanding, this MFN does not belong to any domain (or at
least currd). Right?


No. The mfns do belong to the target domain.


To be fully safe, you need to take a reference on each page you mapped.
So who is going to get a reference on them? Who is going to drop that?



Yes, that's true but it's also true of priv mapping AIUI. I think the correct 
fix is to deal with this in set_p2m_foreign_entry() so that it is fixed for 
both cases. I don't think it is something that ought to be addressed here... 
unless I'm missing something.


For x86 maybe. For Arm, foreign mapping are also exposed to guest and we 
get a reference every time. It is probably an oversight on the x86 side.







So there is no way to get/put a reference on that
page. So I am unconvinced that this is very safe.

Also looking at the x86 side, I can't find such reference in the foreign
path in p2m_add_foreign. Did I miss anything?


No, I don't think there is any reference counting there... but this is no

different to priv mapping. I'm not trying to fix the mapping infrastructure at
this point.




Note that x86 does not handle p2m teardown with foreign map at the
moment (see p2m_add_foreign).

You are by-passing this check and I can't see how this would be safe for
the x86 side too.



I don't follow. What check am I by-passing that is covered when priv

mapping?

  /*
   * hvm fixme: until support is added to p2m teardown code to
cleanup any
   * foreign entries, limit this to hardware domain only.
   */

How this is safe with your n

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Paul Durrant 
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@linaro.org]
> Sent: 19 October 2017 14:29
> To: Paul Durrant <paul.durr...@citrix.com>; xen-de...@lists.xenproject.org
> Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> <wei.l...@citrix.com>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
> George Dunlap <george.dun...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan Beulich
> <jbeul...@suse.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>; Roger
> Pau Monne <roger@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> Hi,
> 
> On 10/19/2017 01:57 PM, Paul Durrant wrote:
> >> -Original Message-
> >> From: Julien Grall [mailto:julien.gr...@linaro.org]
> >> Sent: 19 October 2017 13:23
> >> To: Paul Durrant <paul.durr...@citrix.com>; xen-
> de...@lists.xenproject.org
> >> Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> >> <wei.l...@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.w...@oracle.com>;
> >> George Dunlap <george.dun...@citrix.com>; Andrew Cooper
> >> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Tim
> >> (Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan
> Beulich
> >> <jbeul...@suse.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>; Roger
> >> Pau Monne <roger@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi,
> >>
> >> On 17/10/17 14:24, Paul Durrant wrote:
> >>> Certain memory resources associated with a guest are not necessarily
> >>> present in the guest P2M.
> >>>
> >>> This patch adds the boilerplate for new memory op to allow such a
> >> resource
> >>> to be priv-mapped directly, by either a PV or HVM tools domain.
> >>>
> >>> NOTE: Whilst the new op is not intrinsicly specific to the x86 
> >>> architecture,
> >>> I have no means to test it on an ARM platform and so cannot verify
> >>> that it functions correctly.
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> >>> ---
> >>
> >> [...]
> >>
> >>> diff --git a/xen/common/memory.c b/xen/common/memory.c
> >>> index ad987e0f29..cdd2e030cf 100644
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct
> domain
> >> *d, unsigned int space)
> >>
> >> [...]
> >>
> >>> +if ( rc )
> >>> +goto out;
> >>> +
> >>> +if ( !paging_mode_translate(currd) )
> >>> +{
> >>> +if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> >>> +rc = -EFAULT;
> >>> +}
> >>> +else
> >>> +{
> >>> +xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> >>> +unsigned int i;
> >>> +
> >>> +rc = -EFAULT;
> >>> +if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> >>> +goto out;
> >>> +
> >>> +for ( i = 0; i < xmar.nr_frames; i++ )
> >>> +{
> >>> +rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>> +   _mfn(mfn_list[i]));
> >>
> >> Something looks a bit odd to me here. When I read foreign mapping, I
> >> directly associate to mapping from a foreign domain.
> >>
> >> On Arm, we will always get a reference on that page to prevent it
> >> disappearing if the foreign domain is destroyed but the mapping is still
> >> present.
> >>
> >> This reference will either be put with an unmapped hypercall or while
> >> teardown the domain.
> >>
> >> Per my understanding, this MFN does not belong to any domain (or at
> >> least currd). Right?
> >
> > No. The mfns do belong to the target domain.
> 
> To be fully safe, you need to take a reference on each page you mapped.
> So who is going to get a ref

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Julien Grall 

Hi,

On 10/19/2017 01:57 PM, Paul Durrant wrote:

-Original Message-
From: Julien Grall [mailto:julien.gr...@linaro.org]
Sent: 19 October 2017 13:23
To: Paul Durrant <paul.durr...@citrix.com>; xen-de...@lists.xenproject.org
Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
<wei.l...@citrix.com>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
George Dunlap <george.dun...@citrix.com>; Andrew Cooper
<andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
(Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan Beulich
<jbeul...@suse.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>; Roger
Pau Monne <roger....@citrix.com>
Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
HYPERVISOR_memory_op to acquire guest resources

Hi,

On 17/10/17 14:24, Paul Durrant wrote:

Certain memory resources associated with a guest are not necessarily
present in the guest P2M.

This patch adds the boilerplate for new memory op to allow such a

resource

to be priv-mapped directly, by either a PV or HVM tools domain.

NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
I have no means to test it on an ARM platform and so cannot verify
that it functions correctly.

Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
---


[...]


diff --git a/xen/common/memory.c b/xen/common/memory.c
index ad987e0f29..cdd2e030cf 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain

*d, unsigned int space)

[...]


+if ( rc )
+goto out;
+
+if ( !paging_mode_translate(currd) )
+{
+if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
+rc = -EFAULT;
+}
+else
+{
+xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
+unsigned int i;
+
+rc = -EFAULT;
+if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
+goto out;
+
+for ( i = 0; i < xmar.nr_frames; i++ )
+{
+rc = set_foreign_p2m_entry(currd, gfn_list[i],
+   _mfn(mfn_list[i]));


Something looks a bit odd to me here. When I read foreign mapping, I
directly associate to mapping from a foreign domain.

On Arm, we will always get a reference on that page to prevent it
disappearing if the foreign domain is destroyed but the mapping is still
present.

This reference will either be put with an unmapped hypercall or while
teardown the domain.

Per my understanding, this MFN does not belong to any domain (or at
least currd). Right?
 
No. The mfns do belong to the target domain.


To be fully safe, you need to take a reference on each page you mapped. 
So who is going to get a reference on them? Who is going to drop that?





So there is no way to get/put a reference on that
page. So I am unconvinced that this is very safe.

Also looking at the x86 side, I can't find such reference in the foreign
path in p2m_add_foreign. Did I miss anything?


No, I don't think there is any reference counting there... but this is no 
different to priv mapping. I'm not trying to fix the mapping infrastructure at 
this point.



Note that x86 does not handle p2m teardown with foreign map at the
moment (see p2m_add_foreign).

You are by-passing this check and I can't see how this would be safe for
the x86 side too.



I don't follow. What check am I by-passing that is covered when priv mapping?


/*
 * hvm fixme: until support is added to p2m teardown code to 
cleanup any

 * foreign entries, limit this to hardware domain only.
 */

How this is safe with your new solution? That looks like a regression...

[...]


+ *  will be populated with the MFNs of the resource.
+ *  If the tools domain is HVM then it is expected that, on
+ *  entry, frame_list will be populated with a list of GFNs
+ *  that will be mapped to the MFNs of the resource.
+ *  If -EIO is returned then the frame_list has only been
+ *  partially mapped and it is up to the caller to unmap all
+ *  the GFNs.
+ *  This parameter may be NULL if nr_frames is 0.
+ */
+XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
+};
+typedef struct xen_mem_acquire_resource

xen_mem_acquire_resource_t;

+DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
+
   #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */

   /*




Sorry to be getting frustrated with this, but I'm wondering how many more 
colours I need to paint this bike-shed.


I don't know how x86 looks like and maybe this is fine for Andrew and 
Jan. But for Arm, it does not look correct.


To give you an idea, my first thought to implement your newly wrongly 
named function was to just call p2m_set_entry with p2m_map_foreign. But 
from this discussion it would look plain wr

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Paul Durrant 
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@linaro.org]
> Sent: 19 October 2017 13:23
> To: Paul Durrant <paul.durr...@citrix.com>; xen-de...@lists.xenproject.org
> Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> <wei.l...@citrix.com>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
> George Dunlap <george.dun...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan Beulich
> <jbeul...@suse.com>; Daniel De Graaf <dgde...@tycho.nsa.gov>; Roger
> Pau Monne <roger@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> Hi,
> 
> On 17/10/17 14:24, Paul Durrant wrote:
> > Certain memory resources associated with a guest are not necessarily
> > present in the guest P2M.
> >
> > This patch adds the boilerplate for new memory op to allow such a
> resource
> > to be priv-mapped directly, by either a PV or HVM tools domain.
> >
> > NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
> >I have no means to test it on an ARM platform and so cannot verify
> >that it functions correctly.
> >
> > Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
> > ---
> 
> [...]
> 
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index ad987e0f29..cdd2e030cf 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain
> *d, unsigned int space)
> 
> [...]
> 
> > +if ( rc )
> > +goto out;
> > +
> > +if ( !paging_mode_translate(currd) )
> > +{
> > +if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> > +rc = -EFAULT;
> > +}
> > +else
> > +{
> > +xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> > +unsigned int i;
> > +
> > +rc = -EFAULT;
> > +if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> > +goto out;
> > +
> > +for ( i = 0; i < xmar.nr_frames; i++ )
> > +{
> > +rc = set_foreign_p2m_entry(currd, gfn_list[i],
> > +   _mfn(mfn_list[i]));
> 
> Something looks a bit odd to me here. When I read foreign mapping, I
> directly associate to mapping from a foreign domain.
> 
> On Arm, we will always get a reference on that page to prevent it
> disappearing if the foreign domain is destroyed but the mapping is still
> present.
> 
> This reference will either be put with an unmapped hypercall or while
> teardown the domain.
> 
> Per my understanding, this MFN does not belong to any domain (or at
> least currd). Right?

No. The mfns do belong to the target domain.

> So there is no way to get/put a reference on that
> page. So I am unconvinced that this is very safe.
> 
> Also looking at the x86 side, I can't find such reference in the foreign
> path in p2m_add_foreign. Did I miss anything?

No, I don't think there is any reference counting there... but this is no 
different to priv mapping. I'm not trying to fix the mapping infrastructure at 
this point.

> 
> Note that x86 does not handle p2m teardown with foreign map at the
> moment (see p2m_add_foreign).
> 
> You are by-passing this check and I can't see how this would be safe for
> the x86 side too.
> 

I don't follow. What check am I by-passing that is covered when priv mapping?

> > +if ( rc )
> > +{
> > +/*
> > + * Make sure rc is -EIO for any interation other than
> > + * the first.
> > + */
> > +rc = (i != 0) ? -EIO : rc;
> > +break;
> > +}
> > +}
> > +}
> > +
> > + out:
> > +rcu_unlock_domain(d);
> > +return rc;
> > +}
> > +
> >   long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >   {
> >   struct domain *d, *curr_d = current->domain;
> > @@ -1406,6 +1495,11 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >   }
> >   #endif
> >
> > +case XENMEM_acquire_resource:
> > +rc = acquire_resource(
> > +guest_handle_cast(arg, xen_mem_acquire_resource_t));
> > +break;
> > +
> >

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-19 Thread Julien Grall 

Hi,

On 17/10/17 14:24, Paul Durrant wrote:

Certain memory resources associated with a guest are not necessarily
present in the guest P2M.

This patch adds the boilerplate for new memory op to allow such a resource
to be priv-mapped directly, by either a PV or HVM tools domain.

NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
   I have no means to test it on an ARM platform and so cannot verify
   that it functions correctly.

Signed-off-by: Paul Durrant 
---


[...]


diff --git a/xen/common/memory.c b/xen/common/memory.c
index ad987e0f29..cdd2e030cf 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain *d, 
unsigned int space)


[...]


+if ( rc )
+goto out;
+
+if ( !paging_mode_translate(currd) )
+{
+if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
+rc = -EFAULT;
+}
+else
+{
+xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
+unsigned int i;
+
+rc = -EFAULT;
+if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
+goto out;
+
+for ( i = 0; i < xmar.nr_frames; i++ )
+{
+rc = set_foreign_p2m_entry(currd, gfn_list[i],
+   _mfn(mfn_list[i]));


Something looks a bit odd to me here. When I read foreign mapping, I 
directly associate to mapping from a foreign domain.


On Arm, we will always get a reference on that page to prevent it 
disappearing if the foreign domain is destroyed but the mapping is still 
present.


This reference will either be put with an unmapped hypercall or while 
teardown the domain.


Per my understanding, this MFN does not belong to any domain (or at 
least currd). Right? So there is no way to get/put a reference on that 
page. So I am unconvinced that this is very safe.


Also looking at the x86 side, I can't find such reference in the foreign 
path in p2m_add_foreign. Did I miss anything?


Note that x86 does not handle p2m teardown with foreign map at the 
moment (see p2m_add_foreign).


You are by-passing this check and I can't see how this would be safe for 
the x86 side too.



+if ( rc )
+{
+/*
+ * Make sure rc is -EIO for any interation other than
+ * the first.
+ */
+rc = (i != 0) ? -EIO : rc;
+break;
+}
+}
+}
+
+ out:
+rcu_unlock_domain(d);
+return rc;
+}
+
  long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
  {
  struct domain *d, *curr_d = current->domain;
@@ -1406,6 +1495,11 @@ long do_memory_op(unsigned long cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
  }
  #endif
  
+case XENMEM_acquire_resource:

+rc = acquire_resource(
+guest_handle_cast(arg, xen_mem_acquire_resource_t));
+break;
+
  default:
  rc = arch_memory_op(cmd, arg);
  break;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index faadcfe8fe..a5caa747ce 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -346,6 +346,12 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned 
int order)
  return gfn_add(gfn, 1UL << order);
  }
  
+static inline int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,


Please modifify the prototype to use gfn_t.


+mfn_t mfn)
+{
+return -EOPNOTSUPP;
+} > +
  #endif /* _XEN_P2M_H */
  


[...]


diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 29386df98b..18118ea5c6 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
  typedef struct xen_reserved_device_memory_map 
xen_reserved_device_memory_map_t;
  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
  
+/*

+ * Get the pages for a particular guest resource, so that they can be
+ * mapped directly by a tools domain.
+ */
+#define XENMEM_acquire_resource 28
+struct xen_mem_acquire_resource {
+/* IN - the domain whose resource is to be mapped */
+domid_t domid;
+/* IN - the type of resource */
+uint16_t type;
+/*
+ * IN - a type-specific resource identifier, which must be zero
+ *  unless stated otherwise.
+ */
+uint32_t id;
+/* IN/OUT - As an IN parameter number of frames of the resource


Coding style:

/*
 *


+ *  to be mapped. However, if the specified value is 0 and
+ *  frame_list is NULL then this field will be set to the
+ *  maximum value supported by the implementation on return.
+ */
+uint32_t nr_frames;
+uint32_t pad;
+/* IN - the index of the initial frame to be mapped. This parameter


Ditto


+ *  is ignored if nr_frames is 0.
+ */
+uint64_aligned_t

Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

2017-10-17 Thread Daniel De Graaf 

On 10/17/2017 09:24 AM, Paul Durrant wrote:

Certain memory resources associated with a guest are not necessarily
present in the guest P2M.

This patch adds the boilerplate for new memory op to allow such a resource
to be priv-mapped directly, by either a PV or HVM tools domain.

NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
   I have no means to test it on an ARM platform and so cannot verify
   that it functions correctly.

Signed-off-by: Paul Durrant 


Acked-by: Daniel De Graaf 

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