Re: [PATCH] hw/acpi/piix4: Rename piix4_pm_add_propeties() to piix4_pm_add_properties()
Greg Kurz 于2020年10月3日周六 上午12:07写道：
>
> Signed-off-by: Greg Kurz
Reviewed-by: Li Qiang
> ---
> hw/acpi/piix4.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 894d357f8c35..67a1ea41914f 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -437,7 +437,7 @@ static void piix4_pm_machine_ready(Notifier *n, void
> *opaque)
> (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
> }
>
> -static void piix4_pm_add_propeties(PIIX4PMState *s)
> +static void piix4_pm_add_properties(PIIX4PMState *s)
> {
> static const uint8_t acpi_enable_cmd = ACPI_ENABLE;
> static const uint8_t acpi_disable_cmd = ACPI_DISABLE;
> @@ -509,7 +509,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> pci_get_bus(dev), s);
> qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s));
>
> -piix4_pm_add_propeties(s);
> +piix4_pm_add_properties(s);
> }
>
> I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>
>
>
[PATCH 5/6] docs/devel/qom: Remove usage of
is not valid reST syntax. Function @argument references don't need additional markup, so just remove . Constants were changed to use reST ``code`` syntax Signed-off-by: Eduardo Habkost --- include/qom/object.h | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index e738dfc6744..16c9bcdf3b0 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -1256,7 +1256,7 @@ char *object_property_get_str(Object *obj, const char *name, * Writes an object's canonical path to a property. * * If the link property was created with - * OBJ_PROP_LINK_STRONG bit, the old target object is + * ``OBJ_PROP_LINK_STRONG`` bit, the old target object is * unreferenced, and a reference is added to the new target object. * * Returns: %true on success, %false on failure. @@ -1603,16 +1603,16 @@ void object_property_allow_set_link(const Object *obj, const char *name, * * Links form the graph in the object model. * - * The @check() callback is invoked when + * The @check() callback is invoked when * object_property_set_link() is called and can raise an error to prevent the - * link being set. If @check is NULL, the property is read-only + * link being set. If @check is NULL, the property is read-only * and cannot be set. * * Ownership of the pointer that @child points to is transferred to the - * link property. The reference count for *@child is + * link property. The reference count for *@child is * managed by the property from after the function returns till the * property is deleted with object_property_del(). If the - * @flags OBJ_PROP_LINK_STRONG bit is set, + * @flags ``OBJ_PROP_LINK_STRONG`` bit is set, * the reference count is decremented when the property is deleted or * modified. * @@ -1823,7 +1823,7 @@ ObjectProperty *object_class_property_add_uint64_ptr(ObjectClass *klass, * Add an alias for a property on an object. This function will add a property * of the same type as the forwarded property. * - * The caller must ensure that @target_obj stays alive as long as + * The caller must ensure that @target_obj stays alive as long as * this property exists. In the case of a child object or an alias on the same * object this will be the case. For aliases to other objects the caller is * responsible for taking a reference. -- 2.26.2
[PATCH 3/6] docs/devel/qom: Fix indentation of code blocks
Some code blocks had one extra space, fix that.
Signed-off-by: Eduardo Habkost
---
docs/devel/qom.rst | 76 +++---
1 file changed, 38 insertions(+), 38 deletions(-)
diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index c4857d95c8e..a47e1b9a239 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -284,28 +284,28 @@ in the header file:
.. code-block:: c
:caption: Declaring a simple type
-OBJECT_DECLARE_SIMPLE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+ OBJECT_DECLARE_SIMPLE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
This is equivalent to the following:
.. code-block:: c
:caption: Expansion from declaring a simple type
-typedef struct MyDevice MyDevice;
-typedef struct MyDeviceClass MyDeviceClass;
+ typedef struct MyDevice MyDevice;
+ typedef struct MyDeviceClass MyDeviceClass;
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(MyDeviceClass, object_unref)
+ G_DEFINE_AUTOPTR_CLEANUP_FUNC(MyDeviceClass, object_unref)
-#define MY_DEVICE_GET_CLASS(void *obj) \
-OBJECT_GET_CLASS(MyDeviceClass, obj, TYPE_MY_DEVICE)
-#define MY_DEVICE_CLASS(void *klass) \
-OBJECT_CLASS_CHECK(MyDeviceClass, klass, TYPE_MY_DEVICE)
-#define MY_DEVICE(void *obj)
-OBJECT_CHECK(MyDevice, obj, TYPE_MY_DEVICE)
+ #define MY_DEVICE_GET_CLASS(void *obj) \
+ OBJECT_GET_CLASS(MyDeviceClass, obj, TYPE_MY_DEVICE)
+ #define MY_DEVICE_CLASS(void *klass) \
+ OBJECT_CLASS_CHECK(MyDeviceClass, klass, TYPE_MY_DEVICE)
+ #define MY_DEVICE(void *obj)
+ OBJECT_CHECK(MyDevice, obj, TYPE_MY_DEVICE)
-struct MyDeviceClass {
-DeviceClass parent_class;
-};
+ struct MyDeviceClass {
+ DeviceClass parent_class;
+ };
The 'struct MyDevice' needs to be declared separately.
If the type requires virtual functions to be declared in the class
@@ -319,33 +319,33 @@ In the simple case the OBJECT_DEFINE_TYPE macro is
suitable:
.. code-block:: c
:caption: Defining a simple type
-OBJECT_DEFINE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+ OBJECT_DEFINE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
This is equivalent to the following:
.. code-block:: c
:caption: Expansion from defining a simple type
-static void my_device_finalize(Object *obj);
-static void my_device_class_init(ObjectClass *oc, void *data);
-static void my_device_init(Object *obj);
-
-static const TypeInfo my_device_info = {
-.parent = TYPE_DEVICE,
-.name = TYPE_MY_DEVICE,
-.instance_size = sizeof(MyDevice),
-.instance_init = my_device_init,
-.instance_finalize = my_device_finalize,
-.class_size = sizeof(MyDeviceClass),
-.class_init = my_device_class_init,
-};
-
-static void
-my_device_register_types(void)
-{
-type_register_static(_device_info);
-}
-type_init(my_device_register_types);
+ static void my_device_finalize(Object *obj);
+ static void my_device_class_init(ObjectClass *oc, void *data);
+ static void my_device_init(Object *obj);
+
+ static const TypeInfo my_device_info = {
+ .parent = TYPE_DEVICE,
+ .name = TYPE_MY_DEVICE,
+ .instance_size = sizeof(MyDevice),
+ .instance_init = my_device_init,
+ .instance_finalize = my_device_finalize,
+ .class_size = sizeof(MyDeviceClass),
+ .class_init = my_device_class_init,
+ };
+
+ static void
+ my_device_register_types(void)
+ {
+ type_register_static(_device_info);
+ }
+ type_init(my_device_register_types);
This is sufficient to get the type registered with the type
system, and the three standard methods now need to be implemented
@@ -358,9 +358,9 @@ This accepts an array of interface type names.
.. code-block:: c
:caption: Defining a simple type implementing interfaces
-OBJECT_DEFINE_TYPE_WITH_INTERFACES(MyDevice, my_device,
- MY_DEVICE, DEVICE,
- { TYPE_USER_CREATABLE }, { NULL })
+ OBJECT_DEFINE_TYPE_WITH_INTERFACES(MyDevice, my_device,
+ MY_DEVICE, DEVICE,
+ { TYPE_USER_CREATABLE }, { NULL })
If the type is not intended to be instantiated, then then
the OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
@@ -368,7 +368,7 @@ the OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
.. code-block:: c
:caption: Defining a simple abstract type
-OBJECT_DEFINE_ABSTRACT_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+ OBJECT_DEFINE_ABSTRACT_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
--
2.26.2
[PATCH 6/6] docs/devel/qom: Avoid long lines
Long code lines don't look good in the rendered documents, make
them shorter.
Signed-off-by: Eduardo Habkost
---
docs/devel/qom.rst | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index 0c610e20d62..42d0dc4f4da 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -284,7 +284,8 @@ in the header file:
.. code-block:: c
:caption: Declaring a simple type
- OBJECT_DECLARE_SIMPLE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+ OBJECT_DECLARE_SIMPLE_TYPE(MyDevice, my_device,
+ MY_DEVICE, DEVICE)
This is equivalent to the following:
@@ -360,7 +361,8 @@ This accepts an array of interface type names.
OBJECT_DEFINE_TYPE_WITH_INTERFACES(MyDevice, my_device,
MY_DEVICE, DEVICE,
- { TYPE_USER_CREATABLE }, { NULL })
+ { TYPE_USER_CREATABLE },
+ { NULL })
If the type is not intended to be instantiated, then then
the OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
@@ -368,7 +370,8 @@ the OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
.. code-block:: c
:caption: Defining a simple abstract type
- OBJECT_DEFINE_ABSTRACT_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+ OBJECT_DEFINE_ABSTRACT_TYPE(MyDevice, my_device,
+ MY_DEVICE, DEVICE)
--
2.26.2
[PATCH 4/6] docs/devel/qom: Use *emphasis* for emphasis
is not valid reST syntax. Signed-off-by: Eduardo Habkost --- docs/devel/qom.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst index a47e1b9a239..0c610e20d62 100644 --- a/docs/devel/qom.rst +++ b/docs/devel/qom.rst @@ -174,17 +174,17 @@ dynamically cast it to an object that implements the interface. Methods === -A method is a function within the namespace scope of +A *method* is a function within the namespace scope of a class. It usually operates on the object instance by passing it as a strongly-typed first argument. If it does not operate on an object instance, it is dubbed -class method. +*class method*. Methods cannot be overloaded. That is, the #ObjectClass and method name uniquely identity the function to be called; the signature does not vary except for trailing varargs. -Methods are always virtual. Overriding a method in +Methods are always *virtual*. Overriding a method in #TypeInfo.class_init of a subclass leads to any user of the class obtained via OBJECT_GET_CLASS() accessing the overridden function. The original function is not automatically invoked. It is the responsibility -- 2.26.2
[PATCH 1/6] qom: Fix DECLARE_*CHECKER documentation
Correct copy/paste mistake in the DECLARE_INSTANCE_CHECKER and DECLARE_CLASS_CHECKERS documentation. Signed-off-by: Eduardo Habkost --- include/qom/object.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/qom/object.h b/include/qom/object.h index 27aaa67e63f..e738dfc6744 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -170,7 +170,7 @@ struct Object * Direct usage of this macro should be avoided, and the complete * OBJECT_DECLARE_TYPE macro is recommended instead. * - * This macro will provide the three standard type cast functions for a + * This macro will provide the the instance type cast functions for a * QOM type. */ #define DECLARE_INSTANCE_CHECKER(InstanceType, OBJ_NAME, TYPENAME) \ @@ -187,7 +187,7 @@ struct Object * Direct usage of this macro should be avoided, and the complete * OBJECT_DECLARE_TYPE macro is recommended instead. * - * This macro will provide the three standard type cast functions for a + * This macro will provide the class type cast functions for a * QOM type. */ #define DECLARE_CLASS_CHECKERS(ClassType, OBJ_NAME, TYPENAME) \ -- 2.26.2
[PATCH 0/6] qom documentation fixes
A few fixes to the QOM documentation in docs/devel/qom.rst and include/qom/object.h. Eduardo Habkost (6): qom: Fix DECLARE_*CHECKER documentation docs/devel/qom: Fix indentation of bulleted list docs/devel/qom: Fix indentation of code blocks docs/devel/qom: Use *emphasis* for emphasis docs/devel/qom: Remove usage of docs/devel/qom: Avoid long lines docs/devel/qom.rst | 91 +++- include/qom/object.h | 16 2 files changed, 55 insertions(+), 52 deletions(-) -- 2.26.2
[PATCH 2/6] docs/devel/qom: Fix indentation of bulleted list
The list was incorrectly parsed as a literal block due to indentation. Signed-off-by: Eduardo Habkost --- docs/devel/qom.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst index 0b943b2a1a8..c4857d95c8e 100644 --- a/docs/devel/qom.rst +++ b/docs/devel/qom.rst @@ -8,9 +8,9 @@ The QEMU Object Model provides a framework for registering user creatable types and instantiating objects from those types. QOM provides the following features: - - System for dynamically registering types - - Support for single-inheritance of types - - Multiple inheritance of stateless interfaces +- System for dynamically registering types +- Support for single-inheritance of types +- Multiple inheritance of stateless interfaces .. code-block:: c :caption: Creating a minimal type -- 2.26.2
[PATCH 5/5] kernel-doc: Remove $decl_type='type name' hack
The $decl_type='type name' hack makes it impossible to document
macros with uppercase names (e.g. most of the macros in
object.h).
Now that we have explicitly tagged the struct and typedef doc
comments in memory.h and object.h, we don't need that hack
anymore. This will make the documentation for the macros in
object.h finally be rendered as expected.
Signed-off-by: Eduardo Habkost
---
scripts/kernel-doc | 12 +---
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 57b911ff174..0ff62bb6a2d 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1064,14 +1064,6 @@ sub output_blockhead {
sub dump_declaration($$) {
no strict 'refs';
my ($prototype, $file) = @_;
-if ($decl_type eq 'type name') {
- if ($prototype =~ /^(enum|struct|union)\s+/) {
- $decl_type = $1;
- } else {
- return;
- }
-}
-
my $func = "dump_" . $decl_type;
&$func(@_);
}
@@ -1928,9 +1920,7 @@ sub process_name($$) {
++$warnings;
}
- if ($identifier =~ m/^[A-Z]/) {
- $decl_type = 'type name';
- } elsif ($identifier =~ m/^struct\b/) {
+ if ($identifier =~ m/^struct\b/) {
$decl_type = 'struct';
} elsif ($identifier =~ m/^union\b/) {
$decl_type = 'union';
--
2.26.2
[PATCH 0/5] kernel-doc ixes
Among other fixes in kernel-doc, this series get rid of QEMU-specific $decl_type='type name' hack in kernel-doc, because it made it impossible to document macros with names starting with uppercase letters (like most of the macros at qom/object.h). Eduardo Habkost (5): kernel-doc: Handle function typedefs that return pointers kernel-doc: Handle function typedefs without asterisks qom: Explicitly tag doc comments for typedefs and structs memory: Explicitly tag doc comments for structs kernel-doc: Remove $decl_type='type name' hack include/exec/memory.h | 6 +++--- include/qom/object.h | 22 +++--- scripts/kernel-doc| 16 +++- 3 files changed, 17 insertions(+), 27 deletions(-) -- 2.26.2
[PATCH 4/5] memory: Explicitly tag doc comments for structs
This will allow us to remove the QEMU-specific
$decl_type='type name' hack from the kernel-doc script.
Signed-off-by: Eduardo Habkost
---
include/exec/memory.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index dee09851622..622207bde12 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -443,7 +443,7 @@ struct IOMMUMemoryRegion {
QLIST_FOREACH((n), &(mr)->iommu_notify, node)
/**
- * MemoryListener: callbacks structure for updates to the physical memory map
+ * struct MemoryListener: callbacks structure for updates to the physical
memory map
*
* Allows a component to adjust to changes in the guest-visible memory map.
* Use with memory_listener_register() and memory_listener_unregister().
@@ -681,7 +681,7 @@ struct MemoryListener {
};
/**
- * AddressSpace: describes a mapping of addresses to #MemoryRegion objects
+ * struct AddressSpace: describes a mapping of addresses to #MemoryRegion
objects
*/
struct AddressSpace {
/* private: */
@@ -721,7 +721,7 @@ static inline FlatView
*address_space_to_flatview(AddressSpace *as)
/**
- * MemoryRegionSection: describes a fragment of a #MemoryRegion
+ * struct MemoryRegionSection: describes a fragment of a #MemoryRegion
*
* @mr: the region, or %NULL if empty
* @fv: the flat view of the address space the region is mapped in
--
2.26.2
[PATCH 2/5] kernel-doc: Handle function typedefs without asterisks
Example of typedef that was not parsed by kernel-doc:
typedef void (ObjectUnparent)(Object *obj);
Signed-off-by: Eduardo Habkost
---
scripts/kernel-doc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 57a4a72970f..57b911ff174 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1318,7 +1318,7 @@ sub dump_typedef($$) {
$x =~ s@/\*.*?\*/@@gos;# strip comments.
# Parse function prototypes
-if ($x =~ /typedef\s+(\w+\s*\**)\s*\(\*\s*(\w\S+)\s*\)\s*\((.*)\);/ ||
+if ($x =~ /typedef\s+(\w+\s*\**)\s*\(\*?\s*(\w\S+)\s*\)\s*\((.*)\);/ ||
$x =~ /typedef\s+(\w+\s*\**)\s*(\w\S+)\s*\s*\((.*)\);/) {
# Function typedefs
--
2.26.2
[PATCH 3/5] qom: Explicitly tag doc comments for typedefs and structs
If we explicitly indicate we are documenting a typedef or a
struct, we'll be able to remove the $decl_type='type name' hack
from kernel-doc.
Signed-off-by: Eduardo Habkost
---
include/qom/object.h | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 27aaa67e63f..32bdf4cce53 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -31,7 +31,7 @@ typedef struct InterfaceInfo InterfaceInfo;
typedef struct ObjectProperty ObjectProperty;
/**
- * ObjectPropertyAccessor:
+ * typedef ObjectPropertyAccessor:
* @obj: the object that owns the property
* @v: the visitor that contains the property data
* @name: the name of the property
@@ -47,7 +47,7 @@ typedef void (ObjectPropertyAccessor)(Object *obj,
Error **errp);
/**
- * ObjectPropertyResolve:
+ * typedef ObjectPropertyResolve:
* @obj: the object that owns the property
* @opaque: the opaque registered with the property
* @part: the name of the property
@@ -66,7 +66,7 @@ typedef Object *(ObjectPropertyResolve)(Object *obj,
const char *part);
/**
- * ObjectPropertyRelease:
+ * typedef ObjectPropertyRelease:
* @obj: the object that owns the property
* @name: the name of the property
* @opaque: the opaque registered with the property
@@ -78,7 +78,7 @@ typedef void (ObjectPropertyRelease)(Object *obj,
void *opaque);
/**
- * ObjectPropertyInit:
+ * typedef ObjectPropertyInit:
* @obj: the object that owns the property
* @prop: the property to set
*
@@ -101,7 +101,7 @@ struct ObjectProperty
};
/**
- * ObjectUnparent:
+ * typedef ObjectUnparent:
* @obj: the object that is being removed from the composition tree
*
* Called when an object is being removed from the QOM composition tree.
@@ -110,7 +110,7 @@ struct ObjectProperty
typedef void (ObjectUnparent)(Object *obj);
/**
- * ObjectFree:
+ * typedef ObjectFree:
* @obj: the object being freed
*
* Called when an object's last reference is removed.
@@ -120,7 +120,7 @@ typedef void (ObjectFree)(void *obj);
#define OBJECT_CLASS_CAST_CACHE 4
/**
- * ObjectClass:
+ * struct ObjectClass:
*
* The base for all classes. The only thing that #ObjectClass contains is an
* integer type handle.
@@ -140,7 +140,7 @@ struct ObjectClass
};
/**
- * Object:
+ * struct Object:
*
* The base for all objects. The first member of this object is a pointer to
* a #ObjectClass. Since C guarantees that the first member of a structure
@@ -370,7 +370,7 @@ struct Object
true, { NULL })
/**
- * TypeInfo:
+ * struct TypeInfo:
* @name: The name of the type.
* @parent: The name of the parent type.
* @instance_size: The size of the object (derivative of #Object). If
@@ -496,7 +496,7 @@ struct TypeInfo
OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
/**
- * InterfaceInfo:
+ * struct InterfaceInfo:
* @type: The name of the interface.
*
* The information associated with an interface.
@@ -506,7 +506,7 @@ struct InterfaceInfo {
};
/**
- * InterfaceClass:
+ * struct InterfaceClass:
* @parent_class: the base class
*
* The class for all interfaces. Subclasses of this class should only add
--
2.26.2
[PATCH 1/5] kernel-doc: Handle function typedefs that return pointers
One example that was not being parsed correctly by kernel-doc is:
typedef Object *(ObjectPropertyResolve)(Object *obj,
void *opaque,
const char *part);
Signed-off-by: Eduardo Habkost
---
scripts/kernel-doc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 40ad782e342..57a4a72970f 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1318,8 +1318,8 @@ sub dump_typedef($$) {
$x =~ s@/\*.*?\*/@@gos;# strip comments.
# Parse function prototypes
-if ($x =~ /typedef\s+(\w+)\s*\(\*\s*(\w\S+)\s*\)\s*\((.*)\);/ ||
- $x =~ /typedef\s+(\w+)\s*(\w\S+)\s*\s*\((.*)\);/) {
+if ($x =~ /typedef\s+(\w+\s*\**)\s*\(\*\s*(\w\S+)\s*\)\s*\((.*)\);/ ||
+ $x =~ /typedef\s+(\w+\s*\**)\s*(\w\S+)\s*\s*\((.*)\);/) {
# Function typedefs
$return_type = $1;
--
2.26.2
Re: [PATCH v2 2/2] vhost: Don't call log_access_ok() when using IOTLB
On 2020/9/30 上午12:30, Greg Kurz wrote:
When the IOTLB device is enabled, the log_guest_addr that is passed by
userspace to the VHOST_SET_VRING_ADDR ioctl, and which is then written
to vq->log_addr, is a GIOVA. All writes to this address are translated
by log_user() to writes to an HVA, and then ultimately logged through
the corresponding GPAs in log_write_hva(). No logging will ever occur
with vq->log_addr in this case. It is thus wrong to pass vq->log_addr
and log_guest_addr to log_access_vq() which assumes they are actual
GPAs.
Introduce a new vq_log_used_access_ok() helper that only checks accesses
to the log for the used structure when there isn't an IOTLB device around.
Signed-off-by: Greg Kurz
---
drivers/vhost/vhost.c | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c3b49975dc28..5996e32fa818 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1370,6 +1370,20 @@ bool vhost_log_access_ok(struct vhost_dev *dev)
}
EXPORT_SYMBOL_GPL(vhost_log_access_ok);
+static bool vq_log_used_access_ok(struct vhost_virtqueue *vq,
+ void __user *log_base,
+ bool log_used,
+ u64 log_addr,
+ size_t log_size)
+{
+ /* If an IOTLB device is present, log_addr is a GIOVA that
+* will never be logged by log_used(). */
+ if (vq->iotlb)
+ return true;
+
+ return !log_used || log_access_ok(log_base, log_addr, log_size);
+}
+
/* Verify access for write logging. */
/* Caller should have vq mutex and device mutex */
static bool vq_log_access_ok(struct vhost_virtqueue *vq,
@@ -1377,8 +1391,8 @@ static bool vq_log_access_ok(struct vhost_virtqueue *vq,
{
return vq_memory_access_ok(log_base, vq->umem,
vhost_has_feature(vq, VHOST_F_LOG_ALL)) &&
- (!vq->log_used || log_access_ok(log_base, vq->log_addr,
- vhost_get_used_size(vq, vq->num)));
+ vq_log_used_access_ok(vq, log_base, vq->log_used, vq->log_addr,
+ vhost_get_used_size(vq, vq->num));
}
/* Can we start vq? */
@@ -1517,8 +1531,9 @@ static long vhost_vring_set_addr(struct vhost_dev *d,
return -EINVAL;
/* Also validate log access for used ring if enabled. */
- if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
- !log_access_ok(vq->log_base, a.log_guest_addr,
+ if (!vq_log_used_access_ok(vq, vq->log_base,
+ a.flags & (0x1 << VHOST_VRING_F_LOG),
+ a.log_guest_addr,
sizeof *vq->used +
vq->num * sizeof *vq->used->ring))
It looks to me that we should use vhost_get_used_size() which takes
event into account.
Any reason that we can't reuse vq_log_access_ok() here?
Thanks
return -EINVAL;
Re: [PATCH v2 1/2] vhost: Don't call access_ok() when using IOTLB
On 2020/9/30 上午12:30, Greg Kurz wrote:
When the IOTLB device is enabled, the vring addresses we get
from userspace are GIOVAs. It is thus wrong to pass them down
to access_ok() which only takes HVAs.
Access validation is done at prefetch time with IOTLB. Teach
vq_access_ok() about that by moving the (vq->iotlb) check
from vhost_vq_access_ok() to vq_access_ok(). This prevents
vhost_vring_set_addr() to fail when verifying the accesses.
No behavior change for vhost_vq_access_ok().
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1883084
Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
Cc: jasow...@redhat.com
CC: sta...@vger.kernel.org # 4.14+
Signed-off-by: Greg Kurz
---
drivers/vhost/vhost.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b45519ca66a7..c3b49975dc28 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1290,6 +1290,11 @@ static bool vq_access_ok(struct vhost_virtqueue *vq,
unsigned int num,
vring_used_t __user *used)
{
+ /* If an IOTLB device is present, the vring addresses are
+* GIOVAs. Access validation occurs at prefetch time. */
+ if (vq->iotlb)
+ return true;
+
return access_ok(desc, vhost_get_desc_size(vq, num)) &&
access_ok(avail, vhost_get_avail_size(vq, num)) &&
access_ok(used, vhost_get_used_size(vq, num));
@@ -1383,10 +1388,6 @@ bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
if (!vq_log_access_ok(vq, vq->log_base))
return false;
- /* Access validation occurs at prefetch time with IOTLB */
- if (vq->iotlb)
- return true;
-
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
}
EXPORT_SYMBOL_GPL(vhost_vq_access_ok);
Acked-by: Jason Wang
Thanks
Re: [PULL v2 00/11] capstone + disassembler patch queue
On 10/2/20 3:37 PM, Peter Maydell wrote: > Meson warning on the BSDs: > > Configuring sparc-bsd-user-config-target.h using configuration > Configuring sparc64-bsd-user-config-target.h using configuration > Configuring x86_64-bsd-user-config-target.h using configuration > Did not find CMake 'cmake' > Found CMake: NO > Run-time dependency capstone found: NO (tried pkgconfig and cmake) > ../src/meson.build:753: WARNING: Trying to compare values of different > types (bool, str) using ==. > The result of this is undefined and will become a hard error in a > future Meson release. > Configuring config-host.h using configuration > Program scripts/hxtool found: YES > Program scripts/shaderinclude.pl found: YES > Program scripts/qapi-gen.py found: YES > Program scripts/qemu-version.sh found: YES > Run-time dependency threads found: YES > Program keycodemapdb/tools/keymap-gen found: YES > Program scripts/decodetree.py found: YES > > Warning from ppc64be box (gcc compilefarm one): > > Configuring sh4eb-linux-user-config-target.h using configuration > Configuring sparc-linux-user-config-target.h using configuration > Configuring sparc32plus-linux-user-config-target.h using configuration > Configuring sparc64-linux-user-config-target.h using configuration > Configuring x86_64-linux-user-config-target.h using configuration > Configuring xtensa-linux-user-config-target.h using configuration > Configuring xtensaeb-linux-user-config-target.h using configuration > Found CMake: /usr/bin/cmake (2.8.12.2) > WARNING: The version of CMake /usr/bin/cmake is 2.8.12.2 but version >> =3.4 is required > Run-time dependency capstone found: NO (tried pkgconfig and cmake) > Configuring capstone-defs.h using configuration > Configuring config-host.h using configuration > > We shouldn't be looking for or using cmake at all. Huh. I hadn't noticed that before. I think it's coming from meson internally, but I'm not sure what's causing. Certainly it is not something I asked for. So it's a warning. Does the build succeed? r~
[Bug 1898011] Re: mmap MAP_NORESERVE of 2^42 bytes consumes 16Gb of actual RAM
Without actually looking, an allocation of 2**42 (4PB) requires
2**30 (1G) pages, and thus 1G page table entries, so 16GB memory
allocation sounds about right for qemu's internal page table allocation.
We need to change data structures for representing guest memory,
probably akin to the kernel's VMAs.
** Changed in: qemu
Status: New => Confirmed
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1898011
Title:
mmap MAP_NORESERVE of 2^42 bytes consumes 16Gb of actual RAM
Status in QEMU:
Confirmed
Bug description:
Run this program:
#include
#include
int main() {
for (int i = 30; i <= 44; i++) {
fprintf(stderr, "trying 2**%d\n", i);
mmap((void*)0x6000,1ULL << i,
PROT_NONE,
MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0);
}
}
(tried qemu-x86_64 and qemu-aarch64, 4.2.1 and trunk/5.1.50)
On each iteration qemu will consume 2x more physical RAM,
e.g. when mapping 2^42 it will have RSS of 16Gb.
On normal linux it works w/o consuming much RAM, due to MAP_NORESERVE.
Also: qemu -strace prints 0 instead of the correct size starting from
size=2^32
and prints -2147483648 for size=2^31.
mmap(0x6000,1073741824,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0)
= 0x6000
mmap(0x6000,-2147483648,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0)
= 0x6000
mmap(0x6000,0,PROT_NONE,MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED|MAP_NORESERVE,-1,0)
= 0x6000
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1898011/+subscriptions
[PATCH v10 8/8] tests/tcg/aarch64: Add bti smoke test
This test requires gcc 10 for -mbranch-protection=standard.
Reviewed-by: Peter Maydell
Signed-off-by: Richard Henderson
---
v9: Expect and require gcc 10.
---
tests/tcg/aarch64/bti-1.c | 62 +++
tests/tcg/aarch64/bti-crt.inc.c | 51 +
tests/tcg/aarch64/Makefile.target | 7
tests/tcg/configure.sh| 4 ++
4 files changed, 124 insertions(+)
create mode 100644 tests/tcg/aarch64/bti-1.c
create mode 100644 tests/tcg/aarch64/bti-crt.inc.c
diff --git a/tests/tcg/aarch64/bti-1.c b/tests/tcg/aarch64/bti-1.c
new file mode 100644
index 00..61924f0d7a
--- /dev/null
+++ b/tests/tcg/aarch64/bti-1.c
@@ -0,0 +1,62 @@
+/*
+ * Branch target identification, basic notskip cases.
+ */
+
+#include "bti-crt.inc.c"
+
+static void skip2_sigill(int sig, siginfo_t *info, ucontext_t *uc)
+{
+uc->uc_mcontext.pc += 8;
+uc->uc_mcontext.pstate = 1;
+}
+
+#define NOP "nop"
+#define BTI_N "hint #32"
+#define BTI_C "hint #34"
+#define BTI_J "hint #36"
+#define BTI_JC"hint #38"
+
+#define BTYPE_1(DEST) \
+asm("mov %0,#1; adr x16, 1f; br x16; 1: " DEST "; mov %0,#0" \
+: "=r"(skipped) : : "x16")
+
+#define BTYPE_2(DEST) \
+asm("mov %0,#1; adr x16, 1f; blr x16; 1: " DEST "; mov %0,#0" \
+: "=r"(skipped) : : "x16", "x30")
+
+#define BTYPE_3(DEST) \
+asm("mov %0,#1; adr x15, 1f; br x15; 1: " DEST "; mov %0,#0" \
+: "=r"(skipped) : : "x15")
+
+#define TEST(WHICH, DEST, EXPECT) \
+do { WHICH(DEST); fail += skipped ^ EXPECT; } while (0)
+
+
+int main()
+{
+int fail = 0;
+int skipped;
+
+/* Signal-like with SA_SIGINFO. */
+signal_info(SIGILL, skip2_sigill);
+
+TEST(BTYPE_1, NOP, 1);
+TEST(BTYPE_1, BTI_N, 1);
+TEST(BTYPE_1, BTI_C, 0);
+TEST(BTYPE_1, BTI_J, 0);
+TEST(BTYPE_1, BTI_JC, 0);
+
+TEST(BTYPE_2, NOP, 1);
+TEST(BTYPE_2, BTI_N, 1);
+TEST(BTYPE_2, BTI_C, 0);
+TEST(BTYPE_2, BTI_J, 1);
+TEST(BTYPE_2, BTI_JC, 0);
+
+TEST(BTYPE_3, NOP, 1);
+TEST(BTYPE_3, BTI_N, 1);
+TEST(BTYPE_3, BTI_C, 1);
+TEST(BTYPE_3, BTI_J, 0);
+TEST(BTYPE_3, BTI_JC, 0);
+
+return fail;
+}
diff --git a/tests/tcg/aarch64/bti-crt.inc.c b/tests/tcg/aarch64/bti-crt.inc.c
new file mode 100644
index 00..47805f4e35
--- /dev/null
+++ b/tests/tcg/aarch64/bti-crt.inc.c
@@ -0,0 +1,51 @@
+/*
+ * Minimal user-environment for testing BTI.
+ *
+ * Normal libc is not (yet) built with BTI support enabled,
+ * and so could generate a BTI TRAP before ever reaching main.
+ */
+
+#include
+#include
+#include
+#include
+
+int main(void);
+
+void _start(void)
+{
+exit(main());
+}
+
+void exit(int ret)
+{
+register int x0 __asm__("x0") = ret;
+register int x8 __asm__("x8") = __NR_exit;
+
+asm volatile("svc #0" : : "r"(x0), "r"(x8));
+__builtin_unreachable();
+}
+
+/*
+ * Irritatingly, the user API struct sigaction does not match the
+ * kernel API struct sigaction. So for simplicity, isolate the
+ * kernel ABI here, and make this act like signal.
+ */
+void signal_info(int sig, void (*fn)(int, siginfo_t *, ucontext_t *))
+{
+struct kernel_sigaction {
+void (*handler)(int, siginfo_t *, ucontext_t *);
+unsigned long flags;
+unsigned long restorer;
+unsigned long mask;
+} sa = { fn, SA_SIGINFO, 0, 0 };
+
+register int x0 __asm__("x0") = sig;
+register void *x1 __asm__("x1") =
+register void *x2 __asm__("x2") = 0;
+register int x3 __asm__("x3") = sizeof(unsigned long);
+register int x8 __asm__("x8") = __NR_rt_sigaction;
+
+asm volatile("svc #0"
+ : : "r"(x0), "r"(x1), "r"(x2), "r"(x3), "r"(x8) : "memory");
+}
diff --git a/tests/tcg/aarch64/Makefile.target
b/tests/tcg/aarch64/Makefile.target
index e7249915e7..491683e91d 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -25,6 +25,13 @@ run-pauth-%: QEMU_OPTS += -cpu max
run-plugin-pauth-%: QEMU_OPTS += -cpu max
endif
+# BTI Tests
+ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_ARMV8_BTI),)
+AARCH64_TESTS += bti-1
+bti-%: CFLAGS += -mbranch-protection=standard
+bti-%: LDFLAGS += -nostdlib
+endif
+
# Semihosting smoke test for linux-user
AARCH64_TESTS += semihosting
run-semihosting: semihosting
diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
index be51bdb5a4..e1b70e25f2 100755
--- a/tests/tcg/configure.sh
+++ b/tests/tcg/configure.sh
@@ -240,6 +240,10 @@ for target in $target_list; do
-march=armv8.3-a -o $TMPE $TMPC; then
echo "CROSS_CC_HAS_ARMV8_3=y" >> $config_target_mak
fi
+if do_compiler "$target_compiler" $target_compiler_cflags \
+ -mbranch-protection=standard -o $TMPE $TMPC; then
+echo "CROSS_CC_HAS_ARMV8_BTI=y" >> $config_target_mak
+fi
;;
esac
--
2.25.1
[PATCH v10 7/8] linux-user/elfload: Parse NT_GNU_PROPERTY_TYPE_0 notes
For aarch64, this includes the GNU_PROPERTY_AARCH64_FEATURE_1_BTI bit,
which indicates that the image should be mapped with guarded pages.
Signed-off-by: Richard Henderson
---
v9: Only map the startup executable with BTI; anything else must be
handled by the interpreter.
v10: Split out preparatory patches (pmm).
---
linux-user/qemu.h| 4 +++
linux-user/elfload.c | 68 ++--
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 941ca99722..534753ca12 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -61,6 +61,10 @@ struct image_info {
abi_ulong interpreter_loadmap_addr;
abi_ulong interpreter_pt_dynamic_addr;
struct image_info *other_info;
+
+/* For target-specific processing of NT_GNU_PROPERTY_TYPE_0. */
+uint32_tnote_flags;
+
#ifdef TARGET_MIPS
int fp_abi;
int interp_fp_abi;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 6b422990ff..3c6cbd35c3 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2391,7 +2391,7 @@ static void load_elf_image(const char *image_name, int
image_fd,
struct elfhdr *ehdr = (struct elfhdr *)bprm_buf;
struct elf_phdr *phdr;
abi_ulong load_addr, load_bias, loaddr, hiaddr, error;
-int i, retval;
+int i, retval, prot_exec;
const char *errmsg;
/* First of all, some simple consistency checks */
@@ -2467,6 +2467,50 @@ static void load_elf_image(const char *image_name, int
image_fd,
goto exit_errmsg;
}
*pinterp_name = interp_name;
+} else if (eppnt->p_type == PT_GNU_PROPERTY) {
+/* Process NT_GNU_PROPERTY_TYPE_0. */
+const uint32_t gnu0_magic = const_le32('G' | 'N' << 8 | 'U' << 16);
+uint32_t note[7];
+
+/*
+ * The note contents are 7 words, but depending on LP64 vs ILP32
+ * there may be an 8th padding word at the end. Check for and
+ * read the minimum size. Further checks below will validate
+ * that the sizes of everything involved are as we expect.
+ */
+if (eppnt->p_filesz < sizeof(note)) {
+continue;
+}
+if (eppnt->p_offset + eppnt->p_filesz <= BPRM_BUF_SIZE) {
+memcpy(note, bprm_buf + eppnt->p_offset, sizeof(note));
+} else {
+retval = pread(image_fd, note, sizeof(note), eppnt->p_offset);
+if (retval != sizeof(note)) {
+goto exit_perror;
+}
+}
+#ifdef BSWAP_NEEDED
+for (i = 0; i < ARRAY_SIZE(note); ++i) {
+bswap32s(note + i);
+}
+#endif
+/*
+ * Check that this is a NT_GNU_PROPERTY_TYPE_0 note.
+ * Again, descsz includes padding. Full size validation
+ * awaits checking the final payload.
+ */
+if (note[0] != 4 || /* namesz */
+note[1] < 12 || /* descsz */
+note[2] != NT_GNU_PROPERTY_TYPE_0 || /* type */
+note[3] != gnu0_magic) { /* name */
+continue;
+}
+#ifdef TARGET_AARCH64
+if (note[4] == GNU_PROPERTY_AARCH64_FEATURE_1_AND &&
+note[5] == 4) {
+info->note_flags = note[6];
+}
+#endif /* TARGET_AARCH64 */
}
}
@@ -2555,6 +2599,26 @@ static void load_elf_image(const char *image_name, int
image_fd,
info->brk = 0;
info->elf_flags = ehdr->e_flags;
+prot_exec = PROT_EXEC;
+#ifdef TARGET_AARCH64
+/*
+ * If the BTI feature is present, this indicates that the executable
+ * pages of the startup binary should be mapped with PROT_BTI, so that
+ * branch targets are enforced.
+ *
+ * The startup binary is either the interpreter or the static executable.
+ * The interpreter is responsible for all pages of a dynamic executable.
+ *
+ * Elf notes are backward compatible to older cpus.
+ * Do not enable BTI unless it is supported.
+ */
+if ((info->note_flags & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
+&& (pinterp_name == NULL || *pinterp_name == 0)
+&& cpu_isar_feature(aa64_bti, ARM_CPU(thread_cpu))) {
+prot_exec |= TARGET_PROT_BTI;
+}
+#endif
+
for (i = 0; i < ehdr->e_phnum; i++) {
struct elf_phdr *eppnt = phdr + i;
if (eppnt->p_type == PT_LOAD) {
@@ -2568,7 +2632,7 @@ static void load_elf_image(const char *image_name, int
image_fd,
elf_prot |= PROT_WRITE;
}
if (eppnt->p_flags & PF_X) {
-elf_prot |= PROT_EXEC;
+elf_prot |= prot_exec;
}
vaddr = load_bias + eppnt->p_vaddr;
--
[PATCH v10 5/8] linux-user/elfload: Adjust iteration over phdr
The second loop uses a loop induction variable, and the first
does not. Transform the first to match the second, to simplify
a following patch moving code between them.
Signed-off-by: Richard Henderson
---
linux-user/elfload.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 7572a32a30..735ebfa190 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2426,17 +2426,18 @@ static void load_elf_image(const char *image_name, int
image_fd,
loaddr = -1, hiaddr = 0;
info->alignment = 0;
for (i = 0; i < ehdr->e_phnum; ++i) {
-if (phdr[i].p_type == PT_LOAD) {
-abi_ulong a = phdr[i].p_vaddr - phdr[i].p_offset;
+struct elf_phdr *eppnt = phdr + i;
+if (eppnt->p_type == PT_LOAD) {
+abi_ulong a = eppnt->p_vaddr - eppnt->p_offset;
if (a < loaddr) {
loaddr = a;
}
-a = phdr[i].p_vaddr + phdr[i].p_memsz;
+a = eppnt->p_vaddr + eppnt->p_memsz;
if (a > hiaddr) {
hiaddr = a;
}
++info->nsegs;
-info->alignment |= phdr[i].p_align;
+info->alignment |= eppnt->p_align;
}
}
--
2.25.1
[PATCH v10 6/8] linux-user/elfload: Move PT_INTERP detection to first loop
For BTI, we need to know if the executable is static or dynamic,
which means looking for PT_INTERP earlier.
Signed-off-by: Richard Henderson
---
linux-user/elfload.c | 60 +++-
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 735ebfa190..6b422990ff 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2421,8 +2421,10 @@ static void load_elf_image(const char *image_name, int
image_fd,
mmap_lock();
-/* Find the maximum size of the image and allocate an appropriate
- amount of memory to handle that. */
+/*
+ * Find the maximum size of the image and allocate an appropriate
+ * amount of memory to handle that. Locate the interpreter, if any.
+ */
loaddr = -1, hiaddr = 0;
info->alignment = 0;
for (i = 0; i < ehdr->e_phnum; ++i) {
@@ -2438,6 +2440,33 @@ static void load_elf_image(const char *image_name, int
image_fd,
}
++info->nsegs;
info->alignment |= eppnt->p_align;
+} else if (eppnt->p_type == PT_INTERP && pinterp_name) {
+char *interp_name;
+
+if (*pinterp_name) {
+errmsg = "Multiple PT_INTERP entries";
+goto exit_errmsg;
+}
+interp_name = malloc(eppnt->p_filesz);
+if (!interp_name) {
+goto exit_perror;
+}
+
+if (eppnt->p_offset + eppnt->p_filesz <= BPRM_BUF_SIZE) {
+memcpy(interp_name, bprm_buf + eppnt->p_offset,
+ eppnt->p_filesz);
+} else {
+retval = pread(image_fd, interp_name, eppnt->p_filesz,
+ eppnt->p_offset);
+if (retval != eppnt->p_filesz) {
+goto exit_perror;
+}
+}
+if (interp_name[eppnt->p_filesz - 1] != 0) {
+errmsg = "Invalid PT_INTERP entry";
+goto exit_errmsg;
+}
+*pinterp_name = interp_name;
}
}
@@ -2590,33 +2619,6 @@ static void load_elf_image(const char *image_name, int
image_fd,
if (vaddr_em > info->brk) {
info->brk = vaddr_em;
}
-} else if (eppnt->p_type == PT_INTERP && pinterp_name) {
-char *interp_name;
-
-if (*pinterp_name) {
-errmsg = "Multiple PT_INTERP entries";
-goto exit_errmsg;
-}
-interp_name = malloc(eppnt->p_filesz);
-if (!interp_name) {
-goto exit_perror;
-}
-
-if (eppnt->p_offset + eppnt->p_filesz <= BPRM_BUF_SIZE) {
-memcpy(interp_name, bprm_buf + eppnt->p_offset,
- eppnt->p_filesz);
-} else {
-retval = pread(image_fd, interp_name, eppnt->p_filesz,
- eppnt->p_offset);
-if (retval != eppnt->p_filesz) {
-goto exit_perror;
-}
-}
-if (interp_name[eppnt->p_filesz - 1] != 0) {
-errmsg = "Invalid PT_INTERP entry";
-goto exit_errmsg;
-}
-*pinterp_name = interp_name;
#ifdef TARGET_MIPS
} else if (eppnt->p_type == PT_MIPS_ABIFLAGS) {
Mips_elf_abiflags_v0 abiflags;
--
2.25.1
[PATCH v10 3/8] include/elf: Add defines related to GNU property notes for AArch64
These are all of the defines required to parse
GNU_PROPERTY_AARCH64_FEATURE_1_AND, copied from binutils.
Other missing defines related to other GNU program headers
and notes are elided for now.
Reviewed-by: Peter Maydell
Signed-off-by: Richard Henderson
---
include/elf.h | 22 ++
1 file changed, 22 insertions(+)
diff --git a/include/elf.h b/include/elf.h
index c117a4d1ab..10126ff809 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -26,9 +26,13 @@ typedef int64_t Elf64_Sxword;
#define PT_NOTE4
#define PT_SHLIB 5
#define PT_PHDR6
+#define PT_LOOS0x6000
+#define PT_HIOS0x6fff
#define PT_LOPROC 0x7000
#define PT_HIPROC 0x7fff
+#define PT_GNU_PROPERTY (PT_LOOS + 0x474e553)
+
#define PT_MIPS_REGINFO 0x7000
#define PT_MIPS_RTPROC0x7001
#define PT_MIPS_OPTIONS 0x7002
@@ -1657,6 +1661,24 @@ typedef struct elf64_shdr {
#define NT_ARM_SYSTEM_CALL 0x404 /* ARM system call number */
#define NT_ARM_SVE 0x405 /* ARM Scalable Vector Extension regs
*/
+/* Defined note types for GNU systems. */
+
+#define NT_GNU_PROPERTY_TYPE_0 5 /* Program property */
+
+/* Values used in GNU .note.gnu.property notes (NT_GNU_PROPERTY_TYPE_0). */
+
+#define GNU_PROPERTY_STACK_SIZE 1
+#define GNU_PROPERTY_NO_COPY_ON_PROTECTED 2
+
+#define GNU_PROPERTY_LOPROC 0xc000
+#define GNU_PROPERTY_HIPROC 0xdfff
+#define GNU_PROPERTY_LOUSER 0xe000
+#define GNU_PROPERTY_HIUSER 0x
+
+#define GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc000
+#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1u << 0)
+#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1u << 1)
+
/*
* Physical entry point into the kernel.
*
--
2.25.1
[PATCH v10 4/8] linux-user/elfload: Fix coding style in load_elf_image
Fixing this now will clarify following patches.
Signed-off-by: Richard Henderson
---
linux-user/elfload.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f6022fd704..7572a32a30 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2531,9 +2531,15 @@ static void load_elf_image(const char *image_name, int
image_fd,
abi_ulong vaddr, vaddr_po, vaddr_ps, vaddr_ef, vaddr_em, vaddr_len;
int elf_prot = 0;
-if (eppnt->p_flags & PF_R) elf_prot = PROT_READ;
-if (eppnt->p_flags & PF_W) elf_prot |= PROT_WRITE;
-if (eppnt->p_flags & PF_X) elf_prot |= PROT_EXEC;
+if (eppnt->p_flags & PF_R) {
+elf_prot |= PROT_READ;
+}
+if (eppnt->p_flags & PF_W) {
+elf_prot |= PROT_WRITE;
+}
+if (eppnt->p_flags & PF_X) {
+elf_prot |= PROT_EXEC;
+}
vaddr = load_bias + eppnt->p_vaddr;
vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
--
2.25.1
[PATCH v10 2/8] linux-user: Set PAGE_TARGET_1 for TARGET_PROT_BTI
Transform the prot bit to a qemu internal page bit, and save
it in the page tables.
Reviewed-by: Peter Maydell
Signed-off-by: Richard Henderson
---
v10: Add PAGE_BTI define (pmm).
---
include/exec/cpu-all.h | 2 ++
linux-user/syscall_defs.h | 4
target/arm/cpu.h | 5 +
linux-user/mmap.c | 16
target/arm/translate-a64.c | 6 +++---
5 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index f6439c4705..ba80c46c95 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -274,6 +274,8 @@ extern intptr_t qemu_host_page_mask;
/* FIXME: Code that sets/uses this is broken and needs to go away. */
#define PAGE_RESERVED 0x0020
#endif
+/* Target-specific bits that will be used via page_get_flags(). */
+#define PAGE_TARGET_1 0x0080
#if defined(CONFIG_USER_ONLY)
void page_dump(FILE *f);
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 731c3d5341..cabbfb762d 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -1277,6 +1277,10 @@ struct target_winsize {
#define TARGET_PROT_SEM 0x08
#endif
+#ifdef TARGET_AARCH64
+#define TARGET_PROT_BTI 0x10
+#endif
+
/* Common */
#define TARGET_MAP_SHARED 0x01/* Share changes */
#define TARGET_MAP_PRIVATE 0x02/* Changes are private */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e4549a8cc0..a07d605c2f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3439,6 +3439,11 @@ static inline MemTxAttrs
*typecheck_memtxattrs(MemTxAttrs *x)
#define arm_tlb_bti_gp(x) (typecheck_memtxattrs(x)->target_tlb_bit0)
#define arm_tlb_mte_tagged(x) (typecheck_memtxattrs(x)->target_tlb_bit1)
+/*
+ * AArch64 usage of the PAGE_TARGET_* bits for linux-user.
+ */
+#define PAGE_BTI PAGE_TARGET_1
+
/*
* Naming convention for isar_feature functions:
* Functions which test 32-bit ID registers should have _aa32_ in
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index f261563420..00c05e6a0f 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -83,6 +83,22 @@ static int validate_prot_to_pageflags(int *host_prot, int
prot)
*host_prot = (prot & (PROT_READ | PROT_WRITE))
| (prot & PROT_EXEC ? PROT_READ : 0);
+#ifdef TARGET_AARCH64
+/*
+ * The PROT_BTI bit is only accepted if the cpu supports the feature.
+ * Since this is the unusual case, don't bother checking unless
+ * the bit has been requested. If set and valid, record the bit
+ * within QEMU's page_flags.
+ */
+if (prot & TARGET_PROT_BTI) {
+ARMCPU *cpu = ARM_CPU(thread_cpu);
+if (cpu_isar_feature(aa64_bti, cpu)) {
+valid |= TARGET_PROT_BTI;
+page_flags |= PAGE_BTI;
+}
+}
+#endif
+
return prot & ~valid ? 0 : page_flags;
}
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 7188808341..072754fa24 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14507,10 +14507,10 @@ static void disas_data_proc_simd_fp(DisasContext *s,
uint32_t insn)
*/
static bool is_guarded_page(CPUARMState *env, DisasContext *s)
{
-#ifdef CONFIG_USER_ONLY
-return false; /* FIXME */
-#else
uint64_t addr = s->base.pc_first;
+#ifdef CONFIG_USER_ONLY
+return page_get_flags(addr) & PAGE_BTI;
+#else
int mmu_idx = arm_to_core_mmu_idx(s->mmu_idx);
unsigned int index = tlb_index(env, mmu_idx, addr);
CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
--
2.25.1
[PATCH v10 1/8] linux-user/aarch64: Reset btype for signals
The kernel sets btype for the signal handler as if for a call.
Reviewed-by: Peter Maydell
Signed-off-by: Richard Henderson
---
linux-user/aarch64/signal.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index d50c1ae583..b591790c22 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -506,10 +506,16 @@ static void target_setup_frame(int usig, struct
target_sigaction *ka,
+ offsetof(struct target_rt_frame_record, tramp);
}
env->xregs[0] = usig;
-env->xregs[31] = frame_addr;
env->xregs[29] = frame_addr + fr_ofs;
-env->pc = ka->_sa_handler;
env->xregs[30] = return_addr;
+env->xregs[31] = frame_addr;
+env->pc = ka->_sa_handler;
+
+/* Invoke the signal handler as if by indirect call. */
+if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {
+env->btype = 2;
+}
+
if (info) {
tswap_siginfo(>info, info);
env->xregs[1] = frame_addr + offsetof(struct target_rt_sigframe, info);
--
2.25.1
[PATCH v10 0/8] linux-user: User support for AArch64 BTI
The kernel abi for this was merged in v5.8, just as the qemu 5.1 merge window was closing, so this slipped to the next dev cycle. Changes from v9: * Split what is now patch 7 into 3 more (pmm). * All prerequisites are now upstream. r~ Richard Henderson (8): linux-user/aarch64: Reset btype for signals linux-user: Set PAGE_TARGET_1 for TARGET_PROT_BTI include/elf: Add defines related to GNU property notes for AArch64 linux-user/elfload: Fix coding style in load_elf_image linux-user/elfload: Adjust iteration over phdr linux-user/elfload: Move PT_INTERP detection to first loop linux-user/elfload: Parse NT_GNU_PROPERTY_TYPE_0 notes tests/tcg/aarch64: Add bti smoke test include/elf.h | 22 + include/exec/cpu-all.h| 2 + linux-user/qemu.h | 4 + linux-user/syscall_defs.h | 4 + target/arm/cpu.h | 5 + linux-user/aarch64/signal.c | 10 +- linux-user/elfload.c | 147 ++ linux-user/mmap.c | 16 target/arm/translate-a64.c| 6 +- tests/tcg/aarch64/bti-1.c | 62 + tests/tcg/aarch64/bti-crt.inc.c | 51 +++ tests/tcg/aarch64/Makefile.target | 7 ++ tests/tcg/configure.sh| 4 + 13 files changed, 298 insertions(+), 42 deletions(-) create mode 100644 tests/tcg/aarch64/bti-1.c create mode 100644 tests/tcg/aarch64/bti-crt.inc.c -- 2.25.1
[Bug 1896096] Re: Git version: Build process is broken in block_curl.c.o
Intentional because they possibly broke one of the build scenarios that are tested pre-pull (even though the GitLab CI was fine, it does not yet cover everything). The plan is to flush my queue until there's only the current 5 pending meson patches, and then submit those in isolation. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1896096 Title: Git version: Build process is broken in block_curl.c.o Status in QEMU: Invalid Bug description: Gcc version: 10.2.0 Glusterfs: 8.1 Libguestfs: 1.42 Configure options used: configure \ --prefix=/usr \ --sysconfdir=/etc \ --localstatedir=/var \ --libexecdir=/usr/lib/qemu \ --extra-ldflags="$LDFLAGS" \ --smbd=/usr/bin/smbd \ --enable-modules \ --enable-sdl \ --disable-werror \ --enable-slirp=system \ --enable-xfsctl \ --audio-drv-list="pa alsa sdl" Error log attached. Here is the beginning: /usr/bin/ld: /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../lib/Scrt1.o: in function `_start': (.text+0x24): undefined reference to `main' /usr/bin/ld: libblock-curl.a(block_curl.c.o): in function `curl_block_init': To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1896096/+subscriptions
Re: [PATCH v3 0/2] block: deprecate the sheepdog driver
On Fri, Oct 2, 2020 at 7:34 AM Daniel P. Berrangé wrote: > > 2 years back I proposed dropping the sheepdog mailing list from the > MAINTAINERS file, but somehow the patch never got picked up: > > https://lists.gnu.org/archive/html/qemu-block/2018-03/msg01048.html > > So here I am with the same patch again. > > This time I go further and deprecate the sheepdog driver entirely. > See the rationale in the second patch commit message. > > Changes in v3: > > - A few minor text changes > - Don't initialize static variable to false > > Daniel P. Berrangé (2): > block: drop moderated sheepdog mailing list from MAINTAINERS file > block: deprecate the sheepdog block driver > > MAINTAINERS| 1 - > block/sheepdog.c | 14 ++ > configure | 5 +++-- > docs/system/deprecated.rst | 9 + > 4 files changed, 26 insertions(+), 3 deletions(-) > > -- > 2.26.2 > Series looks good to me. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH v6 04/10] device_core: use drain_call_rcu in in qmp_device_add
On 02/10/20 19:35, Paolo Bonzini wrote:
> From: Maxim Levitsky
>
> Soon, a device removal might only happen on RCU callback execution.
> This is okay for device-del which provides a DEVICE_DELETED event,
> but not for the failure case of device-add. To avoid changing
> monitor semantics, just drain all pending RCU callbacks on error.
Nope, this is not enough... qemu-iotests test 067 still breaks; if I
leave the drain_call_rcu() in qmp_device_del, on the other hand, the
hotplug test in qtest-ppc64:qos-test hangs because DEVICE_DELETED comes
before the {'return'}.
One way to handle this is:
1) handle events properly in libqtest. For example, rename
qtest_qmp_receive to qtest_qmp_receive_dict (most callers can keep on
calling qtest_qmp_receive, though some have to switch if they look for
events); a new qtest_qmp_receive that calls qtest_qmp_receive_dict until
it doesn't get an event, and in the meanwhile stashes the events in a
GList. Then qtest_qmp_eventwait_ref can look for events in the GList
prior to calling qtest_qmp_receive_any if it doesn't find any.
2) with (1) in place we can probably keep the drain_call_rcu(), but
perhaps we can drop it if we remove test 067 and add equivalent tests to
tests/qtest/drive_del.c.
The reason I'm not fond of the drain_call_rcu() in device_del is that
there is already a DEVICE_DELETED callback, and device-del is (at least
for some buses) an advisory operation that the guest needs to attend to.
So there's no guarantee that you get the DEVICE_DELETED event before
drain_call_rcu().
I can look at this next week.
Paolo
[Bug 1896096] Re: Git version: Build process is broken in block_curl.c.o
Hi Paolo, The CFLAGS patches seem to have missed your last big pull req: [PULL v8 00/86] Misc QEMU patches for 2020-09-24 Apparently disappeared between v3 and v4. Oversight or intentional? Thanks. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1896096 Title: Git version: Build process is broken in block_curl.c.o Status in QEMU: Invalid Bug description: Gcc version: 10.2.0 Glusterfs: 8.1 Libguestfs: 1.42 Configure options used: configure \ --prefix=/usr \ --sysconfdir=/etc \ --localstatedir=/var \ --libexecdir=/usr/lib/qemu \ --extra-ldflags="$LDFLAGS" \ --smbd=/usr/bin/smbd \ --enable-modules \ --enable-sdl \ --disable-werror \ --enable-slirp=system \ --enable-xfsctl \ --audio-drv-list="pa alsa sdl" Error log attached. Here is the beginning: /usr/bin/ld: /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../lib/Scrt1.o: in function `_start': (.text+0x24): undefined reference to `main' /usr/bin/ld: libblock-curl.a(block_curl.c.o): in function `curl_block_init': To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1896096/+subscriptions
[PATCH v2] scripts/qmp/qom-set: Allow setting integer value
If the value appears to be an integer, parse it as such.
This allows the following:
qmp/qom-set -s ~/qmp.sock sensor.temperature 2
.. where sensor is a tmp105 device, and temperature is an integer
property.
Signed-off-by: Jonatan Pålsson
---
scripts/qmp/qom-set | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 240a78187f..49eebe4924 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -56,7 +56,10 @@ if len(args) > 1:
path, prop = args[0].rsplit('.', 1)
except:
usage_error("invalid format for path/property/value")
-value = args[1]
+try:
+value = int(args[1])
+except ValueError:
+value = args[1]
else:
usage_error("not enough arguments")
--
2.26.1
[Bug 1898215] Re: [git][archlinux]Build process is busted in spice-display.c
Already reported to Arch:
https://bugs.archlinux.org/task/68061
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1898215
Title:
[git][archlinux]Build process is busted in spice-display.c
Status in QEMU:
New
Bug description:
Linux distribution: Archlinux. Crash log added is based on a build
from scratch.
Gcc version: 10.2.0
Configure options used:
configure \
--prefix=/usr \
--sysconfdir=/etc \
--localstatedir=/var \
--libexecdir=/usr/lib/qemu \
--extra-ldflags="$LDFLAGS" \
--smbd=/usr/bin/smbd \
--enable-modules \
--enable-sdl \
--disable-werror \
--enable-slirp=system \
--enable-xfsctl \
--audio-drv-list="pa alsa sdl"
Crash log:
../ui/spice-display.c: In function 'interface_client_monitors_config':
../ui/spice-display.c:682:25: error:
'VD_AGENT_CONFIG_MONITORS_FLAG_PHYSICAL_SIZE' undeclared (first use in this
function); did you mean 'VD_AGENT_CONFIG_MONITORS_FLAG_USE_POS'?
682 | if (mc->flags & VD_AGENT_CONFIG_MONITORS_FLAG_PHYSICAL_SIZE) {
| ^~~
| VD_AGENT_CONFIG_MONITORS_FLAG_USE_POS
../ui/spice-display.c:682:25: note: each undeclared identifier is reported
only once for each function it appears in
../ui/spice-display.c:683:13: error: unknown type name 'VDAgentMonitorMM'
683 | VDAgentMonitorMM *mm = (void
*)>monitors[mc->num_of_monitors];
| ^~~~
../ui/spice-display.c:684:37: error: request for member 'width' in something
not a structure or union
684 | info.width_mm = mm[head].width;
| ^
../ui/spice-display.c:685:38: error: request for member 'height' in something
not a structure or union
685 | info.height_mm = mm[head].height;
| ^
make: *** [Makefile.ninja:2031: libcommon.fa.p/ui_spice-display.c.o] Error 1
make: *** Waiting for unfinished jobs
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1898215/+subscriptions
Re: [RFC PATCH 0/2] target/arm: Fix tlb flush page vs tbi
I can confirm this resolves the issue we were seeing. Thank you Richard! Best, Jordan On 10/1/20, 10:08 AM, "Richard Henderson" wrote: Since the FAR_ELx fix at 38d931687fa1, it is reported that page granularity flushing is broken. This makes sense, since TCG will record the entire virtual address in its TLB, not simply the 56 significant bits. With no other TCG support, the ARM backend should require 256 different page flushes to clear the virtual address of any possible tag. So I added a new tcg interface that allows passing the size of the virtual address. I thought a simple bit-count was a cleaner interface than passing in a mask, since it means that we couldn't be passed nonsensical masks like 0xdeadbeef. It also makes it easy to re-direct special cases. I don't have a test case that triggers the bug. All I can say is that (1) this still boots a normal kernel and (2) the code paths are triggered since the kernel enables tbi for EL0. Jordan, can you test this please? r~ Richard Henderson (2): accel/tcg: Add tlb_flush_page_bits_by_mmuidx* target/arm: Use tlb_flush_page_bits_by_mmuidx* include/exec/exec-all.h | 36 ++ accel/tcg/cputlb.c | 259 ++-- target/arm/helper.c | 46 +-- 3 files changed, 325 insertions(+), 16 deletions(-) -- 2.25.1
Re: [PATCH] scripts/qmp/qom-set: Allow setting integer value
On 10/2/20 4:36 PM, Jonatan Palsson wrote:
On Fri, Oct 2, 2020 at 10:29 PM John Snow wrote:
On 10/2/20 4:19 PM, Jonatan Pålsson wrote:
If the value appears to be an integer, parse it as such.
This allows the following:
qmp/qom-set -s ~/qmp.sock sensor.temperature 2
.. where sensor is a tmp105 device, and temperature is an integer
property.
Signed-off-by: Jonatan Pålsson
---
scripts/qmp/qom-set | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 240a78187f..61920680eb 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -56,7 +56,10 @@ if len(args) > 1:
path, prop = args[0].rsplit('.', 1)
except:
usage_error("invalid format for path/property/value")
-value = args[1]
+try:
+value = int(args[1])
+except:
+value = args[1]
Please catch the ValueError explicitly.
Sure, I'll send a v2.
else:
usage_error("not enough arguments")
What happens when you don't convert it to int specifically? Does
something break? My understanding was that QOM received everything as a
string anyway, and does its own parsing.
With the current implementation, I see this:
scripts/qmp/qom-set -s ~/qmp.sock sensor.temperature 2
Traceback (most recent call last):
File "scripts/qmp/qom-set", line 66, in
print(srv.command('qom-set', path=path, property=prop, value=value))
File "scripts/qmp/../../python/qemu/qmp.py", line 274, in command
raise QMPResponseError(ret)
qemu.qmp.QMPResponseError: Invalid parameter type for 'temperature',
expected: integer
Oh, unfortunate. I hope there aren't any cases where QOM expects a
string, but can accept a string where a numerical value is otherwise valid.
These are just debugging scripts, so I guess this is probably okay to
just change and see if anything explodes.
(A more robust solution might actually attempt to query QEMU to get the
QOM property types and judiciously apply conversions. A problem for
another day?)
--js
Re: [PATCH] scripts/qmp/qom-set: Allow setting integer value
On Fri, Oct 2, 2020 at 10:29 PM John Snow wrote:
>
> On 10/2/20 4:19 PM, Jonatan Pålsson wrote:
> > If the value appears to be an integer, parse it as such.
> >
> > This allows the following:
> >
> > qmp/qom-set -s ~/qmp.sock sensor.temperature 2
> >
> > .. where sensor is a tmp105 device, and temperature is an integer
> > property.
> >
> > Signed-off-by: Jonatan Pålsson
> > ---
> > scripts/qmp/qom-set | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
> > index 240a78187f..61920680eb 100755
> > --- a/scripts/qmp/qom-set
> > +++ b/scripts/qmp/qom-set
> > @@ -56,7 +56,10 @@ if len(args) > 1:
> > path, prop = args[0].rsplit('.', 1)
> > except:
> > usage_error("invalid format for path/property/value")
> > -value = args[1]
> > +try:
> > +value = int(args[1])
> > +except:
> > +value = args[1]
>
> Please catch the ValueError explicitly.
Sure, I'll send a v2.
>
> > else:
> > usage_error("not enough arguments")
> >
> >
>
> What happens when you don't convert it to int specifically? Does
> something break? My understanding was that QOM received everything as a
> string anyway, and does its own parsing.
With the current implementation, I see this:
scripts/qmp/qom-set -s ~/qmp.sock sensor.temperature 2
Traceback (most recent call last):
File "scripts/qmp/qom-set", line 66, in
print(srv.command('qom-set', path=path, property=prop, value=value))
File "scripts/qmp/../../python/qemu/qmp.py", line 274, in command
raise QMPResponseError(ret)
qemu.qmp.QMPResponseError: Invalid parameter type for 'temperature',
expected: integer
Re: [PULL v2 00/11] capstone + disassembler patch queue
On Fri, 2 Oct 2020 at 17:51, Richard Henderson
wrote:
>
> Version 2 retains a dummy capstone/all makefile target, to avoid
> the build failure that Peter saw.
>
>
> r~
>
>
> The following changes since commit dd8c1e808f1ca311e1f50bff218c3ee3198b1f02:
>
> Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20201002' into
> staging (2020-10-02 14:29:49 +0100)
>
> are available in the Git repository at:
>
> https://gitlab.com/rth7680/qemu.git tags/pull-cap-20201002
>
> for you to fetch changes up to 94816249a1e14f90f56a2f6e1d566e959e9bc26d:
>
> disas/capstone: Add skipdata hook for s390x (2020-10-02 11:05:07 -0500)
>
>
> Update capstone submodule from v3.0.5 to v5 ("next").
> Convert submodule build to meson.
> Enable capstone disassembly for s390x.
> Code cleanups in disas.c
>
Meson warning on the BSDs:
Configuring sparc-bsd-user-config-target.h using configuration
Configuring sparc64-bsd-user-config-target.h using configuration
Configuring x86_64-bsd-user-config-target.h using configuration
Did not find CMake 'cmake'
Found CMake: NO
Run-time dependency capstone found: NO (tried pkgconfig and cmake)
../src/meson.build:753: WARNING: Trying to compare values of different
types (bool, str) using ==.
The result of this is undefined and will become a hard error in a
future Meson release.
Configuring config-host.h using configuration
Program scripts/hxtool found: YES
Program scripts/shaderinclude.pl found: YES
Program scripts/qapi-gen.py found: YES
Program scripts/qemu-version.sh found: YES
Run-time dependency threads found: YES
Program keycodemapdb/tools/keymap-gen found: YES
Program scripts/decodetree.py found: YES
Warning from ppc64be box (gcc compilefarm one):
Configuring sh4eb-linux-user-config-target.h using configuration
Configuring sparc-linux-user-config-target.h using configuration
Configuring sparc32plus-linux-user-config-target.h using configuration
Configuring sparc64-linux-user-config-target.h using configuration
Configuring x86_64-linux-user-config-target.h using configuration
Configuring xtensa-linux-user-config-target.h using configuration
Configuring xtensaeb-linux-user-config-target.h using configuration
Found CMake: /usr/bin/cmake (2.8.12.2)
WARNING: The version of CMake /usr/bin/cmake is 2.8.12.2 but version
>=3.4 is required
Run-time dependency capstone found: NO (tried pkgconfig and cmake)
Configuring capstone-defs.h using configuration
Configuring config-host.h using configuration
We shouldn't be looking for or using cmake at all.
thanks
-- PMM
Re: [PATCH] scripts/qmp/qom-set: Allow setting integer value
On 10/2/20 4:19 PM, Jonatan Pålsson wrote:
If the value appears to be an integer, parse it as such.
This allows the following:
qmp/qom-set -s ~/qmp.sock sensor.temperature 2
.. where sensor is a tmp105 device, and temperature is an integer
property.
Signed-off-by: Jonatan Pålsson
---
scripts/qmp/qom-set | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 240a78187f..61920680eb 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -56,7 +56,10 @@ if len(args) > 1:
path, prop = args[0].rsplit('.', 1)
except:
usage_error("invalid format for path/property/value")
-value = args[1]
+try:
+value = int(args[1])
+except:
+value = args[1]
Please catch the ValueError explicitly.
else:
usage_error("not enough arguments")
What happens when you don't convert it to int specifically? Does
something break? My understanding was that QOM received everything as a
string anyway, and does its own parsing.
[PATCH] scripts/qmp/qom-set: Allow setting integer value
If the value appears to be an integer, parse it as such.
This allows the following:
qmp/qom-set -s ~/qmp.sock sensor.temperature 2
.. where sensor is a tmp105 device, and temperature is an integer
property.
Signed-off-by: Jonatan Pålsson
---
scripts/qmp/qom-set | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 240a78187f..61920680eb 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -56,7 +56,10 @@ if len(args) > 1:
path, prop = args[0].rsplit('.', 1)
except:
usage_error("invalid format for path/property/value")
-value = args[1]
+try:
+value = int(args[1])
+except:
+value = args[1]
else:
usage_error("not enough arguments")
--
2.26.1
[PATCH v2 9/9] s390x/pci: get zPCI function info from host
We use the VFIO_REGION_SUBTYPE_ZDEV_CLP subregion of PCI_VENDOR_ID_IBM to
retrieve the CLP information the kernel exports.
To be compatible with previous kernel versions we fall back on previous
predefined values, same as the emulation values, when the region is not
found. If individual CLP feature(s) are not found in the region, we fall
back on default values for only those features missing from the region.
This patch is based on work previously done by Pierre Morel.
Signed-off-by: Matthew Rosato
---
hw/s390x/meson.build | 1 +
hw/s390x/s390-pci-bus.c | 10 +-
hw/s390x/s390-pci-vfio.c | 235 +++
include/hw/s390x/s390-pci-bus.h | 1 +
include/hw/s390x/s390-pci-clp.h | 12 +-
include/hw/s390x/s390-pci-vfio.h | 19
6 files changed, 271 insertions(+), 7 deletions(-)
create mode 100644 hw/s390x/s390-pci-vfio.c
create mode 100644 include/hw/s390x/s390-pci-vfio.h
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 948ceae..3ee4594 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -10,6 +10,7 @@ s390x_ss.add(files(
's390-ccw.c',
's390-pci-bus.c',
's390-pci-inst.c',
+ 's390-pci-vfio.c',
's390-skeys.c',
's390-stattrib.c',
's390-virtio-hcall.c',
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3dc8a10..182e76e 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -17,6 +17,7 @@
#include "cpu.h"
#include "hw/s390x/s390-pci-bus.h"
#include "hw/s390x/s390-pci-inst.h"
+#include "hw/s390x/s390-pci-vfio.h"
#include "hw/pci/pci_bus.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci_bridge.h"
@@ -737,7 +738,7 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus
*bus, int32_t devfn)
object_unref(OBJECT(iommu));
}
-static S390PCIGroup *s390_group_create(int id)
+S390PCIGroup *s390_group_create(int id)
{
S390PCIGroup *group;
S390pciState *s = s390_get_phb();
@@ -782,7 +783,7 @@ static void set_pbdev_info(S390PCIBusDevice *pbdev)
pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
pbdev->zpci_fn.pchid = 0;
-pbdev->zpci_fn.ug = ZPCI_DEFAULT_FN_GRP;
+pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
pbdev->zpci_fn.fid = pbdev->fid;
pbdev->zpci_fn.uid = pbdev->uid;
pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
@@ -861,7 +862,8 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
memory_region_init_io(>msix_notify_mr, OBJECT(pbdev),
_msi_ctrl_ops, pbdev, name, PAGE_SIZE);
-memory_region_add_subregion(>iommu->mr, ZPCI_MSI_ADDR,
+memory_region_add_subregion(>iommu->mr,
+pbdev->pci_group->zpci_group.msia,
>msix_notify_mr);
g_free(name);
@@ -1013,6 +1015,8 @@ static void s390_pcihost_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
pbdev->fh |= FH_SHM_VFIO;
+/* Fill in CLP information passed via the vfio region */
+s390_pci_get_clp_info(pbdev);
} else {
pbdev->fh |= FH_SHM_EMUL;
}
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
new file mode 100644
index 000..8435df0
--- /dev/null
+++ b/hw/s390x/s390-pci-vfio.c
@@ -0,0 +1,235 @@
+/*
+ * s390 vfio-pci interfaces
+ *
+ * Copyright 2020 IBM Corp.
+ * Author(s): Matthew Rosato
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include
+#include
+#include
+
+#include "qemu/osdep.h"
+#include "hw/s390x/s390-pci-bus.h"
+#include "hw/s390x/s390-pci-clp.h"
+#include "hw/s390x/s390-pci-vfio.h"
+#include "hw/vfio/pci.h"
+
+#ifndef DEBUG_S390PCI_VFIO
+#define DEBUG_S390PCI_VFIO 0
+#endif
+
+#define DPRINTF(fmt, ...) \
+do { \
+if (DEBUG_S390PCI_VFIO) { \
+fprintf(stderr, "S390pci-vfio: " fmt, ## __VA_ARGS__); \
+} \
+} while (0)
+
+static void *get_next_clp_buf(struct vfio_region_zpci_info *zpci_info,
+ struct vfio_region_zpci_info_hdr *hdr)
+{
+/* If the next payload would be beyond the region, we're done */
+if (zpci_info->argsz <= hdr->next) {
+return NULL;
+}
+
+return (void *)zpci_info + hdr->next;
+}
+
+static void *find_clp_data(struct vfio_region_zpci_info *zpci_info, int id)
+{
+struct vfio_region_zpci_info_hdr *hdr;
+void *clp;
+
+assert(zpci_info);
+
+/* Jump to the first CLP feature, which starts with header information */
+clp = (void *)zpci_info + zpci_info->offset;
+
[PATCH v2 6/9] s390x/pci: use a PCI Group structure
From: Pierre Morel
We use a S390PCIGroup structure to hold the information related to a
zPCI Function group.
This allows us to be ready to support multiple groups and to retrieve
the group information from the host.
Signed-off-by: Pierre Morel
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-pci-bus.c | 42 +
hw/s390x/s390-pci-inst.c| 23 +-
include/hw/s390x/s390-pci-bus.h | 10 ++
3 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index a929340..c99f2f0 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -737,6 +737,46 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus
*bus, int32_t devfn)
object_unref(OBJECT(iommu));
}
+static S390PCIGroup *s390_group_create(int id)
+{
+S390PCIGroup *group;
+S390pciState *s = s390_get_phb();
+
+group = g_new0(S390PCIGroup, 1);
+group->id = id;
+QTAILQ_INSERT_TAIL(>zpci_groups, group, link);
+return group;
+}
+
+S390PCIGroup *s390_group_find(int id)
+{
+S390PCIGroup *group;
+S390pciState *s = s390_get_phb();
+
+QTAILQ_FOREACH(group, >zpci_groups, link) {
+if (group->id == id) {
+return group;
+}
+}
+return NULL;
+}
+
+static void s390_pci_init_default_group(void)
+{
+S390PCIGroup *group;
+ClpRspQueryPciGrp *resgrp;
+
+group = s390_group_create(ZPCI_DEFAULT_FN_GRP);
+resgrp = >zpci_group;
+resgrp->fr = 1;
+stq_p(>dasm, 0);
+stq_p(>msia, ZPCI_MSI_ADDR);
+stw_p(>mui, DEFAULT_MUI);
+stw_p(>i, 128);
+stw_p(>maxstbl, 128);
+resgrp->version = 0;
+}
+
static void s390_pcihost_realize(DeviceState *dev, Error **errp)
{
PCIBus *b;
@@ -764,7 +804,9 @@ static void s390_pcihost_realize(DeviceState *dev, Error
**errp)
s->bus_no = 0;
QTAILQ_INIT(>pending_sei);
QTAILQ_INIT(>zpci_devs);
+QTAILQ_INIT(>zpci_groups);
+s390_pci_init_default_group();
css_register_io_adapters(CSS_IO_ADAPTER_PCI, true, false,
S390_ADAPTER_SUPPRESSIBLE, errp);
}
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 639b13c..aeb8b5f 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -284,21 +284,25 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t
ra)
stq_p(>edma, ZPCI_EDMA_ADDR);
stl_p(>fid, pbdev->fid);
stw_p(>pchid, 0);
-stw_p(>ug, 1);
+stw_p(>ug, ZPCI_DEFAULT_FN_GRP);
stl_p(>uid, pbdev->uid);
stw_p(>hdr.rsp, CLP_RC_OK);
break;
}
case CLP_QUERY_PCI_FNGRP: {
ClpRspQueryPciGrp *resgrp = (ClpRspQueryPciGrp *)resh;
-resgrp->fr = 1;
-stq_p(>dasm, 0);
-stq_p(>msia, ZPCI_MSI_ADDR);
-stw_p(>mui, DEFAULT_MUI);
-stw_p(>i, 128);
-stw_p(>maxstbl, 128);
-resgrp->version = 0;
+ClpReqQueryPciGrp *reqgrp = (ClpReqQueryPciGrp *)reqh;
+S390PCIGroup *group;
+
+group = s390_group_find(reqgrp->g);
+if (!group) {
+/* We do not allow access to unknown groups */
+/* The group must have been obtained with a vfio device */
+stw_p(>hdr.rsp, CLP_RC_QUERYPCIFG_PFGID);
+goto out;
+}
+memcpy(resgrp, >zpci_group, sizeof(ClpRspQueryPciGrp));
stw_p(>hdr.rsp, CLP_RC_OK);
break;
}
@@ -754,7 +758,8 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t
r3, uint64_t gaddr,
}
/* Length must be greater than 8, a multiple of 8 */
/* and not greater than maxstbl */
-if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
+if ((len <= 8) || (len % 8) ||
+(len > pbdev->pci_group->zpci_group.maxstbl)) {
goto specification_error;
}
/* Do not cross a 4K-byte boundary */
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index 04e7cb6..52f6e7b 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -308,6 +308,14 @@ typedef struct ZpciFmb {
} ZpciFmb;
QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
+#define ZPCI_DEFAULT_FN_GRP 0x20
+typedef struct S390PCIGroup {
+ClpRspQueryPciGrp zpci_group;
+int id;
+QTAILQ_ENTRY(S390PCIGroup) link;
+} S390PCIGroup;
+S390PCIGroup *s390_group_find(int id);
+
struct S390PCIBusDevice {
DeviceState qdev;
PCIDevice *pdev;
@@ -325,6 +333,7 @@ struct S390PCIBusDevice {
uint16_t noi;
uint16_t maxstbl;
uint8_t sum;
+S390PCIGroup *pci_group;
S390MsixInfo msix;
AdapterRoutes routes;
S390PCIIOMMU *iommu;
@@ -349,6 +358,7 @@ struct S390pciState {
GHashTable *zpci_table;
QTAILQ_HEAD(, SeiContainer) pending_sei;
QTAILQ_HEAD(, S390PCIBusDevice) zpci_devs;
+QTAILQ_HEAD(, S390PCIGroup) zpci_groups;
};
S390pciState *s390_get_phb(void);
--
[PATCH v2 7/9] s390x/pci: clean up s390 PCI groups
Add a step to remove all stashed PCI groups to avoid stale data between
machine resets.
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-pci-bus.c | 12
1 file changed, 12 insertions(+)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index c99f2f0..c34f14a 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -811,6 +811,17 @@ static void s390_pcihost_realize(DeviceState *dev, Error
**errp)
S390_ADAPTER_SUPPRESSIBLE, errp);
}
+static void s390_pcihost_unrealize(DeviceState *dev)
+{
+S390PCIGroup *group;
+S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
+
+while (!QTAILQ_EMPTY(>zpci_groups)) {
+group = QTAILQ_FIRST(>zpci_groups);
+QTAILQ_REMOVE(>zpci_groups, group, link);
+}
+}
+
static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
{
char *name;
@@ -1165,6 +1176,7 @@ static void s390_pcihost_class_init(ObjectClass *klass,
void *data)
dc->reset = s390_pcihost_reset;
dc->realize = s390_pcihost_realize;
+dc->unrealize = s390_pcihost_unrealize;
hc->pre_plug = s390_pcihost_pre_plug;
hc->plug = s390_pcihost_plug;
hc->unplug_request = s390_pcihost_unplug_request;
--
1.8.3.1
[PATCH v2 8/9] s390x/pci: use a PCI Function structure
From: Pierre Morel
We use a ClpRspQueryPci structure to hold the information related to a
zPCI Function.
This allows us to be ready to support different zPCI functions and to
retrieve the zPCI function information from the host.
Signed-off-by: Pierre Morel
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-pci-bus.c | 22 +-
hw/s390x/s390-pci-inst.c| 8 ++--
include/hw/s390x/s390-pci-bus.h | 1 +
3 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index c34f14a..3dc8a10 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -777,6 +777,17 @@ static void s390_pci_init_default_group(void)
resgrp->version = 0;
}
+static void set_pbdev_info(S390PCIBusDevice *pbdev)
+{
+pbdev->zpci_fn.sdma = ZPCI_SDMA_ADDR;
+pbdev->zpci_fn.edma = ZPCI_EDMA_ADDR;
+pbdev->zpci_fn.pchid = 0;
+pbdev->zpci_fn.ug = ZPCI_DEFAULT_FN_GRP;
+pbdev->zpci_fn.fid = pbdev->fid;
+pbdev->zpci_fn.uid = pbdev->uid;
+pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
+}
+
static void s390_pcihost_realize(DeviceState *dev, Error **errp)
{
PCIBus *b;
@@ -994,17 +1005,18 @@ static void s390_pcihost_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
}
}
+pbdev->pdev = pdev;
+pbdev->iommu = s390_pci_get_iommu(s, pci_get_bus(pdev), pdev->devfn);
+pbdev->iommu->pbdev = pbdev;
+pbdev->state = ZPCI_FS_DISABLED;
+set_pbdev_info(pbdev);
+
if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
pbdev->fh |= FH_SHM_VFIO;
} else {
pbdev->fh |= FH_SHM_EMUL;
}
-pbdev->pdev = pdev;
-pbdev->iommu = s390_pci_get_iommu(s, pci_get_bus(pdev), pdev->devfn);
-pbdev->iommu->pbdev = pbdev;
-pbdev->state = ZPCI_FS_DISABLED;
-
if (s390_pci_msix_init(pbdev)) {
error_setg(errp, "MSI-X support is mandatory "
"in the S390 architecture");
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index aeb8b5f..cc3d274 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -267,6 +267,8 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
goto out;
}
+memcpy(resquery, >zpci_fn, sizeof(*resquery));
+
for (i = 0; i < PCI_BAR_COUNT; i++) {
uint32_t data = pci_get_long(pbdev->pdev->config +
PCI_BASE_ADDRESS_0 + (i * 4));
@@ -280,12 +282,6 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t
ra)
resquery->bar_size[i]);
}
-stq_p(>sdma, ZPCI_SDMA_ADDR);
-stq_p(>edma, ZPCI_EDMA_ADDR);
-stl_p(>fid, pbdev->fid);
-stw_p(>pchid, 0);
-stw_p(>ug, ZPCI_DEFAULT_FN_GRP);
-stl_p(>uid, pbdev->uid);
stw_p(>hdr.rsp, CLP_RC_OK);
break;
}
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index 52f6e7b..79206be 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -334,6 +334,7 @@ struct S390PCIBusDevice {
uint16_t maxstbl;
uint8_t sum;
S390PCIGroup *pci_group;
+ClpRspQueryPci zpci_fn;
S390MsixInfo msix;
AdapterRoutes routes;
S390PCIIOMMU *iommu;
--
1.8.3.1
[PATCH v2 1/9] s390x/pci: Move header files to include/hw/s390x
Seems a more appropriate location for them.
Signed-off-by: Matthew Rosato
---
hw/s390x/s390-pci-bus.c | 4 +-
hw/s390x/s390-pci-bus.h | 372 ---
hw/s390x/s390-pci-inst.c | 4 +-
hw/s390x/s390-pci-inst.h | 312
hw/s390x/s390-virtio-ccw.c | 2 +-
include/hw/s390x/s390-pci-bus.h | 372 +++
include/hw/s390x/s390-pci-inst.h | 312
7 files changed, 689 insertions(+), 689 deletions(-)
delete mode 100644 hw/s390x/s390-pci-bus.h
delete mode 100644 hw/s390x/s390-pci-inst.h
create mode 100644 include/hw/s390x/s390-pci-bus.h
create mode 100644 include/hw/s390x/s390-pci-inst.h
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index fb4cee8..a929340 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -15,8 +15,8 @@
#include "qapi/error.h"
#include "qapi/visitor.h"
#include "cpu.h"
-#include "s390-pci-bus.h"
-#include "s390-pci-inst.h"
+#include "hw/s390x/s390-pci-bus.h"
+#include "hw/s390x/s390-pci-inst.h"
#include "hw/pci/pci_bus.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci_bridge.h"
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
deleted file mode 100644
index 97464d0..000
--- a/hw/s390x/s390-pci-bus.h
+++ /dev/null
@@ -1,372 +0,0 @@
-/*
- * s390 PCI BUS definitions
- *
- * Copyright 2014 IBM Corp.
- * Author(s): Frank Blaschka
- *Hong Bo Li
- *Yi Min Zhao
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or (at
- * your option) any later version. See the COPYING file in the top-level
- * directory.
- */
-
-#ifndef HW_S390_PCI_BUS_H
-#define HW_S390_PCI_BUS_H
-
-#include "hw/pci/pci.h"
-#include "hw/pci/pci_host.h"
-#include "hw/s390x/sclp.h"
-#include "hw/s390x/s390_flic.h"
-#include "hw/s390x/css.h"
-#include "qom/object.h"
-
-#define TYPE_S390_PCI_HOST_BRIDGE "s390-pcihost"
-#define TYPE_S390_PCI_BUS "s390-pcibus"
-#define TYPE_S390_PCI_DEVICE "zpci"
-#define TYPE_S390_PCI_IOMMU "s390-pci-iommu"
-#define TYPE_S390_IOMMU_MEMORY_REGION "s390-iommu-memory-region"
-#define FH_MASK_ENABLE 0x8000
-#define FH_MASK_INSTANCE 0x7f00
-#define FH_MASK_SHM 0x00ff
-#define FH_MASK_INDEX0x
-#define FH_SHM_VFIO 0x0001
-#define FH_SHM_EMUL 0x0002
-#define ZPCI_MAX_FID 0x
-#define ZPCI_MAX_UID 0x
-#define UID_UNDEFINED 0
-#define UID_CHECKING_ENABLED 0x01
-
-OBJECT_DECLARE_SIMPLE_TYPE(S390pciState, S390_PCI_HOST_BRIDGE)
-OBJECT_DECLARE_SIMPLE_TYPE(S390PCIBus, S390_PCI_BUS)
-OBJECT_DECLARE_SIMPLE_TYPE(S390PCIBusDevice, S390_PCI_DEVICE)
-OBJECT_DECLARE_SIMPLE_TYPE(S390PCIIOMMU, S390_PCI_IOMMU)
-
-#define HP_EVENT_TO_CONFIGURED0x0301
-#define HP_EVENT_RESERVED_TO_STANDBY 0x0302
-#define HP_EVENT_DECONFIGURE_REQUEST 0x0303
-#define HP_EVENT_CONFIGURED_TO_STBRES 0x0304
-#define HP_EVENT_STANDBY_TO_RESERVED 0x0308
-
-#define ERR_EVENT_INVALAS 0x1
-#define ERR_EVENT_OORANGE 0x2
-#define ERR_EVENT_INVALTF 0x3
-#define ERR_EVENT_TPROTE 0x4
-#define ERR_EVENT_APROTE 0x5
-#define ERR_EVENT_KEYE0x6
-#define ERR_EVENT_INVALTE 0x7
-#define ERR_EVENT_INVALTL 0x8
-#define ERR_EVENT_TT 0x9
-#define ERR_EVENT_INVALMS 0xa
-#define ERR_EVENT_SERR0xb
-#define ERR_EVENT_NOMSI 0x10
-#define ERR_EVENT_INVALBV 0x11
-#define ERR_EVENT_AIBV0x12
-#define ERR_EVENT_AIRERR 0x13
-#define ERR_EVENT_FMBA0x2a
-#define ERR_EVENT_FMBUP 0x2b
-#define ERR_EVENT_FMBPRO 0x2c
-#define ERR_EVENT_CCONF 0x30
-#define ERR_EVENT_SERVAC 0x3a
-#define ERR_EVENT_PERMERR 0x3b
-
-#define ERR_EVENT_Q_BIT 0x2
-#define ERR_EVENT_MVN_OFFSET 16
-
-#define ZPCI_MSI_VEC_BITS 11
-#define ZPCI_MSI_VEC_MASK 0x7ff
-
-#define ZPCI_MSI_ADDR 0xfe00ULL
-#define ZPCI_SDMA_ADDR 0x1ULL
-#define ZPCI_EDMA_ADDR 0x1ffULL
-
-#define PAGE_SHIFT 12
-#define PAGE_SIZE (1 << PAGE_SHIFT)
-#define PAGE_MASK (~(PAGE_SIZE-1))
-#define PAGE_DEFAULT_ACC0
-#define PAGE_DEFAULT_KEY(PAGE_DEFAULT_ACC << 4)
-
-/* I/O Translation Anchor (IOTA) */
-enum ZpciIoatDtype {
-ZPCI_IOTA_STO = 0,
-ZPCI_IOTA_RTTO = 1,
-ZPCI_IOTA_RSTO = 2,
-ZPCI_IOTA_RFTO = 3,
-ZPCI_IOTA_PFAA = 4,
-ZPCI_IOTA_IOPFAA = 5,
-ZPCI_IOTA_IOPTO = 7
-};
-
-#define ZPCI_IOTA_IOT_ENABLED 0x800ULL
-#define ZPCI_IOTA_DT_ST (ZPCI_IOTA_STO << 2)
-#define ZPCI_IOTA_DT_RT (ZPCI_IOTA_RTTO << 2)
-#define ZPCI_IOTA_DT_RS (ZPCI_IOTA_RSTO << 2)
-#define ZPCI_IOTA_DT_RF (ZPCI_IOTA_RFTO << 2)
-#define ZPCI_IOTA_DT_PF (ZPCI_IOTA_PFAA << 2)
-#define ZPCI_IOTA_FS_4K 0
-#define ZPCI_IOTA_FS_1M 1
-#define ZPCI_IOTA_FS_2G 2
-#define ZPCI_KEY(PAGE_DEFAULT_KEY << 5)
-
-#define ZPCI_IOTA_STO_FLAG (ZPCI_IOTA_IOT_ENABLED |
[PATCH v2 5/9] s390x/pci: create a header dedicated to PCI CLP
From: Pierre Morel
To have a clean separation between s390-pci-bus.h and s390-pci-inst.h
headers we export the PCI CLP instructions in a dedicated header.
Signed-off-by: Pierre Morel
Signed-off-by: Matthew Rosato
---
include/hw/s390x/s390-pci-bus.h | 1 +
include/hw/s390x/s390-pci-clp.h | 211 +++
include/hw/s390x/s390-pci-inst.h | 196
3 files changed, 212 insertions(+), 196 deletions(-)
create mode 100644 include/hw/s390x/s390-pci-clp.h
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index 97464d0..04e7cb6 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -19,6 +19,7 @@
#include "hw/s390x/sclp.h"
#include "hw/s390x/s390_flic.h"
#include "hw/s390x/css.h"
+#include "hw/s390x/s390-pci-clp.h"
#include "qom/object.h"
#define TYPE_S390_PCI_HOST_BRIDGE "s390-pcihost"
diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
new file mode 100644
index 000..e442307
--- /dev/null
+++ b/include/hw/s390x/s390-pci-clp.h
@@ -0,0 +1,211 @@
+/*
+ * s390 CLPinstruction definitions
+ *
+ * Copyright 2019 IBM Corp.
+ * Author(s): Pierre Morel
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef HW_S390_PCI_CLP
+#define HW_S390_PCI_CLP
+
+/* CLP common request & response block size */
+#define CLP_BLK_SIZE 4096
+#define PCI_BAR_COUNT 6
+#define PCI_MAX_FUNCTIONS 4096
+
+typedef struct ClpReqHdr {
+uint16_t len;
+uint16_t cmd;
+} QEMU_PACKED ClpReqHdr;
+
+typedef struct ClpRspHdr {
+uint16_t len;
+uint16_t rsp;
+} QEMU_PACKED ClpRspHdr;
+
+/* CLP Response Codes */
+#define CLP_RC_OK 0x0010 /* Command request successfully */
+#define CLP_RC_CMD0x0020 /* Command code not recognized */
+#define CLP_RC_PERM 0x0030 /* Command not authorized */
+#define CLP_RC_FMT0x0040 /* Invalid command request format */
+#define CLP_RC_LEN0x0050 /* Invalid command request length */
+#define CLP_RC_8K 0x0060 /* Command requires 8K LPCB */
+#define CLP_RC_RESNOT00x0070 /* Reserved field not zero */
+#define CLP_RC_NODATA 0x0080 /* No data available */
+#define CLP_RC_FC_UNKNOWN 0x0100 /* Function code not recognized */
+
+/*
+ * Call Logical Processor - Command Codes
+ */
+#define CLP_LIST_PCI0x0002
+#define CLP_QUERY_PCI_FN0x0003
+#define CLP_QUERY_PCI_FNGRP 0x0004
+#define CLP_SET_PCI_FN 0x0005
+
+/* PCI function handle list entry */
+typedef struct ClpFhListEntry {
+uint16_t device_id;
+uint16_t vendor_id;
+#define CLP_FHLIST_MASK_CONFIG 0x8000
+uint32_t config;
+uint32_t fid;
+uint32_t fh;
+} QEMU_PACKED ClpFhListEntry;
+
+#define CLP_RC_SETPCIFN_FH 0x0101 /* Invalid PCI fn handle */
+#define CLP_RC_SETPCIFN_FHOP0x0102 /* Fn handle not valid for op */
+#define CLP_RC_SETPCIFN_DMAAS 0x0103 /* Invalid DMA addr space */
+#define CLP_RC_SETPCIFN_RES 0x0104 /* Insufficient resources */
+#define CLP_RC_SETPCIFN_ALRDY 0x0105 /* Fn already in requested state */
+#define CLP_RC_SETPCIFN_ERR 0x0106 /* Fn in permanent error state */
+#define CLP_RC_SETPCIFN_RECPND 0x0107 /* Error recovery pending */
+#define CLP_RC_SETPCIFN_BUSY0x0108 /* Fn busy */
+#define CLP_RC_LISTPCI_BADRT0x010a /* Resume token not recognized */
+#define CLP_RC_QUERYPCIFG_PFGID 0x010b /* Unrecognized PFGID */
+
+/* request or response block header length */
+#define LIST_PCI_HDR_LEN 32
+
+/* Number of function handles fitting in response block */
+#define CLP_FH_LIST_NR_ENTRIES \
+((CLP_BLK_SIZE - 2 * LIST_PCI_HDR_LEN) \
+/ sizeof(ClpFhListEntry))
+
+#define CLP_SET_ENABLE_PCI_FN 0 /* Yes, 0 enables it */
+#define CLP_SET_DISABLE_PCI_FN 1 /* Yes, 1 disables it */
+
+#define CLP_UTIL_STR_LEN 64
+
+#define CLP_MASK_FMT 0xf000
+
+/* List PCI functions request */
+typedef struct ClpReqListPci {
+ClpReqHdr hdr;
+uint32_t fmt;
+uint64_t reserved1;
+uint64_t resume_token;
+uint64_t reserved2;
+} QEMU_PACKED ClpReqListPci;
+
+/* List PCI functions response */
+typedef struct ClpRspListPci {
+ClpRspHdr hdr;
+uint32_t fmt;
+uint64_t reserved1;
+uint64_t resume_token;
+uint32_t mdd;
+uint16_t max_fn;
+uint8_t flags;
+uint8_t entry_size;
+ClpFhListEntry fh_list[CLP_FH_LIST_NR_ENTRIES];
+} QEMU_PACKED ClpRspListPci;
+
+/* Query PCI function request */
+typedef struct ClpReqQueryPci {
+ClpReqHdr hdr;
+uint32_t fmt;
+uint64_t reserved1;
+uint32_t fh; /* function handle */
+uint32_t reserved2;
+uint64_t reserved3;
+} QEMU_PACKED ClpReqQueryPci;
+
+/* Query PCI function response */
+typedef struct ClpRspQueryPci {
+ClpRspHdr hdr;
+uint32_t fmt;
+uint64_t reserved1;
+uint16_t vfn; /* virtual fn number */
+#define
[PATCH v2 4/9] linux-headers: update against 5.9-rc7
PLACEHOLDER as the kernel patch driving the need for this ("vfio-pci/zdev:
define the vfio_zdev header") isn't merged yet.
Signed-off-by: Matthew Rosato
---
.../drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h | 14 +++---
linux-headers/linux/kvm.h | 6 --
linux-headers/linux/vfio.h | 5 +
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git
a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
index 7b4062a..acd4c83 100644
--- a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
+++ b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h
@@ -68,7 +68,7 @@ static inline int pvrdma_idx_valid(uint32_t idx, uint32_t
max_elems)
static inline int32_t pvrdma_idx(int *var, uint32_t max_elems)
{
- const unsigned int idx = qatomic_read(var);
+ const unsigned int idx = atomic_read(var);
if (pvrdma_idx_valid(idx, max_elems))
return idx & (max_elems - 1);
@@ -77,17 +77,17 @@ static inline int32_t pvrdma_idx(int *var, uint32_t
max_elems)
static inline void pvrdma_idx_ring_inc(int *var, uint32_t max_elems)
{
- uint32_t idx = qatomic_read(var) + 1; /* Increment. */
+ uint32_t idx = atomic_read(var) + 1;/* Increment. */
idx &= (max_elems << 1) - 1;/* Modulo size, flip gen. */
- qatomic_set(var, idx);
+ atomic_set(var, idx);
}
static inline int32_t pvrdma_idx_ring_has_space(const struct pvrdma_ring *r,
uint32_t max_elems, uint32_t
*out_tail)
{
- const uint32_t tail = qatomic_read(>prod_tail);
- const uint32_t head = qatomic_read(>cons_head);
+ const uint32_t tail = atomic_read(>prod_tail);
+ const uint32_t head = atomic_read(>cons_head);
if (pvrdma_idx_valid(tail, max_elems) &&
pvrdma_idx_valid(head, max_elems)) {
@@ -100,8 +100,8 @@ static inline int32_t pvrdma_idx_ring_has_space(const
struct pvrdma_ring *r,
static inline int32_t pvrdma_idx_ring_has_data(const struct pvrdma_ring *r,
uint32_t max_elems, uint32_t
*out_head)
{
- const uint32_t tail = qatomic_read(>prod_tail);
- const uint32_t head = qatomic_read(>cons_head);
+ const uint32_t tail = atomic_read(>prod_tail);
+ const uint32_t head = atomic_read(>cons_head);
if (pvrdma_idx_valid(tail, max_elems) &&
pvrdma_idx_valid(head, max_elems)) {
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 6683e2e..43580c7 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -790,9 +790,10 @@ struct kvm_ppc_resize_hpt {
#define KVM_VM_PPC_HV 1
#define KVM_VM_PPC_PR 2
-/* on MIPS, 0 forces trap & emulate, 1 forces VZ ASE */
-#define KVM_VM_MIPS_TE 0
+/* on MIPS, 0 indicates auto, 1 forces VZ ASE, 2 forces trap & emulate */
+#define KVM_VM_MIPS_AUTO 0
#define KVM_VM_MIPS_VZ 1
+#define KVM_VM_MIPS_TE 2
#define KVM_S390_SIE_PAGE_OFFSET 1
@@ -1035,6 +1036,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_LAST_CPU 184
#define KVM_CAP_SMALLER_MAXPHYADDR 185
#define KVM_CAP_S390_DIAG318 186
+#define KVM_CAP_STEAL_TIME 187
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index a906724..68fd67a 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -326,6 +326,11 @@ struct vfio_region_info_cap_type {
* to do TLB invalidation on a GPU.
*/
#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1)
+/*
+ * IBM zPCI specific hardware feature information for a devcie. The contents
+ * of this region are mapped by struct vfio_region_zpci_info.
+ */
+#define VFIO_REGION_SUBTYPE_IBM_ZPCI_CLP (2)
/* sub-types for VFIO_REGION_TYPE_GFX */
#define VFIO_REGION_SUBTYPE_GFX_EDID(1)
--
1.8.3.1
[PATCH v2 3/9] update-linux-headers: Add vfio_zdev.h
vfio_zdev.h is used by s390x zPCI support to pass device-specific CLP information between host and userspace. Signed-off-by: Matthew Rosato --- scripts/update-linux-headers.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh index 29c27f4..9efbaf2 100755 --- a/scripts/update-linux-headers.sh +++ b/scripts/update-linux-headers.sh @@ -141,7 +141,7 @@ done rm -rf "$output/linux-headers/linux" mkdir -p "$output/linux-headers/linux" -for header in kvm.h vfio.h vfio_ccw.h vhost.h \ +for header in kvm.h vfio.h vfio_ccw.h vfio_zdev.h vhost.h \ psci.h psp-sev.h userfaultfd.h mman.h; do cp "$tmpdir/include/linux/$header" "$output/linux-headers/linux" done -- 1.8.3.1
[PATCH v2 2/9] MAINTAINERS: Update s390 PCI entry to include headers
Accomodate changes to file locations. Signed-off-by: Matthew Rosato --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index b76fb31..dd4e0ea 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1442,6 +1442,7 @@ S390 PCI M: Matthew Rosato S: Supported F: hw/s390x/s390-pci* +F: include/hw/s390x/s390-pci* L: qemu-s3...@nongnu.org UniCore32 Machines -- 1.8.3.1
[PATCH v2 0/9] Retrieve zPCI hardware information from VFIO
This patchset exploits the VFIO ZPCI CLP region, which provides hardware information about passed-through s390 PCI devices that can be shared with the guest. The retrieval of this information is done once per function (and for a subset of data, once per function group) and is performed at time of device plug. Some elements provided in the CLP region must still be forced to default values for now to reflect what QEMU actually provides support for. The original work for this feature was done by Pierre Morel. Associated kernel patchset: https://lkml.org/lkml/2020/10/2/981 Changes from v1: - Added 2 patches to the front of this set that move the s390-pci-bus.h and s390-pci-inst.h files to include + associated MAINTAINERS hit. These can be applied separately, but are included here for the sake of simplicity. - Patch 4: header update placeholder refreshed to rc7 - Patch 5: Move new s390-pci-clp.h to include folder - Patch 6+: s/grp/group/ and fallout from this - Patch 9: Move new s390-pci-vfio.h to include folder Matthew Rosato (6): s390x/pci: Move header files to include/hw/s390x MAINTAINERS: Update s390 PCI entry to include headers update-linux-headers: Add vfio_zdev.h linux-headers: update against 5.9-rc7 s390x/pci: clean up s390 PCI groups s390x/pci: get zPCI function info from host Pierre Morel (3): s390x/pci: create a header dedicated to PCI CLP s390x/pci: use a PCI Group structure s390x/pci: use a PCI Function structure MAINTAINERS| 1 + hw/s390x/meson.build | 1 + hw/s390x/s390-pci-bus.c| 86 - hw/s390x/s390-pci-bus.h| 372 hw/s390x/s390-pci-inst.c | 33 +- hw/s390x/s390-pci-inst.h | 312 - hw/s390x/s390-pci-vfio.c | 235 + hw/s390x/s390-virtio-ccw.c | 2 +- include/hw/s390x/s390-pci-bus.h| 385 + include/hw/s390x/s390-pci-clp.h| 215 include/hw/s390x/s390-pci-inst.h | 116 +++ include/hw/s390x/s390-pci-vfio.h | 19 + .../drivers/infiniband/hw/vmw_pvrdma/pvrdma_ring.h | 14 +- linux-headers/linux/kvm.h | 6 +- linux-headers/linux/vfio.h | 5 + scripts/update-linux-headers.sh| 2 +- 16 files changed, 1085 insertions(+), 719 deletions(-) delete mode 100644 hw/s390x/s390-pci-bus.h delete mode 100644 hw/s390x/s390-pci-inst.h create mode 100644 hw/s390x/s390-pci-vfio.c create mode 100644 include/hw/s390x/s390-pci-bus.h create mode 100644 include/hw/s390x/s390-pci-clp.h create mode 100644 include/hw/s390x/s390-pci-inst.h create mode 100644 include/hw/s390x/s390-pci-vfio.h -- 1.8.3.1
[Bug 1896342] Re: IDE ATA IDENTIFY WORD 106
You might be right, though at present it seems like it doesn't hurt anything that I am aware of to claim that our mapping is 1:1 in such cases. Patches welcome; especially if there is any proof that this has caused any problems anywhere. --js -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1896342 Title: IDE ATA IDENTIFY WORD 106 Status in QEMU: New Bug description: The code at line 202 in hw/ide/core.c (https://git.qemu.org/?p=qemu.git;a=blob;f=hw/ide/core.c;#l201) hard codes bit 13 set. However, get_physical_block_exp() can and may return 0, which is a valid response. If get_physical_block_exp() does return zero, bit 13 should not be set. ATAPI8 states (Section 7.17.7.73): "Bit 13 of word 106 shall be set to one to indicate that the device has more than one logical sector per physical sector" and gives the examples: Bits (3:0): 0 = 2^0 = 1 logical sector per physical sector Bits (3:0): 1 = 2^1 = 2 logical sector per physical sector Bits (3:0): 2 = 2^2 = 4 logical sector per physical sector Bits (3:0): 3 = 2^3 = 8 logical sector per physical sector Therefore, if bit 13 is set, bits 3:0 must be greater than zero. If get_physical_block_exp() returns zero then there is a 1:1 ratio and bit 13 must be 0. Just my opinion. Thanks, Ben To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1896342/+subscriptions
Re: [PATCH v5 10/10] migration: introduce snapshot-{save, load, delete} QMP commands
On 10/2/20 11:27 AM, Daniel P. Berrangé wrote:
> savevm, loadvm and delvm are some of the few HMP commands that have never
> been converted to use QMP. The reasons for the lack of conversion are
> that they blocked execution of the event thread, and the semantics
> around choice of disks were ill-defined.
>
> Despite this downside, however, libvirt and applications using libvirt
> have used these commands for as long as QMP has existed, via the
> "human-monitor-command" passthrough command. IOW, while it is clearly
> desirable to be able to fix the problems, they are not a blocker to
> all real world usage.
>
> Meanwhile there is a need for other features which involve adding new
> parameters to the commands. This is possible with HMP passthrough, but
> it provides no reliable way for apps to introspect features, so using
> QAPI modelling is highly desirable.
>
> This patch thus introduces new snapshot-{load,save,delete} commands to
> QMP that are intended to replace the old HMP counterparts. The new
> commands are given different names, because they will be using the new
> QEMU job framework and thus will have diverging behaviour from the HMP
> originals. It would thus be misleading to keep the same name.
>
> While this design uses the generic job framework, the current impl is
> still blocking. The intention that the blocking problem is fixed later.
> None the less applications using these new commands should assume that
> they are asynchronous and thus wait for the job status change event to
> indicate completion.
>
> In addition to using the job framework, the new commands require the
> caller to be explicit about all the block device nodes used in the
> snapshot operations, with no built-in default heuristics in use.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> +++ b/qapi/job.json
> @@ -22,10 +22,17 @@
> #
> # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
> #
> +# @snapshot-load: snapshot load job type, see "snapshot-load" (since 5.2)
> +#
> +# @snapshot-save: snapshot save job type, see "snapshot-save" (since 5.2)
> +#
> +# @snapshot-delete: snapshot delete job type, see "snapshot-delete" (since
> 5.2)
> +#
> # Since: 1.7
> ##
> { 'enum': 'JobType',
> - 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
> + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
> + 'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
>
> ##
> # @JobStatus:
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7f5e6fd681..d2bd551ad9 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1787,3 +1787,123 @@
> # Since: 5.2
> ##
> { 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
> +
> +##
> +# @snapshot-save:
> +#
> +# Save a VM snapshot
> +#
> +# @job-id: identifier for the newly created job
> +# @tag: name of the snapshot to create
> +# @devices: list of block device node names to save a snapshot to
> +# @vmstate: block device node name to save vmstate to
Here, you document vmstate last,...
> +#
> +# Applications should not assume that the snapshot save is complete
> +# when this command returns. The job commands / events must be used
> +# to determine completion and to fetch details of any errors that arise.
> +#
> +# Note that the VM CPUs will be paused during the time it takes to
> +# save the snapshot
"will be", or "may be"? As you stated above, we may be able to lift the
synchronous limitations down the road, while still maintaining the
present interface of using this command to start the job and waiting on
the job id until it is finished, at which point the CPUs might not need
to be paused as much.
> +#
> +# It is strongly recommended that @devices contain all writable
> +# block device nodes if a consistent snapshot is required.
> +#
> +# If @tag already exists, an error will be reported
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "snapshot-save",
> +# "data": {
> +# "job-id": "snapsave0",
> +# "tag": "my-snap",
> +# "vmstate": "disk0",
> +# "devices": ["disk0", "disk1"]
...here vmstate occurs before devices. I don't know if our doc
generator cares about inconsistent ordering.
> +# }
> +#}
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'snapshot-save',
> + 'data': { 'job-id': 'str',
> +'tag': 'str',
> +'vmstate': 'str',
> +'devices': ['str'] } }
> +
> +##
> +# @snapshot-load:
> +#
> +# Load a VM snapshot
> +#
> +# @job-id: identifier for the newly created job
> +# @tag: name of the snapshot to load.
> +# @devices: list of block device node names to load a snapshot from
> +# @vmstate: block device node name to load vmstate from
> +#
> +# Applications should not assume that the snapshot save is complete
> +# when this command returns. The job commands / events must be used
> +# to determine completion and to fetch details of any errors that
Re: [PATCH] ide:do nothing for identify cmd if no any device attached
On 9/3/20 6:40 AM, Max Reitz wrote:
On 02.09.20 20:02, John Snow wrote:
(CC Max for block backend model confusion, see below)
On 8/16/20 11:38 PM, zhaoxin\RockCuioc wrote:
This patch is for avoiding win7 IDE driver polling 0x1f7 when
no any device attached. During Win7 VM boot procedure, if use virtio for
disk and there is no any device be attached on hda & hdb, the win7 IDE
driver
would poll 0x1f7 for a while. This action may be stop windows LOGO
atomic for
a while too on a poor performance CPU.
A few questions:
(1) How slow is the probing?
(2) If there are no devices attached, why don't you remove the IDE
controller so that Windows doesn't have to probe it?
Signed-off-by: zhaoxin\RockCuioc
---
hw/ide/core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..26d86f4b40 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
s = idebus_active_if(bus);
trace_ide_exec_cmd(bus, s, val);
- /* ignore commands to non existent slave */
- if (s != bus->ifs && !s->blk) {
(Was the first one basically meant to be “s != >ifs[0]”, i.e. to
check that this doesn’t go to the ma^W primary? Not too obvious.)
Yeah, I think it was meant to say:
if (s == bus->ifs[1] && !s->blk)
(But I don't know why it was important to guard device1 specifically.
Knowledge lost to the sands of time.)
By the way, the correct terms are "device0" and "device1" and have been
since at least ATA2. I believe ATA1 used the terms "disk0" and "disk1".
"Primary" and "Secondary" are used to refer to the controllers, not the
devices.
- Primary
- device0
- device1
- Secondary
- device0
- device1
Thanks for coming to my TED talk!
+ /* ignore commands if no any device exist or non existent slave */
+ if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
+ (s != bus->ifs && !s->blk)) {
(Maybe this could be improved here)
return;
}
I think it's the case that Empty CD-ROM drives will have an anonymous
block backend representing the empty drive,
(As far as I remember,) yes.
(ide_dev_initfn() ensures all CD drives have one, even if it’s empty.)
(Thanks)
so I suppose this is maybe
fine?
I suppose the idea is that with no drives on the bus that it's fine to
ignore the register writes, as there are no devices to record those writes.
(But then, why did we ever only check device1? ...)
Maybe before the block-backend split we used to have to check to see if
we had attached media or not, but I think nowadays we should always have
a blk pointer if we have a device model intended to be operating at this
address.
The check in ide_dev_initfn() looks that way to me.
So I guess it can be simplified ...?
if (!s->blk) {
return;
}
Probably. Although there’s a difference, of course, namely if you have
only a secondary device and try to access the primary, this simplified
version will be a no-op, whereas the more complicated version in this
patch would still go on. I don’t know how real hardware would handle
that case. Is it even possible to have just a secondary with no primary?
I think so. From what I understand, two drives on a single channel both
receive all of the same register update commands, including the "SELECT"
register, which has a bit devoted to which drive we have selected.
When we write to the COMMAND register, only the selective drive should
actually respond to it.
so what I expect happens on real machines:
- You select device0
- You write to a bunch of registers
- You issue a command
- Nobody responds.
--js
Re: [PATCH 2/8] docs: tweak kernel-doc for QEMU coding standards
On Fri, Oct 02, 2020 at 08:50:22PM +0200, Paolo Bonzini wrote: > On 02/10/20 20:46, Eduardo Habkost wrote: > > This is not the code that parses "#MemoryRegionSection", it is > > the code that parses: > > > > /** > > * MemoryRegionSection: describes a fragment of a #MemoryRegion > >^^^ this line > > We can probably just adjust the comments to include the "struct" keyword > first. There aren't that many, (un)fortunately. > > It may even be possible to print to stderr the file and line number so > that the script tells us where the hack is firing. > There are 3 lines where the hack is correctly triggered, but we can just change it to use "struct MyTypeName" instead: include/exec/memory.h:681: HERE include/exec/memory.h:699: HERE include/exec/memory.h:743: HERE And 29 lines where it's probably being triggered by mistake: include/qom/object.h:47: HERE include/qom/object.h:66: HERE include/qom/object.h:78: HERE include/qom/object.h:87: HERE include/qom/object.h:110: HERE include/qom/object.h:118: HERE include/qom/object.h:140: HERE include/qom/object.h:162: HERE include/qom/object.h:179: HERE include/qom/object.h:200: HERE include/qom/object.h:218: HERE include/qom/object.h:241: HERE include/qom/object.h:259: HERE include/qom/object.h:313: HERE include/qom/object.h:330: HERE include/qom/object.h:353: HERE include/qom/object.h:370: HERE include/qom/object.h:432: HERE include/qom/object.h:442: HERE include/qom/object.h:452: HERE include/qom/object.h:469: HERE include/qom/object.h:483: HERE include/qom/object.h:496: HERE include/qom/object.h:506: HERE include/qom/object.h:521: HERE include/qom/object.h:531: HERE include/qom/object.h:543: HERE include/qom/object.h:861: HERE -- Eduardo
Re: [PATCH V2 2/3] amd-iommu: Sync IOVA-to-GPA translation during page invalidation
On Fri, Oct 02, 2020 at 09:59:06AM -0500, Wei Huang wrote:
> +static void amdvi_sync_domain(AMDVIState *s, uint32_t domid,
> + uint64_t addr, uint16_t flags)
> +{
[...]
> +/*
> + * In case of syncing more than a page, we invalidate the entire
> + * address range and re-walk the whole page table.
> + */
> +if (size == AMDVI_PGSZ_ENTIRE) {
> +IOMMU_NOTIFIER_FOREACH(n, >iommu) {
> +amdvi_address_space_unmap(as, n);
> +}
> +} else if (size > 0x1000) {
> +IOMMU_NOTIFIER_FOREACH(n, >iommu) {
> +if (n->start <= addr && addr + size < n->end) {
> +amdvi_address_space_unmap(as, n);
> +}
> +}
> +}
So this may not work... Because DMA could happen concurrently with the page
sync, so the DMA could fail if the mapping is suddenly gone (even if it's a
very short period).
We encountered this problem on x86 and those will be reported as host DMAR
errors (since those pages were missing), please feel free to have a look at:
63b88968f1 ("intel-iommu: rework the page walk logic", 2018-05-23)
Majorly this paragraph:
For each VTDAddressSpace, now we maintain what IOVA ranges we have
mapped and what we have not. With that information, now we only send
MAP or UNMAP when necessary. Say, we don't send MAP notifies if we
know we have already mapped the range, meanwhile we don't send UNMAP
notifies if we know we never mapped the range at all.
So the simple idea is, as long as one page was mapped and it's not unmapped, we
can _never_ unmap it.. because the device might be using it simultaneously.
What VT-d does right now is to maintain a per-device iova tree, so that we know
what exactly have been mapped. That's kind of an overkill for sure (especially
because vfio kernel module will maintain a similar iova tree, so it's at least
kind of duplicated), however that should fix this problem.
--
Peter Xu
Re: [PATCH V2 1/3] amd-iommu: Add address space notifier and replay support
On Fri, Oct 02, 2020 at 09:59:05AM -0500, Wei Huang wrote:
> +static void amdvi_address_space_unmap(AMDVIAddressSpace *as, IOMMUNotifier
> *n)
> +{
> +IOMMUTLBEntry entry;
> +hwaddr start = n->start;
> +hwaddr end = n->end;
> +hwaddr size = end - start + 1;
> +
> +entry.target_as = _space_memory;
> +entry.iova = start;
> +entry.translated_addr = 0;
> +entry.perm = IOMMU_NONE;
> +entry.addr_mask = size - 1;
This may race with Eugenio's series:
https://mail.gnu.org/archive/html/qemu-ppc/2020-09/msg00131.html
IMHO that series should be acceptable for merging already. Anyway, there's
probably a trivial conflict to solve.
> +
> +memory_region_notify_one(n, );
> +}
> +
> static gboolean amdvi_iotlb_remove_by_domid(gpointer key, gpointer value,
> gpointer user_data)
> {
> @@ -1473,14 +1491,17 @@ static int
> amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> Error **errp)
> {
> AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
> +AMDVIState *s = as->iommu_state;
>
> -if (new & IOMMU_NOTIFIER_MAP) {
> -error_setg(errp,
> - "device %02x.%02x.%x requires iommu notifier which is not
> "
> - "currently supported", as->bus_num, PCI_SLOT(as->devfn),
> - PCI_FUNC(as->devfn));
> -return -EINVAL;
Ideally, we shouldn't remove this error message until the functionality is
fully implemented. Otherwise an user could potentially boot such a vm with a
broken assigned device.
Thanks,
> +/* Update address space notifier flags */
> +as->notifier_flags = new;
> +
> +if (old == IOMMU_NOTIFIER_NONE) {
> +QLIST_INSERT_HEAD(>amdvi_as_with_notifiers, as, next);
> +} else if (new == IOMMU_NOTIFIER_NONE) {
> +QLIST_REMOVE(as, next);
> }
> +
> return 0;
> }
--
Peter Xu
Re: [PATCH 2/8] docs: tweak kernel-doc for QEMU coding standards
On 02/10/20 20:46, Eduardo Habkost wrote: > This is not the code that parses "#MemoryRegionSection", it is > the code that parses: > > /** > * MemoryRegionSection: describes a fragment of a #MemoryRegion >^^^ this line We can probably just adjust the comments to include the "struct" keyword first. There aren't that many, (un)fortunately. It may even be possible to print to stderr the file and line number so that the script tells us where the hack is firing. Paolo
Re: [PULL v8 00/86] Misc QEMU patches for 2020-09-24
On Fri, 2 Oct 2020 at 19:45, Paolo Bonzini wrote:
> On 02/10/20 20:24, Eduardo Habkost wrote:
> > We already have Sphinx 3.x hacks inside our fork of kernel-doc,
> > see commit 152d1967f650 ("kernel-doc: Use c:struct for Sphinx 3.0
> > and later").
> >
> > If we want to keep deviating from upstream kernel-doc, the
> > following patch seems to work. Do we want to?
> I can try sending these to upstream Linux, too.
I did mention the c:struct change we're carrying to Jon Corbet
when I sent him the missing-close-paren fix to kernel-doc
earlier this year:
http://lkml.iu.edu/hypermail/linux/kernel/2004.1/08760.html
I think it probably mostly needs somebody to try these changes
in the context of producing the kernel docs and see if they
are sufficient to fix kernel doc production too...
thanks
-- PMM
Re: [PATCH] gitlab-ci.yml: Only run one test-case per fuzzer
On 201002 1715, Thomas Huth wrote:
> On 02/10/2020 16.35, Alexander Bulekov wrote:
> > With 1000 runs, there is a non-negligible chance that the fuzzer can
> > trigger a crash. With this CI job, we care about catching build/runtime
> > issues in the core fuzzing code. Actual device fuzzing takes place on
> > oss-fuzz. For these purposes, only running one input should be
> > sufficient.
> >
> > Signed-off-by: Alexander Bulekov
> > Suggested-by: Philippe Mathieu-Daudé
> > ---
> > .gitlab-ci.yml | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index a51c89554f..075c15d45c 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -303,7 +303,7 @@ build-oss-fuzz:
> >| grep -v slirp); do
> > grep "LLVMFuzzerTestOneInput" ${fuzzer} > /dev/null 2>&1 ||
> > continue ;
> > echo Testing ${fuzzer} ... ;
> > -"${fuzzer}" -runs=1000 -seed=1 || exit 1 ;
> > +"${fuzzer}" -runs=1 -seed=1 || exit 1 ;
>
> ... but we're apparently already using a fixed seed for running the
> test, so it should be pretty much deterministic, shouldn't it? So the
> chance that the fuzzer hits a crash here for a pre-existing problem
> should be close to zero? ... so I'm not quite sure whether we really
> need this? Anyway, I certainly also won't object this patch, so in case
> anybody wants to merge it:
In addition to using an RNG+seed, libfuzzer also uses coverage
information to guide mutations. My guess is that as QEMU changes, this
coverage can change as well, so I wouldn't assume that using the same
seed will result in the same inputs generated, in the longer term.
Its true that the main benefit will probably be a few minutes shaved off
the 400 minute limit...
Thanks
-Alex
>
> Acked-by: Thomas Huth
>
Re: [PATCH 2/8] docs: tweak kernel-doc for QEMU coding standards
On Fri, Oct 02, 2020 at 07:43:09PM +0100, Peter Maydell wrote:
> On Fri, 2 Oct 2020 at 19:35, Eduardo Habkost wrote:
> >
> > On Mon, Dec 02, 2019 at 06:01:16PM +, Peter Maydell wrote:
> > > On Fri, 29 Nov 2019 at 14:02, Paolo Bonzini wrote:
> > > >
> > > > Surprisingly, QEMU does have a pretty consistent doc comment style and
> > > > it is not very different from the Linux kernel's. Of the documentation
> > > > "sigils", only "#" separates the QEMU doc comment style from Linux's,
> > > > and it has 200+ instances vs. 6 for the kernel's ' foo' (all in
> > > > accel/tcg/translate-all.c), so it's clear that the two standards are
> > > > different in this respect. In addition, our structs are typedefed and
> > > > recognized by CamelCase names.
> > > >
> > > > Adjust kernel-doc's parser for these two aspects of the QEMU coding
> > > > standards. The patch has been valid, with hardly any change, for over
> > > > two years, so it should not be an issue to keep kernel-doc in sync with
> > > > the Linux copy.
> > > >
> > > > Signed-off-by: Paolo Bonzini
> > [...]
> > > > @@ -1906,7 +1914,9 @@ sub process_name($$) {
> > > > ++$warnings;
> > > > }
> > > >
> > > > - if ($identifier =~ m/^struct\b/) {
> > > > + if ($identifier =~ m/^[A-Z]/) {
> > > > + $decl_type = 'type name';
> > > > + } elsif ($identifier =~ m/^struct\b/) {
> > > > $decl_type = 'struct';
> > > > } elsif ($identifier =~ m/^union\b/) {
> > > > $decl_type = 'union';
> > >
> > > The match for 'type name' is pretty wide but I guess
> > > we can find out through experience if we need to narrow it.
> >
> > This change seems to make it impossible to document any macros
> > with uppercase names.
> >
> > (for example, most of the macros in object.h are not included in
> > the generated docs)
> >
> > What exactly is the purpose of the hunk above?
>
> It's so that QEMU's bare type names like MemoryRegionSection get
> recognized as being types. Kernel naming style always
> prefers to say "struct MemoryRegionSection" where we use the
> typedef and say "MemoryRegionSection", which is why the upstream
> kernel-doc doesn't care about this. IIRC, without it, doc comments
> which reference a type like '#TypeName' like:
>
> * @log_sync:
> *
> * Called by memory_region_snapshot_and_clear_dirty() and
> * memory_global_dirty_log_sync(), before accessing QEMU's "official"
> * copy of the dirty memory bitmap for a #MemoryRegionSection.
> *
>
> don't correctly render the typename into a link to the
> struct definition.
This is not the code that parses "#MemoryRegionSection", it is
the code that parses:
/**
* MemoryRegionSection: describes a fragment of a #MemoryRegion
^^^ this line
--
Eduardo
Re: [PULL v8 00/86] Misc QEMU patches for 2020-09-24
On 02/10/20 20:24, Eduardo Habkost wrote:
> On Fri, Oct 02, 2020 at 06:27:35PM +0200, Paolo Bonzini wrote:
>> On 02/10/20 17:58, Michal Prívozník wrote:
>>>
>>> cd442a45db60a1a72fcf980c24bd1227f13f8a87 is the first bad commit
>>>
>>> Sorry for noticing this earlier, but is this known? The build starts
>>> failing for me after this commit:
>>>
>>> /usr/bin/sphinx-build -Dversion=5.1.50 -Drelease= -W
>>> -Ddepfile=docs/devel.d -Ddepfile_stamp=docs/devel.stamp -b html -d
>>> /home/zippy/work/qemu/qemu.git/build/docs/devel.p
>>> /home/zippy/work/qemu/qemu.git/docs/devel
>>> /home/zippy/work/qemu/qemu.git/build/docs/devel
>>> Running Sphinx v3.2.1
>>> building [mo]: targets for 0 po files that are out of date
>>> building [html]: targets for 20 source files that are out of date
>>> updating environment: [new config] 20 added, 0 changed, 0 removed
>>> reading sources... [100%] testing
>>
>> No, this is new. It works with older versions of Sphinx (I have 2.2.2
>> despite being on Fedora 32 which is pretty recent).
>>
>> For now Sphinx 3 is not supported by kerneldoc, we probably should apply
>> a patch like
>>
>> https://www.spinics.net/lists/linux-doc/msg83277.html
>
> We already have Sphinx 3.x hacks inside our fork of kernel-doc,
> see commit 152d1967f650 ("kernel-doc: Use c:struct for Sphinx 3.0
> and later").
>
> If we want to keep deviating from upstream kernel-doc, the
> following patch seems to work. Do we want to?
>
> Signed-off-by: Eduardo Habkost
> ---
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index 40ad782e342..03b49380426 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -838,6 +838,13 @@ sub output_function_rst(%) {
> $lineprefix = "";
> output_highlight_rst($args{'purpose'});
> $start = "\n\n**Syntax**\n\n ``";
> +} elsif ($args{'functiontype'} eq "") {
> + # this is a macro, Sphinx 3.x requires c:macro::
> + if ((split(/\./, $sphinx_version))[0] >= 3) {
> + print ".. c:macro:: ";
> + } else {
> + print ".. c:function:: ";
> + }
> } else {
> print ".. c:function:: ";
> }
>>
>> Paolo
>>
>
I can try sending these to upstream Linux, too.
Paolo
Re: [PATCH 2/8] docs: tweak kernel-doc for QEMU coding standards
On Fri, 2 Oct 2020 at 19:35, Eduardo Habkost wrote:
>
> On Mon, Dec 02, 2019 at 06:01:16PM +, Peter Maydell wrote:
> > On Fri, 29 Nov 2019 at 14:02, Paolo Bonzini wrote:
> > >
> > > Surprisingly, QEMU does have a pretty consistent doc comment style and
> > > it is not very different from the Linux kernel's. Of the documentation
> > > "sigils", only "#" separates the QEMU doc comment style from Linux's,
> > > and it has 200+ instances vs. 6 for the kernel's ' foo' (all in
> > > accel/tcg/translate-all.c), so it's clear that the two standards are
> > > different in this respect. In addition, our structs are typedefed and
> > > recognized by CamelCase names.
> > >
> > > Adjust kernel-doc's parser for these two aspects of the QEMU coding
> > > standards. The patch has been valid, with hardly any change, for over
> > > two years, so it should not be an issue to keep kernel-doc in sync with
> > > the Linux copy.
> > >
> > > Signed-off-by: Paolo Bonzini
> [...]
> > > @@ -1906,7 +1914,9 @@ sub process_name($$) {
> > > ++$warnings;
> > > }
> > >
> > > - if ($identifier =~ m/^struct\b/) {
> > > + if ($identifier =~ m/^[A-Z]/) {
> > > + $decl_type = 'type name';
> > > + } elsif ($identifier =~ m/^struct\b/) {
> > > $decl_type = 'struct';
> > > } elsif ($identifier =~ m/^union\b/) {
> > > $decl_type = 'union';
> >
> > The match for 'type name' is pretty wide but I guess
> > we can find out through experience if we need to narrow it.
>
> This change seems to make it impossible to document any macros
> with uppercase names.
>
> (for example, most of the macros in object.h are not included in
> the generated docs)
>
> What exactly is the purpose of the hunk above?
It's so that QEMU's bare type names like MemoryRegionSection get
recognized as being types. Kernel naming style always
prefers to say "struct MemoryRegionSection" where we use the
typedef and say "MemoryRegionSection", which is why the upstream
kernel-doc doesn't care about this. IIRC, without it, doc comments
which reference a type like '#TypeName' like:
* @log_sync:
*
* Called by memory_region_snapshot_and_clear_dirty() and
* memory_global_dirty_log_sync(), before accessing QEMU's "official"
* copy of the dirty memory bitmap for a #MemoryRegionSection.
*
don't correctly render the typename into a link to the
struct definition.
thanks
-- PMM
Re: [PATCH 2/8] docs: tweak kernel-doc for QEMU coding standards
On Mon, Dec 02, 2019 at 06:01:16PM +, Peter Maydell wrote:
> On Fri, 29 Nov 2019 at 14:02, Paolo Bonzini wrote:
> >
> > Surprisingly, QEMU does have a pretty consistent doc comment style and
> > it is not very different from the Linux kernel's. Of the documentation
> > "sigils", only "#" separates the QEMU doc comment style from Linux's,
> > and it has 200+ instances vs. 6 for the kernel's ' foo' (all in
> > accel/tcg/translate-all.c), so it's clear that the two standards are
> > different in this respect. In addition, our structs are typedefed and
> > recognized by CamelCase names.
> >
> > Adjust kernel-doc's parser for these two aspects of the QEMU coding
> > standards. The patch has been valid, with hardly any change, for over
> > two years, so it should not be an issue to keep kernel-doc in sync with
> > the Linux copy.
> >
> > Signed-off-by: Paolo Bonzini
[...]
> > @@ -1906,7 +1914,9 @@ sub process_name($$) {
> > ++$warnings;
> > }
> >
> > - if ($identifier =~ m/^struct\b/) {
> > + if ($identifier =~ m/^[A-Z]/) {
> > + $decl_type = 'type name';
> > + } elsif ($identifier =~ m/^struct\b/) {
> > $decl_type = 'struct';
> > } elsif ($identifier =~ m/^union\b/) {
> > $decl_type = 'union';
>
> The match for 'type name' is pretty wide but I guess
> we can find out through experience if we need to narrow it.
This change seems to make it impossible to document any macros
with uppercase names.
(for example, most of the macros in object.h are not included in
the generated docs)
What exactly is the purpose of the hunk above?
--
Eduardo
Re: [PATCH v4 4/4] migration: Sync requested pages after postcopy recovery
* Peter Xu (pet...@redhat.com) wrote:
> We synchronize the requested pages right after a postcopy recovery happens.
> This helps to synchronize the prioritized pages on source so that the faulted
> threads can be served faster.
>
> Reported-by: Xiaohui Li
> Signed-off-by: Peter Xu
Reviewed-by: Dr. David Alan Gilbert
> ---
> migration/savevm.c | 57 ++
> migration/trace-events | 1 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 34e4b71052..1dc021ee53 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2011,6 +2011,49 @@ static int
> loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> return LOADVM_QUIT;
> }
>
> +/* We must be with page_request_mutex held */
> +static gboolean postcopy_sync_page_req(gpointer key, gpointer value,
> + gpointer data)
> +{
> +MigrationIncomingState *mis = data;
> +void *host_addr = (void *) key;
> +ram_addr_t rb_offset;
> +RAMBlock *rb;
> +int ret;
> +
> +rb = qemu_ram_block_from_host(host_addr, true, _offset);
> +if (!rb) {
> +/*
> + * This should _never_ happen. However be nice for a migrating VM to
> + * not crash/assert. Post an error (note: intended to not use *_once
> + * because we do want to see all the illegal addresses; and this can
> + * never be triggered by the guest so we're safe) and move on next.
> + */
> +error_report("%s: illegal host addr %p", __func__, host_addr);
> +/* Try the next entry */
> +return FALSE;
> +}
> +
> +ret = migrate_send_rp_message_req_pages(mis, rb, rb_offset);
> +if (ret) {
> +/* Please refer to above comment. */
> +error_report("%s: send rp message failed for addr %p",
> + __func__, host_addr);
> +return FALSE;
> +}
> +
> +trace_postcopy_page_req_sync(host_addr);
> +
> +return FALSE;
> +}
> +
> +static void migrate_send_rp_req_pages_pending(MigrationIncomingState *mis)
> +{
> +WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
> +g_tree_foreach(mis->page_requested, postcopy_sync_page_req, mis);
> +}
> +}
> +
> static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> {
> if (mis->state != MIGRATION_STATUS_POSTCOPY_RECOVER) {
> @@ -2033,6 +2076,20 @@ static int
> loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> /* Tell source that "we are ready" */
> migrate_send_rp_resume_ack(mis, MIGRATION_RESUME_ACK_VALUE);
>
> +/*
> + * After a postcopy recovery, the source should have lost the postcopy
> + * queue, or potentially the requested pages could have been lost during
> + * the network down phase. Let's re-sync with the source VM by
> re-sending
> + * all the pending pages that we eagerly need, so these threads won't get
> + * blocked too long due to the recovery.
> + *
> + * Without this procedure, the faulted destination VM threads (waiting
> for
> + * page requests right before the postcopy is interrupted) can keep
> hanging
> + * until the pages are sent by the source during the background copying
> of
> + * pages, or another thread faulted on the same address accidentally.
> + */
> +migrate_send_rp_req_pages_pending(mis);
> +
> return 0;
> }
>
> diff --git a/migration/trace-events b/migration/trace-events
> index e4d5eb94ca..0fbfd2da60 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -49,6 +49,7 @@ vmstate_save(const char *idstr, const char *vmsd_name) "%s,
> %s"
> vmstate_load(const char *idstr, const char *vmsd_name) "%s, %s"
> postcopy_pause_incoming(void) ""
> postcopy_pause_incoming_continued(void) ""
> +postcopy_page_req_sync(void *host_addr) "sync page req %p"
>
> # vmstate.c
> vmstate_load_field_error(const char *field, int ret) "field \"%s\" load
> failed, ret = %d"
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v4 3/4] migration: Maintain postcopy faulted addresses
* Peter Xu (pet...@redhat.com) wrote:
> Maintain a list of faulted addresses on the destination host for which we're
> waiting on. This is implemented using a GTree rather than a real list to make
> sure even there're plenty of vCPUs/threads that are faulting, the lookup will
> still be fast with O(log(N)) (because we'll do that after placing each page).
> It should bring a slight overhead, but ideally that shouldn't be a big problem
> simply because in most cases the requested page list will be short.
>
> Actually we did similar things for postcopy blocktime measurements. This
> patch
> didn't use that simply because:
>
> (1) blocktime measurement is towards vcpu threads only, but here we need to
> record all faulted addresses, including main thread and external
> thread (like, DPDK via vhost-user).
>
> (2) blocktime measurement will require UFFD_FEATURE_THREAD_ID, but here we
> don't want to add that extra dependency on the kernel version since not
> necessary. E.g., we don't need to know which thread faulted on which
> page, we also don't care about multiple threads faulting on the same
> page. But we only care about what addresses are faulted so waiting for
> a
> page copying from src.
>
> (3) blocktime measurement is not enabled by default. However we need this
> by
> default especially for postcopy recover.
>
> Another thing to mention is that this patch introduced a new mutex to
> serialize
> the receivedmap and the page_requested tree, however that serialization does
> not cover other procedures like UFFDIO_COPY.
>
> Signed-off-by: Peter Xu
Reviewed-by: Dr. David Alan Gilbert
> ---
> migration/migration.c| 41 +++-
> migration/migration.h| 19 ++-
> migration/postcopy-ram.c | 17 ++---
> migration/trace-events | 2 ++
> 4 files changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b2dac6b39c..e7d179bffc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -143,6 +143,13 @@ static int migration_maybe_pause(MigrationState *s,
> int new_state);
> static void migrate_fd_cancel(MigrationState *s);
>
> +static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
> +{
> +unsigned long a = (unsigned long) ap, b = (unsigned long) bp;
> +
> +return (a > b) - (a < b);
> +}
> +
> void migration_object_init(void)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> @@ -165,6 +172,8 @@ void migration_object_init(void)
> qemu_event_init(_incoming->main_thread_load_event, false);
> qemu_sem_init(_incoming->postcopy_pause_sem_dst, 0);
> qemu_sem_init(_incoming->postcopy_pause_sem_fault, 0);
> +qemu_mutex_init(_incoming->page_request_mutex);
> +current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
>
> if (!migration_object_check(current_migration, )) {
> error_report_err(err);
> @@ -240,6 +249,11 @@ void migration_incoming_state_destroy(void)
>
> qemu_event_reset(>main_thread_load_event);
>
> +if (mis->page_requested) {
> +g_tree_destroy(mis->page_requested);
> +mis->page_requested = NULL;
> +}
> +
> if (mis->socket_address_list) {
> qapi_free_SocketAddressList(mis->socket_address_list);
> mis->socket_address_list = NULL;
> @@ -354,8 +368,33 @@ int
> migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
> }
>
> int migrate_send_rp_req_pages(MigrationIncomingState *mis,
> - RAMBlock *rb, ram_addr_t start)
> + RAMBlock *rb, ram_addr_t start, uint64_t haddr)
> {
> +void *aligned = (void *)(uintptr_t)(haddr & (-qemu_target_page_size()));
> +bool received;
> +
> +WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
> +received = ramblock_recv_bitmap_test_byte_offset(rb, start);
> +if (!received && !g_tree_lookup(mis->page_requested, aligned)) {
> +/*
> + * The page has not been received, and it's not yet in the page
> + * request list. Queue it. Set the value of element to 1, so
> that
> + * things like g_tree_lookup() will return TRUE (1) when found.
> + */
> +g_tree_insert(mis->page_requested, aligned, (gpointer)1);
> +mis->page_requested_count++;
> +trace_postcopy_page_req_add(aligned, mis->page_requested_count);
> +}
> +}
> +
> +/*
> + * If the page is there, skip sending the message. We don't even need
> the
> + * lock because as long as the page arrived, it'll be there forever.
> + */
> +if (received) {
> +return 0;
> +}
> +
> return migrate_send_rp_message_req_pages(mis, rb, start);
> }
>
> diff --git a/migration/migration.h b/migration/migration.h
> index
Re: [PULL v8 00/86] Misc QEMU patches for 2020-09-24
On Fri, Oct 02, 2020 at 06:27:35PM +0200, Paolo Bonzini wrote:
> On 02/10/20 17:58, Michal Prívozník wrote:
> >>
> >
> > cd442a45db60a1a72fcf980c24bd1227f13f8a87 is the first bad commit
> >
> > Sorry for noticing this earlier, but is this known? The build starts
> > failing for me after this commit:
> >
> > /usr/bin/sphinx-build -Dversion=5.1.50 -Drelease= -W
> > -Ddepfile=docs/devel.d -Ddepfile_stamp=docs/devel.stamp -b html -d
> > /home/zippy/work/qemu/qemu.git/build/docs/devel.p
> > /home/zippy/work/qemu/qemu.git/docs/devel
> > /home/zippy/work/qemu/qemu.git/build/docs/devel
> > Running Sphinx v3.2.1
> > building [mo]: targets for 0 po files that are out of date
> > building [html]: targets for 20 source files that are out of date
> > updating environment: [new config] 20 added, 0 changed, 0 removed
> > reading sources... [100%] testing
>
> No, this is new. It works with older versions of Sphinx (I have 2.2.2
> despite being on Fedora 32 which is pretty recent).
>
> For now Sphinx 3 is not supported by kerneldoc, we probably should apply
> a patch like
>
> https://www.spinics.net/lists/linux-doc/msg83277.html
We already have Sphinx 3.x hacks inside our fork of kernel-doc,
see commit 152d1967f650 ("kernel-doc: Use c:struct for Sphinx 3.0
and later").
If we want to keep deviating from upstream kernel-doc, the
following patch seems to work. Do we want to?
Signed-off-by: Eduardo Habkost
---
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 40ad782e342..03b49380426 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -838,6 +838,13 @@ sub output_function_rst(%) {
$lineprefix = "";
output_highlight_rst($args{'purpose'});
$start = "\n\n**Syntax**\n\n ``";
+} elsif ($args{'functiontype'} eq "") {
+ # this is a macro, Sphinx 3.x requires c:macro::
+ if ((split(/\./, $sphinx_version))[0] >= 3) {
+ print ".. c:macro:: ";
+ } else {
+ print ".. c:function:: ";
+ }
} else {
print ".. c:function:: ";
}
>
> Paolo
>
--
Eduardo
Re: [PULL 00/37] Block layer patches
On Fri, 2 Oct 2020 at 15:44, Kevin Wolf wrote: > > The following changes since commit 0d2a4545bf7e763984d3ee3e802617544cb7fc7a: > > Merge remote-tracking branch > 'remotes/stsquad/tags/pull-testing-and-python-021020-1' into staging > (2020-10-02 13:39:20 +0100) > > are available in the Git repository at: > > git://repo.or.cz/qemu/kevin.git tags/for-upstream > > for you to fetch changes up to c508c73dca636cc0fc7413d1e4a43fcfe4a5698c: > > qcow2: Use L1E_SIZE in qcow2_write_l1_entry() (2020-10-02 15:46:40 +0200) > > > Block layer patches: > > - Add block export infrastructure > - iotests improvements > - Document the throttle block filter > - Misc code cleanups Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
[PATCH] hw/char/bcm2835_aux: Allow less than 32-bit accesses
The "BCM2835 ARM Peripherals" datasheet [*] chapter 2
("Auxiliaries: UART1 & SPI1, SPI2"), list the register
sizes as 3/8/16/32 bits. We assume this means this
peripheral allows 8-bit accesses.
This was not an issue until commit 5d971f9e67 which reverted
("memory: accept mismatching sizes in memory_region_access_valid").
The model is implemented as 32-bit accesses (see commit 97398d900c,
all registers are 32-bit) so replace MemoryRegionOps.valid as
MemoryRegionOps.impl, and re-introduce MemoryRegionOps.valid
with a 8/32-bit range.
[*] https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
Fixes: 97398d900c ("bcm2835_aux: add emulation of BCM2835 AUX (aka UART1)
block")
Signed-off-by: Philippe Mathieu-Daudé
---
Noticed while running Trusted Firmware-A on the raspi3:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg680115.html
---
hw/char/bcm2835_aux.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
index ee3dd40e3c..dade2ab5fd 100644
--- a/hw/char/bcm2835_aux.c
+++ b/hw/char/bcm2835_aux.c
@@ -249,7 +249,9 @@ static const MemoryRegionOps bcm2835_aux_ops = {
.read = bcm2835_aux_read,
.write = bcm2835_aux_write,
.endianness = DEVICE_NATIVE_ENDIAN,
-.valid.min_access_size = 4,
+.impl.min_access_size = 4,
+.impl.max_access_size = 4,
+.valid.min_access_size = 1,
.valid.max_access_size = 4,
};
--
2.26.2
[PATCH v7 0/4] Fixes curses on msys2/mingw
V6-V7 Update the configure script for * curses: Fixes compiler error that complain don't have langinfo.h on msys2/m= ingw V5-V6 Dropping configure: Fixes ncursesw detection under msys2/mingw by convert the= m to meson first. That need the meson 0.56 upstream to fixes the curses detection. Add * configure: fixes indent of $meson setup Yonggang Luo (4): configure: fixes indent of $meson setup curses: Fixes compiler error that complain don't have langinfo.h on msys2/mingw curses: Fixes curses compiling errors. win32: Simplify gmtime_r detection not depends on if _POSIX_C_SOURCE are defined on msys2/mingw configure | 47 +-- include/sysemu/os-win32.h | 4 ++-- ui/curses.c | 14 ++-- util/oslib-win32.c| 4 ++-- 4 files changed, 16 insertions(+), 53 deletions(-) --=20 2.28.0.windows.1
[PATCH v7 2/4] curses: Fixes compiler error that complain don't have langinfo.h on msys2/mingw
msys2/mingw lacks the POSIX-required langinfo.h.
gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe -lncursesw
-lgnurx -ltre -lintl -liconv
test.c:4:10: fatal error: langinfo.h: No such file or directory
4 | #include
| ^~~~
compilation terminated.
So we using g_get_codeset instead of nl_langinfo(CODESET)
Signed-off-by: Yonggang Luo
Reviewed-by: Gerd Hoffmann
---
configure | 5 +
ui/curses.c | 10 +-
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/configure b/configure
index 8f7bdbfdd3..fa53bd5c43 100755
--- a/configure
+++ b/configure
@@ -3672,17 +3672,14 @@ if test "$curses" != "no" ; then
#include
#include
#include
-#include
int main(void) {
- const char *codeset;
wchar_t wch = L'w';
setlocale(LC_ALL, "");
resize_term(0, 0);
addwstr(L"wide chars\n");
addnwstr(, 1);
add_wch(WACS_DEGREE);
- codeset = nl_langinfo(CODESET);
- return codeset != 0;
+ return 0;
}
EOF
IFS=:
diff --git a/ui/curses.c b/ui/curses.c
index a59b23a9cf..12bc682cf9 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -30,7 +30,6 @@
#endif
#include
#include
-#include
#include
#include "qapi/error.h"
@@ -526,6 +525,7 @@ static void font_setup(void)
iconv_t nativecharset_to_ucs2;
iconv_t font_conv;
int i;
+g_autofree gchar *local_codeset = g_get_codeset();
/*
* Control characters are normally non-printable, but VGA does have
@@ -566,14 +566,14 @@ static void font_setup(void)
0x25bc
};
-ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET), "UCS-2");
+ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
if (ucs2_to_nativecharset == (iconv_t) -1) {
fprintf(stderr, "Could not convert font glyphs from UCS-2: '%s'\n",
strerror(errno));
exit(1);
}
-nativecharset_to_ucs2 = iconv_open("UCS-2", nl_langinfo(CODESET));
+nativecharset_to_ucs2 = iconv_open("UCS-2", local_codeset);
if (nativecharset_to_ucs2 == (iconv_t) -1) {
iconv_close(ucs2_to_nativecharset);
fprintf(stderr, "Could not convert font glyphs to UCS-2: '%s'\n",
@@ -581,7 +581,7 @@ static void font_setup(void)
exit(1);
}
-font_conv = iconv_open(nl_langinfo(CODESET), font_charset);
+font_conv = iconv_open(local_codeset, font_charset);
if (font_conv == (iconv_t) -1) {
iconv_close(ucs2_to_nativecharset);
iconv_close(nativecharset_to_ucs2);
@@ -602,7 +602,7 @@ static void font_setup(void)
/* DEL */
convert_ucs(0x7F, 0x2302, ucs2_to_nativecharset);
-if (strcmp(nl_langinfo(CODESET), "UTF-8")) {
+if (strcmp(local_codeset, "UTF-8")) {
/* Non-Unicode capable, use termcap equivalents for those available */
for (i = 0; i <= 0xFF; i++) {
wchar_t wch[CCHARW_MAX];
--
2.28.0.windows.1
Re: [PATCH v6 2/4] curses: Fixes compiler error that complain don't have langinfo.h on msys2/mingw
gotcha
On Sat, Oct 3, 2020 at 1:49 AM Paolo Bonzini wrote:
>
> On 02/10/20 19:47, 罗勇刚(Yonggang Luo) wrote:
> > Because the configure script change far more complicated than you
> > imgaine. And I post that before
>
> Daniel is literally asking for a two-line change:
>
> diff --git a/configure b/configure
> index fee5faa054..ffd72b571d 100755
> --- a/configure
> +++ b/configure
> @@ -3671,7 +3671,6 @@ if test "$curses" != "no" ; then
> #include
> #include
> #include
> -#include
> int main(void) {
>const char *codeset;
>wchar_t wch = L'w';
> @@ -3680,7 +3679,6 @@ int main(void) {
>addwstr(L"wide chars\n");
>addnwstr(, 1);
>add_wch(WACS_DEGREE);
> - codeset = nl_langinfo(CODESET);
>return codeset != 0;
> }
> EOF
>
> Paolo
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
[PATCH v4 3/4] migration: Maintain postcopy faulted addresses
Maintain a list of faulted addresses on the destination host for which we're
waiting on. This is implemented using a GTree rather than a real list to make
sure even there're plenty of vCPUs/threads that are faulting, the lookup will
still be fast with O(log(N)) (because we'll do that after placing each page).
It should bring a slight overhead, but ideally that shouldn't be a big problem
simply because in most cases the requested page list will be short.
Actually we did similar things for postcopy blocktime measurements. This patch
didn't use that simply because:
(1) blocktime measurement is towards vcpu threads only, but here we need to
record all faulted addresses, including main thread and external
thread (like, DPDK via vhost-user).
(2) blocktime measurement will require UFFD_FEATURE_THREAD_ID, but here we
don't want to add that extra dependency on the kernel version since not
necessary. E.g., we don't need to know which thread faulted on which
page, we also don't care about multiple threads faulting on the same
page. But we only care about what addresses are faulted so waiting for a
page copying from src.
(3) blocktime measurement is not enabled by default. However we need this by
default especially for postcopy recover.
Another thing to mention is that this patch introduced a new mutex to serialize
the receivedmap and the page_requested tree, however that serialization does
not cover other procedures like UFFDIO_COPY.
Signed-off-by: Peter Xu
---
migration/migration.c| 41 +++-
migration/migration.h| 19 ++-
migration/postcopy-ram.c | 17 ++---
migration/trace-events | 2 ++
4 files changed, 74 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index b2dac6b39c..e7d179bffc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -143,6 +143,13 @@ static int migration_maybe_pause(MigrationState *s,
int new_state);
static void migrate_fd_cancel(MigrationState *s);
+static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
+{
+unsigned long a = (unsigned long) ap, b = (unsigned long) bp;
+
+return (a > b) - (a < b);
+}
+
void migration_object_init(void)
{
MachineState *ms = MACHINE(qdev_get_machine());
@@ -165,6 +172,8 @@ void migration_object_init(void)
qemu_event_init(_incoming->main_thread_load_event, false);
qemu_sem_init(_incoming->postcopy_pause_sem_dst, 0);
qemu_sem_init(_incoming->postcopy_pause_sem_fault, 0);
+qemu_mutex_init(_incoming->page_request_mutex);
+current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
if (!migration_object_check(current_migration, )) {
error_report_err(err);
@@ -240,6 +249,11 @@ void migration_incoming_state_destroy(void)
qemu_event_reset(>main_thread_load_event);
+if (mis->page_requested) {
+g_tree_destroy(mis->page_requested);
+mis->page_requested = NULL;
+}
+
if (mis->socket_address_list) {
qapi_free_SocketAddressList(mis->socket_address_list);
mis->socket_address_list = NULL;
@@ -354,8 +368,33 @@ int
migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
}
int migrate_send_rp_req_pages(MigrationIncomingState *mis,
- RAMBlock *rb, ram_addr_t start)
+ RAMBlock *rb, ram_addr_t start, uint64_t haddr)
{
+void *aligned = (void *)(uintptr_t)(haddr & (-qemu_target_page_size()));
+bool received;
+
+WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
+received = ramblock_recv_bitmap_test_byte_offset(rb, start);
+if (!received && !g_tree_lookup(mis->page_requested, aligned)) {
+/*
+ * The page has not been received, and it's not yet in the page
+ * request list. Queue it. Set the value of element to 1, so that
+ * things like g_tree_lookup() will return TRUE (1) when found.
+ */
+g_tree_insert(mis->page_requested, aligned, (gpointer)1);
+mis->page_requested_count++;
+trace_postcopy_page_req_add(aligned, mis->page_requested_count);
+}
+}
+
+/*
+ * If the page is there, skip sending the message. We don't even need the
+ * lock because as long as the page arrived, it'll be there forever.
+ */
+if (received) {
+return 0;
+}
+
return migrate_send_rp_message_req_pages(mis, rb, start);
}
diff --git a/migration/migration.h b/migration/migration.h
index e853ccf8b1..8d2d1ce839 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -104,6 +104,23 @@ struct MigrationIncomingState {
/* List of listening socket addresses */
SocketAddressList *socket_address_list;
+
+/* A tree of pages that we requested to the source VM */
+GTree *page_requested;
+/* For
[PATCH v4 0/4] migration/postcopy: Sync faulted addresses after network recovered
v4: - use "void */ulong" instead of "uint64_t" where proper in patch 3/4 [Dave] v3: - fix build on 32bit hosts & rebase - remove r-bs for the last 2 patches for Dave due to the changes v2: - add r-bs for Dave - add patch "migration: Properly destroy variables on incoming side" as patch 1 - destroy page_request_mutex in migration_incoming_state_destroy() too [Dave] - use WITH_QEMU_LOCK_GUARD in two places where we can [Dave] We've seen conditional guest hangs on destination VM after postcopy recovered. However the hang will resolve itself after a few minutes. The problem is: after a postcopy recovery, the prioritized postcopy queue on the source VM is actually missing. So all the faulted threads before the postcopy recovery happened will keep halted until (accidentally) the page got copied by the background precopy migration stream. The solution is to also refresh this information after postcopy recovery. To achieve this, we need to maintain a list of faulted addresses on the destination node, so that we can resend the list when necessary. This work is done via patch 2-5. With that, the last thing we need to do is to send this extra information to source VM after recovered. Very luckily, this synchronization can be "emulated" by sending a bunch of page requests (although these pages have been sent previously!) to source VM just like when we've got a page fault. Even in the 1st version of the postcopy code we'll handle duplicated pages well. So this fix does not even need a new capability bit and it'll work smoothly on old QEMUs when we migrate from them to the new QEMUs. Please review, thanks. Peter Xu (4): migration: Pass incoming state into qemu_ufd_copy_ioctl() migration: Introduce migrate_send_rp_message_req_pages() migration: Maintain postcopy faulted addresses migration: Sync requested pages after postcopy recovery migration/migration.c| 49 -- migration/migration.h| 21 ++- migration/postcopy-ram.c | 25 +- migration/savevm.c | 57 migration/trace-events | 3 +++ 5 files changed, 146 insertions(+), 9 deletions(-) -- 2.26.2
Re: [PULL v8 00/86] Misc QEMU patches for 2020-09-24
On Fri, Oct 02, 2020 at 07:30:17PM +0200, Paolo Bonzini wrote:
> On 02/10/20 19:26, Michal Prívozník wrote:
> > On 10/2/20 6:22 PM, Eduardo Habkost wrote:
> >> On Fri, Oct 02, 2020 at 05:58:55PM +0200, Michal Prívozník wrote:
> >>> On 9/30/20 9:58 PM, Paolo Bonzini wrote:
>
> Eduardo Habkost (10):
> >>>
> docs: Create docs/devel/qom.rst
> >>>
> >>> cd442a45db60a1a72fcf980c24bd1227f13f8a87 is the first bad commit
> >>>
> >>> Sorry for noticing this earlier, but is this known? The build starts
> >>> failing
> >>> for me after this commit:
> >>>
> >>> /usr/bin/sphinx-build -Dversion=5.1.50 -Drelease= -W
> >>> -Ddepfile=docs/devel.d
> >>> -Ddepfile_stamp=docs/devel.stamp -b html -d
> >>> /home/zippy/work/qemu/qemu.git/build/docs/devel.p
> >>> /home/zippy/work/qemu/qemu.git/docs/devel
> >>> /home/zippy/work/qemu/qemu.git/build/docs/devel
> >>> Running Sphinx v3.2.1
> >>> building [mo]: targets for 0 po files that are out of date
> >>> building [html]: targets for 20 source files that are out of date
> >>> updating environment: [new config] 20 added, 0 changed, 0 removed
> >>> reading sources... [100%] testing
> >>>
> >>>
> >>>
> >>>
> >>> Warning, treated as error:
> >>> /home/zippy/work/qemu/qemu.git/docs/../include/qom/object.h:747:Error in
> >>> declarator
> >>> If declarator-id with parameters (e.g., 'void f(int arg)'):
> >>> Invalid C declaration: Expected identifier in nested name. [error
> >>> at 24]
> >>> object_initialize_child ( parent, propname, child, type)
> >>> ^
> >>> If parenthesis in noptr-declarator (e.g., 'void (*f(int arg))(double)'):
> >>> Error in declarator or parameters
> >>> Invalid C declaration: Expecting "(" in parameters. [error at 32]
> >>> object_initialize_child ( parent, propname, child, type)
> >>> ^
> >>>
> >>> make[1]: *** [Makefile.ninja:9898: docs/devel.stamp] Error 2
> >>> make[1]: *** Deleting file 'docs/devel.stamp'
> >>> make[1]: Leaving directory '/home/zippy/work/qemu/qemu.git/build'
> >>> make: *** [GNUmakefile:11: all] Error 2
> >>
> >> I can't reproduce it using Sphinx v2.2.2. I'm still trying to
> >> understand what exactly the error means.
> >>
> >
> > Same here.
> >
> >> I really wish we used virtualenv + requirements.txt to require a
> >> specific version of Sphinx instead of wasting time dealing a wide
> >> range of Sphinx versions.
> >>
> >
> > I already have a patch that I keep locally to build with v3:
> >
> > diff --git a/docs/qemu-option-trace.rst.inc
> > b/docs/qemu-option-trace.rst.inc
> > index 7e09773a9c..ae83f6a1a8 100644
> > --- a/docs/qemu-option-trace.rst.inc
> > +++ b/docs/qemu-option-trace.rst.inc
> > @@ -1,7 +1,7 @@
> >
> > Specify tracing options.
> >
> > -.. option:: [enable=]PATTERN
> > +.. option:: enable=PATTERN
> >
> > Immediately enable events matching *PATTERN*
> > (either event name or a globbing pattern). This option is only
> >
> >
> > That said, I'm not objecting to requiring v2 for now and switching to v3
> > later.
> >
> >
> > But interestingly, through trial and error I've came across this hack,
> > which allows me to build again. I have no idea why it works:
> >
> > diff --git i/include/qom/object.h w/include/qom/object.h
> > index 27aaa67e63..59c729ebb7 100644
> > --- i/include/qom/object.h
> > +++ w/include/qom/object.h
> > @@ -762,13 +762,14 @@ bool object_initialize_child_with_propsv(Object
> > *parentobj,
> > * child, sizeof(*child), type,
> > * _abort, NULL)
> > */
> > -#define object_initialize_child(parent, propname, child, type) \
> > - object_initialize_child_internal((parent), (propname), \
> > - (child), sizeof(*(child)), (type))
> > void object_initialize_child_internal(Object *parent, const char
> > *propname,
> > void *child, size_t size,
> > const char *type);
> >
> > +#define object_initialize_child(parent, propname, child, type) \
> > + object_initialize_child_internal((parent), (propname), \
> > + (child), sizeof(*(child)), (type))
> > +
>
> The error is due to kerneldoc treating the macro definition like a
> function, so that makes sense. If the docs look good (no reference to
> object_initialize_child_internal) then the patch can be applied.
The patch makes the document show object_initialize_child_internal().
--
Eduardo
[PATCH v4 4/4] migration: Sync requested pages after postcopy recovery
We synchronize the requested pages right after a postcopy recovery happens.
This helps to synchronize the prioritized pages on source so that the faulted
threads can be served faster.
Reported-by: Xiaohui Li
Signed-off-by: Peter Xu
---
migration/savevm.c | 57 ++
migration/trace-events | 1 +
2 files changed, 58 insertions(+)
diff --git a/migration/savevm.c b/migration/savevm.c
index 34e4b71052..1dc021ee53 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2011,6 +2011,49 @@ static int
loadvm_postcopy_handle_run(MigrationIncomingState *mis)
return LOADVM_QUIT;
}
+/* We must be with page_request_mutex held */
+static gboolean postcopy_sync_page_req(gpointer key, gpointer value,
+ gpointer data)
+{
+MigrationIncomingState *mis = data;
+void *host_addr = (void *) key;
+ram_addr_t rb_offset;
+RAMBlock *rb;
+int ret;
+
+rb = qemu_ram_block_from_host(host_addr, true, _offset);
+if (!rb) {
+/*
+ * This should _never_ happen. However be nice for a migrating VM to
+ * not crash/assert. Post an error (note: intended to not use *_once
+ * because we do want to see all the illegal addresses; and this can
+ * never be triggered by the guest so we're safe) and move on next.
+ */
+error_report("%s: illegal host addr %p", __func__, host_addr);
+/* Try the next entry */
+return FALSE;
+}
+
+ret = migrate_send_rp_message_req_pages(mis, rb, rb_offset);
+if (ret) {
+/* Please refer to above comment. */
+error_report("%s: send rp message failed for addr %p",
+ __func__, host_addr);
+return FALSE;
+}
+
+trace_postcopy_page_req_sync(host_addr);
+
+return FALSE;
+}
+
+static void migrate_send_rp_req_pages_pending(MigrationIncomingState *mis)
+{
+WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
+g_tree_foreach(mis->page_requested, postcopy_sync_page_req, mis);
+}
+}
+
static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
{
if (mis->state != MIGRATION_STATUS_POSTCOPY_RECOVER) {
@@ -2033,6 +2076,20 @@ static int
loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
/* Tell source that "we are ready" */
migrate_send_rp_resume_ack(mis, MIGRATION_RESUME_ACK_VALUE);
+/*
+ * After a postcopy recovery, the source should have lost the postcopy
+ * queue, or potentially the requested pages could have been lost during
+ * the network down phase. Let's re-sync with the source VM by re-sending
+ * all the pending pages that we eagerly need, so these threads won't get
+ * blocked too long due to the recovery.
+ *
+ * Without this procedure, the faulted destination VM threads (waiting for
+ * page requests right before the postcopy is interrupted) can keep hanging
+ * until the pages are sent by the source during the background copying of
+ * pages, or another thread faulted on the same address accidentally.
+ */
+migrate_send_rp_req_pages_pending(mis);
+
return 0;
}
diff --git a/migration/trace-events b/migration/trace-events
index e4d5eb94ca..0fbfd2da60 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -49,6 +49,7 @@ vmstate_save(const char *idstr, const char *vmsd_name) "%s,
%s"
vmstate_load(const char *idstr, const char *vmsd_name) "%s, %s"
postcopy_pause_incoming(void) ""
postcopy_pause_incoming_continued(void) ""
+postcopy_page_req_sync(void *host_addr) "sync page req %p"
# vmstate.c
vmstate_load_field_error(const char *field, int ret) "field \"%s\" load
failed, ret = %d"
--
2.26.2
[PATCH v4 1/4] migration: Pass incoming state into qemu_ufd_copy_ioctl()
It'll be used in follow up patches to access more fields out of it. Meanwhile
fetch the userfaultfd inside the function.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/postcopy-ram.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 0a2f88a87d..722034dc01 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1128,10 +1128,12 @@ int postcopy_ram_incoming_setup(MigrationIncomingState
*mis)
return 0;
}
-static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
+static int qemu_ufd_copy_ioctl(MigrationIncomingState *mis, void *host_addr,
void *from_addr, uint64_t pagesize, RAMBlock
*rb)
{
+int userfault_fd = mis->userfault_fd;
int ret;
+
if (from_addr) {
struct uffdio_copy copy_struct;
copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
@@ -1185,7 +1187,7 @@ int postcopy_place_page(MigrationIncomingState *mis, void
*host, void *from,
* which would be slightly cheaper, but we'd have to be careful
* of the order of updating our page state.
*/
-if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
+if (qemu_ufd_copy_ioctl(mis, host, from, pagesize, rb)) {
int e = errno;
error_report("%s: %s copy host: %p from: %p (size: %zd)",
__func__, strerror(e), host, from, pagesize);
@@ -1212,7 +1214,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis,
void *host,
* but it's not available for everything (e.g. hugetlbpages)
*/
if (qemu_ram_is_uf_zeroable(rb)) {
-if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, pagesize, rb)) {
+if (qemu_ufd_copy_ioctl(mis, host, NULL, pagesize, rb)) {
int e = errno;
error_report("%s: %s zero host: %p",
__func__, strerror(e), host);
--
2.26.2
[PATCH v4 2/4] migration: Introduce migrate_send_rp_message_req_pages()
This is another layer wrapper for sending a page request to the source VM. The
new migrate_send_rp_message_req_pages() will be used elsewhere in coming
patches.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 10 --
migration/migration.h | 2 ++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index aca7fdcd0b..b2dac6b39c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -316,8 +316,8 @@ error:
* Start: Address offset within the RB
* Len: Length in bytes required - must be a multiple of pagesize
*/
-int migrate_send_rp_req_pages(MigrationIncomingState *mis, RAMBlock *rb,
- ram_addr_t start)
+int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
+ RAMBlock *rb, ram_addr_t start)
{
uint8_t bufc[12 + 1 + 255]; /* start (8), len (4), rbname up to 256 */
size_t msglen = 12; /* start + len */
@@ -353,6 +353,12 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis,
RAMBlock *rb,
return migrate_send_rp_message(mis, msg_type, msglen, bufc);
}
+int migrate_send_rp_req_pages(MigrationIncomingState *mis,
+ RAMBlock *rb, ram_addr_t start)
+{
+return migrate_send_rp_message_req_pages(mis, rb, start);
+}
+
static bool migration_colo_enabled;
bool migration_incoming_colo_enabled(void)
{
diff --git a/migration/migration.h b/migration/migration.h
index deb411aaad..e853ccf8b1 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -333,6 +333,8 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
uint32_t value);
int migrate_send_rp_req_pages(MigrationIncomingState *mis, RAMBlock *rb,
ram_addr_t start);
+int migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
+ RAMBlock *rb, ram_addr_t start);
void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis,
char *block_name);
void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
--
2.26.2
Re: [PATCH v2 03/21] configure: Fixes ncursesw detection under msys2/mingw and enable curses
You can queue this instead
On Wed, Sep 9, 2020 at 5:46 PM Yonggang Luo wrote:
>
> The mingw pkg-config are showing following absolute path and contains :
as the separator,
> so we must handling : properly.
>
> -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
-IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
-pipe -lncursesw -lgnurx -ltre -lintl -liconv
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
-lncursesw
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
-lcursesw
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe
-lncursesw -lgnurx -ltre -lintl -liconv
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx -ltre
-lintl -liconv
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
>
> msys2/mingw lacks the POSIX-required langinfo.h.
>
> gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe
-lncursesw -lgnurx -ltre -lintl -liconv
> test.c:4:10: fatal error: langinfo.h: No such file or directory
> 4 | #include
> | ^~~~
> compilation terminated.
>
> So we using g_get_codeset instead of nl_langinfo(CODESET)
>
> Signed-off-by: Yonggang Luo
> Reviewed-by: Gerd Hoffmann
> ---
> configure | 9 +++--
> ui/curses.c | 10 +-
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/configure b/configure
> index f4f8bc3756..2e6d54e15b 100755
> --- a/configure
> +++ b/configure
> @@ -3653,8 +3653,8 @@ if test "$iconv" = "no" ; then
> fi
> if test "$curses" != "no" ; then
>if test "$mingw32" = "yes" ; then
> -curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> -curses_lib_list="$($pkg_config --libs ncurses
2>/dev/null):-lpdcurses"
> +curses_inc_list="$($pkg_config --cflags ncursesw
2>/dev/null):-I/${MSYSTEM,,}/include/ncursesw:"
> +curses_lib_list="$($pkg_config --libs ncursesw
2>/dev/null):-lncursesw"
>else
> curses_inc_list="$($pkg_config --cflags ncursesw
2>/dev/null):-I/usr/include/ncursesw:"
> curses_lib_list="$($pkg_config --libs ncursesw
2>/dev/null):-lncursesw:-lcursesw"
> @@ -3664,17 +3664,14 @@ if test "$curses" != "no" ; then
> #include
> #include
> #include
> -#include
> int main(void) {
> - const char *codeset;
>wchar_t wch = L'w';
>setlocale(LC_ALL, "");
>resize_term(0, 0);
>addwstr(L"wide chars\n");
>addnwstr(, 1);
>add_wch(WACS_DEGREE);
> - codeset = nl_langinfo(CODESET);
> - return codeset != 0;
> + return 0;
> }
> EOF
>IFS=:
> diff --git a/ui/curses.c b/ui/curses.c
> index a59b23a9cf..12bc682cf9 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -30,7 +30,6 @@
> #endif
> #include
> #include
> -#include
> #include
>
> #include "qapi/error.h"
> @@ -526,6 +525,7 @@ static void font_setup(void)
> iconv_t nativecharset_to_ucs2;
> iconv_t font_conv;
> int i;
> +g_autofree gchar *local_codeset = g_get_codeset();
>
> /*
> * Control characters are normally non-printable, but VGA does have
> @@ -566,14 +566,14 @@ static void font_setup(void)
>0x25bc
> };
>
> -ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET), "UCS-2");
> +ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
> if (ucs2_to_nativecharset == (iconv_t) -1) {
> fprintf(stderr, "Could not convert font glyphs from UCS-2:
'%s'\n",
> strerror(errno));
> exit(1);
> }
>
> -nativecharset_to_ucs2 = iconv_open("UCS-2", nl_langinfo(CODESET));
> +nativecharset_to_ucs2 = iconv_open("UCS-2", local_codeset);
> if (nativecharset_to_ucs2 == (iconv_t) -1) {
> iconv_close(ucs2_to_nativecharset);
> fprintf(stderr, "Could not convert font glyphs to UCS-2: '%s'\n",
> @@ -581,7 +581,7 @@ static void font_setup(void)
> exit(1);
> }
>
> -font_conv = iconv_open(nl_langinfo(CODESET), font_charset);
> +font_conv = iconv_open(local_codeset, font_charset);
> if (font_conv == (iconv_t) -1) {
> iconv_close(ucs2_to_nativecharset);
> iconv_close(nativecharset_to_ucs2);
> @@ -602,7 +602,7 @@ static void font_setup(void)
> /* DEL */
> convert_ucs(0x7F, 0x2302, ucs2_to_nativecharset);
>
> -if (strcmp(nl_langinfo(CODESET), "UTF-8")) {
> +if (strcmp(local_codeset, "UTF-8")) {
> /* Non-Unicode capable, use termcap equivalents for those
available */
> for (i = 0; i <= 0xFF; i++) {
> wchar_t wch[CCHARW_MAX];
> --
> 2.28.0.windows.1
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v6 2/4] curses: Fixes compiler error that complain don't have langinfo.h on msys2/mingw
On 02/10/20 18:42, Daniel P. Berrangé wrote: > I don't see why the configure change has any dependancy on meson 0.56. > It just requires you to remove the mentioned header file and function > from the configure check. This patch needs to include that or it is > incomplete IMHO I agree, moving the check to Meson is waiting for 0.56 but for now the configure check needs to be maintained. Paolo
Re: [PATCH v6 2/4] curses: Fixes compiler error that complain don't have langinfo.h on msys2/mingw
On 02/10/20 19:47, 罗勇刚(Yonggang Luo) wrote:
> Because the configure script change far more complicated than you
> imgaine. And I post that before
Daniel is literally asking for a two-line change:
diff --git a/configure b/configure
index fee5faa054..ffd72b571d 100755
--- a/configure
+++ b/configure
@@ -3671,7 +3671,6 @@ if test "$curses" != "no" ; then
#include
#include
#include
-#include
int main(void) {
const char *codeset;
wchar_t wch = L'w';
@@ -3680,7 +3679,6 @@ int main(void) {
addwstr(L"wide chars\n");
addnwstr(, 1);
add_wch(WACS_DEGREE);
- codeset = nl_langinfo(CODESET);
return codeset != 0;
}
EOF
Paolo
Re: [PATCH v6 2/4] curses: Fixes compiler error that complain don't have langinfo.h on msys2/mingw
On Sat, Oct 3, 2020 at 12:42 AM Daniel P. Berrangé
wrote:
>
> On Sat, Oct 03, 2020 at 12:38:50AM +0800, 罗勇刚(Yonggang Luo) wrote:
> > On Fri, Oct 2, 2020 at 11:36 PM Daniel P. Berrangé
> > wrote:
> > >
> > > On Fri, Oct 02, 2020 at 01:32:28AM +0800, Yonggang Luo wrote:
> > > > msys2/mingw lacks the POSIX-required langinfo.h.
> > > >
> > > > gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe
> > -lncursesw -lgnurx -ltre -lintl -liconv
> > > > test.c:4:10: fatal error: langinfo.h: No such file or directory
> > > > 4 | #include
> > > > | ^~~~
> > > > compilation terminated.
> > > >
> > > > So we using g_get_codeset instead of nl_langinfo(CODESET)
> > > >
> > > > Signed-off-by: Yonggang Luo
> > > > Reviewed-by: Gerd Hoffmann
> > > > ---
> > > > ui/curses.c | 10 +-
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/ui/curses.c b/ui/curses.c
> > > > index a59b23a9cf..12bc682cf9 100644
> > > > --- a/ui/curses.c
> > > > +++ b/ui/curses.c
> > > > @@ -30,7 +30,6 @@
> > > > #endif
> > > > #include
> > > > #include
> > > > -#include
> > > > #include
> > > >
> > > > #include "qapi/error.h"
> > > > @@ -526,6 +525,7 @@ static void font_setup(void)
> > > > iconv_t nativecharset_to_ucs2;
> > > > iconv_t font_conv;
> > > > int i;
> > > > +g_autofree gchar *local_codeset = g_get_codeset();
> > > >
> > > > /*
> > > > * Control characters are normally non-printable, but VGA does
have
> > > > @@ -566,14 +566,14 @@ static void font_setup(void)
> > > >0x25bc
> > > > };
> > > >
> > > > -ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET),
"UCS-2");
> > > > +ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
> > > > if (ucs2_to_nativecharset == (iconv_t) -1) {
> > > > fprintf(stderr, "Could not convert font glyphs from UCS-2:
> > '%s'\n",
> > > > strerror(errno));
> > > > exit(1);
> > > > }
> > > >
> > > > -nativecharset_to_ucs2 = iconv_open("UCS-2",
nl_langinfo(CODESET));
> > > > +nativecharset_to_ucs2 = iconv_open("UCS-2", local_codeset);
> > > > if (nativecharset_to_ucs2 == (iconv_t) -1) {
> > > > iconv_close(ucs2_to_nativecharset);
> > > > fprintf(stderr, "Could not convert font glyphs to UCS-2:
> > '%s'\n",
> > > > @@ -581,7 +581,7 @@ static void font_setup(void)
> > > > exit(1);
> > > > }
> > > >
> > > > -font_conv = iconv_open(nl_langinfo(CODESET), font_charset);
> > > > +font_conv = iconv_open(local_codeset, font_charset);
> > > > if (font_conv == (iconv_t) -1) {
> > > > iconv_close(ucs2_to_nativecharset);
> > > > iconv_close(nativecharset_to_ucs2);
> > > > @@ -602,7 +602,7 @@ static void font_setup(void)
> > > > /* DEL */
> > > > convert_ucs(0x7F, 0x2302, ucs2_to_nativecharset);
> > > >
> > > > -if (strcmp(nl_langinfo(CODESET), "UTF-8")) {
> > > > +if (strcmp(local_codeset, "UTF-8")) {
> > >
> > > If you're removing use of nl_langinfo / langinfo.h then you need
> > > to also update configure, because it is checking for this function
> > > and header file when validating curses library support.
> > The change of configure are waiting for meson 0.56, so I didn't post
that
> > yet And this patch
> > is a pre-request for msys2/mingw support and won't hurt other platform
> >
> > We are converting everything to meson, so I am not willing to change
> > configure this time
>
> I don't see why the configure change has any dependancy on meson 0.56.
> It just requires you to remove the mentioned header file and function
> from the configure check. This patch needs to include that or it is
> incomplete IMHO
>
Because the configure script change far more complicated than you imgaine.
And I post that before
> Regards,
> Daniel
> --
> |: https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
https://www.instagram.com/dberrange :|
>
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v3 4/4] migration: Sync requested pages after postcopy recovery
On Fri, Oct 02, 2020 at 06:26:46PM +0100, Dr. David Alan Gilbert wrote: > > +trace_postcopy_page_req_sync((uint64_t)(uintptr_t)host_addr); > > Again that's a case for host_addr and a %p I think. Yeah, I'll fix both places and repost. Thanks. -- Peter Xu
[PATCH v6 09/10] virtio-scsi: use scsi_device_get
From: Maxim Levitsky
This will help us to avoid the scsi device disappearing
after we took a reference to it.
It doesn't by itself forbid case when we try to access
an unrealized device
Suggested-by: Stefan Hajnoczi
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-9-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/virtio-scsi.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 971afbb217..3db9a8aae9 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -33,7 +33,7 @@ static inline int virtio_scsi_get_lun(uint8_t *lun)
return ((lun[2] << 8) | lun[3]) & 0x3FFF;
}
-static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
+static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun)
{
if (lun[0] != 1) {
return NULL;
@@ -41,7 +41,7 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI
*s, uint8_t *lun)
if (lun[2] != 0 && !(lun[2] >= 0x40 && lun[2] < 0x80)) {
return NULL;
}
-return scsi_device_find(>bus, 0, lun[1], virtio_scsi_get_lun(lun));
+return scsi_device_get(>bus, 0, lun[1], virtio_scsi_get_lun(lun));
}
void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
@@ -256,7 +256,7 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s,
SCSIDevice *d)
* case of async cancellation. */
static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
{
-SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
+SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
SCSIRequest *r, *next;
BusChild *kid;
int target;
@@ -370,10 +370,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s,
VirtIOSCSIReq *req)
rcu_read_lock();
QTAILQ_FOREACH_RCU(kid, >bus.qbus.children, sibling) {
- d = SCSI_DEVICE(kid->child);
- if (d->channel == 0 && d->id == target) {
-qdev_reset_all(>qdev);
- }
+SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+if (d1->channel == 0 && d1->id == target) {
+qdev_reset_all(>qdev);
+}
}
rcu_read_unlock();
@@ -386,14 +386,17 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s,
VirtIOSCSIReq *req)
break;
}
+object_unref(OBJECT(d));
return ret;
incorrect_lun:
req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+object_unref(OBJECT(d));
return ret;
fail:
req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+object_unref(OBJECT(d));
return ret;
}
@@ -564,7 +567,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI
*s, VirtIOSCSIReq *req)
}
}
-d = virtio_scsi_device_find(s, req->req.cmd.lun);
+d = virtio_scsi_device_get(s, req->req.cmd.lun);
if (!d) {
req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
virtio_scsi_complete_cmd_req(req);
@@ -580,10 +583,12 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI
*s, VirtIOSCSIReq *req)
req->sreq->cmd.xfer > req->qsgl.size)) {
req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
virtio_scsi_complete_cmd_req(req);
+object_unref(OBJECT(d));
return -ENOBUFS;
}
scsi_req_ref(req->sreq);
blk_io_plug(d->conf.blk);
+object_unref(OBJECT(d));
return 0;
}
--
2.26.2
[PATCH v6 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices
The device core first places a device on the bus and then realizes it.
Make scsi_device_find avoid returing such devices to avoid
races in drivers that use an iothread (currently virtio-scsi)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399
Suggested-by: Paolo Bonzini
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-7-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 83 +-
1 file changed, 53 insertions(+), 30 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 4ab9811cd8..7599113efe 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -24,6 +24,55 @@ static void scsi_target_free_buf(SCSIRequest *req);
static int next_scsi_bus;
+static SCSIDevice *do_scsi_device_find(SCSIBus *bus,
+ int channel, int id, int lun,
+ bool include_unrealized)
+{
+BusChild *kid;
+SCSIDevice *retval = NULL;
+
+QTAILQ_FOREACH_RCU(kid, >qbus.children, sibling) {
+DeviceState *qdev = kid->child;
+SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+if (dev->channel == channel && dev->id == id) {
+if (dev->lun == lun) {
+retval = dev;
+break;
+}
+
+/*
+ * If we don't find exact match (channel/bus/lun),
+ * we will return the first device which matches channel/bus
+ */
+
+if (!retval) {
+retval = dev;
+}
+}
+}
+
+/*
+ * This function might run on the IO thread and we might race against
+ * main thread hot-plugging the device.
+ * We assume that as soon as .realized is set to true we can let
+ * the user access the device.
+ */
+
+if (retval && !include_unrealized &&
+!qatomic_load_acquire(>qdev.realized)) {
+retval = NULL;
+}
+
+return retval;
+}
+
+SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
+{
+RCU_READ_LOCK_GUARD();
+return do_scsi_device_find(bus, channel, id, lun, false);
+}
+
static void scsi_device_realize(SCSIDevice *s, Error **errp)
{
SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
@@ -137,7 +186,10 @@ static bool scsi_bus_is_address_free(SCSIBus *bus,
int channel, int target, int lun,
SCSIDevice **p_dev)
{
-SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+SCSIDevice *d;
+
+RCU_READ_LOCK_GUARD();
+d = do_scsi_device_find(bus, channel, target, lun, true);
if (d && d->lun == lun) {
if (p_dev) {
*p_dev = d;
@@ -1570,35 +1622,6 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
qdev_fw_name(dev), d->id, d->lun);
}
-SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
-{
-BusChild *kid;
-SCSIDevice *target_dev = NULL;
-
-RCU_READ_LOCK_GUARD();
-QTAILQ_FOREACH_RCU(kid, >qbus.children, sibling) {
-DeviceState *qdev = kid->child;
-SCSIDevice *dev = SCSI_DEVICE(qdev);
-
-if (dev->channel == channel && dev->id == id) {
-if (dev->lun == lun) {
-return dev;
-}
-
-/*
- * If we don't find exact match (channel/bus/lun),
- * we will return the first device which matches channel/bus
- */
-
-if (!target_dev) {
-target_dev = dev;
-}
-}
-}
-
-return target_dev;
-}
-
/* SCSI request list. For simplicity, pv points to the whole device */
static int put_scsi_requests(QEMUFile *f, void *pv, size_t size,
--
2.26.2
[PATCH v6 06/10] device-core: use atomic_set on .realized property
From: Maxim Levitsky
Some code might race with placement of new devices on a bus.
We currently first place a (unrealized) device on the bus
and then realize it.
As a workaround, users that scan the child device list, can
check the realized property to see if it is safe to access such a device.
Use an atomic write here too to aid with this.
A separate discussion is what to do with devices that are unrealized:
It looks like for this case we only call the hotplug handler's unplug
callback and its up to it to unrealize the device.
An atomic operation doesn't cause harm for this code path though.
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-6-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/core/qdev.c | 19 ++-
include/hw/qdev-core.h | 2 ++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 59e5e710b7..fc4daa36fa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -946,7 +946,25 @@ static void device_set_realized(Object *obj, bool value,
Error **errp)
}
}
+ qatomic_store_release(>realized, value);
+
} else if (!value && dev->realized) {
+
+/*
+ * Change the value so that any concurrent users are aware
+ * that the device is going to be unrealized
+ *
+ * TODO: change .realized property to enum that states
+ * each phase of the device realization/unrealization
+ */
+
+qatomic_set(>realized, value);
+/*
+ * Ensure that concurrent users see this update prior to
+ * any other changes done by unrealize.
+ */
+smp_wmb();
+
QLIST_FOREACH(bus, >child_bus, sibling) {
qbus_unrealize(bus);
}
@@ -961,7 +979,6 @@ static void device_set_realized(Object *obj, bool value,
Error **errp)
}
assert(local_err == NULL);
-dev->realized = value;
return;
child_realize_fail:
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2c6307e3ed..868973319e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -163,6 +163,8 @@ struct NamedClockList {
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
+ *When accessed outsize big qemu lock, must be accessed with
+ *atomic_load_acquire()
* @reset: ResettableState for the device; handled by Resettable interface.
*
* This structure should not be accessed directly. We declare it here
--
2.26.2
[PATCH v6 05/10] device-core: use RCU for list of children of a bus
From: Maxim Levitsky
This fixes the race between device emulation code that tries to find
a child device to dispatch the request to (e.g a scsi disk),
and hotplug of a new device to that bus.
Note that this doesn't convert all the readers of the list
but only these that might go over that list without BQL held.
This is a very small first step to make this code thread safe.
Suggested-by: Paolo Bonzini
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-5-mlevi...@redhat.com>
[Use RCU_READ_LOCK_GUARD in more places, adjust testcase now that
the delay in DEVICE_DELETED due to RCU is more consistent. - Paolo]
Signed-off-by: Paolo Bonzini
---
hw/core/bus.c| 28 ---
hw/core/qdev.c | 37 ++--
hw/scsi/scsi-bus.c | 12 +---
hw/scsi/virtio-scsi.c| 6 +-
include/hw/qdev-core.h | 9 +
tests/qtest/drive_del-test.c | 10 +++---
6 files changed, 66 insertions(+), 36 deletions(-)
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 6b987b6946..a0483859ae 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -49,12 +49,14 @@ int qbus_walk_children(BusState *bus,
}
}
-QTAILQ_FOREACH(kid, >children, sibling) {
-err = qdev_walk_children(kid->child,
- pre_devfn, pre_busfn,
- post_devfn, post_busfn, opaque);
-if (err < 0) {
-return err;
+WITH_RCU_READ_LOCK_GUARD() {
+QTAILQ_FOREACH_RCU(kid, >children, sibling) {
+err = qdev_walk_children(kid->child,
+ pre_devfn, pre_busfn,
+ post_devfn, post_busfn, opaque);
+if (err < 0) {
+return err;
+}
}
}
@@ -90,8 +92,10 @@ static void bus_reset_child_foreach(Object *obj,
ResettableChildCallback cb,
BusState *bus = BUS(obj);
BusChild *kid;
-QTAILQ_FOREACH(kid, >children, sibling) {
-cb(OBJECT(kid->child), opaque, type);
+WITH_RCU_READ_LOCK_GUARD() {
+QTAILQ_FOREACH_RCU(kid, >children, sibling) {
+cb(OBJECT(kid->child), opaque, type);
+}
}
}
@@ -194,9 +198,11 @@ static void bus_set_realized(Object *obj, bool value,
Error **errp)
/* TODO: recursive realization */
} else if (!value && bus->realized) {
-QTAILQ_FOREACH(kid, >children, sibling) {
-DeviceState *dev = kid->child;
-qdev_unrealize(dev);
+WITH_RCU_READ_LOCK_GUARD() {
+QTAILQ_FOREACH_RCU(kid, >children, sibling) {
+DeviceState *dev = kid->child;
+qdev_unrealize(dev);
+}
}
if (bc->unrealize) {
bc->unrealize(bus);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 74db78df36..59e5e710b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -51,6 +51,12 @@ const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
return dc->vmsd;
}
+static void bus_free_bus_child(BusChild *kid)
+{
+object_unref(OBJECT(kid->child));
+g_free(kid);
+}
+
static void bus_remove_child(BusState *bus, DeviceState *child)
{
BusChild *kid;
@@ -60,15 +66,16 @@ static void bus_remove_child(BusState *bus, DeviceState
*child)
char name[32];
snprintf(name, sizeof(name), "child[%d]", kid->index);
-QTAILQ_REMOVE(>children, kid, sibling);
+QTAILQ_REMOVE_RCU(>children, kid, sibling);
bus->num_children--;
/* This gives back ownership of kid->child back to us. */
object_property_del(OBJECT(bus), name);
-object_unref(OBJECT(kid->child));
-g_free(kid);
-return;
+
+/* free the bus kid, when it is safe to do so*/
+call_rcu(kid, bus_free_bus_child, rcu);
+break;
}
}
}
@@ -83,7 +90,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
kid->child = child;
object_ref(OBJECT(kid->child));
-QTAILQ_INSERT_HEAD(>children, kid, sibling);
+QTAILQ_INSERT_HEAD_RCU(>children, kid, sibling);
/* This transfers ownership of kid->child to the property. */
snprintf(name, sizeof(name), "child[%d]", kid->index);
@@ -672,17 +679,19 @@ DeviceState *qdev_find_recursive(BusState *bus, const
char *id)
DeviceState *ret;
BusState *child;
-QTAILQ_FOREACH(kid, >children, sibling) {
-DeviceState *dev = kid->child;
+WITH_RCU_READ_LOCK_GUARD() {
+QTAILQ_FOREACH_RCU(kid, >children, sibling) {
+DeviceState *dev = kid->child;
-if (dev->id && strcmp(dev->id, id) == 0) {
-return dev;
-}
+if (dev->id && strcmp(dev->id, id) == 0) {
+return dev;
+}
-QLIST_FOREACH(child, >child_bus, sibling) {
-
[PATCH v6 03/10] scsi/scsi_bus: switch search direction in scsi_device_find
From: Maxim Levitsky
This change will allow us to convert the bus children list to RCU,
while not changing the logic of this function
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-2-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 94921c04b1..69d7c3f90c 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1571,7 +1571,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel,
int id, int lun)
BusChild *kid;
SCSIDevice *target_dev = NULL;
-QTAILQ_FOREACH_REVERSE(kid, >qbus.children, sibling) {
+QTAILQ_FOREACH(kid, >qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -1579,7 +1579,15 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel,
int id, int lun)
if (dev->lun == lun) {
return dev;
}
-target_dev = dev;
+
+/*
+ * If we don't find exact match (channel/bus/lun),
+ * we will return the first device which matches channel/bus
+ */
+
+if (!target_dev) {
+target_dev = dev;
+}
}
}
return target_dev;
--
2.26.2
[PATCH v6 10/10] scsi/scsi_bus: fix races in REPORT LUNS
From: Maxim Levitsky
Currently scsi_target_emulate_report_luns iterates over the child device list
twice, and there is no guarantee that this list is the same in both iterations.
The reason for iterating twice is that the first iteration calculates
how much memory to allocate. However if we use a dynamic array we can
avoid iterating twice, and therefore we avoid this race.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
Signed-off-by: Maxim Levitsky
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-10-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 68 ++
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index eda8cb7e70..b901e701f0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -438,19 +438,23 @@ struct SCSITargetReq {
static void store_lun(uint8_t *outbuf, int lun)
{
if (lun < 256) {
+/* Simple logical unit addressing method*/
+outbuf[0] = 0;
outbuf[1] = lun;
-return;
+} else {
+/* Flat space addressing method */
+outbuf[0] = 0x40 | (lun >> 8);
+outbuf[1] = (lun & 255);
}
-outbuf[1] = (lun & 255);
-outbuf[0] = (lun >> 8) | 0x40;
}
static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
{
BusChild *kid;
-int i, len, n;
int channel, id;
-bool found_lun0;
+uint8_t tmp[8] = {0};
+int len = 0;
+GByteArray *buf;
if (r->req.cmd.xfer < 16) {
return false;
@@ -458,46 +462,40 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq
*r)
if (r->req.cmd.buf[2] > 2) {
return false;
}
+
+/* reserve space for 63 LUNs*/
+buf = g_byte_array_sized_new(512);
+
channel = r->req.dev->channel;
id = r->req.dev->id;
-found_lun0 = false;
-n = 0;
-RCU_READ_LOCK_GUARD();
+/* add size (will be updated later to correct value */
+g_byte_array_append(buf, tmp, 8);
+len += 8;
-QTAILQ_FOREACH_RCU(kid, >req.bus->qbus.children, sibling) {
-DeviceState *qdev = kid->child;
-SCSIDevice *dev = SCSI_DEVICE(qdev);
+/* add LUN0 */
+g_byte_array_append(buf, tmp, 8);
+len += 8;
-if (dev->channel == channel && dev->id == id) {
-if (dev->lun == 0) {
-found_lun0 = true;
+WITH_RCU_READ_LOCK_GUARD() {
+QTAILQ_FOREACH_RCU(kid, >req.bus->qbus.children, sibling) {
+DeviceState *qdev = kid->child;
+SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+if (dev->channel == channel && dev->id == id && dev->lun != 0) {
+store_lun(tmp, dev->lun);
+g_byte_array_append(buf, tmp, 8);
+len += 8;
}
-n += 8;
}
}
-if (!found_lun0) {
-n += 8;
-}
-
-scsi_target_alloc_buf(>req, n + 8);
-
-len = MIN(n + 8, r->req.cmd.xfer & ~7);
-memset(r->buf, 0, len);
-stl_be_p(>buf[0], n);
-i = found_lun0 ? 8 : 16;
-QTAILQ_FOREACH_RCU(kid, >req.bus->qbus.children, sibling) {
-DeviceState *qdev = kid->child;
-SCSIDevice *dev = SCSI_DEVICE(qdev);
-if (dev->channel == channel && dev->id == id) {
-store_lun(>buf[i], dev->lun);
-i += 8;
-}
-}
+r->buf_len = len;
+r->buf = g_byte_array_free(buf, FALSE);
+r->len = MIN(len, r->req.cmd.xfer & ~7);
-assert(i == n + 8);
-r->len = len;
+/* store the LUN list length */
+stl_be_p(>buf[0], len - 8);
return true;
}
--
2.26.2
[PATCH v6 04/10] device_core: use drain_call_rcu in in qmp_device_add
From: Maxim Levitsky
Soon, a device removal might only happen on RCU callback execution.
This is okay for device-del which provides a DEVICE_DELETED event,
but not for the failure case of device-add. To avoid changing
monitor semantics, just drain all pending RCU callbacks on error.
Signed-off-by: Maxim Levitsky
Suggested-by: Stefan Hajnoczi
Reviewed-by: Stefan Hajnoczi
Message-Id: <20200913160259.32145-4-mlevi...@redhat.com>
[Don't use it in qmp_device_del. - Paolo]
Signed-off-by: Paolo Bonzini
---
qdev-monitor.c | 12
1 file changed, 12 insertions(+)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e9b7228480..bcfb90a08f 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
Error **errp)
return;
}
dev = qdev_device_add(opts, errp);
+
+/*
+ * Drain all pending RCU callbacks. This is done because
+ * some bus related operations can delay a device removal
+ * (in this case this can happen if device is added and then
+ * removed due to a configuration error)
+ * to a RCU callback, but user might expect that this interface
+ * will finish its job completely once qmp command returns result
+ * to the user
+ */
+drain_call_rcu();
+
if (!dev) {
qemu_opts_del(opts);
return;
--
2.26.2
[PATCH v6 08/10] scsi/scsi_bus: Add scsi_device_get
From: Maxim Levitsky
Add scsi_device_get which finds the scsi device
and takes a reference to it.
Suggested-by: Stefan Hajnoczi
Signed-off-by: Maxim Levitsky
Message-Id: <20200913160259.32145-8-mlevi...@redhat.com>
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 11 +++
include/hw/scsi/scsi.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7599113efe..eda8cb7e70 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int
id, int lun)
return do_scsi_device_find(bus, channel, id, lun, false);
}
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
+{
+SCSIDevice *d;
+RCU_READ_LOCK_GUARD();
+d = do_scsi_device_find(bus, channel, id, lun, false);
+if (d) {
+object_ref(d);
+}
+return d;
+}
+
static void scsi_device_realize(SCSIDevice *s, Error **errp)
{
SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 7a55cdbd74..09fa5c9d2a 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf,
int len, bool fixed);
int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size,
uint8_t *buf, uint8_t buf_size);
SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun);
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun);
/* scsi-generic.c. */
extern const SCSIReqOps scsi_generic_req_ops;
--
2.26.2
[PATCH v6 02/10] scsi: switch to bus->check_address
Signed-off-by: Paolo Bonzini
---
hw/scsi/scsi-bus.c | 122 -
1 file changed, 75 insertions(+), 47 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3284a5d1fb..94921c04b1 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -22,33 +22,6 @@ static void scsi_req_dequeue(SCSIRequest *req);
static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
static void scsi_target_free_buf(SCSIRequest *req);
-static Property scsi_props[] = {
-DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
-DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
-DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
-DEFINE_PROP_END_OF_LIST(),
-};
-
-static void scsi_bus_class_init(ObjectClass *klass, void *data)
-{
-BusClass *k = BUS_CLASS(klass);
-HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
-
-k->get_dev_path = scsibus_get_dev_path;
-k->get_fw_dev_path = scsibus_get_fw_dev_path;
-hc->unplug = qdev_simple_device_unplug_cb;
-}
-
-static const TypeInfo scsi_bus_info = {
-.name = TYPE_SCSI_BUS,
-.parent = TYPE_BUS,
-.instance_size = sizeof(SCSIBus),
-.class_init = scsi_bus_class_init,
-.interfaces = (InterfaceInfo[]) {
-{ TYPE_HOTPLUG_HANDLER },
-{ }
-}
-};
static int next_scsi_bus;
static void scsi_device_realize(SCSIDevice *s, Error **errp)
@@ -160,35 +133,68 @@ static void scsi_dma_restart_cb(void *opaque, int
running, RunState state)
}
}
-static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+static bool scsi_bus_is_address_free(SCSIBus *bus,
+int channel, int target, int lun,
+SCSIDevice **p_dev)
+{
+SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+if (d && d->lun == lun) {
+if (p_dev) {
+*p_dev = d;
+}
+return false;
+}
+if (p_dev) {
+*p_dev = NULL;
+}
+return true;
+}
+
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error
**errp)
{
SCSIDevice *dev = SCSI_DEVICE(qdev);
-SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
-SCSIDevice *d;
-Error *local_err = NULL;
+SCSIBus *bus = SCSI_BUS(qbus);
if (dev->channel > bus->info->max_channel) {
error_setg(errp, "bad scsi channel id: %d", dev->channel);
-return;
+return false;
}
if (dev->id != -1 && dev->id > bus->info->max_target) {
error_setg(errp, "bad scsi device id: %d", dev->id);
-return;
+return false;
}
if (dev->lun != -1 && dev->lun > bus->info->max_lun) {
error_setg(errp, "bad scsi device lun: %d", dev->lun);
-return;
+return false;
+}
+
+if (dev->id != -1 && dev->lun != -1) {
+SCSIDevice *d;
+if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun,
)) {
+error_setg(errp, "lun already used by '%s'", d->qdev.id);
+return false;
+}
}
+return true;
+}
+
+static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+{
+SCSIDevice *dev = SCSI_DEVICE(qdev);
+SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+bool is_free;
+Error *local_err = NULL;
+
if (dev->id == -1) {
int id = -1;
if (dev->lun == -1) {
dev->lun = 0;
}
do {
-d = scsi_device_find(bus, dev->channel, ++id, dev->lun);
-} while (d && d->lun == dev->lun && id < bus->info->max_target);
-if (d && d->lun == dev->lun) {
+is_free = scsi_bus_is_address_free(bus, dev->channel, ++id,
dev->lun, NULL);
+} while (!is_free && id < bus->info->max_target);
+if (!is_free) {
error_setg(errp, "no free target");
return;
}
@@ -196,20 +202,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error
**errp)
} else if (dev->lun == -1) {
int lun = -1;
do {
-d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
-} while (d && d->lun == lun && lun < bus->info->max_lun);
-if (d && d->lun == lun) {
+is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id,
++lun, NULL);
+} while (!is_free && lun < bus->info->max_lun);
+if (!is_free) {
error_setg(errp, "no free lun");
return;
}
dev->lun = lun;
-} else {
-d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
-assert(d);
-if (d->lun == dev->lun && dev != d) {
-error_setg(errp, "lun already used by '%s'", d->qdev.id);
-return;
-}
}
QTAILQ_INIT(>requests);
@@ -1709,6 +1708,13 @@ const VMStateDescription vmstate_scsi_device = {
}
};
+static Property scsi_props[] = {
+DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
+
[PATCH v6 01/10] qdev: add "check if address free" callback for buses
Check if an address is free on the bus before plugging in the
device. This makes it possible to do the check without any
side effects, and to detect the problem early without having
to do it in the realize callback.
Signed-off-by: Paolo Bonzini
---
hw/core/qdev.c | 17 +++--
hw/net/virtio-net.c| 2 +-
hw/sd/core.c | 3 ++-
include/hw/qdev-core.h | 13 -
4 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 96772a15bd..74db78df36 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -94,13 +94,23 @@ static void bus_add_child(BusState *bus, DeviceState *child)
0);
}
-void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
+static bool bus_check_address(BusState *bus, DeviceState *child, Error **errp)
+{
+BusClass *bc = BUS_GET_CLASS(bus);
+return !bc->check_address || bc->check_address(bus, child, errp);
+}
+
+bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
{
BusState *old_parent_bus = dev->parent_bus;
DeviceClass *dc = DEVICE_GET_CLASS(dev);
assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
+if (!bus_check_address(bus, dev, errp)) {
+return false;
+}
+
if (old_parent_bus) {
trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
old_parent_bus, object_get_typename(OBJECT(old_parent_bus)),
@@ -126,6 +136,7 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
object_unref(OBJECT(old_parent_bus));
object_unref(OBJECT(dev));
}
+return true;
}
DeviceState *qdev_new(const char *name)
@@ -371,7 +382,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error
**errp)
assert(!dev->realized && !dev->parent_bus);
if (bus) {
-qdev_set_parent_bus(dev, bus);
+if (!qdev_set_parent_bus(dev, bus, errp)) {
+return false;
+}
} else {
assert(!DEVICE_GET_CLASS(dev)->bus_type);
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7bf27b9db7..268cecc498 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3142,7 +3142,7 @@ static bool failover_replug_primary(VirtIONet *n, Error
**errp)
error_setg(errp, "virtio_net: couldn't find primary bus");
return false;
}
-qdev_set_parent_bus(n->primary_dev, n->primary_bus);
+qdev_set_parent_bus(n->primary_dev, n->primary_bus, _abort);
n->primary_should_be_hidden = false;
if (!qemu_opt_set_bool(n->primary_device_opts,
"partially_hotplugged", true, errp)) {
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 957d116f1a..08c93b5903 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -23,6 +23,7 @@
#include "hw/qdev-core.h"
#include "hw/sd/sd.h"
#include "qemu/module.h"
+#include "qapi/error.h"
#include "trace.h"
static inline const char *sdbus_name(SDBus *sdbus)
@@ -240,7 +241,7 @@ void sdbus_reparent_card(SDBus *from, SDBus *to)
readonly = sc->get_readonly(card);
sdbus_set_inserted(from, false);
-qdev_set_parent_bus(DEVICE(card), >qbus);
+qdev_set_parent_bus(DEVICE(card), >qbus, _abort);
sdbus_set_inserted(to, true);
sdbus_set_readonly(to, readonly);
}
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 72064f4dd4..14d476c587 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -210,13 +210,24 @@ struct BusClass {
/* FIXME first arg should be BusState */
void (*print_dev)(Monitor *mon, DeviceState *dev, int indent);
char *(*get_dev_path)(DeviceState *dev);
+
/*
* This callback is used to create Open Firmware device path in accordance
* with OF spec http://forthworks.com/standards/of1275.pdf. Individual bus
* bindings can be found at http://playground.sun.com/1275/bindings/.
*/
char *(*get_fw_dev_path)(DeviceState *dev);
+
void (*reset)(BusState *bus);
+
+/*
+ * Return whether the device can be added to @bus,
+ * based on the address that was set (via device properties)
+ * before realize. If not, on return @errp contains the
+ * human-readable error message.
+ */
+bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
+
BusRealize realize;
BusUnrealize unrealize;
@@ -788,7 +799,7 @@ const char *qdev_fw_name(DeviceState *dev);
Object *qdev_get_machine(void);
/* FIXME: make this a link<> */
-void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
+bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
extern bool qdev_hotplug;
extern bool qdev_hot_removed;
--
2.26.2
[PATCH v6 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
changes from previous version: - add comment to patch 1 - fix testsuite breakage by not using drain_call_rcu in qmp_device_del Maxim Levitsky (7): scsi/scsi_bus: switch search direction in scsi_device_find device_core: use drain_call_rcu in in qmp_device_add device-core: use RCU for list of children of a bus device-core: use atomic_set on .realized property scsi/scsi_bus: Add scsi_device_get virtio-scsi: use scsi_device_get scsi/scsi_bus: fix races in REPORT LUNS Paolo Bonzini (3): qdev: add "check if address free" callback for buses scsi: switch to bus->check_address scsi/scsi-bus: scsi_device_find: don't return unrealized devices hw/core/bus.c| 28 ++-- hw/core/qdev.c | 73 +++--- hw/net/virtio-net.c | 2 +- hw/scsi/scsi-bus.c | 262 ++- hw/scsi/virtio-scsi.c| 27 ++-- hw/sd/core.c | 3 +- include/hw/qdev-core.h | 24 +++- include/hw/scsi/scsi.h | 1 + qdev-monitor.c | 12 ++ tests/qtest/drive_del-test.c | 10 +- 10 files changed, 301 insertions(+), 141 deletions(-) -- 2.26.2
Re: [PULL v8 00/86] Misc QEMU patches for 2020-09-24
On 02/10/20 19:26, Michal Prívozník wrote:
> On 10/2/20 6:22 PM, Eduardo Habkost wrote:
>> On Fri, Oct 02, 2020 at 05:58:55PM +0200, Michal Prívozník wrote:
>>> On 9/30/20 9:58 PM, Paolo Bonzini wrote:
Eduardo Habkost (10):
>>>
docs: Create docs/devel/qom.rst
>>>
>>> cd442a45db60a1a72fcf980c24bd1227f13f8a87 is the first bad commit
>>>
>>> Sorry for noticing this earlier, but is this known? The build starts
>>> failing
>>> for me after this commit:
>>>
>>> /usr/bin/sphinx-build -Dversion=5.1.50 -Drelease= -W
>>> -Ddepfile=docs/devel.d
>>> -Ddepfile_stamp=docs/devel.stamp -b html -d
>>> /home/zippy/work/qemu/qemu.git/build/docs/devel.p
>>> /home/zippy/work/qemu/qemu.git/docs/devel
>>> /home/zippy/work/qemu/qemu.git/build/docs/devel
>>> Running Sphinx v3.2.1
>>> building [mo]: targets for 0 po files that are out of date
>>> building [html]: targets for 20 source files that are out of date
>>> updating environment: [new config] 20 added, 0 changed, 0 removed
>>> reading sources... [100%] testing
>>>
>>>
>>>
>>>
>>> Warning, treated as error:
>>> /home/zippy/work/qemu/qemu.git/docs/../include/qom/object.h:747:Error in
>>> declarator
>>> If declarator-id with parameters (e.g., 'void f(int arg)'):
>>> Invalid C declaration: Expected identifier in nested name. [error
>>> at 24]
>>> object_initialize_child ( parent, propname, child, type)
>>> ^
>>> If parenthesis in noptr-declarator (e.g., 'void (*f(int arg))(double)'):
>>> Error in declarator or parameters
>>> Invalid C declaration: Expecting "(" in parameters. [error at 32]
>>> object_initialize_child ( parent, propname, child, type)
>>> ^
>>>
>>> make[1]: *** [Makefile.ninja:9898: docs/devel.stamp] Error 2
>>> make[1]: *** Deleting file 'docs/devel.stamp'
>>> make[1]: Leaving directory '/home/zippy/work/qemu/qemu.git/build'
>>> make: *** [GNUmakefile:11: all] Error 2
>>
>> I can't reproduce it using Sphinx v2.2.2. I'm still trying to
>> understand what exactly the error means.
>>
>
> Same here.
>
>> I really wish we used virtualenv + requirements.txt to require a
>> specific version of Sphinx instead of wasting time dealing a wide
>> range of Sphinx versions.
>>
>
> I already have a patch that I keep locally to build with v3:
>
> diff --git a/docs/qemu-option-trace.rst.inc
> b/docs/qemu-option-trace.rst.inc
> index 7e09773a9c..ae83f6a1a8 100644
> --- a/docs/qemu-option-trace.rst.inc
> +++ b/docs/qemu-option-trace.rst.inc
> @@ -1,7 +1,7 @@
>
> Specify tracing options.
>
> -.. option:: [enable=]PATTERN
> +.. option:: enable=PATTERN
>
> Immediately enable events matching *PATTERN*
> (either event name or a globbing pattern). This option is only
>
>
> That said, I'm not objecting to requiring v2 for now and switching to v3
> later.
>
>
> But interestingly, through trial and error I've came across this hack,
> which allows me to build again. I have no idea why it works:
>
> diff --git i/include/qom/object.h w/include/qom/object.h
> index 27aaa67e63..59c729ebb7 100644
> --- i/include/qom/object.h
> +++ w/include/qom/object.h
> @@ -762,13 +762,14 @@ bool object_initialize_child_with_propsv(Object
> *parentobj,
> * child, sizeof(*child), type,
> * _abort, NULL)
> */
> -#define object_initialize_child(parent, propname, child, type) \
> - object_initialize_child_internal((parent), (propname), \
> - (child), sizeof(*(child)), (type))
> void object_initialize_child_internal(Object *parent, const char
> *propname,
> void *child, size_t size,
> const char *type);
>
> +#define object_initialize_child(parent, propname, child, type) \
> + object_initialize_child_internal((parent), (propname), \
> + (child), sizeof(*(child)), (type))
> +
The error is due to kerneldoc treating the macro definition like a
function, so that makes sense. If the docs look good (no reference to
object_initialize_child_internal) then the patch can be applied.
Paolo
Re: [PULL v8 00/86] Misc QEMU patches for 2020-09-24
On 10/2/20 6:22 PM, Eduardo Habkost wrote:
On Fri, Oct 02, 2020 at 05:58:55PM +0200, Michal Prívozník wrote:
On 9/30/20 9:58 PM, Paolo Bonzini wrote:
Eduardo Habkost (10):
docs: Create docs/devel/qom.rst
cd442a45db60a1a72fcf980c24bd1227f13f8a87 is the first bad commit
Sorry for noticing this earlier, but is this known? The build starts failing
for me after this commit:
/usr/bin/sphinx-build -Dversion=5.1.50 -Drelease= -W -Ddepfile=docs/devel.d
-Ddepfile_stamp=docs/devel.stamp -b html -d
/home/zippy/work/qemu/qemu.git/build/docs/devel.p
/home/zippy/work/qemu/qemu.git/docs/devel
/home/zippy/work/qemu/qemu.git/build/docs/devel
Running Sphinx v3.2.1
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 20 source files that are out of date
updating environment: [new config] 20 added, 0 changed, 0 removed
reading sources... [100%] testing
Warning, treated as error:
/home/zippy/work/qemu/qemu.git/docs/../include/qom/object.h:747:Error in
declarator
If declarator-id with parameters (e.g., 'void f(int arg)'):
Invalid C declaration: Expected identifier in nested name. [error at 24]
object_initialize_child ( parent, propname, child, type)
^
If parenthesis in noptr-declarator (e.g., 'void (*f(int arg))(double)'):
Error in declarator or parameters
Invalid C declaration: Expecting "(" in parameters. [error at 32]
object_initialize_child ( parent, propname, child, type)
^
make[1]: *** [Makefile.ninja:9898: docs/devel.stamp] Error 2
make[1]: *** Deleting file 'docs/devel.stamp'
make[1]: Leaving directory '/home/zippy/work/qemu/qemu.git/build'
make: *** [GNUmakefile:11: all] Error 2
I can't reproduce it using Sphinx v2.2.2. I'm still trying to
understand what exactly the error means.
Same here.
I really wish we used virtualenv + requirements.txt to require a
specific version of Sphinx instead of wasting time dealing a wide
range of Sphinx versions.
I already have a patch that I keep locally to build with v3:
diff --git a/docs/qemu-option-trace.rst.inc b/docs/qemu-option-trace.rst.inc
index 7e09773a9c..ae83f6a1a8 100644
--- a/docs/qemu-option-trace.rst.inc
+++ b/docs/qemu-option-trace.rst.inc
@@ -1,7 +1,7 @@
Specify tracing options.
-.. option:: [enable=]PATTERN
+.. option:: enable=PATTERN
Immediately enable events matching *PATTERN*
(either event name or a globbing pattern). This option is only
That said, I'm not objecting to requiring v2 for now and switching to v3
later.
But interestingly, through trial and error I've came across this hack,
which allows me to build again. I have no idea why it works:
diff --git i/include/qom/object.h w/include/qom/object.h
index 27aaa67e63..59c729ebb7 100644
--- i/include/qom/object.h
+++ w/include/qom/object.h
@@ -762,13 +762,14 @@ bool object_initialize_child_with_propsv(Object
*parentobj,
* child, sizeof(*child), type,
* _abort, NULL)
*/
-#define object_initialize_child(parent, propname, child, type) \
-object_initialize_child_internal((parent), (propname), \
- (child), sizeof(*(child)), (type))
void object_initialize_child_internal(Object *parent, const char
*propname,
void *child, size_t size,
const char *type);
+#define object_initialize_child(parent, propname, child, type) \
+object_initialize_child_internal((parent), (propname), \
+ (child), sizeof(*(child)), (type))
+
/**
* object_dynamic_cast:
* @obj: The object to cast.
[PATCH v3 08/11] tests/9pfs: introduce local tests
This patch introduces 9pfs test cases using the 9pfs 'local'
filesystem driver which reads/writes/creates/deletes real files
and directories.
In this initial version, there is only one local test which actually
only checks if the 9pfs 'local' device was created successfully.
Before the 9pfs 'local' tests are run, a test directory 'qtest-9p-local'
is created (with world rwx permissions) under the current working
directory. At this point that test directory is not auto deleted yet.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/virtio-9p.c | 71 ++
tests/qtest/libqos/virtio-9p.h | 5 +++
tests/qtest/virtio-9p-test.c | 44 ++---
3 files changed, 106 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 2e300063e3..1bada47af1 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -24,6 +24,34 @@
#include "qgraph.h"
static QGuestAllocator *alloc;
+static char *local_test_path;
+
+/* Concatenates the passed 2 pathes. Returned result must be freed. */
+static char *concat_path(const char* a, const char* b)
+{
+return g_build_filename(a, b, NULL);
+}
+
+static void init_local_test_path(void)
+{
+char *pwd = get_current_dir_name();
+local_test_path = concat_path(pwd, "qtest-9p-local");
+free(pwd);
+}
+
+/* Creates the directory for the 9pfs 'local' filesystem driver to access. */
+static void create_local_test_dir(void)
+{
+struct stat st;
+
+g_assert(local_test_path != NULL);
+mkdir(local_test_path, 0777);
+
+/* ensure test directory exists now ... */
+g_assert(stat(local_test_path, ) == 0);
+/* ... and is actually a directory */
+g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+}
static void virtio_9p_cleanup(QVirtio9P *interface)
{
@@ -146,11 +174,54 @@ static void *virtio_9p_pci_create(void *pci_bus,
QGuestAllocator *t_alloc,
return obj;
}
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
+{
+GRegex *regex;
+char *s, *arg_repl;
+
+g_assert_nonnull(local_test_path);
+
+/* replace 'synth' driver by 'local' driver */
+regex = g_regex_new("-fsdev synth,", 0, 0, NULL);
+s = g_regex_replace_literal(
+regex, cmd_line->str, -1, 0, "-fsdev local,", 0, NULL
+);
+g_string_assign(cmd_line, s);
+g_free(s);
+g_regex_unref(regex);
+
+/* add 'path=...' to '-fsdev ...' group */
+regex = g_regex_new("(-fsdev \\w+)(\\s*)", 0, 0, NULL);
+arg_repl = g_strdup_printf("\\1\\2,path='%s'", local_test_path);
+s = g_regex_replace(
+regex, cmd_line->str, -1, 0, arg_repl, 0, NULL
+);
+g_string_assign(cmd_line, s);
+g_free(arg_repl);
+g_free(s);
+g_regex_unref(regex);
+
+/* add passed args to '-fsdev ...' group */
+regex = g_regex_new("(-fsdev \\w+)(\\s*)", 0, 0, NULL);
+arg_repl = g_strdup_printf("\\1\\2,%s", args);
+s = g_regex_replace(
+regex, cmd_line->str, -1, 0, arg_repl, 0, NULL
+);
+g_string_assign(cmd_line, s);
+g_free(arg_repl);
+g_free(s);
+g_regex_unref(regex);
+}
+
static void virtio_9p_register_nodes(void)
{
const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
+/* make sure test dir for the 'local' tests exists and is clean */
+init_local_test_path();
+create_local_test_dir();
+
QPCIAddress addr = {
.devfn = QPCI_DEVFN(4, 0),
};
diff --git a/tests/qtest/libqos/virtio-9p.h b/tests/qtest/libqos/virtio-9p.h
index b1e6badc4a..326a603f72 100644
--- a/tests/qtest/libqos/virtio-9p.h
+++ b/tests/qtest/libqos/virtio-9p.h
@@ -44,4 +44,9 @@ struct QVirtio9PDevice {
QVirtio9P v9p;
};
+/**
+ * Prepares QEMU command line for 9pfs tests using the 'local' fs driver.
+ */
+void virtio_9p_assign_local_driver(GString *cmd_line, const char *args);
+
#endif
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 3281153b9c..af7e169d3a 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -895,29 +895,45 @@ static void fs_readdir_split_512(void *obj, void *data,
fs_readdir_split(obj, data, t_alloc, 512);
}
+static void *assign_9p_local_driver(GString *cmd_line, void *arg)
+{
+virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
+return arg;
+}
+
static void register_virtio_9p_test(void)
{
-qos_add_test("synth/config", "virtio-9p", pci_config, NULL);
-qos_add_test("synth/version/basic", "virtio-9p", fs_version, NULL);
-qos_add_test("synth/attach/basic", "virtio-9p", fs_attach, NULL);
-qos_add_test("synth/walk/basic", "virtio-9p", fs_walk, NULL);
+
+QOSGraphTestOptions opts = {
+};
+
+/* 9pfs test cases using the 'synth' filesystem driver */
+qos_add_test("synth/config", "virtio-9p", pci_config, );
+
Re: [PATCH v3 4/4] migration: Sync requested pages after postcopy recovery
* Peter Xu (pet...@redhat.com) wrote:
> We synchronize the requested pages right after a postcopy recovery happens.
> This helps to synchronize the prioritized pages on source so that the faulted
> threads can be served faster.
>
> Reported-by: Xiaohui Li
> Signed-off-by: Peter Xu
> ---
> migration/savevm.c | 57 ++
> migration/trace-events | 1 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 34e4b71052..56a2bfb24c 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2011,6 +2011,49 @@ static int
> loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> return LOADVM_QUIT;
> }
>
> +/* We must be with page_request_mutex held */
> +static gboolean postcopy_sync_page_req(gpointer key, gpointer value,
> + gpointer data)
> +{
> +MigrationIncomingState *mis = data;
> +void *host_addr = (void *) key;
> +ram_addr_t rb_offset;
> +RAMBlock *rb;
> +int ret;
> +
> +rb = qemu_ram_block_from_host(host_addr, true, _offset);
> +if (!rb) {
> +/*
> + * This should _never_ happen. However be nice for a migrating VM to
> + * not crash/assert. Post an error (note: intended to not use *_once
> + * because we do want to see all the illegal addresses; and this can
> + * never be triggered by the guest so we're safe) and move on next.
> + */
> +error_report("%s: illegal host addr %p", __func__, host_addr);
> +/* Try the next entry */
> +return FALSE;
> +}
> +
> +ret = migrate_send_rp_message_req_pages(mis, rb, rb_offset);
> +if (ret) {
> +/* Please refer to above comment. */
> +error_report("%s: send rp message failed for addr %p",
> + __func__, host_addr);
> +return FALSE;
> +}
> +
> +trace_postcopy_page_req_sync((uint64_t)(uintptr_t)host_addr);
Again that's a case for host_addr and a %p I think.
Dave
> +return FALSE;
> +}
> +
> +static void migrate_send_rp_req_pages_pending(MigrationIncomingState *mis)
> +{
> +WITH_QEMU_LOCK_GUARD(>page_request_mutex) {
> +g_tree_foreach(mis->page_requested, postcopy_sync_page_req, mis);
> +}
> +}
> +
> static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> {
> if (mis->state != MIGRATION_STATUS_POSTCOPY_RECOVER) {
> @@ -2033,6 +2076,20 @@ static int
> loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> /* Tell source that "we are ready" */
> migrate_send_rp_resume_ack(mis, MIGRATION_RESUME_ACK_VALUE);
>
> +/*
> + * After a postcopy recovery, the source should have lost the postcopy
> + * queue, or potentially the requested pages could have been lost during
> + * the network down phase. Let's re-sync with the source VM by
> re-sending
> + * all the pending pages that we eagerly need, so these threads won't get
> + * blocked too long due to the recovery.
> + *
> + * Without this procedure, the faulted destination VM threads (waiting
> for
> + * page requests right before the postcopy is interrupted) can keep
> hanging
> + * until the pages are sent by the source during the background copying
> of
> + * pages, or another thread faulted on the same address accidentally.
> + */
> +migrate_send_rp_req_pages_pending(mis);
> +
> return 0;
> }
>
> diff --git a/migration/trace-events b/migration/trace-events
> index 9187b03725..5d0b0662a8 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -49,6 +49,7 @@ vmstate_save(const char *idstr, const char *vmsd_name) "%s,
> %s"
> vmstate_load(const char *idstr, const char *vmsd_name) "%s, %s"
> postcopy_pause_incoming(void) ""
> postcopy_pause_incoming_continued(void) ""
> +postcopy_page_req_sync(uint64_t host_addr) "sync page req 0x%"PRIx64
>
> # vmstate.c
> vmstate_load_field_error(const char *field, int ret) "field \"%s\" load
> failed, ret = %d"
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH v3 05/11] tests/qtest/qos-test: dump environment variables if verbose
If qtests are run in verbose mode (i.e. if --verbose CL argument
was provided) then print all environment variables to stdout
before running the individual tests.
Instead of using g_test_message() rather use printf() in combination
with g_test_verbose(), to avoid g_test_message() cluttering the
output.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/qos-test.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index d98ef78613..fe240b32a7 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -313,9 +313,16 @@ static void walk_path(QOSGraphNode *orig_path, int len)
* machine/drivers/test objects
* - Cleans up everything
*/
-int main(int argc, char **argv)
+int main(int argc, char **argv, char** envp)
{
g_test_init(, , NULL);
+if (g_test_verbose()) {
+printf("ENVIRONMENT VARIABLES: {\n");
+for (char **env = envp; *env != 0; env++) {
+printf("\t%s\n", *env);
+}
+printf("}\n");
+}
qos_graph_init();
module_call_init(MODULE_INIT_QOM);
module_call_init(MODULE_INIT_LIBQOS);
--
2.20.1
[PATCH v3 01/11] libqos/qgraph: add qemu_name to QOSGraphNode
Add new member variable 'qemu_name' to struct QOSGraphNode.
This new member may be optionally set in case a different
name for the node (which must always be a unique name) vs.
its actually associated QEMU (QMP) device name is required.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 1 +
tests/qtest/libqos/qgraph_internal.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index fc49cfa879..e42f3eaafa 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -153,6 +153,7 @@ static QOSGraphNode *create_node(const char *name,
QOSNodeType type)
static void destroy_node(void *val)
{
QOSGraphNode *node = val;
+g_free(node->qemu_name);
g_free(node->command_line);
g_free(node);
}
diff --git a/tests/qtest/libqos/qgraph_internal.h
b/tests/qtest/libqos/qgraph_internal.h
index 968fa69450..974985dce9 100644
--- a/tests/qtest/libqos/qgraph_internal.h
+++ b/tests/qtest/libqos/qgraph_internal.h
@@ -56,6 +56,7 @@ struct QOSGraphNode {
bool available; /* set by QEMU via QMP, used during graph walk */
bool visited; /* used during graph walk */
char *name; /* used to identify the node */
+char *qemu_name;/* optional: see qos_node_create_driver_named() */
char *command_line; /* used to start QEMU at test execution */
union {
struct {
--
2.20.1
[PATCH v3 03/11] libqos/qgraph: add qos_dump_graph()
This new function is purely for debugging purposes. It prints the
current qos graph to stdout and allows to identify problems in the
created qos graph e.g. when writing new qos tests.
Coloured output is used to mark available nodes in green colour,
whereas unavailable nodes are marked in red colour.
Signed-off-by: Christian Schoenebeck
---
tests/qtest/libqos/qgraph.c | 54 +
tests/qtest/libqos/qgraph.h | 20 ++
2 files changed, 74 insertions(+)
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index 61faf6b27d..e70635750e 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -805,3 +805,57 @@ void qos_delete_cmd_line(const char *name)
node->command_line = NULL;
}
}
+
+#define RED(txt) (\
+"\033[0;91m" txt \
+"\033[0m" \
+)
+
+#define GREEN(txt) (\
+"\033[0;92m" txt \
+"\033[0m" \
+)
+
+void qos_dump_graph(void)
+{
+GList *keys;
+GList *l;
+QOSGraphEdgeList *list;
+QOSGraphEdge *e, *next;
+QOSGraphNode *dest_node, *node;
+
+printf("ALL QGRAPH EDGES: {\n");
+keys = g_hash_table_get_keys(edge_table);
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+printf("\t src='%s'\n", key);
+list = get_edgelist(key);
+QSLIST_FOREACH_SAFE(e, list, edge_list, next) {
+dest_node = g_hash_table_lookup(node_table, e->dest);
+printf("\t\t|-> dest='%s' type=%d (node=%p)",
+ e->dest, e->type, dest_node);
+if (!dest_node) {
+printf(RED(" <--- ERROR !"));
+}
+printf("\n");
+}
+}
+g_list_free(keys);
+printf("}\n");
+
+printf("ALL QGRAPH NODES: {\n");
+keys = g_hash_table_get_keys(node_table);
+for (l = keys; l != NULL; l = l->next) {
+const gchar *key = l->data;
+node = g_hash_table_lookup(node_table, key);
+printf("\t name='%s' ", key);
+if (node->qemu_name) {
+printf("qemu_name='%s' ", node->qemu_name);
+}
+printf("type=%d cmd_line='%s' [%s]\n",
+ node->type, node->command_line,
+ node->available ? GREEN("available") : RED("UNAVAILBLE"));
+}
+g_list_free(keys);
+printf("}\n");
+}
diff --git a/tests/qtest/libqos/qgraph.h b/tests/qtest/libqos/qgraph.h
index f472949f68..07a32535f1 100644
--- a/tests/qtest/libqos/qgraph.h
+++ b/tests/qtest/libqos/qgraph.h
@@ -586,5 +586,25 @@ QOSGraphObject *qos_machine_new(QOSGraphNode *node,
QTestState *qts);
QOSGraphObject *qos_driver_new(QOSGraphNode *node, QOSGraphObject *parent,
QGuestAllocator *alloc, void *arg);
+/**
+ * Just for debugging purpose: prints all currently existing nodes and
+ * edges to stdout.
+ *
+ * All qtests add themselves to the overall qos graph by calling qgraph
+ * functions that add device nodes and edges between the individual graph
+ * nodes for tests. As the actual graph is assmbled at runtime by the qos
+ * subsystem, it is sometimes not obvious how the overall graph looks like.
+ * E.g. when writing new tests it may happen that those new tests are simply
+ * ignored by the qtest framework.
+ *
+ * This function allows to identify problems in the created qgraph. Keep in
+ * mind: only tests with a path down from the actual test case node (leaf) up
+ * to the graph's root node are actually executed by the qtest framework. And
+ * the qtest framework uses QMP to automatically check which QEMU drivers are
+ * actually currently available, and accordingly qos marks certain pathes as
+ * 'unavailable' in such cases (e.g. when QEMU was compiled without support for
+ * a certain feature).
+ */
+void qos_dump_graph(void);
#endif
--
2.20.1
Re: [PATCH] tests/test-char: Use a proper fallthrough comment
On 10/2/20 7:13 PM, Thomas Huth wrote: > For being able to compile with -Werror=implicit-fallthrough we need > to use comments that the compiler recognizes. Use "fallthrough" instead > of "no break" here. > > Signed-off-by: Thomas Huth > --- > tests/test-char.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/test-char.c b/tests/test-char.c > index d35cc839bc..9196e566e9 100644 > --- a/tests/test-char.c > +++ b/tests/test-char.c > @@ -70,7 +70,7 @@ static void fe_event(void *opaque, QEMUChrEvent event) > h->openclose_mismatch = true; > } > h->is_open = new_open_state; > -/* no break */ > +/* fallthrough */ > default: > quit = true; > break; > Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé
