From: "Woods, Brian" <Brian.Woods@amd.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Kevin Tian" <kevin.tian@intel.com>,
"Wei Liu" <wei.liu2@citrix.com>,
"Jan Beulich" <JBeulich@suse.com>,
"Jun Nakajima" <jun.nakajima@intel.com>,
Xen-devel <xen-devel@lists.xenproject.org>,
"Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
"Woods, Brian" <Brian.Woods@amd.com>,
"Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 1/2] x86: init_hypercall_page() cleanup
Date: Wed, 19 Jun 2019 16:17:07 +0000 [thread overview]
Message-ID: <20190619161704.GG20907@amd.com> (raw)
In-Reply-To: <1558606816-17842-2-git-send-email-andrew.cooper3@citrix.com>
On Thu, May 23, 2019 at 11:20:15AM +0100, Andy Cooper wrote:
> [CAUTION: External Email]
>
> The various pieces of the hypercall page infrastructure have grown
> organically over time and ended up in a bit of a mess.
>
> * Rename all functions to be of the form *_init_hypercall_page(). This
> makes them somewhat shorter, and means they can actually be grepped
> for in one go.
> * Move init_hypercall_page() to domain.c. The 64-bit traps.c isn't a
> terribly appropriate place for it to live.
> * Drop an obsolete comment from hvm_init_hypercall_page() and drop the
> domain parameter from hvm_funcs.init_hypercall_page() as it isn't
> necessary.
> * Rearrange the logic in the each function to avoid needing extra local
> variables, and to write the page in one single pass.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Brian Woods <brian.woods@amd.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> ---
> xen/arch/x86/domain.c | 14 +++++++++
> xen/arch/x86/domctl.c | 2 +-
> xen/arch/x86/hvm/hvm.c | 8 ++----
> xen/arch/x86/hvm/svm/svm.c | 18 ++++++------
> xen/arch/x86/hvm/vmx/vmx.c | 18 ++++++------
> xen/arch/x86/pv/dom0_build.c | 3 +-
> xen/arch/x86/pv/hypercall.c | 63 ++++++++++++++++++++---------------------
> xen/arch/x86/traps.c | 2 +-
> xen/arch/x86/x86_64/traps.c | 13 ---------
> xen/include/asm-x86/domain.h | 2 +-
> xen/include/asm-x86/hvm/hvm.h | 4 +--
> xen/include/asm-x86/hypercall.h | 4 +--
> 12 files changed, 73 insertions(+), 78 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ac960dd..9485a17 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -175,6 +175,20 @@ static void noreturn continue_idle_domain(struct vcpu *v)
> reset_stack_and_jump(idle_loop);
> }
>
> +void init_hypercall_page(struct domain *d, void *ptr)
> +{
> + memset(ptr, 0xcc, PAGE_SIZE);
> +
> + if ( is_hvm_domain(d) )
> + hvm_init_hypercall_page(d, ptr);
> + else if ( is_pv_64bit_domain(d) )
> + pv_ring3_init_hypercall_page(ptr);
> + else if ( is_pv_32bit_domain(d) )
> + pv_ring1_init_hypercall_page(ptr);
> + else
> + ASSERT_UNREACHABLE();
> +}
> +
> void dump_pageframe_info(struct domain *d)
> {
> struct page_info *page;
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 9bf2d08..7c6b809 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -517,7 +517,7 @@ long arch_do_domctl(
> }
>
> hypercall_page = __map_domain_page(page);
> - hypercall_page_initialise(d, hypercall_page);
> + init_hypercall_page(d, hypercall_page);
> unmap_domain_page(hypercall_page);
>
> put_page_and_type(page);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8993c2a..5666286 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3801,13 +3801,11 @@ static void hvm_latch_shinfo_size(struct domain *d)
> }
> }
>
> -/* Initialise a hypercall transfer page for a VMX domain using
> - paravirtualised drivers. */
> -void hvm_hypercall_page_initialise(struct domain *d,
> - void *hypercall_page)
> +void hvm_init_hypercall_page(struct domain *d, void *ptr)
> {
> hvm_latch_shinfo_size(d);
> - alternative_vcall(hvm_funcs.init_hypercall_page, d, hypercall_page);
> +
> + alternative_vcall(hvm_funcs.init_hypercall_page, ptr);
> }
>
> void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 9f26493..cd6a6b3 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -916,17 +916,20 @@ static unsigned int svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
> return len;
> }
>
> -static void svm_init_hypercall_page(struct domain *d, void *hypercall_page)
> +static void svm_init_hypercall_page(void *p)
> {
> - char *p;
> - int i;
> + unsigned int i;
>
> - for ( i = 0; i < (PAGE_SIZE / 32); i++ )
> + for ( i = 0; i < (PAGE_SIZE / 32); i++, p += 32 )
> {
> - if ( i == __HYPERVISOR_iret )
> + if ( unlikely(i == __HYPERVISOR_iret) )
> + {
> + /* HYPERVISOR_iret isn't supported */
> + *(u16 *)p = 0x0b0f; /* ud2 */
> +
> continue;
> + }
>
> - p = (char *)(hypercall_page + (i * 32));
> *(u8 *)(p + 0) = 0xb8; /* mov imm32, %eax */
> *(u32 *)(p + 1) = i;
> *(u8 *)(p + 5) = 0x0f; /* vmmcall */
> @@ -934,9 +937,6 @@ static void svm_init_hypercall_page(struct domain *d, void *hypercall_page)
> *(u8 *)(p + 7) = 0xd9;
> *(u8 *)(p + 8) = 0xc3; /* ret */
> }
> -
> - /* Don't support HYPERVISOR_iret at the moment */
> - *(u16 *)(hypercall_page + (__HYPERVISOR_iret * 32)) = 0x0b0f; /* ud2 */
> }
>
> static inline void svm_tsc_ratio_save(struct vcpu *v)
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 7d96678..0060310 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1262,17 +1262,20 @@ static void vmx_set_descriptor_access_exiting(struct vcpu *v, bool enable)
> vmx_vmcs_exit(v);
> }
>
> -static void vmx_init_hypercall_page(struct domain *d, void *hypercall_page)
> +static void vmx_init_hypercall_page(void *p)
> {
> - char *p;
> - int i;
> + unsigned int i;
>
> - for ( i = 0; i < (PAGE_SIZE / 32); i++ )
> + for ( i = 0; i < (PAGE_SIZE / 32); i++, p += 32 )
> {
> - if ( i == __HYPERVISOR_iret )
> + if ( unlikely(i == __HYPERVISOR_iret) )
> + {
> + /* HYPERVISOR_iret isn't supported */
> + *(u16 *)p = 0x0b0f; /* ud2 */
> +
> continue;
> + }
>
> - p = (char *)(hypercall_page + (i * 32));
> *(u8 *)(p + 0) = 0xb8; /* mov imm32, %eax */
> *(u32 *)(p + 1) = i;
> *(u8 *)(p + 5) = 0x0f; /* vmcall */
> @@ -1280,9 +1283,6 @@ static void vmx_init_hypercall_page(struct domain *d, void *hypercall_page)
> *(u8 *)(p + 7) = 0xc1;
> *(u8 *)(p + 8) = 0xc3; /* ret */
> }
> -
> - /* Don't support HYPERVISOR_iret at the moment */
> - *(u16 *)(hypercall_page + (__HYPERVISOR_iret * 32)) = 0x0b0f; /* ud2 */
> }
>
> static unsigned int vmx_get_interrupt_shadow(struct vcpu *v)
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index cef2d42..d48d014 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -738,8 +738,7 @@ int __init dom0_construct_pv(struct domain *d,
> rc = -1;
> goto out;
> }
> - hypercall_page_initialise(
> - d, (void *)(unsigned long)parms.virt_hypercall);
> + init_hypercall_page(d, _p(parms.virt_hypercall));
> }
>
> /* Free temporary buffers. */
> diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
> index 5fdb8f9..0c84c0b 100644
> --- a/xen/arch/x86/pv/hypercall.c
> +++ b/xen/arch/x86/pv/hypercall.c
> @@ -267,16 +267,28 @@ enum mc_disposition arch_do_multicall_call(struct mc_state *state)
> ? mc_continue : mc_preempt;
> }
>
> -void hypercall_page_initialise_ring3_kernel(void *hypercall_page)
> +void pv_ring3_init_hypercall_page(void *p)
> {
> - void *p = hypercall_page;
> unsigned int i;
>
> - /* Fill in all the transfer points with template machine code. */
> for ( i = 0; i < (PAGE_SIZE / 32); i++, p += 32 )
> {
> - if ( i == __HYPERVISOR_iret )
> + if ( unlikely(i == __HYPERVISOR_iret) )
> + {
> + /*
> + * HYPERVISOR_iret is special because it doesn't return and
> + * expects a special stack frame. Guests jump at this transfer
> + * point instead of calling it.
> + */
> + *(u8 *)(p+ 0) = 0x51; /* push %rcx */
> + *(u16 *)(p+ 1) = 0x5341; /* push %r11 */
> + *(u8 *)(p+ 3) = 0x50; /* push %rax */
> + *(u8 *)(p+ 4) = 0xb8; /* mov $__HYPERVISOR_iret, %eax */
> + *(u32 *)(p+ 5) = __HYPERVISOR_iret;
> + *(u16 *)(p+ 9) = 0x050f; /* syscall */
> +
> continue;
> + }
>
> *(u8 *)(p+ 0) = 0x51; /* push %rcx */
> *(u16 *)(p+ 1) = 0x5341; /* push %r11 */
> @@ -287,49 +299,34 @@ void hypercall_page_initialise_ring3_kernel(void *hypercall_page)
> *(u8 *)(p+12) = 0x59; /* pop %rcx */
> *(u8 *)(p+13) = 0xc3; /* ret */
> }
> -
> - /*
> - * HYPERVISOR_iret is special because it doesn't return and expects a
> - * special stack frame. Guests jump at this transfer point instead of
> - * calling it.
> - */
> - p = hypercall_page + (__HYPERVISOR_iret * 32);
> - *(u8 *)(p+ 0) = 0x51; /* push %rcx */
> - *(u16 *)(p+ 1) = 0x5341; /* push %r11 */
> - *(u8 *)(p+ 3) = 0x50; /* push %rax */
> - *(u8 *)(p+ 4) = 0xb8; /* mov $__HYPERVISOR_iret,%eax */
> - *(u32 *)(p+ 5) = __HYPERVISOR_iret;
> - *(u16 *)(p+ 9) = 0x050f; /* syscall */
> }
>
> -void hypercall_page_initialise_ring1_kernel(void *hypercall_page)
> +void pv_ring1_init_hypercall_page(void *p)
> {
> - void *p = hypercall_page;
> unsigned int i;
>
> - /* Fill in all the transfer points with template machine code. */
> -
> for ( i = 0; i < (PAGE_SIZE / 32); i++, p += 32 )
> {
> - if ( i == __HYPERVISOR_iret )
> + if ( unlikely(i == __HYPERVISOR_iret) )
> + {
> + /*
> + * HYPERVISOR_iret is special because it doesn't return and
> + * expects a special stack frame. Guests jump at this transfer
> + * point instead of calling it.
> + */
> + *(u8 *)(p+ 0) = 0x50; /* push %eax */
> + *(u8 *)(p+ 1) = 0xb8; /* mov $__HYPERVISOR_iret, %eax */
> + *(u32 *)(p+ 2) = __HYPERVISOR_iret;
> + *(u16 *)(p+ 6) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int $xx */
> +
> continue;
> + }
>
> *(u8 *)(p+ 0) = 0xb8; /* mov $<i>,%eax */
> *(u32 *)(p+ 1) = i;
> *(u16 *)(p+ 5) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int $xx */
> *(u8 *)(p+ 7) = 0xc3; /* ret */
> }
> -
> - /*
> - * HYPERVISOR_iret is special because it doesn't return and expects a
> - * special stack frame. Guests jump at this transfer point instead of
> - * calling it.
> - */
> - p = hypercall_page + (__HYPERVISOR_iret * 32);
> - *(u8 *)(p+ 0) = 0x50; /* push %eax */
> - *(u8 *)(p+ 1) = 0xb8; /* mov $__HYPERVISOR_iret,%eax */
> - *(u32 *)(p+ 2) = __HYPERVISOR_iret;
> - *(u16 *)(p+ 6) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int $xx */
> }
>
> /*
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 05ddc39..ba1053f 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -828,7 +828,7 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
> }
>
> hypercall_page = __map_domain_page(page);
> - hypercall_page_initialise(d, hypercall_page);
> + init_hypercall_page(d, hypercall_page);
> unmap_domain_page(hypercall_page);
>
> put_page_and_type(page);
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index cb4bf0a..23d9357 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -360,19 +360,6 @@ void subarch_percpu_traps_init(void)
> wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
> }
>
> -void hypercall_page_initialise(struct domain *d, void *hypercall_page)
> -{
> - memset(hypercall_page, 0xCC, PAGE_SIZE);
> - if ( is_hvm_domain(d) )
> - hvm_hypercall_page_initialise(d, hypercall_page);
> - else if ( is_pv_64bit_domain(d) )
> - hypercall_page_initialise_ring3_kernel(hypercall_page);
> - else if ( is_pv_32bit_domain(d) )
> - hypercall_page_initialise_ring1_kernel(hypercall_page);
> - else
> - ASSERT_UNREACHABLE();
> -}
> -
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 214e44c..72dea80 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -83,7 +83,7 @@ void cpuid_policy_updated(struct vcpu *v);
> * Initialise a hypercall-transfer page. The given pointer must be mapped
> * in Xen virtual address space (accesses are not validated or checked).
> */
> -void hypercall_page_initialise(struct domain *d, void *);
> +void init_hypercall_page(struct domain *d, void *);
>
> /************************************************/
> /* shadow paging extension */
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 1921422..b327bd2 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -152,7 +152,7 @@ struct hvm_function_table {
>
> void (*inject_event)(const struct x86_event *event);
>
> - void (*init_hypercall_page)(struct domain *d, void *hypercall_page);
> + void (*init_hypercall_page)(void *ptr);
>
> bool (*event_pending)(const struct vcpu *v);
> bool (*get_pending_event)(struct vcpu *v, struct x86_event *info);
> @@ -272,7 +272,7 @@ int hvm_girq_dest_2_vcpu_id(struct domain *d, uint8_t dest, uint8_t dest_mode);
> enum hvm_intblk
> hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack);
>
> -void hvm_hypercall_page_initialise(struct domain *d, void *hypercall_page);
> +void hvm_init_hypercall_page(struct domain *d, void *ptr);
>
> void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
> struct segment_register *reg);
> diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
> index 49eb5f1..1cd8046 100644
> --- a/xen/include/asm-x86/hypercall.h
> +++ b/xen/include/asm-x86/hypercall.h
> @@ -30,8 +30,8 @@ extern const hypercall_table_t pv_hypercall_table[];
> void pv_hypercall(struct cpu_user_regs *regs);
> #endif
>
> -void hypercall_page_initialise_ring3_kernel(void *hypercall_page);
> -void hypercall_page_initialise_ring1_kernel(void *hypercall_page);
> +void pv_ring1_init_hypercall_page(void *ptr);
> +void pv_ring3_init_hypercall_page(void *ptr);
>
> /*
> * Both do_mmuext_op() and do_mmu_update():
> --
> 2.1.4
>
--
Brian Woods
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-06-19 16:17 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-23 10:20 [PATCH 0/2] Hypercall page docs and code cleanup Andrew Cooper
2019-05-23 10:20 ` [Xen-devel] " Andrew Cooper
2019-05-23 10:20 ` [PATCH 1/2] x86: init_hypercall_page() cleanup Andrew Cooper
2019-05-23 10:20 ` [Xen-devel] " Andrew Cooper
2019-05-23 10:23 ` Wei Liu
2019-05-23 10:23 ` [Xen-devel] " Wei Liu
2019-05-23 10:49 ` Jan Beulich
2019-05-23 10:49 ` [Xen-devel] " Jan Beulich
2019-05-28 5:31 ` Tian, Kevin
2019-05-28 5:31 ` [Xen-devel] " Tian, Kevin
2019-06-19 16:17 ` Woods, Brian [this message]
2019-05-23 10:20 ` [PATCH 2/2] docs: Introduce some hypercall page documentation Andrew Cooper
2019-05-23 10:20 ` [Xen-devel] " Andrew Cooper
2019-05-23 10:23 ` Wei Liu
2019-05-23 10:23 ` [Xen-devel] " Wei Liu
2019-05-23 10:56 ` Jan Beulich
2019-05-23 10:56 ` [Xen-devel] " Jan Beulich
2019-05-23 11:01 ` Andrew Cooper
2019-05-23 11:01 ` [Xen-devel] " Andrew Cooper
2019-05-23 11:41 ` Jan Beulich
2019-05-23 11:41 ` [Xen-devel] " Jan Beulich
2019-05-23 12:02 ` Andrew Cooper
2019-05-23 12:02 ` [Xen-devel] " Andrew Cooper
2019-05-23 12:14 ` Jan Beulich
2019-05-23 12:14 ` [Xen-devel] " Jan Beulich
2019-05-29 4:10 ` [PATCH v2 " Andrew Cooper
2019-05-29 4:10 ` [Xen-devel] " Andrew Cooper
[not found] ` <EF573A6C020000F58E2C01CD@prv1-mh.provo.novell.com>
2019-05-29 8:45 ` Jan Beulich
2019-05-29 8:45 ` [Xen-devel] " Jan Beulich
2019-05-29 11:50 ` Andrew Cooper
2019-05-29 11:50 ` [Xen-devel] " Andrew Cooper
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=20190619161704.GG20907@amd.com \
--to=brian.woods@amd.com \
--cc=JBeulich@suse.com \
--cc=Suravee.Suthikulpanit@amd.com \
--cc=andrew.cooper3@citrix.com \
--cc=boris.ostrovsky@oracle.com \
--cc=jun.nakajima@intel.com \
--cc=kevin.tian@intel.com \
--cc=roger.pau@citrix.com \
--cc=wei.liu2@citrix.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).