From: Glauber Costa <glommer@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org, avi@redhat.com
Subject: Re: [PATCH 2/5] reuse kvm_vm_ioctl
Date: Fri, 17 Jul 2009 11:21:49 -0300 [thread overview]
Message-ID: <20090717142149.GD4019@poweredge.glommer> (raw)
In-Reply-To: <20090717133543.GA4287@amt.cnet>
On Fri, Jul 17, 2009 at 10:35:43AM -0300, Marcelo Tosatti wrote:
> On Wed, Jul 15, 2009 at 12:31:40PM -0400, Glauber Costa wrote:
> > Start using kvm_vm_ioctl's code.
> > For type safety, delete vm_fd from kvm_context entirely, so the
> > compiler can play along with us helping to detect errors I might
> > have made.
> >
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> > kvm-all.c | 2 ++
> > qemu-kvm-x86.c | 18 +++++++++---------
> > qemu-kvm.c | 52 ++++++++++++++++++++++++++--------------------------
> > qemu-kvm.h | 6 +++---
> > 4 files changed, 40 insertions(+), 38 deletions(-)
> >
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 67908a7..9373d99 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -809,6 +809,7 @@ int kvm_ioctl(KVMState *s, int type, ...)
> >
> > return ret;
> > }
> > +#endif
> >
> > int kvm_vm_ioctl(KVMState *s, int type, ...)
> > {
> > @@ -827,6 +828,7 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
> > return ret;
> > }
> >
> > +#ifdef KVM_UPSTREAM
> > int kvm_vcpu_ioctl(CPUState *env, int type, ...)
> > {
> > int ret;
> > diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> > index 350f272..c7393cd 100644
> > --- a/qemu-kvm-x86.c
> > +++ b/qemu-kvm-x86.c
> > @@ -40,7 +40,7 @@ int kvm_set_tss_addr(kvm_context_t kvm, unsigned long addr)
> >
> > r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
> > if (r > 0) {
> > - r = ioctl(kvm->vm_fd, KVM_SET_TSS_ADDR, addr);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_TSS_ADDR, addr);
> > if (r == -1) {
> > fprintf(stderr, "kvm_set_tss_addr: %m\n");
> > return -errno;
> > @@ -82,7 +82,7 @@ static int kvm_create_pit(kvm_context_t kvm)
> > if (!kvm->no_pit_creation) {
> > r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_PIT);
> > if (r > 0) {
> > - r = ioctl(kvm->vm_fd, KVM_CREATE_PIT);
> > + r = kvm_vm_ioctl(kvm_state, KVM_CREATE_PIT);
> > if (r >= 0)
> > kvm->pit_in_kernel = 1;
> > else {
> > @@ -211,7 +211,7 @@ int kvm_create_memory_alias(kvm_context_t kvm,
> > .memory_size = len,
> > .target_phys_addr = target_phys,
> > };
> > - int fd = kvm->vm_fd;
> > + int fd = kvm_state->vmfd;
> > int r;
> > int slot;
> >
> > @@ -272,7 +272,7 @@ int kvm_get_pit(kvm_context_t kvm, struct kvm_pit_state *s)
> > int r;
> > if (!kvm->pit_in_kernel)
> > return 0;
> > - r = ioctl(kvm->vm_fd, KVM_GET_PIT, s);
> > + r = kvm_vm_ioctl(kvm_state, KVM_GET_PIT, s);
> > if (r == -1) {
> > r = -errno;
> > perror("kvm_get_pit");
> > @@ -285,7 +285,7 @@ int kvm_set_pit(kvm_context_t kvm, struct kvm_pit_state *s)
> > int r;
> > if (!kvm->pit_in_kernel)
> > return 0;
> > - r = ioctl(kvm->vm_fd, KVM_SET_PIT, s);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_PIT, s);
> > if (r == -1) {
> > r = -errno;
> > perror("kvm_set_pit");
> > @@ -299,7 +299,7 @@ int kvm_get_pit2(kvm_context_t kvm, struct kvm_pit_state2 *ps2)
> > int r;
> > if (!kvm->pit_in_kernel)
> > return 0;
> > - r = ioctl(kvm->vm_fd, KVM_GET_PIT2, ps2);
> > + r = kvm_vm_ioctl(kvm_state, KVM_GET_PIT2, ps2);
> > if (r == -1) {
> > r = -errno;
> > perror("kvm_get_pit2");
> > @@ -312,7 +312,7 @@ int kvm_set_pit2(kvm_context_t kvm, struct kvm_pit_state2 *ps2)
> > int r;
> > if (!kvm->pit_in_kernel)
> > return 0;
> > - r = ioctl(kvm->vm_fd, KVM_SET_PIT2, ps2);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_PIT2, ps2);
> > if (r == -1) {
> > r = -errno;
> > perror("kvm_set_pit2");
> > @@ -549,7 +549,7 @@ int kvm_set_shadow_pages(kvm_context_t kvm, unsigned int nrshadow_pages)
> > r = ioctl(kvm->fd, KVM_CHECK_EXTENSION,
> > KVM_CAP_MMU_SHADOW_CACHE_CONTROL);
> > if (r > 0) {
> > - r = ioctl(kvm->vm_fd, KVM_SET_NR_MMU_PAGES, nrshadow_pages);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_NR_MMU_PAGES, nrshadow_pages);
> > if (r == -1) {
> > fprintf(stderr, "kvm_set_shadow_pages: %m\n");
> > return -errno;
> > @@ -568,7 +568,7 @@ int kvm_get_shadow_pages(kvm_context_t kvm, unsigned int *nrshadow_pages)
> > r = ioctl(kvm->fd, KVM_CHECK_EXTENSION,
> > KVM_CAP_MMU_SHADOW_CACHE_CONTROL);
> > if (r > 0) {
> > - *nrshadow_pages = ioctl(kvm->vm_fd, KVM_GET_NR_MMU_PAGES);
> > + *nrshadow_pages = kvm_vm_ioctl(kvm_state, KVM_GET_NR_MMU_PAGES);
> > return 0;
> > }
> > #endif
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index 904e751..f09bcca 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -44,7 +44,7 @@ int kvm_pit_reinject = 1;
> > int kvm_nested = 0;
> >
> >
> > -static KVMState *kvm_state;
> > +KVMState *kvm_state;
> > kvm_context_t kvm_context;
> >
> > pthread_mutex_t qemu_mutex = PTHREAD_MUTEX_INITIALIZER;
> > @@ -332,7 +332,7 @@ static int kvm_dirty_pages_log_change(kvm_context_t kvm,
> > mem.guest_phys_addr,
> > mem.memory_size,
> > mem.flags);
> > - r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &mem);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_USER_MEMORY_REGION, &mem);
> > if (r == -1)
> > fprintf(stderr, "%s: %m\n", __FUNCTION__);
> > }
> > @@ -452,7 +452,7 @@ int kvm_init(int smp_cpus)
> > kvm_context = &kvm_state->kvm_context;
> >
> > kvm_context->fd = fd;
> > - kvm_context->vm_fd = -1;
> > + kvm_state->vmfd = -1;
> > kvm_context->opaque = cpu_single_env;
> > kvm_context->dirty_pages_log_all = 0;
> > kvm_context->no_irqchip_creation = 0;
> > @@ -516,7 +516,7 @@ kvm_vcpu_context_t kvm_create_vcpu(CPUState *env, int id)
> > vcpu_ctx->kvm = kvm;
> > vcpu_ctx->id = id;
> >
> > - r = ioctl(kvm->vm_fd, KVM_CREATE_VCPU, id);
> > + r = kvm_vm_ioctl(kvm_state, KVM_CREATE_VCPU, id);
> > if (r == -1) {
> > fprintf(stderr, "kvm_create_vcpu: %m\n");
> > goto err;
> > @@ -550,7 +550,7 @@ static int kvm_set_boot_vcpu_id(kvm_context_t kvm, uint32_t id)
> > #ifdef KVM_CAP_SET_BOOT_CPU_ID
> > int r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_SET_BOOT_CPU_ID);
> > if (r > 0)
> > - return ioctl(kvm->vm_fd, KVM_SET_BOOT_CPU_ID, id);
> > + return kvm_vm_ioctl(kvm_state, KVM_SET_BOOT_CPU_ID, id);
> > return -ENOSYS;
> > #else
> > return -ENOSYS;
> > @@ -571,7 +571,7 @@ int kvm_create_vm(kvm_context_t kvm)
> > fprintf(stderr, "kvm_create_vm: %m\n");
> > return -1;
> > }
> > - kvm->vm_fd = fd;
> > + kvm_state->vmfd = fd;
> > return 0;
> > }
> >
> > @@ -594,7 +594,7 @@ int kvm_check_extension(kvm_context_t kvm, int ext)
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->fd, KVM_CHECK_EXTENSION, ext);
> > + ret = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, ext);
> > if (ret > 0)
> > return ret;
> > return 0;
> > @@ -609,7 +609,7 @@ void kvm_create_irqchip(kvm_context_t kvm)
> > if (!kvm->no_irqchip_creation) {
> > r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_IRQCHIP);
> > if (r > 0) { /* kernel irqchip supported */
> > - r = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
> > + r = kvm_vm_ioctl(kvm_state, KVM_CREATE_IRQCHIP);
> > if (r >= 0) {
> > kvm->irqchip_inject_ioctl = KVM_IRQ_LINE;
> > #if defined(KVM_CAP_IRQ_INJECT_STATUS) && defined(KVM_IRQ_LINE_STATUS)
> > @@ -664,7 +664,7 @@ int kvm_register_phys_mem(kvm_context_t kvm,
> > DPRINTF("memory: gpa: %llx, size: %llx, uaddr: %llx, slot: %x, flags: %lx\n",
> > memory.guest_phys_addr, memory.memory_size,
> > memory.userspace_addr, memory.slot, memory.flags);
> > - r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &memory);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_USER_MEMORY_REGION, &memory);
> > if (r == -1) {
> > fprintf(stderr, "create_userspace_phys_mem: %s\n", strerror(errno));
> > return -1;
> > @@ -710,7 +710,7 @@ void kvm_destroy_phys_mem(kvm_context_t kvm, unsigned long phys_start,
> > memory.guest_phys_addr,
> > memory.memory_size,
> > memory.flags);
> > - r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &memory);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_USER_MEMORY_REGION, &memory);
> > if (r == -1) {
> > fprintf(stderr, "destroy_userspace_phys_mem: %s",
> > strerror(errno));
> > @@ -741,7 +741,7 @@ static int kvm_get_map(kvm_context_t kvm, int ioctl_num, int slot, void *buf)
> >
> > log.dirty_bitmap = buf;
> >
> > - r = ioctl(kvm->vm_fd, ioctl_num, &log);
> > + r = kvm_vm_ioctl(kvm_state, ioctl_num, &log);
> > if (r == -1)
> > return -errno;
> > return 0;
> > @@ -794,7 +794,7 @@ int kvm_set_irq_level(kvm_context_t kvm, int irq, int level, int *status)
> > return 0;
> > event.level = level;
> > event.irq = irq;
> > - r = ioctl(kvm->vm_fd, kvm->irqchip_inject_ioctl, &event);
> > + r = kvm_vm_ioctl(kvm_state, kvm->irqchip_inject_ioctl, &event);
> > if (r == -1)
> > perror("kvm_set_irq_level");
> >
> > @@ -816,7 +816,7 @@ int kvm_get_irqchip(kvm_context_t kvm, struct kvm_irqchip *chip)
> >
> > if (!kvm->irqchip_in_kernel)
> > return 0;
> > - r = ioctl(kvm->vm_fd, KVM_GET_IRQCHIP, chip);
> > + r = kvm_vm_ioctl(kvm_state, KVM_GET_IRQCHIP, chip);
> > if (r == -1) {
> > r = -errno;
> > perror("kvm_get_irqchip\n");
> > @@ -830,7 +830,7 @@ int kvm_set_irqchip(kvm_context_t kvm, struct kvm_irqchip *chip)
> >
> > if (!kvm->irqchip_in_kernel)
> > return 0;
> > - r = ioctl(kvm->vm_fd, KVM_SET_IRQCHIP, chip);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_IRQCHIP, chip);
> > if (r == -1) {
> > r = -errno;
> > perror("kvm_set_irqchip\n");
> > @@ -1230,7 +1230,7 @@ int kvm_coalesce_mmio_region(target_phys_addr_t addr, ram_addr_t size)
> > zone.addr = addr;
> > zone.size = size;
> >
> > - r = ioctl(kvm->vm_fd, KVM_REGISTER_COALESCED_MMIO, &zone);
> > + r = kvm_vm_ioctl(kvm_state, KVM_REGISTER_COALESCED_MMIO, &zone);
> > if (r == -1) {
> > perror("kvm_register_coalesced_mmio_zone");
> > return -errno;
> > @@ -1253,7 +1253,7 @@ int kvm_uncoalesce_mmio_region(target_phys_addr_t addr, ram_addr_t size)
> > zone.addr = addr;
> > zone.size = size;
> >
> > - r = ioctl(kvm->vm_fd, KVM_UNREGISTER_COALESCED_MMIO, &zone);
> > + r = kvm_vm_ioctl(kvm_state, KVM_UNREGISTER_COALESCED_MMIO, &zone);
> > if (r == -1) {
> > perror("kvm_unregister_coalesced_mmio_zone");
> > return -errno;
> > @@ -1271,7 +1271,7 @@ int kvm_assign_pci_device(kvm_context_t kvm,
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->vm_fd, KVM_ASSIGN_PCI_DEVICE, assigned_dev);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_ASSIGN_PCI_DEVICE, assigned_dev);
> > if (ret < 0)
> > return -errno;
> >
> > @@ -1283,7 +1283,7 @@ static int kvm_old_assign_irq(kvm_context_t kvm,
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->vm_fd, KVM_ASSIGN_IRQ, assigned_irq);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_ASSIGN_IRQ, assigned_irq);
> > if (ret < 0)
> > return -errno;
> >
> > @@ -1298,7 +1298,7 @@ int kvm_assign_irq(kvm_context_t kvm,
> >
> > ret = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_ASSIGN_DEV_IRQ);
> > if (ret > 0) {
> > - ret = ioctl(kvm->vm_fd, KVM_ASSIGN_DEV_IRQ, assigned_irq);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_ASSIGN_DEV_IRQ, assigned_irq);
> > if (ret < 0)
> > return -errno;
> > return ret;
> > @@ -1312,7 +1312,7 @@ int kvm_deassign_irq(kvm_context_t kvm,
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->vm_fd, KVM_DEASSIGN_DEV_IRQ, assigned_irq);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_DEASSIGN_DEV_IRQ, assigned_irq);
> > if (ret < 0)
> > return -errno;
> >
> > @@ -1333,7 +1333,7 @@ int kvm_deassign_pci_device(kvm_context_t kvm,
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->vm_fd, KVM_DEASSIGN_PCI_DEVICE, assigned_dev);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_DEASSIGN_PCI_DEVICE, assigned_dev);
> > if (ret < 0)
> > return -errno;
> >
> > @@ -1364,7 +1364,7 @@ int kvm_reinject_control(kvm_context_t kvm, int pit_reinject)
> >
> > r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_REINJECT_CONTROL);
> > if (r > 0) {
> > - r = ioctl(kvm->vm_fd, KVM_REINJECT_CONTROL, &control);
> > + r = kvm_vm_ioctl(kvm_state, KVM_REINJECT_CONTROL, &control);
> > if (r == -1)
> > return -errno;
> > return r;
> > @@ -1584,7 +1584,7 @@ int kvm_commit_irq_routes(kvm_context_t kvm)
> > int r;
> >
> > kvm->irq_routes->flags = 0;
> > - r = ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, kvm->irq_routes);
> > + r = kvm_vm_ioctl(kvm_state, KVM_SET_GSI_ROUTING, kvm->irq_routes);
> > if (r == -1)
> > r = -errno;
> > return r;
> > @@ -1616,7 +1616,7 @@ int kvm_assign_set_msix_nr(kvm_context_t kvm,
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->vm_fd, KVM_ASSIGN_SET_MSIX_NR, msix_nr);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_ASSIGN_SET_MSIX_NR, msix_nr);
> > if (ret < 0)
> > return -errno;
> >
> > @@ -1628,7 +1628,7 @@ int kvm_assign_set_msix_entry(kvm_context_t kvm,
> > {
> > int ret;
> >
> > - ret = ioctl(kvm->vm_fd, KVM_ASSIGN_SET_MSIX_ENTRY, entry);
> > + ret = kvm_vm_ioctl(kvm_state, KVM_ASSIGN_SET_MSIX_ENTRY, entry);
> > if (ret < 0)
> > return -errno;
> >
> > @@ -1649,7 +1649,7 @@ static int _kvm_irqfd(kvm_context_t kvm, int fd, int gsi, int flags)
> > .flags = flags,
> > };
> >
> > - r = ioctl(kvm->vm_fd, KVM_IRQFD, &data);
> > + r = kvm_vm_ioctl(kvm_state, KVM_IRQFD, &data);
> > if (r == -1)
> > r = -errno;
> > return r;
> > diff --git a/qemu-kvm.h b/qemu-kvm.h
> > index d5291a3..f27da7c 100644
> > --- a/qemu-kvm.h
> > +++ b/qemu-kvm.h
> > @@ -54,9 +54,6 @@ extern int kvm_abi;
> > struct kvm_context {
> > /// Filedescriptor to /dev/kvm
> > int fd;
> > - int vm_fd;
> > - /// Callbacks that KVM uses to emulate various unvirtualizable functionality
> > - struct kvm_callbacks *callbacks;
>
> So you move some information (vm_fd, kvm_callbacks) from kvm_context to
> KVMState.
>
> Why not wait until KVMState is capable of fully replacing kvm_context,
> and then do a full replacement at once?
>
> I fail to understand the point of the partial convertion, and it seems
> to make the qemu-kvm.c code more confusing?
>
First: incremental steps.
Second: It is not placing in two structures. kvm_context is now _inside_
KVMState. So it is really all inside KVMState, just part of it, in a substructure.
Note that, we have our own copy of KVMState, which is slightly different from upstream.
(it includes kvm_context!). A reasonable next step, is to fold our data of kvm_context
directly inside KVMState.
I wanted, however, to re-use the fields KVMState already have, otherwise the patch
could get quite big.
Also, you have already merged a patch that does exactly this: reuses the breakpoints
structure from KVMState.
next prev parent reply other threads:[~2009-07-17 14:15 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-07-15 16:31 [PATCH 0/5] Further cleanups Glauber Costa
2009-07-15 16:31 ` [PATCH 1/5] remove kvm types from handle unhandled Glauber Costa
2009-07-15 16:31 ` [PATCH 2/5] reuse kvm_vm_ioctl Glauber Costa
2009-07-15 16:31 ` [PATCH 3/5] reuse kvm_ioctl Glauber Costa
2009-07-15 16:31 ` [PATCH 4/5] check extension Glauber Costa
2009-07-15 16:31 ` [PATCH 5/5] use upstream cpuid code Glauber Costa
2009-07-17 13:35 ` [PATCH 2/5] reuse kvm_vm_ioctl Marcelo Tosatti
2009-07-17 14:21 ` Glauber Costa [this message]
2009-07-17 15:39 ` Marcelo Tosatti
2009-07-17 15:49 ` Glauber Costa
2009-07-17 15:46 ` Marcelo Tosatti
2009-07-17 15:57 ` Marcelo Tosatti
2009-07-17 16:48 ` Glauber Costa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090717142149.GD4019@poweredge.glommer \
--to=glommer@redhat.com \
--cc=avi@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=mtosatti@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.