Re: [Xen-devel] [PATCH v3 1/7] x86/xpti: avoid copying L4 page table contents when possible

[Juergen Gross <jgross@xxxxxxxx>]

On 22/03/18 15:31, Jan Beulich wrote:
>>>> On 21.03.18 at 13:51, <jgross@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -158,6 +158,9 @@ unsigned int flush_area_local(const void *va, unsigned 
>> int flags)
>>          }
>>      }
>>  
>> +    if ( flags & FLUSH_ROOT_PGTBL )
>> +        get_cpu_info()->root_pgt_changed = true;
>> +
>>      local_irq_restore(irqfl);
>>  
>>      return flags;
> 
> Does this really need to sit inside the interrupts disabled section?

Hmm, no, I don't think so. I'll move it below local_irq_restore().

> Thinking about it I even wonder whether the cache flush part needs
> to be. Even for the INVLPG portion of the TLB flush part I can't
> seem to see a need for IRQs to be off. I think it's really just the
> pre_flush() / post_flush() pair which needs to be inside such a
> section. I'll prepare a patch (for after 4.11). I think some of the
> changes later in your series will actually further ease this.
> 
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -499,10 +499,15 @@ void free_shared_domheap_page(struct page_info *page)
>>  void make_cr3(struct vcpu *v, mfn_t mfn)
>>  {
>>      v->arch.cr3 = mfn_x(mfn) << PAGE_SHIFT;
>> +    if ( v == current && this_cpu(root_pgt) && is_pv_vcpu(v) &&
>> +         !is_pv_32bit_vcpu(v) )
>> +        get_cpu_info()->root_pgt_changed = true;
>>  }
> 
> As this doesn't actually update CR3, setting the flag shouldn't
> generally be necessary if the caller then invokes write_ptbase().
> Isn't setting the flag here needed solely in the case of
> _toggle_guest_pt() being up the call tree? In which case it would
> perhaps better be set there (and in turn some or even all of the
> conditional around it could be dropped)?

Yes, you are right.

> 
>>  void write_ptbase(struct vcpu *v)
>>  {
>> +    if ( this_cpu(root_pgt) && is_pv_vcpu(v) && !is_pv_32bit_vcpu(v) )
>> +        get_cpu_info()->root_pgt_changed = true;
>>      write_cr3(v->arch.cr3);
> 
> When you come here from e.g. __sync_local_execstate(), you
> don't really need to set the flag. Of course you'll come here again
> before the next 64-bit PV vCPU will make it to restore_all_guest,
> so by the time we make it there the flag will be set anyway.
> However, if you already use such a subtlety, then there's also
> no point excluding 32-bit vCPU-s here (nor in make_cr3()), as
> those will never make it to restore_all_guest. Same then for
> excluding HVM vCPU-s. And I then wonder whether (here or
> more likely in a later patch) the root_pgt check couldn't go away
> as well.

I'm not sure this is worth it. Patch 3 will re-introduce a conditional
here and it will look rather different (e.g. without the root_pgt
check). So micro-optimizing this patch barely makes any sense.

> 
>> @@ -3698,18 +3703,29 @@ long do_mmu_update(
>>                          break;
>>                      rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
>>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> -                    /*
>> -                     * No need to sync if all uses of the page can be 
>> accounted
>> -                     * to the page lock we hold, its pinned status, and 
>> uses on
>> -                     * this (v)CPU.
>> -                     */
>> -                    if ( !rc && !cpu_has_no_xpti &&
>> -                         ((page->u.inuse.type_info & PGT_count_mask) >
>> -                          (1 + !!(page->u.inuse.type_info & PGT_pinned) +
>> -                           (pagetable_get_pfn(curr->arch.guest_table) == 
>> mfn) 
>> +
>> -                           (pagetable_get_pfn(curr->arch.guest_table_user) 
>> ==
>> -                            mfn))) )
>> -                        sync_guest = true;
>> +                    if ( !rc && !cpu_has_no_xpti )
>> +                    {
>> +                        bool local_in_use = false;
>> +
>> +                        if ( (pagetable_get_pfn(curr->arch.guest_table) ==
>> +                              mfn) ||
>> +                             
>> (pagetable_get_pfn(curr->arch.guest_table_user) ==
>> +                              mfn) )
>> +                        {
>> +                            local_in_use = true;
>> +                            get_cpu_info()->root_pgt_changed = true;
>> +                        }
> 
> The conditional causes root_pgt_changed to get set even in cases
> where what CR3 points to doesn't actually change (if it's the user
> page tables that get modified). I think you want to check
> curr->arch.cr3 here, or only curr->arch.guest_table (as user mode
> can't invoke hypercalls).

I'll go with curr->arch.guest_table.

> 
>> +                        /*
>> +                         * No need to sync if all uses of the page can be
>> +                         * accounted to the page lock we hold, its pinned
>> +                         * status, and uses on this (v)CPU.
>> +                         */
>> +                        if ( (page->u.inuse.type_info & PGT_count_mask) >
>> +                             (1 + !!(page->u.inuse.type_info & PGT_pinned) +
>> +                              local_in_use) )
> 
> The boolean local_in_use evaluates to 1 here, when previously the
> value could have been 1 or 2 (I agree that's highly theoretical, but
> anyway). Of course this will be addressed implicitly if you check
> (only) curr->arch.guest_table above and move the
> curr->arch.guest_table_user check here.

Yes.


Juergen

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