[PATCH v2 0/2] xen/grant-table: Avoid m2p_override during mapping

[Zoltan Kiss]

The grant mapping API does m2p_override unnecessarily: only gntdev needs it,
for blkback and future netback patches it just cause a lock contention, as
those pages never go to userspace. Therefore this series does the following:
- move the m2p_override part from grant_[un]map_refs to gntdev, where it is
needed after mapping operations
- but move set_phys_to_machine from m2p_override to gnttab_[un]map_refs,
because it is needed always
- update the function prototypes as kmap_ops are no longer needed

v2:
- move the storing of the old mfn in page->index to gnttab_map_refs
- move the function header update to a separate patch

Signed-off-by: Zoltan Kiss <zolta...@citrix.com>
Suggested-by: David Vrabel <david....@citrix.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Zoltan Kiss]

This patch does the following:

- move the m2p_override part from grant_[un]map_refs to gntdev, where it is
needed after mapping operations

- but move set_phys_to_machine from m2p_override to grant_[un]map_refs,

because it is needed always

Signed-off-by: Zoltan Kiss <zolta...@citrix.com>
Suggested-by: David Vrabel <david....@citrix.com>

---
arch/x86/xen/p2m.c | 5 ---
drivers/xen/gntdev.c | 62 ++++++++++++++++++++++++++++++++++---
drivers/xen/grant-table.c | 75 +++++++++++++++------------------------------
3 files changed, 83 insertions(+), 59 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 2ae8699..b1e9407 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -891,10 +891,6 @@ int m2p_add_override(unsigned long mfn, struct page *page,
WARN_ON(PagePrivate(page));
SetPagePrivate(page);
set_page_private(page, mfn);
- page->index = pfn_to_mfn(pfn);
-
- if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
- return -ENOMEM;

if (kmap_op != NULL) {
if (!PageHighMem(page)) {
@@ -962,7 +958,6 @@ int m2p_remove_override(struct page *page,
WARN_ON(!PagePrivate(page));
ClearPagePrivate(page);

- set_phys_to_machine(pfn, page->index);
if (kmap_op != NULL) {
if (!PageHighMem(page)) {
struct multicall_space mcs;
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index e41c79c..b89aaa2 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -250,6 +250,9 @@ static int find_grant_ptes(pte_t *pte, pgtable_t token,
static int map_grant_pages(struct grant_map *map)
{
int i, err = 0;
+ bool lazy = false;
+ pte_t *pte;
+ unsigned long mfn;

if (!use_ptemod) {
/* Note: it could already be mapped */
@@ -284,8 +287,37 @@ static int map_grant_pages(struct grant_map *map)
}

pr_debug("map %d+%d\n", map->index, map->count);
- err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
- map->pages, map->count);
+ err = gnttab_map_refs(map->map_ops, NULL, map->pages, map->count);
+ if (err)
+ return err;
+
+ if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
+ arch_enter_lazy_mmu_mode();
+ lazy = true;
+ }
+
+ for (i = 0; i < map->count; i++) {
+ /* Do not add to override if the map failed. */
+ if (map->map_ops[i].status)
+ continue;
+
+ if (map->map_ops[i].flags & GNTMAP_contains_pte) {
+ pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map->map_ops[i].host_addr)) +
+ (map->map_ops[i].host_addr & ~PAGE_MASK));
+ mfn = pte_mfn(*pte);
+ } else {
+ mfn = PFN_DOWN(map->map_ops[i].dev_bus_addr);
+ }
+ err = m2p_add_override(mfn,
+ map->pages[i],
+ use_ptemod ? &map->kmap_ops[i] : NULL);
+ if (err)
+ break;
+ }
+
+ if (lazy)
+ arch_leave_lazy_mmu_mode();
+
if (err)
return err;

@@ -304,6 +336,7 @@ static int map_grant_pages(struct grant_map *map)
static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
{
int i, err = 0;
+ bool lazy = false;

if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
int pgno = (map->notify.addr >> PAGE_SHIFT);
@@ -316,8 +349,29 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
}

err = gnttab_unmap_refs(map->unmap_ops + offset,
- use_ptemod ? map->kmap_ops + offset : NULL, map->pages + offset,
- pages);
+ NULL,
+ map->pages + offset,
+ pages);
+ if (err)
+ return err;
+
+ if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
+ arch_enter_lazy_mmu_mode();
+ lazy = true;
+ }
+
+ for (i = 0; i < pages; i++) {
+ err = m2p_remove_override((map->pages + offset)[i],
+ use_ptemod ?
+ &(map->kmap_ops + offset)[i] :
+ NULL);
+ if (err)
+ break;
+ }
+
+ if (lazy)
+ arch_leave_lazy_mmu_mode();
+
if (err)
return err;

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index aa846a4..ad281e4 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -885,7 +885,6 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
struct page **pages, unsigned int count)
{
int i, ret;
- bool lazy = false;
pte_t *pte;
unsigned long mfn;

@@ -904,40 +903,29 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
for (i = 0; i < count; i++) {
if (map_ops[i].status)
continue;
- set_phys_to_machine(map_ops[i].host_addr >> PAGE_SHIFT,
- map_ops[i].dev_bus_addr >> PAGE_SHIFT);
+ if (unlikely(!set_phys_to_machine(map_ops[i].host_addr >> PAGE_SHIFT,
+ map_ops[i].dev_bus_addr >> PAGE_SHIFT)))
+ return -ENOMEM;
}
- return ret;
- }
-
- if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
- arch_enter_lazy_mmu_mode();
- lazy = true;
- }
-
- for (i = 0; i < count; i++) {
- /* Do not add to override if the map failed. */
- if (map_ops[i].status)
- continue;
-
- if (map_ops[i].flags & GNTMAP_contains_pte) {
- pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
- (map_ops[i].host_addr & ~PAGE_MASK));
- mfn = pte_mfn(*pte);
- } else {
- mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
+ } else {
+ for (i = 0; i < count; i++) {
+ if (map_ops[i].status)
+ continue;
+ if (map_ops[i].flags & GNTMAP_contains_pte) {
+ pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) +
+ (map_ops[i].host_addr & ~PAGE_MASK));
+ mfn = pte_mfn(*pte);
+ } else {
+ mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
+ }
+ pages[i]->index = pfn_to_mfn(page_to_pfn(pages[i]));
+ if (unlikely(!set_phys_to_machine(page_to_pfn(pages[i]),
+ FOREIGN_FRAME(mfn))))
+ return -ENOMEM;
}
- ret = m2p_add_override(mfn, pages[i], kmap_ops ?
- &kmap_ops[i] : NULL);
- if (ret)
- goto out;
}

- out:
- if (lazy)
- arch_leave_lazy_mmu_mode();
-
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(gnttab_map_refs);

@@ -946,7 +934,6 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
struct page **pages, unsigned int count)
{
int i, ret;
- bool lazy = false;

ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap_ops, count);
if (ret)
@@ -958,26 +945,14 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
set_phys_to_machine(unmap_ops[i].host_addr >> PAGE_SHIFT,
INVALID_P2M_ENTRY);
}
- return ret;
- }
-
- if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
- arch_enter_lazy_mmu_mode();
- lazy = true;
- }
-
- for (i = 0; i < count; i++) {
- ret = m2p_remove_override(pages[i], kmap_ops ?
- &kmap_ops[i] : NULL);
- if (ret)
- goto out;
+ } else {
+ for (i = 0; i < count; i++) {
+ set_phys_to_machine(page_to_pfn(pages[i]),
+ pages[i]->index);
+ }
}

- out:
- if (lazy)
- arch_leave_lazy_mmu_mode();
-
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(gnttab_unmap_refs);

[Zoltan Kiss]

This patch updates the function prototypes as kmap_ops is no longer needed.

Signed-off-by: Zoltan Kiss <zolta...@citrix.com>
Suggested-by: David Vrabel <david....@citrix.com>

---
drivers/block/xen-blkback/blkback.c | 15 ++++++---------
drivers/xen/gntdev.c | 3 +--
drivers/xen/grant-table.c | 2 --
include/xen/grant_table.h | 2 --
4 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 6620b73..875025f 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -285,8 +285,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,

if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST ||
!rb_next(&persistent_gnt->node)) {
- ret = gnttab_unmap_refs(unmap, NULL, pages,
- segs_to_unmap);
+ ret = gnttab_unmap_refs(unmap, pages, segs_to_unmap);
BUG_ON(ret);
put_free_pages(blkif, pages, segs_to_unmap);
segs_to_unmap = 0;
@@ -321,8 +320,7 @@ static void unmap_purged_grants(struct work_struct *work)
pages[segs_to_unmap] = persistent_gnt->page;

if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
- ret = gnttab_unmap_refs(unmap, NULL, pages,
- segs_to_unmap);
+ ret = gnttab_unmap_refs(unmap, pages, segs_to_unmap);
BUG_ON(ret);
put_free_pages(blkif, pages, segs_to_unmap);
segs_to_unmap = 0;
@@ -330,7 +328,7 @@ static void unmap_purged_grants(struct work_struct *work)
kfree(persistent_gnt);
}
if (segs_to_unmap > 0) {
- ret = gnttab_unmap_refs(unmap, NULL, pages, segs_to_unmap);
+ ret = gnttab_unmap_refs(unmap, pages, segs_to_unmap);
BUG_ON(ret);
put_free_pages(blkif, pages, segs_to_unmap);
}
@@ -670,15 +668,14 @@ static void xen_blkbk_unmap(struct xen_blkif *blkif,
GNTMAP_host_map, pages[i]->handle);
pages[i]->handle = BLKBACK_INVALID_HANDLE;
if (++invcount == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
- ret = gnttab_unmap_refs(unmap, NULL, unmap_pages,
- invcount);
+ ret = gnttab_unmap_refs(unmap, unmap_pages, invcount);
BUG_ON(ret);
put_free_pages(blkif, unmap_pages, invcount);
invcount = 0;
}
}
if (invcount) {
- ret = gnttab_unmap_refs(unmap, NULL, unmap_pages, invcount);
+ ret = gnttab_unmap_refs(unmap, unmap_pages, invcount);
BUG_ON(ret);
put_free_pages(blkif, unmap_pages, invcount);
}
@@ -740,7 +737,7 @@ again:
}

if (segs_to_map) {
- ret = gnttab_map_refs(map, NULL, pages_to_gnt, segs_to_map);
+ ret = gnttab_map_refs(map, pages_to_gnt, segs_to_map);
BUG_ON(ret);
}

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index b89aaa2..3a97342 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -287,7 +287,7 @@ static int map_grant_pages(struct grant_map *map)

}

pr_debug("map %d+%d\n", map->index, map->count);

- err = gnttab_map_refs(map->map_ops, NULL, map->pages, map->count);
+ err = gnttab_map_refs(map->map_ops, map->pages, map->count);
if (err)
return err;

@@ -349,7 +349,6 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)

}

err = gnttab_unmap_refs(map->unmap_ops + offset,

- NULL,
map->pages + offset,
pages);
if (err)
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index ad281e4..1471b5f 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -881,7 +881,6 @@ void gnttab_batch_copy(struct gnttab_copy *batch, unsigned count)
EXPORT_SYMBOL_GPL(gnttab_batch_copy);

int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
- struct gnttab_map_grant_ref *kmap_ops,

struct page **pages, unsigned int count)
{
int i, ret;

@@ -930,7 +929,6 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
EXPORT_SYMBOL_GPL(gnttab_map_refs);

int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct gnttab_map_grant_ref *kmap_ops,

struct page **pages, unsigned int count)
{
int i, ret;

diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 694dcaf..93d363a 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -184,10 +184,8 @@ unsigned int gnttab_max_grant_frames(void);
#define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))

int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
- struct gnttab_map_grant_ref *kmap_ops,

struct page **pages, unsigned int count);

int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct gnttab_map_grant_ref *kunmap_ops,
struct page **pages, unsigned int count);

