[Qemu-devel] Help writing a trivial device

[Qemu-devel] Help writing a trivial device

[Lluís Vilanova]

Hi. I started writing a trivial device on QEMU that should get called on every
read and write on the memory it provides.

The problems are that:

1) Cannot start QEMU with KVM when the device is enabled:
       kvm_set_phys_mem: error registering slot: Invalid argument

2) The driver never gets called on a read/write to its memory

I'm sure this is due to some error in my code, but I'm clueless as to what it
could be.


The testing system is a Linux 2.6.32, with this:

int fd = open("/sys/devices/pci0000:00/000000:00:004.00/resource0", O_RDWR);
void *addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
printf("-> %ld\n", *(uint64_t*)addr);


The device is something like (some code changed for brevity):

typedef struct State
{
    PCIDevice dev;
    MemoryRegion control;
} State;


static uint64_t control_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
{
    return 0xcafe;
}

static void control_io_write(void *opaque, target_phys_addr_t addr, uint64_t data, unsigned size)
{
    /* do something */
}

static const MemoryRegionOps control_ops = {
    .read = control_io_read,
    .write = control_io_write,
    .endianness = DEVICE_NATIVE_ENDIAN,
    .valid = {
        .min_access_size = 8,
        .max_access_size = 8,
    },
};


static int init(PCIDevice *dev)
{
    State *s = DO_UPCAST(State, dev, dev);

    memory_region_init_io(&s->control, &control_ops, s, "backdoor.control",
                          TARGET_PAGE_SIZE);
    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->control);

    return 0;
}

static int fini(PCIDevice *dev)
{
    State *s = DO_UPCAST(State, dev, dev);

    memory_region_destroy(&s->control);

    return 0;
}


static PCIDeviceInfo info = {
    .qdev.name  = "foo",
    .qdev.size  = sizeof(State),
    .init       = init,
    .exit       = fini,
    .vendor_id  = PCI_VENDOR_ID_REDHAT_QUMRANET,
    .device_id  = 0x1005,
    .class_id   = PCI_CLASS_MEMORY_RAM,
};

static void register_devices(void)
{
    pci_qdev_register(&info);
}

device_init(register_devices)



Is there something blatantly wrong in the device code?


Thanks a lot,
    Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] Help writing a trivial device

[Stefan Hajnoczi]

On Mon, Sep 26, 2011 at 12:23:21AM +0200, Lluís Vilanova wrote:
> 1) Cannot start QEMU with KVM when the device is enabled:
>        kvm_set_phys_mem: error registering slot: Invalid argument
> 
> 2) The driver never gets called on a read/write to its memory

If I add #include "pci.h" at the top of the file, change
TARGET_PAGE_SIZE to 4096 (generic device code is not supposed to be
target-dependent), and throw foo.o into Makefile.objs it builds here.

It launches successfully with -enable-kvm and I can see the device
inside a VM.  My qemu.git/HEAD is
1ce9ce6a6184f0192015ba9adf1123d89cd47a7b.

I only had a few minutes and couldn't test reading from BAR0, but you
might want to get 32-bit reads working first before trying 64-bit.

Stefan

Re: [Qemu-devel] Help writing a trivial device

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Mon, Sep 26, 2011 at 12:23:21AM +0200, Lluís Vilanova wrote:
>> 1) Cannot start QEMU with KVM when the device is enabled:
>> kvm_set_phys_mem: error registering slot: Invalid argument
>> 
>> 2) The driver never gets called on a read/write to its memory

> If I add #include "pci.h" at the top of the file, change
> TARGET_PAGE_SIZE to 4096 (generic device code is not supposed to be
> target-dependent), and throw foo.o into Makefile.objs it builds here.

> It launches successfully with -enable-kvm and I can see the device
> inside a VM.  My qemu.git/HEAD is
> 1ce9ce6a6184f0192015ba9adf1123d89cd47a7b.

Well, my intent is to implement the backdoor communication channel I rambled
about some time ago. Now, instead of overloading some unused opcodes, I use a
virtual device that is target agnostic (as someone wisely suggested).

As the device is mmap'ed into userspace to interact with it, I thought it made
sense to use TARGET_PAGE_SIZE to establish the device's mappable ranges.

