Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

[Pavel Fedin]

 Hello!

 Replying to everything in one message.

> > As far as i understand this code, KVM_EXIT_HYPERV is called when one
> > of three MSRs are accessed. But, shouldn't we have implemented
> > instead something more generic, like KVM_EXIT_REG_IO, which would
> > work similar to KVM_EXIT_PIO or KVM_EXIT_MMIO, but carry register
> > code and value?
> 
> Yes, we considered that.  There were actually patches for this as well.

 Ah, i missed them, what a pity. There are lots of patches, i don't review them 
all. Actually i have noticed the change only after
it appeared in linux-next.

>  However, in this case the register is still emulated in the kernel, and
> userspace just gets informed of the new value.

 I see, but this doesn't change the semantic. All we need to do is to tell the 
userland that "register has been written".
Additionally to this we could do whatever we want, including caching the data 
in kernel, using it in kernel, and processing reads in
kernel.

> If we do get that, we will just rename KVM_EXIT_HYPERV to
> KVM_EXIT_MSR_ACCESS, and KVM_EXIT_HYPERV_SYNIC to
> KVM_EXIT_MSR_HYPERV_SYNIC, and struct kvm_hyperv_exit to kvm_msr_exit.

 Actually, i see this in more generic way, something like:

                /* KVM_EXIT_REG_ACCESS */
                struct {
                        __u64 reg;
                        __u64 data;
                        __u8  is_write;
                } mmio;
 
 'data' and 'is_write' are self-explanatory, 'reg' would be generalized 
register code, the same as used for KVM_(GET|SET)_ONE_REG:
 - for ARM64: ARM64_SYS_REG(op0, op1, crn, crm, op2) - see
http://lxr.free-electrons.com/source/arch/arm64/include/uapi/asm/kvm.h#L189
 - for x86  : to be defined (i know, we don't use ..._ONE_REG operations here 
yet), like X86_MSR_REG(id), where the macro itself is:

        #define X86_MSR_REG(id) (KVM_REG_X86 | KVM_REG_X86_MSR | 
KVM_REG_SIZE_U64 | (id))

 - for other architectures: to be defined in a similar way, once needed.

> On brief inspection of Andrey's patch (I have not been following
> closely) it looks like the kvm_hyperv_exit struct that's returned to
> userspace contains more data (control, evt_page, and msg_page fields)
> than simply the value of the MSR, so would the desired SynIC exit fit
> into a general-purpose exit for MSR emulation?

 I have looked at the code too, and these three fields are nothing more than 
values of respective MSR's:

        case HV_X64_MSR_SCONTROL:
                synic->control = data;
                if (!host)
                        synic_exit(synic, msr);
                break;

....

        case HV_X64_MSR_SIEFP:
                if (data & HV_SYNIC_SIEFP_ENABLE)
                        if (kvm_clear_guest(vcpu->kvm,
                                            data & PAGE_MASK, PAGE_SIZE)) {
                                ret = 1;
                                break;
                        }
                synic->evt_page = data;
                if (!host)
                        synic_exit(synic, msr);
                break;
        case HV_X64_MSR_SIMP:
                if (data & HV_SYNIC_SIMP_ENABLE)
                        if (kvm_clear_guest(vcpu->kvm,
                                            data & PAGE_MASK, PAGE_SIZE)) {
                                ret = 1;
                                break;
                        }
                synic->msg_page = data;
                if (!host)
                        synic_exit(synic, msr);
                break;

 So, every time one of these thee MSRs is written, we get a vmexit with values 
of all three registers, and that's all. We could
easily have 'synic_exit(synic, msr, data)' in all three cases, and i think the 
userspace could easily deal with proposed
KVM_EXIT_REG_ACCESS, just cache these values internally if needed.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia