When Linux is running as the root partition, the hypercall page will
have already been setup by Hyper-V. Copy the content over to the
allocated page.
The suspend, resume and cleanup paths remain untouched because they are
not supported in this setup yet.
Signed-off-by: Lillian Grassin-Drake <[email protected]>
Signed-off-by: Sunil Muthuswamy <[email protected]>
Signed-off-by: Nuno Das Neves <[email protected]>
Co-Developed-by: Lillian Grassin-Drake <[email protected]>
Co-Developed-by: Sunil Muthuswamy <[email protected]>
Co-Developed-by: Nuno Das Neves <[email protected]>
Signed-off-by: Wei Liu <[email protected]>
---
arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 0eec1ed32023..26233aebc86c 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -25,6 +25,7 @@
#include <linux/cpuhotplug.h>
#include <linux/syscore_ops.h>
#include <clocksource/hyperv_timer.h>
+#include <linux/highmem.h>
/* Is Linux running as the root partition? */
bool hv_root_partition;
@@ -448,8 +449,29 @@ void __init hyperv_init(void)
rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
hypercall_msr.enable = 1;
- hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
- wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+
+ if (hv_root_partition) {
+ struct page *pg;
+ void *src, *dst;
+
+ /*
+ * Order is important here. We must enable the hypercall page
+ * so it is populated with code, then copy the code to an
+ * executable page.
+ */
+ wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+
+ pg = vmalloc_to_page(hv_hypercall_pg);
+ dst = kmap(pg);
+ src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
+ MEMREMAP_WB);
+ memcpy(dst, src, PAGE_SIZE);
+ memunmap(src);
+ kunmap(pg);
+ } else {
+ hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
+ wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+ }
/*
* Ignore any errors in setting up stimer clockevents
--
2.20.1
Wei Liu <[email protected]> writes:
> When Linux is running as the root partition, the hypercall page will
> have already been setup by Hyper-V. Copy the content over to the
> allocated page.
And we can't setup a new hypercall page by writing something different
to HV_X64_MSR_HYPERCALL, right?
>
> The suspend, resume and cleanup paths remain untouched because they are
> not supported in this setup yet.
>
> Signed-off-by: Lillian Grassin-Drake <[email protected]>
> Signed-off-by: Sunil Muthuswamy <[email protected]>
> Signed-off-by: Nuno Das Neves <[email protected]>
> Co-Developed-by: Lillian Grassin-Drake <[email protected]>
> Co-Developed-by: Sunil Muthuswamy <[email protected]>
> Co-Developed-by: Nuno Das Neves <[email protected]>
> Signed-off-by: Wei Liu <[email protected]>
> ---
> arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 0eec1ed32023..26233aebc86c 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -25,6 +25,7 @@
> #include <linux/cpuhotplug.h>
> #include <linux/syscore_ops.h>
> #include <clocksource/hyperv_timer.h>
> +#include <linux/highmem.h>
>
> /* Is Linux running as the root partition? */
> bool hv_root_partition;
> @@ -448,8 +449,29 @@ void __init hyperv_init(void)
>
> rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> hypercall_msr.enable = 1;
> - hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +
> + if (hv_root_partition) {
> + struct page *pg;
> + void *src, *dst;
> +
> + /*
> + * Order is important here. We must enable the hypercall page
> + * so it is populated with code, then copy the code to an
> + * executable page.
> + */
> + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +
> + pg = vmalloc_to_page(hv_hypercall_pg);
> + dst = kmap(pg);
> + src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
> + MEMREMAP_WB);
memremap() can fail...
> + memcpy(dst, src, PAGE_SIZE);
> + memunmap(src);
> + kunmap(pg);
> + } else {
> + hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> + }
Why can't we do wrmsrl() for both cases here?
>
> /*
> * Ignore any errors in setting up stimer clockevents
--
Vitaly
On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu <[email protected]> writes:
>
> > When Linux is running as the root partition, the hypercall page will
> > have already been setup by Hyper-V. Copy the content over to the
> > allocated page.
>
> And we can't setup a new hypercall page by writing something different
> to HV_X64_MSR_HYPERCALL, right?
>
My understanding is that we can't, but Sunil can maybe correct me.
> >
> > The suspend, resume and cleanup paths remain untouched because they are
> > not supported in this setup yet.
> >
> > Signed-off-by: Lillian Grassin-Drake <[email protected]>
> > Signed-off-by: Sunil Muthuswamy <[email protected]>
> > Signed-off-by: Nuno Das Neves <[email protected]>
> > Co-Developed-by: Lillian Grassin-Drake <[email protected]>
> > Co-Developed-by: Sunil Muthuswamy <[email protected]>
> > Co-Developed-by: Nuno Das Neves <[email protected]>
> > Signed-off-by: Wei Liu <[email protected]>
> > ---
> > arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 0eec1ed32023..26233aebc86c 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -25,6 +25,7 @@
> > #include <linux/cpuhotplug.h>
> > #include <linux/syscore_ops.h>
> > #include <clocksource/hyperv_timer.h>
> > +#include <linux/highmem.h>
> >
> > /* Is Linux running as the root partition? */
> > bool hv_root_partition;
> > @@ -448,8 +449,29 @@ void __init hyperv_init(void)
> >
> > rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > hypercall_msr.enable = 1;
> > - hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> > - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +
> > + if (hv_root_partition) {
> > + struct page *pg;
> > + void *src, *dst;
> > +
> > + /*
> > + * Order is important here. We must enable the hypercall page
> > + * so it is populated with code, then copy the code to an
> > + * executable page.
> > + */
> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +
> > + pg = vmalloc_to_page(hv_hypercall_pg);
> > + dst = kmap(pg);
> > + src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
> > + MEMREMAP_WB);
>
> memremap() can fail...
And we don't care here, if it fails, we would rather it panic or oops.
I was relying on the fact that copying from / to a NULL pointer will
cause the kernel to crash. But of course it wouldn't hurt to explicitly
panic here.
>
> > + memcpy(dst, src, PAGE_SIZE);
> > + memunmap(src);
> > + kunmap(pg);
> > + } else {
> > + hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > + }
>
> Why can't we do wrmsrl() for both cases here?
>
Because the hypercall page has already been set up when Linux is the
root.
I could've tried writing to the MSR again, but because the behaviour
here is not documented and subject to change so I didn't bother trying.
Wei.
> >
> > /*
> > * Ignore any errors in setting up stimer clockevents
>
> --
> Vitaly
>
Wei Liu <[email protected]> writes:
> On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
>> Wei Liu <[email protected]> writes:
>>
>> > When Linux is running as the root partition, the hypercall page will
>> > have already been setup by Hyper-V. Copy the content over to the
>> > allocated page.
>>
>> And we can't setup a new hypercall page by writing something different
>> to HV_X64_MSR_HYPERCALL, right?
>>
>
> My understanding is that we can't, but Sunil can maybe correct me.
>
>> >
>> > The suspend, resume and cleanup paths remain untouched because they are
>> > not supported in this setup yet.
>> >
>> > Signed-off-by: Lillian Grassin-Drake <[email protected]>
>> > Signed-off-by: Sunil Muthuswamy <[email protected]>
>> > Signed-off-by: Nuno Das Neves <[email protected]>
>> > Co-Developed-by: Lillian Grassin-Drake <[email protected]>
>> > Co-Developed-by: Sunil Muthuswamy <[email protected]>
>> > Co-Developed-by: Nuno Das Neves <[email protected]>
>> > Signed-off-by: Wei Liu <[email protected]>
>> > ---
>> > arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
>> > 1 file changed, 24 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> > index 0eec1ed32023..26233aebc86c 100644
>> > --- a/arch/x86/hyperv/hv_init.c
>> > +++ b/arch/x86/hyperv/hv_init.c
>> > @@ -25,6 +25,7 @@
>> > #include <linux/cpuhotplug.h>
>> > #include <linux/syscore_ops.h>
>> > #include <clocksource/hyperv_timer.h>
>> > +#include <linux/highmem.h>
>> >
>> > /* Is Linux running as the root partition? */
>> > bool hv_root_partition;
>> > @@ -448,8 +449,29 @@ void __init hyperv_init(void)
>> >
>> > rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> > hypercall_msr.enable = 1;
>> > - hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
>> > - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> > +
>> > + if (hv_root_partition) {
>> > + struct page *pg;
>> > + void *src, *dst;
>> > +
>> > + /*
>> > + * Order is important here. We must enable the hypercall page
>> > + * so it is populated with code, then copy the code to an
>> > + * executable page.
>> > + */
>> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> > +
>> > + pg = vmalloc_to_page(hv_hypercall_pg);
>> > + dst = kmap(pg);
>> > + src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
>> > + MEMREMAP_WB);
>>
>> memremap() can fail...
>
> And we don't care here, if it fails, we would rather it panic or oops.
>
> I was relying on the fact that copying from / to a NULL pointer will
> cause the kernel to crash. But of course it wouldn't hurt to explicitly
> panic here.
>
>>
>> > + memcpy(dst, src, PAGE_SIZE);
>> > + memunmap(src);
>> > + kunmap(pg);
>> > + } else {
>> > + hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
>> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> > + }
>>
>> Why can't we do wrmsrl() for both cases here?
>>
>
> Because the hypercall page has already been set up when Linux is the
> root.
But you already do wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64)
in 'if (hv_root_partition)' case above, that's why I asked.
>
> I could've tried writing to the MSR again, but because the behaviour
> here is not documented and subject to change so I didn't bother trying.
>
> Wei.
>
>> >
>> > /*
>> > * Ignore any errors in setting up stimer clockevents
>>
>> --
>> Vitaly
>>
>
--
Vitaly
Wei Liu <[email protected]> writes:
> On Tue, Sep 15, 2020 at 01:02:08PM +0200, Vitaly Kuznetsov wrote:
>> Wei Liu <[email protected]> writes:
>>
>> > On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
>> >> Wei Liu <[email protected]> writes:
>> >>
>> >> > When Linux is running as the root partition, the hypercall page will
>> >> > have already been setup by Hyper-V. Copy the content over to the
>> >> > allocated page.
>> >>
>> >> And we can't setup a new hypercall page by writing something different
>> >> to HV_X64_MSR_HYPERCALL, right?
>> >>
>> >
>> > My understanding is that we can't, but Sunil can maybe correct me.
>> >
>> >> >
>> >> > The suspend, resume and cleanup paths remain untouched because they are
>> >> > not supported in this setup yet.
>> >> >
>> >> > Signed-off-by: Lillian Grassin-Drake <[email protected]>
>> >> > Signed-off-by: Sunil Muthuswamy <[email protected]>
>> >> > Signed-off-by: Nuno Das Neves <[email protected]>
>> >> > Co-Developed-by: Lillian Grassin-Drake <[email protected]>
>> >> > Co-Developed-by: Sunil Muthuswamy <[email protected]>
>> >> > Co-Developed-by: Nuno Das Neves <[email protected]>
>> >> > Signed-off-by: Wei Liu <[email protected]>
>> >> > ---
>> >> > arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
>> >> > 1 file changed, 24 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> >> > index 0eec1ed32023..26233aebc86c 100644
>> >> > --- a/arch/x86/hyperv/hv_init.c
>> >> > +++ b/arch/x86/hyperv/hv_init.c
>> >> > @@ -25,6 +25,7 @@
>> >> > #include <linux/cpuhotplug.h>
>> >> > #include <linux/syscore_ops.h>
>> >> > #include <clocksource/hyperv_timer.h>
>> >> > +#include <linux/highmem.h>
>> >> >
>> >> > /* Is Linux running as the root partition? */
>> >> > bool hv_root_partition;
>> >> > @@ -448,8 +449,29 @@ void __init hyperv_init(void)
>> >> >
>> >> > rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> >> > hypercall_msr.enable = 1;
>> >> > - hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
>> >> > - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> >> > +
>> >> > + if (hv_root_partition) {
>> >> > + struct page *pg;
>> >> > + void *src, *dst;
>> >> > +
>> >> > + /*
>> >> > + * Order is important here. We must enable the hypercall page
>> >> > + * so it is populated with code, then copy the code to an
>> >> > + * executable page.
>> >> > + */
>> >> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> >> > +
>> >> > + pg = vmalloc_to_page(hv_hypercall_pg);
>> >> > + dst = kmap(pg);
>> >> > + src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
>> >> > + MEMREMAP_WB);
>> >>
>> >> memremap() can fail...
>> >
>> > And we don't care here, if it fails, we would rather it panic or oops.
>> >
>> > I was relying on the fact that copying from / to a NULL pointer will
>> > cause the kernel to crash. But of course it wouldn't hurt to explicitly
>> > panic here.
>> >
>> >>
>> >> > + memcpy(dst, src, PAGE_SIZE);
>> >> > + memunmap(src);
>> >> > + kunmap(pg);
>> >> > + } else {
>> >> > + hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
>> >> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>> >> > + }
>> >>
>> >> Why can't we do wrmsrl() for both cases here?
>> >>
>> >
>> > Because the hypercall page has already been set up when Linux is the
>> > root.
>>
>> But you already do wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64)
>> in 'if (hv_root_partition)' case above, that's why I asked.
>>
>
> You mean extracting wrmsrl to this point? The ordering matters. See the
> comment in the root branch -- we have to enable the page before copying
> the content.
>
> What can be done is:
>
> if (!root) {
> /* some stuff */
> }
>
> wrmsrl(...)
>
> if (root) {
> /* some stuff */
> }
>
> This is not looking any better than the existing code.
>
Oh, I missed the comment indeed. So Hypervisor already picked a page for
us, however, it didn't enable it and it's not populated? How can we be
sure that we didn't use it for something else already? Maybe we can
still give a different known-to-be-empty page?
--
Vitaly
On Tue, Sep 15, 2020 at 01:23:50PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu <[email protected]> writes:
>
> > On Tue, Sep 15, 2020 at 01:02:08PM +0200, Vitaly Kuznetsov wrote:
> >> Wei Liu <[email protected]> writes:
> >>
> >> > On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
> >> >> Wei Liu <[email protected]> writes:
> >> >>
> >> >> > When Linux is running as the root partition, the hypercall page will
> >> >> > have already been setup by Hyper-V. Copy the content over to the
> >> >> > allocated page.
> >> >>
> >> >> And we can't setup a new hypercall page by writing something different
> >> >> to HV_X64_MSR_HYPERCALL, right?
> >> >>
> >> >
> >> > My understanding is that we can't, but Sunil can maybe correct me.
> >> >
> >> >> >
> >> >> > The suspend, resume and cleanup paths remain untouched because they are
> >> >> > not supported in this setup yet.
> >> >> >
> >> >> > Signed-off-by: Lillian Grassin-Drake <[email protected]>
> >> >> > Signed-off-by: Sunil Muthuswamy <[email protected]>
> >> >> > Signed-off-by: Nuno Das Neves <[email protected]>
> >> >> > Co-Developed-by: Lillian Grassin-Drake <[email protected]>
> >> >> > Co-Developed-by: Sunil Muthuswamy <[email protected]>
> >> >> > Co-Developed-by: Nuno Das Neves <[email protected]>
> >> >> > Signed-off-by: Wei Liu <[email protected]>
> >> >> > ---
> >> >> > arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
> >> >> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> >> >> > index 0eec1ed32023..26233aebc86c 100644
> >> >> > --- a/arch/x86/hyperv/hv_init.c
> >> >> > +++ b/arch/x86/hyperv/hv_init.c
> >> >> > @@ -25,6 +25,7 @@
> >> >> > #include <linux/cpuhotplug.h>
> >> >> > #include <linux/syscore_ops.h>
> >> >> > #include <clocksource/hyperv_timer.h>
> >> >> > +#include <linux/highmem.h>
> >> >> >
> >> >> > /* Is Linux running as the root partition? */
> >> >> > bool hv_root_partition;
> >> >> > @@ -448,8 +449,29 @@ void __init hyperv_init(void)
> >> >> >
> >> >> > rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> >> > hypercall_msr.enable = 1;
> >> >> > - hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> >> >> > - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> >> > +
> >> >> > + if (hv_root_partition) {
> >> >> > + struct page *pg;
> >> >> > + void *src, *dst;
> >> >> > +
> >> >> > + /*
> >> >> > + * Order is important here. We must enable the hypercall page
> >> >> > + * so it is populated with code, then copy the code to an
> >> >> > + * executable page.
> >> >> > + */
> >> >> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> >> > +
> >> >> > + pg = vmalloc_to_page(hv_hypercall_pg);
> >> >> > + dst = kmap(pg);
> >> >> > + src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
> >> >> > + MEMREMAP_WB);
> >> >>
> >> >> memremap() can fail...
> >> >
> >> > And we don't care here, if it fails, we would rather it panic or oops.
> >> >
> >> > I was relying on the fact that copying from / to a NULL pointer will
> >> > cause the kernel to crash. But of course it wouldn't hurt to explicitly
> >> > panic here.
> >> >
> >> >>
> >> >> > + memcpy(dst, src, PAGE_SIZE);
> >> >> > + memunmap(src);
> >> >> > + kunmap(pg);
> >> >> > + } else {
> >> >> > + hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> >> >> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> >> > + }
> >> >>
> >> >> Why can't we do wrmsrl() for both cases here?
> >> >>
> >> >
> >> > Because the hypercall page has already been set up when Linux is the
> >> > root.
> >>
> >> But you already do wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64)
> >> in 'if (hv_root_partition)' case above, that's why I asked.
> >>
> >
> > You mean extracting wrmsrl to this point? The ordering matters. See the
> > comment in the root branch -- we have to enable the page before copying
> > the content.
> >
> > What can be done is:
> >
> > if (!root) {
> > /* some stuff */
> > }
> >
> > wrmsrl(...)
> >
> > if (root) {
> > /* some stuff */
> > }
> >
> > This is not looking any better than the existing code.
> >
>
> Oh, I missed the comment indeed. So Hypervisor already picked a page for
> us, however, it didn't enable it and it's not populated?
Seems to be the case.
> How can we be
> sure that we didn't use it for something else already?
It is a page deposited to the root partition and it is not going to be
used elsewhere, nor it is visible from the root. This is my
understanding. I will let Sunil correct me if I'm wrong.
> Maybe we can
> still give a different known-to-be-empty page?
>
That's the thing I said I didn't bother trying earlier. Something to
check when I have some spare cycles.
Wei.
> --
> Vitaly
>
On Tue, Sep 15, 2020 at 01:02:08PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu <[email protected]> writes:
>
> > On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
> >> Wei Liu <[email protected]> writes:
> >>
> >> > When Linux is running as the root partition, the hypercall page will
> >> > have already been setup by Hyper-V. Copy the content over to the
> >> > allocated page.
> >>
> >> And we can't setup a new hypercall page by writing something different
> >> to HV_X64_MSR_HYPERCALL, right?
> >>
> >
> > My understanding is that we can't, but Sunil can maybe correct me.
> >
> >> >
> >> > The suspend, resume and cleanup paths remain untouched because they are
> >> > not supported in this setup yet.
> >> >
> >> > Signed-off-by: Lillian Grassin-Drake <[email protected]>
> >> > Signed-off-by: Sunil Muthuswamy <[email protected]>
> >> > Signed-off-by: Nuno Das Neves <[email protected]>
> >> > Co-Developed-by: Lillian Grassin-Drake <[email protected]>
> >> > Co-Developed-by: Sunil Muthuswamy <[email protected]>
> >> > Co-Developed-by: Nuno Das Neves <[email protected]>
> >> > Signed-off-by: Wei Liu <[email protected]>
> >> > ---
> >> > arch/x86/hyperv/hv_init.c | 26 ++++++++++++++++++++++++--
> >> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> >> > index 0eec1ed32023..26233aebc86c 100644
> >> > --- a/arch/x86/hyperv/hv_init.c
> >> > +++ b/arch/x86/hyperv/hv_init.c
> >> > @@ -25,6 +25,7 @@
> >> > #include <linux/cpuhotplug.h>
> >> > #include <linux/syscore_ops.h>
> >> > #include <clocksource/hyperv_timer.h>
> >> > +#include <linux/highmem.h>
> >> >
> >> > /* Is Linux running as the root partition? */
> >> > bool hv_root_partition;
> >> > @@ -448,8 +449,29 @@ void __init hyperv_init(void)
> >> >
> >> > rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> > hypercall_msr.enable = 1;
> >> > - hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> >> > - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> > +
> >> > + if (hv_root_partition) {
> >> > + struct page *pg;
> >> > + void *src, *dst;
> >> > +
> >> > + /*
> >> > + * Order is important here. We must enable the hypercall page
> >> > + * so it is populated with code, then copy the code to an
> >> > + * executable page.
> >> > + */
> >> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> > +
> >> > + pg = vmalloc_to_page(hv_hypercall_pg);
> >> > + dst = kmap(pg);
> >> > + src = memremap(hypercall_msr.guest_physical_address << PAGE_SHIFT, PAGE_SIZE,
> >> > + MEMREMAP_WB);
> >>
> >> memremap() can fail...
> >
> > And we don't care here, if it fails, we would rather it panic or oops.
> >
> > I was relying on the fact that copying from / to a NULL pointer will
> > cause the kernel to crash. But of course it wouldn't hurt to explicitly
> > panic here.
> >
> >>
> >> > + memcpy(dst, src, PAGE_SIZE);
> >> > + memunmap(src);
> >> > + kunmap(pg);
> >> > + } else {
> >> > + hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
> >> > + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> >> > + }
> >>
> >> Why can't we do wrmsrl() for both cases here?
> >>
> >
> > Because the hypercall page has already been set up when Linux is the
> > root.
>
> But you already do wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64)
> in 'if (hv_root_partition)' case above, that's why I asked.
>
You mean extracting wrmsrl to this point? The ordering matters. See the
comment in the root branch -- we have to enable the page before copying
the content.
What can be done is:
if (!root) {
/* some stuff */
}
wrmsrl(...)
if (root) {
/* some stuff */
}
This is not looking any better than the existing code.
Wei.
> >
> > I could've tried writing to the MSR again, but because the behaviour
> > here is not documented and subject to change so I didn't bother trying.
> >
> > Wei.
> >
> >> >
> >> > /*
> >> > * Ignore any errors in setting up stimer clockevents
> >>
> >> --
> >> Vitaly
> >>
> >
>
> --
> Vitaly
>
>
> On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
> > Wei Liu <[email protected]> writes:
> >
> > > When Linux is running as the root partition, the hypercall page will
> > > have already been setup by Hyper-V. Copy the content over to the
> > > allocated page.
> >
> > And we can't setup a new hypercall page by writing something different
> > to HV_X64_MSR_HYPERCALL, right?
> >
>
> My understanding is that we can't, but Sunil can maybe correct me.
That is correct. For root partition, the hypervisor has already allocated the
hypercall page. The root is required to query the page, map it in its address
space and wrmsr to enable it. It cannot change the location of the page. For
guest, it can allocate and assign the hypercall page. This is covered a bit in the
hypervisor TLFS (section 3.13 in TLFS v6), for the guest side. The root side is
not covered there, yet.
Sunil Muthuswamy <[email protected]> writes:
>>
>> On Tue, Sep 15, 2020 at 12:32:29PM +0200, Vitaly Kuznetsov wrote:
>> > Wei Liu <[email protected]> writes:
>> >
>> > > When Linux is running as the root partition, the hypercall page will
>> > > have already been setup by Hyper-V. Copy the content over to the
>> > > allocated page.
>> >
>> > And we can't setup a new hypercall page by writing something different
>> > to HV_X64_MSR_HYPERCALL, right?
>> >
>>
>> My understanding is that we can't, but Sunil can maybe correct me.
>
> That is correct. For root partition, the hypervisor has already allocated the
> hypercall page. The root is required to query the page, map it in its address
> space and wrmsr to enable it. It cannot change the location of the page. For
> guest, it can allocate and assign the hypercall page. This is covered a bit in the
> hypervisor TLFS (section 3.13 in TLFS v6), for the guest side. The root side is
> not covered there, yet.
Ok, so it is guaranteed that root partition doesn't have this page in
its address space yet, otherwise it could've been used for something
else (in case it's just normal memory from its PoV).
Please add a comment about this as it is not really obvious.
Thanks,
--
Vitaly