* [PATCH 0/5] x86: follow-on to XSA-408
@ 2022-12-06 13:51 Jan Beulich
2022-12-06 13:53 ` [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables Jan Beulich
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:51 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap
Originally just a single patch (#3 in this series) plus an observation,
this has now grown to five patches, largely as a result of discussing
both.
1: tboot: drop failed attempt to hash shadow page tables
2: mm: rename PGC_page_table to PGC_shadowed_pt
3: mm: PGC_shadowed_pt is used by shadow code only
4: tboot: correct IOMMU (VT-d) interaction
5: tboot: actually wipe contexts
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables
2022-12-06 13:51 [PATCH 0/5] x86: follow-on to XSA-408 Jan Beulich
@ 2022-12-06 13:53 ` Jan Beulich
2022-12-06 13:58 ` Jan Beulich
2022-12-06 13:53 ` [PATCH 2/5] x86/mm: rename PGC_page_table to PGC_shadowed_pt Jan Beulich
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap
While plausible to do what was intended based on the name of the flag
(PGC_page_table), that name was misleading and is going to be changed.
It marks page tables pages _having_ a shadow, not shadows of page table
pages. The attempt also didn't cover the HAP case at all, and it
constituted a potentially very long loop doing nothing when
!SHADOW_PAGING. Instead leave a comment of what actually wants doing
there (which then also may need to account for e.g. the risk of A/D bits
becoming set behind our backs).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -177,29 +177,6 @@ static void update_iommu_mac(vmac_ctx_t
#define is_page_in_use(page) \
(page_state_is(page, inuse) || page_state_is(page, offlining))
-static void update_pagetable_mac(vmac_ctx_t *ctx)
-{
- unsigned long mfn;
-
- for ( mfn = 0; mfn < max_page; mfn++ )
- {
- struct page_info *page = mfn_to_page(_mfn(mfn));
-
- if ( !mfn_valid(_mfn(mfn)) )
- continue;
- if ( is_page_in_use(page) && !is_special_page(page) )
- {
- if ( page->count_info & PGC_page_table )
- {
- void *pg = map_domain_page(_mfn(mfn));
-
- vmac_update(pg, PAGE_SIZE, ctx);
- unmap_domain_page(pg);
- }
- }
- }
-}
-
static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE],
vmac_t *mac)
{
@@ -233,8 +210,7 @@ static void tboot_gen_domain_integrity(c
}
}
- /* MAC all shadow page tables */
- update_pagetable_mac(&ctx);
+ /* TODO: MAC all shadow / HAP page tables */
*mac = vmac(NULL, 0, nonce, NULL, &ctx);
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/5] x86/mm: rename PGC_page_table to PGC_shadowed_pt
2022-12-06 13:51 [PATCH 0/5] x86: follow-on to XSA-408 Jan Beulich
2022-12-06 13:53 ` [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables Jan Beulich
@ 2022-12-06 13:53 ` Jan Beulich
2022-12-06 14:08 ` Andrew Cooper
2022-12-06 13:54 ` [PATCH 3/5] x86/mm: PGC_shadowed_pt is used by shadow code only Jan Beulich
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap
The original name didn't express the purpose of the flag: It is being
set once a page table page obtains a shadow, and it is removed when the
last shadow of a page was destroyed.
In set_tlbflush_timestamp() also remove the 2nd half of the condition as
being redundant (PGC_shadowed_pt can't be set on a page without shadow
mode being enabled on the owning domain).
Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -70,9 +70,9 @@
/* Page is Xen heap? */
#define _PGC_xen_heap PG_shift(2)
#define PGC_xen_heap PG_mask(1, 2)
- /* Set when is using a page as a page table */
-#define _PGC_page_table PG_shift(3)
-#define PGC_page_table PG_mask(1, 3)
+ /* Set when a page table page has been shadowed. */
+#define _PGC_shadowed_pt PG_shift(3)
+#define PGC_shadowed_pt PG_mask(1, 3)
/* Page is broken? */
#define _PGC_broken PG_shift(4)
#define PGC_broken PG_mask(1, 4)
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -803,7 +803,7 @@ p2m_pod_zero_check_superpage(struct p2m_
for ( k = 0, page = mfn_to_page(mfn); k < n; ++k, ++page )
if ( is_special_page(page) ||
!(page->count_info & PGC_allocated) ||
- (page->count_info & PGC_page_table) ||
+ (page->count_info & PGC_shadowed_pt) ||
(page->count_info & PGC_count_mask) > max_ref )
goto out;
}
@@ -946,7 +946,7 @@ p2m_pod_zero_check(struct p2m_domain *p2
if ( !is_special_page(pg) &&
(pg->count_info & PGC_allocated) &&
- !(pg->count_info & PGC_page_table) &&
+ !(pg->count_info & PGC_shadowed_pt) &&
((pg->count_info & PGC_count_mask) <= max_ref) )
map[i] = map_domain_page(mfns[i]);
}
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -185,7 +185,7 @@ static void sh_oos_audit(struct domain *
BUG();
}
pg = mfn_to_page(oos[idx]);
- if ( !(pg->count_info & PGC_page_table) )
+ if ( !(pg->count_info & PGC_shadowed_pt) )
{
printk("%s: idx %x gmfn %lx not a pt (count %lx)\n",
__func__, idx, mfn_x(oos[idx]), pg->count_info);
@@ -716,7 +716,7 @@ void shadow_promote(struct domain *d, mf
|| d->is_shutting_down);
/* Is the page already shadowed? */
- if ( !test_and_set_bit(_PGC_page_table, &page->count_info) )
+ if ( !test_and_set_bit(_PGC_shadowed_pt, &page->count_info) )
{
page->shadow_flags = 0;
#ifdef CONFIG_HVM
@@ -734,7 +734,7 @@ void shadow_demote(struct domain *d, mfn
{
struct page_info *page = mfn_to_page(gmfn);
- ASSERT(test_bit(_PGC_page_table, &page->count_info));
+ ASSERT(test_bit(_PGC_shadowed_pt, &page->count_info));
ASSERT(page->shadow_flags & (1u << type));
page->shadow_flags &= ~(1u << type);
@@ -748,7 +748,7 @@ void shadow_demote(struct domain *d, mfn
oos_hash_remove(d, gmfn);
}
#endif
- clear_bit(_PGC_page_table, &page->count_info);
+ clear_bit(_PGC_shadowed_pt, &page->count_info);
}
TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_DEMOTE);
@@ -779,7 +779,7 @@ sh_validate_guest_entry(struct vcpu *v,
// Ditto for L2s before L3s, etc.
//
- if ( !(page->count_info & PGC_page_table) )
+ if ( !(page->count_info & PGC_shadowed_pt) )
return 0; /* Not shadowed at all */
if ( page->shadow_flags & SHF_L1_32 )
@@ -2266,7 +2266,7 @@ void sh_remove_shadows(struct domain *d,
SHADOW_PRINTK("d%d gmfn=%"PRI_mfn"\n", d->domain_id, mfn_x(gmfn));
/* Bail out now if the page is not shadowed */
- if ( (pg->count_info & PGC_page_table) == 0 )
+ if ( !(pg->count_info & PGC_shadowed_pt) )
{
paging_unlock(d);
return;
@@ -2283,7 +2283,7 @@ void sh_remove_shadows(struct domain *d,
*/
#define DO_UNSHADOW(_type) do { \
t = (_type); \
- if ( !(pg->count_info & PGC_page_table) || \
+ if ( !(pg->count_info & PGC_shadowed_pt) || \
!(pg->shadow_flags & (1 << t)) ) \
break; \
smfn = shadow_hash_lookup(d, mfn_x(gmfn), t); \
@@ -2299,7 +2299,7 @@ void sh_remove_shadows(struct domain *d,
else if ( sh_type_has_up_pointer(d, t) ) \
sh_remove_shadow_via_pointer(d, smfn); \
if ( !fast && \
- (pg->count_info & PGC_page_table) && \
+ (pg->count_info & PGC_shadowed_pt) && \
(pg->shadow_flags & (1 << t)) ) \
{ \
HASH_CALLBACKS_CHECK(SHF_page_type_mask); \
@@ -2322,7 +2322,7 @@ void sh_remove_shadows(struct domain *d,
#undef DO_UNSHADOW
/* If that didn't catch the shadows, something is wrong */
- if ( !fast && all && (pg->count_info & PGC_page_table) )
+ if ( !fast && all && (pg->count_info & PGC_shadowed_pt) )
{
printk(XENLOG_G_ERR "can't find all shadows of mfn %"PRI_mfn
" (shadow_flags=%04x)\n", mfn_x(gmfn), pg->shadow_flags);
@@ -2339,7 +2339,7 @@ void sh_remove_shadows(struct domain *d,
void shadow_prepare_page_type_change(struct domain *d,
const struct page_info *page)
{
- if ( !(page->count_info & PGC_page_table) )
+ if ( !(page->count_info & PGC_shadowed_pt) )
return;
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -320,7 +320,7 @@ static inline void sh_terminate_list(str
static inline int sh_page_has_multiple_shadows(struct page_info *pg)
{
u32 shadows;
- if ( !(pg->count_info & PGC_page_table) )
+ if ( !(pg->count_info & PGC_shadowed_pt) )
return 0;
shadows = pg->shadow_flags & SHF_page_type_mask;
/* More than one type bit set in shadow-flags? */
@@ -332,7 +332,7 @@ static inline int sh_page_has_multiple_s
* domain is translated, &c */
static inline int page_is_out_of_sync(struct page_info *p)
{
- return (p->count_info & PGC_page_table)
+ return (p->count_info & PGC_shadowed_pt)
&& (p->shadow_flags & SHF_out_of_sync);
}
@@ -343,7 +343,7 @@ static inline int mfn_is_out_of_sync(mfn
static inline int page_oos_may_write(struct page_info *p)
{
- return (p->count_info & PGC_page_table)
+ return (p->count_info & PGC_shadowed_pt)
&& (p->shadow_flags & SHF_oos_may_write);
}
@@ -545,7 +545,7 @@ sh_mfn_is_a_page_table(mfn_t gmfn)
owner = page_get_owner(page);
if ( owner && shadow_mode_refcounts(owner)
- && (page->count_info & PGC_page_table) )
+ && (page->count_info & PGC_shadowed_pt) )
return 1;
type_info = page->u.inuse.type_info & PGT_type_mask;
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -593,8 +593,7 @@ static inline void set_tlbflush_timestam
* 2. Shadow mode reuses this field for shadowed page tables to store
* flags info -- we don't want to conflict with that.
*/
- if ( !(page->count_info & PGC_page_table) ||
- !shadow_mode_enabled(page_get_owner(page)) )
+ if ( !(page->count_info & PGC_shadowed_pt) )
page_set_tlbflush_timestamp(page);
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] x86/mm: PGC_shadowed_pt is used by shadow code only
2022-12-06 13:51 [PATCH 0/5] x86: follow-on to XSA-408 Jan Beulich
2022-12-06 13:53 ` [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables Jan Beulich
2022-12-06 13:53 ` [PATCH 2/5] x86/mm: rename PGC_page_table to PGC_shadowed_pt Jan Beulich
@ 2022-12-06 13:54 ` Jan Beulich
2022-12-09 21:48 ` Andrew Cooper
2022-12-06 13:56 ` [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction Jan Beulich
2022-12-06 13:57 ` [PATCH 5/5] x86/tboot: actually wipe contexts Jan Beulich
4 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:54 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap
By defining the constant to zero when !SHADOW_PAGING we give compilers
the chance to eliminate a little more dead code elsewhere in the tree.
Plus, as a minor benefit, the general reference count can be one bit
wider. (To simplify things, have PGC_shadowed_pt change places with
PGC_extra.)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -70,9 +70,9 @@
/* Page is Xen heap? */
#define _PGC_xen_heap PG_shift(2)
#define PGC_xen_heap PG_mask(1, 2)
- /* Set when a page table page has been shadowed. */
-#define _PGC_shadowed_pt PG_shift(3)
-#define PGC_shadowed_pt PG_mask(1, 3)
+ /* Page is not reference counted */
+#define _PGC_extra PG_shift(3)
+#define PGC_extra PG_mask(1, 3)
/* Page is broken? */
#define _PGC_broken PG_shift(4)
#define PGC_broken PG_mask(1, 4)
@@ -83,12 +83,20 @@
#define PGC_state_offlined PG_mask(2, 6)
#define PGC_state_free PG_mask(3, 6)
#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
-/* Page is not reference counted */
-#define _PGC_extra PG_shift(7)
-#define PGC_extra PG_mask(1, 7)
+#ifdef CONFIG_SHADOW_PAGING
+ /* Set when a page table page has been shadowed. */
+#define _PGC_shadowed_pt PG_shift(7)
+#define PGC_shadowed_pt PG_mask(1, 7)
+#else
+#define PGC_shadowed_pt 0
+#endif
/* Count of references to this frame. */
+#if PGC_shadowed_pt
#define PGC_count_width PG_shift(7)
+#else
+#define PGC_count_width PG_shift(6)
+#endif
#define PGC_count_mask ((1UL<<PGC_count_width)-1)
/*
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction
2022-12-06 13:51 [PATCH 0/5] x86: follow-on to XSA-408 Jan Beulich
` (2 preceding siblings ...)
2022-12-06 13:54 ` [PATCH 3/5] x86/mm: PGC_shadowed_pt is used by shadow code only Jan Beulich
@ 2022-12-06 13:56 ` Jan Beulich
2022-12-09 19:52 ` Jason Andryuk
2022-12-09 21:49 ` Andrew Cooper
2022-12-06 13:57 ` [PATCH 5/5] x86/tboot: actually wipe contexts Jan Beulich
4 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:56 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné,
George Dunlap, Lukasz Hawrylko, Daniel P. Smith,
Mateusz Mówka
First of all using is_idle_domain() on the subject domain in the body of
for_each_domain() is pointless. Replace that conditional by one checking
that a domain actually has IOMMU support enabled for it, and that we're
actually on a VT-d system (both are largely cosmetic / documentary with
how things work elsewhere, but still).
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -31,6 +31,8 @@ static vmac_t frametable_mac; /* MAC for
static uint64_t __initdata txt_heap_base, __initdata txt_heap_size;
static uint64_t __initdata sinit_base, __initdata sinit_size;
+static bool __ro_after_init is_vtd;
+
/*
* TXT configuration registers (offsets from TXT_{PUB, PRIV}_CONFIG_REGS_BASE)
*/
@@ -201,7 +203,7 @@ static void tboot_gen_domain_integrity(c
}
spin_unlock(&d->page_alloc_lock);
- if ( !is_idle_domain(d) )
+ if ( is_iommu_enabled(d) && is_vtd )
{
const struct domain_iommu *dio = dom_iommu(d);
@@ -444,6 +446,8 @@ int __init cf_check tboot_parse_dmar_tab
if ( txt_heap_base == 0 )
return 1;
+ is_vtd = true;
+
/* walk heap to SinitMleData */
pa = txt_heap_base;
/* skip BiosData */
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] x86/tboot: actually wipe contexts
2022-12-06 13:51 [PATCH 0/5] x86: follow-on to XSA-408 Jan Beulich
` (3 preceding siblings ...)
2022-12-06 13:56 ` [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction Jan Beulich
@ 2022-12-06 13:57 ` Jan Beulich
2022-12-09 19:52 ` Jason Andryuk
2022-12-09 21:53 ` Andrew Cooper
4 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:57 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné,
George Dunlap, Lukasz Hawrylko, Daniel P. Smith,
Mateusz Mówka
Especially with our use of __builtin_memset() to implement memset() the
compiler is free to eliminate instances when it can prove that the
affected object is dead. Introduce a small helper function accompanying
the memset() with a construct forcing the compiler to retain the
clearing of (stack) memory.
Fixes: c021c95498d9 ("x86: Replace our own specialised versions of memset and memcpy with")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The Fixes: tag names the commit which broke the wrong assumption made by
6deab1ae316b ("txt: perform per-domain (and frametable and xenheap) MAC
on entry into").
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -179,6 +179,17 @@ static void update_iommu_mac(vmac_ctx_t
#define is_page_in_use(page) \
(page_state_is(page, inuse) || page_state_is(page, offlining))
+/* Wipe ctx to ensure key is not left in memory. */
+static void wipe_ctx(vmac_ctx_t *ctx)
+{
+ memset(ctx, 0, sizeof(*ctx));
+ /*
+ * Make sure the compiler won't optimize out the memset(), for the local
+ * variable (at the call sites) going out of scope right afterwards.
+ */
+ asm volatile ( "" :: "m" (*ctx) );
+}
+
static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE],
vmac_t *mac)
{
@@ -216,8 +227,7 @@ static void tboot_gen_domain_integrity(c
*mac = vmac(NULL, 0, nonce, NULL, &ctx);
- /* wipe ctx to ensure key is not left in memory */
- memset(&ctx, 0, sizeof(ctx));
+ wipe_ctx(&ctx);
}
/*
@@ -278,8 +288,7 @@ static void tboot_gen_xenheap_integrity(
}
*mac = vmac(NULL, 0, nonce, NULL, &ctx);
- /* wipe ctx to ensure key is not left in memory */
- memset(&ctx, 0, sizeof(ctx));
+ wipe_ctx(&ctx);
}
static void tboot_gen_frametable_integrity(const uint8_t key[TB_KEY_SIZE],
@@ -307,8 +316,7 @@ static void tboot_gen_frametable_integri
*mac = vmac(NULL, 0, nonce, NULL, &ctx);
- /* wipe ctx to ensure key is not left in memory */
- memset(&ctx, 0, sizeof(ctx));
+ wipe_ctx(&ctx);
}
void tboot_shutdown(uint32_t shutdown_type)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables
2022-12-06 13:53 ` [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables Jan Beulich
@ 2022-12-06 13:58 ` Jan Beulich
2022-12-09 19:51 ` Jason Andryuk
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2022-12-06 13:58 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné,
George Dunlap, Lukasz Hawrylko, Mateusz Mówka,
Daniel P. Smith
On 06.12.2022 14:53, Jan Beulich wrote:
> While plausible to do what was intended based on the name of the flag
> (PGC_page_table), that name was misleading and is going to be changed.
> It marks page tables pages _having_ a shadow, not shadows of page table
> pages. The attempt also didn't cover the HAP case at all, and it
> constituted a potentially very long loop doing nothing when
> !SHADOW_PAGING. Instead leave a comment of what actually wants doing
> there (which then also may need to account for e.g. the risk of A/D bits
> becoming set behind our backs).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
I'm sorry, I should have Cc-ed tboot reviewers here as well.
Jan
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -177,29 +177,6 @@ static void update_iommu_mac(vmac_ctx_t
> #define is_page_in_use(page) \
> (page_state_is(page, inuse) || page_state_is(page, offlining))
>
> -static void update_pagetable_mac(vmac_ctx_t *ctx)
> -{
> - unsigned long mfn;
> -
> - for ( mfn = 0; mfn < max_page; mfn++ )
> - {
> - struct page_info *page = mfn_to_page(_mfn(mfn));
> -
> - if ( !mfn_valid(_mfn(mfn)) )
> - continue;
> - if ( is_page_in_use(page) && !is_special_page(page) )
> - {
> - if ( page->count_info & PGC_page_table )
> - {
> - void *pg = map_domain_page(_mfn(mfn));
> -
> - vmac_update(pg, PAGE_SIZE, ctx);
> - unmap_domain_page(pg);
> - }
> - }
> - }
> -}
> -
> static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE],
> vmac_t *mac)
> {
> @@ -233,8 +210,7 @@ static void tboot_gen_domain_integrity(c
> }
> }
>
> - /* MAC all shadow page tables */
> - update_pagetable_mac(&ctx);
> + /* TODO: MAC all shadow / HAP page tables */
>
> *mac = vmac(NULL, 0, nonce, NULL, &ctx);
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] x86/mm: rename PGC_page_table to PGC_shadowed_pt
2022-12-06 13:53 ` [PATCH 2/5] x86/mm: rename PGC_page_table to PGC_shadowed_pt Jan Beulich
@ 2022-12-06 14:08 ` Andrew Cooper
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2022-12-06 14:08 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monne, George Dunlap
On 06/12/2022 13:53, Jan Beulich wrote:
> The original name didn't express the purpose of the flag: It is being
> set once a page table page obtains a shadow, and it is removed when the
> last shadow of a page was destroyed.
>
> In set_tlbflush_timestamp() also remove the 2nd half of the condition as
> being redundant (PGC_shadowed_pt can't be set on a page without shadow
> mode being enabled on the owning domain).
>
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables
2022-12-06 13:58 ` Jan Beulich
@ 2022-12-09 19:51 ` Jason Andryuk
0 siblings, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2022-12-09 19:51 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné,
George Dunlap, Lukasz Hawrylko, Mateusz Mówka,
Daniel P. Smith
On Tue, Dec 6, 2022 at 8:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 06.12.2022 14:53, Jan Beulich wrote:
> > While plausible to do what was intended based on the name of the flag
> > (PGC_page_table), that name was misleading and is going to be changed.
> > It marks page tables pages _having_ a shadow, not shadows of page table
> > pages. The attempt also didn't cover the HAP case at all, and it
> > constituted a potentially very long loop doing nothing when
> > !SHADOW_PAGING. Instead leave a comment of what actually wants doing
> > there (which then also may need to account for e.g. the risk of A/D bits
> > becoming set behind our backs).
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction
2022-12-06 13:56 ` [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction Jan Beulich
@ 2022-12-09 19:52 ` Jason Andryuk
2022-12-09 21:49 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2022-12-09 19:52 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné,
George Dunlap, Lukasz Hawrylko, Daniel P. Smith,
Mateusz Mówka
On Tue, Dec 6, 2022 at 8:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> First of all using is_idle_domain() on the subject domain in the body of
> for_each_domain() is pointless. Replace that conditional by one checking
> that a domain actually has IOMMU support enabled for it, and that we're
> actually on a VT-d system (both are largely cosmetic / documentary with
> how things work elsewhere, but still).
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] x86/tboot: actually wipe contexts
2022-12-06 13:57 ` [PATCH 5/5] x86/tboot: actually wipe contexts Jan Beulich
@ 2022-12-09 19:52 ` Jason Andryuk
2022-12-09 21:53 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Jason Andryuk @ 2022-12-09 19:52 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné,
George Dunlap, Lukasz Hawrylko, Daniel P. Smith,
Mateusz Mówka
On Tue, Dec 6, 2022 at 8:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Especially with our use of __builtin_memset() to implement memset() the
> compiler is free to eliminate instances when it can prove that the
> affected object is dead. Introduce a small helper function accompanying
> the memset() with a construct forcing the compiler to retain the
> clearing of (stack) memory.
>
> Fixes: c021c95498d9 ("x86: Replace our own specialised versions of memset and memcpy with")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] x86/mm: PGC_shadowed_pt is used by shadow code only
2022-12-06 13:54 ` [PATCH 3/5] x86/mm: PGC_shadowed_pt is used by shadow code only Jan Beulich
@ 2022-12-09 21:48 ` Andrew Cooper
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2022-12-09 21:48 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monne, George Dunlap
On 06/12/2022 13:54, Jan Beulich wrote:
> By defining the constant to zero when !SHADOW_PAGING we give compilers
> the chance to eliminate a little more dead code elsewhere in the tree.
> Plus, as a minor benefit, the general reference count can be one bit
> wider. (To simplify things, have PGC_shadowed_pt change places with
> PGC_extra.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity is going to complain some more. I've still not figured out a
way to get it to ignore "AND with something that's
compile-time-conditionally-0", but we have this pattern elsewhere.
~Andrew
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction
2022-12-06 13:56 ` [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction Jan Beulich
2022-12-09 19:52 ` Jason Andryuk
@ 2022-12-09 21:49 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2022-12-09 21:49 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Wei Liu, Roger Pau Monne, George Dunlap, Lukasz Hawrylko,
Daniel P. Smith, Mateusz Mówka
On 06/12/2022 13:56, Jan Beulich wrote:
> First of all using is_idle_domain() on the subject domain in the body of
> for_each_domain() is pointless. Replace that conditional by one checking
> that a domain actually has IOMMU support enabled for it, and that we're
> actually on a VT-d system (both are largely cosmetic / documentary with
> how things work elsewhere, but still).
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] x86/tboot: actually wipe contexts
2022-12-06 13:57 ` [PATCH 5/5] x86/tboot: actually wipe contexts Jan Beulich
2022-12-09 19:52 ` Jason Andryuk
@ 2022-12-09 21:53 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2022-12-09 21:53 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Wei Liu, Roger Pau Monne, George Dunlap, Lukasz Hawrylko,
Daniel P. Smith, Mateusz Mówka
On 06/12/2022 13:57, Jan Beulich wrote:
> Especially with our use of __builtin_memset() to implement memset() the
> compiler is free to eliminate instances when it can prove that the
> affected object is dead. Introduce a small helper function accompanying
> the memset() with a construct forcing the compiler to retain the
> clearing of (stack) memory.
>
> Fixes: c021c95498d9 ("x86: Replace our own specialised versions of memset and memcpy with")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Thanks. I'd noticed this before wanted to do something about it.
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-12-09 21:53 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-06 13:51 [PATCH 0/5] x86: follow-on to XSA-408 Jan Beulich
2022-12-06 13:53 ` [PATCH 1/5] x86/tboot: drop failed attempt to hash shadow page tables Jan Beulich
2022-12-06 13:58 ` Jan Beulich
2022-12-09 19:51 ` Jason Andryuk
2022-12-06 13:53 ` [PATCH 2/5] x86/mm: rename PGC_page_table to PGC_shadowed_pt Jan Beulich
2022-12-06 14:08 ` Andrew Cooper
2022-12-06 13:54 ` [PATCH 3/5] x86/mm: PGC_shadowed_pt is used by shadow code only Jan Beulich
2022-12-09 21:48 ` Andrew Cooper
2022-12-06 13:56 ` [PATCH 4/5] x86/tboot: correct IOMMU (VT-d) interaction Jan Beulich
2022-12-09 19:52 ` Jason Andryuk
2022-12-09 21:49 ` Andrew Cooper
2022-12-06 13:57 ` [PATCH 5/5] x86/tboot: actually wipe contexts Jan Beulich
2022-12-09 19:52 ` Jason Andryuk
2022-12-09 21:53 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.