Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On Wed, Jan 08, 2020 at 05:53:36PM +, Andrew Cooper wrote:
> On 08/01/2020 17:43, Wei Liu wrote:
> > On Tue, Jan 07, 2020 at 03:42:14PM +, Wei Liu wrote:
> >> On Sun, Jan 05, 2020 at 09:57:56PM +, Andrew Cooper wrote:
> >> [...]
> > The locked bit is probably a good idea, but one aspect missing here is
> > the check to see whether the hypercall page is already enabled, which I
> > expect is for a kexec crash scenario.
> >
> > However, the most important point is the one which describes the #GP
> > properties of the guest trying to modify the page. This can only be
> > achieved with an EPT/NPT mapping lacking the W permission, which will
> > shatter host superpages. Therefore, putting it in .text is going to be
> > rather poor, perf wise.
> >
> > I also note that Xen's implementation of the Viridian hypercall page
> > doesn't conform to these properties, and wants fixing. It is going to
> > need a new kind identification of the page (probably a new p2m type)
> > which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
> >
> > As for suggestions here, I'm struggling to find any memory map details
> > exposed in the Viridian interface, and therefore which gfn is best to
> > choose. I have a sinking feeling that the answer is ACPI...
> TLFS only says "go find one suitable page yourself" without further
> hints.
>
> Since we're still quite far away from a functioning system, finding a
> most suitable page isn't my top priority at this point. If there is a
> simple way to extrapolate suitable information from ACPI, that would be
> great. If it requires writing a set of functionalities, than that will
> need to wait till later.
> >>> To cope with the "one is already established and it is already locked"
> >>> case, the only option is to have a fixmap entry which can be set
> >>> dynamically. The problem is that the fixmap region is marked NX and 64G
> >>> away from .text.
> >>>
> >>> Possibly the least bad option is to have some build-time space (so 0 or
> >>> 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
> >>> XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO
> >>> mappings.
> >>>
> >> OK. This is probably not too difficult.
> >>
> > I have a closer look at this today and want to make sure I understand
> > what you had in mind.
> >
> > Suppose we set aside some space in linker script. Using the following
> > will give you a WAX section. I still need to add CONFIG_GUEST around it,
> > but you get the idea.
> >
> >
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 111edb5360..a7af321139 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -305,6 +305,15 @@ SECTIONS
> > . = ALIGN(POINTER_ALIGN);
> > __bss_end = .;
> >} :text
> > +
> > + . = ALIGN(SECTION_ALIGN);
> > + DECL_SECTION(.text.hypercall_page) {
> > + . = ALIGN(PAGE_SIZE);
> > + __hypercall_page_start = .;
> > + . = . + PAGE_SIZE;
> > + __hypercall_page_end = .;
> > + } :text=0x9090
> > +
> >_end = . ;
> >
> >. = ALIGN(SECTION_ALIGN);
> >
> >
> > And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
> > address to (MAXPHYSADDR-PAGE_SIZE)?
>
> Ah no. This still puts the hypercall page (or at least, space for it)
> in the Xen image itself, which is something we are trying to avoid.
>
> What I meant was to basically copy(/extend) the existing fixmap
> implementation, calling it fixmap_x() (or something better), and put
> FIXMAP_X_SIZE as an additional gap in the calculation
> alloc_stub_page(). Even the current fixmap code has an ifdef example
> for CONFIG_XEN_GUEST.
Not just alloc_stub_page, but also livepatch infrastructure's address
space arrangement -- see arch/x86/liveptch.c:arch_livepatch_init.
Suppose I copy the existing fixmap mechanism, to get the uniform
experience for these two variants for fixmap, I will also need to
statically allocate its l2 and l1 page tables. This ends up putting two
more pages into Xen's image. I want to make sure this is what you want
because you seemed to be concern about enlarging image size.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On 08/01/2020 17:43, Wei Liu wrote:
> On Tue, Jan 07, 2020 at 03:42:14PM +, Wei Liu wrote:
>> On Sun, Jan 05, 2020 at 09:57:56PM +, Andrew Cooper wrote:
>> [...]
> The locked bit is probably a good idea, but one aspect missing here is
> the check to see whether the hypercall page is already enabled, which I
> expect is for a kexec crash scenario.
>
> However, the most important point is the one which describes the #GP
> properties of the guest trying to modify the page. This can only be
> achieved with an EPT/NPT mapping lacking the W permission, which will
> shatter host superpages. Therefore, putting it in .text is going to be
> rather poor, perf wise.
>
> I also note that Xen's implementation of the Viridian hypercall page
> doesn't conform to these properties, and wants fixing. It is going to
> need a new kind identification of the page (probably a new p2m type)
> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
>
> As for suggestions here, I'm struggling to find any memory map details
> exposed in the Viridian interface, and therefore which gfn is best to
> choose. I have a sinking feeling that the answer is ACPI...
TLFS only says "go find one suitable page yourself" without further
hints.
Since we're still quite far away from a functioning system, finding a
most suitable page isn't my top priority at this point. If there is a
simple way to extrapolate suitable information from ACPI, that would be
great. If it requires writing a set of functionalities, than that will
need to wait till later.
>>> To cope with the "one is already established and it is already locked"
>>> case, the only option is to have a fixmap entry which can be set
>>> dynamically. The problem is that the fixmap region is marked NX and 64G
>>> away from .text.
>>>
>>> Possibly the least bad option is to have some build-time space (so 0 or
>>> 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
>>> XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
>>>
>> OK. This is probably not too difficult.
>>
> I have a closer look at this today and want to make sure I understand
> what you had in mind.
>
> Suppose we set aside some space in linker script. Using the following
> will give you a WAX section. I still need to add CONFIG_GUEST around it,
> but you get the idea.
>
>
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 111edb5360..a7af321139 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -305,6 +305,15 @@ SECTIONS
> . = ALIGN(POINTER_ALIGN);
> __bss_end = .;
>} :text
> +
> + . = ALIGN(SECTION_ALIGN);
> + DECL_SECTION(.text.hypercall_page) {
> + . = ALIGN(PAGE_SIZE);
> + __hypercall_page_start = .;
> + . = . + PAGE_SIZE;
> + __hypercall_page_end = .;
> + } :text=0x9090
> +
>_end = . ;
>
>. = ALIGN(SECTION_ALIGN);
>
>
> And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
> address to (MAXPHYSADDR-PAGE_SIZE)?
Ah no. This still puts the hypercall page (or at least, space for it)
in the Xen image itself, which is something we are trying to avoid.
What I meant was to basically copy(/extend) the existing fixmap
implementation, calling it fixmap_x() (or something better), and put
FIXMAP_X_SIZE as an additional gap in the calculation
alloc_stub_page(). Even the current fixmap code has an ifdef example
for CONFIG_XEN_GUEST.
You'd then end up with something like
__set_fixmap_x(FIXMAP_X_HYPERV_HYPERCALL_PAGE, mfn) which uses _RX in
the underlying call to map_pages_to_xen()
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On Tue, Jan 07, 2020 at 03:42:14PM +, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 09:57:56PM +, Andrew Cooper wrote:
> [...]
> > >
> > >> The locked bit is probably a good idea, but one aspect missing here is
> > >> the check to see whether the hypercall page is already enabled, which I
> > >> expect is for a kexec crash scenario.
> > >>
> > >> However, the most important point is the one which describes the #GP
> > >> properties of the guest trying to modify the page. This can only be
> > >> achieved with an EPT/NPT mapping lacking the W permission, which will
> > >> shatter host superpages. Therefore, putting it in .text is going to be
> > >> rather poor, perf wise.
> > >>
> > >> I also note that Xen's implementation of the Viridian hypercall page
> > >> doesn't conform to these properties, and wants fixing. It is going to
> > >> need a new kind identification of the page (probably a new p2m type)
> > >> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
> > >>
> > >> As for suggestions here, I'm struggling to find any memory map details
> > >> exposed in the Viridian interface, and therefore which gfn is best to
> > >> choose. I have a sinking feeling that the answer is ACPI...
> > > TLFS only says "go find one suitable page yourself" without further
> > > hints.
> > >
> > > Since we're still quite far away from a functioning system, finding a
> > > most suitable page isn't my top priority at this point. If there is a
> > > simple way to extrapolate suitable information from ACPI, that would be
> > > great. If it requires writing a set of functionalities, than that will
> > > need to wait till later.
> >
> > To cope with the "one is already established and it is already locked"
> > case, the only option is to have a fixmap entry which can be set
> > dynamically. The problem is that the fixmap region is marked NX and 64G
> > away from .text.
> >
> > Possibly the least bad option is to have some build-time space (so 0 or
> > 4k depending on CONFIG_HYPERV) between the per-cpu stubs and
> > XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
> >
>
> OK. This is probably not too difficult.
>
I have a closer look at this today and want to make sure I understand
what you had in mind.
Suppose we set aside some space in linker script. Using the following
will give you a WAX section. I still need to add CONFIG_GUEST around it,
but you get the idea.
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 111edb5360..a7af321139 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -305,6 +305,15 @@ SECTIONS
. = ALIGN(POINTER_ALIGN);
__bss_end = .;
} :text
+
+ . = ALIGN(SECTION_ALIGN);
+ DECL_SECTION(.text.hypercall_page) {
+ . = ALIGN(PAGE_SIZE);
+ __hypercall_page_start = .;
+ . = . + PAGE_SIZE;
+ __hypercall_page_end = .;
+ } :text=0x9090
+
_end = . ;
. = ALIGN(SECTION_ALIGN);
And then? Use map_pages_to_xen(..., PAGE_HYPERVISOR_RX) to point that
address to (MAXPHYSADDR-PAGE_SIZE)?
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On Sun, Jan 05, 2020 at 09:57:56PM +, Andrew Cooper wrote: [...] > > > >> The locked bit is probably a good idea, but one aspect missing here is > >> the check to see whether the hypercall page is already enabled, which I > >> expect is for a kexec crash scenario. > >> > >> However, the most important point is the one which describes the #GP > >> properties of the guest trying to modify the page. This can only be > >> achieved with an EPT/NPT mapping lacking the W permission, which will > >> shatter host superpages. Therefore, putting it in .text is going to be > >> rather poor, perf wise. > >> > >> I also note that Xen's implementation of the Viridian hypercall page > >> doesn't conform to these properties, and wants fixing. It is going to > >> need a new kind identification of the page (probably a new p2m type) > >> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it. > >> > >> As for suggestions here, I'm struggling to find any memory map details > >> exposed in the Viridian interface, and therefore which gfn is best to > >> choose. I have a sinking feeling that the answer is ACPI... > > TLFS only says "go find one suitable page yourself" without further > > hints. > > > > Since we're still quite far away from a functioning system, finding a > > most suitable page isn't my top priority at this point. If there is a > > simple way to extrapolate suitable information from ACPI, that would be > > great. If it requires writing a set of functionalities, than that will > > need to wait till later. > > To cope with the "one is already established and it is already locked" > case, the only option is to have a fixmap entry which can be set > dynamically. The problem is that the fixmap region is marked NX and 64G > away from .text. > > Possibly the least bad option is to have some build-time space (so 0 or > 4k depending on CONFIG_HYPERV) between the per-cpu stubs and > XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings. > OK. This is probably not too difficult. > That way, the virtual address ends up in a useful position (wrt using > direct call instructions) irrespective of where the gfn is/ends up. As > for guessing, a good start is probably MAXPHYSADDR. To make sure I understand your correctly: you're talking about using the page just below MAXPHYSADDR (derived from paddr_bits from xen source), right? Wei. > > ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On 05/01/2020 21:45, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 05:37:44PM +, Andrew Cooper wrote:
>> On 05/01/2020 16:47, Wei Liu wrote:
>>> diff --git a/xen/arch/x86/guest/hyperv/Makefile
>>> b/xen/arch/x86/guest/hyperv/Makefile
>>> index 68170109a9..1a8887d2f4 100644
>>> --- a/xen/arch/x86/guest/hyperv/Makefile
>>> +++ b/xen/arch/x86/guest/hyperv/Makefile
>>> @@ -1 +1,2 @@
>>> +obj-y += hypercall_page.o
>>> obj-y += hyperv.o
>>> diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S
>>> b/xen/arch/x86/guest/hyperv/hypercall_page.S
>>> new file mode 100644
>>> index 00..6d6ab913be
>>> --- /dev/null
>>> +++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
>>> @@ -0,0 +1,21 @@
>>> +#include
>>> +#include
>>> +
>>> +.section ".text.page_aligned", "ax", @progbits
>>> +.p2align PAGE_SHIFT
>>> +GLOBAL(hv_hypercall_page)
>>> +/* Return -1 for "not yet ready" state */
>>> +mov -1, %rax
>>> +ret
>>> +1:
>>> +/* Fill the rest with `ret` */
>>> +.fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
>> If you want to fill with rets, you can do this more simply with:
>>
>> .p2lign PAGE_SHIFT, 0xc3
>>
>> which will do the size calculation for you.
>>
>> That said, I retract my statement about wanting this in the middle of
>> .text. (Sorry. See below.)
>>
>>> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
>>> b/xen/arch/x86/guest/hyperv/hyperv.c
>>> index 8d38313d7a..381be2a68c 100644
>>> --- a/xen/arch/x86/guest/hyperv/hyperv.c
>>> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
>>> @@ -72,6 +72,27 @@ const struct hypervisor_ops *__init hyperv_probe(void)
>>> return
>>> }
>>>
>>> +static void __init setup_hypercall_page(void)
>>> +{
>>> +union hv_x64_msr_hypercall_contents hypercall_msr;
>>> +
>>> +rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>> +hypercall_msr.enable = 1;
>>> +hypercall_msr.guest_physical_address =
>>> +__pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
>>> +wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>>> +}
>>> +
>>> +static void __init setup(void)
>>> +{
>>> +setup_hypercall_page();
>>> +}
>> The TLFS says that writing enable will fail until the OS identity is
>> set, which AFACIT, isn't done anywhere in the series. The whole
>> sequence is described in "3.13 Establishing the Hypercall Interface"
> Good catch. I will make up an identity number for Xen. I will also
> follow the sequence strictly.
>
>> The locked bit is probably a good idea, but one aspect missing here is
>> the check to see whether the hypercall page is already enabled, which I
>> expect is for a kexec crash scenario.
>>
>> However, the most important point is the one which describes the #GP
>> properties of the guest trying to modify the page. This can only be
>> achieved with an EPT/NPT mapping lacking the W permission, which will
>> shatter host superpages. Therefore, putting it in .text is going to be
>> rather poor, perf wise.
>>
>> I also note that Xen's implementation of the Viridian hypercall page
>> doesn't conform to these properties, and wants fixing. It is going to
>> need a new kind identification of the page (probably a new p2m type)
>> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
>>
>> As for suggestions here, I'm struggling to find any memory map details
>> exposed in the Viridian interface, and therefore which gfn is best to
>> choose. I have a sinking feeling that the answer is ACPI...
> TLFS only says "go find one suitable page yourself" without further
> hints.
>
> Since we're still quite far away from a functioning system, finding a
> most suitable page isn't my top priority at this point. If there is a
> simple way to extrapolate suitable information from ACPI, that would be
> great. If it requires writing a set of functionalities, than that will
> need to wait till later.
To cope with the "one is already established and it is already locked"
case, the only option is to have a fixmap entry which can be set
dynamically. The problem is that the fixmap region is marked NX and 64G
away from .text.
Possibly the least bad option is to have some build-time space (so 0 or
4k depending on CONFIG_HYPERV) between the per-cpu stubs and
XEN_VIRT_END, which operates like the fixmap, but ends up as X/RO mappings.
That way, the virtual address ends up in a useful position (wrt using
direct call instructions) irrespective of where the gfn is/ends up. As
for guessing, a good start is probably MAXPHYSADDR.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On Sun, Jan 05, 2020 at 05:37:44PM +, Andrew Cooper wrote:
> On 05/01/2020 16:47, Wei Liu wrote:
> > diff --git a/xen/arch/x86/guest/hyperv/Makefile
> > b/xen/arch/x86/guest/hyperv/Makefile
> > index 68170109a9..1a8887d2f4 100644
> > --- a/xen/arch/x86/guest/hyperv/Makefile
> > +++ b/xen/arch/x86/guest/hyperv/Makefile
> > @@ -1 +1,2 @@
> > +obj-y += hypercall_page.o
> > obj-y += hyperv.o
> > diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S
> > b/xen/arch/x86/guest/hyperv/hypercall_page.S
> > new file mode 100644
> > index 00..6d6ab913be
> > --- /dev/null
> > +++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
> > @@ -0,0 +1,21 @@
> > +#include
> > +#include
> > +
> > +.section ".text.page_aligned", "ax", @progbits
> > +.p2align PAGE_SHIFT
> > +GLOBAL(hv_hypercall_page)
> > +/* Return -1 for "not yet ready" state */
> > +mov -1, %rax
> > +ret
> > +1:
> > +/* Fill the rest with `ret` */
> > +.fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
>
> If you want to fill with rets, you can do this more simply with:
>
> .p2lign PAGE_SHIFT, 0xc3
>
> which will do the size calculation for you.
>
> That said, I retract my statement about wanting this in the middle of
> .text. (Sorry. See below.)
>
> > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> > b/xen/arch/x86/guest/hyperv/hyperv.c
> > index 8d38313d7a..381be2a68c 100644
> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -72,6 +72,27 @@ const struct hypervisor_ops *__init hyperv_probe(void)
> > return
> > }
> >
> > +static void __init setup_hypercall_page(void)
> > +{
> > +union hv_x64_msr_hypercall_contents hypercall_msr;
> > +
> > +rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +hypercall_msr.enable = 1;
> > +hypercall_msr.guest_physical_address =
> > +__pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
> > +wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +}
> > +
> > +static void __init setup(void)
> > +{
> > +setup_hypercall_page();
> > +}
>
> The TLFS says that writing enable will fail until the OS identity is
> set, which AFACIT, isn't done anywhere in the series. The whole
> sequence is described in "3.13 Establishing the Hypercall Interface"
Good catch. I will make up an identity number for Xen. I will also
follow the sequence strictly.
>
> The locked bit is probably a good idea, but one aspect missing here is
> the check to see whether the hypercall page is already enabled, which I
> expect is for a kexec crash scenario.
>
> However, the most important point is the one which describes the #GP
> properties of the guest trying to modify the page. This can only be
> achieved with an EPT/NPT mapping lacking the W permission, which will
> shatter host superpages. Therefore, putting it in .text is going to be
> rather poor, perf wise.
>
> I also note that Xen's implementation of the Viridian hypercall page
> doesn't conform to these properties, and wants fixing. It is going to
> need a new kind identification of the page (probably a new p2m type)
> which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
>
> As for suggestions here, I'm struggling to find any memory map details
> exposed in the Viridian interface, and therefore which gfn is best to
> choose. I have a sinking feeling that the answer is ACPI...
TLFS only says "go find one suitable page yourself" without further
hints.
Since we're still quite far away from a functioning system, finding a
most suitable page isn't my top priority at this point. If there is a
simple way to extrapolate suitable information from ACPI, that would be
great. If it requires writing a set of functionalities, than that will
need to wait till later.
Wei.
>
> ~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
On 05/01/2020 16:47, Wei Liu wrote:
> diff --git a/xen/arch/x86/guest/hyperv/Makefile
> b/xen/arch/x86/guest/hyperv/Makefile
> index 68170109a9..1a8887d2f4 100644
> --- a/xen/arch/x86/guest/hyperv/Makefile
> +++ b/xen/arch/x86/guest/hyperv/Makefile
> @@ -1 +1,2 @@
> +obj-y += hypercall_page.o
> obj-y += hyperv.o
> diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S
> b/xen/arch/x86/guest/hyperv/hypercall_page.S
> new file mode 100644
> index 00..6d6ab913be
> --- /dev/null
> +++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
> @@ -0,0 +1,21 @@
> +#include
> +#include
> +
> +.section ".text.page_aligned", "ax", @progbits
> +.p2align PAGE_SHIFT
> +GLOBAL(hv_hypercall_page)
> +/* Return -1 for "not yet ready" state */
> +mov -1, %rax
> +ret
> +1:
> +/* Fill the rest with `ret` */
> +.fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
If you want to fill with rets, you can do this more simply with:
.p2lign PAGE_SHIFT, 0xc3
which will do the size calculation for you.
That said, I retract my statement about wanting this in the middle of
.text. (Sorry. See below.)
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index 8d38313d7a..381be2a68c 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -72,6 +72,27 @@ const struct hypervisor_ops *__init hyperv_probe(void)
> return
> }
>
> +static void __init setup_hypercall_page(void)
> +{
> +union hv_x64_msr_hypercall_contents hypercall_msr;
> +
> +rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +hypercall_msr.enable = 1;
> +hypercall_msr.guest_physical_address =
> +__pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
> +wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +}
> +
> +static void __init setup(void)
> +{
> +setup_hypercall_page();
> +}
The TLFS says that writing enable will fail until the OS identity is
set, which AFACIT, isn't done anywhere in the series. The whole
sequence is described in "3.13 Establishing the Hypercall Interface"
The locked bit is probably a good idea, but one aspect missing here is
the check to see whether the hypercall page is already enabled, which I
expect is for a kexec crash scenario.
However, the most important point is the one which describes the #GP
properties of the guest trying to modify the page. This can only be
achieved with an EPT/NPT mapping lacking the W permission, which will
shatter host superpages. Therefore, putting it in .text is going to be
rather poor, perf wise.
I also note that Xen's implementation of the Viridian hypercall page
doesn't conform to these properties, and wants fixing. It is going to
need a new kind identification of the page (probably a new p2m type)
which injects #GP if we ever see an EPT_VIOLATION/NPT_FAULT against it.
As for suggestions here, I'm struggling to find any memory map details
exposed in the Viridian interface, and therefore which gfn is best to
choose. I have a sinking feeling that the answer is ACPI...
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 1/5] x86/hyperv: setup hypercall page
Signed-off-by: Wei Liu
---
v2:
1. Fix issue discovered by Michael
2. Use a statically allocated page as hypercall page
---
xen/arch/x86/guest/hyperv/Makefile | 1 +
xen/arch/x86/guest/hyperv/hypercall_page.S | 21 +
xen/arch/x86/guest/hyperv/hyperv.c | 27 +++---
3 files changed, 46 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/x86/guest/hyperv/hypercall_page.S
diff --git a/xen/arch/x86/guest/hyperv/Makefile
b/xen/arch/x86/guest/hyperv/Makefile
index 68170109a9..1a8887d2f4 100644
--- a/xen/arch/x86/guest/hyperv/Makefile
+++ b/xen/arch/x86/guest/hyperv/Makefile
@@ -1 +1,2 @@
+obj-y += hypercall_page.o
obj-y += hyperv.o
diff --git a/xen/arch/x86/guest/hyperv/hypercall_page.S
b/xen/arch/x86/guest/hyperv/hypercall_page.S
new file mode 100644
index 00..6d6ab913be
--- /dev/null
+++ b/xen/arch/x86/guest/hyperv/hypercall_page.S
@@ -0,0 +1,21 @@
+#include
+#include
+
+.section ".text.page_aligned", "ax", @progbits
+.p2align PAGE_SHIFT
+GLOBAL(hv_hypercall_page)
+/* Return -1 for "not yet ready" state */
+mov -1, %rax
+ret
+1:
+/* Fill the rest with `ret` */
+.fill PAGE_SIZE - (1b - hv_hypercall_page), 1, 0xc3
+.type hv_hypercall_page, STT_OBJECT
+.size hv_hypercall_page, PAGE_SIZE
+
+/*
+ * Local variables:
+ * tab-width: 8
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c
b/xen/arch/x86/guest/hyperv/hyperv.c
index 8d38313d7a..381be2a68c 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -19,16 +19,16 @@
* Copyright (c) 2019 Microsoft.
*/
#include
+#include
#include
#include
struct ms_hyperv_info __read_mostly ms_hyperv;
-static const struct hypervisor_ops ops = {
-.name = "Hyper-V",
-};
+extern char hv_hypercall_page[];
+static const struct hypervisor_ops ops;
const struct hypervisor_ops *__init hyperv_probe(void)
{
uint32_t eax, ebx, ecx, edx;
@@ -72,6 +72,27 @@ const struct hypervisor_ops *__init hyperv_probe(void)
return
}
+static void __init setup_hypercall_page(void)
+{
+union hv_x64_msr_hypercall_contents hypercall_msr;
+
+rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+hypercall_msr.enable = 1;
+hypercall_msr.guest_physical_address =
+__pa(hv_hypercall_page) >> HV_HYP_PAGE_SHIFT;
+wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+}
+
+static void __init setup(void)
+{
+setup_hypercall_page();
+}
+
+static const struct hypervisor_ops ops = {
+.name = "Hyper-V",
+.setup = setup,
+};
+
/*
* Local variables:
* mode: C
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
