On Mon, Sep 16, 2024 at 11:05 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Mon, 16 Sept 2024 at 08:35, Mattias Nissler <mniss...@rivosinc.com> wrote: > > > > On Fri, Sep 13, 2024 at 6:47 PM Peter Maydell <peter.mayd...@linaro.org> > > wrote: > > > > > > On Fri, 13 Sept 2024 at 16:55, Peter Xu <pet...@redhat.com> wrote: > > > > > > > > On Thu, Sep 12, 2024 at 03:27:55PM +0100, Peter Maydell wrote: > > > > > Coverity is pretty unhappy about this trick, because it isn't able > > > > > to recognise that we can figure out the address of 'bounce' > > > > > from the address of 'bounce->buffer' and free it in the > > > > > address_space_unmap() code, so it thinks that every use > > > > > of address_space_map(), pci_dma_map(), etc, is a memory leak. > > > > > We can mark all those as false positives, of course, but it got > > > > > me wondering whether maybe we should have this function return > > > > > a struct that has all the information address_space_unmap() > > > > > needs rather than relying on it being able to figure it out > > > > > from the host memory pointer... > > > > > > > > Indeed that sounds like a viable option. Looks like we don't have a > > > > lot of > > > > address_space_map() users. > > > > > > There's quite a few wrappers of it too, so it's a little hard to count. > > > We might want to avoid the memory allocation in the common case > > > by having the caller pass in an ASMapInfo struct to be filled > > > in rather than having address_space_map() allocate-and-return one. > > > > Hm, this would work, but not only does it complicate the code > > consuming address_space_map, but it also increases memory footprint (a > > pointer being replaced by a struct of sizeof(BounceBuffer) if done > > naively), plus there's an additional pointer indirection (I'm doubtful > > whether this can be optimized away by the compiler). I haven't done > > any measurements of these effects, so can't say anything definitive, > > but this seems pretty costly just to appease coverity... > > > > Is there no way to inform coverity that a resource pointer is being > > transmuted into a handle, so it can track that instead? Given that > > pointer tricks like this and container_of usage is quite frequent, I > > would expect coverity to have a better strategy to handle these rather > > than suppressing false positive leak reports? > > It's not purely that I want to appease Coverity. I also > think for human readers that the current trick with passing > back a pointer into host memory and relying on being able to > get back to either the MR or to the bounce-buffer struct > from that is pretty tricky. Would we have designed it that > way if we weren't starting with the pre-existing address_space_map() > function signature?
Identifying a mapping by its address seems pretty natural to me for callers of the API, at least as long as the address is all that the caller ever needs. The solution I implemented didn't/doesn't seem overly complicated to me, and I am hoping it wouldn't overwhelm anyone else who has worked with container_of (which seems to be used liberally in qemu). But at the end of the day it's a matter of personal taste and the background of the person writing the code, so I'll readily admit that this is personal opinion, and if qemu does prefer a different style then by all means we should change it and I'll be happy to help with that.
