* [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work
@ 2023-01-09 13:38 Jan Beulich
2023-01-09 13:39 ` [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown Jan Beulich
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Jan Beulich @ 2023-01-09 13:38 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Tim Deegan
Various tidying changes accumulated during that effort. They're all
functionally independent afaict, but there may be contextual
interactions between some of them.
v2 addresses review comments, which however goes as far as introducing
a new patch (2).
1: paging: fold most HAP and shadow final teardown
2: paging: drop set-allocation from final-teardown
3: paging: move update_paging_modes() hook
4: paging: move and conditionalize flush_tlb() hook
5: shadow: drop zero initialization from shadow_domain_init()
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown
2023-01-09 13:38 [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work Jan Beulich
@ 2023-01-09 13:39 ` Jan Beulich
2023-03-16 12:24 ` Roger Pau Monné
2023-01-09 13:39 ` [PATCH v2 2/5] x86/paging: drop set-allocation from final-teardown Jan Beulich
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-01-09 13:39 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Tim Deegan
HAP does a few things beyond what's common, which are left there at
least for now. Common operations, however, are moved to
paging_final_teardown(), allowing shadow_final_teardown() to go away.
While moving (and hence generalizing) the respective SHADOW_PRINTK()
drop the logging of total_pages from the 2nd instance - the value is
necessarily zero after {hap,shadow}_set_allocation() - and shorten the
messages, in part accounting for PAGING_PRINTK() logging __func__
already.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The remaining parts of hap_final_teardown() could be moved as well, at
the price of a CONFIG_HVM conditional. I wasn't sure whether that was
deemed reasonable.
---
v2: Shorten PAGING_PRINTK() messages. Adjust comments while being
moved.
--- a/xen/arch/x86/include/asm/shadow.h
+++ b/xen/arch/x86/include/asm/shadow.h
@@ -78,9 +78,6 @@ int shadow_domctl(struct domain *d,
void shadow_vcpu_teardown(struct vcpu *v);
void shadow_teardown(struct domain *d, bool *preempted);
-/* Call once all of the references to the domain have gone away */
-void shadow_final_teardown(struct domain *d);
-
void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all);
/* Adjust shadows ready for a guest page to change its type. */
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -268,8 +268,8 @@ static void hap_free(struct domain *d, m
/*
* For dying domains, actually free the memory here. This way less work is
- * left to hap_final_teardown(), which cannot easily have preemption checks
- * added.
+ * left to paging_final_teardown(), which cannot easily have preemption
+ * checks added.
*/
if ( unlikely(d->is_dying) )
{
@@ -552,18 +552,6 @@ void hap_final_teardown(struct domain *d
for (i = 0; i < MAX_NESTEDP2M; i++) {
p2m_teardown(d->arch.nested_p2m[i], true, NULL);
}
-
- if ( d->arch.paging.total_pages != 0 )
- hap_teardown(d, NULL);
-
- p2m_teardown(p2m_get_hostp2m(d), true, NULL);
- /* Free any memory that the p2m teardown released */
- paging_lock(d);
- hap_set_allocation(d, 0, NULL);
- ASSERT(d->arch.paging.p2m_pages == 0);
- ASSERT(d->arch.paging.free_pages == 0);
- ASSERT(d->arch.paging.total_pages == 0);
- paging_unlock(d);
}
void hap_vcpu_teardown(struct vcpu *v)
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -842,10 +842,45 @@ int paging_teardown(struct domain *d)
/* Call once all of the references to the domain have gone away */
void paging_final_teardown(struct domain *d)
{
- if ( hap_enabled(d) )
+ bool hap = hap_enabled(d);
+
+ PAGING_PRINTK("%pd start: total = %u, free = %u, p2m = %u\n",
+ d, d->arch.paging.total_pages,
+ d->arch.paging.free_pages, d->arch.paging.p2m_pages);
+
+ if ( hap )
hap_final_teardown(d);
+
+ /*
+ * Remove remaining paging memory. This can be nonzero on certain error
+ * paths.
+ */
+ if ( d->arch.paging.total_pages )
+ {
+ if ( hap )
+ hap_teardown(d, NULL);
+ else
+ shadow_teardown(d, NULL);
+ }
+
+ /* It is now safe to pull down the p2m map. */
+ p2m_teardown(p2m_get_hostp2m(d), true, NULL);
+
+ /* Free any paging memory that the p2m teardown released. */
+ paging_lock(d);
+
+ if ( hap )
+ hap_set_allocation(d, 0, NULL);
else
- shadow_final_teardown(d);
+ shadow_set_allocation(d, 0, NULL);
+
+ PAGING_PRINTK("%pd done: free = %u, p2m = %u\n",
+ d, d->arch.paging.free_pages, d->arch.paging.p2m_pages);
+ ASSERT(!d->arch.paging.p2m_pages);
+ ASSERT(!d->arch.paging.free_pages);
+ ASSERT(!d->arch.paging.total_pages);
+
+ paging_unlock(d);
p2m_final_teardown(d);
}
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1194,7 +1194,7 @@ void shadow_free(struct domain *d, mfn_t
/*
* For dying domains, actually free the memory here. This way less
- * work is left to shadow_final_teardown(), which cannot easily have
+ * work is left to paging_final_teardown(), which cannot easily have
* preemption checks added.
*/
if ( unlikely(dying) )
@@ -2898,35 +2898,6 @@ out:
}
}
-void shadow_final_teardown(struct domain *d)
-/* Called by arch_domain_destroy(), when it's safe to pull down the p2m map. */
-{
- SHADOW_PRINTK("dom %u final teardown starts."
- " Shadow pages total = %u, free = %u, p2m=%u\n",
- d->domain_id, d->arch.paging.total_pages,
- d->arch.paging.free_pages, d->arch.paging.p2m_pages);
-
- /* Double-check that the domain didn't have any shadow memory.
- * It is possible for a domain that never got domain_kill()ed
- * to get here with its shadow allocation intact. */
- if ( d->arch.paging.total_pages != 0 )
- shadow_teardown(d, NULL);
-
- /* It is now safe to pull down the p2m map. */
- p2m_teardown(p2m_get_hostp2m(d), true, NULL);
- /* Free any shadow memory that the p2m teardown released */
- paging_lock(d);
- shadow_set_allocation(d, 0, NULL);
- SHADOW_PRINTK("dom %u final teardown done."
- " Shadow pages total = %u, free = %u, p2m=%u\n",
- d->domain_id, d->arch.paging.total_pages,
- d->arch.paging.free_pages, d->arch.paging.p2m_pages);
- ASSERT(d->arch.paging.p2m_pages == 0);
- ASSERT(d->arch.paging.free_pages == 0);
- ASSERT(d->arch.paging.total_pages == 0);
- paging_unlock(d);
-}
-
static int shadow_one_bit_enable(struct domain *d, u32 mode)
/* Turn on a single shadow mode feature */
{
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/5] x86/paging: drop set-allocation from final-teardown
2023-01-09 13:38 [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work Jan Beulich
2023-01-09 13:39 ` [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown Jan Beulich
@ 2023-01-09 13:39 ` Jan Beulich
2023-03-16 12:34 ` Roger Pau Monné
2023-01-09 13:40 ` [PATCH v2 3/5] x86/paging: move update_paging_modes() hook Jan Beulich
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-01-09 13:39 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Tim Deegan
The fixes for XSA-410 have arranged for P2M pages being freed by P2M
code to be properly freed directly, rather than being put back on the
paging pool list. Therefore whatever p2m_teardown() may return will no
longer need taking care of here. Drop the code, leaving the assertions
in place and adding "total" back to the PAGING_PRINTK() message.
With merely the (optional) log message and the assertions left, there's
really no point anymore to hold the paging lock there, so drop that too.
Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The remaining parts of hap_final_teardown() could be moved as well, at
the price of a CONFIG_HVM conditional. I wasn't sure whether that was
deemed reasonable.
---
v2: New.
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -866,22 +866,13 @@ void paging_final_teardown(struct domain
/* It is now safe to pull down the p2m map. */
p2m_teardown(p2m_get_hostp2m(d), true, NULL);
- /* Free any paging memory that the p2m teardown released. */
- paging_lock(d);
-
- if ( hap )
- hap_set_allocation(d, 0, NULL);
- else
- shadow_set_allocation(d, 0, NULL);
-
- PAGING_PRINTK("%pd done: free = %u, p2m = %u\n",
- d, d->arch.paging.free_pages, d->arch.paging.p2m_pages);
+ PAGING_PRINTK("%pd done: total = %u, free = %u, p2m = %u\n",
+ d, d->arch.paging.total_pages,
+ d->arch.paging.free_pages, d->arch.paging.p2m_pages);
ASSERT(!d->arch.paging.p2m_pages);
ASSERT(!d->arch.paging.free_pages);
ASSERT(!d->arch.paging.total_pages);
- paging_unlock(d);
-
p2m_final_teardown(d);
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/5] x86/paging: move update_paging_modes() hook
2023-01-09 13:38 [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work Jan Beulich
2023-01-09 13:39 ` [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown Jan Beulich
2023-01-09 13:39 ` [PATCH v2 2/5] x86/paging: drop set-allocation from final-teardown Jan Beulich
@ 2023-01-09 13:40 ` Jan Beulich
2023-03-16 12:39 ` Roger Pau Monné
2023-01-09 13:41 ` [PATCH v2 4/5] x86/paging: move and conditionalize flush_tlb() hook Jan Beulich
2023-01-09 13:41 ` [PATCH v2 5/5] x86/shadow: drop zero initialization from shadow_domain_init() Jan Beulich
4 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-01-09 13:40 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Tim Deegan
The hook isn't mode dependent, hence it's misplaced in struct
paging_mode. (Or alternatively I see no reason why the alloc_page() and
free_page() hooks don't also live there.) Move it to struct
paging_domain.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Undo rename (plural -> singular). Add a comment in shadow/none.c.
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -235,6 +235,8 @@ struct paging_domain {
* (used by p2m and log-dirty code for their tries) */
struct page_info * (*alloc_page)(struct domain *d);
void (*free_page)(struct domain *d, struct page_info *pg);
+
+ void (*update_paging_modes)(struct vcpu *v);
};
struct paging_vcpu {
--- a/xen/arch/x86/include/asm/paging.h
+++ b/xen/arch/x86/include/asm/paging.h
@@ -140,7 +140,6 @@ struct paging_mode {
#endif
void (*update_cr3 )(struct vcpu *v, int do_locking,
bool noflush);
- void (*update_paging_modes )(struct vcpu *v);
bool (*flush_tlb )(const unsigned long *vcpu_bitmap);
unsigned int guest_levels;
@@ -316,7 +315,7 @@ static inline void paging_update_cr3(str
* has changed, and when bringing up a VCPU for the first time. */
static inline void paging_update_paging_modes(struct vcpu *v)
{
- paging_get_hostmode(v)->update_paging_modes(v);
+ v->domain->arch.paging.update_paging_modes(v);
}
#ifdef CONFIG_PV
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -443,6 +443,9 @@ static void hap_destroy_monitor_table(st
/************************************************/
/* HAP DOMAIN LEVEL FUNCTIONS */
/************************************************/
+
+static void cf_check hap_update_paging_modes(struct vcpu *v);
+
void hap_domain_init(struct domain *d)
{
static const struct log_dirty_ops hap_ops = {
@@ -453,6 +456,8 @@ void hap_domain_init(struct domain *d)
/* Use HAP logdirty mechanism. */
paging_log_dirty_init(d, &hap_ops);
+
+ d->arch.paging.update_paging_modes = hap_update_paging_modes;
}
/* return 0 for success, -errno for failure */
@@ -842,7 +847,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_real_mode,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_real_mode,
.update_cr3 = hap_update_cr3,
- .update_paging_modes = hap_update_paging_modes,
.flush_tlb = flush_tlb,
.guest_levels = 1
};
@@ -853,7 +857,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_2_levels,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_2_levels,
.update_cr3 = hap_update_cr3,
- .update_paging_modes = hap_update_paging_modes,
.flush_tlb = flush_tlb,
.guest_levels = 2
};
@@ -864,7 +867,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_3_levels,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_3_levels,
.update_cr3 = hap_update_cr3,
- .update_paging_modes = hap_update_paging_modes,
.flush_tlb = flush_tlb,
.guest_levels = 3
};
@@ -875,7 +877,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_4_levels,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_4_levels,
.update_cr3 = hap_update_cr3,
- .update_paging_modes = hap_update_paging_modes,
.flush_tlb = flush_tlb,
.guest_levels = 4
};
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -45,6 +45,8 @@ static int cf_check sh_enable_log_dirty(
static int cf_check sh_disable_log_dirty(struct domain *);
static void cf_check sh_clean_dirty_bitmap(struct domain *);
+static void cf_check shadow_update_paging_modes(struct vcpu *);
+
/* Set up the shadow-specific parts of a domain struct at start of day.
* Called for every domain from arch_domain_create() */
int shadow_domain_init(struct domain *d)
@@ -60,6 +62,8 @@ int shadow_domain_init(struct domain *d)
/* Use shadow pagetables for log-dirty support */
paging_log_dirty_init(d, &sh_ops);
+ d->arch.paging.update_paging_modes = shadow_update_paging_modes;
+
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
d->arch.paging.shadow.oos_active = 0;
#endif
@@ -2516,7 +2520,12 @@ static void sh_update_paging_modes(struc
v->arch.paging.mode->update_cr3(v, 0, false);
}
-void cf_check shadow_update_paging_modes(struct vcpu *v)
+/*
+ * Update all the things that are derived from the guest's CR0/CR3/CR4.
+ * Called to initialize paging structures if the paging mode has changed,
+ * and when bringing up a VCPU for the first time.
+ */
+static void cf_check shadow_update_paging_modes(struct vcpu *v)
{
paging_lock(v->domain);
sh_update_paging_modes(v);
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4203,7 +4203,6 @@ const struct paging_mode sh_paging_mode
.gva_to_gfn = sh_gva_to_gfn,
#endif
.update_cr3 = sh_update_cr3,
- .update_paging_modes = shadow_update_paging_modes,
.flush_tlb = shadow_flush_tlb,
.guest_levels = GUEST_PAGING_LEVELS,
.shadow.detach_old_tables = sh_detach_old_tables,
--- a/xen/arch/x86/mm/shadow/none.c
+++ b/xen/arch/x86/mm/shadow/none.c
@@ -18,8 +18,14 @@ static void cf_check _clean_dirty_bitmap
ASSERT(is_pv_domain(d));
}
+static void cf_check _update_paging_modes(struct vcpu *v)
+{
+ ASSERT_UNREACHABLE();
+}
+
int shadow_domain_init(struct domain *d)
{
+ /* For HVM set up pointers for safety, then fail. */
static const struct log_dirty_ops sh_none_ops = {
.enable = _enable_log_dirty,
.disable = _disable_log_dirty,
@@ -27,6 +33,9 @@ int shadow_domain_init(struct domain *d)
};
paging_log_dirty_init(d, &sh_none_ops);
+
+ d->arch.paging.update_paging_modes = _update_paging_modes;
+
return is_hvm_domain(d) ? -EOPNOTSUPP : 0;
}
@@ -57,11 +66,6 @@ static void cf_check _update_cr3(struct
ASSERT_UNREACHABLE();
}
-static void cf_check _update_paging_modes(struct vcpu *v)
-{
- ASSERT_UNREACHABLE();
-}
-
static const struct paging_mode sh_paging_none = {
.page_fault = _page_fault,
.invlpg = _invlpg,
@@ -69,7 +73,6 @@ static const struct paging_mode sh_pagin
.gva_to_gfn = _gva_to_gfn,
#endif
.update_cr3 = _update_cr3,
- .update_paging_modes = _update_paging_modes,
};
void shadow_vcpu_init(struct vcpu *v)
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -426,11 +426,6 @@ void cf_check sh_write_guest_entry(
intpte_t cf_check sh_cmpxchg_guest_entry(
struct vcpu *v, intpte_t *p, intpte_t old, intpte_t new, mfn_t gmfn);
-/* Update all the things that are derived from the guest's CR0/CR3/CR4.
- * Called to initialize paging structures if the paging mode
- * has changed, and when bringing up a VCPU for the first time. */
-void cf_check shadow_update_paging_modes(struct vcpu *v);
-
/* Unhook the non-Xen mappings in this top-level shadow mfn.
* With user_only == 1, unhooks only the user-mode mappings. */
void shadow_unhook_mappings(struct domain *d, mfn_t smfn, int user_only);
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/5] x86/paging: move and conditionalize flush_tlb() hook
2023-01-09 13:38 [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work Jan Beulich
` (2 preceding siblings ...)
2023-01-09 13:40 ` [PATCH v2 3/5] x86/paging: move update_paging_modes() hook Jan Beulich
@ 2023-01-09 13:41 ` Jan Beulich
2023-01-09 13:41 ` [PATCH v2 5/5] x86/shadow: drop zero initialization from shadow_domain_init() Jan Beulich
4 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-01-09 13:41 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Tim Deegan
The hook isn't mode dependent, hence it's misplaced in struct
paging_mode. (Or alternatively I see no reason why the alloc_page() and
free_page() hooks don't also live there.) Move it to struct
paging_domain.
The hook also is used for HVM guests only, so make respective pieces
conditional upon CONFIG_HVM.
While there also add __must_check to the hook declaration, as it's
imperative that callers deal with getting back "false".
While moving the shadow implementation, introduce a "curr" local
variable.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Re-base over changes earlier in the series.
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -237,6 +237,11 @@ struct paging_domain {
void (*free_page)(struct domain *d, struct page_info *pg);
void (*update_paging_modes)(struct vcpu *v);
+
+#ifdef CONFIG_HVM
+ /* Flush selected vCPUs TLBs. NULL for all. */
+ bool __must_check (*flush_tlb)(const unsigned long *vcpu_bitmap);
+#endif
};
struct paging_vcpu {
--- a/xen/arch/x86/include/asm/paging.h
+++ b/xen/arch/x86/include/asm/paging.h
@@ -140,7 +140,6 @@ struct paging_mode {
#endif
void (*update_cr3 )(struct vcpu *v, int do_locking,
bool noflush);
- bool (*flush_tlb )(const unsigned long *vcpu_bitmap);
unsigned int guest_levels;
@@ -300,6 +299,12 @@ static inline unsigned long paging_ga_to
page_order);
}
+/* Flush selected vCPUs TLBs. NULL for all. */
+static inline bool paging_flush_tlb(const unsigned long *vcpu_bitmap)
+{
+ return current->domain->arch.paging.flush_tlb(vcpu_bitmap);
+}
+
#endif /* CONFIG_HVM */
/* Update all the things that are derived from the guest's CR3.
@@ -408,12 +413,6 @@ static always_inline unsigned int paging
return bits;
}
-/* Flush selected vCPUs TLBs. NULL for all. */
-static inline bool paging_flush_tlb(const unsigned long *vcpu_bitmap)
-{
- return paging_get_hostmode(current)->flush_tlb(vcpu_bitmap);
-}
-
#endif /* XEN_PAGING_H */
/*
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -445,6 +445,7 @@ static void hap_destroy_monitor_table(st
/************************************************/
static void cf_check hap_update_paging_modes(struct vcpu *v);
+static bool cf_check flush_tlb(const unsigned long *vcpu_bitmap);
void hap_domain_init(struct domain *d)
{
@@ -458,6 +459,7 @@ void hap_domain_init(struct domain *d)
paging_log_dirty_init(d, &hap_ops);
d->arch.paging.update_paging_modes = hap_update_paging_modes;
+ d->arch.paging.flush_tlb = flush_tlb;
}
/* return 0 for success, -errno for failure */
@@ -847,7 +849,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_real_mode,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_real_mode,
.update_cr3 = hap_update_cr3,
- .flush_tlb = flush_tlb,
.guest_levels = 1
};
@@ -857,7 +858,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_2_levels,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_2_levels,
.update_cr3 = hap_update_cr3,
- .flush_tlb = flush_tlb,
.guest_levels = 2
};
@@ -867,7 +867,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_3_levels,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_3_levels,
.update_cr3 = hap_update_cr3,
- .flush_tlb = flush_tlb,
.guest_levels = 3
};
@@ -877,7 +876,6 @@ static const struct paging_mode hap_pagi
.gva_to_gfn = hap_gva_to_gfn_4_levels,
.p2m_ga_to_gfn = hap_p2m_ga_to_gfn_4_levels,
.update_cr3 = hap_update_cr3,
- .flush_tlb = flush_tlb,
.guest_levels = 4
};
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -68,6 +68,7 @@ int shadow_domain_init(struct domain *d)
d->arch.paging.shadow.oos_active = 0;
#endif
#ifdef CONFIG_HVM
+ d->arch.paging.flush_tlb = shadow_flush_tlb;
d->arch.paging.shadow.pagetable_dying_op = 0;
#endif
@@ -3092,66 +3093,6 @@ static void cf_check sh_clean_dirty_bitm
paging_unlock(d);
}
-
-static bool flush_vcpu(const struct vcpu *v, const unsigned long *vcpu_bitmap)
-{
- return !vcpu_bitmap || test_bit(v->vcpu_id, vcpu_bitmap);
-}
-
-/* Flush TLB of selected vCPUs. NULL for all. */
-bool cf_check shadow_flush_tlb(const unsigned long *vcpu_bitmap)
-{
- static DEFINE_PER_CPU(cpumask_t, flush_cpumask);
- cpumask_t *mask = &this_cpu(flush_cpumask);
- struct domain *d = current->domain;
- struct vcpu *v;
-
- /* Avoid deadlock if more than one vcpu tries this at the same time. */
- if ( !spin_trylock(&d->hypercall_deadlock_mutex) )
- return false;
-
- /* Pause all other vcpus. */
- for_each_vcpu ( d, v )
- if ( v != current && flush_vcpu(v, vcpu_bitmap) )
- vcpu_pause_nosync(v);
-
- /* Now that all VCPUs are signalled to deschedule, we wait... */
- for_each_vcpu ( d, v )
- if ( v != current && flush_vcpu(v, vcpu_bitmap) )
- while ( !vcpu_runnable(v) && v->is_running )
- cpu_relax();
-
- /* All other vcpus are paused, safe to unlock now. */
- spin_unlock(&d->hypercall_deadlock_mutex);
-
- cpumask_clear(mask);
-
- /* Flush paging-mode soft state (e.g., va->gfn cache; PAE PDPE cache). */
- for_each_vcpu ( d, v )
- {
- unsigned int cpu;
-
- if ( !flush_vcpu(v, vcpu_bitmap) )
- continue;
-
- paging_update_cr3(v, false);
-
- cpu = read_atomic(&v->dirty_cpu);
- if ( is_vcpu_dirty_cpu(cpu) )
- __cpumask_set_cpu(cpu, mask);
- }
-
- /* Flush TLBs on all CPUs with dirty vcpu state. */
- guest_flush_tlb_mask(d, mask);
-
- /* Done. */
- for_each_vcpu ( d, v )
- if ( v != current && flush_vcpu(v, vcpu_bitmap) )
- vcpu_unpause(v);
-
- return true;
-}
-
/**************************************************************************/
/* Shadow-control XEN_DOMCTL dispatcher */
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -719,6 +719,66 @@ static void sh_emulate_unmap_dest(struct
atomic_inc(&v->domain->arch.paging.shadow.gtable_dirty_version);
}
+static bool flush_vcpu(const struct vcpu *v, const unsigned long *vcpu_bitmap)
+{
+ return !vcpu_bitmap || test_bit(v->vcpu_id, vcpu_bitmap);
+}
+
+/* Flush TLB of selected vCPUs. NULL for all. */
+bool cf_check shadow_flush_tlb(const unsigned long *vcpu_bitmap)
+{
+ static DEFINE_PER_CPU(cpumask_t, flush_cpumask);
+ cpumask_t *mask = &this_cpu(flush_cpumask);
+ const struct vcpu *curr = current;
+ struct domain *d = curr->domain;
+ struct vcpu *v;
+
+ /* Avoid deadlock if more than one vcpu tries this at the same time. */
+ if ( !spin_trylock(&d->hypercall_deadlock_mutex) )
+ return false;
+
+ /* Pause all other vcpus. */
+ for_each_vcpu ( d, v )
+ if ( v != curr && flush_vcpu(v, vcpu_bitmap) )
+ vcpu_pause_nosync(v);
+
+ /* Now that all VCPUs are signalled to deschedule, we wait... */
+ for_each_vcpu ( d, v )
+ if ( v != curr && flush_vcpu(v, vcpu_bitmap) )
+ while ( !vcpu_runnable(v) && v->is_running )
+ cpu_relax();
+
+ /* All other vcpus are paused, safe to unlock now. */
+ spin_unlock(&d->hypercall_deadlock_mutex);
+
+ cpumask_clear(mask);
+
+ /* Flush paging-mode soft state (e.g., va->gfn cache; PAE PDPE cache). */
+ for_each_vcpu ( d, v )
+ {
+ unsigned int cpu;
+
+ if ( !flush_vcpu(v, vcpu_bitmap) )
+ continue;
+
+ paging_update_cr3(v, false);
+
+ cpu = read_atomic(&v->dirty_cpu);
+ if ( is_vcpu_dirty_cpu(cpu) )
+ __cpumask_set_cpu(cpu, mask);
+ }
+
+ /* Flush TLBs on all CPUs with dirty vcpu state. */
+ guest_flush_tlb_mask(d, mask);
+
+ /* Done. */
+ for_each_vcpu ( d, v )
+ if ( v != curr && flush_vcpu(v, vcpu_bitmap) )
+ vcpu_unpause(v);
+
+ return true;
+}
+
mfn_t sh_make_monitor_table(const struct vcpu *v, unsigned int shadow_levels)
{
struct domain *d = v->domain;
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4203,7 +4203,6 @@ const struct paging_mode sh_paging_mode
.gva_to_gfn = sh_gva_to_gfn,
#endif
.update_cr3 = sh_update_cr3,
- .flush_tlb = shadow_flush_tlb,
.guest_levels = GUEST_PAGING_LEVELS,
.shadow.detach_old_tables = sh_detach_old_tables,
#ifdef CONFIG_PV
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 5/5] x86/shadow: drop zero initialization from shadow_domain_init()
2023-01-09 13:38 [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work Jan Beulich
` (3 preceding siblings ...)
2023-01-09 13:41 ` [PATCH v2 4/5] x86/paging: move and conditionalize flush_tlb() hook Jan Beulich
@ 2023-01-09 13:41 ` Jan Beulich
4 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-01-09 13:41 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Tim Deegan
There's no need for this as struct domain starts out zero-filled.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Re-base over changes earlier in the series.
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -64,12 +64,8 @@ int shadow_domain_init(struct domain *d)
d->arch.paging.update_paging_modes = shadow_update_paging_modes;
-#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
- d->arch.paging.shadow.oos_active = 0;
-#endif
#ifdef CONFIG_HVM
d->arch.paging.flush_tlb = shadow_flush_tlb;
- d->arch.paging.shadow.pagetable_dying_op = 0;
#endif
return 0;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown
2023-01-09 13:39 ` [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown Jan Beulich
@ 2023-03-16 12:24 ` Roger Pau Monné
2023-03-16 12:57 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monné @ 2023-03-16 12:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Tim Deegan
On Mon, Jan 09, 2023 at 02:39:19PM +0100, Jan Beulich wrote:
> HAP does a few things beyond what's common, which are left there at
> least for now. Common operations, however, are moved to
> paging_final_teardown(), allowing shadow_final_teardown() to go away.
>
> While moving (and hence generalizing) the respective SHADOW_PRINTK()
> drop the logging of total_pages from the 2nd instance - the value is
> necessarily zero after {hap,shadow}_set_allocation() - and shorten the
> messages, in part accounting for PAGING_PRINTK() logging __func__
> already.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The remaining parts of hap_final_teardown() could be moved as well, at
> the price of a CONFIG_HVM conditional. I wasn't sure whether that was
> deemed reasonable.
> ---
> v2: Shorten PAGING_PRINTK() messages. Adjust comments while being
> moved.
>
> --- a/xen/arch/x86/include/asm/shadow.h
> +++ b/xen/arch/x86/include/asm/shadow.h
> @@ -78,9 +78,6 @@ int shadow_domctl(struct domain *d,
> void shadow_vcpu_teardown(struct vcpu *v);
> void shadow_teardown(struct domain *d, bool *preempted);
>
> -/* Call once all of the references to the domain have gone away */
> -void shadow_final_teardown(struct domain *d);
> -
> void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all);
>
> /* Adjust shadows ready for a guest page to change its type. */
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -268,8 +268,8 @@ static void hap_free(struct domain *d, m
>
> /*
> * For dying domains, actually free the memory here. This way less work is
> - * left to hap_final_teardown(), which cannot easily have preemption checks
> - * added.
> + * left to paging_final_teardown(), which cannot easily have preemption
> + * checks added.
> */
> if ( unlikely(d->is_dying) )
> {
> @@ -552,18 +552,6 @@ void hap_final_teardown(struct domain *d
> for (i = 0; i < MAX_NESTEDP2M; i++) {
> p2m_teardown(d->arch.nested_p2m[i], true, NULL);
> }
> -
> - if ( d->arch.paging.total_pages != 0 )
> - hap_teardown(d, NULL);
> -
> - p2m_teardown(p2m_get_hostp2m(d), true, NULL);
> - /* Free any memory that the p2m teardown released */
> - paging_lock(d);
> - hap_set_allocation(d, 0, NULL);
> - ASSERT(d->arch.paging.p2m_pages == 0);
> - ASSERT(d->arch.paging.free_pages == 0);
> - ASSERT(d->arch.paging.total_pages == 0);
> - paging_unlock(d);
> }
>
> void hap_vcpu_teardown(struct vcpu *v)
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -842,10 +842,45 @@ int paging_teardown(struct domain *d)
> /* Call once all of the references to the domain have gone away */
> void paging_final_teardown(struct domain *d)
> {
> - if ( hap_enabled(d) )
> + bool hap = hap_enabled(d);
> +
> + PAGING_PRINTK("%pd start: total = %u, free = %u, p2m = %u\n",
> + d, d->arch.paging.total_pages,
> + d->arch.paging.free_pages, d->arch.paging.p2m_pages);
> +
> + if ( hap )
> hap_final_teardown(d);
> +
> + /*
> + * Remove remaining paging memory. This can be nonzero on certain error
> + * paths.
> + */
> + if ( d->arch.paging.total_pages )
> + {
> + if ( hap )
> + hap_teardown(d, NULL);
> + else
> + shadow_teardown(d, NULL);
For a logical PoV, shouldn't hap_teardown() be called before
hap_final_teardown()?
Also hap_final_teardown() already contains a call to hap_teardown() if
total_pages != 0, so this is just redundant in the HAP case?
Maybe we want to pull that hap_teardown() out of hap_final_teardown()
and re-order the logic so hap_teardown() is called before
hap_final_teardown()?
Thanks, Roger.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/5] x86/paging: drop set-allocation from final-teardown
2023-01-09 13:39 ` [PATCH v2 2/5] x86/paging: drop set-allocation from final-teardown Jan Beulich
@ 2023-03-16 12:34 ` Roger Pau Monné
0 siblings, 0 replies; 12+ messages in thread
From: Roger Pau Monné @ 2023-03-16 12:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Tim Deegan
On Mon, Jan 09, 2023 at 02:39:52PM +0100, Jan Beulich wrote:
> The fixes for XSA-410 have arranged for P2M pages being freed by P2M
> code to be properly freed directly, rather than being put back on the
> paging pool list. Therefore whatever p2m_teardown() may return will no
> longer need taking care of here. Drop the code, leaving the assertions
> in place and adding "total" back to the PAGING_PRINTK() message.
>
> With merely the (optional) log message and the assertions left, there's
> really no point anymore to hold the paging lock there, so drop that too.
>
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> The remaining parts of hap_final_teardown() could be moved as well, at
> the price of a CONFIG_HVM conditional. I wasn't sure whether that was
> deemed reasonable.
I think it's cleaner to leave them as-is.
Thanks, Roger.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/5] x86/paging: move update_paging_modes() hook
2023-01-09 13:40 ` [PATCH v2 3/5] x86/paging: move update_paging_modes() hook Jan Beulich
@ 2023-03-16 12:39 ` Roger Pau Monné
0 siblings, 0 replies; 12+ messages in thread
From: Roger Pau Monné @ 2023-03-16 12:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Tim Deegan
On Mon, Jan 09, 2023 at 02:40:50PM +0100, Jan Beulich wrote:
> The hook isn't mode dependent, hence it's misplaced in struct
> paging_mode. (Or alternatively I see no reason why the alloc_page() and
> free_page() hooks don't also live there.) Move it to struct
> paging_domain.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown
2023-03-16 12:24 ` Roger Pau Monné
@ 2023-03-16 12:57 ` Jan Beulich
2023-03-16 13:28 ` Roger Pau Monné
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-03-16 12:57 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Tim Deegan
On 16.03.2023 13:24, Roger Pau Monné wrote:
> On Mon, Jan 09, 2023 at 02:39:19PM +0100, Jan Beulich wrote:
>> HAP does a few things beyond what's common, which are left there at
>> least for now. Common operations, however, are moved to
>> paging_final_teardown(), allowing shadow_final_teardown() to go away.
>>
>> While moving (and hence generalizing) the respective SHADOW_PRINTK()
>> drop the logging of total_pages from the 2nd instance - the value is
>> necessarily zero after {hap,shadow}_set_allocation() - and shorten the
>> messages, in part accounting for PAGING_PRINTK() logging __func__
>> already.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The remaining parts of hap_final_teardown() could be moved as well, at
>> the price of a CONFIG_HVM conditional. I wasn't sure whether that was
>> deemed reasonable.
>> ---
>> v2: Shorten PAGING_PRINTK() messages. Adjust comments while being
>> moved.
>>
>> --- a/xen/arch/x86/include/asm/shadow.h
>> +++ b/xen/arch/x86/include/asm/shadow.h
>> @@ -78,9 +78,6 @@ int shadow_domctl(struct domain *d,
>> void shadow_vcpu_teardown(struct vcpu *v);
>> void shadow_teardown(struct domain *d, bool *preempted);
>>
>> -/* Call once all of the references to the domain have gone away */
>> -void shadow_final_teardown(struct domain *d);
>> -
>> void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all);
>>
>> /* Adjust shadows ready for a guest page to change its type. */
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -268,8 +268,8 @@ static void hap_free(struct domain *d, m
>>
>> /*
>> * For dying domains, actually free the memory here. This way less work is
>> - * left to hap_final_teardown(), which cannot easily have preemption checks
>> - * added.
>> + * left to paging_final_teardown(), which cannot easily have preemption
>> + * checks added.
>> */
>> if ( unlikely(d->is_dying) )
>> {
>> @@ -552,18 +552,6 @@ void hap_final_teardown(struct domain *d
>> for (i = 0; i < MAX_NESTEDP2M; i++) {
>> p2m_teardown(d->arch.nested_p2m[i], true, NULL);
>> }
>> -
>> - if ( d->arch.paging.total_pages != 0 )
>> - hap_teardown(d, NULL);
>> -
>> - p2m_teardown(p2m_get_hostp2m(d), true, NULL);
>> - /* Free any memory that the p2m teardown released */
>> - paging_lock(d);
>> - hap_set_allocation(d, 0, NULL);
>> - ASSERT(d->arch.paging.p2m_pages == 0);
>> - ASSERT(d->arch.paging.free_pages == 0);
>> - ASSERT(d->arch.paging.total_pages == 0);
>> - paging_unlock(d);
>> }
>>
>> void hap_vcpu_teardown(struct vcpu *v)
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -842,10 +842,45 @@ int paging_teardown(struct domain *d)
>> /* Call once all of the references to the domain have gone away */
>> void paging_final_teardown(struct domain *d)
>> {
>> - if ( hap_enabled(d) )
>> + bool hap = hap_enabled(d);
>> +
>> + PAGING_PRINTK("%pd start: total = %u, free = %u, p2m = %u\n",
>> + d, d->arch.paging.total_pages,
>> + d->arch.paging.free_pages, d->arch.paging.p2m_pages);
>> +
>> + if ( hap )
>> hap_final_teardown(d);
>> +
>> + /*
>> + * Remove remaining paging memory. This can be nonzero on certain error
>> + * paths.
>> + */
>> + if ( d->arch.paging.total_pages )
>> + {
>> + if ( hap )
>> + hap_teardown(d, NULL);
>> + else
>> + shadow_teardown(d, NULL);
>
> For a logical PoV, shouldn't hap_teardown() be called before
> hap_final_teardown()?
Yes and no: The meaning of "final" has changed - previously it meant "the
final parts of tearing down" while now it means "the parts of tearing
down which must be done during final cleanup". I can't think of a better
name, so I left "hap_final_teardown" as it was.
> Also hap_final_teardown() already contains a call to hap_teardown() if
> total_pages != 0, so this is just redundant in the HAP case?
Well, like in shadow_final_teardown() there was such a call prior to this
change, but there's none left now.
> Maybe we want to pull that hap_teardown() out of hap_final_teardown()
That's what I'm doing here.
> and re-order the logic so hap_teardown() is called before
> hap_final_teardown()?
I'm not convinced re-ordering would be correct; even if it was I wouldn't
want to change order of operations here. Instead I want to limit the
changes to just the folding of duplicate (with shadow) code.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown
2023-03-16 12:57 ` Jan Beulich
@ 2023-03-16 13:28 ` Roger Pau Monné
2023-03-16 13:35 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Roger Pau Monné @ 2023-03-16 13:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Tim Deegan
On Thu, Mar 16, 2023 at 01:57:45PM +0100, Jan Beulich wrote:
> On 16.03.2023 13:24, Roger Pau Monné wrote:
> > On Mon, Jan 09, 2023 at 02:39:19PM +0100, Jan Beulich wrote:
> >> HAP does a few things beyond what's common, which are left there at
> >> least for now. Common operations, however, are moved to
> >> paging_final_teardown(), allowing shadow_final_teardown() to go away.
> >>
> >> While moving (and hence generalizing) the respective SHADOW_PRINTK()
> >> drop the logging of total_pages from the 2nd instance - the value is
> >> necessarily zero after {hap,shadow}_set_allocation() - and shorten the
> >> messages, in part accounting for PAGING_PRINTK() logging __func__
> >> already.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> The remaining parts of hap_final_teardown() could be moved as well, at
> >> the price of a CONFIG_HVM conditional. I wasn't sure whether that was
> >> deemed reasonable.
> >> ---
> >> v2: Shorten PAGING_PRINTK() messages. Adjust comments while being
> >> moved.
> >>
> >> --- a/xen/arch/x86/include/asm/shadow.h
> >> +++ b/xen/arch/x86/include/asm/shadow.h
> >> @@ -78,9 +78,6 @@ int shadow_domctl(struct domain *d,
> >> void shadow_vcpu_teardown(struct vcpu *v);
> >> void shadow_teardown(struct domain *d, bool *preempted);
> >>
> >> -/* Call once all of the references to the domain have gone away */
> >> -void shadow_final_teardown(struct domain *d);
> >> -
> >> void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all);
> >>
> >> /* Adjust shadows ready for a guest page to change its type. */
> >> --- a/xen/arch/x86/mm/hap/hap.c
> >> +++ b/xen/arch/x86/mm/hap/hap.c
> >> @@ -268,8 +268,8 @@ static void hap_free(struct domain *d, m
> >>
> >> /*
> >> * For dying domains, actually free the memory here. This way less work is
> >> - * left to hap_final_teardown(), which cannot easily have preemption checks
> >> - * added.
> >> + * left to paging_final_teardown(), which cannot easily have preemption
> >> + * checks added.
> >> */
> >> if ( unlikely(d->is_dying) )
> >> {
> >> @@ -552,18 +552,6 @@ void hap_final_teardown(struct domain *d
> >> for (i = 0; i < MAX_NESTEDP2M; i++) {
> >> p2m_teardown(d->arch.nested_p2m[i], true, NULL);
> >> }
> >> -
> >> - if ( d->arch.paging.total_pages != 0 )
> >> - hap_teardown(d, NULL);
> >> -
> >> - p2m_teardown(p2m_get_hostp2m(d), true, NULL);
> >> - /* Free any memory that the p2m teardown released */
> >> - paging_lock(d);
> >> - hap_set_allocation(d, 0, NULL);
> >> - ASSERT(d->arch.paging.p2m_pages == 0);
> >> - ASSERT(d->arch.paging.free_pages == 0);
> >> - ASSERT(d->arch.paging.total_pages == 0);
> >> - paging_unlock(d);
> >> }
> >>
> >> void hap_vcpu_teardown(struct vcpu *v)
> >> --- a/xen/arch/x86/mm/paging.c
> >> +++ b/xen/arch/x86/mm/paging.c
> >> @@ -842,10 +842,45 @@ int paging_teardown(struct domain *d)
> >> /* Call once all of the references to the domain have gone away */
> >> void paging_final_teardown(struct domain *d)
> >> {
> >> - if ( hap_enabled(d) )
> >> + bool hap = hap_enabled(d);
> >> +
> >> + PAGING_PRINTK("%pd start: total = %u, free = %u, p2m = %u\n",
> >> + d, d->arch.paging.total_pages,
> >> + d->arch.paging.free_pages, d->arch.paging.p2m_pages);
> >> +
> >> + if ( hap )
> >> hap_final_teardown(d);
> >> +
> >> + /*
> >> + * Remove remaining paging memory. This can be nonzero on certain error
> >> + * paths.
> >> + */
> >> + if ( d->arch.paging.total_pages )
> >> + {
> >> + if ( hap )
> >> + hap_teardown(d, NULL);
> >> + else
> >> + shadow_teardown(d, NULL);
> >
> > For a logical PoV, shouldn't hap_teardown() be called before
> > hap_final_teardown()?
>
> Yes and no: The meaning of "final" has changed - previously it meant "the
> final parts of tearing down" while now it means "the parts of tearing
> down which must be done during final cleanup". I can't think of a better
> name, so I left "hap_final_teardown" as it was.
>
> > Also hap_final_teardown() already contains a call to hap_teardown() if
> > total_pages != 0, so this is just redundant in the HAP case?
>
> Well, like in shadow_final_teardown() there was such a call prior to this
> change, but there's none left now.
>
> > Maybe we want to pull that hap_teardown() out of hap_final_teardown()
>
> That's what I'm doing here.
Oh, sorry, I've missed that chunk. Then:
Reviewed-by: Roger Pau Monné <roge.rpau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown
2023-03-16 13:28 ` Roger Pau Monné
@ 2023-03-16 13:35 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-03-16 13:35 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Tim Deegan
On 16.03.2023 14:28, Roger Pau Monné wrote:
> On Thu, Mar 16, 2023 at 01:57:45PM +0100, Jan Beulich wrote:
>> On 16.03.2023 13:24, Roger Pau Monné wrote:
>>> Maybe we want to pull that hap_teardown() out of hap_final_teardown()
>>
>> That's what I'm doing here.
>
> Oh, sorry, I've missed that chunk. Then:
>
> Reviewed-by: Roger Pau Monné <roge.rpau@citrix.com>
Thanks. I'll take the liberty and swap '.' with 'r' ...
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-16 13:35 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09 13:38 [PATCH v2 0/5] x86/mm: address aspects noticed during XSA-410 work Jan Beulich
2023-01-09 13:39 ` [PATCH v2 1/5] x86/paging: fold most HAP and shadow final teardown Jan Beulich
2023-03-16 12:24 ` Roger Pau Monné
2023-03-16 12:57 ` Jan Beulich
2023-03-16 13:28 ` Roger Pau Monné
2023-03-16 13:35 ` Jan Beulich
2023-01-09 13:39 ` [PATCH v2 2/5] x86/paging: drop set-allocation from final-teardown Jan Beulich
2023-03-16 12:34 ` Roger Pau Monné
2023-01-09 13:40 ` [PATCH v2 3/5] x86/paging: move update_paging_modes() hook Jan Beulich
2023-03-16 12:39 ` Roger Pau Monné
2023-01-09 13:41 ` [PATCH v2 4/5] x86/paging: move and conditionalize flush_tlb() hook Jan Beulich
2023-01-09 13:41 ` [PATCH v2 5/5] x86/shadow: drop zero initialization from shadow_domain_init() Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).