On 31/01/18 11:36, Jan Beulich wrote:
>>>> On 30.01.18 at 18:19, <jgr...@suse.com> wrote:
>> On 30/01/18 17:07, Jan Beulich wrote:
>>>>>> On 22.01.18 at 13:32, <jgr...@suse.com> 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
>> I tried using the C constants but this led to rather nasty include
> 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
>>>> "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
>>>> - 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.
Xen-devel mailing list