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

[Igor Mammedov <imammedo@redhat.com>]

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.

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 <imammedo@redhat.com>
> > ---
> > 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