Re: [PATCH v4 11/15] x86/altp2m: define and implement alternate p2m HVMOP types.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.07.15 at 02:52, <edmund.h.white@intel.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6443,6 +6443,144 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case HVMOP_altp2m:
> +    {
> +        struct xen_hvm_altp2m_op a;
> +        struct domain *d = NULL;
> +
> +        if ( copy_from_guest(&a, arg, 1) )
> +            return -EFAULT;
> +
> +        switch ( a.cmd )
> +        {
> +        case HVMOP_altp2m_get_domain_state:
> +        case HVMOP_altp2m_set_domain_state:
> +        case HVMOP_altp2m_create_p2m:
> +        case HVMOP_altp2m_destroy_p2m:
> +        case HVMOP_altp2m_switch_p2m:
> +        case HVMOP_altp2m_set_mem_access:
> +        case HVMOP_altp2m_change_gfn:
> +            d = rcu_lock_domain_by_any_id(a.domain);
> +            if ( d == NULL )
> +                return -ESRCH;
> +
> +            if ( !is_hvm_domain(d) || !hvm_altp2m_supported() )
> +                rc = -EINVAL;
> +
> +            break;
> +        case HVMOP_altp2m_vcpu_enable_notify:
> +
> +            break;

The blank line ought to go ahead of the case label.

> +        default:
> +            return -ENOSYS;
> +
> +            break;

Bogus (unreachable) break.

> +        }
> +
> +        if ( !rc )
> +        {
> +            switch ( a.cmd )
> +            {
> +            case HVMOP_altp2m_get_domain_state:
> +                a.u.domain_state.state = altp2m_active(d);
> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +
> +                break;
> +            case HVMOP_altp2m_set_domain_state:
> +            {
> +                struct vcpu *v;
> +                bool_t ostate;
> +                
> +                if ( nestedhvm_enabled(d) )
> +                {
> +                    rc = -EINVAL;
> +                    break;
> +                }
> +
> +                ostate = d->arch.altp2m_active;
> +                d->arch.altp2m_active = !!a.u.domain_state.state;
> +
> +                /* If the alternate p2m state has changed, handle appropriately */
> +                if ( d->arch.altp2m_active != ostate &&
> +                     (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
> +                {
> +                    for_each_vcpu( d, v )
> +                    {
> +                        if ( !ostate )
> +                            altp2m_vcpu_initialise(v);
> +                        else
> +                            altp2m_vcpu_destroy(v);
> +                    }
> +
> +                    if ( ostate )
> +                        p2m_flush_altp2m(d);
> +                }
> +
> +                break;
> +            }
> +            default:
> +            {

Pointless brace.

> +                if ( !(d ? d : current->domain)->arch.altp2m_active )

This is bogus: d is NULL if and only if altp2m_vcpu_enable_notify,
i.e. I don't see why you can't just use current->domain inside that
case (and you really do). That would then also eliminate the need
for this redundant and obfuscating switch() nesting you use.

> +
> +struct xen_hvm_altp2m_set_mem_access {
> +    /* view */
> +    uint16_t view;
> +    /* Memory type */
> +    uint16_t hvmmem_access; /* xenmem_access_t */
> +    uint8_t pad[4];
> +    /* gfn */
> +    uint64_t gfn;
> +};
> +typedef struct xen_hvm_altp2m_set_mem_access 
> xen_hvm_altp2m_set_mem_access_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> +
> +struct xen_hvm_altp2m_change_gfn {
> +    /* view */
> +    uint16_t view;
> +    uint8_t pad[6];
> +    /* old gfn */
> +    uint64_t old_gfn;
> +    /* new gfn, INVALID_GFN (~0UL) means revert */
> +    uint64_t new_gfn;
> +};
> +typedef struct xen_hvm_altp2m_change_gfn xen_hvm_altp2m_change_gfn_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t);
> +
> +struct xen_hvm_altp2m_op {
> +    uint32_t cmd;
> +/* Get/set the altp2m state for a domain */
> +#define HVMOP_altp2m_get_domain_state     1
> +#define HVMOP_altp2m_set_domain_state     2
> +/* Set the current VCPU to receive altp2m event notifications */
> +#define HVMOP_altp2m_vcpu_enable_notify   3
> +/* Create a new view */
> +#define HVMOP_altp2m_create_p2m           4
> +/* Destroy a view */
> +#define HVMOP_altp2m_destroy_p2m          5
> +/* Switch view for an entire domain */
> +#define HVMOP_altp2m_switch_p2m           6
> +/* Notify that a page of memory is to have specific access types */
> +#define HVMOP_altp2m_set_mem_access       7
> +/* Change a p2m entry to have a different gfn->mfn mapping */
> +#define HVMOP_altp2m_change_gfn           8
> +    domid_t domain;
> +    uint8_t pad[2];

While you added padding fields as asked for, you still don't verify
them to be zero on input.

Afaict all other questions raised on v3 still stand.

Jan