Re: [Xen-devel] [PATCH V2 11/25] x86/hvm: Introduce a emulated VTD for HVM

2017-08-24 Thread Roger Pau Monné 
On Thu, Aug 24, 2017 at 04:54:25PM +0800, Lan Tianyu wrote:
> On 2017年08月24日 16:49, Roger Pau Monné wrote:
> > On Thu, Aug 24, 2017 at 10:16:32AM +0800, Lan Tianyu wrote:
> >> On 2017年08月23日 15:58, Roger Pau Monné wrote:
> >>> On Wed, Aug 09, 2017 at 04:34:12PM -0400, Lan Tianyu wrote:
>  From: Chao Gao 
>  +}
>  +
>  +#define vvtd_get_reg_quad(vvtd, reg, val) do {  \
>  +(val) = vvtd_get_reg(vvtd, (reg) + 4 ); \
>  +(val) = (val) << 32;\
>  +(val) += vvtd_get_reg(vvtd, reg);   \
>  +} while(0)
>  +#define vvtd_set_reg_quad(vvtd, reg, val) do {  \
>  +vvtd_set_reg(vvtd, reg, (val)); \
>  +vvtd_set_reg(vvtd, (reg) + 4, (val) >> 32); \
>  +} while(0)
> >>>
> >>> You seem to need to access hvm_hw_vvtd_regs using different sizes, why
> >>> not do:
> >>>
> >>> union hvm_hw_vvtd_regs {
> >>> uint8_t  data8[1024];
> >>> uint16_t data16[512];
> >>> uint32_t data32[256];
> >>> uint64_t data64[128];
> >>> };
> >>>
> >>> Then the access is much more straightforward and you don't need the
> >>> complicated helpers that you have above.
> >>
> >> Yes, that will be simpler.
> > 
> > Keep in mind (as said in another patch) that this approach will only
> > work correctly as long as you force accesses to be size aligned, which
> > you where not doing now.
> > 
> > I've looked at the VT-d spec, but I cannot find any section that
> > explains the restrictions on access sizes and alignments.
> > 
> 
> 10.2 Software Access to Registers?

Oh, thanks. I was grepping for "access sizes".

So yes only size aligned accesses are allowed. You should fix the
patch where you check the access size/alignment so it checks for (addr
& (len - 1)) (IIRC you where checking addr & 3).

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2 11/25] x86/hvm: Introduce a emulated VTD for HVM

2017-08-24 Thread Lan Tianyu 
On 2017年08月24日 16:49, Roger Pau Monné wrote:
> On Thu, Aug 24, 2017 at 10:16:32AM +0800, Lan Tianyu wrote:
>> On 2017年08月23日 15:58, Roger Pau Monné wrote:
>>> On Wed, Aug 09, 2017 at 04:34:12PM -0400, Lan Tianyu wrote:
 From: Chao Gao 
 +}
 +
 +#define vvtd_get_reg_quad(vvtd, reg, val) do {  \
 +(val) = vvtd_get_reg(vvtd, (reg) + 4 ); \
 +(val) = (val) << 32;\
 +(val) += vvtd_get_reg(vvtd, reg);   \
 +} while(0)
 +#define vvtd_set_reg_quad(vvtd, reg, val) do {  \
 +vvtd_set_reg(vvtd, reg, (val)); \
 +vvtd_set_reg(vvtd, (reg) + 4, (val) >> 32); \
 +} while(0)
>>>
>>> You seem to need to access hvm_hw_vvtd_regs using different sizes, why
>>> not do:
>>>
>>> union hvm_hw_vvtd_regs {
>>> uint8_t  data8[1024];
>>> uint16_t data16[512];
>>> uint32_t data32[256];
>>> uint64_t data64[128];
>>> };
>>>
>>> Then the access is much more straightforward and you don't need the
>>> complicated helpers that you have above.
>>
>> Yes, that will be simpler.
> 
> Keep in mind (as said in another patch) that this approach will only
> work correctly as long as you force accesses to be size aligned, which
> you where not doing now.
> 
> I've looked at the VT-d spec, but I cannot find any section that
> explains the restrictions on access sizes and alignments.
> 

10.2 Software Access to Registers?

-- 
Best regards
Tianyu Lan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2 11/25] x86/hvm: Introduce a emulated VTD for HVM

2017-08-24 Thread Roger Pau Monné 
On Thu, Aug 24, 2017 at 10:16:32AM +0800, Lan Tianyu wrote:
> On 2017年08月23日 15:58, Roger Pau Monné wrote:
> > On Wed, Aug 09, 2017 at 04:34:12PM -0400, Lan Tianyu wrote:
> >> From: Chao Gao 
> >> +}
> >> +
> >> +#define vvtd_get_reg_quad(vvtd, reg, val) do {  \
> >> +(val) = vvtd_get_reg(vvtd, (reg) + 4 ); \
> >> +(val) = (val) << 32;\
> >> +(val) += vvtd_get_reg(vvtd, reg);   \
> >> +} while(0)
> >> +#define vvtd_set_reg_quad(vvtd, reg, val) do {  \
> >> +vvtd_set_reg(vvtd, reg, (val)); \
> >> +vvtd_set_reg(vvtd, (reg) + 4, (val) >> 32); \
> >> +} while(0)
> > 
> > You seem to need to access hvm_hw_vvtd_regs using different sizes, why
> > not do:
> > 
> > union hvm_hw_vvtd_regs {
> > uint8_t  data8[1024];
> > uint16_t data16[512];
> > uint32_t data32[256];
> > uint64_t data64[128];
> > };
> > 
> > Then the access is much more straightforward and you don't need the
> > complicated helpers that you have above.
> 
> Yes, that will be simpler.

Keep in mind (as said in another patch) that this approach will only
work correctly as long as you force accesses to be size aligned, which
you where not doing now.

I've looked at the VT-d spec, but I cannot find any section that
explains the restrictions on access sizes and alignments.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V2 11/25] x86/hvm: Introduce a emulated VTD for HVM

2017-08-23 Thread Lan Tianyu 
On 2017年08月23日 15:58, Roger Pau Monné wrote:
> On Wed, Aug 09, 2017 at 04:34:12PM -0400, Lan Tianyu wrote:
>> From: Chao Gao 
>>
>> This patch adds create/destroy/query function for the emulated VTD
>> and adapts it to the common VIOMMU abstraction.
>>
>> Signed-off-by: Chao Gao 
>> Signed-off-by: Lan Tianyu 
>> ---
>>  xen/drivers/passthrough/vtd/Makefile |   7 +-
>>  xen/drivers/passthrough/vtd/iommu.h  |  99 +-
>>  xen/drivers/passthrough/vtd/vvtd.c   | 158 
>> +++
>>  xen/include/asm-x86/viommu.h |   3 +
>>  4 files changed, 241 insertions(+), 26 deletions(-)
>>  create mode 100644 xen/drivers/passthrough/vtd/vvtd.c
>>
>> diff --git a/xen/drivers/passthrough/vtd/Makefile 
>> b/xen/drivers/passthrough/vtd/Makefile
>> index f302653..163c7fe 100644
>> --- a/xen/drivers/passthrough/vtd/Makefile
>> +++ b/xen/drivers/passthrough/vtd/Makefile
>> @@ -1,8 +1,9 @@
>>  subdir-$(CONFIG_X86) += x86
>>  
>> -obj-y += iommu.o
>>  obj-y += dmar.o
>> -obj-y += utils.o
>> -obj-y += qinval.o
>>  obj-y += intremap.o
>> +obj-y += iommu.o
>> +obj-y += qinval.o
>>  obj-y += quirks.o
>> +obj-y += utils.o
>> +obj-$(CONFIG_VIOMMU) += vvtd.o
>> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
>> b/xen/drivers/passthrough/vtd/iommu.h
>> index 72c1a2e..55f3b6e 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.h
>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>> @@ -23,31 +23,54 @@
>>  #include 
>>  
>>  /*
>> - * Intel IOMMU register specification per version 1.0 public spec.
>> + * Intel IOMMU register specification per version 2.4 public spec.
>>   */
>>  
>> -#defineDMAR_VER_REG0x0/* Arch version supported by this IOMMU */
>> -#defineDMAR_CAP_REG0x8/* Hardware supported capabilities */
>> -#defineDMAR_ECAP_REG0x10/* Extended capabilities supported */
>> -#defineDMAR_GCMD_REG0x18/* Global command register */
>> -#defineDMAR_GSTS_REG0x1c/* Global status register */
>> -#defineDMAR_RTADDR_REG0x20/* Root entry table */
>> -#defineDMAR_CCMD_REG0x28/* Context command reg */
>> -#defineDMAR_FSTS_REG0x34/* Fault Status register */
>> -#defineDMAR_FECTL_REG0x38/* Fault control register */
>> -#defineDMAR_FEDATA_REG0x3c/* Fault event interrupt data 
>> register */
>> -#defineDMAR_FEADDR_REG0x40/* Fault event interrupt addr 
>> register */
>> -#defineDMAR_FEUADDR_REG 0x44/* Upper address register */
>> -#defineDMAR_AFLOG_REG0x58/* Advanced Fault control */
>> -#defineDMAR_PMEN_REG0x64/* Enable Protected Memory Region */
>> -#defineDMAR_PLMBASE_REG 0x68/* PMRR Low addr */
>> -#defineDMAR_PLMLIMIT_REG 0x6c/* PMRR low limit */
>> -#defineDMAR_PHMBASE_REG 0x70/* pmrr high base addr */
>> -#defineDMAR_PHMLIMIT_REG 0x78/* pmrr high limit */
>> -#defineDMAR_IQH_REG0x80/* invalidation queue head */
>> -#defineDMAR_IQT_REG0x88/* invalidation queue tail */
>> -#defineDMAR_IQA_REG0x90/* invalidation queue addr */
>> -#defineDMAR_IRTA_REG   0xB8/* intr remap */
>> +#define DMAR_VER_REG0x0  /* Arch version supported by this 
>> IOMMU */
>> +#define DMAR_CAP_REG0x8  /* Hardware supported capabilities */
>> +#define DMAR_ECAP_REG   0x10 /* Extended capabilities supported */
>> +#define DMAR_GCMD_REG   0x18 /* Global command register */
>> +#define DMAR_GSTS_REG   0x1c /* Global status register */
>> +#define DMAR_RTADDR_REG 0x20 /* Root entry table */
>> +#define DMAR_CCMD_REG   0x28 /* Context command reg */
>> +#define DMAR_FSTS_REG   0x34 /* Fault Status register */
>> +#define DMAR_FECTL_REG  0x38 /* Fault control register */
>> +#define DMAR_FEDATA_REG 0x3c /* Fault event interrupt data register 
>> */
>> +#define DMAR_FEADDR_REG 0x40 /* Fault event interrupt addr register 
>> */
>> +#define DMAR_FEUADDR_REG0x44 /* Upper address register */
>> +#define DMAR_AFLOG_REG  0x58 /* Advanced Fault control */
>> +#define DMAR_PMEN_REG   0x64 /* Enable Protected Memory Region */
>> +#define DMAR_PLMBASE_REG0x68 /* PMRR Low addr */
>> +#define DMAR_PLMLIMIT_REG   0x6c /* PMRR low limit */
>> +#define DMAR_PHMBASE_REG0x70 /* pmrr high base addr */
>> +#define DMAR_PHMLIMIT_REG   0x78 /* pmrr high limit */
>> +#define DMAR_IQH_REG0x80 /* invalidation queue head */
>> +#define DMAR_IQT_REG0x88 /* invalidation queue tail */
>> +#define DMAR_IQT_REG_HI 0x8c
>> +#define DMAR_IQA_REG0x90 /* invalidation queue addr */
>> +#define DMAR_IQA_REG_HI 0x94
>> +#define DMAR_ICS_REG0x9c /* Invalidation complete status */
>> +#define DMAR_IECTL_REG  0xa0 /* Invalidation event control */
>> +#define

Re: [Xen-devel] [PATCH V2 11/25] x86/hvm: Introduce a emulated VTD for HVM

2017-08-23 Thread Roger Pau Monné 
On Wed, Aug 09, 2017 at 04:34:12PM -0400, Lan Tianyu wrote:
> From: Chao Gao 
> 
> This patch adds create/destroy/query function for the emulated VTD
> and adapts it to the common VIOMMU abstraction.
> 
> Signed-off-by: Chao Gao 
> Signed-off-by: Lan Tianyu 
> ---
>  xen/drivers/passthrough/vtd/Makefile |   7 +-
>  xen/drivers/passthrough/vtd/iommu.h  |  99 +-
>  xen/drivers/passthrough/vtd/vvtd.c   | 158 
> +++
>  xen/include/asm-x86/viommu.h |   3 +
>  4 files changed, 241 insertions(+), 26 deletions(-)
>  create mode 100644 xen/drivers/passthrough/vtd/vvtd.c
> 
> diff --git a/xen/drivers/passthrough/vtd/Makefile 
> b/xen/drivers/passthrough/vtd/Makefile
> index f302653..163c7fe 100644
> --- a/xen/drivers/passthrough/vtd/Makefile
> +++ b/xen/drivers/passthrough/vtd/Makefile
> @@ -1,8 +1,9 @@
>  subdir-$(CONFIG_X86) += x86
>  
> -obj-y += iommu.o
>  obj-y += dmar.o
> -obj-y += utils.o
> -obj-y += qinval.o
>  obj-y += intremap.o
> +obj-y += iommu.o
> +obj-y += qinval.o
>  obj-y += quirks.o
> +obj-y += utils.o
> +obj-$(CONFIG_VIOMMU) += vvtd.o
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 72c1a2e..55f3b6e 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -23,31 +23,54 @@
>  #include 
>  
>  /*
> - * Intel IOMMU register specification per version 1.0 public spec.
> + * Intel IOMMU register specification per version 2.4 public spec.
>   */
>  
> -#defineDMAR_VER_REG0x0/* Arch version supported by this IOMMU */
> -#defineDMAR_CAP_REG0x8/* Hardware supported capabilities */
> -#defineDMAR_ECAP_REG0x10/* Extended capabilities supported */
> -#defineDMAR_GCMD_REG0x18/* Global command register */
> -#defineDMAR_GSTS_REG0x1c/* Global status register */
> -#defineDMAR_RTADDR_REG0x20/* Root entry table */
> -#defineDMAR_CCMD_REG0x28/* Context command reg */
> -#defineDMAR_FSTS_REG0x34/* Fault Status register */
> -#defineDMAR_FECTL_REG0x38/* Fault control register */
> -#defineDMAR_FEDATA_REG0x3c/* Fault event interrupt data register 
> */
> -#defineDMAR_FEADDR_REG0x40/* Fault event interrupt addr register 
> */
> -#defineDMAR_FEUADDR_REG 0x44/* Upper address register */
> -#defineDMAR_AFLOG_REG0x58/* Advanced Fault control */
> -#defineDMAR_PMEN_REG0x64/* Enable Protected Memory Region */
> -#defineDMAR_PLMBASE_REG 0x68/* PMRR Low addr */
> -#defineDMAR_PLMLIMIT_REG 0x6c/* PMRR low limit */
> -#defineDMAR_PHMBASE_REG 0x70/* pmrr high base addr */
> -#defineDMAR_PHMLIMIT_REG 0x78/* pmrr high limit */
> -#defineDMAR_IQH_REG0x80/* invalidation queue head */
> -#defineDMAR_IQT_REG0x88/* invalidation queue tail */
> -#defineDMAR_IQA_REG0x90/* invalidation queue addr */
> -#defineDMAR_IRTA_REG   0xB8/* intr remap */
> +#define DMAR_VER_REG0x0  /* Arch version supported by this IOMMU 
> */
> +#define DMAR_CAP_REG0x8  /* Hardware supported capabilities */
> +#define DMAR_ECAP_REG   0x10 /* Extended capabilities supported */
> +#define DMAR_GCMD_REG   0x18 /* Global command register */
> +#define DMAR_GSTS_REG   0x1c /* Global status register */
> +#define DMAR_RTADDR_REG 0x20 /* Root entry table */
> +#define DMAR_CCMD_REG   0x28 /* Context command reg */
> +#define DMAR_FSTS_REG   0x34 /* Fault Status register */
> +#define DMAR_FECTL_REG  0x38 /* Fault control register */
> +#define DMAR_FEDATA_REG 0x3c /* Fault event interrupt data register 
> */
> +#define DMAR_FEADDR_REG 0x40 /* Fault event interrupt addr register 
> */
> +#define DMAR_FEUADDR_REG0x44 /* Upper address register */
> +#define DMAR_AFLOG_REG  0x58 /* Advanced Fault control */
> +#define DMAR_PMEN_REG   0x64 /* Enable Protected Memory Region */
> +#define DMAR_PLMBASE_REG0x68 /* PMRR Low addr */
> +#define DMAR_PLMLIMIT_REG   0x6c /* PMRR low limit */
> +#define DMAR_PHMBASE_REG0x70 /* pmrr high base addr */
> +#define DMAR_PHMLIMIT_REG   0x78 /* pmrr high limit */
> +#define DMAR_IQH_REG0x80 /* invalidation queue head */
> +#define DMAR_IQT_REG0x88 /* invalidation queue tail */
> +#define DMAR_IQT_REG_HI 0x8c
> +#define DMAR_IQA_REG0x90 /* invalidation queue addr */
> +#define DMAR_IQA_REG_HI 0x94
> +#define DMAR_ICS_REG0x9c /* Invalidation complete status */
> +#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

[Xen-devel] [PATCH V2 11/25] x86/hvm: Introduce a emulated VTD for HVM

2017-08-09 Thread Lan Tianyu 
From: Chao Gao 

This patch adds create/destroy/query function for the emulated VTD
and adapts it to the common VIOMMU abstraction.

Signed-off-by: Chao Gao 
Signed-off-by: Lan Tianyu 
---
 xen/drivers/passthrough/vtd/Makefile |   7 +-
 xen/drivers/passthrough/vtd/iommu.h  |  99 +-
 xen/drivers/passthrough/vtd/vvtd.c   | 158 +++
 xen/include/asm-x86/viommu.h |   3 +
 4 files changed, 241 insertions(+), 26 deletions(-)
 create mode 100644 xen/drivers/passthrough/vtd/vvtd.c

diff --git a/xen/drivers/passthrough/vtd/Makefile 
b/xen/drivers/passthrough/vtd/Makefile
index f302653..163c7fe 100644
--- a/xen/drivers/passthrough/vtd/Makefile
+++ b/xen/drivers/passthrough/vtd/Makefile
@@ -1,8 +1,9 @@
 subdir-$(CONFIG_X86) += x86
 
-obj-y += iommu.o
 obj-y += dmar.o
-obj-y += utils.o
-obj-y += qinval.o
 obj-y += intremap.o
+obj-y += iommu.o
+obj-y += qinval.o
 obj-y += quirks.o
+obj-y += utils.o
+obj-$(CONFIG_VIOMMU) += vvtd.o
diff --git a/xen/drivers/passthrough/vtd/iommu.h 
b/xen/drivers/passthrough/vtd/iommu.h
index 72c1a2e..55f3b6e 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -23,31 +23,54 @@
 #include 
 
 /*
- * Intel IOMMU register specification per version 1.0 public spec.
+ * Intel IOMMU register specification per version 2.4 public spec.
  */
 
-#defineDMAR_VER_REG0x0/* Arch version supported by this IOMMU */
-#defineDMAR_CAP_REG0x8/* Hardware supported capabilities */
-#defineDMAR_ECAP_REG0x10/* Extended capabilities supported */
-#defineDMAR_GCMD_REG0x18/* Global command register */
-#defineDMAR_GSTS_REG0x1c/* Global status register */
-#defineDMAR_RTADDR_REG0x20/* Root entry table */
-#defineDMAR_CCMD_REG0x28/* Context command reg */
-#defineDMAR_FSTS_REG0x34/* Fault Status register */
-#defineDMAR_FECTL_REG0x38/* Fault control register */
-#defineDMAR_FEDATA_REG0x3c/* Fault event interrupt data register */
-#defineDMAR_FEADDR_REG0x40/* Fault event interrupt addr register */
-#defineDMAR_FEUADDR_REG 0x44/* Upper address register */
-#defineDMAR_AFLOG_REG0x58/* Advanced Fault control */
-#defineDMAR_PMEN_REG0x64/* Enable Protected Memory Region */
-#defineDMAR_PLMBASE_REG 0x68/* PMRR Low addr */
-#defineDMAR_PLMLIMIT_REG 0x6c/* PMRR low limit */
-#defineDMAR_PHMBASE_REG 0x70/* pmrr high base addr */
-#defineDMAR_PHMLIMIT_REG 0x78/* pmrr high limit */
-#defineDMAR_IQH_REG0x80/* invalidation queue head */
-#defineDMAR_IQT_REG0x88/* invalidation queue tail */
-#defineDMAR_IQA_REG0x90/* invalidation queue addr */
-#defineDMAR_IRTA_REG   0xB8/* intr remap */
+#define DMAR_VER_REG0x0  /* Arch version supported by this IOMMU */
+#define DMAR_CAP_REG0x8  /* Hardware supported capabilities */
+#define DMAR_ECAP_REG   0x10 /* Extended capabilities supported */
+#define DMAR_GCMD_REG   0x18 /* Global command register */
+#define DMAR_GSTS_REG   0x1c /* Global status register */
+#define DMAR_RTADDR_REG 0x20 /* Root entry table */
+#define DMAR_CCMD_REG   0x28 /* Context command reg */
+#define DMAR_FSTS_REG   0x34 /* Fault Status register */
+#define DMAR_FECTL_REG  0x38 /* Fault control register */
+#define DMAR_FEDATA_REG 0x3c /* Fault event interrupt data register */
+#define DMAR_FEADDR_REG 0x40 /* Fault event interrupt addr register */
+#define DMAR_FEUADDR_REG0x44 /* Upper address register */
+#define DMAR_AFLOG_REG  0x58 /* Advanced Fault control */
+#define DMAR_PMEN_REG   0x64 /* Enable Protected Memory Region */
+#define DMAR_PLMBASE_REG0x68 /* PMRR Low addr */
+#define DMAR_PLMLIMIT_REG   0x6c /* PMRR low limit */
+#define DMAR_PHMBASE_REG0x70 /* pmrr high base addr */
+#define DMAR_PHMLIMIT_REG   0x78 /* pmrr high limit */
+#define DMAR_IQH_REG0x80 /* invalidation queue head */
+#define DMAR_IQT_REG0x88 /* invalidation queue tail */
+#define DMAR_IQT_REG_HI 0x8c
+#define DMAR_IQA_REG0x90 /* invalidation queue addr */
+#define DMAR_IQA_REG_HI 0x94
+#define DMAR_ICS_REG0x9c /* Invalidation complete status */
+#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_REG0xac /* Invalidation event address */
+#define DMAR_IRTA_REG   0xb8 /* Interrupt remapping table addr */
+#define DMAR_IRTA_REG_HI0xbc
+#define DMAR_PQH_REG0xc0 /* Page request queue head */
+#define DMAR_PQH_REG_HI 0xc4