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

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

[Jan Beulich]

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
 
     unmap_domain_page(si);
 }
@@ -675,6 +675,13 @@ void pv_shim_inject_evtchn(unsigned int
     }
 }
 
+#ifdef CONFIG_PV32
+# include <compat/grant_table.h>
+#else
+# define compat_gnttab_setup_table gnttab_setup_table
+# define compat_handle_okay guest_handle_okay
+#endif
+
 static long pv_shim_grant_table_op(unsigned int cmd,
                                    XEN_GUEST_HANDLE_PARAM(void) uop,
                                    unsigned int count)
@@ -704,10 +711,13 @@ static long pv_shim_grant_table_op(unsig
             rc = -EFAULT;
             break;
         }
+
+#ifdef CONFIG_PV32
         if ( compat )
 #define XLAT_gnttab_setup_table_HNDL_frame_list(d, s)
             XLAT_gnttab_setup_table(&nat, &cmp);
 #undef XLAT_gnttab_setup_table_HNDL_frame_list
+#endif
 
         nat.status = GNTST_okay;
 
@@ -778,6 +788,7 @@ static long pv_shim_grant_table_op(unsig
             }
 
             ASSERT(grant_frames[i]);
+#ifdef CONFIG_PV32
             if ( compat )
             {
                 compat_pfn_t pfn = grant_frames[i];
@@ -789,8 +800,10 @@ static long pv_shim_grant_table_op(unsig
                     break;
                 }
             }
-            else if ( __copy_to_guest_offset(nat.frame_list, i,
-                                             &grant_frames[i], 1) )
+            else
+#endif
+            if ( __copy_to_guest_offset(nat.frame_list, i,
+                                        &grant_frames[i], 1) )
             {
                 nat.status = GNTST_bad_virt_addr;
                 rc = -EFAULT;
@@ -799,10 +812,12 @@ static long pv_shim_grant_table_op(unsig
         }
         spin_unlock(&grant_lock);
 
+#ifdef CONFIG_PV32
         if ( compat )
 #define XLAT_gnttab_setup_table_HNDL_frame_list(d, s)
             XLAT_gnttab_setup_table(&cmp, &nat);
 #undef XLAT_gnttab_setup_table_HNDL_frame_list
+#endif
 
         if ( unlikely(compat ? __copy_to_guest(uop, &cmp, 1)
                              : __copy_to_guest(uop, &nat, 1)) )

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

[Roger Pau Monné]

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?

I'm wondering if there's a way we can force DCE from the compiler and
avoid the ifdefs around the usage of compat.

Thanks, Roger.

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

[Jan Beulich]

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.

Jan

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

[Roger Pau Monné]

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?

Thanks, Roger.

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

[Jan Beulich]

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.

Jan

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

[Roger Pau Monné]

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.