Re: [PATCH 4/6] KVM: x86: Introduce KVM_GET_SREGS2 / KVM_SET_SREGS2

[Paolo Bonzini]

Just a quick review on the API:

On 01/04/21 16:18, Maxim Levitsky wrote:
> +struct kvm_sregs2 {
> +	/* out (KVM_GET_SREGS2) / in (KVM_SET_SREGS2) */
> +	struct kvm_segment cs, ds, es, fs, gs, ss;
> +	struct kvm_segment tr, ldt;
> +	struct kvm_dtable gdt, idt;
> +	__u64 cr0, cr2, cr3, cr4, cr8;
> +	__u64 efer;
> +	__u64 apic_base;
> +	__u64 flags; /* must be zero*/

I think it would make sense to define a flag bit for the PDPTRs, so that 
userspace can use KVM_SET_SREGS2 unconditionally (e.g. even when 
migrating from a source that uses KVM_GET_SREGS and therefore doesn't 
provide the PDPTRs).

> +	__u64 pdptrs[4];
> +	__u64 padding;

No need to add padding; if we add more fields in the future we can use 
the flags to determine the length of the userspace data, similar to 
KVM_GET/SET_NESTED_STATE.


> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> +	if (is_pae_paging(vcpu)) {
> +		for (i = 0 ; i < 4 ; i++)
> +			kvm_pdptr_write(vcpu, i, sregs2->pdptrs[i]);
> +		kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR);
> +		mmu_reset_needed = 1;
> +	}
> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +

SRCU should not be needed here?

> +	case KVM_GET_SREGS2: {
> +		u.sregs2 = kzalloc(sizeof(struct kvm_sregs2), GFP_KERNEL_ACCOUNT);
> +		r = -ENOMEM;
> +		if (!u.sregs2)
> +			goto out;

No need to account, I think it's a little slower and this allocation is 
very short lived.

>  #define KVM_CAP_PPC_DAWR1 194
> +#define KVM_CAP_SREGS2 196

195, not 196.

>  #define KVM_XEN_VCPU_GET_ATTR	_IOWR(KVMIO, 0xca, struct kvm_xen_vcpu_attr)
>  #define KVM_XEN_VCPU_SET_ATTR	_IOW(KVMIO,  0xcb, struct kvm_xen_vcpu_attr)
> +
> +#define KVM_GET_SREGS2             _IOR(KVMIO,  0xca, struct kvm_sregs2)
> +#define KVM_SET_SREGS2             _IOW(KVMIO,  0xcb, struct kvm_sregs2)
> +

It's not exactly overlapping, but please bump the ioctls to 0xcc/0xcd.

Paolo