Peter Maydell <peter.mayd...@linaro.org> writes:
> On Mon, 17 Oct 2022 at 14:50, Alexander Bulekov <alx...@bu.edu> wrote: >> >> On 221015 1710, Chris Friedt wrote: >> > From: Christopher Friedt <cfri...@meta.com> >> > >> > In the case that size1 was zero, because of the explicit >> > 'end1 > addr' check, the range check would fail and the error >> > message would read as shown below. The correct comparison >> > is 'end1 >= addr' (or 'addr <= end1'). >> > >> > EDU: DMA range 0x40000-0x3ffff out of bounds (0x40000-0x40fff)! >> > >> > At the opposite end, in the case that size1 was 4096, within() >> > would fail because of the non-inclusive check 'end1 < end2', >> > which should have been 'end1 <= end2'. The error message would >> > previously say >> > >> > EDU: DMA range 0x40000-0x40fff out of bounds (0x40000-0x40fff)! >> > >> > This change >> > 1. renames local variables to be more less ambiguous >> > 2. fixes the two off-by-one errors described above. >> > >> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1254 >> > >> > Signed-off-by: Christopher Friedt <cfri...@meta.com> >> >> Reviewed-by: Alexander Bulekov <alx...@bu.edu> >> >> As a side-note, seems strange that edu_check_range will abort the entire >> VM if the check fails, rather than handling the error more elegantly. > > Yes, this is bad for a device model, though we have a lot of > older device models that still do it. The preferred pattern is: > * for situations which are "if this happens there is a > bug in QEMU itself", use assert. hw_error() is a kind of > assert that prints a bunch of guest register state: sometimes > you want that, but more often you just want normal assert() > * for situations where the guest has misprogrammed the device, > log that with qemu_log_mask(LOG_GUEST_ERROR, ...) > and continue with whatever the real hardware would do, or > some reasonable choice if the h/w spec is vague > * for situations where the guest is correct but is trying to > get the device to do something our model doesn't implement > yet, same as above but with LOG_UNIMP. > > Probably most hw_error() uses in the codebase should be > replaced with one of the above options. We should probably document this best practice somewhere in docs/devel but I guess we really need a "guide to writing device emulation" section. > > thanks > -- PMM -- Alex Bennée
