[Xen-devel] [PATCH 0/3] purge free_shared_domheap_page()

[Xen-devel] [PATCH 0/3] purge free_shared_domheap_page()

[Paul Durrant]

Paul Durrant (3):
  x86 / vmx: make apic_access_mfn type-safe
  x86 / hvm: add domain_relinquish_resources() method
  x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

 xen/arch/x86/hvm/hvm.c             |  2 ++
 xen/arch/x86/hvm/mtrr.c            |  2 +-
 xen/arch/x86/hvm/svm/svm.c         |  5 ++++
 xen/arch/x86/hvm/vmx/vmx.c         | 40 ++++++++++++++++++++++--------
 xen/arch/x86/mm.c                  | 10 --------
 xen/common/page_alloc.c            |  3 ++-
 xen/include/asm-x86/hvm/hvm.h      |  1 +
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 xen/include/asm-x86/mm.h           |  2 --
 9 files changed, 42 insertions(+), 25 deletions(-)

-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe

[Paul Durrant]

Use mfn_t rather than unsigned long and change previous tests against 0 to
tests against INVALID_MFN (also introducing initialization to that value).

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/mtrr.c            |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c         | 14 +++++++-------
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 5ad15eafe0..8356e8de3d 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -818,7 +818,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     if ( direct_mmio )
     {
-        if ( (mfn_x(mfn) ^ d->arch.hvm.vmx.apic_access_mfn) >> order )
+        if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
             return MTRR_TYPE_UNCACHABLE;
         if ( order )
             return -1;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f83f102638..3d90e67a05 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -413,6 +413,7 @@ static int vmx_domain_initialise(struct domain *d)
     if ( !has_vlapic(d) )
         return 0;
 
+    d->arch.hvm.vmx.apic_access_mfn = INVALID_MFN;
     if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 )
         return rc;
 
@@ -3034,7 +3035,7 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
     mfn = page_to_mfn(pg);
     clear_domain_page(mfn);
     share_xen_page_with_guest(pg, d, SHARE_rw);
-    d->arch.hvm.vmx.apic_access_mfn = mfn_x(mfn);
+    d->arch.hvm.vmx.apic_access_mfn = mfn;
 
     return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
                               PAGE_ORDER_4K,
@@ -3043,24 +3044,23 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
 
 static void vmx_free_vlapic_mapping(struct domain *d)
 {
-    unsigned long mfn = d->arch.hvm.vmx.apic_access_mfn;
+    mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn;
 
-    if ( mfn != 0 )
-        free_shared_domheap_page(mfn_to_page(_mfn(mfn)));
+    if ( !mfn_eq(mfn, INVALID_MFN) )
+        free_shared_domheap_page(mfn_to_page(mfn));
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
 {
     paddr_t virt_page_ma, apic_page_ma;
 
-    if ( v->domain->arch.hvm.vmx.apic_access_mfn == 0 )
+    if ( mfn_eq(v->domain->arch.hvm.vmx.apic_access_mfn, INVALID_MFN) )
         return;
 
     ASSERT(cpu_has_vmx_virtualize_apic_accesses);
 
     virt_page_ma = page_to_maddr(vcpu_vlapic(v)->regs_page);
-    apic_page_ma = v->domain->arch.hvm.vmx.apic_access_mfn;
-    apic_page_ma <<= PAGE_SHIFT;
+    apic_page_ma = mfn_to_maddr(v->domain->arch.hvm.vmx.apic_access_mfn);
 
     vmx_vmcs_enter(v);
     __vmwrite(VIRTUAL_APIC_PAGE_ADDR, virt_page_ma);
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index a514299144..be4661a929 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -59,7 +59,7 @@ struct ept_data {
 #define _VMX_DOMAIN_PML_ENABLED    0
 #define VMX_DOMAIN_PML_ENABLED     (1ul << _VMX_DOMAIN_PML_ENABLED)
 struct vmx_domain {
-    unsigned long apic_access_mfn;
+    mfn_t apic_access_mfn;
     /* VMX_DOMAIN_* */
     unsigned int status;
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe

[Tian, Kevin]

> From: Paul Durrant <pdurrant@amazon.com>
> Sent: Tuesday, January 21, 2020 8:00 PM
> 
> Use mfn_t rather than unsigned long and change previous tests against 0 to
> tests against INVALID_MFN (also introducing initialization to that value).
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe

[Andrew Cooper]

On 21/01/2020 12:00, Paul Durrant wrote:
> Use mfn_t rather than unsigned long and change previous tests against 0 to
> tests against INVALID_MFN (also introducing initialization to that value).
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

I'm afraid this breaks the idempotency of vmx_free_vlapic_mapping(),
which gets in the way of domain/vcpu create/destroy cleanup.

Its fine to use 0 as the sentinel.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86 / vmx: make apic_access_mfn type-safe

[Jan Beulich]

On 22.01.2020 15:05, Andrew Cooper wrote:
> On 21/01/2020 12:00, Paul Durrant wrote:
>> Use mfn_t rather than unsigned long and change previous tests against 0 to
>> tests against INVALID_MFN (also introducing initialization to that value).
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> I'm afraid this breaks the idempotency of vmx_free_vlapic_mapping(),
> which gets in the way of domain/vcpu create/destroy cleanup.
> 
> Its fine to use 0 as the sentinel.

And with this adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method

[Paul Durrant]

There are two functions in hvm.c to deal with tear-down and a domain:
hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, only
the latter has an associated method in 'hvm_funcs'. This patch adds
a method for the former and stub definitions for SVM and VMX.

The VMX method will be used by a subsequent patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/hvm.c        | 2 ++
 xen/arch/x86/hvm/svm/svm.c    | 5 +++++
 xen/arch/x86/hvm/vmx/vmx.c    | 5 +++++
 xen/include/asm-x86/hvm/hvm.h | 1 +
 4 files changed, 13 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4723f5d09c..669dce6731 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d)
 
 void hvm_domain_relinquish_resources(struct domain *d)
 {
+    hvm_funcs.domain_relinquish_resources(d);
+
     if ( hvm_funcs.nhvm_domain_relinquish_resources )
         hvm_funcs.nhvm_domain_relinquish_resources(d);
 
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b1c376d455..24768e8682 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1155,6 +1155,10 @@ static int svm_domain_initialise(struct domain *d)
     return 0;
 }
 
+static void svm_domain_relinquish_resources(struct domain *d)
+{
+}
+
 static void svm_domain_destroy(struct domain *d)
 {
 }
@@ -2425,6 +2429,7 @@ static struct hvm_function_table __initdata svm_function_table = {
     .cpu_up               = svm_cpu_up,
     .cpu_down             = svm_cpu_down,
     .domain_initialise    = svm_domain_initialise,
+    .domain_relinquish_resources = svm_domain_relinquish_resources,
     .domain_destroy       = svm_domain_destroy,
     .vcpu_initialise      = svm_vcpu_initialise,
     .vcpu_destroy         = svm_vcpu_destroy,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3d90e67a05..3fd3ac61e1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -420,6 +420,10 @@ static int vmx_domain_initialise(struct domain *d)
     return 0;
 }
 
+static void vmx_domain_relinquish_resources(struct domain *d)
+{
+}
+
 static void vmx_domain_destroy(struct domain *d)
 {
     if ( !has_vlapic(d) )
@@ -2241,6 +2245,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .cpu_up_prepare       = vmx_cpu_up_prepare,
     .cpu_dead             = vmx_cpu_dead,
     .domain_initialise    = vmx_domain_initialise,
+    .domain_relinquish_resources = vmx_domain_relinquish_resources,
     .domain_destroy       = vmx_domain_destroy,
     .vcpu_initialise      = vmx_vcpu_initialise,
     .vcpu_destroy         = vmx_vcpu_destroy,
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 09793c12e9..9eab1d7493 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -107,6 +107,7 @@ struct hvm_function_table {
      * Initialise/destroy HVM domain/vcpu resources
      */
     int  (*domain_initialise)(struct domain *d);
+    void (*domain_relinquish_resources)(struct domain *d);
     void (*domain_destroy)(struct domain *d);
     int  (*vcpu_initialise)(struct vcpu *v);
     void (*vcpu_destroy)(struct vcpu *v);
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method

[Jan Beulich]

On 21.01.2020 13:00, Paul Durrant wrote:
> There are two functions in hvm.c to deal with tear-down and a domain:
> hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, only
> the latter has an associated method in 'hvm_funcs'. This patch adds
> a method for the former and stub definitions for SVM and VMX.

Why the stubs? Simply ...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d)
>  
>  void hvm_domain_relinquish_resources(struct domain *d)
>  {
> +    hvm_funcs.domain_relinquish_resources(d);

... stick a NULL check around this one. I also wonder whether, it
being entirely new, this wouldn't better use alternative call
patching right from the beginning. It's not the hottest path, but
avoiding indirect calls seems quite desirable, especially when
doing so is relatively cheap.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 22 January 2020 15:51
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>
> Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources()
> method
> 
> On 21.01.2020 13:00, Paul Durrant wrote:
> > There are two functions in hvm.c to deal with tear-down and a domain:
> > hvm_domain_relinquish_resources() and hvm_domain_destroy(). However,
> only
> > the latter has an associated method in 'hvm_funcs'. This patch adds
> > a method for the former and stub definitions for SVM and VMX.
> 
> Why the stubs? Simply ...
> 
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d)
> >
> >  void hvm_domain_relinquish_resources(struct domain *d)
> >  {
> > +    hvm_funcs.domain_relinquish_resources(d);
> 
> ... stick a NULL check around this one. I also wonder whether, it
> being entirely new, this wouldn't better use alternative call
> patching right from the beginning. It's not the hottest path, but
> avoiding indirect calls seems quite desirable, especially when
> doing so is relatively cheap.
> 

I'd like it to align with the rest of the hvm_funcs so I'll add the NULL check, but alternatives patch for all hvm_funcs seems like a good thing I the longer term.

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method

[Jan Beulich]

On 22.01.2020 16:56, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 22 January 2020 15:51
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
>> <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
>> <kevin.tian@intel.com>
>> Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources()
>> method
>>
>> On 21.01.2020 13:00, Paul Durrant wrote:
>>> There are two functions in hvm.c to deal with tear-down and a domain:
>>> hvm_domain_relinquish_resources() and hvm_domain_destroy(). However,
>> only
>>> the latter has an associated method in 'hvm_funcs'. This patch adds
>>> a method for the former and stub definitions for SVM and VMX.
>>
>> Why the stubs? Simply ...
>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d)
>>>
>>>  void hvm_domain_relinquish_resources(struct domain *d)
>>>  {
>>> +    hvm_funcs.domain_relinquish_resources(d);
>>
>> ... stick a NULL check around this one. I also wonder whether, it
>> being entirely new, this wouldn't better use alternative call
>> patching right from the beginning. It's not the hottest path, but
>> avoiding indirect calls seems quite desirable, especially when
>> doing so is relatively cheap.
>>
> 
> I'd like it to align with the rest of the hvm_funcs so I'll add the
> NULL check, but alternatives patch for all hvm_funcs seems like a
> good thing I the longer term.

The frequently used ones have been converted already. Hence my
suggestion to make new ones use that model from the beginning.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86 / hvm: add domain_relinquish_resources() method

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 22 January 2020 16:01
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>
> Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources()
> method
> 
> On 22.01.2020 16:56, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 22 January 2020 15:51
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> >> <roger.pau@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin
> Tian
> >> <kevin.tian@intel.com>
> >> Subject: Re: [PATCH 2/3] x86 / hvm: add domain_relinquish_resources()
> >> method
> >>
> >> On 21.01.2020 13:00, Paul Durrant wrote:
> >>> There are two functions in hvm.c to deal with tear-down and a domain:
> >>> hvm_domain_relinquish_resources() and hvm_domain_destroy(). However,
> >> only
> >>> the latter has an associated method in 'hvm_funcs'. This patch adds
> >>> a method for the former and stub definitions for SVM and VMX.
> >>
> >> Why the stubs? Simply ...
> >>
> >>> --- a/xen/arch/x86/hvm/hvm.c
> >>> +++ b/xen/arch/x86/hvm/hvm.c
> >>> @@ -715,6 +715,8 @@ int hvm_domain_initialise(struct domain *d)
> >>>
> >>>  void hvm_domain_relinquish_resources(struct domain *d)
> >>>  {
> >>> +    hvm_funcs.domain_relinquish_resources(d);
> >>
> >> ... stick a NULL check around this one. I also wonder whether, it
> >> being entirely new, this wouldn't better use alternative call
> >> patching right from the beginning. It's not the hottest path, but
> >> avoiding indirect calls seems quite desirable, especially when
> >> doing so is relatively cheap.
> >>
> >
> > I'd like it to align with the rest of the hvm_funcs so I'll add the
> > NULL check, but alternatives patch for all hvm_funcs seems like a
> > good thing I the longer term.
> 
> The frequently used ones have been converted already. Hence my
> suggestion to make new ones use that model from the beginning.
> 

Oh, ok. I'll go look for some examples.

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Paul Durrant]

vmx_alloc_vlapic_mapping() currently contains some very odd looking code
that allocates a MEMF_no_owner domheap page and then shares with the guest
as if it were a xenheap page. This then requires vmx_free_vlapic_mapping()
to call a special function in the mm code: free_shared_domheap_page().

By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to
alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping()
can simply use get_page_and_type() to set up a writable mapping before
insertion in the P2M and vmx_free_vlapic_mapping() can simply release the
page using put_page_alloc_ref() followed by put_page_and_type(). This
then allows free_shared_domheap_page() to be purged.

There is, however, some fall-out from this simplification:

- alloc_domheap_page() will now call assign_pages() and run into the fact
  that 'max_pages' is not set until some time after domain_create(). To
  avoid an allocation failure, assign_pages() is modified to ignore the
  max_pages limit if 'creation_finished' is false. That value is not set
  to true until domain_unpause_by_systemcontroller() is called, and thus
  the guest cannot run (and hence cause memory allocation) until
  creation_finished is set to true.

- Because the domheap page is no longer a pseudo-xenheap page, the
  reference counting will prevent the domain from being destroyed. Thus
  the call to vmx_free_vlapic_mapping() is moved from the
  domain_destroy() method into the domain_relinquish_resources() method.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/vmx/vmx.c | 29 ++++++++++++++++++++++-------
 xen/arch/x86/mm.c          | 10 ----------
 xen/common/page_alloc.c    |  3 ++-
 xen/include/asm-x86/mm.h   |  2 --
 4 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3fd3ac61e1..a2e6081485 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -421,10 +421,6 @@ static int vmx_domain_initialise(struct domain *d)
 }
 
 static void vmx_domain_relinquish_resources(struct domain *d)
-{
-}
-
-static void vmx_domain_destroy(struct domain *d)
 {
     if ( !has_vlapic(d) )
         return;
@@ -432,6 +428,10 @@ static void vmx_domain_destroy(struct domain *d)
     vmx_free_vlapic_mapping(d);
 }
 
+static void vmx_domain_destroy(struct domain *d)
+{
+}
+
 static int vmx_vcpu_initialise(struct vcpu *v)
 {
     int rc;
@@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
     if ( !cpu_has_vmx_virtualize_apic_accesses )
         return 0;
 
-    pg = alloc_domheap_page(d, MEMF_no_owner);
+    pg = alloc_domheap_page(d, 0);
     if ( !pg )
         return -ENOMEM;
+
+    if ( !get_page_and_type(pg, d, PGT_writable_page) )
+    {
+        /*
+         * The domain can't possibly know about this page yet, so failure
+         * here is a clear indication of something fishy going on.
+         */
+        domain_crash(d);
+        return -ENODATA;
+    }
+
     mfn = page_to_mfn(pg);
     clear_domain_page(mfn);
-    share_xen_page_with_guest(pg, d, SHARE_rw);
     d->arch.hvm.vmx.apic_access_mfn = mfn;
 
     return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
@@ -3052,7 +3062,12 @@ static void vmx_free_vlapic_mapping(struct domain *d)
     mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn;
 
     if ( !mfn_eq(mfn, INVALID_MFN) )
-        free_shared_domheap_page(mfn_to_page(mfn));
+    {
+        struct page_info *pg = mfn_to_page(mfn);
+
+        put_page_alloc_ref(pg);
+        put_page_and_type(pg);
+    }
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 654190e9e9..2a6d2e8af9 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
     spin_unlock(&d->page_alloc_lock);
 }
 
-void free_shared_domheap_page(struct page_info *page)
-{
-    put_page_alloc_ref(page);
-    if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) )
-        ASSERT_UNREACHABLE();
-    page->u.inuse.type_info = 0;
-    page_set_owner(page, NULL);
-    free_domheap_page(page);
-}
-
 void make_cr3(struct vcpu *v, mfn_t mfn)
 {
     struct domain *d = v->domain;
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 919a270587..ef327072ed 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2269,7 +2269,8 @@ int assign_pages(
 
     if ( !(memflags & MEMF_no_refcount) )
     {
-        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
+        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
+             d->creation_finished )
         {
             gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
                     "%u > %u\n", d->domain_id,
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 2ca8882ad0..e429f38228 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -317,8 +317,6 @@ struct page_info
 
 #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
 
-extern void free_shared_domheap_page(struct page_info *page);
-
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
 extern unsigned long max_page;
 extern unsigned long total_pages;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Julien Grall]

Hi,

On 21/01/2020 12:00, Paul Durrant wrote:
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 919a270587..ef327072ed 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2269,7 +2269,8 @@ int assign_pages(
>   
>       if ( !(memflags & MEMF_no_refcount) )
>       {
> -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> +             d->creation_finished )

This is not entirely obvious to me how this is safe. What would happen 
if d->creation_finished is set on another CPU at the same time? At least 
on Arm, this may not be seen directly.

I guess the problem would not only happen in this use case (I am more 
concerned in the physmap code), but it would be good to document how it 
is meant to be safe to use.

However, AFAIU, the only reason for the check to be here is because 
d->max_pages is set quite late. How about setting max_pages as part of 
the domain_create hypercall?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: 21 January 2020 12:29
> To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org
> Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Jun Nakajima <jun.nakajima@intel.com>; Wei Liu
> <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George
> Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
> Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap
> page for APIC_DEFAULT_PHYS_BASE
> 
> Hi,
> 
> On 21/01/2020 12:00, Paul Durrant wrote:
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 919a270587..ef327072ed 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2269,7 +2269,8 @@ int assign_pages(
> >
> >       if ( !(memflags & MEMF_no_refcount) )
> >       {
> > -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> > +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> > +             d->creation_finished )
> 
> This is not entirely obvious to me how this is safe. What would happen
> if d->creation_finished is set on another CPU at the same time? At least
> on Arm, this may not be seen directly.
> 
> I guess the problem would not only happen in this use case (I am more
> concerned in the physmap code), but it would be good to document how it
> is meant to be safe to use.
> 
> However, AFAIU, the only reason for the check to be here is because
> d->max_pages is set quite late. How about setting max_pages as part of
> the domain_create hypercall?

That would be useful but certainly more invasive. There's no way a guest vcpu can see creation_finished set to true as it is still paused. The only concern would be a stub domain causing domheap pages to be allocated on behalf of the guest, and can we not trust a stub domain until it's guest has been unpaused (since there is no scope for the guest to attack it until then)?

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Tian, Kevin]

> From: Paul Durrant <pdurrant@amazon.com>
> Sent: Tuesday, January 21, 2020 8:00 PM
> 
> vmx_alloc_vlapic_mapping() currently contains some very odd looking code
> that allocates a MEMF_no_owner domheap page and then shares with the
> guest
> as if it were a xenheap page. This then requires vmx_free_vlapic_mapping()
> to call a special function in the mm code: free_shared_domheap_page().
> 
> By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping()
> can simply use get_page_and_type() to set up a writable mapping before
> insertion in the P2M and vmx_free_vlapic_mapping() can simply release the
> page using put_page_alloc_ref() followed by put_page_and_type(). This
> then allows free_shared_domheap_page() to be purged.

I doubt whether using a normal domheap page is a right thing in concept.
Note the APIC access page is the backend for virtual LAPIC_BASE which is 
a MMIO range from guest p.o.v. 

> 
> There is, however, some fall-out from this simplification:
> 
> - alloc_domheap_page() will now call assign_pages() and run into the fact
>   that 'max_pages' is not set until some time after domain_create(). To
>   avoid an allocation failure, assign_pages() is modified to ignore the
>   max_pages limit if 'creation_finished' is false. That value is not set
>   to true until domain_unpause_by_systemcontroller() is called, and thus
>   the guest cannot run (and hence cause memory allocation) until
>   creation_finished is set to true.

However, doing so opens the window of no guard of allocations in 
the whole VM creation time. I'm not sure whether it's a worthwhile
just for fixing a bad-looking but conceptually-correct code.

> 
> - Because the domheap page is no longer a pseudo-xenheap page, the
>   reference counting will prevent the domain from being destroyed. Thus
>   the call to vmx_free_vlapic_mapping() is moved from the
>   domain_destroy() method into the domain_relinquish_resources() method.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 29 ++++++++++++++++++++++-------
>  xen/arch/x86/mm.c          | 10 ----------
>  xen/common/page_alloc.c    |  3 ++-
>  xen/include/asm-x86/mm.h   |  2 --
>  4 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 3fd3ac61e1..a2e6081485 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -421,10 +421,6 @@ static int vmx_domain_initialise(struct domain *d)
>  }
> 
>  static void vmx_domain_relinquish_resources(struct domain *d)
> -{
> -}
> -
> -static void vmx_domain_destroy(struct domain *d)
>  {
>      if ( !has_vlapic(d) )
>          return;
> @@ -432,6 +428,10 @@ static void vmx_domain_destroy(struct domain *d)
>      vmx_free_vlapic_mapping(d);
>  }
> 
> +static void vmx_domain_destroy(struct domain *d)
> +{
> +}
> +
>  static int vmx_vcpu_initialise(struct vcpu *v)
>  {
>      int rc;
> @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct
> domain *d)
>      if ( !cpu_has_vmx_virtualize_apic_accesses )
>          return 0;
> 
> -    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    pg = alloc_domheap_page(d, 0);
>      if ( !pg )
>          return -ENOMEM;
> +
> +    if ( !get_page_and_type(pg, d, PGT_writable_page) )
> +    {
> +        /*
> +         * The domain can't possibly know about this page yet, so failure
> +         * here is a clear indication of something fishy going on.
> +         */
> +        domain_crash(d);
> +        return -ENODATA;
> +    }
> +
>      mfn = page_to_mfn(pg);
>      clear_domain_page(mfn);
> -    share_xen_page_with_guest(pg, d, SHARE_rw);
>      d->arch.hvm.vmx.apic_access_mfn = mfn;
> 
>      return set_mmio_p2m_entry(d,
> paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
> @@ -3052,7 +3062,12 @@ static void vmx_free_vlapic_mapping(struct
> domain *d)
>      mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn;
> 
>      if ( !mfn_eq(mfn, INVALID_MFN) )
> -        free_shared_domheap_page(mfn_to_page(mfn));
> +    {
> +        struct page_info *pg = mfn_to_page(mfn);
> +
> +        put_page_alloc_ref(pg);
> +        put_page_and_type(pg);
> +    }
>  }
> 
>  static void vmx_install_vlapic_mapping(struct vcpu *v)
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 654190e9e9..2a6d2e8af9 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info
> *page, struct domain *d,
>      spin_unlock(&d->page_alloc_lock);
>  }
> 
> -void free_shared_domheap_page(struct page_info *page)
> -{
> -    put_page_alloc_ref(page);
> -    if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) )
> -        ASSERT_UNREACHABLE();
> -    page->u.inuse.type_info = 0;
> -    page_set_owner(page, NULL);
> -    free_domheap_page(page);
> -}
> -
>  void make_cr3(struct vcpu *v, mfn_t mfn)
>  {
>      struct domain *d = v->domain;
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 919a270587..ef327072ed 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2269,7 +2269,8 @@ int assign_pages(
> 
>      if ( !(memflags & MEMF_no_refcount) )
>      {
> -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> +             d->creation_finished )
>          {
>              gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
>                      "%u > %u\n", d->domain_id,
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 2ca8882ad0..e429f38228 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -317,8 +317,6 @@ struct page_info
> 
>  #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
> 
> -extern void free_shared_domheap_page(struct page_info *page);
> -
>  #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
>  extern unsigned long max_page;
>  extern unsigned long total_pages;
> --
> 2.20.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Durrant, Paul]

> -----Original Message-----
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: 22 January 2020 03:19
> To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org
> Cc: Nakajima, Jun <jun.nakajima@intel.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu
> <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; George Dunlap
> <George.Dunlap@eu.citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: RE: [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
> 
> > From: Paul Durrant <pdurrant@amazon.com>
> > Sent: Tuesday, January 21, 2020 8:00 PM
> >
> > vmx_alloc_vlapic_mapping() currently contains some very odd looking code
> > that allocates a MEMF_no_owner domheap page and then shares with the
> > guest
> > as if it were a xenheap page. This then requires
> vmx_free_vlapic_mapping()
> > to call a special function in the mm code: free_shared_domheap_page().
> >
> > By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> > alloc_domheap_page()), the odd looking code in
> vmx_alloc_vlapic_mapping()
> > can simply use get_page_and_type() to set up a writable mapping before
> > insertion in the P2M and vmx_free_vlapic_mapping() can simply release
> the
> > page using put_page_alloc_ref() followed by put_page_and_type(). This
> > then allows free_shared_domheap_page() to be purged.
> 
> I doubt whether using a normal domheap page is a right thing in concept.
> Note the APIC access page is the backend for virtual LAPIC_BASE which is
> a MMIO range from guest p.o.v.

Well, using a non-owned domheap page as a fake xenheap page just looks weird. Also 'special' code like this is historically a source of XSAs. IMO a straightforward domheap page here is totally appropriate; AFAICT the code is only as it is to work round the max_pages limit and the lack of a relinquish_resources() hvmfunc.

> 
> >
> > There is, however, some fall-out from this simplification:
> >
> > - alloc_domheap_page() will now call assign_pages() and run into the
> fact
> >   that 'max_pages' is not set until some time after domain_create(). To
> >   avoid an allocation failure, assign_pages() is modified to ignore the
> >   max_pages limit if 'creation_finished' is false. That value is not set
> >   to true until domain_unpause_by_systemcontroller() is called, and thus
> >   the guest cannot run (and hence cause memory allocation) until
> >   creation_finished is set to true.
> 
> However, doing so opens the window of no guard of allocations in
> the whole VM creation time. I'm not sure whether it's a worthwhile
> just for fixing a bad-looking but conceptually-correct code.
> 

I don’t think the code is conceptually correct. The lack of guard is only for pages assigned to that domain, and since the domain cannot possibly be running then I don't see it is a problem. That said, an alternative would be to have the domain creation code to set max_pages is to a minimal initial value sufficient to cover any alloc_domheap_pages() calls done by sub-functions.

  Paul

> >
> > - Because the domheap page is no longer a pseudo-xenheap page, the
> >   reference counting will prevent the domain from being destroyed. Thus
> >   the call to vmx_free_vlapic_mapping() is moved from the
> >   domain_destroy() method into the domain_relinquish_resources() method.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Julien Grall <julien@xen.org>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> >  xen/arch/x86/hvm/vmx/vmx.c | 29 ++++++++++++++++++++++-------
> >  xen/arch/x86/mm.c          | 10 ----------
> >  xen/common/page_alloc.c    |  3 ++-
> >  xen/include/asm-x86/mm.h   |  2 --
> >  4 files changed, 24 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 3fd3ac61e1..a2e6081485 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -421,10 +421,6 @@ static int vmx_domain_initialise(struct domain *d)
> >  }
> >
> >  static void vmx_domain_relinquish_resources(struct domain *d)
> > -{
> > -}
> > -
> > -static void vmx_domain_destroy(struct domain *d)
> >  {
> >      if ( !has_vlapic(d) )
> >          return;
> > @@ -432,6 +428,10 @@ static void vmx_domain_destroy(struct domain *d)
> >      vmx_free_vlapic_mapping(d);
> >  }
> >
> > +static void vmx_domain_destroy(struct domain *d)
> > +{
> > +}
> > +
> >  static int vmx_vcpu_initialise(struct vcpu *v)
> >  {
> >      int rc;
> > @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct
> > domain *d)
> >      if ( !cpu_has_vmx_virtualize_apic_accesses )
> >          return 0;
> >
> > -    pg = alloc_domheap_page(d, MEMF_no_owner);
> > +    pg = alloc_domheap_page(d, 0);
> >      if ( !pg )
> >          return -ENOMEM;
> > +
> > +    if ( !get_page_and_type(pg, d, PGT_writable_page) )
> > +    {
> > +        /*
> > +         * The domain can't possibly know about this page yet, so
> failure
> > +         * here is a clear indication of something fishy going on.
> > +         */
> > +        domain_crash(d);
> > +        return -ENODATA;
> > +    }
> > +
> >      mfn = page_to_mfn(pg);
> >      clear_domain_page(mfn);
> > -    share_xen_page_with_guest(pg, d, SHARE_rw);
> >      d->arch.hvm.vmx.apic_access_mfn = mfn;
> >
> >      return set_mmio_p2m_entry(d,
> > paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
> > @@ -3052,7 +3062,12 @@ static void vmx_free_vlapic_mapping(struct
> > domain *d)
> >      mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn;
> >
> >      if ( !mfn_eq(mfn, INVALID_MFN) )
> > -        free_shared_domheap_page(mfn_to_page(mfn));
> > +    {
> > +        struct page_info *pg = mfn_to_page(mfn);
> > +
> > +        put_page_alloc_ref(pg);
> > +        put_page_and_type(pg);
> > +    }
> >  }
> >
> >  static void vmx_install_vlapic_mapping(struct vcpu *v)
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 654190e9e9..2a6d2e8af9 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info
> > *page, struct domain *d,
> >      spin_unlock(&d->page_alloc_lock);
> >  }
> >
> > -void free_shared_domheap_page(struct page_info *page)
> > -{
> > -    put_page_alloc_ref(page);
> > -    if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) )
> > -        ASSERT_UNREACHABLE();
> > -    page->u.inuse.type_info = 0;
> > -    page_set_owner(page, NULL);
> > -    free_domheap_page(page);
> > -}
> > -
> >  void make_cr3(struct vcpu *v, mfn_t mfn)
> >  {
> >      struct domain *d = v->domain;
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 919a270587..ef327072ed 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2269,7 +2269,8 @@ int assign_pages(
> >
> >      if ( !(memflags & MEMF_no_refcount) )
> >      {
> > -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> > +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> > +             d->creation_finished )
> >          {
> >              gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
> >                      "%u > %u\n", d->domain_id,
> > diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> > index 2ca8882ad0..e429f38228 100644
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -317,8 +317,6 @@ struct page_info
> >
> >  #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
> >
> > -extern void free_shared_domheap_page(struct page_info *page);
> > -
> >  #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
> >  extern unsigned long max_page;
> >  extern unsigned long total_pages;
> > --
> > 2.20.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Jan Beulich]

On 21.01.2020 13:00, Paul Durrant wrote:
> vmx_alloc_vlapic_mapping() currently contains some very odd looking code
> that allocates a MEMF_no_owner domheap page and then shares with the guest
> as if it were a xenheap page. This then requires vmx_free_vlapic_mapping()
> to call a special function in the mm code: free_shared_domheap_page().
> 
> By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping()
> can simply use get_page_and_type() to set up a writable mapping before
> insertion in the P2M and vmx_free_vlapic_mapping() can simply release the
> page using put_page_alloc_ref() followed by put_page_and_type(). This
> then allows free_shared_domheap_page() to be purged.
> 
> There is, however, some fall-out from this simplification:
> 
> - alloc_domheap_page() will now call assign_pages() and run into the fact
>   that 'max_pages' is not set until some time after domain_create(). To
>   avoid an allocation failure, assign_pages() is modified to ignore the
>   max_pages limit if 'creation_finished' is false. That value is not set
>   to true until domain_unpause_by_systemcontroller() is called, and thus
>   the guest cannot run (and hence cause memory allocation) until
>   creation_finished is set to true.

But this check is also to guard against the tool stack (or possibly
the controlling stubdom) to cause excess allocation. I don't think
the checking should be undermined like this (and see also below).

Since certainly you've looked into this while creating the patch,
could you remind me why it is that this page needs to be owned (as
in its owner field set accordingly) by the guest? It's a helper
page only, after all.

> @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
>      if ( !cpu_has_vmx_virtualize_apic_accesses )
>          return 0;
>  
> -    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    pg = alloc_domheap_page(d, 0);

Did you consider passing MEMF_no_refcount here, to avoid the
fiddling with assign_pages()? That'll in particular also
avoid ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2269,7 +2269,8 @@ int assign_pages(
>  
>      if ( !(memflags & MEMF_no_refcount) )
>      {
> -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> +             d->creation_finished )
>          {
>              gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
>                      "%u > %u\n", d->domain_id,

... invoking domain_adjust_tot_pages() right below here, which
is wrong for helper pages like this one (as it reduces the
amount the domain is actually permitted to allocate).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 22 January 2020 16:17
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
> 
> On 21.01.2020 13:00, Paul Durrant wrote:
> > vmx_alloc_vlapic_mapping() currently contains some very odd looking code
> > that allocates a MEMF_no_owner domheap page and then shares with the
> guest
> > as if it were a xenheap page. This then requires
> vmx_free_vlapic_mapping()
> > to call a special function in the mm code: free_shared_domheap_page().
> >
> > By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> > alloc_domheap_page()), the odd looking code in
> vmx_alloc_vlapic_mapping()
> > can simply use get_page_and_type() to set up a writable mapping before
> > insertion in the P2M and vmx_free_vlapic_mapping() can simply release
> the
> > page using put_page_alloc_ref() followed by put_page_and_type(). This
> > then allows free_shared_domheap_page() to be purged.
> >
> > There is, however, some fall-out from this simplification:
> >
> > - alloc_domheap_page() will now call assign_pages() and run into the
> fact
> >   that 'max_pages' is not set until some time after domain_create(). To
> >   avoid an allocation failure, assign_pages() is modified to ignore the
> >   max_pages limit if 'creation_finished' is false. That value is not set
> >   to true until domain_unpause_by_systemcontroller() is called, and thus
> >   the guest cannot run (and hence cause memory allocation) until
> >   creation_finished is set to true.
> 
> But this check is also to guard against the tool stack (or possibly
> the controlling stubdom) to cause excess allocation. I don't think
> the checking should be undermined like this (and see also below).
>

Ok.
 
> Since certainly you've looked into this while creating the patch,
> could you remind me why it is that this page needs to be owned (as
> in its owner field set accordingly) by the guest? It's a helper
> page only, after all.
> 

Not sure why it was done that way. It's inserted into the guest P2M so having it owned by the guest seems like the right thing to do. A malicious guest could decrease-reservation it and I guess it avoids special-casing there.

> > @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct
> domain *d)
> >      if ( !cpu_has_vmx_virtualize_apic_accesses )
> >          return 0;
> >
> > -    pg = alloc_domheap_page(d, MEMF_no_owner);
> > +    pg = alloc_domheap_page(d, 0);
> 
> Did you consider passing MEMF_no_refcount here, to avoid the
> fiddling with assign_pages()? That'll in particular also
> avoid ...
> 

You remember what happened last time we did that (with the ioreq server page), right? That's why assign_pages() vetoes non-refcounted pages.

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2269,7 +2269,8 @@ int assign_pages(
> >
> >      if ( !(memflags & MEMF_no_refcount) )
> >      {
> > -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> > +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> > +             d->creation_finished )
> >          {
> >              gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
> >                      "%u > %u\n", d->domain_id,
> 
> ... invoking domain_adjust_tot_pages() right below here, which
> is wrong for helper pages like this one (as it reduces the
> amount the domain is actually permitted to allocate).
> 

True, but there is 'slop' to deal with things like the ioreq pages and I think this page is logically similar.

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel