Mailing List Archive

[PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

As a lot of x86 code can be re-used on Arm later on, this
patch makes some preparation to x86/hvm/ioreq.c before moving
to the common code. This way we will get a verbatim copy for
a code movement in subsequent patch (arch/x86/hvm/ioreq.c
will be *just* renamed to common/ioreq).

This patch does the following:
1. Introduce *inline* arch_hvm_ioreq_init(), arch_hvm_ioreq_destroy(),
arch_hvm_io_completion(), arch_hvm_destroy_ioreq_server() and
hvm_ioreq_server_get_type_addr() to abstract arch specific materials.
2 Make hvm_map_mem_type_to_ioreq_server() *inline*. It is not going
to be called from the common code.
3. Make get_ioreq_server() global. It is going to be called from
a few places.
4. Add IOREQ_STATUS_* #define-s and update candidates for moving.
5. Re-order #include-s alphabetically.

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

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
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:
- new patch, was split from:
"[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common"
- fold the check of p->type into hvm_get_ioreq_server_range_type()
and make it return success/failure
- remove relocate_portio_handler() call from arch_hvm_ioreq_destroy()
in arch/x86/hvm/ioreq.c
- introduce arch_hvm_destroy_ioreq_server()/arch_handle_hvm_io_completion()

Changes V1 -> V2:
- update patch description
- make arch functions inline and put them into arch header
to achieve a truly rename by the subsequent patch
- return void in arch_hvm_destroy_ioreq_server()
- return bool in arch_hvm_ioreq_destroy()
- bring relocate_portio_handler() back to arch_hvm_ioreq_destroy()
- rename IOREQ_IO* to IOREQ_STATUS*
- remove *handle* from arch_handle_hvm_io_completion()
- re-order #include-s alphabetically
- rename hvm_get_ioreq_server_range_type() to hvm_ioreq_server_get_type_addr()
and add "const" to several arguments
---
xen/arch/x86/hvm/ioreq.c | 153 +++++--------------------------------
xen/include/asm-x86/hvm/ioreq.h | 165 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 184 insertions(+), 134 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 1cc27df..d3433d7 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1,5 +1,5 @@
/*
- * hvm/io.c: hardware virtual machine I/O emulation
+ * ioreq.c: hardware virtual machine I/O emulation
*
* Copyright (c) 2016 Citrix Systems Inc.
*
@@ -17,21 +17,18 @@
*/

#include <xen/ctype.h>
+#include <xen/domain.h>
+#include <xen/event.h>
#include <xen/init.h>
+#include <xen/irq.h>
#include <xen/lib.h>
-#include <xen/trace.h>
+#include <xen/paging.h>
#include <xen/sched.h>
-#include <xen/irq.h>
#include <xen/softirq.h>
-#include <xen/domain.h>
-#include <xen/event.h>
-#include <xen/paging.h>
+#include <xen/trace.h>
#include <xen/vpci.h>

-#include <asm/hvm/emulate.h>
-#include <asm/hvm/hvm.h>
#include <asm/hvm/ioreq.h>
-#include <asm/hvm/vmx/vmx.h>

#include <public/hvm/ioreq.h>
#include <public/hvm/params.h>
@@ -48,8 +45,8 @@ static void set_ioreq_server(struct domain *d, unsigned int id,
#define GET_IOREQ_SERVER(d, id) \
(d)->arch.hvm.ioreq_server.server[id]

-static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
- unsigned int id)
+struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
+ unsigned int id)
{
if ( id >= MAX_NR_IOREQ_SERVERS )
return NULL;
@@ -209,19 +206,8 @@ bool handle_hvm_io_completion(struct vcpu *v)
return handle_pio(vio->io_req.addr, vio->io_req.size,
vio->io_req.dir);

- case HVMIO_realmode_completion:
- {
- struct hvm_emulate_ctxt ctxt;
-
- hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
- vmx_realmode_emulate_one(&ctxt);
- hvm_emulate_writeback(&ctxt);
-
- break;
- }
default:
- ASSERT_UNREACHABLE();
- break;
+ return arch_hvm_io_completion(io_completion);
}

return true;
@@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)

domain_pause(d);

- p2m_set_ioreq_server(d, 0, s);
+ arch_hvm_destroy_ioreq_server(s);

hvm_ioreq_server_disable(s);

@@ -1080,54 +1066,6 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
return rc;
}

-/*
- * Map or unmap an ioreq server to specific memory type. For now, only
- * HVMMEM_ioreq_server is supported, and in the future new types can be
- * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
- * currently, only write operations are to be forwarded to an ioreq server.
- * Support for the emulation of read operations can be added when an ioreq
- * server has such requirement in the future.
- */
-int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
- uint32_t type, uint32_t flags)
-{
- struct hvm_ioreq_server *s;
- int rc;
-
- if ( type != HVMMEM_ioreq_server )
- return -EINVAL;
-
- if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
- return -EINVAL;
-
- spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
-
- s = get_ioreq_server(d, id);
-
- rc = -ENOENT;
- if ( !s )
- goto out;
-
- rc = -EPERM;
- if ( s->emulator != current->domain )
- goto out;
-
- rc = p2m_set_ioreq_server(d, flags, s);
-
- out:
- spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
-
- if ( rc == 0 && flags == 0 )
- {
- struct p2m_domain *p2m = p2m_get_hostp2m(d);
-
- if ( read_atomic(&p2m->ioreq.entry_count) )
- p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
- }
-
- return rc;
-}
-
int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
bool enabled)
{
@@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
struct hvm_ioreq_server *s;
unsigned int id;

- if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
+ if ( !arch_hvm_ioreq_destroy(d) )
return;

spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
@@ -1243,50 +1181,13 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
ioreq_t *p)
{
struct hvm_ioreq_server *s;
- uint32_t cf8;
uint8_t type;
uint64_t addr;
unsigned int id;

- if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
+ if ( hvm_ioreq_server_get_type_addr(d, p, &type, &addr) )
return NULL;

- cf8 = d->arch.hvm.pci_cf8;
-
- if ( p->type == IOREQ_TYPE_PIO &&
- (p->addr & ~3) == 0xcfc &&
- CF8_ENABLED(cf8) )
- {
- uint32_t x86_fam;
- pci_sbdf_t sbdf;
- unsigned int reg;
-
- reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
-
- /* PCI config data cycle */
- type = XEN_DMOP_IO_RANGE_PCI;
- addr = ((uint64_t)sbdf.sbdf << 32) | reg;
- /* AMD extended configuration space access? */
- if ( CF8_ADDR_HI(cf8) &&
- d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
- (x86_fam = get_cpu_family(
- d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 &&
- x86_fam < 0x17 )
- {
- uint64_t msr_val;
-
- if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
- (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
- addr |= CF8_ADDR_HI(cf8);
- }
- }
- else
- {
- type = (p->type == IOREQ_TYPE_PIO) ?
- XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
- addr = p->addr;
- }
-
FOR_EACH_IOREQ_SERVER(d, id, s)
{
struct rangeset *r;
@@ -1351,7 +1252,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
pg = iorp->va;

if ( !pg )
- return X86EMUL_UNHANDLEABLE;
+ return IOREQ_STATUS_UNHANDLED;

/*
* Return 0 for the cases we can't deal with:
@@ -1381,7 +1282,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
break;
default:
gdprintk(XENLOG_WARNING, "unexpected ioreq size: %u\n", p->size);
- return X86EMUL_UNHANDLEABLE;
+ return IOREQ_STATUS_UNHANDLED;
}

spin_lock(&s->bufioreq_lock);
@@ -1391,7 +1292,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
{
/* The queue is full: send the iopacket through the normal path. */
spin_unlock(&s->bufioreq_lock);
- return X86EMUL_UNHANDLEABLE;
+ return IOREQ_STATUS_UNHANDLED;
}

pg->buf_ioreq[pg->ptrs.write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
@@ -1422,7 +1323,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
notify_via_xen_event_channel(d, s->bufioreq_evtchn);
spin_unlock(&s->bufioreq_lock);

- return X86EMUL_OKAY;
+ return IOREQ_STATUS_HANDLED;
}

int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
@@ -1438,7 +1339,7 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
return hvm_send_buffered_ioreq(s, proto_p);

if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
- return X86EMUL_RETRY;
+ return IOREQ_STATUS_RETRY;

list_for_each_entry ( sv,
&s->ioreq_vcpu_list,
@@ -1478,11 +1379,11 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
notify_via_xen_event_channel(d, port);

sv->pending = true;
- return X86EMUL_RETRY;
+ return IOREQ_STATUS_RETRY;
}
}

- return X86EMUL_UNHANDLEABLE;
+ return IOREQ_STATUS_UNHANDLED;
}

unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
@@ -1496,30 +1397,18 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
if ( !s->enabled )
continue;

- if ( hvm_send_ioreq(s, p, buffered) == X86EMUL_UNHANDLEABLE )
+ if ( hvm_send_ioreq(s, p, buffered) == IOREQ_STATUS_UNHANDLED )
failed++;
}

return failed;
}

