| Message ID | 1611182952-9941-2-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Permit fault-less access to non-emulated MSRs | expand |
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@oracle.com> > --- > 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(-) It is mainly missing a #define LIBXL_HAVE_XXX in libxl.h. Other than that, this patch looks good to me. If you end up resending this series, please fix that issue. If other patches are all reviewed, you can provide some text to be merged into this patch. Wei.
On 1/21/21 9:56 AM, Wei Liu 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@oracle.com> >> --- >> 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(-) > > It is mainly missing a #define LIBXL_HAVE_XXX in libxl.h. > > Other than that, this patch looks good to me. If you end up resending > this series, please fix that issue. > > If other patches are all reviewed, you can provide some text to be > merged into this patch. > > Wei. > Ah yes, I forgot about this. diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 3433c950f9aa..f249740daf3f 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -1310,6 +1310,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src); */ #define LIBXL_HAVE_CREATEINFO_XEND_SUSPEND_EVTCHN_COMPAT +/* + * LIBXL_HAVE_IGNORE_MSRS indicates that the libxl_ignore_msrs field is + * present in libxl_domain_build_info. This field describes hypervisor + * behavior on guest accesses to unimplemented MSRs. + */ +#define LIBXL_HAVE_IGNORE_MSRS 1 + typedef char **libxl_string_list; void libxl_string_list_dispose(libxl_string_list *sl); int libxl_string_list_length(const libxl_string_list *sl);
Hi Boris, On 20/01/2021 22:49, 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@oracle.com> > --- > 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. The description of the feature looks very x86 focus. Yet, it seems to be defined as a generic one. Could you clarify whether this is intended to be re-usable by other architectures? > + > +=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. > > =head3 Guest Virtual Time Controls > > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index 05324736b744..7b5fef771ee8 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -477,6 +477,12 @@ libxl_tee_type = Enumeration("tee_type", [ > (1, "optee") > ], init_val = "LIBXL_TEE_TYPE_NONE") > > +libxl_ignore_msrs = Enumeration("ignore_msrs", [ > + (0, "never"), > + (1, "silent"), > + (2, "verbose"), > + ], init_val = "LIBXL_IGNORE_MSRS_NEVER") > + > libxl_rdm_reserve = Struct("rdm_reserve", [ > ("strategy", libxl_rdm_reserve_strategy), > ("policy", libxl_rdm_reserve_policy), > @@ -559,6 +565,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("apic", libxl_defbool), > ("dm_restrict", libxl_defbool), > ("tee", libxl_tee_type), > + ("ignore_msrs", libxl_ignore_msrs), > ("u", KeyedUnion(None, libxl_domain_type, "type", > [("hvm", Struct(None, [("firmware", string), > ("bios", libxl_bios_type), > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c > index 4ebf39620ae7..942086c3f41d 100644 > --- a/tools/xl/xl_parse.c > +++ b/tools/xl/xl_parse.c > @@ -2732,6 +2732,13 @@ skip_usbdev: > } > } > > + if (!xlu_cfg_get_string(config, "ignore_msrs", &buf, 0)) { > + if (libxl_ignore_msrs_from_string(buf, &b_info->ignore_msrs)) { > + fprintf(stderr, "ERROR: invalid value \"%s\" for \"ignore_msrs\"\n", buf); > + exit(1); > + } > + } > + > parse_vkb_list(config, d_config); > > xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat", > Cheers,
On 1/22/21 4:52 AM, Julien Grall wrote: > Hi Boris, > > On 20/01/2021 22:49, 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@oracle.com> >> --- >> 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. > > The description of the feature looks very x86 focus. Yet, it seems to be defined as a generic one. > > Could you clarify whether this is intended to be re-usable by other architectures? x86 only. I'll add appropriate note. -boris
On 22/01/2021 18:28, Boris Ostrovsky wrote: > > > On 1/22/21 4:52 AM, Julien Grall wrote: >> Hi Boris, >> >> On 20/01/2021 22:49, 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@oracle.com> >>> --- >>> 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. >> >> The description of the feature looks very x86 focus. Yet, it seems to be defined as a generic one. >> >> Could you clarify whether this is intended to be re-usable by other architectures? > > > x86 only. I'll add appropriate note. Thanks. In which case, I would suggest to move the definition of ignore_msrs in the idle to an x86 specific structure. Maybe "arch_x86"? I noticed that none exists today, but we could duplicate that "arch_arm" one. Wei, Ian, what do you think? Cheers,
On 1/22/21 1:33 PM, Julien Grall wrote: > > Thanks. In which case, I would suggest to move the definition of ignore_msrs in the idle to an x86 specific structure. Maybe "arch_x86"? I did consider this but left it in common area since there are already a bunch of fields there that are x86-specific. If there is desire to create an x86 sub-struct then we should move all of them there. -boris > > I noticed that none exists today, but we could duplicate that "arch_arm" one. > > Wei, Ian, what do you think? > > Cheers, >
On 22/01/2021 18:39, Boris Ostrovsky wrote: > > > On 1/22/21 1:33 PM, Julien Grall wrote: > >> >> Thanks. In which case, I would suggest to move the definition of ignore_msrs in the idle to an x86 specific structure. Maybe "arch_x86"? > > > I did consider this but left it in common area since there are already a bunch of fields there that are x86-specific. Most of them were introduced before Arm existed :). > > If there is desire to create an x86 sub-struct then we should move all of them there. The libxl interface is meant to be stable at the source level but not ABI stable. So we wouldn't be able to move the existing field in a new x86 without adding a compat layer (see for instance hvm.altp2m vs altp2m). I don't think this is worth it for them. That said, we should try to treat each architecture equally. So I think it is not a good idea to continue to pursue adding an field in common area if the field is only meant to be used for a single arch. If Ian or Wei thinks it is fine to define in common code, then so it be. Cheers,
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@oracle.com> > --- > 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? 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? 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. Also I think it will be fine to only implement: ignore_msrs=[ "0-ffffffff" ] Right now and return an error for any other combination, so that we can get something in soon and expand it later. Thanks, Roger.
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@oracle.com> >> --- >> 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. > 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. Jan
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@oracle.com> > >> --- > >> 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.
On 18.02.2021 16:52, Roger Pau Monné wrote: > On Thu, Feb 18, 2021 at 12:54:13PM +0100, Jan Beulich wrote: >> On 18.02.2021 11:42, Roger Pau Monné wrote: >>> 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. I can certainly see your point. The problem is that for guests heavily accessing such MSRs, all other messages may disappear due to rate limiting. That's not going to be helpful. Jan
On 2/18/21 10:57 AM, Jan Beulich wrote: > On 18.02.2021 16:52, Roger Pau Monné wrote: >> On Thu, Feb 18, 2021 at 12:54:13PM +0100, Jan Beulich wrote: >>> On 18.02.2021 11:42, Roger Pau Monné wrote: >>>> 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? In a multi-tenant environment we may need to figure out why a particular guest is failing to boot, without affecting behavior of other guests. If we had per-guest log level in general then what you are suggesting would be the right thing to do IMO. Maybe that's what we should add? -boris >> >> 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. > I can certainly see your point. The problem is that for guests > heavily accessing such MSRs, all other messages may disappear > due to rate limiting. That's not going to be helpful.
On Fri, Feb 19, 2021 at 09:50:12AM -0500, Boris Ostrovsky wrote: > > On 2/18/21 10:57 AM, Jan Beulich wrote: > > On 18.02.2021 16:52, Roger Pau Monné wrote: > >> On Thu, Feb 18, 2021 at 12:54:13PM +0100, Jan Beulich wrote: > >>> On 18.02.2021 11:42, Roger Pau Monné wrote: > >>>> 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? > > > In a multi-tenant environment we may need to figure out why a particular guest is failing to boot, without affecting behavior of other guests. > > > If we had per-guest log level in general then what you are suggesting would be the right thing to do IMO. Maybe that's what we should add? Yes, that would seem better IMO, but I don't think it's fair to ask you to do that work. Do you think it would be acceptable to untangle both, and try to get the MSR stuff without any logging changes? I know we would be addressing only one part of what the series originally tried to achieve, but I would rather prefer to have a generic way to set a per-guest log level rather than something specific to MSR accesses. Thanks, Roger.
On 22.02.2021 11:24, Roger Pau Monné wrote: > On Fri, Feb 19, 2021 at 09:50:12AM -0500, Boris Ostrovsky wrote: >> >> On 2/18/21 10:57 AM, Jan Beulich wrote: >>> On 18.02.2021 16:52, Roger Pau Monné wrote: >>>> On Thu, Feb 18, 2021 at 12:54:13PM +0100, Jan Beulich wrote: >>>>> On 18.02.2021 11:42, Roger Pau Monné wrote: >>>>>> 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? >> >> >> In a multi-tenant environment we may need to figure out why a particular guest is failing to boot, without affecting behavior of other guests. >> >> >> If we had per-guest log level in general then what you are suggesting would be the right thing to do IMO. Maybe that's what we should add? > > Yes, that would seem better IMO, but I don't think it's fair to ask > you to do that work. > > Do you think it would be acceptable to untangle both, and try to get > the MSR stuff without any logging changes? > > I know we would be addressing only one part of what the series > originally tried to achieve, but I would rather prefer to have a > generic way to set a per-guest log level rather than something > specific to MSR accesses. TBH I'd see us go the other route: Follow Boris'es approach for 4.15, and switch the logging control to per-guest once that ability is there, _and_ if we're really convinced we don't want to have this extra level of control. The latter because I think a domain could end up pretty chatty just because of MSR accesses, and it might therefore be undesirable to also hide all other potentially relevant output. Perhaps the per-domain log level control needs to be finer grained than what "guest_loglvl=" currently permits, more like what "hvm_debug=" has. Jan
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. =head3 Guest Virtual Time Controls diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index 05324736b744..7b5fef771ee8 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -477,6 +477,12 @@ libxl_tee_type = Enumeration("tee_type", [ (1, "optee") ], init_val = "LIBXL_TEE_TYPE_NONE") +libxl_ignore_msrs = Enumeration("ignore_msrs", [ + (0, "never"), + (1, "silent"), + (2, "verbose"), + ], init_val = "LIBXL_IGNORE_MSRS_NEVER") + libxl_rdm_reserve = Struct("rdm_reserve", [ ("strategy", libxl_rdm_reserve_strategy), ("policy", libxl_rdm_reserve_policy), @@ -559,6 +565,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("apic", libxl_defbool), ("dm_restrict", libxl_defbool), ("tee", libxl_tee_type), + ("ignore_msrs", libxl_ignore_msrs), ("u", KeyedUnion(None, libxl_domain_type, "type", [("hvm", Struct(None, [("firmware", string), ("bios", libxl_bios_type), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 4ebf39620ae7..942086c3f41d 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2732,6 +2732,13 @@ skip_usbdev: } } + if (!xlu_cfg_get_string(config, "ignore_msrs", &buf, 0)) { + if (libxl_ignore_msrs_from_string(buf, &b_info->ignore_msrs)) { + fprintf(stderr, "ERROR: invalid value \"%s\" for \"ignore_msrs\"\n", buf); + exit(1); + } + } + parse_vkb_list(config, d_config); xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat",
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@oracle.com> --- 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(-)