Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
On 06.05.15 at 14:32, roger@citrix.com wrote: El 06/05/15 a les 14.10, Jan Beulich ha escrit: On 06.05.15 at 13:55, roger@citrix.com wrote: El 14/04/15 a les 14.14, Jan Beulich ha escrit: On 10.04.15 at 19:29, roger@citrix.com wrote: +BUG_ON(((pages 3) % PAGE_SIZE) + bytes PAGE_SIZE); I don't seem to be able to spot the original for this one. If there was none, please make this an ASSERT() instead. Yes, there's no previous BUG_ON because this was not a problem in the past, since we could write to any position on dirty_bitmap, but that's not the case any more. Since we only have one page mapped at a time we need to make sure that what we are about to write doesn't cross a page boundary. I understand this is not an ideal solution, but AFAICT there's no easy way to deal with writes that expand over a page boundary. I'm afraid I don't really see what you're asking for. Just to clarify - I didn't put anything under question, all I asked for was to use ASSERT() instead of BUG_ON() here. Yet what you wrote above doesn't seem to related to that request. I don't think an ASSERT is appropriate here, because it means that on non-debug versions we might write past the end of the mapped page without noticing. With that argumentation there shouldn't be any ASSERT()s, but only BUG_ON()s. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
El 06/05/15 a les 14.10, Jan Beulich ha escrit: On 06.05.15 at 13:55, roger@citrix.com wrote: El 14/04/15 a les 14.14, Jan Beulich ha escrit: On 10.04.15 at 19:29, roger@citrix.com wrote: +BUG_ON(((pages 3) % PAGE_SIZE) + bytes PAGE_SIZE); I don't seem to be able to spot the original for this one. If there was none, please make this an ASSERT() instead. Yes, there's no previous BUG_ON because this was not a problem in the past, since we could write to any position on dirty_bitmap, but that's not the case any more. Since we only have one page mapped at a time we need to make sure that what we are about to write doesn't cross a page boundary. I understand this is not an ideal solution, but AFAICT there's no easy way to deal with writes that expand over a page boundary. I'm afraid I don't really see what you're asking for. Just to clarify - I didn't put anything under question, all I asked for was to use ASSERT() instead of BUG_ON() here. Yet what you wrote above doesn't seem to related to that request. I don't think an ASSERT is appropriate here, because it means that on non-debug versions we might write past the end of the mapped page without noticing. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
On 06.05.15 at 13:55, roger@citrix.com wrote: El 14/04/15 a les 14.14, Jan Beulich ha escrit: On 10.04.15 at 19:29, roger@citrix.com wrote: +BUG_ON(((pages 3) % PAGE_SIZE) + bytes PAGE_SIZE); I don't seem to be able to spot the original for this one. If there was none, please make this an ASSERT() instead. Yes, there's no previous BUG_ON because this was not a problem in the past, since we could write to any position on dirty_bitmap, but that's not the case any more. Since we only have one page mapped at a time we need to make sure that what we are about to write doesn't cross a page boundary. I understand this is not an ideal solution, but AFAICT there's no easy way to deal with writes that expand over a page boundary. I'm afraid I don't really see what you're asking for. Just to clarify - I didn't put anything under question, all I asked for was to use ASSERT() instead of BUG_ON() here. Yet what you wrote above doesn't seem to related to that request. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
Hello, I think I've fixed the rest of the comments, this one however is not easy to deal with. El 14/04/15 a les 14.14, Jan Beulich ha escrit: On 10.04.15 at 19:29, roger@citrix.com wrote: +BUG_ON(((pages 3) % PAGE_SIZE) + bytes PAGE_SIZE); I don't seem to be able to spot the original for this one. If there was none, please make this an ASSERT() instead. Yes, there's no previous BUG_ON because this was not a problem in the past, since we could write to any position on dirty_bitmap, but that's not the case any more. Since we only have one page mapped at a time we need to make sure that what we are about to write doesn't cross a page boundary. I understand this is not an ideal solution, but AFAICT there's no easy way to deal with writes that expand over a page boundary. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
On 16.04.15 at 11:11, t...@xen.org wrote: At 13:14 +0100 on 14 Apr (1429017287), Jan Beulich wrote: This won't work: The paging lock protects all of d-arch.paging.preempt.log_dirty, of which you hold cached values in local variables. ... so how about reusing the existing restart code instead? We could either move the label further up or tail-recurse into it. Possibly - I didn't look too closely though at how feasible this would be. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
At 13:14 +0100 on 14 Apr (1429017287), Jan Beulich wrote: This won't work: The paging lock protects all of d-arch.paging.preempt.log_dirty, of which you hold cached values in local variables. ... so how about reusing the existing restart code instead? We could either move the label further up or tail-recurse into it. Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
On 10.04.15 at 19:29, roger@citrix.com wrote:
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -397,6 +397,53 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
return rv;
}
+static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
+ unsigned long pages,
+ struct page_info **page,
+ unsigned long *mapped_page)
+{
+p2m_type_t p2mt;
+uint32_t pfec;
+unsigned long gfn;
+
+gfn = paging_gva_to_gfn(current,
+(paddr_t)(((char *)dirty_bitmap.p) + (pages
3)),
I'd suggest dropping the parentheses around the inner cast - they
make this more difficult to read, and I think any C programmer
should know that unary ops have precedence over binary ones.
+pfec);
+if ( gfn == INVALID_GFN )
+return NULL;
+
+*page = get_page_from_gfn(current-domain, gfn, p2mt, P2M_UNSHARE);
+
+if ( !p2m_is_ram(p2mt) )
+{
+put_page(*page);
+return NULL;
+}
+if ( p2m_is_paging(p2mt) )
+{
+put_page(*page);
+p2m_mem_paging_populate(current-domain, gfn);
+return NULL;
+}
+if ( p2m_is_shared(p2mt) || p2m_is_discard_write(p2mt) )
+{
+put_page(*page);
+return NULL;
+}
+
+*mapped_page = pages;
You only ever store the passed in value of pages into
*mapped_pages - what's the point of this? If the caller needs
to track the value it passes here, it should simple make a copy
itself if so needed.
Apart from that both parameter names don't really seem to
express their purpose.
@@ -409,9 +456,23 @@ static int paging_log_dirty_op(struct domain *d,
mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
unsigned long *l1 = NULL;
int i4, i3, i2;
+uint8_t *dirty_bitmap = NULL;
Pointless initializer.
+struct page_info *page;
+unsigned long index_mapped = 0;
if ( !resuming )
domain_pause(d);
+
+dirty_bitmap = map_dirty_bitmap(sc-dirty_bitmap,
+resuming ?
+
d-arch.paging.preempt.log_dirty.done :
+0,
+page, index_mapped);
+if ( dirty_bitmap == NULL )
+{
+domain_unpause(d);
+return -EFAULT;
+}
paging_lock(d);
Blank line above that one please.
@@ -471,15 +534,29 @@ static int paging_log_dirty_op(struct domain *d,
bytes = (unsigned int)((sc-pages - pages + 7) 3);
if ( likely(peek) )
{
-if ( (l1 ? copy_to_guest_offset(sc-dirty_bitmap,
-pages 3, (uint8_t
*)l1,
-bytes)
- : clear_guest_offset(sc-dirty_bitmap,
- pages 3, bytes)) != 0 )
+if ( pages (3 + PAGE_SHIFT) !=
+ index_mapped (3 + PAGE_SHIFT) )
{
-rv = -EFAULT;
-goto out;
+/* We need to map next page */
+paging_unlock(d);
+unmap_dirty_bitmap(dirty_bitmap, page);
+dirty_bitmap = map_dirty_bitmap(sc-dirty_bitmap,
pages,
+page,
index_mapped);
+paging_lock(d);
+if ( dirty_bitmap == NULL )
+{
+rv = -EFAULT;
+goto out;
+}
+goto again;
This won't work: The paging lock protects all of
d-arch.paging.preempt.log_dirty, of which you hold cached values
in local variables.
+BUG_ON(((pages 3) % PAGE_SIZE) + bytes PAGE_SIZE);
I don't seem to be able to spot the original for this one. If there
was none, please make this an ASSERT() instead.
+if ( l1 )
+memcpy(dirty_bitmap + ((pages 3) % PAGE_SIZE),
+ (uint8_t *)l1, bytes);
Pointless cast.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/3] xen: rework paging_log_dirty_op to work with hvm guests
El 10/04/15 a les 19.29, Roger Pau Monne ha escrit:
When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
trying to copy the dirty bitmap to the guest because the paging lock is
already held.
Fix this by independently mapping each page of the guest bitmap as needed
without the paging lock held.
Signed-off-by: Roger Pau Monné roger@citrix.com
Cc: Tim Deegan t...@xen.org
Cc: Jan Beulich jbeul...@suse.com
Cc: Andrew Cooper andrew.coop...@citrix.com
---
Changes since v2:
- Add checks for p2m_is_ram and p2m_is_discard_write when mapping a guest
page.
- Remove error checking from memset/memcpy, they unconditionally return
dst.
---
xen/arch/x86/mm/paging.c | 103
+--
1 file changed, 91 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index b54d76a..cb5feb8 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -397,6 +397,53 @@ int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
return rv;
}
+static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
+ unsigned long pages,
+ struct page_info **page,
+ unsigned long *mapped_page)
+{
+p2m_type_t p2mt;
+uint32_t pfec;
This should be:
uint32_t pfec = PFEC_page_present | PFEC_write_access;
Will wait for other's comments before resending the series.
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
