date:20171207

On Mon, Nov 20, 2017 at 1:49 PM, Ladi Prosek  wrote:
> On Mon, Nov 20, 2017 at 10:07 AM, Ladi Prosek  wrote:
>> On Sun, Nov 19, 2017 at 9:39 PM,   wrote:
>>> I just updated to the latest build and applied this patch set, now on VM
>>> reset the qemu crashes with the following assert:
>>>
>>> ivshmem.c:467: ivshmem_add_kvm_msi_virq: Assertion
>>> `!s->msi_vectors[vector].pdev' failed.
>>
>> I see asserts too. Even with v1 on top of QEMU v2.10.0 so I must have
>> missed something.
>>
>> Looking. And, needless to say, these patches should not be applied just yet 
>> :)
>
> Ok, here goes it.
>
> 1)
> ivshmem.c:467: ivshmem_add_kvm_msi_virq: Assertion
> `!s->msi_vectors[vector].pdev' failed.
>
> Is caused by the ivshmem device not undoing the effects of
> ivshmem_enable_irqfd() on reset.
>
> This fix works for me:
>
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -758,10 +758,15 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>  }
>  }
>
> +
> +static void ivshmem_disable_irqfd(IVShmemState *s);
> +
>  static void ivshmem_reset(DeviceState *d)
>  {
>  IVShmemState *s = IVSHMEM_COMMON(d);
>
> +ivshmem_disable_irqfd(s);
> +
>  s->intrstatus = 0;
>  s->intrmask = 0;
>  if (ivshmem_has_feature(s, IVSHMEM_MSI)) {

I have added this to v3 as patch 4.

> 2)
> ivshmem.c:354: ivshmem_vector_mask: Assertion `v->unmasked' failed.
>
> which I've been also getting after I enabled Driver Verifier and
> Windows started crashing
> (https://github.com/virtio-win/kvm-guest-drivers-windows/pull/199), is
> caused by the MSI-X code masking already masked vectors on reset. I'm
> going to post a patch similar to this:
>
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
>  return;
>  }
>  msix_clear_all_vectors(dev);
> +msix_mask_all(dev, dev->msix_entries_nr);
>  dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
>  memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
>  memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
> -msix_mask_all(dev, dev->msix_entries_nr);
> +msix_update_function_masked(dev);
>  }
>
>
> Then either no further changes to this patchset are necessary. Or, if
> relying on unmasks/masks (or
> msix_vector_use_notifier/msix_vector_release_notifier as it's called
> in msix.c) always being balanced is not recommended, the assert will
> simply change into an if.

This is fixed in "msix: don't mask already masked vectors on reset":
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg01362.html

[Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

The effects of ivshmem_enable_irqfd() was not undone on device reset.

This manifested as:
ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.

when irqfd was enabled before reset and then enabled again after reset, making
ivshmem_enable_irqfd() run for the second time.

To reproduce, run:

  ivshmem-server

and QEMU with:

  -device ivshmem-doorbell,chardev=iv
  -chardev socket,path=/tmp/ivshmem_socket,id=iv

then install the Windows driver, at the time of writing available at:

https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

and crash-reboot the guest by inducing a BSOD.

Signed-off-by: Ladi Prosek 
---
 hw/misc/ivshmem.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index d1bb246d12..4be0d2627b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
 }
 }
 
-static void ivshmem_reset(DeviceState *d)
-{
-IVShmemState *s = IVSHMEM_COMMON(d);
-
-s->intrstatus = 0;
-s->intrmask = 0;
-if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
-ivshmem_msix_vector_use(s);
-}
-}
-
 static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
 {
 /* allocate QEMU callback data for receiving interrupts */
@@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
 
 }
 
+static void ivshmem_reset(DeviceState *d)
+{
+IVShmemState *s = IVSHMEM_COMMON(d);
+
+ivshmem_disable_irqfd(s);
+
+s->intrstatus = 0;
+s->intrmask = 0;
+if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
+ivshmem_msix_vector_use(s);
+}
+}
+
 static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
  uint32_t val, int len)
 {
-- 
2.13.6

[Qemu-devel] [PATCH v3 0/4] ivshmem: MSI bug fixes

Fixes bugs in the ivshmem device implementation uncovered with the new
Windows ivshmem driver:
https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

v1->v2:
* Patch 1 - added reproducer info to commit message (Markus)
* Patch 2 - restructured conditionals, fixed comment formatting (Markus)
* Patch 3 - added reproducer info to commit message (Markus)

v2->v3:
* Added patch 4

Ladi Prosek (4):
  ivshmem: Don't update non-existent MSI routes
  ivshmem: Always remove irqfd notifiers
  ivshmem: Improve MSI irqfd error handling
  ivshmem: Disable irqfd on device reset

 hw/misc/ivshmem.c | 101 ++
 1 file changed, 71 insertions(+), 30 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v3 2/4] ivshmem: Always remove irqfd notifiers

As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
QEMU crashes with:

ivshmem: msix_set_vector_notifiers failed
msix_unset_vector_notifiers: Assertion `dev->msix_vector_use_notifier && 
dev->msix_vector_release_notifier' failed.

if MSI-X is repeatedly enabled and disabled on the ivshmem device, for example
by loading and unloading the Windows ivshmem driver. This is because
msix_unset_vector_notifiers() doesn't call any of the release notifier callbacks
since MSI-X is already disabled at that point (msix_enabled() returning false
is how this transition is detected in the first place). Thus 
ivshmem_vector_mask()
doesn't run and when MSI-X is subsequently enabled again ivshmem_vector_unmask()
fails.

This is fixed by keeping track of unmasked vectors and making sure that
ivshmem_vector_mask() always runs on MSI-X disable.

Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
Signed-off-by: Ladi Prosek 
Reviewed-by: Markus Armbruster 
---
 hw/misc/ivshmem.c | 32 ++--
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 6e46669744..91364d8364 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -77,6 +77,7 @@ typedef struct Peer {
 typedef struct MSIVector {
 PCIDevice *pdev;
 int virq;
+bool unmasked;
 } MSIVector;
 
 typedef struct IVShmemState {
@@ -321,6 +322,7 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned 
vector,
 error_report("ivshmem: vector %d route does not exist", vector);
 return -EINVAL;
 }
+assert(!v->unmasked);
 
 ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
 if (ret < 0) {
@@ -328,7 +330,13 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned 
vector,
 }
 kvm_irqchip_commit_routes(kvm_state);
 
-return kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
+ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
+if (ret < 0) {
+return ret;
+}
+v->unmasked = true;
+
+return 0;
 }
 
 static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
@@ -343,11 +351,14 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned 
vector)
 error_report("ivshmem: vector %d route does not exist", vector);
 return;
 }
+assert(v->unmasked);
 
 ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
-if (ret != 0) {
+if (ret < 0) {
 error_report("remove_irqfd_notifier_gsi failed");
+return;
 }
+v->unmasked = false;
 }
 
 static void ivshmem_vector_poll(PCIDevice *dev,
@@ -817,11 +828,20 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
 PCIDevice *pdev = PCI_DEVICE(s);
 int i;
 
-for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
-ivshmem_remove_kvm_msi_virq(s, i);
-}
-
 msix_unset_vector_notifiers(pdev);
+
+for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
+/*
+ * MSI-X is already disabled here so msix_unset_vector_notifiers()
+ * didn't call our release notifier.  Do it now to keep our masks and
+ * unmasks balanced.
+ */
+if (s->msi_vectors[i].unmasked) {
+ivshmem_vector_mask(pdev, i);
+}
+ivshmem_remove_kvm_msi_virq(s, i);
+}
+
 }
 
 static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
-- 
2.13.6

[Qemu-devel] [PATCH v3 3/4] ivshmem: Improve MSI irqfd error handling

Adds a rollback path to ivshmem_enable_irqfd() and fixes
ivshmem_disable_irqfd() to bail if irqfd has not been enabled.

To reproduce, run:

  ivshmem-server -n 0

and QEMU with:

  -device ivshmem-doorbell,chardev=iv
  -chardev socket,path=/tmp/ivshmem_socket,id=iv

then load, unload, and load again the Windows driver, at the time of writing
available at:

https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

The issue is believed to have been masked by other guest drivers, notably
Linux ones, not enabling MSI-X on the device.

Signed-off-by: Ladi Prosek 
Reviewed-by: Markus Armbruster 
---
 hw/misc/ivshmem.c | 37 -
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 91364d8364..d1bb246d12 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -786,6 +786,20 @@ static int ivshmem_setup_interrupts(IVShmemState *s, Error 
**errp)
 return 0;
 }
 
+static void ivshmem_remove_kvm_msi_virq(IVShmemState *s, int vector)
+{
+IVSHMEM_DPRINTF("ivshmem_remove_kvm_msi_virq vector:%d\n", vector);
+
+if (s->msi_vectors[vector].pdev == NULL) {
+return;
+}
+
+/* it was cleaned when masked in the frontend. */
+kvm_irqchip_release_virq(kvm_state, s->msi_vectors[vector].virq);
+
+s->msi_vectors[vector].pdev = NULL;
+}
+
 static void ivshmem_enable_irqfd(IVShmemState *s)
 {
 PCIDevice *pdev = PCI_DEVICE(s);
@@ -797,7 +811,7 @@ static void ivshmem_enable_irqfd(IVShmemState *s)
 ivshmem_add_kvm_msi_virq(s, i, );
 if (err) {
 error_report_err(err);
-/* TODO do we need to handle the error? */
+goto undo;
 }
 }
 
@@ -806,21 +820,14 @@ static void ivshmem_enable_irqfd(IVShmemState *s)
   ivshmem_vector_mask,
   ivshmem_vector_poll)) {
 error_report("ivshmem: msix_set_vector_notifiers failed");
+goto undo;
 }
-}
+return;
 
-static void ivshmem_remove_kvm_msi_virq(IVShmemState *s, int vector)
-{
-IVSHMEM_DPRINTF("ivshmem_remove_kvm_msi_virq vector:%d\n", vector);
-
-if (s->msi_vectors[vector].pdev == NULL) {
-return;
+undo:
+while (--i >= 0) {
+ivshmem_remove_kvm_msi_virq(s, i);
 }
-
-/* it was cleaned when masked in the frontend. */
-kvm_irqchip_release_virq(kvm_state, s->msi_vectors[vector].virq);
-
-s->msi_vectors[vector].pdev = NULL;
 }
 
 static void ivshmem_disable_irqfd(IVShmemState *s)
@@ -828,6 +835,10 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
 PCIDevice *pdev = PCI_DEVICE(s);
 int i;
 
+if (!pdev->msix_vector_use_notifier) {
+return;
+}
+
 msix_unset_vector_notifiers(pdev);
 
 for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
-- 
2.13.6

[Qemu-devel] [PATCH v3 1/4] ivshmem: Don't update non-existent MSI routes

As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
QEMU crashes with:

  kvm_irqchip_commit_routes: Assertion `ret == 0' failed.

if the ivshmem device is configured with more vectors than what the server
supports. This is caused by the ivshmem_vector_unmask() being called on
vectors that have not been initialized by ivshmem_add_kvm_msi_virq().

This commit fixes it by adding a simple check to the mask and unmask
callbacks.

Note that the opposite mismatch, if the server supplies more vectors than
what the device is configured for, is already handled and leads to output
like:

  Too many eventfd received, device has 1 vectors

To reproduce the assert, run:

  ivshmem-server -n 0

and QEMU with:

  -device ivshmem-doorbell,chardev=iv
  -chardev socket,path=/tmp/ivshmem_socket,id=iv

then load the Windows driver, at the time of writing available at:

https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

The issue is believed to have been masked by other guest drivers, notably
Linux ones, not enabling MSI-X on the device.

Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
Signed-off-by: Ladi Prosek 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Markus Armbruster 
---
 hw/misc/ivshmem.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a5a46827fe..6e46669744 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned 
vector,
 int ret;
 
 IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector);
+if (!v->pdev) {
+error_report("ivshmem: vector %d route does not exist", vector);
+return -EINVAL;
+}
 
 ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
 if (ret < 0) {
@@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned 
vector)
 {
 IVShmemState *s = IVSHMEM_COMMON(dev);
 EventNotifier *n = >peers[s->vm_id].eventfds[vector];
+MSIVector *v = >msi_vectors[vector];
 int ret;
 
 IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector);
+if (!v->pdev) {
+error_report("ivshmem: vector %d route does not exist", vector);
+return;
+}
 
-ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n,
-s->msi_vectors[vector].virq);
+ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
 if (ret != 0) {
 error_report("remove_irqfd_notifier_gsi failed");
 }
-- 
2.13.6

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img: Document --force-share / -U

2017-12-07 Thread Markus Armbruster

Fam Zheng  writes:

> On Thu, 12/07 10:53, Eric Blake wrote:
>> On 12/07/2017 04:58 AM, Kashyap Chamarthy wrote:
>> > On Thu, Dec 07, 2017 at 04:44:53PM +0800, Fam Zheng wrote:
[...]
>> >> +it less likely to conflict with a running guest's permissions due to 
>> >> image
>> >> +locking. For example, this can be used to get the image information (with
>> >> +'info' subcommand) when the image is used by a running guest. Note that 
>> >> this
>> >> +could produce inconsistent result because of concurrent metadata 
>> >> changes, etc..
>> > 
>> > Super nit-pick:  an ellipsis[*] is three dots :-), so, when applying you
>> > might want to: s/../.../
>> > 
>> > [*] https://dictionary.cambridge.org/dictionary/english/ellipsis
>> 
>> Except that both "etc." and "..." independently convey a sense of
>> continuation, which means that using both at once is both redundant
>> (just one will do) and difficult to argue how to typeset (since 'etc.'
>> is often written with an explicit '.' to emphasize that is an
>> abbreviation, does that mean you have to write 'etc.''...' for a total
>> of 4 dots?).
>
> I have the impression that "etc." is more correct than "etc"

It is.

>  so I used even at
> the end of the sensence where there is another period '.', making it "etc..".

That's wrong all the same :)

https://en.wikipedia.org/wiki/Period_(punctuation)#Abbreviations

> If ending the paragraph with "etc." is enough, we can drop one ".".

Please do.

Re: [Qemu-devel] iSER transport name is not good

2017-12-07 Thread Charles Kelimod

Hi Dave and Sahlberg,

for this type of name:iser:, I completely agree with
you.
my issue is when I create an vm from libvirt, the qemu command line should
be:
-drive
file.driver=iser,file.portal=xx.xx.xx.xx:3260,file.target=iqn.xxx,file.lun=0,file.transport=iser

Actually "iser" is added by me in libvirt, which can work by a little
modification. But I think file.transport=rdma would be better, as I think
iser is protocol, and rdma is transport, and libvirt defined it, if I added
a new transport name (iser) in libvirt, it will be confusable.

Best Regards,
Charles.

On Fri, Dec 8, 2017 at 2:07 PM, ronnie sahlberg 
wrote:

> David,
>
> Yes, QEMU has supported iSER (iSCSI extensions for RDMA) via a
> userspace library, libiscsi, since about a year.
> Here is a nice presentation from the developer of iSER support :
> https://www.google.com.au/url?sa=t=j==s=
> web=1=rja=8=0ahUKEwjChpba3PnXAhXBKZQKHQa9D
> EYQFggpMAA=https%3A%2F%2Fwww.snia.org%2Fsites%2Fdefault%2Ffiles%2FSDC%
> 2F2016%2Fpresentations%2Fstorage_networking%2FShterman-Grimberg_Greenberg_
> Performance%2520Implications%2520Libiscsi_%2520RDMA_V6.pdf=
> AOvVaw3xyvdIciRKVprboN6rClTA
>
> In QEMU, you use it the same way as you would use the userspace iSCSI
> support which has been in QEMU for quite a long time.
> The only difference is that instead of providing a
> iscsi: you select iSER by specifying an url of the
> form
> iser:
>
>
> I.e. the only difference is the protocol part of the URL. When QEMU
> passes this URL to libiscsi, it allows the library to decide on
> whether to use normal iSCSI or whether it should use iSER.
>
> I think RDMA would be a less good name for this as RDMA is not only
> used to transport iSCSI but is also used for NFS as well as SMB.
>
>
>
> regards
> ronnie sahlberg
>
>
> On Fri, Dec 8, 2017 at 4:41 AM, Dr. David Alan Gilbert
>  wrote:
> > * Charles Kelimod (lichs...@gmail.com) wrote:
> >> Hello,
> >>
> >>
> >> I'm on the road of modifying libvirt to add iSER support, and I found
> qemu
> >> is actually support that. But, the transport name is iSER, which should
> be
> >> RDMA and there libvirt already defined it.
> >>
> >> Best Regards,
> >> Charles.
> >
> > Hi Charles,
> >   So this is iSCSI extensions for RDMA?  I've cc'd in the qemu iSCSI
> > maintainers.
> >   Can you just explain what you're seeing libvirt do and what you think
> > qemu is expecting it to do for iSER?
> >
> > Dave
> >
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>

Re: [Qemu-devel] [PATCH 2/2] intel_iommu: fix error param in string

2017-12-07 Thread Liu, Yi L

Looks good.

Reviewed-by: Liu, Yi L 

On Fri, Dec 08, 2017 at 12:26:54PM +0800, Peter Xu wrote:
> It should be caching-mode.  It may confuse people when it pops up.
> 
> Signed-off-by: Peter Xu 
> ---
>  hw/i386/intel_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c1fa08d205..fe15d3ba84 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2327,7 +2327,7 @@ static void 
> vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
>  IntelIOMMUNotifierNode *next_node = NULL;
>  
>  if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
> -error_report("We need to set cache_mode=1 for intel-iommu to enable "
> +error_report("We need to set caching-mode=1 for intel-iommu to 
> enable "
>   "device assignment with IOMMU protection.");
>  exit(1);
>  }
> -- 
> 2.14.3
> 
>

Re: [Qemu-devel] [PATCH 1/2] intel_iommu: remove X86_IOMMU_PCI_DEVFN_MAX

2017-12-07 Thread Liu, Yi L

On Fri, Dec 08, 2017 at 12:26:53PM +0800, Peter Xu wrote:

Looks good to me.

Reviewed-by: Liu, Yi L 

> We have PCI_DEVFN_MAX now.
> 
> Signed-off-by: Peter Xu 
> ---
>  hw/i386/intel_iommu.c   | 10 +-
>  include/hw/i386/x86-iommu.h |  1 -
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 3a5bb0bc2e..c1fa08d205 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -186,7 +186,7 @@ static void vtd_reset_context_cache(IntelIOMMUState *s)
>  g_hash_table_iter_init(_it, s->vtd_as_by_busptr);
>  
>  while (g_hash_table_iter_next (_it, NULL, (void**)_bus)) {
> -for (devfn_it = 0; devfn_it < X86_IOMMU_PCI_DEVFN_MAX; ++devfn_it) {
> +for (devfn_it = 0; devfn_it < PCI_DEVFN_MAX; ++devfn_it) {
>  vtd_as = vtd_bus->dev_as[devfn_it];
>  if (!vtd_as) {
>  continue;
> @@ -1002,7 +1002,7 @@ static void 
> vtd_switch_address_space_all(IntelIOMMUState *s)
>  
>  g_hash_table_iter_init(, s->vtd_as_by_busptr);
>  while (g_hash_table_iter_next(, NULL, (void **)_bus)) {
> -for (i = 0; i < X86_IOMMU_PCI_DEVFN_MAX; i++) {
> +for (i = 0; i < PCI_DEVFN_MAX; i++) {
>  if (!vtd_bus->dev_as[i]) {
>  continue;
>  }
> @@ -1294,7 +1294,7 @@ static void 
> vtd_context_device_invalidate(IntelIOMMUState *s,
>  vtd_bus = vtd_find_as_from_bus_num(s, bus_n);
>  if (vtd_bus) {
>  devfn = VTD_SID_TO_DEVFN(source_id);
> -for (devfn_it = 0; devfn_it < X86_IOMMU_PCI_DEVFN_MAX; ++devfn_it) {
> +for (devfn_it = 0; devfn_it < PCI_DEVFN_MAX; ++devfn_it) {
>  vtd_as = vtd_bus->dev_as[devfn_it];
>  if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
>  trace_vtd_inv_desc_cc_device(bus_n, VTD_PCI_SLOT(devfn_it),
> @@ -2699,7 +2699,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
> PCIBus *bus, int devfn)
>  *new_key = (uintptr_t)bus;
>  /* No corresponding free() */
>  vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \
> -X86_IOMMU_PCI_DEVFN_MAX);
> +PCI_DEVFN_MAX);
>  vtd_bus->bus = bus;
>  g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus);
>  }
> @@ -2982,7 +2982,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, 
> void *opaque, int devfn)
>  IntelIOMMUState *s = opaque;
>  VTDAddressSpace *vtd_as;
>  
> -assert(0 <= devfn && devfn < X86_IOMMU_PCI_DEVFN_MAX);
> +assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
>  
>  vtd_as = vtd_find_add_as(s, bus, devfn);
>  return _as->as;
> diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
> index ef89c0c646..7c71fc7470 100644
> --- a/include/hw/i386/x86-iommu.h
> +++ b/include/hw/i386/x86-iommu.h
> @@ -31,7 +31,6 @@
>  #define  X86_IOMMU_GET_CLASS(obj) \
>  OBJECT_GET_CLASS(X86IOMMUClass, obj, TYPE_X86_IOMMU_DEVICE)
>  
> -#define X86_IOMMU_PCI_DEVFN_MAX   256
>  #define X86_IOMMU_SID_INVALID (0x)
>  
>  typedef struct X86IOMMUState X86IOMMUState;
> -- 
> 2.14.3
> 
>

Re: [Qemu-devel] [PATCH v2] 9pfs: Correctly handle cancelled requests

2017-12-07 Thread Greg Kurz

Cc'ing Stefano using a more appropriate address :)

On Thu, 7 Dec 2017 18:04:24 +0100
Greg Kurz  wrote:

> On Mon, 4 Dec 2017 15:36:19 -0500
> Keno Fischer  wrote:
> 
> >  # Background
> > 
> > I was investigating spurious non-deterministic EINTR returns from
> > various 9p file system operations in a Linux guest served from the
> > qemu 9p server.
> > 
> >  ## EINTR, ERESTARTSYS and the linux kernel
> > 
> > When a signal arrives that the Linux kernel needs to deliver to user-space
> > while a given thread is blocked (in the 9p case waiting for a reply to its
> > request in 9p_client_rpc -> wait_event_interruptible), it asks whatever
> > driver is currently running to abort its current operation (in the 9p case
> > causing the submission of a TFLUSH message) and return to user space.
> > In these situations, the error message reported is generally ERESTARTSYS.
> > If the userspace processes specified SA_RESTART, this means that the
> > system call will get restarted upon completion of the signal handler
> > delivery (assuming the signal handler doesn't modify the process state
> > in complicated ways not relevant here). If SA_RESTART is not specified,
> > ERESTARTSYS gets translated to EINTR and user space is expected to handle
> > the restart itself.
> > 
> >  ## The 9p TFLISH command  
>^^^
> I'll fix this before pushing to my tree in case there's no v3.
> 
> > 
> > The 9p TFLUSH commands requests that the server abort an ongoing operation.
> > The man page [1] specifies:
> > 
> > ```
> > If it recognizes oldtag as the tag of a pending transaction, it should 
> > abort any
> > pending response and discard that tag.
> > [...]
> > When the client sends a Tflush, it must wait to receive the corresponding 
> > Rflush
> > before reusing oldtag for subsequent messages. If a response to the flushed 
> > request
> > is received before the Rflush, the client must honor the response as if it 
> > had not
> > been flushed, since the completed request may signify a state change in the 
> > server
> > ```
> > 
> > In particular, this means that the server must not send a reply with the 
> > orignal
> > tag in response to the cancellation request, because the client is obligated
> > to interpret such a reply as a coincidental reply to the original request.
> > 
> >  # The bug
> > 
> > When qemu receives a TFlush request, it sets the `cancelled` flag on the 
> > relevant
> > pdu. This flag is periodically checked, e.g. in `v9fs_co_name_to_path`, and 
> > if
> > set, the operation is aborted and the error is set to EINTR. However, the 
> > server
> > then violates the spec, by returning to the client an Rerror response, 
> > rather
> > than discarding the message entirely. As a result, the client is required
> > to assume that said Rerror response is a result of the original request, not
> > a result of the cancellation and thus passes the EINTR error back to user 
> > space.
> > This is not the worst thing it could do, however as discussed above, the 
> > correct
> > error code would have been ERESTARTSYS, such that user space programs with
> > SA_RESTART set get correctly restarted upon completion of the signal 
> > handler.
> > Instead, such programs get spurious EINTR results that they were not 
> > expecting
> > to handle.
> > 
> > It should be noted that there are plenty of user space programs that do not
> > set SA_RESTART and do not correctly handle EINTR either. However, that is 
> > then
> > a userspace bug. It should also be noted that this bug has been mitigated by
> > a recent commit to the Linux kernel [2], which essentially prevents the 
> > kernel
> > from sending Tflush requests unless the process is about to die (in which 
> > case
> > the process likely doesn't care about the response). Nevertheless, for older
> > kernels and to comply with the spec, I believe this change is beneficial.
> > 
> >  # Implementation
> > 
> > The fix is fairly simple, just skipping notification of a reply if
> > the pdu was previously cancelled. We do however, also notify the transport
> > layer that we're doing this, so it can clean up any resources it may be
> > holding. I also added a new trace event to distinguish
> > operations that caused an error reply from those that were cancelled.
> > 
> > One complication is that we only omit sending the message on EINTR errors in
> > order to avoid confusing the rest of the code (which may assume that a
> > client knows about a fid if it sucessfully passed it off to pud_complete
> > without checking for cancellation status). This does mean that if the server
> > acts upon the cancellation flag, it always needs to set err to EINTR. I 
> > believe
> > this is true of the current code.
> > 
> > [1] https://9fans.github.io/plan9port/man/man9/flush.html
> > [2] 
> > https://github.com/torvalds/linux/commit/9523feac272ccad2ad8186ba4fcc89103754de52
> > 
> > Signed-off-by: Keno Fischer 
> > ---
> >

Re: [Qemu-devel] [virtio-dev] [PATCH v3 0/7] Vhost-pci for inter-VM communication

2017-12-07 Thread Wei Wang

On 12/08/2017 07:54 AM, Michael S. Tsirkin wrote:

On Thu, Dec 07, 2017 at 06:28:19PM +, Stefan Hajnoczi wrote:

On Thu, Dec 7, 2017 at 5:38 PM, Michael S. Tsirkin wrote:

On Thu, Dec 07, 2017 at 05:29:14PM +, Stefan Hajnoczi wrote:

On Thu, Dec 7, 2017 at 4:47 PM, Michael S. Tsirkin wrote:

On Thu, Dec 07, 2017 at 04:29:45PM +, Stefan Hajnoczi wrote:

On Thu, Dec 7, 2017 at 2:02 PM, Michael S. Tsirkin wrote:

On Thu, Dec 07, 2017 at 01:08:04PM +, Stefan Hajnoczi wrote:

Instead of responding individually to these points, I hope this will
explain my perspective. Let me know if you do want individual
responses, I'm happy to talk more about the points above but I think
the biggest difference is our perspective on this:

Existing vhost-user slave code should be able to run on top of
vhost-pci. For example, QEMU's
contrib/vhost-user-scsi/vhost-user-scsi.c should work inside the guest
with only minimal changes to the source file (i.e. today it explicitly
opens a UNIX domain socket and that should be done by libvhost-user
instead). It shouldn't be hard to add vhost-pci vfio support to
contrib/libvhost-user/ alongside the existing UNIX domain socket code.

This seems pretty easy to achieve with the vhost-pci PCI adapter that
I've described but I'm not sure how to implement libvhost-user on top
of vhost-pci vfio if the device doesn't expose the vhost-user
protocol.

I think this is a really important goal. Let's use a single
vhost-user software stack instead of creating a separate one for guest
code only.

Do you agree that the vhost-user software stack should be shared
between host userspace and guest code as much as possible?

The sharing you propose is not necessarily practical because the security goals
of the two are different.

It seems that the best motivation presentation is still the original rfc

http://virtualization.linux-foundation.narkive.com/A7FkzAgp/rfc-vhost-user-enhancements-for-vm2vm-communication

So comparing with vhost-user iotlb handling is different:

With vhost-user guest trusts the vhost-user backend on the host.

With vhost-pci we can strive to limit the trust to qemu only.
The switch running within a VM does not have to be trusted.

Can you give a concrete example?

I have an idea about what you're saying but it may be wrong:

Today the iotlb mechanism in vhost-user does not actually enforce
memory permissions. The vhost-user slave has full access to mmapped
memory regions even when iotlb is enabled. Currently the iotlb just
adds an indirection layer but no real security. (Is this correct?)

Not exactly. iotlb protects against malicious drivers within guest.
But yes, not against a vhost-user driver on the host.

Are you saying the vhost-pci device code in QEMU should enforce iotlb
permissions so the vhost-user slave guest only has access to memory
regions that are allowed by the iotlb?

Yes.

Okay, thanks for confirming.

This can be supported by the approach I've described. The vhost-pci
QEMU code has control over the BAR memory so it can prevent the guest
from accessing regions that are not allowed by the iotlb.

Inside the guest the vhost-user slave still has the memory region
descriptions and sends iotlb messages. This is completely compatible
with the libvirt-user APIs and existing vhost-user slave code can run
fine. The only unique thing is that guest accesses to memory regions
not allowed by the iotlb do not work because QEMU has prevented it.

I don't think this can work since suddenly you need
to map full IOMMU address space into BAR.

The BAR covers all guest RAM
but QEMU can set up MemoryRegions that
hide parts from the guest (e.g. reads produce 0xff). I'm not sure how
expensive that is but implementing a strict IOMMU is hard to do
without performance overhead.

I'm worried about leaking PAs.
fundamentally if you want proper protection you
need your device driver to use VA for addressing,

On the one hand BAR only needs to be as large as guest PA then.
On the other hand it must cover all of guest PA,
not just what is accessible to the device.

Besides, this means implementing iotlb in both qemu and guest.

It's free in the guest, the libvhost-user stack already has it.

That library is designed to work with a unix domain socket
though. We'll need extra kernel code to make a device
pretend it's a socket.

If better performance is needed then it might be possible to optimize
this interface by handling most or even all of the iotlb stuff in QEMU
vhost-pci code and not exposing it to the vhost-user slave in the
guest. But it doesn't change the fact that the vhost-user protocol
can be used and the same software stack works.

For one, the iotlb part would be out of scope then.
Instead you would have code to offset from BAR.

Do you have a concrete example of why sharing the same vhost-user
software stack inside the guest can't work?

With enough dedication some code might be shared. OTOH

[Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset

msix_mask_all() is supposed to invoke the release vector notifier if the state 
of the
respective vector changed from unmasked to masked. The way it's currently 
called from
msix_reset(), though, may result in calling the release notifier even if the 
vector
is already masked.

1) msix_reset() clears out the msix_cap field and the msix_table.
2) msix_mask_all() runs with was_masked=false for all vectors because of 1), 
which
   results in calling the release notifier on all vectors.
3) if msix_reset() is subsequently called again, it goes through the same steps 
and
   calls the release notifier on all vectors again.

This commit moves msix_mask_all() up so it runs before the device state is 
lost. And
it adds an assignment to msix_function_masked so that the device remembers that
MSI-X is masked.

This is likely a low impact issue, found while debugging an already broken 
device. It
is however easy to fix and the expectation that the use and release notifier 
invocations
are always balanced is very natural.

Signed-off-by: Ladi Prosek 
---

v1->v2:
* fixed typo in commit message "or" -> "to" (Marcel)
* directly set msix_function_masked to true instead of calling
  msix_update_function_masked() (Marcel)


 hw/pci/msix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index c944c02135..d6a4dbdb6b 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
 return;
 }
 msix_clear_all_vectors(dev);
+msix_mask_all(dev, dev->msix_entries_nr);
 dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
 memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
 memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
-msix_mask_all(dev, dev->msix_entries_nr);
+dev->msix_function_masked = true;
 }
 
 /* PCI spec suggests that devices make it possible for software to configure
-- 
2.13.6

Re: [Qemu-devel] [virtio-dev] [PATCH v3 0/7] Vhost-pci for inter-VM communication

2017-12-07 Thread Stefan Hajnoczi

On Thu, Dec 7, 2017 at 11:54 PM, Michael S. Tsirkin  wrote:
> On Thu, Dec 07, 2017 at 06:28:19PM +, Stefan Hajnoczi wrote:
>> On Thu, Dec 7, 2017 at 5:38 PM, Michael S. Tsirkin  wrote:
>> > On Thu, Dec 07, 2017 at 05:29:14PM +, Stefan Hajnoczi wrote:
>> >> On Thu, Dec 7, 2017 at 4:47 PM, Michael S. Tsirkin  
>> >> wrote:
>> >> > On Thu, Dec 07, 2017 at 04:29:45PM +, Stefan Hajnoczi wrote:
>> >> >> On Thu, Dec 7, 2017 at 2:02 PM, Michael S. Tsirkin  
>> >> >> wrote:
>> >> >> > On Thu, Dec 07, 2017 at 01:08:04PM +, Stefan Hajnoczi wrote:
>> >> >> >> Instead of responding individually to these points, I hope this will
>> >> >> >> explain my perspective.  Let me know if you do want individual
>> >> >> >> responses, I'm happy to talk more about the points above but I think
>> >> >> >> the biggest difference is our perspective on this:
>> >> >> >>
>> >> >> >> Existing vhost-user slave code should be able to run on top of
>> >> >> >> vhost-pci.  For example, QEMU's
>> >> >> >> contrib/vhost-user-scsi/vhost-user-scsi.c should work inside the 
>> >> >> >> guest
>> >> >> >> with only minimal changes to the source file (i.e. today it 
>> >> >> >> explicitly
>> >> >> >> opens a UNIX domain socket and that should be done by libvhost-user
>> >> >> >> instead).  It shouldn't be hard to add vhost-pci vfio support to
>> >> >> >> contrib/libvhost-user/ alongside the existing UNIX domain socket 
>> >> >> >> code.
>> >> >> >>
>> >> >> >> This seems pretty easy to achieve with the vhost-pci PCI adapter 
>> >> >> >> that
>> >> >> >> I've described but I'm not sure how to implement libvhost-user on 
>> >> >> >> top
>> >> >> >> of vhost-pci vfio if the device doesn't expose the vhost-user
>> >> >> >> protocol.
>> >> >> >>
>> >> >> >> I think this is a really important goal.  Let's use a single
>> >> >> >> vhost-user software stack instead of creating a separate one for 
>> >> >> >> guest
>> >> >> >> code only.
>> >> >> >>
>> >> >> >> Do you agree that the vhost-user software stack should be shared
>> >> >> >> between host userspace and guest code as much as possible?
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > The sharing you propose is not necessarily practical because the 
>> >> >> > security goals
>> >> >> > of the two are different.
>> >> >> >
>> >> >> > It seems that the best motivation presentation is still the original 
>> >> >> > rfc
>> >> >> >
>> >> >> > http://virtualization.linux-foundation.narkive.com/A7FkzAgp/rfc-vhost-user-enhancements-for-vm2vm-communication
>> >> >> >
>> >> >> > So comparing with vhost-user iotlb handling is different:
>> >> >> >
>> >> >> > With vhost-user guest trusts the vhost-user backend on the host.
>> >> >> >
>> >> >> > With vhost-pci we can strive to limit the trust to qemu only.
>> >> >> > The switch running within a VM does not have to be trusted.
>> >> >>
>> >> >> Can you give a concrete example?
>> >> >>
>> >> >> I have an idea about what you're saying but it may be wrong:
>> >> >>
>> >> >> Today the iotlb mechanism in vhost-user does not actually enforce
>> >> >> memory permissions.  The vhost-user slave has full access to mmapped
>> >> >> memory regions even when iotlb is enabled.  Currently the iotlb just
>> >> >> adds an indirection layer but no real security.  (Is this correct?)
>> >> >
>> >> > Not exactly. iotlb protects against malicious drivers within guest.
>> >> > But yes, not against a vhost-user driver on the host.
>> >> >
>> >> >> Are you saying the vhost-pci device code in QEMU should enforce iotlb
>> >> >> permissions so the vhost-user slave guest only has access to memory
>> >> >> regions that are allowed by the iotlb?
>> >> >
>> >> > Yes.
>> >>
>> >> Okay, thanks for confirming.
>> >>
>> >> This can be supported by the approach I've described.  The vhost-pci
>> >> QEMU code has control over the BAR memory so it can prevent the guest
>> >> from accessing regions that are not allowed by the iotlb.
>> >>
>> >> Inside the guest the vhost-user slave still has the memory region
>> >> descriptions and sends iotlb messages.  This is completely compatible
>> >> with the libvirt-user APIs and existing vhost-user slave code can run
>> >> fine.  The only unique thing is that guest accesses to memory regions
>> >> not allowed by the iotlb do not work because QEMU has prevented it.
>> >
>> > I don't think this can work since suddenly you need
>> > to map full IOMMU address space into BAR.
>>
>> The BAR covers all guest RAM
>> but QEMU can set up MemoryRegions that
>> hide parts from the guest (e.g. reads produce 0xff).  I'm not sure how
>> expensive that is but implementing a strict IOMMU is hard to do
>> without performance overhead.
>
> I'm worried about leaking PAs.
> fundamentally if you want proper protection you
> need your device driver to use VA for addressing,
>
> On the one hand BAR only needs to be as large as guest PA then.
> On the other hand it must cover all of guest PA,
> not just

Re: [Qemu-devel] iSER transport name is not good

2017-12-07 Thread ronnie sahlberg

David,

Yes, QEMU has supported iSER (iSCSI extensions for RDMA) via a
userspace library, libiscsi, since about a year.
Here is a nice presentation from the developer of iSER support :
https://www.google.com.au/url?sa=t=j==s=web=1=rja=8=0ahUKEwjChpba3PnXAhXBKZQKHQa9DEYQFggpMAA=https%3A%2F%2Fwww.snia.org%2Fsites%2Fdefault%2Ffiles%2FSDC%2F2016%2Fpresentations%2Fstorage_networking%2FShterman-Grimberg_Greenberg_Performance%2520Implications%2520Libiscsi_%2520RDMA_V6.pdf=AOvVaw3xyvdIciRKVprboN6rClTA

In QEMU, you use it the same way as you would use the userspace iSCSI
support which has been in QEMU for quite a long time.
The only difference is that instead of providing a
iscsi: you select iSER by specifying an url of the
form
iser:

I.e. the only difference is the protocol part of the URL. When QEMU
passes this URL to libiscsi, it allows the library to decide on
whether to use normal iSCSI or whether it should use iSER.

I think RDMA would be a less good name for this as RDMA is not only
used to transport iSCSI but is also used for NFS as well as SMB.

regards
ronnie sahlberg

On Fri, Dec 8, 2017 at 4:41 AM, Dr. David Alan Gilbert
 wrote:
> * Charles Kelimod (lichs...@gmail.com) wrote:
>> Hello,
>>
>>
>> I'm on the road of modifying libvirt to add iSER support, and I found qemu
>> is actually support that. But, the transport name is iSER, which should be
>> RDMA and there libvirt already defined it.
>>
>> Best Regards,
>> Charles.
>
> Hi Charles,
>   So this is iSCSI extensions for RDMA?  I've cc'd in the qemu iSCSI
> maintainers.
>   Can you just explain what you're seeing libvirt do and what you think
> qemu is expecting it to do for iSER?
>
> Dave
>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] rcu: reduce more than 7MB heap memory by malloc_trim()

2017-12-07 Thread Yang Zhong

On Thu, Dec 07, 2017 at 04:33:10PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Nov 23, 2017 at 7:41 AM, Yang Zhong  wrote:
> > Since there are some issues in memory alloc/free machenism
> > in glibc for little chunk memory, if Qemu frequently
> > alloc/free little chunk memory, the glibc doesn't alloc
> > little chunk memory from free list of glibc and still
> > allocate from OS, which make the heap size bigger and bigger.
> >
> > This patch introduce malloc_trim(), which will free heap memory.
> >
> > Below are test results from smaps file.
> > (1)without patch
> > 55f0783e1000-55f07992a000 rw-p  00:00 0  [heap]
> > Size:  21796 kB
> > Rss:   14260 kB
> > Pss:   14260 kB
> >
> > (2)with patch
> > 55cc5fadf000-55cc61008000 rw-p  00:00 0  [heap]
> > Size:  21668 kB
> > Rss:6940 kB
> > Pss:6940 kB
> 
> Have you opened a bug to glibc malloc (or discussed that issue with
> the glibc malloc developpers) ?
> 
> Or is there a justification that qemu should have its own trim
> heuristic on top of malloc?

  Hello Marc-Andr,

  Thanks for your comments!
  I did not open a bug for glibc community, maybe this issue is not new
  issue for them because there are lots of complains about malloc/free 
  issues in the internet. If need, i can file this bug to them.

  Qemu(v2.3) introduce the rcu mechanism for VM performance, but the side 
  effect is heap memory never shrink like below case
  http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg02748.html 

  Glibc let free memory in their free list for next malloc, but the fact 
  is glibc still allocate memory from OS in Qemu. There are lots of memory 
  hole in the heap, escepially the batch malloc and free with rcu mechanism,
  which made heap memory bigger and bigger, and never shrink.

  from the test by mallinfo() and malloc_trim(), i found malloc_trim() not
  only trim heap top free memory, but also can free hole memory in the heap.

  This is V2 patch thread and we are talking in V3 patch thread now.
  http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg04519.html

  Regards,

  Yang

> Marc-André Lureau

[Qemu-devel] [PATCH 2/2] intel_iommu: fix error param in string

2017-12-07 Thread Peter Xu

It should be caching-mode.  It may confuse people when it pops up.

Signed-off-by: Peter Xu 
---
 hw/i386/intel_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c1fa08d205..fe15d3ba84 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2327,7 +2327,7 @@ static void 
vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
 IntelIOMMUNotifierNode *next_node = NULL;
 
 if (!s->caching_mode && new & IOMMU_NOTIFIER_MAP) {
-error_report("We need to set cache_mode=1 for intel-iommu to enable "
+error_report("We need to set caching-mode=1 for intel-iommu to enable "
  "device assignment with IOMMU protection.");
 exit(1);
 }
-- 
2.14.3

[Qemu-devel] [PATCH 0/2] vtd: tiny fixes for 2.12

2017-12-07 Thread Peter Xu

Please review.  Thanks,

Peter Xu (2):
  intel_iommu: remove X86_IOMMU_PCI_DEVFN_MAX
  intel_iommu: fix error param in string

 hw/i386/intel_iommu.c   | 12 ++--
 include/hw/i386/x86-iommu.h |  1 -
 2 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] intel_iommu: remove X86_IOMMU_PCI_DEVFN_MAX

2017-12-07 Thread Peter Xu

We have PCI_DEVFN_MAX now.

Signed-off-by: Peter Xu 
---
 hw/i386/intel_iommu.c   | 10 +-
 include/hw/i386/x86-iommu.h |  1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 3a5bb0bc2e..c1fa08d205 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -186,7 +186,7 @@ static void vtd_reset_context_cache(IntelIOMMUState *s)
 g_hash_table_iter_init(_it, s->vtd_as_by_busptr);
 
 while (g_hash_table_iter_next (_it, NULL, (void**)_bus)) {
-for (devfn_it = 0; devfn_it < X86_IOMMU_PCI_DEVFN_MAX; ++devfn_it) {
+for (devfn_it = 0; devfn_it < PCI_DEVFN_MAX; ++devfn_it) {
 vtd_as = vtd_bus->dev_as[devfn_it];
 if (!vtd_as) {
 continue;
@@ -1002,7 +1002,7 @@ static void vtd_switch_address_space_all(IntelIOMMUState 
*s)
 
 g_hash_table_iter_init(, s->vtd_as_by_busptr);
 while (g_hash_table_iter_next(, NULL, (void **)_bus)) {
-for (i = 0; i < X86_IOMMU_PCI_DEVFN_MAX; i++) {
+for (i = 0; i < PCI_DEVFN_MAX; i++) {
 if (!vtd_bus->dev_as[i]) {
 continue;
 }
@@ -1294,7 +1294,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState 
*s,
 vtd_bus = vtd_find_as_from_bus_num(s, bus_n);
 if (vtd_bus) {
 devfn = VTD_SID_TO_DEVFN(source_id);
-for (devfn_it = 0; devfn_it < X86_IOMMU_PCI_DEVFN_MAX; ++devfn_it) {
+for (devfn_it = 0; devfn_it < PCI_DEVFN_MAX; ++devfn_it) {
 vtd_as = vtd_bus->dev_as[devfn_it];
 if (vtd_as && ((devfn_it & mask) == (devfn & mask))) {
 trace_vtd_inv_desc_cc_device(bus_n, VTD_PCI_SLOT(devfn_it),
@@ -2699,7 +2699,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
PCIBus *bus, int devfn)
 *new_key = (uintptr_t)bus;
 /* No corresponding free() */
 vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \
-X86_IOMMU_PCI_DEVFN_MAX);
+PCI_DEVFN_MAX);
 vtd_bus->bus = bus;
 g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus);
 }
@@ -2982,7 +2982,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void 
*opaque, int devfn)
 IntelIOMMUState *s = opaque;
 VTDAddressSpace *vtd_as;
 
-assert(0 <= devfn && devfn < X86_IOMMU_PCI_DEVFN_MAX);
+assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
 
 vtd_as = vtd_find_add_as(s, bus, devfn);
 return _as->as;
diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index ef89c0c646..7c71fc7470 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -31,7 +31,6 @@
 #define  X86_IOMMU_GET_CLASS(obj) \
 OBJECT_GET_CLASS(X86IOMMUClass, obj, TYPE_X86_IOMMU_DEVICE)
 
-#define X86_IOMMU_PCI_DEVFN_MAX   256
 #define X86_IOMMU_SID_INVALID (0x)
 
 typedef struct X86IOMMUState X86IOMMUState;
-- 
2.14.3

[Qemu-devel] [Question] vhost-user: hot-unplug vhost-user nic for windows guest OS will fail with 100% reproduce rate

2017-12-07 Thread linzhecheng

Hi, guys

I met a problem when hot-unplug vhost-user nic for Windows 2008 rc2 sp1 64 
(Guest OS)

The xml of nic is as followed:

  
  
  
  
  
  


Firstly, I use virsh attach-device win2008 vif.xml to hot-plug a nic for Guest 
OS. This operation returns success.
After guest OS discover nic successfully, I use virsh detach-device win2008 
vif.xml to hot-unplug it. This operation will fail with 100% reproduce rate.

However, if I hot-plug and hot-unplug virtio-net nic , it will not fail.

I have analysis the process of qmp_device_del , I found that qemu have inject 
interrupt to acpi to let it notice guest OS to remove nic.
I guess there is something wrong in Windows when handle the interrupt.

[Qemu-devel] [BUG] vhost-user: hot-unplug vhost-user nic for windows guest OS will fail with 100% reproduce rate

2017-12-07 Thread linzhecheng

Hi, guys

I met a problem when hot-unplug vhost-user nic for Windows 2008 rc2 sp1 64 
(Guest OS)

The xml of nic is as followed:

  
  
  
  
  
  


Firstly, I use virsh attach-device win2008 vif.xml to hot-plug a nic for Guest 
OS. This operation returns success.
After guest OS discover nic successfully, I use virsh detach-device win2008 
vif.xml to hot-unplug it. This operation will fail with 100% reproduce rate.

However, if I hot-plug and hot-unplug virtio-net nic , it will not fail.

I have analysis the process of qmp_device_del , I found that qemu have inject 
interrupt to acpi to let it notice guest OS to remove nic.
I guess there is something wrong in Windows when handle the interrupt.

[Qemu-devel] [PATCH v2] qemu-img: Document --force-share / -U

2017-12-07 Thread Fam Zheng

Signed-off-by: Fam Zheng 

---
v2: - "code{qemu-img}". [Kashyap, Eric]
- "etc.." -> "etc.".
---
 qemu-img.texi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/qemu-img.texi b/qemu-img.texi
index fdcf120f36..6b9f9adfc8 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -57,6 +57,15 @@ exclusive with the @var{-O} parameters. It is currently 
required to also use
 the @var{-n} parameter to skip image creation. This restriction may be relaxed
 in a future release.
 
+@item --force-share (-U)
+
+If specified, @code{qemu-img} will open the image with shared permissions,
+which makes it less likely to conflict with a running guest's permissions due
+to image locking. For example, this can be used to get the image information
+(with 'info' subcommand) when the image is used by a running guest. Note that
+this could produce inconsistent result because of concurrent metadata changes,
+etc.
+
 @item fmt
 is the disk image format. It is guessed automatically in most cases. See below
 for a description of the supported disk formats.
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img: Document --force-share / -U

2017-12-07 Thread Fam Zheng

On Thu, 12/07 10:53, Eric Blake wrote:
> On 12/07/2017 04:58 AM, Kashyap Chamarthy wrote:
> > On Thu, Dec 07, 2017 at 04:44:53PM +0800, Fam Zheng wrote:
> >> Signed-off-by: Fam Zheng 
> >> ---
> >>  qemu-img.texi | 8 
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/qemu-img.texi b/qemu-img.texi
> >> index fdcf120f36..3aa63aad55 100644
> >> --- a/qemu-img.texi
> >> +++ b/qemu-img.texi
> >> @@ -57,6 +57,14 @@ exclusive with the @var{-O} parameters. It is currently 
> >> required to also use
> >>  the @var{-n} parameter to skip image creation. This restriction may be 
> >> relaxed
> >>  in a future release.
> >>  
> >> +@item --force-share (-U)
> >> +
> >> +If specified, qemu-img will open the image with shared permissions, which 
> >> makes
> > 
> > Does 'texi' requires to quote tools like `qemu-img` (or single quotes),
> > to highlight them in documentation?
> 
> Our usual markup appears to be:
> 
> @code{qemu-img}

Sounds good.

> 
> > 
> >> +it less likely to conflict with a running guest's permissions due to image
> >> +locking. For example, this can be used to get the image information (with
> >> +'info' subcommand) when the image is used by a running guest. Note that 
> >> this
> >> +could produce inconsistent result because of concurrent metadata changes, 
> >> etc..
> > 
> > Super nit-pick:  an ellipsis[*] is three dots :-), so, when applying you
> > might want to: s/../.../
> > 
> > [*] https://dictionary.cambridge.org/dictionary/english/ellipsis
> 
> Except that both "etc." and "..." independently convey a sense of
> continuation, which means that using both at once is both redundant
> (just one will do) and difficult to argue how to typeset (since 'etc.'
> is often written with an explicit '.' to emphasize that is an
> abbreviation, does that mean you have to write 'etc.''...' for a total
> of 4 dots?).

I have the impression that "etc." is more correct than "etc" so I used even at
the end of the sensence where there is another period '.', making it "etc..".

If ending the paragraph with "etc." is enough, we can drop one ".".

Fam

[Qemu-devel] [PATCH 12/12] WIP ucontext: annotate coroutine stack for ASAN

Not strictly necessary, but it may help ASAN and remove some false
positives.

Sadly, this annotation produces an ASAN error:

$ tests/test-coroutine -p /basic/lifecycle
/basic/lifecycle: ==27655==WARNING: ASan doesn't fully support 
makecontext/swapcontext functions and may produce false positives in some cases!
==27655==AddressSanitizer CHECK failed: 
/builddir/build/BUILD/compiler-rt-4.0.1.src/lib/asan/asan_poisoning.cc:38 
"((AddrIsAlignedByGranularity(addr + size))) != (0)" (0x0, 0x0)
#0 0x55d1e4e8a695 in __asan::AsanCheckFailed(char const*, int, char const*, 
unsigned long long, unsigned long long) 
(/home/elmarco/src/qq/build/tests/test-coroutine+0x1c0695)
#1 0x55d1e4ea6235 in __sanitizer::CheckFailed(char const*, int, char 
const*, unsigned long long, unsigned long long) 
(/home/elmarco/src/qq/build/tests/test-coroutine+0x1dc235)
#2 0x55d1e4e82ea4 in __asan::PoisonShadow(unsigned long, unsigned long, 
unsigned char) (/home/elmarco/src/qq/build/tests/test-coroutine+0x1b8ea4)
#3 0x55d1e4dde1e7 in __asan::FakeStack::Destroy(int) 
(/home/elmarco/src/qq/build/tests/test-coroutine+0x1141e7)
#4 0x55d1e528b775 in qemu_coroutine_switch 
/home/elmarco/src/qq/util/coroutine-ucontext.c:219:9
#5 0x55d1e528b18d in coroutine_trampoline 
/home/elmarco/src/qq/util/coroutine-ucontext.c:114:9
#6 0x7fb3e0087bef  (/lib64/libc.so.6+0x50bef)

Signed-off-by: Marc-André Lureau 
---
 util/coroutine-ucontext.c | 47 +++
 1 file changed, 47 insertions(+)

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 6621f3f692..d200498c38 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -31,6 +31,25 @@
 #include 
 #endif
 
+#if (defined(__has_feature) && __has_feature(address_sanitizer)) || \
+__SANITIZE_ADDRESS__
+#include 
+#else
+/* stub to check correct arguments */
+static inline void
+__sanitizer_start_switch_fiber(void **fake_stack_save,
+   const void *bottom, size_t size)
+{
+}
+
+static inline void
+__sanitizer_finish_switch_fiber(void *fake_stack_save,
+const void **bottom_old,
+size_t *size_old)
+{
+}
+#endif
+
 typedef struct {
 Coroutine base;
 void *stack;
@@ -64,6 +83,15 @@ static void coroutine_trampoline(int i0, int i1)
 union cc_arg arg;
 CoroutineUContext *self;
 Coroutine *co;
+const void *bottom_old;
+size_t size_old;
+void *fake_stack_save;
+
+__sanitizer_finish_switch_fiber(NULL, _old, _old);
+if (!leader.stack) {
+leader.stack = (void *)bottom_old;
+leader.stack_size = size_old;
+}
 
 arg.i[0] = i0;
 arg.i[1] = i1;
@@ -72,9 +100,14 @@ static void coroutine_trampoline(int i0, int i1)
 
 /* Initialize longjmp environment and switch back the caller */
 if (!sigsetjmp(self->env, 0)) {
+__sanitizer_start_switch_fiber(_stack_save,
+   bottom_old, size_old);
 siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
 }
 
+__sanitizer_finish_switch_fiber(_stack_save,
+NULL, NULL);
+
 while (true) {
 co->entry(co->entry_arg);
 qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
@@ -87,6 +120,7 @@ Coroutine *qemu_coroutine_new(void)
 ucontext_t old_uc, uc;
 sigjmp_buf old_env;
 union cc_arg arg = {0};
+void *fake_stack_save;
 
 /* The ucontext functions preserve signal masks which incurs a
  * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
@@ -122,8 +156,13 @@ Coroutine *qemu_coroutine_new(void)
 
 /* swapcontext() in, siglongjmp() back out */
 if (!sigsetjmp(old_env, 0)) {
+__sanitizer_start_switch_fiber(_stack_save,
+   co->stack, co->stack_size);
 swapcontext(_uc, );
 }
+
+__sanitizer_finish_switch_fiber(_stack_save, NULL, NULL);
+
 return >base;
 }
 
@@ -169,13 +208,21 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
 CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
 CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
 int ret;
+void *fake_stack_save;
 
 current = to_;
 
 ret = sigsetjmp(from->env, 0);
 if (ret == 0) {
+
+__sanitizer_start_switch_fiber(action == COROUTINE_TERMINATE ?
+   NULL : _stack_save,
+   to->stack, to->stack_size);
 siglongjmp(to->env, action);
 }
+
+__sanitizer_finish_switch_fiber(_stack_save, NULL, NULL);
+
 return ret;
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 10/12] qemu-config: fix leak in query-command-line-options

Direct leak of 160 byte(s) in 4 object(s) allocated from:
#0 0x55ed7678cda8 in calloc 
(/home/elmarco/src/qq/build/x86_64-softmmu/qemu-system-x86_64+0x797da8)
#1 0x7f3f5e725f75 in g_malloc0 
/home/elmarco/src/gnome/glib/builddir/../glib/gmem.c:124
#2 0x55ed778aa3a7 in query_option_descs 
/home/elmarco/src/qq/util/qemu-config.c:60:16
#3 0x55ed778aa307 in get_drive_infolist 
/home/elmarco/src/qq/util/qemu-config.c:140:19
#4 0x55ed778a9f40 in qmp_query_command_line_options 
/home/elmarco/src/qq/util/qemu-config.c:254:36
#5 0x55ed76d4868c in qmp_marshal_query_command_line_options 
/home/elmarco/src/qq/build/qmp-marshal.c:3078:14
#6 0x55ed77855dd5 in do_qmp_dispatch 
/home/elmarco/src/qq/qapi/qmp-dispatch.c:104:5
#7 0x55ed778558cc in qmp_dispatch 
/home/elmarco/src/qq/qapi/qmp-dispatch.c:131:11
#8 0x55ed768b592f in handle_qmp_command 
/home/elmarco/src/qq/monitor.c:3840:11
#9 0x55ed7786ccfe in json_message_process_token 
/home/elmarco/src/qq/qobject/json-streamer.c:105:5
#10 0x55ed778fe37c in json_lexer_feed_char 
/home/elmarco/src/qq/qobject/json-lexer.c:323:13
#11 0x55ed778fdde6 in json_lexer_feed 
/home/elmarco/src/qq/qobject/json-lexer.c:373:15
#12 0x55ed7786cd83 in json_message_parser_feed 
/home/elmarco/src/qq/qobject/json-streamer.c:124:12
#13 0x55ed768b559e in monitor_qmp_read /home/elmarco/src/qq/monitor.c:3882:5
#14 0x55ed77714f29 in qemu_chr_be_write_impl 
/home/elmarco/src/qq/chardev/char.c:167:9
#15 0x55ed77714fde in qemu_chr_be_write 
/home/elmarco/src/qq/chardev/char.c:179:9
#16 0x55ed7772ffad in tcp_chr_read 
/home/elmarco/src/qq/chardev/char-socket.c:440:13
#17 0x55ed113b in qio_channel_fd_source_dispatch 
/home/elmarco/src/qq/io/channel-watch.c:84:12
#18 0x7f3f5e71d90b in g_main_dispatch 
/home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3182
#19 0x7f3f5e71e7ac in g_main_context_dispatch 
/home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3847
#20 0x55ed77886ffc in glib_pollfds_poll 
/home/elmarco/src/qq/util/main-loop.c:214:9
#21 0x55ed778865fd in os_host_main_loop_wait 
/home/elmarco/src/qq/util/main-loop.c:261:5
#22 0x55ed77886222 in main_loop_wait 
/home/elmarco/src/qq/util/main-loop.c:515:11
#23 0x55ed76d2a4df in main_loop /home/elmarco/src/qq/vl.c:1995:9
#24 0x55ed76d1cb4a in main /home/elmarco/src/qq/vl.c:4914:5
#25 0x7f3f555f6039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau 
---
 util/qemu-config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 99b0e46fa3..029fec53a9 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -105,7 +105,8 @@ static void cleanup_infolist(CommandLineParameterInfoList 
*head)
 if (!strcmp(pre_entry->value->name, cur->next->value->name)) {
 del_entry = cur->next;
 cur->next = cur->next->next;
-g_free(del_entry);
+del_entry->next = NULL;
+qapi_free_CommandLineParameterInfoList(del_entry);
 break;
 }
 pre_entry = pre_entry->next;
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 09/12] crypto: fix stack-buffer-overflow error

ASAN complains about:

==8856==ERROR: AddressSanitizer: stack-buffer-overflow on address 
0x7ffd8a1fe168 at pc 0x561136cb4451 bp 0x7ffd8a1fe130 sp 0x7ffd8a1fd8e0
READ of size 16 at 0x7ffd8a1fe168 thread T0
#0 0x561136cb4450 in __asan_memcpy 
(/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450)
#1 0x561136d2a6a7 in qcrypto_ivgen_essiv_calculate 
/home/elmarco/src/qq/crypto/ivgen-essiv.c:83:5
#2 0x561136d29af8 in qcrypto_ivgen_calculate 
/home/elmarco/src/qq/crypto/ivgen.c:72:12
#3 0x561136d07c8e in test_ivgen 
/home/elmarco/src/qq/tests/test-crypto-ivgen.c:148:5
#4 0x7f2c3b04 in test_case_run 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2237
#5 0x7f2c3ec4 in g_test_run_suite_internal 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2321
#6 0x7f2c3f6d in g_test_run_suite_internal 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
#7 0x7f2c3f6d in g_test_run_suite_internal 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
#8 0x7f2c3f6d in g_test_run_suite_internal 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
#9 0x7f2c4184 in g_test_run_suite 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2408
#10 0x7f2c2e0d in g_test_run 
/home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:1674
#11 0x561136d0799b in main 
/home/elmarco/src/qq/tests/test-crypto-ivgen.c:173:12
#12 0x7f77756e6039 in __libc_start_main (/lib64/libc.so.6+0x21039)
#13 0x561136c13d89 in _start 
(/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x6fd89)

Address 0x7ffd8a1fe168 is located in stack of thread T0 at offset 40 in frame
#0 0x561136d2a40f in qcrypto_ivgen_essiv_calculate 
/home/elmarco/src/qq/crypto/ivgen-essiv.c:76

  This frame has 1 object(s):
[32, 40) 'sector.addr' <== Memory access at offset 40 overflows this 
variable
HINT: this may be a false positive if your program uses some custom stack 
unwind mechanism or swapcontext
  (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow 
(/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450) in __asan_memcpy
Shadow bytes around the buggy address:
  0x100031437bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100031437c20: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00[f3]f3 f3
  0x100031437c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:   00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:   fa
  Freed heap region:   fd
  Stack left redzone:  f1
  Stack mid redzone:   f2
  Stack right redzone: f3
  Stack after return:  f5
  Stack use after scope:   f8
  Global redzone:  f9
  Global init order:   f6
  Poisoned by user:f7
  Container overflow:  fc
  Array cookie:ac
  Intra object redzone:bb
  ASan internal:   fe
  Left alloca redzone: ca
  Right alloca redzone:cb

It looks like the rest of the code copes with ndata being larger than
sizeof(sector), so limit the memcpy() range.

Signed-off-by: Marc-André Lureau 
---
 crypto/ivgen-essiv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/ivgen-essiv.c b/crypto/ivgen-essiv.c
index cba20bde6c..ad4d926c19 100644
--- a/crypto/ivgen-essiv.c
+++ b/crypto/ivgen-essiv.c
@@ -79,7 +79,7 @@ static int qcrypto_ivgen_essiv_calculate(QCryptoIVGen *ivgen,
 uint8_t *data = g_new(uint8_t, ndata);
 
 sector = cpu_to_le64(sector);
-memcpy(data, (uint8_t *), ndata);
+memcpy(data, (uint8_t *), MIN(sizeof(sector), ndata));
 if (sizeof(sector) < ndata) {
 memset(data + sizeof(sector), 0, ndata - sizeof(sector));
 }
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 07/12] readline: add a free function

Fixes leaks such as:

Direct leak of 2 byte(s) in 1 object(s) allocated from:
#0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
#1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
#2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
#3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
#4 0x55db720f1d46 in readline_hist_add 
/home/elmarco/src/qq/util/readline.c:258
#5 0x55db720f2d34 in readline_handle_byte 
/home/elmarco/src/qq/util/readline.c:387
#6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
#7 0x55db71f9be35 in qemu_chr_be_write_impl 
/home/elmarco/src/qq/chardev/char.c:167
#8 0x55db71f9bed3 in qemu_chr_be_write 
/home/elmarco/src/qq/chardev/char.c:179
#9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
#10 0x55db71fe18a8 in qio_channel_fd_source_dispatch 
/home/elmarco/src/qq/io/channel-watch.c:84
#11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
#12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
#13 0x55db720af3bd in glib_pollfds_poll 
/home/elmarco/src/qq/util/main-loop.c:214
#14 0x55db720af505 in os_host_main_loop_wait 
/home/elmarco/src/qq/util/main-loop.c:261
#15 0x55db720af6d6 in main_loop_wait 
/home/elmarco/src/qq/util/main-loop.c:515
#16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
#17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
#18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)

(while at it, use g_new0(ReadLineState), it's a bit easier to read)

Signed-off-by: Marc-André Lureau 
---
 include/qemu/readline.h |  1 +
 monitor.c   |  2 +-
 util/readline.c | 18 +-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/qemu/readline.h b/include/qemu/readline.h
index c08cf7400e..e81258322b 100644
--- a/include/qemu/readline.h
+++ b/include/qemu/readline.h
@@ -59,5 +59,6 @@ ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
  ReadLineFlushFunc *flush_func,
  void *opaque,
  ReadLineCompletionFunc *completion_finder);
+void readline_free(ReadLineState *rs);
 
 #endif /* READLINE_H */
diff --git a/monitor.c b/monitor.c
index e36fb5308d..024dd3d515 100644
--- a/monitor.c
+++ b/monitor.c
@@ -584,7 +584,7 @@ static void monitor_data_destroy(Monitor *mon)
 if (monitor_is_qmp(mon)) {
 json_message_parser_destroy(>qmp.parser);
 }
-g_free(mon->rs);
+readline_free(mon->rs);
 QDECREF(mon->outbuf);
 qemu_mutex_destroy(>out_lock);
 }
diff --git a/util/readline.c b/util/readline.c
index bbdee790b0..24ec839854 100644
--- a/util/readline.c
+++ b/util/readline.c
@@ -500,12 +500,28 @@ const char *readline_get_history(ReadLineState *rs, 
unsigned int index)
 return rs->history[index];
 }
 
+void readline_free(ReadLineState *rs)
+{
+int i;
+
+if (!rs) {
+return;
+}
+for (i = 0; i < READLINE_MAX_CMDS; i++) {
+g_free(rs->history[i]);
+}
+for (i = 0; i < READLINE_MAX_COMPLETIONS; i++) {
+g_free(rs->completions[i]);
+}
+g_free(rs);
+}
+
 ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
  ReadLineFlushFunc *flush_func,
  void *opaque,
  ReadLineCompletionFunc *completion_finder)
 {
-ReadLineState *rs = g_malloc0(sizeof(*rs));
+ReadLineState *rs = g_new0(ReadLineState, 1);
 
 rs->hist_entry = -1;
 rs->opaque = opaque;
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 11/12] tests: fix qmp-test leak

Direct leak of 913 byte(s) in 43 object(s) allocated from:
#0 0x55880a15df60 in __interceptor_malloc 
(/home/elmarco/src/qq/build/tests/qmp-test+0x110f60)
#1 0x7f3f20fd098f in _IO_vasprintf (/lib64/libc.so.6+0x8098f)

Signed-off-by: Marc-André Lureau 
---
 tests/qmp-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index c5a5c10b41..36feb2204b 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -271,7 +271,7 @@ static void add_query_tests(QmpSchema *schema)
 {
 SchemaInfoList *tail;
 SchemaInfo *si, *arg_type, *ret_type;
-const char *test_name;
+char *test_name;
 
 /* Test the query-like commands */
 for (tail = schema->list; tail; tail = tail->next) {
@@ -297,6 +297,7 @@ static void add_query_tests(QmpSchema *schema)
 
 test_name = g_strdup_printf("qmp/%s", si->name);
 qtest_add_data_func(test_name, si->name, test_query);
+g_free(test_name);
 }
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 06/12] vl: fix direct firmware directories leak

Note that data_dir[] will now point to allocated strings.

Fixes:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x7f1448181850 in malloc (/lib64/libasan.so.4+0xde850)
#1 0x7f1446ed8f0c in g_malloc ../glib/gmem.c:94
#2 0x7f1446ed91cf in g_malloc_n ../glib/gmem.c:331
#3 0x7f1446ef739a in g_strsplit ../glib/gstrfuncs.c:2364
#4 0x55cf276439d7 in main /home/elmarco/src/qq/vl.c:4311
#5 0x7f143dfad039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau 
---
 vl.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 1ad1c04637..763c2bf6ae 100644
--- a/vl.c
+++ b/vl.c
@@ -2363,7 +2363,7 @@ static void qemu_add_data_dir(const char *path)
 return; /* duplicate */
 }
 }
-data_dir[data_dir_idx++] = path;
+data_dir[data_dir_idx++] = g_strdup(path);
 }
 
 static inline bool nonempty_str(const char *str)
@@ -3124,7 +3124,7 @@ int main(int argc, char **argv, char **envp)
 Error *main_loop_err = NULL;
 Error *err = NULL;
 bool list_data_dirs = false;
-char **dirs;
+char *dir, **dirs;
 typedef struct BlockdevOptions_queue {
 BlockdevOptions *bdo;
 Location loc;
@@ -4312,9 +4312,12 @@ int main(int argc, char **argv, char **envp)
 for (i = 0; dirs[i] != NULL; i++) {
 qemu_add_data_dir(dirs[i]);
 }
+g_strfreev(dirs);
 
 /* try to find datadir relative to the executable path */
-qemu_add_data_dir(os_find_datadir());
+dir = os_find_datadir();
+qemu_add_data_dir(dir);
+g_free(dir);
 
 /* add the datadir specified when building */
 qemu_add_data_dir(CONFIG_QEMU_DATADIR);
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 08/12] tests: fix migration-test leak

Direct leak of 12 byte(s) in 2 object(s) allocated from:
#0 0x7f50d403c850 in malloc (/lib64/libasan.so.4+0xde850)
#1 0x7f50d1ddf98f in vasprintf (/lib64/libc.so.6+0x8098f)

Signed-off-by: Marc-André Lureau 
---
 tests/migration-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index be598d3257..799e24ebc6 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -358,13 +358,14 @@ static void migrate_check_parameter(QTestState *who, 
const char *parameter,
 const char *value)
 {
 QDict *rsp, *rsp_return;
-const char *result;
+char *result;
 
 rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
 rsp_return = qdict_get_qdict(rsp, "return");
 result = g_strdup_printf("%" PRId64,
  qdict_get_try_int(rsp_return,  parameter, -1));
 g_assert_cmpstr(result, ==, value);
+g_free(result);
 QDECREF(rsp);
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 05/12] tests: fix check-qobject leak:

/public/qobject_is_equal_conversion: OK

=
==14396==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 56 byte(s) in 1 object(s) allocated from:
#0 0x7f07682c5850 in malloc (/lib64/libasan.so.4+0xde850)
#1 0x7f0767d12f0c in g_malloc ../glib/gmem.c:94
#2 0x7f0767d131cf in g_malloc_n ../glib/gmem.c:331
#3 0x562bd767371f in do_test_equality 
/home/elmarco/src/qq/tests/check-qobject.c:49
#4 0x562bd7674a35 in qobject_is_equal_dict_test 
/home/elmarco/src/qq/tests/check-qobject.c:267
#5 0x7f0767d37b04 in test_case_run ../glib/gtestutils.c:2237
#6 0x7f0767d37ec4 in g_test_run_suite_internal ../glib/gtestutils.c:2321
#7 0x7f0767d37f6d in g_test_run_suite_internal ../glib/gtestutils.c:2333
#8 0x7f0767d38184 in g_test_run_suite ../glib/gtestutils.c:2408
#9 0x7f0767d36e0d in g_test_run ../glib/gtestutils.c:1674
#10 0x562bd7674e75 in main /home/elmarco/src/qq/tests/check-qobject.c:327
#11 0x7f0766009039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau 
---
 tests/check-qobject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 03e9175113..710f9e6b0a 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -59,6 +59,8 @@ static void do_test_equality(bool expected, int _, ...)
 g_assert(qobject_is_equal(args[i], args[j]) == expected);
 }
 }
+
+g_free(args);
 }
 
 #define check_equal(...) \
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 04/12] build-sys: add AddressSanitizer when --enable-debug if possible

Enable ASAN by default if the compiler supports it.

If necessary, we could consider a seperate configure option, although
I like the idea to have it enabled by default with --enable-debug, so
other people more actively fix errors/warnings, and having less
configure options in general.

Signed-off-by: Marc-André Lureau 
---
 configure | 5 +
 1 file changed, 5 insertions(+)

diff --git a/configure b/configure
index 2b8c71f522..52d9fd71e5 100755
--- a/configure
+++ b/configure
@@ -5129,6 +5129,11 @@ elif test "$fortify_source" = "yes" ; then
   CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
 elif test "$debug" = "no"; then
   CFLAGS="-O2 $CFLAGS"
+elif test "$debug" = "yes"; then
+write_c_skeleton;
+if compile_prog "-fsanitize=address" ""; then
+CFLAGS="-fsanitize=address $CFLAGS"
+fi
 fi
 
 ##
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 03/12] build-sys: add a rule to print a variable

$ make print-CFLAGS
CFLAGS=-fsanitize=address -Og -g

Trick from various sources:
https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile
https://www.cmcrossroads.com/article/printing-value-makefile-variable

Signed-off-by: Marc-André Lureau 
---
 Makefile | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index ab0354c153..80683e8c8b 100644
--- a/Makefile
+++ b/Makefile
@@ -6,7 +6,10 @@ BUILD_DIR=$(CURDIR)
 # Before including a proper config-host.mak, assume we are in the source tree
 SRC_PATH=.
 
-UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-% help
+UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-% help print-%
+
+print-%:
+   @echo '$*=$($*)'
 
 # All following code might depend on configuration variables
 ifneq ($(wildcard config-host.mak),)
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 02/12] build-sys: silence make by default

In particular, do not print anything when there is nothing to do, in
particular, after a successful build:

$ make
make[1]: '/home/elmarco/src/qemu/build/capstone/libcapstone.a' is up to date.

Signed-off-by: Marc-André Lureau 
---
 rules.mak | 5 +
 1 file changed, 5 insertions(+)

diff --git a/rules.mak b/rules.mak
index 6e943335f3..b760d54908 100644
--- a/rules.mak
+++ b/rules.mak
@@ -131,6 +131,11 @@ modules:
 # If called with only a single argument, will print nothing in quiet mode.
 quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, @$1))
 
+makeflags_ = $(makeflags_0)
+makeflags_0 = --no-print-directory -s
+makeflags_1 =
+MAKEFLAGS += $(makeflags_$(V))
+
 # cc-option
 # Usage: CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0)
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 00/12] Various build-sys and ASAN related fixes

Hi,

This is a small series that improves a bit the build system, and
introduce ASAN by default when --enable-debug. Them it fixes a few
leaks that occur during make check: common and x86_64 target tests are
leak free after this series, the other targets will need some
work. Finally, the last patch should help ASAN and remove some
false-positive, unfortunately it crashes ASAN and may be
incorrect. Help welcome!

Marc-André Lureau (12):
  build-sys: fix qemu-ga -pthread linking
  build-sys: silence make by default
  build-sys: add a rule to print a variable
  build-sys: add AddressSanitizer when --enable-debug if possible
  tests: fix check-qobject leak:
  vl: fix direct firmware directories leak
  readline: add a free function
  tests: fix migration-test leak
  crypto: fix stack-buffer-overflow error
  qemu-config: fix leak in query-command-line-options
  tests: fix qmp-test leak
  WIP ucontext: annotate coroutine stack for ASAN

 include/qemu/readline.h   |  1 +
 crypto/ivgen-essiv.c  |  2 +-
 monitor.c |  2 +-
 tests/check-qobject.c |  2 ++
 tests/migration-test.c|  3 ++-
 tests/qmp-test.c  |  3 ++-
 util/coroutine-ucontext.c | 47 +++
 util/qemu-config.c|  3 ++-
 util/readline.c   | 18 +-
 vl.c  |  9 ++---
 Makefile  |  5 -
 configure |  6 ++
 rules.mak |  5 +
 13 files changed, 96 insertions(+), 10 deletions(-)

-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH 01/12] build-sys: fix qemu-ga -pthread linking

When linking qemu-ga under some configuration (when gthread-2.0.pc
doesn't have -pthread, as happening atm with meson build), you may
have this linking issue:

/usr/bin/ld: libqemuutil.a(qemu-thread-posix.o): undefined reference to symbol 
'pthread_setname_np@@GLIBC_2.12'
/usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line

Make sure qemu-ga links with the pthread library, by adding correct
flags to libs_qga.

Signed-off-by: Marc-André Lureau 
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 0c6e7572db..2b8c71f522 100755
--- a/configure
+++ b/configure
@@ -3436,6 +3436,7 @@ else
   done
   if test "$found" = "no"; then
 LIBS="$pthread_lib $LIBS"
+libs_qga="$pthread_lib $libs_qga"
   fi
   PTHREAD_LIB="$pthread_lib"
   break
-- 
2.15.1.355.g36791d7216

Re: [Qemu-devel] [PATCH 00/12] Refactor get_phys_addr() not to return FSR values

On 12/05/2017 11:46 AM, Peter Maydell wrote:
> Edgar E. Iglesias (1):
>   target/arm: Extend PAR format determination
> 
> Peter Maydell (11):
>   target/arm: Provide fault type enum and FSR conversion functions
>   target/arm: Remove fsr argument from arm_ld*_ptw()
>   target/arm: Convert get_phys_addr_v5() to not return FSC values
>   target/arm: Convert get_phys_addr_v6() to not return FSC values
>   target/arm: Convert get_phys_addr_lpae() to not return FSC values
>   target/arm: Convert get_phys_addr_pmsav5() to not return FSC values
>   target/arm: Convert get_phys_addr_pmsav7() to not return FSC values
>   target/arm: Convert get_phys_addr_pmsav8() to not return FSC values
>   target/arm: Use ARMMMUFaultInfo in deliver_fault()
>   target/arm: Ignore fsr from get_phys_addr() in do_ats_write()
>   target/arm: Remove fsr argument from get_phys_addr() and
> arm_tlb_fill()

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD

On 12/07/2017 07:31 AM, David Hildenbrand wrote:
> CRW machine check handling requires STCRW. So let's wire it up.
> 
> Reviewed-by: Thomas Huth 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/helper.h  | 1 +
>  target/s390x/insn-data.def | 1 +
>  target/s390x/misc_helper.c | 9 +
>  target/s390x/translate.c   | 8 
>  4 files changed, 19 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 4/5] s390x/tcg: indicate value of TODPR in STCKE

On 12/07/2017 07:31 AM, David Hildenbrand wrote:
> +/* 16 bit value store in an uint32_t (only valid bits set) */
> +tcg_gen_ld32u_i64(todpr, cpu_env, offsetof(CPUS390XState, todpr));

Any reason not to use a uint16_t and use tcg_gen_ld16u_i64 here?


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 3/5] s390x/tcg: implement SET CLOCK PROGRAMMABLE FIELD

On 12/07/2017 07:31 AM, David Hildenbrand wrote:
> +DEF_HELPER_FLAGS_1(sckpf, TCG_CALL_NO_RWG, void, env)
...
> +/* Set Tod Programmable Field */
> +void HELPER(sckpf)(CPUS390XState *env)
> +{
> +uint32_t val = env->regs[0];
> +
> +if (val & 0x) {
> +s390_program_interrupt(env, PGM_SPECIFICATION, 2, GETPC());
> +}
> +env->todpr = val;
> +}

You do read a tcg global -- regs[0].  Either pass in r0 as a parameter or use
TCG_CALL_NO_WG.


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 10/10] s390x: change the QEMU cpu model to a stripped down z12

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> +const S390FeatBitmap qemu_cpu_feat = { S390_FEAT_LIST_QEMU_V2_12 };

static const?


r~

[Qemu-devel] Interactive Boot Menu: New Fields in IPLB

2017-12-07 Thread Collin L. Walling


Hello,

I'd like to bring up a formal discussion regarding the utilization of 
reserved space

in the IPL Parameter Blocks for s390x guests. Particularly the following:

 * How should we approach "claiming" bytes so that we do not obstruct
   future development?
 * What would be "the best" location to store these new fields?

I've posted some relevant information regarding this feature below.


                      --- The Fields We Need To Store ---

Currently, we're utilizing 3 bytes out of a 77 byte reserved field in 
the IPLB for
CCW and SCSI. We do this because need a way to pass the Qemu command 
line options
for a boot menu (on/off and timeout) to the s390-ccw bios. We require 
only two

fields in the IPLB, totaling 3 bytes:

 * uint8_t boot_menu_flag
 o determines if we should show the menu or not

 * uint16_t boot_menu_timeout
 o stored as milliseconds
 o A max value of approx 65,000 gives us 65 seconds -- should be 
plenty

 o could potentially be reduced to one byte, and we store the value
   as seconds instead

Note: these fields *only*have value in a QEMU environment.


                  --- The Data We Have In Place Already ---

The following can be found in qemu/hw/s390x/ipl.h: (a similar structure 
exists

in qemu/pc-bios/s390-ccw/iplb.h)

struct IplBlockCcw {
    uint64_t netboot_start_addr;
    uint8_t  reserved0[74];        // <--- previously a 77 reserved field
    uint16_t boot_menu_timeout;    // new
    uint8_t  boot_menu_flag;       // new
    uint8_t  ssid;
    uint16_t devno;
    uint8_t  vm_flags;
    uint8_t  reserved3[3];
    uint32_t vm_parm_len;
    uint8_t  nss_name[8];
    uint8_t  vm_parm[64];
    uint8_t  reserved4[8];
} QEMU_PACKED;
typedef struct IplBlockCcw IplBlockCcw;

struct IplBlockFcp {
    uint8_t  reserved1[305 - 1];
    uint8_t  opt;
    uint8_t  reserved2[3];
    uint16_t reserved3;
    uint16_t devno;
    uint8_t  reserved4[4];
    uint64_t wwpn;
    uint64_t lun;
    uint32_t bootprog;
    uint8_t  reserved5[12];
    uint64_t br_lba;
    uint32_t scp_data_len;
    uint8_t  reserved6[260];
    uint8_t  scp_data[];
} QEMU_PACKED;
typedef struct IplBlockFcp IplBlockFcp;

struct IplBlockQemuScsi {
    uint32_t lun;
    uint16_t target;
    uint16_t channel;
    uint8_t  reserved0[74];        // <--- previously a 77 reserved field
    uint16_t boot_menu_timeout;    // new
    uint8_t  boot_menu_flag;       // new
    uint8_t  ssid;
    uint16_t devno;
} QEMU_PACKED;
typedef struct IplBlockQemuScsi IplBlockQemuScsi;

[...]

union IplParameterBlock {
    struct {
    uint32_t len;
    uint8_t  reserved0[3];
    uint8_t  version;
    uint32_t blk0_len;
    uint8_t  pbt;
    uint8_t  flags;
    uint16_t reserved01;
    uint8_t  loadparm[8];
    union {
    IplBlockCcw ccw;
    IplBlockFcp fcp;
    IplBlockQemuScsi scsi;
    };
    } QEMU_PACKED;
    struct {
    uint8_t  reserved1[110];
    uint16_t devno;
    uint8_t  reserved2[88];
    uint8_t  reserved_ext[4096 - 200];
    } QEMU_PACKED;
} QEMU_PACKED;
typedef union IplParameterBlock IplParameterBlock;

-

Thanks for your time,
- Collin L Walling

Re: [Qemu-devel] [PATCH v2 for-2.12 08/10] s390x/tcg: implement extract-CPU-time facility

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> It only provides the EXTRACT CPU TIME instruction. We can reuse the stpt
> helper, which calculates the CPU timer value.
> 
> As the instruction is not privileged, but we don't have a CPU timer
> value in case of linux user, we simply fake the CPU timer to be 0.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/cpu_models.c  |  1 +
>  target/s390x/insn-data.def |  2 ++
>  target/s390x/translate.c   | 36 
>  3 files changed, 39 insertions(+)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 94d24e423d..0be037eac1 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -834,6 +834,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
> fbm)
>  S390_FEAT_STORE_CLOCK_FAST,
>  S390_FEAT_MOVE_WITH_OPTIONAL_SPEC,
>  S390_FEAT_ETF3_ENH,
> +S390_FEAT_EXTRACT_CPU_TIME,
>  S390_FEAT_COMPARE_AND_SWAP_AND_STORE,
>  S390_FEAT_COMPARE_AND_SWAP_AND_STORE_2,
>  S390_FEAT_GENERAL_INSTRUCTIONS_EXT,
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index f7b66b0091..5e33bd27ff 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -369,6 +369,8 @@
>  C(0xb24f, EAR, RRE,   Z,   0, 0, new, r1_32, ear, 0)
>  /* EXTRACT CPU ATTRIBUTE */
>  C(0xeb4c, ECAG,RSY_a, GIE, 0, a2, r1, 0, ecag, 0)
> +/* EXTRACT CPU TIME */
> +C(0xc801, ECTG,SSF,   ECT, 0, 0, 0, 0, ectg, 0)
>  /* EXTRACT FPC */
>  C(0xb38c, EFPC,RRE,   Z,   0, 0, new, r1_32, efpc, 0)
>  /* EXTRACT PSW */
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 1e4079464a..e0f55fc8e9 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -3887,6 +3887,41 @@ static ExitStatus op_spm(DisasContext *s, DisasOps *o)
>  return NO_EXIT;
>  }
>  
> +static ExitStatus op_ectg(DisasContext *s, DisasOps *o)
> +{
> +int b1 = get_field(s->fields, b1);
> +int d1 = get_field(s->fields, d1);
> +int b2 = get_field(s->fields, b2);
> +int d2 = get_field(s->fields, d2);
> +int r3 = get_field(s->fields, r3);
> +TCGv_i64 tmp = tcg_temp_new_i64();
> +
> +/* fetch all operands first */
> +o->in1 = tcg_temp_new_i64();
> +tcg_gen_addi_i64(o->in1, regs[b1], d1);
> +o->in2 = tcg_temp_new_i64();
> +tcg_gen_addi_i64(o->in2, regs[b2], d2);
> +o->addr1 = get_address(s, 0, r3, 0);
> +
> +/* load the third operand into r3 before modifying anything */
> +tcg_gen_qemu_ld64(regs[r3], o->addr1, get_mem_index(s));
> +
> +#ifndef CONFIG_USER_ONLY
> +/* subtract CPU timer from first operand and store in GR0 */
> +gen_helper_stpt(tmp, cpu_env);
> +tcg_gen_sub_i64(regs[0], o->in1, tmp);
> +#else
> +/* we don't have a CPU timer, fake value 0 */
> +tcg_gen_mov_i64(regs[0], o->in1);
> +#endif

It's easy enough to pass along cpu_get_host_ticks() for user-only.  Possibly
not quite right, but better than nothing.


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 09/10] s390x/tcg: we already implement the Set-Program-Parameter facility

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> The Set-Program-Parameter facility (also known as Load-Program-Parameter
> facility) provides the LPP instruction used to load the program
> parameter. We already implement that instruction in TCG, so add it to our
> list.
> 
> Note: Not documented in the PoP but in "The Load-Program-Parameter and
> CPU-Measurement Facilities) - SA23-2260-05 document.
> 
> While at it, make the whole list ordered (according to cpu_features_def.h).
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/cpu_models.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [virtio-dev] [PATCH v3 0/7] Vhost-pci for inter-VM communication

2017-12-07 Thread Michael S. Tsirkin

On Thu, Dec 07, 2017 at 06:28:19PM +, Stefan Hajnoczi wrote:
> On Thu, Dec 7, 2017 at 5:38 PM, Michael S. Tsirkin  wrote:
> > On Thu, Dec 07, 2017 at 05:29:14PM +, Stefan Hajnoczi wrote:
> >> On Thu, Dec 7, 2017 at 4:47 PM, Michael S. Tsirkin  wrote:
> >> > On Thu, Dec 07, 2017 at 04:29:45PM +, Stefan Hajnoczi wrote:
> >> >> On Thu, Dec 7, 2017 at 2:02 PM, Michael S. Tsirkin  
> >> >> wrote:
> >> >> > On Thu, Dec 07, 2017 at 01:08:04PM +, Stefan Hajnoczi wrote:
> >> >> >> Instead of responding individually to these points, I hope this will
> >> >> >> explain my perspective.  Let me know if you do want individual
> >> >> >> responses, I'm happy to talk more about the points above but I think
> >> >> >> the biggest difference is our perspective on this:
> >> >> >>
> >> >> >> Existing vhost-user slave code should be able to run on top of
> >> >> >> vhost-pci.  For example, QEMU's
> >> >> >> contrib/vhost-user-scsi/vhost-user-scsi.c should work inside the 
> >> >> >> guest
> >> >> >> with only minimal changes to the source file (i.e. today it 
> >> >> >> explicitly
> >> >> >> opens a UNIX domain socket and that should be done by libvhost-user
> >> >> >> instead).  It shouldn't be hard to add vhost-pci vfio support to
> >> >> >> contrib/libvhost-user/ alongside the existing UNIX domain socket 
> >> >> >> code.
> >> >> >>
> >> >> >> This seems pretty easy to achieve with the vhost-pci PCI adapter that
> >> >> >> I've described but I'm not sure how to implement libvhost-user on top
> >> >> >> of vhost-pci vfio if the device doesn't expose the vhost-user
> >> >> >> protocol.
> >> >> >>
> >> >> >> I think this is a really important goal.  Let's use a single
> >> >> >> vhost-user software stack instead of creating a separate one for 
> >> >> >> guest
> >> >> >> code only.
> >> >> >>
> >> >> >> Do you agree that the vhost-user software stack should be shared
> >> >> >> between host userspace and guest code as much as possible?
> >> >> >
> >> >> >
> >> >> >
> >> >> > The sharing you propose is not necessarily practical because the 
> >> >> > security goals
> >> >> > of the two are different.
> >> >> >
> >> >> > It seems that the best motivation presentation is still the original 
> >> >> > rfc
> >> >> >
> >> >> > http://virtualization.linux-foundation.narkive.com/A7FkzAgp/rfc-vhost-user-enhancements-for-vm2vm-communication
> >> >> >
> >> >> > So comparing with vhost-user iotlb handling is different:
> >> >> >
> >> >> > With vhost-user guest trusts the vhost-user backend on the host.
> >> >> >
> >> >> > With vhost-pci we can strive to limit the trust to qemu only.
> >> >> > The switch running within a VM does not have to be trusted.
> >> >>
> >> >> Can you give a concrete example?
> >> >>
> >> >> I have an idea about what you're saying but it may be wrong:
> >> >>
> >> >> Today the iotlb mechanism in vhost-user does not actually enforce
> >> >> memory permissions.  The vhost-user slave has full access to mmapped
> >> >> memory regions even when iotlb is enabled.  Currently the iotlb just
> >> >> adds an indirection layer but no real security.  (Is this correct?)
> >> >
> >> > Not exactly. iotlb protects against malicious drivers within guest.
> >> > But yes, not against a vhost-user driver on the host.
> >> >
> >> >> Are you saying the vhost-pci device code in QEMU should enforce iotlb
> >> >> permissions so the vhost-user slave guest only has access to memory
> >> >> regions that are allowed by the iotlb?
> >> >
> >> > Yes.
> >>
> >> Okay, thanks for confirming.
> >>
> >> This can be supported by the approach I've described.  The vhost-pci
> >> QEMU code has control over the BAR memory so it can prevent the guest
> >> from accessing regions that are not allowed by the iotlb.
> >>
> >> Inside the guest the vhost-user slave still has the memory region
> >> descriptions and sends iotlb messages.  This is completely compatible
> >> with the libvirt-user APIs and existing vhost-user slave code can run
> >> fine.  The only unique thing is that guest accesses to memory regions
> >> not allowed by the iotlb do not work because QEMU has prevented it.
> >
> > I don't think this can work since suddenly you need
> > to map full IOMMU address space into BAR.
> 
> The BAR covers all guest RAM
> but QEMU can set up MemoryRegions that
> hide parts from the guest (e.g. reads produce 0xff).  I'm not sure how
> expensive that is but implementing a strict IOMMU is hard to do
> without performance overhead.

I'm worried about leaking PAs.
fundamentally if you want proper protection you
need your device driver to use VA for addressing,

On the one hand BAR only needs to be as large as guest PA then.
On the other hand it must cover all of guest PA,
not just what is accessible to the device.


>
> > Besides, this means implementing iotlb in both qemu and guest.
> 
> It's free in the guest, the libvhost-user stack already has it.

That library is designed to work with a unix

Re: [Qemu-devel] [PATCH v2 for-2.12 07/10] s390x/tcg: Implement SIGNAL ADAPTER instruction

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> KVM suppresses SIGA, setting cc=3. Let's do the same for TCG, so we're at
> least equal.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/insn-data.def | 1 +
>  target/s390x/translate.c   | 8 
>  2 files changed, 9 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 06/10] s390x/tcg: Implement STORE CHANNEL PATH STATUS

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> Just like KVM does, we should suppress this instruction:
> When this instruction is not provided, it is
> checked for privileged operation exception and the
> instruction is suppressed by the machine
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/insn-data.def | 1 +
>  target/s390x/translate.c   | 7 +++
>  2 files changed, 8 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 05/10] s390x/tcg: wire up SET CHANNEL MONITOR

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> Let's just wire it up like KVM.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/helper.h  | 1 +
>  target/s390x/insn-data.def | 1 +
>  target/s390x/misc_helper.c | 9 +
>  target/s390x/translate.c   | 7 +++
>  4 files changed, 18 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 04/10] s390x/tcg: wire up SET ADDRESS LIMIT

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> Let's handle it just like KVM:
> Depending on the model, this instruction may not be
> provided. When this instruction is not provided, it is
> checked for operand exception and privileged-opera-
> tion exception, and then is suppressed.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/helper.h  | 1 +
>  target/s390x/insn-data.def | 1 +
>  target/s390x/misc_helper.c | 9 +
>  target/s390x/translate.c   | 7 +++
>  4 files changed, 18 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 03/10] s390x/tcg: implement Interlocked-Access Facility 2

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> +static ExitStatus op_ni(DisasContext *s, DisasOps *o)
> +{
> +o->in1 = tcg_temp_new_i64();
> +/* Perform the atomic operation in memory. */
> +tcg_gen_atomic_fetch_and_i64(o->in1, o->addr1, o->in2, get_mem_index(s),
> + MO_UB);
> +/* We need to recompute the operation for setting CC.  */
> +tcg_gen_and_i64(o->out, o->in1, o->in2);
> +return NO_EXIT;
> +}

Similarly, check interlocked-access-facility-2?


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 02/10] s390x/tcg: ALSI/ALSGI are atomic with interlocked-acccess facility 1

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> We can simply reuse our ASI implementation. Only the way CC is
> calculated differs.
> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/insn-data.def | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Squash with previous?


r~

Re: [Qemu-devel] [PATCH v2 for-2.12 01/10] s390x/tcg: ASI/ASGI are atomic with interlocked-acccess facility 1

On 12/07/2017 08:53 AM, David Hildenbrand wrote:
> +static ExitStatus op_asi(DisasContext *s, DisasOps *o)
> +{
> +o->in1 = tcg_temp_new_i64();
> +/* Perform the atomic addition in memory. */
> +tcg_gen_atomic_fetch_add_i64(o->in1, o->addr1, o->in2, get_mem_index(s),
> + s->insn->data);
> +/* However, we need to recompute the addition for setting CC.  */
> +tcg_gen_add_i64(o->out, o->in1, o->in2);
> +return NO_EXIT;
> +}

Is it worth conditionalizing the atomic operation on having
interlocked-access-facility-1 enabled?


r~

Re: [Qemu-devel] [PATCH v2] Add AVX, AVX-512, MPX support to x86_cpu_dump_state

2017-12-07 Thread Doug Gale

On Thu, Dec 7, 2017 at 5:37 PM, Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 12/02/2017 10:35 PM, Doug Gale wrote:
> > Signed-off-by: Doug Gale 
> > ---
> > Fix MSB LSB showing when SSE is disabled
> >  target/i386/helper.c | 95 ++
> +++---
> >  1 file changed, 83 insertions(+), 12 deletions(-)
> >
> > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > index f63eb3d3f4..03812b6e87 100644
> > --- a/target/i386/helper.c
> > +++ b/target/i386/helper.c
> > @@ -543,6 +543,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf,
> >  }
> >  }
> >  cpu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer);
> > +cpu_fprintf(f, "XCR0=%016" PRIx64 "\n", env->xcr0);
> >  if (flags & CPU_DUMP_FPU) {
> >  int fptag;
> >  fptag = 0;
> > @@ -565,21 +566,91 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf,
> >  else
> >  cpu_fprintf(f, " ");
> >  }
> > -if (env->hflags & HF_CS64_MASK)
> > -nb = 16;
> > -else
> > +
> > +if (env->hflags & HF_CS64_MASK) {
> > +if (env->xcr0 & XSTATE_Hi16_ZMM_MASK) {
> > +/* AVX-512 32 registers enabled */
> > +nb = 32;
> > +} else {
> > +/* 64-bit mode, 16 registers */
> > +nb = 16;
> > +}
> > +} else {
> > +/* 32 bit mode, 8 registers */
> >  nb = 8;
> > -for(i=0;i > -cpu_fprintf(f, "XMM%02d=%08x%08x%08x%08x",
> > -i,
> > -env->xmm_regs[i].ZMM_L(3),
> > -env->xmm_regs[i].ZMM_L(2),
> > -env->xmm_regs[i].ZMM_L(1),
> > -env->xmm_regs[i].ZMM_L(0));
> > -if ((i & 1) == 1)
> > +}
> > +
> > +/* sse register width in units of 64 bits */
> > +int zmm_width;
> > +char zmm_name;
> > +if (env->xcr0 & XSTATE_ZMM_Hi256_MASK) {
> > +/* 512-bit "ZMM" - AVX-512 registers enabled */
> > +zmm_width = 8;
> > +zmm_name = 'Z';
> > +} else if (env->xcr0 & XSTATE_YMM_MASK) {
> > +/* 256-bit "YMM" - AVX enabled */
> > +zmm_width = 4;
> > +zmm_name = 'Y';
> > +} else if (env->cr[4] & CR4_OSFXSR_MASK) {
> > +/* 128-bit "XMM" - SSE enabled */
> > +zmm_width = 2;
> > +zmm_name = 'X';
> > +} else {
> > +/* SSE not enabled */
> > +zmm_width = 0;
> > +zmm_name = 0;
>
> You should exit the function here ...
>
> > +}
> > +
> > +if (zmm_width > 0) {
> > +cpu_fprintf(f, "  MSB%*sLSB\n",
> > +-(zmm_width * 16 + zmm_width - 7), "");
> > +}
> > +
> > +for (i = 0; i < nb; i++) {
> > +if (zmm_width == 0) {
> > +cpu_fprintf(f, "SSE not enabled\n");
> > +break;
> > +}
>
> ... rather than performing this test in a loop.
> Or indeed, printing anything at all.
>
> I assume this is in order to examine registers in the monitor under kvm?
>
>
> r~
>


Yes, to examine the registers in the monitor and improve the output of
things like -d int. I ran into an issue with
 guest
code and I wanted to check XCR0 to see if AVX was enabled, and I didn't see
any AVX registers
, which was very misleading
.
 I couldn't check XCR0 to see if it was enabled either.
I investigated and found that AVX was enabled, and the monitor output was
false. I added XCR0 and the entire AVX
/AVX-512
and MPX state to the
qemu 
output.

Why would it exit the function there? Are you sure the other states I
further down won't be enabled? It prints the MPX state further down. I put
the test in the loop so I wouldn't have to indent that whole `for` loop
another level, to improve readability. Is the performance of that loop
really relevant? The fprintf's will take at least thousands of times more
CPU time than that predictable branch.

Re: [Qemu-devel] [PATCH v5 23/23] sev: add migration blocker

2017-12-07 Thread Brijesh Singh




On 12/07/2017 05:03 AM, Dr. David Alan Gilbert wrote:
...

  
  #define SEV_FW_MAX_ERROR  0x17
  
@@ -460,6 +462,7 @@ static void

  sev_launch_finish(SEVState *s)
  {
  int ret, error;
+Error *local_err = NULL;
  
  ret = sev_ioctl(KVM_SEV_LAUNCH_FINISH, 0, );

  if (ret) {
@@ -470,6 +473,16 @@ sev_launch_finish(SEVState *s)
  
  s->cur_state = SEV_STATE_RUNNING;

  DPRINTF("SEV: LAUNCH_FINISH\n");


(from a previous patch)
Please use the tracing facility rather than new DPRINTF's
if possible - if you've not used it before, then
--enable-trace-backends=log   is the easy way to get going
and you can turn on and off the stuff you're interested in
tracing at run time without having to rebuild.



Thanks for review, I will look into converting those DPRINTF's in trace 
logging.

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12 0/4] qmp dirty bitmap API

2017-12-07 Thread John Snow



On 12/07/2017 06:56 AM, Kashyap Chamarthy wrote:
> On Tue, Nov 21, 2017 at 07:10:19PM -0500, John Snow wrote:
>> On 11/21/2017 12:23 PM, Kevin Wolf wrote:
> 
> [...] # Snip lot of good discussion.
> 
>>> On another note, I would double check before adding a new block job type
>>> that this is the right way to go. We have recently noticed that many, if
>>> not all, of the existing block jobs (at least mirror, commit and backup)
>>> are so similar that they implement the same things multiple times and
>>> are just lacking different options and have different bugs. I'm
>>> seriously considering merging all of them into a single job type
>>> internally that just provides options that effectively turn it into one
>>> of the existing job types.
>>>
>>
>> I'm not particularly opposed. At the very, very least "backup" and
>> "mirror" are pretty much the same thing and "stream" and "commit" are
>> basically the same.
>>
>> Forcing the backuppy-job and the consolidatey-job together seems like an
>> ever-so-slightly harder case to make, but I suppose the truth of the
>> matter in all cases is that we're copying data from one node to another...
> 
> So from the above interesting discussion, it seems like Kevin is leaning
> towards a single job type that offers 'stream', 'commit', 'backup', and
> 'mirror' functionality as part of a single command / job type.  Based on
> an instinct, this sounds a bit too stuffy and complex to me.
> 
> And John seems to be leaning towards two block device job types:
> 
>   - 'blockdev-foo' that offers both current 'stream' and 'commit'
> functionality as two different options to the same QMP command; and
> 
>   - 'blockdev-bar' will offer both 'mirror' and 'backup' functionality
> as part of the same QMP command
> 
> FWIW, this seems a bit more palatable, as it is unifying
> similar-functionality-that-differ-slightly into two distinct commands.
> 
> (/me is still wrapping his head around the bitmaps and incremental
> backups infrastructure.)
> 

The discussion you're honing in on is tangential to the one at the heart
of this topic, which is:

"What's the right API for pull model incremental backups?" which digs up
the question "What is the right API for our existing data copying commands?"

Kevin is probably right in the end, though; he usually is. We do need to
unify our data moving logic to avoid fixing the same bugs in four
different but nearly identical jobs.

My only concern is that the special casing will grow too complex for
that one unified job and the job will become so complicated nobody knows
how to work on it anymore without breaking other cases. Maybe this is a
reasonable concern, maybe it isn't.

I'm also not in a hurry to personally unify the loops myself, but if
someone did, they could CC me and I'd review it thoroughly.

We'd find out in a hurry from the implementation if unification was a
win for SLOC.

--js

Re: [Qemu-devel] [PATCH v2 6/6] qemu-iotests: add 203 savevm with IOThreads test

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> This test case will prevent future regressions with savevm and
> IOThreads.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/203 | 59 
> ++
>  tests/qemu-iotests/203.out |  6 +
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 66 insertions(+)
>  create mode 100755 tests/qemu-iotests/203
>  create mode 100644 tests/qemu-iotests/203.out
> 

> +#
> +# Creator/Owner: Stefan Hajnoczi 

Again, not sure we need this line.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 5/6] iothread: fix iothread_stop() race condition

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> There is a small chance that iothread_stop() hangs as follows:
> 

> 
> The bug is explained by the AioContext->notify_me doc comments:
> 
>   "If this field is 0, everything (file descriptors, bottom halves,
>   timers) will be re-evaluated before the next blocking poll(), thus the
>   event_notifier_set call can be skipped."
> 
> The problem is that "everything" does not include checking
> iothread->stopping.  This means iothread_run() will block in aio_poll()
> if aio_notify() was called just before aio_poll().
> 
> This patch fixes the hang by replacing aio_notify() with
> aio_bh_schedule_oneshot().  This makes aio_poll() or g_main_loop_run()
> to return.

s/to //

> 
> Implementing this properly required a new bool running flag.  The new
> flag prevents races that are tricky if we try to use iothread->stopping.
> Now iothread->stopping is purely for iothread_stop() and
> iothread->running is purely for the iothread_run() thread.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
> I'm including this patch in this series because it is needed to make the
> test case reliable.  It's unrelated to the main goal of the patch
> series.
> ---
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 4/6] iotests: add VM.add_object()

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> The VM.add_object() method can be used to add IOThreads or memory
> backend objects.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/iotests.py | 5 +
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 3/6] blockdev: add x-blockdev-set-iothread force boolean

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> When a node is already associated with a BlockBackend the
> x-blockdev-set-iothread command refuses to set the IOThread.  This is to
> prevent accidentally changing the IOThread when the nodes are in use.
> 
> When the nodes are created with -drive they automatically get a
> BlockBackend.  In that case we know nothing is using them yet and it's
> safe to set the IOThread.  Add a force boolean to override the check.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  qapi/block-core.json |  6 +-
>  blockdev.c   | 11 ++-
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2] Add AVX, AVX-512, MPX support to x86_cpu_dump_state

On 12/02/2017 10:35 PM, Doug Gale wrote:
> Signed-off-by: Doug Gale 
> ---
> Fix MSB LSB showing when SSE is disabled
>  target/i386/helper.c | 95 
> +---
>  1 file changed, 83 insertions(+), 12 deletions(-)
> 
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index f63eb3d3f4..03812b6e87 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -543,6 +543,7 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, 
> fprintf_function cpu_fprintf,
>  }
>  }
>  cpu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer);
> +cpu_fprintf(f, "XCR0=%016" PRIx64 "\n", env->xcr0);
>  if (flags & CPU_DUMP_FPU) {
>  int fptag;
>  fptag = 0;
> @@ -565,21 +566,91 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, 
> fprintf_function cpu_fprintf,
>  else
>  cpu_fprintf(f, " ");
>  }
> -if (env->hflags & HF_CS64_MASK)
> -nb = 16;
> -else
> +
> +if (env->hflags & HF_CS64_MASK) {
> +if (env->xcr0 & XSTATE_Hi16_ZMM_MASK) {
> +/* AVX-512 32 registers enabled */
> +nb = 32;
> +} else {
> +/* 64-bit mode, 16 registers */
> +nb = 16;
> +}
> +} else {
> +/* 32 bit mode, 8 registers */
>  nb = 8;
> -for(i=0;i -cpu_fprintf(f, "XMM%02d=%08x%08x%08x%08x",
> -i,
> -env->xmm_regs[i].ZMM_L(3),
> -env->xmm_regs[i].ZMM_L(2),
> -env->xmm_regs[i].ZMM_L(1),
> -env->xmm_regs[i].ZMM_L(0));
> -if ((i & 1) == 1)
> +}
> +
> +/* sse register width in units of 64 bits */
> +int zmm_width;
> +char zmm_name;
> +if (env->xcr0 & XSTATE_ZMM_Hi256_MASK) {
> +/* 512-bit "ZMM" - AVX-512 registers enabled */
> +zmm_width = 8;
> +zmm_name = 'Z';
> +} else if (env->xcr0 & XSTATE_YMM_MASK) {
> +/* 256-bit "YMM" - AVX enabled */
> +zmm_width = 4;
> +zmm_name = 'Y';
> +} else if (env->cr[4] & CR4_OSFXSR_MASK) {
> +/* 128-bit "XMM" - SSE enabled */
> +zmm_width = 2;
> +zmm_name = 'X';
> +} else {
> +/* SSE not enabled */
> +zmm_width = 0;
> +zmm_name = 0;

You should exit the function here ...

> +}
> +
> +if (zmm_width > 0) {
> +cpu_fprintf(f, "  MSB%*sLSB\n",
> +-(zmm_width * 16 + zmm_width - 7), "");
> +}
> +
> +for (i = 0; i < nb; i++) {
> +if (zmm_width == 0) {
> +cpu_fprintf(f, "SSE not enabled\n");
> +break;
> +}

... rather than performing this test in a loop.
Or indeed, printing anything at all.

I assume this is in order to examine registers in the monitor under kvm?


r~

Re: [Qemu-devel] [PATCH v2 2/6] docs: mark nested AioContext locking as a legacy API

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> See the patch for why nested AioContext locking is no longer allowed.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  docs/devel/multiple-iothreads.txt | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/6] block: avoid recursive AioContext acquire in bdrv_inactivate_all()

On 12/07/2017 02:13 PM, Stefan Hajnoczi wrote:
> From: Paolo Bonzini 
> 
> BDRV_POLL_WHILE() does not support recursive AioContext locking.  It
> only releases the AioContext lock once regardless of how many times the
> caller has acquired it.  This results in a hang since the IOThread does
> not make progress while the AioContext is still locked.
> 
> The following steps trigger the hang:
> 
>   $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \
>-object iothread,id=iothread0 \
>-device virtio-scsi-pci,iothread=iothread0 \
>-drive if=none,id=drive0,file=test.img,format=raw \
>-device scsi-hd,drive=drive0 \
>-drive if=none,id=drive1,file=test.img,format=raw \
>-device scsi-hd,drive=drive1
>   $ qemu-system-x86_64 ...same options... \
>-incoming tcp::1234
>   (qemu) migrate tcp:127.0.0.1:1234
>   ...hang...
> 
> Tested-by: Stefan Hajnoczi 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 9/9] qemu-iotests: add 202 external snapshots IOThread test

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> QMP 'transaction' blockdev-snapshot-sync with multiple disks in an
> IOThread is an untested code path.  Several bugs have been found in
> connection with this command.  This patch adds a test case to prevent
> future regressions.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/202 | 95 
> ++
>  tests/qemu-iotests/202.out | 11 ++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 107 insertions(+)
>  create mode 100755 tests/qemu-iotests/202
>  create mode 100644 tests/qemu-iotests/202.out
> 

> +# Creator/Owner: Stefan Hajnoczi 

There's another cleanup series posted that drops these sorts of lines in
favor of git history.

> +++ b/tests/qemu-iotests/202.out
> @@ -0,0 +1,11 @@
> +Launching VM...
> +Adding IOThread...
> +{u'return': {}}
> +Adding blockdevs...
> +{u'return': {}}
> +{u'return': {}}
> +Setting iothread...
> +{u'return': {}}
> +{u'return': {}}
> +Creating external snapshots...
> +{u'return': {}}

Yay - a python test with sane output - if I had to, I could actually
debug this test without too much effort!

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 5/9] block: don't keep AioContext acquired after internal_snapshot_prepare()

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi 
> ---
>  blockdev.c | 47 +++
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 13/14] i386: Rename bools in PCMachineState to end in _enabled

From: Corey Minyard 

This makes their function more clear and prevents conflicts when adding
the actual devices to the machine state, if necessary.

Signed-off-by: Corey Minyard 
---
 hw/i386/pc.c | 18 +-
 hw/i386/pc_piix.c|  3 ++-
 hw/i386/pc_q35.c |  6 +++---
 include/hw/i386/pc.h |  6 +++---
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 186545d..51f38b8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2184,42 +2184,42 @@ static bool pc_machine_get_smbus(Object *obj, Error 
**errp)
 {
 PCMachineState *pcms = PC_MACHINE(obj);
 
-return pcms->smbus;
+return pcms->smbus_enabled;
 }
 
 static void pc_machine_set_smbus(Object *obj, bool value, Error **errp)
 {
 PCMachineState *pcms = PC_MACHINE(obj);
 
-pcms->smbus = value;
+pcms->smbus_enabled = value;
 }
 
 static bool pc_machine_get_sata(Object *obj, Error **errp)
 {
 PCMachineState *pcms = PC_MACHINE(obj);
 
-return pcms->sata;
+return pcms->sata_enabled;
 }
 
 static void pc_machine_set_sata(Object *obj, bool value, Error **errp)
 {
 PCMachineState *pcms = PC_MACHINE(obj);
 
-pcms->sata = value;
+pcms->sata_enabled = value;
 }
 
 static bool pc_machine_get_pit(Object *obj, Error **errp)
 {
 PCMachineState *pcms = PC_MACHINE(obj);
 
-return pcms->pit;
+return pcms->pit_enabled;
 }
 
 static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
 {
 PCMachineState *pcms = PC_MACHINE(obj);
 
-pcms->pit = value;
+pcms->pit_enabled = value;
 }
 
 static void pc_machine_initfn(Object *obj)
@@ -2233,9 +2233,9 @@ static void pc_machine_initfn(Object *obj)
 pcms->acpi_nvdimm_state.is_enabled = false;
 /* acpi build is enabled by default if machine supports it */
 pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
-pcms->smbus = true;
-pcms->sata = true;
-pcms->pit = true;
+pcms->smbus_enabled = true;
+pcms->sata_enabled = true;
+pcms->pit_enabled = true;
 }
 
 static void pc_machine_reset(void)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5e47528..7e87ef0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -237,7 +237,8 @@ static void pc_init1(MachineState *machine,
 
 /* init basic PC hardware */
 pc_basic_device_init(isa_bus, pcms->gsi, _state, true,
- (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit, 0x4);
+ (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit_enabled,
+ 0x4);
 
 pc_nic_init(isa_bus, pci_bus);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d606004..6e4bf1a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -230,13 +230,13 @@ static void pc_q35_init(MachineState *machine)
 
 /* init basic PC hardware */
 pc_basic_device_init(isa_bus, pcms->gsi, _state, !mc->no_floppy,
- (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit,
+ (pcms->vmport != ON_OFF_AUTO_ON), pcms->pit_enabled,
  0xff0104);
 
 /* connect pm stuff to lpc */
 ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms));
 
-if (pcms->sata) {
+if (pcms->sata_enabled) {
 /* ahci and SATA device, for q35 1 ahci controller is built-in */
 ahci = pci_create_simple_multifunction(host_bus,
PCI_DEVFN(ICH9_SATA1_DEV,
@@ -256,7 +256,7 @@ static void pc_q35_init(MachineState *machine)
 ehci_create_ich9_with_companions(host_bus, 0x1d);
 }
 
-if (pcms->smbus) {
+if (pcms->smbus_enabled) {
 /* TODO: Populate SPD eeprom data.  */
 smbus_eeprom_init(ich9_smb_init(host_bus,
 PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ef438bd..713aa33 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -50,9 +50,9 @@ struct PCMachineState {
 AcpiNVDIMMState acpi_nvdimm_state;
 
 bool acpi_build_enabled;
-bool smbus;
-bool sata;
-bool pit;
+bool smbus_enabled;
+bool sata_enabled;
+bool pit_enabled;
 
 /* RAM information (sizes, addresses, configuration): */
 ram_addr_t below_4g_mem_size, above_4g_mem_size;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 7/9] iothread: add iothread_by_id() API

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Encapsulate IOThread QOM object lookup so that callers don't need to
> know how and where IOThread objects live.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/sysemu/iothread.h | 1 +
>  iothread.c| 7 +++
>  2 files changed, 8 insertions(+)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 11/14] acpi: Add i2c serial bus CRS handling

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 hw/acpi/aml-build.c | 40 
 include/hw/acpi/aml-build.h | 18 ++
 2 files changed, 58 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 36a6cc4..36e2263 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1662,3 +1662,43 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
  "SLIT",
  table_data->len - slit_start, 1, NULL, NULL);
 }
+
+/* ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors */
+static Aml *aml_serial_bus_device(uint8_t serial_bus_type, uint8_t flags,
+  uint16_t type_flags,
+  uint8_t revid, uint16_t data_length,
+  uint16_t resource_source_len)
+{
+Aml *var = aml_alloc();
+uint16_t length = data_length + resource_source_len + 9;
+
+build_append_byte(var->buf, 0x8e); /* Serial Bus Connection Descriptor */
+build_append_int_noprefix(var->buf, length, sizeof(length));
+build_append_byte(var->buf, 1);/* Revision ID */
+build_append_byte(var->buf, 0);/* Resource Source Index */
+build_append_byte(var->buf, serial_bus_type); /* Serial Bus Type */
+build_append_byte(var->buf, flags); /* General Flags */
+build_append_int_noprefix(var->buf, type_flags, /* Type Specific Flags */
+  sizeof(type_flags));
+build_append_byte(var->buf, revid); /* Type Specification Revision ID */
+build_append_int_noprefix(var->buf, data_length, sizeof(data_length));
+
+return var;
+}
+
+/* ACPI 5.0: 6.4.3.8.2.1 I2C Serial Bus Connection Resource Descriptor */
+Aml *aml_i2c_serial_bus_device(uint16_t address, const char *resource_source)
+{
+uint16_t resource_source_len = strlen(resource_source) + 1;
+Aml *var = aml_serial_bus_device(AML_SERIAL_BUS_TYPE_I2C, 0, 0, 1,
+ 6, resource_source_len);
+
+/* Connection Speed.  Just set to 100K for now, it doesn't really matter. 
*/
+build_append_int_noprefix(var->buf, 10, 4);
+build_append_int_noprefix(var->buf, address, sizeof(address));
+
+/* This is a string, not a name, so just copy it directly in. */
+g_array_append_vals(var->buf, resource_source, resource_source_len);
+
+return var;
+}
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 88d0738..26fece8 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -214,6 +214,23 @@ struct AcpiBuildTables {
 BIOSLinker *linker;
 } AcpiBuildTables;
 
+/*
+ * ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors
+ * Serial Bus Type
+ */
+#define AML_SERIAL_BUS_TYPE_I2C  1
+#define AML_SERIAL_BUS_TYPE_SPI  2
+#define AML_SERIAL_BUS_TYPE_UART 3
+
+/*
+ * ACPI 5.0: 6.4.3.8.2 Serial Bus Connection Descriptors
+ * General Flags
+ */
+/* Slave Mode */
+#define AML_SERIAL_BUS_FLAG_MASTER_DEVICE   (1 << 0)
+/* Consumer/Producer */
+#define AML_SERIAL_BUS_FLAG_CONSUME_ONLY(1 << 1)
+
 /**
  * init_aml_allocator:
  *
@@ -338,6 +355,7 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
 Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
  uint8_t channel);
 Aml *aml_sleep(uint64_t msec);
+Aml *aml_i2c_serial_bus_device(uint16_t address, const char *resource_source);
 
 /* Block AML object primitives */
 Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
-- 
2.7.4

[Qemu-devel] [PATCH 14/14] pc: Add an SMB0 ACPI device to q35

From: Corey Minyard 

This is so I2C devices can be found in the ACPI namespace.  Currently
that's only IPMI, but devices can be easily added now.

Adding the devices required some PCI information, and the bus itself
to be added to the PCMachineState structure.

Note that this only works on Q35, the ACPI for PIIX4 is not capable
of handling an SMBus device.

Signed-off-by: Corey Minyard 
---
 hw/i386/acpi-build.c |  15 +++
 hw/i386/pc_piix.c|  12 ++--
 hw/i386/pc_q35.c |   9 +
 include/hw/i386/pc.h |   2 ++
 tests/acpi-test-data/q35/DSDT| Bin 7828 -> 7866 bytes
 tests/acpi-test-data/q35/DSDT.bridge | Bin 7845 -> 7883 bytes
 tests/acpi-test-data/q35/DSDT.cphp   | Bin 8291 -> 8329 bytes
 tests/acpi-test-data/q35/DSDT.ipmibt | Bin 7903 -> 7941 bytes
 tests/acpi-test-data/q35/DSDT.memhp  | Bin 9193 -> 9231 bytes
 9 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 84d82bc..9ec8268 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1859,6 +1859,18 @@ static Aml *build_q35_osc_method(void)
 return method;
 }
 
+static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
+{
+Aml *scope = aml_scope("_SB.PCI0");
+Aml *dev = aml_device("SMB0");
+
+aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
+aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
+build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
+aml_append(scope, dev);
+aml_append(table, scope);
+}
+
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker,
AcpiPmInfo *pm, AcpiMiscInfo *misc,
@@ -1910,6 +1922,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 build_q35_isa_bridge(dsdt);
 build_isa_devices_aml(dsdt);
 build_q35_pci0_int(dsdt);
+if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
+build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
+}
 }
 
 if (pcmc->legacy_cpu_hotplug) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7e87ef0..2a7ae72 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -276,15 +276,14 @@ static void pc_init1(MachineState *machine,
 
 if (pcmc->pci_enabled && acpi_enabled) {
 DeviceState *piix4_pm;
-I2CBus *smbus;
 
 smi_irq = qemu_allocate_irq(pc_acpi_smi_interrupt, first_cpu, 0);
 /* TODO: Populate SPD eeprom data.  */
-smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
-  pcms->gsi[9], smi_irq,
-  pc_machine_is_smm_enabled(pcms),
-  _pm);
-smbus_eeprom_init(smbus, 8, NULL, 0);
+pcms->smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
+pcms->gsi[9], smi_irq,
+pc_machine_is_smm_enabled(pcms),
+_pm);
+smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 
 object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
  TYPE_HOTPLUG_HANDLER,
@@ -489,6 +488,7 @@ static void pc_i440fx_2_6_machine_options(MachineClass *m)
 pc_i440fx_2_7_machine_options(m);
 pcmc->legacy_cpu_hotplug = true;
 pcmc->linuxboot_dma_enabled = false;
+pcmc->do_not_add_smb_acpi = true;
 SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6e4bf1a..0777bb3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -258,10 +258,10 @@ static void pc_q35_init(MachineState *machine)
 
 if (pcms->smbus_enabled) {
 /* TODO: Populate SPD eeprom data.  */
-smbus_eeprom_init(ich9_smb_init(host_bus,
-PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
-0xb100),
-  8, NULL, 0);
+pcms->smbus = ich9_smb_init(host_bus,
+PCI_DEVFN(ICH9_SMB_DEV, ICH9_SMB_FUNC),
+0xb100);
+smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
 }
 
 pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
@@ -358,6 +358,7 @@ static void pc_q35_2_6_machine_options(MachineClass *m)
 pc_q35_2_7_machine_options(m);
 pcmc->legacy_cpu_hotplug = true;
 pcmc->linuxboot_dma_enabled = false;
+pcmc->do_not_add_smb_acpi = true;
 SET_MACHINE_COMPAT(m, PC_COMPAT_2_6);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 713aa33..5b19d10 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -39,6 +39,7 @@ struct PCMachineState {
 HotplugHandler *acpi_dev;
 ISADevice *rtc;
 PCIBus *bus;
+I2CBus *smbus;
 FWCfgState *fw_cfg;
 qemu_irq *gsi;
 
@@ -122,6 +123,7 @@ struct

Re: [Qemu-devel] [PATCH v2 6/9] block: drop unused BlockDirtyBitmapState->aio_context field

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> The dirty bitmap actions in qmp_transaction have not used AioContext
> since the dirty bitmap locking discipline was introduced in commit
> 2119882c7eb7e2c612b24fc0c8d86f5887d6f1c3 ("block: introduce
> dirty_bitmap_mutex").  Remove the unused field.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  blockdev.c | 13 -
>  1 file changed, 13 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 8/9] blockdev: add x-blockdev-set-iothread testing command

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Currently there is no easy way for iotests to ensure that a BDS is bound
> to a particular IOThread.  Normally the virtio-blk device calls
> blk_set_aio_context() when dataplane is enabled during guest driver
> initialization.  This never happens in iotests since -machine
> accel=qtest means there is no guest activity (including device driver
> initialization).
> 
> This patch adds a QMP command to explicitly assign IOThreads in test
> cases.  See qapi/block-core.json for a description of the command.

The x- prefix is perfect for this.

> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  qapi/block-core.json | 36 
>  blockdev.c   | 41 +
>  2 files changed, 77 insertions(+)
> 

> +##
> +# @x-blockdev-set-iothread:
> +#
> +# Move @node and its children into the @iothread.  If @iothread is null then
> +# move @node and its children into the main loop.
> +#
> +# The node must not be attached to a BlockBackend.
> +#
> +# @node-name: the name of the block driver node
> +#
> +# @iothread: the name of the IOThread object or null for the main loop
> +#
> +# Note: this command is experimental and intended for test cases that need
> +# control over IOThreads only.

I'd place 'only' sooner; it fits better as 'intended only for ...'.

As a wording tweak is minor,
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 12/14] ipmi: Fix SSIF ACPI handling to use the right CRS

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 hw/acpi/ipmi-stub.c|  2 +-
 hw/acpi/ipmi.c | 13 +++--
 hw/i386/acpi-build.c   |  2 +-
 include/hw/acpi/ipmi.h |  2 +-
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/acpi/ipmi-stub.c b/hw/acpi/ipmi-stub.c
index 98b6dce..6c71d6c 100644
--- a/hw/acpi/ipmi-stub.c
+++ b/hw/acpi/ipmi-stub.c
@@ -9,6 +9,6 @@
 
 #include "hw/acpi/ipmi.h"
 
-void build_acpi_ipmi_devices(Aml *table, BusState *bus)
+void build_acpi_ipmi_devices(Aml *table, BusState *bus, const char *resource)
 {
 }
diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
index 651e2e9..96e48eb 100644
--- a/hw/acpi/ipmi.c
+++ b/hw/acpi/ipmi.c
@@ -13,7 +13,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ipmi.h"
 
-static Aml *aml_ipmi_crs(IPMIFwInfo *info)
+static Aml *aml_ipmi_crs(IPMIFwInfo *info, const char *resource)
 {
 Aml *crs = aml_resource_template();
 
@@ -48,7 +48,8 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
 info->register_spacing, info->register_length));
 break;
 case IPMI_MEMSPACE_SMBUS:
-aml_append(crs, aml_return(aml_int(info->base_address)));
+aml_append(crs, aml_i2c_serial_bus_device(info->base_address,
+  resource));
 break;
 default:
 abort();
@@ -61,7 +62,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
 return crs;
 }
 
-static Aml *aml_ipmi_device(IPMIFwInfo *info)
+static Aml *aml_ipmi_device(IPMIFwInfo *info, const char *resource)
 {
 Aml *dev;
 uint16_t version = ((info->ipmi_spec_major_revision << 8)
@@ -74,14 +75,14 @@ static Aml *aml_ipmi_device(IPMIFwInfo *info)
 aml_append(dev, aml_name_decl("_STR", aml_string("ipmi_%s",
  info->interface_name)));
 aml_append(dev, aml_name_decl("_UID", aml_int(info->uuid)));
-aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info)));
+aml_append(dev, aml_name_decl("_CRS", aml_ipmi_crs(info, resource)));
 aml_append(dev, aml_name_decl("_IFT", aml_int(info->interface_type)));
 aml_append(dev, aml_name_decl("_SRV", aml_int(version)));
 
 return dev;
 }
 
-void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
+void build_acpi_ipmi_devices(Aml *scope, BusState *bus, const char *resource)
 {
 
 BusChild *kid;
@@ -101,6 +102,6 @@ void build_acpi_ipmi_devices(Aml *scope, BusState *bus)
 iic = IPMI_INTERFACE_GET_CLASS(obj);
 memset(, 0, sizeof(info));
 iic->get_fwinfo(ii, );
-aml_append(scope, aml_ipmi_device());
+aml_append(scope, aml_ipmi_device(, resource));
 }
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 73519ab..84d82bc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1340,7 +1340,7 @@ static void build_isa_devices_aml(Aml *table)
 } else if (!obj) {
 error_report("No ISA bus, unable to define IPMI ACPI data");
 } else {
-build_acpi_ipmi_devices(scope, BUS(obj));
+build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
 }
 
 aml_append(table, scope);
diff --git a/include/hw/acpi/ipmi.h b/include/hw/acpi/ipmi.h
index ab2bb29..d68bc85 100644
--- a/include/hw/acpi/ipmi.h
+++ b/include/hw/acpi/ipmi.h
@@ -17,6 +17,6 @@
  * bus matches the given bus.  The resource is the ACPI resource that
  * contains the IPMI device, this is required for the I2C CRS.
  */
-void build_acpi_ipmi_devices(Aml *table, BusState *bus);
+void build_acpi_ipmi_devices(Aml *table, BusState *bus, const char *resource);
 
 #endif /* HW_ACPI_IPMI_H */
-- 
2.7.4

[Qemu-devel] [PATCH 07/14] i2c:pm_smbus: Add the ability to force block transfer enable

From: Corey Minyard 

The PIIX4 hardware has block transfer buffer always enabled in
the hardware, but the i801 does not.  Add a parameter to pm_smbus_init
to force on the block transfer so the PIIX4 handler can enable this
by default, as it was disabled by default before.

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/acpi/piix4.c   | 2 +-
 hw/i2c/pm_smbus.c | 5 -
 hw/i2c/smbus_ich9.c   | 2 +-
 hw/isa/vt82c686.c | 2 +-
 include/hw/i2c/pm_smbus.h | 2 +-
 5 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 18c1ffa..7018235 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -514,7 +514,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
 pci_conf[0x90] = s->smb_io_base | 1;
 pci_conf[0x91] = s->smb_io_base >> 8;
 pci_conf[0xd2] = 0x09;
-pm_smbus_init(DEVICE(dev), >smb);
+pm_smbus_init(DEVICE(dev), >smb, true);
 memory_region_set_enabled(>smb.io, pci_conf[0xd2] & 1);
 memory_region_add_subregion(pci_address_space_io(dev),
 s->smb_io_base, >smb.io);
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 2fb00d0..2afc5e1 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -401,11 +401,14 @@ const VMStateDescription pmsmb_vmstate = {
 }
 };
 
-void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
+void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
 {
 smb->op_done = true;
 smb->reset = pm_smbus_reset;
 smb->smbus = i2c_init_bus(parent, "i2c");
+if (force_aux_blk) {
+smb->smb_auxctl |= AUX_BLK;
+}
 memory_region_init_io(>io, OBJECT(parent), _smbus_ops, smb,
   "pm-smbus", 64);
 }
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index d029816..ce5ea9e 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -89,7 +89,7 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
 pci_set_byte(d->config + ICH9_SMB_HOSTC, 0);
 /* TODO bar0, bar1: 64bit BAR support*/
 
-pm_smbus_init(>qdev, >smb);
+pm_smbus_init(>qdev, >smb, false);
 pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
  >smb.io);
 }
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index c129985..78b7468 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -370,7 +370,7 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error 
**errp)
 pci_conf[0x90] = s->smb_io_base | 1;
 pci_conf[0x91] = s->smb_io_base >> 8;
 pci_conf[0xd2] = 0x90;
-pm_smbus_init(>dev.qdev, >smb);
+pm_smbus_init(>dev.qdev, >smb, false);
 memory_region_add_subregion(get_system_io(), s->smb_io_base, >smb.io);
 
 apm_init(dev, >apm, NULL, s);
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index cfe596f..471345e 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -33,7 +33,7 @@ typedef struct PMSMBus {
 bool op_done;
 } PMSMBus;
 
-void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
+void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
 
 extern const VMStateDescription pmsmb_vmstate;
 
-- 
2.7.4

[Qemu-devel] [PATCH 02/14] i2c:pm_smbus: Fix the semantics of block I2C transfers

From: Corey Minyard 

The I2C block transfer commands was not implemented correctly, it
read a length byte and such like it was an smbus transfer.

So fix the smbus_read_block() and smbus_write_block() functions
so they can properly handle I2C transfers, and normal SMBus
transfers (for upcoming changes).  Pass in a transfer size and
a bool to know whether to use the size byte (like SMBus) or use
the length given (like I2C).

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/i2c/pm_smbus.c  | 10 --
 hw/i2c/smbus.c | 37 -
 include/hw/i2c/smbus.h | 17 +++--
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 92c3aebd..679edbc 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -115,10 +115,16 @@ static void smb_transaction(PMSMBus *s)
 break;
 case PROT_I2C_BLOCK_DATA:
 if (read) {
-ret = smbus_read_block(bus, addr, cmd, s->smb_data);
+int xfersize = s->smb_data0;
+if (xfersize > sizeof(s->smb_data)) {
+xfersize = sizeof(s->smb_data);
+}
+ret = smbus_read_block(bus, addr, s->smb_data1, s->smb_data,
+   xfersize, false, true);
 goto data8;
 } else {
-ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0);
+ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0,
+false);
 goto done;
 }
 break;
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 2d1b79a..4b0e264 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -301,33 +301,42 @@ int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t 
command, uint16_t data)
 return 0;
 }
 
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data)
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+ int len, bool recv_len, bool send_cmd)
 {
-int len;
+int rlen;
 int i;
 
-if (i2c_start_transfer(bus, addr, 0)) {
-return -1;
+if (send_cmd) {
+if (i2c_start_transfer(bus, addr, 0)) {
+return -1;
+}
+i2c_send(bus, command);
 }
-i2c_send(bus, command);
 if (i2c_start_transfer(bus, addr, 1)) {
-i2c_end_transfer(bus);
+if (send_cmd) {
+i2c_end_transfer(bus);
+}
 return -1;
 }
-len = i2c_recv(bus);
-if (len > 32) {
-len = 0;
+if (recv_len) {
+rlen = i2c_recv(bus);
+} else {
+rlen = len;
 }
-for (i = 0; i < len; i++) {
+if (rlen > len) {
+rlen = 0;
+}
+for (i = 0; i < rlen; i++) {
 data[i] = i2c_recv(bus);
 }
 i2c_nack(bus);
 i2c_end_transfer(bus);
-return len;
+return rlen;
 }
 
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t 
*data,
-  int len)
+  int len, bool send_len)
 {
 int i;
 
@@ -338,7 +347,9 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t 
command, uint8_t *data,
 return -1;
 }
 i2c_send(bus, command);
-i2c_send(bus, len);
+if (send_len) {
+i2c_send(bus, len);
+}
 for (i = 0; i < len; i++) {
 i2c_send(bus, data[i]);
 }
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index 544bbc1..f1b8078 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -73,9 +73,22 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t 
command);
 int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
 int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
 int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t 
data);
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t 
*data);
+
+/*
+ * Do a block transfer from an I2C device.  If recv_len is set, then the
+ * first received byte is a length field and is used to know how much data
+ * to receive.  Otherwise receive "len" bytes.  If send_cmd is set, send
+ * the command byte first before receiving the data.
+ */
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+ int len, bool recv_len, bool send_cmd);
+
+/*
+ * Do a block transfer to an I2C device.  If send_len is set, send the
+ * "len" value before the data.
+ */
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t 
*data,
-  int len);
+  int len, bool send_len);
 
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
const uint8_t *eeprom_spd, int size);
-- 
2.7.4

[Qemu-devel] [PATCH 10/14] ipmi: Add an SMBus IPMI interface

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 default-configs/i386-softmmu.mak   |   1 +
 default-configs/x86_64-softmmu.mak |   1 +
 hw/ipmi/Makefile.objs  |   1 +
 hw/ipmi/smbus_ipmi.c   | 236 +
 4 files changed, 239 insertions(+)
 create mode 100644 hw/ipmi/smbus_ipmi.c

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 07aaf77..7791418 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -17,6 +17,7 @@ CONFIG_ISA_IPMI_KCS=y
 CONFIG_PCI_IPMI_KCS=y
 CONFIG_ISA_IPMI_BT=y
 CONFIG_PCI_IPMI_BT=y
+CONFIG_IPMI_SSIF=y
 CONFIG_SERIAL=y
 CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
diff --git a/default-configs/x86_64-softmmu.mak 
b/default-configs/x86_64-softmmu.mak
index 179ca35..7d8d673 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -17,6 +17,7 @@ CONFIG_ISA_IPMI_KCS=y
 CONFIG_PCI_IPMI_KCS=y
 CONFIG_ISA_IPMI_BT=y
 CONFIG_PCI_IPMI_BT=y
+CONFIG_IPMI_SSIF=y
 CONFIG_SERIAL=y
 CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
index 2d7f080..3cca10b 100644
--- a/hw/ipmi/Makefile.objs
+++ b/hw/ipmi/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-$(CONFIG_ISA_IPMI_KCS) += isa_ipmi_kcs.o
 common-obj-$(CONFIG_PCI_IPMI_KCS) += pci_ipmi_kcs.o
 common-obj-$(CONFIG_ISA_IPMI_BT) += isa_ipmi_bt.o
 common-obj-$(CONFIG_PCI_IPMI_BT) += pci_ipmi_bt.o
+common-obj-$(CONFIG_IPMI_SSIF) += smbus_ipmi.o
diff --git a/hw/ipmi/smbus_ipmi.c b/hw/ipmi/smbus_ipmi.c
new file mode 100644
index 000..75e26c8
--- /dev/null
+++ b/hw/ipmi/smbus_ipmi.c
@@ -0,0 +1,236 @@
+/*
+ * QEMU IPMI SMBus (SSIF) emulation
+ *
+ * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "hw/i2c/smbus.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/ipmi/ipmi.h"
+
+#define TYPE_SMBUS_IPMI "smbus-ipmi"
+#define SMBUS_IPMI(obj) OBJECT_CHECK(SMBusIPMIDevice, (obj), TYPE_SMBUS_IPMI)
+
+#define SSIF_IPMI_REQUEST   2
+#define SSIF_IPMI_MULTI_PART_REQUEST_START  6
+#define SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE 7
+#define SSIF_IPMI_RESPONSE  3
+#define SSIF_IPMI_MULTI_PART_RESPONSE_MIDDLE9
+
+typedef struct SMBusIPMIDevice {
+SMBusDevice parent;
+
+IPMIBmc *bmc;
+
+uint8_t outmsg[MAX_IPMI_MSG_SIZE];
+uint32_t outpos;
+uint32_t outlen;
+
+uint8_t inmsg[MAX_IPMI_MSG_SIZE];
+uint32_t inlen;
+
+/*
+ * This is a response number that we send with the command to make
+ * sure that the response matches the command.
+ */
+uint8_t waiting_rsp;
+
+uint32_t uuid;
+} SMBusIPMIDevice;
+
+static void smbus_ipmi_handle_event(IPMIInterface *ii)
+{
+/* No interrupts, so nothing to do here. */
+}
+
+static void smbus_ipmi_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
+  unsigned char *rsp, unsigned int rsp_len)
+{
+SMBusIPMIDevice *sid = SMBUS_IPMI(ii);
+
+if (sid->waiting_rsp == msg_id) {
+sid->waiting_rsp++;
+
+memcpy(sid->outmsg, rsp, rsp_len);
+sid->outlen = rsp_len;
+sid->outpos = 0;
+}
+}
+
+static void smbus_ipmi_set_atn(IPMIInterface *ii, int val, int irq)
+{
+/* This is where PEC would go. */
+}
+
+static void smbus_ipmi_set_irq_enable(IPMIInterface *ii, int val)
+{
+}
+
+static void ipmi_quick_cmd(SMBusDevice *dev, uint8_t read)
+{
+}
+
+static void ipmi_send_byte(SMBusDevice *dev, uint8_t val)
+{
+}
+
+static uint8_t ipmi_receive_byte(SMBusDevice *dev)
+{
+SMBusIPMIDevice *sid = SMBUS_IPMI(dev);
+
+if (sid->outpos >= sid->outlen) {
+return 0;
+}
+
+return sid->outmsg[sid->outpos++];
+}
+
+static void ipmi_write_data(SMBusDevice *dev, uint8_t

[Qemu-devel] [PATCH 03/14] i2c:pm_smbus: Make the I2C block read command read-only

From: Corey Minyard 

It did have write capability, but the manual says the behavior
with write enabled is undefined.  So just set an error in this
case.

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/i2c/pm_smbus.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 679edbc..e7a8e92 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -56,7 +56,7 @@
 #define PROT_WORD_DATA  3
 #define PROT_PROC_CALL  4
 #define PROT_BLOCK_DATA 5
-#define PROT_I2C_BLOCK_DATA 6
+#define PROT_I2C_BLOCK_READ 6
 
 /*#define DEBUG*/
 
@@ -113,7 +113,7 @@ static void smb_transaction(PMSMBus *s)
 goto done;
 }
 break;
-case PROT_I2C_BLOCK_DATA:
+case PROT_I2C_BLOCK_READ:
 if (read) {
 int xfersize = s->smb_data0;
 if (xfersize > sizeof(s->smb_data)) {
@@ -123,9 +123,8 @@ static void smb_transaction(PMSMBus *s)
xfersize, false, true);
 goto data8;
 } else {
-ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0,
-false);
-goto done;
+/* The manual says the behavior is undefined, just set DEV_ERR. */
+goto error;
 }
 break;
 default:
-- 
2.7.4

[Qemu-devel] [PATCH 09/14] i2c: Add vmstate handling to the smbus eeprom

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 hw/i2c/smbus_eeprom.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index b13ec0f..089005d 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -29,6 +29,8 @@
 
 //#define DEBUG
 
+#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
+
 typedef struct SMBusEEPROMDevice {
 SMBusDevice smbusdev;
 void *data;
@@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t 
cmd, int n)
 return eeprom_receive_byte(dev);
 }
 
+static const VMStateDescription vmstate_smbus_eeprom = {
+.name = TYPE_SMBUS_EEPROM_DEVICE,
+.version_id = 1,
+.minimum_version_id = 1,
+.fields  = (VMStateField[]) {
+VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
+VMSTATE_UINT8(offset, SMBusEEPROMDevice),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static int smbus_eeprom_initfn(SMBusDevice *dev)
 {
 SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
@@ -122,12 +135,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, 
void *data)
 sc->write_data = eeprom_write_data;
 sc->read_data = eeprom_read_data;
 dc->props = smbus_eeprom_properties;
+dc->vmsd = _smbus_eeprom;
 /* Reason: pointer property "data" */
 dc->user_creatable = false;
 }
 
 static const TypeInfo smbus_eeprom_info = {
-.name  = "smbus-eeprom",
+.name  = TYPE_SMBUS_EEPROM_DEVICE,
 .parent= TYPE_SMBUS_DEVICE,
 .instance_size = sizeof(SMBusEEPROMDevice),
 .class_init= smbus_eeprom_class_initfn,
-- 
2.7.4

[Qemu-devel] [PATCH 05/14] i2c:pm_smbus: Fix state transfer

From: Corey Minyard 

Transfer the state information for the SMBus registers and
internal data so it will work on a VM transfer.

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/acpi/piix4.c   |  1 +
 hw/i2c/pm_smbus.c | 19 +++
 hw/i2c/smbus_ich9.c   |  3 ++-
 include/hw/i2c/pm_smbus.h |  2 ++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index a0fb1ce..18c1ffa 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -321,6 +321,7 @@ static const VMStateDescription vmstate_acpi = {
 VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
 VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
 VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
+VMSTATE_STRUCT(smb, PIIX4PMState, 1, pmsmb_vmstate, PMSMBus),
 VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
 VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
 VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 686d411..eb2df4d 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -370,6 +370,25 @@ static const MemoryRegionOps pm_smbus_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+const VMStateDescription pmsmb_vmstate = {
+.name = "pmsmb",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT8(smb_stat, PMSMBus),
+VMSTATE_UINT8(smb_ctl, PMSMBus),
+VMSTATE_UINT8(smb_cmd, PMSMBus),
+VMSTATE_UINT8(smb_addr, PMSMBus),
+VMSTATE_UINT8(smb_data0, PMSMBus),
+VMSTATE_UINT8(smb_data1, PMSMBus),
+VMSTATE_VBUFFER_UINT32(smb_data, PMSMBus, 1, NULL, smb_index),
+VMSTATE_UINT8(smb_auxctl, PMSMBus),
+VMSTATE_BOOL(i2c_enable, PMSMBus),
+VMSTATE_BOOL(op_done, PMSMBus),
+VMSTATE_END_OF_LIST()
+}
+};
+
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
 {
 smb->op_done = true;
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 59e165a..706b9ec 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -49,7 +49,8 @@ static const VMStateDescription vmstate_ich9_smbus = {
 .version_id = 1,
 .minimum_version_id = 1,
 .fields = (VMStateField[]) {
-VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
+VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+VMSTATE_STRUCT(smb, ICH9SMBState, 1, pmsmb_vmstate, PMSMBus),
 VMSTATE_END_OF_LIST()
 }
 };
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 99d5489..b1e1970 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -33,4 +33,6 @@ typedef struct PMSMBus {
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
 
+extern const VMStateDescription pmsmb_vmstate;
+
 #endif /* PM_SMBUS_H */
-- 
2.7.4

[Qemu-devel] [PATCH 08/14] i2c: Add an SMBus vmstate structure

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 hw/i2c/smbus.c | 14 ++
 include/hw/i2c/smbus.h | 18 +++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 4b0e264..e15f3f2 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -357,6 +357,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t 
command, uint8_t *data,
 return 0;
 }
 
+const VMStateDescription vmstate_smbus_device = {
+.name = TYPE_SMBUS_DEVICE,
+.version_id = 1,
+.minimum_version_id = 1,
+.fields  = (VMStateField[]) {
+VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
+VMSTATE_INT32(mode, SMBusDevice),
+VMSTATE_INT32(data_len, SMBusDevice),
+VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
+VMSTATE_UINT8(command, SMBusDevice),
+VMSTATE_END_OF_LIST()
+}
+};
+
 static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
 I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index f1b8078..7794026 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -54,14 +54,16 @@ typedef struct SMBusDeviceClass
 uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
 } SMBusDeviceClass;
 
+#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
+
 struct SMBusDevice {
 /* The SMBus protocol is implemented on top of I2C.  */
 I2CSlave i2c;
 
 /* Remaining fields for internal use only.  */
-int mode;
-int data_len;
-uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
+int32_t mode;
+int32_t data_len;
+uint8_t data_buf[SMBUS_DATA_MAX_LEN];
 uint8_t command;
 };
 
@@ -93,4 +95,14 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t 
command, uint8_t *data,
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
const uint8_t *eeprom_spd, int size);
 
+extern const VMStateDescription vmstate_smbus_device;
+
+#define VMSTATE_SMBUS_DEVICE(_field, _state) {   \
+.name   = (stringify(_field)),   \
+.size   = sizeof(SMBusDevice),   \
+.vmsd   = _smbus_device, \
+.flags  = VMS_STRUCT,\
+.offset = vmstate_offset_value(_state, _field, SMBusDevice), \
+}
+
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH 01/14] i2c:pm_smbus: Clean up some style issues

From: Corey Minyard 

Fix some spacing issues, remove extraneous comments, add some
defines instead of hard-coding numbers.

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/i2c/pm_smbus.c | 58 ---
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 6fc3923..92c3aebd 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -23,8 +23,6 @@
 #include "hw/i2c/pm_smbus.h"
 #include "hw/i2c/smbus.h"
 
-/* no save/load? */
-
 #define SMBHSTSTS   0x00
 #define SMBHSTCNT   0x02
 #define SMBHSTCMD   0x03
@@ -33,19 +31,34 @@
 #define SMBHSTDAT1  0x06
 #define SMBBLKDAT   0x07
 
-#define STS_HOST_BUSY   (1)
-#define STS_INTR(1<<1)
-#define STS_DEV_ERR (1<<2)
-#define STS_BUS_ERR (1<<3)
-#define STS_FAILED  (1<<4)
-#define STS_SMBALERT(1<<5)
-#define STS_INUSE_STS   (1<<6)
-#define STS_BYTE_DONE   (1<<7)
+#define STS_HOST_BUSY   (1 << 0)
+#define STS_INTR(1 << 1)
+#define STS_DEV_ERR (1 << 2)
+#define STS_BUS_ERR (1 << 3)
+#define STS_FAILED  (1 << 4)
+#define STS_SMBALERT(1 << 5)
+#define STS_INUSE_STS   (1 << 6)
+#define STS_BYTE_DONE   (1 << 7)
 /* Signs of successfully transaction end :
 *  ByteDoneStatus = 1 (STS_BYTE_DONE) and INTR = 1 (STS_INTR )
 */
 
-//#define DEBUG
+#define CTL_INTREN  (1 << 0)
+#define CTL_KILL(1 << 1)
+#define CTL_LAST_BYTE   (1 << 5)
+#define CTL_START   (1 << 6)
+#define CTL_PEC_EN  (1 << 7)
+#define CTL_RETURN_MASK 0x1f
+
+#define PROT_QUICK  0
+#define PROT_BYTE   1
+#define PROT_BYTE_DATA  2
+#define PROT_WORD_DATA  3
+#define PROT_PROC_CALL  4
+#define PROT_BLOCK_DATA 5
+#define PROT_I2C_BLOCK_DATA 6
+
+/*#define DEBUG*/
 
 #ifdef DEBUG
 # define SMBUS_DPRINTF(format, ...) printf(format, ## __VA_ARGS__)
@@ -68,11 +81,12 @@ static void smb_transaction(PMSMBus *s)
 if ((s->smb_stat & STS_DEV_ERR) != 0)  {
 goto error;
 }
+
 switch(prot) {
-case 0x0:
+case PROT_QUICK:
 ret = smbus_quick_command(bus, addr, read);
 goto done;
-case 0x1:
+case PROT_BYTE:
 if (read) {
 ret = smbus_receive_byte(bus, addr);
 goto data8;
@@ -80,7 +94,7 @@ static void smb_transaction(PMSMBus *s)
 ret = smbus_send_byte(bus, addr, cmd);
 goto done;
 }
-case 0x2:
+case PROT_BYTE_DATA:
 if (read) {
 ret = smbus_read_byte(bus, addr, cmd);
 goto data8;
@@ -89,16 +103,17 @@ static void smb_transaction(PMSMBus *s)
 goto done;
 }
 break;
-case 0x3:
+case PROT_WORD_DATA:
 if (read) {
 ret = smbus_read_word(bus, addr, cmd);
 goto data16;
 } else {
-ret = smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) | 
s->smb_data0);
+ret = smbus_write_word(bus, addr, cmd,
+   (s->smb_data1 << 8) | s->smb_data0);
 goto done;
 }
 break;
-case 0x5:
+case PROT_I2C_BLOCK_DATA:
 if (read) {
 ret = smbus_read_block(bus, addr, cmd, s->smb_data);
 goto data8;
@@ -149,8 +164,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, 
uint64_t val,
 break;
 case SMBHSTCNT:
 s->smb_ctl = val;
-if (val & 0x40)
+if (s->smb_ctl & CTL_START) {
 smb_transaction(s);
+}
 break;
 case SMBHSTCMD:
 s->smb_cmd = val;
@@ -185,7 +201,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, 
unsigned width)
 break;
 case SMBHSTCNT:
 s->smb_index = 0;
-val = s->smb_ctl & 0x1f;
+val = s->smb_ctl & CTL_RETURN_MASK;
 break;
 case SMBHSTCMD:
 val = s->smb_cmd;
@@ -208,7 +224,9 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, 
unsigned width)
 val = 0;
 break;
 }
-SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n", addr, 
val);
+SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n",
+  addr, val);
+
 return val;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 06/14] i2c:pm_smbus: Add interrupt handling

From: Corey Minyard 

Add the necessary code so that interrupts actually work from
the pm_smbus device.

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/i2c/pm_smbus.c | 14 +-
 hw/i2c/smbus_ich9.c   | 17 +
 include/hw/i2c/pm_smbus.h |  2 ++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index eb2df4d..2fb00d0 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -205,6 +205,12 @@ error:
 return;
 }
 
+static bool
+smb_irq_value(PMSMBus *s)
+{
+return ((s->smb_stat & ~STS_HOST_BUSY) != 0) && (s->smb_ctl & CTL_INTREN);
+}
+
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
   unsigned width)
 {
@@ -300,7 +306,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, 
uint64_t val,
 }
 
  out:
-return;
+if (s->set_irq) {
+s->set_irq(s, smb_irq_value(s));
+}
 }
 
 static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
@@ -352,6 +360,10 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr 
addr, unsigned width)
 SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n",
   addr, val);
 
+if (s->set_irq) {
+s->set_irq(s, smb_irq_value(s));
+}
+
 return val;
 }
 
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 706b9ec..d029816 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -41,6 +41,8 @@
 typedef struct ICH9SMBState {
 PCIDevice dev;
 
+bool irq_enabled;
+
 PMSMBus smb;
 } ICH9SMBState;
 
@@ -50,6 +52,7 @@ static const VMStateDescription vmstate_ich9_smbus = {
 .minimum_version_id = 1,
 .fields = (VMStateField[]) {
 VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+VMSTATE_BOOL(irq_enabled, ICH9SMBState),
 VMSTATE_STRUCT(smb, ICH9SMBState, 1, pmsmb_vmstate, PMSMBus),
 VMSTATE_END_OF_LIST()
 }
@@ -111,11 +114,25 @@ static void ich9_smb_class_init(ObjectClass *klass, void 
*data)
 dc->user_creatable = false;
 }
 
+static void ich9_smb_set_irq(PMSMBus *pmsmb, bool enabled)
+{
+ICH9SMBState *s = pmsmb->opaque;
+
+if (enabled == s->irq_enabled) {
+return;
+}
+
+s->irq_enabled = enabled;
+pci_set_irq(>dev, enabled);
+}
+
 I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
 {
 PCIDevice *d =
 pci_create_simple_multifunction(bus, devfn, true, 
TYPE_ICH9_SMB_DEVICE);
 ICH9SMBState *s = ICH9_SMB_DEVICE(d);
+s->smb.set_irq = ich9_smb_set_irq;
+s->smb.opaque = s;
 return s->smb.smbus;
 }
 
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index b1e1970..cfe596f 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -23,6 +23,8 @@ typedef struct PMSMBus {
 
 /* Set by the user. */
 bool i2c_enable;
+void (*set_irq)(struct PMSMBus *s, bool enabled);
+void *opaque;
 
 /* Internally used by pm_smbus. */
 
-- 
2.7.4

[Qemu-devel] [PATCH 00/13] pm_smbus fixes and and IPMI SMBus device

The pm_smbus code has a few issues, and it's missing a lot of the
device capabilities required by IPMI.  This has been submitted
before, but it has some new things, like vmstate handling for the
smbus devices.

[Qemu-devel] [PATCH 04/14] i2c:pm_smbus: Add block transfer capability

From: Corey Minyard 

There was no block transfer code in pm_smbus.c, and it is needed
for some devices.  So add it.

This adds both byte-by-byte block transfers and buffered block
transfers.

Signed-off-by: Corey Minyard 
Cc: Michael S. Tsirkin 
Cc: Paolo Bonzini 
---
 hw/i2c/pm_smbus.c | 151 ++
 hw/i2c/smbus_ich9.c   |   8 ++-
 include/hw/i2c/pm_smbus.h |  20 +-
 3 files changed, 164 insertions(+), 15 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index e7a8e92..686d411 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -30,6 +30,7 @@
 #define SMBHSTDAT0  0x05
 #define SMBHSTDAT1  0x06
 #define SMBBLKDAT   0x07
+#define SMBAUXCTL   0x0d
 
 #define STS_HOST_BUSY   (1 << 0)
 #define STS_INTR(1 << 1)
@@ -58,6 +59,10 @@
 #define PROT_BLOCK_DATA 5
 #define PROT_I2C_BLOCK_READ 6
 
+#define AUX_PEC   (1 << 0)
+#define AUX_BLK   (1 << 1)
+#define AUX_MASK  0x3
+
 /*#define DEBUG*/
 
 #ifdef DEBUG
@@ -127,6 +132,51 @@ static void smb_transaction(PMSMBus *s)
 goto error;
 }
 break;
+case PROT_BLOCK_DATA:
+if (read) {
+ret = smbus_read_block(bus, addr, cmd, s->smb_data,
+   sizeof(s->smb_data), !s->i2c_enable,
+   !s->i2c_enable);
+if (ret < 0) {
+goto error;
+}
+s->smb_index = 0;
+s->op_done = false;
+if (s->smb_auxctl & AUX_BLK) {
+s->smb_stat |= STS_INTR;
+} else {
+s->smb_blkdata = s->smb_data[0];
+s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
+}
+s->smb_data0 = ret;
+goto out;
+} else {
+if (s->smb_auxctl & AUX_BLK) {
+if (s->smb_index != s->smb_data0) {
+s->smb_index = 0;
+goto error;
+}
+/* Data is already all written to the queue, just do
+   the operation. */
+s->smb_index = 0;
+ret = smbus_write_block(bus, addr, cmd, s->smb_data,
+s->smb_data0, !s->i2c_enable);
+if (ret < 0) {
+goto error;
+}
+s->op_done = true;
+s->smb_stat |= STS_INTR;
+s->smb_stat &= ~STS_HOST_BUSY;
+} else {
+s->op_done = false;
+s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
+s->smb_data[0] = s->smb_blkdata;
+s->smb_index = 0;
+ret = 0;
+}
+goto out;
+}
+break;
 default:
 goto error;
 }
@@ -146,13 +196,13 @@ done:
 if (ret < 0) {
 goto error;
 }
-s->smb_stat |= STS_BYTE_DONE | STS_INTR;
+s->smb_stat |= STS_INTR;
+out:
 return;
 
 error:
 s->smb_stat |= STS_DEV_ERR;
 return;
-
 }
 
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
@@ -164,14 +214,61 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, 
uint64_t val,
   " val=0x%02" PRIx64 "\n", addr, val);
 switch(addr) {
 case SMBHSTSTS:
-s->smb_stat = (~(val & 0xff)) & s->smb_stat;
-s->smb_index = 0;
+s->smb_stat &= ~(val & ~STS_HOST_BUSY);
+if (!s->op_done && !(s->smb_auxctl & AUX_BLK)) {
+uint8_t read = s->smb_addr & 0x01;
+
+s->smb_index++;
+if (!read && s->smb_index == s->smb_data0) {
+uint8_t prot = (s->smb_ctl >> 2) & 0x07;
+uint8_t cmd = s->smb_cmd;
+uint8_t addr = s->smb_addr >> 1;
+int ret;
+
+if (prot == PROT_I2C_BLOCK_READ) {
+s->smb_stat |= STS_DEV_ERR;
+goto out;
+}
+
+ret = smbus_write_block(s->smbus, addr, cmd, s->smb_data,
+s->smb_data0, !s->i2c_enable);
+if (ret < 0) {
+s->smb_stat |= STS_DEV_ERR;
+goto out;
+}
+s->op_done = true;
+s->smb_stat |= STS_INTR;
+s->smb_stat &= ~STS_HOST_BUSY;
+} else if (!read) {
+s->smb_data[s->smb_index] = s->smb_blkdata;
+s->smb_stat |= STS_BYTE_DONE;
+} else if (s->smb_ctl & CTL_LAST_BYTE) {
+s->op_done = true;
+s->smb_blkdata = s->smb_data[s->smb_index];
+s->smb_index = 0;
+s->smb_stat |= STS_INTR;
+s->smb_stat &= ~STS_HOST_BUSY;
+} else {
+s->smb_blkdata = s->smb_data[s->smb_index];
+

[Qemu-devel] [PATCH 4/7] ipmi: Split out BT-specific code from ISA BT code

From: Corey Minyard 

Get ready for PCI and other BT interfaces.

Signed-off-by: Corey Minyard 
---
 hw/ipmi/Makefile.objs |   2 +-
 hw/ipmi/ipmi_bt.c | 399 +++
 hw/ipmi/isa_ipmi_bt.c | 418 +++---
 include/hw/ipmi/ipmi_bt.h |  72 
 4 files changed, 493 insertions(+), 398 deletions(-)
 create mode 100644 hw/ipmi/ipmi_bt.c
 create mode 100644 include/hw/ipmi/ipmi_bt.h

diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
index 6835d2f..4ffa45a 100644
--- a/hw/ipmi/Makefile.objs
+++ b/hw/ipmi/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-$(CONFIG_IPMI) += ipmi.o ipmi_kcs.o
+common-obj-$(CONFIG_IPMI) += ipmi.o ipmi_kcs.o ipmi_bt.o
 common-obj-$(CONFIG_IPMI_LOCAL) += ipmi_bmc_sim.o
 common-obj-$(CONFIG_IPMI_EXTERN) += ipmi_bmc_extern.o
 common-obj-$(CONFIG_ISA_IPMI_KCS) += isa_ipmi_kcs.o
diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c
new file mode 100644
index 000..e07e10a
--- /dev/null
+++ b/hw/ipmi/ipmi_bt.c
@@ -0,0 +1,399 @@
+/*
+ * QEMU IPMI BT emulation
+ *
+ * Copyright (c) 2015 Corey Minyard, MontaVista Software, LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/ipmi/ipmi_bt.h"
+
+/* Control register */
+#define IPMI_BT_CLR_WR_BIT 0
+#define IPMI_BT_CLR_RD_BIT 1
+#define IPMI_BT_H2B_ATN_BIT2
+#define IPMI_BT_B2H_ATN_BIT3
+#define IPMI_BT_SMS_ATN_BIT4
+#define IPMI_BT_HBUSY_BIT  6
+#define IPMI_BT_BBUSY_BIT  7
+
+#define IPMI_BT_GET_CLR_WR(d)  (((d) >> IPMI_BT_CLR_WR_BIT) & 0x1)
+
+#define IPMI_BT_GET_CLR_RD(d)  (((d) >> IPMI_BT_CLR_RD_BIT) & 0x1)
+
+#define IPMI_BT_GET_H2B_ATN(d) (((d) >> IPMI_BT_H2B_ATN_BIT) & 0x1)
+
+#define IPMI_BT_B2H_ATN_MASK   (1 << IPMI_BT_B2H_ATN_BIT)
+#define IPMI_BT_GET_B2H_ATN(d) (((d) >> IPMI_BT_B2H_ATN_BIT) & 0x1)
+#define IPMI_BT_SET_B2H_ATN(d, v)  ((d) = (((d) & ~IPMI_BT_B2H_ATN_MASK) | \
+(!!(v) << IPMI_BT_B2H_ATN_BIT)))
+
+#define IPMI_BT_SMS_ATN_MASK   (1 << IPMI_BT_SMS_ATN_BIT)
+#define IPMI_BT_GET_SMS_ATN(d) (((d) >> IPMI_BT_SMS_ATN_BIT) & 0x1)
+#define IPMI_BT_SET_SMS_ATN(d, v)  ((d) = (((d) & ~IPMI_BT_SMS_ATN_MASK) | \
+(!!(v) << IPMI_BT_SMS_ATN_BIT)))
+
+#define IPMI_BT_HBUSY_MASK (1 << IPMI_BT_HBUSY_BIT)
+#define IPMI_BT_GET_HBUSY(d)   (((d) >> IPMI_BT_HBUSY_BIT) & 0x1)
+#define IPMI_BT_SET_HBUSY(d, v)((d) = (((d) & ~IPMI_BT_HBUSY_MASK) | \
+   (!!(v) << IPMI_BT_HBUSY_BIT)))
+
+#define IPMI_BT_BBUSY_MASK (1 << IPMI_BT_BBUSY_BIT)
+#define IPMI_BT_SET_BBUSY(d, v)((d) = (((d) & ~IPMI_BT_BBUSY_MASK) | \
+   (!!(v) << IPMI_BT_BBUSY_BIT)))
+
+
+/* Mask register */
+#define IPMI_BT_B2H_IRQ_EN_BIT 0
+#define IPMI_BT_B2H_IRQ_BIT1
+
+#define IPMI_BT_B2H_IRQ_EN_MASK  (1 << IPMI_BT_B2H_IRQ_EN_BIT)
+#define IPMI_BT_GET_B2H_IRQ_EN(d)(((d) >> IPMI_BT_B2H_IRQ_EN_BIT) & 0x1)
+#define IPMI_BT_SET_B2H_IRQ_EN(d, v) ((d) = (((d) & ~IPMI_BT_B2H_IRQ_EN_MASK) 
|\
+(!!(v) << IPMI_BT_B2H_IRQ_EN_BIT)))
+
+#define IPMI_BT_B2H_IRQ_MASK (1 << IPMI_BT_B2H_IRQ_BIT)
+#define IPMI_BT_GET_B2H_IRQ(d)   (((d) >> IPMI_BT_B2H_IRQ_BIT) & 0x1)
+#define IPMI_BT_SET_B2H_IRQ(d, v)((d) = (((d) & ~IPMI_BT_B2H_IRQ_MASK) | \
+(!!(v) << IPMI_BT_B2H_IRQ_BIT)))
+
+#define IPMI_CMD_GET_BT_INTF_CAP0x36
+
+static void ipmi_bt_raise_irq(IPMIBT *ib)
+{
+if (ib->use_irq && ib->irqs_enabled && ib->raise_irq) {
+ib->raise_irq(ib);
+}
+}
+
+static void ipmi_bt_lower_irq(IPMIBT *ib)
+{
+if (ib->lower_irq) {
+ib->lower_irq(ib);
+}
+}
+
+static void

Re: [Qemu-devel] [PATCH 0/7] Add PCI IPMI interfaces

On 12/07/2017 03:34 PM, miny...@acm.org wrote:
> I had to do some testing, so I went ahead and did this.  Most of the
> changes are re-arranging and splitting up code to separate out the
> KCS/BT code from the ISA-specific code.
> 
> There are also some migration fixes here.
> 
> This series requires the previous series: Small IPMI (and other)
> fixes

Let patchew know about the dependency:

Based-on: <1512682213-4354-1-git-send-email-miny...@acm.org>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 3/7] ipmi: Split out KCS-specific code from ISA KCS code

From: Corey Minyard 

Get ready for PCI and other KCS interfaces.

Signed-off-by: Corey Minyard 
---
 hw/ipmi/Makefile.objs  |   2 +-
 hw/ipmi/ipmi_kcs.c | 375 ++
 hw/ipmi/isa_ipmi_kcs.c | 394 +++--
 include/hw/ipmi/ipmi_kcs.h |  75 +
 4 files changed, 473 insertions(+), 373 deletions(-)
 create mode 100644 hw/ipmi/ipmi_kcs.c
 create mode 100644 include/hw/ipmi/ipmi_kcs.h

diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
index 1b422bb..6835d2f 100644
--- a/hw/ipmi/Makefile.objs
+++ b/hw/ipmi/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-$(CONFIG_IPMI) += ipmi.o
+common-obj-$(CONFIG_IPMI) += ipmi.o ipmi_kcs.o
 common-obj-$(CONFIG_IPMI_LOCAL) += ipmi_bmc_sim.o
 common-obj-$(CONFIG_IPMI_EXTERN) += ipmi_bmc_extern.o
 common-obj-$(CONFIG_ISA_IPMI_KCS) += isa_ipmi_kcs.o
diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
new file mode 100644
index 000..cb22dee
--- /dev/null
+++ b/hw/ipmi/ipmi_kcs.c
@@ -0,0 +1,375 @@
+/*
+ * QEMU IPMI KCS emulation
+ *
+ * Copyright (c) 2015,2017 Corey Minyard, MontaVista Software, LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/ipmi/ipmi_kcs.h"
+
+#define IPMI_KCS_OBF_BIT0
+#define IPMI_KCS_IBF_BIT1
+#define IPMI_KCS_SMS_ATN_BIT2
+#define IPMI_KCS_CD_BIT 3
+
+#define IPMI_KCS_OBF_MASK  (1 << IPMI_KCS_OBF_BIT)
+#define IPMI_KCS_GET_OBF(d)(((d) >> IPMI_KCS_OBF_BIT) & 0x1)
+#define IPMI_KCS_SET_OBF(d, v) (d) = (((d) & ~IPMI_KCS_OBF_MASK) | \
+   (((v) & 1) << IPMI_KCS_OBF_BIT))
+#define IPMI_KCS_IBF_MASK  (1 << IPMI_KCS_IBF_BIT)
+#define IPMI_KCS_GET_IBF(d)(((d) >> IPMI_KCS_IBF_BIT) & 0x1)
+#define IPMI_KCS_SET_IBF(d, v) (d) = (((d) & ~IPMI_KCS_IBF_MASK) | \
+   (((v) & 1) << IPMI_KCS_IBF_BIT))
+#define IPMI_KCS_SMS_ATN_MASK  (1 << IPMI_KCS_SMS_ATN_BIT)
+#define IPMI_KCS_GET_SMS_ATN(d)(((d) >> IPMI_KCS_SMS_ATN_BIT) & 0x1)
+#define IPMI_KCS_SET_SMS_ATN(d, v) (d) = (((d) & ~IPMI_KCS_SMS_ATN_MASK) | \
+   (((v) & 1) << IPMI_KCS_SMS_ATN_BIT))
+#define IPMI_KCS_CD_MASK   (1 << IPMI_KCS_CD_BIT)
+#define IPMI_KCS_GET_CD(d) (((d) >> IPMI_KCS_CD_BIT) & 0x1)
+#define IPMI_KCS_SET_CD(d, v)  (d) = (((d) & ~IPMI_KCS_CD_MASK) | \
+   (((v) & 1) << IPMI_KCS_CD_BIT))
+
+#define IPMI_KCS_IDLE_STATE0
+#define IPMI_KCS_READ_STATE1
+#define IPMI_KCS_WRITE_STATE   2
+#define IPMI_KCS_ERROR_STATE   3
+
+#define IPMI_KCS_GET_STATE(d)(((d) >> 6) & 0x3)
+#define IPMI_KCS_SET_STATE(d, v) ((d) = ((d) & ~0xc0) | (((v) & 0x3) << 6))
+
+#define IPMI_KCS_ABORT_STATUS_CMD   0x60
+#define IPMI_KCS_WRITE_START_CMD0x61
+#define IPMI_KCS_WRITE_END_CMD  0x62
+#define IPMI_KCS_READ_CMD   0x68
+
+#define IPMI_KCS_STATUS_NO_ERR  0x00
+#define IPMI_KCS_STATUS_ABORTED_ERR 0x01
+#define IPMI_KCS_STATUS_BAD_CC_ERR  0x02
+#define IPMI_KCS_STATUS_LENGTH_ERR  0x06
+
+static void ipmi_kcs_raise_irq(IPMIKCS *ik)
+{
+if (ik->use_irq && ik->irqs_enabled && ik->raise_irq) {
+ik->raise_irq(ik);
+}
+}
+
+static void ipmi_kcs_lower_irq(IPMIKCS *ik)
+{
+if (ik->lower_irq) {
+ik->lower_irq(ik);
+}
+}
+
+#define SET_OBF() \
+do {  \
+IPMI_KCS_SET_OBF(ik->status_reg, 1);  \
+if (!ik->obf_irq_set) {   \
+ik->obf_irq_set = 1;  \
+if (!ik->atn_irq_set) {   \
+

[Qemu-devel] [PATCH 7/7] ipmi: Add PCI IPMI interfaces

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 default-configs/i386-softmmu.mak   |   2 +
 default-configs/x86_64-softmmu.mak |   2 +
 hw/ipmi/Makefile.objs  |   2 +
 hw/ipmi/pci_ipmi_bt.c  | 145 +
 hw/ipmi/pci_ipmi_kcs.c | 145 +
 include/hw/pci/pci.h   |   1 +
 6 files changed, 297 insertions(+)
 create mode 100644 hw/ipmi/pci_ipmi_bt.c
 create mode 100644 hw/ipmi/pci_ipmi_kcs.c

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 95ac4b4..07aaf77 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -14,7 +14,9 @@ CONFIG_IPMI=y
 CONFIG_IPMI_LOCAL=y
 CONFIG_IPMI_EXTERN=y
 CONFIG_ISA_IPMI_KCS=y
+CONFIG_PCI_IPMI_KCS=y
 CONFIG_ISA_IPMI_BT=y
+CONFIG_PCI_IPMI_BT=y
 CONFIG_SERIAL=y
 CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
diff --git a/default-configs/x86_64-softmmu.mak 
b/default-configs/x86_64-softmmu.mak
index 0221236..179ca35 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -14,7 +14,9 @@ CONFIG_IPMI=y
 CONFIG_IPMI_LOCAL=y
 CONFIG_IPMI_EXTERN=y
 CONFIG_ISA_IPMI_KCS=y
+CONFIG_PCI_IPMI_KCS=y
 CONFIG_ISA_IPMI_BT=y
+CONFIG_PCI_IPMI_BT=y
 CONFIG_SERIAL=y
 CONFIG_SERIAL_ISA=y
 CONFIG_PARALLEL=y
diff --git a/hw/ipmi/Makefile.objs b/hw/ipmi/Makefile.objs
index 4ffa45a..2d7f080 100644
--- a/hw/ipmi/Makefile.objs
+++ b/hw/ipmi/Makefile.objs
@@ -2,4 +2,6 @@ common-obj-$(CONFIG_IPMI) += ipmi.o ipmi_kcs.o ipmi_bt.o
 common-obj-$(CONFIG_IPMI_LOCAL) += ipmi_bmc_sim.o
 common-obj-$(CONFIG_IPMI_EXTERN) += ipmi_bmc_extern.o
 common-obj-$(CONFIG_ISA_IPMI_KCS) += isa_ipmi_kcs.o
+common-obj-$(CONFIG_PCI_IPMI_KCS) += pci_ipmi_kcs.o
 common-obj-$(CONFIG_ISA_IPMI_BT) += isa_ipmi_bt.o
+common-obj-$(CONFIG_PCI_IPMI_BT) += pci_ipmi_bt.o
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
new file mode 100644
index 000..8002027
--- /dev/null
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -0,0 +1,145 @@
+/*
+ * QEMU PCI IPMI BT emulation
+ *
+ * Copyright (c) 2017 Corey Minyard, MontaVista Software, LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/ipmi/ipmi_bt.h"
+#include "hw/pci/pci.h"
+
+#define TYPE_PCI_IPMI_BT "pci-ipmi-bt"
+#define PCI_IPMI_BT(obj) OBJECT_CHECK(PCIIPMIBTDevice, (obj), \
+   TYPE_PCI_IPMI_BT)
+
+typedef struct PCIIPMIBTDevice {
+PCIDevice dev;
+IPMIBT bt;
+bool irq_enabled;
+uint32_t uuid;
+} PCIIPMIBTDevice;
+
+static void pci_ipmi_raise_irq(IPMIBT *ik)
+{
+PCIIPMIBTDevice *pik = ik->opaque;
+
+pci_set_irq(>dev, true);
+}
+
+static void pci_ipmi_lower_irq(IPMIBT *ik)
+{
+PCIIPMIBTDevice *pik = ik->opaque;
+
+pci_set_irq(>dev, false);
+}
+
+static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
+{
+PCIIPMIBTDevice *pik = PCI_IPMI_BT(pd);
+IPMIInterface *ii = IPMI_INTERFACE(pd);
+IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
+
+if (!pik->bt.bmc) {
+error_setg(errp, "IPMI device requires a bmc attribute to be set");
+return;
+}
+
+pik->uuid = ipmi_next_uuid();
+
+pik->bt.bmc->intf = ii;
+pik->bt.opaque = pik;
+
+pci_config_set_prog_interface(pd->config, 0x02); /* BT */
+pci_config_set_interrupt_pin(pd->config, 0x01);
+pik->bt.use_irq = 1;
+pik->bt.raise_irq = pci_ipmi_raise_irq;
+pik->bt.lower_irq = pci_ipmi_lower_irq;
+
+iic->init(ii, 8, errp);
+if (*errp) {
+return;
+}
+pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, >bt.io);
+}
+
+const VMStateDescription vmstate_PCIIPMIBTDevice = {
+.name = TYPE_IPMI_INTERFACE_PREFIX "pci-bt",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields  = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(dev,

[Qemu-devel] [PATCH 6/7] smbios:ipmi: Ignore IPMI devices with no fwinfo function

From: Corey Minyard 

Not all devices have fwinfo (like the coming PCI one), so ignore
them if the their fwinfo function is NULL.

Signed-off-by: Corey Minyard 
---
 hw/smbios/smbios_type_38.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
index 56e8609..5475323 100644
--- a/hw/smbios/smbios_type_38.c
+++ b/hw/smbios/smbios_type_38.c
@@ -95,6 +95,9 @@ static void smbios_add_ipmi_devices(BusState *bus)
 ii = IPMI_INTERFACE(obj);
 iic = IPMI_INTERFACE_GET_CLASS(obj);
 memset(, 0, sizeof(info));
+if (!iic->get_fwinfo) {
+continue;
+}
 iic->get_fwinfo(ii, );
 smbios_build_one_type_38();
 continue;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/9] block: don't keep AioContext acquired after blockdev_backup_prepare()

On 12/06/2017 08:45 AM, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi 
> ---
>  blockdev.c | 44 ++--
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 

>  state->job = do_blockdev_backup(backup, common->block_job_txn, 
> _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
> -return;
> +goto out;
>  }
> +
> +out:
> +aio_context_release(aio_context);
>  }

Again a weird one-shot label where fallthrough would do the same.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 1/7] ipmi: Use proper struct reference for KCS vmstate

From: Corey Minyard 

The vmstate for isa_ipmi_kcs was referencing into the kcs structure,
instead create a kcs structure separate and use that.

There was also some issues in the state transfer.  The inlen field
was not being transferred, so if a transaction was in process during
the transfer it would be messed up.  And the use_irq field was
transferred, but that should come from the configuration.  This
also fixes those issues and is tested under heavy load.

Signed-off-by: Corey Minyard 
---
 hw/ipmi/isa_ipmi_kcs.c | 75 --
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 8044497..c887251 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -423,24 +423,69 @@ static void ipmi_isa_realize(DeviceState *dev, Error 
**errp)
 isa_register_ioport(isadev, >kcs.io, iik->kcs.io_base);
 }
 
-const VMStateDescription vmstate_ISAIPMIKCSDevice = {
-.name = TYPE_IPMI_INTERFACE,
+static const VMStateDescription vmstate_IPMIKCS = {
+.name = TYPE_IPMI_INTERFACE_PREFIX "kcs",
 .version_id = 1,
 .minimum_version_id = 1,
 .fields  = (VMStateField[]) {
-VMSTATE_BOOL(kcs.obf_irq_set, ISAIPMIKCSDevice),
-VMSTATE_BOOL(kcs.atn_irq_set, ISAIPMIKCSDevice),
-VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
-VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
-VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
-VMSTATE_UINT8_ARRAY(kcs.outmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
-VMSTATE_UINT8_ARRAY(kcs.inmsg, ISAIPMIKCSDevice, MAX_IPMI_MSG_SIZE),
-VMSTATE_BOOL(kcs.write_end, ISAIPMIKCSDevice),
-VMSTATE_UINT8(kcs.status_reg, ISAIPMIKCSDevice),
-VMSTATE_UINT8(kcs.data_out_reg, ISAIPMIKCSDevice),
-VMSTATE_INT16(kcs.data_in_reg, ISAIPMIKCSDevice),
-VMSTATE_INT16(kcs.cmd_reg, ISAIPMIKCSDevice),
-VMSTATE_UINT8(kcs.waiting_rsp, ISAIPMIKCSDevice),
+VMSTATE_BOOL(obf_irq_set, IPMIKCS),
+VMSTATE_BOOL(atn_irq_set, IPMIKCS),
+VMSTATE_BOOL(use_irq, IPMIKCS),
+VMSTATE_BOOL(irqs_enabled, IPMIKCS),
+VMSTATE_UINT32(outpos, IPMIKCS),
+VMSTATE_UINT8_ARRAY(outmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
+VMSTATE_UINT32(inlen, IPMIKCS),
+VMSTATE_UINT8_ARRAY(inmsg, IPMIKCS, MAX_IPMI_MSG_SIZE),
+VMSTATE_BOOL(write_end, IPMIKCS),
+VMSTATE_UINT8(status_reg, IPMIKCS),
+VMSTATE_UINT8(data_out_reg, IPMIKCS),
+VMSTATE_INT16(data_in_reg, IPMIKCS),
+VMSTATE_INT16(cmd_reg, IPMIKCS),
+VMSTATE_UINT8(waiting_rsp, IPMIKCS),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static int isa_ipmi_kcs_load_old(QEMUFile *f, void *opaque, int version_id)
+{
+ISAIPMIKCSDevice *iik = opaque;
+IPMIKCS *k = >kcs;
+unsigned int i;
+
+if (version_id != 1) {
+return -EINVAL;
+}
+
+k->obf_irq_set = qemu_get_byte(f);
+k->atn_irq_set = qemu_get_byte(f);
+qemu_get_byte(f); /* Used to be use_irq, but that's not a good idea. */
+k->irqs_enabled = qemu_get_byte(f);
+k->outpos = qemu_get_be32(f);
+for (i = 0; i < MAX_IPMI_MSG_SIZE; i++) {
+k->outmsg[i] = qemu_get_byte(f);
+}
+k->inlen = 0; /* This was forgotten on version 1, just reset it. */
+for (i = 0; i < MAX_IPMI_MSG_SIZE; i++) {
+k->inmsg[i] = qemu_get_byte(f);
+}
+k->write_end = qemu_get_byte(f);
+k->status_reg = qemu_get_byte(f);
+k->data_out_reg = qemu_get_byte(f);
+k->data_in_reg = qemu_get_be16(f);
+k->cmd_reg = qemu_get_be16(f);
+k->waiting_rsp = qemu_get_byte(f);
+
+return 0;
+}
+
+static const VMStateDescription vmstate_ISAIPMIKCSDevice = {
+.name = TYPE_IPMI_INTERFACE_PREFIX "isa-kcs",
+.version_id = 2,
+.minimum_version_id = 2,
+.minimum_version_id_old = 1,
+.load_state_old = isa_ipmi_kcs_load_old,
+.fields  = (VMStateField[]) {
+VMSTATE_STRUCT(kcs, ISAIPMIKCSDevice, 1, vmstate_IPMIKCS, IPMIKCS),
 VMSTATE_END_OF_LIST()
 }
 };
-- 
2.7.4

[Qemu-devel] [PATCH 5/7] ipmi: Allow a size value to be passed for I/O space

From: Corey Minyard 

PCI device I/O must be >= 8 bytes in length or they don't work.
Allow the size to be passed in, the default size of 2 or 3
won't work.

Signed-off-by: Corey Minyard 
---
 hw/ipmi/ipmi_bt.c  | 19 +++
 hw/ipmi/ipmi_kcs.c | 23 +++
 hw/ipmi/isa_ipmi_bt.c  |  2 +-
 hw/ipmi/isa_ipmi_kcs.c |  2 +-
 include/hw/ipmi/ipmi.h |  7 ++-
 include/hw/ipmi/ipmi_bt.h  |  1 +
 include/hw/ipmi/ipmi_kcs.h |  1 +
 7 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c
index e07e10a..130ef24 100644
--- a/hw/ipmi/ipmi_bt.c
+++ b/hw/ipmi/ipmi_bt.c
@@ -187,7 +187,7 @@ static uint64_t ipmi_bt_ioport_read(void *opaque, hwaddr 
addr, unsigned size)
 IPMIBT *ib = iic->get_backend_data(ii);
 uint32_t ret = 0xff;
 
-switch (addr & 3) {
+switch (addr & ib->size_mask) {
 case 0:
 ret = ib->control_reg;
 break;
@@ -206,6 +206,9 @@ static uint64_t ipmi_bt_ioport_read(void *opaque, hwaddr 
addr, unsigned size)
 case 2:
 ret = ib->mask_reg;
 break;
+default:
+ret = 0xff;
+break;
 }
 return ret;
 }
@@ -228,7 +231,7 @@ static void ipmi_bt_ioport_write(void *opaque, hwaddr addr, 
uint64_t val,
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
 IPMIBT *ib = iic->get_backend_data(ii);
 
-switch (addr & 3) {
+switch (addr & ib->size_mask) {
 case 0:
 if (IPMI_BT_GET_CLR_WR(val)) {
 ib->inlen = 0;
@@ -283,6 +286,9 @@ static void ipmi_bt_ioport_write(void *opaque, hwaddr addr, 
uint64_t val,
 ipmi_bt_lower_irq(ib);
 }
 break;
+default:
+/* Ignore. */
+break;
 }
 }
 
@@ -344,14 +350,19 @@ static void ipmi_bt_set_irq_enable(IPMIInterface *ii, int 
val)
 ib->irqs_enabled = val;
 }
 
-static void ipmi_bt_init(IPMIInterface *ii, Error **errp)
+static void ipmi_bt_init(IPMIInterface *ii, unsigned int min_size, Error 
**errp)
 {
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
 IPMIBT *ib = iic->get_backend_data(ii);
 
+if (min_size == 0) {
+min_size = 4;
+}
+ib->size_mask = min_size - 1;
 ib->io_length = 3;
 
-memory_region_init_io(>io, NULL, _bt_io_ops, ii, "ipmi-bt", 3);
+memory_region_init_io(>io, NULL, _bt_io_ops, ii, "ipmi-bt",
+  min_size);
 }
 
 const VMStateDescription vmstate_IPMIBT = {
diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
index cb22dee..ec6dc39 100644
--- a/hw/ipmi/ipmi_kcs.c
+++ b/hw/ipmi/ipmi_kcs.c
@@ -230,7 +230,7 @@ static uint64_t ipmi_kcs_ioport_read(void *opaque, hwaddr 
addr, unsigned size)
 IPMIKCS *ik = iic->get_backend_data(ii);
 uint32_t ret;
 
-switch (addr & 1) {
+switch (addr & ik->size_mask) {
 case 0:
 ret = ik->data_out_reg;
 IPMI_KCS_SET_OBF(ik->status_reg, 0);
@@ -241,6 +241,7 @@ static uint64_t ipmi_kcs_ioport_read(void *opaque, hwaddr 
addr, unsigned size)
 }
 }
 break;
+
 case 1:
 ret = ik->status_reg;
 if (ik->atn_irq_set) {
@@ -250,6 +251,9 @@ static uint64_t ipmi_kcs_ioport_read(void *opaque, hwaddr 
addr, unsigned size)
 }
 }
 break;
+
+default:
+ret = 0xff;
 }
 return ret;
 }
@@ -265,7 +269,7 @@ static void ipmi_kcs_ioport_write(void *opaque, hwaddr 
addr, uint64_t val,
 return;
 }
 
-switch (addr & 1) {
+switch (addr & ik->size_mask) {
 case 0:
 ik->data_in_reg = val;
 break;
@@ -273,6 +277,10 @@ static void ipmi_kcs_ioport_write(void *opaque, hwaddr 
addr, uint64_t val,
 case 1:
 ik->cmd_reg = val;
 break;
+
+default:
+/* Ignore. */
+break;
 }
 IPMI_KCS_SET_IBF(ik->status_reg, 1);
 ipmi_kcs_signal(ik, ii);
@@ -319,13 +327,20 @@ static void ipmi_kcs_set_irq_enable(IPMIInterface *ii, 
int val)
 ik->irqs_enabled = val;
 }
 
-static void ipmi_kcs_init(IPMIInterface *ii, Error **errp)
+/* min_size must be a power of 2. */
+static void ipmi_kcs_init(IPMIInterface *ii, unsigned int min_size,
+  Error **errp)
 {
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
 IPMIKCS *ik = iic->get_backend_data(ii);
 
+if (min_size == 0) {
+min_size = 2;
+}
+ik->size_mask = min_size - 1;
 ik->io_length = 2;
-memory_region_init_io(>io, NULL, _kcs_io_ops, ii, "ipmi-kcs", 2);
+memory_region_init_io(>io, NULL, _kcs_io_ops, ii, "ipmi-kcs",
+  min_size);
 }
 
 const VMStateDescription vmstate_IPMIKCS = {
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index b711eca..a98e0ea 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -80,7 +80,7 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error 
**errp)
 iib->bt.bmc->intf = ii;
 iib->bt.opaque =

[Qemu-devel] [PATCH 0/7] Add PCI IPMI interfaces

I had to do some testing, so I went ahead and did this.  Most of the
changes are re-arranging and splitting up code to separate out the
KCS/BT code from the ISA-specific code.

There are also some migration fixes here.

This series requires the previous series: Small IPMI (and other)
fixes

[Qemu-devel] [PATCH 2/7] ipmi: Use proper struct reference for BT vmstate

From: Corey Minyard 

The vmstate for isa_ipmi_bt was referencing into the bt structure,
instead create a bt structure separate and use that.

The version 1 of the BT transfer was fairly broken, if a migration
occured during an IPMI operation, it is likely the migration would
be corrupted because I misunderstood the VMSTATE_VBUFFER_UINT32()
handling, I thought it handled transferring the length field,
too.  So I just remove support for that.  I doubt anyone is using
it at this point.

This also removes the transfer of use_irq, since that should come
from configuration.

Signed-off-by: Corey Minyard 
---
 hw/ipmi/isa_ipmi_bt.c | 43 ++-
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index 13a8c09..c78ec6d 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -451,22 +451,39 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error 
**errp)
 isa_register_ioport(isadev, >bt.io, iib->bt.io_base);
 }
 
-static const VMStateDescription vmstate_ISAIPMIBTDevice = {
-.name = TYPE_IPMI_INTERFACE,
+
+const VMStateDescription vmstate_IPMIBT = {
+.name = TYPE_IPMI_INTERFACE_PREFIX "bt",
 .version_id = 1,
 .minimum_version_id = 1,
 .fields  = (VMStateField[]) {
-VMSTATE_BOOL(bt.obf_irq_set, ISAIPMIBTDevice),
-VMSTATE_BOOL(bt.atn_irq_set, ISAIPMIBTDevice),
-VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
-VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
-VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
-VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, bt.outlen),
-VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, bt.inlen),
-VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
-VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
-VMSTATE_UINT8(bt.waiting_rsp, ISAIPMIBTDevice),
-VMSTATE_UINT8(bt.waiting_seq, ISAIPMIBTDevice),
+VMSTATE_BOOL(obf_irq_set, IPMIBT),
+VMSTATE_BOOL(atn_irq_set, IPMIBT),
+VMSTATE_BOOL(irqs_enabled, IPMIBT),
+VMSTATE_UINT32(outpos, IPMIBT),
+VMSTATE_UINT32(outlen, IPMIBT),
+VMSTATE_UINT8_ARRAY(outmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
+VMSTATE_UINT32(inlen, IPMIBT),
+VMSTATE_UINT8_ARRAY(inmsg, IPMIBT, MAX_IPMI_MSG_SIZE),
+VMSTATE_UINT8(control_reg, IPMIBT),
+VMSTATE_UINT8(mask_reg, IPMIBT),
+VMSTATE_UINT8(waiting_rsp, IPMIBT),
+VMSTATE_UINT8(waiting_seq, IPMIBT),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static const VMStateDescription vmstate_ISAIPMIBTDevice = {
+.name = TYPE_IPMI_INTERFACE_PREFIX "isa-bt",
+.version_id = 2,
+.minimum_version_id = 2,
+/*
+ * Version 1 had messed up the array transfer, it's not even usable
+ * because it used VMSTATE_VBUFFER_UINT32, but it did not transfer
+ * the buffer length, so random things would happen.
+ */
+.fields  = (VMStateField[]) {
+VMSTATE_STRUCT(bt, ISAIPMIBTDevice, 1, vmstate_IPMIBT, IPMIBT),
 VMSTATE_END_OF_LIST()
 }
 };
-- 
2.7.4

[Qemu-devel] [PATCH 8/8] Add maintainer for the IPMI code

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 MAINTAINERS | 9 +
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0255113..6483f79 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -924,6 +924,15 @@ F: tests/ahci-test.c
 F: tests/libqos/ahci*
 T: git git://github.com/jnsnow/qemu.git ide
 
+IPMI
+M: Corey Minyard 
+S: Maintained
+F: include/hw/ipmi/*
+F: hw/ipmi/*
+F: hw/smbios/smbios_type_38.c
+F: tests/ipmi*
+T: git git://github.com/cminyard/qemu.git master-ipmi-rebase
+
 Floppy
 M: John Snow 
 L: qemu-bl...@nongnu.org
-- 
2.7.4

[Qemu-devel] [PATCH 7/8] ipmi: Allow BMC device properties to be set

From: Corey Minyard 

Signed-off-by: Corey Minyard 
---
 hw/ipmi/ipmi_bmc_sim.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index e84d710..9b509f8 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -214,8 +214,8 @@ struct IPMIBmcSim {
 uint8_t device_rev;
 uint8_t fwrev1;
 uint8_t fwrev2;
-uint8_t mfg_id[3];
-uint8_t product_id[2];
+uint32_t mfg_id;
+uint16_t product_id;
 
 uint8_t restart_cause;
 
@@ -867,11 +867,11 @@ static void get_device_id(IPMIBmcSim *ibs,
 rsp_buffer_push(rsp, ibs->fwrev2);
 rsp_buffer_push(rsp, ibs->ipmi_version);
 rsp_buffer_push(rsp, 0x07); /* sensor, SDR, and SEL. */
-rsp_buffer_push(rsp, ibs->mfg_id[0]);
-rsp_buffer_push(rsp, ibs->mfg_id[1]);
-rsp_buffer_push(rsp, ibs->mfg_id[2]);
-rsp_buffer_push(rsp, ibs->product_id[0]);
-rsp_buffer_push(rsp, ibs->product_id[1]);
+rsp_buffer_push(rsp, ibs->mfg_id & 0xff);
+rsp_buffer_push(rsp, (ibs->mfg_id >> 8) & 0xff);
+rsp_buffer_push(rsp, (ibs->mfg_id >> 16) & 0xff);
+rsp_buffer_push(rsp, ibs->product_id & 0xff);
+rsp_buffer_push(rsp, (ibs->product_id >> 8) & 0xff);
 }
 
 static void set_global_enables(IPMIBmcSim *ibs, uint8_t val)
@@ -1997,6 +1997,13 @@ static Property ipmi_sim_properties[] = {
 DEFINE_PROP_UINT16("fruareasize", IPMIBmcSim, fru.areasize, 1024),
 DEFINE_PROP_STRING("frudatafile", IPMIBmcSim, fru.filename),
 DEFINE_PROP_STRING("sdrfile", IPMIBmcSim, sdr_filename),
+DEFINE_PROP_UINT8("device_id", IPMIBmcSim, device_id, 0x20),
+DEFINE_PROP_UINT8("ipmi_version", IPMIBmcSim, ipmi_version, 0x02),
+DEFINE_PROP_UINT8("device_rev", IPMIBmcSim, device_rev, 0),
+DEFINE_PROP_UINT8("fwrev1", IPMIBmcSim, fwrev1, 0),
+DEFINE_PROP_UINT8("fwrev2", IPMIBmcSim, fwrev2, 0),
+DEFINE_PROP_UINT32("mfg_id", IPMIBmcSim, mfg_id, 0),
+DEFINE_PROP_UINT16("product_id", IPMIBmcSim, product_id, 0),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH 1/8] ipmi: Fix SEL get/set time commands

From: Corey Minyard 

The minimum message size was on the wrong commands, for getting
the time it's zero and for setting the time it's 6.

Signed-off-by: Corey Minyard 
Reviewed-by: Cédric Le Goater 
---
 hw/ipmi/ipmi_bmc_sim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 277c28c..cc068f2 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1802,8 +1802,8 @@ static const IPMICmdHandler storage_cmds[] = {
 [IPMI_CMD_GET_SEL_ENTRY] = { get_sel_entry, 8 },
 [IPMI_CMD_ADD_SEL_ENTRY] = { add_sel_entry, 18 },
 [IPMI_CMD_CLEAR_SEL] = { clear_sel, 8 },
-[IPMI_CMD_GET_SEL_TIME] = { get_sel_time, 6 },
-[IPMI_CMD_SET_SEL_TIME] = { set_sel_time },
+[IPMI_CMD_GET_SEL_TIME] = { get_sel_time },
+[IPMI_CMD_SET_SEL_TIME] = { set_sel_time, 6 },
 };
 
 static const IPMINetfn storage_netfn = {
-- 
2.7.4

[Qemu-devel] [PATCH 0/8] Small IPMI (and other) fixes

These are some fixes I've had for a while (mostly).  They are small
fixes and additions for various things.  It would be nice to be able
to get at least some of these into the upcoming release, but it's
probably too late.

I also add myself as the maintainer for the IPMI code, since that
seemed appropriate.

[Qemu-devel] [PATCH 6/8] vl.c: disallow command line fw cfg without opt/

From: "Michael S. Tsirkin" 

Allowing arbitary file names on command line is setting us up for
failure: future guests will look for a specific QEMU-specified name and
will get confused finding a user file there.

Signed-off-by: Michael S. Tsirkin 
[Change "warning" to "error" in the error report.]
Signed-off-by: Corey Minyard 
---
 vl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 1ad1c04..fae9300 100644
--- a/vl.c
+++ b/vl.c
@@ -2400,8 +2400,9 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
Error **errp)
 return -1;
 }
 if (strncmp(name, "opt/", 4) != 0) {
-warn_report("externally provided fw_cfg item names "
-"should be prefixed with \"opt/\"");
+error_report("error: externally provided fw_cfg item names "
+ "should be prefixed with \"opt/\"");
+return -1;
 }
 if (nonempty_str(str)) {
 size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
-- 
2.7.4

[Qemu-devel] [PATCH 5/8] ipmi: disable IRQ and ATN on an external disconnect