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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/02/17 13:28, Jan Beulich wrote:
>>>> On 08.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>> On 08/02/17 12:30, Jan Beulich wrote:
>>>>>> On 08.02.17 at 12:56, <andrew.cooper3@citrix.com> wrote:
>>>> On 08/02/17 11:53, Jan Beulich wrote:
>>>>>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> 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@citrix.com>
>>>>>>> ---
>>>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>>>>
>>>>>>> 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@suse.com>
>>>> 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.

> 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.

~Andrew

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