-static int hvm_access_cf8(
- int dir, unsigned int port, unsigned int bytes, uint32_t *val)
-{
- struct domain *d = current->domain;
-
- if ( dir == IOREQ_WRITE && bytes == 4 )
- d->arch.hvm.pci_cf8 = *val;
-
- /* We always need to fall through to the catch all emulator */
- return X86EMUL_UNHANDLEABLE;
-}
-
void hvm_ioreq_init(struct domain *d)
{
spin_lock_init(&d->arch.hvm.ioreq_server.lock);

- register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
+ arch_hvm_ioreq_init(d);
}

/*
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index e2588e9..376e2ef 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -19,6 +19,165 @@
#ifndef __ASM_X86_HVM_IOREQ_H__
#define __ASM_X86_HVM_IOREQ_H__

+#include <asm/hvm/emulate.h>
+#include <asm/hvm/vmx/vmx.h>
+
+#include <public/hvm/params.h>
+
+struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
+ unsigned int id);
+
+static inline bool arch_hvm_io_completion(enum hvm_io_completion io_completion)
+{
+ switch ( io_completion )
+ {
+ case HVMIO_realmode_completion:
+ {
+ struct hvm_emulate_ctxt ctxt;
+
+ hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
+ vmx_realmode_emulate_one(&ctxt);
+ hvm_emulate_writeback(&ctxt);
+
+ break;
+ }
+
+ default:
+ ASSERT_UNREACHABLE();
+ break;
+ }
+
+ return true;
+}
+
+/* Called when target domain is paused */
+static inline void arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
+{
+ p2m_set_ioreq_server(s->target, 0, s);
+}
+
+/*
+ * Map or unmap an ioreq server to specific memory type. For now, only
+ * HVMMEM_ioreq_server is supported, and in the future new types can be
+ * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
+ * currently, only write operations are to be forwarded to an ioreq server.
+ * Support for the emulation of read operations can be added when an ioreq
+ * server has such requirement in the future.
+ */
+static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
+ ioservid_t id,
+ uint32_t type,
+ uint32_t flags)
+{
+ struct hvm_ioreq_server *s;
+ int rc;
+
+ if ( type != HVMMEM_ioreq_server )
+ return -EINVAL;
+
+ if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
+ return -EINVAL;
+
+ spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
+
+ s = get_ioreq_server(d, id);
+
+ rc = -ENOENT;
+ if ( !s )
+ goto out;
+
+ rc = -EPERM;
+ if ( s->emulator != current->domain )
+ goto out;
+
+ rc = p2m_set_ioreq_server(d, flags, s);
+
+ out:
+ spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
+
+ if ( rc == 0 && flags == 0 )
+ {
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+ if ( read_atomic(&p2m->ioreq.entry_count) )
+ p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
+ }
+
+ return rc;
+}
+
+static inline int hvm_ioreq_server_get_type_addr(const struct domain *d,
+ const ioreq_t *p,
+ uint8_t *type,
+ uint64_t *addr)
+{
+ uint32_t cf8 = d->arch.hvm.pci_cf8;
+
+ if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
+ return -EINVAL;
+
+ if ( p->type == IOREQ_TYPE_PIO &&
+ (p->addr & ~3) == 0xcfc &&
+ CF8_ENABLED(cf8) )
+ {
+ uint32_t x86_fam;
+ pci_sbdf_t sbdf;
+ unsigned int reg;
+
+ reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
+
+ /* PCI config data cycle */
+ *type = XEN_DMOP_IO_RANGE_PCI;
+ *addr = ((uint64_t)sbdf.sbdf << 32) | reg;
+ /* AMD extended configuration space access? */
+ if ( CF8_ADDR_HI(cf8) &&
+ d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
+ (x86_fam = get_cpu_family(
+ d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 &&
+ x86_fam < 0x17 )
+ {
+ uint64_t msr_val;
+
+ if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
+ (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
+ *addr |= CF8_ADDR_HI(cf8);
+ }
+ }
+ else
+ {
+ *type = (p->type == IOREQ_TYPE_PIO) ?
+ XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
+ *addr = p->addr;
+ }
+
+ return 0;
+}
+
+static inline int hvm_access_cf8(
+ int dir, unsigned int port, unsigned int bytes, uint32_t *val)
+{
+ struct domain *d = current->domain;
+
+ if ( dir == IOREQ_WRITE && bytes == 4 )
+ d->arch.hvm.pci_cf8 = *val;
+
+ /* We always need to fall through to the catch all emulator */
+ return X86EMUL_UNHANDLEABLE;
+}
+
+static inline void arch_hvm_ioreq_init(struct domain *d)
+{
+ register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
+}
+
+static inline bool arch_hvm_ioreq_destroy(struct domain *d)
+{
+ if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
+ return false;
+
+ return true;
+}
+
bool hvm_io_pending(struct vcpu *v);
bool handle_hvm_io_completion(struct vcpu *v);
bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
@@ -38,8 +197,6 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
uint32_t type, uint64_t start,
uint64_t end);
-int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
- uint32_t type, uint32_t flags);
int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
bool enabled);

@@ -55,6 +212,10 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);

void hvm_ioreq_init(struct domain *d);

+#define IOREQ_STATUS_HANDLED X86EMUL_OKAY
+#define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
+#define IOREQ_STATUS_RETRY X86EMUL_RETRY
+
#endif /* __ASM_X86_HVM_IOREQ_H__ */

/*
--
2.7.4
RE: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
> -----Original Message-----
> From: Oleksandr Tyshchenko <olekstysh@gmail.com>
> Sent: 15 October 2020 17:44
> To: xen-devel@lists.xenproject.org
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Paul Durrant <paul@xen.org>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common
>
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> As a lot of x86 code can be re-used on Arm later on, this
> patch makes some preparation to x86/hvm/ioreq.c before moving
> to the common code. This way we will get a verbatim copy for
> a code movement in subsequent patch (arch/x86/hvm/ioreq.c
> will be *just* renamed to common/ioreq).
>
> This patch does the following:
> 1. Introduce *inline* arch_hvm_ioreq_init(), arch_hvm_ioreq_destroy(),
> arch_hvm_io_completion(), arch_hvm_destroy_ioreq_server() and
> hvm_ioreq_server_get_type_addr() to abstract arch specific materials.
> 2 Make hvm_map_mem_type_to_ioreq_server() *inline*. It is not going
> to be called from the common code.
> 3. Make get_ioreq_server() global. It is going to be called from
> a few places.
> 4. Add IOREQ_STATUS_* #define-s and update candidates for moving.
> 5. Re-order #include-s alphabetically.
>
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>
>
> ---
> 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:
> - new patch, was split from:
> "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common"
> - fold the check of p->type into hvm_get_ioreq_server_range_type()
> and make it return success/failure
> - remove relocate_portio_handler() call from arch_hvm_ioreq_destroy()
> in arch/x86/hvm/ioreq.c
> - introduce arch_hvm_destroy_ioreq_server()/arch_handle_hvm_io_completion()
>
> Changes V1 -> V2:
> - update patch description
> - make arch functions inline and put them into arch header
> to achieve a truly rename by the subsequent patch
> - return void in arch_hvm_destroy_ioreq_server()
> - return bool in arch_hvm_ioreq_destroy()
> - bring relocate_portio_handler() back to arch_hvm_ioreq_destroy()
> - rename IOREQ_IO* to IOREQ_STATUS*
> - remove *handle* from arch_handle_hvm_io_completion()
> - re-order #include-s alphabetically
> - rename hvm_get_ioreq_server_range_type() to hvm_ioreq_server_get_type_addr()
> and add "const" to several arguments
> ---
> xen/arch/x86/hvm/ioreq.c | 153 +++++--------------------------------
> xen/include/asm-x86/hvm/ioreq.h | 165 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 184 insertions(+), 134 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 1cc27df..d3433d7 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1,5 +1,5 @@
> /*
> - * hvm/io.c: hardware virtual machine I/O emulation
> + * ioreq.c: hardware virtual machine I/O emulation
> *
> * Copyright (c) 2016 Citrix Systems Inc.
> *
> @@ -17,21 +17,18 @@
> */
>
> #include <xen/ctype.h>
> +#include <xen/domain.h>
> +#include <xen/event.h>
> #include <xen/init.h>
> +#include <xen/irq.h>
> #include <xen/lib.h>
> -#include <xen/trace.h>
> +#include <xen/paging.h>
> #include <xen/sched.h>
> -#include <xen/irq.h>
> #include <xen/softirq.h>
> -#include <xen/domain.h>
> -#include <xen/event.h>
> -#include <xen/paging.h>
> +#include <xen/trace.h>
> #include <xen/vpci.h>
>
> -#include <asm/hvm/emulate.h>
> -#include <asm/hvm/hvm.h>
> #include <asm/hvm/ioreq.h>
> -#include <asm/hvm/vmx/vmx.h>
>
> #include <public/hvm/ioreq.h>
> #include <public/hvm/params.h>
> @@ -48,8 +45,8 @@ static void set_ioreq_server(struct domain *d, unsigned int id,
> #define GET_IOREQ_SERVER(d, id) \
> (d)->arch.hvm.ioreq_server.server[id]
>
> -static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> - unsigned int id)
> +struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> + unsigned int id)
> {
> if ( id >= MAX_NR_IOREQ_SERVERS )
> return NULL;
> @@ -209,19 +206,8 @@ bool handle_hvm_io_completion(struct vcpu *v)
> return handle_pio(vio->io_req.addr, vio->io_req.size,
> vio->io_req.dir);
>
> - case HVMIO_realmode_completion:
> - {
> - struct hvm_emulate_ctxt ctxt;
> -
> - hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> - vmx_realmode_emulate_one(&ctxt);
> - hvm_emulate_writeback(&ctxt);
> -
> - break;
> - }
> default:
> - ASSERT_UNREACHABLE();
> - break;
> + return arch_hvm_io_completion(io_completion);
> }
>
> return true;
> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>
> domain_pause(d);
>
> - p2m_set_ioreq_server(d, 0, s);
> + arch_hvm_destroy_ioreq_server(s);
>
> hvm_ioreq_server_disable(s);
>
> @@ -1080,54 +1066,6 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
> return rc;
> }
>
> -/*
> - * Map or unmap an ioreq server to specific memory type. For now, only
> - * HVMMEM_ioreq_server is supported, and in the future new types can be
> - * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
> - * currently, only write operations are to be forwarded to an ioreq server.
> - * Support for the emulation of read operations can be added when an ioreq
> - * server has such requirement in the future.
> - */
> -int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
> - uint32_t type, uint32_t flags)
> -{
> - struct hvm_ioreq_server *s;
> - int rc;
> -
> - if ( type != HVMMEM_ioreq_server )
> - return -EINVAL;
> -
> - if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> - return -EINVAL;
> -
> - spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> -
> - s = get_ioreq_server(d, id);
> -
> - rc = -ENOENT;
> - if ( !s )
> - goto out;
> -
> - rc = -EPERM;
> - if ( s->emulator != current->domain )
> - goto out;
> -
> - rc = p2m_set_ioreq_server(d, flags, s);
> -
> - out:
> - spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> -
> - if ( rc == 0 && flags == 0 )
> - {
> - struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -
> - if ( read_atomic(&p2m->ioreq.entry_count) )
> - p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> - }
> -
> - return rc;
> -}
> -
> int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
> bool enabled)
> {
> @@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
> struct hvm_ioreq_server *s;
> unsigned int id;
>
> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> + if ( !arch_hvm_ioreq_destroy(d) )
> return;
>
> spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> @@ -1243,50 +1181,13 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> ioreq_t *p)
> {
> struct hvm_ioreq_server *s;
> - uint32_t cf8;
> uint8_t type;
> uint64_t addr;
> unsigned int id;
>
> - if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> + if ( hvm_ioreq_server_get_type_addr(d, p, &type, &addr) )
> return NULL;
>
> - cf8 = d->arch.hvm.pci_cf8;
> -
> - if ( p->type == IOREQ_TYPE_PIO &&
> - (p->addr & ~3) == 0xcfc &&
> - CF8_ENABLED(cf8) )
> - {
> - uint32_t x86_fam;
> - pci_sbdf_t sbdf;
> - unsigned int reg;
> -
> - reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
> -
> - /* PCI config data cycle */
> - type = XEN_DMOP_IO_RANGE_PCI;
> - addr = ((uint64_t)sbdf.sbdf << 32) | reg;
> - /* AMD extended configuration space access? */
> - if ( CF8_ADDR_HI(cf8) &&
> - d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
> - (x86_fam = get_cpu_family(
> - d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 &&
> - x86_fam < 0x17 )
> - {
> - uint64_t msr_val;
> -
> - if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> - (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> - addr |= CF8_ADDR_HI(cf8);
> - }
> - }
> - else
> - {
> - type = (p->type == IOREQ_TYPE_PIO) ?
> - XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
> - addr = p->addr;
> - }
> -
> FOR_EACH_IOREQ_SERVER(d, id, s)
> {
> struct rangeset *r;
> @@ -1351,7 +1252,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
> pg = iorp->va;
>
> if ( !pg )
> - return X86EMUL_UNHANDLEABLE;
> + return IOREQ_STATUS_UNHANDLED;
>
> /*
> * Return 0 for the cases we can't deal with:
> @@ -1381,7 +1282,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
> break;
> default:
> gdprintk(XENLOG_WARNING, "unexpected ioreq size: %u\n", p->size);
> - return X86EMUL_UNHANDLEABLE;
> + return IOREQ_STATUS_UNHANDLED;
> }
>
> spin_lock(&s->bufioreq_lock);
> @@ -1391,7 +1292,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
> {
> /* The queue is full: send the iopacket through the normal path. */
> spin_unlock(&s->bufioreq_lock);
> - return X86EMUL_UNHANDLEABLE;
> + return IOREQ_STATUS_UNHANDLED;
> }
>
> pg->buf_ioreq[pg->ptrs.write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
> @@ -1422,7 +1323,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
> notify_via_xen_event_channel(d, s->bufioreq_evtchn);
> spin_unlock(&s->bufioreq_lock);
>
> - return X86EMUL_OKAY;
> + return IOREQ_STATUS_HANDLED;
> }
>
> int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
> @@ -1438,7 +1339,7 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
> return hvm_send_buffered_ioreq(s, proto_p);
>
> if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> - return X86EMUL_RETRY;
> + return IOREQ_STATUS_RETRY;
>
> list_for_each_entry ( sv,
> &s->ioreq_vcpu_list,
> @@ -1478,11 +1379,11 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
> notify_via_xen_event_channel(d, port);
>
> sv->pending = true;
> - return X86EMUL_RETRY;
> + return IOREQ_STATUS_RETRY;
> }
> }
>
> - return X86EMUL_UNHANDLEABLE;
> + return IOREQ_STATUS_UNHANDLED;
> }
>
> unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
> @@ -1496,30 +1397,18 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
> if ( !s->enabled )
> continue;
>
> - if ( hvm_send_ioreq(s, p, buffered) == X86EMUL_UNHANDLEABLE )
> + if ( hvm_send_ioreq(s, p, buffered) == IOREQ_STATUS_UNHANDLED )
> failed++;
> }
>
> return failed;
> }
>
> -static int hvm_access_cf8(
> - int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> -{
> - struct domain *d = current->domain;
> -
> - if ( dir == IOREQ_WRITE && bytes == 4 )
> - d->arch.hvm.pci_cf8 = *val;
> -
> - /* We always need to fall through to the catch all emulator */
> - return X86EMUL_UNHANDLEABLE;
> -}
> -
> void hvm_ioreq_init(struct domain *d)
> {
> spin_lock_init(&d->arch.hvm.ioreq_server.lock);
>
> - register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> + arch_hvm_ioreq_init(d);
> }
>
> /*
> diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
> index e2588e9..376e2ef 100644
> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -19,6 +19,165 @@
> #ifndef __ASM_X86_HVM_IOREQ_H__
> #define __ASM_X86_HVM_IOREQ_H__
>
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/vmx/vmx.h>
> +
> +#include <public/hvm/params.h>
> +
> +struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> + unsigned int id);
> +
> +static inline bool arch_hvm_io_completion(enum hvm_io_completion io_completion)
> +{
> + switch ( io_completion )
> + {
> + case HVMIO_realmode_completion:
> + {
> + struct hvm_emulate_ctxt ctxt;
> +
> + hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> + vmx_realmode_emulate_one(&ctxt);
> + hvm_emulate_writeback(&ctxt);
> +
> + break;
> + }
> +
> + default:
> + ASSERT_UNREACHABLE();
> + break;
> + }
> +
> + return true;
> +}
> +
> +/* Called when target domain is paused */
> +static inline void arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
> +{
> + p2m_set_ioreq_server(s->target, 0, s);
> +}
> +
> +/*
> + * Map or unmap an ioreq server to specific memory type. For now, only
> + * HVMMEM_ioreq_server is supported, and in the future new types can be
> + * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
> + * currently, only write operations are to be forwarded to an ioreq server.
> + * Support for the emulation of read operations can be added when an ioreq
> + * server has such requirement in the future.
> + */
> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
> + ioservid_t id,
> + uint32_t type,
> + uint32_t flags)
> +{
> + struct hvm_ioreq_server *s;
> + int rc;
> +
> + if ( type != HVMMEM_ioreq_server )
> + return -EINVAL;
> +
> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> + return -EINVAL;
> +
> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> + s = get_ioreq_server(d, id);
> +
> + rc = -ENOENT;
> + if ( !s )
> + goto out;
> +
> + rc = -EPERM;
> + if ( s->emulator != current->domain )
> + goto out;
> +
> + rc = p2m_set_ioreq_server(d, flags, s);
> +
> + out:
> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> + if ( rc == 0 && flags == 0 )
> + {
> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> + if ( read_atomic(&p2m->ioreq.entry_count) )
> + p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> + }
> +
> + return rc;
> +}
> +

The above doesn't really feel right to me. It's really an entry point into the ioreq server code and as such I think it ought to be left in the common code. I suggest replacing the p2m_set_ioreq_server() function with an arch specific function (also taking the type) which you can then implement here.

The rest of the patch looks ok.

Paul

> +static inline int hvm_ioreq_server_get_type_addr(const struct domain *d,
> + const ioreq_t *p,
> + uint8_t *type,
> + uint64_t *addr)
> +{
> + uint32_t cf8 = d->arch.hvm.pci_cf8;
> +
> + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> + return -EINVAL;
> +
> + if ( p->type == IOREQ_TYPE_PIO &&
> + (p->addr & ~3) == 0xcfc &&
> + CF8_ENABLED(cf8) )
> + {
> + uint32_t x86_fam;
> + pci_sbdf_t sbdf;
> + unsigned int reg;
> +
> + reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
> +
> + /* PCI config data cycle */
> + *type = XEN_DMOP_IO_RANGE_PCI;
> + *addr = ((uint64_t)sbdf.sbdf << 32) | reg;
> + /* AMD extended configuration space access? */
> + if ( CF8_ADDR_HI(cf8) &&
> + d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
> + (x86_fam = get_cpu_family(
> + d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 &&
> + x86_fam < 0x17 )
> + {
> + uint64_t msr_val;
> +
> + if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> + (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> + *addr |= CF8_ADDR_HI(cf8);
> + }
> + }
> + else
> + {
> + *type = (p->type == IOREQ_TYPE_PIO) ?
> + XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
> + *addr = p->addr;
> + }
> +
> + return 0;
> +}
> +
> +static inline int hvm_access_cf8(
> + int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> +{
> + struct domain *d = current->domain;
> +
> + if ( dir == IOREQ_WRITE && bytes == 4 )
> + d->arch.hvm.pci_cf8 = *val;
> +
> + /* We always need to fall through to the catch all emulator */
> + return X86EMUL_UNHANDLEABLE;
> +}
> +
> +static inline void arch_hvm_ioreq_init(struct domain *d)
> +{
> + register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +}
> +
> +static inline bool arch_hvm_ioreq_destroy(struct domain *d)
> +{
> + if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> + return false;
> +
> + return true;
> +}
> +
> bool hvm_io_pending(struct vcpu *v);
> bool handle_hvm_io_completion(struct vcpu *v);
> bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
> @@ -38,8 +197,6 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
> int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
> uint32_t type, uint64_t start,
> uint64_t end);
> -int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
> - uint32_t type, uint32_t flags);
> int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
> bool enabled);
>
> @@ -55,6 +212,10 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
>
> void hvm_ioreq_init(struct domain *d);
>
> +#define IOREQ_STATUS_HANDLED X86EMUL_OKAY
> +#define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
> +#define IOREQ_STATUS_RETRY X86EMUL_RETRY
> +
> #endif /* __ASM_X86_HVM_IOREQ_H__ */
>
> /*
> --
> 2.7.4
Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
On 20.10.20 10:13, Paul Durrant wrote:

Hi Paul.

Sorry for the late response.

>> +
>> +/* Called when target domain is paused */
>> +static inline void arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
>> +{
>> + p2m_set_ioreq_server(s->target, 0, s);
>> +}
>> +
>> +/*
>> + * Map or unmap an ioreq server to specific memory type. For now, only
>> + * HVMMEM_ioreq_server is supported, and in the future new types can be
>> + * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
>> + * currently, only write operations are to be forwarded to an ioreq server.
>> + * Support for the emulation of read operations can be added when an ioreq
>> + * server has such requirement in the future.
>> + */
>> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
>> + ioservid_t id,
>> + uint32_t type,
>> + uint32_t flags)
>> +{
>> + struct hvm_ioreq_server *s;
>> + int rc;
>> +
>> + if ( type != HVMMEM_ioreq_server )
>> + return -EINVAL;
>> +
>> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
>> + return -EINVAL;
>> +
>> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>> +
>> + s = get_ioreq_server(d, id);
>> +
>> + rc = -ENOENT;
>> + if ( !s )
>> + goto out;
>> +
>> + rc = -EPERM;
>> + if ( s->emulator != current->domain )
>> + goto out;
>> +
>> + rc = p2m_set_ioreq_server(d, flags, s);
>> +
>> + out:
>> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
>> +
>> + if ( rc == 0 && flags == 0 )
>> + {
>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> + if ( read_atomic(&p2m->ioreq.entry_count) )
>> + p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
>> + }
>> +
>> + return rc;
>> +}
>> +
> The above doesn't really feel right to me. It's really an entry point into the ioreq server code and as such I think it ought to be left in the common code. I suggest replacing the p2m_set_ioreq_server() function with an arch specific function (also taking the type) which you can then implement here.

Agree that it ought to be left in the common code.

However, I am afraid I didn't entirely get you suggestion how this
function could be split. On Arm struct p2m_domain doesn't contain IOREQ
fields (p2m->ioreq.XXX), nor p2m_change_entry_type_global() is used, so
they should be abstracted together with p2m_set_ioreq_server().

So the whole "if ( rc == 0 && flags == 0 )" check should be folded into
arch_p2m_set_ioreq_server implementation on x86? This in turn raises a
question can we put a spin_unlock after.

I am wondering would it be acceptable to replace
hvm_map_mem_type_to_ioreq_server by
arch_hvm_map_mem_type_to_ioreq_server here and have the following in the
common code:

int hvm_map_mem_type_to_ioreq_server(struct domain *d,
                                     ioservid_t id,
                                     uint32_t type,
                                     uint32_t flags)
{
    return arch_hvm_map_mem_type_to_ioreq_server(d, id, type, flags);
}


>
> The rest of the patch looks ok.

Thank you.

--
Regards,

Oleksandr Tyshchenko
RE: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Sent: 04 November 2020 09:06
> To: paul@xen.org; xen-devel@lists.xenproject.org
> Cc: 'Oleksandr Tyshchenko' <oleksandr_tyshchenko@epam.com>; 'Jan Beulich' <jbeulich@suse.com>; 'Andrew
> Cooper' <andrew.cooper3@citrix.com>; 'Roger Pau Monné' <roger.pau@citrix.com>; 'Julien Grall'
> <julien@xen.org>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu' <wl@xen.org>; 'Julien
> Grall' <julien.grall@arm.com>
> Subject: Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common
>
>
> On 20.10.20 10:13, Paul Durrant wrote:
>
> Hi Paul.
>
> Sorry for the late response.
>
> >> +
> >> +/* Called when target domain is paused */
> >> +static inline void arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
> >> +{
> >> + p2m_set_ioreq_server(s->target, 0, s);
> >> +}
> >> +
> >> +/*
> >> + * Map or unmap an ioreq server to specific memory type. For now, only
> >> + * HVMMEM_ioreq_server is supported, and in the future new types can be
> >> + * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
> >> + * currently, only write operations are to be forwarded to an ioreq server.
> >> + * Support for the emulation of read operations can be added when an ioreq
> >> + * server has such requirement in the future.
> >> + */
> >> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
> >> + ioservid_t id,
> >> + uint32_t type,
> >> + uint32_t flags)
> >> +{
> >> + struct hvm_ioreq_server *s;
> >> + int rc;
> >> +
> >> + if ( type != HVMMEM_ioreq_server )
> >> + return -EINVAL;
> >> +
> >> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> >> + return -EINVAL;
> >> +
> >> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> >> +
> >> + s = get_ioreq_server(d, id);
> >> +
> >> + rc = -ENOENT;
> >> + if ( !s )
> >> + goto out;
> >> +
> >> + rc = -EPERM;
> >> + if ( s->emulator != current->domain )
> >> + goto out;
> >> +
> >> + rc = p2m_set_ioreq_server(d, flags, s);
> >> +
> >> + out:
> >> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> >> +
> >> + if ( rc == 0 && flags == 0 )
> >> + {
> >> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >> +
> >> + if ( read_atomic(&p2m->ioreq.entry_count) )
> >> + p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> >> + }
> >> +
> >> + return rc;
> >> +}
> >> +
> > The above doesn't really feel right to me. It's really an entry point into the ioreq server code and
> as such I think it ought to be left in the common code. I suggest replacing the p2m_set_ioreq_server()
> function with an arch specific function (also taking the type) which you can then implement here.
>
> Agree that it ought to be left in the common code.
>
> However, I am afraid I didn't entirely get you suggestion how this
> function could be split. On Arm struct p2m_domain doesn't contain IOREQ
> fields (p2m->ioreq.XXX), nor p2m_change_entry_type_global() is used, so
> they should be abstracted together with p2m_set_ioreq_server().
>
> So the whole "if ( rc == 0 && flags == 0 )" check should be folded into
> arch_p2m_set_ioreq_server implementation on x86? This in turn raises a
> question can we put a spin_unlock after.
>

Hi Oleksandr,

I think the code as it stands is really a bit of a layering violation. I don't really see a problem with retaining the ioreq server lock around the call to p2m_change_entry_type_global() so I'd just fold that into p2m_set_ioreq_server().

Paul
Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> As a lot of x86 code can be re-used on Arm later on, this
> patch makes some preparation to x86/hvm/ioreq.c before moving
> to the common code. This way we will get a verbatim copy for
> a code movement in subsequent patch (arch/x86/hvm/ioreq.c
> will be *just* renamed to common/ioreq).
>
> This patch does the following:
> 1. Introduce *inline* arch_hvm_ioreq_init(), arch_hvm_ioreq_destroy(),
> arch_hvm_io_completion(), arch_hvm_destroy_ioreq_server() and
> hvm_ioreq_server_get_type_addr() to abstract arch specific materials.
> 2 Make hvm_map_mem_type_to_ioreq_server() *inline*. It is not going
> to be called from the common code.

As already indicated on another sub-thread, I think some of these
are too large to be inline functions. Additionally, considering
their single-use purpose, I don't think they should be placed in
a header consumed by more than the producer and the sole consumer.

> 3. Make get_ioreq_server() global. It is going to be called from
> a few places.

And with this its name ought to change, to fit the general naming
model of global functions of this subsystem.

> 4. Add IOREQ_STATUS_* #define-s and update candidates for moving.

This, it seems to me, could be a separate patch.

> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>
> domain_pause(d);
>
> - p2m_set_ioreq_server(d, 0, s);
> + arch_hvm_destroy_ioreq_server(s);

Iirc there are plans to rename hvm_destroy_ioreq_server() in the
course of the generalization. If so, this arch hook would imo
better be named following the new scheme right away.

> @@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
> struct hvm_ioreq_server *s;
> unsigned int id;
>
> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> + if ( !arch_hvm_ioreq_destroy(d) )

There's no ioreq being destroyed here, so I think this wants
renaming (and again ideally right away following the planned
new scheme).

> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
> + ioservid_t id,
> + uint32_t type,
> + uint32_t flags)
> +{
> + struct hvm_ioreq_server *s;
> + int rc;
> +
> + if ( type != HVMMEM_ioreq_server )
> + return -EINVAL;
> +
> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> + return -EINVAL;
> +
> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> + s = get_ioreq_server(d, id);
> +
> + rc = -ENOENT;
> + if ( !s )
> + goto out;
> +
> + rc = -EPERM;
> + if ( s->emulator != current->domain )
> + goto out;
> +
> + rc = p2m_set_ioreq_server(d, flags, s);
> +
> + out:
> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> + if ( rc == 0 && flags == 0 )
> + {
> + struct p2m_domain *p2m = p2m_get_hostp2m(d);

I realize I may be asking too much, but would it be possible if,
while moving code, you made simple and likely uncontroversial
adjustments like adding const here? (Such adjustments would be
less desirable to make if they increased the size of the patch,
e.g. if you were touching only nearby code.)

> + if ( read_atomic(&p2m->ioreq.entry_count) )
> + p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> + }
> +
> + return rc;
> +}
> +
> +static inline int hvm_ioreq_server_get_type_addr(const struct domain *d,
> + const ioreq_t *p,
> + uint8_t *type,
> + uint64_t *addr)
> +{
> + uint32_t cf8 = d->arch.hvm.pci_cf8;

Similarly, for example, neither this nor ...

> + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> + return -EINVAL;
> +
> + if ( p->type == IOREQ_TYPE_PIO &&
> + (p->addr & ~3) == 0xcfc &&
> + CF8_ENABLED(cf8) )
> + {
> + uint32_t x86_fam;

... this really need to use a fixed width type - unsigned int is
going to be quite fine. But since you're only moving this code,
I guess I'm not going to insist.

> +static inline bool arch_hvm_ioreq_destroy(struct domain *d)
> +{
> + if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> + return false;
> +
> + return true;

Any reason this cannot simply be

return relocate_portio_handler(d, 0xcf8, 0xcf8, 4);

?

Jan
Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
On 12.11.20 12:58, Jan Beulich wrote:

Hi Jan.

> On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> As a lot of x86 code can be re-used on Arm later on, this
>> patch makes some preparation to x86/hvm/ioreq.c before moving
>> to the common code. This way we will get a verbatim copy for
>> a code movement in subsequent patch (arch/x86/hvm/ioreq.c
>> will be *just* renamed to common/ioreq).
>>
>> This patch does the following:
>> 1. Introduce *inline* arch_hvm_ioreq_init(), arch_hvm_ioreq_destroy(),
>> arch_hvm_io_completion(), arch_hvm_destroy_ioreq_server() and
>> hvm_ioreq_server_get_type_addr() to abstract arch specific materials.
>> 2 Make hvm_map_mem_type_to_ioreq_server() *inline*. It is not going
>> to be called from the common code.
> As already indicated on another sub-thread, I think some of these
> are too large to be inline functions. Additionally, considering
> their single-use purpose, I don't think they should be placed in
> a header consumed by more than the producer and the sole consumer.
ok, the only reason I made these inline was to achieve a moving of the
whole x86/hvm/ioreq.c to the common code.
I will move some of them back to ioreq.c.


>
>> 3. Make get_ioreq_server() global. It is going to be called from
>> a few places.
> And with this its name ought to change, to fit the general naming
> model of global functions of this subsystem.
I think, with new requirements (making
hvm_map_mem_type_to_ioreq_server() common) this helper
doesn't need to be global. I will make it static again.


>
>> 4. Add IOREQ_STATUS_* #define-s and update candidates for moving.
> This, it seems to me, could be a separate patch.

Well, will do.


>
>> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>>
>> domain_pause(d);
>>
>> - p2m_set_ioreq_server(d, 0, s);
>> + arch_hvm_destroy_ioreq_server(s);
> Iirc there are plans to rename hvm_destroy_ioreq_server() in the
> course of the generalization. If so, this arch hook would imo
> better be named following the new scheme right away.
Could you please clarify, are you speaking about the plans discussed there

"[PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved
function names"?

Copy text for the convenience:
AT least some of the functions touched here would be nice to be
moved to a more consistent new naming scheme right away, to
avoid having to touch all the same places again. I guess ioreq
server functions would be nice to all start with ioreq_server_
and ioreq functions to all start with ioreq_. E.g. ioreq_send()
and ioreq_server_select().

or some other plans I am not aware of?


What I really want to avoid with IOREQ enabling work is the round-trips
related to naming things, this patch series
became quite big (and consumes som time to rebase and test it) and I
expect it to become bigger.

So the arch_hvm_destroy_ioreq_server() should be
arch_ioreq_server_destroy()?


>
>> @@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>> struct hvm_ioreq_server *s;
>> unsigned int id;
>>
>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
>> + if ( !arch_hvm_ioreq_destroy(d) )
> There's no ioreq being destroyed here, so I think this wants
> renaming (and again ideally right away following the planned
> new scheme).
Agree that no ioreq being destroyed here. Probably
ioreq_server_check_for_destroy()?
I couldn't think of a better name.


>
>> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
>> + ioservid_t id,
>> + uint32_t type,
>> + uint32_t flags)
>> +{
>> + struct hvm_ioreq_server *s;
>> + int rc;
>> +
>> + if ( type != HVMMEM_ioreq_server )
>> + return -EINVAL;
>> +
>> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
>> + return -EINVAL;
>> +
>> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>> +
>> + s = get_ioreq_server(d, id);
>> +
>> + rc = -ENOENT;
>> + if ( !s )
>> + goto out;
>> +
>> + rc = -EPERM;
>> + if ( s->emulator != current->domain )
>> + goto out;
>> +
>> + rc = p2m_set_ioreq_server(d, flags, s);
>> +
>> + out:
>> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
>> +
>> + if ( rc == 0 && flags == 0 )
>> + {
>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
> I realize I may be asking too much, but would it be possible if,
> while moving code, you made simple and likely uncontroversial
> adjustments like adding const here? (Such adjustments would be
> less desirable to make if they increased the size of the patch,
> e.g. if you were touching only nearby code.)
This function as well as one located below won't be moved to this header
for the next version of patch.

ok, will add const.


>
>> + if ( read_atomic(&p2m->ioreq.entry_count) )
>> + p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +static inline int hvm_ioreq_server_get_type_addr(const struct domain *d,
>> + const ioreq_t *p,
>> + uint8_t *type,
>> + uint64_t *addr)
>> +{
>> + uint32_t cf8 = d->arch.hvm.pci_cf8;
> Similarly, for example, neither this nor ...
>
>> + if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
>> + return -EINVAL;
>> +
>> + if ( p->type == IOREQ_TYPE_PIO &&
>> + (p->addr & ~3) == 0xcfc &&
>> + CF8_ENABLED(cf8) )
>> + {
>> + uint32_t x86_fam;
> ... this really need to use a fixed width type - unsigned int is
> going to be quite fine. But since you're only moving this code,
> I guess I'm not going to insist.

Will use unsigned int.


>
>> +static inline bool arch_hvm_ioreq_destroy(struct domain *d)
>> +{
>> + if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
>> + return false;
>> +
>> + return true;
> Any reason this cannot simply be
>
> return relocate_portio_handler(d, 0xcf8, 0xcf8, 4);

Yes, good point.


--
Regards,

Oleksandr Tyshchenko
Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
On 13.11.2020 12:09, Oleksandr wrote:
> On 12.11.20 12:58, Jan Beulich wrote:
>> On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
>>> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>>>
>>> domain_pause(d);
>>>
>>> - p2m_set_ioreq_server(d, 0, s);
>>> + arch_hvm_destroy_ioreq_server(s);
>> Iirc there are plans to rename hvm_destroy_ioreq_server() in the
>> course of the generalization. If so, this arch hook would imo
>> better be named following the new scheme right away.
> Could you please clarify, are you speaking about the plans discussed there
>
> "[PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved
> function names"?
>
> Copy text for the convenience:
> AT least some of the functions touched here would be nice to be
> moved to a more consistent new naming scheme right away, to
> avoid having to touch all the same places again. I guess ioreq
> server functions would be nice to all start with ioreq_server_
> and ioreq functions to all start with ioreq_. E.g. ioreq_send()
> and ioreq_server_select().
>
> or some other plans I am not aware of?
>
>
> What I really want to avoid with IOREQ enabling work is the round-trips
> related to naming things, this patch series
> became quite big (and consumes som time to rebase and test it) and I
> expect it to become bigger.
>
> So the arch_hvm_destroy_ioreq_server() should be
> arch_ioreq_server_destroy()?

I think so, yes. If you want to avoid doing full patches, how
about you simply list the functions / variables you plan to
rename alongside the intended new names? Would likely be easier
for all involved parties.

>>> @@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>>> struct hvm_ioreq_server *s;
>>> unsigned int id;
>>>
>>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
>>> + if ( !arch_hvm_ioreq_destroy(d) )
>> There's no ioreq being destroyed here, so I think this wants
>> renaming (and again ideally right away following the planned
>> new scheme).
> Agree that no ioreq being destroyed here. Probably
> ioreq_server_check_for_destroy()?
> I couldn't think of a better name.

"check" implies no change (and d ought to then be const struct
domain *). With the containing function likely becoming
ioreq_server_destroy_all(), arch_ioreq_server_destroy_all()
would come to mind, or arch_ioreq_server_prepare_destroy_all().

>>> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
>>> + ioservid_t id,
>>> + uint32_t type,
>>> + uint32_t flags)
>>> +{
>>> + struct hvm_ioreq_server *s;
>>> + int rc;
>>> +
>>> + if ( type != HVMMEM_ioreq_server )
>>> + return -EINVAL;
>>> +
>>> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
>>> + return -EINVAL;
>>> +
>>> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>>> +
>>> + s = get_ioreq_server(d, id);
>>> +
>>> + rc = -ENOENT;
>>> + if ( !s )
>>> + goto out;
>>> +
>>> + rc = -EPERM;
>>> + if ( s->emulator != current->domain )
>>> + goto out;
>>> +
>>> + rc = p2m_set_ioreq_server(d, flags, s);
>>> +
>>> + out:
>>> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
>>> +
>>> + if ( rc == 0 && flags == 0 )
>>> + {
>>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> I realize I may be asking too much, but would it be possible if,
>> while moving code, you made simple and likely uncontroversial
>> adjustments like adding const here? (Such adjustments would be
>> less desirable to make if they increased the size of the patch,
>> e.g. if you were touching only nearby code.)
> This function as well as one located below won't be moved to this header
> for the next version of patch.
>
> ok, will add const.

Well, if you don't move the code, then better keep the diff small
and leave things as they are.

Jan
Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
On 13.11.20 13:20, Jan Beulich wrote:

Hi Jan.

> On 13.11.2020 12:09, Oleksandr wrote:
>> On 12.11.20 12:58, Jan Beulich wrote:
>>> On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
>>>> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>>>>
>>>> domain_pause(d);
>>>>
>>>> - p2m_set_ioreq_server(d, 0, s);
>>>> + arch_hvm_destroy_ioreq_server(s);
>>> Iirc there are plans to rename hvm_destroy_ioreq_server() in the
>>> course of the generalization. If so, this arch hook would imo
>>> better be named following the new scheme right away.
>> Could you please clarify, are you speaking about the plans discussed there
>>
>> "[PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved
>> function names"?
>>
>> Copy text for the convenience:
>> AT least some of the functions touched here would be nice to be
>> moved to a more consistent new naming scheme right away, to
>> avoid having to touch all the same places again. I guess ioreq
>> server functions would be nice to all start with ioreq_server_
>> and ioreq functions to all start with ioreq_. E.g. ioreq_send()
>> and ioreq_server_select().
>>
>> or some other plans I am not aware of?
>>
>>
>> What I really want to avoid with IOREQ enabling work is the round-trips
>> related to naming things, this patch series
>> became quite big (and consumes som time to rebase and test it) and I
>> expect it to become bigger.
>>
>> So the arch_hvm_destroy_ioreq_server() should be
>> arch_ioreq_server_destroy()?
> I think so, yes. If you want to avoid doing full patches, how
> about you simply list the functions / variables you plan to
> rename alongside the intended new names? Would likely be easier
> for all involved parties.
I think it is a good idea. I will prepare a list once I analyze all new
comments to this series.
As I understand that only global IOREQ functions need renaming according
to the new scheme,
local ones can be left as is but without "hvm" prefixes of course?



>
>>>> @@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>>>> struct hvm_ioreq_server *s;
>>>> unsigned int id;
>>>>
>>>> - if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
>>>> + if ( !arch_hvm_ioreq_destroy(d) )
>>> There's no ioreq being destroyed here, so I think this wants
>>> renaming (and again ideally right away following the planned
>>> new scheme).
>> Agree that no ioreq being destroyed here. Probably
>> ioreq_server_check_for_destroy()?
>> I couldn't think of a better name.
> "check" implies no change (and d ought to then be const struct
> domain *). With the containing function likely becoming
> ioreq_server_destroy_all(), arch_ioreq_server_destroy_all()
> would come to mind, or arch_ioreq_server_prepare_destroy_all().

ok, agree


>
>>>> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
>>>> + ioservid_t id,
>>>> + uint32_t type,
>>>> + uint32_t flags)
>>>> +{
>>>> + struct hvm_ioreq_server *s;
>>>> + int rc;
>>>> +
>>>> + if ( type != HVMMEM_ioreq_server )
>>>> + return -EINVAL;
>>>> +
>>>> + if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>>>> +
>>>> + s = get_ioreq_server(d, id);
>>>> +
>>>> + rc = -ENOENT;
>>>> + if ( !s )
>>>> + goto out;
>>>> +
>>>> + rc = -EPERM;
>>>> + if ( s->emulator != current->domain )
>>>> + goto out;
>>>> +
>>>> + rc = p2m_set_ioreq_server(d, flags, s);
>>>> +
>>>> + out:
>>>> + spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
>>>> +
>>>> + if ( rc == 0 && flags == 0 )
>>>> + {
>>>> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>> I realize I may be asking too much, but would it be possible if,
>>> while moving code, you made simple and likely uncontroversial
>>> adjustments like adding const here? (Such adjustments would be
>>> less desirable to make if they increased the size of the patch,
>>> e.g. if you were touching only nearby code.)
>> This function as well as one located below won't be moved to this header
>> for the next version of patch.
>>
>> ok, will add const.
> Well, if you don't move the code, then better keep the diff small
> and leave things as they are.

ok, in case I will have to move that code for any reason, I will take
suggestions into the account.

--
Regards,

Oleksandr Tyshchenko
Re: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common [ In reply to ]
On 13.11.2020 13:45, Oleksandr wrote:
> On 13.11.20 13:20, Jan Beulich wrote:
>> On 13.11.2020 12:09, Oleksandr wrote:
>>> On 12.11.20 12:58, Jan Beulich wrote:
>>>> On 15.10.2020 18:44, Oleksandr Tyshchenko wrote:
>>>>> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>>>>>
>>>>> domain_pause(d);
>>>>>
>>>>> - p2m_set_ioreq_server(d, 0, s);
>>>>> + arch_hvm_destroy_ioreq_server(s);
>>>> Iirc there are plans to rename hvm_destroy_ioreq_server() in the
>>>> course of the generalization. If so, this arch hook would imo
>>>> better be named following the new scheme right away.
>>> Could you please clarify, are you speaking about the plans discussed there
>>>
>>> "[PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved
>>> function names"?
>>>
>>> Copy text for the convenience:
>>> AT least some of the functions touched here would be nice to be
>>> moved to a more consistent new naming scheme right away, to
>>> avoid having to touch all the same places again. I guess ioreq
>>> server functions would be nice to all start with ioreq_server_
>>> and ioreq functions to all start with ioreq_. E.g. ioreq_send()
>>> and ioreq_server_select().
>>>
>>> or some other plans I am not aware of?
>>>
>>>
>>> What I really want to avoid with IOREQ enabling work is the round-trips
>>> related to naming things, this patch series
>>> became quite big (and consumes som time to rebase and test it) and I
>>> expect it to become bigger.
>>>
>>> So the arch_hvm_destroy_ioreq_server() should be
>>> arch_ioreq_server_destroy()?
>> I think so, yes. If you want to avoid doing full patches, how
>> about you simply list the functions / variables you plan to
>> rename alongside the intended new names? Would likely be easier
>> for all involved parties.
> I think it is a good idea. I will prepare a list once I analyze all new
> comments to this series.
> As I understand that only global IOREQ functions need renaming according
> to the new scheme,
> local ones can be left as is but without "hvm" prefixes of course?

Please apply common sense: Static ones, if you have to drop a
hvm_ prefix, may in some cases better further be renamed as well,
when their names aren't really in line with their purpose
(anymore). But yes, following a consistent naming model is more
relevant for non-static functions.

Jan