src/conf/device_conf.c | 2 +- src/conf/device_conf.h | 2 +- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain_address.c | 2 +- src/qemu/qemu_parse_command.c | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-)
spapr-vio addresses are used on POWER platform qemu guests, which are based
on the PAPR specification. PAPR specifies a number of virtual devices (but
not virtio protocol) which are addressed in an abstract namespace.
Currently, libvirt encodes these addresses as 64-bit values. This is not
correct: spapr-vio addresses are, and always have been 32-bit. That's true
both by the PAPR specification and the qemu implementation.
Therefore, change this in libvirt.
This looks like it would be a breaking change, but it actually isn't.
Because these have always been 32-bit at the lower levels, any attempt to
use a value here > 0xffffffff would always have failed in any case, this
will just make it fail earlier and more clearly.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
src/conf/device_conf.c | 2 +-
src/conf/device_conf.h | 2 +-
src/conf/domain_conf.c | 2 +-
src/qemu/qemu_command.c | 4 ++--
src/qemu/qemu_domain_address.c | 2 +-
src/qemu/qemu_parse_command.c | 4 ++--
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 2f82bdc2a7..42f83b0344 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -582,7 +582,7 @@ virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node,
reg = virXMLPropString(node, "reg");
if (reg) {
- if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) {
+ if (virStrToLong_ul(reg, NULL, 16, &addr->reg) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot parse <address> 'reg' attribute"));
ret = -1;
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index b3299ac69d..02cf8c70ad 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -101,7 +101,7 @@ struct _virDomainDeviceUSBAddress {
typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress;
typedef virDomainDeviceSpaprVioAddress *virDomainDeviceSpaprVioAddressPtr;
struct _virDomainDeviceSpaprVioAddress {
- unsigned long long reg;
+ unsigned long reg;
bool has_reg;
};
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 97ba8bd53a..02e58dc54d 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%lx'", info->addr.spaprvio.reg);
break;
case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 59dc134785..fab622f533 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%lx", 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",
@@ -4332,7 +4332,7 @@ qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev)
if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO &&
dev->info.addr.spaprvio.has_reg) {
- virBufferAsprintf(&buf, "spapr-nvram.reg=0x%llx",
+ virBufferAsprintf(&buf, "spapr-nvram.reg=0x%lx",
dev->info.addr.spaprvio.reg);
} else {
virReportError(VIR_ERR_XML_ERROR, "%s",
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 4b99e8ca93..19562c9311 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -204,7 +204,7 @@ qemuDomainAssignSpaprVIOAddress(virDomainDefPtr def,
while (ret != 0) {
if (user_reg) {
virReportError(VIR_ERR_XML_ERROR,
- _("spapr-vio address %#llx already in use"),
+ _("spapr-vio address %#lx already in use"),
info->addr.spaprvio.reg);
return -EEXIST;
}
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index fc3f70fcde..35d1df8ceb 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -2549,8 +2549,8 @@ qemuParseCommandLine(virFileCachePtr capsCache,
def->nvram->info.addr.spaprvio.has_reg = true;
val += strlen("spapr-nvram.reg=");
- if (virStrToLong_ull(val, NULL, 16,
- &def->nvram->info.addr.spaprvio.reg) < 0) {
+ if (virStrToLong_ul(val, NULL, 16,
+ &def->nvram->info.addr.spaprvio.reg) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot parse nvram's address '%s'"), val);
goto error;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-06-04 at 11:38 +1000, David Gibson wrote: > spapr-vio addresses are used on POWER platform qemu guests, which are based > on the PAPR specification. PAPR specifies a number of virtual devices (but > not virtio protocol) which are addressed in an abstract namespace. > > Currently, libvirt encodes these addresses as 64-bit values. This is not > correct: spapr-vio addresses are, and always have been 32-bit. That's true > both by the PAPR specification and the qemu implementation. > > Therefore, change this in libvirt. > > This looks like it would be a breaking change, but it actually isn't. > Because these have always been 32-bit at the lower levels, any attempt to > use a value here > 0xffffffff would always have failed in any case, this > will just make it fail earlier and more clearly. Thanks for providing this patch, and sorry for taking a while to get back to you about it. Unfortunately there's one major issue with your approach: even though it's true that a spapr-vio address that can't be represented as a 32-bit value would always have been rejected by QEMU and so the guest would never have been able to start, refusing to parse the value altogether would cause such a guest to disappear completely from libvirt. We don't consider this to be acceptable, because we want to give users a chance to fix their guests that doesn't involve poking at the filesystem behind libvirt's back. I have posted an alternative implementation: https://www.redhat.com/archives/libvir-list/2019-June/msg00393.html It addresses the issue mentioned above by validating the value after parsing it, so that users will be able to use 'virsh edit' or whatever other libvirt-mediated means to fix invalid configurations. In addition to that, it also updates the schema and documentation to match, and expands the test suite so that we can be sure we won't regress in the future. I even threw in a couple of cosmetic patches while at it :) I hope you don't mind. I'd appreciate any feedback you might want to provide; in the meantime, thanks for pushing me into finally looking into this long-ignored issue. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 14, 2019 at 01:45:24PM +0200, Andrea Bolognani wrote: > On Tue, 2019-06-04 at 11:38 +1000, David Gibson wrote: > > spapr-vio addresses are used on POWER platform qemu guests, which are based > > on the PAPR specification. PAPR specifies a number of virtual devices (but > > not virtio protocol) which are addressed in an abstract namespace. > > > > Currently, libvirt encodes these addresses as 64-bit values. This is not > > correct: spapr-vio addresses are, and always have been 32-bit. That's true > > both by the PAPR specification and the qemu implementation. > > > > Therefore, change this in libvirt. > > > > This looks like it would be a breaking change, but it actually isn't. > > Because these have always been 32-bit at the lower levels, any attempt to > > use a value here > 0xffffffff would always have failed in any case, this > > will just make it fail earlier and more clearly. > > Thanks for providing this patch, and sorry for taking a while to get > back to you about it. > > Unfortunately there's one major issue with your approach: even though > it's true that a spapr-vio address that can't be represented as a > 32-bit value would always have been rejected by QEMU and so the guest > would never have been able to start, refusing to parse the value > altogether would cause such a guest to disappear completely from > libvirt. We don't consider this to be acceptable, because we want to > give users a chance to fix their guests that doesn't involve poking > at the filesystem behind libvirt's back. > > I have posted an alternative implementation: > > https://www.redhat.com/archives/libvir-list/2019-June/msg00393.html > > It addresses the issue mentioned above by validating the value after > parsing it, so that users will be able to use 'virsh edit' or > whatever other libvirt-mediated means to fix invalid configurations. > > In addition to that, it also updates the schema and documentation to > match, and expands the test suite so that we can be sure we won't > regress in the future. I even threw in a couple of cosmetic patches > while at it :) > > I hope you don't mind. I'd appreciate any feedback you might want to > provide; in the meantime, thanks for pushing me into finally looking > into this long-ignored issue. Well, I think libvirt's obsession with maintaining backwards compatibility in even ludicrous circumstances and at all costs continues to make life hard for itself. I seriously can't imagine that anyone, anywhere has ever put a > 32-bit value in here. But, libvirt can do libvirt, I guess, so I'm happy enough to cross it off the list with your fix. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jul 01, 2019 at 10:35:17AM +1000, David Gibson wrote: > On Fri, Jun 14, 2019 at 01:45:24PM +0200, Andrea Bolognani wrote: > > On Tue, 2019-06-04 at 11:38 +1000, David Gibson wrote: > > > spapr-vio addresses are used on POWER platform qemu guests, which are based > > > on the PAPR specification. PAPR specifies a number of virtual devices (but > > > not virtio protocol) which are addressed in an abstract namespace. > > > > > > Currently, libvirt encodes these addresses as 64-bit values. This is not > > > correct: spapr-vio addresses are, and always have been 32-bit. That's true > > > both by the PAPR specification and the qemu implementation. > > > > > > Therefore, change this in libvirt. > > > > > > This looks like it would be a breaking change, but it actually isn't. > > > Because these have always been 32-bit at the lower levels, any attempt to > > > use a value here > 0xffffffff would always have failed in any case, this > > > will just make it fail earlier and more clearly. > > > > Thanks for providing this patch, and sorry for taking a while to get > > back to you about it. > > > > Unfortunately there's one major issue with your approach: even though > > it's true that a spapr-vio address that can't be represented as a > > 32-bit value would always have been rejected by QEMU and so the guest > > would never have been able to start, refusing to parse the value > > altogether would cause such a guest to disappear completely from > > libvirt. We don't consider this to be acceptable, because we want to > > give users a chance to fix their guests that doesn't involve poking > > at the filesystem behind libvirt's back. I missed this when it was first posted, but I would have said that we really don't need to consider this scenario. This is a valid thing to worry about *if* the previous config was actually something a person would have used in the past. In this case though there's no way the guest could ever have worked with value > 0xffffffff. At the very most they could have a defined XML config that they might have tried & failed to use. We would have been justified in just changing the parser to use a 32-bit int straight away. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2019-07-01 at 09:50 +0100, Daniel P. Berrangé wrote: > On Mon, Jul 01, 2019 at 10:35:17AM +1000, David Gibson wrote: > > On Fri, Jun 14, 2019 at 01:45:24PM +0200, Andrea Bolognani wrote: > > > Unfortunately there's one major issue with your approach: even though > > > it's true that a spapr-vio address that can't be represented as a > > > 32-bit value would always have been rejected by QEMU and so the guest > > > would never have been able to start, refusing to parse the value > > > altogether would cause such a guest to disappear completely from > > > libvirt. We don't consider this to be acceptable, because we want to > > > give users a chance to fix their guests that doesn't involve poking > > > at the filesystem behind libvirt's back. > > I missed this when it was first posted, but I would have said that we really > don't need to consider this scenario. This is a valid thing to worry about > *if* the previous config was actually something a person would have used in > the past. In this case though there's no way the guest could ever have > worked with value > 0xffffffff. At the very most they could have a defined > XML config that they might have tried & failed to use. We would have been > justified in just changing the parser to use a 32-bit int straight away. In the past we've bent over backwards to keep guest configurations from failing to parse, a good example being network interface models that never existed in QEMU, and I believe Cole's recent changes in that area made sure such configurations can still be parsed and only fail to validate for this very reason. How is this situation different? What did I miss? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jul 01, 2019 at 11:02:55AM +0200, Andrea Bolognani wrote: > On Mon, 2019-07-01 at 09:50 +0100, Daniel P. Berrangé wrote: > > On Mon, Jul 01, 2019 at 10:35:17AM +1000, David Gibson wrote: > > > On Fri, Jun 14, 2019 at 01:45:24PM +0200, Andrea Bolognani wrote: > > > > Unfortunately there's one major issue with your approach: even though > > > > it's true that a spapr-vio address that can't be represented as a > > > > 32-bit value would always have been rejected by QEMU and so the guest > > > > would never have been able to start, refusing to parse the value > > > > altogether would cause such a guest to disappear completely from > > > > libvirt. We don't consider this to be acceptable, because we want to > > > > give users a chance to fix their guests that doesn't involve poking > > > > at the filesystem behind libvirt's back. > > > > I missed this when it was first posted, but I would have said that we really > > don't need to consider this scenario. This is a valid thing to worry about > > *if* the previous config was actually something a person would have used in > > the past. In this case though there's no way the guest could ever have > > worked with value > 0xffffffff. At the very most they could have a defined > > XML config that they might have tried & failed to use. We would have been > > justified in just changing the parser to use a 32-bit int straight away. > > In the past we've bent over backwards to keep guest configurations > from failing to parse, a good example being network interface models > that never existed in QEMU, and I believe Cole's recent changes in > that area made sure such configurations can still be parsed and only > fail to validate for this very reason. The reason we were careful with network models is that we did not have confidence that we have listed all known NIC models in our enum, most especially for non-x86 archs where QEMU has alot of NIC models. Thus there was a reasonable chance we would break an existing valid working guest configuration & thus we needed to be particularly carefult not to cause a regression. > How is this situation different? What did I miss? There is no way this guest configuration could ever have worked. It was always an error before the proposed change, so we would not be breaking anything that was ever expected to work. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.