Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS

[Philippe Mathieu-Daudé]

On 9/8/20 7:59 PM, Eduardo Habkost wrote:
> On Tue, Sep 08, 2020 at 07:25:47PM +0200, Thomas Huth 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.
> 
> If there's a way to make it work with older kernels, I assume we
> would still want to do it.

IIUC this is available since kernel v5.8-rc1 (merged in 52cd0d972fa6)
less than 3 months ago. v5.9-rc4 just got released, not sure if this
can be fixed for the v5.9 release.

Is this small older kernels window important?

Huacai, can you amend in the commit description since when the
feature is available in the Linux kernel please? (just for
informative purpose).

> 
> However, this kind of kvm-specific + arch-specific knowledge
> doesn't belong to machine core code.  If we are going to add a
> #ifdef TARGET_MIPS to the code, we can simply do it inside
> kvm-all.c:kvm_init().
>