[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 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




 


Rackspace

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