| Message ID | 76aafbed-bea9-445a-8abb-6e1e44996594@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/HVM: get_pat_flags() is needed only by shadow code | expand |
On 2024-07-18 06:10, Jan Beulich wrote: > Therefore with SHADOW_PAGING=n this is better compiled out, to avoid > leaving around unreachable/dead code. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
On Thu, Jul 18, 2024 at 12:10:00PM +0200, Jan Beulich wrote: > Therefore with SHADOW_PAGING=n this is better compiled out, to avoid > leaving around unreachable/dead code. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> I was going to suggest moving this to shadow specific code, but I see it accesses some static variables or macros defined in mtrr.c. Thanks, Roger.
On 18/07/2024 11:10 am, Jan Beulich wrote: > Therefore with SHADOW_PAGING=n this is better compiled out, to avoid > leaving around unreachable/dead code. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/mtrr.c > +++ b/xen/arch/x86/hvm/mtrr.c > @@ -271,6 +271,8 @@ int mtrr_get_type(const struct mtrr_stat > return overlap_mtrr_pos; > } > > +#ifdef CONFIG_SHADOW_PAGING > + > /* > * return the memory type from PAT. > * NOTE: valid only when paging is enabled. > @@ -359,6 +361,8 @@ uint32_t get_pat_flags(struct vcpu *v, > return pat_type_2_pte_flags(pat_entry_value); > } > > +#endif /* CONFIG_SHADOW_PAGING */ > + > static inline bool valid_mtrr_type(uint8_t type) > { > switch ( type ) While I can see this is true, the fact it is indicates that we have bugs/problems elsewhere. It is not only the shadow code that has to combine attributes like this, so we've either got opencoding elsewhere, or a bad abstraction. (This is an observation, rather than a specific objection.) ~Andrew
On Thu, Jul 18, 2024 at 06:06:54PM +0100, Andrew Cooper wrote: > On 18/07/2024 11:10 am, Jan Beulich wrote: > > Therefore with SHADOW_PAGING=n this is better compiled out, to avoid > > leaving around unreachable/dead code. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > --- a/xen/arch/x86/hvm/mtrr.c > > +++ b/xen/arch/x86/hvm/mtrr.c > > @@ -271,6 +271,8 @@ int mtrr_get_type(const struct mtrr_stat > > return overlap_mtrr_pos; > > } > > > > +#ifdef CONFIG_SHADOW_PAGING > > + > > /* > > * return the memory type from PAT. > > * NOTE: valid only when paging is enabled. > > @@ -359,6 +361,8 @@ uint32_t get_pat_flags(struct vcpu *v, > > return pat_type_2_pte_flags(pat_entry_value); > > } > > > > +#endif /* CONFIG_SHADOW_PAGING */ > > + > > static inline bool valid_mtrr_type(uint8_t type) > > { > > switch ( type ) > > While I can see this is true, the fact it is indicates that we have > bugs/problems elsewhere. > > It is not only the shadow code that has to combine attributes like this, > so we've either got opencoding elsewhere, or a bad abstraction. > > (This is an observation, rather than a specific objection.) Won't shadow always need a specific helper, in order to combine both MTRRs and guest PAT attributes, while HAP only needs to merge MTRR attributes? Not that current code couldn't be better structured. Thanks, Roger.
--- a/xen/arch/x86/hvm/mtrr.c +++ b/xen/arch/x86/hvm/mtrr.c @@ -271,6 +271,8 @@ int mtrr_get_type(const struct mtrr_stat return overlap_mtrr_pos; } +#ifdef CONFIG_SHADOW_PAGING + /* * return the memory type from PAT. * NOTE: valid only when paging is enabled. @@ -359,6 +361,8 @@ uint32_t get_pat_flags(struct vcpu *v, return pat_type_2_pte_flags(pat_entry_value); } +#endif /* CONFIG_SHADOW_PAGING */ + static inline bool valid_mtrr_type(uint8_t type) { switch ( type )
Therefore with SHADOW_PAGING=n this is better compiled out, to avoid leaving around unreachable/dead code. Signed-off-by: Jan Beulich <jbeulich@suse.com>