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

[Juergen Gross <jgross@xxxxxxxx>]

On 19/04/18 09:39, Jan Beulich wrote:
>>>> On 19.04.18 at 08:19, <jgross@xxxxxxxx> wrote:
>> On 18/04/18 18:12, Jan Beulich wrote:
>>>>>> On 18.04.18 at 10:30, <jgross@xxxxxxxx> wrote:
>>>> @@ -160,5 +161,20 @@ unsigned int flush_area_local(const void *va, 
>>>> unsigned int flags)
>>>>  
>>>>      local_irq_restore(irqfl);
>>>>  
>>>> +    if ( flags & FLUSH_ROOT_PGTBL )
>>>> +        get_cpu_info()->root_pgt_changed = true;
>>>> +
>>>>      return flags;
>>>>  }
>>>> +
>>>> +void flush_root_pgt_mask(cpumask_t *mask)
>>>
>>> const
>>>
>>>> +{
>>>> +    int cpu;
>>>
>>> unsigned int
>>>
>>>> +    struct cpu_info *cpu_info;
>>>
>>> Declaration in most narrow possible scope please.
>>>
>>>> +    for_each_cpu(cpu, mask)
>>>> +    {
>>>> +        cpu_info = (struct cpu_info *)(stack_base[cpu] + STACK_SIZE) - 1;
>>>> +        cpu_info->root_pgt_changed = true;
>>>> +    }
>>>> +}
>>>
>>> So why is this not sending an IPI to trigger the FLUSH_ROOT_PGTBL processing
>>> visible above?
>>
>> It is not needed for the case this function was introduced. Maybe I
>> should rename it to flush_root_pgt_mask_lazy() ?
>>
>>> Especially the cast you need here looks quite undesirable.
>>
>> In get_cpu_info() it is done the same way. I can add a common helper
>> function to current.h
> 
> While aiui moot with the below, a common helper would indeed be the
> way to go if we really needed such a cross-CPU access.
> 
>>> Plus I
>>> think this is racy (even if quite unlikely to trigger) with a CPU being 
>>> brought down:
>>> What if stack_base[cpu] becomes NULL after you've found the respective bit 
>>> in
>>> mask set. Especially when Xen runs itself virtualized, (almost) arbitrarily 
>>> long
>>> periods of time may pass between any two instructions.
>>
>> Right.
>>
>> So either I'm adding some kind of locking/rcu, or I'm switching to use
>> IPIs and access root_pgt_changed only locally.
>>
>> Do you have any preference?
> 
> Since issuing an IPI is just a single call, I'd prefer not to have new 
> (locking,
> rcu, or whatever else) logic added here. Unless of course someone, in
> particular Tim, thinks sending an IPI here is a rather bad idea.

Okay.

Another alternative would be to pass another flag to the callers to
signal the need for a flush. This would require quite some modifications
to shadow code I'd like to avoid, though. OTOH this way we could combine
flushing the tlb and the root page tables. Tim, any preferences?


Juergen


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