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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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