Re: [PATCH v2 1/4] xl: Add support for ignore_msrs option

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 18, 2021 at 12:54:13PM +0100, Jan Beulich wrote:
> On 18.02.2021 11:42, Roger Pau Monné wrote:
> > On Wed, Jan 20, 2021 at 05:49:09PM -0500, Boris Ostrovsky wrote:
> >> This option allows guest administrator specify what should happen when
> >> guest accesses an MSR which is not explicitly emulated by the hypervisor.
> >>
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> >> ---
> >>  docs/man/xl.cfg.5.pod.in         | 20 +++++++++++++++++++-
> >>  tools/libs/light/libxl_types.idl |  7 +++++++
> >>  tools/xl/xl_parse.c              |  7 +++++++
> >>  3 files changed, 33 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> >> index c8e017f950de..96ce97c42cab 100644
> >> --- a/docs/man/xl.cfg.5.pod.in
> >> +++ b/docs/man/xl.cfg.5.pod.in
> >> @@ -2044,7 +2044,25 @@ Do not provide a VM generation ID.
> >>  See also "Virtual Machine Generation ID" by Microsoft:
> >>  
> >> L<https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/virtual-machine-generation-identifier>
> >>  
> >> -=back 
> >> +=over
> >> +
> >> +=item B<ignore_msrs="STRING">
> >> +
> >> +Determine hypervisor behavior on accesses to MSRs that are not emulated 
> >> by the hypervisor.
> >> +
> >> +=over 4
> >> +
> >> +=item B<never>
> >> +
> >> +Issue a warning to the log and #GP to the guest. This is default.
> >> +
> >> +=item B<silent>
> >> +
> >> +MSR reads return 0, MSR writes are ignored. No warnings to the log.
> >> +
> >> +=item B<verbose>
> >> +
> >> +Similar to B<silent> but a warning is written.
> > 
> > Would it make sense to allow for this option to be more fine-grained
> > in the future?
> 
> From an abstract perspective - maybe. But remember that this information
> will need to be migrated with the guest. It would seem to me that
> Boris'es approach is easier migration-wise.

I'm not an expert on migration, but I seem to recall there's already a
libxl blob that gets migrated that contains the domain configuration,
so having the MSR configuration there seems like a sensible thing to
do.

> > Not that you need to implement the full thing now, but maybe we could
> > have something like:
> > 
> > "
> > =item B<ignore_msrs=[ "MSR_RANGE, "MSR_RANGE", ..]>
> > 
> > Specify a list of MSR ranges that will be ignored by the hypervisor:
> > reads will return zeros and writes will be discarded without raising a
> > #GP.
> > 
> > Each MSR_RANGE is given in hexadecimal format and may be a range, e.g.
> > c00102f0-c00102f1 (inclusive), or a single MSR, e.g. c00102f1.
> > "
> > 
> > Then you can print the messages in the hypervisor using a guest log
> > level and modify it on demand in order to get more verbose output?
> 
> "Modify on demand"? Irrespective of what you mean with this, ...
> 
> > I don't think selecting whether the messages are printed or not from
> > xl is that helpful as the same could be achieved using guest_loglvl.
> 
> ... controlling this via guest_loglvl would affect various other
> log messages' visibility.

Right, but do we really need this level of per-guest log control,
implemented in this way exclusively for MSRs?

We don't have a way for other parts of the code to have such
fine-grained control about what messages should be printed, and I
don't think MSR should be an exception. I assume this would be used to
detect which MSRs a guest is trying to access, and I would be fine
just using guest_loglvl to that end, the more that it can be modified
at run time now.

In any case I'm more worried about having a big switch to ignore all
unhandled MSRs rather than whether accesses should print a message or
not. Worse come to worse we could always add a new option afterwards
to selectively ignore MSR access, but that would be confusing for
users IMO.

Thanks, Roger.