As per the standard this is UB, i.e. we're building on a defacto extension
in the compilers we use. Misra C:2012 rule 20.6 disallows this altogether,
though. Use helper always-inline functions instead.
In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
isn't used anymore by the if() side of the conditional, also reduce the
scope of two other adjacent variables.
For audit_magic() note that both which parameters are needed and what
their types are is attributed to AUDIT_FAIL() accessing variables which
aren't passed as arguments to it.
No functional change intended. Of course codegen does change with this,
first and foremost in register allocation.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Leaving even the fetching of current to the helper in
sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
the fetch will now occur once per present L1E.
Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
here, as identifiers are used which aren't available when the respective
conditions are false.
Note that I tested this only on top of
https://lists.xen.org/archives/html/xen-devel/2023-05/msg01140.html, but I
have no reason to assume that there is a problematic interaction. Of
course it would be really nice if the rest of that series finally could go
in.
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow, sh_next_page)
static inline u32
-guest_index(void *ptr)
+guest_index(const void *ptr)
{
return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
}
@@ -3549,16 +3549,25 @@ static int cf_check sh_guess_wrmap(
}
#endif
+/* Remember the last shadow that we shot a writeable mapping in */
+static always_inline void store_last_writeable_pte_smfn(
+ const struct domain *d, mfn_t sl1mfn)
+{
+#if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC
+ struct vcpu *curr = current;
+
+ if ( curr->domain == d )
+ curr->arch.paging.shadow.last_writeable_pte_smfn = mfn_x(sl1mfn);
+#endif
+}
+
int cf_check sh_rm_write_access_from_l1(
struct domain *d, mfn_t sl1mfn, mfn_t readonly_mfn)
/* Excises all writeable mappings to readonly_mfn from this l1 shadow table */
{
shadow_l1e_t *sl1e;
int done = 0;
-#if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC
- struct vcpu *curr = current;
mfn_t base_sl1mfn = sl1mfn; /* Because sl1mfn changes in the foreach */
-#endif
FOREACH_PRESENT_L1E(sl1mfn, sl1e, NULL, done,
{
@@ -3568,11 +3577,9 @@ int cf_check sh_rm_write_access_from_l1(
shadow_l1e_t ro_sl1e = shadow_l1e_remove_flags(*sl1e, _PAGE_RW);
shadow_set_l1e(d, sl1e, ro_sl1e, p2m_ram_rw, sl1mfn);
-#if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC
- /* Remember the last shadow that we shot a writeable mapping in */
- if ( curr->domain == d )
- curr->arch.paging.shadow.last_writeable_pte_smfn = mfn_x(base_sl1mfn);
-#endif
+
+ store_last_writeable_pte_smfn(d, base_sl1mfn);
+
if ( (mfn_to_page(readonly_mfn)->u.inuse.type_info
& PGT_count_mask) == 0 )
/* This breaks us cleanly out of the FOREACH macro */
@@ -3882,12 +3889,36 @@ static const char *sh_audit_flags(const
return NULL;
}
+static always_inline bool audit_magic(
+ const guest_l1e_t *gl1e, mfn_t gl1mfn,
+ const shadow_l1e_t *sl1e, mfn_t sl1mfn)
+{
+ bool done = false;
+
+#if SHADOW_OPTIMIZATIONS & SHOPT_FAST_FAULT_PATH
+ if ( !sh_l1e_is_gnp(*sl1e) )
+ {
+ gfn_t gfn = sh_l1e_mmio_get_gfn(*sl1e);
+
+ ASSERT(sh_l1e_is_mmio(*sl1e));
+
+ if ( !gfn_eq(gfn, guest_l1e_get_gfn(*gl1e)) )
+ AUDIT_FAIL(1,
+ "shadow MMIO gfn is %" SH_PRI_gfn " but guest gfn is %" SH_PRI_gfn,
+ gfn_x(gfn), gfn_x(guest_l1e_get_gfn(*gl1e)));
+ }
+ else if ( guest_l1e_get_flags(*gl1e) & _PAGE_PRESENT )
+ AUDIT_FAIL(1, "shadow is GNP magic but guest is present");
+#endif
+
+ return done;
+}
+
int cf_check sh_audit_l1_table(struct domain *d, mfn_t sl1mfn, mfn_t x)
{
guest_l1e_t *gl1e, *gp;
shadow_l1e_t *sl1e;
- mfn_t mfn, gmfn, gl1mfn;
- gfn_t gfn;
+ mfn_t gl1mfn;
p2m_type_t p2mt;
const char *s;
int done = 0;
@@ -3906,28 +3937,10 @@ int cf_check sh_audit_l1_table(struct do
#endif
gl1e = gp = map_domain_page(gl1mfn);
- FOREACH_PRESENT_L1E(sl1mfn, sl1e, &gl1e, done, {
-
+ FOREACH_PRESENT_L1E(sl1mfn, sl1e, &gl1e, done,
+ {
if ( sh_l1e_is_magic(*sl1e) )
- {
-#if (SHADOW_OPTIMIZATIONS & SHOPT_FAST_FAULT_PATH)
- if ( sh_l1e_is_gnp(*sl1e) )
- {
- if ( guest_l1e_get_flags(*gl1e) & _PAGE_PRESENT )
- AUDIT_FAIL(1, "shadow is GNP magic but guest is present");
- }
- else
- {
- ASSERT(sh_l1e_is_mmio(*sl1e));
- gfn = sh_l1e_mmio_get_gfn(*sl1e);
- if ( gfn_x(gfn) != gfn_x(guest_l1e_get_gfn(*gl1e)) )
- AUDIT_FAIL(1, "shadow MMIO gfn is %" SH_PRI_gfn
- " but guest gfn is %" SH_PRI_gfn,
- gfn_x(gfn),
- gfn_x(guest_l1e_get_gfn(*gl1e)));
- }
-#endif
- }
+ done = audit_magic(gl1e, gl1mfn, sl1e, sl1mfn);
else
{
s = sh_audit_flags(d, 1, guest_l1e_get_flags(*gl1e),
@@ -3936,9 +3949,10 @@ int cf_check sh_audit_l1_table(struct do
if ( SHADOW_AUDIT & SHADOW_AUDIT_ENTRIES_MFNS )
{
- gfn = guest_l1e_get_gfn(*gl1e);
- mfn = shadow_l1e_get_mfn(*sl1e);
- gmfn = get_gfn_query_unlocked(d, gfn_x(gfn), &p2mt);
+ gfn_t gfn = guest_l1e_get_gfn(*gl1e);
+ mfn_t mfn = shadow_l1e_get_mfn(*sl1e);
+ mfn_t gmfn = get_gfn_query_unlocked(d, gfn_x(gfn), &p2mt);
+
if ( !p2m_is_grant(p2mt) && !mfn_eq(gmfn, mfn) )
AUDIT_FAIL(1, "bad translation: gfn %" SH_PRI_gfn
" --> %" PRI_mfn " != mfn %" PRI_mfn,
@@ -4027,6 +4041,17 @@ int cf_check sh_audit_l2_table(struct do
}
#if GUEST_PAGING_LEVELS >= 4
+static always_inline unsigned int type_from_gl3e(
+ const struct domain *d, const guest_l3e_t *gl3e)
+{
+#ifdef CONFIG_PV32
+ if ( guest_index(gl3e) == 3 && is_pv_32bit_domain(d) )
+ return SH_type_l2h_shadow;
+#endif
+
+ return SH_type_l2_shadow;
+}
+
int cf_check sh_audit_l3_table(struct domain *d, mfn_t sl3mfn, mfn_t x)
{
guest_l3e_t *gl3e, *gp;
@@ -4056,14 +4081,10 @@ int cf_check sh_audit_l3_table(struct do
if ( SHADOW_AUDIT & SHADOW_AUDIT_ENTRIES_MFNS )
{
- unsigned int t = SH_type_l2_shadow;
+ unsigned int t = type_from_gl3e(d, gl3e);
gfn = guest_l3e_get_gfn(*gl3e);
mfn = shadow_l3e_get_mfn(*sl3e);
-#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) )On 18/02/2026 9:03 am, Jan Beulich wrote: > As per the standard this is UB, i.e. we're building on a defacto extension > in the compilers we use. Misra C:2012 rule 20.6 disallows this altogether, > though. Use helper always-inline functions instead. > > In sh_audit_l1_table(), along with reducing the scope of "gfn", which now > isn't used anymore by the if() side of the conditional, also reduce the > scope of two other adjacent variables. > > For audit_magic() note that both which parameters are needed and what > their types are is attributed to AUDIT_FAIL() accessing variables which > aren't passed as arguments to it. > > No functional change intended. Of course codegen does change with this, > first and foremost in register allocation. > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> I included this patch on an interim branch of other MISRA fixes of mine to get a run. https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/13198988953 There's one more violation still to fix: if ( unlikely((level == 1) && sh_mfn_is_a_page_table(target_mfn) #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC ) /* Unless the page is out of sync and the guest is writing to it. */ && !(mfn_oos_may_write(target_mfn) && (ft == ft_demand_write)) #endif /* OOS */ ) ) sflags &= ~_PAGE_RW; I also looked at this one previously. Making mfn_oos_may_write() visible outside of SHOPT_OUT_OF_SYNC is quite invasive. Here, I suggest dropping the unlikely() as the easiest fix. It's almost certainly useless anyway. ~Andrew
On 20.02.2026 16:29, Andrew Cooper wrote:
> On 18/02/2026 9:03 am, Jan Beulich wrote:
>> As per the standard this is UB, i.e. we're building on a defacto extension
>> in the compilers we use. Misra C:2012 rule 20.6 disallows this altogether,
>> though. Use helper always-inline functions instead.
>>
>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
>> isn't used anymore by the if() side of the conditional, also reduce the
>> scope of two other adjacent variables.
>>
>> For audit_magic() note that both which parameters are needed and what
>> their types are is attributed to AUDIT_FAIL() accessing variables which
>> aren't passed as arguments to it.
>>
>> No functional change intended. Of course codegen does change with this,
>> first and foremost in register allocation.
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> I included this patch on an interim branch of other MISRA fixes of mine
> to get a run.
>
> https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/13198988953
>
> There's one more violation still to fix:
>
> if ( unlikely((level == 1)
> && sh_mfn_is_a_page_table(target_mfn)
> #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC )
> /* Unless the page is out of sync and the guest is
> writing to it. */
> && !(mfn_oos_may_write(target_mfn)
> && (ft == ft_demand_write))
> #endif /* OOS */
> ) )
> sflags &= ~_PAGE_RW;
>
>
>
> I also looked at this one previously. Making mfn_oos_may_write()
> visible outside of SHOPT_OUT_OF_SYNC is quite invasive.
We could simply add a stub returning constant true for the !OOS case.
> Here, I suggest dropping the unlikely() as the easiest fix. It's almost
> certainly useless anyway.
Especially when used around an && expression.
I may want to go a little further there, if already we need to touch this,
combining the two adjacent "level == 1" checks:
if ( level == 1 )
{
/* Protect guest page tables. */
if ( unlikely(sh_mfn_is_a_page_table(target_mfn))
#if SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC
/*
* Unless the page is out of sync and the guest is writing to it.
*/
&& (ft != ft_demand_write || !mfn_oos_may_write(target_mfn))
#endif /* OOS */
)
sflags &= ~_PAGE_RW;
/*
* paging_mode_log_dirty support
*
* Only allow the guest write access to a page a) on a demand fault,
* or b) if the page is already marked as dirty.
*
* (We handle log-dirty entirely inside the shadow code, without using
* the p2m_ram_logdirty p2m type: only HAP uses that.)
*/
if ( unlikely(paging_mode_log_dirty(d)) && !mmio_mfn )
{
if ( ft & FETCH_TYPE_WRITE )
paging_mark_dirty(d, target_mfn);
else if ( (sflags & _PAGE_RW) &&
!paging_mfn_is_dirty(d, target_mfn) )
sflags &= ~_PAGE_RW;
}
}
Thoughts?
The two different ways of checking for "guest is writing" also look somewhat
unhelpful. But there's yet another "ft & FETCH_TYPE_WRITE" elsewhere, so it
may want to be a separate patch to switch to uniformly comparing against
ft_demand_write.
Jan
On 18/02/2026 9:03 am, Jan Beulich wrote:
> As per the standard this is UB, i.e. we're building on a defacto extension
> in the compilers we use.
Is it a real extension, or just something that happens to work?
> Misra C:2012 rule 20.6 disallows this altogether,
> though. Use helper always-inline functions instead.
>
> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
> isn't used anymore by the if() side of the conditional, also reduce the
> scope of two other adjacent variables.
>
> For audit_magic() note that both which parameters are needed and what
> their types are is attributed to AUDIT_FAIL() accessing variables which
> aren't passed as arguments to it.
This is grammatically awkward. IMO it would be clearer to say "For
audit_magic() note that there are more parameters than might seem
necessary, caused by the expectations of AUDIT_FAIL()."
>
> No functional change intended. Of course codegen does change with this,
> first and foremost in register allocation.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Definitely a more surgical fix than my approach.
I was attempting to turn FOREACH_*() into real for() loops behind the
scenes, so we don't have the {code} as a parameter, and get real break's
rather than magic behaviour with the _done variable. The problem is
that ever helper is different, and there's rather more hidden under the
covers than appear at first glance.
I'd still like to make this approach work in due course, for the ease of
following the logic if nothing else, but its hard enough to not be a
couple of hours work, and not high enough on my priority list right now.
> ---
> Leaving even the fetching of current to the helper in
> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
> the fetch will now occur once per present L1E.
This will not make a dent in the performance of the shadow code.
> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
> here, as identifiers are used which aren't available when the respective
> conditions are false.
Personally, I'd have put this in the main commit message, because it's
the justification for why out-of-line static inline's need to be used.
>
> Note that I tested this only on top of
> https://lists.xen.org/archives/html/xen-devel/2023-05/msg01140.html, but I
> have no reason to assume that there is a problematic interaction. Of
> course it would be really nice if the rest of that series finally could go
> in.
>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
> shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow, sh_next_page)
>
> static inline u32
> -guest_index(void *ptr)
> +guest_index(const void *ptr)
> {
> return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
> }
While fine per say, this doesn't appear to be related to the patch?
Everything else looks fine.
~Andrew
On 18.02.2026 12:30, Andrew Cooper wrote:
> On 18/02/2026 9:03 am, Jan Beulich wrote:
>> As per the standard this is UB, i.e. we're building on a defacto extension
>> in the compilers we use.
>
> Is it a real extension, or just something that happens to work?
I was hoping I would not need to go through that large swath of gcc doc to
actually figure, because ...
>> Misra C:2012 rule 20.6 disallows this altogether,
>> though.
... this I assumed was reason enough. Still, now that you forced me to: In
The C Preprocessor the behavior is described as intentional, but not as an
extension (section "Directives Within Macro Arguments"). Now you get to
judge whether that's a "real" extension or a "de-facto" one.
>> Use helper always-inline functions instead.
>>
>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
>> isn't used anymore by the if() side of the conditional, also reduce the
>> scope of two other adjacent variables.
>>
>> For audit_magic() note that both which parameters are needed and what
>> their types are is attributed to AUDIT_FAIL() accessing variables which
>> aren't passed as arguments to it.
>
> This is grammatically awkward. IMO it would be clearer to say "For
> audit_magic() note that there are more parameters than might seem
> necessary, caused by the expectations of AUDIT_FAIL()."
I've switched to using that, but one aspect is lost this way: I would have
preferred both gl1e and sl1e to be plain entries, not pointers to ones.
>> ---
>> Leaving even the fetching of current to the helper in
>> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
>> the fetch will now occur once per present L1E.
>
> This will not make a dent in the performance of the shadow code.
>
>> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
>> here, as identifiers are used which aren't available when the respective
>> conditions are false.
>
> Personally, I'd have put this in the main commit message, because it's
> the justification for why out-of-line static inline's need to be used.
I was wondering, so I've moved this up.
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
>> shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow, sh_next_page)
>>
>> static inline u32
>> -guest_index(void *ptr)
>> +guest_index(const void *ptr)
>> {
>> return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
>> }
>
> While fine per say, this doesn't appear to be related to the patch?
It does, the compiler told me to: type_from_gl3e() uses it, and I really
want to keep the const-s on both of its parameters.
Jan
On 2026-02-18 13:42, Jan Beulich wrote:
> On 18.02.2026 12:30, Andrew Cooper wrote:
>> On 18/02/2026 9:03 am, Jan Beulich wrote:
>>> As per the standard this is UB, i.e. we're building on a defacto
>>> extension
>>> in the compilers we use.
>>
>> Is it a real extension, or just something that happens to work?
>
> I was hoping I would not need to go through that large swath of gcc doc
> to
> actually figure, because ...
>
>>> Misra C:2012 rule 20.6 disallows this altogether,
>>> though.
>
> ... this I assumed was reason enough. Still, now that you forced me to:
> In
> The C Preprocessor the behavior is described as intentional, but not as
> an
> extension (section "Directives Within Macro Arguments"). Now you get to
> judge whether that's a "real" extension or a "de-facto" one.
>
Well, since another alternative preprocessor may behave differently or
even choke on this construct (on any instance!) my guess would be to
regard this as a GNU extension.
FWIW MISRA disallows this completely because it can lead to UB 87 from
C99: "There are sequences of preprocessing tokens within the list of
macro arguments that would otherwise act as preprocessing directives
(6.10.3)."
So it just sidesteps the issue without having to look at the actual
token being formed and make our lives as tool implementors a tad easier.
Perhaps a reference to the GCC preprocessor docs could be added in the
commit message or in the code, just to save some brain cycles again next
time.
>>> Use helper always-inline functions instead.
>>>
>>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which
>>> now
>>> isn't used anymore by the if() side of the conditional, also reduce
>>> the
>>> scope of two other adjacent variables.
>>>
>>> For audit_magic() note that both which parameters are needed and what
>>> their types are is attributed to AUDIT_FAIL() accessing variables
>>> which
>>> aren't passed as arguments to it.
>>
>> This is grammatically awkward. IMO it would be clearer to say "For
>> audit_magic() note that there are more parameters than might seem
>> necessary, caused by the expectations of AUDIT_FAIL()."
>
> I've switched to using that, but one aspect is lost this way: I would
> have
> preferred both gl1e and sl1e to be plain entries, not pointers to ones.
>
>>> ---
>>> Leaving even the fetching of current to the helper in
>>> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this
>>> means
>>> the fetch will now occur once per present L1E.
>>
>> This will not make a dent in the performance of the shadow code.
>>
>>> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't
>>> work
>>> here, as identifiers are used which aren't available when the
>>> respective
>>> conditions are false.
>>
>> Personally, I'd have put this in the main commit message, because it's
>> the justification for why out-of-line static inline's need to be used.
>
> I was wondering, so I've moved this up.
>
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
>>> shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow,
>>> sh_next_page)
>>>
>>> static inline u32
>>> -guest_index(void *ptr)
>>> +guest_index(const void *ptr)
>>> {
>>> return (u32)((unsigned long)ptr & ~PAGE_MASK) /
>>> sizeof(guest_l1e_t);
>>> }
>>
>> While fine per say, this doesn't appear to be related to the patch?
>
> It does, the compiler told me to: type_from_gl3e() uses it, and I
> really
> want to keep the const-s on both of its parameters.
>
> Jan
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
On 18/02/2026 12:42 pm, Jan Beulich wrote:
> On 18.02.2026 12:30, Andrew Cooper wrote:
>> On 18/02/2026 9:03 am, Jan Beulich wrote:
>>> As per the standard this is UB, i.e. we're building on a defacto extension
>>> in the compilers we use.
>> Is it a real extension, or just something that happens to work?
> I was hoping I would not need to go through that large swath of gcc doc to
> actually figure, because ...
>
>>> Misra C:2012 rule 20.6 disallows this altogether,
>>> though.
> ... this I assumed was reason enough. Still, now that you forced me to: In
> The C Preprocessor the behavior is described as intentional, but not as an
> extension (section "Directives Within Macro Arguments"). Now you get to
> judge whether that's a "real" extension or a "de-facto" one.
Sorry - all I was trying to do was judge whether it was fair to call it
UB like this or not.
The MISRA complaint alone is fine justification for the patch.
Given this, I'd suggest dropping "defacto" as the easiest way of making
this a little more precise.
>
>>> Use helper always-inline functions instead.
>>>
>>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
>>> isn't used anymore by the if() side of the conditional, also reduce the
>>> scope of two other adjacent variables.
>>>
>>> For audit_magic() note that both which parameters are needed and what
>>> their types are is attributed to AUDIT_FAIL() accessing variables which
>>> aren't passed as arguments to it.
>> This is grammatically awkward. IMO it would be clearer to say "For
>> audit_magic() note that there are more parameters than might seem
>> necessary, caused by the expectations of AUDIT_FAIL()."
> I've switched to using that, but one aspect is lost this way: I would have
> preferred both gl1e and sl1e to be plain entries, not pointers to ones.
>
>>> ---
>>> Leaving even the fetching of current to the helper in
>>> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
>>> the fetch will now occur once per present L1E.
>> This will not make a dent in the performance of the shadow code.
>>
>>> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
>>> here, as identifiers are used which aren't available when the respective
>>> conditions are false.
>> Personally, I'd have put this in the main commit message, because it's
>> the justification for why out-of-line static inline's need to be used.
> I was wondering, so I've moved this up.
>
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
>>> shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow, sh_next_page)
>>>
>>> static inline u32
>>> -guest_index(void *ptr)
>>> +guest_index(const void *ptr)
>>> {
>>> return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
>>> }
>> While fine per say, this doesn't appear to be related to the patch?
> It does, the compiler told me to: type_from_gl3e() uses it, and I really
> want to keep the const-s on both of its parameters.
Oh of course. I'd gone looking explicitly, and failed to spot it.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
© 2016 - 2026 Red Hat, Inc.