Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

[P J P]

+-- On Thu, 18 Jun 2020, Paolo Bonzini wrote --+
| On 18/06/20 15:12, Alex Bennée wrote:
| > If you look at memory_region_dispatch_write you can see that 
| > mr->ops->write being empty is acceptable because it implies 
| > mr->ops->write_with_attrs is set instead. I think the same is true for 
| > read so I think you need something more like:
| > 
| >      assert(ops->read || ops->read_with_attrs);
| >      assert(ops->write || ops->write_with_attrs);
| 
| Also, !ops is acceptable since you have a couple lines below:
| 
|     mr->ops = ops ? ops : &unassigned_mem_ops;

  Oops! :(
 
| Needless to say, this is something that the submitter should have done,
| not the reviewer.

  Sorry, I should've considered full effects of the patch, not just fixing the 
NULL dereference issue at hand. Sorry about the haste, I'll be careful in 
future.


| > Also doesn't this break a load of running stuff without fixes for all the 
| > various missing bits? How far does make check-acceptance get?

I tried to run '$ make check-acceptance', it's breaking with time-out error.

urllib.error.URLError: <urlopen error [Errno 110] Connection timed out>
urllib.error.URLError: <urlopen error [Errno 110] Connection timed out>
...
make: *** [../qemu/tests/Makefile.include:933: get-vm-image-fedora-31-ppc64le] 
Error 1


| This might actually be really close with the above assertions fixed.
| For example, commit 08565552f7 ("cputlb: Move NOTDIRTY handling from I/O
| path to TLB path", 2019-09-25) got rid of io_mem_notdirty.
| 
| The only cases I found with "git grep" are:
| 
| - tz_ppc_dummy_ops which is broken and should just use NULL ops
| 
| - hw/nvram/nrf51_nvm.c's flash_ops which is fixed if ROMD regions are
| changed not to require a read callback.
| 
| - designware_pci_host_msi_ops which is broken and should have a dummy
| read callback.


ie. we add these routines along with the assertions here, right?

Earlier patch series adds routines for nrf51_nvm and designware. I'll add 
r/w routines for tz_ppc_dummy_ops.

Thank you so much for the review.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D