Jason Wang <jasow...@redhat.com> 于2020年9月3日周四 下午2:16写道: > > > On 2020/9/3 下午12:50, Li Qiang wrote: > > Jason Wang <jasow...@redhat.com> 于2020年9月3日周四 下午12:24写道: > >> > >> On 2020/9/3 下午12:06, Alexander Bulekov wrote: > >>> On 200903 1154, Jason Wang wrote: > >>>> On 2020/9/3 上午12:22, Li Qiang wrote: > >>>>> The qemu device fuzzer has found several DMA to MMIO issue. > >>>>> These issues is caused by the guest driver programs the DMA > >>>>> address, then in the device MMIO handler it trigger the DMA > >>>>> and as the DMA address is MMIO it will trigger another dispatch > >>>>> and reenter the MMIO handler again. However most of the device > >>>>> is not reentrant. > >>>>> > >>>>> DMA to MMIO will cause issues depend by the device emulator, > >>>>> mostly it will crash the qemu. Following is three classic > >>>>> DMA to MMIO issue. > >>>>> > >>>>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362 > >>>>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354 > >>>>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606 > >>>>> > >>>>> The DMA to MMIO issue I think can be classified as following: > >>>>> 1. DMA to the device itself > >>>>> 2. device A DMA to device B and to device C > >>>>> 3. device A DMA to device B and to device A > >>>>> > >>>>> The first case of course should not be allowed. > >>>>> The second case I think it ok as the device IO handler has no > >>>>> assumption about the IO data came from no matter it come from > >>>>> device or other device. This is for P2P DMA. > >>>>> The third case I think it also should not be allowed. > >>>>> > >>>>> So our issue has been reduced by one case: not allowed the > >>>>> device's IO handler reenter. > >>>>> > >>>>> Paolo suggested that we can refactor the device emulation with > >>>>> BH. However it is a lot of work. > >>>>> I have thought several propose to address this, also discuss > >>>>> this with Jason Wang in private email. > >>>>> > >>>>> I have can solve this issue in core framework or in specific device. > >>>>> After try several methods I choose address it in per-device for > >>>>> following reason: > >>>>> 1. If we address it in core framwork we have to recored and check the > >>>>> device or MR info in MR dispatch write function. Unfortunally we have > >>>>> no these info in core framework. > >>>>> 2. The performance will also be decrease largely > >>>>> 3. Only the device itself know its IO > >>>> I think we still need to seek a way to address this issue completely. > >>>> > >>>> How about adding a flag in MemoryRegionOps and detect the reentrancy > >>>> through > >>>> that flag? > >>> What happens for devices with multiple MemoryRegions? Make all the > >>> MemoryRegionOps share the same flag? > >> > >> I think there could be two approaches: > >> > >> 1) record the device in MR as Qiang mentioned > > I have tried this as we discussed. But has following consideration: > > 1. The performance, we need to check/record/clean the MR in an > > array/hashtable. > > > > 2. The multiple MR and alias MR process in the memory layer. It is > > complicated and performance effective. > > So If we let the MR issue to the device itself, it is just as this > > patch does-let the device address the reentrancy issue.f > > > > Another solution. We connects a MR with the corresponding device. Now > > the device often tight MR with an 'opaque' field. > > Just uses it in the calling of MR callback. Then we add a flag in the > > device and needs to modify the MR register interface. > > > > So in the memory layer we can check/record/clean the MR->device->flag. > > But this is can't address the DMA (in BH) to MMIO issue as the BH runs > > in main thread. > > > This is probably good enough to start. To my point of view, there're two > different issues: > > 1) re-entrant MMIO handler > 2) MMIO hanlder sync with BH >
Agree, here I want to address these two kind of issue in a manner so it just be left to the device itself. I will try to add a new memory register function memory_region_init_io_with_device to connect the MR and device. And solve it in the memory layer. Thanks, Li Qiang > For 1), we'd better solve it at core, For 2) it can only be solved in > the device. > > Thanks > > > > > > Thanks, > > Li Qiang > > > > > > > >> 2) Only forbid the reentrancy in MMIO handler and depends on the device > >> to solve the multiple Memory Region issue, if the regions want to access > >> the same data, it needs to be synchronized internally > >> > >> But the point is still to try to solve it in the layer of memory > >> regions. Otherwise we may still hit similar issues. > >> > >> > >>> What about the virtio-gpu bug, where the problem happens in a bh->mmio > >>> access rather than an mmio->mmio access? > >> > >> Yes, it needs more thought, but as a first step, we can try to fix the > >> MMIO handler issue and do bh fix on top. > > > > > >> Thanks > >> > >> > >>> -Alex > >>> > >>>> Thanks > >>>> > >>>> > >>>>> The (most of the) device emulation is protected by BQL one time only > >>>>> a device emulation code can be run. We can add a flag to indicate the > >>>>> IO is running. The first two patches does this. For simplicity at the > >>>>> RFC stage I just set it while enter the IO callback and clear it exit > >>>>> the IO callback. It should be check/set/clean according the per-device's > >>>>> IO emulation. > >>>>> The second issue which itself suffers a race condition so I uses a > >>>>> atomic. > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> Li Qiang (3): > >>>>> e1000e: make the IO handler reentrant > >>>>> xhci: make the IO handler reentrant > >>>>> virtio-gpu: make the IO handler reentrant > >>>>> > >>>>> hw/display/virtio-gpu.c | 10 ++++++ > >>>>> hw/net/e1000e.c | 35 +++++++++++++++++++- > >>>>> hw/usb/hcd-xhci.c | 60 > >>>>> ++++++++++++++++++++++++++++++++++ > >>>>> hw/usb/hcd-xhci.h | 1 + > >>>>> include/hw/virtio/virtio-gpu.h | 1 + > >>>>> 5 files changed, 106 insertions(+), 1 deletion(-) > >>>>> >