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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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