[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle
On 9/13/2013 4:24 AM, Jan Beulich wrote:
On 12.09.13 at 19:00, <suravee.suthikulpanit@xxxxxxx> wrote:
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
ASSERT(*s == '[');
id = simple_strtoul(s + 1, &s, 0);
- if ( id != (typeof(hpet_sbdf.id))id || *s != ']' || *++s != '=' )
+ if ( (*s != ']') || (*++s != '=') )
No, unless you have a very good reason.
Oh, sorry. This is my mistake.
return;
s = parse_pci(s + 1, &seg, &bus, &dev, &func);
if ( !s || *s )
return;
+ hpet_sbdf.id = id;
In essence this is the only change not contained in the patch I sent.
So I'd be inclined to commit this (and perhaps the one debug
message adjustment below) under your name, and my proposed
change as a separate one.
Ok.
hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
hpet_sbdf.seg = seg;
hpet_sbdf.cmdline = 1;
}
custom_param("ivrs_hpet[", parse_ivrs_hpet);
+static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)
Missing __init annotation. And anyway, I can't really see why
putting this in a separate function is a significant benefit. It's
only being used in one place afaics.
Sorry, I forgot the __init. I normally just like to keep to code that
does a specific
thing inside it's own function for the ease of reading. But this is
purely coding style.
I can put it in the caller function if you prefer.
+{
+ bool_t ret = 0;
+ int apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
+
+ while ( apic < ARRAY_SIZE(ioapic_sbdf) )
+ {
+ if ( ioapic_sbdf[apic].bdf == bdf &&
+ ioapic_sbdf[apic].seg == seg )
+ break;
+ apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
+ apic + 1);
+ }
+
+ if ( apic < ARRAY_SIZE(ioapic_sbdf) )
+ {
+ AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x "
+ "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
+ apic, handle, seg, PCI_BUS(bdf),
+ PCI_SLOT(bdf), PCI_FUNC(bdf));
+ ret = 1;
+ }
+
+ return ret;
+}
+
static u16 __init parse_ivhd_device_special(
const struct acpi_ivrs_device8c *special, u16 seg,
u16 header_length, u16 block_length, struct amd_iommu *iommu)
@@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special(
return 0;
}
- AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle %#x\n",
+ AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle %#x used_id
%#x\n",
seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
- special->variety, special->handle);
+ special->variety, special->handle, special->used_id);
add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu);
switch ( special->variety )
{
case ACPI_IVHD_IOAPIC:
- if ( !iommu_intremap )
+ if ( !iommu_intremap ||
+ is_ioapic_overidden(seg, bdf, special->handle) )
break;
+
/*
* Some BIOSes have IOAPIC broken entries so we check for IVRS
* consistency here --- whether entry's IOAPIC ID is valid and
@@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
return 0;
}
- if ( test_bit(special->handle, ioapic_cmdline) )
- AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC
%#x\n",
- special->handle);
- else if ( ioapic_sbdf[special->handle].pin_2_idx )
+ if ( ioapic_sbdf[special->handle].pin_2_idx )
Again - no, unless you have a very good reason.
I think with the check for command line stuff already handle prior to
this point,
I don't see why we still need to keep this here. It should not need to
go through
the logic here.
{
if ( ioapic_sbdf[special->handle].bdf == bdf &&
ioapic_sbdf[special->handle].seg == seg )
@@ -770,6 +796,16 @@ static u16 __init parse_ivhd_device_special(
}
break;
case ACPI_IVHD_HPET:
+ if ( hpet_sbdf.cmdline )
+ {
+ AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET %#x "
+ "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
+ hpet_sbdf.id, special->handle, seg, PCI_BUS(bdf),
+ PCI_SLOT(bdf), PCI_FUNC(bdf));
+ hpet_sbdf.iommu = iommu;
+ break;
+ }
+
/* set device id of hpet */
if ( hpet_sbdf.iommu ||
(hpet_sbdf.cmdline && hpet_sbdf.id != special->handle) )
@@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
printk(XENLOG_WARNING "Only one IVHD HPET entry is supported\n");
break;
}
+
hpet_sbdf.id = special->handle;
- if ( !hpet_sbdf.cmdline )
- {
- hpet_sbdf.bdf = bdf;
- hpet_sbdf.seg = seg;
- }
+ hpet_sbdf.bdf = bdf;
+ hpet_sbdf.seg = seg;
I don't see what benefit these HPET related changes provide. Or
if there is any that I overlook, then the previous uses of
hpet_sbdf.cmdline should all be eliminated.
Jan
I found an issue when I was testing. if the "handle" for the IVRS HPET
is not the same where:
- IVRS Table HPET handle = 0x0
- HPET Table = 0x2
This throws the following error message:
(XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET
In this case, the handle is wrong and I am trying to change it.
When I am trying to specify the command line option "ivrs_hpet[2]=00:14.0"
this does not get used because the in the parse_ivrs_hpet() did not
store the
override id. So, I modify the logic in the parse_ivrs_hpet() to
override the hpet_sbdf.bdf
with the new ID specified from command line. Then it doesn't need to go
though this logic here.
Also, the check "hpet_sbdf.cmdline && hpet_sbdf.id != special->handle"
should also be eliminated.
I will split the patch send out V4 with updates, and thank you for your
patience.
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
