Re: [kvm-ia64-devel] [03/15][PATCH] kvm/ia64: Add header files forkvm/ia64. V8

[Zhang, X. <xia...@in...>]

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.