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

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Friday, July 10, 2015 3:01 AM
>
>>>> On 10.07.15 at 02:52, <edmund.h.white@xxxxxxxxx> 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.

ok

>
>> +        default:
>> +            return -ENOSYS;
>> +
>> +            break;
>
>Bogus (unreachable) break.

Wanted to keep this so that if someone removes the error code then they don't 
cause an invalid fall through.
But ok with removing it if you think so.

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

ok

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

We need to check if the target domain is in altp2m mode for all the following 
sub-ops.
If we removed this check, we would need to check for target domain being in 
altp2m for all the following cases.
Andrew wanted to refactor and pull common code up, and that's what this is one 
case of for hvm_op.

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

Specifically what were you thinking we need to do here - also would be good if 
you can explain what was the underlying concern? (thanks)

>
>Afaict all other questions raised on v3 still stand.
>

Those were addressed in another thread.

Thanks,
Ravi

>Jan

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