[Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Peter Maydell]

The memory-internal.h header claims that it is for "obsolete
exec.c functions" which "will be removed soon". This statement
was added in 2011, six years ago, but the header is still here.
(Admittedly none of the prototypes added in commit 67d95c153bef55f6
are still in the header.)

It's convenient to have a place to put prototypes for functions
which are used internally to the various .c files of the memory
system or by the accel/tcg code, which is inevitably fairly
closely coupled. So keep the header but update the comments to
reflect what we're actually using it for.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory-internal.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 98d8296..4162474 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -1,5 +1,5 @@
 /*
- * Declarations for obsolete exec.c functions
+ * Declarations for functions which are internal to the memory subsystem.
  *
  * Copyright 2011 Red Hat, Inc. and/or its affiliates
  *
@@ -12,8 +12,9 @@
  */
 
 /*
- * This header is for use by exec.c and memory.c ONLY.  Do not include it.
- * The functions declared here will be removed soon.
+ * This header is for use by exec.c, memory.c and accel/tcg/cputlb.c ONLY,
+ * for declarations which are shared between the memory subsystem's
+ * internals and the TCG TLB code. Do not include it from elsewhere.
  */
 
 #ifndef MEMORY_INTERNAL_H
-- 
2.7.4

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Paolo Bonzini]

On 21/11/2017 16:08, Peter Maydell wrote:
> The memory-internal.h header claims that it is for "obsolete
> exec.c functions" which "will be removed soon". This statement
> was added in 2011, six years ago, but the header is still here.
> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
> are still in the header.)
> 
> It's convenient to have a place to put prototypes for functions
> which are used internally to the various .c files of the memory
> system or by the accel/tcg code, which is inevitably fairly
> closely coupled. So keep the header but update the comments to
> reflect what we're actually using it for.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory-internal.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 98d8296..4162474 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -1,5 +1,5 @@
>  /*
> - * Declarations for obsolete exec.c functions
> + * Declarations for functions which are internal to the memory subsystem.
>   *
>   * Copyright 2011 Red Hat, Inc. and/or its affiliates
>   *
> @@ -12,8 +12,9 @@
>   */
>  
>  /*
> - * This header is for use by exec.c and memory.c ONLY.  Do not include it.
> - * The functions declared here will be removed soon.
> + * This header is for use by exec.c, memory.c and accel/tcg/cputlb.c ONLY,
> + * for declarations which are shared between the memory subsystem's
> + * internals and the TCG TLB code. Do not include it from elsewhere.
>   */
>  
>  #ifndef MEMORY_INTERNAL_H
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Peter Maydell]

On 21 November 2017 at 15:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 21/11/2017 16:08, Peter Maydell wrote:
>> The memory-internal.h header claims that it is for "obsolete
>> exec.c functions" which "will be removed soon". This statement
>> was added in 2011, six years ago, but the header is still here.
>> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
>> are still in the header.)
>>
>> It's convenient to have a place to put prototypes for functions
>> which are used internally to the various .c files of the memory
>> system or by the accel/tcg code, which is inevitably fairly
>> closely coupled. So keep the header but update the comments to
>> reflect what we're actually using it for.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Ping? Paolo, I was expecting this to go in via your tree --
should it go somewhere else instead?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/21/2017 12:08 PM, Peter Maydell wrote:
> The memory-internal.h header claims that it is for "obsolete
> exec.c functions" which "will be removed soon". This statement
> was added in 2011, six years ago, but the header is still here.
> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
> are still in the header.)
> 
> It's convenient to have a place to put prototypes for functions
> which are used internally to the various .c files of the memory
> system or by the accel/tcg code, which is inevitably fairly
> closely coupled. So keep the header but update the comments to
> reflect what we're actually using it for.

Until your NotDirtyInfo addition, the only prototype used was
memory_region_access_valid() (in s390-pci-inst.c).

Since "none of the prototypes added in commit 67d95c153bef55f6 are still
in the header" we could restrict it out of include/exec/ (kinda 'revert'
022c62cbbc) and only keep memory_region_access_valid() + NotDirtyInfo
exposed in include/exec/.

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory-internal.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 98d8296..4162474 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -1,5 +1,5 @@
>  /*
> - * Declarations for obsolete exec.c functions
> + * Declarations for functions which are internal to the memory subsystem.
>   *
>   * Copyright 2011 Red Hat, Inc. and/or its affiliates
>   *
> @@ -12,8 +12,9 @@
>   */
>  
>  /*
> - * This header is for use by exec.c and memory.c ONLY.  Do not include it.
> - * The functions declared here will be removed soon.
> + * This header is for use by exec.c, memory.c and accel/tcg/cputlb.c ONLY,
> + * for declarations which are shared between the memory subsystem's
> + * internals and the TCG TLB code. Do not include it from elsewhere.
>   */
>  
>  #ifndef MEMORY_INTERNAL_H
> 

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/21/2017 12:08 PM, Peter Maydell wrote:
> The memory-internal.h header claims that it is for "obsolete
> exec.c functions" which "will be removed soon". This statement
> was added in 2011, six years ago, but the header is still here.
> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
> are still in the header.)
> 
> It's convenient to have a place to put prototypes for functions
> which are used internally to the various .c files of the memory
> system or by the accel/tcg code, which is inevitably fairly
> closely coupled. So keep the header but update the comments to
> reflect what we're actually using it for.

Until your NotDirtyInfo addition, the only prototype used was
memory_region_access_valid() (in s390-pci-inst.c).

Since "none of the prototypes added in commit 67d95c153bef55f6 are still
in the header" we could restrict it out of include/exec/ (kinda 'revert'
022c62cbbc) and only keep memory_region_access_valid() + NotDirtyInfo
exposed in include/exec/.

(During 2.12)

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory-internal.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 98d8296..4162474 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -1,5 +1,5 @@
>  /*
> - * Declarations for obsolete exec.c functions
> + * Declarations for functions which are internal to the memory subsystem.
>   *
>   * Copyright 2011 Red Hat, Inc. and/or its affiliates
>   *
> @@ -12,8 +12,9 @@
>   */
>  
>  /*
> - * This header is for use by exec.c and memory.c ONLY.  Do not include it.
> - * The functions declared here will be removed soon.
> + * This header is for use by exec.c, memory.c and accel/tcg/cputlb.c ONLY,
> + * for declarations which are shared between the memory subsystem's
> + * internals and the TCG TLB code. Do not include it from elsewhere.
>   */
>  
>  #ifndef MEMORY_INTERNAL_H
> 

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Peter Maydell]

On 21 November 2017 at 15:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 11/21/2017 12:08 PM, Peter Maydell wrote:
>> The memory-internal.h header claims that it is for "obsolete
>> exec.c functions" which "will be removed soon". This statement
>> was added in 2011, six years ago, but the header is still here.
>> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
>> are still in the header.)
>>
>> It's convenient to have a place to put prototypes for functions
>> which are used internally to the various .c files of the memory
>> system or by the accel/tcg code, which is inevitably fairly
>> closely coupled. So keep the header but update the comments to
>> reflect what we're actually using it for.
>
> Until your NotDirtyInfo addition, the only prototype used was
> memory_region_access_valid() (in s390-pci-inst.c).
>
> Since "none of the prototypes added in commit 67d95c153bef55f6 are still
> in the header" we could restrict it out of include/exec/ (kinda 'revert'
> 022c62cbbc) and only keep memory_region_access_valid() + NotDirtyInfo
> exposed in include/exec/.

I'm not sure what you're suggesting. I definitely think the
s390 usage is pretty nasty but I guess it would need some
rework to get rid of. For everything else, it's nice
to have somewhere to share these things. You could argue
for splitting the header into two, one for 'between memory.c
and exec.c' and one for 'between memory.c and cputlb.c',
but is it worth the effort?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Philippe Mathieu-Daudé]

On 11/21/2017 01:00 PM, Peter Maydell wrote:
> On 21 November 2017 at 15:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Peter,
>>
>> On 11/21/2017 12:08 PM, Peter Maydell wrote:
>>> The memory-internal.h header claims that it is for "obsolete
>>> exec.c functions" which "will be removed soon". This statement
>>> was added in 2011, six years ago, but the header is still here.
>>> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
>>> are still in the header.)
>>>
>>> It's convenient to have a place to put prototypes for functions
>>> which are used internally to the various .c files of the memory
>>> system or by the accel/tcg code, which is inevitably fairly
>>> closely coupled. So keep the header but update the comments to
>>> reflect what we're actually using it for.
>>
>> Until your NotDirtyInfo addition, the only prototype used was
>> memory_region_access_valid() (in s390-pci-inst.c).
>>
>> Since "none of the prototypes added in commit 67d95c153bef55f6 are still
>> in the header" we could restrict it out of include/exec/ (kinda 'revert'
>> 022c62cbbc) and only keep memory_region_access_valid() + NotDirtyInfo
>> exposed in include/exec/.
> 
> I'm not sure what you're suggesting. I definitely think the
> s390 usage is pretty nasty but I guess it would need some
> rework to get rid of. For everything else, it's nice
> to have somewhere to share these things. You could argue
> for splitting the header into two, one for 'between memory.c
> and exec.c' and one for 'between memory.c and cputlb.c',
> but is it worth the effort?

Why not move memory_region_access_valid() + NotDirtyInfo in
"exec/exec-all.h" and only use this to "share these things"?

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Peter Maydell]

On 21 November 2017 at 16:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 11/21/2017 01:00 PM, Peter Maydell wrote:
>> I'm not sure what you're suggesting. I definitely think the
>> s390 usage is pretty nasty but I guess it would need some
>> rework to get rid of. For everything else, it's nice
>> to have somewhere to share these things. You could argue
>> for splitting the header into two, one for 'between memory.c
>> and exec.c' and one for 'between memory.c and cputlb.c',
>> but is it worth the effort?
>
> Why not move memory_region_access_valid() + NotDirtyInfo in
> "exec/exec-all.h" and only use this to "share these things"?

Because exec-all.h is included by an absolute ton of .c files.
The point is to have a header for these basically-internal
functions that's only included by the small set of .c files
that should have access to them, so that other code doesn't
start using them by accident.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Paolo Bonzini]

On 21/11/2017 17:04, Philippe Mathieu-Daudé wrote:
>> I'm not sure what you're suggesting. I definitely think the
>> s390 usage is pretty nasty but I guess it would need some
>> rework to get rid of. For everything else, it's nice
>> to have somewhere to share these things. You could argue
>> for splitting the header into two, one for 'between memory.c
>> and exec.c' and one for 'between memory.c and cputlb.c',
>> but is it worth the effort?
> Why not move memory_region_access_valid() + NotDirtyInfo in
> "exec/exec-all.h" and only use this to "share these things"?

exec/exec-all.h is for the TCG accelerator's runtime.  NotDirtyInfo and
notdirty_write_{prepare,complete} _might_ belong in there (they sit in
the middle between exec/exec-all.h and memory-internal.h), but
memory_region_access_valid certainly doesn't because it's used with KVM
as well.

In fact, memory_region_access_valid might as well be in memory.h.

Paolo

Re: [Qemu-devel] [PATCH] memory-internal.h: Remove obsolete claim that header is obsolete

[Paolo Bonzini]

On 21/11/2017 16:08, Peter Maydell wrote:
> The memory-internal.h header claims that it is for "obsolete
> exec.c functions" which "will be removed soon". This statement
> was added in 2011, six years ago, but the header is still here.
> (Admittedly none of the prototypes added in commit 67d95c153bef55f6
> are still in the header.)
> 
> It's convenient to have a place to put prototypes for functions
> which are used internally to the various .c files of the memory
> system or by the accel/tcg code, which is inevitably fairly
> closely coupled. So keep the header but update the comments to
> reflect what we're actually using it for.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory-internal.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 98d8296..4162474 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -1,5 +1,5 @@
>  /*
> - * Declarations for obsolete exec.c functions
> + * Declarations for functions which are internal to the memory subsystem.
>   *
>   * Copyright 2011 Red Hat, Inc. and/or its affiliates
>   *
> @@ -12,8 +12,9 @@
>   */
>  
>  /*
> - * This header is for use by exec.c and memory.c ONLY.  Do not include it.
> - * The functions declared here will be removed soon.
> + * This header is for use by exec.c, memory.c and accel/tcg/cputlb.c ONLY,
> + * for declarations which are shared between the memory subsystem's
> + * internals and the TCG TLB code. Do not include it from elsewhere.
>   */
>  
>  #ifndef MEMORY_INTERNAL_H
> 

Queued, thanks.

Paolo