Mailing List Archive: [PATCH v2 1/4] xl: Add support for ignore

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

Jan 20, 2021, 2:49 PM

Post #1 of 15 (189 views)

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.

=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",
--
1.8.3.1

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

wl at xen

Jan 21, 2021, 6:56 AM

Post #2 of 15 (189 views)

Permalink

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

boris.ostrovsky at oracle

Jan 21, 2021, 2:43 PM

Post #3 of 15 (189 views)

Permalink

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);

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

julien at xen

Jan 22, 2021, 1:52 AM

Post #4 of 15 (189 views)

Permalink

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,

--
Julien Grall

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

boris.ostrovsky at oracle

Jan 22, 2021, 10:28 AM

Post #5 of 15 (189 views)

Permalink

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

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

julien at xen

Jan 22, 2021, 10:33 AM

Post #6 of 15 (189 views)

Permalink

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,

--
Julien Grall

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

boris.ostrovsky at oracle

Jan 22, 2021, 10:39 AM

Post #7 of 15 (189 views)

Permalink

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

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

julien at xen

Jan 22, 2021, 12:42 PM

Post #8 of 15 (189 views)

Permalink

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,

--
Julien Grall

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

roger.pau at citrix

Feb 18, 2021, 2:42 AM

Post #9 of 15 (184 views)

Permalink

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.

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

jbeulich at suse

Feb 18, 2021, 3:54 AM

Post #10 of 15 (184 views)

Permalink

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

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

roger.pau at citrix

Feb 18, 2021, 7:52 AM

Post #11 of 15 (184 views)

Permalink

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.

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

jbeulich at suse

Feb 18, 2021, 7:57 AM

Post #12 of 15 (184 views)

Permalink

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

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

boris.ostrovsky at oracle

Feb 19, 2021, 6:50 AM

Post #13 of 15 (181 views)

Permalink

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.

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

roger.pau at citrix

Feb 22, 2021, 2:24 AM

Post #14 of 15 (179 views)

Permalink

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.

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

jbeulich at suse

Feb 22, 2021, 2:33 AM

Post #15 of 15 (179 views)

Permalink

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