qemuProcessVerifyHypervFeatures is supposed to check whether all
requested hyperv features were actually honored by QEMU/KVM. This is
done by checking the corresponding CPUID bits reported by the virtual
CPU. In other words, it doesn't work for string properties, such as
VIR_DOMAIN_HYPERV_VENDOR_ID (there is no CPUID bit we could check). We
could theoretically check all 12 bits corresponding to the vendor
string, but luckily we don't have to check the feature at all. If QEMU
is too old to support hyperv features, the domain won't even start.
Otherwise, it is always supported.

Without this patch, libvirt refuses to start a domain which contains

  <features>
    <hyperv>
      <vendor_id state='on' value='...'/>
    </hyperv>
  </features>

reporting internal error: "unknown CPU feature __kvm_hv_vendor_id.

This regression was introduced by commit v3.1.0-186-ge9dbe7011, which
(by fixing the virCPUDataCheckFeature condition in
qemuProcessVerifyHypervFeatures) revealed an old bug in the feature
verification code. It's been there ever since the verification was
implemented by commit v1.3.3-rc1-5-g95bbe4bf5, which effectively did not
check VIR_DOMAIN_HYPERV_VENDOR_ID at all.

https://bugzilla.redhat.com/show_bug.cgi?id=1439424

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---

BTW, I think this should be cherry-picked in v3.2-maint branch.

 src/qemu/qemu_process.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 18ff1e143..0f64200da 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3793,6 +3793,10 @@ qemuProcessVerifyHypervFeatures(virDomainDefPtr def,
     int rc;
 
     for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) {
+        /* always supported string property */
+        if (i == VIR_DOMAIN_HYPERV_VENDOR_ID)
+            continue;
+
         if (def->hyperv_features[i] != VIR_TRISTATE_SWITCH_ON)
             continue;
 
@@ -3821,13 +3825,13 @@ qemuProcessVerifyHypervFeatures(virDomainDefPtr def,
         case VIR_DOMAIN_HYPERV_SYNIC:
         case VIR_DOMAIN_HYPERV_STIMER:
         case VIR_DOMAIN_HYPERV_RESET:
-        case VIR_DOMAIN_HYPERV_VENDOR_ID:
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("host doesn't support hyperv '%s' feature"),
                            virDomainHypervTypeToString(i));
             return -1;
 
         /* coverity[dead_error_begin] */
+        case VIR_DOMAIN_HYPERV_VENDOR_ID:
         case VIR_DOMAIN_HYPERV_LAST:
             break;
         }
-- 
2.12.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 06, 2017 at 14:20:39 +0200, Jiri Denemark wrote:
> qemuProcessVerifyHypervFeatures is supposed to check whether all
> requested hyperv features were actually honored by QEMU/KVM. This is
> done by checking the corresponding CPUID bits reported by the virtual
> CPU. In other words, it doesn't work for string properties, such as
> VIR_DOMAIN_HYPERV_VENDOR_ID (there is no CPUID bit we could check). We
> could theoretically check all 12 bits corresponding to the vendor

12 bytes

> string, but luckily we don't have to check the feature at all. If QEMU
> is too old to support hyperv features, the domain won't even start.
> Otherwise, it is always supported.
> 
> Without this patch, libvirt refuses to start a domain which contains
> 
>   <features>
>     <hyperv>
>       <vendor_id state='on' value='...'/>
>     </hyperv>
>   </features>
> 
> reporting internal error: "unknown CPU feature __kvm_hv_vendor_id.
> 
> This regression was introduced by commit v3.1.0-186-ge9dbe7011, which
> (by fixing the virCPUDataCheckFeature condition in
> qemuProcessVerifyHypervFeatures) revealed an old bug in the feature
> verification code. It's been there ever since the verification was
> implemented by commit v1.3.3-rc1-5-g95bbe4bf5, which effectively did not
> check VIR_DOMAIN_HYPERV_VENDOR_ID at all.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1439424
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
> 
> BTW, I think this should be cherry-picked in v3.2-maint branch.

Yes

> 
>  src/qemu/qemu_process.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 06, 2017 at 14:28:09 +0200, Peter Krempa wrote:
> On Thu, Apr 06, 2017 at 14:20:39 +0200, Jiri Denemark wrote:
> > qemuProcessVerifyHypervFeatures is supposed to check whether all
> > requested hyperv features were actually honored by QEMU/KVM. This is
> > done by checking the corresponding CPUID bits reported by the virtual
> > CPU. In other words, it doesn't work for string properties, such as
> > VIR_DOMAIN_HYPERV_VENDOR_ID (there is no CPUID bit we could check). We
> > could theoretically check all 12 bits corresponding to the vendor
> 
> 12 bytes

Oops, I used 96 bits as "bytes" sounds strange to me in CPUID context.

> ACK


Thanks, pushed to both master and v3.2-maint.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list