[PATCH 0/4] x86: some further follow-up to recent XSAs

[PATCH 0/4] x86: some further follow-up to recent XSAs

[Jan Beulich]

The patches here are really independent of one another, it's
just that they all result from work on the recently published
XSAs.

1: suppress XPTI-related TLB flushes when possible
2: relax a few get_gfn() invocations
3: mm: drop redundant local variable from _get_page_type()
4: PV: remove unnecessary toggle_guest_pt() overhead

Jan



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

[PATCH 1/4] x86: suppress XPTI-related TLB flushes when possible

[Jan Beulich]

When there's no XPTI-enabled PV domain at all, there's no need to issue
respective TLB flushes. Hardwire opt_xpti_* to false when !PV, and
record the creation of PV domains by bumping opt_xpti_* accordingly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: The hardwiring to false could be extended to opt_pv_l1tf_* and (for
     !HVM) opt_l1d_flush as well.

--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -218,7 +218,7 @@ unsigned int flush_area_local(const void
                  */
                 invpcid_flush_one(PCID_PV_PRIV, addr);
                 invpcid_flush_one(PCID_PV_USER, addr);
-                if ( opt_xpti_hwdom || opt_xpti_domu )
+                if ( opt_xpti_hwdom > 1 || opt_xpti_domu > 1 )
                 {
                     invpcid_flush_one(PCID_PV_PRIV | PCID_PV_XPTI, addr);
                     invpcid_flush_one(PCID_PV_USER | PCID_PV_XPTI, addr);
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -270,6 +270,9 @@ void pv_domain_destroy(struct domain *d)
     destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
                               GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
 
+    opt_xpti_hwdom -= IS_ENABLED(CONFIG_LATE_HWDOM) &&
+                      !d->domain_id && opt_xpti_hwdom;
+
     XFREE(d->arch.pv.cpuidmasks);
 
     FREE_XENHEAP_PAGE(d->arch.pv.gdt_ldt_l1tab);
@@ -308,7 +311,16 @@ int pv_domain_initialise(struct domain *
     /* 64-bit PV guest by default. */
     d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
 
-    d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
+    if ( is_hardware_domain(d) && opt_xpti_hwdom )
+    {
+        d->arch.pv.xpti = true;
+        ++opt_xpti_hwdom;
+    }
+    if ( !is_hardware_domain(d) && opt_xpti_domu )
+    {
+        d->arch.pv.xpti = true;
+        opt_xpti_domu = 2;
+    }
 
     if ( !is_pv_32bit_domain(d) && use_invpcid && cpu_has_pcid )
         switch ( ACCESS_ONCE(opt_pcid) )
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -78,10 +78,12 @@ static int __init parse_spec_ctrl(const
 
             opt_eager_fpu = 0;
 
+#ifdef CONFIG_PV
             if ( opt_xpti_hwdom < 0 )
                 opt_xpti_hwdom = 0;
             if ( opt_xpti_domu < 0 )
                 opt_xpti_domu = 0;
+#endif
 
             if ( opt_smt < 0 )
                 opt_smt = 1;
@@ -607,6 +609,7 @@ static __init void l1tf_calculations(uin
                                             : (3ul << (paddr_bits - 2))));
 }
 
+#ifdef CONFIG_PV
 int8_t __read_mostly opt_xpti_hwdom = -1;
 int8_t __read_mostly opt_xpti_domu = -1;
 
@@ -673,6 +676,9 @@ static __init int parse_xpti(const char
     return rc;
 }
 custom_param("xpti", parse_xpti);
+#else /* !CONFIG_PV */
+# define xpti_init_default(caps) ((void)(caps))
+#endif /* CONFIG_PV */
 
 void __init init_speculation_mitigations(void)
 {
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -42,7 +42,12 @@ extern bool bsp_delay_spec_ctrl;
 extern uint8_t default_xen_spec_ctrl;
 extern uint8_t default_spec_ctrl_flags;
 
+#ifdef CONFIG_PV
 extern int8_t opt_xpti_hwdom, opt_xpti_domu;
+#else
+# define opt_xpti_hwdom false
+# define opt_xpti_domu false
+#endif
 
 extern int8_t opt_pv_l1tf_hwdom, opt_pv_l1tf_domu;
 





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

Re: [PATCH 1/4] x86: suppress XPTI-related TLB flushes when possible

[Andrew Cooper]

On 13/03/2019 12:38, Jan Beulich wrote:
> When there's no XPTI-enabled PV domain at all, there's no need to issue
> respective TLB flushes. Hardwire opt_xpti_* to false when !PV, and
> record the creation of PV domains by bumping opt_xpti_* accordingly.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: The hardwiring to false could be extended to opt_pv_l1tf_* and (for
>      !HVM) opt_l1d_flush as well.

For what purpose?  opt_pv_l1tf_* is only read inside a CONFIG_PV section
(despite how pv_l1tf_domain_init() is laid out - there is an outer ifdef
as well), while opt_l1d_flush influences the contents of the guests MSR
load list, which is inherently VT-x only.

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -270,6 +270,9 @@ void pv_domain_destroy(struct domain *d)
>      destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
>                                GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
>  
> +    opt_xpti_hwdom -= IS_ENABLED(CONFIG_LATE_HWDOM) &&
> +                      !d->domain_id && opt_xpti_hwdom;
> +
>      XFREE(d->arch.pv.cpuidmasks);
>  
>      FREE_XENHEAP_PAGE(d->arch.pv.gdt_ldt_l1tab);
> @@ -308,7 +311,16 @@ int pv_domain_initialise(struct domain *
>      /* 64-bit PV guest by default. */
>      d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>  
> -    d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
> +    if ( is_hardware_domain(d) && opt_xpti_hwdom )
> +    {
> +        d->arch.pv.xpti = true;
> +        ++opt_xpti_hwdom;
> +    }
> +    if ( !is_hardware_domain(d) && opt_xpti_domu )
> +    {
> +        d->arch.pv.xpti = true;
> +        opt_xpti_domu = 2;

This logic is asymetric.  We will retain TLB flushing after the final
domu has shut down.

I'm also not sure about the hwdom logic.  There is guaranteed to be
exactly one, and Xen will shut down when it goes offline, but it may not
be a PV guest.  opt_xpti_hwdom should be unconditionally 2 on this path
(I think).

> --- a/xen/include/asm-x86/spec_ctrl.h
> +++ b/xen/include/asm-x86/spec_ctrl.h
> @@ -42,7 +42,12 @@ extern bool bsp_delay_spec_ctrl;
>  extern uint8_t default_xen_spec_ctrl;
>  extern uint8_t default_spec_ctrl_flags;
>  
> +#ifdef CONFIG_PV
>  extern int8_t opt_xpti_hwdom, opt_xpti_domu;
> +#else
> +# define opt_xpti_hwdom false
> +# define opt_xpti_domu false
> +#endif

These now have more complicated interaction with flushing.  At the
absolute minimum, it needs a sentence or two about the new semantics.

~Andrew

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

Re: [PATCH 1/4] x86: suppress XPTI-related TLB flushes when possible

[Jan Beulich]

>>> On 03.04.19 at 20:52, <andrew.cooper3@citrix.com> wrote:
> On 13/03/2019 12:38, Jan Beulich wrote:
>> When there's no XPTI-enabled PV domain at all, there's no need to issue
>> respective TLB flushes. Hardwire opt_xpti_* to false when !PV, and
>> record the creation of PV domains by bumping opt_xpti_* accordingly.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: The hardwiring to false could be extended to opt_pv_l1tf_* and (for
>>      !HVM) opt_l1d_flush as well.
> 
> For what purpose?  opt_pv_l1tf_* is only read inside a CONFIG_PV section
> (despite how pv_l1tf_domain_init() is laid out - there is an outer ifdef
> as well),

Oh, right, the benefit would be smaller. Still I think a PV-less Xen would
better report the command line option as unrecognized.

> while opt_l1d_flush influences the contents of the guests MSR
> load list, which is inherently VT-x only.

Along the above lines, a HVM-less Xen would imo better report
the bogus use of option.

>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -270,6 +270,9 @@ void pv_domain_destroy(struct domain *d)
>>      destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
>>                                GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
>>  
>> +    opt_xpti_hwdom -= IS_ENABLED(CONFIG_LATE_HWDOM) &&
>> +                      !d->domain_id && opt_xpti_hwdom;
>> +
>>      XFREE(d->arch.pv.cpuidmasks);
>>  
>>      FREE_XENHEAP_PAGE(d->arch.pv.gdt_ldt_l1tab);
>> @@ -308,7 +311,16 @@ int pv_domain_initialise(struct domain *
>>      /* 64-bit PV guest by default. */
>>      d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>  
>> -    d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
>> +    if ( is_hardware_domain(d) && opt_xpti_hwdom )
>> +    {
>> +        d->arch.pv.xpti = true;
>> +        ++opt_xpti_hwdom;
>> +    }
>> +    if ( !is_hardware_domain(d) && opt_xpti_domu )
>> +    {
>> +        d->arch.pv.xpti = true;
>> +        opt_xpti_domu = 2;
> 
> This logic is asymetric.  We will retain TLB flushing after the final
> domu has shut down.

Well, yes. I didn't want to introduce full counting logic, not the least
because its management would be non-trivial: Once the last PV
DomU has been destroyed, we'd have to wait until the next full
flush in order to be able to decrement the counter, as we may not
bypass earlier flushes.

In fact I now can't figure anymore why I thought this same
argumentation would not also apply to Dom0; the goal of course
was that at least in the transient-early-boot-PV-Dom0 case we'd
be able to go back to non-flushing mode. But I probably should
drop this - the late-hwdom case is rather exotic anyway.

> I'm also not sure about the hwdom logic.  There is guaranteed to be
> exactly one,

(except aiui for a brief period of time, when the late one is
starting, and Dom0 hasn't been destroyed yet)

> and Xen will shut down when it goes offline, but it may not
> be a PV guest.  opt_xpti_hwdom should be unconditionally 2 on this path
> (I think).

As per above I guess I should make it 2 here, but also drop the
decrement.

>> --- a/xen/include/asm-x86/spec_ctrl.h
>> +++ b/xen/include/asm-x86/spec_ctrl.h
>> @@ -42,7 +42,12 @@ extern bool bsp_delay_spec_ctrl;
>>  extern uint8_t default_xen_spec_ctrl;
>>  extern uint8_t default_spec_ctrl_flags;
>>  
>> +#ifdef CONFIG_PV
>>  extern int8_t opt_xpti_hwdom, opt_xpti_domu;
>> +#else
>> +# define opt_xpti_hwdom false
>> +# define opt_xpti_domu false
>> +#endif
> 
> These now have more complicated interaction with flushing.  At the
> absolute minimum, it needs a sentence or two about the new semantics.

Hmm, would their effect on flushing really belong next to the
declarations? But yes, I'll see about adding something.

Jan



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

[PATCH 2/4] x86: relax a few get_gfn() invocations

[Jan Beulich]

In a few cases only a query is intended, i.e. without populating a
possible PoD or paged out entry, when the intention is to replace the
current entry anyway. Use get_gfn_query() there instead.

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

--- a/xen/arch/x86/hvm/grant_table.c
+++ b/xen/arch/x86/hvm/grant_table.c
@@ -59,7 +59,7 @@ int replace_grant_p2m_mapping(uint64_t a
     if ( new_addr != 0 || (flags & GNTMAP_contains_pte) )
         return GNTST_general_error;
 
-    old_mfn = get_gfn(d, gfn, &type);
+    old_mfn = get_gfn_query(d, gfn, &type);
     if ( !p2m_is_grant(type) || !mfn_eq(old_mfn, frame) )
     {
         put_gfn(d, gfn);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4516,7 +4516,7 @@ int xenmem_add_to_physmap_one(
     }
 
     /* Remove previously mapped page if it was present. */
-    prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt));
+    prev_mfn = mfn_x(get_gfn_query(d, gfn_x(gpfn), &p2mt));
     if ( mfn_valid(_mfn(prev_mfn)) )
     {
         if ( is_xen_heap_mfn(prev_mfn) )
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2938,7 +2938,7 @@ int p2m_add_foreign(struct domain *tdom,
     mfn = page_to_mfn(page);
 
     /* Remove previously mapped page if it is present. */
-    prev_mfn = get_gfn(tdom, gpfn, &p2mt_prev);
+    prev_mfn = get_gfn_query(tdom, gpfn, &p2mt_prev);
     if ( mfn_valid(prev_mfn) )
     {
         if ( is_xen_heap_mfn(mfn_x(prev_mfn)) )





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

Re: [PATCH 2/4] x86: relax a few get_gfn() invocations

[Andrew Cooper]

On 13/03/2019 12:38, Jan Beulich wrote:
> In a few cases only a query is intended, i.e. without populating a
> possible PoD or paged out entry, when the intention is to replace the
> current entry anyway. Use get_gfn_query() there instead.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH 2/4] x86: relax a few get_gfn() invocations

[George Dunlap]

On 3/13/19 12:38 PM, Jan Beulich wrote:
> In a few cases only a query is intended, i.e. without populating a
> possible PoD or paged out entry, when the intention is to replace the
> current entry anyway. Use get_gfn_query() there instead.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

The first one should be fine, but pretty sure the second two will
mis-fire for PoD: p2m->pod.entry_count won't get updated.  As it is,
it's sort-of misfiring already, since the "freed" memory won't be
reclaimed for the PoD cache.

Which probably means it's time for some serious refactoring to prevent
this sort of error from creeping in.  p2m->pod.entry_count accounting
should probably be moved into p2m_entry_modify(), and
p2m_pod_decrease_reservation() should probably be moved somewhere else
-- perhaps guest_remove_page() or something?  At any rate, a we probably
need to review of the layers and what everyone needs from them, and do
things at the right level.

 -George


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

Re: [Xen-devel] [PATCH 2/4] x86: relax a few get_gfn() invocations

[George Dunlap]

On 3/13/19 12:38 PM, Jan Beulich wrote:
> In a few cases only a query is intended, i.e. without populating a
> possible PoD or paged out entry, when the intention is to replace the
> current entry anyway. Use get_gfn_query() there instead.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

The first one should be fine, but pretty sure the second two will
mis-fire for PoD: p2m->pod.entry_count won't get updated.  As it is,
it's sort-of misfiring already, since the "freed" memory won't be
reclaimed for the PoD cache.

Which probably means it's time for some serious refactoring to prevent
this sort of error from creeping in.  p2m->pod.entry_count accounting
should probably be moved into p2m_entry_modify(), and
p2m_pod_decrease_reservation() should probably be moved somewhere else
-- perhaps guest_remove_page() or something?  At any rate, a we probably
need to review of the layers and what everyone needs from them, and do
things at the right level.

 -George


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

Re: [PATCH 2/4] x86: relax a few get_gfn() invocations

[Jan Beulich]

>>> On 05.04.19 at 12:30, <george.dunlap@citrix.com> wrote:
> On 3/13/19 12:38 PM, Jan Beulich wrote:
>> In a few cases only a query is intended, i.e. without populating a
>> possible PoD or paged out entry, when the intention is to replace the
>> current entry anyway. Use get_gfn_query() there instead.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> The first one should be fine, but pretty sure the second two will
> mis-fire for PoD: p2m->pod.entry_count won't get updated.  As it is,
> it's sort-of misfiring already, since the "freed" memory won't be
> reclaimed for the PoD cache.

Well, in the 2nd instance (xenmem_add_to_physmap_one()) we
call guest_physmap_add_page(), which is a thin wrapper around
guest_physmap_add_entry(). That function already does _some_
updating of PoD statistics, but quite not enough, I agree.

Thanks for noticing these issues, I withdraw the latter two parts,
and I'll commit just the first hunk (with a slightly adjusted
description).

> Which probably means it's time for some serious refactoring to prevent
> this sort of error from creeping in.  p2m->pod.entry_count accounting
> should probably be moved into p2m_entry_modify(), and

Right.

> p2m_pod_decrease_reservation() should probably be moved somewhere else
> -- perhaps guest_remove_page() or something?

This one I'm less sure about - it's meant to avoid having to go
through guest_remove_page() for every one of the pages
after all. Since it layers on top of ->set_entry(), perhaps its
call site could be left alone, but its statistics adjustments may
need dropping (or modifying) when the former change gets
done.

Otoh when things are properly layered, perhaps it's better not
to have any such "bypasses", and the function would then best
go away altogether?

Jan



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

Re: [Xen-devel] [PATCH 2/4] x86: relax a few get_gfn() invocations

[Jan Beulich]

>>> On 05.04.19 at 12:30, <george.dunlap@citrix.com> wrote:
> On 3/13/19 12:38 PM, Jan Beulich wrote:
>> In a few cases only a query is intended, i.e. without populating a
>> possible PoD or paged out entry, when the intention is to replace the
>> current entry anyway. Use get_gfn_query() there instead.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> The first one should be fine, but pretty sure the second two will
> mis-fire for PoD: p2m->pod.entry_count won't get updated.  As it is,
> it's sort-of misfiring already, since the "freed" memory won't be
> reclaimed for the PoD cache.

Well, in the 2nd instance (xenmem_add_to_physmap_one()) we
call guest_physmap_add_page(), which is a thin wrapper around
guest_physmap_add_entry(). That function already does _some_
updating of PoD statistics, but quite not enough, I agree.

Thanks for noticing these issues, I withdraw the latter two parts,
and I'll commit just the first hunk (with a slightly adjusted
description).

> Which probably means it's time for some serious refactoring to prevent
> this sort of error from creeping in.  p2m->pod.entry_count accounting
> should probably be moved into p2m_entry_modify(), and

Right.

> p2m_pod_decrease_reservation() should probably be moved somewhere else
> -- perhaps guest_remove_page() or something?

This one I'm less sure about - it's meant to avoid having to go
through guest_remove_page() for every one of the pages
after all. Since it layers on top of ->set_entry(), perhaps its
call site could be left alone, but its statistics adjustments may
need dropping (or modifying) when the former change gets
done.

Otoh when things are properly layered, perhaps it's better not
to have any such "bypasses", and the function would then best
go away altogether?

Jan



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

[PATCH 3/4] x86/mm: drop redundant local variable from _get_page_type()

[Jan Beulich]

Instead of the separate iommu_ret, the general rc can be used even for
the IOMMU operations.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2814,7 +2814,7 @@ static int _get_page_type(struct page_in
                           bool preemptible)
 {
     unsigned long nx, x, y = page->u.inuse.type_info;
-    int rc = 0, iommu_ret = 0;
+    int rc = 0;
 
     ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
     ASSERT(!in_irq());
@@ -2926,18 +2926,14 @@ static int _get_page_type(struct page_in
             mfn_t mfn = page_to_mfn(page);
 
             if ( (x & PGT_type_mask) == PGT_writable_page )
-                iommu_ret = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
-                                               PAGE_ORDER_4K);
+                rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), PAGE_ORDER_4K);
             else if ( type == PGT_writable_page )
-                iommu_ret = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
-                                             PAGE_ORDER_4K,
-                                             IOMMUF_readable |
-                                             IOMMUF_writable);
+                rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
+                                      IOMMUF_readable | IOMMUF_writable);
 
-            if ( unlikely(iommu_ret) )
+            if ( unlikely(rc) )
             {
                 _put_page_type(page, false, NULL);
-                rc = iommu_ret;
                 goto out;
             }
         }





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

Re: [PATCH 3/4] x86/mm: drop redundant local variable from _get_page_type()

[Andrew Cooper]

On 13/03/2019 12:38, Jan Beulich wrote:
> Instead of the separate iommu_ret, the general rc can be used even for
> the IOMMU operations.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH 3/4] x86/mm: drop redundant local variable from _get_page_type()

[George Dunlap]

On 3/13/19 12:38 PM, Jan Beulich wrote:
> Instead of the separate iommu_ret, the general rc can be used even for
> the IOMMU operations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [Xen-devel] [PATCH 3/4] x86/mm: drop redundant local variable from _get_page_type()

[George Dunlap]

On 3/13/19 12:38 PM, Jan Beulich wrote:
> Instead of the separate iommu_ret, the general rc can be used even for
> the IOMMU operations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

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

[PATCH 4/4] x86/PV: remove unnecessary toggle_guest_pt() overhead

[Jan Beulich]

While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly
expensive (but still needed only for the toggle_guest_mode() path), the
effect of the latter on the exit-to-guest path is not insignificant.
Move the logic into toggle_guest_mode().

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

--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -349,18 +349,10 @@ bool __init xpti_pcid_enabled(void)
 
 static void _toggle_guest_pt(struct vcpu *v)
 {
-    const struct domain *d = v->domain;
-    struct cpu_info *cpu_info = get_cpu_info();
     unsigned long cr3;
 
     v->arch.flags ^= TF_kernel_mode;
     update_cr3(v);
-    if ( d->arch.pv.xpti )
-    {
-        cpu_info->root_pgt_changed = true;
-        cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
-                           (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0);
-    }
 
     /*
      * Don't flush user global mappings from the TLB. Don't tick TLB clock.
@@ -368,15 +360,11 @@ static void _toggle_guest_pt(struct vcpu
      * In shadow mode, though, update_cr3() may need to be accompanied by a
      * TLB flush (for just the incoming PCID), as the top level page table may
      * have changed behind our backs. To be on the safe side, suppress the
-     * no-flush unconditionally in this case. The XPTI CR3 write, if enabled,
-     * will then need to be a flushing one too.
+     * no-flush unconditionally in this case.
      */
     cr3 = v->arch.cr3;
-    if ( shadow_mode_enabled(d) )
-    {
+    if ( shadow_mode_enabled(v->domain) )
         cr3 &= ~X86_CR3_NOFLUSH;
-        cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
-    }
     write_cr3(cr3);
 
     if ( !(v->arch.flags & TF_kernel_mode) )
@@ -392,6 +380,8 @@ static void _toggle_guest_pt(struct vcpu
 
 void toggle_guest_mode(struct vcpu *v)
 {
+    const struct domain *d = v->domain;
+
     ASSERT(!is_pv_32bit_vcpu(v));
 
     /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
@@ -405,6 +395,21 @@ void toggle_guest_mode(struct vcpu *v)
     asm volatile ( "swapgs" );
 
     _toggle_guest_pt(v);
+
+    if ( d->arch.pv.xpti )
+    {
+        struct cpu_info *cpu_info = get_cpu_info();
+
+        cpu_info->root_pgt_changed = true;
+        cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
+                           (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0);
+        /*
+         * As in _toggle_guest_pt() the XPTI CR3 write needs to be a TLB-
+         * flushing one too for shadow mode guests.
+         */
+        if ( shadow_mode_enabled(d) )
+            cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
+    }
 }
 
 void toggle_guest_pt(struct vcpu *v)





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

Re: [PATCH 4/4] x86/PV: remove unnecessary toggle_guest_pt() overhead

[Andrew Cooper]

On 13/03/2019 12:39, Jan Beulich wrote:
> While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly
> expensive (but still needed only for the toggle_guest_mode() path), the
> effect of the latter on the exit-to-guest path is not insignificant.
> Move the logic into toggle_guest_mode().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

What case does this make faster?  I'm really struggling to see how this
is safe.

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

Re: [PATCH 4/4] x86/PV: remove unnecessary toggle_guest_pt() overhead

[Jan Beulich]

>>> On 03.04.19 at 20:32, <andrew.cooper3@citrix.com> wrote:
> On 13/03/2019 12:39, Jan Beulich wrote:
>> While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly
>> expensive (but still needed only for the toggle_guest_mode() path), the
>> effect of the latter on the exit-to-guest path is not insignificant.
>> Move the logic into toggle_guest_mode().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> What case does this make faster?  I'm really struggling to see how this
> is safe.

The two cases where toggle_guest_pt() gets called (and then
in pairs): Since we know we're going to switch back to the
original page tables (other than when coming through
toggle_guest_mode()), there's no need to request any special
exit-to-guest processing by setting root_pgt_changed. Imo it
was wrong in the first place to have this code added to
_toggle_guest_pt(). The separation of the two functions was
specifically done so that things only affecting a mode switch
would go into toggle_guest_mode(), not the common helper.

Jan



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

Ping: [PATCH 4/4] x86/PV: remove unnecessary toggle_guest_pt() overhead

[Jan Beulich]

>>> On 13.03.19 at 13:39,  wrote:
> While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly
> expensive (but still needed only for the toggle_guest_mode() path), the
> effect of the latter on the exit-to-guest path is not insignificant.
> Move the logic into toggle_guest_mode().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I think I did address the one concern you had.

Jan

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -349,18 +349,10 @@ bool __init xpti_pcid_enabled(void)
>  
>  static void _toggle_guest_pt(struct vcpu *v)
>  {
> -    const struct domain *d = v->domain;
> -    struct cpu_info *cpu_info = get_cpu_info();
>      unsigned long cr3;
>  
>      v->arch.flags ^= TF_kernel_mode;
>      update_cr3(v);
> -    if ( d->arch.pv.xpti )
> -    {
> -        cpu_info->root_pgt_changed = true;
> -        cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
> -                           (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0);
> -    }
>  
>      /*
>       * Don't flush user global mappings from the TLB. Don't tick TLB clock.
> @@ -368,15 +360,11 @@ static void _toggle_guest_pt(struct vcpu
>       * In shadow mode, though, update_cr3() may need to be accompanied by a
>       * TLB flush (for just the incoming PCID), as the top level page table 
> may
>       * have changed behind our backs. To be on the safe side, suppress the
> -     * no-flush unconditionally in this case. The XPTI CR3 write, if 
> enabled,
> -     * will then need to be a flushing one too.
> +     * no-flush unconditionally in this case.
>       */
>      cr3 = v->arch.cr3;
> -    if ( shadow_mode_enabled(d) )
> -    {
> +    if ( shadow_mode_enabled(v->domain) )
>          cr3 &= ~X86_CR3_NOFLUSH;
> -        cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
> -    }
>      write_cr3(cr3);
>  
>      if ( !(v->arch.flags & TF_kernel_mode) )
> @@ -392,6 +380,8 @@ static void _toggle_guest_pt(struct vcpu
>  
>  void toggle_guest_mode(struct vcpu *v)
>  {
> +    const struct domain *d = v->domain;
> +
>      ASSERT(!is_pv_32bit_vcpu(v));
>  
>      /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
> @@ -405,6 +395,21 @@ void toggle_guest_mode(struct vcpu *v)
>      asm volatile ( "swapgs" );
>  
>      _toggle_guest_pt(v);
> +
> +    if ( d->arch.pv.xpti )
> +    {
> +        struct cpu_info *cpu_info = get_cpu_info();
> +
> +        cpu_info->root_pgt_changed = true;
> +        cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
> +                           (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0);
> +        /*
> +         * As in _toggle_guest_pt() the XPTI CR3 write needs to be a TLB-
> +         * flushing one too for shadow mode guests.
> +         */
> +        if ( shadow_mode_enabled(d) )
> +            cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
> +    }
>  }
>  
>  void toggle_guest_pt(struct vcpu *v)
> 
> 
> 
> 





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

[Xen-devel] Ping: [PATCH 4/4] x86/PV: remove unnecessary toggle_guest_pt() overhead

[Jan Beulich]

>>> On 13.03.19 at 13:39,  wrote:
> While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly
> expensive (but still needed only for the toggle_guest_mode() path), the
> effect of the latter on the exit-to-guest path is not insignificant.
> Move the logic into toggle_guest_mode().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I think I did address the one concern you had.

Jan

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -349,18 +349,10 @@ bool __init xpti_pcid_enabled(void)
>  
>  static void _toggle_guest_pt(struct vcpu *v)
>  {
> -    const struct domain *d = v->domain;
> -    struct cpu_info *cpu_info = get_cpu_info();
>      unsigned long cr3;
>  
>      v->arch.flags ^= TF_kernel_mode;
>      update_cr3(v);
> -    if ( d->arch.pv.xpti )
> -    {
> -        cpu_info->root_pgt_changed = true;
> -        cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
> -                           (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0);
> -    }
>  
>      /*
>       * Don't flush user global mappings from the TLB. Don't tick TLB clock.
> @@ -368,15 +360,11 @@ static void _toggle_guest_pt(struct vcpu
>       * In shadow mode, though, update_cr3() may need to be accompanied by a
>       * TLB flush (for just the incoming PCID), as the top level page table 
> may
>       * have changed behind our backs. To be on the safe side, suppress the
> -     * no-flush unconditionally in this case. The XPTI CR3 write, if 
> enabled,
> -     * will then need to be a flushing one too.
> +     * no-flush unconditionally in this case.
>       */
>      cr3 = v->arch.cr3;
> -    if ( shadow_mode_enabled(d) )
> -    {
> +    if ( shadow_mode_enabled(v->domain) )
>          cr3 &= ~X86_CR3_NOFLUSH;
> -        cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
> -    }
>      write_cr3(cr3);
>  
>      if ( !(v->arch.flags & TF_kernel_mode) )
> @@ -392,6 +380,8 @@ static void _toggle_guest_pt(struct vcpu
>  
>  void toggle_guest_mode(struct vcpu *v)
>  {
> +    const struct domain *d = v->domain;
> +
>      ASSERT(!is_pv_32bit_vcpu(v));
>  
>      /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
> @@ -405,6 +395,21 @@ void toggle_guest_mode(struct vcpu *v)
>      asm volatile ( "swapgs" );
>  
>      _toggle_guest_pt(v);
> +
> +    if ( d->arch.pv.xpti )
> +    {
> +        struct cpu_info *cpu_info = get_cpu_info();
> +
> +        cpu_info->root_pgt_changed = true;
> +        cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
> +                           (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0);
> +        /*
> +         * As in _toggle_guest_pt() the XPTI CR3 write needs to be a TLB-
> +         * flushing one too for shadow mode guests.
> +         */
> +        if ( shadow_mode_enabled(d) )
> +            cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
> +    }
>  }
>  
>  void toggle_guest_pt(struct vcpu *v)
> 
> 
> 
> 





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