From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 09/14] KVM: PPC: Add generic single register ioctls Date: Thu, 10 Nov 2011 14:05:36 -0200 Message-ID: <20111110160536.GD7554@amt.cnet> References: <1320047596-20577-1-git-send-email-agraf@suse.de> <1320047596-20577-10-git-send-email-agraf@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm-ppc@vger.kernel.org, kvm list To: Alexander Graf Return-path: Content-Disposition: inline In-Reply-To: <1320047596-20577-10-git-send-email-agraf@suse.de> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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 > --- > 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(®, argp, sizeof(reg))) > + goto out; > + r = kvm_vcpu_ioctl_get_one_reg(vcpu, ®); > + if (copy_to_user(argp, ®, sizeof(reg))) { > + r = -EFAULT; > + goto out; > + } > + break; > + } > + > + case KVM_SET_ONE_REG: > + { > + struct kvm_one_reg reg; > + r = -EFAULT; > + if (copy_from_user(®, argp, sizeof(reg))) > + goto out; > + r = kvm_vcpu_ioctl_set_one_reg(vcpu, ®); > + 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Date: Thu, 10 Nov 2011 16:05:36 +0000 Subject: Re: [PATCH 09/14] KVM: PPC: Add generic single register ioctls Message-Id: <20111110160536.GD7554@amt.cnet> List-Id: References: <1320047596-20577-1-git-send-email-agraf@suse.de> <1320047596-20577-10-git-send-email-agraf@suse.de> In-Reply-To: <1320047596-20577-10-git-send-email-agraf@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexander Graf Cc: kvm-ppc@vger.kernel.org, kvm list 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 > --- > 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(®, argp, sizeof(reg))) > + goto out; > + r = kvm_vcpu_ioctl_get_one_reg(vcpu, ®); > + if (copy_to_user(argp, ®, sizeof(reg))) { > + r = -EFAULT; > + goto out; > + } > + break; > + } > + > + case KVM_SET_ONE_REG: > + { > + struct kvm_one_reg reg; > + r = -EFAULT; > + if (copy_from_user(®, argp, sizeof(reg))) > + goto out; > + r = kvm_vcpu_ioctl_set_one_reg(vcpu, ®); > + 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