Re: [PATCH 09/14] KVM: PPC: Add generic single register ioctls

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Oct 31, 2011 at 08:53:11AM +0100, Alexander Graf wrote:
> Right now we transfer a static struct every time we want to get or set
> registers. Unfortunately, over time we realize that there are more of
> these than we thought of before and the extensibility and flexibility of
> transferring a full struct every time is limited.
> 
> So this is a new approach to the problem. With these new ioctls, we can
> get and set a single register that is identified by an ID. This allows for
> very precise and limited transmittal of data. When we later realize that
> it's a better idea to shove over multiple registers at once, we can reuse
> most of the infrastructure and simply implement a GET_MANY_REGS / SET_MANY_REGS
> interface.
> 
> The only downpoint I see to this one is that it needs to pad to 1024 bits
> (hardware is already on 512 bit registers, so I wanted to leave some room)
> which is slightly too much for transmitting only 64 bits. But if that's all
> the tradeoff we have to do for getting an extensible interface, I'd say go
> for it nevertheless.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  Documentation/virtual/kvm/api.txt |   47 ++++++++++++++++++++++++++++++++++
>  arch/powerpc/kvm/powerpc.c        |   51 +++++++++++++++++++++++++++++++++++++
>  include/linux/kvm.h               |   32 +++++++++++++++++++++++
>  3 files changed, 130 insertions(+), 0 deletions(-)

I don't see the benefit of this generalization, the current structure where 
context information is hardcoded in the data transmitted works well.

Avi?

> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index ab1136f..a23fe62 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1482,6 +1482,53 @@ is supported; 2 if the processor requires all virtual machines to have
>  an RMA, or 1 if the processor can use an RMA but doesn't require it,
>  because it supports the Virtual RMA (VRMA) facility.
>  
> +4.64 KVM_SET_ONE_REG
> +
> +Capability: KVM_CAP_ONE_REG
> +Architectures: all
> +Type: vcpu ioctl
> +Parameters: struct kvm_one_reg (in)
> +Returns: 0 on success, negative value on failure
> +
> +struct kvm_one_reg {
> +       __u64 id;
> +       union {
> +               __u8 reg8;
> +               __u16 reg16;
> +               __u32 reg32;
> +               __u64 reg64;
> +               __u8 reg128[16];
> +               __u8 reg256[32];
> +               __u8 reg512[64];
> +               __u8 reg1024[128];
> +       } u;
> +};
> +
> +Using this ioctl, a single vcpu register can be set to a specific value
> +defined by user space with the passed in struct kvm_one_reg. There can
> +be architecture agnostic and architecture specific registers. Each have
> +their own range of operation and their own constants and width. To keep
> +track of the implemented registers, find a list below:
> +
> +  Arch  |       Register        | Width (bits)
> +        |                       |
> +
> +4.65 KVM_GET_ONE_REG
> +
> +Capability: KVM_CAP_ONE_REG
> +Architectures: all
> +Type: vcpu ioctl
> +Parameters: struct kvm_one_reg (in and out)
> +Returns: 0 on success, negative value on failure
> +
> +This ioctl allows to receive the value of a single register implemented
> +in a vcpu. The register to read is indicated by the "id" field of the
> +kvm_one_reg struct passed in. On success, the register value can be found
> +in the respective width field of the struct after this call.
> +
> +The list of registers accessible using this interface is identical to the
> +list in 4.64.
> +
>  5. The kvm_run structure
>  
>  Application code obtains a pointer to the kvm_run structure by
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index e75c5ac..39cdb3f 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -214,6 +214,7 @@ int kvm_dev_ioctl_check_extension(long ext)
>  	case KVM_CAP_PPC_UNSET_IRQ:
>  	case KVM_CAP_PPC_IRQ_LEVEL:
>  	case KVM_CAP_ENABLE_CAP:
> +	case KVM_CAP_ONE_REG:
>  		r = 1;
>  		break;
>  #ifndef CONFIG_KVM_BOOK3S_64_HV
> @@ -627,6 +628,32 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  	return r;
>  }
>  
> +static int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
> +				      struct kvm_one_reg *reg)
> +{
> +	int r = -EINVAL;
> +
> +	switch (reg->id) {
> +	default:
> +		break;
> +	}
> +
> +	return r;
> +}
> +
> +static int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
> +				      struct kvm_one_reg *reg)
> +{
> +	int r = -EINVAL;
> +
> +	switch (reg->id) {
> +	default:
> +		break;
> +	}
> +
> +	return r;
> +}
> +
>  int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
>                                      struct kvm_mp_state *mp_state)
>  {
> @@ -666,6 +693,30 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		break;
>  	}
>  
> +	case KVM_GET_ONE_REG:
> +	{
> +		struct kvm_one_reg reg;
> +		r = -EFAULT;
> +		if (copy_from_user(&reg, argp, sizeof(reg)))
> +			goto out;
> +		r = kvm_vcpu_ioctl_get_one_reg(vcpu, &reg);
> +		if (copy_to_user(argp, &reg, sizeof(reg))) {
> +			r = -EFAULT;
> +			goto out;
> +		}
> +		break;
> +	}
> +
> +	case KVM_SET_ONE_REG:
> +	{
> +		struct kvm_one_reg reg;
> +		r = -EFAULT;
> +		if (copy_from_user(&reg, argp, sizeof(reg)))
> +			goto out;
> +		r = kvm_vcpu_ioctl_set_one_reg(vcpu, &reg);
> +		break;
> +	}
> +
>  #ifdef CONFIG_KVM_E500
>  	case KVM_DIRTY_TLB: {
>  		struct kvm_dirty_tlb dirty;
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index a6b1295..e652a7b 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -557,6 +557,7 @@ struct kvm_ppc_pvinfo {
>  #define KVM_CAP_MAX_VCPUS 66       /* returns max vcpus per vm */
>  #define KVM_CAP_PPC_PAPR 68
>  #define KVM_CAP_SW_TLB 69
> +#define KVM_CAP_ONE_REG 70
>  #define KVM_CAP_S390_GMAP 71
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
> @@ -652,6 +653,34 @@ struct kvm_dirty_tlb {
>  	__u32 num_dirty;
>  };
>  
> +/* Available with KVM_CAP_ONE_REG */
> +
> +#define KVM_ONE_REG_GENERIC		0x0000000000000000ULL
> +
> +/*
> + * Architecture specific registers are to be defined in arch headers and
> + * ORed with the arch identifier.
> + */
> +#define KVM_ONE_REG_PPC			0x1000000000000000ULL
> +#define KVM_ONE_REG_X86			0x2000000000000000ULL
> +#define KVM_ONE_REG_IA64		0x3000000000000000ULL
> +#define KVM_ONE_REG_ARM			0x4000000000000000ULL
> +#define KVM_ONE_REG_S390		0x5000000000000000ULL
> +
> +struct kvm_one_reg {
> +	__u64 id;
> +	union {
> +		__u8 reg8;
> +		__u16 reg16;
> +		__u32 reg32;
> +		__u64 reg64;
> +		__u8 reg128[16];
> +		__u8 reg256[32];
> +		__u8 reg512[64];
> +		__u8 reg1024[128];
> +	} u;
> +};
> +
>  /*
>   * ioctls for VM fds
>   */
> @@ -780,6 +809,9 @@ struct kvm_dirty_tlb {
>  #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
>  /* Available with KVM_CAP_SW_TLB */
>  #define KVM_DIRTY_TLB		  _IOW(KVMIO,  0xaa, struct kvm_dirty_tlb)
> +/* Available with KVM_CAP_ONE_REG */
> +#define KVM_GET_ONE_REG		  _IOWR(KVMIO, 0xab, struct kvm_one_reg)
> +#define KVM_SET_ONE_REG		  _IOW(KVMIO,  0xac, struct kvm_one_reg)
>  
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>  
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html