diff mbox

[2/5] x86/vioapic: allow the vIO APIC to have a variable number of pins

Message ID 20170223115217.32764-3-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné Feb. 23, 2017, 11:52 a.m. UTC
Altough it's still always set to VIOAPIC_NUM_PINS (48).

Add a new field to the hvm_hw_ioapic struct to contain the number of pins
(number of IO redirection table entries), and add the migration compatibility
code.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/misc/xen-hvmctx.c                |  25 +++++-
 xen/arch/x86/hvm/vioapic.c             | 138 +++++++++++++++++++++++++++++----
 xen/include/public/arch-x86/hvm/save.h |  50 +++++++-----
 3 files changed, 177 insertions(+), 36 deletions(-)

Comments

Jan Beulich March 3, 2017, 11:56 a.m. UTC | #1
>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> @@ -424,40 +424,141 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>      spin_unlock(&d->arch.hvm_domain.irq_lock);
>  }
>  
> -static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
> +#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)

What is this redirtbl field, which oddly enough is a pointer (not allowed
in the public interface) good for? I can't seem to find any use of it
other than as marker (for use with offsetof()). With all the
complications here and below plus the fixed number of entries
remaining to be fixed for actual migration purposes I wonder if you
wouldn't be better off by keeping the structure mostly as is, simply
converting the redirtbl[] field to a union (guarded by a __XEN__
conditional) containing both a fixed size array and a variable size
one (or to be precise, a zero size one, as a variable size one is not
allowed there).

Another alternative would be to introduce a Xen internal sibling
struct with a variable size array, and with all fields properly build-
time-asserted to be identical between both.

I'll skip the rest of the patch assuming much of it could be dropped
this way.

Jan
Roger Pau Monné March 3, 2017, 12:53 p.m. UTC | #2
On Fri, Mar 03, 2017 at 04:56:20AM -0700, Jan Beulich wrote:
> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> > @@ -424,40 +424,141 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> >      spin_unlock(&d->arch.hvm_domain.irq_lock);
> >  }
> >  
> > -static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
> > +#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)
> 
> What is this redirtbl field, which oddly enough is a pointer (not allowed
> in the public interface) good for? I can't seem to find any use of it
> other than as marker (for use with offsetof()). With all the
> complications here and below plus the fixed number of entries
> remaining to be fixed for actual migration purposes I wonder if you
> wouldn't be better off by keeping the structure mostly as is, simply
> converting the redirtbl[] field to a union (guarded by a __XEN__
> conditional) containing both a fixed size array and a variable size
> one (or to be precise, a zero size one, as a variable size one is not
> allowed there).
> 
> Another alternative would be to introduce a Xen internal sibling
> struct with a variable size array, and with all fields properly build-
> time-asserted to be identical between both.
> 
> I'll skip the rest of the patch assuming much of it could be dropped
> this way.

That was indeed my first approach, but later patches introduce an array of
hvm_hw_vioapic structs (in order to support multiple IO APICs per domain), and
this doesn't work if the struct itself contains a variable size array. I can
encapsulate this inside of a hvm_vioapic struct, like:

struct hvm_vioapic {
    struct hvm_hw_ioapic *vioapic;
}

And make and array of hvm_vioapics instead, but that seemed more cumbersome.

I know this is all quite painful, but I wasn't able to sport a better solution.

Thanks, Roger.
Jan Beulich March 3, 2017, 1:02 p.m. UTC | #3
>>> On 03.03.17 at 13:53, <roger.pau@citrix.com> wrote:
> On Fri, Mar 03, 2017 at 04:56:20AM -0700, Jan Beulich wrote:
>> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
>> > @@ -424,40 +424,141 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>> >      spin_unlock(&d->arch.hvm_domain.irq_lock);
>> >  }
>> >  
>> > -static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
>> > +#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)
>> 
>> What is this redirtbl field, which oddly enough is a pointer (not allowed
>> in the public interface) good for? I can't seem to find any use of it
>> other than as marker (for use with offsetof()). With all the
>> complications here and below plus the fixed number of entries
>> remaining to be fixed for actual migration purposes I wonder if you
>> wouldn't be better off by keeping the structure mostly as is, simply
>> converting the redirtbl[] field to a union (guarded by a __XEN__
>> conditional) containing both a fixed size array and a variable size
>> one (or to be precise, a zero size one, as a variable size one is not
>> allowed there).
>> 
>> Another alternative would be to introduce a Xen internal sibling
>> struct with a variable size array, and with all fields properly build-
>> time-asserted to be identical between both.
>> 
>> I'll skip the rest of the patch assuming much of it could be dropped
>> this way.
> 
> That was indeed my first approach, but later patches introduce an array of
> hvm_hw_vioapic structs (in order to support multiple IO APICs per domain), and
> this doesn't work if the struct itself contains a variable size array.

