diff mbox series

[07/11] x86/shadow: L2H shadow type is PV32-only

Message ID 2743393d-852d-b385-9eba-e22806b1c4af@suse.com (mailing list archive)
State Superseded
Headers show
Series x86/shadow: misc tidying | expand

Commit Message

Jan Beulich Jan. 5, 2023, 4:05 p.m. UTC 
Like for the various HVM-only types, save a little bit of code by suitably
"masking" this type out when !PV32.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wasn't really sure whether it would be worthwhile to also update the
"#else" part of shadow_size(). Doing so would be a little tricky, as the
type to return 0 for has no name right now; I'd need to move down the
#undef to allow for that. Thoughts?

In the 4-level variant of SHADOW_FOREACH_L2E() I was heavily inclined to
also pull out of the loop the entire loop invariant part of the
condition (part of which needs touching here anyway). But in the end I
guess this would better be a separate change.

Comments

Andrew Cooper Jan. 6, 2023, 1:31 a.m. UTC | #1 
On 05/01/2023 4:05 pm, Jan Beulich wrote:
> Like for the various HVM-only types, save a little bit of code by suitably
> "masking" this type out when !PV32.

add/remove: 0/1 grow/shrink: 2/4 up/down: 544/-922 (-378)
Function                                     old     new   delta
sh_map_and_validate_gl2e__guest_4            136     666    +530
sh_destroy_shadow                            289     303     +14
sh_clear_shadow_entry__guest_4               178     176      -2
sh_validate_guest_entry                      521     472     -49
sh_map_and_validate_gl2he__guest_4           136       2    -134
sh_remove_shadows                           4757    4545    -212
validate_gl2e                                525       -    -525
Total: Before=3914702, After=3914324, chg -0.01%

Marginal...

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wasn't really sure whether it would be worthwhile to also update the
> "#else" part of shadow_size(). Doing so would be a little tricky, as the
> type to return 0 for has no name right now; I'd need to move down the
> #undef to allow for that. Thoughts?

This refers to the straight deletion from sh_type_to_size[] ?

I was confused by that at first.  The shadow does have a size of 1.  Might

/*   [SH_type_l2h_64_shadow]  = 1,  PV32 only */

