[kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[Peter Xu]

This series is adding simplest test cases for VT-d.

The series contains some conflict with Alex's PCI framework
enhancement, so I am marking it as RFC, I can rebase to Alex's work
after it's merged. Besides the conflict, the codes is workable, and
passed smoke test.

Currently only a very small test scope is covered:

* VT-d init
* DMAR: 4 bytes copy
* IR: MSI

However this series could be a base point to add more test cases for
VT-d. The problem is, there are many IOMMU error conditions which are
very hard to be triggered in a real guest (IOMMU has merely no
interface for guest user, and it's totally running in the background).
This piece of work can be a start point if we want to do more
complicated things and play around with Intel IOMMU devices (also for
IOMMU regression tests).

Please review. Thanks,

=================

To run the test:

./x86/run ./x86/intel-iommu.flat \
    -M q35,kernel-irqchip=split -global ioapic.version=0x20 \
    -device intel-iommu,intremap=on -device edu

Sample output:

pxdev:kvm-unit-tests [new-iommu-ut]# ./iommu_run.sh
/root/git/qemu/bin/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -kernel ./x86/intel-iommu.flat -M q35,kernel-irqchip=split -global ioapic.version=0x20 -device intel-iommu,intremap=on -device edu
enabling apic
paging enabled
cr0 = 80010011
cr3 = 7fff000
cr4 = 20
VT-d version:   0x10
     cap:       0x0012008c22260206
     ecap:      0x0000000000f00f1a
PASS: init status check
PASS: fault status check
PASS: QI enablement
DMAR table address: 0x0000000007ff9000
PASS: DMAR table setup
IR table address: 0x0000000007ff8000
PASS: IR table setup
PASS: DMAR enablement
PASS: IR enablement
PASS: DMAR support 39 bits address width
PASS: DMAR support huge pages
PCI: init dev 0x0020 BAR 0 [MEM] addr 0xfea00000
PCI detected cap 0x5
Detected MSI for device 0x20 offset 0x40
allocated vt-d root entry for PCI bus 0
allocated vt-d context entry for devfn 0x20
map 4K page IOVA 0x0 to 0x7ff7000 (sid=0x0020)
edu device DMA start TO addr 0x0 size 0x4 off 0x0
edu device DMA start FROM addr 0x4 size 0x4 off 0x0
PASS: DMAR 4B memcpy test
INTR: setup IRTE index 0
MSI: dev 0x20 init 64bit address: addr=0xfee00010, data=0x0
PASS: EDU factorial INTR test

SUMMARY: 11 tests

=================

Peter Xu (14):
  x86: vm: allow multiple init for vm setup
  x86: smp: allow multiple init for smp setup
  x86: intel-iommu: add vt-d init test
  pci: refactor init process to pci_dev_init()
  page: add page alignment checker
  util: move MAX/MIN macro into util.h
  vm/page: provide PGDIR_OFFSET() macro
  x86: pci: add pci_config_{read|write}[bw]() helpers
  pci: provide pci_set_master()
  pci: add bdf helpers
  pci: edu: introduce pci-edu helpers
  x86: intel-iommu: add dmar test
  pci: add msi support for 32/64bit address
  x86: intel-iommu: add IR test

 lib/alloc.c            |   3 -
 lib/asm-generic/page.h |   5 +
 lib/pci-edu.c          | 140 +++++++++++++++++++
 lib/pci-edu.h          |  36 +++++
 lib/pci.c              | 143 +++++++++++++++----
 lib/pci.h              |  37 ++++-
 lib/util.h             |   3 +
 lib/x86/asm/page.h     |   3 +
 lib/x86/asm/pci.h      |  37 ++++-
 lib/x86/intel-iommu.c  | 363 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/x86/intel-iommu.h  | 130 ++++++++++++++++++
 lib/x86/smp.c          |   6 +
 lib/x86/vm.c           |  11 +-
 x86/Makefile.common    |   1 +
 x86/Makefile.x86_64    |   2 +
 x86/intel-iommu.c      | 138 +++++++++++++++++++
 x86/vmexit.c           |  22 +--
 17 files changed, 1026 insertions(+), 54 deletions(-)
 create mode 100644 lib/pci-edu.c
 create mode 100644 lib/pci-edu.h
 create mode 100644 lib/x86/intel-iommu.c
 create mode 100644 lib/x86/intel-iommu.h
 create mode 100644 x86/intel-iommu.c

-- 
2.7.4

[kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/vm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 906fbf2..9771bd7 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -151,9 +151,16 @@ static void setup_mmu(unsigned long len)
 
 void setup_vm()
 {
+    static bool vm_inited = false;
+
+    if (vm_inited) {
+        return;
+    }
+
     end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
     free_memory(&edata, end_of_memory - (unsigned long)&edata);
     setup_mmu(end_of_memory);
+    vm_inited = true;
 }
 
 void *vmalloc(unsigned long size)
-- 
2.7.4

[kvm-unit-tests PATCH 02/14] x86: smp: allow multiple init for smp setup

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/smp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/x86/smp.c b/lib/x86/smp.c
index 1eb49f2..bb74087 100644
--- a/lib/x86/smp.c
+++ b/lib/x86/smp.c
@@ -111,8 +111,13 @@ void on_cpu_async(int cpu, void (*function)(void *data), void *data)
 void smp_init(void)
 {
     int i;
+    bool smp_inited = false;
     void ipi_entry(void);
 
+    if (smp_inited) {
+        return;
+    }
+
     _cpu_count = fwcfg_get_nb_cpus();
 
     setup_idt();
@@ -122,4 +127,5 @@ void smp_init(void)
     for (i = 1; i < cpu_count(); ++i)
         on_cpu(i, setup_smp_id, 0);
 
+    smp_inited = true;
 }
-- 
2.7.4

[kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test

[Peter Xu]

Adding fundamental init test for Intel IOMMU. This includes basic
initialization of Intel IOMMU device, like DMAR (DMA Remapping),
IR (Interrupt Remapping), QI (Queue Invalidation), etc.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/intel-iommu.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/x86/intel-iommu.h | 106 ++++++++++++++++++++++++++++++++++++++
 x86/Makefile.x86_64   |   2 +
 x86/intel-iommu.c     |  41 +++++++++++++++
 4 files changed, 287 insertions(+)
 create mode 100644 lib/x86/intel-iommu.c
 create mode 100644 lib/x86/intel-iommu.h
 create mode 100644 x86/intel-iommu.c

diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
new file mode 100644
index 0000000..9f55394
--- /dev/null
+++ b/lib/x86/intel-iommu.c
@@ -0,0 +1,138 @@
+/*
+ * Intel IOMMU APIs
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * Authors:
+ *   Peter Xu <peterx@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or
+ * later.
+ */
+
+#include "intel-iommu.h"
+
+void vtd_reg_write_4(unsigned int reg, uint32_t value)
+{
+	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
+}
+
+void vtd_reg_write_8(unsigned int reg, uint64_t value)
+{
+	*(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
+}
+
+uint32_t vtd_reg_read_4(unsigned int reg)
+{
+	return *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
+}
+
+uint64_t vtd_reg_read_8(unsigned int reg)
+{
+	return *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
+}
+
+uint32_t vtd_version(void)
+{
+	return vtd_reg_read_4(DMAR_VER_REG);
+}
+
+uint64_t vtd_cap(void)
+{
+	return vtd_reg_read_8(DMAR_CAP_REG);
+}
+
+uint64_t vtd_ecap(void)
+{
+	return vtd_reg_read_8(DMAR_ECAP_REG);
+}
+
+uint32_t vtd_status(void)
+{
+	return vtd_reg_read_4(DMAR_GSTS_REG);
+}
+
+uint32_t vtd_fault_status(void)
+{
+	return vtd_reg_read_4(DMAR_FSTS_REG);
+}
+
+uint64_t vtd_root_table(void)
+{
+	/* No extend root table support yet */
+	return vtd_reg_read_8(DMAR_RTADDR_REG);
+}
+
+uint64_t vtd_ir_table(void)
+{
+	return vtd_reg_read_8(DMAR_IRTA_REG) & 0xfffffffffffff000;
+}
+
+void vtd_gcmd_or(uint32_t cmd)
+{
+	uint32_t status = vtd_status();
+	/*
+	 * Logically, we need to read DMAR_GSTS_REG until IOMMU
+	 * handles the write request. However for QEMU/KVM, this write
+	 * is always sync, so when this returns, we should be sure
+	 * that the GCMD write is done.
+	 */
+	vtd_reg_write_4(DMAR_GCMD_REG, status | cmd);
+}
+
+void vtd_dump_init_info(void)
+{
+	printf("VT-d version:   0x%x\n", vtd_version());
+	printf("     cap:       0x%016lx\n", vtd_cap());
+	printf("     ecap:      0x%016lx\n", vtd_ecap());
+}
+
+void vtd_enable_qi(void)
+{
+	vtd_gcmd_or(VTD_GCMD_QI);
+}
+
+void vtd_setup_root_table(void)
+{
+	void *root = alloc_page();
+
+	memset(root, 0, PAGE_SIZE);
+	vtd_reg_write_8(DMAR_RTADDR_REG, virt_to_phys(root));
+	vtd_gcmd_or(VTD_GCMD_ROOT);
+	printf("DMAR table address: 0x%016lx\n", vtd_root_table());
+}
+
+void vtd_setup_ir_table(void)
+{
+	void *root = alloc_page();
+
+	memset(root, 0, PAGE_SIZE);
+	vtd_reg_write_8(DMAR_IRTA_REG, virt_to_phys(root));
+	/* 0xf stands for table size (2^(0xf+1) == 65536) */
+	vtd_gcmd_or(VTD_GCMD_IR_TABLE | 0xf);
+	printf("IR table address: 0x%016lx\n", vtd_ir_table());
+}
+
+void vtd_enable_dmar(void)
+{
+	vtd_gcmd_or(VTD_GCMD_DMAR);
+}
+
+void vtd_enable_ir(void)
+{
+	vtd_gcmd_or(VTD_GCMD_IR);
+}
+
+void vtd_init(void)
+{
+	setup_vm();
+	smp_init();
+	setup_idt();
+
+	vtd_dump_init_info();
+	vtd_enable_qi();
+	vtd_setup_root_table();
+	vtd_setup_ir_table();
+	vtd_enable_dmar();
+	vtd_enable_ir();
+}
diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
new file mode 100644
index 0000000..0f687d1
--- /dev/null
+++ b/lib/x86/intel-iommu.h
@@ -0,0 +1,106 @@
+/*
+ * Intel IOMMU header
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * Authors:
+ *   Peter Xu <peterx@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or
+ * later.
+ */
+
+#ifndef __INTEL_IOMMU_H__
+#define __INTEL_IOMMU_H__
+
+#include "libcflat.h"
+#include "vm.h"
+#include "isr.h"
+#include "smp.h"
+#include "desc.h"
+
+#define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed90000ULL
+
+/*
+ * Intel IOMMU register specification
+ */
+#define DMAR_VER_REG            0x0  /* Arch version supported by this IOMMU */
+#define DMAR_CAP_REG            0x8  /* Hardware supported capabilities */
+#define DMAR_CAP_REG_HI         0xc  /* High 32-bit of DMAR_CAP_REG */
+#define DMAR_ECAP_REG           0x10 /* Extended capabilities supported */
+#define DMAR_ECAP_REG_HI        0X14
+#define DMAR_GCMD_REG           0x18 /* Global command */
+#define DMAR_GSTS_REG           0x1c /* Global status */
+#define DMAR_RTADDR_REG         0x20 /* Root entry table */
+#define DMAR_RTADDR_REG_HI      0X24
+#define DMAR_CCMD_REG           0x28 /* Context command */
+#define DMAR_CCMD_REG_HI        0x2c
+#define DMAR_FSTS_REG           0x34 /* Fault status */
+#define DMAR_FECTL_REG          0x38 /* Fault control */
+#define DMAR_FEDATA_REG         0x3c /* Fault event interrupt data */
+#define DMAR_FEADDR_REG         0x40 /* Fault event interrupt addr */
+#define DMAR_FEUADDR_REG        0x44 /* Upper address */
+#define DMAR_AFLOG_REG          0x58 /* Advanced fault control */
+#define DMAR_AFLOG_REG_HI       0X5c
+#define DMAR_PMEN_REG           0x64 /* Enable protected memory region */
+#define DMAR_PLMBASE_REG        0x68 /* PMRR low addr */
+#define DMAR_PLMLIMIT_REG       0x6c /* PMRR low limit */
+#define DMAR_PHMBASE_REG        0x70 /* PMRR high base addr */
+#define DMAR_PHMBASE_REG_HI     0X74
+#define DMAR_PHMLIMIT_REG       0x78 /* PMRR high limit */
+#define DMAR_PHMLIMIT_REG_HI    0x7c
+#define DMAR_IQH_REG            0x80 /* Invalidation queue head */
+#define DMAR_IQH_REG_HI         0X84
+#define DMAR_IQT_REG            0x88 /* Invalidation queue tail */
+#define DMAR_IQT_REG_HI         0X8c
+#define DMAR_IQA_REG            0x90 /* Invalidation queue addr */
+#define DMAR_IQA_REG_HI         0x94
+#define DMAR_ICS_REG            0x9c /* Invalidation complete status */
+#define DMAR_IRTA_REG           0xb8 /* Interrupt remapping table addr */
+#define DMAR_IRTA_REG_HI        0xbc
+#define DMAR_IECTL_REG          0xa0 /* Invalidation event control */
+#define DMAR_IEDATA_REG         0xa4 /* Invalidation event data */
+#define DMAR_IEADDR_REG         0xa8 /* Invalidation event address */
+#define DMAR_IEUADDR_REG        0xac /* Invalidation event address */
+#define DMAR_PQH_REG            0xc0 /* Page request queue head */
+#define DMAR_PQH_REG_HI         0xc4
+#define DMAR_PQT_REG            0xc8 /* Page request queue tail*/
+#define DMAR_PQT_REG_HI         0xcc
+#define DMAR_PQA_REG            0xd0 /* Page request queue address */
+#define DMAR_PQA_REG_HI         0xd4
+#define DMAR_PRS_REG            0xdc /* Page request status */
+#define DMAR_PECTL_REG          0xe0 /* Page request event control */
+#define DMAR_PEDATA_REG         0xe4 /* Page request event data */
+#define DMAR_PEADDR_REG         0xe8 /* Page request event address */
+#define DMAR_PEUADDR_REG        0xec /* Page event upper address */
+#define DMAR_MTRRCAP_REG        0x100 /* MTRR capability */
+#define DMAR_MTRRCAP_REG_HI     0x104
+#define DMAR_MTRRDEF_REG        0x108 /* MTRR default type */
+#define DMAR_MTRRDEF_REG_HI     0x10c
+
+#define VTD_GCMD_IR_TABLE       (0x1000000)
+#define VTD_GCMD_IR             (0x2000000)
+#define VTD_GCMD_QI             (0x4000000)
+#define VTD_GCMD_ROOT           (0x40000000)
+#define VTD_GCMD_DMAR           (0x80000000)
+
+void vtd_reg_write_4(unsigned int reg, uint32_t value);
+void vtd_reg_write_8(unsigned int reg, uint64_t value);
+uint32_t vtd_reg_read_4(unsigned int reg);
+uint64_t vtd_reg_read_8(unsigned int reg);
+uint32_t vtd_version(void);
+uint64_t vtd_cap(void);
+uint64_t vtd_ecap(void);
+uint32_t vtd_status(void);
+uint32_t vtd_fault_status(void);
+uint64_t vtd_root_table(void);
+uint64_t vtd_ir_table(void);
+void vtd_dump_init_info(void);
+void vtd_enable_qi(void);
+void vtd_setup_root_table(void);
+void vtd_setup_ir_table(void);
+void vtd_enable_dmar(void);
+void vtd_enable_ir(void);
+void vtd_init(void);
+
+#endif
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index e166911..a8e9445 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -4,6 +4,7 @@ ldarch = elf64-x86-64
 CFLAGS += -mno-red-zone
 
 cflatobjs += lib/x86/setjmp64.o
+cflatobjs += lib/x86/intel-iommu.o
 
 tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
 	  $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
@@ -14,6 +15,7 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
 tests += $(TEST_DIR)/svm.flat
 tests += $(TEST_DIR)/vmx.flat
 tests += $(TEST_DIR)/tscdeadline_latency.flat
+tests += $(TEST_DIR)/intel-iommu.flat
 
 include $(TEST_DIR)/Makefile.common
 
diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
new file mode 100644
index 0000000..60d512a
--- /dev/null
+++ b/x86/intel-iommu.c
@@ -0,0 +1,41 @@
+/*
+ * Intel IOMMU unit test.
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * Authors:
+ *   Peter Xu <peterx@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or
+ * later.
+ */
+
+#include "intel-iommu.h"
+
+int main(int argc, char *argv[])
+{
+	setup_vm();
+	smp_init();
+	setup_idt();
+
+	vtd_dump_init_info();
+	report("init status check", vtd_status() == 0);
+	report("fault status check", vtd_fault_status() == 0);
+
+	vtd_enable_qi();
+	report("QI enablement", vtd_status() | VTD_GCMD_QI);
+
+	vtd_setup_root_table();
+	report("DMAR table setup", vtd_status() | VTD_GCMD_ROOT);
+
+	vtd_setup_ir_table();
+	report("IR table setup", vtd_status() | VTD_GCMD_IR_TABLE);
+
+	vtd_enable_dmar();
+	report("DMAR enablement", vtd_status() & VTD_GCMD_DMAR);
+
+	vtd_enable_ir();
+	report("IR enablement", vtd_status() & VTD_GCMD_IR);
+
+	return report_summary();
+}
-- 
2.7.4

[kvm-unit-tests PATCH 04/14] pci: refactor init process to pci_dev_init()

[Peter Xu]

pci_find_dev() is not sufficient for further tests for Intel IOMMU. This
patch introduced pci_dev struct, as a abstract of a specific PCI device.

All the rest of current PCI APIs are changed to leverage the pci_dev
struct.

x86/vmexit.c is the only user of the old PCI APIs. Changing it to use
the new ones.

(Indentation is fixed from using 4 spaces into tab in pci.c)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/pci.c    | 75 +++++++++++++++++++++++++++++++++++++++++-------------------
 lib/pci.h    | 29 ++++++++++++++++++-----
 x86/vmexit.c | 22 ++++--------------
 3 files changed, 80 insertions(+), 46 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index 0058d70..ef0b02a 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -7,37 +7,66 @@
 #include "pci.h"
 #include "asm/pci.h"
 
-/* Scan bus look for a specific device. Only bus 0 scanned for now. */
-pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
+/*
+ * Scan bus look for a specific device. Only bus 0 scanned for now.
+ * After the scan, a pci_dev is returned with correct BAR information.
+ */
+int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
+{
+	unsigned i;
+	uint32_t id;
+
+	memset(dev, 0, sizeof(*dev));
+
+	for (i = 0; i < PCI_DEVFN_MAX; ++i) {
+		id = pci_config_read(i, 0);
+		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+			break;
+	}
+
+	if (i == PCI_DEVFN_MAX) {
+		printf("PCI: failed init dev (vendor: 0x%04x, "
+		       "device: 0x%04x)\n", vendor_id, device_id);
+		return -1;
+	}
+
+	dev->pci_addr = i;
+	for (i = 0; i < PCI_BAR_NUM; i++) {
+		if (!pci_bar_is_valid(dev, i))
+			continue;
+		dev->pci_bar[i] = pci_bar_addr(dev, i);
+		printf("PCI: init dev 0x%04x BAR %d [%s] addr 0x%lx\n",
+		       dev->pci_addr, i, pci_bar_is_memory(dev, i) ?
+		       "MEM" : "IO", dev->pci_bar[i]);
+	}
+	dev->inited = true;
+
+	return 0;
+}
+
+static inline uint32_t pci_config_read_bar(pci_dev *dev, int n)
 {
-    unsigned dev;
-    for (dev = 0; dev < 256; ++dev) {
-    uint32_t id = pci_config_read(dev, 0);
-    if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) {
-        return dev;
-    }
-    }
-    return PCIDEVADDR_INVALID;
+	return pci_config_read(dev->pci_addr,
+			       PCI_BASE_ADDRESS_0 + n * 4);
 }
 
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
-        return bar & PCI_BASE_ADDRESS_IO_MASK;
-    } else {
-        return bar & PCI_BASE_ADDRESS_MEM_MASK;
-    }
+	uint32_t bar = pci_config_read_bar(dev, bar_num);
+	if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
+		return bar & PCI_BASE_ADDRESS_IO_MASK;
+	} else {
+		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	}
 }
 
-bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
+bool pci_bar_is_memory(pci_dev *dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
+	uint32_t bar = pci_config_read_bar(dev, bar_num);
+	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
 }
 
-bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
+bool pci_bar_is_valid(pci_dev *dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    return bar;
+	return pci_config_read_bar(dev, bar_num);
 }
diff --git a/lib/pci.h b/lib/pci.h
index 9160cfb..b8755ff 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -14,10 +14,21 @@ typedef uint16_t pcidevaddr_t;
 enum {
     PCIDEVADDR_INVALID = 0xffff,
 };
-pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
-bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
-bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
+
+#define PCI_BAR_NUM                     (6)
+#define PCI_DEVFN_MAX                   (256)
+
+struct pci_dev {
+	bool inited;
+	uint16_t pci_addr;
+	phys_addr_t pci_bar[PCI_BAR_NUM];
+};
+typedef struct pci_dev pci_dev;
+
+int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
+phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
+bool pci_bar_is_memory(pci_dev *dev, int bar_num);
+bool pci_bar_is_valid(pci_dev *dev, int bar_num);
 
 /*
  * pci-testdev is a driver for the pci-testdev qemu pci device. The
@@ -27,8 +38,6 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 #define PCI_VENDOR_ID_REDHAT		0x1b36
 #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
 
-#define PCI_TESTDEV_NUM_BARS		2
-
 struct pci_test_dev_hdr {
 	uint8_t  test;
 	uint8_t  width;
@@ -39,4 +48,12 @@ struct pci_test_dev_hdr {
 	uint8_t  name[];
 };
 
+enum pci_dma_dir {
+	PCI_DMA_FROM_DEVICE = 0,
+	PCI_DMA_TO_DEVICE,
+};
+typedef enum pci_dma_dir pci_dma_dir_t;
+
+typedef uint64_t iova_t;
+
 #endif /* PCI_H */
diff --git a/x86/vmexit.c b/x86/vmexit.c
index c2e1e49..908d2dc 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -371,8 +371,7 @@ int main(int ac, char **av)
 {
 	struct fadt_descriptor_rev1 *fadt;
 	int i;
-	unsigned long membar = 0;
-	pcidevaddr_t pcidev;
+	pci_dev dev;
 
 	smp_init();
 	setup_vm();
@@ -385,21 +384,10 @@ int main(int ac, char **av)
 	pm_tmr_blk = fadt->pm_tmr_blk;
 	printf("PM timer port is %x\n", pm_tmr_blk);
 
-	pcidev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
-	if (pcidev != PCIDEVADDR_INVALID) {
-		for (i = 0; i < PCI_TESTDEV_NUM_BARS; i++) {
-			if (!pci_bar_is_valid(pcidev, i)) {
-				continue;
-			}
-			if (pci_bar_is_memory(pcidev, i)) {
-				membar = pci_bar_addr(pcidev, i);
-				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
-			} else {
-				pci_test.iobar = pci_bar_addr(pcidev, i);
-			}
-		}
-		printf("pci-testdev at 0x%x membar %lx iobar %x\n",
-		       pcidev, membar, pci_test.iobar);
+	if (!pci_dev_init(&dev, PCI_VENDOR_ID_REDHAT,
+			  PCI_DEVICE_ID_REDHAT_TEST)) {
+		pci_test.memaddr = ioremap(dev.pci_bar[0], PAGE_SIZE);
+		pci_test.iobar = dev.pci_bar[1];
 	}
 
 	for (i = 0; i < ARRAY_SIZE(tests); ++i)
-- 
2.7.4

[kvm-unit-tests PATCH 05/14] page: add page alignment checker

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/asm-generic/page.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/asm-generic/page.h b/lib/asm-generic/page.h
index 7b8a08b..cbdc8f6 100644
--- a/lib/asm-generic/page.h
+++ b/lib/asm-generic/page.h
@@ -19,6 +19,11 @@
 
 #define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
 
+#define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
+#define PAGE_ALIGNED_4K(x)	IS_ALIGNED((x), (0x1000))
+#define PAGE_ALIGNED_2M(x)	IS_ALIGNED((x), (0x200000))
+#define PAGE_ALIGNED_1G(x)	IS_ALIGNED((x), (0x40000000))
+
 #define __va(x)			((void *)((unsigned long) (x)))
 #define __pa(x)			((unsigned long) (x))
 #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
-- 
2.7.4

[kvm-unit-tests PATCH 06/14] util: move MAX/MIN macro into util.h

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/alloc.c | 3 ---
 lib/util.h  | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/alloc.c b/lib/alloc.c
index e1d7b8a..58af52b 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -7,9 +7,6 @@
 #include "asm/spinlock.h"
 #include "asm/io.h"
 
-#define MIN(a, b)		((a) < (b) ? (a) : (b))
-#define MAX(a, b)		((a) > (b) ? (a) : (b))
-
 #define PHYS_ALLOC_NR_REGIONS	256
 
 struct phys_alloc_region {
diff --git a/lib/util.h b/lib/util.h
index 4c4b441..1462f4f 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -20,4 +20,7 @@
  */
 extern int parse_keyval(char *s, long *val);
 
+#define MIN(a, b)		((a) < (b) ? (a) : (b))
+#define MAX(a, b)		((a) > (b) ? (a) : (b))
+
 #endif
-- 
2.7.4

[kvm-unit-tests PATCH 07/14] vm/page: provide PGDIR_OFFSET() macro

[Peter Xu]

This can be used in further patches.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/asm/page.h | 3 +++
 lib/x86/vm.c       | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index 5044a49..c43bab2 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -41,5 +41,8 @@
 #define	PGDIR_MASK	1023
 #endif
 
+#define PGDIR_BITS(lvl)        (((lvl) - 1) * PGDIR_WIDTH + PAGE_SHIFT)
+#define PGDIR_OFFSET(va, lvl)  (((va) >> PGDIR_BITS(lvl)) & PGDIR_MASK)
+
 #endif /* !__ASSEMBLY__ */
 #endif
diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 9771bd7..f97d1e5 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -48,7 +48,7 @@ unsigned long *install_pte(unsigned long *cr3,
     unsigned offset;
 
     for (level = PAGE_LEVEL; level > pte_level; --level) {
-	offset = ((unsigned long)virt >> ((level-1) * PGDIR_WIDTH + 12)) & PGDIR_MASK;
+	offset = PGDIR_OFFSET((unsigned long)virt, level);
 	if (!(pt[offset] & PT_PRESENT_MASK)) {
 	    unsigned long *new_pt = pt_page;
             if (!new_pt)
@@ -60,7 +60,7 @@ unsigned long *install_pte(unsigned long *cr3,
 	}
 	pt = phys_to_virt(pt[offset] & PT_ADDR_MASK);
     }
-    offset = ((unsigned long)virt >> ((level-1) * PGDIR_WIDTH + 12)) & PGDIR_MASK;
+    offset = PGDIR_OFFSET((unsigned long)virt, level);
     pt[offset] = pte;
     return &pt[offset];
 }
-- 
2.7.4

[kvm-unit-tests PATCH 08/14] x86: pci: add pci_config_{read|write}[bw]() helpers

[Peter Xu]

And some cleanup on the file.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/asm/pci.h | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
index cddde41..ce970e4 100644
--- a/lib/x86/asm/pci.h
+++ b/lib/x86/asm/pci.h
@@ -9,11 +9,42 @@
 #include "pci.h"
 #include "x86/asm/io.h"
 
+#define PCI_HOST_CONFIG_PORT (0xcf8)
+#define PCI_HOST_DATA_PORT   (0xcfc)
+
+#define PCI_HOST_INDEX(dev, reg)   (reg | (dev << 8) | (0x1 << 31))
+
+#define pci_config_readb(dev, reg) (pci_config_read(dev, reg) & 0xff)
+#define pci_config_readw(dev, reg) (pci_config_read(dev, reg) & 0xffff)
+
 static inline uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
 {
-    uint32_t index = reg | (dev << 8) | (0x1 << 31);
-    outl(index, 0xCF8);
-    return inl(0xCFC);
+	uint32_t index = PCI_HOST_INDEX(dev, reg);
+	outl(index, PCI_HOST_CONFIG_PORT);
+	return inl(PCI_HOST_DATA_PORT);
+}
+
+static inline void pci_config_write(pcidevaddr_t dev, uint8_t reg,
+				    uint32_t val)
+{
+	uint32_t index = PCI_HOST_INDEX(dev, reg);
+	outl(index, PCI_HOST_CONFIG_PORT);
+	outl(val, PCI_HOST_DATA_PORT);
 }
 
+static inline void pci_config_writeb(pcidevaddr_t dev, uint8_t reg,
+				     uint8_t val)
+{
+	uint32_t index = PCI_HOST_INDEX(dev, reg);
+	outl(index, PCI_HOST_CONFIG_PORT);
+	outb(val, PCI_HOST_DATA_PORT);
+}
+
+static inline void pci_config_writew(pcidevaddr_t dev, uint8_t reg,
+				     uint16_t val)
+{
+	uint32_t index = PCI_HOST_INDEX(dev, reg);
+	outl(index, PCI_HOST_CONFIG_PORT);
+	outw(val, PCI_HOST_DATA_PORT);
+}
 #endif
-- 
2.7.4

[kvm-unit-tests PATCH 09/14] pci: provide pci_set_master()

[Peter Xu]

And set master by default for pci devices. So that DMA is allowed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/pci.c b/lib/pci.c
index ef0b02a..044e4db 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -7,6 +7,13 @@
 #include "pci.h"
 #include "asm/pci.h"
 
+static void pci_set_master(pci_dev *dev, int master)
+{
+	uint32_t val = pci_config_read(dev->pci_addr, PCI_COMMAND);
+	val |= PCI_COMMAND_MASTER;
+	pci_config_write(dev->pci_addr, PCI_COMMAND, val);
+}
+
 /*
  * Scan bus look for a specific device. Only bus 0 scanned for now.
  * After the scan, a pci_dev is returned with correct BAR information.
@@ -41,6 +48,8 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
 	}
 	dev->inited = true;
 
+	pci_set_master(dev, 1);
+
 	return 0;
 }
 
-- 
2.7.4

[kvm-unit-tests PATCH 10/14] pci: add bdf helpers

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/pci.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/pci.h b/lib/pci.h
index b8755ff..6a1c3c9 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -18,6 +18,12 @@ enum {
 #define PCI_BAR_NUM                     (6)
 #define PCI_DEVFN_MAX                   (256)
 
+#define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
+#define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
+#define PCI_SLOT(devfn)                 (((devfn) >> 3) & 0x1f)
+#define PCI_FUNC(devfn)                 ((devfn) & 0x07)
+#define PCI_BUILD_BDF(bus, devfn)       ((bus << 8) | (devfn))
+
 struct pci_dev {
 	bool inited;
 	uint16_t pci_addr;
-- 
2.7.4

[kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers

[Peter Xu]

QEMU edu device is a pci device that is originally written for
educational purpose, however it also suits for IOMMU unit test. Adding
helpers for this specific device to implement the device logic.

The device supports lots of functions, here only DMA operation is
supported.

The spec of the device can be found at:

  https://github.com/qemu/qemu/blob/master/docs/specs/edu.txt

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/pci-edu.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-edu.h |  29 +++++++++++++
 2 files changed, 157 insertions(+)
 create mode 100644 lib/pci-edu.c
 create mode 100644 lib/pci-edu.h

diff --git a/lib/pci-edu.c b/lib/pci-edu.c
new file mode 100644
index 0000000..4d1a5ab
--- /dev/null
+++ b/lib/pci-edu.c
@@ -0,0 +1,128 @@
+/*
+ * Edu PCI device.
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * Authors:
+ *   Peter Xu <peterx@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or
+ * later.
+ */
+
+#include "pci-edu.h"
+
+#define  PCI_VENDOR_ID_QEMU              (0x1234)
+#define  PCI_DEVICE_ID_EDU               (0x11e8)
+
+/* The only bar used by EDU device */
+#define EDU_BAR_MEM                 (0)
+#define EDU_MAGIC                   (0xed)
+#define EDU_VERSION                 (0x100)
+#define EDU_DMA_BUF_SIZE            (1 << 20)
+#define EDU_INPUT_BUF_SIZE          (256)
+
+#define EDU_REG_ID                  (0x0)
+#define EDU_REG_ALIVE               (0x4)
+#define EDU_REG_FACTORIAL           (0x8)
+#define EDU_REG_STATUS              (0x20)
+#define EDU_REG_DMA_SRC             (0x80)
+#define EDU_REG_DMA_DST             (0x88)
+#define EDU_REG_DMA_COUNT           (0x90)
+#define EDU_REG_DMA_CMD             (0x98)
+
+#define EDU_CMD_DMA_START           (0x01)
+#define EDU_CMD_DMA_FROM            (0x02)
+#define EDU_CMD_DMA_TO              (0x00)
+
+#define EDU_STATUS_FACTORIAL        (0x1)
+#define EDU_STATUS_INT_ENABLE       (0x80)
+
+#define EDU_DMA_START               (0x40000)
+#define EDU_DMA_SIZE_MAX            (4096)
+
+uint64_t edu_reg_readq(pci_edu_dev_t *dev, int reg)
+{
+	return *(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
+}
+
+uint32_t edu_reg_read(pci_edu_dev_t *dev, int reg)
+{
+	return *(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
+}
+
+void edu_reg_writeq(pci_edu_dev_t *dev, int reg, uint64_t val)
+{
+	*(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
+}
+
+void edu_reg_write(pci_edu_dev_t *dev, int reg, uint32_t val)
+{
+	*(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
+}
+
+/* Return true if alive */
+bool edu_check_alive(pci_edu_dev_t *dev)
+{
+	static uint32_t live_count = 1;
+	uint32_t value;
+
+	edu_reg_write(dev, EDU_REG_ALIVE, live_count++);
+	value = edu_reg_read(dev, EDU_REG_ALIVE);
+	return (live_count - 1 == ~value);
+}
+
+int edu_init(pci_edu_dev_t *dev)
+{
+	int ret;
+
+	ret = pci_dev_init(&dev->pci_dev, PCI_VENDOR_ID_QEMU,
+			   PCI_DEVICE_ID_EDU);
+	if (ret)
+		return ret;
+
+	if (!edu_check_alive(dev)) {
+		printf("edu device not alive!\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+uint32_t edu_status(pci_edu_dev_t *dev)
+{
+	return edu_reg_read(dev, EDU_REG_STATUS);
+}
+
+void edu_dma(pci_edu_dev_t *dev, iova_t iova,
+	     size_t size, int dev_offset, pci_dma_dir_t dir)
+{
+	uint64_t from, to;
+	uint32_t cmd = EDU_CMD_DMA_START;
+
+	assert(size <= EDU_DMA_SIZE_MAX);
+	assert(dev_offset < EDU_DMA_SIZE_MAX &&
+	       dev_offset >= 0);
+
+	printf("edu device DMA start %s addr %p size 0x%lu off 0x%x\n",
+	       dir == PCI_DMA_FROM_DEVICE ? "FROM" : "TO",
+	       (void *)iova, size, dev_offset);
+
+	if (dir == PCI_DMA_FROM_DEVICE) {
+		from = dev_offset + EDU_DMA_START;
+		to = iova;
+		cmd |= EDU_CMD_DMA_FROM;
+	} else {
+		from = iova;
+		to = EDU_DMA_START + dev_offset;
+		cmd |= EDU_CMD_DMA_TO;
+	}
+
+	edu_reg_writeq(dev, EDU_REG_DMA_SRC, from);
+	edu_reg_writeq(dev, EDU_REG_DMA_DST, to);
+	edu_reg_writeq(dev, EDU_REG_DMA_COUNT, size);
+	edu_reg_write(dev, EDU_REG_DMA_CMD, cmd);
+
+	/* Wait until DMA finished */
+	while (edu_reg_read(dev, EDU_REG_DMA_CMD) & EDU_CMD_DMA_START);
+}
diff --git a/lib/pci-edu.h b/lib/pci-edu.h
new file mode 100644
index 0000000..6b7dbfd
--- /dev/null
+++ b/lib/pci-edu.h
@@ -0,0 +1,29 @@
+/*
+ * Edu PCI device header.
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * Authors:
+ *   Peter Xu <peterx@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or
+ * later.
+ *
+ * Edu device is a virtualized device in QEMU. Please refer to
+ * docs/specs/edu.txt in QEMU repository for EDU device manual.
+ */
+#ifndef __PCI_EDU_H__
+#define __PCI_EDU_H__
+
+#include "pci.h"
+
+struct pci_edu_dev {
+	pci_dev pci_dev;
+};
+typedef struct pci_edu_dev pci_edu_dev_t;
+
+int edu_init(pci_edu_dev_t *dev);
+void edu_dma(pci_edu_dev_t *dev, iova_t iova,
+	     size_t size, int dev_offset, pci_dma_dir_t dir);
+
+#endif
-- 
2.7.4

[kvm-unit-tests PATCH 12/14] x86: intel-iommu: add dmar test

[Peter Xu]

DMAR test is based on QEMU edu device. A 4B DMA memory copy is carried
out as the simplest DMAR test.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/x86/intel-iommu.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/x86/intel-iommu.h |  23 +++++++++
 x86/Makefile.common   |   1 +
 x86/intel-iommu.c     |  52 ++++++++++++++++++++
 4 files changed, 208 insertions(+)

diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
index 9f55394..30e2c97 100644
--- a/lib/x86/intel-iommu.c
+++ b/lib/x86/intel-iommu.c
@@ -11,6 +11,42 @@
  */
 
 #include "intel-iommu.h"
+#include "asm-generic/page.h"
+
+/*
+ * VT-d in QEMU currently only support 39 bits address width, which is
+ * 3-level translation.
+ */
+#define VTD_PAGE_LEVEL      (3)
+#define VTD_CE_AW_39BIT     (0x1)
+
+typedef uint64_t vtd_pte_t;
+
+struct vtd_root_entry {
+	/* Quad 1 */
+	uint64_t present:1;
+	uint64_t __reserved:11;
+	uint64_t context_table_p:52;
+	/* Quad 2 */
+	uint64_t __reserved_2;
+} __attribute__ ((packed));
+typedef struct vtd_root_entry vtd_re_t;
+
+struct vtd_context_entry {
+	/* Quad 1 */
+	uint64_t present:1;
+	uint64_t disable_fault_report:1;
+	uint64_t trans_type:2;
+	uint64_t __reserved:8;
+	uint64_t slptptr:52;
+	/* Quad 2 */
+	uint64_t addr_width:3;
+	uint64_t __ignore:4;
+	uint64_t __reserved_2:1;
+	uint64_t domain_id:16;
+	uint64_t __reserved_3:40;
+} __attribute__ ((packed));
+typedef struct vtd_context_entry vtd_ce_t;
 
 void vtd_reg_write_4(unsigned int reg, uint32_t value)
 {
@@ -123,6 +159,102 @@ void vtd_enable_ir(void)
 	vtd_gcmd_or(VTD_GCMD_IR);
 }
 
+static void vtd_install_pte(vtd_pte_t *root, iova_t iova,
+			    phys_addr_t pa, int level_target)
+{
+	int level;
+	unsigned int offset;
+	void *page;
+
+	for (level = VTD_PAGE_LEVEL; level > level_target; level--) {
+		offset = PGDIR_OFFSET(iova, level);
+		if (!(root[offset] & VTD_PTE_RW)) {
+			page = alloc_page();
+			memset(page, 0, PAGE_SIZE);
+			root[offset] = virt_to_phys(page) | VTD_PTE_RW;
+		}
+		root = (uint64_t *)(root[offset] & VTD_PTE_ADDR);
+	}
+
+	offset = PGDIR_OFFSET(iova, level);
+	root[offset] = pa | VTD_PTE_RW;
+	if (level != 1) {
+		/* This is huge page */
+		root[offset] |= VTD_PTE_HUGE;
+	}
+}
+
+#define  VTD_FETCH_VIRT_ADDR(x) \
+	((void *)(((uint64_t)phys_to_virt(x)) >> PAGE_SHIFT))
+
+/**
+ * vtd_map_range: setup IO address mapping for specific memory range
+ *
+ * @sid: source ID of the device to setup
+ * @iova: start IO virtual address
+ * @pa: start physical address
+ * @size: size of the mapping area
+ */
+void vtd_map_range(uint16_t sid, iova_t iova, phys_addr_t pa, size_t size)
+{
+	uint8_t bus_n, devfn;
+	void *slptptr;
+	vtd_ce_t *ce;
+	vtd_re_t *re = phys_to_virt(vtd_root_table());
+
+	assert(PAGE_ALIGNED_4K(iova));
+	assert(PAGE_ALIGNED_4K(pa));
+	assert(PAGE_ALIGNED_4K(size));
+
+	bus_n = PCI_BDF_GET_BUS(sid);
+	devfn = PCI_BDF_GET_DEVFN(sid);
+
+	/* Point to the correct root entry */
+	re += bus_n;
+
+	if (!re->present) {
+		ce = alloc_page();
+		memset(ce, 0, PAGE_SIZE);
+		memset(re, 0, sizeof(*re));
+		re->context_table_p = virt_to_phys(ce) >> PAGE_SHIFT;
+		re->present = 1;
+		printf("allocated vt-d root entry for PCI bus %d\n",
+		       bus_n);
+	} else
+		ce = VTD_FETCH_VIRT_ADDR(re->context_table_p);
+
+	/* Point to the correct context entry */
+	ce += devfn;
+
+	if (!ce->present) {
+		slptptr = alloc_page();
+		memset(slptptr, 0, PAGE_SIZE);
+		memset(ce, 0, sizeof(*ce));
+		/* To make it simple, domain ID is the same as SID */
+		ce->domain_id = sid;
+		/* We only test 39 bits width case (3-level paging) */
+		ce->addr_width = VTD_CE_AW_39BIT;
+		ce->slptptr = virt_to_phys(slptptr) >> PAGE_SHIFT;
+		ce->trans_type = VTD_CONTEXT_TT_MULTI_LEVEL;
+		ce->present = 1;
+		/* No error reporting yet */
+		ce->disable_fault_report = 1;
+		printf("allocated vt-d context entry for devfn 0x%x\n",
+		       devfn);
+	} else
+		slptptr = VTD_FETCH_VIRT_ADDR(ce->slptptr);
+
+	while (size) {
+		/* TODO: currently we only map 4K pages (level = 1) */
+		printf("map 4K page IOVA 0x%lx to 0x%lx (sid=0x%04x)\n",
+		       iova, pa, sid);
+		vtd_install_pte(slptptr, iova, pa, 1);
+		size -= PAGE_SIZE;
+		iova += PAGE_SIZE;
+		pa += PAGE_SIZE;
+	}
+}
+
 void vtd_init(void)
 {
 	setup_vm();
diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
index 0f687d1..f1340cd 100644
--- a/lib/x86/intel-iommu.h
+++ b/lib/x86/intel-iommu.h
@@ -18,6 +18,7 @@
 #include "isr.h"
 #include "smp.h"
 #include "desc.h"
+#include "pci.h"
 
 #define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed90000ULL
 
@@ -84,6 +85,27 @@
 #define VTD_GCMD_ROOT           (0x40000000)
 #define VTD_GCMD_DMAR           (0x80000000)
 
+/* Supported Adjusted Guest Address Widths */
+#define VTD_CAP_SAGAW_SHIFT         8
+ /* 39-bit AGAW, 3-level page-table */
+#define VTD_CAP_SAGAW_39bit         (0x2ULL << VTD_CAP_SAGAW_SHIFT)
+ /* 48-bit AGAW, 4-level page-table */
+#define VTD_CAP_SAGAW_48bit         (0x4ULL << VTD_CAP_SAGAW_SHIFT)
+#define VTD_CAP_SAGAW               VTD_CAP_SAGAW_39bit
+
+/* Both 1G/2M huge pages */
+#define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
+
+#define VTD_CONTEXT_TT_MULTI_LEVEL  0
+#define VTD_CONTEXT_TT_DEV_IOTLB    1
+#define VTD_CONTEXT_TT_PASS_THROUGH 2
+
+#define VTD_PTE_R                   (1 << 0)
+#define VTD_PTE_W                   (1 << 1)
+#define VTD_PTE_RW                  (VTD_PTE_R | VTD_PTE_W)
+#define VTD_PTE_ADDR                GENMASK_ULL(51, 12)
+#define VTD_PTE_HUGE                (1 << 7)
+
 void vtd_reg_write_4(unsigned int reg, uint32_t value);
 void vtd_reg_write_8(unsigned int reg, uint64_t value);
 uint32_t vtd_reg_read_4(unsigned int reg);
@@ -102,5 +124,6 @@ void vtd_setup_ir_table(void);
 void vtd_enable_dmar(void);
 void vtd_enable_ir(void);
 void vtd_init(void);
+void vtd_map_range(uint16_t sid, phys_addr_t iova, phys_addr_t pa, size_t size);
 
 #endif
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 356d879..1dad18b 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -3,6 +3,7 @@
 all: test_cases
 
 cflatobjs += lib/pci.o
+cflatobjs += lib/pci-edu.o
 cflatobjs += lib/x86/io.o
 cflatobjs += lib/x86/smp.o
 cflatobjs += lib/x86/vm.o
diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
index 60d512a..0c3ab9b 100644
--- a/x86/intel-iommu.c
+++ b/x86/intel-iommu.c
@@ -11,9 +11,48 @@
  */
 
 #include "intel-iommu.h"
+#include "pci-edu.h"
+
+void vtd_test_dmar(pci_edu_dev_t *dev)
+{
+	void *page = alloc_page();
+
+#define DMA_TEST_WORD (0x12345678)
+	/* Modify the first 4 bytes of the page */
+	*(uint32_t *)page = DMA_TEST_WORD;
+
+	/*
+	 * Map the newly allocated page into IOVA address 0 (size 4K)
+	 * of the device address space. Root entry and context entry
+	 * will be automatically created when needed.
+	 */
+	vtd_map_range(dev->pci_dev.pci_addr, 0, virt_to_phys(page), PAGE_SIZE);
+
+	/*
+	 * DMA the first 4 bytes of the page to EDU device buffer
+	 * offset 0.
+	 */
+	edu_dma(dev, 0, 4, 0, PCI_DMA_TO_DEVICE);
+	/*
+	 * DMA the first 4 bytes of EDU device buffer into the page
+	 * with offset 4 (so it'll be using 4-7 bytes).
+	 */
+	edu_dma(dev, 4, 4, 0, PCI_DMA_FROM_DEVICE);
+
+	/*
+	 * Check data match between 0-3 bytes and 4-7 bytes of the
+	 * page.
+	 */
+	report("DMAR 4B memcpy test", *((uint32_t *)page + 1) == DMA_TEST_WORD);
+
+	free_page(page);
+}
 
 int main(int argc, char *argv[])
 {
+	int ret;
+	pci_edu_dev_t dev;
+
 	setup_vm();
 	smp_init();
 	setup_idt();
@@ -37,5 +76,18 @@ int main(int argc, char *argv[])
 	vtd_enable_ir();
 	report("IR enablement", vtd_status() & VTD_GCMD_IR);
 
+	report("DMAR support 39 bits address width",
+	       vtd_cap() & VTD_CAP_SAGAW);
+
+	report("DMAR support huge pages", vtd_cap() & VTD_CAP_SLLPS);
+
+	ret = edu_init(&dev);
+	if (ret) {
+		printf("Please specify \"-device edu\" to test IOMMU.\n");
+		return -1;
+	}
+
+	vtd_test_dmar(&dev);
+
 	return report_summary();
 }
-- 
2.7.4

[kvm-unit-tests PATCH 13/14] pci: add msi support for 32/64bit address

[Peter Xu]

During each PCI device init, we walk through all the capability list,
and see what we support. If a cap handler is provided, it'll be
triggered if the cap is detected. MSI cap handler is the first one. We
can add more cap handler in the future.

Meanwhile, pci_setup_msi() API is provided to support basic 32/64 bit
address MSI setup.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/pci.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h |  2 ++
 2 files changed, 61 insertions(+)

diff --git a/lib/pci.c b/lib/pci.c
index 044e4db..1037ae3 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -7,6 +7,35 @@
 #include "pci.h"
 #include "asm/pci.h"
 
+typedef void (*pci_cap_handler)(pci_dev *dev, int cap_offset);
+
+static void pci_cap_msi_handler(pci_dev *dev, int cap_offset)
+{
+	printf("Detected MSI for device 0x%x offset 0x%x\n",
+	       dev->pci_addr, cap_offset);
+	dev->msi_offset = cap_offset;
+}
+
+static pci_cap_handler cap_handlers[0xff] = {
+	[PCI_CAP_ID_MSI] = pci_cap_msi_handler,
+};
+
+static void pci_cap_walk(pci_dev *dev)
+{
+	uint8_t cap_offset;
+	uint8_t cap_id;
+
+	cap_offset = pci_config_readb(dev->pci_addr, PCI_CAPABILITY_LIST);
+	while (cap_offset) {
+		cap_id = pci_config_readb(dev->pci_addr, cap_offset);
+		printf("PCI detected cap 0x%x\n", cap_id);
+		if (cap_handlers[cap_id]) {
+			cap_handlers[cap_id](dev, cap_offset);
+		}
+		cap_offset = pci_config_readb(dev->pci_addr, cap_offset + 1);
+	}
+}
+
 static void pci_set_master(pci_dev *dev, int master)
 {
 	uint32_t val = pci_config_read(dev->pci_addr, PCI_COMMAND);
@@ -14,6 +43,35 @@ static void pci_set_master(pci_dev *dev, int master)
 	pci_config_write(dev->pci_addr, PCI_COMMAND, val);
 }
 
+void pci_setup_msi(pci_dev *dev, uint64_t msi_addr, uint32_t msi_data)
+{
+	uint16_t msi_control;
+	pcidevaddr_t addr;
+	uint16_t offset;
+
+	assert(dev && dev->inited && dev->msi_offset);
+
+	offset = dev->msi_offset;
+	addr = dev->pci_addr;
+	msi_control = pci_config_readw(addr, offset + PCI_MSI_FLAGS);
+	pci_config_write(addr, offset + PCI_MSI_ADDRESS_LO,
+			 msi_addr & 0xffffffff);
+
+	if (msi_control & PCI_MSI_FLAGS_64BIT) {
+		pci_config_write(addr, offset + PCI_MSI_ADDRESS_HI,
+				 (uint32_t)(msi_addr >> 32));
+		pci_config_write(addr, offset + PCI_MSI_DATA_64, msi_data);
+		printf("MSI: dev 0x%x init 64bit address: ", addr);
+	} else {
+		pci_config_write(addr, offset + PCI_MSI_DATA_32, msi_data);
+		printf("MSI: dev 0x%x init 32bit address: ", addr);
+	}
+	printf("addr=0x%lx, data=0x%x\n", msi_addr, msi_data);
+
+	msi_control |= PCI_MSI_FLAGS_ENABLE;
+	pci_config_writew(addr, offset + PCI_MSI_FLAGS, msi_control);
+}
+
 /*
  * Scan bus look for a specific device. Only bus 0 scanned for now.
  * After the scan, a pci_dev is returned with correct BAR information.
@@ -49,6 +107,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
 	dev->inited = true;
 
 	pci_set_master(dev, 1);
+	pci_cap_walk(dev);
 
 	return 0;
 }
diff --git a/lib/pci.h b/lib/pci.h
index 6a1c3c9..5581446 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -27,6 +27,7 @@ enum {
 struct pci_dev {
 	bool inited;
 	uint16_t pci_addr;
+	uint16_t msi_offset;
 	phys_addr_t pci_bar[PCI_BAR_NUM];
 };
 typedef struct pci_dev pci_dev;
@@ -35,6 +36,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
 phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
 bool pci_bar_is_memory(pci_dev *dev, int bar_num);
 bool pci_bar_is_valid(pci_dev *dev, int bar_num);
+void pci_setup_msi(pci_dev *dev, uint64_t msi_addr, uint32_t msi_data);
 
 /*
  * pci-testdev is a driver for the pci-testdev qemu pci device. The
-- 
2.7.4

[kvm-unit-tests PATCH 14/14] x86: intel-iommu: add IR test

[Peter Xu]

First of all, vtd_setup_msi() is provided. It setup IRTE entries,
meanwhile, setup PCI device MSI vectors corresponding to VT-d spec.

The basic IR test is carried out by a edu factorial request. When write
to the factorial register, calculation starts in the background of
device, MSI is triggered as long as it finishes.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 lib/pci-edu.c         | 18 ++++++++--
 lib/pci-edu.h         |  7 ++++
 lib/x86/intel-iommu.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/x86/intel-iommu.h |  1 +
 x86/intel-iommu.c     | 45 +++++++++++++++++++++++++
 5 files changed, 161 insertions(+), 3 deletions(-)

diff --git a/lib/pci-edu.c b/lib/pci-edu.c
index 4d1a5ab..9a90a63 100644
--- a/lib/pci-edu.c
+++ b/lib/pci-edu.c
@@ -35,9 +35,6 @@
 #define EDU_CMD_DMA_FROM            (0x02)
 #define EDU_CMD_DMA_TO              (0x00)
 
-#define EDU_STATUS_FACTORIAL        (0x1)
-#define EDU_STATUS_INT_ENABLE       (0x80)
-
 #define EDU_DMA_START               (0x40000)
 #define EDU_DMA_SIZE_MAX            (4096)
 
@@ -94,6 +91,21 @@ uint32_t edu_status(pci_edu_dev_t *dev)
 	return edu_reg_read(dev, EDU_REG_STATUS);
 }
 
+void edu_setup_intr(pci_edu_dev_t *dev, bool enabled)
+{
+	edu_reg_write(dev, EDU_REG_STATUS, EDU_STATUS_INT_ENABLE);
+}
+
+void edu_fact_write(pci_edu_dev_t *dev, uint32_t in)
+{
+	edu_reg_write(dev, EDU_REG_FACTORIAL, in);
+}
+
+uint32_t edu_fact_read(pci_edu_dev_t *dev)
+{
+	return edu_reg_read(dev, EDU_REG_FACTORIAL);
+}
+
 void edu_dma(pci_edu_dev_t *dev, iova_t iova,
 	     size_t size, int dev_offset, pci_dma_dir_t dir)
 {
diff --git a/lib/pci-edu.h b/lib/pci-edu.h
index 6b7dbfd..dcf5b76 100644
--- a/lib/pci-edu.h
+++ b/lib/pci-edu.h
@@ -22,8 +22,15 @@ struct pci_edu_dev {
 };
 typedef struct pci_edu_dev pci_edu_dev_t;
 
+#define EDU_STATUS_FACTORIAL        (0x1)
+#define EDU_STATUS_INT_ENABLE       (0x80)
+
 int edu_init(pci_edu_dev_t *dev);
 void edu_dma(pci_edu_dev_t *dev, iova_t iova,
 	     size_t size, int dev_offset, pci_dma_dir_t dir);
+void edu_setup_intr(pci_edu_dev_t *dev, bool enabled);
+void edu_fact_write(pci_edu_dev_t *dev, uint32_t in);
+uint32_t edu_fact_read(pci_edu_dev_t *dev);
+uint32_t edu_status(pci_edu_dev_t *dev);
 
 #endif
diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
index 30e2c97..de6c732 100644
--- a/lib/x86/intel-iommu.c
+++ b/lib/x86/intel-iommu.c
@@ -12,6 +12,7 @@
 
 #include "intel-iommu.h"
 #include "asm-generic/page.h"
+#include "pci.h"
 
 /*
  * VT-d in QEMU currently only support 39 bits address width, which is
@@ -48,6 +49,26 @@ struct vtd_context_entry {
 } __attribute__ ((packed));
 typedef struct vtd_context_entry vtd_ce_t;
 
+struct vtd_irte {
+	uint32_t present:1;
+	uint32_t fault_disable:1;    /* Fault Processing Disable */
+	uint32_t dest_mode:1;        /* Destination Mode */
+	uint32_t redir_hint:1;       /* Redirection Hint */
+	uint32_t trigger_mode:1;     /* Trigger Mode */
+	uint32_t delivery_mode:3;    /* Delivery Mode */
+	uint32_t __avail:4;          /* Available spaces for software */
+	uint32_t __reserved_0:3;     /* Reserved 0 */
+	uint32_t irte_mode:1;        /* IRTE Mode */
+	uint32_t vector:8;           /* Interrupt Vector */
+	uint32_t __reserved_1:8;     /* Reserved 1 */
+	uint32_t dest_id;            /* Destination ID */
+	uint16_t source_id:16;       /* Source-ID */
+	uint64_t sid_q:2;            /* Source-ID Qualifier */
+	uint64_t sid_vtype:2;        /* Source-ID Validation Type */
+	uint64_t __reserved_2:44;    /* Reserved 2 */
+} __attribute__ ((packed));
+typedef struct vtd_irte vtd_irte_t;
+
 void vtd_reg_write_4(unsigned int reg, uint32_t value)
 {
 	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
@@ -255,6 +276,78 @@ void vtd_map_range(uint16_t sid, iova_t iova, phys_addr_t pa, size_t size)
 	}
 }
 
+static uint16_t vtd_intr_index_alloc(void)
+{
+	static int index_ctr = 0;
+	assert(index_ctr < 65535);
+	return index_ctr++;
+}
+
+static void vtd_setup_irte(pci_dev *dev, vtd_irte_t *irte,
+			   int vector, int dest_id)
+{
+	assert(sizeof(vtd_irte_t) == 16);
+	memset(irte, 0, sizeof(*irte));
+	irte->fault_disable = 1;
+	irte->dest_mode = 0;	 /* physical */
+	irte->trigger_mode = 0;	 /* edge */
+	irte->delivery_mode = 0; /* fixed */
+	irte->irte_mode = 0;	 /* remapped */
+	irte->vector = vector;
+	irte->dest_id = dest_id;
+	irte->source_id = dev->pci_addr;
+	irte->sid_q = 0;
+	irte->sid_vtype = 1;     /* full-sid verify */
+	irte->present = 1;
+}
+
+struct vtd_msi_addr {
+	uint32_t __dont_care:2;
+	uint32_t handle_15:1;	 /* handle[15] */
+	uint32_t shv:1;
+	uint32_t interrupt_format:1;
+	uint32_t handle_0_14:15; /* handle[0:14] */
+	uint32_t head:12;	 /* 0xfee */
+	uint32_t addr_hi;	 /* not used except with x2apic */
+} __attribute__ ((packed));
+typedef struct vtd_msi_addr vtd_msi_addr_t;
+
+struct vtd_msi_data {
+	uint16_t __reserved;
+	uint16_t subhandle;
+} __attribute__ ((packed));
+typedef struct vtd_msi_data vtd_msi_data_t;
+
+/**
+ * vtd_setup_msi - setup MSI message for a device
+ *
+ * @dev: PCI device to setup MSI
+ * @vector: interrupt vector
+ * @dest_id: destination processor
+ */
+void vtd_setup_msi(pci_dev *dev, int vector, int dest_id)
+{
+	vtd_msi_data_t msi_data = {};
+	vtd_msi_addr_t msi_addr = {};
+	vtd_irte_t *irte = phys_to_virt(vtd_ir_table());
+	uint16_t index = vtd_intr_index_alloc();
+
+	assert(sizeof(vtd_msi_addr_t) == 8);
+	assert(sizeof(vtd_msi_data_t) == 4);
+
+	printf("INTR: setup IRTE index %d\n", index);
+	vtd_setup_irte(dev, irte + index, vector, dest_id);
+
+	msi_addr.handle_15 = index >> 15 & 1;
+	msi_addr.shv = 0;
+	msi_addr.interrupt_format = 1;
+	msi_addr.handle_0_14 = index & 0x7fff;
+	msi_addr.head = 0xfee;
+	msi_data.subhandle = 0;
+
+	pci_setup_msi(dev, *(uint64_t *)&msi_addr, *(uint32_t *)&msi_data);
+}
+
 void vtd_init(void)
 {
 	setup_vm();
diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
index f1340cd..fb6b6a6 100644
--- a/lib/x86/intel-iommu.h
+++ b/lib/x86/intel-iommu.h
@@ -125,5 +125,6 @@ void vtd_enable_dmar(void);
 void vtd_enable_ir(void);
 void vtd_init(void);
 void vtd_map_range(uint16_t sid, phys_addr_t iova, phys_addr_t pa, size_t size);
+void vtd_setup_msi(pci_dev *dev, int vector, int dest_id);
 
 #endif
diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
index 0c3ab9b..c826af8 100644
--- a/x86/intel-iommu.c
+++ b/x86/intel-iommu.c
@@ -12,6 +12,7 @@
 
 #include "intel-iommu.h"
 #include "pci-edu.h"
+#include "x86/apic.h"
 
 void vtd_test_dmar(pci_edu_dev_t *dev)
 {
@@ -48,6 +49,49 @@ void vtd_test_dmar(pci_edu_dev_t *dev)
 	free_page(page);
 }
 
+static uint32_t factorial(uint32_t in)
+{
+	uint32_t v = 1;
+	while (in) v *= in--;
+	return v;
+}
+
+static volatile int edu_done;
+
+static void edu_fact_isr(isr_regs_t *regs)
+{
+	edu_done = 1;
+	eoi();
+}
+
+static void vtd_test_ir(pci_edu_dev_t *dev)
+{
+#define VTD_TEST_VECTOR (0xee)
+	/* Choose any number to calculate the factorial value. */
+	const uint32_t fact = 0x8;
+
+	/*
+	 * Enable EDU device interrupt, so when factorial task
+	 * finishes, IRQ will be triggered.
+	 */
+	edu_setup_intr(dev, 1);
+
+	/*
+	 * Setup EDU PCI device MSI, using interrupt remapping. By
+	 * default, EDU device is using INTx.
+	 */
+	vtd_setup_msi(&dev->pci_dev, VTD_TEST_VECTOR, 0);
+
+	handle_irq(VTD_TEST_VECTOR, edu_fact_isr);
+	irq_enable();
+
+	edu_fact_write(dev, fact);
+	while (!edu_done);
+	/* Now results should be put back to edu fact register */
+	report("EDU factorial INTR test",
+	       edu_fact_read(dev) == factorial(fact));
+}
+
 int main(int argc, char *argv[])
 {
 	int ret;
@@ -88,6 +132,7 @@ int main(int argc, char *argv[])
 	}
 
 	vtd_test_dmar(&dev);
+	vtd_test_ir(&dev);
 
 	return report_summary();
 }
-- 
2.7.4

Re: [kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[Radim Krčmář]

2016-10-14 20:40+0800, Peter Xu:
> This series is adding simplest test cases for VT-d.
> 
> The series contains some conflict with Alex's PCI framework
> enhancement, so I am marking it as RFC, I can rebase to Alex's work
> after it's merged. Besides the conflict, the codes is workable, and
> passed smoke test.
> 
> Currently only a very small test scope is covered:
> 
> * VT-d init
> * DMAR: 4 bytes copy
> * IR: MSI
> 
> However this series could be a base point to add more test cases for
> VT-d.

The size already scared away reviewers, so slowly extending it is a good
idea. :)

>       The problem is, there are many IOMMU error conditions which are
> very hard to be triggered in a real guest (IOMMU has merely no
> interface for guest user, and it's totally running in the background).

I gracefully skipped most of VT-d error handling ... how many of those
errors are not a result of a misconfiguration?

> This piece of work can be a start point if we want to do more
> complicated things and play around with Intel IOMMU devices (also for
> IOMMU regression tests).
> 
> Please review. Thanks,
> 
> =================
> 
> To run the test:
> 
> ./x86/run ./x86/intel-iommu.flat \
>     -M q35,kernel-irqchip=split -global ioapic.version=0x20 \
>     -device intel-iommu,intremap=on -device edu

This command line deserved an entry in x86/unittests.cfg, it will run
the test automatically with ./run_tests.sh and serve as a refererence
for manual execution.

Thanks.

> 
> Sample output:
> 
> pxdev:kvm-unit-tests [new-iommu-ut]# ./iommu_run.sh
> /root/git/qemu/bin/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -kernel ./x86/intel-iommu.flat -M q35,kernel-irqchip=split -global ioapic.version=0x20 -device intel-iommu,intremap=on -device edu
> enabling apic
> paging enabled
> cr0 = 80010011
> cr3 = 7fff000
> cr4 = 20
> VT-d version:   0x10
>      cap:       0x0012008c22260206
>      ecap:      0x0000000000f00f1a
> PASS: init status check
> PASS: fault status check
> PASS: QI enablement
> DMAR table address: 0x0000000007ff9000
> PASS: DMAR table setup
> IR table address: 0x0000000007ff8000
> PASS: IR table setup
> PASS: DMAR enablement
> PASS: IR enablement
> PASS: DMAR support 39 bits address width
> PASS: DMAR support huge pages
> PCI: init dev 0x0020 BAR 0 [MEM] addr 0xfea00000
> PCI detected cap 0x5
> Detected MSI for device 0x20 offset 0x40
> allocated vt-d root entry for PCI bus 0
> allocated vt-d context entry for devfn 0x20
> map 4K page IOVA 0x0 to 0x7ff7000 (sid=0x0020)
> edu device DMA start TO addr 0x0 size 0x4 off 0x0
> edu device DMA start FROM addr 0x4 size 0x4 off 0x0
> PASS: DMAR 4B memcpy test
> INTR: setup IRTE index 0
> MSI: dev 0x20 init 64bit address: addr=0xfee00010, data=0x0
> PASS: EDU factorial INTR test
> 
> SUMMARY: 11 tests
> 
> =================
> 
> Peter Xu (14):
>   x86: vm: allow multiple init for vm setup
>   x86: smp: allow multiple init for smp setup
>   x86: intel-iommu: add vt-d init test
>   pci: refactor init process to pci_dev_init()
>   page: add page alignment checker
>   util: move MAX/MIN macro into util.h
>   vm/page: provide PGDIR_OFFSET() macro
>   x86: pci: add pci_config_{read|write}[bw]() helpers
>   pci: provide pci_set_master()
>   pci: add bdf helpers
>   pci: edu: introduce pci-edu helpers
>   x86: intel-iommu: add dmar test
>   pci: add msi support for 32/64bit address
>   x86: intel-iommu: add IR test
> 
>  lib/alloc.c            |   3 -
>  lib/asm-generic/page.h |   5 +
>  lib/pci-edu.c          | 140 +++++++++++++++++++
>  lib/pci-edu.h          |  36 +++++
>  lib/pci.c              | 143 +++++++++++++++----
>  lib/pci.h              |  37 ++++-
>  lib/util.h             |   3 +
>  lib/x86/asm/page.h     |   3 +
>  lib/x86/asm/pci.h      |  37 ++++-
>  lib/x86/intel-iommu.c  | 363 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/x86/intel-iommu.h  | 130 ++++++++++++++++++
>  lib/x86/smp.c          |   6 +
>  lib/x86/vm.c           |  11 +-
>  x86/Makefile.common    |   1 +
>  x86/Makefile.x86_64    |   2 +
>  x86/intel-iommu.c      | 138 +++++++++++++++++++
>  x86/vmexit.c           |  22 +--
>  17 files changed, 1026 insertions(+), 54 deletions(-)
>  create mode 100644 lib/pci-edu.c
>  create mode 100644 lib/pci-edu.h
>  create mode 100644 lib/x86/intel-iommu.c
>  create mode 100644 lib/x86/intel-iommu.h
>  create mode 100644 x86/intel-iommu.c
> 
> -- 
> 2.7.4
> 

Re: [kvm-unit-tests PATCH 02/14] x86: smp: allow multiple init for smp setup

[Radim Krčmář]

2016-10-14 20:40+0800, Peter Xu:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/smp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/x86/smp.c b/lib/x86/smp.c
> index 1eb49f2..bb74087 100644
> --- a/lib/x86/smp.c
> +++ b/lib/x86/smp.c
> @@ -111,8 +111,13 @@ void on_cpu_async(int cpu, void (*function)(void *data), void *data)
>  void smp_init(void)
>  {
>      int i;
> +    bool smp_inited = false;

Missing "static".

The first two patches are just for sanity and not needed in the series?

>      void ipi_entry(void);
>  
> +    if (smp_inited) {
> +        return;
> +    }
> +
>      _cpu_count = fwcfg_get_nb_cpus();
>  
>      setup_idt();
> @@ -122,4 +127,5 @@ void smp_init(void)
>      for (i = 1; i < cpu_count(); ++i)
>          on_cpu(i, setup_smp_id, 0);
>  
> +    smp_inited = true;
>  }
> -- 
> 2.7.4
> 

Re: [kvm-unit-tests PATCH 12/14] x86: intel-iommu: add dmar test

[Radim Krčmář]

2016-10-14 20:40+0800, Peter Xu:
> DMAR test is based on QEMU edu device. A 4B DMA memory copy is carried
> out as the simplest DMAR test.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

[The code made me realize that I won't be able to do a proper review
 right now ...]

>  int main(int argc, char *argv[])
>  {
> +	int ret;
> +	pci_edu_dev_t dev;
> +
>  	setup_vm();
>  	smp_init();
>  	setup_idt();
> @@ -37,5 +76,18 @@ int main(int argc, char *argv[])
>  	vtd_enable_ir();
>  	report("IR enablement", vtd_status() & VTD_GCMD_IR);
>  
> +	report("DMAR support 39 bits address width",
> +	       vtd_cap() & VTD_CAP_SAGAW);
> +
> +	report("DMAR support huge pages", vtd_cap() & VTD_CAP_SLLPS);
> +
> +	ret = edu_init(&dev);
> +	if (ret) {
> +		printf("Please specify \"-device edu\" to test IOMMU.\n");
> +		return -1;

The test did something before this point, so we should print a summary
before exiting.  Probably the best thing would be to report_skip() the
following tests, because the device needed for them couldn't be found.

> +	}
> +
> +	vtd_test_dmar(&dev);
> +
>  	return report_summary();
>  }
> -- 
> 2.7.4
> 

Re: [kvm-unit-tests PATCH 02/14] x86: smp: allow multiple init for smp setup

[Peter Xu]

On Wed, Oct 19, 2016 at 10:23:55PM +0200, Radim Krčmář wrote:
> 2016-10-14 20:40+0800, Peter Xu:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/x86/smp.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/lib/x86/smp.c b/lib/x86/smp.c
> > index 1eb49f2..bb74087 100644
> > --- a/lib/x86/smp.c
> > +++ b/lib/x86/smp.c
> > @@ -111,8 +111,13 @@ void on_cpu_async(int cpu, void (*function)(void *data), void *data)
> >  void smp_init(void)
> >  {
> >      int i;
> > +    bool smp_inited = false;
> 
> Missing "static".

Oops. Will fix.

> 
> The first two patches are just for sanity and not needed in the series?

Yes actually these are not exactly related to vt-d, but good to have
along the way, just like setup_idt() does.

E.g., if one calls the new vtd_init() in future tests to init VT-d
environment, he needs to make sure himself not to call the smp/vm init
function twice (these things are required by VT-d and inited in
vtd_init() already). These two patches will make sure it works in all
cases.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 12/14] x86: intel-iommu: add dmar test

[Peter Xu]

On Wed, Oct 19, 2016 at 10:33:53PM +0200, Radim Krčmář wrote:
> 2016-10-14 20:40+0800, Peter Xu:

[...]

> >  int main(int argc, char *argv[])
> >  {
> > +	int ret;
> > +	pci_edu_dev_t dev;
> > +
> >  	setup_vm();
> >  	smp_init();
> >  	setup_idt();
> > @@ -37,5 +76,18 @@ int main(int argc, char *argv[])
> >  	vtd_enable_ir();
> >  	report("IR enablement", vtd_status() & VTD_GCMD_IR);
> >  
> > +	report("DMAR support 39 bits address width",
> > +	       vtd_cap() & VTD_CAP_SAGAW);
> > +
> > +	report("DMAR support huge pages", vtd_cap() & VTD_CAP_SLLPS);
> > +
> > +	ret = edu_init(&dev);
> > +	if (ret) {
> > +		printf("Please specify \"-device edu\" to test IOMMU.\n");
> > +		return -1;
> 
> The test did something before this point, so we should print a summary
> before exiting.  Probably the best thing would be to report_skip() the
> following tests, because the device needed for them couldn't be found.

Sounds reasonable. Will fix. Thanks!

-- peterx

Re: [kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[Peter Xu]

On Wed, Oct 19, 2016 at 10:21:11PM +0200, Radim Krčmář wrote:
> 2016-10-14 20:40+0800, Peter Xu:
> > This series is adding simplest test cases for VT-d.
> > 
> > The series contains some conflict with Alex's PCI framework
> > enhancement, so I am marking it as RFC, I can rebase to Alex's work
> > after it's merged. Besides the conflict, the codes is workable, and
> > passed smoke test.
> > 
> > Currently only a very small test scope is covered:
> > 
> > * VT-d init
> > * DMAR: 4 bytes copy
> > * IR: MSI
> > 
> > However this series could be a base point to add more test cases for
> > VT-d.
> 
> The size already scared away reviewers, so slowly extending it is a good
> idea. :)

Noted. Thanks for the hint. :)

> 
> >       The problem is, there are many IOMMU error conditions which are
> > very hard to be triggered in a real guest (IOMMU has merely no
> > interface for guest user, and it's totally running in the background).
> 
> I gracefully skipped most of VT-d error handling ... how many of those
> errors are not a result of a misconfiguration?

Do you mean the error handling codes in hw/i386/intel_iommu.c? IMHO
most of those errors will be triggered only if there is bug in guest
OS IOMMU driver.

Here when I talked about error conditions, I mostly meant potential
QEMU IOMMU bugs, not guest OS bugs (of course, AFAICT kvm-unit-tests
are not for guest OS bugs). For example, there are still bugs in IOMMU
IR, and some of the bugs can hardly be triggered by guest OS. In that
case, we need this unit test to reproduce the bug, and re-run it to
verify when I have a fix. Otherwise even if I fixed the bug one day, I
can never reproduce it, nor can I know whether the fix works.

> 
> > This piece of work can be a start point if we want to do more
> > complicated things and play around with Intel IOMMU devices (also for
> > IOMMU regression tests).
> > 
> > Please review. Thanks,
> > 
> > =================
> > 
> > To run the test:
> > 
> > ./x86/run ./x86/intel-iommu.flat \
> >     -M q35,kernel-irqchip=split -global ioapic.version=0x20 \
> >     -device intel-iommu,intremap=on -device edu
> 
> This command line deserved an entry in x86/unittests.cfg, it will run
> the test automatically with ./run_tests.sh and serve as a refererence
> for manual execution.

I will add one more patch for it.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:39PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/vm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 906fbf2..9771bd7 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -151,9 +151,16 @@ static void setup_mmu(unsigned long len)
>  
>  void setup_vm()
>  {
> +    static bool vm_inited = false;
> +
> +    if (vm_inited) {
> +        return;
> +    }
> +
>      end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
>      free_memory(&edata, end_of_memory - (unsigned long)&edata);
>      setup_mmu(end_of_memory);
> +    vm_inited = true;
>  }
>  
>  void *vmalloc(unsigned long size)
> -- 
> 2.7.4
>

My preference for kvm-unit-tests is that we avoid tolerating
unit tests that do things "wrong". This patch allows setup_vm
to be called multiple times, but why do that? Why not just
ensure it's only called once? If it looks like a mistake that's
easy to make and hard to debug, then maybe we need an assert

 assert(!end_of_memory); /* ensure we haven't already called setup_vm */

or something.

Thanks,
drew

Re: [kvm-unit-tests PATCH 02/14] x86: smp: allow multiple init for smp setup

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:40PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/smp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/x86/smp.c b/lib/x86/smp.c
> index 1eb49f2..bb74087 100644
> --- a/lib/x86/smp.c
> +++ b/lib/x86/smp.c
> @@ -111,8 +111,13 @@ void on_cpu_async(int cpu, void (*function)(void *data), void *data)
>  void smp_init(void)
>  {
>      int i;
> +    bool smp_inited = false;
>      void ipi_entry(void);
>  
> +    if (smp_inited) {
> +        return;
> +    }
> +
>      _cpu_count = fwcfg_get_nb_cpus();
>  
>      setup_idt();
> @@ -122,4 +127,5 @@ void smp_init(void)
>      for (i = 1; i < cpu_count(); ++i)
>          on_cpu(i, setup_smp_id, 0);
>  
> +    smp_inited = true;
>  }
> -- 
> 2.7.4

Same comment as last patch. How about

 assert(!_cpu_count)

Thanks,
drew

Re: [kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Peter Xu]

On Thu, Oct 20, 2016 at 10:17:07AM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:39PM +0800, Peter Xu wrote:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/x86/vm.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> > index 906fbf2..9771bd7 100644
> > --- a/lib/x86/vm.c
> > +++ b/lib/x86/vm.c
> > @@ -151,9 +151,16 @@ static void setup_mmu(unsigned long len)
> >  
> >  void setup_vm()
> >  {
> > +    static bool vm_inited = false;
> > +
> > +    if (vm_inited) {
> > +        return;
> > +    }
> > +
> >      end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
> >      free_memory(&edata, end_of_memory - (unsigned long)&edata);
> >      setup_mmu(end_of_memory);
> > +    vm_inited = true;
> >  }
> >  
> >  void *vmalloc(unsigned long size)
> > -- 
> > 2.7.4
> >
> 
> My preference for kvm-unit-tests is that we avoid tolerating
> unit tests that do things "wrong". This patch allows setup_vm
> to be called multiple times, but why do that? Why not just
> ensure it's only called once? If it looks like a mistake that's
> easy to make and hard to debug, then maybe we need an assert
> 
>  assert(!end_of_memory); /* ensure we haven't already called setup_vm */
> 
> or something.

Yeah, sure. The first two patches are just something good to have IMO,
I did it since I see we have similar thing in setup_idt(), and I feel
it good. I can replace it with an assertion.

Thanks.

-- peterx

Re: [kvm-unit-tests PATCH 02/14] x86: smp: allow multiple init for smp setup

[Peter Xu]

On Thu, Oct 20, 2016 at 10:20:30AM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:40PM +0800, Peter Xu wrote:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/x86/smp.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/lib/x86/smp.c b/lib/x86/smp.c
> > index 1eb49f2..bb74087 100644
> > --- a/lib/x86/smp.c
> > +++ b/lib/x86/smp.c
> > @@ -111,8 +111,13 @@ void on_cpu_async(int cpu, void (*function)(void *data), void *data)
> >  void smp_init(void)
> >  {
> >      int i;
> > +    bool smp_inited = false;
> >      void ipi_entry(void);
> >  
> > +    if (smp_inited) {
> > +        return;
> > +    }
> > +
> >      _cpu_count = fwcfg_get_nb_cpus();
> >  
> >      setup_idt();
> > @@ -122,4 +127,5 @@ void smp_init(void)
> >      for (i = 1; i < cpu_count(); ++i)
> >          on_cpu(i, setup_smp_id, 0);
> >  
> > +    smp_inited = true;
> >  }
> > -- 
> > 2.7.4
> 
> Same comment as last patch. How about
> 
>  assert(!_cpu_count)

Will fix. Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Andrew Jones]

On Thu, Oct 20, 2016 at 04:24:08PM +0800, Peter Xu wrote:
> On Thu, Oct 20, 2016 at 10:17:07AM +0200, Andrew Jones wrote:
> > On Fri, Oct 14, 2016 at 08:40:39PM +0800, Peter Xu wrote:
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  lib/x86/vm.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> > > index 906fbf2..9771bd7 100644
> > > --- a/lib/x86/vm.c
> > > +++ b/lib/x86/vm.c
> > > @@ -151,9 +151,16 @@ static void setup_mmu(unsigned long len)
> > >  
> > >  void setup_vm()
> > >  {
> > > +    static bool vm_inited = false;
> > > +
> > > +    if (vm_inited) {
> > > +        return;
> > > +    }
> > > +
> > >      end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
> > >      free_memory(&edata, end_of_memory - (unsigned long)&edata);
> > >      setup_mmu(end_of_memory);
> > > +    vm_inited = true;
> > >  }
> > >  
> > >  void *vmalloc(unsigned long size)
> > > -- 
> > > 2.7.4
> > >
> > 
> > My preference for kvm-unit-tests is that we avoid tolerating
> > unit tests that do things "wrong". This patch allows setup_vm
> > to be called multiple times, but why do that? Why not just
> > ensure it's only called once? If it looks like a mistake that's
> > easy to make and hard to debug, then maybe we need an assert
> > 
> >  assert(!end_of_memory); /* ensure we haven't already called setup_vm */
> > 
> > or something.
> 
> Yeah, sure. The first two patches are just something good to have IMO,
> I did it since I see we have similar thing in setup_idt(), and I feel
> it good. I can replace it with an assertion.

Let's do these patches separate from the series and maybe change setup_idt
too? It'd be interesting to see if the assert in setup_idt would fire,
i.e. if any users are relying on it being tolerant to multiple calls, and
then find out why.

drew

Re: [kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Peter Xu]

On Thu, Oct 20, 2016 at 10:41:37AM +0200, Andrew Jones wrote:

[...]

> Let's do these patches separate from the series and maybe change setup_idt
> too? It'd be interesting to see if the assert in setup_idt would fire,
> i.e. if any users are relying on it being tolerant to multiple calls, and
> then find out why.

The problem should be: smp_init() is calling setup_idt(). So if we
change the init stuff in setup_idt() into an assertion, any test
program that calls both smp_init() and setup_idt() would possibly fail
the assertion. Actually I see most test cases are using:

	setup_vm();
	smp_init();
	setup_idt();

to setup a basic environment, so I guess all of these use cases would
fail. In that sense, I'd slightly prefer keep setup_idt() as it is.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:41PM +0800, Peter Xu wrote:
> Adding fundamental init test for Intel IOMMU. This includes basic
> initialization of Intel IOMMU device, like DMAR (DMA Remapping),
> IR (Interrupt Remapping), QI (Queue Invalidation), etc.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/intel-iommu.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/x86/intel-iommu.h | 106 ++++++++++++++++++++++++++++++++++++++
>  x86/Makefile.x86_64   |   2 +
>  x86/intel-iommu.c     |  41 +++++++++++++++
>  4 files changed, 287 insertions(+)
>  create mode 100644 lib/x86/intel-iommu.c
>  create mode 100644 lib/x86/intel-iommu.h
>  create mode 100644 x86/intel-iommu.c
> 
> diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
> new file mode 100644
> index 0000000..9f55394
> --- /dev/null
> +++ b/lib/x86/intel-iommu.c
> @@ -0,0 +1,138 @@
> +/*
> + * Intel IOMMU APIs
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + *   Peter Xu <peterx@redhat.com>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#include "intel-iommu.h"
> +
> +void vtd_reg_write_4(unsigned int reg, uint32_t value)
> +{
> +	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> +}
> +
> +void vtd_reg_write_8(unsigned int reg, uint64_t value)
> +{
> +	*(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> +}
> +
> +uint32_t vtd_reg_read_4(unsigned int reg)
> +{
> +	return *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> +}
> +
> +uint64_t vtd_reg_read_8(unsigned int reg)
> +{
> +	return *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> +}

The above could be static inlines in intel-iommu.h. How about
calling them vtd_readl, vtd_readq, vtd_writel, vtd_writeq? That
would make them more consistent with the readl, readq... in
lib/asm-generic/io.h

> +
> +uint32_t vtd_version(void)
> +{
> +	return vtd_reg_read_4(DMAR_VER_REG);
> +}
> +
> +uint64_t vtd_cap(void)
> +{
> +	return vtd_reg_read_8(DMAR_CAP_REG);
> +}
> +
> +uint64_t vtd_ecap(void)
> +{
> +	return vtd_reg_read_8(DMAR_ECAP_REG);
> +}
> +
> +uint32_t vtd_status(void)
> +{
> +	return vtd_reg_read_4(DMAR_GSTS_REG);
> +}
> +
> +uint32_t vtd_fault_status(void)
> +{
> +	return vtd_reg_read_4(DMAR_FSTS_REG);
> +}
> +
> +uint64_t vtd_root_table(void)
> +{
> +	/* No extend root table support yet */
> +	return vtd_reg_read_8(DMAR_RTADDR_REG);
> +}

I'd drop all the above and use vtd_readl/q(DMAR_CAP_REG) directly,
the wrappers don't add anything and take the unit test writer
further away from the register names that they should be more
familiar with.

> +
> +uint64_t vtd_ir_table(void)
> +{
> +	return vtd_reg_read_8(DMAR_IRTA_REG) & 0xfffffffffffff000;
> +}

This one has a mask, so makes sense to keep the wrapper. But the
mask looks like PAGE_MASK to me. Why not use that? Or even drop
this wrapper and just use PAGE_MASK when necessary directly...

> +
> +void vtd_gcmd_or(uint32_t cmd)
> +{
> +	uint32_t status = vtd_status();
> +	/*
> +	 * Logically, we need to read DMAR_GSTS_REG until IOMMU
> +	 * handles the write request. However for QEMU/KVM, this write
> +	 * is always sync, so when this returns, we should be sure

should always be in sync. Sounds like something to unit test :-)

> +	 * that the GCMD write is done.
> +	 */
> +	vtd_reg_write_4(DMAR_GCMD_REG, status | cmd);
> +}
> +
> +void vtd_dump_init_info(void)
> +{
> +	printf("VT-d version:   0x%x\n", vtd_version());
> +	printf("     cap:       0x%016lx\n", vtd_cap());
> +	printf("     ecap:      0x%016lx\n", vtd_ecap());
> +}
> +
> +void vtd_enable_qi(void)
> +{
> +	vtd_gcmd_or(VTD_GCMD_QI);
> +}

I'd drop this wrapper and use a comment where it's needed like

 vtd_gcmd_or(VTD_GCMD_QI); /* enable QI */

(By now you see a theme. Keep wrappers to a minimal in order
 to keep the unit test writer closer to the spec.)

> +
> +void vtd_setup_root_table(void)
> +{
> +	void *root = alloc_page();
> +
> +	memset(root, 0, PAGE_SIZE);
> +	vtd_reg_write_8(DMAR_RTADDR_REG, virt_to_phys(root));
> +	vtd_gcmd_or(VTD_GCMD_ROOT);
> +	printf("DMAR table address: 0x%016lx\n", vtd_root_table());
> +}
> +
> +void vtd_setup_ir_table(void)
> +{
> +	void *root = alloc_page();
> +
> +	memset(root, 0, PAGE_SIZE);
> +	vtd_reg_write_8(DMAR_IRTA_REG, virt_to_phys(root));
> +	/* 0xf stands for table size (2^(0xf+1) == 65536) */
> +	vtd_gcmd_or(VTD_GCMD_IR_TABLE | 0xf);
> +	printf("IR table address: 0x%016lx\n", vtd_ir_table());
> +}
> +
> +void vtd_enable_dmar(void)
> +{
> +	vtd_gcmd_or(VTD_GCMD_DMAR);
> +}
> +
> +void vtd_enable_ir(void)
> +{
> +	vtd_gcmd_or(VTD_GCMD_IR);
> +}

More wrappers to drop.

> +
> +void vtd_init(void)
> +{
> +	setup_vm();
> +	smp_init();
> +	setup_idt();
> +
> +	vtd_dump_init_info();
> +	vtd_enable_qi();
> +	vtd_setup_root_table();
> +	vtd_setup_ir_table();
> +	vtd_enable_dmar();
> +	vtd_enable_ir();

I'd open code all the above here. The functions aren't really
gaining us anything that comments can't do for us.

> +}
> diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
> new file mode 100644
> index 0000000..0f687d1
> --- /dev/null
> +++ b/lib/x86/intel-iommu.h
> @@ -0,0 +1,106 @@
> +/*
> + * Intel IOMMU header

Please point out the kernel source these defines come from,
include/linux/intel-iommu.h

> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + *   Peter Xu <peterx@redhat.com>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#ifndef __INTEL_IOMMU_H__
> +#define __INTEL_IOMMU_H__
> +
> +#include "libcflat.h"
> +#include "vm.h"
> +#include "isr.h"
> +#include "smp.h"
> +#include "desc.h"
> +
> +#define Q35_HOST_BRIDGE_IOMMU_ADDR  0xfed90000ULL
> +
> +/*
> + * Intel IOMMU register specification
> + */
> +#define DMAR_VER_REG            0x0  /* Arch version supported by this IOMMU */
> +#define DMAR_CAP_REG            0x8  /* Hardware supported capabilities */
> +#define DMAR_CAP_REG_HI         0xc  /* High 32-bit of DMAR_CAP_REG */
> +#define DMAR_ECAP_REG           0x10 /* Extended capabilities supported */
> +#define DMAR_ECAP_REG_HI        0X14
> +#define DMAR_GCMD_REG           0x18 /* Global command */
> +#define DMAR_GSTS_REG           0x1c /* Global status */
> +#define DMAR_RTADDR_REG         0x20 /* Root entry table */
> +#define DMAR_RTADDR_REG_HI      0X24
> +#define DMAR_CCMD_REG           0x28 /* Context command */
> +#define DMAR_CCMD_REG_HI        0x2c
> +#define DMAR_FSTS_REG           0x34 /* Fault status */
> +#define DMAR_FECTL_REG          0x38 /* Fault control */
> +#define DMAR_FEDATA_REG         0x3c /* Fault event interrupt data */
> +#define DMAR_FEADDR_REG         0x40 /* Fault event interrupt addr */
> +#define DMAR_FEUADDR_REG        0x44 /* Upper address */
> +#define DMAR_AFLOG_REG          0x58 /* Advanced fault control */
> +#define DMAR_AFLOG_REG_HI       0X5c
> +#define DMAR_PMEN_REG           0x64 /* Enable protected memory region */
> +#define DMAR_PLMBASE_REG        0x68 /* PMRR low addr */
> +#define DMAR_PLMLIMIT_REG       0x6c /* PMRR low limit */
> +#define DMAR_PHMBASE_REG        0x70 /* PMRR high base addr */
> +#define DMAR_PHMBASE_REG_HI     0X74
> +#define DMAR_PHMLIMIT_REG       0x78 /* PMRR high limit */
> +#define DMAR_PHMLIMIT_REG_HI    0x7c
> +#define DMAR_IQH_REG            0x80 /* Invalidation queue head */
> +#define DMAR_IQH_REG_HI         0X84
> +#define DMAR_IQT_REG            0x88 /* Invalidation queue tail */
> +#define DMAR_IQT_REG_HI         0X8c
> +#define DMAR_IQA_REG            0x90 /* Invalidation queue addr */
> +#define DMAR_IQA_REG_HI         0x94
> +#define DMAR_ICS_REG            0x9c /* Invalidation complete status */
> +#define DMAR_IRTA_REG           0xb8 /* Interrupt remapping table addr */
> +#define DMAR_IRTA_REG_HI        0xbc
> +#define DMAR_IECTL_REG          0xa0 /* Invalidation event control */
> +#define DMAR_IEDATA_REG         0xa4 /* Invalidation event data */
> +#define DMAR_IEADDR_REG         0xa8 /* Invalidation event address */
> +#define DMAR_IEUADDR_REG        0xac /* Invalidation event address */
> +#define DMAR_PQH_REG            0xc0 /* Page request queue head */
> +#define DMAR_PQH_REG_HI         0xc4
> +#define DMAR_PQT_REG            0xc8 /* Page request queue tail*/
> +#define DMAR_PQT_REG_HI         0xcc
> +#define DMAR_PQA_REG            0xd0 /* Page request queue address */
> +#define DMAR_PQA_REG_HI         0xd4
> +#define DMAR_PRS_REG            0xdc /* Page request status */
> +#define DMAR_PECTL_REG          0xe0 /* Page request event control */
> +#define DMAR_PEDATA_REG         0xe4 /* Page request event data */
> +#define DMAR_PEADDR_REG         0xe8 /* Page request event address */
> +#define DMAR_PEUADDR_REG        0xec /* Page event upper address */
> +#define DMAR_MTRRCAP_REG        0x100 /* MTRR capability */
> +#define DMAR_MTRRCAP_REG_HI     0x104
> +#define DMAR_MTRRDEF_REG        0x108 /* MTRR default type */
> +#define DMAR_MTRRDEF_REG_HI     0x10c
> +
> +#define VTD_GCMD_IR_TABLE       (0x1000000)
> +#define VTD_GCMD_IR             (0x2000000)
> +#define VTD_GCMD_QI             (0x4000000)
> +#define VTD_GCMD_ROOT           (0x40000000)
> +#define VTD_GCMD_DMAR           (0x80000000)
> +
> +void vtd_reg_write_4(unsigned int reg, uint32_t value);
> +void vtd_reg_write_8(unsigned int reg, uint64_t value);
> +uint32_t vtd_reg_read_4(unsigned int reg);
> +uint64_t vtd_reg_read_8(unsigned int reg);
> +uint32_t vtd_version(void);
> +uint64_t vtd_cap(void);
> +uint64_t vtd_ecap(void);
> +uint32_t vtd_status(void);
> +uint32_t vtd_fault_status(void);
> +uint64_t vtd_root_table(void);
> +uint64_t vtd_ir_table(void);
> +void vtd_dump_init_info(void);
> +void vtd_enable_qi(void);
> +void vtd_setup_root_table(void);
> +void vtd_setup_ir_table(void);
> +void vtd_enable_dmar(void);
> +void vtd_enable_ir(void);
> +void vtd_init(void);
> +
> +#endif
> diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
> index e166911..a8e9445 100644
> --- a/x86/Makefile.x86_64
> +++ b/x86/Makefile.x86_64
> @@ -4,6 +4,7 @@ ldarch = elf64-x86-64
>  CFLAGS += -mno-red-zone
>  
>  cflatobjs += lib/x86/setjmp64.o
> +cflatobjs += lib/x86/intel-iommu.o
>  
>  tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
>  	  $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
> @@ -14,6 +15,7 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
>  tests += $(TEST_DIR)/svm.flat
>  tests += $(TEST_DIR)/vmx.flat
>  tests += $(TEST_DIR)/tscdeadline_latency.flat
> +tests += $(TEST_DIR)/intel-iommu.flat
>  
>  include $(TEST_DIR)/Makefile.common
>  
> diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
> new file mode 100644
> index 0000000..60d512a
> --- /dev/null
> +++ b/x86/intel-iommu.c
> @@ -0,0 +1,41 @@
> +/*
> + * Intel IOMMU unit test.
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + *   Peter Xu <peterx@redhat.com>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#include "intel-iommu.h"
> +
> +int main(int argc, char *argv[])
> +{
> +	setup_vm();
> +	smp_init();
> +	setup_idt();
> +
> +	vtd_dump_init_info();
> +	report("init status check", vtd_status() == 0);
> +	report("fault status check", vtd_fault_status() == 0);
> +
> +	vtd_enable_qi();
> +	report("QI enablement", vtd_status() | VTD_GCMD_QI);
> +
> +	vtd_setup_root_table();
> +	report("DMAR table setup", vtd_status() | VTD_GCMD_ROOT);
> +
> +	vtd_setup_ir_table();
> +	report("IR table setup", vtd_status() | VTD_GCMD_IR_TABLE);

The above three report() calls will always pass since you're using OR.

> +
> +	vtd_enable_dmar();
> +	report("DMAR enablement", vtd_status() & VTD_GCMD_DMAR);
> +
> +	vtd_enable_ir();
> +	report("IR enablement", vtd_status() & VTD_GCMD_IR);

You've reproduced vtd_init() here. Why not just call it and then
do a sequence of report() calls where you check each register
has the expected value? I.e.

 vtd_init();
 status = vtd_readl(DMAR_GSTS_REG);
 report("QI enablement", status & VTD_GCMD_QI);
 report("DMAR table setup", status & VTD_GCMD_ROOT);
 ...

Would that not work with this device?

> +
> +	return report_summary();
> +}
> -- 
> 2.7.4
> 

Thanks,
drew

Re: [kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Andrew Jones]

On Thu, Oct 20, 2016 at 04:55:22PM +0800, Peter Xu wrote:
> On Thu, Oct 20, 2016 at 10:41:37AM +0200, Andrew Jones wrote:
> 
> [...]
> 
> > Let's do these patches separate from the series and maybe change setup_idt
> > too? It'd be interesting to see if the assert in setup_idt would fire,
> > i.e. if any users are relying on it being tolerant to multiple calls, and
> > then find out why.
> 
> The problem should be: smp_init() is calling setup_idt(). So if we
> change the init stuff in setup_idt() into an assertion, any test
> program that calls both smp_init() and setup_idt() would possibly fail
> the assertion. Actually I see most test cases are using:
> 
> 	setup_vm();
> 	smp_init();
> 	setup_idt();
> 
> to setup a basic environment, so I guess all of these use cases would
> fail. In that sense, I'd slightly prefer keep setup_idt() as it is.

Well I guess the fix is just to remove the unnecessary setup_idt calls
from all unit tests that do smp_init(). Anyway, I won't fight too hard
for this cleanup, but I certainly prefer not to propagate the sloppiness
further into the other setups.

Thanks,
drew

Re: [kvm-unit-tests PATCH 04/14] pci: refactor init process to pci_dev_init()

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:42PM +0800, Peter Xu wrote:
> pci_find_dev() is not sufficient for further tests for Intel IOMMU. This
> patch introduced pci_dev struct, as a abstract of a specific PCI device.
> 
> All the rest of current PCI APIs are changed to leverage the pci_dev
> struct.
> 
> x86/vmexit.c is the only user of the old PCI APIs. Changing it to use
> the new ones.
> 
> (Indentation is fixed from using 4 spaces into tab in pci.c)

We need to get Alex's series in and then revisit this. He's maintained
the API's use of a devid handle. It's possible a struct would be better,
but I'm not so sure. In any case we need to break this patch up in order
to see the goals step-by-step.

1) cleanup style - Alex's series does that already
2) replace the devid handle with struct pci_dev - should be done
everywhere or nowhere
3) simplify x86/vmexit's by making better use of the API
4) extend the API for IOMMU

I've added few more comments below.

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci.c    | 75 +++++++++++++++++++++++++++++++++++++++++-------------------
>  lib/pci.h    | 29 ++++++++++++++++++-----
>  x86/vmexit.c | 22 ++++--------------
>  3 files changed, 80 insertions(+), 46 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 0058d70..ef0b02a 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -7,37 +7,66 @@
>  #include "pci.h"
>  #include "asm/pci.h"
>  
> -/* Scan bus look for a specific device. Only bus 0 scanned for now. */
> -pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
> +/*
> + * Scan bus look for a specific device. Only bus 0 scanned for now.
> + * After the scan, a pci_dev is returned with correct BAR information.
> + */
> +int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
> +{
> +	unsigned i;
> +	uint32_t id;
> +
> +	memset(dev, 0, sizeof(*dev));
> +
> +	for (i = 0; i < PCI_DEVFN_MAX; ++i) {
> +		id = pci_config_read(i, 0);

Hmm, pci_config_read still uses devid, but everything else now uses
pci_dev. I don't really like that inconsistency.

> +		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
> +			break;
> +	}
> +
> +	if (i == PCI_DEVFN_MAX) {
> +		printf("PCI: failed init dev (vendor: 0x%04x, "
> +		       "device: 0x%04x)\n", vendor_id, device_id);
> +		return -1;
> +	}
> +
> +	dev->pci_addr = i;

but i is the devid, not the addr. Unless I misunderstand what 'addr' means
here.

> +	for (i = 0; i < PCI_BAR_NUM; i++) {
> +		if (!pci_bar_is_valid(dev, i))
> +			continue;
> +		dev->pci_bar[i] = pci_bar_addr(dev, i);
> +		printf("PCI: init dev 0x%04x BAR %d [%s] addr 0x%lx\n",
> +		       dev->pci_addr, i, pci_bar_is_memory(dev, i) ?
> +		       "MEM" : "IO", dev->pci_bar[i]);
> +	}
> +	dev->inited = true;
> +
> +	return 0;
> +}
> +
> +static inline uint32_t pci_config_read_bar(pci_dev *dev, int n)
>  {
> -    unsigned dev;
> -    for (dev = 0; dev < 256; ++dev) {
> -    uint32_t id = pci_config_read(dev, 0);
> -    if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) {
> -        return dev;
> -    }
> -    }
> -    return PCIDEVADDR_INVALID;
> +	return pci_config_read(dev->pci_addr,
> +			       PCI_BASE_ADDRESS_0 + n * 4);
>  }
>  
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num)
>  {
> -    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> -    if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
> -        return bar & PCI_BASE_ADDRESS_IO_MASK;
> -    } else {
> -        return bar & PCI_BASE_ADDRESS_MEM_MASK;
> -    }
> +	uint32_t bar = pci_config_read_bar(dev, bar_num);
> +	if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
> +		return bar & PCI_BASE_ADDRESS_IO_MASK;
> +	} else {
> +		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> +	}
>  }
>  
> -bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
> +bool pci_bar_is_memory(pci_dev *dev, int bar_num)
>  {
> -    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> -    return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
> +	uint32_t bar = pci_config_read_bar(dev, bar_num);
> +	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
>  }
>  
> -bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
> +bool pci_bar_is_valid(pci_dev *dev, int bar_num)
>  {
> -    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> -    return bar;
> +	return pci_config_read_bar(dev, bar_num);
>  }
> diff --git a/lib/pci.h b/lib/pci.h
> index 9160cfb..b8755ff 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -14,10 +14,21 @@ typedef uint16_t pcidevaddr_t;
>  enum {
>      PCIDEVADDR_INVALID = 0xffff,
>  };
> -pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
> -bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> -bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> +
> +#define PCI_BAR_NUM                     (6)
> +#define PCI_DEVFN_MAX                   (256)
> +
> +struct pci_dev {
> +	bool inited;
> +	uint16_t pci_addr;
> +	phys_addr_t pci_bar[PCI_BAR_NUM];
> +};
> +typedef struct pci_dev pci_dev;
> +
> +int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
> +phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
> +bool pci_bar_is_memory(pci_dev *dev, int bar_num);
> +bool pci_bar_is_valid(pci_dev *dev, int bar_num);
>  
>  /*
>   * pci-testdev is a driver for the pci-testdev qemu pci device. The
> @@ -27,8 +38,6 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
>  #define PCI_VENDOR_ID_REDHAT		0x1b36
>  #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
>  
> -#define PCI_TESTDEV_NUM_BARS		2

Why remove this? I could be used to test that we only find this many
BARs on the testdev, no?

> -
>  struct pci_test_dev_hdr {
>  	uint8_t  test;
>  	uint8_t  width;
> @@ -39,4 +48,12 @@ struct pci_test_dev_hdr {
>  	uint8_t  name[];
>  };
>  
> +enum pci_dma_dir {
> +	PCI_DMA_FROM_DEVICE = 0,
> +	PCI_DMA_TO_DEVICE,
> +};
> +typedef enum pci_dma_dir pci_dma_dir_t;
> +
> +typedef uint64_t iova_t;

stray changes

> +
>  #endif /* PCI_H */
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index c2e1e49..908d2dc 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -371,8 +371,7 @@ int main(int ac, char **av)
>  {
>  	struct fadt_descriptor_rev1 *fadt;
>  	int i;
> -	unsigned long membar = 0;
> -	pcidevaddr_t pcidev;
> +	pci_dev dev;
>  
>  	smp_init();
>  	setup_vm();
> @@ -385,21 +384,10 @@ int main(int ac, char **av)
>  	pm_tmr_blk = fadt->pm_tmr_blk;
>  	printf("PM timer port is %x\n", pm_tmr_blk);
>  
> -	pcidev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> -	if (pcidev != PCIDEVADDR_INVALID) {
> -		for (i = 0; i < PCI_TESTDEV_NUM_BARS; i++) {
> -			if (!pci_bar_is_valid(pcidev, i)) {
> -				continue;
> -			}
> -			if (pci_bar_is_memory(pcidev, i)) {
> -				membar = pci_bar_addr(pcidev, i);
> -				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
> -			} else {
> -				pci_test.iobar = pci_bar_addr(pcidev, i);
> -			}
> -		}
> -		printf("pci-testdev at 0x%x membar %lx iobar %x\n",
> -		       pcidev, membar, pci_test.iobar);
> +	if (!pci_dev_init(&dev, PCI_VENDOR_ID_REDHAT,
> +			  PCI_DEVICE_ID_REDHAT_TEST)) {
> +		pci_test.memaddr = ioremap(dev.pci_bar[0], PAGE_SIZE);
> +		pci_test.iobar = dev.pci_bar[1];

Before we didn't require BAR0 to be MEM and BAR1 to be IO, now we do. It's
probably safe, but less flexible. I think we should at least provide
asserts that our assumptions are correct.

>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(tests); ++i)
> -- 
> 2.7.4
>

Thanks,
drew 

Re: [kvm-unit-tests PATCH 01/14] x86: vm: allow multiple init for vm setup

[Peter Xu]

On Thu, Oct 20, 2016 at 11:39:06AM +0200, Andrew Jones wrote:
> On Thu, Oct 20, 2016 at 04:55:22PM +0800, Peter Xu wrote:
> > On Thu, Oct 20, 2016 at 10:41:37AM +0200, Andrew Jones wrote:
> > 
> > [...]
> > 
> > > Let's do these patches separate from the series and maybe change setup_idt
> > > too? It'd be interesting to see if the assert in setup_idt would fire,
> > > i.e. if any users are relying on it being tolerant to multiple calls, and
> > > then find out why.
> > 
> > The problem should be: smp_init() is calling setup_idt(). So if we
> > change the init stuff in setup_idt() into an assertion, any test
> > program that calls both smp_init() and setup_idt() would possibly fail
> > the assertion. Actually I see most test cases are using:
> > 
> > 	setup_vm();
> > 	smp_init();
> > 	setup_idt();
> > 
> > to setup a basic environment, so I guess all of these use cases would
> > fail. In that sense, I'd slightly prefer keep setup_idt() as it is.
> 
> Well I guess the fix is just to remove the unnecessary setup_idt calls
> from all unit tests that do smp_init(). Anyway, I won't fight too hard
> for this cleanup, but I certainly prefer not to propagate the sloppiness
> further into the other setups.

It won't be hard to do that. Let me cook one for this.

Thanks,

-- peterx

Re: [kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[Radim Krčmář]

2016-10-20 14:05+0800, Peter Xu:
> On Wed, Oct 19, 2016 at 10:21:11PM +0200, Radim Krčmář wrote:
>> 2016-10-14 20:40+0800, Peter Xu:
>> >       The problem is, there are many IOMMU error conditions which are
>> > very hard to be triggered in a real guest (IOMMU has merely no
>> > interface for guest user, and it's totally running in the background).
>> 
>> I gracefully skipped most of VT-d error handling ... how many of those
>> errors are not a result of a misconfiguration?
> 
> Do you mean the error handling codes in hw/i386/intel_iommu.c?

Yes, I meant the error codes that IOMMU will pass to the OS.

>                                                                IMHO
> most of those errors will be triggered only if there is bug in guest
> OS IOMMU driver.

We can't know what the OS wanted, so there are no OS bugs for us. :)
If an OS (mis)configures the device, then it should get an expected
error code -- these paths are rarely exercised, so unit tests for them
would be useful too.  (Very low priority, though, so I don't expect that
anyone will every write them.)

> Here when I talked about error conditions, I mostly meant potential
> QEMU IOMMU bugs, not guest OS bugs (of course, AFAICT kvm-unit-tests
> are not for guest OS bugs). For example, there are still bugs in IOMMU
> IR, and some of the bugs can hardly be triggered by guest OS. In that
> case, we need this unit test to reproduce the bug, and re-run it to
> verify when I have a fix. Otherwise even if I fixed the bug one day, I
> can never reproduce it, nor can I know whether the fix works.

I see, thanks, this is a great goal for unit tests.

Btw. isn't this series already testing one fixed QEMU bug?
When I run this test with old QEMU (qemu-2.7.0-3.fc26), then I get this
output:

  [...]
  INTR: setup IRTE index 0
  lib/pci.c:52: assert failed: dev && dev->inited && dev->msi_offset
          STACK: 40344c 402fd1 400567 40028f

new QEMU works fine, so I assume that we should test it and not assert.

Re: [kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[Peter Xu]

On Thu, Oct 20, 2016 at 01:08:59PM +0200, Radim Krčmář wrote:
> 2016-10-20 14:05+0800, Peter Xu:
> > On Wed, Oct 19, 2016 at 10:21:11PM +0200, Radim Krčmář wrote:
> >> 2016-10-14 20:40+0800, Peter Xu:
> >> >       The problem is, there are many IOMMU error conditions which are
> >> > very hard to be triggered in a real guest (IOMMU has merely no
> >> > interface for guest user, and it's totally running in the background).
> >> 
> >> I gracefully skipped most of VT-d error handling ... how many of those
> >> errors are not a result of a misconfiguration?
> > 
> > Do you mean the error handling codes in hw/i386/intel_iommu.c?
> 
> Yes, I meant the error codes that IOMMU will pass to the OS.
> 
> >                                                                IMHO
> > most of those errors will be triggered only if there is bug in guest
> > OS IOMMU driver.
> 
> We can't know what the OS wanted, so there are no OS bugs for us. :)
> If an OS (mis)configures the device, then it should get an expected
> error code -- these paths are rarely exercised, so unit tests for them
> would be useful too.  (Very low priority, though, so I don't expect that
> anyone will every write them.)

Yeah, though my original goal was not for this, but this is a good
point.

> 
> > Here when I talked about error conditions, I mostly meant potential
> > QEMU IOMMU bugs, not guest OS bugs (of course, AFAICT kvm-unit-tests
> > are not for guest OS bugs). For example, there are still bugs in IOMMU
> > IR, and some of the bugs can hardly be triggered by guest OS. In that
> > case, we need this unit test to reproduce the bug, and re-run it to
> > verify when I have a fix. Otherwise even if I fixed the bug one day, I
> > can never reproduce it, nor can I know whether the fix works.
> 
> I see, thanks, this is a great goal for unit tests.
> 
> Btw. isn't this series already testing one fixed QEMU bug?

Nop. Actually...

> When I run this test with old QEMU (qemu-2.7.0-3.fc26), then I get this
> output:
> 
>   [...]
>   INTR: setup IRTE index 0
>   lib/pci.c:52: assert failed: dev && dev->inited && dev->msi_offset
>           STACK: 40344c 402fd1 400567 40028f
> 
> new QEMU works fine, so I assume that we should test it and not assert.

... here the assertion should be failing at msi_offset == 0, since MSI
support for hw/misc/edu.c device is just added days ago in commit:

commit eabb5782f70b4a10975b24ccd7129929a05ac932
Author: Peter Xu <peterx@redhat.com>
Date:   Wed Sep 28 21:03:39 2016 +0800

    hw/misc/edu: support MSI interrupt

I should mention this in the cover letter that we need the latest QEMU
with at least above commit to run the tests. Will do it in v2.

Thanks,

-- peterx

Re: [kvm-unit-tests RFC PATCH 00/14] VT-d unit test

[Peter Xu]

On Thu, Oct 20, 2016 at 01:08:59PM +0200, Radim Krčmář wrote:

[...]

> Btw. isn't this series already testing one fixed QEMU bug?
> When I run this test with old QEMU (qemu-2.7.0-3.fc26), then I get this
> output:
> 
>   [...]
>   INTR: setup IRTE index 0
>   lib/pci.c:52: assert failed: dev && dev->inited && dev->msi_offset
>           STACK: 40344c 402fd1 400567 40028f
> 
> new QEMU works fine, so I assume that we should test it and not assert.

One thing forgot to mention: I think you are right, I should test it
and not assert, so that when people are using old edu device, we can
just skip the MSI tests.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 05/14] page: add page alignment checker

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:43PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/asm-generic/page.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/asm-generic/page.h b/lib/asm-generic/page.h
> index 7b8a08b..cbdc8f6 100644
> --- a/lib/asm-generic/page.h
> +++ b/lib/asm-generic/page.h
> @@ -19,6 +19,11 @@
>  
>  #define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
>  
> +#define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
> +#define PAGE_ALIGNED_4K(x)	IS_ALIGNED((x), (0x1000))
> +#define PAGE_ALIGNED_2M(x)	IS_ALIGNED((x), (0x200000))
> +#define PAGE_ALIGNED_1G(x)	IS_ALIGNED((x), (0x40000000))

I like IS_ALIGNED, but not the others. The others are strangely
named because it's half asking if an address is page aligned and
half asking if it's aligned to a given boundary. Why not just
ask the boundary, like ALIGNED_2M(x)? Anyway, my preference would
be to replace them with SZ_4K, SZ_2M, SZ_1G definitions. Users would
then do

 IS_ALIGNED(addr, SZ_2M)

or whatever, which isn't much more typing.

Thanks,
drew

Re: [kvm-unit-tests PATCH 06/14] util: move MAX/MIN macro into util.h

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:44PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/alloc.c | 3 ---
>  lib/util.h  | 3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index e1d7b8a..58af52b 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -7,9 +7,6 @@
>  #include "asm/spinlock.h"
>  #include "asm/io.h"
>  
> -#define MIN(a, b)		((a) < (b) ? (a) : (b))
> -#define MAX(a, b)		((a) > (b) ? (a) : (b))
> -
>  #define PHYS_ALLOC_NR_REGIONS	256
>  
>  struct phys_alloc_region {
> diff --git a/lib/util.h b/lib/util.h
> index 4c4b441..1462f4f 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -20,4 +20,7 @@
>   */
>  extern int parse_keyval(char *s, long *val);
>  
> +#define MIN(a, b)		((a) < (b) ? (a) : (b))
> +#define MAX(a, b)		((a) > (b) ? (a) : (b))
> +
>  #endif
> -- 
> 2.7.4
>

I'd prefer they move to lib/libcflat.h. Currently util is for unit
test utilities. MIN/MAX are useful to the library code as well. We
shouldn't require util.h be included by library code.

(I agree libcflat.h is getting ugly, but cleaning it up is beyond the
 scope of this series and right now MIN/MAX would fit there best.)

Thanks,
drew

Re: [kvm-unit-tests PATCH 05/14] page: add page alignment checker

[Andrew Jones]

On Thu, Oct 20, 2016 at 02:23:14PM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:43PM +0800, Peter Xu wrote:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/asm-generic/page.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/lib/asm-generic/page.h b/lib/asm-generic/page.h
> > index 7b8a08b..cbdc8f6 100644
> > --- a/lib/asm-generic/page.h
> > +++ b/lib/asm-generic/page.h

Another comment. IS_ALIGNED shouldn't be here, it should be
in lib/libcflat.h under the definition of ALIGN there.

drew

> > @@ -19,6 +19,11 @@
> >  
> >  #define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
> >  
> > +#define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
> > +#define PAGE_ALIGNED_4K(x)	IS_ALIGNED((x), (0x1000))
> > +#define PAGE_ALIGNED_2M(x)	IS_ALIGNED((x), (0x200000))
> > +#define PAGE_ALIGNED_1G(x)	IS_ALIGNED((x), (0x40000000))
> 
> I like IS_ALIGNED, but not the others. The others are strangely
> named because it's half asking if an address is page aligned and
> half asking if it's aligned to a given boundary. Why not just
> ask the boundary, like ALIGNED_2M(x)? Anyway, my preference would
> be to replace them with SZ_4K, SZ_2M, SZ_1G definitions. Users would
> then do
> 
>  IS_ALIGNED(addr, SZ_2M)
> 
> or whatever, which isn't much more typing.
> 
> Thanks,
> drew
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 07/14] vm/page: provide PGDIR_OFFSET() macro

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:45PM +0800, Peter Xu wrote:
> This can be used in further patches.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/asm/page.h | 3 +++
>  lib/x86/vm.c       | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
> index 5044a49..c43bab2 100644
> --- a/lib/x86/asm/page.h
> +++ b/lib/x86/asm/page.h
> @@ -41,5 +41,8 @@
>  #define	PGDIR_MASK	1023
>  #endif
>  
> +#define PGDIR_BITS(lvl)        (((lvl) - 1) * PGDIR_WIDTH + PAGE_SHIFT)
> +#define PGDIR_OFFSET(va, lvl)  (((va) >> PGDIR_BITS(lvl)) & PGDIR_MASK)
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 9771bd7..f97d1e5 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -48,7 +48,7 @@ unsigned long *install_pte(unsigned long *cr3,
>      unsigned offset;
>  
>      for (level = PAGE_LEVEL; level > pte_level; --level) {
> -	offset = ((unsigned long)virt >> ((level-1) * PGDIR_WIDTH + 12)) & PGDIR_MASK;
> +	offset = PGDIR_OFFSET((unsigned long)virt, level);
>  	if (!(pt[offset] & PT_PRESENT_MASK)) {
>  	    unsigned long *new_pt = pt_page;
>              if (!new_pt)
> @@ -60,7 +60,7 @@ unsigned long *install_pte(unsigned long *cr3,
>  	}
>  	pt = phys_to_virt(pt[offset] & PT_ADDR_MASK);
>      }
> -    offset = ((unsigned long)virt >> ((level-1) * PGDIR_WIDTH + 12)) & PGDIR_MASK;
> +    offset = PGDIR_OFFSET((unsigned long)virt, level);
>      pt[offset] = pte;
>      return &pt[offset];
>  }
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 08/14] x86: pci: add pci_config_{read|write}[bw]() helpers

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:46PM +0800, Peter Xu wrote:
> And some cleanup on the file.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/x86/asm/pci.h | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> index cddde41..ce970e4 100644
> --- a/lib/x86/asm/pci.h
> +++ b/lib/x86/asm/pci.h
> @@ -9,11 +9,42 @@
>  #include "pci.h"
>  #include "x86/asm/io.h"
>  
> +#define PCI_HOST_CONFIG_PORT (0xcf8)
> +#define PCI_HOST_DATA_PORT   (0xcfc)

Thomas wanted to do this to, but I disagreed because CF8/CFC are
effectively the names already.

Alex already has a patch in his series just like this one
"pci: x86: Add remaining PCI configuration space accessors"

drew

> +
> +#define PCI_HOST_INDEX(dev, reg)   (reg | (dev << 8) | (0x1 << 31))
> +
> +#define pci_config_readb(dev, reg) (pci_config_read(dev, reg) & 0xff)
> +#define pci_config_readw(dev, reg) (pci_config_read(dev, reg) & 0xffff)
> +
>  static inline uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
>  {
> -    uint32_t index = reg | (dev << 8) | (0x1 << 31);
> -    outl(index, 0xCF8);
> -    return inl(0xCFC);
> +	uint32_t index = PCI_HOST_INDEX(dev, reg);
> +	outl(index, PCI_HOST_CONFIG_PORT);
> +	return inl(PCI_HOST_DATA_PORT);
> +}
> +
> +static inline void pci_config_write(pcidevaddr_t dev, uint8_t reg,
> +				    uint32_t val)
> +{
> +	uint32_t index = PCI_HOST_INDEX(dev, reg);
> +	outl(index, PCI_HOST_CONFIG_PORT);
> +	outl(val, PCI_HOST_DATA_PORT);
>  }
>  
> +static inline void pci_config_writeb(pcidevaddr_t dev, uint8_t reg,
> +				     uint8_t val)
> +{
> +	uint32_t index = PCI_HOST_INDEX(dev, reg);
> +	outl(index, PCI_HOST_CONFIG_PORT);
> +	outb(val, PCI_HOST_DATA_PORT);
> +}
> +
> +static inline void pci_config_writew(pcidevaddr_t dev, uint8_t reg,
> +				     uint16_t val)
> +{
> +	uint32_t index = PCI_HOST_INDEX(dev, reg);
> +	outl(index, PCI_HOST_CONFIG_PORT);
> +	outw(val, PCI_HOST_DATA_PORT);
> +}
>  #endif
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 09/14] pci: provide pci_set_master()

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:47PM +0800, Peter Xu wrote:
> And set master by default for pci devices. So that DMA is allowed.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index ef0b02a..044e4db 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -7,6 +7,13 @@
>  #include "pci.h"
>  #include "asm/pci.h"
>  
> +static void pci_set_master(pci_dev *dev, int master)
> +{
> +	uint32_t val = pci_config_read(dev->pci_addr, PCI_COMMAND);
> +	val |= PCI_COMMAND_MASTER;
> +	pci_config_write(dev->pci_addr, PCI_COMMAND, val);
> +}

I agree with the API extension (well, I know next to nothing about PCI,
so I'll assume this function is correct...) But...

> +
>  /*
>   * Scan bus look for a specific device. Only bus 0 scanned for now.
>   * After the scan, a pci_dev is returned with correct BAR information.
> @@ -41,6 +48,8 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
>  	}
>  	dev->inited = true;
>  
> +	pci_set_master(dev, 1);

...I don't necessarily agree with it being enabled by default. In the
least I think you want a minimal init function available to unit tests
that they then can extend with their own custom init. Then you can also
provide an init that configures a bunch of stuff you think is a
good idea by default. For example, in my gic series I try to do that. I
have a gic_init() function which does nothing except set some base
addresses. Then, I have a gic_enable_defaults function that unit tests
can use if they don't care how we enable it, they just want something
that works. Otherwise the unit test would just call gic_init and then
write all the registers for enabling it that they want/need.

drew

> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 10/14] pci: add bdf helpers

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:48PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/pci.h b/lib/pci.h
> index b8755ff..6a1c3c9 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -18,6 +18,12 @@ enum {
>  #define PCI_BAR_NUM                     (6)
>  #define PCI_DEVFN_MAX                   (256)
>  
> +#define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
> +#define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
> +#define PCI_SLOT(devfn)                 (((devfn) >> 3) & 0x1f)
> +#define PCI_FUNC(devfn)                 ((devfn) & 0x07)
> +#define PCI_BUILD_BDF(bus, devfn)       ((bus << 8) | (devfn))

As I mention frequently; I know nothing about PCI, but this doesn't
look right to me. DEVFN should be defined as

 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))

Care to explain these? Please provide the explanation in the commit
message to along with a motivation for adding them.

Thanks,
drew

> +
>  struct pci_dev {
>  	bool inited;
>  	uint16_t pci_addr;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:49PM +0800, Peter Xu wrote:
> QEMU edu device is a pci device that is originally written for
> educational purpose, however it also suits for IOMMU unit test. Adding
> helpers for this specific device to implement the device logic.
> 
> The device supports lots of functions, here only DMA operation is
> supported.
> 
> The spec of the device can be found at:
> 
>   https://github.com/qemu/qemu/blob/master/docs/specs/edu.txt
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci-edu.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci-edu.h |  29 +++++++++++++
>  2 files changed, 157 insertions(+)
>  create mode 100644 lib/pci-edu.c
>  create mode 100644 lib/pci-edu.h
> 
> diff --git a/lib/pci-edu.c b/lib/pci-edu.c
> new file mode 100644
> index 0000000..4d1a5ab
> --- /dev/null
> +++ b/lib/pci-edu.c
> @@ -0,0 +1,128 @@
> +/*
> + * Edu PCI device.
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + *   Peter Xu <peterx@redhat.com>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#include "pci-edu.h"
> +
> +#define  PCI_VENDOR_ID_QEMU              (0x1234)
> +#define  PCI_DEVICE_ID_EDU               (0x11e8)
> +
> +/* The only bar used by EDU device */
> +#define EDU_BAR_MEM                 (0)
> +#define EDU_MAGIC                   (0xed)
> +#define EDU_VERSION                 (0x100)
> +#define EDU_DMA_BUF_SIZE            (1 << 20)
> +#define EDU_INPUT_BUF_SIZE          (256)
> +
> +#define EDU_REG_ID                  (0x0)
> +#define EDU_REG_ALIVE               (0x4)
> +#define EDU_REG_FACTORIAL           (0x8)
> +#define EDU_REG_STATUS              (0x20)
> +#define EDU_REG_DMA_SRC             (0x80)
> +#define EDU_REG_DMA_DST             (0x88)
> +#define EDU_REG_DMA_COUNT           (0x90)
> +#define EDU_REG_DMA_CMD             (0x98)
> +
> +#define EDU_CMD_DMA_START           (0x01)
> +#define EDU_CMD_DMA_FROM            (0x02)
> +#define EDU_CMD_DMA_TO              (0x00)
> +
> +#define EDU_STATUS_FACTORIAL        (0x1)
> +#define EDU_STATUS_INT_ENABLE       (0x80)
> +
> +#define EDU_DMA_START               (0x40000)
> +#define EDU_DMA_SIZE_MAX            (4096)

shouldn't the above defines be in the header?

> +
> +uint64_t edu_reg_readq(pci_edu_dev_t *dev, int reg)
> +{
> +	return *(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
> +}
> +
> +uint32_t edu_reg_read(pci_edu_dev_t *dev, int reg)
> +{
> +	return *(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
> +}
> +
> +void edu_reg_writeq(pci_edu_dev_t *dev, int reg, uint64_t val)
> +{
> +	*(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
> +}
> +
> +void edu_reg_write(pci_edu_dev_t *dev, int reg, uint32_t val)
> +{
> +	*(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
> +}

I'd put these accessors in the header as static inlines too

> +
> +/* Return true if alive */
> +bool edu_check_alive(pci_edu_dev_t *dev)
> +{
> +	static uint32_t live_count = 1;
> +	uint32_t value;
> +
> +	edu_reg_write(dev, EDU_REG_ALIVE, live_count++);
> +	value = edu_reg_read(dev, EDU_REG_ALIVE);
> +	return (live_count - 1 == ~value);
> +}

Even edu_check_alive could be a static inline in the header,
if it's something you'll do frequently. If it's only needed
for a sanity init test then I'd just inline it directly in
the one caller, edu_init

> +
> +int edu_init(pci_edu_dev_t *dev)
> +{
> +	int ret;
> +
> +	ret = pci_dev_init(&dev->pci_dev, PCI_VENDOR_ID_QEMU,
> +			   PCI_DEVICE_ID_EDU);
> +	if (ret)
> +		return ret;
> +
> +	if (!edu_check_alive(dev)) {
> +		printf("edu device not alive!\n");
> +		return -1;

should this ever fail? Or would an assert by fine here?
 alive = edu_check_alive(dev)
 assert(alive);

> +	}
> +
> +	return 0;
> +}
> +
> +uint32_t edu_status(pci_edu_dev_t *dev)
> +{
> +	return edu_reg_read(dev, EDU_REG_STATUS);
> +}

please, no unnecessary wrappers

> +
> +void edu_dma(pci_edu_dev_t *dev, iova_t iova,
> +	     size_t size, int dev_offset, pci_dma_dir_t dir)
> +{
> +	uint64_t from, to;
> +	uint32_t cmd = EDU_CMD_DMA_START;
> +
> +	assert(size <= EDU_DMA_SIZE_MAX);
> +	assert(dev_offset < EDU_DMA_SIZE_MAX &&
> +	       dev_offset >= 0);
> +
> +	printf("edu device DMA start %s addr %p size 0x%lu off 0x%x\n",
> +	       dir == PCI_DMA_FROM_DEVICE ? "FROM" : "TO",
> +	       (void *)iova, size, dev_offset);

is pci_dma_dir_t just a binary enum? If so, then I find it
sort of crufty. Can't we just have a 'bool write'?

> +
> +	if (dir == PCI_DMA_FROM_DEVICE) {
> +		from = dev_offset + EDU_DMA_START;
> +		to = iova;
> +		cmd |= EDU_CMD_DMA_FROM;
> +	} else {
> +		from = iova;
> +		to = EDU_DMA_START + dev_offset;
> +		cmd |= EDU_CMD_DMA_TO;
> +	}
> +
> +	edu_reg_writeq(dev, EDU_REG_DMA_SRC, from);
> +	edu_reg_writeq(dev, EDU_REG_DMA_DST, to);
> +	edu_reg_writeq(dev, EDU_REG_DMA_COUNT, size);
> +	edu_reg_write(dev, EDU_REG_DMA_CMD, cmd);
> +
> +	/* Wait until DMA finished */
> +	while (edu_reg_read(dev, EDU_REG_DMA_CMD) & EDU_CMD_DMA_START);

You may consider adding a cpu_relax() to lib/x86/asm/barrier.h, like
arm and ppc have. I noticed a big difference when running with tcg
on how fast a 'while (state-not-changed);' loop can complete when
adding that barrier.

> +}
> diff --git a/lib/pci-edu.h b/lib/pci-edu.h
> new file mode 100644
> index 0000000..6b7dbfd
> --- /dev/null
> +++ b/lib/pci-edu.h
> @@ -0,0 +1,29 @@
> +/*
> + * Edu PCI device header.
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * Authors:
> + *   Peter Xu <peterx@redhat.com>,
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + *
> + * Edu device is a virtualized device in QEMU. Please refer to
> + * docs/specs/edu.txt in QEMU repository for EDU device manual.
> + */
> +#ifndef __PCI_EDU_H__
> +#define __PCI_EDU_H__
> +
> +#include "pci.h"
> +
> +struct pci_edu_dev {
> +	pci_dev pci_dev;
> +};
> +typedef struct pci_edu_dev pci_edu_dev_t;

Why wrap pci_dev in this pci_edu_dev struct? Will there ever
be more state? Should we just wait and see? Also, why use a
typedef for pci_dev (assuming we want it)? 'struct pci_dev'
would be more kernel like.

Thanks,
drew

> +
> +int edu_init(pci_edu_dev_t *dev);
> +void edu_dma(pci_edu_dev_t *dev, iova_t iova,
> +	     size_t size, int dev_offset, pci_dma_dir_t dir);
> +
> +#endif
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 13/14] pci: add msi support for 32/64bit address

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:51PM +0800, Peter Xu wrote:
> During each PCI device init, we walk through all the capability list,
> and see what we support. If a cap handler is provided, it'll be
> triggered if the cap is detected. MSI cap handler is the first one. We
> can add more cap handler in the future.
> 
> Meanwhile, pci_setup_msi() API is provided to support basic 32/64 bit
> address MSI setup.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pci.h |  2 ++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 044e4db..1037ae3 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -7,6 +7,35 @@
>  #include "pci.h"
>  #include "asm/pci.h"
>  
> +typedef void (*pci_cap_handler)(pci_dev *dev, int cap_offset);
> +
> +static void pci_cap_msi_handler(pci_dev *dev, int cap_offset)
> +{
> +	printf("Detected MSI for device 0x%x offset 0x%x\n",
> +	       dev->pci_addr, cap_offset);
> +	dev->msi_offset = cap_offset;
> +}
> +
> +static pci_cap_handler cap_handlers[0xff] = {
> +	[PCI_CAP_ID_MSI] = pci_cap_msi_handler,
> +};
> +
> +static void pci_cap_walk(pci_dev *dev)
> +{
> +	uint8_t cap_offset;
> +	uint8_t cap_id;
> +
> +	cap_offset = pci_config_readb(dev->pci_addr, PCI_CAPABILITY_LIST);
> +	while (cap_offset) {
> +		cap_id = pci_config_readb(dev->pci_addr, cap_offset);
> +		printf("PCI detected cap 0x%x\n", cap_id);
> +		if (cap_handlers[cap_id]) {
> +			cap_handlers[cap_id](dev, cap_offset);
> +		}

nit: no need for {}

> +		cap_offset = pci_config_readb(dev->pci_addr, cap_offset + 1);
> +	}
> +}
> +
>  static void pci_set_master(pci_dev *dev, int master)
>  {
>  	uint32_t val = pci_config_read(dev->pci_addr, PCI_COMMAND);
> @@ -14,6 +43,35 @@ static void pci_set_master(pci_dev *dev, int master)
>  	pci_config_write(dev->pci_addr, PCI_COMMAND, val);
>  }
>  
> +void pci_setup_msi(pci_dev *dev, uint64_t msi_addr, uint32_t msi_data)
> +{
> +	uint16_t msi_control;
> +	pcidevaddr_t addr;
> +	uint16_t offset;
> +
> +	assert(dev && dev->inited && dev->msi_offset);
> +
> +	offset = dev->msi_offset;
> +	addr = dev->pci_addr;
> +	msi_control = pci_config_readw(addr, offset + PCI_MSI_FLAGS);
> +	pci_config_write(addr, offset + PCI_MSI_ADDRESS_LO,
> +			 msi_addr & 0xffffffff);
> +
> +	if (msi_control & PCI_MSI_FLAGS_64BIT) {
> +		pci_config_write(addr, offset + PCI_MSI_ADDRESS_HI,
> +				 (uint32_t)(msi_addr >> 32));
> +		pci_config_write(addr, offset + PCI_MSI_DATA_64, msi_data);
> +		printf("MSI: dev 0x%x init 64bit address: ", addr);
> +	} else {
> +		pci_config_write(addr, offset + PCI_MSI_DATA_32, msi_data);
> +		printf("MSI: dev 0x%x init 32bit address: ", addr);
> +	}
> +	printf("addr=0x%lx, data=0x%x\n", msi_addr, msi_data);
> +
> +	msi_control |= PCI_MSI_FLAGS_ENABLE;
> +	pci_config_writew(addr, offset + PCI_MSI_FLAGS, msi_control);
> +}
> +
>  /*
>   * Scan bus look for a specific device. Only bus 0 scanned for now.
>   * After the scan, a pci_dev is returned with correct BAR information.
> @@ -49,6 +107,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
>  	dev->inited = true;
>  
>  	pci_set_master(dev, 1);
> +	pci_cap_walk(dev);

again, not sure we want this in the one and only init function

>  
>  	return 0;
>  }
> diff --git a/lib/pci.h b/lib/pci.h
> index 6a1c3c9..5581446 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -27,6 +27,7 @@ enum {
>  struct pci_dev {
>  	bool inited;
>  	uint16_t pci_addr;
> +	uint16_t msi_offset;
>  	phys_addr_t pci_bar[PCI_BAR_NUM];
>  };
>  typedef struct pci_dev pci_dev;
> @@ -35,6 +36,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
>  phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
>  bool pci_bar_is_memory(pci_dev *dev, int bar_num);
>  bool pci_bar_is_valid(pci_dev *dev, int bar_num);
> +void pci_setup_msi(pci_dev *dev, uint64_t msi_addr, uint32_t msi_data);
>  
>  /*
>   * pci-testdev is a driver for the pci-testdev qemu pci device. The
> -- 
> 2.7.4
>

Looks good from my not knowing much about PCI perspective.

drew

Re: [kvm-unit-tests PATCH 14/14] x86: intel-iommu: add IR test

[Andrew Jones]

On Fri, Oct 14, 2016 at 08:40:52PM +0800, Peter Xu wrote:
> First of all, vtd_setup_msi() is provided. It setup IRTE entries,
> meanwhile, setup PCI device MSI vectors corresponding to VT-d spec.
> 
> The basic IR test is carried out by a edu factorial request. When write
> to the factorial register, calculation starts in the background of
> device, MSI is triggered as long as it finishes.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci-edu.c         | 18 ++++++++--
>  lib/pci-edu.h         |  7 ++++
>  lib/x86/intel-iommu.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/x86/intel-iommu.h |  1 +
>  x86/intel-iommu.c     | 45 +++++++++++++++++++++++++
>  5 files changed, 161 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/pci-edu.c b/lib/pci-edu.c
> index 4d1a5ab..9a90a63 100644
> --- a/lib/pci-edu.c
> +++ b/lib/pci-edu.c
> @@ -35,9 +35,6 @@
>  #define EDU_CMD_DMA_FROM            (0x02)
>  #define EDU_CMD_DMA_TO              (0x00)
>  
> -#define EDU_STATUS_FACTORIAL        (0x1)
> -#define EDU_STATUS_INT_ENABLE       (0x80)
> -

I think these and all EDU_ defines should have been in
pci-edu.h in the first place.

>  #define EDU_DMA_START               (0x40000)
>  #define EDU_DMA_SIZE_MAX            (4096)
>  
> @@ -94,6 +91,21 @@ uint32_t edu_status(pci_edu_dev_t *dev)
>  	return edu_reg_read(dev, EDU_REG_STATUS);
>  }
>  
> +void edu_setup_intr(pci_edu_dev_t *dev, bool enabled)
> +{
> +	edu_reg_write(dev, EDU_REG_STATUS, EDU_STATUS_INT_ENABLE);
> +}
> +
> +void edu_fact_write(pci_edu_dev_t *dev, uint32_t in)
> +{
> +	edu_reg_write(dev, EDU_REG_FACTORIAL, in);
> +}
> +
> +uint32_t edu_fact_read(pci_edu_dev_t *dev)
> +{
> +	return edu_reg_read(dev, EDU_REG_FACTORIAL);
> +}

More wrappers...

> +
>  void edu_dma(pci_edu_dev_t *dev, iova_t iova,
>  	     size_t size, int dev_offset, pci_dma_dir_t dir)
>  {
> diff --git a/lib/pci-edu.h b/lib/pci-edu.h
> index 6b7dbfd..dcf5b76 100644
> --- a/lib/pci-edu.h
> +++ b/lib/pci-edu.h
> @@ -22,8 +22,15 @@ struct pci_edu_dev {
>  };
>  typedef struct pci_edu_dev pci_edu_dev_t;
>  
> +#define EDU_STATUS_FACTORIAL        (0x1)
> +#define EDU_STATUS_INT_ENABLE       (0x80)
> +
>  int edu_init(pci_edu_dev_t *dev);
>  void edu_dma(pci_edu_dev_t *dev, iova_t iova,
>  	     size_t size, int dev_offset, pci_dma_dir_t dir);
> +void edu_setup_intr(pci_edu_dev_t *dev, bool enabled);
> +void edu_fact_write(pci_edu_dev_t *dev, uint32_t in);
> +uint32_t edu_fact_read(pci_edu_dev_t *dev);
> +uint32_t edu_status(pci_edu_dev_t *dev);
>  
>  #endif
> diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
> index 30e2c97..de6c732 100644
> --- a/lib/x86/intel-iommu.c
> +++ b/lib/x86/intel-iommu.c
> @@ -12,6 +12,7 @@
>  
>  #include "intel-iommu.h"
>  #include "asm-generic/page.h"
> +#include "pci.h"
>  
>  /*
>   * VT-d in QEMU currently only support 39 bits address width, which is
> @@ -48,6 +49,26 @@ struct vtd_context_entry {
>  } __attribute__ ((packed));
>  typedef struct vtd_context_entry vtd_ce_t;
>  
> +struct vtd_irte {
> +	uint32_t present:1;
> +	uint32_t fault_disable:1;    /* Fault Processing Disable */
> +	uint32_t dest_mode:1;        /* Destination Mode */
> +	uint32_t redir_hint:1;       /* Redirection Hint */
> +	uint32_t trigger_mode:1;     /* Trigger Mode */
> +	uint32_t delivery_mode:3;    /* Delivery Mode */
> +	uint32_t __avail:4;          /* Available spaces for software */
> +	uint32_t __reserved_0:3;     /* Reserved 0 */
> +	uint32_t irte_mode:1;        /* IRTE Mode */
> +	uint32_t vector:8;           /* Interrupt Vector */
> +	uint32_t __reserved_1:8;     /* Reserved 1 */
> +	uint32_t dest_id;            /* Destination ID */
> +	uint16_t source_id:16;       /* Source-ID */
> +	uint64_t sid_q:2;            /* Source-ID Qualifier */
> +	uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> +	uint64_t __reserved_2:44;    /* Reserved 2 */
> +} __attribute__ ((packed));
> +typedef struct vtd_irte vtd_irte_t;
> +
>  void vtd_reg_write_4(unsigned int reg, uint32_t value)
>  {
>  	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> @@ -255,6 +276,78 @@ void vtd_map_range(uint16_t sid, iova_t iova, phys_addr_t pa, size_t size)
>  	}
>  }
>  
> +static uint16_t vtd_intr_index_alloc(void)
> +{
> +	static int index_ctr = 0;
> +	assert(index_ctr < 65535);
> +	return index_ctr++;
> +}
> +
> +static void vtd_setup_irte(pci_dev *dev, vtd_irte_t *irte,
> +			   int vector, int dest_id)
> +{
> +	assert(sizeof(vtd_irte_t) == 16);
> +	memset(irte, 0, sizeof(*irte));
> +	irte->fault_disable = 1;
> +	irte->dest_mode = 0;	 /* physical */
> +	irte->trigger_mode = 0;	 /* edge */
> +	irte->delivery_mode = 0; /* fixed */
> +	irte->irte_mode = 0;	 /* remapped */
> +	irte->vector = vector;
> +	irte->dest_id = dest_id;
> +	irte->source_id = dev->pci_addr;
> +	irte->sid_q = 0;
> +	irte->sid_vtype = 1;     /* full-sid verify */
> +	irte->present = 1;
> +}
> +
> +struct vtd_msi_addr {
> +	uint32_t __dont_care:2;
> +	uint32_t handle_15:1;	 /* handle[15] */
> +	uint32_t shv:1;
> +	uint32_t interrupt_format:1;
> +	uint32_t handle_0_14:15; /* handle[0:14] */
> +	uint32_t head:12;	 /* 0xfee */
> +	uint32_t addr_hi;	 /* not used except with x2apic */
> +} __attribute__ ((packed));
> +typedef struct vtd_msi_addr vtd_msi_addr_t;
> +
> +struct vtd_msi_data {
> +	uint16_t __reserved;
> +	uint16_t subhandle;
> +} __attribute__ ((packed));
> +typedef struct vtd_msi_data vtd_msi_data_t;
> +
> +/**
> + * vtd_setup_msi - setup MSI message for a device
> + *
> + * @dev: PCI device to setup MSI
> + * @vector: interrupt vector
> + * @dest_id: destination processor
> + */
> +void vtd_setup_msi(pci_dev *dev, int vector, int dest_id)
> +{
> +	vtd_msi_data_t msi_data = {};
> +	vtd_msi_addr_t msi_addr = {};
> +	vtd_irte_t *irte = phys_to_virt(vtd_ir_table());
> +	uint16_t index = vtd_intr_index_alloc();
> +
> +	assert(sizeof(vtd_msi_addr_t) == 8);
> +	assert(sizeof(vtd_msi_data_t) == 4);
> +
> +	printf("INTR: setup IRTE index %d\n", index);
> +	vtd_setup_irte(dev, irte + index, vector, dest_id);
> +
> +	msi_addr.handle_15 = index >> 15 & 1;
> +	msi_addr.shv = 0;
> +	msi_addr.interrupt_format = 1;
> +	msi_addr.handle_0_14 = index & 0x7fff;
> +	msi_addr.head = 0xfee;
> +	msi_data.subhandle = 0;
> +
> +	pci_setup_msi(dev, *(uint64_t *)&msi_addr, *(uint32_t *)&msi_data);
> +}

I skipped reviewing all the VTD stuff because that would require
I read a spec... Sorry.

> +
>  void vtd_init(void)
>  {
>  	setup_vm();
> diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
> index f1340cd..fb6b6a6 100644
> --- a/lib/x86/intel-iommu.h
> +++ b/lib/x86/intel-iommu.h
> @@ -125,5 +125,6 @@ void vtd_enable_dmar(void);
>  void vtd_enable_ir(void);
>  void vtd_init(void);
>  void vtd_map_range(uint16_t sid, phys_addr_t iova, phys_addr_t pa, size_t size);
> +void vtd_setup_msi(pci_dev *dev, int vector, int dest_id);
>  
>  #endif
> diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
> index 0c3ab9b..c826af8 100644
> --- a/x86/intel-iommu.c
> +++ b/x86/intel-iommu.c
> @@ -12,6 +12,7 @@
>  
>  #include "intel-iommu.h"
>  #include "pci-edu.h"
> +#include "x86/apic.h"
>  
>  void vtd_test_dmar(pci_edu_dev_t *dev)
>  {
> @@ -48,6 +49,49 @@ void vtd_test_dmar(pci_edu_dev_t *dev)
>  	free_page(page);
>  }
>  
> +static uint32_t factorial(uint32_t in)
> +{
> +	uint32_t v = 1;
> +	while (in) v *= in--;
> +	return v;
> +}

What, no recursion! That's a disgrace to this classic
problem! Actually I don't think you should bother with
the function at all. You hard code the input, so just
precalculate the result.

> +
> +static volatile int edu_done;
> +
> +static void edu_fact_isr(isr_regs_t *regs)
> +{
> +	edu_done = 1;

nit: bool/true

> +	eoi();
> +}
> +
> +static void vtd_test_ir(pci_edu_dev_t *dev)
> +{
> +#define VTD_TEST_VECTOR (0xee)
> +	/* Choose any number to calculate the factorial value. */
> +	const uint32_t fact = 0x8;

Nobody is choosing anything. How about just

 const uint32_t fact_input = 8; /* fact(8) == 40320 */

> +
> +	/*
> +	 * Enable EDU device interrupt, so when factorial task
> +	 * finishes, IRQ will be triggered.
> +	 */
> +	edu_setup_intr(dev, 1);
> +
> +	/*
> +	 * Setup EDU PCI device MSI, using interrupt remapping. By
> +	 * default, EDU device is using INTx.
> +	 */
> +	vtd_setup_msi(&dev->pci_dev, VTD_TEST_VECTOR, 0);
> +
> +	handle_irq(VTD_TEST_VECTOR, edu_fact_isr);
> +	irq_enable();
> +
> +	edu_fact_write(dev, fact);
> +	while (!edu_done);

I guess you depend on the test framework's timeout support to
deal with never getting the interrupt. Make sure you set a
reasonable timeout in unittests.cfg

> +	/* Now results should be put back to edu fact register */
> +	report("EDU factorial INTR test",
> +	       edu_fact_read(dev) == factorial(fact));

Do we really need this factorial stuff? Can't we just raise an
interrupt with the EDU register offset 0x60?

> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	int ret;
> @@ -88,6 +132,7 @@ int main(int argc, char *argv[])
>  	}
>  
>  	vtd_test_dmar(&dev);
> +	vtd_test_ir(&dev);
>  
>  	return report_summary();
>  }
> -- 
> 2.7.4
>

Thanks,
drew 

Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test

[Peter Xu]

On Thu, Oct 20, 2016 at 11:30:30AM +0200, Andrew Jones wrote:

[...]

> > +void vtd_reg_write_4(unsigned int reg, uint32_t value)
> > +{
> > +	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> > +}
> > +
> > +void vtd_reg_write_8(unsigned int reg, uint64_t value)
> > +{
> > +	*(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> > +}
> > +
> > +uint32_t vtd_reg_read_4(unsigned int reg)
> > +{
> > +	return *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> > +}
> > +
> > +uint64_t vtd_reg_read_8(unsigned int reg)
> > +{
> > +	return *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> > +}
> 
> The above could be static inlines in intel-iommu.h. How about
> calling them vtd_readl, vtd_readq, vtd_writel, vtd_writeq? That
> would make them more consistent with the readl, readq... in
> lib/asm-generic/io.h

Sure! Will fix.

> 
> > +
> > +uint32_t vtd_version(void)
> > +{
> > +	return vtd_reg_read_4(DMAR_VER_REG);
> > +}
> > +
> > +uint64_t vtd_cap(void)
> > +{
> > +	return vtd_reg_read_8(DMAR_CAP_REG);
> > +}
> > +
> > +uint64_t vtd_ecap(void)
> > +{
> > +	return vtd_reg_read_8(DMAR_ECAP_REG);
> > +}
> > +
> > +uint32_t vtd_status(void)
> > +{
> > +	return vtd_reg_read_4(DMAR_GSTS_REG);
> > +}
> > +
> > +uint32_t vtd_fault_status(void)
> > +{
> > +	return vtd_reg_read_4(DMAR_FSTS_REG);
> > +}
> > +
> > +uint64_t vtd_root_table(void)
> > +{
> > +	/* No extend root table support yet */
> > +	return vtd_reg_read_8(DMAR_RTADDR_REG);
> > +}
> 
> I'd drop all the above and use vtd_readl/q(DMAR_CAP_REG) directly,
> the wrappers don't add anything and take the unit test writer
> further away from the register names that they should be more
> familiar with.

Here I used wrapper since I don't want to remember all the register
names, and (especially) I don't want to remember the size of the
registers.

For example, when I want to read ECAP register, calling vtd_ecap()
will let me make sure the return value is 8 bytes (compiler will help
to make sure of it, and I can simply know it from seeing the return
type of the wrapper function, which is uint64_t), and I don't need to
bother whether I should use vtd_readl() or vtd_readq() for it. If I
don't have vtd_ecap() wrapper, I can't see the length from the
register name only, and I need to check the spec every time, or with
comment like:

  #define DMAR_ECAP_REG XXX /* 8 bytes */

In that case, I'd prefer with a wrapper for the registers (maybe I can
move the functions into header files as well, with inline static).

> 
> > +
> > +uint64_t vtd_ir_table(void)
> > +{
> > +	return vtd_reg_read_8(DMAR_IRTA_REG) & 0xfffffffffffff000;
> > +}
> 
> This one has a mask, so makes sense to keep the wrapper. But the
> mask looks like PAGE_MASK to me. Why not use that? Or even drop
> this wrapper and just use PAGE_MASK when necessary directly...

The last 12 bits have other means (I didn't check it here, it's
explained in chap 10.4.29 in vt-d spec in case anyone is interested),
so it's just coincident that it looks like a page mask. However, I
think I can use PAGE_MASK here.

> 
> > +
> > +void vtd_gcmd_or(uint32_t cmd)
> > +{
> > +	uint32_t status = vtd_status();
> > +	/*
> > +	 * Logically, we need to read DMAR_GSTS_REG until IOMMU
> > +	 * handles the write request. However for QEMU/KVM, this write
> > +	 * is always sync, so when this returns, we should be sure
> 
> should always be in sync. Sounds like something to unit test :-)

I am not sure whether I got the point... but real hardware should be
async, so I don't know whether the code can work on real hardware. For
QEMU, it is sync, so it's safe for kvm-unit-tests.

> 
> > +	 * that the GCMD write is done.
> > +	 */
> > +	vtd_reg_write_4(DMAR_GCMD_REG, status | cmd);
> > +}
> > +
> > +void vtd_dump_init_info(void)
> > +{
> > +	printf("VT-d version:   0x%x\n", vtd_version());
> > +	printf("     cap:       0x%016lx\n", vtd_cap());
> > +	printf("     ecap:      0x%016lx\n", vtd_ecap());
> > +}
> > +
> > +void vtd_enable_qi(void)
> > +{
> > +	vtd_gcmd_or(VTD_GCMD_QI);
> > +}
> 
> I'd drop this wrapper and use a comment where it's needed like
> 
>  vtd_gcmd_or(VTD_GCMD_QI); /* enable QI */
> 
> (By now you see a theme. Keep wrappers to a minimal in order
>  to keep the unit test writer closer to the spec.)

Yeah. Besides the case that I mentioned above (which I am still not
quite sure), I agree that I can use register names directly in most
cases. Will fix.

> 
> > +
> > +void vtd_setup_root_table(void)
> > +{
> > +	void *root = alloc_page();
> > +
> > +	memset(root, 0, PAGE_SIZE);
> > +	vtd_reg_write_8(DMAR_RTADDR_REG, virt_to_phys(root));
> > +	vtd_gcmd_or(VTD_GCMD_ROOT);
> > +	printf("DMAR table address: 0x%016lx\n", vtd_root_table());
> > +}
> > +
> > +void vtd_setup_ir_table(void)
> > +{
> > +	void *root = alloc_page();
> > +
> > +	memset(root, 0, PAGE_SIZE);
> > +	vtd_reg_write_8(DMAR_IRTA_REG, virt_to_phys(root));
> > +	/* 0xf stands for table size (2^(0xf+1) == 65536) */
> > +	vtd_gcmd_or(VTD_GCMD_IR_TABLE | 0xf);
> > +	printf("IR table address: 0x%016lx\n", vtd_ir_table());
> > +}
> > +
> > +void vtd_enable_dmar(void)
> > +{
> > +	vtd_gcmd_or(VTD_GCMD_DMAR);
> > +}
> > +
> > +void vtd_enable_ir(void)
> > +{
> > +	vtd_gcmd_or(VTD_GCMD_IR);
> > +}
> 
> More wrappers to drop.

Ok.

> 
> > +
> > +void vtd_init(void)
> > +{
> > +	setup_vm();
> > +	smp_init();
> > +	setup_idt();
> > +
> > +	vtd_dump_init_info();
> > +	vtd_enable_qi();
> > +	vtd_setup_root_table();
> > +	vtd_setup_ir_table();
> > +	vtd_enable_dmar();
> > +	vtd_enable_ir();
> 
> I'd open code all the above here. The functions aren't really
> gaining us anything that comments can't do for us.

Will fix.

> 
> > +}
> > diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
> > new file mode 100644
> > index 0000000..0f687d1
> > --- /dev/null
> > +++ b/lib/x86/intel-iommu.h
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Intel IOMMU header
> 
> Please point out the kernel source these defines come from,
> include/linux/intel-iommu.h

Will add.

[...]

> > +int main(int argc, char *argv[])
> > +{
> > +	setup_vm();
> > +	smp_init();
> > +	setup_idt();
> > +
> > +	vtd_dump_init_info();
> > +	report("init status check", vtd_status() == 0);
> > +	report("fault status check", vtd_fault_status() == 0);
> > +
> > +	vtd_enable_qi();
> > +	report("QI enablement", vtd_status() | VTD_GCMD_QI);
> > +
> > +	vtd_setup_root_table();
> > +	report("DMAR table setup", vtd_status() | VTD_GCMD_ROOT);
> > +
> > +	vtd_setup_ir_table();
> > +	report("IR table setup", vtd_status() | VTD_GCMD_IR_TABLE);
> 
> The above three report() calls will always pass since you're using OR.

Oops... Thanks. :)

> 
> > +
> > +	vtd_enable_dmar();
> > +	report("DMAR enablement", vtd_status() & VTD_GCMD_DMAR);
> > +
> > +	vtd_enable_ir();
> > +	report("IR enablement", vtd_status() & VTD_GCMD_IR);
> 
> You've reproduced vtd_init() here. Why not just call it and then
> do a sequence of report() calls where you check each register
> has the expected value? I.e.
> 
>  vtd_init();
>  status = vtd_readl(DMAR_GSTS_REG);
>  report("QI enablement", status & VTD_GCMD_QI);
>  report("DMAR table setup", status & VTD_GCMD_ROOT);
>  ...
> 
> Would that not work with this device?

I think it should work. Will take your advice.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test

[Andrew Jones]

On Fri, Oct 21, 2016 at 05:52:50PM +0800, Peter Xu wrote:
> On Thu, Oct 20, 2016 at 11:30:30AM +0200, Andrew Jones wrote:
> 
> [...]
> 
> > > +void vtd_reg_write_4(unsigned int reg, uint32_t value)
> > > +{
> > > +	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> > > +}
> > > +
> > > +void vtd_reg_write_8(unsigned int reg, uint64_t value)
> > > +{
> > > +	*(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> > > +}
> > > +
> > > +uint32_t vtd_reg_read_4(unsigned int reg)
> > > +{
> > > +	return *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> > > +}
> > > +
> > > +uint64_t vtd_reg_read_8(unsigned int reg)
> > > +{
> > > +	return *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> > > +}
> > 
> > The above could be static inlines in intel-iommu.h. How about
> > calling them vtd_readl, vtd_readq, vtd_writel, vtd_writeq? That
> > would make them more consistent with the readl, readq... in
> > lib/asm-generic/io.h
> 
> Sure! Will fix.
> 
> > 
> > > +
> > > +uint32_t vtd_version(void)
> > > +{
> > > +	return vtd_reg_read_4(DMAR_VER_REG);
> > > +}
> > > +
> > > +uint64_t vtd_cap(void)
> > > +{
> > > +	return vtd_reg_read_8(DMAR_CAP_REG);
> > > +}
> > > +
> > > +uint64_t vtd_ecap(void)
> > > +{
> > > +	return vtd_reg_read_8(DMAR_ECAP_REG);
> > > +}
> > > +
> > > +uint32_t vtd_status(void)
> > > +{
> > > +	return vtd_reg_read_4(DMAR_GSTS_REG);
> > > +}
> > > +
> > > +uint32_t vtd_fault_status(void)
> > > +{
> > > +	return vtd_reg_read_4(DMAR_FSTS_REG);
> > > +}
> > > +
> > > +uint64_t vtd_root_table(void)
> > > +{
> > > +	/* No extend root table support yet */
> > > +	return vtd_reg_read_8(DMAR_RTADDR_REG);
> > > +}
> > 
> > I'd drop all the above and use vtd_readl/q(DMAR_CAP_REG) directly,
> > the wrappers don't add anything and take the unit test writer
> > further away from the register names that they should be more
> > familiar with.
> 
> Here I used wrapper since I don't want to remember all the register
> names, and (especially) I don't want to remember the size of the
> registers.

You won't. You'll have the spec open reading them and then use them
in the code. Reviewers will also open the spec and then search for
them in the code. Nobody will memorize them, they'll just match them.

> 
> For example, when I want to read ECAP register, calling vtd_ecap()
> will let me make sure the return value is 8 bytes (compiler will help
> to make sure of it, and I can simply know it from seeing the return
> type of the wrapper function, which is uint64_t), and I don't need to
> bother whether I should use vtd_readl() or vtd_readq() for it. If I
> don't have vtd_ecap() wrapper, I can't see the length from the
> register name only, and I need to check the spec every time, or with
> comment like:
> 
>   #define DMAR_ECAP_REG XXX /* 8 bytes */

That comment sounds good to me :-)

> 
> In that case, I'd prefer with a wrapper for the registers (maybe I can
> move the functions into header files as well, with inline static).
> 
> > 
> > > +
> > > +uint64_t vtd_ir_table(void)
> > > +{
> > > +	return vtd_reg_read_8(DMAR_IRTA_REG) & 0xfffffffffffff000;
> > > +}
> > 
> > This one has a mask, so makes sense to keep the wrapper. But the
> > mask looks like PAGE_MASK to me. Why not use that? Or even drop
> > this wrapper and just use PAGE_MASK when necessary directly...
> 
> The last 12 bits have other means (I didn't check it here, it's
> explained in chap 10.4.29 in vt-d spec in case anyone is interested),
> so it's just coincident that it looks like a page mask. However, I
> think I can use PAGE_MASK here.

Yeah, I have no idea bout the mask, /me didn't open the spec. Use
whatever is correct, maybe defining a new mask if this mask is not
always == PAGE_MASK, or even if it is, e.g.

 #define MY_MASK PAGE_MASK

> 
> > 
> > > +
> > > +void vtd_gcmd_or(uint32_t cmd)
> > > +{
> > > +	uint32_t status = vtd_status();
> > > +	/*
> > > +	 * Logically, we need to read DMAR_GSTS_REG until IOMMU
> > > +	 * handles the write request. However for QEMU/KVM, this write
> > > +	 * is always sync, so when this returns, we should be sure
> > 
> > should always be in sync. Sounds like something to unit test :-)
> 
> I am not sure whether I got the point... but real hardware should be
> async, so I don't know whether the code can work on real hardware. For
> QEMU, it is sync, so it's safe for kvm-unit-tests.

I just mean if there are assumptions on how something should work,
then either it should be confirmed and asserted, or even tested
with an explicit try-report unit test.

> 
> > 
> > > +	 * that the GCMD write is done.
> > > +	 */
> > > +	vtd_reg_write_4(DMAR_GCMD_REG, status | cmd);
> > > +}
> > > +
> > > +void vtd_dump_init_info(void)
> > > +{
> > > +	printf("VT-d version:   0x%x\n", vtd_version());
> > > +	printf("     cap:       0x%016lx\n", vtd_cap());
> > > +	printf("     ecap:      0x%016lx\n", vtd_ecap());
> > > +}
> > > +
> > > +void vtd_enable_qi(void)
> > > +{
> > > +	vtd_gcmd_or(VTD_GCMD_QI);
> > > +}
> > 
> > I'd drop this wrapper and use a comment where it's needed like
> > 
> >  vtd_gcmd_or(VTD_GCMD_QI); /* enable QI */
> > 
> > (By now you see a theme. Keep wrappers to a minimal in order
> >  to keep the unit test writer closer to the spec.)
> 
> Yeah. Besides the case that I mentioned above (which I am still not
> quite sure), I agree that I can use register names directly in most
> cases. Will fix.
> 
> > 
> > > +
> > > +void vtd_setup_root_table(void)
> > > +{
> > > +	void *root = alloc_page();
> > > +
> > > +	memset(root, 0, PAGE_SIZE);
> > > +	vtd_reg_write_8(DMAR_RTADDR_REG, virt_to_phys(root));
> > > +	vtd_gcmd_or(VTD_GCMD_ROOT);
> > > +	printf("DMAR table address: 0x%016lx\n", vtd_root_table());
> > > +}
> > > +
> > > +void vtd_setup_ir_table(void)
> > > +{
> > > +	void *root = alloc_page();
> > > +
> > > +	memset(root, 0, PAGE_SIZE);
> > > +	vtd_reg_write_8(DMAR_IRTA_REG, virt_to_phys(root));
> > > +	/* 0xf stands for table size (2^(0xf+1) == 65536) */
> > > +	vtd_gcmd_or(VTD_GCMD_IR_TABLE | 0xf);
> > > +	printf("IR table address: 0x%016lx\n", vtd_ir_table());
> > > +}
> > > +
> > > +void vtd_enable_dmar(void)
> > > +{
> > > +	vtd_gcmd_or(VTD_GCMD_DMAR);
> > > +}
> > > +
> > > +void vtd_enable_ir(void)
> > > +{
> > > +	vtd_gcmd_or(VTD_GCMD_IR);
> > > +}
> > 
> > More wrappers to drop.
> 
> Ok.
> 
> > 
> > > +
> > > +void vtd_init(void)
> > > +{
> > > +	setup_vm();
> > > +	smp_init();
> > > +	setup_idt();
> > > +
> > > +	vtd_dump_init_info();
> > > +	vtd_enable_qi();
> > > +	vtd_setup_root_table();
> > > +	vtd_setup_ir_table();
> > > +	vtd_enable_dmar();
> > > +	vtd_enable_ir();
> > 
> > I'd open code all the above here. The functions aren't really
> > gaining us anything that comments can't do for us.
> 
> Will fix.
> 
> > 
> > > +}
> > > diff --git a/lib/x86/intel-iommu.h b/lib/x86/intel-iommu.h
> > > new file mode 100644
> > > index 0000000..0f687d1
> > > --- /dev/null
> > > +++ b/lib/x86/intel-iommu.h
> > > @@ -0,0 +1,106 @@
> > > +/*
> > > + * Intel IOMMU header
> > 
> > Please point out the kernel source these defines come from,
> > include/linux/intel-iommu.h
> 
> Will add.
> 
> [...]
> 
> > > +int main(int argc, char *argv[])
> > > +{
> > > +	setup_vm();
> > > +	smp_init();
> > > +	setup_idt();
> > > +
> > > +	vtd_dump_init_info();
> > > +	report("init status check", vtd_status() == 0);
> > > +	report("fault status check", vtd_fault_status() == 0);
> > > +
> > > +	vtd_enable_qi();
> > > +	report("QI enablement", vtd_status() | VTD_GCMD_QI);
> > > +
> > > +	vtd_setup_root_table();
> > > +	report("DMAR table setup", vtd_status() | VTD_GCMD_ROOT);
> > > +
> > > +	vtd_setup_ir_table();
> > > +	report("IR table setup", vtd_status() | VTD_GCMD_IR_TABLE);
> > 
> > The above three report() calls will always pass since you're using OR.
> 
> Oops... Thanks. :)
> 
> > 
> > > +
> > > +	vtd_enable_dmar();
> > > +	report("DMAR enablement", vtd_status() & VTD_GCMD_DMAR);
> > > +
> > > +	vtd_enable_ir();
> > > +	report("IR enablement", vtd_status() & VTD_GCMD_IR);
> > 
> > You've reproduced vtd_init() here. Why not just call it and then
> > do a sequence of report() calls where you check each register
> > has the expected value? I.e.
> > 
> >  vtd_init();
> >  status = vtd_readl(DMAR_GSTS_REG);
> >  report("QI enablement", status & VTD_GCMD_QI);
> >  report("DMAR table setup", status & VTD_GCMD_ROOT);
> >  ...
> > 
> > Would that not work with this device?
> 
> I think it should work. Will take your advice.
>

Thanks,
drew 

Re: [kvm-unit-tests PATCH 03/14] x86: intel-iommu: add vt-d init test

[Peter Xu]

On Fri, Oct 21, 2016 at 02:18:04PM +0200, Andrew Jones wrote:
> On Fri, Oct 21, 2016 at 05:52:50PM +0800, Peter Xu wrote:
> > On Thu, Oct 20, 2016 at 11:30:30AM +0200, Andrew Jones wrote:
> > 
> > [...]
> > 
> > > > +void vtd_reg_write_4(unsigned int reg, uint32_t value)
> > > > +{
> > > > +	*(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> > > > +}
> > > > +
> > > > +void vtd_reg_write_8(unsigned int reg, uint64_t value)
> > > > +{
> > > > +	*(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg) = value;
> > > > +}
> > > > +
> > > > +uint32_t vtd_reg_read_4(unsigned int reg)
> > > > +{
> > > > +	return *(uint32_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> > > > +}
> > > > +
> > > > +uint64_t vtd_reg_read_8(unsigned int reg)
> > > > +{
> > > > +	return *(uint64_t *)(Q35_HOST_BRIDGE_IOMMU_ADDR + reg);
> > > > +}
> > > 
> > > The above could be static inlines in intel-iommu.h. How about
> > > calling them vtd_readl, vtd_readq, vtd_writel, vtd_writeq? That
> > > would make them more consistent with the readl, readq... in
> > > lib/asm-generic/io.h
> > 
> > Sure! Will fix.
> > 
> > > 
> > > > +
> > > > +uint32_t vtd_version(void)
> > > > +{
> > > > +	return vtd_reg_read_4(DMAR_VER_REG);
> > > > +}
> > > > +
> > > > +uint64_t vtd_cap(void)
> > > > +{
> > > > +	return vtd_reg_read_8(DMAR_CAP_REG);
> > > > +}
> > > > +
> > > > +uint64_t vtd_ecap(void)
> > > > +{
> > > > +	return vtd_reg_read_8(DMAR_ECAP_REG);
> > > > +}
> > > > +
> > > > +uint32_t vtd_status(void)
> > > > +{
> > > > +	return vtd_reg_read_4(DMAR_GSTS_REG);
> > > > +}
> > > > +
> > > > +uint32_t vtd_fault_status(void)
> > > > +{
> > > > +	return vtd_reg_read_4(DMAR_FSTS_REG);
> > > > +}
> > > > +
> > > > +uint64_t vtd_root_table(void)
> > > > +{
> > > > +	/* No extend root table support yet */
> > > > +	return vtd_reg_read_8(DMAR_RTADDR_REG);
> > > > +}
> > > 
> > > I'd drop all the above and use vtd_readl/q(DMAR_CAP_REG) directly,
> > > the wrappers don't add anything and take the unit test writer
> > > further away from the register names that they should be more
> > > familiar with.
> > 
> > Here I used wrapper since I don't want to remember all the register
> > names, and (especially) I don't want to remember the size of the
> > registers.
> 
> You won't. You'll have the spec open reading them and then use them
> in the code. Reviewers will also open the spec and then search for
> them in the code. Nobody will memorize them, they'll just match them.

Ok.

> 
> > 
> > For example, when I want to read ECAP register, calling vtd_ecap()
> > will let me make sure the return value is 8 bytes (compiler will help
> > to make sure of it, and I can simply know it from seeing the return
> > type of the wrapper function, which is uint64_t), and I don't need to
> > bother whether I should use vtd_readl() or vtd_readq() for it. If I
> > don't have vtd_ecap() wrapper, I can't see the length from the
> > register name only, and I need to check the spec every time, or with
> > comment like:
> > 
> >   #define DMAR_ECAP_REG XXX /* 8 bytes */
> 
> That comment sounds good to me :-)

I see that these registers already have comments, I think it'll be
good to make these lines exactly the same as original header in
Linux/QEMU source, so I didn't do that. :)

However, I'll remove all the useless wrappers and use direct accesses
to registers when necessary. 

> 
> > 
> > In that case, I'd prefer with a wrapper for the registers (maybe I can
> > move the functions into header files as well, with inline static).
> > 
> > > 
> > > > +
> > > > +uint64_t vtd_ir_table(void)
> > > > +{
> > > > +	return vtd_reg_read_8(DMAR_IRTA_REG) & 0xfffffffffffff000;
> > > > +}
> > > 
> > > This one has a mask, so makes sense to keep the wrapper. But the
> > > mask looks like PAGE_MASK to me. Why not use that? Or even drop
> > > this wrapper and just use PAGE_MASK when necessary directly...
> > 
> > The last 12 bits have other means (I didn't check it here, it's
> > explained in chap 10.4.29 in vt-d spec in case anyone is interested),
> > so it's just coincident that it looks like a page mask. However, I
> > think I can use PAGE_MASK here.
> 
> Yeah, I have no idea bout the mask, /me didn't open the spec. Use
> whatever is correct, maybe defining a new mask if this mask is not
> always == PAGE_MASK, or even if it is, e.g.
> 
>  #define MY_MASK PAGE_MASK

Yeah this looks clean. Will use it.

> 
> > 
> > > 
> > > > +
> > > > +void vtd_gcmd_or(uint32_t cmd)
> > > > +{
> > > > +	uint32_t status = vtd_status();
> > > > +	/*
> > > > +	 * Logically, we need to read DMAR_GSTS_REG until IOMMU
> > > > +	 * handles the write request. However for QEMU/KVM, this write
> > > > +	 * is always sync, so when this returns, we should be sure
> > > 
> > > should always be in sync. Sounds like something to unit test :-)
> > 
> > I am not sure whether I got the point... but real hardware should be
> > async, so I don't know whether the code can work on real hardware. For
> > QEMU, it is sync, so it's safe for kvm-unit-tests.
> 
> I just mean if there are assumptions on how something should work,
> then either it should be confirmed and asserted, or even tested
> with an explicit try-report unit test.

I see. Maybe it'll be nicer I just re-write this function to not do
such an assumption. After all, it is not hard to add one more step to
check status.

Will fix. Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 04/14] pci: refactor init process to pci_dev_init()

[Peter Xu]

On Thu, Oct 20, 2016 at 12:02:58PM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:42PM +0800, Peter Xu wrote:
> > pci_find_dev() is not sufficient for further tests for Intel IOMMU. This
> > patch introduced pci_dev struct, as a abstract of a specific PCI device.
> > 
> > All the rest of current PCI APIs are changed to leverage the pci_dev
> > struct.
> > 
> > x86/vmexit.c is the only user of the old PCI APIs. Changing it to use
> > the new ones.
> > 
> > (Indentation is fixed from using 4 spaces into tab in pci.c)
> 
> We need to get Alex's series in and then revisit this. He's maintained
> the API's use of a devid handle. It's possible a struct would be better,
> but I'm not so sure. In any case we need to break this patch up in order
> to see the goals step-by-step.
> 
> 1) cleanup style - Alex's series does that already
> 2) replace the devid handle with struct pci_dev - should be done
> everywhere or nowhere
> 3) simplify x86/vmexit's by making better use of the API
> 4) extend the API for IOMMU

Yes. I'll follow above steps to split the patch. I think I'll just
rebase the branch to Alex's for next version.

[...]

> > +int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
> > +{
> > +	unsigned i;
> > +	uint32_t id;
> > +
> > +	memset(dev, 0, sizeof(*dev));
> > +
> > +	for (i = 0; i < PCI_DEVFN_MAX; ++i) {
> > +		id = pci_config_read(i, 0);
> 
> Hmm, pci_config_read still uses devid, but everything else now uses
> pci_dev. I don't really like that inconsistency.

Yes, I think a struct for PCI device might be still essential
(otherwise I don't know where to store per-device PCI information,
like MSI offsets, etc.). In that sense, I would prefer using pci_dev
in pci_config_*() ops. Will fix based on Alex's tree.

> 
> > +		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
> > +			break;
> > +	}
> > +
> > +	if (i == PCI_DEVFN_MAX) {
> > +		printf("PCI: failed init dev (vendor: 0x%04x, "
> > +		       "device: 0x%04x)\n", vendor_id, device_id);
> > +		return -1;
> > +	}
> > +
> > +	dev->pci_addr = i;
> 
> but i is the devid, not the addr. Unless I misunderstand what 'addr' means
> here.

Right. Maybe I should rename pci_addr to pci_bdf.

[...]

> > @@ -27,8 +38,6 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> >  #define PCI_VENDOR_ID_REDHAT		0x1b36
> >  #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
> >  
> > -#define PCI_TESTDEV_NUM_BARS		2
> 
> Why remove this? I could be used to test that we only find this many
> BARs on the testdev, no?

Because there's no code referencing this macro, so cleaned it up. I
don't remember whether Alex is using it, I think I can avoid touching
it in next version.

> 
> > -
> >  struct pci_test_dev_hdr {
> >  	uint8_t  test;
> >  	uint8_t  width;
> > @@ -39,4 +48,12 @@ struct pci_test_dev_hdr {
> >  	uint8_t  name[];
> >  };
> >  
> > +enum pci_dma_dir {
> > +	PCI_DMA_FROM_DEVICE = 0,
> > +	PCI_DMA_TO_DEVICE,
> > +};
> > +typedef enum pci_dma_dir pci_dma_dir_t;
> > +
> > +typedef uint64_t iova_t;
> 
> stray changes

Yeah... I'll move them outside of this patch, to somewhere more
suitable.

[...]

> > +	if (!pci_dev_init(&dev, PCI_VENDOR_ID_REDHAT,
> > +			  PCI_DEVICE_ID_REDHAT_TEST)) {
> > +		pci_test.memaddr = ioremap(dev.pci_bar[0], PAGE_SIZE);
> > +		pci_test.iobar = dev.pci_bar[1];
> 
> Before we didn't require BAR0 to be MEM and BAR1 to be IO, now we do. It's
> probably safe, but less flexible. I think we should at least provide
> asserts that our assumptions are correct.

Sure. It'll be better to have asserts here.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 05/14] page: add page alignment checker

[Peter Xu]

On Thu, Oct 20, 2016 at 02:30:26PM +0200, Andrew Jones wrote:
> On Thu, Oct 20, 2016 at 02:23:14PM +0200, Andrew Jones wrote:
> > On Fri, Oct 14, 2016 at 08:40:43PM +0800, Peter Xu wrote:
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  lib/asm-generic/page.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/lib/asm-generic/page.h b/lib/asm-generic/page.h
> > > index 7b8a08b..cbdc8f6 100644
> > > --- a/lib/asm-generic/page.h
> > > +++ b/lib/asm-generic/page.h
> 
> Another comment. IS_ALIGNED shouldn't be here, it should be
> in lib/libcflat.h under the definition of ALIGN there.

Moving over.

> 
> drew
> 
> > > @@ -19,6 +19,11 @@
> > >  
> > >  #define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
> > >  
> > > +#define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
> > > +#define PAGE_ALIGNED_4K(x)	IS_ALIGNED((x), (0x1000))
> > > +#define PAGE_ALIGNED_2M(x)	IS_ALIGNED((x), (0x200000))
> > > +#define PAGE_ALIGNED_1G(x)	IS_ALIGNED((x), (0x40000000))
> > 
> > I like IS_ALIGNED, but not the others. The others are strangely
> > named because it's half asking if an address is page aligned and
> > half asking if it's aligned to a given boundary. Why not just
> > ask the boundary, like ALIGNED_2M(x)? Anyway, my preference would
> > be to replace them with SZ_4K, SZ_2M, SZ_1G definitions. Users would
> > then do
> > 
> >  IS_ALIGNED(addr, SZ_2M)
> > 
> > or whatever, which isn't much more typing.

Yeah these looks more clean. Will take them. Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 06/14] util: move MAX/MIN macro into util.h

[Peter Xu]

On Thu, Oct 20, 2016 at 02:28:25PM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:44PM +0800, Peter Xu wrote:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/alloc.c | 3 ---
> >  lib/util.h  | 3 +++
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/alloc.c b/lib/alloc.c
> > index e1d7b8a..58af52b 100644
> > --- a/lib/alloc.c
> > +++ b/lib/alloc.c
> > @@ -7,9 +7,6 @@
> >  #include "asm/spinlock.h"
> >  #include "asm/io.h"
> >  
> > -#define MIN(a, b)		((a) < (b) ? (a) : (b))
> > -#define MAX(a, b)		((a) > (b) ? (a) : (b))
> > -
> >  #define PHYS_ALLOC_NR_REGIONS	256
> >  
> >  struct phys_alloc_region {
> > diff --git a/lib/util.h b/lib/util.h
> > index 4c4b441..1462f4f 100644
> > --- a/lib/util.h
> > +++ b/lib/util.h
> > @@ -20,4 +20,7 @@
> >   */
> >  extern int parse_keyval(char *s, long *val);
> >  
> > +#define MIN(a, b)		((a) < (b) ? (a) : (b))
> > +#define MAX(a, b)		((a) > (b) ? (a) : (b))
> > +
> >  #endif
> > -- 
> > 2.7.4
> >
> 
> I'd prefer they move to lib/libcflat.h. Currently util is for unit
> test utilities. MIN/MAX are useful to the library code as well. We
> shouldn't require util.h be included by library code.
> 
> (I agree libcflat.h is getting ugly, but cleaning it up is beyond the
>  scope of this series and right now MIN/MAX would fit there best.)

Yep. Fixing up. Thanks,

-- peterx

Re: [kvm-unit-tests PATCH 08/14] x86: pci: add pci_config_{read|write}[bw]() helpers

[Peter Xu]

On Thu, Oct 20, 2016 at 02:43:21PM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:46PM +0800, Peter Xu wrote:
> > And some cleanup on the file.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/x86/asm/pci.h | 37 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 34 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> > index cddde41..ce970e4 100644
> > --- a/lib/x86/asm/pci.h
> > +++ b/lib/x86/asm/pci.h
> > @@ -9,11 +9,42 @@
> >  #include "pci.h"
> >  #include "x86/asm/io.h"
> >  
> > +#define PCI_HOST_CONFIG_PORT (0xcf8)
> > +#define PCI_HOST_DATA_PORT   (0xcfc)
> 
> Thomas wanted to do this to, but I disagreed because CF8/CFC are
> effectively the names already.
> 
> Alex already has a patch in his series just like this one
> "pci: x86: Add remaining PCI configuration space accessors"

Right. Will drop this patch. I should mention it in the cover letter.
Sorry!

-- peterx

Re: [kvm-unit-tests PATCH 09/14] pci: provide pci_set_master()

[Peter Xu]

On Thu, Oct 20, 2016 at 02:49:50PM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:47PM +0800, Peter Xu wrote:
> > And set master by default for pci devices. So that DMA is allowed.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/pci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/lib/pci.c b/lib/pci.c
> > index ef0b02a..044e4db 100644
> > --- a/lib/pci.c
> > +++ b/lib/pci.c
> > @@ -7,6 +7,13 @@
> >  #include "pci.h"
> >  #include "asm/pci.h"
> >  
> > +static void pci_set_master(pci_dev *dev, int master)
> > +{
> > +	uint32_t val = pci_config_read(dev->pci_addr, PCI_COMMAND);
> > +	val |= PCI_COMMAND_MASTER;
> > +	pci_config_write(dev->pci_addr, PCI_COMMAND, val);
> > +}
> 
> I agree with the API extension (well, I know next to nothing about PCI,
> so I'll assume this function is correct...) But...
> 
> > +
> >  /*
> >   * Scan bus look for a specific device. Only bus 0 scanned for now.
> >   * After the scan, a pci_dev is returned with correct BAR information.
> > @@ -41,6 +48,8 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
> >  	}
> >  	dev->inited = true;
> >  
> > +	pci_set_master(dev, 1);
> 
> ...I don't necessarily agree with it being enabled by default. In the
> least I think you want a minimal init function available to unit tests
> that they then can extend with their own custom init. Then you can also
> provide an init that configures a bunch of stuff you think is a
> good idea by default. For example, in my gic series I try to do that. I
> have a gic_init() function which does nothing except set some base
> addresses. Then, I have a gic_enable_defaults function that unit tests
> can use if they don't care how we enable it, they just want something
> that works. Otherwise the unit test would just call gic_init and then
> write all the registers for enabling it that they want/need.

Good idea. will follow.

First of all, I'll introduce pci_dev_init() to init the raw structure.
Then, will add one pci_enable_defaults() with all the rests.

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH 10/14] pci: add bdf helpers

[Peter Xu]

On Thu, Oct 20, 2016 at 02:55:39PM +0200, Andrew Jones wrote:
> On Fri, Oct 14, 2016 at 08:40:48PM +0800, Peter Xu wrote:
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  lib/pci.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/lib/pci.h b/lib/pci.h
> > index b8755ff..6a1c3c9 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -18,6 +18,12 @@ enum {
> >  #define PCI_BAR_NUM                     (6)
> >  #define PCI_DEVFN_MAX                   (256)
> >  
> > +#define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
> > +#define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
> > +#define PCI_SLOT(devfn)                 (((devfn) >> 3) & 0x1f)
> > +#define PCI_FUNC(devfn)                 ((devfn) & 0x07)
> > +#define PCI_BUILD_BDF(bus, devfn)       ((bus << 8) | (devfn))
> 
> As I mention frequently; I know nothing about PCI, but this doesn't
> look right to me. DEVFN should be defined as
> 
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> 
> Care to explain these? Please provide the explanation in the commit
> message to along with a motivation for adding them.

Looks like the above lines are not conflicting? E.g., the above patch
(PCI_SLOT and PCI_FUNC) provides way to abstract slot/function out of
devfn value, while the one you mentioned (PCI_DEVFN) should be the
builder macro that builds a devfn value from its slot and function?

However... I just found that I didn't use
PCI_SLOT/PCI_FUNC/PCI_BUILD_BDF macros in the following patches, so
maybe I should just remove the last three of them...

Thanks,

-- peterx

Re: [kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers

[Peter Xu]

On Thu, Oct 20, 2016 at 03:19:28PM +0200, Andrew Jones wrote:

[...]

> > +#include "pci-edu.h"
> > +
> > +#define  PCI_VENDOR_ID_QEMU              (0x1234)
> > +#define  PCI_DEVICE_ID_EDU               (0x11e8)
> > +
> > +/* The only bar used by EDU device */
> > +#define EDU_BAR_MEM                 (0)
> > +#define EDU_MAGIC                   (0xed)
> > +#define EDU_VERSION                 (0x100)
> > +#define EDU_DMA_BUF_SIZE            (1 << 20)
> > +#define EDU_INPUT_BUF_SIZE          (256)
> > +
> > +#define EDU_REG_ID                  (0x0)
> > +#define EDU_REG_ALIVE               (0x4)
> > +#define EDU_REG_FACTORIAL           (0x8)
> > +#define EDU_REG_STATUS              (0x20)
> > +#define EDU_REG_DMA_SRC             (0x80)
> > +#define EDU_REG_DMA_DST             (0x88)
> > +#define EDU_REG_DMA_COUNT           (0x90)
> > +#define EDU_REG_DMA_CMD             (0x98)
> > +
> > +#define EDU_CMD_DMA_START           (0x01)
> > +#define EDU_CMD_DMA_FROM            (0x02)
> > +#define EDU_CMD_DMA_TO              (0x00)
> > +
> > +#define EDU_STATUS_FACTORIAL        (0x1)
> > +#define EDU_STATUS_INT_ENABLE       (0x80)
> > +
> > +#define EDU_DMA_START               (0x40000)
> > +#define EDU_DMA_SIZE_MAX            (4096)
> 
> shouldn't the above defines be in the header?

I put them here since these are macros that I think will only be used
by this file only. However I think it'll still be okay to move them
into header files, so I'll do that.

> 
> > +
> > +uint64_t edu_reg_readq(pci_edu_dev_t *dev, int reg)
> > +{
> > +	return *(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
> > +}
> > +
> > +uint32_t edu_reg_read(pci_edu_dev_t *dev, int reg)
> > +{
> > +	return *(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
> > +}
> > +
> > +void edu_reg_writeq(pci_edu_dev_t *dev, int reg, uint64_t val)
> > +{
> > +	*(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
> > +}
> > +
> > +void edu_reg_write(pci_edu_dev_t *dev, int reg, uint32_t val)
> > +{
> > +	*(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
> > +}
> 
> I'd put these accessors in the header as static inlines too

Sure. Will do.

> 
> > +
> > +/* Return true if alive */
> > +bool edu_check_alive(pci_edu_dev_t *dev)
> > +{
> > +	static uint32_t live_count = 1;
> > +	uint32_t value;
> > +
> > +	edu_reg_write(dev, EDU_REG_ALIVE, live_count++);
> > +	value = edu_reg_read(dev, EDU_REG_ALIVE);
> > +	return (live_count - 1 == ~value);
> > +}
> 
> Even edu_check_alive could be a static inline in the header,
> if it's something you'll do frequently. If it's only needed
> for a sanity init test then I'd just inline it directly in
> the one caller, edu_init

Will do.

> 
> > +
> > +int edu_init(pci_edu_dev_t *dev)
> > +{
> > +	int ret;
> > +
> > +	ret = pci_dev_init(&dev->pci_dev, PCI_VENDOR_ID_QEMU,
> > +			   PCI_DEVICE_ID_EDU);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!edu_check_alive(dev)) {
> > +		printf("edu device not alive!\n");
> > +		return -1;
> 
> should this ever fail? Or would an assert by fine here?
>  alive = edu_check_alive(dev)
>  assert(alive);

Yep. It should not fail. Will take assert.

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +uint32_t edu_status(pci_edu_dev_t *dev)
> > +{
> > +	return edu_reg_read(dev, EDU_REG_STATUS);
> > +}
> 
> please, no unnecessary wrappers

Ok.

> 
> > +
> > +void edu_dma(pci_edu_dev_t *dev, iova_t iova,
> > +	     size_t size, int dev_offset, pci_dma_dir_t dir)
> > +{
> > +	uint64_t from, to;
> > +	uint32_t cmd = EDU_CMD_DMA_START;
> > +
> > +	assert(size <= EDU_DMA_SIZE_MAX);
> > +	assert(dev_offset < EDU_DMA_SIZE_MAX &&
> > +	       dev_offset >= 0);
> > +
> > +	printf("edu device DMA start %s addr %p size 0x%lu off 0x%x\n",
> > +	       dir == PCI_DMA_FROM_DEVICE ? "FROM" : "TO",
> > +	       (void *)iova, size, dev_offset);
> 
> is pci_dma_dir_t just a binary enum? If so, then I find it
> sort of crufty. Can't we just have a 'bool write'?

Will replace with "bool from_device".

> 
> > +
> > +	if (dir == PCI_DMA_FROM_DEVICE) {
> > +		from = dev_offset + EDU_DMA_START;
> > +		to = iova;
> > +		cmd |= EDU_CMD_DMA_FROM;
> > +	} else {
> > +		from = iova;
> > +		to = EDU_DMA_START + dev_offset;
> > +		cmd |= EDU_CMD_DMA_TO;
> > +	}
> > +
> > +	edu_reg_writeq(dev, EDU_REG_DMA_SRC, from);
> > +	edu_reg_writeq(dev, EDU_REG_DMA_DST, to);
> > +	edu_reg_writeq(dev, EDU_REG_DMA_COUNT, size);
> > +	edu_reg_write(dev, EDU_REG_DMA_CMD, cmd);
> > +
> > +	/* Wait until DMA finished */
> > +	while (edu_reg_read(dev, EDU_REG_DMA_CMD) & EDU_CMD_DMA_START);
> 
> You may consider adding a cpu_relax() to lib/x86/asm/barrier.h, like
> arm and ppc have. I noticed a big difference when running with tcg
> on how fast a 'while (state-not-changed);' loop can complete when
> adding that barrier.

Should I just do:

 #include <asm-generic/barrier.h>

in x86/asm/barrier.h, just like ppc?

Btw, could you elaborate what would be the difference? I see
cpu_relax() is defined as:

 #define cpu_relax()	asm volatile(""    : : : "memory")

Then how would the two differ:

(1) while (xxx);
(2) while (xxx) cpu_relax();

(I thought they are still the same? maybe I should assume variables in
 xxx should have volatile keywords)

> 
> > +}
> > diff --git a/lib/pci-edu.h b/lib/pci-edu.h
> > new file mode 100644
> > index 0000000..6b7dbfd
> > --- /dev/null
> > +++ b/lib/pci-edu.h
> > @@ -0,0 +1,29 @@
> > +/*
> > + * Edu PCI device header.
> > + *
> > + * Copyright (C) 2016 Red Hat, Inc.
> > + *
> > + * Authors:
> > + *   Peter Xu <peterx@redhat.com>,
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2 or
> > + * later.
> > + *
> > + * Edu device is a virtualized device in QEMU. Please refer to
> > + * docs/specs/edu.txt in QEMU repository for EDU device manual.
> > + */
> > +#ifndef __PCI_EDU_H__
> > +#define __PCI_EDU_H__
> > +
> > +#include "pci.h"
> > +
> > +struct pci_edu_dev {
> > +	pci_dev pci_dev;
> > +};
> > +typedef struct pci_edu_dev pci_edu_dev_t;
> 
> Why wrap pci_dev in this pci_edu_dev struct? Will there ever
> be more state? Should we just wait and see? Also, why use a
> typedef for pci_dev (assuming we want it)? 'struct pci_dev'
> would be more kernel like.

My own preference is to use typedef just like how QEMU is using it, to
avoid typing "struct", and with more flexibility (e.g., I can just
change a struct into union without touching other part of code, as
long as I keep all the existing fields' name there). However I think I
should follow how kvm-unit-tests/Linux's coding style to use struct.

But should I still keep the wrapper even if there is only one field
which is "struct pci_dev"? Benefits:

(1) people won't be able to edu_dma() a PCI device which is not a edu
    device, or say "struct pci_edu_dev" type is checked during
    compilation.

(2) in case edu device will need more fields, we won't need to touch
    everywhere and change the old "struct pci_dev" into "struct
    pci_edu_dev".

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 13/14] pci: add msi support for 32/64bit address

[Peter Xu]

On Thu, Oct 20, 2016 at 03:30:32PM +0200, Andrew Jones wrote:

[...]

> > +static void pci_cap_walk(pci_dev *dev)
> > +{
> > +	uint8_t cap_offset;
> > +	uint8_t cap_id;
> > +
> > +	cap_offset = pci_config_readb(dev->pci_addr, PCI_CAPABILITY_LIST);
> > +	while (cap_offset) {
> > +		cap_id = pci_config_readb(dev->pci_addr, cap_offset);
> > +		printf("PCI detected cap 0x%x\n", cap_id);
> > +		if (cap_handlers[cap_id]) {
> > +			cap_handlers[cap_id](dev, cap_offset);
> > +		}
> 
> nit: no need for {}

Will fix.

[...]

> > @@ -49,6 +107,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
> >  	dev->inited = true;
> >  
> >  	pci_set_master(dev, 1);
> > +	pci_cap_walk(dev);
> 
> again, not sure we want this in the one and only init function

I'll put it into the new pci_enable_defaults().

> 
> >  
> >  	return 0;
> >  }
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 6a1c3c9..5581446 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -27,6 +27,7 @@ enum {
> >  struct pci_dev {
> >  	bool inited;
> >  	uint16_t pci_addr;
> > +	uint16_t msi_offset;
> >  	phys_addr_t pci_bar[PCI_BAR_NUM];
> >  };
> >  typedef struct pci_dev pci_dev;
> > @@ -35,6 +36,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
> >  phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
> >  bool pci_bar_is_memory(pci_dev *dev, int bar_num);
> >  bool pci_bar_is_valid(pci_dev *dev, int bar_num);
> > +void pci_setup_msi(pci_dev *dev, uint64_t msi_addr, uint32_t msi_data);
> >  
> >  /*
> >   * pci-testdev is a driver for the pci-testdev qemu pci device. The
> > -- 
> > 2.7.4
> >
> 
> Looks good from my not knowing much about PCI perspective.

Thanks for review!

-- peterx

Re: [kvm-unit-tests PATCH 14/14] x86: intel-iommu: add IR test

[Peter Xu]

On Thu, Oct 20, 2016 at 03:45:14PM +0200, Andrew Jones wrote:

[...]

> > --- a/lib/pci-edu.c
> > +++ b/lib/pci-edu.c
> > @@ -35,9 +35,6 @@
> >  #define EDU_CMD_DMA_FROM            (0x02)
> >  #define EDU_CMD_DMA_TO              (0x00)
> >  
> > -#define EDU_STATUS_FACTORIAL        (0x1)
> > -#define EDU_STATUS_INT_ENABLE       (0x80)
> > -
> 
> I think these and all EDU_ defines should have been in
> pci-edu.h in the first place.

Will move them to header file.

> 
> >  #define EDU_DMA_START               (0x40000)
> >  #define EDU_DMA_SIZE_MAX            (4096)
> >  
> > @@ -94,6 +91,21 @@ uint32_t edu_status(pci_edu_dev_t *dev)
> >  	return edu_reg_read(dev, EDU_REG_STATUS);
> >  }
> >  
> > +void edu_setup_intr(pci_edu_dev_t *dev, bool enabled)
> > +{
> > +	edu_reg_write(dev, EDU_REG_STATUS, EDU_STATUS_INT_ENABLE);
> > +}
> > +
> > +void edu_fact_write(pci_edu_dev_t *dev, uint32_t in)
> > +{
> > +	edu_reg_write(dev, EDU_REG_FACTORIAL, in);
> > +}
> > +
> > +uint32_t edu_fact_read(pci_edu_dev_t *dev)
> > +{
> > +	return edu_reg_read(dev, EDU_REG_FACTORIAL);
> > +}
> 
> More wrappers...

Removing...

[...]

> > +static uint32_t factorial(uint32_t in)
> > +{
> > +	uint32_t v = 1;
> > +	while (in) v *= in--;
> > +	return v;
> > +}
> 
> What, no recursion! That's a disgrace to this classic
> problem! Actually I don't think you should bother with
> the function at all. You hard code the input, so just
> precalculate the result.

Ok, and... yes I can use 0x60 to trigger the interrupt, so no need for
any factorial stuffs. Thanks for mentioning. :)

> 
> > +
> > +static volatile int edu_done;
> > +
> > +static void edu_fact_isr(isr_regs_t *regs)
> > +{
> > +	edu_done = 1;
> 
> nit: bool/true

(will fix this)

> 
> > +	eoi();
> > +}
> > +
> > +static void vtd_test_ir(pci_edu_dev_t *dev)
> > +{
> > +#define VTD_TEST_VECTOR (0xee)
> > +	/* Choose any number to calculate the factorial value. */
> > +	const uint32_t fact = 0x8;
> 
> Nobody is choosing anything. How about just
> 
>  const uint32_t fact_input = 8; /* fact(8) == 40320 */

(will skip since I will use 0x60 later...)

> 
> > +
> > +	/*
> > +	 * Enable EDU device interrupt, so when factorial task
> > +	 * finishes, IRQ will be triggered.
> > +	 */
> > +	edu_setup_intr(dev, 1);
> > +
> > +	/*
> > +	 * Setup EDU PCI device MSI, using interrupt remapping. By
> > +	 * default, EDU device is using INTx.
> > +	 */
> > +	vtd_setup_msi(&dev->pci_dev, VTD_TEST_VECTOR, 0);
> > +
> > +	handle_irq(VTD_TEST_VECTOR, edu_fact_isr);
> > +	irq_enable();
> > +
> > +	edu_fact_write(dev, fact);
> > +	while (!edu_done);
> 
> I guess you depend on the test framework's timeout support to
> deal with never getting the interrupt. Make sure you set a
> reasonable timeout in unittests.cfg

Will do.

> 
> > +	/* Now results should be put back to edu fact register */
> > +	report("EDU factorial INTR test",
> > +	       edu_fact_read(dev) == factorial(fact));
> 
> Do we really need this factorial stuff? Can't we just raise an
> interrupt with the EDU register offset 0x60?

As mentioned, will switch to 0x60.

Thanks!

-- peterx

Re: [kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers

[Andrew Jones]

On Tue, Oct 25, 2016 at 11:34:22AM +0800, Peter Xu wrote:
> > You may consider adding a cpu_relax() to lib/x86/asm/barrier.h, like
> > arm and ppc have. I noticed a big difference when running with tcg
> > on how fast a 'while (state-not-changed);' loop can complete when
> > adding that barrier.
> 
> Should I just do:
> 
>  #include <asm-generic/barrier.h>
> 
> in x86/asm/barrier.h, just like ppc?

Nope. See below

> 
> Btw, could you elaborate what would be the difference? I see
> cpu_relax() is defined as:
> 
>  #define cpu_relax()	asm volatile(""    : : : "memory")
> 
> Then how would the two differ:
> 
> (1) while (xxx);
> (2) while (xxx) cpu_relax();

It inserts a compiler-generated memory barrier, which for ARM TCG
helps relinquish some time to QEMU, allowing QEMU to do what it
needs to do in order for the condition to be fulfilled. I just
checked Linux code for x86's definition of cpu_relax(), though, and
see that it's

 /* REP NOP (PAUSE) is a good thing to insert into busy-wait loops. */
 static __always_inline void rep_nop(void)
 {
    asm volatile("rep; nop" ::: "memory");
 }

 static __always_inline void cpu_relax(void)
 {
    rep_nop();
 }

So that may work better for you.

> 
> (I thought they are still the same? maybe I should assume variables in
>  xxx should have volatile keywords)

Don't assume they do, make sure they do. Without volatile for those
variables you may never stop looping.

> 
> > 
> > > +}
> > > diff --git a/lib/pci-edu.h b/lib/pci-edu.h
> > > new file mode 100644
> > > index 0000000..6b7dbfd
> > > --- /dev/null
> > > +++ b/lib/pci-edu.h
> > > @@ -0,0 +1,29 @@
> > > +/*
> > > + * Edu PCI device header.
> > > + *
> > > + * Copyright (C) 2016 Red Hat, Inc.
> > > + *
> > > + * Authors:
> > > + *   Peter Xu <peterx@redhat.com>,
> > > + *
> > > + * This work is licensed under the terms of the GNU LGPL, version 2 or
> > > + * later.
> > > + *
> > > + * Edu device is a virtualized device in QEMU. Please refer to
> > > + * docs/specs/edu.txt in QEMU repository for EDU device manual.
> > > + */
> > > +#ifndef __PCI_EDU_H__
> > > +#define __PCI_EDU_H__
> > > +
> > > +#include "pci.h"
> > > +
> > > +struct pci_edu_dev {
> > > +	pci_dev pci_dev;
> > > +};
> > > +typedef struct pci_edu_dev pci_edu_dev_t;
> > 
> > Why wrap pci_dev in this pci_edu_dev struct? Will there ever
> > be more state? Should we just wait and see? Also, why use a
> > typedef for pci_dev (assuming we want it)? 'struct pci_dev'
> > would be more kernel like.
> 
> My own preference is to use typedef just like how QEMU is using it, to
> avoid typing "struct", and with more flexibility (e.g., I can just
> change a struct into union without touching other part of code, as
> long as I keep all the existing fields' name there). However I think I
> should follow how kvm-unit-tests/Linux's coding style to use struct.

I would. Note, the kernel tends to wrap unions in structs, avoiding
a need to switch the outer type.

> 
> But should I still keep the wrapper even if there is only one field
> which is "struct pci_dev"? Benefits:
> 
> (1) people won't be able to edu_dma() a PCI device which is not a edu
>     device, or say "struct pci_edu_dev" type is checked during
>     compilation.
> 
> (2) in case edu device will need more fields, we won't need to touch
>     everywhere and change the old "struct pci_dev" into "struct
>     pci_edu_dev".

Fair enough, but I think we need to decide if we even need struct
pci_dev first :-) There's no need to introduce any structs if all
we really need to do is pass around devid.

Thanks,
drew

Re: [kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers

[Peter Xu]

On Tue, Oct 25, 2016 at 12:43:41PM +0200, Andrew Jones wrote:
> On Tue, Oct 25, 2016 at 11:34:22AM +0800, Peter Xu wrote:
> > > You may consider adding a cpu_relax() to lib/x86/asm/barrier.h, like
> > > arm and ppc have. I noticed a big difference when running with tcg
> > > on how fast a 'while (state-not-changed);' loop can complete when
> > > adding that barrier.
> > 
> > Should I just do:
> > 
> >  #include <asm-generic/barrier.h>
> > 
> > in x86/asm/barrier.h, just like ppc?
> 
> Nope. See below
> 
> > 
> > Btw, could you elaborate what would be the difference? I see
> > cpu_relax() is defined as:
> > 
> >  #define cpu_relax()	asm volatile(""    : : : "memory")
> > 
> > Then how would the two differ:
> > 
> > (1) while (xxx);
> > (2) while (xxx) cpu_relax();
> 
> It inserts a compiler-generated memory barrier, which for ARM TCG
> helps relinquish some time to QEMU, allowing QEMU to do what it
> needs to do in order for the condition to be fulfilled. I just
> checked Linux code for x86's definition of cpu_relax(), though, and
> see that it's
> 
>  /* REP NOP (PAUSE) is a good thing to insert into busy-wait loops. */
>  static __always_inline void rep_nop(void)
>  {
>     asm volatile("rep; nop" ::: "memory");
>  }
> 
>  static __always_inline void cpu_relax(void)
>  {
>     rep_nop();
>  }
> 
> So that may work better for you.

A lesson for cpu_relax(). Thanks. :-)

So I guess "rep; nop" is a arch-specific hint for x86. Will add one
more patch for it.

> 
> > 
> > (I thought they are still the same? maybe I should assume variables in
> >  xxx should have volatile keywords)
> 
> Don't assume they do, make sure they do. Without volatile for those
> variables you may never stop looping.

Noted.

[...]

> > > Why wrap pci_dev in this pci_edu_dev struct? Will there ever
> > > be more state? Should we just wait and see? Also, why use a
> > > typedef for pci_dev (assuming we want it)? 'struct pci_dev'
> > > would be more kernel like.
> > 
> > My own preference is to use typedef just like how QEMU is using it, to
> > avoid typing "struct", and with more flexibility (e.g., I can just
> > change a struct into union without touching other part of code, as
> > long as I keep all the existing fields' name there). However I think I
> > should follow how kvm-unit-tests/Linux's coding style to use struct.
> 
> I would. Note, the kernel tends to wrap unions in structs, avoiding
> a need to switch the outer type.

Hmm... right.

> 
> > 
> > But should I still keep the wrapper even if there is only one field
> > which is "struct pci_dev"? Benefits:
> > 
> > (1) people won't be able to edu_dma() a PCI device which is not a edu
> >     device, or say "struct pci_edu_dev" type is checked during
> >     compilation.
> > 
> > (2) in case edu device will need more fields, we won't need to touch
> >     everywhere and change the old "struct pci_dev" into "struct
> >     pci_edu_dev".
> 
> Fair enough, but I think we need to decide if we even need struct
> pci_dev first :-) There's no need to introduce any structs if all
> we really need to do is pass around devid.

I think that may depend on how we target kvm-unit-tests, and how we
will try to extend PCI functions in kvm-unit-tests in the future. For
now, I needed it mostly because of MSI offset. For PCI bar, indeed we
can fetch it everytime we need it. However for MSI offset, it's hidden
inside capability list. If we don't cache it, we will need to traverse
the capability list every time, which is really not that efficient.

If one day we want to play with MSI-X? Or more? If we have such a
requirement, it seems just natural to have a "struct pci_dev" and keep
all the device specific information along with the object. :-)

Thanks!

-- peterx