Re: [PATCH v2 2/5] vm_event: Implement ARM SMC events

[Julien Grall <julien.grall@arm.com>]

Hi Tamas,

I have just noticed that you use an old email address for Stefano. 
Please check MAINTAINERS file for any update on email address, new 
maintainers.

On 04/05/2016 13:42, Tamas K Lengyel wrote:
>
>  > On 03/05/2016 19:48, Tamas K Lengyel wrote:
>  >>
>  >>
>  >>
>  >> On Tue, May 3, 2016 at 5:31 AM, Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>
>  >> <mailto:julien.grall@arm.com <mailto:julien.grall@arm.com>>> wrote:
>  >>     On 29/04/16 19:07, Tamas K Lengyel wrote:
>  >>
>  >>         The ARM SMC instructions are already configured to trap to Xen
>  >>         by default. In
>  >>         this patch we allow a user-space process in a privileged domain
>  >>         to receive
>  >>         notification of when such event happens through the vm_event
>  >>         subsystem by
>  >>         introducing the PRIVILEGED_CALL type.
>  >>
>  >>         Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com
> <mailto:tamas@tklengyel.com>
>  >>         <mailto:tamas@tklengyel.com <mailto:tamas@tklengyel.com>>>
>  >>
>  >>         ---
>  >>         Cc: Razvan Cojocaru <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>
>  >>         <mailto:rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>>>
>  >>         Cc: Ian Jackson <ian.jackson@eu.citrix.com
> <mailto:ian.jackson@eu.citrix.com>
>  >>         <mailto:ian.jackson@eu.citrix.com
> <mailto:ian.jackson@eu.citrix.com>>>
>  >>         Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com
> <mailto:stefano.stabellini@eu.citrix.com>
>  >>         <mailto:stefano.stabellini@eu.citrix.com
> <mailto:stefano.stabellini@eu.citrix.com>>>
>  >>         Cc: Wei Liu <wei.liu2@citrix.com
> <mailto:wei.liu2@citrix.com> <mailto:wei.liu2@citrix.com
> <mailto:wei.liu2@citrix.com>>>
>  >>         Cc: Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>
>  >>         <mailto:julien.grall@arm.com <mailto:julien.grall@arm.com>>>
>  >>         Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>
> <mailto:keir@xen.org <mailto:keir@xen.org>>>
>  >>         Cc: Jan Beulich <jbeulich@suse.com
> <mailto:jbeulich@suse.com> <mailto:jbeulich@suse.com
> <mailto:jbeulich@suse.com>>>
>  >>         Cc: Andrew Cooper <andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>
>  >>         <mailto:andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>>>
>  >>
>  >>
>  >>         v2: introduce VM_EVENT_REASON_PRIVELEGED_CALL
>  >>               aarch64 support
>  >>         ---
>  >>            MAINTAINERS                         |   6 +-
>  >>            tools/libxc/include/xenctrl.h       |   2 +
>  >>            tools/libxc/xc_monitor.c            |  26 +++++++-
>  >>            tools/tests/xen-access/xen-access.c |  31 ++++++++-
>  >>            xen/arch/arm/Makefile               |   2 +
>  >>            xen/arch/arm/monitor.c              |  80
> +++++++++++++++++++++++
>  >>            xen/arch/arm/traps.c                |  20 +++++-
>  >>            xen/arch/arm/vm_event.c             | 127
>  >>         ++++++++++++++++++++++++++++++++++++
>  >>            xen/arch/x86/hvm/event.c            |   2 +
>  >>            xen/common/vm_event.c               |   1 -
>  >>            xen/include/asm-arm/domain.h        |   5 ++
>  >>            xen/include/asm-arm/monitor.h       |  20 ++----
>  >>            xen/include/asm-arm/vm_event.h      |  16 ++---
>  >>            xen/include/public/domctl.h         |   1 +
>  >>            xen/include/public/vm_event.h       |  27 ++++++++
>  >>            15 files changed, 333 insertions(+), 33 deletions(-)
>  >>            create mode 100644 xen/arch/arm/monitor.c
>  >>            create mode 100644 xen/arch/arm/vm_event.c
>  >>
>  >>
>  >>     This patch is doing lots of things:
>  >>              - Add support for monitoring
>  >>              - Add support for vm_event
>  >>              - Monitor SMC
>  >>              - Move common code to arch specific code
>  >>
>  >>     As far as I can tell, all are distinct from each other. Can you
>  >>     please split this patch in smaller ones?
>  >>
>  >>
>  >> While I can split off some parts into separate patches, like
>  >> getting/setting ARM registers through VM events and the tools patches,
>  >> the other components are pretty tightly coupled and don't actually make
>  >> sense to split them. For example, enabling a monitor domctl for an event
>  >> without the VM event components doesn't make much sense. Vice verse for
>  >> the vm_event parts without being able to enable them.
>  >
>  >
>  > Well, the commit message does not mention half of the changes of this
> patch. Some changes like moving common code to arch specific code
> clearly needs explanation. It is the same for the fact that you only
> present a reduce set of registers to vm event for AArch64.
>
> This IMHO is not outstanding, it's the same on x86.

It documents the code and will help future reader to understand why we 
choose to only expose a smaller set of registers.

Note that the x86 structure is documented with: "Using a custom struct 
(no hvm_hw_cpu) so as to not fill the vm_event ring buffer too quickly". 
This is not the case for the ARM structure today.

>  >
>  > [...]
>  >
>  >
>  >>         +    if (
> current->domain->arch.monitor.privileged_call_enabled )
>  >>         +    {
>  >>         +        rc = monitor_smc(regs);
>  >>         +    }
>  >>
>  >>
>  >>     The bracket are not necessary.
>  >>
>  >>
>  >> Ack.
>  >>
>  >>
>  >>         +
>  >>         +    if ( rc != 1 )
>  >>
>  >>
>  >>     I think the code would be clearer if you introduce a define for "1".
>  >>
>  >>
>  >> Maybe not a define but calling the variable "handled" as we do on x86
>  >> would be more descriptive.
>  >
>  >
>  > IHMO, "handled" infers that the variable is boolean. This is not the
> case here as you could have negative value.
>
> It may be but thats the convention we have for this on x86 so symmetry
> is better then introducing a new define just for the ARM case.

Whilst I agree that we should use the same convention across all the 
architectures, nothing prevents you to update the x86 code and use the 
new define.

It does not make much sense to introduce code on ARM that may not be 
clearer just because x86 did it.

Anyway, Stefano may have a different view on this.

[...]

>  >
>  > [...]
>  >
>  >
>  >>     AArch64 provides 31 generate-purpose registers. Although, x29 and
>  >>     x30 are respectively used for fp and lr.
>  >>
>  >>
>  >> For vm_event it's not necessary to get all registers, rather it's just a
>  >> handful of selection that may be especially "useful" for introspection.
>  >
>  >
>  > How did you decide that only the first to xN registers are useful? It
> would be valid to have an SMC call using x20 for an arguments.
>  >
>  > Similarly, the hypercall convention for AArch64 makes use of x16
> which is not exposed to the vm event subsystem.
>
> Certainly, as I said, if a future application needs other registers to
> be sent here and can justify it, this can be adjusted.	
>
>  >
>  >
>  >> It's also important not to fill up the vm_event monitor ring with huge
>  >> request/response structs so even on x86 we only have a subset of the
>  >> registers. As right now there are no applications for aarch64, it's only
>  >> a guess of what registers would be "useful" and may be adjusted in
>  >> future versions as we start to have applications using this.
>  >
>  >
>  > Guessing the set of useful registers is usually not a good idea (see
> why above).
>  >
>
> Remember, the subscriber can always get/set the full set of registers
> when it needs to, so completeness here is not critical. You are missing
> the point that the space on the ring is limited and it can fill up fast
> when the full vCPU context is pushed on it for all events. Right now the
> x86 side is still larger so we have some room for additional registers
> to be sent when users of this system have a better view into what they
> find important. Which is IMHO not now.

I may misunderstand some parts of the vm event subsystem. To get/set the 
full set of registers, the user will have to use the 
DOMCTL_{set,get}vcpucontext, correct? So why does Xen expose a part of 
the vCPU context through the vm_event?

Regards,

-- 
Julien Grall

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