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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxx>
> >
> > --- 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.