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

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

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Thursday, July 16, 2015 2:08 AM
>
>>>> On 16.07.15 at 10:57, <ravi.sahita@xxxxxxxxx> wrote:
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>Sent: Tuesday, July 14, 2015 6:13 AM
>>>>>> On 14.07.15 at 02:14, <edmund.h.white@xxxxxxxxx> 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.
>

So in this case there isnt a graceful fallback; this case can happen only if 
there is a bug in the hypervisor - which should be reported via the BUG_ON.

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

Working through these as best as can (treating it as Category 4)

Thanks,
Ravi


>
>Jan


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