Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933508AbaKMPbf (ORCPT ); Thu, 13 Nov 2014 10:31:35 -0500 Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:37990 "EHLO cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753972AbaKMPbe (ORCPT ); Thu, 13 Nov 2014 10:31:34 -0500 Message-ID: <5464CEB4.8010607@arm.com> Date: Thu, 13 Nov 2014 15:31:00 +0000 From: Andre Przywara Organization: ARM Ltd. User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Nikolay Nikolaev , Eric Auger CC: Christoffer Dall , Russell King , "open list:KERNEL VIRTUAL MA..." , open list , Gleb Natapov , Paolo Bonzini , VirtualOpenSystems Technical Team , "kvmarm@lists.cs.columbia.edu" Subject: Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus References: <1394726249-1547-1-git-send-email-a.motakis@virtualopensystems.com> <1394726249-1547-2-git-send-email-a.motakis@virtualopensystems.com> <20140328190913.GJ25519@cbox> <20141110162736.GA16054@cbox> <5464BD30.7040702@linaro.org> <5464BEC9.1050401@linaro.org> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 13/11/14 15:02, Nikolay Nikolaev wrote: > On Thu, Nov 13, 2014 at 4:23 PM, Eric Auger wrote: >> On 11/13/2014 03:16 PM, Eric Auger wrote: >>> On 11/13/2014 11:45 AM, Nikolay Nikolaev wrote: >>>> On Mon, Nov 10, 2014 at 6:27 PM, Christoffer Dall >>>> wrote: >>>>> On Mon, Nov 10, 2014 at 05:09:07PM +0200, Nikolay Nikolaev wrote: >>>>>> Hello, >>>>>> >>>>>> On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall >>>>>> wrote: >>>>>>> >>>>>>> On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote: >>>>>>>> On an unhandled IO memory abort, use the kvm_io_bus_* API in order to >>>>>>>> handle the MMIO access through any registered read/write callbacks. This >>>>>>>> is a dependency for eventfd support (ioeventfd and irqfd). >>>>>>>> >>>>>>>> However, accesses to the VGIC are still left implemented independently, >>>>>>>> since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access. >>>>>>>> >>>>>>>> Signed-off-by: Antonios Motakis >>>>>>>> Signed-off-by: Nikolay Nikolaev >>>>>>>> --- >>>>>>>> arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++ >>>>>>>> virt/kvm/arm/vgic.c | 5 ++++- >>>>>>>> 2 files changed, 36 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c >>>>>>>> index 4cb5a93..1d17831 100644 >>>>>>>> --- a/arch/arm/kvm/mmio.c >>>>>>>> +++ b/arch/arm/kvm/mmio.c >>>>>>>> @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> +/** >>>>>>>> + * kvm_handle_mmio - handle an in-kernel MMIO access >>>>>>>> + * @vcpu: pointer to the vcpu performing the access >>>>>>>> + * @run: pointer to the kvm_run structure >>>>>>>> + * @mmio: pointer to the data describing the access >>>>>>>> + * >>>>>>>> + * returns true if the MMIO access has been performed in kernel space, >>>>>>>> + * and false if it needs to be emulated in user space. >>>>>>>> + */ >>>>>>>> +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, >>>>>>>> + struct kvm_exit_mmio *mmio) >>>>>>>> +{ >>>>>>>> + int ret; >>>>>>>> + if (mmio->is_write) { >>>>>>>> + ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr, >>>>>>>> + mmio->len, &mmio->data); >>>>>>>> + >>>>>>>> + } else { >>>>>>>> + ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr, >>>>>>>> + mmio->len, &mmio->data); >>>>>>>> + } >>>>>>>> + if (!ret) { >>>>>>>> + kvm_prepare_mmio(run, mmio); >>>>>>>> + kvm_handle_mmio_return(vcpu, run); >>>>>>>> + } >>>>>>>> + >>>>>>>> + return !ret; >>>>>>>> +} >>>>>>>> + >>>>>>>> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, >>>>>>>> phys_addr_t fault_ipa) >>>>>>>> { >>>>>>>> @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, >>>>>>>> if (vgic_handle_mmio(vcpu, run, &mmio)) >>>>>>>> return 1; >>>>>>>> >>>>>>>> + if (handle_kernel_mmio(vcpu, run, &mmio)) >>>>>>>> + return 1; >>>>>>>> + >>>>>> >>>>>> >>>>>> We're reconsidering ioeventfds patchseries and we tried to evaluate >>>>>> what you suggested here. >>>>>> >>>>>>> >>>>>>> this special-casing of the vgic is now really terrible. Is there >>>>>>> anything holding you back from doing the necessary restructure of the >>>>>>> kvm_bus_io_*() API instead? >>>>>> >>>>>> Restructuring the kvm_io_bus_ API is not a big thing (we actually did >>>>>> it), but is not directly related to the these patches. >>>>>> Of course it can be justified if we do it in the context of removing >>>>>> vgic_handle_mmio and leaving only handle_kernel_mmio. >>>>>> >>>>>>> >>>>>>> That would allow us to get rid of the ugly >>>>>>> Fix it! in the vgic driver as well. >>>>>> >>>>>> Going through the vgic_handle_mmio we see that it will require large >>>>>> refactoring: >>>>>> - there are 15 MMIO ranges for the vgic now - each should be >>>>>> registered as a separate device >> Re-correcting Andre's address, sorry: >> Hi Nikolay, Andre, >> >> what does mandate to register 15 devices? Isn't possible to register a >> single kvm_io_device covering the whole distributor range [base, base + >> KVM_VGIC_V2_DIST_SIZE] (current code) and in associated >> kvm_io_device_ops read/write locate the addressed range and do the same >> as what is done in current vgic_handle_mmio? Isn't it done that way for > > Well, then we'll actually get slower mmio processing. Instead of calling > vgic_handle_mmio in io_mem_abort, we'll be calling kvm_io_bus_write. > This just adds another level of translation (i.e. find the kvm_io_ device) > and the underlying vgic code will remain almost the same. Agreed. That was one possibility I came around also, but I think it defeats the purpose of the rework, which is mostly to get rid of the GIC's private MMIO dispatching code, right? But honestly I would happily sacrifice "performance" for easier VGIC code - especially if one thinks about security for instance. Though I think that another memory reference doesn't really matter in this context ;-) >> the ioapic? what do I miss? > I looked quickly in the ioapic code, and if I get it right there are no "ranges' > like what we have with the GIC. They have this regselect/regwindow concept > and they seem to have much less "registers" to handle. GIC seems a lot more > complex in terms of MMIO interface. Right, that was my impression, too. IOAPIC isn't really comparable to the GIC in this respect. That's why I was going away from this rework, since I thought that the kvm_io_bus API wasn't really meant for such beasts as the GIC. Cheers, Andre. > > regards, > Nikolay Nikolaev > >> >> Thanks >> >> Best Regards >> >> Eric >>>>>> - the handler of each range should be split into read and write >>>>>> - all handlers take 'struct kvm_exit_mmio', and pass it to >>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read' >>>>>> >>>>>> To sum up - if we do this refactoring of vgic's MMIO handling + >>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner >>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities. >>>>>> >>>>>> We have 3 questions: >>>>>> - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other >>>>>> architectures too? >>>>>> - is this huge vgic MMIO handling redesign acceptable/desired (it >>>>>> touches a lot of code)? >>>>>> - is there a way that ioeventfd is accepted leaving vgic.c in it's >>>>>> current state? >>>>>> >>>>> Not sure how the latter question is relevant to this, but check with >>>>> Andre who recently looked at this as well and decided that for GICv3 the >>>>> only sane thing was to remove that comment for the gic. >>>> @Andre - what's your experience with the GICv3 and MMIO handling, >>>> anything specific? >>>>> >>>>> I don't recall the details of what you were trying to accomplish here >>>>> (it's been 8 months or so) but the surely the vgic handling code should >>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo >>>>> suggested), unless you come back and tell me that that would involve a >>>>> complete rewrite of the vgic code. >>>> I'm experimenting now - it's not exactly rewrite of whole vgic code, >>>> but it will touch a lot of it - all MMIO access handlers and the >>>> supporting functions. >>>> We're ready to spend the effort. My question is - is this acceptable? >>>> >>>> regards, >>>> Nikolay Nikolaev >>>> Virtual Open Systems >>>>> >>>>> -Christoffer >>>> _______________________________________________ >>>> kvmarm mailing list >>>> kvmarm@lists.cs.columbia.edu >>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/