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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 09/03/2022 16:51, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
> 
> On 09.03.2022 16:56, Jane Malalane wrote:
>> On 08/03/2022 14:41, Jan Beulich wrote:
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
>>> unless you have verified the sender and know the content is safe.
>>>
>>> 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
>> Hi Jan, so here I was comparing the hap implementation with the second
>> case, i.e.
>>
>> if ( (assisted_xapic && !assisted_xapic_available) ||
>>        (assisted_x2apic && !assisted_x2apic_available) )
>>
>> and you seem to agree that using -ENODEV would be inconsistent? Have I
>> misinterpreted this?
> 
> Not exactly. I'm comparing existing hap / hvm / !hap / !hvm uses with
> what you add.
> 
> Jan
> 
Okay, I wil swap the error codes then, thank you.

Jane.