Re: [Xen-devel] [PATCH V2] x86/AMD-Vi: Add additional check for invalid special->handle

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.08.13 at 22:35, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
wrote:
> On 8/30/2013 3:06 AM, Jan Beulich wrote:
>>>>> On 29.08.13 at 22:26, Suravee Suthikulpanit 
>>>>> <suravee.suthikulpanit@xxxxxxx> 
> wrote:
>>> On 8/29/2013 2:17 AM, Jan Beulich wrote:
>>>> As said in a previous reply - I'm convinced we can't fix things both
>>>> ways with just a single command line option: We need to know
>>>> which value to use as anchor (I'd generally think that ought to be
>>>> the value inside the square backets) and which value to use as
>>>> overrides.
>>>>
>>>> And since my earlier patch was inspired by the Linux one - I don't
>>>> think Linux allows fixing up things the way you try to here either.
>>>>
>>> Actually, on Linux, the it would try matching the handle (i.e. the value
>>> in the square bracket).  If it is specified via command line, it will
>>> override the value in the IVRS.
>> Right - that's matching the behavior of my patch. Or am I missing
>> something here?
> I believe in your patch is slightly different. In your version, it has 
> the following check
> in the driver/passthrough/amd/iommu_acpi.c:parse_ivhd_device_special().
> 
>          */
>          for ( apic = 0; apic < nr_ioapics; apic++ )
>          {
>              if ( IO_APIC_ID(apic) != special->handle )
>                  continue;
>               .....
> 
> First, the code tries to match the IO_APIC_ID(apic) with the 
> special->handle.  If none is matched,
> it will go directly to the exiting code (see below) without trying to 
> check the command line.
> 
>          if ( apic == nr_ioapics )
>          {
>              printk(XENLOG_ERR "IVHD Error: Invalid IO-APIC %#x\n",
>                     special->handle);
>              return 0;
>          }
>          break;
> 
> This is different on Linux where it check if the value it is trying to 
> matched is coming from the command line.

But again - there being an ID on the command line that doesn't
have any match in IVHD is being taken care of by the adjustment
to parse_ivrs_table(). At least afaict.

> Let's clarify the cases we are trying to handle here:
> 
> CASE1: IOAPIC handle ismissing in IVRS, but exist in APIC table
> Users should specify the missing IOAPIC  using ioapic_ivrs[<handle>]=bdf
> 
> CASE2: IOAPIC handleare duplicated in IVRS entries  
> This case,the code already check for duplications inIVRS. Here,we cannot 
> trust
> any existing entries in the IVRS,

One of them might still be right, and hence not need overriding.

> and  we  shouldonly rely 
> ontheioapic_ivrs[<handle>]=bdf  
> options for all  IOAPICs.
> 
> CASE3: IOAPIChandle are Bogus (i.e. 0xff ornot  existing in APIC table)
> We cannot trust the entry with bogus handle, and users would need to specify 
> the ioapic_ivrs option.
> 
> Which casedo you thinkwould require the second command line option  which we 
> wouldanchor the BDF?

Case 3 in any event would need not only specification of valid entries
on the command line, but also a way to identify the invalid ones (to
avoid parse_ivhd_device_special() returning 0, which is its failure
indicator). As long as the handle here is invalid, but devid is correct,
that would be a case where we'd need a devid-anchored command
line option (or some other second command line option identifying the
IVRS entries needing to be ignored).

Since for case 2 the duplicate handles may again be associated with
valid devid-s, the same would apply there, except we'd not need
invalidation, but just overriding of the handles. Which would again
most easily be done by a devid-anchored command line option.

Case 4 really is where things don't matter: If some entries have
neither a valid handle nor a valid devid, then overriding them can
be done in either way (but some mechanism to identify which ones
they are in order to ignore them would still be needed). I wonder,
however, whether that's a case we indeed need to worry about:
If everything the firmware tells us is a lie, we'd better not do
anything fancy on such a system.

It would, btw, be possible to get along with a single command line
option, just having two alternating forms:

ivrs_ioapic[id]=<sbdf>
ivrs_ioapic[<sbdf>]=<id>

Since parse_pci() tells us whether the input was a valid PCI
device specification, we could try that on the input string first,
and only upon failure use the current code (expecting the ID as
index). But of course the resulting tracking structure would still
need to identify the piece of information to trust and the one to
override.

Jan

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