[libvirt] [PATCH 0/7] Validate spapr-vio addresses as 32-bit
An alternative take on David Gibson's patch https://www.redhat.com/archives/libvir-list/2019-June/msg00061.html from earlier this month. Andrea Bolognani (7): docs: Fix validation of spapr-vio addresses qemu: Rework qemuDomainDeviceDefValidateAddress() qemu: Validate spapr-vio addresses tests: Add pseries-spaprvio-invalid conf: Format spapr-vio addresses as 32-bit qemu: Format spapr-vio addresses as 32-bit docs: Update documentation for spapr-vio addresses docs/formatdomain.html.in | 6 +-- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 41 ++++++++++++++++++- tests/qemuargv2xmldata/nomachine-ppc64.xml | 2 +- tests/qemuargv2xmldata/pseries-disk.xml | 2 +- tests/qemuargv2xmldata/pseries-nvram.xml | 2 +- .../disk-scsi.x86_64-latest.args | 2 +- .../pseries-spaprvio-invalid.xml | 17 ++++++++ .../pseries-vio-user-assigned.args | 2 +- tests/qemuxml2argvdata/pseries-vio.args | 4 +- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/disk-scsi.xml | 2 +- tests/qemuxml2xmloutdata/pseries-nvram.xml | 2 +- 15 files changed, 73 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/pseries-spaprvio-invalid.xml -- 2.21.0
According to sPAPR, addresses are 32-bit (8 hex digits) rather than 64-bit (16 hex digits). Update the schema accordingly. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/schemas/domaincommon.rng | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4bd75e3055..9fd23efb47 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6386,7 +6386,7 @@ </define> <define name="spaprvioReg"> <data type="string"> - <param name="pattern">(0x)?[0-9a-fA-F]{1,16}</param> + <param name="pattern">(0x)?[0-9a-fA-F]{1,8}</param> </data> </define> <define name='aliasName'> -- 2.21.0
Introduce a switch() statement and prepare for validating more address types than just PCI. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e5c6ef3fda..76963d1c5a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6193,9 +6193,35 @@ qemuDomainDeviceDefValidateAddress(const virDomainDeviceDef *dev, if (!(info = virDomainDeviceGetInfo((virDomainDeviceDef *)dev))) return 0; - if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + switch ((virDomainDeviceAddressType) info->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: return qemuDomainDeviceDefValidateZPCIAddress(info, qemuCaps); + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + /* Address validation might happen before we have had a chance to + * automatically assign addresses to devices for which the user + * didn't specify one themselves */ + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM: + /* No validation for these address types yet */ + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainDeviceAddressType, info->type); + return -1; + } + return 0; } -- 2.21.0
According to sPAPR, addresses are 32-bit rather than 64-bit. Update qemuDomainDeviceDefValidateAddress() accordingly. https://bugzilla.redhat.com/show_bug.cgi?id=1598657 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 76963d1c5a..abc8809b56 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6203,11 +6203,24 @@ qemuDomainDeviceDefValidateAddress(const virDomainDeviceDef *dev, * didn't specify one themselves */ break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: { + virDomainDeviceSpaprVioAddressPtr addr = &(info->addr.spaprvio); + + if (addr->has_reg && addr->reg > 0xffffffff) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("spapr-vio reg='0x%llx' exceeds maximum " + "possible value (0xffffffff)"), + addr->reg); + return -1; + } + + break; + } + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: -- 2.21.0
This test case shows that we now reject invalid spapr-vio addresses. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../pseries-spaprvio-invalid.xml | 17 +++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 2 files changed, 18 insertions(+) create mode 100644 tests/qemuxml2argvdata/pseries-spaprvio-invalid.xml diff --git a/tests/qemuxml2argvdata/pseries-spaprvio-invalid.xml b/tests/qemuxml2argvdata/pseries-spaprvio-invalid.xml new file mode 100644 index 0000000000..cd658e314b --- /dev/null +++ b/tests/qemuxml2argvdata/pseries-spaprvio-invalid.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>b35969f7-e7cf-4d90-a9a0-4dd9000f9824</uuid> + <memory unit='KiB'>4194304</memory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='ppc64le' machine='pseries'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <serial type='pty'> + <!-- spapr-vio addresses are 32-bit, so they can go up to 0xffffffff: + the value below is too big and should be rejected --> + <address type='spapr-vio' reg='0x0000000100000000'/> + </serial> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 49220733ae..6a75d663c5 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1898,6 +1898,7 @@ mymain(void) DO_TEST("pseries-console-virtio", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE); DO_TEST_PARSE_ERROR("pseries-serial-invalid-machine", NONE); + DO_TEST_PARSE_ERROR("pseries-spaprvio-invalid", "ppc64"); DO_TEST("mach-virt-serial-native", QEMU_CAPS_DEVICE_PL011); -- 2.21.0
Using 8 hex digits all the time, regardless of whether the actual value can fit in fewer, makes it more obvious to the user what the limits are. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 2 +- tests/qemuargv2xmldata/nomachine-ppc64.xml | 2 +- tests/qemuargv2xmldata/pseries-disk.xml | 2 +- tests/qemuargv2xmldata/pseries-nvram.xml | 2 +- tests/qemuxml2xmloutdata/disk-scsi.xml | 2 +- tests/qemuxml2xmloutdata/pseries-nvram.xml | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 97ba8bd53a..54a24b6276 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7135,7 +7135,7 @@ virDomainDeviceInfoFormat(virBufferPtr buf, case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: if (info->addr.spaprvio.has_reg) - virBufferAsprintf(&attrBuf, " reg='0x%llx'", info->addr.spaprvio.reg); + virBufferAsprintf(&attrBuf, " reg='0x%08llx'", info->addr.spaprvio.reg); break; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: diff --git a/tests/qemuargv2xmldata/nomachine-ppc64.xml b/tests/qemuargv2xmldata/nomachine-ppc64.xml index 18b0c5c20a..f61006efec 100644 --- a/tests/qemuargv2xmldata/nomachine-ppc64.xml +++ b/tests/qemuargv2xmldata/nomachine-ppc64.xml @@ -35,7 +35,7 @@ <target index='0'/> </controller> <controller type='scsi' index='0' model='ibmvscsi'> - <address type='spapr-vio' reg='0x2000'/> + <address type='spapr-vio' reg='0x00002000'/> </controller> <input type='keyboard' bus='usb'/> <input type='mouse' bus='usb'/> diff --git a/tests/qemuargv2xmldata/pseries-disk.xml b/tests/qemuargv2xmldata/pseries-disk.xml index 18b0c5c20a..f61006efec 100644 --- a/tests/qemuargv2xmldata/pseries-disk.xml +++ b/tests/qemuargv2xmldata/pseries-disk.xml @@ -35,7 +35,7 @@ <target index='0'/> </controller> <controller type='scsi' index='0' model='ibmvscsi'> - <address type='spapr-vio' reg='0x2000'/> + <address type='spapr-vio' reg='0x00002000'/> </controller> <input type='keyboard' bus='usb'/> <input type='mouse' bus='usb'/> diff --git a/tests/qemuargv2xmldata/pseries-nvram.xml b/tests/qemuargv2xmldata/pseries-nvram.xml index 500227afc0..78f79f17d7 100644 --- a/tests/qemuargv2xmldata/pseries-nvram.xml +++ b/tests/qemuargv2xmldata/pseries-nvram.xml @@ -23,7 +23,7 @@ </controller> <memballoon model='none'/> <nvram> - <address type='spapr-vio' reg='0x4000'/> + <address type='spapr-vio' reg='0x00004000'/> </nvram> <panic model='pseries'/> </devices> diff --git a/tests/qemuxml2xmloutdata/disk-scsi.xml b/tests/qemuxml2xmloutdata/disk-scsi.xml index 9cfff4b14e..45fed6423a 100644 --- a/tests/qemuxml2xmloutdata/disk-scsi.xml +++ b/tests/qemuxml2xmloutdata/disk-scsi.xml @@ -62,7 +62,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </controller> <controller type='scsi' index='3' model='ibmvscsi'> - <address type='spapr-vio' reg='0x2000'/> + <address type='spapr-vio' reg='0x00002000'/> </controller> <controller type='pci' index='0' model='pci-root'/> <input type='mouse' bus='ps2'/> diff --git a/tests/qemuxml2xmloutdata/pseries-nvram.xml b/tests/qemuxml2xmloutdata/pseries-nvram.xml index f89b23b338..fd77c7aa89 100644 --- a/tests/qemuxml2xmloutdata/pseries-nvram.xml +++ b/tests/qemuxml2xmloutdata/pseries-nvram.xml @@ -23,7 +23,7 @@ </controller> <memballoon model='none'/> <nvram> - <address type='spapr-vio' reg='0x4000'/> + <address type='spapr-vio' reg='0x00004000'/> </nvram> <panic model='pseries'/> </devices> -- 2.21.0
No reason not to be consistent with the user-visible value. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args | 2 +- tests/qemuxml2argvdata/pseries-vio-user-assigned.args | 2 +- tests/qemuxml2argvdata/pseries-vio.args | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 59dc134785..9d4d0b8fc2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -395,7 +395,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, } } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { if (info->addr.spaprvio.has_reg) - virBufferAsprintf(buf, ",reg=0x%llx", info->addr.spaprvio.reg); + virBufferAsprintf(buf, ",reg=0x%08llx", info->addr.spaprvio.reg); } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { if (info->addr.ccw.assigned) virBufferAsprintf(buf, ",devno=%x.%x.%04x", diff --git a/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args b/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args index 6c2618ed61..7bf011fd5f 100644 --- a/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args +++ b/tests/qemuxml2argvdata/disk-scsi.x86_64-latest.args @@ -30,7 +30,7 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -device lsi,id=scsi0,bus=pci.0,addr=0x2 \ -device megasas,id=scsi1,bus=pci.0,addr=0x3 \ -device mptsas1068,id=scsi2,bus=pci.0,addr=0x4 \ --device spapr-vscsi,id=scsi3,reg=0x2000 \ +-device spapr-vscsi,id=scsi3,reg=0x00002000 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ -drive file=/tmp/scsidisk.img,format=raw,if=none,id=drive-scsi0-0-0 \ diff --git a/tests/qemuxml2argvdata/pseries-vio-user-assigned.args b/tests/qemuxml2argvdata/pseries-vio-user-assigned.args index 534037607a..a412593c9a 100644 --- a/tests/qemuxml2argvdata/pseries-vio-user-assigned.args +++ b/tests/qemuxml2argvdata/pseries-vio-user-assigned.args @@ -23,7 +23,7 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ --device spapr-vscsi,id=scsi0,reg=0x2000 \ +-device spapr-vscsi,id=scsi0,reg=0x00002000 \ -device spapr-vscsi,id=scsi1,reg=0x30000000 \ -usb \ -drive file=/tmp/scsidisk.img,format=raw,if=none,id=drive-scsi1-0-0-0 \ diff --git a/tests/qemuxml2argvdata/pseries-vio.args b/tests/qemuxml2argvdata/pseries-vio.args index 9d4f77b85e..a500e9b4b9 100644 --- a/tests/qemuxml2argvdata/pseries-vio.args +++ b/tests/qemuxml2argvdata/pseries-vio.args @@ -23,8 +23,8 @@ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ --device spapr-vscsi,id=scsi0,reg=0x2000 \ --device spapr-vscsi,id=scsi1,reg=0x3000 \ +-device spapr-vscsi,id=scsi0,reg=0x00002000 \ +-device spapr-vscsi,id=scsi1,reg=0x00003000 \ -usb \ -drive file=/tmp/scsidisk.img,format=raw,if=none,id=drive-scsi1-0-0-0 \ -device scsi-hd,bus=scsi1.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi1-0-0-0,\ -- 2.21.0
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 22ddcb71d3..144baa2e85 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4076,9 +4076,9 @@ </dd> <dt><code>spapr-vio</code></dt> <dd>On PowerPC pseries guests, devices can be assigned to the - SPAPR-VIO bus. It has a flat 64-bit address space; by + SPAPR-VIO bus. It has a flat 32-bit address space; by convention, devices are generally assigned at a non-zero - multiple of 0x1000, but other addresses are valid and + multiple of 0x00001000, but other addresses are valid and permitted by libvirt. Each address has the following additional attribute: <code>reg</code> (the hex value address of the starting register). <span class="since">Since @@ -8291,7 +8291,7 @@ qemu-kvm -net nic,model=? /dev/null ... <devices> <nvram> - <address type='spapr-vio' reg='0x3000'/> + <address type='spapr-vio' reg='0x00003000'/> </nvram> </devices> ... -- 2.21.0
On 6/14/19 1:23 PM, Andrea Bolognani wrote:
An alternative take on David Gibson's patch
https://www.redhat.com/archives/libvir-list/2019-June/msg00061.html
from earlier this month.
Andrea Bolognani (7): docs: Fix validation of spapr-vio addresses qemu: Rework qemuDomainDeviceDefValidateAddress() qemu: Validate spapr-vio addresses tests: Add pseries-spaprvio-invalid conf: Format spapr-vio addresses as 32-bit qemu: Format spapr-vio addresses as 32-bit docs: Update documentation for spapr-vio addresses
docs/formatdomain.html.in | 6 +-- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 41 ++++++++++++++++++- tests/qemuargv2xmldata/nomachine-ppc64.xml | 2 +- tests/qemuargv2xmldata/pseries-disk.xml | 2 +- tests/qemuargv2xmldata/pseries-nvram.xml | 2 +- .../disk-scsi.x86_64-latest.args | 2 +- .../pseries-spaprvio-invalid.xml | 17 ++++++++ .../pseries-vio-user-assigned.args | 2 +- tests/qemuxml2argvdata/pseries-vio.args | 4 +- tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/disk-scsi.xml | 2 +- tests/qemuxml2xmloutdata/pseries-nvram.xml | 2 +- 15 files changed, 73 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/pseries-spaprvio-invalid.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Andrea Bolognani -
Michal Privoznik