From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sahita, Ravi" Subject: Re: [PATCH v7 05/15] x86/altp2m: basic data structures and support routines. Date: Thu, 23 Jul 2015 14:36:38 +0000 Message-ID: References: <1437606081-6964-1-git-send-email-edmund.h.white@intel.com> <1437606081-6964-6-git-send-email-edmund.h.white@intel.com> <55B0CE6702000078000946B5@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55B0CE6702000078000946B5@prv-mh.provo.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , "Sahita, Ravi" , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , "White, Edmund H" , "xen-devel@lists.xen.org" , "Nakajima, Jun" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >From: Jan Beulich [mailto:JBeulich@suse.com] >Sent: Thursday, July 23, 2015 2:22 AM > >>>> On 23.07.15 at 01:01, 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 >> +#include /* for struct vcpu, struct domain */ >> +#include /* 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