* [PATCH 1/2] Intel-IOMMU, intr-remap: set the whole 128bits of irte when modify/free it
@ 2009-05-07 6:16 Weidong Han
2009-05-07 6:16 ` [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking Weidong Han
0 siblings, 1 reply; 11+ messages in thread
From: Weidong Han @ 2009-05-07 6:16 UTC (permalink / raw)
To: dwmw2, suresh.b.siddha, mingo; +Cc: linux-kernel, iommu, kvm, Weidong Han
Interrupt remapping table entry is 128bits. Currently, it only sets low
64bits of irte in modify_irte and free_irte. This ignores high 64bits
setting of irte, that means source-id setting will be ignored. This patch
sets the whole 128bits of irte when modify/free it. Following source-id
checking patch depends on this.
Signed-off-by: Weidong Han <weidong.han@intel.com>
---
drivers/pci/intr_remapping.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c
index f5e0ea7..946e170 100644
--- a/drivers/pci/intr_remapping.c
+++ b/drivers/pci/intr_remapping.c
@@ -309,7 +309,8 @@ int modify_irte(int irq, struct irte *irte_modified)
index = irq_iommu->irte_index + irq_iommu->sub_handle;
irte = &iommu->ir_table->base[index];
- set_64bit((unsigned long *)irte, irte_modified->low);
+ set_64bit((unsigned long *)&irte->low, irte_modified->low);
+ set_64bit((unsigned long *)&irte->high, irte_modified->high);
__iommu_flush_cache(iommu, irte, sizeof(*irte));
rc = qi_flush_iec(iommu, index, 0);
@@ -386,8 +387,11 @@ int free_irte(int irq)
irte = &iommu->ir_table->base[index];
if (!irq_iommu->sub_handle) {
- for (i = 0; i < (1 << irq_iommu->irte_mask); i++)
- set_64bit((unsigned long *)(irte + i), 0);
+ for (i = 0; i < (1 << irq_iommu->irte_mask); i++) {
+ set_64bit((unsigned long *)&irte->low, 0);
+ set_64bit((unsigned long *)&irte->high, 0);
+ irte++;
+ }
rc = qi_flush_iec(iommu, index, irq_iommu->irte_mask);
}
--
1.6.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-07 6:16 [PATCH 1/2] Intel-IOMMU, intr-remap: set the whole 128bits of irte when modify/free it Weidong Han
@ 2009-05-07 6:16 ` Weidong Han
2009-05-07 18:19 ` Suresh Siddha
0 siblings, 1 reply; 11+ messages in thread
From: Weidong Han @ 2009-05-07 6:16 UTC (permalink / raw)
To: dwmw2, suresh.b.siddha, mingo; +Cc: linux-kernel, iommu, kvm, Weidong Han
To support domain-isolation usages, the platform hardware must be
capable of uniquely identifying the requestor (source-id) for each
interrupt message. Without source-id checking for interrupt remapping
, a rouge guest/VM with assigned devices can launch interrupt attacks
to bring down anothe guest/VM or the VMM itself.
This patch adds source-id checking for interrupt remapping, and then
really isolates interrupts for guests/VMs with assigned devices.
Signed-off-by: Weidong Han <weidong.han@intel.com>
---
arch/x86/kernel/apic/io_apic.c | 6 +++
drivers/pci/intr_remapping.c | 98 ++++++++++++++++++++++++++++++++++++++++
drivers/pci/intr_remapping.h | 2 +
include/linux/dmar.h | 11 +++++
4 files changed, 117 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 30da617..3d10c68 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1559,6 +1559,9 @@ int setup_ioapic_entry(int apic_id, int irq,
irte.vector = vector;
irte.dest_id = IRTE_DEST(destination);
+ /* Set source-id of interrupt request */
+ set_ioapic_sid(&irte, apic_id);
+
modify_irte(irq, &irte);
ir_entry->index2 = (index >> 15) & 0x1;
@@ -3329,6 +3332,9 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, struct msi_ms
irte.vector = cfg->vector;
irte.dest_id = IRTE_DEST(dest);
+ /* Set source-id of interrupt request */
+ set_msi_sid(&irte, pdev);
+
modify_irte(irq, &irte);
msg->address_hi = MSI_ADDR_BASE_HI;
diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c
index 946e170..825bca2 100644
--- a/drivers/pci/intr_remapping.c
+++ b/drivers/pci/intr_remapping.c
@@ -10,6 +10,7 @@
#include <linux/intel-iommu.h>
#include "intr_remapping.h"
#include <acpi/acpi.h>
+#include "pci.h"
static struct ioapic_scope ir_ioapic[MAX_IO_APICS];
static int ir_ioapic_num;
@@ -405,6 +406,61 @@ int free_irte(int irq)
return rc;
}
+int set_ioapic_sid(struct irte *irte, int apic)
+{
+ int i;
+ u16 sid = 0;
+
+ if (!irte)
+ return -1;
+
+ for (i = 0; i < MAX_IO_APICS; i++)
+ if (ir_ioapic[i].id == apic) {
+ sid = (ir_ioapic[i].bus << 8) | ir_ioapic[i].devfn;
+ break;
+ }
+
+ if (sid == 0) {
+ printk(KERN_WARNING "Failed to set source-id of "
+ "I/O APIC (%d), because it is not under "
+ "any DRHD\n", apic);
+ return -1;
+ }
+
+ irte->svt = 1; /* requestor ID verification SID/SQ */
+ irte->sq = 0; /* comparing all 16-bit of SID */
+ irte->sid = sid;
+
+ return 0;
+}
+
+int set_msi_sid(struct irte *irte, struct pci_dev *dev)
+{
+ struct pci_dev *tmp;
+
+ if (!irte || !dev)
+ return -1;
+
+ tmp = pci_find_upstream_pcie_bridge(dev);
+ if (!tmp) { /* PCIE device or integrated PCI device */
+ irte->svt = 1; /* verify requestor ID verification SID/SQ */
+ irte->sq = 0; /* comparing all 16-bit of SID */
+ irte->sid = (dev->bus->number << 8) | dev->devfn;
+ return 0;
+ }
+
+ if (tmp->is_pcie) { /* this is a PCIE-to-PCI bridge */
+ irte->svt = 2; /* verify request ID verification SID */
+ irte->sid = (tmp->bus->number << 8) | dev->bus->number;
+ } else { /* this is a legacy PCI bridge */
+ irte->svt = 1; /* verify requestor ID verification SID/SQ */
+ irte->sq = 0; /* comparing all 16-bit of SID */
+ irte->sid = (tmp->bus->number << 8) | tmp->devfn;
+ }
+
+ return 0;
+}
+
static void iommu_set_intr_remapping(struct intel_iommu *iommu, int mode)
{
u64 addr;
@@ -616,6 +672,10 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
struct acpi_dmar_hardware_unit *drhd;
struct acpi_dmar_device_scope *scope;
void *start, *end;
+ struct acpi_dmar_pci_path *path;
+ struct pci_bus *bus;
+ struct pci_dev *pdev = NULL;
+ int count;
drhd = (struct acpi_dmar_hardware_unit *)header;
@@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
" 0x%Lx\n", scope->enumeration_id,
drhd->address);
+ bus = pci_find_bus(drhd->segment, scope->bus);
+ path = (struct acpi_dmar_pci_path *)(scope + 1);
+ count = (scope->length -
+ sizeof(struct acpi_dmar_device_scope))
+ / sizeof(struct acpi_dmar_pci_path);
+
+ while (count) {
+ if (pdev)
+ pci_dev_put(pdev);
+
+ if (!bus)
+ break;
+
+ pdev = pci_get_slot(bus,
+ PCI_DEVFN(path->dev, path->fn));
+ if (!pdev)
+ break;
+
+ path++;
+ count--;
+ bus = pdev->subordinate;
+ }
+
+ if (pdev) { /* PCI discoverable IOAPIC*/
+ ir_ioapic[ir_ioapic_num].bus =
+ pdev->bus->number;
+ ir_ioapic[ir_ioapic_num].devfn = pdev->devfn;
+ } else { /* Not PCI discoverable IOAPIC */
+ if (!bus)
+ ir_ioapic[ir_ioapic_num].bus =
+ scope->bus;
+ else
+ ir_ioapic[ir_ioapic_num].bus =
+ bus->number;
+ ir_ioapic[ir_ioapic_num].devfn =
+ PCI_DEVFN(path->dev, path->fn);
+ }
+
ir_ioapic[ir_ioapic_num].iommu = iommu;
ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
ir_ioapic_num++;
diff --git a/drivers/pci/intr_remapping.h b/drivers/pci/intr_remapping.h
index ca48f0d..dd35780 100644
--- a/drivers/pci/intr_remapping.h
+++ b/drivers/pci/intr_remapping.h
@@ -3,6 +3,8 @@
struct ioapic_scope {
struct intel_iommu *iommu;
unsigned int id;
+ u8 bus; /* PCI bus number */
+ u8 devfn; /* PCI devfn number */
};
#define IR_X2APIC_MODE(mode) (mode ? (1 << 11) : 0)
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index e397dc3..7d27284 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -125,6 +125,8 @@ extern int free_irte(int irq);
extern int irq_remapped(int irq);
extern struct intel_iommu *map_dev_to_ir(struct pci_dev *dev);
extern struct intel_iommu *map_ioapic_to_ir(int apic);
+extern int set_ioapic_sid(struct irte *irte, int apic);
+extern int set_msi_sid(struct irte *irte, struct pci_dev *dev);
#else
static inline int alloc_irte(struct intel_iommu *iommu, int irq, u16 count)
{
@@ -155,6 +157,15 @@ static inline struct intel_iommu *map_ioapic_to_ir(int apic)
{
return NULL;
}
+static inline int set_ioapic_sid(struct irte *irte, int apic)
+{
+ return 0;
+}
+static inline int set_msi_sid(struct irte *irte, struct pci_dev *dev)
+{
+ return 0;
+}
+
#define irq_remapped(irq) (0)
#define enable_intr_remapping(mode) (-1)
#define intr_remapping_enabled (0)
--
1.6.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-07 6:16 ` [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking Weidong Han
@ 2009-05-07 18:19 ` Suresh Siddha
2009-05-11 6:22 ` Han, Weidong
0 siblings, 1 reply; 11+ messages in thread
From: Suresh Siddha @ 2009-05-07 18:19 UTC (permalink / raw)
To: Han, Weidong; +Cc: dwmw2, mingo, linux-kernel, iommu, kvm
On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
> " 0x%Lx\n", scope->enumeration_id,
> drhd->address);
>
> + bus = pci_find_bus(drhd->segment, scope->bus);
> + path = (struct acpi_dmar_pci_path *)(scope + 1);
> + count = (scope->length -
> + sizeof(struct acpi_dmar_device_scope))
> + / sizeof(struct acpi_dmar_pci_path);
> +
> + while (count) {
> + if (pdev)
> + pci_dev_put(pdev);
> +
> + if (!bus)
> + break;
> +
> + pdev = pci_get_slot(bus,
> + PCI_DEVFN(path->dev, path->fn));
> + if (!pdev)
> + break;
ir_parse_ioapic_scope() happens very early in the boot. So, I don't
think we can do the pci related discovery here.
thanks,
suresh
> +
> + path++;
> + count--;
> + bus = pdev->subordinate;
> + }
> +
> + if (pdev) { /* PCI discoverable IOAPIC*/
> + ir_ioapic[ir_ioapic_num].bus =
> + pdev->bus->number;
> + ir_ioapic[ir_ioapic_num].devfn = pdev->devfn;
> + } else { /* Not PCI discoverable IOAPIC */
> + if (!bus)
> + ir_ioapic[ir_ioapic_num].bus =
> + scope->bus;
> + else
> + ir_ioapic[ir_ioapic_num].bus =
> + bus->number;
> + ir_ioapic[ir_ioapic_num].devfn =
> + PCI_DEVFN(path->dev, path->fn);
> + }
> +
> ir_ioapic[ir_ioapic_num].iommu = iommu;
> ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
> ir_ioapic_num++;
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-07 18:19 ` Suresh Siddha
@ 2009-05-11 6:22 ` Han, Weidong
2009-05-11 13:20 ` Ingo Molnar
0 siblings, 1 reply; 11+ messages in thread
From: Han, Weidong @ 2009-05-11 6:22 UTC (permalink / raw)
To: Siddha, Suresh B; +Cc: dwmw2, mingo, linux-kernel, iommu, kvm
Siddha, Suresh B wrote:
> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>> acpi_dmar_header *header, " 0x%Lx\n",
>> scope->enumeration_id, drhd->address);
>>
>> + bus = pci_find_bus(drhd->segment, scope->bus);
>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
>> + / sizeof(struct acpi_dmar_pci_path);
>> +
>> + while (count) {
>> + if (pdev)
>> + pci_dev_put(pdev);
>> +
>> + if (!bus)
>> + break;
>> +
>> + pdev = pci_get_slot(bus,
>> + PCI_DEVFN(path->dev, path->fn));
>> + if (!pdev)
>> + break;
>
> ir_parse_ioapic_scope() happens very early in the boot. So, I don't
> think we can do the pci related discovery here.
>
Thanks for your pointing it out. It should enable the source-id checking for io-apic's after the pci subsystem is up. I will change it.
Regards,
Weidong
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-11 6:22 ` Han, Weidong
@ 2009-05-11 13:20 ` Ingo Molnar
2009-05-18 9:46 ` Han, Weidong
0 siblings, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2009-05-11 13:20 UTC (permalink / raw)
To: Han, Weidong; +Cc: Siddha, Suresh B, dwmw2, linux-kernel, iommu, kvm
* Han, Weidong <weidong.han@intel.com> wrote:
> Siddha, Suresh B wrote:
> > On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
> >> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
> >> acpi_dmar_header *header, " 0x%Lx\n",
> >> scope->enumeration_id, drhd->address);
> >>
> >> + bus = pci_find_bus(drhd->segment, scope->bus);
> >> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
> >> (scope->length - + sizeof(struct acpi_dmar_device_scope))
> >> + / sizeof(struct acpi_dmar_pci_path);
> >> +
> >> + while (count) {
> >> + if (pdev)
> >> + pci_dev_put(pdev);
> >> +
> >> + if (!bus)
> >> + break;
> >> +
> >> + pdev = pci_get_slot(bus,
> >> + PCI_DEVFN(path->dev, path->fn));
> >> + if (!pdev)
> >> + break;
> >
> > ir_parse_ioapic_scope() happens very early in the boot. So, I
> > don't think we can do the pci related discovery here.
> >
>
> Thanks for your pointing it out. It should enable the source-id
> checking for io-apic's after the pci subsystem is up. I will
> change it.
Note, there's ways to do early PCI quirks too, check
arch/x86/kernel/early-quirks.c. It's done by reading the PCI
configuration space directly via a careful early-capable subset of
the PCI config space APIs.
But it's a method of last resort.
Ingo
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-11 13:20 ` Ingo Molnar
@ 2009-05-18 9:46 ` Han, Weidong
0 siblings, 0 replies; 11+ messages in thread
From: Han, Weidong @ 2009-05-18 9:46 UTC (permalink / raw)
To: 'Ingo Molnar'
Cc: Siddha, Suresh B, 'dwmw2@infradead.org',
'linux-kernel@vger.kernel.org',
'iommu@lists.linux-foundation.org',
'kvm@vger.kernel.org'
Ingo Molnar wrote:
> * Han, Weidong <weidong.han@intel.com> wrote:
>
>> Siddha, Suresh B wrote:
>>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>>>> acpi_dmar_header *header, " 0x%Lx\n",
>>>> scope->enumeration_id, drhd->address);
>>>>
>>>> + bus = pci_find_bus(drhd->segment, scope->bus);
>>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
>>>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
>>>> + / sizeof(struct acpi_dmar_pci_path);
>>>> +
>>>> + while (count) {
>>>> + if (pdev)
>>>> + pci_dev_put(pdev);
>>>> +
>>>> + if (!bus)
>>>> + break;
>>>> +
>>>> + pdev = pci_get_slot(bus,
>>>> + PCI_DEVFN(path->dev, path->fn));
>>>> + if (!pdev)
>>>> + break;
>>>
>>> ir_parse_ioapic_scope() happens very early in the boot. So, I
>>> don't think we can do the pci related discovery here.
>>>
>>
>> Thanks for your pointing it out. It should enable the source-id
>> checking for io-apic's after the pci subsystem is up. I will
>> change it.
>
> Note, there's ways to do early PCI quirks too, check
> arch/x86/kernel/early-quirks.c. It's done by reading the PCI
> configuration space directly via a careful early-capable subset of
> the PCI config space APIs.
>
> But it's a method of last resort.
>
Thanks for your reminder. It can use direct PCI access here as follows. It's easy and clean. I think it's better than adding the source-id checking for io-apic's after the pci subsystem is up. I will send out updated patches after some tests.
@@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
" 0x%Lx\n", scope->enumeration_id,
drhd->address);
+ bus = scope->bus;
+ path = (struct acpi_dmar_pci_path *)(scope + 1);
+ count = (scope->length -
+ sizeof(struct acpi_dmar_device_scope))
+ / sizeof(struct acpi_dmar_pci_path);
+
+ while (--count > 0) {
+ /* Access PCI directly due to the PCI
+ * subsystem isn't initialized yet.
+ */
+ bus = read_pci_config_byte(bus, path->dev,
+ path->fn, PCI_SECONDARY_BUS);
+ path++;
+ }
+
+ ir_ioapic[ir_ioapic_num].bus = bus;
+ ir_ioapic[ir_ioapic_num].devfn =
+ PCI_DEVFN(path->dev, path->fn);
ir_ioapic[ir_ioapic_num].iommu = iommu;
ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
ir_ioapic_num++;
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
@ 2009-05-18 9:46 ` Han, Weidong
0 siblings, 0 replies; 11+ messages in thread
From: Han, Weidong @ 2009-05-18 9:46 UTC (permalink / raw)
To: 'Ingo Molnar'
Cc: Siddha, Suresh B, 'dwmw2@infradead.org',
'linux-kernel@vger.kernel.org',
'iommu@lists.linux-foundation.org',
'kvm@vger.kernel.org'
Ingo Molnar wrote:
> * Han, Weidong <weidong.han@intel.com> wrote:
>
>> Siddha, Suresh B wrote:
>>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>>>> acpi_dmar_header *header, " 0x%Lx\n",
>>>> scope->enumeration_id, drhd->address);
>>>>
>>>> + bus = pci_find_bus(drhd->segment, scope->bus);
>>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
>>>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
>>>> + / sizeof(struct acpi_dmar_pci_path);
>>>> +
>>>> + while (count) {
>>>> + if (pdev)
>>>> + pci_dev_put(pdev);
>>>> +
>>>> + if (!bus)
>>>> + break;
>>>> +
>>>> + pdev = pci_get_slot(bus,
>>>> + PCI_DEVFN(path->dev, path->fn));
>>>> + if (!pdev)
>>>> + break;
>>>
>>> ir_parse_ioapic_scope() happens very early in the boot. So, I
>>> don't think we can do the pci related discovery here.
>>>
>>
>> Thanks for your pointing it out. It should enable the source-id
>> checking for io-apic's after the pci subsystem is up. I will
>> change it.
>
> Note, there's ways to do early PCI quirks too, check
> arch/x86/kernel/early-quirks.c. It's done by reading the PCI
> configuration space directly via a careful early-capable subset of
> the PCI config space APIs.
>
> But it's a method of last resort.
>
Thanks for your reminder. It can use direct PCI access here as follows. It's easy and clean. I think it's better than adding the source-id checking for io-apic's after the pci subsystem is up. I will send out updated patches after some tests.
@@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
" 0x%Lx\n", scope->enumeration_id,
drhd->address);
+ bus = scope->bus;
+ path = (struct acpi_dmar_pci_path *)(scope + 1);
+ count = (scope->length -
+ sizeof(struct acpi_dmar_device_scope))
+ / sizeof(struct acpi_dmar_pci_path);
+
+ while (--count > 0) {
+ /* Access PCI directly due to the PCI
+ * subsystem isn't initialized yet.
+ */
+ bus = read_pci_config_byte(bus, path->dev,
+ path->fn, PCI_SECONDARY_BUS);
+ path++;
+ }
+
+ ir_ioapic[ir_ioapic_num].bus = bus;
+ ir_ioapic[ir_ioapic_num].devfn =
+ PCI_DEVFN(path->dev, path->fn);
ir_ioapic[ir_ioapic_num].iommu = iommu;
ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
ir_ioapic_num++;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-18 9:46 ` Han, Weidong
@ 2009-05-19 9:32 ` Ingo Molnar
-1 siblings, 0 replies; 11+ messages in thread
From: Ingo Molnar @ 2009-05-19 9:32 UTC (permalink / raw)
To: Han, Weidong
Cc: Siddha, Suresh B, 'dwmw2@infradead.org',
'linux-kernel@vger.kernel.org',
'iommu@lists.linux-foundation.org',
'kvm@vger.kernel.org'
* Han, Weidong <weidong.han@intel.com> wrote:
> Ingo Molnar wrote:
> > * Han, Weidong <weidong.han@intel.com> wrote:
> >
> >> Siddha, Suresh B wrote:
> >>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
> >>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
> >>>> acpi_dmar_header *header, " 0x%Lx\n",
> >>>> scope->enumeration_id, drhd->address);
> >>>>
> >>>> + bus = pci_find_bus(drhd->segment, scope->bus);
> >>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
> >>>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
> >>>> + / sizeof(struct acpi_dmar_pci_path);
> >>>> +
> >>>> + while (count) {
> >>>> + if (pdev)
> >>>> + pci_dev_put(pdev);
> >>>> +
> >>>> + if (!bus)
> >>>> + break;
> >>>> +
> >>>> + pdev = pci_get_slot(bus,
> >>>> + PCI_DEVFN(path->dev, path->fn));
> >>>> + if (!pdev)
> >>>> + break;
> >>>
> >>> ir_parse_ioapic_scope() happens very early in the boot. So, I
> >>> don't think we can do the pci related discovery here.
> >>>
> >>
> >> Thanks for your pointing it out. It should enable the source-id
> >> checking for io-apic's after the pci subsystem is up. I will
> >> change it.
> >
> > Note, there's ways to do early PCI quirks too, check
> > arch/x86/kernel/early-quirks.c. It's done by reading the PCI
> > configuration space directly via a careful early-capable subset of
> > the PCI config space APIs.
> >
> > But it's a method of last resort.
> >
>
> Thanks for your reminder. It can use direct PCI access here as follows. It's easy and clean. I think it's better than adding the source-id checking for io-apic's after the pci subsystem is up. I will send out updated patches after some tests.
>
> @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
> " 0x%Lx\n", scope->enumeration_id,
> drhd->address);
>
> + bus = scope->bus;
> + path = (struct acpi_dmar_pci_path *)(scope + 1);
> + count = (scope->length -
> + sizeof(struct acpi_dmar_device_scope))
> + / sizeof(struct acpi_dmar_pci_path);
> +
> + while (--count > 0) {
> + /* Access PCI directly due to the PCI
> + * subsystem isn't initialized yet.
> + */
> + bus = read_pci_config_byte(bus, path->dev,
> + path->fn, PCI_SECONDARY_BUS);
> + path++;
> + }
> +
> + ir_ioapic[ir_ioapic_num].bus = bus;
> + ir_ioapic[ir_ioapic_num].devfn =
> + PCI_DEVFN(path->dev, path->fn);
looks good IMO, beyond the obligatory comment-style nitpick [*] :-)
Also, the function above seems to be way too large - please split it
into a couple of natural helper functions.
Thanks,
Ingo
[*]
Please use the customary comment style:
/*
* Comment .....
* ...... goes here:
*/
specified in Documentation/CodingStyle.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
@ 2009-05-19 9:32 ` Ingo Molnar
0 siblings, 0 replies; 11+ messages in thread
From: Ingo Molnar @ 2009-05-19 9:32 UTC (permalink / raw)
To: Han, Weidong
Cc: Siddha, Suresh B, 'dwmw2@infradead.org',
'linux-kernel@vger.kernel.org',
'iommu@lists.linux-foundation.org',
'kvm@vger.kernel.org'
* Han, Weidong <weidong.han@intel.com> wrote:
> Ingo Molnar wrote:
> > * Han, Weidong <weidong.han@intel.com> wrote:
> >
> >> Siddha, Suresh B wrote:
> >>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
> >>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
> >>>> acpi_dmar_header *header, " 0x%Lx\n",
> >>>> scope->enumeration_id, drhd->address);
> >>>>
> >>>> + bus = pci_find_bus(drhd->segment, scope->bus);
> >>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
> >>>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
> >>>> + / sizeof(struct acpi_dmar_pci_path);
> >>>> +
> >>>> + while (count) {
> >>>> + if (pdev)
> >>>> + pci_dev_put(pdev);
> >>>> +
> >>>> + if (!bus)
> >>>> + break;
> >>>> +
> >>>> + pdev = pci_get_slot(bus,
> >>>> + PCI_DEVFN(path->dev, path->fn));
> >>>> + if (!pdev)
> >>>> + break;
> >>>
> >>> ir_parse_ioapic_scope() happens very early in the boot. So, I
> >>> don't think we can do the pci related discovery here.
> >>>
> >>
> >> Thanks for your pointing it out. It should enable the source-id
> >> checking for io-apic's after the pci subsystem is up. I will
> >> change it.
> >
> > Note, there's ways to do early PCI quirks too, check
> > arch/x86/kernel/early-quirks.c. It's done by reading the PCI
> > configuration space directly via a careful early-capable subset of
> > the PCI config space APIs.
> >
> > But it's a method of last resort.
> >
>
> Thanks for your reminder. It can use direct PCI access here as follows. It's easy and clean. I think it's better than adding the source-id checking for io-apic's after the pci subsystem is up. I will send out updated patches after some tests.
>
> @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
> " 0x%Lx\n", scope->enumeration_id,
> drhd->address);
>
> + bus = scope->bus;
> + path = (struct acpi_dmar_pci_path *)(scope + 1);
> + count = (scope->length -
> + sizeof(struct acpi_dmar_device_scope))
> + / sizeof(struct acpi_dmar_pci_path);
> +
> + while (--count > 0) {
> + /* Access PCI directly due to the PCI
> + * subsystem isn't initialized yet.
> + */
> + bus = read_pci_config_byte(bus, path->dev,
> + path->fn, PCI_SECONDARY_BUS);
> + path++;
> + }
> +
> + ir_ioapic[ir_ioapic_num].bus = bus;
> + ir_ioapic[ir_ioapic_num].devfn =
> + PCI_DEVFN(path->dev, path->fn);
looks good IMO, beyond the obligatory comment-style nitpick [*] :-)
Also, the function above seems to be way too large - please split it
into a couple of natural helper functions.
Thanks,
Ingo
[*]
Please use the customary comment style:
/*
* Comment .....
* ...... goes here:
*/
specified in Documentation/CodingStyle.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
2009-05-19 9:32 ` Ingo Molnar
@ 2009-05-19 10:34 ` Han, Weidong
-1 siblings, 0 replies; 11+ messages in thread
From: Han, Weidong @ 2009-05-19 10:34 UTC (permalink / raw)
To: 'Ingo Molnar'
Cc: Siddha, Suresh B, 'dwmw2@infradead.org',
'linux-kernel@vger.kernel.org',
'iommu@lists.linux-foundation.org',
'kvm@vger.kernel.org'
Ingo Molnar wrote:
> * Han, Weidong <weidong.han@intel.com> wrote:
>
>> Ingo Molnar wrote:
>>> * Han, Weidong <weidong.han@intel.com> wrote:
>>>
>>>> Siddha, Suresh B wrote:
>>>>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>>>>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>>>>>> acpi_dmar_header *header, " 0x%Lx\n",
>>>>>> scope->enumeration_id, drhd->address);
>>>>>>
>>>>>> + bus = pci_find_bus(drhd->segment, scope->bus);
>>>>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
>>>>>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
>>>>>> + / sizeof(struct acpi_dmar_pci_path);
>>>>>> +
>>>>>> + while (count) {
>>>>>> + if (pdev)
>>>>>> + pci_dev_put(pdev);
>>>>>> +
>>>>>> + if (!bus)
>>>>>> + break;
>>>>>> +
>>>>>> + pdev = pci_get_slot(bus,
>>>>>> + PCI_DEVFN(path->dev, path->fn));
>>>>>> + if (!pdev)
>>>>>> + break;
>>>>>
>>>>> ir_parse_ioapic_scope() happens very early in the boot. So, I
>>>>> don't think we can do the pci related discovery here.
>>>>>
>>>>
>>>> Thanks for your pointing it out. It should enable the source-id
>>>> checking for io-apic's after the pci subsystem is up. I will
>>>> change it.
>>>
>>> Note, there's ways to do early PCI quirks too, check
>>> arch/x86/kernel/early-quirks.c. It's done by reading the PCI
>>> configuration space directly via a careful early-capable subset of
>>> the PCI config space APIs.
>>>
>>> But it's a method of last resort.
>>>
>>
>> Thanks for your reminder. It can use direct PCI access here as
>> follows. It's easy and clean. I think it's better than adding the
>> source-id checking for io-apic's after the pci subsystem is up. I
>> will send out updated patches after some tests.
>>
>> @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct
>> acpi_dmar_header *header, " 0x%Lx\n",
>> scope->enumeration_id, drhd->address);
>>
>> + bus = scope->bus;
>> + path = (struct acpi_dmar_pci_path *)(scope +
>> 1); + count = (scope->length -
>> + sizeof(struct
>> acpi_dmar_device_scope)) + /
>> sizeof(struct acpi_dmar_pci_path); + + while
>> (--count > 0) { + /* Access PCI
>> directly due to the PCI + * subsystem
>> isn't initialized yet. + */
>> + bus = read_pci_config_byte(bus,
>> path->dev, + path->fn,
>> PCI_SECONDARY_BUS); + path++;
>> + }
>> +
>> + ir_ioapic[ir_ioapic_num].bus = bus;
>> + ir_ioapic[ir_ioapic_num].devfn =
>> + PCI_DEVFN(path->dev,
>> path->fn);
>
> looks good IMO, beyond the obligatory comment-style nitpick [*] :-)
> Also, the function above seems to be way too large - please split it
> into a couple of natural helper functions.
>
> Thanks,
>
> Ingo
>
> [*]
>
> Please use the customary comment style:
>
> /*
> * Comment .....
> * ...... goes here:
> */
>
> specified in Documentation/CodingStyle.
I have sent out the updated patches. Thanks!
Regards,
Weidong
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking
@ 2009-05-19 10:34 ` Han, Weidong
0 siblings, 0 replies; 11+ messages in thread
From: Han, Weidong @ 2009-05-19 10:34 UTC (permalink / raw)
To: 'Ingo Molnar'
Cc: Siddha, Suresh B, 'dwmw2@infradead.org',
'linux-kernel@vger.kernel.org',
'iommu@lists.linux-foundation.org',
'kvm@vger.kernel.org'
Ingo Molnar wrote:
> * Han, Weidong <weidong.han@intel.com> wrote:
>
>> Ingo Molnar wrote:
>>> * Han, Weidong <weidong.han@intel.com> wrote:
>>>
>>>> Siddha, Suresh B wrote:
>>>>> On Wed, 2009-05-06 at 23:16 -0700, Han, Weidong wrote:
>>>>>> @@ -634,6 +694,44 @@ static int ir_parse_ioapic_scope(struct
>>>>>> acpi_dmar_header *header, " 0x%Lx\n",
>>>>>> scope->enumeration_id, drhd->address);
>>>>>>
>>>>>> + bus = pci_find_bus(drhd->segment, scope->bus);
>>>>>> + path = (struct acpi_dmar_pci_path *)(scope + 1); + count =
>>>>>> (scope->length - + sizeof(struct acpi_dmar_device_scope))
>>>>>> + / sizeof(struct acpi_dmar_pci_path);
>>>>>> +
>>>>>> + while (count) {
>>>>>> + if (pdev)
>>>>>> + pci_dev_put(pdev);
>>>>>> +
>>>>>> + if (!bus)
>>>>>> + break;
>>>>>> +
>>>>>> + pdev = pci_get_slot(bus,
>>>>>> + PCI_DEVFN(path->dev, path->fn));
>>>>>> + if (!pdev)
>>>>>> + break;
>>>>>
>>>>> ir_parse_ioapic_scope() happens very early in the boot. So, I
>>>>> don't think we can do the pci related discovery here.
>>>>>
>>>>
>>>> Thanks for your pointing it out. It should enable the source-id
>>>> checking for io-apic's after the pci subsystem is up. I will
>>>> change it.
>>>
>>> Note, there's ways to do early PCI quirks too, check
>>> arch/x86/kernel/early-quirks.c. It's done by reading the PCI
>>> configuration space directly via a careful early-capable subset of
>>> the PCI config space APIs.
>>>
>>> But it's a method of last resort.
>>>
>>
>> Thanks for your reminder. It can use direct PCI access here as
>> follows. It's easy and clean. I think it's better than adding the
>> source-id checking for io-apic's after the pci subsystem is up. I
>> will send out updated patches after some tests.
>>
>> @@ -634,6 +695,24 @@ static int ir_parse_ioapic_scope(struct
>> acpi_dmar_header *header, " 0x%Lx\n",
>> scope->enumeration_id, drhd->address);
>>
>> + bus = scope->bus;
>> + path = (struct acpi_dmar_pci_path *)(scope +
>> 1); + count = (scope->length -
>> + sizeof(struct
>> acpi_dmar_device_scope)) + /
>> sizeof(struct acpi_dmar_pci_path); + + while
>> (--count > 0) { + /* Access PCI
>> directly due to the PCI + * subsystem
>> isn't initialized yet. + */
>> + bus = read_pci_config_byte(bus,
>> path->dev, + path->fn,
>> PCI_SECONDARY_BUS); + path++;
>> + }
>> +
>> + ir_ioapic[ir_ioapic_num].bus = bus;
>> + ir_ioapic[ir_ioapic_num].devfn =
>> + PCI_DEVFN(path->dev,
>> path->fn);
>
> looks good IMO, beyond the obligatory comment-style nitpick [*] :-)
> Also, the function above seems to be way too large - please split it
> into a couple of natural helper functions.
>
> Thanks,
>
> Ingo
>
> [*]
>
> Please use the customary comment style:
>
> /*
> * Comment .....
> * ...... goes here:
> */
>
> specified in Documentation/CodingStyle.
I have sent out the updated patches. Thanks!
Regards,
Weidong
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-05-19 10:36 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-07 6:16 [PATCH 1/2] Intel-IOMMU, intr-remap: set the whole 128bits of irte when modify/free it Weidong Han
2009-05-07 6:16 ` [PATCH 2/2] Intel-IOMMU, intr-remap: source-id checking Weidong Han
2009-05-07 18:19 ` Suresh Siddha
2009-05-11 6:22 ` Han, Weidong
2009-05-11 13:20 ` Ingo Molnar
2009-05-18 9:46 ` Han, Weidong
2009-05-18 9:46 ` Han, Weidong
2009-05-19 9:32 ` Ingo Molnar
2009-05-19 9:32 ` Ingo Molnar
2009-05-19 10:34 ` Han, Weidong
2009-05-19 10:34 ` Han, Weidong
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.