| Message ID | 20200129202034.15052-9-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | More Hyper-V infrastructures | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei > Liu > Sent: 29 January 2020 20:21 > To: Xen Development List <xen-devel@lists.xenproject.org> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu > <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@eu.citrix.com>; > Andrew Cooper <andrew.cooper3@citrix.com>; Durrant, Paul > <pdurrant@amazon.co.uk>; Ian Jackson <ian.jackson@eu.citrix.com>; Michael > Kelley <mikelley@microsoft.com>; Julien Grall <julien@xen.org>; Roger Pau > Monné <roger.pau@citrix.com> > Subject: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V > hypercall functions > > These functions will be used later to make hypercalls to Hyper-V. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> Reviewed-by: Paul Durrant <pdurrant@amazon.com> > --- > v5: > 1. Switch back to direct call > 2. Fix some issues pointed out by Jan > > I tried using the asm(".equ ..") trick but hit a problem with %c again. > > mm.c:5736:5: error: invalid 'asm': operand is not a condition code, > invalid operand code 'c' > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > --- > MAINTAINERS | 1 + > xen/arch/x86/guest/hyperv/hyperv.c | 6 ++ > xen/arch/x86/xen.lds.S | 4 + > xen/include/asm-x86/fixmap.h | 3 +- > xen/include/asm-x86/guest/hyperv-hcall.h | 96 ++++++++++++++++++++++++ > 5 files changed, 108 insertions(+), 2 deletions(-) > create mode 100644 xen/include/asm-x86/guest/hyperv-hcall.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 04d91482cd..d0a5ed635b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -519,6 +519,7 @@ S: Supported > F: xen/arch/x86/guest/hyperv/ > F: xen/arch/x86/hvm/viridian/ > F: xen/include/asm-x86/guest/hyperv.h > +F: xen/include/asm-x86/guest/hyperv-hcall.h > F: xen/include/asm-x86/guest/hyperv-tlfs.h > F: xen/include/asm-x86/hvm/viridian.h > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c > b/xen/arch/x86/guest/hyperv/hyperv.c > index 2bedcc438c..932a648ff7 100644 > --- a/xen/arch/x86/guest/hyperv/hyperv.c > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > @@ -123,6 +123,12 @@ static const struct hypervisor_ops ops = { > .setup = setup, > }; > > +static void __maybe_unused build_assertions(void) > +{ > + /* We use 1 in linker script */ > + BUILD_BUG_ON(FIX_X_HYPERV_HCALL != 1); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index 97f9c07891..8e02b4c648 100644 > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -329,6 +329,10 @@ SECTIONS > efi = .; > #endif > > +#ifdef CONFIG_HYPERV_GUEST > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > +#endif > + > /* Sections to be discarded */ > /DISCARD/ : { > *(.exit.text) > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > index 8094546b75..a9bcb068cb 100644 > --- a/xen/include/asm-x86/fixmap.h > +++ b/xen/include/asm-x86/fixmap.h > @@ -16,6 +16,7 @@ > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > #ifndef __ASSEMBLY__ > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x( > > #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > > -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > - > #define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) > > #endif /* __ASSEMBLY__ */ > diff --git a/xen/include/asm-x86/guest/hyperv-hcall.h b/xen/include/asm- > x86/guest/hyperv-hcall.h > new file mode 100644 > index 0000000000..5b7509b3b5 > --- /dev/null > +++ b/xen/include/asm-x86/guest/hyperv-hcall.h > @@ -0,0 +1,96 @@ > +/************************************************************************ > ****** > + * asm-x86/guest/hyperv-hcall.h > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms and conditions of the GNU General Public > + * License, version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU General Public > + * License along with this program; If not, see > <http://www.gnu.org/licenses/>. > + * > + * Copyright (c) 2019 Microsoft. > + */ > + > +#ifndef __X86_HYPERV_HCALL_H__ > +#define __X86_HYPERV_HCALL_H__ > + > +#include <xen/lib.h> > +#include <xen/types.h> > + > +#include <asm/asm_defns.h> > +#include <asm/fixmap.h> > +#include <asm/guest/hyperv-tlfs.h> > +#include <asm/page.h> > + > +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t > input_addr, > + paddr_t output_addr) > +{ > + uint64_t status; > + register unsigned long r8 asm("r8") = output_addr; > + > + asm volatile ( "call hv_hcall_page" > + : "=a" (status), "+c" (control), > + "+d" (input_addr) ASM_CALL_CONSTRAINT > + : "r" (r8) > + : "memory" ); > + > + return status; > +} > + > +static inline uint64_t hv_do_fast_hypercall(uint16_t code, > + uint64_t input1, uint64_t > input2) > +{ > + uint64_t status; > + uint64_t control = code | HV_HYPERCALL_FAST_BIT; > + register unsigned long r8 asm("r8") = input2; > + > + asm volatile ( "call hv_hcall_page" > + : "=a" (status), "+c" (control), > + "+d" (input1) ASM_CALL_CONSTRAINT > + : "r" (r8) > + : ); > + > + return status; > +} > + > +static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t > rep_count, > + uint16_t varhead_size, > + paddr_t input, paddr_t output) > +{ > + uint64_t control = code; > + uint64_t status; > + uint16_t rep_comp; > + > + control |= (uint64_t)varhead_size << HV_HYPERCALL_VARHEAD_OFFSET; > + control |= (uint64_t)rep_count << HV_HYPERCALL_REP_COMP_OFFSET; > + > + do { > + status = hv_do_hypercall(control, input, output); > + if ( (status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS ) > + break; > + > + rep_comp = MASK_EXTR(status, HV_HYPERCALL_REP_COMP_MASK); > + > + control &= ~HV_HYPERCALL_REP_START_MASK; > + control |= MASK_INSR(rep_comp, HV_HYPERCALL_REP_START_MASK); > + } while ( rep_comp < rep_count ); > + > + return status; > +} > + > +#endif /* __X86_HYPERV_HCALL_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > -- > 2.20.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On Wed, Jan 29, 2020 at 08:20:30PM +0000, Wei Liu wrote: > These functions will be used later to make hypercalls to Hyper-V. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > v5: > 1. Switch back to direct call > 2. Fix some issues pointed out by Jan > > I tried using the asm(".equ ..") trick but hit a problem with %c again. > > mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > --- > MAINTAINERS | 1 + > xen/arch/x86/guest/hyperv/hyperv.c | 6 ++ > xen/arch/x86/xen.lds.S | 4 + > xen/include/asm-x86/fixmap.h | 3 +- > xen/include/asm-x86/guest/hyperv-hcall.h | 96 ++++++++++++++++++++++++ > 5 files changed, 108 insertions(+), 2 deletions(-) > create mode 100644 xen/include/asm-x86/guest/hyperv-hcall.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 04d91482cd..d0a5ed635b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -519,6 +519,7 @@ S: Supported > F: xen/arch/x86/guest/hyperv/ > F: xen/arch/x86/hvm/viridian/ > F: xen/include/asm-x86/guest/hyperv.h > +F: xen/include/asm-x86/guest/hyperv-hcall.h > F: xen/include/asm-x86/guest/hyperv-tlfs.h > F: xen/include/asm-x86/hvm/viridian.h > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c > index 2bedcc438c..932a648ff7 100644 > --- a/xen/arch/x86/guest/hyperv/hyperv.c > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > @@ -123,6 +123,12 @@ static const struct hypervisor_ops ops = { > .setup = setup, > }; > > +static void __maybe_unused build_assertions(void) > +{ > + /* We use 1 in linker script */ > + BUILD_BUG_ON(FIX_X_HYPERV_HCALL != 1); I wouldn't mind if this was placed together with the hypercall page setup instead of creating a dummy function for it. > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index 97f9c07891..8e02b4c648 100644 > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -329,6 +329,10 @@ SECTIONS > efi = .; > #endif > > +#ifdef CONFIG_HYPERV_GUEST > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); I assume there's no way to use FIX_X_HYPERV_HCALL because it's an enum? > +#endif > + > /* Sections to be discarded */ > /DISCARD/ : { > *(.exit.text) > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > index 8094546b75..a9bcb068cb 100644 > --- a/xen/include/asm-x86/fixmap.h > +++ b/xen/include/asm-x86/fixmap.h > @@ -16,6 +16,7 @@ > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > #ifndef __ASSEMBLY__ > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x( > > #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > > -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > - > #define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) This seems like some unrelated code movement? Thanks, Roger.
On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > +} > > + > > /* > > * Local variables: > > * mode: C > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > index 97f9c07891..8e02b4c648 100644 > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -329,6 +329,10 @@ SECTIONS > > efi = .; > > #endif > > > > +#ifdef CONFIG_HYPERV_GUEST > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > enum? > Yes. And the trick to generate a symbol didn't work either. > > +#endif > > + > > /* Sections to be discarded */ > > /DISCARD/ : { > > *(.exit.text) > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > > index 8094546b75..a9bcb068cb 100644 > > --- a/xen/include/asm-x86/fixmap.h > > +++ b/xen/include/asm-x86/fixmap.h > > @@ -16,6 +16,7 @@ > > > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > > #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > > > #ifndef __ASSEMBLY__ > > > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x( > > > > #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > > > > -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > - > > #define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) > > This seems like some unrelated code movement? > It is required. This section is not supposed to be used in linker script. I have to move that macro ahead. > Thanks, Roger.
On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > +} > > > + > > > /* > > > * Local variables: > > > * mode: C > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > index 97f9c07891..8e02b4c648 100644 > > > --- a/xen/arch/x86/xen.lds.S > > > +++ b/xen/arch/x86/xen.lds.S > > > @@ -329,6 +329,10 @@ SECTIONS > > > efi = .; > > > #endif > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > enum? > > > > Yes. > > And the trick to generate a symbol didn't work either. And you must define that symbol in the linker script? It doesn't seem to be used at link time. > > > +#endif > > > + > > > /* Sections to be discarded */ > > > /DISCARD/ : { > > > *(.exit.text) > > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > > > index 8094546b75..a9bcb068cb 100644 > > > --- a/xen/include/asm-x86/fixmap.h > > > +++ b/xen/include/asm-x86/fixmap.h > > > @@ -16,6 +16,7 @@ > > > > > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > > > #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > > > > > #ifndef __ASSEMBLY__ > > > > > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x( > > > > > > #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > > > > > > -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > > - > > > #define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) > > > > This seems like some unrelated code movement? > > > > It is required. This section is not supposed to be used in linker > script. I have to move that macro ahead. Oh, but you introduce that macro in patch #5, can you place it at the right position when introduced? Thanks, Roger.
On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > +} > > > > + > > > > /* > > > > * Local variables: > > > > * mode: C > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > index 97f9c07891..8e02b4c648 100644 > > > > --- a/xen/arch/x86/xen.lds.S > > > > +++ b/xen/arch/x86/xen.lds.S > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > efi = .; > > > > #endif > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > enum? > > > > > > > Yes. > > > > And the trick to generate a symbol didn't work either. > > And you must define that symbol in the linker script? It doesn't seem > to be used at link time. > I don't follow. I wish I could define and use a symbol in the linker script but couldn't. As for defining a symbol, see the patch that introduces the executable fixmap facility, in function __set_fixmap_x. > > > > +#endif > > > > + > > > > /* Sections to be discarded */ > > > > /DISCARD/ : { > > > > *(.exit.text) > > > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > > > > index 8094546b75..a9bcb068cb 100644 > > > > --- a/xen/include/asm-x86/fixmap.h > > > > +++ b/xen/include/asm-x86/fixmap.h > > > > @@ -16,6 +16,7 @@ > > > > > > > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > > > > #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > > > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > > > > > > > #ifndef __ASSEMBLY__ > > > > > > > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x( > > > > > > > > #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > > > > > > > > -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > > > - > > > > #define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) > > > > > > This seems like some unrelated code movement? > > > > > > > It is required. This section is not supposed to be used in linker > > script. I have to move that macro ahead. > > Oh, but you introduce that macro in patch #5, can you place it at the > right position when introduced? It wasn't needed in the linker script until now. I don't mind doing it that way, but sometimes I'm told to now introduce something until it is used. I wish we could be more consistent on this sort of things. And frankly this sort of change adds no particular value in this series whatsoever. Wei. > > Thanks, Roger.
On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote: > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > > > +} > > > > > + > > > > > /* > > > > > * Local variables: > > > > > * mode: C > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > > index 97f9c07891..8e02b4c648 100644 > > > > > --- a/xen/arch/x86/xen.lds.S > > > > > +++ b/xen/arch/x86/xen.lds.S > > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > > efi = .; > > > > > #endif > > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > > enum? > > > > > > > > > > Yes. > > > > > > And the trick to generate a symbol didn't work either. > > > > And you must define that symbol in the linker script? It doesn't seem > > to be used at link time. > > > > I don't follow. I wish I could define and use a symbol in the linker > script but couldn't. It's likely my fault, as I haven't been following the patch series in that much detail. I assume this is done in order to generate better code, rather than doing something like: void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL); In a C file somewhere when the hypercall page is setup? Thanks, Roger.
On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote: > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote: > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > > > > > +} > > > > > > + > > > > > > /* > > > > > > * Local variables: > > > > > > * mode: C > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > > > index 97f9c07891..8e02b4c648 100644 > > > > > > --- a/xen/arch/x86/xen.lds.S > > > > > > +++ b/xen/arch/x86/xen.lds.S > > > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > > > efi = .; > > > > > > #endif > > > > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > > > enum? > > > > > > > > > > > > > Yes. > > > > > > > > And the trick to generate a symbol didn't work either. > > > > > > And you must define that symbol in the linker script? It doesn't seem > > > to be used at link time. > > > > > > > I don't follow. I wish I could define and use a symbol in the linker > > script but couldn't. > > It's likely my fault, as I haven't been following the patch series in > that much detail. I assume this is done in order to generate better > code, rather than doing something like: > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL); > > In a C file somewhere when the hypercall page is setup? Andrew wanted badly to be able to use direct call in the hypercall functions. This is what we managed to come up with so far. I think what you wrote will still result in an indirect call. (The majority of my time spent on this series has been extending Xen to do more than it could before.) Wei. > > Thanks, Roger.
On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote: > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote: > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote: > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > > > > > > > +} > > > > > > > + > > > > > > > /* > > > > > > > * Local variables: > > > > > > > * mode: C > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > > > > index 97f9c07891..8e02b4c648 100644 > > > > > > > --- a/xen/arch/x86/xen.lds.S > > > > > > > +++ b/xen/arch/x86/xen.lds.S > > > > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > > > > efi = .; > > > > > > > #endif > > > > > > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > > > > enum? > > > > > > > > > > > > > > > > Yes. > > > > > > > > > > And the trick to generate a symbol didn't work either. > > > > > > > > And you must define that symbol in the linker script? It doesn't seem > > > > to be used at link time. > > > > > > > > > > I don't follow. I wish I could define and use a symbol in the linker > > > script but couldn't. > > > > It's likely my fault, as I haven't been following the patch series in > > that much detail. I assume this is done in order to generate better > > code, rather than doing something like: > > > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL); > > > > In a C file somewhere when the hypercall page is setup? > > Andrew wanted badly to be able to use direct call in the hypercall > functions. This is what we managed to come up with so far. > > I think what you wrote will still result in an indirect call. > > (The majority of my time spent on this series has been extending Xen to > do more than it could before.) Ack, sorry to bother you with questions you have already answered. Not sure whether defining hv_hcall_page as a global const would make much difference. Could you maybe use something like alternative_vcall patching to get rid of the indirection? I have to admit I find this all quite hard to follow and reason about, likely because of the mix of C, assembly, and linker script to build this machinery, but that doesn't mean this isn't the best way. Thanks, Roger.
On Thu, Jan 30, 2020 at 03:47:04PM +0100, Roger Pau Monné wrote: > On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote: > > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote: > > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote: > > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > > > > > > > > > +} > > > > > > > > + > > > > > > > > /* > > > > > > > > * Local variables: > > > > > > > > * mode: C > > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > > > > > index 97f9c07891..8e02b4c648 100644 > > > > > > > > --- a/xen/arch/x86/xen.lds.S > > > > > > > > +++ b/xen/arch/x86/xen.lds.S > > > > > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > > > > > efi = .; > > > > > > > > #endif > > > > > > > > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > > > > > enum? > > > > > > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > And the trick to generate a symbol didn't work either. > > > > > > > > > > And you must define that symbol in the linker script? It doesn't seem > > > > > to be used at link time. > > > > > > > > > > > > > I don't follow. I wish I could define and use a symbol in the linker > > > > script but couldn't. > > > > > > It's likely my fault, as I haven't been following the patch series in > > > that much detail. I assume this is done in order to generate better > > > code, rather than doing something like: > > > > > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL); > > > > > > In a C file somewhere when the hypercall page is setup? > > > > Andrew wanted badly to be able to use direct call in the hypercall > > functions. This is what we managed to come up with so far. > > > > I think what you wrote will still result in an indirect call. > > > > (The majority of my time spent on this series has been extending Xen to > > do more than it could before.) > > Ack, sorry to bother you with questions you have already answered. Not No worries. I value your feedback. And having more people understand what is going on is important to the project. > sure whether defining hv_hcall_page as a global const would make much > difference. Could you maybe use something like alternative_vcall > patching to get rid of the indirection? Tried that and didn't work either. :-( > > I have to admit I find this all quite hard to follow and reason about, > likely because of the mix of C, assembly, and linker script to build > this machinery, but that doesn't mean this isn't the best way. > Yes, a lot of trickeries are used to make this work. Not the most elegant combination I would say, but it does achieve what is desired. Wei. > Thanks, Roger.
On Thu, Jan 30, 2020 at 03:03:03PM +0000, Wei Liu wrote: > On Thu, Jan 30, 2020 at 03:47:04PM +0100, Roger Pau Monné wrote: > > On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote: > > > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote: > > > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote: > > > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > > > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > > > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > /* > > > > > > > > > * Local variables: > > > > > > > > > * mode: C > > > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > > > > > > index 97f9c07891..8e02b4c648 100644 > > > > > > > > > --- a/xen/arch/x86/xen.lds.S > > > > > > > > > +++ b/xen/arch/x86/xen.lds.S > > > > > > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > > > > > > efi = .; > > > > > > > > > #endif > > > > > > > > > > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > > > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > > > > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > > > > > > enum? > > > > > > > > > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > And the trick to generate a symbol didn't work either. > > > > > > > > > > > > And you must define that symbol in the linker script? It doesn't seem > > > > > > to be used at link time. > > > > > > > > > > > > > > > > I don't follow. I wish I could define and use a symbol in the linker > > > > > script but couldn't. > > > > > > > > It's likely my fault, as I haven't been following the patch series in > > > > that much detail. I assume this is done in order to generate better > > > > code, rather than doing something like: > > > > > > > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL); > > > > > > > > In a C file somewhere when the hypercall page is setup? > > > > > > Andrew wanted badly to be able to use direct call in the hypercall > > > functions. This is what we managed to come up with so far. > > > > > > I think what you wrote will still result in an indirect call. > > > > > > (The majority of my time spent on this series has been extending Xen to > > > do more than it could before.) > > > > Ack, sorry to bother you with questions you have already answered. Not > > No worries. I value your feedback. And having more people understand > what is going on is important to the project. > > > sure whether defining hv_hcall_page as a global const would make much > > difference. Could you maybe use something like alternative_vcall > > patching to get rid of the indirection? > > Tried that and didn't work either. :-( How do you check whether there's an indirect call or not when using alternative_vcall? It's my understanding that in that case the patching will happen at runtime, and hence the generated assembly code would still use an indirect call, but once patched at runtime it should become a direct call. Thanks, Roger.
On Thu, Jan 30, 2020 at 04:25:44PM +0100, Roger Pau Monné wrote: > On Thu, Jan 30, 2020 at 03:03:03PM +0000, Wei Liu wrote: > > On Thu, Jan 30, 2020 at 03:47:04PM +0100, Roger Pau Monné wrote: > > > On Thu, Jan 30, 2020 at 02:25:26PM +0000, Wei Liu wrote: > > > > On Thu, Jan 30, 2020 at 03:22:01PM +0100, Roger Pau Monné wrote: > > > > > On Thu, Jan 30, 2020 at 12:39:20PM +0000, Wei Liu wrote: > > > > > > On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote: > > > > > > > On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote: > > > > > > > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > > > > > > > > > > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > /* > > > > > > > > > > * Local variables: > > > > > > > > > > * mode: C > > > > > > > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > > > > > > > > > index 97f9c07891..8e02b4c648 100644 > > > > > > > > > > --- a/xen/arch/x86/xen.lds.S > > > > > > > > > > +++ b/xen/arch/x86/xen.lds.S > > > > > > > > > > @@ -329,6 +329,10 @@ SECTIONS > > > > > > > > > > efi = .; > > > > > > > > > > #endif > > > > > > > > > > > > > > > > > > > > +#ifdef CONFIG_HYPERV_GUEST > > > > > > > > > > + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > > > > > > > > > > > > > > > > > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > > > > > > > > > enum? > > > > > > > > > > > > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > > > And the trick to generate a symbol didn't work either. > > > > > > > > > > > > > > And you must define that symbol in the linker script? It doesn't seem > > > > > > > to be used at link time. > > > > > > > > > > > > > > > > > > > I don't follow. I wish I could define and use a symbol in the linker > > > > > > script but couldn't. > > > > > > > > > > It's likely my fault, as I haven't been following the patch series in > > > > > that much detail. I assume this is done in order to generate better > > > > > code, rather than doing something like: > > > > > > > > > > void *hv_hcall_page = fix_x_to_virt(FIX_X_HYPERV_HCALL); > > > > > > > > > > In a C file somewhere when the hypercall page is setup? > > > > > > > > Andrew wanted badly to be able to use direct call in the hypercall > > > > functions. This is what we managed to come up with so far. > > > > > > > > I think what you wrote will still result in an indirect call. > > > > > > > > (The majority of my time spent on this series has been extending Xen to > > > > do more than it could before.) > > > > > > Ack, sorry to bother you with questions you have already answered. Not > > > > No worries. I value your feedback. And having more people understand > > what is going on is important to the project. > > > > > sure whether defining hv_hcall_page as a global const would make much > > > difference. Could you maybe use something like alternative_vcall > > > patching to get rid of the indirection? > > > > Tried that and didn't work either. :-( > > How do you check whether there's an indirect call or not when using > alternative_vcall? > I didn't check, because alternative_vcall didn't compile in that case. > It's my understanding that in that case the patching will happen at > runtime, and hence the generated assembly code would still use an > indirect call, but once patched at runtime it should become a direct > call. It didn't even compile. :-( Wei. > > Thanks, Roger.
On 30.01.2020 13:28, Wei Liu wrote: > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: >> >>> +} >>> + >>> /* >>> * Local variables: >>> * mode: C >>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S >>> index 97f9c07891..8e02b4c648 100644 >>> --- a/xen/arch/x86/xen.lds.S >>> +++ b/xen/arch/x86/xen.lds.S >>> @@ -329,6 +329,10 @@ SECTIONS >>> efi = .; >>> #endif >>> >>> +#ifdef CONFIG_HYPERV_GUEST >>> + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); >> >> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an >> enum? >> > > Yes. > > And the trick to generate a symbol didn't work either. I guess I need an explanation here. Aiui you don't really need the definition to be in the linker script, and it could as well be in e.g. assembly code. How does the same .equ approach not work in this case? Also I think the above will trigger the warnings Andrew had mentioned (on irc?) from the code generating xen.efi's runtime relocation table. Just like in ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - NR_CPUS * PAGE_SIZE, "Xen image overlaps stubs area") I think you need to adjust by __XEN_VIRT_START - XEN_VIRT_START. Jan
On Fri, Jan 31, 2020 at 03:12:50PM +0100, Jan Beulich wrote: > On 30.01.2020 13:28, Wei Liu wrote: > > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: > >> > >>> +} > >>> + > >>> /* > >>> * Local variables: > >>> * mode: C > >>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > >>> index 97f9c07891..8e02b4c648 100644 > >>> --- a/xen/arch/x86/xen.lds.S > >>> +++ b/xen/arch/x86/xen.lds.S > >>> @@ -329,6 +329,10 @@ SECTIONS > >>> efi = .; > >>> #endif > >>> > >>> +#ifdef CONFIG_HYPERV_GUEST > >>> + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); > >> > >> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an > >> enum? > >> > > > > Yes. > > > > And the trick to generate a symbol didn't work either. > > I guess I need an explanation here. Aiui you don't really need > the definition to be in the linker script, and it could as well > be in e.g. assembly code. How does the same .equ approach not > work in this case? > In commit message: mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" ISTR you once mentioned in IRC that there is a way around this (with a new modifier / qualifier), but I don't have the log anymore. > Also I think the above will trigger the warnings Andrew had > mentioned (on irc?) from the code generating xen.efi's runtime > relocation table. Just like in > It was a reply to v4. <cb0e82dc-a154-f918-e725-f77913f835f9@citrix.com> I don't see the warning with this patch. Wei. > ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - > NR_CPUS * PAGE_SIZE, > "Xen image overlaps stubs area") > > I think you need to adjust by __XEN_VIRT_START - XEN_VIRT_START. > > Jan
On 29.01.2020 21:20, Wei Liu wrote: > I tried using the asm(".equ ..") trick but hit a problem with %c again. > > mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" Would you mind also indicating what the input operand actually was? According to my looking at gcc sources when you first mentioned this (on irc iirc), much depends on it actually be recognizable as a constant by the compiler. > +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input_addr, > + paddr_t output_addr) > +{ > + uint64_t status; > + register unsigned long r8 asm("r8") = output_addr; I guess strictly speaking this wants to be asm ( "r8" ), albeit I now realize that I've similarly not played by style in alternative_callN(). In the end I guess - either way. > + asm volatile ( "call hv_hcall_page" > + : "=a" (status), "+c" (control), > + "+d" (input_addr) ASM_CALL_CONSTRAINT > + : "r" (r8) Why "+c" and "+d" but just "r"? If r8 gets treated differently from rcx and rdx, please attach a brief comment. Jan
On 31.01.2020 15:20, Wei Liu wrote: > On Fri, Jan 31, 2020 at 03:12:50PM +0100, Jan Beulich wrote: >> On 30.01.2020 13:28, Wei Liu wrote: >>> On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote: >>>> >>>>> +} >>>>> + >>>>> /* >>>>> * Local variables: >>>>> * mode: C >>>>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S >>>>> index 97f9c07891..8e02b4c648 100644 >>>>> --- a/xen/arch/x86/xen.lds.S >>>>> +++ b/xen/arch/x86/xen.lds.S >>>>> @@ -329,6 +329,10 @@ SECTIONS >>>>> efi = .; >>>>> #endif >>>>> >>>>> +#ifdef CONFIG_HYPERV_GUEST >>>>> + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); >>>> >>>> I assume there's no way to use FIX_X_HYPERV_HCALL because it's an >>>> enum? >>>> >>> >>> Yes. >>> >>> And the trick to generate a symbol didn't work either. >> >> I guess I need an explanation here. Aiui you don't really need >> the definition to be in the linker script, and it could as well >> be in e.g. assembly code. How does the same .equ approach not >> work in this case? >> > > In commit message: > > mm.c:5736:5: error: invalid 'asm': operand is not a condition code, > invalid operand code 'c' > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" But this lacks the other half of the asm(). > ISTR you once mentioned in IRC that there is a way around this (with a > new modifier / qualifier), but I don't have the log anymore. And I think we did say that we didn't want to go too far with using gcc internals. IOW I think trying different modifiers is out of question now (it was -%n0 iirc), but getting past gcc not recognizing the constant as being a constant may still be a viable route. Jan
On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote: > On 29.01.2020 21:20, Wei Liu wrote: > > I tried using the asm(".equ ..") trick but hit a problem with %c again. > > > > mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' > > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > > Would you mind also indicating what the input operand actually > was? According to my looking at gcc sources when you first > mentioned this (on irc iirc), much depends on it actually be > recognizable as a constant by the compiler. Something along the line: asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" :: "i" (__fix_x_to_virt(FIX_X_HV...)) > > > +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input_addr, > > + paddr_t output_addr) > > +{ > > + uint64_t status; > > + register unsigned long r8 asm("r8") = output_addr; > > I guess strictly speaking this wants to be asm ( "r8" ), > albeit I now realize that I've similarly not played by style > in alternative_callN(). In the end I guess - either way. OK. I can fix this. > > > + asm volatile ( "call hv_hcall_page" > > + : "=a" (status), "+c" (control), > > + "+d" (input_addr) ASM_CALL_CONSTRAINT > > + : "r" (r8) > > Why "+c" and "+d" but just "r"? If r8 gets treated differently > from rcx and rdx, please attach a brief comment. > Off the top of my head: r8 will not be modified by Hyper-V, while others may. Wei. > Jan
On 31.01.2020 15:37, Wei Liu wrote: > On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote: >> On 29.01.2020 21:20, Wei Liu wrote: >>> I tried using the asm(".equ ..") trick but hit a problem with %c again. >>> >>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' >>> asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" >> >> Would you mind also indicating what the input operand actually >> was? According to my looking at gcc sources when you first >> mentioned this (on irc iirc), much depends on it actually be >> recognizable as a constant by the compiler. > > Something along the line: > > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > :: "i" (__fix_x_to_virt(FIX_X_HV...)) Quite a bit of playing later, %P0 is documented, supported already in gcc 4.1.x, and also used in a few cases by Linux. %p0 would be another documented alternative, but support for this looks to have been introduced later. Not being able to use %c0 here still smells like a bug (and I guess I'll enter one.) Jan
On Fri, Jan 31, 2020 at 04:35:23PM +0100, Jan Beulich wrote: > On 31.01.2020 15:37, Wei Liu wrote: > > On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote: > >> On 29.01.2020 21:20, Wei Liu wrote: > >>> I tried using the asm(".equ ..") trick but hit a problem with %c again. > >>> > >>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' > >>> asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > >> > >> Would you mind also indicating what the input operand actually > >> was? According to my looking at gcc sources when you first > >> mentioned this (on irc iirc), much depends on it actually be > >> recognizable as a constant by the compiler. > > > > Something along the line: > > > > asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > > :: "i" (__fix_x_to_virt(FIX_X_HV...)) > > Quite a bit of playing later, %P0 is documented, supported > already in gcc 4.1.x, and also used in a few cases by Linux. > %p0 would be another documented alternative, but support for > this looks to have been introduced later. Not being able to use > %c0 here still smells like a bug (and I guess I'll enter one.) OK. Let me try that. If that turns out successful, do you want me to change the other instance to %P0 too? Wei. > > Jan
On 31.01.2020 17:15, Wei Liu wrote: > On Fri, Jan 31, 2020 at 04:35:23PM +0100, Jan Beulich wrote: >> On 31.01.2020 15:37, Wei Liu wrote: >>> On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote: >>>> On 29.01.2020 21:20, Wei Liu wrote: >>>>> I tried using the asm(".equ ..") trick but hit a problem with %c again. >>>>> >>>>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' >>>>> asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" >>>> >>>> Would you mind also indicating what the input operand actually >>>> was? According to my looking at gcc sources when you first >>>> mentioned this (on irc iirc), much depends on it actually be >>>> recognizable as a constant by the compiler. >>> >>> Something along the line: >>> >>> asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" >>> :: "i" (__fix_x_to_virt(FIX_X_HV...)) >> >> Quite a bit of playing later, %P0 is documented, supported >> already in gcc 4.1.x, and also used in a few cases by Linux. >> %p0 would be another documented alternative, but support for >> this looks to have been introduced later. Not being able to use >> %c0 here still smells like a bug (and I guess I'll enter one.) > > OK. Let me try that. > > If that turns out successful, do you want me to change the other > instance to %P0 too? That was a pretty small value, wasn't it? I guess it might be safer to switch to %P (and then perhaps also elsewhere in the code base). But during my playing with it I also noticed there's a signedness bug (affecting all possible modifiers), so we need to watch out for results being right in any event. Jan
On Fri, Jan 31, 2020 at 05:18:14PM +0100, Jan Beulich wrote: > On 31.01.2020 17:15, Wei Liu wrote: > > On Fri, Jan 31, 2020 at 04:35:23PM +0100, Jan Beulich wrote: > >> On 31.01.2020 15:37, Wei Liu wrote: > >>> On Fri, Jan 31, 2020 at 03:24:07PM +0100, Jan Beulich wrote: > >>>> On 29.01.2020 21:20, Wei Liu wrote: > >>>>> I tried using the asm(".equ ..") trick but hit a problem with %c again. > >>>>> > >>>>> mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' > >>>>> asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > >>>> > >>>> Would you mind also indicating what the input operand actually > >>>> was? According to my looking at gcc sources when you first > >>>> mentioned this (on irc iirc), much depends on it actually be > >>>> recognizable as a constant by the compiler. > >>> > >>> Something along the line: > >>> > >>> asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" > >>> :: "i" (__fix_x_to_virt(FIX_X_HV...)) > >> > >> Quite a bit of playing later, %P0 is documented, supported > >> already in gcc 4.1.x, and also used in a few cases by Linux. > >> %p0 would be another documented alternative, but support for > >> this looks to have been introduced later. Not being able to use > >> %c0 here still smells like a bug (and I guess I'll enter one.) > > > > OK. Let me try that. > > > > If that turns out successful, do you want me to change the other > > instance to %P0 too? > > That was a pretty small value, wasn't it? I guess it might be safer > to switch to %P (and then perhaps also elsewhere in the code base). Yes. That value is 0x2000 at the moment. > But during my playing with it I also noticed there's a signedness > bug (affecting all possible modifiers), so we need to watch out for > results being right in any event. Using %P0 works just fine for that instance, the generated value looks correct, so I will use it. Wei. > > Jan
diff --git a/MAINTAINERS b/MAINTAINERS index 04d91482cd..d0a5ed635b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -519,6 +519,7 @@ S: Supported F: xen/arch/x86/guest/hyperv/ F: xen/arch/x86/hvm/viridian/ F: xen/include/asm-x86/guest/hyperv.h +F: xen/include/asm-x86/guest/hyperv-hcall.h F: xen/include/asm-x86/guest/hyperv-tlfs.h F: xen/include/asm-x86/hvm/viridian.h diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c index 2bedcc438c..932a648ff7 100644 --- a/xen/arch/x86/guest/hyperv/hyperv.c +++ b/xen/arch/x86/guest/hyperv/hyperv.c @@ -123,6 +123,12 @@ static const struct hypervisor_ops ops = { .setup = setup, }; +static void __maybe_unused build_assertions(void) +{ + /* We use 1 in linker script */ + BUILD_BUG_ON(FIX_X_HYPERV_HCALL != 1); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 97f9c07891..8e02b4c648 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -329,6 +329,10 @@ SECTIONS efi = .; #endif +#ifdef CONFIG_HYPERV_GUEST + hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1)); +#endif + /* Sections to be discarded */ /DISCARD/ : { *(.exit.text) diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h index 8094546b75..a9bcb068cb 100644 --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -16,6 +16,7 @@ #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) #ifndef __ASSEMBLY__ @@ -110,8 +111,6 @@ extern void __set_fixmap_x( #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) - #define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) #endif /* __ASSEMBLY__ */ diff --git a/xen/include/asm-x86/guest/hyperv-hcall.h b/xen/include/asm-x86/guest/hyperv-hcall.h new file mode 100644 index 0000000000..5b7509b3b5 --- /dev/null +++ b/xen/include/asm-x86/guest/hyperv-hcall.h @@ -0,0 +1,96 @@ +/****************************************************************************** + * asm-x86/guest/hyperv-hcall.h + * + * This program is free software; you can redistribute it and/or + * modify it under the terms and conditions of the GNU General Public + * License, version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; If not, see <http://www.gnu.org/licenses/>. + * + * Copyright (c) 2019 Microsoft. + */ + +#ifndef __X86_HYPERV_HCALL_H__ +#define __X86_HYPERV_HCALL_H__ + +#include <xen/lib.h> +#include <xen/types.h> + +#include <asm/asm_defns.h> +#include <asm/fixmap.h> +#include <asm/guest/hyperv-tlfs.h> +#include <asm/page.h> + +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input_addr, + paddr_t output_addr) +{ + uint64_t status; + register unsigned long r8 asm("r8") = output_addr; + + asm volatile ( "call hv_hcall_page" + : "=a" (status), "+c" (control), + "+d" (input_addr) ASM_CALL_CONSTRAINT + : "r" (r8) + : "memory" ); + + return status; +} + +static inline uint64_t hv_do_fast_hypercall(uint16_t code, + uint64_t input1, uint64_t input2) +{ + uint64_t status; + uint64_t control = code | HV_HYPERCALL_FAST_BIT; + register unsigned long r8 asm("r8") = input2; + + asm volatile ( "call hv_hcall_page" + : "=a" (status), "+c" (control), + "+d" (input1) ASM_CALL_CONSTRAINT + : "r" (r8) + : ); + + return status; +} + +static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t rep_count, + uint16_t varhead_size, + paddr_t input, paddr_t output) +{ + uint64_t control = code; + uint64_t status; + uint16_t rep_comp; + + control |= (uint64_t)varhead_size << HV_HYPERCALL_VARHEAD_OFFSET; + control |= (uint64_t)rep_count << HV_HYPERCALL_REP_COMP_OFFSET; + + do { + status = hv_do_hypercall(control, input, output); + if ( (status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS ) + break; + + rep_comp = MASK_EXTR(status, HV_HYPERCALL_REP_COMP_MASK); + + control &= ~HV_HYPERCALL_REP_START_MASK; + control |= MASK_INSR(rep_comp, HV_HYPERCALL_REP_START_MASK); + } while ( rep_comp < rep_count ); + + return status; +} + +#endif /* __X86_HYPERV_HCALL_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */
These functions will be used later to make hypercalls to Hyper-V. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- v5: 1. Switch back to direct call 2. Fix some issues pointed out by Jan I tried using the asm(".equ ..") trick but hit a problem with %c again. mm.c:5736:5: error: invalid 'asm': operand is not a condition code, invalid operand code 'c' asm ( ".equ HV_HCALL_PAGE, %c0; .global HV_HCALL_PAGE" --- MAINTAINERS | 1 + xen/arch/x86/guest/hyperv/hyperv.c | 6 ++ xen/arch/x86/xen.lds.S | 4 + xen/include/asm-x86/fixmap.h | 3 +- xen/include/asm-x86/guest/hyperv-hcall.h | 96 ++++++++++++++++++++++++ 5 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 xen/include/asm-x86/guest/hyperv-hcall.h