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

Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common




On 04.08.20 10:45, Paul Durrant wrote:

Hi Paul

-----Original Message-----
From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
Sent: 03 August 2020 19:21
To: xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Jan Beulich 
<jbeulich@xxxxxxxx>; Andrew
Cooper <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné 
<roger.pau@xxxxxxxxxx>;
George Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson 
<ian.jackson@xxxxxxxxxxxxx>; Julien Grall
<julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Paul Durrant 
<paul@xxxxxxx>; Jun
Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Tim Deegan 
<tim@xxxxxxx>; Julien
Grall <julien.grall@xxxxxxx>
Subject: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

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

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

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>
---
  xen/arch/x86/Kconfig            |    1 +
  xen/arch/x86/hvm/dm.c           |    2 +-
  xen/arch/x86/hvm/emulate.c      |    2 +-
  xen/arch/x86/hvm/hvm.c          |    2 +-
  xen/arch/x86/hvm/io.c           |    2 +-
  xen/arch/x86/hvm/ioreq.c        | 1431 +--------------------------------------
  xen/arch/x86/hvm/stdvga.c       |    2 +-
  xen/arch/x86/hvm/vmx/realmode.c |    1 +
  xen/arch/x86/hvm/vmx/vvmx.c     |    2 +-
  xen/arch/x86/mm.c               |    2 +-
  xen/arch/x86/mm/shadow/common.c |    2 +-
  xen/common/Kconfig              |    3 +
  xen/common/Makefile             |    1 +
  xen/common/hvm/Makefile         |    1 +
  xen/common/hvm/ioreq.c          | 1430 ++++++++++++++++++++++++++++++++++++++
  xen/include/asm-x86/hvm/ioreq.h |   45 +-
  xen/include/asm-x86/hvm/vcpu.h  |    7 -
  xen/include/xen/hvm/ioreq.h     |   89 +++
  18 files changed, 1575 insertions(+), 1450 deletions(-)
  create mode 100644 xen/common/hvm/Makefile
  create mode 100644 xen/common/hvm/ioreq.c
  create mode 100644 xen/include/xen/hvm/ioreq.h
You need to adjust the MAINTAINERS file since there will now be common 'I/O 
EMULATION' code. Since I wrote most of ioreq.c, please retain me as a 
maintainer of the common code.

Oh, I completely forgot about MAINTAINERS file. Sure, I will update file and retain you.



[snip]
@@ -1528,13 +143,19 @@ static int hvm_access_cf8(
      return X86EMUL_UNHANDLEABLE;
  }

-void hvm_ioreq_init(struct domain *d)
+void arch_hvm_ioreq_init(struct domain *d)
  {
      spin_lock_init(&d->arch.hvm.ioreq_server.lock);

      register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
  }

+void arch_hvm_ioreq_destroy(struct domain *d)
+{
+    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
+        return;
There's not really a lot of point in this. I think an empty function here would 
be ok.

ok


+struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
+                                                 ioreq_t *p)
+{
+    struct hvm_ioreq_server *s;
+    uint8_t type;
+    uint64_t addr;
+    unsigned int id;
+
+    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
+        return NULL;
+
+    hvm_get_ioreq_server_range_type(d, p, &type, &addr);
Looking at this, I think it would make more sense to fold the check of p->type 
into hvm_get_ioreq_server_range_type() and have it return success/failure.

ok, will update.


diff --git a/xen/include/xen/hvm/ioreq.h b/xen/include/xen/hvm/ioreq.h
new file mode 100644
index 0000000..40b7b5e
--- /dev/null
+++ b/xen/include/xen/hvm/ioreq.h
@@ -0,0 +1,89 @@
+/*
+ * hvm.h: Hardware virtual machine assist interface definitions.
+ *
+ * Copyright (c) 2016 Citrix Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __HVM_IOREQ_H__
+#define __HVM_IOREQ_H__
+
+#include <xen/sched.h>
+
+#include <asm/hvm/ioreq.h>
+
+#define GET_IOREQ_SERVER(d, id) \
+    (d)->arch.hvm.ioreq_server.server[id]
+
+static inline struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
+                                                        unsigned int id)
+{
+    if ( id >= MAX_NR_IOREQ_SERVERS )
+        return NULL;
+
+    return GET_IOREQ_SERVER(d, id);
+}
+
+static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
+{
+    return ioreq->state == STATE_IOREQ_READY &&
+           !ioreq->data_is_ptr &&
+           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
+}
I don't think having this in common code is correct. The short-cut of not 
completing PIO reads seems somewhat x86 specific. Does ARM even have the 
concept of PIO?

I am not 100% sure here, but it seems that doesn't have.

Shall I make hvm_ioreq_needs_completion() per arch? Arm variant would have the same implementation, but without "ioreq->type != IOREQ_TYPE_PIO" check...

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

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