[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode



> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, March 9, 2020 4:59 PM
> 
> On 28.02.2020 13:12, Jan Beulich wrote:
> > The wider cluster mode APIC IDs aren't generally representable. Convert
> > the iommu_intremap variable into a tristate, allowing the AMD IOMMU
> > driver to signal this special restriction to the apic_x2apic_probe().
> > (Note: assignments to the variable get adjusted, while existing
> > consumers - all assuming a boolean property - are left alone.)
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Kevin,
> 
> I see you've looked over and acked Xen patches over the weekend. Did
> you miss this one? It doesn't look controversial, so it'd be nice to
> have your ack, for it to go in.
> 
> Jan

It is in my list. My review process was interrupted by other tasks
yesterday, and now is resumed. 😊

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> > --- a/xen/arch/x86/genapic/x2apic.c
> > +++ b/xen/arch/x86/genapic/x2apic.c
> > @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
> >          x2apic_phys = !iommu_intremap ||
> >                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> >      }
> > -    else if ( !x2apic_phys && !iommu_intremap )
> > -    {
> > -        printk("WARNING: x2APIC cluster mode is not supported without
> interrupt remapping\n"
> > -               "x2APIC: forcing phys mode\n");
> > -        x2apic_phys = true;
> > -    }
> > +    else if ( !x2apic_phys )
> > +        switch ( iommu_intremap )
> > +        {
> > +        case iommu_intremap_off:
> > +        case iommu_intremap_restricted:
> > +            printk("WARNING: x2APIC cluster mode is not supported %s
> interrupt remapping\n"
> > +                   "x2APIC: forcing phys mode\n",
> > +                   iommu_intremap == iommu_intremap_off ? "without"
> > +                                                        : "with 
> > restricted");
> > +            x2apic_phys = true;
> > +            break;
> > +
> > +        case iommu_intremap_full:
> > +            break;
> > +        }
> >
> >      if ( x2apic_phys )
> >          return &apic_x2apic_phys;
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -1139,7 +1139,7 @@ static void __init amd_iommu_init_cleanu
> >
> >      iommu_enabled = 0;
> >      iommu_hwdom_passthrough = false;
> > -    iommu_intremap = 0;
> > +    iommu_intremap = iommu_intremap_off;
> >      iommuv2_enabled = 0;
> >  }
> >
> > @@ -1413,6 +1413,9 @@ int __init amd_iommu_prepare(bool xt)
> >          iommu->ctrl.int_cap_xt_en = xt && has_xt;
> >      }
> >
> > +    if ( iommu_intremap && !has_xt )
> > +        iommu_intremap = iommu_intremap_restricted;
> > +
> >      rc = amd_iommu_update_ivrs_mapping_acpi();
> >
> >   error_out:
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -157,7 +157,7 @@ int __init acpi_ivrs_init(void)
> >
> >      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
> >      {
> > -        iommu_intremap = 0;
> > +        iommu_intremap = iommu_intremap_off;
> >          return -ENODEV;
> >      }
> >
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -35,7 +35,7 @@ bool __read_mostly iommu_quarantine = tr
> >  bool_t __read_mostly iommu_igfx = 1;
> >  bool_t __read_mostly iommu_snoop = 1;
> >  bool_t __read_mostly iommu_qinval = 1;
> > -bool_t __read_mostly iommu_intremap = 1;
> > +enum iommu_intremap __read_mostly iommu_intremap =
> iommu_intremap_full;
> >  bool_t __read_mostly iommu_crash_disable;
> >
> >  static bool __hwdom_initdata iommu_hwdom_none;
> > @@ -91,7 +91,7 @@ static int __init parse_iommu_param(cons
> >          else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
> >              iommu_qinval = val;
> >          else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
> > -            iommu_intremap = val;
> > +            iommu_intremap = val ? iommu_intremap_full :
> iommu_intremap_off;
> >          else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
> >              iommu_intpost = val;
> >  #ifdef CONFIG_KEXEC
> > @@ -475,7 +475,7 @@ int __init iommu_setup(void)
> >          iommu_enabled = (rc == 0);
> >      }
> >      if ( !iommu_enabled )
> > -        iommu_intremap = 0;
> > +        iommu_intremap = iommu_intremap_off;
> >
> >      if ( (force_iommu && !iommu_enabled) ||
> >           (force_intremap && !iommu_intremap) )
> > @@ -557,7 +557,8 @@ void iommu_crash_shutdown(void)
> >
> >      if ( iommu_enabled )
> >          iommu_get_ops()->crash_shutdown();
> > -    iommu_enabled = iommu_intremap = iommu_intpost = 0;
> > +    iommu_enabled = iommu_intpost = 0;
> > +    iommu_intremap = iommu_intremap_off;
> >  }
> >
> >  int iommu_get_reserved_device_memory(iommu_grdm_t *func, void
> *ctxt)
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -2177,7 +2177,7 @@ static int __must_check init_vtd_hw(void
> >          {
> >              if ( ioapic_to_iommu(IO_APIC_ID(apic)) == NULL )
> >              {
> > -                iommu_intremap = 0;
> > +                iommu_intremap = iommu_intremap_off;
> >                  dprintk(XENLOG_ERR VTDPREFIX,
> >                      "ioapic_to_iommu: ioapic %#x (id: %#x) is NULL! "
> >                      "Will not try to enable Interrupt Remapping.\n",
> > @@ -2193,7 +2193,7 @@ static int __must_check init_vtd_hw(void
> >              iommu = drhd->iommu;
> >              if ( enable_intremap(iommu, 0) != 0 )
> >              {
> > -                iommu_intremap = 0;
> > +                iommu_intremap = iommu_intremap_off;
> >                  dprintk(XENLOG_WARNING VTDPREFIX,
> >                          "Interrupt Remapping not enabled\n");
> >
> > @@ -2295,7 +2295,7 @@ static int __init vtd_setup(void)
> >              iommu_qinval = 0;
> >
> >          if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
> > -            iommu_intremap = 0;
> > +            iommu_intremap = iommu_intremap_off;
> >
> >          /*
> >           * We cannot use posted interrupt if X86_FEATURE_CX16 is
> > @@ -2320,7 +2320,7 @@ static int __init vtd_setup(void)
> >
> >      if ( !iommu_qinval && iommu_intremap )
> >      {
> > -        iommu_intremap = 0;
> > +        iommu_intremap = iommu_intremap_off;
> >          dprintk(XENLOG_WARNING VTDPREFIX, "Interrupt Remapping
> disabled "
> >              "since Queued Invalidation isn't supported or enabled.\n");
> >      }
> > @@ -2347,7 +2347,7 @@ static int __init vtd_setup(void)
> >      iommu_snoop = 0;
> >      iommu_hwdom_passthrough = false;
> >      iommu_qinval = 0;
> > -    iommu_intremap = 0;
> > +    iommu_intremap = iommu_intremap_off;
> >      iommu_intpost = 0;
> >      return ret;
> >  }
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -54,7 +54,18 @@ static inline bool_t dfn_eq(dfn_t x, dfn
> >
> >  extern bool_t iommu_enable, iommu_enabled;
> >  extern bool force_iommu, iommu_quarantine, iommu_verbose,
> iommu_igfx;
> > -extern bool_t iommu_snoop, iommu_qinval, iommu_intremap,
> iommu_intpost;
> > +extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
> > +extern enum __packed iommu_intremap {
> > +   /*
> > +    * In order to allow traditional boolean uses of the iommu_intremap
> > +    * variable, the "off" value has to come first (yielding a value of 
> > zero).
> > +    */
> > +   iommu_intremap_off,
> > +#ifdef CONFIG_X86
> > +   iommu_intremap_restricted,
> > +#endif
> > +   iommu_intremap_full,
> > +} iommu_intremap;
> >
> >  #if defined(CONFIG_IOMMU_FORCE_PT_SHARE)
> >  #define iommu_hap_pt_share true
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/xen-devel
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.