On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
> {
> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
How about using guest_cpuid_is_intel(vcpu) directly in the reprogram_gp_counter() ?
On 21-Feb-22 1:27 PM, Like Xu wrote:
> On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
>> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
>> {
>> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
>> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
>
> How about using guest_cpuid_is_intel(vcpu)
Yeah, that's better then strncmp().
> directly in the reprogram_gp_counter() ?
We need this flag in reprogram_fixed_counter() as well.
- Ravi
On Mon, Feb 21, 2022 at 2:02 AM Ravi Bangoria <[email protected]> wrote:
>
>
>
> On 21-Feb-22 1:27 PM, Like Xu wrote:
> > On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
> >> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
> >> {
> >> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
> >> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
> >
> > How about using guest_cpuid_is_intel(vcpu)
>
> Yeah, that's better then strncmp().
>
> > directly in the reprogram_gp_counter() ?
>
> We need this flag in reprogram_fixed_counter() as well.
Explicit "is_intel" checks in any form seem clumsy, since we have
already put some effort into abstracting away the implementation
differences in struct kvm_pmu. It seems like these differences could
be handled by adding three masks to that structure: the "raw event
mask" (i.e. event selector and unit mask), the hsw_in_tx mask, and the
hsw_in_tx_checkpointed mask.
These changes should also be coordinated with Like's series that
eliminates all of the PERF_TYPE_HARDWARE nonsense.
On 03-Mar-22 10:08 AM, Jim Mattson wrote:
> On Mon, Feb 21, 2022 at 2:02 AM Ravi Bangoria <[email protected]> wrote:
>>
>>
>>
>> On 21-Feb-22 1:27 PM, Like Xu wrote:
>>> On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
>>>> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
>>>> {
>>>> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
>>>> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
>>>
>>> How about using guest_cpuid_is_intel(vcpu)
>>
>> Yeah, that's better then strncmp().
>>
>>> directly in the reprogram_gp_counter() ?
>>
>> We need this flag in reprogram_fixed_counter() as well.
>
> Explicit "is_intel" checks in any form seem clumsy,
Indeed. However introducing arch specific callback for such tiny
logic seemed overkill to me. So thought to just do it this way.
> since we have
> already put some effort into abstracting away the implementation
> differences in struct kvm_pmu. It seems like these differences could
> be handled by adding three masks to that structure: the "raw event
> mask" (i.e. event selector and unit mask), the hsw_in_tx mask, and the
> hsw_in_tx_checkpointed mask.
struct kvm_pmu is arch independent. You mean struct kvm_pmu_ops?
>
> These changes should also be coordinated with Like's series that
> eliminates all of the PERF_TYPE_HARDWARE nonsense.
I'll rebase this on Like's patch series.
Thanks,
Ravi
On Thu, Mar 3, 2022 at 8:25 AM Ravi Bangoria <[email protected]> wrote:
>
>
>
> On 03-Mar-22 10:08 AM, Jim Mattson wrote:
> > On Mon, Feb 21, 2022 at 2:02 AM Ravi Bangoria <[email protected]> wrote:
> >>
> >>
> >>
> >> On 21-Feb-22 1:27 PM, Like Xu wrote:
> >>> On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
> >>>> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
> >>>> {
> >>>> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
> >>>> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
> >>>
> >>> How about using guest_cpuid_is_intel(vcpu)
> >>
> >> Yeah, that's better then strncmp().
> >>
> >>> directly in the reprogram_gp_counter() ?
> >>
> >> We need this flag in reprogram_fixed_counter() as well.
> >
> > Explicit "is_intel" checks in any form seem clumsy,
>
> Indeed. However introducing arch specific callback for such tiny
> logic seemed overkill to me. So thought to just do it this way.
I agree that arch-specific callbacks are ridiculous for these distinctions.
> > since we have
> > already put some effort into abstracting away the implementation
> > differences in struct kvm_pmu. It seems like these differences could
> > be handled by adding three masks to that structure: the "raw event
> > mask" (i.e. event selector and unit mask), the hsw_in_tx mask, and the
> > hsw_in_tx_checkpointed mask.
>
> struct kvm_pmu is arch independent. You mean struct kvm_pmu_ops?
No; I meant exactly what I said. See, for example, how the
reserved_bits field is used. It is initialized in the vendor-specific
pmu_refresh functions, and from then on, it facilitates
vendor-specific behaviors without explicit checks or vendor-specific
callbacks. An eventsel_mask field would be a natural addition to this
structure, to deal with the vendor-specific event selector widths. The
hsw_in_tx_mask and hsw_in_tx_checkpointed_mask fields are less
natural, since they will be 0 on AMD, but they would make it simple to
write the corresponding code in a vendor-neutral fashion.
BTW, am I the only one who finds the HSW_ prefixes a bit absurd here,
since TSX was never functional on Haswell?
> >
> > These changes should also be coordinated with Like's series that
> > eliminates all of the PERF_TYPE_HARDWARE nonsense.
>
> I'll rebase this on Like's patch series.
That's not exactly what I meant, but okay.
On 4/3/2022 2:05 am, Jim Mattson wrote:
> On Thu, Mar 3, 2022 at 8:25 AM Ravi Bangoria <[email protected]> wrote:
>>
>>
>>
>> On 03-Mar-22 10:08 AM, Jim Mattson wrote:
>>> On Mon, Feb 21, 2022 at 2:02 AM Ravi Bangoria <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 21-Feb-22 1:27 PM, Like Xu wrote:
>>>>> On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
>>>>>> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
>>>>>> {
>>>>>> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
>>>>>> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
>>>>>
>>>>> How about using guest_cpuid_is_intel(vcpu)
>>>>
>>>> Yeah, that's better then strncmp().
>>>>
>>>>> directly in the reprogram_gp_counter() ?
>>>>
>>>> We need this flag in reprogram_fixed_counter() as well.
>>>
>>> Explicit "is_intel" checks in any form seem clumsy,
>>
>> Indeed. However introducing arch specific callback for such tiny
>> logic seemed overkill to me. So thought to just do it this way.
>
> I agree that arch-specific callbacks are ridiculous for these distinctions.
>
>>> since we have
>>> already put some effort into abstracting away the implementation
>>> differences in struct kvm_pmu. It seems like these differences could
>>> be handled by adding three masks to that structure: the "raw event
>>> mask" (i.e. event selector and unit mask), the hsw_in_tx mask, and the
>>> hsw_in_tx_checkpointed mask.
>>
>> struct kvm_pmu is arch independent. You mean struct kvm_pmu_ops?
>
> No; I meant exactly what I said. See, for example, how the
> reserved_bits field is used. It is initialized in the vendor-specific
> pmu_refresh functions, and from then on, it facilitates
> vendor-specific behaviors without explicit checks or vendor-specific
> callbacks. An eventsel_mask field would be a natural addition to this
> structure, to deal with the vendor-specific event selector widths. The
> hsw_in_tx_mask and hsw_in_tx_checkpointed_mask fields are less
> natural, since they will be 0 on AMD, but they would make it simple to
> write the corresponding code in a vendor-neutral fashion.
>
> BTW, am I the only one who finds the HSW_ prefixes a bit absurd here,
> since TSX was never functional on Haswell?
The TSX story has more twists and turns, but we may start with 3a632cb229bf.
>
>>>
>>> These changes should also be coordinated with Like's series that
>>> eliminates all of the PERF_TYPE_HARDWARE nonsense.
>>
>> I'll rebase this on Like's patch series.
I could take over 3nd patch w/ Co-developed-by and move on if Ravi agrees.
>
> That's not exactly what I meant, but okay.
On 04-Mar-22 3:03 PM, Like Xu wrote:
> On 4/3/2022 2:05 am, Jim Mattson wrote:
>> On Thu, Mar 3, 2022 at 8:25 AM Ravi Bangoria <[email protected]> wrote:
>>>
>>>
>>>
>>> On 03-Mar-22 10:08 AM, Jim Mattson wrote:
>>>> On Mon, Feb 21, 2022 at 2:02 AM Ravi Bangoria <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 21-Feb-22 1:27 PM, Like Xu wrote:
>>>>>> On 21/2/2022 3:31 pm, Ravi Bangoria wrote:
>>>>>>> void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx)
>>>>>>> {
>>>>>>> struct kvm_pmc *pmc = kvm_x86_ops.pmu_ops->pmc_idx_to_pmc(pmu, pmc_idx);
>>>>>>> + bool is_intel = !strncmp(kvm_x86_ops.name, "kvm_intel", 9);
>>>>>>
>>>>>> How about using guest_cpuid_is_intel(vcpu)
>>>>>
>>>>> Yeah, that's better then strncmp().
>>>>>
>>>>>> directly in the reprogram_gp_counter() ?
>>>>>
>>>>> We need this flag in reprogram_fixed_counter() as well.
>>>>
>>>> Explicit "is_intel" checks in any form seem clumsy,
>>>
>>> Indeed. However introducing arch specific callback for such tiny
>>> logic seemed overkill to me. So thought to just do it this way.
>>
>> I agree that arch-specific callbacks are ridiculous for these distinctions.
>>
>>>> since we have
>>>> already put some effort into abstracting away the implementation
>>>> differences in struct kvm_pmu. It seems like these differences could
>>>> be handled by adding three masks to that structure: the "raw event
>>>> mask" (i.e. event selector and unit mask), the hsw_in_tx mask, and the
>>>> hsw_in_tx_checkpointed mask.
>>>
>>> struct kvm_pmu is arch independent. You mean struct kvm_pmu_ops?
>>
>> No; I meant exactly what I said. See, for example, how the
>> reserved_bits field is used. It is initialized in the vendor-specific
>> pmu_refresh functions, and from then on, it facilitates
>> vendor-specific behaviors without explicit checks or vendor-specific
>> callbacks. An eventsel_mask field would be a natural addition to this
>> structure, to deal with the vendor-specific event selector widths. The
>> hsw_in_tx_mask and hsw_in_tx_checkpointed_mask fields are less
>> natural, since they will be 0 on AMD, but they would make it simple to
>> write the corresponding code in a vendor-neutral fashion.
>>
>> BTW, am I the only one who finds the HSW_ prefixes a bit absurd here,
>> since TSX was never functional on Haswell?
>
> The TSX story has more twists and turns, but we may start with 3a632cb229bf.
>
>>
>>>>
>>>> These changes should also be coordinated with Like's series that
>>>> eliminates all of the PERF_TYPE_HARDWARE nonsense.
>>>
>>> I'll rebase this on Like's patch series.
>
> I could take over 3nd patch w/ Co-developed-by and move on if Ravi agrees.
Sure. I don't mind.
Thanks,
Ravi