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.
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
