Re: [Xen-devel] [PATCH v5 5/9] monitor: ARM SMC events

[Julien Grall <julien.grall@xxxxxxx>]

Hello Tamas,

On 03/06/16 14:40, Tamas K Lengyel wrote:
> On Jun 3, 2016 03:49, "Julien Grall" <julien.grall@xxxxxxx
> <mailto:julien.grall@xxxxxxx>> wrote:
>  >
>  > Hello Tamas,
>  >
>  >
>  > On 02/06/16 23:52, Tamas K Lengyel wrote:
>  >>
>  >> diff --git a/xen/include/public/vm_event.h
> b/xen/include/public/vm_event.h
>  >> index 9270d52..797608Burrington0 100644
>  >> --- a/xen/include/public/vm_event.h
>  >> +++ b/xen/include/public/vm_event.h
>  >> @@ -119,6 +119,8 @@
>  >>   #define VM_EVENT_REASON_SINGLESTEP              7
>  >>   /* An event has been requested via HVMOP_guest_request_vm_event. */
>  >>   #define VM_EVENT_REASON_GUEST_REQUEST           8
>  >> +/* Privileged call executed (e.g. SMC) */
>  >> +#define VM_EVENT_REASON_PRIVILEGED_CALL         9
>  >>
>  >>   /* Supported values for the vm_event_write_ctrlreg index. */
>  >>   #define VM_EVENT_X86_CR0    0
>  >> @@ -212,6 +214,13 @@ struct vm_event_mov_to_msr {
>  >>       uint64_t value;
>  >>   };
>  >>
>  >> +#define VM_EVENT_PRIVCALL_SMC   0
>  >> +
>  >> +struct vm_event_privcall {
>  >> +    uint32_t type;
>  >> +    uint32_t vector; /* ESR_EL2.ISS for SMC calls */
>  >
>  >
>  > How do you expect the introspection app to deal with it? As explained `
> in a previous mail [1], the ISS encoding is different between ARMv7
> 32-bit and ARMv8 32-bit. The former is unknown (see B3-1431 in ARM DDI
> 0406C.c) whilst the latter contains fields related to the condition (see
> D7-1897 in ARM DDI 0406C.c).
>  >
>  > This is because on ARMv8, the conditional SMC issued in AArch32 state
> may trap even if the condition has failed.
>  >
>  > So the app would have to know whether the hypervisor is running on an
> ARMv7 or ARMv8 platform. But I am not aware of an easy way to
> differentiate it from the registers.
>
> The app can certainly run other checks to determine what the CPU version
> is, not being exclusively reliant on vm_event and running in a
> privileged domain.
Manufacturers are allowed to build their custom ARM processor based on 
the ARM ARM. The number of CPU version to check will likely be huge and 
you will not be future proof.
`
>  >
>  > Furthermore, I think the vm_event app should only received SMCs whose
> condition has succeeded, because they will be actual SMC. The others
> should just be ignored.
>  >
>  > IHMO, the vm_event should only contain the immediate. The rest only
> matters for the hypervisor.
>
> Absolutely not! The primary usecase I have for SMC trapping is kernel
> execution monitoring by manually writing it into arbitrary kernel code
> locations and hiding them from the guest with mem_access. If some SMCs
> may silently get swallowed by the hypervisor the whole thing becomes
> unreliable.
Please read what I wrote, on ARMv8, a conditional SMC issued in AArch32 
state *may* trap even if the condition has failed. I.e an implementer 
can design its CPU to not trap them (see D1-1506 on ARM DDI 0487A.i).
On ARMv7, only unconditional SMC and conditional SMC *which pass the 
condition test* will be trapped. The others will be ignored.
So even if the hypervisor send an event for each SMC trapped, you may 
not receive all the SMCs. This is already unreliable by the architecture.
If you want something reliable, you will have to inject unconditional 
SMC or HVC which are always unconditional.
If you want to also trap all the SMCs written by a guest, then you will 
have to live with the fact that some may be ignored. Although, I don't 
think that an introspection app should care about instructions that 
would be treated as a nop (for instance because the condition check has 
failed).
Hence my suggestion to check in the hypervisor whether the condition has 
failed and provide processor-agnostic information (the ISS is different 
between ARMv7, ARMv8 and AArch32 and AArch64).
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel