Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries
>>> On 09.02.18 at 13:35, wrote:
> On 30/01/18 16:40, Jan Beulich wrote:
> On 22.01.18 at 13:32, wrote:
>>> @@ -37,10 +52,24 @@ struct vcpu;
>>>
>>> struct cpu_info {
>>> struct cpu_user_regs guest_cpu_user_regs;
>>> -unsigned int processor_id;
>>> -struct vcpu *current_vcpu;
>>> -unsigned long per_cpu_offset;
>>> -unsigned long cr4;
>>> +union {
>>> +/* per physical cpu mapping */
>>> +struct {
>>> +struct vcpu *current_vcpu;
>>> +unsigned long per_cpu_offset;
>>> +unsigned long cr4;
>>> +};
>>> +/* per vcpu mapping (xpti) */
>>> +struct {
>>> +unsigned long pad1;
>>> +unsigned long pad2;
>>> +unsigned long stack_bottom_cpu;
>>> +};
>>
>> In order to avoid accidental use in the wrong context as much as
>> possible, I think you want to name both structures.
>
> I'd like to leave it as is in order to make a possible backport much
> more easier.
Well, I can see why you would want the pre-existing fields left
without structure field name, but the new (vcpu) ones? And
even the pre-existing (pcpu) ones should gain a name, just
perhaps in a patch late in the series, which then wouldn't be
backported.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries
On 30/01/18 16:40, Jan Beulich wrote:
On 22.01.18 at 13:32, wrote:
>> @@ -37,10 +52,24 @@ struct vcpu;
>>
>> struct cpu_info {
>> struct cpu_user_regs guest_cpu_user_regs;
>> -unsigned int processor_id;
>> -struct vcpu *current_vcpu;
>> -unsigned long per_cpu_offset;
>> -unsigned long cr4;
>> +union {
>> +/* per physical cpu mapping */
>> +struct {
>> +struct vcpu *current_vcpu;
>> +unsigned long per_cpu_offset;
>> +unsigned long cr4;
>> +};
>> +/* per vcpu mapping (xpti) */
>> +struct {
>> +unsigned long pad1;
>> +unsigned long pad2;
>> +unsigned long stack_bottom_cpu;
>> +};
>
> In order to avoid accidental use in the wrong context as much as
> possible, I think you want to name both structures.
I'd like to leave it as is in order to make a possible backport much
more easier.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries
>>> On 30.01.18 at 18:12, wrote:
> On 30/01/18 16:40, Jan Beulich wrote:
> On 22.01.18 at 13:32, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries
On 30/01/18 16:40, Jan Beulich wrote:
On 22.01.18 at 13:32, wrote:
>> In case of XPTI being active for a pv-domain allocate and initialize
>> per-vcpu stacks. The stacks are added to the per-domain mappings of
>> the pv-domain.
>
> Considering the intended use of these stacks (as per the overview
> mail) I consider 32k per vCPU a non-negligible amount of extra memory
> use.
Maybe I can shrink this by putting multiple entry stacks into one page.
In the end I only need struct cpu_info and maybe some spare for each
stack.
>
>> +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).
>
>> +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.
>
>> +/* 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.
>
> Also I would assume the TSS can be mapped r/o.
Right.
>
>> +stack_bottom = (unsigned long)&info->guest_cpu_user_regs.es;
>> +ptr = __map_domain_page(pg);
>> +tss_init(ptr, stack_bottom);
>> +unmap_domain_page(ptr);
>> +
>> +/* Map stub trampolines. */
>> +rc = create_perdomain_mapping(d, XPTI_TRAMPOLINE(v), 1, NULL, &pg);
>> +if ( rc )
>> +goto done;
>> +ptr = __map_domain_page(pg);
>> +write_stub_trampoline((unsigned char *)ptr, XPTI_TRAMPOLINE(v),
>
> I would be very surprised if you really needed the cast here.
Oh, this is a leftover from a previous version where ptr was char *.
>
>> @@ -25,6 +25,21 @@
>> */
>>
>> /*
>> + * The vcpu stacks used for XPTI are arranged similar to the physical cpu
>> + * stacks with some modifications. The main difference are the primary stack
>> + * size (only 1 page) and usage of the unused mappings for TSS and IDT.
>> + *
>> + * 7 - Primary stack (with a struct cpu_info at the top)
>> + * 6 - unused
>> + * 5 - TSS
>
> Judging by the comment this might mean "TSS / IDT", or slots 4 or 6
> might be used for the IDT. Otoh I don't see any IDT related logic in
> pv_vcpu_init_xpti(). Please clarify this.
Oh yes. I'll remove the IDT related comments, as I think I can just map
the original IDT.
>
>> @@ -37,10 +52,24 @@ struct vcpu;
>>
>> struct cpu_info {
>> struct cpu_user_regs guest_cpu_user_regs;
>> -unsigned int processor_id;
>> -struct vcpu *current_vcpu;
>> -unsigned long per_cpu_offset;
>> -unsigned long cr4;
>> +union {
>> +/* per physical cpu mapping */
>> +struct {
>> +struct vcpu *current_vcpu;
>> +unsigned long per_cpu_offset;
>> +unsigned long cr4;
>> +};
>> +/* per vcpu mapping (xpti) */
>> +struct {
>> +unsigned long pad1;
>> +unsigned long pad2;
>> +unsigned long stack_bottom_cpu;
>> +};
>
> In order to avoid accidental use in the wrong context as much as
> possible, I think you want to name both structures.
Okay.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC v2 10/12] x86: allocate per-vcpu stacks for interrupt entries
>>> On 22.01.18 at 13:32, wrote:
> In case of XPTI being active for a pv-domain allocate and initialize
> per-vcpu stacks. The stacks are added to the per-domain mappings of
> the pv-domain.
Considering the intended use of these stacks (as per the overview
mail) I consider 32k per vCPU a non-negligible amount of extra memory
use.
> +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 ...
> +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.
> +/* 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.
Also I would assume the TSS can be mapped r/o.
> +stack_bottom = (unsigned long)&info->guest_cpu_user_regs.es;
> +ptr = __map_domain_page(pg);
> +tss_init(ptr, stack_bottom);
> +unmap_domain_page(ptr);
> +
> +/* Map stub trampolines. */
> +rc = create_perdomain_mapping(d, XPTI_TRAMPOLINE(v), 1, NULL, &pg);
> +if ( rc )
> +goto done;
> +ptr = __map_domain_page(pg);
> +write_stub_trampoline((unsigned char *)ptr, XPTI_TRAMPOLINE(v),
I would be very surprised if you really needed the cast here.
> @@ -25,6 +25,21 @@
> */
>
> /*
> + * The vcpu stacks used for XPTI are arranged similar to the physical cpu
> + * stacks with some modifications. The main difference are the primary stack
> + * size (only 1 page) and usage of the unused mappings for TSS and IDT.
> + *
> + * 7 - Primary stack (with a struct cpu_info at the top)
> + * 6 - unused
> + * 5 - TSS
Judging by the comment this might mean "TSS / IDT", or slots 4 or 6
might be used for the IDT. Otoh I don't see any IDT related logic in
pv_vcpu_init_xpti(). Please clarify this.
> @@ -37,10 +52,24 @@ struct vcpu;
>
> struct cpu_info {
> struct cpu_user_regs guest_cpu_user_regs;
> -unsigned int processor_id;
> -struct vcpu *current_vcpu;
> -unsigned long per_cpu_offset;
> -unsigned long cr4;
> +union {
> +/* per physical cpu mapping */
> +struct {
> +struct vcpu *current_vcpu;
> +unsigned long per_cpu_offset;
> +unsigned long cr4;
> +};
> +/* per vcpu mapping (xpti) */
> +struct {
> +unsigned long pad1;
> +unsigned long pad2;
> +unsigned long stack_bottom_cpu;
> +};
In order to avoid accidental use in the wrong context as much as
possible, I think you want to name both structures.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
