* [PATCH] xc_cpuid_x86.c: No need to mask NX twice @ 2014-09-05 10:11 Zhuo Song 2014-09-05 10:32 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Zhuo Song @ 2014-09-05 10:11 UTC (permalink / raw) To: xen-devel Cc: ian.campbell, stefano.stabellini, jinsong.liu, ian.jackson, Zhuo Song, Zhuo Song, boyu.mt Since amd_xc_cpuid_policy or intel_xc_cpuid_policy will do it later as well, we may drop an extra action. Signed-off-by: Zhuo Song <songzhuo.sz@alibaba-inc.com> --- tools/libxc/xc_cpuid_x86.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 6b81641..61af3e6 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -392,7 +392,6 @@ static void xc_cpuid_hvm_policy( case 0x80000001: if ( !is_pae ) { - clear_bit(X86_FEATURE_NX, regs[3]); clear_bit(X86_FEATURE_PSE36, regs[3]); } break; -- 1.8.5.2 (Apple Git-48) ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 10:11 [PATCH] xc_cpuid_x86.c: No need to mask NX twice Zhuo Song @ 2014-09-05 10:32 ` Jan Beulich 2014-09-05 13:54 ` z 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-05 10:32 UTC (permalink / raw) To: Zhuo Song Cc: ian.campbell, stefano.stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 05.09.14 at 12:11, <alfred.z.song@gmail.com> wrote: > Since amd_xc_cpuid_policy or intel_xc_cpuid_policy will do it later as > well, we may drop an extra action. But then I think it would be better to keep the generic one and drop the vendor specific ones. Jan > Signed-off-by: Zhuo Song <songzhuo.sz@alibaba-inc.com> > --- > tools/libxc/xc_cpuid_x86.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > index 6b81641..61af3e6 100644 > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -392,7 +392,6 @@ static void xc_cpuid_hvm_policy( > > case 0x80000001: > if ( !is_pae ) { > - clear_bit(X86_FEATURE_NX, regs[3]); > clear_bit(X86_FEATURE_PSE36, regs[3]); > } > break; > -- > 1.8.5.2 (Apple Git-48) > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 10:32 ` Jan Beulich @ 2014-09-05 13:54 ` z 2014-09-05 14:10 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: z @ 2014-09-05 13:54 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1891 bytes --] Hi, Jan Generally, I agree with you. But it seems that we may not simply remove the ones from the 2 vendor specific functions and keep the generic one, because the bit could be still cleared eventually when is_pae is true. And I find PSE36 will be cleared permanently no matter is_pae is true or not. Actually the work below has been totally overlapped: if ( !is_pae ) { clear_bit(X86_FEATURE_NX, regs[3]); clear_bit(X86_FEATURE_PSE36, regs[3]); } How can we just "passthrough to cpu vendor specific functions" like leaf 0x80000000? like this: case 0x80000000: case 0x80000001: /* Passthrough to cpu vendor specific functions */ break; Correct me if I missed something. Zhuo On Fri, Sep 5, 2014 at 6:32 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 05.09.14 at 12:11, <alfred.z.song@gmail.com> wrote: > > Since amd_xc_cpuid_policy or intel_xc_cpuid_policy will do it later as > > well, we may drop an extra action. > > But then I think it would be better to keep the generic one and > drop the vendor specific ones. > > Jan > > > Signed-off-by: Zhuo Song <songzhuo.sz@alibaba-inc.com> > > --- > > tools/libxc/xc_cpuid_x86.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > > index 6b81641..61af3e6 100644 > > --- a/tools/libxc/xc_cpuid_x86.c > > +++ b/tools/libxc/xc_cpuid_x86.c > > @@ -392,7 +392,6 @@ static void xc_cpuid_hvm_policy( > > > > case 0x80000001: > > if ( !is_pae ) { > > - clear_bit(X86_FEATURE_NX, regs[3]); > > clear_bit(X86_FEATURE_PSE36, regs[3]); > > } > > break; > > -- > > 1.8.5.2 (Apple Git-48) > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > > > [-- Attachment #1.2: Type: text/html, Size: 4315 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 13:54 ` z @ 2014-09-05 14:10 ` Jan Beulich 2014-09-05 15:45 ` z 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-05 14:10 UTC (permalink / raw) To: z Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 05.09.14 at 15:54, <alfred.z.song@gmail.com> wrote: > Hi, Jan > > Generally, I agree with you. But it seems that we may not simply remove the > ones from the 2 vendor specific functions and keep the generic one, because > the bit could be still cleared eventually when is_pae is true. And I find > PSE36 will be cleared permanently no matter is_pae is true or not. Actually > the work below has been totally overlapped: > > if ( !is_pae ) > { > > clear_bit(X86_FEATURE_NX, > regs[3]); > > clear_bit(X86_FEATURE_PSE36, > regs[3]); > > } > > How can we just "passthrough to cpu vendor specific functions" like leaf > 0x80000000? like this: > > > case 0x80000000: > case > 0x80000001: > > /* Passthrough to cpu vendor specific functions > */ > > break; > > > Correct me if I missed something. I'm sorry, but I don't think I understand what you're trying to say or you're asking. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 14:10 ` Jan Beulich @ 2014-09-05 15:45 ` z 2014-09-05 16:09 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: z @ 2014-09-05 15:45 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1649 bytes --] Hi, Jan I am sorry for making you confused. What I mean is there seems to be some redundant work. For example, to leaf 0x80000001, the generic work (masking NX and PSE36) has been overwritten by the the vendor's functions (amd_xc_cpuid_policy and intel_xc_cpuid_policy) , why couldn't we just drop them and leave the work to the vendor? Of cause, another way is just like you said, keeping the generic ones, and changing the logic in the vendor-based implementation. What I want to do is to simplify this if possible. :) Zhuo On Fri, Sep 5, 2014 at 10:10 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 05.09.14 at 15:54, <alfred.z.song@gmail.com> wrote: > > Hi, Jan > > > > Generally, I agree with you. But it seems that we may not simply remove > the > > ones from the 2 vendor specific functions and keep the generic one, > because > > the bit could be still cleared eventually when is_pae is true. And I find > > PSE36 will be cleared permanently no matter is_pae is true or not. > Actually > > the work below has been totally overlapped: > > > > if ( !is_pae ) > > { > > > > clear_bit(X86_FEATURE_NX, > > regs[3]); > > > > clear_bit(X86_FEATURE_PSE36, > > regs[3]); > > > > } > > > > How can we just "passthrough to cpu vendor specific functions" like leaf > > 0x80000000? like this: > > > > > > case 0x80000000: > > case > > 0x80000001: > > > > /* Passthrough to cpu vendor specific functions > > */ > > > > break; > > > > > > Correct me if I missed something. > > I'm sorry, but I don't think I understand what you're trying to say > or you're asking. > > Jan > > [-- Attachment #1.2: Type: text/html, Size: 2480 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 15:45 ` z @ 2014-09-05 16:09 ` Jan Beulich 2014-09-05 16:35 ` z 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-05 16:09 UTC (permalink / raw) To: z Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 05.09.14 at 17:45, <alfred.z.song@gmail.com> wrote: > Hi, Jan > > I am sorry for making you confused. > > What I mean is there seems to be some redundant work. For example, to leaf > 0x80000001, the generic work (masking NX and PSE36) has been overwritten by > the the vendor's functions (amd_xc_cpuid_policy and intel_xc_cpuid_policy) > , why couldn't we just drop them and leave the work to the vendor? Of > cause, another way is just like you said, keeping the generic ones, and > changing the logic in the vendor-based implementation. > > What I want to do is to simplify this if possible. :) Right, yet conceptually anything that is defined by the architecture would better be done in the generic routine. Anything that is truly vendor specific should be done in the vendor ones. And the dependency of NX on PAE is (nowadays) an architectural one. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 16:09 ` Jan Beulich @ 2014-09-05 16:35 ` z 2014-09-08 6:59 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: z @ 2014-09-05 16:35 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 2467 bytes --] Got it. Thanks, Jan. If so, I think we could remove the condition for masking NX in both vendor specific functions, since the architectural logic has help cover it and the judgement is unnecessary. For example: diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 61af3e6..6bd89b0 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -116,7 +116,7 @@ static void amd_xc_cpuid_policy( bitmaskof(X86_FEATURE_TBM) | bitmaskof(X86_FEATURE_DBEXT)); regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */ - (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | + bitmaskof(X86_FEATURE_NX) | (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | bitmaskof(X86_FEATURE_SYSCALL) | bitmaskof(X86_FEATURE_MP) | @@ -201,7 +201,7 @@ static void intel_xc_cpuid_policy( regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | bitmaskof(X86_FEATURE_3DNOWPREFETCH) | bitmaskof(X86_FEATURE_ABM); - regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | + regs[3] &= (bitmaskof(X86_FEATURE_NX) | (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); Zhuo On Sat, Sep 6, 2014 at 12:09 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 05.09.14 at 17:45, <alfred.z.song@gmail.com> wrote: > > Hi, Jan > > > > I am sorry for making you confused. > > > > What I mean is there seems to be some redundant work. For example, to > leaf > > 0x80000001, the generic work (masking NX and PSE36) has been overwritten > by > > the the vendor's functions (amd_xc_cpuid_policy and > intel_xc_cpuid_policy) > > , why couldn't we just drop them and leave the work to the vendor? Of > > cause, another way is just like you said, keeping the generic ones, and > > changing the logic in the vendor-based implementation. > > > > What I want to do is to simplify this if possible. :) > > Right, yet conceptually anything that is defined by the architecture > would better be done in the generic routine. Anything that is truly > vendor specific should be done in the vendor ones. And the > dependency of NX on PAE is (nowadays) an architectural one. > > Jan > > [-- Attachment #1.2: Type: text/html, Size: 3435 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-05 16:35 ` z @ 2014-09-08 6:59 ` Jan Beulich 2014-09-08 8:48 ` z 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-08 6:59 UTC (permalink / raw) To: z Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 05.09.14 at 18:35, <alfred.z.song@gmail.com> wrote: > Got it. Thanks, Jan. > If so, I think we could remove the condition for masking NX in both vendor > specific functions, since the architectural logic has help cover it and the > judgement is unnecessary. For example: > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > index 61af3e6..6bd89b0 100644 > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -116,7 +116,7 @@ static void amd_xc_cpuid_policy( > bitmaskof(X86_FEATURE_TBM) | > bitmaskof(X86_FEATURE_DBEXT)); > regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */ > - (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | > + bitmaskof(X86_FEATURE_NX) | > (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | > bitmaskof(X86_FEATURE_SYSCALL) | > bitmaskof(X86_FEATURE_MP) | > @@ -201,7 +201,7 @@ static void intel_xc_cpuid_policy( > regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | > bitmaskof(X86_FEATURE_3DNOWPREFETCH) | > bitmaskof(X86_FEATURE_ABM); > - regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | > + regs[3] &= (bitmaskof(X86_FEATURE_NX) | > (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | > (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | > (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); Right, and I think you could go further and move at least the LM check into vendor-independent code too. Looking at the context above I also wonder whether tying RDTSCP to 64-bit guests is really correct in (at least) the Intel case. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 6:59 ` Jan Beulich @ 2014-09-08 8:48 ` z 2014-09-08 9:09 ` Jan Beulich 2014-09-08 14:43 ` z 0 siblings, 2 replies; 16+ messages in thread From: z @ 2014-09-08 8:48 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 6576 bytes --] On Mon, Sep 8, 2014 at 2:59 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 05.09.14 at 18:35, <alfred.z.song@gmail.com> wrote: > > Got it. Thanks, Jan. > > If so, I think we could remove the condition for masking NX in both > vendor > > specific functions, since the architectural logic has help cover it and > the > > judgement is unnecessary. For example: > > > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > > index 61af3e6..6bd89b0 100644 > > --- a/tools/libxc/xc_cpuid_x86.c > > +++ b/tools/libxc/xc_cpuid_x86.c > > @@ -116,7 +116,7 @@ static void amd_xc_cpuid_policy( > > bitmaskof(X86_FEATURE_TBM) | > > bitmaskof(X86_FEATURE_DBEXT)); > > regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX > */ > > - (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | > > + bitmaskof(X86_FEATURE_NX) | > > (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | > > bitmaskof(X86_FEATURE_SYSCALL) | > > bitmaskof(X86_FEATURE_MP) | > > @@ -201,7 +201,7 @@ static void intel_xc_cpuid_policy( > > regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | > > bitmaskof(X86_FEATURE_3DNOWPREFETCH) | > > bitmaskof(X86_FEATURE_ABM); > > - regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | > > + regs[3] &= (bitmaskof(X86_FEATURE_NX) | > > (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | > > (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | > > (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); > > Right, and I think you could go further and move at least the LM > check into vendor-independent code too. > Hi, Jan Thanks. Yes, and I did some changes as below. Please correct me if I did some improper ones. diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 9add109..7d79dc2 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -80,7 +80,7 @@ static void xc_cpuid_brand_get(char *str) static void amd_xc_cpuid_policy( xc_interface *xch, domid_t domid, const unsigned int *input, unsigned int *regs, - int is_pae, int is_nestedhvm) + int is_64bit, int is_pae, int is_nestedhvm) { switch ( input[0] ) { @@ -95,13 +95,11 @@ static void amd_xc_cpuid_policy( break; case 0x80000001: { - int is_64bit = 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] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | + regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) | bitmaskof(X86_FEATURE_CMP_LEGACY) | (is_nestedhvm ? bitmaskof(X86_FEATURE_SVM) : 0) | bitmaskof(X86_FEATURE_CR8_LEGACY) | @@ -117,7 +115,7 @@ static void amd_xc_cpuid_policy( bitmaskof(X86_FEATURE_DBEXT)); regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */ bitmaskof(X86_FEATURE_NX) | - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | + bitmaskof(X86_FEATURE_LM) | bitmaskof(X86_FEATURE_SYSCALL) | bitmaskof(X86_FEATURE_MP) | bitmaskof(X86_FEATURE_MMXEXT) | @@ -169,7 +167,7 @@ static void amd_xc_cpuid_policy( static void intel_xc_cpuid_policy( xc_interface *xch, domid_t domid, const unsigned int *input, unsigned int *regs, - int is_pae, int is_nestedhvm) + int is_64bit, int is_pae, int is_nestedhvm) { switch ( input[0] ) { @@ -195,14 +193,12 @@ static void intel_xc_cpuid_policy( break; case 0x80000001: { - int is_64bit = hypervisor_is_64bit(xch) && is_pae; - /* Only a few features are advertised in Intel's 0x80000001. */ - regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | - bitmaskof(X86_FEATURE_3DNOWPREFETCH) | - bitmaskof(X86_FEATURE_ABM); + regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) | + bitmaskof(X86_FEATURE_3DNOWPREFETCH) | + bitmaskof(X86_FEATURE_ABM); regs[3] &= (bitmaskof(X86_FEATURE_NX) | - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | + bitmaskof(X86_FEATURE_LM) | (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); break; @@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy( DECLARE_DOMCTL; char brand[13]; uint64_t val; - int is_pae, is_nestedhvm; + int is_64bit, is_pae, is_nestedhvm; uint64_t xfeature_mask; xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val); is_pae = !!val; - + + is_64bit = hypervisor_is_64bit(xch) && is_pae; + /* Detecting Xen's atitude towards XSAVE */ memset(&domctl, 0, sizeof(domctl)); domctl.cmd = XEN_DOMCTL_getvcpuextstate; @@ -391,10 +389,18 @@ static void xc_cpuid_hvm_policy( break; case 0x80000001: - if ( !is_pae ) { + if ( !is_64bit ) { + clear_bit(X86_FEATURE_LAHF_LM, regs[2]); + clear_bit(X86_FEATURE_LM, regs[3]); + clear_bit(X86_FEATURE_NX, regs[3]); + clear_bit(X86_FEATURE_PSE36, regs[3]); + } else if ( !is_pae ) { clear_bit(X86_FEATURE_NX, regs[3]); clear_bit(X86_FEATURE_PSE36, regs[3]); + } else { + /* Do nothing for 32-bit guest */ } + break; case 0x80000007: @@ -430,10 +436,11 @@ static void xc_cpuid_hvm_policy( xc_cpuid_brand_get(brand); if ( strstr(brand, "AMD") ) - amd_xc_cpuid_policy(xch, domid, input, regs, is_pae, is_nestedhvm); + amd_xc_cpuid_policy(xch, domid, input, regs, + is_64bit, is_pae, is_nestedhvm); else - intel_xc_cpuid_policy(xch, domid, input, regs, is_pae, is_nestedhvm); - + intel_xc_cpuid_policy(xch, domid, input, regs, + is_64bit, is_pae, is_nestedhvm); > > Looking at the context above I also wonder whether tying RDTSCP > to 64-bit guests is really correct in (at least) the Intel case. > I am not so sure for now, but I will check it later as well:) > > Jan > > Zhuo [-- Attachment #1.2: Type: text/html, Size: 9088 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 8:48 ` z @ 2014-09-08 9:09 ` Jan Beulich 2014-09-08 9:56 ` z 2014-09-08 14:43 ` z 1 sibling, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-08 9:09 UTC (permalink / raw) To: z Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 08.09.14 at 10:48, <alfred.z.song@gmail.com> wrote: Things look fine from a general pov, but > @@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy( > DECLARE_DOMCTL; > char brand[13]; > uint64_t val; > - int is_pae, is_nestedhvm; > + int is_64bit, is_pae, is_nestedhvm; > uint64_t xfeature_mask; > > xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val); > is_pae = !!val; > - > + > + is_64bit = hypervisor_is_64bit(xch) && is_pae; ... with this using hypervisor_is_64bit() and there not being a 32-bit hypervisor anymore, there's clearly room for more cleanup (and in particular no need to pass around an "is_64bit" variable that's always going to be set to true). > @@ -391,10 +389,18 @@ static void xc_cpuid_hvm_policy( > break; > > case 0x80000001: > - if ( !is_pae ) { > + if ( !is_64bit ) { > + clear_bit(X86_FEATURE_LAHF_LM, regs[2]); > + clear_bit(X86_FEATURE_LM, regs[3]); > + clear_bit(X86_FEATURE_NX, regs[3]); > + clear_bit(X86_FEATURE_PSE36, regs[3]); > + } else if ( !is_pae ) { > clear_bit(X86_FEATURE_NX, regs[3]); > clear_bit(X86_FEATURE_PSE36, regs[3]); > + } else { > + /* Do nothing for 32-bit guest */ > } The ordering of the if/else-if above seems wrong to me, but this would become moot anyway if "is_64bit" got dropped. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 9:09 ` Jan Beulich @ 2014-09-08 9:56 ` z 2014-09-08 10:54 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: z @ 2014-09-08 9:56 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 5856 bytes --] On Mon, Sep 8, 2014 at 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 08.09.14 at 10:48, <alfred.z.song@gmail.com> wrote: > > Things look fine from a general pov, but > > > @@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy( > > DECLARE_DOMCTL; > > char brand[13]; > > uint64_t val; > > - int is_pae, is_nestedhvm; > > + int is_64bit, is_pae, is_nestedhvm; > > uint64_t xfeature_mask; > > > > xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val); > > is_pae = !!val; > > - > > + > > + is_64bit = hypervisor_is_64bit(xch) && is_pae; > > ... with this using hypervisor_is_64bit() and there not being a 32-bit > hypervisor anymore, there's clearly room for more cleanup (and in > particular no need to pass around an "is_64bit" variable that's always > going to be set to true). > Do your mean hypervisor_is_64bit() will always return true? Then it means that is_64bit will only depend on is_pae here, so we could simply use is_pae instead of is_64bit in both vendor specific functions. If so, I think xen_64bit in xc_cpuid_pv_policy could also be dropped and function hypervisor_is_64bit() is also redundant now. Right? Maybe something like this? 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 @@ #define DEF_MAX_INTELEXT 0x80000008u #define DEF_MAX_AMDEXT 0x8000001cu -static int hypervisor_is_64bit(xc_interface *xch) -{ - xen_capabilities_info_t xen_caps = ""; - return ((xc_version(xch, XENVER_capabilities, &xen_caps) == 0) && - (strstr(xen_caps, "x86_64") != NULL)); -} - static void cpuid(const unsigned int *input, unsigned int *regs) { unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1]; @@ -95,13 +88,11 @@ static void amd_xc_cpuid_policy( break; case 0x80000001: { - int is_64bit = 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] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | + regs[2] &= (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] &= (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) | bitmaskof(X86_FEATURE_SYSCALL) | bitmaskof(X86_FEATURE_MP) | bitmaskof(X86_FEATURE_MMXEXT) | @@ -195,16 +186,14 @@ static void intel_xc_cpuid_policy( break; case 0x80000001: { - int is_64bit = hypervisor_is_64bit(xch) && is_pae; - /* Only a few features are advertised in Intel's 0x80000001. */ - regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | - bitmaskof(X86_FEATURE_3DNOWPREFETCH) | - bitmaskof(X86_FEATURE_ABM); - regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | - (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | - (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); + regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) | + bitmaskof(X86_FEATURE_3DNOWPREFETCH) | + bitmaskof(X86_FEATURE_ABM); + regs[3] &= (bitmaskof(X86_FEATURE_NX) | + bitmaskof(X86_FEATURE_LM) | + (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | + (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); break; } @@ -392,6 +381,8 @@ static void xc_cpuid_hvm_policy( case 0x80000001: if ( !is_pae ) { + clear_bit(X86_FEATURE_LAHF_LM, regs[2]); + clear_bit(X86_FEATURE_LM, regs[3]); clear_bit(X86_FEATURE_NX, regs[3]); clear_bit(X86_FEATURE_PSE36, regs[3]); } @@ -442,7 +433,7 @@ static void xc_cpuid_pv_policy( { DECLARE_DOMCTL; unsigned int guest_width; - int guest_64bit, xen_64bit = hypervisor_is_64bit(xch); + int guest_64bit; char brand[13]; uint64_t xfeature_mask; @@ -474,7 +465,7 @@ static void xc_cpuid_pv_policy( switch ( input[0] ) { case 0x00000001: - if ( !xen_64bit || strstr(brand, "AMD") ) + if ( strstr(brand, "AMD") ) clear_bit(X86_FEATURE_SEP, regs[3]); clear_bit(X86_FEATURE_DS, regs[3]); clear_bit(X86_FEATURE_ACC, regs[3]); Zhuo > > @@ -391,10 +389,18 @@ static void xc_cpuid_hvm_policy( > > break; > > > > case 0x80000001: > > - if ( !is_pae ) { > > + if ( !is_64bit ) { > > + clear_bit(X86_FEATURE_LAHF_LM, regs[2]); > > + clear_bit(X86_FEATURE_LM, regs[3]); > > + clear_bit(X86_FEATURE_NX, regs[3]); > > + clear_bit(X86_FEATURE_PSE36, regs[3]); > > + } else if ( !is_pae ) { > > clear_bit(X86_FEATURE_NX, regs[3]); > > clear_bit(X86_FEATURE_PSE36, regs[3]); > > + } else { > > + /* Do nothing for 32-bit guest */ > > } > > The ordering of the if/else-if above seems wrong to me, but this > would become moot anyway if "is_64bit" got dropped. > > Jan > > [-- Attachment #1.2: Type: text/html, Size: 8191 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 9:56 ` z @ 2014-09-08 10:54 ` Jan Beulich 2014-09-08 12:07 ` z 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-08 10:54 UTC (permalink / raw) To: z Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 08.09.14 at 11:56, <alfred.z.song@gmail.com> wrote: > On Mon, Sep 8, 2014 at 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 08.09.14 at 10:48, <alfred.z.song@gmail.com> wrote: >> >> Things look fine from a general pov, but >> >> > @@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy( >> > DECLARE_DOMCTL; >> > char brand[13]; >> > uint64_t val; >> > - int is_pae, is_nestedhvm; >> > + int is_64bit, is_pae, is_nestedhvm; >> > uint64_t xfeature_mask; >> > >> > xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val); >> > is_pae = !!val; >> > - >> > + >> > + is_64bit = hypervisor_is_64bit(xch) && is_pae; >> >> ... with this using hypervisor_is_64bit() and there not being a 32-bit >> hypervisor anymore, there's clearly room for more cleanup (and in >> particular no need to pass around an "is_64bit" variable that's always >> going to be set to true). >> > > Do your mean hypervisor_is_64bit() will always return true? Then it means > that is_64bit will only depend on is_pae here, so we could simply use > is_pae instead of is_64bit in both vendor specific functions. If so, I > think xen_64bit in xc_cpuid_pv_policy could also be dropped and > function hypervisor_is_64bit() > is also redundant now. Right? Right. > Maybe something like this? Yes. Thanks, Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 10:54 ` Jan Beulich @ 2014-09-08 12:07 ` z 0 siblings, 0 replies; 16+ messages in thread From: z @ 2014-09-08 12:07 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1556 bytes --] On Mon, Sep 8, 2014 at 6:54 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 08.09.14 at 11:56, <alfred.z.song@gmail.com> wrote: > > On Mon, Sep 8, 2014 at 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote: > > > >> >>> On 08.09.14 at 10:48, <alfred.z.song@gmail.com> wrote: > >> > >> Things look fine from a general pov, but > >> > >> > @@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy( > >> > DECLARE_DOMCTL; > >> > char brand[13]; > >> > uint64_t val; > >> > - int is_pae, is_nestedhvm; > >> > + int is_64bit, is_pae, is_nestedhvm; > >> > uint64_t xfeature_mask; > >> > > >> > xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val); > >> > is_pae = !!val; > >> > - > >> > + > >> > + is_64bit = hypervisor_is_64bit(xch) && is_pae; > >> > >> ... with this using hypervisor_is_64bit() and there not being a 32-bit > >> hypervisor anymore, there's clearly room for more cleanup (and in > >> particular no need to pass around an "is_64bit" variable that's always > >> going to be set to true). > >> > > > > Do your mean hypervisor_is_64bit() will always return true? Then it means > > that is_64bit will only depend on is_pae here, so we could simply use > > is_pae instead of is_64bit in both vendor specific functions. If so, I > > think xen_64bit in xc_cpuid_pv_policy could also be dropped and > > function hypervisor_is_64bit() > > is also redundant now. Right? > > Right. > > > Maybe something like this? > > Yes. > > Thanks, Jan > You are welcome and thanks for your comments and suggestions :) Zhuo [-- Attachment #1.2: Type: text/html, Size: 2510 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 8:48 ` z 2014-09-08 9:09 ` Jan Beulich @ 2014-09-08 14:43 ` z 2014-09-08 15:07 ` Jan Beulich 1 sibling, 1 reply; 16+ messages in thread From: z @ 2014-09-08 14:43 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 7661 bytes --] On Mon, Sep 8, 2014 at 4:48 PM, z <alfred.z.song@gmail.com> wrote: > > > On Mon, Sep 8, 2014 at 2:59 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 05.09.14 at 18:35, <alfred.z.song@gmail.com> wrote: >> > Got it. Thanks, Jan. >> > If so, I think we could remove the condition for masking NX in both >> vendor >> > specific functions, since the architectural logic has help cover it and >> the >> > judgement is unnecessary. For example: >> > >> > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c >> > index 61af3e6..6bd89b0 100644 >> > --- a/tools/libxc/xc_cpuid_x86.c >> > +++ b/tools/libxc/xc_cpuid_x86.c >> > @@ -116,7 +116,7 @@ static void amd_xc_cpuid_policy( >> > bitmaskof(X86_FEATURE_TBM) | >> > bitmaskof(X86_FEATURE_DBEXT)); >> > regs[3] &= (0x0183f3ff | /* features shared with >> 0x00000001:EDX */ >> > - (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | >> > + bitmaskof(X86_FEATURE_NX) | >> > (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | >> > bitmaskof(X86_FEATURE_SYSCALL) | >> > bitmaskof(X86_FEATURE_MP) | >> > @@ -201,7 +201,7 @@ static void intel_xc_cpuid_policy( >> > regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | >> > bitmaskof(X86_FEATURE_3DNOWPREFETCH) | >> > bitmaskof(X86_FEATURE_ABM); >> > - regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | >> > + regs[3] &= (bitmaskof(X86_FEATURE_NX) | >> > (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | >> > (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | >> > (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); >> >> Right, and I think you could go further and move at least the LM >> check into vendor-independent code too. >> > > > Hi, Jan > > Thanks. Yes, and I did some changes as below. > Please correct me if I did some improper ones. > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > index 9add109..7d79dc2 100644 > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -80,7 +80,7 @@ static void xc_cpuid_brand_get(char *str) > static void amd_xc_cpuid_policy( > xc_interface *xch, domid_t domid, > const unsigned int *input, unsigned int *regs, > - int is_pae, int is_nestedhvm) > + int is_64bit, int is_pae, int is_nestedhvm) > { > switch ( input[0] ) > { > @@ -95,13 +95,11 @@ static void amd_xc_cpuid_policy( > break; > > case 0x80000001: { > - int is_64bit = 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] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | > + regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) | > bitmaskof(X86_FEATURE_CMP_LEGACY) | > (is_nestedhvm ? bitmaskof(X86_FEATURE_SVM) : 0) | > bitmaskof(X86_FEATURE_CR8_LEGACY) | > @@ -117,7 +115,7 @@ static void amd_xc_cpuid_policy( > bitmaskof(X86_FEATURE_DBEXT)); > regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */ > bitmaskof(X86_FEATURE_NX) | > - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | > + bitmaskof(X86_FEATURE_LM) | > bitmaskof(X86_FEATURE_SYSCALL) | > bitmaskof(X86_FEATURE_MP) | > bitmaskof(X86_FEATURE_MMXEXT) | > @@ -169,7 +167,7 @@ static void amd_xc_cpuid_policy( > static void intel_xc_cpuid_policy( > xc_interface *xch, domid_t domid, > const unsigned int *input, unsigned int *regs, > - int is_pae, int is_nestedhvm) > + int is_64bit, int is_pae, int is_nestedhvm) > { > switch ( input[0] ) > { > @@ -195,14 +193,12 @@ static void intel_xc_cpuid_policy( > break; > > case 0x80000001: { > - int is_64bit = hypervisor_is_64bit(xch) && is_pae; > - > /* Only a few features are advertised in Intel's 0x80000001. */ > - regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | > - bitmaskof(X86_FEATURE_3DNOWPREFETCH) | > - bitmaskof(X86_FEATURE_ABM); > + regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) | > + bitmaskof(X86_FEATURE_3DNOWPREFETCH) | > + bitmaskof(X86_FEATURE_ABM); > regs[3] &= (bitmaskof(X86_FEATURE_NX) | > - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | > + bitmaskof(X86_FEATURE_LM) | > (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | > (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); > break; > @@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy( > DECLARE_DOMCTL; > char brand[13]; > uint64_t val; > - int is_pae, is_nestedhvm; > + int is_64bit, is_pae, is_nestedhvm; > uint64_t xfeature_mask; > > xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val); > is_pae = !!val; > - > + > + is_64bit = hypervisor_is_64bit(xch) && is_pae; > + > /* Detecting Xen's atitude towards XSAVE */ > memset(&domctl, 0, sizeof(domctl)); > domctl.cmd = XEN_DOMCTL_getvcpuextstate; > @@ -391,10 +389,18 @@ static void xc_cpuid_hvm_policy( > break; > > case 0x80000001: > - if ( !is_pae ) { > + if ( !is_64bit ) { > + clear_bit(X86_FEATURE_LAHF_LM, regs[2]); > + clear_bit(X86_FEATURE_LM, regs[3]); > + clear_bit(X86_FEATURE_NX, regs[3]); > + clear_bit(X86_FEATURE_PSE36, regs[3]); > + } else if ( !is_pae ) { > clear_bit(X86_FEATURE_NX, regs[3]); > clear_bit(X86_FEATURE_PSE36, regs[3]); > + } else { > + /* Do nothing for 32-bit guest */ > } > + > break; > > case 0x80000007: > @@ -430,10 +436,11 @@ static void xc_cpuid_hvm_policy( > > xc_cpuid_brand_get(brand); > if ( strstr(brand, "AMD") ) > - amd_xc_cpuid_policy(xch, domid, input, regs, is_pae, > is_nestedhvm); > + amd_xc_cpuid_policy(xch, domid, input, regs, > + is_64bit, is_pae, is_nestedhvm); > else > - intel_xc_cpuid_policy(xch, domid, input, regs, is_pae, > is_nestedhvm); > - > + intel_xc_cpuid_policy(xch, domid, input, regs, > + is_64bit, is_pae, is_nestedhvm); > > > >> >> Looking at the context above I also wonder whether tying RDTSCP >> to 64-bit guests is really correct in (at least) the Intel case. >> > > I am not so sure for now, but I will check it later as well:) > Hi, Jan I just checked Intel SDM. It seems you are right. RDTSCP could be used for both 64-bit and 32-bit architectures. Maybe the limit for 64-bit should be removed too. diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 710fd61..e7b50b1 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -193,7 +193,7 @@ static void intel_xc_cpuid_policy( regs[3] &= (bitmaskof(X86_FEATURE_NX) | bitmaskof(X86_FEATURE_LM) | (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | - (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); + (bitmaskof(X86_FEATURE_RDTSCP)); break; } Zhuo > > >> >> Jan >> >> > > Zhuo > [-- Attachment #1.2: Type: text/html, Size: 10952 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 14:43 ` z @ 2014-09-08 15:07 ` Jan Beulich 2014-09-09 4:48 ` z 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-09-08 15:07 UTC (permalink / raw) To: z Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, boyu.mt, xen-devel >>> On 08.09.14 at 16:43, <alfred.z.song@gmail.com> wrote: > On Mon, Sep 8, 2014 at 4:48 PM, z <alfred.z.song@gmail.com> wrote: >> On Mon, Sep 8, 2014 at 2:59 PM, Jan Beulich <JBeulich@suse.com> wrote: >>> Looking at the context above I also wonder whether tying RDTSCP >>> to 64-bit guests is really correct in (at least) the Intel case. >>> >> >> I am not so sure for now, but I will check it later as well:) > > I just checked Intel SDM. It seems you are right. RDTSCP could be used for > both 64-bit and 32-bit architectures. Maybe the limit for 64-bit should be > removed too. > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > index 710fd61..e7b50b1 100644 > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -193,7 +193,7 @@ static void intel_xc_cpuid_policy( > regs[3] &= (bitmaskof(X86_FEATURE_NX) | > bitmaskof(X86_FEATURE_LM) | > (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | > - (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); > + (bitmaskof(X86_FEATURE_RDTSCP)); > break; > } That's what I though it should look like; the tying of various features to "is_pae" with your most recent patch is a little strange anyway, i.e. I'd hope that many of those could get cleaned up just like the RDTSCP one. In any even, please submit a format patch for the change above and feel free to extend the cleanup. Thanks again, Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice 2014-09-08 15:07 ` Jan Beulich @ 2014-09-09 4:48 ` z 0 siblings, 0 replies; 16+ messages in thread From: z @ 2014-09-09 4:48 UTC (permalink / raw) To: Jan Beulich Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson, Zhuo Song, 马涛(伯瑜), xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1658 bytes --] On Mon, Sep 8, 2014 at 11:07 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 08.09.14 at 16:43, <alfred.z.song@gmail.com> wrote: > > On Mon, Sep 8, 2014 at 4:48 PM, z <alfred.z.song@gmail.com> wrote: > >> On Mon, Sep 8, 2014 at 2:59 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> Looking at the context above I also wonder whether tying RDTSCP > >>> to 64-bit guests is really correct in (at least) the Intel case. > >>> > >> > >> I am not so sure for now, but I will check it later as well:) > > > > I just checked Intel SDM. It seems you are right. RDTSCP could be used > for > > both 64-bit and 32-bit architectures. Maybe the limit for 64-bit should > be > > removed too. > > > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > > index 710fd61..e7b50b1 100644 > > --- a/tools/libxc/xc_cpuid_x86.c > > +++ b/tools/libxc/xc_cpuid_x86.c > > @@ -193,7 +193,7 @@ static void intel_xc_cpuid_policy( > > regs[3] &= (bitmaskof(X86_FEATURE_NX) | > > bitmaskof(X86_FEATURE_LM) | > > (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | > > - (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); > > + (bitmaskof(X86_FEATURE_RDTSCP)); > > break; > > } > > That's what I though it should look like; the tying of various features > to "is_pae" with your most recent patch is a little strange anyway, > i.e. I'd hope that many of those could get cleaned up just like the > RDTSCP one. > > In any even, please submit a format patch for the change above > and feel free to extend the cleanup. > > Thanks again, Jan > Hi, Jan Okay, thanks. Zhuo [-- Attachment #1.2: Type: text/html, Size: 2565 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-09-09 4:48 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-05 10:11 [PATCH] xc_cpuid_x86.c: No need to mask NX twice Zhuo Song 2014-09-05 10:32 ` Jan Beulich 2014-09-05 13:54 ` z 2014-09-05 14:10 ` Jan Beulich 2014-09-05 15:45 ` z 2014-09-05 16:09 ` Jan Beulich 2014-09-05 16:35 ` z 2014-09-08 6:59 ` Jan Beulich 2014-09-08 8:48 ` z 2014-09-08 9:09 ` Jan Beulich 2014-09-08 9:56 ` z 2014-09-08 10:54 ` Jan Beulich 2014-09-08 12:07 ` z 2014-09-08 14:43 ` z 2014-09-08 15:07 ` Jan Beulich 2014-09-09 4:48 ` z
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).