From: Julien Grall <julien.grall@arm.com>
To: Bhupinder Thakur <bhupinder.thakur@linaro.org>,
xen-devel@lists.xenproject.org
Cc: Wei Liu <wei.liu2@citrix.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Ian Jackson <ian.jackson@eu.citrix.com>,
Jan Beulich <jbeulich@suse.com>,
Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 02/10 v2] xen/arm: vpl011: Add new vuart domctl interface to setup pfn and evtchn
Date: Wed, 3 May 2017 11:14:38 +0100 [thread overview]
Message-ID: <70159e01-6c66-f9b1-188c-93c83b0fcd1a@arm.com> (raw)
In-Reply-To: <1493395284-18430-3-git-send-email-bhupinder.thakur@linaro.org>
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@linaro.org>
> ---
>
> 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@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2017-05-03 10:14 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-28 16:01 [PATCH 00/10 v2] pl011 emulation support in Xen Bhupinder Thakur
2017-04-28 16:01 ` [PATCH 01/10 v2] xen/arm: vpl011: Add pl011 uart emulation " Bhupinder Thakur
2017-04-28 19:08 ` Stefano Stabellini
2017-05-02 7:34 ` Jan Beulich
2017-05-02 16:02 ` Julien Grall
2017-05-05 11:18 ` Bhupinder Thakur
2017-05-05 13:27 ` Julien Grall
2017-05-06 5:20 ` Bhupinder Thakur
2017-04-28 16:01 ` [PATCH 02/10 v2] xen/arm: vpl011: Add new vuart domctl interface to setup pfn and evtchn Bhupinder Thakur
2017-04-28 19:23 ` Stefano Stabellini
2017-05-02 7:39 ` Jan Beulich
2017-05-02 9:47 ` Bhupinder Thakur
2017-05-02 7:47 ` Jan Beulich
2017-05-02 9:58 ` Bhupinder Thakur
2017-05-02 11:22 ` Jan Beulich
2017-05-03 10:14 ` Julien Grall [this message]
2017-04-28 16:01 ` [PATCH 03/10 v2] xen/arm: vpl011: Enable pl011 emulation for a guest domain in Xen Bhupinder Thakur
2017-04-28 19:15 ` Stefano Stabellini
2017-05-02 7:48 ` Jan Beulich
2017-05-02 15:20 ` Bhupinder Thakur
2017-05-02 15:23 ` Julien Grall
2017-05-03 10:22 ` Julien Grall
2017-05-03 10:47 ` Jan Beulich
2017-05-05 7:10 ` Bhupinder Thakur
2017-05-05 13:43 ` Julien Grall
2017-05-08 6:34 ` Bhupinder Thakur
2017-05-11 10:35 ` Wei Liu
2017-04-28 16:01 ` [PATCH 04/10 v2] xen/arm: vpl011: Add support for vuart in libxl Bhupinder Thakur
2017-04-28 21:45 ` Stefano Stabellini
2017-05-03 10:27 ` Julien Grall
2017-04-28 16:01 ` [PATCH 05/10 v2] xen/arm: vpl011: Allocate a new PFN in the toolstack for vuart Bhupinder Thakur
2017-04-28 21:48 ` Stefano Stabellini
2017-04-28 16:01 ` [PATCH 06/10 v2] xen/arm: vpl011: Add vuart ring-buf and evtchn to xenstore Bhupinder Thakur
2017-04-28 21:57 ` Stefano Stabellini
2017-05-01 11:21 ` Bhupinder Thakur
2017-05-01 17:56 ` Stefano Stabellini
2017-05-03 11:00 ` Bhupinder Thakur
2017-05-03 22:35 ` Stefano Stabellini
2017-05-04 19:37 ` Bhupinder Thakur
2017-05-04 20:38 ` Stefano Stabellini
2017-05-05 9:52 ` Bhupinder Thakur
2017-05-05 18:59 ` Stefano Stabellini
2017-05-08 5:37 ` Bhupinder Thakur
2017-04-28 16:01 ` [PATCH 07/10 v2] xen/arm: vpl011: Add support for vuart in xenconsole Bhupinder Thakur
2017-04-28 23:10 ` Stefano Stabellini
2017-05-08 6:18 ` Bhupinder Thakur
2017-04-28 16:01 ` [PATCH 08/10 v2] xen/arm: vpl011: Add a new vuart console type to xenconsole client Bhupinder Thakur
2017-04-28 22:01 ` Stefano Stabellini
2017-04-28 16:01 ` [PATCH 09/10 v2] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree Bhupinder Thakur
2017-05-03 10:38 ` Julien Grall
2017-05-08 6:43 ` Bhupinder Thakur
2017-04-28 16:01 ` [PATCH 10/10 v2] xen/arm: vpl011: Update documentation for vuart console support Bhupinder Thakur
2017-04-28 22:06 ` Stefano Stabellini
2017-05-11 10:32 ` [PATCH 00/10 v2] pl011 emulation support in Xen Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70159e01-6c66-f9b1-188c-93c83b0fcd1a@arm.com \
--to=julien.grall@arm.com \
--cc=andrew.cooper3@citrix.com \
--cc=bhupinder.thakur@linaro.org \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=sstabellini@kernel.org \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.