Re: [PATCH v5 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2022 15:31, Jane Malalane wrote:
> On 08/03/2022 12:33, Roger Pau Monné wrote:
>> On Tue, Mar 08, 2022 at 01:24:23PM +0100, Jan Beulich wrote:
>>> On 08.03.2022 12:38, Roger Pau Monné wrote:
>>>> On Mon, Mar 07, 2022 at 03:06:09PM +0000, Jane Malalane wrote:
>>>>> @@ -685,13 +687,31 @@ int arch_sanitise_domain_config(struct 
>>>>> xen_domctl_createdomain *config)
>>>>>           }
>>>>>       }
>>>>>   
>>>>> -    if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
>>>>> +    if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
>>>>> +                                     XEN_X86_ASSISTED_XAPIC |
>>>>> +                                     XEN_X86_ASSISTED_X2APIC) )
>>>>>       {
>>>>>           dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
>>>>>                   config->arch.misc_flags);
>>>>>           return -EINVAL;
>>>>>       }
>>>>>   
>>>>> +    if ( (assisted_xapic || assisted_x2apic) && !hvm )
>>>>> +    {
>>>>> +        dprintk(XENLOG_INFO,
>>>>> +                "Interrupt Controller Virtualization not supported for 
>>>>> PV\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    if ( (assisted_xapic && !assisted_xapic_available) ||
>>>>> +         (assisted_x2apic && !assisted_x2apic_available) )
>>>>> +    {
>>>>> +        dprintk(XENLOG_INFO,
>>>>> +                "Hardware assisted x%sAPIC requested but not 
>>>>> available\n",
>>>>> +                assisted_xapic && !assisted_xapic_available ? "" : "2");
>>>>> +        return -EINVAL;
>>>>
>>>> I think for those two you could return -ENODEV if others agree.
>>>
>>> If by "two" you mean the xAPIC and x2APIC aspects here (and not e.g. this
>>> and the earlier if()), then I agree. I'm always in favor of using distinct
>>> error codes when possible and at least halfway sensible.
>>
>> I would be fine by using it for the !hvm if also. IMO it makes sense
>> as PV doesn't have an APIC 'device' at all, so ENODEV would seem
>> fitting. EINVAL is also fine as the caller shouldn't even attempt that
>> in the first place.
>>
>> So let's use it for the last if only.
> Wouldn't it make more sense to use -ENODEV particularly for the first? I 
> agree that -ENODEV should be reported in the first case because it 
> doesn't make sense to request acceleration of something that doesn't 
> exist and I should have put that. But having a look at the hap code 
> (since it resembles the second case), it returns -EINVAL when it is not 
> available, unless you deem this to be different or, in retrospective, 
> that the hap code should too have been coded to return -ENODEV.
> 
> if ( hap && !hvm_hap_supported() )
>      {
>          dprintk(XENLOG_INFO, "HAP requested but not available\n");
>          return -EINVAL;
>      }

This is just one of the examples where using -ENODEV as you suggest
would introduce an inconsistency. We use -EINVAL also for other
purely guest-type dependent checks.

Jan