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

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

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Thursday, July 23, 2015 2:22 AM
>
>>>> On 23.07.15 at 01:01, <edmund.h.white@xxxxxxxxx> wrote:
>> @@ -6569,6 +6571,25 @@ void hvm_toggle_singlestep(struct vcpu *v)
>>      v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;  }
>>
>> +void altp2m_vcpu_update_p2m(struct vcpu *v) {
>> +    if ( hvm_funcs.altp2m_vcpu_update_p2m )
>> +        hvm_funcs.altp2m_vcpu_update_p2m(v);
>> +}
>> +
>> +void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v) {
>> +    if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
>> +        hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
>> +}
>> +
>> +bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) {
>> +    if ( hvm_funcs.altp2m_vcpu_emulate_ve )
>> +        return hvm_funcs.altp2m_vcpu_emulate_ve(v);
>> +    return 0;
>> +}
>
>These are _still_ not inline functions (in hvm.h), and 00/15 also doesn't
>mention that this was intentionally left out.
>

Yup possibly missed this one.

>> @@ -498,6 +498,28 @@ int hap_enable(struct domain *d, u32 mode)
>>             goto out;
>>      }
>>
>> +    if ( hvm_altp2m_supported() )
>> +    {
>> +        /* 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;
>
>And there is _still_ EPT-specific code left in a generic file.
>

This was mentioned explicitly in the cover letter and in the patch header - 
Altough our error was that the comment was only in patch 10, and should have 
been in patch 5 also.
"    Patch 10:   ... not done - abstracting some ept functionality from p2m"

>> @@ -183,6 +184,43 @@ static void p2m_teardown_nestedp2m(struct
>domain *d)
>>      }
>>  }
>>
>> +static void p2m_teardown_altp2m(struct domain *d) {
>> +    unsigned int i;
>> +    struct p2m_domain *p2m;
>> +
>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        if ( !d->arch.altp2m_p2m[i] )
>> +            continue;
>> +        p2m = d->arch.altp2m_p2m[i];
>> +        p2m_free_one(p2m);
>> +        d->arch.altp2m_p2m[i] = NULL;
>
>If you already think it's necessary to latch the array entry into a local 
>variable,
>why don't you zap the array entry _before_ freeing the structure?
>

Could be done.

>> @@ -1940,6 +1988,59 @@ int unmap_mmio_regions(struct domain *d,
>>      return err;
>>  }
>>
>> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
>> +{
>> +    struct p2m_domain *p2m;
>> +    struct ept_data *ept;
>> +    unsigned int i;
>> +
>> +    altp2m_list_lock(d);
>> +
>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
>> +            continue;
>> +
>> +        p2m = d->arch.altp2m_p2m[i];
>> +        ept = &p2m->ept;
>> +
>> +        if ( eptp == ept_get_eptp(ept) )
>> +            goto out;
>> +    }
>> +
>> +    i = INVALID_ALTP2M;
>> +
>> +out:
>

Missed sorry.


>Just to repeat - labels should be indented by at least one space.
>
>And you already know what the comment is regarding this being EPT-specific
>code (and here one can't even debate whether it's just unfortunate naming,
>since ept_get_eptp() is _very_ EPT- specific, and that macro - if headers were
>properly structured - shouldn't even be visible here).
>

Was mentioned albeit in another patch (should have been mentioned here also)

>> --- /dev/null
>> +++ b/xen/include/asm-x86/altp2m.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Alternate p2m HVM
>> + * Copyright (c) 2014, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> +modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but
>> +WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of
>MERCHANTABILITY
>> +or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
>> +License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> +along with
>> + * this program; if not, write to the Free Software Foundation, Inc.,
>> +59 Temple
>> + * Place - Suite 330, Boston, MA 02111-1307 USA.
>> + */
>> +
>> +#ifndef _ALTP2M_H
>> +#define _ALTP2M_H
>
>I'm sure I said so before - this is not a specific enough guard symbol.
>It should have at least an _X86 prefix.
>

Right (don't think I saw this before but comment makes sense)

>> +#include <xen/types.h>
>> +#include <xen/sched.h>         /* for struct vcpu, struct domain */
>> +#include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */
>
>I don't see the type mentioned in the comment used anywhere below - is this
>a stray include?
>
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -233,6 +233,10 @@ struct paging_vcpu {  typedef xen_domctl_cpuid_t
>> cpuid_input_t;
>>
>>  #define MAX_NESTEDP2M 10
>> +
>> +#define MAX_ALTP2M      ((uint16_t)10)
>> +#define INVALID_ALTP2M  ((uint16_t)~0)
>
>Didn't you claim to have removed all stray casts?
>
>Considering how late we are with this, this patch can have my ack only
>provided you promise to address _all_ of the issues above in follow-up
>patches.

Yes - no problem.

>
>Jan

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