[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V2 02/23] xen/ioreq: Make x86's IOREQ feature common
On 17.11.20 17:29, Paul Durrant wrote: Hi Paul Thank you for the prompt answer. The 'legacy' mechanism of mapping magic pages for ioreq servers should remain x86 specific I think that aspect of the code needs to remain behind and not get moved into common code. You could do that in arch specific calls in hvm_ioreq_server_enable/disable() and hvm_get_ioreq_server_info().Well, if legacy mechanism is not going to be used for other arch and should remain x86 specific, I will try to investigate what should be left in x86 code and rework the series. As a side note, I am afraid, we won't get a 100% code movement (which I managed to achieve here) for the next version of this patch as we need arch/x86/hvm/ioreq.c.I am investigating how to split the code in order to leave the 'legacy' mechanism x86 specific and have a few questions. Could you please clarify the following: 1. The split of hvm_ioreq_server_enable/disable() is obvious to me, I would like to clarify regarding hvm_get_ioreq_server_info(). Is it close to what you had in mind when suggesting the split of hvm_get_ioreq_server_info() or I just need to abstract hvm_ioreq_server_map_pages() call only?I think it is sufficient to just abstract hvm_ioreq_server_map_pages() (and return -EOPNOTSUPP in the Arm case). The buf ioreq port should be common. ok, will do. (Not completed and non tested) +/* Called with ioreq_server lock held */ +int arch_ioreq_server_get_info(struct hvm_ioreq_server *s, + unsigned long *ioreq_gfn, + unsigned long *bufioreq_gfn, + evtchn_port_t *bufioreq_port) +{ + if ( ioreq_gfn || bufioreq_gfn ) + { + int rc = hvm_ioreq_server_map_pages(s); + + if ( rc ) + return rc; + } + + if ( ioreq_gfn ) + *ioreq_gfn = gfn_x(s->ioreq.gfn); + + if ( HANDLE_BUFIOREQ(s) ) + { + if ( bufioreq_gfn ) + *bufioreq_gfn = gfn_x(s->bufioreq.gfn); + + if ( bufioreq_port ) + *bufioreq_port = s->bufioreq_evtchn; + } + + return 0; +} + int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id, unsigned long *ioreq_gfn, unsigned long *bufioreq_gfn, @@ -916,26 +954,7 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id, if ( s->emulator != current->domain ) goto out; - if ( ioreq_gfn || bufioreq_gfn ) - { - rc = hvm_ioreq_server_map_pages(s); - if ( rc ) - goto out; - } - - if ( ioreq_gfn ) - *ioreq_gfn = gfn_x(s->ioreq.gfn); - - if ( HANDLE_BUFIOREQ(s) ) - { - if ( bufioreq_gfn ) - *bufioreq_gfn = gfn_x(s->bufioreq.gfn); - - if ( bufioreq_port ) - *bufioreq_port = s->bufioreq_evtchn; - } - - rc = 0; + rc = arch_ioreq_server_get_info(s, ioreq_gfn, bufioreq_gfn, bufioreq_port); out: spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); 2. If I understand the code correctly, besides of the above-mentioned functions the arch specific calls should be in hvm_ioreq_server_init() and hvm_ioreq_server_deinit() to actually hide "hvm_ioreq_server_unmap_pages" usage from the common code. I noticed that the rollback code in hvm_ioreq_server_init() and the whole hvm_ioreq_server_deinit() have a lot in common except an extra ASSERT() and hvm_ioreq_server_free_pages() call in the latter. My question is could we just replace the rollback code by hvm_ioreq_server_deinit()? I assume an extra hvm_ioreq_server_free_pages() call wouldn't be an issue here, but I am not sure what to do with the ASSERT, I expect it to be triggered at such an early stage (so it probably needs moving out of the hvm_ioreq_server_deinit() (or dropped?) as well as comment needs updating). I am asking, because this way we would get *a single* arch hook here which would be arch_ioreq_server_deinit() and remove code duplication a bit.I would arch specific init and deinit, even if one of them does nothing... but then I like symmetry :-) Both hvm_ioreq_server_init() and hvm_ioreq_server_deinit() call "legacy" hvm_ioreq_server_unmap_pages() which we want to be abstracted. The only difference between these two usages is that the former calls it during rollback only (in case of error). Taking into the account what has been suggested for question #1 could we just introduce arch_ioreq_server_unmap_pages() to be called from both init and deinit? [Not completed not tested]@@ -762,7 +772,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, fail_add: hvm_ioreq_server_remove_all_vcpus(s); - hvm_ioreq_server_unmap_pages(s); + arch_ioreq_server_unmap_pages(s); hvm_ioreq_server_free_rangesets(s);@@ -776,7 +786,7 @@ static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s) hvm_ioreq_server_remove_all_vcpus(s); /* - * NOTE: It is safe to call both hvm_ioreq_server_unmap_pages() and + * NOTE: It is safe to call both arch_ioreq_server_unmap_pages() and * hvm_ioreq_server_free_pages() in that order. * This is because the former will do nothing if the pages * are not mapped, leaving the page to be freed by the latter.@@ -784,7 +794,7 @@ static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s) * the page_info pointer to NULL, meaning the latter will do * nothing. */ - hvm_ioreq_server_unmap_pages(s); + arch_ioreq_server_unmap_pages(s); hvm_ioreq_server_free_pages(s); hvm_ioreq_server_free_rangesets(s);@@ -918,7 +928,7 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id, if ( ioreq_gfn || bufioreq_gfn ) { - rc = hvm_ioreq_server_map_pages(s); + rc = arch_ioreq_server_map_pages(s); if ( rc ) goto out; }So looks like for leaving legacy mechanism x86 specific we need 4 new arch callbacks: - arch_ioreq_server_enable - arch_ioreq_server_disable - arch_ioreq_server_map_pages - arch_ioreq_server_unmap_pages Something close to this. (Not completed and non tested) @@ -761,18 +771,17 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, return 0; fail_add: - hvm_ioreq_server_remove_all_vcpus(s); - hvm_ioreq_server_unmap_pages(s); - - hvm_ioreq_server_free_rangesets(s); - - put_domain(s->emulator); + hvm_ioreq_server_deinit(s); return rc; } +void arch_ioreq_server_deinit(struct hvm_ioreq_server *s) +{ + hvm_ioreq_server_unmap_pages(s); +} + static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s) { - ASSERT(!s->enabled);I assume this is the ASSERT you're referring to... There's no way we should be deinit-ing an enabled server so that should remain in common code as is. ok, I agree. -- Regards, Oleksandr Tyshchenko
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |