[PATCH] x86/shim: Simplify compat handling in write_start_info()

[PATCH] x86/shim: Simplify compat handling in write_start_info()

[Andrew Cooper]

Factor out a compat boolean to remove the lfence overhead from multiple
is_pv_32bit_domain() calls.

For a compat guest, the upper 32 bits of rdx are zero, so there is no need to
have any conditional logic at all when mapping the start info page.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/pv/shim.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index d16c0048c0..533c194eef 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -280,12 +280,12 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
 static void write_start_info(struct domain *d)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
-    start_info_t *si = map_domain_page(_mfn(is_pv_32bit_domain(d) ? regs->edx
-                                                                  : regs->rdx));
+    bool compat = is_pv_32bit_domain(d);
+    start_info_t *si = map_domain_page(_mfn(regs->rdx));
     uint64_t param;
 
     snprintf(si->magic, sizeof(si->magic), "xen-3.0-x86_%s",
-             is_pv_32bit_domain(d) ? "32p" : "64");
+             compat ? "32p" : "64");
     si->nr_pages = domain_tot_pages(d);
     si->shared_info = virt_to_maddr(d->shared_info);
     si->flags = 0;
@@ -300,7 +300,7 @@ static void write_start_info(struct domain *d)
                                           &si->console.domU.mfn) )
         BUG();
 
-    if ( is_pv_32bit_domain(d) )
+    if ( compat )
         xlat_start_info(si, XLAT_start_info_console_domU);
 
     unmap_domain_page(si);
-- 
2.11.0

Re: [PATCH] x86/shim: Simplify compat handling in write_start_info()

[Jan Beulich]

On 19.04.2021 16:45, Andrew Cooper wrote:
> Factor out a compat boolean to remove the lfence overhead from multiple
> is_pv_32bit_domain() calls.
> 
> For a compat guest, the upper 32 bits of rdx are zero, so there is no need to
> have any conditional logic at all when mapping the start info page.

Iirc the contents of the upper halves hold unspecified contents after
a switch from compat to 64-bit mode. Therefore only with this part of
the change dropped ...

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

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86/shim: Simplify compat handling in write_start_info()

[Andrew Cooper]

On 19/04/2021 16:55, Jan Beulich wrote:
> On 19.04.2021 16:45, Andrew Cooper wrote:
>> Factor out a compat boolean to remove the lfence overhead from multiple
>> is_pv_32bit_domain() calls.
>>
>> For a compat guest, the upper 32 bits of rdx are zero, so there is no need to
>> have any conditional logic at all when mapping the start info page.
> Iirc the contents of the upper halves hold unspecified contents after
> a switch from compat to 64-bit mode. Therefore only with this part of
> the change dropped ...

But we're shim, so will never ever mix compat and non-compat guests.

~Andrew

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan

Re: [PATCH] x86/shim: Simplify compat handling in write_start_info()

[Jan Beulich]

On 19.04.2021 17:57, Andrew Cooper wrote:
> On 19/04/2021 16:55, Jan Beulich wrote:
>> On 19.04.2021 16:45, Andrew Cooper wrote:
>>> Factor out a compat boolean to remove the lfence overhead from multiple
>>> is_pv_32bit_domain() calls.
>>>
>>> For a compat guest, the upper 32 bits of rdx are zero, so there is no need to
>>> have any conditional logic at all when mapping the start info page.
>> Iirc the contents of the upper halves hold unspecified contents after
>> a switch from compat to 64-bit mode. Therefore only with this part of
>> the change dropped ...
> 
> But we're shim, so will never ever mix compat and non-compat guests.

That's not the point: A compat guest will still cause the CPU to
transition back and forth between 64-bit and compat modes. It is
this transitioning which leaves the upper halves of all GPRs in
undefined state (even if in reality a CPU would likely need to go
through extra hoops to prevent them from being zero if they were
written to in compat mode).

Jan

Re: [PATCH] x86/shim: Simplify compat handling in write_start_info()

[Andrew Cooper]

On 19/04/2021 17:00, Jan Beulich wrote:
> On 19.04.2021 17:57, Andrew Cooper wrote:
>> On 19/04/2021 16:55, Jan Beulich wrote:
>>> On 19.04.2021 16:45, Andrew Cooper wrote:
>>>> Factor out a compat boolean to remove the lfence overhead from multiple
>>>> is_pv_32bit_domain() calls.
>>>>
>>>> For a compat guest, the upper 32 bits of rdx are zero, so there is no need to
>>>> have any conditional logic at all when mapping the start info page.
>>> Iirc the contents of the upper halves hold unspecified contents after
>>> a switch from compat to 64-bit mode. Therefore only with this part of
>>> the change dropped ...
>> But we're shim, so will never ever mix compat and non-compat guests.
> That's not the point: A compat guest will still cause the CPU to
> transition back and forth between 64-bit and compat modes. It is
> this transitioning which leaves the upper halves of all GPRs in
> undefined state (even if in reality a CPU would likely need to go
> through extra hoops to prevent them from being zero if they were
> written to in compat mode).

Hmm.  That's awkward.

So real behaviour (I've checked with some contacts) is that upper bits
are preserved until the next write to the register, after which the
upper bits are zeroed.

I wonder whether I'll have any luck formally asking AMD and Intel for a
tweak to this effect in the manuals.

~Andrew