Re: [Qemu-devel] [PATCH 17/21] introduce memory_region_get_address() and use it in kvm/ioapic

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 26 Apr 2013 17:35:41 +0000
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Fri, Apr 26, 2013 at 2:17 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 25 Apr 2013 18:37:19 +0000
> > Blue Swirl <blauwirbel@gmail.com> wrote:
> >
> >> On Tue, Apr 23, 2013 at 8:29 AM, Igor Mammedov <imammedo@redhat.com> 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