Note that the device is not compiled with all the libhw code, so
TARGET_PAGE_SIZE is not poisoned.

In any case, now I've just seen that the code I sent does indeed (almost)
work... the offending code is in the part I did not send. The second bar uses a
simple malloc'ed piece of data that the user can use as a scratchpad for the
communication with QEMU, so that the flow is:

  - guest writes data into data channel
  - guest writes anything into the control channel
  - do in QEMU whatever is encoded in the data channel

It is the second bar that generates the problems with KVM:

    s->data_ptr = g_malloc(s->size);
    memory_region_init_ram_ptr(&s->data, &s->dev.qdev, "backdoor.data",
                               s->size, s->data_ptr);
    pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->data);


> I only had a few minutes and couldn't test reading from BAR0, but you
> might want to get 32-bit reads working first before trying 64-bit.

The next problem is using 64bit reads and writes... if I set min_access_size and
max_access_size to 4, I get the calls to 'control_io_read', but not with 8...

I've attached the real code I'm using, just to make it easier to test.


Thanks a lot,
    Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] Help writing a trivial device

[Lluís Vilanova]

[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]

Lluís Vilanova writes:

> Stefan Hajnoczi writes:
>> On Mon, Sep 26, 2011 at 12:23:21AM +0200, Lluís Vilanova wrote:
>>> 1) Cannot start QEMU with KVM when the device is enabled:
>>> kvm_set_phys_mem: error registering slot: Invalid argument
>>> 
>>> 2) The driver never gets called on a read/write to its memory

>> If I add #include "pci.h" at the top of the file, change
>> TARGET_PAGE_SIZE to 4096 (generic device code is not supposed to be
>> target-dependent), and throw foo.o into Makefile.objs it builds here.

>> It launches successfully with -enable-kvm and I can see the device
>> inside a VM.  My qemu.git/HEAD is
>> 1ce9ce6a6184f0192015ba9adf1123d89cd47a7b.

> Well, my intent is to implement the backdoor communication channel I rambled
> about some time ago. Now, instead of overloading some unused opcodes, I use a
> virtual device that is target agnostic (as someone wisely suggested).

> As the device is mmap'ed into userspace to interact with it, I thought it made
> sense to use TARGET_PAGE_SIZE to establish the device's mappable ranges.

> Note that the device is not compiled with all the libhw code, so
> TARGET_PAGE_SIZE is not poisoned.

> In any case, now I've just seen that the code I sent does indeed (almost)
> work... the offending code is in the part I did not send. The second bar uses a
> simple malloc'ed piece of data that the user can use as a scratchpad for the
> communication with QEMU, so that the flow is:

>   - guest writes data into data channel
>   - guest writes anything into the control channel
>   - do in QEMU whatever is encoded in the data channel

> It is the second bar that generates the problems with KVM:

s-> data_ptr = g_malloc(s->size);
>     memory_region_init_ram_ptr(&s->data, &s->dev.qdev, "backdoor.data",
s-> size, s->data_ptr);
>     pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->data);


>> I only had a few minutes and couldn't test reading from BAR0, but you
>> might want to get 32-bit reads working first before trying 64-bit.

> The next problem is using 64bit reads and writes... if I set min_access_size and
> max_access_size to 4, I get the calls to 'control_io_read', but not with 8...

> I've attached the real code I'm using, just to make it easier to test.

Sorry, I forgot the attachment.

The attached virtual device is just a proxy to the user-provided functions
'qemu_backdoor_init' and 'qemu_backdoor', which implement the semantics of the
backdoor channel.

The code in "#if 1" is the piece that offends KVM.


Thanks,
   Lluis



[-- Attachment #2: softmmu.c --]
[-- Type: text/plain, Size: 3162 bytes --]

/*
 * Virtual device for user-defined guest<->QEMU communication.
 *
 * Copyright (C) 2011 Lluís Vilanova <vilanova@ac.upc.edu>
 *
 * This work is licensed under the terms of the GNU GPL, version 2 or later.
 * See the COPYING file in the top-level directory.
 */

#include "hw/pci.h"
#include "backdoor/qemu/qemu-backdoor.h"


#define PAGE_SIZE TARGET_PAGE_SIZE
#define CTRL_BYTES sizeof(uint64_t)


typedef struct State
{
    PCIDevice dev;

    uint8_t pages;
    uint64_t size;

    uint64_t cmd;

    void *data_ptr;
    MemoryRegion data;
    MemoryRegion control;
} State;


static uint64_t control_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
{
    State *s = opaque;

    uint64_t res = ldq_p(&s->size);
    uint8_t *resb = (uint8_t*)&res;
    return resb[addr % CTRL_BYTES];
}

static void control_io_write(void *opaque, target_phys_addr_t addr, uint64_t data, unsigned size)
{
    State *s = opaque;

    uint8_t *cmdb = (uint8_t*)&s->cmd;
    cmdb[addr % CTRL_BYTES] = (uint8_t)data;

    if ((addr + size) % CTRL_BYTES == 0) {
        qemu_backdoor(ldq_p(&s->cmd), s->data_ptr);
    }
}

static const MemoryRegionOps control_ops = {
    .read = control_io_read,
    .write = control_io_write,
    .endianness = DEVICE_NATIVE_ENDIAN,
    .impl = {
        .min_access_size = 1,
        .max_access_size = 1,
    },
};


static int init(PCIDevice *dev)
{
    State *s = DO_UPCAST(State, dev, dev);

    if (s->pages < 1) {
        fprintf(stderr, "error: backdoor: "
                "the data channel must have one or more pages\n");
        return -1;
    }
    s->size = s->pages * PAGE_SIZE;

    pci_set_word(s->dev.config + PCI_COMMAND,
                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY);

    memory_region_init_io(&s->control, &control_ops, s, "backdoor.control",
                          PAGE_SIZE);
    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->control);

#if 1                                   /* KVM doesn't like it */
    s->data_ptr = g_malloc(s->size);
    memory_region_init_ram_ptr(&s->data, &s->dev.qdev, "backdoor.data",
                               s->size, s->data_ptr);
    pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->data);
#endif

    qemu_backdoor_init(s->size);

    return 0;
}

static int fini(PCIDevice *dev)
{
    State *s = DO_UPCAST(State, dev, dev);

    g_free(s->data_ptr);
    memory_region_destroy(&s->data);
    memory_region_destroy(&s->control);

    return 0;
}


static PCIDeviceInfo info = {
    .qdev.name  = "backdoor",
    .qdev.desc  = "Backdoor communication channel",
    .qdev.size  = sizeof(State),
    .init       = init,
    .exit       = fini,
    .vendor_id  = PCI_VENDOR_ID_REDHAT_QUMRANET,
    .device_id  = PCI_DEVICE_ID_BACKDOOR,
    .class_id   = PCI_CLASS_MEMORY_RAM,
    .qdev.props = (Property[]) {
        DEFINE_PROP_UINT8("pages", State, pages, 1),
        DEFINE_PROP_END_OF_LIST(),
    }
};

static void register_device(void)
{
    pci_qdev_register(&info);
}

device_init(register_device)

[-- Attachment #3: Type: text/plain, Size: 218 bytes --]


-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] Help writing a trivial device

[Stefan Hajnoczi]

On Mon, Sep 26, 2011 at 08:03:12PM +0200, Lluís Vilanova wrote:
> #if 1                                   /* KVM doesn't like it */
>     s->data_ptr = g_malloc(s->size);
>     memory_region_init_ram_ptr(&s->data, &s->dev.qdev, "backdoor.data",
>                                s->size, s->data_ptr);
>     pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->data);
> #endif

Have you tried page-aligning data_ptr with qemu_memalign() or doing an
anonymous mmap instead of g_malloc()?

>From virt/kvm/kvm_main.c:__kvm_set_memory_region():

/* General sanity checks */
if (mem->memory_size & (PAGE_SIZE - 1))
	goto out;
if (mem->guest_phys_addr & (PAGE_SIZE - 1))
	goto out;
/* We can read the guest memory with __xxx_user() later on. */
if (user_alloc &&
    ((mem->userspace_addr & (PAGE_SIZE - 1)) ||
     !access_ok(VERIFY_WRITE,
		(void __user *)(unsigned long)mem->userspace_addr,
		mem->memory_size)))
	goto out;

Stefan

Re: [Qemu-devel] Help writing a trivial device

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Mon, Sep 26, 2011 at 08:03:12PM +0200, Lluís Vilanova wrote:
>> #if 1                                   /* KVM doesn't like it */
>> s-> data_ptr = g_malloc(s->size);
>> memory_region_init_ram_ptr(&s->data, &s->dev.qdev, "backdoor.data",
>>                            s-> size, s->data_ptr);
>> pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->data);
>> #endif

> Have you tried page-aligning data_ptr with qemu_memalign() or doing an
> anonymous mmap instead of g_malloc()?

> From virt/kvm/kvm_main.c:__kvm_set_memory_region():

> /* General sanity checks */
> if (mem->memory_size & (PAGE_SIZE - 1))
> 	goto out;
> if (mem->guest_phys_addr & (PAGE_SIZE - 1))
> 	goto out;
> /* We can read the guest memory with __xxx_user() later on. */
> if (user_alloc &&
>     ((mem->userspace_addr & (PAGE_SIZE - 1)) ||
>      !access_ok(VERIFY_WRITE,
> 		(void __user *)(unsigned long)mem->userspace_addr, mem-> memory_size)))
> 	goto out;

Damn! It's so obvious I even feel ashamed :)


Thanks a lot,
    Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] Help writing a trivial device

[Zhi Yong Wu]

2011/9/26 Lluís Vilanova <vilanova@ac.upc.edu>:
> Stefan Hajnoczi writes:
>
>> On Mon, Sep 26, 2011 at 12:23:21AM +0200, Lluís Vilanova wrote:
>>> 1) Cannot start QEMU with KVM when the device is enabled:
>>> kvm_set_phys_mem: error registering slot: Invalid argument
>>>
>>> 2) The driver never gets called on a read/write to its memory
>
>> If I add #include "pci.h" at the top of the file, change
>> TARGET_PAGE_SIZE to 4096 (generic device code is not supposed to be
>> target-dependent), and throw foo.o into Makefile.objs it builds here.
>
>> It launches successfully with -enable-kvm and I can see the device
>> inside a VM.  My qemu.git/HEAD is
>> 1ce9ce6a6184f0192015ba9adf1123d89cd47a7b.
>
> Well, my intent is to implement the backdoor communication channel I rambled
> about some time ago. Now, instead of overloading some unused opcodes, I use a
> virtual device that is target agnostic (as someone wisely suggested).
>
> As the device is mmap'ed into userspace to interact with it, I thought it made
> sense to use TARGET_PAGE_SIZE to establish the device's mappable ranges.
>
> Note that the device is not compiled with all the libhw code, so
> TARGET_PAGE_SIZE is not poisoned.
>
> In any case, now I've just seen that the code I sent does indeed (almost)
> work... the offending code is in the part I did not send. The second bar uses a
> simple malloc'ed piece of data that the user can use as a scratchpad for the
> communication with QEMU, so that the flow is:
>
>  - guest writes data into data channel
>  - guest writes anything into the control channel
>  - do in QEMU whatever is encoded in the data channel
>
> It is the second bar that generates the problems with KVM:
It can work in my env with the codes below, and i can see the device.
But it is pity that i don't know to test the two bar.

>
>    s->data_ptr = g_malloc(s->size);
>    memory_region_init_ram_ptr(&s->data, &s->dev.qdev, "backdoor.data",
>                               s->size, s->data_ptr);
>    pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->data);
>
>
>> I only had a few minutes and couldn't test reading from BAR0, but you
>> might want to get 32-bit reads working first before trying 64-bit.
>
> The next problem is using 64bit reads and writes... if I set min_access_size and
> max_access_size to 4, I get the calls to 'control_io_read', but not with 8...
>
> I've attached the real code I'm using, just to make it easier to test.
>
>
> Thanks a lot,
>    Lluis
>
> --
>  "And it's much the same thing with knowledge, for whenever you learn
>  something new, the whole world becomes that much richer."
>  -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
>  Tollbooth
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] Help writing a trivial device

[Avi Kivity]

