Re: [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Jan Beulich <JBeulich@suse.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	mpohlack@amazon.de, ross.lagerwall@citrix.com,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	xen-devel@lists.xenproject.org,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>,
	Keir Fraser <keir@xen.org>,
	sasha.levin@oracle.com
Subject: Re: [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Date: Tue, 22 Mar 2016 19:28:57 +0000	[thread overview]
Message-ID: <56F19CF9.7020007@citrix.com> (raw)
In-Reply-To: <20160322185744.GA28711@char.us.oracle.com>

On 22/03/16 18:57, Konrad Rzeszutek Wilk wrote:
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -751,3 +751,22 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG uint32_t op)
>>>>>          return xsm_default_action(XSM_PRIV, current->domain, NULL);
>>>>>      }
>>>>>  }
>>>>> +
>>>>> +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
>>>>> +{
>>>>> +    XSM_ASSERT_ACTION(XSM_OTHER);
>>>>> +    switch ( op )
>>>>> +    {
>>>>> +    case XEN_VERSION_version:
>>>>> +    case XEN_VERSION_extraversion:
>>>>> +    case XEN_VERSION_capabilities:
>>>>> +    case XEN_VERSION_platform_parameters:
>>>>> +    case XEN_VERSION_get_features:
>>>>> +    case XEN_VERSION_pagesize:
>>>>> +    case XEN_VERSION_guest_handle:
>>>>> +        /* These MUST always be accessible to any guest by default. */
>>>>> +        return xsm_default_action(XSM_HOOK, current->domain, NULL);
>>>>> +    default:
>>>>> +        return xsm_default_action(XSM_PRIV, current->domain, NULL);
>>>> Considering that we seem to have settled on some exceptions here
>>>> for the change adding XSM check to the legacy version op, do you
>>>> really think going with no exception at all here is the right approach?
>>>> Because if we do, that'll prevent guests getting fully converted over
>>>> to the new interface. Of course, we could also make this conversion
>>>> specifically a non-goal, and omit e.g. XEN_VERSION_VERSION from
>>>> this new interface.
>>> No no. I think convesion is the right long-term goal. 
>>>
>>> However the nice thing about this hypercall is that it can return -EPERM.
>>>
>>> Making it always return an value for XEN_VERSION_version,
>>> XEN_VERSION_platform_parameters, XEN_VERSION_get_features means that
>>> there are some exceptions to this "can return -EPERM" as they will
>>> be guaranteed an postive return value. They can ignore the -EPERM
>>> case.
>>>
>>> And means that guest can still take shortcuts.
>>>
>>> I agree with you that guests need these hypercalls but at the same
>>> time I am not sure what can be done so they don't fall flat on their
>>> faces if they are presented with -EPERM. The Linux xenbus_init can be
>>> modified to deal with this returning -EPERM where it makes some assumptions.
>>>
>>> But in a likelyhood it is the bad assumption!
>> I'm afraid I can't conclude what you mean to say with all of the
>> above.
> That I am waffling.
>
> Andrew, what is your opinion?

Nothing good can come from failing a XEN_VERSION_version hypercall. 
There are a number easy ways for a guest to infer such information.

XEN_VERSION_platform_parameters is only useful for 32bit PV guests, and
the toolstack.  Given that it is returning a fixed number in the ABI,
nothing good can come of failing this either.

get_features can effectively be failed for permission reasons by
returning 0.  As such, explicitly failing with -EPERM is similarly
pointless.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel