Stefano Stabellini
2013-Dec-11 16:58 UTC
[PATCH] xen/balloon: do not modify the p2m for auto_translate guests
decrease_reservation doesn''t modify the p2m for auto_translate guests,
but increase_reservation does.
Fix that by avoiding any p2m modifications in both increase_reservation
and decrease_reservation for auto_translated guests.
Avoid allocating or using scratch pages for auto_translated guests.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
drivers/xen/balloon.c | 63 ++++++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 29 deletions(-)
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 55ea73f..4c02e2b 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -350,17 +350,19 @@ static enum bp_state increase_reservation(unsigned long
nr_pages)
pfn = page_to_pfn(page);
- set_phys_to_machine(pfn, frame_list[i]);
-
#ifdef CONFIG_XEN_HAVE_PVMMU
- /* Link back into the page tables if not highmem. */
- if (xen_pv_domain() && !PageHighMem(page)) {
- int ret;
- ret = HYPERVISOR_update_va_mapping(
- (unsigned long)__va(pfn << PAGE_SHIFT),
- mfn_pte(frame_list[i], PAGE_KERNEL),
- 0);
- BUG_ON(ret);
+ 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 (!PageHighMem(page)) {
+ int ret;
+ ret = HYPERVISOR_update_va_mapping(
+ (unsigned long)__va(pfn << PAGE_SHIFT),
+ mfn_pte(frame_list[i], PAGE_KERNEL),
+ 0);
+ BUG_ON(ret);
+ }
}
#endif
@@ -378,7 +380,6 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
enum bp_state state = BP_DONE;
unsigned long pfn, i;
struct page *page;
- struct page *scratch_page;
int ret;
struct xen_memory_reservation reservation = {
.address_bits = 0,
@@ -411,27 +412,29 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
scrub_page(page);
+#ifdef CONFIG_XEN_HAVE_PVMMU
/*
* Ballooned out frames are effectively replaced with
* a scratch frame. Ensure direct mappings and the
* p2m are consistent.
*/
- scratch_page = get_balloon_scratch_page();
-#ifdef CONFIG_XEN_HAVE_PVMMU
- if (xen_pv_domain() && !PageHighMem(page)) {
- ret = HYPERVISOR_update_va_mapping(
- (unsigned long)__va(pfn << PAGE_SHIFT),
- pfn_pte(page_to_pfn(scratch_page),
- PAGE_KERNEL_RO), 0);
- BUG_ON(ret);
- }
-#endif
if (!xen_feature(XENFEAT_auto_translated_physmap)) {
unsigned long p;
+ struct page *scratch_page = get_balloon_scratch_page();
+
+ if (!PageHighMem(page)) {
+ ret = HYPERVISOR_update_va_mapping(
+ (unsigned long)__va(pfn << PAGE_SHIFT),
+ pfn_pte(page_to_pfn(scratch_page),
+ PAGE_KERNEL_RO), 0);
+ BUG_ON(ret);
+ }
p = page_to_pfn(scratch_page);
__set_phys_to_machine(pfn, pfn_to_mfn(p));
+
+ put_balloon_scratch_page();
}
- put_balloon_scratch_page();
+#endif
balloon_append(pfn_to_page(pfn));
}
@@ -627,15 +630,17 @@ static int __init balloon_init(void)
if (!xen_domain())
return -ENODEV;
- for_each_online_cpu(cpu)
- {
- per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
- if (per_cpu(balloon_scratch_page, cpu) == NULL) {
- pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n",
cpu);
- return -ENOMEM;
+ if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+ for_each_online_cpu(cpu)
+ {
+ per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL);
+ if (per_cpu(balloon_scratch_page, cpu) == NULL) {
+ pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n",
cpu);
+ return -ENOMEM;
+ }
}
+ register_cpu_notifier(&balloon_cpu_notifier);
}
- register_cpu_notifier(&balloon_cpu_notifier);
pr_info("Initialising balloon driver\n");
--
1.7.10.4
Konrad Rzeszutek Wilk
2013-Dec-11 17:12 UTC
Re: [PATCH] xen/balloon: do not modify the p2m for auto_translate guests
On Wed, Dec 11, 2013 at 04:58:42PM +0000, Stefano Stabellini wrote:> decrease_reservation doesn''t modify the p2m for auto_translate guests, > but increase_reservation does. > Fix that by avoiding any p2m modifications in both increase_reservation > and decrease_reservation for auto_translated guests. > > Avoid allocating or using scratch pages for auto_translated guests.Mukesh, is this similar to your patch?> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > drivers/xen/balloon.c | 63 ++++++++++++++++++++++++++----------------------- > 1 file changed, 34 insertions(+), 29 deletions(-) > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c > index 55ea73f..4c02e2b 100644 > --- a/drivers/xen/balloon.c > +++ b/drivers/xen/balloon.c > @@ -350,17 +350,19 @@ static enum bp_state increase_reservation(unsigned long nr_pages) > > pfn = page_to_pfn(page); > > - set_phys_to_machine(pfn, frame_list[i]); > - > #ifdef CONFIG_XEN_HAVE_PVMMU > - /* Link back into the page tables if not highmem. */ > - if (xen_pv_domain() && !PageHighMem(page)) { > - int ret; > - ret = HYPERVISOR_update_va_mapping( > - (unsigned long)__va(pfn << PAGE_SHIFT), > - mfn_pte(frame_list[i], PAGE_KERNEL), > - 0); > - BUG_ON(ret); > + 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 (!PageHighMem(page)) { > + int ret; > + ret = HYPERVISOR_update_va_mapping( > + (unsigned long)__va(pfn << PAGE_SHIFT), > + mfn_pte(frame_list[i], PAGE_KERNEL), > + 0); > + BUG_ON(ret); > + } > } > #endif > > @@ -378,7 +380,6 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > enum bp_state state = BP_DONE; > unsigned long pfn, i; > struct page *page; > - struct page *scratch_page; > int ret; > struct xen_memory_reservation reservation = { > .address_bits = 0, > @@ -411,27 +412,29 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > > scrub_page(page); > > +#ifdef CONFIG_XEN_HAVE_PVMMU > /* > * Ballooned out frames are effectively replaced with > * a scratch frame. Ensure direct mappings and the > * p2m are consistent. > */ > - scratch_page = get_balloon_scratch_page(); > -#ifdef CONFIG_XEN_HAVE_PVMMU > - if (xen_pv_domain() && !PageHighMem(page)) { > - ret = HYPERVISOR_update_va_mapping( > - (unsigned long)__va(pfn << PAGE_SHIFT), > - pfn_pte(page_to_pfn(scratch_page), > - PAGE_KERNEL_RO), 0); > - BUG_ON(ret); > - } > -#endif > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > unsigned long p; > + struct page *scratch_page = get_balloon_scratch_page(); > + > + if (!PageHighMem(page)) { > + ret = HYPERVISOR_update_va_mapping( > + (unsigned long)__va(pfn << PAGE_SHIFT), > + pfn_pte(page_to_pfn(scratch_page), > + PAGE_KERNEL_RO), 0); > + BUG_ON(ret); > + } > p = page_to_pfn(scratch_page); > __set_phys_to_machine(pfn, pfn_to_mfn(p)); > + > + put_balloon_scratch_page(); > } > - put_balloon_scratch_page(); > +#endif > > balloon_append(pfn_to_page(pfn)); > } > @@ -627,15 +630,17 @@ static int __init balloon_init(void) > if (!xen_domain()) > return -ENODEV; > > - for_each_online_cpu(cpu) > - { > - per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL); > - if (per_cpu(balloon_scratch_page, cpu) == NULL) { > - pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu); > - return -ENOMEM; > + if (!xen_feature(XENFEAT_auto_translated_physmap)) { > + for_each_online_cpu(cpu) > + { > + per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL); > + if (per_cpu(balloon_scratch_page, cpu) == NULL) { > + pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu); > + return -ENOMEM; > + } > } > + register_cpu_notifier(&balloon_cpu_notifier); > } > - register_cpu_notifier(&balloon_cpu_notifier); > > pr_info("Initialising balloon driver\n"); > > -- > 1.7.10.4 >
Konrad Rzeszutek Wilk
2013-Dec-13 14:35 UTC
Re: [PATCH] xen/balloon: do not modify the p2m for auto_translate guests
On Wed, Dec 11, 2013 at 04:58:42PM +0000, Stefano Stabellini wrote:> decrease_reservation doesn''t modify the p2m for auto_translate guests, > but increase_reservation does. > Fix that by avoiding any p2m modifications in both increase_reservation > and decrease_reservation for auto_translated guests. > > Avoid allocating or using scratch pages for auto_translated guests. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > drivers/xen/balloon.c | 63 ++++++++++++++++++++++++++----------------------- > 1 file changed, 34 insertions(+), 29 deletions(-) > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c > index 55ea73f..4c02e2b 100644 > --- a/drivers/xen/balloon.c > +++ b/drivers/xen/balloon.c > @@ -350,17 +350,19 @@ static enum bp_state increase_reservation(unsigned long nr_pages) > > pfn = page_to_pfn(page); > > - set_phys_to_machine(pfn, frame_list[i]); > - > #ifdef CONFIG_XEN_HAVE_PVMMU > - /* Link back into the page tables if not highmem. */ > - if (xen_pv_domain() && !PageHighMem(page)) { > - int ret; > - ret = HYPERVISOR_update_va_mapping( > - (unsigned long)__va(pfn << PAGE_SHIFT), > - mfn_pte(frame_list[i], PAGE_KERNEL), > - 0); > - BUG_ON(ret); > + 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 (!PageHighMem(page)) { > + int ret; > + ret = HYPERVISOR_update_va_mapping( > + (unsigned long)__va(pfn << PAGE_SHIFT), > + mfn_pte(frame_list[i], PAGE_KERNEL), > + 0); > + BUG_ON(ret); > + } > } > #endif > > @@ -378,7 +380,6 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > enum bp_state state = BP_DONE; > unsigned long pfn, i; > struct page *page; > - struct page *scratch_page; > int ret; > struct xen_memory_reservation reservation = { > .address_bits = 0, > @@ -411,27 +412,29 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > > scrub_page(page); > > +#ifdef CONFIG_XEN_HAVE_PVMMU > /* > * Ballooned out frames are effectively replaced with > * a scratch frame. Ensure direct mappings and the > * p2m are consistent. > */ > - scratch_page = get_balloon_scratch_page(); > -#ifdef CONFIG_XEN_HAVE_PVMMU > - if (xen_pv_domain() && !PageHighMem(page)) { > - ret = HYPERVISOR_update_va_mapping( > - (unsigned long)__va(pfn << PAGE_SHIFT), > - pfn_pte(page_to_pfn(scratch_page), > - PAGE_KERNEL_RO), 0); > - BUG_ON(ret); > - } > -#endif > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > unsigned long p; > + struct page *scratch_page = get_balloon_scratch_page(); > + > + if (!PageHighMem(page)) {How come you removed the ''xen_pv_domain'' part?> + ret = HYPERVISOR_update_va_mapping( > + (unsigned long)__va(pfn << PAGE_SHIFT), > + pfn_pte(page_to_pfn(scratch_page), > + PAGE_KERNEL_RO), 0); > + BUG_ON(ret); > + } > p = page_to_pfn(scratch_page); > __set_phys_to_machine(pfn, pfn_to_mfn(p)); > + > + put_balloon_scratch_page(); > } > - put_balloon_scratch_page(); > +#endif > > balloon_append(pfn_to_page(pfn)); > } > @@ -627,15 +630,17 @@ static int __init balloon_init(void) > if (!xen_domain()) > return -ENODEV; > > - for_each_online_cpu(cpu) > - { > - per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL); > - if (per_cpu(balloon_scratch_page, cpu) == NULL) { > - pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu); > - return -ENOMEM; > + if (!xen_feature(XENFEAT_auto_translated_physmap)) { > + for_each_online_cpu(cpu) > + { > + per_cpu(balloon_scratch_page, cpu) = alloc_page(GFP_KERNEL); > + if (per_cpu(balloon_scratch_page, cpu) == NULL) { > + pr_warn("Failed to allocate balloon_scratch_page for cpu %d\n", cpu); > + return -ENOMEM; > + } > } > + register_cpu_notifier(&balloon_cpu_notifier); > } > - register_cpu_notifier(&balloon_cpu_notifier); > > pr_info("Initialising balloon driver\n"); > > -- > 1.7.10.4 >
Stefano Stabellini
2013-Dec-13 14:47 UTC
Re: [PATCH] xen/balloon: do not modify the p2m for auto_translate guests
On Fri, 13 Dec 2013, Konrad Rzeszutek Wilk wrote:> On Wed, Dec 11, 2013 at 04:58:42PM +0000, Stefano Stabellini wrote: > > decrease_reservation doesn''t modify the p2m for auto_translate guests, > > but increase_reservation does. > > Fix that by avoiding any p2m modifications in both increase_reservation > > and decrease_reservation for auto_translated guests. > > > > Avoid allocating or using scratch pages for auto_translated guests. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > drivers/xen/balloon.c | 63 ++++++++++++++++++++++++++----------------------- > > 1 file changed, 34 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c > > index 55ea73f..4c02e2b 100644 > > --- a/drivers/xen/balloon.c > > +++ b/drivers/xen/balloon.c > > @@ -350,17 +350,19 @@ static enum bp_state increase_reservation(unsigned long nr_pages) > > > > pfn = page_to_pfn(page); > > > > - set_phys_to_machine(pfn, frame_list[i]); > > - > > #ifdef CONFIG_XEN_HAVE_PVMMU > > - /* Link back into the page tables if not highmem. */ > > - if (xen_pv_domain() && !PageHighMem(page)) { > > - int ret; > > - ret = HYPERVISOR_update_va_mapping( > > - (unsigned long)__va(pfn << PAGE_SHIFT), > > - mfn_pte(frame_list[i], PAGE_KERNEL), > > - 0); > > - BUG_ON(ret); > > + 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 (!PageHighMem(page)) { > > + int ret; > > + ret = HYPERVISOR_update_va_mapping( > > + (unsigned long)__va(pfn << PAGE_SHIFT), > > + mfn_pte(frame_list[i], PAGE_KERNEL), > > + 0); > > + BUG_ON(ret); > > + } > > } > > #endif > > > > @@ -378,7 +380,6 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > > enum bp_state state = BP_DONE; > > unsigned long pfn, i; > > struct page *page; > > - struct page *scratch_page; > > int ret; > > struct xen_memory_reservation reservation = { > > .address_bits = 0, > > @@ -411,27 +412,29 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > > > > scrub_page(page); > > > > +#ifdef CONFIG_XEN_HAVE_PVMMU > > /* > > * Ballooned out frames are effectively replaced with > > * a scratch frame. Ensure direct mappings and the > > * p2m are consistent. > > */ > > - scratch_page = get_balloon_scratch_page(); > > -#ifdef CONFIG_XEN_HAVE_PVMMU > > - if (xen_pv_domain() && !PageHighMem(page)) { > > - ret = HYPERVISOR_update_va_mapping( > > - (unsigned long)__va(pfn << PAGE_SHIFT), > > - pfn_pte(page_to_pfn(scratch_page), > > - PAGE_KERNEL_RO), 0); > > - BUG_ON(ret); > > - } > > -#endif > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > unsigned long p; > > + struct page *scratch_page = get_balloon_scratch_page(); > > + > > + if (!PageHighMem(page)) { > > How come you removed the ''xen_pv_domain'' part?I thought that it''s meaningless here. What we care about is whether it is an auto_translated guests or not, everything else is irrelevant. It also goes in the direction of using specific tests for things instead of catch all tests like xen_pv_domain that don''t mean much anymore.
Konrad Rzeszutek Wilk
2013-Dec-13 14:57 UTC
Re: [PATCH] xen/balloon: do not modify the p2m for auto_translate guests
On Fri, Dec 13, 2013 at 02:47:45PM +0000, Stefano Stabellini wrote:> On Fri, 13 Dec 2013, Konrad Rzeszutek Wilk wrote: > > On Wed, Dec 11, 2013 at 04:58:42PM +0000, Stefano Stabellini wrote: > > > decrease_reservation doesn''t modify the p2m for auto_translate guests, > > > but increase_reservation does. > > > Fix that by avoiding any p2m modifications in both increase_reservation > > > and decrease_reservation for auto_translated guests. > > > > > > Avoid allocating or using scratch pages for auto_translated guests. > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > --- > > > drivers/xen/balloon.c | 63 ++++++++++++++++++++++++++----------------------- > > > 1 file changed, 34 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c > > > index 55ea73f..4c02e2b 100644 > > > --- a/drivers/xen/balloon.c > > > +++ b/drivers/xen/balloon.c > > > @@ -350,17 +350,19 @@ static enum bp_state increase_reservation(unsigned long nr_pages) > > > > > > pfn = page_to_pfn(page); > > > > > > - set_phys_to_machine(pfn, frame_list[i]); > > > - > > > #ifdef CONFIG_XEN_HAVE_PVMMU > > > - /* Link back into the page tables if not highmem. */ > > > - if (xen_pv_domain() && !PageHighMem(page)) { > > > - int ret; > > > - ret = HYPERVISOR_update_va_mapping( > > > - (unsigned long)__va(pfn << PAGE_SHIFT), > > > - mfn_pte(frame_list[i], PAGE_KERNEL), > > > - 0); > > > - BUG_ON(ret); > > > + 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 (!PageHighMem(page)) { > > > + int ret; > > > + ret = HYPERVISOR_update_va_mapping( > > > + (unsigned long)__va(pfn << PAGE_SHIFT), > > > + mfn_pte(frame_list[i], PAGE_KERNEL), > > > + 0); > > > + BUG_ON(ret); > > > + } > > > } > > > #endif > > > > > > @@ -378,7 +380,6 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > > > enum bp_state state = BP_DONE; > > > unsigned long pfn, i; > > > struct page *page; > > > - struct page *scratch_page; > > > int ret; > > > struct xen_memory_reservation reservation = { > > > .address_bits = 0, > > > @@ -411,27 +412,29 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp) > > > > > > scrub_page(page); > > > > > > +#ifdef CONFIG_XEN_HAVE_PVMMU > > > /* > > > * Ballooned out frames are effectively replaced with > > > * a scratch frame. Ensure direct mappings and the > > > * p2m are consistent. > > > */ > > > - scratch_page = get_balloon_scratch_page(); > > > -#ifdef CONFIG_XEN_HAVE_PVMMU > > > - if (xen_pv_domain() && !PageHighMem(page)) { > > > - ret = HYPERVISOR_update_va_mapping( > > > - (unsigned long)__va(pfn << PAGE_SHIFT), > > > - pfn_pte(page_to_pfn(scratch_page), > > > - PAGE_KERNEL_RO), 0); > > > - BUG_ON(ret); > > > - } > > > -#endif > > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > > unsigned long p; > > > + struct page *scratch_page = get_balloon_scratch_page(); > > > + > > > + if (!PageHighMem(page)) { > > > > How come you removed the ''xen_pv_domain'' part? > > I thought that it''s meaningless here. What we care about is whether it > is an auto_translated guests or not, everything else is irrelevant. > It also goes in the direction of using specific tests for things instead > of catch all tests like xen_pv_domain that don''t mean much anymore.Please mention that in the commit description. Actually looking at the description you could say: "xen/balloon: Seperate the auto-translate logic properly. The auto-xlat logic vs the non-xlat means that we don''t need to for auto-xlat guests (like PVH, HVM or ARM): - use P2M - use scratch page. However the code in increase_reservation does modify the p2m for auto_translate guests, but not in decrease_reservation. Fix that by avoiding any p2m modifications in both increase_reservation and decrease_reservation for auto_translated guests. And also avoid allocating or using scratch pages for auto_translated guests. Lastly, since !auto-xlat is really another way of saying ''xen_pv'' remove the redundant ''xen_pv_domain'' check. " or so.