[Qemu-devel] [0.14?][PATCH 0/4] IOAPIC fixes

[Qemu-devel] [0.14?][PATCH 0/4] IOAPIC fixes

[Jan Kiszka]

This series fixes the re-injection of level-triggered IRQs that are
still raised on APIC EOI, adds a must-have field to the vmstate of the
IOAPIC, and also aligns that vmstate with qemu-kvm.

I would recommend the whole series for 0.14, but at least patch 1 should
be applied.



Jan Kiszka (4):
  ioapic: Implement EOI handling for level-triggered IRQs
  ioapic: Save/restore irr
  ioapic: Prepare for base address relocation
  ioapic: Style & magics cleanup

 hw/apic.c    |    9 ++-
 hw/ioapic.c  |  244 +++++++++++++++++++++++++++++++++++++++++-----------------
 hw/ioapic.h  |   24 ++++++
 hw/pc_piix.c |    5 +-
 4 files changed, 206 insertions(+), 76 deletions(-)
 create mode 100644 hw/ioapic.h

[Qemu-devel] [0.14?][PATCH 1/4] ioapic: Implement EOI handling for level-triggered IRQs

[Jan Kiszka]

Add the missing EOI broadcast from local APIC to the IOAPICs on
completion of level-triggered IRQs. This ensures that a still asserted
IRQ source properly re-triggers an APIC IRQ.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/apic.c   |    9 ++++++---
 hw/ioapic.c |   43 +++++++++++++++++++++++++++++++++++++++++--
 hw/ioapic.h |   20 ++++++++++++++++++++
 3 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100644 hw/ioapic.h

diff --git a/hw/apic.c b/hw/apic.c
index ff581f0..05a115f 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -18,6 +18,7 @@
  */
 #include "hw.h"
 #include "apic.h"
+#include "ioapic.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
 #include "sysbus.h"
@@ -57,7 +58,8 @@
 
 #define ESR_ILLEGAL_ADDRESS (1 << 7)
 
-#define APIC_SV_ENABLE (1 << 8)
+#define APIC_SV_DIRECTED_IO             (1<<12)
+#define APIC_SV_ENABLE                  (1<<8)
 
 #define MAX_APICS 255
 #define MAX_APIC_WORDS 8
@@ -420,8 +422,9 @@ static void apic_eoi(APICState *s)
     if (isrv < 0)
         return;
     reset_bit(s->isr, isrv);
-    /* XXX: send the EOI packet to the APIC bus to allow the I/O APIC to
-            set the remote IRR bit for level triggered interrupts. */
+    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
+        ioapic_eio_broadcast(isrv);
+    }
     apic_update_irq(s);
 }
 
diff --git a/hw/ioapic.c b/hw/ioapic.c
index 2109568..443c579 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -23,6 +23,7 @@
 #include "hw.h"
 #include "pc.h"
 #include "apic.h"
+#include "ioapic.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
 #include "sysbus.h"
@@ -36,7 +37,10 @@
 #define DPRINTF(fmt, ...)
 #endif
 
-#define IOAPIC_LVT_MASKED 		(1<<16)
+#define MAX_IOAPICS                     1
+
+#define IOAPIC_LVT_MASKED               (1 << 16)
+#define IOAPIC_LVT_REMOTE_IRR           (1 << 14)
 
 #define IOAPIC_TRIGGER_EDGE		0
 #define IOAPIC_TRIGGER_LEVEL		1
@@ -61,6 +65,8 @@ struct IOAPICState {
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
 };
 
+static IOAPICState *ioapics[MAX_IOAPICS];
+
 static void ioapic_service(IOAPICState *s)
 {
     uint8_t i;
@@ -83,8 +89,11 @@ static void ioapic_service(IOAPICState *s)
                 dest_mode = (entry >> 11) & 1;
                 delivery_mode = (entry >> 8) & 7;
                 polarity = (entry >> 13) & 1;
-                if (trig_mode == IOAPIC_TRIGGER_EDGE)
+                if (trig_mode == IOAPIC_TRIGGER_EDGE) {
                     s->irr &= ~mask;
+                } else {
+                    s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
+                }
                 if (delivery_mode == IOAPIC_DM_EXTINT)
                     vector = pic_read_irq(isa_pic);
                 else
@@ -131,6 +140,29 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
     }
 }
 
+void ioapic_eio_broadcast(int vector)
+{
+    IOAPICState *s;
+    uint64_t entry;
+    int i, n;
+
+    for (i = 0; i < MAX_IOAPICS; i++) {
+        s = ioapics[i];
+        if (!s) {
+            continue;
+        }
+        for (n = 0; n < IOAPIC_NUM_PINS; n++) {
+            entry = s->ioredtbl[n];
+            if ((entry & IOAPIC_LVT_REMOTE_IRR) && (entry & 0xff) == vector) {
+                s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
+                if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
+                    ioapic_service(s);
+                }
+            }
+        }
+    }
+}
+
 static uint32_t ioapic_mem_readl(void *opaque, target_phys_addr_t addr)
 {
     IOAPICState *s = opaque;
@@ -240,6 +272,11 @@ static int ioapic_init1(SysBusDevice *dev)
 {
     IOAPICState *s = FROM_SYSBUS(IOAPICState, dev);
     int io_memory;
+    static int ioapic_no;
+
+    if (ioapic_no >= MAX_IOAPICS) {
+        return -1;
+    }
 
     io_memory = cpu_register_io_memory(ioapic_mem_read,
                                        ioapic_mem_write, s,
@@ -248,6 +285,8 @@ static int ioapic_init1(SysBusDevice *dev)
 
     qdev_init_gpio_in(&dev->qdev, ioapic_set_irq, IOAPIC_NUM_PINS);
 
+    ioapics[ioapic_no++] = s;
+
     return 0;
 }
 
diff --git a/hw/ioapic.h b/hw/ioapic.h
new file mode 100644
index 0000000..c441567
--- /dev/null
+++ b/hw/ioapic.h
@@ -0,0 +1,20 @@
+/*
+ *  ioapic.c IOAPIC emulation logic
+ *
+ *  Copyright (c) 2011 Jan Kiszka, Siemens AG
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+void ioapic_eio_broadcast(int vector);
-- 
1.7.1

[Qemu-devel] Re: [0.14?][PATCH 1/4] ioapic: Implement EOI handling for level-triggered IRQs

[Anthony Liguori]

On 02/03/2011 08:55 AM, Jan Kiszka wrote:
> Add the missing EOI broadcast from local APIC to the IOAPICs on
> completion of level-triggered IRQs. This ensures that a still asserted
> IRQ source properly re-triggers an APIC IRQ.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>   hw/apic.c   |    9 ++++++---
>   hw/ioapic.c |   43 +++++++++++++++++++++++++++++++++++++++++--
>   hw/ioapic.h |   20 ++++++++++++++++++++
>   3 files changed, 67 insertions(+), 5 deletions(-)
>   create mode 100644 hw/ioapic.h
>
> diff --git a/hw/apic.c b/hw/apic.c
> index ff581f0..05a115f 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -18,6 +18,7 @@
>    */
>   #include "hw.h"
>   #include "apic.h"
> +#include "ioapic.h"
>   #include "qemu-timer.h"
>   #include "host-utils.h"
>   #include "sysbus.h"
> @@ -57,7 +58,8 @@
>
>   #define ESR_ILLEGAL_ADDRESS (1<<  7)
>
> -#define APIC_SV_ENABLE (1<<  8)
> +#define APIC_SV_DIRECTED_IO             (1<<12)
> +#define APIC_SV_ENABLE                  (1<<8)
>
>   #define MAX_APICS 255
>   #define MAX_APIC_WORDS 8
> @@ -420,8 +422,9 @@ static void apic_eoi(APICState *s)
>       if (isrv<  0)
>           return;
>       reset_bit(s->isr, isrv);
> -    /* XXX: send the EOI packet to the APIC bus to allow the I/O APIC to
> -            set the remote IRR bit for level triggered interrupts. */
> +    if (!(s->spurious_vec&  APIC_SV_DIRECTED_IO)&&  get_bit(s->tmr, isrv)) {
> +        ioapic_eio_broadcast(isrv);
> +    }
>       apic_update_irq(s);
>   }
>
> diff --git a/hw/ioapic.c b/hw/ioapic.c
> index 2109568..443c579 100644
> --- a/hw/ioapic.c
> +++ b/hw/ioapic.c
> @@ -23,6 +23,7 @@
>   #include "hw.h"
>   #include "pc.h"
>   #include "apic.h"
> +#include "ioapic.h"
>   #include "qemu-timer.h"
>   #include "host-utils.h"
>   #include "sysbus.h"
> @@ -36,7 +37,10 @@
>   #define DPRINTF(fmt, ...)
>   #endif
>
> -#define IOAPIC_LVT_MASKED 		(1<<16)
> +#define MAX_IOAPICS                     1
> +
> +#define IOAPIC_LVT_MASKED               (1<<  16)
> +#define IOAPIC_LVT_REMOTE_IRR           (1<<  14)
>
>   #define IOAPIC_TRIGGER_EDGE		0
>   #define IOAPIC_TRIGGER_LEVEL		1
> @@ -61,6 +65,8 @@ struct IOAPICState {
>       uint64_t ioredtbl[IOAPIC_NUM_PINS];
>   };
>
> +static IOAPICState *ioapics[MAX_IOAPICS];
> +
>   static void ioapic_service(IOAPICState *s)
>   {
>       uint8_t i;
> @@ -83,8 +89,11 @@ static void ioapic_service(IOAPICState *s)
>                   dest_mode = (entry>>  11)&  1;
>                   delivery_mode = (entry>>  8)&  7;
>                   polarity = (entry>>  13)&  1;
> -                if (trig_mode == IOAPIC_TRIGGER_EDGE)
> +                if (trig_mode == IOAPIC_TRIGGER_EDGE) {
>                       s->irr&= ~mask;
> +                } else {
> +                    s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
> +                }
>                   if (delivery_mode == IOAPIC_DM_EXTINT)
>                       vector = pic_read_irq(isa_pic);
>                   else
> @@ -131,6 +140,29 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
>       }
>   }
>
> +void ioapic_eio_broadcast(int vector)
>    

You mean iopaic_eoi_broadcast(), no?

Regards,

Anthony Liguori

> +{
> +    IOAPICState *s;
> +    uint64_t entry;
> +    int i, n;
> +
> +    for (i = 0; i<  MAX_IOAPICS; i++) {
> +        s = ioapics[i];
> +        if (!s) {
> +            continue;
> +        }
> +        for (n = 0; n<  IOAPIC_NUM_PINS; n++) {
> +            entry = s->ioredtbl[n];
> +            if ((entry&  IOAPIC_LVT_REMOTE_IRR)&&  (entry&  0xff) == vector) {
> +                s->ioredtbl[n] = entry&  ~IOAPIC_LVT_REMOTE_IRR;
> +                if (!(entry&  IOAPIC_LVT_MASKED)&&  (s->irr&  (1<<  n))) {
> +                    ioapic_service(s);
> +                }
> +            }
> +        }
> +    }
> +}
> +
>   static uint32_t ioapic_mem_readl(void *opaque, target_phys_addr_t addr)
>   {
>       IOAPICState *s = opaque;
> @@ -240,6 +272,11 @@ static int ioapic_init1(SysBusDevice *dev)
>   {
>       IOAPICState *s = FROM_SYSBUS(IOAPICState, dev);
>       int io_memory;
> +    static int ioapic_no;
> +
> +    if (ioapic_no>= MAX_IOAPICS) {
> +        return -1;
> +    }
>
>       io_memory = cpu_register_io_memory(ioapic_mem_read,
>                                          ioapic_mem_write, s,
> @@ -248,6 +285,8 @@ static int ioapic_init1(SysBusDevice *dev)
>
>       qdev_init_gpio_in(&dev->qdev, ioapic_set_irq, IOAPIC_NUM_PINS);
>
> +    ioapics[ioapic_no++] = s;
> +
>       return 0;
>   }
>
> diff --git a/hw/ioapic.h b/hw/ioapic.h
> new file mode 100644
> index 0000000..c441567
> --- /dev/null
> +++ b/hw/ioapic.h
> @@ -0,0 +1,20 @@
> +/*
> + *  ioapic.c IOAPIC emulation logic
> + *
> + *  Copyright (c) 2011 Jan Kiszka, Siemens AG
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see<http://www.gnu.org/licenses/>.
> + */
> +
> +void ioapic_eio_broadcast(int vector);
>    

[Qemu-devel] [0.14?][PATCH 2/4] ioapic: Save/restore irr

[Jan Kiszka]

This is a guest modifiable state that must be saved/restored properly.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioapic.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 443c579..c7019f5 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -231,14 +231,25 @@ static void ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t va
     }
 }
 
+static int ioapic_pre_load(void *opaque)
+{
+    IOAPICState *s = opaque;
+
+    /* in case we are loading version 1, set these to sane values */
+    s->irr = 0;
+    return 0;
+}
+
 static const VMStateDescription vmstate_ioapic = {
     .name = "ioapic",
-    .version_id = 1,
+    .version_id = 2,
+    .pre_load = ioapic_pre_load,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
         VMSTATE_UINT8(id, IOAPICState),
         VMSTATE_UINT8(ioregsel, IOAPICState),
+        VMSTATE_UINT32_V(irr, IOAPICState, 2),
         VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
         VMSTATE_END_OF_LIST()
     }
-- 
1.7.1

[Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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(-)

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);
-- 
1.7.1

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Blue Swirl]

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.

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Blue Swirl]

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.

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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.

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Blue Swirl]

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?

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Blue Swirl]

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.

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Blue Swirl]

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.

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Blue Swirl]

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.

Re: [Qemu-devel] [0.14?][PATCH 3/4] ioapic: Prepare for base address relocation

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [0.14?][PATCH 4/4] ioapic: Style & magics cleanup

[Jan Kiszka]

Fix a few style issues and convert magic numbers into prober symbolic
constants, also fixing the wrong but unused IOAPIC_DM_SIPI value.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioapic.c |  177 +++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 107 insertions(+), 70 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index b53d436..a9d7fcd 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -39,20 +39,48 @@
 
 #define MAX_IOAPICS                     1
 
-#define IOAPIC_LVT_MASKED               (1 << 16)
-#define IOAPIC_LVT_REMOTE_IRR           (1 << 14)
+#define IOAPIC_VERSION                  0x11
 
-#define IOAPIC_TRIGGER_EDGE		0
-#define IOAPIC_TRIGGER_LEVEL		1
+#define IOAPIC_LVT_DEST_SHIFT           56
+#define IOAPIC_LVT_MASKED_SHIFT         16
+#define IOAPIC_LVT_TRIGGER_MODE_SHIFT   15
+#define IOAPIC_LVT_REMOTE_IRR_SHIFT     14
+#define IOAPIC_LVT_POLARITY_SHIFT       13
+#define IOAPIC_LVT_DELIV_STATUS_SHIFT   12
+#define IOAPIC_LVT_DEST_MODE_SHIFT      11
+#define IOAPIC_LVT_DELIV_MODE_SHIFT     8
+
+#define IOAPIC_LVT_MASKED               (1 << IOAPIC_LVT_MASKED_SHIFT)
+#define IOAPIC_LVT_REMOTE_IRR           (1 << IOAPIC_LVT_REMOTE_IRR_SHIFT)
+
+#define IOAPIC_TRIGGER_EDGE             0
+#define IOAPIC_TRIGGER_LEVEL            1
 
 /*io{apic,sapic} delivery mode*/
-#define IOAPIC_DM_FIXED			0x0
-#define IOAPIC_DM_LOWEST_PRIORITY	0x1
-#define IOAPIC_DM_PMI			0x2
-#define IOAPIC_DM_NMI			0x4
-#define IOAPIC_DM_INIT			0x5
-#define IOAPIC_DM_SIPI			0x5
-#define IOAPIC_DM_EXTINT		0x7
+#define IOAPIC_DM_FIXED                 0x0
+#define IOAPIC_DM_LOWEST_PRIORITY       0x1
+#define IOAPIC_DM_PMI                   0x2
+#define IOAPIC_DM_NMI                   0x4
+#define IOAPIC_DM_INIT                  0x5
+#define IOAPIC_DM_SIPI                  0x6
+#define IOAPIC_DM_EXTINT                0x7
+#define IOAPIC_DM_MASK                  0x7
+
+#define IOAPIC_VECTOR_MASK              0xff
+
+#define IOAPIC_IOREGSEL                 0x00
+#define IOAPIC_IOWIN                    0x10
+
+#define IOAPIC_REG_ID                   0x00
+#define IOAPIC_REG_VER                  0x01
+#define IOAPIC_REG_ARB                  0x02
+#define IOAPIC_REG_REDTBL_BASE          0x10
+#define IOAPIC_ID                       0x00
+
+#define IOAPIC_ID_SHIFT                 24
+#define IOAPIC_ID_MASK                  0xf
+
+#define IOAPIC_VER_ENTRIES_SHIFT        16
 
 typedef struct IOAPICState IOAPICState;
 
@@ -60,7 +88,6 @@ struct IOAPICState {
     SysBusDevice busdev;
     uint8_t id;
     uint8_t ioregsel;
-
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
     uint64_t default_base_address;
@@ -86,21 +113,22 @@ static void ioapic_service(IOAPICState *s)
         if (s->irr & mask) {
             entry = s->ioredtbl[i];
             if (!(entry & IOAPIC_LVT_MASKED)) {
-                trig_mode = ((entry >> 15) & 1);
-                dest = entry >> 56;
-                dest_mode = (entry >> 11) & 1;
-                delivery_mode = (entry >> 8) & 7;
-                polarity = (entry >> 13) & 1;
+                trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
+                dest = entry >> IOAPIC_LVT_DEST_SHIFT;
+                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
+                delivery_mode =
+                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
+                polarity = (entry >> IOAPIC_LVT_POLARITY_SHIFT) & 1;
                 if (trig_mode == IOAPIC_TRIGGER_EDGE) {
                     s->irr &= ~mask;
                 } else {
                     s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
                 }
-                if (delivery_mode == IOAPIC_DM_EXTINT)
+                if (delivery_mode == IOAPIC_DM_EXTINT) {
                     vector = pic_read_irq(isa_pic);
-                else
-                    vector = entry & 0xff;
-
+                } else {
+                    vector = entry & IOAPIC_VECTOR_MASK;
+                }
                 apic_deliver_irq(dest, dest_mode, delivery_mode,
                                  vector, polarity, trig_mode);
             }
@@ -116,15 +144,16 @@ static void ioapic_set_irq(void *opaque, int vector, int level)
      * to GSI 2.  GSI maps to ioapic 1-1.  This is not
      * the cleanest way of doing it but it should work. */
 
-    DPRINTF("%s: %s vec %x\n", __func__, level? "raise" : "lower", vector);
-    if (vector == 0)
+    DPRINTF("%s: %s vec %x\n", __func__, level ? "raise" : "lower", vector);
+    if (vector == 0) {
         vector = 2;
-
+    }
     if (vector >= 0 && vector < IOAPIC_NUM_PINS) {
         uint32_t mask = 1 << vector;
         uint64_t entry = s->ioredtbl[vector];
 
-        if ((entry >> 15) & 1) {
+        if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) ==
+            IOAPIC_TRIGGER_LEVEL) {
             /* level triggered */
             if (level) {
                 s->irr |= mask;
@@ -155,7 +184,8 @@ void ioapic_eio_broadcast(int vector)
         }
         for (n = 0; n < IOAPIC_NUM_PINS; n++) {
             entry = s->ioredtbl[n];
-            if ((entry & IOAPIC_LVT_REMOTE_IRR) && (entry & 0xff) == vector) {
+            if ((entry & IOAPIC_LVT_REMOTE_IRR)
+                && (entry & IOAPIC_VECTOR_MASK) == vector) {
                 s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
                 if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
                     ioapic_service(s);
@@ -171,65 +201,71 @@ static uint32_t ioapic_mem_readl(void *opaque, target_phys_addr_t addr)
     int index;
     uint32_t val = 0;
 
-    addr &= 0xff;
-    if (addr == 0x00) {
+    switch (addr & 0xff) {
+    case IOAPIC_IOREGSEL:
         val = s->ioregsel;
-    } else if (addr == 0x10) {
+        break;
+    case IOAPIC_IOWIN:
         switch (s->ioregsel) {
-            case 0x00:
-                val = s->id << 24;
-                break;
-            case 0x01:
-                val = 0x11 | ((IOAPIC_NUM_PINS - 1) << 16); /* version 0x11 */
-                break;
-            case 0x02:
-                val = 0;
-                break;
-            default:
-                index = (s->ioregsel - 0x10) >> 1;
-                if (index >= 0 && index < IOAPIC_NUM_PINS) {
-                    if (s->ioregsel & 1)
-                        val = s->ioredtbl[index] >> 32;
-                    else
-                        val = s->ioredtbl[index] & 0xffffffff;
+        case IOAPIC_REG_ID:
+            val = s->id << IOAPIC_ID_SHIFT;
+            break;
+        case IOAPIC_REG_VER:
+            val = IOAPIC_VERSION |
+                ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT);
+            break;
+        case IOAPIC_REG_ARB:
+            val = 0;
+            break;
+        default:
+            index = (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1;
+            if (index >= 0 && index < IOAPIC_NUM_PINS) {
+                if (s->ioregsel & 1) {
+                    val = s->ioredtbl[index] >> 32;
+                } else {
+                    val = s->ioredtbl[index] & 0xffffffff;
                 }
+            }
         }
         DPRINTF("read: %08x = %08x\n", s->ioregsel, val);
+        break;
     }
     return val;
 }
 
-static void ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+static void
+ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     IOAPICState *s = opaque;
     int index;
 
-    addr &= 0xff;
-    if (addr == 0x00)  {
+    switch (addr & 0xff) {
+    case IOAPIC_IOREGSEL:
         s->ioregsel = val;
-        return;
-    } else if (addr == 0x10) {
+        break;
+    case IOAPIC_IOWIN:
         DPRINTF("write: %08x = %08x\n", s->ioregsel, val);
         switch (s->ioregsel) {
-            case 0x00:
-                s->id = (val >> 24) & 0xff;
-                return;
-            case 0x01:
-            case 0x02:
-                return;
-            default:
-                index = (s->ioregsel - 0x10) >> 1;
-                if (index >= 0 && index < IOAPIC_NUM_PINS) {
-                    if (s->ioregsel & 1) {
-                        s->ioredtbl[index] &= 0xffffffff;
-                        s->ioredtbl[index] |= (uint64_t)val << 32;
-                    } else {
-                        s->ioredtbl[index] &= ~0xffffffffULL;
-                        s->ioredtbl[index] |= val;
-                    }
-                    ioapic_service(s);
+        case IOAPIC_REG_ID:
+            s->id = (val >> IOAPIC_ID_SHIFT) & IOAPIC_ID_MASK;
+            break;
+        case IOAPIC_REG_VER:
+        case IOAPIC_REG_ARB:
+            break;
+        default:
+            index = (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1;
+            if (index >= 0 && index < IOAPIC_NUM_PINS) {
+                if (s->ioregsel & 1) {
+                    s->ioredtbl[index] &= 0xffffffff;
+                    s->ioredtbl[index] |= (uint64_t)val << 32;
+                } else {
+                    s->ioredtbl[index] &= ~0xffffffffULL;
+                    s->ioredtbl[index] |= val;
                 }
+                ioapic_service(s);
+            }
         }
+        break;
     }
 }
 
@@ -249,7 +285,7 @@ static const VMStateDescription vmstate_ioapic = {
     .pre_load = ioapic_pre_load,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .fields      = (VMStateField []) {
+    .fields = (VMStateField[]) {
         VMSTATE_UINT8(id, IOAPICState),
         VMSTATE_UINT8(ioregsel, IOAPICState),
         VMSTATE_UINT64_V(current_base_address, IOAPICState, 2),
@@ -269,8 +305,9 @@ static void ioapic_reset(DeviceState *d)
     s->id = 0;
     s->ioregsel = 0;
     s->irr = 0;
-    for(i = 0; i < IOAPIC_NUM_PINS; i++)
-        s->ioredtbl[i] = 1 << 16; /* mask LVT */
+    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+        s->ioredtbl[i] = 1 << IOAPIC_LVT_MASKED_SHIFT;
+    }
 }
 
 static CPUReadMemoryFunc * const ioapic_mem_read[3] = {
-- 
1.7.1