Re: [Xen-devel] [PATCH 9/9] xen-access: Add support for vm_event_ng interface

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/05/2019 15:18, Petre Pircalabu wrote:
> +#define to_channels(_vme) container_of((_vme), vm_event_channels_t, vme)
> +
> +static int vm_event_channels_init(xc_interface *xch, xenevtchn_handle *xce,
> +                                  domid_t domain_id, vm_event_ops_t *ops,
> +                                  vm_event_t **vm_event)
> +{
> +    vm_event_channels_t *impl = NULL;
> +    int rc, i, num_vcpus;
> +    xc_dominfo_t info;
> +    unsigned long nr_frames;
> +
> +    /* Get the numbers of vcpus */
> +    rc = xc_domain_getinfo(xch, domain_id, 1, &info);
> +    if ( rc != 1 )

|| info.domid != domain_id

The API is idiotic.  However... (see several below)

> +    {
> +        ERROR("xc_domain_getinfo failed. rc = %d\n", rc);
> +        return rc;
> +    }
> +
> +    num_vcpus = info.max_vcpu_id + 1;
> +
> +    impl = (vm_event_channels_t *)calloc(1, sizeof(vm_event_channels_t) +
> +                                            num_vcpus * sizeof(int));

This is C, not C++

> +    if ( !impl )
> +        return -ENOMEM;
> +
> +    impl->num_vcpus = num_vcpus;
> +
> +    impl->fmem = xenforeignmemory_open(0,0);

Spaces and NULL.

> +    if ( !impl->fmem )
> +    {
> +        rc = -errno;
> +        goto err;
> +    }
> +
> +    rc = xc_monitor_ng_create(xch, domain_id);
> +    if ( rc )
> +    {
> +        ERROR("Failed to enable monitor");
> +        goto err;
> +    }
> +
> +    nr_frames = PFN_UP(num_vcpus * sizeof(struct vm_event_slot));
> +
> +    impl->fres = xenforeignmemory_map_resource(impl->fmem, domain_id,
> +                                               XENMEM_resource_vm_event,
> +                                               XEN_VM_EVENT_TYPE_MONITOR, 0,
> +                                               nr_frames, 
> (void*)&impl->slots,
> +                                               PROT_READ | PROT_WRITE, 0);

... one big problem with the existing vm_event interface is that it
requires domctls.

In particular, I was hoping we could take the opportunity of this new
interface to see if we could also remove the use of all unstable interfaces.

Do you happen to know offhand which non-stable hypercalls are currently
needed for introspection purposes?

> +vm_event_ops_t channel_ops = {
> +    .get_request = vm_event_channels_get_request,
> +    .put_response = vm_event_channels_put_response,
> +    .notify_port = vm_event_channels_notify_port,
> +    .init = vm_event_channels_init,
> +    .teardown = vm_event_channels_teardown

Here and elsewhere, a trailing comma please.  It simplifies future diffs.

> diff --git a/tools/tests/xen-access/vm-event.c 
> b/tools/tests/xen-access/vm-event.c
> new file mode 100644
> index 0000000..ffd5476
> --- /dev/null
> +++ b/tools/tests/xen-access/vm-event.c
>
> +static int vm_event_ring_init(xc_interface *xch, xenevtchn_handle *xce,
> +                              domid_t domain_id, vm_event_ops_t *ops,
> +                              vm_event_t **vm_event)
> +{
> +    vm_event_ring_t *impl;
> +    int rc;
> +
> +    impl = (vm_event_ring_t*) calloc (1, sizeof(vm_event_ring_t));
> +    if ( !impl )
> +        return -ENOMEM;
> +
> +    /* Enable mem_access */
> +    impl->ring_page = xc_monitor_enable(xch, domain_id, &impl->evtchn_port);
> +    if ( impl->ring_page == NULL )
> +    {
> +        switch ( errno ) {

Style, seeing as you're moving it anyway.

> diff --git a/tools/tests/xen-access/xen-access.c 
> b/tools/tests/xen-access/xen-access.c
> index 6aaee16..267d163 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -35,12 +35,8 @@
>  #include <time.h>
>  #include <signal.h>
>  #include <unistd.h>
> -#include <sys/mman.h>
>  #include <poll.h>
> -
> -#include <xenctrl.h>
> -#include <xenevtchn.h>
> -#include <xen/vm_event.h>
> +#include <getopt.h>
>  
>  #include <xen-tools/libs.h>
>  
> @@ -51,9 +47,7 @@
>  #define START_PFN 0ULL
>  #endif
>  
> -#define DPRINTF(a, b...) fprintf(stderr, a, ## b)
> -#define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
> -#define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
> +#include "xen-access.h"
>  
>  /* From xen/include/asm-x86/processor.h */
>  #define X86_TRAP_DEBUG  1
> @@ -62,32 +56,14 @@
>  /* From xen/include/asm-x86/x86-defns.h */
>  #define X86_CR4_PGE        0x00000080 /* enable global pages */
>  
> -typedef struct vm_event {
> -    domid_t domain_id;
> -    xenevtchn_handle *xce_handle;
> -    int port;
> -    vm_event_back_ring_t back_ring;
> -    uint32_t evtchn_port;
> -    void *ring_page;
> -} vm_event_t;
> -
> -typedef struct xenaccess {
> -    xc_interface *xc_handle;
> -
> -    xen_pfn_t max_gpfn;
> -
> -    vm_event_t vm_event;
> -} xenaccess_t;
> -
>  static int interrupted;
> -bool evtchn_bind = 0, evtchn_open = 0, mem_access_enable = 0;
>  
>  static void close_handler(int sig)
>  {
>      interrupted = sig;
>  }
>  
> -int xc_wait_for_event_or_timeout(xc_interface *xch, xenevtchn_handle *xce, 
> unsigned long ms)
> +static int xc_wait_for_event_or_timeout(xc_interface *xch, xenevtchn_handle 
> *xce, unsigned long ms)

This looks like the patch wants at least splitting into two.  The first
doing cleanup/renaming/etc, and the second doing the interface
splitting.  Perhaps even a 3rd for the getopt() change.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel