On 2/19/2016 8:42 PM, Tamas K Lengyel wrote: > > > On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU > wrote: > > On 2/19/2016 8:27 PM, Tamas K Lengyel wrote: >> >> >> On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU >> > wrote: >> >> On 2/19/2016 7:54 PM, Tamas K Lengyel wrote: >>> >>> >>> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini >>> >> > wrote: >>> >>> On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> > On 2/19/2016 6:05 PM, Andrew Cooper wrote: >>> > > On 19/02/16 16:00, Stefano Stabellini wrote: >>> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote: >>> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote: >>> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote: >>> > > > > > > + >>> > > > > > > + if ( sync ) >>> > > > > > > + { >>> > > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >>> > > > > > > + vm_event_vcpu_pause(v); >>> > > > > > > + } >>> > > > > > > + >>> > > > > > > +#if CONFIG_X86 >>> > > > > > > + if ( altp2m_active(d) ) >>> > > > > > I would rather >>> > > > > > >>> > > > > > #define altp2m_active(d) (0) >>> > > > > > >>> > > > > > on ARM, removing the two ifdefs in this file. >>> > > > > Yeah, I actually wanted to get rid of that too >>> at some point, the >>> > > > > question is, >>> > > > > what do I do with "req->altp2m_idx = >>> vcpu_altp2m(v).p2midx"? I'm not >>> > > > > familiar >>> > > > > w/ altp2m design, maybe someone that knows more >>> of the internals of that >>> > > > > can >>> > > > > give a suggestion. >>> > > > If you #define altp2m_active to (0), gcc will >>> automatically avoid the if >>> > > > statement. >>> > > You will still get the compile error from ARM's >>> struct vcpu not having >>> > > altp2m information. >>> > > >>> > > ~Andrew >>> > > >>> > >>> > Yep. >>> >>> Yes, you are right, especially given that Xen is >>> compiled -Wall -Werror. >>> >>> How do you plan to introduce altp2m support on ARM? Is >>> there going to be >>> a struct altp2mvcpu on ARM too? It is not nice to access >>> stuff under >>> v->arch from common code. Maybe we need another >>> arch_blah function to >>> set altp2m_idx. >>> >>> >>> As altp2m could be implemented for ARM as well it might make >>> sense to start introducing bits and pieces that would make >>> it easier to do that work in the future. But I agree, >>> accessing v->arch directly from common is not a good way to >>> go about it. >>> >>> Tamas >> >> I am not at all familiar w/ altp2m at the moment, but I'll >> try to look into it. >> Since that doesn't relate so much with the code motion of >> this changeset and it might not be that straightforward to >> implement, would it be ok to leave the #ifdef CONFIG_X86 >> there for now and remove it in a separate patch? >> >> >> We are trying to avoid having to do ifdefs where-ever possible. >> So in this case too introducing arch-specific function(s) that >> are empty for ARM would be more appropriate. >> >> Tamas >> > > I understand that, I was merely asking if it would be okay to do > it in another patch, because it didn't seem that straightforward. > More concretely, are you suggesting to: > * do the "#define altp2m_active(d) (0)" as Stefano suggested > > > That is easy enough to implement so go with that. > > * incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function > > > Implement an arch-specific function for this, say > p2m_get_vcpu_altp2m_idx(v) which would just return 0 on ARM for now. > > Tamas > Got it. Corneliu.