All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.