[PATCH] Don't track all memory when enabling log dirty to track vram

[PATCH] Don't track all memory when enabling log dirty to track vram

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

When enabling log dirty mode, it sets all guest's memory to readonly.
And in HAP enabled domain, it modifies all EPT entries to clear write bit
to make sure it is readonly. This will cause problem if VT-d shares page
table with EPT: the device may issue a DMA write request, then VT-d engine
tells it the target memory is readonly and result in VT-d fault.

Currnetly, there are two places will enable log dirty mode: migration and vram
tracking. Migration with device assigned is not allowed, so it is ok. For vram,
it doesn't need to set all memory to readonly. Only track the vram range is enough.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/mm/hap/hap.c       |   20 ++++++++++++++------
 xen/arch/x86/mm/paging.c        |    9 +++++----
 xen/arch/x86/mm/shadow/common.c |    2 +-
 xen/include/asm-x86/domain.h    |    2 +-
 xen/include/asm-x86/paging.h    |    5 +++--
 xen/include/asm-x86/shadow.h    |    2 +-
 6 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index d3f64bd..5f75636 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -82,7 +82,7 @@ int hap_track_dirty_vram(struct domain *d,
         if ( !paging_mode_log_dirty(d) )
         {
             hap_logdirty_init(d);
-            rc = paging_log_dirty_enable(d);
+            rc = paging_log_dirty_enable(d, 0);
             if ( rc )
                 goto out;
         }
@@ -167,17 +167,25 @@ out:
 /*            HAP LOG DIRTY SUPPORT             */
 /************************************************/
 
-/* hap code to call when log_dirty is enable. return 0 if no problem found. */
-static int hap_enable_log_dirty(struct domain *d)
+/*
+ * hap code to call when log_dirty is enable. return 0 if no problem found.
+ *
+ * NB: Domain that having device assigned should not set log_global. Because
+ * there is no way to track the memory updating from device.
+ */
+static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
 {
     /* turn on PG_log_dirty bit in paging mode */
     paging_lock(d);
     d->arch.paging.mode |= PG_log_dirty;
     paging_unlock(d);
 
-    /* set l1e entries of P2M table to be read-only. */
-    p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
-    flush_tlb_mask(d->domain_dirty_cpumask);
+    if ( log_global )
+    {
+        /* set l1e entries of P2M table to be read-only. */
+        p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
+        flush_tlb_mask(d->domain_dirty_cpumask);
+    }
     return 0;
 }
 
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 21344e5..ab5eacb 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -164,7 +164,7 @@ void paging_free_log_dirty_bitmap(struct domain *d)
     paging_unlock(d);
 }
 
-int paging_log_dirty_enable(struct domain *d)
+int paging_log_dirty_enable(struct domain *d, bool_t log_global)
 {
     int ret;
 
@@ -172,7 +172,7 @@ int paging_log_dirty_enable(struct domain *d)
         return -EINVAL;
 
     domain_pause(d);
-    ret = d->arch.paging.log_dirty.enable_log_dirty(d);
+    ret = d->arch.paging.log_dirty.enable_log_dirty(d, log_global);
     domain_unpause(d);
 
     return ret;
@@ -489,7 +489,8 @@ void paging_log_dirty_range(struct domain *d,
  * These function pointers must not be followed with the log-dirty lock held.
  */
 void paging_log_dirty_init(struct domain *d,
-                           int    (*enable_log_dirty)(struct domain *d),
+                           int    (*enable_log_dirty)(struct domain *d,
+                                                      bool_t log_global),
                            int    (*disable_log_dirty)(struct domain *d),
                            void   (*clean_dirty_bitmap)(struct domain *d))
 {
@@ -590,7 +591,7 @@ int paging_domctl(struct domain *d, xen_domctl_shadow_op_t *sc,
     case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
         if ( hap_enabled(d) )
             hap_logdirty_init(d);
-        return paging_log_dirty_enable(d);
+        return paging_log_dirty_enable(d, 1);
 
     case XEN_DOMCTL_SHADOW_OP_OFF:
         if ( paging_mode_log_dirty(d) )
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 0bfa595..11c6b62 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3418,7 +3418,7 @@ shadow_write_p2m_entry(struct vcpu *v, unsigned long gfn,
 /* Shadow specific code which is called in paging_log_dirty_enable().
  * Return 0 if no problem found.
  */
-int shadow_enable_log_dirty(struct domain *d)
+int shadow_enable_log_dirty(struct domain *d, bool_t log_global)
 {
     int ret;
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index ea72db2..4ff89f0 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -169,7 +169,7 @@ struct log_dirty_domain {
     unsigned int   dirty_count;
 
     /* functions which are paging mode specific */
-    int            (*enable_log_dirty   )(struct domain *d);
+    int            (*enable_log_dirty   )(struct domain *d, bool_t log_global);
     int            (*disable_log_dirty  )(struct domain *d);
     void           (*clean_dirty_bitmap )(struct domain *d);
 };
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index cd7ee3b..8dd2a61 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -143,14 +143,15 @@ void paging_log_dirty_range(struct domain *d,
                             uint8_t *dirty_bitmap);
 
 /* enable log dirty */
-int paging_log_dirty_enable(struct domain *d);
+int paging_log_dirty_enable(struct domain *d, bool_t log_global);
 
 /* disable log dirty */
 int paging_log_dirty_disable(struct domain *d);
 
 /* log dirty initialization */
 void paging_log_dirty_init(struct domain *d,
-                           int  (*enable_log_dirty)(struct domain *d),
+                           int  (*enable_log_dirty)(struct domain *d,
+                                                    bool_t log_global),
                            int  (*disable_log_dirty)(struct domain *d),
                            void (*clean_dirty_bitmap)(struct domain *d));
 
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 852023d..348915e 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -82,7 +82,7 @@ void shadow_teardown(struct domain *d);
 void shadow_final_teardown(struct domain *d);
 
 /* shadow code to call when log dirty is enabled */
-int shadow_enable_log_dirty(struct domain *d);
+int shadow_enable_log_dirty(struct domain *d, bool_t log_global);
 
 /* shadow code to call when log dirty is disabled */
 int shadow_disable_log_dirty(struct domain *d);
-- 
1.7.1

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Tim Deegan]

At 14:14 +0800 on 10 Feb (1392038040), Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> When enabling log dirty mode, it sets all guest's memory to readonly.
> And in HAP enabled domain, it modifies all EPT entries to clear write bit
> to make sure it is readonly. This will cause problem if VT-d shares page
> table with EPT: the device may issue a DMA write request, then VT-d engine
> tells it the target memory is readonly and result in VT-d fault.

So that's a problem even if only the VGA framebuffer is being tracked
-- DMA from a passthrough device will either cause a spurious error or
fail to update the dirt bitmap. 

I think it would be better not to allow VT-d and EPT to share
pagetables in cases where devices are passed through (i.e. all cases
where VT-d is in use).

Tim.

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Tim Deegan wrote on 2014-02-10:
> At 14:14 +0800 on 10 Feb (1392038040), Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> When enabling log dirty mode, it sets all guest's memory to readonly.
>> And in HAP enabled domain, it modifies all EPT entries to clear
>> write bit to make sure it is readonly. This will cause problem if
>> VT-d shares page table with EPT: the device may issue a DMA write
>> request, then VT-d engine tells it the target memory is readonly and
>> result in VT-d
> fault.
> 
> So that's a problem even if only the VGA framebuffer is being tracked
> -- DMA from a passthrough device will either cause a spurious error or
> fail to update the dirt bitmap.

Do you mean the VGA frambuffer will be used as DMA buffer in guest? If yes, I think it's guest's responsibility to ensure it never happens.

> 
> I think it would be better not to allow VT-d and EPT to share
> pagetables in cases where devices are passed through (i.e. all cases where VT-d is in use).

Without VT-d and EPT share page, we still cannot track the memory updating from DMA. I think the point is that we cannot track the memory updating via DMA. So the user should use the log dirty mode carefully. Also, I am not sure whether the memory updating from dom0 and QEMU is tracked currently.

> 
> Tim.


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Tim Deegan]

At 08:15 +0000 on 10 Feb (1392016516), Zhang, Yang Z wrote:
> Tim Deegan wrote on 2014-02-10:
> > At 14:14 +0800 on 10 Feb (1392038040), Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> When enabling log dirty mode, it sets all guest's memory to readonly.
> >> And in HAP enabled domain, it modifies all EPT entries to clear
> >> write bit to make sure it is readonly. This will cause problem if
> >> VT-d shares page table with EPT: the device may issue a DMA write
> >> request, then VT-d engine tells it the target memory is readonly and
> >> result in VT-d
> > fault.
> > 
> > So that's a problem even if only the VGA framebuffer is being tracked
> > -- DMA from a passthrough device will either cause a spurious error or
> > fail to update the dirt bitmap.
> 
> Do you mean the VGA frambuffer will be used as DMA buffer in guest? If yes, I think it's guest's responsibility to ensure it never happens.
> 

I don't think that works.  We can't expect arbitrary OSes to (a) know
they're running on Xen and (b) know that that means they can't DMA to
or from their framebuffers.

> Without VT-d and EPT share page, we still cannot track the memory
> updating from DMA.

Yeah, but at least we don't risk crashing the _host_ by throwing DMA
failures around. 

> I think the point is that we cannot track the
> memory updating via DMA. So the user should use the log dirty mode
> carefully. Also, I am not sure whether the memory updating from dom0
> and QEMU is tracked currently.

Yes, dom0 and qemu updates are tracked in the log-dirty bitmaps.

Tim.

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On Tue, Feb 11, 2014 at 9:02 AM, Tim Deegan <tim@xen.org> wrote:
> At 08:15 +0000 on 10 Feb (1392016516), Zhang, Yang Z wrote:
>> Tim Deegan wrote on 2014-02-10:
>> > At 14:14 +0800 on 10 Feb (1392038040), Yang Zhang wrote:
>> >> From: Yang Zhang <yang.z.zhang@Intel.com>
>> >>
>> >> When enabling log dirty mode, it sets all guest's memory to readonly.
>> >> And in HAP enabled domain, it modifies all EPT entries to clear
>> >> write bit to make sure it is readonly. This will cause problem if
>> >> VT-d shares page table with EPT: the device may issue a DMA write
>> >> request, then VT-d engine tells it the target memory is readonly and
>> >> result in VT-d
>> > fault.
>> >
>> > So that's a problem even if only the VGA framebuffer is being tracked
>> > -- DMA from a passthrough device will either cause a spurious error or
>> > fail to update the dirt bitmap.
>>
>> Do you mean the VGA frambuffer will be used as DMA buffer in guest? If yes, I think it's guest's responsibility to ensure it never happens.
>>
>
> I don't think that works.  We can't expect arbitrary OSes to (a) know
> they're running on Xen and (b) know that that means they can't DMA to
> or from their framebuffers.
>
>> Without VT-d and EPT share page, we still cannot track the memory
>> updating from DMA.
>
> Yeah, but at least we don't risk crashing the _host_ by throwing DMA
> failures around.

What I'm missing here is what you think a proper solution is.  It seems we have:

A. Share EPT/IOMMU tables, only do log-dirty tracking on the buffer
being tracked, and hope the guest doesn't DMA into video ram; DMA
causes IOMMU fault. (This really shouldn't crash the host under normal
circumstances; if it does it's a hardware bug.)
B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA into
video ram.  DMA causes missed update to dirty bitmap, which will
hopefully just cause screen corruption.
C. Do buffer scanning rather than dirty vram tracking (SLOW)
D. Don't allow both a virtual video card and pass-through

Given that most operating systems will probably *not* DMA into video
ram, and that an IOMMU fault isn't *supposed* to be able to crash the
host, 'A' seems like the most reasonable option to me.

 -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Tim Deegan]

At 10:59 +0000 on 11 Feb (1392112778), George Dunlap wrote:
> On Tue, Feb 11, 2014 at 9:02 AM, Tim Deegan <tim@xen.org> wrote:
> > At 08:15 +0000 on 10 Feb (1392016516), Zhang, Yang Z wrote:
> >> Tim Deegan wrote on 2014-02-10:
> >> > At 14:14 +0800 on 10 Feb (1392038040), Yang Zhang wrote:
> >> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> >>
> >> >> When enabling log dirty mode, it sets all guest's memory to readonly.
> >> >> And in HAP enabled domain, it modifies all EPT entries to clear
> >> >> write bit to make sure it is readonly. This will cause problem if
> >> >> VT-d shares page table with EPT: the device may issue a DMA write
> >> >> request, then VT-d engine tells it the target memory is readonly and
> >> >> result in VT-d
> >> > fault.
> >> >
> >> > So that's a problem even if only the VGA framebuffer is being tracked
> >> > -- DMA from a passthrough device will either cause a spurious error or
> >> > fail to update the dirt bitmap.
> >>
> >> Do you mean the VGA frambuffer will be used as DMA buffer in guest? If yes, I think it's guest's responsibility to ensure it never happens.
> >>
> >
> > I don't think that works.  We can't expect arbitrary OSes to (a) know
> > they're running on Xen and (b) know that that means they can't DMA to
> > or from their framebuffers.
> >
> >> Without VT-d and EPT share page, we still cannot track the memory
> >> updating from DMA.
> >
> > Yeah, but at least we don't risk crashing the _host_ by throwing DMA
> > failures around.
> 
> What I'm missing here is what you think a proper solution is.

A _proper_ solution would be for the IOMMU h/w to allow restartable
faults, so that we can do all the usual fault-driven virtual memory
operations with DMA. :)  In the meantime...

>  It seems we have:
> 
> A. Share EPT/IOMMU tables, only do log-dirty tracking on the buffer
> being tracked, and hope the guest doesn't DMA into video ram; DMA
> causes IOMMU fault. (This really shouldn't crash the host under normal
> circumstances; if it does it's a hardware bug.)

Note "hope" and "shouldn't" there. :)

> B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA into
> video ram.  DMA causes missed update to dirty bitmap, which will
> hopefully just cause screen corruption.

Yep.  At a cost of about 0.2% in space and some extra bookkeeping
(for VMs that actually have devices passed through to them).
The extra bookkeeping could be expensive in some cases, but basically
all of those cases are already incompatible with IOMMU.

> C. Do buffer scanning rather than dirty vram tracking (SLOW)
> D. Don't allow both a virtual video card and pass-through

E. Share EPT and IOMMU tables until someone turns on log-dirty mode
and then split them out.  That one 

> Given that most operating systems will probably *not* DMA into video
> ram, and that an IOMMU fault isn't *supposed* to be able to crash the
> host, 'A' seems like the most reasonable option to me.

Meh, OK.  I prefer 'B' but 'A' is better than nothing, I guess, and
seems to have most support from other people.  On that basis this
patch can have my Ack.

Is it intended for 4.4 as a bugfix (i.e. should I be checking it in?)

Tim.

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 11.02.14 at 12:55, Tim Deegan <tim@xen.org> wrote:
> At 10:59 +0000 on 11 Feb (1392112778), George Dunlap wrote:
>> What I'm missing here is what you think a proper solution is.
> 
> A _proper_ solution would be for the IOMMU h/w to allow restartable
> faults, so that we can do all the usual fault-driven virtual memory
> operations with DMA. :)  In the meantime...

Or maintaining the A/D bits for IOMMU side accesses too.

>>  It seems we have:
>> 
>> A. Share EPT/IOMMU tables, only do log-dirty tracking on the buffer
>> being tracked, and hope the guest doesn't DMA into video ram; DMA
>> causes IOMMU fault. (This really shouldn't crash the host under normal
>> circumstances; if it does it's a hardware bug.)
> 
> Note "hope" and "shouldn't" there. :)
> 
>> B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA into
>> video ram.  DMA causes missed update to dirty bitmap, which will
>> hopefully just cause screen corruption.
> 
> Yep.  At a cost of about 0.2% in space and some extra bookkeeping
> (for VMs that actually have devices passed through to them).
> The extra bookkeeping could be expensive in some cases, but basically
> all of those cases are already incompatible with IOMMU.
> 
>> C. Do buffer scanning rather than dirty vram tracking (SLOW)
>> D. Don't allow both a virtual video card and pass-through
> 
> E. Share EPT and IOMMU tables until someone turns on log-dirty mode
> and then split them out.  That one 

Wouldn't that be problematic in terms of memory being available,
namely when using ballooning in Dom0?

>> Given that most operating systems will probably *not* DMA into video
>> ram, and that an IOMMU fault isn't *supposed* to be able to crash the
>> host, 'A' seems like the most reasonable option to me.
> 
> Meh, OK.  I prefer 'B' but 'A' is better than nothing, I guess, and
> seems to have most support from other people.  On that basis this
> patch can have my Ack.

I too would consider B better than A.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/11/2014 12:57 PM, Jan Beulich wrote:
>>>> On 11.02.14 at 12:55, Tim Deegan <tim@xen.org> wrote:
>> At 10:59 +0000 on 11 Feb (1392112778), George Dunlap wrote:
>>> What I'm missing here is what you think a proper solution is.
>>
>> A _proper_ solution would be for the IOMMU h/w to allow restartable
>> faults, so that we can do all the usual fault-driven virtual memory
>> operations with DMA. :)  In the meantime...
>
> Or maintaining the A/D bits for IOMMU side accesses too.
>
>>>   It seems we have:
>>>
>>> A. Share EPT/IOMMU tables, only do log-dirty tracking on the buffer
>>> being tracked, and hope the guest doesn't DMA into video ram; DMA
>>> causes IOMMU fault. (This really shouldn't crash the host under normal
>>> circumstances; if it does it's a hardware bug.)
>>
>> Note "hope" and "shouldn't" there. :)
>>
>>> B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA into
>>> video ram.  DMA causes missed update to dirty bitmap, which will
>>> hopefully just cause screen corruption.
>>
>> Yep.  At a cost of about 0.2% in space and some extra bookkeeping
>> (for VMs that actually have devices passed through to them).
>> The extra bookkeeping could be expensive in some cases, but basically
>> all of those cases are already incompatible with IOMMU.
>>
>>> C. Do buffer scanning rather than dirty vram tracking (SLOW)
>>> D. Don't allow both a virtual video card and pass-through
>>
>> E. Share EPT and IOMMU tables until someone turns on log-dirty mode
>> and then split them out.  That one
>
> Wouldn't that be problematic in terms of memory being available,
> namely when using ballooning in Dom0?
>
>>> Given that most operating systems will probably *not* DMA into video
>>> ram, and that an IOMMU fault isn't *supposed* to be able to crash the
>>> host, 'A' seems like the most reasonable option to me.
>>
>> Meh, OK.  I prefer 'B' but 'A' is better than nothing, I guess, and
>> seems to have most support from other people.  On that basis this
>> patch can have my Ack.
>
> I too would consider B better than A.

I think I got a bit distracted with the "A isn't really so bad" thing. 
Actually, if the overhead of not sharing tables isn't very high, then B 
isn't such a bad option.  In fact, B is what I expected Yang to submit 
when he originally described the problem.

I was going to say, from a release perspective, B is probably the safest 
option for now.  But on the other hand, if we've been testing sharing 
all this time, maybe switching back over to non-sharing whole-hog has 
the higher risk?

Anyway, both are at least probably equal risk-wise.  How easy is it to 
implement?

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

George Dunlap wrote on 2014-02-11:
> On 02/11/2014 12:57 PM, Jan Beulich wrote:
>>>>> On 11.02.14 at 12:55, Tim Deegan <tim@xen.org> wrote:
>>> At 10:59 +0000 on 11 Feb (1392112778), George Dunlap wrote:
>>>> What I'm missing here is what you think a proper solution is.
>>> 
>>> A _proper_ solution would be for the IOMMU h/w to allow restartable
>>> faults, so that we can do all the usual fault-driven virtual memory
>>> operations with DMA. :)  In the meantime...
>> 
>> Or maintaining the A/D bits for IOMMU side accesses too.
>> 
>>>>   It seems we have:
>>>> A. Share EPT/IOMMU tables, only do log-dirty tracking on the
>>>> buffer being tracked, and hope the guest doesn't DMA into video
>>>> ram; DMA causes IOMMU fault. (This really shouldn't crash the host
>>>> under normal circumstances; if it does it's a hardware bug.)
>>> 
>>> Note "hope" and "shouldn't" there. :)
>>> 
>>>> B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA
>>>> into video ram.  DMA causes missed update to dirty bitmap, which
>>>> will hopefully just cause screen corruption.
>>> 
>>> Yep.  At a cost of about 0.2% in space and some extra bookkeeping
>>> (for VMs that actually have devices passed through to them).
>>> The extra bookkeeping could be expensive in some cases, but
>>> basically all of those cases are already incompatible with IOMMU.
>>> 
>>>> C. Do buffer scanning rather than dirty vram tracking (SLOW) D.
>>>> Don't allow both a virtual video card and pass-through
>>> 
>>> E. Share EPT and IOMMU tables until someone turns on log-dirty mode
>>> and then split them out.  That one
>> 
>> Wouldn't that be problematic in terms of memory being available,
>> namely when using ballooning in Dom0?
>> 
>>>> Given that most operating systems will probably *not* DMA into
>>>> video ram, and that an IOMMU fault isn't *supposed* to be able to
>>>> crash the host, 'A' seems like the most reasonable option to me.
>>> 
>>> Meh, OK.  I prefer 'B' but 'A' is better than nothing, I guess, and
>>> seems to have most support from other people.  On that basis this
>>> patch can have my Ack.
>> 
>> I too would consider B better than A.
> 
> I think I got a bit distracted with the "A isn't really so bad" thing.
> Actually, if the overhead of not sharing tables isn't very high, then
> B isn't such a bad option.  In fact, B is what I expected Yang to
> submit when he originally described the problem.

Actually, the first solution came to my mind is B. Then I realized that even chose B, we still cannot track the memory updating from DMA(even with A/D bit, it still a problem). Also, considering the current usage case of log dirty in Xen(only vram tracking has problem), I though A is better.: Hypervisor only need to track the vram change. If a malicious guest try to DMA to vram range, it only crashed himself (This should be reasonable).

> 
> I was going to say, from a release perspective, B is probably the
> safest option for now.  But on the other hand, if we've been testing
> sharing all this time, maybe switching back over to non-sharing whole-hog has the higher risk?

Another problem with B is that current VT-d large paging supporting relies on the sharing EPT and VT-d page table. This means if we choose B, then we need to re-enable VT-d large page. This would be a huge performance impaction for Xen 4.4 on using VT-d solution.

> 
> Anyway, both are at least probably equal risk-wise.  How easy is it to
> implement?
> 
>   -George


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
> George Dunlap wrote on 2014-02-11:
>> On 02/11/2014 12:57 PM, Jan Beulich wrote:
>>>>>> On 11.02.14 at 12:55, Tim Deegan <tim@xen.org> wrote:
>>>> At 10:59 +0000 on 11 Feb (1392112778), George Dunlap wrote:
>>>>> What I'm missing here is what you think a proper solution is.
>>>> A _proper_ solution would be for the IOMMU h/w to allow restartable
>>>> faults, so that we can do all the usual fault-driven virtual memory
>>>> operations with DMA. :)  In the meantime...
>>> Or maintaining the A/D bits for IOMMU side accesses too.
>>>
>>>>>    It seems we have:
>>>>> A. Share EPT/IOMMU tables, only do log-dirty tracking on the
>>>>> buffer being tracked, and hope the guest doesn't DMA into video
>>>>> ram; DMA causes IOMMU fault. (This really shouldn't crash the host
>>>>> under normal circumstances; if it does it's a hardware bug.)
>>>> Note "hope" and "shouldn't" there. :)
>>>>
>>>>> B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA
>>>>> into video ram.  DMA causes missed update to dirty bitmap, which
>>>>> will hopefully just cause screen corruption.
>>>> Yep.  At a cost of about 0.2% in space and some extra bookkeeping
>>>> (for VMs that actually have devices passed through to them).
>>>> The extra bookkeeping could be expensive in some cases, but
>>>> basically all of those cases are already incompatible with IOMMU.
>>>>
>>>>> C. Do buffer scanning rather than dirty vram tracking (SLOW) D.
>>>>> Don't allow both a virtual video card and pass-through
>>>> E. Share EPT and IOMMU tables until someone turns on log-dirty mode
>>>> and then split them out.  That one
>>> Wouldn't that be problematic in terms of memory being available,
>>> namely when using ballooning in Dom0?
>>>
>>>>> Given that most operating systems will probably *not* DMA into
>>>>> video ram, and that an IOMMU fault isn't *supposed* to be able to
>>>>> crash the host, 'A' seems like the most reasonable option to me.
>>>> Meh, OK.  I prefer 'B' but 'A' is better than nothing, I guess, and
>>>> seems to have most support from other people.  On that basis this
>>>> patch can have my Ack.
>>> I too would consider B better than A.
>> I think I got a bit distracted with the "A isn't really so bad" thing.
>> Actually, if the overhead of not sharing tables isn't very high, then
>> B isn't such a bad option.  In fact, B is what I expected Yang to
>> submit when he originally described the problem.
> Actually, the first solution came to my mind is B. Then I realized that even chose B, we still cannot track the memory updating from DMA(even with A/D bit, it still a problem). Also, considering the current usage case of log dirty in Xen(only vram tracking has problem), I though A is better.: Hypervisor only need to track the vram change. If a malicious guest try to DMA to vram range, it only crashed himself (This should be reasonable).
>
>> I was going to say, from a release perspective, B is probably the
>> safest option for now.  But on the other hand, if we've been testing
>> sharing all this time, maybe switching back over to non-sharing whole-hog has the higher risk?
> Another problem with B is that current VT-d large paging supporting relies on the sharing EPT and VT-d page table. This means if we choose B, then we need to re-enable VT-d large page. This would be a huge performance impaction for Xen 4.4 on using VT-d solution.

OK -- if that's the case, then it definitely tips the balance back to 
A.  Unless Tim or Jan disagrees, can one of you two check it in?

Don't rush your judgement; but it would be nice to have this in before 
RC4, which would mean checking it in today preferrably, or early 
tomorrow at the latest.

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 13.02.14 at 16:46, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>> George Dunlap wrote on 2014-02-11:
>>> I think I got a bit distracted with the "A isn't really so bad" thing.
>>> Actually, if the overhead of not sharing tables isn't very high, then
>>> B isn't such a bad option.  In fact, B is what I expected Yang to
>>> submit when he originally described the problem.
>> Actually, the first solution came to my mind is B. Then I realized that even 
> chose B, we still cannot track the memory updating from DMA(even with A/D 
> bit, it still a problem). Also, considering the current usage case of log 
> dirty in Xen(only vram tracking has problem), I though A is better.: 
> Hypervisor only need to track the vram change. If a malicious guest try to 
> DMA to vram range, it only crashed himself (This should be reasonable).
>>
>>> I was going to say, from a release perspective, B is probably the
>>> safest option for now.  But on the other hand, if we've been testing
>>> sharing all this time, maybe switching back over to non-sharing whole-hog has 
> the higher risk?
>> Another problem with B is that current VT-d large paging supporting relies on 
> the sharing EPT and VT-d page table. This means if we choose B, then we need 
> to re-enable VT-d large page. This would be a huge performance impaction for 
> Xen 4.4 on using VT-d solution.
> 
> OK -- if that's the case, then it definitely tips the balance back to 
> A.  Unless Tim or Jan disagrees, can one of you two check it in?
> 
> Don't rush your judgement; but it would be nice to have this in before 
> RC4, which would mean checking it in today preferrably, or early 
> tomorrow at the latest.

That would be Tim then, as he would have to approve of it anyway.
I should also say that while I certainly understand the argumentation
above, I would still want to go this route only with the promise that
B is going to be worked on reasonably soon after the release, ideally
with the goal of backporting the changes for 4.4.1.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Tim Deegan]

At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
> >>> On 13.02.14 at 16:46, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> > On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
> >> George Dunlap wrote on 2014-02-11:
> >>> I think I got a bit distracted with the "A isn't really so bad" thing.
> >>> Actually, if the overhead of not sharing tables isn't very high, then
> >>> B isn't such a bad option.  In fact, B is what I expected Yang to
> >>> submit when he originally described the problem.
> >> Actually, the first solution came to my mind is B. Then I realized that even 
> > chose B, we still cannot track the memory updating from DMA(even with A/D 
> > bit, it still a problem). Also, considering the current usage case of log 
> > dirty in Xen(only vram tracking has problem), I though A is better.: 
> > Hypervisor only need to track the vram change. If a malicious guest try to 
> > DMA to vram range, it only crashed himself (This should be reasonable).
> >>
> >>> I was going to say, from a release perspective, B is probably the
> >>> safest option for now.  But on the other hand, if we've been testing
> >>> sharing all this time, maybe switching back over to non-sharing whole-hog has 
> > the higher risk?
> >> Another problem with B is that current VT-d large paging supporting relies on 
> > the sharing EPT and VT-d page table. This means if we choose B, then we need 
> > to re-enable VT-d large page. This would be a huge performance impaction for 
> > Xen 4.4 on using VT-d solution.
> > 
> > OK -- if that's the case, then it definitely tips the balance back to 
> > A.  Unless Tim or Jan disagrees, can one of you two check it in?
> > 
> > Don't rush your judgement; but it would be nice to have this in before 
> > RC4, which would mean checking it in today preferrably, or early 
> > tomorrow at the latest.
> 
> That would be Tim then, as he would have to approve of it anyway.

Done.

> I should also say that while I certainly understand the argumentation
> above, I would still want to go this route only with the promise that
> B is going to be worked on reasonably soon after the release, ideally
> with the goal of backporting the changes for 4.4.1.

Agreed.

Tim.

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

title 38 Implement VT-d large pages so we can avoid sharing between EPT 
and IOMMU
owner it Yang Z Zhang <yang.z.zhang@intel.com>
thanks

On 02/13/2014 04:20 PM, Tim Deegan wrote:
> At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
>>>>> On 13.02.14 at 16:46, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>>>> George Dunlap wrote on 2014-02-11:
>>>>> I think I got a bit distracted with the "A isn't really so bad" thing.
>>>>> Actually, if the overhead of not sharing tables isn't very high, then
>>>>> B isn't such a bad option.  In fact, B is what I expected Yang to
>>>>> submit when he originally described the problem.
>>>> Actually, the first solution came to my mind is B. Then I realized that even
>>> chose B, we still cannot track the memory updating from DMA(even with A/D
>>> bit, it still a problem). Also, considering the current usage case of log
>>> dirty in Xen(only vram tracking has problem), I though A is better.:
>>> Hypervisor only need to track the vram change. If a malicious guest try to
>>> DMA to vram range, it only crashed himself (This should be reasonable).
>>>>> I was going to say, from a release perspective, B is probably the
>>>>> safest option for now.  But on the other hand, if we've been testing
>>>>> sharing all this time, maybe switching back over to non-sharing whole-hog has
>>> the higher risk?
>>>> Another problem with B is that current VT-d large paging supporting relies on
>>> the sharing EPT and VT-d page table. This means if we choose B, then we need
>>> to re-enable VT-d large page. This would be a huge performance impaction for
>>> Xen 4.4 on using VT-d solution.
>>>
>>> OK -- if that's the case, then it definitely tips the balance back to
>>> A.  Unless Tim or Jan disagrees, can one of you two check it in?
>>>
>>> Don't rush your judgement; but it would be nice to have this in before
>>> RC4, which would mean checking it in today preferrably, or early
>>> tomorrow at the latest.
>> That would be Tim then, as he would have to approve of it anyway.
> Done.
>
>> I should also say that while I certainly understand the argumentation
>> above, I would still want to go this route only with the promise that
>> B is going to be worked on reasonably soon after the release, ideally
>> with the goal of backporting the changes for 4.4.1.
> Agreed.

OK -- I've retitled the bug and am going to leave it open.

  -George

Processed: Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[xen]

Processing commands for xen@bugs.xenproject.org:

> title 38 Implement VT-d large pages so we can avoid sharing between EPT 
Set title for #38 to `Implement VT-d large pages so we can avoid sharing between EPT'
> and IOMMU
Command failed: Unknown command `and'. at /srv/xen-devel-bugs/lib/emesinae/control.pl line 455, <GEN1> line 2.
Stop processing here.

Modified/created Bugs:
 - 38: http://bugs.xenproject.org/xen/bug/38

---
Xen Hypervisor Bug Tracker
See http://wiki.xen.org/wiki/Reporting_Bugs_against_Xen for information on reporting bugs
Contact xen-bugs-owner@bugs.xenproject.org with any infrastructure issues

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 13.02.14 at 17:20, Tim Deegan <tim@xen.org> wrote:
> At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
>> >>> On 13.02.14 at 16:46, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> > On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>> >> George Dunlap wrote on 2014-02-11:
>> >>> I think I got a bit distracted with the "A isn't really so bad" thing.
>> >>> Actually, if the overhead of not sharing tables isn't very high, then
>> >>> B isn't such a bad option.  In fact, B is what I expected Yang to
>> >>> submit when he originally described the problem.
>> >> Actually, the first solution came to my mind is B. Then I realized that 
> even 
>> > chose B, we still cannot track the memory updating from DMA(even with A/D 
>> > bit, it still a problem). Also, considering the current usage case of log 
>> > dirty in Xen(only vram tracking has problem), I though A is better.: 
>> > Hypervisor only need to track the vram change. If a malicious guest try to 
>> > DMA to vram range, it only crashed himself (This should be reasonable).
>> >>
>> >>> I was going to say, from a release perspective, B is probably the
>> >>> safest option for now.  But on the other hand, if we've been testing
>> >>> sharing all this time, maybe switching back over to non-sharing whole-hog has 
> 
>> > the higher risk?
>> >> Another problem with B is that current VT-d large paging supporting relies 
> on 
>> > the sharing EPT and VT-d page table. This means if we choose B, then we need 
> 
>> > to re-enable VT-d large page. This would be a huge performance impaction for 
>> > Xen 4.4 on using VT-d solution.
>> > 
>> > OK -- if that's the case, then it definitely tips the balance back to 
>> > A.  Unless Tim or Jan disagrees, can one of you two check it in?
>> > 
>> > Don't rush your judgement; but it would be nice to have this in before 
>> > RC4, which would mean checking it in today preferrably, or early 
>> > tomorrow at the latest.
>> 
>> That would be Tim then, as he would have to approve of it anyway.
> 
> Done.

Actually I'm afraid there are two problems with this patch:

For one, is enabling "global" log dirty mode still going to work
after VRAM-only mode already got enabled? I ask because the
paging_mode_log_dirty() check which paging_log_dirty_enable()
does first thing suggests otherwise to me (i.e. the now
conditional setting of all p2m entries to p2m_ram_logdirty would
seem to never get executed). IOW I would think that we're now
lacking a control operation allowing the transition from dirty VRAM
tracking mode to full log dirty mode.

And second, I have been fighting with finding both conditions
and (eventually) the root cause of a severe performance
regression (compared to 4.3.x) I'm observing on an EPT+IOMMU
system. This became _much_ worse after adding in the patch here
(while in fact I had hoped it might help with the originally observed
degradation): X startup fails due to timing out, and booting the
guest now takes about 20 minutes). I didn't find the root cause of
this yet, but meanwhile I know that
- the same isn't observable on SVM
- there's no problem when forcing the domain to use shadow
  mode
- there's no need for any device to actually be assigned to the
  guest
- the regression is very likely purely graphics related (based on
  the observation that when running something that regularly but
  not heavily updates the screen with X up, the guest consumes a
  full CPU's worth of processing power, yet when that updating
  doesn't happen, CPU consumption goes down, and it goes further
  down when shutting down X altogether - at least as log as the
  patch here doesn't get involved).
This I'm observing on a Westmere box (and I didn't notice it earlier
because that's one of those where due to a chipset erratum the
IOMMU gets turned off by default), so it's possible that this can't
be seen on more modern hardware. I'll hopefully find time today to
check this on the one newer (Sandy Bridge) box I have.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/17/2014 10:18 AM, Jan Beulich wrote:
>>>> On 13.02.14 at 17:20, Tim Deegan <tim@xen.org> wrote:
>> At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
>>>>>> On 13.02.14 at 16:46, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>>> On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>>>>> George Dunlap wrote on 2014-02-11:
>>>>>> I think I got a bit distracted with the "A isn't really so bad" thing.
>>>>>> Actually, if the overhead of not sharing tables isn't very high, then
>>>>>> B isn't such a bad option.  In fact, B is what I expected Yang to
>>>>>> submit when he originally described the problem.
>>>>> Actually, the first solution came to my mind is B. Then I realized that
>> even
>>>> chose B, we still cannot track the memory updating from DMA(even with A/D
>>>> bit, it still a problem). Also, considering the current usage case of log
>>>> dirty in Xen(only vram tracking has problem), I though A is better.:
>>>> Hypervisor only need to track the vram change. If a malicious guest try to
>>>> DMA to vram range, it only crashed himself (This should be reasonable).
>>>>>> I was going to say, from a release perspective, B is probably the
>>>>>> safest option for now.  But on the other hand, if we've been testing
>>>>>> sharing all this time, maybe switching back over to non-sharing whole-hog has
>>>> the higher risk?
>>>>> Another problem with B is that current VT-d large paging supporting relies
>> on
>>>> the sharing EPT and VT-d page table. This means if we choose B, then we need
>>>> to re-enable VT-d large page. This would be a huge performance impaction for
>>>> Xen 4.4 on using VT-d solution.
>>>>
>>>> OK -- if that's the case, then it definitely tips the balance back to
>>>> A.  Unless Tim or Jan disagrees, can one of you two check it in?
>>>>
>>>> Don't rush your judgement; but it would be nice to have this in before
>>>> RC4, which would mean checking it in today preferrably, or early
>>>> tomorrow at the latest.
>>> That would be Tim then, as he would have to approve of it anyway.
>> Done.
> Actually I'm afraid there are two problems with this patch:
>
> For one, is enabling "global" log dirty mode still going to work
> after VRAM-only mode already got enabled? I ask because the
> paging_mode_log_dirty() check which paging_log_dirty_enable()
> does first thing suggests otherwise to me (i.e. the now
> conditional setting of all p2m entries to p2m_ram_logdirty would
> seem to never get executed). IOW I would think that we're now
> lacking a control operation allowing the transition from dirty VRAM
> tracking mode to full log dirty mode.

Hmm, yes, doing a code inspection, that would appear to be the case.  
This probably wouldn't be caught by osstest, because (as I understand 
it) we never attach to the display, so dirty vram tracking is probably 
never enabled.

> And second, I have been fighting with finding both conditions
> and (eventually) the root cause of a severe performance
> regression (compared to 4.3.x) I'm observing on an EPT+IOMMU
> system. This became _much_ worse after adding in the patch here
> (while in fact I had hoped it might help with the originally observed
> degradation): X startup fails due to timing out, and booting the
> guest now takes about 20 minutes). I didn't find the root cause of
> this yet, but meanwhile I know that
> - the same isn't observable on SVM
> - there's no problem when forcing the domain to use shadow
>    mode
> - there's no need for any device to actually be assigned to the
>    guest
> - the regression is very likely purely graphics related (based on
>    the observation that when running something that regularly but
>    not heavily updates the screen with X up, the guest consumes a
>    full CPU's worth of processing power, yet when that updating
>    doesn't happen, CPU consumption goes down, and it goes further
>    down when shutting down X altogether - at least as log as the
>    patch here doesn't get involved).
> This I'm observing on a Westmere box (and I didn't notice it earlier
> because that's one of those where due to a chipset erratum the
> IOMMU gets turned off by default), so it's possible that this can't
> be seen on more modern hardware. I'll hopefully find time today to
> check this on the one newer (Sandy Bridge) box I have.

So you're saying that the slowdown happens if you have EPT+IOMMU, but 
*not* if you have EPT alone (IOMMU disabled), or shadow + IOMMU?

I have an issue I haven't had time to look into where windows installs 
are sometimes terribly slow on my Nehalem box; but it seems to be only 
with qemu-xen, not qemu-traditional.  I haven't tried with shadow.

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 17.02.14 at 13:23, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/17/2014 10:18 AM, Jan Beulich wrote:
>> And second, I have been fighting with finding both conditions
>> and (eventually) the root cause of a severe performance
>> regression (compared to 4.3.x) I'm observing on an EPT+IOMMU
>> system. This became _much_ worse after adding in the patch here
>> (while in fact I had hoped it might help with the originally observed
>> degradation): X startup fails due to timing out, and booting the
>> guest now takes about 20 minutes). I didn't find the root cause of
>> this yet, but meanwhile I know that
>> - the same isn't observable on SVM
>> - there's no problem when forcing the domain to use shadow
>>    mode
>> - there's no need for any device to actually be assigned to the
>>    guest
>> - the regression is very likely purely graphics related (based on
>>    the observation that when running something that regularly but
>>    not heavily updates the screen with X up, the guest consumes a
>>    full CPU's worth of processing power, yet when that updating
>>    doesn't happen, CPU consumption goes down, and it goes further
>>    down when shutting down X altogether - at least as log as the
>>    patch here doesn't get involved).
>> This I'm observing on a Westmere box (and I didn't notice it earlier
>> because that's one of those where due to a chipset erratum the
>> IOMMU gets turned off by default), so it's possible that this can't
>> be seen on more modern hardware. I'll hopefully find time today to
>> check this on the one newer (Sandy Bridge) box I have.
> 
> So you're saying that the slowdown happens if you have EPT+IOMMU, but 
> *not* if you have EPT alone (IOMMU disabled), or shadow + IOMMU?
> 
> I have an issue I haven't had time to look into where windows installs 
> are sometimes terribly slow on my Nehalem box; but it seems to be only 
> with qemu-xen, not qemu-traditional.  I haven't tried with shadow.

Sorry I forgot to mention this - I too suspected the qemu version
update to be one possible reason, but I'm seeing the same behavior
with qemu-trad.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/17/2014 10:18 AM, Jan Beulich wrote:
>>>> On 13.02.14 at 17:20, Tim Deegan <tim@xen.org> wrote:
>> At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
>>>>>> On 13.02.14 at 16:46, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>>> On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>>>>> George Dunlap wrote on 2014-02-11:
>>>>>> I think I got a bit distracted with the "A isn't really so bad" thing.
>>>>>> Actually, if the overhead of not sharing tables isn't very high, then
>>>>>> B isn't such a bad option.  In fact, B is what I expected Yang to
>>>>>> submit when he originally described the problem.
>>>>> Actually, the first solution came to my mind is B. Then I realized that
>> even
>>>> chose B, we still cannot track the memory updating from DMA(even with A/D
>>>> bit, it still a problem). Also, considering the current usage case of log
>>>> dirty in Xen(only vram tracking has problem), I though A is better.:
>>>> Hypervisor only need to track the vram change. If a malicious guest try to
>>>> DMA to vram range, it only crashed himself (This should be reasonable).
>>>>>> I was going to say, from a release perspective, B is probably the
>>>>>> safest option for now.  But on the other hand, if we've been testing
>>>>>> sharing all this time, maybe switching back over to non-sharing whole-hog has
>>>> the higher risk?
>>>>> Another problem with B is that current VT-d large paging supporting relies
>> on
>>>> the sharing EPT and VT-d page table. This means if we choose B, then we need
>>>> to re-enable VT-d large page. This would be a huge performance impaction for
>>>> Xen 4.4 on using VT-d solution.
>>>>
>>>> OK -- if that's the case, then it definitely tips the balance back to
>>>> A.  Unless Tim or Jan disagrees, can one of you two check it in?
>>>>
>>>> Don't rush your judgement; but it would be nice to have this in before
>>>> RC4, which would mean checking it in today preferrably, or early
>>>> tomorrow at the latest.
>>> That would be Tim then, as he would have to approve of it anyway.
>> Done.
> Actually I'm afraid there are two problems with this patch:
>
> For one, is enabling "global" log dirty mode still going to work
> after VRAM-only mode already got enabled? I ask because the
> paging_mode_log_dirty() check which paging_log_dirty_enable()
> does first thing suggests otherwise to me (i.e. the now
> conditional setting of all p2m entries to p2m_ram_logdirty would
> seem to never get executed). IOW I would think that we're now
> lacking a control operation allowing the transition from dirty VRAM
> tracking mode to full log dirty mode.

Hrm, will so far playing with this I've been unable to get a localhost 
migrate to fail with the vncviewer attached.  Which seems a bit strange...

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 17.02.14 at 15:51, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/17/2014 10:18 AM, Jan Beulich wrote:
>> Actually I'm afraid there are two problems with this patch:
>>
>> For one, is enabling "global" log dirty mode still going to work
>> after VRAM-only mode already got enabled? I ask because the
>> paging_mode_log_dirty() check which paging_log_dirty_enable()
>> does first thing suggests otherwise to me (i.e. the now
>> conditional setting of all p2m entries to p2m_ram_logdirty would
>> seem to never get executed). IOW I would think that we're now
>> lacking a control operation allowing the transition from dirty VRAM
>> tracking mode to full log dirty mode.
> 
> Hrm, will so far playing with this I've been unable to get a localhost 
> migrate to fail with the vncviewer attached.  Which seems a bit strange...

Not necessarily - it may depend on how the tools actually do this:
They might temporarily disable log dirty mode altogether, just to
re-enable full mode again right away. But this specific usage of the
hypervisor interface wouldn't (to me) mean that other tool stacks
might not be doing this differently.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-02-17:
>>>> On 17.02.14 at 15:51, George Dunlap <george.dunlap@eu.citrix.com>
> wrote:
>> On 02/17/2014 10:18 AM, Jan Beulich wrote:
>>> Actually I'm afraid there are two problems with this patch:
>>> 
>>> For one, is enabling "global" log dirty mode still going to work
>>> after VRAM-only mode already got enabled? I ask because the
>>> paging_mode_log_dirty() check which paging_log_dirty_enable() does
>>> first thing suggests otherwise to me (i.e. the now conditional
>>> setting of all p2m entries to p2m_ram_logdirty would seem to never
>>> get executed). IOW I would think that we're now lacking a control
>>> operation allowing the transition from dirty VRAM tracking mode to
>>> full log dirty mode.
>> 
>> Hrm, will so far playing with this I've been unable to get a
>> localhost migrate to fail with the vncviewer attached.  Which seems a bit strange...
> 
> Not necessarily - it may depend on how the tools actually do this:
> They might temporarily disable log dirty mode altogether, just to
> re-enable full mode again right away. But this specific usage of the
> hypervisor interface wouldn't (to me) mean that other tool stacks
> might not be doing this differently.

You are right. Before migration, libxc will disable log dirty mode if it already enabled before and re-enable it. So when I am developing this patch, I think it is ok for migration.

If there really have other tool stacks also will use this interface (Is it true?), perhaps my original patch is better which will check paging_mode_log_dirty(d) && log_global:

diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index ab5eacb..368c975 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -168,7 +168,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
 {
     int ret;
 
-    if ( paging_mode_log_dirty(d) )
+    if ( paging_mode_log_dirty(d) && !log_global )
         return -EINVAL;
 
     domain_pause(d);


> 
> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-02-17:
>>>>> On 17.02.14 at 15:51, George Dunlap <george.dunlap@eu.citrix.com>
>> wrote:
>>> On 02/17/2014 10:18 AM, Jan Beulich wrote:
>>>> Actually I'm afraid there are two problems with this patch:
>>>>
>>>> For one, is enabling "global" log dirty mode still going to work
>>>> after VRAM-only mode already got enabled? I ask because the
>>>> paging_mode_log_dirty() check which paging_log_dirty_enable() does
>>>> first thing suggests otherwise to me (i.e. the now conditional
>>>> setting of all p2m entries to p2m_ram_logdirty would seem to never
>>>> get executed). IOW I would think that we're now lacking a control
>>>> operation allowing the transition from dirty VRAM tracking mode to
>>>> full log dirty mode.
>>> Hrm, will so far playing with this I've been unable to get a
>>> localhost migrate to fail with the vncviewer attached.  Which seems a bit strange...
>> Not necessarily - it may depend on how the tools actually do this:
>> They might temporarily disable log dirty mode altogether, just to
>> re-enable full mode again right away. But this specific usage of the
>> hypervisor interface wouldn't (to me) mean that other tool stacks
>> might not be doing this differently.
> You are right. Before migration, libxc will disable log dirty mode if it already enabled before and re-enable it. So when I am developing this patch, I think it is ok for migration.
>
> If there really have other tool stacks also will use this interface (Is it true?), perhaps my original patch is better which will check paging_mode_log_dirty(d) && log_global:

It turns out that the reason I couldn't get a crash was because libxc 
was actually paying attention to the -EINVAL return value, and disabling 
and then re-enabling logdirty.  That's what would happen before your 
dirty vram patch, and that's what happens after.  And arguably, that's 
the correct behavior for any toolstack, given that the interface returns 
an error.

This patch would actually change the interface; if we check this in, 
then if you enable logdirty when dirty vram tracking is enabled, you 
*won't* get an error, and thus *won't* disable and re-enable logdirty 
mode.  So actually, this patch would be more disruptive.

Thanks for the patch, though. :-)

  -George

>
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index ab5eacb..368c975 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -168,7 +168,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
>   {
>       int ret;
>   
> -    if ( paging_mode_log_dirty(d) )
> +    if ( paging_mode_log_dirty(d) && !log_global )
>           return -EINVAL;
>   
>       domain_pause(d);
>
>
>> Jan
>
> Best regards,
> Yang
>
>

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

George Dunlap wrote on 2014-02-18:
> On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
>> Jan Beulich wrote on 2014-02-17:
>>>>>> On 17.02.14 at 15:51, George Dunlap
>>>>>> <george.dunlap@eu.citrix.com>
>>> wrote:
>>>> On 02/17/2014 10:18 AM, Jan Beulich wrote:
>>>>> Actually I'm afraid there are two problems with this patch:
>>>>> 
>>>>> For one, is enabling "global" log dirty mode still going to work
>>>>> after VRAM-only mode already got enabled? I ask because the
>>>>> paging_mode_log_dirty() check which paging_log_dirty_enable()
>>>>> does first thing suggests otherwise to me (i.e. the now
>>>>> conditional setting of all p2m entries to p2m_ram_logdirty would
>>>>> seem to never get executed). IOW I would think that we're now
>>>>> lacking a control operation allowing the transition from dirty
>>>>> VRAM tracking mode to full log dirty mode.
>>>> Hrm, will so far playing with this I've been unable to get a
>>>> localhost migrate to fail with the vncviewer attached.  Which
>>>> seems a bit
> strange...
>>> Not necessarily - it may depend on how the tools actually do this:
>>> They might temporarily disable log dirty mode altogether, just to
>>> re-enable full mode again right away. But this specific usage of
>>> the hypervisor interface wouldn't (to me) mean that other tool
>>> stacks might not be doing this differently.
>> You are right. Before migration, libxc will disable log dirty mode
>> if it already
> enabled before and re-enable it. So when I am developing this patch, I
> think it is ok for migration.
>> 
>> If there really have other tool stacks also will use this interface
>> (Is it true?),
> perhaps my original patch is better which will check
> paging_mode_log_dirty(d) && log_global:
> 
> It turns out that the reason I couldn't get a crash was because libxc
> was actually paying attention to the -EINVAL return value, and
> disabling and then re-enabling logdirty.  That's what would happen
> before your dirty vram patch, and that's what happens after.  And
> arguably, that's the correct behavior for any toolstack, given that the interface returns an error.

Agree.

> 
> This patch would actually change the interface; if we check this in,
> then if you enable logdirty when dirty vram tracking is enabled, you
> *won't* get an error, and thus *won't* disable and re-enable logdirty mode.
> So actually, this patch would be more disruptive.
> 

Jan, do you have any comment? 

> Thanks for the patch, though. :-)
> 
>   -George
>> 
>> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
>> index
>> ab5eacb..368c975 100644
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -168,7 +168,7 @@ int paging_log_dirty_enable(struct domain *d,
> bool_t log_global)
>>   {
>>       int ret;
>> -    if ( paging_mode_log_dirty(d) )
>> +    if ( paging_mode_log_dirty(d) && !log_global )
>>           return -EINVAL;
>>       domain_pause(d);
>> 
>>> Jan
>> 
>> Best regards,
>> Yang
>> 
>>


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 19.02.14 at 02:28, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> George Dunlap wrote on 2014-02-18:
>> On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
>> perhaps my original patch is better which will check
>> paging_mode_log_dirty(d) && log_global:
>> 
>> It turns out that the reason I couldn't get a crash was because libxc
>> was actually paying attention to the -EINVAL return value, and
>> disabling and then re-enabling logdirty.  That's what would happen
>> before your dirty vram patch, and that's what happens after.  And
>> arguably, that's the correct behavior for any toolstack, given that the 
> interface returns an error.
> 
> Agree.
> 
>> 
>> This patch would actually change the interface; if we check this in,
>> then if you enable logdirty when dirty vram tracking is enabled, you
>> *won't* get an error, and thus *won't* disable and re-enable logdirty mode.
>> So actually, this patch would be more disruptive.
>> 
> 
> Jan, do you have any comment? 

This simplistic variant is just calling for problems. As was already
said elsewhere on this thread, we should simply do the mode change
properly: Track that a partial log-dirty mode is in use, and allow
switching to global log-dirty mode (converting all entries to R/O).

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/19/2014 08:55 AM, Jan Beulich wrote:
>>>> On 19.02.14 at 02:28, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> George Dunlap wrote on 2014-02-18:
>>> On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
>>> perhaps my original patch is better which will check
>>> paging_mode_log_dirty(d) && log_global:
>>>
>>> It turns out that the reason I couldn't get a crash was because libxc
>>> was actually paying attention to the -EINVAL return value, and
>>> disabling and then re-enabling logdirty.  That's what would happen
>>> before your dirty vram patch, and that's what happens after.  And
>>> arguably, that's the correct behavior for any toolstack, given that the
>> interface returns an error.
>>
>> Agree.
>>
>>> This patch would actually change the interface; if we check this in,
>>> then if you enable logdirty when dirty vram tracking is enabled, you
>>> *won't* get an error, and thus *won't* disable and re-enable logdirty mode.
>>> So actually, this patch would be more disruptive.
>>>
>> Jan, do you have any comment?
> This simplistic variant is just calling for problems. As was already
> said elsewhere on this thread, we should simply do the mode change
> properly: Track that a partial log-dirty mode is in use, and allow
> switching to global log-dirty mode (converting all entries to R/O).

I think Yang was asking you for your opinion on my suggestion that 
nothing actually needed to be done.  Enabling full logdirty mode for 
migration when dirty vram tracking was enabled has *always* returned an 
error (or at least for a long time now), and *always* resulted in the 
toolstack disabling and re-enabling logdirty mode; Yang's patch doesn't 
change that at all.

If you think that's an interface we need to improve in the future, we 
can put it on the list of improvements.  But at this point it seems to 
me more like a nice-to-have.

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 19.02.14 at 12:03, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/19/2014 08:55 AM, Jan Beulich wrote:
>>>>> On 19.02.14 at 02:28, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> George Dunlap wrote on 2014-02-18:
>>>> On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
>>>> perhaps my original patch is better which will check
>>>> paging_mode_log_dirty(d) && log_global:
>>>>
>>>> It turns out that the reason I couldn't get a crash was because libxc
>>>> was actually paying attention to the -EINVAL return value, and
>>>> disabling and then re-enabling logdirty.  That's what would happen
>>>> before your dirty vram patch, and that's what happens after.  And
>>>> arguably, that's the correct behavior for any toolstack, given that the
>>> interface returns an error.
>>>
>>> Agree.
>>>
>>>> This patch would actually change the interface; if we check this in,
>>>> then if you enable logdirty when dirty vram tracking is enabled, you
>>>> *won't* get an error, and thus *won't* disable and re-enable logdirty mode.
>>>> So actually, this patch would be more disruptive.
>>>>
>>> Jan, do you have any comment?
>> This simplistic variant is just calling for problems. As was already
>> said elsewhere on this thread, we should simply do the mode change
>> properly: Track that a partial log-dirty mode is in use, and allow
>> switching to global log-dirty mode (converting all entries to R/O).
> 
> I think Yang was asking you for your opinion on my suggestion that 
> nothing actually needed to be done.  Enabling full logdirty mode for 
> migration when dirty vram tracking was enabled has *always* returned an 
> error (or at least for a long time now), and *always* resulted in the 
> toolstack disabling and re-enabling logdirty mode; Yang's patch doesn't 
> change that at all.
> 
> If you think that's an interface we need to improve in the future, we 
> can put it on the list of improvements.  But at this point it seems to 
> me more like a nice-to-have.

I agree - for 4.4.0 we shouldn't need any further adjustments. And
I hoped to imply that I don't see a need for this incremental change
to go in by having said "This simplistic variant is just calling for
problems".

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 02/19/2014 11:13 AM, Jan Beulich wrote:
>>>> On 19.02.14 at 12:03, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 02/19/2014 08:55 AM, Jan Beulich wrote:
>>>>>> On 19.02.14 at 02:28, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>>> George Dunlap wrote on 2014-02-18:
>>>>> On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
>>>>> perhaps my original patch is better which will check
>>>>> paging_mode_log_dirty(d) && log_global:
>>>>>
>>>>> It turns out that the reason I couldn't get a crash was because libxc
>>>>> was actually paying attention to the -EINVAL return value, and
>>>>> disabling and then re-enabling logdirty.  That's what would happen
>>>>> before your dirty vram patch, and that's what happens after.  And
>>>>> arguably, that's the correct behavior for any toolstack, given that the
>>>> interface returns an error.
>>>>
>>>> Agree.
>>>>
>>>>> This patch would actually change the interface; if we check this in,
>>>>> then if you enable logdirty when dirty vram tracking is enabled, you
>>>>> *won't* get an error, and thus *won't* disable and re-enable logdirty mode.
>>>>> So actually, this patch would be more disruptive.
>>>>>
>>>> Jan, do you have any comment?
>>> This simplistic variant is just calling for problems. As was already
>>> said elsewhere on this thread, we should simply do the mode change
>>> properly: Track that a partial log-dirty mode is in use, and allow
>>> switching to global log-dirty mode (converting all entries to R/O).
>> I think Yang was asking you for your opinion on my suggestion that
>> nothing actually needed to be done.  Enabling full logdirty mode for
>> migration when dirty vram tracking was enabled has *always* returned an
>> error (or at least for a long time now), and *always* resulted in the
>> toolstack disabling and re-enabling logdirty mode; Yang's patch doesn't
>> change that at all.
>>
>> If you think that's an interface we need to improve in the future, we
>> can put it on the list of improvements.  But at this point it seems to
>> me more like a nice-to-have.
> I agree - for 4.4.0 we shouldn't need any further adjustments. And
> I hoped to imply that I don't see a need for this incremental change
> to go in by having said "This simplistic variant is just calling for
> problems".

No, but "we should simply do the mode change properly" could be 
interpreted as saying, "this needs to be done as a follow-up to the 
dirty vram tracking patch"; someone might even interpret it as, "you 
need to do this as a follow-up".  That's what I was trying to clarify / 
express an opinion on. :-)

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 17.02.14 at 11:18, "Jan Beulich" <JBeulich@suse.com> wrote:
> And second, I have been fighting with finding both conditions
> and (eventually) the root cause of a severe performance
> regression (compared to 4.3.x) I'm observing on an EPT+IOMMU
> system. This became _much_ worse after adding in the patch here
> (while in fact I had hoped it might help with the originally observed
> degradation): X startup fails due to timing out, and booting the
> guest now takes about 20 minutes). I didn't find the root cause of
> this yet, but meanwhile I know that
> - the same isn't observable on SVM
> - there's no problem when forcing the domain to use shadow
>   mode
> - there's no need for any device to actually be assigned to the
>   guest
> - the regression is very likely purely graphics related (based on
>   the observation that when running something that regularly but
>   not heavily updates the screen with X up, the guest consumes a
>   full CPU's worth of processing power, yet when that updating
>   doesn't happen, CPU consumption goes down, and it goes further
>   down when shutting down X altogether - at least as log as the
>   patch here doesn't get involved).
> This I'm observing on a Westmere box (and I didn't notice it earlier
> because that's one of those where due to a chipset erratum the
> IOMMU gets turned off by default), so it's possible that this can't
> be seen on more modern hardware. I'll hopefully find time today to
> check this on the one newer (Sandy Bridge) box I have.

Just got done with trying this: By default, things work fine there.
As soon as I use "iommu=no-snoop", things go bad (even worse
than one the older box - the guest is consuming about 2.5 CPUs
worth of processing power _without_ the patch here in use, so I
don't even want to think about trying it there); I guessed that to
be another of the potential sources of the problem since on that
older box the respective hardware feature is unavailable.

While I'll try to look into this further, I guess I have to defer to our
VT-d specialists at Intel at this point...

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-02-17:
>>>> On 17.02.14 at 11:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>> And second, I have been fighting with finding both conditions and
>> (eventually) the root cause of a severe performance regression
>> (compared to 4.3.x) I'm observing on an EPT+IOMMU system. This
>> became _much_ worse after adding in the patch here (while in fact I
>> had hoped it might help with the originally observed
>> degradation): X startup fails due to timing out, and booting the
>> guest now takes about 20 minutes). I didn't find the root cause of
>> this yet, but meanwhile I know that
>> - the same isn't observable on SVM
>> - there's no problem when forcing the domain to use shadow
>>   mode - there's no need for any device to actually be assigned to the
>>   guest - the regression is very likely purely graphics related (based
>>   on the observation that when running something that regularly but not
>>   heavily updates the screen with X up, the guest consumes a full CPU's
>>   worth of processing power, yet when that updating doesn't happen, CPU
>>   consumption goes down, and it goes further down when shutting down X
>>   altogether - at least as log as the patch here doesn't get involved).
>> This I'm observing on a Westmere box (and I didn't notice it earlier
>> because that's one of those where due to a chipset erratum the IOMMU
>> gets turned off by default), so it's possible that this can't be
>> seen on more modern hardware. I'll hopefully find time today to
>> check this on the one newer (Sandy Bridge) box I have.
> 
> Just got done with trying this: By default, things work fine there.
> As soon as I use "iommu=no-snoop", things go bad (even worse than one
> the older box - the guest is consuming about 2.5 CPUs worth of
> processing power _without_ the patch here in use, so I don't even want
> to think about trying it there); I guessed that to be another of the
> potential sources of the problem since on that older box the respective hardware feature is unavailable.
> 
> While I'll try to look into this further, I guess I have to defer to
> our VT-d specialists at Intel at this point...
> 

Hi, Jan,

I tried to reproduce it. But unfortunately, I cannot reproduce it in my box (sandy bridge EP)with latest Xen(include my patch). I guess my configuration or steps may wrong, here is mine:

1. add iommu=1,no-snoop in by xen cmd line:
(XEN) Intel VT-d Snoop Control not enabled.
(XEN) Intel VT-d Dom0 DMA Passthrough not enabled.
(XEN) Intel VT-d Queued Invalidation enabled.
(XEN) Intel VT-d Interrupt Remapping enabled.
(XEN) Intel VT-d Shared EPT tables enabled.

2. boot a rhel6u4 guest.

3. after guest boot up, run startx inside guest.

4. a few second, the X windows shows and didn't see any error. Also the CPU utilization is about 1.7%.

Any thing wrong?

> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 18.02.14 at 04:25, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> I tried to reproduce it. But unfortunately, I cannot reproduce it in my box 
> (sandy bridge EP)with latest Xen(include my patch). I guess my configuration 
> or steps may wrong, here is mine:
> 
> 1. add iommu=1,no-snoop in by xen cmd line:
> (XEN) Intel VT-d Snoop Control not enabled.
> (XEN) Intel VT-d Dom0 DMA Passthrough not enabled.
> (XEN) Intel VT-d Queued Invalidation enabled.
> (XEN) Intel VT-d Interrupt Remapping enabled.
> (XEN) Intel VT-d Shared EPT tables enabled.
> 
> 2. boot a rhel6u4 guest.
> 
> 3. after guest boot up, run startx inside guest.
> 
> 4. a few second, the X windows shows and didn't see any error. Also the CPU 
> utilization is about 1.7%.
> 
> Any thing wrong?

Nothing I can see. The main difference might be that I'm using a
SLES11 SP3 guest instead of a RHEL one.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 18.02.14 at 04:25, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-02-17:
>>>>> On 17.02.14 at 11:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> And second, I have been fighting with finding both conditions and
>>> (eventually) the root cause of a severe performance regression
>>> (compared to 4.3.x) I'm observing on an EPT+IOMMU system. This
>>> became _much_ worse after adding in the patch here (while in fact I
>>> had hoped it might help with the originally observed
>>> degradation): X startup fails due to timing out, and booting the
>>> guest now takes about 20 minutes). I didn't find the root cause of
>>> this yet, but meanwhile I know that
>>> - the same isn't observable on SVM
>>> - there's no problem when forcing the domain to use shadow
>>>   mode - there's no need for any device to actually be assigned to the
>>>   guest - the regression is very likely purely graphics related (based
>>>   on the observation that when running something that regularly but not
>>>   heavily updates the screen with X up, the guest consumes a full CPU's
>>>   worth of processing power, yet when that updating doesn't happen, CPU
>>>   consumption goes down, and it goes further down when shutting down X
>>>   altogether - at least as log as the patch here doesn't get involved).
>>> This I'm observing on a Westmere box (and I didn't notice it earlier
>>> because that's one of those where due to a chipset erratum the IOMMU
>>> gets turned off by default), so it's possible that this can't be
>>> seen on more modern hardware. I'll hopefully find time today to
>>> check this on the one newer (Sandy Bridge) box I have.
>> 
>> Just got done with trying this: By default, things work fine there.
>> As soon as I use "iommu=no-snoop", things go bad (even worse than one
>> the older box - the guest is consuming about 2.5 CPUs worth of
>> processing power _without_ the patch here in use, so I don't even want
>> to think about trying it there); I guessed that to be another of the
>> potential sources of the problem since on that older box the respective 
> hardware feature is unavailable.
>> 
>> While I'll try to look into this further, I guess I have to defer to
>> our VT-d specialists at Intel at this point...
>> 
> 
> Hi, Jan,
> 
> I tried to reproduce it. But unfortunately, I cannot reproduce it in my box 
> (sandy bridge EP)with latest Xen(include my patch). I guess my configuration 
> or steps may wrong, here is mine:
> 
> 1. add iommu=1,no-snoop in by xen cmd line:
> (XEN) Intel VT-d Snoop Control not enabled.
> (XEN) Intel VT-d Dom0 DMA Passthrough not enabled.
> (XEN) Intel VT-d Queued Invalidation enabled.
> (XEN) Intel VT-d Interrupt Remapping enabled.
> (XEN) Intel VT-d Shared EPT tables enabled.
> 
> 2. boot a rhel6u4 guest.
> 
> 3. after guest boot up, run startx inside guest.
> 
> 4. a few second, the X windows shows and didn't see any error. Also the CPU 
> utilization is about 1.7%.
> 
> Any thing wrong?

Nothing at all, as it turns out. The regression is due to Dongxiao's

http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg00367.html

which I have in my tree as part of various things pending for 4.5.
And which at the first, second, and third glance looks pretty
innocent (IOW I still have to find out _why_ it is wrong).

In any case - I'm very sorry for the false alarm.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2808 bytes --]

>>> On 18.02.14 at 12:46, "Jan Beulich" <JBeulich@suse.com> wrote:
> Nothing at all, as it turns out. The regression is due to Dongxiao's
> 
> http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg00367.html 
> 
> which I have in my tree as part of various things pending for 4.5.
> And which at the first, second, and third glance looks pretty
> innocent (IOW I still have to find out _why_ it is wrong).

And here's a fixed version of the patch - we simply can't drop
the bogus HVM_PARAM_IDENT_PT check entirely yet.

In the course of fixing this I also found two other shortcomings:
- EPT EMT field should be updated upon guest MTRR writes (the
  lack thereof is the reason for needing to retain the bogus check)
- epte_get_entry_emt() either needs "order" passed, or its callers
  must call it more than once for big/huge pages

Jan

x86/hvm: refine the judgment on IDENT_PT for EMT

When trying to get the EPT EMT type, the judgment on
HVM_PARAM_IDENT_PT is not correct which always returns WB type if
the parameter is not set. Remove the related code.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

We can't fully drop the dependency yet, but we should certainly avoid
overriding cases already properly handled. The reason for this is that
the guest setting up its MTRRs happens _after_ the EPT tables got
already constructed, and no code is in place to propagate this to the
EPT code. Without this check we're forcing the guest to run with all of
its memory uncachable until something happens to re-write every single
EPT entry. But of course this has to be just a temporary solution.

In the same spirit we should defer the "very early" (when the guest is
still being constructed and has no vCPU yet) override to the last
possible point.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -689,13 +689,8 @@ uint8_t epte_get_entry_emt(struct domain
 
     *ipat = 0;
 
-    if ( (current->domain != d) &&
-         ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) )
-        return MTRR_TYPE_WRBACK;
-
-    if ( !is_pvh_vcpu(v) &&
-         !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
-        return MTRR_TYPE_WRBACK;
+    if ( v->domain != d )
+        v = d->vcpu ? d->vcpu[0] : NULL;
 
     if ( !mfn_valid(mfn_x(mfn)) )
         return MTRR_TYPE_UNCACHABLE;
@@ -718,7 +713,8 @@ uint8_t epte_get_entry_emt(struct domain
         return MTRR_TYPE_WRBACK;
     }
 
-    gmtrr_mtype = is_hvm_vcpu(v) ?
+    gmtrr_mtype = is_hvm_domain(d) && v &&
+                  d->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] ?
                   get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT)) :
                   MTRR_TYPE_WRBACK;
 





[-- Attachment #2: EPT-ignore-IDENT_PT --]
[-- Type: application/octet-stream, Size: 1883 bytes --]

x86/hvm: refine the judgment on IDENT_PT for EMT

When trying to get the EPT EMT type, the judgment on
HVM_PARAM_IDENT_PT is not correct which always returns WB type if
the parameter is not set. Remove the related code.

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

We can't fully drop the dependency yet, but we should certainly avoid
overriding cases already properly handled. The reason for this is that
the guest setting up its MTRRs happens _after_ the EPT tables got
already constructed, and no code is in place to propagate this to the
EPT code. Without this check we're forcing the guest to run with all of
its memory uncachable until something happens to re-write every single
EPT entry. But of course this has to be just a temporary solution.

In the same spirit we should defer the "very early" (when the guest is
still being constructed and has no vCPU yet) override to the last
possible point.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -689,13 +689,8 @@ uint8_t epte_get_entry_emt(struct domain
 
     *ipat = 0;
 
-    if ( (current->domain != d) &&
-         ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) )
-        return MTRR_TYPE_WRBACK;
-
-    if ( !is_pvh_vcpu(v) &&
-         !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
-        return MTRR_TYPE_WRBACK;
+    if ( v->domain != d )
+        v = d->vcpu ? d->vcpu[0] : NULL;
 
     if ( !mfn_valid(mfn_x(mfn)) )
         return MTRR_TYPE_UNCACHABLE;
@@ -718,7 +713,8 @@ uint8_t epte_get_entry_emt(struct domain
         return MTRR_TYPE_WRBACK;
     }
 
-    gmtrr_mtype = is_hvm_vcpu(v) ?
+    gmtrr_mtype = is_hvm_domain(d) && v &&
+                  d->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] ?
                   get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT)) :
                   MTRR_TYPE_WRBACK;
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Xu, Dongxiao]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, February 18, 2014 11:29 PM
> To: Xu, Dongxiao; Zhang, Xiantao; Zhang, Yang Z
> Cc: andrew.cooper3@citrix.com; George Dunlap; Dugger, Donald D;
> xen-devel@lists.xen.org; Tim Deegan
> Subject: Re: [Xen-devel] [PATCH] Don't track all memory when enabling log dirty
> to track vram
> 
> >>> On 18.02.14 at 12:46, "Jan Beulich" <JBeulich@suse.com> wrote:
> > Nothing at all, as it turns out. The regression is due to Dongxiao's
> >
> > http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg00367.html
> >
> > which I have in my tree as part of various things pending for 4.5.
> > And which at the first, second, and third glance looks pretty
> > innocent (IOW I still have to find out _why_ it is wrong).
> 
> And here's a fixed version of the patch - we simply can't drop
> the bogus HVM_PARAM_IDENT_PT check entirely yet.
> 
> In the course of fixing this I also found two other shortcomings:
> - EPT EMT field should be updated upon guest MTRR writes (the
>   lack thereof is the reason for needing to retain the bogus check)
> - epte_get_entry_emt() either needs "order" passed, or its callers
>   must call it more than once for big/huge pages
> 

The below patch looks okay to me. Thanks!

Thanks,
Dongxiao

> Jan
> 
> x86/hvm: refine the judgment on IDENT_PT for EMT
> 
> When trying to get the EPT EMT type, the judgment on
> HVM_PARAM_IDENT_PT is not correct which always returns WB type if
> the parameter is not set. Remove the related code.
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> 
> We can't fully drop the dependency yet, but we should certainly avoid
> overriding cases already properly handled. The reason for this is that
> the guest setting up its MTRRs happens _after_ the EPT tables got
> already constructed, and no code is in place to propagate this to the
> EPT code. Without this check we're forcing the guest to run with all of
> its memory uncachable until something happens to re-write every single
> EPT entry. But of course this has to be just a temporary solution.
> 
> In the same spirit we should defer the "very early" (when the guest is
> still being constructed and has no vCPU yet) override to the last
> possible point.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -689,13 +689,8 @@ uint8_t epte_get_entry_emt(struct domain
> 
>      *ipat = 0;
> 
> -    if ( (current->domain != d) &&
> -         ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) )
> -        return MTRR_TYPE_WRBACK;
> -
> -    if ( !is_pvh_vcpu(v) &&
> -         !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> -        return MTRR_TYPE_WRBACK;
> +    if ( v->domain != d )
> +        v = d->vcpu ? d->vcpu[0] : NULL;
> 
>      if ( !mfn_valid(mfn_x(mfn)) )
>          return MTRR_TYPE_UNCACHABLE;
> @@ -718,7 +713,8 @@ uint8_t epte_get_entry_emt(struct domain
>          return MTRR_TYPE_WRBACK;
>      }
> 
> -    gmtrr_mtype = is_hvm_vcpu(v) ?
> +    gmtrr_mtype = is_hvm_domain(d) && v &&
> +                  d->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] ?
>                    get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn <<
> PAGE_SHIFT)) :
>                    MTRR_TYPE_WRBACK;
> 
> 
> 
> 

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-02-18:
>>>> On 18.02.14 at 04:25, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-02-17:
>>>>>> On 17.02.14 at 11:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> And second, I have been fighting with finding both conditions and
>>>> (eventually) the root cause of a severe performance regression
>>>> (compared to 4.3.x) I'm observing on an EPT+IOMMU system. This
>>>> became _much_ worse after adding in the patch here (while in fact
>>>> I had hoped it might help with the originally observed
>>>> degradation): X startup fails due to timing out, and booting the
>>>> guest now takes about 20 minutes). I didn't find the root cause of
>>>> this yet, but meanwhile I know that
>>>> - the same isn't observable on SVM
>>>> - there's no problem when forcing the domain to use shadow
>>>>   mode - there's no need for any device to actually be assigned to the
>>>>   guest - the regression is very likely purely graphics related (based
>>>>   on the observation that when running something that regularly but not
>>>>   heavily updates the screen with X up, the guest consumes a full CPU's
>>>>   worth of processing power, yet when that updating doesn't
>>>> happen,
> CPU
>>>>   consumption goes down, and it goes further down when shutting
>>>> down
> X
>>>>   altogether - at least as log as the patch here doesn't get involved).
>>>> This I'm observing on a Westmere box (and I didn't notice it
>>>> earlier because that's one of those where due to a chipset erratum
>>>> the IOMMU gets turned off by default), so it's possible that this
>>>> can't be seen on more modern hardware. I'll hopefully find time
>>>> today to check this on the one newer (Sandy Bridge) box I have.
>>> 
>>> Just got done with trying this: By default, things work fine there. As
>>> soon as I use "iommu=no-snoop", things go bad (even worse than one the
>>> older box - the guest is consuming about 2.5 CPUs worth of processing
>>> power _without_ the patch here in use, so I don't even want to think
>>> about trying it there); I guessed that to be another of the potential
>>> sources of the problem since on that older box the respective hardware
>>> feature is unavailable.
>>> 
>>> While I'll try to look into this further, I guess I have to defer
>>> to our VT-d specialists at Intel at this point...
>>> 
>> 
>> Hi, Jan,
>> 
>> I tried to reproduce it. But unfortunately, I cannot reproduce it in
>> my box (sandy bridge EP)with latest Xen(include my patch). I guess
>> my configuration or steps may wrong, here is mine:
>> 
>> 1. add iommu=1,no-snoop in by xen cmd line:
>> (XEN) Intel VT-d Snoop Control not enabled.
>> (XEN) Intel VT-d Dom0 DMA Passthrough not enabled.
>> (XEN) Intel VT-d Queued Invalidation enabled.
>> (XEN) Intel VT-d Interrupt Remapping enabled.
>> (XEN) Intel VT-d Shared EPT tables enabled.
>> 
>> 2. boot a rhel6u4 guest.
>> 
>> 3. after guest boot up, run startx inside guest.
>> 
>> 4. a few second, the X windows shows and didn't see any error. Also
>> the CPU utilization is about 1.7%.
>> 
>> Any thing wrong?
> 
> Nothing at all, as it turns out. The regression is due to Dongxiao's
> 
> http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg00367.h
> tml
> 
> which I have in my tree as part of various things pending for 4.5.
> And which at the first, second, and third glance looks pretty innocent
> (IOW I still have to find out _why_ it is wrong).
> 
> In any case - I'm very sorry for the false alarm.
> 

It doesn't matter. Conversely, we need to thank you for helping us to fix this issue. :)

BTW, I still cannot reproduce it in my box, even I uses SLES 11 SP3 as guest.

> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 19.02.14 at 02:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-02-18:
>>>>> On 18.02.14 at 04:25, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-02-17:
>>>>>>> On 17.02.14 at 11:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> And second, I have been fighting with finding both conditions and
>>>>> (eventually) the root cause of a severe performance regression
>>>>> (compared to 4.3.x) I'm observing on an EPT+IOMMU system. This
>>>>> became _much_ worse after adding in the patch here (while in fact
>>>>> I had hoped it might help with the originally observed
>>>>> degradation): X startup fails due to timing out, and booting the
>>>>> guest now takes about 20 minutes). I didn't find the root cause of
>>>>> this yet, but meanwhile I know that
>>>>> - the same isn't observable on SVM
>>>>> - there's no problem when forcing the domain to use shadow
>>>>>   mode - there's no need for any device to actually be assigned to the
>>>>>   guest - the regression is very likely purely graphics related (based
>>>>>   on the observation that when running something that regularly but not
>>>>>   heavily updates the screen with X up, the guest consumes a full CPU's
>>>>>   worth of processing power, yet when that updating doesn't
>>>>> happen,
>> CPU
>>>>>   consumption goes down, and it goes further down when shutting
>>>>> down
>> X
>>>>>   altogether - at least as log as the patch here doesn't get involved).
>>>>> This I'm observing on a Westmere box (and I didn't notice it
>>>>> earlier because that's one of those where due to a chipset erratum
>>>>> the IOMMU gets turned off by default), so it's possible that this
>>>>> can't be seen on more modern hardware. I'll hopefully find time
>>>>> today to check this on the one newer (Sandy Bridge) box I have.
>>>> 
>>>> Just got done with trying this: By default, things work fine there. As
>>>> soon as I use "iommu=no-snoop", things go bad (even worse than one the
>>>> older box - the guest is consuming about 2.5 CPUs worth of processing
>>>> power _without_ the patch here in use, so I don't even want to think
>>>> about trying it there); I guessed that to be another of the potential
>>>> sources of the problem since on that older box the respective hardware
>>>> feature is unavailable.
>>>> 
>>>> While I'll try to look into this further, I guess I have to defer
>>>> to our VT-d specialists at Intel at this point...
>>>> 
>>> 
>>> Hi, Jan,
>>> 
>>> I tried to reproduce it. But unfortunately, I cannot reproduce it in
>>> my box (sandy bridge EP)with latest Xen(include my patch). I guess
>>> my configuration or steps may wrong, here is mine:
>>> 
>>> 1. add iommu=1,no-snoop in by xen cmd line:
>>> (XEN) Intel VT-d Snoop Control not enabled.
>>> (XEN) Intel VT-d Dom0 DMA Passthrough not enabled.
>>> (XEN) Intel VT-d Queued Invalidation enabled.
>>> (XEN) Intel VT-d Interrupt Remapping enabled.
>>> (XEN) Intel VT-d Shared EPT tables enabled.
>>> 
>>> 2. boot a rhel6u4 guest.
>>> 
>>> 3. after guest boot up, run startx inside guest.
>>> 
>>> 4. a few second, the X windows shows and didn't see any error. Also
>>> the CPU utilization is about 1.7%.
>>> 
>>> Any thing wrong?
>> 
>> Nothing at all, as it turns out. The regression is due to Dongxiao's
>> 
>> http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg00367.h 
>> tml
>> 
>> which I have in my tree as part of various things pending for 4.5.
>> And which at the first, second, and third glance looks pretty innocent
>> (IOW I still have to find out _why_ it is wrong).
>> 
>> In any case - I'm very sorry for the false alarm.
>> 
> 
> It doesn't matter. Conversely, we need to thank you for helping us to fix 
> this issue. :)
> 
> BTW, I still cannot reproduce it in my box, even I uses SLES 11 SP3 as 
> guest.

I assume you didn't pull in the broken patch - I'm sure you would
see the problem if you did.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 17.02.14 at 16:00, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 17.02.14 at 11:18, "Jan Beulich" <JBeulich@suse.com> wrote:
>> This I'm observing on a Westmere box (and I didn't notice it earlier
>> because that's one of those where due to a chipset erratum the
>> IOMMU gets turned off by default), so it's possible that this can't
>> be seen on more modern hardware. I'll hopefully find time today to
>> check this on the one newer (Sandy Bridge) box I have.
> 
> Just got done with trying this: By default, things work fine there.
> As soon as I use "iommu=no-snoop", things go bad (even worse
> than one the older box - the guest is consuming about 2.5 CPUs
> worth of processing power _without_ the patch here in use, so I
> don't even want to think about trying it there); I guessed that to
> be another of the potential sources of the problem since on that
> older box the respective hardware feature is unavailable.

That wasn't a fair comparison: The guest here had an SR-IOV NIC
assigned. Once removing that, badness goes back to about the
level I observe on the Westmere box. I'm then relatively certain
that this "extra" badness can be attributed to the excessive use of
wbinvd. Which of course puts under question what good assigning
a device does on a system without snoop control, if overall
performance gets worse rather than better.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Tim Deegan wrote on 2014-02-14:
> At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
>>>>> On 13.02.14 at 16:46, George Dunlap
>>>>> <george.dunlap@eu.citrix.com>
> wrote:
>>> On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>>>> George Dunlap wrote on 2014-02-11:
>>>>> I think I got a bit distracted with the "A isn't really so bad" thing.
>>>>> Actually, if the overhead of not sharing tables isn't very high,
>>>>> then B isn't such a bad option.  In fact, B is what I expected
>>>>> Yang to submit when he originally described the problem.
>>>> Actually, the first solution came to my mind is B. Then I
>>>> realized that even
>>> chose B, we still cannot track the memory updating from DMA(even with
>>> A/D bit, it still a problem). Also, considering the current usage case
>>> of log dirty in Xen(only vram tracking has problem), I though A is
>>> better.: Hypervisor only need to track the vram change. If a malicious
>>> guest try to DMA to vram range, it only crashed himself (This should be
> reasonable).
>>>> 
>>>>> I was going to say, from a release perspective, B is probably
>>>>> the safest option for now.  But on the other hand, if we've been
>>>>> testing sharing all this time, maybe switching back over to
>>>>> non-sharing whole-hog has
>>> the higher risk?
>>>> Another problem with B is that current VT-d large paging
>>>> supporting relies on
>>> the sharing EPT and VT-d page table. This means if we choose B,
>>> then we need to re-enable VT-d large page. This would be a huge
>>> performance impaction for Xen 4.4 on using VT-d solution.
>>> 
>>> OK -- if that's the case, then it definitely tips the balance back
>>> to A.  Unless Tim or Jan disagrees, can one of you two check it in?
>>> 
>>> Don't rush your judgement; but it would be nice to have this in
>>> before RC4, which would mean checking it in today preferrably, or
>>> early tomorrow at the latest.
>> 
>> That would be Tim then, as he would have to approve of it anyway.
> 
> Done.
> 
>> I should also say that while I certainly understand the
>> argumentation above, I would still want to go this route only with
>> the promise that B is going to be worked on reasonably soon after
>> the release, ideally with the goal of backporting the changes for 4.4.1.
> 
> Agreed.
> 
> Tim.

Hi all

Sorry to turn out this old thread. 
Because I just noticed that someone is asking when Intel will implement the VT-d page table separately. Actually, I am totally unaware it. The original issue that this patch tries to fix is the VRAM tracking which using the global log dirty mode. And I thought the best solution to fix it is in VRAM side not VT-d side. Because even use separate VT-d page table, we still cannot track the memory update from DMA. Even worse, I think two page tables introduce redundant code and maintain effort. So I wonder is it really necessary to implement the separate VT-d large page?


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 19.05.14 at 09:48, <yang.z.zhang@intel.com> wrote:
> Because I just noticed that someone is asking when Intel will implement the 
> VT-d page table separately. Actually, I am totally unaware it.

This was a request sent directly to you, so it shouldn't be a surprise.

> The original 
> issue that this patch tries to fix is the VRAM tracking which using the 
> global log dirty mode. And I thought the best solution to fix it is in VRAM 
> side not VT-d side. Because even use separate VT-d page table, we still cannot 
> track the memory update from DMA.

Correct. But at least we can avoid IOMMU faults by not marking read-
only the VRAM region. Unless the guest stores data in the VRAM region
that's not directly displayed information, but needs to be preserved
across migration, the only downside of this would be temporary screen
corruption in the guest immediately following migration. Clearly far
better than eventually turning off passed through devices due to
excessive IOMMU faults.

> Even worse, I think two page tables 
> introduce redundant code and maintain effort. So I wonder is it really 
> necessary to implement the separate VT-d large page?

Tim and I certainly think so. Andrew had a valid point in stating that
guests without the need for VRAM dirty tracking, but with assigned
PCI device(s) could still benefit from sharing page tables, so working
towards a model where this could be retained would clearly be the
optimal solution.

I wonder whether you actually too the time to go through the old
thread before writing your mail, since all you did is repeat your old
arguments, without addressing any of the reasons given why we
consider separate page tables better than shared ones.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-05-19:
>>>> On 19.05.14 at 09:48, <yang.z.zhang@intel.com> wrote:
>> Because I just noticed that someone is asking when Intel will
>> implement the VT-d page table separately. Actually, I am totally unaware it.
> 
> This was a request sent directly to you, so it shouldn't be a surprise.

Yes, but I am not buying in it since I think it not the right direction to fix this issue:

This is my original point:
Actually, the first solution came to my mind is B. Then I realized that even chose B, we still cannot track the memory updating from DMA(even with A/D bit, it still a problem). Also, considering the current usage case of log dirty in Xen(only vram tracking has problem), I though A is better.: Hypervisor only need to track the vram change. If a malicious guest try to DMA to vram range, it only crashed himself (This should be reasonable).


> 
>> The original
>> issue that this patch tries to fix is the VRAM tracking which using
>> the global log dirty mode. And I thought the best solution to fix it
>> is in VRAM side not VT-d side. Because even use separate VT-d page
>> table, we still cannot track the memory update from DMA.
> 
> Correct. But at least we can avoid IOMMU faults by not marking read-
> only the VRAM region. Unless the guest stores data in the VRAM region

It is easy to trigger an IOMMU faults by guest, like set an invalid DMA address. We cannot prevent it.

> that's not directly displayed information, but needs to be preserved
> across migration, the only downside of this would be temporary screen
> corruption in the guest immediately following migration. Clearly far
> better than eventually turning off passed through devices due to excessive IOMMU faults.
> 
>> Even worse, I think two page tables
>> introduce redundant code and maintain effort. So I wonder is it
>> really necessary to implement the separate VT-d large page?
> 
> Tim and I certainly think so. Andrew had a valid point in stating that
> guests without the need for VRAM dirty tracking, but with assigned PCI
> device(s) could still benefit from sharing page tables, so working
> towards a model where this could be retained would clearly be the optimal solution.

I still cannot understand what we want to achieve or fix by separate the page table.

> 
> I wonder whether you actually too the time to go through the old
> thread before writing your mail, since all you did is repeat your old
> arguments, without addressing any of the reasons given why we consider
> separate page tables better than shared ones.

I have gone through it many times. But I cannot find a strong reason to persuade me to do it. I think it is a 'nice to have' feature, not required. Unless if anyone can show me that current sharing mechanism do have problem and it only can be fixed by separate page table, then I think we must to do it. 

> 
> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 20.05.14 at 05:09, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-05-19:
>>>>> On 19.05.14 at 09:48, <yang.z.zhang@intel.com> wrote:
>>> Because I just noticed that someone is asking when Intel will
>>> implement the VT-d page table separately. Actually, I am totally unaware it.
>> 
>> This was a request sent directly to you, so it shouldn't be a surprise.
> 
> Yes, but I am not buying in it since I think it not the right direction to 
> fix this issue:
> 
> This is my original point:
> Actually, the first solution came to my mind is B. Then I realized that even 
> chose B, we still cannot track the memory updating from DMA(even with A/D 
> bit, it still a problem). Also, considering the current usage case of log 
> dirty in Xen(only vram tracking has problem), I though A is better.: 
> Hypervisor only need to track the vram change. If a malicious guest try to 
> DMA to vram range, it only crashed himself (This should be reasonable).

Except that the guest in no way needs to be malicious. I think you
forget that the ultimate goal of virtualization ought to be to make
guests behave the same (in terms of correctness, not generally in
terms of performance) as if run on real hardware, not matter what
they do. And DMA to VRAM wouldn't crash on real hardware (and I
can see legitimate uses of such).

>>> The original
>>> issue that this patch tries to fix is the VRAM tracking which using
>>> the global log dirty mode. And I thought the best solution to fix it
>>> is in VRAM side not VT-d side. Because even use separate VT-d page
>>> table, we still cannot track the memory update from DMA.
>> 
>> Correct. But at least we can avoid IOMMU faults by not marking read-
>> only the VRAM region. Unless the guest stores data in the VRAM region
> 
> It is easy to trigger an IOMMU faults by guest, like set an invalid DMA 
> address. We cannot prevent it.

Correct (and this btw also contradicts above spelled out goal: On
real hardware this would in the majority of cases also just cause
certain operations to become ineffectual rather than bring down
the system - we just have to be more rigid here to deal with
potential malicious guests).

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On Mon, May 19, 2014 at 8:48 AM, Zhang, Yang Z <yang.z.zhang@intel.com> wrote:
> Tim Deegan wrote on 2014-02-14:
>> At 15:55 +0000 on 13 Feb (1392303343), Jan Beulich wrote:
>>>>>> On 13.02.14 at 16:46, George Dunlap
>>>>>> <george.dunlap@eu.citrix.com>
>> wrote:
>>>> On 02/12/2014 12:53 AM, Zhang, Yang Z wrote:
>>>>> George Dunlap wrote on 2014-02-11:
>>>>>> I think I got a bit distracted with the "A isn't really so bad" thing.
>>>>>> Actually, if the overhead of not sharing tables isn't very high,
>>>>>> then B isn't such a bad option.  In fact, B is what I expected
>>>>>> Yang to submit when he originally described the problem.
>>>>> Actually, the first solution came to my mind is B. Then I
>>>>> realized that even
>>>> chose B, we still cannot track the memory updating from DMA(even with
>>>> A/D bit, it still a problem). Also, considering the current usage case
>>>> of log dirty in Xen(only vram tracking has problem), I though A is
>>>> better.: Hypervisor only need to track the vram change. If a malicious
>>>> guest try to DMA to vram range, it only crashed himself (This should be
>> reasonable).
>>>>>
>>>>>> I was going to say, from a release perspective, B is probably
>>>>>> the safest option for now.  But on the other hand, if we've been
>>>>>> testing sharing all this time, maybe switching back over to
>>>>>> non-sharing whole-hog has
>>>> the higher risk?
>>>>> Another problem with B is that current VT-d large paging
>>>>> supporting relies on
>>>> the sharing EPT and VT-d page table. This means if we choose B,
>>>> then we need to re-enable VT-d large page. This would be a huge
>>>> performance impaction for Xen 4.4 on using VT-d solution.
>>>>
>>>> OK -- if that's the case, then it definitely tips the balance back
>>>> to A.  Unless Tim or Jan disagrees, can one of you two check it in?
>>>>
>>>> Don't rush your judgement; but it would be nice to have this in
>>>> before RC4, which would mean checking it in today preferrably, or
>>>> early tomorrow at the latest.
>>>
>>> That would be Tim then, as he would have to approve of it anyway.
>>
>> Done.
>>
>>> I should also say that while I certainly understand the
>>> argumentation above, I would still want to go this route only with
>>> the promise that B is going to be worked on reasonably soon after
>>> the release, ideally with the goal of backporting the changes for 4.4.1.
>>
>> Agreed.
>>
>> Tim.
>
> Hi all
>
> Sorry to turn out this old thread.
> Because I just noticed that someone is asking when Intel will implement the VT-d page table separately. Actually, I am totally unaware it. The original issue that this patch tries to fix is the VRAM tracking which using the global log dirty mode. And I thought the best solution to fix it is in VRAM side not VT-d side. Because even use separate VT-d page table, we still cannot track the memory update from DMA. Even worse, I think two page tables introduce redundant code and maintain effort. So I wonder is it really necessary to implement the separate VT-d large page?

Yes, it does introduce redundant code.  But unfortunately, IOMMU
faults at the moment have to be considered rather risky; having on
happens risks (in order of decreasing probability / increasing
damage):
* Device stops working for that VM until an FLR (losing a lot of its state)
* The VM has to be killed
* The device stops working until a host reboot
* The host crashes

Avoiding these by "hoping" that the guest OS doesn't DMA into a video
buffer isn't really robust enough.  I think that was Tim and Jan's
primary reason for wanting the ability to have separate tables for HAP
and IOMMU.

Is that about right, Jan / Tim?

 -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 19.05.14 at 15:27, <George.Dunlap@eu.citrix.com> wrote:
> On Mon, May 19, 2014 at 8:48 AM, Zhang, Yang Z <yang.z.zhang@intel.com> wrote:
>> Because I just noticed that someone is asking when Intel will implement the 
> VT-d page table separately. Actually, I am totally unaware it. The original 
> issue that this patch tries to fix is the VRAM tracking which using the 
> global log dirty mode. And I thought the best solution to fix it is in VRAM 
> side not VT-d side. Because even use separate VT-d page table, we still 
> cannot track the memory update from DMA. Even worse, I think two page tables 
> introduce redundant code and maintain effort. So I wonder is it really 
> necessary to implement the separate VT-d large page?
> 
> Yes, it does introduce redundant code.  But unfortunately, IOMMU
> faults at the moment have to be considered rather risky; having on
> happens risks (in order of decreasing probability / increasing
> damage):
> * Device stops working for that VM until an FLR (losing a lot of its state)
> * The VM has to be killed
> * The device stops working until a host reboot
> * The host crashes
> 
> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
> buffer isn't really robust enough.  I think that was Tim and Jan's
> primary reason for wanting the ability to have separate tables for HAP
> and IOMMU.
> 
> Is that about right, Jan / Tim?

Yes, and not just "about" (perhaps with the exception that I think/
hope we don't have any lurking host crashes here).

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 05/19/2014 02:50 PM, Jan Beulich wrote:
>>>> On 19.05.14 at 15:27, <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, May 19, 2014 at 8:48 AM, Zhang, Yang Z <yang.z.zhang@intel.com> wrote:
>>> Because I just noticed that someone is asking when Intel will implement the
>> VT-d page table separately. Actually, I am totally unaware it. The original
>> issue that this patch tries to fix is the VRAM tracking which using the
>> global log dirty mode. And I thought the best solution to fix it is in VRAM
>> side not VT-d side. Because even use separate VT-d page table, we still
>> cannot track the memory update from DMA. Even worse, I think two page tables
>> introduce redundant code and maintain effort. So I wonder is it really
>> necessary to implement the separate VT-d large page?
>>
>> Yes, it does introduce redundant code.  But unfortunately, IOMMU
>> faults at the moment have to be considered rather risky; having on
>> happens risks (in order of decreasing probability / increasing
>> damage):
>> * Device stops working for that VM until an FLR (losing a lot of its state)
>> * The VM has to be killed
>> * The device stops working until a host reboot
>> * The host crashes
>>
>> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
>> buffer isn't really robust enough.  I think that was Tim and Jan's
>> primary reason for wanting the ability to have separate tables for HAP
>> and IOMMU.
>>
>> Is that about right, Jan / Tim?
> Yes, and not just "about" (perhaps with the exception that I think/
> hope we don't have any lurking host crashes here).

I think the fear was that buggy hardware might cause a host crash / hang.

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 19.05.14 at 15:59, <george.dunlap@eu.citrix.com> wrote:
> On 05/19/2014 02:50 PM, Jan Beulich wrote:
>>>>> On 19.05.14 at 15:27, <George.Dunlap@eu.citrix.com> wrote:
>>> On Mon, May 19, 2014 at 8:48 AM, Zhang, Yang Z <yang.z.zhang@intel.com> wrote:
>>>> Because I just noticed that someone is asking when Intel will implement the
>>> VT-d page table separately. Actually, I am totally unaware it. The original
>>> issue that this patch tries to fix is the VRAM tracking which using the
>>> global log dirty mode. And I thought the best solution to fix it is in VRAM
>>> side not VT-d side. Because even use separate VT-d page table, we still
>>> cannot track the memory update from DMA. Even worse, I think two page tables
>>> introduce redundant code and maintain effort. So I wonder is it really
>>> necessary to implement the separate VT-d large page?
>>>
>>> Yes, it does introduce redundant code.  But unfortunately, IOMMU
>>> faults at the moment have to be considered rather risky; having on
>>> happens risks (in order of decreasing probability / increasing
>>> damage):
>>> * Device stops working for that VM until an FLR (losing a lot of its state)
>>> * The VM has to be killed
>>> * The device stops working until a host reboot
>>> * The host crashes
>>>
>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
>>> buffer isn't really robust enough.  I think that was Tim and Jan's
>>> primary reason for wanting the ability to have separate tables for HAP
>>> and IOMMU.
>>>
>>> Is that about right, Jan / Tim?
>> Yes, and not just "about" (perhaps with the exception that I think/
>> hope we don't have any lurking host crashes here).
> 
> I think the fear was that buggy hardware might cause a host crash / hang.

That's a valid concern indeed.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

George Dunlap wrote on 2014-05-19:
>> 
>> Hi all
>> 
>> Sorry to turn out this old thread.
>> Because I just noticed that someone is asking when Intel will
>> implement the
> VT-d page table separately. Actually, I am totally unaware it. The
> original issue that this patch tries to fix is the VRAM tracking which
> using the global log dirty mode. And I thought the best solution to fix it is in VRAM side not VT-d side.
> Because even use separate VT-d page table, we still cannot track the
> memory update from DMA. Even worse, I think two page tables introduce
> redundant code and maintain effort. So I wonder is it really necessary
> to implement the separate VT-d large page?
> 
> Yes, it does introduce redundant code.  But unfortunately, IOMMU faults
> at the moment have to be considered rather risky; having on happens

You cannot prevent guest to trigger the IOMMU faults. It is easy to trigger an IOMMU fault by setting a invalid DMA address.

> risks (in order of decreasing probability / increasing damage): * Device
> stops working for that VM until an FLR (losing a lot of its state) * The
> VM has to be killed * The device stops working until a host reboot * The
> host crashes
> 
> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
> buffer isn't really robust enough.  I think that was Tim and Jan's

Video buffer is only one case. How we can prevent the DMA to other reserved region?

> primary reason for wanting the ability to have separate tables for HAP and IOMMU.
> 
> Is that about right, Jan / Tim?
> 
>  -George


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
> George Dunlap wrote on 2014-05-19:
>> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
>> buffer isn't really robust enough.  I think that was Tim and Jan's
> 
> Video buffer is only one case. How we can prevent the DMA to other reserved 
> region?

You continue to neglect the difference: Accessing VRAM this way is
legitimate (and potentially useful). And - as just said in the other
reply - ideally we'd also simply ignore accesses to reserved regions
(and in fact we try to, by not immediately bringing down a guest
device doing such).

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On Tue, May 20, 2014 at 8:20 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
>> George Dunlap wrote on 2014-05-19:
>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
>>> buffer isn't really robust enough.  I think that was Tim and Jan's
>>
>> Video buffer is only one case. How we can prevent the DMA to other reserved
>> region?
>
> You continue to neglect the difference: Accessing VRAM this way is
> legitimate (and potentially useful). And - as just said in the other
> reply - ideally we'd also simply ignore accesses to reserved regions
> (and in fact we try to, by not immediately bringing down a guest
> device doing such).

On the other hand, just to play devil's advocate here: Implementing
separate IOMMU tables (including superpages) isn't free; it has a
non-negligible cost, both in initial developer time, continuing
maintenance (code complexity, fixing bugs), extra memory at run-time,
&c.

Of all the things we could invest that developer time doing, why
should we make it possible to DMA into VRAM, rather than doing
something else?

 -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 20.05.14 at 12:12, <George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 20, 2014 at 8:20 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
>>> George Dunlap wrote on 2014-05-19:
>>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a video
>>>> buffer isn't really robust enough.  I think that was Tim and Jan's
>>>
>>> Video buffer is only one case. How we can prevent the DMA to other reserved
>>> region?
>>
>> You continue to neglect the difference: Accessing VRAM this way is
>> legitimate (and potentially useful). And - as just said in the other
>> reply - ideally we'd also simply ignore accesses to reserved regions
>> (and in fact we try to, by not immediately bringing down a guest
>> device doing such).
> 
> On the other hand, just to play devil's advocate here: Implementing
> separate IOMMU tables (including superpages) isn't free; it has a
> non-negligible cost, both in initial developer time, continuing
> maintenance (code complexity, fixing bugs), extra memory at run-time,
> &c.
> 
> Of all the things we could invest that developer time doing, why
> should we make it possible to DMA into VRAM, rather than doing
> something else?

While I agree that the question is valid, my position really is that it
was a mistake to implement the IOMMU code without superpage
support, i.e. I view this as a shortcoming independent of the VRAM
issue, and I would want to see this fixed rather sooner than later.
Had it been done properly from the beginning (like one would
expect for non-experimental code), a lot of this discussion could
have been avoided, and we wouldn't have had to take the
respective workaround close to the 4.4 release.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-05-20:
>>>> On 20.05.14 at 12:12, <George.Dunlap@eu.citrix.com> wrote:
>> On Tue, May 20, 2014 at 8:20 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
>>>> George Dunlap wrote on 2014-05-19:
>>>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a
>>>>> video buffer isn't really robust enough.  I think that was Tim
>>>>> and Jan's
>>>> 
>>>> Video buffer is only one case. How we can prevent the DMA to other
>>>> reserved region?
>>> 
>>> You continue to neglect the difference: Accessing VRAM this way is
>>> legitimate (and potentially useful). And - as just said in the
>>> other reply - ideally we'd also simply ignore accesses to reserved

Can you give an example of what legitmate case you are mentioned twice(You mentioned it also in other reply)? I cannot understand why we need to restrict the CPU access to VRAM region but allow accessing from device. As I known, for gfx passthrough, both device and CPU are able to access them. And for emulated gfx, only software will access it which same as current we see in Xen.

>>> regions (and in fact we try to, by not immediately bringing down a
>>> guest device doing such).
>> 
>> On the other hand, just to play devil's advocate here: Implementing
>> separate IOMMU tables (including superpages) isn't free; it has a
>> non-negligible cost, both in initial developer time, continuing
>> maintenance (code complexity, fixing bugs), extra memory at
>> run-time, &c.
>> 
>> Of all the things we could invest that developer time doing, why
>> should we make it possible to DMA into VRAM, rather than doing
>> something else?
> 
> While I agree that the question is valid, my position really is that
> it was a mistake to implement the IOMMU code without superpage

We support the superpage via sharing EPT and VT-d pagetable.

> support, i.e. I view this as a shortcoming independent of the VRAM
> issue, and I would want to see this fixed rather sooner than later.
> Had it been done properly from the beginning (like one would expect
> for non-experimental code), a lot of this discussion could have been
> avoided, and we wouldn't have had to take the respective workaround
> close to the 4.4 release.

I still think the best solution is fixing the VRAM global log dirty mechanism which my previous patch already did. Because I cannot see any benefit with separating the page table.

> 
> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 21.05.14 at 03:02, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-05-20:
>>>>> On 20.05.14 at 12:12, <George.Dunlap@eu.citrix.com> wrote:
>>> On Tue, May 20, 2014 at 8:20 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
>>>>> George Dunlap wrote on 2014-05-19:
>>>>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a
>>>>>> video buffer isn't really robust enough.  I think that was Tim
>>>>>> and Jan's
>>>>> 
>>>>> Video buffer is only one case. How we can prevent the DMA to other
>>>>> reserved region?
>>>> 
>>>> You continue to neglect the difference: Accessing VRAM this way is
>>>> legitimate (and potentially useful). And - as just said in the
>>>> other reply - ideally we'd also simply ignore accesses to reserved
> 
> Can you give an example of what legitmate case you are mentioned twice(You 
> mentioned it also in other reply)?

What's wrong with loading some graphics data right from an I/O device
(disk, network) into the frame buffer?

> I cannot understand why we need to 
> restrict the CPU access to VRAM region but allow accessing from device.

We'd love to also do this for device accesses, but to date IOMMU
fault are unrecoverable, and hence this is not an option.

> As I 
> known, for gfx passthrough, both device and CPU are able to access them. And 
> for emulated gfx, only software will access it which same as current we see 
> in Xen.

Why's that? I.e. why should a guest with emulated graphics not be
able to program a passed through device to access the VRAM directly?

>>>> regions (and in fact we try to, by not immediately bringing down a
>>>> guest device doing such).
>>> 
>>> On the other hand, just to play devil's advocate here: Implementing
>>> separate IOMMU tables (including superpages) isn't free; it has a
>>> non-negligible cost, both in initial developer time, continuing
>>> maintenance (code complexity, fixing bugs), extra memory at
>>> run-time, &c.
>>> 
>>> Of all the things we could invest that developer time doing, why
>>> should we make it possible to DMA into VRAM, rather than doing
>>> something else?
>> 
>> While I agree that the question is valid, my position really is that
>> it was a mistake to implement the IOMMU code without superpage
> 
> We support the superpage via sharing EPT and VT-d pagetable.

I.e. without EPT no superpages. Afaict there's no such limitation on
the AMD side.

>> support, i.e. I view this as a shortcoming independent of the VRAM
>> issue, and I would want to see this fixed rather sooner than later.
>> Had it been done properly from the beginning (like one would expect
>> for non-experimental code), a lot of this discussion could have been
>> avoided, and we wouldn't have had to take the respective workaround
>> close to the 4.4 release.
> 
> I still think the best solution is fixing the VRAM global log dirty 
> mechanism which my previous patch already did. Because I cannot see any 
> benefit with separating the page table.

You didn't in any way negate the condition of superpage support to
be added post-4.4 in order for your other change to go in: Neither
http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01230.html
nor
http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01236.html
have been responded to by you. By not doing so, to me at least you
implicitly accepted the condition. And by now refusing to meet it, you
basically tell us that we shouldn't be doing compromises like this with
you in the future.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-05-21:
>>>> On 21.05.14 at 03:02, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-05-20:
>>>>>> On 20.05.14 at 12:12, <George.Dunlap@eu.citrix.com> wrote:
>>>> On Tue, May 20, 2014 at 8:20 AM, Jan Beulich <JBeulich@suse.com>
> wrote:
>>>>>>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
>>>>>> George Dunlap wrote on 2014-05-19:
>>>>>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a
>>>>>>> video buffer isn't really robust enough.  I think that was Tim
>>>>>>> and Jan's
>>>>>> 
>>>>>> Video buffer is only one case. How we can prevent the DMA to other
>>>>>> reserved region?
>>>>> 
>>>>> You continue to neglect the difference: Accessing VRAM this way is
>>>>> legitimate (and potentially useful). And - as just said in the
>>>>> other reply - ideally we'd also simply ignore accesses to reserved
>> 
>> Can you give an example of what legitmate case you are mentioned twice(You
>> mentioned it also in other reply)?
> 
> What's wrong with loading some graphics data right from an I/O device
> (disk, network) into the frame buffer?

Yes, it is ok. But we are talking about the DMA access to a 'readonly' buffer. It is totally a wrong usage model to me.

> 
>> I cannot understand why we need to
>> restrict the CPU access to VRAM region but allow accessing from device.
> 
> We'd love to also do this for device accesses, but to date IOMMU
> fault are unrecoverable, and hence this is not an option.
> 
>> As I known, for gfx passthrough, both device and CPU are able to access
>> them. And for emulated gfx, only software will access it which same as
>> current we see in Xen.
> 
> Why's that? I.e. why should a guest with emulated graphics not be
> able to program a passed through device to access the VRAM directly?

I think you confused the background of the issue: the essence of the issue is that we marked all pages as read-only and meanwhile we allow DMA to access those page. So the right solution is only mark the required page (in this case means VRAM region) as readonly and if guest still DMA to that region, then any unpredictable behavior to that guest is reasonable. What you said before totally ignore the 'read-only' thing. If there is no read-only operation, there will be no problem and no discussion around it.

> 
>>>>> regions (and in fact we try to, by not immediately bringing down a
>>>>> guest device doing such).
>>>> 
>>>> On the other hand, just to play devil's advocate here: Implementing
>>>> separate IOMMU tables (including superpages) isn't free; it has a
>>>> non-negligible cost, both in initial developer time, continuing
>>>> maintenance (code complexity, fixing bugs), extra memory at
>>>> run-time, &c.
>>>> 
>>>> Of all the things we could invest that developer time doing, why
>>>> should we make it possible to DMA into VRAM, rather than doing
>>>> something else?
>>> 
>>> While I agree that the question is valid, my position really is that
>>> it was a mistake to implement the IOMMU code without superpage
>> 
>> We support the superpage via sharing EPT and VT-d pagetable.
> 
> I.e. without EPT no superpages. Afaict there's no such limitation on
> the AMD side.

I don't think this is a 'limitation'. The original motivation to sharing page table is to use memory efficiently and easy for code maintain. And when we tried to add superpage support, we follow this way (we only need to change one copy of page table). But you think it is a limitation. I cannot understand.

> 
>>> support, i.e. I view this as a shortcoming independent of the VRAM
>>> issue, and I would want to see this fixed rather sooner than later.
>>> Had it been done properly from the beginning (like one would expect
>>> for non-experimental code), a lot of this discussion could have been
>>> avoided, and we wouldn't have had to take the respective workaround
>>> close to the 4.4 release.
>> 
>> I still think the best solution is fixing the VRAM global log dirty
>> mechanism which my previous patch already did. Because I cannot see any
>> benefit with separating the page table.
> 
> You didn't in any way negate the condition of superpage support to be
> added post-4.4 in order for your other change to go in: Neither
> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01230.html
>  nor
> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01236.html
>  have been responded to by you. By not doing so, to me at least you
> implicitly accepted the condition. And by now refusing to meet it, you
> basically tell us that we shouldn't be doing compromises like this with
> you in the future.

I have said before I am totally unware of it. And I know it only two days ago after someone told me. So please do not confuse this with the thing what we are discussing now. If you think I gave a promise implicitly at that time, I am sorry to let you think so.

As I said in previous thread, if we can prove that add hugepage for the separate VT-d page table is the only choice to solve problem, then now I am promising that I will do it ASAP. But till now, I didn't see any point that we must to have it. To me, it is still a nice to have feature.

> 
> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 21.05.14 at 10:37, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-05-21:
>>>>> On 21.05.14 at 03:02, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-05-20:
>>>>>>> On 20.05.14 at 12:12, <George.Dunlap@eu.citrix.com> wrote:
>>>>> On Tue, May 20, 2014 at 8:20 AM, Jan Beulich <JBeulich@suse.com>
>> wrote:
>>>>>>>>> On 20.05.14 at 05:13, <yang.z.zhang@intel.com> wrote:
>>>>>>> George Dunlap wrote on 2014-05-19:
>>>>>>>> Avoiding these by "hoping" that the guest OS doesn't DMA into a
>>>>>>>> video buffer isn't really robust enough.  I think that was Tim
>>>>>>>> and Jan's
>>>>>>> 
>>>>>>> Video buffer is only one case. How we can prevent the DMA to other
>>>>>>> reserved region?
>>>>>> 
>>>>>> You continue to neglect the difference: Accessing VRAM this way is
>>>>>> legitimate (and potentially useful). And - as just said in the
>>>>>> other reply - ideally we'd also simply ignore accesses to reserved
>>> 
>>> Can you give an example of what legitmate case you are mentioned twice(You
>>> mentioned it also in other reply)?
>> 
>> What's wrong with loading some graphics data right from an I/O device
>> (disk, network) into the frame buffer?
> 
> Yes, it is ok. But we are talking about the DMA access to a 'readonly' 
> buffer. It is totally a wrong usage model to me.

What read-only buffer are you referring to? From guest perspective,
nothing that gets marked read-only due to log-dirty handling really is
read-only. The fact that migration doesn't work due to this with an
assigned device can be excused by the handling of such an assigned
device not being implemented anyway. But the fact that behind the
scenes VRAM gets marked read-only should be (but isn't) entirely
transparent to the guest.

I'm not going to reply to the rest of your mail, both because I'm getting
the impression that we're moving in circles, and because I might become
impolite otherwise.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 21.05.14 at 10:37, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-05-21:
>> You didn't in any way negate the condition of superpage support to be
>> added post-4.4 in order for your other change to go in: Neither
>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01230.html 
>>  nor
>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01236.html 
>>  have been responded to by you. By not doing so, to me at least you
>> implicitly accepted the condition. And by now refusing to meet it, you
>> basically tell us that we shouldn't be doing compromises like this with
>> you in the future.
> 
> I have said before I am totally unware of it. And I know it only two days 
> ago after someone told me. So please do not confuse this with the thing what 
> we are discussing now. If you think I gave a promise implicitly at that time, 
> I am sorry to let you think so.
> 
> As I said in previous thread, if we can prove that add hugepage for the 
> separate VT-d page table is the only choice to solve problem, then now I am 
> promising that I will do it ASAP. But till now, I didn't see any point that 
> we must to have it. To me, it is still a nice to have feature.

Btw., I think I just spotted a second thing not working without split
page tables: mem-access (which doesn't and imo shouldn't depend
on !need_iommu(), other than mem-sharing and mem-paging)
likewise has the potential of creating entries resulting in IOMMU
faults.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Zhang, Yang Z]

Jan Beulich wrote on 2014-05-23:
>>>> On 21.05.14 at 10:37, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-05-21:
>>> You didn't in any way negate the condition of superpage support to
>>> be added post-4.4 in order for your other change to go in: Neither
>>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01230.
>>> html
>>>  nor
>>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01236.
>>> html  have been responded to by you. By not doing so, to me at
>>> least you implicitly accepted the condition. And by now refusing to
>>> meet it, you basically tell us that we shouldn't be doing
>>> compromises like this with you in the future.
>> 
>> I have said before I am totally unware of it. And I know it only two
>> days ago after someone told me. So please do not confuse this with
>> the thing what we are discussing now. If you think I gave a promise
>> implicitly at that time, I am sorry to let you think so.
>> 
>> As I said in previous thread, if we can prove that add hugepage for
>> the separate VT-d page table is the only choice to solve problem,
>> then now I am promising that I will do it ASAP. But till now, I
>> didn't see any point that we must to have it. To me, it is still a nice to have feature.
> 
> Btw., I think I just spotted a second thing not working without split page tables:
> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
> other than mem-sharing and mem-paging) likewise has the potential of
> creating entries resulting in IOMMU faults.
> 

I don't know what mem-access is? Do you mean Xenaccess? If not, can you elaborate it or provide a link to help me to understand how it works?

> Jan


Best regards,
Yang

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 26.05.14 at 10:16, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-05-23:
>>>>> On 21.05.14 at 10:37, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-05-21:
>>>> You didn't in any way negate the condition of superpage support to
>>>> be added post-4.4 in order for your other change to go in: Neither
>>>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01230.
>>>> html
>>>>  nor
>>>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01236.
>>>> html  have been responded to by you. By not doing so, to me at
>>>> least you implicitly accepted the condition. And by now refusing to
>>>> meet it, you basically tell us that we shouldn't be doing
>>>> compromises like this with you in the future.
>>> 
>>> I have said before I am totally unware of it. And I know it only two
>>> days ago after someone told me. So please do not confuse this with
>>> the thing what we are discussing now. If you think I gave a promise
>>> implicitly at that time, I am sorry to let you think so.
>>> 
>>> As I said in previous thread, if we can prove that add hugepage for
>>> the separate VT-d page table is the only choice to solve problem,
>>> then now I am promising that I will do it ASAP. But till now, I
>>> didn't see any point that we must to have it. To me, it is still a nice to 
> have feature.
>> 
>> Btw., I think I just spotted a second thing not working without split page 
> tables:
>> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
>> other than mem-sharing and mem-paging) likewise has the potential of
>> creating entries resulting in IOMMU faults.
>> 
> 
> I don't know what mem-access is? Do you mean Xenaccess? If not, can you 
> elaborate it or provide a link to help me to understand how it works?

The (example) tool indeed is named xen-access. See XENMEM_access_op
(used to be HVMOP_{get,set}_mem_access).

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Nakajima, Jun]

On Mon, May 26, 2014 at 2:04 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.05.14 at 10:16, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-05-23:
>>>>>> On 21.05.14 at 10:37, <yang.z.zhang@intel.com> wrote:
>>>> Jan Beulich wrote on 2014-05-21:
>>>>> You didn't in any way negate the condition of superpage support to
>>>>> be added post-4.4 in order for your other change to go in: Neither
>>>>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01230.
>>>>> html
>>>>>  nor
>>>>> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg01236.
>>>>> html  have been responded to by you. By not doing so, to me at
>>>>> least you implicitly accepted the condition. And by now refusing to
>>>>> meet it, you basically tell us that we shouldn't be doing
>>>>> compromises like this with you in the future.
>>>>
>>>> I have said before I am totally unware of it. And I know it only two
>>>> days ago after someone told me. So please do not confuse this with
>>>> the thing what we are discussing now. If you think I gave a promise
>>>> implicitly at that time, I am sorry to let you think so.
>>>>
>>>> As I said in previous thread, if we can prove that add hugepage for
>>>> the separate VT-d page table is the only choice to solve problem,
>>>> then now I am promising that I will do it ASAP. But till now, I
>>>> didn't see any point that we must to have it. To me, it is still a nice to
>> have feature.
>>>
>>> Btw., I think I just spotted a second thing not working without split page
>> tables:
>>> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
>>> other than mem-sharing and mem-paging) likewise has the potential of
>>> creating entries resulting in IOMMU faults.
>>>
>>
>> I don't know what mem-access is? Do you mean Xenaccess? If not, can you
>> elaborate it or provide a link to help me to understand how it works?
>
> The (example) tool indeed is named xen-access. See XENMEM_access_op
> (used to be HVMOP_{get,set}_mem_access).
>

The tool xen-access is located in tools/tests, and I think that this
is used mostly by developers who know what they are doing.
If we had separate VT-d page tables, they might observe confusing
results; even if they write-protect pages, somebody (i.e. I/O devices)
modifies those pages.
To me, observing IOMMU faults seems consistent with the consequence of
changes to guest memory permission.

-- 
Jun
Intel Open Source Technology Center

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 31.05.14 at 03:26, <jun.nakajima@intel.com> wrote:
> On Mon, May 26, 2014 at 2:04 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.05.14 at 10:16, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-05-23:
>>>> Btw., I think I just spotted a second thing not working without split page
>>> tables:
>>>> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
>>>> other than mem-sharing and mem-paging) likewise has the potential of
>>>> creating entries resulting in IOMMU faults.
>>>>
>>>
>>> I don't know what mem-access is? Do you mean Xenaccess? If not, can you
>>> elaborate it or provide a link to help me to understand how it works?
>>
>> The (example) tool indeed is named xen-access. See XENMEM_access_op
>> (used to be HVMOP_{get,set}_mem_access).
>>
> 
> The tool xen-access is located in tools/tests, and I think that this
> is used mostly by developers who know what they are doing.

The tool is, indeed. But the underlying feature clearly isn't limited
to or solely intended for developers.

> If we had separate VT-d page tables, they might observe confusing
> results; even if they write-protect pages, somebody (i.e. I/O devices)
> modifies those pages.
> To me, observing IOMMU faults seems consistent with the consequence of
> changes to guest memory permission.

And I would agree if these faults were restartable. You're certainly
aware that a not too large amount of faults within a reasonably short
period of time will lead to the device being turned off, with quite likely
fatal consequences to the guest.

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On Mon, Jun 2, 2014 at 7:55 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 31.05.14 at 03:26, <jun.nakajima@intel.com> wrote:
>> On Mon, May 26, 2014 at 2:04 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 26.05.14 at 10:16, <yang.z.zhang@intel.com> wrote:
>>>> Jan Beulich wrote on 2014-05-23:
>>>>> Btw., I think I just spotted a second thing not working without split page
>>>> tables:
>>>>> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
>>>>> other than mem-sharing and mem-paging) likewise has the potential of
>>>>> creating entries resulting in IOMMU faults.
>>>>>
>>>>
>>>> I don't know what mem-access is? Do you mean Xenaccess? If not, can you
>>>> elaborate it or provide a link to help me to understand how it works?
>>>
>>> The (example) tool indeed is named xen-access. See XENMEM_access_op
>>> (used to be HVMOP_{get,set}_mem_access).
>>>
>>
>> The tool xen-access is located in tools/tests, and I think that this
>> is used mostly by developers who know what they are doing.
>
> The tool is, indeed. But the underlying feature clearly isn't limited
> to or solely intended for developers.
>
>> If we had separate VT-d page tables, they might observe confusing
>> results; even if they write-protect pages, somebody (i.e. I/O devices)
>> modifies those pages.
>> To me, observing IOMMU faults seems consistent with the consequence of
>> changes to guest memory permission.
>
> And I would agree if these faults were restartable. You're certainly
> aware that a not too large amount of faults within a reasonably short
> period of time will lead to the device being turned off, with quite likely
> fatal consequences to the guest.

Sure -- but there are a number of features (PoD, paging, page sharing,
even migration) which are incompatible with pass-through, and the user
is simply not allowed to use them together.  Why not just add this one
to the list?

 -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Jan Beulich]

>>> On 02.06.14 at 16:06, <George.Dunlap@eu.citrix.com> wrote:
> On Mon, Jun 2, 2014 at 7:55 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 31.05.14 at 03:26, <jun.nakajima@intel.com> wrote:
>>> On Mon, May 26, 2014 at 2:04 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 26.05.14 at 10:16, <yang.z.zhang@intel.com> wrote:
>>>>> Jan Beulich wrote on 2014-05-23:
>>>>>> Btw., I think I just spotted a second thing not working without split page
>>>>> tables:
>>>>>> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
>>>>>> other than mem-sharing and mem-paging) likewise has the potential of
>>>>>> creating entries resulting in IOMMU faults.
>>>>>>
>>>>>
>>>>> I don't know what mem-access is? Do you mean Xenaccess? If not, can you
>>>>> elaborate it or provide a link to help me to understand how it works?
>>>>
>>>> The (example) tool indeed is named xen-access. See XENMEM_access_op
>>>> (used to be HVMOP_{get,set}_mem_access).
>>>>
>>>
>>> The tool xen-access is located in tools/tests, and I think that this
>>> is used mostly by developers who know what they are doing.
>>
>> The tool is, indeed. But the underlying feature clearly isn't limited
>> to or solely intended for developers.
>>
>>> If we had separate VT-d page tables, they might observe confusing
>>> results; even if they write-protect pages, somebody (i.e. I/O devices)
>>> modifies those pages.
>>> To me, observing IOMMU faults seems consistent with the consequence of
>>> changes to guest memory permission.
>>
>> And I would agree if these faults were restartable. You're certainly
>> aware that a not too large amount of faults within a reasonably short
>> period of time will lead to the device being turned off, with quite likely
>> fatal consequences to the guest.
> 
> Sure -- but there are a number of features (PoD, paging, page sharing,
> even migration) which are incompatible with pass-through, and the user
> is simply not allowed to use them together.  Why not just add this one
> to the list?

Considering that mem-access came at about the same time or even
later than mem-paging and mem-sharing, I was silently assuming that
it wasn't enforcing the same restrictions as the other two for a reason.
Maybe it was just forgotten...

Jan

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

On 06/02/2014 03:27 PM, Jan Beulich wrote:
>>>> On 02.06.14 at 16:06, <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, Jun 2, 2014 at 7:55 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 31.05.14 at 03:26, <jun.nakajima@intel.com> wrote:
>>>> On Mon, May 26, 2014 at 2:04 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 26.05.14 at 10:16, <yang.z.zhang@intel.com> wrote:
>>>>>> Jan Beulich wrote on 2014-05-23:
>>>>>>> Btw., I think I just spotted a second thing not working without split page
>>>>>> tables:
>>>>>>> mem-access (which doesn't and imo shouldn't depend on !need_iommu(),
>>>>>>> other than mem-sharing and mem-paging) likewise has the potential of
>>>>>>> creating entries resulting in IOMMU faults.
>>>>>>>
>>>>>> I don't know what mem-access is? Do you mean Xenaccess? If not, can you
>>>>>> elaborate it or provide a link to help me to understand how it works?
>>>>> The (example) tool indeed is named xen-access. See XENMEM_access_op
>>>>> (used to be HVMOP_{get,set}_mem_access).
>>>>>
>>>> The tool xen-access is located in tools/tests, and I think that this
>>>> is used mostly by developers who know what they are doing.
>>> The tool is, indeed. But the underlying feature clearly isn't limited
>>> to or solely intended for developers.
>>>
>>>> If we had separate VT-d page tables, they might observe confusing
>>>> results; even if they write-protect pages, somebody (i.e. I/O devices)
>>>> modifies those pages.
>>>> To me, observing IOMMU faults seems consistent with the consequence of
>>>> changes to guest memory permission.
>>> And I would agree if these faults were restartable. You're certainly
>>> aware that a not too large amount of faults within a reasonably short
>>> period of time will lead to the device being turned off, with quite likely
>>> fatal consequences to the guest.
>> Sure -- but there are a number of features (PoD, paging, page sharing,
>> even migration) which are incompatible with pass-through, and the user
>> is simply not allowed to use them together.  Why not just add this one
>> to the list?
> Considering that mem-access came at about the same time or even
> later than mem-paging and mem-sharing, I was silently assuming that
> it wasn't enforcing the same restrictions as the other two for a reason.
> Maybe it was just forgotten...

Well one can certainly imagine someone wanting to run it in that mode, 
and "knowing" that it would be safe because they were going to be 
careful not going to use memaccess on pages on which a DMA was going to 
happen.  I think in that situation, a developer would probably want to 
know that their assumption was wrong immediately (by having the IOMMU 
fault perhaps crash the domain), than have to figure out that their 
assumption was wrong by having unexplainable results.

If memaccess + passthru is enabled by default at the moment, there may 
be an argument for disabling it by default, perhaps with an override for 
those who want the ability to shoot themselves in the foot.  But I don't 
think that silent violations of the memaccess "promise" is better than 
just having the IOMMU faults; so I don't think that's an argument for 
implementing non-shared IOMMU superpages.

  -George

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[Andrew Cooper]

On 10/02/14 08:03, Tim Deegan wrote:
> At 14:14 +0800 on 10 Feb (1392038040), Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>
>> When enabling log dirty mode, it sets all guest's memory to readonly.
>> And in HAP enabled domain, it modifies all EPT entries to clear write bit
>> to make sure it is readonly. This will cause problem if VT-d shares page
>> table with EPT: the device may issue a DMA write request, then VT-d engine
>> tells it the target memory is readonly and result in VT-d fault.
> So that's a problem even if only the VGA framebuffer is being tracked
> -- DMA from a passthrough device will either cause a spurious error or
> fail to update the dirt bitmap. 
>
> I think it would be better not to allow VT-d and EPT to share
> pagetables in cases where devices are passed through (i.e. all cases
> where VT-d is in use).
>
> Tim.

Sadly, this would make shared VT-d/EPT completely pointless as a
feature, causing extra memory overhead in Xen by having to maintain EPT
and IOMMU tables separately.

Any usecase which doesn't involve dirty vram tracking (e.g. headless VM
with SRIOV, PVH dom0) would be adversely affected.

~Andrew

Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[George Dunlap]

create ^
title it HAP dirty vram tracking breaks pass-through
thanks

On Mon, Feb 10, 2014 at 6:14 AM, Yang Zhang <yang.z.zhang@intel.com> wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> When enabling log dirty mode, it sets all guest's memory to readonly.
> And in HAP enabled domain, it modifies all EPT entries to clear write bit
> to make sure it is readonly. This will cause problem if VT-d shares page
> table with EPT: the device may issue a DMA write request, then VT-d engine
> tells it the target memory is readonly and result in VT-d fault.
>
> Currnetly, there are two places will enable log dirty mode: migration and vram
> tracking. Migration with device assigned is not allowed, so it is ok. For vram,
> it doesn't need to set all memory to readonly. Only track the vram range is enough.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/mm/hap/hap.c       |   20 ++++++++++++++------
>  xen/arch/x86/mm/paging.c        |    9 +++++----
>  xen/arch/x86/mm/shadow/common.c |    2 +-
>  xen/include/asm-x86/domain.h    |    2 +-
>  xen/include/asm-x86/paging.h    |    5 +++--
>  xen/include/asm-x86/shadow.h    |    2 +-
>  6 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index d3f64bd..5f75636 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -82,7 +82,7 @@ int hap_track_dirty_vram(struct domain *d,
>          if ( !paging_mode_log_dirty(d) )
>          {
>              hap_logdirty_init(d);
> -            rc = paging_log_dirty_enable(d);
> +            rc = paging_log_dirty_enable(d, 0);
>              if ( rc )
>                  goto out;
>          }
> @@ -167,17 +167,25 @@ out:
>  /*            HAP LOG DIRTY SUPPORT             */
>  /************************************************/
>
> -/* hap code to call when log_dirty is enable. return 0 if no problem found. */
> -static int hap_enable_log_dirty(struct domain *d)
> +/*
> + * hap code to call when log_dirty is enable. return 0 if no problem found.
> + *
> + * NB: Domain that having device assigned should not set log_global. Because
> + * there is no way to track the memory updating from device.
> + */
> +static int hap_enable_log_dirty(struct domain *d, bool_t log_global)
>  {
>      /* turn on PG_log_dirty bit in paging mode */
>      paging_lock(d);
>      d->arch.paging.mode |= PG_log_dirty;
>      paging_unlock(d);
>
> -    /* set l1e entries of P2M table to be read-only. */
> -    p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
> -    flush_tlb_mask(d->domain_dirty_cpumask);
> +    if ( log_global )
> +    {
> +        /* set l1e entries of P2M table to be read-only. */
> +        p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
> +        flush_tlb_mask(d->domain_dirty_cpumask);
> +    }
>      return 0;
>  }
>
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index 21344e5..ab5eacb 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -164,7 +164,7 @@ void paging_free_log_dirty_bitmap(struct domain *d)
>      paging_unlock(d);
>  }
>
> -int paging_log_dirty_enable(struct domain *d)
> +int paging_log_dirty_enable(struct domain *d, bool_t log_global)
>  {
>      int ret;
>
> @@ -172,7 +172,7 @@ int paging_log_dirty_enable(struct domain *d)
>          return -EINVAL;
>
>      domain_pause(d);
> -    ret = d->arch.paging.log_dirty.enable_log_dirty(d);
> +    ret = d->arch.paging.log_dirty.enable_log_dirty(d, log_global);
>      domain_unpause(d);
>
>      return ret;
> @@ -489,7 +489,8 @@ void paging_log_dirty_range(struct domain *d,
>   * These function pointers must not be followed with the log-dirty lock held.
>   */
>  void paging_log_dirty_init(struct domain *d,
> -                           int    (*enable_log_dirty)(struct domain *d),
> +                           int    (*enable_log_dirty)(struct domain *d,
> +                                                      bool_t log_global),
>                             int    (*disable_log_dirty)(struct domain *d),
>                             void   (*clean_dirty_bitmap)(struct domain *d))
>  {
> @@ -590,7 +591,7 @@ int paging_domctl(struct domain *d, xen_domctl_shadow_op_t *sc,
>      case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
>          if ( hap_enabled(d) )
>              hap_logdirty_init(d);
> -        return paging_log_dirty_enable(d);
> +        return paging_log_dirty_enable(d, 1);
>
>      case XEN_DOMCTL_SHADOW_OP_OFF:
>          if ( paging_mode_log_dirty(d) )
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 0bfa595..11c6b62 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -3418,7 +3418,7 @@ shadow_write_p2m_entry(struct vcpu *v, unsigned long gfn,
>  /* Shadow specific code which is called in paging_log_dirty_enable().
>   * Return 0 if no problem found.
>   */
> -int shadow_enable_log_dirty(struct domain *d)
> +int shadow_enable_log_dirty(struct domain *d, bool_t log_global)
>  {
>      int ret;
>
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index ea72db2..4ff89f0 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -169,7 +169,7 @@ struct log_dirty_domain {
>      unsigned int   dirty_count;
>
>      /* functions which are paging mode specific */
> -    int            (*enable_log_dirty   )(struct domain *d);
> +    int            (*enable_log_dirty   )(struct domain *d, bool_t log_global);
>      int            (*disable_log_dirty  )(struct domain *d);
>      void           (*clean_dirty_bitmap )(struct domain *d);
>  };
> diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
> index cd7ee3b..8dd2a61 100644
> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -143,14 +143,15 @@ void paging_log_dirty_range(struct domain *d,
>                              uint8_t *dirty_bitmap);
>
>  /* enable log dirty */
> -int paging_log_dirty_enable(struct domain *d);
> +int paging_log_dirty_enable(struct domain *d, bool_t log_global);
>
>  /* disable log dirty */
>  int paging_log_dirty_disable(struct domain *d);
>
>  /* log dirty initialization */
>  void paging_log_dirty_init(struct domain *d,
> -                           int  (*enable_log_dirty)(struct domain *d),
> +                           int  (*enable_log_dirty)(struct domain *d,
> +                                                    bool_t log_global),
>                             int  (*disable_log_dirty)(struct domain *d),
>                             void (*clean_dirty_bitmap)(struct domain *d));
>
> diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
> index 852023d..348915e 100644
> --- a/xen/include/asm-x86/shadow.h
> +++ b/xen/include/asm-x86/shadow.h
> @@ -82,7 +82,7 @@ void shadow_teardown(struct domain *d);
>  void shadow_final_teardown(struct domain *d);
>
>  /* shadow code to call when log dirty is enabled */
> -int shadow_enable_log_dirty(struct domain *d);
> +int shadow_enable_log_dirty(struct domain *d, bool_t log_global);
>
>  /* shadow code to call when log dirty is disabled */
>  int shadow_disable_log_dirty(struct domain *d);
> --
> 1.7.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Processed: Re: [PATCH] Don't track all memory when enabling log dirty to track vram

[xen]

Processing commands for xen@bugs.xenproject.org:

> create ^
Created new bug #38 rooted at `<1392012840-22555-1-git-send-email-yang.z.zhang@intel.com>'
Title: `Re: [Xen-devel] [PATCH] Don't track all memory when enabling log dirty to track vram'
> title it HAP dirty vram tracking breaks pass-through
Set title for #38 to `HAP dirty vram tracking breaks pass-through'
> thanks
Finished processing.

Modified/created Bugs:
 - 38: http://bugs.xenproject.org/xen/bug/38 (new)

---
Xen Hypervisor Bug Tracker
See http://wiki.xen.org/wiki/Reporting_Bugs_against_Xen for information on reporting bugs
Contact xen-bugs-owner@bugs.xenproject.org with any infrastructure issues