Re: [Xen-devel] [PATCH] Provide support for multiple frame buffers in Xen

[Tim Deegan <tim@xxxxxxx>]

Hi, 

We're very nearly there now.  I think I agree on almost all the
technical decisions but there are still a few things to tidy up (some of
which I mentioned before).

At 16:31 -0500 on 12 Nov (1352737913), Robert Phillips wrote:
> Support is provided for both shadow and hardware assisted paging (HAP) modes.
> This code bookkeeps the set of video frame buffers (vram),
> detects when the guest has modified any of those buffers and, upon request,
> returns a bitmap of the modified pages.
> This lets other software components re-paint the portions of the monitor (or 
> monitors) that have changed.
> Each monitor has a frame buffer of some size at some position in guest 
> physical memory.
> The set of frame buffers being tracked can change over time as monitors are 
> plugged and unplugged.
> (Version 3 of this patch.)

Please linewrap at something less than 80 characters.

> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> index eea5555..e374aac 100644
> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -22,4 +22,4 @@ obj-y += vlapic.o
>  obj-y += vmsi.o
>  obj-y += vpic.o
>  obj-y += vpt.o
> -obj-y += vpmu.o
> \ No newline at end of file
> +obj-y += vpmu.o

This is an unrelated fix, so doesn't belong in this changeset.

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 34da2f5..3a3e5e4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -57,6 +57,7 @@
>  #include <asm/hvm/cacheattr.h>
>  #include <asm/hvm/trace.h>
>  #include <asm/hvm/nestedhvm.h>
> +#include <asm/dirty_vram.h>
>  #include <asm/mtrr.h>
>  #include <asm/apic.h>
>  #include <public/sched.h>
> @@ -66,6 +67,7 @@
>  #include <asm/mem_event.h>
>  #include <asm/mem_access.h>
>  #include <public/mem_event.h>
> +#include "../mm/mm-locks.h"
>  
>  bool_t __read_mostly hvm_enabled;
>  
> @@ -1433,8 +1435,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>           */
>          if ( access_w )
>          {
> +            p2m_type_t pt;
> +            pt = p2m_change_type(v->domain, gfn, p2m_ram_logdirty, 
> p2m_ram_rw);
> +            
> +            paging_lock(v->domain);
> +            if ( pt == p2m_ram_logdirty )
> +            {
> +                dv_range_t *range;
> +                v->domain->arch.paging.log_dirty.dirty_count++;
> +                range = dirty_vram_range_find_gfn(v->domain, gfn);
> +                if ( range )
> +                    range->dirty_count++;
> +            }
>              paging_mark_dirty(v->domain, mfn_x(mfn));
> -            p2m_change_type(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw);
> +            paging_unlock(v->domain);

This is much nicer than the previous version, but I think it would be
even better if this bookkeeping went into paging_mark_dirty() so that
the other callers of paging_mark_dirty() also DTRT with the vram map.
That would avoid leaking mm-locks.h into this non-mm code, too.

Then this change becomes just swapping the order of the two lines (and
perhaps a comment to say why).

> diff --git a/xen/arch/x86/mm/dirty_vram.c b/xen/arch/x86/mm/dirty_vram.c
> new file mode 100644
> index 0000000..e3c7c1f
> --- /dev/null
> +++ b/xen/arch/x86/mm/dirty_vram.c
> @@ -0,0 +1,992 @@
> +/*
> + * arch/x86/mm/dirty_vram.c: Bookkeep/query dirty VRAM pages
> + * with support for multiple frame buffers.
> + *
> + * Copyright (c) 2012, Citrix Systems, Inc. (Robert Phillips)

Please bring in the copyright and authorship notices for the files you
copied code from.  That's at least mm/shadow/common.c and mm/hap/hap.c.

Apart from that this is looking good.  

Are you willing to take on maintainership of this feature (that is, to
respond to questions and fix bugs)?  If so, we should make an update to
the MAINTAINERS file for xen/arch/x86/mm/dirty_vram.c and
xen/include/asm-x86/dirty_vram.h.  That can happen separately, as it'll
need an ack from the other maintainers.

Cheers,

Tim.

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