Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size

On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote: > On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote: > > On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote: > > > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote: > > > > > > > > This is way too sparse. > > > > > > > > On 05/21/2018 11:00 AM, Martin Kletzander wrote: > > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338 > > > > > > > > > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > > > > > --- > > > > > src/qemu/qemu_command.c | 18 ++++ > > > > > src/qemu/qemu_domain.c | 84 +++++++++++++++++++ > > > > > .../qemuxml2argvdata/tseg-explicit-size.args | 28 +++++++ > > > > > tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++ > > > > > tests/qemuxml2argvdata/tseg-i440fx.xml | 23 +++++ > > > > > tests/qemuxml2argvdata/tseg-invalid-size.xml | 23 +++++ > > > > > .../tseg-old-machine-type.args | 27 ++++++ > > > > > .../tseg-old-machine-type.xml | 21 +++++ > > > > > tests/qemuxml2argvdata/tseg.args | 28 +++++++ > > > > > tests/qemuxml2argvdata/tseg.xml | 21 +++++ > > > > > tests/qemuxml2argvtest.c | 48 +++++++++++ > > > > > .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++ > > > > > .../tseg-old-machine-type.xml | 44 ++++++++++ > > > > > tests/qemuxml2xmloutdata/tseg.xml | 46 ++++++++++ > > > > > tests/qemuxml2xmltest.c | 25 ++++++ > > > > > 15 files changed, 505 insertions(+) > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args > > > > > create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.args > > > > > create mode 100644 tests/qemuxml2argvdata/tseg.xml > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml > > > > > create mode 100644 tests/qemuxml2xmloutdata/tseg.xml > > > > > > [...] > > > > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > > > > index 881d0ea46a75..3ea9e3d47344 100644 > > > > > --- a/src/qemu/qemu_domain.c > > > > > +++ b/src/qemu/qemu_domain.c > > > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) > > > > > } > > > > > > > > > > > > > > > +static int > > > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def, > > > > > + virQEMUCapsPtr qemuCaps) > > > > > +{ > > > > > + const char *machine = NULL; > > > > > + char *end_ptr = NULL; > > > > > + unsigned int major = 0; > > > > > + unsigned int minor = 0; > > > > > + > > > > > + def->tseg_size = 0; > > > > > > > > Pointless since the only way in here is "if (tseg_size == 0)" > > > > > > > > > + > > > > > + if (!qemuDomainIsQ35(def)) > > > > > + return 0; > > > > > + > > > > > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) > > > > > > > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC > > > > though since it does follow the QEMU name. > > > > > > > > > + return 0; > > > > > + > > > > > + machine = STRSKIP(def->os.machine, "pc-q35-"); > > > > > + > > > > > + if (!machine) > > > > > + goto error; > > > > > + > > > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0) > > > > > + goto error; > > > > > + > > > > > + if (*end_ptr != '.') > > > > > + goto error; > > > > > + > > > > > + machine = end_ptr + 1; > > > > > + > > > > > + if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0) > > > > > + goto error; > > > > > + if (*end_ptr != '\0') > > > > > + goto error; > > > > > + > > > > > + /* QEMU started defaulting to 16MiB after 2.9 */ > > > > > + if (major > 2 || (major == 2 && minor > 9)) > > > > > + def->tseg_size = 16 * 1024 * 1024; > > > > > > > > So, if QEMU defaults to 16MiB, then why do we need so set this at all? > > > > > > > > This seems to me we are setting policy which based on history of many > > > > patches before is a no go. I'd say NAK to this, unless there is some > > > > convincing argument made in the commit message and followup responses to > > > > the series (or you can take Jan's R-By and ignore me - your call. > > > > > > I agree with John, this whole function should be removed, we don't have > > > to set the default in config XML. However we should record that default > > > value into live/status XML to make sure that it will not be changed > > > during migration and to let the user know what is the default value. > > > > > > > That doesn't make sense. This is part of guest ABI and if you want to be on the > > safe side when migrating, then you should be way more cautious with the > > stop/start scenario. Migration will probably fail (or silently corrupt data, > > but I believe that wouldn't happen), but stop/start without keeping the default > > will change that in case QEMU changes default and then the guest ABI will > > change. See my other reply to this particular concern. > > I'm not sure that's true. You can consider CPU as guest ABI but using > host-model the actual CPU is described only in live/status XML but the > config XML always has mode set to host-model. > Well, but you used the wrong example. The documentation for host-model properly says: "...but shutting down and restarting the guest may present different hardware to the guest according to the capabilities of the new host." host-model is one of the special cases where we cannot provide stable guest ABI and we explicitly say that. > Another thing is that if user doesn't care about this value they would > lose the benefit of using the default hypervisor value and once it get's > updated in the hypervisor they would be stuck with the previous default. > Yes. But that's actually precisely the reason we are doing this. > We might need to somehow specify what guest ABI means and what it should > cover and what can be left to hypervisor. > Well, we don't, guest ABI is not a term we would thought of. Whatever the guest OS sees as part of the hardware, whatever it can gather about the virtual machine is part of the guest ABI.