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.
--- 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)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
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
© 2016 - 2026 Red Hat, Inc.