On 04/02/2013 08:02:39 PM, Paul Mackerras wrote:
On Mon, Apr 01, 2013 at 05:47:48PM -0500, Scott Wood wrote:
> Currently, devices that are emulated inside KVM are configured in a
> hardcoded manner based on an assumption that any given architecture
> only has one way to do it. If there's any need to access device
state,
> it is done through inflexible one-purpose-only IOCTLs (e.g.
> KVM_GET/SET_LAPIC). Defining new IOCTLs for every little thing is
> cumbersome and depletes a limited numberspace.
>
> This API provides a mechanism to instantiate a device of a certain
> type, returning an ID that can be used to set/get attributes of the
> device. Attributes may include configuration parameters (e.g.
> register base address), device state, operational commands, etc. It
> is similar to the ONE_REG API, except that it acts on devices rather
> than vcpus.
>
> Both device types and individual attributes can be tested without
having
> to create the device or get/set the attribute, without the need for
> separately managing enumerated capabilities.
>
> Signed-off-by: Scott Wood <scottwood@xxxxxxxxxxxxx>
Some comments below...
> diff --git a/Documentation/virtual/kvm/api.txt
b/Documentation/virtual/kvm/api.txt
> index 976eb65..77328aa 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2173,6 +2173,76 @@ header; first `n_valid' valid entries with
contents from the data
> written, then `n_invalid' invalid entries, invalidating any
previously
> valid entries found.
>
> +4.79 KVM_CREATE_DEVICE
> +
> +Capability: KVM_CAP_DEVICE_CTRL
I notice this patch doesn't add this capability;
Yes, it does (see below).
you add it in a later patch.
Maybe you're thinking of KVM_CAP_IRQ_MPIC?
> +Type: vm ioctl
> +Parameters: struct kvm_create_device (in/out)
> +Returns: 0 on success, -1 on error
> +Errors:
> + ENODEV: The device type is unknown or unsupported
> + EEXIST: Device already created, and this type of device may not
> + be instantiated multiple times
> + ENOSPC: Too many devices have been created
Is this still a possible error code?
If you mean ENOSPC, probably not -- it'd be replaced with whatever
errors can come out of creating a file descriptor.
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -370,6 +370,9 @@ struct kvmppc_booke_debug_reg {
> u64 dac[KVMPPC_BOOKE_MAX_DAC];
> };
>
> +#define KVMPPC_IRQCHIP_NONE 0
> +#define KVMPPC_IRQCHIP_MPIC 1
This define should go in the patch that adds the MPIC device.
> struct kvm_vcpu_arch {
> ulong host_stack;
> u32 host_pid;
> @@ -549,6 +552,9 @@ struct kvm_vcpu_arch {
> unsigned long magic_page_pa; /* phys addr to map the magic page
to */
> unsigned long magic_page_ea; /* effect. addr to map the magic
page to */
>
> + int irqchip_type;
> + void *irqchip_priv;
Since you add this (irqchip_priv) only to remove it in a later patch
and replace it by a device-specific pointer, why bother adding it
here? And why not give irqchip_type the name it ultimately ends up
with?
Oops... These were patch shuffling accidents and will be removed from
the next iteration.
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 16b4595..bdfa526 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -459,6 +459,13 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
> tasklet_kill(&vcpu->arch.tasklet);
>
> kvmppc_remove_vcpu_debugfs(vcpu);
> +
> + switch (vcpu->arch.irqchip_type) {
> + case KVMPPC_IRQCHIP_MPIC:
> + mpic_put(vcpu->arch.irqchip_priv);
> + break;
> + }
This is going to break bisection, since you don't define mpic_put() in
this patch.
Sigh. Something got messed up; I'll try to sort it out and resubmit.
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 74d0ff3..20ce2d2 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -668,6 +668,7 @@ struct kvm_ppc_smmu_info {
> #define KVM_CAP_PPC_EPR 86
> #define KVM_CAP_ARM_PSCI 87
> #define KVM_CAP_ARM_SET_DEVICE_ADDR 88
> +#define KVM_CAP_DEVICE_CTRL 89
See, here's the capability. :-)
> /*
> + * Device control API, available with KVM_CAP_DEVICE_CTRL
> + */
> +#define KVM_CREATE_DEVICE_TEST 1
> +
> +struct kvm_create_device {
> + __u32 type; /* in: KVM_DEV_TYPE_xxx */
> + __u32 fd; /* out: device handle */
> + __u32 flags; /* in: KVM_CREATE_DEVICE_xxx */
> +};
> +
> +struct kvm_device_attr {
> + __u32 flags; /* no flags currently defined */
> + __u32 group; /* device-defined */
> + __u64 attr; /* group-defined */
> + __u64 addr; /* userspace address of attr data */
> +};
> +
> +/* ioctl for vm fd */
> +#define KVM_CREATE_DEVICE _IOWR(KVMIO, 0xe0, struct
kvm_create_device)
This define should go with the other VM ioctls, otherwise the next
person to add a VM ioctl will probably miss it and reuse the 0xe0
code.
That's actually why I moved it to a new section, with device control
ioctls getting their own range, as the legacy "device model" and some
other things did. 0xe0 is not the next ioctl that would be used for
either vm or vcpu. The ioctl numbering is actually already a mess,
with sometimes care being taken to keep vcpu and vm ioctls from
overlapping, but on other places overlapping does happen. I'm not sure
what exactly I should do here.
-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
