On 27.12.2019 10:01, Jan Beulich wrote:
> (re-sending, as I still don't see the mail having appeared on the list)
>
> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>> Changes since V5:
>> - Add black lines
>
> Luckily no color comes through in plain text mails ;-)
>
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -366,11 +366,12 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
>> uint32_t nr,
>> #ifdef CONFIG_HVM
>> if ( altp2m_idx )
>> {
>> - if ( altp2m_idx >= MAX_ALTP2M ||
>> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
>
> Stray blank after >= .
>
>> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)]
>> ==
>
> I accept you can't (currently) use array_access_nospec() here,
> but ...
>
>> + mfn_x(INVALID_MFN) )
>> return -EINVAL;
>>
>> - ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> + ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
>> MAX_ALTP2M)];
>
> ... I don't see why you still effectively open-code it here.
I am not sure I follow you here, that is what we agreed in v5
(https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html).
Did I miss something?
>
>> @@ -425,11 +426,12 @@ long p2m_set_mem_access_multi(struct domain *d,
>> #ifdef CONFIG_HVM
>> if ( altp2m_idx )
>> {
>> - if ( altp2m_idx >= MAX_ALTP2M ||
>> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
>> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)]
>> ==
>> + mfn_x(INVALID_MFN) )
>> return -EINVAL;
>>
>> - ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> + ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
>> MAX_ALTP2M)];
>
> Same two remarks here then, and again further down.
>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2577,6 +2577,8 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned
>> int idx)
>> if ( idx >= MAX_ALTP2M )
>> return rc;
>>
>> + idx = array_index_nospec(idx, MAX_ALTP2M);
>> +
>> altp2m_list_lock(d);
>>
>> if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>
> What about this array access?
>
>> @@ -2618,6 +2620,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d,
>> unsigned int idx)
>> if ( !idx || idx >= MAX_ALTP2M )
>> return rc;
>>
>> + idx = array_index_nospec(idx, MAX_ALTP2M);
>
> There's a d->arch.altp2m_eptp[] access down from here too. I'm not
> going to look further. Please get things into consistent shape while
> you do this transformation.
>
I will change the idx part in p2m_init_altp2m_by_id() and
p2m_destroy_altp2m_by_id() so they match the rest of the checks:
"if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP))...", drop
the idx = array_index_nospec(idx, MAX_ALTP2M); and have
array_index_nospec() into place.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