/* Perform a batch of grant map/copy operations. Retry every batch slot

[Stefano Stabellini]

On Mon, 13 Jan 2014, Zoltan Kiss wrote:
> This patch does the following:
> - move the m2p_override part from grant_[un]map_refs to gntdev, where it is
> needed after mapping operations

As I wrote earlier, I am not against the idea of moving the m2p_override
calls in principle, but I would like to see either the calls being moved
to x86 specific areas or left in grant-table.c.

The reason is simple: from an architectural perspective if we wanted to
introduce an m2p on arm (actual we already have it but at the moment we
are not setting it up using m2p_override calls), we would want to do it
by calls from grant-table.c because unfortunately we need it for things
other than gntdev. In fact for devices that are not behind an IOMMU, we
do need to make sure that DMA operations involving granted pages are
handled correctly (by translating the pfns to mfns and vice versa). This
would have to happen for netback and blkback too.
So if we say that m2p_override functions are arch-independent, then they
need to stay where they are.

On the other hand, we if say that the m2p_override is an x86 specific
hack that is going away, then I don't want it in common code.
If you insist on calling it from common code, then please remove the
m2p_override stubs from arch/arm and ifdef x86 the whole thing in
gntdev.c.

[Zoltan Kiss]

This patch has a fundamental problem here: we change the pfn in
gnttab_map_refs, then fetch it in m2p_override again, but then we have a
different one than we need. This causes Dom0 crash. I will send a new
version to fix that.

Zoli

[Zoltan Kiss]

The grant mapping API does m2p_override unnecessarily: only gntdev needs it,
for blkback and future netback patches it just cause a lock contention, as
those pages never go to userspace. Therefore this series does the following:

- the original functions were renamed to __gnttab_[un]map_refs, with a new
parameter m2p_override
- based on m2p_override either they follow the original behaviour, or just set
the private flag and call set_phys_to_machine
- gnttab_[un]map_refs are now a wrapper to call __gnttab_[un]map_refs with
m2p_override false
- a new function gnttab_[un]map_refs_userspace provides the old behaviour

v2:
- move the storing of the old mfn in page->index to gnttab_map_refs
- move the function header update to a separate patch

v3:
- a new approach to retain old behaviour where it needed
- squash the patches into one

Signed-off-by: Zoltan Kiss <zolta...@citrix.com>
Suggested-by: David Vrabel <david....@citrix.com>
---

drivers/block/xen-blkback/blkback.c | 15 +++----
drivers/xen/gntdev.c | 13 +++---
drivers/xen/grant-table.c | 81 +++++++++++++++++++++++++++++------
include/xen/grant_table.h | 8 +++-
4 files changed, 87 insertions(+), 30 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index e41c79c..e652c0e 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -284,8 +284,10 @@ static int map_grant_pages(struct grant_map *map)

}

pr_debug("map %d+%d\n", map->index, map->count);
- err = gnttab_map_refs(map->map_ops, use_ptemod ? map->kmap_ops : NULL,
- map->pages, map->count);

+ err = gnttab_map_refs_userspace(map->map_ops,
+ use_ptemod ? map->kmap_ops : NULL,
+ map->pages,
+ map->count);
if (err)
return err;

@@ -315,9 +317,10 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
}
}

- err = gnttab_unmap_refs(map->unmap_ops + offset,

- use_ptemod ? map->kmap_ops + offset : NULL, map->pages + offset,
- pages);

+ err = gnttab_unmap_refs_userspace(map->unmap_ops + offset,
+ use_ptemod ? map->kmap_ops + offset : NULL,

+ map->pages + offset,
+ pages);

if (err)
return err;

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c

index aa846a4..87ded60 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -880,15 +880,17 @@ void gnttab_batch_copy(struct gnttab_copy *batch, unsigned count)
}
EXPORT_SYMBOL_GPL(gnttab_batch_copy);

-int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
+int __gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
struct gnttab_map_grant_ref *kmap_ops,
- struct page **pages, unsigned int count)
+ struct page **pages, unsigned int count,
+ bool m2p_override)
{
int i, ret;

bool lazy = false;
pte_t *pte;
unsigned long mfn;

+ BUG_ON(kmap_ops && !m2p_override);
ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map_ops, count);
if (ret)
return ret;
@@ -907,10 +909,12 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
set_phys_to_machine(map_ops[i].host_addr >> PAGE_SHIFT,
map_ops[i].dev_bus_addr >> PAGE_SHIFT);

}
- return ret;
+ return 0;
}

- if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
+ if (m2p_override &&
+ !in_interrupt() &&
+ paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
arch_enter_lazy_mmu_mode();
lazy = true;
}
@@ -927,8 +931,18 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
} else {

mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
}

- ret = m2p_add_override(mfn, pages[i], kmap_ops ?
- &kmap_ops[i] : NULL);

+ if (m2p_override)
+ ret = m2p_add_override(mfn, pages[i], kmap_ops ?
+ &kmap_ops[i] : NULL);
+ else {
+ unsigned long pfn = page_to_pfn(pages[i]);
+ WARN_ON(PagePrivate(pages[i]));
+ SetPagePrivate(pages[i]);
+ set_page_private(pages[i], mfn);
+ pages[i]->index = pfn_to_mfn(pfn);
+ if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
+ return -ENOMEM;
+ }
if (ret)
goto out;
}
@@ -937,17 +951,33 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
if (lazy)
arch_leave_lazy_mmu_mode();

- return ret;
+ return 0;

+}
+
+int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
+ struct page **pages, unsigned int count)
+{
+ return __gnttab_map_refs(map_ops, NULL, pages, count, false);
}
EXPORT_SYMBOL_GPL(gnttab_map_refs);

-int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
+int gnttab_map_refs_userspace(struct gnttab_map_grant_ref *map_ops,
+ struct gnttab_map_grant_ref *kmap_ops,
+ struct page **pages, unsigned int count)
+{
+ return __gnttab_map_refs(map_ops, kmap_ops, pages, count, true);
+}
+EXPORT_SYMBOL_GPL(gnttab_map_refs_userspace);
+
+int __gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
struct gnttab_map_grant_ref *kmap_ops,
- struct page **pages, unsigned int count)
+ struct page **pages, unsigned int count,
+ bool m2p_override)
{
int i, ret;
bool lazy = false;

+ BUG_ON(kmap_ops && !m2p_override);

ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap_ops, count);
if (ret)

return ret;
@@ -958,17 +988,26 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,

set_phys_to_machine(unmap_ops[i].host_addr >> PAGE_SHIFT,
INVALID_P2M_ENTRY);
}
- return ret;

+ return 0;
}

- if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
+ if (m2p_override &&
+ !in_interrupt() &&
+ paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
arch_enter_lazy_mmu_mode();
lazy = true;

}

for (i = 0; i < count; i++) {
- ret = m2p_remove_override(pages[i], kmap_ops ?
- &kmap_ops[i] : NULL);

+ if (m2p_override)
+ ret = m2p_remove_override(pages[i], kmap_ops ?
+ &kmap_ops[i] : NULL);
+ else {
+ unsigned long pfn = page_to_pfn(pages[i]);
+ WARN_ON(!PagePrivate(pages[i]));
+ ClearPagePrivate(pages[i]);
+ set_phys_to_machine(pfn, pages[i]->index);
+ }
if (ret)
goto out;
}
@@ -977,10 +1016,24 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
if (lazy)
arch_leave_lazy_mmu_mode();

- return ret;
+ return 0;

+}
+
+int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *map_ops,
+ struct page **pages, unsigned int count)
+{
+ return __gnttab_unmap_refs(map_ops, NULL, pages, count, false);
}
EXPORT_SYMBOL_GPL(gnttab_unmap_refs);

+int gnttab_unmap_refs_userspace(struct gnttab_unmap_grant_ref *map_ops,
+ struct gnttab_map_grant_ref *kmap_ops,
+ struct page **pages, unsigned int count)
+{
+ return __gnttab_unmap_refs(map_ops, kmap_ops, pages, count, true);
+}
+EXPORT_SYMBOL_GPL(gnttab_unmap_refs_userspace);
+
static unsigned nr_status_frames(unsigned nr_grant_frames)
{
BUG_ON(grefs_per_grant_frame == 0);
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 694dcaf..9a919b1 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -184,11 +184,15 @@ unsigned int gnttab_max_grant_frames(void);

#define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))

int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
- struct gnttab_map_grant_ref *kmap_ops,

struct page **pages, unsigned int count);
+int gnttab_map_refs_userspace(struct gnttab_map_grant_ref *map_ops,
+ struct gnttab_map_grant_ref *kmap_ops,
+ struct page **pages, unsigned int count);

int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct gnttab_map_grant_ref *kunmap_ops,

struct page **pages, unsigned int count);
+int gnttab_unmap_refs_userspace(struct gnttab_unmap_grant_ref *unmap_ops,
+ struct gnttab_map_grant_ref *kunmap_ops,
+ struct page **pages, unsigned int count);

/* Perform a batch of grant map/copy operations. Retry every batch slot

* for which the hypervisor returns GNTST_eagain. This is typically due

[Stefano Stabellini]

What happens if the page is PageHighMem?

This looks like a subset of m2p_add_override, but it is missing some
relevant bits, like the PageHighMem check, or the p2m(m2p(mfn)) == mfn
check. Maybe we can find a way to avoid duplicating the code.
We could split m2p_add_override in two functions or add yet another
parameter to it.

same here: let's try to avoid code duplication

[David Vrabel]

On 21/01/14 12:26, Stefano Stabellini wrote:
> On Mon, 20 Jan 2014, Zoltan Kiss wrote:
>
>> - ret = m2p_add_override(mfn, pages[i], kmap_ops ?
>> - &kmap_ops[i] : NULL);
>> + if (m2p_override)
>> + ret = m2p_add_override(mfn, pages[i], kmap_ops ?
>> + &kmap_ops[i] : NULL);
>> + else {
>> + unsigned long pfn = page_to_pfn(pages[i]);
>> + WARN_ON(PagePrivate(pages[i]));
>> + SetPagePrivate(pages[i]);
>> + set_page_private(pages[i], mfn);
>> + pages[i]->index = pfn_to_mfn(pfn);
>> + if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
>> + return -ENOMEM;
>
> What happens if the page is PageHighMem?
>
> This looks like a subset of m2p_add_override, but it is missing some
> relevant bits, like the PageHighMem check, or the p2m(m2p(mfn)) == mfn
> check. Maybe we can find a way to avoid duplicating the code.
> We could split m2p_add_override in two functions or add yet another
> parameter to it.

The PageHighMem() check isn't relevant as we're not mapping anything
here. Also, a page for a kernel grant mapping only cannot be highmem.

The check for a local mfn and the additional set_phys_to_machine() is
only necessary if something tries an mfn_to_pfn() on the local mfn. We
can only omit adding an m2p override if we know thing will try
mfn_to_pfn(), therefore the check and set_phys_to_machine() is unnecessary.

David

[Stefano Stabellini]

OK, you convinced me that the two checks are superfluous for this case.

Can we still avoid the code duplication by removing the corresponding
code from m2p_add_override and m2p_remove_override and doing the
set_page_private thing uniquely in grant-table.c?

[Zoltan Kiss]

Yes, I moved these parts out from the m2p* funcions to the gntmap
functions. One change is that now we pass pfn/mfn to m2p* functions as
they are changing right before the call. Also, to avoid racing I clear
the page->private value before calling m2p_remove_override. I'll send in
the patch soon.

Zoli

[Zoltan Kiss]

The grant mapping API does m2p_override unnecessarily: only gntdev needs it,
for blkback and future netback patches it just cause a lock contention, as
those pages never go to userspace. Therefore this series does the following:
- the original functions were renamed to __gnttab_[un]map_refs, with a new
parameter m2p_override
- based on m2p_override either they follow the original behaviour, or just set
the private flag and call set_phys_to_machine
- gnttab_[un]map_refs are now a wrapper to call __gnttab_[un]map_refs with
m2p_override false
- a new function gnttab_[un]map_refs_userspace provides the old behaviour

v2:
- move the storing of the old mfn in page->index to gnttab_map_refs
- move the function header update to a separate patch

v3:
- a new approach to retain old behaviour where it needed
- squash the patches into one

v4:
- move out the common bits from m2p* functions, and pass pfn/mfn as parameter
- clear page->private before doing anything with the page, so m2p_find_override
won't race with this

Signed-off-by: Zoltan Kiss <zolta...@citrix.com>
Suggested-by: David Vrabel <david....@citrix.com>
---

arch/x86/include/asm/xen/page.h | 12 +++--
arch/x86/xen/p2m.c | 25 ++--------
drivers/block/xen-blkback/blkback.c | 15 +++---
drivers/xen/gntdev.c | 13 +++--
drivers/xen/grant-table.c | 90 +++++++++++++++++++++++++++++------
include/xen/grant_table.h | 8 +++-
6 files changed, 107 insertions(+), 56 deletions(-)

diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index b913915..68a1438 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -49,10 +49,14 @@ extern bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn);
extern unsigned long set_phys_range_identity(unsigned long pfn_s,
unsigned long pfn_e);

-extern int m2p_add_override(unsigned long mfn, struct page *page,
- struct gnttab_map_grant_ref *kmap_op);
+extern int m2p_add_override(unsigned long mfn,
+ struct page *page,
+ struct gnttab_map_grant_ref *kmap_op,
+ unsigned long pfn);
extern int m2p_remove_override(struct page *page,
- struct gnttab_map_grant_ref *kmap_op);
+ struct gnttab_map_grant_ref *kmap_op,
+ unsigned long pfn,
+ unsigned long mfn);
extern struct page *m2p_find_override(unsigned long mfn);
extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long pfn);

@@ -121,7 +125,7 @@ static inline unsigned long mfn_to_pfn(unsigned long mfn)
pfn = m2p_find_override_pfn(mfn, ~0);
}

- /*
+ /*
* pfn is ~0 if there are no entries in the m2p for mfn or if the
* entry doesn't map back to the mfn and m2p_override doesn't have a
* valid entry for it.
diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 2ae8699..0060178 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -872,15 +872,13 @@ static unsigned long mfn_hash(unsigned long mfn)

/* Add an MFN override for a particular page */

