Date:   Tue, 14 Mar 2023 17:44:36 +0200
From:   Zhi Wang <zhi.wang.linux@gmail.com>
To:     Steven Price <steven.price@arm.com>
Cc:     kvm@vger.kernel.org, kvmarm@lists.linux.dev,
        Catalin Marinas <catalin.marinas@arm.com>,
        Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
        James Morse <james.morse@arm.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Joey Gouly <joey.gouly@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Christoffer Dall <christoffer.dall@arm.com>,
        Fuad Tabba <tabba@google.com>, linux-coco@lists.linux.dev
Subject: Re: [RFC PATCH 15/28] KVM: arm64: Handle realm MMIO emulation
Message-ID: <20230314174436.0000584d@gmail.com>
In-Reply-To: <e0354676-e8cf-6ea0-3229-a55d90259f8e@arm.com>
References: <20230127112248.136810-1-suzuki.poulose@arm.com>
        <20230127112932.38045-1-steven.price@arm.com>
        <20230127112932.38045-16-steven.price@arm.com>
        <20230306173751.000026d4@gmail.com>
        <e0354676-e8cf-6ea0-3229-a55d90259f8e@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 10 Mar 2023 15:47:14 +0000
Steven Price <steven.price@arm.com> wrote:

> On 06/03/2023 15:37, Zhi Wang wrote:
> > On Fri, 27 Jan 2023 11:29:19 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> MMIO emulation for a realm cannot be done directly with the VM's
> >> registers as they are protected from the host. However the RMM interface
> >> provides a structure member for providing the read/written value and  
> > 
> > More details would be better for helping the review. I can only see the
> > emulated mmio value from the device model (kvmtool or kvm_io_bus) is put into
> > the GPRS[0] of the RecEntry object. But the rest of the flow is missing.  
> 
> The commit message is out of date (sorry about that). A previous version
> of the spec had a dedicated member for the read/write value, but this
> was changed to just use GPRS[0] as you've spotted. I'll update the text.
> 
> > I guess RMM copies the value in the RecEntry.GPRS[0] to the target GPR in the
> > guest context in RMI_REC_ENTER when seeing RMI_EMULATED_MMIO. This is for
> > the guest MMIO read path.  
> 
> Yes, when entering the guest after an (emulatable) read data abort the
> value in GPRS[0] is loaded from the RecEntry structure into the
> appropriate register for the guest.
> 
> > How about the MMIO write path? I don't see where the RecExit.GPRS[0] is loaded
> > to a varible and returned to the userspace.  
> 

-----
> The RMM will populate GPRS[0] with the written value in this case (even
> if another register was actually used in the instruction). We then
> transfer that to the usual VCPU structure so that the normal fault
> handling logic works.
> 
-----

Are these in this patch or another patch?

> >> we can transfer this to the appropriate VCPU's register entry and then
> >> depend on the generic MMIO handling code in KVM.
> >>
> >> Signed-off-by: Steven Price <steven.price@arm.com>
> >> ---
> >>  arch/arm64/kvm/mmio.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> >> index 3dd38a151d2a..c4879fa3a8d3 100644
> >> --- a/arch/arm64/kvm/mmio.c
> >> +++ b/arch/arm64/kvm/mmio.c
> >> @@ -6,6 +6,7 @@
> >>  
> >>  #include <linux/kvm_host.h>
> >>  #include <asm/kvm_emulate.h>
> >> +#include <asm/rmi_smc.h>
> >>  #include <trace/events/kvm.h>
> >>  
> >>  #include "trace.h"
> >> @@ -109,6 +110,9 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
> >>  			       &data);
> >>  		data = vcpu_data_host_to_guest(vcpu, data, len);
> >>  		vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu), data);
> >> +
> >> +		if (vcpu_is_rec(vcpu))
> >> +			vcpu->arch.rec.run->entry.gprs[0] = data;  
> > 
> > I think the guest context is maintained by RMM (while KVM can only touch
> > Rec{Entry, Exit} object) so that guest context in the legacy VHE mode is
> > unused.
> > 
> > If yes, I guess here is should be:
> > 
> > if (unlikely(vcpu_is_rec(vcpu)))
> > 	vcpu->arch.rec.run->entry.gprs[0] = data;
> > else
> > 	vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu), data);  
> 
> Correct. Although there's no harm in updating with vcpu_set_reg(). But
> I'll make the change because it's clearer.
> 
> >>  	}
> >>  
> >>  	/*
> >> @@ -179,6 +183,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
> >>  	run->mmio.len		= len;
> >>  	vcpu->mmio_needed	= 1;
> >>  
> >> +	if (vcpu_is_rec(vcpu))
> >> +		vcpu->arch.rec.run->entry.flags |= RMI_EMULATED_MMIO;
> >> +  
> > 
> > Wouldn't it be better to set this in the kvm_handle_mmio_return where the MMIO
> > read emulation has been surely successful?  
> 
> Yes, that makes sense - I'll move this.
> 
> Thanks,
> 
> Steve
> 
> >>  	if (!ret) {
> >>  		/* We handled the access successfully in the kernel. */
> >>  		if (!is_write)  
> >   
>