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: Sunday, July 19, 2015 11:20 PM
>
>>>> On 18.07.15 at 00:36, <ravi.sahita@xxxxxxxxx> wrote:
>>> 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.
>
>Generally (as an example), if a hypervisor bug can be confined to a guest,
>killing just the guest instead of the hypervisor would still be preferred
>(allowing the admin to attempt to gracefully shut down other guests before
>updating/restarting).
>
>Jan

I agree with that principle, and that's what I was looking to do in the last 
iteration, but in this sort of error condition there is no telling what else 
could have gone wrong on the hypervisor side to cause this, so it seems the 
crash treatment seems suitable.

Ravi



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