int m2p_add_override(unsigned long mfn, struct page *page,

- struct gnttab_map_grant_ref *kmap_op)
+ struct gnttab_map_grant_ref *kmap_op, unsigned long pfn)
{
unsigned long flags;
- unsigned long pfn;
unsigned long uninitialized_var(address);
unsigned level;
pte_t *ptep = NULL;

- pfn = page_to_pfn(page);
if (!PageHighMem(page)) {
address = (unsigned long)__va(pfn << PAGE_SHIFT);
ptep = lookup_address(address, &level);
@@ -888,13 +886,6 @@ int m2p_add_override(unsigned long mfn, struct page *page,
"m2p_add_override: pfn %lx not mapped", pfn))
return -EINVAL;
}
- WARN_ON(PagePrivate(page));
- SetPagePrivate(page);
- set_page_private(page, mfn);

- page->index = pfn_to_mfn(pfn);
-
- if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
- return -ENOMEM;

if (kmap_op != NULL) {
if (!PageHighMem(page)) {

@@ -933,20 +924,15 @@ int m2p_add_override(unsigned long mfn, struct page *page,
}
EXPORT_SYMBOL_GPL(m2p_add_override);
int m2p_remove_override(struct page *page,
- struct gnttab_map_grant_ref *kmap_op)
+ struct gnttab_map_grant_ref *kmap_op,
+ unsigned long pfn,
+ unsigned long mfn)
{
unsigned long flags;
- unsigned long mfn;
- unsigned long pfn;
unsigned long uninitialized_var(address);
unsigned level;
pte_t *ptep = NULL;

- pfn = page_to_pfn(page);
- mfn = get_phys_to_machine(pfn);
- if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT))
- return -EINVAL;
-
if (!PageHighMem(page)) {
address = (unsigned long)__va(pfn << PAGE_SHIFT);
ptep = lookup_address(address, &level);
@@ -959,10 +945,7 @@ int m2p_remove_override(struct page *page,
spin_lock_irqsave(&m2p_override_lock, flags);
list_del(&page->lru);
spin_unlock_irqrestore(&m2p_override_lock, flags);
- WARN_ON(!PagePrivate(page));
- ClearPagePrivate(page);

- set_phys_to_machine(pfn, page->index);
if (kmap_op != NULL) {
if (!PageHighMem(page)) {
struct multicall_space mcs;

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index aa846a4..1f97fa0 100644

--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -880,15 +880,17 @@ void gnttab_batch_copy(struct gnttab_copy *batch, unsigned count)
}
EXPORT_SYMBOL_GPL(gnttab_batch_copy);

-int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
+int __gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
struct gnttab_map_grant_ref *kmap_ops,
- struct page **pages, unsigned int count)
+ struct page **pages, unsigned int count,
+ bool m2p_override)
{
int i, ret;
bool lazy = false;
pte_t *pte;

- unsigned long mfn;
+ unsigned long mfn, pfn;

+ BUG_ON(kmap_ops && !m2p_override);
ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, map_ops, count);
if (ret)
return ret;
@@ -907,10 +909,12 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
set_phys_to_machine(map_ops[i].host_addr >> PAGE_SHIFT,
map_ops[i].dev_bus_addr >> PAGE_SHIFT);
}
- return ret;
+ return 0;
}

