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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 28, 2020 at 01:39:59PM +0100, Jan Beulich wrote:
> On 28.02.2020 13:31, Roger Pau Monné wrote:
> > On Fri, Feb 28, 2020 at 01:12:03PM +0100, Jan Beulich wrote:
> >> --- 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;
> > 
> > I think you also need to fixup the usage of iommu_intremap in __cpu_up
> > so that CPUs with APIC IDs > 255 are not brought up when in
> > iommu_intremap_restricted mode.
> 
> That certainly wants changing, yes, but I view this as an orthogonal
> adjustment, which I'd like to make only once I understand what the
> behavior for APIC ID 0xff should be in this setup.

I would say APIC ID 0xff should be the broadcast ID, or else remapped
interrupts won't be able to use a broadcast destination? I'm however
not able to find any mention to this in the AMD-Vi spec.

So the check in __cpu_up should be adjusted to iommu_intremap !=
iommu_intremap_full I think, or else you won't be able to address the
CPUs brought up from the interrupt remapping tables.

Anyway, the change looks fine, so:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.

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