Re: [Xen-devel] [PATCH 07 of 20] Emulation of guest vmptrld

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

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