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

Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features





On 05/08/2020 20:30, Oleksandr wrote:

On 05.08.20 17:12, Julien Grall wrote:
Hi,

Hi Julien



On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

This patch makes possible to forward Guest MMIO accesses
to a device emulator on Arm and enables that support for
Arm64.

Also update XSM code a bit to let DM op be used on Arm.
New arch DM op will be introduced in the follow-up patch.

Please note, at the moment build on Arm32 is broken
(see cmpxchg usage in hvm_send_buffered_ioreq()) if someone
wants to enable CONFIG_IOREQ_SERVER due to the lack of
cmpxchg_64 support on Arm32.

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

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
---
  tools/libxc/xc_dom_arm.c        |  25 +++++++---
  xen/arch/arm/Kconfig            |   1 +
  xen/arch/arm/Makefile           |   2 +
  xen/arch/arm/dm.c               |  34 +++++++++++++
  xen/arch/arm/domain.c           |   9 ++++
  xen/arch/arm/hvm.c              |  46 +++++++++++++++++-
  xen/arch/arm/io.c               |  67 +++++++++++++++++++++++++-
  xen/arch/arm/ioreq.c            |  86 +++++++++++++++++++++++++++++++++
  xen/arch/arm/traps.c            |  17 +++++++
  xen/common/memory.c             |   5 +-
  xen/include/asm-arm/domain.h    |  80 +++++++++++++++++++++++++++++++
  xen/include/asm-arm/hvm/ioreq.h | 103 ++++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/mmio.h      |   1 +
  xen/include/asm-arm/p2m.h       |   7 +--
  xen/include/xsm/dummy.h         |   4 +-
  xen/include/xsm/xsm.h           |   6 +--
  xen/xsm/dummy.c                 |   2 +-
  xen/xsm/flask/hooks.c           |   5 +-
  18 files changed, 476 insertions(+), 24 deletions(-)
  create mode 100644 xen/arch/arm/dm.c
  create mode 100644 xen/arch/arm/ioreq.c
  create mode 100644 xen/include/asm-arm/hvm/ioreq.h

It feels to me the patch is doing quite a few things that are indirectly related. Can this be split to make the review easier?

I would like at least the following split from the series:
   - The tools changes
   - The P2M changes
   - The HVMOP plumbing (if we still require them)
Sure, will split.
However, I don't quite understand where we should leave HVMOP plumbing.

I think they will need to be droppped if we decide to use the acquire interface.

If I understand correctly the suggestion was to switch to acquire interface instead (which requires a Linux version to be v4.17 at least)?

This was the suggestion.

I suspect, this is all about "xen/privcmd: add IOCTL_PRIVCMD_MMAP_RESOURCE" support for Linux?

Correct.

What is this function supposed to do?
Agree, sounds confusing a bit. I assume it is supposed to complete Guest MMIO access after finishing emulation.

Shall I rename it to something appropriate (maybe by adding ioreq prefix)?

How about ioreq_handle_complete_mmio()?

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 9283e5e..0000477 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -8,6 +8,7 @@
   */
    #include <xen/domain_page.h>
+#include <xen/hvm/ioreq.h>
  #include <xen/types.h>
  #include <xen/lib.h>
  #include <xen/mm.h>
@@ -30,10 +31,6 @@
  #include <public/memory.h>
  #include <xsm/xsm.h>
  -#ifdef CONFIG_IOREQ_SERVER
-#include <xen/hvm/ioreq.h>
-#endif
-

Why do you remove something your just introduced?
The reason I guarded that header is to make "xen/mm: Make x86's XENMEM_resource_ioreq_server handling common" (previous) patch buildable on Arm without arch IOREQ header added yet. I tried to make sure that the result after each patch was buildable to retain bisectability. As current patch adds Arm IOREQ specific bits (including header), that guard could be removed as not needed anymore.
I agree we want to have the build bisectable. However, I am still puzzled why it is necessary to remove the #ifdef and move it earlier in the list.

Do you mind to provide more details?

[...]

+
+bool handle_mmio(void);
+
+static inline bool handle_pio(uint16_t port, unsigned int size, int dir)
+{
+    /* XXX */

Can you expand this TODO? What do you expect to do?
I didn't expect this to be called on Arm. Sorry, I am not sure l have an idea how to handle this properly. I would keep it unimplemented until a real reason.
Will expand TODO.

Let see how the conversation on patch#1 goes about PIO vs MMIO.



+    BUG();
+    return true;
+}
+
+static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
+{
+    return p->addr;
+}

I understand that the x86 version is more complex as it check p->df. However, aside reducing the complexity, I am not sure why we would want to diverge it.

After all, IOREQ is now meant to be a common feature.
Well, no objections at all.
Could you please clarify how could 'df' (Direction Flag?) be handled/used on Arm?

On x86, this is used by 'rep' instruction to tell the direction to iterate (forward or backward).

On Arm, all the accesses to MMIO region will do a single memory access. So for now, we can safely always set to 0.

I see that try_fwd_ioserv() always sets it 0. Or I need to just reuse x86's helpers as is, which (together with count = df = 0) will result in what we actually have here?
AFAIU, both count and df should be 0 on Arm.



+
+static inline int p2m_set_ioreq_server(struct domain *d,
+                                       unsigned int flags,
+                                       struct hvm_ioreq_server *s)
+{
+    return -EOPNOTSUPP;
+}

This should be defined in p2m.h. But I am not even sure what it is meant for. Can you expand it?

ok, will move.


In this series I tried to make as much IOREQ code common as possible and avoid complicating things, in order to achieve that a few stubs were added here. Please note, that I also considered splitting into arch parts. But some functions couldn't be split easily. This one is called from common hvm_destroy_ioreq_server() with flag being 0 (which will result in unmapping ioreq server from p2m type on x86).
I could add a comment describing why this stub is present here.

Sorry if I wasn't clear. I wasn't asking why the stub is there but what should be the expected implementation of the function.

In particular, you are returning -EOPNOTSUPP. The only reason you are getting away from trouble is because the caller doesn't check the return.

Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?





+
+static inline void msix_write_completion(struct vcpu *v)
+{
+}
+
+static inline void handle_realmode_completion(void)
+{
+    ASSERT_UNREACHABLE();
+}

realmode is very x86 specific. So I don't think this function should be called from common code. It might be worth considering to split handle_hvm_io_completion() is 2 parts: common and arch specific.

I agree with you that realmode is x86 specific and looks not good in Arm header.
It is not a problem of looking good or not. Instead, it is about abstraction. A developper shouldn't need to understand all the other architectures we support in order to follow the common code.

I was thinking how to split handle_hvm_io_completion() gracefully but I failed find a good solution for that, so decided to add two stubs (msix_write_completion and handle_realmode_completion) on Arm. I could add a comment describing why they are here if appropriate. But if you think they shouldn't be called from the common code in any way, I will try to split it.

I am not entirely sure what msix_write_completion is meant to do on x86. Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could help?

Regarding handle_realmode_completion, I would add a new stub:

arch_ioreq_handle_io_completion() that is called from the default case of the switch.

On x86 it would be implemented as:

 switch (io_completion)
 {
    case HVMIO_realmode_completion:
      ...
    default:
      ASSERT_UNREACHABLE();
 }

On Arm, it would be implemented as:

  ASSERT_UNREACHABLE();

Cheers,

--
Julien Grall



 


Rackspace

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