[PATCH] KVM: arm/arm64: avoid using kvm_run for in-kernel emulation
Andre Przywara
andre.przywara at arm.com
Fri Apr 10 06:17:16 PDT 2015
Hi Christoffer,
On 10/04/15 10:15, Christoffer Dall wrote:
> On Thu, Apr 09, 2015 at 04:55:45PM +0100, Andre Przywara wrote:
>> Hej Christoffer,
>>
>> On 09/04/15 14:30, Christoffer Dall wrote:
>>> On Sat, Mar 28, 2015 at 01:48:20AM +0000, Andre Przywara wrote:
>>>> Our in-kernel VGIC emulation still uses struct kvm_run briefly before
>>>> writing back the emulation result into the guest register. Using a
>>>> userspace mapped data structure within the kernel sounds dodgy, also
>>>> we do some extra copying at the moment at the end of the VGIC
>>>> emulation code.
>>>> Replace the usage of struct kvm_run in favour of passing separate
>>>> parameters into kvm_handle_mmio_return (and rename the function on
>>>> the way) to optimise the VGIC emulation. The real userland MMIO code
>>>> path does not change much.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>>>> ---
>>>> Hi,
>>>>
>>>> this is an optimization of the VGIC code totally removing struct
>>>> kvm_run from the VGIC emulation. In my eyes it provides a nice
>>>> cleanup and is a logical consequence of the kvm_io_bus patches (on
>>>> which it goes on top). On the other hand it is optional and I didn't
>>>> want to merge it with the already quite large last patch 11.
>>>> Marc, I leave it up to you whether you take this as part of the
>>>> kvm_io_bus series or not.
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>> arch/arm/include/asm/kvm_mmio.h | 3 +-
>>>> arch/arm/kvm/arm.c | 6 ++--
>>>> arch/arm/kvm/mmio.c | 55 ++++++++++++++++++-------------------
>>>> arch/arm64/include/asm/kvm_mmio.h | 3 +-
>>>> virt/kvm/arm/vgic.c | 8 ++----
>>>> 5 files changed, 37 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
>>>> index d8e90c8..53461a6 100644
>>>> --- a/arch/arm/include/asm/kvm_mmio.h
>>>> +++ b/arch/arm/include/asm/kvm_mmio.h
>>>> @@ -28,7 +28,8 @@ struct kvm_decode {
>>>> bool sign_extend;
>>>> };
>>>>
>>>> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>>> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, unsigned int len,
>>>> + void *val, gpa_t phys_addr);
>>>> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> phys_addr_t fault_ipa);
>>>>
>>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>>> index e98370c..b837aef 100644
>>>> --- a/arch/arm/kvm/arm.c
>>>> +++ b/arch/arm/kvm/arm.c
>>>> @@ -506,8 +506,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - if (run->exit_reason == KVM_EXIT_MMIO) {
>>>> - ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>>>> + if (run->exit_reason == KVM_EXIT_MMIO && !run->mmio.is_write) {
>>>> + ret = kvm_writeback_mmio_data(vcpu, run->mmio.len,
>>>> + run->mmio.data,
>>>> + run->mmio.phys_addr);
>>>> if (ret)
>>>> return ret;
>>>> }
>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>> index 974b1c6..3c57f96 100644
>>>> --- a/arch/arm/kvm/mmio.c
>>>> +++ b/arch/arm/kvm/mmio.c
>>>> @@ -86,38 +86,36 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
>>>> }
>>>>
>>>> /**
>>>> - * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
>>>> - * @vcpu: The VCPU pointer
>>>> - * @run: The VCPU run struct containing the mmio data
>>>> + * kvm_writeback_mmio_data -- Handle MMIO loads after user space emulation
>>>> + * @vcpu: The VCPU pointer
>>>> + * @len: The length in Bytes of the MMIO access
>>>> + * @data_ptr: Pointer to the data to be written back into the guest
>>>> + * @phys_addr: Physical address of the originating MMIO access
>>>> *
>>>> * This should only be called after returning from userspace for MMIO load
>>>> - * emulation.
>>>> + * emulation. phys_addr is only used for the tracepoint output.
>>>> */
>>>> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, unsigned int len,
>>>> + void *data_ptr, gpa_t phys_addr)
>>>> {
>>>> unsigned long data;
>>>> - unsigned int len;
>>>> int mask;
>>>>
>>>> - if (!run->mmio.is_write) {
>>>> - len = run->mmio.len;
>>>> - if (len > sizeof(unsigned long))
>>>> - return -EINVAL;
>>>> + if (len > sizeof(unsigned long))
>>>> + return -EINVAL;
>>>>
>>>> - data = mmio_read_buf(run->mmio.data, len);
>>>> + data = mmio_read_buf(data_ptr, len);
>>>>
>>>> - if (vcpu->arch.mmio_decode.sign_extend &&
>>>> - len < sizeof(unsigned long)) {
>>>> - mask = 1U << ((len * 8) - 1);
>>>> - data = (data ^ mask) - mask;
>>>> - }
>>>> -
>>>> - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
>>>> - data);
>>>> - data = vcpu_data_host_to_guest(vcpu, data, len);
>>>> - *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data;
>>>> + if (vcpu->arch.mmio_decode.sign_extend &&
>>>> + len < sizeof(unsigned long)) {
>>>> + mask = 1U << ((len * 8) - 1);
>>>> + data = (data ^ mask) - mask;
>>>> }
>>>>
>>>> + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, phys_addr, data);
>>>> + data = vcpu_data_host_to_guest(vcpu, data, len);
>>>> + *vcpu_reg(vcpu, vcpu->arch.mmio_decode.rt) = data;
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -201,18 +199,19 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> data_buf);
>>>> }
>>>>
>>>> - /* Now prepare kvm_run for the potential return to userland. */
>>>> - run->mmio.is_write = is_write;
>>>> - run->mmio.phys_addr = fault_ipa;
>>>> - run->mmio.len = len;
>>>> - memcpy(run->mmio.data, data_buf, len);
>>>> -
>>>> if (!ret) {
>>>> /* We handled the access successfully in the kernel. */
>>>> - kvm_handle_mmio_return(vcpu, run);
>>>> + if (!is_write)
>>>> + kvm_writeback_mmio_data(vcpu, len, data_buf, fault_ipa);
>>>> return 1;
>>>> }
>>>>
>>>> + /* Now prepare the kvm_run structure for the return to userland. */
>>>> + run->mmio.phys_addr = fault_ipa;
>>>> + run->mmio.len = len;
>>>> + run->mmio.is_write = is_write;
>>>> + memcpy(run->mmio.data, data_buf, len);
>>>> run->exit_reason = KVM_EXIT_MMIO;
>>>> +
>>>> return 0;
>>>> }
>>>> diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
>>>> index 889c908..3b83475 100644
>>>> --- a/arch/arm64/include/asm/kvm_mmio.h
>>>> +++ b/arch/arm64/include/asm/kvm_mmio.h
>>>> @@ -31,7 +31,8 @@ struct kvm_decode {
>>>> bool sign_extend;
>>>> };
>>>>
>>>> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>>> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, unsigned int len,
>>>> + void *val, gpa_t phys_addr);
>>>> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> phys_addr_t fault_ipa);
>>>>
>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>> index b70174e..4047fc0 100644
>>>> --- a/virt/kvm/arm/vgic.c
>>>> +++ b/virt/kvm/arm/vgic.c
>>>> @@ -803,7 +803,6 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>>>> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> struct vgic_io_device *iodev = container_of(this,
>>>> struct vgic_io_device, dev);
>>>> - struct kvm_run *run = vcpu->run;
>>>> const struct vgic_io_range *range;
>>>> struct kvm_exit_mmio mmio;
>>>> bool updated_state;
>>>> @@ -832,12 +831,9 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>>>> updated_state = false;
>>>> }
>>>> spin_unlock(&dist->lock);
>>>> - run->mmio.is_write = is_write;
>>>> - run->mmio.len = len;
>>>> - run->mmio.phys_addr = addr;
>>>> - memcpy(run->mmio.data, val, len);
>>>>
>>>> - kvm_handle_mmio_return(vcpu, run);
>>>> + if (!is_write)
>>>> + kvm_writeback_mmio_data(vcpu, len, val, addr);
>>>
>>> why do we do a kvm_writeback_mmio_data() call here in addition to
>>> io_mem_abort() ? Are we not doing this twice? What am I missing?
>>
>> You are missing: ... nothing, we are indeed doing it twice. This looks
>> like an oversight when converting from using a data buffer to a pointer.
>>
>> So we can drop this here, and io_mem_abort() takes care (tested
>> quickly). Shall I add a comment or just remove those two lines?
>>
>> Are you OK with the rest of the patch?
>>
> Roughly ok, I think it would be nicer to not rename
> kvm_handle_mmio_return() but call that as is, and then introduce
> kvm_writeback_mmio_data() as a static function called only within mmio.c
> and do the check for !run->mmio.is_write in the kvm_handle_mmio_return()
> function before calling kvm_writeback_mmio_data().
>
> That would reduce the impact of this patch and keep the run-loop code
> slightly less cluttered.
Very neat! Thanks for that suggestion! I think that didn't work before
because of the former usage in vgic, but now it is indeed much nicer
this way.
Will send out the patch ASAP.
Cheers,
Andre.
More information about the linux-arm-kernel
mailing list