struct vcpu_guest_core_reg stable ABI?

struct vcpu_guest_core_reg stable ABI?

[Julien Grall]

Hi all,

It looks like the structures vcpu_guest_core_regs and vcpu_guest_context 
does not correctly reflect the AArch64 state. For instance, all Arm64 
system registers (e.g sctlr, cpsr, spsr_el1) should be 64-bit wide not 
32-bit wide.

On ARMv8.0, some of the registers have only the low 32-bit defined, the 
rest is RES0. RES0 only means they are reserved for future use, it does 
not mean they can be ignored. Newer revision (such as ARMv8.0-SSBS) 
actually began to define bit in the top 32-bit.

This means that the structures vcpu_guest_core_regs and 
vcpu_guest_context would not be able to store the top 32-bit and 
therefore misrepresenting the hardware.

 From my understanding, vcpu_guest_context is defined between the tools 
and Xen. So it would be possible to modify it without caring on backward 
compatibly.

Howerver, struct vcpu_guest_core_reg seems to be outside of the #ifdef. 
So I assume it is part of the stable ABI. Am I correct?

Do you have any suggestion how this could safely be extended?

Cheers,

-- 
Julien Grall

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

[Xen-devel] struct vcpu_guest_core_reg stable ABI?

[Julien Grall]

Hi all,

It looks like the structures vcpu_guest_core_regs and vcpu_guest_context 
does not correctly reflect the AArch64 state. For instance, all Arm64 
system registers (e.g sctlr, cpsr, spsr_el1) should be 64-bit wide not 
32-bit wide.

On ARMv8.0, some of the registers have only the low 32-bit defined, the 
rest is RES0. RES0 only means they are reserved for future use, it does 
not mean they can be ignored. Newer revision (such as ARMv8.0-SSBS) 
actually began to define bit in the top 32-bit.

This means that the structures vcpu_guest_core_regs and 
vcpu_guest_context would not be able to store the top 32-bit and 
therefore misrepresenting the hardware.

 From my understanding, vcpu_guest_context is defined between the tools 
and Xen. So it would be possible to modify it without caring on backward 
compatibly.

Howerver, struct vcpu_guest_core_reg seems to be outside of the #ifdef. 
So I assume it is part of the stable ABI. Am I correct?

Do you have any suggestion how this could safely be extended?

Cheers,

-- 
Julien Grall

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

Re: struct vcpu_guest_core_reg stable ABI?

[Andrew Cooper]

On 15/05/2019 20:58, Julien Grall wrote:
> Hi all,
>
> It looks like the structures vcpu_guest_core_regs and
> vcpu_guest_context does not correctly reflect the AArch64 state. For
> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
> should be 64-bit wide not 32-bit wide.
>
> On ARMv8.0, some of the registers have only the low 32-bit defined,
> the rest is RES0. RES0 only means they are reserved for future use, it
> does not mean they can be ignored. Newer revision (such as
> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>
> This means that the structures vcpu_guest_core_regs and
> vcpu_guest_context would not be able to store the top 32-bit and
> therefore misrepresenting the hardware.
>
> From my understanding, vcpu_guest_context is defined between the tools
> and Xen. So it would be possible to modify it without caring on
> backward compatibly.
>
> Howerver, struct vcpu_guest_core_reg seems to be outside of the
> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>
> Do you have any suggestion how this could safely be extended?

Stuff like this should never have been in the public API to begin with. 
x86 has some nasty issues which I have yet to find a good-enough way to fix.

For ARM, and future architectures, I'd use the fact that there are no
non-tools interfaces which use this structure to allow yourself the
wiggleroom to declare history a mistake, and fix it by making it tools-only.

No point lumbering yourself with a broke structure in the API if you can
avoid it.

~Andrew

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

Re: [Xen-devel] struct vcpu_guest_core_reg stable ABI?

[Andrew Cooper]

On 15/05/2019 20:58, Julien Grall wrote:
> Hi all,
>
> It looks like the structures vcpu_guest_core_regs and
> vcpu_guest_context does not correctly reflect the AArch64 state. For
> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
> should be 64-bit wide not 32-bit wide.
>
> On ARMv8.0, some of the registers have only the low 32-bit defined,
> the rest is RES0. RES0 only means they are reserved for future use, it
> does not mean they can be ignored. Newer revision (such as
> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>
> This means that the structures vcpu_guest_core_regs and
> vcpu_guest_context would not be able to store the top 32-bit and
> therefore misrepresenting the hardware.
>
> From my understanding, vcpu_guest_context is defined between the tools
> and Xen. So it would be possible to modify it without caring on
> backward compatibly.
>
> Howerver, struct vcpu_guest_core_reg seems to be outside of the
> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>
> Do you have any suggestion how this could safely be extended?

Stuff like this should never have been in the public API to begin with. 
x86 has some nasty issues which I have yet to find a good-enough way to fix.

For ARM, and future architectures, I'd use the fact that there are no
non-tools interfaces which use this structure to allow yourself the
wiggleroom to declare history a mistake, and fix it by making it tools-only.

No point lumbering yourself with a broke structure in the API if you can
avoid it.

~Andrew

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

Re: struct vcpu_guest_core_reg stable ABI?

[Jan Beulich]

>>> On 15.05.19 at 22:12, <andrew.cooper3@citrix.com> wrote:
> On 15/05/2019 20:58, Julien Grall wrote:
>> Hi all,
>>
>> It looks like the structures vcpu_guest_core_regs and
>> vcpu_guest_context does not correctly reflect the AArch64 state. For
>> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
>> should be 64-bit wide not 32-bit wide.
>>
>> On ARMv8.0, some of the registers have only the low 32-bit defined,
>> the rest is RES0. RES0 only means they are reserved for future use, it
>> does not mean they can be ignored. Newer revision (such as
>> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>>
>> This means that the structures vcpu_guest_core_regs and
>> vcpu_guest_context would not be able to store the top 32-bit and
>> therefore misrepresenting the hardware.
>>
>> From my understanding, vcpu_guest_context is defined between the tools
>> and Xen. So it would be possible to modify it without caring on
>> backward compatibly.
>>
>> Howerver, struct vcpu_guest_core_reg seems to be outside of the
>> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>>
>> Do you have any suggestion how this could safely be extended?
> 
> Stuff like this should never have been in the public API to begin with. 
> x86 has some nasty issues which I have yet to find a good-enough way to fix.
> 
> For ARM, and future architectures, I'd use the fact that there are no
> non-tools interfaces which use this structure to allow yourself the
> wiggleroom to declare history a mistake, and fix it by making it tools-only.

I'm unconvinced of the "declare history a mistake" part, but I agree
with the suggestion of simply moving the structure declaration down
into the guarded area. It simply was a mistake to not put it there in
the first place.

For x86 PV at least I don't really see how we could have gone
without exposing this - we have to allow guests to specify at least
some of a to-be-brought-up-vCPU's registers. Anything else
wouldn't really have been PV anymore. For PVH this may have
been avoidable.

Jan



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

Re: [Xen-devel] struct vcpu_guest_core_reg stable ABI?

[Jan Beulich]

>>> On 15.05.19 at 22:12, <andrew.cooper3@citrix.com> wrote:
> On 15/05/2019 20:58, Julien Grall wrote:
>> Hi all,
>>
>> It looks like the structures vcpu_guest_core_regs and
>> vcpu_guest_context does not correctly reflect the AArch64 state. For
>> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
>> should be 64-bit wide not 32-bit wide.
>>
>> On ARMv8.0, some of the registers have only the low 32-bit defined,
>> the rest is RES0. RES0 only means they are reserved for future use, it
>> does not mean they can be ignored. Newer revision (such as
>> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>>
>> This means that the structures vcpu_guest_core_regs and
>> vcpu_guest_context would not be able to store the top 32-bit and
>> therefore misrepresenting the hardware.
>>
>> From my understanding, vcpu_guest_context is defined between the tools
>> and Xen. So it would be possible to modify it without caring on
>> backward compatibly.
>>
>> Howerver, struct vcpu_guest_core_reg seems to be outside of the
>> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>>
>> Do you have any suggestion how this could safely be extended?
> 
> Stuff like this should never have been in the public API to begin with. 
> x86 has some nasty issues which I have yet to find a good-enough way to fix.
> 
> For ARM, and future architectures, I'd use the fact that there are no
> non-tools interfaces which use this structure to allow yourself the
> wiggleroom to declare history a mistake, and fix it by making it tools-only.

I'm unconvinced of the "declare history a mistake" part, but I agree
with the suggestion of simply moving the structure declaration down
into the guarded area. It simply was a mistake to not put it there in
the first place.

For x86 PV at least I don't really see how we could have gone
without exposing this - we have to allow guests to specify at least
some of a to-be-brought-up-vCPU's registers. Anything else
wouldn't really have been PV anymore. For PVH this may have
been avoidable.

Jan



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

Re: struct vcpu_guest_core_reg stable ABI?

[Andrew Cooper]

On 16/05/2019 08:58, Jan Beulich wrote:
>>>> On 15.05.19 at 22:12, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/2019 20:58, Julien Grall wrote:
>>> Hi all,
>>>
>>> It looks like the structures vcpu_guest_core_regs and
>>> vcpu_guest_context does not correctly reflect the AArch64 state. For
>>> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
>>> should be 64-bit wide not 32-bit wide.
>>>
>>> On ARMv8.0, some of the registers have only the low 32-bit defined,
>>> the rest is RES0. RES0 only means they are reserved for future use, it
>>> does not mean they can be ignored. Newer revision (such as
>>> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>>>
>>> This means that the structures vcpu_guest_core_regs and
>>> vcpu_guest_context would not be able to store the top 32-bit and
>>> therefore misrepresenting the hardware.
>>>
>>> From my understanding, vcpu_guest_context is defined between the tools
>>> and Xen. So it would be possible to modify it without caring on
>>> backward compatibly.
>>>
>>> Howerver, struct vcpu_guest_core_reg seems to be outside of the
>>> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>>>
>>> Do you have any suggestion how this could safely be extended?
>> Stuff like this should never have been in the public API to begin with. 
>> x86 has some nasty issues which I have yet to find a good-enough way to fix.
>>
>> For ARM, and future architectures, I'd use the fact that there are no
>> non-tools interfaces which use this structure to allow yourself the
>> wiggleroom to declare history a mistake, and fix it by making it tools-only.
> I'm unconvinced of the "declare history a mistake" part, but I agree
> with the suggestion of simply moving the structure declaration down
> into the guarded area. It simply was a mistake to not put it there in
> the first place.
>
> For x86 PV at least I don't really see how we could have gone
> without exposing this - we have to allow guests to specify at least
> some of a to-be-brought-up-vCPU's registers.  Anything else
> wouldn't really have been PV anymore.

All that is necessary for PV is %rip.  %rsp would also be convenient,
but literally nothing else is needed from the guests point of view.  The
ABI already has guests starting with a flat memory layout using the
Xen-provided selectors.  Everything else can be set up  by the entry code.

The real problem x86 has is guest_user_regs being in the public API. 
This absolutely shouldn't have happened, because it fixes the larger
chunk of the top-of-stack cpu_info block in the public API.

> For PVH this may have been avoidable.

PVH (for better or worse) followed suit with PV, because PV already
existed.  This was due to the expected lack of an LAPIC in guest
context, and see where that ended up.

Do *any* OSes actually use VCPUOP_initialise for PVH guests?  I can't
think of any offhand.

Furthermore, we now have an even more tangled mess of hypercalls where
DOMCTL_{get,set}vcpucontext and VCPUOP_initialise have an asymmetric API
when issues from the toolstack.

~Andrew

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

Re: [Xen-devel] struct vcpu_guest_core_reg stable ABI?

[Andrew Cooper]

On 16/05/2019 08:58, Jan Beulich wrote:
>>>> On 15.05.19 at 22:12, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/2019 20:58, Julien Grall wrote:
>>> Hi all,
>>>
>>> It looks like the structures vcpu_guest_core_regs and
>>> vcpu_guest_context does not correctly reflect the AArch64 state. For
>>> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
>>> should be 64-bit wide not 32-bit wide.
>>>
>>> On ARMv8.0, some of the registers have only the low 32-bit defined,
>>> the rest is RES0. RES0 only means they are reserved for future use, it
>>> does not mean they can be ignored. Newer revision (such as
>>> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>>>
>>> This means that the structures vcpu_guest_core_regs and
>>> vcpu_guest_context would not be able to store the top 32-bit and
>>> therefore misrepresenting the hardware.
>>>
>>> From my understanding, vcpu_guest_context is defined between the tools
>>> and Xen. So it would be possible to modify it without caring on
>>> backward compatibly.
>>>
>>> Howerver, struct vcpu_guest_core_reg seems to be outside of the
>>> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>>>
>>> Do you have any suggestion how this could safely be extended?
>> Stuff like this should never have been in the public API to begin with. 
>> x86 has some nasty issues which I have yet to find a good-enough way to fix.
>>
>> For ARM, and future architectures, I'd use the fact that there are no
>> non-tools interfaces which use this structure to allow yourself the
>> wiggleroom to declare history a mistake, and fix it by making it tools-only.
> I'm unconvinced of the "declare history a mistake" part, but I agree
> with the suggestion of simply moving the structure declaration down
> into the guarded area. It simply was a mistake to not put it there in
> the first place.
>
> For x86 PV at least I don't really see how we could have gone
> without exposing this - we have to allow guests to specify at least
> some of a to-be-brought-up-vCPU's registers.  Anything else
> wouldn't really have been PV anymore.

All that is necessary for PV is %rip.  %rsp would also be convenient,
but literally nothing else is needed from the guests point of view.  The
ABI already has guests starting with a flat memory layout using the
Xen-provided selectors.  Everything else can be set up  by the entry code.

The real problem x86 has is guest_user_regs being in the public API. 
This absolutely shouldn't have happened, because it fixes the larger
chunk of the top-of-stack cpu_info block in the public API.

> For PVH this may have been avoidable.

PVH (for better or worse) followed suit with PV, because PV already
existed.  This was due to the expected lack of an LAPIC in guest
context, and see where that ended up.

Do *any* OSes actually use VCPUOP_initialise for PVH guests?  I can't
think of any offhand.

Furthermore, we now have an even more tangled mess of hypercalls where
DOMCTL_{get,set}vcpucontext and VCPUOP_initialise have an asymmetric API
when issues from the toolstack.

~Andrew

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

Re: struct vcpu_guest_core_reg stable ABI?

[Jan Beulich]

>>> On 16.05.19 at 11:18, <andrew.cooper3@citrix.com> wrote:
> Do *any* OSes actually use VCPUOP_initialise for PVH guests?  I can't
> think of any offhand.

If none does, why would arch_set_info_hvm_guest() have been
introduced, to carry out that op (via arch_initialise_vcpu())?
XEN_DOMCTL_setvcpucontext uses arch_set_info_guest()
uniformly after all. (The patch doing this even has your S-o-b.)

Jan



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

Re: [Xen-devel] struct vcpu_guest_core_reg stable ABI?

[Jan Beulich]

>>> On 16.05.19 at 11:18, <andrew.cooper3@citrix.com> wrote:
> Do *any* OSes actually use VCPUOP_initialise for PVH guests?  I can't
> think of any offhand.

If none does, why would arch_set_info_hvm_guest() have been
introduced, to carry out that op (via arch_initialise_vcpu())?
XEN_DOMCTL_setvcpucontext uses arch_set_info_guest()
uniformly after all. (The patch doing this even has your S-o-b.)

Jan



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

Re: struct vcpu_guest_core_reg stable ABI?

[Julien Grall]

Hi Andrew & Jan,

On 5/16/19 8:58 AM, Jan Beulich wrote:
>>>> On 15.05.19 at 22:12, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/2019 20:58, Julien Grall wrote:
>>> Hi all,
>>>
>>> It looks like the structures vcpu_guest_core_regs and
>>> vcpu_guest_context does not correctly reflect the AArch64 state. For
>>> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
>>> should be 64-bit wide not 32-bit wide.
>>>
>>> On ARMv8.0, some of the registers have only the low 32-bit defined,
>>> the rest is RES0. RES0 only means they are reserved for future use, it
>>> does not mean they can be ignored. Newer revision (such as
>>> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>>>
>>> This means that the structures vcpu_guest_core_regs and
>>> vcpu_guest_context would not be able to store the top 32-bit and
>>> therefore misrepresenting the hardware.
>>>
>>>  From my understanding, vcpu_guest_context is defined between the tools
>>> and Xen. So it would be possible to modify it without caring on
>>> backward compatibly.
>>>
>>> Howerver, struct vcpu_guest_core_reg seems to be outside of the
>>> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>>>
>>> Do you have any suggestion how this could safely be extended?
>>
>> Stuff like this should never have been in the public API to begin with.
>> x86 has some nasty issues which I have yet to find a good-enough way to fix.
>>
>> For ARM, and future architectures, I'd use the fact that there are no
>> non-tools interfaces which use this structure to allow yourself the
>> wiggleroom to declare history a mistake, and fix it by making it tools-only.
> 
> I'm unconvinced of the "declare history a mistake" part, but I agree
> with the suggestion of simply moving the structure declaration down
> into the guarded area. It simply was a mistake to not put it there in
> the first place.

Thank you both of you for the feedback. I will prepare a patch to make 
struct vcpu_guest_core_regs tools only.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] struct vcpu_guest_core_reg stable ABI?

[Julien Grall]

Hi Andrew & Jan,

On 5/16/19 8:58 AM, Jan Beulich wrote:
>>>> On 15.05.19 at 22:12, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/2019 20:58, Julien Grall wrote:
>>> Hi all,
>>>
>>> It looks like the structures vcpu_guest_core_regs and
>>> vcpu_guest_context does not correctly reflect the AArch64 state. For
>>> instance, all Arm64 system registers (e.g sctlr, cpsr, spsr_el1)
>>> should be 64-bit wide not 32-bit wide.
>>>
>>> On ARMv8.0, some of the registers have only the low 32-bit defined,
>>> the rest is RES0. RES0 only means they are reserved for future use, it
>>> does not mean they can be ignored. Newer revision (such as
>>> ARMv8.0-SSBS) actually began to define bit in the top 32-bit.
>>>
>>> This means that the structures vcpu_guest_core_regs and
>>> vcpu_guest_context would not be able to store the top 32-bit and
>>> therefore misrepresenting the hardware.
>>>
>>>  From my understanding, vcpu_guest_context is defined between the tools
>>> and Xen. So it would be possible to modify it without caring on
>>> backward compatibly.
>>>
>>> Howerver, struct vcpu_guest_core_reg seems to be outside of the
>>> #ifdef. So I assume it is part of the stable ABI. Am I correct?
>>>
>>> Do you have any suggestion how this could safely be extended?
>>
>> Stuff like this should never have been in the public API to begin with.
>> x86 has some nasty issues which I have yet to find a good-enough way to fix.
>>
>> For ARM, and future architectures, I'd use the fact that there are no
>> non-tools interfaces which use this structure to allow yourself the
>> wiggleroom to declare history a mistake, and fix it by making it tools-only.
> 
> I'm unconvinced of the "declare history a mistake" part, but I agree
> with the suggestion of simply moving the structure declaration down
> into the guarded area. It simply was a mistake to not put it there in
> the first place.

Thank you both of you for the feedback. I will prepare a patch to make 
struct vcpu_guest_core_regs tools only.

Cheers,

-- 
Julien Grall

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