* [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs
@ 2017-03-27 10:18 Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure Roger Pau Monne
` (6 more replies)
0 siblings, 7 replies; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel
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 48 entries.
The functionality introduced is only used by PVHv2 Dom0, in order to copy the
IO APIC topology found on bare metal.
I'm in the process of setting up an osstest run in order to test this series.
They can also be found in my personal git tree:
git://xenbits.xen.org/people/royger/xen.git vioapics_v2
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-27 15:38 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro Roger Pau Monne
` (5 subsequent siblings)
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne
This is required in order to have a variable number of vIO APIC pins, instead
of the current fixed value (48). Note that this patch only expands the fields
of the hvm_vioapic struct, without actually introducing any new fields or
functionality.
The reason to expand the hvm_vioapic structure instead of the hvm_hw_vioapic
one is that the variable number of pins functionality is only going to be used
by the hardware domain, so no modifications are needed to the save format.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
- New in this version.
---
xen/arch/x86/hvm/vioapic.c | 43 ++++++++++++++++++++++-----------------
xen/include/asm-x86/hvm/vioapic.h | 11 ++++++----
2 files changed, 31 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index fdbb21f097..84139a3b7d 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -42,9 +42,9 @@
/* 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 hvm_vioapic *vioapic, int irq);
-static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic)
+static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic)
{
uint32_t result = 0;
@@ -94,7 +94,7 @@ 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_vioapic *vioapic = domain_vioapic(v->domain);
uint32_t result;
HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "addr %lx", addr);
@@ -119,7 +119,7 @@ static int vioapic_read(
}
static void vioapic_write_redirent(
- struct hvm_hw_vioapic *vioapic, unsigned int idx,
+ struct hvm_vioapic *vioapic, unsigned int idx,
int top_word, uint32_t val)
{
struct domain *d = vioapic_domain(vioapic);
@@ -170,7 +170,7 @@ static void vioapic_write_redirent(
}
static void vioapic_write_indirect(
- struct hvm_hw_vioapic *vioapic, uint32_t val)
+ struct hvm_vioapic *vioapic, uint32_t val)
{
switch ( vioapic->ioregsel )
{
@@ -215,7 +215,7 @@ 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_vioapic *vioapic = domain_vioapic(v->domain);
switch ( addr & 0xff )
{
@@ -242,7 +242,7 @@ static int vioapic_write(
static int vioapic_range(struct vcpu *v, unsigned long addr)
{
- struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
+ struct hvm_vioapic *vioapic = domain_vioapic(v->domain);
return ((addr >= vioapic->base_address &&
(addr < vioapic->base_address + VIOAPIC_MEM_LENGTH)));
@@ -255,7 +255,7 @@ static const struct hvm_mmio_ops vioapic_mmio_ops = {
};
static void ioapic_inj_irq(
- struct hvm_hw_vioapic *vioapic,
+ struct hvm_vioapic *vioapic,
struct vlapic *target,
uint8_t vector,
uint8_t trig_mode,
@@ -275,7 +275,7 @@ static inline int pit_channel0_enabled(void)
return pt_active(¤t->domain->arch.vpit.pt0);
}
-static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq)
+static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
{
uint16_t dest = vioapic->redirtbl[irq].fields.dest_id;
uint8_t dest_mode = vioapic->redirtbl[irq].fields.dest_mode;
@@ -361,7 +361,7 @@ static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq)
void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
{
- struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
+ struct hvm_vioapic *vioapic = domain_vioapic(d);
union vioapic_redir_entry *ent;
ASSERT(has_vioapic(d));
@@ -388,7 +388,7 @@ 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_vioapic *vioapic = domain_vioapic(d);
struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
union vioapic_redir_entry *ent;
int gsi;
@@ -426,38 +426,43 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_hw_vioapic *s = domain_vioapic(d);
+ struct hvm_vioapic *s = domain_vioapic(d);
if ( !has_vioapic(d) )
return 0;
- return hvm_save_entry(IOAPIC, 0, h, s);
+ BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) !=
+ sizeof(struct hvm_vioapic) -
+ offsetof(struct hvm_vioapic, base_address));
+
+ return hvm_save_entry(IOAPIC, 0, h, &s->base_address);
}
static int ioapic_load(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_hw_vioapic *s = domain_vioapic(d);
+ struct hvm_vioapic *s = domain_vioapic(d);
if ( !has_vioapic(d) )
return -ENODEV;
- return hvm_load_entry(IOAPIC, h, s);
+ return hvm_load_entry(IOAPIC, h, &s->base_address);
}
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_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));
+ vioapic->domain = d;
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)
diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h
index 745c09ab5c..e8ec0be7b6 100644
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -48,13 +48,16 @@
#define VIOAPIC_REG_RTE0 0x10
struct hvm_vioapic {
- struct hvm_hw_vioapic hvm_hw_vioapic;
struct domain *domain;
+ /* Layout below must match hvm_hw_vioapic. */
+ uint64_t base_address;
+ uint32_t ioregsel;
+ uint32_t id;
+ union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
};
-#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)
+#define vioapic_domain(v) ((v)->domain)
int vioapic_init(struct domain *d);
void vioapic_deinit(struct domain *d);
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-27 15:41 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array Roger Pau Monne
` (4 subsequent siblings)
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Roger Pau Monne
Introduce a macro to get a pointer to the hvm_irq for a HVM domain. No
functional change.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
xen/arch/x86/hvm/irq.c | 30 +++++++++++++++---------------
xen/arch/x86/hvm/vioapic.c | 4 ++--
xen/arch/x86/hvm/vlapic.c | 6 +++---
xen/arch/x86/physdev.c | 2 +-
xen/drivers/passthrough/vtd/x86/vtd.c | 2 +-
xen/include/asm-x86/hvm/irq.h | 1 +
6 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index a774ed7450..c2951ccf8a 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -60,7 +60,7 @@ static void deassert_irq(struct domain *d, unsigned isa_irq)
static void __hvm_pci_intx_assert(
struct domain *d, unsigned int device, unsigned int intx)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi, link, isa_irq;
ASSERT((device <= 31) && (intx <= 3));
@@ -90,7 +90,7 @@ void hvm_pci_intx_assert(
static void __hvm_pci_intx_deassert(
struct domain *d, unsigned int device, unsigned int intx)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi, link, isa_irq;
ASSERT((device <= 31) && (intx <= 3));
@@ -119,7 +119,7 @@ void hvm_pci_intx_deassert(
void hvm_isa_irq_assert(
struct domain *d, unsigned int isa_irq)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
ASSERT(isa_irq <= 15);
@@ -136,7 +136,7 @@ void hvm_isa_irq_assert(
void hvm_isa_irq_deassert(
struct domain *d, unsigned int isa_irq)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
ASSERT(isa_irq <= 15);
@@ -153,7 +153,7 @@ void hvm_isa_irq_deassert(
static void hvm_set_callback_irq_level(struct vcpu *v)
{
struct domain *d = v->domain;
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi, pdev, pintx, asserted;
ASSERT(v->vcpu_id == 0);
@@ -201,7 +201,7 @@ static void hvm_set_callback_irq_level(struct vcpu *v)
void hvm_maybe_deassert_evtchn_irq(void)
{
struct domain *d = current->domain;
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
if ( hvm_irq->callback_via_asserted &&
!vcpu_info(d->vcpu[0], evtchn_upcall_pending) )
@@ -230,7 +230,7 @@ void hvm_assert_evtchn_irq(struct vcpu *v)
int hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
u8 old_isa_irq;
int i;
@@ -323,7 +323,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
void hvm_set_callback_via(struct domain *d, uint64_t via)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi=0, pdev=0, pintx=0;
uint8_t via_type;
@@ -486,7 +486,7 @@ void arch_evtchn_inject(struct vcpu *v)
static void irq_dump(struct domain *d)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
int i;
printk("Domain %d:\n", d->domain_id);
printk("PCI 0x%16.16"PRIx64"%16.16"PRIx64
@@ -541,7 +541,7 @@ __initcall(dump_irq_info_key_init);
static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int asserted, pdev, pintx;
int rc;
@@ -573,7 +573,7 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
static int irq_save_isa(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
/* Save ISA IRQ lines */
return ( hvm_save_entry(ISA_IRQ, 0, h, &hvm_irq->isa_irq) );
@@ -581,7 +581,7 @@ static int irq_save_isa(struct domain *d, hvm_domain_context_t *h)
static int irq_save_link(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
/* Save PCI-ISA link state */
return ( hvm_save_entry(PCI_LINK, 0, h, &hvm_irq->pci_link) );
@@ -589,7 +589,7 @@ static int irq_save_link(struct domain *d, hvm_domain_context_t *h)
static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
int link, dev, intx, gsi;
/* Load the PCI IRQ lines */
@@ -622,7 +622,7 @@ static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
static int irq_load_isa(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
int irq;
/* Load the ISA IRQ lines */
@@ -641,7 +641,7 @@ static int irq_load_isa(struct domain *d, hvm_domain_context_t *h)
static int irq_load_link(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
int link, gsi;
/* Load the PCI-ISA IRQ link routing table */
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 84139a3b7d..39dbf832b3 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -123,7 +123,7 @@ static void vioapic_write_redirent(
int top_word, uint32_t val)
{
struct domain *d = vioapic_domain(vioapic);
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
union vioapic_redir_entry *pent, ent;
int unmasked = 0;
@@ -389,7 +389,7 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
void vioapic_update_EOI(struct domain *d, u8 vector)
{
struct hvm_vioapic *vioapic = domain_vioapic(d);
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
union vioapic_redir_entry *ent;
int gsi;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 14356a78fe..0590d6c69d 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -373,7 +373,7 @@ struct vlapic *vlapic_lowest_prio(
struct domain *d, const struct vlapic *source,
int short_hand, uint32_t dest, bool_t dest_mode)
{
- int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
+ int old = hvm_domain_irq(d)->round_robin_prev_vcpu;
uint32_t ppr, target_ppr = UINT_MAX;
struct vlapic *vlapic, *target = NULL;
struct vcpu *v;
@@ -394,8 +394,8 @@ struct vlapic *vlapic_lowest_prio(
} while ( v->vcpu_id != old );
if ( target != NULL )
- d->arch.hvm_domain.irq.round_robin_prev_vcpu =
- vlapic_vcpu(target)->vcpu_id;
+ hvm_domain_irq(d)->round_robin_prev_vcpu =
+ vlapic_vcpu(target)->vcpu_id;
return target;
}
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 81cd6c94e7..6c15f9bf49 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -317,7 +317,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( is_hvm_domain(currd) &&
domain_pirq_to_emuirq(currd, eoi.irq) > 0 )
{
- struct hvm_irq *hvm_irq = &currd->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(currd);
int gsi = domain_pirq_to_emuirq(currd, eoi.irq);
/* if this is a level irq and count > 0, send another
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index 8a89f3471f..88a60b3307 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -66,7 +66,7 @@ void flush_all_cache()
static int _hvm_dpci_isairq_eoi(struct domain *d,
struct hvm_pirq_dpci *pirq_dpci, void *arg)
{
- struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
+ struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int isairq = (long)arg;
const struct dev_intx_gsi_link *digl;
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 73b8fb0457..17a957d4b5 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -95,6 +95,7 @@ struct hvm_irq {
(((((dev)<<2) + ((dev)>>3) + (intx)) & 31) + 16)
#define hvm_pci_intx_link(dev, intx) \
(((dev) + (intx)) & 3)
+#define hvm_domain_irq(d) (&(d)->arch.hvm_domain.irq)
#define hvm_isa_irq_to_gsi(isa_irq) ((isa_irq) ? : 2)
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-27 15:59 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 4/7] x86/vioapic: allow the vIO APIC to have a variable number of pins Roger Pau Monne
` (3 subsequent siblings)
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne
Rearrange the fields of hvm_irq so that gsi_assert_count can be converted into
a variable size array and add a new field to account the number of GSIs.
Due to this changes the irq member in the hvm_domain struct also needs to
become a pointer set at runtime.
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/hvm.c | 13 +++++++++++--
xen/arch/x86/hvm/irq.c | 27 +++++++++++++++++++++------
xen/drivers/passthrough/io.c | 8 ++++----
xen/drivers/passthrough/pci.c | 2 +-
xen/include/asm-x86/domain.h | 2 +-
xen/include/asm-x86/hvm/domain.h | 2 +-
xen/include/asm-x86/hvm/irq.h | 28 +++++++++++++++-------------
7 files changed, 54 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0282986738..9a6cd9c9bf 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -457,7 +457,7 @@ void hvm_migrate_pirqs(struct vcpu *v)
{
struct domain *d = v->domain;
- if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
+ if ( !iommu_enabled || !d->arch.hvm_domain.irq->dpci )
return;
spin_lock(&d->event_lock);
@@ -619,11 +619,16 @@ int hvm_domain_initialise(struct domain *d)
d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
d->arch.hvm_domain.io_handler = xzalloc_array(struct hvm_io_handler,
NR_IO_HANDLERS);
+ d->arch.hvm_domain.irq = xzalloc_bytes(hvm_irq_size(VIOAPIC_NUM_PINS));
+
rc = -ENOMEM;
- if ( !d->arch.hvm_domain.pl_time ||
+ if ( !d->arch.hvm_domain.pl_time || !d->arch.hvm_domain.irq ||
!d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
goto fail1;
+ /* Set the default number of GSIs */
+ hvm_domain_irq(d)->nr_gsis = VIOAPIC_NUM_PINS;
+
/* need link to containing domain */
d->arch.hvm_domain.pl_time->domain = d;
@@ -680,6 +685,7 @@ int hvm_domain_initialise(struct domain *d)
xfree(d->arch.hvm_domain.io_handler);
xfree(d->arch.hvm_domain.params);
xfree(d->arch.hvm_domain.pl_time);
+ xfree(d->arch.hvm_domain.irq);
fail0:
hvm_destroy_cacheattr_region_list(d);
return rc;
@@ -722,6 +728,9 @@ void hvm_domain_destroy(struct domain *d)
xfree(d->arch.hvm_domain.pl_time);
d->arch.hvm_domain.pl_time = NULL;
+
+ xfree(d->arch.hvm_domain.irq);
+ d->arch.hvm_domain.irq = NULL;
}
static int hvm_save_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index c2951ccf8a..6e67cae9bd 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -69,6 +69,7 @@ static void __hvm_pci_intx_assert(
return;
gsi = hvm_pci_intx_gsi(device, intx);
+ ASSERT(gsi < hvm_irq->nr_gsis);
if ( hvm_irq->gsi_assert_count[gsi]++ == 0 )
assert_gsi(d, gsi);
@@ -99,6 +100,7 @@ static void __hvm_pci_intx_deassert(
return;
gsi = hvm_pci_intx_gsi(device, intx);
+ ASSERT(gsi < hvm_irq->nr_gsis);
--hvm_irq->gsi_assert_count[gsi];
link = hvm_pci_intx_link(device, intx);
@@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
- ASSERT(isa_irq <= 15);
+ ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
spin_lock(&d->arch.hvm_domain.irq_lock);
@@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
- ASSERT(isa_irq <= 15);
+ ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
spin_lock(&d->arch.hvm_domain.irq_lock);
@@ -363,7 +365,7 @@ void hvm_set_callback_via(struct domain *d, uint64_t via)
{
case HVMIRQ_callback_gsi:
gsi = hvm_irq->callback_via.gsi = (uint8_t)via;
- if ( (gsi == 0) || (gsi >= ARRAY_SIZE(hvm_irq->gsi_assert_count)) )
+ if ( (gsi == 0) || (gsi >= hvm_irq->nr_gsis) )
hvm_irq->callback_via_type = HVMIRQ_callback_none;
else if ( hvm_irq->callback_via_asserted &&
(hvm_irq->gsi_assert_count[gsi]++ == 0) )
@@ -419,9 +421,9 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
if ( unlikely(v->mce_pending) )
return hvm_intack_mce;
- if ( (plat->irq.callback_via_type == HVMIRQ_callback_vector)
+ if ( (plat->irq->callback_via_type == HVMIRQ_callback_vector)
&& vcpu_info(v, evtchn_upcall_pending) )
- return hvm_intack_vector(plat->irq.callback_via.vector);
+ return hvm_intack_vector(plat->irq->callback_via.vector);
if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
return hvm_intack_pic(0);
@@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
(uint32_t) hvm_irq->isa_irq.pad[0],
hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
- for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
+ for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
i, i+7,
@@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
hvm_irq->gsi_assert_count[i+5],
hvm_irq->gsi_assert_count[i+6],
hvm_irq->gsi_assert_count[i+7]);
+ if ( i != hvm_irq->nr_gsis )
+ {
+ printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
+ for ( ; i < hvm_irq->nr_gsis; i++)
+ printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
+ printk("\n");
+ }
printk("Link %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
hvm_irq->pci_link_assert_count[0],
hvm_irq->pci_link_assert_count[1],
@@ -545,6 +554,9 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
unsigned int asserted, pdev, pintx;
int rc;
+ if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
+ return -EOPNOTSUPP;
+
spin_lock(&d->arch.hvm_domain.irq_lock);
pdev = hvm_irq->callback_via.pci.dev;
@@ -592,6 +604,9 @@ static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
int link, dev, intx, gsi;
+ if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
+ return -EOPNOTSUPP;
+
/* Load the PCI IRQ lines */
if ( hvm_load_entry(PCI_IRQ, h, &hvm_irq->pci_intx) != 0 )
return -EINVAL;
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 080183ea31..50e2f00214 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -195,7 +195,7 @@ struct hvm_irq_dpci *domain_get_irq_dpci(const struct domain *d)
if ( !d || !is_hvm_domain(d) )
return NULL;
- return d->arch.hvm_domain.irq.dpci;
+ return d->arch.hvm_domain.irq->dpci;
}
void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci)
@@ -333,7 +333,7 @@ int pt_irq_create_bind(
for ( i = 0; i < NR_HVM_IRQS; i++ )
INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]);
- d->arch.hvm_domain.irq.dpci = hvm_irq_dpci;
+ d->arch.hvm_domain.irq->dpci = hvm_irq_dpci;
}
info = pirq_get_info(d, pirq);
@@ -788,7 +788,7 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
void hvm_dpci_msi_eoi(struct domain *d, int vector)
{
- if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
+ if ( !iommu_enabled || !d->arch.hvm_domain.irq->dpci )
return;
spin_lock(&d->event_lock);
@@ -798,7 +798,7 @@ void hvm_dpci_msi_eoi(struct domain *d, int vector)
static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
- if ( unlikely(!d->arch.hvm_domain.irq.dpci) )
+ if ( unlikely(!d->arch.hvm_domain.irq->dpci) )
{
ASSERT_UNREACHABLE();
return;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index beddd42701..b5b865a2d4 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -815,7 +815,7 @@ static int pci_clean_dpci_irqs(struct domain *d)
return ret;
}
- d->arch.hvm_domain.irq.dpci = NULL;
+ d->arch.hvm_domain.irq->dpci = NULL;
free_hvm_irq_dpci(hvm_irq_dpci);
}
spin_unlock(&d->event_lock);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index ec14cce81f..0b7e43fa16 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -17,7 +17,7 @@
#define is_pv_32bit_vcpu(v) (is_pv_32bit_domain((v)->domain))
#define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \
- d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
+ d->arch.hvm_domain.irq->callback_via_type == HVMIRQ_callback_vector)
#define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain))
#define is_domain_direct_mapped(d) ((void)(d), 0)
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 420cbdc609..c3cca94a97 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -125,7 +125,7 @@ struct hvm_domain {
/* Lock protects access to irq, vpic and vioapic. */
spinlock_t irq_lock;
- struct hvm_irq irq;
+ struct hvm_irq *irq;
struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */
struct hvm_vioapic *vioapic;
struct hvm_hw_stdvga stdvga;
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 17a957d4b5..7d45293aed 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -67,18 +67,6 @@ struct hvm_irq {
u8 pci_link_assert_count[4];
/*
- * Number of wires asserting each GSI.
- *
- * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
- * except ISA IRQ 0, which is connected to GSI 2.
- * PCI links map into this space via the PCI-ISA bridge.
- *
- * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
- * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
- */
- u8 gsi_assert_count[VIOAPIC_NUM_PINS];
-
- /*
* GSIs map onto PIC/IO-APIC in the usual way:
* 0-7: Master 8259 PIC, IO-APIC pins 0-7
* 8-15: Slave 8259 PIC, IO-APIC pins 8-15
@@ -89,13 +77,27 @@ struct hvm_irq {
u8 round_robin_prev_vcpu;
struct hvm_irq_dpci *dpci;
+
+ /*
+ * Number of wires asserting each GSI.
+ *
+ * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
+ * except ISA IRQ 0, which is connected to GSI 2.
+ * PCI links map into this space via the PCI-ISA bridge.
+ *
+ * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
+ * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
+ */
+ unsigned int nr_gsis;
+ u8 gsi_assert_count[];
};
#define hvm_pci_intx_gsi(dev, intx) \
(((((dev)<<2) + ((dev)>>3) + (intx)) & 31) + 16)
#define hvm_pci_intx_link(dev, intx) \
(((dev) + (intx)) & 3)
-#define hvm_domain_irq(d) (&(d)->arch.hvm_domain.irq)
+#define hvm_domain_irq(d) ((d)->arch.hvm_domain.irq)
+#define hvm_irq_size(cnt) offsetof(struct hvm_irq, gsi_assert_count[cnt])
#define hvm_isa_irq_to_gsi(isa_irq) ((isa_irq) ? : 2)
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 4/7] x86/vioapic: allow the vIO APIC to have a variable number of pins
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
` (2 preceding siblings ...)
2017-03-27 10:18 ` [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-27 16:10 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS Roger Pau Monne
` (2 subsequent siblings)
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monne
Although it's still always set to VIOAPIC_NUM_PINS (48).
Add a new field to the hvm_ioapic struct to contain the number of pins (number
of IO redirection table entries) and turn the redirection table into a variable
sized array.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
- Almost completely reworked due to previous changes.
---
xen/arch/x86/hvm/vioapic.c | 28 +++++++++++++++++-----------
xen/include/asm-x86/hvm/vioapic.h | 4 +++-
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 39dbf832b3..00048ad65d 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_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;
@@ -73,7 +73,7 @@ static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic)
uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
uint64_t redir_content;
- if ( redir_index >= VIOAPIC_NUM_PINS )
+ if ( redir_index >= vioapic->nr_pins )
{
gdprintk(XENLOG_WARNING, "apic_mem_readl:undefined ioregsel %x\n",
vioapic->ioregsel);
@@ -197,7 +197,7 @@ static void vioapic_write_indirect(
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 )
@@ -431,9 +431,8 @@ static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
if ( !has_vioapic(d) )
return 0;
- BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) !=
- sizeof(struct hvm_vioapic) -
- offsetof(struct hvm_vioapic, base_address));
+ if ( s->nr_pins != VIOAPIC_NUM_PINS )
+ return -EOPNOTSUPP;
return hvm_save_entry(IOAPIC, 0, h, &s->base_address);
}
@@ -445,6 +444,9 @@ static int ioapic_load(struct domain *d, hvm_domain_context_t *h)
if ( !has_vioapic(d) )
return -ENODEV;
+ if ( s->nr_pins != VIOAPIC_NUM_PINS )
+ return -EOPNOTSUPP;
+
return hvm_load_entry(IOAPIC, h, &s->base_address);
}
@@ -453,14 +455,16 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
void vioapic_reset(struct domain *d)
{
struct hvm_vioapic *vioapic = domain_vioapic(d);
+ uint32_t nr_pins = vioapic->nr_pins;
int i;
if ( !has_vioapic(d) )
return;
- memset(vioapic, 0, sizeof(*vioapic));
+ memset(vioapic, 0, hvm_vioapic_size(nr_pins));
vioapic->domain = d;
- for ( i = 0; i < VIOAPIC_NUM_PINS; i++ )
+ vioapic->nr_pins = nr_pins;
+ for ( i = 0; i < nr_pins; i++ )
vioapic->redirtbl[i].fields.mask = 1;
vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
}
@@ -471,10 +475,12 @@ 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_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
return -ENOMEM;
d->arch.hvm_domain.vioapic->domain = d;
+ d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
vioapic_reset(d);
register_mmio_handler(d, &vioapic_mmio_ops);
diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h
index e8ec0be7b6..df8154390f 100644
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -49,13 +49,15 @@
struct hvm_vioapic {
struct domain *domain;
+ uint32_t nr_pins;
/* Layout below must match hvm_hw_vioapic. */
uint64_t base_address;
uint32_t ioregsel;
uint32_t id;
- union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
+ union vioapic_redir_entry redirtbl[];
};
+#define hvm_vioapic_size(cnt) offsetof(struct hvm_vioapic, redirtbl[cnt])
#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic)
#define vioapic_domain(v) ((v)->domain)
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
` (3 preceding siblings ...)
2017-03-27 10:18 ` [PATCH v2 4/7] x86/vioapic: allow the vIO APIC to have a variable number of pins Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-28 10:32 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 6/7] x86/ioapic: add prototype for apic_gsi_base to io_apic.h Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 7/7] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC Roger Pau Monne
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, 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>
---
Changes since v1:
- Constify some parameters.
- Make gsi_vioapic return the base GSI of the IO APIC.
- Add checks to pt_irq_vector in order to prevent dereferencing NULL.
---
xen/arch/x86/hvm/dom0_build.c | 2 +-
xen/arch/x86/hvm/vioapic.c | 216 ++++++++++++++++++++++++++++----------
xen/arch/x86/hvm/vlapic.c | 2 +-
xen/arch/x86/hvm/vpt.c | 19 +++-
xen/include/asm-x86/hvm/domain.h | 3 +-
xen/include/asm-x86/hvm/vioapic.h | 4 +-
6 files changed, 181 insertions(+), 65 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 5576db4ee8..daa791d3f4 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -729,7 +729,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 00048ad65d..327a9758e0 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -44,6 +44,54 @@
static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq);
+static struct hvm_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_vioapic *vioapic = domain_vioapic(d, i);
+
+ if ( addr >= vioapic->base_address &&
+ addr < vioapic->base_address + VIOAPIC_MEM_LENGTH )
+ return vioapic;
+ }
+
+ return NULL;
+}
+
+struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
+ unsigned int *base_gsi)
+{
+ unsigned int i;
+ *base_gsi = 0;
+
+ for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
+ {
+ struct hvm_vioapic *vioapic = domain_vioapic(d, i);
+
+ if ( gsi >= *base_gsi && gsi < *base_gsi + vioapic->nr_pins )
+ return vioapic;
+
+ *base_gsi += vioapic->nr_pins;
+ }
+
+ return NULL;
+}
+
+static unsigned int pin_gsi(const struct domain *d,
+ const struct hvm_vioapic *vioapic,
+ unsigned int pin)
+{
+ const struct hvm_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_vioapic *vioapic)
{
uint32_t result = 0;
@@ -94,11 +142,14 @@ static int vioapic_read(
struct vcpu *v, unsigned long addr,
unsigned int length, unsigned long *pval)
{
- const struct hvm_vioapic *vioapic = domain_vioapic(v->domain);
+ const struct hvm_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:
@@ -126,6 +177,7 @@ static void vioapic_write_redirent(
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
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 +201,7 @@ static void vioapic_write_redirent(
*pent = ent;
- if ( idx == 0 )
+ if ( gsi == 0 )
{
vlapic_adjust_i8259_target(d);
}
@@ -165,7 +217,7 @@ static void vioapic_write_redirent(
spin_unlock(&d->arch.hvm_domain.irq_lock);
- if ( idx == 0 || unmasked )
+ if ( gsi == 0 || unmasked )
pt_may_unmask_irq(d, NULL);
}
@@ -215,7 +267,10 @@ static int vioapic_write(
struct vcpu *v, unsigned long addr,
unsigned int length, unsigned long val)
{
- struct hvm_vioapic *vioapic = domain_vioapic(v->domain);
+ struct hvm_vioapic *vioapic;
+
+ vioapic = addr_vioapic(v->domain, addr);
+ ASSERT(vioapic);
switch ( addr & 0xff )
{
@@ -242,10 +297,7 @@ static int vioapic_write(
static int vioapic_range(struct vcpu *v, unsigned long addr)
{
- struct hvm_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 = {
@@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void)
return pt_active(¤t->domain->arch.vpit.pt0);
}
-static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
+static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin)
{
- 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;
+ 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 domain *d = vioapic_domain(vioapic);
struct vlapic *target;
struct vcpu *v;
+ unsigned int irq = pin_gsi(d, vioapic, pin);
ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
@@ -361,64 +414,72 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
{
- struct hvm_vioapic *vioapic = domain_vioapic(d);
+ unsigned int base_gsi;
+ struct hvm_vioapic *vioapic = gsi_vioapic(d, irq, &base_gsi);
+ unsigned int pin = irq - base_gsi;
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;
if ( ent->fields.trig_mode == VIOAPIC_EDGE_TRIG )
{
- vioapic_deliver(vioapic, irq);
+ vioapic_deliver(vioapic, pin);
}
else if ( !ent->fields.remote_irr )
{
ent->fields.remote_irr = 1;
- vioapic_deliver(vioapic, irq);
+ vioapic_deliver(vioapic, pin);
}
}
void vioapic_update_EOI(struct domain *d, u8 vector)
{
- struct hvm_vioapic *vioapic = domain_vioapic(d);
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
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_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(vioapic, 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(vioapic, j);
+ }
}
+ base_gsi += vioapic->nr_pins;
}
spin_unlock(&d->arch.hvm_domain.irq_lock);
@@ -426,12 +487,13 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_vioapic *s = domain_vioapic(d);
+ struct hvm_vioapic *s = domain_vioapic(d, 0);
if ( !has_vioapic(d) )
return 0;
- if ( s->nr_pins != VIOAPIC_NUM_PINS )
+ if ( s->nr_pins != VIOAPIC_NUM_PINS ||
+ d->arch.hvm_domain.nr_vioapics != 1 )
return -EOPNOTSUPP;
return hvm_save_entry(IOAPIC, 0, h, &s->base_address);
@@ -439,12 +501,13 @@ static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
static int ioapic_load(struct domain *d, hvm_domain_context_t *h)
{
- struct hvm_vioapic *s = domain_vioapic(d);
+ struct hvm_vioapic *s = domain_vioapic(d, 0);
if ( !has_vioapic(d) )
return -ENODEV;
- if ( s->nr_pins != VIOAPIC_NUM_PINS )
+ if ( s->nr_pins != VIOAPIC_NUM_PINS ||
+ d->arch.hvm_domain.nr_vioapics != 1 )
return -EOPNOTSUPP;
return hvm_load_entry(IOAPIC, h, &s->base_address);
@@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
void vioapic_reset(struct domain *d)
{
- struct hvm_vioapic *vioapic = domain_vioapic(d);
- uint32_t nr_pins = vioapic->nr_pins;
- int i;
+ unsigned int i;
if ( !has_vioapic(d) )
+ {
+ ASSERT(!d->arch.hvm_domain.nr_vioapics);
return;
+ }
- memset(vioapic, 0, hvm_vioapic_size(nr_pins));
- vioapic->domain = d;
- vioapic->nr_pins = nr_pins;
- for ( i = 0; i < nr_pins; i++ )
- vioapic->redirtbl[i].fields.mask = 1;
- vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
+ for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
+ {
+ struct hvm_vioapic *vioapic = domain_vioapic(d, i);
+ unsigned int j, nr_pins = vioapic->nr_pins;
+
+ memset(vioapic, 0, hvm_vioapic_size(nr_pins));
+ for ( j = 0; j < nr_pins; j++ )
+ vioapic->redirtbl[j].fields.mask = 1;
+ vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
+ VIOAPIC_MEM_LENGTH * i;
+ vioapic->id = i;
+ vioapic->nr_pins = nr_pins;
+ vioapic->domain = d;
+ }
+}
+
+static void vioapic_free(const struct domain *d, unsigned int nr_vioapics)
+{
+ unsigned int i;
+
+ for ( i = 0; i < nr_vioapics; i++)
+ {
+ if ( domain_vioapic(d, i) == NULL )
+ break;
+ xfree(domain_vioapic(d, i));
+ }
+ xfree(d->arch.hvm_domain.vioapic);
}
int vioapic_init(struct domain *d)
{
+ unsigned int i, nr_vioapics = 1;
+
if ( !has_vioapic(d) )
+ {
+ ASSERT(!d->arch.hvm_domain.nr_vioapics);
return 0;
+ }
if ( (d->arch.hvm_domain.vioapic == NULL) &&
((d->arch.hvm_domain.vioapic =
- xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
+ xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
return -ENOMEM;
- d->arch.hvm_domain.vioapic->domain = d;
- d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
+ for ( i = 0; i < nr_vioapics; i++ )
+ {
+ if ( (domain_vioapic(d, i) =
+ xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
+ {
+ vioapic_free(d, nr_vioapics);
+ return -ENOMEM;
+ }
+ domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
+ }
+
+ d->arch.hvm_domain.nr_vioapics = nr_vioapics;
vioapic_reset(d);
register_mmio_handler(d, &vioapic_mmio_ops);
@@ -491,8 +591,10 @@ int vioapic_init(struct domain *d)
void vioapic_deinit(struct domain *d)
{
if ( !has_vioapic(d) )
+ {
+ ASSERT(!d->arch.hvm_domain.nr_vioapics);
return;
+ }
- xfree(d->arch.hvm_domain.vioapic);
- d->arch.hvm_domain.vioapic = NULL;
+ vioapic_free(d, d->arch.hvm_domain.nr_vioapics);
}
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 0590d6c69d..2653ba80fd 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1107,7 +1107,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 5c48fdb4b5..97c5a13748 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_vioapic *vioapic;
+ unsigned int gsi, isa_irq, base_gsi;
if ( pt->source == PTSRC_lapic )
return pt->irq;
@@ -91,13 +92,22 @@ 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, &base_gsi);
+ if ( !vioapic )
+ {
+ gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", gsi);
+ domain_crash(v->domain);
+ return -1;
+ }
+
+ return vioapic->redirtbl[gsi - base_gsi].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, base_gsi;
+ struct hvm_vioapic *vioapic;
uint8_t pic_imr;
if ( pt->source == PTSRC_lapic )
@@ -110,9 +120,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, &base_gsi);
return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) &&
- domain_vioapic(v->domain)->redirtbl[gsi].fields.mask);
+ vioapic->redirtbl[gsi - base_gsi].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 c3cca94a97..63b0d927ea 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -127,7 +127,8 @@ 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_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 df8154390f..07455e2b61 100644
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -58,8 +58,10 @@ struct hvm_vioapic {
};
#define hvm_vioapic_size(cnt) offsetof(struct hvm_vioapic, redirtbl[cnt])
-#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic)
+#define domain_vioapic(d, i) ((d)->arch.hvm_domain.vioapic[i])
#define vioapic_domain(v) ((v)->domain)
+struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
+ unsigned int *base_gsi);
int vioapic_init(struct domain *d);
void vioapic_deinit(struct domain *d);
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 6/7] x86/ioapic: add prototype for apic_gsi_base to io_apic.h
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
` (4 preceding siblings ...)
2017-03-27 10:18 ` [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-28 11:35 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 7/7] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC Roger Pau Monne
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, 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>
---
Changes since v1:
- Add io_ prefix to avoid confusion.
- Make the parameter unsigned.
---
xen/arch/x86/io_apic.c | 4 +---
xen/arch/x86/mpparse.c | 2 +-
xen/include/asm-x86/io_apic.h | 3 +++
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 24ee431b00..d18046067c 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;
@@ -2286,7 +2284,7 @@ static int apic_pin_2_gsi_irq(int apic, int pin)
idx = find_irq_entry(apic, pin, mp_INT);
return idx >= 0 ? pin_2_irq(idx, apic, pin)
- : apic_gsi_base(apic) + pin;
+ : io_apic_gsi_base(apic) + pin;
}
int ioapic_guest_read(unsigned long physbase, unsigned int reg, u32 *pval)
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index 1eb7c99ea7..efcbc6115d 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -913,7 +913,7 @@ unsigned __init highest_gsi(void)
return res;
}
-unsigned apic_gsi_base(int apic)
+unsigned int io_apic_gsi_base(unsigned int apic)
{
return mp_ioapic_routing[apic].gsi_base;
}
diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index 225edd63b2..8029c8f400 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 int io_apic_gsi_base(unsigned int apic);
+
/* Only need to remap ioapic RTE (reg: 10~3Fh) */
#define ioapic_reg_remapped(reg) (iommu_intremap && ((reg) >= 0x10))
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 7/7] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
` (5 preceding siblings ...)
2017-03-27 10:18 ` [PATCH v2 6/7] x86/ioapic: add prototype for apic_gsi_base to io_apic.h Roger Pau Monne
@ 2017-03-27 10:18 ` Roger Pau Monne
2017-03-28 11:48 ` Jan Beulich
6 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich, 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/hvm/dom0_build.c | 33 ++++++++++++---------------------
xen/arch/x86/hvm/hvm.c | 8 +++++---
xen/arch/x86/hvm/vioapic.c | 30 ++++++++++++++++++++++++------
3 files changed, 41 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index daa791d3f4..db9be87612 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -681,12 +681,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;
@@ -716,23 +711,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 = io_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/hvm.c b/xen/arch/x86/hvm/hvm.c
index 9a6cd9c9bf..322b3b8235 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -595,6 +595,7 @@ static int hvm_print_line(
int hvm_domain_initialise(struct domain *d)
{
+ unsigned int nr_gsis;
int rc;
if ( !hvm_enabled )
@@ -615,19 +616,20 @@ int hvm_domain_initialise(struct domain *d)
if ( rc != 0 )
goto fail0;
+ nr_gsis = is_hardware_domain(d) ? nr_irqs_gsi : VIOAPIC_NUM_PINS;
d->arch.hvm_domain.pl_time = xzalloc(struct pl_time);
d->arch.hvm_domain.params = xzalloc_array(uint64_t, HVM_NR_PARAMS);
d->arch.hvm_domain.io_handler = xzalloc_array(struct hvm_io_handler,
NR_IO_HANDLERS);
- d->arch.hvm_domain.irq = xzalloc_bytes(hvm_irq_size(VIOAPIC_NUM_PINS));
+ d->arch.hvm_domain.irq = xzalloc_bytes(hvm_irq_size(nr_gsis));
rc = -ENOMEM;
if ( !d->arch.hvm_domain.pl_time || !d->arch.hvm_domain.irq ||
!d->arch.hvm_domain.params || !d->arch.hvm_domain.io_handler )
goto fail1;
- /* Set the default number of GSIs */
- hvm_domain_irq(d)->nr_gsis = VIOAPIC_NUM_PINS;
+ /* Set the number of GSIs */
+ hvm_domain_irq(d)->nr_gsis = nr_gsis;
/* need link to containing domain */
d->arch.hvm_domain.pl_time->domain = d;
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 327a9758e0..c349a3ee61 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -533,9 +533,19 @@ void vioapic_reset(struct domain *d)
memset(vioapic, 0, hvm_vioapic_size(nr_pins));
for ( j = 0; j < 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->nr_pins = nr_pins;
vioapic->domain = d;
}
@@ -556,7 +566,7 @@ static void vioapic_free(const struct domain *d, unsigned int nr_vioapics)
int vioapic_init(struct domain *d)
{
- unsigned int i, nr_vioapics = 1;
+ unsigned int i, nr_vioapics, nr_gsis = 0;
if ( !has_vioapic(d) )
{
@@ -564,6 +574,8 @@ int vioapic_init(struct domain *d)
return 0;
}
+ nr_vioapics = is_hardware_domain(d) ? nr_ioapics : 1;
+
if ( (d->arch.hvm_domain.vioapic == NULL) &&
((d->arch.hvm_domain.vioapic =
xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
@@ -571,15 +583,21 @@ int vioapic_init(struct domain *d)
for ( i = 0; i < nr_vioapics; i++ )
{
+ unsigned int nr_pins = is_hardware_domain(d) ? nr_ioapic_entries[i]
+ : VIOAPIC_NUM_PINS;
+
if ( (domain_vioapic(d, i) =
- xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
+ xmalloc_bytes(hvm_vioapic_size(nr_pins))) == NULL )
{
vioapic_free(d, nr_vioapics);
return -ENOMEM;
}
- domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
+ domain_vioapic(d, i)->nr_pins = nr_pins;
+ nr_gsis += nr_pins;
}
+ ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis);
+
d->arch.hvm_domain.nr_vioapics = nr_vioapics;
vioapic_reset(d);
--
2.12.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure
2017-03-27 10:18 ` [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure Roger Pau Monne
@ 2017-03-27 15:38 ` Jan Beulich
2017-03-27 16:49 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-27 15:38 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> The reason to expand the hvm_vioapic structure instead of the hvm_hw_vioapic
> one is that the variable number of pins functionality is only going to be used
> by the hardware domain, so no modifications are needed to the save format.
As you say here you expand an existing structure, so the title
isn't really correct.
> @@ -426,38 +426,43 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>
> static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
> {
> - struct hvm_hw_vioapic *s = domain_vioapic(d);
> + struct hvm_vioapic *s = domain_vioapic(d);
>
> if ( !has_vioapic(d) )
> return 0;
>
> - return hvm_save_entry(IOAPIC, 0, h, s);
> + BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) !=
> + sizeof(struct hvm_vioapic) -
> + offsetof(struct hvm_vioapic, base_address));
This is too weak a check for my taste, and ...
> + return hvm_save_entry(IOAPIC, 0, h, &s->base_address);
... this too fragile a use. See also below.
> --- a/xen/include/asm-x86/hvm/vioapic.h
> +++ b/xen/include/asm-x86/hvm/vioapic.h
> @@ -48,13 +48,16 @@
> #define VIOAPIC_REG_RTE0 0x10
>
> struct hvm_vioapic {
> - struct hvm_hw_vioapic hvm_hw_vioapic;
> struct domain *domain;
> + /* Layout below must match hvm_hw_vioapic. */
> + uint64_t base_address;
> + uint32_t ioregsel;
> + uint32_t id;
> + union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
> };
It is mere luck that last old and first new fields aren't both 32-bit
ones, or else this approach would not have worked at all. I think
we need some better approach here, but the absolute minimum
would be to also add a comment on the other side.
One possible approach would be to move the entire set of field
declarations of struct hvm_hw_vioapic into a macro, using it both
there and here. Which would then leave making sure there are no
alignment effects because of fields added ahead of that macro's
use (perhaps via BUILD_BUG_ON(), or maybe even better by
making this an unnamed structure member inside struct
hvm_ioapic).
The macro should then be #undef-ed in the header, except in the
__XEN__ case. Perhaps the macro would also want to be given a
parameter right away for the invoking site to specify the number
of pins.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro
2017-03-27 10:18 ` [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro Roger Pau Monne
@ 2017-03-27 15:41 ` Jan Beulich
2017-03-27 15:55 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-27 15:41 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, Kevin Tian, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> Introduce a macro to get a pointer to the hvm_irq for a HVM domain.
What does this buy us? I don't (yet?) see the connection to this
series.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro
2017-03-27 15:41 ` Jan Beulich
@ 2017-03-27 15:55 ` Roger Pau Monne
0 siblings, 0 replies; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 15:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Kevin Tian, xen-devel
On Mon, Mar 27, 2017 at 09:41:40AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> > Introduce a macro to get a pointer to the hvm_irq for a HVM domain.
>
> What does this buy us? I don't (yet?) see the connection to this
> series.
It's a pre-patch to make #3 smaller, I should probably mention this in the
commit.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-27 10:18 ` [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array Roger Pau Monne
@ 2017-03-27 15:59 ` Jan Beulich
2017-03-27 17:28 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-27 15:59 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> Rearrange the fields of hvm_irq so that gsi_assert_count can be converted into
> a variable size array and add a new field to account the number of GSIs.
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -457,7 +457,7 @@ void hvm_migrate_pirqs(struct vcpu *v)
> {
> struct domain *d = v->domain;
>
> - if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
> + if ( !iommu_enabled || !d->arch.hvm_domain.irq->dpci )
If the prior patch is really needed - why did it not convert this?
(There are more such instances further down.)
> @@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
> struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>
> - ASSERT(isa_irq <= 15);
> + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>
> spin_lock(&d->arch.hvm_domain.irq_lock);
>
> @@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
> struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>
> - ASSERT(isa_irq <= 15);
> + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
Not sure about these two - perhaps they'd better be
BUILD_BUG_ON()s for the DomU side, and Dom0 should never
be allocated less than 16?
> @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
> (uint32_t) hvm_irq->isa_irq.pad[0],
> hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
> hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
> - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
> + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
> printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
> " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
> i, i+7,
> @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
> hvm_irq->gsi_assert_count[i+5],
> hvm_irq->gsi_assert_count[i+6],
> hvm_irq->gsi_assert_count[i+7]);
> + if ( i != hvm_irq->nr_gsis )
> + {
> + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
> + for ( ; i < hvm_irq->nr_gsis; i++)
> + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
> + printk("\n");
> + }
I realize you've just copied this from existing code, but what
advantage does %2.2 have over just %2 here?
> @@ -545,6 +554,9 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
> unsigned int asserted, pdev, pintx;
> int rc;
>
> + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
> + return -EOPNOTSUPP;
> +
> spin_lock(&d->arch.hvm_domain.irq_lock);
>
> pdev = hvm_irq->callback_via.pci.dev;
> @@ -592,6 +604,9 @@ static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
> struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> int link, dev, intx, gsi;
>
> + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
> + return -EOPNOTSUPP;
Why do you add these checks here?
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -17,7 +17,7 @@
> #define is_pv_32bit_vcpu(v) (is_pv_32bit_domain((v)->domain))
>
> #define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \
> - d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
> + d->arch.hvm_domain.irq->callback_via_type == HVMIRQ_callback_vector)
Please take the opportunity and properly parenthesize the use of
d here.
> @@ -89,13 +77,27 @@ struct hvm_irq {
> u8 round_robin_prev_vcpu;
>
> struct hvm_irq_dpci *dpci;
> +
> + /*
> + * Number of wires asserting each GSI.
> + *
> + * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
> + * except ISA IRQ 0, which is connected to GSI 2.
> + * PCI links map into this space via the PCI-ISA bridge.
> + *
> + * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
> + * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
> + */
> + unsigned int nr_gsis;
> + u8 gsi_assert_count[];
> };
I'm afraid at the same time (or in a later patch) the type of this array
(and maybe also the type of pci_link_assert_count[]) will need to be
widened.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 4/7] x86/vioapic: allow the vIO APIC to have a variable number of pins
2017-03-27 10:18 ` [PATCH v2 4/7] x86/vioapic: allow the vIO APIC to have a variable number of pins Roger Pau Monne
@ 2017-03-27 16:10 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-03-27 16:10 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> @@ -431,9 +431,8 @@ static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
> if ( !has_vioapic(d) )
> return 0;
>
> - BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) !=
> - sizeof(struct hvm_vioapic) -
> - offsetof(struct hvm_vioapic, base_address));
> + if ( s->nr_pins != VIOAPIC_NUM_PINS )
> + return -EOPNOTSUPP;
So now you remove the little bit of guarding you had altogether?
It would also be nice if you #undef-ed VIOAPIC_NUM_PINS in the
__XEN__ case to document that all uses of it were caught. In the
few cases were you really still need the number (like above) you
could easily use ARRAY_SIZE().
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure
2017-03-27 15:38 ` Jan Beulich
@ 2017-03-27 16:49 ` Roger Pau Monne
2017-03-28 7:17 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 16:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Mon, Mar 27, 2017 at 09:38:49AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> > The reason to expand the hvm_vioapic structure instead of the hvm_hw_vioapic
> > one is that the variable number of pins functionality is only going to be used
> > by the hardware domain, so no modifications are needed to the save format.
>
> As you say here you expand an existing structure, so the title
> isn't really correct.
>
> > @@ -426,38 +426,43 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> >
> > static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
> > {
> > - struct hvm_hw_vioapic *s = domain_vioapic(d);
> > + struct hvm_vioapic *s = domain_vioapic(d);
> >
> > if ( !has_vioapic(d) )
> > return 0;
> >
> > - return hvm_save_entry(IOAPIC, 0, h, s);
> > + BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) !=
> > + sizeof(struct hvm_vioapic) -
> > + offsetof(struct hvm_vioapic, base_address));
>
> This is too weak a check for my taste, and ...
I've removed this BUILD_BUG_ON.
> > + return hvm_save_entry(IOAPIC, 0, h, &s->base_address);
>
> ... this too fragile a use. See also below.
>
> > --- a/xen/include/asm-x86/hvm/vioapic.h
> > +++ b/xen/include/asm-x86/hvm/vioapic.h
> > @@ -48,13 +48,16 @@
> > #define VIOAPIC_REG_RTE0 0x10
> >
> > struct hvm_vioapic {
> > - struct hvm_hw_vioapic hvm_hw_vioapic;
> > struct domain *domain;
> > + /* Layout below must match hvm_hw_vioapic. */
> > + uint64_t base_address;
> > + uint32_t ioregsel;
> > + uint32_t id;
> > + union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
> > };
>
> It is mere luck that last old and first new fields aren't both 32-bit
> ones, or else this approach would not have worked at all. I think
> we need some better approach here, but the absolute minimum
> would be to also add a comment on the other side.
>
> One possible approach would be to move the entire set of field
> declarations of struct hvm_hw_vioapic into a macro, using it both
> there and here. Which would then leave making sure there are no
> alignment effects because of fields added ahead of that macro's
> use (perhaps via BUILD_BUG_ON(), or maybe even better by
> making this an unnamed structure member inside struct
> hvm_ioapic).
>
> The macro should then be #undef-ed in the header, except in the
> __XEN__ case. Perhaps the macro would also want to be given a
> parameter right away for the invoking site to specify the number
> of pins.
Yes, I think the unnamed structure is way better, here's what I've done:
save.h:
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;
};
#define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */
#define DECLARE_VIOAPIC(name, cnt) \
struct name { \
uint64_t base_address; \
uint32_t ioregsel; \
uint32_t id; \
union vioapic_redir_entry redirtbl[cnt]; \
}
DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS);
#ifndef __XEN__
#undef DECLARE_VIOAPIC
#endif
vioapic.h:
struct hvm_vioapic {
struct domain *domain;
DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
};
This seems to work fine, and now the BUILD_BUG_ON is just pointless.
Thanks for the tip, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-27 15:59 ` Jan Beulich
@ 2017-03-27 17:28 ` Roger Pau Monne
2017-03-28 7:25 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-27 17:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Mon, Mar 27, 2017 at 09:59:42AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -457,7 +457,7 @@ void hvm_migrate_pirqs(struct vcpu *v)
> > {
> > struct domain *d = v->domain;
> >
> > - if ( !iommu_enabled || !d->arch.hvm_domain.irq.dpci )
> > + if ( !iommu_enabled || !d->arch.hvm_domain.irq->dpci )
>
> If the prior patch is really needed - why did it not convert this?
> (There are more such instances further down.)
It's now done.
> > @@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
> >
> > - ASSERT(isa_irq <= 15);
> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
> >
> > spin_lock(&d->arch.hvm_domain.irq_lock);
> >
> > @@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
> >
> > - ASSERT(isa_irq <= 15);
> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>
> Not sure about these two - perhaps they'd better be
> BUILD_BUG_ON()s for the DomU side, and Dom0 should never
> be allocated less than 16?
Hm, I could add:
BUILD_BUG_ON(VIOAPIC_NUM_PINS < 16);
But this is going to make it harder to remove VIOAPIC_NUM_PINS later on.
> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
> > (uint32_t) hvm_irq->isa_irq.pad[0],
> > hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
> > hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
> > - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
> > + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
> > printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
> > " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
> > i, i+7,
> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
> > hvm_irq->gsi_assert_count[i+5],
> > hvm_irq->gsi_assert_count[i+6],
> > hvm_irq->gsi_assert_count[i+7]);
> > + if ( i != hvm_irq->nr_gsis )
> > + {
> > + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
> > + for ( ; i < hvm_irq->nr_gsis; i++)
> > + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
> > + printk("\n");
> > + }
>
> I realize you've just copied this from existing code, but what
> advantage does %2.2 have over just %2 here?
Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
would you rather prefer a separate patch?
> > @@ -545,6 +554,9 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
> > unsigned int asserted, pdev, pintx;
> > int rc;
> >
> > + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
> > + return -EOPNOTSUPP;
> > +
> > spin_lock(&d->arch.hvm_domain.irq_lock);
> >
> > pdev = hvm_irq->callback_via.pci.dev;
> > @@ -592,6 +604,9 @@ static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> > int link, dev, intx, gsi;
> >
> > + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
> > + return -EOPNOTSUPP;
>
> Why do you add these checks here?
Because there's a:
for ( gsi = 0; gsi < VIOAPIC_NUM_PINS; gsi++ )
hvm_irq->gsi_assert_count[gsi] = 0;
A little bit below. I can change that to use nr_gsis instead and remove those
checks (both here and in irq_save_pci).
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -17,7 +17,7 @@
> > #define is_pv_32bit_vcpu(v) (is_pv_32bit_domain((v)->domain))
> >
> > #define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \
> > - d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
> > + d->arch.hvm_domain.irq->callback_via_type == HVMIRQ_callback_vector)
>
> Please take the opportunity and properly parenthesize the use of
> d here.
Done.
> > @@ -89,13 +77,27 @@ struct hvm_irq {
> > u8 round_robin_prev_vcpu;
> >
> > struct hvm_irq_dpci *dpci;
> > +
> > + /*
> > + * Number of wires asserting each GSI.
> > + *
> > + * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
> > + * except ISA IRQ 0, which is connected to GSI 2.
> > + * PCI links map into this space via the PCI-ISA bridge.
> > + *
> > + * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
> > + * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
> > + */
> > + unsigned int nr_gsis;
> > + u8 gsi_assert_count[];
> > };
>
> I'm afraid at the same time (or in a later patch) the type of this array
> (and maybe also the type of pci_link_assert_count[]) will need to be
> widened.
This one seems to be fine for PVH Dom0 (you will have to look at the next
series), but I specifically avoid touching pci_link_assert_count. Again, better
to comment this on the next patch series that actually implements the interrupt
binding. This is needed because code in vioapic makes use of
gsi_assert_count.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure
2017-03-27 16:49 ` Roger Pau Monne
@ 2017-03-28 7:17 ` Jan Beulich
2017-03-28 8:15 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 7:17 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 18:49, <roger.pau@citrix.com> wrote:
> Yes, I think the unnamed structure is way better, here's what I've done:
>
> save.h:
>
> 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;
> };
>
> #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */
>
> #define DECLARE_VIOAPIC(name, cnt) \
> struct name { \
> uint64_t base_address; \
> uint32_t ioregsel; \
> uint32_t id; \
> union vioapic_redir_entry redirtbl[cnt]; \
> }
>
> DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS);
>
> #ifndef __XEN__
> #undef DECLARE_VIOAPIC
> #endif
>
> vioapic.h:
>
> struct hvm_vioapic {
> struct domain *domain;
> DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
> };
>
> This seems to work fine, and now the BUILD_BUG_ON is just pointless.
Well, no, not entirely. As said you still want to exclude alignment
effects prior structure members of hvm_vioapic may have (please
continue to not make assumptions on the alignment of the first
field of the structure here).
Furthermore, despite the #undef the macro name should start
with XEN_, perhaps even with XEN_HVM_. Whether the
DECLARE part is really needed/useful I'm not sure.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-27 17:28 ` Roger Pau Monne
@ 2017-03-28 7:25 ` Jan Beulich
2017-03-28 8:40 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 7:25 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 19:28, <roger.pau@citrix.com> wrote:
> On Mon, Mar 27, 2017 at 09:59:42AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
>> > @@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
>> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>> >
>> > - ASSERT(isa_irq <= 15);
>> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>> >
>> > spin_lock(&d->arch.hvm_domain.irq_lock);
>> >
>> > @@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
>> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>> >
>> > - ASSERT(isa_irq <= 15);
>> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>>
>> Not sure about these two - perhaps they'd better be
>> BUILD_BUG_ON()s for the DomU side, and Dom0 should never
>> be allocated less than 16?
>
> Hm, I could add:
>
> BUILD_BUG_ON(VIOAPIC_NUM_PINS < 16);
>
> But this is going to make it harder to remove VIOAPIC_NUM_PINS later on.
As said elsewhere, those remaining uses could/should become
ARRAY_SIZE().
>> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
>> > (uint32_t) hvm_irq->isa_irq.pad[0],
>> > hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
>> > hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
>> > - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
>> > + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
>> > printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
>> > " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
>> > i, i+7,
>> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
>> > hvm_irq->gsi_assert_count[i+5],
>> > hvm_irq->gsi_assert_count[i+6],
>> > hvm_irq->gsi_assert_count[i+7]);
>> > + if ( i != hvm_irq->nr_gsis )
>> > + {
>> > + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
>> > + for ( ; i < hvm_irq->nr_gsis; i++)
>> > + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
>> > + printk("\n");
>> > + }
>>
>> I realize you've just copied this from existing code, but what
>> advantage does %2.2 have over just %2 here?
>
> Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
> would you rather prefer a separate patch?
Well, if you also want to change the existing ones, then a separate
patch would be preferred. As to %2 vs %3 - how would the latter
be any better if we want/need to change the type from u8 anyway?
>> > @@ -545,6 +554,9 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
>> > unsigned int asserted, pdev, pintx;
>> > int rc;
>> >
>> > + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
>> > + return -EOPNOTSUPP;
>> > +
>> > spin_lock(&d->arch.hvm_domain.irq_lock);
>> >
>> > pdev = hvm_irq->callback_via.pci.dev;
>> > @@ -592,6 +604,9 @@ static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
>> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> > int link, dev, intx, gsi;
>> >
>> > + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
>> > + return -EOPNOTSUPP;
>>
>> Why do you add these checks here?
>
> Because there's a:
>
> for ( gsi = 0; gsi < VIOAPIC_NUM_PINS; gsi++ )
> hvm_irq->gsi_assert_count[gsi] = 0;
>
> A little bit below.
True for the load side, but I can't see anything like that for save.
> I can change that to use nr_gsis instead and remove those
> checks (both here and in irq_save_pci).
Please do, there's no harm using the dynamic upper bound in
that case.
>> > @@ -89,13 +77,27 @@ struct hvm_irq {
>> > u8 round_robin_prev_vcpu;
>> >
>> > struct hvm_irq_dpci *dpci;
>> > +
>> > + /*
>> > + * Number of wires asserting each GSI.
>> > + *
>> > + * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
>> > + * except ISA IRQ 0, which is connected to GSI 2.
>> > + * PCI links map into this space via the PCI-ISA bridge.
>> > + *
>> > + * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
>> > + * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
>> > + */
>> > + unsigned int nr_gsis;
>> > + u8 gsi_assert_count[];
>> > };
>>
>> I'm afraid at the same time (or in a later patch) the type of this array
>> (and maybe also the type of pci_link_assert_count[]) will need to be
>> widened.
>
> This one seems to be fine for PVH Dom0 (you will have to look at the next
> series), but I specifically avoid touching pci_link_assert_count. Again, better
> to comment this on the next patch series that actually implements the interrupt
> binding. This is needed because code in vioapic makes use of
> gsi_assert_count.
Well, we can certainly discuss this there (whenever I get to look at
it, as that's not 4.9 stuff now anymore), but for now I don't see the
connection between my comment and your reply.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure
2017-03-28 7:17 ` Jan Beulich
@ 2017-03-28 8:15 ` Roger Pau Monne
2017-03-28 8:58 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-28 8:15 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Mar 28, 2017 at 01:17:27AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 18:49, <roger.pau@citrix.com> wrote:
> > Yes, I think the unnamed structure is way better, here's what I've done:
> >
> > save.h:
> >
> > 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;
> > };
> >
> > #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */
> >
> > #define DECLARE_VIOAPIC(name, cnt) \
> > struct name { \
> > uint64_t base_address; \
> > uint32_t ioregsel; \
> > uint32_t id; \
> > union vioapic_redir_entry redirtbl[cnt]; \
> > }
> >
> > DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS);
> >
> > #ifndef __XEN__
> > #undef DECLARE_VIOAPIC
> > #endif
> >
> > vioapic.h:
> >
> > struct hvm_vioapic {
> > struct domain *domain;
> > DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
> > };
> >
> > This seems to work fine, and now the BUILD_BUG_ON is just pointless.
>
> Well, no, not entirely. As said you still want to exclude alignment
> effects prior structure members of hvm_vioapic may have (please
> continue to not make assumptions on the alignment of the first
> field of the structure here).
But doesn't the usage of an unnamed structure get rid of the alignment issue?
For example I have the following code:
struct bar {
uint8_t b;
uint8_t c;
uint32_t d;
uint32_t e;
};
struct foo {
uint8_t a;
struct {
uint8_t b;
uint8_t c;
uint32_t d;
uint32_t e;
};
};
struct foobar {
uint8_t a;
uint8_t b;
uint8_t c;
uint32_t d;
uint32_t e;
};
This according to pahole has the following layout:
struct bar {
uint8_t b; /* 0 1 */
uint8_t c; /* 1 1 */
/* XXX 2 bytes hole, try to pack */
uint32_t d; /* 4 4 */
uint32_t e; /* 8 4 */
/* size: 12, cachelines: 1, members: 4 */
/* sum members: 10, holes: 1, sum holes: 2 */
/* last cacheline: 12 bytes */
};
struct foo {
uint8_t a; /* 0 1 */
/* XXX 3 bytes hole, try to pack */
struct {
uint8_t b; /* 4 1 */
uint8_t c; /* 5 1 */
uint32_t d; /* 8 4 */
uint32_t e; /* 12 4 */
}; /* 4 12 */
/* size: 16, cachelines: 1, members: 2 */
/* sum members: 13, holes: 1, sum holes: 3 */
/* last cacheline: 16 bytes */
};
struct foobar {
uint8_t a; /* 0 1 */
uint8_t b; /* 1 1 */
uint8_t c; /* 2 1 */
/* XXX 1 byte hole, try to pack */
uint32_t d; /* 4 4 */
uint32_t e; /* 8 4 */
/* size: 12, cachelines: 1, members: 5 */
/* sum members: 11, holes: 1, sum holes: 1 */
/* last cacheline: 12 bytes */
};
The unnamed struct (clone of bar) inside of foo is already aligned (so it ends
up with the same layout as bar), as opposed to foobar, which indeed ends up
with a different layout.
> Furthermore, despite the #undef the macro name should start
> with XEN_, perhaps even with XEN_HVM_. Whether the
> DECLARE part is really needed/useful I'm not sure.
I've renamed it to XEN_HVM_VIOAPIC.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 7:25 ` Jan Beulich
@ 2017-03-28 8:40 ` Roger Pau Monne
2017-03-28 9:10 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-28 8:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 19:28, <roger.pau@citrix.com> wrote:
> > On Mon, Mar 27, 2017 at 09:59:42AM -0600, Jan Beulich wrote:
> >> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> >> > @@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
> >> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> >> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
> >> >
> >> > - ASSERT(isa_irq <= 15);
> >> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
> >> >
> >> > spin_lock(&d->arch.hvm_domain.irq_lock);
> >> >
> >> > @@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
> >> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> >> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
> >> >
> >> > - ASSERT(isa_irq <= 15);
> >> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
> >>
> >> Not sure about these two - perhaps they'd better be
> >> BUILD_BUG_ON()s for the DomU side, and Dom0 should never
> >> be allocated less than 16?
> >
> > Hm, I could add:
> >
> > BUILD_BUG_ON(VIOAPIC_NUM_PINS < 16);
> >
> > But this is going to make it harder to remove VIOAPIC_NUM_PINS later on.
>
> As said elsewhere, those remaining uses could/should become
> ARRAY_SIZE().
Hm, but then I will have to use:
ARRAY_SIZE(((struct hvm_hw_vioapic *)0)->redirtbl)
Which is kind of cumbersome? I can define that into a macro I guess.
> >> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
> >> > (uint32_t) hvm_irq->isa_irq.pad[0],
> >> > hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
> >> > hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
> >> > - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
> >> > + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
> >> > printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
> >> > " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
> >> > i, i+7,
> >> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
> >> > hvm_irq->gsi_assert_count[i+5],
> >> > hvm_irq->gsi_assert_count[i+6],
> >> > hvm_irq->gsi_assert_count[i+7]);
> >> > + if ( i != hvm_irq->nr_gsis )
> >> > + {
> >> > + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
> >> > + for ( ; i < hvm_irq->nr_gsis; i++)
> >> > + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
> >> > + printk("\n");
> >> > + }
> >>
> >> I realize you've just copied this from existing code, but what
> >> advantage does %2.2 have over just %2 here?
> >
> > Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
> > would you rather prefer a separate patch?
>
> Well, if you also want to change the existing ones, then a separate
> patch would be preferred. As to %2 vs %3 - how would the latter
> be any better if we want/need to change the type from u8 anyway?
ATM uint8_t can be 255, so %3 seems better (although AFAICT this is limited to
4, because there are 4 INT# lines to each GSI, and pending GSIs are not
accumulated). I'm not sure I follow why do we want/need to change the type.
> >> > @@ -545,6 +554,9 @@ static int irq_save_pci(struct domain *d, hvm_domain_context_t *h)
> >> > unsigned int asserted, pdev, pintx;
> >> > int rc;
> >> >
> >> > + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
> >> > + return -EOPNOTSUPP;
> >> > +
> >> > spin_lock(&d->arch.hvm_domain.irq_lock);
> >> >
> >> > pdev = hvm_irq->callback_via.pci.dev;
> >> > @@ -592,6 +604,9 @@ static int irq_load_pci(struct domain *d, hvm_domain_context_t *h)
> >> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> >> > int link, dev, intx, gsi;
> >> >
> >> > + if ( hvm_irq->nr_gsis != VIOAPIC_NUM_PINS )
> >> > + return -EOPNOTSUPP;
> >>
> >> Why do you add these checks here?
> >
> > Because there's a:
> >
> > for ( gsi = 0; gsi < VIOAPIC_NUM_PINS; gsi++ )
> > hvm_irq->gsi_assert_count[gsi] = 0;
> >
> > A little bit below.
>
> True for the load side, but I can't see anything like that for save.
>
> > I can change that to use nr_gsis instead and remove those
> > checks (both here and in irq_save_pci).
>
> Please do, there's no harm using the dynamic upper bound in
> that case.
Now done.
> >> > @@ -89,13 +77,27 @@ struct hvm_irq {
> >> > u8 round_robin_prev_vcpu;
> >> >
> >> > struct hvm_irq_dpci *dpci;
> >> > +
> >> > + /*
> >> > + * Number of wires asserting each GSI.
> >> > + *
> >> > + * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
> >> > + * except ISA IRQ 0, which is connected to GSI 2.
> >> > + * PCI links map into this space via the PCI-ISA bridge.
> >> > + *
> >> > + * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
> >> > + * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
> >> > + */
> >> > + unsigned int nr_gsis;
> >> > + u8 gsi_assert_count[];
> >> > };
> >>
> >> I'm afraid at the same time (or in a later patch) the type of this array
> >> (and maybe also the type of pci_link_assert_count[]) will need to be
> >> widened.
> >
> > This one seems to be fine for PVH Dom0 (you will have to look at the next
> > series), but I specifically avoid touching pci_link_assert_count. Again, better
> > to comment this on the next patch series that actually implements the interrupt
> > binding. This is needed because code in vioapic makes use of
> > gsi_assert_count.
>
> Well, we can certainly discuss this there (whenever I get to look at
> it, as that's not 4.9 stuff now anymore), but for now I don't see the
> connection between my comment and your reply.
Hm, I'm not sure I understand why would you like to change the type of this
array. AFAICT the type of the array is not related to the number of vIO APIC
pins, or the number of vIO APICs.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure
2017-03-28 8:15 ` Roger Pau Monne
@ 2017-03-28 8:58 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 8:58 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 28.03.17 at 10:15, <roger.pau@citrix.com> wrote:
> On Tue, Mar 28, 2017 at 01:17:27AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 18:49, <roger.pau@citrix.com> wrote:
>> > Yes, I think the unnamed structure is way better, here's what I've done:
>> >
>> > save.h:
>> >
>> > 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;
>> > };
>> >
>> > #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */
>> >
>> > #define DECLARE_VIOAPIC(name, cnt) \
>> > struct name { \
>> > uint64_t base_address; \
>> > uint32_t ioregsel; \
>> > uint32_t id; \
>> > union vioapic_redir_entry redirtbl[cnt]; \
>> > }
>> >
>> > DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS);
>> >
>> > #ifndef __XEN__
>> > #undef DECLARE_VIOAPIC
>> > #endif
>> >
>> > vioapic.h:
>> >
>> > struct hvm_vioapic {
>> > struct domain *domain;
>> > DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
>> > };
>> >
>> > This seems to work fine, and now the BUILD_BUG_ON is just pointless.
>>
>> Well, no, not entirely. As said you still want to exclude alignment
>> effects prior structure members of hvm_vioapic may have (please
>> continue to not make assumptions on the alignment of the first
>> field of the structure here).
>
> But doesn't the usage of an unnamed structure get rid of the alignment issue?
Oh, of course it does - provided there are no differing packing
options in effect at the time the two structures get actually
declared. But yes, perhaps we can live with this being implied.
And too big of an alignment is no problem in the first place.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 8:40 ` Roger Pau Monne
@ 2017-03-28 9:10 ` Jan Beulich
2017-03-28 9:34 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 9:10 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 28.03.17 at 10:40, <roger.pau@citrix.com> wrote:
> On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 19:28, <roger.pau@citrix.com> wrote:
>> > On Mon, Mar 27, 2017 at 09:59:42AM -0600, Jan Beulich wrote:
>> >> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
>> >> > @@ -122,7 +124,7 @@ void hvm_isa_irq_assert(
>> >> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> >> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>> >> >
>> >> > - ASSERT(isa_irq <= 15);
>> >> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>> >> >
>> >> > spin_lock(&d->arch.hvm_domain.irq_lock);
>> >> >
>> >> > @@ -139,7 +141,7 @@ void hvm_isa_irq_deassert(
>> >> > struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>> >> > unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>> >> >
>> >> > - ASSERT(isa_irq <= 15);
>> >> > + ASSERT(isa_irq <= 15 && isa_irq < hvm_irq->nr_gsis);
>> >>
>> >> Not sure about these two - perhaps they'd better be
>> >> BUILD_BUG_ON()s for the DomU side, and Dom0 should never
>> >> be allocated less than 16?
>> >
>> > Hm, I could add:
>> >
>> > BUILD_BUG_ON(VIOAPIC_NUM_PINS < 16);
>> >
>> > But this is going to make it harder to remove VIOAPIC_NUM_PINS later on.
>>
>> As said elsewhere, those remaining uses could/should become
>> ARRAY_SIZE().
>
> Hm, but then I will have to use:
>
> ARRAY_SIZE(((struct hvm_hw_vioapic *)0)->redirtbl)
>
> Which is kind of cumbersome? I can define that into a macro I guess.
Indeed, ugly. How about
struct hvm_vioapic {
struct domain *domain;
union {
DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
struct hvm_hw_vioapic domU;
};
};
(with "domU" subject to improvement)? This might at once allow
making the save/restore code look better.
>> >> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
>> >> > (uint32_t) hvm_irq->isa_irq.pad[0],
>> >> > hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
>> >> > hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
>> >> > - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
>> >> > + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
>> >> > printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
>> >> > " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
>> >> > i, i+7,
>> >> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
>> >> > hvm_irq->gsi_assert_count[i+5],
>> >> > hvm_irq->gsi_assert_count[i+6],
>> >> > hvm_irq->gsi_assert_count[i+7]);
>> >> > + if ( i != hvm_irq->nr_gsis )
>> >> > + {
>> >> > + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
>> >> > + for ( ; i < hvm_irq->nr_gsis; i++)
>> >> > + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
>> >> > + printk("\n");
>> >> > + }
>> >>
>> >> I realize you've just copied this from existing code, but what
>> >> advantage does %2.2 have over just %2 here?
>> >
>> > Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
>> > would you rather prefer a separate patch?
>>
>> Well, if you also want to change the existing ones, then a separate
>> patch would be preferred. As to %2 vs %3 - how would the latter
>> be any better if we want/need to change the type from u8 anyway?
>
> ATM uint8_t can be 255, so %3 seems better (although AFAICT this is limited to
> 4, because there are 4 INT# lines to each GSI, and pending GSIs are not
> accumulated). I'm not sure I follow why do we want/need to change the type.
For our DomU model there are four lines. Physical machines often
declare more in ACPI, and I'm not sure there's a theoretical upper
limit.
>> >> > @@ -89,13 +77,27 @@ struct hvm_irq {
>> >> > u8 round_robin_prev_vcpu;
>> >> >
>> >> > struct hvm_irq_dpci *dpci;
>> >> > +
>> >> > + /*
>> >> > + * Number of wires asserting each GSI.
>> >> > + *
>> >> > + * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
>> >> > + * except ISA IRQ 0, which is connected to GSI 2.
>> >> > + * PCI links map into this space via the PCI-ISA bridge.
>> >> > + *
>> >> > + * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
>> >> > + * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
>> >> > + */
>> >> > + unsigned int nr_gsis;
>> >> > + u8 gsi_assert_count[];
>> >> > };
>> >>
>> >> I'm afraid at the same time (or in a later patch) the type of this array
>> >> (and maybe also the type of pci_link_assert_count[]) will need to be
>> >> widened.
>> >
>> > This one seems to be fine for PVH Dom0 (you will have to look at the next
>> > series), but I specifically avoid touching pci_link_assert_count. Again, better
>> > to comment this on the next patch series that actually implements the interrupt
>> > binding. This is needed because code in vioapic makes use of
>> > gsi_assert_count.
>>
>> Well, we can certainly discuss this there (whenever I get to look at
>> it, as that's not 4.9 stuff now anymore), but for now I don't see the
>> connection between my comment and your reply.
>
> Hm, I'm not sure I understand why would you like to change the type of this
> array. AFAICT the type of the array is not related to the number of vIO APIC
> pins, or the number of vIO APICs.
See above - I think it is related to the number of declared INTx
lines (of which a DomU at present would only have 4). Or maybe
I'm mistaken where the INTx value comes from here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 9:10 ` Jan Beulich
@ 2017-03-28 9:34 ` Roger Pau Monne
2017-03-28 9:49 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-28 9:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Mar 28, 2017 at 03:10:27AM -0600, Jan Beulich wrote:
> >>> On 28.03.17 at 10:40, <roger.pau@citrix.com> wrote:
> > On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
> >> >>> On 27.03.17 at 19:28, <roger.pau@citrix.com> wrote:
> > ARRAY_SIZE(((struct hvm_hw_vioapic *)0)->redirtbl)
> >
> > Which is kind of cumbersome? I can define that into a macro I guess.
>
> Indeed, ugly. How about
>
> struct hvm_vioapic {
> struct domain *domain;
> union {
> DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
> struct hvm_hw_vioapic domU;
> };
> };
>
> (with "domU" subject to improvement)? This might at once allow
> making the save/restore code look better.
Right, this indeed looks better, and will make the save/restore code neater.
I'm not sure I can come up with a better name, "guest", "static"?
> >> >> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
> >> >> > (uint32_t) hvm_irq->isa_irq.pad[0],
> >> >> > hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
> >> >> > hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
> >> >> > - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
> >> >> > + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
> >> >> > printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
> >> >> > " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
> >> >> > i, i+7,
> >> >> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
> >> >> > hvm_irq->gsi_assert_count[i+5],
> >> >> > hvm_irq->gsi_assert_count[i+6],
> >> >> > hvm_irq->gsi_assert_count[i+7]);
> >> >> > + if ( i != hvm_irq->nr_gsis )
> >> >> > + {
> >> >> > + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
> >> >> > + for ( ; i < hvm_irq->nr_gsis; i++)
> >> >> > + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
> >> >> > + printk("\n");
> >> >> > + }
> >> >>
> >> >> I realize you've just copied this from existing code, but what
> >> >> advantage does %2.2 have over just %2 here?
> >> >
> >> > Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
> >> > would you rather prefer a separate patch?
> >>
> >> Well, if you also want to change the existing ones, then a separate
> >> patch would be preferred. As to %2 vs %3 - how would the latter
> >> be any better if we want/need to change the type from u8 anyway?
> >
> > ATM uint8_t can be 255, so %3 seems better (although AFAICT this is limited to
> > 4, because there are 4 INT# lines to each GSI, and pending GSIs are not
> > accumulated). I'm not sure I follow why do we want/need to change the type.
>
> For our DomU model there are four lines. Physical machines often
> declare more in ACPI, and I'm not sure there's a theoretical upper
> limit.
In any case, I'm not sure this is relevant to PVH Dom0, where Xen should simply
bind GSIs, but has no idea of all the interrupt routing behind them. This is
relevant to DomUs because in that case Xen acts as a PCI interrupt router
AFAICT.
> >> >> > @@ -89,13 +77,27 @@ struct hvm_irq {
> >> >> > u8 round_robin_prev_vcpu;
> >> >> >
> >> >> > struct hvm_irq_dpci *dpci;
> >> >> > +
> >> >> > + /*
> >> >> > + * Number of wires asserting each GSI.
> >> >> > + *
> >> >> > + * GSIs 0-15 are the ISA IRQs. ISA devices map directly into this space
> >> >> > + * except ISA IRQ 0, which is connected to GSI 2.
> >> >> > + * PCI links map into this space via the PCI-ISA bridge.
> >> >> > + *
> >> >> > + * GSIs 16+ are used only be PCI devices. The mapping from PCI device to
> >> >> > + * GSI is as follows: ((device*4 + device/8 + INTx#) & 31) + 16
> >> >> > + */
> >> >> > + unsigned int nr_gsis;
> >> >> > + u8 gsi_assert_count[];
> >> >> > };
> >> >>
> >> >> I'm afraid at the same time (or in a later patch) the type of this array
> >> >> (and maybe also the type of pci_link_assert_count[]) will need to be
> >> >> widened.
> >> >
> >> > This one seems to be fine for PVH Dom0 (you will have to look at the next
> >> > series), but I specifically avoid touching pci_link_assert_count. Again, better
> >> > to comment this on the next patch series that actually implements the interrupt
> >> > binding. This is needed because code in vioapic makes use of
> >> > gsi_assert_count.
> >>
> >> Well, we can certainly discuss this there (whenever I get to look at
> >> it, as that's not 4.9 stuff now anymore), but for now I don't see the
> >> connection between my comment and your reply.
> >
> > Hm, I'm not sure I understand why would you like to change the type of this
> > array. AFAICT the type of the array is not related to the number of vIO APIC
> > pins, or the number of vIO APICs.
>
> See above - I think it is related to the number of declared INTx
> lines (of which a DomU at present would only have 4). Or maybe
> I'm mistaken where the INTx value comes from here.
AFAICT in any case the INTx is always limited to INTA-INTD (4), but I don't
think this is important for Dom0, because there Xen doesn't act as a PCI
interrupt router, and thus doesn't need to know the INTx. In fact
gsi_assert_count used with a PVH Dom0 should either be 0 or 1, because Xen is
identity mapping machine GSIs into guest GSIs, without any kind of routing.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 9:34 ` Roger Pau Monne
@ 2017-03-28 9:49 ` Jan Beulich
2017-03-28 9:59 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 9:49 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 28.03.17 at 11:34, <roger.pau@citrix.com> wrote:
> On Tue, Mar 28, 2017 at 03:10:27AM -0600, Jan Beulich wrote:
>> >>> On 28.03.17 at 10:40, <roger.pau@citrix.com> wrote:
>> > On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
>> >> >>> On 27.03.17 at 19:28, <roger.pau@citrix.com> wrote:
>> > ARRAY_SIZE(((struct hvm_hw_vioapic *)0)->redirtbl)
>> >
>> > Which is kind of cumbersome? I can define that into a macro I guess.
>>
>> Indeed, ugly. How about
>>
>> struct hvm_vioapic {
>> struct domain *domain;
>> union {
>> DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS);
>> struct hvm_hw_vioapic domU;
>> };
>> };
>>
>> (with "domU" subject to improvement)? This might at once allow
>> making the save/restore code look better.
>
> Right, this indeed looks better, and will make the save/restore code neater.
> I'm not sure I can come up with a better name, "guest", "static"?
Well, "static" is a keyword, so can't be used, and Dom0 is a "guest"
too.
>> >> >> > @@ -495,7 +497,7 @@ static void irq_dump(struct domain *d)
>> >> >> > (uint32_t) hvm_irq->isa_irq.pad[0],
>> >> >> > hvm_irq->pci_link.route[0], hvm_irq->pci_link.route[1],
>> >> >> > hvm_irq->pci_link.route[2], hvm_irq->pci_link.route[3]);
>> >> >> > - for ( i = 0 ; i < VIOAPIC_NUM_PINS; i += 8 )
>> >> >> > + for ( i = 0; i < hvm_irq->nr_gsis && i + 8 <= hvm_irq->nr_gsis; i += 8 )
>> >> >> > printk("GSI [%x - %x] %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8
>> >> >> > " %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8" %2.2"PRIu8"\n",
>> >> >> > i, i+7,
>> >> >> > @@ -507,6 +509,13 @@ static void irq_dump(struct domain *d)
>> >> >> > hvm_irq->gsi_assert_count[i+5],
>> >> >> > hvm_irq->gsi_assert_count[i+6],
>> >> >> > hvm_irq->gsi_assert_count[i+7]);
>> >> >> > + if ( i != hvm_irq->nr_gsis )
>> >> >> > + {
>> >> >> > + printk("GSI [%x - %x]", i, hvm_irq->nr_gsis - 1);
>> >> >> > + for ( ; i < hvm_irq->nr_gsis; i++)
>> >> >> > + printk(" %2.2"PRIu8, hvm_irq->gsi_assert_count[i]);
>> >> >> > + printk("\n");
>> >> >> > + }
>> >> >>
>> >> >> I realize you've just copied this from existing code, but what
>> >> >> advantage does %2.2 have over just %2 here?
>> >> >
>> >> > Shouldn't it be just %3 anyway? Would you be fine with me fixing this here, or
>> >> > would you rather prefer a separate patch?
>> >>
>> >> Well, if you also want to change the existing ones, then a separate
>> >> patch would be preferred. As to %2 vs %3 - how would the latter
>> >> be any better if we want/need to change the type from u8 anyway?
>> >
>> > ATM uint8_t can be 255, so %3 seems better (although AFAICT this is limited to
>> > 4, because there are 4 INT# lines to each GSI, and pending GSIs are not
>> > accumulated). I'm not sure I follow why do we want/need to change the type.
>>
>> For our DomU model there are four lines. Physical machines often
>> declare more in ACPI, and I'm not sure there's a theoretical upper
>> limit.
>
> In any case, I'm not sure this is relevant to PVH Dom0, where Xen should simply
> bind GSIs, but has no idea of all the interrupt routing behind them. This is
> relevant to DomUs because in that case Xen acts as a PCI interrupt router
> AFAICT.
Good point. But is all this refcounting code being bypassed for Dom0?
If so, you wouldn't need to extend the array here. If not, overflows
may still matter.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 9:49 ` Jan Beulich
@ 2017-03-28 9:59 ` Roger Pau Monne
2017-03-28 10:33 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-28 9:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Mar 28, 2017 at 03:49:23AM -0600, Jan Beulich wrote:
> >>> On 28.03.17 at 11:34, <roger.pau@citrix.com> wrote:
> > On Tue, Mar 28, 2017 at 03:10:27AM -0600, Jan Beulich wrote:
> >> >>> On 28.03.17 at 10:40, <roger.pau@citrix.com> wrote:
> >> > On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
> >> For our DomU model there are four lines. Physical machines often
> >> declare more in ACPI, and I'm not sure there's a theoretical upper
> >> limit.
> >
> > In any case, I'm not sure this is relevant to PVH Dom0, where Xen should simply
> > bind GSIs, but has no idea of all the interrupt routing behind them. This is
> > relevant to DomUs because in that case Xen acts as a PCI interrupt router
> > AFAICT.
>
> Good point. But is all this refcounting code being bypassed for Dom0?
> If so, you wouldn't need to extend the array here. If not, overflows
> may still matter.
It's not really bypassed for Dom0 (Xen still needs to keep track of pending
interrupts), but it's more simple.
As said above a GSI is either pending (gsi_assert_count[gsi] == 1) or not
(gsi_assert_count[gsi] == 0). I could maybe use a bitmap for that and avoid
touching gsi_assert_count at all, but then I will have to introduce more
changes.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS
2017-03-27 10:18 ` [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS Roger Pau Monne
@ 2017-03-28 10:32 ` Jan Beulich
2017-03-28 11:40 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 10:32 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> Changes since v1:
> - Constify some parameters.
Considering this I'm surprised the first thing I have to ask is ...
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -44,6 +44,54 @@
>
> static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq);
>
> +static struct hvm_vioapic *addr_vioapic(struct domain *d, unsigned long addr)
... const? The more that you have it like this ...
> +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
... here.
> + unsigned int *base_gsi)
All callers of the function really want the pin number rather than
the base GSI - why don't you make the function provide that
instead?
> +{
> + unsigned int i;
> + *base_gsi = 0;
Blank line between declaration(s) and statement(s) please.
> +static unsigned int pin_gsi(const struct domain *d,
> + const struct hvm_vioapic *vioapic,
> + unsigned int pin)
> +{
> + const struct hvm_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;
> +}
Didn't we settle on having a function base_gsi(), with callers
adding in the pin number as needed? The only reason the function
here takes pin as a parameter is this final addition.
Also shouldn't this function somehow guard itself against
overrunning the array?
> @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void)
> return pt_active(¤t->domain->arch.vpit.pt0);
> }
>
> -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
> +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin)
Occasionally we pass around negative IRQ numbers, but if this
really is a pin number, I think the type wants changing to unsigned
int - the more that it is being used as an array index below.
> @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
>
> void vioapic_reset(struct domain *d)
> {
> - struct hvm_vioapic *vioapic = domain_vioapic(d);
> - uint32_t nr_pins = vioapic->nr_pins;
> - int i;
> + unsigned int i;
>
> if ( !has_vioapic(d) )
> + {
> + ASSERT(!d->arch.hvm_domain.nr_vioapics);
> return;
> + }
>
> - memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> - vioapic->domain = d;
> - vioapic->nr_pins = nr_pins;
> - for ( i = 0; i < nr_pins; i++ )
> - vioapic->redirtbl[i].fields.mask = 1;
> - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
> + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
> + {
> + struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> + unsigned int j, nr_pins = vioapic->nr_pins;
> +
> + memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> + for ( j = 0; j < nr_pins; j++ )
> + vioapic->redirtbl[j].fields.mask = 1;
> + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
> + VIOAPIC_MEM_LENGTH * i;
> + vioapic->id = i;
> + vioapic->nr_pins = nr_pins;
> + vioapic->domain = d;
> + }
> +}
Is this function validly reachable for Dom0? If not, better leave it
alone (adding a nr_vioapics == 1 check), as at least the base
address calculation looks rather suspicious (there shouldn't be
multiple IO-APICs on a single page). If so, that same memory
address calculation is actively wrong in the Dom0 case.
> +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics)
> +{
> + unsigned int i;
> +
> + for ( i = 0; i < nr_vioapics; i++)
> + {
> + if ( domain_vioapic(d, i) == NULL )
> + break;
Is this if() really needed?
> int vioapic_init(struct domain *d)
> {
> + unsigned int i, nr_vioapics = 1;
> +
> if ( !has_vioapic(d) )
> + {
> + ASSERT(!d->arch.hvm_domain.nr_vioapics);
> return 0;
> + }
>
> if ( (d->arch.hvm_domain.vioapic == NULL) &&
> ((d->arch.hvm_domain.vioapic =
> - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
> + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
> return -ENOMEM;
>
> - d->arch.hvm_domain.vioapic->domain = d;
> - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
> + for ( i = 0; i < nr_vioapics; i++ )
> + {
> + if ( (domain_vioapic(d, i) =
> + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
> + {
> + vioapic_free(d, nr_vioapics);
> + return -ENOMEM;
> + }
> + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
> + }
> +
> + d->arch.hvm_domain.nr_vioapics = nr_vioapics;
> vioapic_reset(d);
These adjustments again don't look right for nr_vioapics > 1, so I
wonder whether again this function wouldn't better be left alone.
Alternatively, if Dom0 needs to come here, a comment is going to
be needed to explain the (temporary) wrong setting of nr_pins.
> @@ -91,13 +92,22 @@ 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, &base_gsi);
> + if ( !vioapic )
> + {
> + gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", gsi);
> + domain_crash(v->domain);
> + return -1;
> + }
Now that you have this check, ...
> @@ -110,9 +120,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, &base_gsi);
>
> return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) &&
> - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask);
> + vioapic->redirtbl[gsi - base_gsi].fields.mask);
> }
... why is there still none here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 9:59 ` Roger Pau Monne
@ 2017-03-28 10:33 ` Jan Beulich
2017-03-28 10:53 ` Roger Pau Monne
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 10:33 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 28.03.17 at 11:59, <roger.pau@citrix.com> wrote:
> On Tue, Mar 28, 2017 at 03:49:23AM -0600, Jan Beulich wrote:
>> >>> On 28.03.17 at 11:34, <roger.pau@citrix.com> wrote:
>> > On Tue, Mar 28, 2017 at 03:10:27AM -0600, Jan Beulich wrote:
>> >> >>> On 28.03.17 at 10:40, <roger.pau@citrix.com> wrote:
>> >> > On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
>> >> For our DomU model there are four lines. Physical machines often
>> >> declare more in ACPI, and I'm not sure there's a theoretical upper
>> >> limit.
>> >
>> > In any case, I'm not sure this is relevant to PVH Dom0, where Xen should simply
>> > bind GSIs, but has no idea of all the interrupt routing behind them. This is
>> > relevant to DomUs because in that case Xen acts as a PCI interrupt router
>> > AFAICT.
>>
>> Good point. But is all this refcounting code being bypassed for Dom0?
>> If so, you wouldn't need to extend the array here. If not, overflows
>> may still matter.
>
> It's not really bypassed for Dom0 (Xen still needs to keep track of pending
> interrupts), but it's more simple.
>
> As said above a GSI is either pending (gsi_assert_count[gsi] == 1) or not
> (gsi_assert_count[gsi] == 0). I could maybe use a bitmap for that and avoid
> touching gsi_assert_count at all, but then I will have to introduce more
> changes.
I don't think there's a need, indeed. But I'd appreciate if you
would add ASSERT()s to that effect.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array
2017-03-28 10:33 ` Jan Beulich
@ 2017-03-28 10:53 ` Roger Pau Monne
0 siblings, 0 replies; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-28 10:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Mar 28, 2017 at 04:33:49AM -0600, Jan Beulich wrote:
> >>> On 28.03.17 at 11:59, <roger.pau@citrix.com> wrote:
> > On Tue, Mar 28, 2017 at 03:49:23AM -0600, Jan Beulich wrote:
> >> >>> On 28.03.17 at 11:34, <roger.pau@citrix.com> wrote:
> >> > On Tue, Mar 28, 2017 at 03:10:27AM -0600, Jan Beulich wrote:
> >> >> >>> On 28.03.17 at 10:40, <roger.pau@citrix.com> wrote:
> >> >> > On Tue, Mar 28, 2017 at 01:25:44AM -0600, Jan Beulich wrote:
> >> >> For our DomU model there are four lines. Physical machines often
> >> >> declare more in ACPI, and I'm not sure there's a theoretical upper
> >> >> limit.
> >> >
> >> > In any case, I'm not sure this is relevant to PVH Dom0, where Xen should simply
> >> > bind GSIs, but has no idea of all the interrupt routing behind them. This is
> >> > relevant to DomUs because in that case Xen acts as a PCI interrupt router
> >> > AFAICT.
> >>
> >> Good point. But is all this refcounting code being bypassed for Dom0?
> >> If so, you wouldn't need to extend the array here. If not, overflows
> >> may still matter.
> >
> > It's not really bypassed for Dom0 (Xen still needs to keep track of pending
> > interrupts), but it's more simple.
> >
> > As said above a GSI is either pending (gsi_assert_count[gsi] == 1) or not
> > (gsi_assert_count[gsi] == 0). I could maybe use a bitmap for that and avoid
> > touching gsi_assert_count at all, but then I will have to introduce more
> > changes.
>
> I don't think there's a need, indeed. But I'd appreciate if you
> would add ASSERT()s to that effect.
OK, such ASSERTs would be added to hvm_gsi_{de}assert in patch #3 of the next
series ("x86/dpci: bind legacy PCI interrupts to PVHv2 Dom0").
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 6/7] x86/ioapic: add prototype for apic_gsi_base to io_apic.h
2017-03-27 10:18 ` [PATCH v2 6/7] x86/ioapic: add prototype for apic_gsi_base to io_apic.h Roger Pau Monne
@ 2017-03-28 11:35 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 11:35 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
albeit perhaps ...
> Changes since v1:
> - Add io_ prefix to avoid confusion.
... this should be carried through to the title.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS
2017-03-28 10:32 ` Jan Beulich
@ 2017-03-28 11:40 ` Roger Pau Monne
2017-03-28 11:51 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Roger Pau Monne @ 2017-03-28 11:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Tue, Mar 28, 2017 at 04:32:05AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> > Changes since v1:
> > - Constify some parameters.
>
> Considering this I'm surprised the first thing I have to ask is ...
>
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -44,6 +44,54 @@
> >
> > static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq);
> >
> > +static struct hvm_vioapic *addr_vioapic(struct domain *d, unsigned long addr)
>
> ... const? The more that you have it like this ...
Shame, I guess I missed this one, sorry.
> > +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
>
> ... here.
>
> > + unsigned int *base_gsi)
>
> All callers of the function really want the pin number rather than
> the base GSI - why don't you make the function provide that
> instead?
Done.
> > +static unsigned int pin_gsi(const struct domain *d,
> > + const struct hvm_vioapic *vioapic,
> > + unsigned int pin)
> > +{
> > + const struct hvm_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;
> > +}
>
> Didn't we settle on having a function base_gsi(), with callers
> adding in the pin number as needed? The only reason the function
> here takes pin as a parameter is this final addition.
>
> Also shouldn't this function somehow guard itself against
> overrunning the array?
Done for both.
> > @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void)
> > return pt_active(¤t->domain->arch.vpit.pt0);
> > }
> >
> > -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
> > +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin)
>
> Occasionally we pass around negative IRQ numbers, but if this
> really is a pin number, I think the type wants changing to unsigned
> int - the more that it is being used as an array index below.
Fixed.
> > @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
> >
> > void vioapic_reset(struct domain *d)
> > {
> > - struct hvm_vioapic *vioapic = domain_vioapic(d);
> > - uint32_t nr_pins = vioapic->nr_pins;
> > - int i;
> > + unsigned int i;
> >
> > if ( !has_vioapic(d) )
> > + {
> > + ASSERT(!d->arch.hvm_domain.nr_vioapics);
> > return;
> > + }
> >
> > - memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> > - vioapic->domain = d;
> > - vioapic->nr_pins = nr_pins;
> > - for ( i = 0; i < nr_pins; i++ )
> > - vioapic->redirtbl[i].fields.mask = 1;
> > - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
> > + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
> > + {
> > + struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> > + unsigned int j, nr_pins = vioapic->nr_pins;
> > +
> > + memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> > + for ( j = 0; j < nr_pins; j++ )
> > + vioapic->redirtbl[j].fields.mask = 1;
> > + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
> > + VIOAPIC_MEM_LENGTH * i;
> > + vioapic->id = i;
> > + vioapic->nr_pins = nr_pins;
> > + vioapic->domain = d;
> > + }
> > +}
>
> Is this function validly reachable for Dom0? If not, better leave it
> alone (adding a nr_vioapics == 1 check), as at least the base
> address calculation looks rather suspicious (there shouldn't be
> multiple IO-APICs on a single page). If so, that same memory
> address calculation is actively wrong in the Dom0 case.
Yes, this is called from vioapic_init in order to set the initial vIO APIC
state and mask all the redir entries. For the Dom0 case we use what's found on
the native MADT tables.
For now I could fix that by adding:
BUILD_BUG_ON(VIOAPIC_MEM_LENGTH > PAGE_SIZE);
And then use PAGE_SIZE above instead of VIOAPIC_MEM_LENGTH.
> > +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics)
> > +{
> > + unsigned int i;
> > +
> > + for ( i = 0; i < nr_vioapics; i++)
> > + {
> > + if ( domain_vioapic(d, i) == NULL )
> > + break;
>
> Is this if() really needed?
Not at all, thanks for pointing it out.
> > int vioapic_init(struct domain *d)
> > {
> > + unsigned int i, nr_vioapics = 1;
> > +
> > if ( !has_vioapic(d) )
> > + {
> > + ASSERT(!d->arch.hvm_domain.nr_vioapics);
> > return 0;
> > + }
> >
> > if ( (d->arch.hvm_domain.vioapic == NULL) &&
> > ((d->arch.hvm_domain.vioapic =
> > - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
> > + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
> > return -ENOMEM;
> >
> > - d->arch.hvm_domain.vioapic->domain = d;
> > - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
> > + for ( i = 0; i < nr_vioapics; i++ )
> > + {
> > + if ( (domain_vioapic(d, i) =
> > + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
> > + {
> > + vioapic_free(d, nr_vioapics);
> > + return -ENOMEM;
> > + }
> > + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
> > + }
> > +
> > + d->arch.hvm_domain.nr_vioapics = nr_vioapics;
> > vioapic_reset(d);
>
> These adjustments again don't look right for nr_vioapics > 1, so I
> wonder whether again this function wouldn't better be left alone.
> Alternatively, if Dom0 needs to come here, a comment is going to
> be needed to explain the (temporary) wrong setting of nr_pins.
Yes, Dom0 will need to come here. If it's cleaner I could have two different
init functions, one for DomU and one for Dom0, and call them from vioapic_init.
Or add something like:
ASSERT(is_hardware_domain(d) || nr_vioapics == 1);
(the same could be added to vioapic_reset).
> > @@ -91,13 +92,22 @@ 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, &base_gsi);
> > + if ( !vioapic )
> > + {
> > + gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", gsi);
> > + domain_crash(v->domain);
> > + return -1;
> > + }
>
> Now that you have this check, ...
>
> > @@ -110,9 +120,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, &base_gsi);
> >
> > return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) &&
> > - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask);
> > + vioapic->redirtbl[gsi - base_gsi].fields.mask);
> > }
>
> ... why is there still none here?
Fixed.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 7/7] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC
2017-03-27 10:18 ` [PATCH v2 7/7] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC Roger Pau Monne
@ 2017-03-28 11:48 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 11:48 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
> 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with ...
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -533,9 +533,19 @@ void vioapic_reset(struct domain *d)
> memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> for ( j = 0; j < 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;
> + }
... this suitably adjusted according to the earlier requested change,
avoiding to leave bogus address calculations here. One option would
be to remove the dependency on i from the calculation, adding
ASSERT(!i).
> @@ -571,15 +583,21 @@ int vioapic_init(struct domain *d)
>
> for ( i = 0; i < nr_vioapics; i++ )
> {
> + unsigned int nr_pins = is_hardware_domain(d) ? nr_ioapic_entries[i]
> + : VIOAPIC_NUM_PINS;
> +
> if ( (domain_vioapic(d, i) =
> - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
> + xmalloc_bytes(hvm_vioapic_size(nr_pins))) == NULL )
> {
> vioapic_free(d, nr_vioapics);
> return -ENOMEM;
> }
> - domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
> + domain_vioapic(d, i)->nr_pins = nr_pins;
> + nr_gsis += nr_pins;
> }
Hmm, seeing this I can accept how patch 5 altered this function.
As per above this doesn't extend to ioapic_reset() though.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS
2017-03-28 11:40 ` Roger Pau Monne
@ 2017-03-28 11:51 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-03-28 11:51 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
>>> On 28.03.17 at 13:40, <roger.pau@citrix.com> wrote:
> On Tue, Mar 28, 2017 at 04:32:05AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote:
>> > @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM);
>> >
>> > void vioapic_reset(struct domain *d)
>> > {
>> > - struct hvm_vioapic *vioapic = domain_vioapic(d);
>> > - uint32_t nr_pins = vioapic->nr_pins;
>> > - int i;
>> > + unsigned int i;
>> >
>> > if ( !has_vioapic(d) )
>> > + {
>> > + ASSERT(!d->arch.hvm_domain.nr_vioapics);
>> > return;
>> > + }
>> >
>> > - memset(vioapic, 0, hvm_vioapic_size(nr_pins));
>> > - vioapic->domain = d;
>> > - vioapic->nr_pins = nr_pins;
>> > - for ( i = 0; i < nr_pins; i++ )
>> > - vioapic->redirtbl[i].fields.mask = 1;
>> > - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
>> > + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
>> > + {
>> > + struct hvm_vioapic *vioapic = domain_vioapic(d, i);
>> > + unsigned int j, nr_pins = vioapic->nr_pins;
>> > +
>> > + memset(vioapic, 0, hvm_vioapic_size(nr_pins));
>> > + for ( j = 0; j < nr_pins; j++ )
>> > + vioapic->redirtbl[j].fields.mask = 1;
>> > + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
>> > + VIOAPIC_MEM_LENGTH * i;
>> > + vioapic->id = i;
>> > + vioapic->nr_pins = nr_pins;
>> > + vioapic->domain = d;
>> > + }
>> > +}
>>
>> Is this function validly reachable for Dom0? If not, better leave it
>> alone (adding a nr_vioapics == 1 check), as at least the base
>> address calculation looks rather suspicious (there shouldn't be
>> multiple IO-APICs on a single page). If so, that same memory
>> address calculation is actively wrong in the Dom0 case.
>
> Yes, this is called from vioapic_init in order to set the initial vIO APIC
> state and mask all the redir entries. For the Dom0 case we use what's found on
> the native MADT tables.
>
> For now I could fix that by adding:
>
> BUILD_BUG_ON(VIOAPIC_MEM_LENGTH > PAGE_SIZE);
>
> And then use PAGE_SIZE above instead of VIOAPIC_MEM_LENGTH.
Hmm - see my reply to patch 7.
>> > int vioapic_init(struct domain *d)
>> > {
>> > + unsigned int i, nr_vioapics = 1;
>> > +
>> > if ( !has_vioapic(d) )
>> > + {
>> > + ASSERT(!d->arch.hvm_domain.nr_vioapics);
>> > return 0;
>> > + }
>> >
>> > if ( (d->arch.hvm_domain.vioapic == NULL) &&
>> > ((d->arch.hvm_domain.vioapic =
>> > - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
>> > + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
>> > return -ENOMEM;
>> >
>> > - d->arch.hvm_domain.vioapic->domain = d;
>> > - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
>> > + for ( i = 0; i < nr_vioapics; i++ )
>> > + {
>> > + if ( (domain_vioapic(d, i) =
>> > + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
>> > + {
>> > + vioapic_free(d, nr_vioapics);
>> > + return -ENOMEM;
>> > + }
>> > + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
>> > + }
>> > +
>> > + d->arch.hvm_domain.nr_vioapics = nr_vioapics;
>> > vioapic_reset(d);
>>
>> These adjustments again don't look right for nr_vioapics > 1, so I
>> wonder whether again this function wouldn't better be left alone.
>> Alternatively, if Dom0 needs to come here, a comment is going to
>> be needed to explain the (temporary) wrong setting of nr_pins.
>
> Yes, Dom0 will need to come here. If it's cleaner I could have two different
> init functions, one for DomU and one for Dom0, and call them from
> vioapic_init.
>
> Or add something like:
>
> ASSERT(is_hardware_domain(d) || nr_vioapics == 1);
>
> (the same could be added to vioapic_reset).
Which is basically in line with what I've said in reply to patch 7.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-03-28 11:52 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-27 10:18 [PATCH v2 0/7] x86/vioapic: introduce support for multiple vIO APICs Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure Roger Pau Monne
2017-03-27 15:38 ` Jan Beulich
2017-03-27 16:49 ` Roger Pau Monne
2017-03-28 7:17 ` Jan Beulich
2017-03-28 8:15 ` Roger Pau Monne
2017-03-28 8:58 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 2/7] x86/hvm: introduce hvm_domain_irq macro Roger Pau Monne
2017-03-27 15:41 ` Jan Beulich
2017-03-27 15:55 ` Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 3/7] x86/hvm: convert gsi_assert_count into a variable size array Roger Pau Monne
2017-03-27 15:59 ` Jan Beulich
2017-03-27 17:28 ` Roger Pau Monne
2017-03-28 7:25 ` Jan Beulich
2017-03-28 8:40 ` Roger Pau Monne
2017-03-28 9:10 ` Jan Beulich
2017-03-28 9:34 ` Roger Pau Monne
2017-03-28 9:49 ` Jan Beulich
2017-03-28 9:59 ` Roger Pau Monne
2017-03-28 10:33 ` Jan Beulich
2017-03-28 10:53 ` Roger Pau Monne
2017-03-27 10:18 ` [PATCH v2 4/7] x86/vioapic: allow the vIO APIC to have a variable number of pins Roger Pau Monne
2017-03-27 16:10 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 5/7] x86/vioapic: introduce support for multiple vIO APICS Roger Pau Monne
2017-03-28 10:32 ` Jan Beulich
2017-03-28 11:40 ` Roger Pau Monne
2017-03-28 11:51 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 6/7] x86/ioapic: add prototype for apic_gsi_base to io_apic.h Roger Pau Monne
2017-03-28 11:35 ` Jan Beulich
2017-03-27 10:18 ` [PATCH v2 7/7] x86/vioapic: allow PVHv2 Dom0 to have more than one IO APIC Roger Pau Monne
2017-03-28 11:48 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.