Xen devel - Oct 2012 - [PATCH 0/3] x86: HPET adjustments

1: obtain proper lock for changing IRQ affinity
2: allow use for broadcast when interrupt remapping is in effect
3: cache MSI message last written

Signed-off-by: Jan Beulich <jbeulich@suse.com>

The IRQ descriptor lock should be held while adjusting the affinity of
any IRQ; the HPET channel lock isn''t sufficient to protect namely
against races with moving the IRQ to a different CPU.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -436,6 +436,16 @@ static struct hpet_event_channel *hpet_g
     return ch;
 }
 
+static void _hpet_msi_set_affinity(const struct hpet_event_channel *ch)
+{
+    struct irq_desc *desc = irq_to_desc(ch->irq);
+
+    ASSERT(!local_irq_is_enabled());
+    spin_lock(&desc->lock);
+    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
+    spin_unlock(&desc->lock);
+}
+
 static void hpet_attach_channel(unsigned int cpu,
                                 struct hpet_event_channel *ch)
 {
@@ -450,7 +460,7 @@ static void hpet_attach_channel(unsigned
     if ( ch->cpu != cpu )
         return;
 
-    hpet_msi_set_affinity(irq_to_desc(ch->irq), cpumask_of(ch->cpu));
+    _hpet_msi_set_affinity(ch);
 }
 
 static void hpet_detach_channel(unsigned int cpu,
@@ -472,7 +482,7 @@ static void hpet_detach_channel(unsigned
     }
 
     ch->cpu = cpumask_first(ch->cpumask);
-    hpet_msi_set_affinity(irq_to_desc(ch->irq), cpumask_of(ch->cpu));
+    _hpet_msi_set_affinity(ch);
 }
 
 #include <asm/mc146818rtc.h>





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

This requires some additions to the VT-d side; AMD IOMMUs use the
"normal" MSI message format even when interrupt remapping is enabled,
thus making adjustments here unnecessary.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -276,6 +276,7 @@ static int __init acpi_parse_hpet(struct
 	}
 
 	hpet_address = hpet_tbl->address.address;
+	hpet_blockid = hpet_tbl->sequence;
 	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
 	       hpet_tbl->id, hpet_address);
 
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -40,7 +40,7 @@ struct hpet_event_channel
 
     unsigned int idx;   /* physical channel idx */
     unsigned int cpu;   /* msi target */
-    int irq;            /* msi irq */
+    struct msi_desc msi;/* msi state */
     unsigned int flags; /* HPET_EVT_x */
 } __cacheline_aligned;
 static struct hpet_event_channel *__read_mostly hpet_events;
@@ -51,6 +51,7 @@ static unsigned int __read_mostly num_hp
 DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
 
 unsigned long __read_mostly hpet_address;
+u8 __initdata hpet_blockid;
 
 /*
  * force_hpet_broadcast: by default legacy hpet broadcast will be stopped
@@ -252,6 +253,8 @@ static void hpet_msi_mask(struct irq_des
 
 static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
+    if ( iommu_intremap )
+        iommu_update_ire_from_msi(&ch->msi, msg);
     hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
     hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
 }
@@ -261,6 +264,8 @@ static void hpet_msi_read(struct hpet_ev
     msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
     msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
     msg->address_hi = 0;
+    if ( iommu_intremap )
+        iommu_read_msi_from_ire(&ch->msi, msg);
 }
 
 static unsigned int hpet_msi_startup(struct irq_desc *desc)
@@ -292,6 +297,7 @@ static void hpet_msi_set_affinity(struct
     msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
     msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
     msg.address_lo |= MSI_ADDR_DEST_ID(dest);
+    msg.dest32 = dest;
     hpet_msi_write(desc->action->dev_id, &msg);
 }
 
@@ -316,35 +322,48 @@ static void __hpet_setup_msi_irq(struct 
     hpet_msi_write(desc->action->dev_id, &msg);
 }
 
-static int __init hpet_setup_msi_irq(unsigned int irq, struct
hpet_event_channel *ch)
+static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
 {
     int ret;
-    irq_desc_t *desc = irq_to_desc(irq);
+    irq_desc_t *desc = irq_to_desc(ch->msi.irq);
+
+    if ( iommu_intremap )
+    {
+        ch->msi.hpet_id = hpet_blockid;
+        ret = iommu_setup_hpet_msi(&ch->msi);
+        if ( ret )
+            return ret;
+    }
 
     desc->handler = &hpet_msi_type;
-    ret = request_irq(irq, hpet_interrupt_handler, 0, "HPET", ch);
+    ret = request_irq(ch->msi.irq, hpet_interrupt_handler, 0,
"HPET", ch);
     if ( ret < 0 )
+    {
+        if ( iommu_intremap )
+            iommu_update_ire_from_msi(&ch->msi, NULL);
         return ret;
+    }
 
     __hpet_setup_msi_irq(desc);
 
     return 0;
 }
 
-static int __init hpet_assign_irq(unsigned int idx)
+static int __init hpet_assign_irq(struct hpet_event_channel *ch)
 {
     int irq;
 
     if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
         return irq;
 
-    if ( hpet_setup_msi_irq(irq, hpet_events + idx) )
+    ch->msi.irq = irq;
+    if ( hpet_setup_msi_irq(ch) )
     {
         destroy_irq(irq);
         return -EINVAL;
     }
 
-    return irq;
+    return 0;
 }
 
 static void __init hpet_fsb_cap_lookup(void)
@@ -352,14 +371,6 @@ static void __init hpet_fsb_cap_lookup(v
     u32 id;
     unsigned int i, num_chs;
 
-    /* TODO. */
-    if ( iommu_intremap )
-    {
-        printk(XENLOG_INFO "HPET''s MSI mode hasn''t been
supported when "
-            "Interrupt Remapping is enabled.\n");
-        return;
-    }
-
     id = hpet_read32(HPET_ID);
 
     num_chs = ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT);
@@ -391,8 +402,8 @@ static void __init hpet_fsb_cap_lookup(v
         ch->flags = 0;
         ch->idx = i;
 
-        if ( (ch->irq = hpet_assign_irq(num_hpets_used++)) < 0 )
-            num_hpets_used--;
+        if ( hpet_assign_irq(ch) == 0 )
+            num_hpets_used++;
     }
 
     printk(XENLOG_INFO "HPET: %u timers (%u will be used for
broadcast)\n",
@@ -438,7 +449,7 @@ static struct hpet_event_channel *hpet_g
 
 static void _hpet_msi_set_affinity(const struct hpet_event_channel *ch)
 {
-    struct irq_desc *desc = irq_to_desc(ch->irq);
+    struct irq_desc *desc = irq_to_desc(ch->msi.irq);
 
     ASSERT(!local_irq_is_enabled());
     spin_lock(&desc->lock);
@@ -530,7 +541,7 @@ void __init hpet_broadcast_init(void)
             hpet_events = xzalloc(struct hpet_event_channel);
         if ( !hpet_events || !zalloc_cpumask_var(&hpet_events->cpumask)
)
             return;
-        hpet_events->irq = -1;
+        hpet_events->msi.irq = -1;
 
         /* Start HPET legacy interrupts */
         cfg |= HPET_CFG_LEGACY;
@@ -598,8 +609,8 @@ void hpet_broadcast_resume(void)
 
     for ( i = 0; i < n; i++ )
     {
-        if ( hpet_events[i].irq >= 0 )
-            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].irq));
+        if ( hpet_events[i].msi.irq >= 0 )
+            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
 
         /* set HPET Tn as oneshot */
         cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -495,6 +495,12 @@ unsigned int iommu_read_apic_from_ire(un
     return ops->read_apic_from_ire(apic, reg);
 }
 