- if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
+ if (m2p_override &&
+ !in_interrupt() &&
+ paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
arch_enter_lazy_mmu_mode();
lazy = true;
}
@@ -927,8 +931,18 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
} else {
mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
}

- ret = m2p_add_override(mfn, pages[i], kmap_ops ?
- &kmap_ops[i] : NULL);

+ pfn = page_to_pfn(pages[i]);

+
+ WARN_ON(PagePrivate(pages[i]));
+ SetPagePrivate(pages[i]);
+ set_page_private(pages[i], mfn);
+
+ pages[i]->index = pfn_to_mfn(pfn);
+ if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
+ return -ENOMEM;

+ if (m2p_override)
+ ret = m2p_add_override(mfn, pages[i], kmap_ops ?

+ &kmap_ops[i] : NULL, pfn);
if (ret)
goto out;
}
@@ -937,17 +951,34 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,

+ unsigned long pfn, mfn;

+ BUG_ON(kmap_ops && !m2p_override);

ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap_ops, count);
if (ret)
return ret;

@@ -958,17 +989,32 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,

set_phys_to_machine(unmap_ops[i].host_addr >> PAGE_SHIFT,
INVALID_P2M_ENTRY);
}
- return ret;
+ return 0;
}

- if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
+ if (m2p_override &&
+ !in_interrupt() &&
+ paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
arch_enter_lazy_mmu_mode();
lazy = true;
}

for (i = 0; i < count; i++) {

- ret = m2p_remove_override(pages[i], kmap_ops ?
- &kmap_ops[i] : NULL);
+ pfn = page_to_pfn(pages[i]);
+ mfn = get_phys_to_machine(pfn);
+ if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT))
+ return -EINVAL;
+
+ set_page_private(pages[i], INVALID_P2M_ENTRY);

+ WARN_ON(!PagePrivate(pages[i]));
+ ClearPagePrivate(pages[i]);
+ set_phys_to_machine(pfn, pages[i]->index);

+ if (m2p_override)
+ ret = m2p_remove_override(pages[i],
+ kmap_ops ?
+ &kmap_ops[i] : NULL,
+ pfn,
+ mfn);
if (ret)
goto out;
}
@@ -977,10 +1023,24 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,

if (lazy)
arch_leave_lazy_mmu_mode();

- return ret;
+ return 0;
+}
+
+int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *map_ops,
+ struct page **pages, unsigned int count)
+{
+ return __gnttab_unmap_refs(map_ops, NULL, pages, count, false);
}
EXPORT_SYMBOL_GPL(gnttab_unmap_refs);

+int gnttab_unmap_refs_userspace(struct gnttab_unmap_grant_ref *map_ops,
+ struct gnttab_map_grant_ref *kmap_ops,
+ struct page **pages, unsigned int count)
+{
+ return __gnttab_unmap_refs(map_ops, kmap_ops, pages, count, true);
+}
+EXPORT_SYMBOL_GPL(gnttab_unmap_refs_userspace);
+
static unsigned nr_status_frames(unsigned nr_grant_frames)
{
BUG_ON(grefs_per_grant_frame == 0);

diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 694dcaf..9a919b1 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -184,11 +184,15 @@ unsigned int gnttab_max_grant_frames(void);
#define gnttab_map_vaddr(map) ((void *)(map.host_virt_addr))

int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
- struct gnttab_map_grant_ref *kmap_ops,

struct page **pages, unsigned int count);

+int gnttab_map_refs_userspace(struct gnttab_map_grant_ref *map_ops,
+ struct gnttab_map_grant_ref *kmap_ops,

+ struct page **pages, unsigned int count);
int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct gnttab_map_grant_ref *kunmap_ops,
struct page **pages, unsigned int count);
+int gnttab_unmap_refs_userspace(struct gnttab_unmap_grant_ref *unmap_ops,
+ struct gnttab_map_grant_ref *kunmap_ops,
+ struct page **pages, unsigned int count);

/* Perform a batch of grant map/copy operations. Retry every batch slot
* for which the hypervisor returns GNTST_eagain. This is typically due

[David Vrabel]

On 21/01/14 20:22, Zoltan Kiss wrote:
> The grant mapping API does m2p_override unnecessarily: only gntdev needs it,
> for blkback and future netback patches it just cause a lock contention, as
> those pages never go to userspace. Therefore this series does the following:
> - the original functions were renamed to __gnttab_[un]map_refs, with a new
> parameter m2p_override
> - based on m2p_override either they follow the original behaviour, or just set
> the private flag and call set_phys_to_machine
> - gnttab_[un]map_refs are now a wrapper to call __gnttab_[un]map_refs with
> m2p_override false
> - a new function gnttab_[un]map_refs_userspace provides the old behaviour
>
> v2:
> - move the storing of the old mfn in page->index to gnttab_map_refs
> - move the function header update to a separate patch
>
> v3:
> - a new approach to retain old behaviour where it needed
> - squash the patches into one
>
> v4:
> - move out the common bits from m2p* functions, and pass pfn/mfn as parameter
> - clear page->private before doing anything with the page, so m2p_find_override
> won't race with this

I would prune this version information out of the commit message.

> Signed-off-by: Zoltan Kiss <zolta...@citrix.com>
> Suggested-by: David Vrabel <david....@citrix.com>

Reviewed-by: David Vrabel <david....@citrix.com>

But I think this needs Stefano's ack.

It would be useful to get this into 3.14 if possible.

David

[Stefano Stabellini]

Spurious change?

> * pfn is ~0 if there are no entries in the m2p for mfn or if the
> * entry doesn't map back to the mfn and m2p_override doesn't have a
> * valid entry for it.
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 2ae8699..0060178 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -872,15 +872,13 @@ static unsigned long mfn_hash(unsigned long mfn)
>
> /* Add an MFN override for a particular page */
> int m2p_add_override(unsigned long mfn, struct page *page,
> - struct gnttab_map_grant_ref *kmap_op)
> + struct gnttab_map_grant_ref *kmap_op, unsigned long pfn)

Do we really need to add another additional parameter to
m2p_add_override?
I would just let m2p_add_override and m2p_remove_override call
page_to_pfn again. It is not that expensive.

Same here

goto out

> + if (m2p_override)
> + ret = m2p_add_override(mfn, pages[i], kmap_ops ?
> + &kmap_ops[i] : NULL, pfn);
> if (ret)
> goto out;
> }
> @@ -937,17 +951,34 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
> if (lazy)
> arch_leave_lazy_mmu_mode();
>
> - return ret;
> + return 0;

We are loosing the error code possibly returned by m2p_add_override and
the previous check.

goto out

> + set_page_private(pages[i], INVALID_P2M_ENTRY);
> + WARN_ON(!PagePrivate(pages[i]));
> + ClearPagePrivate(pages[i]);
> + set_phys_to_machine(pfn, pages[i]->index);
> + if (m2p_override)
> + ret = m2p_remove_override(pages[i],
> + kmap_ops ?
> + &kmap_ops[i] : NULL,
> + pfn,
> + mfn);
> if (ret)
> goto out;
> }
> @@ -977,10 +1023,24 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
> if (lazy)
> arch_leave_lazy_mmu_mode();
>
> - return ret;
> + return 0;

We are loosing the error code possibly returned by m2p_remove_override
and the two previous checks.

> _______________________________________________
> Xen-devel mailing list
> Xen-...@lists.xen.org
> http://lists.xen.org/xen-devel

[Zoltan Kiss]

On 22/01/14 16:39, Stefano Stabellini wrote:
> On Tue, 21 Jan 2014, Zoltan Kiss wrote:
>> @@ -121,7 +125,7 @@ static inline unsigned long mfn_to_pfn(unsigned long mfn)
>> pfn = m2p_find_override_pfn(mfn, ~0);
>> }
>>
>> - /*
>> + /*
>
> Spurious change?

It removes a stray space from the original code. Not necessary, but if
it's there, I think we can keep it.

>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>> index 2ae8699..0060178 100644
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -872,15 +872,13 @@ static unsigned long mfn_hash(unsigned long mfn)
>>
>> /* Add an MFN override for a particular page */
>> int m2p_add_override(unsigned long mfn, struct page *page,
>> - struct gnttab_map_grant_ref *kmap_op)
>> + struct gnttab_map_grant_ref *kmap_op, unsigned long pfn)
>
> Do we really need to add another additional parameter to
> m2p_add_override?
> I would just let m2p_add_override and m2p_remove_override call
> page_to_pfn again. It is not that expensive.

Yes, because that page_to_pfn can return something different. That's why
the v2 patches failed.

>> @@ -933,20 +924,15 @@ int m2p_add_override(unsigned long mfn, struct page *page,
>> }
>> EXPORT_SYMBOL_GPL(m2p_add_override);
>> int m2p_remove_override(struct page *page,
>> - struct gnttab_map_grant_ref *kmap_op)
>> + struct gnttab_map_grant_ref *kmap_op,
>> + unsigned long pfn,
>> + unsigned long mfn)
>
> Same here

Same as above.

>> @@ -927,8 +931,18 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> } else {
>> mfn = PFN_DOWN(map_ops[i].dev_bus_addr);
>> }
>> - ret = m2p_add_override(mfn, pages[i], kmap_ops ?
>> - &kmap_ops[i] : NULL);
>> + pfn = page_to_pfn(pages[i]);
>> +
>> + WARN_ON(PagePrivate(pages[i]));
>> + SetPagePrivate(pages[i]);
>> + set_page_private(pages[i], mfn);
>> +
>> + pages[i]->index = pfn_to_mfn(pfn);
>> + if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
>> + return -ENOMEM;
>
> goto out

And ret = -ENOMEM

>
>
>> + if (m2p_override)
>> + ret = m2p_add_override(mfn, pages[i], kmap_ops ?
>> + &kmap_ops[i] : NULL, pfn);
>> if (ret)
>> goto out;
>> }
>> @@ -937,17 +951,34 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>> if (lazy)
>> arch_leave_lazy_mmu_mode();
>>
>> - return ret;
>> + return 0;
>
> We are loosing the error code possibly returned by m2p_add_override and
> the previous check.

I'll fix that. Also in unmap.

return ret;
>> @@ -958,17 +989,32 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>> set_phys_to_machine(unmap_ops[i].host_addr >> PAGE_SHIFT,
>> INVALID_P2M_ENTRY);
>> }
>> - return ret;
>> + return 0;
>> }
>>
>> - if (!in_interrupt() && paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>> + if (m2p_override &&
>> + !in_interrupt() &&
>> + paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>> arch_enter_lazy_mmu_mode();
>> lazy = true;
>> }
>>
>> for (i = 0; i < count; i++) {
>> - ret = m2p_remove_override(pages[i], kmap_ops ?
>> - &kmap_ops[i] : NULL);
>> + pfn = page_to_pfn(pages[i]);
>> + mfn = get_phys_to_machine(pfn);
>> + if (mfn == INVALID_P2M_ENTRY || !(mfn & FOREIGN_FRAME_BIT))
>> + return -EINVAL;
>
> goto out

And ret = -EINVAL

The above are the the result of the fact that I've based this originally
on 3.10, where the out label haven't existed. I'll send the next version
when the tests pass.

Zoli

[Stefano Stabellini]

On Wed, 22 Jan 2014, Zoltan Kiss wrote:
> On 22/01/14 16:39, Stefano Stabellini wrote:
> > On Tue, 21 Jan 2014, Zoltan Kiss wrote:
> > > @@ -121,7 +125,7 @@ static inline unsigned long mfn_to_pfn(unsigned long
> > > mfn)
> > > pfn = m2p_find_override_pfn(mfn, ~0);
> > > }
> > >
> > > - /*
> > > + /*
> >
> > Spurious change?
> It removes a stray space from the original code. Not necessary, but if it's
> there, I think we can keep it.

Usually cosmetic changes are done in a separate patch, or at the very
least they are mentioned in the commit message.

> > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> > > index 2ae8699..0060178 100644
> > > --- a/arch/x86/xen/p2m.c
> > > +++ b/arch/x86/xen/p2m.c
> > > @@ -872,15 +872,13 @@ static unsigned long mfn_hash(unsigned long mfn)
> > >
> > > /* Add an MFN override for a particular page */
> > > int m2p_add_override(unsigned long mfn, struct page *page,
> > > - struct gnttab_map_grant_ref *kmap_op)
> > > + struct gnttab_map_grant_ref *kmap_op, unsigned long pfn)
> >
> > Do we really need to add another additional parameter to
> > m2p_add_override?
> > I would just let m2p_add_override and m2p_remove_override call
> > page_to_pfn again. It is not that expensive.
> Yes, because that page_to_pfn can return something different. That's why the
> v2 patches failed.

