All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"Dario Faggioli" <dfaggioli@suse.com>,
	"Daniel P. Smith" <dpsmith@apertussolutions.com>
Subject: [PATCH 1/3] common: reduce PV shim footprint
Date: Tue, 6 Dec 2022 15:29:00 +0100	[thread overview]
Message-ID: <b00a93e0-9845-7d2d-0a5d-3b2afa41943f@suse.com> (raw)
In-Reply-To: <e5833b09-0ce3-b991-111e-07e64dfcc85a@suse.com>

Having CONFIG_PV_SHIM conditionals in common code isn't really nice.
Utilize that we're no longer invoking hypercall handlers via indirect
calls through a table of function vectors. With the use of direct calls
from the macros defined by hypercall-defs.h, we can simply define
overriding macros for event channel and grant table ops handling. All
this requires is arrangement for careful double inclusion of
asm/hypercall.h out of xen/hypercall.h. Such double inclusion is
required because hypercall-defs.h expects certain definitions to be in
place, while the new handling (placed in pv/shim.h, which is now
included from asm/hypercall.h despite the apparent cyclic dependency)
requires prototypes from hypercall-defs.h to be available already.

Note that this makes it necessary to further constrain the stubbing of
pv_shim from common/sched/core.c, and allows removing the inclusion of
asm/guest.h there as well. Since this is actually part of the overall
goal, leverage the mechanism to also get rid of the similar construct in
xsm/flask/hooks.c, including xen/hypercall.h instead.

Note further that kind of as a side effect this fixes grant table
handling for 32-bit shim guests when GRANT_TABLE=y, as the non-stub
compat_grant_table_op() did not redirect to pv_shim_grant_table_op().

A downside of this is that now do_{event_channel,grant_table}_op() are
built in full again when PV_SHIM_EXCLUSIVE=y, despite all the code
actually being dead in that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: Sadly I had to restore the two "#define pv_shim false", for Arm to
     continue to build. Originally I was hoping to get rid of that
     #ifdef-ary altogether. Would it be acceptable to put a single,
     central #define in e.g. xen/sched.h or xen/hypercall.h?

--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -6,14 +6,23 @@
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
 
-#ifndef __ASM_X86_HYPERCALL_H__
-#define __ASM_X86_HYPERCALL_H__
-
 #include <xen/types.h>
+#include <public/xen.h>
 #include <public/physdev.h>
-#include <public/event_channel.h>
 #include <public/arch-x86/xen-mca.h> /* for do_mca */
+
+#ifdef CONFIG_COMPAT
+#include <compat/arch-x86/xen.h>
+#include <compat/physdev.h>
+#include <compat/platform.h>
+#endif
+
+#if !defined(__ASM_X86_HYPERCALL_H__) && \
+    (!defined(CONFIG_PV_SHIM) || defined(hypercall_args_pv64))
+#define __ASM_X86_HYPERCALL_H__
+
 #include <asm/paging.h>
+#include <asm/pv/shim.h>
 
 #define __HYPERVISOR_paging_domctl_cont __HYPERVISOR_arch_1
 
@@ -33,10 +42,6 @@ void pv_ring3_init_hypercall_page(void *
 
 #ifdef CONFIG_COMPAT
 
-#include <compat/arch-x86/xen.h>
-#include <compat/physdev.h>
-#include <compat/platform.h>
-
 extern int
 compat_common_vcpu_op(
     int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
--- a/xen/arch/x86/include/asm/pv/shim.h
+++ b/xen/arch/x86/include/asm/pv/shim.h
@@ -49,6 +49,22 @@ const struct platform_bad_page *pv_shim_
 typeof(do_event_channel_op) pv_shim_event_channel_op;
 typeof(do_grant_table_op) pv_shim_grant_table_op;
 
+#ifdef CONFIG_PV_SHIM_EXCLUSIVE
+#define REVECTOR(pfx, op, args...) pv_shim_ ## op(args)
+#else
+#define REVECTOR(pfx, op, args...) ({ \
+    likely(!pv_shim) \
+    ? pfx ## _ ## op(args) \
+    : pv_shim_ ## op(args); \
+})
+#endif
+
+#define do_event_channel_op(args...) REVECTOR(do, event_channel_op, args)
+#define do_grant_table_op(args...) REVECTOR(do, grant_table_op, args)
+#ifdef CONFIG_COMPAT
+#define compat_grant_table_op(args...) REVECTOR(compat, grant_table_op, args)
+#endif
+
 #else
 
 static inline void pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -822,9 +822,9 @@ long pv_shim_grant_table_op(unsigned int
     return rc;
 }
 
-#ifndef CONFIG_GRANT_TABLE
+#if !defined(CONFIG_GRANT_TABLE) && !defined(CONFIG_PV_SHIM_EXCLUSIVE)
 /* Thin wrapper(s) needed. */
-long do_grant_table_op(
+long (do_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     if ( !pv_shim )
@@ -834,7 +834,7 @@ long do_grant_table_op(
 }
 
 #ifdef CONFIG_PV32
-int compat_grant_table_op(
+int (compat_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     if ( !pv_shim )
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -56,7 +56,7 @@ CHECK_gnttab_swap_grant_ref;
 CHECK_gnttab_cache_flush;
 #undef xen_gnttab_cache_flush
 
-int compat_grant_table_op(
+int (compat_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) cmp_uop, unsigned int count)
 {
     int rc = 0;
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -32,10 +32,6 @@
 #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,                                    \
@@ -1222,15 +1218,10 @@ static int evtchn_set_priority(const str
     return ret;
 }
 
-long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+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: {
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -45,10 +45,6 @@
 #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 {
     /*
@@ -3563,17 +3559,12 @@ gnttab_cache_flush(XEN_GUEST_HANDLE_PARA
     return 0;
 }
 
-long do_grant_table_op(
+long (do_grant_table_op)(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     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;
 
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -40,9 +40,7 @@
 
 #include "private.h"
 
-#ifdef CONFIG_XEN_GUEST
-#include <asm/guest.h>
-#else
+#ifndef CONFIG_X86
 #define pv_shim false
 #endif
 
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -24,6 +24,9 @@
 /* Needs to be after asm/hypercall.h. */
 #include <xen/hypercall-defs.h>
 
+/* Include a 2nd time, for x86'es PV shim. */
+#include <asm/hypercall.h>
+
 extern long
 arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -19,6 +19,7 @@
 #include <xen/cpumask.h>
 #include <xen/errno.h>
 #include <xen/guest_access.h>
+#include <xen/hypercall.h>
 #include <xen/xenoprof.h>
 #include <xen/iommu.h>
 #ifdef CONFIG_HAS_PCI_MSI
@@ -38,9 +39,7 @@
 #include <conditional.h>
 #include "private.h"
 
-#ifdef CONFIG_X86
-#include <asm/pv/shim.h>
-#else
+#ifndef CONFIG_X86
 #define pv_shim false
 #endif
 



  reply	other threads:[~2022-12-06 14:29 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-06 14:26 [PATCH 0/3] x86/pv-shim: configuring / building re-arrangements Jan Beulich
2022-12-06 14:29 ` Jan Beulich [this message]
2022-12-06 14:30 ` [PATCH 2/3] x86/pv-shim: don't even allow enabling GRANT_TABLE Jan Beulich
2022-12-06 20:26   ` Andrew Cooper
2022-12-07  7:21     ` Jan Beulich
2022-12-07  8:55       ` Jan Beulich
2022-12-07  9:11         ` Juergen Gross
2022-12-07  9:35           ` Jan Beulich
2022-12-07 10:00             ` Juergen Gross
2022-12-06 14:30 ` [PATCH 3/3] x86/pv-shim: suppress core-parking logic Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b00a93e0-9845-7d2d-0a5d-3b2afa41943f@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dfaggioli@suse.com \
    --cc=dpsmith@apertussolutions.com \
    --cc=george.dunlap@citrix.com \
    --cc=julien@xen.org \
    --cc=roger.pau@citrix.com \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.