Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

2020-09-24 Thread Oleksandr 



On 24.09.20 14:03, Jan Beulich wrote:

Hi Jan


On 22.09.2020 18:46, Oleksandr wrote:

On 14.09.20 18:56, Jan Beulich wrote:
Hi Jan


On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:

--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -150,6 +150,18 @@ do_dm_op(
   unsigned int nr_bufs,
   XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
   
+struct dmop_args {

+domid_t domid;
+unsigned int nr_bufs;
+/* Reserve enough buf elements for all current hypercalls. */
+struct xen_dm_op_buf buf[2];
+};
+
+int arch_dm_op(struct xen_dm_op *op,
+   struct domain *d,
+   const struct dmop_args *op_args,
+   bool *const_op);
+
   #ifdef CONFIG_HYPFS
   extern long
   do_hypfs_op(

There are exactly two CUs which need to see these two declarations.
Personally I think they should go into a new header, or at least
into one that half-way fits (from the pov of its other contents)
and doesn't get included by half the code base. But maybe it's
just me ...

I am afraid, I didn't get why this header is not suitable for keeping
this stuff...

While I have no major objection against exposing arch_dm_op() to more
than just the relevant CUs, I don't think I'd like to see struct
dmop_args becoming visible to "everyone", and in particular changes
to it causing a re-build of (almost) everything.


Thank you for clarification, I got your point

--
Regards,

Oleksandr Tyshchenko

Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

2020-09-24 Thread Jan Beulich 
On 22.09.2020 18:46, Oleksandr wrote:
> 
> On 14.09.20 18:56, Jan Beulich wrote:
> Hi Jan
> 
>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>> --- a/xen/include/xen/hypercall.h
>>> +++ b/xen/include/xen/hypercall.h
>>> @@ -150,6 +150,18 @@ do_dm_op(
>>>   unsigned int nr_bufs,
>>>   XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
>>>   
>>> +struct dmop_args {
>>> +domid_t domid;
>>> +unsigned int nr_bufs;
>>> +/* Reserve enough buf elements for all current hypercalls. */
>>> +struct xen_dm_op_buf buf[2];
>>> +};
>>> +
>>> +int arch_dm_op(struct xen_dm_op *op,
>>> +   struct domain *d,
>>> +   const struct dmop_args *op_args,
>>> +   bool *const_op);
>>> +
>>>   #ifdef CONFIG_HYPFS
>>>   extern long
>>>   do_hypfs_op(
>> There are exactly two CUs which need to see these two declarations.
>> Personally I think they should go into a new header, or at least
>> into one that half-way fits (from the pov of its other contents)
>> and doesn't get included by half the code base. But maybe it's
>> just me ...
> 
> I am afraid, I didn't get why this header is not suitable for keeping 
> this stuff...

While I have no major objection against exposing arch_dm_op() to more
than just the relevant CUs, I don't think I'd like to see struct
dmop_args becoming visible to "everyone", and in particular changes
to it causing a re-build of (almost) everything.

Jan

Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

2020-09-23 Thread Oleksandr 



On 23.09.20 20:35, Julien Grall wrote:

Hi Julien



On 10/09/2020 21:22, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 


I believe I am the original author of this code. So this needs to be 
fixed accordingly.


Sorry, will fix.


--
Regards,

Oleksandr Tyshchenko

Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

2020-09-23 Thread Julien Grall 




On 10/09/2020 21:22, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 


I believe I am the original author of this code. So this needs to be 
fixed accordingly.




As a lot of x86 code can be re-used on Arm later on, this patch
splits devicemodel support into common and arch specific parts.

Also update XSM code a bit to let DM op be used on Arm.

This support is going to be used on Arm to be able run device
emulator outside of Xen hypervisor.

Signed-off-by: Julien Grall 
Signed-off-by: Oleksandr Tyshchenko 

---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Changes RFC -> V1:
- update XSM, related changes were pulled from:
  [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM 
features
---
---
  xen/arch/x86/hvm/dm.c   | 287 +++-
  xen/common/Makefile |   1 +
  xen/common/dm.c | 287 
  xen/include/xen/hypercall.h |  12 ++
  xen/include/xsm/dummy.h |   4 +-
  xen/include/xsm/xsm.h   |   6 +-
  xen/xsm/dummy.c |   2 +-
  xen/xsm/flask/hooks.c   |   5 +-
  8 files changed, 327 insertions(+), 277 deletions(-)
  create mode 100644 xen/common/dm.c

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 5ce484a..6ae535e 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -29,13 +29,6 @@
  
  #include 
  
-struct dmop_args {

-domid_t domid;
-unsigned int nr_bufs;
-/* Reserve enough buf elements for all current hypercalls. */
-struct xen_dm_op_buf buf[2];
-};
-
  static bool _raw_copy_from_guest_buf_offset(void *dst,
  const struct dmop_args *args,
  unsigned int buf_idx,
@@ -338,148 +331,20 @@ static int inject_event(struct domain *d,
  return 0;
  }
  
-static int dm_op(const struct dmop_args *op_args)

+int arch_dm_op(struct xen_dm_op *op, struct domain *d,
+   const struct dmop_args *op_args, bool *const_op)
  {
-struct domain *d;
-struct xen_dm_op op;
-bool const_op = true;
  long rc;
-size_t offset;
-
-static const uint8_t op_size[] = {
-[XEN_DMOP_create_ioreq_server]  = sizeof(struct 
xen_dm_op_create_ioreq_server),
-[XEN_DMOP_get_ioreq_server_info]= sizeof(struct 
xen_dm_op_get_ioreq_server_info),
-[XEN_DMOP_map_io_range_to_ioreq_server] = sizeof(struct 
xen_dm_op_ioreq_server_range),
-[XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct 
xen_dm_op_ioreq_server_range),
-[XEN_DMOP_set_ioreq_server_state]   = sizeof(struct 
xen_dm_op_set_ioreq_server_state),
-[XEN_DMOP_destroy_ioreq_server] = sizeof(struct 
xen_dm_op_destroy_ioreq_server),
-[XEN_DMOP_track_dirty_vram] = sizeof(struct 
xen_dm_op_track_dirty_vram),
-[XEN_DMOP_set_pci_intx_level]   = sizeof(struct 
xen_dm_op_set_pci_intx_level),
-[XEN_DMOP_set_isa_irq_level]= sizeof(struct 
xen_dm_op_set_isa_irq_level),
-[XEN_DMOP_set_pci_link_route]   = sizeof(struct 
xen_dm_op_set_pci_link_route),
-[XEN_DMOP_modified_memory]  = sizeof(struct 
xen_dm_op_modified_memory),
-[XEN_DMOP_set_mem_type] = sizeof(struct 
xen_dm_op_set_mem_type),
-[XEN_DMOP_inject_event] = sizeof(struct 
xen_dm_op_inject_event),
-[XEN_DMOP_inject_msi]   = sizeof(struct 
xen_dm_op_inject_msi),
-[XEN_DMOP_map_mem_type_to_ioreq_server] = sizeof(struct 
xen_dm_op_map_mem_type_to_ioreq_server),
-[XEN_DMOP_remote_shutdown]  = sizeof(struct 
xen_dm_op_remote_shutdown),
-[XEN_DMOP_relocate_memory]  = sizeof(struct 
xen_dm_op_relocate_memory),
-[XEN_DMOP_pin_memory_cacheattr] = sizeof(struct 
xen_dm_op_pin_memory_cacheattr),
-};
-
-rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
-if ( rc )
-return rc;
-
-if ( !is_hvm_domain(d) )
-goto out;
-
-rc = xsm_dm_op(XSM_DM_PRIV, d);
-if ( rc )
-goto out;
-
-offset = offsetof(struct xen_dm_op, u);
-
-rc = -EFAULT;
-if ( op_args->buf[0].size < offset )
-goto out;
-
-if ( copy_from_guest_offset((void *)&op, op_args->buf[0].h, 0, offset) )
-goto out;
-
-if ( op.op >= ARRAY_SIZE(op_size) )
-{
-rc = -EOPNOTSUPP;
-goto out;
-}
-
-op.op = array_index_nospec(op.op, ARRAY_SIZE(op_size));
-
-if ( op_args->buf[0].size < offset + op_size[op.op] )
-goto out;
-
-if ( copy_from_guest_offset((void *)&op.u, op_args->buf[0].h, offset,
-op_size[op.op]) )
-goto out;
-
-rc = -EINVAL;
-if ( op.pad )
-goto

Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

2020-09-22 Thread Oleksandr 



On 14.09.20 18:56, Jan Beulich wrote:
Hi Jan


On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:

--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -150,6 +150,18 @@ do_dm_op(
  unsigned int nr_bufs,
  XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
  
+struct dmop_args {

+domid_t domid;
+unsigned int nr_bufs;
+/* Reserve enough buf elements for all current hypercalls. */
+struct xen_dm_op_buf buf[2];
+};
+
+int arch_dm_op(struct xen_dm_op *op,
+   struct domain *d,
+   const struct dmop_args *op_args,
+   bool *const_op);
+
  #ifdef CONFIG_HYPFS
  extern long
  do_hypfs_op(

There are exactly two CUs which need to see these two declarations.
Personally I think they should go into a new header, or at least
into one that half-way fits (from the pov of its other contents)
and doesn't get included by half the code base. But maybe it's
just me ...


I am afraid, I didn't get why this header is not suitable for keeping 
this stuff...


But, I don't against moving this into a new header (probably dm.h?)

--
Regards,

Oleksandr Tyshchenko

Re: [PATCH V1 07/16] xen/dm: Make x86's DM feature common

2020-09-14 Thread Jan Beulich 
On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -150,6 +150,18 @@ do_dm_op(
>  unsigned int nr_bufs,
>  XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs);
>  
> +struct dmop_args {
> +domid_t domid;
> +unsigned int nr_bufs;
> +/* Reserve enough buf elements for all current hypercalls. */
> +struct xen_dm_op_buf buf[2];
> +};
> +
> +int arch_dm_op(struct xen_dm_op *op,
> +   struct domain *d,
> +   const struct dmop_args *op_args,
> +   bool *const_op);
> +
>  #ifdef CONFIG_HYPFS
>  extern long
>  do_hypfs_op(

There are exactly two CUs which need to see these two declarations.
Personally I think they should go into a new header, or at least
into one that half-way fits (from the pov of its other contents)
and doesn't get included by half the code base. But maybe it's
just me ...

Jan