* [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed @ 2016-10-25 18:10 Andrew Cooper 2016-10-25 18:10 ` [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding Andrew Cooper 2016-10-26 8:54 ` [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Jan Beulich 0 siblings, 2 replies; 12+ messages in thread From: Andrew Cooper @ 2016-10-25 18:10 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich x86_emulate.h is included by the userspace test harness. Avoid using constructs which don't come from standard header files. Reposition the test harnesses inclusion of x86_emulate.h to avoid relying on any definitons intended for use by x86_emulate.c alone. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- tools/tests/x86_emulator/test_x86_emulator.c | 2 -- tools/tests/x86_emulator/x86_emulate.c | 6 ++---- xen/arch/x86/x86_emulate/x86_emulate.h | 4 ++-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c index 15d100a..6f6d70f 100644 --- a/tools/tests/x86_emulator/test_x86_emulator.c +++ b/tools/tests/x86_emulator/test_x86_emulator.c @@ -8,8 +8,6 @@ #include <xen/xen.h> #include <sys/mman.h> -#define __packed __attribute__((packed)) - #include "x86_emulate/x86_emulate.h" #include "blowfish.h" diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c index 50a4262..af90b6e 100644 --- a/tools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -6,6 +6,8 @@ #include <string.h> #include <xen/xen.h> +#include "x86_emulate/x86_emulate.h" + typedef bool bool_t; #define is_canonical_address(x) (((int64_t)(x) >> 47) == ((int64_t)(x) >> 63)) @@ -23,8 +25,6 @@ typedef bool bool_t; #define cpu_has_amd_erratum(nr) 0 #define mark_regs_dirty(r) ((void)(r)) -#define __packed __attribute__((packed)) - /* For generic assembly code: use macros to define operation/operand sizes. */ #ifdef __i386__ # define __OS "l" /* Operation Suffix */ @@ -34,8 +34,6 @@ typedef bool bool_t; # define __OP "r" /* Operand Prefix */ #endif -#include "x86_emulate/x86_emulate.h" - #define get_stub(stb) ((void *)((stb).addr = (uintptr_t)(stb).buf)) #define put_stub(stb) diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index 641711e..2b39b81 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -71,7 +71,7 @@ enum x86_swint_emulation { * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the * segment descriptor. It happens to match the format of an AMD SVM VMCB. */ -typedef union __packed segment_attributes { +typedef union __attribute__((packed)) segment_attributes { uint16_t bytes; struct { @@ -91,7 +91,7 @@ typedef union __packed segment_attributes { * Full state of a segment register (visible and hidden portions). * Again, this happens to match the format of an AMD SVM VMCB. */ -struct __packed segment_register { +struct __attribute__((packed)) segment_register { uint16_t sel; segment_attributes_t attr; uint32_t limit; -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-25 18:10 [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Andrew Cooper @ 2016-10-25 18:10 ` Andrew Cooper 2016-10-26 8:57 ` Jan Beulich 2016-10-26 8:54 ` [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Jan Beulich 1 sibling, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2016-10-25 18:10 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich This avoids needing a translation table between hardware ordering and Xen's ordering. This also fixes a bug whereby an encoding using REX.R wasn't ignored. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v2: * Mask out REX.R, which is ignored by hardware. * Expose the BUILD_BUG_ON()s to the test harness, and fix its build. --- tools/tests/x86_emulator/x86_emulate.c | 13 +++++++++++++ xen/arch/x86/x86_emulate/x86_emulate.c | 35 +++++++++++++++------------------- xen/arch/x86/x86_emulate/x86_emulate.h | 4 ++-- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c index af90b6e..c46b7fc 100644 --- a/tools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -19,6 +19,16 @@ typedef bool bool_t; #define ASSERT assert #define ASSERT_UNREACHABLE() assert(!__LINE__) +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6) +/* Force a compilation error if condition is true */ +#define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); }) +#define BUILD_BUG_ON_ZERO(cond) \ + sizeof(struct { _Static_assert(!(cond), "!(" #cond ")"); }) +#else +#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); }) +#define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) +#endif + #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) #define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) @@ -37,4 +47,7 @@ typedef bool bool_t; #define get_stub(stb) ((void *)((stb).addr = (uintptr_t)(stb).buf)) #define put_stub(stb) +#define __init +#define __maybe_unused __attribute__((__unused__)) + #include "x86_emulate/x86_emulate.c" diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index a1821d5..295907e 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1557,22 +1557,6 @@ decode_register( return p; } -#define decode_segment_failed x86_seg_tr -static enum x86_segment -decode_segment(uint8_t modrm_reg) -{ - switch ( modrm_reg ) - { - case 0: return x86_seg_es; - case 1: return x86_seg_cs; - case 2: return x86_seg_ss; - case 3: return x86_seg_ds; - case 4: return x86_seg_fs; - case 5: return x86_seg_gs; - } - return decode_segment_failed; -} - static bool is_aligned(enum x86_segment seg, unsigned long offs, unsigned int size, struct x86_emulate_ctxt *ctxt, const struct x86_emulate_ops *ops) @@ -2982,8 +2966,8 @@ x86_emulate( break; case 0x8c: /* mov Sreg,r/m */ - seg = decode_segment(modrm_reg); - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1); + seg = modrm_reg & 7; /* REX.R is ignored. */ + generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1); store_selector: fail_if(ops->read_segment == NULL); if ( (rc = ops->read_segment(seg, &sreg, ctxt)) != 0 ) @@ -2994,8 +2978,8 @@ x86_emulate( break; case 0x8e: /* mov r/m,Sreg */ - seg = decode_segment(modrm_reg); - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1); + seg = modrm_reg & 7; /* REX.R is ignored. */ + generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1); generate_exception_if(seg == x86_seg_cs, EXC_UD, -1); if ( (rc = load_seg(seg, src.val, 0, NULL, ctxt, ops)) != 0 ) goto done; @@ -5438,6 +5422,17 @@ x86_emulate( #undef override_seg #undef ea +static void __init __maybe_unused build_assertions(void) +{ + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */ + BUILD_BUG_ON(x86_seg_es != 0); + BUILD_BUG_ON(x86_seg_cs != 1); + BUILD_BUG_ON(x86_seg_ss != 2); + BUILD_BUG_ON(x86_seg_ds != 3); + BUILD_BUG_ON(x86_seg_fs != 4); + BUILD_BUG_ON(x86_seg_gs != 5); +} + #ifdef __XEN__ #include <xen/err.h> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index 2b39b81..639356a 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -29,11 +29,11 @@ struct x86_emulate_ctxt; /* Comprehensive enumeration of x86 segment registers. */ enum x86_segment { - /* General purpose. */ + /* General purpose. Matches the SReg3 encoding in opcode/ModRM bytes. */ + x86_seg_es, x86_seg_cs, x86_seg_ss, x86_seg_ds, - x86_seg_es, x86_seg_fs, x86_seg_gs, /* System. */ -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-25 18:10 ` [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding Andrew Cooper @ 2016-10-26 8:57 ` Jan Beulich 2016-10-26 9:40 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2016-10-26 8:57 UTC (permalink / raw) To: Andrew Cooper; +Cc: Wei Liu, Xen-devel >>> On 25.10.16 at 20:10, <andrew.cooper3@citrix.com> wrote: > @@ -2994,8 +2978,8 @@ x86_emulate( > break; > > case 0x8e: /* mov r/m,Sreg */ > - seg = decode_segment(modrm_reg); > - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1); > + seg = modrm_reg & 7; /* REX.R is ignored. */ > + generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1); > generate_exception_if(seg == x86_seg_cs, EXC_UD, -1); Can I talk you into folding these two generate_exception_if()s, as they logically belong together? > @@ -5438,6 +5422,17 @@ x86_emulate( > #undef override_seg > #undef ea > > +static void __init __maybe_unused build_assertions(void) > +{ > + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */ > + BUILD_BUG_ON(x86_seg_es != 0); > + BUILD_BUG_ON(x86_seg_cs != 1); > + BUILD_BUG_ON(x86_seg_ss != 2); > + BUILD_BUG_ON(x86_seg_ds != 3); > + BUILD_BUG_ON(x86_seg_fs != 4); > + BUILD_BUG_ON(x86_seg_gs != 5); > +} So using an inline function in the header did not work out? In any event, Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 8:57 ` Jan Beulich @ 2016-10-26 9:40 ` Andrew Cooper 2016-10-26 9:48 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2016-10-26 9:40 UTC (permalink / raw) To: Jan Beulich; +Cc: Wei Liu, Xen-devel On 26/10/16 09:57, Jan Beulich wrote: >>>> On 25.10.16 at 20:10, <andrew.cooper3@citrix.com> wrote: >> @@ -2994,8 +2978,8 @@ x86_emulate( >> break; >> >> case 0x8e: /* mov r/m,Sreg */ >> - seg = decode_segment(modrm_reg); >> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1); >> + seg = modrm_reg & 7; /* REX.R is ignored. */ >> + generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1); >> generate_exception_if(seg == x86_seg_cs, EXC_UD, -1); > Can I talk you into folding these two generate_exception_if()s, as > they logically belong together? Ok. > >> @@ -5438,6 +5422,17 @@ x86_emulate( >> #undef override_seg >> #undef ea >> >> +static void __init __maybe_unused build_assertions(void) >> +{ >> + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */ >> + BUILD_BUG_ON(x86_seg_es != 0); >> + BUILD_BUG_ON(x86_seg_cs != 1); >> + BUILD_BUG_ON(x86_seg_ss != 2); >> + BUILD_BUG_ON(x86_seg_ds != 3); >> + BUILD_BUG_ON(x86_seg_fs != 4); >> + BUILD_BUG_ON(x86_seg_gs != 5); >> +} > So using an inline function in the header did not work out? No - that would involve making BUILD_BUG_ON() available to x86_emulate.h In this position, it does also affect the test harness build, as it is outside an #ifdef __XEN__ block. > > In any event, > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 9:40 ` Andrew Cooper @ 2016-10-26 9:48 ` Andrew Cooper 2016-10-26 9:54 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2016-10-26 9:48 UTC (permalink / raw) To: Wei Liu; +Cc: Jan Beulich, Xen-devel On 26/10/16 10:40, Andrew Cooper wrote: > On 26/10/16 09:57, Jan Beulich wrote: >>>>> On 25.10.16 at 20:10, <andrew.cooper3@citrix.com> wrote: >>> @@ -2994,8 +2978,8 @@ x86_emulate( >>> break; >>> >>> case 0x8e: /* mov r/m,Sreg */ >>> - seg = decode_segment(modrm_reg); >>> - generate_exception_if(seg == decode_segment_failed, EXC_UD, -1); >>> + seg = modrm_reg & 7; /* REX.R is ignored. */ >>> + generate_exception_if(!is_x86_user_segment(seg), EXC_UD, -1); >>> generate_exception_if(seg == x86_seg_cs, EXC_UD, -1); >> Can I talk you into folding these two generate_exception_if()s, as >> they logically belong together? > Ok. > >>> @@ -5438,6 +5422,17 @@ x86_emulate( >>> #undef override_seg >>> #undef ea >>> >>> +static void __init __maybe_unused build_assertions(void) >>> +{ >>> + /* Check the values against SReg3 encoding in opcode/ModRM bytes. */ >>> + BUILD_BUG_ON(x86_seg_es != 0); >>> + BUILD_BUG_ON(x86_seg_cs != 1); >>> + BUILD_BUG_ON(x86_seg_ss != 2); >>> + BUILD_BUG_ON(x86_seg_ds != 3); >>> + BUILD_BUG_ON(x86_seg_fs != 4); >>> + BUILD_BUG_ON(x86_seg_gs != 5); >>> +} >> So using an inline function in the header did not work out? > No - that would involve making BUILD_BUG_ON() available to x86_emulate.h > > In this position, it does also affect the test harness build, as it is > outside an #ifdef __XEN__ block. > >> In any event, >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > Thanks Wei: Turns out that I typo'd the subject, and I meant "for 4.8" here, as this is a bugfix in the emulator. Please can I get a view towards a release ack? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 9:48 ` Andrew Cooper @ 2016-10-26 9:54 ` Jan Beulich 2016-10-26 9:57 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2016-10-26 9:54 UTC (permalink / raw) To: Andrew Cooper, Wei Liu; +Cc: Xen-devel >>> On 26.10.16 at 11:48, <andrew.cooper3@citrix.com> wrote: > Wei: Turns out that I typo'd the subject, and I meant "for 4.8" here, as > this is a bugfix in the emulator. Please can I get a view towards a > release ack? Oh, I didn't even realize that v2 has become a bugfix instead of mere simplification. Somehow I had been under the impression that decode_segment() behaved correctly. Perhaps the subject should then be changed? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 9:54 ` Jan Beulich @ 2016-10-26 9:57 ` Andrew Cooper 2016-10-26 10:16 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2016-10-26 9:57 UTC (permalink / raw) To: Jan Beulich, Wei Liu; +Cc: Xen-devel On 26/10/16 10:54, Jan Beulich wrote: >>>> On 26.10.16 at 11:48, <andrew.cooper3@citrix.com> wrote: >> Wei: Turns out that I typo'd the subject, and I meant "for 4.8" here, as >> this is a bugfix in the emulator. Please can I get a view towards a >> release ack? > Oh, I didn't even realize that v2 has become a bugfix instead of mere > simplification. Somehow I had been under the impression that > decode_segment() behaved correctly. Perhaps the subject should > then be changed? I was entirely accidentally a bugfix, but a bugfix none the less. I am open to changing the title. How about "Correct the decoding of SReg3 operands"? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 9:57 ` Andrew Cooper @ 2016-10-26 10:16 ` Jan Beulich 2016-10-26 11:26 ` Andrew Cooper 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2016-10-26 10:16 UTC (permalink / raw) To: Andrew Cooper; +Cc: Wei Liu, Xen-devel >>> On 26.10.16 at 11:57, <andrew.cooper3@citrix.com> wrote: > On 26/10/16 10:54, Jan Beulich wrote: >>>>> On 26.10.16 at 11:48, <andrew.cooper3@citrix.com> wrote: >>> Wei: Turns out that I typo'd the subject, and I meant "for 4.8" here, as >>> this is a bugfix in the emulator. Please can I get a view towards a >>> release ack? >> Oh, I didn't even realize that v2 has become a bugfix instead of mere >> simplification. Somehow I had been under the impression that >> decode_segment() behaved correctly. Perhaps the subject should >> then be changed? > > I was entirely accidentally a bugfix, but a bugfix none the less. > > I am open to changing the title. How about "Correct the decoding of > SReg3 operands"? SGTM, together with some adjustment to the description then (as the original patch purpose now has become a cleanup side effect). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 10:16 ` Jan Beulich @ 2016-10-26 11:26 ` Andrew Cooper 2016-10-26 12:19 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2016-10-26 11:26 UTC (permalink / raw) To: Jan Beulich; +Cc: Wei Liu, Xen-devel On 26/10/16 11:16, Jan Beulich wrote: >>>> On 26.10.16 at 11:57, <andrew.cooper3@citrix.com> wrote: >> On 26/10/16 10:54, Jan Beulich wrote: >>>>>> On 26.10.16 at 11:48, <andrew.cooper3@citrix.com> wrote: >>>> Wei: Turns out that I typo'd the subject, and I meant "for 4.8" here, as >>>> this is a bugfix in the emulator. Please can I get a view towards a >>>> release ack? >>> Oh, I didn't even realize that v2 has become a bugfix instead of mere >>> simplification. Somehow I had been under the impression that >>> decode_segment() behaved correctly. Perhaps the subject should >>> then be changed? >> I was entirely accidentally a bugfix, but a bugfix none the less. >> >> I am open to changing the title. How about "Correct the decoding of >> SReg3 operands"? > SGTM, together with some adjustment to the description then (as > the original patch purpose now has become a cleanup side effect). Text now reads: x86/emul: Correct the decoding of SReg3 operands REX.R is ignored when considering segment register operands, and needs masking out first. While fixing this, reorder the user segments in x86_segment to match SReg3 encoding. This avoids needing a translation table between hardware ordering and Xen's ordering. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding 2016-10-26 11:26 ` Andrew Cooper @ 2016-10-26 12:19 ` Jan Beulich 0 siblings, 0 replies; 12+ messages in thread From: Jan Beulich @ 2016-10-26 12:19 UTC (permalink / raw) To: Andrew Cooper; +Cc: Wei Liu, Xen-devel >>> On 26.10.16 at 13:26, <andrew.cooper3@citrix.com> wrote: > On 26/10/16 11:16, Jan Beulich wrote: >>>>> On 26.10.16 at 11:57, <andrew.cooper3@citrix.com> wrote: >>> On 26/10/16 10:54, Jan Beulich wrote: >>>>>>> On 26.10.16 at 11:48, <andrew.cooper3@citrix.com> wrote: >>>>> Wei: Turns out that I typo'd the subject, and I meant "for 4.8" here, as >>>>> this is a bugfix in the emulator. Please can I get a view towards a >>>>> release ack? >>>> Oh, I didn't even realize that v2 has become a bugfix instead of mere >>>> simplification. Somehow I had been under the impression that >>>> decode_segment() behaved correctly. Perhaps the subject should >>>> then be changed? >>> I was entirely accidentally a bugfix, but a bugfix none the less. >>> >>> I am open to changing the title. How about "Correct the decoding of >>> SReg3 operands"? >> SGTM, together with some adjustment to the description then (as >> the original patch purpose now has become a cleanup side effect). > > Text now reads: > > x86/emul: Correct the decoding of SReg3 operands > > REX.R is ignored when considering segment register operands, and needs > masking > out first. > > While fixing this, reorder the user segments in x86_segment to match SReg3 > encoding. This avoids needing a translation table between hardware ordering > and Xen's ordering. Thanks, sounds good. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed 2016-10-25 18:10 [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Andrew Cooper 2016-10-25 18:10 ` [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding Andrew Cooper @ 2016-10-26 8:54 ` Jan Beulich 2016-10-26 9:39 ` Andrew Cooper 1 sibling, 1 reply; 12+ messages in thread From: Jan Beulich @ 2016-10-26 8:54 UTC (permalink / raw) To: Andrew Cooper; +Cc: Wei Liu, Xen-devel >>> On 25.10.16 at 20:10, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -71,7 +71,7 @@ enum x86_swint_emulation { > * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the > * segment descriptor. It happens to match the format of an AMD SVM VMCB. > */ > -typedef union __packed segment_attributes { > +typedef union __attribute__((packed)) segment_attributes { > uint16_t bytes; > struct > { > @@ -91,7 +91,7 @@ typedef union __packed segment_attributes { > * Full state of a segment register (visible and hidden portions). > * Again, this happens to match the format of an AMD SVM VMCB. > */ > -struct __packed segment_register { > +struct __attribute__((packed)) segment_register { > uint16_t sel; > segment_attributes_t attr; > uint32_t limit; Please use __packed__ in both cases (using the underscore free variant is better to be restricted to just .c files). With that Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed 2016-10-26 8:54 ` [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Jan Beulich @ 2016-10-26 9:39 ` Andrew Cooper 0 siblings, 0 replies; 12+ messages in thread From: Andrew Cooper @ 2016-10-26 9:39 UTC (permalink / raw) To: Jan Beulich; +Cc: Wei Liu, Xen-devel On 26/10/16 09:54, Jan Beulich wrote: >>>> On 25.10.16 at 20:10, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -71,7 +71,7 @@ enum x86_swint_emulation { >> * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the >> * segment descriptor. It happens to match the format of an AMD SVM VMCB. >> */ >> -typedef union __packed segment_attributes { >> +typedef union __attribute__((packed)) segment_attributes { >> uint16_t bytes; >> struct >> { >> @@ -91,7 +91,7 @@ typedef union __packed segment_attributes { >> * Full state of a segment register (visible and hidden portions). >> * Again, this happens to match the format of an AMD SVM VMCB. >> */ >> -struct __packed segment_register { >> +struct __attribute__((packed)) segment_register { >> uint16_t sel; >> segment_attributes_t attr; >> uint32_t limit; > Please use __packed__ in both cases (using the underscore free > variant is better to be restricted to just .c files). With that > Reviewed-by: Jan Beulich <jbeulich@suse.com> Will do. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-10-26 12:19 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-25 18:10 [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Andrew Cooper 2016-10-25 18:10 ` [PATCH v2 for-4.9 2/2] x86/emul: Reorder the user segments in x86_segment to match SReg3 encoding Andrew Cooper 2016-10-26 8:57 ` Jan Beulich 2016-10-26 9:40 ` Andrew Cooper 2016-10-26 9:48 ` Andrew Cooper 2016-10-26 9:54 ` Jan Beulich 2016-10-26 9:57 ` Andrew Cooper 2016-10-26 10:16 ` Jan Beulich 2016-10-26 11:26 ` Andrew Cooper 2016-10-26 12:19 ` Jan Beulich 2016-10-26 8:54 ` [PATCH v2 for-4.9 1/2] x86/emul: Use explicit __attribute__((packed)) rather than __packed Jan Beulich 2016-10-26 9:39 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.