Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[Andres Lagar-Cavilla]

-- Resend as I haven't seen this hit the lists. Maybe some smtp misconfig. Apologies. Also expanded cc --

When a foreign mapper attempts to map guest frames that are paged out,
the mapper receives an ENOENT response and will have to try again
while a helper process pages the target frame back in.

Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
of mapping calls.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
---
drivers/xen/privcmd.c |   32 +++++++++++++++++++++++++++-----
1 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index f8e5dd7..44a26c6 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -43,9 +43,12 @@ MODULE_LICENSE("GPL");

#define PRIV_VMA_LOCKED ((void *)1)

-#ifndef HAVE_ARCH_PRIVCMD_MMAP
static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma);
-#endif
+
+static int privcmd_enforce_singleshot_mapping_granular(
+               struct vm_area_struct *vma,
+               unsigned long addr,
+               unsigned long nr_pages);

static long privcmd_ioctl_hypercall(void __user *udata)
{
@@ -422,9 +425,9 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
	vma = find_vma(mm, m.addr);
	if (!vma ||
	    vma->vm_ops != &privcmd_vm_ops ||
-	    (m.addr != vma->vm_start) ||
-	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
-	    !privcmd_enforce_singleshot_mapping(vma)) {
+	    (m.addr < vma->vm_start) ||
+	    ((m.addr + (nr_pages << PAGE_SHIFT)) > vma->vm_end) ||
+	    !privcmd_enforce_singleshot_mapping_granular(vma, m.addr, nr_pages)) {
		up_write(&mm->mmap_sem);
		ret = -EINVAL;
		goto out;
@@ -540,11 +543,30 @@ static int privcmd_mmap(struct file *file, struct vm_area_struct *vma)
	return 0;
}

+/* For MMAP */
static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma)
{
	return !cmpxchg(&vma->vm_private_data, NULL, PRIV_VMA_LOCKED);
}

+/* For MMAPBATCH*. This allows asserting the singleshot mapping
+ * on a per pfn/pte basis. Mapping calls that fail with ENOENT
+ * can be then retried until success. */
+static int enforce_singleshot_mapping_fn(pte_t *pte, struct page *pmd_page,
+	                unsigned long addr, void *data)
+{
+	return pte_none(*pte) ? 0 : -EBUSY;
+}
+
+static int privcmd_enforce_singleshot_mapping_granular(
+	           struct vm_area_struct *vma,
+	           unsigned long addr,
+	           unsigned long nr_pages)
+{
+	return apply_to_page_range(vma->vm_mm, addr, nr_pages << PAGE_SHIFT,
+	              enforce_singleshot_mapping_fn, NULL) == 0;
+}
+
const struct file_operations xen_privcmd_fops = {
	.owner = THIS_MODULE,
	.unlocked_ioctl = privcmd_ioctl,
-- 
1.7.1
> 

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[David Vrabel]

On 01/08/13 04:30, Andres Lagar-Cavilla wrote:
> -- Resend as I haven't seen this hit the lists. Maybe some smtp misconfig. Apologies. Also expanded cc --
> 
> When a foreign mapper attempts to map guest frames that are paged out,
> the mapper receives an ENOENT response and will have to try again
> while a helper process pages the target frame back in.
> 
> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
> of mapping calls.

This breaks the auto_translated_physmap case as will allocate another
set of empty pages and leak the previous set.

This privcmd_enforce_singleshot_mapping() stuff seems very odd anyway.
Does anyone know what it was for originally?  It would be preferrable if
we could update the mappings with a new set of foreign MFNs without
having to tear down the VMA and recreate a new VMA.

> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
[...]
> +/* For MMAPBATCH*. This allows asserting the singleshot mapping
> + * on a per pfn/pte basis. Mapping calls that fail with ENOENT
> + * can be then retried until success. */

Comment coding style.

David

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[Andres Lagar-Cavilla]

On Aug 1, 2013, at 7:23 AM, David Vrabel <david.vrabel@citrix.com> wrote:

> On 01/08/13 04:30, Andres Lagar-Cavilla wrote:
>> -- Resend as I haven't seen this hit the lists. Maybe some smtp misconfig. Apologies. Also expanded cc --
>> 
>> When a foreign mapper attempts to map guest frames that are paged out,
>> the mapper receives an ENOENT response and will have to try again
>> while a helper process pages the target frame back in.
>> 
>> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
>> of mapping calls.
> 
> This breaks the auto_translated_physmap case as will allocate another
> set of empty pages and leak the previous set.

David,
not able to follow you here. Under what circumstances will another set of empty pages be allocated? And where? are we talking page table pages?

> 
> This privcmd_enforce_singleshot_mapping() stuff seems very odd anyway.
> Does anyone know what it was for originally?  It would be preferrable if
> we could update the mappings with a new set of foreign MFNs without
> having to tear down the VMA and recreate a new VMA.

I believe it's mostly historical. I agree with you on principle, but recreating VMAs is super-cheap.

Andres
> 
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
> [...]
>> +/* For MMAPBATCH*. This allows asserting the singleshot mapping
>> + * on a per pfn/pte basis. Mapping calls that fail with ENOENT
>> + * can be then retried until success. */
> 
> Comment coding style.
> 
> David

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[David Vrabel]

On 01/08/13 12:49, Andres Lagar-Cavilla wrote:
> On Aug 1, 2013, at 7:23 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> 
>> On 01/08/13 04:30, Andres Lagar-Cavilla wrote:
>>> -- Resend as I haven't seen this hit the lists. Maybe some smtp misconfig. Apologies. Also expanded cc --
>>>
>>> When a foreign mapper attempts to map guest frames that are paged out,
>>> the mapper receives an ENOENT response and will have to try again
>>> while a helper process pages the target frame back in.
>>>
>>> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
>>> of mapping calls.
>>
>> This breaks the auto_translated_physmap case as will allocate another
>> set of empty pages and leak the previous set.
> 
> David,
> not able to follow you here. Under what circumstances will another
> set of empty pages be allocated? And where? are we talking page table pages?

	....
	vma = find_vma(mm, m.addr);
	if (!vma ||
	    vma->vm_ops != &privcmd_vm_ops ||
	    (m.addr != vma->vm_start) ||
	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
	    !privcmd_enforce_singleshot_mapping(vma)) {
		up_write(&mm->mmap_sem);
		ret = -EINVAL;
		goto out;
	}
	if (xen_feature(XENFEAT_auto_translated_physmap)) {
		ret = alloc_empty_pages(vma, m.num);

Here.

		if (ret < 0) {
			up_write(&mm->mmap_sem);
			goto out;
		}
	}


>> This privcmd_enforce_singleshot_mapping() stuff seems very odd anyway.
>> Does anyone know what it was for originally?  It would be preferrable if
>> we could update the mappings with a new set of foreign MFNs without
>> having to tear down the VMA and recreate a new VMA.
> 
> I believe it's mostly historical. I agree with you on principle, but recreating VMAs is super-cheap.

Tearing them down is not cheap as each page requires a trap-and-emulate
to clear the PTE (see ptep_get_and_clear_full() in zap_pte_range()).

David

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[Andres Lagar-Cavilla]

On Aug 1, 2013, at 8:04 AM, David Vrabel <david.vrabel@citrix.com> wrote:

> On 01/08/13 12:49, Andres Lagar-Cavilla wrote:
>> On Aug 1, 2013, at 7:23 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>> 
>>> On 01/08/13 04:30, Andres Lagar-Cavilla wrote:
>>>> -- Resend as I haven't seen this hit the lists. Maybe some smtp misconfig. Apologies. Also expanded cc --
>>>> 
>>>> When a foreign mapper attempts to map guest frames that are paged out,
>>>> the mapper receives an ENOENT response and will have to try again
>>>> while a helper process pages the target frame back in.
>>>> 
>>>> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
>>>> of mapping calls.
>>> 
>>> This breaks the auto_translated_physmap case as will allocate another
>>> set of empty pages and leak the previous set.
>> 
>> David,
>> not able to follow you here. Under what circumstances will another
>> set of empty pages be allocated? And where? are we talking page table pages?
> 
> 	....
> 	vma = find_vma(mm, m.addr);
> 	if (!vma ||
> 	    vma->vm_ops != &privcmd_vm_ops ||
> 	    (m.addr != vma->vm_start) ||
> 	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
> 	    !privcmd_enforce_singleshot_mapping(vma)) {
> 		up_write(&mm->mmap_sem);
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 	if (xen_feature(XENFEAT_auto_translated_physmap)) {
> 		ret = alloc_empty_pages(vma, m.num);
> 
> Here.

Right right right. Excellent observation thanks. I fwd ported from 3.4 and this slipped through the cracks. Ok, V2 coming.
> 
> 		if (ret < 0) {
> 			up_write(&mm->mmap_sem);
> 			goto out;
> 		}
> 	}
> 
> 
>>> This privcmd_enforce_singleshot_mapping() stuff seems very odd anyway.
>>> Does anyone know what it was for originally?  It would be preferrable if
>>> we could update the mappings with a new set of foreign MFNs without
>>> having to tear down the VMA and recreate a new VMA.
>> 
>> I believe it's mostly historical. I agree with you on principle, but recreating VMAs is super-cheap.
> 
> Tearing them down is not cheap as each page requires a trap-and-emulate
> to clear the PTE (see ptep_get_and_clear_full() in zap_pte_range()).

You need to tell the hypervisor to drop the ref on the mapped page. So you'd need a hyper call (arguably a multi-call) to do that, which is not free. Then you'd need privcmd and libxc to collude on agreeing to reuse the vma -- which has very low value in itself, just a piece of metadata. And you still need to deal with cleaning up the mapped refs when the mapping process crashes.

So a whole lot of new complexity for small value, imho.

Probably that's the whole point of the singleshot: don't forget you have something mapped in there. Because if you do you might leak the ref forever.

Andres
> 
> David

[PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[Andres Lagar-Cavilla]

From: Andres Lagar-Cavilla <andres@lagarcavilla.org>

When a foreign mapper attempts to map guest frames that are paged out,
the mapper receives an ENOENT response and will have to try again
while a helper process pages the target frame back in.

Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
of mapping calls.

V2: Fixed autotranslated physmap mode breakage introduced by V1.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
---
 drivers/xen/privcmd.c |   41 +++++++++++++++++++++++++++++++++++------
 1 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index f8e5dd7..6ebdf98 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -43,9 +43,12 @@ MODULE_LICENSE("GPL");
 
 #define PRIV_VMA_LOCKED ((void *)1)
 
-#ifndef HAVE_ARCH_PRIVCMD_MMAP
 static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma);
-#endif
+
+static int privcmd_enforce_singleshot_mapping_granular(
+               struct vm_area_struct *vma,
+               unsigned long addr,
+               unsigned long nr_pages);
 
 static long privcmd_ioctl_hypercall(void __user *udata)
 {
@@ -422,14 +425,15 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 	vma = find_vma(mm, m.addr);
 	if (!vma ||
 	    vma->vm_ops != &privcmd_vm_ops ||
-	    (m.addr != vma->vm_start) ||
-	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
-	    !privcmd_enforce_singleshot_mapping(vma)) {
+	    (m.addr < vma->vm_start) ||
+	    ((m.addr + (nr_pages << PAGE_SHIFT)) > vma->vm_end) ||
+	    !privcmd_enforce_singleshot_mapping_granular(vma, m.addr, nr_pages)) {
 		up_write(&mm->mmap_sem);
 		ret = -EINVAL;
 		goto out;
 	}
-	if (xen_feature(XENFEAT_auto_translated_physmap)) {
+	if (xen_feature(XENFEAT_auto_translated_physmap) &&
+	    privcmd_enforce_singleshot_mapping(vma)) {
 		ret = alloc_empty_pages(vma, m.num);
 		if (ret < 0) {
 			up_write(&mm->mmap_sem);
@@ -540,11 +544,36 @@ static int privcmd_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
+/*
+ * For Asserting on a whole VMA. This is used by the legacy PRIVCMD_MMAP
+ * call and autotranslated physmap mode to allocate the ballooned pages that
+ * back a mapping only once.
+ */
 static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma)
 {
 	return !cmpxchg(&vma->vm_private_data, NULL, PRIV_VMA_LOCKED);
 }
 
+/*
+ * For MMAPBATCH*. This allows asserting the singleshot mapping
+ * on a per pfn/pte basis. Mapping calls that fail with ENOENT
+ * can be then retried until success.
+ */
+static int enforce_singleshot_mapping_fn(pte_t *pte, struct page *pmd_page,
+	                unsigned long addr, void *data)
+{
+	return pte_none(*pte) ? 0 : -EBUSY;
+}
+
+static int privcmd_enforce_singleshot_mapping_granular(
+	           struct vm_area_struct *vma,
+	           unsigned long addr,
+	           unsigned long nr_pages)
+{
+	return apply_to_page_range(vma->vm_mm, addr, nr_pages << PAGE_SHIFT,
+	              enforce_singleshot_mapping_fn, NULL) == 0;
+}
+
 const struct file_operations xen_privcmd_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = privcmd_ioctl,
-- 
1.7.1

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[David Vrabel]

On 01/08/13 15:26, Andres Lagar-Cavilla wrote:
> From: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> When a foreign mapper attempts to map guest frames that are paged out,
> the mapper receives an ENOENT response and will have to try again
> while a helper process pages the target frame back in.
> 
> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
> of mapping calls.
> 
> V2: Fixed autotranslated physmap mode breakage introduced by V1.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
>  drivers/xen/privcmd.c |   41 +++++++++++++++++++++++++++++++++++------
>  1 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index f8e5dd7..6ebdf98 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -43,9 +43,12 @@ MODULE_LICENSE("GPL");
>  
>  #define PRIV_VMA_LOCKED ((void *)1)
>  
> -#ifndef HAVE_ARCH_PRIVCMD_MMAP
>  static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma);
> -#endif
> +
> +static int privcmd_enforce_singleshot_mapping_granular(
> +               struct vm_area_struct *vma,
> +               unsigned long addr,
> +               unsigned long nr_pages);
>  
>  static long privcmd_ioctl_hypercall(void __user *udata)
>  {
> @@ -422,14 +425,15 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  	vma = find_vma(mm, m.addr);
>  	if (!vma ||
>  	    vma->vm_ops != &privcmd_vm_ops ||
> -	    (m.addr != vma->vm_start) ||
> -	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
> -	    !privcmd_enforce_singleshot_mapping(vma)) {
> +	    (m.addr < vma->vm_start) ||
> +	    ((m.addr + (nr_pages << PAGE_SHIFT)) > vma->vm_end) ||
> +	    !privcmd_enforce_singleshot_mapping_granular(vma, m.addr, nr_pages)) {
>  		up_write(&mm->mmap_sem);
>  		ret = -EINVAL;
>  		goto out;

This change to the range checks allows the user to partially populate
the VMA on the first call which will result in too few pages being
allocated in the auto_translated_physmap case.

Can you do the following behaviour instead?

On the first call, require the user to try to populate fully the VMA.

On subsequent calls, allow the user to specify a subset of pages
(sub-range) provided all the pages in the subset are not populated.

David

Re: [PATCH] Xen: Fix retry calls into PRIVCMD_MMAPBATCH*.

[David Vrabel]

On 09/08/13 15:11, Andres Lagar-Cavilla wrote:
> From: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> When a foreign mapper attempts to map guest frames that are paged out,
> the mapper receives an ENOENT response and will have to try again
> while a helper process pages the target frame back in.
> 
> Gating checks on PRIVCMD_MMAPBATCH* ioctl args were preventing retries
> of mapping calls.
> 
> V2: Fixed autotranslated physmap mode breakage introduced by V1.
> V3: Enforce full range mapping for first ioctl call on a given VMA.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
>  drivers/xen/privcmd.c |   58 +++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index f8e5dd7..938e6e6 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -43,9 +43,12 @@ MODULE_LICENSE("GPL");
>  
>  #define PRIV_VMA_LOCKED ((void *)1)
>  
> -#ifndef HAVE_ARCH_PRIVCMD_MMAP
>  static int privcmd_enforce_singleshot_mapping(struct vm_area_struct *vma);
> -#endif
> +
> +static int privcmd_enforce_singleshot_mapping_granular(
> +               struct vm_area_struct *vma,
> +               unsigned long addr,
> +               unsigned long nr_pages);
>  
>  static long privcmd_ioctl_hypercall(void __user *udata)
>  {
> @@ -422,19 +425,26 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  	vma = find_vma(mm, m.addr);
>  	if (!vma ||
>  	    vma->vm_ops != &privcmd_vm_ops ||
> -	    (m.addr != vma->vm_start) ||
> -	    ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) ||
> -	    !privcmd_enforce_singleshot_mapping(vma)) {
> +	    (m.addr < vma->vm_start) ||
> +	    ((m.addr + (nr_pages << PAGE_SHIFT)) > vma->vm_end) ||
> +	    !privcmd_enforce_singleshot_mapping_granular(vma, m.addr, nr_pages)) {
>  		up_write(&mm->mmap_sem);
>  		ret = -EINVAL;
>  		goto out;

I think this unnecessarily calls
privcmd_enforce_singleshot_mapping_granular() on the first call.

If you change the tests to the following, does it work for you?

/* Suitable VMA? */
if (!vma || vma->vm_ops != &privcmd_vm_ops)
	// error

if (!privcmd_enforce_singleshot_mapping(..)) {
        /* Already populated into this VMA once, are we populating a
           subset that is not yet populated? */
	if (is_subset_of_vma(vma, m,addr, nr_pages)
            && privcmd_enforce_single_shot_mapping_granular())
        	// ok
	else
		// error
} else {
	/* Nothing populated in this VMA, must try to fully populate
	   it. */
	if (is_all_of_vma(vma, m.addr, nr_pages))
		// ok
	else
		// error
}


Thanks.

David