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 )

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.

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 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>