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

[Tim Deegan <tim@xxxxxxx>]

At 14:15 -0400 on 16 Oct (1350396902), Robert Phillips wrote:
> From: Robert Phillips <robert.phillips@xxxxxxxxxxxxxxxxxxx>
> 
> 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.
> 
> Signed-Off-By: Robert Phillips <robert.phillips@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/Makefile            |    3 +-
>  xen/arch/x86/hvm/dirty_vram.c        |  878 
> ++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c               |    4 +-
>  xen/arch/x86/mm/hap/hap.c            |  140 +-----
>  xen/arch/x86/mm/paging.c             |  232 ++++-----
>  xen/arch/x86/mm/shadow/common.c      |  335 +++++++------
>  xen/arch/x86/mm/shadow/multi.c       |  169 +++----
>  xen/arch/x86/mm/shadow/multi.h       |    7 +-
>  xen/arch/x86/mm/shadow/types.h       |    1 +
>  xen/include/asm-x86/hap.h            |    4 -
>  xen/include/asm-x86/hvm/dirty_vram.h |  157 ++++++
>  xen/include/asm-x86/hvm/domain.h     |    2 +-
>  xen/include/asm-x86/paging.h         |   22 +-
>  xen/include/asm-x86/shadow.h         |    6 -
>  14 files changed, 1403 insertions(+), 557 deletions(-)

Wow.  That's a bunch of code! :)  Thanks for sending it, and for the
document too.

You don't say why it's useful to have multiple framebuffers -- I take it
this is useful for laptop environments?  Also, I'm a bit surprised not
to see some hypercall API changes to go with it.

Reading the PDF you sent, it looks like you've implemented log-dirty in
two new ways: by scanning the shadow PTEs and by scanning EPT entries.
But you also leave the old ways (trap-on-access and populate a bitmap) 
despite saying "The eliminated code was complex, buggy and unnecessary".
If the bitmap-tracking code is really buggy we need to know about it, as
that's what we use for live migration!  What's the problem with it?

I've had a brief skim of the code, and it looks rather over-complex for
what it does.  I'm not sure what the purpose of all these linked lists
of ranges is -- couldn't this just be done with a struct rangeset
describing which areas are being tracked?  Then since we already
maintain a sparse bitmap to hold the dirty log we don't need any other
metadata, or other ways of checking dirtiness.

Style nits: you're not consistently following the Xen coding style, in
particular the spaces around parentheses in 'if's and 'for's.  Also
you've got some code in new files that's clearly at least derived from
old code but just got your own name and copyright at the head of the
file.  Please carry the copyright over from old code when you move it.

Cheers,

Tim.


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