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;
MIME-Version: 1.0
References: <20230419221716.3603068-1-atishp@rivosinc.com> <20230419221716.3603068-11-atishp@rivosinc.com>
 <ZEFXiXu+0XLSdRkQ@google.com>
In-Reply-To: <ZEFXiXu+0XLSdRkQ@google.com>
From:   Atish Kumar Patra <atishp@rivosinc.com>
Date:   Sat, 22 Apr 2023 00:20:08 +0530
Message-ID: <CAHBxVyGGsvYrRpx1=ahW-5ALskAQJsgjF=9a=BMreuQov0En6Q@mail.gmail.com>
Subject: Re: [RFC 10/48] RISC-V: KVM: Implement static memory region measurement
To:     Sean Christopherson <seanjc@google.com>
Cc:     linux-kernel@vger.kernel.org, Alexandre Ghiti <alex@ghiti.fr>,
        Andrew Jones <ajones@ventanamicro.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Anup Patel <anup@brainfault.org>,
        Atish Patra <atishp@atishpatra.org>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@rivosinc.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
        linux-coco@lists.linux.dev, Dylan Reid <dylan@rivosinc.com>,
        abrestic@rivosinc.com, Samuel Ortiz <sameo@rivosinc.com>,
        Christoph Hellwig <hch@infradead.org>,
        Conor Dooley <conor.dooley@microchip.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Guo Ren <guoren@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
        Jiri Slaby <jirislaby@kernel.org>,
        kvm-riscv@lists.infradead.org, kvm@vger.kernel.org,
        linux-mm@kvack.org, linux-riscv@lists.infradead.org,
        Mayuresh Chitale <mchitale@ventanamicro.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Rajnesh Kanwal <rkanwal@rivosinc.com>,
        Uladzislau Rezki <urezki@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Apr 20, 2023 at 8:47=E2=80=AFPM Sean Christopherson <seanjc@google.=
com> wrote:
>
> On Wed, Apr 19, 2023, Atish Patra wrote:
> > +int kvm_riscv_cove_vm_measure_pages(struct kvm *kvm, struct kvm_riscv_=
cove_measure_region *mr)
> > +{
> > +     struct kvm_cove_tvm_context *tvmc =3D kvm->arch.tvmc;
> > +     int rc =3D 0, idx, num_pages;
> > +     struct kvm_riscv_cove_mem_region *conf;
> > +     struct page *pinned_page, *conf_page;
> > +     struct kvm_riscv_cove_page *cpage;
> > +
> > +     if (!tvmc)
> > +             return -EFAULT;
> > +
> > +     if (tvmc->finalized_done) {
> > +             kvm_err("measured_mr pages can not be added after finaliz=
e\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     num_pages =3D bytes_to_pages(mr->size);
> > +     conf =3D &tvmc->confidential_region;
> > +
> > +     if (!IS_ALIGNED(mr->userspace_addr, PAGE_SIZE) ||
> > +         !IS_ALIGNED(mr->gpa, PAGE_SIZE) || !mr->size ||
> > +         !cove_is_within_region(conf->gpa, conf->npages << PAGE_SHIFT,=
 mr->gpa, mr->size))
> > +             return -EINVAL;
> > +
> > +     idx =3D srcu_read_lock(&kvm->srcu);
> > +
> > +     /*TODO: Iterate one page at a time as pinning multiple pages fail=
 with unmapped panic
> > +      * with a virtual address range belonging to vmalloc region for s=
ome reason.
>
> I've no idea what code you had, but I suspect the fact that vmalloc'd mem=
ory isn't
> guaranteed to be physically contiguous is relevant to the panic.
>

Ahh. That makes sense. Thanks.

> > +      */
> > +     while (num_pages) {
> > +             if (signal_pending(current)) {
> > +                     rc =3D -ERESTARTSYS;
> > +                     break;
> > +             }
> > +
> > +             if (need_resched())
> > +                     cond_resched();
> > +
> > +             rc =3D get_user_pages_fast(mr->userspace_addr, 1, 0, &pin=
ned_page);
> > +             if (rc < 0) {
> > +                     kvm_err("Pinning the userpsace addr %lx failed\n"=
, mr->userspace_addr);
> > +                     break;
> > +             }
> > +
> > +             /* Enough pages are not available to be pinned */
> > +             if (rc !=3D 1) {
> > +                     rc =3D -ENOMEM;
> > +                     break;
> > +             }
> > +             conf_page =3D alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +             if (!conf_page) {
> > +                     rc =3D -ENOMEM;
> > +                     break;
> > +             }
> > +
> > +             rc =3D cove_convert_pages(page_to_phys(conf_page), 1, tru=
e);
> > +             if (rc)
> > +                     break;
> > +
> > +             /*TODO: Support other pages sizes */
> > +             rc =3D sbi_covh_add_measured_pages(tvmc->tvm_guest_id, pa=
ge_to_phys(pinned_page),
> > +                                              page_to_phys(conf_page),=
 SBI_COVE_PAGE_4K,
> > +                                              1, mr->gpa);
> > +             if (rc)
> > +                     break;
> > +
> > +             /* Unpin the page now */
> > +             put_page(pinned_page);
> > +
> > +             cpage =3D kmalloc(sizeof(*cpage), GFP_KERNEL_ACCOUNT);
> > +             if (!cpage) {
> > +                     rc =3D -ENOMEM;
> > +                     break;
> > +             }
> > +
> > +             cpage->page =3D conf_page;
> > +             cpage->npages =3D 1;
> > +             cpage->gpa =3D mr->gpa;
> > +             cpage->hva =3D mr->userspace_addr;
>
> Snapshotting the userspace address for the _source_ page can't possibly b=
e useful.
>

Yeah. Currently, the hva in the kvm_riscv_cove_page is not used
anywhere in the code. We can remove it.

> > +             cpage->is_mapped =3D true;
> > +             INIT_LIST_HEAD(&cpage->link);
> > +             list_add(&cpage->link, &tvmc->measured_pages);
> > +
> > +             mr->userspace_addr +=3D PAGE_SIZE;
> > +             mr->gpa +=3D PAGE_SIZE;
> > +             num_pages--;
> > +             conf_page =3D NULL;
> > +
> > +             continue;
> > +     }
> > +     srcu_read_unlock(&kvm->srcu, idx);
> > +
> > +     if (rc < 0) {
> > +             /* We don't to need unpin pages as it is allocated by the=
 hypervisor itself */
>
> This comment makes no sense.  The above code is doing all of the allocati=
on and
> pinning, which strongly suggests that KVM is the hypervisor.  But this co=
mment
> implies that KVM is not the hypervisor.
>

I mean to say here the conf_page is allocated in the kernel using
alloc_page. So no pinning/unpinning is required.
It seems the comment is probably misleading & confusing at best. I
will remove it.

> And "pinned_page" is cleared unpinned in the loop after the page is added=
+measured,
> which looks to be the same model as TDX where "pinned_page" is the source=
 and
> "conf_page" is gifted to the hypervisor.  But on failure, e.g. when alloc=
ating
> "conf_page", that reference is not put.
>

Thanks. Will fix it.

> > +             cove_delete_page_list(kvm, &tvmc->measured_pages, false);
> > +             /* Free the last allocated page for which conversion/meas=
urement failed */
> > +             kfree(conf_page);
>
> Assuming my guesses about how the architecture works are correct, this is=
 broken
> if sbi_covh_add_measured_pages() fails. The page has already been gifted =
to the

Yeah. The last conf_page should be reclaimed as well if measured_pages
fail at any point in the loop.
All other allocated ones would be reclaimed as a part of cove_delete_page_l=
ist.


> TSM by cove_convert_pages(), but there is no call to sbi_covh_tsm_reclaim=
_pages(),
> which I'm guessing is necesary to transition the page back to a state whe=
re it can
> be safely used by the host.