By what are you indexing that array? How about using a linked list
instead (or see below)? There won't normally be many entries, so
traversal is not an issue.

> I can
> encapsulate this inside of a hvm_vioapic struct, like:
> 
> struct hvm_vioapic {
>     struct hvm_hw_ioapic *vioapic;
> }
> 
> And make and array of hvm_vioapics instead, but that seemed more cumbersome.

Well, you don't need a structure here at all. Just have an array
of pointers. In any event I think the respective public interface
change should be avoided here if at all possible.

Jan
Roger Pau Monné March 3, 2017, 3:30 p.m. UTC | #4
On Fri, Mar 03, 2017 at 06:02:58AM -0700, Jan Beulich wrote:
> >>> On 03.03.17 at 13:53, <roger.pau@citrix.com> wrote:
> > On Fri, Mar 03, 2017 at 04:56:20AM -0700, Jan Beulich wrote:
> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> >> > @@ -424,40 +424,141 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> >> >      spin_unlock(&d->arch.hvm_domain.irq_lock);
> >> >  }
> >> >  
> >> > -static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
> >> > +#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)
> >> 
> >> What is this redirtbl field, which oddly enough is a pointer (not allowed
> >> in the public interface) good for? I can't seem to find any use of it
> >> other than as marker (for use with offsetof()). With all the
> >> complications here and below plus the fixed number of entries
> >> remaining to be fixed for actual migration purposes I wonder if you
> >> wouldn't be better off by keeping the structure mostly as is, simply
> >> converting the redirtbl[] field to a union (guarded by a __XEN__
> >> conditional) containing both a fixed size array and a variable size
> >> one (or to be precise, a zero size one, as a variable size one is not
> >> allowed there).

In that same file hvm_msr struct is using a variable size array defined as
follows:

#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
    } msr[];
#elif defined(__GNUC__)
    } msr[0];
#else
    } msr[1 /* variable size */];
#endif

I guess using the same should be fine.

Note that I'm also adding a new field to the struct here (nr_pins) so this is
going to look a little bit weird IMHO:

union vioapic_redir_entry
{
    uint64_t bits;
    struct {
        uint8_t vector;
        uint8_t delivery_mode:3;
        uint8_t dest_mode:1;
        uint8_t delivery_status:1;
        uint8_t polarity:1;
        uint8_t remote_irr:1;
        uint8_t trig_mode:1;
        uint8_t mask:1;
        uint8_t reserve:7;
        uint8_t reserved[4];
        uint8_t dest_id;
    } fields;
};

struct hvm_hw_vioapic {
    uint64_t base_address;
    uint32_t ioregsel;
    uint32_t id;
    uint32_t nr_pins;

#ifndef __XEN__
    union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
#else
    union {
        union vioapic_redir_entry gredirtbl[VIOAPIC_NUM_PINS];
        union vioapic_redir_entry
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
        hredirtbl[];
#elif defined(__GNUC__)
        hredirtbl[0];
#else
        hredirtbl[1 /* variable size */];
#endif
#endif
};

Wouldn't it be better to keep hvm_hw_vioapic as-is, and introduce a new
hvm_vioapic struct in vioapic.h, that's used only internally by Xen?

vioapic_{save/load} would need to perform the translation to/from
hvm_hw_ioapic, but I wouldn't need to add any compatibility stuff in save.h.

And I could just declare the hvm_vioapic struct as:

struct hvm_hw_vioapic {
    uint64_t base_address;
    uint32_t ioregsel;
    uint32_t id;
    uint32_t nr_pins;
    union vioapic_redir_entry
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
    hredirtbl[];
#elif defined(__GNUC__)
    hredirtbl[0];
#else
    hredirtbl[1 /* variable size */];
#endif
};

In vioapic.h

> >> Another alternative would be to introduce a Xen internal sibling
> >> struct with a variable size array, and with all fields properly build-
> >> time-asserted to be identical between both.
> >> 
> >> I'll skip the rest of the patch assuming much of it could be dropped
> >> this way.
> > 
> > That was indeed my first approach, but later patches introduce an array of
> > hvm_hw_vioapic structs (in order to support multiple IO APICs per domain), and
> > this doesn't work if the struct itself contains a variable size array.
> 
> By what are you indexing that array? How about using a linked list
> instead (or see below)? There won't normally be many entries, so
> traversal is not an issue.

Right, I would rather prefer an array of pointers to hvm_hw_vioapic, so
domain_vioapic doesn't need to traverse the list. That way I can add the
variable size array without issues.

> > I can
> > encapsulate this inside of a hvm_vioapic struct, like:
> > 
> > struct hvm_vioapic {
> >     struct hvm_hw_ioapic *vioapic;
> > }
> > 
> > And make and array of hvm_vioapics instead, but that seemed more cumbersome.
> 
> Well, you don't need a structure here at all. Just have an array
> of pointers. In any event I think the respective public interface
> change should be avoided here if at all possible.

Right, thanks!

Roger.
Jan Beulich March 3, 2017, 3:58 p.m. UTC | #5
>>> On 03.03.17 at 16:30, <roger.pau@citrix.com> wrote:
> On Fri, Mar 03, 2017 at 06:02:58AM -0700, Jan Beulich wrote:
>> >>> On 03.03.17 at 13:53, <roger.pau@citrix.com> wrote:
>> > On Fri, Mar 03, 2017 at 04:56:20AM -0700, Jan Beulich wrote:
>> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
>> >> > @@ -424,40 +424,141 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>> >> >      spin_unlock(&d->arch.hvm_domain.irq_lock);
>> >> >  }
>> >> >  
>> >> > -static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
>> >> > +#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)
>> >> 
>> >> What is this redirtbl field, which oddly enough is a pointer (not allowed
>> >> in the public interface) good for? I can't seem to find any use of it
>> >> other than as marker (for use with offsetof()). With all the
>> >> complications here and below plus the fixed number of entries
>> >> remaining to be fixed for actual migration purposes I wonder if you
>> >> wouldn't be better off by keeping the structure mostly as is, simply
>> >> converting the redirtbl[] field to a union (guarded by a __XEN__
>> >> conditional) containing both a fixed size array and a variable size
>> >> one (or to be precise, a zero size one, as a variable size one is not
>> >> allowed there).
> 
> In that same file hvm_msr struct is using a variable size array defined as
> follows:
> 
> #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
>     } msr[];
> #elif defined(__GNUC__)
>     } msr[0];
> #else
>     } msr[1 /* variable size */];
> #endif
> 
> I guess using the same should be fine.

Well, the structures here are for migration, and you don't wan to
change the data that gets migrated, so preferably you'd leave the
structures here alone, also ...

> Note that I'm also adding a new field to the struct here (nr_pins) so this is

... in this regard, not the least to ...

> going to look a little bit weird IMHO:

... avoid this weirdness.

Jan
diff mbox

Patch

diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
index 32be120..8135a0e 100644
--- a/tools/misc/xen-hvmctx.c
+++ b/tools/misc/xen-hvmctx.c
@@ -227,14 +227,31 @@  static void dump_pic(void)
 static void dump_ioapic(void) 
 {
     int i;
-    HVM_SAVE_TYPE(IOAPIC) p;
-    READ(p);
+    struct hvm_hw_vioapic p;
+    union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
+
+    /*
+     * NB: due to the fact that the IO APIC struct can have a variable number
+     * of pins (in order to support PVHv2 Dom0), the migration code needs to
+     * support this structure, although migration of guests with a number of
+     * pins different than VIOAPIC_NUM_PINS is not supported.
+     */
+    memcpy(&p, buf + off, offsetof(struct hvm_hw_vioapic, redirtbl));
+    off += offsetof(struct hvm_hw_vioapic, redirtbl);
+    if ( p.nr_pins != VIOAPIC_NUM_PINS )
+    {
+        printf("Invalid number of IO APIC pins %u\n", p.nr_pins);
+        exit(EXIT_FAILURE);
+    }
+    memcpy(redirtbl, buf + off, sizeof(redirtbl));
+    off += sizeof(redirtbl);
+
     printf("    IOAPIC: base_address %#llx, ioregsel %#x id %#x\n",
            (unsigned long long) p.base_address, p.ioregsel, p.id);
     for ( i = 0; i < VIOAPIC_NUM_PINS; i++ )
     {
         printf("            pin %.2i: 0x%.16llx\n", i, 
-               (unsigned long long) p.redirtbl[i].bits);
+               (unsigned long long) redirtbl[i].bits);
     }
 }
 
@@ -453,7 +470,7 @@  int main(int argc, char **argv)
         case HVM_SAVE_CODE(HEADER): dump_header(); break;
         case HVM_SAVE_CODE(CPU): dump_cpu(); break;
         case HVM_SAVE_CODE(PIC): dump_pic(); break;
-        case HVM_SAVE_CODE(IOAPIC): dump_ioapic(); break;
+        case IOAPIC_CODE: dump_ioapic(); break;
         case HVM_SAVE_CODE(LAPIC): dump_lapic(); break;
         case HVM_SAVE_CODE(LAPIC_REGS): dump_lapic_regs(); break;
         case HVM_SAVE_CODE(PCI_IRQ): dump_pci_irq(); break;
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 9677227..f469cbf 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -53,7 +53,7 @@  static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic)
     case VIOAPIC_REG_VERSION:
         result = ((union IO_APIC_reg_01){
                   .bits = { .version = VIOAPIC_VERSION_ID,
-                            .entries = VIOAPIC_NUM_PINS - 1 }
+                            .entries = vioapic->nr_pins - 1 }
                   }).raw;
         break;
 
@@ -198,7 +198,7 @@  static void vioapic_write_indirect(struct domain *d, uint32_t val)
         HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
                     redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
 
-        if ( redir_index >= VIOAPIC_NUM_PINS )
+        if ( redir_index >= vioapic->nr_pins )
         {
             gdprintk(XENLOG_WARNING, "vioapic_write_indirect "
                      "error register %x\n", vioapic->ioregsel);
@@ -368,7 +368,7 @@  void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
 
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "irq %x", irq);
 
-    ASSERT(irq < VIOAPIC_NUM_PINS);
+    ASSERT(irq < vioapic->nr_pins);
     ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
 
     ent = &vioapic->redirtbl[irq];
@@ -397,7 +397,7 @@  void vioapic_update_EOI(struct domain *d, u8 vector)
 
     spin_lock(&d->arch.hvm_domain.irq_lock);
 
-    for ( gsi = 0; gsi < VIOAPIC_NUM_PINS; gsi++ )
+    for ( gsi = 0; gsi < vioapic->nr_pins; gsi++ )
     {
         ent = &vioapic->redirtbl[gsi];
         if ( ent->fields.vector != vector )
@@ -424,40 +424,141 @@  void vioapic_update_EOI(struct domain *d, u8 vector)
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
 
-static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
+#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)
+#define VIOAPIC_SAVE_VAR(cnt) (sizeof(union vioapic_redir_entry) * (cnt))
+#define VIOAPIC_SAVE_SIZE(cnt) (VIOAPIC_SAVE_CONST + VIOAPIC_SAVE_VAR(cnt))
+
+static int vioapic_save(struct domain *d, hvm_domain_context_t *h)
 {
-    struct hvm_hw_vioapic *s = domain_vioapic(d);
+    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
 
     if ( !has_vioapic(d) )
         return 0;
 
-    return hvm_save_entry(IOAPIC, 0, h, s);
+    if ( vioapic->nr_pins != VIOAPIC_NUM_PINS )
+        return -ENOSYS;
+
+    if ( _hvm_init_entry(h, IOAPIC_CODE, 0,
+                         VIOAPIC_SAVE_SIZE(vioapic->nr_pins)) )
+        return 1;
+
+    memcpy(&h->data[h->cur], vioapic, VIOAPIC_SAVE_CONST);
+    h->cur += VIOAPIC_SAVE_CONST;
+    memcpy(&h->data[h->cur], vioapic->redirtbl,
+           VIOAPIC_SAVE_VAR(vioapic->nr_pins));
+    h->cur += VIOAPIC_SAVE_VAR(vioapic->nr_pins);
+
+    return 0;
 }
 
-static int ioapic_load(struct domain *d, hvm_domain_context_t *h)
+static int vioapic_load(struct domain *d, hvm_domain_context_t *h)
 {
-    struct hvm_hw_vioapic *s = domain_vioapic(d);
+    unsigned int ioapic_nr = hvm_load_instance(h);
+    const struct hvm_save_descriptor *desc;
+    struct hvm_hw_vioapic_compat *ioapic_compat;
+    struct hvm_hw_vioapic *ioapic = domain_vioapic(d);
 
     if ( !has_vioapic(d) )
         return -ENODEV;
 
-    return hvm_load_entry(IOAPIC, h, s);
+    if ( ioapic_nr != 0 )
+        return -ENOSYS;
+
+    desc = (struct hvm_save_descriptor *)&h->data[h->cur];
+    if ( sizeof (*desc) > h->size - h->cur)
+    {
+        printk(XENLOG_G_WARNING
+               "HVM%d restore: not enough data left to read IOAPIC descriptor\n",
+               d->domain_id);
+        return -ENODATA;
+    }
+    if ( desc->length + sizeof (*desc) > h->size - h->cur)
+    {
+        printk(XENLOG_G_WARNING
+               "HVM%d restore: not enough data left to read %u IOAPIC bytes\n",
+               d->domain_id, desc->length);
+        return -ENODATA;
+    }
+    if ( desc->length < sizeof(*ioapic_compat) )
+    {
+        printk(XENLOG_G_WARNING
+               "HVM%d restore mismatch: IOAPIC length %u < %lu\n",
+               d->domain_id, desc->length, sizeof(*ioapic_compat));
+        return -EINVAL;
+    }
+
+    h->cur += sizeof(*desc);
+
+    switch ( desc->length )
+    {
+    case sizeof(*ioapic_compat):
+        ioapic_compat = (struct hvm_hw_vioapic_compat *)&h->data[h->cur];
+        ioapic->base_address = ioapic_compat->base_address;
+        ioapic->ioregsel = ioapic_compat->ioregsel;
+        ioapic->id = ioapic_compat->id;
+        ioapic->nr_pins = VIOAPIC_NUM_PINS;
+        memcpy(ioapic->redirtbl, ioapic_compat->redirtbl,
+               sizeof(ioapic_compat->redirtbl));
+        h->cur += sizeof(*ioapic_compat);
+        break;
+    case VIOAPIC_SAVE_SIZE(VIOAPIC_NUM_PINS):
+        memcpy(ioapic, &h->data[h->cur], VIOAPIC_SAVE_CONST);
+        h->cur += VIOAPIC_SAVE_CONST;
+        if ( ioapic->nr_pins != VIOAPIC_NUM_PINS )
+        {
+            printk(XENLOG_G_WARNING
+                   "HVM%d restore mismatch: unexpected number of IO APIC entries: %u\n",
+                   d->domain_id, ioapic->nr_pins);
+            return -EINVAL;
+        }
+        memcpy(ioapic->redirtbl, &h->data[h->cur],
+               VIOAPIC_SAVE_VAR(ioapic->nr_pins));
+        h->cur += VIOAPIC_SAVE_VAR(ioapic->nr_pins);
+        break;
+    default:
+        printk(XENLOG_G_WARNING "HVM%d restore mismatch: IO APIC length\n",
+               d->domain_id);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+/*
+ * We need variable length (variable number of pins) IO APICs, although
+ * those would only be used by the hardware domain, so migration wise
+ * we are always going to use VIOAPIC_NUM_PINS.
+ */
+static int __init vioapic_register_save_and_restore(void)
+{
+    hvm_register_savevm(IOAPIC_CODE, "IOAPIC", vioapic_save, vioapic_load,
+                        VIOAPIC_SAVE_SIZE(VIOAPIC_NUM_PINS) +
+                            sizeof(struct hvm_save_descriptor),
+                        HVMSR_PER_DOM);
+
+    return 0;
 }
+__initcall(vioapic_register_save_and_restore);
 
-HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
+#undef VIOAPIC_SAVE_CONST
+#undef VIOAPIC_SAVE_VAR
+#undef VIOAPIC_SAVE_SIZE
 
 void vioapic_reset(struct domain *d)
 {
     struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
-    int i;
+    unsigned int i;
 
     if ( !has_vioapic(d) )
         return;
 
-    memset(vioapic, 0, sizeof(*vioapic));
-    for ( i = 0; i < VIOAPIC_NUM_PINS; i++ )
+    memset(vioapic->redirtbl, 0,
+           sizeof(*vioapic->redirtbl) * vioapic->nr_pins);
+    for ( i = 0; i < vioapic->nr_pins; i++ )
         vioapic->redirtbl[i].fields.mask = 1;
     vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
+    vioapic->id = 0;
+    vioapic->ioregsel = 0;
 }
 
 int vioapic_init(struct domain *d)
@@ -470,6 +571,15 @@  int vioapic_init(struct domain *d)
            xmalloc(struct hvm_hw_vioapic)) == NULL) )
         return -ENOMEM;
 
