Re: [PATCH kvm-unit-tests v2 16/17] x86: intel-iommu: add IR MSI test

[Andrew Jones <drjones@redhat.com>]

On Wed, Nov 09, 2016 at 10:10:23AM -0500, 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.
> 
> Meanwhile, IR MSI test is added to intel IOMMU unit test. The basic IR
> test is carried out by a edu INTR raise request. When write to the intr
> raise register, interrupt will be generated. Type of interrupt will
> depend on the setup (either INTx or MSI).
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  lib/pci-edu.h         |  1 +
>  lib/x86/intel-iommu.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/x86/intel-iommu.h |  1 +
>  x86/intel-iommu.c     | 44 +++++++++++++++++++++++-
>  4 files changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/pci-edu.h b/lib/pci-edu.h
> index af457df..5be278c 100644
> --- a/lib/pci-edu.h
> +++ b/lib/pci-edu.h
> @@ -32,6 +32,7 @@
>  #define EDU_REG_ALIVE               (0x4)
>  #define EDU_REG_FACTORIAL           (0x8)
>  #define EDU_REG_STATUS              (0x20)
> +#define EDU_REG_INTR_RAISE          (0x60)
>  #define EDU_REG_DMA_SRC             (0x80)
>  #define EDU_REG_DMA_DST             (0x88)
>  #define EDU_REG_DMA_COUNT           (0x90)
> diff --git a/lib/x86/intel-iommu.c b/lib/x86/intel-iommu.c
> index 912a379..1d5f4b2 100644
> --- a/lib/x86/intel-iommu.c
> +++ b/lib/x86/intel-iommu.c
> @@ -12,6 +12,7 @@
>  
>  #include "intel-iommu.h"
>  #include "libcflat.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;
> +
>  #define VTD_RTA_MASK  (PAGE_MASK)
>  #define VTD_IRTA_MASK (PAGE_MASK)
>  
> @@ -205,6 +226,79 @@ 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(struct 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->bdf;
> +	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
> + */
> +bool vtd_setup_msi(struct 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);

Ideally, we'd have a BUILD_BUG type thing for this, but we don't,
so OK.

> +
> +	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;
> +
> +	return 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 806d9fa..0725f59 100644
> --- a/lib/x86/intel-iommu.h
> +++ b/lib/x86/intel-iommu.h
> @@ -137,5 +137,6 @@ static inline uint64_t vtd_readq(unsigned int reg)
>  
>  void vtd_init(void);
>  void vtd_map_range(uint16_t sid, phys_addr_t iova, phys_addr_t pa, size_t size);
> +bool vtd_setup_msi(struct pci_dev *dev, int vector, int dest_id);
>  
>  #endif
> diff --git a/x86/intel-iommu.c b/x86/intel-iommu.c
> index 755ccb1..cb9a991 100644
> --- a/x86/intel-iommu.c
> +++ b/x86/intel-iommu.c
> @@ -12,8 +12,10 @@
>  
>  #include "intel-iommu.h"
>  #include "pci-edu.h"
> +#include "x86/apic.h"
>  
>  #define VTD_TEST_DMAR_4B ("DMAR 4B memcpy test")
> +#define VTD_TEST_IR_MSI ("IR MSI")
>  
>  void vtd_test_dmar(struct pci_edu_dev *dev)
>  {
> @@ -50,6 +52,43 @@ void vtd_test_dmar(struct pci_edu_dev *dev)
>  	free_page(page);
>  }
>  
> +static volatile bool edu_intr_recved;
> +
> +static void edu_isr(isr_regs_t *regs)
> +{
> +	edu_intr_recved = true;
> +	eoi();
> +}
> +
> +static void vtd_test_ir(struct pci_edu_dev *dev)
> +{
> +#define VTD_TEST_VECTOR (0xee)
> +	/*
> +	 * Setup EDU PCI device MSI, using interrupt remapping. By
> +	 * default, EDU device is using INTx.
> +	 */
> +	if (!vtd_setup_msi(&dev->pci_dev, VTD_TEST_VECTOR, 0)) {
> +		printf("edu device does not support MSI, skip test\n");
> +		report_skip(VTD_TEST_IR_MSI);
> +		return;
> +	}
> +
> +	handle_irq(VTD_TEST_VECTOR, edu_isr);
> +	irq_enable();
> +
> +	/* Manually trigger INTR */
> +	edu_reg_writel(dev, EDU_REG_INTR_RAISE, 1);
> +
> +	while (!edu_intr_recved)
> +		cpu_relax();
> +
> +	/* Clear INTR bits */
> +	edu_reg_writel(dev, EDU_REG_INTR_RAISE, 0);
> +
> +	/* We are good as long as we reach here */
> +	report(VTD_TEST_IR_MSI, true);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct pci_edu_dev dev;
> @@ -70,8 +109,11 @@ int main(int argc, char *argv[])
>  		printf("Please specify \"-device edu\" to do "
>  		       "further IOMMU tests.\n");
>  		report_skip(VTD_TEST_DMAR_4B);
> -	} else
> +		report_skip(VTD_TEST_IR_MSI);
> +	} else {
>  		vtd_test_dmar(&dev);
> +		vtd_test_ir(&dev);
> +	}
>  
>  	return report_summary();
>  }
> -- 
> 2.7.4
>

I agree with the style/approach, but didn't open a spec.

drew