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

[Juergen Gross <jgross@xxxxxxxx>]

On 31/01/18 11:36, Jan Beulich wrote:
>>>> 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.

Some #ifndef __ASSEMBLY__ made it work.

I think I had the defines in another header in the beginning and just
didn't switch back after moving them to current.h.

> 
>> 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.

Okay, added.


Juergen

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