+int __init iommu_setup_hpet_msi(struct msi_desc *msi)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : 0;
+}
+
 void iommu_resume()
 {
     const struct iommu_ops *ops = iommu_get_ops();
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -147,6 +147,34 @@ struct iommu * ioapic_to_iommu(unsigned 
     return NULL;
 }
 
+static bool_t acpi_hpet_device_match(
+    struct list_head *list, unsigned int hpet_id)
+{
+    struct acpi_hpet_unit *hpet;
+
+    list_for_each_entry( hpet, list, list )
+        if (hpet->id == hpet_id)
+            return 1;
+    return 0;
+}
+
+struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id)
+{
+    struct acpi_drhd_unit *drhd;
+
+    list_for_each_entry( drhd, &acpi_drhd_units, list )
+        if ( acpi_hpet_device_match(&drhd->hpet_list, hpet_id) )
+            return drhd;
+    return NULL;
+}
+
+struct iommu *hpet_to_iommu(unsigned int hpet_id)
+{
+    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
+
+    return drhd ? drhd->iommu : NULL;
+}
+
 static int __init acpi_register_atsr_unit(struct acpi_atsr_unit *atsr)
 {
     /*
@@ -330,6 +358,22 @@ static int __init acpi_parse_dev_scope(
             if ( iommu_verbose )
                 dprintk(VTDPREFIX, " MSI HPET: %04x:%02x:%02x.%u\n",
                         seg, bus, path->dev, path->fn);
+
+            if ( type == DMAR_TYPE )
+            {
+                struct acpi_drhd_unit *drhd = acpi_entry;
+                struct acpi_hpet_unit *acpi_hpet_unit;
+
+                acpi_hpet_unit = xmalloc(struct acpi_hpet_unit);
+                if ( !acpi_hpet_unit )
+                    return -ENOMEM;
+                acpi_hpet_unit->id = acpi_scope->enumeration_id;
+                acpi_hpet_unit->bus = bus;
+                acpi_hpet_unit->dev = path->dev;
+                acpi_hpet_unit->func = path->fn;
+                list_add(&acpi_hpet_unit->list,
&drhd->hpet_list);
+            }
+
             break;
 
         case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
@@ -407,6 +451,7 @@ acpi_parse_one_drhd(struct acpi_dmar_hea
     dmaru->segment = drhd->segment;
     dmaru->include_all = drhd->flags & ACPI_DMAR_INCLUDE_ALL;
     INIT_LIST_HEAD(&dmaru->ioapic_list);
+    INIT_LIST_HEAD(&dmaru->hpet_list);
     if ( iommu_verbose )
         dprintk(VTDPREFIX, "  dmaru->address =
%"PRIx64"\n",
                 dmaru->address);
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -39,6 +39,19 @@ struct acpi_ioapic_unit {
     }ioapic;
 };
 
+struct acpi_hpet_unit {
+    struct list_head list;
+    unsigned int id;
+    union {
+        u16 bdf;
+        struct {
+            u16 func: 3,
+                dev:  5,
+                bus:  8;
+        };
+    };
+};
+
 struct dmar_scope {
     DECLARE_BITMAP(buses, 256);         /* buses owned by this unit */
     u16    *devices;                    /* devices owned by this unit */
@@ -53,6 +66,7 @@ struct acpi_drhd_unit {
     u8     include_all:1;
     struct iommu *iommu;
     struct list_head ioapic_list;
+    struct list_head hpet_list;
 };
 
 struct acpi_rmrr_unit {
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -54,7 +54,9 @@ int iommu_flush_iec_index(struct iommu *
 void clear_fault_bits(struct iommu *iommu);
 
 struct iommu * ioapic_to_iommu(unsigned int apic_id);
+struct iommu * hpet_to_iommu(unsigned int hpet_id);
 struct acpi_drhd_unit * ioapic_to_drhd(unsigned int apic_id);
+struct acpi_drhd_unit * hpet_to_drhd(unsigned int hpet_id);
 struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu);
 struct acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
 
@@ -90,6 +92,8 @@ struct msi_msg;
 void msi_msg_read_remap_rte(struct msi_desc *, struct msi_msg *);
 void msi_msg_write_remap_rte(struct msi_desc *, struct msi_msg *);
 
+int intel_setup_hpet_msi(struct msi_desc *);
+
 int is_igd_vt_enabled_quirk(void);
 void platform_quirks_init(void);
 void vtd_ops_preamble_quirk(struct iommu* iommu);
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -107,6 +107,19 @@ static u16 apicid_to_bdf(int apic_id)
     return 0;
 }
 
+static u16 hpetid_to_bdf(unsigned int hpet_id)
+{
+    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
+    struct acpi_hpet_unit *acpi_hpet_unit;
+
+    list_for_each_entry ( acpi_hpet_unit, &drhd->hpet_list, list )
+        if ( acpi_hpet_unit->id == hpet_id )
+            return acpi_hpet_unit->bdf;
+
+    dprintk(XENLOG_ERR VTDPREFIX, "Didn''t find the bdf for HPET
%u!\n", hpet_id);
+    return 0;
+}
+
 static void set_ire_sid(struct iremap_entry *ire,
                         unsigned int svt, unsigned int sq, unsigned int sid)
 {
@@ -121,6 +134,16 @@ static void set_ioapic_source_id(int api
                 apicid_to_bdf(apic_id));
 }
 
+static void set_hpet_source_id(unsigned int id, struct iremap_entry *ire)
+{
+    /*
+     * Should really use SQ_ALL_16. Some platforms are broken.
+     * While we figure out the right quirks for these broken platforms, use
+     * SQ_13_IGNORE_3 for now.
+     */
+    set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
+}
+
 int iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
@@ -592,7 +615,10 @@ static int msi_msg_to_remap_entry(
         new_ire.lo.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT)
                           & 0xff) << 8;
 
-    set_msi_source_id(pdev, &new_ire);
+    if ( pdev )
+        set_msi_source_id(pdev, &new_ire);
+    else
+        set_hpet_source_id(msi_desc->hpet_id, &new_ire);
     new_ire.hi.res_1 = 0;
     new_ire.lo.p = 1;    /* finally, set present bit */
 
@@ -624,7 +650,9 @@ void msi_msg_read_remap_rte(
     struct iommu *iommu = NULL;
     struct ir_ctrl *ir_ctrl;
 
-    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
+    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
+                : hpet_to_drhd(msi_desc->hpet_id);
+    if ( !drhd )
         return;
     iommu = drhd->iommu;
 
@@ -643,7 +671,9 @@ void msi_msg_write_remap_rte(
     struct iommu *iommu = NULL;
     struct ir_ctrl *ir_ctrl;
 
-    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
+    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
+                : hpet_to_drhd(msi_desc->hpet_id);
+    if ( !drhd )
         return;
     iommu = drhd->iommu;
 
@@ -654,6 +684,32 @@ void msi_msg_write_remap_rte(
     msi_msg_to_remap_entry(iommu, pdev, msi_desc, msg);
 }
 
+int __init intel_setup_hpet_msi(struct msi_desc *msi_desc)
+{
+    struct iommu *iommu = hpet_to_iommu(msi_desc->hpet_id);
+    struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
+    unsigned long flags;
+    int rc = 0;
+
+    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
+        return 0;
+
+    spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
+    msi_desc->remap_index = alloc_remap_entry(iommu);
+    if ( msi_desc->remap_index >= IREMAP_ENTRY_NR )
+    {
+        dprintk(XENLOG_ERR VTDPREFIX,
+                "%s: intremap index (%d) is larger than"
+                " the maximum index (%d)!\n",
+                __func__, msi_desc->remap_index, IREMAP_ENTRY_NR - 1);
+        msi_desc->remap_index = -1;
+        rc = -ENXIO;
+    }
+    spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
+
+    return rc;
+}
+
 int enable_intremap(struct iommu *iommu, int eim)
 {
     struct acpi_drhd_unit *drhd;
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2396,6 +2396,7 @@ const struct iommu_ops intel_iommu_ops     
.update_ire_from_msi = msi_msg_write_remap_rte,
     .read_apic_from_ire = io_apic_read_remap_rte,
     .read_msi_from_ire = msi_msg_read_remap_rte,
+    .setup_hpet_msi = intel_setup_hpet_msi,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
     .share_p2m = iommu_set_pgd,
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -53,6 +53,7 @@
     (*(volatile u32 *)(fix_to_virt(FIX_HPET_BASE) + (x)) = (y))
 
 extern unsigned long hpet_address;
+extern u8 hpet_blockid;
 
 /*
  * Detect and initialise HPET hardware: return counter update frequency.
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -97,7 +97,10 @@ struct msi_desc {
 
 	struct list_head list;
 
-	void __iomem *mask_base;        /* va for the entry in mask table */
+	union {
+		void __iomem *mask_base;/* va for the entry in mask table */
+		unsigned int hpet_id;   /* HPET (dev is NULL)             */
+	};
 	struct pci_dev *dev;
 	int irq;
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -109,6 +109,7 @@ struct iommu_ops {
     void (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg
*msg);
     void (*read_msi_from_ire)(struct msi_desc *msi_desc, struct msi_msg *msg);
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
+    int (*setup_hpet_msi)(struct msi_desc *);
     void (*suspend)(void);
     void (*resume)(void);
     void (*share_p2m)(struct domain *d);
@@ -122,6 +123,7 @@ void iommu_update_ire_from_apic(unsigned
 void iommu_update_ire_from_msi(struct msi_desc *msi_desc, struct msi_msg *msg);
 void iommu_read_msi_from_ire(struct msi_desc *msi_desc, struct msi_msg *msg);
 unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
+int iommu_setup_hpet_msi(struct msi_desc *);
 
 void iommu_suspend(void);
 void iommu_resume(void);




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

Rather than spending measurable amounts of time reading back the most
recently written message, cache it in space previously unused, and thus
accelerate the CPU''s entering of the intended C-state.

hpet_msi_read() ends up being unused after this change, but rather than
removing the function, it''s being marked "unused" in order -
that way
it can easily get used again should a new need for it arise.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -253,17 +253,19 @@ static void hpet_msi_mask(struct irq_des
 
 static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
+    ch->msi.msg = *msg;
     if ( iommu_intremap )
         iommu_update_ire_from_msi(&ch->msi, msg);
     hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
     hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
 }
 
-static void hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
+static void __attribute__((__unused__))
+hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
     msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
     msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
-    msg->address_hi = 0;
+    msg->address_hi = MSI_ADDR_BASE_HI;
     if ( iommu_intremap )
         iommu_read_msi_from_ire(&ch->msi, msg);
 }
@@ -285,20 +287,19 @@ static void hpet_msi_ack(struct irq_desc
 
 static void hpet_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
 {
-    struct msi_msg msg;
-    unsigned int dest;
+    struct hpet_event_channel *ch = desc->action->dev_id;
+    struct msi_msg msg = ch->msi.msg;
 
-    dest = set_desc_affinity(desc, mask);
-    if (dest == BAD_APICID)
+    msg.dest32 = set_desc_affinity(desc, mask);
+    if ( msg.dest32 == BAD_APICID )
         return;
 
-    hpet_msi_read(desc->action->dev_id, &msg);
     msg.data &= ~MSI_DATA_VECTOR_MASK;
     msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
     msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
-    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
-    msg.dest32 = dest;
-    hpet_msi_write(desc->action->dev_id, &msg);
+    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
+    if ( msg.data != ch->msi.msg.data || msg.dest32 != ch->msi.msg.dest32
)
+        hpet_msi_write(ch, &msg);
 }
 
 /*





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

On 16/10/2012 16:09, "Jan Beulich" <JBeulich@suse.com> wrote:
> The IRQ descriptor lock should be held while adjusting the affinity of
> any IRQ; the HPET channel lock isn''t sufficient to protect namely
> against races with moving the IRQ to a different CPU.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Would be more usual for the underscore-prefixed name to be the one that
doesn''t take locks?

 -- Keir
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -436,6 +436,16 @@ static struct hpet_event_channel *hpet_g
>      return ch;
>  }
>  
> +static void _hpet_msi_set_affinity(const struct hpet_event_channel *ch)
> +{
> +    struct irq_desc *desc = irq_to_desc(ch->irq);
> +
> +    ASSERT(!local_irq_is_enabled());
> +    spin_lock(&desc->lock);
> +    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
> +    spin_unlock(&desc->lock);
> +}
> +
>  static void hpet_attach_channel(unsigned int cpu,
>                                  struct hpet_event_channel *ch)
>  {
> @@ -450,7 +460,7 @@ static void hpet_attach_channel(unsigned
>      if ( ch->cpu != cpu )
>          return;
>  
> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
> +    _hpet_msi_set_affinity(ch);
>  }
>  
>  static void hpet_detach_channel(unsigned int cpu,
> @@ -472,7 +482,7 @@ static void hpet_detach_channel(unsigned
>      }
>  
>      ch->cpu = cpumask_first(ch->cpumask);
> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
> +    _hpet_msi_set_affinity(ch);
>  }
>  
>  #include <asm/mc146818rtc.h>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 16.10.12 at 17:41, Keir Fraser <keir.xen@gmail.com> wrote:
> On 16/10/2012 16:09, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> The IRQ descriptor lock should be held while adjusting the affinity of
>> any IRQ; the HPET channel lock isn''t sufficient to protect
namely
>> against races with moving the IRQ to a different CPU.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Would be more usual for the underscore-prefixed name to be the one that
> doesn''t take locks?
Will make the patch bigger, but I can certainly do that. I specifically
picked only a single underscore to distinguish this a little from the
"usual" case.

Jan
>> --- a/xen/arch/x86/hpet.c
>> +++ b/xen/arch/x86/hpet.c
>> @@ -436,6 +436,16 @@ static struct hpet_event_channel *hpet_g
>>      return ch;
>>  }
>>  
>> +static void _hpet_msi_set_affinity(const struct hpet_event_channel
*ch)
>> +{
>> +    struct irq_desc *desc = irq_to_desc(ch->irq);
>> +
>> +    ASSERT(!local_irq_is_enabled());
>> +    spin_lock(&desc->lock);
>> +    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
>> +    spin_unlock(&desc->lock);
>> +}
>> +
>>  static void hpet_attach_channel(unsigned int cpu,
>>                                  struct hpet_event_channel *ch)
>>  {
>> @@ -450,7 +460,7 @@ static void hpet_attach_channel(unsigned
>>      if ( ch->cpu != cpu )
>>          return;
>>  
>> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
>> +    _hpet_msi_set_affinity(ch);
>>  }
>>  
>>  static void hpet_detach_channel(unsigned int cpu,
>> @@ -472,7 +482,7 @@ static void hpet_detach_channel(unsigned
>>      }
>>  
>>      ch->cpu = cpumask_first(ch->cpumask);
>> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
>> +    _hpet_msi_set_affinity(ch);
>>  }
>>  
>>  #include <asm/mc146818rtc.h>
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org 
>> http://lists.xen.org/xen-devel

On 16/10/2012 17:11, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 16.10.12 at 17:41, Keir Fraser <keir.xen@gmail.com>
wrote:
>> On 16/10/2012 16:09, "Jan Beulich" <JBeulich@suse.com>
wrote:
>> 
>>> The IRQ descriptor lock should be held while adjusting the affinity
of
>>> any IRQ; the HPET channel lock isn''t sufficient to protect
namely
>>> against races with moving the IRQ to a different CPU.
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Would be more usual for the underscore-prefixed name to be the one that
>> doesn''t take locks?
> 
> Will make the patch bigger, but I can certainly do that. I specifically
> picked only a single underscore to distinguish this a little from the
> "usual" case.
It doesn''t make the patch much bigger, and it would be preferable.
> Jan
> 
>>> --- a/xen/arch/x86/hpet.c
>>> +++ b/xen/arch/x86/hpet.c
>>> @@ -436,6 +436,16 @@ static struct hpet_event_channel *hpet_g
>>>      return ch;
>>>  }
>>>  
>>> +static void _hpet_msi_set_affinity(const struct hpet_event_channel
*ch)
>>> +{
>>> +    struct irq_desc *desc = irq_to_desc(ch->irq);
>>> +
>>> +    ASSERT(!local_irq_is_enabled());
>>> +    spin_lock(&desc->lock);
>>> +    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
>>> +    spin_unlock(&desc->lock);
>>> +}
>>> +
>>>  static void hpet_attach_channel(unsigned int cpu,
>>>                                  struct hpet_event_channel *ch)
>>>  {
>>> @@ -450,7 +460,7 @@ static void hpet_attach_channel(unsigned
>>>      if ( ch->cpu != cpu )
>>>          return;
>>>  
>>> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
>>> +    _hpet_msi_set_affinity(ch);
>>>  }
>>>  
>>>  static void hpet_detach_channel(unsigned int cpu,
>>> @@ -472,7 +482,7 @@ static void hpet_detach_channel(unsigned
>>>      }
>>>  
>>>      ch->cpu = cpumask_first(ch->cpumask);
>>> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
>>> +    _hpet_msi_set_affinity(ch);
>>>  }
>>>  
>>>  #include <asm/mc146818rtc.h>
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
> 
> 
>

The IRQ descriptor lock should be held while adjusting the affinity of
any IRQ; the HPET channel lock isn''t sufficient to protect namely
against races with moving the IRQ to a different CPU.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: rename new helper function

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -436,6 +436,16 @@ static struct hpet_event_channel *hpet_g
     return ch;
 }
 
+static void set_channel_irq_affinity(const struct hpet_event_channel *ch)
+{
+    struct irq_desc *desc = irq_to_desc(ch->irq);
+
+    ASSERT(!local_irq_is_enabled());
+    spin_lock(&desc->lock);
+    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
+    spin_unlock(&desc->lock);
+}
+
 static void hpet_attach_channel(unsigned int cpu,
                                 struct hpet_event_channel *ch)
 {
@@ -450,7 +460,7 @@ static void hpet_attach_channel(unsigned
     if ( ch->cpu != cpu )
         return;
 
-    hpet_msi_set_affinity(irq_to_desc(ch->irq), cpumask_of(ch->cpu));
+    set_channel_irq_affinity(ch);
 }
 
 static void hpet_detach_channel(unsigned int cpu,
@@ -472,7 +482,7 @@ static void hpet_detach_channel(unsigned
     }
 
     ch->cpu = cpumask_first(ch->cpumask);
-    hpet_msi_set_affinity(irq_to_desc(ch->irq), cpumask_of(ch->cpu));
+    set_channel_irq_affinity(ch);
 }
 
 #include <asm/mc146818rtc.h>





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

This requires some additions to the VT-d side; AMD IOMMUs use the
"normal" MSI message format even when interrupt remapping is enabled,
thus making adjustments here unnecessary.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: refresh after updating patch 1 of this series (patch 3 is unchanged)

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -276,6 +276,7 @@ static int __init acpi_parse_hpet(struct
 	}
 
 	hpet_address = hpet_tbl->address.address;
+	hpet_blockid = hpet_tbl->sequence;
 	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
 	       hpet_tbl->id, hpet_address);
 
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -40,7 +40,7 @@ struct hpet_event_channel
 
     unsigned int idx;   /* physical channel idx */
     unsigned int cpu;   /* msi target */
-    int irq;            /* msi irq */
+    struct msi_desc msi;/* msi state */
     unsigned int flags; /* HPET_EVT_x */
 } __cacheline_aligned;
 static struct hpet_event_channel *__read_mostly hpet_events;
@@ -51,6 +51,7 @@ static unsigned int __read_mostly num_hp
 DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
 
 unsigned long __read_mostly hpet_address;
+u8 __initdata hpet_blockid;
 
 /*
  * force_hpet_broadcast: by default legacy hpet broadcast will be stopped
@@ -252,6 +253,8 @@ static void hpet_msi_mask(struct irq_des
 
 static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
+    if ( iommu_intremap )
+        iommu_update_ire_from_msi(&ch->msi, msg);
     hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
     hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
 }
@@ -261,6 +264,8 @@ static void hpet_msi_read(struct hpet_ev
     msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
     msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
     msg->address_hi = 0;
+    if ( iommu_intremap )
+        iommu_read_msi_from_ire(&ch->msi, msg);
 }
 
 static unsigned int hpet_msi_startup(struct irq_desc *desc)
@@ -292,6 +297,7 @@ static void hpet_msi_set_affinity(struct
     msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
     msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
     msg.address_lo |= MSI_ADDR_DEST_ID(dest);
+    msg.dest32 = dest;
     hpet_msi_write(desc->action->dev_id, &msg);
 }
 
@@ -316,35 +322,48 @@ static void __hpet_setup_msi_irq(struct 
     hpet_msi_write(desc->action->dev_id, &msg);
 }
 
-static int __init hpet_setup_msi_irq(unsigned int irq, struct
hpet_event_channel *ch)
+static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
 {
     int ret;
-    irq_desc_t *desc = irq_to_desc(irq);
+    irq_desc_t *desc = irq_to_desc(ch->msi.irq);
+
+    if ( iommu_intremap )
+    {
+        ch->msi.hpet_id = hpet_blockid;
+        ret = iommu_setup_hpet_msi(&ch->msi);
+        if ( ret )
+            return ret;
+    }
 
     desc->handler = &hpet_msi_type;
-    ret = request_irq(irq, hpet_interrupt_handler, 0, "HPET", ch);
+    ret = request_irq(ch->msi.irq, hpet_interrupt_handler, 0,
"HPET", ch);
     if ( ret < 0 )
+    {
+        if ( iommu_intremap )
+            iommu_update_ire_from_msi(&ch->msi, NULL);
         return ret;
+    }
 
     __hpet_setup_msi_irq(desc);
 
     return 0;
 }
 
-static int __init hpet_assign_irq(unsigned int idx)
+static int __init hpet_assign_irq(struct hpet_event_channel *ch)
 {
     int irq;
 
     if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
         return irq;
 
-    if ( hpet_setup_msi_irq(irq, hpet_events + idx) )
+    ch->msi.irq = irq;
+    if ( hpet_setup_msi_irq(ch) )
     {
         destroy_irq(irq);
         return -EINVAL;
     }
 
-    return irq;
+    return 0;
 }
 
 static void __init hpet_fsb_cap_lookup(void)
@@ -352,14 +371,6 @@ static void __init hpet_fsb_cap_lookup(v
     u32 id;
     unsigned int i, num_chs;
 
-    /* TODO. */
-    if ( iommu_intremap )
-    {
-        printk(XENLOG_INFO "HPET''s MSI mode hasn''t been
supported when "
-            "Interrupt Remapping is enabled.\n");
-        return;
-    }
-
     id = hpet_read32(HPET_ID);
 
     num_chs = ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT);
@@ -391,8 +402,8 @@ static void __init hpet_fsb_cap_lookup(v
         ch->flags = 0;
         ch->idx = i;
 
-        if ( (ch->irq = hpet_assign_irq(num_hpets_used++)) < 0 )
-            num_hpets_used--;
+        if ( hpet_assign_irq(ch) == 0 )
+            num_hpets_used++;
     }
 
     printk(XENLOG_INFO "HPET: %u timers (%u will be used for
broadcast)\n",
@@ -438,7 +449,7 @@ static struct hpet_event_channel *hpet_g
 
 static void set_channel_irq_affinity(const struct hpet_event_channel *ch)
 {
-    struct irq_desc *desc = irq_to_desc(ch->irq);
+    struct irq_desc *desc = irq_to_desc(ch->msi.irq);
 
     ASSERT(!local_irq_is_enabled());
     spin_lock(&desc->lock);
@@ -530,7 +541,7 @@ void __init hpet_broadcast_init(void)
             hpet_events = xzalloc(struct hpet_event_channel);
         if ( !hpet_events || !zalloc_cpumask_var(&hpet_events->cpumask)
)
             return;
-        hpet_events->irq = -1;
+        hpet_events->msi.irq = -1;
 
         /* Start HPET legacy interrupts */
         cfg |= HPET_CFG_LEGACY;
@@ -598,8 +609,8 @@ void hpet_broadcast_resume(void)
 
     for ( i = 0; i < n; i++ )
     {
-        if ( hpet_events[i].irq >= 0 )
-            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].irq));
+        if ( hpet_events[i].msi.irq >= 0 )
+            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
 
         /* set HPET Tn as oneshot */
         cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -495,6 +495,12 @@ unsigned int iommu_read_apic_from_ire(un
     return ops->read_apic_from_ire(apic, reg);
 }
 
+int __init iommu_setup_hpet_msi(struct msi_desc *msi)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : 0;
+}
+
 void iommu_resume()
 {
     const struct iommu_ops *ops = iommu_get_ops();
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -147,6 +147,34 @@ struct iommu * ioapic_to_iommu(unsigned 
     return NULL;
 }
 
+static bool_t acpi_hpet_device_match(
+    struct list_head *list, unsigned int hpet_id)
+{
+    struct acpi_hpet_unit *hpet;
+
+    list_for_each_entry( hpet, list, list )
+        if (hpet->id == hpet_id)
+            return 1;
+    return 0;
+}
+
+struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id)
+{
+    struct acpi_drhd_unit *drhd;
+
+    list_for_each_entry( drhd, &acpi_drhd_units, list )
+        if ( acpi_hpet_device_match(&drhd->hpet_list, hpet_id) )
+            return drhd;
+    return NULL;
+}
+
+struct iommu *hpet_to_iommu(unsigned int hpet_id)
+{
+    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
+
+    return drhd ? drhd->iommu : NULL;
+}
+
 static int __init acpi_register_atsr_unit(struct acpi_atsr_unit *atsr)
 {
     /*
@@ -330,6 +358,22 @@ static int __init acpi_parse_dev_scope(
             if ( iommu_verbose )
                 dprintk(VTDPREFIX, " MSI HPET: %04x:%02x:%02x.%u\n",
                         seg, bus, path->dev, path->fn);
+
+            if ( type == DMAR_TYPE )
+            {
+                struct acpi_drhd_unit *drhd = acpi_entry;
+                struct acpi_hpet_unit *acpi_hpet_unit;
+
+                acpi_hpet_unit = xmalloc(struct acpi_hpet_unit);
+                if ( !acpi_hpet_unit )
+                    return -ENOMEM;
+                acpi_hpet_unit->id = acpi_scope->enumeration_id;
+                acpi_hpet_unit->bus = bus;
+                acpi_hpet_unit->dev = path->dev;
+                acpi_hpet_unit->func = path->fn;
+                list_add(&acpi_hpet_unit->list,
&drhd->hpet_list);
+            }
+
             break;
 
         case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
@@ -407,6 +451,7 @@ acpi_parse_one_drhd(struct acpi_dmar_hea
     dmaru->segment = drhd->segment;
     dmaru->include_all = drhd->flags & ACPI_DMAR_INCLUDE_ALL;
     INIT_LIST_HEAD(&dmaru->ioapic_list);
+    INIT_LIST_HEAD(&dmaru->hpet_list);
     if ( iommu_verbose )
         dprintk(VTDPREFIX, "  dmaru->address =
%"PRIx64"\n",
                 dmaru->address);
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -39,6 +39,19 @@ struct acpi_ioapic_unit {
     }ioapic;
 };
 
+struct acpi_hpet_unit {
+    struct list_head list;
+    unsigned int id;
+    union {
+        u16 bdf;
+        struct {
+            u16 func: 3,
+                dev:  5,
+                bus:  8;
+        };
+    };
+};
+
 struct dmar_scope {
     DECLARE_BITMAP(buses, 256);         /* buses owned by this unit */
     u16    *devices;                    /* devices owned by this unit */
@@ -53,6 +66,7 @@ struct acpi_drhd_unit {
     u8     include_all:1;
     struct iommu *iommu;
     struct list_head ioapic_list;
+    struct list_head hpet_list;
 };
 
 struct acpi_rmrr_unit {
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -54,7 +54,9 @@ int iommu_flush_iec_index(struct iommu *
 void clear_fault_bits(struct iommu *iommu);
 
 struct iommu * ioapic_to_iommu(unsigned int apic_id);
+struct iommu * hpet_to_iommu(unsigned int hpet_id);
 struct acpi_drhd_unit * ioapic_to_drhd(unsigned int apic_id);
+struct acpi_drhd_unit * hpet_to_drhd(unsigned int hpet_id);
 struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu);
 struct acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
 
@@ -90,6 +92,8 @@ struct msi_msg;
 void msi_msg_read_remap_rte(struct msi_desc *, struct msi_msg *);
 void msi_msg_write_remap_rte(struct msi_desc *, struct msi_msg *);
 
+int intel_setup_hpet_msi(struct msi_desc *);
+
 int is_igd_vt_enabled_quirk(void);
 void platform_quirks_init(void);
 void vtd_ops_preamble_quirk(struct iommu* iommu);
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -107,6 +107,19 @@ static u16 apicid_to_bdf(int apic_id)
     return 0;
 }
 
+static u16 hpetid_to_bdf(unsigned int hpet_id)
+{
+    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
+    struct acpi_hpet_unit *acpi_hpet_unit;
+
+    list_for_each_entry ( acpi_hpet_unit, &drhd->hpet_list, list )
+        if ( acpi_hpet_unit->id == hpet_id )
+            return acpi_hpet_unit->bdf;
+
+    dprintk(XENLOG_ERR VTDPREFIX, "Didn''t find the bdf for HPET
%u!\n", hpet_id);
+    return 0;
+}
+
 static void set_ire_sid(struct iremap_entry *ire,
                         unsigned int svt, unsigned int sq, unsigned int sid)
 {
@@ -121,6 +134,16 @@ static void set_ioapic_source_id(int api
                 apicid_to_bdf(apic_id));
 }
 
+static void set_hpet_source_id(unsigned int id, struct iremap_entry *ire)
+{
+    /*
+     * Should really use SQ_ALL_16. Some platforms are broken.
+     * While we figure out the right quirks for these broken platforms, use
+     * SQ_13_IGNORE_3 for now.
+     */
+    set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3, hpetid_to_bdf(id));
+}
+
 int iommu_supports_eim(void)
 {
     struct acpi_drhd_unit *drhd;
@@ -592,7 +615,10 @@ static int msi_msg_to_remap_entry(
         new_ire.lo.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT)
                           & 0xff) << 8;
 
-    set_msi_source_id(pdev, &new_ire);
+    if ( pdev )
+        set_msi_source_id(pdev, &new_ire);
+    else
+        set_hpet_source_id(msi_desc->hpet_id, &new_ire);
     new_ire.hi.res_1 = 0;
     new_ire.lo.p = 1;    /* finally, set present bit */
 
@@ -624,7 +650,9 @@ void msi_msg_read_remap_rte(
     struct iommu *iommu = NULL;
     struct ir_ctrl *ir_ctrl;
 
-    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
+    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
+                : hpet_to_drhd(msi_desc->hpet_id);
+    if ( !drhd )
         return;
     iommu = drhd->iommu;
 
@@ -643,7 +671,9 @@ void msi_msg_write_remap_rte(
     struct iommu *iommu = NULL;
     struct ir_ctrl *ir_ctrl;
 
-    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
+    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
+                : hpet_to_drhd(msi_desc->hpet_id);
+    if ( !drhd )
         return;
     iommu = drhd->iommu;
 
@@ -654,6 +684,32 @@ void msi_msg_write_remap_rte(
     msi_msg_to_remap_entry(iommu, pdev, msi_desc, msg);
 }
 
+int __init intel_setup_hpet_msi(struct msi_desc *msi_desc)
+{
+    struct iommu *iommu = hpet_to_iommu(msi_desc->hpet_id);
+    struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
+    unsigned long flags;
+    int rc = 0;
+
+    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
+        return 0;
+
+    spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
+    msi_desc->remap_index = alloc_remap_entry(iommu);
+    if ( msi_desc->remap_index >= IREMAP_ENTRY_NR )
+    {
+        dprintk(XENLOG_ERR VTDPREFIX,
+                "%s: intremap index (%d) is larger than"
+                " the maximum index (%d)!\n",
+                __func__, msi_desc->remap_index, IREMAP_ENTRY_NR - 1);
+        msi_desc->remap_index = -1;
+        rc = -ENXIO;
+    }
+    spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
+
+    return rc;
+}
+
 int enable_intremap(struct iommu *iommu, int eim)
 {
     struct acpi_drhd_unit *drhd;
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2396,6 +2396,7 @@ const struct iommu_ops intel_iommu_ops     
.update_ire_from_msi = msi_msg_write_remap_rte,
     .read_apic_from_ire = io_apic_read_remap_rte,
     .read_msi_from_ire = msi_msg_read_remap_rte,
+    .setup_hpet_msi = intel_setup_hpet_msi,
     .suspend = vtd_suspend,
     .resume = vtd_resume,
     .share_p2m = iommu_set_pgd,
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -53,6 +53,7 @@
     (*(volatile u32 *)(fix_to_virt(FIX_HPET_BASE) + (x)) = (y))
 
 extern unsigned long hpet_address;
+extern u8 hpet_blockid;
 
 /*
  * Detect and initialise HPET hardware: return counter update frequency.
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -97,7 +97,10 @@ struct msi_desc {
 
 	struct list_head list;
 
-	void __iomem *mask_base;        /* va for the entry in mask table */
+	union {
+		void __iomem *mask_base;/* va for the entry in mask table */
+		unsigned int hpet_id;   /* HPET (dev is NULL)             */
+	};
 	struct pci_dev *dev;
 	int irq;
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -109,6 +109,7 @@ struct iommu_ops {
     void (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg
*msg);
     void (*read_msi_from_ire)(struct msi_desc *msi_desc, struct msi_msg *msg);
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
+    int (*setup_hpet_msi)(struct msi_desc *);
     void (*suspend)(void);
     void (*resume)(void);
     void (*share_p2m)(struct domain *d);
@@ -122,6 +123,7 @@ void iommu_update_ire_from_apic(unsigned
 void iommu_update_ire_from_msi(struct msi_desc *msi_desc, struct msi_msg *msg);
 void iommu_read_msi_from_ire(struct msi_desc *msi_desc, struct msi_msg *msg);
 unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
+int iommu_setup_hpet_msi(struct msi_desc *);
 
 void iommu_suspend(void);
 void iommu_resume(void);




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

On 17/10/2012 12:45, "Jan Beulich" <JBeulich@suse.com> wrote:
> The IRQ descriptor lock should be held while adjusting the affinity of
> any IRQ; the HPET channel lock isn''t sufficient to protect namely
> against races with moving the IRQ to a different CPU.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> ---
> v2: rename new helper function
> 
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -436,6 +436,16 @@ static struct hpet_event_channel *hpet_g
>      return ch;
>  }
>  
> +static void set_channel_irq_affinity(const struct hpet_event_channel *ch)
> +{
> +    struct irq_desc *desc = irq_to_desc(ch->irq);
> +
> +    ASSERT(!local_irq_is_enabled());
> +    spin_lock(&desc->lock);
> +    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
> +    spin_unlock(&desc->lock);
> +}
> +
>  static void hpet_attach_channel(unsigned int cpu,
>                                  struct hpet_event_channel *ch)
>  {
> @@ -450,7 +460,7 @@ static void hpet_attach_channel(unsigned
>      if ( ch->cpu != cpu )
>          return;
>  
> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
> +    set_channel_irq_affinity(ch);
>  }
>  
>  static void hpet_detach_channel(unsigned int cpu,
> @@ -472,7 +482,7 @@ static void hpet_detach_channel(unsigned
>      }
>  
>      ch->cpu = cpumask_first(ch->cpumask);
> -    hpet_msi_set_affinity(irq_to_desc(ch->irq),
cpumask_of(ch->cpu));
> +    set_channel_irq_affinity(ch);
>  }
>  
>  #include <asm/mc146818rtc.h>
> 
> 
>

On 17/10/2012 12:47, "Jan Beulich" <JBeulich@suse.com> wrote:
> This requires some additions to the VT-d side; AMD IOMMUs use the
> "normal" MSI message format even when interrupt remapping is
enabled,
> thus making adjustments here unnecessary.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>

...for the principle, but needs an Intel ack for the details.

 -- Keir
> ---
> v2: refresh after updating patch 1 of this series (patch 3 is unchanged)
> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -276,6 +276,7 @@ static int __init acpi_parse_hpet(struct
> }
>  
> hpet_address = hpet_tbl->address.address;
> + hpet_blockid = hpet_tbl->sequence;
> printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
>       hpet_tbl->id, hpet_address);
>  
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -40,7 +40,7 @@ struct hpet_event_channel
>  
>      unsigned int idx;   /* physical channel idx */
>      unsigned int cpu;   /* msi target */
> -    int irq;            /* msi irq */
> +    struct msi_desc msi;/* msi state */
>      unsigned int flags; /* HPET_EVT_x */
>  } __cacheline_aligned;
>  static struct hpet_event_channel *__read_mostly hpet_events;
> @@ -51,6 +51,7 @@ static unsigned int __read_mostly num_hp
>  DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
>  
>  unsigned long __read_mostly hpet_address;
> +u8 __initdata hpet_blockid;
>  
>  /*
>   * force_hpet_broadcast: by default legacy hpet broadcast will be stopped
> @@ -252,6 +253,8 @@ static void hpet_msi_mask(struct irq_des
>  
>  static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg
> *msg)
>  {
> +    if ( iommu_intremap )
> +        iommu_update_ire_from_msi(&ch->msi, msg);
>      hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
>      hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>  }
> @@ -261,6 +264,8 @@ static void hpet_msi_read(struct hpet_ev
>      msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
>      msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
>      msg->address_hi = 0;
> +    if ( iommu_intremap )
> +        iommu_read_msi_from_ire(&ch->msi, msg);
>  }
>  
>  static unsigned int hpet_msi_startup(struct irq_desc *desc)
> @@ -292,6 +297,7 @@ static void hpet_msi_set_affinity(struct
>      msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
>      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>      msg.address_lo |= MSI_ADDR_DEST_ID(dest);
> +    msg.dest32 = dest;
>      hpet_msi_write(desc->action->dev_id, &msg);
>  }
>  
> @@ -316,35 +322,48 @@ static void __hpet_setup_msi_irq(struct
>      hpet_msi_write(desc->action->dev_id, &msg);
>  }
>  
> -static int __init hpet_setup_msi_irq(unsigned int irq, struct
> hpet_event_channel *ch)
> +static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
>  {
>      int ret;
> -    irq_desc_t *desc = irq_to_desc(irq);
> +    irq_desc_t *desc = irq_to_desc(ch->msi.irq);
> +
> +    if ( iommu_intremap )
> +    {
> +        ch->msi.hpet_id = hpet_blockid;
> +        ret = iommu_setup_hpet_msi(&ch->msi);
> +        if ( ret )
> +            return ret;
> +    }
>  
>      desc->handler = &hpet_msi_type;
> -    ret = request_irq(irq, hpet_interrupt_handler, 0, "HPET",
ch);
> +    ret = request_irq(ch->msi.irq, hpet_interrupt_handler, 0,
"HPET", ch);
>      if ( ret < 0 )
> +    {
> +        if ( iommu_intremap )
> +            iommu_update_ire_from_msi(&ch->msi, NULL);
>          return ret;
> +    }
>  
>      __hpet_setup_msi_irq(desc);
>  
>      return 0;
>  }
>  
> -static int __init hpet_assign_irq(unsigned int idx)
> +static int __init hpet_assign_irq(struct hpet_event_channel *ch)
>  {
>      int irq;
>  
>      if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
>          return irq;
>  
> -    if ( hpet_setup_msi_irq(irq, hpet_events + idx) )
> +    ch->msi.irq = irq;
> +    if ( hpet_setup_msi_irq(ch) )
>      {
>          destroy_irq(irq);
>          return -EINVAL;
>      }
>  
> -    return irq;
> +    return 0;
>  }
>  
>  static void __init hpet_fsb_cap_lookup(void)
> @@ -352,14 +371,6 @@ static void __init hpet_fsb_cap_lookup(v
>      u32 id;
>      unsigned int i, num_chs;
>  
> -    /* TODO. */
> -    if ( iommu_intremap )
> -    {
> -        printk(XENLOG_INFO "HPET''s MSI mode hasn''t
been supported when "
> -            "Interrupt Remapping is enabled.\n");
> -        return;
> -    }
> -
>      id = hpet_read32(HPET_ID);
>  
>      num_chs = ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT);
> @@ -391,8 +402,8 @@ static void __init hpet_fsb_cap_lookup(v
>          ch->flags = 0;
>          ch->idx = i;
>  
> -        if ( (ch->irq = hpet_assign_irq(num_hpets_used++)) < 0 )
> -            num_hpets_used--;
> +        if ( hpet_assign_irq(ch) == 0 )
> +            num_hpets_used++;
>      }
>  
>      printk(XENLOG_INFO "HPET: %u timers (%u will be used for
broadcast)\n",
> @@ -438,7 +449,7 @@ static struct hpet_event_channel *hpet_g
>  
>  static void set_channel_irq_affinity(const struct hpet_event_channel *ch)
>  {
> -    struct irq_desc *desc = irq_to_desc(ch->irq);
> +    struct irq_desc *desc = irq_to_desc(ch->msi.irq);
>  
>      ASSERT(!local_irq_is_enabled());
>      spin_lock(&desc->lock);
> @@ -530,7 +541,7 @@ void __init hpet_broadcast_init(void)
>              hpet_events = xzalloc(struct hpet_event_channel);
>          if ( !hpet_events ||
!zalloc_cpumask_var(&hpet_events->cpumask) )
>              return;
> -        hpet_events->irq = -1;
> +        hpet_events->msi.irq = -1;
>  
>          /* Start HPET legacy interrupts */
>          cfg |= HPET_CFG_LEGACY;
> @@ -598,8 +609,8 @@ void hpet_broadcast_resume(void)
>  
>      for ( i = 0; i < n; i++ )
>      {
> -        if ( hpet_events[i].irq >= 0 )
> -            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].irq));
> +        if ( hpet_events[i].msi.irq >= 0 )
> +            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
>  
>          /* set HPET Tn as oneshot */
>          cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -495,6 +495,12 @@ unsigned int iommu_read_apic_from_ire(un
>      return ops->read_apic_from_ire(apic, reg);
>  }
>  
> +int __init iommu_setup_hpet_msi(struct msi_desc *msi)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : 0;
> +}
> +
>  void iommu_resume()
>  {
>      const struct iommu_ops *ops = iommu_get_ops();
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -147,6 +147,34 @@ struct iommu * ioapic_to_iommu(unsigned
>      return NULL;
>  }
>  
> +static bool_t acpi_hpet_device_match(
> +    struct list_head *list, unsigned int hpet_id)
> +{
> +    struct acpi_hpet_unit *hpet;
> +
> +    list_for_each_entry( hpet, list, list )
> +        if (hpet->id == hpet_id)
> +            return 1;
> +    return 0;
> +}
> +
> +struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id)
> +{
> +    struct acpi_drhd_unit *drhd;
> +
> +    list_for_each_entry( drhd, &acpi_drhd_units, list )
> +        if ( acpi_hpet_device_match(&drhd->hpet_list, hpet_id) )
> +            return drhd;
> +    return NULL;
> +}
> +
> +struct iommu *hpet_to_iommu(unsigned int hpet_id)
> +{
> +    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
> +
> +    return drhd ? drhd->iommu : NULL;
> +}
> +
>  static int __init acpi_register_atsr_unit(struct acpi_atsr_unit *atsr)
>  {
>      /*
> @@ -330,6 +358,22 @@ static int __init acpi_parse_dev_scope(
>              if ( iommu_verbose )
>                  dprintk(VTDPREFIX, " MSI HPET:
%04x:%02x:%02x.%u\n",
>                          seg, bus, path->dev, path->fn);
> +
> +            if ( type == DMAR_TYPE )
> +            {
> +                struct acpi_drhd_unit *drhd = acpi_entry;
> +                struct acpi_hpet_unit *acpi_hpet_unit;
> +
> +                acpi_hpet_unit = xmalloc(struct acpi_hpet_unit);
> +                if ( !acpi_hpet_unit )
> +                    return -ENOMEM;
> +                acpi_hpet_unit->id = acpi_scope->enumeration_id;
> +                acpi_hpet_unit->bus = bus;
> +                acpi_hpet_unit->dev = path->dev;
> +                acpi_hpet_unit->func = path->fn;
> +                list_add(&acpi_hpet_unit->list,
&drhd->hpet_list);
> +            }
> +
>              break;
>  
>          case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
> @@ -407,6 +451,7 @@ acpi_parse_one_drhd(struct acpi_dmar_hea
>      dmaru->segment = drhd->segment;
>      dmaru->include_all = drhd->flags & ACPI_DMAR_INCLUDE_ALL;
>      INIT_LIST_HEAD(&dmaru->ioapic_list);
> +    INIT_LIST_HEAD(&dmaru->hpet_list);
>      if ( iommu_verbose )
>          dprintk(VTDPREFIX, "  dmaru->address =
%"PRIx64"\n",
>                  dmaru->address);
> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -39,6 +39,19 @@ struct acpi_ioapic_unit {
>      }ioapic;
>  };
>  
> +struct acpi_hpet_unit {
> +    struct list_head list;
> +    unsigned int id;
> +    union {
> +        u16 bdf;
> +        struct {
> +            u16 func: 3,
> +                dev:  5,
> +                bus:  8;
> +        };
> +    };
> +};
> +
>  struct dmar_scope {
>      DECLARE_BITMAP(buses, 256);         /* buses owned by this unit */
>      u16    *devices;                    /* devices owned by this unit */
> @@ -53,6 +66,7 @@ struct acpi_drhd_unit {
>      u8     include_all:1;
>      struct iommu *iommu;
>      struct list_head ioapic_list;
> +    struct list_head hpet_list;
>  };
>  
>  struct acpi_rmrr_unit {
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -54,7 +54,9 @@ int iommu_flush_iec_index(struct iommu *
>  void clear_fault_bits(struct iommu *iommu);
>  
>  struct iommu * ioapic_to_iommu(unsigned int apic_id);
> +struct iommu * hpet_to_iommu(unsigned int hpet_id);
>  struct acpi_drhd_unit * ioapic_to_drhd(unsigned int apic_id);
> +struct acpi_drhd_unit * hpet_to_drhd(unsigned int hpet_id);
>  struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu);
>  struct acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
>  
> @@ -90,6 +92,8 @@ struct msi_msg;
>  void msi_msg_read_remap_rte(struct msi_desc *, struct msi_msg *);
>  void msi_msg_write_remap_rte(struct msi_desc *, struct msi_msg *);
>  
> +int intel_setup_hpet_msi(struct msi_desc *);
> +
>  int is_igd_vt_enabled_quirk(void);
>  void platform_quirks_init(void);
>  void vtd_ops_preamble_quirk(struct iommu* iommu);
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -107,6 +107,19 @@ static u16 apicid_to_bdf(int apic_id)
>      return 0;
>  }
>  
> +static u16 hpetid_to_bdf(unsigned int hpet_id)
> +{
> +    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
> +    struct acpi_hpet_unit *acpi_hpet_unit;
> +
> +    list_for_each_entry ( acpi_hpet_unit, &drhd->hpet_list, list )
> +        if ( acpi_hpet_unit->id == hpet_id )
> +            return acpi_hpet_unit->bdf;
> +
> +    dprintk(XENLOG_ERR VTDPREFIX, "Didn''t find the bdf for
HPET %u!\n",
> hpet_id);
> +    return 0;
> +}
> +
>  static void set_ire_sid(struct iremap_entry *ire,
>                          unsigned int svt, unsigned int sq, unsigned int
sid)
>  {
> @@ -121,6 +134,16 @@ static void set_ioapic_source_id(int api
>                  apicid_to_bdf(apic_id));
>  }
>  
> +static void set_hpet_source_id(unsigned int id, struct iremap_entry *ire)
> +{
> +    /*
> +     * Should really use SQ_ALL_16. Some platforms are broken.
> +     * While we figure out the right quirks for these broken platforms,
use
> +     * SQ_13_IGNORE_3 for now.
> +     */
> +    set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3,
hpetid_to_bdf(id));
> +}
> +
>  int iommu_supports_eim(void)
>  {
>      struct acpi_drhd_unit *drhd;
> @@ -592,7 +615,10 @@ static int msi_msg_to_remap_entry(
>          new_ire.lo.dst = ((msg->address_lo >>
MSI_ADDR_DEST_ID_SHIFT)
>                            & 0xff) << 8;
>  
> -    set_msi_source_id(pdev, &new_ire);
> +    if ( pdev )
> +        set_msi_source_id(pdev, &new_ire);
> +    else
> +        set_hpet_source_id(msi_desc->hpet_id, &new_ire);
>      new_ire.hi.res_1 = 0;
>      new_ire.lo.p = 1;    /* finally, set present bit */
>  
> @@ -624,7 +650,9 @@ void msi_msg_read_remap_rte(
>      struct iommu *iommu = NULL;
>      struct ir_ctrl *ir_ctrl;
>  
> -    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
> +    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
> +                : hpet_to_drhd(msi_desc->hpet_id);
> +    if ( !drhd )
>          return;
>      iommu = drhd->iommu;
>  
> @@ -643,7 +671,9 @@ void msi_msg_write_remap_rte(
>      struct iommu *iommu = NULL;
>      struct ir_ctrl *ir_ctrl;
>  
> -    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
> +    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
> +                : hpet_to_drhd(msi_desc->hpet_id);
> +    if ( !drhd )
>          return;
>      iommu = drhd->iommu;
>  
> @@ -654,6 +684,32 @@ void msi_msg_write_remap_rte(
>      msi_msg_to_remap_entry(iommu, pdev, msi_desc, msg);
>  }
>  
> +int __init intel_setup_hpet_msi(struct msi_desc *msi_desc)
> +{
> +    struct iommu *iommu = hpet_to_iommu(msi_desc->hpet_id);
> +    struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
> +    unsigned long flags;
> +    int rc = 0;
> +
> +    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
> +        return 0;
> +
> +    spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
> +    msi_desc->remap_index = alloc_remap_entry(iommu);
> +    if ( msi_desc->remap_index >= IREMAP_ENTRY_NR )
> +    {
> +        dprintk(XENLOG_ERR VTDPREFIX,
> +                "%s: intremap index (%d) is larger than"
> +                " the maximum index (%d)!\n",
> +                __func__, msi_desc->remap_index, IREMAP_ENTRY_NR - 1);
> +        msi_desc->remap_index = -1;
> +        rc = -ENXIO;
> +    }
> +    spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
> +
> +    return rc;
> +}
> +
>  int enable_intremap(struct iommu *iommu, int eim)
>  {
>      struct acpi_drhd_unit *drhd;
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2396,6 +2396,7 @@ const struct iommu_ops intel_iommu_ops >     
.update_ire_from_msi = msi_msg_write_remap_rte,
>      .read_apic_from_ire = io_apic_read_remap_rte,
>      .read_msi_from_ire = msi_msg_read_remap_rte,
> +    .setup_hpet_msi = intel_setup_hpet_msi,
>      .suspend = vtd_suspend,
>      .resume = vtd_resume,
>      .share_p2m = iommu_set_pgd,
> --- a/xen/include/asm-x86/hpet.h
> +++ b/xen/include/asm-x86/hpet.h
> @@ -53,6 +53,7 @@
>      (*(volatile u32 *)(fix_to_virt(FIX_HPET_BASE) + (x)) = (y))
>  
>  extern unsigned long hpet_address;
> +extern u8 hpet_blockid;
>  
>  /*
>   * Detect and initialise HPET hardware: return counter update frequency.
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -97,7 +97,10 @@ struct msi_desc {
>  
> struct list_head list;
>  
> - void __iomem *mask_base;        /* va for the entry in mask table */
> + union {
> +  void __iomem *mask_base;/* va for the entry in mask table */
> +  unsigned int hpet_id;   /* HPET (dev is NULL)             */
> + };
> struct pci_dev *dev;
> int irq;
>  
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -109,6 +109,7 @@ struct iommu_ops {
>      void (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg
> *msg);
>      void (*read_msi_from_ire)(struct msi_desc *msi_desc, struct msi_msg
> *msg);
>      unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int
reg);
> +    int (*setup_hpet_msi)(struct msi_desc *);
>      void (*suspend)(void);
>      void (*resume)(void);
>      void (*share_p2m)(struct domain *d);
> @@ -122,6 +123,7 @@ void iommu_update_ire_from_apic(unsigned
>  void iommu_update_ire_from_msi(struct msi_desc *msi_desc, struct msi_msg
> *msg);
>  void iommu_read_msi_from_ire(struct msi_desc *msi_desc, struct msi_msg
*msg);
>  unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int
reg);
> +int iommu_setup_hpet_msi(struct msi_desc *);
>  
>  void iommu_suspend(void);
>  void iommu_resume(void);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Acked-by: Xiantao Zhang<xiantao.zhang@intel.com>
> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com] On Behalf Of Keir Fraser
> Sent: Wednesday, October 17, 2012 8:00 PM
> To: Jan Beulich; xen-devel
> Cc: Zhang, Xiantao
> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/HPET: allow use for broadcast
> when interrupt remapping is in effect
> 
> On 17/10/2012 12:47, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
> > This requires some additions to the VT-d side; AMD IOMMUs use the
> > "normal" MSI message format even when interrupt remapping is
enabled,
> > thus making adjustments here unnecessary.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Keir Fraser <keir@xen.org>
> 
> ...for the principle, but needs an Intel ack for the details.
> 
>  -- Keir
> 
> > ---
> > v2: refresh after updating patch 1 of this series (patch 3 is
> > unchanged)
> >
> > --- a/xen/arch/x86/acpi/boot.c
> > +++ b/xen/arch/x86/acpi/boot.c
> > @@ -276,6 +276,7 @@ static int __init acpi_parse_hpet(struct }
> >
> > hpet_address = hpet_tbl->address.address;
> > + hpet_blockid = hpet_tbl->sequence;
> > printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
> >       hpet_tbl->id, hpet_address);
> >
> > --- a/xen/arch/x86/hpet.c
> > +++ b/xen/arch/x86/hpet.c
> > @@ -40,7 +40,7 @@ struct hpet_event_channel
> >
> >      unsigned int idx;   /* physical channel idx */
> >      unsigned int cpu;   /* msi target */
> > -    int irq;            /* msi irq */
> > +    struct msi_desc msi;/* msi state */
> >      unsigned int flags; /* HPET_EVT_x */  } __cacheline_aligned;
> > static struct hpet_event_channel *__read_mostly hpet_events; @@ -51,6
> > +51,7 @@ static unsigned int __read_mostly num_hp
> > DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
> >
> >  unsigned long __read_mostly hpet_address;
> > +u8 __initdata hpet_blockid;
> >
> >  /*
> >   * force_hpet_broadcast: by default legacy hpet broadcast will be
> > stopped @@ -252,6 +253,8 @@ static void hpet_msi_mask(struct irq_des
> >
> >  static void hpet_msi_write(struct hpet_event_channel *ch, struct
> > msi_msg
> > *msg)
> >  {
> > +    if ( iommu_intremap )
> > +        iommu_update_ire_from_msi(&ch->msi, msg);
> >      hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
> >      hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4); 
} @@
> > -261,6 +264,8 @@ static void hpet_msi_read(struct hpet_ev
> >      msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
> >      msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
> >      msg->address_hi = 0;
> > +    if ( iommu_intremap )
> > +        iommu_read_msi_from_ire(&ch->msi, msg);
> >  }
> >
> >  static unsigned int hpet_msi_startup(struct irq_desc *desc) @@ -292,6
> > +297,7 @@ static void hpet_msi_set_affinity(struct
> >      msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
> >      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
> >      msg.address_lo |= MSI_ADDR_DEST_ID(dest);
> > +    msg.dest32 = dest;
> >      hpet_msi_write(desc->action->dev_id, &msg);  }
> >
> > @@ -316,35 +322,48 @@ static void __hpet_setup_msi_irq(struct
> >      hpet_msi_write(desc->action->dev_id, &msg);  }
> >
> > -static int __init hpet_setup_msi_irq(unsigned int irq, struct
> > hpet_event_channel *ch)
> > +static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
> >  {
> >      int ret;
> > -    irq_desc_t *desc = irq_to_desc(irq);
> > +    irq_desc_t *desc = irq_to_desc(ch->msi.irq);
> > +
> > +    if ( iommu_intremap )
> > +    {
> > +        ch->msi.hpet_id = hpet_blockid;
> > +        ret = iommu_setup_hpet_msi(&ch->msi);
> > +        if ( ret )
> > +            return ret;
> > +    }
> >
> >      desc->handler = &hpet_msi_type;
> > -    ret = request_irq(irq, hpet_interrupt_handler, 0,
"HPET", ch);
> > +    ret = request_irq(ch->msi.irq, hpet_interrupt_handler, 0,
"HPET",
> > + ch);
> >      if ( ret < 0 )
> > +    {
> > +        if ( iommu_intremap )
> > +            iommu_update_ire_from_msi(&ch->msi, NULL);
> >          return ret;
> > +    }
> >
> >      __hpet_setup_msi_irq(desc);
> >
> >      return 0;
> >  }
> >
> > -static int __init hpet_assign_irq(unsigned int idx)
> > +static int __init hpet_assign_irq(struct hpet_event_channel *ch)
> >  {
> >      int irq;
> >
> >      if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
> >          return irq;
> >
> > -    if ( hpet_setup_msi_irq(irq, hpet_events + idx) )
> > +    ch->msi.irq = irq;
> > +    if ( hpet_setup_msi_irq(ch) )
> >      {
> >          destroy_irq(irq);
> >          return -EINVAL;
> >      }
> >
> > -    return irq;
> > +    return 0;
> >  }
> >
> >  static void __init hpet_fsb_cap_lookup(void) @@ -352,14 +371,6 @@
> > static void __init hpet_fsb_cap_lookup(v
> >      u32 id;
> >      unsigned int i, num_chs;
> >
> > -    /* TODO. */
> > -    if ( iommu_intremap )
> > -    {
> > -        printk(XENLOG_INFO "HPET''s MSI mode
hasn''t been supported when
> "
> > -            "Interrupt Remapping is enabled.\n");
> > -        return;
> > -    }
> > -
> >      id = hpet_read32(HPET_ID);
> >
> >      num_chs = ((id & HPET_ID_NUMBER) >>
HPET_ID_NUMBER_SHIFT);
> @@
> > -391,8 +402,8 @@ static void __init hpet_fsb_cap_lookup(v
> >          ch->flags = 0;
> >          ch->idx = i;
> >
> > -        if ( (ch->irq = hpet_assign_irq(num_hpets_used++)) < 0
)
> > -            num_hpets_used--;
> > +        if ( hpet_assign_irq(ch) == 0 )
> > +            num_hpets_used++;
> >      }
> >
> >      printk(XENLOG_INFO "HPET: %u timers (%u will be used for
> > broadcast)\n", @@ -438,7 +449,7 @@ static struct
hpet_event_channel
> > *hpet_g
> >
> >  static void set_channel_irq_affinity(const struct hpet_event_channel
> > *ch)  {
> > -    struct irq_desc *desc = irq_to_desc(ch->irq);
> > +    struct irq_desc *desc = irq_to_desc(ch->msi.irq);
> >
> >      ASSERT(!local_irq_is_enabled());
> >      spin_lock(&desc->lock);
> > @@ -530,7 +541,7 @@ void __init hpet_broadcast_init(void)
> >              hpet_events = xzalloc(struct hpet_event_channel);
> >          if ( !hpet_events ||
!zalloc_cpumask_var(&hpet_events->cpumask) )
> >              return;
> > -        hpet_events->irq = -1;
> > +        hpet_events->msi.irq = -1;
> >
> >          /* Start HPET legacy interrupts */
> >          cfg |= HPET_CFG_LEGACY;
> > @@ -598,8 +609,8 @@ void hpet_broadcast_resume(void)
> >
> >      for ( i = 0; i < n; i++ )
> >      {
> > -        if ( hpet_events[i].irq >= 0 )
> > -            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].irq));
> > +        if ( hpet_events[i].msi.irq >= 0 )
> > +
> > + __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
> >
> >          /* set HPET Tn as oneshot */
> >          cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -495,6 +495,12 @@ unsigned int iommu_read_apic_from_ire(un
> >      return ops->read_apic_from_ire(apic, reg);  }
> >
> > +int __init iommu_setup_hpet_msi(struct msi_desc *msi) {
> > +    const struct iommu_ops *ops = iommu_get_ops();
> > +    return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : 0;
}
> > +
> >  void iommu_resume()
> >  {
> >      const struct iommu_ops *ops = iommu_get_ops();
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -147,6 +147,34 @@ struct iommu * ioapic_to_iommu(unsigned
> >      return NULL;
> >  }
> >
> > +static bool_t acpi_hpet_device_match(
> > +    struct list_head *list, unsigned int hpet_id) {
> > +    struct acpi_hpet_unit *hpet;
> > +
> > +    list_for_each_entry( hpet, list, list )
> > +        if (hpet->id == hpet_id)
> > +            return 1;
> > +    return 0;
> > +}
> > +
> > +struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id) {
> > +    struct acpi_drhd_unit *drhd;
> > +
> > +    list_for_each_entry( drhd, &acpi_drhd_units, list )
> > +        if ( acpi_hpet_device_match(&drhd->hpet_list, hpet_id)
)
> > +            return drhd;
> > +    return NULL;
> > +}
> > +
> > +struct iommu *hpet_to_iommu(unsigned int hpet_id) {
> > +    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
> > +
> > +    return drhd ? drhd->iommu : NULL; }
> > +
> >  static int __init acpi_register_atsr_unit(struct acpi_atsr_unit
> > *atsr)  {
> >      /*
> > @@ -330,6 +358,22 @@ static int __init acpi_parse_dev_scope(
> >              if ( iommu_verbose )
> >                  dprintk(VTDPREFIX, " MSI HPET:
%04x:%02x:%02x.%u\n",
> >                          seg, bus, path->dev, path->fn);
> > +
> > +            if ( type == DMAR_TYPE )
> > +            {
> > +                struct acpi_drhd_unit *drhd = acpi_entry;
> > +                struct acpi_hpet_unit *acpi_hpet_unit;
> > +
> > +                acpi_hpet_unit = xmalloc(struct acpi_hpet_unit);
> > +                if ( !acpi_hpet_unit )
> > +                    return -ENOMEM;
> > +                acpi_hpet_unit->id =
acpi_scope->enumeration_id;
> > +                acpi_hpet_unit->bus = bus;
> > +                acpi_hpet_unit->dev = path->dev;
> > +                acpi_hpet_unit->func = path->fn;
> > +                list_add(&acpi_hpet_unit->list,
&drhd->hpet_list);
> > +            }
> > +
> >              break;
> >
> >          case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
> > @@ -407,6 +451,7 @@ acpi_parse_one_drhd(struct acpi_dmar_hea
> >      dmaru->segment = drhd->segment;
> >      dmaru->include_all = drhd->flags &
ACPI_DMAR_INCLUDE_ALL;
> >      INIT_LIST_HEAD(&dmaru->ioapic_list);
> > +    INIT_LIST_HEAD(&dmaru->hpet_list);
> >      if ( iommu_verbose )
> >          dprintk(VTDPREFIX, "  dmaru->address =
%"PRIx64"\n",
> >                  dmaru->address);
> > --- a/xen/drivers/passthrough/vtd/dmar.h
> > +++ b/xen/drivers/passthrough/vtd/dmar.h
> > @@ -39,6 +39,19 @@ struct acpi_ioapic_unit {
> >      }ioapic;
> >  };
> >
> > +struct acpi_hpet_unit {
> > +    struct list_head list;
> > +    unsigned int id;
> > +    union {
> > +        u16 bdf;
> > +        struct {
> > +            u16 func: 3,
> > +                dev:  5,
> > +                bus:  8;
> > +        };
> > +    };
> > +};
> > +
> >  struct dmar_scope {
> >      DECLARE_BITMAP(buses, 256);         /* buses owned by this unit
*/
> >      u16    *devices;                    /* devices owned by this unit
*/
> > @@ -53,6 +66,7 @@ struct acpi_drhd_unit {
> >      u8     include_all:1;
> >      struct iommu *iommu;
> >      struct list_head ioapic_list;
> > +    struct list_head hpet_list;
> >  };
> >
> >  struct acpi_rmrr_unit {
> > --- a/xen/drivers/passthrough/vtd/extern.h
> > +++ b/xen/drivers/passthrough/vtd/extern.h
> > @@ -54,7 +54,9 @@ int iommu_flush_iec_index(struct iommu *  void
> > clear_fault_bits(struct iommu *iommu);
> >
> >  struct iommu * ioapic_to_iommu(unsigned int apic_id);
> > +struct iommu * hpet_to_iommu(unsigned int hpet_id);
> >  struct acpi_drhd_unit * ioapic_to_drhd(unsigned int apic_id);
> > +struct acpi_drhd_unit * hpet_to_drhd(unsigned int hpet_id);
> >  struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu);  struct
> > acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
> >
> > @@ -90,6 +92,8 @@ struct msi_msg;
> >  void msi_msg_read_remap_rte(struct msi_desc *, struct msi_msg *);
> > void msi_msg_write_remap_rte(struct msi_desc *, struct msi_msg *);
> >
> > +int intel_setup_hpet_msi(struct msi_desc *);
> > +
> >  int is_igd_vt_enabled_quirk(void);
> >  void platform_quirks_init(void);
> >  void vtd_ops_preamble_quirk(struct iommu* iommu);
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -107,6 +107,19 @@ static u16 apicid_to_bdf(int apic_id)
> >      return 0;
> >  }
> >
> > +static u16 hpetid_to_bdf(unsigned int hpet_id) {
> > +    struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
> > +    struct acpi_hpet_unit *acpi_hpet_unit;
> > +
> > +    list_for_each_entry ( acpi_hpet_unit, &drhd->hpet_list,
list )
> > +        if ( acpi_hpet_unit->id == hpet_id )
> > +            return acpi_hpet_unit->bdf;
> > +
> > +    dprintk(XENLOG_ERR VTDPREFIX, "Didn''t find the bdf
for HPET
> > + %u!\n",
> > hpet_id);
> > +    return 0;
> > +}
> > +
> >  static void set_ire_sid(struct iremap_entry *ire,
> >                          unsigned int svt, unsigned int sq, unsigned
> > int sid)  { @@ -121,6 +134,16 @@ static void set_ioapic_source_id(int
> > api
> >                  apicid_to_bdf(apic_id));  }
> >
> > +static void set_hpet_source_id(unsigned int id, struct iremap_entry
> > +*ire) {
> > +    /*
> > +     * Should really use SQ_ALL_16. Some platforms are broken.
> > +     * While we figure out the right quirks for these broken
platforms, use
> > +     * SQ_13_IGNORE_3 for now.
> > +     */
> > +    set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_13_IGNORE_3,
> > +hpetid_to_bdf(id)); }
> > +
> >  int iommu_supports_eim(void)
> >  {
> >      struct acpi_drhd_unit *drhd;
> > @@ -592,7 +615,10 @@ static int msi_msg_to_remap_entry(
> >          new_ire.lo.dst = ((msg->address_lo >>
MSI_ADDR_DEST_ID_SHIFT)
> >                            & 0xff) << 8;
> >
> > -    set_msi_source_id(pdev, &new_ire);
> > +    if ( pdev )
> > +        set_msi_source_id(pdev, &new_ire);
> > +    else
> > +        set_hpet_source_id(msi_desc->hpet_id, &new_ire);
> >      new_ire.hi.res_1 = 0;
> >      new_ire.lo.p = 1;    /* finally, set present bit */
> >
> > @@ -624,7 +650,9 @@ void msi_msg_read_remap_rte(
> >      struct iommu *iommu = NULL;
> >      struct ir_ctrl *ir_ctrl;
> >
> > -    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
> > +    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
> > +                : hpet_to_drhd(msi_desc->hpet_id);
> > +    if ( !drhd )
> >          return;
> >      iommu = drhd->iommu;
> >
> > @@ -643,7 +671,9 @@ void msi_msg_write_remap_rte(
> >      struct iommu *iommu = NULL;
> >      struct ir_ctrl *ir_ctrl;
> >
> > -    if ( (drhd = acpi_find_matched_drhd_unit(pdev)) == NULL )
> > +    drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
> > +                : hpet_to_drhd(msi_desc->hpet_id);
> > +    if ( !drhd )
> >          return;
> >      iommu = drhd->iommu;
> >
> > @@ -654,6 +684,32 @@ void msi_msg_write_remap_rte(
> >      msi_msg_to_remap_entry(iommu, pdev, msi_desc, msg);  }
> >
> > +int __init intel_setup_hpet_msi(struct msi_desc *msi_desc) {
> > +    struct iommu *iommu = hpet_to_iommu(msi_desc->hpet_id);
> > +    struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
> > +    unsigned long flags;
> > +    int rc = 0;
> > +
> > +    if ( !ir_ctrl || !ir_ctrl->iremap_maddr )
> > +        return 0;
> > +
> > +    spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
> > +    msi_desc->remap_index = alloc_remap_entry(iommu);
> > +    if ( msi_desc->remap_index >= IREMAP_ENTRY_NR )
> > +    {
> > +        dprintk(XENLOG_ERR VTDPREFIX,
> > +                "%s: intremap index (%d) is larger than"
> > +                " the maximum index (%d)!\n",
> > +                __func__, msi_desc->remap_index, IREMAP_ENTRY_NR -
1);
> > +        msi_desc->remap_index = -1;
> > +        rc = -ENXIO;
> > +    }
> > +    spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
> > +
> > +    return rc;
> > +}
> > +
> >  int enable_intremap(struct iommu *iommu, int eim)  {
> >      struct acpi_drhd_unit *drhd;
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -2396,6 +2396,7 @@ const struct iommu_ops intel_iommu_ops > >
.update_ire_from_msi = msi_msg_write_remap_rte,
> >      .read_apic_from_ire = io_apic_read_remap_rte,
> >      .read_msi_from_ire = msi_msg_read_remap_rte,
> > +    .setup_hpet_msi = intel_setup_hpet_msi,
> >      .suspend = vtd_suspend,
> >      .resume = vtd_resume,
> >      .share_p2m = iommu_set_pgd,
> > --- a/xen/include/asm-x86/hpet.h
> > +++ b/xen/include/asm-x86/hpet.h
> > @@ -53,6 +53,7 @@
> >      (*(volatile u32 *)(fix_to_virt(FIX_HPET_BASE) + (x)) = (y))
> >
> >  extern unsigned long hpet_address;
> > +extern u8 hpet_blockid;
> >
> >  /*
> >   * Detect and initialise HPET hardware: return counter update
frequency.
> > --- a/xen/include/asm-x86/msi.h
> > +++ b/xen/include/asm-x86/msi.h
> > @@ -97,7 +97,10 @@ struct msi_desc {
> >
> > struct list_head list;
> >
> > - void __iomem *mask_base;        /* va for the entry in mask table */
> > + union {
> > +  void __iomem *mask_base;/* va for the entry in mask table */
> > +  unsigned int hpet_id;   /* HPET (dev is NULL)             */
> > + };
> > struct pci_dev *dev;
> > int irq;
> >
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -109,6 +109,7 @@ struct iommu_ops {
> >      void (*update_ire_from_msi)(struct msi_desc *msi_desc, struct
> > msi_msg *msg);
> >      void (*read_msi_from_ire)(struct msi_desc *msi_desc, struct
> > msi_msg *msg);
> >      unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned
> > int reg);
> > +    int (*setup_hpet_msi)(struct msi_desc *);
> >      void (*suspend)(void);
> >      void (*resume)(void);
> >      void (*share_p2m)(struct domain *d); @@ -122,6 +123,7 @@ void
> > iommu_update_ire_from_apic(unsigned
> >  void iommu_update_ire_from_msi(struct msi_desc *msi_desc, struct
> > msi_msg *msg);  void iommu_read_msi_from_ire(struct msi_desc
> > *msi_desc, struct msi_msg *msg);  unsigned int
> > iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
> > +int iommu_setup_hpet_msi(struct msi_desc *);
> >
> >  void iommu_suspend(void);
> >  void iommu_resume(void);
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>

>>> On 17.10.12 at 13:59, Keir Fraser <keir@xen.org> wrote:
> On 17/10/2012 12:47, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> This requires some additions to the VT-d side; AMD IOMMUs use the
>> "normal" MSI message format even when interrupt remapping is
enabled,
>> thus making adjustments here unnecessary.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Keir Fraser <keir@xen.org>
> 
> ...for the principle, but needs an Intel ack for the details.
> 
>  -- Keir
> 
>> ---
>> v2: refresh after updating patch 1 of this series (patch 3 is
unchanged)
Committed - how about patch 3 then?

Jan

On 16/10/2012 16:11, "Jan Beulich" <JBeulich@suse.com> wrote:
> Rather than spending measurable amounts of time reading back the most
> recently written message, cache it in space previously unused, and thus
> accelerate the CPU''s entering of the intended C-state.
> 
> hpet_msi_read() ends up being unused after this change, but rather than
> removing the function, it''s being marked "unused" in
order - that way
> it can easily get used again should a new need for it arise.
Please use __attribute_used__
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -253,17 +253,19 @@ static void hpet_msi_mask(struct irq_des
>  
>  static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg
> *msg)
>  {
> +    ch->msi.msg = *msg;
>      if ( iommu_intremap )
>          iommu_update_ire_from_msi(&ch->msi, msg);
>      hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
>      hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>  }
>  
> -static void hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg
*msg)
> +static void __attribute__((__unused__))
> +hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
>  {
>      msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
>      msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
> -    msg->address_hi = 0;
> +    msg->address_hi = MSI_ADDR_BASE_HI;
>      if ( iommu_intremap )
>          iommu_read_msi_from_ire(&ch->msi, msg);
>  }
> @@ -285,20 +287,19 @@ static void hpet_msi_ack(struct irq_desc
>  
>  static void hpet_msi_set_affinity(struct irq_desc *desc, const cpumask_t
> *mask)
>  {
> -    struct msi_msg msg;
> -    unsigned int dest;
> +    struct hpet_event_channel *ch = desc->action->dev_id;
> +    struct msi_msg msg = ch->msi.msg;
>  
> -    dest = set_desc_affinity(desc, mask);
> -    if (dest == BAD_APICID)
> +    msg.dest32 = set_desc_affinity(desc, mask);
> +    if ( msg.dest32 == BAD_APICID )
>          return;
>  
> -    hpet_msi_read(desc->action->dev_id, &msg);
>      msg.data &= ~MSI_DATA_VECTOR_MASK;
>      msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
>      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
> -    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
> -    msg.dest32 = dest;
> -    hpet_msi_write(desc->action->dev_id, &msg);
> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
> +    if ( msg.data != ch->msi.msg.data || msg.dest32 !=
ch->msi.msg.dest32 )
> +        hpet_msi_write(ch, &msg);
>  }
>  
>  /*
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 18.10.12 at 10:22, Keir Fraser <keir@xen.org> wrote:
> On 16/10/2012 16:11, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> Rather than spending measurable amounts of time reading back the most
>> recently written message, cache it in space previously unused, and thus
>> accelerate the CPU''s entering of the intended C-state.
>> 
>> hpet_msi_read() ends up being unused after this change, but rather than
>> removing the function, it''s being marked "unused" in
order - that way
>> it can easily get used again should a new need for it arise.
> 
> Please use __attribute_used__
That wouldn''t be correct: The function _is_ unused (and
there''s
no issue if it was used afaik), and the __used__ attribute ought
to tell the compiler to keep the function around despite not
having (visible to it) callers.

Jan
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Keir Fraser <keir@xen.org>
> 
>> --- a/xen/arch/x86/hpet.c
>> +++ b/xen/arch/x86/hpet.c
>> @@ -253,17 +253,19 @@ static void hpet_msi_mask(struct irq_des
>>  
>>  static void hpet_msi_write(struct hpet_event_channel *ch, struct
msi_msg
>> *msg)
>>  {
>> +    ch->msi.msg = *msg;
>>      if ( iommu_intremap )
>>          iommu_update_ire_from_msi(&ch->msi, msg);
>>      hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
>>      hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>>  }
>>  
>> -static void hpet_msi_read(struct hpet_event_channel *ch, struct
msi_msg
> *msg)
>> +static void __attribute__((__unused__))
>> +hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
>>  {
>>      msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
>>      msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
>> -    msg->address_hi = 0;
>> +    msg->address_hi = MSI_ADDR_BASE_HI;
>>      if ( iommu_intremap )
>>          iommu_read_msi_from_ire(&ch->msi, msg);
>>  }
>> @@ -285,20 +287,19 @@ static void hpet_msi_ack(struct irq_desc
>>  
>>  static void hpet_msi_set_affinity(struct irq_desc *desc, const
cpumask_t
>> *mask)
>>  {
>> -    struct msi_msg msg;
>> -    unsigned int dest;
>> +    struct hpet_event_channel *ch = desc->action->dev_id;
>> +    struct msi_msg msg = ch->msi.msg;
>>  
>> -    dest = set_desc_affinity(desc, mask);
>> -    if (dest == BAD_APICID)
>> +    msg.dest32 = set_desc_affinity(desc, mask);
>> +    if ( msg.dest32 == BAD_APICID )
>>          return;
>>  
>> -    hpet_msi_read(desc->action->dev_id, &msg);
>>      msg.data &= ~MSI_DATA_VECTOR_MASK;
>>      msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
>>      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>> -    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
>> -    msg.dest32 = dest;
>> -    hpet_msi_write(desc->action->dev_id, &msg);
>> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
>> +    if ( msg.data != ch->msi.msg.data || msg.dest32 !=
ch->msi.msg.dest32 )
>> +        hpet_msi_write(ch, &msg);
>>  }
>>  
>>  /*
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org 
>> http://lists.xen.org/xen-devel

On 18/10/2012 11:39, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 18.10.12 at 10:22, Keir Fraser <keir@xen.org> wrote:
>> On 16/10/2012 16:11, "Jan Beulich" <JBeulich@suse.com>
wrote:
>> 
>>> Rather than spending measurable amounts of time reading back the
most
>>> recently written message, cache it in space previously unused, and
thus
>>> accelerate the CPU''s entering of the intended C-state.
>>> 
>>> hpet_msi_read() ends up being unused after this change, but rather
than
>>> removing the function, it''s being marked
"unused" in order - that way
>>> it can easily get used again should a new need for it arise.
>> 
>> Please use __attribute_used__
> 
> That wouldn''t be correct: The function _is_ unused (and
there''s
> no issue if it was used afaik), and the __used__ attribute ought
> to tell the compiler to keep the function around despite not
> having (visible to it) callers.
Perhaps our __attribute_used__ definition should change, then?

 -- Keir
> Jan
> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Acked-by: Keir Fraser <keir@xen.org>
>> 
>>> --- a/xen/arch/x86/hpet.c
>>> +++ b/xen/arch/x86/hpet.c
>>> @@ -253,17 +253,19 @@ static void hpet_msi_mask(struct irq_des
>>>  
>>>  static void hpet_msi_write(struct hpet_event_channel *ch, struct
msi_msg
>>> *msg)
>>>  {
>>> +    ch->msi.msg = *msg;
>>>      if ( iommu_intremap )
>>>          iommu_update_ire_from_msi(&ch->msi, msg);
>>>      hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
>>>      hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) +
4);
>>>  }
>>>  
>>> -static void hpet_msi_read(struct hpet_event_channel *ch, struct
msi_msg
>> *msg)
>>> +static void __attribute__((__unused__))
>>> +hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
>>>  {
>>>      msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
>>>      msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) +
4);
>>> -    msg->address_hi = 0;
>>> +    msg->address_hi = MSI_ADDR_BASE_HI;
>>>      if ( iommu_intremap )
>>>          iommu_read_msi_from_ire(&ch->msi, msg);
>>>  }
>>> @@ -285,20 +287,19 @@ static void hpet_msi_ack(struct irq_desc
>>>  
>>>  static void hpet_msi_set_affinity(struct irq_desc *desc, const
cpumask_t
>>> *mask)
>>>  {
>>> -    struct msi_msg msg;
>>> -    unsigned int dest;
>>> +    struct hpet_event_channel *ch = desc->action->dev_id;
>>> +    struct msi_msg msg = ch->msi.msg;
>>>  
>>> -    dest = set_desc_affinity(desc, mask);
>>> -    if (dest == BAD_APICID)
>>> +    msg.dest32 = set_desc_affinity(desc, mask);
>>> +    if ( msg.dest32 == BAD_APICID )
>>>          return;
>>>  
>>> -    hpet_msi_read(desc->action->dev_id, &msg);
>>>      msg.data &= ~MSI_DATA_VECTOR_MASK;
>>>      msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
>>>      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>>> -    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
>>> -    msg.dest32 = dest;
>>> -    hpet_msi_write(desc->action->dev_id, &msg);
>>> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
>>> +    if ( msg.data != ch->msi.msg.data || msg.dest32 !=
ch->msi.msg.dest32 )
>>> +        hpet_msi_write(ch, &msg);
>>>  }
>>>  
>>>  /*
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
> 
> 
>

>>> On 18.10.12 at 18:42, Keir Fraser <keir.xen@gmail.com> wrote:
> On 18/10/2012 11:39, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>>> On 18.10.12 at 10:22, Keir Fraser <keir@xen.org>
wrote:
>>> On 16/10/2012 16:11, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>> 
>>>> Rather than spending measurable amounts of time reading back
the most
>>>> recently written message, cache it in space previously unused,
and thus
>>>> accelerate the CPU''s entering of the intended C-state.
>>>> 
>>>> hpet_msi_read() ends up being unused after this change, but
rather than
>>>> removing the function, it''s being marked
"unused" in order - that way
>>>> it can easily get used again should a new need for it arise.
>>> 
>>> Please use __attribute_used__
>> 
>> That wouldn''t be correct: The function _is_ unused (and
there''s
>> no issue if it was used afaik), and the __used__ attribute ought
>> to tell the compiler to keep the function around despite not
>> having (visible to it) callers.
> 
> Perhaps our __attribute_used__ definition should change, then?
I don''t think so - this has its own value as is. We may want to add
a Linux-like __maybe_unused, though.

Jan

On 19/10/2012 08:57, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> That wouldn''t be correct: The function _is_ unused (and
there''s
>>> no issue if it was used afaik), and the __used__ attribute ought
>>> to tell the compiler to keep the function around despite not
>>> having (visible to it) callers.
>> 
>> Perhaps our __attribute_used__ definition should change, then?
> 
> I don''t think so - this has its own value as is. We may want to
add
> a Linux-like __maybe_unused, though.
Could you do that, and add comments to the definitions of __attribute_used__
and __maybe_unused, describing exactly what they are to be used for?

 Thanks,
 Keir

>>> On 19.10.12 at 11:32, Keir Fraser <keir@xen.org> wrote:
> On 19/10/2012 08:57, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>> That wouldn''t be correct: The function _is_ unused
(and there''s
>>>> no issue if it was used afaik), and the __used__ attribute
ought
>>>> to tell the compiler to keep the function around despite not
>>>> having (visible to it) callers.
>>> 
>>> Perhaps our __attribute_used__ definition should change, then?
>> 
>> I don''t think so - this has its own value as is. We may want
to add
>> a Linux-like __maybe_unused, though.
> 
> Could you do that, and add comments to the definitions of
__attribute_used__
> and __maybe_unused, describing exactly what they are to be used for?
Sure, will do.

Jan

Rather than spending measurable amounts of time reading back the most
recently written message, cache it in space previously unused, and thus
accelerate the CPU''s entering of the intended C-state.

hpet_msi_read() ends up being unused after this change, but rather than
removing the function, it''s being marked "unused" in order -
that way
it can easily get used again should a new need for it arise.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-based on top of the previously sent patch "compiler.h
    adjustments".

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -253,17 +253,19 @@ static void hpet_msi_mask(struct irq_des
 
 static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
+    ch->msi.msg = *msg;
     if ( iommu_intremap )
         iommu_update_ire_from_msi(&ch->msi, msg);
     hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
     hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
 }
 
-static void hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
+static void __maybe_unused
+hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
     msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
     msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
-    msg->address_hi = 0;
+    msg->address_hi = MSI_ADDR_BASE_HI;
     if ( iommu_intremap )
         iommu_read_msi_from_ire(&ch->msi, msg);
 }
@@ -285,20 +287,19 @@ static void hpet_msi_ack(struct irq_desc
 
 static void hpet_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
 {
-    struct msi_msg msg;
-    unsigned int dest;
+    struct hpet_event_channel *ch = desc->action->dev_id;
+    struct msi_msg msg = ch->msi.msg;
 
-    dest = set_desc_affinity(desc, mask);
-    if (dest == BAD_APICID)
+    msg.dest32 = set_desc_affinity(desc, mask);
+    if ( msg.dest32 == BAD_APICID )
         return;
 
-    hpet_msi_read(desc->action->dev_id, &msg);
     msg.data &= ~MSI_DATA_VECTOR_MASK;
     msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
     msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
-    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
-    msg.dest32 = dest;
-    hpet_msi_write(desc->action->dev_id, &msg);
+    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
+    if ( msg.data != ch->msi.msg.data || msg.dest32 != ch->msi.msg.dest32
)
+        hpet_msi_write(ch, &msg);
 }
 
 /*





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

On 25/10/2012 04:53, "Jan Beulich" <JBeulich@suse.com> wrote:
> Rather than spending measurable amounts of time reading back the most
> recently written message, cache it in space previously unused, and thus
> accelerate the CPU''s entering of the intended C-state.
> 
> hpet_msi_read() ends up being unused after this change, but rather than
> removing the function, it''s being marked "unused" in
order - that way
> it can easily get used again should a new need for it arise.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> ---
> v2: Re-based on top of the previously sent patch "compiler.h
>     adjustments".
> 
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -253,17 +253,19 @@ static void hpet_msi_mask(struct irq_des
>  
>  static void hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg
> *msg)
>  {
> +    ch->msi.msg = *msg;
>      if ( iommu_intremap )
>          iommu_update_ire_from_msi(&ch->msi, msg);
>      hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
>      hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>  }
>  
> -static void hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg
*msg)
> +static void __maybe_unused
> +hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
>  {
>      msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
>      msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
> -    msg->address_hi = 0;
> +    msg->address_hi = MSI_ADDR_BASE_HI;
>      if ( iommu_intremap )
>          iommu_read_msi_from_ire(&ch->msi, msg);
>  }
> @@ -285,20 +287,19 @@ static void hpet_msi_ack(struct irq_desc
>  
>  static void hpet_msi_set_affinity(struct irq_desc *desc, const cpumask_t
> *mask)
>  {
> -    struct msi_msg msg;
> -    unsigned int dest;
> +    struct hpet_event_channel *ch = desc->action->dev_id;
> +    struct msi_msg msg = ch->msi.msg;
>  
> -    dest = set_desc_affinity(desc, mask);
> -    if (dest == BAD_APICID)
> +    msg.dest32 = set_desc_affinity(desc, mask);
> +    if ( msg.dest32 == BAD_APICID )
>          return;
>  
> -    hpet_msi_read(desc->action->dev_id, &msg);
>      msg.data &= ~MSI_DATA_VECTOR_MASK;
>      msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
>      msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
> -    msg.address_lo |= MSI_ADDR_DEST_ID(dest);
> -    msg.dest32 = dest;
> -    hpet_msi_write(desc->action->dev_id, &msg);
> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
> +    if ( msg.data != ch->msi.msg.data || msg.dest32 !=
ch->msi.msg.dest32 )
> +        hpet_msi_write(ch, &msg);
>  }
>  
>  /*
> 
> 
>