On 07/07/2015 08:21 PM, Thomas Huth wrote:
On Tue, 7 Jul 2015 20:05:25 +1000
Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
On 07/07/2015 05:23 PM, Thomas Huth wrote:
On Mon, 6 Jul 2015 12:11:09 +1000
Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
...
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8eacfd7..0c7ba8c 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -488,6 +488,76 @@ static void vfio_listener_release(VFIOContainer *container)
memory_listener_unregister(&container->iommu_data.type1.listener);
}
+static void vfio_ram_do_region(VFIOContainer *container,
+ MemoryRegionSection *section, unsigned long req)
+{
+ int ret;
+ struct vfio_iommu_spapr_register_memory reg = { .argsz = sizeof(reg) };
+
+ if (!memory_region_is_ram(section->mr) ||
+ memory_region_is_skip_dump(section->mr)) {
+ return;
+ }
+
+ if (unlikely((section->offset_within_region & (getpagesize() - 1)))) {
+ error_report("%s received unaligned region", __func__);
+ return;
+ }
+
+ reg.vaddr = (__u64) memory_region_get_ram_ptr(section->mr) +
We're in usespace here ... I think it would be better to use uint64_t
instead of the kernel-type __u64.
We are calling a kernel here - @reg is a kernel-defined struct.
If you grep for __u64 in the QEMU sources, you'll see that hardly
anybody is using this type - even if calling ioctls. So for
consistency, I'd really suggest to use uint64_t here.
I am not using it, I am packing data to a struct. So does vfio_dma_map()
already.
@@ -698,14 +768,18 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as)
container->iommu_data.type1.initialized = true;
- } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
+ } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) ||
+ ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) {
+ bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU);
That "!!" sounds somewhat wrong here. I think you either want to check
for "ioctl() == 1" (because only in this case you can be sure that v2
is supported), or you can simply omit the "!!" because you're 100% sure
that the ioctl only returns 0 or 1 (and never a negative error code).
The host kernel does not return an error on these ioctls, it returns 0 or
1. And "!!" is shorter than "(bool)". VFIO_CHECK_EXTENSION for Type1 does
exactly the same already.
Simply using nothing instead is even shorter than using "!!". The
compiler is smart enough to convert from 0 and 1 to bool.
"!!" is IMHO quite ugly and should only be used when it is really
necessary.
imho it is not but either way I'd rather follow the existing style,
especially if I do literally the same thing (checking IOMMU version).
Unless the original author tells me to convert all the existing occurences
of "!!" to "!=0" (or something like this) before I post new ones.
Alex, should I get rid of "!!"s in the patch?
@@ -717,19 +791,36 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as)
* when container fd is closed so we do not call it explicitly
* in this file.
*/
- ret = ioctl(fd, VFIO_IOMMU_ENABLE);
- if (ret) {
- error_report("vfio: failed to enable container: %m");
- ret = -errno;
- goto free_container_exit;
+ if (!v2) {
+ ret = ioctl(fd, VFIO_IOMMU_ENABLE);
+ if (ret) {
+ error_report("vfio: failed to enable container: %m");
+ ret = -errno;
+ goto free_container_exit;
+ }
}
container->iommu_data.type1.listener = vfio_memory_listener;
- container->iommu_data.release = vfio_listener_release;
-
memory_listener_register(&container->iommu_data.type1.listener,
container->space->as);
+ if (!v2) {
+ container->iommu_data.release = vfio_listener_release;
+ } else {
+ container->iommu_data.release = vfio_spapr_listener_release_v2;
+ container->iommu_data.register_listener =
+ vfio_ram_memory_listener;
+ memory_listener_register(&container->iommu_data.register_listener,
+ &address_space_memory);
+
+ if (container->iommu_data.ram_reg_error) {
+ error_report("vfio: RAM memory listener initialization failed for
container");
Line > 80 columns?
afaik user visible strings are an exception in QEMU and kernel.
You're right for the kernel, but AFAIK QEMU (currently still) has a
hard limit at 80 columns.
This is not an error, this is warning and in fact nobody is enforcing this
(and this is a good thing) and for example VFIO already has longer lines.
--
Alexey
