Re: [PATCH 5/5] x86: Rework MSR_TSC_AUX handling from scratch.

From: Wei Liu <wei.liu2@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Kevin Tian" <kevin.tian@intel.com>,
	"Wei Liu" <wei.liu2@citrix.com>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	"Jan Beulich" <JBeulich@suse.com>,
	"Suravee Suthikulpanit" <suravee.suthikulpanit@amd.com>,
	"Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH 5/5] x86: Rework MSR_TSC_AUX handling from scratch.
Date: Tue, 20 Feb 2018 17:03:44 +0000	[thread overview]
Message-ID: <20180220170344.s5lyrsi5t6yotllv@citrix.com> (raw)
In-Reply-To: <1519127923-23539-6-git-send-email-andrew.cooper3@citrix.com>

On Tue, Feb 20, 2018 at 11:58:43AM +0000, Andrew Cooper wrote:
> There are many problems with MSR_TSC_AUX handling.
> 
> To being with, the RDPID instruction reads MSR_TSC_AUX, but it is only the
     ^
     begin

> RDTSCP feature which enumerates the MSR.  Therefore, RDPID functionally
> depends on RDTSCP.
> 
> For PV guests, we hide RDTSCP but advertise RDPID.  We also silently drop
> writes to MSR_TSC_AUX, which is very antisocial.  Therefore, enable RDTSCP for
> PV guests, which in turn allows RDPID to work.
> 
> To support RDTSCP properly for PV guests, the MSR_TSC_AUX handling is moved
> into the generic MSR policy infrastructure, and becomes common.  One
> improvement is that we will now reject invalid values, rather than silently
> truncating an accepting them.  This also causes the emulator to reject RDTSCP
             ^
             and

> for guests without the features.
> 
>  
[...]
> @@ -1284,7 +1285,18 @@ long arch_do_domctl(
>                  for ( j = 0; j < ARRAY_SIZE(msrs_to_send); ++j )
>                  {
>                      uint64_t val;
> -                    int rc = guest_rdmsr(v, msrs_to_send[j], &val);
> +                    int rc;
> +
> +                    /*
> +                     * Skip MSR_TSC_AUX if using TSC_MODE_PVRDTSCP.  In this
> +                     * case, the MSR is read-only, and should be rejected if
> +                     * seen on the restore side.
> +                     */
> +                    if ( msrs_to_send[j] == MSR_TSC_AUX &&
> +                         d->arch.tsc_mode == TSC_MODE_PVRDTSCP )
> +                        continue;

Shouldn't we increment incarnation and send it over to the remote end?
Or send the original value and let the remote increments it?

Frankly I'm not sure how guest is supposed to use that value, but
letting the receiving end restart at 0, which can end up using the same
incarnation as before seems conceptually wrong to me. The more so you
mention a lot of migration's in your previous patches.

This is not disagreeing with the point that IA32_TSC_AUX should be
read-only for PV guest.

And +1 for the unification of HVM and PV code for handling this.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel