On 23/03/2015 16:11, Peter Maydell wrote: > On 23 March 2015 at 14:39, Paolo Bonzini <pbonz...@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
