Re: [RFC PATCH 15/28] KVM: arm64: Handle realm MMIO emulation

[Steven Price]

On 14/03/2023 15:44, Zhi Wang wrote:
> On Fri, 10 Mar 2023 15:47:14 +0000
> Steven Price <steven.price@xxxxxxx> wrote:
> 
>> On 06/03/2023 15:37, Zhi Wang wrote:
>>> On Fri, 27 Jan 2023 11:29:19 +0000
>>> Steven Price <steven.price@xxxxxxx> 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?

The RMM (not included in this particular series[1]) sets the first
element of the 'GPRS' array which is in memory shared with the host.

The Linux half to populate the vcpu structure is in the previous patch:

+static int rec_exit_sync_dabt(struct kvm_vcpu *vcpu)
+{
+	struct rec *rec = &vcpu->arch.rec;
+
+	if (kvm_vcpu_dabt_iswrite(vcpu) && kvm_vcpu_dabt_isvalid(vcpu))
+		vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu),
+			     rec->run->exit.gprs[0]);
+
+	return kvm_handle_guest_abort(vcpu);
+}

I guess it might make sense to pull the 'if' statement out of the
previous patch and into this one to keep all the MMIO code together.

Steve

[1] This Linux code is written against the RMM specification and in
theory could work with any RMM implementation. But the TF RMM is open
source, so I can point you at the assignment in the latest commit here:
https://git.trustedfirmware.org/TF-RMM/tf-rmm.git/tree/runtime/core/exit.c?id=d294b1b05e8d234d32684a982552aa2a17fb9157#n264

>>>> 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@xxxxxxx>
>>>> ---
>>>>  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)  
>>>   
>>
>