On 02.12.20 10:00, Jan Beulich wrote:
Hi Jan
On 01.12.2020 19:53, Oleksandr wrote:
On 01.12.20 13:03, Alex Bennée wrote:
Oleksandr Tyshchenko <olekst...@gmail.com> writes:
@@ -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);
- }
-
It should be noted that p2m holds it's own lock but I'm unfamiliar with
Xen's locking architecture. Is there anything that prevents another vCPU
accessing a page that is also being used my ioreq on the first vCPU?
I am not sure that I would be able to provide reasonable explanations here.
All what I understand is that p2m_change_entry_type_global() x86
specific (we don't have p2m_ioreq_server concept on Arm) and should
remain as such (not exposed to the common code).
IIRC, I raised a question during V2 review whether we could have ioreq
server lock around the call to p2m_change_entry_type_global() and didn't
get objections.
Not getting objections doesn't mean much. Personally I don't recall
such a question, but this doesn't mean much.
Sorry for not being clear here. Discussion happened at [1] when I was
asked to move hvm_map_mem_type_to_ioreq_server() to the common code.
The important thing
here is that you properly justify this change in the description (I
didn't look at this version of the patch as a whole yet, so quite
likely you actually do). This is because you need to guarantee that
you don't introduce any lock order violations by this.
Yes, almost all changes in this patch are mostly mechanical and leave
things as they are.
The change with p2m_change_entry_type_global() requires an additional
attention, so decided to put emphasis on touching that
in the description and add a comment in the code that it is called with
ioreq_server lock held.
[1]
https://patchwork.kernel.org/project/xen-devel/patch/1602780274-29141-2-git-send-email-olekst...@gmail.com/#23734839
--
Regards,
Oleksandr Tyshchenko
