* Re: [RFC PATCH 6/8]: Ballooning changes
2012-08-16 1:05 [RFC PATCH 6/8]: Ballooning changes Mukesh Rathor
@ 2012-08-16 14:13 ` Stefano Stabellini
2012-08-17 9:46 ` Ian Campbell
2012-09-13 16:33 ` Ian Campbell
2 siblings, 0 replies; 4+ messages in thread
From: Stefano Stabellini @ 2012-08-16 14:13 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: Xen-devel@lists.xensource.com, Konrad Rzeszutek Wilk
And patches like this one are the reason why I don't want xen_pvh_domain
in non-x86 xen files: I am pretty sure that if you didn't use
xen_pvh_domain here but a combination of XENFEAT_auto_translated_physmap
and xen_hvm_domain I would be able to reuse this work on ARM (that
doesn't have a working balloon driver yet) as is.
On Thu, 16 Aug 2012, Mukesh Rathor wrote:
> ---
> drivers/xen/balloon.c | 26 +++++++++++++++++++++-----
> 1 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 31ab82f..57960a1 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -358,10 +358,18 @@ 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_pvh_domain()) {
> + set_phys_to_machine(pfn, frame_list[i]);
> + } else {
> + pte_t *ptep;
> + unsigned int level;
> + void *addr = __va(pfn << PAGE_SHIFT);
> + ptep = lookup_address((unsigned long)addr, &level);
> + set_pte(ptep, pfn_pte(pfn, PAGE_KERNEL));
> + }
> /* Link back into the page tables if not highmem. */
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pv_domain() && !PageHighMem(page) &&
> + !xen_pvh_domain()) {
> int ret;
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> @@ -417,7 +425,14 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>
> scrub_page(page);
>
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pvh_domain() && !PageHighMem(page)) {
> + unsigned int level;
> + pte_t *ptep;
> + void *addr = __va(pfn << PAGE_SHIFT);
> + ptep = lookup_address((unsigned long)addr, &level);
> + set_pte(ptep, __pte(0));
> +
> + } else if (xen_pv_domain() && !PageHighMem(page)) {
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> __pte_ma(0), 0);
> @@ -433,7 +448,8 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> /* No more mappings: invalidate P2M and add to balloon. */
> for (i = 0; i < nr_pages; i++) {
> pfn = mfn_to_pfn(frame_list[i]);
> - __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> + if (!xen_pvh_domain())
> + __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> balloon_append(pfn_to_page(pfn));
> }
>
> --
> 1.7.2.3
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH 6/8]: Ballooning changes
2012-08-16 1:05 [RFC PATCH 6/8]: Ballooning changes Mukesh Rathor
2012-08-16 14:13 ` Stefano Stabellini
@ 2012-08-17 9:46 ` Ian Campbell
2012-09-13 16:33 ` Ian Campbell
2 siblings, 0 replies; 4+ messages in thread
From: Ian Campbell @ 2012-08-17 9:46 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: Xen-devel@lists.xensource.com, Konrad Rzeszutek Wilk
On Thu, 2012-08-16 at 02:05 +0100, Mukesh Rathor wrote:
> ---
> drivers/xen/balloon.c | 26 +++++++++++++++++++++-----
> 1 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 31ab82f..57960a1 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -358,10 +358,18 @@ 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_pvh_domain()) {
XENFEAT_auto_translated...?
> + set_phys_to_machine(pfn, frame_list[i]);
> + } else {
> + pte_t *ptep;
> + unsigned int level;
> + void *addr = __va(pfn << PAGE_SHIFT);
> + ptep = lookup_address((unsigned long)addr, &level);
> + set_pte(ptep, pfn_pte(pfn, PAGE_KERNEL));
> + }
> /* Link back into the page tables if not highmem. */
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pv_domain() && !PageHighMem(page) &&
> + !xen_pvh_domain()) {
It feels like this ought to be inside the !xen_pvh_domain above as well
as just hte set_phys_to_machine.
And is the else above not missing a !PageHighMem check?
> int ret;
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> @@ -417,7 +425,14 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>
> scrub_page(page);
>
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pvh_domain() && !PageHighMem(page)) {
> + unsigned int level;
> + pte_t *ptep;
> + void *addr = __va(pfn << PAGE_SHIFT);
> + ptep = lookup_address((unsigned long)addr, &level);
> + set_pte(ptep, __pte(0));
> +
> + } else if (xen_pv_domain() && !PageHighMem(page)) {
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> __pte_ma(0), 0);
This pattern:
if ( xen_pvh_... )
... lookup_address / set_pte ...
else
... HYOERVISOR_update_va_mapping
Was present above too -- candidate for a helper? (I was a bit surprised
there wasn't already one...)
> @@ -433,7 +448,8 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> /* No more mappings: invalidate P2M and add to balloon. */
> for (i = 0; i < nr_pages; i++) {
> pfn = mfn_to_pfn(frame_list[i]);
> - __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> + if (!xen_pvh_domain())
XENFEAT_something?
> + __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> balloon_append(pfn_to_page(pfn));
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH 6/8]: Ballooning changes
2012-08-16 1:05 [RFC PATCH 6/8]: Ballooning changes Mukesh Rathor
2012-08-16 14:13 ` Stefano Stabellini
2012-08-17 9:46 ` Ian Campbell
@ 2012-09-13 16:33 ` Ian Campbell
2 siblings, 0 replies; 4+ messages in thread
From: Ian Campbell @ 2012-09-13 16:33 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: Xen-devel@lists.xensource.com, Konrad Rzeszutek Wilk
On Thu, 2012-08-16 at 02:05 +0100, Mukesh Rathor wrote:
> ---
> drivers/xen/balloon.c | 26 +++++++++++++++++++++-----
> 1 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 31ab82f..57960a1 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -358,10 +358,18 @@ 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_pvh_domain()) {
> + set_phys_to_machine(pfn, frame_list[i]);
> + } else {
> + pte_t *ptep;
> + unsigned int level;
> + void *addr = __va(pfn << PAGE_SHIFT);
> + ptep = lookup_address((unsigned long)addr, &level);
> + set_pte(ptep, pfn_pte(pfn, PAGE_KERNEL));
Another thing I just tripped over on ARM which might be pertinent on x86
PVH too is that lowmem mappings on ARM are super 2M mappings, so trying
to clear the PTE here fails.
I was a bit wary of leaving stage 1 mappings in place for pfn's with no
backing mfn but this 2M mapping issue has caused me to revise that
opinion -- there's no need to worry about the stage 1 mapping still
being present as long as you guarantee you never touch the associated
virtual address, which the balloon driver should be able to do. The
other option is to shatter such mappings which is just too much pain to
contemplate.
Long story short I reckon you can drop this hunk (and associated similar
changes) and rely on the XENFEAT_auto_translated_physmap check inside
set_phys_to_machine to do the right thing.
I guess PVH currently suppresses superpage mappings on x86 (probably
inherited from PV) but undoing that might be something to investigate
for the future? It'll help perf I expect.
Ian.
> + }
> /* Link back into the page tables if not highmem. */
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pv_domain() && !PageHighMem(page) &&
> + !xen_pvh_domain()) {
> int ret;
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> @@ -417,7 +425,14 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>
> scrub_page(page);
>
> - if (xen_pv_domain() && !PageHighMem(page)) {
> + if (xen_pvh_domain() && !PageHighMem(page)) {
> + unsigned int level;
> + pte_t *ptep;
> + void *addr = __va(pfn << PAGE_SHIFT);
> + ptep = lookup_address((unsigned long)addr, &level);
> + set_pte(ptep, __pte(0));
> +
> + } else if (xen_pv_domain() && !PageHighMem(page)) {
> ret = HYPERVISOR_update_va_mapping(
> (unsigned long)__va(pfn << PAGE_SHIFT),
> __pte_ma(0), 0);
> @@ -433,7 +448,8 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> /* No more mappings: invalidate P2M and add to balloon. */
> for (i = 0; i < nr_pages; i++) {
> pfn = mfn_to_pfn(frame_list[i]);
> - __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> + if (!xen_pvh_domain())
> + __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> balloon_append(pfn_to_page(pfn));
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread