On 17.02.2020 16:14, Jan Beulich wrote:
> On 30.01.2020 14:07, Alexandru Stefan ISAILA wrote:
>> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
>> break;
>> }
>>
>> + case HVMOP_altp2m_set_visibility:
>> + {
>> + uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>> +
>> + if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
>> + rc = -EINVAL;
>> + else
>> + {
>> + if ( !altp2m_active(d) || !hap_enabled(d) )
>
> Doesn't altp2m_active() imply hap_enabled()? At the very least
> there's no other use of hap_enabled() in do_altp2m_op().
Yes, the hap_enabled can be dropped.
>
>> + {
>> + rc = -EOPNOTSUPP;
>> + break;
>> + }
>> +
>> + if ( a.u.set_visibility.visible )
>> + d->arch.altp2m_working_eptp[altp2m_idx] =
>> + d->arch.altp2m_eptp[altp2m_idx];
>> + else
>> + d->arch.altp2m_working_eptp[altp2m_idx] =
>> + mfn_x(INVALID_MFN);
>> + }
>> + break;
>
> Also the code here lends itself to reduction of indentation
> depth:
>
> case HVMOP_altp2m_set_visibility:
> {
> uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>
> if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
> rc = -EINVAL;
> else if ( !altp2m_active(d) || !hap_enabled(d) )
> rc = -EOPNOTSUPP;
> else if ( a.u.set_visibility.visible )
> d->arch.altp2m_working_eptp[altp2m_idx] =
> d->arch.altp2m_eptp[altp2m_idx];
> else
> d->arch.altp2m_working_eptp[altp2m_idx] =
> mfn_x(INVALID_MFN);
>
> break;
> }
>
>
> Also note the altered indentation of the assignments.
I will fix the else if alignment as well as the indentation for the
assignments.
>
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
>> goto out;
>> }
>>
>> + if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
>> + {
>> + rv = -ENOMEM;
>> + goto out;
>> + }
>
> Isn't there a pre-existing error handling issue here which you
> widen, in that later encountered errors don't cause clean up
> of what had already succeeded before?
It seems non of the errors perform a cleanup. It might be better to have
a general cleanup done at "out:" if ( !rv ) and then check what should
be "p2m_teardown()" or "free_xenheap_page()".
>
>> @@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d,
>> unsigned int idx)
>> p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
>> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> mfn_x(INVALID_MFN);
>> + d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> + mfn_x(INVALID_MFN);
>
> Like above, and irrespective of you cloning existing code -
> indentation of the 2nd line is wrong here.
>
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
>> uint16_t altp2m_idx;
>> };
>>
>> +struct xen_hvm_altp2m_set_visibility {
>> + uint16_t altp2m_idx;
>> + uint8_t visible;
>> + uint8_t pad;
>> + uint32_t pad2;
>> +};
>
> What is pad2 good/intended for? 32-bit padding fields in some
> other structures are needed because one or more uint64_t
> fields follow, but this isn't the case here.
Right, pad2 can be dropped.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
