Message-ID: <Yjw0KFZ+DG0A1KxK@google.com>
Date:   Thu, 24 Mar 2022 09:04:40 +0000
From:   Oliver Upton <oupton@...gle.com>
To:     Gavin Shan <gshan@...hat.com>
Cc:     kvmarm@...ts.cs.columbia.edu, maz@...nel.org,
        linux-kernel@...r.kernel.org, eauger@...hat.com,
        shan.gavin@...il.com, Jonathan.Cameron@...wei.com,
        pbonzini@...hat.com, vkuznets@...hat.com, will@...nel.org
Subject: Re: [PATCH v5 02/22] KVM: arm64: Add SDEI virtualization
 infrastructure

On Thu, Mar 24, 2022 at 02:54:00PM +0800, Gavin Shan wrote:
> > > +#define KVM_SDEI_DEFAULT_EVENT	0
> > > +
> > > +#define KVM_SDEI_MAX_VCPUS	512	/* Aligned to 64 */
> > > +#define KVM_SDEI_MAX_EVENTS	128
> > 
> > I would *strongly* recommend against having these limits. I find the
> > vCPU limit especially concerning, because we're making KVM_MAX_VCPUS
> > ABI, which it definitely is not. Anything that deals with a vCPU should
> > be accessed through a vCPU FD (and thus agnostic to the maximum number
> > of vCPUs) to avoid such a complication.
> > 
> 
> For KVM_SDEI_DEFAULT_EVENT, which corresponds to the software signaled
> event. As you suggested on PATCH[15/22], we can't assume its usage.
> I will define it with SDEI_SW_SIGNALED_EVENT in uapi/linux/arm_sdei.h
> 
> For KVM_SDEI_MAX_EVENTS, it will be moved from this header file to
> kvm_sdei.h after static arrays to hold the data structures or their
> pointers are used, as you suggested early for this patch (PATCH[02/22]).
> 
> There are two types of (SDEI) events: shared and private. For the private
> event, it can be registered independently from the vcpus. It also means
> the address and argument for the entry points, corresponding to @ep_address
> and @ep_arg in struct kvm_sdei_registered_event, can be different on
> the individual vcpus. However, all the registered/enabled states and
> the entry point address and argument are same on all vcpus for the shared
> event. KVM_SDEI_MAX_VCPUS was introduced to use same data structure to
> represent both shared and private event.

You're providing a great deal of abstraction around the SDEI
specification, but I really am unconvinced that KVM needs that. This
series needs to add support for a single SDEI event (event 0) and async
PF to follow. Since we are going to support a static set of events under
KVM I believe a lot of the complexity in this design should fade away.

> > > +struct kvm_sdei_exposed_event_state {
> > > +	__u64	num;
> > > +
> > > +	__u8	type;
> > > +	__u8	signaled;
> > > +	__u8	priority;
> > > +	__u8	padding[5];
> > > +	__u64	notifier;
> > 
> > Wait, isn't this a kernel function pointer!?
> > 
> 
> Yeah, it is a kernel function pointer, used by Async PF to know if
> the corresponding event has been handled or not. Async PF can cancel
> the previously injected event for performance concerns. Either Async PF
> or SDEI needs to migrate it. To keep SDEI transparent enough to Async PF,
> SDEI is responsible for its migration.

But this is a UAPI header, why would we even consider giving userspace a
window into the kernel like this? Couldn't userspace crash the kernel by
writing whatever it wants to this field, knowing that it will call it as
a function pointer?

Security aside, there's no guarantee that a function winds up at the
same address between compiler versions or kernel versions.

Overall, I don't think that userspace should have the ability to add
arbitrary SDEI events. KVM takes ownership of its own vendor-specific
ABI in patch 3/22 by declaring its vendor identifier, so any new events
we support must remain within KVM. There is going to be some state that
will need to be migrated for KVM's SDEI events, that ought to be
surfaced to userspace through the KVM_{GET,SET}_ONE_REG ioctls.

Given that there isn't much userspace involvement to make SDEI
work, do you think it would be possible to drop the proposed UAPI from
your series and work on enabling software-signaled SDEI events within
KVM first? By this I mean a VM running under KVM shouldn't require any
ioctls to make it work.

In so doing, we can discover exactly what the mechanics look like in KVM
and only then talk about the necessary UAPI to migrate state. One piece
of the mechanics that is top of mind which I'd like to see addressed is
the use of linked lists and the preallocations that have been made in
structures. KVM will know how many events exist at compile time, so we
can represent these statically.

> > > +};
> > > +
> > > +struct kvm_sdei_registered_event_state {
> > 
> > You should fold these fields together with kvm_sdei_exposed_event_state
> > into a single 'kvm_sdei_event' structure:
> > 
> 
> @route_mode and @route_affinity can't be configured or modified until
> the event is registered. Besides, they're only valid to the shared
> events. For private events, they don't have the routing needs. It means
> those two fields would be part of struct kvm_sdei_registered_event instead
> of kvm_sdei_exposed_event.
> 
> 
> > > +	__u64	num;
> > > +
> > > +	__u8	route_mode;
> > > +	__u8	padding[3];
> > > +	__u64	route_affinity;
> > 
> > And these shouldn't be UAPI at the VM scope. Each of these properties
> > could be accessed via a synthetic/'pseudo-firmware' register on a vCPU FD:
> > 
> 
> They're accessed through vcpu or kvm fd depending on what type the event
> is. For the VM-owned shared event, they're accessed through KVM fd. For the
> vcpu-owned private event, they're accessed through vcpu fd.

Some of the state that you represent in struct kvm_sdei_registered_event_state
is really per-vCPU state. Any time that there's data available at a vCPU
granularity userspace should access it with a vCPU FD.

> I'm not sure if I catch the idea to have a synthetic register and I'm to
> confirm. If I'm correct, you're talking about the "IMPLEMENTATION DEFINED"
> system register, whose OP0 and CRn are 0B11 and 0B1x11. If two implementation
> defined registers can be adopted, I don't think we need to expose anything
> through ABI. All the operations and the needed data can be passed through
> the system registers.

No, I'm not talking about the guest-facing interface. I'm talking about
what gets exposed to userspace to migrate the VM's state. For parts of
the guest that do not map to an architectural construct, we've defined
the concept of a firmware pseudo-register. What that really means is we
expose a register to userspace that is inaccessible from the guest which
migrates KVM's nonarchitected state. We are abusing the fact that VMMs
will save/restore whatever registers are reported on KVM_GET_REG_LIST to
trick it into migrating KVM state, and it has worked quite nicely to
avoid adding new ioctls for new features. It also means that older VMMs
are capable of utilizing new KVM features, so long as they obey the
prescribed rules.

--
Thanks,
Oliver

Powered by blists - more mailing lists