Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 6 September 2012 02:44, Rusty Russell rusty.russ...@linaro.org wrote: Actually, I hadn't realized ARM didn't do 128-bit FP regs already. But I'd guess they'll arrive one day. AArch64 has them. I had thought that you'd be able to treat one 128-bit reg as two 64 bit regs (in the same way that in 32-bit ARM the 64 bit register d0 is accessible as the pair of 32 bit regs s0,s1). However AArch64 doesn't overlap its registers in the same way, so userspace will need to access them as 128 bit registers to get the full state. -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 09/04/2012 04:59 PM, Alexander Graf wrote: Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love. I doubt that anyone wants to run 31-bit user space on an s390x system. In fact, I wouldn't be surprised if exactly that case is broken already. Arnd sent patches to fix it long ago. Of course, something else may be broken. I don't think that is what makes the API hard to use. What is it then? I forgot what the original complaints/complainers were. I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it. x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however. Another option is to use the size parameter from the ioctl. It just sits there doing nothing. It would require quite a bunch of changes throughout the stack. Even in user space, like strace... I'm sure strace could cope. I once had a proposal for extensible ioctl using the size parameter. You want to extend an ioctl, the kernel zero-extends the input struct for you, and truncates it on the way back. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 09/05/2012 09:48 AM, Rusty Russell wrote: Peter Maydell peter.mayd...@linaro.org writes: On 1 September 2012 13:28, Rusty Russell ru...@rustcorp.com.au wrote: Rusty Russell (8): KVM: ARM: Fix walk_msrs() KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code. KVM: Add KVM_REG_SIZE() helper. KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG. KVM: Add KVM_VCPU_GET_REG_LIST. KVM: ARM: Use KVM_VCPU_GET_REG_LIST. KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t). This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved. It's there. There are bits in the id which indicate the size: #define KVM_REG_SIZE_SHIFT52 #define KVM_REG_SIZE_MASK 0x00f0ULL #define KVM_REG_SIZE_U8 0xULL #define KVM_REG_SIZE_U16 0x0010ULL #define KVM_REG_SIZE_U32 0x0020ULL #define KVM_REG_SIZE_U64 0x0030ULL #define KVM_REG_SIZE_U128 0x0040ULL #define KVM_REG_SIZE_U256 0x0050ULL #define KVM_REG_SIZE_U512 0x0060ULL #define KVM_REG_SIZE_U10240x0070ULL Assumes power-of-two registers. On x86 IDTR is 10 bytes long (2 byte limit, 8 byte address). We could split it into two registers, or add padding, but it's unnatural. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 06.09.2012, at 10:48, Avi Kivity a...@redhat.com wrote: On 09/05/2012 09:48 AM, Rusty Russell wrote: Peter Maydell peter.mayd...@linaro.org writes: On 1 September 2012 13:28, Rusty Russell ru...@rustcorp.com.au wrote: Rusty Russell (8): KVM: ARM: Fix walk_msrs() KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code. KVM: Add KVM_REG_SIZE() helper. KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG. KVM: Add KVM_VCPU_GET_REG_LIST. KVM: ARM: Use KVM_VCPU_GET_REG_LIST. KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t). This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved. It's there. There are bits in the id which indicate the size: #define KVM_REG_SIZE_SHIFT52 #define KVM_REG_SIZE_MASK0x00f0ULL #define KVM_REG_SIZE_U80xULL #define KVM_REG_SIZE_U160x0010ULL #define KVM_REG_SIZE_U320x0020ULL #define KVM_REG_SIZE_U640x0030ULL #define KVM_REG_SIZE_U1280x0040ULL #define KVM_REG_SIZE_U2560x0050ULL #define KVM_REG_SIZE_U5120x0060ULL #define KVM_REG_SIZE_U10240x0070ULL Assumes power-of-two registers. On x86 IDTR is 10 bytes long (2 byte limit, 8 byte address). We could split it into two registers, or add padding, but it's unnatural. Why is padding bad? How do you model IDTR throughout the stack today? How does QEMU's savevm serialize it? Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 09/06/2012 06:08 PM, Alexander Graf wrote:
On 06.09.2012, at 10:48, Avi Kivity a...@redhat.com wrote:
On 09/05/2012 09:48 AM, Rusty Russell wrote:
Peter Maydell peter.mayd...@linaro.org writes:
On 1 September 2012 13:28, Rusty Russell ru...@rustcorp.com.au wrote:
Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
KVM ARM: Update api.txt
So I was thinking about this, and I remembered that the SET_ONE_REG/
GET_ONE_REG API has userspace pass a pointer to the variable the
kernel should read/write (unlike the _MSR x86 ioctls, where the
actual data value is sent back and forth in the struct). Further,
the kernel only writes a data value of the size of the register
(rather than always reading/writing a uint64_t).
This is a problem because it means userspace needs to know the
size of each register, and the kernel doesn't provide any way
to determine the size. This defeats the idea that userspace should
be able to migrate kernel register state without having to know
the semantics of all the registers involved.
It's there. There are bits in the id which indicate the size:
#define KVM_REG_SIZE_SHIFT52
#define KVM_REG_SIZE_MASK0x00f0ULL
#define KVM_REG_SIZE_U80xULL
#define KVM_REG_SIZE_U160x0010ULL
#define KVM_REG_SIZE_U320x0020ULL
#define KVM_REG_SIZE_U640x0030ULL
#define KVM_REG_SIZE_U1280x0040ULL
#define KVM_REG_SIZE_U2560x0050ULL
#define KVM_REG_SIZE_U5120x0060ULL
#define KVM_REG_SIZE_U10240x0070ULL
Assumes power-of-two registers. On x86 IDTR is 10 bytes long (2 byte
limit, 8 byte address). We could split it into two registers, or add
padding, but it's unnatural.
(and the APIC, if treated as one-large-register) is 4k)
Why is padding bad?
Where does it come? between the 2 byte and the 8 byte element? After
the 10 bytes?
It means that users must either include the padding in their internal
data structures, or copy to a temporary.
How do you model IDTR throughout the stack today?
struct kvm_dtable {
__u64 base;
__u16 limit;
__u16 padding[3];
};
:p
Internally, it's held in hardware registers.
How does QEMU's savevm serialize it?
Two separate fields (actually four, of which two are ignored).
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 6 September 2012 16:16, Avi Kivity a...@redhat.com wrote: (and the APIC, if treated as one-large-register) is 4k) ...so don't do that then. Trying to treat the whole APIC as a single register means you don't get any of the advantages of does this kernel support this register? etc. Is there some reason I'm not seeing why it would make sense to do it that way? -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 09/06/2012 06:23 PM, Peter Maydell wrote: On 6 September 2012 16:16, Avi Kivity a...@redhat.com wrote: (and the APIC, if treated as one-large-register) is 4k) ...so don't do that then. Trying to treat the whole APIC as a single register means you don't get any of the advantages of does this kernel support this register? etc. Is there some reason I'm not seeing why it would make sense to do it that way? It's just the easiest path forward. one large register is mainly useful if registers have interdependencies. That doesn't exist in the APIC AFAIR, but it does exist elsewhere. Another way to handle interdependencies is to defer applying the changes until a KVM_RUN, and then evaluate them as a group. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Avi Kivity a...@redhat.com writes: On 09/06/2012 06:23 PM, Peter Maydell wrote: On 6 September 2012 16:16, Avi Kivity a...@redhat.com wrote: (and the APIC, if treated as one-large-register) is 4k) ...so don't do that then. Trying to treat the whole APIC as a single register means you don't get any of the advantages of does this kernel support this register? etc. Is there some reason I'm not seeing why it would make sense to do it that way? It's just the easiest path forward. one large register is mainly useful if registers have interdependencies. That doesn't exist in the APIC AFAIR, but it does exist elsewhere. Another way to handle interdependencies is to defer applying the changes until a KVM_RUN, and then evaluate them as a group. The other option is to implement KVM_SET_MULTI_REG. I have enough of an implmentation to show it's trivial, but it's needless complexity until/if we need it. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Avi Kivity a...@redhat.com writes: On 09/03/2012 03:33 PM, Rusty Russell wrote: Avi Kivity a...@redhat.com writes: On 09/01/2012 03:35 PM, Rusty Russell wrote: Passing an address in a struct is pretty bad, since it involves compatibility wrappers. Right, some s390 thing. Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64 Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this. Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love. OK, yes. Or skip pointers altogether, like I do. Another option is to use the size parameter from the ioctl. It just sits there doing nothing. Not nothing, it defines the head struct size. It's redundant, but proven a useful sanity check over the years. Perhaps somewhere else does use these 14 bits to represent a variable size, but it would surprise me a bit to see it. We'd probably want some way to tell userspace the size then, so we have a different redundancy. We're being too clever, that's why I copied the x86 MSR discovery interface. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Peter Maydell peter.mayd...@linaro.org writes: On 1 September 2012 13:28, Rusty Russell ru...@rustcorp.com.au wrote: Rusty Russell (8): KVM: ARM: Fix walk_msrs() KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code. KVM: Add KVM_REG_SIZE() helper. KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG. KVM: Add KVM_VCPU_GET_REG_LIST. KVM: ARM: Use KVM_VCPU_GET_REG_LIST. KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t). This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved. It's there. There are bits in the id which indicate the size: #define KVM_REG_SIZE_SHIFT 52 #define KVM_REG_SIZE_MASK 0x00f0ULL #define KVM_REG_SIZE_U8 0xULL #define KVM_REG_SIZE_U160x0010ULL #define KVM_REG_SIZE_U320x0020ULL #define KVM_REG_SIZE_U640x0030ULL #define KVM_REG_SIZE_U128 0x0040ULL #define KVM_REG_SIZE_U256 0x0050ULL #define KVM_REG_SIZE_U512 0x0060ULL #define KVM_REG_SIZE_U1024 0x0070ULL And my patches added a helper: #define KVM_REG_SIZE(id)\ (1U (((id) KVM_REG_SIZE_MASK) KVM_REG_SIZE_SHIFT)) I could live with always read/write 64 bits. I definitely don't want to have to deal with matching up register widths to accesses in userspace, please. I changed my mind about the old scheme when I realized we have to deal with 128-bit FPU registers. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 5 September 2012 07:48, Rusty Russell ru...@rustcorp.com.au wrote: Peter Maydell peter.mayd...@linaro.org writes: This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved. It's there. There are bits in the id which indicate the size: And my patches added a helper: #define KVM_REG_SIZE(id)\ (1U (((id) KVM_REG_SIZE_MASK) KVM_REG_SIZE_SHIFT)) Ah, right, I hadn't realised that was in the exposed-to-userspace bit of the code. I could live with always read/write 64 bits. I definitely don't want to have to deal with matching up register widths to accesses in userspace, please. I changed my mind about the old scheme when I realized we have to deal with 128-bit FPU registers. Mmm, ARM might not have any awkward size registers but there's x86 weirdisms to consider for a generic ABI I guess. -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Peter Maydell peter.mayd...@linaro.org writes: On 5 September 2012 07:48, Rusty Russell ru...@rustcorp.com.au wrote: Peter Maydell peter.mayd...@linaro.org writes: I could live with always read/write 64 bits. I definitely don't want to have to deal with matching up register widths to accesses in userspace, please. I changed my mind about the old scheme when I realized we have to deal with 128-bit FPU registers. Mmm, ARM might not have any awkward size registers but there's x86 weirdisms to consider for a generic ABI I guess. Actually, I hadn't realized ARM didn't do 128-bit FP regs already. But I'd guess they'll arrive one day. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 09/03/2012 03:33 PM, Rusty Russell wrote: Avi Kivity a...@redhat.com writes: On 09/01/2012 03:35 PM, Rusty Russell wrote: Passing an address in a struct is pretty bad, since it involves compatibility wrappers. Right, some s390 thing. Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64 Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this. Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love. I don't think that is what makes the API hard to use. What is it then? I forgot what the original complaints/complainers were. I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it. x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however. Another option is to use the size parameter from the ioctl. It just sits there doing nothing. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 1 September 2012 13:28, Rusty Russell ru...@rustcorp.com.au wrote: Rusty Russell (8): KVM: ARM: Fix walk_msrs() KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code. KVM: Add KVM_REG_SIZE() helper. KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG. KVM: Add KVM_VCPU_GET_REG_LIST. KVM: ARM: Use KVM_VCPU_GET_REG_LIST. KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t). This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved. Possible solutions: * switch GET/SET_ONE_REG to just passing data, same as the MSR ioctls * switch GET/SET_ONE_REG to always writing 64 bits regardless of actual guest register width * make GET_REG_LIST return register width as well as index Personally I would really prefer the MSR-style pass the data. Otherwise I'm going to end up constructing something like uint64_t actual_values[] struct kvm_one_reg regs[] where regs[x].addr = actual_values[x] for all x. Which seems like unnecessary indirection really :-) I could live with always read/write 64 bits. I definitely don't want to have to deal with matching up register widths to accesses in userspace, please. thanks -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 04.09.2012, at 07:48, Avi Kivity a...@redhat.com wrote: On 09/03/2012 03:33 PM, Rusty Russell wrote: Avi Kivity a...@redhat.com writes: On 09/01/2012 03:35 PM, Rusty Russell wrote: Passing an address in a struct is pretty bad, since it involves compatibility wrappers. Right, some s390 thing. Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64 Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this. Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love. I doubt that anyone wants to run 31-bit user space on an s390x system. In fact, I wouldn't be surprised if exactly that case is broken already. I don't think that is what makes the API hard to use. What is it then? I forgot what the original complaints/complainers were. I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it. x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however. Another option is to use the size parameter from the ioctl. It just sits there doing nothing. It would require quite a bunch of changes throughout the stack. Even in user space, like strace... Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 04.09.2012, at 09:31, Peter Maydell wrote: On 1 September 2012 13:28, Rusty Russell ru...@rustcorp.com.au wrote: Rusty Russell (8): KVM: ARM: Fix walk_msrs() KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code. KVM: Add KVM_REG_SIZE() helper. KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG. KVM: Add KVM_VCPU_GET_REG_LIST. KVM: ARM: Use KVM_VCPU_GET_REG_LIST. KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t). This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. It does, as it's encoded in the register ID. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved. Possible solutions: * switch GET/SET_ONE_REG to just passing data, same as the MSR ioctls * switch GET/SET_ONE_REG to always writing 64 bits regardless of actual guest register width * make GET_REG_LIST return register width as well as index Personally I would really prefer the MSR-style pass the data. Well, the reason I put dynamic sizes in there is that we already have very big register sizes on x86 (265 bits iirc), and so far chances are that it'll rather get bigger than smaller over time. So I would really like to keep the size encoding in the register id so that we can support big multimedia registers later on. Otherwise I'm going to end up constructing something like uint64_t actual_values[] struct kvm_one_reg regs[] where regs[x].addr = actual_values[x] for all x. Which seems like unnecessary indirection really :-) I could live with always read/write 64 bits. I definitely don't want to have to deal with matching up register widths to accesses in userspace, please. If I understood Rusty correctly, he wanted to do exactly that. Just make all the ARM registers be 64-bit wide, so that you can just keep them all as uint64_t in QEMU's env and then put env's pointers into the ONE_REG ioctl. Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 09/01/2012 03:35 PM, Rusty Russell wrote:
Avi Kivity a...@redhat.com writes:
-Capability: basic
+Capability: KVM_CAP_REG_LIST
Architectures: arm
all
OK, I guess that's to be true in future. Fixed.
Type: vcpu ioctl
-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
Returns: 0 on success; -1 on error
Errors:
- E2BIG: the msr index list is too big to fit in the array specified by
- the user.
+ E2BIG: the reg index list is too big to fit in the array specified by
+ the user (the number required will be written into n).
struct kvm_msr_list {
- __u32 nmsrs; /* number of msrs in entries */
- __u32 indices[0];
+ __u64 n; /* number of registers in reg[] */
+ __u64 reg[0];
};
People complain that this interface is hard to use.
How about supplying the address of the array (in addition to n) so you
don't have to deal with variable sized arrays, and dropping E2BIG in
favour of always updating n (n changed to something bigger than you had
- reallocate and rerun)
We re-write n anyway, *and* return -E2BIG. Not returning an error is
asking for trouble.
Passing an address in a struct is pretty bad, since it involves
compatibility wrappers.
Right, some s390 thing.
I don't think that is what makes the API hard
to use.
What is it then? I forgot what the original complaints/complainers were.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Avi Kivity a...@redhat.com writes: On 09/01/2012 03:35 PM, Rusty Russell wrote: Passing an address in a struct is pretty bad, since it involves compatibility wrappers. Right, some s390 thing. Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64 Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this. I don't think that is what makes the API hard to use. What is it then? I forgot what the original complaints/complainers were. I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it. x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 3 September 2012 13:33, Rusty Russell ru...@rustcorp.com.au wrote: I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it. x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however. Yes, I didn't particularly find the ioctls Rusty initially suggested hard to use (apart from briefly getting confused between struct kvm_msrs and struct kvm_msr_list; you could collapse those down into one struct if you wanted I suppose). The nastiest problem in the target-i386 qemu code that gets the MSR list is the fact it has to work around some ancient kernel bug where the ioctl would write beyond the end of the array... -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 08/29/2012 11:39 AM, Rusty Russell wrote:
-4.76 KVM_VCPU_GET_MSR_INDEX_LIST
+4.76 KVM_VCPU_GET_REG_LIST
-Capability: basic
+Capability: KVM_CAP_REG_LIST
Architectures: arm
all
Type: vcpu ioctl
-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
Returns: 0 on success; -1 on error
Errors:
- E2BIG: the msr index list is too big to fit in the array specified by
- the user.
+ E2BIG: the reg index list is too big to fit in the array specified by
+ the user (the number required will be written into n).
struct kvm_msr_list {
- __u32 nmsrs; /* number of msrs in entries */
- __u32 indices[0];
+ __u64 n; /* number of registers in reg[] */
+ __u64 reg[0];
};
People complain that this interface is hard to use.
How about supplying the address of the array (in addition to n) so you
don't have to deal with variable sized arrays, and dropping E2BIG in
favour of always updating n (n changed to something bigger than you had
- reallocate and rerun)
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Rusty Russell rusty.russ...@linaro.org writes: Hi all, This compiles, completely untested, but it's my attempt to give Avi (and Alexander) what he asked for in a generic register accessor. And here's the tested version: see my new onereg-abi branch. My next step is to demux CSELR, but that won't be until Tuesday. Cheers, Rusty. The following changes since commit bc9cf74d26786dd2063155f9c703b8cb19d4270d: KVM: ARM: Add trace and fix prints on guest aborts (2012-08-28 22:35:53 -0700) are available in the git repository at: gitol...@ra.kernel.org:/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi for you to fetch changes up to fcecd7ddb31cf75d509b7a7bb2df033042b1d6a8: KVM ARM: Update api.txt (2012-09-01 21:56:06 +0930) Rusty Russell (8): KVM: ARM: Fix walk_msrs() KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code. KVM: Add KVM_REG_SIZE() helper. KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG. KVM: Add KVM_VCPU_GET_REG_LIST. KVM: ARM: Use KVM_VCPU_GET_REG_LIST. KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt Documentation/virtual/kvm/api.txt | 61 - arch/arm/include/asm/kvm.h | 77 arch/arm/include/asm/kvm_coproc.h |6 +- arch/arm/include/asm/kvm_host.h | 35 -- arch/arm/kvm/arm.c | 29 - arch/arm/kvm/coproc.c | 234 +++ arch/arm/kvm/emulate.c |2 +- arch/arm/kvm/guest.c| 158 ++- arch/arm/kvm/reset.c|4 +- arch/powerpc/include/asm/kvm_host.h |1 + arch/powerpc/kvm/book3s_hv.c|4 +- arch/powerpc/kvm/book3s_pr.c|4 +- arch/powerpc/kvm/booke.c|4 +- arch/powerpc/kvm/powerpc.c | 15 --- arch/s390/include/asm/kvm_host.h|1 + arch/s390/kvm/kvm-s390.c| 19 +-- include/linux/kvm.h | 10 +- include/linux/kvm_host.h|9 +- virt/kvm/kvm_main.c | 38 ++ 19 files changed, 340 insertions(+), 371 deletions(-) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Avi Kivity a...@redhat.com writes:
-Capability: basic
+Capability: KVM_CAP_REG_LIST
Architectures: arm
all
OK, I guess that's to be true in future. Fixed.
Type: vcpu ioctl
-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
Returns: 0 on success; -1 on error
Errors:
- E2BIG: the msr index list is too big to fit in the array specified by
- the user.
+ E2BIG: the reg index list is too big to fit in the array specified by
+ the user (the number required will be written into n).
struct kvm_msr_list {
-__u32 nmsrs; /* number of msrs in entries */
-__u32 indices[0];
+__u64 n; /* number of registers in reg[] */
+__u64 reg[0];
};
People complain that this interface is hard to use.
How about supplying the address of the array (in addition to n) so you
don't have to deal with variable sized arrays, and dropping E2BIG in
favour of always updating n (n changed to something bigger than you had
- reallocate and rerun)
We re-write n anyway, *and* return -E2BIG. Not returning an error is
asking for trouble.
Passing an address in a struct is pretty bad, since it involves
compatibility wrappers. I don't think that is what makes the API hard
to use.
Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Rusty Russell ru...@rustcorp.com.au writes: Rusty Russell rusty.russ...@linaro.org writes: Hi all, This compiles, completely untested, but it's my attempt to give Avi (and Alexander) what he asked for in a generic register accessor. And here's the tested version: see my new onereg-abi branch. My next step is to demux CSELR, but that won't be until Tuesday. Cheers, Rusty. The following changes since commit bc9cf74d26786dd2063155f9c703b8cb19d4270d: KVM: ARM: Add trace and fix prints on guest aborts (2012-08-28 22:35:53 -0700) are available in the git repository at: gitol...@ra.kernel.org:/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi ie: git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
On 29 August 2012 00:37, Rusty Russell rusty.russ...@linaro.org wrote: This compiles, completely untested, but it's my attempt to give Avi (and Alexander) what he asked for in a generic register accessor. Mingled in these patches is the conversion of the latest KVM ARM code, which is the first proposed user: by the end, we use these accessors for *every* register and piece of state. GET_MULTI/SET_MULTI is an obvious extension which is not yet implemented. Hi Rusty. I don't see any api.txt patches in here, did I miss them? (your cover letter doesn't include a diffstat...) I don't particularly have comments on the implementation but I would like to see the kernel-userspace ABI clearly described rather than having to infer it from the code. (including the complete set of index mappings for the ARM registers that will use this) thanks -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Peter Maydell peter.mayd...@linaro.org writes:
On 29 August 2012 00:37, Rusty Russell rusty.russ...@linaro.org wrote:
This compiles, completely untested, but it's my attempt to give
Avi (and Alexander) what he asked for in a generic register accessor.
Mingled in these patches is the conversion of the latest KVM ARM code,
which is the first proposed user: by the end, we use these accessors for
*every* register and piece of state.
GET_MULTI/SET_MULTI is an obvious extension which is not yet
implemented.
Hi Rusty.
I don't see any api.txt patches in here, did I miss them?
(your cover letter doesn't include a diffstat...)
I don't particularly have comments on the implementation but I would
like to see the kernel-userspace ABI clearly described rather than
having to infer it from the code. (including the complete set of
index mappings for the ARM registers that will use this)
It would look something like this:
diff --git a/Documentation/virtual/kvm/api.txt
b/Documentation/virtual/kvm/api.txt
index 19d8915..c0453d7 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -293,7 +293,7 @@ kvm_run' (see below).
4.11 KVM_GET_REGS
Capability: basic
-Architectures: all
+Architectures: all except ARM
Type: vcpu ioctl
Parameters: struct kvm_regs (out)
Returns: 0 on success, -1 on error
@@ -314,7 +314,7 @@ struct kvm_regs {
4.12 KVM_SET_REGS
Capability: basic
-Architectures: all
+Architectures: all except ARM
Type: vcpu ioctl
Parameters: struct kvm_regs (in)
Returns: 0 on success, -1 on error
@@ -1733,6 +1733,17 @@ registers, find a list below:
| |
PPC | KVM_REG_PPC_HIOR | 64
+ARM registers are mapped using the lower 32 bits. The upper 16 of that
+is the coprocessor number (or 16 for core registers):
+
+ARM 32-bit CP15 registers have the following id bit patterns:
+ 0x4002 000F zero:1 crn:4 crm:4 opc1:4 opc2:3
+
+ARM 64-bit CP15 registers have the following id bit patterns:
+ 0x4003 000F zero:1 zero:4 crm:4 opc1:4 zero:3
+
+ARM core registers have the following id format:
+ 0x4003 0010 offset in struct kvm_regs, divided by 4
4.69 KVM_GET_ONE_REG
@@ -1986,50 +1997,26 @@ the virtualized real-mode area (VRMA) facility, the
kernel will
re-create the VMRA HPTEs on the next KVM_RUN of any vcpu.)
-4.76 KVM_VCPU_GET_MSR_INDEX_LIST
+4.76 KVM_VCPU_GET_REG_LIST
-Capability: basic
+Capability: KVM_CAP_REG_LIST
Architectures: arm
Type: vcpu ioctl
-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
Returns: 0 on success; -1 on error
Errors:
- E2BIG: the msr index list is too big to fit in the array specified by
- the user.
+ E2BIG: the reg index list is too big to fit in the array specified by
+ the user (the number required will be written into n).
struct kvm_msr_list {
- __u32 nmsrs; /* number of msrs in entries */
- __u32 indices[0];
+ __u64 n; /* number of registers in reg[] */
+ __u64 reg[0];
};
-This ioctl returns the guest special registers that are supported, and
-is only valid after KVM_ARM_VCPU_INIT has been performed to initialize
-the vcpu type and features. It is otherwise the equivalent of the
-x86-specific KVM_GET_MSR_INDEX_LIST, for arm's coprocessor registers
-and other non-register state.
-
-The numbering for the indices for coprocesors is simple: the upper 16
-bits are the coprocessor number. If it's 15, it's something else,
-for future expansion.
-
-Bit 15 indicates a 64-bit register. For 64 bit registers the bottom 4
-bits are CRm, the next 4 are opc1 (just like the MCRR/MRCC instruction
-encoding). For 32 bit registers, the bottom 4 bits are CRm, the next
-3 are opc2, the next 4 CRn, and the next 3 opc1 (the same order as the
-MRC/MCR instruction encoding, but not the same bit positions).
-
-64-bit coprocessor register:
- ...|19 18 17 16|15|14 13 12 11 10 9 8| 7 6 5 4 |3 2 1 0|
- ...0 0 | cp num | 1| 0 0 0 0 0 0 0| opc1 | CRm |
-
-32-bit coprocessor register:
- ...|19 18 17 16|15|14|13 12 11|10 9 8 7 |6 5 4 |3 2 1 0|
- ...0 0 | cp num | 0| 0| opc1 | CRn | opc2 | CRm |
-
-Non-coprocessor register:
-
- | 32 31 30 29 28 27 26 25 24 23 22 21 20|19 18 17 16 15 ...
- | some non-zero value | ...
+This ioctl returns the guest registers that are supported for the
+KVM_GET_ONE_REG/KVM_SET_ONE_REG calls, and is only valid after
+KVM_ARM_VCPU_INIT has been performed to initialize the vcpu type and
+features.
4.77 KVM_ARM_VCPU_INIT
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 0/5] Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.
Hi all, This compiles, completely untested, but it's my attempt to give Avi (and Alexander) what he asked for in a generic register accessor. Mingled in these patches is the conversion of the latest KVM ARM code, which is the first proposed user: by the end, we use these accessors for *every* register and piece of state. GET_MULTI/SET_MULTI is an obvious extension which is not yet implemented. Comments please! Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
