* [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
@ 2017-04-19 19:44 Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 1/2] vfio: Set MemoryRegionOps:max_access_size and min_access_size Jose Ricardo Ziviani
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jose Ricardo Ziviani @ 2017-04-19 19:44 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, pbonzini, mdroth, aik
This patchset has two patches:
[1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
[2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
Patches based on master.
Jose Ricardo Ziviani (2):
vfio: Set MemoryRegionOps:max_access_size and min_access_size
vfio: enable 8-byte reads/writes to vfio
hw/vfio/common.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] vfio: Set MemoryRegionOps:max_access_size and min_access_size
2017-04-19 19:44 [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Jose Ricardo Ziviani
@ 2017-04-19 19:44 ` Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 2/2] vfio: enable 8-byte reads/writes to vfio Jose Ricardo Ziviani
2017-04-20 7:19 ` [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Richard Henderson
2 siblings, 0 replies; 9+ messages in thread
From: Jose Ricardo Ziviani @ 2017-04-19 19:44 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, pbonzini, mdroth, aik
Sets valid.max_access_size and valid.min_access_size to ensure safe
8-byte accesses to vfio. Today, 8-byte accesses are broken into pairs
of 4-byte calls that goes unprotected:
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc0, 0x2020c, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc4, 0xa0000, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
which occasionally leads to:
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc0, 0x2030c, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc0, 0x1000c, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc4, 0xb0000, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc4, 0xa0000, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
causing strange errors in guest OS. With this patch, such accesses
are protected by the same lock guard:
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc0, 0x2000c, 4)
vfio_region_write (0001:03:00.0:region1+0xc4, 0xb0000, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
This happens because the 8-byte write should be broken into 4-byte
writes by memory.c:access_with_adjusted_size() in order to be under
the same lock. Today, it's done in exec.c:address_space_write_continue()
which was able to handle only 4 bytes due to a zero'ed
valid.max_access_size (see exec.c:memory_access_size()).
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
hw/vfio/common.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index f3ba9b9..145f2f4 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -190,6 +190,10 @@ const MemoryRegionOps vfio_region_ops = {
.read = vfio_region_read,
.write = vfio_region_write,
.endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 8,
+ },
};
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] vfio: enable 8-byte reads/writes to vfio
2017-04-19 19:44 [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 1/2] vfio: Set MemoryRegionOps:max_access_size and min_access_size Jose Ricardo Ziviani
@ 2017-04-19 19:44 ` Jose Ricardo Ziviani
2017-04-20 7:19 ` [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Richard Henderson
2 siblings, 0 replies; 9+ messages in thread
From: Jose Ricardo Ziviani @ 2017-04-19 19:44 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, pbonzini, mdroth, aik
This patch enables 8-byte writes and reads to VFIO. Such implemention
is already done but it's missing the 'case' to handle such accesses in
both vfio_region_write and vfio_region_read and the MemoryRegionOps:
impl.max_access_size and impl.min_access_size.
After this patch, 8-byte writes such as:
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc0, 0x4140c, 4)
vfio_region_write (0001:03:00.0:region1+0xc4, 0xa0000, 4)
qemu_mutex_unlock unlocked mutex 0x10905ad8
goes like this:
qemu_mutex_lock locked mutex 0x10905ad8
vfio_region_write (0001:03:00.0:region1+0xc0, 0xbfd0008, 8)
qemu_mutex_unlock unlocked mutex 0x10905ad8
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
---
hw/vfio/common.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 145f2f4..43c8e98 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -119,6 +119,9 @@ void vfio_region_write(void *opaque, hwaddr addr,
case 4:
buf.dword = cpu_to_le32(data);
break;
+ case 8:
+ buf.qword = cpu_to_le64(data);
+ break;
default:
hw_error("vfio: unsupported write size, %d bytes", size);
break;
@@ -173,6 +176,9 @@ uint64_t vfio_region_read(void *opaque,
case 4:
data = le32_to_cpu(buf.dword);
break;
+ case 8:
+ data = le64_to_cpu(buf.qword);
+ break;
default:
hw_error("vfio: unsupported read size, %d bytes", size);
break;
@@ -194,6 +200,10 @@ const MemoryRegionOps vfio_region_ops = {
.min_access_size = 1,
.max_access_size = 8,
},
+ .impl = {
+ .min_access_size = 1,
+ .max_access_size = 8,
+ },
};
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
2017-04-19 19:44 [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 1/2] vfio: Set MemoryRegionOps:max_access_size and min_access_size Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 2/2] vfio: enable 8-byte reads/writes to vfio Jose Ricardo Ziviani
@ 2017-04-20 7:19 ` Richard Henderson
2017-04-20 16:03 ` Alex Williamson
2 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2017-04-20 7:19 UTC (permalink / raw)
To: Jose Ricardo Ziviani, qemu-devel; +Cc: aik, pbonzini, alex.williamson, mdroth
On 04/19/2017 12:44 PM, Jose Ricardo Ziviani wrote:
> This patchset has two patches:
> [1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
>
> [2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
>
> Patches based on master.
>
> Jose Ricardo Ziviani (2):
> vfio: Set MemoryRegionOps:max_access_size and min_access_size
> vfio: enable 8-byte reads/writes to vfio
>
> hw/vfio/common.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
I think these patches need to be squashed to be bisectable.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
2017-04-20 7:19 ` [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Richard Henderson
@ 2017-04-20 16:03 ` Alex Williamson
2017-04-21 10:06 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2017-04-20 16:03 UTC (permalink / raw)
To: Richard Henderson; +Cc: Jose Ricardo Ziviani, qemu-devel, aik, pbonzini, mdroth
On Thu, 20 Apr 2017 00:19:23 -0700
Richard Henderson <rth@twiddle.net> wrote:
> On 04/19/2017 12:44 PM, Jose Ricardo Ziviani wrote:
> > This patchset has two patches:
> > [1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
> >
> > [2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
> >
> > Patches based on master.
> >
> > Jose Ricardo Ziviani (2):
> > vfio: Set MemoryRegionOps:max_access_size and min_access_size
> > vfio: enable 8-byte reads/writes to vfio
> >
> > hw/vfio/common.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
>
> I think these patches need to be squashed to be bisectable.
No, I think it's fine. The point of patch 1/2 is to indicate that the
hardware supports 8-byte accesses, which will still be broken into 2
4-byte accesses because we don't yet set the implemented width beyond
the default. The important part is that the mutex will now group the 4
byte access pair together rather than letting them get re-ordered.
Patch 2/2 then implements native 8-byte access. I appreciate them
being separate for this subtle nuance, but maybe I'm not seeing the
same issue as you. Thanks,
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
2017-04-20 16:03 ` Alex Williamson
@ 2017-04-21 10:06 ` Paolo Bonzini
2017-04-21 15:51 ` Richard Henderson
2017-05-03 2:41 ` joserz
0 siblings, 2 replies; 9+ messages in thread
From: Paolo Bonzini @ 2017-04-21 10:06 UTC (permalink / raw)
To: Alex Williamson, Richard Henderson
Cc: Jose Ricardo Ziviani, qemu-devel, aik, mdroth
On 20/04/2017 18:03, Alex Williamson wrote:
> On Thu, 20 Apr 2017 00:19:23 -0700
> Richard Henderson <rth@twiddle.net> wrote:
>
>> On 04/19/2017 12:44 PM, Jose Ricardo Ziviani wrote:
>>> This patchset has two patches:
>>> [1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
>>>
>>> [2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
>>>
>>> Patches based on master.
>>>
>>> Jose Ricardo Ziviani (2):
>>> vfio: Set MemoryRegionOps:max_access_size and min_access_size
>>> vfio: enable 8-byte reads/writes to vfio
>>>
>>> hw/vfio/common.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>
>> I think these patches need to be squashed to be bisectable.
>
> No, I think it's fine. The point of patch 1/2 is to indicate that the
> hardware supports 8-byte accesses, which will still be broken into 2
> 4-byte accesses because we don't yet set the implemented width beyond
> the default. The important part is that the mutex will now group the 4
> byte access pair together rather than letting them get re-ordered.
> Patch 2/2 then implements native 8-byte access. I appreciate them
> being separate for this subtle nuance, but maybe I'm not seeing the
> same issue as you. Thanks,
'
I agree, the patches looks fine as is.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
2017-04-21 10:06 ` Paolo Bonzini
@ 2017-04-21 15:51 ` Richard Henderson
2017-05-03 2:41 ` joserz
1 sibling, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2017-04-21 15:51 UTC (permalink / raw)
To: Paolo Bonzini, Alex Williamson
Cc: Jose Ricardo Ziviani, qemu-devel, aik, mdroth
On 04/21/2017 03:06 AM, Paolo Bonzini wrote:
>
>
> On 20/04/2017 18:03, Alex Williamson wrote:
>> On Thu, 20 Apr 2017 00:19:23 -0700
>> Richard Henderson <rth@twiddle.net> wrote:
>>
>>> On 04/19/2017 12:44 PM, Jose Ricardo Ziviani wrote:
>>>> This patchset has two patches:
>>>> [1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
>>>>
>>>> [2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
>>>>
>>>> Patches based on master.
>>>>
>>>> Jose Ricardo Ziviani (2):
>>>> vfio: Set MemoryRegionOps:max_access_size and min_access_size
>>>> vfio: enable 8-byte reads/writes to vfio
>>>>
>>>> hw/vfio/common.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>
>>> I think these patches need to be squashed to be bisectable.
>>
>> No, I think it's fine. The point of patch 1/2 is to indicate that the
>> hardware supports 8-byte accesses, which will still be broken into 2
>> 4-byte accesses because we don't yet set the implemented width beyond
>> the default. The important part is that the mutex will now group the 4
>> byte access pair together rather than letting them get re-ordered.
>> Patch 2/2 then implements native 8-byte access. I appreciate them
>> being separate for this subtle nuance, but maybe I'm not seeing the
>> same issue as you. Thanks,
> '
> I agree, the patches looks fine as is.
Sorry, I misread the first patch, thinking that indicating hw support for 8
byte required implementation of 8 byte.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
2017-04-21 10:06 ` Paolo Bonzini
2017-04-21 15:51 ` Richard Henderson
@ 2017-05-03 2:41 ` joserz
2017-05-03 2:52 ` Alex Williamson
1 sibling, 1 reply; 9+ messages in thread
From: joserz @ 2017-05-03 2:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Alex Williamson, Richard Henderson, qemu-devel, aik, mdroth
On Fri, Apr 21, 2017 at 12:06:15PM +0200, Paolo Bonzini wrote:
>
>
> On 20/04/2017 18:03, Alex Williamson wrote:
> > On Thu, 20 Apr 2017 00:19:23 -0700
> > Richard Henderson <rth@twiddle.net> wrote:
> >
> >> On 04/19/2017 12:44 PM, Jose Ricardo Ziviani wrote:
> >>> This patchset has two patches:
> >>> [1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
> >>>
> >>> [2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
> >>>
> >>> Patches based on master.
> >>>
> >>> Jose Ricardo Ziviani (2):
> >>> vfio: Set MemoryRegionOps:max_access_size and min_access_size
> >>> vfio: enable 8-byte reads/writes to vfio
> >>>
> >>> hw/vfio/common.c | 14 ++++++++++++++
> >>> 1 file changed, 14 insertions(+)
> >>>
> >>
> >> I think these patches need to be squashed to be bisectable.
> >
> > No, I think it's fine. The point of patch 1/2 is to indicate that the
> > hardware supports 8-byte accesses, which will still be broken into 2
> > 4-byte accesses because we don't yet set the implemented width beyond
> > the default. The important part is that the mutex will now group the 4
> > byte access pair together rather than letting them get re-ordered.
> > Patch 2/2 then implements native 8-byte access. I appreciate them
> > being separate for this subtle nuance, but maybe I'm not seeing the
> > same issue as you. Thanks,
> '
> I agree, the patches looks fine as is.
>
> Paolo
>
Hello!
Thank you all for your review but I have a quick question: is it ok for
merge? :)
Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic
2017-05-03 2:41 ` joserz
@ 2017-05-03 2:52 ` Alex Williamson
0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2017-05-03 2:52 UTC (permalink / raw)
To: joserz; +Cc: Paolo Bonzini, Richard Henderson, qemu-devel, aik, mdroth
On Tue, 2 May 2017 23:41:01 -0300
joserz@linux.vnet.ibm.com wrote:
> On Fri, Apr 21, 2017 at 12:06:15PM +0200, Paolo Bonzini wrote:
> >
> >
> > On 20/04/2017 18:03, Alex Williamson wrote:
> > > On Thu, 20 Apr 2017 00:19:23 -0700
> > > Richard Henderson <rth@twiddle.net> wrote:
> > >
> > >> On 04/19/2017 12:44 PM, Jose Ricardo Ziviani wrote:
> > >>> This patchset has two patches:
> > >>> [1] 8-byte writes to non-mapped MMIO are broken into pairs of 4-byte writes, this patch makes such pairs atomic.
> > >>>
> > >>> [2] Enable 8-byte accesses in vfio_region_write and vfio_region_read.
> > >>>
> > >>> Patches based on master.
> > >>>
> > >>> Jose Ricardo Ziviani (2):
> > >>> vfio: Set MemoryRegionOps:max_access_size and min_access_size
> > >>> vfio: enable 8-byte reads/writes to vfio
> > >>>
> > >>> hw/vfio/common.c | 14 ++++++++++++++
> > >>> 1 file changed, 14 insertions(+)
> > >>>
> > >>
> > >> I think these patches need to be squashed to be bisectable.
> > >
> > > No, I think it's fine. The point of patch 1/2 is to indicate that the
> > > hardware supports 8-byte accesses, which will still be broken into 2
> > > 4-byte accesses because we don't yet set the implemented width beyond
> > > the default. The important part is that the mutex will now group the 4
> > > byte access pair together rather than letting them get re-ordered.
> > > Patch 2/2 then implements native 8-byte access. I appreciate them
> > > being separate for this subtle nuance, but maybe I'm not seeing the
> > > same issue as you. Thanks,
> > '
> > I agree, the patches looks fine as is.
> >
> > Paolo
> >
>
> Hello!
>
> Thank you all for your review but I have a quick question: is it ok for
> merge? :)
Yes, I've got it in a local branch, I'll send a pull request this
week. Thanks,
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-03 2:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-19 19:44 [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 1/2] vfio: Set MemoryRegionOps:max_access_size and min_access_size Jose Ricardo Ziviani
2017-04-19 19:44 ` [Qemu-devel] [PATCH 2/2] vfio: enable 8-byte reads/writes to vfio Jose Ricardo Ziviani
2017-04-20 7:19 ` [Qemu-devel] [PATCH 0/2] VFIO: Make 8-byte accesses atomic Richard Henderson
2017-04-20 16:03 ` Alex Williamson
2017-04-21 10:06 ` Paolo Bonzini
2017-04-21 15:51 ` Richard Henderson
2017-05-03 2:41 ` joserz
2017-05-03 2:52 ` Alex Williamson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.