Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> +            {
> +                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.

> --- 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?

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

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel