[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS
From: |
Thomas Huth |
Subject: |
Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS |
Date: |
Wed, 9 Sep 2020 09:20:34 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
On 09/09/2020 04.57, chen huacai wrote:
> Hi, all,
>
> On Wed, Sep 9, 2020 at 1:25 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 24/08/2020 10.11, Huacai Chen wrote:
>>> MIPS has two types of KVM: TE & VZ, and TE is the default type. Now,
>>> libvirt uses a null-machine to detect the kvm capability. In the MIPS
>>> case, it will return "KVM not supported" on a VZ platform by default.
>>> So, add the kvm_type() hook to the null-machine.
>>>
>>> This seems not a very good solution, but I cannot do it better now.
>>
>> This is still ugly. Why do the other architectures do not have the
>> same problem? Let's see... in kvm-all.c, we have:
>>
>> int type = 0;
>> [...]
>> kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
>> if (mc->kvm_type) {
>> type = mc->kvm_type(ms, kvm_type);
>> } else if (kvm_type) {
>> ret = -EINVAL;
>> fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
>> goto err;
>> }
>>
>> do {
>> ret = kvm_ioctl(s, KVM_CREATE_VM, type);
>> } while (ret == -EINTR);
>>
>> Thus the KVM_CREATE_VM ioctl is likely called with type = 0 in this
>> case (i.e. when libvirt probes with the "null"-machine).
>>
>> Now let's have a look at the kernel. The "type" parameter is passed
>> there to the architecture specific function kvm_arch_init_vm().
>> For powerpc, this looks like:
>>
>> if (type == 0) {
>> if (kvmppc_hv_ops)
>> kvm_ops = kvmppc_hv_ops;
>> else
>> kvm_ops = kvmppc_pr_ops;
>> if (!kvm_ops)
>> goto err_out;
>> } else if (type == KVM_VM_PPC_HV) {
>> if (!kvmppc_hv_ops)
>> goto err_out;
>> kvm_ops = kvmppc_hv_ops;
>> } else if (type == KVM_VM_PPC_PR) {
>> if (!kvmppc_pr_ops)
>> goto err_out;
>> kvm_ops = kvmppc_pr_ops;
>> } else
>> goto err_out;
>>
>> That means for type == 0, it automatically detects the best
>> kvm-type.
>>
>> For mips, this function looks like this:
>>
>> switch (type) {
>> #ifdef CONFIG_KVM_MIPS_VZ
>> case KVM_VM_MIPS_VZ:
>> #else
>> case KVM_VM_MIPS_TE:
>> #endif
>> break;
>> default:
>> /* Unsupported KVM type */
>> return -EINVAL;
>> };
>>
>> That means, for type == 0, it returns -EINVAL here!
>>
>> Looking at the API docu in Documentation/virt/kvm/api.rst
>> the description of the type parameter is quite sparse, but it
>> says:
>>
>> "You probably want to use 0 as machine type."
>>
>> So I think this is a bug in the implementation of KVM in the
>> mips kernel code. The kvm_arch_init_vm() in the mips code should
>> do the same as on powerpc, and use the best available KVM type
>> there instead of returning EINVAL. Once that is fixed there,
>> you don't need this patch here for QEMU anymore.
> Yes, PPC use a good method, because it can use 0 as "automatic"
> #define KVM_VM_PPC_HV 1
> #define KVM_VM_PPC_PR 2
>
> Unfortunately, MIPS cannot do like this because it define 0 as "TE":
> #define KVM_VM_MIPS_TE 0
> #define KVM_VM_MIPS_VZ 1
>
> So, it cannot be solved in kernel side, unless changing the definition
> of TE/VZ, but I think changing their definition is also unacceptable.
Ouch, ok, now I understood the problem. That sounds like a really bad
decision on the kernel side.
But I think you could at least try to get it fixed on the kernel side:
Propose a patch to rename KVM_VM_MIPS_TE to KVM_VM_MIPS_DEFAULT and use
2 as new value for TE. The code that handles the default 0 case should
then prefer TE over VZ, so that old userspace applications that provide
0 will continue to work as they did before, so I hope that the change is
acceptable by the kernel folks. You should add a remark to the patch
description that 0 is the established default value for probing in the
QEMU/libvirt stack and that your patch is required on the kernel side to
avoid ugly hacks in QEMU userspace code.
If they still reject your patch, I guess we have to bite the bullet and
add your patch here...
Thanks,
Thomas