Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 10 Jun 2022 14:29:55 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Grzegorz Jaszczyk <jaz@semihalf.com>
Cc:     linux-kernel@vger.kernel.org, Dmytro Maluka <dmy@semihalf.com>,
        Zide Chen <zide.chen@intel.corp-partner.google.com>,
        Peter Fang <peter.fang@intel.corp-partner.google.com>,
        Tomasz Nowicki <tn@semihalf.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>, Pavel Machek <pavel@ucw.cz>,
        Ashish Kalra <ashish.kalra@amd.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Sachi King <nakato@nakato.io>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        David Dunn <daviddunn@google.com>,
        Wei Wang <wei.w.wang@intel.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        "open list:KERNEL VIRTUAL MACHINE (KVM)" <kvm@vger.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        "open list:ACPI" <linux-acpi@vger.kernel.org>,
        "open list:HIBERNATION (aka Software Suspend, aka swsusp)" 
        <linux-pm@vger.kernel.org>, dbehr@google.com, dtor@google.com
Subject: Re: [PATCH 1/2] x86: notify hypervisor about guest entering s2idle
 state
Message-ID: <YqNVYz4+yVbWnmNv@google.com>
References: <20220609110337.1238762-1-jaz@semihalf.com>
 <20220609110337.1238762-2-jaz@semihalf.com>
 <YqIJ8HtdqnoVzfQD@google.com>
 <CAH76GKNRDXAyGYvs2ji5Phu=5YPW8+SV8-6TLjizBRzTCnEROg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAH76GKNRDXAyGYvs2ji5Phu=5YPW8+SV8-6TLjizBRzTCnEROg@mail.gmail.com>
Precedence: bulk

On Fri, Jun 10, 2022, Grzegorz Jaszczyk wrote:
> czw., 9 cze 2022 o 16:55 Sean Christopherson <seanjc@google.com> napisał(a):
> Above could be actually prevented if the VMM had control over the
> guest resumption. E.g. after VMM receives notification about guest
> entering s2idle state, it would park the vCPU actually preventing it
> from exiting s2idle without VMM intervention.

Ah, so you avoid races by assuming the VM wakes itself from s2idle any time a vCPU
is run, even if the vCPU doesn't actually have a wake event.  That would be very
useful info to put in the changelog.

> > > +static void s2idle_hypervisor_notify(void)
> > > +{
> > > +     if (static_cpu_has(X86_FEATURE_HYPERVISOR))
> > > +             kvm_hypercall0(KVM_HC_SYSTEM_S2IDLE);
> >
> > Checking the HYPERVISOR flag is not remotely sufficient.  The hypervisor may not
> > be KVM, and if it is KVM, it may be an older version of KVM that doesn't support
> > the hypercall.  The latter scenario won't be fatal unless KVM has been modified,
> > but blindly doing a hypercall for a different hypervisor could have disastrous
> > results, e.g. the registers ABIs are different, so the above will make a random
> > request depending on what is in other GPRs.
> 
> Good point: we've actually thought about not confusing/breaking VMMs
> so I've introduced KVM_CAP_X86_SYSTEM_S2IDLE VM capability in the
> second patch, but not breaking different hypervisors is another story.
> Would hiding it under new 's2idle_notify_kvm' module parameter work
> for upstream?:

No, enumerating support via KVM_CPUID_FEATURES is the correct way to do something
like this, e.g. see KVM_FEATURE_CLOCKSOURCE2.  But honestly I wouldn't spend too
much time understanding how all of that works, because I still feel quite strongly
that getting KVM involved is completely unnecessary.  A solution that isn't KVM
specific is preferable as it can then be implemented by any VMM that enumerates
s2idle support to the guest.

> > The bigger question is, why is KVM involved at all?  KVM is just a dumb pipe out
> > to userspace, and not a very good one at that.  There are multiple well established
> > ways to communicate with the VMM without custom hypercalls.
> 
> Could you please kindly advise about the recommended way of
> communication with VMM, taking into account that we want to send this
> notification just before entering s2idle state (please see also answer
> to next comment), which is at a very late stage of the suspend process
> with a lot of functionality already suspended?

MMIO or PIO for the actual exit, there's nothing special about hypercalls.  As for
enumerating to the guest that it should do something, why not add a new ACPI_LPS0_*
function?  E.g. something like

static void s2idle_hypervisor_notify(void)
{
	if (lps0_dsm_func_mask > 0)
		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT_HYPERVISOR_NOTIFY
					lps0_dsm_func_mask, lps0_dsm_guid);
}