lkml.org 
[lkml]   [2009]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/2] Intel-IOMMU, intr-remap: source-id checking

    * Weidong Han <weidong.han@intel.com> wrote:

    > 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.
    >
    > Because PCI subsystem is not initialized yet when set up IOAPIC
    > entries, use read_pci_config_byte to access PCI config space
    > directly.
    >
    > Signed-off-by: Weidong Han <weidong.han@intel.com>
    > ---
    > arch/x86/kernel/apic/io_apic.c | 6 +++
    > drivers/pci/intr_remapping.c | 90 ++++++++++++++++++++++++++++++++++++++-
    > drivers/pci/intr_remapping.h | 2 +
    > include/linux/dmar.h | 11 +++++
    > 4 files changed, 106 insertions(+), 3 deletions(-)

    Code structure looks nice now. (and i susect you have tested this on
    real and relevant hardware?) I've Cc:-ed Eric too ... does this
    direction look good to you too Eric?

    Have a few minor nits only:

    > 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..9ef7b0d 100644
    > --- a/drivers/pci/intr_remapping.c
    > +++ b/drivers/pci/intr_remapping.c
    > @@ -10,6 +10,8 @@
    > #include <linux/intel-iommu.h>
    > #include "intr_remapping.h"
    > #include <acpi/acpi.h>
    > +#include <asm/pci-direct.h>
    > +#include "pci.h"
    >
    > static struct ioapic_scope ir_ioapic[MAX_IO_APICS];
    > static int ir_ioapic_num;
    > @@ -405,6 +407,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;
    > + }

    Please generally put extra curly braces around each multi-line loop
    body. One reason beyond readability is robustness: the above
    structure can be easily extended in a buggy way via [mockup patch
    hunk]:

    > sid = (ir_ioapic[i].bus << 8) | ir_ioapic[i].devfn;
    > break;
    > }
    > + if (!sid)
    > + break;

    And note that if this slips in by accident how unobvious this bug is
    during patch review - the loop head context is not present in the
    3-line default context and the code "looks" correct at a glance.

    With extra braces, such typos or mismerges:

    > }
    > }
    > + if (!sid)
    > + break;

    stick out during review like a sore thumb :-)

    > + 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;
    > + }

    please try to keep kernel messages on a single line - even if
    checkpatch complains. Also, it's a good idea to use pr_warning(),
    it's shorter by 8 characters.

    > +
    > + irte->svt = 1; /* requestor ID verification SID/SQ */
    > + irte->sq = 0; /* comparing all 16-bit of SID */
    > + irte->sid = sid;

    this is a borderline suggestion:

    Note how you already lined up the _comments_ vertically, so you did
    notice that it makes sense to align vertically. The same visual
    arguments can be made for the initialization itself too:

    > +
    > + irte->svt = 1; /* requestor ID verification SID/SQ */
    > + irte->sq = 0; /* comparing all 16-bit of SID */
    > + irte->sid = sid;
    > +
    > + return 0;

    But ... it might make even more sense to introduce a set_irte()
    helper method, so it can all be written in a single line as:

    set_irte(irte, 1, 0, sid);

    and explain common values in the set_irte() function's description -
    that way those comments above (and below) dont have to be made at
    the usage sites.

    > +}
    > +
    > +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);

    variable names like 'tmp' are only used if they are _really_ short
    in scope. Here a much better name would be 'bridge'.

    > + 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/PCIX bridge */
    > + irte->svt = 2; /* verify request ID verification SID */
    > + irte->sid = (tmp->bus->number << 8) | dev->bus->number;

    is irte->sq left alone intentionally?

    > + } 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;
    > + }
    > + if (tmp->is_pcie) { /* this is a PCIE-to-PCI/PCIX bridge */
    > + irte->svt = 2; /* verify request ID verification SID */
    > + irte->sid = (tmp->bus->number << 8) | dev->bus->number;

    here too?

    > + } 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;
    > @@ -610,6 +667,35 @@ error:
    > return -1;
    > }
    >
    > +static void ir_parse_one_ioapic_scope(struct acpi_dmar_device_scope *scope,
    > + struct intel_iommu *iommu)
    > +{
    > + struct acpi_dmar_pci_path *path;
    > + u8 bus;
    > + int count;
    > +
    > + 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;

    this too can be aligned vertically.

    > + ir_ioapic_num++;
    > +}
    > +
    > static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
    > struct intel_iommu *iommu)
    > {
    > @@ -634,9 +720,7 @@ static int ir_parse_ioapic_scope(struct acpi_dmar_header *header,
    > " 0x%Lx\n", scope->enumeration_id,
    > drhd->address);
    >
    > - ir_ioapic[ir_ioapic_num].iommu = iommu;
    > - ir_ioapic[ir_ioapic_num].id = scope->enumeration_id;
    > - ir_ioapic_num++;
    > + ir_parse_one_ioapic_scope(scope, iommu);

    how much nicer this helper looks like now!

    > }
    > start += scope->length;
    > }
    > 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 */

    hm, in kernel data structures we usually encode devfn as unsigned
    int - and sometimes even bus. Only 8 bits will be used of both so
    it's the same end result, but it results in more efficient
    instructions without byte shuffling.

    Ingo


    \
     
     \ /
      Last update: 2009-05-19 13:53    [W:0.038 / U:60.080 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site