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;
}
I agree with all the other comments and have made the approp changes for
v6. Thanks a lot.
Jane.
