NVMM Review List
----------------

Branch:
https://gitweb.dragonflybsd.org/~aly/dragonfly.git/shortlog/refs/heads/nvmm

Commits:

(For those commits don't have any subitems, please review them in general.
I'd like to make sure they're OK for push.)

* x86_64/cpufunc.h: Add rxcr() and rename xsetbv() to load_xcr()
  83b0338b0f3dce27f77facfb9536ecf52e820b11

  Looks ok

* nvmm: Port to DragonFly #10: cpu_info etc.
  81a188fbdf122d474ee5aa7fbede6fbf1e46e55e

  Looks ok.  Not 100% sure about the various HOST_ control registers
  but I think they are correct.
  
  - vmx_vcpu_run(): VMCS_HOST_TR_SELECTOR, VMCS_HOST_TR_BASE, VMCS_HOST_GDTR_BASE
  - vmx_vcpu_init(): VMCS_HOST_IDTR_BASE
  
  Are these registers correctly set?
  
  I did this patch based on the information found in pc64/vmm.
  The 'curcpu()'->'mycpu' substitution in vmx_vmcs_enter() is wrong,
  but I fixed that in a later commit.

* nvmm: Port to DragonFly #12: FPU save & restore
  2b872fb7b7bc323dbef4bb70f80f094f6bc26fd3

  Looks ok.

* nvmm: Port to DragonFly #13: debug register save & restore
  148d10e52eceb5049fe31f4256481733e0a03b30

  Looks ok.  I'm not entirely sure about the load order in
  x86_dbregs_restore().

* nvmm: Port to DragonFly #15: anonymous object management
  8017f3ec5571be61721cbe5fb5d3e79c4a724495

  Looks ok, conditionally, but requires testing in a low-memory
  environment that forces some paging (may be once we get a
  real kernel to boot via the nvmm).

  
  - uao_create(): is OBJ_NOSPLIT required? why or why not?

    You should set NOSPLIT to prevent the destruction of the
    underlying pages at munmap() time under certain circumstances.
    kern/sysv_shm.c also sets this flag.  This is because the
    object is separately managed by a kernel data structure so
    unmapped areas need to be retained.

* nvmm: Port to DragonFly #18: kernel memory allocation
  63dc901e67f5301163022bd838d27f0d09da3cdb

  This generally looks ok.

* pmap: Implement pmap_ept_transform() for NVMM
  2b538d14923e3b64ee4a3b7b03d1e19d3b27748e

  Looks ok.  I'm not entirely sure about PG_V_IDX having two bits set
  but it should work.

  It is unclear whether we actually need a pm_pmlpv_iso page for the EPT
  since it is never actually installed as the host pmap.  We probably don't,
  but we can keep that code in-place for now.
  
  - why is PG_NX_IDX bit set to 0? what does the 'inverted sense' mean?

    The pmap_bits_default[] array is an OR bitmap, so '0' is not
    bit 0.  0 means there is no support in the EPT for that hardware
    pte bit.

    NX is the 'no execute' bit.  It is not applicable to an EPT map.

    I'm not sure what the 'inverted sense' means.

* pmap: Implement pmap_npt_transform() for NVMM
  d968f99c0adb8af100e30bf601076c3bf3ccb2f9

  Looks ok. Same as above re: the pm_pmlpv_iso page.  Doesn't hurt for now.
  
  - still need to set PG_NX_IDX bit to 0, like EPT?

    I think you can leave it set to 0 (disabled), yes.

* nvmm: Port to DragonFly #24: pmap transform & TLB invalidation
  e7aff62f5045878e68dea50d1562c3e1eab003d4

  Looks ok.  Probably a bit inefficient blasting all real CPUs (a virtual
  machine might only have a few vcpu's). But it shouldn't hurt for testing.

  Ultimately we can think about changing pmap_inval_smp() to be an inline
  which directly calls pmap->pm_tlb_flush(), and then have a default
  assignment for pm_tlb_flush that points to a 'default' invalidator
  (which would be the original pmap_inval_smp code).

  Then you would be able to remove the NULL check stuff and make the
  functional API a bit cleaner.
  
  - pmap_inval_smp(): pm_tlb_flush callback ok?
  - {svm,vmx}_tlb_flush(): pmap_inval_smp() doesn't handle PTE_G/X86_PG_G, ok?

  # Mmm... that looks like a function loop.  pmap_inval_smp() will
  # call pm_tlb_flush via the function pointer, so you don't want it
  # to then turn-around and call pmap_inval_smp() again.

    Its ok for PG_G to not be handled, it does not look like [G]lobal
    entries are ever used inside the EPT (either Intel or AMD) pmap.

    TODO:
    The current implementation is inefficient, because it tries to
    invalidate the 'kernel_pmap' in {svm,vmx}_tlb_flush()...