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

[Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>]

On 9/4/2013 4:57 AM, Jan Beulich wrote:
>>>> On 30.08.13 at 22:35, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> wrote:
>> On 8/30/2013 3:06 AM, Jan Beulich wrote:
>>>>>> On 29.08.13 at 22:26, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> 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.
Yes, I agree that the current code here is handling the CASE1 mentioned 
below, but not the CASE3,
  which I am trying to handle.
>
>> 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.
Just to make sure.  For example, in the case below:

IOAPIC 0: handle = 0x0: bdf=00:14.0   //good
IOAPIC 1: handle = 0x0: bdf=00:00.1   //bad handle due to duplication

Here, user should specify "ivrs_ioapic[00:00.1]=0x1" which will override 
IOAPIC 1 and keep the IOAPIC 0.
Although, I have never seen the case where bdf is duplicated.

>> 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).
We should be able to identify the invalid one by checking if the handle 
is not in the APIC table, in which case, we will not be using the value 
in the IVRS.
If users gives option "ivrs_ioapic[bb:dd.f]=<handle>", then just use 
that one.
> 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).
I still don't think we should have two version of the same command. This 
would only make it more confusing.  For example,
IOAPIC 0: handle = 0x00: bdf=00:14.0   //good
IOAPIC 1: handle = 0xff: bdf=00:00.1    //bad since the value 0xff is 
not in the APIC table.
Here, this would be sufficient to just specify option 
"ivrs_ioapic[00:00.1]=0x1".
> 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.
See above.
> 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.
I think we can just ignore this case.
> 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>
After looking through the examples above, I still think we only need the 
"ivrs_ioapic[<sbdf>]=<id>" version.

Suravee