From: andre.przywara@arm.com (Andre Przywara)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 08/12] KVM: arm/arm64: implement kvm_io_bus MMIO handling for the VGIC
Date: Fri, 20 Mar 2015 12:40:02 +0000 [thread overview]
Message-ID: <550C1522.80105@arm.com> (raw)
In-Reply-To: <550AEEF3.80702@arm.com>
On 03/19/2015 03:44 PM, Andre Przywara wrote:
> Hej Christoffer,
>
[ ... ]
>>> +static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
>>> + struct kvm_io_device *this, gpa_t addr,
>>> + int len, void *val, bool is_write)
>>> +{
>>> + 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;
>>> + gpa_t offset;
>>> +
>>> + offset = addr - iodev->addr;
>>> + range = vgic_find_range(iodev->reg_ranges, len, offset);
>>> + if (unlikely(!range || !range->handle_mmio)) {
>>> + pr_warn("Unhandled access %d %08llx %d\n", is_write, addr, len);
>>> + return -ENXIO;
>>> + }
>>> +
>>> + mmio.phys_addr = addr;
>>> + mmio.len = len;
>>> + mmio.is_write = is_write;
>>> + if (is_write)
>>> + memcpy(mmio.data, val, len);
>>> + mmio.private = iodev->redist_vcpu;
>>> +
>>> + spin_lock(&dist->lock);
>>> + offset -= range->base;
>>> + if (vgic_validate_access(dist, range, offset)) {
>>> + updated_state = call_range_handler(vcpu, &mmio, offset, range);
>>> + if (!is_write)
>>> + memcpy(val, mmio.data, len);
>>> + } else {
>>> + if (!is_write)
>>> + memset(val, 0, len);
>>> + updated_state = false;
>>> + }
>>> + spin_unlock(&dist->lock);
>>> + kvm_prepare_mmio(run, &mmio);
>>
>> we're not the only user of kvm_exit_mmio I believe, so we could rename
>
> (assuming you mean we _are_ the only user here, which I can acknowledge)
>
>> this to vgic_io as well and you could change the mmio.data array to be a
>> void *val pointer, which just gets set to the pointer passed into this
>> function (which I think points to the kvm_run structs data array) and
>> you can avoid all these memcopies, right?
>
> That sounds indeed tempting, but the comment on the struct kvm_exit_mmio
> declaration reads:
> /*
> * The in-kernel MMIO emulation code wants to use a copy of run->mmio,
> * which is an anonymous type. Use our own type instead.
> */
> How I understand this the structure was introduced to _not_ use the same
> memory, but use a copy instead. Do you remember any reason for this? And
> in how far is this type anonymous? It's even in an uapi header.
>
> Briefly looking at the code we do quite some memcpy on the way.
> I am about to go all the way down into that ARM MMIO handling cave now
> to check this (Marc, if I am not showing up again after some hours,
> please come and rescue me ;-)
So, I feel that there is quite some unneeded copying and masking on the
way, but a real fix would be quite invasive and needs quite some testing
and review. I don't feel like rushing this into a v2 of this series.
I quickly did what you proposed (replacing memcpy by pointer
assignment), and that seems to work, but I don't have many chances of
testing this this weekend, since I am on the road. Also I have to dig
out my cross-endian test scripts first. So not sure if you want to take
the risk with this series.
I changed the other minor points you mentioned in the review though, so
do you want to have a "v1.5" or how do we proceed from here?
Cheers,
Andre.
next prev parent reply other threads:[~2015-03-20 12:40 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-13 16:10 [PATCH 00/12] KVM: arm/arm64: move VGIC MMIO to kvm_io_bus Andre Przywara
2015-03-13 16:10 ` [PATCH 01/12] KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks Andre Przywara
2015-03-14 14:43 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 02/12] KVM: move iodev.h from virt/kvm/ to include/kvm Andre Przywara
2015-03-14 14:43 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 03/12] KVM: arm/arm64: remove now unneeded include directory from Makefile Andre Przywara
2015-03-14 14:43 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 04/12] KVM: x86: " Andre Przywara
2015-03-14 13:57 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 05/12] KVM: arm/arm64: rename struct kvm_mmio_range to vgic_io_range Andre Przywara
2015-03-14 14:43 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 06/12] KVM: mark kvm->buses as empty once they were destroyed Andre Przywara
2015-03-14 14:43 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 07/12] KVM: arm/arm64: simplify vgic_find_range() and callers Andre Przywara
2015-03-14 14:44 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 08/12] KVM: arm/arm64: implement kvm_io_bus MMIO handling for the VGIC Andre Przywara
2015-03-14 14:27 ` Christoffer Dall
2015-03-19 15:44 ` Andre Przywara
2015-03-20 12:40 ` Andre Przywara [this message]
2015-03-20 14:25 ` Christoffer Dall
2015-03-20 14:24 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 09/12] KVM: arm/arm64: prepare GICv2 emulation to be handled by kvm_io_bus Andre Przywara
2015-03-14 14:30 ` Christoffer Dall
2015-03-17 18:02 ` Andre Przywara
2015-03-17 18:51 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 10/12] KVM: arm/arm64: prepare GICv3 emulation to use kvm_io_bus MMIO handling Andre Przywara
2015-03-14 14:39 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 11/12] KVM: ARM: on IO mem abort - route the call to KVM MMIO bus Andre Przywara
2015-03-14 14:43 ` Christoffer Dall
2015-03-13 16:10 ` [PATCH 12/12] KVM: arm/arm64: remove now obsolete VGIC specific MMIO handling code Andre Przywara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=550C1522.80105@arm.com \
--to=andre.przywara@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).