[Qemu-devel] RFC: memory API changes

[Qemu-devel] RFC: memory API changes

[Peter Maydell]

(This is part of the work I'm doing for transaction attributes.)

Currently we have several APIs used for making physical
memory accesses:

1. cpu_physical_memory_rw &c

2. address_space_rw/read/write/map

3. ld/st*_phys

These do more-or-less overlapping jobs and it's not
obvious which should be used when. Also they need to be
expanded to support transaction attributes and (in some
cases) reporting of failed memory transactions. I propose:

 * ld/st*_phys to be renamed to as_ld*, eg
    ldub_phys -> as_ldub
    ldl_be_phys -> as_ldl_be
    stq_phys -> as_stq
    stl_le_phys -> as_ldl_le
   and to take two new arguments:
    MemTxAttrs attrs, MemTxResult *result
   (existing callsites can pass MEMTXATTRS_UNSPECIFIED, NULL
   to get their current behaviour.)
 * address_space_rw &c to be renamed:
    address_space_rw -> as_rw_buf
    address_space_read -> as_read_buf
    address_space_write -> as_write_buf
    address_space_map -> as_map_buf &c
   This is just so the names line up nicely and we have a
   clear indication that this is a family of functions
 * address_space_read/write/rw should return MemTxResult rather
   than plain bool
 * we should put all the as_* function prototypes in one
   header, probably memory.h, rather than some in cpu-common.h
   and some in memory.h
 * cpu_physical_memory_rw are obsolete and should be replaced
   with uses of the as_* functions -- we should at least note
   this in the header file. (Can't do this as an automated change
   really because the correct AS to use is callsite dependent.)

I'm hoping I can use coccinelle to do the heavy lifting of
the renames. Before I make a start on that I wanted to see if
people agreed with this or had better naming suggestions.
as_* is a bit abbreviated but I felt that calling these
all address_space_lduw_le() and so on was a little unwieldy
(especially if compared to the lduw_le_phys names we have now).
Certainly we could do that if people prefer it, though.

The point of indicating failure via MemTxResult is that at
some point we need to correct the current broken handling of
the CPUClass do_unassigned_access hook, because that should
only be invoked if the CPU itself does an access to an unassigned
address, not if some random DMA'ing device does!

Comments appreciated; will try to produce actual patches this
week...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Andreas Färber]

Am 23.03.2015 um 13:24 schrieb Peter Maydell:
>  * address_space_rw &c to be renamed:
>     address_space_rw -> as_rw_buf
>     address_space_read -> as_read_buf
>     address_space_write -> as_write_buf
>     address_space_map -> as_map_buf &c
>    This is just so the names line up nicely and we have a
>    clear indication that this is a family of functions

How does address_space_* not indicate they are a family? address_space_
is much more intuitive to read than shortening it to as_, which can
confusingly be read as another English word.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 12:30, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.03.2015 um 13:24 schrieb Peter Maydell:
>>  * address_space_rw &c to be renamed:
>>     address_space_rw -> as_rw_buf
>>     address_space_read -> as_read_buf
>>     address_space_write -> as_write_buf
>>     address_space_map -> as_map_buf &c
>>    This is just so the names line up nicely and we have a
>>    clear indication that this is a family of functions
>
> How does address_space_* not indicate they are a family? address_space_
> is much more intuitive to read than shortening it to as_, which can
> confusingly be read as another English word.

Sorry, that was a little cryptic in retrospect. I meant "they should
have the same prefix as the renamed ld*_phys functions, to indicate
that all of these functions are in the same family rather than being
two sets of unrelated functions". As I note later on, if we want the
prefix to be address_space_ rather than as_ for all these
we could do that, though it's a little long and unwieldy.

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 23/03/2015 13:24, Peter Maydell wrote:
> (This is part of the work I'm doing for transaction attributes.)
> 
> Currently we have several APIs used for making physical
> memory accesses:
> 
> 1. cpu_physical_memory_rw &c
> 
> 2. address_space_rw/read/write/map
> 
> 3. ld/st*_phys
> 
> These do more-or-less overlapping jobs and it's not
> obvious which should be used when. Also they need to be
> expanded to support transaction attributes and (in some
> cases) reporting of failed memory transactions. I propose:
> 
>  * ld/st*_phys to be renamed to as_ld*, eg
>     ldub_phys -> as_ldub
>     ldl_be_phys -> as_ldl_be
>     stq_phys -> as_stq
>     stl_le_phys -> as_ldl_le

I think shorthand functions with no extra arguments still have a place.
 I was thinking of having them only temporarily, until we add functions
(e.g. pci_dma_ld or amba_ld) that deal with the MemTxResult by setting
some bus-specific abort bit.  However, this API would complicate the
case when the same core code is used for both PCI and sysbus devices.
Perhaps AddressSpaces can grow a callback that transforms a "bad"
MemTxResult to a "good" one with some side effects?

I do not like the as_ prefix, mostly because it is an English word.  It
doesn't help that ", MEMTXATTRS_UNSPECIFIED, NULL", tacked on each
ld*_phys function invocation, is way more verbose than any savings you
get from shortening the name. :)

