diff mbox series

x86/shim: fix build when !PV32

Message ID 08ea57f0-732e-fe12-409c-5487fb493429@suse.com (mailing list archive)
State New
Headers show
Series x86/shim: fix build when !PV32 | expand

Commit Message

Jan Beulich May 7, 2021, 6:22 a.m. UTC
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>

Comments

Roger Pau Monné May 7, 2021, 8:21 a.m. UTC | #1
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.
Jan Beulich May 7, 2021, 8:34 a.m. UTC | #2
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
Roger Pau Monné May 7, 2021, 9:08 a.m. UTC | #3
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.
Jan Beulich May 7, 2021, 9:17 a.m. UTC | #4
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
Roger Pau Monné May 7, 2021, 10:14 a.m. UTC | #5
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.
diff mbox series

Patch

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