Re: [Xen-devel] [PATCH RFC v2 11/12] x86: modify interrupt handlers to support stack switching

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.18 at 18:19, <jgross@xxxxxxxx> wrote:
> On 30/01/18 17:07, Jan Beulich wrote:
>>>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
>>> --- a/xen/arch/x86/x86_64/asm-offsets.c
>>> +++ b/xen/arch/x86/x86_64/asm-offsets.c
>>> @@ -137,6 +137,10 @@ void __dummy__(void)
>>>      OFFSET(CPUINFO_processor_id, struct cpu_info, processor_id);
>>>      OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
>>>      OFFSET(CPUINFO_cr4, struct cpu_info, cr4);
>>> +    OFFSET(CPUINFO_stack_bottom_cpu, struct cpu_info, stack_bottom_cpu);
>>> +    OFFSET(CPUINFO_flags, struct cpu_info, flags);
>>> +    DEFINE(ASM_ON_VCPUSTACK, ON_VCPUSTACK);
>>> +    DEFINE(ASM_VCPUSTACK_ACTIVE, VCPUSTACK_ACTIVE);
>> 
>> Seeing their uses in asm_defns.h it's not really clear to me why
>> you can't use the C constants there, the more that those uses
>> are inside C macros (which perhaps would better be assembler
>> ones). The latter doesn't even appear to be used in assembly
>> code.
> 
> I tried using the C constants but this led to rather nasty include
> dependencies.

Hmm, I can imagine this to be the case, but I'd like to have more
detail for justification. current.h itself doesn't have that many
dependencies, and if half-way reasonable disentangling our
headers may be the better choice.

> ASM_VCPUSTACK_ACTIVE will be used when %cr3 switching is being added.

Please introduce it when needed.

>>> --- a/xen/common/wait.c
>>> +++ b/xen/common/wait.c
>>> @@ -122,10 +122,10 @@ void wake_up_all(struct waitqueue_head *wq)
>>>  
>>>  static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>>>  {
>>> -    struct cpu_info *cpu_info = get_cpu_info();
>>> +    struct cpu_user_regs *user_regs = guest_cpu_user_regs();
>>>      struct vcpu *curr = current;
>>>      unsigned long dummy;
>>> -    u32 entry_vector = cpu_info->guest_cpu_user_regs.entry_vector;
>>> +    u32 entry_vector = user_regs->entry_vector;
>>>  
>>>      ASSERT(wqv->esp == 0);
>>>  
>>> @@ -160,7 +160,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu 
>>> *wqv)
>>>          "pop %%r11; pop %%r10; pop %%r9;  pop %%r8;"
>>>          "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
>>>          : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
>>> -        : "i" (PAGE_SIZE), "0" (0), "1" (cpu_info), "2" (wqv->stack)
>>> +        : "i" (PAGE_SIZE), "0" (0), "1" (user_regs), "2" (wqv->stack)
>>>          : "memory" );
>>>  
>>>      if ( unlikely(wqv->esp == 0) )
>>> @@ -169,7 +169,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu 
>>> *wqv)
>>>          domain_crash_synchronous();
>>>      }
>>>  
>>> -    cpu_info->guest_cpu_user_regs.entry_vector = entry_vector;
>>> +    user_regs->entry_vector = entry_vector;
>>>  }
>> 
>> I don't see how this change is related to the purpose of this patch,
>> or why the change is needed. All you do is utilize that
>> guest_cpu_user_regs is the first field of struct cpu_info afaics.
> 
> guest_cpu_user_regs() might point to either stack, while get_cpu_info()
> will always reference the Xen stack and never the per-vcpu one.

Then the description should say so for justification.

Jan


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