From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v2 15/30] xen/x86: Improvements to
 in-hypervisor cpuid sanity checks
Date: Wed, 17 Feb 2016 03:55:38 -0700
Message-ID: <56C45FBA02000078000D30D9@prv-mh.provo.novell.com>
References: <1454679743-18133-1-git-send-email-andrew.cooper3@citrix.com>
	<1454679743-18133-16-git-send-email-andrew.cooper3@citrix.com>
	<56C2003902000078000D23AC@prv-mh.provo.novell.com>
	<56C2070D.10803@citrix.com>
	<56C302C402000078000D2852@prv-mh.provo.novell.com>
	<56C44EBA.3090906@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <56C44EBA.3090906@citrix.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

>>> On 17.02.16 at 11:43, <andrew.cooper3@citrix.com> wrote:
> On 16/02/16 10:06, Jan Beulich wrote:
>>>>> On 15.02.16 at 18:12, <andrew.cooper3@citrix.com> wrote:
>>> On 15/02/16 15:43, Jan Beulich wrote:
>>>>>>> On 05.02.16 at 14:42, <andrew.cooper3@citrix.com> wrote:
>>>>> @@ -4617,50 +4618,39 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>>>>>          /* Fix up VLAPIC details. */
>>>>>          *ebx &= 0x00FFFFFFu;
>>>>>          *ebx |= (v->vcpu_id * 2) << 24;
>>>>> +
>>>>> +        *ecx &= hvm_featureset[FEATURESET_1c];
>>>>> +        *edx &= hvm_featureset[FEATURESET_1d];
>>>> Looks like I've overlooked an issue in patch 11, which becomes
>>>> apparent here: How can you use a domain-independent featureset
>>>> here, when features vary between HAP and shadow mode guests?
>>>> I.e. in the earlier patch I suppose you need to calculate two
>>>> hvm_*_featureset[]s, with the HAP one perhaps empty when
>>>> !hvm_funcs.hap_supported.
>>> Their use here is a halfway house between nothing and the planned full
>>> per-domain policies.
>>>
>>> In this case, the "don't expose $X to a non-hap domain" checks have been
>>> retained, to cover the difference.
>> Well, doesn't it seem to you that doing only half of the HAP/shadow
>> separation is odd/confusing? I.e. could I talk you into not doing any
>> such separation (enforcing the non-HAP overrides as is done now)
>> or finishing the separation to become visible/usable here?
> 
> The HAP/shadow distinction is needed in the toolstack to account for the
> hap=<bool> option.
> 
> The distinction will disappear when per-domain policies are introduced. 
> If you notice, the distinction is private to the data generated by the
> autogen script, and does not form a part of any API/ABI.  The sysctl
> only has a single hvm featureset.

I don't see this as being in line with

    hvm_featuremask = hvm_funcs.hap_supported ?
        hvm_hap_featuremask : hvm_shadow_featuremask;

in patch 11. A shadow mode guest should see exactly the same
set of features, regardless of whether HAP was available (and
enabled) on a host.

>>>>> +    case 0x80000007:
>>>>> +        d &= pv_featureset[FEATURESET_e7d];
>>>>> +        break;
>>>> By not clearing eax and ebx (not sure about ecx) here you would
>>>> again expose flags to guests without proper white listing.
>>>>
>>>>> +    case 0x80000008:
>>>>> +        b &= pv_featureset[FEATURESET_e8b];
>>>>>          break;
>>>> Same here for ecx and edx and perhaps the upper 8 bits of eax.
>>> Both of these would be changes to how these things are currently
>>> handled, whereby a guest gets to see whatever the toolstack managed to
>>> find in said leaf.  I was hoping to put off some of these decisions, but
>>> they probably need making now.  On the PV side they definitely can't be
>>> fully hidden, as these leaves are not maskable.
>> Right, but many are meaningful primarily to kernels, and there we
>> can hide them.
>>
>> Since you're switching from black to white listing here, I also think
>> we need a default label alongside the "unsupported" one here.
>> Similarly I would think XSTATE sub-leaves beyond 63 need hiding
>> now.
> 
> I would prefer not to do that now.  It is conflated with the future
> work, deliberately deferred to make this work a manageable size.

I understand that you need to set some boundaries for this
first step. But I also think that we shouldn't stop in the middle
of switching from black listing to white listing here.

Jan