Re: [Xen-devel] [PATCH v5 06/15] VMX/altp2m: add code to support EPTP switching and #VE.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.15 at 23:08, <ravi.sahita@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>Sent: Thursday, July 16, 2015 2:39 AM
>>
>>>>> On 16.07.15 at 11:20, <ravi.sahita@xxxxxxxxx> wrote:
>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>>Sent: Tuesday, July 14, 2015 6:57 AM
>>>>>>> On 14.07.15 at 02:14, <edmund.h.white@xxxxxxxxx> wrote:
>>>>> +static bool_t vmx_vcpu_emulate_ve(struct vcpu *v) {
>>>>> +    bool_t rc = 0;
>>>>> +    ve_info_t *veinfo = gfn_x(vcpu_altp2m(v).veinfo_gfn) !=
>>>>> +INVALID_GFN
>>>>?
>>>>> +        hvm_map_guest_frame_rw(gfn_x(vcpu_altp2m(v).veinfo_gfn), 0) :
>>>>> +NULL;
>>>>> +
>>>>> +    if ( !veinfo )
>>>>> +        return 0;
>>>>> +
>>>>> +    if ( veinfo->semaphore != 0 )
>>>>> +        goto out;
>>>>> +
>>>>> +    rc = 1;
>>>>> +
>>>>> +    veinfo->exit_reason = EXIT_REASON_EPT_VIOLATION;
>>>>> +    veinfo->semaphore = ~0l;
>>>>
>>>>Isn't semaphore a 32-bit quantity?
>>>
>>> Yes.
>>
>>I.e. the l suffix can and should be dropped.
>>
> 
> Ok.
> 
>>>>> +    {
>>>>> +        unsigned long idx;
>>>>> +
>>>>> +        if ( v->arch.hvm_vmx.secondary_exec_control &
>>>>> +            SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS )
>>>>> +            __vmread(EPTP_INDEX, &idx);
>>>>> +        else
>>>>> +        {
>>>>> +            unsigned long eptp;
>>>>> +
>>>>> +            __vmread(EPT_POINTER, &eptp);
>>>>> +
>>>>> +            if ( (idx = p2m_find_altp2m_by_eptp(v->domain, eptp)) ==
>>>>> +                 INVALID_ALTP2M )
>>>>> +            {
>>>>> +                gdprintk(XENLOG_ERR, "EPTP not found in alternate p2m
>>list\n");
>>>>> +                domain_crash(v->domain);
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        if ( (uint16_t)idx != vcpu_altp2m(v).p2midx )
>>>>
>>>>Is this cast really necessary?
>>>
>>> Yes - The index is 16-bits, this reflects how the field is specified
>>> in the vmcs also.
>>
>>While "yes" answers the question, the explanation you give suggests that the
>>answer may be wrong: Can idx indeed have bits set beyond bit 15? Because if
>>it can't, the cast is pointless.
>>
> 
> We were just trying to ensure we matched the hardware behavior (I think 
> there was a message George had posted earlier for SVE that asked for that).
> Since hardware considers only a 16 bit field we were doing the same.
> 
>>>>> +        {
>>>>> +            BUG_ON(idx >= MAX_ALTP2M);
>>>>> +            atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>>>>> +            vcpu_altp2m(v).p2midx = (uint16_t)idx;
>>>>
>>>>This one surely isn't (or else the field type is wrong).
>>>
>>> Again required. idx can't be uint16_t because __vmread() requires
>>> unsigned long*, but the index is 16 bits.
>>
>>But it's a 16-bit VMCS field that you read it from, and hence the upper 48 
> bits
>>are necessarily zero.
>>
>>Just to re-iterate: Casts are necessary in certain places, yes, but I see them
>>used pointlessly or even wrongly more often than not.
> 
> Same approach as above - emulating hardware exactly.
> Should we add a comment?

No, drop the casts.

Jan


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