[PATCH for-next 0/8] Refactor x86/domain.c

[PATCH for-next 0/8] Refactor x86/domain.c

[Wei Liu]

This series refactors x86/domain.c to move HVM and PV specific code to their
respective directory.

The arch_set_info_guest is not touched yet. Refactoring that function will be
in another series because 1) I need to touch ARM code as well; 2) I need to do
some archaeology to know why it is done like that. The end result possibily
will involve changing toolstack a bit as well.

Wei.

Wei Liu (8):
  xen.h: fix comment for vcpu_guest_context
  x86/domain: factor out pv_vcpu_initialise
  x86/domain: factor out pv_vcpu_destroy
  x86/domain: push some code down to hvm_domain_initialise
  x86/domain: factor out pv_domain_destroy
  x86/domain: factor out pv_domain_initialise
  x86/domain: move PV specific code to pv/domain.c
  x86/domain: move HVM specific code to hvm/domain.c

 xen/arch/x86/domain.c             | 538 ++------------------------------------
 xen/arch/x86/hvm/Makefile         |   1 +
 xen/arch/x86/hvm/domain.c         | 322 +++++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c            |  25 +-
 xen/arch/x86/pv/Makefile          |   1 +
 xen/arch/x86/pv/domain.c          | 270 +++++++++++++++++++
 xen/include/asm-x86/domain.h      |   3 +
 xen/include/asm-x86/hvm/hvm.h     |   2 +-
 xen/include/asm-x86/pv/pv.h       |  29 ++
 xen/include/public/arch-x86/xen.h |   4 +-
 10 files changed, 662 insertions(+), 533 deletions(-)
 create mode 100644 xen/arch/x86/hvm/domain.c
 create mode 100644 xen/arch/x86/pv/domain.c
 create mode 100644 xen/include/asm-x86/pv/pv.h

-- 
2.11.0


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

[PATCH for-next 1/8] xen.h: fix comment for vcpu_guest_context

[Wei Liu]

Use the correct vcpuop name and delete on trailing white space.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/public/arch-x86/xen.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 8a9ba7982b..f21332e897 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -159,10 +159,10 @@ DEFINE_XEN_GUEST_HANDLE(trap_info_t);
 typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
 
 /*
- * The following is all CPU context. Note that the fpu_ctxt block is filled 
+ * The following is all CPU context. Note that the fpu_ctxt block is filled
  * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
  *
- * Also note that when calling DOMCTL_setvcpucontext and VCPU_initialise
+ * Also note that when calling DOMCTL_setvcpucontext and VCPUOP_initialise
  * for HVM and PVH guests, not all information in this structure is updated:
  *
  * - For HVM guests, the structures read include: fpu_ctxt (if
-- 
2.11.0


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

[PATCH for-next 2/8] x86/domain: factor out pv_vcpu_initialise

[Wei Liu]

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c | 65 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 90e2b1f82a..96c777c771 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -387,37 +387,10 @@ int switch_compat(struct domain *d)
     return rc;
 }
 
-int vcpu_initialise(struct vcpu *v)
+static int pv_vcpu_initialise(struct vcpu *v)
 {
     struct domain *d = v->domain;
-    int rc;
-
-    v->arch.flags = TF_kernel_mode;
-
-    rc = mapcache_vcpu_init(v);
-    if ( rc )
-        return rc;
-
-    if ( !is_idle_domain(d) )
-    {
-        paging_vcpu_init(v);
-
-        if ( (rc = vcpu_init_fpu(v)) != 0 )
-            return rc;
-
-        vmce_init_vcpu(v);
-    }
-    else if ( (rc = xstate_alloc_save_area(v)) != 0 )
-        return rc;
-
-    spin_lock_init(&v->arch.vpmu.vpmu_lock);
-
-    if ( is_hvm_domain(d) )
-    {
-        rc = hvm_vcpu_initialise(v);
-        goto done;
-    }
-
+    int rc = 0;
 
     spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
 
@@ -458,7 +431,41 @@ int vcpu_initialise(struct vcpu *v)
             goto done;
         }
     }
+
  done:
+    return rc;
+}
+
+int vcpu_initialise(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+    int rc;
+
+    v->arch.flags = TF_kernel_mode;
+
+    rc = mapcache_vcpu_init(v);
+    if ( rc )
+        return rc;
+
+    if ( !is_idle_domain(d) )
+    {
+        paging_vcpu_init(v);
+
+        if ( (rc = vcpu_init_fpu(v)) != 0 )
+            return rc;
+
+        vmce_init_vcpu(v);
+    }
+    else if ( (rc = xstate_alloc_save_area(v)) != 0 )
+        return rc;
+
+    spin_lock_init(&v->arch.vpmu.vpmu_lock);
+
+    if ( is_hvm_domain(d) )
+        rc = hvm_vcpu_initialise(v);
+    else
+        rc = pv_vcpu_initialise(v);
+
     if ( rc )
     {
         vcpu_destroy_fpu(v);
-- 
2.11.0


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

[PATCH for-next 3/8] x86/domain: factor out pv_vcpu_destroy

[Wei Liu]

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 96c777c771..ddebff6187 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -479,17 +479,22 @@ int vcpu_initialise(struct vcpu *v)
     return rc;
 }
 
-void vcpu_destroy(struct vcpu *v)
+static void pv_vcpu_destroy(struct vcpu *v)
 {
-    xfree(v->arch.vm_event);
-    v->arch.vm_event = NULL;
-
     if ( is_pv_32bit_vcpu(v) )
     {
         free_compat_arg_xlat(v);
         release_compat_l4(v);
     }
 
+    xfree(v->arch.pv_vcpu.trap_ctxt);
+}
+
+void vcpu_destroy(struct vcpu *v)
+{
+    xfree(v->arch.vm_event);
+    v->arch.vm_event = NULL;
+
     vcpu_destroy_fpu(v);
 
     if ( !is_idle_domain(v->domain) )
@@ -498,8 +503,8 @@ void vcpu_destroy(struct vcpu *v)
     if ( is_hvm_vcpu(v) )
         hvm_vcpu_destroy(v);
     else
-        xfree(v->arch.pv_vcpu.trap_ctxt);
-}
+        pv_vcpu_destroy(v);
+ }
 
 static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 {
-- 
2.11.0


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

[PATCH for-next 4/8] x86/domain: push some code down to hvm_domain_initialise

[Wei Liu]

We want to have a single entry point to initialise hvm guest.  The
timing to set hap bit and create per domain mapping is deferred, but
that's not a problem.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c         | 11 ++---------
 xen/arch/x86/hvm/hvm.c        | 25 +++++++++++++++++--------
 xen/include/asm-x86/hvm/hvm.h |  2 +-
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ddebff6187..af060d8239 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -587,14 +587,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         d->arch.emulation_flags = emflags;
     }
 
-    if ( is_hvm_domain(d) )
-    {
-        d->arch.hvm_domain.hap_enabled =
-            hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
-
-        rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
-    }
-    else if ( is_idle_domain(d) )
+    if ( is_idle_domain(d) )
         rc = 0;
     else
     {
@@ -663,7 +656,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
 
     if ( is_hvm_domain(d) )
     {
-        if ( (rc = hvm_domain_initialise(d)) != 0 )
+        if ( (rc = hvm_domain_initialise(d, domcr_flags)) != 0 )
             goto fail;
     }
     else
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f50d15ff50..7fc49bb03d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -595,7 +595,7 @@ static int hvm_print_line(
     return X86EMUL_OKAY;
 }
 
-int hvm_domain_initialise(struct domain *d)
+int hvm_domain_initialise(struct domain *d, unsigned int domcr_flags)
 {
     unsigned int nr_gsis;
     int rc;
@@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d)
 
     hvm_init_cacheattr_region_list(d);
 
-    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
+    d->arch.hvm_domain.hap_enabled =
+        hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
+
+    rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
     if ( rc != 0 )
         goto fail0;
 
+    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
+    if ( rc != 0 )
+        goto fail1;
+
     nr_gsis = is_hardware_domain(d) ? nr_irqs_gsi : NR_HVM_DOMU_IRQS;
     d->arch.hvm_domain.pl_time = xzalloc(struct pl_time);
     d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
@@ -629,7 +636,7 @@ int hvm_domain_initialise(struct domain *d)
     rc = -ENOMEM;
     if ( !d->arch.hvm_domain.pl_time || !d->arch.hvm_domain.irq ||
          !d->arch.hvm_domain.params  || !d->arch.hvm_domain.io_handler )
-        goto fail1;
+        goto fail2;
 
     /* Set the number of GSIs */
     hvm_domain_irq(d)->nr_gsis = nr_gsis;
@@ -647,7 +654,7 @@ int hvm_domain_initialise(struct domain *d)
         if ( d->arch.hvm_domain.io_bitmap == NULL )
         {
             rc = -ENOMEM;
-            goto fail1;
+            goto fail2;
         }
         memset(d->arch.hvm_domain.io_bitmap, ~0, HVM_IOBITMAP_SIZE);
     }
@@ -666,7 +673,7 @@ int hvm_domain_initialise(struct domain *d)
 
     rc = vioapic_init(d);
     if ( rc != 0 )
-        goto fail1;
+        goto fail2;
 
     stdvga_init(d);
 
@@ -679,21 +686,23 @@ int hvm_domain_initialise(struct domain *d)
 
     rc = hvm_funcs.domain_initialise(d);
     if ( rc != 0 )
-        goto fail2;
+        goto fail3;
 
     return 0;
 
- fail2:
+ fail3:
     rtc_deinit(d);
     stdvga_deinit(d);
     vioapic_deinit(d);
- fail1:
+ fail2:
     if ( is_hardware_domain(d) )
         xfree(d->arch.hvm_domain.io_bitmap);
     xfree(d->arch.hvm_domain.io_handler);
     xfree(d->arch.hvm_domain.params);
     xfree(d->arch.hvm_domain.pl_time);
     xfree(d->arch.hvm_domain.irq);
+ fail1:
+    destroy_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0);
  fail0:
     hvm_destroy_cacheattr_region_list(d);
     return rc;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 7a85b2e3b5..d3fef9ca52 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -236,7 +236,7 @@ extern s8 hvm_port80_allowed;
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);
 
-int hvm_domain_initialise(struct domain *d);
+int hvm_domain_initialise(struct domain *d, unsigned int domcr_flags);
 void hvm_domain_relinquish_resources(struct domain *d);
 void hvm_domain_destroy(struct domain *d);
 void hvm_domain_soft_reset(struct domain *d);
-- 
2.11.0


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

[PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Wei Liu]

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index af060d8239..05885a103d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -527,6 +527,12 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
     return true;
 }
 
+static void pv_domain_destroy(struct domain *d)
+{
+    xfree(d->arch.pv_domain.cpuidmasks);
+    free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+}
+
 int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
@@ -704,10 +710,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         paging_final_teardown(d);
     free_perdomain_mappings(d);
     if ( is_pv_domain(d) )
-    {
-        xfree(d->arch.pv_domain.cpuidmasks);
-        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
-    }
+        pv_domain_destroy(d);
+
     return rc;
 }
 
@@ -727,10 +731,7 @@ void arch_domain_destroy(struct domain *d)
 
     free_perdomain_mappings(d);
     if ( is_pv_domain(d) )
-    {
-        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
-        xfree(d->arch.pv_domain.cpuidmasks);
-    }
+        pv_domain_destroy(d);
 
     free_xenheap_page(d->shared_info);
     cleanup_domain_irq_mapping(d);
-- 
2.11.0


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

[PATCH for-next 6/8] x86/domain: factor out pv_domain_initialise

[Wei Liu]

Lump everything PV related in arch_domain_create into
pv_domain_initialise.

Though domcr_flags is not necessary, the new function is given one to
match hvm counterpart.

Since it cleans up after itself there is no need to clean up in
arch_domain_create in case of failure.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c | 97 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 05885a103d..ed16adf77a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -533,6 +533,58 @@ static void pv_domain_destroy(struct domain *d)
     free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
 }
 
+static int pv_domain_initialise(struct domain *d, unsigned int domcr_flags)
+{
+    static const struct arch_csw pv_csw = {
+        .from = paravirt_ctxt_switch_from,
+        .to   = paravirt_ctxt_switch_to,
+        .tail = continue_nonidle_domain,
+    };
+    int rc = -ENOMEM;
+
+    d->arch.pv_domain.gdt_ldt_l1tab =
+        alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
+    if ( !d->arch.pv_domain.gdt_ldt_l1tab )
+        goto fail;
+    clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
+
+    if ( levelling_caps & ~LCAP_faulting )
+    {
+        d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
+        if ( !d->arch.pv_domain.cpuidmasks )
+            goto fail;
+        *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
+    }
+
+    rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
+                                  GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
+                                  NULL, NULL);
+    if ( rc )
+        goto fail;
+
+    d->arch.ctxt_switch = &pv_csw;
+
+    /* 64-bit PV guest by default. */
+    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+
+    return 0;
+
+fail:
+    if ( d->arch.pv_domain.gdt_ldt_l1tab )
+    {
+        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+        d->arch.pv_domain.gdt_ldt_l1tab = NULL;
+    }
+
+    if ( d->arch.pv_domain.cpuidmasks )
+    {
+        xfree(d->arch.pv_domain.cpuidmasks);
+        d->arch.pv_domain.cpuidmasks = NULL;
+    }
+
+    return rc;
+}
+
 int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
@@ -593,30 +645,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         d->arch.emulation_flags = emflags;
     }
 
-    if ( is_idle_domain(d) )
-        rc = 0;
-    else
-    {
-        d->arch.pv_domain.gdt_ldt_l1tab =
-            alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
-        if ( !d->arch.pv_domain.gdt_ldt_l1tab )
-            goto fail;
-        clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
-
-        if ( levelling_caps & ~LCAP_faulting )
-        {
-            d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
-            if ( !d->arch.pv_domain.cpuidmasks )
-                goto fail;
-            *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
-        }
-
-        rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
-                                      GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
-                                      NULL, NULL);
-    }
-    if ( rc )
-        goto fail;
+    rc = 0;
 
     mapcache_domain_init(d);
 
@@ -665,23 +694,20 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         if ( (rc = hvm_domain_initialise(d, domcr_flags)) != 0 )
             goto fail;
     }
-    else
+    else if ( is_idle_domain(d) )
     {
-        static const struct arch_csw pv_csw = {
-            .from = paravirt_ctxt_switch_from,
-            .to   = paravirt_ctxt_switch_to,
-            .tail = continue_nonidle_domain,
-        };
         static const struct arch_csw idle_csw = {
             .from = paravirt_ctxt_switch_from,
             .to   = paravirt_ctxt_switch_to,
             .tail = continue_idle_domain,
         };
 
-        d->arch.ctxt_switch = is_idle_domain(d) ? &idle_csw : &pv_csw;
-
-        /* 64-bit PV guest by default. */
-        d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+        d->arch.ctxt_switch = &idle_csw;
+    }
+    else
+    {
+        if ( (rc = pv_domain_initialise(d, domcr_flags)) != 0 )
+            goto fail;
     }
 
     /* initialize default tsc behavior in case tools don't */
@@ -709,9 +735,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( paging_initialised )
         paging_final_teardown(d);
     free_perdomain_mappings(d);
-    if ( is_pv_domain(d) )
-        pv_domain_destroy(d);
-
     return rc;
 }
 
-- 
2.11.0


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

[PATCH for-next 7/8] x86/domain: move PV specific code to pv/domain.c

[Wei Liu]

Move all the PV specific code along with the supporting code to
pv/domain.c.

This in turn requires exporting a few functions in header files. Export
paravirt context switch functions in domain.h and create pv/pv.h for
the rest.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c        | 250 +--------------------------------------
 xen/arch/x86/pv/Makefile     |   1 +
 xen/arch/x86/pv/domain.c     | 270 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/domain.h |   3 +
 xen/include/asm-x86/pv/pv.h  |  29 +++++
 5 files changed, 306 insertions(+), 247 deletions(-)
 create mode 100644 xen/arch/x86/pv/domain.c
 create mode 100644 xen/include/asm-x86/pv/pv.h

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ed16adf77a..4a2363fc96 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -63,6 +63,7 @@
 #include <xen/iommu.h>
 #include <compat/vcpu.h>
 #include <asm/psr.h>
+#include <asm/pv/pv.h>
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 
@@ -70,9 +71,6 @@ static void default_idle(void);
 void (*pm_idle) (void) __read_mostly = default_idle;
 void (*dead_idle) (void) __read_mostly = default_dead_idle;
 
-static void paravirt_ctxt_switch_from(struct vcpu *v);
-static void paravirt_ctxt_switch_to(struct vcpu *v);
-
 static void default_idle(void)
 {
     local_irq_disable();
@@ -145,13 +143,6 @@ static void noreturn continue_idle_domain(struct vcpu *v)
     reset_stack_and_jump(idle_loop);
 }
 
-static void noreturn continue_nonidle_domain(struct vcpu *v)
-{
-    check_wakeup_from_wait();
-    mark_regs_dirty(guest_cpu_user_regs());
-    reset_stack_and_jump(ret_from_intr);
-}
-
 void dump_pageframe_info(struct domain *d)
 {
     struct page_info *page;
@@ -313,129 +304,6 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_page(v);
 }
 
-static int setup_compat_l4(struct vcpu *v)
-{
-    struct page_info *pg;
-    l4_pgentry_t *l4tab;
-
-    pg = alloc_domheap_page(v->domain, MEMF_no_owner);
-    if ( pg == NULL )
-        return -ENOMEM;
-
-    /* This page needs to look like a pagetable so that it can be shadowed */
-    pg->u.inuse.type_info = PGT_l4_page_table|PGT_validated|1;
-
-    l4tab = __map_domain_page(pg);
-    clear_page(l4tab);
-    init_guest_l4_table(l4tab, v->domain, 1);
-    unmap_domain_page(l4tab);
-
-    v->arch.guest_table = pagetable_from_page(pg);
-    v->arch.guest_table_user = v->arch.guest_table;
-
-    return 0;
-}
-
-static void release_compat_l4(struct vcpu *v)
-{
-    free_domheap_page(pagetable_get_page(v->arch.guest_table));
-    v->arch.guest_table = pagetable_null();
-    v->arch.guest_table_user = pagetable_null();
-}
-
-int switch_compat(struct domain *d)
-{
-    struct vcpu *v;
-    int rc;
-
-    if ( is_hvm_domain(d) || d->tot_pages != 0 )
-        return -EACCES;
-    if ( is_pv_32bit_domain(d) )
-        return 0;
-
-    d->arch.has_32bit_shinfo = 1;
-    if ( is_pv_domain(d) )
-        d->arch.is_32bit_pv = 1;
-
-    for_each_vcpu( d, v )
-    {
-        rc = setup_compat_arg_xlat(v);
-        if ( !rc )
-            rc = setup_compat_l4(v);
-
-        if ( rc )
-            goto undo_and_fail;
-    }
-
-    domain_set_alloc_bitsize(d);
-    recalculate_cpuid_policy(d);
-
-    d->arch.x87_fip_width = 4;
-
-    return 0;
-
- undo_and_fail:
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
-    for_each_vcpu( d, v )
-    {
-        free_compat_arg_xlat(v);
-
-        if ( !pagetable_is_null(v->arch.guest_table) )
-            release_compat_l4(v);
-    }
-
-    return rc;
-}
-
-static int pv_vcpu_initialise(struct vcpu *v)
-{
-    struct domain *d = v->domain;
-    int rc = 0;
-
-    spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
-
-    if ( !is_idle_domain(d) )
-    {
-        rc = create_perdomain_mapping(d, GDT_VIRT_START(v),
-                                      1 << GDT_LDT_VCPU_SHIFT,
-                                      d->arch.pv_domain.gdt_ldt_l1tab, NULL);
-        if ( rc )
-            goto done;
-
-        BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
-                     PAGE_SIZE);
-        v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
-                                                  NR_VECTORS);
-        if ( !v->arch.pv_vcpu.trap_ctxt )
-        {
-            rc = -ENOMEM;
-            goto done;
-        }
-
-        /* PV guests by default have a 100Hz ticker. */
-        v->periodic_period = MILLISECS(10);
-    }
-    else
-        v->arch.cr3 = __pa(idle_pg_table);
-
-    v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
-
-    if ( is_pv_32bit_domain(d) )
-    {
-        if ( (rc = setup_compat_arg_xlat(v)) )
-            goto done;
-
-        if ( (rc = setup_compat_l4(v)) )
-        {
-            free_compat_arg_xlat(v);
-            goto done;
-        }
-    }
-
- done:
-    return rc;
-}
-
 int vcpu_initialise(struct vcpu *v)
 {
     struct domain *d = v->domain;
@@ -479,17 +347,6 @@ int vcpu_initialise(struct vcpu *v)
     return rc;
 }
 
-static void pv_vcpu_destroy(struct vcpu *v)
-{
-    if ( is_pv_32bit_vcpu(v) )
-    {
-        free_compat_arg_xlat(v);
-        release_compat_l4(v);
-    }
-
-    xfree(v->arch.pv_vcpu.trap_ctxt);
-}
-
 void vcpu_destroy(struct vcpu *v)
 {
     xfree(v->arch.vm_event);
@@ -527,64 +384,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
     return true;
 }
 
-static void pv_domain_destroy(struct domain *d)
-{
-    xfree(d->arch.pv_domain.cpuidmasks);
-    free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
-}
-
-static int pv_domain_initialise(struct domain *d, unsigned int domcr_flags)
-{
-    static const struct arch_csw pv_csw = {
-        .from = paravirt_ctxt_switch_from,
-        .to   = paravirt_ctxt_switch_to,
-        .tail = continue_nonidle_domain,
-    };
-    int rc = -ENOMEM;
-
-    d->arch.pv_domain.gdt_ldt_l1tab =
-        alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
-    if ( !d->arch.pv_domain.gdt_ldt_l1tab )
-        goto fail;
-    clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
-
-    if ( levelling_caps & ~LCAP_faulting )
-    {
-        d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
-        if ( !d->arch.pv_domain.cpuidmasks )
-            goto fail;
-        *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
-    }
-
-    rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
-                                  GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
-                                  NULL, NULL);
-    if ( rc )
-        goto fail;
-
-    d->arch.ctxt_switch = &pv_csw;
-
-    /* 64-bit PV guest by default. */
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
-
-    return 0;
-
-fail:
-    if ( d->arch.pv_domain.gdt_ldt_l1tab )
-    {
-        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
-        d->arch.pv_domain.gdt_ldt_l1tab = NULL;
-    }
-
-    if ( d->arch.pv_domain.cpuidmasks )
-    {
-        xfree(d->arch.pv_domain.cpuidmasks);
-        d->arch.pv_domain.cpuidmasks = NULL;
-    }
-
-    return rc;
-}
-
 int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
@@ -862,49 +661,6 @@ int arch_domain_soft_reset(struct domain *d)
     return ret;
 }
 
-/*
- * These are the masks of CR4 bits (subject to hardware availability) which a
- * PV guest may not legitimiately attempt to modify.
- */
-static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
-
-static int __init init_pv_cr4_masks(void)
-{
-    unsigned long common_mask = ~X86_CR4_TSD;
-
-    /*
-     * All PV guests may attempt to modify TSD, DE and OSXSAVE.
-     */
-    if ( cpu_has_de )
-        common_mask &= ~X86_CR4_DE;
-    if ( cpu_has_xsave )
-        common_mask &= ~X86_CR4_OSXSAVE;
-
-    pv_cr4_mask = compat_pv_cr4_mask = common_mask;
-
-    /*
-     * 64bit PV guests may attempt to modify FSGSBASE.
-     */
-    if ( cpu_has_fsgsbase )
-        pv_cr4_mask &= ~X86_CR4_FSGSBASE;
-
-    return 0;
-}
-__initcall(init_pv_cr4_masks);
-
-unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
-{
-    unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
-    unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
-
-    if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
-        printk(XENLOG_G_WARNING
-               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
-               current->domain->domain_id, v, hv_cr4, guest_cr4);
-
-    return (hv_cr4 & mask) | (guest_cr4 & ~mask);
-}
-
 #define xen_vcpu_guest_context vcpu_guest_context
 #define fpu_ctxt fpu_ctxt.x
 CHECK_FIELD_(struct, vcpu_guest_context, fpu_ctxt);
@@ -1917,7 +1673,7 @@ static void save_segments(struct vcpu *v)
 
 #define switch_kernel_stack(v) ((void)0)
 
-static void paravirt_ctxt_switch_from(struct vcpu *v)
+void paravirt_ctxt_switch_from(struct vcpu *v)
 {
     save_segments(v);
 
@@ -1931,7 +1687,7 @@ static void paravirt_ctxt_switch_from(struct vcpu *v)
         write_debugreg(7, 0);
 }
 
-static void paravirt_ctxt_switch_to(struct vcpu *v)
+void paravirt_ctxt_switch_to(struct vcpu *v)
 {
     unsigned long cr4;
 
diff --git a/xen/arch/x86/pv/Makefile b/xen/arch/x86/pv/Makefile
index ea94599438..2737824e81 100644
--- a/xen/arch/x86/pv/Makefile
+++ b/xen/arch/x86/pv/Makefile
@@ -1,2 +1,3 @@
 obj-y += hypercall.o
 obj-bin-y += dom0_build.init.o
+obj-y += domain.o
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
new file mode 100644
index 0000000000..0b82ee2bbc
--- /dev/null
+++ b/xen/arch/x86/pv/domain.c
@@ -0,0 +1,270 @@
+/******************************************************************************
+ * arch/x86/pv/domain.c
+ *
+ * PV-specific domain handling
+ */
+
+/*
+ *  Copyright (C) 1995  Linus Torvalds
+ *
+ *  Pentium III FXSR, SSE support
+ *  Gareth Hughes <gareth@valinux.com>, May 2000
+ */
+
+
+#include <xen/domain_page.h>
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/sched.h>
+
+static void noreturn continue_nonidle_domain(struct vcpu *v)
+{
+    check_wakeup_from_wait();
+    mark_regs_dirty(guest_cpu_user_regs());
+    reset_stack_and_jump(ret_from_intr);
+}
+
+static int setup_compat_l4(struct vcpu *v)
+{
+    struct page_info *pg;
+    l4_pgentry_t *l4tab;
+
+    pg = alloc_domheap_page(v->domain, MEMF_no_owner);
+    if ( pg == NULL )
+        return -ENOMEM;
+
+    /* This page needs to look like a pagetable so that it can be shadowed */
+    pg->u.inuse.type_info = PGT_l4_page_table|PGT_validated|1;
+
+    l4tab = __map_domain_page(pg);
+    clear_page(l4tab);
+    init_guest_l4_table(l4tab, v->domain, 1);
+    unmap_domain_page(l4tab);
+
+    v->arch.guest_table = pagetable_from_page(pg);
+    v->arch.guest_table_user = v->arch.guest_table;
+
+    return 0;
+}
+
+static void release_compat_l4(struct vcpu *v)
+{
+    free_domheap_page(pagetable_get_page(v->arch.guest_table));
+    v->arch.guest_table = pagetable_null();
+    v->arch.guest_table_user = pagetable_null();
+}
+
+int switch_compat(struct domain *d)
+{
+    struct vcpu *v;
+    int rc;
+
+    if ( is_hvm_domain(d) || d->tot_pages != 0 )
+        return -EACCES;
+    if ( is_pv_32bit_domain(d) )
+        return 0;
+
+    d->arch.has_32bit_shinfo = 1;
+    if ( is_pv_domain(d) )
+        d->arch.is_32bit_pv = 1;
+
+    for_each_vcpu( d, v )
+    {
+        rc = setup_compat_arg_xlat(v);
+        if ( !rc )
+            rc = setup_compat_l4(v);
+
+        if ( rc )
+            goto undo_and_fail;
+    }
+
+    domain_set_alloc_bitsize(d);
+    recalculate_cpuid_policy(d);
+
+    d->arch.x87_fip_width = 4;
+
+    return 0;
+
+ undo_and_fail:
+    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+    for_each_vcpu( d, v )
+    {
+        free_compat_arg_xlat(v);
+
+        if ( !pagetable_is_null(v->arch.guest_table) )
+            release_compat_l4(v);
+    }
+
+    return rc;
+}
+
+int pv_vcpu_initialise(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+    int rc = 0;
+
+    spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
+
+    if ( !is_idle_domain(d) )
+    {
+        rc = create_perdomain_mapping(d, GDT_VIRT_START(v),
+                                      1 << GDT_LDT_VCPU_SHIFT,
+                                      d->arch.pv_domain.gdt_ldt_l1tab, NULL);
+        if ( rc )
+            goto done;
+
+        BUILD_BUG_ON(NR_VECTORS * sizeof(*v->arch.pv_vcpu.trap_ctxt) >
+                     PAGE_SIZE);
+        v->arch.pv_vcpu.trap_ctxt = xzalloc_array(struct trap_info,
+                                                  NR_VECTORS);
+        if ( !v->arch.pv_vcpu.trap_ctxt )
+        {
+            rc = -ENOMEM;
+            goto done;
+        }
+
+        /* PV guests by default have a 100Hz ticker. */
+        v->periodic_period = MILLISECS(10);
+    }
+    else
+        v->arch.cr3 = __pa(idle_pg_table);
+
+    v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+
+    if ( is_pv_32bit_domain(d) )
+    {
+        if ( (rc = setup_compat_arg_xlat(v)) )
+            goto done;
+
+        if ( (rc = setup_compat_l4(v)) )
+        {
+            free_compat_arg_xlat(v);
+            goto done;
+        }
+    }
+
+ done:
+    return rc;
+}
+
+void pv_vcpu_destroy(struct vcpu *v)
+{
+    if ( is_pv_32bit_vcpu(v) )
+    {
+        free_compat_arg_xlat(v);
+        release_compat_l4(v);
+    }
+
+    xfree(v->arch.pv_vcpu.trap_ctxt);
+}
+
+void pv_domain_destroy(struct domain *d)
+{
+    xfree(d->arch.pv_domain.cpuidmasks);
+    free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+}
+
+int pv_domain_initialise(struct domain *d, unsigned int domcr_flags)
+{
+    static const struct arch_csw pv_csw = {
+        .from = paravirt_ctxt_switch_from,
+        .to   = paravirt_ctxt_switch_to,
+        .tail = continue_nonidle_domain,
+    };
+    int rc = -ENOMEM;
+
+    d->arch.pv_domain.gdt_ldt_l1tab =
+        alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
+    if ( !d->arch.pv_domain.gdt_ldt_l1tab )
+        goto fail;
+    clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
+
+    if ( levelling_caps & ~LCAP_faulting )
+    {
+        d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
+        if ( !d->arch.pv_domain.cpuidmasks )
+            goto fail;
+        *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
+    }
+
+    rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
+                                  GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
+                                  NULL, NULL);
+    if ( rc )
+        goto fail;
+
+    d->arch.ctxt_switch = &pv_csw;
+
+    /* 64-bit PV guest by default. */
+    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+
+    return 0;
+
+fail:
+    if ( d->arch.pv_domain.gdt_ldt_l1tab )
+    {
+        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
+        d->arch.pv_domain.gdt_ldt_l1tab = NULL;
+    }
+
+    if ( d->arch.pv_domain.cpuidmasks )
+    {
+        xfree(d->arch.pv_domain.cpuidmasks);
+        d->arch.pv_domain.cpuidmasks = NULL;
+    }
+
+    return rc;
+}
+
+/*
+ * These are the masks of CR4 bits (subject to hardware availability) which a
+ * PV guest may not legitimiately attempt to modify.
+ */
+static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
+
+static int __init init_pv_cr4_masks(void)
+{
+    unsigned long common_mask = ~X86_CR4_TSD;
+
+    /*
+     * All PV guests may attempt to modify TSD, DE and OSXSAVE.
+     */
+    if ( cpu_has_de )
+        common_mask &= ~X86_CR4_DE;
+    if ( cpu_has_xsave )
+        common_mask &= ~X86_CR4_OSXSAVE;
+
+    pv_cr4_mask = compat_pv_cr4_mask = common_mask;
+
+    /*
+     * 64bit PV guests may attempt to modify FSGSBASE.
+     */
+    if ( cpu_has_fsgsbase )
+        pv_cr4_mask &= ~X86_CR4_FSGSBASE;
+
+    return 0;
+}
+__initcall(init_pv_cr4_masks);
+
+unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
+{
+    unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
+    unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
+
+    if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
+        printk(XENLOG_G_WARNING
+               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
+               current->domain->domain_id, v, hv_cr4, guest_cr4);
+
+    return (hv_cr4 & mask) | (guest_cr4 & ~mask);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 6ab987f231..e6262178e8 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -74,6 +74,9 @@ int mapcache_domain_init(struct domain *);
 int mapcache_vcpu_init(struct vcpu *);
 void mapcache_override_current(struct vcpu *);
 
+void paravirt_ctxt_switch_from(struct vcpu *v);
+void paravirt_ctxt_switch_to(struct vcpu *v);
+
 /* x86/64: toggle guest between kernel and user modes. */
 void toggle_guest_mode(struct vcpu *);
 
diff --git a/xen/include/asm-x86/pv/pv.h b/xen/include/asm-x86/pv/pv.h
new file mode 100644
index 0000000000..ba2d054d08
--- /dev/null
+++ b/xen/include/asm-x86/pv/pv.h
@@ -0,0 +1,29 @@
+/*
+ * pv/pv.h
+ *
+ * PV guest interface definitions
+ *
+ * Copyright (C) 2017 Wei Liu <wei.liu2@citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __X86_PV_H__
+#define __X86_PV_H__
+
+int pv_vcpu_initialise(struct vcpu *v);
+void pv_vcpu_destroy(struct vcpu *v);
+void pv_domain_destroy(struct domain *d);
+int pv_domain_initialise(struct domain *d, unsigned int domcr_flags);
+
+#endif	/* __X86_PV_H__ */
-- 
2.11.0


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

[PATCH for-next 8/8] x86/domain: move HVM specific code to hvm/domain.c

[Wei Liu]

There is only one function arch_set_info_hvm_guest is moved. The
check_segment function is also moved since arch_set_info_hvm_guest is
the only user.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/domain.c     | 291 -----------------------------------------
 xen/arch/x86/hvm/Makefile |   1 +
 xen/arch/x86/hvm/domain.c | 322 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 323 insertions(+), 291 deletions(-)
 create mode 100644 xen/arch/x86/hvm/domain.c

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 4a2363fc96..80a86e1ba2 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1060,297 +1060,6 @@ int arch_set_info_guest(
 #undef c
 }
 
-static inline int check_segment(struct segment_register *reg,
-                                enum x86_segment seg)
-{
-
-    if ( reg->attr.fields.pad != 0 )
-    {
-        gprintk(XENLOG_ERR, "Segment attribute bits 12-15 are not zero\n");
-        return -EINVAL;
-    }
-
-    if ( reg->attr.bytes == 0 )
-    {
-        if ( seg != x86_seg_ds && seg != x86_seg_es )
-        {
-            gprintk(XENLOG_ERR, "Null selector provided for CS, SS or TR\n");
-            return -EINVAL;
-        }
-        return 0;
-    }
-
-    if ( seg == x86_seg_tr )
-    {
-        if ( reg->attr.fields.s )
-        {
-            gprintk(XENLOG_ERR, "Code or data segment provided for TR\n");
-            return -EINVAL;
-        }
-
-        if ( reg->attr.fields.type != SYS_DESC_tss_busy )
-        {
-            gprintk(XENLOG_ERR, "Non-32-bit-TSS segment provided for TR\n");
-            return -EINVAL;
-        }
-    }
-    else if ( !reg->attr.fields.s )
-    {
-        gprintk(XENLOG_ERR,
-                "System segment provided for a code or data segment\n");
-        return -EINVAL;
-    }
-
-    if ( !reg->attr.fields.p )
-    {
-        gprintk(XENLOG_ERR, "Non-present segment provided\n");
-        return -EINVAL;
-    }
-
-    if ( seg == x86_seg_cs && !(reg->attr.fields.type & 0x8) )
-    {
-        gprintk(XENLOG_ERR, "Non-code segment provided for CS\n");
-        return -EINVAL;
-    }
-
-    if ( seg == x86_seg_ss &&
-         ((reg->attr.fields.type & 0x8) || !(reg->attr.fields.type & 0x2)) )
-    {
-        gprintk(XENLOG_ERR, "Non-writeable segment provided for SS\n");
-        return -EINVAL;
-    }
-
-    if ( reg->attr.fields.s && seg != x86_seg_ss && seg != x86_seg_cs &&
-         (reg->attr.fields.type & 0x8) && !(reg->attr.fields.type & 0x2) )
-    {
-        gprintk(XENLOG_ERR, "Non-readable segment provided for DS or ES\n");
-        return -EINVAL;
-    }
-
-    return 0;
-}
-
-/* Called by VCPUOP_initialise for HVM guests. */
-int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
-{
-    struct cpu_user_regs *uregs = &v->arch.user_regs;
-    struct segment_register cs, ds, ss, es, tr;
-    const char *errstr;
-    int rc;
-
-    if ( ctx->pad != 0 )
-        return -EINVAL;
-
-    switch ( ctx->mode )
-    {
-    default:
-        return -EINVAL;
-
-    case VCPU_HVM_MODE_32B:
-    {
-        const struct vcpu_hvm_x86_32 *regs = &ctx->cpu_regs.x86_32;
-        uint32_t limit;
-
-        if ( ctx->cpu_regs.x86_32.pad1 != 0 ||
-             ctx->cpu_regs.x86_32.pad2[0] != 0 ||
-             ctx->cpu_regs.x86_32.pad2[1] != 0 ||
-             ctx->cpu_regs.x86_32.pad2[2] != 0 )
-            return -EINVAL;
-
-#define SEG(s, r) ({                                                        \
-    s = (struct segment_register){ .base = (r)->s ## _base,                 \
-                                   .limit = (r)->s ## _limit,               \
-                                   .attr.bytes = (r)->s ## _ar };           \
-    /* Set accessed / busy bit for present segments. */                     \
-    if ( s.attr.fields.p )                                                  \
-        s.attr.fields.type |= (x86_seg_##s != x86_seg_tr ? 1 : 2);          \
-    check_segment(&s, x86_seg_ ## s); })
-
-        rc = SEG(cs, regs);
-        rc |= SEG(ds, regs);
-        rc |= SEG(ss, regs);
-        rc |= SEG(es, regs);
-        rc |= SEG(tr, regs);
-#undef SEG
-
-        if ( rc != 0 )
-            return rc;
-
-        /* Basic sanity checks. */
-        limit = cs.limit;
-        if ( cs.attr.fields.g )
-            limit = (limit << 12) | 0xfff;
-        if ( regs->eip > limit )
-        {
-            gprintk(XENLOG_ERR, "EIP (%#08x) outside CS limit (%#08x)\n",
-                    regs->eip, limit);
-            return -EINVAL;
-        }
-
-        if ( ss.attr.fields.dpl != cs.attr.fields.dpl )
-        {
-            gprintk(XENLOG_ERR, "SS.DPL (%u) is different than CS.DPL (%u)\n",
-                    ss.attr.fields.dpl, cs.attr.fields.dpl);
-            return -EINVAL;
-        }
-
-        if ( ds.attr.fields.p && ds.attr.fields.dpl > cs.attr.fields.dpl )
-        {
-            gprintk(XENLOG_ERR, "DS.DPL (%u) is greater than CS.DPL (%u)\n",
-                    ds.attr.fields.dpl, cs.attr.fields.dpl);
-            return -EINVAL;
-        }
-
-        if ( es.attr.fields.p && es.attr.fields.dpl > cs.attr.fields.dpl )
-        {
-            gprintk(XENLOG_ERR, "ES.DPL (%u) is greater than CS.DPL (%u)\n",
-                    es.attr.fields.dpl, cs.attr.fields.dpl);
-            return -EINVAL;
-        }
-
-        if ( (regs->efer & EFER_LMA) && !(regs->efer & EFER_LME) )
-        {
-            gprintk(XENLOG_ERR, "EFER.LMA set without EFER.LME (%#016lx)\n",
-                    regs->efer);
-            return -EINVAL;
-        }
-
-        uregs->rax    = regs->eax;
-        uregs->rcx    = regs->ecx;
-        uregs->rdx    = regs->edx;
-        uregs->rbx    = regs->ebx;
-        uregs->rsp    = regs->esp;
-        uregs->rbp    = regs->ebp;
-        uregs->rsi    = regs->esi;
-        uregs->rdi    = regs->edi;
-        uregs->rip    = regs->eip;
-        uregs->rflags = regs->eflags;
-
-        v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
-        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
-        v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
-        v->arch.hvm_vcpu.guest_efer  = regs->efer;
-    }
-    break;
-
-    case VCPU_HVM_MODE_64B:
-    {
-        const struct vcpu_hvm_x86_64 *regs = &ctx->cpu_regs.x86_64;
-
-        /* Basic sanity checks. */
-        if ( !is_canonical_address(regs->rip) )
-        {
-            gprintk(XENLOG_ERR, "RIP contains a non-canonical address (%#lx)\n",
-                    regs->rip);
-            return -EINVAL;
-        }
-
-        if ( !(regs->cr0 & X86_CR0_PG) )
-        {
-            gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled (%#016lx)\n",
-                    regs->cr0);
-            return -EINVAL;
-        }
-
-        if ( !(regs->cr4 & X86_CR4_PAE) )
-        {
-            gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled (%#016lx)\n",
-                    regs->cr4);
-            return -EINVAL;
-        }
-
-        if ( !(regs->efer & EFER_LME) )
-        {
-            gprintk(XENLOG_ERR, "EFER doesn't have LME enabled (%#016lx)\n",
-                    regs->efer);
-            return -EINVAL;
-        }
-
-        uregs->rax    = regs->rax;
-        uregs->rcx    = regs->rcx;
-        uregs->rdx    = regs->rdx;
-        uregs->rbx    = regs->rbx;
-        uregs->rsp    = regs->rsp;
-        uregs->rbp    = regs->rbp;
-        uregs->rsi    = regs->rsi;
-        uregs->rdi    = regs->rdi;
-        uregs->rip    = regs->rip;
-        uregs->rflags = regs->rflags;
-
-        v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
-        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
-        v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
-        v->arch.hvm_vcpu.guest_efer  = regs->efer;
-
-#define SEG(l, a) (struct segment_register){ .limit = (l), .attr.bytes = (a) }
-        cs = SEG(~0u, 0xa9b); /* 64bit code segment. */
-        ds = ss = es = SEG(~0u, 0xc93);
-        tr = SEG(0x67, 0x8b); /* 64bit TSS (busy). */
-#undef SEG
-    }
-    break;
-
-    }
-
-    if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
-        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
-
-    if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(v, 0) )
-    {
-        gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
-                v->arch.hvm_vcpu.guest_cr[4]);
-        return -EINVAL;
-    }
-
-    errstr = hvm_efer_valid(v, v->arch.hvm_vcpu.guest_efer, -1);
-    if ( errstr )
-    {
-        gprintk(XENLOG_ERR, "Bad EFER value (%#016lx): %s\n",
-               v->arch.hvm_vcpu.guest_efer, errstr);
-        return -EINVAL;
-    }
-
-    hvm_update_guest_cr(v, 0);
-    hvm_update_guest_cr(v, 3);
-    hvm_update_guest_cr(v, 4);
-    hvm_update_guest_efer(v);
-
-    if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
-    {
-        /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
-        struct page_info *page = get_page_from_gfn(v->domain,
-                                 v->arch.hvm_vcpu.guest_cr[3] >> PAGE_SHIFT,
-                                 NULL, P2M_ALLOC);
-        if ( !page )
-        {
-            gprintk(XENLOG_ERR, "Invalid CR3: %#lx\n",
-                    v->arch.hvm_vcpu.guest_cr[3]);
-            return -EINVAL;
-        }
-
-        v->arch.guest_table = pagetable_from_page(page);
-    }
-
-    hvm_set_segment_register(v, x86_seg_cs, &cs);
-    hvm_set_segment_register(v, x86_seg_ds, &ds);
-    hvm_set_segment_register(v, x86_seg_ss, &ss);
-    hvm_set_segment_register(v, x86_seg_es, &es);
-    hvm_set_segment_register(v, x86_seg_tr, &tr);
-
-    /* Sync AP's TSC with BSP's. */
-    v->arch.hvm_vcpu.cache_tsc_offset =
-        v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
-    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
-                             v->domain->arch.hvm_domain.sync_tsc);
-
-    paging_update_paging_modes(v);
-
-    v->is_initialised = 1;
-    set_bit(_VPF_down, &v->pause_flags);
-
-    return 0;
-}
-
 int arch_initialise_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     int rc;
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 0a3d0f4f7e..4dc0773a93 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -4,6 +4,7 @@ subdir-y += vmx
 obj-y += asid.o
 obj-y += dm.o
 obj-bin-y += dom0_build.init.o
+obj-y += domain.o
 obj-y += emulate.o
 obj-y += hpet.o
 obj-y += hvm.o
diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
new file mode 100644
index 0000000000..c3358f4f8d
--- /dev/null
+++ b/xen/arch/x86/hvm/domain.c
@@ -0,0 +1,322 @@
+/******************************************************************************
+ * arch/x86/hvm/domain.c
+ *
+ * HVM-specific domain handling
+ */
+
+/*
+ *  Copyright (C) 1995  Linus Torvalds
+ *
+ *  Pentium III FXSR, SSE support
+ *  Gareth Hughes <gareth@valinux.com>, May 2000
+ */
+
+
+#include <xen/domain_page.h>
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/paging.h>
+#include <xen/sched.h>
+
+#include <public/hvm/hvm_vcpu.h>
+
+static inline int check_segment(struct segment_register *reg,
+                                enum x86_segment seg)
+{
+
+    if ( reg->attr.fields.pad != 0 )
+    {
+        gprintk(XENLOG_ERR, "Segment attribute bits 12-15 are not zero\n");
+        return -EINVAL;
+    }
+
+    if ( reg->attr.bytes == 0 )
+    {
+        if ( seg != x86_seg_ds && seg != x86_seg_es )
+        {
+            gprintk(XENLOG_ERR, "Null selector provided for CS, SS or TR\n");
+            return -EINVAL;
+        }
+        return 0;
+    }
+
+    if ( seg == x86_seg_tr )
+    {
+        if ( reg->attr.fields.s )
+        {
+            gprintk(XENLOG_ERR, "Code or data segment provided for TR\n");
+            return -EINVAL;
+        }
+
+        if ( reg->attr.fields.type != SYS_DESC_tss_busy )
+        {
+            gprintk(XENLOG_ERR, "Non-32-bit-TSS segment provided for TR\n");
+            return -EINVAL;
+        }
+    }
+    else if ( !reg->attr.fields.s )
+    {
+        gprintk(XENLOG_ERR,
+                "System segment provided for a code or data segment\n");
+        return -EINVAL;
+    }
+
+    if ( !reg->attr.fields.p )
+    {
+        gprintk(XENLOG_ERR, "Non-present segment provided\n");
+        return -EINVAL;
+    }
+
+    if ( seg == x86_seg_cs && !(reg->attr.fields.type & 0x8) )
+    {
+        gprintk(XENLOG_ERR, "Non-code segment provided for CS\n");
+        return -EINVAL;
+    }
+
+    if ( seg == x86_seg_ss &&
+         ((reg->attr.fields.type & 0x8) || !(reg->attr.fields.type & 0x2)) )
+    {
+        gprintk(XENLOG_ERR, "Non-writeable segment provided for SS\n");
+        return -EINVAL;
+    }
+
+    if ( reg->attr.fields.s && seg != x86_seg_ss && seg != x86_seg_cs &&
+         (reg->attr.fields.type & 0x8) && !(reg->attr.fields.type & 0x2) )
+    {
+        gprintk(XENLOG_ERR, "Non-readable segment provided for DS or ES\n");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+/* Called by VCPUOP_initialise for HVM guests. */
+int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
+{
+    struct cpu_user_regs *uregs = &v->arch.user_regs;
+    struct segment_register cs, ds, ss, es, tr;
+    const char *errstr;
+    int rc;
+
+    if ( ctx->pad != 0 )
+        return -EINVAL;
+
+    switch ( ctx->mode )
+    {
+    default:
+        return -EINVAL;
+
+    case VCPU_HVM_MODE_32B:
+    {
+        const struct vcpu_hvm_x86_32 *regs = &ctx->cpu_regs.x86_32;
+        uint32_t limit;
+
+        if ( ctx->cpu_regs.x86_32.pad1 != 0 ||
+             ctx->cpu_regs.x86_32.pad2[0] != 0 ||
+             ctx->cpu_regs.x86_32.pad2[1] != 0 ||
+             ctx->cpu_regs.x86_32.pad2[2] != 0 )
+            return -EINVAL;
+
+#define SEG(s, r) ({                                                        \
+    s = (struct segment_register){ .base = (r)->s ## _base,                 \
+                                   .limit = (r)->s ## _limit,               \
+                                   .attr.bytes = (r)->s ## _ar };           \
+    /* Set accessed / busy bit for present segments. */                     \
+    if ( s.attr.fields.p )                                                  \
+        s.attr.fields.type |= (x86_seg_##s != x86_seg_tr ? 1 : 2);          \
+    check_segment(&s, x86_seg_ ## s); })
+
+        rc = SEG(cs, regs);
+        rc |= SEG(ds, regs);
+        rc |= SEG(ss, regs);
+        rc |= SEG(es, regs);
+        rc |= SEG(tr, regs);
+#undef SEG
+
+        if ( rc != 0 )
+            return rc;
+
+        /* Basic sanity checks. */
+        limit = cs.limit;
+        if ( cs.attr.fields.g )
+            limit = (limit << 12) | 0xfff;
+        if ( regs->eip > limit )
+        {
+            gprintk(XENLOG_ERR, "EIP (%#08x) outside CS limit (%#08x)\n",
+                    regs->eip, limit);
+            return -EINVAL;
+        }
+
+        if ( ss.attr.fields.dpl != cs.attr.fields.dpl )
+        {
+            gprintk(XENLOG_ERR, "SS.DPL (%u) is different than CS.DPL (%u)\n",
+                    ss.attr.fields.dpl, cs.attr.fields.dpl);
+            return -EINVAL;
+        }
+
+        if ( ds.attr.fields.p && ds.attr.fields.dpl > cs.attr.fields.dpl )
+        {
+            gprintk(XENLOG_ERR, "DS.DPL (%u) is greater than CS.DPL (%u)\n",
+                    ds.attr.fields.dpl, cs.attr.fields.dpl);
+            return -EINVAL;
+        }
+
+        if ( es.attr.fields.p && es.attr.fields.dpl > cs.attr.fields.dpl )
+        {
+            gprintk(XENLOG_ERR, "ES.DPL (%u) is greater than CS.DPL (%u)\n",
+                    es.attr.fields.dpl, cs.attr.fields.dpl);
+            return -EINVAL;
+        }
+
+        if ( (regs->efer & EFER_LMA) && !(regs->efer & EFER_LME) )
+        {
+            gprintk(XENLOG_ERR, "EFER.LMA set without EFER.LME (%#016lx)\n",
+                    regs->efer);
+            return -EINVAL;
+        }
+
+        uregs->rax    = regs->eax;
+        uregs->rcx    = regs->ecx;
+        uregs->rdx    = regs->edx;
+        uregs->rbx    = regs->ebx;
+        uregs->rsp    = regs->esp;
+        uregs->rbp    = regs->ebp;
+        uregs->rsi    = regs->esi;
+        uregs->rdi    = regs->edi;
+        uregs->rip    = regs->eip;
+        uregs->rflags = regs->eflags;
+
+        v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
+        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
+        v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
+        v->arch.hvm_vcpu.guest_efer  = regs->efer;
+    }
+    break;
+
+    case VCPU_HVM_MODE_64B:
+    {
+        const struct vcpu_hvm_x86_64 *regs = &ctx->cpu_regs.x86_64;
+
+        /* Basic sanity checks. */
+        if ( !is_canonical_address(regs->rip) )
+        {
+            gprintk(XENLOG_ERR, "RIP contains a non-canonical address (%#lx)\n",
+                    regs->rip);
+            return -EINVAL;
+        }
+
+        if ( !(regs->cr0 & X86_CR0_PG) )
+        {
+            gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled (%#016lx)\n",
+                    regs->cr0);
+            return -EINVAL;
+        }
+
+        if ( !(regs->cr4 & X86_CR4_PAE) )
+        {
+            gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled (%#016lx)\n",
+                    regs->cr4);
+            return -EINVAL;
+        }
+
+        if ( !(regs->efer & EFER_LME) )
+        {
+            gprintk(XENLOG_ERR, "EFER doesn't have LME enabled (%#016lx)\n",
+                    regs->efer);
+            return -EINVAL;
+        }
+
+        uregs->rax    = regs->rax;
+        uregs->rcx    = regs->rcx;
+        uregs->rdx    = regs->rdx;
+        uregs->rbx    = regs->rbx;
+        uregs->rsp    = regs->rsp;
+        uregs->rbp    = regs->rbp;
+        uregs->rsi    = regs->rsi;
+        uregs->rdi    = regs->rdi;
+        uregs->rip    = regs->rip;
+        uregs->rflags = regs->rflags;
+
+        v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
+        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
+        v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
+        v->arch.hvm_vcpu.guest_efer  = regs->efer;
+
+#define SEG(l, a) (struct segment_register){ .limit = (l), .attr.bytes = (a) }
+        cs = SEG(~0u, 0xa9b); /* 64bit code segment. */
+        ds = ss = es = SEG(~0u, 0xc93);
+        tr = SEG(0x67, 0x8b); /* 64bit TSS (busy). */
+#undef SEG
+    }
+    break;
+
+    }
+
+    if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
+        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
+
+    if ( v->arch.hvm_vcpu.guest_cr[4] & ~hvm_cr4_guest_valid_bits(v, 0) )
+    {
+        gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
+                v->arch.hvm_vcpu.guest_cr[4]);
+        return -EINVAL;
+    }
+
+    errstr = hvm_efer_valid(v, v->arch.hvm_vcpu.guest_efer, -1);
+    if ( errstr )
+    {
+        gprintk(XENLOG_ERR, "Bad EFER value (%#016lx): %s\n",
+               v->arch.hvm_vcpu.guest_efer, errstr);
+        return -EINVAL;
+    }
+
+    hvm_update_guest_cr(v, 0);
+    hvm_update_guest_cr(v, 3);
+    hvm_update_guest_cr(v, 4);
+    hvm_update_guest_efer(v);
+
+    if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
+    {
+        /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
+        struct page_info *page = get_page_from_gfn(v->domain,
+                                 v->arch.hvm_vcpu.guest_cr[3] >> PAGE_SHIFT,
+                                 NULL, P2M_ALLOC);
+        if ( !page )
+        {
+            gprintk(XENLOG_ERR, "Invalid CR3: %#lx\n",
+                    v->arch.hvm_vcpu.guest_cr[3]);
+            return -EINVAL;
+        }
+
+        v->arch.guest_table = pagetable_from_page(page);
+    }
+
+    hvm_set_segment_register(v, x86_seg_cs, &cs);
+    hvm_set_segment_register(v, x86_seg_ds, &ds);
+    hvm_set_segment_register(v, x86_seg_ss, &ss);
+    hvm_set_segment_register(v, x86_seg_es, &es);
+    hvm_set_segment_register(v, x86_seg_tr, &tr);
+
+    /* Sync AP's TSC with BSP's. */
+    v->arch.hvm_vcpu.cache_tsc_offset =
+        v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
+    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
+                             v->domain->arch.hvm_domain.sync_tsc);
+
+    paging_update_paging_modes(v);
+
+    v->is_initialised = 1;
+    set_bit(_VPF_down, &v->pause_flags);
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.11.0


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

Re: [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Andrew Cooper]

On 10/04/17 14:27, Wei Liu wrote:
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Throughout this series, please make sure you add in proper NULL'ing of
freed data.

While this patch is no functional change at the moment, you have
introduced a latent double-free bug for if (/when) pv_domain_destroy()
gets used on a failed create path.

Please make all of these functions idempotent when breaking them out.

~Andrew

> ---
>  xen/arch/x86/domain.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index af060d8239..05885a103d 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -527,6 +527,12 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>      return true;
>  }
>  
> +static void pv_domain_destroy(struct domain *d)
> +{
> +    xfree(d->arch.pv_domain.cpuidmasks);
> +    free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> +}
> +
>  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                         struct xen_arch_domainconfig *config)
>  {
> @@ -704,10 +710,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>          paging_final_teardown(d);
>      free_perdomain_mappings(d);
>      if ( is_pv_domain(d) )
> -    {
> -        xfree(d->arch.pv_domain.cpuidmasks);
> -        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> -    }
> +        pv_domain_destroy(d);
> +
>      return rc;
>  }
>  
> @@ -727,10 +731,7 @@ void arch_domain_destroy(struct domain *d)
>  
>      free_perdomain_mappings(d);
>      if ( is_pv_domain(d) )
> -    {
> -        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> -        xfree(d->arch.pv_domain.cpuidmasks);
> -    }
> +        pv_domain_destroy(d);
>  
>      free_xenheap_page(d->shared_info);
>      cleanup_domain_irq_mapping(d);


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

Re: [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Wei Liu]

On Mon, Apr 10, 2017 at 04:04:22PM +0100, Andrew Cooper wrote:
> On 10/04/17 14:27, Wei Liu wrote:
> > No functional change.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Throughout this series, please make sure you add in proper NULL'ing of
> freed data.
> 
> While this patch is no functional change at the moment, you have
> introduced a latent double-free bug for if (/when) pv_domain_destroy()
> gets used on a failed create path.
> 

No it won't. I made pv_domain_initialise idempotent.

> Please make all of these functions idempotent when breaking them out.
> 

This is a good point. I can make this one idempotent as well.

Wei.

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

Re: [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Andrew Cooper]

On 10/04/17 16:12, Wei Liu wrote:
> On Mon, Apr 10, 2017 at 04:04:22PM +0100, Andrew Cooper wrote:
>> On 10/04/17 14:27, Wei Liu wrote:
>>> No functional change.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Throughout this series, please make sure you add in proper NULL'ing of
>> freed data.
>>
>> While this patch is no functional change at the moment, you have
>> introduced a latent double-free bug for if (/when) pv_domain_destroy()
>> gets used on a failed create path.
>>
> No it won't. I made pv_domain_initialise idempotent.

Ah - I hadn't spotted that.

My long-term goal is to reorder the init/destroy logic such that init
has no cleanup in it, and any failure comes out and calls destroy at the
top level.

This will avoid us opencoding the teardown logic twice;  we have had
bugs in the past where the two paths are different (the most common of
which is missing cleanup in the destroy path, leading to a memory leak).

~Andrew

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

Re: [PATCH for-next 4/8] x86/domain: push some code down to hvm_domain_initialise

[Andrew Cooper]

On 10/04/17 14:27, Wei Liu wrote:
> We want to have a single entry point to initialise hvm guest.  The
> timing to set hap bit and create per domain mapping is deferred, but
> that's not a problem.
>
> No functional change.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Two observations...

> ---
>  xen/arch/x86/domain.c         | 11 ++---------
>  xen/arch/x86/hvm/hvm.c        | 25 +++++++++++++++++--------
>  xen/include/asm-x86/hvm/hvm.h |  2 +-
>  3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ddebff6187..af060d8239 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -663,7 +656,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>  
>      if ( is_hvm_domain(d) )
>      {
> -        if ( (rc = hvm_domain_initialise(d)) != 0 )
> +        if ( (rc = hvm_domain_initialise(d, domcr_flags)) != 0 )

If we are pushing these values down (which I agree is a good idea),
please can we push xen_arch_domainconfig down as well.

>              goto fail;
>      }
>      else
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f50d15ff50..7fc49bb03d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d)
>  
>      hvm_init_cacheattr_region_list(d);
>  
> -    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> +    d->arch.hvm_domain.hap_enabled =
> +        hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);

We really should fail with -EOPNOTSUPP.

The toolstack already knows whether HAP is available, and nothing good
can come from Xen falling silently back to shadow behind the toolstacks
back.

Probably worth being in a solo patch though.

~Andrew

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

Re: [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Wei Liu]

On Mon, Apr 10, 2017 at 04:16:12PM +0100, Andrew Cooper wrote:
> On 10/04/17 16:12, Wei Liu wrote:
> > On Mon, Apr 10, 2017 at 04:04:22PM +0100, Andrew Cooper wrote:
> >> On 10/04/17 14:27, Wei Liu wrote:
> >>> No functional change.
> >>>
> >>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> Throughout this series, please make sure you add in proper NULL'ing of
> >> freed data.
> >>
> >> While this patch is no functional change at the moment, you have
> >> introduced a latent double-free bug for if (/when) pv_domain_destroy()
> >> gets used on a failed create path.
> >>
> > No it won't. I made pv_domain_initialise idempotent.
> 
> Ah - I hadn't spotted that.
> 
> My long-term goal is to reorder the init/destroy logic such that init
> has no cleanup in it, and any failure comes out and calls destroy at the
> top level.

Just like what we do in libxl? :-)

