Re: [Qemu-devel] [PATCH 30/40] xenner: libxc emu: memory mapping

[Alexander Graf]

On 01.11.2010, at 11:12, malc wrote:

> On Mon, 1 Nov 2010, Alexander Graf wrote:
> 
>> Xenner emulates parts of libxc, so we can not use the real xen infrastructure
>> when running xen pv guests without xen.
>> 
>> This patch adds support for guest memory mapping.
>> 
>> Signed-off-by: Alexander Graf <address@hidden>
>> ---
>> hw/xenner_libxc_if.c |  124 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 124 insertions(+), 0 deletions(-)
>> create mode 100644 hw/xenner_libxc_if.c
>> 
>> diff --git a/hw/xenner_libxc_if.c b/hw/xenner_libxc_if.c
>> new file mode 100644
>> index 0000000..7ccd3c0
>> --- /dev/null
>> +++ b/hw/xenner_libxc_if.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + *  Copyright (C) Red Hat 2007
>> + *  Copyright (C) Novell Inc. 2010
>> + *
>> + *  Author(s): Gerd Hoffmann <address@hidden>
>> + *             Alexander Graf <address@hidden>
>> + *
>> + *  Xenner Emulation -- memory management
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; under version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License along
>> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <sys/mman.h>
>> +#include <xenctrl.h>
>> +
>> +#include "hw.h"
>> +#include "xen_interfaces.h"
>> +#include "xenner.h"
>> +
>> +/* ------------------------------------------------------------- */
>> +
>> +static int _qemu_open(void)
>> +{
>> +    return 42;
>> +}
> 
> Identifiers with leading underscore are reserved in this context.

Yeah. Will change to xc_qemu_open.

> 
>> +
>> +static int qemu_close(int xc_handle)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void *qemu_map_foreign_range(int xc_handle, uint32_t dom,
>> +                                    int size, int prot, unsigned long mfn)
>> +{
>> +    target_phys_addr_t addr, len;
>> +    void *ptr;
>> +
>> +    addr = (target_phys_addr_t)mfn << PAGE_SHIFT;
>> +    len = size;
>> +    ptr = cpu_physical_memory_map(addr, &len, 1);
>> +
>> +    if (len != size) {
>> +        fprintf(stderr, "%s: couldn't allocate %d bytes\n", __FUNCTION__, 
>> size);
>> +        return NULL;
>> +    }
>> +
>> +    return ptr;
>> +}
>> +
>> +static void *qemu_map_foreign_batch(int xc_handle, uint32_t dom, int prot,
>> +                                    xen_pfn_t *arr, int num)
>> +{
>> +    ram_addr_t offset;
>> +    void *ptr;
>> +    int i;
>> +    target_phys_addr_t len = num * TARGET_PAGE_SIZE;
>> +
>> +    char filename[] = "/dev/shm/qemu-vmcore.XXXXXX";
>> +    int rc, fd;
>> +
>> +    fd = mkstemp(filename);
>> +    if (fd == -1) {
>> +        fprintf(stderr, "mkstemp(%s): %s\n", filename, strerror(errno));
>> +        return NULL;
>> +    };
>> +    unlink(filename);
>> +
>> +    rc = ftruncate(fd, len);
>> +    if (rc != 0) {
>> +        fprintf(stderr, "ftruncate(0x%" PRIx64 "): %s\n",
>> +                (uint64_t)len, strerror(errno));
>> +        return NULL;
>> +    }
>> +
>> +    ptr = mmap(NULL, len, PROT_WRITE | PROT_READ, MAP_SHARED | MAP_POPULATE,
>> +               fd, 0);
> 
> mmap can fail.

Everything can fail, but what should we do if it does? Doesn't a failing mmap 
indicate something went really wrong?

Either way, this whole function is just plain wrong. It breaks once two callers 
try to access the same memory for example. I'm very open to suggestions on how 
to replace it with something that works.


Alex