All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	"Michael Kelley (LINUX)" <mikelley@microsoft.com>,
	Marc Zyngier <maz@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	will Deacon <will@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	Ard Biesheuvel <ardb@kernel.org>,
	broonie@kernel.org,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>
Subject: Re: Should arm64 have a custom crash shutdown handler?
Date: Thu, 05 May 2022 16:51:54 +0200	[thread overview]
Message-ID: <87y1zgyqut.fsf@redhat.com> (raw)
In-Reply-To: <YnPf3KPBXDNTpQoG@FVFF77S0Q05N.cambridge.arm.com>

Mark Rutland <mark.rutland@arm.com> writes:

> On Thu, May 05, 2022 at 03:52:24PM +0200, Vitaly Kuznetsov wrote:
>> "Guilherme G. Piccoli" <gpiccoli@igalia.com> writes:
>> 
>> > On 05/05/2022 09:53, Mark Rutland wrote:
>> >> [...]
>> >> Looking at those, the cleanup work is all arch-specific. What exactly would we
>> >> need to do on arm64, and why does it need to happen at that point specifically?
>> >> On arm64 we don't expect as much paravirtualization as on x86, so it's not
>> >> clear to me whether we need anything at all.
>> >> 
>> >>> Anyway, the idea here was to gather a feedback on how "receptive" arm64
>> >>> community would be to allow such customization, appreciated your feedback =)
>> >> 
>> >> ... and are you trying to do this for Hyper-V or just using that as an example?
>> >> 
>> >> I think we're not going to be very receptive without a more concrete example of
>> >> what you want.
>> >> 
>> >> What exactly do *you* need, and *why*? Is that for Hyper-V or another hypervisor?
>> >> 
>> >> Thanks
>> >> Mark.
>> >
>> > Hi Mark, my plan would be doing that for Hyper-V - kind of the same
>> > code, almost. For example, in hv_crash_handler() there is a stimer
>> > clean-up and the vmbus unload - my understanding is that this same code
>> > would need to run in arm64. Michael Kelley is CCed, he was discussing
>> > with me in the panic notifiers thread and may elaborate more on the needs.
>> >
>> > But also (not related with my specific plan), I've seen KVM quiesce code
>> > on x86 as well [see kvm_crash_shutdown() on arch/x86] , I'm not sure if
>> > this is necessary for arm64 or if this already executing in some
>> > abstracted form, I didn't dig deep - probably Vitaly is aware of that,
>> > hence I've CCed him here.
>> 
>> Speaking about the difference between reboot notifiers call chain and
>> machine_ops.crash_shutdown for KVM/x86, the main difference is that
>> reboot notifier is called on some CPU while the VM is fully functional,
>> this way we may e.g. still use IPIs (see kvm_pv_reboot_notify() doing
>> on_each_cpu()). When we're in a crash situation,
>> machine_ops.crash_shutdown is called on the CPU which crashed. We can't
>> count on IPIs still being functional so we do the very basic minimum so
>> *this* CPU can boot kdump kernel. There's no guarantee other CPUs can
>> still boot but normally we do kdump with 'nprocs=1'.
>
> Sure; IIUC the IPI problem doesn't apply to arm64, though, since that doesn't
> use a PV mechanism (and practically speaking will either be GICv2 or GICv3).
>

This isn't really about PV: when the kernel is crashing, you have no
idea what's going on on other CPUs, they may be crashing too, locked in
a tight loop, ... so sending an IPI there to do some work and expecting
it to report back is dangerous.

>> For Hyper-V, the situation is similar: hv_crash_handler() intitiates
>> VMbus unload on the crashing CPU only, there's no mechanism to do
>> 'global' unload so other CPUs will likely not be able to connect Vmbus
>> devices in kdump kernel but this should not be necessary.
>
> Given kdump is best-effort (and we can't rely on secondary CPUs even making it
> into the kdump kernel), I also don't think that should be necessary.

Yes, exactly.

>
>> There's a crash_kexec_post_notifiers mechanism which can be used instead
>> but it's disabled by default so using machine_ops.crash_shutdown is
>> better.
>
> Another option is to defer this to the kdump kernel. On arm64 at least, we know
> if we're in a kdump kernel early on, and can reset some state based upon that.
>
> Looking at x86's hyperv_cleanup(), everything relevant to arm64 can be deferred
> to just before the kdump kernel detects and initializes anything relating to
> hyperv. So AFAICT we could have hyperv_init() check is_kdump_kernel() prior to
> the first hypercall, and do the cleanup/reset there.

In theory yes, it is possible to try sending CHANNELMSG_UNLOAD on kdump
kernel boot and not upon crash, I don't remember if this approach was
tried in the past. 

>
> Maybe we need more data for the vmbus bits? ... if so it seems that could blow
> up anyway when the first kernel was tearing down.

Not sure I understood what you mean... From what I remember, there were
issues with CHANNELMSG_UNLOAD handling on the Hyper-V host side in the
past (it was taking *minutes* for the host to reply) but this is
orthogonal to the fact that we need to do this cleanup so kdump kernel
is able to connect to Vmbus devices again.

-- 
Vitaly


WARNING: multiple messages have this Message-ID (diff)
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	"Michael Kelley (LINUX)" <mikelley@microsoft.com>,
	Marc Zyngier <maz@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	will Deacon <will@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	Ard Biesheuvel <ardb@kernel.org>,
	broonie@kernel.org,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>
Subject: Re: Should arm64 have a custom crash shutdown handler?
Date: Thu, 05 May 2022 16:51:54 +0200	[thread overview]
Message-ID: <87y1zgyqut.fsf@redhat.com> (raw)
In-Reply-To: <YnPf3KPBXDNTpQoG@FVFF77S0Q05N.cambridge.arm.com>

Mark Rutland <mark.rutland@arm.com> writes:

> On Thu, May 05, 2022 at 03:52:24PM +0200, Vitaly Kuznetsov wrote:
>> "Guilherme G. Piccoli" <gpiccoli@igalia.com> writes:
>> 
>> > On 05/05/2022 09:53, Mark Rutland wrote:
>> >> [...]
>> >> Looking at those, the cleanup work is all arch-specific. What exactly would we
>> >> need to do on arm64, and why does it need to happen at that point specifically?
>> >> On arm64 we don't expect as much paravirtualization as on x86, so it's not
>> >> clear to me whether we need anything at all.
>> >> 
>> >>> Anyway, the idea here was to gather a feedback on how "receptive" arm64
>> >>> community would be to allow such customization, appreciated your feedback =)
>> >> 
>> >> ... and are you trying to do this for Hyper-V or just using that as an example?
>> >> 
>> >> I think we're not going to be very receptive without a more concrete example of
>> >> what you want.
>> >> 
>> >> What exactly do *you* need, and *why*? Is that for Hyper-V or another hypervisor?
>> >> 
>> >> Thanks
>> >> Mark.
>> >
>> > Hi Mark, my plan would be doing that for Hyper-V - kind of the same
>> > code, almost. For example, in hv_crash_handler() there is a stimer
>> > clean-up and the vmbus unload - my understanding is that this same code
>> > would need to run in arm64. Michael Kelley is CCed, he was discussing
>> > with me in the panic notifiers thread and may elaborate more on the needs.
>> >
>> > But also (not related with my specific plan), I've seen KVM quiesce code
>> > on x86 as well [see kvm_crash_shutdown() on arch/x86] , I'm not sure if
>> > this is necessary for arm64 or if this already executing in some
>> > abstracted form, I didn't dig deep - probably Vitaly is aware of that,
>> > hence I've CCed him here.
>> 
>> Speaking about the difference between reboot notifiers call chain and
>> machine_ops.crash_shutdown for KVM/x86, the main difference is that
>> reboot notifier is called on some CPU while the VM is fully functional,
>> this way we may e.g. still use IPIs (see kvm_pv_reboot_notify() doing
>> on_each_cpu()). When we're in a crash situation,
>> machine_ops.crash_shutdown is called on the CPU which crashed. We can't
>> count on IPIs still being functional so we do the very basic minimum so
>> *this* CPU can boot kdump kernel. There's no guarantee other CPUs can
>> still boot but normally we do kdump with 'nprocs=1'.
>
> Sure; IIUC the IPI problem doesn't apply to arm64, though, since that doesn't
> use a PV mechanism (and practically speaking will either be GICv2 or GICv3).
>

This isn't really about PV: when the kernel is crashing, you have no
idea what's going on on other CPUs, they may be crashing too, locked in
a tight loop, ... so sending an IPI there to do some work and expecting
it to report back is dangerous.

>> For Hyper-V, the situation is similar: hv_crash_handler() intitiates
>> VMbus unload on the crashing CPU only, there's no mechanism to do
>> 'global' unload so other CPUs will likely not be able to connect Vmbus
>> devices in kdump kernel but this should not be necessary.
>
> Given kdump is best-effort (and we can't rely on secondary CPUs even making it
> into the kdump kernel), I also don't think that should be necessary.

Yes, exactly.

>
>> There's a crash_kexec_post_notifiers mechanism which can be used instead
>> but it's disabled by default so using machine_ops.crash_shutdown is
>> better.
>
> Another option is to defer this to the kdump kernel. On arm64 at least, we know
> if we're in a kdump kernel early on, and can reset some state based upon that.
>
> Looking at x86's hyperv_cleanup(), everything relevant to arm64 can be deferred
> to just before the kdump kernel detects and initializes anything relating to
> hyperv. So AFAICT we could have hyperv_init() check is_kdump_kernel() prior to
> the first hypercall, and do the cleanup/reset there.

In theory yes, it is possible to try sending CHANNELMSG_UNLOAD on kdump
kernel boot and not upon crash, I don't remember if this approach was
tried in the past. 

>
> Maybe we need more data for the vmbus bits? ... if so it seems that could blow
> up anyway when the first kernel was tearing down.

Not sure I understood what you mean... From what I remember, there were
issues with CHANNELMSG_UNLOAD handling on the Hyper-V host side in the
past (it was taking *minutes* for the host to reply) but this is
orthogonal to the fact that we need to do this cleanup so kdump kernel
is able to connect to Vmbus devices again.

-- 
Vitaly


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2022-05-05 14:52 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-04 20:00 Should arm64 have a custom crash shutdown handler? Guilherme G. Piccoli
2022-05-04 20:00 ` Guilherme G. Piccoli
2022-05-05  7:29 ` Marc Zyngier
2022-05-05  7:29   ` Marc Zyngier
2022-05-05 12:44   ` Guilherme G. Piccoli
2022-05-05 12:44     ` Guilherme G. Piccoli
2022-05-05 12:53     ` Mark Rutland
2022-05-05 12:53       ` Mark Rutland
2022-05-05 13:05       ` Guilherme G. Piccoli
2022-05-05 13:05         ` Guilherme G. Piccoli
2022-05-05 13:15         ` Mark Rutland
2022-05-05 13:15           ` Mark Rutland
2022-05-05 13:19           ` Guilherme G. Piccoli
2022-05-05 13:19             ` Guilherme G. Piccoli
2022-05-05 13:52         ` Vitaly Kuznetsov
2022-05-05 13:52           ` Vitaly Kuznetsov
2022-05-05 14:07           ` Guilherme G. Piccoli
2022-05-05 14:07             ` Guilherme G. Piccoli
2022-05-05 14:31           ` Mark Rutland
2022-05-05 14:31             ` Mark Rutland
2022-05-05 14:51             ` Vitaly Kuznetsov [this message]
2022-05-05 14:51               ` Vitaly Kuznetsov
2022-05-06 11:01               ` Mark Rutland
2022-05-06 11:01                 ` Mark Rutland
2022-05-30  1:51                 ` Michael Kelley (LINUX)
2022-05-30  1:51                   ` Michael Kelley (LINUX)
2022-05-05 11:10 ` Mark Rutland
2022-05-05 11:10   ` Mark Rutland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y1zgyqut.fsf@redhat.com \
    --to=vkuznets@redhat.com \
    --cc=ardb@kernel.org \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=gpiccoli@igalia.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.