On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:

> On 2/19/2016 8:27 PM, Tamas K Lengyel wrote:
>
>
>
> On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU <czuzu@bitdefender.com>
> wrote:
>
>> On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:
>>
>>
>>
>> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini <
>> stefano.stabellini@eu.citrix.com> 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