| Message ID | 1325639448-9494-8-git-send-email-agraf@suse.de |
|---|---|
| State | New, archived |
| Headers | show |
On 01/03/2012 07:10 PM, 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. If we end up replacing get/set sregs with this (either totally or just for new registers), we should consider a runtime facility for userspace to extract from the kernel a list of supported registers (with sizes) that are supported, so that userspace doesn't have to know about every register in order to migrate (this could also be useful if there is non-architecturally-readable state that needs to be preserved, such as a pending book3e doorbell exception, or the MCAR to be set when machine checks are unmasked). It could also avoid the need for explicit capabilities in some situations when new registers are added (like KVM_CAP_PPC_HIOR). > 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. Does it really make sense to consider these large things as single registers (even if that's how hw documents it)? Do they need to be set atomically? How do you get/set them in hardware if GPRs aren't that large? > +4.65 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; > +}; Do we need reg8/16/32, or can we simplify (and reduce potential size mismatch errors) by using __u64 for everything that doesn't need to be an array? > +/* Available with KVM_CAP_ONE_REG */ > + > +#define KVM_ONE_REG_GENERIC 0x0000000000000000ULL Generic registers? Is the idea to use this in place of dedicated ioctls for certain KVM knobs? > +/* > + * 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 Might want to allow space for more than 16 architectures -- there's room to spare. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04.01.2012, at 21:08, Scott Wood wrote: > On 01/03/2012 07:10 PM, 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. > > If we end up replacing get/set sregs with this (either totally or just > for new registers), we should consider a runtime facility for userspace > to extract from the kernel a list of supported registers (with sizes) > that are supported, so that userspace doesn't have to know about every > register in order to migrate (this could also be useful if there is > non-architecturally-readable state that needs to be preserved, such as a > pending book3e doorbell exception, or the MCAR to be set when machine > checks are unmasked). It could also avoid the need for explicit > capabilities in some situations when new registers are added (like > KVM_CAP_PPC_HIOR). Interesting idea. That doesn't quite match up with the way we treat migration today with all fields getting copied into the CPUState and then taken on the wire from there, but I can see how a more generic approach feels more elegant. Yes, the MANY_REGS interface should be really clever if we want to use it extensively, allowing us to only write a byte stream of exactly the register sizes we care about - no need to waste 128 bytes for every register - and with proper enumeration of available registers. But to be honest I'm unsure on what the best interface is really. We have 3 different possible way of implementing register sharing between user space and kernel space: 1) Current way with new CAP and struct enhancements or new structs every time we find something we didn't think about. I count ONE_REG in on this one. 2) Something fancy like GET_MANY_REGS that would allow us to have a very flexible interface between kernel and user space for getting and setting registers, but we would still need an ioctl for them, so the registers shouldn't be read/set in the fast path anyway, at which point maybe using get/set individually with ONE_REG is still ok? 3) Putting registers in the shared kvm_run struct so kernel and user space can access them; add a "dirty" bitmap in there so kernel space knows when a register is written from user space. This should be the preferred method for "fast path" registers - registers we need very often from user space. Since we do have 1) today and 3) should be there for the fast path, I'm not sure how much we really do need 2). But the nice thing here is that we can cross that bridge when we get to it. This patch merely lays the groundwork. >> 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. > > Does it really make sense to consider these large things as single > registers (even if that's how hw documents it)? Do they need to be set > atomically? How do you get/set them in hardware if GPRs aren't that large? For x86 SSE registers, you usually populate them using memory access IIRC, for altivec it's the same IIUC. What would you suggest? Have 16 pseudo-registers for a single 1024bit register? > >> +4.65 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; >> +}; > > Do we need reg8/16/32, or can we simplify (and reduce potential size > mismatch errors) by using __u64 for everything that doesn't need to be > an array? Hrm. Interesting idea. So you would basically reduce everything to __u64 by padding smaller registers and splitting bigger ones into separate IDs? That really is appealing, but might uglify the code quite a bit when remerging bigger registers. For the padding things should be good. Btw, any reason you're only bringing up these really great ideas on my 2nd pull request after these patches were uncommented on the ML for quite a while? :) > >> +/* Available with KVM_CAP_ONE_REG */ >> + >> +#define KVM_ONE_REG_GENERIC 0x0000000000000000ULL > > Generic registers? Is the idea to use this in place of dedicated ioctls > for certain KVM knobs? Well, it felt logical to reserve some range for "other use". > >> +/* >> + * 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 > > Might want to allow space for more than 16 architectures -- there's room > to spare. I don't think we'll get to 16 KVM-enabled architectures anytime soon. And if we do, we can just declare the full high byte as arch mask and then we have plenty space :). Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.01.2012, at 03:32, Alexander Graf <agraf@suse.de> wrote: > > On 04.01.2012, at 21:08, Scott Wood wrote: > >> On 01/03/2012 07:10 PM, 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. >> >> If we end up replacing get/set sregs with this (either totally or just >> for new registers), we should consider a runtime facility for userspace >> to extract from the kernel a list of supported registers (with sizes) >> that are supported, so that userspace doesn't have to know about every >> register in order to migrate (this could also be useful if there is >> non-architecturally-readable state that needs to be preserved, such as a >> pending book3e doorbell exception, or the MCAR to be set when machine >> checks are unmasked). It could also avoid the need for explicit >> capabilities in some situations when new registers are added (like >> KVM_CAP_PPC_HIOR). > > Interesting idea. That doesn't quite match up with the way we treat migration today with all fields getting copied into the CPUState and then taken on the wire from there, but I can see how a more generic approach feels more elegant. > > Yes, the MANY_REGS interface should be really clever if we want to use it extensively, allowing us to only write a byte stream of exactly the register sizes we care about - no need to waste 128 bytes for every register - and with proper enumeration of available registers. > > But to be honest I'm unsure on what the best interface is really. We have 3 different possible way of implementing register sharing between user space and kernel space: > > 1) Current way with new CAP and struct enhancements or new structs every time we find something we didn't think about. I count ONE_REG in on this one. > 2) Something fancy like GET_MANY_REGS that would allow us to have a very flexible interface between kernel and user space for getting and setting registers, but we would still need an ioctl for them, so the registers shouldn't be read/set in the fast path anyway, at which point maybe using get/set individually with ONE_REG is still ok? > 3) Putting registers in the shared kvm_run struct so kernel and user space can access them; add a "dirty" bitmap in there so kernel space knows when a register is written from user space. This should be the preferred method for "fast path" registers - registers we need very often from user space. > > Since we do have 1) today and 3) should be there for the fast path, I'm not sure how much we really do need 2). But the nice thing here is that we can cross that bridge when we get to it. This patch merely lays the groundwork. > >>> 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. >> >> Does it really make sense to consider these large things as single >> registers (even if that's how hw documents it)? Do they need to be set >> atomically? How do you get/set them in hardware if GPRs aren't that large? > > For x86 SSE registers, you usually populate them using memory access IIRC, for altivec it's the same IIUC. > > What would you suggest? Have 16 pseudo-registers for a single 1024bit register? > >> >>> +4.65 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; >>> +}; >> >> Do we need reg8/16/32, or can we simplify (and reduce potential size >> mismatch errors) by using __u64 for everything that doesn't need to be >> an array? > > Hrm. Interesting idea. So you would basically reduce everything to __u64 by padding smaller registers and splitting bigger ones into separate IDs? That really is appealing, but might uglify the code quite a bit when remerging bigger registers. For the padding things should be good. Ok here's another idea on how to handle this. What if we encode the register size in the constant? That way, if the register grows later, we can still be backwards compatible, but give user space exactly the size it asks for. We could then just take a void* from user space and merely c_t_u and c_f_u the value in exactly the size defined by the constant. Later, for MANY_REGS we could then just take a list of registers and write a bytestream of results to a user pointer. That should be a lot easier and efficient than an interface that treats everything as u64. Alex > > Btw, any reason you're only bringing up these really great ideas on my 2nd pull request after these patches were uncommented on the ML for quite a while? :) > >> >>> +/* Available with KVM_CAP_ONE_REG */ >>> + >>> +#define KVM_ONE_REG_GENERIC 0x0000000000000000ULL >> >> Generic registers? Is the idea to use this in place of dedicated ioctls >> for certain KVM knobs? > > Well, it felt logical to reserve some range for "other use". > >> >>> +/* >>> + * 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 >> >> Might want to allow space for more than 16 architectures -- there's room >> to spare. > > I don't think we'll get to 16 KVM-enabled architectures anytime soon. And if we do, we can just declare the full high byte as arch mask and then we have plenty space :). > > > Alex > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/04/2012 10:07 PM, Alexander Graf wrote: > Ok here's another idea on how to handle this. What if we encode the > register size in the constant? That way, if the register grows later, > we can still be backwards compatible, but give user space exactly the > size it asks for. > > We could then just take a void* from user space and merely c_t_u and c_f_u the value in exactly the size defined by the constant. > Later, for MANY_REGS we could then just take a list of registers and write a bytestream of results to a user pointer. > > That should be a lot easier and efficient than an interface that treats everything as u64. OK. > Alex > >> >> Btw, any reason you're only bringing up these really great ideas on my 2nd pull request after these patches were uncommented on the ML for quite a while? :) Sorry about that -- I was trying to focus on getting e500mc patches out before vacation (and various other distractions), and missed looking at the patch itself the first time around. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/05/2012 06:07 AM, Alexander Graf wrote: > > > > Hrm. Interesting idea. So you would basically reduce everything to __u64 by padding smaller registers and splitting bigger ones into separate IDs? That really is appealing, but might uglify the code quite a bit when remerging bigger registers. For the padding things should be good. > > Ok here's another idea on how to handle this. What if we encode the register size in the constant? That way, if the register grows later, we can still be backwards compatible, but give user space exactly the size it asks for. > > We could then just take a void* from user space and merely c_t_u and c_f_u the value in exactly the size defined by the constant. > > Later, for MANY_REGS we could then just take a list of registers and write a bytestream of results to a user pointer. > > That should be a lot easier and efficient than an interface that treats everything as u64. > Yet another option is to have a large struct which we keep extending, as well as a bitmask of which fields are valid and which aren't. We'd have APIs to query the supported registers, read a struct (with all known registers filled in) and write a struct (with the bitmask indicating which registers are available). This is less flexible, but easier to use, since you don't need to decode the stream.
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 36295d8..b8c558d 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -1523,6 +1523,53 @@ following algorithm: Some guests configure the LINT1 NMI input to cause a panic, aiding in debugging. +4.65 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.66 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 fd70932..4b01823 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -213,6 +213,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 @@ -626,6 +627,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) { @@ -665,6 +692,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 8d40db7..51ab25e 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 #define KVM_CAP_TSC_DEADLINE_TIMER 72 @@ -653,6 +654,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 */ @@ -781,6 +810,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)
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(-)