Re: RFC/patch portability: split kvm_vm_ioctl v3

Hollis Blanchard
2007-10-26T18:32:03Z

Acked-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

On Fri, 2007-10-26 at 14:01 +0200, Carsten Otte wrote:
> This patch splits kvm_vm_ioctl into archtecture independent parts, and
> x86 specific parts which go to kvm_arch_vcpu_ioctl in x86.c.
> The patch has been updated to current git, and it leaves out memory slot
> registration work which is currently subject to a detailed discussion.
>
> Common ioctls for all architectures are:
> KVM_CREATE_VCPU, KVM_GET_DIRTY_LOG, KVM_SET_USER_MEMORY_REGION
>
> KVM_SET_USER_MEMORY_REGION implementation is no longer moved to x86.c.
> It seems to me that more fine-grained refinement then just moving the
> code is required here.
>
> x86 specific ioctls are:
> KVM_SET_MEMORY_REGION,
> KVM_GET/SET_NR_MMU_PAGES, KVM_SET_MEMORY_ALIAS, KVM_CREATE_IRQCHIP,
> KVM_CREATE_IRQ_LINE, KVM_GET/SET_IRQCHIP
> KVM_SET_TSS_ADDR
>
> KVM_SET_TSS_ADDR has been added to the list of x86 specifics, as Izik's
> commit states it is used for emulating real mode on intel.
>
>
> kvm.h | 7 +
> kvm_main.c | 255
> +-----------------------------------------------------------
> x86.c | 258
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 271 insertions(+), 249 deletions(-)
> Signed-off-by: Carsten Otte <cotte-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
> ---
> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index b08fc9e..438d4a9 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -621,6 +621,13 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg);
> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu);
> +int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
> + struct
> + kvm_userspace_memory_region *mem,
> + int user_alloc);
> +long kvm_arch_vm_ioctl(struct file *filp,
> + unsigned int ioctl, unsigned long arg);
> +void kvm_arch_destroy_vm(struct kvm *kvm);
>
> __init void kvm_arch_init(void);
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 9a3d663..7a85be9 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -792,36 +792,16 @@ out:
> }
> EXPORT_SYMBOL_GPL(kvm_set_memory_region);
>
> -static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
> - struct
> - kvm_userspace_memory_region *mem,
> - int user_alloc)
> +int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
> + struct
> + kvm_userspace_memory_region *mem,
> + int user_alloc)
> {
> if (mem->slot >= KVM_MEMORY_SLOTS)
> return -EINVAL;
> return kvm_set_memory_region(kvm, mem, user_alloc);
> }
>
> -static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
> - u32 kvm_nr_mmu_pages)
> -{
> - if (kvm_nr_mmu_pages < KVM_MIN_ALLOC_MMU_PAGES)
> - return -EINVAL;
> -
> - mutex_lock(&kvm->lock);
> -
> - kvm_mmu_change_mmu_pages(kvm, kvm_nr_mmu_pages);
> - kvm->n_requested_mmu_pages = kvm_nr_mmu_pages;
> -
> - mutex_unlock(&kvm->lock);
> - return 0;
> -}
> -
> -static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> -{
> - return kvm->n_alloc_mmu_pages;
> -}
> -
> /*
> * Get (and clear) the dirty memory log for a memory slot.
> */
> @@ -867,111 +847,6 @@ out:
> return r;
> }
>
> -/*
> - * Set a new alias region. Aliases map a portion of physical memory into
> - * another portion. This is useful for memory windows, for example the PC
> - * VGA region.
> - */
> -static int kvm_vm_ioctl_set_memory_alias(struct kvm *kvm,
> - struct kvm_memory_alias *alias)
> -{
> - int r, n;
> - struct kvm_mem_alias *p;
> -
> - r = -EINVAL;
> - /* General sanity checks */
> - if (alias->memory_size & (PAGE_SIZE - 1))
> - goto out;
> - if (alias->guest_phys_addr & (PAGE_SIZE - 1))
> - goto out;
> - if (alias->slot >= KVM_ALIAS_SLOTS)
> - goto out;
> - if (alias->guest_phys_addr + alias->memory_size
> - < alias->guest_phys_addr)
> - goto out;
> - if (alias->target_phys_addr + alias->memory_size
> - < alias->target_phys_addr)
> - goto out;
> -
> - mutex_lock(&kvm->lock);
> -
> - p = &kvm->aliases[alias->slot];
> - p->base_gfn = alias->guest_phys_addr >> PAGE_SHIFT;
> - p->npages = alias->memory_size >> PAGE_SHIFT;
> - p->target_gfn = alias->target_phys_addr >> PAGE_SHIFT;
> -
> - for (n = KVM_ALIAS_SLOTS; n > 0; --n)
> - if (kvm->aliases[n - 1].npages)
> - break;
> - kvm->naliases = n;
> -
> - kvm_mmu_zap_all(kvm);
> -
> - mutex_unlock(&kvm->lock);
> -
> - return 0;
> -
> -out:
> - return r;
> -}
> -
> -static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
> -{
> - int r;
> -
> - r = 0;
> - switch (chip->chip_id) {
> - case KVM_IRQCHIP_PIC_MASTER:
> - memcpy(&chip->chip.pic,
> - &pic_irqchip(kvm)->pics[0],
> - sizeof(struct kvm_pic_state));
> - break;
> - case KVM_IRQCHIP_PIC_SLAVE:
> - memcpy(&chip->chip.pic,
> - &pic_irqchip(kvm)->pics[1],
> - sizeof(struct kvm_pic_state));
> - break;
> - case KVM_IRQCHIP_IOAPIC:
> - memcpy(&chip->chip.ioapic,
> - ioapic_irqchip(kvm),
> - sizeof(struct kvm_ioapic_state));
> - break;
> - default:
> - r = -EINVAL;
> - break;
> - }
> - return r;
> -}
> -
> -static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
> -{
> - int r;
> -
> - r = 0;
> - switch (chip->chip_id) {
> - case KVM_IRQCHIP_PIC_MASTER:
> - memcpy(&pic_irqchip(kvm)->pics[0],
> - &chip->chip.pic,
> - sizeof(struct kvm_pic_state));
> - break;
> - case KVM_IRQCHIP_PIC_SLAVE:
> - memcpy(&pic_irqchip(kvm)->pics[1],
> - &chip->chip.pic,
> - sizeof(struct kvm_pic_state));
> - break;
> - case KVM_IRQCHIP_IOAPIC:
> - memcpy(ioapic_irqchip(kvm),
> - &chip->chip.ioapic,
> - sizeof(struct kvm_ioapic_state));
> - break;
> - default:
> - r = -EINVAL;
> - break;
> - }
> - kvm_pic_update_irq(pic_irqchip(kvm));
> - return r;
> -}
> -
> int is_error_page(struct page *page)
> {
> return page == bad_page;
> @@ -2669,16 +2544,6 @@ static int create_vcpu_fd(struct kvm_vcpu *vcpu)
> return fd;
> }
>
> -static int kvm_vm_ioctl_set_tss_addr(struct kvm *kvm, unsigned long addr)
> -{
> - int ret;
> -
> - if (addr > (unsigned int)(-3 * PAGE_SIZE))
> - return -1;
> - ret = kvm_x86_ops->set_tss_addr(kvm, addr);
> - return ret;
> -}
> -
> /*
> * Creates some virtual cpus. Good luck creating more than one.
> */
> @@ -2972,35 +2837,14 @@ static long kvm_vm_ioctl(struct file *filp,
> {
> struct kvm *kvm = filp->private_data;
> void __user *argp = (void __user *)arg;
> - int r = -EINVAL;
> + int r;
>
> switch (ioctl) {
> - case KVM_SET_TSS_ADDR:
> - r = kvm_vm_ioctl_set_tss_addr(kvm, arg);
> - if (r < 0)
> - goto out;
> - break;
> case KVM_CREATE_VCPU:
> r = kvm_vm_ioctl_create_vcpu(kvm, arg);
> if (r < 0)
> goto out;
> break;
> - case KVM_SET_MEMORY_REGION: {
> - struct kvm_memory_region kvm_mem;
> - struct kvm_userspace_memory_region kvm_userspace_mem;
> -
> - r = -EFAULT;
> - if (copy_from_user(&kvm_mem, argp, sizeof kvm_mem))
> - goto out;
> - kvm_userspace_mem.slot = kvm_mem.slot;
> - kvm_userspace_mem.flags = kvm_mem.flags;
> - kvm_userspace_mem.guest_phys_addr = kvm_mem.guest_phys_addr;
> - kvm_userspace_mem.memory_size = kvm_mem.memory_size;
> - r = kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem, 0);
> - if (r)
> - goto out;
> - break;
> - }
> case KVM_SET_USER_MEMORY_REGION: {
> struct kvm_userspace_memory_region kvm_userspace_mem;
>
> @@ -3014,14 +2858,6 @@ static long kvm_vm_ioctl(struct file *filp,
> goto out;
> break;
> }
> - case KVM_SET_NR_MMU_PAGES:
> - r = kvm_vm_ioctl_set_nr_mmu_pages(kvm, arg);
> - if (r)
> - goto out;
> - break;
> - case KVM_GET_NR_MMU_PAGES:
> - r = kvm_vm_ioctl_get_nr_mmu_pages(kvm);
> - break;
> case KVM_GET_DIRTY_LOG: {
> struct kvm_dirty_log log;
>
> @@ -3033,87 +2869,8 @@ static long kvm_vm_ioctl(struct file *filp,
> goto out;
> break;
> }
> - case KVM_SET_MEMORY_ALIAS: {
> - struct kvm_memory_alias alias;
> -
> - r = -EFAULT;
> - if (copy_from_user(&alias, argp, sizeof alias))
> - goto out;
> - r = kvm_vm_ioctl_set_memory_alias(kvm, &alias);
> - if (r)
> - goto out;
> - break;
> - }
> - case KVM_CREATE_IRQCHIP:
> - r = -ENOMEM;
> - kvm->vpic = kvm_create_pic(kvm);
> - if (kvm->vpic) {
> - r = kvm_ioapic_init(kvm);
> - if (r) {
> - kfree(kvm->vpic);
> - kvm->vpic = NULL;
> - goto out;
> - }
> - } else
> - goto out;
> - break;
> - case KVM_IRQ_LINE: {
> - struct kvm_irq_level irq_event;
> -
> - r = -EFAULT;
> - if (copy_from_user(&irq_event, argp, sizeof irq_event))
> - goto out;
> - if (irqchip_in_kernel(kvm)) {
> - mutex_lock(&kvm->lock);
> - if (irq_event.irq < 16)
> - kvm_pic_set_irq(pic_irqchip(kvm),
> - irq_event.irq,
> - irq_event.level);
> - kvm_ioapic_set_irq(kvm->vioapic,
> - irq_event.irq,
> - irq_event.level);
> - mutex_unlock(&kvm->lock);
> - r = 0;
> - }
> - break;
> - }
> - case KVM_GET_IRQCHIP: {
> - /* 0: PIC master, 1: PIC slave, 2: IOAPIC */
> - struct kvm_irqchip chip;
> -
> - r = -EFAULT;
> - if (copy_from_user(&chip, argp, sizeof chip))
> - goto out;
> - r = -ENXIO;
> - if (!irqchip_in_kernel(kvm))
> - goto out;
> - r = kvm_vm_ioctl_get_irqchip(kvm, &chip);
> - if (r)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(argp, &chip, sizeof chip))
> - goto out;
> - r = 0;
> - break;
> - }
> - case KVM_SET_IRQCHIP: {
> - /* 0: PIC master, 1: PIC slave, 2: IOAPIC */
> - struct kvm_irqchip chip;
> -
> - r = -EFAULT;
> - if (copy_from_user(&chip, argp, sizeof chip))
> - goto out;
> - r = -ENXIO;
> - if (!irqchip_in_kernel(kvm))
> - goto out;
> - r = kvm_vm_ioctl_set_irqchip(kvm, &chip);
> - if (r)
> - goto out;
> - r = 0;
> - break;
> - }
> default:
> - ;
> + r = kvm_arch_vm_ioctl(filp, ioctl, arg);
> }
> out:
> return r;
> diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
> index 1fe209d..b84cb67 100644
> --- a/drivers/kvm/x86.c
> +++ b/drivers/kvm/x86.c
> @@ -300,6 +300,264 @@ out:
> return r;
> }
>
> +static int kvm_vm_ioctl_set_tss_addr(struct kvm *kvm, unsigned long addr)
> +{
> + int ret;
> +
> + if (addr > (unsigned int)(-3 * PAGE_SIZE))
> + return -1;
> + ret = kvm_x86_ops->set_tss_addr(kvm, addr);
> + return ret;
> +}
> +
> +static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
> + u32 kvm_nr_mmu_pages)
> +{
> + if (kvm_nr_mmu_pages < KVM_MIN_ALLOC_MMU_PAGES)
> + return -EINVAL;
> +
> + mutex_lock(&kvm->lock);
> +
> + kvm_mmu_change_mmu_pages(kvm, kvm_nr_mmu_pages);
> + kvm->n_requested_mmu_pages = kvm_nr_mmu_pages;
> +
> + mutex_unlock(&kvm->lock);
> + return 0;
> +}
> +
> +static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> +{
> + return kvm->n_alloc_mmu_pages;
> +}
> +
> +/*
> + * Set a new alias region. Aliases map a portion of physical memory into
> + * another portion. This is useful for memory windows, for example the PC
> + * VGA region.
> + */
> +static int kvm_vm_ioctl_set_memory_alias(struct kvm *kvm,
> + struct kvm_memory_alias *alias)
> +{
> + int r, n;
> + struct kvm_mem_alias *p;
> +
> + r = -EINVAL;
> + /* General sanity checks */
> + if (alias->memory_size & (PAGE_SIZE - 1))
> + goto out;
> + if (alias->guest_phys_addr & (PAGE_SIZE - 1))
> + goto out;
> + if (alias->slot >= KVM_ALIAS_SLOTS)
> + goto out;
> + if (alias->guest_phys_addr + alias->memory_size
> + < alias->guest_phys_addr)
> + goto out;
> + if (alias->target_phys_addr + alias->memory_size
> + < alias->target_phys_addr)
> + goto out;
> +
> + mutex_lock(&kvm->lock);
> +
> + p = &kvm->aliases[alias->slot];
> + p->base_gfn = alias->guest_phys_addr >> PAGE_SHIFT;
> + p->npages = alias->memory_size >> PAGE_SHIFT;
> + p->target_gfn = alias->target_phys_addr >> PAGE_SHIFT;
> +
> + for (n = KVM_ALIAS_SLOTS; n > 0; --n)
> + if (kvm->aliases[n - 1].npages)
> + break;
> + kvm->naliases = n;
> +
> + kvm_mmu_zap_all(kvm);
> +
> + mutex_unlock(&kvm->lock);
> +
> + return 0;
> +
> +out:
> + return r;
> +}
> +
> +static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
> +{
> + int r;
> +
> + r = 0;
> + switch (chip->chip_id) {
> + case KVM_IRQCHIP_PIC_MASTER:
> + memcpy(&chip->chip.pic,
> + &pic_irqchip(kvm)->pics[0],
> + sizeof(struct kvm_pic_state));
> + break;
> + case KVM_IRQCHIP_PIC_SLAVE:
> + memcpy(&chip->chip.pic,
> + &pic_irqchip(kvm)->pics[1],
> + sizeof(struct kvm_pic_state));
> + break;
> + case KVM_IRQCHIP_IOAPIC:
> + memcpy(&chip->chip.ioapic,
> + ioapic_irqchip(kvm),
> + sizeof(struct kvm_ioapic_state));
> + break;
> + default:
> + r = -EINVAL;
> + break;
> + }
> + return r;
> +}
> +
> +static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
> +{
> + int r;
> +
> + r = 0;
> + switch (chip->chip_id) {
> + case KVM_IRQCHIP_PIC_MASTER:
> + memcpy(&pic_irqchip(kvm)->pics[0],
> + &chip->chip.pic,
> + sizeof(struct kvm_pic_state));
> + break;
> + case KVM_IRQCHIP_PIC_SLAVE:
> + memcpy(&pic_irqchip(kvm)->pics[1],
> + &chip->chip.pic,
> + sizeof(struct kvm_pic_state));
> + break;
> + case KVM_IRQCHIP_IOAPIC:
> + memcpy(ioapic_irqchip(kvm),
> + &chip->chip.ioapic,
> + sizeof(struct kvm_ioapic_state));
> + break;
> + default:
> + r = -EINVAL;
> + break;
> + }
> + kvm_pic_update_irq(pic_irqchip(kvm));
> + return r;
> +}
> +
> +long kvm_arch_vm_ioctl(struct file *filp,
> + unsigned int ioctl, unsigned long arg)
> +{
> + struct kvm *kvm = filp->private_data;
> + void __user *argp = (void __user *)arg;
> + int r = -EINVAL;
> +
> + switch (ioctl) {
> + case KVM_SET_TSS_ADDR:
> + r = kvm_vm_ioctl_set_tss_addr(kvm, arg);
> + if (r < 0)
> + goto out;
> + break;
> + case KVM_SET_MEMORY_REGION: {
> + struct kvm_memory_region kvm_mem;
> + struct kvm_userspace_memory_region kvm_userspace_mem;
> +
> + r = -EFAULT;
> + if (copy_from_user(&kvm_mem, argp, sizeof kvm_mem))
> + goto out;
> + kvm_userspace_mem.slot = kvm_mem.slot;
> + kvm_userspace_mem.flags = kvm_mem.flags;
> + kvm_userspace_mem.guest_phys_addr = kvm_mem.guest_phys_addr;
> + kvm_userspace_mem.memory_size = kvm_mem.memory_size;
> + r = kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem, 0);
> + if (r)
> + goto out;
> + break;
> + }
> + case KVM_SET_NR_MMU_PAGES:
> + r = kvm_vm_ioctl_set_nr_mmu_pages(kvm, arg);
> + if (r)
> + goto out;
> + break;
> + case KVM_GET_NR_MMU_PAGES:
> + r = kvm_vm_ioctl_get_nr_mmu_pages(kvm);
> + break;
> + case KVM_SET_MEMORY_ALIAS: {
> + struct kvm_memory_alias alias;
> +
> + r = -EFAULT;
> + if (copy_from_user(&alias, argp, sizeof alias))
> + goto out;
> + r = kvm_vm_ioctl_set_memory_alias(kvm, &alias);
> + if (r)
> + goto out;
> + break;
> + }
> + case KVM_CREATE_IRQCHIP:
> + r = -ENOMEM;
> + kvm->vpic = kvm_create_pic(kvm);
> + if (kvm->vpic) {
> + r = kvm_ioapic_init(kvm);
> + if (r) {
> + kfree(kvm->vpic);
> + kvm->vpic = NULL;
> + goto out;
> + }
> + } else
> + goto out;
> + break;
> + case KVM_IRQ_LINE: {
> + struct kvm_irq_level irq_event;
> +
> + r = -EFAULT;
> + if (copy_from_user(&irq_event, argp, sizeof irq_event))
> + goto out;
> + if (irqchip_in_kernel(kvm)) {
> + mutex_lock(&kvm->lock);
> + if (irq_event.irq < 16)
> + kvm_pic_set_irq(pic_irqchip(kvm),
> + irq_event.irq,
> + irq_event.level);
> + kvm_ioapic_set_irq(kvm->vioapic,
> + irq_event.irq,
> + irq_event.level);
> + mutex_unlock(&kvm->lock);
> + r = 0;
> + }
> + break;
> + }
> + case KVM_GET_IRQCHIP: {
> + /* 0: PIC master, 1: PIC slave, 2: IOAPIC */
> + struct kvm_irqchip chip;
> +
> + r = -EFAULT;
> + if (copy_from_user(&chip, argp, sizeof chip))
> + goto out;
> + r = -ENXIO;
> + if (!irqchip_in_kernel(kvm))
> + goto out;
> + r = kvm_vm_ioctl_get_irqchip(kvm, &chip);
> + if (r)
> + goto out;
> + r = -EFAULT;
> + if (copy_to_user(argp, &chip, sizeof chip))
> + goto out;
> + r = 0;
> + break;
> + }
> + case KVM_SET_IRQCHIP: {
> + /* 0: PIC master, 1: PIC slave, 2: IOAPIC */
> + struct kvm_irqchip chip;
> +
> + r = -EFAULT;
> + if (copy_from_user(&chip, argp, sizeof chip))
> + goto out;
> + r = -ENXIO;
> + if (!irqchip_in_kernel(kvm))
> + goto out;
> + r = kvm_vm_ioctl_set_irqchip(kvm, &chip);
> + if (r)
> + goto out;
> + r = 0;
> + break;
> + }
> + default:
> + ;
> + }
> +out:
> + return r;
> +}
> +
> static __init void kvm_init_msr_list(void)
> {
> u32 dummy[2];
>
>
--
Hollis Blanchard
IBM Linux Technology Center