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

[Robert Phillips <robert.phillips@xxxxxxxxxx>]

Hi,

The updated patch was sent out just a few minutes ago.


> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Thursday, November 15, 2012 8:23 AM
> To: Robert Phillips
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH] Provide support for multiple frame buffers
> in Xen
> 
> 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.

Done

> 
> > 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.

Removed

> 
> > 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).

Done.  I had to split paging_mark_dirty() into two functions -- the old 
function does the gfn-to-mfn translation and then calls the new function 
paging_mark_dirty_gpfn() which does the heavy lifiting.  
The code above now calls the new function thereby bypassing the gfn-to-mfn 
translation.

> 
> > 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.

Done

> 
> 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)? 

Yes

 >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