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

[Blue Swirl]

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

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.
>
>>
>> >
>> > Fix kvm/ioapic by providing and using helper function that returns
>> > absolute region address in respective address space.
>> >
>> > Signed-off-by: Igor Mammedov <address@hidden>
>> > ---
>> > Note:
>> >   next patch "move IOAPIC to ICC bus" converts IOAPICs to ICCDevice
>> >   and breaks SysBus device assumption used by kvm/ioapic.
>> > ---
>> >  hw/i386/kvm/ioapic.c  |    2 +-
>> >  include/exec/memory.h |   10 ++++++++++
>> >  memory.c              |   11 +++++++++++
>> >  3 files changed, 22 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
>> > index a3bd519..b80d41a 100644
>> > --- a/hw/i386/kvm/ioapic.c
>> > +++ b/hw/i386/kvm/ioapic.c
>> > @@ -96,7 +96,7 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
>> >
>> >      kioapic->id = s->id;
>> >      kioapic->ioregsel = s->ioregsel;
>> > -    kioapic->base_address = s->busdev.mmio[0].addr;
>> > +    kioapic->base_address = memory_region_get_address(&s->io_memory);
>> >      kioapic->irr = s->irr;
>> >      for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>> >          kioapic->redirtbl[i].bits = s->ioredtbl[i];
>> > diff --git a/include/exec/memory.h b/include/exec/memory.h
>> > index 9e88320..954f353 100644
>> > --- a/include/exec/memory.h
>> > +++ b/include/exec/memory.h
>> > @@ -706,6 +706,16 @@ void memory_region_set_enabled(MemoryRegion *mr, bool 
>> > enabled);
>> >  void memory_region_set_address(MemoryRegion *mr, hwaddr addr);
>> >
>> >  /*
>> > + * memory_region_get_address: get current the address of a region
>> > + *
>> > + * Returns the absolute address of a region.
>> > + * May be used on regions that are currently part of a memory hierarchy.
>> > + *
>> > + * @mr: the region being queried
>> > + */
>> > +hwaddr memory_region_get_address(MemoryRegion *mr);
>> > +
>> > +/*
>> >   * memory_region_set_alias_offset: dynamically update a memory alias's 
>> > offset
>> >   *
>> >   * Dynamically updates the offset into the target region that an alias 
>> > points
>> > diff --git a/memory.c b/memory.c
>> > index 75ca281..0651050 100644
>> > --- a/memory.c
>> > +++ b/memory.c
>> > @@ -1413,6 +1413,17 @@ void memory_region_set_address(MemoryRegion *mr, 
>> > hwaddr addr)
>> >      memory_region_transaction_commit();
>> >  }
>> >
>> > +hwaddr memory_region_get_address(MemoryRegion *mr)
>> > +{
>> > +    hwaddr addr = mr->addr;
>> > +
>> > +    while (mr->parent) {
>> > +        mr = mr->parent;
>> > +        addr += mr->addr;
>> > +    }
>> > +    return addr;
>> > +}
>> > +
>> >  void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset)
>> >  {
>> >      assert(mr->alias);
>> > --
>> > 1.7.1
>> >
>
>
> --
> Regards,
>   Igor