+    domain_vioapic(d)->redirtbl = xmalloc_array(union vioapic_redir_entry,
+                                                VIOAPIC_NUM_PINS);
+    if ( !domain_vioapic(d)->redirtbl )
+    {
+        xfree(d->arch.hvm_domain.vioapic);
+        return -ENOMEM;
+    }
+
+    domain_vioapic(d)->nr_pins = VIOAPIC_NUM_PINS;
     vioapic_reset(d);
 
     register_mmio_handler(d, &vioapic_mmio_ops);
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 419a3b2..a218804 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -363,30 +363,44 @@  DECLARE_HVM_SAVE_TYPE(PIC, 3, struct hvm_hw_vpic);
 
 #define VIOAPIC_NUM_PINS  48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */
 
+/*
+ * Needs to be declared independently of the ioapic structs, or else the
+ * compat check fails.
+ */
+union vioapic_redir_entry
+{
+    uint64_t bits;
+    struct {
+        uint8_t vector;
+        uint8_t delivery_mode:3;
+        uint8_t dest_mode:1;
+        uint8_t delivery_status:1;
+        uint8_t polarity:1;
+        uint8_t remote_irr:1;
+        uint8_t trig_mode:1;
+        uint8_t mask:1;
+        uint8_t reserve:7;
+        uint8_t reserved[4];
+        uint8_t dest_id;
+    } fields;
+};
+
 struct hvm_hw_vioapic {
     uint64_t base_address;
     uint32_t ioregsel;
     uint32_t id;
-    union vioapic_redir_entry
-    {
-        uint64_t bits;
-        struct {
-            uint8_t vector;
-            uint8_t delivery_mode:3;
-            uint8_t dest_mode:1;
-            uint8_t delivery_status:1;
-            uint8_t polarity:1;
-            uint8_t remote_irr:1;
-            uint8_t trig_mode:1;
-            uint8_t mask:1;
-            uint8_t reserve:7;
-            uint8_t reserved[4];
-            uint8_t dest_id;
-        } fields;
-    } redirtbl[VIOAPIC_NUM_PINS];
+    uint32_t nr_pins;
+    union vioapic_redir_entry *redirtbl;
+};
+
+struct hvm_hw_vioapic_compat {
+    uint64_t base_address;
+    uint32_t ioregsel;
+    uint32_t id;
+    union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
 };
 
-DECLARE_HVM_SAVE_TYPE(IOAPIC, 4, struct hvm_hw_vioapic);
+#define IOAPIC_CODE  4
 
 
 /*