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?
> >> 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)
> >
>
