Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.18 at 18:12, <jgross@xxxxxxxx> wrote:
> On 30/01/18 16:40, Jan Beulich wrote:
>>>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
>>> +static int pv_vcpu_init_xpti(struct vcpu *v)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    struct page_info *pg;
>>> +    void *ptr;
>>> +    struct cpu_info *info;
>>> +    unsigned long stack_bottom;
>>> +    int rc;
>>> +
>>> +    /* Populate page tables. */
>>> +    rc = create_perdomain_mapping(d, XPTI_START(v), STACK_PAGES,
>>> +                                  NIL(l1_pgentry_t *), NULL);
>>> +    if ( rc )
>>> +        goto done;
>>> +
>>> +    /* Map stacks. */
>>> +    rc = create_perdomain_mapping(d, XPTI_START(v), IST_MAX,
>>> +                                  NULL, NIL(struct page_info *));
>>> +    if ( rc )
>>> +        goto done;
>>> +
>>> +    ptr = alloc_xenheap_page();
>>> +    if ( !ptr )
>>> +    {
>>> +        rc = -ENOMEM;
>>> +        goto done;
>>> +    }
>>> +    clear_page(ptr);
>>> +    addmfn_to_perdomain_mapping(d, XPTI_START(v) + STACK_SIZE - PAGE_SIZE,
>>> +                                _mfn(virt_to_mfn(ptr)));
>> 
>> This can't be create_perdomain_mapping() because of ...? If it's
>> the Xen heap page you use here - that would be the next question:
>> Does it need to be such, rather than a domheap one? I do see ...
> 
> I need to reference the user regs in __context_switch() before
> switching to the new address space (otherwise I'd had to rework
> __context_switch() which I wanted to avoid).

And a suitably mapped domain-heap page won't do?

>>> +    info = (struct cpu_info *)((unsigned long)ptr + PAGE_SIZE) - 1;
>>> +    info->flags = ON_VCPUSTACK;
>>> +    v->arch.pv_vcpu.stack_regs = &info->guest_cpu_user_regs;
>> 
>> ... this pointer, but without a clear picture on intended use it's
>> hard to judge.
> 
> See patch 12.

Well, that's one of the big problems with this RFC: The overview
mail doesn't give a clear picture of the intended overall changes
(including ones yet to be submitted), and individual patches rely
on the reader to pull out information from later patches to
understand what the current patch one is looking at does.

>>> +    /* Map TSS. */
>>> +    rc = create_perdomain_mapping(d, XPTI_TSS(v), 1, NULL, &pg);
>>> +    if ( rc )
>>> +        goto done;
>>> +    info = (struct cpu_info *)(XPTI_START(v) + STACK_SIZE) - 1;
>> 
>> Iiuc this is a pointer one absolutely must not de-reference. A bit
>> dangerous, I would say, the more that further up the same
>> variable is being de-referenced.
> 
> Okay, I'll add another variable for this purpose.

Or at least add a comment clearly stating the restriction.

Jan


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