Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Jan Beulich]

>>> On 21.08.17 at 12:28,  wrote:
> Hi Jan,
> 
> On 7 August 2017 at 14:44, Jan Beulich  wrote:
> Bhupinder Thakur  08/07/17 10:55 AM >>>
>>>@@ -1148,6 +1149,24 @@ 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_INIT  0
>>>+uint32_t cmd;   /* XEN_DOMCTL_VUART_OP_* */
>>>+domid_t console_domid;  /* IN */
>>>+#define XEN_DOMCTL_VUART_TYPE_VPL011 0
>>>+uint32_t type;  /* IN - type of vuart.
>>>+ *  Currently only vpl011 supported.
>>>+ */
>>>+xen_pfn_t gfn;  /* IN - guest gfn to be used as a
>>>+ *  ring buffer.
>>>+ */
>>>+evtchn_port_t evtchn;   /* OUT - remote port of the event
>>>+ *   channel used for sending
>>>+ *   ring buffer events.
>>>+ */
>>>+};
>>
>> Please try to limit the amount of padding needed and make all remaining
>> padding fields explicit. That should then also make obvious that using
>> xen_pfn_t in domctl.h is not a good idea (the existing uses are bogus too
>> afaict - in the getpageframeinfo3 case the handler deals with the differing
>> width between 32- and 64-bit callers, and the cacheflush one is fine right
>> now only because it's ARM-only).
> 
> Please correct me if I understood your comment wrongly.
> 
> I believe you are referring to re-ordering the fields in the above
> structure so that the structure size is minimised.

Yes.

> Should I use type as unsigned long instead of xen_pfn_t in this structure?

No, uint64_aligned_t is what you want.

Jan


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

Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Bhupinder Thakur]

Hi Jan,

On 7 August 2017 at 14:44, Jan Beulich  wrote:
 Bhupinder Thakur  08/07/17 10:55 AM >>>
>>@@ -1148,6 +1149,24 @@ 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_INIT  0
>>+uint32_t cmd;   /* XEN_DOMCTL_VUART_OP_* */
>>+domid_t console_domid;  /* IN */
>>+#define XEN_DOMCTL_VUART_TYPE_VPL011 0
>>+uint32_t type;  /* IN - type of vuart.
>>+ *  Currently only vpl011 supported.
>>+ */
>>+xen_pfn_t gfn;  /* IN - guest gfn to be used as a
>>+ *  ring buffer.
>>+ */
>>+evtchn_port_t evtchn;   /* OUT - remote port of the event
>>+ *   channel used for sending
>>+ *   ring buffer events.
>>+ */
>>+};
>
> Please try to limit the amount of padding needed and make all remaining
> padding fields explicit. That should then also make obvious that using
> xen_pfn_t in domctl.h is not a good idea (the existing uses are bogus too
> afaict - in the getpageframeinfo3 case the handler deals with the differing
> width between 32- and 64-bit callers, and the cacheflush one is fine right
> now only because it's ARM-only).

Please correct me if I understood your comment wrongly.

I believe you are referring to re-ordering the fields in the above
structure so that the structure size is minimised.
Should I use type as unsigned long instead of xen_pfn_t in this structure?

Regards,
Bhupinder

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

Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Julien Grall]

Hi Bhupinder,

On 07/08/17 09:52, Bhupinder Thakur wrote:

diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 5e1fc60..784ec7f 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -44,6 +44,13 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
   libxl_domain_build_info *info,
   struct xc_dom_image *dom);

+/* perform any pending hardware initialization */
+_hidden
+int libxl__arch_build_dom_finish(libxl__gc *gc,
+ libxl_domain_build_info *info,
+ struct xc_dom_image *dom,
+ libxl__domain_build_state *state);
+
 /* build vNUMA vmemrange with arch specific information */
 _hidden
 int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..a33d3c9 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1038,6 +1038,27 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc 
*gc,
 return 0;
 }

+int libxl__arch_build_dom_finish(libxl__gc *gc,
+ libxl_domain_build_info *info,
+ struct xc_dom_image *dom,
+ libxl__domain_build_state *state)
+{
+int ret = 0;
+
+if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {


NIT: You could avoid on level of indentation if you do:

if ( info->arch_arm.vuart != LIBXL_VUART_TYPE_SBSA_UART )
  return 0;




+ret = xc_dom_vuart_init(CTX->xch,
+XEN_DOMCTL_VUART_TYPE_VPL011,
+dom->guest_domid,
+dom->console_domid,
+dom->vuart_gfn,
+>vuart_port);
+if (ret < 0)
+LOG(ERROR, "xc_dom_vuart_init failed\n");
+}
+
+return ret;
+}
+
 int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
   uint32_t domid,
   libxl_domain_build_info *info,


[...]


diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index db6838d..c7f650e 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -5,9 +5,11 @@
  */

 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -119,6 +121,46 @@ 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;
+struct xen_domctl_vuart_op *vuart_op = >u.vuart_op;
+
+switch(vuart_op->cmd)
+{
+case XEN_DOMCTL_VUART_OP_INIT:
+
+if ( !d->creation_finished )
+{
+if (vuart_op->type == XEN_DOMCTL_VUART_TYPE_VPL011)


Coding style.


+{
+struct vpl011_init_info info;


The indentation is wrong.


+
+info.console_domid = vuart_op->console_domid;
+info.gfn = _gfn(vuart_op->gfn);
+
+rc = domain_vpl011_init(d, );
+if ( !rc )
+{
+vuart_op->evtchn = info.evtchn;
+rc = __copy_to_guest(u_domctl, domctl, 1);
+}
+}
+else
+rc = -EINVAL;


I think this one should be -EOPNOTSUPP.


+}
+else
+rc = - EPERM;


Can we please avoid the number of nested if? Maybe by introducing a 
function to handle the domctl.



+
+break;
+
+default:
+rc = -EINVAL;


Same here.


+break;
+}
+
+return rc;
+}
 default:
 {
 int rc;


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Julien Grall]

Hi,

On 08/08/17 14:30, Wei Liu wrote:

On Tue, Aug 08, 2017 at 02:11:08PM +0100, Wei Liu wrote:

+else
+rc = -EINVAL;


Indentation.


Ignore this please.


You were right, the indentation is wrong :). It is 8 spaces rather than 4.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Wei Liu]

On Tue, Aug 08, 2017 at 02:11:08PM +0100, Wei Liu wrote:
> > +else
> > +rc = -EINVAL;
> 
> Indentation.

Ignore this please.

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

Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Wei Liu]

On Mon, Aug 07, 2017 at 02:22:58PM +0530, Bhupinder Thakur wrote:
> Add a new domctl API to initialize vpl011. It takes the GFN and console
> backend domid as input and returns an event channel to be used for
> sending and receiving events from Xen.
> 
> Xen will communicate with xenconsole using GFN as the ring buffer and
> the event channel to transmit and receive pl011 data on the guest domain's
> behalf.
> 
> Signed-off-by: Bhupinder Thakur 
> ---
[...]
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 3bab4e8..899bbd4 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -343,6 +343,31 @@ int xc_domain_get_guest_width(xc_interface *xch, 
> uint32_t domid,
>  return 0;
>  }
>  
> +int xc_dom_vuart_init(xc_interface *xch,
> +  uint32_t type,
> +  domid_t domid,
> +  domid_t console_domid,
> +  xen_pfn_t gfn,
> +  evtchn_port_t *evtchn)
> +{
> +DECLARE_DOMCTL;
> +int rc = 0;
> +
> +domctl.cmd = XEN_DOMCTL_vuart_op;
> +domctl.domain = (domid_t)domid;

The cast is pointless.

> +domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_INIT;
> +domctl.u.vuart_op.type = type;
> +domctl.u.vuart_op.console_domid = console_domid;
> +domctl.u.vuart_op.gfn = gfn;
> +
> +if ( (rc = do_domctl(xch, )) < 0 )
> +return rc;
> +
> +*evtchn = domctl.u.vuart_op.evtchn;
> +
> +return rc;
> +}
> +
[...]
>  
> +int libxl__arch_build_dom_finish(libxl__gc *gc,
> + libxl_domain_build_info *info,
> + struct xc_dom_image *dom,
> + libxl__domain_build_state *state)
> +{
> +int ret = 0;

int rc = 0;

> +
> +if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
> +ret = xc_dom_vuart_init(CTX->xch,
> +XEN_DOMCTL_VUART_TYPE_VPL011,
> +dom->guest_domid,
> +dom->console_domid,
> +dom->vuart_gfn,
> +>vuart_port);
> +if (ret < 0)

rc = ERROR_FAIL;

> +LOG(ERROR, "xc_dom_vuart_init failed\n");
> +}
> +
> +return ret;

return rc;

For reasons, see libxl/CODING_STYLE.

> +}
> +
[...]
>uint32_t domid,
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index db6838d..c7f650e 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -5,9 +5,11 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -119,6 +121,46 @@ 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;
> +struct xen_domctl_vuart_op *vuart_op = >u.vuart_op;
> +
> +switch(vuart_op->cmd)

Coding style.

> +{
> +case XEN_DOMCTL_VUART_OP_INIT:
> +
> +if ( !d->creation_finished )
> +{
> +if (vuart_op->type == XEN_DOMCTL_VUART_TYPE_VPL011)
> +{
> +struct vpl011_init_info info;
> +
> +info.console_domid = vuart_op->console_domid;
> +info.gfn = _gfn(vuart_op->gfn);
> +
> +rc = domain_vpl011_init(d, );
> +if ( !rc )
> +{
> +vuart_op->evtchn = info.evtchn;
> +rc = __copy_to_guest(u_domctl, domctl, 1);
> +}
> +}
> +else
> +rc = -EINVAL;

Indentation.

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

Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Jan Beulich]

>>> Bhupinder Thakur  08/07/17 10:55 AM >>>
>@@ -1148,6 +1149,24 @@ 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_INIT  0
>+uint32_t cmd;   /* XEN_DOMCTL_VUART_OP_* */
>+domid_t console_domid;  /* IN */
>+#define XEN_DOMCTL_VUART_TYPE_VPL011 0
>+uint32_t type;  /* IN - type of vuart.
>+ *  Currently only vpl011 supported.
>+ */
>+xen_pfn_t gfn;  /* IN - guest gfn to be used as a
>+ *  ring buffer.
>+ */
>+evtchn_port_t evtchn;   /* OUT - remote port of the event
>+ *   channel used for sending
>+ *   ring buffer events.
>+ */
>+};

Please try to limit the amount of padding needed and make all remaining
padding fields explicit. That should then also make obvious that using
xen_pfn_t in domctl.h is not a good idea (the existing uses are bogus too
afaict - in the getpageframeinfo3 case the handler deals with the differing
width between 32- and 64-bit callers, and the cacheflush one is fine right
now only because it's ARM-only).

Jan


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

[Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Bhupinder Thakur]

Add a new domctl API to initialize vpl011. It takes the GFN and console
backend domid as input and returns an event channel to be used for
sending and receiving events from Xen.

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

Signed-off-by: Bhupinder Thakur 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Andrew Cooper 
CC: George Dunlap 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Julien Grall 

Changes since v6:
- Renamed the vuart initialization function to a generic name xc_dom_vuart_init 
- Used domid_t as a type instead of uint32_t for domid
- Checking the vuart type explicitly against vpl011 enum value

Changes since v5:
- xc_dom_vpl011_init() will be compiled for both x86/arm architectures as there
  is nothing architecture specific in this function. This function will return 
  error when called for x86.
- Fixed coding style issues in libxl.

Changes since v4:
- Removed libxl__arch_domain_create_finish().
- Added a new function libxl__arch_build_dom_finish(), which is called at the 
last
  in libxl__build_dom(). This function calls the vpl011 initialization function 
now.

Changes since v3:
- Added a new arch specific function libxl__arch_domain_create_finish(), which
  calls the vpl011 initialization function. For x86 this function does not do
  anything.
- domain_vpl011_init() takes a pointer to a structure which contains all the 
  required information such as console_domid, gfn instead of passing parameters
  separately.
- Dropped a DOMCTL API defined for de-initializing vpl011 as that should be
  taken care when the domain is destroyed (and not dependent on userspace 
  libraries/applications).

Changes since v2:
- Replaced the DOMCTL APIs defined for get/set of event channel and GFN with 
  a set of DOMCTL APIs for initializing and de-initializing vpl011 emulation.

 tools/libxc/include/xenctrl.h | 20 
 tools/libxc/xc_domain.c   | 25 +
 tools/libxl/libxl_arch.h  |  7 +++
 tools/libxl/libxl_arm.c   | 21 +
 tools/libxl/libxl_dom.c   |  4 
 tools/libxl/libxl_x86.c   |  8 
 xen/arch/arm/domain.c |  6 ++
 xen/arch/arm/domctl.c | 42 ++
 xen/include/public/domctl.h   | 21 +
 9 files changed, 154 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c7710b8..35bbb3b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -886,6 +886,26 @@ int xc_vcpu_getcontext(xc_interface *xch,
vcpu_guest_context_any_t *ctxt);
 
 /**
+ * This function initializes the vuart emulation and returns
+ * the event to be used by the backend for communicating with
+ * the emulation code.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * #parm type type of vuart
+ * @parm domid the domain to get information from
+ * @parm console_domid the domid of the backend console
+ * @parm gfn the guest pfn to be used as the ring buffer
+ * @parm evtchn the event channel to be used for events
+ * @return 0 on success, negative error on failure
+ */
+int xc_dom_vuart_init(xc_interface *xch,
+  uint32_t type,
+  domid_t domid,
+  domid_t console_domid,
+  xen_pfn_t gfn,
+  evtchn_port_t *evtchn);
+
+/**
  * 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 3bab4e8..899bbd4 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -343,6 +343,31 @@ int xc_domain_get_guest_width(xc_interface *xch, uint32_t 
domid,
 return 0;
 }
 
+int xc_dom_vuart_init(xc_interface *xch,
+  uint32_t type,
+  domid_t domid,
+  domid_t console_domid,
+  xen_pfn_t gfn,
+  evtchn_port_t *evtchn)
+{
+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_INIT;
+domctl.u.vuart_op.type = type;
+domctl.u.vuart_op.console_domid = console_domid;
+domctl.u.vuart_op.gfn = gfn;
+
+if ( (rc = do_domctl(xch, )) < 0 )
+return rc;
+
+*evtchn = domctl.u.vuart_op.evtchn;
+
+return rc;
+}
+
 int xc_domain_getinfo(xc_interface *xch,