[Xen-devel] Re: Linux Stubdom Problem

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 22 Aug 2011, Jiageng Yu wrote:
> Hi Stefano,
> 
>      I am trying to fix the vram mapping issue. That is my new patch.
> It is still needed to debug. Please check it for me and make sure I am
> running on the right way.
> 
>     I define a new enmu type Stubdom_Vga_State which is used to notify
> xen_remap_bucket whether to map the vram memory. In
> fbdev_pv_display_prepare function, I map the xen_fbfront memory to
> qemu (fb_mem) and directly incoke ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH,
> &ioctlx) to map the vram of hvm guest.
> 

The current implementation of IOCTL_PRIVCMD_MMAPBATCH is only capable of
mapping foreign mfns into the address space of the caller, while we need
to remap a set of pages already mapped in the address space of the
caller to some gmfns of a foreign domain. In other words we need the
same functionality but in the other direction.

First of all we need an hypercall to remap a given mfn to a guest gmfn
and currently there are no hypercalls that do that, so we need to add a
new one or extend an existing hypercall.
I suggest extending xc_domain_add_to_physmap with a new XENMAPSPACE,
called XENMAPSPACE_mfn that takes an mfn and maps it into a particular
gmfn.


>From the Xen point of view we need to add a new XENMAPSPACE_mfn case to
xen/arch/x86/mm.c:arch_memory_op, the basic implementation should be
something like the following (uncompiled, untested):

diff -r a79c1d5b946e xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c Fri Aug 19 10:00:25 2011 +0100
+++ b/xen/arch/x86/mm.c Mon Aug 22 17:51:41 2011 +0000
@@ -4618,6 +4618,16 @@ long arch_memory_op(int op, XEN_GUEST_HA
             page = mfn_to_page(mfn);
             break;
         }
+        case XENMAPSPACE_mfn:
+        {
+            if ( !IS_PRIV_FOR(current->domain, d) )
+                return -EINVAL;
+            mfn = xatp.idx;
+            page = mfn_to_page(mfn);
+            break;
+        }
         default:
             break;
         }
@@ -4648,10 +4658,12 @@ long arch_memory_op(int op, XEN_GUEST_HA
         }
 
         /* Unmap from old location, if any. */
-        gpfn = get_gpfn_from_mfn(mfn);
-        ASSERT( gpfn != SHARED_M2P_ENTRY );
-        if ( gpfn != INVALID_M2P_ENTRY )
-            guest_physmap_remove_page(d, gpfn, mfn, 0);
+        if ( xatp.space != XENMAPSPACE_mfn && d != current->domain ) {
+            gpfn = get_gpfn_from_mfn(mfn);
+            ASSERT( gpfn != SHARED_M2P_ENTRY );
+            if ( gpfn != INVALID_M2P_ENTRY )
+                guest_physmap_remove_page(d, gpfn, mfn, 0);
+        }
 
         /* Map at new location. */
         rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);


Unfortunately qemu doesn't know how to find the mfns corresponding to
the mmap'ed framebuffer and I would rather avoid writing a pagetable
walker in qemu.
Thus we need to use the linux kernel to do the virtual address to mfn
translation and issue the hypercall.
We can add a new privcmd ioctl IOCTL_PRIVCMD_FOREIGN_MAP: qemu calls this
ioctl with the mmap'ed framebuffer address, the size of the framebuffer
and the destination gmfns.
The implementation of the ioctl in the kernel would:

- find the list of mfns corresponding to the arguments, using
arbitrary_virt_to_machine;

- for each mfn, call the hypercall we extended with the appropriate
arguments, it would end up being
HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);

- there would be no "if (!xen_initial_domain())" check, because this
ioctl can be called by stubdoms.


So the call trace would be:

qemu: ioctl(fd, IOCTL_PRIVCMD_FOREIGN_MAP, &ioctlx);

|
v

linux: HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);

|
v

xen: guest_physmap_add_page


Has anybody any better ideas?


> diff --git a/fbdev.c b/fbdev.c
> index a601b48..f7ff682 100644
> --- a/fbdev.c
> +++ b/fbdev.c
> @@ -30,6 +30,12 @@
>  #include "sysemu.h"
>  #include "pflib.h"
> 
> +#include "hw/xen_backend.h"
> +#include <xen/hvm/params.h>
> +#include <sys/ioctl.h>
> +#include "xenctrlosdep.h"
> +#include <xen/privcmd.h>
> +
>  /* -------------------------------------------------------------------- */
> 
>  /* file handles */
> @@ -541,29 +547,23 @@ static void fbdev_cleanup(void)
>      }
>  }
> 
> -static int fbdev_init(const char *device)
> +static int fbdev_init(int prot, unsigned long size)
>  {
>      struct vt_stat vts;
>         unsigned long page_mask;
>      char ttyname[32];
> 
>      /* open framebuffer */
> -    if (device == NULL) {
> -       device = getenv("FRAMEBUFFER");
> -    }
> -    if (device == NULL) {
> -       device = "/dev/fb0";
> -    }
> -    fb = open(device, O_RDWR);
> +    fb = open("/dev/fb0", O_RDWR);
>      if (fb == -1) {
> -       fprintf(stderr, "open %s: %s\n", device, strerror(errno));
> +        fprintf(stderr, "open /dev/fb0: %s\n", strerror(errno));
>          return -1;
>      }
> 
>      /* open virtual console */
>      tty = 0;
>      if (ioctl(tty, VT_GETSTATE, &vts) < 0) {
>              fprintf(stderr, "Not started from virtual terminal, trying to
> open one.\n");
> 
>          snprintf(ttyname, sizeof(ttyname), "/dev/tty0");
>          tty = open(ttyname, O_RDWR);
> @@ -632,14 +632,14 @@ static int fbdev_init(const char *device)
>         goto err;
>      }
> 
>      page_mask = getpagesize()-1;
>      fb_mem_offset = (unsigned long)(fb_fix.smem_start) & page_mask;
> -    fb_mem = mmap(NULL,fb_fix.smem_len+fb_mem_offset,
> -                 PROT_READ|PROT_WRITE,MAP_SHARED,fb,0);
> +    fb_mem = mmap(NULL, size << XC_PAGE_SHIFT, prot, MAP_SHARED, fb, 0);
>      if (fb_mem == MAP_FAILED) {
>         perror("mmap");
>         goto err;
>      }
> +
>      /* move viewport to upper left corner */
>      if (fb_var.xoffset != 0 || fb_var.yoffset != 0) {
>         fb_var.xoffset = 0;
> @@ -930,3 +928,71 @@ void fbdev_display_uninit(void)
>      qemu_remove_exit_notifier(&exit_notifier);
>      uninit_mouse();
>  }

I would rather avoid modifing fbdev_init and add a new function to
xen-all.c that independently mmaps the xen_fbfront buffer with the right
size and maps it into the guest.


> +int fbdev_pv_display_prepare(unsigned long domid, int prot, const
> unsigned long *arr,
> +                                                               int *err, 
> unsigned int num)
> +{
> +       xen_pfn_t *pfn;
> +       privcmd_mmapbatch_t ioctlx;
> +       int fd,i,rc;
> +
> +    if (fbdev_init(prot,num) != 0) {
> +        exit(1);
> +    }
> +
> +       pfn = malloc(num * sizeof(*pfn));
> +       if(!pfn){
> +               errno = -ENOMEM;
> +               rc = -1;
> +               return rc;
> +       }
> +       memcpy(pfn, arr, num*sizeof(*arr));
> +
> +       fd = open("/proc/xen/privcmd", O_RDWR);
> +       if(fd == -1){
> +               fprintf(stderr,"Could not obtain handle on privcmd device\n");
> +               exit(1);
> +       }
> +
> +       ioctlx.num = num;
> +       ioctlx.dom = domid;
> +       ioctlx.addr = (unsigned long)fb_mem;
> +       ioctlx.arr = pfn;
> +
> +       rc = ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx);
> +
> +       for(i=0; i<num; i++)
> +       {
> +               switch(pfn[i] ^ arr[i])
> +               {
> +                       case 0:
> +                               err[i] = rc != -ENOENT ? rc:0;
> +                               continue;
> +                       default:
> +                               err[i] = -EINVAL;
> +                               continue;
> +               }
> +               break;
> +       }
> +
> +       free(pfn);
> +
> +       if (rc == -ENOENT && i == num)
> +               rc=0;
> +       else if(rc)
> +       {
> +               errno = -rc;
> +               rc = -1;
> +       }
> +
> +       if(rc < 0)
> +       {
> +               fprintf(stderr,"privcmd ioctl failed\n");
> +               munmap(fb_mem, num << XC_PAGE_SHIFT);
> +               return NULL;
> +       }
> +
> +       close(fd);
> +
> +       return fb_mem;
> +}

We shouldn't use IOCTL_PRIVCMD_MMAPBATCH, we need a new ioctl, see above.


> diff --git a/hw/vga.c b/hw/vga.c
> index 0f54734..de7dd85 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -28,6 +28,7 @@
>  #include "vga_int.h"
>  #include "pixel_ops.h"
>  #include "qemu-timer.h"
> +#include "xen_backend.h"
> 
>  //#define DEBUG_VGA
>  //#define DEBUG_VGA_MEM
> @@ -2237,7 +2238,12 @@ void vga_common_init(VGACommonState *s, int 
> vga_ram_size)
>      s->is_vbe_vmstate = 0;
>  #endif
>      s->vram_offset = qemu_ram_alloc(NULL, "vga.vram", vga_ram_size);
> +#ifdef CONFIG_STUBDOM
> +       stubdom_vga_state = VGA_INIT_READY;
> +#endif
>      s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
>      s->vram_size = vga_ram_size;
>      s->get_bpp = vga_get_bpp;
>      s->get_offsets = vga_get_offsets;
> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
> index c506dfe..f4ecce4 100644
> --- a/hw/xen_backend.c
> +++ b/hw/xen_backend.c
> @@ -48,6 +48,10 @@ XenGnttab xen_xcg = XC_HANDLER_INITIAL_VALUE;
>  struct xs_handle *xenstore = NULL;
>  const char *xen_protocol;
> 
> +#ifdef CONFIG_STUBDOM
> +enum Stubdom_Vga_State stubdom_vga_state=0;
> +#endif
> +
>  /* private */
>  static QTAILQ_HEAD(XenDeviceHead, XenDevice) xendevs =
> QTAILQ_HEAD_INITIALIZER(xendevs);
>  static int debug = 0;
> diff --git a/hw/xen_backend.h b/hw/xen_backend.h
> index 3305630..36167d1 100644
> --- a/hw/xen_backend.h
> +++ b/hw/xen_backend.h
> @@ -60,6 +60,16 @@ extern XenXC xen_xc;
>  extern struct xs_handle *xenstore;
>  extern const char *xen_protocol;
> 
> +#ifdef CONFIG_STUBDOM
> +/* linux stubdom vga initialization*/
> +enum Stubdom_Vga_State{
> +       VGA_INIT_WAIT = 0,
> +       VGA_INIT_READY,
> +       VGA_INIT_COMPLETE
> +};
> +extern enum Stubdom_Vga_State stubdom_vga_state;
> +#endif
> +
>  /* xenstore helper functions */
>  int xenstore_write_str(const char *base, const char *node, const char *val);
>  int xenstore_write_int(const char *base, const char *node, int ival);
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 007136a..e615f98 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -20,6 +20,7 @@
>  #include "xen-mapcache.h"
>  #include "trace.h"
> 
> +#include "hw/xen.h"
> 
>  //#define MAPCACHE_DEBUG
> 
> @@ -144,8 +145,19 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>      }
> 
> +#ifdef CONFIG_STUBDOM
> +       if(stubdom_vga_state == VGA_INIT_READY){
> +               fprintf(stderr,"xen_remap_bucket: start linux stubdom vga\n");
> +               vaddr_base = fbdev_pv_display_prepare(xen_domid, 
> PROT_READ|PROT_WRITE,
> +                                                                             
>   pfns, err, nb_pfn);
> +               stubdom_vga_state = VGA_INIT_COMPLETE;
> +       }else
> +       vaddr_base = xc_map_foreign_bulk(xen_xc, xen_domid, 
> PROT_READ|PROT_WRITE,
> +                                       pfns, err, nb_pfn);
> +#else
>      vaddr_base = xc_map_foreign_bulk(xen_xc, xen_domid, PROT_READ|PROT_WRITE,
>                                       pfns, err, nb_pfn);
> +#endif
>      if (vaddr_base == NULL) {
>          perror("xc_map_foreign_bulk");
>          exit(-1);
> 
> 

I don't like the stubdom_vga_init approach: in general it is a good idea
to avoid state machines unless necessary.
I would implement a new function called xen_vga_ram_map in xen-all.c
that mmaps the xen_fbfront buffer and uses the new ioctl to
map the buffer into the guest.
xen_vga_ram_map should be called instead of xen_ram_alloc by
qemu_ram_alloc_from_ptr if name == "vga.vram".


Another suggestion: before starting to write any new lines of code, I
would make sure that mmaping the /dev/fb device actually works correctly.
For debugging purposes you can try to modify fbdev_init and write
something to the framebuffer right after the mmap, to see if the new
pattern is displayed correctly on the screen.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel