[PATCH 0/5] x86/vioapic: introduce support for multiple vIO APICs

[PATCH 0/5] x86/vioapic: introduce support for multiple vIO APICs

[Roger Pau Monne]

Hello,

This patch series introduce support for having a variable number of entries in
vIO APICs, and also having a variable number of vIO APICs per domain. This
functionality is not used by unprivileged guests, that are still limited to a
single IO APIC with 49 entries.

The functionality introduced is only used by PVHv2 Dom0, in order to copy the
IO APIC topology found on bare metal.

The following series has been tested by the Citrix internal osstest instance,
without finding any issues.

They can also be found in my personal git tree:

git://xenbits.xen.org/people/royger/xen.git vioapics_v1

This is based on top of the PVHv2 initial Dom0 support series.

Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/5] x86/vioapic: move domain out of hvm_vioapic struct

[Roger Pau Monne]

And then remove hvm_vioapic (since it just contains a hvm_hw_ioapic struct
now). This is a preparatory change for introducing support for multiple vIO
APICs per domain.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vioapic.c        | 40 +++++++++++++++++++--------------------
 xen/include/asm-x86/hvm/domain.h  |  2 +-
 xen/include/asm-x86/hvm/vioapic.h |  9 +--------
 3 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index fdbb21f..9677227 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -42,7 +42,7 @@
 /* HACK: Route IRQ0 only to VCPU0 to prevent time jumps. */
 #define IRQ0_SPECIAL_ROUTING 1
 
-static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq);
+static void vioapic_deliver(struct domain *d, int irq);
 
 static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic)
 {
@@ -119,10 +119,10 @@ static int vioapic_read(
 }
 
 static void vioapic_write_redirent(
-    struct hvm_hw_vioapic *vioapic, unsigned int idx,
+    struct domain *d, unsigned int idx,
     int top_word, uint32_t val)
 {
-    struct domain *d = vioapic_domain(vioapic);
+    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     union vioapic_redir_entry *pent, ent;
     int unmasked = 0;
@@ -160,7 +160,7 @@ static void vioapic_write_redirent(
               hvm_irq->gsi_assert_count[idx] )
     {
         pent->fields.remote_irr = 1;
-        vioapic_deliver(vioapic, idx);
+        vioapic_deliver(d, idx);
     }
 
     spin_unlock(&d->arch.hvm_domain.irq_lock);
@@ -169,9 +169,10 @@ static void vioapic_write_redirent(
         pt_may_unmask_irq(d, NULL);
 }
 
-static void vioapic_write_indirect(
-    struct hvm_hw_vioapic *vioapic, uint32_t val)
+static void vioapic_write_indirect(struct domain *d, uint32_t val)
 {
+    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
+
     switch ( vioapic->ioregsel )
     {
     case VIOAPIC_REG_VERSION:
@@ -204,8 +205,7 @@ static void vioapic_write_indirect(
             break;
         }
 
-        vioapic_write_redirent(
-            vioapic, redir_index, vioapic->ioregsel&1, val);
+        vioapic_write_redirent(d, redir_index, vioapic->ioregsel&1, val);
         break;
     }
     }
@@ -224,7 +224,7 @@ static int vioapic_write(
         break;
 
     case VIOAPIC_REG_WINDOW:
-        vioapic_write_indirect(vioapic, val);
+        vioapic_write_indirect(v->domain, val);
         break;
 
 #if VIOAPIC_VERSION_ID >= 0x20
@@ -275,14 +275,14 @@ static inline int pit_channel0_enabled(void)
     return pt_active(&current->domain->arch.vpit.pt0);
 }
 
-static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq)
+static void vioapic_deliver(struct domain *d, int irq)
 {
+    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
     uint16_t dest = vioapic->redirtbl[irq].fields.dest_id;
     uint8_t dest_mode = vioapic->redirtbl[irq].fields.dest_mode;
     uint8_t delivery_mode = vioapic->redirtbl[irq].fields.delivery_mode;
     uint8_t vector = vioapic->redirtbl[irq].fields.vector;
     uint8_t trig_mode = vioapic->redirtbl[irq].fields.trig_mode;
-    struct domain *d = vioapic_domain(vioapic);
     struct vlapic *target;
     struct vcpu *v;
 
@@ -377,12 +377,12 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
 
     if ( ent->fields.trig_mode == VIOAPIC_EDGE_TRIG )
     {
-        vioapic_deliver(vioapic, irq);
+        vioapic_deliver(d, irq);
     }
     else if ( !ent->fields.remote_irr )
     {
         ent->fields.remote_irr = 1;
-        vioapic_deliver(vioapic, irq);
+        vioapic_deliver(d, irq);
     }
 }
 
@@ -417,7 +417,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
              hvm_irq->gsi_assert_count[gsi] )
         {
             ent->fields.remote_irr = 1;
-            vioapic_deliver(vioapic, gsi);
+            vioapic_deliver(d, gsi);
         }
     }
 
@@ -448,16 +448,16 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
 
 void vioapic_reset(struct domain *d)
 {
-    struct hvm_vioapic *vioapic = d->arch.hvm_domain.vioapic;
+    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
     int i;
 
     if ( !has_vioapic(d) )
         return;
 
-    memset(&vioapic->hvm_hw_vioapic, 0, sizeof(vioapic->hvm_hw_vioapic));
+    memset(vioapic, 0, sizeof(*vioapic));
     for ( i = 0; i < VIOAPIC_NUM_PINS; i++ )
-        vioapic->hvm_hw_vioapic.redirtbl[i].fields.mask = 1;
-    vioapic->hvm_hw_vioapic.base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
+        vioapic->redirtbl[i].fields.mask = 1;
+    vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
 }
 
 int vioapic_init(struct domain *d)
@@ -466,10 +466,10 @@ int vioapic_init(struct domain *d)
         return 0;
 
     if ( (d->arch.hvm_domain.vioapic == NULL) &&
-         ((d->arch.hvm_domain.vioapic = xmalloc(struct hvm_vioapic)) == NULL) )
+         ((d->arch.hvm_domain.vioapic =
+           xmalloc(struct hvm_hw_vioapic)) == NULL) )
         return -ENOMEM;
 
