Re: [Qemu-devel] [PATCH v2 2/9] i386/kvm: add support for KVM_GET_SUPPORTED_HV_CPUID

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Roman Kagan <rkagan@virtuozzo.com> writes:

> On Mon, May 27, 2019 at 06:39:53PM +0200, Vitaly Kuznetsov wrote:
>> Roman Kagan <rkagan@virtuozzo.com> writes:
>> > On Fri, May 17, 2019 at 04:19:17PM +0200, Vitaly Kuznetsov wrote:
>> >> +static struct kvm_cpuid2 *try_get_hv_cpuid(CPUState *cs, int max)
>> >> +{
>> >> +    struct kvm_cpuid2 *cpuid;
>> >> +    int r, size;
>> >> +
>> >> +    size = sizeof(*cpuid) + max * sizeof(*cpuid->entries);
>> >> +    cpuid = g_malloc0(size);
>> >> +    cpuid->nent = max;
>> >> +
>> >> +    r = kvm_vcpu_ioctl(cs, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
>> >> +    if (r == 0 && cpuid->nent >= max) {
>> >> +        r = -E2BIG;
>> >> +    }
>> >> +    if (r < 0) {
>> >> +        if (r == -E2BIG) {
>> >> +            g_free(cpuid);
>> >> +            return NULL;
>> >> +        } else {
>> >> +            fprintf(stderr, "KVM_GET_SUPPORTED_HV_CPUID failed: %s\n",
>> >> +                    strerror(-r));
>> >> +            exit(1);
>> >> +        }
>> >> +    }
>> >> +    return cpuid;
>> >> +}
>> >> +
>> >> +/*
>> >> + * Run KVM_GET_SUPPORTED_HV_CPUID ioctl(), allocating a buffer large enough
>> >> + * for all entries.
>> >> + */
>> >> +static struct kvm_cpuid2 *get_supported_hv_cpuid(CPUState *cs)
>> >> +{
>> >> +    struct kvm_cpuid2 *cpuid;
>> >> +    int max = 7; /* 0x40000000..0x40000005, 0x4000000A */
>> >> +
>> >> +    /*
>> >> +     * When the buffer is too small, KVM_GET_SUPPORTED_HV_CPUID fails with
>> >> +     * -E2BIG, however, it doesn't report back the right size. Keep increasing
>> >> +     * it and re-trying until we succeed.
>> >> +     */
>> >
>> > I'm still missing the idea of reiterating more than once: the ioctl
>> > returns the actual size of the array.
>> 
>> Hm, I think I checked that and it doesn't seem to be the case.
>> 
>> The code in kvm_vcpu_ioctl_get_hv_cpuid():
>> 
>> 	if (cpuid->nent < nent)
>> 		return -E2BIG;
>> 
>> 	if (cpuid->nent > nent)
>> 		cpuid->nent = nent;
>> 
>> (I think I even ran a test after your comment on v1 and it it
>> confirmed nent is not set on E2BIG). Am I missing something obvious?
>
> Indeed, I saw kvm_vcpu_ioctl_get_cpuid2() always setting ->nent on
> return and assumed so did kvm_vcpu_ioctl_get_hv_cpuid().  I stand
> corrected, please disregard this comment.

No problem at all!

> (What was the reason for not following this pattern in
> kvm_vcpu_ioctl_get_hv_cpuid BTW?)

The opportunity to set nent in E2BIG case was probabbly overlooked. I
was looking at QEMU's get_supported_cpuid() implementation which
iterates trying to find the right number and used this as a pattern.

While setting nent in E2BIG case seems to be very convenient, this is an
unobvious side-effect: usually, where the return value indicates an
error we don't inspect the payload. I'm, however, not at all against
changing kvm_vcpu_ioctl_get_hv_cpuid(). Unfortunately, this won't help
QEMU and we'll still have to iterate to support legacy kernels.

-- 
Vitaly