At 14:07 +0800 on 03 Jun (1307110060), Dong, Eddie wrote:
> > > + if ( vmcs_reg == IO_BITMAP_A )
> > > + {
> > > + if (nvmx->iobitmap[0]) {
> > > + unmap_domain_page_global(nvmx->iobitmap[0]);
> > > + }
> > > + gpa = __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
> > IO_BITMAP_A);
> > > + mfn = mfn_x(gfn_to_mfn(p2m_get_hostp2m(v->domain),
> > > + gpa >> PAGE_SHIFT, &p2mt));
> > > + nvmx->iobitmap[0] = map_domain_page_global(mfn);
> >
> > Why are these maps _global? It might be OK to use 2 more global
> > mappings per VCPU but the reason should probably go in a comment beside
> > the call.
>
> Do you mean to use hvm_map_guest_frame_ro? Fine to me.
Yes, I think that would be better unless you know there's a point where
the bitmaps are accessed on a vcpu other than current. (On 64-bit it
makes no difference but on 32-bit map_domain_page_global() uses up a
global shared resource).
> >
> > Also, I don't see where these mappings get torn down on domain
> > destruction.
> >
> Yes. Fixed in nvmx_vcpu_destroy.
>
> > (While I'm looking at this code, this function is quite ugly. Why have
> > a single function if you're going to duplicate its contents anyway?)
>
> ??? We don't know fi guest changed the bitmap, so we have to check each time.
I think I wasn't clear. The logic is fine, I was just cavilling about
coding style. You have some code that's basically
f1() { BUNCH_O_CODE(1) }
f2() { BUNCH_O_CODE(2) }
and places that need to call f1(), f2() or both. Merging those into a
single function is a good idea, but the function should look like
f(x) {
int i = (x ? 1 : 2)
BUNCH_O_CODE(i)
}
and what you have is
f(x) {
if (x)
BUNCH_O_CODE(1)
else
BUNCH_O_CODE(2)
}
which keeps the duplication.
Cheers,
Tim.
--
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home