| Message ID | 1467743871-9644-4-git-send-email-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 05.07.16 at 20:37, <tamas.lengyel@zentific.com> 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
On 07/06/16 10:43, Jan Beulich wrote: >>>> On 05.07.16 at 20:37, <tamas.lengyel@zentific.com> 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
On 06/07/16 08:43, Jan Beulich wrote: >>>> On 05.07.16 at 20:37, <tamas.lengyel@zentific.com> 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,
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,
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. Thanks, Tamas
On Wed, Jul 6, 2016 at 1:43 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 05.07.16 at 20:37, <tamas.lengyel@zentific.com> 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
On 06/07/2016 20:23, Tamas K Lengyel wrote: > On Wed, Jul 6, 2016 at 1:43 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 05.07.16 at 20:37, <tamas.lengyel@zentific.com> 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,
On Wed, Jul 6, 2016 at 3:12 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 06/07/2016 20:23, Tamas K Lengyel wrote: >> >> On Wed, Jul 6, 2016 at 1:43 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> >>>>>> On 05.07.16 at 20:37, <tamas.lengyel@zentific.com> 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
>>> 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
On 07/07/16 09:23, Jan Beulich wrote: >>>> 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. 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,
>>> On 07.07.16 at 11:46, <julien.grall@arm.com> 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. Jan
On 07/07/16 10:57, Jan Beulich wrote: >>>> On 07.07.16 at 11:46, <julien.grall@arm.com> 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,
On Thu, Jul 7, 2016 at 4:09 AM, Julien Grall <julien.grall@arm.com> wrote: > > > On 07/07/16 10:57, Jan Beulich wrote: >>>>> >>>>> On 07.07.16 at 11:46, <julien.grall@arm.com> 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
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 0000000..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 (tamas@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 <http://www.gnu.org/licenses/>. + */ + +#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.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; + req->data.regs.arm.arch.arm64.x30 = regs->lr; + req->data.regs.arm.arch.arm64.pc = regs->pc; + } +#endif +} + +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp) +{ + struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs; + + regs->cpsr = rsp->data.regs.arm.cpsr; + v->arch.ttbr0 = rsp->data.regs.arm.ttbr0; + v->arch.ttbr1 = rsp->data.regs.arm.ttbr1; + + if ( psr_mode_is_32bit(regs->cpsr) ) + { + regs->r0 = rsp->data.regs.arm.arch.arm32.r0; + regs->r1 = rsp->data.regs.arm.arch.arm32.r1; + regs->r2 = rsp->data.regs.arm.arch.arm32.r2; + regs->r3 = rsp->data.regs.arm.arch.arm32.r3; + regs->r4 = rsp->data.regs.arm.arch.arm32.r4; + regs->r5 = rsp->data.regs.arm.arch.arm32.r5; + regs->r6 = rsp->data.regs.arm.arch.arm32.r6; + regs->r7 = rsp->data.regs.arm.arch.arm32.r7; + regs->r8 = rsp->data.regs.arm.arch.arm32.r8; + regs->r9 = rsp->data.regs.arm.arch.arm32.r9; + regs->r10 = rsp->data.regs.arm.arch.arm32.r10; + regs->fp = rsp->data.regs.arm.arch.arm32.r11; + regs->r12 = rsp->data.regs.arm.arch.arm32.r12; + regs->sp = rsp->data.regs.arm.arch.arm32.r13; + regs->lr = rsp->data.regs.arm.arch.arm32.r14; + regs->pc32 = rsp->data.regs.arm.arch.arm32.pc; + } +#ifdef CONFIG_ARM_64 + else + { + regs->x0 = rsp->data.regs.arm.arch.arm64.x0; + regs->x1 = rsp->data.regs.arm.arch.arm64.x1; + regs->x2 = rsp->data.regs.arm.arch.arm64.x2; + regs->x3 = rsp->data.regs.arm.arch.arm64.x3; + regs->x4 = rsp->data.regs.arm.arch.arm64.x4; + regs->x5 = rsp->data.regs.arm.arch.arm64.x5; + regs->x6 = rsp->data.regs.arm.arch.arm64.x6; + regs->x7 = rsp->data.regs.arm.arch.arm64.x7; + regs->x8 = rsp->data.regs.arm.arch.arm64.x8; + regs->x9 = rsp->data.regs.arm.arch.arm64.x9; + regs->x10 = rsp->data.regs.arm.arch.arm64.x10; + regs->x11 = rsp->data.regs.arm.arch.arm64.x11; + regs->x12 = rsp->data.regs.arm.arch.arm64.x12; + regs->x13 = rsp->data.regs.arm.arch.arm64.x13; + regs->x14 = rsp->data.regs.arm.arch.arm64.x14; + regs->x15 = rsp->data.regs.arm.arch.arm64.x15; + regs->x16 = rsp->data.regs.arm.arch.arm64.x16; + regs->x17 = rsp->data.regs.arm.arch.arm64.x17; + regs->x18 = rsp->data.regs.arm.arch.arm64.x18; + regs->x19 = rsp->data.regs.arm.arch.arm64.x19; + regs->x20 = rsp->data.regs.arm.arch.arm64.x20; + regs->x21 = rsp->data.regs.arm.arch.arm64.x21; + regs->x22 = rsp->data.regs.arm.arch.arm64.x22; + regs->x23 = rsp->data.regs.arm.arch.arm64.x23; + regs->x24 = rsp->data.regs.arm.arch.arm64.x24; + regs->x25 = rsp->data.regs.arm.arch.arm64.x25; + regs->x26 = rsp->data.regs.arm.arch.arm64.x26; + regs->x27 = rsp->data.regs.arm.arch.arm64.x27; + regs->x28 = rsp->data.regs.arm.arch.arm64.x28; + regs->fp = rsp->data.regs.arm.arch.arm64.x29; + regs->lr = rsp->data.regs.arm.arch.arm64.x30; + regs->pc = rsp->data.regs.arm.arch.arm64.pc; + } +#endif +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index a3fc4ce..a4922b3 100644 --- a/xen/include/asm-arm/vm_event.h +++ b/xen/include/asm-arm/vm_event.h @@ -48,15 +48,4 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp) /* Not supported on ARM. */ } -static inline -void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp) -{ - /* Not supported on ARM. */ -} - -static inline void vm_event_fill_regs(vm_event_request_t *req) -{ - /* Not supported on ARM. */ -} - #endif /* __ASM_ARM_VM_EVENT_H__ */ diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h index 026f42e..cf2077c 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -40,8 +40,4 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v); void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp); -void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp); - -void vm_event_fill_regs(vm_event_request_t *req); - #endif /* __ASM_X86_VM_EVENT_H__ */ diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 66e27b7..d3b0c4d 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -132,8 +132,8 @@ #define VM_EVENT_X86_XCR0 3 /* - * Using a custom struct (not hvm_hw_cpu) so as to not fill - * the vm_event ring buffer too quickly. + * Using custom vCPU structs (i.e. not hvm_hw_cpu) for both x86 and ARM + * so as to not fill the vm_event ring buffer too quickly. */ struct vm_event_regs_x86 { uint64_t rax; @@ -171,6 +171,71 @@ struct vm_event_regs_x86 { uint32_t _pad; }; +struct vm_event_regs_arm32 { + uint32_t r0; + uint32_t r1; + uint32_t r2; + uint32_t r3; + uint32_t r4; + uint32_t r5; + uint32_t r6; + uint32_t r7; + uint32_t r8; + uint32_t r9; + uint32_t r10; + uint32_t r11; + uint32_t r12; + uint32_t r13; + uint32_t r14; + uint32_t pc; +}; + +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; +}; + +struct vm_event_regs_arm { + uint32_t cpsr; /* PSR_MODE_BIT is set iff arm32 is used below */ + uint32_t _pad; + uint64_t ttbr0; + uint64_t ttbr1; + union { + struct vm_event_regs_arm32 arm32; + struct vm_event_regs_arm64 arm64; + } arch; +}; + /* * mem_access flag definitions * @@ -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)]; }; typedef struct vm_event_st { @@ -275,6 +344,7 @@ typedef struct vm_event_st { union { union { struct vm_event_regs_x86 x86; + struct vm_event_regs_arm arm; } regs; struct vm_event_emul_read_data emul_read_data; diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h index 42bd9f6..95420f8 100644 --- a/xen/include/xen/vm_event.h +++ b/xen/include/xen/vm_event.h @@ -75,6 +75,9 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, void vm_event_vcpu_pause(struct vcpu *v); void vm_event_vcpu_unpause(struct vcpu *v); +void vm_event_fill_regs(vm_event_request_t *req); +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp); + #endif /* __VM_EVENT_H__ */