[libvirt] [PATCH v3 00/12] PCI passthrough support on s390

Abstract ======== The PCI representation in QEMU has recently been extended for S390 allowing configuration of zPCI attributes like uid (user-defined identifier) and fid (PCI function identifier). The details can be found here: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html To support the new zPCI feature of the S390 platform, two new XML attributes, @uid and @fid, are introduced for device addresses of type 'pci', i.e.: <hostdev mode='subsystem' type='pci'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> </hostdev> uid and fid are optional attributes. If they are defined by the user, unique values within the guest domain must be used. If they are not specified and the architecture requires them, they are automatically generated with non-conflicting values. Current implementation is the most seamless one for the user as it unites the address specific data of a PCI device on one XML element. It could accommodate both specifying our special parameters (uid and fid) and re-using standard statements (domain, bus, slot and function) for PCI devices. User can still specify bus/slot/function for the virtualized PCI devices in the XML. Thus uid/fid act as an extension to the PCI address and are stored in a new structure 'virZPCIDeviceAddress' which is a member of common PCI Address structure. Additionally, two hashtables are used for assignment and reservation of uid/fid. In support of extending the PCI address, a new PCI address extension flag is introduced. This extension flag allows is not only dedicated for the S390 platform but also other architectures needing certain extensions to PCI address space. Code Base ========= commit in master: 087de2f5a3: docs: formatdomain: fix spacing before parentheses Change Log ========== v2->v3: 1. Revise code style. 2. Update test cases. 3. Introduce qemuDomainCollectPCIAddressExtension() to collect PCI extension addresses. 4. Introduce virDeviceInfoPCIAddressExtensionPresent() to check if zPCI address exists. 5. Optimize zPCI address check logic. 6. Optimize passed parameters of zPCI addr alloc/release/reserve functions. 7. Report enum range error in qemuDomainDeviceSupportZPCI(). 8. Update commit messages. v1->v2: 1. Separate test commit and merge testcases into corresponding commits that introduce the functionalities firstly. 2. Spare some checks for zpci device. 3. Add vsock and controller support. 4. Add uin32 type schema. 5. Rename zpciuid and zpcifid to zpci_uid and zpci_fid. 6. Always return multibus support on S390. Yi Min Zhao (12): conf: Add definitions for 'uid' and 'fid' PCI address attributes qemu: Introduce zPCI capability conf: Introduce a new PCI address extension flag qemu: Enable PCI multi bus for S390 guests qemu: Auto add pci-root for s390/s390x guests conf: Introduce address caching for PCI extensions conf: Introduce parser, formatter for uid and fid conf: Allocate/release 'uid' and 'fid' in PCI address qemu: Generate and use zPCI device in QEMU command line qemu: Add hotpluging support for PCI devices on S390 guests docs: Add 'uid' and 'fid' information news: Update news for PCI address extension attributes docs/formatdomain.html.in | 9 +- docs/news.xml | 11 + docs/schemas/basictypes.rng | 23 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 78 +++++ src/conf/device_conf.h | 8 + src/conf/domain_addr.c | 379 +++++++++++++++++++++ src/conf/domain_addr.h | 29 ++ src/conf/domain_conf.c | 6 + src/libvirt_private.syms | 4 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 114 +++++++ src/qemu/qemu_command.h | 4 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain_address.c | 204 ++++++++++- src/qemu/qemu_hotplug.c | 155 ++++++++- src/util/virpci.h | 13 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 27 ++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 + .../hostdev-vfio-zpci-autogenerate.args | 26 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 + .../hostdev-vfio-zpci-boundaries.args | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 26 ++ .../hostdev-vfio-zpci-multidomain-many.args | 40 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 ++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 26 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++ tests/qemuxml2argvtest.c | 17 + tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 ++ .../hostdev-vfio-zpci-autogenerate.xml | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 42 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++ tests/qemuxml2xmltest.c | 14 + 41 files changed, 1575 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml -- Yi Min

Add zPCI definitions in preparation of extending the PCI address with parameters uid (user-defined identifier) and fid (PCI function identifier). Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/util/virpci.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/virpci.h b/src/util/virpci.h index 2ac87694df..b137eb01e6 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -37,12 +37,22 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr; +typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; +struct _virZPCIDeviceAddress { + unsigned int zpci_fid; + unsigned int zpci_uid; + bool fid_assigned; + bool uid_assigned; +}; + struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ + virZPCIDeviceAddressPtr zpci; }; typedef enum { -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; +struct _virZPCIDeviceAddress { + unsigned int zpci_fid; + unsigned int zpci_uid; + bool fid_assigned; + bool uid_assigned; +};
A couple of questions about the approach here, one of which I have mentioned already and one of which I probably haven't (my bad): * do you really need to have separate booleans tracking whether or not either id has been assigned? Wouldn't the same approach as virPCIDeviceAddressIsEmpty() work, eg. consider the address absent if both are zero and present otherwise? * especially if you don't need the additional booleans, would it be preferable to just add the two ids to the existing struct instead of declaring a new one that you'll have to allocate and make sure it's not NULL before accessing it? Again, I seem to remember Laine feeling somewhat strongly about the topic. Cosmetic issues: * uid should be listed before fid; * the zpci_ prefix is unnecessary if you have a separate struct that contains "ZPCI" in the name. But see above :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; +struct _virZPCIDeviceAddress { + unsigned int zpci_fid; + unsigned int zpci_uid; + bool fid_assigned; + bool uid_assigned; +}; A couple of questions about the approach here, one of which I have mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether or not either id has been assigned? Wouldn't the same approach as virPCIDeviceAddressIsEmpty() work, eg. consider the address absent if both are zero and present otherwise? It's OK for uid. But for fid, zero is a valid value, so we need a bool to track its assignment. If we add a boolean for fid, why not also add another one for uid to keep consistency? This also makes code easy to read and obvious. It doesn't waste much memory space.
* especially if you don't need the additional booleans, would it be preferable to just add the two ids to the existing struct instead of declaring a new one that you'll have to allocate and make sure it's not NULL before accessing it? Again, I seem to remember Laine feeling somewhat strongly about the topic. For other platforms, is it OK to waste this unused memory (of course, it's little) ?
Cosmetic issues:
* uid should be listed before fid;
* the zpci_ prefix is unnecessary if you have a separate struct that contains "ZPCI" in the name. But see above :)

On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote:
在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+struct _virZPCIDeviceAddress { + unsigned int zpci_fid; + unsigned int zpci_uid; + bool fid_assigned; + bool uid_assigned; +};
A couple of questions about the approach here, one of which I have mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether or not either id has been assigned? Wouldn't the same approach as virPCIDeviceAddressIsEmpty() work, eg. consider the address absent if both are zero and present otherwise?
It's OK for uid. But for fid, zero is a valid value, so we need a bool to track its assignment.
See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(), which have very similar requirement but don't use extra booleans to keep track of state. You could do the same thing those functions do: * the zPCI address is empty if both uid and fid are zero; * the zPCI address is invalid if it's empty or uid is too large. You already have the latter covered, so it's only a matter of implementing the former.
If we add a boolean for fid, why not also add another one for uid to keep consistency? This also makes code easy to read and obvious. It doesn't waste much memory space.
I think it actually makes more complicated, which is why I'd rather get rid of it :)
* especially if you don't need the additional booleans, would it be preferable to just add the two ids to the existing struct instead of declaring a new one that you'll have to allocate and make sure it's not NULL before accessing it? Again, I seem to remember Laine feeling somewhat strongly about the topic.
For other platforms, is it OK to waste this unused memory (of course, it's little) ?
I believe adding a couple of unsigned ints is worth it if we can get rid of all the checks on addr->zpci because of it. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/20 下午6:35, Andrea Bolognani 写道:
On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote:
在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+struct _virZPCIDeviceAddress { + unsigned int zpci_fid; + unsigned int zpci_uid; + bool fid_assigned; + bool uid_assigned; +}; A couple of questions about the approach here, one of which I have mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether or not either id has been assigned? Wouldn't the same approach as virPCIDeviceAddressIsEmpty() work, eg. consider the address absent if both are zero and present otherwise? It's OK for uid. But for fid, zero is a valid value, so we need a bool to track its assignment. See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(), which have very similar requirement but don't use extra booleans to keep track of state.
You could do the same thing those functions do:
* the zPCI address is empty if both uid and fid are zero; uid=0 and fid=0 can't mean zPCI address is empty, because the user might only define fid with 0. If fid=0 has been assigned, we should report error. If it is not defined by user, fid is also 0, then we should allocate a valid value for fid. * the zPCI address is invalid if it's empty or uid is too large.
You already have the latter covered, so it's only a matter of implementing the former.
If we add a boolean for fid, why not also add another one for uid to keep consistency? This also makes code easy to read and obvious. It doesn't waste much memory space. I think it actually makes more complicated, which is why I'd rather get rid of it :)
* especially if you don't need the additional booleans, would it be preferable to just add the two ids to the existing struct instead of declaring a new one that you'll have to allocate and make sure it's not NULL before accessing it? Again, I seem to remember Laine feeling somewhat strongly about the topic.
For other platforms, is it OK to waste this unused memory (of course, it's little) ? I believe adding a couple of unsigned ints is worth it if we can get rid of all the checks on addr->zpci because of it.

On Tue, 2018-08-21 at 11:24 +0800, Yi Min Zhao wrote:
在 2018/8/20 下午6:35, Andrea Bolognani 写道:
You could do the same thing those functions do:
* the zPCI address is empty if both uid and fid are zero;
uid=0 and fid=0 can't mean zPCI address is empty, because the user might only define fid with 0. If fid=0 has been assigned, we should report error. If it is not defined by user, fid is also 0, then we should allocate a valid value for fid.
For the PCI part <address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> behaves the same as <address type='pci'/> and results in a PCI address being allocated automatically by libvirt rather than an error being reported. As with zPCI, zero is a valid value for some, but not all, of the attributes: validation is performed if at least one of them is non-zero and might result in an error being reported. Doing the same with the zPCI part would not only allow you to get rid of the extra booleans but would also guarantee a consistent behavior, which is a worthy goal in and of itself. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/21 下午4:06, Andrea Bolognani 写道:
On Tue, 2018-08-21 at 11:24 +0800, Yi Min Zhao wrote:
在 2018/8/20 下午6:35, Andrea Bolognani 写道:
You could do the same thing those functions do:
* the zPCI address is empty if both uid and fid are zero; uid=0 and fid=0 can't mean zPCI address is empty, because the user might only define fid with 0. If fid=0 has been assigned, we should report error. If it is not defined by user, fid is also 0, then we should allocate a valid value for fid. For the PCI part
<address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>
behaves the same as
<address type='pci'/>
and results in a PCI address being allocated automatically by libvirt rather than an error being reported.
As with zPCI, zero is a valid value for some, but not all, of the attributes: validation is performed if at least one of them is non-zero and might result in an error being reported.
Doing the same with the zPCI part would not only allow you to get rid of the extra booleans but would also guarantee a consistent behavior, which is a worthy goal in and of itself.
I want to ask a question. For pci address, any pci device can't use slot 0. Is that a reason why PCI part could treat all zeros as empty address? For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid. Anyway, when I wrote this code, I also wanted to use the similar logic to check if zPCI address is empty.

On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
I want to ask a question. For pci address, any pci device can't use slot 0. Is that a reason why PCI part could treat all zeros as empty address?
A PCI address where all attributes are zero can't be used, so there's no ambiguity there; same for a zPCI address where all attributes are zero, which also can't be used.
For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid.
If the user wants to use a specific zPCI address they can simply specify both attributes, eg. uid=1,fid=0 will work just fine with the proposed approach and won't produce errors or cause a new zPCI address to be automatically assigned. If the user only specified fid=0 while leaving uid unspecified, well, an address is going to be picked for them. This is consistent with how PCI addresses are treated so it shouldn't be a problem: if anything, *deviating* from this behavior would cause confusion. -- Andrea Bolognani / Red Hat / Virtualization

I want to ask a question. For pci address, any pci device can't use slot 0. Is that a reason why PCI part could treat all zeros as empty address? A PCI address where all attributes are zero can't be used, so
On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote: there's no ambiguity there; same for a zPCI address where all attributes are zero, which also can't be used. Yes, uid must be non-zero value. But the user could only define fid like you mentioned below. Validation check happens during parsing xml. So if the user only defines fid=0, zpci address (fid=0, uid=0 although the user doesn't specify uid value) is going to be invalid as PCI address check logic. UID and FID must be unique separately, but we can't treat
在 2018/8/21 下午7:00, Andrea Bolognani 写道: them as a combination. This doesn't like PCI address(domain:bus:slot:function).
For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid. If the user wants to use a specific zPCI address they can simply specify both attributes, eg. uid=1,fid=0 will work just fine with the proposed approach and won't produce errors or cause a new zPCI address to be automatically assigned.
If the user only specified fid=0 while leaving uid unspecified, well, an address is going to be picked for them. This is consistent
This would be an empty zpci address while parsing XML so that we might pick a zpci address with non-zero fid. For example: dev1: fid=0 (this address would be treated as an unassigned zpci address) dev2 (no definition for both fid and uid) Then the process of assigning addresses will iterate devices as type by type. If dev2 is processed before dev1, dev2's fid would be assigned by 0. This causes dev1's fid=1. The result doesn't match what the user wants.
with how PCI addresses are treated so it shouldn't be a problem: if anything, *deviating* from this behavior would cause confusion.

On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
在 2018/8/21 下午7:00, Andrea Bolognani 写道:
On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
I want to ask a question. For pci address, any pci device can't use slot 0. Is that a reason why PCI part could treat all zeros as empty address?
A PCI address where all attributes are zero can't be used, so there's no ambiguity there; same for a zPCI address where all attributes are zero, which also can't be used.
Yes, uid must be non-zero value. But the user could only define fid like you mentioned below. Validation check happens during parsing xml. So if the user only defines fid=0, zpci address (fid=0, uid=0 although the user doesn't specify uid value) is going to be invalid as PCI address check logic.
In the context of PCI addresses, empty implies invalid but during XML parsing the address is only validated if it's not empty. If that was not the case, an empty address would never have a chance to be automatically filled in by libvirt :)
UID and FID must be unique separately, but we can't treat them as a combination. This doesn't like PCI address(domain:bus:slot:function).
I asked a question about this in reply to patch 09/12 yesterday by the way, and it would be great if you could answer it.
For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid.
If the user wants to use a specific zPCI address they can simply specify both attributes, eg. uid=1,fid=0 will work just fine with the proposed approach and won't produce errors or cause a new zPCI address to be automatically assigned.
If the user only specified fid=0 while leaving uid unspecified, well, an address is going to be picked for them.
This would be an empty zpci address while parsing XML so that we might pick a zpci address with non-zero fid. For example:
dev1: fid=0 (this address would be treated as an unassigned zpci address) dev2 (no definition for both fid and uid)
Then the process of assigning addresses will iterate devices as type by type. If dev2 is processed before dev1, dev2's fid would be assigned by 0. This causes dev1's fid=1. The result doesn't match what the user wants.
This would be the same as specifying a partial PCI address such as <address type='pci' slot='0x00'/> and getting an address with slot != 0x00 back: surprising, perhaps, but that's the way it's been with PCI addresses forever so you can assume users are familiar with it by now. For zPCI addresses to be inconsistent with this behavior would be utterly confusing; moreover, if the user really needs the uid and fid to have certain values, all they have to do is specify both and libvirt will not attempt to override them. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/21 下午9:19, Andrea Bolognani 写道:
在 2018/8/21 下午7:00, Andrea Bolognani 写道:
I want to ask a question. For pci address, any pci device can't use slot 0. Is that a reason why PCI part could treat all zeros as empty address? A PCI address where all attributes are zero can't be used, so
On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote: there's no ambiguity there; same for a zPCI address where all attributes are zero, which also can't be used. Yes, uid must be non-zero value. But the user could only define fid like you mentioned below. Validation check happens during parsing xml. So if the user only defines fid=0, zpci address (fid=0, uid=0 although the user doesn't specify uid value) is going to be invalid as PCI address check logic. In the context of PCI addresses, empty implies invalid but during XML parsing the address is only validated if it's not empty. If
On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote: that was not the case, an empty address would never have a chance to be automatically filled in by libvirt :)
UID and FID must be unique separately, but we can't treat them as a combination. This doesn't like PCI address(domain:bus:slot:function). I asked a question about this in reply to patch 09/12 yesterday by the way, and it would be great if you could answer it. I answered you in the morning.
For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid. If the user wants to use a specific zPCI address they can simply specify both attributes, eg. uid=1,fid=0 will work just fine with the proposed approach and won't produce errors or cause a new zPCI address to be automatically assigned.
If the user only specified fid=0 while leaving uid unspecified, well, an address is going to be picked for them. This would be an empty zpci address while parsing XML so that we might pick a zpci address with non-zero fid. For example:
dev1: fid=0 (this address would be treated as an unassigned zpci address) dev2 (no definition for both fid and uid)
Then the process of assigning addresses will iterate devices as type by type. If dev2 is processed before dev1, dev2's fid would be assigned by 0. This causes dev1's fid=1. The result doesn't match what the user wants. This would be the same as specifying a partial PCI address such as
<address type='pci' slot='0x00'/>
and getting an address with slot != 0x00 back: surprising, perhaps, but that's the way it's been with PCI addresses forever so you can assume users are familiar with it by now.
For zPCI addresses to be inconsistent with this behavior would be utterly confusing; moreover, if the user really needs the uid and fid to have certain values, all they have to do is specify both and libvirt will not attempt to override them.
I tried as your idea. It makes everything complicated, especially alloc/reserve/release zpci address. If the user defines uid=1 and fid=0, we don't know whether should reserve fid. (uid=1 fid=0) is the same with (uid=1).

On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
在 2018/8/21 下午9:19, Andrea Bolognani 写道:
On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid.
If the user wants to use a specific zPCI address they can simply specify both attributes, eg. uid=1,fid=0 will work just fine with the proposed approach and won't produce errors or cause a new zPCI address to be automatically assigned.
If the user only specified fid=0 while leaving uid unspecified, well, an address is going to be picked for them.
This would be an empty zpci address while parsing XML so that we might pick a zpci address with non-zero fid. For example:
dev1: fid=0 (this address would be treated as an unassigned zpci address) dev2 (no definition for both fid and uid)
Then the process of assigning addresses will iterate devices as type by type. If dev2 is processed before dev1, dev2's fid would be assigned by 0. This causes dev1's fid=1. The result doesn't match what the user wants.
This would be the same as specifying a partial PCI address such as
<address type='pci' slot='0x00'/>
and getting an address with slot != 0x00 back: surprising, perhaps, but that's the way it's been with PCI addresses forever so you can assume users are familiar with it by now.
For zPCI addresses to be inconsistent with this behavior would be utterly confusing; moreover, if the user really needs the uid and fid to have certain values, all they have to do is specify both and libvirt will not attempt to override them.
I tried as your idea. It makes everything complicated, especially alloc/reserve/release zpci address. If the user defines uid=1 and fid=0, we don't know whether should reserve fid. (uid=1 fid=0) is the same with (uid=1).
You should reserve it. The user can either * not specify zPCI information at all: automatic assignment will be performed, no error should be reported; * specify a *full* zPCI address: manual assignment, will result in either that exact address being used or an error; * specify partial information: missing properties will default to zero, which will probably cause errors eg. if only the uid is specified for a bunch of devices. The last option is less predictable so it should probably never be used. All of the above is consistent with how the PCI part works. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/22 下午6:09, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
在 2018/8/21 下午9:19, Andrea Bolognani 写道:
On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
For zPCI address, if we use the same strategy as PCI part and user wants to assign fid=0 to the specific device, he might encounter assignment failure. At least, according to the doc, 0 shoud be a valid value to assign to fid. If the user wants to use a specific zPCI address they can simply specify both attributes, eg. uid=1,fid=0 will work just fine with the proposed approach and won't produce errors or cause a new zPCI address to be automatically assigned.
If the user only specified fid=0 while leaving uid unspecified, well, an address is going to be picked for them. This would be an empty zpci address while parsing XML so that we might pick a zpci address with non-zero fid. For example:
dev1: fid=0 (this address would be treated as an unassigned zpci address) dev2 (no definition for both fid and uid)
Then the process of assigning addresses will iterate devices as type by type. If dev2 is processed before dev1, dev2's fid would be assigned by 0. This causes dev1's fid=1. The result doesn't match what the user wants. This would be the same as specifying a partial PCI address such as
<address type='pci' slot='0x00'/>
and getting an address with slot != 0x00 back: surprising, perhaps, but that's the way it's been with PCI addresses forever so you can assume users are familiar with it by now.
For zPCI addresses to be inconsistent with this behavior would be utterly confusing; moreover, if the user really needs the uid and fid to have certain values, all they have to do is specify both and libvirt will not attempt to override them. I tried as your idea. It makes everything complicated, especially alloc/reserve/release zpci address. If the user defines uid=1 and fid=0, we don't know whether should reserve fid. (uid=1 fid=0) is the same with (uid=1). You should reserve it. The user can either
* not specify zPCI information at all: automatic assignment will be performed, no error should be reported;
* specify a *full* zPCI address: manual assignment, will result in either that exact address being used or an error;
* specify partial information: missing properties will default to zero, which will probably cause errors eg. if only the uid is specified for a bunch of devices.
The last option is less predictable so it should probably never be used. All of the above is consistent with how the PCI part works.
And then, zpci address instance could be a member of pci address structure? I mean not using pointer at all?

On Thu, 2018-08-23 at 14:40 +0800, Yi Min Zhao wrote:
在 2018/8/22 下午6:09, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
I tried as your idea. It makes everything complicated, especially alloc/reserve/release zpci address. If the user defines uid=1 and fid=0, we don't know whether should reserve fid. (uid=1 fid=0) is the same with (uid=1).
You should reserve it. The user can either
* not specify zPCI information at all: automatic assignment will be performed, no error should be reported;
* specify a *full* zPCI address: manual assignment, will result in either that exact address being used or an error;
* specify partial information: missing properties will default to zero, which will probably cause errors eg. if only the uid is specified for a bunch of devices.
The last option is less predictable so it should probably never be used. All of the above is consistent with how the PCI part works.
And then, zpci address instance could be a member of pci address structure? I mean not using pointer at all?
Exactly. You can either just add zpci_uid and zpci_fid to the virPCIDeviceAddress struct, or have struct _virZPCIDeviceAddress { unsigned int uid; unsigned int fid; }; struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ virZPCIDeviceAddress zpci; }; that is, have a separate struct for zPCI but *embed it* inside the existing one instead of allocating it separately, the same way we embed virPCIDeviceAddress itself virDomainDeviceInfo, for example. This second option looks better to me. This got me thinking that the extension flags *also* belongs to virPCIDeviceAddress, since we need them to know whether the zPCI part should be taken into account when formatting and such: you used to check whether the zpci pointer was NULL, but of course you can no longer do that once the pointer is gone; moreover, checking for a flag is more explicit so that's also an improvement. Using the flags stored into virDomainDeviceInfo would be ugly because it would make it so virPCIDeviceAddress is no longer a stand-alone object, so we should avoid that. I'm not sure you can get away with not storing the extension flags in virDomainDeviceInfo, though, because IIRC you might need to use them *before* you have figured out that you're going to assign a PCI address to the device. We might just have to store them twice, which is not great but I think preferable to introducing a reverse dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I haven't really looked too closely into it, so perhaps there's a way to avoid that duplication after all :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/23 下午4:12, Andrea Bolognani 写道:
On Thu, 2018-08-23 at 14:40 +0800, Yi Min Zhao wrote:
在 2018/8/22 下午6:09, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
I tried as your idea. It makes everything complicated, especially alloc/reserve/release zpci address. If the user defines uid=1 and fid=0, we don't know whether should reserve fid. (uid=1 fid=0) is the same with (uid=1). You should reserve it. The user can either
* not specify zPCI information at all: automatic assignment will be performed, no error should be reported;
* specify a *full* zPCI address: manual assignment, will result in either that exact address being used or an error;
* specify partial information: missing properties will default to zero, which will probably cause errors eg. if only the uid is specified for a bunch of devices.
The last option is less predictable so it should probably never be used. All of the above is consistent with how the PCI part works. And then, zpci address instance could be a member of pci address structure? I mean not using pointer at all? Exactly. You can either just add zpci_uid and zpci_fid to the virPCIDeviceAddress struct, or have
struct _virZPCIDeviceAddress { unsigned int uid; unsigned int fid; };
struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ virZPCIDeviceAddress zpci; }; I have updated them as your recommendation in my new version. that is, have a separate struct for zPCI but *embed it* inside the existing one instead of allocating it separately, the same way we embed virPCIDeviceAddress itself virDomainDeviceInfo, for example. This second option looks better to me.
This got me thinking that the extension flags *also* belongs to virPCIDeviceAddress, since we need them to know whether the zPCI part should be taken into account when formatting and such: you used to check whether the zpci pointer was NULL, but of course you can no longer do that once the pointer is gone; moreover, checking for a flag is more explicit so that's also an improvement. Using the flags stored into virDomainDeviceInfo would be ugly because it would make it so virPCIDeviceAddress is no longer a stand-alone object, so we should avoid that.
I'm not sure you can get away with not storing the extension flags in virDomainDeviceInfo, though, because IIRC you might need to use them *before* you have figured out that you're going to assign a PCI address to the device. We might just have to store them twice, which is not great but I think preferable to introducing a reverse dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I haven't really looked too closely into it, so perhaps there's a way to avoid that duplication after all :)
I think it's enough to store extension flags in virDomainDeviceInfo in my new code.

On Thu, 2018-08-23 at 16:46 +0800, Yi Min Zhao wrote:
在 2018/8/23 下午4:12, Andrea Bolognani 写道:
This got me thinking that the extension flags *also* belongs to virPCIDeviceAddress, since we need them to know whether the zPCI part should be taken into account when formatting and such: you used to check whether the zpci pointer was NULL, but of course you can no longer do that once the pointer is gone; moreover, checking for a flag is more explicit so that's also an improvement. Using the flags stored into virDomainDeviceInfo would be ugly because it would make it so virPCIDeviceAddress is no longer a stand-alone object, so we should avoid that.
I'm not sure you can get away with not storing the extension flags in virDomainDeviceInfo, though, because IIRC you might need to use them *before* you have figured out that you're going to assign a PCI address to the device. We might just have to store them twice, which is not great but I think preferable to introducing a reverse dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I haven't really looked too closely into it, so perhaps there's a way to avoid that duplication after all :)
I think it's enough to store extension flags in virDomainDeviceInfo in my new code.
As explained above, that would cause virPCIDeviceAddress to no longer be usable on its own, which I don't think is a good idea. If you have to store the flags in virDomainDeviceInfo for address allocation purposes that's fine, but once address allocation has been performed you should no longer need to look at those and should use virPCIDeviceAddress on its own instead. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/23 下午4:12, Andrea Bolognani 写道:
Exactly. You can either just add zpci_uid and zpci_fid to the virPCIDeviceAddress struct, or have
struct _virZPCIDeviceAddress { unsigned int uid; unsigned int fid; };
struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ virZPCIDeviceAddress zpci; }; There's an error in syntax-check. I think it's from common code.
src/util/virpci.h:47: unsigned int uid; maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid I think it mistakes zpci's uid.

On Thu, 2018-08-23 at 17:15 +0800, Yi Min Zhao wrote:
在 2018/8/23 下午4:12, Andrea Bolognani 写道:
Exactly. You can either just add zpci_uid and zpci_fid to the virPCIDeviceAddress struct, or have
struct _virZPCIDeviceAddress { unsigned int uid; unsigned int fid; };
struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ virZPCIDeviceAddress zpci; };
There's an error in syntax-check. I think it's from common code.
src/util/virpci.h:47: unsigned int uid; maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid
I think it mistakes zpci's uid.
Easy enough to fix: just apply the patch below, change the struct definition to struct _virZPCIDeviceAddress { unsigned int uid; /* exempt from syntax-check */ unsigned int fid; }; and it will go away :) diff --git a/cfg.mk b/cfg.mk index 609ae869c2..1116feb299 100644 --- a/cfg.mk +++ b/cfg.mk @@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name: # Insist on correct types for [pug]id. sc_correct_id_types: @prohibit='\<(int|long) *[pug]id\>' \ + exclude='exempt from syntax-check' \ halt='use pid_t for pid, uid_t for uid, gid_t for gid' \ $(_sc_search_regexp) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/23 下午6:01, Andrea Bolognani 写道:
On Thu, 2018-08-23 at 17:15 +0800, Yi Min Zhao wrote:
在 2018/8/23 下午4:12, Andrea Bolognani 写道:
Exactly. You can either just add zpci_uid and zpci_fid to the virPCIDeviceAddress struct, or have
struct _virZPCIDeviceAddress { unsigned int uid; unsigned int fid; };
struct _virPCIDeviceAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; int multi; /* virTristateSwitch */ virZPCIDeviceAddress zpci; }; There's an error in syntax-check. I think it's from common code.
src/util/virpci.h:47: unsigned int uid; maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid
I think it mistakes zpci's uid. Easy enough to fix: just apply the patch below, change the struct definition to
struct _virZPCIDeviceAddress { unsigned int uid; /* exempt from syntax-check */ unsigned int fid; };
and it will go away :)
diff --git a/cfg.mk b/cfg.mk index 609ae869c2..1116feb299 100644 --- a/cfg.mk +++ b/cfg.mk @@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name: # Insist on correct types for [pug]id. sc_correct_id_types: @prohibit='\<(int|long) *[pug]id\>' \ + exclude='exempt from syntax-check' \ halt='use pid_t for pid, uid_t for uid, gid_t for gid' \ $(_sc_search_regexp)
Thanks for your help! I will look into this. Never meet this before.

在 2018/8/16 下午10:38, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; +struct _virZPCIDeviceAddress { + unsigned int zpci_fid; + unsigned int zpci_uid; + bool fid_assigned; + bool uid_assigned; +}; A couple of questions about the approach here, one of which I have mentioned already and one of which I probably haven't (my bad):
* do you really need to have separate booleans tracking whether or not either id has been assigned? Wouldn't the same approach as virPCIDeviceAddressIsEmpty() work, eg. consider the address absent if both are zero and present otherwise?
* especially if you don't need the additional booleans, would it be preferable to just add the two ids to the existing struct instead of declaring a new one that you'll have to allocate and make sure it's not NULL before accessing it? Again, I seem to remember Laine feeling somewhat strongly about the topic.
Cosmetic issues: Sorry, forgot this comment.
* uid should be listed before fid; Sure.
* the zpci_ prefix is unnecessary if you have a separate struct that contains "ZPCI" in the name. But see above :)
Let's see your response to another mail.

Let's introduce zPCI capability. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + 8 files changed, 9 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0fb800589a..aad6ef9461 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -507,6 +507,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 315 */ "vfio-pci.display", + "zpci", ); @@ -1148,6 +1149,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "vhost-vsock-device", QEMU_CAPS_DEVICE_VHOST_VSOCK }, { "mch", QEMU_CAPS_DEVICE_MCH }, { "sev-guest", QEMU_CAPS_SEV_GUEST }, + { "zpci", QEMU_CAPS_DEVICE_ZPCI }, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 9e8ad5f5c3..d2cb215c3b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -491,6 +491,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 315 */ QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */ + QEMU_CAPS_DEVICE_ZPCI, /* -device zpci */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml index 7347f5683f..b46efe880d 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -120,6 +120,7 @@ <flag name='blockdev-del'/> <flag name='vhost-vsock'/> <flag name='egl-headless'/> + <flag name='zpci'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>307899</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index b359f9a049..3ac1fc941c 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -127,6 +127,7 @@ <flag name='vhost-vsock'/> <flag name='tpm-emulator'/> <flag name='egl-headless'/> + <flag name='zpci'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>346751</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 7121da27a0..184f115fe4 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -135,6 +135,7 @@ <flag name='tpm-emulator'/> <flag name='egl-headless'/> <flag name='vfio-pci.display'/> + <flag name='zpci'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>375999</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml index 9ed25178f8..b04a9fbfd5 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml @@ -107,6 +107,7 @@ <flag name='nbd-tls'/> <flag name='virtual-css-bridge'/> <flag name='sdl-gl'/> + <flag name='zpci'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>220792</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml index 646239ff25..b2b267be8d 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -110,6 +110,7 @@ <flag name='virtual-css-bridge'/> <flag name='sdl-gl'/> <flag name='vhost-vsock'/> + <flag name='zpci'/> <version>2007093</version> <kvmVersion>0</kvmVersion> <microcodeVersion>246206</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index 09d68e1f18..f908ab88f3 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -114,6 +114,7 @@ <flag name='sdl-gl'/> <flag name='blockdev-del'/> <flag name='vhost-vsock'/> + <flag name='zpci'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>269625</microcodeVersion> -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
Let's introduce zPCI capability.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + 8 files changed, 9 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午10:39, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
Let's introduce zPCI capability.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + 8 files changed, 9 insertions(+) Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks!

This patch introduces a new attribute PCI address extension flag to deal with the extension PCI attributes such as 'uid' and 'fid' on the S390 platform. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/device_conf.h | 1 + src/conf/domain_addr.h | 5 ++ src/qemu/qemu_domain_address.c | 139 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 143 insertions(+), 2 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index a31ce9c376..6f926dff1d 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -164,6 +164,7 @@ struct _virDomainDeviceInfo { * assignment, never saved and never reported. */ int pciConnectFlags; /* enum virDomainPCIConnectFlags */ + int pciAddressExtFlags; /* enum virDomainPCIAddressExtensionFlags */ char *loadparm; /* PCI devices will only be automatically placed on a PCI bus diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5ad9d8ef3d..5219d2f208 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -29,6 +29,11 @@ # define VIR_PCI_ADDRESS_SLOT_LAST 31 # define VIR_PCI_ADDRESS_FUNCTION_LAST 7 +typedef enum { + VIR_PCI_ADDRESS_EXTENSION_NONE = 0, /* no extension */ + VIR_PCI_ADDRESS_EXTENSION_ZPCI = 1 << 0, /* zpci support */ +} virDomainPCIAddressExtensionFlags; + typedef enum { VIR_PCI_CONNECT_HOTPLUGGABLE = 1 << 0, /* is hotplug needed/supported */ diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 0d27e6ce9c..c582a531db 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -480,6 +480,62 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def, } +static bool +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { + case VIR_DOMAIN_DEVICE_CHR: + return false; + + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + break; + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: + virReportEnumRangeError(virDomainDeviceType, device->type); + return false; + } + return true; +} + + +static virDomainPCIAddressExtensionFlags +qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCapsPtr qemuCaps, + virDomainDeviceDefPtr dev) +{ + virDomainPCIAddressExtensionFlags extFlags = 0; + + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + qemuDomainDeviceSupportZPCI(dev)) { + extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI; + } + + return extFlags; +} + + /** * qemuDomainDeviceCalculatePCIConnectFlags: * @@ -962,6 +1018,55 @@ qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def, } +/** + * qemuDomainFillDevicePCIExtensionFlagsIter: + * + * @def: the entire DomainDef + * @dev: The device to be checked + * @info: virDomainDeviceInfo within the device + * @opaque: qemu capabilities + * + * Sets the pciAddressExtFlags for a single device's info. Has properly + * formatted arguments to be called by virDomainDeviceInfoIterate(). + * + * Always returns 0 - there is no failure. + */ +static int +qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virQEMUCapsPtr qemuCaps = opaque; + + info->pciAddressExtFlags + = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); + return 0; +} + + +/** + * qemuDomainFillAllPCIExtensionFlags: + * + * @def: the entire DomainDef + * @qemuCaps: as you'd expect + * + * Set the info->pciAddressExtFlags for all devices in the domain. + * + * Returns 0 on success or -1 on failure (the only possibility of + * failure would be some internal problem with + * virDomainDeviceInfoIterate()) + */ +static int +qemuDomainFillAllPCIExtensionFlags(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + return virDomainDeviceInfoIterate(def, + qemuDomainFillDevicePCIExtensionFlagsIter, + qemuCaps); +} + + /** * qemuDomainFindUnusedIsolationGroupIter: * @def: domain definition @@ -1236,6 +1341,29 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def, } +/** + * qemuDomainFillDevicePCIExtensionFlags: + * + * @dev: The device to be checked + * @qemuCaps: as you'd expect + * + * Set the info->pciAddressExtFlags for a single device. + * + * No return value. + */ +static void +qemuDomainFillDevicePCIExtensionFlags(virDomainDeviceDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev); + + if (info) { + info->pciAddressExtFlags + = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); + } +} + + static int qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, virDomainDeviceInfoPtr dev) @@ -2369,6 +2497,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainFillAllPCIConnectFlags(def, qemuCaps, driver) < 0) goto cleanup; + if (qemuDomainFillAllPCIExtensionFlags(def, qemuCaps) < 0) + goto cleanup; + if (qemuDomainSetupIsolationGroups(def) < 0) goto cleanup; @@ -2404,7 +2535,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, */ virDomainDeviceInfo info = { .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCI_DEVICE) + VIR_PCI_CONNECT_TYPE_PCI_DEVICE), + .pciAddressExtFlags = VIR_PCI_ADDRESS_EXTENSION_NONE }; bool buses_reserved = true; @@ -2441,7 +2573,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, qemuDomainHasPCIeRoot(def)) { virDomainDeviceInfo info = { .pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE | - VIR_PCI_CONNECT_TYPE_PCIE_DEVICE) + VIR_PCI_CONNECT_TYPE_PCIE_DEVICE), + .pciAddressExtFlags = VIR_PCI_ADDRESS_EXTENSION_NONE }; /* if there isn't an empty pcie-root-port, this will @@ -2958,6 +3091,8 @@ qemuDomainEnsurePCIAddress(virDomainObjPtr obj, qemuDomainFillDevicePCIConnectFlags(obj->def, dev, priv->qemuCaps, driver); + qemuDomainFillDevicePCIExtensionFlags(dev, priv->qemuCaps); + return virDomainPCIAddressEnsureAddr(priv->pciaddrs, info, info->pciConnectFlags); } -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { + case VIR_DOMAIN_DEVICE_CHR: + return false; + + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + break; + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST:
Missing 'default' case.
+ virReportEnumRangeError(virDomainDeviceType, device->type); + return false; + }
Add an empty line here.
+ return true; +}
[...]
+static int +qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virQEMUCapsPtr qemuCaps = opaque; + + info->pciAddressExtFlags + = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev);
Add an empty line here.
+ return 0; +}
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

Andrea Bolognani <abologna@redhat.com> [2018-08-16, 04:44PM +0200]:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { + case VIR_DOMAIN_DEVICE_CHR: + return false; + + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + break; + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST:
Missing 'default' case.
I thought we explicitly don't want a default case so that the compiler can catch this is another enum entry is added?

On Fri, 2018-08-17 at 06:41 +0200, Bjoern Walk wrote:
Andrea Bolognani <abologna@redhat.com> [2018-08-16, 04:44PM +0200]:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+ case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST:
Missing 'default' case.
I thought we explicitly don't want a default case so that the compiler can catch this is another enum entry is added?
The compiler can still catch that even when the 'default' case is present, and having it will catch cases when a random number has been assigned to the variable. We added 'default' cases to all switch statements a while back. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午10:44, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { + case VIR_DOMAIN_DEVICE_CHR: + return false; + + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_DISK: + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_VSOCK: + break; + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: Missing 'default' case.
+ virReportEnumRangeError(virDomainDeviceType, device->type); + return false; + } Add an empty line here.
+ return true; +} [...] +static int +qemuDomainFillDevicePCIExtensionFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virQEMUCapsPtr qemuCaps = opaque; + + info->pciAddressExtFlags + = qemuDomainDeviceCalculatePCIAddressExtensionFlags(qemuCaps, dev); Add an empty line here.
+ return 0; +} Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks!

QEMU on s390 supports PCI multibus since forever. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index aad6ef9461..30acc8e5e5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1767,6 +1767,10 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps, return false; } + /* S390 supports PCI-multibus. */ + if (ARCH_IS_S390(def->os.arch)) + return true; + /* If 'virt' supports PCI, it supports multibus. * No extra conditions here for simplicity. */ -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
QEMU on s390 supports PCI multibus since forever.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午10:45, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
QEMU on s390 supports PCI multibus since forever.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+) Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks!

The pci-root depends on zpci capability. So autogenerate pci-root if zpci exists. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de056272e8..a84e5f06b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; + addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break; case VIR_ARCH_SPARC: -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
The pci-root depends on zpci capability. So autogenerate pci-root if zpci exists.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de056272e8..a84e5f06b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; + addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break;
I wonder if you might want to do this, and other stuff, only if qemuDomainIsS390CCW()... It seems like that function is not used a lot, which might mean that someone is going to have to perform some serious cleaning up if there is ever another machine type on s390, or perhaps that it just doesn't quite apply here. If the latter, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

On Thu, 16 Aug 2018 16:52:18 +0200 Andrea Bolognani <abologna@redhat.com> wrote:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
The pci-root depends on zpci capability. So autogenerate pci-root if zpci exists.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de056272e8..a84e5f06b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; + addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break;
I wonder if you might want to do this, and other stuff, only if qemuDomainIsS390CCW()... It seems like that function is not used a lot, which might mean that someone is going to have to perform some serious cleaning up if there is ever another machine type on s390, or perhaps that it just doesn't quite apply here.
Not a libvirt person, but that other QEMU s390 machine type is long gone (and was never supported in libvirt AFAIK anyway), and I don't see any reason to add an additional s390 machine type to QEMU from where we stand now.
If the latter,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>

On Thu, 2018-08-16 at 17:01 +0200, Cornelia Huck wrote:
On Thu, 16 Aug 2018 16:52:18 +0200 Andrea Bolognani <abologna@redhat.com> wrote:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
@@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; + addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break;
I wonder if you might want to do this, and other stuff, only if qemuDomainIsS390CCW()... It seems like that function is not used a lot, which might mean that someone is going to have to perform some serious cleaning up if there is ever another machine type on s390, or perhaps that it just doesn't quite apply here.
Not a libvirt person, but that other QEMU s390 machine type is long gone (and was never supported in libvirt AFAIK anyway), and I don't see any reason to add an additional s390 machine type to QEMU from where we stand now.
New machine types happen when you least expect them :) But given that the current code would need an overhaul anyway if a new machine type was introduced, I'm okay with merging this as-is. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午10:52, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
The pci-root depends on zpci capability. So autogenerate pci-root if zpci exists.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index de056272e8..a84e5f06b2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3227,6 +3227,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def, case VIR_ARCH_S390X: addDefaultUSB = false; addPanicDevice = true; + addPCIRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI); break; I wonder if you might want to do this, and other stuff, only if qemuDomainIsS390CCW()... It seems like that function is not used a lot, which might mean that someone is going to have to perform some serious cleaning up if there is ever another machine type on s390, or perhaps that it just doesn't quite apply here.
If the latter,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks!

This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 9 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 ++++ 4 files changed, 102 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 16f7ffa928..8d3e75f19a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -27,11 +27,23 @@ #include "virlog.h" #include "virstring.h" #include "domain_addr.h" +#include "virhashcode.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN VIR_LOG_INIT("conf.domain_addr"); +static void +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) +{ + if (!addrs || !addrs->zpciIds) + return; + + virHashFree(addrs->zpciIds->uids); + virHashFree(addrs->zpciIds->fids); + VIR_FREE(addrs->zpciIds); +} + virDomainPCIConnectFlags virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) { @@ -741,6 +753,78 @@ virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, addrs->buses[addr->bus].slot[addr->slot].functions &= ~(1 << addr->function); } + +static uint32_t +virZPCIAddrCode(const void *name, + uint32_t seed) +{ + unsigned int value = *((unsigned int *)name); + return virHashCodeGen(&value, sizeof(value), seed); +} + + +static bool +virZPCIAddrEqual(const void *namea, + const void *nameb) +{ + return *((unsigned int *)namea) == *((unsigned int *)nameb); +} + + +static void * +virZPCIAddrCopy(const void *name) +{ + unsigned int *copy; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + *copy = *((unsigned int *)name); + return (void *)copy; +} + + +static void +virZPCIAddrKeyFree(void *name) +{ + VIR_FREE(name); +} + + +int +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + if (addrs->zpciIds) + return 0; + + if (VIR_ALLOC(addrs->zpciIds) < 0) + return -1; + + if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL, + virZPCIAddrCode, + virZPCIAddrEqual, + virZPCIAddrCopy, + virZPCIAddrKeyFree))) + goto error; + + if (!(addrs->zpciIds->fids = virHashCreateFull(10, NULL, + virZPCIAddrCode, + virZPCIAddrEqual, + virZPCIAddrCopy, + virZPCIAddrKeyFree))) + goto error; + } + + return 0; + + error: + virDomainPCIAddressSetExtensionFree(addrs); + return -1; +} + + virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses) { @@ -767,6 +851,7 @@ virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs) if (!addrs) return; + virDomainPCIAddressSetExtensionFree(addrs); VIR_FREE(addrs->buses); VIR_FREE(addrs); } diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5219d2f208..d1ec869da4 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -116,6 +116,11 @@ typedef struct { } virDomainPCIAddressBus; typedef virDomainPCIAddressBus *virDomainPCIAddressBusPtr; +typedef struct { + virHashTablePtr uids, fids; +} virDomainZPCIAddressIds; +typedef virDomainZPCIAddressIds *virDomainZPCIAddressIdsPtr; + struct _virDomainPCIAddressSet { virDomainPCIAddressBus *buses; size_t nbuses; @@ -125,6 +130,7 @@ struct _virDomainPCIAddressSet { bool areMultipleRootsSupported; /* If true, the guest can use the pcie-to-pci-bridge controller */ bool isPCIeToPCIBridgeSupported; + virDomainZPCIAddressIdsPtr zpciIds; }; typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; @@ -132,6 +138,9 @@ typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1); +int virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, + virDomainPCIAddressExtensionFlags extFlags); + virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses); void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 70dfcc5e29..d57a836c2e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -116,6 +116,7 @@ virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; virDomainPCIAddressSetAllMulti; virDomainPCIAddressSetAlloc; +virDomainPCIAddressSetExtensionAlloc; virDomainPCIAddressSetFree; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index c582a531db..200d96a790 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1482,6 +1482,13 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, addrs->dryRun = dryRun; + /* create zpci address set for s390 domain */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) && + virDomainPCIAddressSetExtensionAlloc(addrs, + VIR_PCI_ADDRESS_EXTENSION_ZPCI)) { + goto error; + } + /* pSeries domains support multiple pci-root controllers */ if (qemuDomainIsPSeries(def)) addrs->areMultipleRootsSupported = true; -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 9 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 ++++ 4 files changed, 102 insertions(+)
I haven't looked into the hash table handling in detail but I wonder if it's really necessary? IIUC you're using it just to mark which uids and fids have been already used by a device, which the PCI address allocation code does by setting bits inside integer variables - having a custom hash table for the same seems like overkill, and from the maintenance point of view it would be great to have the logic for PCI address and zPCI address allocation be similar unless diverging is strictly necessary. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午11:03, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
This patch provides a caching mechanism for the device address extensions uid and fid on S390. For efficient sparse address allocation, we introduce two hash tables for uid/fid which hold the address set information per domain. Also in order to improve performance of searching available value, we introduce our own callbacks for the two hashtables. In this way, uid/fid is saved in hash key and hash value could be any non-NULL pointer due to no operation on hash value. That is also the reason why we don't introduce hash value free callback.
Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_addr.c | 85 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 9 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 7 ++++ 4 files changed, 102 insertions(+) I haven't looked into the hash table handling in detail but I wonder if it's really necessary? IIUC you're using it just to mark which uids and fids have been already used by a device, which the PCI address allocation code does by setting bits inside integer variables - having a custom hash table for the same seems like overkill, and from the maintenance point of view it would be great to have the logic for PCI address and zPCI address allocation be similar unless diverging is strictly necessary.
PCI address set uses array to store pci addresses' assignment. It doesn't need too much memory because the buses are allocated dynamically, one bus only has 32 slot, and it's a tree topology. But in zpci case, fid and uid are flat. FID is 32-bit so that we need a 4294967295 sized array. Don't you think it's too large?

On Mon, 2018-08-20 at 16:32 +0800, Yi Min Zhao wrote:
在 2018/8/16 下午11:03, Andrea Bolognani 写道:
I haven't looked into the hash table handling in detail but I wonder if it's really necessary? IIUC you're using it just to mark which uids and fids have been already used by a device, which the PCI address allocation code does by setting bits inside integer variables - having a custom hash table for the same seems like overkill, and from the maintenance point of view it would be great to have the logic for PCI address and zPCI address allocation be similar unless diverging is strictly necessary.
PCI address set uses array to store pci addresses' assignment. It doesn't need too much memory because the buses are allocated dynamically, one bus only has 32 slot, and it's a tree topology. But in zpci case, fid and uid are flat. FID is 32-bit so that we need a 4294967295 sized array. Don't you think it's too large?
Welp, I guess you're right. Disregard this comment then. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/20 下午6:48, Andrea Bolognani 写道:
On Mon, 2018-08-20 at 16:32 +0800, Yi Min Zhao wrote:
在 2018/8/16 下午11:03, Andrea Bolognani 写道:
I haven't looked into the hash table handling in detail but I wonder if it's really necessary? IIUC you're using it just to mark which uids and fids have been already used by a device, which the PCI address allocation code does by setting bits inside integer variables - having a custom hash table for the same seems like overkill, and from the maintenance point of view it would be great to have the logic for PCI address and zPCI address allocation be similar unless diverging is strictly necessary. PCI address set uses array to store pci addresses' assignment. It doesn't need too much memory because the buses are allocated dynamically, one bus only has 32 slot, and it's a tree topology. But in zpci case, fid and uid are flat. FID is 32-bit so that we need a 4294967295 sized array. Don't you think it's too large? Welp, I guess you're right. Disregard this comment then.
Thanks for your comment anyway!

This patch introduces new XML parser/formatter functions. Uid is 16-bit and non-zero. Fid is 32-bit. They are added as two new attributes of PCI address, and parsed/formatted along with PCI address parser/formatter. The related test is also added. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- docs/schemas/basictypes.rng | 23 +++++++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 78 ++++++++++++++++++++++ src/conf/domain_addr.c | 3 + src/conf/domain_conf.c | 6 ++ src/util/virpci.h | 3 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 26 ++++++++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 +++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 24 +++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++++++ tests/qemuxml2argvtest.c | 4 ++ tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 ++++++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 +++++++++ tests/qemuxml2xmltest.c | 3 + 14 files changed, 266 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 1a18cd31b1..4e3365c211 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -65,6 +65,17 @@ </data> </choice> </define> + <define name="uint32"> + <choice> + <data type="string"> + <param name="pattern">(0x)?[0-9a-fA-F]{1,8}</param> + </data> + <data type="unsignedInt"> + <param name="minInclusive">0</param> + <param name="maxInclusive">4294967295</param> + </data> + </choice> + </define> <define name="UUID"> <choice> @@ -111,6 +122,18 @@ </attribute> </optional> </define> + <define name="zpciaddress"> + <optional> + <attribute name="uid"> + <ref name="uint16"/> + </attribute> + </optional> + <optional> + <attribute name="fid"> + <ref name="uint32"/> + </attribute> + </optional> + </define> <!-- a 6 byte MAC address in ASCII-hex format, eg "12:34:56:78:9A:BC" --> <!-- The lowest bit of the 1st byte is the "multicast" bit. a --> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ac04af51a1..e4d908f4ab 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5180,6 +5180,7 @@ <value>pci</value> </attribute> <ref name="pciaddress"/> + <ref name="zpciaddress"/> </group> <group> <attribute name="type"> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index d69f94fadf..4d09b8219a 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -32,6 +32,79 @@ #define VIR_FROM_THIS VIR_FROM_DEVICE +static int +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) +{ + if (zpci->uid_assigned && + (zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || + zpci->zpci_uid == 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%x', " + "must be > 0x0 and <= 0x%x"), + zpci->zpci_uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); + return 0; + } + + if (zpci->fid_assigned) { + /* We don't need to check fid because fid covers + * all range of uint32 type. + */ + return 1; + } + + return 1; +} + +static int +virZPCIDeviceAddressParseXML(xmlNodePtr node, + virPCIDeviceAddressPtr addr) +{ + char *uid, *fid; + int ret = -1; + virZPCIDeviceAddressPtr def = NULL; + + if (VIR_ALLOC(def) < 0) + return -1; + + uid = virXMLPropString(node, "uid"); + fid = virXMLPropString(node, "fid"); + + if (uid) { + if (virStrToLong_uip(uid, NULL, 0, &def->zpci_uid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'uid' attribute")); + goto cleanup; + } + def->uid_assigned = true; + } + + if (fid) { + if (virStrToLong_uip(fid, NULL, 0, &def->zpci_fid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'fid' attribute")); + goto cleanup; + } + def->fid_assigned = true; + } + + if (uid || fid) { + if (!virZPCIDeviceAddressIsValid(def)) + goto cleanup; + + addr->zpci = def; + def = NULL; + } + + ret = 0; + + cleanup: + VIR_FREE(uid); + VIR_FREE(fid); + VIR_FREE(def); + return ret; +} + int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src) @@ -57,6 +130,8 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) { VIR_FREE(info->alias); + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + VIR_FREE(info->addr.pci.zpci); memset(&info->addr, 0, sizeof(info->addr)); info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; VIR_FREE(info->romfile); @@ -245,6 +320,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, if (!virPCIDeviceAddressIsEmpty(addr) && !virPCIDeviceAddressIsValid(addr, true)) goto cleanup; + if (virZPCIDeviceAddressParseXML(node, addr) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8d3e75f19a..90d8abc1f4 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1044,6 +1044,9 @@ virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, dev->isolationGroup, function) < 0) return -1; + if (dev->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) + addr.zpci = dev->addr.pci.zpci; + if (virDomainPCIAddressReserveAddrInternal(addrs, &addr, flags, dev->isolationGroup, false) < 0) return -1; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7dcbe8a20b..309e63e04d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6380,6 +6380,12 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.pci.slot, info->addr.pci.function); } + if (info->addr.pci.zpci) { + virBufferAsprintf(buf, " uid='0x%.4x'", + info->addr.pci.zpci->zpci_uid); + virBufferAsprintf(buf, " fid='0x%.8x'", + info->addr.pci.zpci->zpci_fid); + } if (info->addr.pci.multi) { virBufferAsprintf(buf, " multifunction='%s'", virTristateSwitchTypeToString(info->addr.pci.multi)); diff --git a/src/util/virpci.h b/src/util/virpci.h index b137eb01e6..902bc53017 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr; +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX + typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress; typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr; struct _virZPCIDeviceAddress { diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args new file mode 100644 index 0000000000..20e63a15b5 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000 diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml new file mode 100644 index 0000000000..7887b97b2c --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </disk> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args new file mode 100644 index 0000000000..622c504da0 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x8 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml new file mode 100644 index 0000000000..cde333e220 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 84117a3e63..2bfc3ed215 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1010,6 +1010,8 @@ mymain(void) QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); + DO_TEST("disk-virtio-s390-zpci", QEMU_CAPS_DEVICE_ZPCI, + QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); DO_TEST("disk-order", QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_BLK_SCSI); DO_TEST("disk-virtio-queues", @@ -1591,6 +1593,8 @@ mymain(void) DO_TEST_PARSE_ERROR("hostdev-mdev-display-missing-graphics", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_VFIO_PCI_DISPLAY); + DO_TEST("hostdev-vfio-zpci", + QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); diff --git a/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml new file mode 100644 index 0000000000..39b5acdf3b --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </disk> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml new file mode 100644 index 0000000000..1f48c44e30 --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0019' fid='0x0000001f'/> + </hostdev> + <memballoon model='virtio'> + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c6cb2dda0c..6e41fd9bff 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -379,6 +379,8 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-ioeventfd", QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-s390-zpci", + QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW); DO_TEST("disk-scsi-megasas", QEMU_CAPS_SCSI_MEGASAS); DO_TEST("disk-scsi-mptsas1068", @@ -460,6 +462,7 @@ mymain(void) DO_TEST("hostdev-usb-address", NONE); DO_TEST("hostdev-pci-address", NONE); DO_TEST("hostdev-vfio", NONE); + DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW); DO_TEST("hostdev-mdev-precreated", NONE); DO_TEST("hostdev-mdev-display", QEMU_CAPS_VFIO_PCI_DISPLAY); DO_TEST("pci-rom", NONE); -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+static int +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) +{ + if (zpci->uid_assigned && + (zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || + zpci->zpci_uid == 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%x', " + "must be > 0x0 and <= 0x%x"), + zpci->zpci_uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); + return 0; + } + + if (zpci->fid_assigned) { + /* We don't need to check fid because fid covers + * all range of uint32 type. + */ + return 1; + }
This branch is pointless, just drop it (but leave the comment). [...]
@@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr;
+# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX
A single space between the name and the value will do. This should be DO_TEST("disk-virtio-s390-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); Same later. The rest looks good. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/16 下午11:14, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+static int +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci) +{ + if (zpci->uid_assigned && + (zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID || + zpci->zpci_uid == 0)) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address uid='0x%x', " + "must be > 0x0 and <= 0x%x"), + zpci->zpci_uid, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID); + return 0; + } + + if (zpci->fid_assigned) { + /* We don't need to check fid because fid covers + * all range of uint32 type. + */ + return 1; + } This branch is pointless, just drop it (but leave the comment).
[...]
@@ -37,6 +37,9 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr;
+# define VIR_DOMAIN_DEVICE_ZPCI_MAX_UID UINT16_MAX +# define VIR_DOMAIN_DEVICE_ZPCI_MAX_FID UINT32_MAX A single space between the name and the value will do.
This should be
DO_TEST("disk-virtio-s390-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
Same later.
The rest looks good.
OK.

This patch adds new functions for reservation, assignment and release to handle the uid/fid. If the uid/fid is defined in the domain XML, they will be reserved directly in collecting phase. If any of them is not defined, we will find out an available value for it from zPCI address hashtable, and reserve it. For hotplug case, there might be or not zPCI definition. So allocate and reserve uid/fid for undefined case. Assign if needed and reserve uid/fid for defined case. If the user define zPCI extension address but zPCI capability doesn't exist, an error will be reported. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/conf/device_conf.h | 7 + src/conf/domain_addr.c | 291 +++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 15 +++ src/libvirt_private.syms | 3 + src/qemu/qemu_domain_address.c | 58 +++++++- 5 files changed, 373 insertions(+), 1 deletion(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 6f926dff1d..40838a9401 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -211,6 +211,13 @@ virDeviceInfoPCIAddressPresent(const virDomainDeviceInfo *info) !virPCIDeviceAddressIsEmpty(&info->addr.pci); } +static inline bool +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->addr.pci.zpci; +} + int virPCIDeviceAddressParseXML(xmlNodePtr node, virPCIDeviceAddressPtr addr); diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 90d8abc1f4..cf1fc467f2 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -33,6 +33,170 @@ VIR_LOG_INIT("conf.domain_addr"); +static int +virDomainZPCIAddressReserveId(virHashTablePtr set, + unsigned int id, + const char *name) +{ + if (virHashLookup(set, &id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zPCI %s %u is already reserved"), + name, id); + return -1; + } + + if (virHashAddEntry(set, &id, (void*)1) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to reserve %s %u"), + name, id); + return -1; + } + + return 0; +} + + +static int +virDomainZPCIAddressReserveUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + return virDomainZPCIAddressReserveId(set, addr->zpci_uid, "uid"); +} + + +static int +virDomainZPCIAddressReserveFid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + return virDomainZPCIAddressReserveId(set, addr->zpci_fid, "fid"); +} + + +static bool +virDomainZPCIAddressAssignId(virHashTablePtr set, + unsigned int *id, + unsigned int min, + unsigned int max, + const char *name) +{ + while (virHashLookup(set, &min)) { + if (min == max) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("There is no more free %s."), + name); + return false; + } + ++min; + } + *id = min; + + return true; +} + + +static int +virDomainZPCIAddressAssignUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (addr->uid_assigned) + return 0; + + addr->uid_assigned = + virDomainZPCIAddressAssignId(set, &addr->zpci_uid, 1, + VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid"); + return addr->uid_assigned ? 0 : -1; +} + + +static int +virDomainZPCIAddressAssignFid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (addr->fid_assigned) + return 0; + + addr->fid_assigned = + virDomainZPCIAddressAssignId(set, &addr->zpci_fid, 0, + VIR_DOMAIN_DEVICE_ZPCI_MAX_FID, "fid"); + return addr->fid_assigned ? 0 : -1; +} + + +static void +virDomainZPCIAddressReleaseUid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->uid_assigned) + return; + + if (virHashRemoveEntry(set, &addr->zpci_uid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release uid %u failed"), addr->zpci_uid); + } + + addr->uid_assigned = false; +} + + +static void +virDomainZPCIAddressReleaseFid(virHashTablePtr set, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->fid_assigned) + return; + + if (virHashRemoveEntry(set, &addr->zpci_fid)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Release fid %u failed"), addr->zpci_fid); + } + + addr->fid_assigned = false; +} + + +static void +virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds, + virPCIDeviceAddressPtr addr) +{ + if (!zpciIds || !addr->zpci) + return; + + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr->zpci); + + virDomainZPCIAddressReleaseFid(zpciIds->fids, addr->zpci); + + VIR_FREE(addr->zpci); +} + + +static int +virDomainZPCIAddressReserveNextUid(virHashTablePtr uids, + virZPCIDeviceAddressPtr zpci) +{ + if (virDomainZPCIAddressAssignUid(uids, zpci) < 0) + return -1; + + if (virDomainZPCIAddressReserveUid(uids, zpci) < 0) + return -1; + + return 0; +} + + +static int +virDomainZPCIAddressReserveNextFid(virHashTablePtr fids, + virZPCIDeviceAddressPtr zpci) +{ + if (virDomainZPCIAddressAssignFid(fids, zpci) < 0) + return -1; + + if (virDomainZPCIAddressReserveFid(fids, zpci) < 0) + return -1; + + return 0; +} + + static void virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) { @@ -44,6 +208,120 @@ virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs) VIR_FREE(addrs->zpciIds); } + +static int +virDomainZPCIAddressReserveAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!addr) + return 0; + + if (addr->uid_assigned && + virDomainZPCIAddressReserveUid(zpciIds->uids, addr)) { + return -1; + } + + if (addr->fid_assigned && + virDomainZPCIAddressReserveFid(zpciIds->fids, addr)) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + } + + return 0; +} + + +static int +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->uid_assigned && + virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) { + return -1; + } + + if (!addr->fid_assigned && + virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + } + + return 0; +} + + +int +virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + /* Reserve uid/fid to ZPCI device which has defined uid/fid + * in the domain. + */ + if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, addr->zpci) < 0) + return -1; + } + + return 0; +} + + +int +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr dev, + virDomainPCIAddressExtensionFlags extFlags) +{ + if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + /* Assign and reserve uid/fid to ZPCI device which + * has not defined uid/fid in the domain. + */ + virZPCIDeviceAddress zpci = { 0 }; + + if (dev->zpci) + zpci = *dev->zpci; + + if (virDomainZPCIAddressReserveNextAddr(addrs->zpciIds, &zpci) < 0) + return -1; + + if (!addrs->dryRun) { + if (!dev->zpci && VIR_ALLOC(dev->zpci) < 0) + return -1; + *dev->zpci = zpci; + } + } + + return 0; +} + +static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{ + virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci; + + if (zpci && !dev->pciAddressExtFlags) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported.")); + return -1; + } + + if (!zpci) { + return virDomainPCIAddressExtensionReserveNextAddr(addrs, &dev->addr.pci, + dev->pciAddressExtFlags); + } else { + if (virDomainZPCIAddressAssignUid(addrs->zpciIds->uids, zpci)) + return -1; + + if (virDomainZPCIAddressAssignFid(addrs->zpciIds->fids, zpci)) + return -1; + + if (virDomainZPCIAddressReserveAddr(addrs->zpciIds, zpci)) + return -1; + } + + return 0; +} + virDomainPCIConnectFlags virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) { @@ -740,12 +1018,25 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, ret = virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1); } + if (virDomainPCIAddressExtensionEnsureAddr(addrs, dev) < 0) + goto cleanup; + cleanup: VIR_FREE(addrStr); return ret; } +void +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + int extFlags) +{ + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)) + virDomainZPCIAddressReleaseIds(addrs->zpciIds, addr); +} + + void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index d1ec869da4..a1f71f15f4 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -164,6 +164,16 @@ bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + virDomainPCIAddressExtensionFlags extFlags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + virDomainPCIAddressExtensionFlags extFlags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int virDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr, virDomainPCIConnectFlags flags, @@ -185,6 +195,11 @@ void virDomainPCIAddressReleaseAddr(virDomainPCIAddressSetPtr addrs, virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs, + virPCIDeviceAddressPtr addr, + int extFlags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + void virDomainPCIAddressSetAllMulti(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d57a836c2e..490fbf5739 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -111,6 +111,9 @@ virDomainPCIAddressAsString; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; +virDomainPCIAddressExtensionReleaseAddr; +virDomainPCIAddressExtensionReserveAddr; +virDomainPCIAddressExtensionReserveNextAddr; virDomainPCIAddressReleaseAddr; virDomainPCIAddressReserveAddr; virDomainPCIAddressReserveNextAddr; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 200d96a790..68897cde2c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1373,6 +1373,24 @@ qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs, } +static int +qemuDomainAssignPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainPCIAddressSetPtr addrs = opaque; + virPCIDeviceAddressPtr addr = &info->addr.pci; + virDomainPCIAddressExtensionFlags extFlags = info->pciAddressExtFlags; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDomainPCIAddressExtensionReserveNextAddr(addrs, addr, extFlags) < 0) + return -1; + } + + return 0; +} + static int qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceDefPtr device, @@ -1466,6 +1484,29 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, return ret; } +static int +qemuDomainCollectPCIAddressExtension(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainPCIAddressSetPtr addrs = opaque; + virPCIDeviceAddressPtr addr = &info->addr.pci; + + if (!virDeviceInfoPCIAddressExtensionPresent(info) || + ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) && + (device->data.hostdev->parent.type != VIR_DOMAIN_DEVICE_NONE))) { + /* If a hostdev has a parent, its info will be a part of the + * parent, and will have its address collected during the scan + * of the parent's device type. + */ + return 0; + } + + return virDomainPCIAddressExtensionReserveAddr(addrs, addr, + info->pciAddressExtFlags); +} + static virDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -1560,6 +1601,12 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, addrs) < 0) goto error; + if (virDomainDeviceInfoIterate(def, + qemuDomainCollectPCIAddressExtension, + addrs) < 0) { + goto error; + } + return addrs; error: @@ -2561,6 +2608,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; + if (virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) < 0) + goto cleanup; + /* Only for *new* domains with pcie-root (and no other * manually specified PCI controllers in the definition): If, * after assigning addresses/reserving slots for all devices, @@ -2655,6 +2705,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; + if (virDomainDeviceInfoIterate(def, qemuDomainAssignPCIAddressExtension, addrs) < 0) + goto cleanup; + /* set multi attribute for devices at function 0 of * any slot that has multiple functions in use */ @@ -3114,8 +3167,11 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, if (!devstr) devstr = info->alias; - if (virDeviceInfoPCIAddressPresent(info)) + if (virDeviceInfoPCIAddressPresent(info)) { virDomainPCIAddressReleaseAddr(priv->pciaddrs, &info->addr.pci); + virDomainPCIAddressExtensionReleaseAddr(priv->pciaddrs, &info->addr.pci, + info->pciAddressExtFlags); + } if (virDomainUSBAddressRelease(priv->usbaddrs, info) < 0) VIR_WARN("Unable to release USB address on %s", NULLSTR(devstr)); -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+static inline bool +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->addr.pci.zpci; +}
This should be called virDeviceInfoPCIAddressExtensionIsPresent() since it's a predicate. I know the corresponding PCI function gets the naming wrong, but that doesn't mean you should too :) In the same vein, I don't think this (or the PCI version, for that matter) need to be inline... I'd rather have them both as regular functions in the .c file. I'll probably cook up a patch cleaning up the PCI part later, see what the feedback is. [...]
+static int +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->uid_assigned && + virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) { + return -1; + } + + if (!addr->fid_assigned && + virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + }
Not sure I get the logic here... ReserveNextAddress() is supposed to pick the next available address and reserve it, but IIUC this will skip picking either id based on whether they were assigned. I don't think that's what we want. Also all functions that return an int should be called like if (virDomainZPCIAddressReserveNextUid() < 0) { ... } [...]
+static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{
It's weird that this function doesn't get extFlags as an argument, unlike the other ones you've introduced. Better make it consistent.
+ virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci; + + if (zpci && !dev->pciAddressExtFlags) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported.")); + return -1; + }
The error message is very generic here, it should at the very least make it clear that zPCI is not supported *for the specific device*. I'm not sure this is the best place to perform that kind of check, either. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/17 上午12:06, Andrea Bolognani 写道:
+static inline bool +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->addr.pci.zpci; +} This should be called virDeviceInfoPCIAddressExtensionIsPresent() since it's a predicate. I know the corresponding PCI function gets
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that matter) need to be inline... I'd rather have them both as regular functions in the .c file. I'll probably cook up a patch cleaning up the PCI part later, see what the feedback is.
Got it. Thank!
[...]
+static int +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->uid_assigned && + virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) { + return -1; + } + + if (!addr->fid_assigned && + virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + } Not sure I get the logic here... ReserveNextAddress() is supposed to pick the next available address and reserve it, but IIUC this will skip picking either id based on whether they were assigned.
If uid/fid is assigned, we call ***Reserve***(). If uid/fid is unassigned, we call ***ReserveNext***(). But I'm not very clear about your concern.
I don't think that's what we want.
Also all functions that return an int should be called like
if (virDomainZPCIAddressReserveNextUid() < 0) { ... }
OK.
[...]
+static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{ It's weird that this function doesn't get extFlags as an argument, unlike the other ones you've introduced. Better make it consistent.
We have to pass DeviceInfo which already has extFlags. If we pass extFlags again, isn't it redundant? This function is only called in one place for hotplug case.
+ virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci; + + if (zpci && !dev->pciAddressExtFlags) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported.")); + return -1; + } The error message is very generic here, it should at the very least make it clear that zPCI is not supported *for the specific device*.
I'm not sure this is the best place to perform that kind of check, either.
This is only called by virDomainPCIAddressEnsureAddr() for hotplug case. I thought this again. If we remove those two boolean members from zPCI address structure as what we discuss on the 1st patch, we could move this check to virDomainPCIAddressEnsureAddr() like what PCI addr check does.

On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
在 2018/8/17 上午12:06, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+static inline bool +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->addr.pci.zpci; +}
This should be called virDeviceInfoPCIAddressExtensionIsPresent() since it's a predicate. I know the corresponding PCI function gets the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that matter) need to be inline... I'd rather have them both as regular functions in the .c file. I'll probably cook up a patch cleaning up the PCI part later, see what the feedback is.
Got it. Thank!
The patches I said I'd write are now on the list: https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html No reviews yet, we'll see whether they get ACKed or NACKed :)
[...]
+static int +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->uid_assigned && + virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) { + return -1; + } + + if (!addr->fid_assigned && + virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + }
Not sure I get the logic here... ReserveNextAddress() is supposed to pick the next available address and reserve it, but IIUC this will skip picking either id based on whether they were assigned.
If uid/fid is assigned, we call ***Reserve***(). If uid/fid is unassigned, we call ***ReserveNext***().
But I'm not very clear about your concern.
That in ReserveNext() you're checking whether addr->*id_assigned when you know that ids have not been assigned or you wouldn't have called ReserveNext() in the first place... It seems unnecessary and confusing to me, so unless I've missed something that makes it necessary please just drop those checks.
[...]
+static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{
It's weird that this function doesn't get extFlags as an argument, unlike the other ones you've introduced. Better make it consistent.
We have to pass DeviceInfo which already has extFlags. If we pass extFlags again, isn't it redundant? This function is only called in one place for hotplug case.
I wanted the API to be more consistent but I realize now you have to pass either virPCIDeviceAddress or virDomainDeviceInfo depending on the context, so it doesn't really matter, you can leave it as it is. The signatures for the corresponding PCI functions are not entirely consistent either :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/22 下午5:56, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
在 2018/8/17 上午12:06, Andrea Bolognani 写道:
+static inline bool +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info) +{ + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->addr.pci.zpci; +} This should be called virDeviceInfoPCIAddressExtensionIsPresent() since it's a predicate. I know the corresponding PCI function gets
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] the naming wrong, but that doesn't mean you should too :)
In the same vein, I don't think this (or the PCI version, for that matter) need to be inline... I'd rather have them both as regular functions in the .c file. I'll probably cook up a patch cleaning up the PCI part later, see what the feedback is. Got it. Thank! The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :) I saw them. Could I give my ACKed?
[...]
+static int +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds, + virZPCIDeviceAddressPtr addr) +{ + if (!addr->uid_assigned && + virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) { + return -1; + } + + if (!addr->fid_assigned && + virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) { + virDomainZPCIAddressReleaseUid(zpciIds->uids, addr); + return -1; + } Not sure I get the logic here... ReserveNextAddress() is supposed to pick the next available address and reserve it, but IIUC this will skip picking either id based on whether they were assigned. If uid/fid is assigned, we call ***Reserve***(). If uid/fid is unassigned, we call ***ReserveNext***().
But I'm not very clear about your concern. That in ReserveNext() you're checking whether addr->*id_assigned when you know that ids have not been assigned or you wouldn't have called ReserveNext() in the first place... It seems unnecessary and confusing to me, so unless I've missed something that makes it necessary please just drop those checks. As our discussion on the 1st patch, boolean values are removed. So we don't need checks here.
[...]
+static int +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs, + virDomainDeviceInfoPtr dev) +{ It's weird that this function doesn't get extFlags as an argument, unlike the other ones you've introduced. Better make it consistent. We have to pass DeviceInfo which already has extFlags. If we pass extFlags again, isn't it redundant? This function is only called in one place for hotplug case. I wanted the API to be more consistent but I realize now you have to pass either virPCIDeviceAddress or virDomainDeviceInfo depending on the context, so it doesn't really matter, you can leave it as it is. The signatures for the corresponding PCI functions are not entirely consistent either :)
I have a question about your previous comment about error report. You thought we should report more specific information.
+ virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci; + + if (zpci && !dev->pciAddressExtFlags) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported.")); + return -1; + }
It's called by all device types which possibly use PCI address. I'm not sure how to report device's name in error string.

On Thu, 2018-08-23 at 18:01 +0800, Yi Min Zhao wrote:
在 2018/8/22 下午5:56, Andrea Bolognani 写道:
The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :)
I saw them. Could I give my ACKed?
Feel free to do that, but before pushing I'd like to have Laine's ACK since he's the one who introduced the functions in the first place.
I have a question about your previous comment about error report. You thought we should report more specific information.
+ virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci; + + if (zpci && !dev->pciAddressExtFlags) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported.")); + return -1; + }
It's called by all device types which possibly use PCI address. I'm not sure how to report device's name in error string.
I think reporting the address is enough. The idea is to give the user some information they can use to figure out which device is misconfigured rather than just telling them there is an issue and leaving it at that. I also think we might be at a point where enough big changes have been made that the best way forward is probably to post a v3 that includes those and then use that as the basis for more fine-tuning for minor stuff such as error messages and the like. Does that sound reasonable? -- Andrea Bolognani / Red Hat / Virtualization

On Thu, 2018-08-23 at 13:01 +0200, Andrea Bolognani wrote:
I also think we might be at a point where enough big changes have been made that the best way forward is probably to post a v3 that includes those and then use that as the basis for more fine-tuning for minor stuff such as error messages and the like. Does that sound reasonable?
Of course I meant to say v4, not v3, above :) -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/23 下午7:01, Andrea Bolognani 写道:
在 2018/8/22 下午5:56, Andrea Bolognani 写道:
The patches I said I'd write are now on the list:
https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
No reviews yet, we'll see whether they get ACKed or NACKed :) I saw them. Could I give my ACKed? Feel free to do that, but before pushing I'd like to have Laine's ACK since he's the one who introduced the functions in the first
On Thu, 2018-08-23 at 18:01 +0800, Yi Min Zhao wrote: place.
I have a question about your previous comment about error report. You thought we should report more specific information.
+ virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci; + + if (zpci && !dev->pciAddressExtFlags) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported.")); + return -1; + } It's called by all device types which possibly use PCI address. I'm not sure how to report device's name in error string. I think reporting the address is enough. The idea is to give the user some information they can use to figure out which device is misconfigured rather than just telling them there is an issue and leaving it at that.
I also think we might be at a point where enough big changes have been made that the best way forward is probably to post a v3 that includes those and then use that as the basis for more fine-tuning for minor stuff such as error messages and the like. Does that sound reasonable?
Yes, it makes sense. I'm preparing V4 and could post it today.

Add new functions to generate zPCI command string and append it to QEMU command line. And the related tests are added. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 114 +++++++++++++++++++++ src/qemu/qemu_command.h | 4 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 1 + .../hostdev-vfio-zpci-autogenerate.args | 26 +++++ .../hostdev-vfio-zpci-autogenerate.xml | 18 ++++ .../hostdev-vfio-zpci-boundaries.args | 30 ++++++ .../hostdev-vfio-zpci-boundaries.xml | 26 +++++ .../hostdev-vfio-zpci-multidomain-many.args | 40 ++++++++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 ++++++++++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 2 + tests/qemuxml2argvtest.c | 13 +++ .../hostdev-vfio-zpci-autogenerate.xml | 30 ++++++ .../hostdev-vfio-zpci-boundaries.xml | 42 ++++++++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 ++++++++++++++ tests/qemuxml2xmltest.c | 11 ++ 15 files changed, 503 insertions(+) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d148db90fa..e46c362646 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2151,6 +2151,69 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, return NULL; } +char * +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "zpci"); + virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid); + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid); + virBufferAsprintf(&buf, ",target=%s", dev->alias); + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid); + + if (virBufferCheckError(&buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +int +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info) +{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + return 1; + } + + if (info->addr.pci.zpci) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support zpci devices")); + return -1; + } + + return 0; +} + +static int +qemuAppendZPCIDevStr(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) +{ + char *devstr = NULL; + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildZPCIDevStr(dev))) + return -1; + + virCommandAddArg(cmd, devstr); + + VIR_FREE(devstr); + return 0; +} + +static int +qemuBuildExtensionCommandLine(virCommandPtr cmd, + virDomainDeviceInfoPtr dev) +{ + int ret; + + if ((ret = qemuCheckDeviceIsZPCI(dev)) == 1) + return qemuAppendZPCIDevStr(cmd, dev); + + return ret; +} static int qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, @@ -2294,6 +2357,9 @@ qemuBuildDiskCommandLine(virCommandPtr cmd, bootindex) < 0) return -1; } else { + if (qemuBuildExtensionCommandLine(cmd, &disk->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildDiskDeviceStr(def, disk, bootindex, @@ -2493,6 +2559,9 @@ qemuBuildFSDevCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, optstr); VIR_FREE(optstr); + if (qemuBuildExtensionCommandLine(cmd, &fs->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildFSDevStr(def, fs, qemuCaps))) return -1; @@ -2977,6 +3046,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, goto cleanup; if (devstr) { + if (qemuBuildExtensionCommandLine(cmd, &cont->info) < 0) { + VIR_FREE(devstr); + goto cleanup; + } virCommandAddArg(cmd, "-device"); virCommandAddArg(cmd, devstr); VIR_FREE(devstr); @@ -3780,6 +3853,9 @@ qemuBuildWatchdogCommandLine(virCommandPtr cmd, if (!def->watchdog) return 0; + if (qemuBuildExtensionCommandLine(cmd, &def->watchdog->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); optstr = qemuBuildWatchdogDevStr(def, watchdog, qemuCaps); @@ -3864,6 +3940,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, if (qemuBuildVirtioOptionsStr(&buf, def->memballoon->virtio, qemuCaps) < 0) goto error; + if (qemuBuildExtensionCommandLine(cmd, &def->memballoon->info) < 0) + goto error; + virCommandAddArg(cmd, "-device"); virCommandAddArgBuffer(cmd, &buf); return 0; @@ -4086,6 +4165,9 @@ qemuBuildInputCommandLine(virCommandPtr cmd, virDomainInputDefPtr input = def->inputs[i]; char *devstr = NULL; + if (qemuBuildExtensionCommandLine(cmd, &input->info) < 0) + return -1; + if (qemuBuildInputDevStr(&devstr, def, input, qemuCaps) < 0) return -1; @@ -4227,6 +4309,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd, if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) { virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL); } else { + if (qemuBuildExtensionCommandLine(cmd, &sound->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildSoundDevStr(def, sound, qemuCaps))) return -1; @@ -4466,6 +4551,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, if (video->primary) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) { + if (qemuBuildExtensionCommandLine(cmd, + &def->videos[i]->info) < 0) + return -1; virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps))) @@ -4478,6 +4566,9 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, return -1; } } else { + if (qemuBuildExtensionCommandLine(cmd, &def->videos[i]->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildDeviceVideoStr(def, video, qemuCaps))) @@ -5349,6 +5440,10 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); } } + + if (qemuBuildExtensionCommandLine(cmd, hostdev->info) < 0) + return -1; + virCommandAddArg(cmd, "-device"); devstr = qemuBuildPCIHostdevDevStr(def, hostdev, bootIndex, configfd_name, qemuCaps); @@ -5815,6 +5910,9 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager, virCommandAddArgBuffer(cmd, &buf); /* add the device */ + if (qemuBuildExtensionCommandLine(cmd, &rng->info) < 0) + return -1; + if (!(tmp = qemuBuildRNGDevStr(def, rng, qemuCaps))) return -1; virCommandAddArgList(cmd, "-device", tmp, NULL); @@ -8371,6 +8469,9 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virCommandAddArg(cmd, "-netdev"); virCommandAddArg(cmd, netdev); + if (qemuBuildExtensionCommandLine(cmd, &net->info) < 0) + goto cleanup; + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, queues, qemuCaps))) { goto cleanup; @@ -8652,6 +8753,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, * New way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 */ if (qemuDomainSupportsNicdev(def, net)) { + if (qemuBuildExtensionCommandLine(cmd, &net->info) < 0) + goto cleanup; + if (!(nic = qemuBuildNicDevStr(def, net, bootindex, vhostfdSize, qemuCaps))) goto cleanup; @@ -9076,6 +9180,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, switch ((virDomainShmemModel)shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0) + return -1; + devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps); break; @@ -9094,6 +9201,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, ATTRIBUTE_FALLTHROUGH; case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + if (qemuBuildExtensionCommandLine(cmd, &shmem->info) < 0) + return -1; + devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps); break; @@ -10253,6 +10363,10 @@ qemuBuildVsockCommandLine(virCommandPtr cmd, virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); priv->vhostfd = -1; + + if (qemuBuildExtensionCommandLine(cmd, &vsock->info) < 0) + goto cleanup; + virCommandAddArgList(cmd, "-device", devstr, NULL); ret = 0; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index cf17dc1ede..bee4029b75 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -175,6 +175,10 @@ char *qemuBuildRedirdevDevStr(const virDomainDef *def, virDomainRedirdevDefPtr dev, virQEMUCapsPtr qemuCaps); +char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev); + +int qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info); + int qemuNetworkPrepareDevices(virDomainDefPtr def); int qemuGetDriveSourceString(virStorageSourcePtr src, diff --git a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args index 20e63a15b5..ffb5d1597e 100644 --- a/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args +++ b/tests/qemuxml2argvdata/disk-virtio-s390-zpci.args @@ -21,6 +21,7 @@ server,nowait \ -no-shutdown \ -boot c \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ +-device zpci,uid=25,fid=31,target=virtio-disk0,id=zpci25 \ -device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk0,\ id=virtio-disk0 \ -device virtio-balloon-ccw,id=balloon0,devno=fe.0.0000 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args new file mode 100644 index 0000000000..e5987e1053 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args @@ -0,0 +1,26 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-device zpci,uid=1,fid=0,target=hostdev0,id=zpci1 \ +-device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x1 \ +-device zpci,uid=2,fid=1,target=balloon0,id=zpci2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml new file mode 100644 index 0000000000..36161006ab --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args new file mode 100644 index 0000000000..dcbb179a7d --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-device zpci,uid=3,fid=2,target=pci.1,id=zpci3 \ +-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \ +-device zpci,uid=65535,fid=4294967295,target=hostdev0,id=zpci65535 \ +-device vfio-pci,host=ffff:00:00.0,id=hostdev0,bus=pci.1,addr=0x1f \ +-device zpci,uid=1,fid=0,target=hostdev1,id=zpci1 \ +-device vfio-pci,host=00:00.0,id=hostdev1,bus=pci.0,addr=0x2 \ +-device zpci,uid=2,fid=1,target=balloon0,id=zpci2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml new file mode 100644 index 0000000000..779eb12ac2 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0xffff' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x01' slot='0x1f' function='0x0' fid='0xffffffff' uid='0xffff'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' fid='0x00000000' uid='0x0001'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args new file mode 100644 index 0000000000..476237fc45 --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args @@ -0,0 +1,40 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-s390x \ +-name QEMUGuest1 \ +-S \ +-machine s390-ccw-virtio,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot c \ +-device zpci,uid=35,fid=63,target=hostdev0,id=zpci35 \ +-device vfio-pci,host=0001:00:00.0,id=hostdev0,bus=pci.0,addr=0x3 \ +-device zpci,uid=53,fid=104,target=hostdev1,id=zpci53 \ +-device vfio-pci,host=0002:00:00.0,id=hostdev1,bus=pci.0,addr=0x1 \ +-device zpci,uid=1,fid=0,target=hostdev2,id=zpci1 \ +-device vfio-pci,host=0003:00:00.0,id=hostdev2,bus=pci.0,addr=0x2 \ +-device zpci,uid=2,fid=1,target=hostdev3,id=zpci2 \ +-device vfio-pci,host=0004:00:00.0,id=hostdev3,bus=pci.0,addr=0x5 \ +-device zpci,uid=83,fid=2,target=hostdev4,id=zpci83 \ +-device vfio-pci,host=0005:00:00.0,id=hostdev4,bus=pci.0,addr=0x7 \ +-device zpci,uid=3,fid=114,target=hostdev5,id=zpci3 \ +-device vfio-pci,host=0006:00:00.0,id=hostdev5,bus=pci.0,addr=0x9 \ +-device zpci,uid=23,fid=3,target=hostdev6,id=zpci23 \ +-device vfio-pci,host=0007:00:00.0,id=hostdev6,bus=pci.0,addr=0x4 \ +-device zpci,uid=4,fid=40,target=hostdev7,id=zpci4 \ +-device vfio-pci,host=0008:00:00.0,id=hostdev7,bus=pci.0,addr=0x6 \ +-device zpci,uid=5,fid=4,target=balloon0,id=zpci5 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml new file mode 100644 index 0000000000..e69714fc9c --- /dev/null +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml @@ -0,0 +1,67 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0023' fid='0x0000003f'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' uid='0x0035' fid='0x00000068'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0' uid='0x0053'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0006' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' fid='0x00000072'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' uid='0x0017'/> + </hostdev> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' fid='0x00000028'/> + </hostdev> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args index 622c504da0..80de60acaa 100644 --- a/tests/qemuxml2argvdata/hostdev-vfio-zpci.args +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci.args @@ -20,5 +20,7 @@ server,nowait \ -rtc base=utc \ -no-shutdown \ -boot c \ +-device zpci,uid=25,fid=31,target=hostdev0,id=zpci25 \ -device vfio-pci,host=00:00.0,id=hostdev0,bus=pci.0,addr=0x8 \ +-device zpci,uid=1,fid=0,target=balloon0,id=zpci1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x1 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2bfc3ed215..21890a7a4e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1595,6 +1595,19 @@ mymain(void) QEMU_CAPS_VFIO_PCI_DISPLAY); DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-multidomain-many", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-boundaries", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST_FAILURE("hostdev-vfio-zpci", + QEMU_CAPS_DEVICE_VFIO_PCI); DO_TEST("pci-rom", NONE); DO_TEST("pci-rom-disabled", NONE); DO_TEST("pci-rom-disabled-invalid", NONE); diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml new file mode 100644 index 0000000000..8647cab1fc --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0001' fid='0x00000000'/> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0002' fid='0x00000001'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml new file mode 100644 index 0000000000..0b48c7658a --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml @@ -0,0 +1,42 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <controller type='pci' index='1' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0xffff' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x01' slot='0x1f' function='0x0' uid='0xffff' fid='0xffffffff'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0001' fid='0x00000000'/> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0002' fid='0x00000001'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml new file mode 100644 index 0000000000..9a31057d0b --- /dev/null +++ b/tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml @@ -0,0 +1,79 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0' uid='0x0023' fid='0x0000003f'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0002' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0035' fid='0x00000068'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0003' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' uid='0x0001' fid='0x00000000'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0004' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0' uid='0x0002' fid='0x00000001'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0005' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0' uid='0x0053' fid='0x00000002'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0006' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' uid='0x0003' fid='0x00000072'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0007' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' uid='0x0017' fid='0x00000003'/> + </hostdev> + <hostdev mode='subsystem' type='pci' managed='no'> + <driver name='vfio'/> + <source> + <address domain='0x0008' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0' uid='0x0004' fid='0x00000028'/> + </hostdev> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0' uid='0x0005' fid='0x00000004'/> + </memballoon> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6e41fd9bff..ed78233754 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -463,6 +463,17 @@ mymain(void) DO_TEST("hostdev-pci-address", NONE); DO_TEST("hostdev-vfio", NONE); DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW); + DO_TEST("hostdev-vfio-zpci-multidomain-many", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-autogenerate", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_ZPCI); + DO_TEST("hostdev-vfio-zpci-boundaries", + QEMU_CAPS_DEVICE_VFIO_PCI, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_ZPCI); DO_TEST("hostdev-mdev-precreated", NONE); DO_TEST("hostdev-mdev-display", QEMU_CAPS_VFIO_PCI_DISPLAY); DO_TEST("pci-rom", NONE); -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+char * +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "zpci"); + virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid); + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid); + virBufferAsprintf(&buf, ",target=%s", dev->alias); + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
Just making sure: is the uid a good choice for naming the zpci device? I would perhaps expect the fid to be in there as well, but since both have to be unique (right?) I guess it doesn't really matter...
+ + if (virBufferCheckError(&buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +int +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
Based on the name, this is a predicate: it should return a boolean value rather than an int.
+{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + return 1; + } + + if (info->addr.pci.zpci) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support zpci devices")); + return -1; + }
Not sure about this second check. I guess the logic is supposed to be that, when passed a "proper" zPCI device, the function would have returned with the first check, and if we find a zPCI address after that it's an error. Makes sense, but it's kinda obfuscated and I'm not sure the error message is accurate: can it really only be a problem of the QEMU binary not supporting the zPCI feature, or could we have gotten here because the user is trying to assing zPCI addresses to devices that don't support them? Either way, calling a predicate should never result in an error being reported, so you need to move this check somewhere else. [...]
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'/> + </hostdev> + </devices> +</domain>
This test case would have been a great addition to the previous commit, where you actually introduced the ability to automatically allocate zPCI addresses... Another thing that I forgot to ask earlier and this is as good a time as any: once zPCI support has been merged, will users have to opt-in to it using <address type='pci'/> or will they get zPCI devices by default? And if so, will it still be possible for them to get CCW devices instead if wanted? -- Andrea Bolognani / Red Hat / Virtualization

On 08/16/2018 06:31 PM, Andrea Bolognani wrote:
Another thing that I forgot to ask earlier and this is as good a time as any: once zPCI support has been merged, will users have to opt-in to it using
<address type='pci'/>
or will they get zPCI devices by default? And if so, will it still be possible for them to get CCW devices instead if wanted?
On S390 the default behavior is to assign CCW addresses. This has not and is not going to be changed. Therefore a user has to explicitly specify a PCI address or in your words: zPCI is an opt-in. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Fri, 2018-08-17 at 08:35 +0200, Boris Fiuczynski wrote:
On 08/16/2018 06:31 PM, Andrea Bolognani wrote:
Another thing that I forgot to ask earlier and this is as good a time as any: once zPCI support has been merged, will users have to opt-in to it using
<address type='pci'/>
or will they get zPCI devices by default? And if so, will it still be possible for them to get CCW devices instead if wanted?
On S390 the default behavior is to assign CCW addresses. This has not and is not going to be changed. Therefore a user has to explicitly specify a PCI address or in your words: zPCI is an opt-in.
Sounds good to me! Just making sure :) -- Andrea Bolognani / Red Hat / Virtualization

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+char * +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "zpci"); + virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid); + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid); + virBufferAsprintf(&buf, ",target=%s", dev->alias); + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid); Just making sure: is the uid a good choice for naming the zpci device? I would perhaps expect the fid to be in there as well, but since both have to be unique (right?) I guess it doesn't really matter... Either uid or fid, the value must be unique. But uid is defined by user. Of course, we also give the permission to define fid. But uid is more appropriate. + + if (virBufferCheckError(&buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +int +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info) Based on the name, this is a predicate: it should return a boolean value rather than an int. 0 means it's not zpci. 1 means it's zpci. -1 means error.
+{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) { + return 1; + } + + if (info->addr.pci.zpci) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support zpci devices")); + return -1; + } Not sure about this second check. I guess the logic is supposed to be that, when passed a "proper" zPCI device, the function would have returned with the first check, and if we find a zPCI address after that it's an error. Makes sense, but it's kinda obfuscated and I'm not sure the error message is accurate: can it really only be a problem of the QEMU binary not supporting the zPCI feature, or could we have gotten here because the user is trying to assing zPCI addresses to devices that don't support them? Yes, it's because user could define zpci address in xml but
在 2018/8/17 上午12:31, Andrea Bolognani 写道: there's no zpci support.
Either way, calling a predicate should never result in an error being reported, so you need to move this check somewhere else.
Acutally I have found out a proper place to add this check for a long time. So this is the best place to add, at least I think for now.
[...]
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <os> + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <hostdev mode='subsystem' type='pci'> + <driver name='vfio'/> + <source> + <address domain='0x0000' bus='0x00' slot='0x00' function='0x0'/> + </source> + <address type='pci'/> + </hostdev> + </devices> +</domain> This test case would have been a great addition to the previous commit, where you actually introduced the ability to automatically allocate zPCI addresses...
Another thing that I forgot to ask earlier and this is as good a time as any: once zPCI support has been merged, will users have to opt-in to it using
<address type='pci'/>
or will they get zPCI devices by default? And if so, will it still be possible for them to get CCW devices instead if wanted?

On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote:
在 2018/8/17 上午12:31, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+ virBufferAddLit(&buf, "zpci"); + virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid); + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid); + virBufferAsprintf(&buf, ",target=%s", dev->alias); + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
Just making sure: is the uid a good choice for naming the zpci device? I would perhaps expect the fid to be in there as well, but since both have to be unique (right?) I guess it doesn't really matter...
Either uid or fid, the value must be unique. But uid is defined by user. Of course, we also give the permission to define fid. But uid is more appropriate.
So which is unique: uid, fid, or the combination of the two? Could I have -device zpci,uid=1,fid=1 -device zpci,uid=1,fid=2 or would that not work? What about -device zpci,uid=1,fid=1 -device zpci,uid=2,fid=1 would that be okay?
+int +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
Based on the name, this is a predicate: it should return a boolean value rather than an int.
0 means it's not zpci. 1 means it's zpci. -1 means error.
My point is that a funtion called fooIsBaz() should only ever check whether the foo in question is indeed a baz, without taking any other action such as reporting errors. Leave that to the caller, or give the function a different name.
Either way, calling a predicate should never result in an error being reported, so you need to move this check somewhere else.
Acutally I have found out a proper place to add this check for a long time. So this is the best place to add, at least I think for now.
qemuDomainDeviceDefValidate() looks like a reasonable entry point for checks such as "you specified a zPCI address but this is not an s390 guest" or "your configuration requires zPCI but the QEMU binary doesn't support that feature". Both of the cases above should have proper test suite coverage, by the way. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/20 下午7:14, Andrea Bolognani 写道:
On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote:
在 2018/8/17 上午12:31, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
+ virBufferAddLit(&buf, "zpci"); + virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid); + virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid); + virBufferAsprintf(&buf, ",target=%s", dev->alias); + virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid); Just making sure: is the uid a good choice for naming the zpci device? I would perhaps expect the fid to be in there as well, but since both have to be unique (right?) I guess it doesn't really matter... Either uid or fid, the value must be unique. But uid is defined by user. Of course, we also give the permission to define fid. But uid is more appropriate. So which is unique: uid, fid, or the combination of the two? Could I have
-device zpci,uid=1,fid=1 -device zpci,uid=1,fid=2
or would that not work? What about
-device zpci,uid=1,fid=1 -device zpci,uid=2,fid=1
would that be okay? Both can't work. FID must be unique and UID must be also unique. They are independent.
+int +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info) Based on the name, this is a predicate: it should return a boolean value rather than an int. 0 means it's not zpci. 1 means it's zpci. -1 means error. My point is that a funtion called fooIsBaz() should only ever check whether the foo in question is indeed a baz, without taking any other action such as reporting errors. Leave that to the caller, or give the function a different name. I think moving to the caller is proper.
Either way, calling a predicate should never result in an error being reported, so you need to move this check somewhere else. Acutally I have found out a proper place to add this check for a long time. So this is the best place to add, at least I think for now. qemuDomainDeviceDefValidate() looks like a reasonable entry point for checks such as "you specified a zPCI address but this is not an s390 guest" or "your configuration requires zPCI but the QEMU binary doesn't support that feature". Both of the cases above should have proper test suite coverage, by the way.
How about my idea mentioned above?

On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote:
在 2018/8/20 下午7:14, Andrea Bolognani 写道:
So which is unique: uid, fid, or the combination of the two? Could I have
-device zpci,uid=1,fid=1 -device zpci,uid=1,fid=2
or would that not work? What about
-device zpci,uid=1,fid=1 -device zpci,uid=2,fid=1
would that be okay?
Both can't work. FID must be unique and UID must be also unique. They are independent.
Got it, thanks.
My point is that a funtion called fooIsBaz() should only ever check whether the foo in question is indeed a baz, without taking any other action such as reporting errors. Leave that to the caller, or give the function a different name.
I think moving to the caller is proper.
qemuDomainDeviceDefValidate() looks like a reasonable entry point for checks such as "you specified a zPCI address but this is not an s390 guest" or "your configuration requires zPCI but the QEMU binary doesn't support that feature". Both of the cases above should have proper test suite coverage, by the way.
How about my idea mentioned above?
You mean moving the check to the caller? I think qemuDomainDeviceDefValidate() is a better choice because it should allow you to implement the checks once, potentially with more context such as information about the domain and QEMU capabilities, and have them performed regardless of whether the device is being processed eg. during regular parsing or hotplug. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/22 下午4:42, Andrea Bolognani 写道:
On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote:
在 2018/8/20 下午7:14, Andrea Bolognani 写道:
So which is unique: uid, fid, or the combination of the two? Could I have
-device zpci,uid=1,fid=1 -device zpci,uid=1,fid=2
or would that not work? What about
-device zpci,uid=1,fid=1 -device zpci,uid=2,fid=1
would that be okay? Both can't work. FID must be unique and UID must be also unique. They are independent. Got it, thanks.
My point is that a funtion called fooIsBaz() should only ever check whether the foo in question is indeed a baz, without taking any other action such as reporting errors. Leave that to the caller, or give the function a different name. I think moving to the caller is proper.
qemuDomainDeviceDefValidate() looks like a reasonable entry point for checks such as "you specified a zPCI address but this is not an s390 guest" or "your configuration requires zPCI but the QEMU binary doesn't support that feature". Both of the cases above should have proper test suite coverage, by the way. How about my idea mentioned above? You mean moving the check to the caller? Yes.
I think qemuDomainDeviceDefValidate() is a better choice because it should allow you to implement the checks once, potentially with more context such as information about the domain and QEMU capabilities, and have them performed regardless of whether the device is being processed eg. during regular parsing or hotplug.
IIUC, in qemuDomainDeviceDefValidate(), we have to directly access device_info from DeviceDef to check zpci. I think direct accessing is insecure and we have to process those device types that has device_info. In addition, these is a special case that address is not defined and pci address type will be chosen by default. I found there are a lot of caps check code while building command line.

This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way. Currently, the following PCI devices are supported: virtio-blk-pci virtio-net-pci virtio-rng-pci virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci vfio-pci SCSIVhost device Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_hotplug.c | 155 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 145 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1488f0a7c2..d7f427597c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -154,6 +154,74 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, } +static int +qemuDomainAttachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + int ret = -1; + char *devstr_zpci = NULL; + + if (!(devstr_zpci = qemuBuildZPCIDevStr(info))) + goto cleanup; + + if (qemuMonitorAddDevice(mon, devstr_zpci) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(devstr_zpci); + return ret; +} + + +static int +qemuDomainDetachZPCIDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + char *zpciAlias = NULL; + int ret = -1; + + if (virAsprintf(&zpciAlias, "zpci%d", info->addr.pci.zpci->zpci_uid) < 0) + goto cleanup; + + if (qemuMonitorDelDevice(mon, zpciAlias) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(zpciAlias); + return ret; +} + + +static int +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + int ret; + + if ((ret = qemuCheckDeviceIsZPCI(info)) == 1) + return qemuDomainAttachZPCIDevice(mon, info); + + return ret; +} + + +static int +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon, + virDomainDeviceInfoPtr info) +{ + int ret; + + if ((ret = qemuCheckDeviceIsZPCI(info)) == 1) + return qemuDomainDetachZPCIDevice(mon, info); + + return ret; +} + + static int qemuHotplugWaitForTrayEject(virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; @@ -778,7 +851,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + + if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &controller->info)); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; ret = -1; @@ -1224,7 +1305,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - if (qemuDomainIsS390CCW(vm->def) && + if (net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + qemuDomainIsS390CCW(vm->def) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CCW)) { net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; if (!(ccwaddrs = virDomainCCWAddressSetCreateFromDomain(vm->def))) @@ -1294,7 +1376,15 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto try_remove; qemuDomainObjEnterMonitor(driver, vm); + + if (qemuDomainAttachExtensionDevice(priv->mon, &net->info) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditNet(vm, NULL, net, "attach", false); + goto try_remove; + } + if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &net->info)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); virDomainAuditNet(vm, NULL, net, "attach", false); goto try_remove; @@ -1512,8 +1602,15 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, goto error; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, - configfd, configfd_name); + + if ((ret = qemuDomainAttachExtensionDevice(priv->mon, hostdev->info)) < 0) + goto exit_monitor; + + if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name)) < 0) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info)); + + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) goto error; @@ -2169,8 +2266,13 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (qemuMonitorAddObject(priv->mon, &props, &objAlias) < 0) goto exit_monitor; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &rng->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &rng->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; @@ -2663,8 +2765,16 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, vhostfdName); + if ((ret = qemuDomainAttachExtensionDevice(priv->mon, hostdev->info)) < 0) + goto exit_monitor; + + if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, + vhostfdName)) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info)); + goto exit_monitor; + } + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) goto audit; @@ -2909,8 +3019,13 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, release_backing = true; - if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &shmem->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &shmem->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { release_address = false; @@ -3083,8 +3198,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) { + if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0) + goto exit_monitor; + } + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info)); goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; @@ -3162,9 +3286,15 @@ qemuDomainAttachVsockDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname) < 0) + + if (qemuDomainAttachExtensionDevice(priv->mon, &vsock->info) < 0) goto exit_monitor; + if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &vsock->info)); + goto exit_monitor; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { releaseaddr = false; goto cleanup; @@ -5143,6 +5273,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); + if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way.
It looks like you ended up implementing explicit hot unplug at least for controllers. I think perhaps it would be a good idea to implement it for all devices instead of relying on QEMU's own unplug cascading so that we have more control over the whole process.
@@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); goto exit_monitor; + }
if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2;
So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable :) I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/18 上午12:10, Andrea Bolognani 写道:
This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way. It looks like you ended up implementing explicit hot unplug at least for controllers. I think perhaps it would be a good idea to implement it for all devices instead of relying on QEMU's own unplug cascading so that we have more control over the whole
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: process. It's different between controller and device. And we only hot unplug pci controller, not for all controller types. In addition, we only could do hot-unplug one time, either zpci device or corresponding pci device. It's due to Qemu logic. Qemu will hot-unplug zpci device automatically while doing hotplug pci device, and vice versa.
@@ -669,8 +737,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) + goto exit_monitor; + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); goto exit_monitor; + }
if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -2; So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable :)
I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review.
We have to add zpci device firstly and add corresponding pci device secondly. Do you think it's redundant to call monitor twice to add two devices?

On Mon, 2018-08-20 at 16:04 +0800, Yi Min Zhao wrote:
在 2018/8/18 上午12:10, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
This commit adds hotplug support for PCI devices on S390 guests. There's no need to implement hot unplug for zPCI as QEMU implements an unplug callback which will unplug both PCI and zPCI device in a cascaded way.
It looks like you ended up implementing explicit hot unplug at least for controllers. I think perhaps it would be a good idea to implement it for all devices instead of relying on QEMU's own unplug cascading so that we have more control over the whole process.
It's different between controller and device. And we only hot unplug pci controller, not for all controller types. In addition, we only could do hot-unplug one time, either zpci device or corresponding pci device. It's due to Qemu logic. Qemu will hot-unplug zpci device automatically while doing hotplug pci device, and vice versa.
Ouch, that's kinda nasty... And I wonder why that doesn't work for controllers too? They're not *that* special :)
So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable :)
I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review.
We have to add zpci device firstly and add corresponding pci device secondly. Do you think it's redundant to call monitor twice to add two devices?
Again, I'm not familiar at all with this code, but entering (and exiting) the monitor once for each device you're dealing with seems to be a pattern. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/20 下午7:34, Andrea Bolognani 写道:
So, I'm very much not familiar with the hotplug code and seeing changes to stuff like qemuDomainAttachDiskGeneric() makes me a bit uncomfortable:)
I can't spot anything obviously wrong in your changes, but I think perhaps you might want to enter and exit the monitor separately for the zpci device and for the virtio device? I'm not sure that's useful at all, but network devices for example seems to follow that pattern... It would be great if someone with more experience in this area could provide a review. We have to add zpci device firstly and add corresponding pci device secondly. Do you think it's redundant to call monitor twice to add two devices? Again, I'm not familiar at all with this code, but entering (and exiting) the monitor once for each device you're dealing with seems to be a pattern. So let's see other reviewers' comment.

Update 'Device address' section to describe the 'uid' and 'fid' attributes. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- docs/formatdomain.html.in | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b63467bd91..2a1b6b2ffa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3838,7 +3838,14 @@ (<span class="since">since 0.9.7, requires QEMU 0.13</span>). <code>multifunction</code> defaults to 'off', but should be set to 'on' for function 0 of a slot that will - have multiple functions used.<br/> + have multiple functions used. (<span class="since">Since 4.7.0 + </span>), PCI address extensions depending on the architecture + are supported. E.g. for S390 guests, PCI addresses have + additional attributes: <code>uid</code> (a hex value between + 0x1 and 0xffff, inclusive), and <code>fid</code> (a hex value + between 0x0 and 0xffffffff, inclusive), which are used by PCI + devices on S390 for User-defined Identifiers and Function + Identifiers.<br/> <span class="since">Since 1.3.5</span>, some hypervisor drivers may accept an <code><address type='pci'/></code> element with no other attributes as an explicit request to -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+ have multiple functions used. (<span class="since">Since 4.7.0 + </span>), PCI address extensions depending on the architecture + are supported. E.g. for S390 guests, PCI addresses have + additional attributes: <code>uid</code> (a hex value between + 0x1 and 0xffff, inclusive), and <code>fid</code> (a hex value + between 0x0 and 0xffffffff, inclusive), which are used by PCI + devices on S390 for User-defined Identifiers and Function + Identifiers.<br/>
I would format the minimum values for uid and fid as 0x0001 and 0x00000000 respectively for consistency - that's how they will show up in the domain XML after all. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/18 上午12:19, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+ have multiple functions used. (<span class="since">Since 4.7.0 + </span>), PCI address extensions depending on the architecture + are supported. E.g. for S390 guests, PCI addresses have + additional attributes: <code>uid</code> (a hex value between + 0x1 and 0xffff, inclusive), and <code>fid</code> (a hex value + between 0x0 and 0xffffffff, inclusive), which are used by PCI + devices on S390 for User-defined Identifiers and Function + Identifiers.<br/> I would format the minimum values for uid and fid as 0x0001 and 0x00000000 respectively for consistency - that's how they will show up in the domain XML after all.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks! Have revised.

Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index afafb9c337..545d7c552a 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,17 @@ iscsiadm. It support basic pool operations: checkPool and refreshPool. </description> </change> + <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + The new zPCI attributes uid (user-defined identifier) + and fid (PCI function identifier) of the S390 platform + extend the PCI device address to support the PCI + passthrough on S390. + </description> + </change> </section> <section title="Improvements"> </section> -- Yi Min

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+ <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + The new zPCI attributes uid (user-defined identifier) + and fid (PCI function identifier) of the S390 platform + extend the PCI device address to support the PCI + passthrough on S390. + </description> + </change>
This doesn't look entirely accurate: the attributes are not only used for assigned devices but for emulated devices as well, so the text should reflect that. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/18 上午12:21, Andrea Bolognani 写道:
+ <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + The new zPCI attributes uid (user-defined identifier) + and fid (PCI function identifier) of the S390 platform + extend the PCI device address to support the PCI + passthrough on S390. + </description> + </change> This doesn't look entirely accurate: the attributes are not only used for assigned devices but for emulated devices as well, so
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] the text should reflect that.
How about: *** to support the PCI functionality on S390.

On Mon, 2018-08-20 at 11:39 +0800, Yi Min Zhao wrote:
在 2018/8/18 上午12:21, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...]
+ <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + The new zPCI attributes uid (user-defined identifier) + and fid (PCI function identifier) of the S390 platform + extend the PCI device address to support the PCI + passthrough on S390. + </description> + </change>
This doesn't look entirely accurate: the attributes are not only used for assigned devices but for emulated devices as well, so the text should reflect that.
How about:
*** to support the PCI functionality on S390.
Perhaps something like qemu: Added support for PCI devices on s390 PCI addresses can now include the new uid (user-defined identifier) and fid (PCI function identifier) attributes, which make the corresponding devices usable by s390 guests. -- Andrea Bolognani / Red Hat / Virtualization

在 2018/8/20 下午6:17, Andrea Bolognani 写道:
On Mon, 2018-08-20 at 11:39 +0800, Yi Min Zhao wrote:
在 2018/8/18 上午12:21, Andrea Bolognani 写道:
+ <change> + <summary> + qemu: Added support for PCI device passthrough on S390 + </summary> + <description> + The new zPCI attributes uid (user-defined identifier) + and fid (PCI function identifier) of the S390 platform + extend the PCI device address to support the PCI + passthrough on S390. + </description> + </change> This doesn't look entirely accurate: the attributes are not only used for assigned devices but for emulated devices as well, so
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] the text should reflect that. How about:
*** to support the PCI functionality on S390. Perhaps something like
qemu: Added support for PCI devices on s390
PCI addresses can now include the new uid (user-defined identifier) and fid (PCI function identifier) attributes, which make the corresponding devices usable by s390 guests.
Better, I will do a little update based on your words.

Is there any comment? I expect comments from all of you. 在 2018/8/7 下午5:10, Yi Min Zhao 写道:
Abstract ======== The PCI representation in QEMU has recently been extended for S390 allowing configuration of zPCI attributes like uid (user-defined identifier) and fid (PCI function identifier). The details can be found here: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html
To support the new zPCI feature of the S390 platform, two new XML attributes, @uid and @fid, are introduced for device addresses of type 'pci', i.e.: <hostdev mode='subsystem' type='pci'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> </hostdev>
uid and fid are optional attributes. If they are defined by the user, unique values within the guest domain must be used. If they are not specified and the architecture requires them, they are automatically generated with non-conflicting values.
Current implementation is the most seamless one for the user as it unites the address specific data of a PCI device on one XML element. It could accommodate both specifying our special parameters (uid and fid) and re-using standard statements (domain, bus, slot and function) for PCI devices. User can still specify bus/slot/function for the virtualized PCI devices in the XML.
Thus uid/fid act as an extension to the PCI address and are stored in a new structure 'virZPCIDeviceAddress' which is a member of common PCI Address structure. Additionally, two hashtables are used for assignment and reservation of uid/fid.
In support of extending the PCI address, a new PCI address extension flag is introduced. This extension flag allows is not only dedicated for the S390 platform but also other architectures needing certain extensions to PCI address space.
Code Base ========= commit in master: 087de2f5a3: docs: formatdomain: fix spacing before parentheses
Change Log ========== v2->v3: 1. Revise code style. 2. Update test cases. 3. Introduce qemuDomainCollectPCIAddressExtension() to collect PCI extension addresses. 4. Introduce virDeviceInfoPCIAddressExtensionPresent() to check if zPCI address exists. 5. Optimize zPCI address check logic. 6. Optimize passed parameters of zPCI addr alloc/release/reserve functions. 7. Report enum range error in qemuDomainDeviceSupportZPCI(). 8. Update commit messages.
v1->v2: 1. Separate test commit and merge testcases into corresponding commits that introduce the functionalities firstly. 2. Spare some checks for zpci device. 3. Add vsock and controller support. 4. Add uin32 type schema. 5. Rename zpciuid and zpcifid to zpci_uid and zpci_fid. 6. Always return multibus support on S390.
Yi Min Zhao (12): conf: Add definitions for 'uid' and 'fid' PCI address attributes qemu: Introduce zPCI capability conf: Introduce a new PCI address extension flag qemu: Enable PCI multi bus for S390 guests qemu: Auto add pci-root for s390/s390x guests conf: Introduce address caching for PCI extensions conf: Introduce parser, formatter for uid and fid conf: Allocate/release 'uid' and 'fid' in PCI address qemu: Generate and use zPCI device in QEMU command line qemu: Add hotpluging support for PCI devices on S390 guests docs: Add 'uid' and 'fid' information news: Update news for PCI address extension attributes
docs/formatdomain.html.in | 9 +- docs/news.xml | 11 + docs/schemas/basictypes.rng | 23 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 78 +++++ src/conf/device_conf.h | 8 + src/conf/domain_addr.c | 379 +++++++++++++++++++++ src/conf/domain_addr.h | 29 ++ src/conf/domain_conf.c | 6 + src/libvirt_private.syms | 4 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 114 +++++++ src/qemu/qemu_command.h | 4 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain_address.c | 204 ++++++++++- src/qemu/qemu_hotplug.c | 155 ++++++++- src/util/virpci.h | 13 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 27 ++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 + .../hostdev-vfio-zpci-autogenerate.args | 26 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 + .../hostdev-vfio-zpci-boundaries.args | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 26 ++ .../hostdev-vfio-zpci-multidomain-many.args | 40 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 ++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 26 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++ tests/qemuxml2argvtest.c | 17 + tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 ++ .../hostdev-vfio-zpci-autogenerate.xml | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 42 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++ tests/qemuxml2xmltest.c | 14 + 41 files changed, 1575 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml

On Mon, 13 Aug 2018 12:46:16 +0800 Yi Min Zhao <zyimin@linux.ibm.com> wrote:
Is there any comment? I expect comments from all of you.
Well, I don't have any objections from my side, but you need the libvirt folks' opinion on this.
在 2018/8/7 下午5:10, Yi Min Zhao 写道:
Abstract ======== The PCI representation in QEMU has recently been extended for S390 allowing configuration of zPCI attributes like uid (user-defined identifier) and fid (PCI function identifier). The details can be found here: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html
To support the new zPCI feature of the S390 platform, two new XML attributes, @uid and @fid, are introduced for device addresses of type 'pci', i.e.: <hostdev mode='subsystem' type='pci'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> </hostdev>
uid and fid are optional attributes. If they are defined by the user, unique values within the guest domain must be used. If they are not specified and the architecture requires them, they are automatically generated with non-conflicting values.
Current implementation is the most seamless one for the user as it unites the address specific data of a PCI device on one XML element. It could accommodate both specifying our special parameters (uid and fid) and re-using standard statements (domain, bus, slot and function) for PCI devices. User can still specify bus/slot/function for the virtualized PCI devices in the XML.
Thus uid/fid act as an extension to the PCI address and are stored in a new structure 'virZPCIDeviceAddress' which is a member of common PCI Address structure. Additionally, two hashtables are used for assignment and reservation of uid/fid.
In support of extending the PCI address, a new PCI address extension flag is introduced. This extension flag allows is not only dedicated for the S390 platform but also other architectures needing certain extensions to PCI address space.
Code Base ========= commit in master: 087de2f5a3: docs: formatdomain: fix spacing before parentheses
Change Log ========== v2->v3: 1. Revise code style. 2. Update test cases. 3. Introduce qemuDomainCollectPCIAddressExtension() to collect PCI extension addresses. 4. Introduce virDeviceInfoPCIAddressExtensionPresent() to check if zPCI address exists. 5. Optimize zPCI address check logic. 6. Optimize passed parameters of zPCI addr alloc/release/reserve functions. 7. Report enum range error in qemuDomainDeviceSupportZPCI(). 8. Update commit messages.
v1->v2: 1. Separate test commit and merge testcases into corresponding commits that introduce the functionalities firstly. 2. Spare some checks for zpci device. 3. Add vsock and controller support. 4. Add uin32 type schema. 5. Rename zpciuid and zpcifid to zpci_uid and zpci_fid. 6. Always return multibus support on S390.
Yi Min Zhao (12): conf: Add definitions for 'uid' and 'fid' PCI address attributes qemu: Introduce zPCI capability conf: Introduce a new PCI address extension flag qemu: Enable PCI multi bus for S390 guests qemu: Auto add pci-root for s390/s390x guests conf: Introduce address caching for PCI extensions conf: Introduce parser, formatter for uid and fid conf: Allocate/release 'uid' and 'fid' in PCI address qemu: Generate and use zPCI device in QEMU command line qemu: Add hotpluging support for PCI devices on S390 guests docs: Add 'uid' and 'fid' information news: Update news for PCI address extension attributes
docs/formatdomain.html.in | 9 +- docs/news.xml | 11 + docs/schemas/basictypes.rng | 23 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 78 +++++ src/conf/device_conf.h | 8 + src/conf/domain_addr.c | 379 +++++++++++++++++++++ src/conf/domain_addr.h | 29 ++ src/conf/domain_conf.c | 6 + src/libvirt_private.syms | 4 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 114 +++++++ src/qemu/qemu_command.h | 4 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain_address.c | 204 ++++++++++- src/qemu/qemu_hotplug.c | 155 ++++++++- src/util/virpci.h | 13 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 27 ++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 + .../hostdev-vfio-zpci-autogenerate.args | 26 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 + .../hostdev-vfio-zpci-boundaries.args | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 26 ++ .../hostdev-vfio-zpci-multidomain-many.args | 40 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 ++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 26 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++ tests/qemuxml2argvtest.c | 17 + tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 ++ .../hostdev-vfio-zpci-autogenerate.xml | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 42 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++ tests/qemuxml2xmltest.c | 14 + 41 files changed, 1575 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml

在 2018/8/13 下午2:59, Cornelia Huck 写道:
On Mon, 13 Aug 2018 12:46:16 +0800 Yi Min Zhao <zyimin@linux.ibm.com> wrote:
Is there any comment? I expect comments from all of you. Well, I don't have any objections from my side, but you need the libvirt folks' opinion on this. Thanks for your response!
在 2018/8/7 下午5:10, Yi Min Zhao 写道:
Abstract ======== The PCI representation in QEMU has recently been extended for S390 allowing configuration of zPCI attributes like uid (user-defined identifier) and fid (PCI function identifier). The details can be found here: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html
To support the new zPCI feature of the S390 platform, two new XML attributes, @uid and @fid, are introduced for device addresses of type 'pci', i.e.: <hostdev mode='subsystem' type='pci'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> </hostdev>
uid and fid are optional attributes. If they are defined by the user, unique values within the guest domain must be used. If they are not specified and the architecture requires them, they are automatically generated with non-conflicting values.
Current implementation is the most seamless one for the user as it unites the address specific data of a PCI device on one XML element. It could accommodate both specifying our special parameters (uid and fid) and re-using standard statements (domain, bus, slot and function) for PCI devices. User can still specify bus/slot/function for the virtualized PCI devices in the XML.
Thus uid/fid act as an extension to the PCI address and are stored in a new structure 'virZPCIDeviceAddress' which is a member of common PCI Address structure. Additionally, two hashtables are used for assignment and reservation of uid/fid.
In support of extending the PCI address, a new PCI address extension flag is introduced. This extension flag allows is not only dedicated for the S390 platform but also other architectures needing certain extensions to PCI address space.
Code Base ========= commit in master: 087de2f5a3: docs: formatdomain: fix spacing before parentheses
Change Log ========== v2->v3: 1. Revise code style. 2. Update test cases. 3. Introduce qemuDomainCollectPCIAddressExtension() to collect PCI extension addresses. 4. Introduce virDeviceInfoPCIAddressExtensionPresent() to check if zPCI address exists. 5. Optimize zPCI address check logic. 6. Optimize passed parameters of zPCI addr alloc/release/reserve functions. 7. Report enum range error in qemuDomainDeviceSupportZPCI(). 8. Update commit messages.
v1->v2: 1. Separate test commit and merge testcases into corresponding commits that introduce the functionalities firstly. 2. Spare some checks for zpci device. 3. Add vsock and controller support. 4. Add uin32 type schema. 5. Rename zpciuid and zpcifid to zpci_uid and zpci_fid. 6. Always return multibus support on S390.
Yi Min Zhao (12): conf: Add definitions for 'uid' and 'fid' PCI address attributes qemu: Introduce zPCI capability conf: Introduce a new PCI address extension flag qemu: Enable PCI multi bus for S390 guests qemu: Auto add pci-root for s390/s390x guests conf: Introduce address caching for PCI extensions conf: Introduce parser, formatter for uid and fid conf: Allocate/release 'uid' and 'fid' in PCI address qemu: Generate and use zPCI device in QEMU command line qemu: Add hotpluging support for PCI devices on S390 guests docs: Add 'uid' and 'fid' information news: Update news for PCI address extension attributes
docs/formatdomain.html.in | 9 +- docs/news.xml | 11 + docs/schemas/basictypes.rng | 23 ++ docs/schemas/domaincommon.rng | 1 + src/conf/device_conf.c | 78 +++++ src/conf/device_conf.h | 8 + src/conf/domain_addr.c | 379 +++++++++++++++++++++ src/conf/domain_addr.h | 29 ++ src/conf/domain_conf.c | 6 + src/libvirt_private.syms | 4 + src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 114 +++++++ src/qemu/qemu_command.h | 4 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain_address.c | 204 ++++++++++- src/qemu/qemu_hotplug.c | 155 ++++++++- src/util/virpci.h | 13 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemuxml2argvdata/disk-virtio-s390-zpci.args | 27 ++ tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml | 17 + .../hostdev-vfio-zpci-autogenerate.args | 26 ++ .../hostdev-vfio-zpci-autogenerate.xml | 18 + .../hostdev-vfio-zpci-boundaries.args | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 26 ++ .../hostdev-vfio-zpci-multidomain-many.args | 40 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 67 ++++ tests/qemuxml2argvdata/hostdev-vfio-zpci.args | 26 ++ tests/qemuxml2argvdata/hostdev-vfio-zpci.xml | 19 ++ tests/qemuxml2argvtest.c | 17 + tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml | 29 ++ .../hostdev-vfio-zpci-autogenerate.xml | 30 ++ .../hostdev-vfio-zpci-boundaries.xml | 42 +++ .../hostdev-vfio-zpci-multidomain-many.xml | 79 +++++ tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml | 30 ++ tests/qemuxml2xmltest.c | 14 + 41 files changed, 1575 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-s390-zpci.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-boundaries.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-multidomain-many.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci.xml

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
Abstract ======== The PCI representation in QEMU has recently been extended for S390 allowing configuration of zPCI attributes like uid (user-defined identifier) and fid (PCI function identifier). The details can be found here: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html
To support the new zPCI feature of the S390 platform, two new XML attributes, @uid and @fid, are introduced for device addresses of type 'pci', i.e.: <hostdev mode='subsystem' type='pci'> <driver name='vfio'/> <source> <address domain='0x0001' bus='0x00' slot='0x00' function='0x0'/> </source> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' uid='0x0003' fid='0x00000027'/> </hostdev>
Overall thoughts on the series: the direction is good and the design looks solid, but there are still a few questions regarding the implementation that we'll need to figure out. We also need someone experienced in the area to look over the hotplug part, and I would feel way more comfortable merging this if Laine could go over the changes too... Hopefully he'll find time to do that when v3 rolls around :) -- Andrea Bolognani / Red Hat / Virtualization
participants (5)
-
Andrea Bolognani
-
Bjoern Walk
-
Boris Fiuczynski
-
Cornelia Huck
-
Yi Min Zhao