Re: [Xen-devel] [PATCH v3 11/13] x86/altp2m: define and implement alternate p2m HVMOP types.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.15 at 20:09, <edmund.h.white@xxxxxxxxx> wrote:
> +    case HVMOP_altp2m_set_domain_state:
> +    {
> +        struct xen_hvm_altp2m_domain_state a;
> +        struct domain *d;
> +        struct vcpu *v;
> +        bool_t ostate;
> +
> +        if ( copy_from_guest(&a, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_domain_by_any_id(a.domid);
> +        if ( d == NULL )
> +            return -ESRCH;

Is it indeed intended for a guest to enable this on itself?

> +        rc = -EINVAL;
> +        if ( is_hvm_domain(d) && hvm_altp2m_supported() &&
> +             !nestedhvm_enabled(d) )
> +        {
> +            ostate = d->arch.altp2m_active;
> +            d->arch.altp2m_active = !!a.state;
> +
> +            rc = 0;
> +
> +            /* If the alternate p2m state has changed, handle appropriately 
> */
> +            if ( d->arch.altp2m_active != ostate )
> +            {
> +                if ( ostate || !(rc = p2m_init_altp2m_by_id(d, 0)) )
> +                {

Two if()-s like these should be combined into one.

> +    case HVMOP_altp2m_vcpu_enable_notify:
> +    {
> +        struct domain *curr_d = current->domain;
> +        struct vcpu *curr = current;

The other way around please.

> +    case HVMOP_altp2m_set_mem_access:
> +    {
> +        struct xen_hvm_altp2m_set_mem_access a;
> +        struct domain *d;
> +
> +        if ( copy_from_guest(&a, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_domain_by_any_id(a.domid);
> +        if ( d == NULL )
> +            return -ESRCH;

Again - is it intended for this to be invokable by the guest for itself?
If so, is it being made certain that it can't increase access permissions
beyond what its controlling domain dictates?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -396,6 +396,75 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_evtchn_upcall_vector_t);
>  
>  #endif /* defined(__i386__) || defined(__x86_64__) */
>  
> +/* Set/get the altp2m state for a domain */
> +#define HVMOP_altp2m_set_domain_state     24
> +#define HVMOP_altp2m_get_domain_state     25
> +struct xen_hvm_altp2m_domain_state {
> +    /* Domain to be updated or queried */
> +    domid_t domid;
> +    /* IN or OUT variable on/off */
> +    uint8_t state;
> +};

Not sure if it was said before - explicit padding please (with padding
fields verified to be zero for future extensibility).

> +struct xen_hvm_altp2m_view {
> +    /* Domain to be updated */
> +    domid_t domid;
> +    /* IN/OUT variable */
> +    uint16_t view;

Is it certain that the number of views will never exceed 64k?

Jan


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