* [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle
@ 2013-09-12 17:00 suravee.suthikulpanit
2013-09-13 9:24 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: suravee.suthikulpanit @ 2013-09-12 17:00 UTC (permalink / raw)
To: JBeulich; +Cc: Suravee Suthikulpanit, xen-devel
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
This patch handle additional cases for IVRS bugs where special->handle
is not correctly initialized for IOAPIC and HPETS due to firmware bugs.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Provide logic in "is_ioapic_overidden()"
Signed-off-by: Jan Beulich <JBeulich@suse.com>
---
Changes from V2:
- Add logic from Jan
- Also invalid handle for HPET
- Clean up old logics
xen/drivers/passthrough/amd/iommu_acpi.c | 60 +++++++++++++++++++++++++-------
1 file changed, 47 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 89b359c..0e63e35 100644
--- 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 != '=') )
return;
s = parse_pci(s + 1, &seg, &bus, &dev, &func);
if ( !s || *s )
return;
+ hpet_sbdf.id = id;
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)
+{
+ 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 )
{
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;
hpet_sbdf.iommu = iommu;
break;
default:
--
1.8.1.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle
2013-09-12 17:00 [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle suravee.suthikulpanit
@ 2013-09-13 9:24 ` Jan Beulich
2013-09-13 22:48 ` Suravee Suthikulpanit
2013-09-13 23:31 ` Suravee Suthikulpanit
0 siblings, 2 replies; 4+ messages in thread
From: Jan Beulich @ 2013-09-13 9:24 UTC (permalink / raw)
To: suravee.suthikulpanit; +Cc: xen-devel
>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@amd.com> 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.
> 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.
> 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.
> +{
> + 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.
> {
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle
2013-09-13 9:24 ` Jan Beulich
@ 2013-09-13 22:48 ` Suravee Suthikulpanit
2013-09-13 23:31 ` Suravee Suthikulpanit
1 sibling, 0 replies; 4+ messages in thread
From: Suravee Suthikulpanit @ 2013-09-13 22:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 9/13/2013 4:24 AM, Jan Beulich wrote:
>>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@amd.com> 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 send out V4 with updates, and thank you for your patience.
Suravee
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle
2013-09-13 9:24 ` Jan Beulich
2013-09-13 22:48 ` Suravee Suthikulpanit
@ 2013-09-13 23:31 ` Suravee Suthikulpanit
1 sibling, 0 replies; 4+ messages in thread
From: Suravee Suthikulpanit @ 2013-09-13 23:31 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 9/13/2013 4:24 AM, Jan Beulich wrote:
>>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@amd.com> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-13 23:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-12 17:00 [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle suravee.suthikulpanit
2013-09-13 9:24 ` Jan Beulich
2013-09-13 22:48 ` Suravee Suthikulpanit
2013-09-13 23:31 ` Suravee Suthikulpanit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.