[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V3 01/23] x86/ioreq: Prepare IOREQ feature for making it common
Oleksandr <olekstysh@xxxxxxxxx> writes: > On 01.12.20 13:03, Alex Bennée wrote: > > Hi Alex > >> Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> writes: >> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx> >>> >>> As a lot of x86 code can be re-used on Arm later on, this >>> patch makes some preparation to x86/hvm/ioreq.c before moving >>> to the common code. This way we will get a verbatim copy >> <snip> >>> It worth mentioning that a code which checks the return value of >>> p2m_set_ioreq_server() in hvm_map_mem_type_to_ioreq_server() was >>> folded into arch_ioreq_server_map_mem_type() for the clear split. >>> So the p2m_change_entry_type_global() is called with ioreq_server >>> lock held. >> <snip> >>> >>> +/* 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); >>> - } >>> - >> 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. I may mistake, but looks like the lock being used > in p2m_change_entry_type_global() is yet another lock for protecting > page table operations, so unlikely we could get into the trouble calling > this function with the ioreq server lock held. The p2m lock code looks designed to be recursive so I could only envision a problem where a page somehow racing to lock under the ioreq lock which I don't think is possible. However reasoning about locking is hard if your not familiar - it's one reason we added Promela/Spin [1] models to QEMU for our various locking regimes. [1] http://spinroot.com/spin/whatispin.html [2] https://git.qemu.org/?p=qemu.git;a=tree;f=docs/spin;h=cc168025131676429a560ca70d7234a56f958092;hb=HEAD > > >> >> Assuming that deadlock isn't a possibility to my relatively untrained >> eye this looks good to me: >> >> Reviewed-by: Alex Bennée <alex.bennee@xxxxxxxxxx> > > Thank you. -- Alex Bennée
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |