Re: [Xen-devel] [PATCH v2 1/4] x86/vpmu: Move vpmu_do_cpuid() handling into {pv, hvm}_cpuid()

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 12/09/2016 01:15 PM, Andrew Cooper wrote:
> On 06/12/16 11:11, Andrew Cooper wrote:
>> On 05/12/16 20:59, Boris Ostrovsky wrote:
>>> On 12/05/2016 01:24 PM, Andrew Cooper wrote:
>>>> @@ -3516,6 +3516,17 @@ void hvm_cpuid(unsigned int input, unsigned int 
>>>> *eax, unsigned int *ebx,
>>>>              if ( !(hvm_pae_enabled(v) || hvm_long_mode_enabled(v)) )
>>>>                  *edx &= ~cpufeat_mask(X86_FEATURE_PSE36);
>>>>          }
>>>> +
>>>> +        if ( vpmu_enabled(v) &&
>>>> +             vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
>>>> +        {
>>>> +            *edx |= cpufeat_mask(X86_FEATURE_DS);
>>>> +            if ( cpu_has(&current_cpu_data, X86_FEATURE_DTES64) )
>>>> +                *ecx |= cpufeat_mask(X86_FEATURE_DTES64);
>>>> +            if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
>>>> +                *ecx |= cpufeat_mask(X86_FEATURE_DSCPL);
>>>> +        }
>>>> +
>>>>          break;
>>>>  
>>>>      case 0x7:
>>>> @@ -3646,6 +3657,18 @@ void hvm_cpuid(unsigned int input, unsigned int 
>>>> *eax, unsigned int *ebx,
>>>>          }
>>>>          break;
>>>>  
>>>> +    case 0x0000000a: /* Architectural Performance Monitor Features 
>>>> (Intel) */
>>>> +        if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || 
>>>> !vpmu_enabled(v) )
>>>> +        {
>>>> +            *eax = *ebx = *ecx = *edx = 0;
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        /* Report at most version 3 since that's all we currently emulate 
>>>> */
>>>> +        if ( (*eax & 0xff) > 3 )
>>>> +            *eax = (*eax & ~0xff) | 3;
>>>> +        break;
>>> Both this and Debug Store checks are the same for both HVM and PV. Can
>>> they be factored out?
>> The purpose of this patch series is to untangle the current call tree to
>> make it easier to finally merge the PV and HVM paths into a single
>> guest_cpuid().
>>
>> Yes, this does add a bit of duplication in the short timer, but allows
>> for easier movement to the longterm goal.
>>
>>> (and then perhaps version update can gain back PMU_VERSION_MASK macro)
>> That involves moving a whole load of Intel internals with generic names
>> into vpmu.h, which is why I chose not to do it.  The end result in
>> guest_cpuid() won't use it.
> Boris: Any further comment, or is my explanation ok?  Strictly speaking
> the patch isn't blocked on your review, but I'd prefer not to use that
> technicality if you are unhappy with it.


Since in the end both of these concerned will be addressed by
guest_cpuid() implementation I think it's all good.

Thanks.
-boris


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