> 
> This will avoid us opencoding the teardown logic twice;  we have had
> bugs in the past where the two paths are different (the most common of
> which is missing cleanup in the destroy path, leading to a memory leak).
> 

Given there is going to be a lot of code churn, I think the best bet at
the moment is to have the teardown logic twice, with each setting
respective fields to their "NULL" state. Although that's a bit
excessive, but it would prevent accidental memory leak or double free
errors. And then after we are happy with the overall structures of
hypervisor code, we can then audit the code to remove the clean up code
in _init.

What do you think?

Wei.

> ~Andrew

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

Re: [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Andrew Cooper]

On 10/04/17 16:22, Wei Liu wrote:
> On Mon, Apr 10, 2017 at 04:16:12PM +0100, Andrew Cooper wrote:
>> On 10/04/17 16:12, Wei Liu wrote:
>>> On Mon, Apr 10, 2017 at 04:04:22PM +0100, Andrew Cooper wrote:
>>>> On 10/04/17 14:27, Wei Liu wrote:
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>> Throughout this series, please make sure you add in proper NULL'ing of
>>>> freed data.
>>>>
>>>> While this patch is no functional change at the moment, you have
>>>> introduced a latent double-free bug for if (/when) pv_domain_destroy()
>>>> gets used on a failed create path.
>>>>
>>> No it won't. I made pv_domain_initialise idempotent.
>> Ah - I hadn't spotted that.
>>
>> My long-term goal is to reorder the init/destroy logic such that init
>> has no cleanup in it, and any failure comes out and calls destroy at the
>> top level.
> Just like what we do in libxl? :-)
>
>> This will avoid us opencoding the teardown logic twice;  we have had
>> bugs in the past where the two paths are different (the most common of
>> which is missing cleanup in the destroy path, leading to a memory leak).
>>
> Given there is going to be a lot of code churn, I think the best bet at
> the moment is to have the teardown logic twice, with each setting
> respective fields to their "NULL" state. Although that's a bit
> excessive, but it would prevent accidental memory leak or double free
> errors. And then after we are happy with the overall structures of
> hypervisor code, we can then audit the code to remove the clean up code
> in _init.
>
> What do you think?

For now, keep the patches along their current line, but making the
teardown idempotent.  I wasn't trying to lumber you with all the work. ;)

I have a series starting from the common code, but it will require
coordinated changes between all architectures.  This will be far easier
to reason about when all the underlying functions are idempotent.

~Andrew

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

Re: [PATCH for-next 1/8] xen.h: fix comment for vcpu_guest_context

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> Use the correct vcpuop name and delete on trailing white space.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Afaic fine to go in right away (no code change at all). Julien?

Jan


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

Re: [PATCH for-next 2/8] x86/domain: factor out pv_vcpu_initialise

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> +int vcpu_initialise(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    int rc;
> +
> +    v->arch.flags = TF_kernel_mode;
> +
> +    rc = mapcache_vcpu_init(v);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !is_idle_domain(d) )
> +    {
> +        paging_vcpu_init(v);
> +
> +        if ( (rc = vcpu_init_fpu(v)) != 0 )
> +            return rc;
> +
> +        vmce_init_vcpu(v);
> +    }
> +    else if ( (rc = xstate_alloc_save_area(v)) != 0 )
> +        return rc;
> +
> +    spin_lock_init(&v->arch.vpmu.vpmu_lock);
> +
> +    if ( is_hvm_domain(d) )
> +        rc = hvm_vcpu_initialise(v);
> +    else
> +        rc = pv_vcpu_initialise(v);
> +
>      if ( rc )
>      {
>          vcpu_destroy_fpu(v);

Below here v->arch.pv_vcpu.trap_ctxt is being freed, which now also
belongs into the PV function.

Jan


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

Re: [PATCH for-next 3/8] x86/domain: factor out pv_vcpu_destroy

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> @@ -498,8 +503,8 @@ void vcpu_destroy(struct vcpu *v)
>      if ( is_hvm_vcpu(v) )
>          hvm_vcpu_destroy(v);
>      else
> -        xfree(v->arch.pv_vcpu.trap_ctxt);
> -}
> +        pv_vcpu_destroy(v);
> + }

Stray leading blank. Other than that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH for-next 4/8] x86/domain: push some code down to hvm_domain_initialise

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> We want to have a single entry point to initialise hvm guest.  The
> timing to set hap bit and create per domain mapping is deferred, but
> that's not a problem.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/domain.c         | 11 ++---------
>  xen/arch/x86/hvm/hvm.c        | 25 +++++++++++++++++--------
>  xen/include/asm-x86/hvm/hvm.h |  2 +-
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ddebff6187..af060d8239 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -587,14 +587,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>          d->arch.emulation_flags = emflags;
>      }
>  
> -    if ( is_hvm_domain(d) )
> -    {
> -        d->arch.hvm_domain.hap_enabled =
> -            hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> -
> -        rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);

I'm not really certain it is a good idea to move this last one, as I can't
immediately spot uses of it from the hvm/ subtree.

> -    }
> -    else if ( is_idle_domain(d) )
> +    if ( is_idle_domain(d) )
>          rc = 0;
>      else

This now needs to be "else if ( !is_hvm_domain(d) )" afaict.

> @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d)
>  
>      hvm_init_cacheattr_region_list(d);
>  
> -    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> +    d->arch.hvm_domain.hap_enabled =
> +        hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> +
> +    rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
>      if ( rc != 0 )
>          goto fail0;
>  
> +    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> +    if ( rc != 0 )
> +        goto fail1;

Is the order of these required to be that way? The various failN
labels would seem to not require re-numbering if you swapped
the last two actions.

Jan


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

Re: [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH for-next 6/8] x86/domain: factor out pv_domain_initialise

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -533,6 +533,58 @@ static void pv_domain_destroy(struct domain *d)
>      free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
>  }
>  
> +static int pv_domain_initialise(struct domain *d, unsigned int domcr_flags)
> +{
> +    static const struct arch_csw pv_csw = {
> +        .from = paravirt_ctxt_switch_from,
> +        .to   = paravirt_ctxt_switch_to,
> +        .tail = continue_nonidle_domain,
> +    };
> +    int rc = -ENOMEM;
> +
> +    d->arch.pv_domain.gdt_ldt_l1tab =
> +        alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
> +    if ( !d->arch.pv_domain.gdt_ldt_l1tab )
> +        goto fail;
> +    clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
> +
> +    if ( levelling_caps & ~LCAP_faulting )
> +    {
> +        d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
> +        if ( !d->arch.pv_domain.cpuidmasks )
> +            goto fail;
> +        *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
> +    }
> +
> +    rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> +                                  GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> +                                  NULL, NULL);
> +    if ( rc )
> +        goto fail;
> +
> +    d->arch.ctxt_switch = &pv_csw;
> +
> +    /* 64-bit PV guest by default. */
> +    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +
> +    return 0;
> +
> +fail:

Labels indented by at least one space please.

> +    if ( d->arch.pv_domain.gdt_ldt_l1tab )
> +    {
> +        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> +        d->arch.pv_domain.gdt_ldt_l1tab = NULL;
> +    }
> +
> +    if ( d->arch.pv_domain.cpuidmasks )
> +    {
> +        xfree(d->arch.pv_domain.cpuidmasks);
> +        d->arch.pv_domain.cpuidmasks = NULL;
> +    }

Perhaps better to amend pv_domain_destroy() with the two NULL
assignments, and call it from here?

> @@ -593,30 +645,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>          d->arch.emulation_flags = emflags;
>      }
>  
> -    if ( is_idle_domain(d) )
> -        rc = 0;
> -    else
> -    {
> -        d->arch.pv_domain.gdt_ldt_l1tab =
> -            alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
> -        if ( !d->arch.pv_domain.gdt_ldt_l1tab )
> -            goto fail;
> -        clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
> -
> -        if ( levelling_caps & ~LCAP_faulting )
> -        {
> -            d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
> -            if ( !d->arch.pv_domain.cpuidmasks )
> -                goto fail;
> -            *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
> -        }
> -
> -        rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> -                                      GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> -                                      NULL, NULL);
> -    }
> -    if ( rc )
> -        goto fail;
> +    rc = 0;

Seems to point out that the latest now the initializer of the variable
is pointless (and therefore could be switched to 0 instead of this
assignment, if an initializer is needed at all - not sure about that).

Jan


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

Re: [PATCH for-next 1/8] xen.h: fix comment for vcpu_guest_context

[Julien Grall]

Hi Jan,

On 24/04/17 10:51, Jan Beulich wrote:
>>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
>> Use the correct vcpuop name and delete on trailing white space.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Afaic fine to go in right away (no code change at all). Julien?

Yes, I am happy to see the documentation/comment going in the tree 
without release-ack.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 1/8] xen.h: fix comment for vcpu_guest_context

[Wei Liu]

On Mon, Apr 24, 2017 at 03:51:42AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> > Use the correct vcpuop name and delete on trailing white space.

There is a typo. It should be:

delete *one* trailing ...

I fixed it in my local copy, but since you want this in right away, it
would be good if you can fix it while committing.

Thanks.

> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Afaic fine to go in right away (no code change at all). Julien?
> 
> Jan
> 

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

Re: [PATCH for-next 2/8] x86/domain: factor out pv_vcpu_initialise

[Wei Liu]

