Re: [PATCH v2 05/12] VMX/altp2m: add code to support EPTP switching and #VE.

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

On 24/06/15 18:31, Ed White wrote:
> On 06/24/2015 04:59 AM, Andrew Cooper wrote:
>>> +
>>> +    if ( !veinfo )
>>> +        return 0;
>>> +
>>> +    if ( veinfo->semaphore != 0 )
>>> +        goto out;
>> The semantics of this semaphore are not clearly spelled out in the
>> manual.  The only information I can locate concerning this field is in
>> note in 25.5.6.1 which says:
>>
>> "Delivery of virtualization exceptions writes the value FFFFFFFFH to
>> offset 4 in the virtualization-exception informa-
>> tion area (see Section 25.5.6.2). Thus, once a virtualization exception
>> occurs, another can occur only if software
>> clears this field."
>>
>> I presume this should be taken to mean "software writes 0 to this
>> field", but some clarification would be nice.
>>
> Immediately above that, where the conditions required to deliver #VE
> are discussed, it says "the 32 bits at offset 4 in the
> virtualization-exception information area are all 0". Hardware never
> writes anything other than FFFFFFFFH there, so only software can make
> that be so.

So it does.  Sorry for missing that.

>
>>> +
>>> +            if ( !p2m_find_altp2m_by_eptp(v->domain, eptp, &idx) )
>>> +            {
>>> +                gdprintk(XENLOG_ERR, "EPTP not found in alternate p2m list\n");
>>> +                domain_crash(v->domain);
>>> +            }
>>> +        }
>>> +
>> Is it worth checking that idx is plausible at this point, before blindly
>> writing it back into the vcpu structure?
> I'm not sure I follow your logic. In the case where the hardware supports
> EPTP_INDEX, the hardware itself is asserting that the index is valid. In the
> case quoted above, if the index isn't valid p2m_find_altp2m_by_eptp() will
> fail.

I tend to be somewhat more pessimistic when coding.

It is possible for __vmread(EPTP_INDEX, &idx); to return any index
between 0 and 511 (including some other software bug which has gone and
accidentally set of altp2m 11).

I would put a BUG_ON(idx >= MAX_ALTP2M) here just for safety.  Nothing
good can come of attempting to continue in such a state, but it is
certainly not an impossible situation to get into.

~Andrew