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

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Sunday, July 19, 2015 11:22 PM
>
>>>> 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

Ok

Ravi

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