From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3EB1AC2B9F4 for ; Tue, 22 Jun 2021 10:59:10 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0A3A961352 for ; Tue, 22 Jun 2021 10:59:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0A3A961352 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=rqV80FjRH5c0kwVVGRYIGvv3g8fp/Alfnih78glNVw4=; b=FctcMoY4H3yx43 1MIhH9V9AjtrZYvQlbsmb2rM/u5++yNBCQ8d0VNshnUq/6/wEMpOqtQaJjeP3+UTbkhClzOjFmgZj 9c2zV0/pQjBjJW27zdDIRdNDrEJsFxyNt+wFVbc2q3RfewIXpyef6jzyMNy7iKOYkbTOGDjTQ0tWp H5EhyTRBUEJhPAxVC6KU4dCegf3q7pTKN9l22V/Q7V5xy+zAl1WNqa8ZDrdC4DjtdXohQFLMh1tJz W7ur/5d9kw0VbUpZwsu/PWPk3nAFdl94jW6ZVIOuhqi1PpqMeZYmovzGP/DHg8XFkT4wTWVwho2oR fY+xjqOOnATaV5co7rLQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lve5p-006nXt-Uo; Tue, 22 Jun 2021 10:57:22 +0000 Received: from mail-oi1-x235.google.com ([2607:f8b0:4864:20::235]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lve5i-006nWS-AT for linux-arm-kernel@lists.infradead.org; Tue, 22 Jun 2021 10:57:19 +0000 Received: by mail-oi1-x235.google.com with SMTP id d19so23294585oic.7 for ; Tue, 22 Jun 2021 03:57:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=HnUuefF6pQt/5KIq6n+0n8uuy3Ij0ViBdDnbo/wnUBQ=; b=v2iGUHfXJmUEDSMyQJPjBTsxpCniaCK4XEaVIHbLtw0MOLxkmtcrwd8Nf8TGNr9eU3 yKo3eLVqj7x3+LuDTbPN7oJTYuje3aTHLcu2MOsCoZvrrTZKh6WkGYsGp2deZuzNs5Dl D+S6Gn6stNcXDegcJlErLCyQIn0UT8GI7vtF3KekwYwrmoi7pJvgh7TNSVfHdSbNhdXI qFjpbB2N6hNqWwXzXReTFa87IHkQrfTy73f91u7HNx0r6UGEfI4ikszxmmBH37mvhvV/ 5KzQO/6J5WpjlQ6SmjokgnG4vV7aw8mEytCI9ifoflnOyk1tMqgFIy0QNJ9UKjd3ZbEc HhDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=HnUuefF6pQt/5KIq6n+0n8uuy3Ij0ViBdDnbo/wnUBQ=; b=sGmnpfjQkitw4pws/qYw3xQusot1mAQQJLIX8fnMahbiZobS878lLwcD3y387Kkfi+ 9RzNJjY5bmFjQ5vMaFqFjoA4VGLwrbzFMPbUv/7uUe8gHxNJOcHZCoJMzMbwUovv5dX7 jdgWh9r8WjeRkVr/baTBB0Id43fNq5EykXXckghQSHOOTZnrcZ9WcVqxbvVWB807FGE3 SkOLHsjqzHeif6+MbYw0uUwB/qeFrD8TZqzX2wlKcjbiKwxfkgU1C4QMMtT4Yh/ZZ7PC yLF0hcKjeUbgQjjDxffyYTPagp8ga73tl77ZFnAP9V0NF7igRqrtWJLHiSbdvzULW5jn xAmw== X-Gm-Message-State: AOAM5333EXMoPchSvpuCgJI2UUcU1fbGnO54Kn6EkwsVnggkUbNQlbG2 dAxPGUuYUNOZW0ZFBo+Glrh/Qy2FrCkJF8ZIgcC8OQ== X-Google-Smtp-Source: ABdhPJzDc1X7iIOGSMjc4ltvXLq3EcfZ4hStQ7IHZaNwCEuG3WuoK0wf2exQLr0OAvu+BjQIixbqJmuByA8fdy/G8qQ= X-Received: by 2002:aca:de07:: with SMTP id v7mr2542559oig.8.1624359433329; Tue, 22 Jun 2021 03:57:13 -0700 (PDT) MIME-Version: 1.0 References: <20210621111716.37157-1-steven.price@arm.com> <20210621111716.37157-6-steven.price@arm.com> <875yy6ci20.wl-maz@kernel.org> In-Reply-To: <875yy6ci20.wl-maz@kernel.org> From: Fuad Tabba Date: Tue, 22 Jun 2021 11:56:37 +0100 Message-ID: Subject: Re: [PATCH v17 5/6] KVM: arm64: ioctl to fetch/store tags in a guest To: Marc Zyngier Cc: Steven Price , Catalin Marinas , Will Deacon , "Dr. David Alan Gilbert" , qemu-devel@nongnu.org, Dave Martin , Juan Quintela , Richard Henderson , linux-kernel@vger.kernel.org, Thomas Gleixner , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210622_035714_435279_B36C95B0 X-CRM114-Status: GOOD ( 59.76 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Marc, On Tue, Jun 22, 2021 at 11:25 AM Marc Zyngier wrote: > > Hi Fuad, > > On Tue, 22 Jun 2021 09:56:22 +0100, > Fuad Tabba wrote: > > > > Hi, > > > > > > On Mon, Jun 21, 2021 at 12:18 PM Steven Price wrote: > > > > > > The VMM may not wish to have it's own mapping of guest memory mapped > > > with PROT_MTE because this causes problems if the VMM has tag checking > > > enabled (the guest controls the tags in physical RAM and it's unlikely > > > the tags are correct for the VMM). > > > > > > Instead add a new ioctl which allows the VMM to easily read/write the > > > tags from guest memory, allowing the VMM's mapping to be non-PROT_MTE > > > while the VMM can still read/write the tags for the purpose of > > > migration. > > > > > > Reviewed-by: Catalin Marinas > > > Signed-off-by: Steven Price > > > --- > > > arch/arm64/include/asm/kvm_host.h | 3 ++ > > > arch/arm64/include/asm/mte-def.h | 1 + > > > arch/arm64/include/uapi/asm/kvm.h | 11 +++++ > > > arch/arm64/kvm/arm.c | 7 +++ > > > arch/arm64/kvm/guest.c | 82 +++++++++++++++++++++++++++++++ > > > include/uapi/linux/kvm.h | 1 + > > > 6 files changed, 105 insertions(+) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > > index 309e36cc1b42..6a2ac4636d42 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -729,6 +729,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > > > int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > > > struct kvm_device_attr *attr); > > > > > > +long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > > + struct kvm_arm_copy_mte_tags *copy_tags); > > > + > > > /* Guest/host FPSIMD coordination helpers */ > > > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); > > > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); > > > diff --git a/arch/arm64/include/asm/mte-def.h b/arch/arm64/include/asm/mte-def.h > > > index cf241b0f0a42..626d359b396e 100644 > > > --- a/arch/arm64/include/asm/mte-def.h > > > +++ b/arch/arm64/include/asm/mte-def.h > > > @@ -7,6 +7,7 @@ > > > > > > #define MTE_GRANULE_SIZE UL(16) > > > #define MTE_GRANULE_MASK (~(MTE_GRANULE_SIZE - 1)) > > > +#define MTE_GRANULES_PER_PAGE (PAGE_SIZE / MTE_GRANULE_SIZE) > > > #define MTE_TAG_SHIFT 56 > > > #define MTE_TAG_SIZE 4 > > > #define MTE_TAG_MASK GENMASK((MTE_TAG_SHIFT + (MTE_TAG_SIZE - 1)), MTE_TAG_SHIFT) > > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > > > index 24223adae150..b3edde68bc3e 100644 > > > --- a/arch/arm64/include/uapi/asm/kvm.h > > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > > @@ -184,6 +184,17 @@ struct kvm_vcpu_events { > > > __u32 reserved[12]; > > > }; > > > > > > +struct kvm_arm_copy_mte_tags { > > > + __u64 guest_ipa; > > > + __u64 length; > > > + void __user *addr; > > > + __u64 flags; > > > + __u64 reserved[2]; > > > +}; > > > + > > > +#define KVM_ARM_TAGS_TO_GUEST 0 > > > +#define KVM_ARM_TAGS_FROM_GUEST 1 > > > + > > > /* If you need to interpret the index values, here is the key: */ > > > #define KVM_REG_ARM_COPROC_MASK 0x000000000FFF0000 > > > #define KVM_REG_ARM_COPROC_SHIFT 16 > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > > index 28ce26a68f09..511f3716fe33 100644 > > > --- a/arch/arm64/kvm/arm.c > > > +++ b/arch/arm64/kvm/arm.c > > > @@ -1359,6 +1359,13 @@ long kvm_arch_vm_ioctl(struct file *filp, > > > > > > return 0; > > > } > > > + case KVM_ARM_MTE_COPY_TAGS: { > > > + struct kvm_arm_copy_mte_tags copy_tags; > > > + > > > + if (copy_from_user(©_tags, argp, sizeof(copy_tags))) > > > + return -EFAULT; > > > + return kvm_vm_ioctl_mte_copy_tags(kvm, ©_tags); > > > + } > > > default: > > > return -EINVAL; > > > } > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > index 5cb4a1cd5603..4ddb20017b2f 100644 > > > --- a/arch/arm64/kvm/guest.c > > > +++ b/arch/arm64/kvm/guest.c > > > @@ -995,3 +995,85 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > > > > > > return ret; > > > } > > > + > > > +long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > > > + struct kvm_arm_copy_mte_tags *copy_tags) > > > +{ > > > + gpa_t guest_ipa = copy_tags->guest_ipa; > > > + size_t length = copy_tags->length; > > > + void __user *tags = copy_tags->addr; > > > + gpa_t gfn; > > > + bool write = !(copy_tags->flags & KVM_ARM_TAGS_FROM_GUEST); > > > + int ret = 0; > > > + > > > + if (!kvm_has_mte(kvm)) > > > + return -EINVAL; > > > + > > > + if (copy_tags->reserved[0] || copy_tags->reserved[1]) > > > + return -EINVAL; > > > + > > > + if (copy_tags->flags & ~KVM_ARM_TAGS_FROM_GUEST) > > > + return -EINVAL; > > > + > > > + if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK) > > > + return -EINVAL; > > > + > > > + gfn = gpa_to_gfn(guest_ipa); > > > + > > > + mutex_lock(&kvm->slots_lock); > > > + > > > + while (length > 0) { > > > + kvm_pfn_t pfn = gfn_to_pfn_prot(kvm, gfn, write, NULL); > > > + void *maddr; > > > + unsigned long num_tags; > > > + struct page *page; > > > + > > > + if (is_error_noslot_pfn(pfn)) { > > > + ret = -EFAULT; > > > + goto out; > > > + } > > > + > > > + page = pfn_to_online_page(pfn); > > > + if (!page) { > > > + /* Reject ZONE_DEVICE memory */ > > > + ret = -EFAULT; > > > + goto out; > > > + } > > > + maddr = page_address(page); > > > + > > > + if (!write) { > > > + if (test_bit(PG_mte_tagged, &page->flags)) > > > + num_tags = mte_copy_tags_to_user(tags, maddr, > > > + MTE_GRANULES_PER_PAGE); > > > + else > > > + /* No tags in memory, so write zeros */ > > > + num_tags = MTE_GRANULES_PER_PAGE - > > > + clear_user(tags, MTE_GRANULES_PER_PAGE); > > > + kvm_release_pfn_clean(pfn); > > > + } else { > > > + num_tags = mte_copy_tags_from_user(maddr, tags, > > > + MTE_GRANULES_PER_PAGE); > > > + kvm_release_pfn_dirty(pfn); > > > + } > > > + > > > + if (num_tags != MTE_GRANULES_PER_PAGE) { > > > + ret = -EFAULT; > > > + goto out; > > > + } > > > + > > > + /* Set the flag after checking the write completed fully */ > > > + if (write) > > > + set_bit(PG_mte_tagged, &page->flags); > > > + > > > + gfn++; > > > + tags += num_tags; > > > + length -= PAGE_SIZE; > > > + } > > > + > > > +out: > > > + mutex_unlock(&kvm->slots_lock); > > > + /* If some data has been copied report the number of bytes copied */ > > > + if (length != copy_tags->length) > > > + return copy_tags->length - length; > > > > I'm not sure if this is actually an issue, but a couple of comments on > > the return value if there is an error after a partial copy has been > > done. If mte_copy_tags_to_user or mte_copy_tags_from_user don't return > > MTE_GRANULES_PER_PAGE, then the check for num_tags would fail, but > > some of the tags would have been copied, which wouldn't be reflected > > in length. That said, on a write the tagged bit wouldn't be set, and > > on read then the return value would be conservative, but not > > incorrect. > > > > That said, even though it is described that way in the documentation > > (rather deep in the description though), it might be confusing to > > return a non-negative value on an error. The other kvm ioctl I could > > find that does something similar, KVM_S390_GET_IRQ_STATE, seems to > > always return a -ERROR on error, rather than the number of bytes > > copied. > > My mental analogy for this ioctl is the read()/write() syscalls, which > return the number of bytes that have been transferred in either > direction. > > I agree that there are some corner cases (a tag copy that fails > because of a faulty page adjacent to a valid page will still report > some degree of success), but it is also important to report what has > actually been done in either direction. read()/write() return an error (-1) and not the amount copied if there is an actual error I believe: https://man7.org/linux/man-pages/man2/read.2.html > It is not an error if this number is smaller than the number of > bytes requested; this may happen for example because fewer bytes > are actually available right now (maybe because we were close to > end-of-file, or because we are reading from a pipe, or from a > terminal), or because read() was interrupted by a signal. > > On error, -1 is returned, and errno is set to indicate the error. > In this case, it is left unspecified whether the file position > (if any) changes. I think that for the current return value, then it would be good for the documentation in patch 6/6 to be more explicit. There it says: > :Returns: number of bytes copied, < 0 on error (-EINVAL for incorrect > arguments, -EFAULT if memory cannot be accessed). Later on it does state that if an error happens after some copying has been done, it returns the number copied. But that's at the end of the section. I think it would be less confusing to have it in the summary (with the "Returns"). Thanks, /fuad > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel