* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Baoquan He @ 2020-03-18 14:50 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv,
Vitaly Kuznetsov, Yumei Huang, Igor Mammedov, Eduardo Habkost,
Milan Zamazal, Andrew Morton, Benjamin Herrenschmidt,
Greg Kroah-Hartman, Haiyang Zhang, K. Y. Srinivasan,
Michael Ellerman, Michal Hocko, Michal Hocko, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <67a054f6-df07-e4fb-dd4b-e503cb767276@redhat.com>
On 03/18/20 at 02:50pm, David Hildenbrand wrote:
> On 18.03.20 14:05, Baoquan He wrote:
> > On 03/17/20 at 11:49am, David Hildenbrand wrote:
> >> Distributions nowadays use udev rules ([1] [2]) to specify if and
> >> how to online hotplugged memory. The rules seem to get more complex with
> >> many special cases. Due to the various special cases,
> >> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
> >> is handled via udev rules.
> >>
> >> Everytime we hotplug memory, the udev rule will come to the same
> >> conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
> >> memory in separate memory blocks and wait for memory to get onlined by user
> >> space before continuing to add more memory blocks (to not add memory faster
> >> than it is getting onlined). This of course slows down the whole memory
> >> hotplug process.
> >>
> >> To make the job of distributions easier and to avoid udev rules that get
> >> more and more complicated, let's extend the mechanism provided by
> >> - /sys/devices/system/memory/auto_online_blocks
> >> - "memhp_default_state=" on the kernel cmdline
> >> to be able to specify also "online_movable" as well as "online_kernel"
> >
> > This patch series looks good, thanks. Since Andrew has merged it to -mm again,
> > I won't add my Reviewed-by to bother.
> >
> > Hi David, Vitaly
> >
> > There are several things unclear to me.
> >
> > So, these improved interfaces are used to alleviate the burden of the
> > existing udev rules, or try to replace it? As you know, we have been
>
> At least in RHEL, my plan is to replace it / use a udev rules as a
> fallback on older kernels (see the example scripts below). But other
Ok, got it. Didn't notice the script and the systemd service are your
part of plan, thought you are demonstrating the status. Thanks.
> distribution can handle it as they want.
>
> > using udev rules to interact between kernel and user space on bare metal,
> > and guests who want to hot add/remove.>
> > And also the OOM issue in hyperV when onlining pages after adding memory
> > block. I am not a virt devel expert, could this happen on bare metal
> > system?
>
> Don't think it's relevant on bare metal. If you plug a big DIMM, all
> memory blocks will be added first in one shot and then all memory blocks
> will be onlined. So it doesn't matter "how fast" you online that memory.
>
> In contrast, Hyper-V (and virtio-mem) add one (or a limited number of)
> memory block at a time and wait for them to get onlined.
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Baoquan He @ 2020-03-18 14:41 UTC (permalink / raw)
To: Michal Hocko
Cc: David Hildenbrand, linux-kernel, linux-mm, linuxppc-dev,
linux-hyperv, Vitaly Kuznetsov, Yumei Huang, Igor Mammedov,
Eduardo Habkost, Milan Zamazal, Andrew Morton,
Benjamin Herrenschmidt, Greg Kroah-Hartman, Haiyang Zhang,
K. Y. Srinivasan, Michael Ellerman, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <20200318135408.GP21362@dhcp22.suse.cz>
On 03/18/20 at 02:54pm, Michal Hocko wrote:
> On Wed 18-03-20 21:05:17, Baoquan He wrote:
> > On 03/17/20 at 11:49am, David Hildenbrand wrote:
> > > Distributions nowadays use udev rules ([1] [2]) to specify if and
> > > how to online hotplugged memory. The rules seem to get more complex with
> > > many special cases. Due to the various special cases,
> > > CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
> > > is handled via udev rules.
> > >
> > > Everytime we hotplug memory, the udev rule will come to the same
> > > conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
> > > memory in separate memory blocks and wait for memory to get onlined by user
> > > space before continuing to add more memory blocks (to not add memory faster
> > > than it is getting onlined). This of course slows down the whole memory
> > > hotplug process.
> > >
> > > To make the job of distributions easier and to avoid udev rules that get
> > > more and more complicated, let's extend the mechanism provided by
> > > - /sys/devices/system/memory/auto_online_blocks
> > > - "memhp_default_state=" on the kernel cmdline
> > > to be able to specify also "online_movable" as well as "online_kernel"
> >
> > This patch series looks good, thanks. Since Andrew has merged it to -mm again,
> > I won't add my Reviewed-by to bother.
>
> JFYI, Andrew usually adds R-b or A-b tags as they are posted.
Got it, thanks for telling.
^ permalink raw reply
* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Baoquan He @ 2020-03-18 14:41 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: David Hildenbrand, linux-kernel, linux-mm, linuxppc-dev,
linux-hyperv, Yumei Huang, Igor Mammedov, Eduardo Habkost,
Milan Zamazal, Andrew Morton, Benjamin Herrenschmidt,
Greg Kroah-Hartman, Haiyang Zhang, K. Y. Srinivasan,
Michael Ellerman, Michal Hocko, Michal Hocko, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <87d0993gto.fsf@vitty.brq.redhat.com>
On 03/18/20 at 02:58pm, Vitaly Kuznetsov wrote:
> Baoquan He <bhe@redhat.com> writes:
>
> > On 03/17/20 at 11:49am, David Hildenbrand wrote:
> >> Distributions nowadays use udev rules ([1] [2]) to specify if and
> >> how to online hotplugged memory. The rules seem to get more complex with
> >> many special cases. Due to the various special cases,
> >> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
> >> is handled via udev rules.
> >>
> >> Everytime we hotplug memory, the udev rule will come to the same
> >> conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
> >> memory in separate memory blocks and wait for memory to get onlined by user
> >> space before continuing to add more memory blocks (to not add memory faster
> >> than it is getting onlined). This of course slows down the whole memory
> >> hotplug process.
> >>
> >> To make the job of distributions easier and to avoid udev rules that get
> >> more and more complicated, let's extend the mechanism provided by
> >> - /sys/devices/system/memory/auto_online_blocks
> >> - "memhp_default_state=" on the kernel cmdline
> >> to be able to specify also "online_movable" as well as "online_kernel"
> >
> > This patch series looks good, thanks. Since Andrew has merged it to -mm again,
> > I won't add my Reviewed-by to bother.
> >
> > Hi David, Vitaly
> >
> > There are several things unclear to me.
> >
> > So, these improved interfaces are used to alleviate the burden of the
> > existing udev rules, or try to replace it? As you know, we have been
> > using udev rules to interact between kernel and user space on bare metal,
> > and guests who want to hot add/remove.
>
> With 'auto_online_blocks' interface you don't need the udev rule. David
> is trying to make it more versatile.
>
> >
> > And also the OOM issue in hyperV when onlining pages after adding memory
> > block. I am not a virt devel expert, could this happen on bare metal
> > system?
>
> Yes - in theory, very unlikely - in practice.
>
> The root cause of the problem here is adding more memory to the system
> requires memory (page tables, memmaps,..) so if your system is low on
> memory and you're trying to hotplug A LOT you may run into OOM before
> you're able to online anything. With bare metal it's usualy not the
> case: servers, which are able to hotplug memory, are usually booted with
> enough memory and memory hotplug is a manual action (you need to insert
> DIMMs!). But, if you boot your server with e.g. 4G, almost exhaust it
> and then try to hotplug e.g. 256G ... well, OOM is almost guaranteed.
Thanks for this detailed explanation.
I finally know why this is a problem in hyperV. But with the current
mechanism, it will happen on any system if thing is done like this.
Is there a reason hyperV need boot with small memory, then enlarge it
with huge memory? Since it's a real case in hyperV, I guess there must
be reason, I am just curious.
> With virtual machines it's very common (e.g. with Hyper-V VMs) to boot
> them with low memory and hotplug it (automatically, by some management
> software) when neededm thus the problem is way more common.
^ permalink raw reply
* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Vitaly Kuznetsov @ 2020-03-18 13:58 UTC (permalink / raw)
To: Baoquan He, David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Yumei Huang,
Igor Mammedov, Eduardo Habkost, Milan Zamazal, Andrew Morton,
Benjamin Herrenschmidt, Greg Kroah-Hartman, Haiyang Zhang,
K. Y. Srinivasan, Michael Ellerman, Michal Hocko, Michal Hocko,
Oscar Salvador, Paul Mackerras, Rafael J. Wysocki,
Stephen Hemminger, Wei Liu, Wei Yang
In-Reply-To: <20200318130517.GC30899@MiWiFi-R3L-srv>
Baoquan He <bhe@redhat.com> writes:
> On 03/17/20 at 11:49am, David Hildenbrand wrote:
>> Distributions nowadays use udev rules ([1] [2]) to specify if and
>> how to online hotplugged memory. The rules seem to get more complex with
>> many special cases. Due to the various special cases,
>> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
>> is handled via udev rules.
>>
>> Everytime we hotplug memory, the udev rule will come to the same
>> conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
>> memory in separate memory blocks and wait for memory to get onlined by user
>> space before continuing to add more memory blocks (to not add memory faster
>> than it is getting onlined). This of course slows down the whole memory
>> hotplug process.
>>
>> To make the job of distributions easier and to avoid udev rules that get
>> more and more complicated, let's extend the mechanism provided by
>> - /sys/devices/system/memory/auto_online_blocks
>> - "memhp_default_state=" on the kernel cmdline
>> to be able to specify also "online_movable" as well as "online_kernel"
>
> This patch series looks good, thanks. Since Andrew has merged it to -mm again,
> I won't add my Reviewed-by to bother.
>
> Hi David, Vitaly
>
> There are several things unclear to me.
>
> So, these improved interfaces are used to alleviate the burden of the
> existing udev rules, or try to replace it? As you know, we have been
> using udev rules to interact between kernel and user space on bare metal,
> and guests who want to hot add/remove.
With 'auto_online_blocks' interface you don't need the udev rule. David
is trying to make it more versatile.
>
> And also the OOM issue in hyperV when onlining pages after adding memory
> block. I am not a virt devel expert, could this happen on bare metal
> system?
Yes - in theory, very unlikely - in practice.
The root cause of the problem here is adding more memory to the system
requires memory (page tables, memmaps,..) so if your system is low on
memory and you're trying to hotplug A LOT you may run into OOM before
you're able to online anything. With bare metal it's usualy not the
case: servers, which are able to hotplug memory, are usually booted with
enough memory and memory hotplug is a manual action (you need to insert
DIMMs!). But, if you boot your server with e.g. 4G, almost exhaust it
and then try to hotplug e.g. 256G ... well, OOM is almost guaranteed.
With virtual machines it's very common (e.g. with Hyper-V VMs) to boot
them with low memory and hotplug it (automatically, by some management
software) when neededm thus the problem is way more common.
--
Vitaly
^ permalink raw reply
* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Michal Hocko @ 2020-03-18 13:54 UTC (permalink / raw)
To: Baoquan He
Cc: David Hildenbrand, linux-kernel, linux-mm, linuxppc-dev,
linux-hyperv, Vitaly Kuznetsov, Yumei Huang, Igor Mammedov,
Eduardo Habkost, Milan Zamazal, Andrew Morton,
Benjamin Herrenschmidt, Greg Kroah-Hartman, Haiyang Zhang,
K. Y. Srinivasan, Michael Ellerman, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <20200318130517.GC30899@MiWiFi-R3L-srv>
On Wed 18-03-20 21:05:17, Baoquan He wrote:
> On 03/17/20 at 11:49am, David Hildenbrand wrote:
> > Distributions nowadays use udev rules ([1] [2]) to specify if and
> > how to online hotplugged memory. The rules seem to get more complex with
> > many special cases. Due to the various special cases,
> > CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
> > is handled via udev rules.
> >
> > Everytime we hotplug memory, the udev rule will come to the same
> > conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
> > memory in separate memory blocks and wait for memory to get onlined by user
> > space before continuing to add more memory blocks (to not add memory faster
> > than it is getting onlined). This of course slows down the whole memory
> > hotplug process.
> >
> > To make the job of distributions easier and to avoid udev rules that get
> > more and more complicated, let's extend the mechanism provided by
> > - /sys/devices/system/memory/auto_online_blocks
> > - "memhp_default_state=" on the kernel cmdline
> > to be able to specify also "online_movable" as well as "online_kernel"
>
> This patch series looks good, thanks. Since Andrew has merged it to -mm again,
> I won't add my Reviewed-by to bother.
JFYI, Andrew usually adds R-b or A-b tags as they are posted.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: David Hildenbrand @ 2020-03-18 13:50 UTC (permalink / raw)
To: Baoquan He
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv,
Vitaly Kuznetsov, Yumei Huang, Igor Mammedov, Eduardo Habkost,
Milan Zamazal, Andrew Morton, Benjamin Herrenschmidt,
Greg Kroah-Hartman, Haiyang Zhang, K. Y. Srinivasan,
Michael Ellerman, Michal Hocko, Michal Hocko, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <20200318130517.GC30899@MiWiFi-R3L-srv>
On 18.03.20 14:05, Baoquan He wrote:
> On 03/17/20 at 11:49am, David Hildenbrand wrote:
>> Distributions nowadays use udev rules ([1] [2]) to specify if and
>> how to online hotplugged memory. The rules seem to get more complex with
>> many special cases. Due to the various special cases,
>> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
>> is handled via udev rules.
>>
>> Everytime we hotplug memory, the udev rule will come to the same
>> conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
>> memory in separate memory blocks and wait for memory to get onlined by user
>> space before continuing to add more memory blocks (to not add memory faster
>> than it is getting onlined). This of course slows down the whole memory
>> hotplug process.
>>
>> To make the job of distributions easier and to avoid udev rules that get
>> more and more complicated, let's extend the mechanism provided by
>> - /sys/devices/system/memory/auto_online_blocks
>> - "memhp_default_state=" on the kernel cmdline
>> to be able to specify also "online_movable" as well as "online_kernel"
>
> This patch series looks good, thanks. Since Andrew has merged it to -mm again,
> I won't add my Reviewed-by to bother.
>
> Hi David, Vitaly
>
> There are several things unclear to me.
>
> So, these improved interfaces are used to alleviate the burden of the
> existing udev rules, or try to replace it? As you know, we have been
At least in RHEL, my plan is to replace it / use a udev rules as a
fallback on older kernels (see the example scripts below). But other
distribution can handle it as they want.
> using udev rules to interact between kernel and user space on bare metal,
> and guests who want to hot add/remove.>
> And also the OOM issue in hyperV when onlining pages after adding memory
> block. I am not a virt devel expert, could this happen on bare metal
> system?
Don't think it's relevant on bare metal. If you plug a big DIMM, all
memory blocks will be added first in one shot and then all memory blocks
will be onlined. So it doesn't matter "how fast" you online that memory.
In contrast, Hyper-V (and virtio-mem) add one (or a limited number of)
memory block at a time and wait for them to get onlined.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v6 5/5] x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls
From: Vitaly Kuznetsov @ 2020-03-18 13:13 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200317034804.112538-6-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> There is another mode for the synthetic debugger which uses hypercalls
> to send/recv network data instead of the MSR interface.
>
> This interface is much slower and less recommended since you might get
> a lot of VMExits while KDVM polling for new packets to recv, rather
> than simply checking the pending page to see if there is data avialble
> and then request.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> arch/x86/kvm/hyperv.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 917b10a637fc..4a77ff61658b 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1821,6 +1821,28 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> }
> ret = kvm_hv_send_ipi(vcpu, ingpa, outgpa, true, false);
> break;
> + case HVCALL_POST_DEBUG_DATA:
> + case HVCALL_RETRIEVE_DEBUG_DATA:
> + if (unlikely(fast)) {
> + ret = HV_STATUS_INVALID_PARAMETER;
> + break;
> + }
> + /* fallthrough */
This is now being changed to 'fallthrough;'
> + case HVCALL_RESET_DEBUG_SESSION: {
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> + if (!(syndbg->options & HV_X64_SYNDBG_OPTION_USE_HCALLS)) {
> + ret = HV_STATUS_OPERATION_DENIED;
> + break;
> + }
> + vcpu->run->exit_reason = KVM_EXIT_HYPERV;
> + vcpu->run->hyperv.type = KVM_EXIT_HYPERV_HCALL;
> + vcpu->run->hyperv.u.hcall.input = param;
> + vcpu->run->hyperv.u.hcall.params[0] = ingpa;
> + vcpu->run->hyperv.u.hcall.params[1] = outgpa;
> + vcpu->arch.complete_userspace_io =
> + kvm_hv_hypercall_complete_userspace;
> + return 0;
> + }
> default:
> ret = HV_STATUS_INVALID_HYPERCALL_CODE;
> break;
And the hypercall should also check whether KVM_CAP_HYPERV_DEBUGGING is
enabled.
--
Vitaly
^ permalink raw reply
* Re: [PATCH v6 4/5] x86/kvm/hyper-v: enable hypercalls regardless of hypercall page
From: Vitaly Kuznetsov @ 2020-03-18 13:11 UTC (permalink / raw)
To: Jon Doron; +Cc: Jon Doron, kvm, linux-hyperv
In-Reply-To: <20200317034804.112538-5-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> Microsoft's kdvm.dll dbgtransport module does not respect the hypercall
> page and simply identifies the CPU being used (AMD/Intel) and according
> to it simply makes hypercalls with the relevant instruction
> (vmmcall/vmcall respectively).
>
> The relevant function in kdvm is KdHvConnectHypervisor which first checks
> if the hypercall page has been enabled via HV_X64_MSR_HYPERCALL_ENABLE,
> and in case it was not it simply sets the HV_X64_MSR_GUEST_OS_ID to
> 0x1000101010001 which means:
> build_number = 0x0001
> service_version = 0x01
> minor_version = 0x01
> major_version = 0x01
> os_id = 0x00 (Undefined)
> vendor_id = 1 (Microsoft)
> os_type = 0 (A value of 0 indicates a proprietary, closed source OS)
>
> and starts issuing the hypercall without setting the hypercall page.
>
> To resolve this issue simply enable hypercalls also if the guest_os_id
> is not 0.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> arch/x86/kvm/hyperv.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index b6a97abe2bc9..917b10a637fc 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1639,7 +1639,10 @@ static u64 kvm_hv_send_ipi(struct kvm_vcpu *current_vcpu, u64 ingpa, u64 outgpa,
>
> bool kvm_hv_hypercall_enabled(struct kvm *kvm)
> {
> - return READ_ONCE(kvm->arch.hyperv.hv_hypercall) & HV_X64_MSR_HYPERCALL_ENABLE;
> + struct kvm_hv *hv = &kvm->arch.hyperv;
> +
> + return READ_ONCE(hv->hv_hypercall) & HV_X64_MSR_HYPERCALL_ENABLE ||
> + READ_ONCE(hv->hv_guest_os_id) != 0;
> }
>
> static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
Also, as we're introducing KVM_CAP_HYPERV_DEBUGGING please include it in
the check: no need to enable hypercalls without a hypercall page if we
haven't enabled KVM_CAP_HYPERV_DEBUGGING (preserve the status quo).
--
Vitaly
^ permalink raw reply
* Re: [PATCH v6 3/5] x86/kvm/hyper-v: Add support for synthetic debugger capability
From: Vitaly Kuznetsov @ 2020-03-18 13:08 UTC (permalink / raw)
To: Jon Doron, kvm, linux-hyperv; +Cc: Jon Doron
In-Reply-To: <20200317034804.112538-4-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> Add support for Hyper-V synthetic debugger (syndbg) interface.
> The syndbg interface is using MSRs to emulate a way to send/recv packets
> data.
>
> The debug transport dll (kdvm/kdnet) will identify if Hyper-V is enabled
> and if it supports the synthetic debugger interface it will attempt to
> use it, instead of trying to initialize a network adapter.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> Documentation/virt/kvm/api.rst | 16 ++++
> arch/x86/include/asm/kvm_host.h | 13 +++
> arch/x86/kvm/hyperv.c | 135 +++++++++++++++++++++++++++++++-
> arch/x86/kvm/hyperv.h | 5 ++
> arch/x86/kvm/trace.h | 51 ++++++++++++
> arch/x86/kvm/x86.c | 9 +++
> include/uapi/linux/kvm.h | 11 +++
> 7 files changed, 239 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 4872c47bbcff..fe992dcf4f93 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5024,6 +5024,7 @@ EOI was received.
> struct kvm_hyperv_exit {
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> + #define KVM_EXIT_HYPERV_SYNDBG 3
> __u32 type;
> __u32 pad1;
> union {
> @@ -5039,6 +5040,15 @@ EOI was received.
> __u64 result;
> __u64 params[2];
> } hcall;
> + struct {
> + __u32 msr;
> + __u32 pad2;
> + __u64 control;
> + __u64 status;
> + __u64 send_page;
> + __u64 recv_page;
> + __u64 pending_page;
> + } syndbg;
> } u;
> };
> /* KVM_EXIT_HYPERV */
> @@ -5055,6 +5065,12 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
> event/message pages and to enable/disable SynIC messages/events processing
> in userspace.
>
> + - KVM_EXIT_HYPERV_SYNDBG -- synchronously notify user-space about
> +
> +Hyper-V Synthetic debugger state change. Notification is used to either update
> +the pending_page location or to send a control command (send the buffer located
> +in send_page or recv a buffer to recv_page).
> +
> ::
>
> /* KVM_EXIT_ARM_NISV */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 98959e8cd448..f8e58e8866bb 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -854,6 +854,18 @@ struct kvm_apic_map {
> struct kvm_lapic *phys_map[];
> };
>
> +/* Hyper-V synthetic debugger (SynDbg)*/
> +struct kvm_hv_syndbg {
> + struct {
> + u64 control;
> + u64 status;
> + u64 send_page;
> + u64 recv_page;
> + u64 pending_page;
> + } control;
> + u64 options;
> +};
> +
> /* Hyper-V emulation context */
> struct kvm_hv {
> struct mutex hv_lock;
> @@ -877,6 +889,7 @@ struct kvm_hv {
> atomic_t num_mismatched_vp_indexes;
>
> struct hv_partition_assist_pg *hv_pa_pg;
> + struct kvm_hv_syndbg hv_syndbg;
> };
>
> enum kvm_irqchip_mode {
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index a86fda7a1d03..b6a97abe2bc9 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -266,6 +266,107 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
> return ret;
> }
>
> +static int kvm_hv_syndbg_complete_userspace(struct kvm_vcpu *vcpu)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + struct kvm_hv *hv = &kvm->arch.hyperv;
> +
> + if (vcpu->run->hyperv.u.syndbg.msr == HV_X64_MSR_SYNDBG_CONTROL)
> + hv->hv_syndbg.control.status =
> + vcpu->run->hyperv.u.syndbg.status;
> + return 1;
> +}
> +
> +static void syndbg_exit(struct kvm_vcpu *vcpu, u32 msr)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> + struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
> +
> + hv_vcpu->exit.type = KVM_EXIT_HYPERV_SYNDBG;
> + hv_vcpu->exit.u.syndbg.msr = msr;
> + hv_vcpu->exit.u.syndbg.control = syndbg->control.control;
> + hv_vcpu->exit.u.syndbg.send_page = syndbg->control.send_page;
> + hv_vcpu->exit.u.syndbg.recv_page = syndbg->control.recv_page;
> + hv_vcpu->exit.u.syndbg.pending_page = syndbg->control.pending_page;
> + vcpu->arch.complete_userspace_io =
> + kvm_hv_syndbg_complete_userspace;
> +
> + kvm_make_request(KVM_REQ_HV_EXIT, vcpu);
> +}
> +
> +static int syndbg_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> + int ret;
Minor nitpick: you could've initialized 'ret' to '0' here. But actually,
looking at the code, you don't really need the return value as 'default'
case can just BUG(): this can never happen. The whole function can be
'void'.
> +
> + trace_kvm_hv_syndbg_set_msr(vcpu->vcpu_id,
> + vcpu_to_hv_vcpu(vcpu)->vp_index, msr, data);
> + ret = 0;
> + switch (msr) {
> + case HV_X64_MSR_SYNDBG_CONTROL:
> + syndbg->control.control = data;
> + syndbg_exit(vcpu, msr);
> + break;
> + case HV_X64_MSR_SYNDBG_STATUS:
> + syndbg->control.status = data;
> + break;
> + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> + syndbg->control.send_page = data;
> + break;
> + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> + syndbg->control.recv_page = data;
> + break;
> + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + syndbg->control.pending_page = data;
> + syndbg_exit(vcpu, msr);
> + break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + syndbg->options = data;
> + break;
> + default:
> + ret = 1;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int syndbg_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> +{
> + struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
> + int ret;
> +
> + ret = 0;
Same comment as the above.
> + switch (msr) {
> + case HV_X64_MSR_SYNDBG_CONTROL:
> + *pdata = syndbg->control.control;
> + break;
> + case HV_X64_MSR_SYNDBG_STATUS:
> + *pdata = syndbg->control.status;
> + break;
> + case HV_X64_MSR_SYNDBG_SEND_BUFFER:
> + *pdata = syndbg->control.send_page;
> + break;
> + case HV_X64_MSR_SYNDBG_RECV_BUFFER:
> + *pdata = syndbg->control.recv_page;
> + break;
> + case HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + *pdata = syndbg->control.pending_page;
> + break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + *pdata = syndbg->options;
> + break;
> + default:
> + ret = 1;
> + break;
> + }
> +
> + trace_kvm_hv_syndbg_get_msr(vcpu->vcpu_id,
> + vcpu_to_hv_vcpu(vcpu)->vp_index, msr,
> + *pdata);
> + return ret;
> +}
> +
> static int synic_get_msr(struct kvm_vcpu_hv_synic *synic, u32 msr, u64 *pdata,
> bool host)
> {
> @@ -800,6 +901,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr)
> case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
> case HV_X64_MSR_TSC_EMULATION_CONTROL:
> case HV_X64_MSR_TSC_EMULATION_STATUS:
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> r = true;
> break;
> }
> @@ -1061,6 +1164,9 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
> if (!host)
> return 1;
> break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + return syndbg_set_msr(vcpu, msr, data);
> default:
> vcpu_unimpl(vcpu, "Hyper-V unhandled wrmsr: 0x%x data 0x%llx\n",
> msr, data);
> @@ -1227,6 +1333,9 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> case HV_X64_MSR_TSC_EMULATION_STATUS:
> data = hv->hv_tsc_emulation_status;
> break;
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + return syndbg_get_msr(vcpu, msr, pdata);
> default:
> vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
> return 1;
> @@ -1797,6 +1906,9 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> { .function = HYPERV_CPUID_ENLIGHTMENT_INFO },
> { .function = HYPERV_CPUID_IMPLEMENT_LIMITS },
> { .function = HYPERV_CPUID_NESTED_FEATURES },
> + { .function = HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS },
> + { .function = HYPERV_CPUID_SYNDBG_INTERFACE },
> + { .function = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES },
> };
> int i, nent = ARRAY_SIZE(cpuid_entries);
>
> @@ -1821,7 +1933,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> case HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS:
> memcpy(signature, "Linux KVM Hv", 12);
>
> - ent->eax = HYPERV_CPUID_NESTED_FEATURES;
> + ent->eax = HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES;
This should depend on KVM_CAP_HYPERV_DEBUGGING being enabled (see below).
> ent->ebx = signature[0];
> ent->ecx = signature[1];
> ent->edx = signature[2];
> @@ -1856,9 +1968,12 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>
> ent->ebx |= HV_X64_POST_MESSAGES;
> ent->ebx |= HV_X64_SIGNAL_EVENTS;
> + ent->ebx |= HV_X64_DEBUGGING;
>
> ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE;
> ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
> + ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE;
> + ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE;
>
> /*
> * Direct Synthetic timers only make sense with in-kernel
> @@ -1903,6 +2018,24 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>
> break;
>
> + case HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS:
> + memcpy(signature, "Linux KVM Hv", 12);
> +
> + ent->eax = 0;
> + ent->ebx = signature[0];
> + ent->ecx = signature[1];
> + ent->edx = signature[2];
> + break;
> +
> + case HYPERV_CPUID_SYNDBG_INTERFACE:
> + memcpy(signature, "VS#1\0\0\0\0\0\0\0\0", 12);
> + ent->eax = signature[0];
> + break;
> +
> + case HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES:
> + ent->eax |= HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING;
> + break;
> +
> default:
> break;
> }
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index 5e4780bf6dd7..29729258fa49 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -73,6 +73,11 @@ static inline struct kvm_vcpu *synic_to_vcpu(struct kvm_vcpu_hv_synic *synic)
> return hv_vcpu_to_vcpu(container_of(synic, struct kvm_vcpu_hv, synic));
> }
>
> +static inline struct kvm_hv_syndbg *vcpu_to_hv_syndbg(struct kvm_vcpu *vcpu)
> +{
> + return &vcpu->kvm->arch.hyperv.hv_syndbg;
> +}
> +
> int kvm_hv_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host);
> int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host);
>
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index f194dd058470..bf6c3852868d 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -1515,6 +1515,57 @@ TRACE_EVENT(kvm_nested_vmenter_failed,
> __print_symbolic(__entry->err, VMX_VMENTER_INSTRUCTION_ERRORS))
> );
>
> +/*
> + * Tracepoint for syndbg_set_msr.
> + */
> +TRACE_EVENT(kvm_hv_syndbg_set_msr,
> + TP_PROTO(int vcpu_id, u32 vp_index, u32 msr, u64 data),
> + TP_ARGS(vcpu_id, vp_index, msr, data),
> +
> + TP_STRUCT__entry(
> + __field(int, vcpu_id)
> + __field(u32, vp_index)
> + __field(u32, msr)
> + __field(u64, data)
> + ),
> +
> + TP_fast_assign(
> + __entry->vcpu_id = vcpu_id;
> + __entry->vp_index = vp_index;
> + __entry->msr = msr;
> + __entry->data = data;
> + ),
> +
> + TP_printk("vcpu_id %d vp_index %u msr 0x%x data 0x%llx",
> + __entry->vcpu_id, __entry->vp_index, __entry->msr,
> + __entry->data)
> +);
> +
> +/*
> + * Tracepoint for syndbg_get_msr.
> + */
> +TRACE_EVENT(kvm_hv_syndbg_get_msr,
> + TP_PROTO(int vcpu_id, u32 vp_index, u32 msr, u64 data),
> + TP_ARGS(vcpu_id, vp_index, msr, data),
> +
> + TP_STRUCT__entry(
> + __field(int, vcpu_id)
> + __field(u32, vp_index)
> + __field(u32, msr)
> + __field(u64, data)
> + ),
> +
> + TP_fast_assign(
> + __entry->vcpu_id = vcpu_id;
> + __entry->vp_index = vp_index;
> + __entry->msr = msr;
> + __entry->data = data;
> + ),
> +
> + TP_printk("vcpu_id %d vp_index %u msr 0x%x data 0x%llx",
> + __entry->vcpu_id, __entry->vp_index, __entry->msr,
> + __entry->data)
> +);
> #endif /* _TRACE_KVM_H */
>
> #undef TRACE_INCLUDE_PATH
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5de200663f51..619c24bac79e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1214,6 +1214,10 @@ static const u32 emulated_msrs_all[] = {
> HV_X64_MSR_VP_ASSIST_PAGE,
> HV_X64_MSR_REENLIGHTENMENT_CONTROL, HV_X64_MSR_TSC_EMULATION_CONTROL,
> HV_X64_MSR_TSC_EMULATION_STATUS,
> + HV_X64_MSR_SYNDBG_OPTIONS,
> + HV_X64_MSR_SYNDBG_CONTROL, HV_X64_MSR_SYNDBG_STATUS,
> + HV_X64_MSR_SYNDBG_SEND_BUFFER, HV_X64_MSR_SYNDBG_RECV_BUFFER,
> + HV_X64_MSR_SYNDBG_PENDING_BUFFER,
>
> MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
> MSR_KVM_PV_EOI_EN,
> @@ -2906,6 +2910,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> */
> break;
> case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
> case HV_X64_MSR_CRASH_CTL:
> case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
> @@ -3151,6 +3157,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> msr_info->data = 0x20000000;
> break;
> case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> + case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER:
> + case HV_X64_MSR_SYNDBG_OPTIONS:
> case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
> case HV_X64_MSR_CRASH_CTL:
> case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
> @@ -3323,6 +3331,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_HYPERV_TLBFLUSH:
> case KVM_CAP_HYPERV_SEND_IPI:
> case KVM_CAP_HYPERV_CPUID:
> + case KVM_CAP_HYPERV_DEBUGGING:
This is not enough: the userspace actually need to enable
KVM_CAP_HYPERV_DEBUGGING to start receiving
KVM_EXIT_HYPERV_SYNDBG. (see kvm_vcpu_ioctl_enable_cap())
Whem the capability is disabled, we should likely forbid writing to
these debug MSRs.
The problem here is: what if an unprepared userspace receives
KVM_EXIT_HYPERV_SYNDBG? It will likely crash or misbehave (and this is
guest triggerable!) so we need to prevent this.
> case KVM_CAP_PCI_SEGMENT:
> case KVM_CAP_DEBUGREGS:
> case KVM_CAP_X86_ROBUST_SINGLESTEP:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7ee0ddc4c457..f20b81894f23 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -188,6 +188,7 @@ struct kvm_s390_cmma_log {
> struct kvm_hyperv_exit {
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> +#define KVM_EXIT_HYPERV_SYNDBG 3
> __u32 type;
> __u32 pad1;
> union {
> @@ -203,6 +204,15 @@ struct kvm_hyperv_exit {
> __u64 result;
> __u64 params[2];
> } hcall;
> + struct {
> + __u32 msr;
> + __u32 pad2;
> + __u64 control;
> + __u64 status;
> + __u64 send_page;
> + __u64 recv_page;
> + __u64 pending_page;
> + } syndbg;
> } u;
> };
>
> @@ -1012,6 +1022,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_NISV_TO_USER 177
> #define KVM_CAP_ARM_INJECT_EXT_DABT 178
> #define KVM_CAP_S390_VCPU_RESETS 179
> +#define KVM_CAP_HYPERV_DEBUGGING 180
I'd name this KVM_CAP_HYPERV_SYNDBG to not cause confusion.
>
> #ifdef KVM_CAP_IRQ_ROUTING
--
Vitaly
^ permalink raw reply
* Re: [PATCH v2 0/8] mm/memory_hotplug: allow to specify a default online_type
From: Baoquan He @ 2020-03-18 13:05 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv,
Vitaly Kuznetsov, Yumei Huang, Igor Mammedov, Eduardo Habkost,
Milan Zamazal, Andrew Morton, Benjamin Herrenschmidt,
Greg Kroah-Hartman, Haiyang Zhang, K. Y. Srinivasan,
Michael Ellerman, Michal Hocko, Michal Hocko, Oscar Salvador,
Paul Mackerras, Rafael J. Wysocki, Stephen Hemminger, Wei Liu,
Wei Yang
In-Reply-To: <20200317104942.11178-1-david@redhat.com>
On 03/17/20 at 11:49am, David Hildenbrand wrote:
> Distributions nowadays use udev rules ([1] [2]) to specify if and
> how to online hotplugged memory. The rules seem to get more complex with
> many special cases. Due to the various special cases,
> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
> is handled via udev rules.
>
> Everytime we hotplug memory, the udev rule will come to the same
> conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
> memory in separate memory blocks and wait for memory to get onlined by user
> space before continuing to add more memory blocks (to not add memory faster
> than it is getting onlined). This of course slows down the whole memory
> hotplug process.
>
> To make the job of distributions easier and to avoid udev rules that get
> more and more complicated, let's extend the mechanism provided by
> - /sys/devices/system/memory/auto_online_blocks
> - "memhp_default_state=" on the kernel cmdline
> to be able to specify also "online_movable" as well as "online_kernel"
This patch series looks good, thanks. Since Andrew has merged it to -mm again,
I won't add my Reviewed-by to bother.
Hi David, Vitaly
There are several things unclear to me.
So, these improved interfaces are used to alleviate the burden of the
existing udev rules, or try to replace it? As you know, we have been
using udev rules to interact between kernel and user space on bare metal,
and guests who want to hot add/remove.
And also the OOM issue in hyperV when onlining pages after adding memory
block. I am not a virt devel expert, could this happen on bare metal
system?
Thanks
Baoquan
^ permalink raw reply
* Re: [PATCH v6 1/5] x86/kvm/hyper-v: Explicitly align hcall param for kvm_hyperv_exit
From: Vitaly Kuznetsov @ 2020-03-18 12:56 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv
In-Reply-To: <20200317034804.112538-2-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> Signed-off-by: Jon Doron <arilou@gmail.com>
> ---
> Documentation/virt/kvm/api.rst | 2 ++
> include/uapi/linux/kvm.h | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index ebd383fba939..4872c47bbcff 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5025,9 +5025,11 @@ EOI was received.
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> __u32 type;
> + __u32 pad1;
> union {
> struct {
> __u32 msr;
> + __u32 pad2;
> __u64 control;
> __u64 evt_page;
> __u64 msg_page;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..7ee0ddc4c457 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -189,9 +189,11 @@ struct kvm_hyperv_exit {
> #define KVM_EXIT_HYPERV_SYNIC 1
> #define KVM_EXIT_HYPERV_HCALL 2
> __u32 type;
> + __u32 pad1;
> union {
> struct {
> __u32 msr;
> + __u32 pad2;
> __u64 control;
> __u64 evt_page;
> __u64 msg_page;
This looks good to me but probably not to an unprepared reader some time
later. What's going on here is:
The problem the patch is trying to address is the fact that 'struct
kvm_hyperv_exit' has different layout on when compiling in 32 and 64 bit
modes. In 64-bit mode the default alignment boundary is 64 bits thus
forcing extra gaps after 'type' and 'msr' but in 32-bit mode the
boundary is at 32 bits thus no extra gaps. This is an issue as even when
the kernel is 64 bit, the userspace using the interface can be both 32
and 64 bit but the same 32 bit userspace has to work with 32 bit kernel.
The issue is fixed by forcing the 64 bit layout, this leads to ABI
change for 32 bit builds and while we are obviously breaking '32 bit
userspace with 32 bit kernel' case, we're fixing the '32 bit userspace
with 64 bit kernel' one. As the interface has no (known) users and 32
bit KVM is rather baroque nowadays, this seems like a reasonable
decision.
I think something like the paragraph above should be the commit
message. With this fixed,
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply
* Re: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
From: Arnd Bergmann @ 2020-03-18 10:10 UTC (permalink / raw)
To: Michael Kelley
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <MW2PR2101MB10524879CD685710A51AB740D7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, Mar 18, 2020 at 1:12 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > > +
> > > +/* Define input and output layout for Get VP Register hypercall */
> > > +struct hv_get_vp_register_input {
> > > + u64 partitionid;
> > > + u32 vpindex;
> > > + u8 inputvtl;
> > > + u8 padding[3];
> > > + u32 name0;
> > > + u32 name1;
> > > +} __packed;
> >
> > Are you sure these need to be made byte-aligned according to the
> > specification? If the structure itself is aligned to 64 bit, better mark only
> > the individual fields that are misaligned as __packed.
> >
> > If the structure is aligned to only 32-bit addresses instead of
> > 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> > generate better code for accessing it.
>
> None of the fields are misaligned and it will always be aligned to 64-bit
> addresses, so there should be no padding needed or added. There was
> a discussion of __packed and the Hyper-V data structures in general on
> LKML here: https://lkml.org/lkml/2018/11/30/848. Adding __packed was
> done as a preventative measure, not because anything was actually
> broken. Marking as __aligned(8) here would indicate the correct intent,
> though the use of the structure always ensures 64-bit alignment.
Just drop the __packed annotations then, they just confuse the compiler
in this case. In particular, when the compiler thinks that a structure is
misaligned, it tries to avoid using load/store instructions on it that are
inefficient or trap with misaligned code, so having default alignment
produces better object code.
> > Also, in order to write portable code, it would be helpful to mark
> > all the fields as explicitly little-endian, and use __le32_to_cpu()
> > etc for accessing them.
>
> There's an opening comment in this file stating that all data
> structures shared between Hyper-V and a guest VM are little
> endian. Is there some other marking to consider using?
Yes, device drivers should generally define data structures using
the __le32, __le64 etc types, and use the conversion functions
to access them. Building with 'make C=1' usually tells you when
you have mismatched annotations.
> We have definitely not allowed for the case of Hyper-V running on
> a big endian architecture. There are a *lot* of messages and data
> structures passed between the guest and Hyper-V, and coding
> to handle either endianness is a big project. I'm doubtful
> of the value until and unless we actually have a need for it.
In general, the use of big-endian software on Linux is declining, however
- arm64 as an architecture is meant to support both endian types,
and we still try to ensure it works either way as long as there are
users that depend on it.
- The remaining users of big-endian software are probably
more likely to run on virtual machines than on real hardware
- Any device driver should generally be written against portable
interfaces, even if you think you know how it will be used. As
driver writers tend to look at existing code for new drivers, it's
better to have them all be portable. (This is a similar argument
to the irqchip interface).
Even if you don't convert any of the existing architecture independent
code to run both ways, I see no reason to not do it for new drivers.
> > > +/* Define synthetic interrupt controller message flags. */
> > > +union hv_message_flags {
> > > + __u8 asu8;
> > > + struct {
> > > + __u8 msg_pending:1;
> > > + __u8 reserved:7;
> > > + } __packed;
> > > +};
> >
> > For similar reasons, please avoid bit fields and just use a
> > bit mask on the first member of the union.
>
> Unfortunately, changing to a bit mask ripples into
> architecture independent code and into the x86
> implementation. I'd prefer not to drag that complexity
> into this patch set.
How so? If this file is arm64 specific, there should be no need to make
x86 do the same change.
> > > + * Use the Hyper-V provided stimer0 as the timer that is made
> > > + * available to the architecture independent Hyper-V drivers.
> > > + */
> > > +#define hv_init_timer(timer, tick) \
> > > + hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
> > > +#define hv_init_timer_config(timer, val) \
> > > + hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
> > > +#define hv_get_current_tick(tick) \
> > > + (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> >
> > In general, we prefer inline functions over macros in header files.
>
> I can change the "set" calls to inline functions. As you can see, the "get"
> functions are coded and used in architecture independent code and on
> the x86 side in a way that won't convert to inline functions.
Ok.
Arnd
^ permalink raw reply
* Re: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Arnd Bergmann @ 2020-03-18 9:57 UTC (permalink / raw)
To: Michael Kelley
Cc: Marc Zyngier, gregkh, Will Deacon, Ard Biesheuvel,
Catalin Marinas, Mark Rutland, Linux ARM,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
Sunil Muthuswamy, Boqun Feng
In-Reply-To: <MW2PR2101MB1052D91D3A9CEEBD7E2EA82FD7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
> >
> > On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > > wrote:
> > > >> /*
> > > >> + * Functions for allocating and freeing memory with size and
> > > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > > >> + * the guest page size may not be the same as the Hyper-V page
> > > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > > >> + * allocations to the allocation size boundary, so that the
> > > >> + * allocated memory appears to Hyper-V as a page of the size
> > > >> + * it expects.
> > > >> + *
> > > >> + * These functions are used by arm64 specific code as well as
> > > >> + * arch independent Hyper-V drivers.
> > > >> + */
> > > >> +
> > > >> +void *hv_alloc_hyperv_page(void)
> > > >> +{
> > > >> + BUILD_BUG_ON(PAGE_SIZE < HV_HYP_PAGE_SIZE);
> > > >> + return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > > >> +}
> > > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > > >
> > > > I don't think there is any guarantee that kmalloc() returns
> > > > page-aligned
> > > > allocations in general.
> > >
> > > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > > guarantee
> > > natural alignment for kmalloc(power-of-two)").
> > >
> > > > How about using get_free_pages() to implement this?
> > >
> > > This would certainly work, at the expense of a lot of wasted memory when
> > > PAGE_SIZE isn't 4k.
> >
> > I'm sure this is the least of your problems when the guest runs with
> > a large base page size, you've already wasted most of your memory
> > otherwise then.
> >
>
> I think there's value in keeping these functions. There are 8 uses in
> architecture independent code at the moment, which admittedly saves
> only ~1/2 Mbyte of memory with a 64K page size, but we will have
> additional uses with more memory savings as we get all of the
> Hyper-V synthetic drivers to work with 64K page size. Furthermore,
> there's coming work that will require additional steps to share a page
> between a guest and the Hyper-V host. These functions are the right
> place to put the code for the additional sharing steps. Removing them
> now in favor of a bare kmalloc() and then adding them back doesn't
> seem worthwhile.
My point was to keep the functions but use alloc_pages() internally,
so you can deal with the hypervisor having a larger page size than
the guest, which seems to be a more important scenario if I correctly
understand the differences between the way Windows and Linux
deal with page cache.
As far as I understand, using 64kb pages on Windows is generally
a win as the VFS code already deals with units of that size, while
on Linux the 4kb page size is too deeply entrenched within the
file system code to mess with it: Whatever you gain in terms of
TLB pressure on workloads that cannot use huge pages is all lost
again through extra I/O and wasted physical memory.
Arnd
^ permalink raw reply
* Re: [PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer
From: Arnd Bergmann @ 2020-03-18 9:52 UTC (permalink / raw)
To: Michael Kelley
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <MW2PR2101MB1052B9C24DAB19FBBD818347D7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, Mar 18, 2020 at 1:16 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > Add ARM64-specific code to set up and handle the interrupts
> > > generated by Hyper-V for VMbus messages and for stimer expiration.
> > >
> > > This code is architecture dependent and is mostly driven by
> > > architecture independent code in the VMbus driver and the
> > > Hyper-V timer clocksource driver.
> > >
> > > This code is built only when CONFIG_HYPERV is enabled.
> > >
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >
> > This looks like it should be a nested irqchip driver instead, so your
> > device drivers can use the normal request_irq() functions etc.
> >
> > Is anything preventing you from doing that? If so, please describe
> > that in the changelog and in a comment in the driver.
> >
>
> As mentioned in my reply on Patch 1, Hyper-V offers a limited synthetic
> interrupt controller managed by Linux code that's been around the last
> 10 years on the x86 side. For reasons that pre-date me, it was not written
> as an irqchip driver.
I think the reason is just that 10 years ago, we did not have the concept
of irqchips as device drivers.
> Modulo the small routines you see in this patch, the code is architecture
> independent, and it seems we ought to keep the high degree of commonality.
> Re-architecting the arch independent code to model as an irqchip driver seems
> to carry some risk to the x86 side that has a lot of real-world usage today, but
> I'll take a look and see what the risks look like and if it adds any clarity.
How many drivers link against the custom interface? If it's less than 10,
making it a real driver is probably not too hard to do.
Arnd
^ permalink raw reply
* Re: [PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot
From: Arnd Bergmann @ 2020-03-18 9:44 UTC (permalink / raw)
To: Michael Kelley
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng, John Stultz, Thomas Gleixner, Stephen Boyd
In-Reply-To: <MW2PR2101MB1052281E5B197F2AA8E4D622D7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:30 AM
> >
> > As you are effectively adding a new clocksource driver here, please move the
> > code to drivers/clocksource and send the patch to the respective maintainers
> > (added to Cc here), splitting it out from the rest of the patch.
> >
> > You should also describe why your platform doesn't just use the normal
> > architected timer interface.
> >
> > > +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_init);
> >
> > This looks like it registers a driver for the same device as the normal
> > arch timer. Won't that clash?
>
> There is a Hyper-V clocksource driver in drivers/clocksource/hyperv_timer.c.
> It is architecture independent and works for both x86 and ARM64.
>
> The requirement here is really for a place to hang the general Hyper-V
> initialization code. On the x86 side, there's infrastructure already in place
> to do hypervisor initialization, but nothing corresponding on the ARM64 side.
> The TIMER_ACPI_DECLARE hook is admittedly a temporary approach, and I'm
> happy to hear if someone has a better way to handle this.
>
> FWIW, Hyper-V doesn't currently virtualize the ARM arch counter/timer for
> guest VMs. The Hyper-V synthetic counter/timer in the Hyper-V clocksource
> driver is used on both ARM64 and x86. But this Hyper-V init code doesn't actually
> touch the GTDT device, so it won't interfere with the ARM arch counter/timer
> when a future Hyper-V version does virtualize it.
I don't have a good idea to solve it, just a few more thoughts:
- if your platform does not actually provide the generic timer, then the
ACPI tables should not list one either. Instead, create a separate
description for your custom timer, and have that added to the ACPI
spec.
- To treat the timer more like a normal driver, better have the
TIMER_ACPI_DECLARE() function live only in the driver itself,
and use an early initcall (arch_initcall, subsys_initcall, etc)
it initialize the rest as late as you can.
- Some of the other code added to arch/arm64/ might be able to
live in drivers/virt/hyperv in order to be shared between x86 and
arm64. (No idea how much of it there is).
Arnd
^ permalink raw reply
* Re: [PATCH v6 09/10] arm64: efi: Export screen_info
From: Arnd Bergmann @ 2020-03-18 9:26 UTC (permalink / raw)
To: Michael Kelley
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <MW2PR2101MB10524254D2FE3EFC72329465D7F70@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > The Hyper-V frame buffer driver may be built as a module, and
> > > it needs access to screen_info. So export screen_info.
> > >
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >
> > Is there any chance of using a more modern KMS based driver for the screen
> > than the old fbdev subsystem? I had hoped to one day completely remove
> > support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> > architectures.
> >
>
> The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> frame buffer device. That driver builds and runs on both ARM64 and x86.
>
> I'm not knowledgeable about video/graphics drivers, but when you
> say "a more modern KMS based driver", are you meaning one based on
> DRM & KMS? Does DRM make sense for a "dumb" frame buffer device?
> Are there any drivers that would be a good pattern to look at?
It used to be a lot harder to write a DRM driver compared to an fbdev
driver, but this has changed to the opposite over the years.
A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
or anything in drivers/gpu/drm/tiny/, but you may want to look at the
other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
Arnd
^ permalink raw reply
* RE: [PATCH v6 09/10] arm64: efi: Export screen_info
From: Michael Kelley @ 2020-03-18 0:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <CAK8P3a1YUjhaVUmjVC2pCoTTBTU408iN44Q=QZ0RDz8rmzJisQ@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de>
>
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > The Hyper-V frame buffer driver may be built as a module, and
> > it needs access to screen_info. So export screen_info.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>
> Is there any chance of using a more modern KMS based driver for the screen
> than the old fbdev subsystem? I had hoped to one day completely remove
> support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> architectures.
>
The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
frame buffer device. That driver builds and runs on both ARM64 and x86.
I'm not knowledgeable about video/graphics drivers, but when you
say "a more modern KMS based driver", are you meaning one based on
DRM & KMS? Does DRM make sense for a "dumb" frame buffer device?
Are there any drivers that would be a good pattern to look at?
Michael
^ permalink raw reply
* RE: [PATCH v6 07/10] arm64: hyperv: Initialize hypervisor on boot
From: Michael Kelley @ 2020-03-18 0:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng, John Stultz, Thomas Gleixner, Stephen Boyd
In-Reply-To: <CAK8P3a0+uBsurfQ4smjPDGkJQSkMe-TxJ4cWR_EZXgDR4-bAWQ@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:30 AM
>
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > Add ARM64-specific code to initialize the Hyper-V
> > hypervisor when booting as a guest VM. Provide functions
> > and data structures indicating hypervisor status that
> > are needed by VMbus driver.
> >
> > This code is built only when CONFIG_HYPERV is enabled.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> > arch/arm64/hyperv/hv_core.c | 156
> ++++++++++++++++++++++++++++++++++++++++++++
>
> As you are effectively adding a new clocksource driver here, please move the
> code to drivers/clocksource and send the patch to the respective maintainers
> (added to Cc here), splitting it out from the rest of the patch.
>
> You should also describe why your platform doesn't just use the normal
> architected timer interface.
>
> > +TIMER_ACPI_DECLARE(hyperv, ACPI_SIG_GTDT, hyperv_init);
>
> This looks like it registers a driver for the same device as the normal
> arch timer. Won't that clash?
>
> Arnd
There is a Hyper-V clocksource driver in drivers/clocksource/hyperv_timer.c.
It is architecture independent and works for both x86 and ARM64.
The requirement here is really for a place to hang the general Hyper-V
initialization code. On the x86 side, there's infrastructure already in place
to do hypervisor initialization, but nothing corresponding on the ARM64 side.
The TIMER_ACPI_DECLARE hook is admittedly a temporary approach, and I'm
happy to hear if someone has a better way to handle this.
FWIW, Hyper-V doesn't currently virtualize the ARM arch counter/timer for
guest VMs. The Hyper-V synthetic counter/timer in the Hyper-V clocksource
driver is used on both ARM64 and x86. But this Hyper-V init code doesn't actually
touch the GTDT device, so it won't interfere with the ARM arch counter/timer
when a future Hyper-V version does virtualize it.
Michael
^ permalink raw reply
* RE: [PATCH v6 06/10] arm64: hyperv: Add kexec and panic handlers
From: Michael Kelley @ 2020-03-18 0:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: will@kernel.org, ardb@kernel.org, arnd@arndb.de,
catalin.marinas@arm.com, mark.rutland@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi@vger.kernel.org, linux-arch@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
boqun.feng@gmail.com
In-Reply-To: <86tv2pzdpr.wl-maz@kernel.org>
From: Marc Zyngier <maz@kernel.org> Sent: Sunday, March 15, 2020 11:16 AM
>
> On Sat, 14 Mar 2020 15:35:15 +0000,
> Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > Add functions to set up and remove kexec and panic
> > handlers, and to inform Hyper-V about a guest panic.
> > These functions are called from architecture independent
> > code in the VMbus driver.
> >
> > This code is built only when CONFIG_HYPERV is enabled.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> > arch/arm64/hyperv/hv_core.c | 61
> ++++++++++++++++++++++++++++++++++++++++++++
> > arch/arm64/hyperv/mshyperv.c | 26 +++++++++++++++++++
> > 2 files changed, 87 insertions(+)
> >
> > diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
> > index 4aa6b8f..8d6de9f 100644
> > --- a/arch/arm64/hyperv/hv_core.c
> > +++ b/arch/arm64/hyperv/hv_core.c
> > @@ -199,3 +199,64 @@ void hv_get_vpreg_128(u32 msr, struct
> hv_get_vp_register_output *res)
> > kfree(output);
> > }
> > EXPORT_SYMBOL_GPL(hv_get_vpreg_128);
> > +
> > +void hyperv_report_panic(struct pt_regs *regs, long err)
> > +{
> > + static bool panic_reported;
> > + u64 guest_id;
> > +
> > + /*
> > + * We prefer to report panic on 'die' chain as we have proper
> > + * registers to report, but if we miss it (e.g. on BUG()) we need
> > + * to report it on 'panic'.
> > + */
> > + if (panic_reported)
> > + return;
> > + panic_reported = true;
>
> How does this work when multiple vcpus are crashing at once? Are you
> guaranteed to be single-threaded at this point?
I'll need to go research. If not, the above should be an atomic
test-and-set.
>
> > +
> > + guest_id = hv_get_vpreg(HV_REGISTER_GUEST_OSID);
> > +
> > + /*
> > + * Hyper-V provides the ability to store only 5 values.
> > + * Pick the passed in error value, the guest_id, and the PC.
> > + * The first two general registers are added arbitrarily.
> > + */
> > + hv_set_vpreg(HV_REGISTER_CRASH_P0, err);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P1, guest_id);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P2, regs->pc);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P3, regs->regs[0]);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P4, regs->regs[1]);
>
> How about reporting useful information, a pointer to some data
> structure describing the fault? As it is, the usefulness of this is
> pretty dubious.
Yes, it is dubious. The version with more data describing the fault is
hyperv_report_panic_msg() below, which is provided by newer versions
of Hyper-V, including all versions for ARM64. So the above function
should never get called on ARM64. While we could stub it out, I'd
like to keep the notification to Hyper-V just in case
hyperv_report_panic_msg() is not available for some reason I can't
currently anticipate. The important thing is the notification, and
the register values aren't really important. I'll add a comment to
that effect.
>
> > +
> > + /*
> > + * Let Hyper-V know there is crash data available
> > + */
> > + hv_set_vpreg(HV_REGISTER_CRASH_CTL, HV_CRASH_CTL_CRASH_NOTIFY);
> > +}
> > +EXPORT_SYMBOL_GPL(hyperv_report_panic);
> > +
> > +/*
> > + * hyperv_report_panic_msg - report panic message to Hyper-V
> > + * @pa: physical address of the panic page containing the message
> > + * @size: size of the message in the page
> > + */
> > +void hyperv_report_panic_msg(phys_addr_t pa, size_t size)
> > +{
> > + /*
> > + * P3 to contain the physical address of the panic page & P4 to
> > + * contain the size of the panic data in that page. Rest of the
> > + * registers are no-op when the NOTIFY_MSG flag is set.
> > + */
> > + hv_set_vpreg(HV_REGISTER_CRASH_P0, 0);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P1, 0);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P2, 0);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P3, pa);
> > + hv_set_vpreg(HV_REGISTER_CRASH_P4, size);
> > +
> > + /*
> > + * Let Hyper-V know there is crash data available along with
> > + * the panic message.
> > + */
> > + hv_set_vpreg(HV_REGISTER_CRASH_CTL,
> > + (HV_CRASH_CTL_CRASH_NOTIFY | HV_CRASH_CTL_CRASH_NOTIFY_MSG));
> > +}
> > +EXPORT_SYMBOL_GPL(hyperv_report_panic_msg);
> > diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> > index ae6ece6..c58940d 100644
> > --- a/arch/arm64/hyperv/mshyperv.c
> > +++ b/arch/arm64/hyperv/mshyperv.c
> > @@ -23,6 +23,8 @@
> >
> > static void (*vmbus_handler)(void);
> > static void (*hv_stimer0_handler)(void);
> > +static void (*hv_kexec_handler)(void);
> > +static void (*hv_crash_handler)(struct pt_regs *regs);
>
> Why is this in the arch-specific code? Yes, it lives in the x86 arch
> code too, but I don't see what prevents it from being moved to the
> vmbus_drv.c code.
OK -- I'll see about moving it to arch independent code.
>
> >
> > static int vmbus_irq;
> > static long __percpu *vmbus_evt;
> > @@ -137,3 +139,27 @@ void hv_remove_stimer0_irq(int irq)
> > }
> > }
> > EXPORT_SYMBOL_GPL(hv_remove_stimer0_irq);
> > +
> > +void hv_setup_kexec_handler(void (*handler)(void))
> > +{
> > + hv_kexec_handler = handler;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_setup_kexec_handler);
> > +
> > +void hv_remove_kexec_handler(void)
> > +{
> > + hv_kexec_handler = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_remove_kexec_handler);
> > +
> > +void hv_setup_crash_handler(void (*handler)(struct pt_regs *regs))
> > +{
> > + hv_crash_handler = handler;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_setup_crash_handler);
> > +
> > +void hv_remove_crash_handler(void)
> > +{
> > + hv_crash_handler = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(hv_remove_crash_handler);
> > --
> > 1.8.3.1
> >
> >
>
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smells funny.
^ permalink raw reply
* RE: [PATCH v6 05/10] arm64: hyperv: Add interrupt handlers for VMbus and stimer
From: Michael Kelley @ 2020-03-18 0:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <CAK8P3a2yve3R1w5igBYMy3HSFJ8Xt4BHhXQcaTAkNCdZXZ1v-w@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de>
>
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > Add ARM64-specific code to set up and handle the interrupts
> > generated by Hyper-V for VMbus messages and for stimer expiration.
> >
> > This code is architecture dependent and is mostly driven by
> > architecture independent code in the VMbus driver and the
> > Hyper-V timer clocksource driver.
> >
> > This code is built only when CONFIG_HYPERV is enabled.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>
> This looks like it should be a nested irqchip driver instead, so your
> device drivers can use the normal request_irq() functions etc.
>
> Is anything preventing you from doing that? If so, please describe
> that in the changelog and in a comment in the driver.
>
As mentioned in my reply on Patch 1, Hyper-V offers a limited synthetic
interrupt controller managed by Linux code that's been around the last
10 years on the x86 side. For reasons that pre-date me, it was not written
as an irqchip driver.
Modulo the small routines you see in this patch, the code is architecture
independent, and it seems we ought to keep the high degree of commonality.
Re-architecting the arch independent code to model as an irqchip driver seems
to carry some risk to the x86 side that has a lot of real-world usage today, but
I'll take a look and see what the risks look like and if it adds any clarity.
^ permalink raw reply
* RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Michael Kelley @ 2020-03-18 0:15 UTC (permalink / raw)
To: Arnd Bergmann, Marc Zyngier, gregkh
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Linux ARM, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <CAK8P3a3aihZeriUWAhWJMsOtdiY4Lo29syrRbB4Po3v4dsLhvA@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
>
> On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > wrote:
> > >> /*
> > >> + * Functions for allocating and freeing memory with size and
> > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > >> + * the guest page size may not be the same as the Hyper-V page
> > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > >> + * allocations to the allocation size boundary, so that the
> > >> + * allocated memory appears to Hyper-V as a page of the size
> > >> + * it expects.
> > >> + *
> > >> + * These functions are used by arm64 specific code as well as
> > >> + * arch independent Hyper-V drivers.
> > >> + */
> > >> +
> > >> +void *hv_alloc_hyperv_page(void)
> > >> +{
> > >> + BUILD_BUG_ON(PAGE_SIZE < HV_HYP_PAGE_SIZE);
> > >> + return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > >
> > > I don't think there is any guarantee that kmalloc() returns
> > > page-aligned
> > > allocations in general.
> >
> > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > guarantee
> > natural alignment for kmalloc(power-of-two)").
> >
> > > How about using get_free_pages() to implement this?
> >
> > This would certainly work, at the expense of a lot of wasted memory when
> > PAGE_SIZE isn't 4k.
>
> I'm sure this is the least of your problems when the guest runs with
> a large base page size, you've already wasted most of your memory
> otherwise then.
>
I think there's value in keeping these functions. There are 8 uses in
architecture independent code at the moment, which admittedly saves
only ~1/2 Mbyte of memory with a 64K page size, but we will have
additional uses with more memory savings as we get all of the
Hyper-V synthetic drivers to work with 64K page size. Furthermore,
there's coming work that will require additional steps to share a page
between a guest and the Hyper-V host. These functions are the right
place to put the code for the additional sharing steps. Removing them
now in favor of a bare kmalloc() and then adding them back doesn't
seem worthwhile.
Michael
^ permalink raw reply
* RE: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
From: Michael Kelley @ 2020-03-18 0:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
Boqun Feng
In-Reply-To: <CAK8P3a1GFDUY4mXzst4Ds+S-4SGXso6-jfpsYyy-eHyceAC1Zg@mail.gmail.com>
From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
>
> On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
>
> > +
> > +/* Define input and output layout for Get VP Register hypercall */
> > +struct hv_get_vp_register_input {
> > + u64 partitionid;
> > + u32 vpindex;
> > + u8 inputvtl;
> > + u8 padding[3];
> > + u32 name0;
> > + u32 name1;
> > +} __packed;
>
> Are you sure these need to be made byte-aligned according to the
> specification? If the structure itself is aligned to 64 bit, better mark only
> the individual fields that are misaligned as __packed.
>
> If the structure is aligned to only 32-bit addresses instead of
> 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> generate better code for accessing it.
None of the fields are misaligned and it will always be aligned to 64-bit
addresses, so there should be no padding needed or added. There was
a discussion of __packed and the Hyper-V data structures in general on
LKML here: https://lkml.org/lkml/2018/11/30/848. Adding __packed was
done as a preventative measure, not because anything was actually
broken. Marking as __aligned(8) here would indicate the correct intent,
though the use of the structure always ensures 64-bit alignment.
>
> Also, in order to write portable code, it would be helpful to mark
> all the fields as explicitly little-endian, and use __le32_to_cpu()
> etc for accessing them.
There's an opening comment in this file stating that all data
structures shared between Hyper-V and a guest VM are little
endian. Is there some other marking to consider using?
We have definitely not allowed for the case of Hyper-V running on
a big endian architecture. There are a *lot* of messages and data
structures passed between the guest and Hyper-V, and coding
to handle either endianness is a big project. I'm doubtful
of the value until and unless we actually have a need for it.
>
> > +/* Define synthetic interrupt controller message flags. */
> > +union hv_message_flags {
> > + __u8 asu8;
> > + struct {
> > + __u8 msg_pending:1;
> > + __u8 reserved:7;
> > + } __packed;
> > +};
>
> For similar reasons, please avoid bit fields and just use a
> bit mask on the first member of the union.
Unfortunately, changing to a bit mask ripples into
architecture independent code and into the x86
implementation. I'd prefer not to drag that complexity
into this patch set.
>
> The __packed annotation here is not meaningful,
> as the total size is already only a single byte.
Agreed.
>
> > +/* Define port identifier type. */
> > +union hv_port_id {
> > + __u32 asu32;
> > + struct {
> > + __u32 id:24;
> > + __u32 reserved:8;
> > + } __packed u;
> > +};
>
> Here, the __packed annotation is inconsistent with the use
> in the rest of the file: marking only one member of the union
> as __packed means that the union itself is still expected to
> be 4 byte aligned. I would expect that either all of these
> structures have a sensible alignment, or they are all
> completely unaligned.
Agreed. Looks like it is wrong on the x86 side too.
>
> > + * Use the Hyper-V provided stimer0 as the timer that is made
> > + * available to the architecture independent Hyper-V drivers.
> > + */
> > +#define hv_init_timer(timer, tick) \
> > + hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
> > +#define hv_init_timer_config(timer, val) \
> > + hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
> > +#define hv_get_current_tick(tick) \
> > + (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
>
> In general, we prefer inline functions over macros in header files.
I can change the "set" calls to inline functions. As you can see, the "get"
functions are coded and used in architecture independent code and on
the x86 side in a way that won't convert to inline functions.
>
> > +#if IS_ENABLED(CONFIG_HYPERV)
> > +#define hv_enable_stimer0_percpu_irq(irq) enable_percpu_irq(irq, 0)
> > +#define hv_disable_stimer0_percpu_irq(irq) disable_percpu_irq(irq)
> > +#endif
>
> Should there be an #else definition here? It helps readability
> to have the two versions (with and without hyperv support) close
> together rather than in different files. If there is no other
> definition, just drop the #if.
Agreed. I'll figure out a way to handle this better.
>
> Arnd
^ permalink raw reply
* RE: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
From: Michael Kelley @ 2020-03-18 0:10 UTC (permalink / raw)
To: Marc Zyngier
Cc: will@kernel.org, ardb@kernel.org, arnd@arndb.de,
catalin.marinas@arm.com, mark.rutland@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-efi@vger.kernel.org, linux-arch@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
boqun.feng@gmail.com
In-Reply-To: <86v9n5zfrm.wl-maz@kernel.org>
From: Marc Zyngier <maz@kernel.org> Sent: Sunday, March 15, 2020 10:31 AM
>
> On Sat, 14 Mar 2020 15:35:10 +0000,
> Hi Michael,
>
> Michael Kelley <mikelley@microsoft.com> wrote:
> >
> > hyperv-tlfs.h defines Hyper-V interfaces from the Hyper-V Top Level
> > Functional Spec (TLFS). The TLFS is distinctly oriented to x86/x64,
> > and Hyper-V has not separated out the architecture-dependent parts into
> > x86/x64 vs. ARM64. So hyperv-tlfs.h includes information for ARM64
> > that is not yet formally published. The TLFS is available here:
> >
> > docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> >
> > mshyperv.h defines Linux-specific structures and routines for
> > interacting with Hyper-V on ARM64, and #includes the architecture-
> > independent part of mshyperv.h in include/asm-generic.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> > MAINTAINERS | 2 +
> > arch/arm64/include/asm/hyperv-tlfs.h | 413
> +++++++++++++++++++++++++++++++++++
> > arch/arm64/include/asm/mshyperv.h | 115 ++++++++++
> > 3 files changed, 530 insertions(+)
> > create mode 100644 arch/arm64/include/asm/hyperv-tlfs.h
> > create mode 100644 arch/arm64/include/asm/mshyperv.h
>
> So this is a pretty large patch, mostly containing constants and other
> data structures that don't necessarily make sense immediately (or at
> least, I can't make sense of it without reading all the other 9
> patches and going back to patch #1).
>
> Could you please consider splitting this into more discreet bits that
> get added as required by the supporting code?
Yes, I'll do this in the next version.
>
> So here's only a few sparse comments:
>
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 58bb5c4..398cfdb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7809,6 +7809,8 @@ F: arch/x86/include/asm/trace/hyperv.h
> > F: arch/x86/include/asm/hyperv-tlfs.h
> > F: arch/x86/kernel/cpu/mshyperv.c
> > F: arch/x86/hyperv
> > +F: arch/arm64/include/asm/hyperv-tlfs.h
> > +F: arch/arm64/include/asm/mshyperv.h
> > F: drivers/clocksource/hyperv_timer.c
> > F: drivers/hid/hid-hyperv.c
> > F: drivers/hv/
> > diff --git a/arch/arm64/include/asm/hyperv-tlfs.h b/arch/arm64/include/asm/hyperv-
> tlfs.h
> > new file mode 100644
> > index 0000000..5e6a087
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/hyperv-tlfs.h
> > @@ -0,0 +1,413 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * This file contains definitions from the Hyper-V Hypervisor Top-Level
> > + * Functional Specification (TLFS):
> > + * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> > + *
> > + * Copyright (C) 2019, Microsoft, Inc.
> > + *
> > + * Author : Michael Kelley <mikelley@microsoft.com>
> > + */
> > +
> > +#ifndef _ASM_HYPERV_TLFS_H
> > +#define _ASM_HYPERV_TLFS_H
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * All data structures defined in the TLFS that are shared between Hyper-V
> > + * and a guest VM use Little Endian byte ordering. This matches the default
> > + * byte ordering of Linux running on ARM64, so no special handling is required.
> > + */
> > +
> > +
> > +/*
> > + * While not explicitly listed in the TLFS, Hyper-V always runs with a page
> > + * size of 4096. These definitions are used when communicating with Hyper-V
> > + * using guest physical pages and guest physical page addresses, since the
> > + * guest page size may not be 4096 on ARM64.
> > + */
> > +#define HV_HYP_PAGE_SHIFT 12
> > +#define HV_HYP_PAGE_SIZE (1 << HV_HYP_PAGE_SHIFT)
>
> Probably worth writing this as 1UL to be on the safe side.
Agreed.
>
> > +#define HV_HYP_PAGE_MASK (~(HV_HYP_PAGE_SIZE - 1))
> > +
> > +/*
> > + * These Hyper-V registers provide information equivalent to the CPUID
> > + * instruction on x86/x64.
> > + */
> > +#define HV_REGISTER_HYPERVISOR_VERSION 0x00000100 /*CPUID
> 0x40000002 */
> > +#define HV_REGISTER_PRIVILEGES_AND_FEATURES 0x00000200 /*CPUID
> 0x40000003 */
> > +#define HV_REGISTER_FEATURES 0x00000201 /*CPUID
> 0x40000004 */
> > +#define HV_REGISTER_IMPLEMENTATION_LIMITS 0x00000202 /*CPUID
> 0x40000005 */
> > +#define HV_ARM64_REGISTER_INTERFACE_VERSION 0x00090006 /*CPUID
> 0x40000001 */
> > +
> > +/*
> > + * Feature identification. HvRegisterPrivilegesAndFeaturesInfo returns a
> > + * 128-bit value with flags indicating which features are available to the
> > + * partition based upon the current partition privileges. The 128-bit
> > + * value is broken up with different portions stored in different 32-bit
> > + * fields in the ms_hyperv structure.
> > + */
> > +
> > +/* Partition Reference Counter available*/
> > +#define HV_MSR_TIME_REF_COUNT_AVAILABLE BIT(1)
> > +
> > +/* Synthetic Timers available */
> > +#define HV_MSR_SYNTIMER_AVAILABLE BIT(3)
> > +
> > +/* Reference TSC available */
> > +#define HV_MSR_REFERENCE_TSC_AVAILABLE BIT(9)
> > +
> > +
> > +/*
> > + * This group of flags is in the high order 64-bits of the returned
> > + * 128-bit value. Note that this set of bit positions differs from what
> > + * is used on x86/x64 architecture.
> > + */
> > +
> > +/* Crash MSRs available */
> > +#define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE BIT(8)
>
> It is confusing that you don't have a single bit space for all these
> flags. It'd probably help if you had a structure describing this
> 128bit value in multiple 32bit or 64bit words, and indicating which
> field the bit position is relevant to.
I'll add this in the next version.
>
> [...]
>
> > +/* Define the hypercall status result */
> > +
> > +union hv_hypercall_status {
> > + u64 as_uint64;
>
> nit: it'd be more consistent if as_uint64 was actually a uint64 type.
Agreed.
>
> > + struct {
> > + u16 status;
> > + u16 reserved;
> > + u16 reps_completed; /* Low 12 bits */
> > + u16 reserved2;
> > + };
> > +};
> > +
> > +/* hypercall status code */
> > +#define HV_STATUS_SUCCESS 0
> > +#define HV_STATUS_INVALID_HYPERCALL_CODE 2
> > +#define HV_STATUS_INVALID_HYPERCALL_INPUT 3
> > +#define HV_STATUS_INVALID_ALIGNMENT 4
> > +#define HV_STATUS_INSUFFICIENT_MEMORY 11
> > +#define HV_STATUS_INVALID_CONNECTION_ID 18
> > +#define HV_STATUS_INSUFFICIENT_BUFFERS 19
> > +
> > +/* Define input and output layout for Get VP Register hypercall */
> > +struct hv_get_vp_register_input {
> > + u64 partitionid;
> > + u32 vpindex;
> > + u8 inputvtl;
> > + u8 padding[3];
> > + u32 name0;
> > + u32 name1;
> > +} __packed;
> > +
> > +struct hv_get_vp_register_output {
> > + u64 registervaluelow;
> > + u64 registervaluehigh;
> > +} __packed;
> > +
> > +#define HV_FLUSH_ALL_PROCESSORS BIT(0)
> > +#define HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES BIT(1)
> > +#define HV_FLUSH_NON_GLOBAL_MAPPINGS_ONLY BIT(2)
>
> I"m curious: Are these supposed to be PV'd TLB invalidation
> operations?
Yes, they are. Hyper-V provides PV TLB flush hypercalls on ARM64, but
this patch set doesn't use those hypercalls. I'll remove the definitions.
>
> > +#define HV_FLUSH_USE_EXTENDED_RANGE_FORMAT BIT(3)
> > +
> > +enum HV_GENERIC_SET_FORMAT {
> > + HV_GENERIC_SET_SPARSE_4K,
> > + HV_GENERIC_SET_ALL,
> > +};
> > +
> > +/*
> > + * The Hyper-V TimeRefCount register and the TSC
> > + * page provide a guest VM clock with 100ns tick rate
> > + */
> > +#define HV_CLOCK_HZ (NSEC_PER_SEC/100)
> > +
> > +/*
> > + * The fields in this structure are set by Hyper-V and read
> > + * by the Linux guest. They should be accessed with READ_ONCE()
> > + * so the compiler doesn't optimize in a way that will cause
> > + * problems. The union pads the size out to the page size
> > + * used to communicate with Hyper-V.
> > + */
> > +struct ms_hyperv_tsc_page {
> > + union {
> > + struct {
> > + u32 tsc_sequence;
> > + u32 reserved1;
> > + u64 tsc_scale;
> > + s64 tsc_offset;
> > + } __packed;
> > + u8 reserved2[HV_HYP_PAGE_SIZE];
> > + };
> > +};
> > +
> > +/* Define the number of synthetic interrupt sources. */
> > +#define HV_SYNIC_SINT_COUNT (16)
> > +/* Define the expected SynIC version. */
> > +#define HV_SYNIC_VERSION_1 (0x1)
> > +
> > +#define HV_SYNIC_CONTROL_ENABLE (1ULL << 0)
> > +#define HV_SYNIC_SIMP_ENABLE (1ULL << 0)
> > +#define HV_SYNIC_SIEFP_ENABLE (1ULL << 0)
> > +#define HV_SYNIC_SINT_MASKED (1ULL << 16)
> > +#define HV_SYNIC_SINT_AUTO_EOI (1ULL << 17)
> > +#define HV_SYNIC_SINT_VECTOR_MASK (0xFF)
>
> Let's me guess: a PV interrupt controller? Do you really need this?
> Specially as I don't see any PV irqchip driver in this submission...
>
No, the above definitions aren't needed. I'll remove them.
Hyper-V does provide a limited synthetic interrupt controller that's
implemented entirely in architecture independent code and has been
used on the x86 side since the beginning of Hyper-V support. It
pre-dates me by a lot of years, and I've never considered whether it
should be modeled as an irqchip.
> [...]
>
> > diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
> > new file mode 100644
> > index 0000000..60b3f68
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/mshyperv.h
> > @@ -0,0 +1,115 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * Linux-specific definitions for managing interactions with Microsoft's
> > + * Hyper-V hypervisor. The definitions in this file are specific to
> > + * the ARM64 architecture. See include/asm-generic/mshyperv.h for
> > + * definitions are that architecture independent.
> > + *
> > + * Definitions that are specified in the Hyper-V Top Level Functional
> > + * Spec (TLFS) should not go in this file, but should instead go in
> > + * hyperv-tlfs.h.
> > + *
> > + * Copyright (C) 2019, Microsoft, Inc.
> > + *
> > + * Author : Michael Kelley <mikelley@microsoft.com>
> > + */
> > +
> > +#ifndef _ASM_MSHYPERV_H
> > +#define _ASM_MSHYPERV_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdesc.h>
> > +#include <linux/arm-smccc.h>
> > +#include <asm/hyperv-tlfs.h>
> > +
> > +/*
> > + * Define the IRQ numbers/vectors used by Hyper-V VMbus interrupts
> > + * and by STIMER0 Direct Mode interrupts. Hyper-V should be supplying
> > + * these values through ACPI, but there are no other interrupting
> > + * devices in a Hyper-V VM on ARM64, so it's OK to hard code for now.
>
> I'm not convinced it is OK. If you don't try to do the right thing
> now, what is the incentive to do it later? Starting to hard code
> things is akin to going back to the ARM board files of old. Been
> there, managed to fix it, not going back to it again anytime soon.
I'll check back with the Hyper-V guys on getting appropriate values
assigned in ACPI.
>
> > + * The "CALLBACK_VECTOR" terminology is a left-over from the x86/x64
> > + * world that is used in architecture independent Hyper-V code.
> > + */
> > +#define HYPERVISOR_CALLBACK_VECTOR 16
> > +#define HV_STIMER0_IRQNR 17
> > +
> > +extern u64 hv_do_hvc(u64 control, ...);
> > +extern u64 hv_do_hvc_fast_get(u64 control, u64 input1, u64 input2, u64 input3,
> > + struct hv_get_vp_register_output *output);
> > +
> > +/*
> > + * Declare calls to get and set Hyper-V VP register values on ARM64, which
> > + * requires a hypercall.
> > + */
> > +extern void hv_set_vpreg(u32 reg, u64 value);
> > +extern u64 hv_get_vpreg(u32 reg);
> > +extern void hv_get_vpreg_128(u32 reg, struct hv_get_vp_register_output *result);
> > +
> > +/*
> > + * Use the Hyper-V provided stimer0 as the timer that is made
> > + * available to the architecture independent Hyper-V drivers.
> > + */
> > +#define hv_init_timer(timer, tick) \
> > + hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
> > +#define hv_init_timer_config(timer, val) \
> > + hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
> > +#define hv_get_current_tick(tick) \
> > + (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> > +
> > +#define hv_get_simp(val) (val = hv_get_vpreg(HV_REGISTER_SIPP))
> > +#define hv_set_simp(val) hv_set_vpreg(HV_REGISTER_SIPP, val)
> > +
> > +#define hv_get_siefp(val) (val = hv_get_vpreg(HV_REGISTER_SIFP))
> > +#define hv_set_siefp(val) hv_set_vpreg(HV_REGISTER_SIFP, val)
> > +
> > +#define hv_get_synic_state(val) (val = hv_get_vpreg(HV_REGISTER_SCONTROL))
> > +#define hv_set_synic_state(val) hv_set_vpreg(HV_REGISTER_SCONTROL, val)
> > +
> > +#define hv_get_vp_index(index) (index = hv_get_vpreg(HV_REGISTER_VPINDEX))
> > +
> > +#define hv_signal_eom() hv_set_vpreg(HV_REGISTER_EOM, 0)
> > +
> > +/*
> > + * Hyper-V SINT registers are numbered sequentially, so we can just
> > + * add the SINT number to the register number of SINT0
> > + */
> > +#define hv_get_synint_state(sint_num, val) \
> > + (val = hv_get_vpreg(HV_REGISTER_SINT0 + sint_num))
> > +#define hv_set_synint_state(sint_num, val) \
> > + hv_set_vpreg(HV_REGISTER_SINT0 + sint_num, val)
> > +
> > +#define hv_get_crash_ctl(val) \
> > + (val = hv_get_vpreg(HV_REGISTER_CRASH_CTL))
> > +#define hv_get_time_ref_count(val) \
> > + (val = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> > +#define hv_get_reference_tsc(val) \
> > + (val = hv_get_vpreg(HV_REGISTER_REFERENCE_TSC))
> > +#define hv_set_reference_tsc(val) \
> > + hv_set_vpreg(HV_REGISTER_REFERENCE_TSC, val)
> > +#define hv_enable_vdso_clocksource()
> > +#define hv_set_clocksource_vdso(val) \
> > + ((val).vdso_clock_mode = VDSO_CLOCKMODE_NONE)
> > +
> > +#if IS_ENABLED(CONFIG_HYPERV)
>
> I don't think this guards anything useful.
You are probably right. I'll double-check.
>
> > +#define hv_enable_stimer0_percpu_irq(irq) enable_percpu_irq(irq, 0)
> > +#define hv_disable_stimer0_percpu_irq(irq) disable_percpu_irq(irq)
>
> and this looks pretty premature.
I'm not sure I understand your comment about "premature". Could
you clarify?
>
> > +#endif
> > +
> > +/* ARM64 specific code to read the hardware clock */
> > +#define hv_get_raw_timer() arch_timer_read_counter()
> > +
> > +/* SMCCC hypercall parameters */
> > +#define HV_SMCCC_FUNC_NUMBER 1
> > +#define HV_FUNC_ID ARM_SMCCC_CALL_VAL( \
> > + ARM_SMCCC_STD_CALL, \
> > + ARM_SMCCC_SMC_64, \
> > + ARM_SMCCC_OWNER_VENDOR_HYP, \
>
> This is only defined in patch #2...
Indeed. :-( I'll fix as part of breaking up this patch into smaller
pieces.
Michael
>
> > + HV_SMCCC_FUNC_NUMBER)
> > +
> > +#include <asm-generic/mshyperv.h>
> > +
> > +#endif
>
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smells funny.
^ permalink raw reply
* Re: [PATCH v2 6/8] mm/memory_hotplug: unexport memhp_auto_online
From: Wei Yang @ 2020-03-17 22:24 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Andrew Morton,
Michal Hocko, Oscar Salvador, Rafael J. Wysocki, Baoquan He,
Wei Yang
In-Reply-To: <20200317104942.11178-7-david@redhat.com>
On Tue, Mar 17, 2020 at 11:49:40AM +0100, David Hildenbrand wrote:
>All in-tree users except the mm-core are gone. Let's drop the export.
>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> mm/memory_hotplug.c | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 1a00b5a37ef6..2d2aae830b92 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -71,7 +71,6 @@ bool memhp_auto_online;
> #else
> bool memhp_auto_online = true;
> #endif
>-EXPORT_SYMBOL_GPL(memhp_auto_online);
>
> static int __init setup_memhp_default_state(char *str)
> {
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH v2 4/8] powernv/memtrace: always online added memory blocks
From: Wei Yang @ 2020-03-17 22:04 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Andrew Morton, Greg Kroah-Hartman, Michal Hocko, Oscar Salvador,
Rafael J. Wysocki, Baoquan He, Wei Yang
In-Reply-To: <20200317104942.11178-5-david@redhat.com>
On Tue, Mar 17, 2020 at 11:49:38AM +0100, David Hildenbrand wrote:
>Let's always try to online the re-added memory blocks. In case add_memory()
>already onlined the added memory blocks, the first device_online() call
>will fail and stop processing the remaining memory blocks.
>
>This avoids manually having to check memhp_auto_online.
>
>Note: PPC always onlines all hotplugged memory directly from the kernel
>as well - something that is handled by user space on other
>architectures.
>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Cc: Paul Mackerras <paulus@samba.org>
>Cc: Michael Ellerman <mpe@ellerman.id.au>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Michal Hocko <mhocko@kernel.org>
>Cc: Oscar Salvador <osalvador@suse.de>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Wei Yang <richard.weiyang@gmail.com>
>Cc: linuxppc-dev@lists.ozlabs.org
>Signed-off-by: David Hildenbrand <david@redhat.com>
Looks good.
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>---
> arch/powerpc/platforms/powernv/memtrace.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
>index d6d64f8718e6..13b369d2cc45 100644
>--- a/arch/powerpc/platforms/powernv/memtrace.c
>+++ b/arch/powerpc/platforms/powernv/memtrace.c
>@@ -231,16 +231,10 @@ static int memtrace_online(void)
> continue;
> }
>
>- /*
>- * If kernel isn't compiled with the auto online option
>- * we need to online the memory ourselves.
>- */
>- if (!memhp_auto_online) {
>- lock_device_hotplug();
>- walk_memory_blocks(ent->start, ent->size, NULL,
>- online_mem_block);
>- unlock_device_hotplug();
>- }
>+ lock_device_hotplug();
>+ walk_memory_blocks(ent->start, ent->size, NULL,
>+ online_mem_block);
>+ unlock_device_hotplug();
>
> /*
> * Memory was added successfully so clean up references to it
>--
>2.24.1
--
Wei Yang
Help you, Help me
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox