Re: [Xen-devel] [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, April 23, 2014 8:46 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; ian.campbell@xxxxxxxxxx; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@xxxxxxxxxxxxx
> Subject: RE: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions
> 
> >>> On 23.04.14 at 14:32, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >>> On 23.04.14 at 16:34, <feng.wu@xxxxxxxxx> wrote:
> >> > +/* "Raw" instruction opcodes */
> >> > +#define __ASM_CLAC      .byte 0x0f,0x01,0xca
> >> > +#define __ASM_STAC      .byte 0x0f,0x01,0xcb
> >> > +
> >> > +#ifdef __ASSEMBLY__
> >> > +#define ASM_AC(op)                                       \
> >> > +        pushq %rax;                                      \
> >> > +        leaq boot_cpu_data(%rip), %rax;                  \
> >> > +        btl $X86_FEATURE_SMAP-7*32,
> CPUINFO86_leaf7_features(%rax);
> >> \
> >> > +        jnc 881f;                                        \
> >> > +        op;                                              \
> >> > +881:    popq %rax
> >>
> >> So why are you pushing/popping %rax here? There's no need for the
> >> lea.
> >>
> >> And the hard coded 7 here should be replaced too; I don't see a need
> >> for CPUINFO86_leaf7_features either - just calculate everything you
> >> need from X86_FEATURE_SMAP (these are all constants, so other than
> >> the expression getting a little long there's nothing keeping this from
> >> being a single btl).
> >
> > In my understanding, CPUINFO86_leaf7_features is the offset for
> > x86_capability[i] in struct cpuinfo_x86{}, seems we cannot get the
> > right offset only from X86_FEATURE_SMAP?
> 
> Of course you need to add in the offset of x86_capability[].
> See my other reply just sent.
> 
> >> > +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
> >> > +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
> >>
> >> What is "prefix" good for here, i.e. why can't you put the % right
> >> in the macro?
> >
> > Because this macro will be used in the basic inline assembly (use "%" as the
> > register prefix)
> > and extended assembly (use "%%" as the register prefix).
> 
> Perhaps worth avoiding the basic uses then, by converting them to
> extended? Passing these % or %% to the macro looks rather ugly,
> so if the suggestion isn't viable, some other trick can certainly be
> found to avoid this.

Need to add CLAC in the beginning of interrupt in the following macro, which 
uses
the basic inline assembly, seems it is hard to convert this one to extended 
format. I
have been thinking about this for some time and tried several method, but I am 
kind of
run out of ideas about it. Jan, do you have any suggestion about this?

Thanks very much in advance!

#define BUILD_COMMON_IRQ()                      \
__asm__(                                        \
    "\n" __ALIGN_STR"\n"                        \
    "common_interrupt:\n\t"                     \
    STR(SAVE_ALL) "\n\t"                        \
    "movq %rsp,%rdi\n\t"                        \
    "callq " STR(do_IRQ) "\n\t"                 \
    "jmp ret_from_intr\n");

> 
> Jan

Thanks,
Feng

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel