* [PATCH] x86/altp2m: help gcc13 to avoid it emitting a warning
@ 2023-03-03 7:31 Jan Beulich
2023-03-13 13:17 ` Andrew Cooper
2023-03-14 1:55 ` Tian, Kevin
0 siblings, 2 replies; 3+ messages in thread
From: Jan Beulich @ 2023-03-03 7:31 UTC (permalink / raw
To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap,
Kevin Tian, Jun Nakajima
Switches of altp2m-s always expect a valid altp2m to be in place (and
indeed altp2m_vcpu_initialise() sets the active one to be at index 0).
The compiler, however, cannot know that, and hence it cannot eliminate
p2m_get_altp2m()'s case of returnin (literal) NULL. If then the compiler
decides to special case that code path in the caller, the dereference in
instances of
atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
can, to the code generator, appear to be NULL dereferences, leading to
In function 'atomic_dec',
inlined from '...' at ...:
./arch/x86/include/asm/atomic.h:182:5: error: array subscript 0 is outside array bounds of 'int[0]' [-Werror=array-bounds=]
Aid the compiler by adding a BUG_ON() checking the return value of the
problematic p2m_get_altp2m(). Since with the use of the local variable
the 2nd p2m_get_altp2m() each will look questionable at the first glance
(Why is the local variable not used here?), open-code the only relevant
piece of p2m_get_altp2m() there.
To avoid repeatedly doing these transformations, and also to limit how
"bad" the open-coding really is, convert the entire operation to an
inline helper, used by all three instances (and accepting the redundant
BUG_ON(idx >= MAX_ALTP2M) in two of the three cases).
Reported-by: Charles Arnold <carnold@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4128,13 +4128,7 @@ void vmx_vmexit_handler(struct cpu_user_
}
}
- if ( idx != vcpu_altp2m(v).p2midx )
- {
- BUG_ON(idx >= MAX_ALTP2M);
- atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
- vcpu_altp2m(v).p2midx = idx;
- atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
- }
+ p2m_set_altp2m(v, idx);
}
if ( unlikely(currd->arch.monitor.vmexit_enabled) )
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -879,6 +879,26 @@ static inline struct p2m_domain *p2m_get
return v->domain->arch.altp2m_p2m[index];
}
+/* set current alternate p2m table */
+static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx)
+{
+ struct p2m_domain *orig;
+
+ BUG_ON(idx >= MAX_ALTP2M);
+
+ if ( idx == vcpu_altp2m(v).p2midx )
+ return false;
+
+ orig = p2m_get_altp2m(v);
+ BUG_ON(!orig);
+ atomic_dec(&orig->active_vcpus);
+
+ vcpu_altp2m(v).p2midx = idx;
+ atomic_inc(&v->domain->arch.altp2m_p2m[idx]->active_vcpus);
+
+ return true;
+}
+
/* Switch alternate p2m for a single vcpu */
bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx);
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1789,13 +1789,8 @@ bool_t p2m_switch_vcpu_altp2m_by_id(stru
if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
{
- if ( idx != vcpu_altp2m(v).p2midx )
- {
- atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
- vcpu_altp2m(v).p2midx = idx;
- atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
+ if ( p2m_set_altp2m(v, idx) )
altp2m_vcpu_update_p2m(v);
- }
rc = 1;
}
@@ -2072,13 +2067,8 @@ int p2m_switch_domain_altp2m_by_id(struc
if ( d->arch.altp2m_visible_eptp[idx] != mfn_x(INVALID_MFN) )
{
for_each_vcpu( d, v )
- if ( idx != vcpu_altp2m(v).p2midx )
- {
- atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
- vcpu_altp2m(v).p2midx = idx;
- atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
+ if ( p2m_set_altp2m(v, idx) )
altp2m_vcpu_update_p2m(v);
- }
rc = 0;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86/altp2m: help gcc13 to avoid it emitting a warning
2023-03-03 7:31 [PATCH] x86/altp2m: help gcc13 to avoid it emitting a warning Jan Beulich
@ 2023-03-13 13:17 ` Andrew Cooper
2023-03-14 1:55 ` Tian, Kevin
1 sibling, 0 replies; 3+ messages in thread
From: Andrew Cooper @ 2023-03-13 13:17 UTC (permalink / raw
To: Jan Beulich, xen-devel@lists.xenproject.org
Cc: Wei Liu, Roger Pau Monné, George Dunlap, Kevin Tian,
Jun Nakajima
On 03/03/2023 7:31 am, Jan Beulich wrote:
> Switches of altp2m-s always expect a valid altp2m to be in place (and
> indeed altp2m_vcpu_initialise() sets the active one to be at index 0).
> The compiler, however, cannot know that, and hence it cannot eliminate
> p2m_get_altp2m()'s case of returnin (literal) NULL. If then the compiler
> decides to special case that code path in the caller, the dereference in
> instances of
>
> atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>
> can, to the code generator, appear to be NULL dereferences, leading to
>
> In function 'atomic_dec',
> inlined from '...' at ...:
> ./arch/x86/include/asm/atomic.h:182:5: error: array subscript 0 is outside array bounds of 'int[0]' [-Werror=array-bounds=]
>
> Aid the compiler by adding a BUG_ON() checking the return value of the
> problematic p2m_get_altp2m(). Since with the use of the local variable
> the 2nd p2m_get_altp2m() each will look questionable at the first glance
> (Why is the local variable not used here?), open-code the only relevant
> piece of p2m_get_altp2m() there.
>
> To avoid repeatedly doing these transformations, and also to limit how
> "bad" the open-coding really is, convert the entire operation to an
> inline helper, used by all three instances (and accepting the redundant
> BUG_ON(idx >= MAX_ALTP2M) in two of the three cases).
>
> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] x86/altp2m: help gcc13 to avoid it emitting a warning
2023-03-03 7:31 [PATCH] x86/altp2m: help gcc13 to avoid it emitting a warning Jan Beulich
2023-03-13 13:17 ` Andrew Cooper
@ 2023-03-14 1:55 ` Tian, Kevin
1 sibling, 0 replies; 3+ messages in thread
From: Tian, Kevin @ 2023-03-14 1:55 UTC (permalink / raw
To: Beulich, Jan, xen-devel@lists.xenproject.org
Cc: andrew.cooper3@citrix.com, Wei Liu, Pau Monné, Roger,
George Dunlap, Nakajima, Jun
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, March 3, 2023 3:32 PM
>
> Switches of altp2m-s always expect a valid altp2m to be in place (and
> indeed altp2m_vcpu_initialise() sets the active one to be at index 0).
> The compiler, however, cannot know that, and hence it cannot eliminate
> p2m_get_altp2m()'s case of returnin (literal) NULL. If then the compiler
> decides to special case that code path in the caller, the dereference in
> instances of
>
> atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>
> can, to the code generator, appear to be NULL dereferences, leading to
>
> In function 'atomic_dec',
> inlined from '...' at ...:
> ./arch/x86/include/asm/atomic.h:182:5: error: array subscript 0 is outside
> array bounds of 'int[0]' [-Werror=array-bounds=]
>
> Aid the compiler by adding a BUG_ON() checking the return value of the
> problematic p2m_get_altp2m(). Since with the use of the local variable
> the 2nd p2m_get_altp2m() each will look questionable at the first glance
> (Why is the local variable not used here?), open-code the only relevant
> piece of p2m_get_altp2m() there.
>
> To avoid repeatedly doing these transformations, and also to limit how
> "bad" the open-coding really is, convert the entire operation to an
> inline helper, used by all three instances (and accepting the redundant
> BUG_ON(idx >= MAX_ALTP2M) in two of the three cases).
>
> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-03-14 1:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-03 7:31 [PATCH] x86/altp2m: help gcc13 to avoid it emitting a warning Jan Beulich
2023-03-13 13:17 ` Andrew Cooper
2023-03-14 1:55 ` Tian, Kevin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).