diff mbox series

[v2,8/9] x86/shadow: call sh_detach_old_tables() directly

Message ID 8ed7a628-f64e-5512-efdb-4116a7b88a1d@suse.com (mailing list archive)
State Superseded
Headers show
Series x86/shadow: misc tidying | expand

Commit Message

Jan Beulich Jan. 11, 2023, 1:57 p.m. UTC 
There's nothing really mode specific in this function anymore (the
varying number of valid entries in v->arch.paging.shadow.shadow_table[]
is dealt with fine by the zero check, and we have other similar cases of
iterating through the full array in common.c), and hence there's neither
a need to have multiple instances of it, nor does it need calling
through a function pointer.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I've retained the C++-style comment in the function as this style is
used elsewhere as well in shadow code. I wouldn't mind changing the
comment to conform to ./CODING_STYLE.
---
v2: New.

Comments

Andrew Cooper Jan. 11, 2023, 11:56 p.m. UTC | #1 
On 11/01/2023 1:57 pm, Jan Beulich wrote:
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2264,6 +2264,29 @@ void shadow_prepare_page_type_change(str
>      shadow_remove_all_shadows(d, page_to_mfn(page));
>  }
>  
> +/*
> + * Removes v->arch.paging.shadow.shadow_table[].
> + * Does all appropriate management/bookkeeping/refcounting/etc...
> + */
> +static void sh_detach_old_tables(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    unsigned int i;
> +
> +    ////
> +    //// vcpu->arch.paging.shadow.shadow_table[]
> +    ////

Honestly, I don't see what the point of this comment is at all.  I'd
suggest just dropping it as you move the function, which avoids the need
to debate over C++ comments.

Preferably with this done, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>
Jan Beulich Jan. 12, 2023, 9:37 a.m. UTC | #2 
On 12.01.2023 00:56, Andrew Cooper wrote:
> On 11/01/2023 1:57 pm, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -2264,6 +2264,29 @@ void shadow_prepare_page_type_change(str
>>      shadow_remove_all_shadows(d, page_to_mfn(page));
>>  }
>>  
>> +/*
>> + * Removes v->arch.paging.shadow.shadow_table[].
>> + * Does all appropriate management/bookkeeping/refcounting/etc...
>> + */
>> +static void sh_detach_old_tables(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +    unsigned int i;
>> +
>> +    ////
>> +    //// vcpu->arch.paging.shadow.shadow_table[]
>> +    ////
> 
> Honestly, I don't see what the point of this comment is at all.  I'd
> suggest just dropping it as you move the function, which avoids the need
> to debate over C++ comments.

As said in the remark, this style of comments is used elsewhere as well,
to indicate what data structure a certain piece of code in a function is
updating. Earlier on the function here also played with
vcpu->arch.paging.shadow.guest_vtable, at which point having such comments
was certainly not entirely useless.

> Preferably with this done, Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Thanks. I guess I'll drop it then; should the function become more
involved again, we could clearly resurrect comments in whatever shape is
then deemed best.

Jan
diff mbox series

Patch

--- a/xen/arch/x86/include/asm/paging.h
+++ b/xen/arch/x86/include/asm/paging.h
@@ -98,7 +98,6 @@ 
 
 struct shadow_paging_mode {
 #ifdef CONFIG_SHADOW_PAGING
-    void          (*detach_old_tables     )(struct vcpu *v);
 #ifdef CONFIG_PV
     void          (*write_guest_entry     )(struct vcpu *v, intpte_t *p,
                                             intpte_t new, mfn_t gmfn);
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2264,6 +2264,29 @@  void shadow_prepare_page_type_change(str
     shadow_remove_all_shadows(d, page_to_mfn(page));
 }
 
+/*
+ * Removes v->arch.paging.shadow.shadow_table[].
+ * Does all appropriate management/bookkeeping/refcounting/etc...
+ */
+static void sh_detach_old_tables(struct vcpu *v)
+{
+    struct domain *d = v->domain;
+    unsigned int i;
+
+    ////
+    //// vcpu->arch.paging.shadow.shadow_table[]
+    ////
+
+    for ( i = 0; i < ARRAY_SIZE(v->arch.paging.shadow.shadow_table); ++i )
+    {
+        mfn_t smfn = pagetable_get_mfn(v->arch.paging.shadow.shadow_table[i]);
+
+        if ( mfn_x(smfn) )
+            sh_put_ref(d, smfn, 0);
+        v->arch.paging.shadow.shadow_table[i] = pagetable_null();
+    }
+}
+
 /**************************************************************************/
 
 static void sh_update_paging_modes(struct vcpu *v)
@@ -2312,7 +2335,7 @@  static void sh_update_paging_modes(struc
     // First, tear down any old shadow tables held by this vcpu.
     //
     if ( v->arch.paging.mode )
-        v->arch.paging.mode->shadow.detach_old_tables(v);
+        sh_detach_old_tables(v);
 
 #ifdef CONFIG_HVM
     if ( is_hvm_domain(d) )
@@ -2700,7 +2723,7 @@  void shadow_vcpu_teardown(struct vcpu *v
     if ( !paging_mode_shadow(d) || !v->arch.paging.mode )
         goto out;
 
-    v->arch.paging.mode->shadow.detach_old_tables(v);
+    sh_detach_old_tables(v);
 #ifdef CONFIG_HVM
     if ( shadow_mode_external(d) )
     {
@@ -2935,7 +2958,7 @@  static int shadow_one_bit_disable(struct
         for_each_vcpu(d, v)
         {
             if ( v->arch.paging.mode )
-                v->arch.paging.mode->shadow.detach_old_tables(v);
+                sh_detach_old_tables(v);
             if ( !(v->arch.flags & TF_kernel_mode) )
                 make_cr3(v, pagetable_get_mfn(v->arch.guest_table_user));
             else
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3207,30 +3207,6 @@  sh_update_linear_entries(struct vcpu *v)
     sh_flush_local(d);
 }
 
-
-/*
- * Removes v->arch.paging.shadow.shadow_table[].
- * Does all appropriate management/bookkeeping/refcounting/etc...
- */
-static void cf_check sh_detach_old_tables(struct vcpu *v)
-{
-    struct domain *d = v->domain;
-    mfn_t smfn;
-    unsigned int i;
-
-    ////
-    //// vcpu->arch.paging.shadow.shadow_table[]
-    ////
-
-    for_each_shadow_table(v, i)
-    {
-        smfn = pagetable_get_mfn(v->arch.paging.shadow.shadow_table[i]);
-        if ( mfn_x(smfn) )
-            sh_put_ref(d, smfn, 0);
-        v->arch.paging.shadow.shadow_table[i] = pagetable_null();
-    }
-}
-
 static void cf_check sh_update_cr3(struct vcpu *v, int do_locking, bool noflush)
 /* Updates vcpu->arch.cr3 after the guest has changed CR3.
  * Paravirtual guests should set v->arch.guest_table (and guest_table_user,
@@ -4211,7 +4187,6 @@  const struct paging_mode sh_paging_mode
     .update_paging_modes           = shadow_update_paging_modes,
     .flush_tlb                     = shadow_flush_tlb,
     .guest_levels                  = GUEST_PAGING_LEVELS,
-    .shadow.detach_old_tables      = sh_detach_old_tables,
 #ifdef CONFIG_PV
     .shadow.write_guest_entry      = sh_write_guest_entry,
     .shadow.cmpxchg_guest_entry    = sh_cmpxchg_guest_entry,
--- a/xen/arch/x86/mm/shadow/types.h
+++ b/xen/arch/x86/mm/shadow/types.h
@@ -236,7 +236,6 @@  static inline shadow_l4e_t shadow_l4e_fr
 #define sh_unhook_pae_mappings     INTERNAL_NAME(sh_unhook_pae_mappings)
 #define sh_unhook_64b_mappings     INTERNAL_NAME(sh_unhook_64b_mappings)
 #define sh_paging_mode             INTERNAL_NAME(sh_paging_mode)
-#define sh_detach_old_tables       INTERNAL_NAME(sh_detach_old_tables)
 #define sh_audit_l1_table          INTERNAL_NAME(sh_audit_l1_table)
 #define sh_audit_fl1_table         INTERNAL_NAME(sh_audit_fl1_table)
 #define sh_audit_l2_table          INTERNAL_NAME(sh_audit_l2_table)