-    d->arch.hvm_domain.vioapic->domain = d;
     vioapic_reset(d);
 
     register_mmio_handler(d, &vioapic_mmio_ops);
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 420cbdc..5cf3398 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -127,7 +127,7 @@ struct hvm_domain {
     spinlock_t             irq_lock;
     struct hvm_irq         irq;
     struct hvm_hw_vpic     vpic[2]; /* 0=master; 1=slave */
-    struct hvm_vioapic    *vioapic;
+    struct hvm_hw_vioapic *vioapic;
     struct hvm_hw_stdvga   stdvga;
 
     /*
diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h
index 745c09a..6762835 100644
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -47,14 +47,7 @@
 #define VIOAPIC_REG_ARB_ID  0x02 /* x86 IOAPIC only */
 #define VIOAPIC_REG_RTE0    0x10
 
-struct hvm_vioapic {
-    struct hvm_hw_vioapic hvm_hw_vioapic;
-    struct domain *domain;
-};
-
-#define domain_vioapic(d) (&(d)->arch.hvm_domain.vioapic->hvm_hw_vioapic)
-#define vioapic_domain(v) (container_of((v), struct hvm_vioapic, \
-                                        hvm_hw_vioapic)->domain)
+#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic)
 
 int vioapic_init(struct domain *d);
 void vioapic_deinit(struct domain *d);
-- 
2.10.1 (Apple Git-78)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/vioapic: move domain out of hvm_vioapic struct

[Jan Beulich]

>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> @@ -204,8 +205,7 @@ static void vioapic_write_indirect(
>              break;
>          }
>  
> -        vioapic_write_redirent(
> -            vioapic, redir_index, vioapic->ioregsel&1, val);
> +        vioapic_write_redirent(d, redir_index, vioapic->ioregsel&1, val);

Please correct obvious coding style violations in cases like this. Of
course here it can be taken care of while committing.

> @@ -224,7 +224,7 @@ static int vioapic_write(
>          break;
>  
>      case VIOAPIC_REG_WINDOW:
> -        vioapic_write_indirect(vioapic, val);
> +        vioapic_write_indirect(v->domain, val);

I'll assume this will clarify itself with later patches: At this point it
feels wrong to lose the IO-APIC pointer in places like this, as from
domain and value alone you won't be able to reconstruct it. I.e.
I would have considered it more natural if you simply added
another function parameter.

I also wonder whether in at least some of the cases the new
struct domain * parameters of functions couldn't be const-
qualified.

In any event I'll want to have looked at more of this series to
understand whether the patch here is okay in its current form.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/vioapic: move domain out of hvm_vioapic struct

[Jan Beulich]

>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> And then remove hvm_vioapic (since it just contains a hvm_hw_ioapic struct
> now). This is a preparatory change for introducing support for multiple vIO
> APICs per domain.

Having gone through the rest of this series, I think the replacing of
vioapic pointers by domain ones goes too far here. Especially the
low level functions really ought to be dealing with individual IO-APICs
instead of their entire set. Hence where a domain pointer is needed,
I think it should be added without removing the vioapic one.

In the end - what's the fundamental need you try to address with
the patch here? I.e. with multiple IO-APICs, what's wrong with still
having a "back pointer" to the domain in each of the structures?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/5] x86/vioapic: move domain out of hvm_vioapic struct

[Roger Pau Monne]

On Mon, Mar 06, 2017 at 09:43:43AM -0700, Jan Beulich wrote:
> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> > And then remove hvm_vioapic (since it just contains a hvm_hw_ioapic struct
> > now). This is a preparatory change for introducing support for multiple vIO
> > APICs per domain.
> 
> Having gone through the rest of this series, I think the replacing of
> vioapic pointers by domain ones goes too far here. Especially the
> low level functions really ought to be dealing with individual IO-APICs
> instead of their entire set. Hence where a domain pointer is needed,
> I think it should be added without removing the vioapic one.
> 
> In the end - what's the fundamental need you try to address with
> the patch here? I.e. with multiple IO-APICs, what's wrong with still
> having a "back pointer" to the domain in each of the structures?

It looked kind of redundant to have a domain back pointer in each of the
structures, but maybe it's best to do it that way, since I expect less changes
will be needed overall. Let me try to apply the requested changes to the layout
of the structures and I will get back to you.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Roger Pau Monne]

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

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
 
 
 /*
-- 
2.10.1 (Apple Git-78)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Roger Pau Monne]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Roger Pau Monne]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Roger Pau Monne]

Add support for multiple vIO APICs on the same domain, thus turning
d->arch.hvm_domain.vioapic into an array of vIO APIC control structures.

Note that this functionality is not exposed to unprivileged guests, and will
only be used by PVHv2 Dom0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain_build.c       |   2 +-
 xen/arch/x86/hvm/vioapic.c        | 204 +++++++++++++++++++++++++++-----------
 xen/arch/x86/hvm/vlapic.c         |   2 +-
 xen/arch/x86/hvm/vpt.c            |  13 ++-
 xen/include/asm-x86/hvm/domain.h  |   1 +
 xen/include/asm-x86/hvm/vioapic.h |   4 +-
 6 files changed, 160 insertions(+), 66 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 932fa4e..dad3b4e 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -2317,7 +2317,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
     io_apic = (void *)(madt + 1);
     io_apic->header.type = ACPI_MADT_TYPE_IO_APIC;
     io_apic->header.length = sizeof(*io_apic);
-    io_apic->id = domain_vioapic(d)->id;
+    io_apic->id = domain_vioapic(d, 0)->id;
     io_apic->address = VIOAPIC_DEFAULT_BASE_ADDRESS;
 
     x2apic = (void *)(io_apic + 1);
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index f469cbf..6421971 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -44,6 +44,56 @@
 
 static void vioapic_deliver(struct domain *d, int irq);
 
+static struct hvm_hw_vioapic *addr_vioapic(struct domain *d,
+                                           unsigned long addr)
+{
+    unsigned int i;
+
+    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
+    {
+        struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i);
+
+        if ( addr >= vioapic->base_address &&
+             addr < vioapic->base_address + VIOAPIC_MEM_LENGTH )
+            return vioapic;
+    }
+
+    return NULL;
+}
+
+struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi,
+                                   unsigned int *pin)
+{
+    unsigned int i, base_gsi = 0;
+
+    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
+    {
+        struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i);
+
+        if ( gsi >= base_gsi && gsi < base_gsi + vioapic->nr_pins )
+        {
+            if ( pin != NULL )
+                *pin = gsi - base_gsi;
+            return vioapic;
+        }
+        base_gsi += vioapic->nr_pins;
+    }
+
+    return NULL;
+}
+
+static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic,
+                            unsigned int pin)
+{
+    struct hvm_hw_vioapic *tmp;
+    unsigned int base_gsi = 0;
+
+    for ( tmp = domain_vioapic(d, 0); tmp != vioapic; tmp++ )
+        base_gsi += tmp->nr_pins;
+
+    return base_gsi + pin;
+}
+
 static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic)
 {
     uint32_t result = 0;
@@ -94,11 +144,14 @@ static int vioapic_read(
     struct vcpu *v, unsigned long addr,
     unsigned int length, unsigned long *pval)
 {
-    const struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
+    const struct hvm_hw_vioapic *vioapic;
     uint32_t result;
 
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "addr %lx", addr);
 
+    vioapic = addr_vioapic(v->domain, addr);
+    ASSERT(vioapic);
+
     switch ( addr & 0xff )
     {
     case VIOAPIC_REG_SELECT:
@@ -119,13 +172,13 @@ static int vioapic_read(
 }
 
 static void vioapic_write_redirent(
-    struct domain *d, unsigned int idx,
+    struct domain *d, struct hvm_hw_vioapic *vioapic, unsigned int idx,
     int top_word, uint32_t val)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     union vioapic_redir_entry *pent, ent;
     int unmasked = 0;
+    unsigned int gsi = pin_gsi(d, vioapic, idx);
 
     spin_lock(&d->arch.hvm_domain.irq_lock);
 
@@ -149,7 +202,7 @@ static void vioapic_write_redirent(
 
     *pent = ent;
 
-    if ( idx == 0 )
+    if ( gsi == 0 )
     {
         vlapic_adjust_i8259_target(d);
     }
@@ -157,21 +210,22 @@ static void vioapic_write_redirent(
         pent->fields.remote_irr = 0;
     else if ( !ent.fields.mask &&
               !ent.fields.remote_irr &&
-              hvm_irq->gsi_assert_count[idx] )
+              hvm_irq->gsi_assert_count[gsi] )
     {
         pent->fields.remote_irr = 1;
-        vioapic_deliver(d, idx);
+        vioapic_deliver(d, gsi);
     }
 
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 
-    if ( idx == 0 || unmasked )
+    if ( gsi == 0 || unmasked )
         pt_may_unmask_irq(d, NULL);
 }
 
-static void vioapic_write_indirect(struct domain *d, uint32_t val)
+static void vioapic_write_indirect(struct domain *d, unsigned long addr,
+                                   uint32_t val)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
+    struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr);
 
     switch ( vioapic->ioregsel )
     {
@@ -205,7 +259,8 @@ static void vioapic_write_indirect(struct domain *d, uint32_t val)
             break;
         }
 
-        vioapic_write_redirent(d, redir_index, vioapic->ioregsel&1, val);
+        vioapic_write_redirent(d, vioapic, redir_index, vioapic->ioregsel&1,
+                               val);
         break;
     }
     }
@@ -215,7 +270,10 @@ static int vioapic_write(
     struct vcpu *v, unsigned long addr,
     unsigned int length, unsigned long val)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
+    struct hvm_hw_vioapic *vioapic;
+
+    vioapic = addr_vioapic(v->domain, addr);
+    ASSERT(vioapic);
 
     switch ( addr & 0xff )
     {
@@ -224,7 +282,7 @@ static int vioapic_write(
         break;
 
     case VIOAPIC_REG_WINDOW:
-        vioapic_write_indirect(v->domain, val);
+        vioapic_write_indirect(v->domain, addr, val);
         break;
 
 #if VIOAPIC_VERSION_ID >= 0x20
@@ -242,10 +300,7 @@ static int vioapic_write(
 
 static int vioapic_range(struct vcpu *v, unsigned long addr)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
-
-    return ((addr >= vioapic->base_address &&
-             (addr < vioapic->base_address + VIOAPIC_MEM_LENGTH)));
+    return !!addr_vioapic(v->domain, addr);
 }
 
 static const struct hvm_mmio_ops vioapic_mmio_ops = {
@@ -277,12 +332,13 @@ static inline int pit_channel0_enabled(void)
 
 static void vioapic_deliver(struct domain *d, int irq)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
-    uint16_t dest = vioapic->redirtbl[irq].fields.dest_id;
-    uint8_t dest_mode = vioapic->redirtbl[irq].fields.dest_mode;
-    uint8_t delivery_mode = vioapic->redirtbl[irq].fields.delivery_mode;
-    uint8_t vector = vioapic->redirtbl[irq].fields.vector;
-    uint8_t trig_mode = vioapic->redirtbl[irq].fields.trig_mode;
+    unsigned int pin;
+    struct hvm_hw_vioapic *vioapic = gsi_vioapic(d, irq, &pin);
+    uint16_t dest = vioapic->redirtbl[pin].fields.dest_id;
+    uint8_t dest_mode = vioapic->redirtbl[pin].fields.dest_mode;
+    uint8_t delivery_mode = vioapic->redirtbl[pin].fields.delivery_mode;
+    uint8_t vector = vioapic->redirtbl[pin].fields.vector;
+    uint8_t trig_mode = vioapic->redirtbl[pin].fields.trig_mode;
     struct vlapic *target;
     struct vcpu *v;
 
@@ -361,17 +417,18 @@ static void vioapic_deliver(struct domain *d, int irq)
 
 void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
+    unsigned int pin;
+    struct hvm_hw_vioapic *vioapic = gsi_vioapic(d, irq, &pin);
     union vioapic_redir_entry *ent;
 
     ASSERT(has_vioapic(d));
 
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "irq %x", irq);
 
-    ASSERT(irq < vioapic->nr_pins);
+    ASSERT(pin < vioapic->nr_pins);
     ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
 
-    ent = &vioapic->redirtbl[irq];
+    ent = &vioapic->redirtbl[pin];
     if ( ent->fields.mask )
         return;
 
@@ -388,37 +445,43 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
 
 void vioapic_update_EOI(struct domain *d, u8 vector)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     union vioapic_redir_entry *ent;
-    int gsi;
+    unsigned int i, base_gsi = 0;
 
     ASSERT(has_vioapic(d));
 
     spin_lock(&d->arch.hvm_domain.irq_lock);
 
-    for ( gsi = 0; gsi < vioapic->nr_pins; gsi++ )
+    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
     {
-        ent = &vioapic->redirtbl[gsi];
-        if ( ent->fields.vector != vector )
-            continue;
-
-        ent->fields.remote_irr = 0;
+        struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i);
+        unsigned int j;
 
-        if ( iommu_enabled )
+        for ( j = 0; j < vioapic->nr_pins; j++ )
         {
-            spin_unlock(&d->arch.hvm_domain.irq_lock);
-            hvm_dpci_eoi(d, gsi, ent);
-            spin_lock(&d->arch.hvm_domain.irq_lock);
-        }
-
-        if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
-             !ent->fields.mask &&
-             hvm_irq->gsi_assert_count[gsi] )
-        {
-            ent->fields.remote_irr = 1;
-            vioapic_deliver(d, gsi);
+            ent = &vioapic->redirtbl[j];
+            if ( ent->fields.vector != vector )
+                continue;
+
+            ent->fields.remote_irr = 0;
+
+            if ( iommu_enabled )
+            {
+                spin_unlock(&d->arch.hvm_domain.irq_lock);
+                hvm_dpci_eoi(d, base_gsi + j, ent);
+                spin_lock(&d->arch.hvm_domain.irq_lock);
+            }
+
+            if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
+                 !ent->fields.mask &&
+                 hvm_irq->gsi_assert_count[base_gsi + j] )
+            {
+                ent->fields.remote_irr = 1;
+                vioapic_deliver(d, base_gsi + j);
+            }
         }
+        base_gsi += vioapic->nr_pins;
     }
 
     spin_unlock(&d->arch.hvm_domain.irq_lock);
@@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
 
 static int vioapic_save(struct domain *d, hvm_domain_context_t *h)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
+    struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0);
 
     if ( !has_vioapic(d) )
         return 0;
 
+    if ( d->arch.hvm_domain.nr_vioapics != 1 )
+        return -ENOSYS;
+
     if ( vioapic->nr_pins != VIOAPIC_NUM_PINS )
         return -ENOSYS;
 
@@ -456,7 +522,7 @@ static int vioapic_load(struct domain *d, hvm_domain_context_t *h)
     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);
+    struct hvm_hw_vioapic *ioapic = domain_vioapic(d, 0);
 
     if ( !has_vioapic(d) )
         return -ENODEV;
@@ -546,40 +612,53 @@ __initcall(vioapic_register_save_and_restore);
 
 void vioapic_reset(struct domain *d)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
     unsigned int i;
 
     if ( !has_vioapic(d) )
+    {
+        ASSERT(!d->arch.hvm_domain.nr_vioapics);
         return;
+    }
 
-    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;
+    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
+    {
+        struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i);
+        unsigned int j;
+
+        memset(vioapic->redirtbl, 0,
+               sizeof(*vioapic->redirtbl) * vioapic->nr_pins);
+        for ( j = 0; j < vioapic->nr_pins; j++ )
+            vioapic->redirtbl[j].fields.mask = 1;
+        vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
+                                VIOAPIC_MEM_LENGTH * i;
+        vioapic->id = i;
+        vioapic->ioregsel = 0;
+    }
 }
 
 int vioapic_init(struct domain *d)
 {
     if ( !has_vioapic(d) )
+    {
+        d->arch.hvm_domain.nr_vioapics = 0;
         return 0;
+    }
 
     if ( (d->arch.hvm_domain.vioapic == NULL) &&
          ((d->arch.hvm_domain.vioapic =
            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 )
+    domain_vioapic(d, 0)->redirtbl = xmalloc_array(union vioapic_redir_entry,
+                                                   VIOAPIC_NUM_PINS);
+    if ( !domain_vioapic(d, 0)->redirtbl )
     {
         xfree(d->arch.hvm_domain.vioapic);
         return -ENOMEM;
     }
 
-    domain_vioapic(d)->nr_pins = VIOAPIC_NUM_PINS;
+    domain_vioapic(d, 0)->nr_pins = VIOAPIC_NUM_PINS;
+    d->arch.hvm_domain.nr_vioapics = 1;
     vioapic_reset(d);
 
     register_mmio_handler(d, &vioapic_mmio_ops);
@@ -589,9 +668,16 @@ int vioapic_init(struct domain *d)
 
 void vioapic_deinit(struct domain *d)
 {
+    unsigned int i;
+
     if ( !has_vioapic(d) )
+    {
+        ASSERT(!d->arch.hvm_domain.nr_vioapics);
         return;
+    }
 
+    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
+        xfree(domain_vioapic(d, i)->redirtbl);
     xfree(d->arch.hvm_domain.vioapic);
     d->arch.hvm_domain.vioapic = NULL;
 }
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 3fa3727..187f106 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
     if ( !has_vioapic(d) )
         return 0;
 
-    redir0 = domain_vioapic(d)->redirtbl[0];
+    redir0 = domain_vioapic(d, 0)->redirtbl[0];
 
     /* We deliver 8259 interrupts to the appropriate CPU as follows. */
     return ((/* IOAPIC pin0 is unmasked and routing to this LAPIC? */
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 5c48fdb..fd30dfb 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
 static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
 {
     struct vcpu *v = pt->vcpu;
-    unsigned int gsi, isa_irq;
+    struct hvm_hw_vioapic *vioapic;
+    unsigned int gsi, isa_irq, pin;
 
     if ( pt->source == PTSRC_lapic )
         return pt->irq;
@@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
                 + (isa_irq & 7));
 
     ASSERT(src == hvm_intsrc_lapic);
-    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
+    vioapic = gsi_vioapic(v->domain, gsi, &pin);
+
+    return vioapic->redirtbl[pin].fields.vector;
 }
 
 static int pt_irq_masked(struct periodic_time *pt)
 {
     struct vcpu *v = pt->vcpu;
-    unsigned int gsi, isa_irq;
+    unsigned int gsi, isa_irq, pin;
+    struct hvm_hw_vioapic *vioapic;
     uint8_t pic_imr;
 
     if ( pt->source == PTSRC_lapic )
@@ -110,9 +114,10 @@ static int pt_irq_masked(struct periodic_time *pt)
     isa_irq = pt->irq;
     gsi = hvm_isa_irq_to_gsi(isa_irq);
     pic_imr = v->domain->arch.hvm_domain.vpic[isa_irq >> 3].imr;
+    vioapic = gsi_vioapic(v->domain, gsi, &pin);
 
     return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) &&
-            domain_vioapic(v->domain)->redirtbl[gsi].fields.mask);
+            vioapic->redirtbl[pin].fields.mask);
 }
 
 static void pt_lock(struct periodic_time *pt)
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 5cf3398..cf1be37 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -128,6 +128,7 @@ struct hvm_domain {
     struct hvm_irq         irq;
     struct hvm_hw_vpic     vpic[2]; /* 0=master; 1=slave */
     struct hvm_hw_vioapic *vioapic;
+    unsigned int           nr_vioapics;
     struct hvm_hw_stdvga   stdvga;
 
     /*
diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h
index 6762835..6889b57 100644
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -47,7 +47,9 @@
 #define VIOAPIC_REG_ARB_ID  0x02 /* x86 IOAPIC only */
 #define VIOAPIC_REG_RTE0    0x10
 
-#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic)
+#define domain_vioapic(d, i) (&(d)->arch.hvm_domain.vioapic[i])
+struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi,
+                                   unsigned int *pin);
 
 int vioapic_init(struct domain *d);
 void vioapic_deinit(struct domain *d);
-- 
2.10.1 (Apple Git-78)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Jan Beulich]

>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi,

const struct domain *d ?

> +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic,

const for both?

> +                            unsigned int pin)
> +{
> +    struct hvm_hw_vioapic *tmp;

And here.

Also wouldn't this function more naturally (i.e. more generally useful)
simply return the base GSI, leaving it to the caller to add the pin
number?

> @@ -157,21 +210,22 @@ static void vioapic_write_redirent(
>          pent->fields.remote_irr = 0;
>      else if ( !ent.fields.mask &&
>                !ent.fields.remote_irr &&
> -              hvm_irq->gsi_assert_count[idx] )
> +              hvm_irq->gsi_assert_count[gsi] )

Neither this nor any earlier patch modified the size of this array, afaics.

> -static void vioapic_write_indirect(struct domain *d, uint32_t val)
> +static void vioapic_write_indirect(struct domain *d, unsigned long addr,
> +                                   uint32_t val)
>  {
> -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> +    struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr);

I think it would be better for the caller to pass the vioapic it
already established (and ASSERT()ed).

> @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>  
>  static int vioapic_save(struct domain *d, hvm_domain_context_t *h)
>  {
> -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> +    struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0);
>  
>      if ( !has_vioapic(d) )
>          return 0;
>  
> +    if ( d->arch.hvm_domain.nr_vioapics != 1 )
> +        return -ENOSYS;
> +
>      if ( vioapic->nr_pins != VIOAPIC_NUM_PINS )
>          return -ENOSYS;

Perhaps merge the two if()s? Otherwise -EOPNOTSUPP again.

>  int vioapic_init(struct domain *d)
>  {
>      if ( !has_vioapic(d) )
> +    {
> +        d->arch.hvm_domain.nr_vioapics = 0;

I don't think this is needed - if anything you may want to again
ASSERT() it being zero.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
>      if ( !has_vioapic(d) )
>          return 0;
>  
> -    redir0 = domain_vioapic(d)->redirtbl[0];
> +    redir0 = domain_vioapic(d, 0)->redirtbl[0];

What if the first IO-APIC has less than 16 pins?

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
>  static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>  {
>      struct vcpu *v = pt->vcpu;
> -    unsigned int gsi, isa_irq;
> +    struct hvm_hw_vioapic *vioapic;
> +    unsigned int gsi, isa_irq, pin;
>  
>      if ( pt->source == PTSRC_lapic )
>          return pt->irq;
> @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>                  + (isa_irq & 7));
>  
>      ASSERT(src == hvm_intsrc_lapic);
> -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
> +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> +
> +    return vioapic->redirtbl[pin].fields.vector;

Please don't chance de-referencing NULL here and below.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Roger Pau Monne]

On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> > +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi,
> 
> const struct domain *d ?
> 
> > +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic,
> 
> const for both?
>
> > +                            unsigned int pin)
> > +{
> > +    struct hvm_hw_vioapic *tmp;
> 
> And here.

Done for all the above.

> Also wouldn't this function more naturally (i.e. more generally useful)
> simply return the base GSI, leaving it to the caller to add the pin
> number?

I don't have a strong opinion. Most (if not all callers) seem to want the pin,
but you can obtain that by just doing gsi - base_gsi, so I changed it to return
base_gsi (which also simplifies the code).

> > @@ -157,21 +210,22 @@ static void vioapic_write_redirent(
> >          pent->fields.remote_irr = 0;
> >      else if ( !ent.fields.mask &&
> >                !ent.fields.remote_irr &&
> > -              hvm_irq->gsi_assert_count[idx] )
> > +              hvm_irq->gsi_assert_count[gsi] )
> 
> Neither this nor any earlier patch modified the size of this array, afaics.

Hm, right. This is still hardcoded to VIOAPIC_NUM_PINS. I should change it in
the earlier patch also.

> > -static void vioapic_write_indirect(struct domain *d, uint32_t val)
> > +static void vioapic_write_indirect(struct domain *d, unsigned long addr,
> > +                                   uint32_t val)
> >  {
> > -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> > +    struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr);
> 
> I think it would be better for the caller to pass the vioapic it
> already established (and ASSERT()ed).

Done. Now that hvm_vioapic has a reference to struct domain I no longer need to
pass the domain around.

> > @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> >  
> >  static int vioapic_save(struct domain *d, hvm_domain_context_t *h)
> >  {
> > -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> > +    struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0);
> >  
> >      if ( !has_vioapic(d) )
> >          return 0;
> >  
> > +    if ( d->arch.hvm_domain.nr_vioapics != 1 )
> > +        return -ENOSYS;
> > +
> >      if ( vioapic->nr_pins != VIOAPIC_NUM_PINS )
> >          return -ENOSYS;
> 
> Perhaps merge the two if()s? Otherwise -EOPNOTSUPP again.

OK.

> >  int vioapic_init(struct domain *d)
> >  {
> >      if ( !has_vioapic(d) )
> > +    {
> > +        d->arch.hvm_domain.nr_vioapics = 0;
> 
> I don't think this is needed - if anything you may want to again
> ASSERT() it being zero.

Done.

> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
> >      if ( !has_vioapic(d) )
> >          return 0;
> >  
> > -    redir0 = domain_vioapic(d)->redirtbl[0];
> > +    redir0 = domain_vioapic(d, 0)->redirtbl[0];
> 
> What if the first IO-APIC has less than 16 pins?

I'm not sure I understand what this piece of code is trying to do. Why is PIC
relying on the value of the first redirection entry of the IO APIC? Aren't the
PIC and the IO APIC modes mutually exclusive?

I would appreciate if you could provide some reference here.

> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
> >  static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> >  {
> >      struct vcpu *v = pt->vcpu;
> > -    unsigned int gsi, isa_irq;
> > +    struct hvm_hw_vioapic *vioapic;
> > +    unsigned int gsi, isa_irq, pin;
> >  
> >      if ( pt->source == PTSRC_lapic )
> >          return pt->irq;
> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> >                  + (isa_irq & 7));
> >  
> >      ASSERT(src == hvm_intsrc_lapic);
> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> > +
> > +    return vioapic->redirtbl[pin].fields.vector;
> 
> Please don't chance de-referencing NULL here and below.

Done, I've added an ASSERT.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Jan Beulich]

>>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote:
> On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
>> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/vlapic.c
>> > +++ b/xen/arch/x86/hvm/vlapic.c
>> > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
>> >      if ( !has_vioapic(d) )
>> >          return 0;
>> >  
>> > -    redir0 = domain_vioapic(d)->redirtbl[0];
>> > +    redir0 = domain_vioapic(d, 0)->redirtbl[0];
>> 
>> What if the first IO-APIC has less than 16 pins?
> 
> I'm not sure I understand what this piece of code is trying to do. Why is PIC
> relying on the value of the first redirection entry of the IO APIC? Aren't the
> PIC and the IO APIC modes mutually exclusive?
> 
> I would appreciate if you could provide some reference here.

Well, we're both in the same position here: All there is as explanation
is the code plus its history in git. Looking at the code I see that it
uses RTE 0 for ExtINT handling, which is reasonable. After all that's
the main connection between PIC and IO-APIC (so called Virtual Wire
Mode B iirc). See also our own code dealing with this (just look for
ExtINT in io_apic.c).

>> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>> >                  + (isa_irq & 7));
>> >  
>> >      ASSERT(src == hvm_intsrc_lapic);
>> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
>> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
>> > +
>> > +    return vioapic->redirtbl[pin].fields.vector;
>> 
>> Please don't chance de-referencing NULL here and below.
> 
> Done, I've added an ASSERT.

How about release builds then?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Roger Pau Monne]

On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote:
> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote:
> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> >> > --- a/xen/arch/x86/hvm/vlapic.c
> >> > +++ b/xen/arch/x86/hvm/vlapic.c
> >> > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
> >> >      if ( !has_vioapic(d) )
> >> >          return 0;
> >> >  
> >> > -    redir0 = domain_vioapic(d)->redirtbl[0];
> >> > +    redir0 = domain_vioapic(d, 0)->redirtbl[0];
> >> 
> >> What if the first IO-APIC has less than 16 pins?
> > 
> > I'm not sure I understand what this piece of code is trying to do. Why is PIC
> > relying on the value of the first redirection entry of the IO APIC? Aren't the
> > PIC and the IO APIC modes mutually exclusive?
> > 
> > I would appreciate if you could provide some reference here.
> 
> Well, we're both in the same position here: All there is as explanation
> is the code plus its history in git. Looking at the code I see that it
> uses RTE 0 for ExtINT handling, which is reasonable. After all that's
> the main connection between PIC and IO-APIC (so called Virtual Wire
> Mode B iirc). See also our own code dealing with this (just look for
> ExtINT in io_apic.c).

Oh, so that's hardcoded to pin 0 of the first IO APIC (that's where the
8259A-master output pin is hooked up AFAICT), in which case the code above is
right, and doesn't depend on whether IO APIC #0 has less than 16 pins.

> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> >> >                  + (isa_irq & 7));
> >> >  
> >> >      ASSERT(src == hvm_intsrc_lapic);
> >> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
> >> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> >> > +
> >> > +    return vioapic->redirtbl[pin].fields.vector;
> >> 
> >> Please don't chance de-referencing NULL here and below.
> > 
> > Done, I've added an ASSERT.
> 
> How about release builds then?

OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and
suitable printk in case this triggers. AFAICT if Xen happens to trigger this it
would mean that the gsi of the platform timer is higher than the maximum gsi,
which at the moment it's impossible.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Jan Beulich]

>>> On 21.03.17 at 11:52, <roger.pau@citrix.com> wrote:
> On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote:
>> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote:
>> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
>> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
>> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
>> >> >                  + (isa_irq & 7));
>> >> >  
>> >> >      ASSERT(src == hvm_intsrc_lapic);
>> >> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
>> >> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
>> >> > +
>> >> > +    return vioapic->redirtbl[pin].fields.vector;
>> >> 
>> >> Please don't chance de-referencing NULL here and below.
>> > 
>> > Done, I've added an ASSERT.
>> 
>> How about release builds then?
> 
> OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and
> suitable printk in case this triggers.

The latter, please, plus returning the spurious vector (as you still
need to return something).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Roger Pau Monne]

On Tue, Mar 21, 2017 at 07:45:13AM -0600, Jan Beulich wrote:
> >>> On 21.03.17 at 11:52, <roger.pau@citrix.com> wrote:
> > On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote:
> >> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote:
> >> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
> >> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> >> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> >> >> >                  + (isa_irq & 7));
> >> >> >  
> >> >> >      ASSERT(src == hvm_intsrc_lapic);
> >> >> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
> >> >> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> >> >> > +
> >> >> > +    return vioapic->redirtbl[pin].fields.vector;
> >> >> 
> >> >> Please don't chance de-referencing NULL here and below.
> >> > 
> >> > Done, I've added an ASSERT.
> >> 
> >> How about release builds then?
> > 
> > OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and
> > suitable printk in case this triggers.
> 
> The latter, please, plus returning the spurious vector (as you still
> need to return something).

Do we really need the domain_crash? I've added a gdprintk and returned -1.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Jan Beulich]

>>> On 21.03.17 at 14:59, <roger.pau@citrix.com> wrote:
> On Tue, Mar 21, 2017 at 07:45:13AM -0600, Jan Beulich wrote:
>> >>> On 21.03.17 at 11:52, <roger.pau@citrix.com> wrote:
>> > On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote:
>> >> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote:
>> >> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
>> >> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
>> >> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, 
> enum hvm_intsrc src)
>> >> >> >                  + (isa_irq & 7));
>> >> >> >  
>> >> >> >      ASSERT(src == hvm_intsrc_lapic);
>> >> >> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
>> >> >> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
>> >> >> > +
>> >> >> > +    return vioapic->redirtbl[pin].fields.vector;
>> >> >> 
>> >> >> Please don't chance de-referencing NULL here and below.
>> >> > 
>> >> > Done, I've added an ASSERT.
>> >> 
>> >> How about release builds then?
>> > 
>> > OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and
>> > suitable printk in case this triggers.
>> 
>> The latter, please, plus returning the spurious vector (as you still
>> need to return something).
> 
> Do we really need the domain_crash?

It's not strictly needed, but allowing the guest to continue to run
may make the (highly theoretical) issue harder to debug, the more
that ...

> I've added a gdprintk and returned -1.

... gdprintk() expands to nothing in release builds. Returning -1,
otoh, seems fine for all (two) present callers.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/5] x86/ioapic: add prototype for apic_gsi_base to io_apic.h

[Roger Pau Monne]

So that the function can be called from other files without adding prototypes
to each of them.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/io_apic.c        | 2 --
 xen/include/asm-x86/io_apic.h | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 24ee431..77572fc 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2274,8 +2274,6 @@ static int ioapic_physbase_to_id(unsigned long physbase)
     return -EINVAL;
 }
 
-unsigned apic_gsi_base(int apic);
-
 static int apic_pin_2_gsi_irq(int apic, int pin)
 {
     int idx;
diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index 225edd6..411fa3f 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -127,6 +127,9 @@ struct __packed IO_APIC_route_entry {
 /* I/O APIC entries */
 extern struct mpc_config_ioapic mp_ioapics[MAX_IO_APICS];
 
+/* Base GSI for this IO APIC */
+unsigned apic_gsi_base(int apic);
+
 /* Only need to remap ioapic RTE (reg: 10~3Fh) */
 #define ioapic_reg_remapped(reg) (iommu_intremap && ((reg) >= 0x10))
 
-- 
2.10.1 (Apple Git-78)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/5] x86/ioapic: add prototype for apic_gsi_base to io_apic.h

[Jan Beulich]

>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
> So that the function can be called from other files without adding prototypes
> to each of them.

If you plan to add more callers, this is the time to name the function
properly (an apic_ prefix rather suggests the LAPIC) and to convert
its parameter to unsigned int.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 5/5] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC

[Roger Pau Monne]

The base address, id and number of pins of the vIO APICs exposed to PVHv2 Dom0
is the same as the values found on bare metal.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain_build.c | 33 +++++++++++------------------
 xen/arch/x86/hvm/vioapic.c  | 51 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index dad3b4e..b9062ee 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -2271,12 +2271,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
     max_vcpus = dom0_max_vcpus();
     /* Calculate the size of the crafted MADT. */
     size = sizeof(*madt);
-    /*
-     * FIXME: the current vIO-APIC code just supports one IO-APIC instance
-     * per domain. This must be fixed in order to provide the same amount of
-     * IO APICs as available on bare metal.
-     */
-    size += sizeof(*io_apic);
+    size += sizeof(*io_apic) * nr_ioapics;
     size += sizeof(*intsrcovr) * acpi_intr_overrides;
     size += sizeof(*nmisrc) * acpi_nmi_sources;
     size += sizeof(*x2apic) * max_vcpus;
@@ -2304,23 +2299,19 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
      */
     madt->header.revision = min_t(unsigned char, table->revision, 4);
 
-    /*
-     * Setup the IO APIC entry.
-     * FIXME: the current vIO-APIC code just supports one IO-APIC instance
-     * per domain. This must be fixed in order to provide the same amount of
-     * IO APICs as available on bare metal, and with the same IDs as found in
-     * the native IO APIC MADT entries.
-     */
-    if ( nr_ioapics > 1 )
-        printk("WARNING: found %d IO APICs, Dom0 will only have access to 1 emulated IO APIC\n",
-               nr_ioapics);
+    /* Setup the IO APIC entries. */
     io_apic = (void *)(madt + 1);
-    io_apic->header.type = ACPI_MADT_TYPE_IO_APIC;
-    io_apic->header.length = sizeof(*io_apic);
-    io_apic->id = domain_vioapic(d, 0)->id;
-    io_apic->address = VIOAPIC_DEFAULT_BASE_ADDRESS;
+    for ( i = 0; i < nr_ioapics; i++ )
+    {
+        io_apic->header.type = ACPI_MADT_TYPE_IO_APIC;
+        io_apic->header.length = sizeof(*io_apic);
+        io_apic->id = domain_vioapic(d, i)->id;
+        io_apic->address = domain_vioapic(d, i)->base_address;
+        io_apic->global_irq_base = apic_gsi_base(i);
+        io_apic++;
+    }
 
-    x2apic = (void *)(io_apic + 1);
+    x2apic = (void *)io_apic;
     for ( i = 0; i < max_vcpus; i++ )
     {
         x2apic->header.type = ACPI_MADT_TYPE_LOCAL_X2APIC;
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 6421971..1467b25 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -629,15 +629,25 @@ void vioapic_reset(struct domain *d)
                sizeof(*vioapic->redirtbl) * vioapic->nr_pins);
         for ( j = 0; j < vioapic->nr_pins; j++ )
             vioapic->redirtbl[j].fields.mask = 1;
-        vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
-                                VIOAPIC_MEM_LENGTH * i;
-        vioapic->id = i;
+        if ( !is_hardware_domain(d) )
+        {
+            vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
+                                    VIOAPIC_MEM_LENGTH * i;
+            vioapic->id = i;
+        }
+        else
+        {
+            vioapic->base_address = mp_ioapics[i].mpc_apicaddr;
+            vioapic->id = mp_ioapics[i].mpc_apicid;
+        }
         vioapic->ioregsel = 0;
     }
 }
 
 int vioapic_init(struct domain *d)
 {
+    unsigned int i, nr_vioapics = is_hardware_domain(d) ? nr_ioapics : 1;
+
     if ( !has_vioapic(d) )
     {
         d->arch.hvm_domain.nr_vioapics = 0;
@@ -646,24 +656,41 @@ int vioapic_init(struct domain *d)
 
     if ( (d->arch.hvm_domain.vioapic == NULL) &&
          ((d->arch.hvm_domain.vioapic =
-           xmalloc(struct hvm_hw_vioapic)) == NULL) )
+           xzalloc_array(struct hvm_hw_vioapic, nr_vioapics)) == NULL) )
         return -ENOMEM;
 
-    domain_vioapic(d, 0)->redirtbl = xmalloc_array(union vioapic_redir_entry,
-                                                   VIOAPIC_NUM_PINS);
-    if ( !domain_vioapic(d, 0)->redirtbl )
+    if ( !is_hardware_domain(d) )
     {
-        xfree(d->arch.hvm_domain.vioapic);
-        return -ENOMEM;
+        ASSERT(nr_vioapics == 1);
+        domain_vioapic(d, 0)->redirtbl =
+            xmalloc_array(union vioapic_redir_entry, VIOAPIC_NUM_PINS);
+        if ( !domain_vioapic(d, 0)->redirtbl )
+            goto error;
+        domain_vioapic(d, 0)->nr_pins = VIOAPIC_NUM_PINS;
+    }
+    else
+    {
+        for ( i = 0; i < nr_vioapics; i++ )
+        {
+            domain_vioapic(d, i)->redirtbl =
+                xmalloc_array(union vioapic_redir_entry, nr_ioapic_entries[i]);
+            if ( !domain_vioapic(d, i)->redirtbl )
+                goto error;
+            domain_vioapic(d, i)->nr_pins = nr_ioapic_entries[i];
+        }
     }
 
-    domain_vioapic(d, 0)->nr_pins = VIOAPIC_NUM_PINS;
-    d->arch.hvm_domain.nr_vioapics = 1;
+    d->arch.hvm_domain.nr_vioapics = nr_vioapics;
     vioapic_reset(d);
-
     register_mmio_handler(d, &vioapic_mmio_ops);
 
     return 0;
+
+ error:
+    for ( i = 0; i < nr_vioapics; i++ )
+        xfree(domain_vioapic(d, i)->redirtbl);
+    xfree(d->arch.hvm_domain.vioapic);
+    return -ENOMEM;
 }
 
 void vioapic_deinit(struct domain *d)
-- 
2.10.1 (Apple Git-78)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 5/5] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC

[Jan Beulich]

>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote:
>  int vioapic_init(struct domain *d)
>  {
> +    unsigned int i, nr_vioapics = is_hardware_domain(d) ? nr_ioapics : 1;

Considering this ...

> @@ -646,24 +656,41 @@ int vioapic_init(struct domain *d)
>  
>      if ( (d->arch.hvm_domain.vioapic == NULL) &&
>           ((d->arch.hvm_domain.vioapic =
> -           xmalloc(struct hvm_hw_vioapic)) == NULL) )
> +           xzalloc_array(struct hvm_hw_vioapic, nr_vioapics)) == NULL) )
>          return -ENOMEM;
>  
> -    domain_vioapic(d, 0)->redirtbl = xmalloc_array(union vioapic_redir_entry,
> -                                                   VIOAPIC_NUM_PINS);
> -    if ( !domain_vioapic(d, 0)->redirtbl )
> +    if ( !is_hardware_domain(d) )
>      {
> -        xfree(d->arch.hvm_domain.vioapic);
> -        return -ENOMEM;
> +        ASSERT(nr_vioapics == 1);

... this is kind of strange a check.

Everything else looks fine, but this will see some changes due to
the intended data structure adjustments in an earlier patch.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel