April 2017 - Wine-Devel - list.winehq.org

Re: [v6 PATCH 11/21] insn/eval: Incorporate segment base in address computation

by Ricardo Neri

On Fri, 2017-04-21 at 16:55 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:44PM -0800, Ricardo Neri wrote: > > insn_get_addr_ref returns the effective address as defined by the > > Please end function names with parentheses. Will do. > > > section 3.7.5.1 Vol 1 of the Intel 64 and IA-32 Architectures Software > > Developer's Manual. In order to compute the linear address, we must add > > to the effective address the segment base address as set in the segment > > descriptor. Furthermore, the segment descriptor to use depends on the > > register that is used as the base of the effective address. The effective > > base address varies depending on whether the operand is a register or a > > memory address and on whether a SiB byte is used. > > > > In most cases, the segment base address will be 0 if the USER_DS/USER32_DS > > segment is used or if segmentation is not used. However, the base address > > is not necessarily zero if a user programs defines its own segments. This > > is possible by using a local descriptor table. > > > > Since the effective address is a signed quantity, the unsigned segment > > base address saved in a separate variable and added to the final effective > > ".. is saved..." I will correct this. Thanks and BR, Ricardo

April 27, 2017

Re: [v6 PATCH 10/21] x86/insn-eval: Do not use R/EBP as base if mod in ModRM is zero

by Ricardo Neri

On Fri, 2017-04-21 at 12:52 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:43PM -0800, Ricardo Neri wrote: > > Section 2.2.1.3 of the Intel 64 and IA-32 Architectures Software > > Developer's Manual volume 2A states that when the mod part of the ModRM > > byte is zero and R/EBP is specified in the R/M part of such bit, the value > > of the aforementioned register should not be used in the address > > computation. Instead, a 32-bit displacement is expected. The instruction > > decoder takes care of setting the displacement to the expected value. > > Returning -EDOM signals callers that they should ignore the value of such > > register when computing the address encoded in the instruction operands. > > > > Also, callers should exercise care to correctly interpret this particular > > case. In IA-32e 64-bit mode, the address is given by the displacement plus > > the value of the RIP. In IA-32e compatibility mode, the value of EIP is > > ignored. This correction is done for our insn_get_addr_ref. > > > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > > Cc: Colin Ian King <colin.king(a)canonical.com> > > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > > Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> > > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > > Cc: Adrian Hunter <adrian.hunter(a)intel.com> > > Cc: Kees Cook <keescook(a)chromium.org> > > Cc: Thomas Garnier <thgarnie(a)google.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Dmitry Vyukov <dvyukov(a)google.com> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: x86(a)kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/lib/insn-eval.c | 25 +++++++++++++++++++++++-- > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index cda6c71..ea10b03 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -250,6 +250,14 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > switch (type) { > > case REG_TYPE_RM: > > regno = X86_MODRM_RM(insn->modrm.value); > > + /* if mod=0, register R/EBP is not used in the address > > + * computation. Instead, a 32-bit displacement is expected; > > + * the instruction decoder takes care of reading such > > + * displacement. This is true for both R/EBP and R13, as the > > + * REX.B bit is not decoded. > > + */ > > I'd simply write here: "ModRM.mod == 0 and ModRM.rm == 5 means a 32-bit > displacement is following." I will shorten the comment. > > In addition, kernel comments style is: > > /* > * A sentence ending with a full-stop. > * Another sentence. ... > * More sentences. ... > */ ... and use the correct style. I feel bad I missed this one. > > > + if (regno == 5 && X86_MODRM_MOD(insn->modrm.value) == 0) > > + return -EDOM; > > if (X86_MODRM_MOD(insn->modrm.value) == 0 && > X86_MODRM_RM(insn->modrm.value) == 5) > > looks more understandable to me. Should I go with !(X86_MODRM_MOD(insn->modrm.value)) as you suggested in other patches? > > > if (X86_REX_B(insn->rex_prefix.value)) > > regno += 8; > > break; > > @@ -599,9 +607,22 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs) > > eff_addr = base + indx * (1 << X86_SIB_SCALE(sib)); > > } else { > > addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM); > > - if (addr_offset < 0) > > + /* -EDOM means that we must ignore the address_offset. > > + * The only case in which we see this value is when > > + * R/M points to R/EBP. In such a case, in 64-bit mode > > + * the effective address is relative to tho RIP. > > s/tho// Will correct. > > > + */ > > Kernel comments style is: > > /* > * A sentence ending with a full-stop. > * Another sentence. ... > * More sentences. ... > */ > Will correct. > > + if (addr_offset == -EDOM) { > > + eff_addr = 0; > > +#ifdef CONFIG_X86_64 > > + if (user_64bit_mode(regs)) > > + eff_addr = (long)regs->ip; > > Is regs->ip the rIP of the *following* insn? No this is a bug. This should be regs->ip + insn.length. > > > +#endif > > You can do this in a prepatch and then get rid of the ifdeffery here: > > diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h > index 2b5d686ea9f3..f6239273c5f1 100644 > --- a/arch/x86/include/asm/ptrace.h > +++ b/arch/x86/include/asm/ptrace.h > @@ -115,9 +115,9 @@ static inline int v8086_mode(struct pt_regs *regs) > #endif > } > > -#ifdef CONFIG_X86_64 > static inline bool user_64bit_mode(struct pt_regs *regs) > { > +#ifdef CONFIG_X86_64 > #ifndef CONFIG_PARAVIRT > /* > * On non-paravirt systems, this is the only long mode CPL 3 > @@ -128,6 +128,9 @@ static inline bool user_64bit_mode(struct pt_regs *regs) > /* Headers are too twisted for this to go in paravirt.h. */ > return regs->cs == __USER_CS || regs->cs == pv_info.extra_user_64bit_cs; > #endif > +#else /* !CONFIG_X86_64 */ > + return false; > +#endif > } This look nice. I will add this pre-patch. Thanks and BR, Ricardo

April 27, 2017

Re: [v6 PATCH 09/21] x86/insn-eval: Add functions to get default operand and address sizes

by Ricardo Neri

On Thu, 2017-04-20 at 15:06 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:42PM -0800, Ricardo Neri wrote: > > These functions read the default values of the address and operand sizes > > as specified in the segment descriptor. This information is determined > > from the D and L bits. Hence, it can be used for both IA-32e 64-bit and > > 32-bit legacy modes. For virtual-8086 mode, the default address and > > operand sizes are always 2 bytes. > > Yeah, we tend to call that customarily 16-bit :) I will call it like this. > > > The D bit is only meaningful for code segments. Thus, these functions > > always use the code segment selector contained in regs. > > > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > > Cc: Colin Ian King <colin.king(a)canonical.com> > > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > > Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> > > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > > Cc: Adrian Hunter <adrian.hunter(a)intel.com> > > Cc: Kees Cook <keescook(a)chromium.org> > > Cc: Thomas Garnier <thgarnie(a)google.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Dmitry Vyukov <dvyukov(a)google.com> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: x86(a)kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/include/asm/insn-eval.h | 2 + > > arch/x86/lib/insn-eval.c | 80 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 82 insertions(+) > > > > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h > > index b201742..a0d81fc 100644 > > --- a/arch/x86/include/asm/insn-eval.h > > +++ b/arch/x86/include/asm/insn-eval.h > > @@ -15,6 +15,8 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > +unsigned char insn_get_seg_default_address_bytes(struct pt_regs *regs); > > +unsigned char insn_get_seg_default_operand_bytes(struct pt_regs *regs); > > unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > int regoff, bool use_default_seg); > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 383ca83..cda6c71 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -421,6 +421,86 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > } > > > > /** > > + * insn_get_seg_default_address_bytes - Obtain default address size of segment > > + * @regs: Set of registers containing the segment selector > > + * > > + * Obtain the default address size as indicated in the segment descriptor > > + * selected in regs' code segment selector. In protected mode, the default > > + * address is determined by inspecting the L and D bits of the segment > > + * descriptor. In virtual-8086 mode, the default is always two bytes. > > + * > > + * Return: Default address size of segment > > 0 on error. > > > + */ > > +unsigned char insn_get_seg_default_address_bytes(struct pt_regs *regs) > > +{ > > + struct desc_struct *desc; > > + unsigned short seg; > > + int ret; > > + > > + if (v8086_mode(regs)) > > + return 2; > > + > > + seg = (unsigned short)regs->cs; > > + > > + ret = get_desc(seg, &desc); > > + if (ret) > > + return 0; > > + > > + switch ((desc->l << 1) | desc->d) { > > + case 0: /* Legacy mode. 16-bit addresses. CS.L=0, CS.D=0 */ > > + return 2; > > + case 1: /* Legacy mode. 32-bit addresses. CS.L=0, CS.D=1 */ > > + return 4; > > + case 2: /* IA-32e 64-bit mode. 64-bit addresses. CS.L=1, CS.D=0 */ > > + return 8; > > + case 3: /* Invalid setting. CS.L=1, CS.D=1 */ > > + /* fall through */ > > + default: > > + return 0; > > + } > > +} > > + > > +/** > > + * insn_get_seg_default_operand_bytes - Obtain default operand size of segment > > + * @regs: Set of registers containing the segment selector > > + * > > + * Obtain the default operand size as indicated in the segment descriptor > > + * selected in regs' code segment selector. In protected mode, the default > > + * operand size is determined by inspecting the L and D bits of the segment > > + * descriptor. In virtual-8086 mode, the default is always two bytes. > > + * > > + * Return: Default operand size of segment > > + */ > > +unsigned char insn_get_seg_default_operand_bytes(struct pt_regs *regs) > > Right, so default address and operand size always go together so I don't > think you need two separate functions. > > So what I'd suggest - provided this pans out (I still haven't reviewed > the whole thing) - is to determine the operating mode of the segment: > long, legacy, etc and then return both address and operand sizes. Patch > 17/21 needs them both at the same time AFAICT. It makes sense to me. So far these two functions are used in the same place. Thanks and BR, Ricardo

April 27, 2017

Re: [v6 PATCH 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

by Ricardo Neri

On Thu, 2017-04-20 at 10:25 +0200, Borislav Petkov wrote: > > + * insn_get_seg_base() - Obtain base address contained in > descriptor > > + * @regs: Set of registers containing the segment selector > > + * @insn: Instruction structure with selector override prefixes > > + * @regoff: Operand offset, in pt_regs, of which the selector is > needed > > + * @use_default_seg: Use the default segment instead of prefix > overrides > > I'm wondering whether you really need that bool or you can deduce this > from pt_regs... I guess I'll see... Probably insn_get_seg_base() itself can verify if there are segment override prefixes in the struct insn. If yes, use them except for specific cases such as CS. On an unrelated note, I still have the problem of using DS vs ES for string instructions. Perhaps instead of a use_default_seg flag, a string_instruction flag that indicates how to determine the default segment. Thanks and BR, Ricardo

April 26, 2017

Re: [v6 PATCH 08/21] x86/insn-eval: Add utility function to get segment descriptor base address

by Ricardo Neri

On Thu, 2017-04-20 at 10:25 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:41PM -0800, Ricardo Neri wrote: > > With segmentation, the base address of the segment descriptor is needed > > to compute a linear address. The segment descriptor used in the address > > computation depends on either any segment override prefixes in the in the > > s/in the // I will fix this typo. > > > instruction or the default segment determined by the registers involved > > in the address computation. Thus, both the instruction as well as the > > register (specified as the offset from the base of pt_regs) are given as > > inputs, along with a boolean variable to select between override and > > default. > > > > The segment selector is determined by get_seg_selector with the inputs > > Please end function names with parentheses: get_seg_selector(). I will use parentheses. > > > described above. Once the selector is known the base address is > > known, ... Will fix. > > > determined. In protected mode, the selector is used to obtain the segment > > descriptor and then its base address. If in 64-bit user mode, the segment = > > base address is zero except when FS or GS are used. In virtual-8086 mode, > > the base address is computed as the value of the segment selector shifted 4 > > positions to the left. > > Good. > > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > > Cc: Colin Ian King <colin.king(a)canonical.com> > > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > > Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> > > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > > Cc: Adrian Hunter <adrian.hunter(a)intel.com> > > Cc: Kees Cook <keescook(a)chromium.org> > > Cc: Thomas Garnier <thgarnie(a)google.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Dmitry Vyukov <dvyukov(a)google.com> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: x86(a)kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/include/asm/insn-eval.h | 2 ++ > > arch/x86/lib/insn-eval.c | 66 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 68 insertions(+) > > > > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h > > index 754211b..b201742 100644 > > --- a/arch/x86/include/asm/insn-eval.h > > +++ b/arch/x86/include/asm/insn-eval.h > > @@ -15,5 +15,7 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > + int regoff, bool use_default_seg); > > > > #endif /* _ASM_X86_INSN_EVAL_H */ > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 8608adf..383ca83 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -355,6 +355,72 @@ static int get_desc(unsigned short seg, struct desc_struct **desc) > > } > > > > /** > > + * insn_get_seg_base() - Obtain base address contained in descriptor > > + * @regs: Set of registers containing the segment selector > > + * @insn: Instruction structure with selector override prefixes > > + * @regoff: Operand offset, in pt_regs, of which the selector is needed > > + * @use_default_seg: Use the default segment instead of prefix overrides > > I'm wondering whether you really need that bool or you can deduce this > from pt_regs... I guess I'll see... > > > + * > > + * Obtain the base address of the segment descriptor as indicated by either > > + * any segment override prefixes contained in insn or the default segment > > + * applicable to the register indicated by regoff. regoff is specified as the > > + * offset in bytes from the base of pt_regs. > > + * > > + * Return: In protected mode, base address of the segment. It may be zero in > > + * certain cases for 64-bit builds and/or 64-bit applications. In virtual-8086 > > + * mode, the segment selector shifed 4 positions to the right. -1L in case of > > s/shifed/shifted/ I will correct the typo. > > > + * error. > > + */ > > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > + int regoff, bool use_default_seg) > > +{ > > + struct desc_struct *desc; > > + unsigned short seg; > > + enum segment seg_type; > > + int ret; > > + > > + seg_type = resolve_seg_selector(insn, regoff, use_default_seg); > > <--- error handling. I will add it. > > And that's not really a "seg_type" but simply the "sel"-ector. I will update the variable names to reflect the fact that they are segment selectors. > And that > "enum segment" is not really a segment but an segment override prefixes > enum. Can we please get the nomenclature right first? I need a human-readable way of identifying what segment selector (in pt_regs, vm86regs or directly reading the segment registers) to use. Since there is a segment override prefix for all of them, I thought I could use them. Perhaps I can rename enum segment to enum segment_selector and comment that the values in the enum are those of the override prefixes. Would that be reasonable? > > > + > > + seg = get_segment_selector(regs, seg_type); > > s/seg/sel/ Will change. > > > + if (seg < 0) > > + return -1L; > > + > > + if (v8086_mode(regs)) > > + /* > > + * Base is simply the segment selector shifted 4 > > + * positions to the right. > > + */ > > + return (unsigned long)(seg << 4); > > + > > +#ifdef CONFIG_X86_64 > > + if (user_64bit_mode(regs)) { > > if (IS_ENABLED(CONFIG_X86_64) && user_64bit_mode(regs)) { I will change it. Thanks and BR, Ricardo

April 26, 2017

Re: [v6 PATCH 07/21] x86/insn-eval: Add utility function to get segment descriptor

by Ricardo Neri

On Wed, 2017-04-19 at 12:26 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:40PM -0800, Ricardo Neri wrote: > > The segment descriptor contains information that is relevant to how linear > > address need to be computed. It contains the default size of addresses as > > well as the base address of the segment. Thus, given a segment selector, > > we ought look at segment descriptor to correctly calculate the linear > > address. > > > > In protected mode, the segment selector might indicate a segment > > descriptor from either the global descriptor table or a local descriptor > > table. Both cases are considered in this function. > > > > This function is the initial implementation for subsequent functions that > > will obtain the aforementioned attributes of the segment descriptor. > > > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > > Cc: Colin Ian King <colin.king(a)canonical.com> > > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > > Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> > > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > > Cc: Adrian Hunter <adrian.hunter(a)intel.com> > > Cc: Kees Cook <keescook(a)chromium.org> > > Cc: Thomas Garnier <thgarnie(a)google.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Dmitry Vyukov <dvyukov(a)google.com> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: x86(a)kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/lib/insn-eval.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 8d45df8..8608adf 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -5,9 +5,13 @@ > > */ > > #include <linux/kernel.h> > > #include <linux/string.h> > > +#include <asm/desc_defs.h> > > +#include <asm/desc.h> > > #include <asm/inat.h> > > #include <asm/insn.h> > > #include <asm/insn-eval.h> > > +#include <asm/ldt.h> > > +#include <linux/mmu_context.h> > > #include <asm/vm86.h> > > > > enum reg_type { > > @@ -294,6 +298,63 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > > } > > > > /** > > + * get_desc() - Obtain address of segment descriptor > > + * @seg: Segment selector > > Maybe that should be > > @sel > > if it is a sel-ector. :) It makes sense. I will rename it. > > And using "sel" makes more sense then when you look at: > > desc_base = sel & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK); > > for example: > > > + * @desc: Pointer to the selected segment descriptor > > + * > > + * Given a segment selector, obtain a memory pointer to the segment > > s/memory // Will update it. > > > + * descriptor. Both global and local descriptor tables are supported. > > + * desc will contain the address of the descriptor. > > + * > > + * Return: 0 if success, -EINVAL if failure > > Why isn't this function returning the pointer or NULL on error? Maybe > the later patches have an answer and I'll discover it if I continue > reviewing :) After revisiting the code, I don't see why the function cannot return NULL. > > > + */ > > +static int get_desc(unsigned short seg, struct desc_struct **desc) > > +{ > > + struct desc_ptr gdt_desc = {0, 0}; > > + unsigned long desc_base; > > + > > + if (!desc) > > + return -EINVAL; > > + > > + desc_base = seg & ~(SEGMENT_RPL_MASK | SEGMENT_TI_MASK); > > That looks useless as you're doing it below again. Yes, it is useless. Please see my comment below. > > > + > > +#ifdef CONFIG_MODIFY_LDT_SYSCALL > > + if ((seg & SEGMENT_TI_MASK) == SEGMENT_LDT) { > > + seg >>= 3; > > + > > + mutex_lock(&current->active_mm->context.lock); > > + if (unlikely(!current->active_mm->context.ldt || > > Is that really a fast path to complicate the if-test with an unlikely()? > If not, you don't really need it. I will remove it. > > > + seg >= current->active_mm->context.ldt->size)) { > > ldt->size is the size of the descriptor table but you've shifted seg by > 3. That selector index is shifted by 3 (to the left) to form an offset > into the descriptor table because the entries there are 8 bytes. I double-checked the ldt code and it seems to me that size refers to the number of entries in the table; it is always multiplied by LDT_ENTRY_SIZE [1], [2]. Am I missing something? > > So I *think* you wanna use the "useless" desc_base above... :) > > > + *desc = NULL; > > + mutex_unlock(&current->active_mm->context.lock); > > + return -EINVAL; > > + } > > + > > + *desc = &current->active_mm->context.ldt->entries[seg]; > > ... and seg here as it is an index into the table. > > > + mutex_unlock(&current->active_mm->context.lock); > > + return 0; > > + } > > +#endif > > + native_store_gdt(&gdt_desc); > > + > > + /* > > + * Bits [15:3] of the segment selector contain the index. Such > > + * index needs to be multiplied by 8. > > ... because <insert reason I typed in above>. I will elaborate on the reason for this. Thanks and BR, Ricardo [1]. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arc… [2]. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arc…

April 26, 2017

Re: [PATCH 5/5] d3dx9/tests: Get rid of compare_planes.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=30490 Your paranoid android. === build (build) === Patch failed to apply

April 26, 2017

Re: [PATCH 4/5] d3dx9/tests: Cleanup compare_vectors.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=30489 Your paranoid android. === build (build) === Patch failed to apply

April 26, 2017

Re: [PATCH 3/5] d3dx9/tests: Get rid of compare_translation.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=30488 Your paranoid android. === build (build) === Patch failed to apply

April 26, 2017

Re: [PATCH 2/5] d3dx9/tests: Get rid of compare_scale.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=30487 Your paranoid android. === build (build) === Patch failed to apply

April 26, 2017