Re: [Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Ben Warren via Qemu-devel]

Hi Michael,

> On Dec 19, 2017, at 8:27 PM, Michael S. Tsirkin  wrote:
> 
> On Wed, Dec 13, 2017 at 12:26:44AM -0800, b...@skyportsystems.com wrote:
>> From: Ben Warren 
>> 
>> Now that virtio-win guest drivers provided by non-Redhat vendors need to
>> use a different Subsystem Vendor ID value, a way is needed to set this
>> parameter on the host.  This works with all of the PCI-based devices,
>> such as NetKVM, viostor, vioscsi, vioserial and balloon.
>> 
>> Signed-off-by: Ben Warren 
> 
> I applied a related patch by Ladi, pls take a look.
> 
If you’re talking about the one titled "virtio-pci: Don't force Subsystem 
Vendor ID = Vendor ID”, then this is complementary.  If it’s another one, I 
must have missed it.  The patch I’m aware of doesn’t provide any way of setting 
the subsystem vendor ID, which is needed.

BTW - would you prefer this to be contained to virtio-pci, or all of PCI, as 
another reviewer suggested.  Either is easy.

>> ---
>> hw/virtio/virtio-pci.c | 5 +
>> hw/virtio/virtio-pci.h | 1 +
>> 2 files changed, 6 insertions(+)
>> 
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index e92837c..b5c86e3 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1757,6 +1757,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>> Error **errp)
>> if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
>> proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>> }
>> +/* Set the PCI Subsystem Vendor ID */
>> +k->parent_class.subsystem_vendor_id = proxy->subsystem_vendor_id;
>> +pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, 
>> proxy->subsystem_vendor_id);
>> 
>> if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
>> error_setg(errp, "device cannot work as neither modern nor legacy 
>> mode"
>> @@ -1876,6 +1879,8 @@ static Property virtio_pci_properties[] = {
>> VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>> DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>> VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +DEFINE_PROP_UINT16("subsystem-vendor-id", VirtIOPCIProxy,
>> +   subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>> 
>> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>> index 12d3a90..8a897ea 100644
>> --- a/hw/virtio/virtio-pci.h
>> +++ b/hw/virtio/virtio-pci.h
>> @@ -186,6 +186,7 @@ struct VirtIOPCIProxy {
>> VirtIOIRQFD *vector_irqfd;
>> int nvqs_with_notifiers;
>> VirtioBusState bus;
>> +uint16_t subsystem_vendor_id;
>> };
>> 
>> static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
>> -- 
>> 2.7.4



smime.p7s
Description: S/MIME cryptographic signature

Re: [Qemu-devel] [PATCH v4 12/43] misc: remove headers implicitly included

[Ben Warren via Qemu-devel]

> On Dec 14, 2017, at 7:29 PM, Philippe Mathieu-Daudé  wrote:
> 
> applied using ./scripts/clean-includes
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Peter Maydell 
> Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Ben Warren 

> ---
> hw/audio/fmopl.h | 1 -
> bsd-user/main.c  | 1 -
> chardev/wctablet.c   | 4 
> hw/scsi/vhost-user-scsi.c| 1 -
> linux-user/main.c| 1 -
> net/colo-compare.c   | 1 -
> tests/test-aio-multithread.c | 1 -
> tests/test-clone-visitor.c   | 1 -
> tests/vmgenid-test.c | 3 ---
> 9 files changed, 14 deletions(-)
> 
> diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
> index f4065f425c..e7e578a48e 100644
> --- a/hw/audio/fmopl.h
> +++ b/hw/audio/fmopl.h
> @@ -1,7 +1,6 @@
> #ifndef FMOPL_H
> #define FMOPL_H
> 
> -#include 
> 
> typedef void (*OPL_TIMERHANDLER)(void *param, int channel, double 
> interval_Sec);
> 
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index f1b244b59b..efef5ff8c5 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -32,7 +32,6 @@
> #include "qemu/envlist.h"
> #include "exec/log.h"
> #include "trace/control.h"
> -#include "glib-compat.h"
> 
> int singlestep;
> unsigned long mmap_min_addr;
> diff --git a/chardev/wctablet.c b/chardev/wctablet.c
> index 6c13c2c58a..969d014574 100644
> --- a/chardev/wctablet.c
> +++ b/chardev/wctablet.c
> @@ -25,10 +25,6 @@
> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> * THE SOFTWARE.
> */
> -#include 
> -#include 
> -#include 
> -#include 
> 
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index f7561e23fa..9389ed48e0 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -18,7 +18,6 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/error-report.h"
> -#include "qemu/typedefs.h"
> #include "qom/object.h"
> #include "hw/fw-path-provider.h"
> #include "hw/qdev-core.h"
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 6286661bd3..2fd2a143ed 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -35,7 +35,6 @@
> #include "elf.h"
> #include "exec/log.h"
> #include "trace/control.h"
> -#include "glib-compat.h"
> 
> char *exec_path;
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 1ce195f877..0ebdec936c 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -23,7 +23,6 @@
> #include "qom/object_interfaces.h"
> #include "qemu/iov.h"
> #include "qom/object.h"
> -#include "qemu/typedefs.h"
> #include "net/queue.h"
> #include "chardev/char-fe.h"
> #include "qemu/sockets.h"
> diff --git a/tests/test-aio-multithread.c b/tests/test-aio-multithread.c
> index d396185972..c8bec81520 100644
> --- a/tests/test-aio-multithread.c
> +++ b/tests/test-aio-multithread.c
> @@ -11,7 +11,6 @@
> */
> 
> #include "qemu/osdep.h"
> -#include 
> #include "block/aio.h"
> #include "qapi/error.h"
> #include "qemu/coroutine.h"
> diff --git a/tests/test-clone-visitor.c b/tests/test-clone-visitor.c
> index 96982163e4..ac6afc562e 100644
> --- a/tests/test-clone-visitor.c
> +++ b/tests/test-clone-visitor.c
> @@ -8,7 +8,6 @@
> */
> 
> #include "qemu/osdep.h"
> -#include 
> 
> #include "qemu-common.h"
> #include "qapi/clone-visitor.h"
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 5a86b40775..68ff954578 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -8,9 +8,6 @@
> * See the COPYING file in the top-level directory.
> */
> 
> -#include 
> -#include 
> -#include 
> #include "qemu/osdep.h"
> #include "qemu/bitmap.h"
> #include "qemu/uuid.h"
> -- 
> 2.15.1
> 



smime.p7s
Description: S/MIME cryptographic signature

[Qemu-devel] [PATCH] virtio-pci: Add subsystem-vendor-id property

[Ben Warren via Qemu-devel]

From: Ben Warren 

Now that virtio-win guest drivers provided by non-Redhat vendors need to
use a different Subsystem Vendor ID value, a way is needed to set this
parameter on the host.  This works with all of the PCI-based devices,
such as NetKVM, viostor, vioscsi, vioserial and balloon.

Signed-off-by: Ben Warren 
---
 hw/virtio/virtio-pci.c | 5 +
 hw/virtio/virtio-pci.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e92837c..b5c86e3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1757,6 +1757,9 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error 
**errp)
 if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
 proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
 }
+/* Set the PCI Subsystem Vendor ID */
+k->parent_class.subsystem_vendor_id = proxy->subsystem_vendor_id;
+pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, 
proxy->subsystem_vendor_id);
 
 if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
 error_setg(errp, "device cannot work as neither modern nor legacy mode"
@@ -1876,6 +1879,8 @@ static Property virtio_pci_properties[] = {
 VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
 DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
 VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
+DEFINE_PROP_UINT16("subsystem-vendor-id", VirtIOPCIProxy,
+   subsystem_vendor_id, PCI_VENDOR_ID_REDHAT_QUMRANET),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 12d3a90..8a897ea 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -186,6 +186,7 @@ struct VirtIOPCIProxy {
 VirtIOIRQFD *vector_irqfd;
 int nvqs_with_notifiers;
 VirtioBusState bus;
+uint16_t subsystem_vendor_id;
 };
 
 static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/2] vmgenid: use UUID property type

[Ben Warren via Qemu-devel]

It looks like you dropped Marc-André and my Reviewed-by lines.  Please put them 
back.

> On Nov 27, 2017, at 5:05 AM, Roman Kagan  wrote:
> 
> Switch vmgenid device to use the UUID property type introduced in the
> previous patch for its 'guid' property.
> 
> One semantic change it introduces is that post-realize modification of
> 'guid' via HMP or QMP will now be rejected with an error; however,
> according to docs/specs/vmgenid.txt this is actually desirable.
> 
> Signed-off-by: Roman Kagan 
> ---
> v1 -> v2:
> - use the corresponding define for "guid" field name
> 
> hw/acpi/vmgenid.c | 30 --
> 1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index 105044f666..ba6f47b67b 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -162,21 +162,6 @@ static void vmgenid_update_guest(VmGenIdState *vms)
> }
> }
> 
> -static void vmgenid_set_guid(Object *obj, const char *value, Error **errp)
> -{
> -VmGenIdState *vms = VMGENID(obj);
> -
> -if (!strcmp(value, "auto")) {
> -qemu_uuid_generate(&vms->guid);
> -} else if (qemu_uuid_parse(value, &vms->guid) < 0) {
> -error_setg(errp, "'%s. %s': Failed to parse GUID string: %s",
> -   object_get_typename(OBJECT(vms)), VMGENID_GUID, value);
> -return;
> -}
> -
> -vmgenid_update_guest(vms);
> -}
> -
> /* After restoring an image, we need to update the guest memory and notify
>  * it of a potential change to VM Generation ID
>  */
> @@ -224,23 +209,24 @@ static void vmgenid_realize(DeviceState *dev, Error 
> **errp)
> }
> 
> qemu_register_reset(vmgenid_handle_reset, vms);
> +
> +vmgenid_update_guest(vms);
> }
> 
> +static Property vmgenid_device_properties[] = {
> +DEFINE_PROP_UUID(VMGENID_GUID, VmGenIdState, guid),
> +DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void vmgenid_device_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> 
> dc->vmsd = &vmstate_vmgenid;
> dc->realize = vmgenid_realize;
> +dc->props = vmgenid_device_properties;
> dc->hotpluggable = false;
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> -
> -object_class_property_add_str(klass, VMGENID_GUID, NULL,
> -  vmgenid_set_guid, NULL);
> -object_class_property_set_description(klass, VMGENID_GUID,
> -"Set Global Unique Identifier "
> -"(big-endian) or auto for random value",
> -NULL);
> }
> 
> static const TypeInfo vmgenid_device_info = {
> -- 
> 2.14.3
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: [Qemu-devel] [PATCH 1/2] qdev-properties: add UUID property type

[Ben Warren via Qemu-devel]

> On Nov 24, 2017, at 7:36 AM, Roman Kagan  wrote:
> 
> UUIDs (GUIDs) are widely used in VMBus-related stuff, so a dedicated
> property type becomes helpful.
> 
> Signed-off-by: Roman Kagan 
Reviewed-by: Ben Warren 
> ---
> include/hw/qdev-properties.h |  3 +++
> hw/core/qdev-properties.c| 52 
> 2 files changed, 55 insertions(+)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index e2321f1cc1..d4da7dd1f1 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -30,6 +30,7 @@ extern const PropertyInfo qdev_prop_vlan;
> extern const PropertyInfo qdev_prop_pci_devfn;
> extern const PropertyInfo qdev_prop_blocksize;
> extern const PropertyInfo qdev_prop_pci_host_devaddr;
> +extern const PropertyInfo qdev_prop_uuid;
> extern const PropertyInfo qdev_prop_arraylen;
> extern const PropertyInfo qdev_prop_link;
> 
> @@ -212,6 +213,8 @@ extern const PropertyInfo qdev_prop_link;
> DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f) \
> DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, MemoryRegion *)
> +#define DEFINE_PROP_UUID(_n, _s, _f) \
> +DEFINE_PROP(_n, _s, _f, qdev_prop_uuid, QemuUUID)
> 
> #define DEFINE_PROP_END_OF_LIST()   \
> {}
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 1dc80fcea2..49fea5a40a 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -10,6 +10,7 @@
> #include "net/hub.h"
> #include "qapi/visitor.h"
> #include "chardev/char.h"
> +#include "qemu/uuid.h"
> 
> void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>   Error **errp)
> @@ -883,6 +884,57 @@ const PropertyInfo qdev_prop_pci_host_devaddr = {
> .set = set_pci_host_devaddr,
> };
> 
> +/* --- UUID --- */
> +
> +static void get_uuid(Object *obj, Visitor *v, const char *name, void *opaque,
> + Error **errp)
> +{
> +DeviceState *dev = DEVICE(obj);
> +Property *prop = opaque;
> +QemuUUID *uuid = qdev_get_prop_ptr(dev, prop);
> +char buffer[UUID_FMT_LEN + 1];
> +char *p = buffer;
> +
> +qemu_uuid_unparse(uuid, buffer);
> +
> +visit_type_str(v, name, &p, errp);
> +}
> +
> +static void set_uuid(Object *obj, Visitor *v, const char *name, void *opaque,
> +Error **errp)
> +{
> +DeviceState *dev = DEVICE(obj);
> +Property *prop = opaque;
> +QemuUUID *uuid = qdev_get_prop_ptr(dev, prop);
> +Error *local_err = NULL;
> +char *str;
> +
> +if (dev->realized) {
> +qdev_prop_set_after_realize(dev, name, errp);
> +return;
> +}
> +
> +visit_type_str(v, name, &str, &local_err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +if (!strcmp(str, "auto")) {
> +qemu_uuid_generate(uuid);
> +} else if (qemu_uuid_parse(str, uuid) < 0) {
> +error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> +}
> +g_free(str);
> +}
> +
> +const PropertyInfo qdev_prop_uuid = {
> +.name  = "str",
> +.description = "UUID (aka GUID) or \"auto\" for random value",
> +.get   = get_uuid,
> +.set   = set_uuid,
> +};
> +
> /* --- support for array properties --- */
> 
> /* Used as an opaque for the object properties we add for each
> -- 
> 2.14.3
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: [Qemu-devel] [PATCH 2/2] vmgenid: use UUID property type

[Ben Warren via Qemu-devel]

Thanks Roman, this seems like a nice improvement.  Fix the minor point below, 
and you can add my R-B.

> On Nov 24, 2017, at 7:36 AM, Roman Kagan  wrote:
> 
> Switch vmgenid device to use the UUID property type introduced in the
> previous patch for its 'guid' property.
> 
> One semantic change it introduces is that post-realize modification of
> 'guid' via HMP or QMP will now be rejected with an error; however,
> according to docs/specs/vmgenid.txt this is actually desirable.
> 
> Signed-off-by: Roman Kagan 
Reviewed-by: Ben Warren 
> ---
> hw/acpi/vmgenid.c | 30 --
> 1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index 105044f666..84ff015bda 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -162,21 +162,6 @@ static void vmgenid_update_guest(VmGenIdState *vms)
> }
> }
> 
> -static void vmgenid_set_guid(Object *obj, const char *value, Error **errp)
> -{
> -VmGenIdState *vms = VMGENID(obj);
> -
> -if (!strcmp(value, "auto")) {
> -qemu_uuid_generate(&vms->guid);
> -} else if (qemu_uuid_parse(value, &vms->guid) < 0) {
> -error_setg(errp, "'%s. %s': Failed to parse GUID string: %s",
> -   object_get_typename(OBJECT(vms)), VMGENID_GUID, value);
> -return;
> -}
> -
> -vmgenid_update_guest(vms);
> -}
> -
> /* After restoring an image, we need to update the guest memory and notify
>  * it of a potential change to VM Generation ID
>  */
> @@ -224,23 +209,24 @@ static void vmgenid_realize(DeviceState *dev, Error 
> **errp)
> }
> 
> qemu_register_reset(vmgenid_handle_reset, vms);
> +
> +vmgenid_update_guest(vms);
> }
> 
> +static Property vmgenid_device_properties[] = {
> +DEFINE_PROP_UUID("guid", VmGenIdState, guid),
Use VMGENID_GUID instead of “guid"
> +DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void vmgenid_device_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> 
> dc->vmsd = &vmstate_vmgenid;
> dc->realize = vmgenid_realize;
> +dc->props = vmgenid_device_properties;
> dc->hotpluggable = false;
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> -
> -object_class_property_add_str(klass, VMGENID_GUID, NULL,
> -  vmgenid_set_guid, NULL);
> -object_class_property_set_description(klass, VMGENID_GUID,
> -"Set Global Unique Identifier "
> -"(big-endian) or auto for random value",
> -NULL);
> }
> 
> static const TypeInfo vmgenid_device_info = {
> -- 
> 2.14.3
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: [Qemu-devel] [PATCH 81/88] hw/acpi: use g_new() family of functions

[Ben Warren via Qemu-devel]

> On Oct 6, 2017, at 4:50 PM, Philippe Mathieu-Daudé  wrote:
> 
> Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Ben Warren 
> ---
> hw/acpi/vmgenid.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index 2876d8a639..ced507d218 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -269,7 +269,7 @@ GuidInfo *qmp_query_vm_generation_id(Error **errp)
> }
> vms = VMGENID(obj);
> 
> -info = g_malloc0(sizeof(*info));
> +info = g_new0(GuidInfo, 1);
> info->guid = qemu_uuid_unparse_strdup(&vms->guid);
> return info;
> }
> -- 
> 2.14.2
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: [Qemu-devel] [PATCH v2] vmgenid-test: use boot-sector infrastructure

[Ben Warren via Qemu-devel]

Hi Michael,

Looks good.  Thanks for taking care of this.
> On Jul 14, 2017, at 8:30 AM, Michael S. Tsirkin  wrote:
> 
> There's no requirement for RSDP to be installed last
> by the firmware, so in rare cases vmgen id test hits
> a race: RSDP is there but VM GEN ID isn't.
> 
> To fix, switch to common boot sector infrastructure.
> 
> Cc: Laszlo Ersek 
> Cc: Peter Maydell 
> Cc: Ben Warren 
> Signed-off-by: Michael S. Tsirkin 
Reviewed-by: Ben Warren mailto:b...@skyportsystems.com>>
> ---
> 
> Changes from v1
>fix coding style violation reported by Peter
> 
> tests/vmgenid-test.c   | 46 ++
> tests/Makefile.include |  2 +-
> 2 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index e7ba38c..a49de92 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -15,6 +15,7 @@
> #include "qemu/bitmap.h"
> #include "qemu/uuid.h"
> #include "hw/acpi/acpi-defs.h"
> +#include "boot-sector.h"
> #include "acpi-utils.h"
> #include "libqtest.h"
> 
> @@ -23,8 +24,6 @@
>   * OVMF SDT Header Probe Supressor
>   */
> #define RSDP_ADDR_INVALID 0x10 /* RSDP must be below this address */
> -#define RSDP_SLEEP_US 10   /* Sleep for 100ms between tries */
> -#define RSDP_TRIES_MAX100  /* Max total time is 10 seconds */
> 
> typedef struct {
> AcpiTableHeader header;
> @@ -47,14 +46,12 @@ static uint32_t acpi_find_vgia(void)
> VgidTable vgid_table;
> int i;
> 
> -/* Tables may take a short time to be set up by the guest */
> -for (i = 0; i < RSDP_TRIES_MAX; i++) {
> -rsdp_offset = acpi_find_rsdp_address();
> -if (rsdp_offset < RSDP_ADDR_INVALID) {
> -break;
> -}
> -g_usleep(RSDP_SLEEP_US);
> -}
> +/* Wait for guest firmware to finish and start the payload. */
> +boot_sector_test();
> +
> +/* Tables should be initialized now. */
> +rsdp_offset = acpi_find_rsdp_address();
> +
> g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
> 
> acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
> @@ -131,6 +128,18 @@ static void read_guid_from_monitor(QemuUUID *guid)
> QDECREF(rsp);
> }
> 
> +static char disk[] = "tests/vmgenid-test-disk-XX";
> +
> +static char *guid_cmd_strdup(const char *guid)
> +{
> +return g_strdup_printf("-machine accel=tcg "
> +   "-device vmgenid,id=testvgid,guid=%s "
> +   "-drive id=hd0,if=none,file=%s,format=raw "
> +   "-device ide-hd,drive=hd0 ",
> +   guid, disk);
> +}
> +
> +
> static void vmgenid_set_guid_test(void)
> {
> QemuUUID expected, measured;
> @@ -138,8 +147,7 @@ static void vmgenid_set_guid_test(void)
> 
> g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
> 
> -cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
> -  "guid=%s", VGID_GUID);
> +cmd = guid_cmd_strdup(VGID_GUID);
> qtest_start(cmd);
> 
> /* Read the GUID from accessing guest memory */
> @@ -152,10 +160,10 @@ static void vmgenid_set_guid_test(void)
> 
> static void vmgenid_set_guid_auto_test(void)
> {
> -const char *cmd;
> +char *cmd;
> QemuUUID measured;
> 
> -cmd = "-machine accel=tcg -device vmgenid,id=testvgid," "guid=auto";
> +cmd = guid_cmd_strdup("auto");
> qtest_start(cmd);
> 
> read_guid_from_memory(&measured);
> @@ -164,6 +172,7 @@ static void vmgenid_set_guid_auto_test(void)
> g_assert(!qemu_uuid_is_null(&measured));
> 
> qtest_quit(global_qtest);
> +g_free(cmd);
> }
> 
> static void vmgenid_query_monitor_test(void)
> @@ -173,8 +182,7 @@ static void vmgenid_query_monitor_test(void)
> 
> g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
> 
> -cmd = g_strdup_printf("-machine accel=tcg -device vmgenid,id=testvgid,"
> -  "guid=%s", VGID_GUID);
> +cmd = guid_cmd_strdup(VGID_GUID);
> qtest_start(cmd);
> 
> /* Read the GUID via the monitor */
> @@ -189,6 +197,11 @@ int main(int argc, char **argv)
> {
> int ret;
> 
> +ret = boot_sector_init(disk);
> +if (ret) {
> +return ret;
> +}
> +
> g_test_init(&argc, &argv, NULL);
> 
> qtest_add_func("/vmgenid/vmgenid/set-guid",
> @@ -198,6 +211,7 @@ int main(int argc, char **argv)
> qtest_add_func("/vmgenid/vmgenid/query-monitor",
>vmgenid_query_monitor_test);
> ret = g_test_run();
> +boot_sector_cleanup(disk);
> 
> return ret;
> }
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 18cd06a..aef374b 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -761,7 +761,7 @@ tests/test-uuid$(EXESUF): tests/test-uuid.o 
> $(test-util-obj-y)
> tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
> tests/test-qapi-util$(EXESUF): tests/test-qapi-

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

Hi,
> On Jul 13, 2017, at 4:51 AM, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> On Thu, Jul 13, 2017 at 1:32 PM Laszlo Ersek  > wrote:
> On 07/13/17 12:47, Peter Maydell wrote:
> > On 12 July 2017 at 00:43, Ben Warren  > > wrote:
> >> Yes, it’s definitely a setup time problem.  With the values that are 
> >> checked
> >> in, I can’t get it to fail on my setup, but if I wind the numbers down I 
> >> see
> >> the same failure as Peter.  So now we have the ages-old problem of “what 
> >> new
> >> arbitrary value should I use that will not cause false failures but will
> >> eventually time out”.
> >
> > Empirically, we already have an answer to this, in the form
> > of the existing code in tests/boot-sector.c, which is used
> > by both that test and the bios-tables-test.c code to wait
> > for the BIOS initialization to complete, and which doesn't
> > cause false test timeouts in practice.
> >
> > Can we make this test just use that existing function to
> > wait for the BIOS to be done, rather than having its own
> > timeout loop?
> 
> This is incredibly cool. Now that I've looked at "tests/boot-sector.c"
> (again), I recall having seen it earlier, but I couldn't have remembered
> it off-hand.
> 
> Perhaps this boot sector code should be factored out and moved to
> "tests/acpi-utils".
> 
> Marc-André, do you think it would be feasible for the vmcoreinfo unit
> test as well? (Which is derived from the vmgenid unit test.)
> 
> It seems likely.
> 
> Ben, are you going to work on the fix?
> 
Yes, I will take care of this.  I remember seeing the boot-sector 
synchronization code before, but didn’t really grok it.  This time I’ll dig 
deeper.
> Thanks
> -- 
> Marc-André Lureau

—Ben

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

Hi Laszlo,

> On Jul 11, 2017, at 3:13 PM, Laszlo Ersek  wrote:
> 
> On 07/11/17 22:42, Peter Maydell wrote:
>> On 11 July 2017 at 20:10, Michael S. Tsirkin  wrote:
>>> On Tue, Jul 11, 2017 at 05:49:07PM +0100, Peter Maydell wrote:
 The good news is it's not aarch64-specific. I just hit this on
 a build on x86_64 host, gcc, debug build:
 
  GTESTER check-qtest-x86_64
 **
 ERROR:/home/petmay01/linaro/qemu-for-merges/tests/vmgenid-test.c:65:acpi_find_vgia:
 assertion failed (ACPI_ASSERT_CMP_str == "RSDT"): ("" == "RSDT")
 GTester: last random seed: R02S9eefaf38297e67e4f67d5db77989350e
 /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:826:
 recipe for target 'check-qtest-x86_64' failed
>> 
>>> Couldn't reproduce here. I suspect VM didn't start at all.
>>> Am I right to assume this is without KVM?
>> 
>> On aarch64 host, definitely without KVM. On x86-64 host,
>> I think it is without KVM but am not totally sure.
>> 
>> It may be one of those cases that only triggers if the
>> host is heavily loaded at the time the test is running.
> 
> (I apologize if the root cause is obvious at this point -- I'm unclear
> if the discussion is now about understanding the failure, or about ways
> to mitigate it. I'm assuming the former.)
> 
> This test can occasionally fail because the test case has to wait until
> the guest firmware proceeds far enough with executing the ACPI
> linker/loader script. See RSDP_SLEEP_US and RSDP_TRIES_MAX in
> acpi_find_vgia(). If the test case pokes at guest RAM "too early", using
> monitor commands, then the guest fw will not have yet shaped guest RAM
> as required.
> 
Yes, it’s definitely a setup time problem.  With the values that are checked 
in, I can’t get it to fail on my setup, but if I wind the numbers down I see 
the same failure as Peter.  So now we have the ages-old problem of “what new 
arbitrary value should I use that will not cause false failures but will 
eventually time out”.  Can you think of an easy way to tell if firmware is 
running so we can make this more state-driven instead of time-dependent?


> (Again, apologies if this has been obvious all along.)
> 
> Thanks
> Laszlo

Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

Hi Peter,
> On Jul 11, 2017, at 6:32 AM, Peter Maydell  wrote:
> 
> On 3 July 2017 at 20:45, Michael S. Tsirkin  wrote:
>> From: Ben Warren 
>> 
>> The following tests are implemented:
>> * test that a GUID passed in by command line is propagated to the guest.
>>  Read the GUID from guest memory
>> * test that the "auto" argument to the GUID generates a valid GUID, as
>>  seen by the guest.
>> * test that a GUID passed in can be queried from the monitor
>> 
>>  This patch is loosely based on a previous patch from:
>>  Gal Hammer   and Igor Mammedov 
>> 
>> Signed-off-by: Ben Warren 
>> Reviewed-by: Igor Mammedov 
>> Reviewed-by: Marc-André Lureau 
>> Reviewed-by: Michael S. Tsirkin 
>> Signed-off-by: Michael S. Tsirkin 
> 
> Hi -- this test seems to intermittently fail:
> 
> TEST: tests/vmgenid-test... (pid=15466)
>  /i386/vmgenid/vmgenid/set-guid:  **
> ERROR:/home/peter.maydell/qemu/tests/vmgenid-test.c:65:acpi_find_vgia:
> assertion failed (ACPI_ASSERT_CMP
> _str == "RSDT"): ("" == "RSDT")
> FAIL
> GTester: last random seed: R02S27021da892f2124a377287729b848ff8
> (pid=15470)
>  /i386/vmgenid/vmgenid/set-guid-auto: OK
>  /i386/vmgenid/vmgenid/query-monitor: OK
> FAIL: tests/vmgenid-test
> 
> (that's an aarch32 build).
> 
> 
I’ll try to reproduce.  How exactly are the tests called?
> Earlier in the run I see there was also a warning from acpi-test:
> 
>  /i386/acpi/q35/memhp:
> "kvm" accelerator not found.
> 
> Looking for expected file 'tests/acpi-test-data/q35/DSDT.memhp'
> 
> Using expected file 'tests/acpi-test-data/q35/DSDT.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/APIC.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/APIC'
> 
> Using expected file 'tests/acpi-test-data/q35/APIC'
> 
> Looking for expected file 'tests/acpi-test-data/q35/HPET.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/HPET'
> 
> Using expected file 'tests/acpi-test-data/q35/HPET'
> 
> Looking for expected file 'tests/acpi-test-data/q35/SRAT.memhp'
> 
> Using expected file 'tests/acpi-test-data/q35/SRAT.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/SLIT.memhp'
> 
> Using expected file 'tests/acpi-test-data/q35/SLIT.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/MCFG.memhp'
> 
> Looking for expected file 'tests/acpi-test-data/q35/MCFG'
> 
> Using expected file 'tests/acpi-test-data/q35/MCFG'
> acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-E3X12Y.dsl,
> aml:/tmp/aml-GIX12Y], Expected [asl:/tmp/asl-5Z502Y.dsl,
> aml:tests/acpi-test-data/q35/DSDT.memhp].
> acpi-test: Warning. not showing difference since no diff utility is
> specified. Set 'DIFF' environment variable to a preferred diff utility
> and run 'make V=1 check' again to see ASL difference.OK
> 
> (Shouldn't a DSDT mismatch cause a test failure?)
> 
> 
> thanks
> -- PMM

[Qemu-devel] [PATCH v2] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

From: Ben Warren 

The following tests are implemented:
* test that a GUID passed in by command line is propagated to the guest.
  Read the GUID from guest memory
* test that the "auto" argument to the GUID generates a valid GUID, as
  seen by the guest.
* test that a GUID passed in can be queried from the monitor

  This patch is loosely based on a previous patch from:
  Gal Hammer   and Igor Mammedov 

Signed-off-by: Ben Warren 
Reviewed-by: Igor Mammedov 
Reviewed-by: Marc-André Lureau 
---

v1->v2: free g_alloc'd tables

 tests/Makefile.include |   2 +
 tests/vmgenid-test.c   | 203 +
 2 files changed, 205 insertions(+)
 create mode 100644 tests/vmgenid-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index ae889ca..18cd06a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -250,6 +250,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
 check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += 
tests/vhost-user-test$(EXESUF)
 ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
@@ -760,6 +761,7 @@ tests/test-uuid$(EXESUF): tests/test-uuid.o 
$(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
+tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/acpi-utils.o
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< 
,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
new file mode 100644
index 000..e7ba38c
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,203 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include 
+#include 
+#include 
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "acpi-utils.h"
+#include "libqtest.h"
+
+#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+#define VMGENID_GUID_OFFSET 40   /* allow space for
+  * OVMF SDT Header Probe Supressor
+  */
+#define RSDP_ADDR_INVALID 0x10 /* RSDP must be below this address */
+#define RSDP_SLEEP_US 10   /* Sleep for 100ms between tries */
+#define RSDP_TRIES_MAX100  /* Max total time is 10 seconds */
+
+typedef struct {
+AcpiTableHeader header;
+gchar name_op;
+gchar vgia[4];
+gchar val_op;
+uint32_t vgia_val;
+} QEMU_PACKED VgidTable;
+
+static uint32_t acpi_find_vgia(void)
+{
+uint32_t rsdp_offset;
+uint32_t guid_offset = 0;
+AcpiRsdpDescriptor rsdp_table;
+uint32_t rsdt;
+AcpiRsdtDescriptorRev1 rsdt_table;
+int tables_nr;
+uint32_t *tables;
+AcpiTableHeader ssdt_table;
+VgidTable vgid_table;
+int i;
+
+/* Tables may take a short time to be set up by the guest */
+for (i = 0; i < RSDP_TRIES_MAX; i++) {
+rsdp_offset = acpi_find_rsdp_address();
+if (rsdp_offset < RSDP_ADDR_INVALID) {
+break;
+}
+g_usleep(RSDP_SLEEP_US);
+}
+g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
+
+acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
+
+rsdt = rsdp_table.rsdt_physical_address;
+/* read the header */
+ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
+ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+
+/* compute the table entries in rsdt */
+tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+sizeof(uint32_t);
+g_assert_cmpint(tables_nr, >, 0);
+
+/* get the addresses of the tables pointed by rsdt */
+tables = g_new0(uint32_t, tables_nr);
+ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
+
+for (i = 0; i < tables_nr; i++) {
+ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+/* the first entry in the table should be VGIA
+ * That's all we need
+ */
+ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
+g_assert(vgid_table.name_op == 0x08);  /* name */
+ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
+g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
+ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+g_assert(vgid_table.val_op == 0x0C);  /* dword */
+ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
+/* The GUID is written at a fixed offset 

Re: [Qemu-devel] [PATCH 1/7] vmgenid: replace x-write-pointer-available hack

[Ben Warren via Qemu-devel]

Nice improvement!
> On Jun 29, 2017, at 9:23 AM, Marc-André Lureau  
> wrote:
> 
> This compat property sole function is to prevent the device from being
> instantiated. Instead of requiring an extra compat property, check if
> fw_cfg has DMA enabled.
> 
> This has the additional benefit of handling other cases properly, like:
> 
>  $ qemu-system-x86_64 -device vmgenid -machine none
>  qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support in 
> fw_cfg, which this machine type does not provide
>  $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.9 -global 
> fw_cfg.dma_enabled=off
>  qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support in 
> fw_cfg, which this machine type does not provide
>  $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.6 -global 
> fw_cfg.dma_enabled=on
>  [boots normally]
> 
> Suggested-by: Eduardo Habkost 
> Signed-off-by: Marc-André Lureau 
Reviewed-by: Ben Warren mailto:m...@redhat.com>>
> ---
> include/hw/acpi/bios-linker-loader.h | 2 ++
> include/hw/compat.h  | 4 
> hw/acpi/bios-linker-loader.c | 6 ++
> hw/acpi/vmgenid.c| 9 +
> 4 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/acpi/bios-linker-loader.h 
> b/include/hw/acpi/bios-linker-loader.h
> index efe17b0b9c..a711dbced8 100644
> --- a/include/hw/acpi/bios-linker-loader.h
> +++ b/include/hw/acpi/bios-linker-loader.h
> @@ -7,6 +7,8 @@ typedef struct BIOSLinker {
> GArray *file_list;
> } BIOSLinker;
> 
> +bool bios_linker_loader_can_write_pointer(void);
> +
> BIOSLinker *bios_linker_loader_init(void);
> 
> void bios_linker_loader_alloc(BIOSLinker *linker,
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 26cd5851a5..36f02179ac 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -150,10 +150,6 @@
> .driver   = "fw_cfg_io",\
> .property = "dma_enabled",\
> .value= "off",\
> -},{\
> -.driver   = "vmgenid",\
> -.property = "x-write-pointer-available",\
> -.value= "off",\
> },
> 
> #define HW_COMPAT_2_3 \
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index 046183a0f1..587d62cb93 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -168,6 +168,12 @@ bios_linker_find_file(const BIOSLinker *linker, const 
> char *name)
> return NULL;
> }
> 
> +bool bios_linker_loader_can_write_pointer(void)
> +{
> +FWCfgState *fw_cfg = fw_cfg_find();
> +return fw_cfg && fw_cfg_dma_enabled(fw_cfg);
> +}
> +
> /*
>  * bios_linker_loader_alloc: ask guest to load file into guest memory.
>  *
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index a32b847fe0..ab5da293fd 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -205,17 +205,11 @@ static void vmgenid_handle_reset(void *opaque)
> memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
> }
> 
> -static Property vmgenid_properties[] = {
> -DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
> - write_pointer_available, true),
> -DEFINE_PROP_END_OF_LIST(),
> -};
> -
> static void vmgenid_realize(DeviceState *dev, Error **errp)
> {
> VmGenIdState *vms = VMGENID(dev);
> 
> -if (!vms->write_pointer_available) {
> +if (!bios_linker_loader_can_write_pointer()) {
> error_setg(errp, "%s requires DMA write support in fw_cfg, "
>"which this machine type does not provide", 
> VMGENID_DEVICE);
> return;
> @@ -239,7 +233,6 @@ static void vmgenid_device_class_init(ObjectClass *klass, 
> void *data)
> dc->vmsd = &vmstate_vmgenid;
> dc->realize = vmgenid_realize;
> dc->hotpluggable = false;
> -dc->props = vmgenid_properties;
> 
> object_class_property_add_str(klass, VMGENID_GUID, NULL,
>   vmgenid_set_guid, NULL);
> -- 
> 2.13.1.395.gf7b71de06
> 

Re: [Qemu-devel] [PATCH] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

> On Jun 1, 2017, at 7:52 AM, Michael S. Tsirkin  wrote:
> 
> On Thu, Jun 01, 2017 at 07:46:24AM -0700, Ben Warren wrote:
>> 
>>> On Jun 1, 2017, at 7:21 AM, Michael S. Tsirkin  wrote:
>>> 
>>> On Thu, Jun 01, 2017 at 08:10:27AM +, Marc-André Lureau wrote:
>>>> Hi
>>>> 
>>>> On Mon, May 29, 2017 at 7:18 PM Ben Warren via Qemu-devel <
>>>> qemu-devel@nongnu.org> wrote:
>>>> 
>>>>   From: Ben Warren 
>>>> 
>>>>   The following tests are implemented:
>>>>   * test that a GUID passed in by command line is propagated to the guest.
>>>> Read the GUID from guest memory
>>>>   * test that the "auto" argument to the GUID generates a valid GUID, as
>>>> seen by the guest.
>>>>   * test that a GUID passed in can be queried from the monitor
>>>> 
>>>> This patch is loosely based on a previous patch from:
>>>> Gal Hammer   and Igor Mammedov 
>>>> 
>>>> 
>>>>   Signed-off-by: Ben Warren 
>>>>   Reviewed-by: Igor Mammedov 
>>>>   ---
>>> 
>>> What happened to indentation here?
>>> 
>> I don’t know, this is unchanged from the patch that passed through the 
>> gauntlet earlier this year, other than rebasing.  If you can please clarify 
>> exactly what you see that is wrong, that’ll be helpful.
> 
> That question is directed to Marc-André - when he replied to your patch
> he has corrupted the indentation.

OK :)  Thanks
> 
>>>>   + * in order to implement the "OVMF SDT Header probe
>>>>   suppressor"
>>>>   + * see docs/specs/vmgenid.txt for more details
>>>>   + */
>>>>   +return vgid_table.vgia_val + VMGENID_GUID_OFFSET;
>>>>   +}
>>>> 
>>>> 
>>>> It leaks tables
>>>> 
>>>> Other than that:
>>>> Reviewed-by: Marc-André Lureau 
>>> 
>>> Ben, are you going to post a version that doesn't leak?
>> Yes, I’ll try to get to it today.
>> 
>> —Ben

Re: [Qemu-devel] [PATCH] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

> On Jun 1, 2017, at 7:21 AM, Michael S. Tsirkin  wrote:
> 
> On Thu, Jun 01, 2017 at 08:10:27AM +, Marc-André Lureau wrote:
>> Hi
>> 
>> On Mon, May 29, 2017 at 7:18 PM Ben Warren via Qemu-devel <
>> qemu-devel@nongnu.org> wrote:
>> 
>>From: Ben Warren 
>> 
>>The following tests are implemented:
>>* test that a GUID passed in by command line is propagated to the guest.
>>  Read the GUID from guest memory
>>* test that the "auto" argument to the GUID generates a valid GUID, as
>>  seen by the guest.
>>* test that a GUID passed in can be queried from the monitor
>> 
>>  This patch is loosely based on a previous patch from:
>>  Gal Hammer   and Igor Mammedov 
>> 
>>Signed-off-by: Ben Warren 
>>Reviewed-by: Igor Mammedov 
>>---
> 
> What happened to indentation here?
> 
I don’t know, this is unchanged from the patch that passed through the gauntlet 
earlier this year, other than rebasing.  If you can please clarify exactly what 
you see that is wrong, that’ll be helpful.

>>+ * in order to implement the "OVMF SDT Header probe
>>suppressor"
>>+ * see docs/specs/vmgenid.txt for more details
>>+ */
>>+return vgid_table.vgia_val + VMGENID_GUID_OFFSET;
>>+}
>> 
>> 
>> It leaks tables
>>  
>> Other than that:
>> Reviewed-by: Marc-André Lureau 
> 
> Ben, are you going to post a version that doesn't leak?
Yes, I’ll try to get to it today.

—Ben

[Qemu-devel] [PATCH] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

From: Ben Warren 

The following tests are implemented:
* test that a GUID passed in by command line is propagated to the guest.
  Read the GUID from guest memory
* test that the "auto" argument to the GUID generates a valid GUID, as
  seen by the guest.
* test that a GUID passed in can be queried from the monitor

  This patch is loosely based on a previous patch from:
  Gal Hammer   and Igor Mammedov 

Signed-off-by: Ben Warren 
Reviewed-by: Igor Mammedov 
---

Unchanged from v8, simply rebased to current top-of-tree

 tests/Makefile.include |   2 +
 tests/vmgenid-test.c   | 200 +
 2 files changed, 202 insertions(+)
 create mode 100644 tests/vmgenid-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 75893838e5..79bf960336 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -250,6 +250,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
 check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += 
tests/vhost-user-test$(EXESUF)
 ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
@@ -758,6 +759,7 @@ tests/test-uuid$(EXESUF): tests/test-uuid.o 
$(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
 tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
+tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/acpi-utils.o
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< 
,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
new file mode 100644
index 00..123beaea13
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,200 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include 
+#include 
+#include 
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "acpi-utils.h"
+#include "libqtest.h"
+
+#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+#define VMGENID_GUID_OFFSET 40   /* allow space for
+  * OVMF SDT Header Probe Supressor
+  */
+#define RSDP_ADDR_INVALID 0x10 /* RSDP must be below this address */
+#define RSDP_SLEEP_US 10   /* Sleep for 100ms between tries */
+#define RSDP_TRIES_MAX100  /* Max total time is 10 seconds */
+
+typedef struct {
+AcpiTableHeader header;
+gchar name_op;
+gchar vgia[4];
+gchar val_op;
+uint32_t vgia_val;
+} QEMU_PACKED VgidTable;
+
+static uint32_t acpi_find_vgia(void)
+{
+uint32_t off;
+AcpiRsdpDescriptor rsdp_table;
+uint32_t rsdt;
+AcpiRsdtDescriptorRev1 rsdt_table;
+int tables_nr;
+uint32_t *tables;
+AcpiTableHeader ssdt_table;
+VgidTable vgid_table;
+int i;
+
+/* Tables may take a short time to be set up by the guest */
+for (i = 0; i < RSDP_TRIES_MAX; i++) {
+off = acpi_find_rsdp_address();
+if (off < RSDP_ADDR_INVALID) {
+break;
+}
+g_usleep(RSDP_SLEEP_US);
+}
+g_assert_cmphex(off, <, RSDP_ADDR_INVALID);
+
+acpi_parse_rsdp_table(off, &rsdp_table);
+
+rsdt = rsdp_table.rsdt_physical_address;
+/* read the header */
+ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
+ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+
+/* compute the table entries in rsdt */
+tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+sizeof(uint32_t);
+g_assert_cmpint(tables_nr, >, 0);
+
+/* get the addresses of the tables pointed by rsdt */
+tables = g_new0(uint32_t, tables_nr);
+ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
+
+for (i = 0; i < tables_nr; i++) {
+ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+/* the first entry in the table should be VGIA
+ * That's all we need
+ */
+ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
+g_assert(vgid_table.name_op == 0x08);  /* name */
+ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
+g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
+ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+g_assert(vgid_table.val_op == 0x0C);  /* dword */
+ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
+/* The GUID is written at a fixed offset into the fw_cfg file
+ * in order to implement the "

Re: [Qemu-devel] [PATCH v8 7/8] tests: Add unit tests for the VM Generation ID feature

[Ben Warren via Qemu-devel]

Hi,

> On Apr 21, 2017, at 3:14 AM, Marc-André Lureau  
> wrote:
> 
> Hi,
> 
> Was this patch intentionally dropped from the series?
> 

Good question.  I thought the whole patch series was pulled in, but it looks 
like this one was not.  I guess we’ll see what Michael has to say.

—Ben

> 
> 
> -- 
> Marc-André Lureau

Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description

[Ben Warren via Qemu-devel]

> On Apr 12, 2017, at 1:47 PM, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> On Thu, Apr 13, 2017 at 12:25 AM Ben Warren  > wrote:
>> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau > > wrote:
>> 
>> Hi
>> 
>> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren > > wrote:
>>> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau >> > wrote:
>>> 
>>> +Device Usage:
>>> +-
>>> +
>>> +The device has one property, which may be only be set using the command 
>>> line:
>>> +
>>> +  guid - sets the value of the GUID.  A special value "auto" instructs
>>> + QEMU to generate a new random GUID.
>>> +
>>> +For example:
>>> +
>>> +  QEMU  -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>>> +  QEMU  -device vmgenid,guid=auto
>>> 
>>> The default will keep uuid to null, should it be documented? Wouldn't it 
>>> make sense to default to auto?
>> 
>> There is no default - you have to supply a value. It’s up to whatever 
>> software is managing VM lifecycle to decide what value to pass in.  Always 
>> setting to ‘auto’ will cause a lot of churn within Windows that may or may 
>> not be acceptable to your use case.
>> 
>> 
>> Why would you have a vmgenid device if it's always null? Does that please 
>> some windows use-cases as well? 
>>  
> 
> I don’t get what you mean by this.  What device is always null?  Either the 
> device is instantiated or it isn’t.  If not there, Windows will not find a 
> device and I don’t know how derived objects (Invocation ID, etc.) are handled.
> 
> If you start a VM without specifying guid argument, you'll always have a 
> genid null uuid, event after a migration (this could have been handled by 
> qemu without requiring management layer, no?). I don't understand why auto 
> would create more churn than what the management layer would do by setting 
> new uuid for each VM started. Could you explain?
> 
Looks like there’s a bug.  GUID should be a mandatory parameter.  As for the 
churn, I’ll give you one example.  If an Active Directory Domain Controller 
(ADDC) detects a change in VM Generation ID, it takes this to mean that the VM 
has been rolled back in time, and so its replication sequence numbers are 
“dirty”.  This has the effect of causing the Domain controller to perform a 
full “pull replication” with other ADDCs.  In large deployments this can be 
costly.  VM Generation ID is used by other applications besides AD.
> 
>>>  
>>> +The property may be queried via QMP/HMP:
>>> +
>>> +  (QEMU) query-vm-generation-id
>>> +  {"return": {"guid": "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"}}
>>> +
>>> +Setting of this parameter is intentionally left out from the QMP/HMP
>>> +interfaces.  There are no known use cases for changing the GUID once QEMU 
>>> is
>>> +running, and adding this capability would greatly increase the complexity.
>>>  
>>> Is this supposed to be not permitted?
>>> 
>>> { "execute": "qom-set", "arguments": { "path": 
>>> "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" } 
>>> }
>>> 
>>> Is there any linux kernel support being worked on?
>> 
>> This isn’t really relevant to the Linux kernel, at least in any way I can 
>> think of.  What did you have in mind?
>> 
>> Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
> 
> OK, so you mean a guest driver.  I do have one that needs work to go 
> upstream, but has been helpful to me in testing.
> https://github.com/ben-skyportsystems/vmgenid-test 
> 
> 
> Thanks, that's exactly what I was looking for :) 

Good.  I wish I had the time to integrate this upstream, but it’s one of those 
things that is good enough, and so will have to wait for another time.
> -- 
> Marc-André Lureau

Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description

[Ben Warren via Qemu-devel]

> On Apr 12, 2017, at 1:22 PM, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> On Thu, Apr 13, 2017 at 12:17 AM Ben Warren  > wrote:
>> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau > > wrote:
>> 
>> +Device Usage:
>> +-
>> +
>> +The device has one property, which may be only be set using the command 
>> line:
>> +
>> +  guid - sets the value of the GUID.  A special value "auto" instructs
>> + QEMU to generate a new random GUID.
>> +
>> +For example:
>> +
>> +  QEMU  -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>> +  QEMU  -device vmgenid,guid=auto
>> 
>> The default will keep uuid to null, should it be documented? Wouldn't it 
>> make sense to default to auto?
> 
> There is no default - you have to supply a value. It’s up to whatever 
> software is managing VM lifecycle to decide what value to pass in.  Always 
> setting to ‘auto’ will cause a lot of churn within Windows that may or may 
> not be acceptable to your use case.
> 
> 
> Why would you have a vmgenid device if it's always null? Does that please 
> some windows use-cases as well? 
>  
I don’t get what you mean by this.  What device is always null?  Either the 
device is instantiated or it isn’t.  If not there, Windows will not find a 
device and I don’t know how derived objects (Invocation ID, etc.) are handled.
>>  
>> +The property may be queried via QMP/HMP:
>> +
>> +  (QEMU) query-vm-generation-id
>> +  {"return": {"guid": "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"}}
>> +
>> +Setting of this parameter is intentionally left out from the QMP/HMP
>> +interfaces.  There are no known use cases for changing the GUID once QEMU is
>> +running, and adding this capability would greatly increase the complexity.
>>  
>> Is this supposed to be not permitted?
>> 
>> { "execute": "qom-set", "arguments": { "path": 
>> "/machine/peripheral-anon/device[1]", "property": "guid", "value": "auto" } }
>> 
>> Is there any linux kernel support being worked on?
> 
> This isn’t really relevant to the Linux kernel, at least in any way I can 
> think of.  What did you have in mind?
> 
> Testing, but apparently we do have RFE for RHEL as Laszlo pointed out.
OK, so you mean a guest driver.  I do have one that needs work to go upstream, 
but has been helpful to me in testing.
https://github.com/ben-skyportsystems/vmgenid-test 


> 
> Thanks
> -- 
> Marc-André Lureau

Re: [Qemu-devel] [PULL 02/15] docs: VM Generation ID device description

[Ben Warren via Qemu-devel]

> On Apr 12, 2017, at 1:06 PM, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> On Thu, Mar 2, 2017 at 10:22 AM Michael S. Tsirkin  > wrote:
> From: Ben Warren mailto:b...@skyportsystems.com>>
> 
> This patch is based off an earlier version by
> Gal Hammer (gham...@redhat.com )
> 
> Requirements section, ASCII diagrams and overall help
> provided by Laszlo Ersek (ler...@redhat.com )
> 
> Signed-off-by: Gal Hammer mailto:gham...@redhat.com>>
> Signed-off-by: Ben Warren  >
> Reviewed-by: Laszlo Ersek mailto:ler...@redhat.com>>
> Reviewed-by: Igor Mammedov mailto:imamm...@redhat.com>>
> Reviewed-by: Michael S. Tsirkin mailto:m...@redhat.com>>
> Signed-off-by: Michael S. Tsirkin mailto:m...@redhat.com>>
> ---
>  docs/specs/vmgenid.txt | 245 
> +
>  1 file changed, 245 insertions(+)
>  create mode 100644 docs/specs/vmgenid.txt
> 
> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
> new file mode 100644
> index 000..aa9f518
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,245 @@
> +VIRTUAL MACHINE GENERATION ID
> +=
> +
> +Copyright (C) 2016 Red Hat, Inc.
> +Copyright (C) 2017 Skyport Systems, Inc.
> +
> +This work is licensed under the terms of the GNU GPL, version 2 or later.
> +See the COPYING file in the top-level directory.
> +
> +===
> +
> +The VM generation ID (vmgenid) device is an emulated device which
> +exposes a 128-bit, cryptographically random, integer value identifier,
> +referred to as a Globally Unique Identifier, or GUID.
> +
> +This allows management applications (e.g. libvirt) to notify the guest
> +operating system when the virtual machine is executed with a different
> +configuration (e.g. snapshot execution or creation from a template).  The
> +guest operating system notices the change, and is then able to react as
> +appropriate by marking its copies of distributed databases as dirty,
> +re-initializing its random number generator etc.
> +
> +
> +Requirements
> +
> +
> +These requirements are extracted from the "How to implement virtual machine
> +generation ID support in a virtualization platform" section of the
> +specification, dated August 1, 2012.
> +
> +
> +The document may be found on the web at:
> +  http://go.microsoft.com/fwlink/?LinkId=260709 
> 
> +
> +R1a. The generation ID shall live in an 8-byte aligned buffer.
> +
> +R1b. The buffer holding the generation ID shall be in guest RAM, ROM, or 
> device
> + MMIO range.
> +
> +R1c. The buffer holding the generation ID shall be kept separate from areas
> + used by the operating system.
> +
> +R1d. The buffer shall not be covered by an AddressRangeMemory or
> + AddressRangeACPI entry in the E820 or UEFI memory map.
> +
> +R1e. The generation ID shall not live in a page frame that could be mapped 
> with
> + caching disabled. (In other words, regardless of whether the generation 
> ID
> + lives in RAM, ROM or MMIO, it shall only be mapped as cacheable.)
> +
> +R2 to R5. [These AML requirements are isolated well enough in the Microsoft
> +  specification for us to simply refer to them here.]
> +
> +R6. The hypervisor shall expose a _HID (hardware identifier) object in the
> +VMGenId device's scope that is unique to the hypervisor vendor.
> +
> +
> +QEMU Implementation
> +---
> +
> +The above-mentioned specification does not dictate which ACPI descriptor 
> table
> +will contain the VM Generation ID device.  Other implementations (Hyper-V and
> +Xen) put it in the main descriptor table (Differentiated System Description
> +Table or DSDT).  For ease of debugging and implementation, we have decided to
> +put it in its own Secondary System Description Table, or SSDT.
> +
> +The following is a dump of the contents from a running system:
> +
> +# iasl -p ./SSDT -d /sys/firmware/acpi/tables/SSDT
> +
> +Intel ACPI Component Architecture
> +ASL+ Optimizing Compiler version 20150717-64
> +Copyright (c) 2000 - 2015 Intel Corporation
> +
> +Reading ACPI table from file /sys/firmware/acpi/tables/SSDT - Length
> +0198 (0xC6)
> +ACPI: SSDT 0x C6 (v01 BOCHS  VMGENID  0001 BXPC
> +0001)
> +Acpi table [SSDT] successfully installed and loaded
> +Pass 1 parse of [SSDT]
> +Pass 2 parse of [SSDT]
> +Parsing Deferred Opcodes (Methods/Buffers/Packages/Regions)
> +
> +Parsing completed
> +Disassembly completed
> +ASL Output:./SSDT.dsl - 1631 bytes
> +# cat SSDT.dsl
> +/*
> + * Intel ACPI Component Architecture
> + * AML/ASL+ Disassembler version 20150717-64
> + * Copyright (c) 2000 - 2015 Intel Corporation
> + *
> + * Disassembling to symbolic ASL+ operators
> + *
> + * Disassembly of /sys/firmware/acpi/tables/SSDT, Sun Feb  5 00:19:37 2017
> + *
> + * Original Table Header:
> + * Signature"SSDT"
>