Re: [libvirt] [PATCH v2 2/2] drm/i915/gvt: export mdev device version to sysfs for Intel vGPU
On Mon, May 06, 2019 at 03:41:58PM +0800, Zhenyu Wang wrote: > On 2019.05.06 11:20:32 +0800, Zhenyu Wang wrote: > > On 2019.05.05 21:51:02 -0400, Yan Zhao wrote: > > > This feature implements the version attribute for Intel's vGPU mdev > > > devices. > > > > > > version attribute is rw. > > > It's used to check device compatibility for two mdev devices. > > > version string format and length are private for vendor driver. vendor > > > driver is able to define them freely. > > > > > > For Intel vGPU of gen8 and gen9, the mdev device version > > > consists of 3 fields: "vendor id" + "device id" + "mdev type". > > > > > > Reading from a vGPU's version attribute, a string is returned in below > > > format: --. e.g. > > > 8086-193b-i915-GVTg_V5_2. > > > > > > Writing a string to a vGPU's version attribute will trigger GVT to check > > > whether a vGPU identified by the written string is compatible with > > > current vGPU owning this version attribute. errno is returned if the two > > > vGPUs are incompatible. The length of written string is returned in > > > compatible case. > > > > > > For other platforms, and for GVT not supporting vGPU live migration > > > feature, errnos are returned when read/write of mdev devices' version > > > attributes. > > > > > > For old GVT versions where no version attributes exposed in sysfs, it is > > > regarded as not supporting vGPU live migration. > > > > > > For future platforms, besides the current 2 fields in vendor proprietary > > > part, more fields may be added to identify Intel vGPU well for live > > > migration purpose. > > > > > > v2: > > > 1. removed 32 common part of version string > > > (Alex Williamson) > > > 2. do not register version attribute for GVT not supporting live > > > migration.(Cornelia Huck) > > > 3. for platforms out of gen8, gen9, return -EINVAL --> -ENODEV for > > > incompatible. (Cornelia Huck) > > > > > > Cc: Alex Williamson > > > Cc: Erik Skultety > > > Cc: "Dr. David Alan Gilbert" > > > Cc: Cornelia Huck > > > Cc: "Tian, Kevin" > > > Cc: Zhenyu Wang > > > Cc: "Wang, Zhi A" > > > c: Neo Jia > > > Cc: Kirti Wankhede > > > > > > Signed-off-by: Yan Zhao > > > --- > > > drivers/gpu/drm/i915/gvt/Makefile | 2 +- > > > drivers/gpu/drm/i915/gvt/device_version.c | 87 +++ > > > drivers/gpu/drm/i915/gvt/gvt.c| 51 + > > > drivers/gpu/drm/i915/gvt/gvt.h| 6 ++ > > > 4 files changed, 145 insertions(+), 1 deletion(-) > > > create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c > > > > > > diff --git a/drivers/gpu/drm/i915/gvt/Makefile > > > b/drivers/gpu/drm/i915/gvt/Makefile > > > index 271fb46d4dd0..54e209a23899 100644 > > > --- a/drivers/gpu/drm/i915/gvt/Makefile > > > +++ b/drivers/gpu/drm/i915/gvt/Makefile > > > @@ -3,7 +3,7 @@ GVT_DIR := gvt > > > GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o > > > firmware.o \ > > > interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ > > > execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o > > > debugfs.o \ > > > - fb_decoder.o dmabuf.o page_track.o > > > + fb_decoder.o dmabuf.o page_track.o device_version.o > > > > > > ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) > > > i915-y += $(addprefix $(GVT_DIR)/, > > > $(GVT_SOURCE)) > > > diff --git a/drivers/gpu/drm/i915/gvt/device_version.c > > > b/drivers/gpu/drm/i915/gvt/device_version.c > > > new file mode 100644 > > > index ..bd4cdcbdba95 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/i915/gvt/device_version.c > > > @@ -0,0 +1,87 @@ > > > +/* > > > + * Copyright(c) 2011-2017 Intel Corporation. All rights reserved. > > > + * > > > + * Permission is hereby granted, free of charge, to any person obtaining > > > a > > > + * copy of this software and associated documentation files (the > > > "Software"), > > > + * to deal in the Software without restriction, including without > > > limitation > > > + * the rights to use, copy, modify, merge, publish, distribute, > > > sublicense, > > > + * and/or sell copies of the Software, and to permit persons to whom the > > > + * Software is furnished to do so, subject to the following conditions: > > > + * > > > + * The above copyright notice and this permission notice (including the > > > next > > > + * paragraph) shall be included in all copies or substantial portions of > > > the > > > + * Software. > > > + * > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > > EXPRESS OR > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > > MERCHANTABILITY, > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > > > SHALL > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > > OTHER > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > > ARISING FROM, > > > + * OUT OF OR IN CONNECTION WITH THE SO
Re: [libvirt] [PATCH 1/2] vfio/mdev: add version field as mandatory attribute for mdev device
On Tue, Apr 30, 2019 at 11:29:08PM +0800, Cornelia Huck wrote: > On Wed, 24 Apr 2019 04:15:58 -0400 > Yan Zhao wrote: > > > On Wed, Apr 24, 2019 at 03:56:24PM +0800, Cornelia Huck wrote: > > > On Tue, 23 Apr 2019 23:10:37 -0400 > > > Yan Zhao wrote: > > > > > > > On Tue, Apr 23, 2019 at 05:59:32PM +0800, Cornelia Huck wrote: > > > > > On Fri, 19 Apr 2019 04:35:04 -0400 > > > > > Yan Zhao wrote: > > > > > > > @@ -225,6 +228,8 @@ Directories and files under the sysfs for Each > > > > > > Physical Device > > > > > >[], device_api, and available_instances are mandatory > > > > > > attributes > > > > > >that should be provided by vendor driver. > > > > > > > > > > > > + version is a mandatory attribute if a mdev device supports live > > > > > > migration. > > > > > > > > > > What about "An mdev device wishing to support live migration must > > > > > provide the version attribute."? > > > > yes, I just want to keep consistent with the line above it > > > > " [], device_api, and available_instances are mandatory > > > > attributes > > > > that should be provided by vendor driver." > > > > what about below one? > > > > "version is a mandatory attribute if a mdev device wishing to support > > > > live > > > > migration." > > > > > > My point is that an attribute is not mandatory if it can be left out :) > > > (I'm not a native speaker, though; maybe this makes perfect sense > > > after all?) > > > > > > Maybe "version is a required attribute if live migration is supported > > > for an mdev device"? > > > > > you are right, "mandatory" may bring some confusion. > > Maybe > > "vendor driver must provide version attribute for an mdev device wishing to > > support live migration." ? > > based on your first version :) > > "The vendor driver must provide the version attribute for any mdev > device it wishes to support live migration for." ? > > > > > > > > > > > > > > > > > + > > > > > > * [] > > > > > > > > > > > >The [] name is created by adding the device driver > > > > > > string as a prefix > > > > > > @@ -246,6 +251,35 @@ Directories and files under the sysfs for Each > > > > > > Physical Device > > > > > >This attribute should show the number of devices of type > > > > > > that can be > > > > > >created. > > > > > > > > > > > > +* version > > > > > > + > > > > > > + This attribute is rw. It is used to check whether two devices > > > > > > are compatible > > > > > > + for live migration. If this attribute is missing, then the > > > > > > corresponding mdev > > > > > > + device is regarded as not supporting live migration. > > > > > > + > > > > > > + It consists of two parts: common part and vendor proprietary > > > > > > part. > > > > > > + common part: 32 bit. lower 16 bits is vendor id and higher 16 > > > > > > bits identifies > > > > > > + device type. e.g., for pci device, it is > > > > > > + "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16). > > > > > > + vendor proprietary part: this part is varied in length. vendor > > > > > > driver can > > > > > > + specify any string to identify a device. > > > > > > + > > > > > > + When reading this attribute, it should show device version > > > > > > string of the device > > > > > > + of type . If a device does not support live migration, > > > > > > it should > > > > > > + return errno. > > > > > > + When writing a string to this attribute, it returns errno for > > > > > > incompatibility > > > > > > + or returns written string length in compatibility case. If a > > > > > > device does not > > > > > > + support live migration, it always returns errno. > > > > > > > > > > I'm not sure whether a device that does not support live migration > > > > > should expose this attribute in the first place. Or is that to cover > > > > > cases where a driver supports live migration only for some of the > > > > > devices it supports? > > > > yes, driver returning error code is to cover the cases where only part > > > > of devices it > > > > supports can be migrated. > > > > > > > > > > > > > Also, I'm not sure if a string that has to be parsed is a good idea... > > > > > is this 'version' attribute supposed to convey some human-readable > > > > > information as well? The procedure you describe for compatibility > > > > > checking does the checking within the vendor driver which I would > > > > > expect to have a table/rules for that anyway. > > > > right. if a vendor driver has the confidence to migrate between devices > > > > of > > > > diffent platform or mdev types, it can maintain a compatibility table > > > > for that > > > > purpose. That's the reason why we would leave the compatibility check > > > > to vendor > > > > driver. vendor driver can freely choose its own complicated way to > > > > decide > > > > which device is migratable to which device. > > > > > > I think there are two scenarios here: > > > - Migrating between different device types, which is unlikely to
Re: [libvirt] [PATCH v8 10/21] conf: Add parameter to virDomainDiskSourceFormat
On 4/25/19 7:46 AM, Peter Krempa wrote: > On Wed, Apr 17, 2019 at 09:09:10 -0500, Eric Blake wrote: >> Commits 4bc42986 and 218c81ea removed virDomainStorageSourceFormat on >> the grounds that there were no external callers; however, the upcoming >> backup code wants to output a (push mode) or (pull >> mode) element that is in all other respects identical to a domain's >> element, where the previous virDomainStorageSourceFormat fit >> the bill nicely. But rather than reverting the commits, it's easier to >> just add an additional parameter for the element name to use, and >> update all callers. >> >> Signed-off-by: Eric Blake >> --- >> src/conf/domain_conf.h | 1 + >> src/conf/domain_conf.c | 14 +- >> src/conf/snapshot_conf.c | 3 ++- >> src/qemu/qemu_domain.c | 2 +- >> tests/qemublocktest.c| 3 ++- >> tests/virstoragetest.c | 2 +- >> 6 files changed, 16 insertions(+), 9 deletions(-) > > ACK I've pushed this one. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v8 16/21] backup: qemu: Implement metadata tracking for checkpoint APIs
On 4/26/19 8:42 AM, Peter Krempa wrote: > On Wed, Apr 17, 2019 at 09:09:16 -0500, Eric Blake wrote: >> A lot of this work heavily copies from the existing snapshot >> APIs. The interaction with qemu during create/delete still >> needs to be implemented, but this takes care of all the libvirt >> metadata (saving and restoring XML, and tracking the relations >> between multiple checkpoints). > > I'd prefer at least the 'qemu_conf' related stuff to be separate. > >> @@ -242,6 +244,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool >> privileged) >> goto error; >> if (virAsprintf(&cfg->snapshotDir, "%s/qemu/snapshot", >> cfg->configBaseDir) < 0) >> goto error; >> +if (virAsprintf(&cfg->checkpointDir, "%s/qemu/checkpoint", >> cfg->configBaseDir) < 0) >> +goto error; > > This directory will need to be versionable as we need to record > alternate histories when creating snapshots. I'm not sure how to achieve > thant though. We'll probably need to add a directory into 'snapshot' > folder for each snapshot which will copy the whole checkpoint tree. No, we don't need versioned directories. Rather, we simply rely on the fact that checkpoints, like snapshots, are already stored as a tree relationship, where one checkpoint can have more than one child where those multiple children are created across the original snapshot vs. the revert to the snapshot. We may need one extra XML line in snapshots to record which checkpoint to use as parent of any child created during a revert, but that should be sufficient. >> +/* Easiest way to clone inactive portion of vm->def is via >> + * conversion in and back out of xml. */ >> +if (!(xml = qemuDomainDefFormatLive(driver, vm->def, priv->origCPU, >> +true, true)) || >> +!(def->common.dom = virDomainDefParseString(xml, caps, >> driver->xmlopt, NULL, >> + >> VIR_DOMAIN_DEF_PARSE_INACTIVE | >> + >> VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) >> +goto cleanup; >> + >> +if (virDomainCheckpointAlignDisks(def) < 0 || >> +qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) > > This is always done when starting the qemu instance and on every update, > thus should not be necessary. In my initial demos, I was running into instances where it was not happening correctly. But if I can remove it and things work, all the better. > > Also note that if necessary it must be guarded by > if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV) && > > With blockdev we shouldn't ever do that. > >> +static int >> +qemuDomainHasCurrentCheckpoint(virDomainPtr domain, >> + unsigned int flags) > > This API seems a bit pointless if you can call > qemuDomainCheckpointCurrent. It mirrors what snapshots provide. But I can always try to factor it out into a separate patch, and add it back in only if we need it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v8 07/21] backup: Allow for lists of checkpoint objects
On 4/25/19 7:45 AM, Peter Krempa wrote:
> On Wed, Apr 17, 2019 at 09:09:07 -0500, Eric Blake wrote:
>> Create a new file for managing a list of checkpoint objects, borrowing
>> heavily from existing virDomainSnapshotObjList paradigms.
>>
>> Note that while checkpoints definitely have a use case for multiple
>> children to a single parent (create a base snapshot, create a child
>> snapshot, revert to the base, then create another child snapshot),
>> it's harder to predict how checkpoints will play out with reverting to
>> prior points in time. Thus, in initial use, we may find that in a list
>> of checkpoints, you never have more than one child. However, as the
>> snapshot machinery is already generic, it is easier to reuse the
>> generic machinery that tracks relations between domain moments than it
>> is to open-code a new list-management scheme just for checkpoints.
>>
>> The virDomainMomentObjList code is not quite polymorphic enough until
>> we patch virDomainMomentDef to be a proper virObject, but doing that
>> is a bigger audit. So for now, I had to duplicate the cleanup calls
>> based on a bool flag for which type needs cleaning. Oh well.
>
> I'm afraid that with this being committed the motivation to refactor it
> properly will be gone.
Then I'll try harder to get that audit done before v9.
>> +++ b/src/conf/virdomaincheckpointobjlist.c
>> @@ -0,0 +1,223 @@
>
> [...]
>
>> +
>> +#define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT
>> +
>> +VIR_LOG_INIT("conf.virdomaincheckpointobjlist");
>> +
>> +struct _virDomainCheckpointObjList {
>> +virDomainMomentObjListPtr base;
>> +};
>
> Is this just for compile time typechecking?
Pretty much (but also because I don't have the VirObject inheritance
going, so doing that for snapshots may change what we need here as well).
>> +static int
>> +virDomainCheckpointObjListGetNames(virDomainCheckpointObjListPtr
>> checkpoints,
>> + virDomainMomentObjPtr from,
>> + char **const names,
>> + int maxnames,
>> + unsigned int flags)
>> +{
>> +/* We intentionally chose our public flags to match the common flags */
>> +verify(VIR_DOMAIN_CHECKPOINT_LIST_ROOTS ==
>> + (int) VIR_DOMAIN_MOMENT_LIST_ROOTS);
>> +verify(VIR_DOMAIN_CHECKPOINT_LIST_TOPOLOGICAL ==
>> + (int) VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL);
>> +verify(VIR_DOMAIN_CHECKPOINT_LIST_LEAVES ==
>> + (int) VIR_DOMAIN_MOMENT_LIST_LEAVES);
>> +verify(VIR_DOMAIN_CHECKPOINT_LIST_NO_LEAVES ==
>> + (int) VIR_DOMAIN_MOMENT_LIST_NO_LEAVES);
>> +verify(VIR_DOMAIN_CHECKPOINT_LIST_METADATA ==
>> + (int) VIR_DOMAIN_MOMENT_LIST_METADATA);
>> +verify(VIR_DOMAIN_CHECKPOINT_LIST_NO_METADATA ==
>> + (int) VIR_DOMAIN_MOMENT_LIST_NO_METADATA);
>
> This looks like it should be near the VIR_DOMAIN_CHECKPOINT_LIST enum
> rather than somewhere in the code randomly.
Okay, I can try to move it.
>> +
>> +static void
>> +virDomainSnapshotObjListDataFree(void *payload,
>> + const void *name ATTRIBUTE_UNUSED)
>> +{
>> +virDomainMomentObjPtr obj = payload;
>> +
>> +virDomainSnapshotObjFree(obj);
>> }
>>
>>
>> virDomainMomentObjListPtr
>> -virDomainMomentObjListNew(void)
>> +virDomainMomentObjListNew(bool snapshot)
>
> Please create a new entrypoint for this. I don't see the need to
> overload it using the argument.
Better than a bool would be a virClassPtr that describes the class of
the virObject to construct. So yes, making things properly polymorphic
is worthwhile to make this code cleaner.
>
>> {
>> virDomainMomentObjListPtr moments;
>>
>> if (VIR_ALLOC(moments) < 0)
>> return NULL;
>> -moments->objs = virHashCreate(50, virDomainMomentObjListDataFree);
>> +moments->objs = virHashCreate(50,
>> + snapshot ?
>> virDomainSnapshotObjListDataFree :
>> + virDomainCheckpointObjListDataFree);
With virObject in play, the destructor is already automatically
polymorphic (with virObjectUnref() being the common call to trigger the
correct cleanup).
>> if (!moments->objs) {
>> VIR_FREE(moments);
>> return NULL;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v8 06/21] backup: Parse and output checkpoint XML
On 4/24/19 10:53 AM, Peter Krempa wrote:
> On Wed, Apr 17, 2019 at 09:09:06 -0500, Eric Blake wrote:
>> Add a new file checkpoint_conf.c that performs the translation to and
>> from new XML describing a checkpoint. The code shares a common base
>> class with snapshots, since a checkpoint similarly represents the
>> domain state at a moment in time. Add some basic testing of round trip
>> XML handling through the new code.
>>
>> +bitmap = virXMLPropString(node, "bitmap");
>> +if (bitmap) {
>> +if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("disk checkpoint bitmap '%s' requires "
>> + "type='bitmap'"),
>> + bitmap);
>
> This should be expressable by schema. If we make schema validation
> mandatory for the new APIs which we should anyways we can skip all of
> these as we'll automagically get them validated before.
We have several existing APIs that have not yet been taught flags to
perform validation (virDomainManagedSave, for example). Do we really
want all new APIs to perform unconditional schema validation, or do we
want to finish the ongoing project to add flags to existing APIs and
merely insist that new APIs also be given a schema validation flag? So
far, my implementation has not wired in any schema validation.
>> +static virDomainCheckpointDefPtr
>> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
>> +virCapsPtr caps,
>> +virDomainXMLOptionPtr xmlopt,
>> +bool *current,
>> +unsigned int flags)
>> +{
>> +virDomainCheckpointDefPtr def = NULL;
>> +virDomainCheckpointDefPtr ret = NULL;
>> +xmlNodePtr *nodes = NULL;
>
> I'm not going to point out that we have more modern approaches here ...
>
>> +size_t i;
>> +int n;
>> +char *creation = NULL;
>> +int active;
>> +char *tmp;
>> +
>> +if (VIR_ALLOC(def) < 0)
>> +goto cleanup;
>> +
>> +def->common.name = virXPathString("string(./name)", ctxt);
>> +if (def->common.name == NULL) {
>> +if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
>
> ... but validation of the XML should not be intermixed with the parser
> itself. The parser should only validate what is described by the schema.
>
> (I'm aware that there is plenty prior art, but adding new code just
> increases the technical debt and this kind of stuff is hard to refactor
> later)
This is copying from the snapshot code. If we desire, it may be worth
cleaning up the snapshot code to use the modern approaches first, and
then the checkpoint code should resemble the cleaned up version (rather
than having to clean up both copies at once).
Do we want mandatory schema validation for the new API? Then again, a
lot of things in the schema are optional - in particular, it is okay for
a user to define a new checkpoint without a name (we create a name
during post-parse processing); but NOT okay to use the redefine flag
without a name - but the schema doesn't really have a way to express a
conditional (aspects of the XML that are optional under one use, but
mandatory under another - where that use is an external flag rather
than something in the XML itself).
>> +/**
>> + * virDomainCheckpointDefAssignBitmapNames:
>> + * @def: checkpoint def object
>> + *
>> + * Generate default bitmap names for checkpoint targets. Returns 0 on
>> + * success, -1 on error.
>
> Isn't a bitmap a qemuism?
Yes, bitmaps are how qemu implements checkpoints. But per patch 1/21,
the XML being proposed uses:
+ checkpoint
+ An optional attribute; possible values
+are no when the disk does not participate
+in this checkpoint; or bitmap if the disk
+will track all changes since the creation of this
+checkpoint via a bitmap.
+ bitmap
+ The attribute bitmap is only valid
+if checkpoint='bitmap'; it describes the
+name of the tracking bitmap (defaulting to the
+checkpoint name).
It is conceivable that adding checkpoint support for another driver may
pick a different (third) checkpoint='MODE', at which point that mode
will add a different attribute that ties in with that mode (other than
qemu's checkpoint='bitmap' bitmap='NAME'). So parsing and assigning the
'bitmap' name, when checkpoint='bitmap', makes sense here, while
generating a default bitmap name starts to be a bit more specific to
qemu. Is the question whether this function should be moved into
src/qemu/ somewhere, rather than generic to the checkpoint XML
parse/format code?
>> +/* Align def->disks to def->domain. Sort the list of def->disks,
>> + * filling in any missing disks with appropriate default. Convert
>> + * paths to disk targets for uniformity. Issue an error
Re: [libvirt] [PATCH v8 04/21] backup: Introduce virDomainBackup APIs
On 4/24/19 8:50 AM, Peter Krempa wrote: > On Wed, Apr 17, 2019 at 09:09:04 -0500, Eric Blake wrote: >> Introduce a few new public APIs related to incremental backups. This >> builds on the previous notion of a checkpoint (without an existing >> checkpoint, the new API is a full backup, differing from >> virDomainBlockCopy in the point of time chosen and in operation on >> multiple disks at once); and also allows creation of a new checkpoint >> at the same time as starting the backup (after all, an incremental >> backup is only useful if it covers the state since the previous >> backup). Snapshot creation is also a point in time at which creating >> a checkpoint atomically can be useful; as checkpoints are independent >> objects, it is not worth embedding inside >> , and therefore we need a more powerful version of >> virDomainSnapshotCreateXML(), where we borrow from the naming pattern >> of virDomainMigrate2() and friends. >> >> A backup job also affects filtering a listing of domains, as well as >> adding event reporting for signaling when a push model backup >> completes (where the hypervisor creates the backup); note that the >> pull model does not have an event (starting the backup lets a third >> party access the data, and only the third party knows when it is >> finished). >> >> Since multiple backup jobs can be run in parallel in the future (well, >> qemu doesn't support it yet, but we don't want to preclude the idea), >> virDomainBackupBegin() returns a positive job id, and the id is also >> visible in the backup XML. But until a future libvirt release adds a >> bunch of APIs related to parallel job management where job ids will >> actually matter, the documentation is also clear that job id 0 means >> the 'currently running backup job' (provided one exists), for use in >> virDomainBackupGetXMLDesc() and virDomainBackupEnd(). >> >> The full list of new APIs: >> virDomainBackupBegin; >> virDomainBackupEnd; >> virDomainBackupGetXMLDesc; >> virDomainSnapshotCreateXML2; >> >> Signed-off-by: Eric Blake >> Reviewed-by: Daniel P. Berrangé >> --- >> include/libvirt/libvirt-domain-snapshot.h | 8 +- >> include/libvirt/libvirt-domain.h | 41 +++- >> src/driver-hypervisor.h | 21 +++ >> src/qemu/qemu_blockjob.h | 1 + >> examples/object-events/event-test.c | 3 + >> src/conf/domain_conf.c| 2 +- >> src/libvirt-domain-snapshot.c | 89 + >> src/libvirt-domain.c | 219 ++ >> src/libvirt_public.syms | 4 + >> tools/virsh-domain.c | 8 +- >> 10 files changed, 390 insertions(+), 6 deletions(-) > > [...] > >> >> @@ -78,6 +78,12 @@ virDomainSnapshotPtr >> virDomainSnapshotCreateXML(virDomainPtr domain, >> const char *xmlDesc, >> unsigned int flags); >> >> +/* Take a snapshot of the current VM state, possibly creating a checkpoint >> */ >> +virDomainSnapshotPtr virDomainSnapshotCreateXML2(virDomainPtr domain, >> + const char *xmlDesc, >> + const char *checkpointXml, >> + unsigned int flags); >> + > > This feels weird in this patch. It does not deal with backup in any way. It > would > be better to have this separately or at least together with checkpoint > creating. Particularly true if we decide that the XML for a domain snapshot needs to track which checkpoint was active at the time the snapshot was created, as well as adding a counterpart API for virDomainSnapshotRevert() in order to create a new bitmap at the time of a revert action. And since I have not actually reached the point of demonstrating this API in action in the qemu driver, I agree with pulling it out of this patch, and instead saving it for a separate API patch that deals with any other API additions (such as virDomainSnapshotRevert2(), if needed) when we do figure out how we want to support snapshots and checkpoints on the same domain, even if it is not in the same release as the initial incremental backup support. >> +typedef int >> +(*virDrvDomainBackupEnd)(virDomainPtr domain, int id, unsigned int flags); > > Since the getter APIs are overloaded on top of shouldn't this be > overloaded with qemuDomainAbortJob as well? > Possibly - except virDomainAbortJob() takes no flags and no job id. So the best I can do is teach virDomainAbortJob(dom) to be a synonym for virDomainBackupEnd(dom, 0, VIR_DOMAIN_BACKUP_END_ABORT) to abort an ongoing backup, but only when there is exactly one ongoing backup, and only when there are no other parallel jobs (at least the initial implementation says that backup and migration are mutually exclusive, so that is not too hard to figure out). Down the r
Re: [libvirt] [PATCH] x86: Multi-key Total Memory Encryption (Intel)
Hello Peter,
Thank you for the feedback.
I'll submit another patch series.
I have a question regarding the test cap replies file.
We are querying Qemu to get the capability information.
The Qemu patch is not yet upstream, hence, we are not able to create the
replies file for the same.
Is it ok to submit the patch for the feedback without the replies file?
Since the Qemu support for MKTME is not yet upstream and without Qemu support
we cannot merge this changes to Libvirt.
Regards,
Larkins
-Original Message-
From: Peter Krempa [mailto:pkre...@redhat.com]
Sent: Monday, May 6, 2019 1:39 AM
To: Carvalho, Larkins L
Cc: libvir-list@redhat.com; Mohammed, Karimullah
Subject: Re: [libvirt] [PATCH] x86: Multi-key Total Memory Encryption (Intel)
On Wed, May 01, 2019 at 14:41:57 -0700, Larkins Carvalho wrote:
> From: llcarval
>
> Total Memory Encryption (TME) – provides the capability to encrypt the
> entirety of the physical memory of a system. MKTME builds on TME and
> adds support for multiple encryption keys.
>
> High Level flow:
>
> 1. Management tool calls virNodeGetMKTMEInfo. This returns an XML
> document that includes the following
>
>
> ...
>
>
>
>
>
> 2. Management tool requests to start a guest calling virCreateXML().
> The xml would include
>
>
> m0
> user
> samplekey
> aes-xts-128
>
>
> 3. Libvirt makes system call with the provided information to generate a key
> handle using linux keyring services.
> Qemu uses the key handle to launch the workload.
>
> 4. Libvirt generate the QEMU cli arg to enable the MKTME feature, a
> typical args looks like this:
>
> -machine memory-encryption=m0 \
> -object mktme-guest,id=m0,handle=${serial}
>
> Intel MKTME spec:
> https://software.intel.com/sites/default/files/managed/a5/16/Multi-Key
> -Total-Memory-Encryption-Spec.pdf
>
> WIP: Qemu and KVM patch to support Intel MKTME are in the process of
> development and community approval.
>
> The purpose of this initial review is to get on par with libvirt development
> and the proposed Intel MKTME feature in libvirt.
> Considering we have not added tests, this is a preliminary patch and based on
> the community feedback, we expect more updates to follow.
>
> TODO:
> Add tests for launch security of type mktme.
> Update domaincommon.rng to add attribute of type mktme.
> ---
> docs/formatdomain.html.in | 62 +-
> docs/formatdomaincaps.html.in | 18 +
> docs/schemas/domaincaps.rng | 14 +
> include/libvirt/libvirt-host.h| 18 +
> src/conf/domain_capabilities.c| 29 +
> src/conf/domain_capabilities.h| 12 +
> src/conf/domain_conf.c| 114 +-
> src/conf/domain_conf.h| 13 +
> src/conf/virconftypes.h |3 +
> src/driver-hypervisor.h |7 +
> src/libvirt-host.c| 48 +
> src/libvirt_private.syms |5 +
> src/libvirt_public.syms |5 +
> src/qemu/qemu_capabilities.c | 130 +-
> src/qemu/qemu_capabilities.h |4 +
> src/qemu/qemu_capspriv.h |4 +
> src/qemu/qemu_command.c | 40 +
> src/qemu/qemu_driver.c| 63 +
> src/qemu/qemu_monitor.c |9 +
> src/qemu/qemu_monitor.h |5 +
> src/qemu/qemu_monitor_json.c | 52 +
> src/qemu/qemu_monitor_json.h |3 +
> src/remote/remote_daemon_dispatch.c | 43 +
> src/remote/remote_driver.c| 40 +-
> src/remote/remote_protocol.x | 21 +-
> src/remote_protocol-structs | 12 +
> src/util/Makefile.inc.am |2 +
> src/util/virmktme.c | 112 ++
> src/util/virmktme.h | 33 +
> .../bhyve_basic.x86_64.xml|1 +
> .../bhyve_fbuf.x86_64.xml |1 +
> .../bhyve_uefi.x86_64.xml |1 +
> tests/domaincapsschemadata/empty.xml |1 +
> tests/domaincapsschemadata/libxl-xenfv.xml|1 +
> tests/domaincapsschemadata/libxl-xenpv.xml|1 +
> .../qemu_1.7.0.x86_64.xml |1 +
> .../qemu_2.12.0-virt.aarch64.xml |1 +
> .../qemu_2.12.0.ppc64.xml |1 +
> .../qemu_2.12.0.s390x.xml |1 +
> .../qemu_2.12.0.x86_64.xml|1 +
> .../qemu_2.6.0-virt.aarch64.xml |1 +
> .../qemu_2.6.0.aarch64.xml|1 +
> .../domaincapsschemadata/qemu_2.6.0.ppc64.xml |1 +
> .../qemu_2.6.0.x86_64.xml |1 +
> .../domaincapsschemadata/qemu_2.7.0.s390x.xml |1 +
> .../qemu_2.8.0-t
Re: [libvirt] [PATCH 03/12] qemu: Split entry points to qemuDomainStorageSourceChainAccessPrepare
On Thu, Apr 18, 2019 at 04:42:58PM +0200, Peter Krempa wrote: Introduce qemuDomainStorageSourceChainAccess(Allow|Revoke) as entry points to qemuDomainStorageSourceChainAccessPrepare for symmetry with the functions for single backing chain elements. Signed-off-by: Peter Krempa --- src/qemu/qemu_domain.c | 20 +++- src/qemu/qemu_domain.h | 10 ++ src/qemu/qemu_hotplug.c | 12 ++-- 3 files changed, 31 insertions(+), 11 deletions(-) Reviewed-by: Ján Tomko Thank you, Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/12] qemu: Move and rename qemuHotplugPrepareDiskSourceAccess
On Thu, Apr 18, 2019 at 04:42:57PM +0200, Peter Krempa wrote: Move it to qemu_domain.c and call it qemuDomainStorageSourceChainAccessPrepare. Signed-off-by: Peter Krempa --- src/qemu/qemu_domain.c | 68 +++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_hotplug.c | 80 - 3 files changed, 79 insertions(+), 74 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/12] qemu: Rename qemuDomainDiskChainElement(Revoke|Prepare)
On Thu, Apr 18, 2019 at 04:42:56PM +0200, Peter Krempa wrote: Use qemuDomainStorageSourceAccess(Allow|Revoke) instead. Signed-off-by: Peter Krempa --- src/qemu/qemu_domain.c | 20 ++-- src/qemu/qemu_domain.h | 16 src/qemu/qemu_driver.c | 24 3 files changed, 30 insertions(+), 30 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] lib: Build sources before running 'check-access'
If the source tree was freshly configured and no objects are built yet then 'make check-access' has no test to run. Build the sources beforehand. Signed-off-by: Michal Privoznik --- Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index eba5916352..0d8bb733e6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -83,7 +83,7 @@ srpm: clean check-local: all tests -check-access: +check-access: all @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access) MAINTAINERCLEANFILES = .git-module-status -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] Trivial fixes mocking fixes
I'm not pushing these just yet. I'm leaving some time for our grammar enthusiasts to fix my grammar. Michal Prívozník (3): virfilemock: Init symbols in canonicalize_file_name() virtestmock: Initialize symbols from stat() and its friends lib: Build sources before running 'check-access' Makefile.am | 2 +- tests/virfilemock.c | 3 +++ tests/virtestmock.c | 4 +++- 3 files changed, 7 insertions(+), 2 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] virtestmock: Initialize symbols from stat() and its friends
Introduced by ff376c6283c97. Previously, init_syms() was called from stat() mock and its friends. This is crucial because checkPath() might call printFile() which in turn calls real_fopen(). But if stat() or one of its friends is the first function called then because of lacking init_syms() call no real_* is initialized. The other thing is that we really want the recorded action to be "stat" instead of __FUNCTION__ because there's no good in recording that it was __xstat64 who touched some file. Signed-off-by: Michal Privoznik --- tests/virtestmock.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/virtestmock.c b/tests/virtestmock.c index bc62312444..ad6958ac87 100644 --- a/tests/virtestmock.c +++ b/tests/virtestmock.c @@ -190,7 +190,9 @@ int access(const char *path, int mode) } -#define VIR_MOCK_STAT_HOOK CHECK_PATH(path) +#define VIR_MOCK_STAT_HOOK \ +init_syms(); \ +checkPath(path, "stat") #include "virmockstathelpers.c" -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] virfilemock: Init symbols in canonicalize_file_name()
If a program that is using this mock calls canonicalize_file_name()
as the very first function then it will face SIGSEGV because
real_canonicalize_file_name is uninitialized.
Signed-off-by: Michal Privoznik
---
tests/virfilemock.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/virfilemock.c b/tests/virfilemock.c
index 89e14c5b67..106032f857 100644
--- a/tests/virfilemock.c
+++ b/tests/virfilemock.c
@@ -177,6 +177,9 @@ statfs(const char *path, struct statfs *buf)
char *
canonicalize_file_name(const char *path)
{
+
+init_syms();
+
if (getenv("LIBVIRT_MTAB")) {
const char *p;
char *ret;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] tests: domaincaps: Add QEMU 4.0.0 for s390x
On Fri, 2019-05-03 at 10:47 +0200, Boris Fiuczynski wrote:
[...]
> +++ b/tests/domaincapstest.c
> @@ -453,6 +453,10 @@ mymain(void)
> DO_TEST_QEMU("4.0.0", "caps_4.0.0",
> "/usr/bin/qemu-system-x86_64", NULL,
> "x86_64", VIR_DOMAIN_VIRT_KVM);
> +
> +DO_TEST_QEMU("4.0.0", "caps_4.0.0",
> + "/usr/bin/qemu-system-s390x", NULL,
> + "s390x", VIR_DOMAIN_VIRT_KVM);
It's pretty annoying that we have to update this test program
manually every time new .replies are added. Easy to miss, too, and
in fact we seem to only have coverage for about half the possible
input files.
We should modify this so that dropping .replies into the source
directory is enough for the test program to pick them up, just like
qemucapabilitiestest and qemucaps2xmltest do.
But that's work for another day, so in the meantime
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/12] qemu: Refactor image permission/lock setting (blockdev-add saga)
On Thu, Apr 18, 2019 at 16:42:55 +0200, Peter Krempa wrote: > With new blockjob handling we'll need to modify permissions for chains > and individual images. The individual image code was universally > accessible but the chain setting code reimplemented it mostly only in > qemu_hotplug.h. > > Refactor the handling by moving the code to qemu_domain.c and making it > universal. Ping? signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] tests: Add capabilities for QEMU 4.0.0 on s390x
On Fri, 2019-05-03 at 10:47 +0200, Boris Fiuczynski wrote: > In addition adjusting iothreads-virtio-scsi-ccw.s390x-latest.args to prevent > accidential drive id exposure by QEMU fixed by commit a1dce96236f > (qemu: Use the 'device_id' property of SCSI disks to avoid regressing). > > Signed-off-by: Boris Fiuczynski > --- > .../caps_4.0.0.s390x.replies | 21514 > .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2894 +++ > ...othreads-virtio-scsi-ccw.s390x-latest.args | 4 +- > 3 files changed, 24410 insertions(+), 2 deletions(-) > create mode 100644 tests/qemucapabilitiesdata/caps_4.0.0.s390x.replies > create mode 100644 tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml With the necessary bits of patch 1/3 squashed in and the commit message updated accordingly, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] tests: Add capabilities for QEMU 3.1.0 on s390x
On Mon, 2019-05-06 at 13:23 +0200, Boris Fiuczynski wrote: > On 5/3/19 1:04 PM, Andrea Bolognani wrote: > > As for the patch itself: while it looks fine, personally I don't > > feel we need to have capabilities for every single QEMU version on > > every single architecture, so considering that you're adding 4.0.0 > > in the same series and there aren't any 3.1.0-specific tests, I > > would say let's just skip this one version and add capabilitie for > > 4.0.0 only. Does that sound reasonable? > > In general I kind of disagree because if the s390x capabilities for > every qemu version would have exited the commits e8c2c8bd078 and > a1dce96236f mentioned would not have missed the changes in the s390x > related args files. In this case it is already history and therefor > skipping 3.1.0 by moving the *.args files from the 3.1.0 patch 1 into 3 > and removing patch 2 feels OK. Alright, I'll review patches 3 and 4 with this in mind then. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Move tools under tests/tools/
On 5/6/19 1:58 PM, Andrea Bolognani wrote: On Mon, 2019-05-06 at 10:01 +0200, Michal Privoznik wrote: [...] +++ b/configure.ac @@ -925,6 +925,7 @@ AC_CONFIG_FILES([\ include/libvirt/libvirt-common.h \ examples/Makefile \ tests/Makefile \ +tests/tools/Makefile \ Are we sure we want to have a tests/tools/Makefile.am rather than a tests/tools/Makefile.inc.am that we include from tests/Makefile.am here? We seem to be going in the former direction, though I'm not actually sure what the trade-offs are. CC'ing Dan who introduced the Makefile.inc.am files in the first place. I don't think that will work. The problem is that Makefile.inc.am would still be included from tests/Makefile.am and thus all the relative paths would be evaluated in relation to that. That is the reason why for instance src/qemu/Makefile.inc.am lists source files as qemu/qemu_*.c and all the libraries/binaries it builds are placed right under src/. What I'd like to have is for all these tools to live under separate directory - even the compiled binaries. And this is the point where my automake knowledge was weak enough so the only solution I was able to come up with was a separate Makefile. But I'm open to learn something new. [...] +++ b/tests/Makefile.am EXTRA_DIST += \ - check-file-access.pl \ - file_access_whitelist.txt + tools/check-file-access.pl \ + tools/file_access_whitelist.txt These should go into the new Makefile(.inc).am, just like you've done with .valgrind.supp and oomtrace.pl. Shoot. I wanted to move these. Consider fixed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Move tools under tests/tools/
On Mon, May 06, 2019 at 01:58:49PM +0200, Andrea Bolognani wrote:
On Mon, 2019-05-06 at 10:01 +0200, Michal Privoznik wrote:
[...]
+++ b/configure.ac
@@ -925,6 +925,7 @@ AC_CONFIG_FILES([\
include/libvirt/libvirt-common.h \
examples/Makefile \
tests/Makefile \
+tests/tools/Makefile \
Are we sure we want to have a tests/tools/Makefile.am rather than a
tests/tools/Makefile.inc.am that we include from tests/Makefile.am
including a makefile one level up helps paralellism, especialy if
there's not much done in it.
Jano
here? We seem to be going in the former direction, though I'm not
actually sure what the trade-offs are. CC'ing Dan who introduced
the Makefile.inc.am files in the first place.
[...]
+++ b/tests/Makefile.am
EXTRA_DIST += \
- check-file-access.pl \
- file_access_whitelist.txt
+ tools/check-file-access.pl \
+ tools/file_access_whitelist.txt
These should go into the new Makefile(.inc).am, just like you've
done with .valgrind.supp and oomtrace.pl.
Everything else looks sane from a very cursory looks, but I'd like
to put it through its paces ('make distcheck', 'make rpm', ...)
before ACKing it.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Move tools under tests/tools/
On Mon, 2019-05-06 at 10:01 +0200, Michal Privoznik wrote:
[...]
> +++ b/configure.ac
> @@ -925,6 +925,7 @@ AC_CONFIG_FILES([\
> include/libvirt/libvirt-common.h \
> examples/Makefile \
> tests/Makefile \
> +tests/tools/Makefile \
Are we sure we want to have a tests/tools/Makefile.am rather than a
tests/tools/Makefile.inc.am that we include from tests/Makefile.am
here? We seem to be going in the former direction, though I'm not
actually sure what the trade-offs are. CC'ing Dan who introduced
the Makefile.inc.am files in the first place.
[...]
> +++ b/tests/Makefile.am
> EXTRA_DIST += \
> - check-file-access.pl \
> - file_access_whitelist.txt
> + tools/check-file-access.pl \
> + tools/file_access_whitelist.txt
These should go into the new Makefile(.inc).am, just like you've
done with .valgrind.supp and oomtrace.pl.
Everything else looks sane from a very cursory looks, but I'd like
to put it through its paces ('make distcheck', 'make rpm', ...)
before ACKing it.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] tests: Add capabilities for QEMU 3.1.0 on s390x
On 5/3/19 1:04 PM, Andrea Bolognani wrote: On Fri, 2019-05-03 at 10:46 +0200, Boris Fiuczynski wrote: In addition adjusting *s390x-latest.args files to qemu deprecation changes made in commit e8c2c8bd078 (Prefer '-overcommit mem-lock' over -realtime mlock'). Signed-off-by: Boris Fiuczynski --- .../caps_3.1.0.s390x.replies | 20893 .../qemucapabilitiesdata/caps_3.1.0.s390x.xml | 2728 ++ ...othreads-virtio-scsi-ccw.s390x-latest.args | 2 +- .../s390x-ccw-graphics.s390x-latest.args | 2 +- .../s390x-ccw-headless.s390x-latest.args | 2 +- .../vhost-vsock-ccw-auto.s390x-latest.args| 2 +- .../vhost-vsock-ccw.s390x-latest.args | 2 +- 7 files changed, 23626 insertions(+), 5 deletions(-) create mode 100644 tests/qemucapabilitiesdata/caps_3.1.0.s390x.replies create mode 100644 tests/qemucapabilitiesdata/caps_3.1.0.s390x.xml A few things. First of all, while this message reached my inbox just fine, it didn't reach the list yet because of the sheer size, which is way over the configured limit and thus got stuck in the moderation, needing manual intervention to get through. In the future, please consider sending a snipped version to the mailing list and making the full changes available somewhere, like I've done for example with https://www.redhat.com/archives/libvir-list/2019-April/msg01490.html You also mention adjusting some .args files: I hope you haven't done so manually, but rather executed VIR_TEST_REGENERATE_OUTPUT=1 make check and let it fix the output files for you. If not, TYL ;) Thanks, I used VIR_TEST_REGENERATE_OUTPUT but not with make check. As for the patch itself: while it looks fine, personally I don't feel we need to have capabilities for every single QEMU version on every single architecture, so considering that you're adding 4.0.0 in the same series and there aren't any 3.1.0-specific tests, I would say let's just skip this one version and add capabilitie for 4.0.0 only. Does that sound reasonable? In general I kind of disagree because if the s390x capabilities for every qemu version would have exited the commits e8c2c8bd078 and a1dce96236f mentioned would not have missed the changes in the s390x related args files. In this case it is already history and therefor skipping 3.1.0 by moving the *.args files from the 3.1.0 patch 1 into 3 and removing patch 2 feels OK. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] x86: Multi-key Total Memory Encryption (Intel)
On Wed, May 01, 2019 at 14:41:57 -0700, Larkins Carvalho wrote:
> From: llcarval
>
> Total Memory Encryption (TME) – provides the capability to encrypt the
> entirety of the physical memory of a system. MKTME builds on TME and
> adds support for multiple encryption keys.
>
> High Level flow:
>
> 1. Management tool calls virNodeGetMKTMEInfo. This returns an XML document
> that includes the following
>
>
> ...
>
>
>
>
>
> 2. Management tool requests to start a guest calling virCreateXML(). The xml
> would include
>
>
> m0
> user
> samplekey
> aes-xts-128
>
>
> 3. Libvirt makes system call with the provided information to generate a key
> handle using linux keyring services.
> Qemu uses the key handle to launch the workload.
>
> 4. Libvirt generate the QEMU cli arg to enable the MKTME feature, a typical
> args looks like this:
>
> -machine memory-encryption=m0 \
> -object mktme-guest,id=m0,handle=${serial}
>
> Intel MKTME spec:
> https://software.intel.com/sites/default/files/managed/a5/16/Multi-Key-Total-Memory-Encryption-Spec.pdf
>
> WIP: Qemu and KVM patch to support Intel MKTME are in the process of
> development and community approval.
>
> The purpose of this initial review is to get on par with libvirt development
> and the proposed Intel MKTME feature in libvirt.
> Considering we have not added tests, this is a preliminary patch and based on
> the community feedback, we expect more updates to follow.
>
> TODO:
> Add tests for launch security of type mktme.
> Update domaincommon.rng to add attribute of type mktme.
> ---
> docs/formatdomain.html.in | 62 +-
> docs/formatdomaincaps.html.in | 18 +
> docs/schemas/domaincaps.rng | 14 +
> include/libvirt/libvirt-host.h| 18 +
> src/conf/domain_capabilities.c| 29 +
> src/conf/domain_capabilities.h| 12 +
> src/conf/domain_conf.c| 114 +-
> src/conf/domain_conf.h| 13 +
> src/conf/virconftypes.h |3 +
> src/driver-hypervisor.h |7 +
> src/libvirt-host.c| 48 +
> src/libvirt_private.syms |5 +
> src/libvirt_public.syms |5 +
> src/qemu/qemu_capabilities.c | 130 +-
> src/qemu/qemu_capabilities.h |4 +
> src/qemu/qemu_capspriv.h |4 +
> src/qemu/qemu_command.c | 40 +
> src/qemu/qemu_driver.c| 63 +
> src/qemu/qemu_monitor.c |9 +
> src/qemu/qemu_monitor.h |5 +
> src/qemu/qemu_monitor_json.c | 52 +
> src/qemu/qemu_monitor_json.h |3 +
> src/remote/remote_daemon_dispatch.c | 43 +
> src/remote/remote_driver.c| 40 +-
> src/remote/remote_protocol.x | 21 +-
> src/remote_protocol-structs | 12 +
> src/util/Makefile.inc.am |2 +
> src/util/virmktme.c | 112 ++
> src/util/virmktme.h | 33 +
> .../bhyve_basic.x86_64.xml|1 +
> .../bhyve_fbuf.x86_64.xml |1 +
> .../bhyve_uefi.x86_64.xml |1 +
> tests/domaincapsschemadata/empty.xml |1 +
> tests/domaincapsschemadata/libxl-xenfv.xml|1 +
> tests/domaincapsschemadata/libxl-xenpv.xml|1 +
> .../qemu_1.7.0.x86_64.xml |1 +
> .../qemu_2.12.0-virt.aarch64.xml |1 +
> .../qemu_2.12.0.ppc64.xml |1 +
> .../qemu_2.12.0.s390x.xml |1 +
> .../qemu_2.12.0.x86_64.xml|1 +
> .../qemu_2.6.0-virt.aarch64.xml |1 +
> .../qemu_2.6.0.aarch64.xml|1 +
> .../domaincapsschemadata/qemu_2.6.0.ppc64.xml |1 +
> .../qemu_2.6.0.x86_64.xml |1 +
> .../domaincapsschemadata/qemu_2.7.0.s390x.xml |1 +
> .../qemu_2.8.0-tcg.x86_64.xml |1 +
> .../domaincapsschemadata/qemu_2.8.0.s390x.xml |1 +
> .../qemu_2.8.0.x86_64.xml |1 +
> .../qemu_2.9.0-q35.x86_64.xml |1 +
> .../qemu_2.9.0-tcg.x86_64.xml |1 +
> .../qemu_2.9.0.x86_64.xml |1 +
> .../domaincapsschemadata/qemu_3.0.0.s390x.xml |1 +
> .../qemu_3.1.0.x86_64.xml |1 +
> .../qemu_4.0.0.x86_64.xml |1 +
> .../qemu_5.0.0.x86_64.xml | 164 ++
> tests/domaincapstest.c|4 +
> .../caps_5.0.0.x86_64.xml | 1389 +
Please be advised that we will not accept everything stuffed into one
commi
Re: [libvirt] [PATCH 5/5] tests: Add capabilities for QEMU 4.0.0 on ppc64
On Mon, Apr 29, 2019 at 18:25:55 +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > .../caps_4.0.0.ppc64.replies | 23960 > .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1077 + > 2 files changed, 25037 insertions(+) > create mode 100644 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.replies > create mode 100644 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] tests: Add capabilities for QEMU 4.0.0 on aarch64
On Mon, Apr 29, 2019 at 18:25:54 +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > The changes in the .args file seem entirely justified by the fact > that those for aarch64-virt-headless and aarch64-virt-graphics use > the new command line option too. ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] tests: Refresh capabilities for QEMU 4.0.0 on riscv64
On Mon, Apr 29, 2019 at 18:25:53 +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > tests/qemucapabilitiesdata/caps_4.0.0.riscv64.replies | 10 +- > tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 4 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] tests: Refresh capabilities for QEMU 4.0.0 on riscv32
On Mon, Apr 29, 2019 at 18:25:52 +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > tests/qemucapabilitiesdata/caps_4.0.0.riscv32.replies | 10 +- > tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 4 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) ACK signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: Move tools under tests/tools/
There are some scripts/binaries that are not tests themselves but
rather fulfill support purpose. Separate them from the rest of
the tests.
Signed-off-by: Michal Privoznik
---
.gitignore | 2 +-
Makefile.am | 2 +-
cfg.mk | 4 +-
configure.ac| 1 +
tests/Makefile.am | 22 +-
tests/qemucapabilitiestest.c| 4 +-
tests/testutils.c | 2 +-
tests/{ => tools}/.valgrind.supp| 0
tests/tools/Makefile.am | 85 +
tests/{ => tools}/check-file-access.pl | 0
tests/{ => tools}/file_access_whitelist.txt | 0
tests/{ => tools}/group-qemu-caps.pl| 0
tests/{ => tools}/oomtrace.pl | 0
tests/{ => tools}/qemucapsprobe.c | 0
tests/{ => tools}/qemucapsprobemock.c | 0
tests/{ => tools}/test-wrap-argv.pl | 2 +-
16 files changed, 98 insertions(+), 26 deletions(-)
rename tests/{ => tools}/.valgrind.supp (100%)
create mode 100644 tests/tools/Makefile.am
rename tests/{ => tools}/check-file-access.pl (100%)
rename tests/{ => tools}/file_access_whitelist.txt (100%)
rename tests/{ => tools}/group-qemu-caps.pl (100%)
rename tests/{ => tools}/oomtrace.pl (100%)
rename tests/{ => tools}/qemucapsprobe.c (100%)
rename tests/{ => tools}/qemucapsprobemock.c (100%)
rename tests/{ => tools}/test-wrap-argv.pl (98%)
diff --git a/.gitignore b/.gitignore
index 16eb4a3e2e..c231d394f3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -170,7 +170,7 @@
/tests/*.trs
/tests/*test
/tests/commandhelper
-/tests/qemucapsprobe
+/tests/tools/qemucapsprobe
!/tests/virsh-self-test
!/tests/virt-aa-helper-test
!/tests/virt-admin-self-test
diff --git a/Makefile.am b/Makefile.am
index eba5916352..875c0fa997 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -17,7 +17,7 @@
##
Re: [libvirt] [PATCH v2 2/2] drm/i915/gvt: export mdev device version to sysfs for Intel vGPU
On 2019.05.06 11:20:32 +0800, Zhenyu Wang wrote:
> On 2019.05.05 21:51:02 -0400, Yan Zhao wrote:
> > This feature implements the version attribute for Intel's vGPU mdev
> > devices.
> >
> > version attribute is rw.
> > It's used to check device compatibility for two mdev devices.
> > version string format and length are private for vendor driver. vendor
> > driver is able to define them freely.
> >
> > For Intel vGPU of gen8 and gen9, the mdev device version
> > consists of 3 fields: "vendor id" + "device id" + "mdev type".
> >
> > Reading from a vGPU's version attribute, a string is returned in below
> > format: --. e.g.
> > 8086-193b-i915-GVTg_V5_2.
> >
> > Writing a string to a vGPU's version attribute will trigger GVT to check
> > whether a vGPU identified by the written string is compatible with
> > current vGPU owning this version attribute. errno is returned if the two
> > vGPUs are incompatible. The length of written string is returned in
> > compatible case.
> >
> > For other platforms, and for GVT not supporting vGPU live migration
> > feature, errnos are returned when read/write of mdev devices' version
> > attributes.
> >
> > For old GVT versions where no version attributes exposed in sysfs, it is
> > regarded as not supporting vGPU live migration.
> >
> > For future platforms, besides the current 2 fields in vendor proprietary
> > part, more fields may be added to identify Intel vGPU well for live
> > migration purpose.
> >
> > v2:
> > 1. removed 32 common part of version string
> > (Alex Williamson)
> > 2. do not register version attribute for GVT not supporting live
> > migration.(Cornelia Huck)
> > 3. for platforms out of gen8, gen9, return -EINVAL --> -ENODEV for
> > incompatible. (Cornelia Huck)
> >
> > Cc: Alex Williamson
> > Cc: Erik Skultety
> > Cc: "Dr. David Alan Gilbert"
> > Cc: Cornelia Huck
> > Cc: "Tian, Kevin"
> > Cc: Zhenyu Wang
> > Cc: "Wang, Zhi A"
> > c: Neo Jia
> > Cc: Kirti Wankhede
> >
> > Signed-off-by: Yan Zhao
> > ---
> > drivers/gpu/drm/i915/gvt/Makefile | 2 +-
> > drivers/gpu/drm/i915/gvt/device_version.c | 87 +++
> > drivers/gpu/drm/i915/gvt/gvt.c| 51 +
> > drivers/gpu/drm/i915/gvt/gvt.h| 6 ++
> > 4 files changed, 145 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/Makefile
> > b/drivers/gpu/drm/i915/gvt/Makefile
> > index 271fb46d4dd0..54e209a23899 100644
> > --- a/drivers/gpu/drm/i915/gvt/Makefile
> > +++ b/drivers/gpu/drm/i915/gvt/Makefile
> > @@ -3,7 +3,7 @@ GVT_DIR := gvt
> > GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o
> > firmware.o \
> > interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
> > execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o
> > debugfs.o \
> > - fb_decoder.o dmabuf.o page_track.o
> > + fb_decoder.o dmabuf.o page_track.o device_version.o
> >
> > ccflags-y += -I$(src) -I$(src)/$(GVT_DIR)
> > i915-y += $(addprefix $(GVT_DIR)/,
> > $(GVT_SOURCE))
> > diff --git a/drivers/gpu/drm/i915/gvt/device_version.c
> > b/drivers/gpu/drm/i915/gvt/device_version.c
> > new file mode 100644
> > index ..bd4cdcbdba95
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gvt/device_version.c
> > @@ -0,0 +1,87 @@
> > +/*
> > + * Copyright(c) 2011-2017 Intel Corporation. All rights reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > next
> > + * paragraph) shall be included in all copies or substantial portions of
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > IN THE
> > + * SOFTWARE.
> > + *
> > + * Authors:
> > + *Yan Zhao
> > + */
> > +#include
> > +#include "i915_drv.h"
> > +
> > +static bool is_compatible(const char *self, const char *remote)
> > +{
> > + if (strlen(remote) != strlen(self))
> > +
Re: [libvirt] [PATCH 3/3] virBuffer: Try harder to free buffer
On Fri, May 03, 2019 at 11:18:09AM +0200, Michal Privoznik wrote:
> On 5/3/19 11:01 AM, Erik Skultety wrote:
> > On Thu, Apr 18, 2019 at 02:11:25PM +0200, Michal Privoznik wrote:
> > > Currently, the way virBufferFreeAndReset() works is it relies on
> > > virBufferContentAndReset() to fetch the buffer content which is
> > > then freed. This works as long as there is no bug in virBuffer*
> > > implementation (not true apparently). Explicitly call free() over
> > > buffer content.
> > >
> > > Signed-off-by: Michal Privoznik
> > > ---
> > > src/util/virbuffer.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> > > index b2ae7963a1..ac03b15a61 100644
> > > --- a/src/util/virbuffer.c
> > > +++ b/src/util/virbuffer.c
> > > @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf)
> > >*/
> > > void virBufferFreeAndReset(virBufferPtr buf)
> > > {
> > > -char *str = virBufferContentAndReset(buf);
> > > -
> > > -VIR_FREE(str);
> > > +if (buf)
> > > +virBufferSetError(buf, 0);
> >
> > You saved 1 call to memset. I also don't see how we can break
> > virBufferContentAndReset in a way that we couldn't mess up
> > virBufferSetError in
> > the same way. Additionally, seeing a call to virBufferSetError in a free
> > helper
> > looks suspicious (even if it has a '0' as argument). If anything, I'd rename
> > virBuggerFreeAndReset to virBufferClean.
>
> Thing is, if 1/3 wasn't merged, then the test I'm introducing there would
> leak memory. But not with this patch merged. So from long term perspective,
> if there is ever some similar bug we won't leak any memory.
>
> Yeah, naming is hard.
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
