Re: [PATCH] x86/shim: fix build when !PV32

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 07, 2021 at 11:17:26AM +0200, Jan Beulich wrote:
> On 07.05.2021 11:08, Roger Pau Monné wrote:
> > On Fri, May 07, 2021 at 10:34:24AM +0200, Jan Beulich wrote:
> >> On 07.05.2021 10:21, Roger Pau Monné wrote:
> >>> On Fri, May 07, 2021 at 08:22:38AM +0200, Jan Beulich wrote:
> >>>> In this case compat headers don't get generated (and aren't needed).
> >>>> The changes made by 527922008bce ("x86: slim down hypercall handling
> >>>> when !PV32") also weren't quite sufficient for this case.
> >>>>
> >>>> Try to limit #ifdef-ary by introducing two "fallback" #define-s.
> >>>>
> >>>> Fixes: d23d792478db ("x86: avoid building COMPAT code when !HVM && !PV32")
> >>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> --- a/xen/arch/x86/pv/shim.c
> >>>> +++ b/xen/arch/x86/pv/shim.c
> >>>> @@ -34,8 +34,6 @@
> >>>>  #include <public/arch-x86/cpuid.h>
> >>>>  #include <public/hvm/params.h>
> >>>>  
> >>>> -#include <compat/grant_table.h>
> >>>> -
> >>>>  #undef virt_to_mfn
> >>>>  #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> >>>>  
> >>>> @@ -300,8 +298,10 @@ static void write_start_info(struct doma
> >>>>                                            &si->console.domU.mfn) )
> >>>>          BUG();
> >>>>  
> >>>> +#ifdef CONFIG_PV32
> >>>>      if ( compat )
> >>>>          xlat_start_info(si, XLAT_start_info_console_domU);
> >>>> +#endif
> >>>
> >>> Would it help the compiler logic if the 'compat' local variable was
> >>> made const?
> >>
> >> No, because XLAT_start_info_console_domU is undeclared when there are
> >> no compat headers.
> >>
> >>> I'm wondering if there's a way we can force DCE from the compiler and
> >>> avoid the ifdefs around the usage of compat.
> >>
> >> The issue isn't with DCE - I believe the compiler does okay in that
> >> regard. The issue is with things simply lacking a declaration /
> >> definition. That's no different from e.g. struct fields living
> >> inside an #ifdef - all uses then need to as well, no matter whether
> >> the compiler is capable of otherwise recognizing the code as dead.
> > 
> > Right, I see those are no longer declared anywhere. Since this is
> > gating compat code, would it make more sense to use CONFIG_COMPAT
> > rather than CONFIG_PV32 here?
> 
> I don't think so, no, from the abstract perspective that it's really
> PV that the shim cares about, and hence other causes of COMPAT getting
> selected shouldn't count.

Ack, and we already use CONFIG_PV32 for similar stuff in the file
anyway.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

It's just becoming slightly trickier to figure out what do you need to
gate with CONFIG_PV32 IMO.

Thanks, Roger.