From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVmjJ-0004hh-KR for qemu-devel@nongnu.org; Fri, 26 Apr 2013 13:46:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVmjI-0000sl-1P for qemu-devel@nongnu.org; Fri, 26 Apr 2013 13:46:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52817) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVmjH-0000sV-MG for qemu-devel@nongnu.org; Fri, 26 Apr 2013 13:46:39 -0400 Date: Fri, 26 Apr 2013 19:46:16 +0200 From: Igor Mammedov Message-ID: <20130426194616.50534c82@thinkpad> In-Reply-To: References: <1366705795-24732-1-git-send-email-imammedo@redhat.com> <1366705795-24732-18-git-send-email-imammedo@redhat.com> <20130426161754.583e3f6b@thinkpad> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 17/21] introduce memory_region_get_address() and use it in kvm/ioapic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl , jan.kiszka@siemens.com, pbonzini@redhat.com Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, ehabkost@redhat.com, gleb@redhat.com, mst@redhat.com, armbru@redhat.com, quintela@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, aderumier@odiso.com, lcapitulino@redhat.com, anthony.perard@citrix.com, alex.williamson@redhat.com, kraxel@redhat.com, yang.z.zhang@intel.com, afaerber@suse.de, stefano.stabellini@eu.citrix.com, rth@twiddle.net On Fri, 26 Apr 2013 17:35:41 +0000 Blue Swirl wrote: > On Fri, Apr 26, 2013 at 2:17 PM, Igor Mammedov wrote: > > On Thu, 25 Apr 2013 18:37:19 +0000 > > Blue Swirl wrote: > > > >> On Tue, Apr 23, 2013 at 8:29 AM, Igor Mammedov wrote: > >> > kvm/ioapic is relying on the fact that SysBus device > >> > maps mmio regions with offset counted from start of system memory. > >> > But if ioapic's region is moved to another sub-region which doesn't > >> > start at the beginning of system memory then using offset isn't correct. > >> > >> But base_address in only initialized once, never changed. Does this > >> patch matter now? > > this code path is used on only at machine start-up but also on resets and > > post migration to initialize IO-APIC. And unfortunately KVM API takes not > > only base address but a bunch of other parameters in that ioctl, so splitting > > it doesn't look feasible for 1.5. Patch is useful in aspect that it hides > > direct access to parent's internals and without dangling SysBusDevice > > internals it allows to convert kvm/ioapic to ICCDevice [next patch in series], > > leaving code a bit cleaner. > > But as the address can't be changed (yet), the entire patch could be simply: > - kioapic->base_address = s->busdev.mmio[0].addr; > + kioapic->base_address = IO_APIC_DEFAULT_ADDRESS; It's a bit fragile, but that for sure simpler and can work. Jan, Paolo, Are you ok with this approach? > > Later, when it's possible to change the address via PIIX3 registers, > we can adjust the base and pass that properly to kioapic and on to > KVM. > > Resolving the base address every time when kvm_ioapic_put() is called > is also less efficient, assuming of course that the base address > changes less often than the KVM ioctl is used. > > > > > BTW: > > there is an improved patch: > > http://permalink.gmane.org/gmane.comp.emulators.qemu/208512 > > > > that instead of introducing a new function improves current > > memory_region_find() API. > > > >> The correct solution would be to track changes to APICBASE register at > >> PIIX3 chipset level and adjust mapping and KVM base also from there. > > That we would probably need a change in KVM API first to allow set > > IO-APIC's base separately. > > -- Regards, Igor