Message-ID: <1ea52603-2e99-1e81-92b3-0e14cf1e2d1e@arm.com>
Date:   Thu, 11 Mar 2021 12:35:22 +0000
From:   Steven Price <steven.price@....com>
To:     Peter Maydell <peter.maydell@...aro.org>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>,
        James Morse <james.morse@....com>,
        Julien Thierry <julien.thierry.kdev@...il.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        kvmarm <kvmarm@...ts.cs.columbia.edu>,
        arm-mail-list <linux-arm-kernel@...ts.infradead.org>,
        lkml - Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dave Martin <Dave.Martin@....com>,
        Mark Rutland <mark.rutland@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        QEMU Developers <qemu-devel@...gnu.org>,
        Juan Quintela <quintela@...hat.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        Richard Henderson <richard.henderson@...aro.org>,
        Haibo Xu <Haibo.Xu@....com>, Andrew Jones <drjones@...hat.com>
Subject: Re: [PATCH v9 6/6] KVM: arm64: Document MTE capability and ioctl

On 09/03/2021 11:01, Peter Maydell wrote:
> On Mon, 1 Mar 2021 at 14:23, Steven Price <steven.price@....com> wrote:
>>
>> A new capability (KVM_CAP_ARM_MTE) identifies that the kernel supports
>> granting a guest access to the tags, and provides a mechanism for the
>> VMM to enable it.
>>
>> A new ioctl (KVM_ARM_MTE_COPY_TAGS) provides a simple way for a VMM to
>> access the tags of a guest without having to maintain a PROT_MTE mapping
>> in userspace. The above capability gates access to the ioctl.
>>
>> Signed-off-by: Steven Price <steven.price@....com>
>> ---
>>   Documentation/virt/kvm/api.rst | 37 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>> index aed52b0fc16e..1406ea138127 100644
>> --- a/Documentation/virt/kvm/api.rst
>> +++ b/Documentation/virt/kvm/api.rst
>> @@ -4939,6 +4939,23 @@ KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO
>>   Allows Xen vCPU attributes to be read. For the structure and types,
>>   see KVM_XEN_VCPU_SET_ATTR above.
>>
>> +4.131 KVM_ARM_MTE_COPY_TAGS
>> +---------------------------
>> +
>> +:Capability: KVM_CAP_ARM_MTE
>> +:Architectures: arm64
>> +:Type: vm ioctl
>> +:Parameters: struct kvm_arm_copy_mte_tags
>> +:Returns: 0 on success, < 0 on error
>> +
>> +Copies Memory Tagging Extension (MTE) tags to/from guest tag memory.
> 
> Mostly virt/kvm/api.rst seems to include documentation of the
> associated structs, something like:
> 
> ::
> 
>    struct kvm_arm_copy_mte_tags {
>           __u64 guest_ipa;
>           __u64 length;
>           union {
>                   void __user *addr;
>                   __u64 padding;
>           };
>           __u64 flags;
>    };
> 
> 
> which saves the reader having to cross-reference against the header file.

Good point - I'll add that.

> It also means you can more naturally use the actual field names in the doc,
> eg:
> 
>> +The
>> +starting address and length of guest memory must be ``PAGE_SIZE`` aligned.
> 
> you could say "The guest_ipa and length fields" here.
> 
> Also "The addr field must point to a buffer which the tags will
> be copied to or from." I assume.

Indeed - I'll add the clarification.

>> +The size of the buffer to store the tags is ``(length / MTE_GRANULE_SIZE)``
>> +bytes (i.e. 1/16th of the corresponding size).
> 
>> + Each byte contains a single tag
>> +value. This matches the format of ``PTRACE_PEEKMTETAGS`` and
>> +``PTRACE_POKEMTETAGS``.
> 
> What are the valid values for 'flags' ? It looks like they specify which
> direction the copy is, which we definitely need to document here.

Yes either KVM_ARM_TAGS_TO_GUEST or KVM_ARM_TAGS_FROM_GUEST - again I'll 
clarify that.

> What happens if the caller requests a tag copy for an area of guest
> address space which doesn't have tags (eg it has nothing mapped),
> or for an area of guest addres space which has tags in some parts
> but not in others ?

Guest memory either exists (and has tags) or doesn't exist (assuming MTE 
is enabled for the guest). So the cases this can fail are:

  * The region isn't completely covered with memslots
  * The region isn't completely writable (and KVM_ARM_TAGS_TO_GUEST is 
specified).
  * User space doesn't have access to the memory (i.e. the memory would 
SIGSEGV or similar if the VMM accessed it).

Currently all the above produce the error -ENOENT, which now I come to 
enumerate the cases doesn't seem like a great error code (it's really 
only appropriate for the first)! Perhaps -EFAULT would be better.

>> +
>>   5. The kvm_run structure
>>   ========================
>>
>> @@ -6227,6 +6244,25 @@ KVM_RUN_BUS_LOCK flag is used to distinguish between them.
>>   This capability can be used to check / enable 2nd DAWR feature provided
>>   by POWER10 processor.
>>
>> +7.23 KVM_CAP_ARM_MTE
>> +--------------------
>> +
>> +:Architectures: arm64
>> +:Parameters: none
>> +
>> +This capability indicates that KVM (and the hardware) supports exposing the
>> +Memory Tagging Extensions (MTE) to the guest. It must also be enabled by the
>> +VMM before the guest will be granted access.
>> +
>> +When enabled the guest is able to access tags associated with any memory given
>> +to the guest. KVM will ensure that the pages are flagged ``PG_mte_tagged`` so
>> +that the tags are maintained during swap or hibernation of the host, however
> 
> s/,/;/

Yep

>> +the VMM needs to manually save/restore the tags as appropriate if the VM is
>> +migrated.
>> +
>> +When enabled the VMM may make use of the ``KVM_ARM_MTE_COPY_TAGS`` ioctl to
>> +perform a bulk copy of tags to/from the guest
> 
> "guest."

Good spot.

>> +
>>   8. Other capabilities.
>>   ======================
>>
>> @@ -6716,3 +6752,4 @@ KVM_XEN_HVM_SET_ATTR, KVM_XEN_HVM_GET_ATTR, KVM_XEN_VCPU_SET_ATTR and
>>   KVM_XEN_VCPU_GET_ATTR ioctls, as well as the delivery of exception vectors
>>   for event channel upcalls when the evtchn_upcall_pending field of a vcpu's
>>   vcpu_info is set.
>> +
>> --
>> 2.20.1
> 
> 
> Stray whitespace change ?

Not sure how that got there - but will remove.

Thanks,

Steve

Powered by blists - more mailing lists