| Message ID | 1440583448-15797-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 26 August 2015 at 11:04, Jason Wang <jasowang@redhat.com> wrote: > Wildcard mmio eventfd use zero size, but it will lead abort() since it > was illegal in adjust_endianness(). Fix this by allowing zero size. > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> This seems to me like a bug in the caller. Why would anything try to call into the memory subsystem to do a zero-size transaction? thanks -- PMM
On Wed, 26 Aug 2015 15:21:59 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 August 2015 at 11:04, Jason Wang <jasowang@redhat.com> wrote: > > Wildcard mmio eventfd use zero size, but it will lead abort() since it > > was illegal in adjust_endianness(). Fix this by allowing zero size. > > > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > This seems to me like a bug in the caller. Why would anything > try to call into the memory subsystem to do a zero-size > transaction? > > thanks > -- PMM > Here's the patch which needs zero-size eventfd: http://patchwork.ozlabs.org/patch/509428/ Cheers. -- Greg
On 08/26/2015 10:51 PM, Greg Kurz wrote: > On Wed, 26 Aug 2015 15:21:59 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 26 August 2015 at 11:04, Jason Wang <jasowang@redhat.com> wrote: >>> Wildcard mmio eventfd use zero size, but it will lead abort() since it >>> was illegal in adjust_endianness(). Fix this by allowing zero size. >>> >>> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Jason Wang <jasowang@redhat.com> >> This seems to me like a bug in the caller. Why would anything >> try to call into the memory subsystem to do a zero-size >> transaction? >> >> thanks >> -- PMM >> > Here's the patch which needs zero-size eventfd: > > http://patchwork.ozlabs.org/patch/509428/ > > Cheers. > > -- > Greg > Yes, this is because we want to use wildcard mmio eventfd (which requires size to be zero) to speed up virtio 1.0 mmio.
On 27 August 2015 at 05:50, Jason Wang <jasowang@redhat.com> wrote: > On 08/26/2015 10:51 PM, Greg Kurz wrote: >> On Wed, 26 Aug 2015 15:21:59 +0100 >> Peter Maydell <peter.maydell@linaro.org> wrote: >>> This seems to me like a bug in the caller. Why would anything >>> try to call into the memory subsystem to do a zero-size >>> transaction? >> Here's the patch which needs zero-size eventfd: >> >> http://patchwork.ozlabs.org/patch/509428/ > Yes, this is because we want to use wildcard mmio eventfd (which > requires size to be zero) to speed up virtio 1.0 mmio. But *why* does it require the size to be zero? I still think the caller should just avoid trying to do zero-size memory operations: they don't make sense. What is a zero size operation supposed to mean? -- PMM
On Thu, Aug 27, 2015 at 11:49:32AM +0100, Peter Maydell wrote: > On 27 August 2015 at 05:50, Jason Wang <jasowang@redhat.com> wrote: > > On 08/26/2015 10:51 PM, Greg Kurz wrote: > >> On Wed, 26 Aug 2015 15:21:59 +0100 > >> Peter Maydell <peter.maydell@linaro.org> wrote: > >>> This seems to me like a bug in the caller. Why would anything > >>> try to call into the memory subsystem to do a zero-size > >>> transaction? > > >> Here's the patch which needs zero-size eventfd: > >> > >> http://patchwork.ozlabs.org/patch/509428/ > > > Yes, this is because we want to use wildcard mmio eventfd (which > > requires size to be zero) to speed up virtio 1.0 mmio. > > But *why* does it require the size to be zero? I still think > the caller should just avoid trying to do zero-size memory > operations: they don't make sense. What is a zero size > operation supposed to mean? > > -- PMM This just mirrors an API we have in kvm: if you pass 0 size when registering an ioeventfd, it will match on access of any size.
On 27 August 2015 at 11:53, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Aug 27, 2015 at 11:49:32AM +0100, Peter Maydell wrote: >> But *why* does it require the size to be zero? I still think >> the caller should just avoid trying to do zero-size memory >> operations: they don't make sense. What is a zero size >> operation supposed to mean? > This just mirrors an API we have in kvm: if you pass 0 > size when registering an ioeventfd, it will match on access > of any size. Hrm. It feels to me like the memory APIs ought to filter out bad access sizes at an earlier stage, rather than trying to make them work all the way through. -- PMM
On Thu, Aug 27, 2015 at 12:04:49PM +0100, Peter Maydell wrote: > On 27 August 2015 at 11:53, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Aug 27, 2015 at 11:49:32AM +0100, Peter Maydell wrote: > >> But *why* does it require the size to be zero? I still think > >> the caller should just avoid trying to do zero-size memory > >> operations: they don't make sense. What is a zero size > >> operation supposed to mean? > > > This just mirrors an API we have in kvm: if you pass 0 > > size when registering an ioeventfd, it will match on access > > of any size. > > Hrm. It feels to me like the memory APIs ought to filter > out bad access sizes at an earlier stage, rather than > trying to make them work all the way through. > > -- PMM Why do you mention APIs? It's all internal to memory.c, isn't it?
On 27 August 2015 at 12:08, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Aug 27, 2015 at 12:04:49PM +0100, Peter Maydell wrote: >> On 27 August 2015 at 11:53, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Thu, Aug 27, 2015 at 11:49:32AM +0100, Peter Maydell wrote: >> >> But *why* does it require the size to be zero? I still think >> >> the caller should just avoid trying to do zero-size memory >> >> operations: they don't make sense. What is a zero size >> >> operation supposed to mean? >> >> > This just mirrors an API we have in kvm: if you pass 0 >> > size when registering an ioeventfd, it will match on access >> > of any size. >> >> Hrm. It feels to me like the memory APIs ought to filter >> out bad access sizes at an earlier stage, rather than >> trying to make them work all the way through. > Why do you mention APIs? It's all internal to memory.c, isn't it? adjust_endianness() is internal to memory.c. The APIs memory.c exposes to the rest of the world are the ones declared in memory.h. I'm suggesting that it would be better to filter out rubbish like zero sizes at the point where the rest of the world calls the memory subsystem rather than ensuring that every part of the memory subsystem code can handle what is basically a completely meaningless request. -- PMM
On Thu, Aug 27, 2015 at 01:12:32PM +0100, Peter Maydell wrote: > On 27 August 2015 at 12:08, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Aug 27, 2015 at 12:04:49PM +0100, Peter Maydell wrote: > >> On 27 August 2015 at 11:53, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Thu, Aug 27, 2015 at 11:49:32AM +0100, Peter Maydell wrote: > >> >> But *why* does it require the size to be zero? I still think > >> >> the caller should just avoid trying to do zero-size memory > >> >> operations: they don't make sense. What is a zero size > >> >> operation supposed to mean? > >> > >> > This just mirrors an API we have in kvm: if you pass 0 > >> > size when registering an ioeventfd, it will match on access > >> > of any size. > >> > >> Hrm. It feels to me like the memory APIs ought to filter > >> out bad access sizes at an earlier stage, rather than > >> trying to make them work all the way through. > > > Why do you mention APIs? It's all internal to memory.c, isn't it? > > adjust_endianness() is internal to memory.c. The APIs > memory.c exposes to the rest of the world are the ones > declared in memory.h. I'm suggesting that it would be > better to filter out rubbish like zero sizes at the > point where the rest of the world calls the memory > subsystem rather than ensuring that every part of the > memory subsystem code can handle what is basically > a completely meaningless request. > > -- PMM Basically the point is that ABI is extended to make ioeventfd with len = 0 mean "any length". 0 is thus not meaningless anymore.
On 27 August 2015 at 13:17, Michael S. Tsirkin <mst@redhat.com> wrote: > Basically the point is that ABI is extended to make > ioeventfd with len = 0 mean "any length". > 0 is thus not meaningless anymore. But how can you do adjustment for incorrect endianness if you don't know the size of the data that you're trying to work with? That's why this switch insists that the size is 1, 2, 4 or 8. -- PMM
On Thu, Aug 27, 2015 at 01:20:52PM +0100, Peter Maydell wrote: > On 27 August 2015 at 13:17, Michael S. Tsirkin <mst@redhat.com> wrote: > > Basically the point is that ABI is extended to make > > ioeventfd with len = 0 mean "any length". > > 0 is thus not meaningless anymore. > > But how can you do adjustment for incorrect endianness > if you don't know the size of the data that you're > trying to work with? That's why this switch insists > that the size is 1, 2, 4 or 8. > > -- PMM For kvm at least, "any length" implies "any data". So data is eventually discarded, we don't really need to adjust it for endian-ness.
On 27 August 2015 at 13:25, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Aug 27, 2015 at 01:20:52PM +0100, Peter Maydell wrote: >> On 27 August 2015 at 13:17, Michael S. Tsirkin <mst@redhat.com> wrote: >> > Basically the point is that ABI is extended to make >> > ioeventfd with len = 0 mean "any length". >> > 0 is thus not meaningless anymore. >> >> But how can you do adjustment for incorrect endianness >> if you don't know the size of the data that you're >> trying to work with? That's why this switch insists >> that the size is 1, 2, 4 or 8. > For kvm at least, "any length" implies "any data". > So data is eventually discarded, we don't really need > to adjust it for endian-ness. I'm still confused. If you have data it needs to be adjusted. If we're not actually doing anything with the data why are we calling this function in the first place? -- PMM
On Thu, Aug 27, 2015 at 01:27:54PM +0100, Peter Maydell wrote: > On 27 August 2015 at 13:25, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Aug 27, 2015 at 01:20:52PM +0100, Peter Maydell wrote: > >> On 27 August 2015 at 13:17, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > Basically the point is that ABI is extended to make > >> > ioeventfd with len = 0 mean "any length". > >> > 0 is thus not meaningless anymore. > >> > >> But how can you do adjustment for incorrect endianness > >> if you don't know the size of the data that you're > >> trying to work with? That's why this switch insists > >> that the size is 1, 2, 4 or 8. > > > For kvm at least, "any length" implies "any data". > > So data is eventually discarded, we don't really need > > to adjust it for endian-ness. > > I'm still confused. If you have data it needs to be > adjusted. If we're not actually doing anything with > the data why are we calling this function in the first > place? > > -- PMM I guess you could skip calls to adjust_endianness when len == 0, that should work just as well.
On Thu, 27 Aug 2015 15:30:55 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Aug 27, 2015 at 01:27:54PM +0100, Peter Maydell wrote: > > On 27 August 2015 at 13:25, Michael S. Tsirkin <mst@redhat.com> wrote: > > > On Thu, Aug 27, 2015 at 01:20:52PM +0100, Peter Maydell wrote: > > >> On 27 August 2015 at 13:17, Michael S. Tsirkin <mst@redhat.com> wrote: > > >> > Basically the point is that ABI is extended to make > > >> > ioeventfd with len = 0 mean "any length". > > >> > 0 is thus not meaningless anymore. > > >> > > >> But how can you do adjustment for incorrect endianness > > >> if you don't know the size of the data that you're > > >> trying to work with? That's why this switch insists > > >> that the size is 1, 2, 4 or 8. > > > > > For kvm at least, "any length" implies "any data". > > > So data is eventually discarded, we don't really need > > > to adjust it for endian-ness. > > > > I'm still confused. If you have data it needs to be > > adjusted. If we're not actually doing anything with > > the data why are we calling this function in the first > > place? > > > > -- PMM > > I guess you could skip calls to adjust_endianness when len == 0, > that should work just as well. > adjust_endianness() is called from 4 different locations: - memory_region_dispatch_read() - memory_region_dispatch_write() - memory_region_add_eventfd() - memory_region_del_eventfd() Since the issue was raised for the eventfd ones, it makes more sense to check in the caller indeed... and to preserve other paths. Cheers. -- Greg
On 08/27/2015 09:10 PM, Greg Kurz wrote: > On Thu, 27 Aug 2015 15:30:55 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Thu, Aug 27, 2015 at 01:27:54PM +0100, Peter Maydell wrote: >>> On 27 August 2015 at 13:25, Michael S. Tsirkin <mst@redhat.com> wrote: >>>> On Thu, Aug 27, 2015 at 01:20:52PM +0100, Peter Maydell wrote: >>>>> On 27 August 2015 at 13:17, Michael S. Tsirkin <mst@redhat.com> wrote: >>>>>> Basically the point is that ABI is extended to make >>>>>> ioeventfd with len = 0 mean "any length". >>>>>> 0 is thus not meaningless anymore. >>>>> But how can you do adjustment for incorrect endianness >>>>> if you don't know the size of the data that you're >>>>> trying to work with? That's why this switch insists >>>>> that the size is 1, 2, 4 or 8. >>>> For kvm at least, "any length" implies "any data". >>>> So data is eventually discarded, we don't really need >>>> to adjust it for endian-ness. >>> I'm still confused. If you have data it needs to be >>> adjusted. If we're not actually doing anything with >>> the data why are we calling this function in the first >>> place? >>> >>> -- PMM >> I guess you could skip calls to adjust_endianness when len == 0, >> that should work just as well. >> > adjust_endianness() is called from 4 different locations: > - memory_region_dispatch_read() > - memory_region_dispatch_write() > - memory_region_add_eventfd() > - memory_region_del_eventfd() > > Since the issue was raised for the eventfd ones, it makes more sense to check > in the caller indeed... and to preserve other paths. > > Cheers. > > -- > Greg > Yes, this seems fine.
diff --git a/memory.c b/memory.c index 4eb138a..134aa57 100644 --- a/memory.c +++ b/memory.c @@ -353,6 +353,7 @@ static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned size) { if (memory_region_wrong_endianness(mr)) { switch (size) { + case 0: case 1: break; case 2:
Wildcard mmio eventfd use zero size, but it will lead abort() since it was illegal in adjust_endianness(). Fix this by allowing zero size. Cc: Greg Kurz <gkurz@linux.vnet.ibm.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- memory.c | 1 + 1 file changed, 1 insertion(+)