I could be persuaded about addrspace_ as the prefix.  Hence, adding new
functions addrspace_ld* and addrspace_st* with the two extra arguments
would be fine.  Still, rather than saving four characters in the prefix,
I'd rather move the maximum line length up from 80 to 90 characters, and
actually change that to a checkpatch error.

>    and to take two new arguments:
>     MemTxAttrs attrs, MemTxResult *result
>    (existing callsites can pass MEMTXATTRS_UNSPECIFIED, NULL
>    to get their current behaviour.)
>  * address_space_rw &c to be renamed:
>     address_space_rw -> as_rw_buf
>     address_space_read -> as_read_buf
>     address_space_write -> as_write_buf
>     address_space_map -> as_map_buf &c

address_space_map doesn't map into or out of a buffer, so the name is
fine as it is.

>    This is just so the names line up nicely and we have a
>    clear indication that this is a family of functions
>  * address_space_read/write/rw should return MemTxResult rather
>    than plain bool

Certainly okay.  Same for address_space_access_valid.

>  * we should put all the as_* function prototypes in one
>    header, probably memory.h, rather than some in cpu-common.h
>    and some in memory.h

I think separating "creator" and "user" functions in two headers could
be nice.  If we cannot come up with a name for the two headers (memory.h
and mem_ldst.h?), putting both in the same is okay too.

>  * cpu_physical_memory_rw are obsolete and should be replaced
>    with uses of the as_* functions -- we should at least note
>    this in the header file. (Can't do this as an automated change
>    really because the correct AS to use is callsite dependent.)

All users that should _not_ be using address_space_memory have been
already changed to address_space_rw, or should have, so it can be done
automatically.  Same for cpu_physical_memory_map/unmap, BTW.

> The point of indicating failure via MemTxResult is that at
> some point we need to correct the current broken handling of
> the CPUClass do_unassigned_access hook, because that should
> only be invoked if the CPU itself does an access to an unassigned
> address, not if some random DMA'ing device does!

100% agreement on this.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 14:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 23/03/2015 13:24, Peter Maydell wrote:
>> (This is part of the work I'm doing for transaction attributes.)
>>
>> Currently we have several APIs used for making physical
>> memory accesses:
>>
>> 1. cpu_physical_memory_rw &c
>>
>> 2. address_space_rw/read/write/map
>>
>> 3. ld/st*_phys
>>
>> These do more-or-less overlapping jobs and it's not
>> obvious which should be used when. Also they need to be
>> expanded to support transaction attributes and (in some
>> cases) reporting of failed memory transactions. I propose:
>>
>>  * ld/st*_phys to be renamed to as_ld*, eg
>>     ldub_phys -> as_ldub
>>     ldl_be_phys -> as_ldl_be
>>     stq_phys -> as_stq
>>     stl_le_phys -> as_ldl_le
>
> I think shorthand functions with no extra arguments still have a place.

The trouble is that since C doesn't do polymorphism you
then end up with awkward names for one or the other...

>  I was thinking of having them only temporarily, until we add functions
> (e.g. pci_dma_ld or amba_ld) that deal with the MemTxResult by setting
> some bus-specific abort bit.  However, this API would complicate the
> case when the same core code is used for both PCI and sysbus devices.
> Perhaps AddressSpaces can grow a callback that transforms a "bad"
> MemTxResult to a "good" one with some side effects?

So, for PCI you can have something which sets an abort bit
automatically, because the PCI spec mandates that kind of
register level exposure of transaction failures. But for AMBA
(and I guess many other buses), there's no such standardization.
The bus standard says "your transaction might fail", but what
the device actually does in that situation is up to the device
(which might ignore it, go into some lockup mode til the guest
resets it, make a note in a device-specific status register...)

For PCI, I thought the approach here was going to be that the default
background AddressSpace handlers set the abort bit and then returned
the "-1" or whatever result the spec says? In that case the
ldl functions would never return a failure result.

> I do not like the as_ prefix, mostly because it is an English word.  It
> doesn't help that ", MEMTXATTRS_UNSPECIFIED, NULL", tacked on each
> ld*_phys function invocation, is way more verbose than any savings you
> get from shortening the name. :)
>
> I could be persuaded about addrspace_ as the prefix.  Hence, adding new
> functions addrspace_ld* and addrspace_st* with the two extra arguments
> would be fine.  Still, rather than saving four characters in the prefix,
> I'd rather move the maximum line length up from 80 to 90 characters, and
> actually change that to a checkpatch error.

OK, that's two voices against as_*. I'll leave it at
address_space_*...

>>    and to take two new arguments:
>>     MemTxAttrs attrs, MemTxResult *result
>>    (existing callsites can pass MEMTXATTRS_UNSPECIFIED, NULL
>>    to get their current behaviour.)
>>  * address_space_rw &c to be renamed:
>>     address_space_rw -> as_rw_buf
>>     address_space_read -> as_read_buf
>>     address_space_write -> as_write_buf
>>     address_space_map -> as_map_buf &c
>
> address_space_map doesn't map into or out of a buffer, so the name is
> fine as it is.

Agreed.

>>    This is just so the names line up nicely and we have a
>>    clear indication that this is a family of functions
>>  * address_space_read/write/rw should return MemTxResult rather
>>    than plain bool
>
> Certainly okay.  Same for address_space_access_valid.
>
>>  * we should put all the as_* function prototypes in one
>>    header, probably memory.h, rather than some in cpu-common.h
>>    and some in memory.h
>
> I think separating "creator" and "user" functions in two headers could
> be nice.  If we cannot come up with a name for the two headers (memory.h
> and mem_ldst.h?), putting both in the same is okay too.

I'm not sure there is a clean split here; I'd rather
just put them all in memory.h. We can split things up as
a separate thing (and keep the scope of this change smaller).

>>  * cpu_physical_memory_rw are obsolete and should be replaced
>>    with uses of the as_* functions -- we should at least note
>>    this in the header file. (Can't do this as an automated change
>>    really because the correct AS to use is callsite dependent.)
>
> All users that should _not_ be using address_space_memory have been
> already changed to address_space_rw, or should have, so it can be done
> automatically.  Same for cpu_physical_memory_map/unmap, BTW.

Hmm. Checking a few, I notice that for instance the kvm-all.c
cpu_physical_memory_rw() should probably be using cpu->as.
And the uses in the bitband read/write accessors in hw/arm/armv7m.c
should also be using a CPU address space. Most uses in devices
should really be taking a pointer to the address space to use
as a device property...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 23/03/2015 16:11, Peter Maydell wrote:
> On 23 March 2015 at 14:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 23/03/2015 13:24, Peter Maydell wrote:
>>> (This is part of the work I'm doing for transaction attributes.)
>>>
>>> Currently we have several APIs used for making physical
>>> memory accesses:
>>>
>>> 1. cpu_physical_memory_rw &c
>>>
>>> 2. address_space_rw/read/write/map
>>>
>>> 3. ld/st*_phys
>>>
>>> These do more-or-less overlapping jobs and it's not
>>> obvious which should be used when. Also they need to be
>>> expanded to support transaction attributes and (in some
>>> cases) reporting of failed memory transactions. I propose:
>>>
>>>  * ld/st*_phys to be renamed to as_ld*, eg
>>>     ldub_phys -> as_ldub
>>>     ldl_be_phys -> as_ldl_be
>>>     stq_phys -> as_stq
>>>     stl_le_phys -> as_ldl_le
>>
>> I think shorthand functions with no extra arguments still have a place.
> 
> The trouble is that since C doesn't do polymorphism you
> then end up with awkward names for one or the other...

True.  But since it's not a new API we can keep the old name for the
simple one.

>>  I was thinking of having them only temporarily, until we add functions
>> (e.g. pci_dma_ld or amba_ld) that deal with the MemTxResult by setting
>> some bus-specific abort bit.  However, this API would complicate the
>> case when the same core code is used for both PCI and sysbus devices.
>> Perhaps AddressSpaces can grow a callback that transforms a "bad"
>> MemTxResult to a "good" one with some side effects?
> 
> So, for PCI you can have something which sets an abort bit
> automatically, because the PCI spec mandates that kind of
> register level exposure of transaction failures. But for AMBA
> (and I guess many other buses), there's no such standardization.
> The bus standard says "your transaction might fail", but what
> the device actually does in that situation is up to the device
> (which might ignore it, go into some lockup mode til the guest
> resets it, make a note in a device-specific status register...)

Still, you have the problem of sharing code between devices that might
have different failure modes. :(  I don't really have a solution.

> For PCI, I thought the approach here was going to be that the default
> background AddressSpace handlers set the abort bit and then returned
> the "-1" or whatever result the spec says? In that case the
> ldl functions would never return a failure result.

Yes.  I'm not sure why it didn't work out however.

>>>  * cpu_physical_memory_rw are obsolete and should be replaced
>>>    with uses of the as_* functions -- we should at least note
>>>    this in the header file. (Can't do this as an automated change
>>>    really because the correct AS to use is callsite dependent.)
>>
>> All users that should _not_ be using address_space_memory have been
>> already changed to address_space_rw, or should have, so it can be done
>> automatically.  Same for cpu_physical_memory_map/unmap, BTW.
> 
> Hmm. Checking a few, I notice that for instance the kvm-all.c
> cpu_physical_memory_rw() should probably be using cpu->as.

Yes, that's something that I'll have to change soon as I implement
system management mode support in x86 KVM...

> And the uses in the bitband read/write accessors in hw/arm/armv7m.c
> should also be using a CPU address space. Most uses in devices
> should really be taking a pointer to the address space to use
> as a device property...

Yes, that's what was done for PCI devices and thus their sysbus variants
too (when they exist).  But for most MMIO devices address_space_memory
is probably good enough, and changing it wholesale is not going to make
things worse than they are.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 15:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 23/03/2015 16:11, Peter Maydell wrote:
>> On 23 March 2015 at 14:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 23/03/2015 13:24, Peter Maydell wrote:
>>>>  * ld/st*_phys to be renamed to as_ld*, eg
>>>>     ldub_phys -> as_ldub
>>>>     ldl_be_phys -> as_ldl_be
>>>>     stq_phys -> as_stq
>>>>     stl_le_phys -> as_ldl_le
>>>
>>> I think shorthand functions with no extra arguments still have a place.
>>
>> The trouble is that since C doesn't do polymorphism you
>> then end up with awkward names for one or the other...
>
> True.  But since it's not a new API we can keep the old name for the
> simple one.

...except that means that the function you should in
general not be using as default is the one with the short
name, which is the wrong way round. We should be guiding
people writing new code to think about the required
behaviour on memory transaction failure (and attributes)
as they write the code, not making it easy for them to
ignore the issue...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 23/03/2015 16:26, Peter Maydell wrote:
> > True.  But since it's not a new API we can keep the old name for the
> > simple one.
>
> ...except that means that the function you should in
> general not be using as default is the one with the short
> name, which is the wrong way round. We should be guiding
> people writing new code to think about the required
> behaviour on memory transaction failure (and attributes)
> as they write the code, not making it easy for them to
> ignore the issue...

Not necessarily, for example PCI devices can certainly use the short name.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 15:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 23/03/2015 16:26, Peter Maydell wrote:
>> > True.  But since it's not a new API we can keep the old name for the
>> > simple one.
>>
>> ...except that means that the function you should in
>> general not be using as default is the one with the short
>> name, which is the wrong way round. We should be guiding
>> people writing new code to think about the required
>> behaviour on memory transaction failure (and attributes)
>> as they write the code, not making it easy for them to
>> ignore the issue...
>
> Not necessarily, for example PCI devices can certainly
> use the short name.

PCI devices should all be using ld*_pci_dma and st*_pci_dma
so they go through the correct address space for the PCIDevice.
Any PCI device making direct calls to ld*_phys/st*_phys
is broken...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 23/03/2015 16:39, Peter Maydell wrote:
> > Not necessarily, for example PCI devices can certainly
> > use the short name.
>
> PCI devices should all be using ld*_pci_dma and st*_pci_dma
> so they go through the correct address space for the PCIDevice.
> Any PCI device making direct calls to ld*_phys/st*_phys
> is broken...

Ok, so most ld*_phys users are already using cs->as.  The remaining ones
are:

hw/alpha/typhoon.c
hw/display/sm501_template.h
hw/dma/pl080.c
hw/dma/sun4m_iommu.c
hw/net/vmware_utils.h
hw/pci-host/apb.c
hw/s390x/css.c
hw/s390x/s390-pci-bus.c
hw/s390x/s390-virtio-bus.c
hw/s390x/virtio-ccw.c
hw/scsi/megasas.c
hw/scsi/vmw_pvscsi.c

So let's keep ld*_phys in cpu-common.h, fix the uses above, and then
move it to cpu-all.h (cpu-common.h probably should die).

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 15:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Ok, so most ld*_phys users are already using cs->as.

But this is only because when the AddressSpace* argument
to ld*_phys was introduced we made it be cs->as for
existing callers, to retain the existing behaviour.
That doesn't mean that callers using cs->as are doing
the right thing. In general I would expect that any
caller that's not part of the CPU itself should not be
rummaging around inside the CPU struct to find an
AddressSpace to use...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 23/03/2015 17:00, Peter Maydell wrote:
> On 23 March 2015 at 15:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Ok, so most ld*_phys users are already using cs->as.
> 
> But this is only because when the AddressSpace* argument
> to ld*_phys was introduced we made it be cs->as for
> existing callers, to retain the existing behaviour.
> That doesn't mean that callers using cs->as are doing
> the right thing. In general I would expect that any
> caller that's not part of the CPU itself should not be
> rummaging around inside the CPU struct to find an
> AddressSpace to use...

None of them does, uses of ld*_phys(cs->as, ...) are almost all in
target-* already.

There's just hw/ppc/spapr_hcall.c and hw/ppc/spapr_iommu.c that are
using cs->as in hw/, but they are correct because these are hypercalls.

The others use &address_space_memory.  I sent a patch to fix megasas and
vmware.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Andreas Färber]

Am 23.03.2015 um 16:11 schrieb Peter Maydell:
> On 23 March 2015 at 14:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 23/03/2015 13:24, Peter Maydell wrote:
>>>  * cpu_physical_memory_rw are obsolete and should be replaced
>>>    with uses of the as_* functions -- we should at least note
>>>    this in the header file. (Can't do this as an automated change
>>>    really because the correct AS to use is callsite dependent.)
>>
>> All users that should _not_ be using address_space_memory have been
>> already changed to address_space_rw, or should have, so it can be done
>> automatically.  Same for cpu_physical_memory_map/unmap, BTW.
> 
> Hmm. Checking a few, I notice that for instance the kvm-all.c
> cpu_physical_memory_rw() should probably be using cpu->as.
[snip]

Igor, does that impact our APIC discussion?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 16:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
> None of them does, uses of ld*_phys(cs->as, ...) are almost all in
> target-* already.

Oh good :-)

Regardless, I don't see why this means that ld*_phys should be
in cpu-common.h or cpu-all.h rather than memory.h ?

> There's just hw/ppc/spapr_hcall.c and hw/ppc/spapr_iommu.c that are
> using cs->as in hw/, but they are correct because these are hypercalls.

Also hw/ppc/ppc405_uc.c (using stl_be_phys).

> The others use &address_space_memory.  I sent a patch to fix megasas and
> vmware.

Thanks.

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Andreas Färber]

Am 23.03.2015 um 15:39 schrieb Paolo Bonzini:
> On 23/03/2015 13:24, Peter Maydell wrote:
>> The point of indicating failure via MemTxResult is that at
>> some point we need to correct the current broken handling of
>> the CPUClass do_unassigned_access hook, because that should
>> only be invoked if the CPU itself does an access to an unassigned
>> address, not if some random DMA'ing device does!
> 
> 100% agreement on this.

Just to clarify, CPUClass::do_unassigned_access() was a simple
refactoring towards multi-target builds (~two inline functions remain).

If you rather need it on MemoryRegion level or somewhere else, feel free
to post patches, just please keep the original goal in mind.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 17:51, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.03.2015 um 15:39 schrieb Paolo Bonzini:
>> On 23/03/2015 13:24, Peter Maydell wrote:
>>> The point of indicating failure via MemTxResult is that at
>>> some point we need to correct the current broken handling of
>>> the CPUClass do_unassigned_access hook, because that should
>>> only be invoked if the CPU itself does an access to an unassigned
>>> address, not if some random DMA'ing device does!
>>
>> 100% agreement on this.
>
> Just to clarify, CPUClass::do_unassigned_access() was a simple
> refactoring towards multi-target builds (~two inline functions remain).
>
> If you rather need it on MemoryRegion level or somewhere else, feel free
> to post patches, just please keep the original goal in mind.

Yes, the change here would just be to hoist the callsite
for the hook up to somewhere else, probably io_read*/io_write*
but at any rate somewhere which is in the "definitely only
for CPU generated loads and stores" code path.

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 23 March 2015 at 12:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> (This is part of the work I'm doing for transaction attributes.)

OK, here's try 2, based on feedback on the first proposal:

 * address_space_rw &c remain with their current names, but
   take an extra MemTxAttrs argument and return MemTxResult
   rather than bool. (The latter conveniently doesn't require changes to
   callsites because conversion to bool gives the same true-on-error
   semantics as before.)
   [maybe readbuf/writebuf would be clearer than read/write,
   but it didn't seem sufficiently obvious a win to make the change]
 * the ld/st_*phys functions are renamed as:
     ldl_be_phys -> address_space_ldl_be &c
   and all take MemTxAttrs, *MemTxResult
 * rather than MEMTXATTRS_UNSPECIFIED, use TXATTRS_NONE, so the
   extra arguments (TXATTRS_NONE, NULL) aren't too unwieldy
 * prototypes in memory.h
 * no default-to-no-attrs/etc versions of ld/st*_ phys
   (if in specific devices/buses it's the best thing we should
   have bus-specific dma accessors, as we do for pci)
 * mechanically convert all uses of cpu_physical_memory_* to
   address_space_*(&address_space_memory, ...)

[This leaves the "where do we call the unassigned-access
hooks" problem for a different patchset.]

Is there anything in there people strongly dislike, or
should I start writing coccinelle patches for this?

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 24/03/2015 14:47, Peter Maydell wrote:
> On 23 March 2015 at 12:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>> (This is part of the work I'm doing for transaction attributes.)
> 
> OK, here's try 2, based on feedback on the first proposal:
> 
>  * address_space_rw &c remain with their current names, but
>    take an extra MemTxAttrs argument and return MemTxResult
>    rather than bool. (The latter conveniently doesn't require changes to
>    callsites because conversion to bool gives the same true-on-error
>    semantics as before.)
>    [maybe readbuf/writebuf would be clearer than read/write,
>    but it didn't seem sufficiently obvious a win to make the change]
>  * the ld/st_*phys functions are renamed as:
>      ldl_be_phys -> address_space_ldl_be &c
>    and all take MemTxAttrs, *MemTxResult
>  * rather than MEMTXATTRS_UNSPECIFIED, use TXATTRS_NONE, so the
>    extra arguments (TXATTRS_NONE, NULL) aren't too unwieldy
>  * prototypes in memory.h
>  * no default-to-no-attrs/etc versions of ld/st*_ phys
>    (if in specific devices/buses it's the best thing we should
>    have bus-specific dma accessors, as we do for pci)

I would keep them since they're really heavily used with cs->as as the
first argument.  But definitely move them to a different header than
cpu-common.h, and perhaps make them takes a CPUState instead of an
AddressSpace (which might help solving "where do we call the
unassigned-access hooks" in the future).

In any case, the removal or segregation of ld/st*_phys should be a
separate series for ease of review.

Apart from this, I'm on board.

Paolo

>  * mechanically convert all uses of cpu_physical_memory_* to
>    address_space_*(&address_space_memory, ...)
> 
> [This leaves the "where do we call the unassigned-access
> hooks" problem for a different patchset.]
> 
> Is there anything in there people strongly dislike, or
> should I start writing coccinelle patches for this?
> 
> -- PMM
> 

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 24 March 2015 at 14:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 24/03/2015 14:47, Peter Maydell wrote:
>> On 23 March 2015 at 12:24, Peter Maydell <peter.maydell@linaro.org> wrote:
>>  * no default-to-no-attrs/etc versions of ld/st*_ phys
>>    (if in specific devices/buses it's the best thing we should
>>    have bus-specific dma accessors, as we do for pci)
>
> I would keep them since they're really heavily used with cs->as as the
> first argument.  But definitely move them to a different header than
> cpu-common.h, and perhaps make them takes a CPUState instead of an
> AddressSpace (which might help solving "where do we call the
> unassigned-access hooks" in the future).

No, I think we want to be able to do simple "load a word
from an arbitrary AddressSpace" functions. And these should
definitely include the parameters for attributes and result
information, because otherwise it's way too easy to just
forget completely about those when you're writing a device.
What I don't think there's much call for is "load/store a
word, but completely ignore the possibility that might fail".

At some point we might want to have functions that take a
CPUState, yes. (We probably *don't* want those to do the
"call the unassigned access hook", because longjumping out of
functions is really hard to deal with, and we should I think
restrict that to code paths that come directly or near directly
from translated code. For instance if we take an exception trying
to read code during translation at the moment we do some completely
lunatic things that happen to work, and it would be much better
to just return a failure...) But that's separate cleanup IMHO.

In other words I'd rather try to restrict this to sorting out
the APIs that work generically on AddressSpaces. The amount
of change involved is already starting to feel very unwieldy.

> In any case, the removal or segregation of ld/st*_phys should be a
> separate series for ease of review.

Who wants to remove ld/st*_phys? Not me...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 24/03/2015 15:53, Peter Maydell wrote:
>> > In any case, the removal or segregation of ld/st*_phys should be a
>> > separate series for ease of review.
> Who wants to remove ld/st*_phys? Not me...

Well, you want to rename them _and_ add new arguments.  Basically at the
end they don't exist anymore as we know them now. :)

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 24 March 2015 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 24/03/2015 15:53, Peter Maydell wrote:
>>> > In any case, the removal or segregation of ld/st*_phys should be a
>>> > separate series for ease of review.
>> Who wants to remove ld/st*_phys? Not me...
>
> Well, you want to rename them _and_ add new arguments.  Basically at the
> end they don't exist anymore as we know them now. :)

I guess :-)  So what exactly would you like to see as a
separate series?

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

> On 24 March 2015 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 24/03/2015 15:53, Peter Maydell wrote:
> >>> > In any case, the removal or segregation of ld/st*_phys should be a
> >>> > separate series for ease of review.
> >> Who wants to remove ld/st*_phys? Not me...
> >
> > Well, you want to rename them _and_ add new arguments.  Basically at the
> > end they don't exist anymore as we know them now. :)
> 
> I guess :-)  So what exactly would you like to see as a
> separate series?

Adding the arguments / renaming the functions, for those callers
of ld/st*_phys that use cs->as as the first argument.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 24 March 2015 at 16:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 24 March 2015 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > On 24/03/2015 15:53, Peter Maydell wrote:
>> >>> > In any case, the removal or segregation of ld/st*_phys should be a
>> >>> > separate series for ease of review.
>> >> Who wants to remove ld/st*_phys? Not me...
>> >
>> > Well, you want to rename them _and_ add new arguments.  Basically at the
>> > end they don't exist anymore as we know them now. :)
>>
>> I guess :-)  So what exactly would you like to see as a
>> separate series?
>
> Adding the arguments / renaming the functions

OK. (This will need the patch that actually at least defines
the MemTxAttr and MemTxResult types, obviously.)

>, for those callers
> of ld/st*_phys that use cs->as as the first argument.

...but I don't understand this caveat. I want to add arguments
and rename the functions for *all* callers of ld/st*_phys.
I don't want to specialcase the ones which happen to be
operating on cs->as.

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 24/03/2015 17:35, Peter Maydell wrote:
> On 24 March 2015 at 16:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 24 March 2015 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> On 24/03/2015 15:53, Peter Maydell wrote:
>>>>>>> In any case, the removal or segregation of ld/st*_phys should be a
>>>>>>> separate series for ease of review.
>>>>> Who wants to remove ld/st*_phys? Not me...
>>>>
>>>> Well, you want to rename them _and_ add new arguments.  Basically at the
>>>> end they don't exist anymore as we know them now. :)
>>>
>>> I guess :-)  So what exactly would you like to see as a
>>> separate series?
>>
>> Adding the arguments / renaming the functions
> 
> OK. (This will need the patch that actually at least defines
> the MemTxAttr and MemTxResult types, obviously.)
> 
>> , for those callers
>> of ld/st*_phys that use cs->as as the first argument.
> 
> ...but I don't understand this caveat. I want to add arguments
> and rename the functions for *all* callers of ld/st*_phys.
> I don't want to specialcase the ones which happen to be
> operating on cs->as.

The ones that operate on cs->as could become (for some CPUs at least)
special-cased accessors like the bus ones; for example building the
MemTxAttrs according to internal CPU state.

ld/st*_phys actually started as CPU-specific accessors, and most uses
are still of that kind, so it makes sense to me that we special-case
them.  Maybe it limits churn, maybe it doesn't.  But if it doesn't, it's
not like anything is lost.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 24 March 2015 at 17:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 24/03/2015 17:35, Peter Maydell wrote:
>> On 24 March 2015 at 16:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> On 24 March 2015 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> , for those callers
>>> of ld/st*_phys that use cs->as as the first argument.
>>
>> ...but I don't understand this caveat. I want to add arguments
>> and rename the functions for *all* callers of ld/st*_phys.
>> I don't want to specialcase the ones which happen to be
>> operating on cs->as.
>
> The ones that operate on cs->as could become (for some CPUs at least)
> special-cased accessors like the bus ones; for example building the
> MemTxAttrs according to internal CPU state.

Sure, individual targets could do something like this if they
wanted (compare the arm_ldl_code functions), once these renames
have gone in.

> ld/st*_phys actually started as CPU-specific accessors, and most uses
> are still of that kind, so it makes sense to me that we special-case
> them.  Maybe it limits churn, maybe it doesn't.  But if it doesn't, it's
> not like anything is lost.

I think this is where we disagree. I see ld/st*_phys as being
really generic -- they take an AddressSpace, after all, and
part of the same family with address_space_read &c. If you
don't leave them as generic, then you end up having to use
the really awkward _read/_write for simple accesses and
then manage the byteswapping yourself. That's why I want
to rename them into address_space_* : to indicate that they
are all part of the same family, and you can use
address_space_read if you want to read an arbitrary byte
buffer, or address_space_ldl_be if you want to read a
big endian 32 bit word, and so on.

(The only reason they started out CPU specific is because
we didn't have any concept at all of having more than
one address space, so there wasn't any need to say which
one you meant when you were doing a load.)

To me it makes much more sense that if a DMA controller
like pl080 wants to do an LE word read from the AS which
its bus master is connected to, that it can just do
 word = ldl_le_phys(my_as, addr, ...);

I'd expect pretty much any bus master to want to do this
kind of thing, in fact. It just happens that most of the
bus masters we have in QEMU right now are CPUs...

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 24/03/2015 19:06, Peter Maydell wrote:
> On 24 March 2015 at 17:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 24/03/2015 17:35, Peter Maydell wrote:
>>> On 24 March 2015 at 16:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> On 24 March 2015 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> , for those callers
>>>> of ld/st*_phys that use cs->as as the first argument.
>>>
>>> ...but I don't understand this caveat. I want to add arguments
>>> and rename the functions for *all* callers of ld/st*_phys.
>>> I don't want to specialcase the ones which happen to be
>>> operating on cs->as.
>>
>> The ones that operate on cs->as could become (for some CPUs at least)
>> special-cased accessors like the bus ones; for example building the
>> MemTxAttrs according to internal CPU state.
> 
> Sure, individual targets could do something like this if they
> wanted (compare the arm_ldl_code functions), once these renames
> have gone in.
> 
>> ld/st*_phys actually started as CPU-specific accessors, and most uses
>> are still of that kind, so it makes sense to me that we special-case
>> them.  Maybe it limits churn, maybe it doesn't.  But if it doesn't, it's
>> not like anything is lost.
> 
> I think this is where we disagree. I see ld/st*_phys as being
> really generic -- they take an AddressSpace, after all, and
> part of the same family with address_space_read &c. If you
> don't leave them as generic, then you end up having to use
> the really awkward _read/_write for simple accesses and
> then manage the byteswapping yourself. That's why I want
> to rename them into address_space_* : to indicate that they
> are all part of the same family, and you can use
> address_space_read if you want to read an arbitrary byte
> buffer, or address_space_ldl_be if you want to read a
> big endian 32 bit word, and so on.

I agree with that.  I just want to keep ld/st*_phys _in addition_ as the
short forms of address_space_ld/st*, and keep ld/st*_phys instead of
address_space_ld/st* for those uses that have cs->as as the first argument.

The rationale is to evolve ld/st*_phys into CPU-specific accessors
paralleling the bus-specific accessors.

Paolo

> (The only reason they started out CPU specific is because
> we didn't have any concept at all of having more than
> one address space, so there wasn't any need to say which
> one you meant when you were doing a load.)
> 
> To me it makes much more sense that if a DMA controller
> like pl080 wants to do an LE word read from the AS which
> its bus master is connected to, that it can just do
>  word = ldl_le_phys(my_as, addr, ...);
> 
> I'd expect pretty much any bus master to want to do this
> kind of thing, in fact. It just happens that most of the
> bus masters we have in QEMU right now are CPUs...
> 
> -- PMM
> 
> 

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 24 March 2015 at 20:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 24/03/2015 19:06, Peter Maydell wrote:
>> I think this is where we disagree. I see ld/st*_phys as being
>> really generic -- they take an AddressSpace, after all, and
>> part of the same family with address_space_read &c. If you
>> don't leave them as generic, then you end up having to use
>> the really awkward _read/_write for simple accesses and
>> then manage the byteswapping yourself. That's why I want
>> to rename them into address_space_* : to indicate that they
>> are all part of the same family, and you can use
>> address_space_read if you want to read an arbitrary byte
>> buffer, or address_space_ldl_be if you want to read a
>> big endian 32 bit word, and so on.
>
> I agree with that.  I just want to keep ld/st*_phys _in addition_ as the
> short forms of address_space_ld/st*, and keep ld/st*_phys instead of
> address_space_ld/st* for those uses that have cs->as as the first argument.

...but for ARM I want to be able to specify the memory
attribute argument (and possibly also get the behaviour
right on failure). So I definitely don't want the short
forms for my cs->as uses. And it seems to me at best
uncertain that anybody does, in the long run. So why leave
temptingly short and simple looking function names in
the codebase to trip people up?

> The rationale is to evolve ld/st*_phys into CPU-specific accessors
> paralleling the bus-specific accessors.

I don't think this is any harder starting from
address_space_ld/st* than if we leave ld/st*_phys
around.

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Igor Mammedov]

On Mon, 23 Mar 2015 17:32:21 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 23.03.2015 um 16:11 schrieb Peter Maydell:
> > On 23 March 2015 at 14:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On 23/03/2015 13:24, Peter Maydell wrote:
> >>>  * cpu_physical_memory_rw are obsolete and should be replaced
> >>>    with uses of the as_* functions -- we should at least note
> >>>    this in the header file. (Can't do this as an automated change
> >>>    really because the correct AS to use is callsite dependent.)
> >>
> >> All users that should _not_ be using address_space_memory have been
> >> already changed to address_space_rw, or should have, so it can be done
> >> automatically.  Same for cpu_physical_memory_map/unmap, BTW.
> > 
> > Hmm. Checking a few, I notice that for instance the kvm-all.c
> > cpu_physical_memory_rw() should probably be using cpu->as.
> [snip]
> 
> Igor, does that impact our APIC discussion?
it should work but it would still be the same as we do now
i.e. APIC MMIO regions would be mapped overlapped in 
address_space_memory (see 09daed84). But dispatch wouldn't
go through cpu->as listener (it's initialized only for TCG).

My hope was that with cpu->as we can get rid of 'current' usage
in APIC code. But even without that it would be nice cleanup
that removes not needed anymore icc bus and moves default APIC
mapping from board to a place where it belongs (i.e. into CPU).

> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 25/03/2015 00:41, Peter Maydell wrote:
> On 24 March 2015 at 20:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> I agree with that.  I just want to keep ld/st*_phys _in addition_ as the
>> short forms of address_space_ld/st*, and keep ld/st*_phys instead of
>> address_space_ld/st* for those uses that have cs->as as the first argument.
> 
> ...but for ARM I want to be able to specify the memory
> attribute argument (and possibly also get the behaviour
> right on failure). So I definitely don't want the short
> forms for my cs->as uses.

You're free to move ARM to the longer versions, and/or to push the short
versions to all cpu.h files except ARM's.

> And it seems to me at best
> uncertain that anybody does, in the long run.

I disagree: most CPUs are in odd fixes/unmaintained state (so attributes
probably won't matter), and most don't even define an unassigned_access
callback (so result won't matter either).

>> The rationale is to evolve ld/st*_phys into CPU-specific accessors
>> paralleling the bus-specific accessors.
> 
> I don't think this is any harder starting from
> address_space_ld/st* than if we leave ld/st*_phys
> around.

It does cause unnecessary churn though.

Paolo

Re: [Qemu-devel] RFC: memory API changes

[Peter Maydell]

On 25 March 2015 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 25/03/2015 00:41, Peter Maydell wrote:
>> On 24 March 2015 at 20:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> I agree with that.  I just want to keep ld/st*_phys _in addition_ as the
>>> short forms of address_space_ld/st*, and keep ld/st*_phys instead of
>>> address_space_ld/st* for those uses that have cs->as as the first argument.
>>
>> ...but for ARM I want to be able to specify the memory
>> attribute argument (and possibly also get the behaviour
>> right on failure). So I definitely don't want the short
>> forms for my cs->as uses.
>
> You're free to move ARM to the longer versions, and/or to push the short
> versions to all cpu.h files except ARM's.
>
>> And it seems to me at best
>> uncertain that anybody does, in the long run.
>
> I disagree: most CPUs are in odd fixes/unmaintained state (so attributes
> probably won't matter), and most don't even define an unassigned_access
> callback (so result won't matter either).

I was trying to avoid leaving us with yet another half-finished
set of API transitions: because many of our CPUs are in this
odd-fixes state, it's unlikely anybody will get round to
updating them in the near future, so we'll be carrying a
duplicate set of functions around for a long time. Doing
an automated update to change everything to the new style
seemed a better plan to me.

If you insist I can leave the ldl_phys&c around as wrappers
with a comment saying /* Do not use these in new code;
use address_space_* instead. */,
though.

-- PMM

Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 25/03/2015 12:43, Peter Maydell wrote:
> I was trying to avoid leaving us with yet another half-finished
> set of API transitions: because many of our CPUs are in this
> odd-fixes state, it's unlikely anybody will get round to
> updating them in the near future, so we'll be carrying a
> duplicate set of functions around for a long time.

They're not duplicate, they're shortcuts.  Using longer function names
with more arguments is unnecessary if there's no need for the extra
features.

> If you insist I can leave the ldl_phys&c around as wrappers
> with a comment saying /* Do not use these in new code;
> use address_space_* instead. */,
> though.

I'll take care of changing the wrappers to take a CPUState instead of
AddressSpace, and move them to cpu.h.

If you convert ARM to address_space_*, ARM's cpu.h obviously won't get them.

Paolo