[libvirt] [PATCH 0/3] bhyve: implement MSRs ignore unknown writes feature
Roman Bogorodskiy (3): conf: introduce 'msrs' feature bhyve: implement MSRs ignore unknown writes feature news: document bhyve msrs feature docs/drvbhyve.html.in | 16 +++++++++ docs/formatdomain.html.in | 1 + docs/news.xml | 11 ++++++ docs/schemas/domaincommon.rng | 14 ++++++++ src/bhyve/bhyve_command.c | 4 +++ src/conf/domain_conf.c | 33 +++++++++++++++++ src/conf/domain_conf.h | 8 +++++ src/qemu/qemu_domain.c | 1 + .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 14 files changed, 165 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml -- 2.20.1
Introduce the 'msrs' feature element that controls Model Specific Registers related behaviour. At this moment it allows only single tunable attribute "ignoreUnknownWrites": <msrs ignoreUnknownWrites='yes'/> Which tells hypervisor to ignore accesses to unimplemented Model Specific Registers. The only user of that for now is going to be the bhyve driver. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- docs/formatdomain.html.in | 1 + docs/schemas/domaincommon.rng | 14 ++++++++++++++ src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 ++++++++ src/qemu/qemu_domain.c | 1 + 5 files changed, 57 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7f07bb7f55..b6496c63db 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2010,6 +2010,7 @@ <tlbflush state='on'/> <ipi state='on'/> <evmcs state='on'/> + <msrs ignoreUnknownWrites='yes'/> </hyperv> <kvm> <hidden state='on'/> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index aa50eac424..e9dd99ab3a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4994,6 +4994,9 @@ <ref name="featurestate"/> </element> </optional> + <optional> + <ref name="msrs"/> + </optional> </interleave> </element> </optional> @@ -5242,6 +5245,17 @@ </element> </define> + <define name="msrs"> + <element name="msrs"> + <attribute name="ignoreUnknownWrites"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </element> + </define> + <define name="address"> <element name="address"> <choice> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 22979e6c4e..c8ec3a9011 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -153,6 +153,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, "vmcoreinfo", "htm", "nested-hv", + "msrs", ); VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, @@ -20232,6 +20233,7 @@ virDomainDefParseXML(xmlDocPtr xml, case VIR_DOMAIN_FEATURE_PRIVNET: case VIR_DOMAIN_FEATURE_HYPERV: case VIR_DOMAIN_FEATURE_KVM: + case VIR_DOMAIN_FEATURE_MSRS: def->features[val] = VIR_TRISTATE_SWITCH_ON; break; @@ -20520,6 +20522,26 @@ virDomainDefParseXML(xmlDocPtr xml, def->tseg_specified = rv; } + if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) { + if ((node = virXPathNode("./features/msrs", ctxt)) == NULL) + goto error; + + if (!(tmp = virXMLPropString(node, "ignoreUnknownWrites"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("missing ignoreUnknownWrites attribute for feature '%s'"), + virDomainFeatureTypeToString(VIR_DOMAIN_FEATURE_MSRS)); + goto error; + } + + if ((def->msrs_features[VIR_DOMAIN_MSRS_IGNORE_UNKNOWN_WRITES] = virTristateBoolTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown ignoreUnknownWrites value '%s'"), + tmp); + goto error; + } + VIR_FREE(tmp); + } + if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0) goto error; @@ -22582,6 +22604,9 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } break; + case VIR_DOMAIN_FEATURE_MSRS: + break; + case VIR_DOMAIN_FEATURE_LAST: break; } @@ -28647,6 +28672,14 @@ virDomainDefFormatInternal(virDomainDefPtr def, } break; + case VIR_DOMAIN_FEATURE_MSRS: + if (def->features[i] != VIR_TRISTATE_SWITCH_ON) + break; + + virBufferAsprintf(buf, "<msrs ignoreUnknownWrites='%s'/>\n", + virTristateBoolTypeToString(def->msrs_features[VIR_DOMAIN_MSRS_IGNORE_UNKNOWN_WRITES])); + break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f1e6e4e8b7..36926d5e1d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1782,6 +1782,7 @@ typedef enum { VIR_DOMAIN_FEATURE_VMCOREINFO, VIR_DOMAIN_FEATURE_HTM, VIR_DOMAIN_FEATURE_NESTED_HV, + VIR_DOMAIN_FEATURE_MSRS, VIR_DOMAIN_FEATURE_LAST } virDomainFeature; @@ -1813,6 +1814,12 @@ typedef enum { VIR_DOMAIN_KVM_LAST } virDomainKVM; +typedef enum { + VIR_DOMAIN_MSRS_IGNORE_UNKNOWN_WRITES = 0, + + VIR_DOMAIN_MSRS_LAST +} virDomainMsrs; + typedef enum { VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT = 0, VIR_DOMAIN_CAPABILITIES_POLICY_ALLOW, @@ -2466,6 +2473,7 @@ struct _virDomainDef { int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST]; int hyperv_features[VIR_DOMAIN_HYPERV_LAST]; int kvm_features[VIR_DOMAIN_KVM_LAST]; + int msrs_features[VIR_DOMAIN_MSRS_LAST]; unsigned int hyperv_spinlocks; virGICVersion gic_version; virDomainHPTResizing hpt_resizing; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f42903a343..f484814977 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4030,6 +4030,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_VMCOREINFO: + case VIR_DOMAIN_FEATURE_MSRS: case VIR_DOMAIN_FEATURE_LAST: break; } -- 2.20.1
Implement the MSRs ignore unkown writes feature that's specified using: <features> ... <msrs ignoreUnknownWrites='yes'> ... </features> in the domain XML. In bhyve, it's just passing '-w' command line argument to the bhyve(8) executable. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- docs/drvbhyve.html.in | 16 +++++++++ src/bhyve/bhyve_command.c | 4 +++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 8 files changed, 97 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in index b4d7df2edb..5d67bf358e 100644 --- a/docs/drvbhyve.html.in +++ b/docs/drvbhyve.html.in @@ -462,5 +462,21 @@ Example:</p> </domain> </pre> +<h3><a id="msrsignorewrites">Ignoring unknown MSRs writes</a></h3> + +<p>Some guests might require ignoring unknown Model Specific Registers (MSRs) writes. +<span class="since">Since 5.1.0</span> it's possible to switch this on using:</p> +<pre> +<domain type="bhyve"> + ... + <features> + <msrs ignoreUnknownWrites='yes'/> + </features> + ... +</domain> +</pre> + +<p>By default unknown writes are not ignored.</p> + </body> </html> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 84fda08943..ae2d989a95 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -505,6 +505,10 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn, virCommandAddArg(cmd, "-A"); /* Create an ACPI table */ if (def->features[VIR_DOMAIN_FEATURE_APIC] == VIR_TRISTATE_SWITCH_ON) virCommandAddArg(cmd, "-I"); /* Present ioapic to the guest */ + if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) { + if (def->msrs_features[VIR_DOMAIN_MSRS_IGNORE_UNKNOWN_WRITES] == VIR_TRISTATE_SWITCH_ON) + virCommandAddArg(cmd, "-w"); + } switch (def->clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args b/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args new file mode 100644 index 0000000000..dbe377421b --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args @@ -0,0 +1,10 @@ +/usr/sbin/bhyve \ +-c 1 \ +-m 214 \ +-w \ +-u \ +-H \ +-P \ +-s 0:0,hostbridge \ +-s 2:0,ahci,hd:/tmp/freebsd.img \ +-s 3:0,virtio-net,faketapdev,mac=52:54:00:b9:94:02 bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs new file mode 100644 index 0000000000..32538b558e --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs @@ -0,0 +1,3 @@ +/usr/sbin/bhyveload \ +-m 214 \ +-d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml new file mode 100644 index 0000000000..7543eb682c --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml @@ -0,0 +1,26 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <os> + <type>hvm</type> + </os> + <features> + <msrs ignoreUnknownWrites='yes'/> + </features> + <devices> + <disk type='file'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='2' unit='0'/> + </disk> + <interface type='bridge'> + <mac address='52:54:00:b9:94:02'/> + <model type='virtio'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index d1b486fa64..5ea5b26466 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -210,6 +210,7 @@ mymain(void) DO_TEST("vnc-autoport"); DO_TEST("cputopology"); DO_TEST_FAILURE("cputopology-nvcpu-mismatch"); + DO_TEST("msrs"); /* Address allocation tests */ DO_TEST("addr-single-sata-disk"); diff --git a/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml new file mode 100644 index 0000000000..dcf53703bf --- /dev/null +++ b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml @@ -0,0 +1,36 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <msrs ignoreUnknownWrites='yes'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='2' unit='0'/> + </disk> + <controller type='pci' index='0' model='pci-root'/> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </controller> + <interface type='bridge'> + <mac address='52:54:00:b9:94:02'/> + <source bridge='virbr0'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c index 6aaeab741e..916bf38c66 100644 --- a/tests/bhyvexml2xmltest.c +++ b/tests/bhyvexml2xmltest.c @@ -110,6 +110,7 @@ mymain(void) DO_TEST_DIFFERENT("vnc-vgaconf-off"); DO_TEST_DIFFERENT("vnc-vgaconf-io"); DO_TEST_DIFFERENT("vnc-autoport"); + DO_TEST_DIFFERENT("msrs"); /* Address allocation tests */ DO_TEST_DIFFERENT("addr-single-sata-disk"); -- 2.20.1
Describe bhyve's ignoring unknown MSRs writes feature introduced by commit 525918ac5c. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index bc11791d03..2acb51a082 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -63,6 +63,17 @@ capabilities whether KVM nesting support is available. </description> </change> + <change> + <summary> + bhyve: support for ignoring unknown MSRs writes + </summary> + <description> + A new features element <msrs ignoreUnknownWrites='yes'/> was + introduced and the bhyve driver supports it to control unknown + Model Specific Registers (MSRs) writes. + </description> + </change> + </section> <section title="Removed features"> <change> -- 2.20.1
On 1/25/19 12:54 PM, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): conf: introduce 'msrs' feature bhyve: implement MSRs ignore unknown writes feature news: document bhyve msrs feature
docs/drvbhyve.html.in | 16 +++++++++ docs/formatdomain.html.in | 1 + docs/news.xml | 11 ++++++ docs/schemas/domaincommon.rng | 14 ++++++++ src/bhyve/bhyve_command.c | 4 +++ src/conf/domain_conf.c | 33 +++++++++++++++++ src/conf/domain_conf.h | 8 +++++ src/qemu/qemu_domain.c | 1 + .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 14 files changed, 165 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml
The code looks fine to me but this needs a bit more context. Particularly 'ignore' is a bit ambiguous here so I had to dig kvm+linux has arguably always ignored unknown MSRs but historically it would print an error in host dmesg which often confused users quite a bit. In the bhyve case it seems that unknown MSRs cause the VM to essentially crash which is a pretty intense reaction. This option disables that crashing behavior. So for this feature to be really descriptive it would be <msrs crashOnUnknown='yes|no' /> or something like that The bhyve man page says: https://www.freebsd.org/cgi/man.cgi?query=bhyve&sektion=8 -w Ignore accesses to unimplemented Model Specific Registers (MSRs). This is intended for debug purposes. Calling it 'intended for debug purposes' also makes me question whether this should be encoded in the libvirt XML or just worked around with commandline passthrough (IMO To be friendly to users this option should be enabled by default but obviously that's against the intention of bhyve devs...) ccing danpb to see if he has any thoughs about exposing this in the XML - Cole
On Wed, Feb 06, 2019 at 04:00:06PM -0500, Cole Robinson wrote:
On 1/25/19 12:54 PM, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): conf: introduce 'msrs' feature bhyve: implement MSRs ignore unknown writes feature news: document bhyve msrs feature
docs/drvbhyve.html.in | 16 +++++++++ docs/formatdomain.html.in | 1 + docs/news.xml | 11 ++++++ docs/schemas/domaincommon.rng | 14 ++++++++ src/bhyve/bhyve_command.c | 4 +++ src/conf/domain_conf.c | 33 +++++++++++++++++ src/conf/domain_conf.h | 8 +++++ src/qemu/qemu_domain.c | 1 + .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 14 files changed, 165 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml
The code looks fine to me but this needs a bit more context. Particularly 'ignore' is a bit ambiguous here so I had to dig
kvm+linux has arguably always ignored unknown MSRs but historically it would print an error in host dmesg which often confused users quite a bit. In the bhyve case it seems that unknown MSRs cause the VM to essentially crash which is a pretty intense reaction. This option disables that crashing behavior. So for this feature to be really descriptive it would be <msrs crashOnUnknown='yes|no' /> or something like that
That is not actually the case for KVM. When KVM sees an unhandled MSR it will inject a GPF to the guest, which will usually cause the guest to crash. KVM does this GPF injection for both reads and writes of unknown MSRs. There is an opt-in kvm.ko option "ignore-msrs" which tells KVM to silently ignore unknown MSRs instead. Ideally the KVM option would be exposed to QEMU as an option, since when people do need it, they usually only need it for a single guest.
The bhyve man page says:
https://www.freebsd.org/cgi/man.cgi?query=bhyve&sektion=8
-w Ignore accesses to unimplemented Model Specific Registers (MSRs). This is intended for debug purposes.
I'm curious whether this applies to boths reads & writes of unknown MSRs, or just to writes. '-w' naming suggests the latter, but the descrption "accesses" would suggest reads too.
Calling it 'intended for debug purposes' also makes me question whether this should be encoded in the libvirt XML or just worked around with commandline passthrough
(IMO To be friendly to users this option should be enabled by default but obviously that's against the intention of bhyve devs...)
Enabling it by default is not a good idea as it can lead to silently incorrect guest OS behaviour. That could lead to all manner of bad things depending on the MSR in question.
ccing danpb to see if he has any thoughs about exposing this in the XML
My only thought is the naming & whether we have one attribute for ignoring reads & writes, or separate attributes. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé wrote:
On Wed, Feb 06, 2019 at 04:00:06PM -0500, Cole Robinson wrote:
On 1/25/19 12:54 PM, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): conf: introduce 'msrs' feature bhyve: implement MSRs ignore unknown writes feature news: document bhyve msrs feature
docs/drvbhyve.html.in | 16 +++++++++ docs/formatdomain.html.in | 1 + docs/news.xml | 11 ++++++ docs/schemas/domaincommon.rng | 14 ++++++++ src/bhyve/bhyve_command.c | 4 +++ src/conf/domain_conf.c | 33 +++++++++++++++++ src/conf/domain_conf.h | 8 +++++ src/qemu/qemu_domain.c | 1 + .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 14 files changed, 165 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml
The code looks fine to me but this needs a bit more context. Particularly 'ignore' is a bit ambiguous here so I had to dig
kvm+linux has arguably always ignored unknown MSRs but historically it would print an error in host dmesg which often confused users quite a bit. In the bhyve case it seems that unknown MSRs cause the VM to essentially crash which is a pretty intense reaction. This option disables that crashing behavior. So for this feature to be really descriptive it would be <msrs crashOnUnknown='yes|no' /> or something like that
That is not actually the case for KVM. When KVM sees an unhandled MSR it will inject a GPF to the guest, which will usually cause the guest to crash. KVM does this GPF injection for both reads and writes of unknown MSRs.
There is an opt-in kvm.ko option "ignore-msrs" which tells KVM to silently ignore unknown MSRs instead. Ideally the KVM option would be exposed to QEMU as an option, since when people do need it, they usually only need it for a single guest.
The bhyve man page says:
https://www.freebsd.org/cgi/man.cgi?query=bhyve&sektion=8
-w Ignore accesses to unimplemented Model Specific Registers (MSRs). This is intended for debug purposes.
I'm curious whether this applies to boths reads & writes of unknown MSRs, or just to writes. '-w' naming suggests the latter, but the descrption "accesses" would suggest reads too.
From what I can see by briefly looking at the code, it applies to both reads and writes: https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/bhyverun.c?revision=343642&view=markup#l520 Check two functions below this line.
Calling it 'intended for debug purposes' also makes me question whether this should be encoded in the libvirt XML or just worked around with commandline passthrough
(IMO To be friendly to users this option should be enabled by default but obviously that's against the intention of bhyve devs...)
Enabling it by default is not a good idea as it can lead to silently incorrect guest OS behaviour. That could lead to all manner of bad things depending on the MSR in question.
ccing danpb to see if he has any thoughs about exposing this in the XML
My only thought is the naming & whether we have one attribute for ignoring reads & writes, or separate attributes.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Roman Bogorodskiy
On Thu, Feb 07, 2019 at 06:05:25PM +0400, Roman Bogorodskiy wrote:
Daniel P. Berrangé wrote:
On Wed, Feb 06, 2019 at 04:00:06PM -0500, Cole Robinson wrote:
On 1/25/19 12:54 PM, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): conf: introduce 'msrs' feature bhyve: implement MSRs ignore unknown writes feature news: document bhyve msrs feature
docs/drvbhyve.html.in | 16 +++++++++ docs/formatdomain.html.in | 1 + docs/news.xml | 11 ++++++ docs/schemas/domaincommon.rng | 14 ++++++++ src/bhyve/bhyve_command.c | 4 +++ src/conf/domain_conf.c | 33 +++++++++++++++++ src/conf/domain_conf.h | 8 +++++ src/qemu/qemu_domain.c | 1 + .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 14 files changed, 165 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml
The code looks fine to me but this needs a bit more context. Particularly 'ignore' is a bit ambiguous here so I had to dig
kvm+linux has arguably always ignored unknown MSRs but historically it would print an error in host dmesg which often confused users quite a bit. In the bhyve case it seems that unknown MSRs cause the VM to essentially crash which is a pretty intense reaction. This option disables that crashing behavior. So for this feature to be really descriptive it would be <msrs crashOnUnknown='yes|no' /> or something like that
That is not actually the case for KVM. When KVM sees an unhandled MSR it will inject a GPF to the guest, which will usually cause the guest to crash. KVM does this GPF injection for both reads and writes of unknown MSRs.
There is an opt-in kvm.ko option "ignore-msrs" which tells KVM to silently ignore unknown MSRs instead. Ideally the KVM option would be exposed to QEMU as an option, since when people do need it, they usually only need it for a single guest.
The bhyve man page says:
https://www.freebsd.org/cgi/man.cgi?query=bhyve&sektion=8
-w Ignore accesses to unimplemented Model Specific Registers (MSRs). This is intended for debug purposes.
I'm curious whether this applies to boths reads & writes of unknown MSRs, or just to writes. '-w' naming suggests the latter, but the descrption "accesses" would suggest reads too.
From what I can see by briefly looking at the code, it applies to both reads and writes:
https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/bhyverun.c?revision=343642&view=markup#l520
Check two functions below this line.
Yes, I agree. This is good as its the same behaviour as KVM. How about we change the XML to <msrs unknown="ignore|fault"/> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé wrote:
On Thu, Feb 07, 2019 at 06:05:25PM +0400, Roman Bogorodskiy wrote:
Daniel P. Berrangé wrote:
On Wed, Feb 06, 2019 at 04:00:06PM -0500, Cole Robinson wrote:
On 1/25/19 12:54 PM, Roman Bogorodskiy wrote:
Roman Bogorodskiy (3): conf: introduce 'msrs' feature bhyve: implement MSRs ignore unknown writes feature news: document bhyve msrs feature
docs/drvbhyve.html.in | 16 +++++++++ docs/formatdomain.html.in | 1 + docs/news.xml | 11 ++++++ docs/schemas/domaincommon.rng | 14 ++++++++ src/bhyve/bhyve_command.c | 4 +++ src/conf/domain_conf.c | 33 +++++++++++++++++ src/conf/domain_conf.h | 8 +++++ src/qemu/qemu_domain.c | 1 + .../bhyvexml2argvdata/bhyvexml2argv-msrs.args | 10 ++++++ .../bhyvexml2argv-msrs.ldargs | 3 ++ .../bhyvexml2argvdata/bhyvexml2argv-msrs.xml | 26 ++++++++++++++ tests/bhyvexml2argvtest.c | 1 + .../bhyvexml2xmlout-msrs.xml | 36 +++++++++++++++++++ tests/bhyvexml2xmltest.c | 1 + 14 files changed, 165 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-msrs.xml create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-msrs.xml
The code looks fine to me but this needs a bit more context. Particularly 'ignore' is a bit ambiguous here so I had to dig
kvm+linux has arguably always ignored unknown MSRs but historically it would print an error in host dmesg which often confused users quite a bit. In the bhyve case it seems that unknown MSRs cause the VM to essentially crash which is a pretty intense reaction. This option disables that crashing behavior. So for this feature to be really descriptive it would be <msrs crashOnUnknown='yes|no' /> or something like that
That is not actually the case for KVM. When KVM sees an unhandled MSR it will inject a GPF to the guest, which will usually cause the guest to crash. KVM does this GPF injection for both reads and writes of unknown MSRs.
There is an opt-in kvm.ko option "ignore-msrs" which tells KVM to silently ignore unknown MSRs instead. Ideally the KVM option would be exposed to QEMU as an option, since when people do need it, they usually only need it for a single guest.
The bhyve man page says:
https://www.freebsd.org/cgi/man.cgi?query=bhyve&sektion=8
-w Ignore accesses to unimplemented Model Specific Registers (MSRs). This is intended for debug purposes.
I'm curious whether this applies to boths reads & writes of unknown MSRs, or just to writes. '-w' naming suggests the latter, but the descrption "accesses" would suggest reads too.
From what I can see by briefly looking at the code, it applies to both reads and writes:
https://svnweb.freebsd.org/base/head/usr.sbin/bhyve/bhyverun.c?revision=343642&view=markup#l520
Check two functions below this line.
Yes, I agree. This is good as its the same behaviour as KVM.
How about we change the XML to
<msrs unknown="ignore|fault"/>
Sounds good to me, I'll send v2 soon.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Roman Bogorodskiy
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
Roman Bogorodskiy