Re: [v2] hw: misc: edu: fix 2 off-by-one errors

[Alex Bennée]

Chris Friedt <cfriedt@meta.com> writes:

>> On Oct 17, 2022, at 1:22 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 
>> > 
>> 
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>>>> On Mon, 17 Oct 2022 at 14:50, Alexander Bulekov <alxndr@bu.edu> wrote:
>>>> 
>>>> On 221015 1710, Chris Friedt wrote:
>>>>> From: Christopher Friedt <cfriedt@meta.com>
>>>>> 
>>>> Reviewed-by: Alexander Bulekov <alxndr@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.
>
> Should I make a separate PR for that or attach it to the existing
> series (at v3 now)?

I don't think improving our developer documentation needs to be part of
this fix. However if you want to take a run at improving it in a new
series be my guest ;-)

>
> Thanks,
>
> C


-- 
Alex Bennée