[PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Tiejun Chen]

clflush is a light weight but not serializing instruction, so
hence memory fence is necessary to make sure all load/store
visible before flush cache line.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/x86/vtd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index 109234e..fd2ff04 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
 
 void cacheline_flush(char * addr)
 {
+    mb();
     clflush(addr);
+    mb();
 }
 
 void flush_all_cache()
-- 
1.9.1

[PATCH 2/3] xen/vt-d: mask interrupt message generation

[Tiejun Chen]

While initializing VT-D we should mask interrupt message generation
to avoid receiving any interrupt as pending before enable DMA
translation, and also mask that before disable DMA engine.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index c7bda73..72cd854 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1000,15 +1000,21 @@ static void dma_msi_unmask(struct irq_desc *desc)
     iommu->msi.msi_attrib.masked = 0;
 }
 
-static void dma_msi_mask(struct irq_desc *desc)
+static void mask_dma_interrupt(struct iommu *iommu)
 {
     unsigned long flags;
-    struct iommu *iommu = desc->action->dev_id;
 
-    /* mask it */
     spin_lock_irqsave(&iommu->register_lock, flags);
     dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
+}
+
+static void dma_msi_mask(struct irq_desc *desc)
+{
+    struct iommu *iommu = desc->action->dev_id;
+
+    /* mask it */
+    mask_dma_interrupt(iommu);
     iommu->msi.msi_attrib.masked = 1;
 }
 
@@ -1997,7 +2003,6 @@ static int init_vtd_hw(void)
     struct iommu *iommu;
     struct iommu_flush *flush = NULL;
     int ret;
-    unsigned long flags;
 
     /*
      * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults.  
@@ -2008,11 +2013,16 @@ static int init_vtd_hw(void)
 
         iommu = drhd->iommu;
 
-        clear_fault_bits(iommu);
+        /*
+         * We shouldn't receive any VT-d interrupt while initializing
+         * VT-d so just mask interrupt message generation.
+         */
+        mask_dma_interrupt(iommu);
 
-        spin_lock_irqsave(&iommu->register_lock, flags);
-        dmar_writel(iommu->reg, DMAR_FECTL_REG, 0);
-        spin_unlock_irqrestore(&iommu->register_lock, flags);
+        /*
+         * And then clear all previous faults.
+         */
+        clear_fault_bits(iommu);
     }
 
     /*
@@ -2350,6 +2360,11 @@ static void vtd_suspend(void)
         if ( force_iommu )
             continue;
 
+        /*
+         * Mask interrupt message generation.
+         */
+        mask_dma_interrupt(iommu);
+
         iommu_disable_translation(iommu);
 
         /* If interrupt remapping is enabled, queued invalidation
-- 
1.9.1

[PATCH 3/3] xen/iommu: disable IOMMU engine completely before enter S5

[Tiejun Chen]

S5 menas we also shut down IOMMU engine as well, so we should
disable IOMMU engine completely before enter S5, otherwise we
still probably receive some interrupts in this period. But
currently VT-D path depends on force_iommu and iommu_intremap
to determine if we should disable translation and queued
invalidation. So this unexpected behaviour can be fixed by
clearing force_iommu and iommu_intremap. Additionally, some
cleanups to code comments.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/arch/x86/acpi/power.c           |  2 ++
 xen/arch/x86/crash.c                | 12 ++++++++----
 xen/drivers/passthrough/vtd/iommu.c |  5 +++--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index f41f0de..345470d 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -183,6 +183,8 @@ static int enter_state(u32 state)
         break;
     case ACPI_STATE_S5:
         acpi_enter_sleep_state(ACPI_STATE_S5);
+        /* Make sure we really disable VT-d engine completely. */
+        force_iommu = iommu_intremap = 0;
         break;
     default:
         error = -EINVAL;
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index eb7be9c..123fb12 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -171,15 +171,19 @@ static void nmi_shootdown_cpus(void)
         printk("Failed to shoot down CPUs {%s}\n", keyhandler_scratch);
     }
 
-    /* Crash shutdown any IOMMU functionality as the crashdump kernel is not
-     * happy when booting if interrupt/dma remapping is still enabled */
+    /*
+     * Crash shutdown any IOMMU functionality as the crashdump kernel is not
+     * happy when booting if interrupt/dma remapping is still enabled.
+     */
     iommu_crash_shutdown();
 
     __stop_this_cpu();
 
-    /* This is a bit of a hack due to the problems with the x2apic_enabled
+    /*
+     * This is a bit of a hack due to the problems with the x2apic_enabled
      * variable, but we can't do any better without a significant refactoring
-     * of the APIC code */
+     * of the APIC code.
+     */
     x2apic_enabled = (current_local_apic_mode() == APIC_MODE_X2APIC);
 
     disable_IO_APIC();
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 72cd854..28244ec 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2367,9 +2367,10 @@ static void vtd_suspend(void)
 
         iommu_disable_translation(iommu);
 
-        /* If interrupt remapping is enabled, queued invalidation
+        /*
+         * If interrupt remapping is enabled, queued invalidation
          * will be disabled following interupt remapping disabling
-         * in local apic suspend
+         * in local apic suspend.
          */
         if ( !iommu_intremap && iommu_qinval )
             disable_qinval(iommu);
-- 
1.9.1

Re: [PATCH 2/3] xen/vt-d: mask interrupt message generation

[Zhang, Yang Z]

Chen, Tiejun wrote on 2015-05-04:
> While initializing VT-D we should mask interrupt message generation
> to avoid receiving any interrupt as pending before enable DMA
> translation, and also mask that before disable DMA engine.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/drivers/passthrough/vtd/iommu.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c index c7bda73..72cd854 100644 ---
> a/xen/drivers/passthrough/vtd/iommu.c +++
> b/xen/drivers/passthrough/vtd/iommu.c @@ -1000,15 +1000,21 @@ static
> void dma_msi_unmask(struct irq_desc *desc)
>      iommu->msi.msi_attrib.masked = 0;
>  }
> -static void dma_msi_mask(struct irq_desc *desc)
> +static void mask_dma_interrupt(struct iommu *iommu)
>  {
>      unsigned long flags;
> -    struct iommu *iommu = desc->action->dev_id;
> 
> -    /* mask it */
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
>      spin_unlock_irqrestore(&iommu->register_lock, flags);
> +}
> +
> +static void dma_msi_mask(struct irq_desc *desc)
> +{
> +    struct iommu *iommu = desc->action->dev_id;
> +
> +    /* mask it */
> +    mask_dma_interrupt(iommu);
>      iommu->msi.msi_attrib.masked = 1;
>  }
> @@ -1997,7 +2003,6 @@ static int init_vtd_hw(void)
>      struct iommu *iommu;
>      struct iommu_flush *flush = NULL;
>      int ret;
> -    unsigned long flags;
> 
>      /*
>       * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults.
> @@ -2008,11 +2013,16 @@ static int init_vtd_hw(void)
> 
>          iommu = drhd->iommu;
> -        clear_fault_bits(iommu);
> +        /*
> +         * We shouldn't receive any VT-d interrupt while initializing
> +         * VT-d so just mask interrupt message generation.
> +         */
> +        mask_dma_interrupt(iommu);
> 
> -        spin_lock_irqsave(&iommu->register_lock, flags);
> -        dmar_writel(iommu->reg, DMAR_FECTL_REG, 0);
> -        spin_unlock_irqrestore(&iommu->register_lock, flags);
> +        /*
> +         * And then clear all previous faults.
> +         */
> +        clear_fault_bits(iommu);
>      }
>      
>      /*
> @@ -2350,6 +2360,11 @@ static void vtd_suspend(void)
>          if ( force_iommu )
>              continue;
> +        /*
> +         * Mask interrupt message generation.
> +         */
> +        mask_dma_interrupt(iommu);
> +
>          iommu_disable_translation(iommu);
>          
>          /* If interrupt remapping is enabled, queued invalidation

Just curious that do you observe a real issue or just catch it through reading code?

Best regards,
Yang

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Zhang, Yang Z]

Chen, Tiejun wrote on 2015-05-04:
> clflush is a light weight but not serializing instruction, so hence
> memory fence is necessary to make sure all load/store visible before flush cache line.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

> ---
>  xen/drivers/passthrough/vtd/x86/vtd.c | 2 ++
>  1 file changed, 2 insertions(+)
> diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
> b/xen/drivers/passthrough/vtd/x86/vtd.c index 109234e..fd2ff04 100644
> --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++
> b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -56,7 +56,9 @@ unsigned int
> get_cache_line_size(void)
> 
>  void cacheline_flush(char * addr)
>  {
> +    mb();
>      clflush(addr);
> +    mb();
>  }
>  
>  void flush_all_cache()


Best regards,
Yang

Re: [PATCH 2/3] xen/vt-d: mask interrupt message generation

[Chen, Tiejun]

Yang,

Thanks for your review.

On 2015/5/4 12:07, Zhang, Yang Z wrote:
> Chen, Tiejun wrote on 2015-05-04:
>> While initializing VT-D we should mask interrupt message generation
>> to avoid receiving any interrupt as pending before enable DMA
>> translation, and also mask that before disable DMA engine.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   xen/drivers/passthrough/vtd/iommu.c | 31 +++++++++++++++++++++++--------
>>   1 file changed, 23 insertions(+), 8 deletions(-)
>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>> b/xen/drivers/passthrough/vtd/iommu.c index c7bda73..72cd854 100644 ---
>> a/xen/drivers/passthrough/vtd/iommu.c +++
>> b/xen/drivers/passthrough/vtd/iommu.c @@ -1000,15 +1000,21 @@ static
>> void dma_msi_unmask(struct irq_desc *desc)
>>       iommu->msi.msi_attrib.masked = 0;
>>   }
>> -static void dma_msi_mask(struct irq_desc *desc)
>> +static void mask_dma_interrupt(struct iommu *iommu)
>>   {
>>       unsigned long flags;
>> -    struct iommu *iommu = desc->action->dev_id;
>>
>> -    /* mask it */
>>       spin_lock_irqsave(&iommu->register_lock, flags);
>>       dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
>>       spin_unlock_irqrestore(&iommu->register_lock, flags);
>> +}
>> +
>> +static void dma_msi_mask(struct irq_desc *desc)
>> +{
>> +    struct iommu *iommu = desc->action->dev_id;
>> +
>> +    /* mask it */
>> +    mask_dma_interrupt(iommu);
>>       iommu->msi.msi_attrib.masked = 1;
>>   }
>> @@ -1997,7 +2003,6 @@ static int init_vtd_hw(void)
>>       struct iommu *iommu;
>>       struct iommu_flush *flush = NULL;
>>       int ret;
>> -    unsigned long flags;
>>
>>       /*
>>        * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults.
>> @@ -2008,11 +2013,16 @@ static int init_vtd_hw(void)
>>
>>           iommu = drhd->iommu;
>> -        clear_fault_bits(iommu);
>> +        /*
>> +         * We shouldn't receive any VT-d interrupt while initializing
>> +         * VT-d so just mask interrupt message generation.
>> +         */
>> +        mask_dma_interrupt(iommu);
>>
>> -        spin_lock_irqsave(&iommu->register_lock, flags);
>> -        dmar_writel(iommu->reg, DMAR_FECTL_REG, 0);
>> -        spin_unlock_irqrestore(&iommu->register_lock, flags);
>> +        /*
>> +         * And then clear all previous faults.
>> +         */
>> +        clear_fault_bits(iommu);
>>       }
>>
>>       /*
>> @@ -2350,6 +2360,11 @@ static void vtd_suspend(void)
>>           if ( force_iommu )
>>               continue;
>> +        /*
>> +         * Mask interrupt message generation.
>> +         */
>> +        mask_dma_interrupt(iommu);
>> +
>>           iommu_disable_translation(iommu);
>>
>>           /* If interrupt remapping is enabled, queued invalidation
>
> Just curious that do you observe a real issue or just catch it through reading code?
>

Yes, we observed this problem on BDW. And actually one HP customer also 
had this same issue.

Once we enable IOMMU translation, an IOMMU fault, 'Present bit in root 
entry is clear', is triggered like this,

(XEN) [VT-D]iommu.c:731: iommu_enable_translation: iommu->reg = 
ffff82c000201000
(XEN) [VT-D]iommu.c:875: iommu_fault_status: Fault Overflow
(XEN) [VT-D]iommu.c:877: iommu_fault_status: Primary Pending Fault
(XEN) [VT-D]DMAR:[DMA Write] Request device [0000:00:02.0] fault addr 0, 
iommu reg = ffff82c000201000
(XEN) [VT-D]d32767v0 DMAR: reason 01 - Present bit in root entry is clear
(XEN) print_vtd_entries: iommu ffff83012aa88600 dev 0000:00:02.0 gmfn 0
(XEN)     root_entry = ffff83012aa85000
(XEN)     root_entry[0] = 80f5001
(XEN)     context = ffff8300080f5000
(XEN)     context[10] = 2_8b75001
(XEN)     l4 = ffff830008b75000
(XEN)     l4_index = 0
(XEN)     l4[0] = 8b74003
(XEN)     l3 = ffff830008b74000
(XEN)     l3_index = 0
(XEN)     l3[0] = 8b73003
(XEN)     l2 = ffff830008b73000
(XEN)     l2_index = 0
(XEN)     l2[0] = 8b72003
(XEN)     l1 = ffff830008b72000
(XEN)     l1_index = 0
(XEN)     l1[0] = 3

At first I doubted this is issued by some improper cache behaviors.
Because as you see, "root_entry[0] = 80f5001" indicates we already set 
that present bit. But Caching Mode bit is zero in BDW so this means 
remapping hardware doesn't own contest cache. And I also check xen 
always calls clflush to write back memory on CPU side. (Even I also 
tried to use wbinvd to replace clflush). And plus, you can see this 
guest fault address is a weird zero, so I turned to concern if this is 
just an unexpected fault which is pending because of some BIOS cleanup, 
or undefined hardware behaviors. Here I mean clear_fault_bits() is 
already not enough to clear this kind of pending interrupt before we 
complete all VT-D initializations.

And anyway, I also think we should disable interrupt generation during 
VT-D setup until VT-D device enable interrupt explicitly.

Thanks
Tiejun

Re: [PATCH 2/3] xen/vt-d: mask interrupt message generation

[Zhang, Yang Z]

Chen, Tiejun wrote on 2015-05-04:
> Yang,
> 
> Thanks for your review.
> 
> On 2015/5/4 12:07, Zhang, Yang Z wrote:
>> Chen, Tiejun wrote on 2015-05-04:
>>> While initializing VT-D we should mask interrupt message generation
>>> to avoid receiving any interrupt as pending before enable DMA
>>> translation, and also mask that before disable DMA engine.
>>> 
>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>> ---
>>>   xen/drivers/passthrough/vtd/iommu.c | 31
>>>   +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8
>>>   deletions(-)
>>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>>> b/xen/drivers/passthrough/vtd/iommu.c index c7bda73..72cd854 100644 ---
>>> a/xen/drivers/passthrough/vtd/iommu.c +++
>>> b/xen/drivers/passthrough/vtd/iommu.c @@ -1000,15 +1000,21 @@ static
>>> void dma_msi_unmask(struct irq_desc *desc)
>>>       iommu->msi.msi_attrib.masked = 0;
>>>   }
>>> -static void dma_msi_mask(struct irq_desc *desc)
>>> +static void mask_dma_interrupt(struct iommu *iommu)
>>>   {
>>>       unsigned long flags;
>>> -    struct iommu *iommu = desc->action->dev_id;
>>> 
>>> -    /* mask it */
>>>       spin_lock_irqsave(&iommu->register_lock, flags);
>>>       dmar_writel(iommu->reg, DMAR_FECTL_REG, DMA_FECTL_IM);
>>>       spin_unlock_irqrestore(&iommu->register_lock, flags);
>>> +}
>>> +
>>> +static void dma_msi_mask(struct irq_desc *desc)
>>> +{
>>> +    struct iommu *iommu = desc->action->dev_id;
>>> +
>>> +    /* mask it */
>>> +    mask_dma_interrupt(iommu);
>>>       iommu->msi.msi_attrib.masked = 1;
>>>   }
>>> @@ -1997,7 +2003,6 @@ static int init_vtd_hw(void)
>>>       struct iommu *iommu;
>>>       struct iommu_flush *flush = NULL;
>>>       int ret;
>>> -    unsigned long flags;
>>> 
>>>       /*
>>>        * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults.
>>> @@ -2008,11 +2013,16 @@ static int init_vtd_hw(void)
>>> 
>>>           iommu = drhd->iommu;
>>> -        clear_fault_bits(iommu);
>>> +        /*
>>> +         * We shouldn't receive any VT-d interrupt while initializing
>>> +         * VT-d so just mask interrupt message generation.
>>> +         */
>>> +        mask_dma_interrupt(iommu);
>>> 
>>> -        spin_lock_irqsave(&iommu->register_lock, flags);
>>> -        dmar_writel(iommu->reg, DMAR_FECTL_REG, 0);
>>> -        spin_unlock_irqrestore(&iommu->register_lock, flags);
>>> +        /*
>>> +         * And then clear all previous faults.
>>> +         */
>>> +        clear_fault_bits(iommu);
>>>       }
>>>       
>>>       /*
>>> @@ -2350,6 +2360,11 @@ static void vtd_suspend(void)
>>>           if ( force_iommu )
>>>               continue;
>>> +        /*
>>> +         * Mask interrupt message generation.
>>> +         */
>>> +        mask_dma_interrupt(iommu);
>>> +
>>>           iommu_disable_translation(iommu);
>>>           
>>>           /* If interrupt remapping is enabled, queued invalidation
>> 
>> Just curious that do you observe a real issue or just catch it through
>> reading code?
>> 
> 
> Yes, we observed this problem on BDW. And actually one HP customer also
> had this same issue.
> 
> Once we enable IOMMU translation, an IOMMU fault, 'Present bit in root
> entry is clear', is triggered like this,
> 
> (XEN) [VT-D]iommu.c:731: iommu_enable_translation: iommu->reg =
> ffff82c000201000 (XEN) [VT-D]iommu.c:875: iommu_fault_status: Fault
> Overflow (XEN) [VT-D]iommu.c:877: iommu_fault_status: Primary Pending
> Fault (XEN) [VT-D]DMAR:[DMA Write] Request device [0000:00:02.0] fault
> addr 0, iommu reg = ffff82c000201000 (XEN) [VT-D]d32767v0 DMAR: reason
> 01 - Present bit in root entry is clear (XEN) print_vtd_entries: iommu
> ffff83012aa88600 dev 0000:00:02.0 gmfn 0 (XEN)     root_entry =
> ffff83012aa85000 (XEN)     root_entry[0] = 80f5001 (XEN)     context =
> ffff8300080f5000 (XEN)     context[10] = 2_8b75001 (XEN)     l4 =
> ffff830008b75000 (XEN)     l4_index = 0 (XEN)     l4[0] = 8b74003 (XEN) 
>    l3 = ffff830008b74000 (XEN)     l3_index = 0 (XEN)     l3[0] =
> 8b73003 (XEN)     l2 = ffff830008b73000 (XEN)     l2_index = 0 (XEN)    
> l2[0] = 8b72003 (XEN)     l1 = ffff830008b72000 (XEN)     l1_index = 0
> (XEN)     l1[0] = 3
> 
> At first I doubted this is issued by some improper cache behaviors.
> Because as you see, "root_entry[0] = 80f5001" indicates we already set
> that present bit. But Caching Mode bit is zero in BDW so this means
> remapping hardware doesn't own contest cache. And I also check xen
> always calls clflush to write back memory on CPU side. (Even I also
> tried to use wbinvd to replace clflush). And plus, you can see this
> guest fault address is a weird zero, so I turned to concern if this is
> just an unexpected fault which is pending because of some BIOS cleanup,
> or undefined hardware behaviors. Here I mean clear_fault_bits() is
> already not enough to clear this kind of pending interrupt before we
> complete all VT-D initializations.

So this patch helps nothing in this case?

> 
> And anyway, I also think we should disable interrupt generation during
> VT-D setup until VT-D device enable interrupt explicitly.

The question is why there will be a fault interrupt generated during VT-D setup?

> 
> Thanks
> Tiejun


Best regards,
Yang

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>  
>  void cacheline_flush(char * addr)
>  {
> +    mb();
>      clflush(addr);
> +    mb();
>  }

I think the purpose of the flush is to force write back, not to evict
the cache line, and if so wmb() would appear to be sufficient. As
the SDM says that's not the case, a comment explaining why wmb()
is not sufficient would seem necessary. Plus in the description I
think "serializing" needs to be changed to "fencing", as serialization
is not what we really care about here. If you and the maintainers
agree, I could certainly fix up both aspects while committing.

Jan

Re: [PATCH 2/3] xen/vt-d: mask interrupt message generation

[Jan Beulich]

>>> On 04.05.15 at 07:08, <tiejun.chen@intel.com> wrote:
> At first I doubted this is issued by some improper cache behaviors.
> Because as you see, "root_entry[0] = 80f5001" indicates we already set 
> that present bit. But Caching Mode bit is zero in BDW so this means 
> remapping hardware doesn't own contest cache. And I also check xen 
> always calls clflush to write back memory on CPU side. (Even I also 
> tried to use wbinvd to replace clflush). And plus, you can see this 
> guest fault address is a weird zero, so I turned to concern if this is 
> just an unexpected fault which is pending because of some BIOS cleanup, 
> or undefined hardware behaviors. Here I mean clear_fault_bits() is 
> already not enough to clear this kind of pending interrupt before we 
> complete all VT-D initializations.

This last aspect in particular needs more explanation, but I'm
wary of the change in general (see Yang's response).

Jan

Re: [PATCH 3/3] xen/iommu: disable IOMMU engine completely before enter S5

[Jan Beulich]

>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
> S5 menas we also shut down IOMMU engine as well, so we should
> disable IOMMU engine completely before enter S5, otherwise we
> still probably receive some interrupts in this period. But
> currently VT-D path depends on force_iommu and iommu_intremap
> to determine if we should disable translation and queued
> invalidation. So this unexpected behaviour can be fixed by
> clearing force_iommu and iommu_intremap.

Maybe it can, but is this the right way to do it? Fiddling with IOMMU
internal variables in xen/arch/x86/acpi/power.c seems bogus (not
to say hacky) at least. And why would this be S5-specific?

> Additionally, some cleanups to code comments.

Which has nothing to do in tis patch.

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Andrew Cooper]

On 04/05/2015 09:52, Jan Beulich wrote:
>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>  
>>  void cacheline_flush(char * addr)
>>  {
>> +    mb();
>>      clflush(addr);
>> +    mb();
>>  }
> I think the purpose of the flush is to force write back, not to evict
> the cache line, and if so wmb() would appear to be sufficient. As
> the SDM says that's not the case, a comment explaining why wmb()
> is not sufficient would seem necessary. Plus in the description I
> think "serializing" needs to be changed to "fencing", as serialization
> is not what we really care about here. If you and the maintainers
> agree, I could certainly fix up both aspects while committing.

On the subject of writebacks, we should get around to alternating-up the
use of clflushopt and clwb, either of which would be better than a
clflush in this case (avoiding the need for the leading mfence).

However, the ISA extension document does not indicate which processors
will have support for these new instructions.

~Andrew

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 04.05.15 at 11:14, <andrew.cooper3@citrix.com> wrote:
> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>  
>>>  void cacheline_flush(char * addr)
>>>  {
>>> +    mb();
>>>      clflush(addr);
>>> +    mb();
>>>  }
>> I think the purpose of the flush is to force write back, not to evict
>> the cache line, and if so wmb() would appear to be sufficient. As
>> the SDM says that's not the case, a comment explaining why wmb()
>> is not sufficient would seem necessary. Plus in the description I
>> think "serializing" needs to be changed to "fencing", as serialization
>> is not what we really care about here. If you and the maintainers
>> agree, I could certainly fix up both aspects while committing.
> 
> On the subject of writebacks, we should get around to alternating-up the
> use of clflushopt and clwb, either of which would be better than a
> clflush in this case (avoiding the need for the leading mfence).

Plus the barrier would perhaps rather sit around the loop
invoking cacheline_flush() in __iommu_flush_cache(), and I
wonder whether VT-d code shouldn't use available flushing
code elsewhere in the system, and whether that code then
wouldn't need barriers added (or use clflushopt/clwb as you
suggest) instead.

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Chen, Tiejun]

On 2015/5/4 16:52, Jan Beulich wrote:
>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>
>>   void cacheline_flush(char * addr)
>>   {
>> +    mb();
>>       clflush(addr);
>> +    mb();
>>   }
>
> I think the purpose of the flush is to force write back, not to evict
> the cache line, and if so wmb() would appear to be sufficient. As
> the SDM says that's not the case, a comment explaining why wmb()
> is not sufficient would seem necessary. Plus in the description I

Seems wmb() is not sufficient here.

"CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed 
to be ordered by any other fencing, serializing or other CLFLUSH 
instruction."

Thanks
Tiejun

> think "serializing" needs to be changed to "fencing", as serialization
> is not what we really care about here. If you and the maintainers
> agree, I could certainly fix up both aspects while committing.
>
> Jan
>
>
>

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 04.05.15 at 12:39, <tiejun.chen@intel.com> wrote:
> On 2015/5/4 16:52, Jan Beulich wrote:
>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>
>>>   void cacheline_flush(char * addr)
>>>   {
>>> +    mb();
>>>       clflush(addr);
>>> +    mb();
>>>   }
>>
>> I think the purpose of the flush is to force write back, not to evict
>> the cache line, and if so wmb() would appear to be sufficient. As
>> the SDM says that's not the case, a comment explaining why wmb()
>> is not sufficient would seem necessary. Plus in the description I
> 
> Seems wmb() is not sufficient here.
> 
> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed 
> to be ordered by any other fencing, serializing or other CLFLUSH 
> instruction."

Right - that's what I said in the second sentence.

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, May 04, 2015 6:44 PM
> 
> >>> On 04.05.15 at 12:39, <tiejun.chen@intel.com> wrote:
> > On 2015/5/4 16:52, Jan Beulich wrote:
> >>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
> >>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> >>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> >>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
> >>>
> >>>   void cacheline_flush(char * addr)
> >>>   {
> >>> +    mb();
> >>>       clflush(addr);
> >>> +    mb();
> >>>   }
> >>
> >> I think the purpose of the flush is to force write back, not to evict
> >> the cache line, and if so wmb() would appear to be sufficient. As
> >> the SDM says that's not the case, a comment explaining why wmb()
> >> is not sufficient would seem necessary. Plus in the description I
> >
> > Seems wmb() is not sufficient here.
> >
> > "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed
> > to be ordered by any other fencing, serializing or other CLFLUSH
> > instruction."
> 
> Right - that's what I said in the second sentence.
> 

btw why do we need two fences here? Suppose we just care about
writes before the flush point...

Thanks
Kevin

Re: [PATCH 2/3] xen/vt-d: mask interrupt message generation

[Chen, Tiejun]

On 2015/5/4 16:57, Jan Beulich wrote:
>>>> On 04.05.15 at 07:08, <tiejun.chen@intel.com> wrote:
>> At first I doubted this is issued by some improper cache behaviors.
>> Because as you see, "root_entry[0] = 80f5001" indicates we already set
>> that present bit. But Caching Mode bit is zero in BDW so this means
>> remapping hardware doesn't own contest cache. And I also check xen
>> always calls clflush to write back memory on CPU side. (Even I also
>> tried to use wbinvd to replace clflush). And plus, you can see this
>> guest fault address is a weird zero, so I turned to concern if this is
>> just an unexpected fault which is pending because of some BIOS cleanup,
>> or undefined hardware behaviors. Here I mean clear_fault_bits() is
>> already not enough to clear this kind of pending interrupt before we
>> complete all VT-D initializations.
>
> This last aspect in particular needs more explanation, but I'm
> wary of the change in general (see Yang's response).
>

Sure.

As we discussed offline, Yang though at most this is a way to workaround 
our problem. So I would ping BIOS team to double check if this is an 
issue specific to some given BIOSs or platforms since this doesn't meet 
the current specification

Thanks
Tiejun

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Chen, Tiejun]

On 2015/5/4 18:52, Tian, Kevin wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, May 04, 2015 6:44 PM
>>
>>>>> On 04.05.15 at 12:39, <tiejun.chen@intel.com> wrote:
>>> On 2015/5/4 16:52, Jan Beulich wrote:
>>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>>
>>>>>    void cacheline_flush(char * addr)
>>>>>    {
>>>>> +    mb();
>>>>>        clflush(addr);
>>>>> +    mb();
>>>>>    }
>>>>
>>>> I think the purpose of the flush is to force write back, not to evict
>>>> the cache line, and if so wmb() would appear to be sufficient. As
>>>> the SDM says that's not the case, a comment explaining why wmb()
>>>> is not sufficient would seem necessary. Plus in the description I
>>>
>>> Seems wmb() is not sufficient here.
>>>
>>> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed
>>> to be ordered by any other fencing, serializing or other CLFLUSH
>>> instruction."
>>
>> Right - that's what I said in the second sentence.
>>
>
> btw why do we need two fences here? Suppose we just care about
> writes before the flush point...
>

The first MFENCE guarantees all load/store visible before flush cache 
line. But the second MFENCE just makes sure CLFLUSH is not ordered by 
that ensuing load/store, right?

Thanks
Tiejun

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Konrad Rzeszutek Wilk]

On Mon, May 04, 2015 at 06:39:56PM +0800, Chen, Tiejun wrote:
> On 2015/5/4 16:52, Jan Beulich wrote:
> >>>>On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
> >>--- a/xen/drivers/passthrough/vtd/x86/vtd.c
> >>+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> >>@@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
> >>
> >>  void cacheline_flush(char * addr)
> >>  {
> >>+    mb();
> >>      clflush(addr);
> >>+    mb();
> >>  }
> >
> >I think the purpose of the flush is to force write back, not to evict
> >the cache line, and if so wmb() would appear to be sufficient. As
> >the SDM says that's not the case, a comment explaining why wmb()
> >is not sufficient would seem necessary. Plus in the description I
> 
> Seems wmb() is not sufficient here.
> 
> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed to
> be ordered by any other fencing, serializing or other CLFLUSH instruction."

That is incorrect. We have observed that CLFLUSH instruction do serialize each
other. That is if on a core you send a bunch of CLFLUSH it stalls the pipeline.

Cc-ing Boris who discovered this.
> 
> Thanks
> Tiejun
> 
> >think "serializing" needs to be changed to "fencing", as serialization
> >is not what we really care about here. If you and the maintainers
> >agree, I could certainly fix up both aspects while committing.
> >
> >Jan
> >
> >
> >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Andrew Cooper]

On 04/05/2015 16:22, Konrad Rzeszutek Wilk wrote:
> On Mon, May 04, 2015 at 06:39:56PM +0800, Chen, Tiejun wrote:
>> On 2015/5/4 16:52, Jan Beulich wrote:
>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>
>>>>  void cacheline_flush(char * addr)
>>>>  {
>>>> +    mb();
>>>>      clflush(addr);
>>>> +    mb();
>>>>  }
>>> I think the purpose of the flush is to force write back, not to evict
>>> the cache line, and if so wmb() would appear to be sufficient. As
>>> the SDM says that's not the case, a comment explaining why wmb()
>>> is not sufficient would seem necessary. Plus in the description I
>> Seems wmb() is not sufficient here.
>>
>> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed to
>> be ordered by any other fencing, serializing or other CLFLUSH instruction."
> That is incorrect. We have observed that CLFLUSH instruction do serialize each
> other. That is if on a core you send a bunch of CLFLUSH it stalls the pipeline.
>
> Cc-ing Boris who discovered this.

Simply stalling the pipeline says nothing about its ordering with
respect to other instructions.

It is plausible that certain processor pipelines employ stricter
restrictions on CLFLUSH, but Xen must not assume that this is the case
in general, especially as it is in direct contradiction to both the
Intel and AMD instruction manuals.

~Andrew

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Monday, May 04, 2015 7:26 PM
> 
> On 2015/5/4 18:52, Tian, Kevin wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, May 04, 2015 6:44 PM
> >>
> >>>>> On 04.05.15 at 12:39, <tiejun.chen@intel.com> wrote:
> >>> On 2015/5/4 16:52, Jan Beulich wrote:
> >>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
> >>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> >>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> >>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
> >>>>>
> >>>>>    void cacheline_flush(char * addr)
> >>>>>    {
> >>>>> +    mb();
> >>>>>        clflush(addr);
> >>>>> +    mb();
> >>>>>    }
> >>>>
> >>>> I think the purpose of the flush is to force write back, not to evict
> >>>> the cache line, and if so wmb() would appear to be sufficient. As
> >>>> the SDM says that's not the case, a comment explaining why wmb()
> >>>> is not sufficient would seem necessary. Plus in the description I
> >>>
> >>> Seems wmb() is not sufficient here.
> >>>
> >>> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed
> >>> to be ordered by any other fencing, serializing or other CLFLUSH
> >>> instruction."
> >>
> >> Right - that's what I said in the second sentence.
> >>
> >
> > btw why do we need two fences here? Suppose we just care about
> > writes before the flush point...
> >
> 
> The first MFENCE guarantees all load/store visible before flush cache
> line. But the second MFENCE just makes sure CLFLUSH is not ordered by
> that ensuing load/store, right?
> 

It's not an usual case to have another store to same destination address
right after CLFLUSH. Usually there's some protocol to do another update
until device completes access. Store to different address is not a concern.
But yes if just looking at this interface alone, we'd better not make
assumption about the caller so two mb() look necessary...

btw as Jan commented in another thread, if there's no other caller of
this function, it might be more reasonable to expand it into the bigger
loop in __iommu_flush_cache so you don't need to fence for each
cache line flush.

Thanks
Kevin

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Zhang, Yang Z]

Jan Beulich wrote on 2015-05-04:
>>>> On 04.05.15 at 11:14, <andrew.cooper3@citrix.com> wrote:
>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>> 
>>>>  void cacheline_flush(char * addr)  {
>>>> +    mb();
>>>>      clflush(addr);
>>>> +    mb();
>>>>  }
>>> I think the purpose of the flush is to force write back, not to
>>> evict the cache line, and if so wmb() would appear to be
>>> sufficient. As the SDM says that's not the case, a comment
>>> explaining why wmb() is not sufficient would seem necessary. Plus
>>> in the description I think "serializing" needs to be changed to
>>> "fencing", as serialization is not what we really care about here.
>>> If you and the maintainers agree, I could certainly fix up both aspects while committing.
>> 
>> On the subject of writebacks, we should get around to alternating-up
>> the use of clflushopt and clwb, either of which would be better than
>> a clflush in this case (avoiding the need for the leading mfence).
> 
> Plus the barrier would perhaps rather sit around the loop invoking
> cacheline_flush() in __iommu_flush_cache(), and I wonder whether VT-d

Agree. It's better to batch the flush operations, like:

@@ -167,11 +167,15 @@ static void __iommu_flush_cache(void *addr, unsigned int size)
     if ( !iommus_incoherent )
         return;
 
+    mb();
+
     if ( clflush_size == 0 )
         clflush_size = get_cache_line_size();
 
     for ( i = 0; i < size; i += clflush_size )
         cacheline_flush((char *)addr + i);
+
+    mb();
 }

> code shouldn't use available flushing code elsewhere in the system,
> and whether that code then wouldn't need barriers added (or use
> clflushopt/clwb as you
> suggest) instead.


> 
> Jan


Best regards,
Yang

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Chen, Tiejun]

On 2015/5/4 18:43, Jan Beulich wrote:
>>>> On 04.05.15 at 12:39, <tiejun.chen@intel.com> wrote:
>> On 2015/5/4 16:52, Jan Beulich wrote:
>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>
>>>>    void cacheline_flush(char * addr)
>>>>    {
>>>> +    mb();
>>>>        clflush(addr);
>>>> +    mb();
>>>>    }
>>>
>>> I think the purpose of the flush is to force write back, not to evict
>>> the cache line, and if so wmb() would appear to be sufficient. As
>>> the SDM says that's not the case, a comment explaining why wmb()
>>> is not sufficient would seem necessary. Plus in the description I
>>
>> Seems wmb() is not sufficient here.
>>
>> "CLFLUSH is only ordered by the MFENCE instruction. It is not guaranteed
>> to be ordered by any other fencing, serializing or other CLFLUSH
>> instruction."
>
> Right - that's what I said in the second sentence.
>

Thanks for all guys' comments.

Does this work for everyone?

     xen/vt-d: need barriers to workaround CLFLUSH

     clflush is a light weight but not fencing instruction, so
     hence memory fence is necessary to make sure all load/store
     visible before flush cache line.

     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index c7bda73..1248a17 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -170,8 +170,15 @@ static void __iommu_flush_cache(void *addr, 
unsigned int size)
      if ( clflush_size == 0 )
          clflush_size = get_cache_line_size();

+    /*
+     * CLFLUSH is only ordered by the MFENCE instruction.
+     * It is not guaranteed to be ordered by any other fencing,
+     * serializing or other CLFLUSH instruction.
+     */
+    mb();
      for ( i = 0; i < size; i += clflush_size )
          cacheline_flush((char *)addr + i);
+    mb();
  }

  void iommu_flush_cache_entry(void *addr, unsigned int size)

Thanks
Tiejun

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 05.05.15 at 04:45, <tiejun.chen@intel.com> wrote:
> Does this work for everyone?

Please first of all explain why the interfaces in asm/flushtlb.h can't
be used here (at least when flushing entire pages). Because - as
said before - for a complete fix you'd need to deal with the CLFLUSH
use(s) elsewhere in the system too.

Btw, isn't the !iommus_incoherent check in that function inverted?
I.e. why would we _not_ need to flush caches when IOMMUs
are not coherent (and why would flushing be needed when they're
coherent anyway)?

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Boris Ostrovsky]

On 05/04/2015 05:14 AM, Andrew Cooper wrote:
> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>   
>>>   void cacheline_flush(char * addr)
>>>   {
>>> +    mb();
>>>       clflush(addr);
>>> +    mb();
>>>   }
>> I think the purpose of the flush is to force write back, not to evict
>> the cache line, and if so wmb() would appear to be sufficient. As
>> the SDM says that's not the case, a comment explaining why wmb()
>> is not sufficient would seem necessary. Plus in the description I
>> think "serializing" needs to be changed to "fencing", as serialization
>> is not what we really care about here. If you and the maintainers
>> agree, I could certainly fix up both aspects while committing.
> On the subject of writebacks, we should get around to alternating-up the
> use of clflushopt and clwb, either of which would be better than a
> clflush in this case (avoiding the need for the leading mfence).
>
> However, the ISA extension document does not indicate which processors
> will have support for these new instructions.

https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf

We really should add support for this. On shutting down a very large 
guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU. 
This was due to serializing nature of CLFLUSH.

-boris

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 05.05.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>   
>>>>   void cacheline_flush(char * addr)
>>>>   {
>>>> +    mb();
>>>>       clflush(addr);
>>>> +    mb();
>>>>   }
>>> I think the purpose of the flush is to force write back, not to evict
>>> the cache line, and if so wmb() would appear to be sufficient. As
>>> the SDM says that's not the case, a comment explaining why wmb()
>>> is not sufficient would seem necessary. Plus in the description I
>>> think "serializing" needs to be changed to "fencing", as serialization
>>> is not what we really care about here. If you and the maintainers
>>> agree, I could certainly fix up both aspects while committing.
>> On the subject of writebacks, we should get around to alternating-up the
>> use of clflushopt and clwb, either of which would be better than a
>> clflush in this case (avoiding the need for the leading mfence).
>>
>> However, the ISA extension document does not indicate which processors
>> will have support for these new instructions.
> 
> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf 
> 
> We really should add support for this. On shutting down a very large 
> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU. 
> This was due to serializing nature of CLFLUSH.

But flushing the IOMMU isn't being done via CPU instructions, but
rather via commands sent to the IOMMU. I.e. I'm somewhat
confused by your reply.

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Boris Ostrovsky]

On 05/05/2015 11:58 AM, Jan Beulich wrote:
>>>> On 05.05.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>>    
>>>>>    void cacheline_flush(char * addr)
>>>>>    {
>>>>> +    mb();
>>>>>        clflush(addr);
>>>>> +    mb();
>>>>>    }
>>>> I think the purpose of the flush is to force write back, not to evict
>>>> the cache line, and if so wmb() would appear to be sufficient. As
>>>> the SDM says that's not the case, a comment explaining why wmb()
>>>> is not sufficient would seem necessary. Plus in the description I
>>>> think "serializing" needs to be changed to "fencing", as serialization
>>>> is not what we really care about here. If you and the maintainers
>>>> agree, I could certainly fix up both aspects while committing.
>>> On the subject of writebacks, we should get around to alternating-up the
>>> use of clflushopt and clwb, either of which would be better than a
>>> clflush in this case (avoiding the need for the leading mfence).
>>>
>>> However, the ISA extension document does not indicate which processors
>>> will have support for these new instructions.
>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>>
>> We really should add support for this. On shutting down a very large
>> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU.
>> This was due to serializing nature of CLFLUSH.
> But flushing the IOMMU isn't being done via CPU instructions, but
> rather via commands sent to the IOMMU. I.e. I'm somewhat
> confused by your reply.

I didn't mean flushing IOMMU itself, sorry. I meant 
__iommu_flush_cache() (or whatever it's equivalent we had in the 
product, which was 4.1-based).

-boris

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Chen, Tiejun]

On 2015/5/5 17:24, Jan Beulich wrote:
>>>> On 05.05.15 at 04:45, <tiejun.chen@intel.com> wrote:
>> Does this work for everyone?
>
> Please first of all explain why the interfaces in asm/flushtlb.h can't
> be used here (at least when flushing entire pages). Because - as

I also don't understand any reason we didn't use this previously on 
IOMMU side...

> said before - for a complete fix you'd need to deal with the CLFLUSH
> use(s) elsewhere in the system too.

Looks we need to do this in xen/arch/x86/flushtlb.c:flush_area_local().

>
> Btw, isn't the !iommus_incoherent check in that function inverted?
> I.e. why would we _not_ need to flush caches when IOMMUs
> are not coherent (and why would flushing be needed when they're
> coherent anyway)?
>

I guess you're misunderstanding this

     if ( !ecap_coherent(iommu->ecap) )
         iommus_incoherent = 1;

So here !iommus_incoherent means IOMMU is coherent and then we don't 
need to flush cache in this case.

Thanks
Tiejun

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Chen, Tiejun]

On 2015/5/5 23:46, Boris Ostrovsky wrote:
> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>   void cacheline_flush(char * addr)
>>>>   {
>>>> +    mb();
>>>>       clflush(addr);
>>>> +    mb();
>>>>   }
>>> I think the purpose of the flush is to force write back, not to evict
>>> the cache line, and if so wmb() would appear to be sufficient. As
>>> the SDM says that's not the case, a comment explaining why wmb()
>>> is not sufficient would seem necessary. Plus in the description I
>>> think "serializing" needs to be changed to "fencing", as serialization
>>> is not what we really care about here. If you and the maintainers
>>> agree, I could certainly fix up both aspects while committing.
>> On the subject of writebacks, we should get around to alternating-up the
>> use of clflushopt and clwb, either of which would be better than a
>> clflush in this case (avoiding the need for the leading mfence).
>>
>> However, the ISA extension document does not indicate which processors
>> will have support for these new instructions.
>
> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>
> We really should add support for this. On shutting down a very large
> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU.
> This was due to serializing nature of CLFLUSH.
>

CLFLUSHOPT/CLWB instructions are ordered only by store-fencing 
operations, so at least we still need SFENCE.

Thanks
Tiejun

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 05.05.15 at 18:11, <boris.ostrovsky@oracle.com> wrote:
> On 05/05/2015 11:58 AM, Jan Beulich wrote:
>>>>> On 05.05.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>>>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>>>    
>>>>>>    void cacheline_flush(char * addr)
>>>>>>    {
>>>>>> +    mb();
>>>>>>        clflush(addr);
>>>>>> +    mb();
>>>>>>    }
>>>>> I think the purpose of the flush is to force write back, not to evict
>>>>> the cache line, and if so wmb() would appear to be sufficient. As
>>>>> the SDM says that's not the case, a comment explaining why wmb()
>>>>> is not sufficient would seem necessary. Plus in the description I
>>>>> think "serializing" needs to be changed to "fencing", as serialization
>>>>> is not what we really care about here. If you and the maintainers
>>>>> agree, I could certainly fix up both aspects while committing.
>>>> On the subject of writebacks, we should get around to alternating-up the
>>>> use of clflushopt and clwb, either of which would be better than a
>>>> clflush in this case (avoiding the need for the leading mfence).
>>>>
>>>> However, the ISA extension document does not indicate which processors
>>>> will have support for these new instructions.
>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf 
>>>
>>> We really should add support for this. On shutting down a very large
>>> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU.
>>> This was due to serializing nature of CLFLUSH.
>> But flushing the IOMMU isn't being done via CPU instructions, but
>> rather via commands sent to the IOMMU. I.e. I'm somewhat
>> confused by your reply.
> 
> I didn't mean flushing IOMMU itself, sorry. I meant 
> __iommu_flush_cache() (or whatever it's equivalent we had in the 
> product, which was 4.1-based).

In that case I wonder how much of that flushing is really necessary
during IOMMU teardown. VT-d maintainers?

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 06.05.15 at 08:47, <tiejun.chen@intel.com> wrote:
> On 2015/5/5 17:24, Jan Beulich wrote:
>> Btw, isn't the !iommus_incoherent check in that function inverted?
>> I.e. why would we _not_ need to flush caches when IOMMUs
>> are not coherent (and why would flushing be needed when they're
>> coherent anyway)?
>>
> 
> I guess you're misunderstanding this
> 
>      if ( !ecap_coherent(iommu->ecap) )
>          iommus_incoherent = 1;
> 
> So here !iommus_incoherent means IOMMU is coherent and then we don't 
> need to flush cache in this case.

Ah, indeed - I overlooked the "in" in the variable name (together with
the thus resulting double negation in the expression). Sorry for the
noise.

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Chen, Tiejun]

On 2015/5/6 15:12, Jan Beulich wrote:
>>>> On 05.05.15 at 18:11, <boris.ostrovsky@oracle.com> wrote:
>> On 05/05/2015 11:58 AM, Jan Beulich wrote:
>>>>>> On 05.05.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>>>>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>>>>
>>>>>>>     void cacheline_flush(char * addr)
>>>>>>>     {
>>>>>>> +    mb();
>>>>>>>         clflush(addr);
>>>>>>> +    mb();
>>>>>>>     }
>>>>>> I think the purpose of the flush is to force write back, not to evict
>>>>>> the cache line, and if so wmb() would appear to be sufficient. As
>>>>>> the SDM says that's not the case, a comment explaining why wmb()
>>>>>> is not sufficient would seem necessary. Plus in the description I
>>>>>> think "serializing" needs to be changed to "fencing", as serialization
>>>>>> is not what we really care about here. If you and the maintainers
>>>>>> agree, I could certainly fix up both aspects while committing.
>>>>> On the subject of writebacks, we should get around to alternating-up the
>>>>> use of clflushopt and clwb, either of which would be better than a
>>>>> clflush in this case (avoiding the need for the leading mfence).
>>>>>
>>>>> However, the ISA extension document does not indicate which processors
>>>>> will have support for these new instructions.
>>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>>>>
>>>> We really should add support for this. On shutting down a very large
>>>> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU.
>>>> This was due to serializing nature of CLFLUSH.
>>> But flushing the IOMMU isn't being done via CPU instructions, but
>>> rather via commands sent to the IOMMU. I.e. I'm somewhat
>>> confused by your reply.
>>
>> I didn't mean flushing IOMMU itself, sorry. I meant
>> __iommu_flush_cache() (or whatever it's equivalent we had in the
>> product, which was 4.1-based).
>
> In that case I wonder how much of that flushing is really necessary

Sorry, what is that case?

> during IOMMU teardown. VT-d maintainers?
>

In most cases __iommu_flush_cache() is used to flush any remapping 
structures into memory then IOMMU can get proper data.

Thanks
Tiejun

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Jan Beulich]

>>> On 06.05.15 at 09:26, <tiejun.chen@intel.com> wrote:
> On 2015/5/6 15:12, Jan Beulich wrote:
>>>>> On 05.05.15 at 18:11, <boris.ostrovsky@oracle.com> wrote:
>>> On 05/05/2015 11:58 AM, Jan Beulich wrote:
>>>>>>> On 05.05.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>>>>>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>>>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>>>>>
>>>>>>>>     void cacheline_flush(char * addr)
>>>>>>>>     {
>>>>>>>> +    mb();
>>>>>>>>         clflush(addr);
>>>>>>>> +    mb();
>>>>>>>>     }
>>>>>>> I think the purpose of the flush is to force write back, not to evict
>>>>>>> the cache line, and if so wmb() would appear to be sufficient. As
>>>>>>> the SDM says that's not the case, a comment explaining why wmb()
>>>>>>> is not sufficient would seem necessary. Plus in the description I
>>>>>>> think "serializing" needs to be changed to "fencing", as serialization
>>>>>>> is not what we really care about here. If you and the maintainers
>>>>>>> agree, I could certainly fix up both aspects while committing.
>>>>>> On the subject of writebacks, we should get around to alternating-up the
>>>>>> use of clflushopt and clwb, either of which would be better than a
>>>>>> clflush in this case (avoiding the need for the leading mfence).
>>>>>>
>>>>>> However, the ISA extension document does not indicate which processors
>>>>>> will have support for these new instructions.
>>>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf 
>>>>>
>>>>> We really should add support for this. On shutting down a very large
>>>>> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU.
>>>>> This was due to serializing nature of CLFLUSH.
>>>> But flushing the IOMMU isn't being done via CPU instructions, but
>>>> rather via commands sent to the IOMMU. I.e. I'm somewhat
>>>> confused by your reply.
>>>
>>> I didn't mean flushing IOMMU itself, sorry. I meant
>>> __iommu_flush_cache() (or whatever it's equivalent we had in the
>>> product, which was 4.1-based).
>>
>> In that case I wonder how much of that flushing is really necessary
> 
> Sorry, what is that case?

The flushing CPU of side caches during IOMMU teardown for a guest.

>> during IOMMU teardown. VT-d maintainers?
>>
> 
> In most cases __iommu_flush_cache() is used to flush any remapping 
> structures into memory then IOMMU can get proper data.

Right, but here we're talking about teardown. Since the IOMMU
isn't to use any mappings anymore anyway for the dying guest,
there's little point in flushing respective changes from caches (or
at least that flushing could be limited to the ripping out of top
level structures, provided these get zapped before anything
hanging off of them).

Jan

Re: [PATCH 1/3] xen/vt-d: need barriers to workaround CLFLUSH

[Boris Ostrovsky]

On 05/06/2015 03:12 AM, Jan Beulich wrote:
>>>> On 05.05.15 at 18:11, <boris.ostrovsky@oracle.com> wrote:
>> On 05/05/2015 11:58 AM, Jan Beulich wrote:
>>>>>> On 05.05.15 at 17:46, <boris.ostrovsky@oracle.com> wrote:
>>>> On 05/04/2015 05:14 AM, Andrew Cooper wrote:
>>>>> On 04/05/2015 09:52, Jan Beulich wrote:
>>>>>>>>> On 04.05.15 at 04:16, <tiejun.chen@intel.com> wrote:
>>>>>>> --- a/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>>> +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
>>>>>>> @@ -56,7 +56,9 @@ unsigned int get_cache_line_size(void)
>>>>>>>     
>>>>>>>     void cacheline_flush(char * addr)
>>>>>>>     {
>>>>>>> +    mb();
>>>>>>>         clflush(addr);
>>>>>>> +    mb();
>>>>>>>     }
>>>>>> I think the purpose of the flush is to force write back, not to evict
>>>>>> the cache line, and if so wmb() would appear to be sufficient. As
>>>>>> the SDM says that's not the case, a comment explaining why wmb()
>>>>>> is not sufficient would seem necessary. Plus in the description I
>>>>>> think "serializing" needs to be changed to "fencing", as serialization
>>>>>> is not what we really care about here. If you and the maintainers
>>>>>> agree, I could certainly fix up both aspects while committing.
>>>>> On the subject of writebacks, we should get around to alternating-up the
>>>>> use of clflushopt and clwb, either of which would be better than a
>>>>> clflush in this case (avoiding the need for the leading mfence).
>>>>>
>>>>> However, the ISA extension document does not indicate which processors
>>>>> will have support for these new instructions.
>>>> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
>>>>
>>>> We really should add support for this. On shutting down a very large
>>>> guest (hundreds of GB) we observed *minutes* spent in flushing IOMMU.
>>>> This was due to serializing nature of CLFLUSH.
>>> But flushing the IOMMU isn't being done via CPU instructions, but
>>> rather via commands sent to the IOMMU. I.e. I'm somewhat
>>> confused by your reply.
>> I didn't mean flushing IOMMU itself, sorry. I meant
>> __iommu_flush_cache() (or whatever it's equivalent we had in the
>> product, which was 4.1-based).
> In that case I wonder how much of that flushing is really necessary
> during IOMMU teardown. VT-d maintainers?

I should mention that we saw this on 4.1, where iommu teardown is done 
in the domain destruction code path. After we backported code that moves 
this into a tasklet things got much better.

Of course we still are doing flushing, even if in the background, and 
therefore system resources are still being consumed (and memory is not 
available until flushing is done). So if this is unnecessary then there 
are good reasons not to do it.

-boris