Re: [PATCH v2 3/3] xen: Expose the PMU to the guests

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Jan,

On 07.10.2021 10:03, Jan Beulich wrote:
> On 06.10.2021 12:58, Michal Orzel wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -692,6 +692,12 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>          return -EINVAL;
>>      }
>>  
>> +    if ( config->flags & XEN_DOMCTL_CDF_vpmu )
>> +    {
>> +        dprintk(XENLOG_INFO, "vpmu support not ready yet\n");
>> +        return -EINVAL;
>> +    }
> 
> I consider this message potentially misleading (as x86 does have vPMU
> support, it merely doesn't get enabled this way). But isn't this redundant
> with ...
> 
>> @@ -534,6 +535,12 @@ static int sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>          return -EINVAL;
>>      }
>>  
>> +    if ( vpmu && !vpmu_is_available )
>> +    {
>> +        dprintk(XENLOG_INFO, "vpmu requested but not available\n");
>> +        return -EINVAL;
>> +    }
> 
> ... this? (This message is again potentially misleading.)
> 
Ok. vpmu_is_available is false for x86 so the check in x86's 
arch_sanitise_domain_config is redundant.
I will fix it. When it comes to the message itself "vpmu requested but not 
available".
Does the following sound better for you?
"vpmu requested but the platform does not support it"
If not, can you please suggest a better message?
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -70,9 +70,12 @@ struct xen_domctl_createdomain {
>>  #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
>>  #define _XEN_DOMCTL_CDF_nested_virt   6
>>  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
>> +/* Should we expose the vPMU to the guest? */
>> +#define _XEN_DOMCTL_CDF_vpmu           7
>> +#define XEN_DOMCTL_CDF_vpmu            (1U << _XEN_DOMCTL_CDF_vpmu)
> 
> Like for the earlier patch, I think we should stop with the bad habit of
> defining both the bit position and the mask separately.
> 
Ok. I can change it. However there are lots of other places in Xen (e.g. 
asm-x86/hvm/irq.h)
when such methodology is used. But I agree with you and I will change it.

> Jan
> 
Cheers,
Michal