Review Board 2.0.15


cpu, kvm: fix MMIO handling

Review Request #2774 - Created May 11, 2015 and updated

Information
Nikos Nikoleris
gem5
default
Reviewers
Default 
Changeset 10833:9fd3d5837e1e

---------------------------

cpu, kvm: Don't flag TC as dirty after m5op


Currently, none of the m5ops changes the TC, and therefore it cannot be

dirty. Syncing the TC results in incorrect register state for the

simulated system, since the kernel will first emulate the instruction

and then update the guest's register state.

   

  


 


 

  

  









  
 


  

 













 



 

  


 

  

   

   
Review request changed

   
Updated  (May 11, 2015, 8:52 a.m.)

  

  







   



    

     
Description:

     
      
 ~   
Changeset 10833:9fd3d5837e1e
   ~ 
Changeset 10833:9fd3d5837e1e
   + ---------------------------
   + cpu, kvm: Don't flag TC as dirty after m5op
     

 ~   
cpu, kvm: fix MMIO handling
 ~   

 ~   
The thread context is not dirty and we don't need to force a sync with
 ~   KVM. Doing so, is harmful, since syncing is completed after the MMIO
   ~ 
Currently, none of the m5ops changes the TC, and therefore it cannot be
   ~ dirty. Syncing the TC results in incorrect register state for the
   ~ simulated system, since the kernel will first emulate the instruction
   ~ and then update the guest's register state.
 -   request is handled, and might clobber registers (eg. rax should have
 -   the size of the MMIO, when the function call returns).

     		
    



   




  

 


  

 



















 

  


 


  



   

   
Nikos Nikoleris

   
Posted  (May 11, 2015, 9:17 a.m.)


  


  


  

   
I just noticed that there is at least one m5op (m5Syscall) that changes the TC, and this patch will break it. So we need more work to fix this.



  

   

   
    
    
   


  





  

 


  

 

















 



 

  


 


  



   

   
Nilay Vaish

   
Posted  (June 18, 2015, 3:07 p.m.)


  


  


  

   
Can you give a more detailed example as to what's wrong with the current version?



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Forcing kvm to update the register state breaks the m5_readfile, this patch fixes it. The problem is that m5_readfile expects the numbers of bytes that were read from the MMIO to be written to rax which is not happening at the current version.
    

    When we mark the thread context as dirty, a sync with the kvm state is forced and an ioctl with KVM_SET_REGS is executed. I don't fully understand how the kernel handles the update of the registers in the middle of an MMIO request but it seems that the update to the registers overwrites the value of rax.
    

    When I wrote this patch I had a look at the pseudoInsts that are implemented through MMIO requests but I missed a couple that explictly change the register state like m5Syscall. So while the patch fixes m5_readfile and probably other pseudoInsts, it breaks m5Syscall and maybe others.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    I see, indeed m5Syscall in particular requires the context to be updated in order to be able to read RAX which specifies the syscall to perform. However, the context needs to updated as it is required for the finalization of MMIO operations; I am sure Andreas can have a more detailed comment on this. Are you running in SE mode? Maybe the solution to this is to handle differently m5 ops.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    I remember discussing this with Nikos a few months ago. IIRC, this is what currently happens:
    

    * ld %rax <- [magic address]
* Trap into gem5 and handle pseudo inst
* Return to KVM
* Complete load and update %rax
* Overwrite KVM register state with values from gem5

    

    The problem is that the intended semantics of memory mapped m5ops is that the result of a load is the return value of the function. The problem here is that the register state gets overwritten after the kernel emulates the load instruction. My conclusion at the time was that there isn't an easy fix for this problem if we want to allow m5ops to modify the register state.
    

    One solution to this problem would be to change the memory mapped m5ops interface. We could, for example, use stores instead of loads and let the stored value be the m5op ID. This would have the benefit of reducing the amount of memory we need to reserve for m5ops to one page.
    
    
    
   
    4.