From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work Date: Mon, 8 Sep 2014 14:00:40 +0100 Message-ID: <540DA878.6050303@citrix.com> References: <1410177747-20623-1-git-send-email-alfred.z.song@gmail.com> <540D9BF9.90607@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4542423289896395575==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: z Cc: Ian Campbell , Stefano Stabellini , jinsong.liu@alibaba-inc.com, ian.jackson@eu.citrix.com, Zhuo Song , =?UTF-8?B?IumprOa2myjkvK/nkZwpIg==?= , Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============4542423289896395575== Content-Type: multipart/alternative; boundary="------------020604010304010005000906" --------------020604010304010005000906 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 08/09/14 13:17, z wrote: > > > On Mon, Sep 8, 2014 at 8:07 PM, Andrew Cooper > > wrote: > > On 08/09/14 13:02, Zhuo Song wrote: > > * Since there would not be 32-bit hypervisor, we do not need > > hypervisor_is_64bit() again. > > > > * Remove xen_64bit from xc_cpuid_pv_policy(). > > > > * Because is_64bit only depends on is_pae, only use is_pae for bo= th > > vendor specific functions. > > > > * Move conditions for LM/NX masking into architectural logic > > > > Signed-off-by: Zhuo Song > > > --- > > tools/libxc/xc_cpuid_x86.c | 37 > ++++++++++++++----------------------- > > 1 file changed, 14 insertions(+), 23 deletions(-) > > > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x8= 6.c > > index 6b81641..710fd61 100644 > > --- a/tools/libxc/xc_cpuid_x86.c > > +++ b/tools/libxc/xc_cpuid_x86.c > > @@ -34,13 +34,6 @@ > > #define DEF_MAX_INTELEXT 0x80000008u > > #define DEF_MAX_AMDEXT 0x8000001cu > > > > -static int hypervisor_is_64bit(xc_interface *xch) > > -{ > > - xen_capabilities_info_t xen_caps =3D ""; > > - return ((xc_version(xch, XENVER_capabilities, &xen_caps) =3D= =3D > 0) && > > - (strstr(xen_caps, "x86_64") !=3D NULL)); > > -} > > - > > static void cpuid(const unsigned int *input, unsigned int *regs)= > > { > > unsigned int count =3D (input[1] =3D=3D XEN_CPUID_INPUT_UNUS= ED) ? > 0 : input[1]; > > @@ -95,13 +88,11 @@ static void amd_xc_cpuid_policy( > > break; > > > > case 0x80000001: { > > - int is_64bit =3D hypervisor_is_64bit(xch) && is_pae; > > - > > if ( !is_pae ) > > clear_bit(X86_FEATURE_PAE, regs[3]); > > > > /* Filter all other features according to a whitelist. *= / > > - regs[2] &=3D ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM)= > : 0) | > > + regs[2] &=3D (bitmaskof(X86_FEATURE_LAHF_LM) | > > bitmaskof(X86_FEATURE_CMP_LEGACY) | > > (is_nestedhvm ? bitmaskof(X86_FEATURE_SVM) > : 0) | > > bitmaskof(X86_FEATURE_CR8_LEGACY) | > > @@ -116,8 +107,8 @@ static void amd_xc_cpuid_policy( > > bitmaskof(X86_FEATURE_TBM) | > > bitmaskof(X86_FEATURE_DBEXT)); > > regs[3] &=3D (0x0183f3ff | /* features shared with > 0x00000001:EDX */ > > - (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | > > - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |= > > + bitmaskof(X86_FEATURE_NX) | > > + bitmaskof(X86_FEATURE_LM) | > > You are changing the behaviour here, due to dropping is_pae. > > This will break VM migrate. > > ~Andrew > > > I move it to architectural logic as I said. See: > > In my opinion, for LM: > > > + clear_bit(X86_FEATURE_LAHF_LM, regs[2]); > > + clear_bit(X86_FEATURE_LM, regs[3]); > > for NX: > clear_bit(X86_FEATURE_NX, regs[3]);=20 > > should have done the work, so we do not need to do it again both in > amd_xc_cpuid_policy or intel_xc_cpuid_policy > > Zhuo Oh - its setting a bit for an AND mask where the maskee has already had the bit cleared. In which case, it looks like it isn't changing the behaviour. ~Andrew --------------020604010304010005000906 Content-Type: text/html; charset="UTF-8" Content-Length: 7797 Content-Transfer-Encoding: quoted-printable
On 08/09/14 13:17, z wrote:


On Mon, Sep 8, 2014 at 8:07 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
On 08/09/14 13:02, Zhuo Song wrote:
> * Since there would not be 32-bit hypervisor, we do not need
>=C2=A0 =C2=A0hypervisor_is_64bit() again.
>
> * Remove xen_64bit from xc_cpuid_pv_policy().
>
> * Because is_64bit only depends on is_pae, only use is_pae for both
>=C2=A0 =C2=A0vendor specific functions.
>
> * Move conditions for LM/NX masking into architectural logic
>
> Signed-off-by: Zhuo Song <songzhuo.sz@alibaba-inc.com>
> ---
>=C2=A0 tools/libxc/xc_cpuid_x86.c | 37 ++++++++++++++-----------------------
>=C2=A0 1 file changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
> index 6b81641..710fd61 100644
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -34,13 +34,6 @@
>=C2=A0 #define DEF_MAX_INTELEXT=C2=A0 0x80000008u
>=C2=A0 #define DEF_MAX_AMDEXT=C2=A0 =C2=A0 0x8000001cu
>
> -static int hypervisor_is_64bit(xc_interface *xch)
> -{
> -=C2=A0 =C2=A0 xen_capabilities_info_t xen_caps =3D "";
> -=C2=A0 =C2=A0 return ((xc_version(xch, XENVER_capabilities, &xen_caps) =3D=3D 0) &&
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (strstr(xen_caps, "x86_64") !=3D NULL));
> -}
> -
>=C2=A0 static void cpuid(const unsigned int *input, unsigned int *regs)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 unsigned int count =3D (input[1] =3D=3D XEN_CPUID_INPUT_UNUSED) =3F 0 : input[1];
> @@ -95,13 +88,11 @@ static void amd_xc_cpuid_policy(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>
>=C2=A0 =C2=A0 =C2=A0 case 0x80000001: {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 int is_64bit =3D hypervisor_is_64bit(xch) && is_pae;
> -
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( !is_pae )
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(X86_FEATURE_PAE, regs[3]);
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Filter all other features according to a whitelist. */
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 regs[2] &=3D ((is_64bit =3F bitmaskof(X86_FEATURE_LAHF_LM) : 0) |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 regs[2] &=3D (bitmaskof(X86_FEATURE_LAHF_LM) |
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bitmaskof(X86_FEATURE_CMP_LEGACY) |
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (is_nestedhvm =3F bitmaskof(X86_FEATURE_SVM) : 0) |
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bitmaskof(X86_FEATURE_CR8_LEGACY) |
> @@ -116,8 +107,8 @@ static void amd_xc_cpuid_policy(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bitmaskof(X86_FEATURE_TBM) |
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bitmaskof(X86_FEATURE_DBEXT));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 regs[3] &=3D (0x0183f3ff | /* features shared with 0x00000001:EDX */
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (is_pae =3F bitmaskof(X86_FEATURE_NX) : 0) |
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (is_64bit =3F bitmaskof(X86_FEATURE_LM) : 0) |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bitmaskof(X86_FEATURE_NX) |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bitmaskof(X86_FEATURE_LM) |

You are changing the behaviour here, due to dropping is_pae.

This will break VM migrate.

~Andrew

I move it to architectural logic as I said. See:

In my opinion, for LM:

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(X86_FEATURE_LAHF_LM, regs[2]);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(X86_FEATURE_LM, regs[3]);

for NX:
clear_bit(X86_FEATURE_NX, regs[3]);=C2=A0

should have done the work, so we do not need to do it again both in amd_xc_cpuid_policy or intel_xc_cpuid_policy

Zhuo

Oh - its setting a bit for an AND mask where the maskee has already had the bit cleared.

In which case, it looks like it isn't changing the behaviour.

~Andrew
--------------020604010304010005000906-- --===============4542423289896395575== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4542423289896395575==--