Re: [PATCH v19 11/14] x86/VPMU: Handle PMU interrupts for PV guests

["Jan Beulich" <JBeulich@suse.com>]

>>> On 17.03.15 at 15:54, <boris.ostrovsky@oracle.com> wrote:
> Changes in v19:
> * Adjusted for new ops interfaces (passing vcpu vs. vpmu)
> * Test for domain->max_cpu in choose_hwdom_vcpu() instead of 
> 'domain->vcpu!=NULL'

I suppose that's something that then should also be done in patch 7?

> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -87,31 +87,57 @@ static void __init parse_vpmu_param(char *s)
>  void vpmu_lvtpc_update(uint32_t val)
>  {
>      struct vpmu_struct *vpmu;
> +    struct vcpu *curr;
>  
>      if ( vpmu_mode == XENPMU_MODE_OFF )
>          return;
>  
> -    vpmu = vcpu_vpmu(current);
> +    curr = current;

Please make this the initializer of the variable, to be consistent with
other code (including yours a few lines down).

> +    vpmu = vcpu_vpmu(curr);
>  
>      vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | (val & APIC_LVT_MASKED);
> -    apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
> +
> +    /* Postpone APIC updates for PV(H) guests if PMU interrupt is pending */
> +    if ( is_hvm_vcpu(curr) || !vpmu->xenpmu_data ||

Here and below you use is_hvm_vcpu() - is the title wrongly saying
just PV when PV/PVH is meant?

> @@ -548,6 +719,24 @@ long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>          pvpmu_finish(current->domain, &pmu_params);
>          break;
>  
> +    case XENPMU_lvtpc_set:
> +        curr = current;
> +        xenpmu_data = curr->arch.vpmu.xenpmu_data;
> +        if ( xenpmu_data == NULL )
> +            return -EINVAL;
> +        vpmu_lvtpc_update(xenpmu_data->pmu.l.lapic_lvtpc);
> +        break;

You don't use curr more than once here, hence I don't see the point
of latching current into a local variable.

These aside,
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan