[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Hi Tamas,

Can you configure your email client to quote properly? I.e using ">" rather than tabulation to show the quoting.

On 03/05/2016 19:48, Tamas K Lengyel wrote:

On Tue, May 3, 2016 at 5:31 AM, Julien Grall <julien.grall@xxxxxxx
<mailto:julien.grall@xxxxxxx>> 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@xxxxxxxxxxxxx
        Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx
        Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx
        Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx
        Cc: Wei Liu <wei.liu2@xxxxxxxxxx <mailto:wei.liu2@xxxxxxxxxx>>
        Cc: Julien Grall <julien.grall@xxxxxxx
        Cc: Keir Fraser <keir@xxxxxxx <mailto:keir@xxxxxxx>>
        Cc: Jan Beulich <jbeulich@xxxxxxxx <mailto:jbeulich@xxxxxxxx>>
        Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx

              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.

In any case, there is too many logical changes in this patch, which makes difficult to review it. So please split this patch in smaller chunk.


        +    if ( current->domain->arch.monitor.privileged_call_enabled )
        +    {
        +        rc = monitor_smc(regs);
        +    }

    The bracket are not necessary.


        +    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.

        +    {
        +        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));

    This check cannot work for AArch64 guest.

For HSR_EC_SMC32 there is already
GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr)); and for HSR_EC_SMC64 there
is GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr)); before calling
do_trap_smc. So are you saying that check is wrong for AArch64 as it is
right now in unstable? Also, is there any reason those checks are
opposite of each other?

No, I am saying that your check is wrong here. psr_mode_is_32bit returns true if the guest was running in 32-bit mode, else false when running in 64-bit mode.

The check are opposites each other because the exception SMC64 can only be taken from an AArch64 state, and SMC32 from an AArch32 State.


    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.

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).


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.