[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 9/4/2013 4:57 AM, Jan Beulich wrote:
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.
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


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.