| Message ID | 1511276888-17834-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | memory-internal.h: Remove obsolete claim that header is obsolete | expand |
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
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 >
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 >
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
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"?
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
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
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
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
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
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(-)