On 09/26/2011 03:54 PM, Lluís Vilanova wrote:
> It is the second bar that generates the problems with KVM:
>
>      s->data_ptr = g_malloc(s->size);
>      memory_region_init_ram_ptr(&s->data,&s->dev.qdev, "backdoor.data",
>                                 s->size, s->data_ptr);
>      pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,&s->data);
>

s->data_ptr is not properly aligned.  Use memory_region_init_ram().

> >  I only had a few minutes and couldn't test reading from BAR0, but you
> >  might want to get 32-bit reads working first before trying 64-bit.
>
> The next problem is using 64bit reads and writes... if I set min_access_size and
> max_access_size to 4, I get the calls to 'control_io_read', but not with 8...
>

8-byte accesses are not yet supported.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Help writing a trivial device

[Zhi Yong Wu]

2011/9/26 Lluís Vilanova <vilanova@ac.upc.edu>:
> Hi. I started writing a trivial device on QEMU that should get called on every
> read and write on the memory it provides.
>
> The problems are that:
>
> 1) Cannot start QEMU with KVM when the device is enabled:
>       kvm_set_phys_mem: error registering slot: Invalid argument
>
> 2) The driver never gets called on a read/write to its memory
>
> I'm sure this is due to some error in my code, but I'm clueless as to what it
> could be.
>
>
> The testing system is a Linux 2.6.32, with this:
>
> int fd = open("/sys/devices/pci0000:00/000000:00:004.00/resource0", O_RDWR);
> void *addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> printf("-> %ld\n", *(uint64_t*)addr);
This is the way about how to test bar? Does bar1 correspond to guest
file /sys/devices/pci0000:00/000000:00:004.00/resource0?

>
>
> The device is something like (some code changed for brevity):
>
> typedef struct State
> {
>    PCIDevice dev;
>    MemoryRegion control;
> } State;
>
>
> static uint64_t control_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
> {
>    return 0xcafe;
> }
>
> static void control_io_write(void *opaque, target_phys_addr_t addr, uint64_t data, unsigned size)
> {
>    /* do something */
> }
>
> static const MemoryRegionOps control_ops = {
>    .read = control_io_read,
>    .write = control_io_write,
>    .endianness = DEVICE_NATIVE_ENDIAN,
>    .valid = {
>        .min_access_size = 8,
>        .max_access_size = 8,
>    },
> };
>
>
> static int init(PCIDevice *dev)
> {
>    State *s = DO_UPCAST(State, dev, dev);
>
>    memory_region_init_io(&s->control, &control_ops, s, "backdoor.control",
>                          TARGET_PAGE_SIZE);
>    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->control);
>
>    return 0;
> }
>
> static int fini(PCIDevice *dev)
> {
>    State *s = DO_UPCAST(State, dev, dev);
>
>    memory_region_destroy(&s->control);
>
>    return 0;
> }
>
>
> static PCIDeviceInfo info = {
>    .qdev.name  = "foo",
>    .qdev.size  = sizeof(State),
>    .init       = init,
>    .exit       = fini,
>    .vendor_id  = PCI_VENDOR_ID_REDHAT_QUMRANET,
>    .device_id  = 0x1005,
>    .class_id   = PCI_CLASS_MEMORY_RAM,
> };
>
> static void register_devices(void)
> {
>    pci_qdev_register(&info);
> }
>
> device_init(register_devices)
>
>
>
> Is there something blatantly wrong in the device code?
>
>
> Thanks a lot,
>    Lluis
>
> --
>  "And it's much the same thing with knowledge, for whenever you learn
>  something new, the whole world becomes that much richer."
>  -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
>  Tollbooth
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] Help writing a trivial device

[Lluís Vilanova]

Zhi Yong Wu writes:

> 2011/9/26 Lluís Vilanova <vilanova@ac.upc.edu>:
>> The testing system is a Linux 2.6.32, with this:
>> 
>> int fd = open("/sys/devices/pci0000:00/000000:00:004.00/resource0", O_RDWR);
>> void *addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>> printf("-> %ld\n", *(uint64_t*)addr);
> This is the way about how to test bar? Does bar1 correspond to guest
> file /sys/devices/pci0000:00/000000:00:004.00/resource0?

In linux, resourceN corresponds to the Nth BAR.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth