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

[Andrey Smirnov]

On Mon, Jul 11, 2016 at 10:27 PM, David Gibson
<address@hidden> wrote:
> On Mon, Jul 11, 2016 at 05:27:50PM +0100, Peter Maydell wrote:
>> On 11 July 2016 at 03:24, David Gibson <address@hidden> wrote:
>> > On Sun, Jul 10, 2016 at 08:32:32PM +0100, Peter Maydell wrote:
>> >> On 8 July 2016 at 04:42, David Gibson <address@hidden> 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.

Thanks,
Andrey