Re: [Xen-devel] [RFC PATCH 8/8]: PVH: privcmd changes

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-09-13 at 02:19 +0100, Mukesh Rathor wrote:
> On Tue, 11 Sep 2012 15:10:23 +0100
> Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 
> > I think you can't rely on the implicit teardown here since you need to
> > unmap before you hand the page back to the balloon. The reason this
> > doesn't look necessary now is that you don't give the page back.
> > Also not ordering the stage 1 and stage 2 teardown correctly is
> > dangerous, depending on the eventual ordering you potentially turn an
> > erroneous access to a virtual address, which should result in a guest
> > OS level page fault (and e.g. a seg fault to the offending process)
> > into a hypervisor shoots the guest due to an unexpected stage 2 fault
> > type failure, which is somewhat undesirable ;-)
> > 
> > With that in mind I think you do in the end need to add
> > xen_unmap_domain_mfn_range which does the unmap from both stage 1 and
> > stage 2 -- that balances out the interface (making pvh_rem_xen_p2m
> > internal) too, which is nice. This function may turn out to be a nop
> > on !pvh, but that's ok (although maybe there would be no harm in doing
> > explicit unmaps, for consistency?).
>  
> Ok, I added xen_unmap_domain_mfn_range(). Take a look.

Thanks, I'll take a gander once I get ballooning working on ARM.

>  It appears that
> by the time privcmd_close() is called, the kernel has already freed 
> process resources and lookup_address() returns NULL. Now I am wondering
> if the kernel/mmu does anything to the page while shooting the pte
> entry. Well the page was orig from balloon, so the cleanup hopefully
> leaves it alone.

I suppose it depends on whether the core "takes over" the reference
which you hold. I think it doesn't, so this is just a leak, rather than
putting a ballooned page back into the general allocation pool (things
would be crashing left & right if it was doing this I reckon)

> 
> I had looked for other hooks initially when I did this, but 
> vm_operations_struct->close was the only one to pan out.
> 
> I can't really move pvh_privcmd_resv_pfns to mmu.c because the 
> xen_remap_domain_mfn_range is called one page at a time, and I need
> to allocate the array first. I'd have to change it to linked list, worth
> it? Or I'd have to move and export it.

Another alternative would be to add the page array as a parameter to the
map/unmap function, rather than relying on it propagating via
vma_private.

The other option I can see is for privcmd to use traverse_pages to hook
all the struct pages in at the right virtual address and then have
remap_mfn_range do a walk for each page. That's O(N) walks for each
mapping though, unless perhaps apply_to_page_range gives the callback
something which can be turned back into a struct pag or a pfn?

> > WRT passing data between interfaces in vma->vm_private, which is
> > pretty subtle, can we push that whole thing down into
> > xen_{remap,unmap}_domain_mfn_range too? This would make the
> > requirement on the caller be simple "never use vm_private", as
> > opposed to now where the requirement is "sometimes you might have to
> > allocate some stuff and stick it in here". The downside is that it
> > pushes code which could be generic down into per-arch stuff, although
> > with suitable generic helper functions this isn't so bad (whatever
> > happened to {alloc,free}_empty_pages_and_pagevec from the classic
> > kernels? Those did exactly what we want here, I think)
> 
> Well, it has to hang off of vma->vm_private. The alternative would be to
> have a hash table by process id or something, again not sure if worth it. 

I think using vm_private within a subsystem/layer is ok, what I think we
should avoid is having layers pass data back and forth in that field.

> 
> Take a look at my latest files attached. Now the alloc_balloon and free
> are split between privcmd and mmu.c. The alternative would be to call 
> xen_unmap_domain_mfn_range one pfn at a time and have it call 
> pvh_rem_xen_p2m(), and move free_xenballooned_pages to privcmd. But
> that would be same as just changing the name of pvh_rem_xen_p2m to
> xen_unmap_domain_mfn_range(). Also, remap and unmap won't be much
> symmetric then.
> 
> Not sure if there's a lot we could do here to be honest. LMK what you 
> think.
> 
> thanks,
> Mukesh
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel