Patchwork [0.14?,3/4] ioapic: Prepare for base address relocation

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 3, 2011, 2:55 p.m.
Message ID <0072079efad1c31da849cff7ad2cb426aeb6c29f.1296744934.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/81671/
State New
Headers show

Comments

Jan Kiszka - Feb. 3, 2011, 2:55 p.m.
The registers of real IOAPICs can be relocated during runtime (via
chipset registers). We don't support this yet, but qemu-kvm carries the
current base address in its version 2 vmstate.

To align both implementations for migratability, add the proper
infrastructure to accept initial as well as updated base addresses and
include the current address in the vmstate. This is done in a way that
will also allow multiple IOAPICs in the future.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioapic.c  |   17 +++++++++++++++++
 hw/ioapic.h  |    4 ++++
 hw/pc_piix.c |    5 ++---
 3 files changed, 23 insertions(+), 3 deletions(-)
Blue Swirl - Feb. 3, 2011, 5:03 p.m.
On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> The registers of real IOAPICs can be relocated during runtime (via
> chipset registers). We don't support this yet, but qemu-kvm carries the
> current base address in its version 2 vmstate.
>
> To align both implementations for migratability, add the proper
> infrastructure to accept initial as well as updated base addresses and
> include the current address in the vmstate. This is done in a way that
> will also allow multiple IOAPICs in the future.

Nack, the addresses should be device properties.
Jan Kiszka - Feb. 3, 2011, 5:18 p.m.
On 2011-02-03 18:03, Blue Swirl wrote:
> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> The registers of real IOAPICs can be relocated during runtime (via
>> chipset registers). We don't support this yet, but qemu-kvm carries the
>> current base address in its version 2 vmstate.
>>
>> To align both implementations for migratability, add the proper
>> infrastructure to accept initial as well as updated base addresses and
>> include the current address in the vmstate. This is done in a way that
>> will also allow multiple IOAPICs in the future.
> 
> Nack, the addresses should be device properties.

Hmm.... we could make default_base_address a property. Will change that.
But current_base_address is just the same as apicbase and can't be a
property.

Jan
Blue Swirl - Feb. 3, 2011, 5:36 p.m.
On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-02-03 18:03, Blue Swirl wrote:
>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> The registers of real IOAPICs can be relocated during runtime (via
>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>> current base address in its version 2 vmstate.
>>>
>>> To align both implementations for migratability, add the proper
>>> infrastructure to accept initial as well as updated base addresses and
>>> include the current address in the vmstate. This is done in a way that
>>> will also allow multiple IOAPICs in the future.
>>
>> Nack, the addresses should be device properties.
>
> Hmm.... we could make default_base_address a property. Will change that.
> But current_base_address is just the same as apicbase and can't be a
> property.

Oh, right. What will current_base_address used for? Why can't board
just unmap IOAPIC from current address and remap it at the new
address? Then the device would not need to know its base address.
Jan Kiszka - Feb. 3, 2011, 5:43 p.m.
On 2011-02-03 18:36, Blue Swirl wrote:
> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-02-03 18:03, Blue Swirl wrote:
>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>> current base address in its version 2 vmstate.
>>>>
>>>> To align both implementations for migratability, add the proper
>>>> infrastructure to accept initial as well as updated base addresses and
>>>> include the current address in the vmstate. This is done in a way that
>>>> will also allow multiple IOAPICs in the future.
>>>
>>> Nack, the addresses should be device properties.
>>
>> Hmm.... we could make default_base_address a property. Will change that.
>> But current_base_address is just the same as apicbase and can't be a
>> property.
> 
> Oh, right. What will current_base_address used for? Why can't board
> just unmap IOAPIC from current address and remap it at the new
> address? Then the device would not need to know its base address.

The board could do this. The question is where we put this service, in
the context if the IOAPIC as ioapic_set_base_address (compare to
cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
each and every board code. In the latter case, the boards would also be
responsible for saving/restoring the address.

Well, unless we have a good reason, I would prefer to keep the split as
suggested, also to avoid that qemu-kvm needs to break its vmstate.

Jan

PS: Another bug in the APIC emulation: cpu_set_apic_base lacks a call to
sysbus_mmio_map to update its mapping.
Blue Swirl - Feb. 3, 2011, 5:54 p.m.
On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-02-03 18:36, Blue Swirl wrote:
>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>> current base address in its version 2 vmstate.
>>>>>
>>>>> To align both implementations for migratability, add the proper
>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>> include the current address in the vmstate. This is done in a way that
>>>>> will also allow multiple IOAPICs in the future.
>>>>
>>>> Nack, the addresses should be device properties.
>>>
>>> Hmm.... we could make default_base_address a property. Will change that.
>>> But current_base_address is just the same as apicbase and can't be a
>>> property.
>>
>> Oh, right. What will current_base_address used for? Why can't board
>> just unmap IOAPIC from current address and remap it at the new
>> address? Then the device would not need to know its base address.
>
> The board could do this. The question is where we put this service, in
> the context if the IOAPIC as ioapic_set_base_address (compare to
> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
> each and every board code. In the latter case, the boards would also be
> responsible for saving/restoring the address.

How is the device relocated? Where are the chipset registers you mention?
Jan Kiszka - Feb. 3, 2011, 6:01 p.m.
On 2011-02-03 18:54, Blue Swirl wrote:
> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-02-03 18:36, Blue Swirl wrote:
>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>> current base address in its version 2 vmstate.
>>>>>>
>>>>>> To align both implementations for migratability, add the proper
>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>> will also allow multiple IOAPICs in the future.
>>>>>
>>>>> Nack, the addresses should be device properties.
>>>>
>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>> But current_base_address is just the same as apicbase and can't be a
>>>> property.
>>>
>>> Oh, right. What will current_base_address used for? Why can't board
>>> just unmap IOAPIC from current address and remap it at the new
>>> address? Then the device would not need to know its base address.
>>
>> The board could do this. The question is where we put this service, in
>> the context if the IOAPIC as ioapic_set_base_address (compare to
>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>> each and every board code. In the latter case, the boards would also be
>> responsible for saving/restoring the address.
> 
> How is the device relocated? Where are the chipset registers you mention?

Intel's PIIX chipsets contain a register called APICBASE (but it means
the IOAPIC), and that defines the location. The analogy in the APIC
world is the MSR_IA32_APICBASE which we maintain via the APIC state.

Jan
Blue Swirl - Feb. 3, 2011, 7:01 p.m.
On Thu, Feb 3, 2011 at 6:01 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-02-03 18:54, Blue Swirl wrote:
>> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2011-02-03 18:36, Blue Swirl wrote:
>>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>>> current base address in its version 2 vmstate.
>>>>>>>
>>>>>>> To align both implementations for migratability, add the proper
>>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>>> will also allow multiple IOAPICs in the future.
>>>>>>
>>>>>> Nack, the addresses should be device properties.
>>>>>
>>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>>> But current_base_address is just the same as apicbase and can't be a
>>>>> property.
>>>>
>>>> Oh, right. What will current_base_address used for? Why can't board
>>>> just unmap IOAPIC from current address and remap it at the new
>>>> address? Then the device would not need to know its base address.
>>>
>>> The board could do this. The question is where we put this service, in
>>> the context if the IOAPIC as ioapic_set_base_address (compare to
>>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>>> each and every board code. In the latter case, the boards would also be
>>> responsible for saving/restoring the address.
>>
>> How is the device relocated? Where are the chipset registers you mention?
>
> Intel's PIIX chipsets contain a register called APICBASE (but it means
> the IOAPIC), and that defines the location. The analogy in the APIC
> world is the MSR_IA32_APICBASE which we maintain via the APIC state.

In ICH10 the register is called OIC—Other Interrupt Control Register
and the interesting bits APIC Range Select (ASEL).

So actually PIIX should manage IOAPIC mapping, not board level.
Jan Kiszka - Feb. 3, 2011, 7:06 p.m.
On 2011-02-03 20:01, Blue Swirl wrote:
> On Thu, Feb 3, 2011 at 6:01 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-02-03 18:54, Blue Swirl wrote:
>>> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-02-03 18:36, Blue Swirl wrote:
>>>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>>>> current base address in its version 2 vmstate.
>>>>>>>>
>>>>>>>> To align both implementations for migratability, add the proper
>>>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>>>> will also allow multiple IOAPICs in the future.
>>>>>>>
>>>>>>> Nack, the addresses should be device properties.
>>>>>>
>>>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>>>> But current_base_address is just the same as apicbase and can't be a
>>>>>> property.
>>>>>
>>>>> Oh, right. What will current_base_address used for? Why can't board
>>>>> just unmap IOAPIC from current address and remap it at the new
>>>>> address? Then the device would not need to know its base address.
>>>>
>>>> The board could do this. The question is where we put this service, in
>>>> the context if the IOAPIC as ioapic_set_base_address (compare to
>>>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>>>> each and every board code. In the latter case, the boards would also be
>>>> responsible for saving/restoring the address.
>>>
>>> How is the device relocated? Where are the chipset registers you mention?
>>
>> Intel's PIIX chipsets contain a register called APICBASE (but it means
>> the IOAPIC), and that defines the location. The analogy in the APIC
>> world is the MSR_IA32_APICBASE which we maintain via the APIC state.
> 
> In ICH10 the register is called OIC—Other Interrupt Control Register
> and the interesting bits APIC Range Select (ASEL).
> 
> So actually PIIX should manage IOAPIC mapping, not board level.

The point is we need ioapic_set_base_address logic in multiple places
(once chipsets start to implement it). Better push it to a central place
from the beginning. Also the bit keeping. There is no difference to
apicbase.

Jan
Blue Swirl - Feb. 3, 2011, 7:11 p.m.
On Thu, Feb 3, 2011 at 7:06 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-02-03 20:01, Blue Swirl wrote:
>> On Thu, Feb 3, 2011 at 6:01 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2011-02-03 18:54, Blue Swirl wrote:
>>>> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2011-02-03 18:36, Blue Swirl wrote:
>>>>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>>>>> current base address in its version 2 vmstate.
>>>>>>>>>
>>>>>>>>> To align both implementations for migratability, add the proper
>>>>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>>>>> will also allow multiple IOAPICs in the future.
>>>>>>>>
>>>>>>>> Nack, the addresses should be device properties.
>>>>>>>
>>>>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>>>>> But current_base_address is just the same as apicbase and can't be a
>>>>>>> property.
>>>>>>
>>>>>> Oh, right. What will current_base_address used for? Why can't board
>>>>>> just unmap IOAPIC from current address and remap it at the new
>>>>>> address? Then the device would not need to know its base address.
>>>>>
>>>>> The board could do this. The question is where we put this service, in
>>>>> the context if the IOAPIC as ioapic_set_base_address (compare to
>>>>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>>>>> each and every board code. In the latter case, the boards would also be
>>>>> responsible for saving/restoring the address.
>>>>
>>>> How is the device relocated? Where are the chipset registers you mention?
>>>
>>> Intel's PIIX chipsets contain a register called APICBASE (but it means
>>> the IOAPIC), and that defines the location. The analogy in the APIC
>>> world is the MSR_IA32_APICBASE which we maintain via the APIC state.
>>
>> In ICH10 the register is called OIC—Other Interrupt Control Register
>> and the interesting bits APIC Range Select (ASEL).
>>
>> So actually PIIX should manage IOAPIC mapping, not board level.
>
> The point is we need ioapic_set_base_address logic in multiple places
> (once chipsets start to implement it). Better push it to a central place
> from the beginning. Also the bit keeping. There is no difference to
> apicbase.

In that case, the function should be made inline version in ioapic.h.
Jan Kiszka - Feb. 3, 2011, 7:25 p.m.
On 2011-02-03 20:11, Blue Swirl wrote:
> On Thu, Feb 3, 2011 at 7:06 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-02-03 20:01, Blue Swirl wrote:
>>> On Thu, Feb 3, 2011 at 6:01 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-02-03 18:54, Blue Swirl wrote:
>>>>> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> On 2011-02-03 18:36, Blue Swirl wrote:
>>>>>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>>>>>> current base address in its version 2 vmstate.
>>>>>>>>>>
>>>>>>>>>> To align both implementations for migratability, add the proper
>>>>>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>>>>>> will also allow multiple IOAPICs in the future.
>>>>>>>>>
>>>>>>>>> Nack, the addresses should be device properties.
>>>>>>>>
>>>>>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>>>>>> But current_base_address is just the same as apicbase and can't be a
>>>>>>>> property.
>>>>>>>
>>>>>>> Oh, right. What will current_base_address used for? Why can't board
>>>>>>> just unmap IOAPIC from current address and remap it at the new
>>>>>>> address? Then the device would not need to know its base address.
>>>>>>
>>>>>> The board could do this. The question is where we put this service, in
>>>>>> the context if the IOAPIC as ioapic_set_base_address (compare to
>>>>>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>>>>>> each and every board code. In the latter case, the boards would also be
>>>>>> responsible for saving/restoring the address.
>>>>>
>>>>> How is the device relocated? Where are the chipset registers you mention?
>>>>
>>>> Intel's PIIX chipsets contain a register called APICBASE (but it means
>>>> the IOAPIC), and that defines the location. The analogy in the APIC
>>>> world is the MSR_IA32_APICBASE which we maintain via the APIC state.
>>>
>>> In ICH10 the register is called OIC—Other Interrupt Control Register
>>> and the interesting bits APIC Range Select (ASEL).
>>>
>>> So actually PIIX should manage IOAPIC mapping, not board level.
>>
>> The point is we need ioapic_set_base_address logic in multiple places
>> (once chipsets start to implement it). Better push it to a central place
>> from the beginning. Also the bit keeping. There is no difference to
>> apicbase.
> 
> In that case, the function should be made inline version in ioapic.h.

That still replicates the bit keeping.

I don't see the benefit of moving it over, even less when we want to
consolidate with a vmstate layout that is already in use.

Jan
Blue Swirl - Feb. 3, 2011, 7:30 p.m.
On Thu, Feb 3, 2011 at 7:25 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-02-03 20:11, Blue Swirl wrote:
>> On Thu, Feb 3, 2011 at 7:06 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2011-02-03 20:01, Blue Swirl wrote:
>>>> On Thu, Feb 3, 2011 at 6:01 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> On 2011-02-03 18:54, Blue Swirl wrote:
>>>>>> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>> On 2011-02-03 18:36, Blue Swirl wrote:
>>>>>>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>>>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>>>>>>> current base address in its version 2 vmstate.
>>>>>>>>>>>
>>>>>>>>>>> To align both implementations for migratability, add the proper
>>>>>>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>>>>>>> will also allow multiple IOAPICs in the future.
>>>>>>>>>>
>>>>>>>>>> Nack, the addresses should be device properties.
>>>>>>>>>
>>>>>>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>>>>>>> But current_base_address is just the same as apicbase and can't be a
>>>>>>>>> property.
>>>>>>>>
>>>>>>>> Oh, right. What will current_base_address used for? Why can't board
>>>>>>>> just unmap IOAPIC from current address and remap it at the new
>>>>>>>> address? Then the device would not need to know its base address.
>>>>>>>
>>>>>>> The board could do this. The question is where we put this service, in
>>>>>>> the context if the IOAPIC as ioapic_set_base_address (compare to
>>>>>>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>>>>>>> each and every board code. In the latter case, the boards would also be
>>>>>>> responsible for saving/restoring the address.
>>>>>>
>>>>>> How is the device relocated? Where are the chipset registers you mention?
>>>>>
>>>>> Intel's PIIX chipsets contain a register called APICBASE (but it means
>>>>> the IOAPIC), and that defines the location. The analogy in the APIC
>>>>> world is the MSR_IA32_APICBASE which we maintain via the APIC state.
>>>>
>>>> In ICH10 the register is called OIC—Other Interrupt Control Register
>>>> and the interesting bits APIC Range Select (ASEL).
>>>>
>>>> So actually PIIX should manage IOAPIC mapping, not board level.
>>>
>>> The point is we need ioapic_set_base_address logic in multiple places
>>> (once chipsets start to implement it). Better push it to a central place
>>> from the beginning. Also the bit keeping. There is no difference to
>>> apicbase.
>>
>> In that case, the function should be made inline version in ioapic.h.
>
> That still replicates the bit keeping.
>
> I don't see the benefit of moving it over, even less when we want to
> consolidate with a vmstate layout that is already in use.

The benefit is that the device model is improved.
Jan Kiszka - Feb. 3, 2011, 7:42 p.m.
On 2011-02-03 20:30, Blue Swirl wrote:
> On Thu, Feb 3, 2011 at 7:25 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-02-03 20:11, Blue Swirl wrote:
>>> On Thu, Feb 3, 2011 at 7:06 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2011-02-03 20:01, Blue Swirl wrote:
>>>>> On Thu, Feb 3, 2011 at 6:01 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> On 2011-02-03 18:54, Blue Swirl wrote:
>>>>>>> On Thu, Feb 3, 2011 at 5:43 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>> On 2011-02-03 18:36, Blue Swirl wrote:
>>>>>>>>> On Thu, Feb 3, 2011 at 5:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>>> On 2011-02-03 18:03, Blue Swirl wrote:
>>>>>>>>>>> On Thu, Feb 3, 2011 at 2:55 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>>>>> The registers of real IOAPICs can be relocated during runtime (via
>>>>>>>>>>>> chipset registers). We don't support this yet, but qemu-kvm carries the
>>>>>>>>>>>> current base address in its version 2 vmstate.
>>>>>>>>>>>>
>>>>>>>>>>>> To align both implementations for migratability, add the proper
>>>>>>>>>>>> infrastructure to accept initial as well as updated base addresses and
>>>>>>>>>>>> include the current address in the vmstate. This is done in a way that
>>>>>>>>>>>> will also allow multiple IOAPICs in the future.
>>>>>>>>>>>
>>>>>>>>>>> Nack, the addresses should be device properties.
>>>>>>>>>>
>>>>>>>>>> Hmm.... we could make default_base_address a property. Will change that.
>>>>>>>>>> But current_base_address is just the same as apicbase and can't be a
>>>>>>>>>> property.
>>>>>>>>>
>>>>>>>>> Oh, right. What will current_base_address used for? Why can't board
>>>>>>>>> just unmap IOAPIC from current address and remap it at the new
>>>>>>>>> address? Then the device would not need to know its base address.
>>>>>>>>
>>>>>>>> The board could do this. The question is where we put this service, in
>>>>>>>> the context if the IOAPIC as ioapic_set_base_address (compare to
>>>>>>>> cpu_set_apic_base - which is buggy as it lacks sysbus_mmio_map) or into
>>>>>>>> each and every board code. In the latter case, the boards would also be
>>>>>>>> responsible for saving/restoring the address.
>>>>>>>
>>>>>>> How is the device relocated? Where are the chipset registers you mention?
>>>>>>
>>>>>> Intel's PIIX chipsets contain a register called APICBASE (but it means
>>>>>> the IOAPIC), and that defines the location. The analogy in the APIC
>>>>>> world is the MSR_IA32_APICBASE which we maintain via the APIC state.
>>>>>
>>>>> In ICH10 the register is called OIC—Other Interrupt Control Register
>>>>> and the interesting bits APIC Range Select (ASEL).
>>>>>
>>>>> So actually PIIX should manage IOAPIC mapping, not board level.
>>>>
>>>> The point is we need ioapic_set_base_address logic in multiple places
>>>> (once chipsets start to implement it). Better push it to a central place
>>>> from the beginning. Also the bit keeping. There is no difference to
>>>> apicbase.
>>>
>>> In that case, the function should be made inline version in ioapic.h.
>>
>> That still replicates the bit keeping.
>>
>> I don't see the benefit of moving it over, even less when we want to
>> consolidate with a vmstate layout that is already in use.
> 
> The benefit is that the device model is improved.

I disagree about the benefit, but I will simply drop this patch and
instead add a dummy field to a vmstate version 3 so that qemu-kvm can
remove the base_address evaluation logic when updating.

Jan

Patch

diff --git a/hw/ioapic.c b/hw/ioapic.c
index c7019f5..b53d436 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -63,6 +63,8 @@  struct IOAPICState {
 
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
+    uint64_t default_base_address;
+    uint64_t current_base_address;
 };
 
 static IOAPICState *ioapics[MAX_IOAPICS];
@@ -237,6 +239,7 @@  static int ioapic_pre_load(void *opaque)
 
     /* in case we are loading version 1, set these to sane values */
     s->irr = 0;
+    s->current_base_address = s->default_base_address;
     return 0;
 }
 
@@ -249,6 +252,7 @@  static const VMStateDescription vmstate_ioapic = {
     .fields      = (VMStateField []) {
         VMSTATE_UINT8(id, IOAPICState),
         VMSTATE_UINT8(ioregsel, IOAPICState),
+        VMSTATE_UINT64_V(current_base_address, IOAPICState, 2),
         VMSTATE_UINT32_V(irr, IOAPICState, 2),
         VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
         VMSTATE_END_OF_LIST()
@@ -260,6 +264,8 @@  static void ioapic_reset(DeviceState *d)
     IOAPICState *s = DO_UPCAST(IOAPICState, busdev.qdev, d);
     int i;
 
+    s->current_base_address = s->default_base_address;
+    sysbus_mmio_map(&s->busdev, 0, s->current_base_address);
     s->id = 0;
     s->ioregsel = 0;
     s->irr = 0;
@@ -279,6 +285,17 @@  static CPUWriteMemoryFunc * const ioapic_mem_write[3] = {
     ioapic_mem_writel,
 };
 
+void ioapic_set_base_address(DeviceState *d, target_phys_addr_t addr)
+{
+    IOAPICState *s = DO_UPCAST(IOAPICState, busdev.qdev, d);
+
+    s->current_base_address = addr;
+    if (!s->default_base_address) {
+        s->default_base_address = addr;
+    }
+    sysbus_mmio_map(&s->busdev, 0, addr);
+}
+
 static int ioapic_init1(SysBusDevice *dev)
 {
     IOAPICState *s = FROM_SYSBUS(IOAPICState, dev);
diff --git a/hw/ioapic.h b/hw/ioapic.h
index c441567..1130d60 100644
--- a/hw/ioapic.h
+++ b/hw/ioapic.h
@@ -17,4 +17,8 @@ 
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#define IOAPIC_DEFAULT_BASE_ADDRESS    0xfec00000
+
 void ioapic_eio_broadcast(int vector);
+
+void ioapic_set_base_address(DeviceState *d, target_phys_addr_t addr);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 7b74473..479334f 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -25,6 +25,7 @@ 
 #include "hw.h"
 #include "pc.h"
 #include "apic.h"
+#include "ioapic.h"
 #include "pci.h"
 #include "usb-uhci.h"
 #include "usb-ohci.h"
@@ -46,13 +47,11 @@  static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 static void ioapic_init(IsaIrqState *isa_irq_state)
 {
     DeviceState *dev;
-    SysBusDevice *d;
     unsigned int i;
 
     dev = qdev_create(NULL, "ioapic");
     qdev_init_nofail(dev);
-    d = sysbus_from_qdev(dev);
-    sysbus_mmio_map(d, 0, 0xfec00000);
+    ioapic_set_base_address(dev, IOAPIC_DEFAULT_BASE_ADDRESS);
 
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         isa_irq_state->ioapic[i] = qdev_get_gpio_in(dev, i);