work better?  That leaves it clearly in there as a 1, but not needing
any further ifdefary.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -859,13 +866,12 @@ do {
>      int _i;                                                                 \
>      int _xen = !shadow_mode_external(_dom);                                 \
>      shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                         \
> -    ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow ||\
> -           mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_64_shadow);\
> +    ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);                       \
>      for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )                  \
>      {                                                                       \
>          if ( (!(_xen))                                                      \
>               || !is_pv_32bit_domain(_dom)                                   \
> -             || mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2h_64_shadow    \
> +             || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow     \

Isn't this redundant with the ASSERT_VALID_L2() now?

Per your other question, yes this desperately wants rearranging, but I
would agree with it being another patch.

I did previously play at trying to simplify the PV pagetable loops in a
similar way.  Code-gen wise, I think the L2 loops what to calculate an
upper bound which is either 512, or compat_first_slot, while the L4
loops what an "if(i == 256) i += 7; continue;" rather than having
LFENCE-ing predicates on each iteration.

~Andrew
Jan Beulich Jan. 9, 2023, 9:12 a.m. UTC | #2 
On 06.01.2023 02:31, Andrew Cooper wrote:
> On 05/01/2023 4:05 pm, Jan Beulich wrote:
>> Like for the various HVM-only types, save a little bit of code by suitably
>> "masking" this type out when !PV32.
> 
> add/remove: 0/1 grow/shrink: 2/4 up/down: 544/-922 (-378)
> Function                                     old     new   delta
> sh_map_and_validate_gl2e__guest_4            136     666    +530
> sh_destroy_shadow                            289     303     +14
> sh_clear_shadow_entry__guest_4               178     176      -2
> sh_validate_guest_entry                      521     472     -49
> sh_map_and_validate_gl2he__guest_4           136       2    -134
> sh_remove_shadows                           4757    4545    -212
> validate_gl2e                                525       -    -525
> Total: Before=3914702, After=3914324, chg -0.01%
> 
> Marginal...

Talk wasn't of only size, but also of what actually is being executed
for no gain e.g. in sh_remove_shadows(). I think "a little bit" is a
fair statement.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wasn't really sure whether it would be worthwhile to also update the
>> "#else" part of shadow_size(). Doing so would be a little tricky, as the
>> type to return 0 for has no name right now; I'd need to move down the
>> #undef to allow for that. Thoughts?
> 
> This refers to the straight deletion from sh_type_to_size[] ?

Not really, no, but ...

> I was confused by that at first.  The shadow does have a size of 1.  Might
> 
> /*   [SH_type_l2h_64_shadow]  = 1,  PV32 only */
> 
> work better?  That leaves it clearly in there as a 1, but not needing
> any further ifdefary.

... I've made this adjustment anyway. I'd like to note though that such
commenting is being disliked by Misra.

The remark is rather about shadow_size() itself which already doesn't
return the "correct" size for all of the types when !HVM, utilizing that
the returned size is of no real interest for types which aren't used in
that case (and which then also don't really exist anymore as
"distinguishable" types). Plus the connection to assertions like this

    ASSERT(pages);

in shadow_alloc(): "pages" is the return value from shadow_size(), and
I think it wouldn't be bad to trigger for "dead" types, requiring the
function to return zero for this type despite it not becoming aliased to
SH_type_unused (i.e. unlike the types unavailable when !HVM).

Which makes me notice that with HVM=y shadow_size() would probably
better use array_access_nospec(). I'll add another patch for this.

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -859,13 +866,12 @@ do {
>>      int _i;                                                                 \
>>      int _xen = !shadow_mode_external(_dom);                                 \
>>      shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                         \
>> -    ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow ||\
>> -           mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_64_shadow);\
>> +    ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);                       \
>>      for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )                  \
>>      {                                                                       \
>>          if ( (!(_xen))                                                      \
>>               || !is_pv_32bit_domain(_dom)                                   \
>> -             || mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2h_64_shadow    \
>> +             || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow     \
> 
> Isn't this redundant with the ASSERT_VALID_L2() now?

How would that be? ASSERT_VALID_L2(), when PV32=y allows for both l2 and
l2h, whereas here we strictly mean only l2. The sole reason for the change
is that the l2h constant simply doesn't exist anymore when !PV32.

> Per your other question, yes this desperately wants rearranging, but I
> would agree with it being another patch.
> 
> I did previously play at trying to simplify the PV pagetable loops in a
> similar way.  Code-gen wise, I think the L2 loops what to calculate an
> upper bound which is either 512, or compat_first_slot, while the L4
> loops what an "if(i == 256) i += 7; continue;" rather than having
> LFENCE-ing predicates on each iteration.

I'll see what I can do without getting distracted too much. (I also guess
you mean "i += 15".)

Jan
diff mbox series

Patch

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1740,9 +1740,11 @@  void sh_destroy_shadow(struct domain *d,
     case SH_type_fl1_64_shadow:
         SHADOW_INTERNAL_NAME(sh_destroy_l1_shadow, 4)(d, smfn);
         break;
+#ifdef CONFIG_PV32
     case SH_type_l2h_64_shadow:
         ASSERT(is_pv_32bit_domain(d));
         /* Fall through... */
+#endif
     case SH_type_l2_64_shadow:
         SHADOW_INTERNAL_NAME(sh_destroy_l2_shadow, 4)(d, smfn);
         break;
@@ -2099,7 +2101,9 @@  static int sh_remove_shadow_via_pointer(
 #endif
     case SH_type_l1_64_shadow:
     case SH_type_l2_64_shadow:
+#ifdef CONFIG_PV32
     case SH_type_l2h_64_shadow:
+#endif
     case SH_type_l3_64_shadow:
     case SH_type_l4_64_shadow:
         SHADOW_INTERNAL_NAME(sh_clear_shadow_entry, 4)(d, vaddr, pmfn);
@@ -2137,7 +2141,9 @@  void sh_remove_shadows(struct domain *d,
         [SH_type_l2_pae_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 3),
 #endif
         [SH_type_l2_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4),
+#ifdef CONFIG_PV32
         [SH_type_l2h_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l1_shadow, 4),
+#endif
         [SH_type_l3_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l2_shadow, 4),
         [SH_type_l4_64_shadow] = SHADOW_INTERNAL_NAME(sh_remove_l3_shadow, 4),
     };
@@ -2150,7 +2156,9 @@  void sh_remove_shadows(struct domain *d,
 #endif
         [SH_type_l1_64_shadow] = SHF_L2H_64 | SHF_L2_64,
         [SH_type_l2_64_shadow] = SHF_L3_64,
+#ifdef CONFIG_PV32
         [SH_type_l2h_64_shadow] = SHF_L3_64,
+#endif
         [SH_type_l3_64_shadow] = SHF_L4_64,
     };
 
@@ -2214,7 +2222,9 @@  void sh_remove_shadows(struct domain *d,
 #endif
     DO_UNSHADOW(SH_type_l4_64_shadow);
     DO_UNSHADOW(SH_type_l3_64_shadow);
+#ifdef CONFIG_PV32
     DO_UNSHADOW(SH_type_l2h_64_shadow);
+#endif
     DO_UNSHADOW(SH_type_l2_64_shadow);
     DO_UNSHADOW(SH_type_l1_64_shadow);
 
@@ -3179,7 +3189,9 @@  void shadow_audit_tables(struct vcpu *v)
         [SH_type_l1_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l1_table, 4),
         [SH_type_fl1_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_fl1_table, 4),
         [SH_type_l2_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4),
+# ifdef CONFIG_PV32
         [SH_type_l2h_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l2_table, 4),
+# endif
         [SH_type_l3_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l3_table, 4),
         [SH_type_l4_64_shadow] = SHADOW_INTERNAL_NAME(sh_audit_l4_table, 4),
 #endif
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -56,7 +56,6 @@  const uint8_t sh_type_to_size[] = {
     [SH_type_l1_64_shadow]   = 1,
     [SH_type_fl1_64_shadow]  = 1,
     [SH_type_l2_64_shadow]   = 1,
-    [SH_type_l2h_64_shadow]  = 1,
     [SH_type_l3_64_shadow]   = 1,
     [SH_type_l4_64_shadow]   = 1,
     [SH_type_p2m_table]      = 1,
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -97,6 +97,13 @@  static void sh_flush_local(const struct
     flush_local(guest_flush_tlb_flags(d));
 }
 
+#if GUEST_PAGING_LEVELS >= 4 && defined(CONFIG_PV32)
+#define ASSERT_VALID_L2(t) \
+    ASSERT((t) == SH_type_l2_shadow || (t) == SH_type_l2h_shadow)
+#else
+#define ASSERT_VALID_L2(t) ASSERT((t) == SH_type_l2_shadow)
+#endif
+
 /**************************************************************************/
 /* Hash table mapping from guest pagetables to shadows
  *
@@ -337,7 +344,7 @@  static void sh_audit_gw(struct vcpu *v,
         if ( mfn_valid((smfn = get_shadow_status(d, gw->l2mfn,
                                                  SH_type_l2_shadow))) )
             sh_audit_l2_table(d, smfn, INVALID_MFN);
-#if GUEST_PAGING_LEVELS >= 4 /* 32-bit PV only */
+#if GUEST_PAGING_LEVELS >= 4 && defined(CONFIG_PV32)
         if ( mfn_valid((smfn = get_shadow_status(d, gw->l2mfn,
                                                  SH_type_l2h_shadow))) )
             sh_audit_l2_table(d, smfn, INVALID_MFN);
@@ -859,13 +866,12 @@  do {
     int _i;                                                                 \
     int _xen = !shadow_mode_external(_dom);                                 \
     shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                         \
-    ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow ||\
-           mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_64_shadow);\
+    ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);                       \
     for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )                  \
     {                                                                       \
         if ( (!(_xen))                                                      \
              || !is_pv_32bit_domain(_dom)                                   \
-             || mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2h_64_shadow    \
+             || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow     \
              || (_i < COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(_dom)) )           \
         {                                                                   \
             (_sl2e) = _sp + _i;                                             \
@@ -992,6 +998,7 @@  sh_make_shadow(struct vcpu *v, mfn_t gmf
         }
         break;
 
+#ifdef CONFIG_PV32
         case SH_type_l2h_shadow:
             BUILD_BUG_ON(sizeof(l2_pgentry_t) != sizeof(shadow_l2e_t));
             if ( is_pv_32bit_domain(d) )
@@ -1002,6 +1009,8 @@  sh_make_shadow(struct vcpu *v, mfn_t gmf
                 unmap_domain_page(l2t);
             }
             break;
+#endif
+
         default: /* Do nothing */ break;
         }
     }
@@ -1123,11 +1132,13 @@  static shadow_l2e_t * shadow_get_and_cre
         shadow_l3e_t new_sl3e;
         unsigned int t = SH_type_l2_shadow;
 
+#ifdef CONFIG_PV32
         /* Tag compat L2 containing hypervisor (m2p) mappings */
         if ( is_pv_32bit_domain(d) &&
              guest_l4_table_offset(gw->va) == 0 &&
              guest_l3_table_offset(gw->va) == 3 )
             t = SH_type_l2h_shadow;
+#endif
 
         /* No l2 shadow installed: find and install it. */
         *sl2mfn = get_shadow_status(d, gw->l2mfn, t);
@@ -1337,11 +1348,7 @@  void sh_destroy_l2_shadow(struct domain
 
     SHADOW_DEBUG(DESTROY_SHADOW, "%"PRI_mfn"\n", mfn_x(smfn));
 
-#if GUEST_PAGING_LEVELS >= 4
-    ASSERT(t == SH_type_l2_shadow || t == SH_type_l2h_shadow);
-#else
-    ASSERT(t == SH_type_l2_shadow);
-#endif
+    ASSERT_VALID_L2(t);
     ASSERT(sp->u.sh.head);
 
     /* Record that the guest page isn't shadowed any more (in this type) */
@@ -1865,7 +1872,7 @@  int
 sh_map_and_validate_gl2he(struct vcpu *v, mfn_t gl2mfn,
                            void *new_gl2p, u32 size)
 {
-#if GUEST_PAGING_LEVELS >= 4
+#if GUEST_PAGING_LEVELS >= 4 && defined(CONFIG_PV32)
     return sh_map_and_validate(v, gl2mfn, new_gl2p, size,
                                 SH_type_l2h_shadow,
                                 shadow_l2_index,
@@ -3674,7 +3681,7 @@  void sh_clear_shadow_entry(struct domain
         shadow_set_l1e(d, ep, shadow_l1e_empty(), p2m_invalid, smfn);
         break;
     case SH_type_l2_shadow:
-#if GUEST_PAGING_LEVELS >= 4
+#if GUEST_PAGING_LEVELS >= 4 && defined(CONFIG_PV32)
     case SH_type_l2h_shadow:
 #endif
         shadow_set_l2e(d, ep, shadow_l2e_empty(), smfn);
@@ -4124,14 +4131,16 @@  int cf_check sh_audit_l3_table(struct do
 
         if ( SHADOW_AUDIT & SHADOW_AUDIT_ENTRIES_MFNS )
         {
+            unsigned int t = SH_type_l2_shadow;
+
             gfn = guest_l3e_get_gfn(*gl3e);
             mfn = shadow_l3e_get_mfn(*sl3e);
-            gmfn = get_shadow_status(d, get_gfn_query_unlocked(
-                                        d, gfn_x(gfn), &p2mt),
-                                     (is_pv_32bit_domain(d) &&
-                                      guest_index(gl3e) == 3)
-                                     ? SH_type_l2h_shadow
-                                     : SH_type_l2_shadow);
+#ifdef CONFIG_PV32
+            if ( guest_index(gl3e) == 3 && is_pv_32bit_domain(d) )
+                t = SH_type_l2h_shadow;
+#endif
+            gmfn = get_shadow_status(
+                       d, get_gfn_query_unlocked(d, gfn_x(gfn), &p2mt), t);
             if ( !mfn_eq(gmfn, mfn) )
                 AUDIT_FAIL(3, "bad translation: gfn %" SH_PRI_gfn
                            " --> %" PRI_mfn " != mfn %" PRI_mfn,
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -209,6 +209,10 @@  extern void shadow_audit_tables(struct v
 #define SH_type_unused        10U
 #endif
 
+#ifndef CONFIG_PV32 /* Unused (but uglier to #ifdef above): */
+#undef SH_type_l2h_64_shadow
+#endif
+
 /*
  * What counts as a pinnable shadow?
  */
@@ -286,7 +290,11 @@  static inline void sh_terminate_list(str
 #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)
+#ifdef CONFIG_PV32
 #define SHF_L2H_64  (1u << SH_type_l2h_64_shadow)
+#else
+#define SHF_L2H_64  0
+#endif
 #define SHF_L3_64   (1u << SH_type_l3_64_shadow)
 #define SHF_L4_64   (1u << SH_type_l4_64_shadow)