diff mbox series

[15/17] x86/shadow: drop SH_type_l2h_pae_shadow

Message ID c412bbe4-d555-7d36-997c-92274679d9ae@suse.com (mailing list archive)
State New, archived
Headers show
Series x86/PV: avoid speculation abuse through guest accessors plus ... | expand

Commit Message

Jan Beulich Jan. 14, 2021, 3:10 p.m. UTC 
This is a remnant from 32-bit days, having no place anymore where a
shadow of this type would be created.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
hash_{domain,vcpu}_foreach() have a use each of literal 15. It's not
clear to me what the proper replacement constant would be, as it
doesn't look as if SH_type_monitor_table was meant.

Comments

Tim Deegan Jan. 22, 2021, 1:11 p.m. UTC | #1 
Hi,

At 16:10 +0100 on 14 Jan (1610640627), Jan Beulich wrote:
> This is a remnant from 32-bit days, having no place anymore where a
> shadow of this type would be created.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> hash_{domain,vcpu}_foreach() have a use each of literal 15. It's not
> clear to me what the proper replacement constant would be, as it
> doesn't look as if SH_type_monitor_table was meant.

Good spot.  I think the '<= 15' should be replaced with '< SH_type_unused'.
It originally matched the callback arrays all being coded as
"static hash_callback_t callbacks[16]".

Cheers,

Tim
Jan Beulich Jan. 22, 2021, 4:31 p.m. UTC | #2 
On 22.01.2021 14:11, Tim Deegan wrote:
> At 16:10 +0100 on 14 Jan (1610640627), Jan Beulich wrote:
>> This is a remnant from 32-bit days, having no place anymore where a
>> shadow of this type would be created.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> hash_{domain,vcpu}_foreach() have a use each of literal 15. It's not
>> clear to me what the proper replacement constant would be, as it
>> doesn't look as if SH_type_monitor_table was meant.
> 
> Good spot.  I think the '<= 15' should be replaced with '< SH_type_unused'.
> It originally matched the callback arrays all being coded as
> "static hash_callback_t callbacks[16]".

So are you saying this was off by one then prior to this patch
(when SH_type_unused was still 17), albeit in apparently a
benign way? And the comments in sh_remove_write_access(),
sh_remove_all_mappings(), sh_remove_shadows(), and
sh_reset_l3_up_pointers() are then wrong as well, and would
instead better be like in shadow_audit_tables()?

Because of this having been benign (due to none of the callback
tables specifying non-NULL entries there), wouldn't it make
sense to dimension the tables by SH_type_max_shadow + 1 only?
Or would you consider this too risky?

In any event I guess I'll make the patch addressing this (one
way or the other) a prereq to everything here.

Jan
Tim Deegan Jan. 22, 2021, 8:02 p.m. UTC | #3 
Hi,

At 17:31 +0100 on 22 Jan (1611336662), Jan Beulich wrote:
> On 22.01.2021 14:11, Tim Deegan wrote:
> > At 16:10 +0100 on 14 Jan (1610640627), Jan Beulich wrote:
> >> hash_{domain,vcpu}_foreach() have a use each of literal 15. It's not
> >> clear to me what the proper replacement constant would be, as it
> >> doesn't look as if SH_type_monitor_table was meant.
> > 
> > Good spot.  I think the '<= 15' should be replaced with '< SH_type_unused'.
> > It originally matched the callback arrays all being coded as
> > "static hash_callback_t callbacks[16]".
> 
> So are you saying this was off by one then prior to this patch
> (when SH_type_unused was still 17), albeit in apparently a
> benign way?

Yes - this assertion is just to catch overruns of the callback table,
and so it was OK for its limit to be too low.  The new types that were
added since then are never put in the hash table, so don't trigger
this assertion.

> And the comments in sh_remove_write_access(),
> sh_remove_all_mappings(), sh_remove_shadows(), and
> sh_reset_l3_up_pointers() are then wrong as well, and would
> instead better be like in shadow_audit_tables()?

Yes, it looks like those comments are also out of date where they
mention 'unused'.

> Because of this having been benign (due to none of the callback
> tables specifying non-NULL entries there), wouldn't it make
> sense to dimension the tables by SH_type_max_shadow + 1 only?
> Or would you consider this too risky?

Yes, I think that would be fine, also changing '<= 15' to
'<= SH_type_max_shadow'.  Maybe add a matching
ASSERT(t <= SH_type_max_shadow) in shadow_hash_insert as well?

Cheers,

Tim.
Jan Beulich Jan. 25, 2021, 11:09 a.m. UTC | #4 
On 22.01.2021 21:02, Tim Deegan wrote:
> At 17:31 +0100 on 22 Jan (1611336662), Jan Beulich wrote:
>> On 22.01.2021 14:11, Tim Deegan wrote:
>>> At 16:10 +0100 on 14 Jan (1610640627), Jan Beulich wrote:
>>>> hash_{domain,vcpu}_foreach() have a use each of literal 15. It's not
>>>> clear to me what the proper replacement constant would be, as it
>>>> doesn't look as if SH_type_monitor_table was meant.
>>>
>>> Good spot.  I think the '<= 15' should be replaced with '< SH_type_unused'.
>>> It originally matched the callback arrays all being coded as
>>> "static hash_callback_t callbacks[16]".
>>
>> So are you saying this was off by one then prior to this patch
>> (when SH_type_unused was still 17), albeit in apparently a
>> benign way?
> 
> Yes - this assertion is just to catch overruns of the callback table,
> and so it was OK for its limit to be too low.  The new types that were
> added since then are never put in the hash table, so don't trigger
> this assertion.
> 
>> And the comments in sh_remove_write_access(),
>> sh_remove_all_mappings(), sh_remove_shadows(), and
>> sh_reset_l3_up_pointers() are then wrong as well, and would
>> instead better be like in shadow_audit_tables()?
> 
> Yes, it looks like those comments are also out of date where they
> mention 'unused'.

For this, which likely will end up being part of ...

>> Because of this having been benign (due to none of the callback
>> tables specifying non-NULL entries there), wouldn't it make
>> sense to dimension the tables by SH_type_max_shadow + 1 only?
>> Or would you consider this too risky?
> 
> Yes, I think that would be fine, also changing '<= 15' to
> '<= SH_type_max_shadow'.  Maybe add a matching
> ASSERT(t <= SH_type_max_shadow) in shadow_hash_insert as well?

... this, I'll send a patch for 4.16 going beyond the more
immediate one sent, which I'll ask Ian to consider for 4.15
(assuming of course you consider it okay in the first place).

Jan
Jan Beulich Jan. 25, 2021, 11:33 a.m. UTC | #5 
On 22.01.2021 21:02, Tim Deegan wrote:
> At 17:31 +0100 on 22 Jan (1611336662), Jan Beulich wrote:
>> Because of this having been benign (due to none of the callback
>> tables specifying non-NULL entries there), wouldn't it make
>> sense to dimension the tables by SH_type_max_shadow + 1 only?
>> Or would you consider this too risky?
> 
> Yes, I think that would be fine, also changing '<= 15' to
> '<= SH_type_max_shadow'.  Maybe add a matching
> ASSERT(t <= SH_type_max_shadow) in shadow_hash_insert as well?

The latter (also for shadow_hash_delete()) would seem kind
of orthogonal to me, but for now I've put it in there.

Jan
diff mbox series

Patch

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -792,9 +792,6 @@  sh_validate_guest_entry(struct vcpu *v,
     if ( page->shadow_flags & SHF_L2_PAE )
         result |= SHADOW_INTERNAL_NAME(sh_map_and_validate_gl2e, 3)
             (v, gmfn, entry, size);
-    if ( page->shadow_flags & SHF_L2H_PAE )
-        result |= SHADOW_INTERNAL_NAME(sh_map_and_validate_gl2he, 3)
-            (v, gmfn, entry, size);
 
     if ( page->shadow_flags & SHF_L1_64 )
         result |= SHADOW_INTERNAL_NAME(sh_map_and_validate_gl1e, 4)
@@ -859,7 +856,6 @@  const u8 sh_type_to_size[] = {
     1, /* SH_type_l1_pae_shadow  */
     1, /* SH_type_fl1_pae_shadow */
     1, /* SH_type_l2_pae_shadow  */
-    1, /* SH_type_l2h_pae_shadow */
     1, /* SH_type_l1_64_shadow   */
     1, /* SH_type_fl1_64_shadow  */
     1, /* SH_type_l2_64_shadow   */
@@ -900,7 +896,6 @@  void shadow_unhook_mappings(struct domai
         SHADOW_INTERNAL_NAME(sh_unhook_32b_mappings, 2)(d, smfn, user_only);
         break;
     case SH_type_l2_pae_shadow:
-    case SH_type_l2h_pae_shadow:
         SHADOW_INTERNAL_NAME(sh_unhook_pae_mappings, 3)(d, smfn, user_only);
         break;
     case SH_type_l4_64_shadow:
@@ -1757,7 +1752,6 @@  void sh_destroy_shadow(struct domain *d,
         SHADOW_INTERNAL_NAME(sh_destroy_l1_shadow, 3)(d, smfn);
         break;
     case SH_type_l2_pae_shadow:
-    case SH_type_l2h_pae_shadow:
         SHADOW_INTERNAL_NAME(sh_destroy_l2_shadow, 3)(d, smfn);
         break;
 
@@ -1816,7 +1810,6 @@  int sh_remove_write_access(struct domain
         SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 3), /* l1_pae  */
         SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 3), /* fl1_pae */
         NULL, /* l2_pae  */
-        NULL, /* l2h_pae */
         SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 4), /* l1_64   */
         SHADOW_INTERNAL_NAME(sh_rm_write_access_from_l1, 4), /* fl1_64  */
         NULL, /* l2_64   */
@@ -2039,7 +2032,6 @@  int sh_remove_all_mappings(struct domain
         SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 3), /* l1_pae  */
         SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 3), /* fl1_pae */
         NULL, /* l2_pae  */
-        NULL, /* l2h_pae */
         SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 4), /* l1_64   */
         SHADOW_INTERNAL_NAME(sh_rm_mappings_from_l1, 4), /* fl1_64  */
         NULL, /* l2_64   */
@@ -2136,7 +2128,6 @@  static int sh_remove_shadow_via_pointer(
         break;
     case SH_type_l1_pae_shadow:
     case SH_type_l2_pae_shadow:
-    case SH_type_l2h_pae_shadow:
         SHADOW_INTERNAL_NAME(sh_clear_shadow_entry, 3)(d, vaddr, pmfn);
         break;
     case SH_type_l1_64_shadow:
@@ -2181,7 +2172,6 @@  void sh_remove_shadows(struct domain *d,
         NULL, /* l1_pae  */
         NULL, /* fl1_pae */
         SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 3), /* l2_pae  */
-        SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 3), /* l2h_pae */
         NULL, /* l1_64   */
         NULL, /* fl1_64  */
         SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4), /* l2_64   */
@@ -2198,10 +2188,9 @@  void sh_remove_shadows(struct domain *d,
         SHF_L2_32, /* l1_32   */
         0, /* fl1_32  */
         0, /* l2_32   */
-        SHF_L2H_PAE | SHF_L2_PAE, /* l1_pae  */
+        SHF_L2_PAE, /* l1_pae  */
         0, /* fl1_pae */
         0, /* l2_pae  */
-        0, /* l2h_pae  */
         SHF_L2H_64 | SHF_L2_64, /* l1_64   */
         0, /* fl1_64  */
         SHF_L3_64, /* l2_64   */
@@ -2261,7 +2250,6 @@  void sh_remove_shadows(struct domain *d,
 
     DO_UNSHADOW(SH_type_l2_32_shadow);
     DO_UNSHADOW(SH_type_l1_32_shadow);
-    DO_UNSHADOW(SH_type_l2h_pae_shadow);
     DO_UNSHADOW(SH_type_l2_pae_shadow);
     DO_UNSHADOW(SH_type_l1_pae_shadow);
     DO_UNSHADOW(SH_type_l4_64_shadow);
@@ -2344,7 +2332,6 @@  void sh_reset_l3_up_pointers(struct vcpu
         NULL, /* l1_pae  */
         NULL, /* fl1_pae */
         NULL, /* l2_pae  */
-        NULL, /* l2h_pae */
         NULL, /* l1_64   */
         NULL, /* fl1_64  */
         NULL, /* l2_64   */
@@ -3368,7 +3355,6 @@  void shadow_audit_tables(struct vcpu *v)
         SHADOW_INTERNAL_NAME(sh_audit_l1_table, 3),  /* l1_pae  */
         SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 3), /* fl1_pae */
         SHADOW_INTERNAL_NAME(sh_audit_l2_table, 3),  /* l2_pae  */
-        SHADOW_INTERNAL_NAME(sh_audit_l2_table, 3),  /* l2h_pae */
         SHADOW_INTERNAL_NAME(sh_audit_l1_table, 4),  /* l1_64   */
         SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 4), /* fl1_64  */
         SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4),  /* l2_64   */
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -568,7 +568,7 @@  static inline void check_for_early_unsha
          && sh_mfn_is_a_page_table(gmfn)
          && (!d->arch.paging.shadow.pagetable_dying_op ||
              !(mfn_to_page(gmfn)->shadow_flags
-               & (SHF_L2_32|SHF_L2_PAE|SHF_L2H_PAE|SHF_L4_64))) )
+               & (SHF_L2_32|SHF_L2_PAE|SHF_L4_64))) )
     {
         perfc_incr(shadow_early_unshadow);
         sh_remove_shadows(d, gmfn, 1, 0 /* Fast, can fail to unshadow */ );
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -837,8 +837,7 @@  do {
     int _i;                                                                \
     shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                        \
     ASSERT(shadow_mode_external(_dom));                                    \
-    ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_pae_shadow        \
-           || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_pae_shadow);  \
+    ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_pae_shadow);      \
     for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )                 \
     {                                                                      \
         (_sl2e) = _sp + _i;                                                \
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -177,18 +177,17 @@  extern void shadow_audit_tables(struct v
 #define SH_type_l1_pae_shadow  (4U) /* shadowing a pae L1 page */
 #define SH_type_fl1_pae_shadow (5U) /* L1 shadow for pae 2M superpg */
 #define SH_type_l2_pae_shadow  (6U) /* shadowing a pae L2-low page */
-#define SH_type_l2h_pae_shadow (7U) /* shadowing a pae L2-high page */
-#define SH_type_l1_64_shadow   (8U) /* shadowing a 64-bit L1 page */
-#define SH_type_fl1_64_shadow  (9U) /* L1 shadow for 64-bit 2M superpg */
-#define SH_type_l2_64_shadow  (10U) /* shadowing a 64-bit L2 page */
-#define SH_type_l2h_64_shadow (11U) /* shadowing a compat PAE L2 high page */
-#define SH_type_l3_64_shadow  (12U) /* shadowing a 64-bit L3 page */
-#define SH_type_l4_64_shadow  (13U) /* shadowing a 64-bit L4 page */
-#define SH_type_max_shadow    (13U)
-#define SH_type_p2m_table     (14U) /* in use as the p2m table */
-#define SH_type_monitor_table (15U) /* in use as a monitor table */
-#define SH_type_oos_snapshot  (16U) /* in use as OOS snapshot */
-#define SH_type_unused        (17U)
+#define SH_type_l1_64_shadow   (7U) /* shadowing a 64-bit L1 page */
+#define SH_type_fl1_64_shadow  (8U) /* L1 shadow for 64-bit 2M superpg */
+#define SH_type_l2_64_shadow   (9U) /* shadowing a 64-bit L2 page */
+#define SH_type_l2h_64_shadow (10U) /* shadowing a compat PAE L2 high page */
+#define SH_type_l3_64_shadow  (11U) /* shadowing a 64-bit L3 page */
+#define SH_type_l4_64_shadow  (12U) /* shadowing a 64-bit L4 page */
+#define SH_type_max_shadow    (12U)
+#define SH_type_p2m_table     (13U) /* in use as the p2m table */
+#define SH_type_monitor_table (14U) /* in use as a monitor table */
+#define SH_type_oos_snapshot  (15U) /* in use as OOS snapshot */
+#define SH_type_unused        (16U)
 
 /*
  * What counts as a pinnable shadow?
@@ -200,7 +199,6 @@  static inline int sh_type_is_pinnable(st
      * persist even when not currently in use in a guest CR3 */
     if ( t == SH_type_l2_32_shadow
          || t == SH_type_l2_pae_shadow
-         || t == SH_type_l2h_pae_shadow
          || t == SH_type_l4_64_shadow )
         return 1;
 
@@ -256,7 +254,6 @@  static inline void sh_terminate_list(str
 #define SHF_L1_PAE  (1u << SH_type_l1_pae_shadow)
 #define SHF_FL1_PAE (1u << SH_type_fl1_pae_shadow)
 #define SHF_L2_PAE  (1u << SH_type_l2_pae_shadow)
-#define SHF_L2H_PAE (1u << SH_type_l2h_pae_shadow)
 #define SHF_L1_64   (1u << SH_type_l1_64_shadow)
 #define SHF_FL1_64  (1u << SH_type_fl1_64_shadow)
 #define SHF_L2_64   (1u << SH_type_l2_64_shadow)
@@ -265,7 +262,7 @@  static inline void sh_terminate_list(str
 #define SHF_L4_64   (1u << SH_type_l4_64_shadow)
 
 #define SHF_32  (SHF_L1_32|SHF_FL1_32|SHF_L2_32)
-#define SHF_PAE (SHF_L1_PAE|SHF_FL1_PAE|SHF_L2_PAE|SHF_L2H_PAE)
+#define SHF_PAE (SHF_L1_PAE|SHF_FL1_PAE|SHF_L2_PAE)
 #define SHF_64  (SHF_L1_64|SHF_FL1_64|SHF_L2_64|SHF_L2H_64|SHF_L3_64|SHF_L4_64)
 
 #define SHF_L1_ANY  (SHF_L1_32|SHF_L1_PAE|SHF_L1_64)