Re: [PATCH 1/2] xen-gntdev: support mapping in HVM domains

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 12/08/2010 11:40 AM, Ian Campbell wrote:
> Hi Daniel,
> 
> On Fri, 2010-12-03 at 15:37 +0000, Daniel De Graaf wrote:
>> This changes the /dev/xen/gntdev device to work in HVM domains, and
>> also makes the mmap() behavior more closely match the behavior of
>> files instead of requiring an ioctl() call for each mmap/munmap.
> 
> This patch seems to contain a lot more than the above very brief
> description would suggest. As well as those two changes it looks to
> include various refactoring and code-motion, some clean up and other
> changes.

Sorry about that, I should have made a better effort to explain the reason
for the refactoring in the patch comment.

In the old gntdev device, a hypercall was made to adjust the actual page
tables of the userspace process according to the mapping set up in the
previous ioctl(). This direct page table manipulation does not work in HVM,
because the page tables refer to guest-physical addresses. The solution is
to not use the GNTMAP_contains_pte flag when making the hypercall, and instead
use guest-physical address space to contain the mapping.

Due to this change, the mmap() call is no longer the best place to do the
mapping; the eventual userspace address does not matter and the cleanup is
handled differently. This is most of the refactoring, elimination of MMU
notifier dependency, and the patch for this is probably going to be large
no matter what I do in order to keep it working before and after.

> Please can you also go into more detail in the relevant patch on the new
> ioctl/mmap semantics rather than just mentioning that you've changed
> them. Do you retain compatibility with the old behaviour? Is there a
> corresponding toolstack change which makes use of this functionality?

The change is fully backwards compatible, so the current Xen toolchain will
work with the changed API. I have a test program that uses the new ioctl, which
I could include if it would be useful; the modifications to the vchan library
that use the new ioctl() are not yet finished. The other mentioned changes
in mmap() aren't used in any code I have; they are just limitations caused by
direct PTE manipulation that no longer apply.

> The change to support HVM and the change in mmap/ioctl behaviour should
> certainly be separate patches but there seems to also be a some data
> structure refactoring going on, a change from using the mmu notifiers to
> something else, some sort of lazy mapping functionality etc these should
> all get their own individual patches with a changelog describing what
> they do and why.

I will try to strip down the patch to only support the old ioctl() API,
and then introduce the new call in another patch; same for the semantic
change in limit tracking. That will make it easier to evaluate changes that
are visible from userspace. The data structure changes, mmu notifier
elimination, and a large part of the refactoring can't be split up because
of the change from PTE to guest-physical remapping.

> Thanks,
> Ian.
> 

-- 
Daniel De Graaf
National Security Agency