[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/6] Add various undefined MMIO r/w functions
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 0/6] Add various undefined MMIO r/w functions |
Date: |
Wed, 17 Jun 2020 16:42:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 6/17/20 4:05 PM, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 6/17/20 3:06 PM, Alex Williamson wrote:
>>> On Wed, 17 Jun 2020 16:39:56 +1000
>>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>>
>>>> On Wed, Jun 17, 2020 at 11:09:27AM +0530, P J P wrote:
>>>>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>>>>
>>>>> Hello,
>>>>>
>>>>> This series adds various undefined MMIO read/write functions
>>>>> to avoid potential guest crash via a NULL pointer dereference.
>>>>
>>>> Hrm. If this is such a common problem, maybe we should just add a
>>>> NULL check in the common paths.
>>>
>>> +1, clearly the behavior is already expected. Thanks,
>>
>> 20 months ago Peter suggested:
>>
>> "assert that every MemoryRegionOps has pointers to callbacks
>> in it, when it is registered in memory_region_init_io() and
>> memory_region_init_rom_device_nomigrate()."
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg573310.html
>>
>> Li Qiang refers to this post from Paolo:
>>
>>> static const MemoryRegionOps notdirty_mem_ops = {
>>> + .read = notdirty_mem_read,
>>> .write = notdirty_mem_write,
>>> .valid.accepts = notdirty_mem_accepts,
>>> .endianness = DEVICE_NATIVE_ENDIAN,
>>
>> "This cannot happen, since TLB_NOTDIRTY is only added
>> to the addr_write member (see accel/tcg/cputlb.c)."
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg561345.html
>
> What about catching it in memory_region_dispatch_write:
>
> if (mr->ops->write) {
> return access_with_adjusted_size(addr, &data, size,
> mr->ops->impl.min_access_size,
> mr->ops->impl.max_access_size,
> memory_region_write_accessor, mr,
> attrs);
> } else if (mr->ops->write_with_attrs) {
> return
> access_with_adjusted_size(addr, &data, size,
> mr->ops->impl.min_access_size,
> mr->ops->impl.max_access_size,
> memory_region_write_with_attrs_accessor,
> mr, attrs);
> } else {
> qemu_log_mask(LOG_UNIMP|LOG_GUEST_ERROR, "%s: %s un-handled write\n",
> __func__, mr->name);
The problem is what return value to return...
MEMTX_OK/MEMTX_ERROR/MEMTX_DECODE_ERROR? This is very
device-specific and can't be decided here for all the
cases.
Better to abort() and fix each device?
> }
>
>
- Re: [PATCH 4/6] prep: add ppc-parity write method, (continued)