Re: [PATCH v5 05/15] x86/altp2m: basic data structures and support routines.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.07.15 at 10:57, <ravi.sahita@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>Sent: Tuesday, July 14, 2015 6:13 AM
>>>>> On 14.07.15 at 02:14, <edmund.h.white@intel.com> wrote:
>>> @@ -722,6 +731,27 @@ void nestedp2m_write_p2m_entry(struct
>>p2m_domain *p2m, unsigned long gfn,
>>>      l1_pgentry_t *p, l1_pgentry_t new, unsigned int level);
>>>
>>>  /*
>>> + * Alternate p2m: shadow p2m tables used for alternate memory views
>>> + */
>>> +
>>> +/* get current alternate p2m table */ static inline struct p2m_domain
>>> +*p2m_get_altp2m(struct vcpu *v) {
>>> +    struct domain *d = v->domain;
>>> +    uint16_t index = vcpu_altp2m(v).p2midx;
>>> +
>>> +    ASSERT(index < MAX_ALTP2M);
>>> +
>>> +    return (index == INVALID_ALTP2M) ? NULL :
>>> +d->arch.altp2m_p2m[index]; }
>>
>>Looking at this again, I'm afraid I'd still prefer index < MAX_ALTP2M in the
>>return statement (and the ASSERT() dropped): The ASSERT() does nothing in a
>>debug=n build, and hence wouldn't shield us from possibly having to issue an
>>XSA if somehow an access outside the array's bounds turned out possible.
>>
> 
> the assert was breaking v5 anyway. BUG_ON (with the right check) is probably 
> the right thing to do, as we do in the exit handling that checks for a VMFUNC 
> having changed the index.
> So will make that change.

But why use a BUG_ON() when you can deal with this more
gracefully? Please try to avoid crashing the hypervisor when
there are other ways to recover.

>>I've also avoided to repeat any of the un-addressed points that I raised 
> against
>>earlier versions.
>>
> 
> I went back and looked at the earlier versions of the comments on this patch 
> and afaict we have either addressed (accepted) those points or described why 
> they shouldn't cause changes with reasoning . so if I missed something please 
> let me know.

Just one example of what wasn't done is the conversion of local
variable, function return, and function parameter types from
(bogus) uint8_t / uint16_t to unsigned int (iirc also in other
patches).

Jan