[Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Jason Wang]

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(+)

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:
-- 
2.1.4

[Qemu-devel] [PATCH 2/2] pci: test-dev: try to test fast mmio bus for wildcard mmio event

[Jason Wang]

Test fast mmio by using zero size eventfd.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/misc/pci-testdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/misc/pci-testdev.c b/hw/misc/pci-testdev.c
index 26b9b86..7bf67ed 100644
--- a/hw/misc/pci-testdev.c
+++ b/hw/misc/pci-testdev.c
@@ -261,8 +261,12 @@ static void pci_testdev_realize(PCIDevice *pci_dev, Error **errp)
         memcpy(test->hdr->name, name, strlen(name) + 1);
         g_free(name);
         test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
-        test->size = IOTEST_ACCESS_WIDTH;
         test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
+        if (!test->match_data && !strcmp(IOTEST_TYPE(i), "mmio")) {
+            test->size = 0;
+        } else {
+            test->size = IOTEST_ACCESS_WIDTH;
+        }
         test->hdr->test = i;
         test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
         test->hdr->width = IOTEST_ACCESS_WIDTH;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Greg Kurz]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Jason Wang]

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.

Re: [Qemu-devel] [PATCH 2/2] pci: test-dev: try to test fast mmio bus for wildcard mmio event

[Michael S. Tsirkin]

On Wed, Aug 26, 2015 at 06:04:08PM +0800, Jason Wang wrote:
> Test fast mmio by using zero size eventfd.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

wildcard is not the same as ignoring length.
I think I have a nicer patch, will post now.

> ---
>  hw/misc/pci-testdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/pci-testdev.c b/hw/misc/pci-testdev.c
> index 26b9b86..7bf67ed 100644
> --- a/hw/misc/pci-testdev.c
> +++ b/hw/misc/pci-testdev.c
> @@ -261,8 +261,12 @@ static void pci_testdev_realize(PCIDevice *pci_dev, Error **errp)
>          memcpy(test->hdr->name, name, strlen(name) + 1);
>          g_free(name);
>          test->hdr->offset = cpu_to_le32(IOTEST_SIZE(i) + i * IOTEST_ACCESS_WIDTH);
> -        test->size = IOTEST_ACCESS_WIDTH;
>          test->match_data = strcmp(IOTEST_TEST(i), "wildcard-eventfd");
> +        if (!test->match_data && !strcmp(IOTEST_TYPE(i), "mmio")) {
> +            test->size = 0;
> +        } else {
> +            test->size = IOTEST_ACCESS_WIDTH;
> +        }
>          test->hdr->test = i;
>          test->hdr->data = test->match_data ? IOTEST_DATAMATCH : IOTEST_NOMATCH;
>          test->hdr->width = IOTEST_ACCESS_WIDTH;
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Greg Kurz]

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

Re: [Qemu-devel] [PATCH 1/2] memory: allow zero size for adjust_endianness()

[Jason Wang]

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.