Re: [Xen-devel] [PATCH v7 10/15] x86/altp2m: add remaining support routines.

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

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Thursday, July 23, 2015 3:06 AM
>
>>>> On 23.07.15 at 01:01, <edmund.h.white@xxxxxxxxx> wrote:
>> Add the remaining routines required to support enabling the alternate
>> p2m functionality.
>>
>> Signed-off-by: Ed White <edmund.h.white@xxxxxxxxx>
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> Changes since v6:
>>         rename altp2m lazy copier, make bool_t, use __put_gfn throughout,
>>           and move to p2m.c, eliminating altp2m_hap.c
>>         change various p2m_altp2m... routines from long to int
>>         change uint16_t's/uint8_t's to unsigned int's
>>         optimize remapped gfn tracking and altp2m invalidation check
>>         mechanical changes due to patch 5 changes
>
>Taken together clearly requiring dropping any earlier review tags.
>
>Non-mm parts
>Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>

Ok  - other maintainers, please review the mm parts - thanks.

>> +void p2m_flush_altp2m(struct domain *d) {
>> +    unsigned int i;
>> +
>> +    altp2m_list_lock(d);
>> +
>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        p2m_flush_table(d->arch.altp2m_p2m[i]);
>> +        /* Uninit and reinit ept to force TLB shootdown */
>> +        ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>> +        ept_p2m_init(d->arch.altp2m_p2m[i]);
>> +        d->arch.altp2m_eptp[i] = INVALID_MFN;
>> +    }
>
>And more EPT-specific code in a generic file...
>

This was mentioned explicitly as pending in the cover letter and in the patch 
header.
"    Patch 10:   ... not done - abstracting some ept functionality from p2m"

>> +int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) {
>> +    struct p2m_domain *p2m;
>> +    int rc = -EINVAL;
>> +
>> +    if ( !idx || idx > MAX_ALTP2M )
>
>     >= (and then also elsewhere further down)?
>

Right.

>> +        return rc;
>> +
>> +    domain_pause_except_self(d);
>> +
>> +    altp2m_list_lock(d);
>> +
>> +    if ( d->arch.altp2m_eptp[idx] != INVALID_MFN )
>> +    {
>> +        p2m = d->arch.altp2m_p2m[idx];
>> +
>> +        if ( !_atomic_read(p2m->active_vcpus) )
>> +        {
>> +            p2m_flush_table(d->arch.altp2m_p2m[idx]);
>> +            /* Uninit and reinit ept to force TLB shootdown */
>> +            ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>> +            ept_p2m_init(d->arch.altp2m_p2m[idx]);
>> +            d->arch.altp2m_eptp[idx] = INVALID_MFN;
>
>Perhaps worth making all of the above if() body a helper function (considering
>that the loop body in p2m_flush_altp2m() does exactly the same)?
>

Ok can consider.


>> @@ -758,6 +765,38 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct
>vcpu
>> *v, unsigned int idx);
>>  /* Check to see if vcpu should be switched to a different p2m. */
>> void p2m_altp2m_check(struct vcpu *v, uint16_t idx);
>
>The context here suggests that I overlooked a still not replaced uint16_t.
>

Ok.

Just wanted to make sure these are also ok to do post 4.6

Thanks,
Ravi

>> +/* Flush all the alternate p2m's for a domain */ void
>> +p2m_flush_altp2m(struct domain *d);
>> +
>> +/* Alternate p2m paging */
>> +bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
>> +    unsigned long gla, struct npfec npfec, struct p2m_domain **ap2m);
>> +
>> +/* Make a specific alternate p2m valid */ int
>> +p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);
>> +
>> +/* Find an available alternate p2m and make it valid */ int
>> +p2m_init_next_altp2m(struct domain *d, uint16_t *idx);
>
>For this one, however, things really depend on the intended call sites, i.e. I
>could see reasons for this to be kept as is.
>
>Jan


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