Re: [Xen-devel] [PATCH RFC 2/2] AMD IOMMU: allow command line overrides for broken IVRS tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.13 at 16:59, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx> 
>>> wrote:
> The current patch does not handle the case where the "handle" value is 
> mis-configured in the IVRS.

Right, but I don't think we can handle all possible cases here.

And in any case (not the least because your patch seems to be
incremental to my earlier one) - do you still see any issues with that
one, or does the mail here represent sort of an ack to that earlier
one (in its split form - as indicated earlier, I broke out the actual
bug fixes from the workaround, which really is an enhancement)?

> I have also included the patch which has additional check.  Below is the 
> output from the patch.
> 
> (XEN) AMD-Vi: IVHD Device Entry: type 0x48 id 0 flags 0xd7
> (XEN) AMD-Vi: IVHD Special: 0000:00:14.0 variety 0x1 handle 0xff used_id 
> 0xa0
> (XEN) AMD-Vi: IVHD Special: Usinging command override value for IO-APIC 0x8, 
> bdf=0xa0
> (XEN) AMD-Vi: IVHD Device Entry: type 0x48 id 0 flags 0xd7
> (XEN) AMD-Vi: IVHD Special: 0000:00:14.0 variety 0x2 handle 0 used_id 0xa0
> (XEN) AMD-Vi: IVHD Device Entry: type 0x48 id 0 flags 0
> (XEN) AMD-Vi: IVHD Special: 0000:00:00.1 variety 0x1 handle 0xff used_id 0x1
> (XEN) AMD-Vi: IVHD Special: Usinging command override value for IO-APIC 0x8, 
> bdf=0xa0
> (XEN) AMD-Vi: IOMMU 0 Enabled.
> (XEN) I/O virtualisation enabled

Which already indicates that you apparently only considered the
single IO-APIC case, because on a multi IO-APIC system ...

> @@ -715,29 +717,43 @@ static u16 __init parse_ivhd_device_special(
>            */
>           for ( apic = 0; apic < nr_ioapics; apic++ )
>           {
> -            if ( IO_APIC_ID(apic) != special->handle )
> -                continue;
> +            apic_id = special->handle;
> 
> -            if ( special->handle >= ARRAY_SIZE(ioapic_sbdf) )
> +            if ( IO_APIC_ID(apic) != apic_id )

... this would otherwise trigger on all IO-APICs but the one we're
currently supposed to deal with ...

> +            {
> +                /* Some BIOSes have invalid value in the special->handle.
> +                 * Here we check to see if the user is overiding them from 
> commandline
> +                 */
> +                if ( test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
> +                {
> +                    apic_id = IO_APIC_ID(apic);
> +                    AMD_IOMMU_DEBUG ("IVHD Special: Usinging command 
> override "
> +                        "value for IO-APIC %#x, bdf=%#x\n",
> +                        apic_id, ioapic_sbdf[apic_id].bdf);
> +                }

... and hence if overridden each IVRS entry - correct or not -
would get overruled here.

Jan


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