[libvirt] [PATCHv1.5 1/2] cpu:x86: fix specified features will disappear after migrate/restore
https://bugzilla.redhat.com/show_bug.cgi?id=1207095 We allow set the feature with the host-passthrough cpu, but won't save them in the migration xml, the features we specified will disappear after migrate/restore. This is because we skip the virCPUDefUpdateFeature if cpu mode is host-passthrough. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v1.5: just update the description. src/cpu/cpu_x86.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 2a14705..26601b6 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2087,8 +2087,7 @@ x86UpdateCustom(virCPUDefPtr guest, static int x86UpdateHostModel(virCPUDefPtr guest, - const virCPUDef *host, - bool passthrough) + const virCPUDef *host) { virCPUDefPtr oldguest = NULL; const struct x86_map *map; @@ -2124,7 +2123,7 @@ x86UpdateHostModel(virCPUDefPtr guest, } } } - for (i = 0; !passthrough && i < oldguest->nfeatures; i++) { + for (i = 0; i < oldguest->nfeatures; i++) { if (virCPUDefUpdateFeature(guest, oldguest->features[i].name, oldguest->features[i].policy) < 0) @@ -2149,11 +2148,11 @@ x86Update(virCPUDefPtr guest, case VIR_CPU_MODE_HOST_MODEL: guest->match = VIR_CPU_MATCH_EXACT; - return x86UpdateHostModel(guest, host, false); + return x86UpdateHostModel(guest, host); case VIR_CPU_MODE_HOST_PASSTHROUGH: guest->match = VIR_CPU_MATCH_MINIMUM; - return x86UpdateHostModel(guest, host, true); + return x86UpdateHostModel(guest, host); case VIR_CPU_MODE_LAST: break; -- 1.8.3.1
From the documentation :"With this mode, the CPU visible to the guest should be exactly the same as the host CPU even in the aspects that libvirt does not understand."
So this place document need fix. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 64ee049..3cd0ff4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4024,8 +4024,10 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, bool ret = false; size_t i; - /* no features are passed to QEMU with -cpu host - * so it makes no sense to verify them */ + /* Although we allow set features with host-passthrough + * cpu mode, we allow user require/disable the feature + * that libvirt does not understand, so it makes no sense + * to verify them */ if (def->cpu && def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) return true; -- 1.8.3.1
On Wed, Jun 17, 2015 at 22:22:45 +0800, Luyao Huang wrote:
From the documentation :"With this mode, the CPU visible to the guest should be exactly the same as the host CPU even in the aspects that libvirt does not understand."
So this place document need fix.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 64ee049..3cd0ff4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4024,8 +4024,10 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, bool ret = false; size_t i;
- /* no features are passed to QEMU with -cpu host - * so it makes no sense to verify them */ + /* Although we allow set features with host-passthrough + * cpu mode, we allow user require/disable the feature + * that libvirt does not understand, so it makes no sense + * to verify them */ if (def->cpu && def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) return true;
NACK. We certainly don't want features to be just passed through without any validation if used with host-passthrough. We rather need to fix the code check the features used in a guest <cpu> element are all known to libvirt. Jirka
On 06/18/2015 08:42 PM, Jiri Denemark wrote:
On Wed, Jun 17, 2015 at 22:22:45 +0800, Luyao Huang wrote:
From the documentation :"With this mode, the CPU visible to the guest should be exactly the same as the host CPU even in the aspects that libvirt does not understand."
So this place document need fix.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 64ee049..3cd0ff4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4024,8 +4024,10 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, bool ret = false; size_t i;
- /* no features are passed to QEMU with -cpu host - * so it makes no sense to verify them */ + /* Although we allow set features with host-passthrough + * cpu mode, we allow user require/disable the feature + * that libvirt does not understand, so it makes no sense + * to verify them */ if (def->cpu && def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) return true;
NACK. We certainly don't want features to be just passed through without any validation if used with host-passthrough. We rather need to fix the code check the features used in a guest <cpu> element are all known to libvirt.
Okay, get it Thanks a lot for your review.
Jirka
Luyao
On Wed, Jun 17, 2015 at 22:22:44 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1207095
We allow set the feature with the host-passthrough cpu, but won't save them in the migration xml, the features we specified will disappear after migrate/restore. This is because we skip the virCPUDefUpdateFeature if cpu mode is host-passthrough.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v1.5: just update the description.
src/cpu/cpu_x86.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 2a14705..26601b6 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2087,8 +2087,7 @@ x86UpdateCustom(virCPUDefPtr guest,
static int x86UpdateHostModel(virCPUDefPtr guest, - const virCPUDef *host, - bool passthrough) + const virCPUDef *host) { virCPUDefPtr oldguest = NULL; const struct x86_map *map; @@ -2124,7 +2123,7 @@ x86UpdateHostModel(virCPUDefPtr guest, } } } - for (i = 0; !passthrough && i < oldguest->nfeatures; i++) { + for (i = 0; i < oldguest->nfeatures; i++) { if (virCPUDefUpdateFeature(guest, oldguest->features[i].name, oldguest->features[i].policy) < 0)
While this looks correct, save/restore (and likely migration too) with -cpu host has even more bugs (e.g., -cpu host turns into -cpu host,+... after save/restore) and I'd like to fix all that at once. Jirka
On 06/18/2015 09:29 PM, Jiri Denemark wrote:
On Wed, Jun 17, 2015 at 22:22:44 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1207095
We allow set the feature with the host-passthrough cpu, but won't save them in the migration xml, the features we specified will disappear after migrate/restore. This is because we skip the virCPUDefUpdateFeature if cpu mode is host-passthrough.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v1.5: just update the description.
src/cpu/cpu_x86.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 2a14705..26601b6 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2087,8 +2087,7 @@ x86UpdateCustom(virCPUDefPtr guest,
static int x86UpdateHostModel(virCPUDefPtr guest, - const virCPUDef *host, - bool passthrough) + const virCPUDef *host) { virCPUDefPtr oldguest = NULL; const struct x86_map *map; @@ -2124,7 +2123,7 @@ x86UpdateHostModel(virCPUDefPtr guest, } } } - for (i = 0; !passthrough && i < oldguest->nfeatures; i++) { + for (i = 0; i < oldguest->nfeatures; i++) { if (virCPUDefUpdateFeature(guest, oldguest->features[i].name, oldguest->features[i].policy) < 0) While this looks correct, save/restore (and likely migration too) with -cpu host has even more bugs (e.g., -cpu host turns into -cpu host,+... after save/restore) and I'd like to fix all that at once.
Yes, there are more bugs here, seems i didn't notice that at that time. Thanks a lot for your review
Jirka
Luyao
participants (3)
-
Jiri Denemark -
lhuang -
Luyao Huang