Hi Paul
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?
(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.
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);
hvm_ioreq_server_remove_all_vcpus(s);
/*
@@ -784,7 +793,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_deinit(s);
hvm_ioreq_server_free_pages(s);
hvm_ioreq_server_free_rangesets(s);
put_domain(s->emulator);
--
Regards,
Oleksandr Tyshchenko
