Re: [Xen-devel] [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.17 at 16:04, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/02/17 13:28, Jan Beulich wrote:
>>>>> On 08.02.17 at 14:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 08/02/17 12:30, Jan Beulich wrote:
>>>>>>> On 08.02.17 at 12:56, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> On 08/02/17 11:53, Jan Beulich wrote:
>>>>>>>>> On 08.02.17 at 11:44, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>>> On 08/02/17 10:42, Andrew Cooper wrote:
>>>>>>>> This results in rather more readable code.  No functional change.
>>>>>>>>
>>>>>>>> All fields currently specified are included, but commented out as no 
>>>>>>>> support
>>>>>>>> for their use is present.
>>>>>>> Apologies - sent a slightly stale version of the patch.  I have dropped
>>>>>>> this paragraph from the commit message, but the code is correct for v2.
>>>>>> With that and despite ...
>>>>>>
>>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>>>>>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>>>>>>>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>>>>>>>>
>>>>>>>> v2:
>>>>>>>>  * Use a transparent union rather than modifying the caller of
>>>>>>>>    ept_handle_violation()
>>>>>>>>  * Drop the extranious commented out bitfield names, but keep 
>>>>>>>> eff_user_exec 
> so
>>>>>>>>    gla_{valid,fault} are appropriately located.
>>>>>> ... this not really being what Kevin and I had asked for,
>>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> In which case I am confused?  What were you asking for if it isn't this?
>>>> The request was to keep commented out just the one field use of
>>>> which needs enabling work elsewhere, but present as normal
>>>> (named) fields the three higher ones we currently lack.
>>> All of the originally commented out bits need further work in Xen to enable.
>>>
>>> The higher bits are undefined if advanced EPT Violation VM-exit
>>> information is unavailable in hardware, meaning that they are not safe
>>> to use currently.
>> Bit 6 requires a control to be turned on, but the same doesn't hold
>> for bits 9..11 according to my reading of the SDM.
> 
> This matches my reading.
> 
>> So consuming the bits would, as an integral part, require an additional 
> feature check,
>> but that is entirely independent of naming these fields.
> 
> Without a new vmx_ept_vpid_cap-based predicate, it is unsafe to read
> those bits, because they are not guaranteed to be zero.  (It is curious
> that some fields are documented as reserved, and should be zero, while
> these are documented as undefined.)
> 
> I purposefully don't want to make them available, because forcing
> someone to modify ept_qual_t to use them will make it less likely that
> we miss the above requirement on the review of the first patch which
> starts to use them.

Well, as indicated by my R-b I'm fine with the patch going on as is.

>> And for bit 12 I don't see how it would be dangerous to give a name
>> in any event (it's even shared with at least one other exit qual).
> 
> This particular bit falls into the grey category of "despite having a
> clear name, its not obvious what to do with it".  I haven't yet found
> any indication in the manuals as to whether it is purely informative, or
> whether action needs to be taken.

I'm pretty sure it means we need to take action, but we simply
neglect to do so right now. Part of the reason likely being that
people involved here either don't know what action we're meant
to take or don't care enough to provide a patch.

Jan


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