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
