Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Stefano Stabellini]

On Sat, 21 May 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 21/05/2016 14:27, Stefano Stabellini wrote:
> > > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> > > index 2d11b62..563f49b 100644
> > > --- a/xen/include/asm-arm/config.h
> > > +++ b/xen/include/asm-arm/config.h
> > > @@ -199,6 +199,8 @@ extern unsigned long frametable_virt_end;
> > >  #define watchdog_disable() ((void)0)
> > >  #define watchdog_enable()  ((void)0)
> > > 
> > > +#define VM_ASSIST_VALID  (0)
> > 
> > This is a nit, but as VM_ASSIST_VALID is only used with #ifdef, I would
> > just:
> > 
> > #define VM_ASSIST_VALID
> > 
> > the two previous #define are to 0, because they are replacing functions.
> 
> VM_ASSIST_VALID is used as an argument for vm_assist (see kernel.c) to know
> which option is valid. We have to define VM_ASSIST_VALID to 0 because there no
> valid option for the moment on ARM.

Ah, I missed that one. And I see that in the following patch is updated
with the new bit.

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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Julien Grall]

Hi Stefano,

On 21/05/2016 14:27, Stefano Stabellini wrote:

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 2d11b62..563f49b 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -199,6 +199,8 @@ extern unsigned long frametable_virt_end;
 #define watchdog_disable() ((void)0)
 #define watchdog_enable()  ((void)0)

+#define VM_ASSIST_VALID  (0)


This is a nit, but as VM_ASSIST_VALID is only used with #ifdef, I would
just:

#define VM_ASSIST_VALID

the two previous #define are to 0, because they are replacing functions.


VM_ASSIST_VALID is used as an argument for vm_assist (see kernel.c) to 
know which option is valid. We have to define VM_ASSIST_VALID to 0 
because there no valid option for the moment on ARM.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Stefano Stabellini]

On Fri, 20 May 2016, Juergen Gross wrote:
> Up to now the vm_assist hypercall hasn't been supported on ARM, as
> there are only x86 specific features to switch. Add support of
> vm_assist on ARM for future use.
> 
> Signed-off-by: Juergen Gross 
> ---
>  xen/arch/arm/traps.c | 1 +
>  xen/common/domain.c  | 2 --
>  xen/common/kernel.c  | 2 --
>  xen/include/asm-arm/config.h | 2 ++
>  4 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1828ea1..ccc6351 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1284,6 +1284,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
>  HYPERCALL(multicall, 2),
>  HYPERCALL(platform_op, 1),
>  HYPERCALL_ARM(vcpu_op, 3),
> +HYPERCALL(vm_assist, 2),
>  };
>  
>  #ifndef NDEBUG
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 45273d4..0afb1ee 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  return rc;
>  }
>  
> -#ifdef VM_ASSIST_VALID
>  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
> unsigned long valid)
>  {
> @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
> unsigned int type,
>  
>  return -ENOSYS;
>  }
> -#endif
>  
>  struct pirq *pirq_get_info(struct domain *d, int pirq)
>  {
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 1a6823a..74b6e1f 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  return rc;
>  }
>  
> -#ifdef VM_ASSIST_VALID
>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>  {
>  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
>  }
> -#endif
>  
>  DO(ni_hypercall)(void)
>  {
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 2d11b62..563f49b 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -199,6 +199,8 @@ extern unsigned long frametable_virt_end;
>  #define watchdog_disable() ((void)0)
>  #define watchdog_enable()  ((void)0)
>  
> +#define VM_ASSIST_VALID  (0)
 
This is a nit, but as VM_ASSIST_VALID is only used with #ifdef, I would
just:

#define VM_ASSIST_VALID

the two previous #define are to 0, because they are replacing functions.

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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Juergen Gross]

On 20/05/16 17:33, Jan Beulich wrote:
 On 20.05.16 at 17:08,  wrote:
>> On 20/05/16 16:51, Jan Beulich wrote:
>> On 20.05.16 at 16:42,  wrote:
 On 20/05/16 16:34, Jan Beulich wrote:
 On 20.05.16 at 15:22,  wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
 XEN_GUEST_HANDLE_PARAM(void) arg)
>>  return rc;
>>  }
>>  
>> -#ifdef VM_ASSIST_VALID
>>  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
>> unsigned long valid)
>>  {
>> @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
>> unsigned int type,
>>  
>>  return -ENOSYS;
>>  }
>> -#endif
>>  
>>  struct pirq *pirq_get_info(struct domain *d, int pirq)
>>  {
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
 XEN_GUEST_HANDLE_PARAM(void) arg)
>>  return rc;
>>  }
>>  
>> -#ifdef VM_ASSIST_VALID
>>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>>  {
>>  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
>>  }
>> -#endif
>
> Removing these #ifdef-s is neither necessary for this patch (at least
> afaict) nor desirable (after all they had got added so that an arch
> doesn't get this code compiled for no reason).

 Removing is not necessary, right.

 OTOH there is no arch left needing those #ifdef-s to be in place. Or do
 you think we should guard each single functionality in xen/common by
 such means? I don't think so. In this case keeping the #ifdef-s would be
 for historical reasons only.
>>>
>>> No, I don't want to go overboard with this. But we added these
>>> not so long ago, so I see no reason why they should now be
>>> removed again, just to maybe have them added in a couple of
>>> years again.
>>
>> Hmm, those #ifdef-s where needed for ARM, as on ARM there just was no
>> flag to be set via vm_assist hypercall. The new flag added in the next
>> patch is suitable for all architectures, so there would be no reason
>> for not supporting vm_assist in a new to be supported architecture.
> 
> Hmm, you have a point here. Otoh new architectures should
> probably assume that behavior even without having explicitly
> asked for it.

I don't think so, but you are the maintainer. In case there are no
objections by other maintainers I'll leave the #ifdef-s in place.


Juergen


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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Jan Beulich]

>>> On 20.05.16 at 17:08,  wrote:
> On 20/05/16 16:51, Jan Beulich wrote:
> On 20.05.16 at 16:42,  wrote:
>>> On 20/05/16 16:34, Jan Beulich wrote:
>>> On 20.05.16 at 15:22,  wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>  return rc;
>  }
>  
> -#ifdef VM_ASSIST_VALID
>  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
> unsigned long valid)
>  {
> @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
> unsigned int type,
>  
>  return -ENOSYS;
>  }
> -#endif
>  
>  struct pirq *pirq_get_info(struct domain *d, int pirq)
>  {
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>  return rc;
>  }
>  
> -#ifdef VM_ASSIST_VALID
>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>  {
>  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
>  }
> -#endif

 Removing these #ifdef-s is neither necessary for this patch (at least
 afaict) nor desirable (after all they had got added so that an arch
 doesn't get this code compiled for no reason).
>>>
>>> Removing is not necessary, right.
>>>
>>> OTOH there is no arch left needing those #ifdef-s to be in place. Or do
>>> you think we should guard each single functionality in xen/common by
>>> such means? I don't think so. In this case keeping the #ifdef-s would be
>>> for historical reasons only.
>> 
>> No, I don't want to go overboard with this. But we added these
>> not so long ago, so I see no reason why they should now be
>> removed again, just to maybe have them added in a couple of
>> years again.
> 
> Hmm, those #ifdef-s where needed for ARM, as on ARM there just was no
> flag to be set via vm_assist hypercall. The new flag added in the next
> patch is suitable for all architectures, so there would be no reason
> for not supporting vm_assist in a new to be supported architecture.

Hmm, you have a point here. Otoh new architectures should
probably assume that behavior even without having explicitly
asked for it.

Jan


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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Jan Beulich]

>>> On 20.05.16 at 16:42,  wrote:
> On 20/05/16 16:34, Jan Beulich wrote:
> On 20.05.16 at 15:22,  wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>  return rc;
>>>  }
>>>  
>>> -#ifdef VM_ASSIST_VALID
>>>  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
>>> unsigned long valid)
>>>  {
>>> @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
>>> unsigned int type,
>>>  
>>>  return -ENOSYS;
>>>  }
>>> -#endif
>>>  
>>>  struct pirq *pirq_get_info(struct domain *d, int pirq)
>>>  {
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>  return rc;
>>>  }
>>>  
>>> -#ifdef VM_ASSIST_VALID
>>>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>>>  {
>>>  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
>>>  }
>>> -#endif
>> 
>> Removing these #ifdef-s is neither necessary for this patch (at least
>> afaict) nor desirable (after all they had got added so that an arch
>> doesn't get this code compiled for no reason).
> 
> Removing is not necessary, right.
> 
> OTOH there is no arch left needing those #ifdef-s to be in place. Or do
> you think we should guard each single functionality in xen/common by
> such means? I don't think so. In this case keeping the #ifdef-s would be
> for historical reasons only.

No, I don't want to go overboard with this. But we added these
not so long ago, so I see no reason why they should now be
removed again, just to maybe have them added in a couple of
years again.

Jan


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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Juergen Gross]

On 20/05/16 16:51, Jan Beulich wrote:
 On 20.05.16 at 16:42,  wrote:
>> On 20/05/16 16:34, Jan Beulich wrote:
>> On 20.05.16 at 15:22,  wrote:
 --- a/xen/common/domain.c
 +++ b/xen/common/domain.c
 @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
  return rc;
  }
  
 -#ifdef VM_ASSIST_VALID
  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
 unsigned long valid)
  {
 @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
 unsigned int type,
  
  return -ENOSYS;
  }
 -#endif
  
  struct pirq *pirq_get_info(struct domain *d, int pirq)
  {
 --- a/xen/common/kernel.c
 +++ b/xen/common/kernel.c
 @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
  return rc;
  }
  
 -#ifdef VM_ASSIST_VALID
  DO(vm_assist)(unsigned int cmd, unsigned int type)
  {
  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
  }
 -#endif
>>>
>>> Removing these #ifdef-s is neither necessary for this patch (at least
>>> afaict) nor desirable (after all they had got added so that an arch
>>> doesn't get this code compiled for no reason).
>>
>> Removing is not necessary, right.
>>
>> OTOH there is no arch left needing those #ifdef-s to be in place. Or do
>> you think we should guard each single functionality in xen/common by
>> such means? I don't think so. In this case keeping the #ifdef-s would be
>> for historical reasons only.
> 
> No, I don't want to go overboard with this. But we added these
> not so long ago, so I see no reason why they should now be
> removed again, just to maybe have them added in a couple of
> years again.

Hmm, those #ifdef-s where needed for ARM, as on ARM there just was no
flag to be set via vm_assist hypercall. The new flag added in the next
patch is suitable for all architectures, so there would be no reason
for not supporting vm_assist in a new to be supported architecture.


Juergen


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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Juergen Gross]

On 20/05/16 16:34, Jan Beulich wrote:
 On 20.05.16 at 15:22,  wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>  return rc;
>>  }
>>  
>> -#ifdef VM_ASSIST_VALID
>>  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
>> unsigned long valid)
>>  {
>> @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
>> unsigned int type,
>>  
>>  return -ENOSYS;
>>  }
>> -#endif
>>  
>>  struct pirq *pirq_get_info(struct domain *d, int pirq)
>>  {
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>  return rc;
>>  }
>>  
>> -#ifdef VM_ASSIST_VALID
>>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>>  {
>>  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
>>  }
>> -#endif
> 
> Removing these #ifdef-s is neither necessary for this patch (at least
> afaict) nor desirable (after all they had got added so that an arch
> doesn't get this code compiled for no reason).

Removing is not necessary, right.

OTOH there is no arch left needing those #ifdef-s to be in place. Or do
you think we should guard each single functionality in xen/common by
such means? I don't think so. In this case keeping the #ifdef-s would be
for historical reasons only.

If you really want I can keep them or do the removal in a separate patch
if you want to split functional addition and related cleanup.


Juergen

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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Jan Beulich]

>>> On 20.05.16 at 15:22,  wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  return rc;
>  }
>  
> -#ifdef VM_ASSIST_VALID
>  long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
> unsigned long valid)
>  {
> @@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
> unsigned int type,
>  
>  return -ENOSYS;
>  }
> -#endif
>  
>  struct pirq *pirq_get_info(struct domain *d, int pirq)
>  {
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  return rc;
>  }
>  
> -#ifdef VM_ASSIST_VALID
>  DO(vm_assist)(unsigned int cmd, unsigned int type)
>  {
>  return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
>  }
> -#endif

Removing these #ifdef-s is neither necessary for this patch (at least
afaict) nor desirable (after all they had got added so that an arch
doesn't get this code compiled for no reason).

Jan


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

Re: [Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Julien Grall]

Hi Juergen,

On 20/05/16 14:22, Juergen Gross wrote:

Up to now the vm_assist hypercall hasn't been supported on ARM, as
there are only x86 specific features to switch. Add support of
vm_assist on ARM for future use.

Signed-off-by: Juergen Gross 


Reviewed-by: Julien Grall 

Regards,

--
Julien Grall

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

[Xen-devel] [PATCH 1/2] xen/arm: add support for vm_assist hypercall

[Juergen Gross]

Up to now the vm_assist hypercall hasn't been supported on ARM, as
there are only x86 specific features to switch. Add support of
vm_assist on ARM for future use.

Signed-off-by: Juergen Gross 
---
 xen/arch/arm/traps.c | 1 +
 xen/common/domain.c  | 2 --
 xen/common/kernel.c  | 2 --
 xen/include/asm-arm/config.h | 2 ++
 4 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1828ea1..ccc6351 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1284,6 +1284,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
 HYPERCALL(multicall, 2),
 HYPERCALL(platform_op, 1),
 HYPERCALL_ARM(vcpu_op, 3),
+HYPERCALL(vm_assist, 2),
 };
 
 #ifndef NDEBUG
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 45273d4..0afb1ee 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1408,7 +1408,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 return rc;
 }
 
-#ifdef VM_ASSIST_VALID
 long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
unsigned long valid)
 {
@@ -1427,7 +1426,6 @@ long vm_assist(struct domain *p, unsigned int cmd, 
unsigned int type,
 
 return -ENOSYS;
 }
-#endif
 
 struct pirq *pirq_get_info(struct domain *d, int pirq)
 {
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 1a6823a..74b6e1f 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -441,12 +441,10 @@ DO(nmi_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) 
arg)
 return rc;
 }
 
-#ifdef VM_ASSIST_VALID
 DO(vm_assist)(unsigned int cmd, unsigned int type)
 {
 return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
 }
-#endif
 
 DO(ni_hypercall)(void)
 {
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 2d11b62..563f49b 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -199,6 +199,8 @@ extern unsigned long frametable_virt_end;
 #define watchdog_disable() ((void)0)
 #define watchdog_enable()  ((void)0)
 
+#define VM_ASSIST_VALID  (0)
+
 #endif /* __ARM_CONFIG_H__ */
 /*
  * Local variables:
-- 
2.6.6


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