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