I am really curious: how can page_to_pfn return something different?
I don't think is supposed to happen.

[Zoltan Kiss]

On 22/01/14 18:50, Stefano Stabellini wrote:
> On Wed, 22 Jan 2014, Zoltan Kiss wrote:
>> On 22/01/14 16:39, Stefano Stabellini wrote:
>>> On Tue, 21 Jan 2014, Zoltan Kiss wrote:
>>>> @@ -121,7 +125,7 @@ static inline unsigned long mfn_to_pfn(unsigned long
>>>> mfn)
>>>> pfn = m2p_find_override_pfn(mfn, ~0);
>>>> }
>>>>
>>>> - /*
>>>> + /*
>>>
>>> Spurious change?
>> It removes a stray space from the original code. Not necessary, but if it's
>> there, I think we can keep it.
>
> Usually cosmetic changes are done in a separate patch, or at the very
> least they are mentioned in the commit message.

Ok, I'll mention it.

>
>
>>>> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
>>>> index 2ae8699..0060178 100644
>>>> --- a/arch/x86/xen/p2m.c
>>>> +++ b/arch/x86/xen/p2m.c
>>>> @@ -872,15 +872,13 @@ static unsigned long mfn_hash(unsigned long mfn)
>>>>
>>>> /* Add an MFN override for a particular page */
>>>> int m2p_add_override(unsigned long mfn, struct page *page,
>>>> - struct gnttab_map_grant_ref *kmap_op)
>>>> + struct gnttab_map_grant_ref *kmap_op, unsigned long pfn)
>>>
>>> Do we really need to add another additional parameter to
>>> m2p_add_override?
>>> I would just let m2p_add_override and m2p_remove_override call
>>> page_to_pfn again. It is not that expensive.
>> Yes, because that page_to_pfn can return something different. That's why the
>> v2 patches failed.
>
> I am really curious: how can page_to_pfn return something different?
> I don't think is supposed to happen.

You call set_phys_to_machine before calling m2p* functions.

Zoli

[Konrad Rzeszutek Wilk]

Zoltan Kiss <zolta...@citrix.com> wrote:
>The grant mapping API does m2p_override unnecessarily: only gntdev
>needs it,
>for blkback and future netback patches it just cause a lock contention,
>as
>those pages never go to userspace. Therefore this series does the
>following:
>- the original functions were renamed to __gnttab_[un]map_refs, with a
>new
> parameter m2p_override
>- based on m2p_override either they follow the original behaviour, or
>just set
> the private flag and call set_phys_to_machine
>- gnttab_[un]map_refs are now a wrapper to call __gnttab_[un]map_refs
>with
> m2p_override false
>- a new function gnttab_[un]map_refs_userspace provides the old
>behaviour

You don't say anything about the 'return ret' changed to 'return 0'.

Any particular reason for that?

Thanks

>_______________________________________________
>Xen-devel mailing list
>Xen-...@lists.xen.org
>http://lists.xen.org/xen-devel

[Zoltan Kiss]

On 20/01/14 21:22, Konrad Rzeszutek Wilk wrote:
> Zoltan Kiss <zolta...@citrix.com> wrote:
>> The grant mapping API does m2p_override unnecessarily: only gntdev
>> needs it,
>> for blkback and future netback patches it just cause a lock contention,
>> as
>> those pages never go to userspace. Therefore this series does the
>> following:
>> - the original functions were renamed to __gnttab_[un]map_refs, with a
>> new
>> parameter m2p_override
>> - based on m2p_override either they follow the original behaviour, or
>> just set
>> the private flag and call set_phys_to_machine
>> - gnttab_[un]map_refs are now a wrapper to call __gnttab_[un]map_refs
>> with
>> m2p_override false
>> - a new function gnttab_[un]map_refs_userspace provides the old
>> behaviour
>
> You don't say anything about the 'return ret' changed to 'return 0'.
>
> Any particular reason for that?

That's the only possible return value there, so it just makes it more
obvious. I'll add a description about that.

Zoli

[Stefano Stabellini]

On Wed, 22 Jan 2014, Zoltan Kiss wrote:
> > > > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> > > > > index 2ae8699..0060178 100644
> > > > > --- a/arch/x86/xen/p2m.c
> > > > > +++ b/arch/x86/xen/p2m.c
> > > > > @@ -872,15 +872,13 @@ static unsigned long mfn_hash(unsigned long mfn)
> > > > >
> > > > > /* Add an MFN override for a particular page */
> > > > > int m2p_add_override(unsigned long mfn, struct page *page,
> > > > > - struct gnttab_map_grant_ref *kmap_op)
> > > > > + struct gnttab_map_grant_ref *kmap_op, unsigned long
> > > > > pfn)
> > > >
> > > > Do we really need to add another additional parameter to
> > > > m2p_add_override?
> > > > I would just let m2p_add_override and m2p_remove_override call
> > > > page_to_pfn again. It is not that expensive.
> > > Yes, because that page_to_pfn can return something different. That's why
> > > the
> > > v2 patches failed.
> >
> > I am really curious: how can page_to_pfn return something different?
> > I don't think is supposed to happen.
> You call set_phys_to_machine before calling m2p* functions.

set_phys_to_machine changes the physical to machine mapping, that would
be the mfn corresponding to a given pfn. It shouldn't affect the output
of page_to_pfn that returns the pfn corresponding to a given struct
page. The calculation of which is based on address offsets and should be
static and unaffected by things like set_phys_to_machine.

[Zoltan Kiss]

Indeed, my mistake. The mfn is the only thing which changes, it still
has to be passed to m2p_remove_override. I'll send in a next version

Zoli

[Stefano Stabellini]

Passing the mfn to m2p_remove_override is OK.