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

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

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

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

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 :|

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

  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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

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

  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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list