| Message ID | 20220214125127.17985-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for CET Indirect Branch Tracking | expand |
On 14.02.2022 13:50, Andrew Cooper wrote: > From: Juergen Gross <jgross@suse.com> > > When running as pv-shim the hypercall is modified today in order to > replace the functions for __HYPERVISOR_event_channel_op and > __HYPERVISOR_grant_table_op hypercalls. > > Change this to call the related functions from the normal handlers > instead when running as shim. The performance implications are not > really relevant, as a normal production hypervisor will not be > configured to support shim mode, so the related calls will be dropped > due to optimization of the compiler. > > Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy > wrapper do_grant_table_op() needed, as in this case grant_table.c > isn't being built. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I don't think you sync-ed this with Jürgen's v3. There were only minor changes but having a stale version sent two months later isn't very nice. > --- a/xen/common/compat/multicall.c > +++ b/xen/common/compat/multicall.c > @@ -5,7 +5,7 @@ > EMIT_FILE; > > #include <xen/types.h> > -#include <xen/multicall.h> > +#include <xen/hypercall.h> > #include <xen/trace.h> > > #define COMPAT > @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) > mcs->compat_call.args[i] = mcs->call.args[i]; > } > > -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); > #define multicall_entry compat_multicall_entry > #define multicall_entry_t multicall_entry_compat_t > #define do_multicall_call compat_multicall_call Jürgen's patch doesn't have any change to this file, and I'm afraid I also don't see how these adjustments are related here. The commit message sadly also doesn't help ... Jan
On 14/02/2022 13:33, Jan Beulich wrote: > On 14.02.2022 13:50, Andrew Cooper wrote: >> From: Juergen Gross <jgross@suse.com> >> >> When running as pv-shim the hypercall is modified today in order to >> replace the functions for __HYPERVISOR_event_channel_op and >> __HYPERVISOR_grant_table_op hypercalls. >> >> Change this to call the related functions from the normal handlers >> instead when running as shim. The performance implications are not >> really relevant, as a normal production hypervisor will not be >> configured to support shim mode, so the related calls will be dropped >> due to optimization of the compiler. >> >> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >> wrapper do_grant_table_op() needed, as in this case grant_table.c >> isn't being built. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > I don't think you sync-ed this with Jürgen's v3. There were only minor > changes but having a stale version sent two months later isn't very > nice. I did resync. What do you think is missing? > >> --- a/xen/common/compat/multicall.c >> +++ b/xen/common/compat/multicall.c >> @@ -5,7 +5,7 @@ >> EMIT_FILE; >> >> #include <xen/types.h> >> -#include <xen/multicall.h> >> +#include <xen/hypercall.h> >> #include <xen/trace.h> >> >> #define COMPAT >> @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) >> mcs->compat_call.args[i] = mcs->call.args[i]; >> } >> >> -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); >> #define multicall_entry compat_multicall_entry >> #define multicall_entry_t multicall_entry_compat_t >> #define do_multicall_call compat_multicall_call > Jürgen's patch doesn't have any change to this file, and I'm afraid I > also don't see how these adjustments are related here. The commit > message sadly also doesn't help ... The changes are very necessary to split it out of Juergen's series. Without the adjustment, the correction of compat_platform_op()'s guest handle type from void to compat_platform_op_t doesn't compile. ~Andrew
On 14.02.2022 14:50, Andrew Cooper wrote: > On 14/02/2022 13:33, Jan Beulich wrote: >> On 14.02.2022 13:50, Andrew Cooper wrote: >>> From: Juergen Gross <jgross@suse.com> >>> >>> When running as pv-shim the hypercall is modified today in order to >>> replace the functions for __HYPERVISOR_event_channel_op and >>> __HYPERVISOR_grant_table_op hypercalls. >>> >>> Change this to call the related functions from the normal handlers >>> instead when running as shim. The performance implications are not >>> really relevant, as a normal production hypervisor will not be >>> configured to support shim mode, so the related calls will be dropped >>> due to optimization of the compiler. >>> >>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>> isn't being built. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> I don't think you sync-ed this with Jürgen's v3. There were only minor >> changes but having a stale version sent two months later isn't very >> nice. > > I did resync. What do you think is missing? A few likely() / unlikely() as far as I could see. >>> --- a/xen/common/compat/multicall.c >>> +++ b/xen/common/compat/multicall.c >>> @@ -5,7 +5,7 @@ >>> EMIT_FILE; >>> >>> #include <xen/types.h> >>> -#include <xen/multicall.h> >>> +#include <xen/hypercall.h> >>> #include <xen/trace.h> >>> >>> #define COMPAT >>> @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) >>> mcs->compat_call.args[i] = mcs->call.args[i]; >>> } >>> >>> -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); >>> #define multicall_entry compat_multicall_entry >>> #define multicall_entry_t multicall_entry_compat_t >>> #define do_multicall_call compat_multicall_call >> Jürgen's patch doesn't have any change to this file, and I'm afraid I >> also don't see how these adjustments are related here. The commit >> message sadly also doesn't help ... > > The changes are very necessary to split it out of Juergen's series. > > Without the adjustment, the correction of compat_platform_op()'s guest > handle type from void to compat_platform_op_t doesn't compile. Interesting. That's quite far from obvious in this context, so clarifying the purpose in the description would seem helpful. Coming back to the syncing with v3: Was this change the reason then why you did drop my R-b? Jan
On 14/02/2022 13:56, Jan Beulich wrote: > On 14.02.2022 14:50, Andrew Cooper wrote: >> On 14/02/2022 13:33, Jan Beulich wrote: >>> On 14.02.2022 13:50, Andrew Cooper wrote: >>>> From: Juergen Gross <jgross@suse.com> >>>> >>>> When running as pv-shim the hypercall is modified today in order to >>>> replace the functions for __HYPERVISOR_event_channel_op and >>>> __HYPERVISOR_grant_table_op hypercalls. >>>> >>>> Change this to call the related functions from the normal handlers >>>> instead when running as shim. The performance implications are not >>>> really relevant, as a normal production hypervisor will not be >>>> configured to support shim mode, so the related calls will be dropped >>>> due to optimization of the compiler. >>>> >>>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>>> isn't being built. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> I don't think you sync-ed this with Jürgen's v3. There were only minor >>> changes but having a stale version sent two months later isn't very >>> nice. >> I did resync. What do you think is missing? > A few likely() / unlikely() as far as I could see. Oh those two. I appear to have forgot to email. They're wrong - observe they're in an ifndef block, not an ifdef block. Obligatory citation of the recommendation for humans not to try annotating. >>>> --- a/xen/common/compat/multicall.c >>>> +++ b/xen/common/compat/multicall.c >>>> @@ -5,7 +5,7 @@ >>>> EMIT_FILE; >>>> >>>> #include <xen/types.h> >>>> -#include <xen/multicall.h> >>>> +#include <xen/hypercall.h> >>>> #include <xen/trace.h> >>>> >>>> #define COMPAT >>>> @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) >>>> mcs->compat_call.args[i] = mcs->call.args[i]; >>>> } >>>> >>>> -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); >>>> #define multicall_entry compat_multicall_entry >>>> #define multicall_entry_t multicall_entry_compat_t >>>> #define do_multicall_call compat_multicall_call >>> Jürgen's patch doesn't have any change to this file, and I'm afraid I >>> also don't see how these adjustments are related here. The commit >>> message sadly also doesn't help ... >> The changes are very necessary to split it out of Juergen's series. >> >> Without the adjustment, the correction of compat_platform_op()'s guest >> handle type from void to compat_platform_op_t doesn't compile. > Interesting. That's quite far from obvious in this context, so clarifying > the purpose in the description would seem helpful. > > Coming back to the syncing with v3: Was this change the reason then why > you did drop my R-b? My porting of this patch is a non-trivial modification from Juergen's version, and not eligible to retain any tags. I thought I'd discussed this, but I appear to have missed it from both versions of the series. Sorry. Either way. It's exactly the same purpose as before, but modified to compile in isolation. Juergen's second patch, subsequent to this one, is unmodified which is why I retained your R-by there. ~Andrew
On 16.02.2022 23:17, Andrew Cooper wrote: > On 14/02/2022 13:56, Jan Beulich wrote: >> On 14.02.2022 14:50, Andrew Cooper wrote: >>> On 14/02/2022 13:33, Jan Beulich wrote: >>>> On 14.02.2022 13:50, Andrew Cooper wrote: >>>>> From: Juergen Gross <jgross@suse.com> >>>>> >>>>> When running as pv-shim the hypercall is modified today in order to >>>>> replace the functions for __HYPERVISOR_event_channel_op and >>>>> __HYPERVISOR_grant_table_op hypercalls. >>>>> >>>>> Change this to call the related functions from the normal handlers >>>>> instead when running as shim. The performance implications are not >>>>> really relevant, as a normal production hypervisor will not be >>>>> configured to support shim mode, so the related calls will be dropped >>>>> due to optimization of the compiler. >>>>> >>>>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>>>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>>>> isn't being built. >>>>> >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> I don't think you sync-ed this with Jürgen's v3. There were only minor >>>> changes but having a stale version sent two months later isn't very >>>> nice. >>> I did resync. What do you think is missing? >> A few likely() / unlikely() as far as I could see. > > Oh those two. I appear to have forgot to email. > > They're wrong - observe they're in an ifndef block, not an ifdef block. I don't see how the (unrelated) #ifndef matters here: The #ifndef is about grant table availability. The two likely() are about running as shim. I'm of the firm opinion that a binary built without PV_SHIM_EXCLUSIVE is far more likely to be used as a bare metal hypervisor. And for a PV_SHIM_EXCLUSIVE hypervisor the conditions are constant anyway, and hence the unlikely() has no effect. And if your way should really be followed, why did you deem the two unlikely() in do_event_channel_op() and do_grant_table_op() okay? >>>>> --- a/xen/common/compat/multicall.c >>>>> +++ b/xen/common/compat/multicall.c >>>>> @@ -5,7 +5,7 @@ >>>>> EMIT_FILE; >>>>> >>>>> #include <xen/types.h> >>>>> -#include <xen/multicall.h> >>>>> +#include <xen/hypercall.h> >>>>> #include <xen/trace.h> >>>>> >>>>> #define COMPAT >>>>> @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) >>>>> mcs->compat_call.args[i] = mcs->call.args[i]; >>>>> } >>>>> >>>>> -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); >>>>> #define multicall_entry compat_multicall_entry >>>>> #define multicall_entry_t multicall_entry_compat_t >>>>> #define do_multicall_call compat_multicall_call >>>> Jürgen's patch doesn't have any change to this file, and I'm afraid I >>>> also don't see how these adjustments are related here. The commit >>>> message sadly also doesn't help ... >>> The changes are very necessary to split it out of Juergen's series. >>> >>> Without the adjustment, the correction of compat_platform_op()'s guest >>> handle type from void to compat_platform_op_t doesn't compile. >> Interesting. That's quite far from obvious in this context, so clarifying >> the purpose in the description would seem helpful. >> >> Coming back to the syncing with v3: Was this change the reason then why >> you did drop my R-b? > > My porting of this patch is a non-trivial modification from Juergen's > version, and not eligible to retain any tags. > > I thought I'd discussed this, but I appear to have missed it from both > versions of the series. Sorry. > > Either way. It's exactly the same purpose as before, but modified to > compile in isolation. I see. I'm under the impression though that parts were effectively present elsewhere in Jürgen's series. Perhaps it would have been easier if his series (at least up to the point to which you need it here) would (long) have gone in already. What it looks to be blocked on are two or three Arm acks and an x86 ack on patch 1 (which I've expressed I'm not entirely happy about, and hence I'm not going to either ack or nack it). Jan
On 17.02.22 11:20, Jan Beulich wrote: > On 16.02.2022 23:17, Andrew Cooper wrote: >> On 14/02/2022 13:56, Jan Beulich wrote: >>> On 14.02.2022 14:50, Andrew Cooper wrote: >>>> On 14/02/2022 13:33, Jan Beulich wrote: >>>>> On 14.02.2022 13:50, Andrew Cooper wrote: >>>>>> From: Juergen Gross <jgross@suse.com> >>>>>> >>>>>> When running as pv-shim the hypercall is modified today in order to >>>>>> replace the functions for __HYPERVISOR_event_channel_op and >>>>>> __HYPERVISOR_grant_table_op hypercalls. >>>>>> >>>>>> Change this to call the related functions from the normal handlers >>>>>> instead when running as shim. The performance implications are not >>>>>> really relevant, as a normal production hypervisor will not be >>>>>> configured to support shim mode, so the related calls will be dropped >>>>>> due to optimization of the compiler. >>>>>> >>>>>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>>>>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>>>>> isn't being built. >>>>>> >>>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> I don't think you sync-ed this with Jürgen's v3. There were only minor >>>>> changes but having a stale version sent two months later isn't very >>>>> nice. >>>> I did resync. What do you think is missing? >>> A few likely() / unlikely() as far as I could see. >> >> Oh those two. I appear to have forgot to email. >> >> They're wrong - observe they're in an ifndef block, not an ifdef block. > > I don't see how the (unrelated) #ifndef matters here: The #ifndef > is about grant table availability. The two likely() are about > running as shim. I'm of the firm opinion that a binary built > without PV_SHIM_EXCLUSIVE is far more likely to be used as a bare > metal hypervisor. And for a PV_SHIM_EXCLUSIVE hypervisor the > conditions are constant anyway, and hence the unlikely() has no > effect. > > And if your way should really be followed, why did you deem the two > unlikely() in do_event_channel_op() and do_grant_table_op() okay? > >>>>>> --- a/xen/common/compat/multicall.c >>>>>> +++ b/xen/common/compat/multicall.c >>>>>> @@ -5,7 +5,7 @@ >>>>>> EMIT_FILE; >>>>>> >>>>>> #include <xen/types.h> >>>>>> -#include <xen/multicall.h> >>>>>> +#include <xen/hypercall.h> >>>>>> #include <xen/trace.h> >>>>>> >>>>>> #define COMPAT >>>>>> @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) >>>>>> mcs->compat_call.args[i] = mcs->call.args[i]; >>>>>> } >>>>>> >>>>>> -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); >>>>>> #define multicall_entry compat_multicall_entry >>>>>> #define multicall_entry_t multicall_entry_compat_t >>>>>> #define do_multicall_call compat_multicall_call >>>>> Jürgen's patch doesn't have any change to this file, and I'm afraid I >>>>> also don't see how these adjustments are related here. The commit >>>>> message sadly also doesn't help ... >>>> The changes are very necessary to split it out of Juergen's series. >>>> >>>> Without the adjustment, the correction of compat_platform_op()'s guest >>>> handle type from void to compat_platform_op_t doesn't compile. >>> Interesting. That's quite far from obvious in this context, so clarifying >>> the purpose in the description would seem helpful. >>> >>> Coming back to the syncing with v3: Was this change the reason then why >>> you did drop my R-b? >> >> My porting of this patch is a non-trivial modification from Juergen's >> version, and not eligible to retain any tags. >> >> I thought I'd discussed this, but I appear to have missed it from both >> versions of the series. Sorry. >> >> Either way. It's exactly the same purpose as before, but modified to >> compile in isolation. > > I see. I'm under the impression though that parts were effectively > present elsewhere in Jürgen's series. Perhaps it would have been easier > if his series (at least up to the point to which you need it here) > would (long) have gone in already. What it looks to be blocked on are > two or three Arm acks and an x86 ack on patch 1 (which I've expressed > I'm not entirely happy about, and hence I'm not going to either ack or > nack it). The main blocking point currently is that Julien would like me to let all hypercalls return an int (apart from the ones which really need a long). This will affect lot of common code and I need to have more time for that endeavor. An alternative to that would be to not rework the Arm side of the hypercall logic. Juergen
On 17/02/2022 10:20, Jan Beulich wrote: > On 16.02.2022 23:17, Andrew Cooper wrote: >> On 14/02/2022 13:56, Jan Beulich wrote: >>> On 14.02.2022 14:50, Andrew Cooper wrote: >>>> On 14/02/2022 13:33, Jan Beulich wrote: >>>>> On 14.02.2022 13:50, Andrew Cooper wrote: >>>>>> From: Juergen Gross <jgross@suse.com> >>>>>> >>>>>> When running as pv-shim the hypercall is modified today in order to >>>>>> replace the functions for __HYPERVISOR_event_channel_op and >>>>>> __HYPERVISOR_grant_table_op hypercalls. >>>>>> >>>>>> Change this to call the related functions from the normal handlers >>>>>> instead when running as shim. The performance implications are not >>>>>> really relevant, as a normal production hypervisor will not be >>>>>> configured to support shim mode, so the related calls will be dropped >>>>>> due to optimization of the compiler. >>>>>> >>>>>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>>>>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>>>>> isn't being built. >>>>>> >>>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> I don't think you sync-ed this with Jürgen's v3. There were only minor >>>>> changes but having a stale version sent two months later isn't very >>>>> nice. >>>> I did resync. What do you think is missing? >>> A few likely() / unlikely() as far as I could see. >> Oh those two. I appear to have forgot to email. >> >> They're wrong - observe they're in an ifndef block, not an ifdef block. > I don't see how the (unrelated) #ifndef matters here: The #ifndef > is about grant table availability. The two likely() are about > running as shim. I'm of the firm opinion that a binary built > without PV_SHIM_EXCLUSIVE is far more likely to be used as a bare > metal hypervisor. And for a PV_SHIM_EXCLUSIVE hypervisor the > conditions are constant anyway, and hence the unlikely() has no > effect. > > And if your way should really be followed, why did you deem the two > unlikely() in do_event_channel_op() and do_grant_table_op() okay? Because those are at least not incorrect. (I still think we have far too many annotations, and I doubt they're all helpful.) The gnttab stubs in the !GNTTAB case exist strictly for compile tests (there's no such thing as a production build of Xen without grant tables) and PV_SHIM_EXCLUSIVE builds. Code layout only matters for cases where we're executing code, which is the PV Shim case, at which point the condition is constant and doesn't generate a branch. A compiler ought to raise a warning on finding that __builtin_expect() has a constant parameter, because it's a nop in one case, and demonstrably false in the other. As for the function in question, the compiled result is an unconditional tailcall to pv_shim_grant_table_op. ~Andrew
On 21.02.2022 20:21, Andrew Cooper wrote: > On 17/02/2022 10:20, Jan Beulich wrote: >> On 16.02.2022 23:17, Andrew Cooper wrote: >>> On 14/02/2022 13:56, Jan Beulich wrote: >>>> On 14.02.2022 14:50, Andrew Cooper wrote: >>>>> On 14/02/2022 13:33, Jan Beulich wrote: >>>>>> On 14.02.2022 13:50, Andrew Cooper wrote: >>>>>>> From: Juergen Gross <jgross@suse.com> >>>>>>> >>>>>>> When running as pv-shim the hypercall is modified today in order to >>>>>>> replace the functions for __HYPERVISOR_event_channel_op and >>>>>>> __HYPERVISOR_grant_table_op hypercalls. >>>>>>> >>>>>>> Change this to call the related functions from the normal handlers >>>>>>> instead when running as shim. The performance implications are not >>>>>>> really relevant, as a normal production hypervisor will not be >>>>>>> configured to support shim mode, so the related calls will be dropped >>>>>>> due to optimization of the compiler. >>>>>>> >>>>>>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy >>>>>>> wrapper do_grant_table_op() needed, as in this case grant_table.c >>>>>>> isn't being built. >>>>>>> >>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> I don't think you sync-ed this with Jürgen's v3. There were only minor >>>>>> changes but having a stale version sent two months later isn't very >>>>>> nice. >>>>> I did resync. What do you think is missing? >>>> A few likely() / unlikely() as far as I could see. >>> Oh those two. I appear to have forgot to email. >>> >>> They're wrong - observe they're in an ifndef block, not an ifdef block. >> I don't see how the (unrelated) #ifndef matters here: The #ifndef >> is about grant table availability. The two likely() are about >> running as shim. I'm of the firm opinion that a binary built >> without PV_SHIM_EXCLUSIVE is far more likely to be used as a bare >> metal hypervisor. And for a PV_SHIM_EXCLUSIVE hypervisor the >> conditions are constant anyway, and hence the unlikely() has no >> effect. >> >> And if your way should really be followed, why did you deem the two >> unlikely() in do_event_channel_op() and do_grant_table_op() okay? > > Because those are at least not incorrect. (I still think we have far > too many annotations, and I doubt they're all helpful.) I'm afraid I'm completely lost then as to the (consistent) model you want to see used. When putting them side by side: @@ -3543,6 +3547,11 @@ do_grant_table_op( long rc; unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0; +#ifdef CONFIG_PV_SHIM + if ( unlikely(pv_shim) ) + return pv_shim_grant_table_op(cmd, uop, count); +#endif + if ( (int)count < 0 ) return -EINVAL; and long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count) { if ( likely(!pv_shim) ) return -ENOSYS; return pv_shim_grant_table_op(cmd, uop, count); } it is (to me at least) quite obvious that the unlikely() and likely() both express _exactly_ the same thing. > The gnttab stubs in the !GNTTAB case exist strictly for compile tests > (there's no such thing as a production build of Xen without grant > tables) and PV_SHIM_EXCLUSIVE builds. If certain options (or combinations thereof) are not supposed to be used in practice, why would we allow them in the first place? Sadly the commit introducing the GRANT_TABLE option supplies no justification at all as to _why_ this control is/was wanted. > Code layout only matters for cases where we're executing code, which is > the PV Shim case, at which point the condition is constant and doesn't > generate a branch. > > A compiler ought to raise a warning on finding that __builtin_expect() > has a constant parameter, because it's a nop in one case, and > demonstrably false in the other. Such a warning would imo be as appropriate (or not) as one for e.g. "if ( 1 )". > As for the function in question, the compiled result is an unconditional > tailcall to pv_shim_grant_table_op. For the "#define pv_shim true" case, I suppose. And then yes, as expected for this particular case. Anyway - once again in the interest of not blocking progress of the full series, and once again contrary to my intention to not ever again do so in situations like this one: Reviewed-by: Jan Beulich <jbeulich@suse.com> (with at least half a sentence said on the seemingly unrelated changes) Jan
diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h index 5d394d492318..f004824f16b6 100644 --- a/xen/arch/x86/include/asm/hypercall.h +++ b/xen/arch/x86/include/asm/hypercall.h @@ -145,6 +145,7 @@ do_set_segment_base( #include <compat/arch-x86/xen.h> #include <compat/physdev.h> +#include <compat/platform.h> extern int compat_physdev_op( @@ -161,8 +162,9 @@ extern int compat_mmuext_op( XEN_GUEST_HANDLE_PARAM(uint) pdone, unsigned int foreigndom); +DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t); extern int compat_platform_op( - XEN_GUEST_HANDLE_PARAM(void) u_xenpf_op); + XEN_GUEST_HANDLE_PARAM(compat_platform_op_t) u_xenpf_op); extern long compat_callback_op( int cmd, XEN_GUEST_HANDLE(void) arg); diff --git a/xen/arch/x86/include/asm/pv/shim.h b/xen/arch/x86/include/asm/pv/shim.h index 8a91f4f9dfbf..6415f8068e5c 100644 --- a/xen/arch/x86/include/asm/pv/shim.h +++ b/xen/arch/x86/include/asm/pv/shim.h @@ -19,6 +19,7 @@ #ifndef __X86_PV_SHIM_H__ #define __X86_PV_SHIM_H__ +#include <xen/hypercall.h> #include <xen/types.h> #if defined(CONFIG_PV_SHIM_EXCLUSIVE) @@ -45,6 +46,8 @@ domid_t get_initial_domain_id(void); uint64_t pv_shim_mem(uint64_t avail); void pv_shim_fixup_e820(struct e820map *e820); const struct platform_bad_page *pv_shim_reserved_pages(unsigned int *size); +typeof(do_event_channel_op) pv_shim_event_channel_op; +typeof(do_grant_table_op) pv_shim_grant_table_op; #else diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c index ecdd58deea69..50cd219c18fc 100644 --- a/xen/arch/x86/pv/hypercall.c +++ b/xen/arch/x86/pv/hypercall.c @@ -64,7 +64,7 @@ const pv_hypercall_table_t pv_hypercall_table[] = { COMPAT_CALL(xen_version), HYPERCALL(console_io), COMPAT_CALL(physdev_op_compat), -#ifdef CONFIG_GRANT_TABLE +#if defined(CONFIG_GRANT_TABLE) || defined(CONFIG_PV_SHIM) COMPAT_CALL(grant_table_op), #endif HYPERCALL(vm_assist), diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c index d9704121a739..7e891fe2f7a4 100644 --- a/xen/arch/x86/pv/shim.c +++ b/xen/arch/x86/pv/shim.c @@ -56,11 +56,6 @@ static DEFINE_SPINLOCK(balloon_lock); static struct platform_bad_page __initdata reserved_pages[2]; -static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg); -static long pv_shim_grant_table_op(unsigned int cmd, - XEN_GUEST_HANDLE_PARAM(void) uop, - unsigned int count); - /* * By default give the shim 1MB of free memory slack. Some users may wish to * tune this constants for better memory utilization. This can be achieved @@ -203,7 +198,6 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start, start_info_t *si) { bool compat = is_pv_32bit_domain(d); - pv_hypercall_table_t *rw_pv_hypercall_table; uint64_t param = 0; long rc; @@ -249,23 +243,6 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start, consoled_set_ring_addr(page); } - /* - * Locate pv_hypercall_table[] (usually .rodata) in the directmap (which - * is writeable) and insert some shim-specific hypercall handlers. - */ - rw_pv_hypercall_table = __va(__pa(pv_hypercall_table)); - rw_pv_hypercall_table[__HYPERVISOR_event_channel_op].native = - (hypercall_fn_t *)pv_shim_event_channel_op; - rw_pv_hypercall_table[__HYPERVISOR_grant_table_op].native = - (hypercall_fn_t *)pv_shim_grant_table_op; - -#ifdef CONFIG_PV32 - rw_pv_hypercall_table[__HYPERVISOR_event_channel_op].compat = - (hypercall_fn_t *)pv_shim_event_channel_op; - rw_pv_hypercall_table[__HYPERVISOR_grant_table_op].compat = - (hypercall_fn_t *)pv_shim_grant_table_op; -#endif - guest = d; /* @@ -435,7 +412,7 @@ int pv_shim_shutdown(uint8_t reason) return 0; } -static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) +long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d = current->domain; struct evtchn_close close; @@ -683,9 +660,9 @@ void pv_shim_inject_evtchn(unsigned int port) # 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) +long pv_shim_grant_table_op(unsigned int cmd, + XEN_GUEST_HANDLE_PARAM(void) uop, + unsigned int count) { struct domain *d = current->domain; long rc = 0; @@ -845,6 +822,29 @@ static long pv_shim_grant_table_op(unsigned int cmd, return rc; } +#ifndef CONFIG_GRANT_TABLE +/* Thin wrapper(s) needed. */ +long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, + unsigned int count) +{ + if ( !pv_shim ) + return -ENOSYS; + + return pv_shim_grant_table_op(cmd, uop, count); +} + +#ifdef CONFIG_PV32 +int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, + unsigned int count) +{ + if ( !pv_shim ) + return -ENOSYS; + + return pv_shim_grant_table_op(cmd, uop, count); +} +#endif +#endif + long pv_shim_cpu_up(void *data) { struct vcpu *v = data; diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c index fbba893a47cb..966fd27b5f22 100644 --- a/xen/arch/x86/x86_64/platform_hypercall.c +++ b/xen/arch/x86/x86_64/platform_hypercall.c @@ -6,8 +6,8 @@ EMIT_FILE; #include <xen/lib.h> #include <compat/platform.h> +#include <xen/hypercall.h> -DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t); #define xen_platform_op compat_platform_op #define xen_platform_op_t compat_platform_op_t #define do_platform_op(x) compat_platform_op(_##x) diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c index a0e9918f4805..b17739d21829 100644 --- a/xen/common/compat/multicall.c +++ b/xen/common/compat/multicall.c @@ -5,7 +5,7 @@ EMIT_FILE; #include <xen/types.h> -#include <xen/multicall.h> +#include <xen/hypercall.h> #include <xen/trace.h> #define COMPAT @@ -19,7 +19,6 @@ static inline void xlat_multicall_entry(struct mc_state *mcs) mcs->compat_call.args[i] = mcs->call.args[i]; } -DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); #define multicall_entry compat_multicall_entry #define multicall_entry_t multicall_entry_compat_t #define do_multicall_call compat_multicall_call diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index da88ad141a69..c9912122d1e5 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -31,6 +31,10 @@ #include <public/event_channel.h> #include <xsm/xsm.h> +#ifdef CONFIG_PV_SHIM +#include <asm/guest.h> +#endif + #define ERROR_EXIT(_errno) \ do { \ gdprintk(XENLOG_WARNING, \ @@ -1189,6 +1193,11 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; +#ifdef CONFIG_PV_SHIM + if ( unlikely(pv_shim) ) + return pv_shim_event_channel_op(cmd, arg); +#endif + switch ( cmd ) { case EVTCHNOP_alloc_unbound: { diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 3d92fee59285..925ed7d6bee2 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -44,6 +44,10 @@ #include <asm/flushtlb.h> #include <asm/guest_atomics.h> +#ifdef CONFIG_PV_SHIM +#include <asm/guest.h> +#endif + /* Per-domain grant information. */ struct grant_table { /* @@ -3561,6 +3565,11 @@ do_grant_table_op( long rc; unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0; +#ifdef CONFIG_PV_SHIM + if ( unlikely(pv_shim) ) + return pv_shim_grant_table_op(cmd, uop, count); +#endif + if ( (int)count < 0 ) return -EINVAL;