Jes Sorensen
2008-Mar-31 11:41 UTC
[03/15][PATCH] kvm/ia64: Add header files for kvm/ia64. V8
Hi Xiantao, Some more nit-picking, though some of this is a bit more important to fixup. Cheers, Jes> +typedef struct thash_data {Urgh! argh! Please avoid typedefs unless you really need them, see Chapter 5 of Documentation/CodingStyle for details.> diff --git a/include/asm-ia64/kvm_host.h b/include/asm-ia64/kvm_host.h > new file mode 100644 > index 0000000..522bde0 > --- /dev/null > +++ b/include/asm-ia64/kvm_host.h > @@ -0,0 +1,530 @@ > +/* -*- Mode:C; c-basic-offset:4; tab-width:4; indent-tabs-mode:nil -*- > */The standard indentation for Linux is 8 characters using tabs. If possible it's preferred to comply with that to make the entire kernel tree easier for everybody to deal with. See CodingStyle for details.> +struct kvm_mmio_req { > + uint64_t addr; /* physical address */ > + uint64_t size; /* size in bytes */ > + uint64_t data; /* data (or paddr of data) */ > + uint8_t state:4; > + uint8_t dir:1; /* 1=read, 0=write */ > +}; > +typedef struct kvm_mmio_req mmio_req_t;More typedefs> +/*Pal data struct */ > +typedef struct pal_call{and again.> + /*In area*/ > + uint64_t gr28; > + uint64_t gr29; > + uint64_t gr30; > + uint64_t gr31; > + /*Out area*/ > + struct ia64_pal_retval ret; > +} pal_call_t; > + > +/* Sal data structure */ > +typedef struct sal_call{and again...> + /*In area*/ > + uint64_t in0; > + uint64_t in1; > + uint64_t in2; > + uint64_t in3; > + uint64_t in4; > + uint64_t in5; > + uint64_t in6; > + uint64_t in7; > + /*Our area*/ > + struct sal_ret_values ret; > +} sal_call_t;
Carsten Otte
2008-Mar-31 13:46 UTC
[03/15][PATCH] kvm/ia64: Add header files for kvm/ia64. V8
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.> +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.> +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.
Apparently Analagous Threads
- [03/15][PATCH] kvm/ia64: Add header files for kvm/ia64. V8
- Accuracy problem in dchisq for non-central chi-squared
- use of tapply?
- Remote database & local database, and adding new weight found vtable error
- Bad smbclient put performance and smbfs write performance