Re: [Xen-devel] [PATCH v13 for-xen-4.5 09/21] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.10.14 at 23:40, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -222,6 +215,7 @@ static int is_core2_vpmu_msr(u32 msr_index, int *type, 
> int *index)
>      }
>  }
>  
> +#define msraddr_to_bitpos(x) (((x)&0xffff) + ((x)>>31)*0x2000)

I understand this just gets moved here, but did it not occur to you
that this is bogus? It should really be

#define msraddr_to_bitpos(x) (((x)&0x1fff) + ((x)>>31)*0x2000)

and asserting that the zapped bits have the expected value would
certainly be beneficial too.

> @@ -367,12 +368,20 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
>          goto out_err;
>      vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>  
> -    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct core2_vpmu_context) +
> -                    (arch_pmc_cnt-1)*sizeof(struct arch_msr_pair));
> -    if ( !core2_vpmu_cxt )
> +    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct xen_pmu_intel_ctxt) +
> +                                   sizeof(uint64_t) * fixed_pmc_cnt +
> +                                   sizeof(struct xen_pmu_cntr_pair) *

I asked you before to use sizeof(<expression>) in favor of
sizeof(<type>) wherever possible. I.e. I can see why you do so
for the latter two, but for the first one I can't.

> @@ -418,7 +430,8 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
> msr_content,
>      int type = -1, index = -1;
>      struct vcpu *v = current;
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    struct core2_vpmu_context *core2_vpmu_cxt = NULL;
> +    struct xen_pmu_intel_ctxt *core2_vpmu_cxt = NULL;

If you have to touch this line anyway, please at once drop the
pointless initializer (also elsewhere).

> --- a/xen/include/public/arch-x86/xen-x86_32.h
> +++ b/xen/include/public/arch-x86/xen-x86_32.h
> @@ -136,6 +136,16 @@ struct cpu_user_regs {
>  typedef struct cpu_user_regs cpu_user_regs_t;
>  DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
>  
> +struct xen_pmu_regs {
> +    uint32_t eip;
> +    uint32_t esp;
> +    uint32_t eflags;
> +    uint16_t cs;
> +    uint16_t ss;
> +};

I still don't see why this and ...

> --- a/xen/include/public/arch-x86/xen-x86_64.h
> +++ b/xen/include/public/arch-x86/xen-x86_64.h
> @@ -174,6 +174,16 @@ struct cpu_user_regs {
>  typedef struct cpu_user_regs cpu_user_regs_t;
>  DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
>  
> +struct xen_pmu_regs {
> +    __DECL_REG(ip);
> +    __DECL_REG(sp);
> +    __DECL_REG(flags);
> +    uint16_t cs;
> +    uint16_t ss;
> +};

aren't folded and aren't placed in the PMU-specific header.

Furthermore I'm sure you realized (the latest when adding back
the eflags field for VM86 mode recognition) that to the consumer of
this data things are still ambiguous: You can't tell protected from
real mode, yet HVM guests definitely use that mode.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel