Re: [Qemu-devel] [PATCH 6/9] Convert cpu_memory_rw_debug to use MMUAccessType

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

On Tue, Jul 12, 2016 at 09:05:24AM -0700, Andrey Smirnov wrote:
> On Mon, Jul 11, 2016 at 10:27 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Mon, Jul 11, 2016 at 05:27:50PM +0100, Peter Maydell wrote:
> >> On 11 July 2016 at 03:24, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
> >> >> On 8 July 2016 at 04:42, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> >> > My only concern here is that the constants are named
> >> >> > *MMU*_DATA_... whereas these are physical memory accesses not
> >> >> > involving the MMU.  I can't actually see any current users of
> >> >> > MMUAccessType which makes me a bit confused as to what it's intended
> >> >> > meaning was
> >> >>
> >> >> If you grep for MMU_DATA_LOAD/MMU_DATA_STORE/MMU_INST_FETCH
> >> >> you'll see the uses. A lot of the softmmu code uses the
> >> >> convention of 0=read,1=write,2=insn (which developed I
> >> >> think historically from a bool "is_write", which you'll
> >> >> still see in some function argument names, that was
> >> >> augmented to handle insn-fetch separately). The enum
> >> >> gives us some symbolic names for the constant values.
> >> >> (There's a proposed patch somewhere to change the
> >> >> 'int is_write' arguments to actually use the enum type.)
> >> >
> >> > Ah, yes, I see.  Still surprisingly few, actually.
> >>
> >> Yeah, we didn't go through (yet) and update the legacy code,
> >> just provided the common type so new code could use it.
> >
> > Ah, yes, I see.
> >
> >> > My concern about the potentially misleading name still stands.
> >>
> >> I don't mind if we want to rename it, but I don't think we want
> >> to have two types. This is all in the softmmu code, whether it's
> >> in the physical-address parts or the virtual-address parts.
> >
> > Right, I agree we shouldn't have two types.
> >
> > I think we should rename the existing constants, though, since it
> > doesn't really have anything to do with the MMU.
> >
> 
> I agree with your concern about confusing naming, David, and think
> those should be renamed as well. In the original version of this patch
> I introduced my own enumeration which was duplication MMUAccessType,
> so in his feedback Peter asked me to use MMUAccessType instead.
> 
> Please let me know how I should proceed with this series.

My preference would be to start the series with something which
renames MMUAccessType and its individual values to something more
generic, including changing the existing users.

But I can't speak to what Peter would prefer.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]