Re: [Xen-devel] [PATCH v2 2/3] x86/HPET: allow use for broadcast when interrupt remapping is in effect

["Zhang, Xiantao" <xiantao.zhang@xxxxxxxxx>]

Acked-by: Xiantao Zhang<xiantao.zhang@xxxxxxxxx>

> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx] 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@xxxxxxxx> 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@xxxxxxxx>
> 
> Acked-by: Keir Fraser <keir@xxxxxxx>
> 
> ...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@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
> 


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