| Message ID | 2848ec72-6e26-4331-a218-0e3e8f16572f@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86emul: misc additions | expand |
On 04/09/2024 1:29 pm, Jan Beulich wrote: > To represent the USER-MSR bitmap access, a new segment type needs > introducing, behaving like x86_seg_none in terms of address treatment, > but behaving like a system segment for page walk purposes (implicit > supervisor-mode access). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > This feels a little fragile: Of course I did look through uses of the > enumerators, and I didn't find further places which would need > adjustment, but I'm not really sure I didn't miss any place. It does feel a bit fragile, but it may help to consider the other related cases. Here, we need a linear access with implicit-supervisor paging properties. From what I can tell, it needs to be exactly like other implicit supervisor accesses. For CET, we get two new cases. The legacy bitmap has a pointer out of MSR_[U,S]_CET, but otherwise obeys CPL rules, so wants to be x86_seg_none. However, WRUSS is both a CPL0 instruction, and generates implicit-user accesses. It's the first instruction of it's like, that I'm aware of. If we're going down this x86_seg_sys route, we'd need x86_seg_user too. Really, this is a consequence of the memory APIs we've got. It's the intermediate layers which generate PFEC_* for the pagewalk, and we're (ab)using segment at the top level to encode "skip segmentation but I still want certain properties". But, there's actually a 3rd case we get from CET, and it breaks everything. Shstk accesses are a new type, architecturally expressed as a new input (and output) to the pagewalk, but are also regular user-segment relative. We either do the same trick of expressing fetch() in terms of read(PFEC_insn) and implement new shstk_{read,write}() accessors which wrap {read,write}(PFEC_shstk), or we need to plumb the PFEC parameters higher in the call tree. It's worth noting that alignment restrictions make things even more complicated. Generally, shstk accesses should be 8 or 4 byte aligned (based on osize), and the pseudocode for WR{U}SS calls this out; after all they're converting from arbitrary memory operands. However, there's a fun corner case where a 64bit code segment can use INCSSPD to misalign SSP, then CALL to generate a misaligned store. This combines with an erratum in Zen3 and possibly Zen4 where there's a missing #GP check on LRET and you can forge a return address formed of two misaligned addresses. So misaligned stores are definitely possible (I checked this on both vendors at the time), so it wouldn't be appropriate to have in a general shstk_*() helper. In turn, this means that the implementation of WR{U}SS would need a way to linearise it's operand manually to insert the additional check before then making a regular memory access. And I can't see a way of doing this without exposing PFEC inputs at the top level. Thoughts? ~Andrew
On 04.09.2024 18:54, Andrew Cooper wrote: > On 04/09/2024 1:29 pm, Jan Beulich wrote: >> To represent the USER-MSR bitmap access, a new segment type needs >> introducing, behaving like x86_seg_none in terms of address treatment, >> but behaving like a system segment for page walk purposes (implicit >> supervisor-mode access). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> This feels a little fragile: Of course I did look through uses of the >> enumerators, and I didn't find further places which would need >> adjustment, but I'm not really sure I didn't miss any place. > > It does feel a bit fragile, but it may help to consider the other > related cases. > > Here, we need a linear access with implicit-supervisor paging > properties. From what I can tell, it needs to be exactly like other > implicit supervisor accesses. Well, not exactly. There's no segment (and hence no segment base) involved here. Hence, as said in the description, it's a mix of two things we've got so far. > For CET, we get two new cases. > > The legacy bitmap has a pointer out of MSR_[U,S]_CET, but otherwise > obeys CPL rules, so wants to be x86_seg_none. > > However, WRUSS is both a CPL0 instruction, and generates implicit-user > accesses. It's the first instruction of it's like, that I'm aware of. With MOVU having got ripped back out of the 386, yes. (Whether to call such "implicit" user is a separate question.) > If we're going down this x86_seg_sys route, we'd need x86_seg_user too. That won't work, as we need to express the real x86_seg_[cdefgs]s associated with the insn's memory operand. Whereas x86_seg_sys doesn't need combining with anything. > Really, this is a consequence of the memory APIs we've got. It's the > intermediate layers which generate PFEC_* for the pagewalk, and we're > (ab)using segment at the top level to encode "skip segmentation but I > still want certain properties". Right, for USER-MSR. For WRUSS it's "do segmentation and I want two extra properties" (just one for WRSS). > But, there's actually a 3rd case we get from CET, and it breaks everything. > > Shstk accesses are a new type, architecturally expressed as a new input > (and output) to the pagewalk, but are also regular user-segment relative. WR{,U}SS are part of that, aren't they? > We either do the same trick of expressing fetch() in terms of > read(PFEC_insn) and implement new shstk_{read,write}() accessors which > wrap {read,write}(PFEC_shstk), or we need to plumb the PFEC parameters > higher in the call tree. > > It's worth noting that alignment restrictions make things even more > complicated. Generally, shstk accesses should be 8 or 4 byte aligned > (based on osize), and the pseudocode for WR{U}SS calls this out; after > all they're converting from arbitrary memory operands. > > However, there's a fun corner case where a 64bit code segment can use > INCSSPD to misalign SSP, then CALL to generate a misaligned store. This > combines with an erratum in Zen3 and possibly Zen4 where there's a > missing #GP check on LRET and you can forge a return address formed of > two misaligned addresses. Well, we certainly don't need to emulate errata, I'd say. > So misaligned stores are definitely possible (I checked this on both > vendors at the time), so it wouldn't be appropriate to have in a general > shstk_*() helper. In turn, this means that the implementation of > WR{U}SS would need a way to linearise it's operand manually to insert > the additional check before then making a regular memory access. We do such for SSE alignment checking already; see the emulator's is_aligned(). I don't see why we couldn't re-use that for WR{,U}SS. > And I can't see a way of doing this without exposing PFEC inputs at the > top level. Certainly we'll need a qualifier alongside x86_seg_[cdefgs]s, which of course could then also be allowed to be combined with x86_seg_none. Moving PFEC inputs to the top level, while certainly possible, would involve a lot of churn. Plus I'm also hesitant to further grow the hooks' numbers of parameters. IOW introducing new shstk_{read,write}() hooks would look somewhat preferable to me, at least for the moment, if we don't want to have a x86_seg_{shstk,user} flags that can be OR-ed into the other x86_seg_*. Jan
On 04/09/2024 1:29 pm, Jan Beulich wrote: > To represent the USER-MSR bitmap access, a new segment type needs > introducing, behaving like x86_seg_none in terms of address treatment, > but behaving like a system segment for page walk purposes (implicit > supervisor-mode access). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> After discussing this extensively in the maintainers call, and with others, it's probably the best way forwards given the structure of Xen right now. ~Andrew
--- a/tools/tests/x86_emulator/test_x86_emulator.c +++ b/tools/tests/x86_emulator/test_x86_emulator.c @@ -593,6 +593,7 @@ static int read( default: if ( !is_x86_user_segment(seg) ) return X86EMUL_UNHANDLEABLE; + case x86_seg_sys: case x86_seg_none: bytes_read += bytes; break; --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -840,7 +840,7 @@ static int hvmemul_virtual_to_linear( int okay; unsigned long reps = 1; - if ( seg == x86_seg_none ) + if ( seg == x86_seg_none || seg == x86_seg_sys ) { *linear = offset; return X86EMUL_OKAY; --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2601,7 +2601,7 @@ bool hvm_vcpu_virtual_to_linear( * It is expected that the access rights of reg are suitable for seg (and * that this is enforced at the point that seg is loaded). */ - ASSERT(seg < x86_seg_none); + ASSERT(seg < x86_seg_sys); /* However, check that insn fetches only ever specify CS. */ ASSERT(access_type != hvm_access_insn_fetch || seg == x86_seg_cs); --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -749,6 +749,7 @@ static void cf_check svm_set_segment_reg vmcb->ldtr = *reg; break; + case x86_seg_sys: case x86_seg_none: ASSERT_UNREACHABLE(); break; --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -43,7 +43,8 @@ enum x86_segment { x86_seg_ldtr, x86_seg_gdtr, x86_seg_idtr, - /* No Segment: For accesses which are already linear. */ + /* No Segment: For (system/normal) accesses which are already linear. */ + x86_seg_sys, x86_seg_none };
To represent the USER-MSR bitmap access, a new segment type needs introducing, behaving like x86_seg_none in terms of address treatment, but behaving like a system segment for page walk purposes (implicit supervisor-mode access). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This feels a little fragile: Of course I did look through uses of the enumerators, and I didn't find further places which would need adjustment, but I'm not really sure I didn't miss any place. --- v3: New.