[Xen-devel] Re: Linux Stubdom Problem

[Jiageng Yu <yujiageng734@xxxxxxxxx>]

2011/8/23 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


Yes. I am already working on it.

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

I have question here. I think xen_vga_ram_map() is used to instead of
xc_map_foreign_bulk() which maps hvm guest's vram.

vga_common_init
    -->qemu_get_ram_ptr
             -->xen_map_cache
                     -->xen_remap_bucket
                               -->xc_map_foreign_bulk

The xen_ram_alloc() calls xc_domain_populate_physmap_exact() to
increase the physical memory of hvm guest. And the increased physical
memory becomes hvm guest's vram.

For the xen_remap_bucket(), there is no "vga.vram" input parameter. So
we need the notification mechanism to invoke xen_vga_ram_map().


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

I have tested the xen_fbfront driver in linux stubdom. As shown in
attached file, I could print a small colored region.

Attachment: Screenshot-QEMU (fedora14-dm)-4.png
Description: PNG image

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