On Mon, Apr 24, 2017 at 03:57:42AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> > +int vcpu_initialise(struct vcpu *v)
> > +{
> > +    struct domain *d = v->domain;
> > +    int rc;
> > +
> > +    v->arch.flags = TF_kernel_mode;
> > +
> > +    rc = mapcache_vcpu_init(v);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( !is_idle_domain(d) )
> > +    {
> > +        paging_vcpu_init(v);
> > +
> > +        if ( (rc = vcpu_init_fpu(v)) != 0 )
> > +            return rc;
> > +
> > +        vmce_init_vcpu(v);
> > +    }
> > +    else if ( (rc = xstate_alloc_save_area(v)) != 0 )
> > +        return rc;
> > +
> > +    spin_lock_init(&v->arch.vpmu.vpmu_lock);
> > +
> > +    if ( is_hvm_domain(d) )
> > +        rc = hvm_vcpu_initialise(v);
> > +    else
> > +        rc = pv_vcpu_initialise(v);
> > +
> >      if ( rc )
> >      {
> >          vcpu_destroy_fpu(v);
> 
> Below here v->arch.pv_vcpu.trap_ctxt is being freed, which now also
> belongs into the PV function.
> 

After looking more closely into the original code, I think there is at
least one issue that the perdomain mapping is  not destroyed in the
error path.

Note there is no memory leak because eventually all perdomain mappings
will be freed in arch_domain_destroy. But I think we should make the
code look less bad while we are at it.  It is probably going to be more
code churn coming.

Wei.

> Jan
> 

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

Re: [PATCH for-next 1/8] xen.h: fix comment for vcpu_guest_context

[Jan Beulich]

>>> On 24.04.17 at 12:42, <wei.liu2@citrix.com> wrote:
> On Mon, Apr 24, 2017 at 03:51:42AM -0600, Jan Beulich wrote:
>> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
>> > Use the correct vcpuop name and delete on trailing white space.
> 
> There is a typo. It should be:
> 
> delete *one* trailing ...

I did notice it, but it didn't seem important enough to call out.

> I fixed it in my local copy, but since you want this in right away, it
> would be good if you can fix it while committing.

Hmm, I thought you would commit your own patch yourself.

Jan


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

Re: [PATCH for-next 7/8] x86/domain: move PV specific code to pv/domain.c

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> Move all the PV specific code along with the supporting code to
> pv/domain.c.

Had you done this series in a different order, or had the earlier
patches moved their broken out functions right away, this patch
would have been quite a bit smaller. Anyways, it looks to be pure
code motion so ought to be fine once re-based over the changes
expected to the earlier patches (this re-basing would also be
easier if the new functions were moved right away).

> --- /dev/null
> +++ b/xen/arch/x86/pv/domain.c
> @@ -0,0 +1,270 @@
> +/******************************************************************************
> + * arch/x86/pv/domain.c
> + *
> + * PV-specific domain handling
> + */
> +
> +/*
> + *  Copyright (C) 1995  Linus Torvalds
> + *
> + *  Pentium III FXSR, SSE support
> + *  Gareth Hughes <gareth@valinux.com>, May 2000
> + */

It's probably necessary/appropriate to keep (copy) this, but I highly
doubt there is any bit left here of Linux origin.

Jan


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

Re: [PATCH for-next 8/8] x86/domain: move HVM specific code to hvm/domain.c

[Jan Beulich]

>>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> There is only one function arch_set_info_hvm_guest is moved. The
> check_segment function is also moved since arch_set_info_hvm_guest is
> the only user.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Same comment regarding the odd copyright copy.

Jan


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

Re: [PATCH for-next 1/8] xen.h: fix comment for vcpu_guest_context

[Wei Liu]

On Mon, Apr 24, 2017 at 06:29:20AM -0600, Jan Beulich wrote:
> >>> On 24.04.17 at 12:42, <wei.liu2@citrix.com> wrote:
> > On Mon, Apr 24, 2017 at 03:51:42AM -0600, Jan Beulich wrote:
> >> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> >> > Use the correct vcpuop name and delete on trailing white space.
> > 
> > There is a typo. It should be:
> > 
> > delete *one* trailing ...
> 
> I did notice it, but it didn't seem important enough to call out.
> 
> > I fixed it in my local copy, but since you want this in right away, it
> > would be good if you can fix it while committing.
> 
> Hmm, I thought you would commit your own patch yourself.
> 

NP, I will commit it soon.

> Jan
> 

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

Re: [PATCH for-next 8/8] x86/domain: move HVM specific code to hvm/domain.c

[Roger Pau Monné]

On Mon, Apr 24, 2017 at 06:41:10AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> > There is only one function arch_set_info_hvm_guest is moved. The
> > check_segment function is also moved since arch_set_info_hvm_guest is
> > the only user.
> > 
> > No functional change.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Same comment regarding the odd copyright copy.

I was the one that added both functions, and I can attest it's not based on
Linus/Greg code in any way, could you please replace the header with:

/*
 * HVM domain specific functions.
 *
 * Copyright (C) 2017 Citrix Systems R&D
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms and conditions of the GNU General Public
 * License, version 2, as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 * General Public License for more details.
 *
 * You should have received a copy of the GNU General Public
 * License along with this program; If not, see <http://www.gnu.org/licenses/>.
 */

Or similar (I don't think spelling out my name is required).

Thanks, Roger.


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

Re: [PATCH for-next 7/8] x86/domain: move PV specific code to pv/domain.c

[Wei Liu]

On Mon, Apr 24, 2017 at 06:39:38AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> > Move all the PV specific code along with the supporting code to
> > pv/domain.c.
> 
> Had you done this series in a different order, or had the earlier
> patches moved their broken out functions right away, this patch
> would have been quite a bit smaller. Anyways, it looks to be pure

It is just trading one kind of code churn for another. For example,
free_compact_l4 is used by several PV functions. Should I choose to move
those PV functions (pv_vcpu_initialise and _destroy) as I go along I
will then need to export these static helper functions while I am doing
it then unexport them when I am finished.

I am not too fuss either way in this particular series. I am inclined to
move everything in one or more patches when the refactoring is done. But
please let me know if you feel strongly about how it should be done.

Wei.

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

Re: [PATCH for-next 4/8] x86/domain: push some code down to hvm_domain_initialise

[Wei Liu]

On Mon, Apr 24, 2017 at 04:10:14AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
> > We want to have a single entry point to initialise hvm guest.  The
> > timing to set hap bit and create per domain mapping is deferred, but
> > that's not a problem.
> > 
> > No functional change.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xen/arch/x86/domain.c         | 11 ++---------
> >  xen/arch/x86/hvm/hvm.c        | 25 +++++++++++++++++--------
> >  xen/include/asm-x86/hvm/hvm.h |  2 +-
> >  3 files changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index ddebff6187..af060d8239 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -587,14 +587,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> >          d->arch.emulation_flags = emflags;
> >      }
> >  
> > -    if ( is_hvm_domain(d) )
> > -    {
> > -        d->arch.hvm_domain.hap_enabled =
> > -            hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> > -
> > -        rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
> 
> I'm not really certain it is a good idea to move this last one, as I can't
> immediately spot uses of it from the hvm/ subtree.
> 

I think the sole purpose of this call is to create perdomain_l3_pg (when
nr is 0), which is then referenced in monitor table creation. Moving
this doesn't seem to cause problem.

> > -    }
> > -    else if ( is_idle_domain(d) )
> > +    if ( is_idle_domain(d) )
> >          rc = 0;
> >      else
> 
> This now needs to be "else if ( !is_hvm_domain(d) )" afaict.
> 

I see what you mean. 

> > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d)
> >  
> >      hvm_init_cacheattr_region_list(d);
> >  
> > -    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> > +    d->arch.hvm_domain.hap_enabled =
> > +        hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> > +
> > +    rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL, NULL);
> >      if ( rc != 0 )
> >          goto fail0;
> >  
> > +    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> > +    if ( rc != 0 )
> > +        goto fail1;
> 
> Is the order of these required to be that way? The various failN
> labels would seem to not require re-numbering if you swapped
> the last two actions.
> 

The mapping needs to be created before updating paging mode. I don't
think the order is important in this function.

Wei.

> Jan
> 

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

Re: [PATCH for-next 7/8] x86/domain: move PV specific code to pv/domain.c

[Jan Beulich]

>>> On 24.04.17 at 16:24, <wei.liu2@citrix.com> wrote:
> On Mon, Apr 24, 2017 at 06:39:38AM -0600, Jan Beulich wrote:
>> >>> On 10.04.17 at 15:27, <wei.liu2@citrix.com> wrote:
>> > Move all the PV specific code along with the supporting code to
>> > pv/domain.c.
>> 
>> Had you done this series in a different order, or had the earlier
>> patches moved their broken out functions right away, this patch
>> would have been quite a bit smaller. Anyways, it looks to be pure
> 
> It is just trading one kind of code churn for another. For example,
> free_compact_l4 is used by several PV functions. Should I choose to move
> those PV functions (pv_vcpu_initialise and _destroy) as I go along I
> will then need to export these static helper functions while I am doing
> it then unexport them when I am finished.

Ah, true.

> I am not too fuss either way in this particular series. I am inclined to
> move everything in one or more patches when the refactoring is done. But
> please let me know if you feel strongly about how it should be done.

As long as there are reasons behind how things are done, I don't
think my personal taste matters all that much.

Jan


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

Re: [PATCH for-next 4/8] x86/domain: push some code down to hvm_domain_initialise

[Wei Liu]

On Mon, Apr 10, 2017 at 04:19:12PM +0100, Andrew Cooper wrote:
> On 10/04/17 14:27, Wei Liu wrote:
> > We want to have a single entry point to initialise hvm guest.  The
> > timing to set hap bit and create per domain mapping is deferred, but
> > that's not a problem.
> >
> > No functional change.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Two observations...
> 
> > ---
> >  xen/arch/x86/domain.c         | 11 ++---------
> >  xen/arch/x86/hvm/hvm.c        | 25 +++++++++++++++++--------
> >  xen/include/asm-x86/hvm/hvm.h |  2 +-
> >  3 files changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index ddebff6187..af060d8239 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -663,7 +656,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> >  
> >      if ( is_hvm_domain(d) )
> >      {
> > -        if ( (rc = hvm_domain_initialise(d)) != 0 )
> > +        if ( (rc = hvm_domain_initialise(d, domcr_flags)) != 0 )
> 
> If we are pushing these values down (which I agree is a good idea),
> please can we push xen_arch_domainconfig down as well.
> 

Fine by me.

> >              goto fail;
> >      }
> >      else
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index f50d15ff50..7fc49bb03d 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -615,10 +615,17 @@ int hvm_domain_initialise(struct domain *d)
> >  
> >      hvm_init_cacheattr_region_list(d);
> >  
> > -    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
> > +    d->arch.hvm_domain.hap_enabled =
> > +        hvm_funcs.hap_supported && (domcr_flags & DOMCRF_hap);
> 
> We really should fail with -EOPNOTSUPP.
> 
> The toolstack already knows whether HAP is available, and nothing good
> can come from Xen falling silently back to shadow behind the toolstacks
> back.
> 
> Probably worth being in a solo patch though.
> 

A solo series to be precise. Toolstack needs some fixes to make this
work.

Wei.

> ~Andrew

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

Re: [PATCH for-next 6/8] x86/domain: factor out pv_domain_initialise

[Wei Liu]

On Mon, Apr 24, 2017 at 04:20:36AM -0600, Jan Beulich wrote:
> > +    if ( d->arch.pv_domain.gdt_ldt_l1tab )
> > +    {
> > +        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
> > +        d->arch.pv_domain.gdt_ldt_l1tab = NULL;
> > +    }
> > +
> > +    if ( d->arch.pv_domain.cpuidmasks )
> > +    {
> > +        xfree(d->arch.pv_domain.cpuidmasks);
> > +        d->arch.pv_domain.cpuidmasks = NULL;
> > +    }
> 
> Perhaps better to amend pv_domain_destroy() with the two NULL
> assignments, and call it from here?
> 

That is how it is done in v2 now.

> > @@ -593,30 +645,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
> >          d->arch.emulation_flags = emflags;
> >      }
> >  
> > -    if ( is_idle_domain(d) )
> > -        rc = 0;
> > -    else
> > -    {
> > -        d->arch.pv_domain.gdt_ldt_l1tab =
> > -            alloc_xenheap_pages(0, MEMF_node(domain_to_node(d)));
> > -        if ( !d->arch.pv_domain.gdt_ldt_l1tab )
> > -            goto fail;
> > -        clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
> > -
> > -        if ( levelling_caps & ~LCAP_faulting )
> > -        {
> > -            d->arch.pv_domain.cpuidmasks = xmalloc(struct cpuidmasks);
> > -            if ( !d->arch.pv_domain.cpuidmasks )
> > -                goto fail;
> > -            *d->arch.pv_domain.cpuidmasks = cpuidmask_defaults;
> > -        }
> > -
> > -        rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> > -                                      GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> > -                                      NULL, NULL);
> > -    }
> > -    if ( rc )
> > -        goto fail;
> > +    rc = 0;
> 
> Seems to point out that the latest now the initializer of the variable
> is pointless (and therefore could be switched to 0 instead of this
> assignment, if an initializer is needed at all - not sure about that).

This assignment is gone in v2. The initializer isn't needed anymore.

> 
> Jan
> 

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