2012-10-11 22:01:22

by Mukesh Rathor

[permalink] [raw]
Subject: [PATCH V2 6/7]: PVH: balloon and grant changes

PVH: balloon and grant changes. For balloon changes we skip setting of
local p2m as it's updated in xen. For grant, the shared grant frame is
the pfn and not mfn, hence its mapped via the same code path as HVM

Signed-off-by: Mukesh R <[email protected]>
---
drivers/xen/balloon.c | 18 +++++++++++-------
drivers/xen/gntdev.c | 3 ++-
drivers/xen/grant-table.c | 25 +++++++++++++++++++++----
3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 31ab82f..9b895ad 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -358,10 +358,13 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
phys_to_machine_mapping_valid(pfn));

- set_phys_to_machine(pfn, frame_list[i]);
+ if (!xen_feature(XENFEAT_auto_translated_physmap))
+ set_phys_to_machine(pfn, frame_list[i]);

/* Link back into the page tables if not highmem. */
- if (xen_pv_domain() && !PageHighMem(page)) {
+ if (xen_pv_domain() && !PageHighMem(page) &&
+ !xen_feature(XENFEAT_auto_translated_physmap)) {
+
int ret;
ret = HYPERVISOR_update_va_mapping(
(unsigned long)__va(pfn << PAGE_SHIFT),
@@ -418,12 +421,13 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
scrub_page(page);

if (xen_pv_domain() && !PageHighMem(page)) {
- ret = HYPERVISOR_update_va_mapping(
- (unsigned long)__va(pfn << PAGE_SHIFT),
- __pte_ma(0), 0);
- BUG_ON(ret);
+ if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+ ret = HYPERVISOR_update_va_mapping(
+ (unsigned long)__va(pfn << PAGE_SHIFT),
+ __pte_ma(0), 0);
+ BUG_ON(ret);
+ }
}
-
}

/* Ensure that ballooned highmem pages don't have kmaps. */
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 5df9fd8..36ec380 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -803,7 +803,8 @@ static int __init gntdev_init(void)
if (!xen_domain())
return -ENODEV;

- use_ptemod = xen_pv_domain();
+ use_ptemod = xen_pv_domain() &&
+ !xen_feature(XENFEAT_auto_translated_physmap);

err = misc_register(&gntdev_miscdev);
if (err != 0) {
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index f37faf6..feaebeb 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -976,14 +976,19 @@ static void gnttab_unmap_frames_v2(void)
static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
{
struct gnttab_setup_table setup;
- unsigned long *frames;
+ unsigned long *frames, start_gpfn;
unsigned int nr_gframes = end_idx + 1;
int rc;

- if (xen_hvm_domain()) {
+ if (xen_hvm_domain() || xen_feature(XENFEAT_auto_translated_physmap)) {
struct xen_add_to_physmap xatp;
unsigned int i = end_idx;
rc = 0;
+
+ if (xen_hvm_domain())
+ start_gpfn = xen_hvm_resume_frames >> PAGE_SHIFT;
+ else
+ start_gpfn = virt_to_pfn(gnttab_shared.addr);
/*
* Loop backwards, so that the first hypercall has the largest
* index, ensuring that the table will grow only once.
@@ -992,7 +997,7 @@ static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
xatp.domid = DOMID_SELF;
xatp.idx = i;
xatp.space = XENMAPSPACE_grant_table;
- xatp.gpfn = (xen_hvm_resume_frames >> PAGE_SHIFT) + i;
+ xatp.gpfn = start_gpfn + i;
rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
if (rc != 0) {
printk(KERN_WARNING
@@ -1055,7 +1060,7 @@ static void gnttab_request_version(void)
int rc;
struct gnttab_set_version gsv;

- if (xen_hvm_domain())
+ if (xen_hvm_domain() || xen_feature(XENFEAT_auto_translated_physmap))
gsv.version = 1;
else
gsv.version = 2;
@@ -1083,12 +1088,24 @@ static void gnttab_request_version(void)
int gnttab_resume(void)
{
unsigned int max_nr_gframes;
+ char *kmsg = "Failed to kmalloc pages for pv in hvm grant frames\n";

gnttab_request_version();
max_nr_gframes = gnttab_max_grant_frames();
if (max_nr_gframes < nr_grant_frames)
return -ENOSYS;

+ /* PVH note: xen will free existing kmalloc'd mfn in
+ * XENMEM_add_to_physmap */
+ if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap) &&
+ !gnttab_shared.addr) {
+ gnttab_shared.addr =
+ kmalloc(max_nr_gframes * PAGE_SIZE, GFP_KERNEL);
+ if (!gnttab_shared.addr) {
+ pr_warn("%s", kmsg);
+ return -ENOMEM;
+ }
+ }
if (xen_pv_domain())
return gnttab_map(0, nr_grant_frames - 1);

--
1.7.2.3


2012-10-12 09:07:01

by Ian Campbell

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH V2 6/7]: PVH: balloon and grant changes

On Thu, 2012-10-11 at 23:01 +0100, Mukesh Rathor wrote:
> PVH: balloon and grant changes. For balloon changes we skip setting of
> local p2m as it's updated in xen. For grant, the shared grant frame is
> the pfn and not mfn, hence its mapped via the same code path as HVM
>
> Signed-off-by: Mukesh R <[email protected]>
> ---
> drivers/xen/balloon.c | 18 +++++++++++-------
> drivers/xen/gntdev.c | 3 ++-
> drivers/xen/grant-table.c | 25 +++++++++++++++++++++----
> 3 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 31ab82f..9b895ad 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -358,10 +358,13 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
> BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
> phys_to_machine_mapping_valid(pfn));
>
> - set_phys_to_machine(pfn, frame_list[i]);
> + if (!xen_feature(XENFEAT_auto_translated_physmap))
> + set_phys_to_machine(pfn, frame_list[i]);

set_phys_to_machine is a NOP if XENFEAT_auto_translated_physmap but it
includes some useful sanity checks (BUG_ON(pfn != mfn && mfn !=
INVALID_P2M_ENTRY)) so it would be useful to keep calling it I think.

>
> /* Link back into the page tables if not highmem. */
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pv_domain() && !PageHighMem(page) &&
> + !xen_feature(XENFEAT_auto_translated_physmap)) {
> +
> int ret;
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> @@ -418,12 +421,13 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> scrub_page(page);
>
> if (xen_pv_domain() && !PageHighMem(page)) {
> - ret = HYPERVISOR_update_va_mapping(
> - (unsigned long)__va(pfn << PAGE_SHIFT),
> - __pte_ma(0), 0);
> - BUG_ON(ret);
> + if (!xen_feature(XENFEAT_auto_translated_physmap)) {

You can combine this two ifs into one (like you did above).

> + ret = HYPERVISOR_update_va_mapping(
> + (unsigned long)__va(pfn << PAGE_SHIFT),
> + __pte_ma(0), 0);
> + BUG_ON(ret);
> + }
> }
> -
> }
>
> /* Ensure that ballooned highmem pages don't have kmaps. */
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 5df9fd8..36ec380 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -803,7 +803,8 @@ static int __init gntdev_init(void)
> if (!xen_domain())
> return -ENODEV;
>
> - use_ptemod = xen_pv_domain();
> + use_ptemod = xen_pv_domain() &&
> + !xen_feature(XENFEAT_auto_translated_physmap);
>
> err = misc_register(&gntdev_miscdev);
> if (err != 0) {
> @@ -1055,7 +1060,7 @@ static void gnttab_request_version(void)
> int rc;
> struct gnttab_set_version gsv;
>
> - if (xen_hvm_domain())
> + if (xen_hvm_domain() || xen_feature(XENFEAT_auto_translated_physmap))

This is just a case of not yet implemented rather than a fundamental
problem with v2 grant tables, correct? (i.e. future work)

> gsv.version = 1;
> else
> gsv.version = 2;
> @@ -1083,12 +1088,24 @@ static void gnttab_request_version(void)
> int gnttab_resume(void)
> {
> unsigned int max_nr_gframes;
> + char *kmsg = "Failed to kmalloc pages for pv in hvm grant frames\n";
>
> gnttab_request_version();
> max_nr_gframes = gnttab_max_grant_frames();
> if (max_nr_gframes < nr_grant_frames)
> return -ENOSYS;
>
> + /* PVH note: xen will free existing kmalloc'd mfn in
> + * XENMEM_add_to_physmap */

It doesn't leak it?

We should consider using xenballoon_alloc_pages here.

> + if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap) &&
> + !gnttab_shared.addr) {
> + gnttab_shared.addr =
> + kmalloc(max_nr_gframes * PAGE_SIZE, GFP_KERNEL);
> + if (!gnttab_shared.addr) {
> + pr_warn("%s", kmsg);
> + return -ENOMEM;
> + }
> + }
> if (xen_pv_domain())
> return gnttab_map(0, nr_grant_frames - 1);
>

2012-10-17 00:09:09

by Mukesh Rathor

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH V2 6/7]: PVH: balloon and grant changes

On Fri, 12 Oct 2012 10:06:57 +0100
Ian Campbell <[email protected]> wrote:

> On Thu, 2012-10-11 at 23:01 +0100, Mukesh Rathor wrote:
> > PVH: balloon and grant changes. For balloon changes we skip setting
> > of local p2m as it's updated in xen. For grant, the shared grant
> > frame is the pfn and not mfn, hence its mapped via the same code
> > path as HVM
> >
> > Signed-off-by: Mukesh R <[email protected]>
> > ---
> > drivers/xen/balloon.c | 18 +++++++++++-------
> > drivers/xen/gntdev.c | 3 ++-
> > drivers/xen/grant-table.c | 25 +++++++++++++++++++++----
> > 3 files changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > index 31ab82f..9b895ad 100644
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -358,10 +358,13 @@ static enum bp_state
> > increase_reservation(unsigned long nr_pages)
> > BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
> > phys_to_machine_mapping_valid(pfn));
> > - set_phys_to_machine(pfn, frame_list[i]);
> > + if (!xen_feature(XENFEAT_auto_translated_physmap))
> > + set_phys_to_machine(pfn, frame_list[i]);
>
> set_phys_to_machine is a NOP if XENFEAT_auto_translated_physmap but it
> includes some useful sanity checks (BUG_ON(pfn != mfn && mfn !=
> INVALID_P2M_ENTRY)) so it would be useful to keep calling it I think.

ok, fine, done.

> > + !xen_feature(XENFEAT_auto_translated_physmap);
> >
> > err = misc_register(&gntdev_miscdev);
> > if (err != 0) {
> > @@ -1055,7 +1060,7 @@ static void gnttab_request_version(void)
> > int rc;
> > struct gnttab_set_version gsv;
> >
> > - if (xen_hvm_domain())
> > + if (xen_hvm_domain() ||
> > xen_feature(XENFEAT_auto_translated_physmap))
>
> This is just a case of not yet implemented rather than a fundamental
> problem with v2 grant tables, correct? (i.e. future work)

Right.

> > gsv.version = 1;
> > else
> > gsv.version = 2;
> > @@ -1083,12 +1088,24 @@ static void gnttab_request_version(void)
> > int gnttab_resume(void)
> > {
> > unsigned int max_nr_gframes;
> > + char *kmsg = "Failed to kmalloc pages for pv in hvm grant
> > frames\n";
> > gnttab_request_version();
> > max_nr_gframes = gnttab_max_grant_frames();
> > if (max_nr_gframes < nr_grant_frames)
> > return -ENOSYS;
> >
> > + /* PVH note: xen will free existing kmalloc'd mfn in
> > + * XENMEM_add_to_physmap */
>
> It doesn't leak it?
> We should consider using xenballoon_alloc_pages here.

Well, it's a one time allocation of a dozen (iirc) pages, that are
returned back to dom heap. We could use xenballoon, but for that we
need to create vma first so we get the contigous VA, then map each pte
since xenballoon_ doesn't return contigous pages, and then populate
physmap. I'm gonna punt it to phase II. Lets get some working baseline
in asap.

thanks
mukesh

2012-10-17 09:32:13

by Ian Campbell

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH V2 6/7]: PVH: balloon and grant changes

On Wed, 2012-10-17 at 01:09 +0100, Mukesh Rathor wrote:
> > > gsv.version = 1;
> > > else
> > > gsv.version = 2;
> > > @@ -1083,12 +1088,24 @@ static void gnttab_request_version(void)
> > > int gnttab_resume(void)
> > > {
> > > unsigned int max_nr_gframes;
> > > + char *kmsg = "Failed to kmalloc pages for pv in hvm grant
> > > frames\n";
> > > gnttab_request_version();
> > > max_nr_gframes = gnttab_max_grant_frames();
> > > if (max_nr_gframes < nr_grant_frames)
> > > return -ENOSYS;
> > >
> > > + /* PVH note: xen will free existing kmalloc'd mfn in
> > > + * XENMEM_add_to_physmap */
> >
> > It doesn't leak it?
> > We should consider using xenballoon_alloc_pages here.
>
> Well, it's a one time allocation of a dozen (iirc) pages, that are
> returned back to dom heap.

> We could use xenballoon, but for that we
> need to create vma first so we get the contigous VA,
> then map each pte
> since xenballoon_ doesn't return contigous pages, and then populate
> physmap. I'm gonna punt it to phase II. Lets get some working baseline
> in asap.

That's fair enough.