Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-07 Thread Tamas K Lengyel 
On Thu, Jul 7, 2016 at 4:09 AM, Julien Grall  wrote:
>
>
> On 07/07/16 10:57, Jan Beulich wrote:
>
> On 07.07.16 at 11:46,  wrote:
>>>
>>> Anyway, I think this patch was in a good state (though few registers
>>> were needed clarification). Assuming it is ok to break the compatibility
>>> later on, I will not oppose to have a reduce set.
>>
>>
>> Iirc we did bump that interface version already after the tree got
>> opened for 4.8.
>
>
> I had the impression that Tamas does not plan to add the full set of the
> registers for 4.8. If so, we would have to bump another time later.

I have no current plans for sending the full set of registers. As
Razvan pointed pointed out earlier, it would be better to do that as
part of a bigger extension of vm_event that would allow the ring to be
multi-page. That however is well beyond what I'm aiming to achieve
here at the moment. I rewrote the vm_event system specifically to
allow extensions to it that break the ABI in a way that applications
can safely deal with it. Yes, it may require applications to be
recompiled for newer versions of Xen but that is to be expected and
should not be a show-stopper.

Thanks,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-07 Thread Julien Grall 



On 07/07/16 10:57, Jan Beulich wrote:

On 07.07.16 at 11:46,  wrote:

Anyway, I think this patch was in a good state (though few registers
were needed clarification). Assuming it is ok to break the compatibility
later on, I will not oppose to have a reduce set.


Iirc we did bump that interface version already after the tree got
opened for 4.8.


I had the impression that Tamas does not plan to add the full set of the 
registers for 4.8. If so, we would have to bump another time later.


Maybe he can clarify whether the full set will be added for Xen 4.8 or not.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-07 Thread Julien Grall 



On 07/07/16 09:23, Jan Beulich wrote:

On 06.07.16 at 21:12,  wrote:

On Wed, Jul 6, 2016 at 12:04 PM, Julien Grall  wrote:



On 05/07/16 19:37, Tamas K Lengyel wrote:


+void vm_event_fill_regs(vm_event_request_t *req)
+{
+const struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+req->data.regs.arm.cpsr = regs->cpsr;
+req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
+req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
+
+if ( psr_mode_is_32bit(regs->cpsr) )
+{
+req->data.regs.arm.arch.arm32.r0 = regs->r0;
+req->data.regs.arm.arch.arm32.r1 = regs->r1;
+req->data.regs.arm.arch.arm32.r2 = regs->r2;
+req->data.regs.arm.arch.arm32.r3 = regs->r3;
+req->data.regs.arm.arch.arm32.r4 = regs->r4;
+req->data.regs.arm.arch.arm32.r5 = regs->r5;
+req->data.regs.arm.arch.arm32.r6 = regs->r6;
+req->data.regs.arm.arch.arm32.r7 = regs->r7;
+req->data.regs.arm.arch.arm32.r8 = regs->r8;
+req->data.regs.arm.arch.arm32.r9 = regs->r9;
+req->data.regs.arm.arch.arm32.r10 = regs->r10;
+req->data.regs.arm.arch.arm32.r11 = regs->fp;
+req->data.regs.arm.arch.arm32.r12 = regs->r12;
+req->data.regs.arm.arch.arm32.r13 = regs->sp;



Please look at the description of "sp" in cpu_user_regs. You will notice
this is only valid for the hypervisor.

There are multiple stack pointers for the guest depending on the running
mode (see B9.2.1 in ARM DDI 0406C.c), so you may want to pass all of them.


+req->data.regs.arm.arch.arm32.r14 = regs->lr;



Whilst lr is an union with lr_usr on ARM32 port, for the ARM64 port they are
distinct (see cpu_users). So you would use the wrong register here.

However, as for sp, there are multiple lr pointers for the guest depending
on the running mode. So you will pass the wrong lr if the guest is running
in another mode than user.



This patch is becoming a lot more work then what I need it for so I'm
inclined to just reduce it to the bare minimum my use-case requires,
which is only cpsr, pc and ttbr0 and ttbr1. I had reservation about
growing the data field in the vm_event struct anyway, especially since
there is no use-case that requires it. In case someone has an actual
use-case in the future that requires other registers to be submitted
through vm_event, the interface is available for extension.


Rather than dropping this patch entirely (as you suggest in your
other reply) I am, fwiw, fine with a register set not including any
GPRs at all. Not sure about Julien though.


Well, the SMC instructions are used to communicate with the secure mode 
(usually a trusted firmware).


In general, if you want to trap the SMC instructions it is for emulating 
them and not using them for breakpoint in the guest (as for Tamas's 
use-case). In this case, you want to have the set of GPRs available in 
the vm_event request to avoid the overhead of the DOMCTL_getvcpucontext 
(assuming the vCPU has been paused).


Adding the GPRs in the vm_event will likely require to bump the version 
number (VM_EVENT_INTERFACE_VERSION) because the ABI will not be 
compatible. If we consider that it is ok to ask developers to rebuild 
their vm-event app, then fine.


Anyway, I think this patch was in a good state (though few registers 
were needed clarification). Assuming it is ok to break the compatibility 
later on, I will not oppose to have a reduce set.


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-07 Thread Jan Beulich 
>>> On 06.07.16 at 21:12,  wrote:
> On Wed, Jul 6, 2016 at 12:04 PM, Julien Grall  wrote:
>>
>>
>> On 05/07/16 19:37, Tamas K Lengyel wrote:
>>>
>>> +void vm_event_fill_regs(vm_event_request_t *req)
>>> +{
>>> +const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>> +
>>> +req->data.regs.arm.cpsr = regs->cpsr;
>>> +req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
>>> +req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
>>> +
>>> +if ( psr_mode_is_32bit(regs->cpsr) )
>>> +{
>>> +req->data.regs.arm.arch.arm32.r0 = regs->r0;
>>> +req->data.regs.arm.arch.arm32.r1 = regs->r1;
>>> +req->data.regs.arm.arch.arm32.r2 = regs->r2;
>>> +req->data.regs.arm.arch.arm32.r3 = regs->r3;
>>> +req->data.regs.arm.arch.arm32.r4 = regs->r4;
>>> +req->data.regs.arm.arch.arm32.r5 = regs->r5;
>>> +req->data.regs.arm.arch.arm32.r6 = regs->r6;
>>> +req->data.regs.arm.arch.arm32.r7 = regs->r7;
>>> +req->data.regs.arm.arch.arm32.r8 = regs->r8;
>>> +req->data.regs.arm.arch.arm32.r9 = regs->r9;
>>> +req->data.regs.arm.arch.arm32.r10 = regs->r10;
>>> +req->data.regs.arm.arch.arm32.r11 = regs->fp;
>>> +req->data.regs.arm.arch.arm32.r12 = regs->r12;
>>> +req->data.regs.arm.arch.arm32.r13 = regs->sp;
>>
>>
>> Please look at the description of "sp" in cpu_user_regs. You will notice
>> this is only valid for the hypervisor.
>>
>> There are multiple stack pointers for the guest depending on the running
>> mode (see B9.2.1 in ARM DDI 0406C.c), so you may want to pass all of them.
>>
>>> +req->data.regs.arm.arch.arm32.r14 = regs->lr;
>>
>>
>> Whilst lr is an union with lr_usr on ARM32 port, for the ARM64 port they are
>> distinct (see cpu_users). So you would use the wrong register here.
>>
>> However, as for sp, there are multiple lr pointers for the guest depending
>> on the running mode. So you will pass the wrong lr if the guest is running
>> in another mode than user.
>>
> 
> This patch is becoming a lot more work then what I need it for so I'm
> inclined to just reduce it to the bare minimum my use-case requires,
> which is only cpsr, pc and ttbr0 and ttbr1. I had reservation about
> growing the data field in the vm_event struct anyway, especially since
> there is no use-case that requires it. In case someone has an actual
> use-case in the future that requires other registers to be submitted
> through vm_event, the interface is available for extension.

Rather than dropping this patch entirely (as you suggest in your
other reply) I am, fwiw, fine with a register set not including any
GPRs at all. Not sure about Julien though.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Tamas K Lengyel 
On Wed, Jul 6, 2016 at 3:12 PM, Julien Grall  wrote:
>
>
> On 06/07/2016 20:23, Tamas K Lengyel wrote:
>>
>> On Wed, Jul 6, 2016 at 1:43 AM, Jan Beulich  wrote:
>>
>> On 05.07.16 at 20:37,  wrote:

 +struct vm_event_regs_arm64 {
 +uint64_t x0;
 +uint64_t x1;
 +uint64_t x2;
 +uint64_t x3;
 +uint64_t x4;
 +uint64_t x5;
 +uint64_t x6;
 +uint64_t x7;
 +uint64_t x8;
 +uint64_t x9;
 +uint64_t x10;
 +uint64_t x11;
 +uint64_t x12;
 +uint64_t x13;
 +uint64_t x14;
 +uint64_t x15;
 +uint64_t x16;
 +uint64_t x17;
 +uint64_t x18;
 +uint64_t x19;
 +uint64_t x20;
 +uint64_t x21;
 +uint64_t x22;
 +uint64_t x23;
 +uint64_t x24;
 +uint64_t x25;
 +uint64_t x26;
 +uint64_t x27;
 +uint64_t x28;
 +uint64_t x29;
 +uint64_t x30;
 +uint64_t pc;
 +};
>>>
>>>
>>> Isn't the stack pointer a fully separate register in aarch64? Not
>>> making available something as essential as that seems wrong to
>>> me.
>>>
>>
>> The register is available for access already, so unless there is an
>> actual use-case that requires it to be transmitted through vm_event I
>> don't see the point for transmitting it. So as I mentioned in my other
>> response, I'm inclined to reduce this patch to the bare essentials my
>> use-case requires at this point and leave the extensions up for the
>> future when - and if - it will be of use. Since this patch is just an
>> optimization, if transmitting such reduced set is not acceptable for
>> some reason, I'll forgo this patch entirely.
>
>
> Here we go with again the same argument: "this is not necessary for my
> use-case". This data structure is part of the ABI between the hypervisor and
> the vm-event app, i.e modifying this structure for adding ARM64/ARM
> registers will result to an incompatibility with a previous version of the
> hypervisor. Better to do it now than in a couple of years when vm-event will
> have more users. I agree that it is time consuming to get an ABI correct,
> but it will save users to recompile/ship another version of their vm-event
> app because of this incompatibility.
>
> As mentioned in a previous thread, the main use-case for trapping an SMC is
> emulating the call, hence a vm-event app would want to have access to the
> general-purpose registers. And yes, I know that your use-case is different
> and does not require those registers, I already expressed my concerns about
> it.
>
> Now, if you drop this patch, how will you retrieve the vCPU register? I
> guess via DOMCTL_getvcpucontext? If so, if the vCPU has not been paused, you
> will get a context which is different compare to the time the vm-event has
> occurred. And yes, I know that in your use-case the vCPU is paused. This
> call will always be more expensive than passing the registers with event.

Ack but as right now this patch is just an optimization with no
use-case where it is required, so I'll just drop it from my series.

>
> Anyway, I really don't think we ask for something really difficult to
> accomplish.
>

That may be so and it can be revisited in the future when and if it
will be a hard requirement.

Thanks,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Julien Grall 



On 06/07/2016 20:23, Tamas K Lengyel wrote:

On Wed, Jul 6, 2016 at 1:43 AM, Jan Beulich  wrote:

On 05.07.16 at 20:37,  wrote:

+struct vm_event_regs_arm64 {
+uint64_t x0;
+uint64_t x1;
+uint64_t x2;
+uint64_t x3;
+uint64_t x4;
+uint64_t x5;
+uint64_t x6;
+uint64_t x7;
+uint64_t x8;
+uint64_t x9;
+uint64_t x10;
+uint64_t x11;
+uint64_t x12;
+uint64_t x13;
+uint64_t x14;
+uint64_t x15;
+uint64_t x16;
+uint64_t x17;
+uint64_t x18;
+uint64_t x19;
+uint64_t x20;
+uint64_t x21;
+uint64_t x22;
+uint64_t x23;
+uint64_t x24;
+uint64_t x25;
+uint64_t x26;
+uint64_t x27;
+uint64_t x28;
+uint64_t x29;
+uint64_t x30;
+uint64_t pc;
+};


Isn't the stack pointer a fully separate register in aarch64? Not
making available something as essential as that seems wrong to
me.



The register is available for access already, so unless there is an
actual use-case that requires it to be transmitted through vm_event I
don't see the point for transmitting it. So as I mentioned in my other
response, I'm inclined to reduce this patch to the bare essentials my
use-case requires at this point and leave the extensions up for the
future when - and if - it will be of use. Since this patch is just an
optimization, if transmitting such reduced set is not acceptable for
some reason, I'll forgo this patch entirely.


Here we go with again the same argument: "this is not necessary for my 
use-case". This data structure is part of the ABI between the hypervisor 
and the vm-event app, i.e modifying this structure for adding ARM64/ARM 
registers will result to an incompatibility with a previous version of 
the hypervisor. Better to do it now than in a couple of years when 
vm-event will have more users. I agree that it is time consuming to get 
an ABI correct, but it will save users to recompile/ship another version 
of their vm-event app because of this incompatibility.


As mentioned in a previous thread, the main use-case for trapping an SMC 
is emulating the call, hence a vm-event app would want to have access to 
the general-purpose registers. And yes, I know that your use-case is 
different and does not require those registers, I already expressed my 
concerns about it.


Now, if you drop this patch, how will you retrieve the vCPU register? I 
guess via DOMCTL_getvcpucontext? If so, if the vCPU has not been paused, 
you will get a context which is different compare to the time the 
vm-event has occurred. And yes, I know that in your use-case the vCPU is 
paused. This call will always be more expensive than passing the 
registers with event.


Anyway, I really don't think we ask for something really difficult to 
accomplish.


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Tamas K Lengyel 
On Wed, Jul 6, 2016 at 1:43 AM, Jan Beulich  wrote:
 On 05.07.16 at 20:37,  wrote:
>> +struct vm_event_regs_arm64 {
>> +uint64_t x0;
>> +uint64_t x1;
>> +uint64_t x2;
>> +uint64_t x3;
>> +uint64_t x4;
>> +uint64_t x5;
>> +uint64_t x6;
>> +uint64_t x7;
>> +uint64_t x8;
>> +uint64_t x9;
>> +uint64_t x10;
>> +uint64_t x11;
>> +uint64_t x12;
>> +uint64_t x13;
>> +uint64_t x14;
>> +uint64_t x15;
>> +uint64_t x16;
>> +uint64_t x17;
>> +uint64_t x18;
>> +uint64_t x19;
>> +uint64_t x20;
>> +uint64_t x21;
>> +uint64_t x22;
>> +uint64_t x23;
>> +uint64_t x24;
>> +uint64_t x25;
>> +uint64_t x26;
>> +uint64_t x27;
>> +uint64_t x28;
>> +uint64_t x29;
>> +uint64_t x30;
>> +uint64_t pc;
>> +};
>
> Isn't the stack pointer a fully separate register in aarch64? Not
> making available something as essential as that seems wrong to
> me.
>

The register is available for access already, so unless there is an
actual use-case that requires it to be transmitted through vm_event I
don't see the point for transmitting it. So as I mentioned in my other
response, I'm inclined to reduce this patch to the bare essentials my
use-case requires at this point and leave the extensions up for the
future when - and if - it will be of use. Since this patch is just an
optimization, if transmitting such reduced set is not acceptable for
some reason, I'll forgo this patch entirely.

Thanks,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Tamas K Lengyel 
On Wed, Jul 6, 2016 at 12:04 PM, Julien Grall  wrote:
>
>
> On 05/07/16 19:37, Tamas K Lengyel wrote:
>>
>> +void vm_event_fill_regs(vm_event_request_t *req)
>> +{
>> +const struct cpu_user_regs *regs = guest_cpu_user_regs();
>> +
>> +req->data.regs.arm.cpsr = regs->cpsr;
>> +req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
>> +req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
>> +
>> +if ( psr_mode_is_32bit(regs->cpsr) )
>> +{
>> +req->data.regs.arm.arch.arm32.r0 = regs->r0;
>> +req->data.regs.arm.arch.arm32.r1 = regs->r1;
>> +req->data.regs.arm.arch.arm32.r2 = regs->r2;
>> +req->data.regs.arm.arch.arm32.r3 = regs->r3;
>> +req->data.regs.arm.arch.arm32.r4 = regs->r4;
>> +req->data.regs.arm.arch.arm32.r5 = regs->r5;
>> +req->data.regs.arm.arch.arm32.r6 = regs->r6;
>> +req->data.regs.arm.arch.arm32.r7 = regs->r7;
>> +req->data.regs.arm.arch.arm32.r8 = regs->r8;
>> +req->data.regs.arm.arch.arm32.r9 = regs->r9;
>> +req->data.regs.arm.arch.arm32.r10 = regs->r10;
>> +req->data.regs.arm.arch.arm32.r11 = regs->fp;
>> +req->data.regs.arm.arch.arm32.r12 = regs->r12;
>> +req->data.regs.arm.arch.arm32.r13 = regs->sp;
>
>
> Please look at the description of "sp" in cpu_user_regs. You will notice
> this is only valid for the hypervisor.
>
> There are multiple stack pointers for the guest depending on the running
> mode (see B9.2.1 in ARM DDI 0406C.c), so you may want to pass all of them.
>
>> +req->data.regs.arm.arch.arm32.r14 = regs->lr;
>
>
> Whilst lr is an union with lr_usr on ARM32 port, for the ARM64 port they are
> distinct (see cpu_users). So you would use the wrong register here.
>
> However, as for sp, there are multiple lr pointers for the guest depending
> on the running mode. So you will pass the wrong lr if the guest is running
> in another mode than user.
>

This patch is becoming a lot more work then what I need it for so I'm
inclined to just reduce it to the bare minimum my use-case requires,
which is only cpsr, pc and ttbr0 and ttbr1. I had reservation about
growing the data field in the vm_event struct anyway, especially since
there is no use-case that requires it. In case someone has an actual
use-case in the future that requires other registers to be submitted
through vm_event, the interface is available for extension.

Thanks,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Julien Grall 



On 05/07/16 19:37, Tamas K Lengyel wrote:

+void vm_event_fill_regs(vm_event_request_t *req)
+{
+const struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+req->data.regs.arm.cpsr = regs->cpsr;
+req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
+req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
+
+if ( psr_mode_is_32bit(regs->cpsr) )
+{
+req->data.regs.arm.arch.arm32.r0 = regs->r0;
+req->data.regs.arm.arch.arm32.r1 = regs->r1;
+req->data.regs.arm.arch.arm32.r2 = regs->r2;
+req->data.regs.arm.arch.arm32.r3 = regs->r3;
+req->data.regs.arm.arch.arm32.r4 = regs->r4;
+req->data.regs.arm.arch.arm32.r5 = regs->r5;
+req->data.regs.arm.arch.arm32.r6 = regs->r6;
+req->data.regs.arm.arch.arm32.r7 = regs->r7;
+req->data.regs.arm.arch.arm32.r8 = regs->r8;
+req->data.regs.arm.arch.arm32.r9 = regs->r9;
+req->data.regs.arm.arch.arm32.r10 = regs->r10;
+req->data.regs.arm.arch.arm32.r11 = regs->fp;
+req->data.regs.arm.arch.arm32.r12 = regs->r12;
+req->data.regs.arm.arch.arm32.r13 = regs->sp;


Please look at the description of "sp" in cpu_user_regs. You will notice 
this is only valid for the hypervisor.


There are multiple stack pointers for the guest depending on the running 
mode (see B9.2.1 in ARM DDI 0406C.c), so you may want to pass all of them.



+req->data.regs.arm.arch.arm32.r14 = regs->lr;


Whilst lr is an union with lr_usr on ARM32 port, for the ARM64 port they 
are distinct (see cpu_users). So you would use the wrong register here.


However, as for sp, there are multiple lr pointers for the guest 
depending on the running mode. So you will pass the wrong lr if the 
guest is running in another mode than user.


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Julien Grall 



On 06/07/16 08:43, Jan Beulich wrote:

On 05.07.16 at 20:37,  wrote:

+struct vm_event_regs_arm64 {
+uint64_t x0;
+uint64_t x1;
+uint64_t x2;
+uint64_t x3;
+uint64_t x4;
+uint64_t x5;
+uint64_t x6;
+uint64_t x7;
+uint64_t x8;
+uint64_t x9;
+uint64_t x10;
+uint64_t x11;
+uint64_t x12;
+uint64_t x13;
+uint64_t x14;
+uint64_t x15;
+uint64_t x16;
+uint64_t x17;
+uint64_t x18;
+uint64_t x19;
+uint64_t x20;
+uint64_t x21;
+uint64_t x22;
+uint64_t x23;
+uint64_t x24;
+uint64_t x25;
+uint64_t x26;
+uint64_t x27;
+uint64_t x28;
+uint64_t x29;
+uint64_t x30;
+uint64_t pc;
+};


Isn't the stack pointer a fully separate register in aarch64?


It is. Actually, there are 2 stack pointers (one for the kernel, the 
other for userspace. See sp_el0 and sp_el1.


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Razvan Cojocaru 
On 07/06/16 10:43, Jan Beulich wrote:
 On 05.07.16 at 20:37,  wrote:
>> +struct vm_event_regs_arm64 {
>> +uint64_t x0;
>> +uint64_t x1;
>> +uint64_t x2;
>> +uint64_t x3;
>> +uint64_t x4;
>> +uint64_t x5;
>> +uint64_t x6;
>> +uint64_t x7;
>> +uint64_t x8;
>> +uint64_t x9;
>> +uint64_t x10;
>> +uint64_t x11;
>> +uint64_t x12;
>> +uint64_t x13;
>> +uint64_t x14;
>> +uint64_t x15;
>> +uint64_t x16;
>> +uint64_t x17;
>> +uint64_t x18;
>> +uint64_t x19;
>> +uint64_t x20;
>> +uint64_t x21;
>> +uint64_t x22;
>> +uint64_t x23;
>> +uint64_t x24;
>> +uint64_t x25;
>> +uint64_t x26;
>> +uint64_t x27;
>> +uint64_t x28;
>> +uint64_t x29;
>> +uint64_t x30;
>> +uint64_t pc;
>> +};
> 
> Isn't the stack pointer a fully separate register in aarch64? Not
> making available something as essential as that seems wrong to
> me.
> 
>> @@ -246,10 +311,14 @@ struct vm_event_sharing {
>>  uint32_t _pad;
>>  };
>>  
>> +#define VM_EVENT_MAX_DATA_SIZE \
>> +(sizeof(struct vm_event_regs_x86) > sizeof(struct vm_event_regs_arm) ? \
>> +sizeof(struct vm_event_regs_x86) : sizeof(struct vm_event_regs_arm))
>> +
>>  struct vm_event_emul_read_data {
>>  uint32_t size;
>>  /* The struct is used in a union with vm_event_regs_x86. */
>> -uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
>> +uint8_t  data[VM_EVENT_MAX_DATA_SIZE - sizeof(uint32_t)];
>>  };
> 
> Would there be a problem leaving this alone, i.e. not growing the
> current limit? I ask because the way VM_EVENT_MAX_DATA_SIZE gets
> established doesn't look very scalable - just think about how that
> would look like after half a dozen more architectures got added.
> 
>> @@ -275,6 +344,7 @@ typedef struct vm_event_st {
>>  union {
>>  union {
>>  struct vm_event_regs_x86 x86;
>> +struct vm_event_regs_arm arm;
>>  } regs;
> 
> So the growth in size here then is not really a problem?

Should be fine for our purposes.


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-06 Thread Jan Beulich 
>>> On 05.07.16 at 20:37,  wrote:
> +struct vm_event_regs_arm64 {
> +uint64_t x0;
> +uint64_t x1;
> +uint64_t x2;
> +uint64_t x3;
> +uint64_t x4;
> +uint64_t x5;
> +uint64_t x6;
> +uint64_t x7;
> +uint64_t x8;
> +uint64_t x9;
> +uint64_t x10;
> +uint64_t x11;
> +uint64_t x12;
> +uint64_t x13;
> +uint64_t x14;
> +uint64_t x15;
> +uint64_t x16;
> +uint64_t x17;
> +uint64_t x18;
> +uint64_t x19;
> +uint64_t x20;
> +uint64_t x21;
> +uint64_t x22;
> +uint64_t x23;
> +uint64_t x24;
> +uint64_t x25;
> +uint64_t x26;
> +uint64_t x27;
> +uint64_t x28;
> +uint64_t x29;
> +uint64_t x30;
> +uint64_t pc;
> +};

Isn't the stack pointer a fully separate register in aarch64? Not
making available something as essential as that seems wrong to
me.

> @@ -246,10 +311,14 @@ struct vm_event_sharing {
>  uint32_t _pad;
>  };
>  
> +#define VM_EVENT_MAX_DATA_SIZE \
> +(sizeof(struct vm_event_regs_x86) > sizeof(struct vm_event_regs_arm) ? \
> +sizeof(struct vm_event_regs_x86) : sizeof(struct vm_event_regs_arm))
> +
>  struct vm_event_emul_read_data {
>  uint32_t size;
>  /* The struct is used in a union with vm_event_regs_x86. */
> -uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
> +uint8_t  data[VM_EVENT_MAX_DATA_SIZE - sizeof(uint32_t)];
>  };

Would there be a problem leaving this alone, i.e. not growing the
current limit? I ask because the way VM_EVENT_MAX_DATA_SIZE gets
established doesn't look very scalable - just think about how that
would look like after half a dozen more architectures got added.

> @@ -275,6 +344,7 @@ typedef struct vm_event_st {
>  union {
>  union {
>  struct vm_event_regs_x86 x86;
> +struct vm_event_regs_arm arm;
>  } regs;

So the growth in size here then is not really a problem?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers

2016-07-05 Thread Tamas K Lengyel 
Add support for getting/setting registers through vm_event on ARM.
The set of registers can be expanded in the future to include other registers
as well if required. The set is limited to the GPRs, PC, CPSR and TTBR0/1 in
this patch.

Signed-off-by: Tamas K Lengyel 
Acked-by: Razvan Cojocaru 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
 xen/arch/arm/Makefile  |   1 +
 xen/arch/arm/vm_event.c| 163 +
 xen/include/asm-arm/vm_event.h |  11 ---
 xen/include/asm-x86/vm_event.h |   4 -
 xen/include/public/vm_event.h  |  76 ++-
 xen/include/xen/vm_event.h |   3 +
 6 files changed, 240 insertions(+), 18 deletions(-)
 create mode 100644 xen/arch/arm/vm_event.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index ffb0e96..82ac34c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -42,6 +42,7 @@ obj-y += processor.o
 obj-y += smc.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += monitor.o
+obj-y += vm_event.o
 
 #obj-bin-y += o
 
diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 000..0e59fae
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,163 @@
+/*
+ * arch/arm/vm_event.c
+ *
+ * Architecture-specific vm_event handling routines
+ *
+ * Copyright (c) 2016 Tamas K Lengyel (ta...@tklengyel.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see .
+ */
+
+#include 
+#include 
+
+void vm_event_fill_regs(vm_event_request_t *req)
+{
+const struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+req->data.regs.arm.cpsr = regs->cpsr;
+req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
+req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
+
+if ( psr_mode_is_32bit(regs->cpsr) )
+{
+req->data.regs.arm.arch.arm32.r0 = regs->r0;
+req->data.regs.arm.arch.arm32.r1 = regs->r1;
+req->data.regs.arm.arch.arm32.r2 = regs->r2;
+req->data.regs.arm.arch.arm32.r3 = regs->r3;
+req->data.regs.arm.arch.arm32.r4 = regs->r4;
+req->data.regs.arm.arch.arm32.r5 = regs->r5;
+req->data.regs.arm.arch.arm32.r6 = regs->r6;
+req->data.regs.arm.arch.arm32.r7 = regs->r7;
+req->data.regs.arm.arch.arm32.r8 = regs->r8;
+req->data.regs.arm.arch.arm32.r9 = regs->r9;
+req->data.regs.arm.arch.arm32.r10 = regs->r10;
+req->data.regs.arm.arch.arm32.r11 = regs->fp;
+req->data.regs.arm.arch.arm32.r12 = regs->r12;
+req->data.regs.arm.arch.arm32.r13 = regs->sp;
+req->data.regs.arm.arch.arm32.r14 = regs->lr;
+req->data.regs.arm.arch.arm32.pc = regs->pc32;
+}
+#ifdef CONFIG_ARM_64
+else
+{
+req->data.regs.arm.arch.arm64.x0 = regs->x0;
+req->data.regs.arm.arch.arm64.x1 = regs->x1;
+req->data.regs.arm.arch.arm64.x2 = regs->x2;
+req->data.regs.arm.arch.arm64.x3 = regs->x3;
+req->data.regs.arm.arch.arm64.x4 = regs->x4;
+req->data.regs.arm.arch.arm64.x5 = regs->x5;
+req->data.regs.arm.arch.arm64.x6 = regs->x6;
+req->data.regs.arm.arch.arm64.x7 = regs->x7;
+req->data.regs.arm.arch.arm64.x8 = regs->x8;
+req->data.regs.arm.arch.arm64.x9 = regs->x9;
+req->data.regs.arm.arch.arm64.x10 = regs->x10;
+req->data.regs.arm.arch.arm64.x11 = regs->x11;
+req->data.regs.arm.arch.arm64.x12 = regs->x12;
+req->data.regs.arm.arch.arm64.x13 = regs->x13;
+req->data.regs.arm.arch.arm64.x14 = regs->x14;
+req->data.regs.arm.arch.arm64.x15 = regs->x15;
+req->data.regs.arm.arch.arm64.x16 = regs->x16;
+req->data.regs.arm.arch.arm64.x17 = regs->x17;
+req->data.regs.arm.arch.arm64.x18 = regs->x18;
+req->data.regs.arm.arch.arm64.x19 = regs->x19;
+req->data.regs.arm.arch.arm64.x20 = regs->x20;
+req->data.regs.arm.arch.arm64.x21 = regs->x21;
+req->data.regs.arm.arch.arm64.x22 = regs->x22;
+req->data.regs.arm.arch.arm64.x23 = regs->x23;
+req->data.regs.arm.arch.arm64.x24 = regs->x24;
+req->data.regs.arm.arch.arm64.x25 = regs->x25;
+req->data.regs.arm.arch.arm64.x26 = regs->x26;
+req->data.regs.arm.arch.arm64.x27 = regs->x27;
+req->data.regs.arm.arch.arm64.x28 = regs->x28;
+req->data.regs.arm.arch.arm64.x29 = regs->fp;
+