Hi Jan
@@ -1080,6 +1105,24 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain
*d, ioservid_t id,
return rc;
}
+/* Called with ioreq_server lock held */
+int arch_ioreq_server_map_mem_type(struct domain *d,
+ struct hvm_ioreq_server *s,
+ uint32_t flags)
+{
+ int rc = p2m_set_ioreq_server(d, flags, s);
+
+ if ( rc == 0 && flags == 0 )
+ {
+ const 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;
+}
+
/*
* 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
@@ -1112,19 +1155,11 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d,
ioservid_t id,
if ( s->emulator != current->domain )
goto out;
- rc = p2m_set_ioreq_server(d, flags, s);
+ rc = arch_ioreq_server_map_mem_type(d, s, flags);
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;
}
While you mention this change in the description, I'm still
missing justification as to why the change is safe to make. I
continue to think p2m_change_entry_type_global() would better
not be called inside the locked region, if at all possible.
Well. I am afraid, I don't have a 100% justification why the change is
safe to make as well
as I don't see an obvious reason why it is not safe to make (at least I
didn't find a possible deadlock scenario while investigating the code).
I raised a question earlier whether I can fold this check in, which
implied calling p2m_change_entry_type_global() with ioreq_server lock held.
I'm aware of the earlier discussion. But "didn't find" isn't good
enough in a case like this, and since it's likely hard to indeed
prove there's no deadlock possible, I think it's best to avoid
having to provide such a proof by avoiding the nesting.
Agree here.
If there is a concern with calling this inside the locked region
(unfortunately still unclear for me at the moment), I will try to find
another way how to split hvm_map_mem_type_to_ioreq_server() without
potentially unsafe change here *and* exposing
p2m_change_entry_type_global() to the common code. Right now, I don't
have any ideas how this could be split other than
introducing one more hook here to deal with p2m_change_entry_type_global
(probably arch_ioreq_server_map_mem_type_complete?), but I don't expect
it to be accepted.
I appreciate any ideas on that.
Is there a reason why the simplest solution (two independent
arch_*() calls) won't do? If so, what are the constraints?
There is no reason.
Can the first one e.g. somehow indicate what needs to happen
after the lock was dropped?
I think, yes.
But the two calls look independent
right now, so I don't see any complicating factors.
ok, will go "two independent arch hooks" route then.
Thank you for the idea.
--
Regards,
Oleksandr Tyshchenko
