Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

From: Cornelia Huck <cohuck@redhat.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Jing Zhang <jingzhangos@google.com>,
	Marc Zyngier <maz@kernel.org>, KVM <kvm@vger.kernel.org>,
	KVMARM <kvmarm@lists.linux.dev>,
	ARMLinux <linux-arm-kernel@lists.infradead.org>,
	Will Deacon <will@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Fuad Tabba <tabba@google.com>, Reiji Watanabe <reijiw@google.com>,
	Raghavendra Rao Ananta <rananta@google.com>,
	Suraj Jitindar Singh <surajjs@amazon.com>
Subject: Re: [PATCH v6 3/6] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1
Date: Thu, 27 Jul 2023 11:34:57 +0200	[thread overview]
Message-ID: <878rb166ke.fsf@redhat.com> (raw)
In-Reply-To: <ZMFWsvWzfkkz2VNB@thinky-boi>

On Wed, Jul 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:

> Hi Cornelia,
>
> On Mon, Jul 24, 2023 at 10:45:44AM +0200, Cornelia Huck wrote:
>> On Fri, Jul 21 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
>> > What I had in mind was something similar to the KVM_GET_ONE_REG ioctl,
>> > but instead of returning the register value it'd return the mask of the
>> > register. This would keep the kernel implementation dead simple (I'm
>> > lazy) and more easily allow for future expansion in case we want to
>> > start describing more registers this way. Userspace would iterate the ID
>> > register space and ask the kernel for the mask of registers it wants to
>> > change.
>> 
>> Hm... for userspace it might be easier to get one big list and then
>> parse it afterwards? Similar to what GET_REG_LIST does today.
>
> Possibly, but I felt like it was a bit different from GET_REG_LIST since
> this would actually be a list of key-value pairs (reg_id, mask) instead
> of a pure enumeration of IDs. My worry is that if/when we wind up describing
> more registers in this list-based ioctl then userspace is going to wind
> up traversing that structure a lot to find the register masks it actually
> cares about.

Depends on how userspace actually digests it, but point taken.

>
>> Are you thinking more of a KVM_GET_REG_INFO or so ioctl, that could
>> support different kinds of extra info (and might also make sense for
>> other architectures?) If we end up with something more versatile, it
>> might make sense going that route.
>
> TBH, I hadn't considered the extensibililty of a per-register ioctl, but
> that does seem like a good point.

Maybe smth like

/* available with KVM_CAP_GET_REG_INFO */
struct kvm_reg_info {
	__u64 id;
	__u32 op;
	__u32 len;
	__u8 data[];
};

/* operations for kvm_reg_info->op */
#define KVM_REG_INFO_ARM_ID_REG 0

#define KVM_GET_REG_INFO _IOW(KVMIO, 0xd2, struct kvm_reg_info)

and returning sys_reg_desc->val in data if id points to a valid id reg.