Re: [PATCH 2/4] KVM: Add unified KVM_GET/SET_VCPU_STATE IOCTL

[Jan Kiszka <jan.kiszka@siemens.com>]

Avi Kivity wrote:
> On 10/14/2009 01:06 AM, Jan Kiszka wrote:
>> Add a new IOCTL pair to retrieve or set the VCPU state in one chunk.
>> More precisely, the IOCTL is able to process a list of substates to be
>> read or written. This list is easily extensible without breaking the
>> existing ABI, thus we will no longer have to add new IOCTLs when we
>> discover a missing VCPU state field or want to support new hardware
>> features.
>>
>> This patch establishes the generic infrastructure for KVM_GET/
>> SET_VCPU_STATE and adds support for the generic substates REGS, SREGS,
>> FPU, and MP. To avoid code duplication, the entry point for the
>> corresponding original IOCTLs are converted to make use of the new
>> infrastructure internally, too.
>>
>>
>>
>> +/* for KVM_GET_VCPU_STATE and KVM_SET_VCPU_STATE */
>> +#define KVM_VCPU_REGS			0
>> +#define KVM_VCPU_SREGS			1
>> +#define KVM_VCPU_FPU			2
>> +#define KVM_VCPU_MP			3
>>    
> 
> KVM_VCPU_STATE_*, to avoid collisions.

OK.

> 
> Better to split sse from fpu since we already know it is about to be 
> replaced.

The idea is to reuse the existing state structures, including struct
kvm_fpu. This allows to provide the avoid substates for all archs and
simplifies the migration (see my qemu conversion patch). I think, once
we need support for new/wider registers in x86, we can introduce new
KVM_X86_VCPU_STATE_FPU_WHATEVER substates that are able to replace the
old one.

> 
>> +
>> +struct kvm_vcpu_substate {
>> +	__u32 type;
>> +	__u32 pad;
>> +	__s64 offset;
>> +};
>> +
>> +#define KVM_MAX_VCPU_SUBSTATES		64
>> +
>> +struct kvm_vcpu_state {
>> +	__u32 nsubstates; /* number of elements in substates */
>> +	__u32 nprocessed; /* return value: successfully processed substates */
>> +	struct kvm_vcpu_substate substates[0];
>> +};
>> +
>>    
> 
> Wouldn't having an ordinary struct with lots of reserved space be 
> simpler?  If we add a bitmask, we can even selectively get/set the 
> fields we want (important if new state extends old state: avx vs sse).

Simpler - hmm, maybe. But also less flexible. This would establish a
static order inside this constantly growing mega struct. And a user only
interested in something small at its end would still have to allocate
memory for the whole thing (maybe megabytes in the future, who knows?).
And this mega struct will always carry all the legacy substates, even if
they aren't used anymore in practice.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux