On Thu, Jul 28, 2016 at 2:41 PM, Andrew Cooper
<andrew.coop...@citrix.com> wrote:
> On 28/07/2016 21:36, Tamas K Lengyel wrote:
>> On Thu, Jul 28, 2016 at 2:26 PM, Andrew Cooper
>> <andrew.coop...@citrix.com> wrote:
>>> On 28/07/2016 21:05, Tamas K Lengyel wrote:
>>>> 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 necessary but for now it is limited to TTB/CR/R0/R1, PC and
>>>> CPSR.
>>>>
>>>> Signed-off-by: Tamas K Lengyel <tamas.leng...@zentific.com>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabell...@kernel.org>
>>>> Cc: Julien Grall <julien.gr...@arm.com>
>>>> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com>
>>>> Cc: Jan Beulich <jbeul...@suse.com>
>>>> Cc: Andrew Cooper <andrew.coop...@citrix.com>
>>> For the x86 and common bits, Reviewed-by: Andrew Cooper
>>> <andrew.coop...@citrix.com>
>>>
>>> However,
>>>
>>>> +#include <xen/sched.h>
>>>> +#include <asm/vm_event.h>
>>>> +
>>>> +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.pc = regs->pc;
>>>> + req->data.regs.arm.ttbcr = READ_SYSREG(TCR_EL1);
>>>> + req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
>>>> + req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
>>>> +}
>>>> +
>>>> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>>>> +{
>>>> + struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>> +
>>>> + regs->cpsr = rsp->data.regs.arm.cpsr;
>>>> + regs->pc = rsp->data.regs.arm.pc;
>>>> + v->arch.ttbcr = rsp->data.regs.arm.ttbcr;
>>>> + v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
>>>> + v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
>>> Not knowing anything about ARM, but this looks like it is missing some
>>> sanity/plausibility checks (to protect Xen against accidental clobbering
>>> from the vm_event listener), and some WRITE_SYSREG() to reload the new
>>> values (unless this is done unconditionally later, at which point you
>>> should at least leave a comment here saying so).
>>>
>> This function only ever gets called if the vm_event response
>> specifically has the VM_EVENT_FLAG_SET_REGISTERS set, so accidental
>> clobbering is not possible.
>
> That isn't my point. Are there any reserved bits in the registers
> themselves which could cause Xen to fault when it tries to reload? If
> all that happens is a domain_crash() then ok, but if Xen falls over with
> a fatal fault, that should be avoided.
I agree. At the moment the only register I actually need access
through vm_event setting is PC so I'll just leave the other registers
out and document it in the vm_event header.
>
> (i.e. there should be no bit pattern a vm_event listener could ever set
> which causes a crash of the hypervisor itself)
>
>> Also, using WRITE_SYSREG() is not safe at
>> this point because current != v.
>
> Ok, but how do these new values end up getting propagated into hardware?
>
AFAIK during scheduling the registers get loaded from this save state.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
