On 03.12.2019 10:14, Jan Beulich wrote:
> On 02.12.2019 15:40, Alexandru Stefan ISAILA wrote:
>> On 29.11.2019 13:31, Jan Beulich wrote:
>>> On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote:
>>>> @@ -4711,6 +4712,18 @@ static int do_altp2m_op(
>>>> }
>>>> break;
>>>>
>>>> + case HVMOP_altp2m_set_suppress_ve_multi:
>>>> + if ( a.u.suppress_ve_multi.pad1 || !a.u.suppress_ve_multi.pad2 )
>>>> + rc = -EINVAL;
>>>> + else
>>>> + {
>>>> + rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve_multi);
>>>> +
>>>> + if ( __copy_to_guest(arg, &a, 1) )
>>>> + rc = -EFAULT;
>>>
>>> Do you really want to replace a possible prior error here?
>>
>> I thought about this and the only error that can be replaced here is
>> EINVAL. A error on __copy_to_guest has a grater importance if this fails.
>
> I'm afraid I don't understand the reference to EINVAL.
>
> As to "greater importance" - I'm not sure I follow. Please take a
> look at e.g. do_event_channel_op(), but there are numerous other
> examples throughout the tree. The pattern there is a common on,
> and what you do here doesn't match that.
I will follow that pattern.
>
>>>> + while ( sve->last_gfn >= start )
>>>
>>> There are no checks on ->last_gfn, ->first_gfn, or ->opaque.
>>> At the very least a bogus ->opaque should result in an error.
>>> I wonder though why you don't simply update ->first_gfn,
>>> omitting the need for ->opaque. All this would need is a
>>> comment in the public header clarifying that callers should
>>> expect the values to change.
>>
>> I was following the pattern from range_share() after Tamas requested the
>> opaque field. I agree that it would be simpler to have ->first_gfn
>> update and I can change to that in the next version.
>>
>>> Furthermore I think it would be helpful to bail on entirely
>>> out of range ->first_gfn. This being a 64-bit field, only
>>> 40 of the bits are actually usable from an architecture pov
>>> (in reality it may be even less). Otherwise you potentially
>>> invoke p2m_ept_set_entry() perhaps trillions of times just
>>> for it to return -EINVAL from its first if().
>>
>> Do you mean to check ->first_gfn(that will be updated in the next
>> version) against domain_get_maximum_gpfn() and bail after that range?
>
> This may be one possibility (depending on what the inteneded
> behavior for GFNs above this value is). Another would be to
> simply judge from the guest's CPUID setting for the number of
> physical address bits.
>
I will check ->first_gfn against d->arch.cpuid->extd.maxphysaddr and
bail out on out of range.
Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
