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

Re: [Xen-devel] [PATCH 02/10 v2] xen/arm: vpl011: Add new vuart domctl interface to setup pfn and evtchn



Hi Bhupinder,

Title: Please avoid the term pfn and use gfn or mfn.

On 28/04/17 17:01, Bhupinder Thakur wrote:
1. Add two new domctl API to:
    - Allocate a new event channel for sending/receiving events to/from Xen.
    - Map the PFN allocted by the toolstack to be used as IN/OUT ring buffers.

Ditto and s/allocted/allocated/


Xen will communicate with xenconsole over the ring buffer and the event
channel to transmit and receive pl011 data on guest domain's behalf.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
---

Changes since v1:

- Replaced the hvm interface with domctl interface

 tools/libxc/include/xenctrl.h | 26 ++++++++++++++++++++++++++
 tools/libxc/xc_domain.c       | 39 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/domctl.c         | 20 ++++++++++++++++++++
 xen/arch/x86/domctl.c         |  4 ++++
 xen/include/public/domctl.h   | 11 +++++++++++
 5 files changed, 100 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 1629f41..bebfe7d 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -886,6 +886,32 @@ int xc_vcpu_getcontext(xc_interface *xch,
                        vcpu_guest_context_any_t *ctxt);

 /**
+ * This function returns information about the pfn and the event channel

Ditto.

+ * to be used for setting up a virtual uart console.

s/uart/UART/

+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid the domain to get information from
+ * @parm vuart_pfn the pfn to be used for the ring buffer

Ditto for pfn.

+ * @return 0 on success, negative error on failure
+ */
+int xc_domain_vuart_set_pfn(xc_interface *xch,

s/pfn/gfn/

+                            uint32_t domid,
+                            uint32_t vuart_pfn);

s/pfn/gfn/ also a frame number may not fit in uint32_t. You want to use xen_pfn_t.

+
+/**
+ * This function returns information about the pfn and the event channel
+ * to be used for setting up a virtual console.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid the domain to get information from
+ * @parm vuart_evtchn the event channel to be used for console events
+ * @return 0 on success, negative error on failure
+ */
+int xc_domain_vuart_get_evtchn(xc_interface *xch,
+                               uint32_t domid,
+                               uint32_t *vuart_evtchn);

You want to use evtchn_port_t here.

+
+/**
  * This function returns information about the XSAVE state of a particular
  * vcpu of a domain. If extstate->size and extstate->xfeature_mask are 0,
  * the call is considered a query to retrieve them and the buffer is not
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 00909ad4..943f202 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -343,6 +343,45 @@ int xc_domain_get_guest_width(xc_interface *xch, uint32_t 
domid,
     return 0;
 }

+int xc_domain_vuart_set_pfn(xc_interface *xch,
+                            uint32_t domid,
+                            uint32_t vuart_pfn)

See all my remarks above.

+{
+    DECLARE_DOMCTL;
+    int rc = 0;
+
+    domctl.cmd = XEN_DOMCTL_vuart_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_SET_PFN;
+    domctl.u.vuart_op.pfn = vuart_pfn;
+
+    if ( (rc = do_domctl(xch, &domctl)) < 0 )
+        return rc;
+
+    return rc;
+}
+
+int xc_domain_vuart_get_evtchn(xc_interface *xch,
+                               uint32_t domid,
+                               uint32_t *vuart_evtchn)

See all my remarks above.

+{
+    DECLARE_DOMCTL;
+    int rc = 0;
+
+       *vuart_evtchn = -1;
+
+    domctl.cmd = XEN_DOMCTL_vuart_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_GET_EVTCHN;
+
+    if ( (rc = do_domctl(xch, &domctl)) < 0 )
+        return rc;
+
+    *vuart_evtchn = domctl.u.vuart_op.evtchn;
+
+    return rc;
+}
+
 int xc_domain_getinfo(xc_interface *xch,
                       uint32_t first_domid,
                       unsigned int max_doms,
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 971caec..e400f87 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -10,6 +10,7 @@
 #include <xen/sched.h>
 #include <xen/hypercall.h>
 #include <xen/iocap.h>
+#include <xen/guest_access.h>
 #include <xsm/xsm.h>
 #include <public/domctl.h>

@@ -119,6 +120,25 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
domain *d,
         d->disable_migrate = domctl->u.disable_migrate.disable;
         return 0;

+    case XEN_DOMCTL_vuart_op:
+    {
+        int rc;
+        if ( d->arch.vpl011.initialized )

You can avoid one level of "if" if you do:

if ( !d->arch.vpl011.initialized )
  return -EINVAL.

if ( domctl->u.vuart_op.cmd == .. )

+        {
+            if ( domctl->u.vuart_op.cmd == XEN_DOMCTL_VUART_OP_SET_PFN )

I would prefer if you use a switch case for the cmd, this would make easier to return an error if the command is not correct rather assuming that all others commands means "getting the event channel".

+            {
+                rc = vpl011_map_guest_page(d, domctl->u.vuart_op.pfn);

There is nothing to prevent vpl011_map_guest_page to be called twice. So you would end up to leak memory if XEN_DOMCTL_VUART_OP_SET_PFN is called twice.

+            }
+            else
+            {
+                domctl->u.vuart_op.evtchn = d->arch.vpl011.evtchn;
+                rc = __copy_to_guest(u_domctl, domctl, 1);
+            }
+            return rc;
+        }
+        else
+            return -EINVAL;

I don't think -EINVAL is the correct value if not initialized. It would be better to be -ENOSYS.

+    }
     default:
     {
         int rc;
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index e104be2..49e907d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1465,6 +1465,10 @@ long arch_do_domctl(
         recalculate_cpuid_policy(d);
         break;

+    case XEN_DOMCTL_vuart_op:
+        ret = -EOPNOTSUPP;
+        break;
+
     default:
         ret = iommu_do_domctl(domctl, d, u_domctl);
         break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e6cf211..8bee0c3 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1138,6 +1138,15 @@ struct xen_domctl_psr_cat_op {
     uint32_t target;    /* IN */
     uint64_t data;      /* IN/OUT */
 };
+
+struct xen_domctl_vuart_op {
+#define XEN_DOMCTL_VUART_OP_SET_PFN      0
+#define XEN_DOMCTL_VUART_OP_GET_EVTCHN   1
+        uint32_t cmd;       /* XEN_DOMCTL_VUART_OP_* */
+        uint32_t pfn;       /* IN */

This should be xen_pfn_t and ...

+        uint32_t evtchn;    /* OUT */

... evtchn_port_t.

+};
+
 typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);

@@ -1218,6 +1227,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_monitor_op                    77
 #define XEN_DOMCTL_psr_cat_op                    78
 #define XEN_DOMCTL_soft_reset                    79
+#define XEN_DOMCTL_vuart_op                      80
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1280,6 +1290,7 @@ struct xen_domctl {
         struct xen_domctl_psr_cmt_op        psr_cmt_op;
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_cat_op        psr_cat_op;
+        struct xen_domctl_vuart_op          vuart_op;
         uint8_t                             pad[128];
     } u;
 };


Cheers,

--
Julien Grall

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

 


Rackspace

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