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:   Wed, 23 Mar 2022 17:28:26 +0000
From:   Oliver Upton <oupton@google.com>
To:     Gavin Shan <gshan@redhat.com>
Cc:     kvmarm@lists.cs.columbia.edu, maz@kernel.org,
        linux-kernel@vger.kernel.org, eauger@redhat.com,
        shan.gavin@gmail.com, Jonathan.Cameron@huawei.com,
        pbonzini@redhat.com, vkuznets@redhat.com, will@kernel.org
Subject: Re: [PATCH v5 18/22] KVM: arm64: Support SDEI ioctl commands on VM
Message-ID: <YjtYuk+Jx1dFPQQ9@google.com>
References: <20220322080710.51727-1-gshan@redhat.com>
 <20220322080710.51727-19-gshan@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220322080710.51727-19-gshan@redhat.com>
Precedence: bulk

On Tue, Mar 22, 2022 at 04:07:06PM +0800, Gavin Shan wrote:
> This supports ioctl commands on VM to manage the various objects.
> It's primarily used by VMM to accomplish migration. The ioctl
> commands introduced by this are highlighted as below:
> 
>    * KVM_SDEI_CMD_GET_VERSION
>      Retrieve the version of current implementation. It's different
>      from the version of the followed SDEI specification. This version
>      is used to indicates what functionalities documented in the SDEI
>      specification have been supported or not supported.

Don't we need a way to set the version as well? KVM is very much
responsible for upholding ABI of older specs. So, if a VMM and guest
expect SDEI v1.1, we can't just forcibly raise it to something else
during a migration.

The PSCI implementation is a great example of how KVM has grown its
implementation in line with a specification, all the while preserving
backwards compatibility.

>    * KVM_SDEI_CMD_GET_EXPOSED_EVENT_COUNT
>      Return the total count of exposed events.
> 
>    * KVM_SDEI_CMD_GET_EXPOSED_EVENT
>    * KVM_SDEI_CMD_SET_EXPOSED_EVENT
>      Get or set exposed event
> 
>    * KVM_SDEI_CMD_GET_REGISTERED_EVENT_COUNT
>      Return the total count of registered events.
> 
>    * KVM_SDEI_CMD_GET_REGISTERED_EVENT
>    * KVM_SDEI_CMD_SET_REGISTERED_EVENT
>      Get or set registered event.

Any new UAPI needs to be documented in Documentation/virt/kvm/api.rst

Additionally, we desperately need a better, generic way to save/restore
VM scoped state. IMO, we should only be adding ioctls if we are
affording userspace a meaningful interface. Every save/restore pair of
ioctls winds up wasting precious ioctl numbers and requires userspace
take a change to read/write an otherwise opaque value.

Marc had made some suggestions in this area already that Raghavendra
experimented with [1], and I think its time to meaningfully consider
our options. Basically, KVM_GET_REG_LIST needs to convey whether a
particular register is VM or vCPU state. We only need to save/restore a
VM state register once. That way, userspace doesn't have to care about
the underlying data and the next piece of VM state that comes along
doesn't require an ioctl nr nor VMM participation.

[1]: http://lore.kernel.org/r/20220224172559.4170192-3-rananta@google.com

--
Thanks,
Oliver