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

From: "Jan Beulich" <JBeulich@suse.com>
To: Tamas K Lengyel <tamas.lengyel@zentific.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH v8 4/6] arm/vm_event: get/set registers
Date: Thu, 07 Jul 2016 02:23:10 -0600	[thread overview]
Message-ID: <577E2D8E02000078000FBFE7@prv-mh.provo.novell.com> (raw)
In-Reply-To: <CAErYnsjOpuC_rCfrBHdvF=RcaTuvW5_Xa78O_NYo_DrAa7masQ@mail.gmail.com>

>>> On 06.07.16 at 21:12, <tamas.lengyel@zentific.com> wrote:
> On Wed, Jul 6, 2016 at 12:04 PM, Julien Grall <julien.grall@arm.com> 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