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

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.

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




 


Rackspace

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