| Message ID | 51E67B7A.8000800@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/17/2013 04:09 AM, Paolo Bonzini wrote: >> >> Fails for me: >> >> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed. > > This: > > unsigned access_size_min = mr->ops->impl.min_access_size; > unsigned access_size_max = mr->ops->impl.max_access_size; > > must be respectively: > > unsigned access_size_min = 1; > unsigned access_size_max = mr->ops->valid.max_access_size; > > access_size_min can be 1 because erroneous accesses must not crash > QEMU, they should trigger exceptions in the guest or just return > garbage (depending on the CPU). I'm not sure I understand the comment, > placing a 4-byte field at the last byte of a region makes no sense > (unless impl.unaligned is true). > > access_size_max can be mr->ops->valid.max_access_size because memory.c > can and will still break accesses bigger than > mr->ops->impl.max_access_size. > > Markus, can you try the minimal patch above? Or this one that also > does the consequent simplifications. NAK. If you remove the check here, you're just trading it for one in the device. The device told you that it can't support a 1 byte read. (Either that, or the device incorrectly reported what it can actually do.) The proper fix is to change the interface of memory_access_size such that it can report errors. Indeed, very likely we should change it and its callers to also support over-sized reads, like access_with_adjusted_size in memory.c. r~
Il 17/07/2013 15:23, Richard Henderson ha scritto: > On 07/17/2013 04:09 AM, Paolo Bonzini wrote: >>> >>> Fails for me: >>> >>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed. >> >> This: >> >> unsigned access_size_min = mr->ops->impl.min_access_size; >> unsigned access_size_max = mr->ops->impl.max_access_size; >> >> must be respectively: >> >> unsigned access_size_min = 1; >> unsigned access_size_max = mr->ops->valid.max_access_size; >> >> access_size_min can be 1 because erroneous accesses must not crash >> QEMU, they should trigger exceptions in the guest or just return >> garbage (depending on the CPU). I'm not sure I understand the comment, >> placing a 4-byte field at the last byte of a region makes no sense >> (unless impl.unaligned is true). >> >> access_size_max can be mr->ops->valid.max_access_size because memory.c >> can and will still break accesses bigger than >> mr->ops->impl.max_access_size. >> >> Markus, can you try the minimal patch above? Or this one that also >> does the consequent simplifications. > > NAK. > > If you remove the check here, you're just trading it for one in the device. > The device told you that it can't support a 1 byte read. (Either that, or the > device incorrectly reported what it can actually do.) There are two parts to this. First of all, mr->ops->impl.min_access_size is definitely wrong. The device told me that the MMIO functions only know about 2-byte accesses, but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done by memory.c). So I could change access_size_min to mr->ops->valid.min_access_size, which would also fix Markus's problem. But then, accesses smaller than mr->ops->valid.min_access_size are fine, they just result in exceptions or garbage reads (depending on the CPU). address_space_rw reports these errors just fine, memory_access_size's only purpose is to split address_space_rw's MMIO writes in a sensible manner. There is no error reporting because it is done in memory.c. In fact, I'm not even sure if users of memory_access_size (DMA to an MMIO destination) exist in real hardware. I'm curious if "BSAVE"ing 16-color EGA graphics works with a modern graphic card and a BIOS that doesn't use PIO. Paolo > The proper fix is to change the interface of memory_access_size such that it > can report errors. Indeed, very likely we should change it and its callers to > also support over-sized reads, like access_with_adjusted_size in memory.c. > > > r~ >
On 07/17/2013 06:45 AM, Paolo Bonzini wrote: >> NAK. >> >> If you remove the check here, you're just trading it for one in the device. >> The device told you that it can't support a 1 byte read. (Either that, or the >> device incorrectly reported what it can actually do.) > > There are two parts to this. > > First of all, mr->ops->impl.min_access_size is definitely wrong. The > device told me that the MMIO functions only know about 2-byte accesses, > but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done > by memory.c). I don't know enough about the specific device (or even which device it was) to know whether the IMPL and VALID fields are correct. > So I could change access_size_min to > mr->ops->valid.min_access_size, which would also fix Markus's problem. No, you can't. At least not without changing all of the callers. If you do as you suggest, the callers will invoke the device with a value of SIZE that is illegal according to IMPL. We might as well crash now than later. There are three possible solutions: (1) Return an error from memory_access_size, change the callers to propagate the error in some fashion. This isn't ideal, since in this case VALID indicates that the guest access is correct. (2) Return the implementation minimum, change the callers to interact with the device using that minimum. With this scenario, we should likely share code with access_with_adjusted_size. (3) Determine that the device's impl.min_access_size is wrong and adjust it. Responding to your earlier > erroneous accesses must not crash > QEMU, they should trigger exceptions in the guest or just return > garbage (depending on the CPU). I completely agree -- if we were talking about VALID. Since this is IMPL, it's not an "erroneous access", but rather QEMU not being self-consistent. And for internal logic errors, we've got asserts and aborts all over. r~
Il 17/07/2013 16:29, Richard Henderson ha scritto: > On 07/17/2013 06:45 AM, Paolo Bonzini wrote: >>> NAK. >>> >>> If you remove the check here, you're just trading it for one in the device. >>> The device told you that it can't support a 1 byte read. (Either that, or the >>> device incorrectly reported what it can actually do.) >> >> There are two parts to this. >> >> First of all, mr->ops->impl.min_access_size is definitely wrong. The >> device told me that the MMIO functions only know about 2-byte accesses, >> but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done >> by memory.c). > > I don't know enough about the specific device (or even which device it was) > to know whether the IMPL and VALID fields are correct. They are correct. The device was usb-uhci, FWIW. >> So I could change access_size_min to >> mr->ops->valid.min_access_size, which would also fix Markus's problem. > > No, you can't. At least not without changing all of the callers. > > If you do as you suggest, the callers will invoke the device with a value of > SIZE that is illegal according to IMPL. We might as well crash now than later. No, it won't. access_with_adjusted_size will take care of taking a size that IMPL rejects, and producing one or more accesses in a size that IMPL accepts. Now of course access_with_adjusted_size may have bugs handling misaligned addresses. That's possible. > There are three possible solutions: > > (1) Return an error from memory_access_size, change the callers to propagate > the error in some fashion. This isn't ideal, since in this case VALID > indicates that the guest access is correct. Agreed. > (2) Return the implementation minimum, change the callers to interact with > the device using that minimum. With this scenario, we should likely > share code with access_with_adjusted_size. I think you misunderstand what the impl.*_access_size are. impl.min/max_access_size is a private interface between the device and memory.c, to avoid having code all over the place to combine/split MMIO accesses. The public interface of the device is valid.*_access_size. >> erroneous accesses must not crash >> QEMU, they should trigger exceptions in the guest or just return >> garbage (depending on the CPU). > > I completely agree -- if we were talking about VALID. Since this is IMPL, it's > not an "erroneous access", but rather QEMU not being self-consistent. Actually, no, for two reasons: - address_space_rw memory accesses are exactly the same as memory accesses started by the guest. In many cases, they use addr/range pairs passed directly by the guest. It is not acceptable to crash on these. - as said above, impl.*_access_size is not visible outside the device itself, the public interface of the device is valid.*_access_size. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 17/07/2013 11:50, Markus Armbruster ha scritto: >> Richard Henderson <rth@twiddle.net> writes: >> >>> Honor the implementation maximum access size, and at least check >>> the minimum access size. >>> >>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Richard Henderson <rth@twiddle.net> >> >> Fails for me: >> >> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed. > > This: > > unsigned access_size_min = mr->ops->impl.min_access_size; > unsigned access_size_max = mr->ops->impl.max_access_size; > > must be respectively: > > unsigned access_size_min = 1; > unsigned access_size_max = mr->ops->valid.max_access_size; > > access_size_min can be 1 because erroneous accesses must not crash > QEMU, they should trigger exceptions in the guest or just return > garbage (depending on the CPU). I'm not sure I understand the comment, > placing a 4-byte field at the last byte of a region makes no sense > (unless impl.unaligned is true). > > access_size_max can be mr->ops->valid.max_access_size because memory.c > can and will still break accesses bigger than > mr->ops->impl.max_access_size. > > Markus, can you try the minimal patch above? Or this one that also > does the consequent simplifications. FYI, the reproducer is very simple: qemu-system-x86_64 -usb Regards, Anthony Liguori > > diff --git a/exec.c b/exec.c > index c99a883..0904283 100644 > --- a/exec.c > +++ b/exec.c > @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) > > static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) > { > - unsigned access_size_min = mr->ops->impl.min_access_size; > - unsigned access_size_max = mr->ops->impl.max_access_size; > + unsigned access_size_max = mr->ops->valid.max_access_size; > > - /* Regions are assumed to support 1-4 byte accesses unless > - otherwise specified. */ > - if (access_size_min == 0) { > - access_size_min = 1; > - } > if (access_size_max == 0) { > access_size_max = 4; > } > @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) > if (l > access_size_max) { > l = access_size_max; > } > - /* ??? The users of this function are wrong, not supporting minimums larger > - than the remaining length. C.f. memory.c:access_with_adjusted_size. */ > - assert(l >= access_size_min); > > return l; > } > > Paolo
Il 17/07/2013 17:50, Anthony Liguori ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 17/07/2013 11:50, Markus Armbruster ha scritto: >>> Richard Henderson <rth@twiddle.net> writes: >>> >>>> Honor the implementation maximum access size, and at least check >>>> the minimum access size. >>>> >>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >>>> Signed-off-by: Richard Henderson <rth@twiddle.net> >>> >>> Fails for me: >>> >>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed. >> >> This: >> >> unsigned access_size_min = mr->ops->impl.min_access_size; >> unsigned access_size_max = mr->ops->impl.max_access_size; >> >> must be respectively: >> >> unsigned access_size_min = 1; >> unsigned access_size_max = mr->ops->valid.max_access_size; >> >> access_size_min can be 1 because erroneous accesses must not crash >> QEMU, they should trigger exceptions in the guest or just return >> garbage (depending on the CPU). I'm not sure I understand the comment, >> placing a 4-byte field at the last byte of a region makes no sense >> (unless impl.unaligned is true). >> >> access_size_max can be mr->ops->valid.max_access_size because memory.c >> can and will still break accesses bigger than >> mr->ops->impl.max_access_size. >> >> Markus, can you try the minimal patch above? Or this one that also >> does the consequent simplifications. > > FYI, the reproducer is very simple: > > qemu-system-x86_64 -usb My patch works. Paolo > Regards, > > Anthony Liguori > >> >> diff --git a/exec.c b/exec.c >> index c99a883..0904283 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) >> >> static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) >> { >> - unsigned access_size_min = mr->ops->impl.min_access_size; >> - unsigned access_size_max = mr->ops->impl.max_access_size; >> + unsigned access_size_max = mr->ops->valid.max_access_size; >> >> - /* Regions are assumed to support 1-4 byte accesses unless >> - otherwise specified. */ >> - if (access_size_min == 0) { >> - access_size_min = 1; >> - } >> if (access_size_max == 0) { >> access_size_max = 4; >> } >> @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) >> if (l > access_size_max) { >> l = access_size_max; >> } >> - /* ??? The users of this function are wrong, not supporting minimums larger >> - than the remaining length. C.f. memory.c:access_with_adjusted_size. */ >> - assert(l >= access_size_min); >> >> return l; >> } >> >> Paolo >
On 07/17/2013 10:32 AM, Paolo Bonzini wrote:
> My patch works.
You patch doesn't crash for this device, which isn't quite the same thing.
But it's certainly no worse than we had before my patch, so I'll not object so
long as a fixme sort of comment is installed too.
r~
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 17/07/2013 17:50, Anthony Liguori ha scritto: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 17/07/2013 11:50, Markus Armbruster ha scritto: >>>> Richard Henderson <rth@twiddle.net> writes: >>>> >>>>> Honor the implementation maximum access size, and at least check >>>>> the minimum access size. >>>>> >>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >>>>> Signed-off-by: Richard Henderson <rth@twiddle.net> >>>> >>>> Fails for me: >>>> >>>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed. >>> >>> This: >>> >>> unsigned access_size_min = mr->ops->impl.min_access_size; >>> unsigned access_size_max = mr->ops->impl.max_access_size; >>> >>> must be respectively: >>> >>> unsigned access_size_min = 1; >>> unsigned access_size_max = mr->ops->valid.max_access_size; >>> >>> access_size_min can be 1 because erroneous accesses must not crash >>> QEMU, they should trigger exceptions in the guest or just return >>> garbage (depending on the CPU). I'm not sure I understand the comment, >>> placing a 4-byte field at the last byte of a region makes no sense >>> (unless impl.unaligned is true). >>> >>> access_size_max can be mr->ops->valid.max_access_size because memory.c >>> can and will still break accesses bigger than >>> mr->ops->impl.max_access_size. >>> >>> Markus, can you try the minimal patch above? Or this one that also >>> does the consequent simplifications. >> >> FYI, the reproducer is very simple: >> >> qemu-system-x86_64 -usb > > My patch works. Yes, can you send a SoB and submit as a top level? Right now uhci is completely broken. Regards, Anthony Liguori > > Paolo > >> Regards, >> >> Anthony Liguori >> >>> >>> diff --git a/exec.c b/exec.c >>> index c99a883..0904283 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) >>> >>> static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) >>> { >>> - unsigned access_size_min = mr->ops->impl.min_access_size; >>> - unsigned access_size_max = mr->ops->impl.max_access_size; >>> + unsigned access_size_max = mr->ops->valid.max_access_size; >>> >>> - /* Regions are assumed to support 1-4 byte accesses unless >>> - otherwise specified. */ >>> - if (access_size_min == 0) { >>> - access_size_min = 1; >>> - } >>> if (access_size_max == 0) { >>> access_size_max = 4; >>> } >>> @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) >>> if (l > access_size_max) { >>> l = access_size_max; >>> } >>> - /* ??? The users of this function are wrong, not supporting minimums larger >>> - than the remaining length. C.f. memory.c:access_with_adjusted_size. */ >>> - assert(l >= access_size_min); >>> >>> return l; >>> } >>> >>> Paolo >>
Il 17/07/2013 20:26, Richard Henderson ha scritto: > On 07/17/2013 10:32 AM, Paolo Bonzini wrote: >> My patch works. > > You patch doesn't crash for this device, which isn't quite the same thing. > > But it's certainly no worse than we had before my patch, so I'll not object so > long as a fixme sort of comment is installed too. I'm still not sure what the bug is (so what the FIXME comment would be)... except of course that there may be bug in access_with_adjusted_size. Paolo
On 07/17/2013 11:57 AM, Paolo Bonzini wrote: > I'm still not sure what the bug is (so what the FIXME comment would > be)... except of course that there may be bug in access_with_adjusted_size. The code here in exec.c is not using access_with_adjusted_size. Unfortunately, access_with_adjusted_size only handles single copies, one direction at a time. We're attempting a sort of "memcpy", which calls for some amount of caching across the loop... r~
Il 17/07/2013 21:28, Richard Henderson ha scritto: > On 07/17/2013 11:57 AM, Paolo Bonzini wrote: >> I'm still not sure what the bug is (so what the FIXME comment would >> be)... except of course that there may be bug in access_with_adjusted_size. > > The code here in exec.c is not using access_with_adjusted_size. It is: cpu_outb -> address_space_write -> address_space_rw -> io_mem_write -> memory_region_dispatch_write -> access_with_adjusted_size memory_access_size is just returning a length that makes sense when passed to io_mem_write and ultimately to access_with_adjusted_size. Paolo > Unfortunately, access_with_adjusted_size only handles single copies, > one direction at a time. We're attempting a sort of "memcpy", which > calls for some amount of caching across the loop... > > > r~ >
On 07/17/2013 12:56 PM, Paolo Bonzini wrote: > It is: > > cpu_outb > -> address_space_write > -> address_space_rw > -> io_mem_write > -> memory_region_dispatch_write > -> access_with_adjusted_size > > memory_access_size is just returning a length that makes sense when > passed to io_mem_write and ultimately to access_with_adjusted_size. Ah, ok. Sorry for being dense about the full context here. I agree ignoring impl.minimum is ok here, since a real assert ought to be lower down in access_with_adjusted_size, right before actually dispatching to the device code. r~
diff --git a/exec.c b/exec.c index c99a883..0904283 100644 --- a/exec.c +++ b/exec.c @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) { - unsigned access_size_min = mr->ops->impl.min_access_size; - unsigned access_size_max = mr->ops->impl.max_access_size; + unsigned access_size_max = mr->ops->valid.max_access_size; - /* Regions are assumed to support 1-4 byte accesses unless - otherwise specified. */ - if (access_size_min == 0) { - access_size_min = 1; - } if (access_size_max == 0) { access_size_max = 4; } @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) if (l > access_size_max) { l = access_size_max; } - /* ??? The users of this function are wrong, not supporting minimums larger - than the remaining length. C.f. memory.c:access_with_adjusted_size. */ - assert(l >= access_size_min); return l; }