Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
Kevin Wolf kw...@redhat.com writes: Am 04.11.2014 um 10:36 hat Markus Armbruster geschrieben: Kevin Wolf kw...@redhat.com writes: Am 31.10.2014 um 23:45 hat Eric Blake geschrieben: On 10/30/2014 06:49 AM, Markus Armbruster wrote: You either have to prevent *any* writing of the first 2048 bytes (the part that can be examined by a bdrv_probe() method, or your have to prevent writing anything a probe recognizes, or the user has to specify the format explicitly. If you do the former, you're way outside the realm of theoretical. If you do the latter, the degree of theoreticalness depends on the union of the patterns you prevent. Issues: 1. Anthony's method of checking a few known signatures is fragile. The only obviously robust way to test is probing going to find something other than 'raw'? is running the probes. Feasible. 2. Whether the union of patterns qualifies as theoretical for all our targets is not obvious, and whether it'll remain theoretical for all future formats and target machines even less. This one scares me. The proof of concept patch you posted tests whether a write to the first sector would result in the sector matching a _currently known probe_ for the file formats that were compiled in at configure time to the currently running binary. But this is NOT the set of all possible binary formats that may be introduced in the future. By extrapolation, if we pursue write blocking, then the only SAFE way to is to reject ALL writes to the first sector, because we can't know which writes will match some theoretical future probe - but by the time you get to that point, then we no longer match bare metal (rejecting ALL writes to the first sector is ridiculous). There is no absolute security without forbidding raw. Who says that the next format doesn't have its magic in sector 42? Correct. You are right that if an attacker knows which new format supporting backing files we'll have in the next version, and the user uses probed raw despite the warning (which means they are not using libvirt), the attacker can write the header of the new image format now and hope that the user leaves it alone until the update happens at some point in the future. Then the malicious guest can access that one file, but not obtain access to the next one (because the new checks are in place then). I don't think this is a relevant attack vector. It's probably much easier to get the user to run an untrusted image than converting a trusted image into a time bomb and waiting potentially for months or years for it to explode. The threat from using untrusted images with embedded filenames is real. Regardless, I wouldn't discount the (different) threat from guests exploiting future probes so cavalierly. If your host is running a stable distro, its future is easy to know. Even the point of time when it gets upgraded can be predictable. In general, I recommend against leaving security holes in place just because you think nobody could be bothered to exploit them :) Security is not an absolute that cannot be trumped by any other consideration. I'm perfectly happy to consider usability and compatibility issues, as well as implementation complexity. I'm much less willing to accept a come on, nobody is going to try *that* argument. No, I didn't mean to make a come on, nobody is going to try *that* argument. I just can think of more attacks that we won't avoid with either approach. For example, what if another program on the host is exploitable and you just need a file with the right content to attack it? A raw image gives you what you need. Nobody is going to try *that* then, if you're content with allowing this? When I give a raw image to an untrusted guest, it's entire contents becomes untrusted. I should treat it with the same caution as any other untrusted data. This is not the only way to put files of untrusted data on your system. That it is one may be a bit more surprising than for downloading tools, though. We can't stop users from using untrusted data carelessly. What we *can* do is making it easy for users to treat untrusted data with the proper care as far as the software we're producing is concerned. The most obvious and simple way to use images with our software does not treat untrusted data with the proper care. This is also the way our documentation advertises. Worse, exercising proper care *consistently* is hard. I couldn't use raw images securely in all contexts myself, not without a lot more staring at our source code. And even then I wouldn't bet my own money on it. Libvirt has had a hard time doing it, with multiple failures, including recent ones. The only full solution is forbidding raw. We have already agreed that this isn't an option. Now we have to draw a line somewhere. I'm drawing the line around the software we actually maintain. Make
Re: [Qemu-devel] [RFC PATCH] virtio-mmio: support for multiple irqs
On 2014/11/4 17:35, Shannon Zhao wrote: As the current virtio-mmio only support single irq, so some advanced features such as vhost-net with irqfd are not supported. And the net performance is not the best without vhost-net and irqfd supporting. Hi Joel, Peter, Mst, Some virtio-net with virtio-mmio performance data on ARM added as followed: Type of backend bandwith(GBytes/sec) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 Test cmd: ./iperf -c 192.168.0.2 -P 1 -i 10 -p 5001 -f G -t 60 From this test data, irqfd has great improvement (about 30%) on performance. So maybe it's necessary to enable multiple irq support to make vhost-net with virtio-mmio on ARM be able to use irqfd. How do you guys think? Look forward for your feedback. Thanks, Shannon This patch support virtio-mmio to request multiple irqs like virtio-pci. With this patch and qemu assigning multiple irqs for virtio-mmio device, it's ok to use vhost-net with irqfd on arm/arm64.
Re: [Qemu-devel] [PATCH 1/5] pci: introduce PC_PCI_CONFIG_ENABLED()
On Tue, Nov 04, 2014 at 03:41:11PM +0200, Marcel Apfelbaum wrote:
Hi,
On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
This makes code more readable.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/mips/gt64xxx_pci.c | 4 ++--
hw/pci/pci_host.c | 5 +++--
include/hw/pci/pci.h | 2 ++
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 1f2fe5f..a49dbd7 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
if (!(s-regs[GT_PCI0_CMD] 1) (phb-config_reg 0x00fff800))
{
val = bswap32(val);
}
-if (phb-config_reg (1u 31)) {
+if (PC_PCI_CONFIG_ENABLED(phb-config_reg))
I really like readable MACROS instead of magic numbers,
however I have 3 suggestions:
1. PC_PCI_CONFIG_ENABLED is still not really clear, because
of the PC prefix that is too wide in my IMHO (maybe
PCI_HOST_BRIDGE_CONFIG_ENABLED)
when this macro refers only to host bridges.
2. Maybe go the extra mile and let the macro receive
a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge).
Sounds reasonable. I changed it to an inline function and prefixed it
with pci_host_ to follow the convention in pci_host.c.
3. Maybe make it an inline function? Just wondering
pci_data_write(phb-bus, phb-config_reg, val, 4);
}
break;
@@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
val = phb-config_reg;
break;
case GT_PCI0_CFGDATA:
-if (!(phb-config_reg (1 31))) {
+if (!PC_PCI_CONFIG_ENABLED(phb-config_reg)) {
val = 0x;
} else {
val = pci_data_read(phb-bus, phb-config_reg, 4);
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..f2a69ea 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr
addr,
PCIHostState *s = opaque;
PCI_DPRINTF(write addr TARGET_FMT_plx len %d val %x\n,
addr, len, (unsigned)val);
-if (s-config_reg (1u 31))
+if (PC_PCI_CONFIG_ENABLED(s-config_reg)) {
pci_data_write(s-bus, s-config_reg | (addr 3), val, len);
+}
}
static uint64_t pci_host_data_read(void *opaque,
@@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
{
PCIHostState *s = opaque;
uint32_t val;
-if (!(s-config_reg (1U 31))) {
+if (!PC_PCI_CONFIG_ENABLED(s-config_reg)) {
return 0x;
}
val = pci_data_read(s-bus, s-config_reg | (addr 3), len);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c352c7b..3d42d7f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -13,6 +13,8 @@
#include hw/pci/pcie.h
+#define PC_PCI_CONFIG_ENABLED(addr) (addr (1U 31))
Again, maybe move this to hw/pci/pci_host since is specific to host bridges?
Done.
Thanks!
Thanks,
Marcel
+
/* PCI bus */
#define PCI_DEVFN(slot, func) slot) 0x1f) 3) | ((func) 0x07))
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
Jeff Cody jc...@redhat.com writes: On Tue, Nov 04, 2014 at 10:39:36AM +0100, Markus Armbruster wrote: Kevin Wolf kw...@redhat.com writes: Am 30.10.2014 um 13:49 hat Markus Armbruster geschrieben: Kevin Wolf kw...@redhat.com writes: Am 29.10.2014 um 14:54 hat Markus Armbruster geschrieben: Kevin Wolf kw...@redhat.com writes: Instead, let me try once more to sell my old proposal [1] from the thread you mentioned: What if we let the raw driver know that it was probed and then it enables a check that returns -EIO for any write on the first 2k if that write would make the image look like a different format? Attacks the problem where it arises instead of trying to detect the outcome of it, and works in whatever way it is nested in the BDS graph and whatever way is used to address the image file. Kevin [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02548.html Arbitrarily breaking the virtual hardware that way is incompatible, too. It's a bigger no-no for me than changing user interface corner cases or deciding what to do with a file based on its file name extension. It is arbitrarily breaking theoretical cases, while your patch is less arbitrarily breaking common cases. I strongly prefer the former. I challenge theoretical as well as common. For theoretical, see below. As to common: are unrecognizable image names really common? I doubt it. If I'm wrong, I expect users to complain about my warning, and then we can reconsider. As I said in my other mail, I remember seeing BZs with command lines that don't have a file extension. Block devices are affected anyway. Your RFC also misses the extension .raw that I personally use occasionally, so I would get the warning even though the image name isn't unrecognizable at all. And other people probably have other extensions in use that we don't know of. Nobody will ever want to write a qcow2 header into their boot sector for just running a guest: 0: 51 push %cx 1: 46 inc%si 2: 49 dec%cx 3: fb sti This code doesn't make any sense. It's similar for other formats. And if they really have some odd reason to do that, the fix is easy: Either don't use raw, or pass an explicit format=raw. A disk needn't start with a PC-style boot sector. Even on a PC. Not every disk needs to be bootable, or even partitioned. Databases exist that like to work on raw devices. Giving them /dev/sdb instead of a whole-disk partition /dev/sdb1 may not be the smartest thing to do, but it *works* with real hardware, so why not with virtual hardware? Oh, it does. Just use a format that can represent this reliably (and as a compromise, we're going to declare explicitly requested raw as such - again, see my other mail for details). This is opt in safety. I dislike that for the same reason I dislike opt in security. Insecure: we risk guest escaping isolation. Unsafe: we risk virtual hardware breakage. You either have to prevent *any* writing of the first 2048 bytes (the part that can be examined by a bdrv_probe() method, or your have to prevent writing anything a probe recognizes, or the user has to specify the format explicitly. If you do the former, you're way outside the realm of theoretical. Right, that's not an option. If you do the latter, the degree of theoreticalness depends on the union of the patterns you prevent. Issues: 1. Anthony's method of checking a few known signatures is fragile. The only obviously robust way to test is probing going to find something other than 'raw'? is running the probes. Feasible. Yes, this is what I've been talking about. 2. Whether the union of patterns qualifies as theoretical for all our targets is not obvious, and whether it'll remain theoretical for all future formats and target machines even less. At least we don't have examples of it happening yet. And even if it happens to become not theoretical at some point, the only thing that happens is that they need to add format=raw - something that we already know is sure to happen with your approach. Once we get to know of such cases, we can probably even resolve them by improving the probing function of the problematic format. Your proposal improves things from insecure by default to now less insecure, but additionally a bit unsafe. Your proposed remedy for unsafe seems to be if it breaks, you get to keep the pieces, but you may ask us to narrow the conditions for breakage so it wouldn't have broken for you if you had imported the changed version from the future ;) 3. A write access that works just fine in one QEMU version can be rejected by another version that recognizes more formats. Or probes the
Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] spapr: Fix stale HTAB during live migration (TCG)
On 05.11.14 07:17, Samuel Mendoza-Jonas wrote:
If a TCG guest reboots during a running migration HTAB entries are not
marked dirty, and the destination boots with an invalid HTAB.
When a reboot occurs reset the state of HTAB migration, and explicitly
inform the destination of invalid entries.
Signed-off-by: Samuel Mendoza-Jonas sam...@au1.ibm.com
---
hw/ppc/spapr.c | 59
+++---
include/hw/ppc/spapr.h | 1 +
2 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1610c28..9f419e8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -829,26 +829,30 @@ static void spapr_reset_htab(sPAPREnvironment *spapr)
shift = kvmppc_reset_htab(spapr-htab_shift);
+pthread_mutex_lock(spapr-htab_mutex);
if (shift 0) {
/* Kernel handles htab, we don't need to allocate one */
spapr-htab_shift = shift;
kvmppc_kern_htab = true;
/* Tell readers to update their file descriptor */
-pthread_mutex_lock(spapr-htab_mutex);
if (spapr-htab_fd 0) {
spapr-htab_fd_stale = true;
}
-pthread_mutex_unlock(spapr-htab_mutex);
} else {
if (!spapr-htab) {
/* Allocate an htab if we don't yet have one */
spapr-htab = qemu_memalign(HTAB_SIZE(spapr), HTAB_SIZE(spapr));
+} else {
+spapr-htab_mig_full = true;
+spapr-htab_first_pass = true;
+spapr-htab_save_index = 0;
You could just set the dirty bitmap to all dirty here, no? Then you
don't need all the changes belong I presume?
}
/* And clear it */
memset(spapr-htab, 0, HTAB_SIZE(spapr));
... so instead of memset(0)ing it, you could just
ppc_hash64_store_hpte(env, i, HPTE64_V_HPTE_DIRTY, 0);
the HTAB in a loop.
Alex
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
On 2014-11-05 at 09:05, Markus Armbruster wrote: Jeff Cody jc...@redhat.com writes: On Tue, Nov 04, 2014 at 10:39:36AM +0100, Markus Armbruster wrote: Kevin Wolf kw...@redhat.com writes: Am 30.10.2014 um 13:49 hat Markus Armbruster geschrieben: Kevin Wolf kw...@redhat.com writes: Am 29.10.2014 um 14:54 hat Markus Armbruster geschrieben: Kevin Wolf kw...@redhat.com writes: Instead, let me try once more to sell my old proposal [1] from the thread you mentioned: What if we let the raw driver know that it was probed and then it enables a check that returns -EIO for any write on the first 2k if that write would make the image look like a different format? Attacks the problem where it arises instead of trying to detect the outcome of it, and works in whatever way it is nested in the BDS graph and whatever way is used to address the image file. Kevin [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg02548.html Arbitrarily breaking the virtual hardware that way is incompatible, too. It's a bigger no-no for me than changing user interface corner cases or deciding what to do with a file based on its file name extension. It is arbitrarily breaking theoretical cases, while your patch is less arbitrarily breaking common cases. I strongly prefer the former. I challenge theoretical as well as common. For theoretical, see below. As to common: are unrecognizable image names really common? I doubt it. If I'm wrong, I expect users to complain about my warning, and then we can reconsider. As I said in my other mail, I remember seeing BZs with command lines that don't have a file extension. Block devices are affected anyway. Your RFC also misses the extension .raw that I personally use occasionally, so I would get the warning even though the image name isn't unrecognizable at all. And other people probably have other extensions in use that we don't know of. Nobody will ever want to write a qcow2 header into their boot sector for just running a guest: 0: 51 push %cx 1: 46 inc%si 2: 49 dec%cx 3: fb sti This code doesn't make any sense. It's similar for other formats. And if they really have some odd reason to do that, the fix is easy: Either don't use raw, or pass an explicit format=raw. A disk needn't start with a PC-style boot sector. Even on a PC. Not every disk needs to be bootable, or even partitioned. Databases exist that like to work on raw devices. Giving them /dev/sdb instead of a whole-disk partition /dev/sdb1 may not be the smartest thing to do, but it *works* with real hardware, so why not with virtual hardware? Oh, it does. Just use a format that can represent this reliably (and as a compromise, we're going to declare explicitly requested raw as such - again, see my other mail for details). This is opt in safety. I dislike that for the same reason I dislike opt in security. Insecure: we risk guest escaping isolation. Unsafe: we risk virtual hardware breakage. You either have to prevent *any* writing of the first 2048 bytes (the part that can be examined by a bdrv_probe() method, or your have to prevent writing anything a probe recognizes, or the user has to specify the format explicitly. If you do the former, you're way outside the realm of theoretical. Right, that's not an option. If you do the latter, the degree of theoreticalness depends on the union of the patterns you prevent. Issues: 1. Anthony's method of checking a few known signatures is fragile. The only obviously robust way to test is probing going to find something other than 'raw'? is running the probes. Feasible. Yes, this is what I've been talking about. 2. Whether the union of patterns qualifies as theoretical for all our targets is not obvious, and whether it'll remain theoretical for all future formats and target machines even less. At least we don't have examples of it happening yet. And even if it happens to become not theoretical at some point, the only thing that happens is that they need to add format=raw - something that we already know is sure to happen with your approach. Once we get to know of such cases, we can probably even resolve them by improving the probing function of the problematic format. Your proposal improves things from insecure by default to now less insecure, but additionally a bit unsafe. Your proposed remedy for unsafe seems to be if it breaks, you get to keep the pieces, but you may ask us to narrow the conditions for breakage so it wouldn't have broken for you if you had imported the changed version from the future ;) 3. A write access that works just fine in one QEMU version can be rejected by another version that recognizes more formats. Or probes the same formats differently. True. We're only sure to protect this binary, and we have good chances of protecting some other qemu versions and some other tools, too. If the attacker has a time
[Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
Negative type of errno like -ERRNO is used a lot by developers. Therefore,
error_set_errno() is modified to deal with a negative type of os_error.
(Negative type is used at pcie_cap_slot_hotplug_common() in hw/pci/pcie.c)
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
---
util/error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/error.c b/util/error.c
index 2ace0d8..5db00c9 100644
--- a/util/error.c
+++ b/util/error.c
@@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass
err_class,
va_start(ap, fmt);
msg1 = g_strdup_vprintf(fmt, ap);
if (os_errno != 0) {
-err-msg = g_strdup_printf(%s: %s, msg1, strerror(os_errno));
+err-msg = g_strdup_printf(%s: %s, msg1, strerror(abs(os_errno)));
g_free(msg1);
} else {
err-msg = msg1;
--
2.1.0
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob
On 05.11.14 08:12, Aravinda Prasad wrote:
Extend rtas-blob to accommodate error log. Error log
structure is saved in rtas space upon a machine check
exception.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr.c |7 +++
include/hw/ppc/spapr.h |5 +
2 files changed, 12 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30de25d..38e26af 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, spapr-rtas.bin);
spapr-rtas_size = get_image_size(filename);
+
+/*
+ * Resize blob to accommodate error log. The layout of the rtas
+ * blob is defined in include/hw/ppc/spapr.h
+ */
+spapr-rtas_size = TARGET_PAGE_ALIGN(spapr-rtas_size);
How big is the error log? You could just extend the RTAS blob to include
space for it if it's not too big.
+
spapr-rtas_blob = g_malloc(spapr-rtas_size);
if (load_image_size(filename, spapr-rtas_blob, spapr-rtas_size) 0) {
hw_error(qemu: could not load LPAR rtas '%s'\n, filename);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 749daf4..d08fcc2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char
*propname,
int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
sPAPRTCETable *tcet);
+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET0
+#define RTAS_TRAMPOLINE_OFFSET 0x200
+#define RTAS_ERRLOG_OFFSET 0x800
I thought we agreed that these offsets should've been defined by the
blob itself?
Alex
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
On 2014-11-05 at 09:09, SeokYeon Hwang wrote:
Negative type of errno like -ERRNO is used a lot by developers. Therefore,
error_set_errno() is modified to deal with a negative type of os_error.
(Negative type is used at pcie_cap_slot_hotplug_common() in hw/pci/pcie.c)
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
---
util/error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/error.c b/util/error.c
index 2ace0d8..5db00c9 100644
--- a/util/error.c
+++ b/util/error.c
@@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass
err_class,
va_start(ap, fmt);
msg1 = g_strdup_vprintf(fmt, ap);
if (os_errno != 0) {
-err-msg = g_strdup_printf(%s: %s, msg1, strerror(os_errno));
+err-msg = g_strdup_printf(%s: %s, msg1, strerror(abs(os_errno)));
g_free(msg1);
} else {
err-msg = msg1;
This is utterly broken and we should fix all callers instead.
...But I like it.
Reviewed-by: Max Reitz mre...@redhat.com
Re: [Qemu-devel] [PATCH 1/6] configure: add dependency from libxml2
On 05/11/14 10:32, Michael Tokarev wrote: 29.10.2014 16:38, Denis V. Lunev wrote: This dependency is required for adequate Parallels images support. Typically the disk consists of several images which are glued by XML disk descriptor. Also XML hides inside several important parameters which are not available in the image header. [] +# check for libxml2 +if test $libxml2 != no ; then +if $pkg_config --exists libxml-2.0; then +libxml2=yes +libxml2_cflags=$($pkg_config --cflags libxml-2.0) +libxml2_libs=$($pkg_config --libs libxml-2.0) +QEMU_CFLAGS=$QEMU_CFLAGS $libxml2_cflags +libs_softmmu=$libs_softmmu $libxml2_libs +libs_tools=$libs_tools $libxml2_libs Please NO. NO NO NO NO :) Create a separate make variable, $LIBXML or something, and add it as a parallels.o dependency, not libs_softmmu etc. Ditto for the cflags. See examples -- libiscsi, libcurl, librbd - in block/Makefile.objs. no prob After that, I think we'll move parallels.o to be a module ;) (Cc'ing Fam) what does it mean for me? Could you point out the place to read/understand the difference. Thank you in advance, Den
Re: [Qemu-devel] [RFC PATCH] virtio-mmio: support for multiple irqs
Hi Shannon, Type of backend bandwith(GBytes/sec) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 Test cmd: ./iperf -c 192.168.0.2 -P 1 -i 10 -p 5001 -f G -t 60 Impressive results ! Could you please detail your setup ? which platform are you using and which GbE controller ? As a reference, it would be good also to have result with an iperf to the HOST to see how far we are from a native configuration... Also, I assume a pending Qemu patch is necessary to assign multiple irqs ? I'm correct ? Thanks a lot, Best regards Rémy -Message d'origine- De : Shannon Zhao [mailto:zhaoshengl...@huawei.com] Envoyé : mercredi 5 novembre 2014 09:00 À : linux-ker...@vger.kernel.org Cc : m...@redhat.com; peter.mayd...@linaro.org; john.li...@huawei.com; joel.sch...@amd.com; GAUGUEY Rémy 228890; qemu-devel@nongnu.org; n.nikol...@virtualopensystems.com; virtualizat...@lists.linux-foundation.org; peter.huangp...@huawei.com; hangaoh...@huawei.com Objet : Re: [RFC PATCH] virtio-mmio: support for multiple irqs On 2014/11/4 17:35, Shannon Zhao wrote: As the current virtio-mmio only support single irq, so some advanced features such as vhost-net with irqfd are not supported. And the net performance is not the best without vhost-net and irqfd supporting. Hi Joel, Peter, Mst, Some virtio-net with virtio-mmio performance data on ARM added as followed: Type of backend bandwith(GBytes/sec) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 Test cmd: ./iperf -c 192.168.0.2 -P 1 -i 10 -p 5001 -f G -t 60 From this test data, irqfd has great improvement (about 30%) on performance. So maybe it's necessary to enable multiple irq support to make vhost-net with virtio-mmio on ARM be able to use irqfd. How do you guys think? Look forward for your feedback. Thanks, Shannon This patch support virtio-mmio to request multiple irqs like virtio-pci. With this patch and qemu assigning multiple irqs for virtio-mmio device, it's ok to use vhost-net with irqfd on arm/arm64.
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call
On 05.11.14 08:13, Aravinda Prasad wrote:
This patch adds FWNMI support in qemu for powerKVM
guests by handling the ibm,nmi-register rtas call.
Whenever OS issues ibm,nmi-register RTAS call, the
machine check notification address is saved and the
machine check interrupt vector 0x200 is patched to
issue a private hcall.
This patch also handles the cases when multi-processors
experience machine check at or about the same time.
As per PAPR, subsequent processors serialize waiting
for the first processor to issue the ibm,nmi-interlock call.
The second processor retries if the first processor which
received a machine check is still reading the error log
and is yet to issue ibm,nmi-interlock call.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr_hcall.c| 16 +++
hw/ppc/spapr_rtas.c | 93
+++
include/hw/ppc/spapr.h | 17 +++
pc-bios/spapr-rtas/spapr-rtas.S | 38
4 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f16160..eceb5e5 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -97,6 +97,9 @@ struct rtas_mc_log {
struct rtas_error_log err_log;
};
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
static void do_spr_sync(void *arg)
{
struct SPRSyncState *s = arg;
@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
/*
+ * Only one VCPU can process machine check NMI at a time. Hence
+ * set the lock mc_in_progress. Once the VCPU finishes processing
+ * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+ * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+ * NMI we return 0 asking the VCPU to retry h_report_mc_err
+ */
+if (mc_in_progress == 1) {
Please don't depend on bools being numbers. Use true / false. For if()s,
just don't use == at all - it makes it more readable.
+return 0;
+}
+
+mc_in_progress = 1;
+
+/*
* We save the original r3 register in SPRG2 in 0x200 vector,
* which is patched during call to ibm.nmi-register. Original
* r3 is required to be included in error log
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2ec2a8e..71c7662 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -36,6 +36,9 @@
#include libfdt.h
+#define BRANCH_INST_MASK 0xFC00
+extern bool mc_in_progress;
Please put this into the spapr struct.
+
static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
uint32_t token, uint32_t nargs,
target_ulong args,
@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
rtas_st(rets, 0, ret);
}
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+ sPAPREnvironment *spapr,
+ uint32_t token, uint32_t nargs,
+ target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+int i;
+uint32_t ori_inst = 0x6063;
+uint32_t branch_inst = 0x4802;
+target_ulong guest_machine_check_addr;
+uint32_t trampoline[TRAMPOLINE_INSTS];
+int total_inst = sizeof(trampoline) / sizeof(uint32_t);
ARRAY_SIZE(trampoline), though I don't quite understand why you need a
variable that contains the same value as a constant (TRAMPOLINE_INSTS).
But since you're moving all of those bits into variable fields on the
rtas blob itself as we discussed in the last version, I guess this code
will go away anyways ;).
+PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+/* Store the system reset and machine check address */
+guest_machine_check_addr = rtas_ld(args, 1);
Load or Store? I don't find the comment particularly useful either ;).
+
+/*
+ * Read the trampoline instructions from RTAS Blob and patch
+ * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+ * machine check address before copying to 0x200 vector
+ */
+cpu_physical_memory_read(spapr-rtas_addr + RTAS_TRAMPOLINE_OFFSET,
+ trampoline, sizeof(trampoline));
+
+/* Safety Check */
Same for this comment.
+QEMU_BUILD_BUG_ON(sizeof(trampoline) MC_INTERRUPT_VECTOR_SIZE);
+
+/* Update the KVMPPC_H_REPORT_MC_ERR value in trampoline */
+ori_inst |= KVMPPC_H_REPORT_MC_ERR;
+memcpy(trampoline[TRAMPOLINE_ORI_INST_INDEX], ori_inst,
+sizeof(ori_inst));
Why memcpy a u32 into a u32 array?
+
+/*
+ * Sanity check guest_machine_check_addr to prevent clobbering
+ * operator value in branch instruction
+
Re: [Qemu-devel] The status about vhost-net on kvm-arm?
Hi Nikolay, From this mail I know you guys have done some work about ioeventfd support on kvm-arm before. Do you have plan to rework your patch based on the new branch? If not, I think we should send a patch to make eventfd support on kvm-arm and make vhost-net work. Based on the new kvm-arm branch with Eric's patch ARM: KVM: add irqfd support and eventfd patch in both kvm and qemu we have run virtio-mmio with vhost-net. And with our patch to enable virtio-mmio multiple irq and irqfd support, it work well to run vhost-net with irqfd on ARM. The performance test data as followed : Type of backend bandwith(GBytes/sec) (From Host to Guest) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 From this test data, vhost-net and irqfd have great improvement on performance. So maybe it's necessary to enable ioeventfd make vhost-net work with virtio-mmio on ARM. It's ok if I send these patches? Thanks, Shannon On 2014/8/12 23:47, Nikolay Nikolaev wrote: Hello, On Tue, Aug 12, 2014 at 5:41 AM, Li Liu john.li...@huawei.com wrote: Hi all, Is anyone there can tell the current status of vhost-net on kvm-arm? Half a year has passed from Isa Ansharullah asked this question: http://www.spinics.net/lists/kvm-arm/msg08152.html I have found two patches which have provided the kvm-arm support of eventfd and irqfd: 1) [RFC PATCH 0/4] ARM: KVM: Enable the ioeventfd capability of KVM on ARM http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg01770.html 2) [RFC,v3] ARM: KVM: add irqfd and irq routing support https://patches.linaro.org/32261/ And there's a rough patch for qemu to support eventfd from Ying-Shiuan Pan: [Qemu-devel] [PATCH 0/4] ioeventfd support for virtio-mmio https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg00715.html But there no any comments of this patch. And I can found nothing about qemu to support irqfd. Do I lost the track? If nobody try to fix it. We have a plan to complete it about virtio-mmio supporing irqfd and multiqueue. we at Virtual Open Systems did some work and tested vhost-net on ARM back in March. The setup was based on: - host kernel with our ioeventfd patches: http://www.spinics.net/lists/kvm-arm/msg08413.html - qemu with the aforementioned patches from Ying-Shiuan Pan https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg00715.html The testbed was ARM Chromebook with Exynos 5250, using a 1Gbps USB3 Ethernet adapter connected to a 1Gbps switch. I can't find the actual numbers but I remember that with multiple streams the gain was clearly seen. Note that it used the minimum required ioventfd implementation and not irqfd. I guess it is feasible to think that it all can be put together and rebased + the recent irqfd work. One can achiev even better performance (because of the irqfd). ___ kvmarm mailing list kvm...@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm regards, Nikolay Nikolaev Virtual Open Systems ___ kvmarm mailing list kvm...@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm -- Shannon
[Qemu-devel] [PATCH] Provide the missing LIBUSB_LOG_LEVEL_* for older libusb or FreeBSD. Providing just the needed value as a defined.
Signed-off-by: Chris Johns chr...@rtems.org --- hw/usb/host-libusb.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c index d2d161b..032a0e4 100644 --- a/hw/usb/host-libusb.c +++ b/hw/usb/host-libusb.c @@ -143,6 +143,12 @@ static void usb_host_attach_kernel(USBHostDevice *s); /* */ +#ifndef LIBUSB_LOG_LEVEL_WARNING /* older libusb didn't define these */ +#define LIBUSB_LOG_LEVEL_WARNING 2 +#endif + +/* */ + #define CONTROL_TIMEOUT 1/* 10 sec*/ #define BULK_TIMEOUT 0/* unlimited */ #define INTR_TIMEOUT 0/* unlimited */ -- 2.1.2
Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it
On 2014-11-04 at 19:45, Markus Armbruster wrote:
I'll try to explain all solutions fairly. Isn't easy when you're as
biased towards one of them as I am. Please bear with me.
= The trust boundary between image contents and meta-data =
A disk image consists of image contents and meta-data.
Example: all of a raw image's contents is image contents. Leaves just
file name and attributes for meta-data.
Example: QCOW2 meta-data includes header, header extensions, L1 table,
L2 tables, ... The meta-data defines where in the image the actual
contents is stored.
A guest can access the image contents, not the meta-data.
Image contents you've let an untrusted guest write is untrusted.
Therefore, there's a trust boundary between image contents and
meta-data. QEMU has to trust image meta-data, but shouldn't trust image
contents. The exact location of the trust boundary depends on the image
format.
= How we instruct QEMU what to trust =
By configuring QEMU to use an image, the user instructs QEMU to trust
the image's meta-data.
When the user's configuration specifies the image format explicitly, the
trust boundary is clear.
Else, the trust boundary is ambigous when more than one format is
possible.
QEMU resolves this ambiguity by picking the first format with the
highest score. Raw format is always possible, and always has the
lowest score.
= How this lets the guest escape isolation =
Unfortunately, this lets the guest shift the trust boundary and escape
isolation, as follows:
* Expose a raw image to the guest (whether you specify the format=raw or
let QEMU guess it doesn't matter). The complete contents becomes
untrusted.
* Reuse the image *without* specifying the raw format. QEMU guesses the
format based on untrusted image contents. Now QEMU guesses a format
chosen by the guest, with meta-data chosen by the guest. By
controlling image meta-data, the malicious guest can access arbitrary
files as QEMU, enlarge its storage, and more. A non-malicious guest
can accidentally DoS itself, by writing a pattern probing recognizes.
Thank you for bringing that to my attention. This means that I'm even
more in favor of using Kevin's patches because in fact they don't break
anything.
This is CVE-2008-2004.
= Aside: other trust boundaries =
Of course, this is not the only trust boundary that matters. For
instance, there's normally one between your host and somebody else's
computers. Telling QEMU to trust meta-data of some image you got from
the internet violates it. There's nothing QEMU can do about that.
= Insecure usage is easy, secure usage is hard =
The oldest stratum of user interfaces doesn't let you specify the image
format. Use of raw images with these is insecure by design. These
interfaces are still recommended for human users.
Example of insecure usage: -hda foo.img, where foo.img is raw.
With the next generation of interfaces, specifying the image format is
optional. Use of raw images with these is insecure by default.
Example of insecure usage: -drive file=foo.img,index=0,media=cdrom,
where foo.img is raw. The -hda above is actually sugar for this.
Equivalent secure usage: add format=raw.
Note that specifying just the top image's format is not enough, you also
have to specify any backing images' formats. QCOW2 can optionally store
the backing image format in the image. The other COW formats can't.
Well, they can, with json:. *cough*
Example of insecure usage: -hda bar.vmdk, where bar.vmdk is a VMDK image
with a raw backing file.
Yesterday I found out that doesn't seem possible. You apparently can
only use VMDK with VMDK backing files. Other than that, we only have
qcow1 and qed as COW formats which should not be used anyway.
Equivalent secure usage: Beats me. Maybe there's a funky -drive
backing.whatever to specify the backing image's format.
With the latest interface blockdev-add, specifying the format is
mandatory. Secure, but not really suitable for humans.
Example of secure usage:
{ execute: blockdev-add,
arguments: {'options': {
'driver': 'raw', 'id':'foo',
'file': { 'driver': 'file', 'filename': 'foo.img' } } } }
Insecure usage is easy, secure usage is *hard*. Even for sophisticated
users like libvirt developers. Evidence: libvirt CVE-2010-2237,
CVE-2010-2238, CVE-2010-2239, and more that didn't get a CVE, like the
recent accidental probing in drive-mirror.
= How can we better guard the trust boundary in QEMU? =
The guest can violate the trust boundary only because
(a) QEMU supports both raw images and image formats, and
(b) QEMU guesses image format from raw image contents, and
(c) given a raw image, guests can change its contents and control a
future QEMU's format guess.
We can attack one ore more of these three conditions:
I'd like to attack more because any of these steps might be carried out
in another program which thus either becomes vulnerable itself (which we
don't
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
Kevin Wolf kw...@redhat.com writes: Am 04.11.2014 um 16:25 hat Stefan Hajnoczi geschrieben: On Tue, Nov 04, 2014 at 11:11:33AM +0100, Kevin Wolf wrote: Am 03.11.2014 um 16:05 hat Stefan Hajnoczi geschrieben: The argument that there might not be a traditional filename doesn't make sense to me. When there is no filename the command-line is already sufficiently complex and usage is fancy enough that probing adds no convenience, the user can just specify the format. -hda nbd://localhost -drive file=nbd://localhost,format=raw Almost double the length, and I don't see anything fancy in the first line. Anyway, does this sound reasonable: In QEMU 3.0, require the format= option for -drive. Keep probing the way it is for non-drive options because they are used for convenience by local users. And being hacked while using -hda is better in which way? Markus is proposing that we look at the filename extension. In that case QEMU cannot be tricked by the contents of a raw image. That makes -hda perfectly safe although there are cases where QEMU doesn't know what to do and requires format=. Wait, by keep probing the way it is you mean implementing one of the other proposals? So you're only suggesting being stricter on -drive as an additional measure? I do worry that changing QEMU's probing behavior drastically can lead to consistencies where libvirt does its own probing :(. Haven't thought through the bug scenarios but that could be a security problem in itself. Hm... In which cases does libvirt probe the image format? And is it even consistent with qemu today? I had a quick look at the source. Eric, please correct misunderstandings. Enumation type virStorageFileProbeFormat enumerates supported formats. It has pseudo-formats VIR_STORAGE_FILE_AUTO, VIR_STORAGE_FILE_AUTO_SAFE. I don't understand VIR_STORAGE_FILE_AUTO_SAFE offhand. VIR_STORAGE_FILE_AUTO means probing. Its use appears to be deprecated. Actual probing happens in virStorageFileProbeFormatFromBuf(): For all formats: if magic and version match, pick this format If some magic matched, but not the version: warn For all formats: if file name extension matches, pick this format Pick raw. The formats' magic, version and extension are defined in fileTypeInfo[]. If I remember correctly, libvirt has its own probing because running an external program just to probe is too slow. Another reason for having its own probing might be providing a secure replacement for QEMU's insecure probing. If you can get libvirt to explicitly pass the wrong format=... option because it did its own probing, we have a problem no matter what we change in qemu. Yes, but that would be a libvirt problem. No excuse for us to ignore our own problems.
Re: [Qemu-devel] [RFC PATCH 0/2] virtio-mmio: add irqfd support for vhost-net based on virtio-mmio
On 10/27/2014 12:23 PM, Li Liu wrote: On 2014/10/27 17:37, Peter Maydell wrote: On 25 October 2014 09:24, john.liuli john.li...@huawei.com wrote: To get the interrupt reason to support such VIRTIO_NET_F_STATUS features I add a new register offset VIRTIO_MMIO_ISRMEM which will help to establish a shared memory region between qemu and virtio-mmio device. Then the interrupt reason can be accessed by guest driver through this region. At the same time, the virtio-mmio dirver check this region to see irqfd is supported or not during the irq handler registration, and different handler will be assigned. If you want to add a new register you should probably propose an update to the virtio spec. However, it seems to me it would be better to get generic PCI/PCIe working on the ARM virt board instead; then we can let virtio-mmio quietly fade away. This has been on the todo list for ages (and there have been RFC patches posted for plain PCI), it's just nobody's had time to work on it. thanks -- PMM So you mean virtio-mmio will be replaced by PCI/PCIe on ARM at last? If so, let this patch go with the wind:). Thx. Hi, As a fix of current situation where ISR is only partially updated when vhost-irqfd handles standard IRQ and waiting for PCI emuluation, wouldn't it make sense to store ISR content on vhost driver side and introduce ioctls to read/write it. When using vhost BE, virtio QEMU device would use those ioctl to read/update the ISR content. On top of that we would update the ISR in vhost before triggering the irqfd. If I do not miss anything this would at least make things functional with irqfd. As a second step, we could try to introduce in-kernel emulation of ISR/ACK to fix the performance issue related to going to user-side each time ISR/ACK accesses are done. Do you think it is worth investigating this direction? Thank you in advance Best Regards Eric Li. .
[Qemu-devel] [PATCH] target-i386: cpu: keeping function parameters alignment on new line
Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
---
target-i386/cpu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index fa860de..3f13dfe 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -540,8 +540,8 @@ void host_cpuid(uint32_t function, uint32_t count,
* otherwise the string is assumed to sized by a terminating nul.
* Return lexical ordering of *s1:*s2.
*/
-static int sstrcmp(const char *s1, const char *e1, const char *s2,
-const char *e2)
+static int sstrcmp(const char *s1, const char *e1,
+ const char *s2, const char *e2)
{
for (;;) {
if (!*s1 || !*s2 || *s1 != *s2)
@@ -1859,7 +1859,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char
*features,
* if flags, suppress names undefined in featureset.
*/
static void listflags(char *buf, int bufsize, uint32_t fbits,
-const char **featureset, uint32_t flags)
+ const char **featureset, uint32_t flags)
{
const char **p = featureset[31];
char *q, *b, bit;
--
1.9.3
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob
On Wednesday 05 November 2014 01:41 PM, Alexander Graf wrote:
On 05.11.14 08:12, Aravinda Prasad wrote:
Extend rtas-blob to accommodate error log. Error log
structure is saved in rtas space upon a machine check
exception.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr.c |7 +++
include/hw/ppc/spapr.h |5 +
2 files changed, 12 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30de25d..38e26af 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, spapr-rtas.bin);
spapr-rtas_size = get_image_size(filename);
+
+/*
+ * Resize blob to accommodate error log. The layout of the rtas
+ * blob is defined in include/hw/ppc/spapr.h
+ */
+spapr-rtas_size = TARGET_PAGE_ALIGN(spapr-rtas_size);
How big is the error log? You could just extend the RTAS blob to include
space for it if it's not too big.
Error log is around 10 bytes and requires additional 24 bytes to store
saved sro/srr1.
Hmm.. yes it can be included in RTAS blob itself.
+
spapr-rtas_blob = g_malloc(spapr-rtas_size);
if (load_image_size(filename, spapr-rtas_blob, spapr-rtas_size) 0) {
hw_error(qemu: could not load LPAR rtas '%s'\n, filename);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 749daf4..d08fcc2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char
*propname,
int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
sPAPRTCETable *tcet);
+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET0
+#define RTAS_TRAMPOLINE_OFFSET 0x200
+#define RTAS_ERRLOG_OFFSET 0x800
I thought we agreed that these offsets should've been defined by the
blob itself?
I think I got it wrong.
I will include these indexes at the entry of RTAS blob. With that we
will have something like this:
RTAS_ENTRY_OFFSET = *(spapr-rtas_addr)
RTAS_TRAMPOLINE_OFFSET = *(spapr-rtas_addr+8)
RTAS_ERRLOG_OFFSET = *(spapr-rtas_addr+16)
I will fix this.
Regards,
Aravinda
Alex
--
Regards,
Aravinda
Re: [Qemu-devel] [PATCH] ui/input: strictly check console in finding input handler
On Mi, 2014-11-05 at 00:49 +0800, Amos Kong wrote: qemu_input_find_handler() prefers a handler associated with con. But if none exists, it takes any. This patch added a parameter to strictly check console, in case we want to input event to special console. 'input-send-event' has a parameter to assign special console, so we should enable strict checking in finding handler. I don't think we want do that by default. It only matters in case of a multiseat setup where you actually have multiple input devices of the same kind. Which isn't a very typical use case. Options I see are: (a) Turn console into an optional parameter, do strict checking in case it is present. (b) Add a optional 'strict' parameter. cheers, Gerd
Re: [Qemu-devel] [PATCH 2/5] pc: define PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA
On Tue, Nov 04, 2014 at 03:44:35PM +0200, Marcel Apfelbaum wrote:
On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
PC_PCI_CONFIG_ADDR and PC_PCI_CONFIG_DATA are defined in PCI
specification, so move them to common place.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci-host/piix.c| 8
hw/pci-host/q35.c | 8
include/hw/pci-host/q35.h | 3 ---
include/hw/pci/pci.h | 5 +
tests/libqos/pci-pc.c | 24
5 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 1530038..eb92bde 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -288,11 +288,11 @@ static void i440fx_pcihost_realize(DeviceState *dev,
Error **errp)
PCIHostState *s = PCI_HOST_BRIDGE(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-sysbus_add_io(sbd, 0xcf8, s-conf_mem);
-sysbus_init_ioports(sbd, 0xcf8, 4);
+sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, s-conf_mem);
+sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
-sysbus_add_io(sbd, 0xcfc, s-data_mem);
-sysbus_init_ioports(sbd, 0xcfc, 4);
+sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, s-data_mem);
+sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
}
static int i440fx_initfn(PCIDevice *dev)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b20bad8..9e66835 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -41,11 +41,11 @@ static void q35_host_realize(DeviceState *dev, Error
**errp)
Q35PCIHost *s = Q35_HOST_DEVICE(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, pci-conf_mem);
-sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
+sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, pci-conf_mem);
+sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
-sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, pci-data_mem);
-sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
+sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, pci-data_mem);
+sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
pci-bus = pci_bus_new(DEVICE(s), pcie.0,
s-mch.pci_address_space,
s-mch.address_space_io,
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 025d6e6..3a026b0 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -82,9 +82,6 @@ typedef struct Q35PCIHost {
/* PCI configuration */
#define MCH_HOST_BRIDGEMCH
-#define MCH_HOST_BRIDGE_CONFIG_ADDR0xcf8
-#define MCH_HOST_BRIDGE_CONFIG_DATA0xcfc
-
/* D0:F0 configuration space */
#define MCH_HOST_BRIDGE_REVISION_DEFAULT 0x0
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 3d42d7f..e42589a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -13,6 +13,11 @@
#include hw/pci/pcie.h
+/* PCI configuration */
+
+#define PC_PCI_CONFIG_ADDR 0xcf8
+#define PC_PCI_CONFIG_DATA 0xcfc
I would move the macros also to hw/pci/pci_host.h,
and only personal opinion, change them to
PCI_HOST_BRIDGE_...
Done.
Regards,
Hu
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob
On 05.11.14 09:46, Aravinda Prasad wrote:
On Wednesday 05 November 2014 01:41 PM, Alexander Graf wrote:
On 05.11.14 08:12, Aravinda Prasad wrote:
Extend rtas-blob to accommodate error log. Error log
structure is saved in rtas space upon a machine check
exception.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr.c |7 +++
include/hw/ppc/spapr.h |5 +
2 files changed, 12 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30de25d..38e26af 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, spapr-rtas.bin);
spapr-rtas_size = get_image_size(filename);
+
+/*
+ * Resize blob to accommodate error log. The layout of the rtas
+ * blob is defined in include/hw/ppc/spapr.h
+ */
+spapr-rtas_size = TARGET_PAGE_ALIGN(spapr-rtas_size);
How big is the error log? You could just extend the RTAS blob to include
space for it if it's not too big.
Error log is around 10 bytes and requires additional 24 bytes to store
saved sro/srr1.
Hmm.. yes it can be included in RTAS blob itself.
+
spapr-rtas_blob = g_malloc(spapr-rtas_size);
if (load_image_size(filename, spapr-rtas_blob, spapr-rtas_size) 0)
{
hw_error(qemu: could not load LPAR rtas '%s'\n, filename);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 749daf4..d08fcc2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char
*propname,
int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
sPAPRTCETable *tcet);
+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET0
+#define RTAS_TRAMPOLINE_OFFSET 0x200
+#define RTAS_ERRLOG_OFFSET 0x800
I thought we agreed that these offsets should've been defined by the
blob itself?
I think I got it wrong.
I will include these indexes at the entry of RTAS blob. With that we
will have something like this:
RTAS_ENTRY_OFFSET = *(spapr-rtas_addr)
RTAS_TRAMPOLINE_OFFSET = *(spapr-rtas_addr+8)
RTAS_ERRLOG_OFFSET = *(spapr-rtas_addr+16)
I will fix this.
Cool :). Just store the offsets inside of a helper struct that you for
example store in the spapr struct, then we don't need to read volatile
guest memory for the offsets.
Alex
[Qemu-devel] [PATCH v2 2/5] pci: introduce pci_host_config_enabled()
This makes code more readable.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/mips/gt64xxx_pci.c | 4 ++--
hw/pci/pci_host.c | 5 +++--
include/hw/pci/pci_host.h | 5 +
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 1f2fe5f..f118c9c 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr,
if (!(s-regs[GT_PCI0_CMD] 1) (phb-config_reg 0x00fff800)) {
val = bswap32(val);
}
-if (phb-config_reg (1u 31)) {
+if (pci_host_config_enabled(phb)) {
pci_data_write(phb-bus, phb-config_reg, val, 4);
}
break;
@@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque,
val = phb-config_reg;
break;
case GT_PCI0_CFGDATA:
-if (!(phb-config_reg (1 31))) {
+if (!pci_host_config_enabled(phb)) {
val = 0x;
} else {
val = pci_data_read(phb-bus, phb-config_reg, 4);
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..9bc47d8 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
PCIHostState *s = opaque;
PCI_DPRINTF(write addr TARGET_FMT_plx len %d val %x\n,
addr, len, (unsigned)val);
-if (s-config_reg (1u 31))
+if (pci_host_config_enabled(s)) {
pci_data_write(s-bus, s-config_reg | (addr 3), val, len);
+}
}
static uint64_t pci_host_data_read(void *opaque,
@@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque,
{
PCIHostState *s = opaque;
uint32_t val;
-if (!(s-config_reg (1U 31))) {
+if (!pci_host_config_enabled(s)) {
return 0x;
}
val = pci_data_read(s-bus, s-config_reg | (addr 3), len);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index ba31595..b48791d 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -65,6 +65,11 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev,
uint32_t addr,
void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
+static inline bool pci_host_config_enabled(struct PCIHostState *pci_host)
+{
+return pci_host-config_reg (1U 31);
+}
+
extern const MemoryRegionOps pci_host_conf_le_ops;
extern const MemoryRegionOps pci_host_conf_be_ops;
extern const MemoryRegionOps pci_host_data_le_ops;
--
1.9.3
[Qemu-devel] [PATCH v2 1/5] pci: reorganize QEMU_PCI_CAP_*
This makes code more readable.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
include/hw/pci/pci.h | 39 ---
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c352c7b..b18759a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -142,25 +142,26 @@ enum {
/* Bits in cap_present field. */
enum {
-QEMU_PCI_CAP_MSI = 0x1,
-QEMU_PCI_CAP_MSIX = 0x2,
-QEMU_PCI_CAP_EXPRESS = 0x4,
-
-/* multifunction capable device */
-#define QEMU_PCI_CAP_MULTIFUNCTION_BITNR3
-QEMU_PCI_CAP_MULTIFUNCTION = (1 QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
-
-/* command register SERR bit enabled */
-#define QEMU_PCI_CAP_SERR_BITNR 4
-QEMU_PCI_CAP_SERR = (1 QEMU_PCI_CAP_SERR_BITNR),
-/* Standard hot plug controller. */
-#define QEMU_PCI_SHPC_BITNR 5
-QEMU_PCI_CAP_SHPC = (1 QEMU_PCI_SHPC_BITNR),
-#define QEMU_PCI_SLOTID_BITNR 6
-QEMU_PCI_CAP_SLOTID = (1 QEMU_PCI_SLOTID_BITNR),
-/* PCI Express capability - Power Controller Present */
-#define QEMU_PCIE_SLTCAP_PCP_BITNR 7
-QEMU_PCIE_SLTCAP_PCP = (1 QEMU_PCIE_SLTCAP_PCP_BITNR),
+QEMU_PCI_CAP_MSI_BITNR = 0,
+QEMU_PCI_CAP_MSIX_BITNR,
+QEMU_PCI_CAP_EXPRESS_BITNR,
+QEMU_PCI_CAP_MULTIFUNCTION_BITNR, /* multifunction capable device */
+QEMU_PCI_CAP_SERR_BITNR, /* command register SERR bit enabled */
+QEMU_PCI_SHPC_BITNR, /* Standard hot plug controller */
+QEMU_PCI_SLOTID_BITNR,
+QEMU_PCIE_SLTCAP_PCP_BITNR, /* PCI Express capability - Power Controller
+ Present */
+};
+
+enum {
+QEMU_PCI_CAP_MSI= (1 QEMU_PCI_CAP_MSI_BITNR),
+QEMU_PCI_CAP_MSIX = (1 QEMU_PCI_CAP_MSIX_BITNR),
+QEMU_PCI_CAP_EXPRESS= (1 QEMU_PCI_CAP_EXPRESS_BITNR),
+QEMU_PCI_CAP_MULTIFUNCTION = (1 QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
+QEMU_PCI_CAP_SERR = (1 QEMU_PCI_CAP_SERR_BITNR),
+QEMU_PCI_CAP_SHPC = (1 QEMU_PCI_SHPC_BITNR),
+QEMU_PCI_CAP_SLOTID = (1 QEMU_PCI_SLOTID_BITNR),
+QEMU_PCIE_SLTCAP_PCP= (1 QEMU_PCIE_SLTCAP_PCP_BITNR),
};
#define TYPE_PCI_DEVICE pci-device
--
1.9.3
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob
On 05.11.14 10:00, Alexander Graf wrote:
On 05.11.14 09:46, Aravinda Prasad wrote:
On Wednesday 05 November 2014 01:41 PM, Alexander Graf wrote:
On 05.11.14 08:12, Aravinda Prasad wrote:
Extend rtas-blob to accommodate error log. Error log
structure is saved in rtas space upon a machine check
exception.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr.c |7 +++
include/hw/ppc/spapr.h |5 +
2 files changed, 12 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30de25d..38e26af 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, spapr-rtas.bin);
spapr-rtas_size = get_image_size(filename);
+
+/*
+ * Resize blob to accommodate error log. The layout of the rtas
+ * blob is defined in include/hw/ppc/spapr.h
+ */
+spapr-rtas_size = TARGET_PAGE_ALIGN(spapr-rtas_size);
How big is the error log? You could just extend the RTAS blob to include
space for it if it's not too big.
Error log is around 10 bytes and requires additional 24 bytes to store
saved sro/srr1.
Hmm.. yes it can be included in RTAS blob itself.
+
spapr-rtas_blob = g_malloc(spapr-rtas_size);
if (load_image_size(filename, spapr-rtas_blob, spapr-rtas_size)
0) {
hw_error(qemu: could not load LPAR rtas '%s'\n, filename);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 749daf4..d08fcc2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char
*propname,
int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
sPAPRTCETable *tcet);
+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET0
+#define RTAS_TRAMPOLINE_OFFSET 0x200
+#define RTAS_ERRLOG_OFFSET 0x800
I thought we agreed that these offsets should've been defined by the
blob itself?
I think I got it wrong.
I will include these indexes at the entry of RTAS blob. With that we
will have something like this:
RTAS_ENTRY_OFFSET = *(spapr-rtas_addr)
RTAS_TRAMPOLINE_OFFSET = *(spapr-rtas_addr+8)
RTAS_ERRLOG_OFFSET = *(spapr-rtas_addr+16)
I will fix this.
Cool :). Just store the offsets inside of a helper struct that you for
example store in the spapr struct, then we don't need to read volatile
guest memory for the offsets.
I just reread what I wrote and figured it's not exactly verbose. What I
meant was that you read them on load into a struct. Then when working
with the offsets, you only use the cached ones from the struct.
That way when the guest for whatever reason modifies the RTAS blob in
memory, we would still use the old offsets and ensure that we don't end
up overwriting memory that we never intended to overwrite ;).
Alex
[Qemu-devel] [PATCH v2 3/5] pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA.
PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA are
defined in PCI specification, so move them to common place.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci-host/piix.c| 8
hw/pci-host/prep.c| 6 --
hw/pci-host/q35.c | 8
include/hw/pci-host/q35.h | 3 ---
include/hw/pci/pci_host.h | 5 +
tests/libqos/pci-pc.c | 25 +
6 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 1530038..76f3757 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -288,11 +288,11 @@ static void i440fx_pcihost_realize(DeviceState *dev,
Error **errp)
PCIHostState *s = PCI_HOST_BRIDGE(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-sysbus_add_io(sbd, 0xcf8, s-conf_mem);
-sysbus_init_ioports(sbd, 0xcf8, 4);
+sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, s-conf_mem);
+sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
-sysbus_add_io(sbd, 0xcfc, s-data_mem);
-sysbus_init_ioports(sbd, 0xcfc, 4);
+sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, s-data_mem);
+sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
}
static int i440fx_initfn(PCIDevice *dev)
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 1de3681..2ae21ad 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -228,11 +228,13 @@ static void raven_pcihost_realizefn(DeviceState *d, Error
**errp)
memory_region_init_io(h-conf_mem, OBJECT(h), pci_host_conf_le_ops, s,
pci-conf-idx, 4);
-memory_region_add_subregion(s-pci_io, 0xcf8, h-conf_mem);
+memory_region_add_subregion(s-pci_io, PCI_HOST_BRIDGE_CONFIG_ADDR,
+h-conf_mem);
memory_region_init_io(h-data_mem, OBJECT(h), pci_host_data_le_ops, s,
pci-conf-data, 4);
-memory_region_add_subregion(s-pci_io, 0xcfc, h-data_mem);
+memory_region_add_subregion(s-pci_io, PCI_HOST_BRIDGE_CONFIG_DATA,
+h-data_mem);
memory_region_init_io(h-mmcfg, OBJECT(s), raven_pci_io_ops, s,
pciio, 0x0040);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b20bad8..666afea 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -41,11 +41,11 @@ static void q35_host_realize(DeviceState *dev, Error **errp)
Q35PCIHost *s = Q35_HOST_DEVICE(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, pci-conf_mem);
-sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_ADDR, 4);
+sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, pci-conf_mem);
+sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_ADDR, 4);
-sysbus_add_io(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, pci-data_mem);
-sysbus_init_ioports(sbd, MCH_HOST_BRIDGE_CONFIG_DATA, 4);
+sysbus_add_io(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, pci-data_mem);
+sysbus_init_ioports(sbd, PCI_HOST_BRIDGE_CONFIG_DATA, 4);
pci-bus = pci_bus_new(DEVICE(s), pcie.0,
s-mch.pci_address_space, s-mch.address_space_io,
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 025d6e6..3a026b0 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -82,9 +82,6 @@ typedef struct Q35PCIHost {
/* PCI configuration */
#define MCH_HOST_BRIDGEMCH
-#define MCH_HOST_BRIDGE_CONFIG_ADDR0xcf8
-#define MCH_HOST_BRIDGE_CONFIG_DATA0xcfc
-
/* D0:F0 configuration space */
#define MCH_HOST_BRIDGE_REVISION_DEFAULT 0x0
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index b48791d..2bae45a 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -30,6 +30,11 @@
#include hw/sysbus.h
+/* PCI configuration */
+
+#define PCI_HOST_BRIDGE_CONFIG_ADDR 0xcf8
+#define PCI_HOST_BRIDGE_CONFIG_DATA 0xcfc
+
#define TYPE_PCI_HOST_BRIDGE pci-host-bridge
#define PCI_HOST_BRIDGE(obj) \
OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index 6dba0db..e4c3c11 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -14,6 +14,7 @@
#include libqos/pci-pc.h
#include hw/pci/pci_regs.h
+#include hw/pci/pci_host.h
#include qemu-common.h
#include qemu/host-utils.h
@@ -113,38 +114,38 @@ static void qpci_pc_io_writel(QPCIBus *bus, void *addr,
uint32_t value)
static uint8_t qpci_pc_config_readb(QPCIBus *bus, int devfn, uint8_t offset)
{
-outl(0xcf8, (1U 31) | (devfn 8) | offset);
-return inb(0xcfc);
+outl(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U 31) | (devfn 8) | offset);
+return inb(PCI_HOST_BRIDGE_CONFIG_DATA);
}
static uint16_t qpci_pc_config_readw(QPCIBus *bus, int devfn, uint8_t offset)
{
-outl(0xcf8, (1U 31) | (devfn 8) | offset);
-return inw(0xcfc);
+outl(PCI_HOST_BRIDGE_CONFIG_ADDR, (1U
[Qemu-devel] [PATCH v2 0/5] Some PCI related cleanup patches
Hi, This is v2 of PCI clenaup series. See each patch for the detail. changes: v2: - remove patch 3 from v1 which is incorrect. - rename defined macros as per Marcel's suggestion - place macros in pci_host.h as per Marcel's suggestion - new patch 'pci: reorganize QEMU_PCI_CAP_*' Hu Tao (5): pci: reorganize QEMU_PCI_CAP_* pci: introduce pci_host_config_enabled() pci: define PCI_HOST_BRIDGE_CONFIG_ADDR and PCI_HOST_BRIDGE_CONFIG_DATA. pci: remove the limit parameter of pci_host_config_read_common pci: remove the limit parameter of pci_host_config_write_common hw/mips/gt64xxx_pci.c | 4 ++-- hw/pci-host/piix.c| 8 hw/pci-host/prep.c| 6 -- hw/pci-host/q35.c | 8 hw/pci/pci_host.c | 33 - hw/pci/pcie_host.c| 18 ++ hw/ppc/spapr_pci.c| 6 ++ include/hw/pci-host/q35.h | 3 --- include/hw/pci/pci.h | 39 --- include/hw/pci/pci_host.h | 14 -- tests/libqos/pci-pc.c | 25 + 11 files changed, 87 insertions(+), 77 deletions(-) -- 1.9.3
[Qemu-devel] [PATCH v2 5/5] pci: remove the limit parameter of pci_host_config_write_common
Since the limit parameter is always set to the size of pci device's
configuration space, and we can determine the size from the type of pci
device.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci/pci_host.c | 13 ++---
hw/pci/pcie_host.c| 9 +
hw/ppc/spapr_pci.c| 3 +--
include/hw/pci/pci_host.h | 2 +-
4 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 2b11551..4a59b0e 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -49,8 +49,16 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus,
uint32_t addr)
}
void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
- uint32_t limit, uint32_t val, uint32_t len)
+ uint32_t val, uint32_t len)
{
+uint32_t limit = pci_config_size(pci_dev);
+
+if (limit = addr) {
+/* conventional pci device can be behind pcie-to-pci bridge.
+ 256 = addr 4K has no effects. */
+return;
+}
+
assert(len = 4);
trace_pci_cfg_write(pci_dev-name, PCI_SLOT(pci_dev-devfn),
PCI_FUNC(pci_dev-devfn), addr, val);
@@ -89,8 +97,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val,
int len)
PCI_DPRINTF(%s: %s: addr=%02 PRIx32 val=%08 PRIx32 len=%d\n,
__func__, pci_dev-name, config_addr, val, len);
-pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
- val, len);
+pci_host_config_write_common(pci_dev, config_addr, val, len);
}
uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index cf8587b..e3a2a80 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -39,19 +39,12 @@ static void pcie_mmcfg_data_write(void *opaque, hwaddr
mmcfg_addr,
PCIBus *s = e-pci.bus;
PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
uint32_t addr;
-uint32_t limit;
if (!pci_dev) {
return;
}
addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-limit = pci_config_size(pci_dev);
-if (limit = addr) {
-/* conventional pci device can be behind pcie-to-pci bridge.
- 256 = addr 4K has no effects. */
-return;
-}
-pci_host_config_write_common(pci_dev, addr, limit, val, len);
+pci_host_config_write_common(pci_dev, addr, val, len);
}
static uint64_t pcie_mmcfg_data_read(void *opaque,
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7f38117..f306d42 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -171,8 +171,7 @@ static void finish_write_pci_config(sPAPREnvironment
*spapr, uint64_t buid,
return;
}
-pci_host_config_write_common(pci_dev, addr, pci_config_size(pci_dev),
- val, size);
+pci_host_config_write_common(pci_dev, addr, val, size);
rtas_st(rets, 0, RTAS_OUT_SUCCESS);
}
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 72a1b8b..67e007f 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -63,7 +63,7 @@ typedef struct PCIHostBridgeClass {
/* common internal helpers for PCI/PCIe hosts, cut off overflows */
void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
- uint32_t limit, uint32_t val, uint32_t len);
+ uint32_t val, uint32_t len);
uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
uint32_t len);
--
1.9.3
[Qemu-devel] [PATCH v2 4/5] pci: remove the limit parameter of pci_host_config_read_common
Since the limit parameter is always set to the size of pci device's
configuration space, and we can determine the size from the type of pci
device.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci/pci_host.c | 15 +++
hw/pci/pcie_host.c| 9 +
hw/ppc/spapr_pci.c| 3 +--
include/hw/pci/pci_host.h | 2 +-
4 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 9bc47d8..2b11551 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -58,12 +58,20 @@ void pci_host_config_write_common(PCIDevice *pci_dev,
uint32_t addr,
}
uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
- uint32_t limit, uint32_t len)
+ uint32_t len)
{
+uint32_t limit = pci_config_size(pci_dev);
uint32_t ret;
assert(len = 4);
-ret = pci_dev-config_read(pci_dev, addr, MIN(len, limit - addr));
+
+if (limit = addr) {
+/* conventional pci device can be behind pcie-to-pci bridge.
+ 256 = addr 4K has no effects. */
+ret = ~0x0;
+} else {
+ret = pci_dev-config_read(pci_dev, addr, MIN(len, limit - addr));
+}
trace_pci_cfg_read(pci_dev-name, PCI_SLOT(pci_dev-devfn),
PCI_FUNC(pci_dev-devfn), addr, ret);
@@ -95,8 +103,7 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
return ~0x0;
}
-val = pci_host_config_read_common(pci_dev, config_addr,
- PCI_CONFIG_SPACE_SIZE, len);
+val = pci_host_config_read_common(pci_dev, config_addr, len);
PCI_DPRINTF(%s: %s: addr=%02PRIx32 val=%08PRIx32 len=%d\n,
__func__, pci_dev-name, config_addr, val, len);
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 3db038f..cf8587b 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -62,19 +62,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
PCIBus *s = e-pci.bus;
PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
uint32_t addr;
-uint32_t limit;
if (!pci_dev) {
return ~0x0;
}
addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-limit = pci_config_size(pci_dev);
-if (limit = addr) {
-/* conventional pci device can be behind pcie-to-pci bridge.
- 256 = addr 4K has no effects. */
-return ~0x0;
-}
-return pci_host_config_read_common(pci_dev, addr, limit, len);
+return pci_host_config_read_common(pci_dev, addr, len);
}
static const MemoryRegionOps pcie_mmcfg_ops = {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ad0da7f..7f38117 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -105,8 +105,7 @@ static void finish_read_pci_config(sPAPREnvironment *spapr,
uint64_t buid,
return;
}
-val = pci_host_config_read_common(pci_dev, addr,
- pci_config_size(pci_dev), size);
+val = pci_host_config_read_common(pci_dev, addr, size);
rtas_st(rets, 0, RTAS_OUT_SUCCESS);
rtas_st(rets, 1, val);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 2bae45a..72a1b8b 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -65,7 +65,7 @@ typedef struct PCIHostBridgeClass {
void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
uint32_t limit, uint32_t val, uint32_t len);
uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
- uint32_t limit, uint32_t len);
+ uint32_t len);
void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
--
1.9.3
Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 4/5] qemu-char: convert some open functions to use Error API
Michael Tokarev m...@tls.msk.ru writes:
04.11.2014 16:39, Alex Bennée wrote:
zhanghailiang zhang.zhanghaili...@huawei.com writes:
Convert several Character backend open functions to use the Error API.
Signed-off-by: zhanghailiang zhang.zhanghaili...@huawei.com
---
qemu-char.c | 76
+
1 file changed, 41 insertions(+), 35 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 0f38cdd..a1d25c7 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1077,7 +1077,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in,
int fd_out)
return chr;
}
-static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
+static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts, Error
**errp)
{
int fd_in, fd_out;
char filename_in[CHR_MAX_FILENAME_SIZE];
snip
Why convert the call if we are not using the passed parameter?
This is actually a good question, -- one way or another. On one hand,
this way we're making it all consistent for the caller at least. On
another, this, at least, introduces a warning about unused parameter,
so an 'unused' attribute might be a good idea.
Matter of taste.
As long as the open functions remain different, I don't see the value of
adding unused Error ** parameters.
The churn would save a purpose if it actually unified the function types
and simplified the switch to an indirect call.
Marking unused parameters with __attribute__((unused)) is a good idea
indeed.
[]
-static CharDriverState *qemu_chr_open_win_path(const char *filename)
+static CharDriverState *qemu_chr_open_win_path(const char *filename,
+ Error **errp)
{
CharDriverState *chr;
WinCharState *s;
+Error *local_err = NULL;
chr = qemu_chr_alloc();
s = g_malloc0(sizeof(WinCharState));
@@ -2033,9 +2035,11 @@ static CharDriverState *qemu_chr_open_win_path(const
char *filename)
chr-chr_write = win_chr_write;
chr-chr_close = win_chr_close;
-if (win_chr_init(chr, filename) 0) {
+win_chr_init(chr, filename, local_err);
+if (local_err) {
g_free(s);
g_free(chr);
+error_propagate(errp, local_err);
return NULL;
Hmm I'm not sure I find the change from a return value to
pass-by-reference return value intuitive. What does this gain us?
Are the messages now being reported actually more suitable for user
consumption? For example ClearCommError failed doesn't actually tell
the user much apart from something went wrong.
Alex, I think you're way too late into the game already. This
error api has been designed this way quite some time ago, and
many places uses it this way. I don't really like it too, but
heck, what can I do?
I'm not really happy with it either, but it addresses real problems.
Down where the error happens, you know what went wrong, but have no idea
how to handle it.
Further up where you know how to handle errors, you have no idea what
went wrong.
Passing up success/failure doesn't help with that.
Passing up -errno helps in simple cases.
Inventing other error enumerations tends to produce a mess and/or end in
confusion.
Passing up an error *object* can provide information on what went wrong.
It's admittedly cumbersome, and prone to leak memory. I prefer to stick
to -errno in simple cases.
Our current object is basically a container for a human-readable error
message for error handling to use.
Example: when win_chr_init() fails, it prints to stderr. This is
actually wrong when it runs on behalf of monitor command chardev-add.
It should print to the monitor when it's HMP, and should send an error
reply when it's QMP. win_chr_init() doesn't know. The monitor calling
qmp_chardev_add() does.
I don't actually get it why, when converting some function which
returned success/failure before, to this error API, the return
value is always discarded and the function becomes void? I'd
keep the return value (success/failure) _and_ the error, to
have better shugar in places like this one. But I guess it'll
be a bit more confusing, which condition should be treated as
error - failure function return or non-null error argument.
Yes, this annoys me, too. It can turn a simple
if (frobnicate(arg) 0) {
goto fail;
}
into
Error *local_err = NULL;
frobnicate(arg, local_err);
if (local_err) {
error_free(local_err);
goto fail;
}
when it could be simply
if (frobnicate(arg, NULL) 0) {
goto fail;
}
But this is. again, not about this patch/change -- this is how
qemu is doing for quite some time, discusing/changing this
should be elsewhere.
Moaning about it in review is fine (I indulge in such things myself from
time to time), but if you want infrastructure changed, you probably have
to start with a patch changing it.
[...]
Re: [Qemu-devel] [RFC PATCH] virtio-mmio: support for multiple irqs
Hi Rémy, On 2014/11/5 16:26, GAUGUEY Rémy 228890 wrote: Hi Shannon, Type of backend bandwith(GBytes/sec) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 Test cmd: ./iperf -c 192.168.0.2 -P 1 -i 10 -p 5001 -f G -t 60 Impressive results ! Could you please detail your setup ? which platform are you using and which GbE controller ? Sorry for not telling the test scenario. This test scenario is from Host to Guest. It just compare the performance of different backends. I did this test on ARM64 platform. The setup was based on: 1)on host kvm-arm should support ioeventfd and irqfd The irqfd patch is from Eric ARM: KVM: add irqfd support. http://www.spinics.net/lists/kvm-arm/msg11014.html The ioeventfd patch is reworked by me from Antonios. http://www.spinics.net/lists/kvm-arm/msg08413.html 2)qemu should enable ioeventfd support for virtio-mmio This patch is refer to Ying-Shiuan Pan and reworked for new qemu branch. https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg00594.html 3)qemu should enable multiple irqs for virtio-mmio This patch isn't sent to qemu maillist as we want to check whether this is the right direction. If you want to test, I'll send it to you. 4)in guest should enable support virtio-mmio to request multiple irqs This is what this patch do. As a reference, it would be good also to have result with an iperf to the HOST to see how far we are from a native configuration... Agree! Also, I assume a pending Qemu patch is necessary to assign multiple irqs ? I'm correct ? Yes, the patch is on it's way :) Thanks a lot, Best regards Rémy -Message d'origine- De : Shannon Zhao [mailto:zhaoshengl...@huawei.com] Envoyé : mercredi 5 novembre 2014 09:00 À : linux-ker...@vger.kernel.org Cc : m...@redhat.com; peter.mayd...@linaro.org; john.li...@huawei.com; joel.sch...@amd.com; GAUGUEY Rémy 228890; qemu-devel@nongnu.org; n.nikol...@virtualopensystems.com; virtualizat...@lists.linux-foundation.org; peter.huangp...@huawei.com; hangaoh...@huawei.com Objet : Re: [RFC PATCH] virtio-mmio: support for multiple irqs On 2014/11/4 17:35, Shannon Zhao wrote: As the current virtio-mmio only support single irq, so some advanced features such as vhost-net with irqfd are not supported. And the net performance is not the best without vhost-net and irqfd supporting. Hi Joel, Peter, Mst, Some virtio-net with virtio-mmio performance data on ARM added as followed: Type of backend bandwith(GBytes/sec) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 Test cmd: ./iperf -c 192.168.0.2 -P 1 -i 10 -p 5001 -f G -t 60 From this test data, irqfd has great improvement (about 30%) on performance. So maybe it's necessary to enable multiple irq support to make vhost-net with virtio-mmio on ARM be able to use irqfd. How do you guys think? Look forward for your feedback. Thanks, Shannon This patch support virtio-mmio to request multiple irqs like virtio-pci. With this patch and qemu assigning multiple irqs for virtio-mmio device, it's ok to use vhost-net with irqfd on arm/arm64. . -- Shannon
Re: [Qemu-devel] [RFC PATCH 0/2] virtio-mmio: add irqfd support for vhost-net based on virtio-mmio
On Mon, Oct 27, 2014 at 12:58 PM, Peter Maydell peter.mayd...@linaro.org wrote: On 27 October 2014 11:23, Li Liu john.li...@huawei.com wrote: So you mean virtio-mmio will be replaced by PCI/PCIe on ARM at last? That is the plan, yes. I can't make any promises on timescales at the moment, though... Linaro has scheduled resources to work on this (Ard, cc'ed) and we expect to be able to deliver this within a reasonable time frame. -Christoffer
Re: [Qemu-devel] [PATCH 0/4] ioeventfd support for virtio-mmio
On 2014/11/4 20:47, Shannon Zhao wrote: Add host/guest notifiers support for virtio-mmio, so that qemu can enable vhost-net for kvm-arm. Refer to the patches from Ying-Shiuan Pan https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg00715.html As vhost-net can improve the net performance by about 30%, so I think it's necessary to make virtio-mmio work with vhost-net on arm/arm64. Hi, Some virtio-net with virtio-mmio performance data on ARM added as followed: Type of backend bandwith(GBytes/sec) (From Host to Guest) virtio-net 0.66 vhost-net 1.49 vhost-net with irqfd2.01 Test cmd: ./iperf -c 192.168.0.2 -P 1 -i 10 -p 5001 -f G -t 60 From this test data, vhost-net and irqfd have great improvement on performance. So maybe it's necessary to enable ioeventfd make vhost-net work with virtio-mmio on ARM/ARM64. Look forward to your feedback :) Thanks, Shannon Shannon Zhao (4): virtio-mmio: introduce set_host_notifier() virtio-mmio: introduce set_guest_notifiers virtio-mmio: start ioeventfd when status gets DRIVER_OK virtio-mmio: add a new property for ioeventfd hw/net/virtio-net.c|1 + hw/virtio/virtio-mmio.c| 176 include/hw/virtio/virtio.h |1 + 3 files changed, 178 insertions(+), 0 deletions(-) -- Shannon
Re: [Qemu-devel] [PATCH] Provide the missing LIBUSB_LOG_LEVEL_* for older libusb or FreeBSD. Providing just the needed value as a defined.
On Mi, 2014-11-05 at 19:35 +1100, Chris Johns wrote: +#ifndef LIBUSB_LOG_LEVEL_WARNING /* older libusb didn't define these */ +#define LIBUSB_LOG_LEVEL_WARNING 2 +#endif Added to usb patch queue. thanks, Gerd
[Qemu-devel] [PATCH 2/6] pci: remove pci_config_set_device_id
See also commit 'pci: remove pci_config_set_vendor_id'.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci/pci.c | 2 +-
include/hw/pci/pci.h | 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 6c544ed..e5d192d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -841,7 +841,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
pci_config_alloc(pci_dev);
pci_set_word(pci_dev-config + PCI_VENDOR_ID, pc-vendor_id);
-pci_config_set_device_id(pci_dev-config, pc-device_id);
+pci_set_word(pci_dev-config + PCI_DEVICE_ID, pc-device_id);
pci_config_set_revision(pci_dev-config, pc-revision);
pci_config_set_class(pci_dev-config, pc-class_id);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 390aa83..b659192 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -462,12 +462,6 @@ pci_get_quad(const uint8_t *config)
}
static inline void
-pci_config_set_device_id(uint8_t *pci_config, uint16_t val)
-{
-pci_set_word(pci_config[PCI_DEVICE_ID], val);
-}
-
-static inline void
pci_config_set_revision(uint8_t *pci_config, uint8_t val)
{
pci_set_byte(pci_config[PCI_REVISION_ID], val);
--
1.9.3
[Qemu-devel] [PATCH 3/6] pci: remove pci_config_set_revision
See also commit 'pci: remove pci_config_set_vendor_id'.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci/pci.c | 2 +-
include/hw/pci/pci.h | 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e5d192d..fdab941 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -842,7 +842,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
pci_set_word(pci_dev-config + PCI_VENDOR_ID, pc-vendor_id);
pci_set_word(pci_dev-config + PCI_DEVICE_ID, pc-device_id);
-pci_config_set_revision(pci_dev-config, pc-revision);
+pci_set_byte(pci_dev-config + PCI_REVISION_ID, pc-revision);
pci_config_set_class(pci_dev-config, pc-class_id);
if (!pc-is_bridge) {
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b659192..5106b44 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -462,12 +462,6 @@ pci_get_quad(const uint8_t *config)
}
static inline void
-pci_config_set_revision(uint8_t *pci_config, uint8_t val)
-{
-pci_set_byte(pci_config[PCI_REVISION_ID], val);
-}
-
-static inline void
pci_config_set_class(uint8_t *pci_config, uint16_t val)
{
pci_set_word(pci_config[PCI_CLASS_DEVICE], val);
--
1.9.3
[Qemu-devel] [PATCH 5/6] pci: remove pci_config_set_prog_interface
See also commit 'pci: remove pci_config_set_vendor_id'.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/block/nvme.c| 2 +-
hw/i386/xen/xen_platform.c | 2 +-
hw/i386/xen/xen_pvdevice.c | 2 +-
hw/ide/ich.c | 2 +-
hw/ide/via.c | 2 +-
hw/isa/vt82c686.c | 2 +-
hw/mips/gt64xxx_pci.c | 2 +-
hw/pci-bridge/i82801b11.c | 2 +-
hw/pci-host/bonito.c | 2 +-
include/hw/pci/pci.h | 6 --
10 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 8d7ed78..d9bc149 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -767,7 +767,7 @@ static int nvme_init(PCIDevice *pci_dev)
pci_conf = pci_dev-config;
pci_conf[PCI_INTERRUPT_PIN] = 1;
-pci_config_set_prog_interface(pci_dev-config, 0x2);
+pci_set_byte(pci_dev-config + PCI_CLASS_PROG, 0x2);
pci_set_word(pci_dev-config + PCI_CLASS_DEVICE,
PCI_CLASS_STORAGE_EXPRESS);
pcie_endpoint_cap_init(n-parent_obj, 0x80);
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 28b324a..bcf7038 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -393,7 +393,7 @@ static int xen_platform_initfn(PCIDevice *dev)
pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
-pci_config_set_prog_interface(pci_conf, 0);
+pci_set_byte(pci_conf + PCI_CLASS_PROG, 0);
pci_conf[PCI_INTERRUPT_PIN] = 1;
diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
index c218947..7ebc919 100644
--- a/hw/i386/xen/xen_pvdevice.c
+++ b/hw/i386/xen/xen_pvdevice.c
@@ -88,7 +88,7 @@ static int xen_pv_init(PCIDevice *pci_dev)
pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_MEMORY);
-pci_config_set_prog_interface(pci_conf, 0);
+pci_set_byte(pci_conf + PCI_CLASS_PROG, 0);
pci_conf[PCI_INTERRUPT_PIN] = 1;
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index fb1d095..0263579 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -107,7 +107,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
ahci_init(d-ahci, DEVICE(dev), pci_get_address_space(dev), 6);
-pci_config_set_prog_interface(dev-config, AHCI_PROGMODE_MAJOR_REV_1);
+pci_set_byte(dev-config + PCI_CLASS_PROG, AHCI_PROGMODE_MAJOR_REV_1);
dev-config[PCI_CACHE_LINE_SIZE] = 0x08; /* Cache line size */
dev-config[PCI_LATENCY_TIMER] = 0x00; /* Latency timer */
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 4d8089d..bd80821 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -177,7 +177,7 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
PCIIDEState *d = PCI_IDE(dev);
uint8_t *pci_conf = dev-config;
-pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
+pci_set_byte(pci_conf + PCI_CLASS_PROG, 0x8a); /* legacy ATA mode */
pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
qemu_register_reset(via_reset, d);
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index e0c235c..773d102 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -435,7 +435,7 @@ static int vt82c686b_initfn(PCIDevice *d)
isa_bus = isa_bus_new(d-qdev, pci_address_space_io(d));
pci_conf = d-config;
-pci_config_set_prog_interface(pci_conf, 0x0);
+pci_set_byte(pci_conf + PCI_CLASS_PROG, 0x0);
wmask = d-wmask;
for (i = 0x00; i 0xff; i++) {
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 1f2fe5f..02488b0 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1157,7 +1157,7 @@ static int gt64120_pci_init(PCIDevice *d)
pci_set_word(d-config + PCI_COMMAND, 0);
pci_set_word(d-config + PCI_STATUS,
PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
-pci_config_set_prog_interface(d-config, 0);
+pci_set_byte(d-config + PCI_CLASS_PROG, 0);
pci_set_long(d-config + PCI_BASE_ADDRESS_0, 0x0008);
pci_set_long(d-config + PCI_BASE_ADDRESS_1, 0x0108);
pci_set_long(d-config + PCI_BASE_ADDRESS_2, 0x1c00);
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 14cd7fd..79e5445 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -71,7 +71,7 @@ static int i82801b11_bridge_initfn(PCIDevice *d)
if (rc 0) {
goto err_bridge;
}
-pci_config_set_prog_interface(d-config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
+pci_set_byte(d-config + PCI_CLASS_PROG, PCI_CLASS_BRIDGE_PCI_INF_SUB);
return 0;
err_bridge:
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index 56292ad..061fe8c 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -712,7 +712,7 @@ static int bonito_initfn(PCIDevice *dev)
PCIHostState *phb = PCI_HOST_BRIDGE(s-pcihost);
/* Bonito North Bridge, built on FPGA, VENDOR_ID/DEVICE_ID are undefined
*/
-pci_config_set_prog_interface(dev-config, 0x00);
+pci_set_byte(dev-config + PCI_CLASS_PROG, 0x00);
/* set the north bridge register mapping */
[Qemu-devel] [PATCH 4/6] pci: remove pci_config_set_class
See also commit 'pci: remove pci_config_set_vendor_id'.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/block/nvme.c| 2 +-
hw/pci-host/ppce500.c | 2 +-
hw/pci/pci.c | 2 +-
hw/pci/pci_bridge.c| 2 +-
hw/virtio/virtio-pci.c | 2 +-
include/hw/pci/pci.h | 6 --
6 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b6263dc..8d7ed78 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -768,7 +768,7 @@ static int nvme_init(PCIDevice *pci_dev)
pci_conf = pci_dev-config;
pci_conf[PCI_INTERRUPT_PIN] = 1;
pci_config_set_prog_interface(pci_dev-config, 0x2);
-pci_config_set_class(pci_dev-config, PCI_CLASS_STORAGE_EXPRESS);
+pci_set_word(pci_dev-config + PCI_CLASS_DEVICE,
PCI_CLASS_STORAGE_EXPRESS);
pcie_endpoint_cap_init(n-parent_obj, 0x80);
n-num_namespaces = 1;
diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index 1b4c0f0..aa4ed76 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -337,7 +337,7 @@ static int e500_pcihost_bridge_initfn(PCIDevice *d)
PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
/e500-ccsr));
-pci_config_set_class(d-config, PCI_CLASS_BRIDGE_PCI);
+pci_set_word(d-config + PCI_CLASS_DEVICE, PCI_CLASS_BRIDGE_PCI);
d-config[PCI_HEADER_TYPE] =
(d-config[PCI_HEADER_TYPE] PCI_HEADER_TYPE_MULTI_FUNCTION) |
PCI_HEADER_TYPE_BRIDGE;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index fdab941..05f8c9e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -843,7 +843,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
pci_set_word(pci_dev-config + PCI_VENDOR_ID, pc-vendor_id);
pci_set_word(pci_dev-config + PCI_DEVICE_ID, pc-device_id);
pci_set_byte(pci_dev-config + PCI_REVISION_ID, pc-revision);
-pci_config_set_class(pci_dev-config, pc-class_id);
+pci_set_word(pci_dev-config + PCI_CLASS_DEVICE, pc-class_id);
if (!pc-is_bridge) {
if (pc-subsystem_vendor_id || pc-subsystem_id) {
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 40c97b1..04a5c10 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -350,7 +350,7 @@ int pci_bridge_initfn(PCIDevice *dev, const char *typename)
*PCI_COMMAND_VGA_PALETTE);
*/
-pci_config_set_class(dev-config, PCI_CLASS_BRIDGE_PCI);
+pci_set_word(dev-config + PCI_CLASS_DEVICE, PCI_CLASS_BRIDGE_PCI);
dev-config[PCI_HEADER_TYPE] =
(dev-config[PCI_HEADER_TYPE] PCI_HEADER_TYPE_MULTI_FUNCTION) |
PCI_HEADER_TYPE_BRIDGE;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index dde1d73..c1bf96c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -965,7 +965,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
config = proxy-pci_dev.config;
if (proxy-class_code) {
-pci_config_set_class(config, proxy-class_code);
+pci_set_word(config + PCI_CLASS_DEVICE, proxy-class_code);
}
pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
pci_get_word(config + PCI_VENDOR_ID));
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 5106b44..eb9a2c3 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -462,12 +462,6 @@ pci_get_quad(const uint8_t *config)
}
static inline void
-pci_config_set_class(uint8_t *pci_config, uint16_t val)
-{
-pci_set_word(pci_config[PCI_CLASS_DEVICE], val);
-}
-
-static inline void
pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val)
{
pci_set_byte(pci_config[PCI_CLASS_PROG], val);
--
1.9.3
[Qemu-devel] [PATCH 1/6] pci: remove pci_config_set_vendor_id
Use pci_set_word() instead. The main purpose is for API
consistency. Detailed reasons:
- pci_config_set_* are not complete. 1) Only part of registers in
predefined header portion of PCI Configuration Space are
supported. 2) Lack of get_ counterparts.
- pci_set_word() and friends are extensively used in qemu. They
are used both for predefined registers and device specific
registers.
- another option is to complete the pci_config_set_* for all
predefined registers (though they will co-exist with pci_set_*).
Hence the RFC.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/pci/pci.c | 2 +-
include/hw/pci/pci.h | 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..6c544ed 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -840,7 +840,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
*pci_dev, PCIBus *bus,
pci_dev-irq_state = 0;
pci_config_alloc(pci_dev);
-pci_config_set_vendor_id(pci_dev-config, pc-vendor_id);
+pci_set_word(pci_dev-config + PCI_VENDOR_ID, pc-vendor_id);
pci_config_set_device_id(pci_dev-config, pc-device_id);
pci_config_set_revision(pci_dev-config, pc-revision);
pci_config_set_class(pci_dev-config, pc-class_id);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c352c7b..390aa83 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -462,12 +462,6 @@ pci_get_quad(const uint8_t *config)
}
static inline void
-pci_config_set_vendor_id(uint8_t *pci_config, uint16_t val)
-{
-pci_set_word(pci_config[PCI_VENDOR_ID], val);
-}
-
-static inline void
pci_config_set_device_id(uint8_t *pci_config, uint16_t val)
{
pci_set_word(pci_config[PCI_DEVICE_ID], val);
--
1.9.3
[Qemu-devel] [PULL 2/2] s390x: Implement SAM{24,31,64}
The SAM instructions simply change 2 bits in PSW.MASK to advertise
the current memory mode. While we can't fully guarantee that 31 bit
mode (or even remotely 24 bit mode) actually work correctly, we don't
check whether lpswe modifies these bits, so we shouldn't keep the
guest from executing SAM instructions either.
This patch implements all SAM instrutions with their actual PSW changing
semantics, making more recent Linux kernels boot properly which do issue
a SAM31 call during early boot.
Signed-off-by: Alexander Graf ag...@suse.de
Reviewed-by: Bastian Koppelmann kbast...@mail.uni-paderborn.de
Reviewed-by: Richard Henderson r...@twiddle.net
---
target-s390x/insn-data.def | 6 +++---
target-s390x/translate.c | 12
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/target-s390x/insn-data.def b/target-s390x/insn-data.def
index b42ebb6..4d2feb6 100644
--- a/target-s390x/insn-data.def
+++ b/target-s390x/insn-data.def
@@ -744,9 +744,9 @@
/* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
C(0xb220, SERVC, RRE, Z, r1_o, r2_o, 0, 0, servc, 0)
/* SET ADDRESSING MODE */
-/* We only do 64-bit, so accept this as a no-op.
- Let SAM24 and SAM31 signal illegal instruction. */
-C(0x010e, SAM64, E, Z, 0, 0, 0, 0, 0, 0)
+D(0x010c, SAM24, E, Z, 0, 0, 0, 0, sam, 0, 0)
+D(0x010d, SAM31, E, Z, 0, 0, 0, 0, sam, 0, 1)
+D(0x010e, SAM64, E, Z, 0, 0, 0, 0, sam, 0, 3)
/* SET ADDRESS SPACE CONTROL FAST */
C(0xb279, SACF,S, Z, 0, a2, 0, 0, sacf, 0)
/* SET CLOCK */
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 0cb036f..827cda4 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2927,6 +2927,18 @@ static ExitStatus op_sacf(DisasContext *s, DisasOps *o)
}
#endif
+static ExitStatus op_sam(DisasContext *s, DisasOps *o)
+{
+int sam = s-insn-data;
+TCGv_i64 tsam = tcg_const_i64(sam);
+
+/* Overwrite PSW_MASK_64 and PSW_MASK_32 */
+tcg_gen_deposit_i64(psw_mask, psw_mask, tsam, 31, 2);
+
+tcg_temp_free_i64(tsam);
+return EXIT_PC_STALE;
+}
+
static ExitStatus op_sar(DisasContext *s, DisasOps *o)
{
int r1 = get_field(s-fields, r1);
--
1.7.12.4
[Qemu-devel] [PULL 2.2 0/2] s390 patch queue 2014-11-05 for 2.2
Hi Peter,
This is my current patch queue for s390. Please pull.
Alex
The following changes since commit d5b4dc3b50175f0c34f3cf4b053e123fb37f5aed:
Merge remote-tracking branch 'remotes/afaerber/tags/qom-devices-for-peter'
into staging (2014-11-04 17:33:34 +)
are available in the git repository at:
git://github.com/agraf/qemu.git tags/signed-s390-for-upstream
for you to fetch changes up to ab6b1c0a36b23e3e8c47c00fb6c9931de957f608:
s390x: Implement SAM{24,31,64} (2014-11-05 10:54:28 +0100)
Patch queue for s390 - 2014-11-05
Two simple bug fixes to enable slightly newer guest kernels
and preliminary -M s390-ccw support for TCG (virtio doesn't work yet!)
Alexander Graf (2):
s390x: Fix sclp console input
s390x: Implement SAM{24,31,64}
target-s390x/insn-data.def | 6 +++---
target-s390x/interrupt.c | 2 --
target-s390x/translate.c | 12
3 files changed, 15 insertions(+), 5 deletions(-)
[Qemu-devel] [PATCH 6/6] pci: remove pci_config_set_interrupt_pin
See also commit 'pci: remove pci_config_set_vendor_id'.
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
hw/audio/intel-hda.c | 2 +-
hw/i2c/smbus_ich9.c | 2 +-
hw/ide/ich.c | 2 +-
hw/isa/i82378.c | 2 +-
hw/misc/ivshmem.c| 2 +-
hw/misc/vfio.c | 2 +-
hw/scsi/vmw_pvscsi.c | 2 +-
hw/usb/hcd-uhci.c| 2 +-
include/hw/pci/pci.h | 6 --
9 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 2885231..50ff3dd 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1133,7 +1133,7 @@ static int intel_hda_init(PCIDevice *pci)
d-name = object_get_typename(OBJECT(d));
-pci_config_set_interrupt_pin(conf, 1);
+pci_set_byte(conf + PCI_INTERRUPT_PIN, 1);
/* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */
conf[0x40] = 0x01;
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 0803dc4..ce14450 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -76,7 +76,7 @@ static int ich9_smbus_initfn(PCIDevice *d)
ICH9SMBState *s = ICH9_SMB_DEVICE(d);
/* TODO? D31IP.SMIP in chipset configuration space */
-pci_config_set_interrupt_pin(d-config, 0x01); /* interrupt pin 1 */
+pci_set_byte(d-config + PCI_INTERRUPT_PIN, 0x01); /* interrupt pin 1 */
pci_set_byte(d-config + ICH9_SMB_HOSTC, 0);
/* TODO bar0, bar1: 64bit BAR support*/
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 0263579..0be60f6 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -111,7 +111,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
dev-config[PCI_CACHE_LINE_SIZE] = 0x08; /* Cache line size */
dev-config[PCI_LATENCY_TIMER] = 0x00; /* Latency timer */
-pci_config_set_interrupt_pin(dev-config, 1);
+pci_set_byte(dev-config + PCI_INTERRUPT_PIN, 1);
/* XXX Software should program this register */
dev-config[0x90] = 1 6; /* Address Map Register - AHCI mode */
diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index a7d9aa6..7399121 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -73,7 +73,7 @@ static int i82378_initfn(PCIDevice *pci)
pci_set_word(pci_conf + PCI_STATUS,
PCI_STATUS_DEVSEL_MEDIUM);
-pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */
+pci_set_byte(pci_conf + PCI_INTERRUPT_PIN, 1); /* interrupt pin 0 */
isabus = isa_bus_new(dev, pci_address_space_io(pci));
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 5d272c8..06573ea 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -747,7 +747,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
pci_conf = dev-config;
pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
-pci_config_set_interrupt_pin(pci_conf, 1);
+pci_set_byte(pci_conf + PCI_INTERRUPT_PIN, 1);
s-shm_fd = 0;
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index fd318a1..151d77e 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -561,7 +561,7 @@ static int vfio_enable_intx(VFIODevice *vdev)
vfio_disable_interrupts(vdev);
vdev-intx.pin = pin - 1; /* Pin A (1) - irq[0] */
-pci_config_set_interrupt_pin(vdev-pdev.config, pin);
+pci_set_byte(vdev-pdev.config + PCI_INTERRUPT_PIN, pin);
#ifdef CONFIG_KVM
/*
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index d3a92fb..6f3a48d 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1077,7 +1077,7 @@ pvscsi_init(PCIDevice *pci_dev)
pci_dev-config[PCI_LATENCY_TIMER] = 0xff;
/* Interrupt pin A */
-pci_config_set_interrupt_pin(pci_dev-config, 1);
+pci_set_byte(pci_dev-config + PCI_INTERRUPT_PIN, 1);
memory_region_init_io(s-io_space, OBJECT(s), pvscsi_ops, s,
pvscsi-io, PVSCSI_MEM_SPACE_SIZE);
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 4a4215d..4e2efd9 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1202,7 +1202,7 @@ static int usb_uhci_common_initfn(PCIDevice *dev)
/* TODO: reset value should be 0. */
pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
-pci_config_set_interrupt_pin(pci_conf, u-info.irq_pin + 1);
+pci_set_byte(pci_conf + PCI_INTERRUPT_PIN, u-info.irq_pin + 1);
if (s-masterbus) {
USBPort *ports[NB_PORTS];
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 1294e23..1da4be7 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -461,12 +461,6 @@ pci_get_quad(const uint8_t *config)
return le64_to_cpup((const uint64_t *)config);
}
-static inline void
-pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val)
-{
-pci_set_byte(pci_config[PCI_INTERRUPT_PIN], val);
-}
-
/*
* helper functions to do bit mask operation on configuration space.
* Just to set bit, use test-and-set and discard returned value.
--
1.9.3
[Qemu-devel] [PULL 1/2] s390x: Fix sclp console input
When injecting an sclp console interrupt into the guest, we increase
the PC by 4 for some reason. I have no idea why I put that code there,
but it's clearly wrong. Remove the increment.
This patch fixes sclp serial input for the ccw machine.
Signed-off-by: Alexander Graf ag...@suse.de
Reviewed-by: Bastian Koppelmann kbast...@mail.uni-paderborn.de
---
target-s390x/interrupt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/target-s390x/interrupt.c b/target-s390x/interrupt.c
index 23a9114..1404d0a 100644
--- a/target-s390x/interrupt.c
+++ b/target-s390x/interrupt.c
@@ -22,9 +22,7 @@ void s390_sclp_extint(uint32_t parm)
kvm_s390_service_interrupt(parm);
} else {
S390CPU *dummy_cpu = s390_cpu_addr2state(0);
-CPUS390XState *env = dummy_cpu-env;
-env-psw.addr += 4;
cpu_inject_ext(dummy_cpu, EXT_SERVICE, parm, 0);
}
}
--
1.7.12.4
Re: [Qemu-devel] [Linaro-acpi] [RFC PATCH 0/7] hw/arm/virt: Dynamic ACPI v5.1 table generation
Hi all, On 30.10.2014 19:02, Mark Rutland wrote: On Thu, Oct 30, 2014 at 05:52:44PM +, Peter Maydell wrote: On 30 October 2014 17:43, Alexander Spyridakis a.spyrida...@virtualopensystems.com wrote: Currently, the virt machine model generates Device Tree information dynamically based on the existing devices in the system. This patch series extends the same concept but for ACPI information instead. A total of seven tables have been implemented in this patch series, which is the minimum for a basic ARM support. The set of generated tables are: - RSDP - XSDT - MADT - GTDT - FADT - FACS - DSDT The tables are created in standalone buffers, taking into account the needed information passed from the virt machine model. When the generation is finalized, the individual buffers are compacted to a single ACPI binary blob, where it is injected on the guest memory space in a fixed location. The guest kernel can find the ACPI tables by providing to it the physical address of the ACPI blob (e.g. acpi_rsdp=0x4700 boot argument). (Sorry, I should have waited for the cover letter to arrive before replying.) I think this is definitely the wrong approach. We already have to generate device tree information for the hardware we have, and having an equivalent parallel infrastructure for generating ACPI as well seems like it would be a tremendous mess. We should support guests that require ACPI by having QEMU boot a UEFI bios blob and have that UEFI code generate ACPI tables based on the DTB we hand it. (Chances seem good that any guest that wants ACPI is going to want UEFI runtime services anyway.) Depending on why people want ACPI in a guest environment, generating ACPI tables from a DTB might not be possible (e.g. if they want to use AML for some reason). So the important question is _why_ the guest needs to see an ACPI environment. What exactly can ACPI provide to the guest that DT does not already provide, and why is that necessary? What infrastrucutre is needed for that use case? Translating DT tables into the equivalent ACPI tables seems like a waste of effort unless it enables something we can't do at the moment. Thanks, Mark. Please correct me if I am wrong, my understanding at the moment is that for X86 there is an ACPI implementation in hw/acpi, with the table generation happening in hw/i386/acpi-build.c . Couldn't there be some unification where part of the infrastructure for ACPI is reused, with arch-specific code specializing for X86 and ARM? Why are ACPI tables created for X86, but cannot be created likewise for ARM? We need ACPI guest support in QEMU for AArch64 over here, with all features (including the ability to run ACPI code and add specific tables), for ACPI-based guests. Thanks, Claudio
[Qemu-devel] [RFC PATCH 0/6] pci cleanup: remove pci_config_set_*
Hi, This series removes pci_config_set_*. The main purpose is for API consistency. Detailed reasons: - pci_config_set_* are not complete. 1) Only part of registers in predefined header portion of PCI Configuration Space are supported. 2) Lack of get_ counterparts. - pci_set_word() and friends are extensively used in qemu. They are used both for predefined registers and device specific registers. - another option is to complete the pci_config_set_* for all predefined registers (though they will co-exist with pci_set_*). Hence the RFC. Hu Tao (6): pci: remove pci_config_set_vendor_id pci: remove pci_config_set_device_id pci: remove pci_config_set_revision pci: remove pci_config_set_class pci: remove pci_config_set_prog_interface pci: remove pci_config_set_interrupt_pin hw/audio/intel-hda.c | 2 +- hw/block/nvme.c| 4 ++-- hw/i2c/smbus_ich9.c| 2 +- hw/i386/xen/xen_platform.c | 2 +- hw/i386/xen/xen_pvdevice.c | 2 +- hw/ide/ich.c | 4 ++-- hw/ide/via.c | 2 +- hw/isa/i82378.c| 2 +- hw/isa/vt82c686.c | 2 +- hw/mips/gt64xxx_pci.c | 2 +- hw/misc/ivshmem.c | 2 +- hw/misc/vfio.c | 2 +- hw/pci-bridge/i82801b11.c | 2 +- hw/pci-host/bonito.c | 2 +- hw/pci-host/ppce500.c | 2 +- hw/pci/pci.c | 8 hw/pci/pci_bridge.c| 2 +- hw/scsi/vmw_pvscsi.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/virtio/virtio-pci.c | 2 +- include/hw/pci/pci.h | 36 21 files changed, 25 insertions(+), 61 deletions(-) -- 1.9.3
[Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev
pci_qdev_init() checks whether return value is 0 or not to figure out pci
device is initialized successfully. Otherwise, device_realize() in qdev checks
that return value is negative value to figure out the device is realized
successfully.
When pci device returns positive number, pci_qdev_init() thinks that error is
occured and makes the device unregistered. Nevertheless, qdev thinks that
device is realized.
Finally, crash is occured by commands like 'qtree' that traverse qdev list.
So, pci_qdev_init() returns -1 when init function returns not 0.
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
---
hw/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..c149fdf 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
rc = pc-init(pci_dev);
if (rc != 0) {
do_pci_unregister_device(pci_dev);
-return rc;
+return -1;
}
}
--
2.1.0
Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it
Hi,
My proposal to ditch image contents probing entirely has more serious
compatibility issues. In particular, we'd have to forgo sugared
convenience syntax for a number of less common things. It definitely
needs a grace period where all usage we're going to break warns. On the
up side, it will actually be secure by default when it's done.
This makes most sense to me. We can even have a config option to
control this, i.e. something like ...
-guessformat={allow-content,allow-content-with-warning,filename-only,off}
... and over time we'll make things more strict by default.
People can tweak things locally via cfg file in /etc/qemu if they wish.
cheers,
Gerd
Re: [Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing
On 11/05/2014 09:39 AM, Markus Armbruster wrote: Hm... In which cases does libvirt probe the image format? And is it even consistent with qemu today? I had a quick look at the source. Eric, please correct misunderstandings. Enumation type virStorageFileProbeFormat enumerates supported formats. It has pseudo-formats VIR_STORAGE_FILE_AUTO, VIR_STORAGE_FILE_AUTO_SAFE. VIR_STORAGE_FILE_AUTO is the format that libvirt assigns an image where the libvirt XML was not explicit. VIR_STORAGE_FILE_AUTO_SAFE is what the image gets reassigned to for QED (as the only format where the backing format is mandatory as part of the backing chain), which basically says: trust the backing chain if the XML omitted a type, instead of doing the normal rules of auto. I don't understand VIR_STORAGE_FILE_AUTO_SAFE offhand. VIR_STORAGE_FILE_AUTO means probing. Its use appears to be deprecated. Close. It actually means: the user did not specify a format in their XML, so it is now up to a configuration knob whether we will probe or whether we will forcefully error out and tell the user to fix their XML. The configuration knob (allow_disk_format_probing in /etc/libvirt/qemu.conf) defaults to 0 by default (error out and tell the user to fix their XML) but can be overridden to 1 by someone that knows what they are doing (probes are allowed at the user's own risk). Actual probing happens in virStorageFileProbeFormatFromBuf(): For all formats: if magic and version match, pick this format If some magic matched, but not the version: warn For all formats: if file name extension matches, pick this format Pick raw. The formats' magic, version and extension are defined in fileTypeInfo[]. If I remember correctly, libvirt has its own probing because running an external program just to probe is too slow. Correct. And while the libvirt probing was originally modeled after qemu, the two approaches have probably diverged a bit over time. An obvious difference: qemu only probes 512? bytes (maybe 4096?), but libvirt probes deep enough to determine the .iso file format (a 5-byte magic number at decimal offset 32769). See VIR_STORAGE_MAX_HEADER 0x8200 in src/util/virstoragefile.h. Another reason for having its own probing might be providing a secure replacement for QEMU's insecure probing. While that may be a possible outcome, I'm not sure it was an intentional design. If you can get libvirt to explicitly pass the wrong format=... option because it did its own probing, we have a problem no matter what we change in qemu. Yes, but that would be a libvirt problem. No excuse for us to ignore our own problems. Correct - for the purposes of this thread, we need only figure out how to make qemu closer to being secure by default. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RESEND PATCH v4 08/10] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.
On Wed, 5 Nov 2014 13:49:53 +0800
Tang Chen tangc...@cn.fujitsu.com wrote:
From: Hu Tao hu...@cn.fujitsu.com
Implement unrealize function for pc-dimm device. It remove subregion from
hotplug region, and delete ram address range from guest ram list.
This still doesn't address comments made in V3
looks like there isn't any need for unrealize so far
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
---
hw/mem/pc-dimm.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index ee802bb..b105871 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -270,12 +270,22 @@ static MemoryRegion
*pc_dimm_get_memory_region(PCDIMMDevice *dimm)
return host_memory_backend_get_memory(dimm-hostmem, error_abort);
}
+static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
+{
+PCDIMMDevice *dimm = PC_DIMM(dev);
+MemoryRegion *mr = pc_dimm_get_memory_region(dimm);
+
+memory_region_del_subregion(mr-container, mr);
you wouldn't need to access fields if it's done in unplug handler
and you shouldn't access MemoryRegion fields directly in the first place
+vmstate_unregister_ram(mr, dev);
all above should be done unplug handler by pc-machine
+}
+
static void pc_dimm_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
dc-realize = pc_dimm_realize;
+dc-unrealize = pc_dimm_unrealize;
dc-props = pc_dimm_properties;
ddc-get_memory_region = pc_dimm_get_memory_region;
Re: [Qemu-devel] [PATCH v4 6/6] hw/arm/virt: add dynamic sysbus device support
On 31.10.14 14:53, Eric Auger wrote:
Allows sysbus devices to be instantiated from command line by
using -device option. Machvirt creates a platform bus at init.
The dynamic sysbus devices are attached to a platform bus device.
The platform bus device registers a machine init done notifier
whose role will be to bind the dynamic sysbus devices. Indeed
dynamic sysbus devices are created after machine init.
machvirt also registers a notifier that will start the VFIO
dynamic device IRQ handling.
Signed-off-by: Alexander Graf ag...@suse.de
Signed-off-by: Eric Auger eric.au...@linaro.org
---
v3 - v4:
- use platform bus object, instantiated in create_platform_bus
- device tree generation for platform bus and children dynamic
sysbus devices is no more handled at reset but in a
machine_init_done_notifier (due to the change in implementaion
of ARM load dtb using rom_add_blob_fixed).
- device tree enhancement now takes into account the case of
user provided dtb. Before the user dtb was overwritten which
was wrong. However in case the dtb is provided by the user,
dynamic sysbus nodes are not added there.
- renaming of MACHVIRT_PLATFORM defines
- MACHVIRT_PLATFORM_PAGE_SHIFT and SIZE_PAGES not needed anymore,
hence removed.
- DynSysbusParams struct renamed into ARMPlatformBusSystemParams
and above params removed.
- separation of dt creation and QEMU binding is not mandated anymore
since the device tree is not created from scratch anymore. Instead
the modify_dtb function is used.
- create_platform_bus registers another machine init done notifier
to start VFIO IRQ handling. This latter executes after the
dynamic sysbus device binding.
v2 - v3:
- renaming of arm_platform_bus_create_devtree and arm_load_dtb
- add copyright in hw/arm/dyn_sysbus_devtree.c
v1 - v2:
- remove useless vfio-platform.h include file
- s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
- use dyn_sysbus_binding and dyn_sysbus_devtree
- dynamic sysbus platform buse size shrinked to 4MB and
moved between RTC and MMIO
v1:
Inspired from what Alex Graf did in ppc e500
https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
Conflicts:
hw/arm/sysbus-fdt.c
---
hw/arm/virt.c | 59
+++
1 file changed, 59 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 78f618d..3a09d58 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -42,6 +42,8 @@
#include exec/address-spaces.h
#include qemu/bitops.h
#include qemu/error-report.h
+#include hw/arm/sysbus-fdt.h
+#include hw/platform-bus.h
#define NUM_VIRTIO_TRANSPORTS 32
@@ -59,6 +61,11 @@
#define GIC_FDT_IRQ_PPI_CPU_START 8
#define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
+#define PLATFORM_BUS_BASE 0x940
+#define PLATFORM_BUS_SIZE (4ULL * 1024 * 1024)
+#define PLATFORM_BUS_FIRST_IRQ48
+#define PLATFORM_BUS_NUM_IRQS 20
+
enum {
VIRT_FLASH,
VIRT_MEM,
@@ -68,6 +75,7 @@ enum {
VIRT_UART,
VIRT_MMIO,
VIRT_RTC,
+VIRT_PLATFORM_BUS,
};
typedef struct MemMapEntry {
@@ -107,6 +115,7 @@ static const MemMapEntry a15memmap[] = {
[VIRT_GIC_CPU] ={ 0x0801, 0x0001 },
[VIRT_UART] = { 0x0900, 0x1000 },
[VIRT_RTC] ={ 0x0901, 0x1000 },
+[VIRT_PLATFORM_BUS] = {PLATFORM_BUS_BASE , PLATFORM_BUS_SIZE},
[VIRT_MMIO] = { 0x0a00, 0x0200 },
/* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
*/
/* 0x1000 .. 0x4000 reserved for PCI */
@@ -117,6 +126,14 @@ static const int a15irqmap[] = {
[VIRT_UART] = 1,
[VIRT_RTC] = 2,
[VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
+[VIRT_PLATFORM_BUS] = PLATFORM_BUS_FIRST_IRQ,
+};
+
+ARMPlatformBusSystemParams platform_bus_params = {
static const?
Alex
Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it
On 11/05/2014 09:38 AM, Max Reitz wrote: Note that specifying just the top image's format is not enough, you also have to specify any backing images' formats. QCOW2 can optionally store the backing image format in the image. The other COW formats can't. Well, they can, with json:. *cough* Example of insecure usage: -hda bar.vmdk, where bar.vmdk is a VMDK image with a raw backing file. Yesterday I found out that doesn't seem possible. You apparently can only use VMDK with VMDK backing files. Other than that, we only have qcow1 and qed as COW formats which should not be used anyway. Actually, qed requires the backing format to be recorded (it is non-optional) and is therefore immune to probing problems of backing files. That's one thing it got right. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition
On 31.10.14 14:53, Eric Auger wrote:
This new C module will be used by ARM machine files to generate
platform bus node and their dynamic sysbus device tree nodes.
Dynamic sysbus device node addition is done in a machine init
done notifier. arm_register_platform_bus_fdt_creator does the
registration of this latter and is supposed to be called by
ARM machine files that support platform bus and their dynamic
sysbus. Addition of dynamic sysbus nodes is done only if the
user did not provide any dtb.
Signed-off-by: Alexander Graf ag...@suse.de
Signed-off-by: Eric Auger eric.au...@linaro.org
---
v3 - v4:
- dyn_sysbus_devtree.c renamed into sysbus-fdt.c
- use new PlatformBusDevice object
- the dtb upgrade is done through modify_dtb. Before the fdt
was recreated from scratch. When the user provided a dtb this
latter was overwritten which was not correct.
- an array contains the association between device type names
and their node creation function
- I must aknowledge I did not find any cleaner way to implement
a FDT_BUILDER interface, as suggested by Paolo. The class method
would need to be initialized somewhere and since it cannot
happen in the device itself - according to Alex Peter comments -,
I don't see when I shall associate the device type and its
interface implementation.
v2 - v3:
- add arm_ prefix
- arm_sysbus_device_create_devtree becomes static
v1 - v2:
- Code moved in an arch specific file to accomodate architecture
dependent specificities.
- remove platform_bus_base from PlatformDevtreeData
v1: code originally written by Alex Graf in e500.c and reused for
ARM [Eric Auger]
---
hw/arm/Makefile.objs| 1 +
hw/arm/sysbus-fdt.c | 181
include/hw/arm/sysbus-fdt.h | 50
3 files changed, 232 insertions(+)
create mode 100644 hw/arm/sysbus-fdt.c
create mode 100644 include/hw/arm/sysbus-fdt.h
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 6088e53..0cc63e1 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -3,6 +3,7 @@ obj-$(CONFIG_DIGIC) += digic_boards.o
obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
+obj-y += sysbus-fdt.o
obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
obj-$(CONFIG_DIGIC) += digic.o
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
new file mode 100644
index 000..d5476f1
--- /dev/null
+++ b/hw/arm/sysbus-fdt.c
@@ -0,0 +1,181 @@
+/*
+ * ARM Platform Bus device tree generation helpers
+ *
+ * Copyright (c) 2014 Linaro Limited
+ *
+ * Authors:
+ * Alex Graf ag...@suse.de
+ * Eric Auger eric.au...@linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
with
+ * this program. If not, see http://www.gnu.org/licenses/.
+ *
+ */
+
+#include hw/arm/sysbus-fdt.h
+#include qemu/error-report.h
+#include sysemu/device_tree.h
+#include hw/platform-bus.h
+#include sysemu/sysemu.h
+#include hw/platform-bus.h
+
+/*
+ * internal struct that contains the information to create dynamic
+ * sysbus device node
+ */
+typedef struct PlatformBusFdtData {
+void *fdt; /* device tree handle */
+int irq_start; /* index of the first IRQ usable by platform bus devices
*/
+const char *pbus_node_name; /* name of the platform bus node */
+PlatformBusDevice *pbus;
+} PlatformBusFdtData;
+
+/*
+ * struct used when calling the machine init done notifier
+ * that constructs the fdt nodes of platform bus devices
+ */
+typedef struct PlatformBusFdtNotifierParams {
+ARMPlatformBusFdtParams *fdt_params;
+Notifier notifier;
+} PlatformBusFdtNotifierParams;
+
+/* struct that associates a device type name and a node creation function */
+typedef struct NodeCreationPair {
+const char *typename;
+int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
+} NodeCreationPair;
+
+/* list of supported dynamic sysbus devices */
+NodeCreationPair add_fdt_node_functions[] = {
+{, NULL}, /*last element*/
s/// :)
Alex
Re: [Qemu-devel] [PATCH v4 0/6] machvirt dynamic sysbus device instantiation
On 31.10.14 14:53, Eric Auger wrote: This patch series enables machvirt to dynamically instantiate sysbus devices from command line (using -device option). All those sysbus devices are plugged onto a platform bus. This latter device is instantiated in machvirt and takes care of the binding of children sysbus devices on a machine init done notifier. The device tree node generation for children dynamic sysbus device also happens on a subsequent notifier that must be executed after the above one. machvirt registers that notifier before the platform bus creation to make sure notifiers are executed in the right order: dt generation after actual QOM binding. Very few sysbus devices are supposed to be instantiated that way. VFIO devices belong to them. Node creation really is architecture specific. On ARM the dynamic sysbus device node creation is implemented in a new C module, hw/arm/sysbus-fdt.c and not in the machine file. This series applies on top of Alex Graf's series [PATCH v3 0/7] Dynamic sysbus device allocation support http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04860.html Machvirt transformations and sysbus-fdt are largely inspired from Alex work. The patch series can be found at: http://git.linaro.org/people/eric.auger/qemu.git (branch vfio_integ_v7) Overall the approach looks sane to me. I'm not 100% convinced it's a good idea to make the fdt generation arm generic, but I'll leave that for Peter to decide. On PPC this definitely wouldn't fly with all the different subarchs. I'm not sure whether ARM is more standardized on the device tree generation, especially with different #address-size properties and the likes. Alex
Re: [Qemu-devel] [RFC PATCH 0/8] Add Generic PCI host device update
Hi Alvise, On 11.07.2014 09:21, Alvise Rigo wrote: This patch series is based on the previous work [1] and [2] by Rob Herring and it tries to enhance this work on these points: do your patches need to be applied on top of Rob's? I ask because I cannot see the patch hw/arm/virt: Add generic PCI host device and among these, as wll as the hw/pci-host: add a generic PCI host. I think those two need modifications as well, as they hardcode addresses, and also try to register the PCI bus as a PCIE bus: does it really provide a PCI-Express? (probably harmless but still would benefit from review). - Some of the hardcoded values have been moved to an header file. This header file is also used to share some device structures with the mach-virt machine. Some additional hardcoded addresses have been also introduced with your changes though. (I'll post comments to the the patches in the series momentarily). - The interrupt-map dt node generation has been revisited; it is now done after the generic devices init so that it's possible to attach PCI devices by mean of the qdev infrastructure. This allows to have several devices in the PCI bus, with the current limitation of one interrupt per PCI slot. Probably the most objectionable part of these patches regards the way some data and definitions have been shared between the machine and the device code; a better solution is still under evaluation. Any advice on this and on the rest of the work is highly appreciated. This work has been tested attaching several PCI devices to the mach-virt platform. The tested devices are: virtio-blk-pci, virtio-net-pci, lsi53c895a and pci-ohci (all attached at the same time). Even if the original work was not changed in its core functionalities, I couldn't reproduce the malfunctioning of the LSI SCSI mentioned in [1]. After attaching several qcow2 images, formatting and filling them, I didn't notice anything wrong. Am I missing something? Thank you, alvise [1] [Qemu-devel] [RFC PATCH 1/2] hw/pci-host: add a generic PCI host http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html [2] [Qemu-devel] [RFC PATCH 2/2] hw/arm/virt: Add generic PCI host device http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html Alvise Rigo (8): mach-virt: move GIC inside mach-virt structure mach-virt: improve PCI memory topology definition QEMUMachine: finalize_dt function generic_pci: create header file generic_pci: create own map irq function generic_pci: generate dt node after devices init generic_pci: realize device with machine data generic_pci: add interrupt map structures hw/arm/virt.c | 110 +++- hw/pci-host/generic-pci.c | 173 +- include/hw/boards.h | 4 + include/hw/pci-host/pci_generic.h | 66 +++ vl.c | 5 ++ 5 files changed, 277 insertions(+), 81 deletions(-) create mode 100644 include/hw/pci-host/pci_generic.h
Re: [Qemu-devel] [PATCH v7 10/16] hw/vfio: calxeda xgmac device
On 31.10.14 15:05, Eric Auger wrote:
The platform device class has become abstract. The device can be be
instantiated on command line using such option.
-device vfio-calxeda-xgmac,host=fff51000.ethernet
Signed-off-by: Eric Auger eric.au...@linaro.org
---
v5 - v6
- back again following Alex Graf advises
- fix a bug related to compat override
v4 - v5:
removed since device tree was moved to hw/arm/dyn_sysbus_devtree.c
v4: creation for device tree specialization
---
hw/vfio/Makefile.objs| 1 +
hw/vfio/calxeda_xgmac.c | 54
include/hw/vfio/vfio-calxeda-xgmac.h | 41 +++
3 files changed, 96 insertions(+)
create mode 100644 hw/vfio/calxeda_xgmac.c
create mode 100644 include/hw/vfio/vfio-calxeda-xgmac.h
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index c5c76fe..913ab14 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -2,4 +2,5 @@ ifeq ($(CONFIG_LINUX), y)
obj-$(CONFIG_SOFTMMU) += common.o
obj-$(CONFIG_PCI) += pci.o
obj-$(CONFIG_SOFTMMU) += platform.o
+obj-$(CONFIG_SOFTMMU) += calxeda_xgmac.o
endif
diff --git a/hw/vfio/calxeda_xgmac.c b/hw/vfio/calxeda_xgmac.c
new file mode 100644
index 000..199e076
--- /dev/null
+++ b/hw/vfio/calxeda_xgmac.c
@@ -0,0 +1,54 @@
+/*
+ * calxeda xgmac example VFIO device
+ *
+ * Copyright Linaro Limited, 2014
+ *
+ * Authors:
+ * Eric Auger eric.au...@linaro.org
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include hw/vfio/vfio-calxeda-xgmac.h
+
+static void calxeda_xgmac_realize(DeviceState *dev, Error **errp)
+{
+VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
+VFIOCalxedaXgmacDeviceClass *k =
VFIO_CALXEDA_XGMAC_DEVICE_GET_CLASS(dev);
+
+vdev-compat = g_strdup(calxeda,hb-xgmac);
+
+k-parent_realize(dev, errp);
Since MMIO and IRQ line exposure happens in the parent, I would like to
see a comment here explaining the semantics of each region here. That
way users at least have the chance to figure out what each MMIO number
and IRQ number mean.
Also, since this device will probably get used as example code for
others, I'd like to make sure we set a proper precedence, even if it's
trivial in this case.
Alex
Re: [Qemu-devel] [PATCH] kvmclock: Add comment explaining why we need cpu_clean_all_dirty()
On 03/11/2014 18:45, Eduardo Habkost wrote:
Try to explain why commit 317b0a6d8ba44e9bf8f9c3dbd776c4536843d82c
needed a cpu_clean_all_dirty() call just after calling
cpu_synchronize_all_states().
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
Cc: Andrey Korolyov and...@xdel.ru
Cc: Marcin Gibuła m.gib...@beyond.pl
Cc: Marcelo Tosatti mtosa...@redhat.com
Cc: Paolo Bonzini pbonz...@redhat.com
---
hw/i386/kvm/clock.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 1ac60d6..58be2bd 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -127,7 +127,21 @@ static void kvmclock_vm_state_change(void *opaque, int
running,
}
cpu_synchronize_all_states();
+/* In theory, the cpu_synchronize_all_states() call above wouldn't
+ * affect the rest of the code, as the VCPU state inside CPUState
+ * is supposed to always match the VCPU state on the kernel side.
+ *
+ * In practice, calling cpu_synchronize_state() too soon will load
the
+ * kernel-side APIC state into X86CPU.apic_state too early, APIC
state
+ * won't be reloaded later because CPUState.vcpu_dirty==true, and
+ * outdated APIC state may be migrated to another host.
+ *
+ * The real fix would be to make sure outdated APIC state is read
+ * from the kernel again when necessary. While this is not fixed, we
+ * need the cpu_clean_all_dirty() call below.
+ */
cpu_clean_all_dirty();
+
ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, data);
if (ret 0) {
fprintf(stderr, KVM_GET_CLOCK failed: %s\n, strerror(ret));
Thanks, applying to uq/master.
Paolo
Re: [Qemu-devel] [PATCH v7 09/16] hw/vfio/platform: add vfio-platform support
On 31.10.14 15:05, Eric Auger wrote: Minimal VFIO platform implementation supporting - register space user mapping, - IRQ assignment based on eventfds handled on qemu side. irqfd kernel acceleration comes in a subsequent patch. Signed-off-by: Kim Phillips kim.phill...@linaro.org Signed-off-by: Eric Auger eric.au...@linaro.org --- v6 - v7: - compat is not exposed anymore as a user option. Rationale is the vfio device became abstract and a specialization is needed anyway. The derived device must set the compat string. - in v6 vfio_start_irq_injection was exposed in vfio-platform.h. A new function dubbed vfio_register_irq_starter replaces it. It registers a machine init done notifier that programs starts all dynamic VFIO device IRQs. This function is supposed to be called by the machine file. A set of static helper routines are added too. It must be called before the creation of the platform bus device. v5 - v6: - vfio_device property renamed into host property - correct error handling of VFIO_DEVICE_GET_IRQ_INFO ioctl and remove PCI related comment - remove declaration of vfio_setup_irqfd and irqfd_allowed property.Both belong to next patch (irqfd) - remove declaration of vfio_intp_interrupt in vfio-platform.h - functions that can be static get this characteristic - remove declarations of vfio_region_ops, vfio_memory_listener, group_list, vfio_address_spaces. All are moved to vfio-common.h - remove vfio_put_device declaration and definition - print_regions removed. code moved into vfio_populate_regions - replace DPRINTF by trace events - new helper routine to set the trigger eventfd - dissociate intp init from the injection enablement: vfio_enable_intp renamed into vfio_init_intp and new function named vfio_start_eventfd_injection - injection start moved to vfio_start_irq_injection (not anymore in vfio_populate_interrupt) - new start_irq_fn field in VFIOPlatformDevice corresponding to the function that will be used for starting injection - user handled eventfd: x add mutex to protect IRQ state list manipulation, x correct misleading comment in vfio_intp_interrupt. x Fix bugs thanks to fake interrupt modality - VFIOPlatformDeviceClass becomes abstract - add error_setg in vfio_platform_realize v4 - v5: - vfio-plaform.h included first - cleanup error handling in *populate*, vfio_get_device, vfio_enable_intp - vfio_put_device not called anymore - add some includes to follow vfio policy v3 - v4: [Eric Auger] - merge of vfio: Add initial IRQ support in platform device to get a full functional patch although perfs are limited. - removal of unrealize function since I currently understand it is only used with device hot-plug feature. v2 - v3: [Eric Auger] - further factorization between PCI and platform (VFIORegion, VFIODevice). same level of functionality. = v2: [Kim Philipps] - Initial Creation of the device supporting register space mapping --- hw/vfio/Makefile.objs | 1 + hw/vfio/platform.c | 672 include/hw/vfio/vfio-common.h | 1 + include/hw/vfio/vfio-platform.h | 87 ++ trace-events| 12 + 5 files changed, 773 insertions(+) create mode 100644 hw/vfio/platform.c create mode 100644 include/hw/vfio/vfio-platform.h diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs index e31f30e..c5c76fe 100644 --- a/hw/vfio/Makefile.objs +++ b/hw/vfio/Makefile.objs @@ -1,4 +1,5 @@ ifeq ($(CONFIG_LINUX), y) obj-$(CONFIG_SOFTMMU) += common.o obj-$(CONFIG_PCI) += pci.o +obj-$(CONFIG_SOFTMMU) += platform.o endif diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c new file mode 100644 index 000..9f66610 --- /dev/null +++ b/hw/vfio/platform.c @@ -0,0 +1,672 @@ +/* + * vfio based device assignment support - platform devices + * + * Copyright Linaro Limited, 2014 + * + * Authors: + * Kim Phillips kim.phill...@linaro.org + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + * Based on vfio based PCI device assignment support: + * Copyright Red Hat, Inc. 2012 + */ + +#include linux/vfio.h +#include sys/ioctl.h + +#include hw/vfio/vfio-platform.h +#include qemu/error-report.h +#include qemu/range.h +#include sysemu/sysemu.h +#include exec/memory.h +#include qemu/queue.h +#include hw/sysbus.h +#include trace.h +#include hw/platform-bus.h + +static void vfio_intp_interrupt(VFIOINTp *intp); +typedef void (*eventfd_user_side_handler_t)(VFIOINTp *intp); +static int vfio_set_trigger_eventfd(VFIOINTp *intp, +eventfd_user_side_handler_t handler); + +/* + * Functions only used when eventfd are handled on user-side + * ie. without irqfd + */ + +/** + * vfio_platform_eoi - IRQ completion routine + * @vbasedev: the VFIO device
Re: [Qemu-devel] [RESEND PATCH v4 00/10] QEmu memory hot unplug support.
On Wed, 5 Nov 2014 13:49:45 +0800 Tang Chen tangc...@cn.fujitsu.com wrote: This patch-set implements memory hot-remove for QEmu. Rebased on Igor's asynchronize hotplug framework (qemu v2.1.2, the latest). As I've wrote before, there were hot-unplug patches merged last month, see for reference http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg05195.html and this series could benefit from them. So please look take in account not addressed yet comments on V3 and rebase on current master instead of stable 2.1.2. Approach: QEmu sets GPE status bit, then triggers SCI to notify guest os. Guest os checks device status, and free memory resource if possible, then generate OST. NOTE: In this version, memory hot-remove is independent from _OST, following Igor's comments. Will implement _OST for memory hot-remove soon if this part is OK. Change log v3 - v4 (RESEND): 1. Add new patch 1 ~ 4, fix some small problems in coding. 2. In patch 9, make memory hot-remove independent from _OST. 3. In patch 9, add comment for is_removing flag to document. 4. In patch 10, use macro instead of number. 5. Remove original patch 8 and 12 to coincident with asynchronize hotplug framework. Hu Tao (4): acpi, piix4: Add memory hot unplug support for piix4. pc: Add memory hot unplug support for pc machine. pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support. pc, acpi bios: Add memory hot unplug interface. Tang Chen (6): acpi, mem-hotplug: Use PC_DIMM_SLOT_PROP in acpi_memory_plug_cb(). acpi, mem-hotplug: Add acpi_memory_get_slot_status_descriptor() to get MemStatus. acpi, mem-hotplug: Add acpi_memory_hotplug_sci() to rise sci for memory hotplug. acpi, mem-hotplug: Add acpi_memory_unplug_cb() to implement memory unplug. acpi, ich9: Add memory hot unplug support for ich9. acpi: Add hardware implementation for memory hot unplug. docs/specs/acpi_mem_hotplug.txt | 8 +++-- hw/acpi/ich9.c | 12 +++ hw/acpi/memory_hotplug.c | 71 +++- hw/acpi/piix4.c | 6 +++- hw/i386/pc.c | 31 ++ hw/i386/ssdt-mem.dsl | 5 +++ hw/i386/ssdt-misc.dsl| 13 +++- hw/isa/lpc_ich9.c| 5 +-- hw/mem/pc-dimm.c | 10 ++ include/hw/acpi/ich9.h | 2 ++ include/hw/acpi/memory_hotplug.h | 3 ++ include/hw/acpi/pc-hotplug.h | 2 ++ 12 files changed, 146 insertions(+), 22 deletions(-)
Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it
On 11/04/2014 07:45 PM, Markus Armbruster wrote: I'll try to explain all solutions fairly. Isn't easy when you're as biased towards one of them as I am. Please bear with me. Thanks for this write-up. I'll probably reply again, but for now I'm focusing on just one thing I think you missed that came up in the related threads: = How can we better guard the trust boundary in QEMU? = The guest can violate the trust boundary only because (a) QEMU supports both raw images and image formats, and (b) QEMU guesses image format from raw image contents, and (c) given a raw image, guests can change its contents and control a future QEMU's format guess. We can attack one ore more of these three conditions: (a) Outlaw raw images (b) Don't guess format from untrusted image contents (c) Prevent bad guest writes (d) write metadata that records our guess before releasing data across a trust boundary, so that we no longer need to probe on the next encounter While this won't work for top-level images, it is a possibility for backing store. Any time we open a qcow2 file that has a backing file without a format, we can rewrite the qcow2 metadata to record the type that we ended up settling on, prior to allowing the guest to manipulate the data. The initial open is safe (we haven't yet handed the data to the guest - and the trust boundary is not broken until the point that the guest has had a chance to overwrite data) and all subsequent opens are now safe (because we rewrote the metadata, subsequent operations no longer need to probe the backing file, but are guaranteed to use the same format as the first open, whether or not those subsequent operations are performed by a different qemu that would have probed a different type). PRO: Plugs hole for backing files CON: Doesn't help for top-level images. In a chain base - mid - top where neither mid nor top recorded backing type, it would require mid to be temporarily opened read-write to update the metadata. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
On 30/10/2014 10:36, Christian Borntraeger wrote:
struct kvm_msrs contains a pad field. Lets initialize this pad
field. A designated initializer seems not appropriate here, as
struct kvm_msrs is embedded in the msr_data structure.
Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
What about this:
msr_data.info = (struct kvm_msrs) {
.nmsrs = n
};
? It would also be applicable to other uses of kvm_msrs.
Also, you're missing one occurrence in kvm_put_msr_feature_control.
Paolo
Re: [Qemu-devel] [PATCH 7/9] valgrind/i386: avoid false positives on KVM_GET_MSRS ioctl
On 05/11/2014 11:33, Paolo Bonzini wrote:
On 30/10/2014 10:36, Christian Borntraeger wrote:
struct kvm_msrs contains a pad field. Lets initialize this pad
field. A designated initializer seems not appropriate here, as
struct kvm_msrs is embedded in the msr_data structure.
Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
What about this:
msr_data.info = (struct kvm_msrs) {
.nmsrs = n
};
? It would also be applicable to other uses of kvm_msrs.
Also, KVM_SET_MSRS has to deal with a reserved field in struct
kvm_msr_entry. Currently you handle it with a relatively large memset
produced by the designated initializer = {} in kvm_put_msrs. However,
you could set it in kvm_msr_entry_set, and avoid the memset.
Paolo
Also, you're missing one occurrence in kvm_put_msr_feature_control.
Re: [Qemu-devel] [PATCH v3 2/2] vfio: use kvm_resamplefds_enabled()
On 04/11/2014 23:25, Alex Williamson wrote: Acked-by: Alex Williamson alex.william...@redhat.com Thanks, I'll get it into the KVM tree (uq/master). Paolo
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call
On Wednesday 05 November 2014 02:02 PM, Alexander Graf wrote:
On 05.11.14 08:13, Aravinda Prasad wrote:
This patch adds FWNMI support in qemu for powerKVM
guests by handling the ibm,nmi-register rtas call.
Whenever OS issues ibm,nmi-register RTAS call, the
machine check notification address is saved and the
machine check interrupt vector 0x200 is patched to
issue a private hcall.
This patch also handles the cases when multi-processors
experience machine check at or about the same time.
As per PAPR, subsequent processors serialize waiting
for the first processor to issue the ibm,nmi-interlock call.
The second processor retries if the first processor which
received a machine check is still reading the error log
and is yet to issue ibm,nmi-interlock call.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr_hcall.c| 16 +++
hw/ppc/spapr_rtas.c | 93
+++
include/hw/ppc/spapr.h | 17 +++
pc-bios/spapr-rtas/spapr-rtas.S | 38
4 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f16160..eceb5e5 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -97,6 +97,9 @@ struct rtas_mc_log {
struct rtas_error_log err_log;
};
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
static void do_spr_sync(void *arg)
{
struct SPRSyncState *s = arg;
@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
/*
+ * Only one VCPU can process machine check NMI at a time. Hence
+ * set the lock mc_in_progress. Once the VCPU finishes processing
+ * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+ * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+ * NMI we return 0 asking the VCPU to retry h_report_mc_err
+ */
+if (mc_in_progress == 1) {
Please don't depend on bools being numbers. Use true / false. For if()s,
just don't use == at all - it makes it more readable.
ok
+return 0;
+}
+
+mc_in_progress = 1;
+
+/*
* We save the original r3 register in SPRG2 in 0x200 vector,
* which is patched during call to ibm.nmi-register. Original
* r3 is required to be included in error log
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2ec2a8e..71c7662 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -36,6 +36,9 @@
#include libfdt.h
+#define BRANCH_INST_MASK 0xFC00
+extern bool mc_in_progress;
Please put this into the spapr struct.
ok
+
static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
uint32_t token, uint32_t nargs,
target_ulong args,
@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
rtas_st(rets, 0, ret);
}
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+ sPAPREnvironment *spapr,
+ uint32_t token, uint32_t nargs,
+ target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+int i;
+uint32_t ori_inst = 0x6063;
+uint32_t branch_inst = 0x4802;
+target_ulong guest_machine_check_addr;
+uint32_t trampoline[TRAMPOLINE_INSTS];
+int total_inst = sizeof(trampoline) / sizeof(uint32_t);
ARRAY_SIZE(trampoline), though I don't quite understand why you need a
variable that contains the same value as a constant (TRAMPOLINE_INSTS).
But since you're moving all of those bits into variable fields on the
rtas blob itself as we discussed in the last version, I guess this code
will go away anyways ;).
I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR
number and branch address in the trampoline and also, depending on
whether the guest running in LE/BE we may need to flip the bits in the
trampoline before copying it to 0x200 machine check vector.
As rtas-blob is part of the guest memory I do not want to patch these in
rtas-blob, hence I copy the trampoline from the rtas-blob to an array,
modify accordingly and then move it to 0x200 machine check vector.
+PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+/* Store the system reset and machine check address */
+guest_machine_check_addr = rtas_ld(args, 1);
Load or Store? I don't find the comment particularly useful either ;).
will reword it or may delete it.
+
+/*
+ * Read the trampoline instructions from RTAS Blob and patch
+ * the KVMPPC_H_REPORT_MC_ERR hcall number and the guest
+ * machine check address before copying to 0x200 vector
+ */
+
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 1/4] target-ppc: Extend rtas-blob
On Wednesday 05 November 2014 02:37 PM, Alexander Graf wrote:
On 05.11.14 10:00, Alexander Graf wrote:
On 05.11.14 09:46, Aravinda Prasad wrote:
On Wednesday 05 November 2014 01:41 PM, Alexander Graf wrote:
On 05.11.14 08:12, Aravinda Prasad wrote:
Extend rtas-blob to accommodate error log. Error log
structure is saved in rtas space upon a machine check
exception.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr.c |7 +++
include/hw/ppc/spapr.h |5 +
2 files changed, 12 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30de25d..38e26af 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1431,6 +1431,13 @@ static void ppc_spapr_init(MachineState *machine)
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, spapr-rtas.bin);
spapr-rtas_size = get_image_size(filename);
+
+/*
+ * Resize blob to accommodate error log. The layout of the rtas
+ * blob is defined in include/hw/ppc/spapr.h
+ */
+spapr-rtas_size = TARGET_PAGE_ALIGN(spapr-rtas_size);
How big is the error log? You could just extend the RTAS blob to include
space for it if it's not too big.
Error log is around 10 bytes and requires additional 24 bytes to store
saved sro/srr1.
Hmm.. yes it can be included in RTAS blob itself.
+
spapr-rtas_blob = g_malloc(spapr-rtas_size);
if (load_image_size(filename, spapr-rtas_blob, spapr-rtas_size)
0) {
hw_error(qemu: could not load LPAR rtas '%s'\n, filename);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 749daf4..d08fcc2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -480,4 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char
*propname,
int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
sPAPRTCETable *tcet);
+/* RTAS Blob layout in memory */
+#define RTAS_ENTRY_OFFSET0
+#define RTAS_TRAMPOLINE_OFFSET 0x200
+#define RTAS_ERRLOG_OFFSET 0x800
I thought we agreed that these offsets should've been defined by the
blob itself?
I think I got it wrong.
I will include these indexes at the entry of RTAS blob. With that we
will have something like this:
RTAS_ENTRY_OFFSET = *(spapr-rtas_addr)
RTAS_TRAMPOLINE_OFFSET = *(spapr-rtas_addr+8)
RTAS_ERRLOG_OFFSET = *(spapr-rtas_addr+16)
I will fix this.
Cool :). Just store the offsets inside of a helper struct that you for
example store in the spapr struct, then we don't need to read volatile
guest memory for the offsets.
I just reread what I wrote and figured it's not exactly verbose. What I
meant was that you read them on load into a struct. Then when working
with the offsets, you only use the cached ones from the struct.
That way when the guest for whatever reason modifies the RTAS blob in
memory, we would still use the old offsets and ensure that we don't end
up overwriting memory that we never intended to overwrite ;).
sure
Alex
--
Regards,
Aravinda
Re: [Qemu-devel] [PATCH 2/6] block/parallels: allow to specify DiskDescriptor.xml instead of image file
On 05/11/14 05:12, Jeff Cody wrote:
On Wed, Oct 29, 2014 at 04:38:07PM +0300, Denis V. Lunev wrote:
Typically Parallels disk bundle consists of several images which are
glued by XML disk descriptor. Also XML hides inside several important
parameters which are not available in the image header.
This patch allows to specify this XML file as a filename for an image
to open. It is allowed only to open Compressed images with the only
snapshot inside. No additional options are parsed at the moment.
The code itself is dumb enough for a while. If XML file is specified,
the file is parsed and the image is reopened as bs-file to keep the
rest of the driver untouched. This would be changed later with more
features added.
Signed-off-by: Denis V. Lunev d...@openvz.org
Acked-by: Roman Kagan rka...@parallels.com
CC: Jeff Cody jc...@redhat.com
CC: Kevin Wolf kw...@redhat.com
CC: Stefan Hajnoczi stefa...@redhat.com
---
block/parallels.c | 231 --
1 file changed, 226 insertions(+), 5 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 4f9cd8d..201c0f1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -27,6 +27,11 @@
#include block/block_int.h
#include qemu/module.h
+#if CONFIG_LIBXML2
+#include libxml/parser.h
+#include libxml/tree.h
+#endif
+
/**/
#define HEADER_MAGIC WithoutFreeSpace
@@ -34,6 +39,10 @@
#define HEADER_VERSION 2
#define HEADER_SIZE 64
+#define PARALLELS_XML 100
+#define PARALLELS_IMAGE 101
+
+
// always little-endian
struct parallels_header {
char magic[16]; // WithoutFreeSpace
@@ -59,6 +68,194 @@ typedef struct BDRVParallelsState {
unsigned int off_multiplier;
} BDRVParallelsState;
+static int parallels_open_image(BlockDriverState *bs, Error **errp);
You shouldn't need this forward declaration, if you put your new
function parallels_open_xml() after parallels_open_image().
ok, fair enough. This is a rudiment from my previous internal version
which has been implemented in the other way.
+
+#if CONFIG_LIBXML2
+static xmlNodePtr xml_find(xmlNode *node, const char *elem)
+{
+xmlNode *child;
+
+for (child = node-xmlChildrenNode; child != NULL; child = child-next) {
+if (!xmlStrcmp(child-name, (const xmlChar *)elem)
+child-type == XML_ELEMENT_NODE) {
+return child;
+}
+}
+return NULL;
+}
+
+static xmlNodePtr xml_seek(xmlNode *root, const char *elem)
+{
+xmlNode *child = root;
+const char *path;
+char nodename[128];
+int last = 0;
+
+path = elem;
+if (path[0] == '/') {
+path++;
+}
+if (path[0] == 0) {
+return NULL;
+}
+while (!last) {
+const char *p = strchr(path, '/');
+int length;
+if (p == NULL) {
+length = strlen(path);
+last = 1;
+} else {
+length = p - path;
+}
+memcpy(nodename, path, length);
+nodename[length] = 0;
It looks like elem is always controlled by us, and not passed by the
user - will this always be the case?
If not, this doesn't seem safe, with nodename being a local array of
128 bytes. How about using g_strdup() or g_strndup() here?
yes, this constraint is used now and will be used in the future.
XML structure is known and it is fixed. Here we just shortcut
the navigation in the tree in a convenient way to pass entire
path is one call.
I think that we can switch to interface like
static xmlNodePtr xml_seek(xmlNode *root, ...)
and call it like
xml_seek(root, StorageData, Storage, Image, NULL);
in this case we will be able to drop a lot of processing and
parsing inside including strchr, memcpy, strlen etc
+child = xml_find(child, nodename);
+if (child == NULL) {
+return NULL;
+}
+path = ++p;
+}
+return child;
+}
+
+static const char *xml_get_text(xmlNode *node, const char *name)
+{
+xmlNode *child;
+
+node = xml_seek(node, name);
+if (node == NULL) {
+return NULL;
+}
+
+for (child = node-xmlChildrenNode; child; child = child-next) {
+if (child-type == XML_TEXT_NODE) {
+return (const char *)child-content;
+}
+}
+return NULL;
+}
+
+static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
+{
+int size, ret;
+xmlDoc *doc = NULL;
+xmlNode *root, *image;
+char *xml = NULL;
+const char *data;
+char image_path[PATH_MAX];
+Error *local_err = NULL;
+
+ret = size = bdrv_getlength(bs-file);
+if (ret 0) {
+goto fail;
+}
+/* XML file size should be resonable */
s/resonable/reasonable
ok
+ret = -EFBIG;
+if (size 65536) {
+goto fail;
+}
+
+xml = g_malloc(size + 1);
+
+ret = bdrv_pread(bs-file, 0, xml, size);
+if (ret != size) {
+goto fail;
+}
+
Re: [Qemu-devel] [PATCH v3 2/2] main-loop: Use epoll on Linux
On 27/10/2014 08:30, Fam Zheng wrote:
+static int epoll_prepare(int epollfd,
+ GPollFD *fds, guint nfds,
+ GPollFD **g_poll_fds,
+ guint *g_poll_nfds,
+ int **g_poll_fd_idx)
Please pass the QEMUPollContext to epoll_prepare.
+if (ctx-last_fds) {
+close(ctx-epollfd);
+}
+ctx-epollfd = epoll_create(1);
Please use epoll_create1 with the EPOLL_CLOEXEC flag.
+if (ctx-epollfd 0) {
+perror(epoll_create);
+abort();
+}
Interesting. Is it cheaper to do this than to compute a symmetric
difference?
I am worried that the epoll_create fails with EMFILE, and that the
destruction/recreation happens often with networking (which uses
qemu_set_fd_handler2) if you use the right workload.
Paolo
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
On 05/11/2014 09:12, Max Reitz wrote:
On 2014-11-05 at 09:09, SeokYeon Hwang wrote:
Negative type of errno like -ERRNO is used a lot by developers.
Therefore, error_set_errno() is modified to deal with a negative type
of os_error.
(Negative type is used at pcie_cap_slot_hotplug_common() in
hw/pci/pcie.c)
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
---
util/error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/error.c b/util/error.c
index 2ace0d8..5db00c9 100644
--- a/util/error.c
+++ b/util/error.c
@@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno,
ErrorClass err_class,
va_start(ap, fmt);
msg1 = g_strdup_vprintf(fmt, ap);
if (os_errno != 0) {
-err-msg = g_strdup_printf(%s: %s, msg1, strerror(os_errno));
+err-msg = g_strdup_printf(%s: %s, msg1,
strerror(abs(os_errno)));
g_free(msg1);
} else {
err-msg = msg1;
This is utterly broken and we should fix all callers instead.
...But I like it.
I don't, we really should fix the callers.
Paolo
Reviewed-by: Max Reitz mre...@redhat.com
Re: [Qemu-devel] [PATCH v3] smbios: need to change some of 'ram_addr_t' variable to 'uint64_t'
On 05/11/2014 07:19, SeokYeon Hwang wrote:
Some of variables handling 64bit address must be changed from 'ram_addr_t' to
'uint64_t'.
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
Thanks, applied. I modified the commit message as follows:
smbios: change 'ram_addr_t' variables to 'uint64_t'
ram_addr_t should not be used except if referring to a RAMBlobk.
Using 'uint64_t' avoids a -Wconstant-conversion warning, which
clang = 3.4 produces in smbios_get_tables().
Paolo
---
hw/i386/smbios.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 8a7ad48..024e594 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -645,7 +645,7 @@ static void smbios_build_type_4_table(unsigned instance)
static void smbios_build_type_16_table(unsigned dimm_cnt)
{
-ram_addr_t size_kb;
+uint64_t size_kb;
SMBIOS_BUILD_TABLE_PRE(16, 0x1000, true); /* required */
@@ -669,10 +669,10 @@ static void smbios_build_type_16_table(unsigned
dimm_cnt)
#define MAX_T17_STD_SZ 0x7FFF /* (32G - 1M), in Megabytes */
#define MAX_T17_EXT_SZ 0x8000 /* 2P, in Megabytes */
-static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
+static void smbios_build_type_17_table(unsigned instance, uint64_t size)
{
char loc_str[128];
-ram_addr_t size_mb;
+uint64_t size_mb;
SMBIOS_BUILD_TABLE_PRE(17, 0x1100 + instance, true); /* required */
@@ -711,9 +711,9 @@ static void smbios_build_type_17_table(unsigned instance,
ram_addr_t size)
}
static void smbios_build_type_19_table(unsigned instance,
- ram_addr_t start, ram_addr_t size)
+ uint64_t start, uint64_t size)
{
-ram_addr_t end, start_kb, end_kb;
+uint64_t end, start_kb, end_kb;
SMBIOS_BUILD_TABLE_PRE(19, 0x1300 + instance, true); /* required */
Re: [Qemu-devel] [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic instantiation
On 31.10.14 15:05, Eric Auger wrote:
vfio-calxeda-xgmac now can be instantiated using the -device option.
The node creation function generates a very basic dt node composed
of the compat, reg and interrupts properties
Signed-off-by: Eric Auger eric.au...@linaro.org
---
v6 - v7:
- compat string re-formatting removed since compat string is not exposed
anymore as a user option
- VFIO IRQ kick-off removed from sysbus-fdt and moved to VFIO platform
device
---
hw/arm/sysbus-fdt.c | 88
+
1 file changed, 88 insertions(+)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d5476f1..f8b310b 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -27,6 +27,8 @@
#include hw/platform-bus.h
#include sysemu/sysemu.h
#include hw/platform-bus.h
+#include hw/vfio/vfio-platform.h
+#include hw/vfio/vfio-calxeda-xgmac.h
/*
* internal struct that contains the information to create dynamic
@@ -54,8 +56,11 @@ typedef struct NodeCreationPair {
int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
} NodeCreationPair;
+static int add_basic_vfio_fdt_node(SysBusDevice *sbdev, void *opaque);
+
/* list of supported dynamic sysbus devices */
NodeCreationPair add_fdt_node_functions[] = {
+{TYPE_VFIO_CALXEDA_XGMAC, add_basic_vfio_fdt_node},
{, NULL}, /*last element*/
};
Can you maybe place the list somewhere smartly to make sure we don't
need forward declarations? Either put it in between the generic and
device specific code or at the end of the file with a single forward
declaration for the array?
@@ -86,6 +91,89 @@ static int add_fdt_node(SysBusDevice *sbdev, void *opaque)
}
/**
+ * add_basic_vfio_fdt_node - generates the most basic node for a VFIO node
+ *
+ * set properties are:
+ * - compatible string
+ * - regs
+ * - interrupts
+ */
+static int add_basic_vfio_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+PlatformBusFdtData *data = opaque;
+PlatformBusDevice *pbus = data-pbus;
+void *fdt = data-fdt;
+const char *parent_node = data-pbus_node_name;
+int compat_str_len;
+char *nodename;
+int i, ret;
+uint32_t *irq_attr;
+uint64_t *reg_attr;
+uint64_t mmio_base;
+uint64_t irq_number;
+VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+VFIODevice *vbasedev = vdev-vbasedev;
+Object *obj = OBJECT(sbdev);
+
+mmio_base = object_property_get_int(obj, mmio[0], NULL);
+
+nodename = g_strdup_printf(%s/%s@% PRIx64, parent_node,
+ vbasedev-name,
+ mmio_base);
+
+qemu_fdt_add_subnode(fdt, nodename);
+
+compat_str_len = strlen(vdev-compat) + 1;
+qemu_fdt_setprop(fdt, nodename, compatible,
+ vdev-compat, compat_str_len);
What if there are multiple compatibles?
+
+reg_attr = g_new(uint64_t, vbasedev-num_regions*4);
+
+for (i = 0; i vbasedev-num_regions; i++) {
+mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
+reg_attr[4*i] = 1;
What is the 1 here?
+reg_attr[4*i+1] = mmio_base;
+reg_attr[4*i+2] = 1;
and here?
+reg_attr[4*i+3] = memory_region_size(vdev-regions[i]-mem);
+}
+
+ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, reg,
+ vbasedev-num_regions*2, reg_attr);
+if (ret 0) {
+error_report(could not set reg property of node %s, nodename);
+goto fail;
+}
+
+irq_attr = g_new(uint32_t, vbasedev-num_irqs*3);
+
+for (i = 0; i vbasedev-num_irqs; i++) {
+irq_number = platform_bus_get_irqn(pbus, sbdev , i)
+ + data-irq_start;
+irq_attr[3*i] = cpu_to_be32(0);
+irq_attr[3*i+1] = cpu_to_be32(irq_number);
+irq_attr[3*i+2] = cpu_to_be32(0x4);
Why 0x4? How do you know whether an IRQ is edge or level triggered?
I'm still not convinced we can make anything generic on the VFIO path.
How about you call the function xgmac specific for now, but keep the
code as dynamic as it is?
Alex
+}
+
+ ret = qemu_fdt_setprop(fdt, nodename, interrupts,
+ irq_attr, vbasedev-num_irqs*3*sizeof(uint32_t));
+if (ret 0) {
+error_report(could not set interrupts property of node %s,
+ nodename);
+goto fail;
+}
+
+g_free(nodename);
+g_free(irq_attr);
+g_free(reg_attr);
+
+return 0;
+
+fail:
+
+ return -1;
+}
+
+/**
* add_all_platform_bus_fdt_nodes - create all the platform bus nodes
*
* builds the parent platform bus node and all the nodes of dynamic
Re: [Qemu-devel] [PATCH 2/4] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend driver
On Tue, 4 Nov 2014, Xu, Quan wrote: -Original Message- From: Stefano Stabellini [mailto:stefano.stabell...@eu.citrix.com] Sent: Monday, November 03, 2014 7:54 PM To: Xu, Quan Cc: qemu-devel@nongnu.org; xen-de...@lists.xen.org; stefano.stabell...@eu.citrix.com Subject: Re: [PATCH 2/4] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend driver On Sun, 2 Nov 2014, Quan Xu wrote: This drvier transfers any request/repond between TPM xenstubdoms driver and Xen vTPM stubdom, and facilitates communications between Xen vTPM stubdom domain and vTPM xenstubdoms driver Signed-off-by: Quan Xu quan...@intel.com Please describe what changes did make to xen_backend.c and why. The commit message should contains info on all the changes made by the patch below. Thanks Stefano. one more day, I will explain in detail what changes did make to xen_backend.c and why. The following 2 sections are introduction and architecture. Please also describe what is the Xen vTPM stubdom, what is the vTPM xenstubdoms driver and how the communicate with each others. Add 2 parts for detailed descriptions, introduction and architecture. *INTRODUCTION* The goal of virtual Trusted Platform Module (vTPM) is to provide a TPM functionality to virtual machines (Fedora, Ubuntu, Redhat, Windows .etc). This allows programs to interact with a TPM in a virtual machine the same way they interact with a TPM on the physical system. Each virtual machine gets its own unique, emulated, software TPM. Each major component of vTPM is implemented as a stubdom, providing secure separation guaranteed by the hypervisor. The vTPM stubdom is a Xen mini-OS domain that emulates a TPM for the virtual machine to use. It is a small wrapper around the Berlios TPM emulator. TPM commands are passed from mini-os TPM backend driver. This patch series are only the Qemu part to enable Xen stubdom vTPM for HVM virtual machine. === *ARCHITECTURE* The architecture of stubdom vTPM for HVM virtual machine: ++ | Windows/Linux DomU | ... || ^| |v || | Qemu tpm1.2 Tis | || ^| |v || |vTPM| | XenStubdoms driver | (new ..) ++ | ^ v | ++ | xen_vtpmdev_ops | (new ..) ++ | ^ v | ++ | mini-os/tpmback | || ^| |v || | vTPM stubdom | ... || ^| |v || | mini-os/tpmfront | ++ | ^ v | ++ | mini-os/tpmback | || ^| |v || | vtpmmgr stubdom | || ^| |v || | mini-os/tpm_tis | ++ | ^ v | ++ |Hardware TPM| ++ * Windows/Linux DomU: The HVM based guest that wants to use a vTPM. There may be more than one of these. * Qemu tpm1.2 Tis: Implementation of the tpm1.2 Tis interface for HVM virtual machines. It is Qemu emulation device. It looks like you are running upstream QEMU within a Mini-OS stubdom? If so, where are the patches to do that? As far as I know upstream QEMU doesn't run on Mini-OS. Or maybe it is a Linux stubdom? Either way, it is not clear. * vTPM xenstubdoms driver: Similar to a TPM passthrough backend driver, it is a new TPM backend for emulated TPM TIS interface. This driver provides vTPM initialization and sending data and commends to a Xen vTPM stubdom. This is patch #3, right? It is basically the internal QEMU glue to connect the tpm emulator to xen_vtpmdev_ops, correct? * xen_vtpmdev_ops: Register Xen stubdom vTPM backend, and transfer any request/ repond between TPM xenstubdoms driver and Xen vTPM stubdom. Facilitate communications between Xen vTPM stubdom and vTPM xenstubdoms driver. It looks like this correspond to patch #2? If so, this is a regular Xen PV frontend, living in QEMU? It requires the gntalloc device so I guess it is supposed to run on Linux. In that case is it just a tpmfront implementation in QEMU? If you are running it on Linux, we might need to introduce Linux based stundoms to the Xen build system.
Re: [Qemu-devel] [PATCH v2] translate-all: Marked map_exec() with the 'unused' attribute
On 03/11/2014 09:11, SeokYeon Hwang wrote:
Marked map_exec() with the 'unused' attribute to avoid '-Wunused-function' on
clang 3.4 or later.
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
Cc: qemu-triv...@nongnu.org
Reviewed-by: Paolo Bonzini pbonz...@redhat.com
---
translate-all.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/translate-all.c b/translate-all.c
index ba5c840..9d150fb 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -270,14 +270,14 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
}
#ifdef _WIN32
-static inline void map_exec(void *addr, long size)
+static __attribute__((unused)) void map_exec(void *addr, long size)
{
DWORD old_protect;
VirtualProtect(addr, size,
PAGE_EXECUTE_READWRITE, old_protect);
}
#else
-static inline void map_exec(void *addr, long size)
+static __attribute__((unused)) void map_exec(void *addr, long size)
{
unsigned long start, end, page_size;
Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it
Am 04.11.2014 um 19:45 hat Markus Armbruster geschrieben: I'll try to explain all solutions fairly. Isn't easy when you're as biased towards one of them as I am. Please bear with me. = The trust boundary between image contents and meta-data = A disk image consists of image contents and meta-data. Example: all of a raw image's contents is image contents. Leaves just file name and attributes for meta-data. Better: Leaves only protocol-specific metadata (e.g. file name and attributes for raw-posix). Example: QCOW2 meta-data includes header, header extensions, L1 table, L2 tables, ... The meta-data defines where in the image the actual contents is stored. A guest can access the image contents, not the meta-data. Image contents you've let an untrusted guest write is untrusted. Therefore, there's a trust boundary between image contents and meta-data. QEMU has to trust image meta-data, but shouldn't trust image contents. The exact location of the trust boundary depends on the image format. Trust metadata to a certain degree - the block layer audit was started because we noticed that it might not be all that trustworthy in practice. Different problem, though, it just shows that there are hardly any clear borders between always completely trusted and always completely untrusted. = How we instruct QEMU what to trust = By configuring QEMU to use an image, the user instructs QEMU to trust the image's meta-data. When the user's configuration specifies the image format explicitly, the trust boundary is clear. Else, the trust boundary is ambigous when more than one format is possible. QEMU resolves this ambiguity by picking the first format with the highest score. Raw format is always possible, and always has the lowest score. You used the term untrusted guest before. Are there any trusted guests, or should we assume that guests are untrusted by definition? If you use virtualisation for isolation, then the answer is probably that guests are always untrusted. Other users may know exactly what their guest is doing and are using qemu for other reasons. The former would probably want to disable probing completely, the latter don't care about it and prefer convenience. My guess is that the share of those with trusted guests is higher among direct qemu users than libvirt users, but it's just that, a guess. It also doesn't mean that they are the majority of direct qemu users (they might be, but I honestly don't know). If there are trusted and untrusted guests, does this section need some thoughts about instructing qemu whether to trust the guest or not? = How this lets the guest escape isolation = Unfortunately, this lets the guest shift the trust boundary and escape isolation, as follows: * Expose a raw image to the guest (whether you specify the format=raw or let QEMU guess it doesn't matter). The complete contents becomes untrusted. * Reuse the image *without* specifying the raw format. QEMU guesses the format based on untrusted image contents. Now QEMU guesses a format chosen by the guest, with meta-data chosen by the guest. By controlling image meta-data, the malicious guest can access arbitrary files as QEMU, enlarge its storage, and more. A non-malicious guest can accidentally DoS itself, by writing a pattern probing recognizes. This is CVE-2008-2004. = Aside: other trust boundaries = Of course, this is not the only trust boundary that matters. For instance, there's normally one between your host and somebody else's computers. Telling QEMU to trust meta-data of some image you got from the internet violates it. There's nothing QEMU can do about that. Okay, this addresses what I commented above. = Insecure usage is easy, secure usage is hard = The oldest stratum of user interfaces doesn't let you specify the image format. Use of raw images with these is insecure by design. These interfaces are still recommended for human users. Example of insecure usage: -hda foo.img, where foo.img is raw. With the next generation of interfaces, specifying the image format is optional. Use of raw images with these is insecure by default. Example of insecure usage: -drive file=foo.img,index=0,media=cdrom, where foo.img is raw. The -hda above is actually sugar for this. Equivalent secure usage: add format=raw. Note that specifying just the top image's format is not enough, you also have to specify any backing images' formats. QCOW2 can optionally store the backing image format in the image. The other COW formats can't. Example of insecure usage: -hda bar.vmdk, where bar.vmdk is a VMDK image with a raw backing file. Usually this is mitigated by the fact that backing files are read-only. Trouble is starting when you use things like commit. Equivalent secure usage: Beats me. Maybe there's a funky -drive backing.whatever to specify the backing image's format. Yes, you can override the
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call
On 05.11.14 11:37, Aravinda Prasad wrote:
On Wednesday 05 November 2014 02:02 PM, Alexander Graf wrote:
On 05.11.14 08:13, Aravinda Prasad wrote:
This patch adds FWNMI support in qemu for powerKVM
guests by handling the ibm,nmi-register rtas call.
Whenever OS issues ibm,nmi-register RTAS call, the
machine check notification address is saved and the
machine check interrupt vector 0x200 is patched to
issue a private hcall.
This patch also handles the cases when multi-processors
experience machine check at or about the same time.
As per PAPR, subsequent processors serialize waiting
for the first processor to issue the ibm,nmi-interlock call.
The second processor retries if the first processor which
received a machine check is still reading the error log
and is yet to issue ibm,nmi-interlock call.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr_hcall.c| 16 +++
hw/ppc/spapr_rtas.c | 93
+++
include/hw/ppc/spapr.h | 17 +++
pc-bios/spapr-rtas/spapr-rtas.S | 38
4 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f16160..eceb5e5 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -97,6 +97,9 @@ struct rtas_mc_log {
struct rtas_error_log err_log;
};
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
static void do_spr_sync(void *arg)
{
struct SPRSyncState *s = arg;
@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
/*
+ * Only one VCPU can process machine check NMI at a time. Hence
+ * set the lock mc_in_progress. Once the VCPU finishes processing
+ * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+ * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+ * NMI we return 0 asking the VCPU to retry h_report_mc_err
+ */
+if (mc_in_progress == 1) {
Please don't depend on bools being numbers. Use true / false. For if()s,
just don't use == at all - it makes it more readable.
ok
+return 0;
+}
+
+mc_in_progress = 1;
+
+/*
* We save the original r3 register in SPRG2 in 0x200 vector,
* which is patched during call to ibm.nmi-register. Original
* r3 is required to be included in error log
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2ec2a8e..71c7662 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -36,6 +36,9 @@
#include libfdt.h
+#define BRANCH_INST_MASK 0xFC00
+extern bool mc_in_progress;
Please put this into the spapr struct.
ok
+
static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment
*spapr,
uint32_t token, uint32_t nargs,
target_ulong args,
@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
rtas_st(rets, 0, ret);
}
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+ sPAPREnvironment *spapr,
+ uint32_t token, uint32_t nargs,
+ target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+int i;
+uint32_t ori_inst = 0x6063;
+uint32_t branch_inst = 0x4802;
+target_ulong guest_machine_check_addr;
+uint32_t trampoline[TRAMPOLINE_INSTS];
+int total_inst = sizeof(trampoline) / sizeof(uint32_t);
ARRAY_SIZE(trampoline), though I don't quite understand why you need a
variable that contains the same value as a constant (TRAMPOLINE_INSTS).
But since you're moving all of those bits into variable fields on the
rtas blob itself as we discussed in the last version, I guess this code
will go away anyways ;).
I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR
number and branch address in the trampoline and also, depending on
whether the guest running in LE/BE we may need to flip the bits in the
trampoline before copying it to 0x200 machine check vector.
As rtas-blob is part of the guest memory I do not want to patch these in
rtas-blob, hence I copy the trampoline from the rtas-blob to an array,
modify accordingly and then move it to 0x200 machine check vector.
Yes, you will still need the array. But the array should be dynamically
sized based on spapr-rtas_info-fwnmi_size which you extract from the
blob on load.
That way you wouldn't need the total_inst variable anymore ;).
+PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+/* Store the system reset and machine check address */
+guest_machine_check_addr = rtas_ld(args, 1);
Load or Store? I don't find the comment particularly useful either ;).
Re: [Qemu-devel] [RFC PATCH 0/8] Add Generic PCI host device update
On Wed, Nov 5, 2014 at 11:23 AM, Claudio Fontana claudio.font...@huawei.com wrote: Hi Alvise, Hi Claudio, On 11.07.2014 09:21, Alvise Rigo wrote: This patch series is based on the previous work [1] and [2] by Rob Herring and it tries to enhance this work on these points: do your patches need to be applied on top of Rob's? Yes, definitively. I ask because I cannot see the patch hw/arm/virt: Add generic PCI host device and among these, as wll as the hw/pci-host: add a generic PCI host. It seems to me that it's common practice in this mailing list to not include the patches upon which the work is based. What I could do for the next version is to set up a repository with all the dependences included. I think those two need modifications as well, as they hardcode addresses, and also try to register the PCI bus as a PCIE bus: does it really provide a PCI-Express? (probably harmless but still would benefit from review). This patches should not be compatible with PCI-Express devices, since for them the ECAM access mechanism is required. The initial idea was to add a second bus for PCI-Express cards, however there wouldn't have been devices to use/test it, so I put this on hold. In any case, I will consider it for the next iteration. Regards, alvise - Some of the hardcoded values have been moved to an header file. This header file is also used to share some device structures with the mach-virt machine. Some additional hardcoded addresses have been also introduced with your changes though. (I'll post comments to the the patches in the series momentarily). - The interrupt-map dt node generation has been revisited; it is now done after the generic devices init so that it's possible to attach PCI devices by mean of the qdev infrastructure. This allows to have several devices in the PCI bus, with the current limitation of one interrupt per PCI slot. Probably the most objectionable part of these patches regards the way some data and definitions have been shared between the machine and the device code; a better solution is still under evaluation. Any advice on this and on the rest of the work is highly appreciated. This work has been tested attaching several PCI devices to the mach-virt platform. The tested devices are: virtio-blk-pci, virtio-net-pci, lsi53c895a and pci-ohci (all attached at the same time). Even if the original work was not changed in its core functionalities, I couldn't reproduce the malfunctioning of the LSI SCSI mentioned in [1]. After attaching several qcow2 images, formatting and filling them, I didn't notice anything wrong. Am I missing something? Thank you, alvise [1] [Qemu-devel] [RFC PATCH 1/2] hw/pci-host: add a generic PCI host http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html [2] [Qemu-devel] [RFC PATCH 2/2] hw/arm/virt: Add generic PCI host device http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html Alvise Rigo (8): mach-virt: move GIC inside mach-virt structure mach-virt: improve PCI memory topology definition QEMUMachine: finalize_dt function generic_pci: create header file generic_pci: create own map irq function generic_pci: generate dt node after devices init generic_pci: realize device with machine data generic_pci: add interrupt map structures hw/arm/virt.c | 110 +++- hw/pci-host/generic-pci.c | 173 +- include/hw/boards.h | 4 + include/hw/pci-host/pci_generic.h | 66 +++ vl.c | 5 ++ 5 files changed, 277 insertions(+), 81 deletions(-) create mode 100644 include/hw/pci-host/pci_generic.h
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
On 2014-11-05 at 11:57, Paolo Bonzini wrote:
On 05/11/2014 09:12, Max Reitz wrote:
On 2014-11-05 at 09:09, SeokYeon Hwang wrote:
Negative type of errno like -ERRNO is used a lot by developers.
Therefore, error_set_errno() is modified to deal with a negative type
of os_error.
(Negative type is used at pcie_cap_slot_hotplug_common() in
hw/pci/pcie.c)
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
---
util/error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/error.c b/util/error.c
index 2ace0d8..5db00c9 100644
--- a/util/error.c
+++ b/util/error.c
@@ -68,7 +68,7 @@ void error_set_errno(Error **errp, int os_errno,
ErrorClass err_class,
va_start(ap, fmt);
msg1 = g_strdup_vprintf(fmt, ap);
if (os_errno != 0) {
-err-msg = g_strdup_printf(%s: %s, msg1, strerror(os_errno));
+err-msg = g_strdup_printf(%s: %s, msg1,
strerror(abs(os_errno)));
g_free(msg1);
} else {
err-msg = msg1;
This is utterly broken and we should fix all callers instead.
...But I like it.
I don't, we really should fix the callers.
Of course I understand, but this patch doesn't make matters worse, as
long as there are not systems which have negative values for errno
(which I think we generally assume not to exist throughout qemu). That's
why I'm fine with it. We should fix the callers but I don't see why we
shouldn't apply this patch as well.
A similar issue already came up and led to commit b276d2499, where
callers of error_setg_errno() assumed that it would not clobber errno,
so we fixed some of the callers but also applied that commit which just
saves errno because there's no reason not to.
Max
Paolo
Reviewed-by: Max Reitz mre...@redhat.com
Re: [Qemu-devel] Image probing: how it can be insecure, and what we could do about it
Am 05.11.2014 um 09:38 hat Max Reitz geschrieben: My conclusion: Don't ditch probing. It increases entropy, why would you ditch probing? Just combine it with the extension and if both don't seem to match, that's an error. I actually kind of like this (in addition to preventing bad writes). If we do have file name (or other metadata-specific) information that gives us a clue, use it to double check the guess. If we don't, rely on probing like we do today. .qcow2 should never contain anything but qcow2, .iso should always be raw. If we don't have a recognised extension, anything is okay. We need to decide what to do with ambiguous extensions like .img or .vhd. This again wouldn't be a perfect solution that catches all cases, but it improves the situation and shouldn't cause too many compatibility issues. So, for fixing (b): Just use the extensions as a safeguard and issue a warning for now. We can discuss about making it an error later. Warnings are useless. They warn too late. It needs to be an error, and I think when we don't require the filename check, it's reasonable enough to do it from the start. And for fixing (c): As you pointed out, if guests wrote some probe-matching pattern in the past, it would break qemu (which is what we're trying to fix). Since noone ever said that some guest did that by accident, I think we can safely assume that prohibiting such writes will not hurt anyone in the future either; at least there are no compatibility issues Good point, thanks for pointing it out. Kevin
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
On 11/05/2014 12:11 PM, Max Reitz wrote: +err-msg = g_strdup_printf(%s: %s, msg1, strerror(abs(os_errno))); I don't, we really should fix the callers. Of course I understand, but this patch doesn't make matters worse, as long as there are not systems which have negative values for errno POSIX requires all defined errno values to be positive; negative errno values are unambiguous as values that will cause strerror() to have to generate a message about an unknown value. (which I think we generally assume not to exist throughout qemu). That's why I'm fine with it. We should fix the callers but I don't see why we shouldn't apply this patch as well. This patch is a bandaid; it makes it harder to find callers that need to be fixed. I'd almost argue the exact opposite - add an assert(os_errno 0). Then we'd loudly break on broken callers, making them easier to find. A similar issue already came up and led to commit b276d2499, where callers of error_setg_errno() assumed that it would not clobber errno, so we fixed some of the callers but also applied that commit which just saves errno because there's no reason not to. If we're willing to accept the convenience so that callers can be lazy, then I like this patch. If we want to fix bugs in the callers, then this patch makes it harder to find those bugs. I'm actually 60:40 in favor of this patch (I think the convenience outweighs an audit of fixing all callers); but if we do that, then we might also want to intentionally switch existing callers to pass negative values rather than declaring that passing a negative value is a bug. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call
On Wednesday 05 November 2014 04:37 PM, Alexander Graf wrote:
On 05.11.14 11:37, Aravinda Prasad wrote:
On Wednesday 05 November 2014 02:02 PM, Alexander Graf wrote:
On 05.11.14 08:13, Aravinda Prasad wrote:
This patch adds FWNMI support in qemu for powerKVM
guests by handling the ibm,nmi-register rtas call.
Whenever OS issues ibm,nmi-register RTAS call, the
machine check notification address is saved and the
machine check interrupt vector 0x200 is patched to
issue a private hcall.
This patch also handles the cases when multi-processors
experience machine check at or about the same time.
As per PAPR, subsequent processors serialize waiting
for the first processor to issue the ibm,nmi-interlock call.
The second processor retries if the first processor which
received a machine check is still reading the error log
and is yet to issue ibm,nmi-interlock call.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr_hcall.c| 16 +++
hw/ppc/spapr_rtas.c | 93
+++
include/hw/ppc/spapr.h | 17 +++
pc-bios/spapr-rtas/spapr-rtas.S | 38
4 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f16160..eceb5e5 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -97,6 +97,9 @@ struct rtas_mc_log {
struct rtas_error_log err_log;
};
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
static void do_spr_sync(void *arg)
{
struct SPRSyncState *s = arg;
@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
/*
+ * Only one VCPU can process machine check NMI at a time. Hence
+ * set the lock mc_in_progress. Once the VCPU finishes processing
+ * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+ * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+ * NMI we return 0 asking the VCPU to retry h_report_mc_err
+ */
+if (mc_in_progress == 1) {
Please don't depend on bools being numbers. Use true / false. For if()s,
just don't use == at all - it makes it more readable.
ok
+return 0;
+}
+
+mc_in_progress = 1;
+
+/*
* We save the original r3 register in SPRG2 in 0x200 vector,
* which is patched during call to ibm.nmi-register. Original
* r3 is required to be included in error log
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2ec2a8e..71c7662 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -36,6 +36,9 @@
#include libfdt.h
+#define BRANCH_INST_MASK 0xFC00
+extern bool mc_in_progress;
Please put this into the spapr struct.
ok
+
static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment
*spapr,
uint32_t token, uint32_t nargs,
target_ulong args,
@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
rtas_st(rets, 0, ret);
}
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+ sPAPREnvironment *spapr,
+ uint32_t token, uint32_t nargs,
+ target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+int i;
+uint32_t ori_inst = 0x6063;
+uint32_t branch_inst = 0x4802;
+target_ulong guest_machine_check_addr;
+uint32_t trampoline[TRAMPOLINE_INSTS];
+int total_inst = sizeof(trampoline) / sizeof(uint32_t);
ARRAY_SIZE(trampoline), though I don't quite understand why you need a
variable that contains the same value as a constant (TRAMPOLINE_INSTS).
But since you're moving all of those bits into variable fields on the
rtas blob itself as we discussed in the last version, I guess this code
will go away anyways ;).
I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR
number and branch address in the trampoline and also, depending on
whether the guest running in LE/BE we may need to flip the bits in the
trampoline before copying it to 0x200 machine check vector.
As rtas-blob is part of the guest memory I do not want to patch these in
rtas-blob, hence I copy the trampoline from the rtas-blob to an array,
modify accordingly and then move it to 0x200 machine check vector.
Yes, you will still need the array. But the array should be dynamically
sized based on spapr-rtas_info-fwnmi_size which you extract from the
blob on load.
That way you wouldn't need the total_inst variable anymore ;).
Yes, I will fix it that way.
+PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+/* Store the system reset and machine check address */
+
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call
On 05.11.14 12:24, Aravinda Prasad wrote:
On Wednesday 05 November 2014 04:37 PM, Alexander Graf wrote:
On 05.11.14 11:37, Aravinda Prasad wrote:
On Wednesday 05 November 2014 02:02 PM, Alexander Graf wrote:
On 05.11.14 08:13, Aravinda Prasad wrote:
This patch adds FWNMI support in qemu for powerKVM
guests by handling the ibm,nmi-register rtas call.
Whenever OS issues ibm,nmi-register RTAS call, the
machine check notification address is saved and the
machine check interrupt vector 0x200 is patched to
issue a private hcall.
This patch also handles the cases when multi-processors
experience machine check at or about the same time.
As per PAPR, subsequent processors serialize waiting
for the first processor to issue the ibm,nmi-interlock call.
The second processor retries if the first processor which
received a machine check is still reading the error log
and is yet to issue ibm,nmi-interlock call.
Signed-off-by: Aravinda Prasad aravi...@linux.vnet.ibm.com
---
hw/ppc/spapr_hcall.c| 16 +++
hw/ppc/spapr_rtas.c | 93
+++
include/hw/ppc/spapr.h | 17 +++
pc-bios/spapr-rtas/spapr-rtas.S | 38
4 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f16160..eceb5e5 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -97,6 +97,9 @@ struct rtas_mc_log {
struct rtas_error_log err_log;
};
+/* Whether machine check handling is in progress by any CPU */
+bool mc_in_progress;
+
static void do_spr_sync(void *arg)
{
struct SPRSyncState *s = arg;
@@ -678,6 +681,19 @@ static target_ulong h_report_mc_err(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
cpu_synchronize_state(CPU(ppc_env_get_cpu(env)));
/*
+ * Only one VCPU can process machine check NMI at a time. Hence
+ * set the lock mc_in_progress. Once the VCPU finishes processing
+ * NMI, it executes ibm,nmi-interlock and mc_in_progress is unset
+ * in ibm,nmi-interlock handler. Meanwhile if other VCPUs encounter
+ * NMI we return 0 asking the VCPU to retry h_report_mc_err
+ */
+if (mc_in_progress == 1) {
Please don't depend on bools being numbers. Use true / false. For if()s,
just don't use == at all - it makes it more readable.
ok
+return 0;
+}
+
+mc_in_progress = 1;
+
+/*
* We save the original r3 register in SPRG2 in 0x200 vector,
* which is patched during call to ibm.nmi-register. Original
* r3 is required to be included in error log
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2ec2a8e..71c7662 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -36,6 +36,9 @@
#include libfdt.h
+#define BRANCH_INST_MASK 0xFC00
+extern bool mc_in_progress;
Please put this into the spapr struct.
ok
+
static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment
*spapr,
uint32_t token, uint32_t nargs,
target_ulong args,
@@ -290,6 +293,90 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
rtas_st(rets, 0, ret);
}
+static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+ sPAPREnvironment *spapr,
+ uint32_t token, uint32_t nargs,
+ target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+int i;
+uint32_t ori_inst = 0x6063;
+uint32_t branch_inst = 0x4802;
+target_ulong guest_machine_check_addr;
+uint32_t trampoline[TRAMPOLINE_INSTS];
+int total_inst = sizeof(trampoline) / sizeof(uint32_t);
ARRAY_SIZE(trampoline), though I don't quite understand why you need a
variable that contains the same value as a constant (TRAMPOLINE_INSTS).
But since you're moving all of those bits into variable fields on the
rtas blob itself as we discussed in the last version, I guess this code
will go away anyways ;).
I think we still need this. We need to patch the KVMPPC_H_REPORT_MC_ERR
number and branch address in the trampoline and also, depending on
whether the guest running in LE/BE we may need to flip the bits in the
trampoline before copying it to 0x200 machine check vector.
As rtas-blob is part of the guest memory I do not want to patch these in
rtas-blob, hence I copy the trampoline from the rtas-blob to an array,
modify accordingly and then move it to 0x200 machine check vector.
Yes, you will still need the array. But the array should be dynamically
sized based on spapr-rtas_info-fwnmi_size which you extract from the
spapr-rtas_info.fwnmi_size of course ;). No need for yet another
allocation to keep track of.
blob on load.
That way you wouldn't need the total_inst variable anymore ;).
Yes, I will fix it that way.
+
Re: [Qemu-devel] [PATCH v7 09/16] hw/vfio/platform: add vfio-platform support
On 11/05/2014 11:29 AM, Alexander Graf wrote: On 31.10.14 15:05, Eric Auger wrote: Minimal VFIO platform implementation supporting - register space user mapping, - IRQ assignment based on eventfds handled on qemu side. irqfd kernel acceleration comes in a subsequent patch. Signed-off-by: Kim Phillips kim.phill...@linaro.org Signed-off-by: Eric Auger eric.au...@linaro.org --- v6 - v7: - compat is not exposed anymore as a user option. Rationale is the vfio device became abstract and a specialization is needed anyway. The derived device must set the compat string. - in v6 vfio_start_irq_injection was exposed in vfio-platform.h. A new function dubbed vfio_register_irq_starter replaces it. It registers a machine init done notifier that programs starts all dynamic VFIO device IRQs. This function is supposed to be called by the machine file. A set of static helper routines are added too. It must be called before the creation of the platform bus device. v5 - v6: - vfio_device property renamed into host property - correct error handling of VFIO_DEVICE_GET_IRQ_INFO ioctl and remove PCI related comment - remove declaration of vfio_setup_irqfd and irqfd_allowed property.Both belong to next patch (irqfd) - remove declaration of vfio_intp_interrupt in vfio-platform.h - functions that can be static get this characteristic - remove declarations of vfio_region_ops, vfio_memory_listener, group_list, vfio_address_spaces. All are moved to vfio-common.h - remove vfio_put_device declaration and definition - print_regions removed. code moved into vfio_populate_regions - replace DPRINTF by trace events - new helper routine to set the trigger eventfd - dissociate intp init from the injection enablement: vfio_enable_intp renamed into vfio_init_intp and new function named vfio_start_eventfd_injection - injection start moved to vfio_start_irq_injection (not anymore in vfio_populate_interrupt) - new start_irq_fn field in VFIOPlatformDevice corresponding to the function that will be used for starting injection - user handled eventfd: x add mutex to protect IRQ state list manipulation, x correct misleading comment in vfio_intp_interrupt. x Fix bugs thanks to fake interrupt modality - VFIOPlatformDeviceClass becomes abstract - add error_setg in vfio_platform_realize v4 - v5: - vfio-plaform.h included first - cleanup error handling in *populate*, vfio_get_device, vfio_enable_intp - vfio_put_device not called anymore - add some includes to follow vfio policy v3 - v4: [Eric Auger] - merge of vfio: Add initial IRQ support in platform device to get a full functional patch although perfs are limited. - removal of unrealize function since I currently understand it is only used with device hot-plug feature. v2 - v3: [Eric Auger] - further factorization between PCI and platform (VFIORegion, VFIODevice). same level of functionality. = v2: [Kim Philipps] - Initial Creation of the device supporting register space mapping --- hw/vfio/Makefile.objs | 1 + hw/vfio/platform.c | 672 include/hw/vfio/vfio-common.h | 1 + include/hw/vfio/vfio-platform.h | 87 ++ trace-events| 12 + 5 files changed, 773 insertions(+) create mode 100644 hw/vfio/platform.c create mode 100644 include/hw/vfio/vfio-platform.h diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs index e31f30e..c5c76fe 100644 --- a/hw/vfio/Makefile.objs +++ b/hw/vfio/Makefile.objs @@ -1,4 +1,5 @@ ifeq ($(CONFIG_LINUX), y) obj-$(CONFIG_SOFTMMU) += common.o obj-$(CONFIG_PCI) += pci.o +obj-$(CONFIG_SOFTMMU) += platform.o endif diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c new file mode 100644 index 000..9f66610 --- /dev/null +++ b/hw/vfio/platform.c @@ -0,0 +1,672 @@ +/* + * vfio based device assignment support - platform devices + * + * Copyright Linaro Limited, 2014 + * + * Authors: + * Kim Phillips kim.phill...@linaro.org + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + * Based on vfio based PCI device assignment support: + * Copyright Red Hat, Inc. 2012 + */ + +#include linux/vfio.h +#include sys/ioctl.h + +#include hw/vfio/vfio-platform.h +#include qemu/error-report.h +#include qemu/range.h +#include sysemu/sysemu.h +#include exec/memory.h +#include qemu/queue.h +#include hw/sysbus.h +#include trace.h +#include hw/platform-bus.h + +static void vfio_intp_interrupt(VFIOINTp *intp); +typedef void (*eventfd_user_side_handler_t)(VFIOINTp *intp); +static int vfio_set_trigger_eventfd(VFIOINTp *intp, +eventfd_user_side_handler_t handler); + +/* + * Functions only used when eventfd are handled on user-side + * ie. without irqfd + */ + +/** + * vfio_platform_eoi - IRQ completion
Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions
On 2014/11/5 15:05, Michael Tokarev wrote:
04.11.2014 16:25, Alex Bennée wrote:
zhanghailiang zhang.zhanghaili...@huawei.com writes:
For some qemu_chr_parse_* functions, we just check whether the parameter
is NULL or not, but do not check if it is empty.
For example:
qemu-system-x86_64 -chardev pipe,id=id,path=
It will pass the check of NULL but will not find the error until
trying to open it, while essentially missing and empty parameter
is the same thing.
So check the parameters for emptiness too, and avoid emptiness
check at open time.
Signed-off-by: zhanghailiang zhang.zhanghaili...@huawei.com
Signed-off-by: Michael Tokarev m...@tls.msk.ru
---
qemu-char.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index bd0709b..a09bbf6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1084,11 +1084,6 @@ static CharDriverState
*qemu_chr_open_pipe(ChardevHostdev *opts)
char filename_out[CHR_MAX_FILENAME_SIZE];
const char *filename = opts-device;
-if (filename == NULL) {
-fprintf(stderr, chardev: pipe: no filename given\n);
-return NULL;
-}
-
You seem to have dropped a check here, are you sure all avenues into
this code have validated filename? What if a new function gets added?
Hi Michael,
Yes, the code first calls parse_pipe() and only after it is
successfully completed, it calls open_pipe(). I don't see
Unfortunately :( , That's right for hmp command 'chardev-add' and
startUp configure, but not true for qmp command 'chardev-add'.
It is my fault, i didn't test qmp command before :(
The call process is different from hmp command,
Its route not include parse_* function.
process:
qmp_call_cmd
---qmp_marshal_input_chardev_add
---qmp_chardev_add
---qemu_chr_open_pipe
test result:
{ execute : chardev-add,arguments : { id : bar1,backend : \
{ type : pipe,data : {device : } } } }
{id:libvirt-12,error:{class:GenericError,\
desc:Failed to create chardev}}
As you see, we still need check if filename is empty or not in open_pipe.
(Actually, filename will still never to be NULL,
it is assured by the 'qmp_marshal ' layer, but better to keep it there)
So what's your suggestion? Keep two checks both in open_* and parse_*?
Or move check into open_*? (It should be OK to g_strdup(NULL)). Thanks.
a good reason for having assert here.
Agreed, assert here is still not unnecessary,
filename will never to be NULL in these two cases.
At a minimum I'd replace it with a g_assert(filename) to make the
calling contract clear.
This is an internal set of APIs for a chr device, each kind is
having a pair of functions which are called in order (first parse,
next open), -- _that_ is the contract.
[]
All this boilerplate checking makes me think that either the qemu_opt
machinery should be ensuring we get a valid option string?
Might be a good idea, yes, but that'd be a huge change, since that
should be done in a lot of places, and in many cases we can't
express our rules easily (eg, only one of two parameters should
be present). I think at this stage adding simple checks to
_parse functions is the way to go, and it is easy to read too.
Thanks,
/mjt
.
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
On 05/11/2014 12:11, Max Reitz wrote: Of course I understand, but this patch doesn't make matters worse, as long as there are not systems which have negative values for errno (which I think we generally assume not to exist throughout qemu). That's why I'm fine with it. We should fix the callers but I don't see why we shouldn't apply this patch as well. A similar issue already came up and led to commit b276d2499, where callers of error_setg_errno() assumed that it would not clobber errno, so we fixed some of the callers but also applied that commit which just saves errno because there's no reason not to. I think side effect are a different matter than misuse of QEMU. There are only 157 calls to error_setg_errno; 67 use errno as the argument, and 4 use an explicit errno value (one of them is the wrong -EBUSY). The other 86 seem correct and should not be hard to audit. Let's instead add an assertion check to error_setg_errno. Paolo
Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev
On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
pci_qdev_init() checks whether return value is 0 or not to figure out pci
device is initialized successfully. Otherwise, device_realize() in qdev
checks that return value is negative value to figure out the device is
realized successfully.
When pci device returns positive number, pci_qdev_init() thinks that error is
occured and makes the device unregistered. Nevertheless, qdev thinks that
device is realized.
Finally, crash is occured by commands like 'qtree' that traverse qdev list.
So, pci_qdev_init() returns -1 when init function returns not 0.
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
Question: is there a simple way to trigger this error?
---
hw/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 371699c..c149fdf 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
rc = pc-init(pci_dev);
if (rc != 0) {
do_pci_unregister_device(pci_dev);
-return rc;
+return -1;
}
}
--
2.1.0
Re: [Qemu-devel] [PULL 2.2 00/33] ppc patch queue 2014-11-04 for 2.2
On 4 November 2014 19:26, Alexander Graf ag...@suse.de wrote: Hi Peter, This is my current patch queue for ppc. Please pull. Alex The following changes since commit d5b4dc3b50175f0c34f3cf4b053e123fb37f5aed: Merge remote-tracking branch 'remotes/afaerber/tags/qom-devices-for-peter' into staging (2014-11-04 17:33:34 +) are available in the git repository at: git://github.com/agraf/qemu.git tags/signed-ppc-for-upstream for you to fetch changes up to 875d0edd7ce262d61a62c391b49edc2602c06150: target-ppc: Fix Altivec Round Opcodes (2014-11-04 20:22:08 +0100) Applied the updated version which passes make check, thanks. -- PMM
Re: [Qemu-devel] [PATCH v7 09/16] hw/vfio/platform: add vfio-platform support
On 05.11.14 13:03, Eric Auger wrote: On 11/05/2014 11:29 AM, Alexander Graf wrote: On 31.10.14 15:05, Eric Auger wrote: Minimal VFIO platform implementation supporting - register space user mapping, - IRQ assignment based on eventfds handled on qemu side. irqfd kernel acceleration comes in a subsequent patch. Signed-off-by: Kim Phillips kim.phill...@linaro.org Signed-off-by: Eric Auger eric.au...@linaro.org --- v6 - v7: - compat is not exposed anymore as a user option. Rationale is the vfio device became abstract and a specialization is needed anyway. The derived device must set the compat string. - in v6 vfio_start_irq_injection was exposed in vfio-platform.h. A new function dubbed vfio_register_irq_starter replaces it. It registers a machine init done notifier that programs starts all dynamic VFIO device IRQs. This function is supposed to be called by the machine file. A set of static helper routines are added too. It must be called before the creation of the platform bus device. v5 - v6: - vfio_device property renamed into host property - correct error handling of VFIO_DEVICE_GET_IRQ_INFO ioctl and remove PCI related comment - remove declaration of vfio_setup_irqfd and irqfd_allowed property.Both belong to next patch (irqfd) - remove declaration of vfio_intp_interrupt in vfio-platform.h - functions that can be static get this characteristic - remove declarations of vfio_region_ops, vfio_memory_listener, group_list, vfio_address_spaces. All are moved to vfio-common.h - remove vfio_put_device declaration and definition - print_regions removed. code moved into vfio_populate_regions - replace DPRINTF by trace events - new helper routine to set the trigger eventfd - dissociate intp init from the injection enablement: vfio_enable_intp renamed into vfio_init_intp and new function named vfio_start_eventfd_injection - injection start moved to vfio_start_irq_injection (not anymore in vfio_populate_interrupt) - new start_irq_fn field in VFIOPlatformDevice corresponding to the function that will be used for starting injection - user handled eventfd: x add mutex to protect IRQ state list manipulation, x correct misleading comment in vfio_intp_interrupt. x Fix bugs thanks to fake interrupt modality - VFIOPlatformDeviceClass becomes abstract - add error_setg in vfio_platform_realize v4 - v5: - vfio-plaform.h included first - cleanup error handling in *populate*, vfio_get_device, vfio_enable_intp - vfio_put_device not called anymore - add some includes to follow vfio policy v3 - v4: [Eric Auger] - merge of vfio: Add initial IRQ support in platform device to get a full functional patch although perfs are limited. - removal of unrealize function since I currently understand it is only used with device hot-plug feature. v2 - v3: [Eric Auger] - further factorization between PCI and platform (VFIORegion, VFIODevice). same level of functionality. = v2: [Kim Philipps] - Initial Creation of the device supporting register space mapping --- hw/vfio/Makefile.objs | 1 + hw/vfio/platform.c | 672 include/hw/vfio/vfio-common.h | 1 + include/hw/vfio/vfio-platform.h | 87 ++ trace-events| 12 + 5 files changed, 773 insertions(+) create mode 100644 hw/vfio/platform.c create mode 100644 include/hw/vfio/vfio-platform.h diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs index e31f30e..c5c76fe 100644 --- a/hw/vfio/Makefile.objs +++ b/hw/vfio/Makefile.objs @@ -1,4 +1,5 @@ ifeq ($(CONFIG_LINUX), y) obj-$(CONFIG_SOFTMMU) += common.o obj-$(CONFIG_PCI) += pci.o +obj-$(CONFIG_SOFTMMU) += platform.o endif diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c new file mode 100644 index 000..9f66610 --- /dev/null +++ b/hw/vfio/platform.c @@ -0,0 +1,672 @@ +/* + * vfio based device assignment support - platform devices + * + * Copyright Linaro Limited, 2014 + * + * Authors: + * Kim Phillips kim.phill...@linaro.org + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + * Based on vfio based PCI device assignment support: + * Copyright Red Hat, Inc. 2012 + */ + +#include linux/vfio.h +#include sys/ioctl.h + +#include hw/vfio/vfio-platform.h +#include qemu/error-report.h +#include qemu/range.h +#include sysemu/sysemu.h +#include exec/memory.h +#include qemu/queue.h +#include hw/sysbus.h +#include trace.h +#include hw/platform-bus.h + +static void vfio_intp_interrupt(VFIOINTp *intp); +typedef void (*eventfd_user_side_handler_t)(VFIOINTp *intp); +static int vfio_set_trigger_eventfd(VFIOINTp *intp, +eventfd_user_side_handler_t handler); + +/* + * Functions only used when eventfd are handled on user-side + * ie. without irqfd + */ + +/** +
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
-Original Message- From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo Bonzini Sent: Wednesday, November 05, 2014 9:45 PM To: Max Reitz; SeokYeon Hwang; qemu-devel@nongnu.org Cc: arm...@redhat.com; paolo.bonz...@gmail.com Subject: Re: [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error. On 05/11/2014 12:11, Max Reitz wrote: Of course I understand, but this patch doesn't make matters worse, as long as there are not systems which have negative values for errno (which I think we generally assume not to exist throughout qemu). That's why I'm fine with it. We should fix the callers but I don't see why we shouldn't apply this patch as well. A similar issue already came up and led to commit b276d2499, where callers of error_setg_errno() assumed that it would not clobber errno, so we fixed some of the callers but also applied that commit which just saves errno because there's no reason not to. I think side effect are a different matter than misuse of QEMU. There are only 157 calls to error_setg_errno; 67 use errno as the argument, and 4 use an explicit errno value (one of them is the wrong - EBUSY). The other 86 seem correct and should not be hard to audit. Let's instead add an assertion check to error_setg_errno. Paolo I have expected to come out several opinions about this patch. The use of negative errno on strerror() was obviously wrong. But that does not mean it is wrong to use the negative errno on error_set_errno(). The reason that I chose this one among the solutions is to change function specification. I think it seems good to us to respect the tradition of the developers that use negative errno. But if error_set_errno() has strict specification - so, we must not change it's spec - I agree with Paolo's opinion.
Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev
Michael S. Tsirkin m...@redhat.com writes: On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote: pci_qdev_init() checks whether return value is 0 or not to figure out pci device is initialized successfully. Otherwise, device_realize() in qdev checks that return value is negative value to figure out the device is realized successfully. When pci device returns positive number, pci_qdev_init() thinks that error is occured and makes the device unregistered. Nevertheless, qdev thinks that device is realized. Finally, crash is occured by commands like 'qtree' that traverse qdev list. So, pci_qdev_init() returns -1 when init function returns not 0. Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com Question: is there a simple way to trigger this error? Next question: what's the contract of PCIDeviceClass method init()? Positive return value feels like bug to me...
Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev
On 05/11/2014 14:16, Markus Armbruster wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote: pci_qdev_init() checks whether return value is 0 or not to figure out pci device is initialized successfully. Otherwise, device_realize() in qdev checks that return value is negative value to figure out the device is realized successfully. When pci device returns positive number, pci_qdev_init() thinks that error is occured and makes the device unregistered. Nevertheless, qdev thinks that device is realized. Finally, crash is occured by commands like 'qtree' that traverse qdev list. So, pci_qdev_init() returns -1 when init function returns not 0. Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com Question: is there a simple way to trigger this error? Next question: what's the contract of PCIDeviceClass method init()? Positive return value feels like bug to me... I think bypassing the question by converting to realize makes the most sense... Paolo
Re: [Qemu-devel] [PULL 00/01] Adding new syscalls to seccomp whitelist
On 22 October 2014 11:02, Peter Maydell peter.mayd...@linaro.org wrote: On 22 October 2014 09:04, Eduardo Otubo eduardo.ot...@profitbricks.com wrote: On Fri, Sep 19, 2014 at 08:11:14AM -0700, Peter Maydell wrote: You have compile problems in current master as well. Your macros probably need to guard themselves on whether the syscall they're adding to the list actually exists on the host. (See bug https://bugs.launchpad.net/qemu/+bug/1363641 -- select doesn't exist as a syscall on all archs.) The fix for that problem is upstream at libseccomp. The maintainer has no plans yet to make a new release, though. Once he does a release nad fix this issue, I'll go and resubmit this pull request. The bug is already in QEMU master, so that needs a fix now regardless of the status of this new patch. Ping! You still need to fix this for QEMU 2.2 (minimally, by disabling seccomp in configure for hosts it won't work on). thanks -- PMM
Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions
zhanghailiang zhang.zhanghaili...@huawei.com writes: On 2014/11/5 15:05, Michael Tokarev wrote: 04.11.2014 16:25, Alex Bennée wrote: zhanghailiang zhang.zhanghaili...@huawei.com writes: snip a good reason for having assert here. Agreed, assert here is still not unnecessary, filename will never to be NULL in these two cases. At a minimum I'd replace it with a g_assert(filename) to make the calling contract clear. This is an internal set of APIs for a chr device, each kind is having a pair of functions which are called in order (first parse, next open), -- _that_ is the contract. assert isn't really about informing the user, it just makes an explicit statement that this API will always have a filled in filename and if it ever gets a NULL that's a programming bug in using the API, internal or otherwise. [] All this boilerplate checking makes me think that either the qemu_opt machinery should be ensuring we get a valid option string? Might be a good idea, yes, but that'd be a huge change, since that should be done in a lot of places, and in many cases we can't express our rules easily (eg, only one of two parameters should be present). I think at this stage adding simple checks to _parse functions is the way to go, and it is easy to read too. Yes, I wasn't intending to suggest expanding this patch set to encompase the larger task ;-) -- Alex Bennée
Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev
On Wed, Nov 05, 2014 at 02:18:31PM +0100, Paolo Bonzini wrote: On 05/11/2014 14:16, Markus Armbruster wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote: pci_qdev_init() checks whether return value is 0 or not to figure out pci device is initialized successfully. Otherwise, device_realize() in qdev checks that return value is negative value to figure out the device is realized successfully. When pci device returns positive number, pci_qdev_init() thinks that error is occured and makes the device unregistered. Nevertheless, qdev thinks that device is realized. Finally, crash is occured by commands like 'qtree' that traverse qdev list. So, pci_qdev_init() returns -1 when init function returns not 0. Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com Question: is there a simple way to trigger this error? Next question: what's the contract of PCIDeviceClass method init()? Positive return value feels like bug to me... I think bypassing the question by converting to realize makes the most sense... Paolo I'm fine with doing that but Markus's patches wouldn't yet have solved the problem by themselves since init is still around, right? This probably means fixing this bug can't justify merging the realize patchset after freeze. -- MST
Re: [Qemu-devel] [PATCH] pci: fixed mismatch of error-handling between pci_qdev_init() and qdev
-Original Message-
From: Michael S. Tsirkin [mailto:m...@redhat.com]
Sent: Wednesday, November 05, 2014 9:46 PM
To: SeokYeon Hwang
Cc: qemu-devel@nongnu.org; arm...@redhat.com; pbonz...@redhat.com
Subject: Re: [PATCH] pci: fixed mismatch of error-handling between
pci_qdev_init() and qdev
On Wed, Nov 05, 2014 at 07:11:51PM +0900, SeokYeon Hwang wrote:
pci_qdev_init() checks whether return value is 0 or not to figure out
pci device is initialized successfully. Otherwise, device_realize() in
qdev checks that return value is negative value to figure out the device
is realized successfully.
When pci device returns positive number, pci_qdev_init() thinks that
error is occured and makes the device unregistered. Nevertheless, qdev
thinks that device is realized.
Finally, crash is occured by commands like 'qtree' that traverse qdev
list.
So, pci_qdev_init() returns -1 when init function returns not 0.
Signed-off-by: SeokYeon Hwang syeon.hw...@samsung.com
Question: is there a simple way to trigger this error?
You can reproduce this error by changing the return value of the unimportant
device's init() to 1.
Actually, I found this bug through the device that is not exist in upstream
qemu.
(It is Tizen emulator's device.)
---
hw/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 371699c..c149fdf 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1766,7 +1766,7 @@ static int pci_qdev_init(DeviceState *qdev)
rc = pc-init(pci_dev);
if (rc != 0) {
do_pci_unregister_device(pci_dev);
-return rc;
+return -1;
}
}
--
2.1.0
Re: [Qemu-devel] [PATCH] error: fixed error_set_errno() to deal with a negative type of os_error.
Paolo Bonzini bonz...@gnu.org writes: On 05/11/2014 12:11, Max Reitz wrote: Of course I understand, but this patch doesn't make matters worse, as long as there are not systems which have negative values for errno (which I think we generally assume not to exist throughout qemu). That's why I'm fine with it. We should fix the callers but I don't see why we shouldn't apply this patch as well. A similar issue already came up and led to commit b276d2499, where callers of error_setg_errno() assumed that it would not clobber errno, so we fixed some of the callers but also applied that commit which just saves errno because there's no reason not to. I think side effect are a different matter than misuse of QEMU. Yes. error_setg_errno() had an unclear contract regarding errno. Some callers unjustifiedly assumed that it preserves errno. We clearly needed to clarify the contract. Both preserves and doesn't preserve make sense. We picked the one that saves us fixing up callers. error_setg_errno()'s contract regarding os_error isn't spelled out, but the general errno contract applies by association: needs to be non-negative. There are only 157 calls to error_setg_errno; 67 use errno as the argument, and 4 use an explicit errno value (one of them is the wrong -EBUSY). The other 86 seem correct and should not be hard to audit. Negative value is probably just a sign error, but it *could* be a logic error. I'd rather not sweep the latter under the carpet. Let's instead add an assertion check to error_setg_errno. Yes, please.
Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub
On Tue, 4 Nov 2014 19:09:43 +, Peter Maydell said:
On 4 November 2014 17:51, Martin Simmons mar...@lispworks.com wrote:
While using qemu with gdb target remote to debug an application that uses
fork and exec, the qemu process receives SIGSTOP every time the forked
process
terminates (sending SIGCHLD).
This is caused by a missing call to gdb_signal_to_target in gdbstub.c, which
is fixed by this patch:
Signed-off-by: Martin Simmons mar...@lispworks.com
diff --git a/gdbstub.c b/gdbstub.c
index d1b5afd..6a73a35 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -823,7 +823,9 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
action = *p++;
signal = 0;
if (action == 'C' || action == 'S') {
-signal = strtoul(p, (char **)p, 16);
+signal = gdb_signal_to_target (strtoul(p, (char **)p,
16));
+if (signal == -1)
+signal = 0;
} else if (action != 'c' action != 's') {
res = 0;
break;
The if() statement should have braces for our coding style,
and no space before the '(' in function calls; otherwise this
looks good to me.
Do you want a new patch with it like that? I was following the style of the
rest of that file, in particular the other 'C' case :-(
__Martin
Re: [Qemu-devel] [RFC][PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough
On Wed, Nov 05, 2014 at 03:22:59PM +0800, Tiejun Chen wrote:
Currently IGD drivers always need to access PCH by 1f.0, and
PCH vendor/device id is used to identify the card.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
hw/i386/pc_piix.c | 28 +++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b559181..b19c7a9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -50,7 +50,7 @@
#include cpu.h
#include qemu/error-report.h
#ifdef CONFIG_XEN
-# include xen/hvm/hvm_info_table.h
+#include xen/hvm/hvm_info_table.h
#endif
#define MAX_IDE_BUS 2
@@ -452,6 +452,31 @@ static void pc_init_isa(MachineState *machine)
}
#ifdef CONFIG_XEN
+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
+{
+struct PCIDevice *dev;
+Error *local_err = NULL;
+uint16_t device_id = 0x;
+
+/* Currently IGD drivers always need to access PCH by 1f.0. */
+dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+xen-igd-passthrough-isa-bridge);
+
+/* Identify PCH card with its own real vendor/device ids.
+ * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+ */
+if (dev) {
+device_id = object_property_get_int(OBJECT(dev), device-id,
+local_err);
+if (!local_err device_id != 0x) {
+pci_config_set_device_id(dev-config, device_id);
+return;
+}
+}
+
+fprintf(stderr, xen set xen-igd-passthrough-isa-bridge failed\n);
+}
+
static void pc_xen_hvm_init(MachineState *machine)
{
PCIBus *bus;
@@ -461,6 +486,7 @@ static void pc_xen_hvm_init(MachineState *machine)
bus = pci_find_primary_bus();
if (bus != NULL) {
pci_create_simple(bus, -1, xen-platform);
+xen_igd_passthrough_isa_bridge_create(bus);
}
}
#endif
Can't we defer this step until the GPU is added?
This way there won't be need to poke at host device
directly, you could get all info from dev-config
of the host device.
Additionally the correct bridge would be initialized automatically.
--
1.9.1
Re: [Qemu-devel] [PATCH RESEND] vhost-user-test: Fix 'make check' broken on glib 2.26
On 5 November 2014 01:00, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com After commit 89b516d8, some logics is turbid and breaks 'make check' as below errors: tests/vhost-user-test.c: In function '_cond_wait_until': tests/vhost-user-test.c:154: error: 'G_TIME_SPAN_SECOND' undeclared (first use in this function) tests/vhost-user-test.c:154: error: (Each undeclared identifier is reported only once tests/vhost-user-test.c:154: error: for each function it appears in.) tests/vhost-user-test.c: In function 'read_guest_mem': tests/vhost-user-test.c:192: warning: implicit declaration of function 'g_get_monotonic_time' tests/vhost-user-test.c:192: warning: nested extern declaration of 'g_get_monotonic_time' tests/vhost-user-test.c:192: error: 'G_TIME_SPAN_SECOND' undeclared (first use in this function) make: *** [tests/vhost-user-test.o] Error 1 First, vhost-usr-test.c rely on glib-compat.h because of using G_TIME_SPAN_SECOND [glib 2.26] and g_get_monotonic_time(), but vhost-usr-test.c defined QEMU_GLIB_COMPAT_H, which make glib-compat.h will not be included. Second, if we remove QEMU_GLIB_COMPAT_H definability in vhost-usr-test.c, then we will get below warnings: tests/vhost-user-test.c: In function 'read_guest_mem': tests/vhost-user-test.c:190: warning: passing argument 1 of 'g_mutex_lock' from incompatible pointer type tests/vhost-user-test.c:234: warning: passing argument 1 of 'g_mutex_unlock' from incompatible pointer type That's because glib-compat.h redefine the g_mutex_lock/unlock function. Those functions' arguments is CompatGMutex/CompatGCond, but vhost-user-test.c is using GMutex/GCond, which cause the type is not consistent. We can rerealize those functions of vhost-user-test.c, which need a lots of patches. Let's simply address it, and leave this file alone. Signed-off-by: Gonglei arei.gong...@huawei.com --- Applied to master, thanks. -- PMM
Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub
On 5 November 2014 13:50, Martin Simmons mar...@lispworks.com wrote:
On Tue, 4 Nov 2014 19:09:43 +, Peter Maydell said:
The if() statement should have braces for our coding style,
and no space before the '(' in function calls; otherwise this
looks good to me.
Do you want a new patch with it like that? I was following the style of the
rest of that file, in particular the other 'C' case :-(
Yes, if you could respin it that would be nice. Unfortunately there
are still areas of our codebase which don't follow the coding
style; we update bits of code as we change them, so new patches
follow the style guide for the lines they touch. You can use
scripts/checkpatch.pl to check your patch for the most common
issues.
thanks
-- PMM
[Qemu-devel] [PATCH] hw/pci: fix crash on shpc error flow
If the pci bridge enters in error flow as part
of init process it will only delete the shpc mmio
subregion but not remove it from the properties list,
resulting in segmentation fault when the bridge runs
the exit function.
Example: add a pci bridge without specifing the chassis number:
qemu-bin ... -device pci-bridge,id=p1
Result:
(qemu) qemu-system-x86_64: -device pci-bridge,id=p1: Bridge chassis not
specified. Each bridge is required to be assigned a unique chassis id 0.
qemu-system-x86_64: -device pci-bridge,id=p1: Device
initialization failed.
Segmentation fault (core dumped)
if (child-class-unparent) {
#0 0x558d629b in object_finalize_child_property
(obj=0x56d2e830, name=0x56d30630 shpc-mmio[0], opaque=0x56a42fc8)
at qom/object.c:1078
#1 0x558d4b1f in object_property_del_all (obj=0x56d2e830) at
qom/object.c:367
#2 0x558d4ca1 in object_finalize (data=0x56d2e830) at
qom/object.c:412
#3 0x558d55a1 in object_unref (obj=0x56d2e830) at
qom/object.c:720
#4 0x5572c907 in qdev_device_add (opts=0x563544f0) at
qdev-monitor.c:566
#5 0x55744f16 in device_init_func (opts=0x563544f0,
opaque=0x0) at vl.c:2213
#6 0x559cf5f0 in qemu_opts_foreach (list=0x55e0f8e0
qemu_device_opts, func=0x55744efa device_init_func, opaque=0x0,
abort_on_failure=1) at util/qemu-option.c:1057
#7 0x5574a11b in main (argc=16, argv=0x7fffdde8,
envp=0x7fffde70) at vl.c:423
Unparent the shpc mmio region as part of shpc cleanup.
Signed-off-by: Marcel Apfelbaum marce...@redhat.com
---
hw/pci/shpc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 65b2f51..2e887d7 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -662,6 +662,7 @@ void shpc_cleanup(PCIDevice *d, MemoryRegion *bar)
SHPCDevice *shpc = d-shpc;
d-cap_present = ~QEMU_PCI_CAP_SHPC;
memory_region_del_subregion(bar, shpc-mmio);
+object_unparent(OBJECT(shpc-mmio));
/* TODO: cleanup config space changes? */
g_free(shpc-config);
g_free(shpc-cmask);
--
1.8.3.1
[Qemu-devel] [PATCHv2] virtio-serial: avoid crash when port has no name
It seems name is not mandatory, and the following command line (based
on one generated by current libvirt) will crash qemu at start:
qemu-system-x86_64 \
-device virtio-serial-pci \
-device virtserialport,name=foo \
-device virtconsole
Program received signal SIGSEGV, Segmentation fault.
__strcmp_ssse3 () at ../sysdeps/x86_64/strcmp.S:210
210movlpd(%rsi), %xmm2
Missing separate debuginfos, use: debuginfo-install
python-libs-2.7.5-13.fc20.x86_64
(gdb) bt
#0 __strcmp_ssse3 () at ../sysdeps/x86_64/strcmp.S:210
#1 0x5566bdc6 in find_port_by_name (name=0x0) at
/home/elmarco/src/qemu/hw/char/virtio-serial-bus.c:67
Signed-off-by: Marc-André Lureau marcandre.lur...@gmail.com
---
hw/char/virtio-serial-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3931085..f16452e 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -871,7 +871,7 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-if (find_port_by_name(port-name)) {
+if (port-name != NULL find_port_by_name(port-name)) {
error_setg(errp, virtio-serial-bus: A port already exists by name %s,
port-name);
return;
--
1.9.3
Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub
On Wed, 5 Nov 2014 14:17:36 +, Peter Maydell said:
On 5 November 2014 13:50, Martin Simmons mar...@lispworks.com wrote:
On Tue, 4 Nov 2014 19:09:43 +, Peter Maydell said:
The if() statement should have braces for our coding style,
and no space before the '(' in function calls; otherwise this
looks good to me.
Do you want a new patch with it like that? I was following the style of the
rest of that file, in particular the other 'C' case :-(
Yes, if you could respin it that would be nice. Unfortunately there
are still areas of our codebase which don't follow the coding
style; we update bits of code as we change them, so new patches
follow the style guide for the lines they touch. You can use
scripts/checkpatch.pl to check your patch for the most common
issues.
OK, here it is.
Signed-off-by: Martin Simmons mar...@lispworks.com
diff --git a/gdbstub.c b/gdbstub.c
index d1b5afd..0faca56 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -823,7 +823,10 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
action = *p++;
signal = 0;
if (action == 'C' || action == 'S') {
-signal = strtoul(p, (char **)p, 16);
+signal = gdb_signal_to_target(strtoul(p, (char **)p, 16));
+if (signal == -1) {
+signal = 0;
+}
} else if (action != 'c' action != 's') {
res = 0;
break;
__Martin
