* [PATCH 2/2] kvm: qemu: check device assignment command @ 2009-03-26 9:02 Han, Weidong 2009-03-26 12:04 ` Avi Kivity 0 siblings, 1 reply; 6+ messages in thread From: Han, Weidong @ 2009-03-26 9:02 UTC (permalink / raw) To: 'Avi Kivity'; +Cc: 'kvm@vger.kernel.org' [-- Attachment #1: Type: text/plain, Size: 1071 bytes --] Device assignment command is like "-pcidevice host=xx:yy.z". Check bus:dev.func length to make sure its format is xx:yy.z. Signed-off-by: Weidong Han <weidong.han@intel.com> --- qemu/hw/device-assignment.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c index cef7c8a..50a0d5c 100644 --- a/qemu/hw/device-assignment.c +++ b/qemu/hw/device-assignment.c @@ -1196,7 +1196,7 @@ out: AssignedDevInfo *add_assigned_device(const char *arg) { char *cp, *cp1; - char device[8]; + char device[9]; char dma[6]; int r; AssignedDevInfo *adev; @@ -1207,6 +1207,9 @@ AssignedDevInfo *add_assigned_device(const char *arg) return NULL; } r = get_param_value(device, sizeof(device), "host", arg); + /* b:d.f format: xx:yy.z */ + if (r != 7) + goto bad; r = get_param_value(adev->name, sizeof(adev->name), "name", arg); if (!r) snprintf(adev->name, sizeof(adev->name), "%s", device); -- 1.6.0.4 [-- Attachment #2: 0002-kvm-qemu-check-device-assignment-command.patch --] [-- Type: application/octet-stream, Size: 1251 bytes --] From 7ce0e9784ec2d9743fce28537c3369d924b33a90 Mon Sep 17 00:00:00 2001 From: Weidong Han <weidong.han@intel.com> Date: Fri, 27 Mar 2009 00:53:06 +0800 Subject: [PATCH] kvm: qemu: check device assignment command Device assignment command is like "-pcidevice host=xx:yy.z". Check bus:dev.func length to make sure its format is xx:yy.z. Signed-off-by: Weidong Han <weidong.han@intel.com> --- qemu/hw/device-assignment.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c index cef7c8a..50a0d5c 100644 --- a/qemu/hw/device-assignment.c +++ b/qemu/hw/device-assignment.c @@ -1196,7 +1196,7 @@ out: AssignedDevInfo *add_assigned_device(const char *arg) { char *cp, *cp1; - char device[8]; + char device[9]; char dma[6]; int r; AssignedDevInfo *adev; @@ -1207,6 +1207,9 @@ AssignedDevInfo *add_assigned_device(const char *arg) return NULL; } r = get_param_value(device, sizeof(device), "host", arg); + /* b:d.f format: xx:yy.z */ + if (r != 7) + goto bad; r = get_param_value(adev->name, sizeof(adev->name), "name", arg); if (!r) snprintf(adev->name, sizeof(adev->name), "%s", device); -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] kvm: qemu: check device assignment command 2009-03-26 9:02 [PATCH 2/2] kvm: qemu: check device assignment command Han, Weidong @ 2009-03-26 12:04 ` Avi Kivity 2009-03-27 9:31 ` Han, Weidong 0 siblings, 1 reply; 6+ messages in thread From: Avi Kivity @ 2009-03-26 12:04 UTC (permalink / raw) To: Han, Weidong; +Cc: 'kvm@vger.kernel.org' Han, Weidong wrote: > Device assignment command is like "-pcidevice host=xx:yy.z". > Check bus:dev.func length to make sure its format is xx:yy.z. > > Signed-off-by: Weidong Han <weidong.han@intel.com> > --- > qemu/hw/device-assignment.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c > index cef7c8a..50a0d5c 100644 > --- a/qemu/hw/device-assignment.c > +++ b/qemu/hw/device-assignment.c > @@ -1196,7 +1196,7 @@ out: > AssignedDevInfo *add_assigned_device(const char *arg) > { > char *cp, *cp1; > - char device[8]; > + char device[9]; > char dma[6]; > int r; > AssignedDevInfo *adev; > @@ -1207,6 +1207,9 @@ AssignedDevInfo *add_assigned_device(const char *arg) > return NULL; > } > r = get_param_value(device, sizeof(device), "host", arg); > + /* b:d.f format: xx:yy.z */ > + if (r != 7) > + goto bad; > r = get_param_value(adev->name, sizeof(adev->name), "name", arg); > if (!r) > snprintf(adev->name, sizeof(adev->name), "%s", device); > I suggest replacing the parsing code with pci_parse_devaddr() (needs to be extended to support functions) so that all the checking and parsing is done in one place. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] kvm: qemu: check device assignment command 2009-03-26 12:04 ` Avi Kivity @ 2009-03-27 9:31 ` Han, Weidong 2009-03-29 14:35 ` Avi Kivity 0 siblings, 1 reply; 6+ messages in thread From: Han, Weidong @ 2009-03-27 9:31 UTC (permalink / raw) To: 'Avi Kivity'; +Cc: 'kvm@vger.kernel.org' Avi Kivity wrote: > Han, Weidong wrote: >> Device assignment command is like "-pcidevice host=xx:yy.z". >> Check bus:dev.func length to make sure its format is xx:yy.z. >> >> Signed-off-by: Weidong Han <weidong.han@intel.com> >> --- >> qemu/hw/device-assignment.c | 5 ++++- >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/qemu/hw/device-assignment.c >> b/qemu/hw/device-assignment.c index cef7c8a..50a0d5c 100644 --- >> a/qemu/hw/device-assignment.c +++ b/qemu/hw/device-assignment.c >> @@ -1196,7 +1196,7 @@ out: >> AssignedDevInfo *add_assigned_device(const char *arg) { >> char *cp, *cp1; >> - char device[8]; >> + char device[9]; >> char dma[6]; >> int r; >> AssignedDevInfo *adev; >> @@ -1207,6 +1207,9 @@ AssignedDevInfo *add_assigned_device(const >> char *arg) return NULL; } >> r = get_param_value(device, sizeof(device), "host", arg); >> + /* b:d.f format: xx:yy.z */ >> + if (r != 7) >> + goto bad; >> r = get_param_value(adev->name, sizeof(adev->name), "name", >> arg); if (!r) snprintf(adev->name, sizeof(adev->name), "%s", >> device); >> > > I suggest replacing the parsing code with pci_parse_devaddr() (needs > to be extended to support functions) so that all the checking and > parsing is done in one place. If use pci_parse_devaddr(), it needs to add domain section to assigning command, and add function section to pci_add/pci_del commands. What's more, pci_parse_devaddr() parses guest device bdf, there are some assumption, such as function is 0. But here parse host bdf. It's a little complex to combine them together. Regards, Weidong ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] kvm: qemu: check device assignment command 2009-03-27 9:31 ` Han, Weidong @ 2009-03-29 14:35 ` Avi Kivity 2009-03-30 12:41 ` Han, Weidong 0 siblings, 1 reply; 6+ messages in thread From: Avi Kivity @ 2009-03-29 14:35 UTC (permalink / raw) To: Han, Weidong; +Cc: 'kvm@vger.kernel.org' Han, Weidong wrote: >> I suggest replacing the parsing code with pci_parse_devaddr() (needs >> to be extended to support functions) so that all the checking and >> parsing is done in one place. >> > > If use pci_parse_devaddr(), it needs to add domain section to assigning command, and add function section to pci_add/pci_del commands. What's more, pci_parse_devaddr() parses guest device bdf, there are some assumption, such as function is 0. But here parse host bdf. It's a little complex to combine them together. > Right, but we end up with overall better code. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] kvm: qemu: check device assignment command 2009-03-29 14:35 ` Avi Kivity @ 2009-03-30 12:41 ` Han, Weidong 2009-04-01 9:18 ` Avi Kivity 0 siblings, 1 reply; 6+ messages in thread From: Han, Weidong @ 2009-03-30 12:41 UTC (permalink / raw) To: 'Avi Kivity'; +Cc: 'kvm@vger.kernel.org' Avi Kivity wrote: > Han, Weidong wrote: >>> I suggest replacing the parsing code with pci_parse_devaddr() (needs >>> to be extended to support functions) so that all the checking and >>> parsing is done in one place. >>> >> >> If use pci_parse_devaddr(), it needs to add domain section to >> assigning command, and add function section to pci_add/pci_del >> commands. What's more, pci_parse_devaddr() parses guest device bdf, >> there are some assumption, such as function is 0. But here parse >> host bdf. It's a little complex to combine them together. >> > > Right, but we end up with overall better code. pci_parse_devaddr parses [[<domain>:][<bus>:]<slot>, it's valid when even enter only slot, whereas it must be bus:slot.func in device assignment command (-pcidevice host=bus:slot.func). So I implemented a dedicated function to parse device bdf in device assignment command, rather than mix two parsing function together. Signed-off-by: Weidong Han <weidong.han@intel.com> diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c index cef7c8a..53375ff 100644 --- a/qemu/hw/device-assignment.c +++ b/qemu/hw/device-assignment.c @@ -1195,8 +1195,7 @@ out: */ AssignedDevInfo *add_assigned_device(const char *arg) { - char *cp, *cp1; - char device[8]; + char device[16]; char dma[6]; int r; AssignedDevInfo *adev; @@ -1207,6 +1206,13 @@ AssignedDevInfo *add_assigned_device(const char *arg) return NULL; } r = get_param_value(device, sizeof(device), "host", arg); + if (!r) + goto bad; + + r = pci_parse_host_devaddr(device, &adev->bus, &adev->dev, &adev->func); + if (r) + goto bad; + r = get_param_value(adev->name, sizeof(adev->name), "name", arg); if (!r) snprintf(adev->name, sizeof(adev->name), "%s", device); @@ -1216,18 +1222,6 @@ AssignedDevInfo *add_assigned_device(const char *arg) if (r && !strncmp(dma, "none", 4)) adev->disable_iommu = 1; #endif - cp = device; - adev->bus = strtoul(cp, &cp1, 16); - if (*cp1 != ':') - goto bad; - cp = cp1 + 1; - - adev->dev = strtoul(cp, &cp1, 16); - if (*cp1 != '.') - goto bad; - cp = cp1 + 1; - - adev->func = strtoul(cp, &cp1, 16); LIST_INSERT_HEAD(&adev_head, adev, next); return adev; diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c index eca0517..bf97c8c 100644 --- a/qemu/hw/pci.c +++ b/qemu/hw/pci.c @@ -163,6 +163,7 @@ static int pci_set_default_subsystem_id(PCIDevice *pci_dev) } /* + * Parse pci address in qemu command * Parse [[<domain>:]<bus>:]<slot>, return -1 on error */ static int pci_parse_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp) @@ -211,6 +212,55 @@ static int pci_parse_devaddr(const char *addr, int *domp, int *busp, unsigned *s return 0; } +/* + * Parse device bdf in device assignment command: + * + * -pcidevice host=bus:dev.func + * + * Parse <bus>:<slot>.<func> return -1 on error + */ +int pci_parse_host_devaddr(const char *addr, int *busp, + int *slotp, int *funcp) +{ + const char *p; + char *e; + int val; + int bus = 0, slot = 0, func = 0; + + p = addr; + val = strtoul(p, &e, 16); + if (e == p) + return -1; + if (*e == ':') { + bus = val; + p = e + 1; + val = strtoul(p, &e, 16); + if (e == p) + return -1; + if (*e == '.') { + slot = val; + p = e + 1; + val = strtoul(p, &e, 16); + if (e == p) + return -1; + func = val; + } else + return -1; + } else + return -1; + + if (bus > 0xff || slot > 0x1f || func > 0x7) + return -1; + + if (*e) + return -1; + + *busp = bus; + *slotp = slot; + *funcp = func; + return 0; +} + int pci_read_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp) { char devaddr[32]; diff --git a/qemu/hw/pci.h b/qemu/hw/pci.h index a7438f2..bfdd29a 100644 --- a/qemu/hw/pci.h +++ b/qemu/hw/pci.h @@ -227,6 +227,9 @@ PCIDevice *pci_find_device(int bus_num, int slot, int function); int pci_read_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp); int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp); +int pci_parse_host_devaddr(const char *addr, int *busp, + int *slotp, int *funcp); + void pci_info(Monitor *mon); PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, pci_map_irq_fn map_irq, const char *name); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] kvm: qemu: check device assignment command 2009-03-30 12:41 ` Han, Weidong @ 2009-04-01 9:18 ` Avi Kivity 0 siblings, 0 replies; 6+ messages in thread From: Avi Kivity @ 2009-04-01 9:18 UTC (permalink / raw) To: Han, Weidong; +Cc: 'kvm@vger.kernel.org' Han, Weidong wrote: > pci_parse_devaddr parses [[<domain>:][<bus>:]<slot>, it's valid when even enter only slot, whereas it must be bus:slot.func in device assignment command (-pcidevice host=bus:slot.func). So I implemented a dedicated function to parse device bdf in device assignment command, rather than mix two parsing function together. > > Applied, thanks. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-01 9:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-26 9:02 [PATCH 2/2] kvm: qemu: check device assignment command Han, Weidong 2009-03-26 12:04 ` Avi Kivity 2009-03-27 9:31 ` Han, Weidong 2009-03-29 14:35 ` Avi Kivity 2009-03-30 12:41 ` Han, Weidong 2009-04-01 9:18 ` Avi Kivity
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox