[PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

---
 arch/x86/include/asm/xen/hypercall.h |    2 ++
 1 file changed, 2 insertions(+)

--- linux-2.6.29-rc7/arch/x86/include/asm/xen/hypercall.h	2009-03-11 17:52:10.000000000 +0100
+++ 2.6.29-rc7-x86_64-xen-update-descr/arch/x86/include/asm/xen/hypercall.h	2009-02-13 11:41:39.000000000 +0100
@@ -296,6 +296,8 @@ HYPERVISOR_get_debugreg(int reg)
 static inline int
 HYPERVISOR_update_descriptor(u64 ma, u64 desc)
 {
+	if (sizeof(u64) == sizeof(long))
+		return _hypercall2(int, update_descriptor, ma, desc);
 	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);
 }
 

Re: [PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 
> ---
>  arch/x86/include/asm/xen/hypercall.h |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- linux-2.6.29-rc7/arch/x86/include/asm/xen/hypercall.h	2009-03-11 17:52:10.000000000 +0100
> +++ 2.6.29-rc7-x86_64-xen-update-descr/arch/x86/include/asm/xen/hypercall.h	2009-02-13 11:41:39.000000000 +0100
> @@ -296,6 +296,8 @@ HYPERVISOR_get_debugreg(int reg)
>  static inline int
>  HYPERVISOR_update_descriptor(u64 ma, u64 desc)
>  {
> +	if (sizeof(u64) == sizeof(long))
> +		return _hypercall2(int, update_descriptor, ma, desc);
>  	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);

missing changelog and Impact line.

	Ingo

Re: [PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 12.03.09 11:54 >>>
>* Jan Beulich <jbeulich@novell.com> wrote:
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>> 
>> ---
>>  arch/x86/include/asm/xen/hypercall.h |    2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> --- linux-2.6.29-rc7/arch/x86/include/asm/xen/hypercall.h	2009-03-11 17:52:10.000000000 +0100
>> +++ 2.6.29-rc7-x86_64-xen-update-descr/arch/x86/include/asm/xen/hypercall.h	2009-02-13 11:41:39.000000000 +0100
>> @@ -296,6 +296,8 @@ HYPERVISOR_get_debugreg(int reg)
>>  static inline int
>>  HYPERVISOR_update_descriptor(u64 ma, u64 desc)
>>  {
>> +	if (sizeof(u64) == sizeof(long))
>> +		return _hypercall2(int, update_descriptor, ma, desc);
>>  	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);
>
>missing changelog and Impact line.

I'm confused: What point is there to add a textual description that matches
the subject? And where is the need for an impact line documented (clearly
neither SubmitChecklist no SubmittingPatches have any occurrence of the
word impact), i.e. what are the valid values to chose from?

Jan

Re: [PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> >>> Ingo Molnar <mingo@elte.hu> 12.03.09 11:54 >>>
> >* Jan Beulich <jbeulich@novell.com> wrote:
> >> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> >> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >> 
> >> ---
> >>  arch/x86/include/asm/xen/hypercall.h |    2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> --- linux-2.6.29-rc7/arch/x86/include/asm/xen/hypercall.h	2009-03-11 17:52:10.000000000 +0100
> >> +++ 2.6.29-rc7-x86_64-xen-update-descr/arch/x86/include/asm/xen/hypercall.h	2009-02-13 11:41:39.000000000 +0100
> >> @@ -296,6 +296,8 @@ HYPERVISOR_get_debugreg(int reg)
> >>  static inline int
> >>  HYPERVISOR_update_descriptor(u64 ma, u64 desc)
> >>  {
> >> +	if (sizeof(u64) == sizeof(long))
> >> +		return _hypercall2(int, update_descriptor, ma, desc);
> >>  	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);
> >
> >missing changelog and Impact line.
> 
> I'm confused: What point is there to add a textual description 
> that matches the subject? [...]

For example, under what circumstances did you trigger the bug, 
how widely does it affect people, how did you test it. You are 
sending patches very close to the 2.6.29 release, and your 
commit log is non-existent.

Yes, i can figure out what the patch does, but that is not the 
point.

The point is for you to be forthcoming with such information and 
trying to be helpful to the maintenance process, by properly 
describing changes, by describing how you found the bug, how you 
tested the fix, how significant you find the fix, etc.

I.e. try to emit the information you have about this _already_, 
and generously so, instead of hiding it and forcing others to 
recover it. It might be a small work for me to recover it and 
put it into the changelog, but many of your past patches showed 
such a pattern and such overhead mounts up quickly.

> [...] And where is the need for an impact line documented 
> (clearly neither SubmitChecklist no SubmittingPatches have any 
> occurrence of the word impact), i.e. what are the valid values 
> to chose from?

See:

  http://lkml.org/lkml/2008/10/28/67

	Ingo

Re: [PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 12.03.09 12:35 >>>
>* Jan Beulich <jbeulich@novell.com> wrote:
>> I'm confused: What point is there to add a textual description 
>> that matches the subject? [...]
>
>For example, under what circumstances did you trigger the bug, 
>how widely does it affect people, how did you test it. You are 
>sending patches very close to the 2.6.29 release, and your 
>commit log is non-existent.
>
>Yes, i can figure out what the patch does, but that is not the 
>point.
>
>The point is for you to be forthcoming with such information and 
>trying to be helpful to the maintenance process, by properly 
>describing changes, by describing how you found the bug, how you 
>tested the fix, how significant you find the fix, etc.
>
>I.e. try to emit the information you have about this _already_, 
>and generously so, instead of hiding it and forcing others to 
>recover it.

Hmm, I'm really just following what I see from many others. And I have
to admit that there are [tiny] patches that really don't need much
explanation (and I often find quite the inverse - huge patches that have
[almost] no description).

>>It might be a small work for me to recover it and 
>>put it into the changelog, but many of your past patches showed 
>>such a pattern and such overhead mounts up quickly.

I'm sorry for that - I simply wasn't aware I'm causing you to do extra
work. I usually try to be as verbose with patches as seems necessary
to me - after all I have no other way to judge ho much is too little or
too much.

>> [...] And where is the need for an impact line documented 
>> (clearly neither SubmitChecklist no SubmittingPatches have any 
>> occurrence of the word impact), i.e. what are the valid values 
>> to chose from?
>
>See:
>
>  http://lkml.org/lkml/2008/10/28/67 

Thanks. Would certainly be helpful to put into Documentation/ if this is
meant to be more than just a personal requirement of yours.

Jan

Re: [PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[H. Peter Anvin]

Jan Beulich wrote:
>> See:
>>
>>  http://lkml.org/lkml/2008/10/28/67 
> 
> Thanks. Would certainly be helpful to put into Documentation/ if this is
> meant to be more than just a personal requirement of yours.

It's something we started with in the x86 tree (it came out of a long
discussion Ingo and I had about how to filter patches.)  It's not like
we can impose this workflow on others, but it has been *way* useful: a
patch with a *meaningful* "Impact:" line typically needs less than a
third of the human time than one without -- simply because it forces the
submitter to think about it.

Similarly "Fix foo" is not a complete description in any way.  Why was
foo broken in the first place?

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/include/asm/xen/hypercall.h |    2 ++
 1 file changed, 2 insertions(+)

--- linux-2.6.28-rc8/arch/x86/include/asm/xen/hypercall.h	2008-12-11 14:45:37.000000000 +0100
+++ 2.6.28-rc8-x86_64-xen-update-descr/arch/x86/include/asm/xen/hypercall.h	2008-12-12 10:25:55.000000000 +0100
@@ -290,6 +290,8 @@ HYPERVISOR_get_debugreg(int reg)
 static inline int
 HYPERVISOR_update_descriptor(u64 ma, u64 desc)
 {
+	if (sizeof(u64) == sizeof(long))
+		return _hypercall2(int, update_descriptor, ma, desc);
 	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);
 }
 

Re: [PATCH] x86-64: fix HYPERVISOR_update_descriptor()

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>   

Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

> ---
>  arch/x86/include/asm/xen/hypercall.h |    2 ++
>  1 file changed, 2 insertions(+)
>
> --- linux-2.6.28-rc8/arch/x86/include/asm/xen/hypercall.h	2008-12-11 14:45:37.000000000 +0100
> +++ 2.6.28-rc8-x86_64-xen-update-descr/arch/x86/include/asm/xen/hypercall.h	2008-12-12 10:25:55.000000000 +0100
> @@ -290,6 +290,8 @@ HYPERVISOR_get_debugreg(int reg)
>  static inline int
>  HYPERVISOR_update_descriptor(u64 ma, u64 desc)
>  {
> +	if (sizeof(u64) == sizeof(long))
> +		return _hypercall2(int, update_descriptor, ma, desc);
>  	return _hypercall4(int, update_descriptor, ma, ma>>32, desc, desc>>32);
>  }
>  
>
>
>
>