Re: [Xen-devel] [PATCH v3 05/13] x86/altp2m: basic data structures and support routines.

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

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Thursday, July 09, 2015 6:30 AM
>
>>>> On 01.07.15 at 20:09, <edmund.h.white@xxxxxxxxx> wrote:
>> ---
>>  xen/arch/x86/hvm/Makefile        |  1 +
>>  xen/arch/x86/hvm/altp2m.c        | 92
>+++++++++++++++++++++++++++++++++++++
>
>Wouldn't this better go into xen/arch/x86/mm/?

In this case we followed the pattern of nestedhvm - hope that's ok.

>
>> +int
>> +altp2m_vcpu_initialise(struct vcpu *v) {
>> +    int rc = -EOPNOTSUPP;
>> +
>> +    if ( v != current )
>> +        vcpu_pause(v);
>> +
>> +    if ( !hvm_funcs.ap2m_vcpu_initialise ||
>> +         (hvm_funcs.ap2m_vcpu_initialise(v) == 0) )
>> +    {
>> +        rc = 0;
>
>I think you would better honor the error code returned by
>hvm_funcs.ap2m_vcpu_initialise() and enter this block only if it was zero.

The code is checking that condition - did I misinterpret?

>
>> +        altp2m_vcpu_reset(v);
>> +        vcpu_altp2m(v).p2midx = 0;
>> +        atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
>> +
>> +        ap2m_vcpu_update_eptp(v);
>
>We're in vendor independent code here - either the function is misnamed, or
>it shouldn't be called directly from here.

Would it be reasonable to add if hap_enabled and cpu_has_vmx checks like other 
code in this file that invokes ept specific ops?
Otherwise it seems ok that the function would be called from here for 
p2m_altp2m interactions such as switching altp2m by id etc.
Open to any other suggestions from you, or we would like to leave it as it is.

>
>> +void
>> +altp2m_vcpu_destroy(struct vcpu *v)
>> +{
>> +    struct p2m_domain *p2m;
>> +
>> +    if ( v != current )
>> +        vcpu_pause(v);
>> +
>> +    if ( hvm_funcs.ap2m_vcpu_destroy )
>> +        hvm_funcs.ap2m_vcpu_destroy(v);
>> +
>> +    if ( (p2m = p2m_get_altp2m(v)) )
>> +        atomic_dec(&p2m->active_vcpus);
>
>The ordering looks odd - from an abstract perspective I'd expect
>p2m_get_altp2m() to not return the p2m anymore that was just destroyed via
>hvm_funcs.ap2m_vcpu_destroy().
>

ap2m_vcpu_destroy is for destroying vcpu context related to altp2m - note this 
is not implemented since its not needed for Intel implementation.  The idea is 
that if something needs to be done specifically for for AMD then that could be 
done here. 

>> +void ap2m_vcpu_update_eptp(struct vcpu *v) {
>
>As I think I said before, I consider these ap2m_ prefixes ambiguous - the 'a'
>could also stand for accelerated, advanced, ... Consistently staying with
>altp2m_ would seem better.
>

We have a comment above the list of these ap2m_ functions in hvm.h stating 
these are for Alternate p2m - do you feel strongly about us changing this 
naming? Also this is the interface naming, and if we renamed it altp2m_xxx it 
would cause confusion with the actual altp2m_xx functionality - so we would 
like to leave it as proposed.

>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -459,7 +459,7 @@ void hap_domain_init(struct domain *d)  int
>> hap_enable(struct domain *d, u32 mode)  {
>>      unsigned int old_pages;
>> -    uint8_t i;
>> +    uint16_t i;
>
>unsigned int (also elsewhere, including uint8_t-s)

We used existing iterator types that were being used (uint8_t was being used in 
hap_final_teardown).
If you feel strongly we could change it but we would change code that we didn't 
need to touch for this patch.

>
>> @@ -498,6 +498,24 @@ int hap_enable(struct domain *d, u32 mode)
>>             goto out;
>>      }
>>
>> +    /* Init alternate p2m data */
>> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
>> +    {
>> +        rv = -ENOMEM;
>> +        goto out;
>> +    }
>> +
>> +    for (i = 0; i < MAX_EPTP; i++)
>> +        d->arch.altp2m_eptp[i] = INVALID_MFN;
>
>The above again seems EPT-specific in a vendor independent file.

See above comment.

>
>> @@ -196,7 +234,14 @@ int p2m_init(struct domain *d)
>>       * (p2m_init runs too early for HVM_PARAM_* options) */
>>      rc = p2m_init_nestedp2m(d);
>>      if ( rc )
>> +    {
>>          p2m_teardown_hostp2m(d);
>> +        return rc;
>> +    }
>> +
>> +    rc = p2m_init_altp2m(d);
>> +    if ( rc )
>> +        p2m_teardown_altp2m(d);
>>
>>      return rc;
>>  }
>
>And why not also p2m_teardown_hostp2m() in this last error case?
>And doesn't p2m_init_nestedp2m() need undoing too? (Overall this
>suggests the error cleanup structuring here should be changed.)

Sounds right - we will make this change.

>
>> @@ -294,6 +298,12 @@ struct arch_domain
>>      struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
>>      mm_lock_t nested_p2m_lock;
>>
>> +    /* altp2m: allow multiple copies of host p2m */
>> +    bool_t altp2m_active;
>> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
>> +    mm_lock_t altp2m_lock;
>> +    uint64_t *altp2m_eptp;
>
>This is a non-insignificant increase of the structure size - perhaps all
>of these should hang off of struct arch_domain via a single,
>separately allocated pointer?

Is this a nice-to-have - again we modelled after the nestedhvm extensions to 
the struct.
This will affect a lot of our patch without really changing how much memory is 
allocated.

>
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -210,6 +210,14 @@ struct hvm_function_table {
>>                                    uint32_t *ecx, uint32_t *edx);
>>
>>      void (*enable_msr_exit_interception)(struct domain *d);
>> +
>> +    /* Alternate p2m */
>> +    int (*ap2m_vcpu_initialise)(struct vcpu *v);
>> +    void (*ap2m_vcpu_destroy)(struct vcpu *v);
>> +    int (*ap2m_vcpu_reset)(struct vcpu *v);
>
>Why is this returning int when altp2m_vcpu_reset() returns void?

Currently altp2m_vcpu_reset cannot fail - but at the interface level from hvm, 
we wanted to allow someone to change that in the future, so the interface 
allows for a return code.

>
>> +static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +    uint16_t index = vcpu_altp2m(v).p2midx;
>> +
>> +    return (index == INVALID_ALTP2M) ? NULL : d-
>>arch.altp2m_p2m[index];
>
>It would feel better if you checked against MAX_ALTP2M here.

There is no way for p2midx to be >= MAX_ALTP2M without being INVALID_ALTP2M.

Thanks,
Ravi

>
>Jan

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