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