[v2,3/9] x86/mm: honor opt_pcid also for 32-bit PV domains

Commit Message

Jan Beulich Sept. 17, 2019, 6:14 a.m. UTC

I can't see any technical or performance reason why we should treat
32-bit PV different from 64-bit PV in this regard.

Signed-off-by: Jan Beulich <[e-mail]>
Reviewed-by: Roger Pau Monné <[e-mail]>

Comments

Andrew Cooper Sept. 17, 2019, 7 p.m. UTC | #1

On 17/09/2019 07:14, Jan Beulich wrote:
> I can't see any technical or performance reason why we should treat
> 32-bit PV different from 64-bit PV in this regard.

Well, other than the fact this setting is only read for a 64bit guest...

The reason it isn't set for 32bit guests is that there is no scenario
where we use it.

For PV guests in general, we could consider making a similar
optimisation to Linux and having a LRU list of 4 or so PCIDs to speed up
process context switches, which would be applicable to 32bit guests as
well (so long as someone can reason about the TLB flushing behaviour of
the PV ABI), but this sounds like a large quantity of work.

~Andrew

Jan Beulich Sept. 18, 2019, 9:22 a.m. UTC | #2

On 17.09.2019 21:00, Andrew Cooper wrote:
> On 17/09/2019 07:14, Jan Beulich wrote:
>> I can't see any technical or performance reason why we should treat
>> 32-bit PV different from 64-bit PV in this regard.
> 
> Well, other than the fact this setting is only read for a 64bit guest...

How come? make_cr3() uses it uniformly, as does pv_make_cr4().
toggle_guest_mode() is the one case where it's strictly 64-bit
guest only.

> The reason it isn't set for 32bit guests is that there is no scenario
> where we use it.

"pcid=1" and "pcid=noxpti" both are scenarios where, with this
patch in place, we would use it.

Jan

Andrew Cooper Sept. 18, 2019, 11:55 a.m. UTC | #3

On 18/09/2019 10:22, Jan Beulich wrote:
> On 17.09.2019 21:00, Andrew Cooper wrote:
>> On 17/09/2019 07:14, Jan Beulich wrote:
>>> I can't see any technical or performance reason why we should treat
>>> 32-bit PV different from 64-bit PV in this regard.
>> Well, other than the fact this setting is only read for a 64bit guest...
> How come? make_cr3() uses it uniformly, as does pv_make_cr4().
> toggle_guest_mode() is the one case where it's strictly 64-bit
> guest only.

Oh - you are right.  I don't know how I came to the above conclusion,
but ...

>> The reason it isn't set for 32bit guests is that there is no scenario
>> where we use it.
> "pcid=1" and "pcid=noxpti" both are scenarios where, with this
> patch in place, we would use it.

... I still don't see why this is sensible.

As far as I can tell, all it will do for a 32bit PV guest is start using
2 PCIDs for the same logical gCR3, which will be a net perf it hit for
32bit PV guests.

This is ultimately wrapped up in the confusion over TF_kernel_mode and
v->arch.guest_table{,_user}.

~Andrew

Jan Beulich Sept. 18, 2019, 12:04 p.m. UTC | #4

On 18.09.2019 13:55, Andrew Cooper wrote:
> On 18/09/2019 10:22, Jan Beulich wrote:
>> On 17.09.2019 21:00, Andrew Cooper wrote:
>>> On 17/09/2019 07:14, Jan Beulich wrote:
>>>> I can't see any technical or performance reason why we should treat
>>>> 32-bit PV different from 64-bit PV in this regard.
>>> Well, other than the fact this setting is only read for a 64bit guest...
>> How come? make_cr3() uses it uniformly, as does pv_make_cr4().
>> toggle_guest_mode() is the one case where it's strictly 64-bit
>> guest only.
> 
> Oh - you are right.  I don't know how I came to the above conclusion,
> but ...
> 
>>> The reason it isn't set for 32bit guests is that there is no scenario
>>> where we use it.
>> "pcid=1" and "pcid=noxpti" both are scenarios where, with this
>> patch in place, we would use it.
> 
> ... I still don't see why this is sensible.

Whether it's sensible to at least try out I'm not going to judge.
What I find wrong though is that, for no reason, we don't fully
honor the command line option prior to this change.

> As far as I can tell, all it will do for a 32bit PV guest is start using
> 2 PCIDs for the same logical gCR3, which will be a net perf it hit for
> 32bit PV guests.
> 
> This is ultimately wrapped up in the confusion over TF_kernel_mode and
> v->arch.guest_table{,_user}.

Is there still confusion, despite the cleanup done not overly long
ago? TF_kernel_mode is now uniformly set for HVM and 32-bit PV
vCPU-s; only 64-bit PV vCPU-s can have it clear.

Jan

Patch

--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -180,7 +180,24 @@  int switch_compat(struct domain *d)
     d->arch.x87_fip_width = 4;
 
     d->arch.pv.xpti = false;
-    d->arch.pv.pcid = false;
+
+    if ( use_invpcid && cpu_has_pcid )
+        switch ( ACCESS_ONCE(opt_pcid) )
+        {
+        case PCID_OFF:
+        case PCID_XPTI:
+            d->arch.pv.pcid = false;
+            break;
+
+        case PCID_ALL:
+        case PCID_NOXPTI:
+            d->arch.pv.pcid = true;
+            break;
+
+        default:
+            ASSERT_UNREACHABLE();
+            break;
+        }
 
     return 0;
 
@@ -312,7 +329,7 @@  int pv_domain_initialise(struct domain *
 
     d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
 
-    if ( !is_pv_32bit_domain(d) && use_invpcid && cpu_has_pcid )
+    if ( use_invpcid && cpu_has_pcid )
         switch ( ACCESS_ONCE(opt_pcid) )
         {
         case PCID_OFF: