1. Patchew
  2. Xen
  3. View series

[Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI

Andrew Cooper posted 1 patch 4 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200208151939.31691-1-andrew.cooper3@citrix.com
docs/misc/pvh.pandoc     | 2 +-
tools/libxc/xc_dom_x86.c | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
[Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Andrew Cooper 4 years, 2 months ago 
The written ABI states that %es will be set up, but libxc doesn't do so.  In
practice, it breaks `rep movs` inside guests before they reload %es.

The written ABI doesn't mention %ss, but libxc does set it up.  Having %ds
different to %ss is obnoxous to work with, as different registers have
different implicit segments.

Modify the spec to state that %ss is set up as a flat read/write segment.
This a) matches the Multiboot 1 spec, b) matches what is set up in practice,
and c) is the more sane behaviour for guests to use.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>

This wants backporting.
---
 docs/misc/pvh.pandoc     | 2 +-
 tools/libxc/xc_dom_x86.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/misc/pvh.pandoc b/docs/misc/pvh.pandoc
index f892e6e641..ccf1c8fe69 100644
--- a/docs/misc/pvh.pandoc
+++ b/docs/misc/pvh.pandoc
@@ -23,7 +23,7 @@ following machine state:
  * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
    and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
 
- * `ds`, `es`: must be a 32-bit read/write data segment with a base of
+ * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
    ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
 
  * `tr`: must be a 32-bit TSS (active) with a base of '0' and a limit of '0x67'.
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 155ef69037..9439805eaa 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1053,14 +1053,17 @@ static int vcpu_hvm(struct xc_dom_image *dom)
     /* Set the cached part of the relevant segment registers. */
     bsp_ctx.cpu.cs_base = 0;
     bsp_ctx.cpu.ds_base = 0;
+    bsp_ctx.cpu.es_base = 0;
     bsp_ctx.cpu.ss_base = 0;
     bsp_ctx.cpu.tr_base = 0;
     bsp_ctx.cpu.cs_limit = ~0u;
     bsp_ctx.cpu.ds_limit = ~0u;
+    bsp_ctx.cpu.es_limit = ~0u;
     bsp_ctx.cpu.ss_limit = ~0u;
     bsp_ctx.cpu.tr_limit = 0x67;
     bsp_ctx.cpu.cs_arbytes = 0xc9b;
     bsp_ctx.cpu.ds_arbytes = 0xc93;
+    bsp_ctx.cpu.es_arbytes = 0xc93;
     bsp_ctx.cpu.ss_arbytes = 0xc93;
     bsp_ctx.cpu.tr_arbytes = 0x8b;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Wei Liu 4 years, 2 months ago 
On Sat, Feb 08, 2020 at 03:19:39PM +0000, Andrew Cooper wrote:
> The written ABI states that %es will be set up, but libxc doesn't do so.  In
> practice, it breaks `rep movs` inside guests before they reload %es.
> 
> The written ABI doesn't mention %ss, but libxc does set it up.  Having %ds
> different to %ss is obnoxous to work with, as different registers have
> different implicit segments.
> 
> Modify the spec to state that %ss is set up as a flat read/write segment.
> This a) matches the Multiboot 1 spec, b) matches what is set up in practice,
> and c) is the more sane behaviour for guests to use.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Jan Beulich 4 years, 2 months ago 
On 08.02.2020 16:19, Andrew Cooper wrote:
> --- a/docs/misc/pvh.pandoc
> +++ b/docs/misc/pvh.pandoc
> @@ -23,7 +23,7 @@ following machine state:
>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>  
> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.

Wouldn't this want accompanying with an adjustment to
xen/arch/x86/hvm/domain.c:check_segment(), which right now
isn't in line with either old or new version of this doc?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Andrew Cooper 4 years, 2 months ago 
On 10/02/2020 13:22, Jan Beulich wrote:
> On 08.02.2020 16:19, Andrew Cooper wrote:
>> --- a/docs/misc/pvh.pandoc
>> +++ b/docs/misc/pvh.pandoc
>> @@ -23,7 +23,7 @@ following machine state:
>>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>>  
>> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
>> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
> Wouldn't this want accompanying with an adjustment to
> xen/arch/x86/hvm/domain.c:check_segment(), which right now
> isn't in line with either old or new version of this doc?

What do you thing is missing?  It too is written with the expectation of
%es being set up, which I checked before sending this patch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Jan Beulich 4 years, 2 months ago 
On 10.02.2020 14:29, Andrew Cooper wrote:
> On 10/02/2020 13:22, Jan Beulich wrote:
>> On 08.02.2020 16:19, Andrew Cooper wrote:
>>> --- a/docs/misc/pvh.pandoc
>>> +++ b/docs/misc/pvh.pandoc
>>> @@ -23,7 +23,7 @@ following machine state:
>>>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>>>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>>>  
>>> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
>>> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>>>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
>> Wouldn't this want accompanying with an adjustment to
>> xen/arch/x86/hvm/domain.c:check_segment(), which right now
>> isn't in line with either old or new version of this doc?
> 
> What do you thing is missing?  It too is written with the expectation of
> %es being set up, which I checked before sending this patch.

The function for example looks to permit zero segment attributes
for both DS and ES. It also looks to permit non-writable
attributes for both, and a non-readable CS.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Andrew Cooper 4 years, 2 months ago 
On 10/02/2020 13:47, Jan Beulich wrote:
> On 10.02.2020 14:29, Andrew Cooper wrote:
>> On 10/02/2020 13:22, Jan Beulich wrote:
>>> On 08.02.2020 16:19, Andrew Cooper wrote:
>>>> --- a/docs/misc/pvh.pandoc
>>>> +++ b/docs/misc/pvh.pandoc
>>>> @@ -23,7 +23,7 @@ following machine state:
>>>>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>>>>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>>>>  
>>>> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
>>>> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>>>>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
>>> Wouldn't this want accompanying with an adjustment to
>>> xen/arch/x86/hvm/domain.c:check_segment(), which right now
>>> isn't in line with either old or new version of this doc?
>> What do you thing is missing?  It too is written with the expectation of
>> %es being set up, which I checked before sending this patch.
> The function for example looks to permit zero segment attributes
> for both DS and ES. It also looks to permit non-writable
> attributes for both, and a non-readable CS.

It is not a PVH-auditing function.

It is reachable from plain HVM guests, and is only supposed to be a
minimum set of checks to prevent a vmentry failure of the
newly-initialised vcpu state.  (Whether it actually meets this goal is a
separate matter.)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Jan Beulich 4 years, 2 months ago 
On 10.02.2020 14:56, Andrew Cooper wrote:
> On 10/02/2020 13:47, Jan Beulich wrote:
>> On 10.02.2020 14:29, Andrew Cooper wrote:
>>> On 10/02/2020 13:22, Jan Beulich wrote:
>>>> On 08.02.2020 16:19, Andrew Cooper wrote:
>>>>> --- a/docs/misc/pvh.pandoc
>>>>> +++ b/docs/misc/pvh.pandoc
>>>>> @@ -23,7 +23,7 @@ following machine state:
>>>>>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>>>>>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>>>>>  
>>>>> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
>>>>> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>>>>>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
>>>> Wouldn't this want accompanying with an adjustment to
>>>> xen/arch/x86/hvm/domain.c:check_segment(), which right now
>>>> isn't in line with either old or new version of this doc?
>>> What do you thing is missing?  It too is written with the expectation of
>>> %es being set up, which I checked before sending this patch.
>> The function for example looks to permit zero segment attributes
>> for both DS and ES. It also looks to permit non-writable
>> attributes for both, and a non-readable CS.
> 
> It is not a PVH-auditing function.
> 
> It is reachable from plain HVM guests, and is only supposed to be a
> minimum set of checks to prevent a vmentry failure of the
> newly-initialised vcpu state.  (Whether it actually meets this goal is a
> separate matter.)

Well, that's fine, but what other place am I missing then where the
documented restrictions actually get enforced? Or if we don't mean
to enforce them, then perhaps there should be a distinction in the
doc between "must" and "should"?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Andrew Cooper 4 years, 2 months ago 
On 10/02/2020 14:00, Jan Beulich wrote:
> On 10.02.2020 14:56, Andrew Cooper wrote:
>> On 10/02/2020 13:47, Jan Beulich wrote:
>>> On 10.02.2020 14:29, Andrew Cooper wrote:
>>>> On 10/02/2020 13:22, Jan Beulich wrote:
>>>>> On 08.02.2020 16:19, Andrew Cooper wrote:
>>>>>> --- a/docs/misc/pvh.pandoc
>>>>>> +++ b/docs/misc/pvh.pandoc
>>>>>> @@ -23,7 +23,7 @@ following machine state:
>>>>>>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>>>>>>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>>>>>>  
>>>>>> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
>>>>>> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>>>>>>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
>>>>> Wouldn't this want accompanying with an adjustment to
>>>>> xen/arch/x86/hvm/domain.c:check_segment(), which right now
>>>>> isn't in line with either old or new version of this doc?
>>>> What do you thing is missing?  It too is written with the expectation of
>>>> %es being set up, which I checked before sending this patch.
>>> The function for example looks to permit zero segment attributes
>>> for both DS and ES. It also looks to permit non-writable
>>> attributes for both, and a non-readable CS.
>> It is not a PVH-auditing function.
>>
>> It is reachable from plain HVM guests, and is only supposed to be a
>> minimum set of checks to prevent a vmentry failure of the
>> newly-initialised vcpu state.  (Whether it actually meets this goal is a
>> separate matter.)
> Well, that's fine, but what other place am I missing then where the
> documented restrictions actually get enforced? Or if we don't mean
> to enforce them, then perhaps there should be a distinction in the
> doc between "must" and "should"?

The written ABI is the ABI.  Conforming implementations must (as in
must) follow the rules.

The domain builder(s) are the only places which knows that the PVH start
ABI is in use.

Xen does not know, and therefore cannot legitimately check.  This
hypercall is used for more than just the PVH starting ABI, so must (as
it must) not have any PVH-ABI checks.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Jan Beulich 4 years, 2 months ago 
On 10.02.2020 15:06, Andrew Cooper wrote:
> On 10/02/2020 14:00, Jan Beulich wrote:
>> On 10.02.2020 14:56, Andrew Cooper wrote:
>>> On 10/02/2020 13:47, Jan Beulich wrote:
>>>> On 10.02.2020 14:29, Andrew Cooper wrote:
>>>>> On 10/02/2020 13:22, Jan Beulich wrote:
>>>>>> On 08.02.2020 16:19, Andrew Cooper wrote:
>>>>>>> --- a/docs/misc/pvh.pandoc
>>>>>>> +++ b/docs/misc/pvh.pandoc
>>>>>>> @@ -23,7 +23,7 @@ following machine state:
>>>>>>>   * `cs`: must be a 32-bit read/execute code segment with a base of ‘0’
>>>>>>>     and a limit of ‘0xFFFFFFFF’. The selector value is unspecified.
>>>>>>>  
>>>>>>> - * `ds`, `es`: must be a 32-bit read/write data segment with a base of
>>>>>>> + * `ds`, `es`, `ss`: must be a 32-bit read/write data segment with a base of
>>>>>>>     ‘0’ and a limit of ‘0xFFFFFFFF’. The selector values are all unspecified.
>>>>>> Wouldn't this want accompanying with an adjustment to
>>>>>> xen/arch/x86/hvm/domain.c:check_segment(), which right now
>>>>>> isn't in line with either old or new version of this doc?
>>>>> What do you thing is missing?  It too is written with the expectation of
>>>>> %es being set up, which I checked before sending this patch.
>>>> The function for example looks to permit zero segment attributes
>>>> for both DS and ES. It also looks to permit non-writable
>>>> attributes for both, and a non-readable CS.
>>> It is not a PVH-auditing function.
>>>
>>> It is reachable from plain HVM guests, and is only supposed to be a
>>> minimum set of checks to prevent a vmentry failure of the
>>> newly-initialised vcpu state.  (Whether it actually meets this goal is a
>>> separate matter.)
>> Well, that's fine, but what other place am I missing then where the
>> documented restrictions actually get enforced? Or if we don't mean
>> to enforce them, then perhaps there should be a distinction in the
>> doc between "must" and "should"?
> 
> The written ABI is the ABI.  Conforming implementations must (as in
> must) follow the rules.
> 
> The domain builder(s) are the only places which knows that the PVH start
> ABI is in use.

Looks like I got confused - I'm sorry for the noise.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pvh: Fix segment selector ABI
Posted by Roger Pau Monné 4 years, 2 months ago 
On Sat, Feb 08, 2020 at 03:19:39PM +0000, Andrew Cooper wrote:
> The written ABI states that %es will be set up, but libxc doesn't do so.  In
> practice, it breaks `rep movs` inside guests before they reload %es.
> 
> The written ABI doesn't mention %ss, but libxc does set it up.  Having %ds
> different to %ss is obnoxous to work with, as different registers have
> different implicit segments.
> 
> Modify the spec to state that %ss is set up as a flat read/write segment.
> This a) matches the Multiboot 1 spec, b) matches what is set up in practice,
> and c) is the more sane behaviour for guests to use.

Fixes: 68e1183411b ('libxc: introduce a xc_dom_arch for hvm-3.0-x86_32 guests')

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I would add a note that this also affects the HVM initial CPU state
(albeit that is not part of an ABI).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.