[PATCH] x86/HVM: get_pat_flags() is needed only by shadow code

[PATCH] x86/HVM: get_pat_flags() is needed only by shadow code

[Jan Beulich]

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 )

Re: [PATCH] x86/HVM: get_pat_flags() is needed only by shadow code

[Jason Andryuk]

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>

Re: [PATCH] x86/HVM: get_pat_flags() is needed only by shadow code

[Roger Pau Monné]

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.

Re: [PATCH] x86/HVM: get_pat_flags() is needed only by shadow code

[Andrew Cooper]

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

Re: [PATCH] x86/HVM: get_pat_flags() is needed only by shadow code

[Roger Pau Monné]

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.