Re: [Qemu-devel] [PATCH v4 1/2] i386: Add Intel Processor Trace feature support

[Kang, Luwei]

> > +
> > +        if (!eax_0 ||
> > +           ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > +           ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > +           ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > +           ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > +                                           INTEL_PT_ADDR_RANGES_NUM) ||
> > +           ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > +                (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
> 
> I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as 
> I mentioned when reviewing v3.

Hi Eduardo,
    Thanks for the code review. I don't quite understand here why bit31 must 
same with host (meaning we must reject a host
where ecx_0 & (1 << 31) is set).
    Do you mean PT must be disabled in guest when host bit31 is set? 
    Bit 31: If 1, generated packets which contain IP payloads have LIP values, 
which include the CS base component.
    I can't find any special on this bit. Could you help clarify?

Thanks,
Luwei Kang

> 
> The rest of the patch looks good.
> 
> > +            /*
> > +             * Processor Trace capabilities aren't configurable, so if the
> > +             * host can't emulate the capabilities we report on
> > +             * cpu_x86_cpuid(), intel-pt can't be enabled on the current 
> > host.
> > +             */
> > +            env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
> > +            cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
> > +            rv = 1;
> > +        }
> > +    }
> > +
> >      return rv;
> >  }
> >
> [...]
> 
> --
> Eduardo