Carsten Otte wrote: > Zhang, Xiantao wrote: >> +typedef union context { >> + /* 8K size */ >> + char dummy[KVM_CONTEXT_SIZE]; >> + struct { >> + unsigned long psr; >> + unsigned long pr; >> + unsigned long caller_unat; >> + unsigned long pad; >> + unsigned long gr[32]; >> + unsigned long ar[128]; >> + unsigned long br[8]; >> + unsigned long cr[128]; >> + unsigned long rr[8]; >> + unsigned long ibr[8]; >> + unsigned long dbr[8]; >> + unsigned long pkr[8]; >> + struct ia64_fpreg fr[128]; >> + }; >> +} context_t; > This looks ugly to me. I'd rather prefer to have a straight struct > with elements psr...fr[], and cast the pointer to char* when needed. > KVM_CONTEXT_SIZE can be used as parameter to kzalloc() on allocation, > it's too large to be on stack anyway. We need to allocate enough memory fix area, considering back-ward compabitility. In migration or save/restore case, we need to save this area. If migration happens in different kvm versions, and the size of different, it may cause issues. For example, we added a new field in new kvm, and restore a new snapshot to old versions, it may fail. >> +typedef struct thash_data { >> + union { >> + struct { >> + unsigned long p : 1; /* 0 */ >> + unsigned long rv1 : 1; /* 1 */ >> + unsigned long ma : 3; /* 2-4 */ >> + unsigned long a : 1; /* 5 */ >> + unsigned long d : 1; /* 6 */ >> + unsigned long pl : 2; /* 7-8 */ >> + unsigned long ar : 3; /* 9-11 */ >> + unsigned long ppn : 38; /* 12-49 */ >> + unsigned long rv2 : 2; /* 50-51 */ >> + unsigned long ed : 1; /* 52 */ >> + unsigned long ig1 : 11; /* 53-63 */ >> + }; >> + struct { >> + unsigned long __rv1 : 53; /* 0-52 */ >> + unsigned long contiguous : 1; /*53 */ >> + unsigned long tc : 1; /* 54 TR or TC */ + unsigned >> long cl : 1; + /* 55 I side or D side cache line */ >> + unsigned long len : 4; /* 56-59 */ >> + unsigned long io : 1; /* 60 entry is for io or >> not */ >> + unsigned long nomap : 1; >> + /* 61 entry cann't be inserted into machine >> TLB.*/ >> + unsigned long checked : 1; >> + /* 62 for VTLB/VHPT sanity check */ >> + unsigned long invalid : 1; >> + /* 63 invalid entry */ >> + }; >> + unsigned long page_flags; >> + }; /* same for VHPT and TLB */ >> + >> + union { >> + struct { >> + unsigned long rv3 : 2; >> + unsigned long ps : 6; >> + unsigned long key : 24; >> + unsigned long rv4 : 32; >> + }; >> + unsigned long itir; >> + }; >> + union { >> + struct { >> + unsigned long ig2 : 12; >> + unsigned long vpn : 49; >> + unsigned long vrn : 3; >> + }; >> + unsigned long ifa; >> + unsigned long vadr; >> + struct { >> + unsigned long tag : 63; >> + unsigned long ti : 1; >> + }; >> + unsigned long etag; >> + }; >> + union { >> + struct thash_data *next; >> + unsigned long rid; >> + unsigned long gpaddr; >> + }; >> +} thash_data_t; > A matter of taste, but I'd prefer unsigned long mask, and > #define MASK_BIT_FOR_PURPUSE over bitfields. This structure could be > much smaller that way. Yes, but it may be not so flexible to use. >> +struct kvm_regs { >> + char *saved_guest; >> + char *saved_stack; >> + struct saved_vpd vpd; >> + /*Arch-regs*/ >> + int mp_state; >> + unsigned long vmm_rr; >> + /* TR and TC. */ >> + struct thash_data itrs[NITRS]; >> + struct thash_data dtrs[NDTRS]; >> + /* Bit is set if there is a tr/tc for the region. */ + unsigned >> char itr_regions; + unsigned char dtr_regions; >> + unsigned char tc_regions; >> + >> + char irq_check; >> + unsigned long saved_itc; >> + unsigned long itc_check; >> + unsigned long timer_check; >> + unsigned long timer_pending; >> + unsigned long last_itc; >> + >> + unsigned long vrr[8]; >> + unsigned long ibr[8]; >> + unsigned long dbr[8]; >> + unsigned long insvc[4]; /* Interrupt in service. */ + unsigned >> long xtp; + >> + unsigned long metaphysical_rr0; /* from kvm_arch (so is pinned) */ >> + unsigned long metaphysical_rr4; /* from kvm_arch (so is pinned) */ >> + unsigned long metaphysical_saved_rr0; /* from kvm_arch */ >> + unsigned long metaphysical_saved_rr4; /* from kvm_arch */ >> + unsigned long fp_psr; /*used for lazy float register */ >> + unsigned long saved_gp; + /*for phycial emulation */ >> +}; > This looks like it does'nt just have guest register content in it. It > seems to me preferable to have another ioctl different from > KVM_GET_REGS/KVM_SET_REGS to read and set the rest of the content and > seperate it from struct kvm_regs. We want to add a ioctl for that later. -- To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |