date:20200721

Peter Maydell  writes:

> On Tue, 21 Jul 2020 at 17:07, Philippe Mathieu-Daudé  
> wrote:
>>
>> Hi Stefan,
>>
>> I'm trying to understand what is modelling the
>> TYPE_TPM_TIS_ISA device.
>>
>> It inherits from TYPE_ISA_DEVICE, so I expected
>> to see an ISA device, but then I noticed:
>>
>> 1/ it doesn't use the ISA I/O space, it directly
>> maps the device in the system memory at a fixed
>> address that is not addressable by the ISA bus:
>>
>> #define TPM_TIS_ADDR_BASE   0xFED4
>
> Why do you think this is mapping to the system memory?
> tpm_tis_isa_realizefn() does:
>
> memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
> TPM_TIS_ADDR_BASE, >mmio);
>
> which puts it into the ISA memory address space.
>
> The weird thing about this is not which AS it's
> going in but the fact that the TPM_TIS_ADDR_BASE
> is way higher than an actual ISA bus can address
> (so for instance it's out of range of the size of
> the ISA memory window on the Jazz board).
>
>> 2/ it is not plugged to an ISA BUS (ISABus*)
>
> Won't it autoplug into the ISA bus if you say "-device tpm-tis",
> the same as any other ISA device ?

Yup:

$ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -chardev 
socket,id=chrtpm,path=tpm/swtpm-sock -tpmdev emulator,id=tpm0,chardev=chrtpm 
-device tpm-tis,tpmdev=tpm0
QEMU 5.0.90 monitor - type 'help' for more information
(qemu) info qtree
bus: main-system-bus
  type System
  [...]
  dev: i440FX-pcihost, id ""
[...]
bus: pci.0
  type PCI
[...]
  dev: PIIX3, id ""
[...]
bus: isa.0
  type ISA
  dev: tpm-tis, id ""
irq = 5 (0x5)
tpmdev = "tpm0"
ppi = true
isa irq 5
  [...]

This is with

$ swtpm socket --tpmstate dir=tpm --ctrl type=unixio,path=tpm/swtpm-soc

running in another terminal.

>> 3/ no machine plug it using isa_register_ioport()
>>(it is not registered to the ISA memory space)
>
> There's no requirement for an ISA device to have IO ports...
>
> thanks
> -- PMM

Thread hijack!  Since I didn't have swtpm installed, I tried to take a
shortcut:

$ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -chardev 
null,id=tpm0 -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-tis,tpmdev=tpm0
qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: tpm-emulator: 
tpm chardev 'chrtpm' not found.
qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: tpm-emulator: 
Could not cleanly shutdown the TPM: No such file or directory
QEMU 5.0.90 monitor - type 'help' for more information
(qemu) qemu-system-x86_64: -device tpm-tis,tpmdev=tpm0: Property 
'tpm-tis.tpmdev' can't find value 'tpm0'
$ echo $?
1

That a null chardev doesn't work is fine.  But the error handling looks
broken: QEMU diagnoses and reports the problem, then continues.  The
final error message indicates that it continued without creating the
backend "tpm0".  That's wrong.

Different tack: could -tpmdev be made sugar for -object?  I'm asking
because other kinds of backends use -object instead of their very own
option.

Re: [PATCH v3] e1000e: using bottom half to send packets




On 2020/7/22 下午12:47, Li Qiang wrote:

Jason Wang  于2020年7月22日周三 上午11:32写道：


On 2020/7/21 下午11:17, Li Qiang wrote:

Alexander Bulekov reported a UAF bug related e1000e packets send.

-->https://bugs.launchpad.net/qemu/+bug/1886362

This is because the guest trigger a e1000e packet send and set the
data's address to e1000e's MMIO address. So when the e1000e do DMA
it will write the MMIO again and trigger re-entrancy and finally
causes this UAF.

Paolo suggested to use a bottom half whenever MMIO is doing complicate
things in here:
-->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html

Reference here:
'The easiest solution is to delay processing of descriptors to a bottom
half whenever MMIO is doing something complicated.  This is also better
for latency because it will free the vCPU thread more quickly and leave
the work to the I/O thread.'

This patch fixes this UAF.

Reported-by: Alexander Bulekov 
Signed-off-by: Li Qiang 
---
Change since v2:
1. Add comments for the tx bh schdule when VM resumes
2. Leave the set ics code in 'e1000e_start_xmit'
3. Cancel the tx bh and reset tx_waiting in e1000e_core_reset


So based on our discussion this is probably not sufficient. It solves
the issue TX re-entrancy but not RX (e.g RX DMA to RDT?) Or is e1000e's
RX is reentrant?


It seems the RX has no reentrant issue if we address the TX reentrant issue.



Just to make sure I understand.

Did you mean

1) RX code is reentrant

or

2) the following can't happen

e1000_receive()
    e1000e_write_packet_to_guest()
        e1000e_write_to_rx_buffers()
            pci_dma_write()
                ...
                    e1000e_set_rdt()
                        qemu_flush_queued_packets()
                            e1000e_receive()

?



Though we can write the RX-related DMA register, then when we receive
packets, we write to the MMIO with any data.
However this is not different between the guest write any data to MMIO.

I think we can hold on this patch and discuss more about this MMIO
reentrant issue then make
the decision.



Right, and we need start to think of how to do the test, e.g qtest?

Thanks




Thanks,
Li Qiang



Thanks



Change since v1:
Per Jason's review here:
-- https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg05368.html
1. Cancel and schedule the tx bh when VM is stopped or resume
2. Add a tx_burst for e1000e configuration to throttle the bh execution
3. Add a tx_waiting to record whether the bh is pending or not
Don't use BQL in the tx_bh handler as when tx_bh is executed, the BQL is
acquired.

   hw/net/e1000e.c  |   6 +++
   hw/net/e1000e_core.c | 107 +++
   hw/net/e1000e_core.h |   8 
   3 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index fda34518c9..24e35a78bf 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -77,10 +77,14 @@ typedef struct E1000EState {

   bool disable_vnet;

+int32_t tx_burst;
+
   E1000ECore core;

   } E1000EState;

+#define TX_BURST 256
+
   #define E1000E_MMIO_IDX 0
   #define E1000E_FLASH_IDX1
   #define E1000E_IO_IDX   2
@@ -263,6 +267,7 @@ static void e1000e_core_realize(E1000EState *s)
   {
   s->core.owner = >parent_obj;
   s->core.owner_nic = s->nic;
+s->core.tx_burst = s->tx_burst;
   }

   static void
@@ -665,6 +670,7 @@ static Property e1000e_properties[] = {
   e1000e_prop_subsys_ven, uint16_t),
   DEFINE_PROP_SIGNED("subsys", E1000EState, subsys, 0,
   e1000e_prop_subsys, uint16_t),
+DEFINE_PROP_INT32("x-txburst", E1000EState, tx_burst, TX_BURST),
   DEFINE_PROP_END_OF_LIST(),
   };

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index bcd186cac5..2fdfc23204 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -910,18 +910,17 @@ e1000e_rx_ring_init(E1000ECore *core, E1000E_RxRing *rxr, 
int idx)
   }

   static void
-e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
+e1000e_start_xmit(struct e1000e_tx *q)
   {
+E1000ECore *core = q->core;
   dma_addr_t base;
   struct e1000_tx_desc desc;
-bool ide = false;
-const E1000E_RingInfo *txi = txr->i;
-uint32_t cause = E1000_ICS_TXQE;
+const E1000E_RingInfo *txi;
+E1000E_TxRing txr;
+int32_t num_packets = 0;

-if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
-trace_e1000e_tx_disabled();
-return;
-}
+e1000e_tx_ring_init(core, , q - >tx[0]);
+txi = txr.i;

   while (!e1000e_ring_empty(core, txi)) {
   base = e1000e_ring_head_descr(core, txi);
@@ -931,14 +930,24 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing 
*txr)
   trace_e1000e_tx_descr((void *)(intptr_t)desc.buffer_addr,
 desc.lower.data, desc.upper.data);

-e1000e_process_tx_desc(core, txr->tx, , txi->idx);
-cause |= e1000e_txdesc_writeback(core, base, , ,

Re: [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64

2020-07-21 Thread Bin Meng

Hi Zong,

On Tue, Jul 21, 2020 at 8:41 PM Zong Li  wrote:
>
> On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
> entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
> implementation, the second parameter of pmp_write_cfg is
> "reg_index * sizeof(target_ulong)", and we get the the result
> which is started from 16 if reg_index is 2, but we expect that
> it should be started from 8. Separate the implementation for
> RV32 and RV64 respectively.
>
> Signed-off-by: Zong Li 
>
> Changed in v3:
>  - Refine the implementation. Suggested by Bin Meng.
>
> Changed in v2:
>  - Move out the shifting operation from loop. Suggested by Bin Meng.

As I mentioned previously, these changelog should go after --- below.
It should not appear in the commit message.

> ---
>  target/riscv/pmp.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 2a2b9f5363..f2d50bace5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t 
> reg_index,
>  return;
>  }
>
> +#if defined(TARGET_RISCV64)
> +reg_index >>= 1;
> +#endif
> +
>  for (i = 0; i < sizeof(target_ulong); i++) {
>  cfg_val = (val >> 8 * i)  & 0xff;
>  pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
> @@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, 
> uint32_t reg_index)
>  target_ulong cfg_val = 0;
>  target_ulong val = 0;
>
> +#if defined(TARGET_RISCV64)
> +reg_index >>= 1;
> +#endif

We should also move the following:

trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);

before shifting reg_index. Otherwise it traces the wrong pmpcfg CSR read.

> +
>  for (i = 0; i < sizeof(target_ulong); i++) {
>  val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
>  cfg_val |= (val << (i * 8));
> --

Regards,
Bin

Re: [PATCH v3] e1000e: using bottom half to send packets

2020-07-21 Thread P J P

+-- On Wed, 22 Jul 2020, Jason Wang wrote --+
| On 2020/7/21 下午11:17, Li Qiang wrote:
| > Alexander Bulekov reported a UAF bug related e1000e packets send.
| >
| > -->https://bugs.launchpad.net/qemu/+bug/1886362
| >
| > This is because the guest trigger a e1000e packet send and set the
| > data's address to e1000e's MMIO address. So when the e1000e do DMA
| > it will write the MMIO again and trigger re-entrancy and finally
| > causes this UAF.
| >
| > Paolo suggested to use a bottom half whenever MMIO is doing complicate
| > things in here:
| > -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html
| >
| > Reference here:
| > 'The easiest solution is to delay processing of descriptors to a bottom
| > half whenever MMIO is doing something complicated.  This is also better
| > for latency because it will free the vCPU thread more quickly and leave
| > the work to the I/O thread.'
| >
| > This patch fixes this UAF.
| >
| > Reported-by: Alexander Bulekov 
| > Signed-off-by: Li Qiang 
| > ---
| > Change since v2:
| > 1. Add comments for the tx bh schdule when VM resumes
| > 2. Leave the set ics code in 'e1000e_start_xmit'
| > 3. Cancel the tx bh and reset tx_waiting in e1000e_core_reset
| 
| 
| So based on our discussion this is probably not sufficient. It solves the
| issue TX re-entrancy but not RX (e.g RX DMA to RDT?) Or is e1000e's RX is
| reentrant?

Fixes: (CVE-2020-15859) -> https://bugzilla.redhat.com/show_bug.cgi?id=1859168

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[Bug 1751494] Re: tcg-target.inc.c:3495:no such instruction: `xgetbv'

This has been fixed here:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=1019242af11400252

** Changed in: qemu
   Status: New => Fix Released

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1751494

Title:
  tcg-target.inc.c:3495:no such instruction: `xgetbv'

Status in QEMU:
  Fix Released

Bug description:
  While building QEMU on Mac OS 10.6.8 I saw this error message:
  tag-target.inc.c:3495:no such instruction: `xgetbv'
  In the file tcg/i386/tcg-target.inc.c at line 3495 is where the issue is 
located. This is the problem code:
  asm ("xgetbv" : "=a" (xcrl), "=d" (xcrh) : "c" (0));

  https://github.com/asmjit/asmjit/issues/78
  According to the above link, another project also experienced this problem on 
Mac OS X. The fix was to replace the name of the instruction with the encoded 
form '.byte 0x0F, 0x01, 0xd0'. 

  Host info:
  Mac OS 10.6.8
  GCC 5.2.0

  Additional information:
  This may be a gcc issue. I have compiled QEMU on Mac OS 10.12 and didn't 
experience any issues. The compiler used was Apple's clang.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1751494/+subscriptions

Re: [PATCH v3] e1000e: using bottom half to send packets

2020-07-21 Thread Li Qiang

Jason Wang  于2020年7月22日周三 上午11:32写道：
>
>
> On 2020/7/21 下午11:17, Li Qiang wrote:
> > Alexander Bulekov reported a UAF bug related e1000e packets send.
> >
> > -->https://bugs.launchpad.net/qemu/+bug/1886362
> >
> > This is because the guest trigger a e1000e packet send and set the
> > data's address to e1000e's MMIO address. So when the e1000e do DMA
> > it will write the MMIO again and trigger re-entrancy and finally
> > causes this UAF.
> >
> > Paolo suggested to use a bottom half whenever MMIO is doing complicate
> > things in here:
> > -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html
> >
> > Reference here:
> > 'The easiest solution is to delay processing of descriptors to a bottom
> > half whenever MMIO is doing something complicated.  This is also better
> > for latency because it will free the vCPU thread more quickly and leave
> > the work to the I/O thread.'
> >
> > This patch fixes this UAF.
> >
> > Reported-by: Alexander Bulekov 
> > Signed-off-by: Li Qiang 
> > ---
> > Change since v2:
> > 1. Add comments for the tx bh schdule when VM resumes
> > 2. Leave the set ics code in 'e1000e_start_xmit'
> > 3. Cancel the tx bh and reset tx_waiting in e1000e_core_reset
>
>
> So based on our discussion this is probably not sufficient. It solves
> the issue TX re-entrancy but not RX (e.g RX DMA to RDT?) Or is e1000e's
> RX is reentrant?
>

It seems the RX has no reentrant issue if we address the TX reentrant issue.
Though we can write the RX-related DMA register, then when we receive
packets, we write to the MMIO with any data.
However this is not different between the guest write any data to MMIO.

I think we can hold on this patch and discuss more about this MMIO
reentrant issue then make
the decision.

Thanks,
Li Qiang


> Thanks
>
>
> >
> > Change since v1:
> > Per Jason's review here:
> > -- https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg05368.html
> > 1. Cancel and schedule the tx bh when VM is stopped or resume
> > 2. Add a tx_burst for e1000e configuration to throttle the bh execution
> > 3. Add a tx_waiting to record whether the bh is pending or not
> > Don't use BQL in the tx_bh handler as when tx_bh is executed, the BQL is
> > acquired.
> >
> >   hw/net/e1000e.c  |   6 +++
> >   hw/net/e1000e_core.c | 107 +++
> >   hw/net/e1000e_core.h |   8 
> >   3 files changed, 101 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > index fda34518c9..24e35a78bf 100644
> > --- a/hw/net/e1000e.c
> > +++ b/hw/net/e1000e.c
> > @@ -77,10 +77,14 @@ typedef struct E1000EState {
> >
> >   bool disable_vnet;
> >
> > +int32_t tx_burst;
> > +
> >   E1000ECore core;
> >
> >   } E1000EState;
> >
> > +#define TX_BURST 256
> > +
> >   #define E1000E_MMIO_IDX 0
> >   #define E1000E_FLASH_IDX1
> >   #define E1000E_IO_IDX   2
> > @@ -263,6 +267,7 @@ static void e1000e_core_realize(E1000EState *s)
> >   {
> >   s->core.owner = >parent_obj;
> >   s->core.owner_nic = s->nic;
> > +s->core.tx_burst = s->tx_burst;
> >   }
> >
> >   static void
> > @@ -665,6 +670,7 @@ static Property e1000e_properties[] = {
> >   e1000e_prop_subsys_ven, uint16_t),
> >   DEFINE_PROP_SIGNED("subsys", E1000EState, subsys, 0,
> >   e1000e_prop_subsys, uint16_t),
> > +DEFINE_PROP_INT32("x-txburst", E1000EState, tx_burst, TX_BURST),
> >   DEFINE_PROP_END_OF_LIST(),
> >   };
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index bcd186cac5..2fdfc23204 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -910,18 +910,17 @@ e1000e_rx_ring_init(E1000ECore *core, E1000E_RxRing 
> > *rxr, int idx)
> >   }
> >
> >   static void
> > -e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
> > +e1000e_start_xmit(struct e1000e_tx *q)
> >   {
> > +E1000ECore *core = q->core;
> >   dma_addr_t base;
> >   struct e1000_tx_desc desc;
> > -bool ide = false;
> > -const E1000E_RingInfo *txi = txr->i;
> > -uint32_t cause = E1000_ICS_TXQE;
> > +const E1000E_RingInfo *txi;
> > +E1000E_TxRing txr;
> > +int32_t num_packets = 0;
> >
> > -if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> > -trace_e1000e_tx_disabled();
> > -return;
> > -}
> > +e1000e_tx_ring_init(core, , q - >tx[0]);
> > +txi = txr.i;
> >
> >   while (!e1000e_ring_empty(core, txi)) {
> >   base = e1000e_ring_head_descr(core, txi);
> > @@ -931,14 +930,24 @@ e1000e_start_xmit(E1000ECore *core, const 
> > E1000E_TxRing *txr)
> >   trace_e1000e_tx_descr((void *)(intptr_t)desc.buffer_addr,
> > desc.lower.data, desc.upper.data);
> >
> > -e1000e_process_tx_desc(core, txr->tx, , txi->idx);
> > -cause |= e1000e_txdesc_writeback(core, base, , , 
> > txi->idx);
> > +e1000e_process_tx_desc(core, txr.tx, , txi->idx);
> > +

[PATCH v2 3/4] qga/commands-posix: Move the udev code from the pci to the generic function

The libudev-related code is independent from the other pci-related code
and can be re-used for non-pci devices (like ccw devices on s390x). Thus
move this part to the generic function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075
Signed-off-by: Thomas Huth 
---
 qga/commands-posix.c | 62 +++-
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a42ec8171..e8467ac567 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -874,10 +874,6 @@ static bool build_guest_fsinfo_for_pci_dev(char const 
*syspath,
 GuestPCIAddress *pciaddr = disk->pci_controller;
 bool has_ata = false, has_host = false, has_tgt = false;
 char *p, *q, *driver = NULL;
-#ifdef CONFIG_LIBUDEV
-struct udev *udev = NULL;
-struct udev_device *udevice = NULL;
-#endif
 bool ret = false;
 
 p = strstr(syspath, "/devices/pci");
@@ -936,26 +932,6 @@ static bool build_guest_fsinfo_for_pci_dev(char const 
*syspath,
 pciaddr->slot = pci[2];
 pciaddr->function = pci[3];
 
-#ifdef CONFIG_LIBUDEV
-udev = udev_new();
-udevice = udev_device_new_from_syspath(udev, syspath);
-if (udev == NULL || udevice == NULL) {
-g_debug("failed to query udev");
-} else {
-const char *devnode, *serial;
-devnode = udev_device_get_devnode(udevice);
-if (devnode != NULL) {
-disk->dev = g_strdup(devnode);
-disk->has_dev = true;
-}
-serial = udev_device_get_property_value(udevice, "ID_SERIAL");
-if (serial != NULL && *serial != 0) {
-disk->serial = g_strdup(serial);
-disk->has_serial = true;
-}
-}
-#endif
-
 if (strcmp(driver, "ata_piix") == 0) {
 /* a host per ide bus, target*:0::0 */
 if (!has_host || !has_tgt) {
@@ -1017,10 +993,6 @@ static bool build_guest_fsinfo_for_pci_dev(char const 
*syspath,
 
 cleanup:
 g_free(driver);
-#ifdef CONFIG_LIBUDEV
-udev_unref(udev);
-udev_device_unref(udevice);
-#endif
 return ret;
 }
 
@@ -1033,18 +1005,50 @@ static void build_guest_fsinfo_for_real_device(char 
const *syspath,
 GuestPCIAddress *pciaddr;
 GuestDiskAddressList *list = NULL;
 bool has_hwinf;
+#ifdef CONFIG_LIBUDEV
+struct udev *udev = NULL;
+struct udev_device *udevice = NULL;
+#endif
 
 pciaddr = g_new0(GuestPCIAddress, 1);
+pciaddr->domain = -1;   /* -1 means field is invalid */
+pciaddr->bus = -1;
+pciaddr->slot = -1;
+pciaddr->function = -1;
 
 disk = g_new0(GuestDiskAddress, 1);
 disk->pci_controller = pciaddr;
+disk->bus_type = GUEST_DISK_BUS_TYPE_UNKNOWN;
 
 list = g_new0(GuestDiskAddressList, 1);
 list->value = disk;
 
+#ifdef CONFIG_LIBUDEV
+udev = udev_new();
+udevice = udev_device_new_from_syspath(udev, syspath);
+if (udev == NULL || udevice == NULL) {
+g_debug("failed to query udev");
+} else {
+const char *devnode, *serial;
+devnode = udev_device_get_devnode(udevice);
+if (devnode != NULL) {
+disk->dev = g_strdup(devnode);
+disk->has_dev = true;
+}
+serial = udev_device_get_property_value(udevice, "ID_SERIAL");
+if (serial != NULL && *serial != 0) {
+disk->serial = g_strdup(serial);
+disk->has_serial = true;
+}
+}
+
+udev_unref(udev);
+udev_device_unref(udevice);
+#endif
+
 has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
 
-if (has_hwinf) {
+if (has_hwinf || disk->has_dev || disk->has_serial) {
 list->next = fs->disk;
 fs->disk = list;
 } else {
-- 
2.18.1

[PATCH v2 2/4] qga/commands-posix: Rework build_guest_fsinfo_for_real_device() function

We are going to support non-PCI devices soon. For this we need to split
the generic GuestDiskAddress and GuestDiskAddressList memory allocation
and list chaining into a separate function first.

Signed-off-by: Thomas Huth 
---
 qga/commands-posix.c | 65 
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a62a3a70d..1a42ec8171 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -861,28 +861,30 @@ static int build_hosts(char const *syspath, char const 
*host, bool ata,
 return i;
 }
 
-/* Store disk device info specified by @sysfs into @fs */
-static void build_guest_fsinfo_for_real_device(char const *syspath,
-   GuestFilesystemInfo *fs,
-   Error **errp)
+/*
+ * Store disk device info for devices on the PCI bus.
+ * Returns true if information has been stored, or false for failure.
+ */
+static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
+   GuestDiskAddress *disk,
+   Error **errp)
 {
 unsigned int pci[4], host, hosts[8], tgt[3];
 int i, nhosts = 0, pcilen;
-GuestDiskAddress *disk;
-GuestPCIAddress *pciaddr;
-GuestDiskAddressList *list = NULL;
+GuestPCIAddress *pciaddr = disk->pci_controller;
 bool has_ata = false, has_host = false, has_tgt = false;
 char *p, *q, *driver = NULL;
 #ifdef CONFIG_LIBUDEV
 struct udev *udev = NULL;
 struct udev_device *udevice = NULL;
 #endif
+bool ret = false;
 
 p = strstr(syspath, "/devices/pci");
 if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
  pci, pci + 1, pci + 2, pci + 3, ) < 4) {
 g_debug("only pci device is supported: sysfs path '%s'", syspath);
-return;
+return false;
 }
 
 p += 12 + pcilen;
@@ -903,7 +905,7 @@ static void build_guest_fsinfo_for_real_device(char const 
*syspath,
 }
 
 g_debug("unsupported driver or sysfs path '%s'", syspath);
-return;
+return false;
 }
 
 p = strstr(syspath, "/target");
@@ -929,18 +931,11 @@ static void build_guest_fsinfo_for_real_device(char const 
*syspath,
 }
 }
 
-pciaddr = g_malloc0(sizeof(*pciaddr));
 pciaddr->domain = pci[0];
 pciaddr->bus = pci[1];
 pciaddr->slot = pci[2];
 pciaddr->function = pci[3];
 
-disk = g_malloc0(sizeof(*disk));
-disk->pci_controller = pciaddr;
-
-list = g_malloc0(sizeof(*list));
-list->value = disk;
-
 #ifdef CONFIG_LIBUDEV
 udev = udev_new();
 udevice = udev_device_new_from_syspath(udev, syspath);
@@ -1018,21 +1013,43 @@ static void build_guest_fsinfo_for_real_device(char 
const *syspath,
 goto cleanup;
 }
 
-list->next = fs->disk;
-fs->disk = list;
-goto out;
+ret = true;
 
 cleanup:
-if (list) {
-qapi_free_GuestDiskAddressList(list);
-}
-out:
 g_free(driver);
 #ifdef CONFIG_LIBUDEV
 udev_unref(udev);
 udev_device_unref(udevice);
 #endif
-return;
+return ret;
+}
+
+/* Store disk device info specified by @sysfs into @fs */
+static void build_guest_fsinfo_for_real_device(char const *syspath,
+   GuestFilesystemInfo *fs,
+   Error **errp)
+{
+GuestDiskAddress *disk;
+GuestPCIAddress *pciaddr;
+GuestDiskAddressList *list = NULL;
+bool has_hwinf;
+
+pciaddr = g_new0(GuestPCIAddress, 1);
+
+disk = g_new0(GuestDiskAddress, 1);
+disk->pci_controller = pciaddr;
+
+list = g_new0(GuestDiskAddressList, 1);
+list->value = disk;
+
+has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+
+if (has_hwinf) {
+list->next = fs->disk;
+fs->disk = list;
+} else {
+qapi_free_GuestDiskAddressList(list);
+}
 }
 
 static void build_guest_fsinfo_for_device(char const *devpath,
-- 
2.18.1

[PATCH v2 4/4] qga/commands-posix: Support fsinfo for non-PCI virtio devices, too

QEMU on s390x uses virtio via channel I/O instead of PCI by default.
Add a function to detect and provide information for virtio-scsi and
virtio-block devices here, too.

Signed-off-by: Thomas Huth 
---
 qga/commands-posix.c | 42 +-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index e8467ac567..744c2b5a5d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -996,6 +996,39 @@ cleanup:
 return ret;
 }
 
+/*
+ * Store disk device info for non-PCI virtio devices (for example s390x
+ * channel I/O devices). Returns true if information has been stored, or
+ * false for failure.
+ */
+static bool build_guest_fsinfo_for_nonpci_virtio(char const *syspath,
+ GuestDiskAddress *disk,
+ Error **errp)
+{
+unsigned int tgt[3];
+char *p;
+
+if (!strstr(syspath, "/virtio") || !strstr(syspath, "/block")) {
+g_debug("Unsupported virtio device '%s'", syspath);
+return false;
+}
+
+p = strstr(syspath, "/target");
+if (p && sscanf(p + 7, "%*u:%*u:%*u/%*u:%u:%u:%u",
+[0], [1], [2]) == 3) {
+/* virtio-scsi: target*:0:: */
+disk->bus_type = GUEST_DISK_BUS_TYPE_SCSI;
+disk->bus = tgt[0];
+disk->target = tgt[1];
+disk->unit = tgt[2];
+} else {
+/* virtio-blk: 1 disk per 1 device */
+disk->bus_type = GUEST_DISK_BUS_TYPE_VIRTIO;
+}
+
+return true;
+}
+
 /* Store disk device info specified by @sysfs into @fs */
 static void build_guest_fsinfo_for_real_device(char const *syspath,
GuestFilesystemInfo *fs,
@@ -1046,7 +1079,14 @@ static void build_guest_fsinfo_for_real_device(char 
const *syspath,
 udev_device_unref(udevice);
 #endif
 
-has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+if (strstr(syspath, "/devices/pci")) {
+has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+} else if (strstr(syspath, "/virtio")) {
+has_hwinf = build_guest_fsinfo_for_nonpci_virtio(syspath, disk, errp);
+} else {
+g_debug("Unsupported device type for '%s'", syspath);
+has_hwinf = false;
+}
 
 if (has_hwinf || disk->has_dev || disk->has_serial) {
 list->next = fs->disk;
-- 
2.18.1

[PATCH v2 1/4] qga/qapi-schema: Document -1 for invalid PCI address fields

The "guest-get-fsinfo" could also be used for non-PCI devices in the
future. And the code in GuestPCIAddress() in qga/commands-win32.c seems
to be using "-1" for fields that it can not determine already. Thus
let's properly document "-1" as value for invalid PCI address fields.

Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Thomas Huth 
---
 qga/qapi-schema.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 4be9aad48e..408a662ea5 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -846,7 +846,7 @@
 ##
 # @GuestDiskAddress:
 #
-# @pci-controller: controller's PCI address
+# @pci-controller: controller's PCI address (fields are set to -1 if invalid)
 # @bus-type: bus type
 # @bus: bus id
 # @target: target id
-- 
2.18.1

[PATCH v2 0/4] Allow guest-get-fsinfo also for non-PCI devices

The information that can be retrieved via UDEV is also usable for non-PCI
devices. So let's allow build_guest_fsinfo_for_real_device() on non-PCI
devices, too. This is required to fix the bug that CCW devices show up
without "Target" when running libvirt's "virsh domfsinfo" command (see
https://bugzilla.redhat.com/show_bug.cgi?id=1755075 for details).

v2:
 - Use g_new0 instead of g_malloc0 (as suggested by Daniel)
 - Init fields to -1 explicitely, not via memset (Daniel)
 - Add the fourth patch to also fill in virtio information on s390x

Thomas Huth (4):
  qga/qapi-schema: Document -1 for invalid PCI address fields
  qga/commands-posix: Rework build_guest_fsinfo_for_real_device()
function
  qga/commands-posix: Move the udev code from the pci to the generic
function
  qga/commands-posix: Support fsinfo for non-PCI virtio devices, too

 qga/commands-posix.c | 157 ++-
 qga/qapi-schema.json |   2 +-
 2 files changed, 110 insertions(+), 49 deletions(-)

-- 
2.18.1

[Bug 1888431] Re: v5.1.0-rc1 build fails on Mac OS X 10.11.6

I'm sorry, but the QEMU project only supports the two most recent
versions of macOS (see https://www.qemu.org/docs/master/system/build-
platforms.html#macos ), i.e. everything that is older than macOS 10.14
is not supported anymore.

** Changed in: qemu
   Status: New => Won't Fix

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1888431

Title:
  v5.1.0-rc1 build fails on Mac OS X 10.11.6

Status in QEMU:
  Won't Fix

Bug description:
  Hi all,

  build of tag v5.1.0-rc1 fails on Mac OS X 10.11.6 (El Capitan) with
  the following error:

  git clone https://git.qemu.org/git/qemu.git
  
  cd qemu
  git submodule init
  
  git submodule update --recursive
  
  ./configure
  
  make
  

CC  trace/control.o
  In file included from trace/control.c:29:
  In file included from /Users/rtb/src/qemu/include/monitor/monitor.h:4:
  In file included from /Users/rtb/src/qemu/include/block/block.h:4:
  In file included from /Users/rtb/src/qemu/include/block/aio.h:23:
  /Users/rtb/src/qemu/include/qemu/timer.h:843:9: warning: implicit declaration 
of function 'clock_gettime' is invalid in C99
[-Wimplicit-function-declaration]
  clock_gettime(CLOCK_MONOTONIC, );
  ^
  /Users/rtb/src/qemu/include/qemu/timer.h:843:23: error: use of undeclared 
identifier 'CLOCK_MONOTONIC'
  clock_gettime(CLOCK_MONOTONIC, );
^
  1 warning and 1 error generated.
  make: *** [trace/control.o] Error 1

  
  rtb:qemu rtb$ git log -n1
  commit c8004fe6bbfc0d9c2e7b942c418a85efb3ac4b00 (HEAD -> master, tag: 
v5.1.0-rc1, origin/master, origin/HEAD)
  Author: Peter Maydell 
  Date:   Tue Jul 21 20:28:59 2020 +0100

  Update version for v5.1.0-rc1 release
  
  Signed-off-by: Peter Maydell 
  rtb:qemu rtb$ 

  
  Please find the full output of all the commands (from git clone of the repo, 
to the make) in the attached file "buildfail.txt".

  Thank you!

  Best regards,

  Robert Ball

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1888431/+subscriptions

Re: [Bug 1878253] [NEW] null-ptr dereference in address_space_to_flatview through ide


On 7/4/20 12:14 PM, Launchpad Bug Tracker wrote:

You have been subscribed to a public bug by Philippe Mathieu-Daudé (philmd):

Hello,
While fuzzing, I found an input that triggers a null-ptr dereference in
address_space_to_flatview through ide:

==31699==ERROR: AddressSanitizer: SEGV on unknown address 0x0020 (pc 
0x55e0f562bafd bp 0x7ffee92355b0 sp 0x7ffee92354e0 T0)
==31699==The signal is caused by a READ memory access.
==31699==Hint: address points to the zero page.
 #0 0x55e0f562bafd in address_space_to_flatview 
/home/alxndr/Development/qemu/include/exec/memory.h:693:12
 #1 0x55e0f562bafd in address_space_write 
/home/alxndr/Development/qemu/exec.c:3267:14
 #2 0x55e0f562dd9c in address_space_unmap 
/home/alxndr/Development/qemu/exec.c:3592:9
 #3 0x55e0f5ab8277 in dma_memory_unmap 
/home/alxndr/Development/qemu/include/sysemu/dma.h:145:5
 #4 0x55e0f5ab8277 in dma_blk_unmap 
/home/alxndr/Development/qemu/dma-helpers.c:104:9
 #5 0x55e0f5ab8277 in dma_blk_cb 
/home/alxndr/Development/qemu/dma-helpers.c:139:5
 #6 0x55e0f617a6b8 in blk_aio_complete 
/home/alxndr/Development/qemu/block/block-backend.c:1398:9
 #7 0x55e0f617a6b8 in blk_aio_complete_bh 
/home/alxndr/Development/qemu/block/block-backend.c:1408:5
 #8 0x55e0f6355efb in aio_bh_call 
/home/alxndr/Development/qemu/util/async.c:136:5
 #9 0x55e0f6355efb in aio_bh_poll 
/home/alxndr/Development/qemu/util/async.c:164:13
 #10 0x55e0f63608ce in aio_dispatch 
/home/alxndr/Development/qemu/util/aio-posix.c:380:5
 #11 0x55e0f635799a in aio_ctx_dispatch 
/home/alxndr/Development/qemu/util/async.c:306:5
 #12 0x7f16e85d69ed in g_main_context_dispatch 
(/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e9ed)
 #13 0x55e0f635e384 in glib_pollfds_poll 
/home/alxndr/Development/qemu/util/main-loop.c:219:9
 #14 0x55e0f635e384 in os_host_main_loop_wait 
/home/alxndr/Development/qemu/util/main-loop.c:242:5
 #15 0x55e0f635e384 in main_loop_wait 
/home/alxndr/Development/qemu/util/main-loop.c:518:11
 #16 0x55e0f593d676 in qemu_main_loop 
/home/alxndr/Development/qemu/softmmu/vl.c:1664:9
 #17 0x55e0f6267c6a in main 
/home/alxndr/Development/qemu/softmmu/main.c:49:5
 #18 0x7f16e7186e0a in __libc_start_main 
/build/glibc-GwnBeO/glibc-2.30/csu/../csu/libc-start.c:308:16
 #19 0x55e0f55727b9 in _start 
(/home/alxndr/Development/qemu/build/i386-softmmu/qemu-system-i386+0x9027b9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV 
/home/alxndr/Development/qemu/include/exec/memory.h:693:12 in 
address_space_to_flatview

I can reproduce it in qemu 5.0 using:

cat << EOF | ~/Development/qemu/build/i386-softmmu/qemu-system-i386 -M pc 
-nographic -drive file=null-co://,if=ide,cache=writeback,format=raw -nodefaults 
-display none -nographic -qtest stdio -monitor none -serial none
outl 0xcf8 0x8920
outl 0xcfc 0xc001
outl 0xcf8 0x8924
outl 0xcf8 0x8904
outw 0xcfc 0x7
outb 0x1f7 0xc8
outw 0x3f6 0xe784
outw 0x3f6 0xeb01
outb 0xc005 0x21
write 0x2103 0x1 0x4e
outb 0xc000 0x1b
outw 0x1f7 0xff35
EOF



Willing to bet this is the same root cause as some of the others, 
because of this sequence:


outb 0x1f7 0xc8 (Issues a command)
outb 0x3f6 0x84 [1000 0100] - Arms SRST
outb 0x3f6 0x01 [ 0001] - Issues SRST
...
outb 0x1f7 0x35 - Issues another command

The problem continues to be that SRST allows new commands to come in 
while the state machine is still stuck on the first command.


--js


I also attached the traces to this launchpad report, in case the
formatting is broken:

qemu-system-i386 -M pc -nographic -drive file=null-
co://,if=ide,cache=writeback,format=raw -nodefaults -display none
-nographic -qtest stdio -monitor none -serial none < attachment

Please let me know if I can provide any further info.
-Alex

** Affects: qemu
  Importance: Undecided
  Status: New

[Bug 1693667] Re: -cpu haswell / broadwell have no MONITOR in features1

2020-07-21 Thread Launchpad Bug Tracker

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1693667

Title:
  -cpu haswell / broadwell have no MONITOR in features1

Status in QEMU:
  Expired

Bug description:
  In qemu 2.9.0 if you run

  qemu-system-x86_64 -cpu Broadwell (or Haswell)

  then the CPU features1 flag include the SSE3 bit, but do NOT include
  the MONITOR/MWAIT bit.  This is so even when the host includes the
  features.

  
  Additionally, running qemu in this manner results in several error messages:

  warning: TCG doesn't support requested feature: CPUID.01H:ECX.fma [bit 12]
  warning: TCG doesn't support requested feature: CPUID.01H:ECX.pcid [bit 17]
  warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21]
  warning: TCG doesn't support requested feature: CPUID.01H:ECX.tsc-deadline 
[bit 24]
  warning: TCG doesn't support requested feature: CPUID.01H:ECX.avx [bit 28]
  warning: TCG doesn't support requested feature: CPUID.01H:ECX.f16c [bit 29]
  warning: TCG doesn't support requested feature: CPUID.01H:ECX.rdrand [bit 30]
  warning: TCG doesn't support requested feature: CPUID.07H:EBX.hle [bit 4]
  warning: TCG doesn't support requested feature: CPUID.07H:EBX.avx2 [bit 5]
  warning: TCG doesn't support requested feature: CPUID.07H:EBX.invpcid [bit 10]
  warning: TCG doesn't support requested feature: CPUID.07H:EBX.rtm [bit 11]
  warning: TCG doesn't support requested feature: CPUID.07H:EBX.rdseed [bit 18]
  warning: TCG doesn't support requested feature: 
CPUID.8001H:ECX.3dnowprefetch

  
  (Among possible other uses, the lack of the MONITOR feature bit causes NetBSD 
to fall-back on a
  check-and-pause loop while an application CPU is waiting to be told to 
proceed by the boot CPU.)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1693667/+subscriptions

Re: [RFC PATCH-for-5.1 v2] hw/ide: Cancel pending DMA requests before setting as inactive


On 7/17/20 3:53 AM, Philippe Mathieu-Daudé wrote:

libFuzzer found a case where requests are queued for later in the
AIO context, but a command set the bus inactive, then when finally
the requests are processed by the DMA it aborts because it is
inactive:

  include/hw/ide/pci.h:59: IDEState *bmdma_active_if(BMDMAState *): Assertion 
`bmdma->bus->retry_unit != (uint8_t)-1' failed.

Reproducer available on the BugLink.

Fix by draining the pending DMA requests before inactivating the bus.

BugLink: https://bugs.launchpad.net/qemu/+bug/1887303
Signed-off-by: Philippe Mathieu-Daudé 
---
RFC because I don't have much clue about block drive and IDE,
so block-team please be very careful while reviewing this bug.
---
  hw/ide/core.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..f7affafb0c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -804,7 +804,7 @@ void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
  
  void ide_set_inactive(IDEState *s, bool more)

  {


Generally, ide_set_inactive is meant to be used as the normative 
function to transition to the idle state; not something that performs a 
cancellation.


(It should probably assert that there are no pending BHs.)

...Let's run through the reproducer!
In my annotation here,

0x1F0 - Primary Bus I/O
0x3F6 - Primary Bus Control
  [0] Primary Bus, dev0
  [1] Primary Bus, dev1
0x170 - Secondary Bus I/O
0x376 - Secondary Bus Control
  [2] Secondary Bus, dev0
  [3] Secondary Bus, dev1


> outw 0x176 0x3538

  [2].select = 0x38 [0011 1000]
^ select secondary device
  [3].command = 0x35
  ^ WRITE DMA EXT

outw 0x376 0x6007

  [3].control = 0x07 [ 0111]
^- +SRST
  # 0x06 goes into the void?

outw 0x376 0x6b6b

  [3].control = 0x6b; [0110 1011]
 ^- -SRST
  # Oops, this does a Software Reset without cancelling the DMA again.
  # second write goes into the void?

outw 0x176 0x985c

  [3].select = 0x5c; [0101 1100]
  [3].command = 0x98; CHECK POWER MODE
 (Note: Deprecated in ATA4!)
  # Oops, this command shouldn't start when another one is in-process.
  # It also has the bad effect of setting the nsector register to 0xff!

outl 0xcf8 0x8903
outl 0xcfc 0x2f2931
outl 0xcf8 0x8920
outb 0xcfc 0x6b
^- PCI stuff. I'm not as fast at reading hex here.
   My brain has adapted to ATA only.

outb 0x68 0x7
  # Not entirely sure where this goes, but it seems to kick the DMA BH.
  # ... The pending DMA BH that belongs to WRITE_DMA_EXT.

outw 0x176 0x2530

[3].select = 0x30 [0011 ]
  ^ select drive1
[3].command = 0x25 (READ DMA EXT)

... At this point, it explodes because there's a pending DMA already, 
and the sector registers are all wrong.


This bug actually seems to have the same root cause as the other one: 
SRST does not perform the SRST sufficiently.




-s->bus->dma->aiocb = NULL;
+ide_cancel_dma_sync(s);
  ide_clear_retry(s);
  if (s->bus->dma->ops->set_inactive) {
  s->bus->dma->ops->set_inactive(s->bus->dma, more);

[RFC PATCH v2 9/9] target/s390x: Use start-powered-off CPUState property

Instead of setting CPUState::halted to 1 in s390_cpu_initfn(), use the
start-powered-off property which makes cpu_common_reset() initialize it to
1 in common code.

Signed-off-by: Thiago Jung Bauermann 
---
 target/s390x/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 08eb674d22..d3a14af1d9 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -291,7 +291,8 @@ static void s390_cpu_initfn(Object *obj)
 S390CPU *cpu = S390_CPU(obj);
 
 cpu_set_cpustate_pointers(cpu);
-cs->halted = 1;
+object_property_set_bool(OBJECT(cs), "start-powered-off", true,
+ _abort);
 cs->exception_index = EXCP_HLT;
 #if !defined(CONFIG_USER_ONLY)
 object_property_add(obj, "crash-information", "GuestPanicInformation",

[RFC PATCH v2 7/9] sparc/sun4m: Don't set CPUState::halted in cpu_devinit()

Remove setting of cs->halted from cpu_devinit(), which seems out of place
when compared to similar code in other architectures (e.g., ppce500_init()
in hw/ppc/e500.c).

Signed-off-by: Thiago Jung Bauermann 
---
 hw/sparc/sun4m.c | 1 -
 1 file changed, 1 deletion(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 766e79bb5e..7b3042a801 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -831,7 +831,6 @@ static void cpu_devinit(const char *cpu_type, unsigned int 
id,
 } else {
 qemu_register_reset(secondary_cpu_reset, cpu);
 cs = CPU(cpu);
-cs->halted = 1;
 object_property_set_bool(OBJECT(cs), "start-powered-off", true,
  _abort);
 }

[PATCH v2 4/9] ppc/e500: Use start-powered-off CPUState property

Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use
the start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

Signed-off-by: Thiago Jung Bauermann 
---
 hw/ppc/e500.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ab9884e315..dda71bc05d 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque)
 
 cpu_reset(cs);
 
-/* Secondary CPU starts in halted state for now. Needs to change when
-   implementing non-kernel boot. */
-cs->halted = 1;
 cs->exception_index = EXCP_HLT;
 }
 
@@ -897,6 +894,13 @@ void ppce500_init(MachineState *machine)
 } else {
 /* Secondary CPUs */
 qemu_register_reset(ppce500_cpu_reset_sec, cpu);
+
+/*
+ * Secondary CPU starts in halted state for now. Needs to change
+ * when implementing non-kernel boot.
+ */
+object_property_set_bool(OBJECT(cs), "start-powered-off", true,
+ _abort);
 }
 }

[RFC PATCH v2 8/9] sparc/sun4m: Use one cpu_reset() function for main and secondary CPUs

If we rely on cpu_common_reset() setting CPUState::halted according to the
start-powered-off property, both reset functions become equivalent and we
can use only one.

Signed-off-by: Thiago Jung Bauermann 
---
 hw/sparc/sun4m.c | 21 -
 1 file changed, 4 insertions(+), 17 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 7b3042a801..deb5e9f027 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -218,16 +218,7 @@ static void dummy_cpu_set_irq(void *opaque, int irq, int 
level)
 {
 }
 
-static void main_cpu_reset(void *opaque)
-{
-SPARCCPU *cpu = opaque;
-CPUState *cs = CPU(cpu);
-
-cpu_reset(cs);
-cs->halted = 0;
-}
-
-static void secondary_cpu_reset(void *opaque)
+static void sun4m_cpu_reset(void *opaque)
 {
 SPARCCPU *cpu = opaque;
 CPUState *cs = CPU(cpu);
@@ -818,7 +809,6 @@ static const TypeInfo ram_info = {
 static void cpu_devinit(const char *cpu_type, unsigned int id,
 uint64_t prom_addr, qemu_irq **cpu_irqs)
 {
-CPUState *cs;
 SPARCCPU *cpu;
 CPUSPARCState *env;
 
@@ -826,12 +816,9 @@ static void cpu_devinit(const char *cpu_type, unsigned int 
id,
 env = >env;
 
 cpu_sparc_set_id(env, id);
-if (id == 0) {
-qemu_register_reset(main_cpu_reset, cpu);
-} else {
-qemu_register_reset(secondary_cpu_reset, cpu);
-cs = CPU(cpu);
-object_property_set_bool(OBJECT(cs), "start-powered-off", true,
+qemu_register_reset(sun4m_cpu_reset, cpu);
+if (id != 0) {
+object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
  _abort);
 }
 *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);

[PATCH v2 5/9] mips/cps: Use start-powered-off CPUState property

Instead of setting CPUState::halted to 1 in main_cpu_reset(), use the
start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

Signed-off-by: Thiago Jung Bauermann 
---
 hw/mips/cps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 615e1a1ad2..d5b6c78019 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque)
 CPUState *cs = CPU(cpu);
 
 cpu_reset(cs);
-
-/* All VPs are halted on reset. Leave powering up to CPC. */
-cs->halted = 1;
 }
 
 static bool cpu_mips_itu_supported(CPUMIPSState *env)
@@ -89,6 +86,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
 env->itc_tag = mips_itu_get_tag_region(>itu);
 env->itu = >itu;
 }
+/* All VPs are halted on reset. Leave powering up to CPC. */
+object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
+ _abort);
 qemu_register_reset(main_cpu_reset, cpu);
 }

[PATCH v2 6/9] sparc/sun4m: Use start-powered-off CPUState property

Instead of setting CPUState::halted to 1 in secondary_cpu_reset(), use the
start-powered-off property which makes cpu_common_reset() initialize it
to 1 in common code.

Signed-off-by: Thiago Jung Bauermann 
---
 hw/sparc/sun4m.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 9be930415f..766e79bb5e 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -233,7 +233,6 @@ static void secondary_cpu_reset(void *opaque)
 CPUState *cs = CPU(cpu);
 
 cpu_reset(cs);
-cs->halted = 1;
 }
 
 static void cpu_halt_signal(void *opaque, int irq, int level)
@@ -833,6 +832,8 @@ static void cpu_devinit(const char *cpu_type, unsigned int 
id,
 qemu_register_reset(secondary_cpu_reset, cpu);
 cs = CPU(cpu);
 cs->halted = 1;
+object_property_set_bool(OBJECT(cs), "start-powered-off", true,
+ _abort);
 }
 *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
 env->prom_addr = prom_addr;

[PATCH v2 3/9] ppc/spapr: Use start-powered-off CPUState property

PowerPC sPAPR CPUs start in the halted state, and spapr_reset_vcpu()
attempts to implement this by setting CPUState::halted to 1. But that's too
late for the case of hotplugged CPUs in a machine configure with 2 or more
threads per core.

By then, other parts of QEMU have already caused the vCPU to run in an
unitialized state a couple of times. For example, ppc_cpu_reset() calls
ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
a KVM_RUN ioctl on the new vCPU before the guest is able to make the
start-cpu RTAS call to initialize its register state.

This problem doesn't seem to cause visible issues for regular guests, but
on a secure guest running under the Ultravisor it does. The Ultravisor
relies on being able to snoop on the start-cpu RTAS call to map vCPUs to
guests, and this issue causes it to see a stray vCPU that doesn't belong to
any guest.

Fix by setting the start-powered-off CPUState property in
spapr_create_vcpu(), which makes cpu_common_reset() initialize
CPUState::halted to 1 at an earlier moment.

Suggested-by: Eduardo Habkost 
Signed-off-by: Thiago Jung Bauermann 
---
 hw/ppc/spapr_cpu_core.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

NB: Tested on ppc64le pseries KVM guest with two threads per core. 
Hot-plugging additional cores doesn't cause the bug described above
anymore.

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c4f47dcc04..09feeb5f8f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
 
 cpu_reset(cs);
 
-/* All CPUs start halted.  CPU0 is unhalted from the machine level
- * reset code and the rest are explicitly started up by the guest
- * using an RTAS call */
-cs->halted = 1;
-
 env->spr[SPR_HIOR] = 0;
 
 lpcr = env->spr[SPR_LPCR];
@@ -288,6 +283,13 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int 
i, Error **errp)
 
 cpu->machine_data = g_new0(SpaprCpuState, 1);
 
+/*
+ * All CPUs start halted. CPU0 is unhalted from the machine level reset 
code
+ * and the rest are explicitly started up by the guest using an RTAS call.
+ */
+object_property_set_bool(OBJECT(cs), "start-powered-off", true,
+ _abort);
+
 object_unref(obj);
 return cpu;

[PATCH v2 1/9] target/arm: Move start-powered-off property to generic CPUState

There are other platforms which also have CPUs that start powered off, so
generalize the start-powered-off property so that it can be used by them.

Note that ARMv7MState also has a property of the same name but this patch
doesn't change it because that class isn't a subclass of CPUState so it
wouldn't be a trivial change.

This change should not cause any change in behavior.

Suggested-by: Eduardo Habkost 
Signed-off-by: Thiago Jung Bauermann 
---
 exec.c| 1 +
 include/hw/core/cpu.h | 4 
 target/arm/cpu.c  | 5 ++---
 target/arm/cpu.h  | 3 ---
 target/arm/kvm32.c| 2 +-
 target/arm/kvm64.c| 2 +-
 6 files changed, 9 insertions(+), 8 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it.

diff --git a/exec.c b/exec.c
index 6f381f98e2..82e82fab09 100644
--- a/exec.c
+++ b/exec.c
@@ -899,6 +899,7 @@ Property cpu_common_props[] = {
 DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
  MemoryRegion *),
 #endif
+DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8f145733ce..9fc2696db5 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -374,6 +374,10 @@ struct CPUState {
 bool created;
 bool stop;
 bool stopped;
+
+/* Should CPU start in powered-off state? */
+bool start_powered_off;
+
 bool unplug;
 bool crash_occurred;
 bool exit_request;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 111579554f..ec65c7653f 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -174,8 +174,8 @@ static void arm_cpu_reset(DeviceState *dev)
 env->vfp.xregs[ARM_VFP_MVFR1] = cpu->isar.mvfr1;
 env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2;
 
-cpu->power_state = cpu->start_powered_off ? PSCI_OFF : PSCI_ON;
-s->halted = cpu->start_powered_off;
+cpu->power_state = s->start_powered_off ? PSCI_OFF : PSCI_ON;
+s->halted = s->start_powered_off;
 
 if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
 env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
@@ -2182,7 +2182,6 @@ static const ARMCPUInfo arm_cpus[] = {
 };
 
 static Property arm_cpu_properties[] = {
-DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
 DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
 DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0),
 DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8ed423ea..a925d26996 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -810,9 +810,6 @@ struct ARMCPU {
  */
 uint32_t psci_version;
 
-/* Should CPU start in PSCI powered-off state? */
-bool start_powered_off;
-
 /* Current power state, access guarded by BQL */
 ARMPSCIState power_state;
 
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0af46b41c8..1f2b8f8b7a 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -218,7 +218,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
 /* Determine init features for this CPU */
 memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
-if (cpu->start_powered_off) {
+if (cs->start_powered_off) {
 cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
 }
 if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1169237905..f8a6d905fb 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -775,7 +775,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
 /* Determine init features for this CPU */
 memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
-if (cpu->start_powered_off) {
+if (cs->start_powered_off) {
 cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
 }
 if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {

[PATCH v2 2/9] target/arm: Move setting of CPU halted state to generic code

This change is in a separate patch because it's not so obvious that it
won't cause a regression.

Suggested-by: Eduardo Habkost 
Signed-off-by: Thiago Jung Bauermann 
---
 hw/core/cpu.c| 2 +-
 target/arm/cpu.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

NB: I was only able to test that this patch builds. I wasn't able to
run it on an ARM machine. I did on a ppc64le pseries KVM guest.

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 594441a150..71bb7859f1 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -258,7 +258,7 @@ static void cpu_common_reset(DeviceState *dev)
 }
 
 cpu->interrupt_request = 0;
-cpu->halted = 0;
+cpu->halted = cpu->start_powered_off;
 cpu->mem_io_pc = 0;
 cpu->icount_extra = 0;
 atomic_set(>icount_decr_ptr->u32, 0);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ec65c7653f..b6c65e4df6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -175,7 +175,6 @@ static void arm_cpu_reset(DeviceState *dev)
 env->vfp.xregs[ARM_VFP_MVFR2] = cpu->isar.mvfr2;
 
 cpu->power_state = s->start_powered_off ? PSCI_OFF : PSCI_ON;
-s->halted = s->start_powered_off;
 
 if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
 env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';

[PATCH v2 0/9] Generalize start-powered-off property from ARM

The ARM code has a start-powered-off property in ARMCPU, which is a
subclass of CPUState. This property causes arm_cpu_reset() to set
CPUState::halted to 1, signalling that the CPU should start in a halted
state. Other architectures also have code which aim to achieve the same
effect, but without using a property.

The ppc/spapr version has a bug where QEMU does a KVM_RUN on the vcpu
before cs->halted is set to 1, causing the vcpu to run while it's still in
an unitialized state (more details in patch 3).

Peter Maydell mentioned the ARM start-powered-off property and
Eduardo Habkost suggested making it generic, so this patch series does
that, for all cases which I was able to find via grep in the code.

The only problem is that I was only able to test these changes on a ppc64le
pseries KVM guest, so except for patches 2 and 3, all others are only
build-tested. Also, my grasp of QOM lifecycle is basically non-existant so
please be aware of that when reviewing this series.

The last 3 patches I think are good cleanups but I'm even less confident in
their correctness compared to the other patches, so I marked them as RFC.

Applies cleanly on yesterday's master.

Thiago Jung Bauermann (9):
  target/arm: Move start-powered-off property to generic CPUState
  target/arm: Move setting of CPU halted state to generic code
  ppc/spapr: Use start-powered-off CPUState property
  ppc/e500: Use start-powered-off CPUState property
  mips/cps: Use start-powered-off CPUState property
  sparc/sun4m: Use start-powered-off CPUState property
  sparc/sun4m: Don't set CPUState::halted in cpu_devinit()
  sparc/sun4m: Use one cpu_reset() function for main and secondary CPUs
  target/s390x: Use start-powered-off CPUState property

 exec.c  |  1 +
 hw/core/cpu.c   |  2 +-
 hw/mips/cps.c   |  6 +++---
 hw/ppc/e500.c   | 10 +++---
 hw/ppc/spapr_cpu_core.c | 12 +++-
 hw/sparc/sun4m.c| 23 +--
 include/hw/core/cpu.h   |  4 
 target/arm/cpu.c|  4 +---
 target/arm/cpu.h|  3 ---
 target/arm/kvm32.c  |  2 +-
 target/arm/kvm64.c  |  2 +-
 target/s390x/cpu.c  |  3 ++-
 12 files changed, 33 insertions(+), 39 deletions(-)

Re: [PATCH v1] migration: tls: unref creds after used

2020-07-21 Thread Zhenyu Ye

On 2020/7/21 19:54, Daniel P. Berrangé wrote:
> On Fri, Jul 17, 2020 at 05:19:43PM +0800, Zhenyu Ye wrote:
>> We add the reference of creds in migration_tls_get_creds(),
>> but there was no place to unref it.  So the OBJECT(creds) will
>> never be freed and result in memory leak.
>>
>> Unref the creds after creating the tls-channel server/client.
>> ...
>>
>> +
>> +cleanup:
>> +object_unref(OBJECT(creds));
>>  }
> 
> Simpler to just change  migration_tls_get_creds() to remove the
> object_ref() call, since we're fine to use the borrowed reference
> in these methods.
> 

Thanks for your review.  I have sent a new series since the current
title "unref creds after used" is no longer appropriate:

https://lore.kernel.org/qemu-devel/20200722033228.71-1-yezhen...@huawei.com/

Thanks,
Zhenyu

Re: please try to avoid sending pullreqs late on release-candidate day




On 2020/7/21 下午11:56, Peter Maydell wrote:

It is not helpful if everybody sends their pullrequests late
on the Tuesday afternoon, as there just isn't enough time in the
day to merge test and apply them all before I have to cut the tag.
Please, if you can, try to send pullrequests earlier, eg Monday.



Ok, will do.

Thanks




thanks
-- PMM

[PATCH v1] migration: tls: fix memory leak in migration_tls_get_creds

2020-07-21 Thread Zhenyu Ye

Currently migration_tls_get_creds() adds the reference of creds
but there was no place to unref it.  So the OBJECT(creds) will
never be freed and result in memory leak.

The leak stack:
Direct leak of 104 byte(s) in 1 object(s) allocated from:
#0 0xa88bd20b in __interceptor_malloc (/usr/lib64/libasan.so.4+0xd320b)
#1 0xa7f0cb1b in g_malloc (/usr/lib64/libglib-2.0.so.0+0x58b1b)
#2 0x14b58cb in object_new_with_type qom/object.c:634
#3 0x14b597b in object_new qom/object.c:645
#4 0x14c0e4f in user_creatable_add_type qom/object_interfaces.c:59
#5 0x141c78b in qmp_object_add qom/qom-qmp-cmds.c:312
#6 0x140e513 in qmp_marshal_object_add qapi/qapi-commands-qom.c:279
#7 0x176ba97 in do_qmp_dispatch qapi/qmp-dispatch.c:165
#8 0x176bee7 in qmp_dispatch qapi/qmp-dispatch.c:208
#9 0x136e337 in monitor_qmp_dispatch monitor/qmp.c:150
#10 0x136eae3 in monitor_qmp_bh_dispatcher monitor/qmp.c:239
#11 0x1852e93 in aio_bh_call util/async.c:89
#12 0x18531b7 in aio_bh_poll util/async.c:117
#13 0x18616bf in aio_dispatch util/aio-posix.c:459
#14 0x1853f37 in aio_ctx_dispatch util/async.c:268
#15 0xa7f06a7b in g_main_context_dispatch 
(/usr/lib64/libglib-2.0.so.0+0x52a7b)

Since we're fine to use the borrowed reference when using the creds,
so just remove the object_ref() in migration_tls_get_creds().

Signed-off-by: Zhenyu Ye 
---
 migration/tls.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/tls.c b/migration/tls.c
index 5171afc6c4..7a02ec8656 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -58,7 +58,6 @@ migration_tls_get_creds(MigrationState *s,
 return NULL;
 }
 
-object_ref(OBJECT(ret));
 return ret;
 }
 
-- 
2.19.1

Re: [PATCH v3] e1000e: using bottom half to send packets




On 2020/7/21 下午11:17, Li Qiang wrote:

Alexander Bulekov reported a UAF bug related e1000e packets send.

-->https://bugs.launchpad.net/qemu/+bug/1886362

This is because the guest trigger a e1000e packet send and set the
data's address to e1000e's MMIO address. So when the e1000e do DMA
it will write the MMIO again and trigger re-entrancy and finally
causes this UAF.

Paolo suggested to use a bottom half whenever MMIO is doing complicate
things in here:
-->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html

Reference here:
'The easiest solution is to delay processing of descriptors to a bottom
half whenever MMIO is doing something complicated.  This is also better
for latency because it will free the vCPU thread more quickly and leave
the work to the I/O thread.'

This patch fixes this UAF.

Reported-by: Alexander Bulekov 
Signed-off-by: Li Qiang 
---
Change since v2:
1. Add comments for the tx bh schdule when VM resumes
2. Leave the set ics code in 'e1000e_start_xmit'
3. Cancel the tx bh and reset tx_waiting in e1000e_core_reset



So based on our discussion this is probably not sufficient. It solves 
the issue TX re-entrancy but not RX (e.g RX DMA to RDT?) Or is e1000e's 
RX is reentrant?


Thanks




Change since v1:
Per Jason's review here:
-- https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg05368.html
1. Cancel and schedule the tx bh when VM is stopped or resume
2. Add a tx_burst for e1000e configuration to throttle the bh execution
3. Add a tx_waiting to record whether the bh is pending or not
Don't use BQL in the tx_bh handler as when tx_bh is executed, the BQL is
acquired.

  hw/net/e1000e.c  |   6 +++
  hw/net/e1000e_core.c | 107 +++
  hw/net/e1000e_core.h |   8 
  3 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index fda34518c9..24e35a78bf 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -77,10 +77,14 @@ typedef struct E1000EState {
  
  bool disable_vnet;
  
+int32_t tx_burst;

+
  E1000ECore core;
  
  } E1000EState;
  
+#define TX_BURST 256

+
  #define E1000E_MMIO_IDX 0
  #define E1000E_FLASH_IDX1
  #define E1000E_IO_IDX   2
@@ -263,6 +267,7 @@ static void e1000e_core_realize(E1000EState *s)
  {
  s->core.owner = >parent_obj;
  s->core.owner_nic = s->nic;
+s->core.tx_burst = s->tx_burst;
  }
  
  static void

@@ -665,6 +670,7 @@ static Property e1000e_properties[] = {
  e1000e_prop_subsys_ven, uint16_t),
  DEFINE_PROP_SIGNED("subsys", E1000EState, subsys, 0,
  e1000e_prop_subsys, uint16_t),
+DEFINE_PROP_INT32("x-txburst", E1000EState, tx_burst, TX_BURST),
  DEFINE_PROP_END_OF_LIST(),
  };
  
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c

index bcd186cac5..2fdfc23204 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -910,18 +910,17 @@ e1000e_rx_ring_init(E1000ECore *core, E1000E_RxRing *rxr, 
int idx)
  }
  
  static void

-e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
+e1000e_start_xmit(struct e1000e_tx *q)
  {
+E1000ECore *core = q->core;
  dma_addr_t base;
  struct e1000_tx_desc desc;
-bool ide = false;
-const E1000E_RingInfo *txi = txr->i;
-uint32_t cause = E1000_ICS_TXQE;
+const E1000E_RingInfo *txi;
+E1000E_TxRing txr;
+int32_t num_packets = 0;
  
-if (!(core->mac[TCTL] & E1000_TCTL_EN)) {

-trace_e1000e_tx_disabled();
-return;
-}
+e1000e_tx_ring_init(core, , q - >tx[0]);
+txi = txr.i;
  
  while (!e1000e_ring_empty(core, txi)) {

  base = e1000e_ring_head_descr(core, txi);
@@ -931,14 +930,24 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing 
*txr)
  trace_e1000e_tx_descr((void *)(intptr_t)desc.buffer_addr,
desc.lower.data, desc.upper.data);
  
-e1000e_process_tx_desc(core, txr->tx, , txi->idx);

-cause |= e1000e_txdesc_writeback(core, base, , , txi->idx);
+e1000e_process_tx_desc(core, txr.tx, , txi->idx);
+q->cause |= e1000e_txdesc_writeback(core, base, ,
+>ide, txi->idx);
  
  e1000e_ring_advance(core, txi, 1);

+if (++num_packets >= core->tx_burst) {
+break;
+}
+}
+
+if (num_packets >= core->tx_burst) {
+qemu_bh_schedule(q->tx_bh);
+q->tx_waiting = 1;
+return;
  }
  
-if (!ide || !e1000e_intrmgr_delay_tx_causes(core, )) {

-e1000e_set_interrupt_cause(core, cause);
+if (!q->ide || !e1000e_intrmgr_delay_tx_causes(core, >cause)) {
+e1000e_set_interrupt_cause(core, q->cause);
  }
  }
  
@@ -2423,32 +2432,41 @@ e1000e_set_dbal(E1000ECore *core, int index, uint32_t val)

  static void
  e1000e_set_tctl(E1000ECore *core, int index, uint32_t val)
  {
-E1000E_TxRing txr;
  core->mac[index] = val;
  
  if

Re: [Bug 1886362] [NEW] Heap use-after-free in lduw_he_p through e1000e_write_to_rx_buffers

On 2020/7/21 下午9:46, Li Qiang wrote:

Jason Wang 于2020年7月21日周二 下午9:21写道：

On 2020/7/21 下午8:31, Peter Maydell wrote:

On Wed, 15 Jul 2020 at 09:36, Jason Wang wrote:

I think the point is to make DMA to MMIO work as real hardware.

I wouldn't care to give a 100% guarantee that asking a real
h/w device to DMA to itself didn't cause it to misbehave :-)
It's more likely to happen-to-work because the DMA engine bit
of a real h/w device is going to be decoupled somewhat from
the respond-to-memory-transactions-for-registers logic, but
it probably wasn't something the designers were actively
thinking about either...

I think some device want such peer to peer transactions:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/pci/p2pdma.rst

For
e1000e and other networking devices we need make sure such DMA doesn't
break anything.

Yeah, this is the interesting part for QEMU. How should we
structure devices that do DMA so that we can be sure that
the device emulation at least doesn't crash? We could have
a rule that all devices that do DMA must always postpone
all of that DMA to a bottom-half, but that's a lot of
refactoring of a lot of device code...

It looks to me the issue happens only for device with loopback

IMO I think this is not related-loopback.

It happens when the guest write the MMIO address to the device's
DMA-related registers.
The one we see UAF occurs in loopback device because the same
structure uses in re-entry.
But we can't say there are no issue for non-loopback device.

Yes.

Simply git grep loopback in hw/net tells me we probably need only to
audit dp8393x and rtl8139.

Qiang, want to help to audit those devices?

No problem. Once I finish the e1000e patch I will try to audit those and
also try to audit some no-loopback device re-entry issue.

Thanks.

Thanks,
Li Qiang

Thanks

thanks
-- PMM

Re: [Bug 1886362] [NEW] Heap use-after-free in lduw_he_p through e1000e_write_to_rx_buffers

On 2020/7/21 下午9:44, Peter Maydell wrote:

On Tue, 21 Jul 2020 at 14:21, Jason Wang wrote:

On 2020/7/21 下午8:31, Peter Maydell wrote:

On Wed, 15 Jul 2020 at 09:36, Jason Wang wrote:

I think the point is to make DMA to MMIO work as real hardware.

I think some device want such peer to peer transactions:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/pci/p2pdma.rst

That's a device DMAing to another device, not DMAing to *itself*
(device-to-another-device DMA should work fine in QEMU). And only
a very few devices will ever be sensible targets of the DMA --
basically things like nvme that have a looks-like-memory area,
or special cases like doorbell registers.

Well, my understanding is:

- it's not about whether or not we have an actual device that can do DMA
into itself but whether it's allowed by PCI spec
- it's not really matter whether or not it tries to DMA into itself.
Devices could be taught to DMA into each other's RX:

e1000e(1) RX DMA to e1000e(2) MMIO (RX)
e1000e(2) RX DMA to e1000e(1) RX

So we get re-reentrancy again.

It looks to me the issue happens only for device with loopback

I think in principle we have a problem for any device that
(a) has memory mapped registers and (b) does DMA reads
whose address is guest-controlled. Loopback isn't a
requirement -- if the guest programs, say, an RX descriptor
base address to point at the device's own registers, you
get exactly the same kind of unexpected-reentrancy.

Right, so about the solution, instead of refactoring DMA I wonder we can
simply detect and fail the RX by device itself.

Thanks

thanks
-- PMM

RE: [PATCH Kernel v24 0/8] Add UAPIs to support migration for VFIO devices

2020-07-21 Thread Tian, Kevin

> From: Xiang Zheng
> Sent: Wednesday, July 22, 2020 10:56 AM
> 
> Hi Alex,
> 
> Thank you for your suggestion.
> 
> On 2020/7/22 6:43, Alex Williamson wrote:
> > On Tue, 21 Jul 2020 10:43:21 +0800
> > Xiang Zheng  wrote:
> >
> >> Hi Kirti,
> >>
> >> Sorry to disturb you since this patch set has been merged, and I cannot
> >> receive the qemu-side emails about this patch set.
> >>
> >> We are going to support migration for VFIO devices which support dirty
> >> pages tracking.
> >>
> >> And we also plan to leverage SMMU HTTU feature to do the dirty pages
> >> tracking for the devices which don't support dirty pages tracking.
> >>
> >> For the above two cases, which side determines to choose IOMMU driver
> or
> >> vendor driver to do dirty bitmap tracking, Qemu or VFIO?
> >>
> >> In brief, if both IOMMU and VFIO devices support dirty pages tracking,
> >> we can check the capability and prefer to track dirty pages on device
> >> vendor driver which is more efficient.
> >>
> >> The qusetion is which side to do the check and selection? In my opinion,
> >> Qemu/userspace seems more suitable.
> >
> > Dirty page tracking is consolidated at the vfio container level.
> > Userspace has no basis for determining or interface for selecting a
> > dirty bitmap provider, so I would disagree that QEMU should play any
> > role here.  The container dirty bitmap tries to provide the finest
> > granularity available based on the support of all the devices/groups
> > managed by the container.  If there are groups attached to the
> > container that have not participated in page pinning, then we consider
> > all DMA mappings within the container as persistently dirty.  Once all
> > of the participants subscribe to page pinning, the dirty scope is
> > reduced to the pinned pages.  IOMMU support for dirty page logging would
> > introduce finer granularity yet, which we would probably prefer over
> > page pinning, but interfaces for this have not been devised.
> 
> Kevin and his colleagues may add these APIs in the future.
> We also plan to support these interfaces on SMMU driver and afterwards we
> can have a further discussion.

Yes, we are working on that part. Generally speaking I agree with Alex
that the kernel should decide dirty bitmap provider and IOMMU dirty
logging is likely preferred over page pinning. Page pinning either provides
only coarse-grained dirty info (e.g. 100's MB pinned pages observed in
vGPU case) or when reaching finer granularity it may affect fast-path 
performance (e.g. driver may have to intercept fast-path operations to
track dirtied pages closely). IOMMU provides an architectural and
lighter approach for tracking dirty pages.

Thanks
Kevin

> 
> >
> > Ideally userspace should be unaware of any of this, the benefit would
> > be seen transparently by having a more sparsely filled dirty bitmap,
> > which more accurately reflects how memory is actually being dirtied.
> 
> Yes, indeed.
> 
> --
> Thanks,
> Xiang

Re: [PATCH Kernel v24 0/8] Add UAPIs to support migration for VFIO devices

2020-07-21 Thread Xiang Zheng

Hi Alex,

Thank you for your suggestion.

On 2020/7/22 6:43, Alex Williamson wrote:
> On Tue, 21 Jul 2020 10:43:21 +0800
> Xiang Zheng  wrote:
> 
>> Hi Kirti,
>>
>> Sorry to disturb you since this patch set has been merged, and I cannot
>> receive the qemu-side emails about this patch set.
>>
>> We are going to support migration for VFIO devices which support dirty
>> pages tracking.
>>
>> And we also plan to leverage SMMU HTTU feature to do the dirty pages
>> tracking for the devices which don't support dirty pages tracking.
>>
>> For the above two cases, which side determines to choose IOMMU driver or
>> vendor driver to do dirty bitmap tracking, Qemu or VFIO?
>>
>> In brief, if both IOMMU and VFIO devices support dirty pages tracking,
>> we can check the capability and prefer to track dirty pages on device
>> vendor driver which is more efficient.
>>
>> The qusetion is which side to do the check and selection? In my opinion,
>> Qemu/userspace seems more suitable.
> 
> Dirty page tracking is consolidated at the vfio container level.
> Userspace has no basis for determining or interface for selecting a
> dirty bitmap provider, so I would disagree that QEMU should play any
> role here.  The container dirty bitmap tries to provide the finest
> granularity available based on the support of all the devices/groups
> managed by the container.  If there are groups attached to the
> container that have not participated in page pinning, then we consider
> all DMA mappings within the container as persistently dirty.  Once all
> of the participants subscribe to page pinning, the dirty scope is
> reduced to the pinned pages.  IOMMU support for dirty page logging would
> introduce finer granularity yet, which we would probably prefer over
> page pinning, but interfaces for this have not been devised.

Kevin and his colleagues may add these APIs in the future.
We also plan to support these interfaces on SMMU driver and afterwards we
can have a further discussion.

> 
> Ideally userspace should be unaware of any of this, the benefit would
> be seen transparently by having a more sparsely filled dirty bitmap,
> which more accurately reflects how memory is actually being dirtied.

Yes, indeed.

-- 
Thanks,
Xiang

Re: [PATCH 00/11] RISC-V risu porting

2020-07-21 Thread LIU Zhiwei


Ping.

On 2020/7/12 0:16, LIU Zhiwei wrote:

In contrast to the RFC, add more instructions description. Now it supports
RV64IMACFD. Some cross verifications have been done, such as comparison
between QEMU and TinyEMU, and comparison between QEMU and C906 FPGA.

Now it has some productive.

Features:
* support RV64IMACFD.
* support multi-precision float point.
* support accurate special values generation.

Todo:
* support RVV and RVP.


LIU Zhiwei (11):
   riscv: Add RV64I instructions description
   riscv: Add RV64M instructions description
   riscv: Add RV64A instructions description
   riscv: Add RV64F instructions description
   riscv: Add RV64D instructions description
   riscv: Add RV64C instructions description
   riscv: Generate payload scripts
   riscv: Add standard test case
   riscv: Define riscv struct reginfo
   riscv: Implement payload load interfaces
   riscv: Add configure script

  configure  |   4 +-
  risu_reginfo_riscv64.c | 132 +
  risu_reginfo_riscv64.h |  28 ++
  risu_riscv64.c |  47 +++
  risugen_riscv.pm   | 643 +
  rv64.risu  | 466 +
  rv64c.risu |  97 +++
  test_riscv64.s |  85 ++
  upstream/configure | 204 +
  9 files changed, 1705 insertions(+), 1 deletion(-)
  create mode 100644 risu_reginfo_riscv64.c
  create mode 100644 risu_reginfo_riscv64.h
  create mode 100644 risu_riscv64.c
  create mode 100644 risugen_riscv.pm
  create mode 100644 rv64.risu
  create mode 100644 rv64c.risu
  create mode 100644 test_riscv64.s
  create mode 100644 upstream/configure

[ANNOUNCE] QEMU 5.1.0-rc1 is now available

2020-07-21 Thread Michael Roth

Hello,

On behalf of the QEMU Team, I'd like to announce the availability of the
second release candidate for the QEMU 5.1 release.  This release is meant
for testing purposes and should not be used in a production environment.

  http://download.qemu-project.org/qemu-5.1.0-rc1.tar.xz
  http://download.qemu-project.org/qemu-5.1.0-rc1.tar.xz.sig

You can help improve the quality of the QEMU 5.1 release by testing this
release and reporting bugs on Launchpad:

  https://bugs.launchpad.net/qemu/

The release plan, as well a documented known issues for release
candidates, are available at:

  http://wiki.qemu.org/Planning/5.1

Please add entries to the ChangeLog for the 5.1 release below:

  http://wiki.qemu.org/ChangeLog/5.1

Thank you to everyone involved!

Changes since rc0:

c8004fe6bb: Update version for v5.1.0-rc1 release (Peter Maydell)
4a01e27ddc: iotests: Test sparseness for qemu-img convert -n (Kevin Wolf)
0dde9fd12f: qom: Make info qom-tree sort children more efficiently (Markus 
Armbruster)
61b3043965: qcow2: Implement v2 zero writes with discard if possible (Kevin 
Wolf)
bae127d4dc: file-posix: Handle `EINVAL` fallocate return value (Antoine Damhet)
5bd929d2ff: qom: Document object_get_canonical_path() returns malloced string 
(Markus Armbruster)
7a309cc95b: qom: Change object_get_canonical_path_component() not to malloc 
(Markus Armbruster)
5519724a13: hw/net/xgmac: Fix buffer overflow in xgmac_enet_send() (Mauro 
Matteo Cascella)
e219d30910: hw/net: Added plen fix for IPv6 (Andrew)
cbf97d5b79: qapi: Fix visit_type_STRUCT() not to fail for null object (Markus 
Armbruster)
1d719ddc35: block: fix bdrv_aio_cancel() for ENOMEDIUM requests (Stefan 
Hajnoczi)
d87350b065: module: ignore NULL type (Gerd Hoffmann)
d97df4b84b: qxl: fix modular builds with dtrace (Gerd Hoffmann)
8e67fda2dd: xhci: fix valid.max_access_size to access address registers 
(Laurent Vivier)
0fca43de1b: qemu-iotests: add testcase for bz #1857490 (Maxim Levitsky)
662d0c5392: block/crypto: disallow write sharing by default (Maxim Levitsky)
7ad36e2e24: hw: Mark nd_table[] misuse in realize methods FIXME (Markus 
Armbruster)
2b0650205b: msf2: Unbreak device-list-properties for "msf-soc" (Markus 
Armbruster)
6184e5fb42: MAINTAINERS: Extend the device fuzzing section (Thomas Huth)
09a14f586c: docs/fuzz: add instructions for generating a coverage report 
(Alexander Bulekov)
19a91e4af8: docs/fuzz: add information about useful libFuzzer flags (Alexander 
Bulekov)
ee16da12d7: docs/fuzz: describe building fuzzers with enable-sanitizers 
(Alexander Bulekov)
dd0162653c: fuzz: build without AddressSanitizer, by default (Alexander Bulekov)
48eac10197: gitlab-ci.yml: Add oss-fuzz build tests (Alexander Bulekov)
bcbad8b05c: fuzz: Fix leak when assembling datadir path string (Alexander 
Bulekov)
7cee363bc2: scripts/oss-fuzz: Limit target list to i386-softmmu (Thomas Huth)
6a0b7505f1: docs/system: Document the arm virt board (Peter Maydell)
bb309000c8: docs/system: Briefly document gumstix boards (Peter Maydell)
b76b60f59b: docs/system: Briefly document collie board (Peter Maydell)
2d21dd17c5: docs/system: Briefly document canon-a1100 board (Peter Maydell)
3f410039b7: hw/arm/armsse: Assert info->num_cpus is in-bounds in 
armsse_realize() (Peter Maydell)
cd07d7f9f5: qdev: Document GPIO related functions (Peter Maydell)
46ea1be1ee: qdev: Document qdev_unrealize() (Peter Maydell)
b51238e251: qdev: Move doc comments from qdev.c to qdev-core.h (Peter Maydell)
8edbca515c: util: Implement qemu_get_thread_id() for OpenBSD (David CARLIER)
19bd6aafbd: hw/arm/virt: Disable memory hotplug when MTE is enabled (Richard 
Henderson)
7f6185ed9c: hw/arm/virt: Error for MTE enabled with KVM (Richard Henderson)
6f4e1405b9: hw/arm/virt: Enable MTE via a machine property (Richard Henderson)
d69cda7ed7: Makefile: Remove config-devices.mak on "make clean" (Peter Maydell)
b25fbd6a13: pseries: Update SLOF firmware image (Alexey Kardashevskiy)
a6030d7e0b: spapr: Add a new level of NUMA for GPUs (Reza Arbab)
a4beb5f5d4: spapr_pci: Robustify support of PCI bridges (Greg Kurz)
14de3d4ac5: ppc/pnv: Make PSI device types not user creatable (Greg Kurz)
ba3c35d9c4: tcg/cpu-exec: precise single-stepping after an interrupt (Richard 
Henderson)
e3f7320caa: ipmi: add SET_SENSOR_READING command (Cédric Le Goater)
789101b73d: ipmi: Fix a man page entry (Corey Minyard)
323679da77: ipmi: Add man page pieces for the IPMI PCI devices (Corey Minyard)
7cb015197b: migration/block-dirty-bitmap: fix add_bitmaps_to_list (Vladimir 
Sementsov-Ogievskiy)
a8c5cf27c9: file-posix: Fix leaked fd in raw_open_common() error path (Kevin 
Wolf)
bca5283bd4: file-posix: Fix check_hdev_writable() with auto-read-only (Kevin 
Wolf)
20eaf1bf6e: file-posix: Move check_hdev_writable() up (Kevin Wolf)
5edc85571e: file-posix: Allow byte-aligned O_DIRECT with NFS (Kevin Wolf)
9c60a5d197: block: Require aligned image size to avoid assertion failure (Kevin 
Wolf)
d047cfa78d: iotests: test shutdown when bitmap is exported

[PATCH 1/1] scripts/performance: Add bisect.py script

2020-07-21 Thread Ahmed Karaman

Python script that locates the commit that caused a performance
degradation or improvement in QEMU using the git bisect command
(binary search).

Syntax:
bisect.py [-h] -s,--start START [-e,--end END] [-q,--qemu QEMU] \
--target TARGET --tool {perf,callgrind} -- \
 []

[-h] - Print the script arguments help message
-s,--start START - First commit hash in the search range
[-e,--end END] - Last commit hash in the search range
(default: Latest commit)
[-q,--qemu QEMU] - QEMU path.
(default: Path to a GitHub QEMU clone)
--target TARGET - QEMU target name
--tool {perf,callgrind} - Underlying tool used for measurements

Example of usage:
bisect.py --start=fdd76fecdd --qemu=/path/to/qemu --target=ppc \
--tool=perf -- coulomb_double-ppc -n 1000

Example output:
Start Commit Instructions: 12,710,790,060
End Commit Instructions:   13,031,083,512
Performance Change:-2.458%

Estimated Number of Steps: 10

*BISECT STEP 1*
Instructions:13,031,097,790
Status:  slow commit
*BISECT STEP 2*
Instructions:12,710,805,265
Status:  fast commit
*BISECT STEP 3*
Instructions:13,031,028,053
Status:  slow commit
*BISECT STEP 4*
Instructions:12,711,763,211
Status:  fast commit
*BISECT STEP 5*
Instructions:13,031,027,292
Status:  slow commit
*BISECT STEP 6*
Instructions:12,711,748,738
Status:  fast commit
*BISECT STEP 7*
Instructions:12,711,748,788
Status:  fast commit
*BISECT STEP 8*
Instructions:13,031,100,493
Status:  slow commit
*BISECT STEP 9*
Instructions:12,714,472,954
Status:  fast commit
BISECT STEP 10*
Instructions:12,715,409,153
Status:  fast commit
BISECT STEP 11*
Instructions:12,715,394,739
Status:  fast commit

*BISECT RESULT*
commit 0673ecdf6cb2b1445a85283db8cbacb251c46516
Author: Richard Henderson 
Date:   Tue May 5 10:40:23 2020 -0700

softfloat: Inline float64 compare specializations

Replace the float64 compare specializations with inline functions
that call the standard float64_compare{,_quiet} functions.
Use bool as the return type.
***

Signed-off-by: Ahmed Karaman 
---
 scripts/performance/bisect.py | 374 ++
 1 file changed, 374 insertions(+)
 create mode 100755 scripts/performance/bisect.py

diff --git a/scripts/performance/bisect.py b/scripts/performance/bisect.py
new file mode 100755
index 00..869cc69ef4
--- /dev/null
+++ b/scripts/performance/bisect.py
@@ -0,0 +1,374 @@
+#!/usr/bin/env python3
+
+#  Locate the commit that caused a performance degradation or improvement in
+#  QEMU using the git bisect command (binary search).
+#
+#  Syntax:
+#  bisect.py [-h] -s,--start START [-e,--end END] [-q,--qemu QEMU] \
+#  --target TARGET --tool {perf,callgrind} -- \
+#   []
+#
+#  [-h] - Print the script arguments help message
+#  -s,--start START - First commit hash in the search range
+#  [-e,--end END] - Last commit hash in the search range
+# (default: Latest commit)
+#  [-q,--qemu QEMU] - QEMU path.
+#  (default: Path to a GitHub QEMU clone)
+#  --target TARGET - QEMU target name
+#  --tool {perf,callgrind} - Underlying tool used for measurements
+
+#  Example of usage:
+#  bisect.py --start=fdd76fecdd --qemu=/path/to/qemu --target=ppc --tool=perf \
+#  -- coulomb_double-ppc -n 1000
+#
+#  This file is a part of the project "TCG Continuous Benchmarking".
+#
+#  Copyright (C) 2020  Ahmed Karaman 
+#  Copyright (C) 2020  Aleksandar Markovic 
+#
+#  This program is free software: you can redistribute it and/or modify
+#  it under the terms of the GNU General Public License as published by
+#  the Free Software Foundation, either version 2 of the License, or
+#  (at your option) any later version.
+#
+#  This program is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+#  GNU General Public License for more details.
+#
+#  You should have received a copy of the GNU General Public License
+#  along with this program. If not, see .
+
+import argparse
+import multiprocessing
+import tempfile
+import os
+import shutil
+import subprocess
+import sys
+
+
+ GIT WRAPPERS 
+def git_bisect(qemu_path, command, args=None):
+"""
+Wrapper

[PATCH 0/1] Add bisect.py script

2020-07-21 Thread Ahmed Karaman

Hi,

This series adds the new bisect.py script introduced in report 5 of
the "TCG Continuous Benchmarking" GSoC project.

The script is used for locating the commit that caused a performance
degradation or improvement in QEMU using the git bisect command
(binary search).

To learn more about how the script works and how it can be used for
detecting two commits, one that introduced a performance degradation
in PowerPC targets, and the other introducing a performance
improvement in MIPS, please check the
"Finding Commits Affecting QEMU Performance" report.

Report link:
https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg05769.html

Best regards,
Ahmed Karaman

Ahmed Karaman (1):
  scripts/performance: Add bisect.py script

 scripts/performance/bisect.py | 374 ++
 1 file changed, 374 insertions(+)
 create mode 100755 scripts/performance/bisect.py

-- 
2.17.1

Re: [PATCH Kernel v24 0/8] Add UAPIs to support migration for VFIO devices

2020-07-21 Thread Alex Williamson

On Tue, 21 Jul 2020 10:43:21 +0800
Xiang Zheng  wrote:

> Hi Kirti,
> 
> Sorry to disturb you since this patch set has been merged, and I cannot
> receive the qemu-side emails about this patch set.
> 
> We are going to support migration for VFIO devices which support dirty
> pages tracking.
> 
> And we also plan to leverage SMMU HTTU feature to do the dirty pages
> tracking for the devices which don't support dirty pages tracking.
> 
> For the above two cases, which side determines to choose IOMMU driver or
> vendor driver to do dirty bitmap tracking, Qemu or VFIO?
> 
> In brief, if both IOMMU and VFIO devices support dirty pages tracking,
> we can check the capability and prefer to track dirty pages on device
> vendor driver which is more efficient.
> 
> The qusetion is which side to do the check and selection? In my opinion,
> Qemu/userspace seems more suitable.

Dirty page tracking is consolidated at the vfio container level.
Userspace has no basis for determining or interface for selecting a
dirty bitmap provider, so I would disagree that QEMU should play any
role here.  The container dirty bitmap tries to provide the finest
granularity available based on the support of all the devices/groups
managed by the container.  If there are groups attached to the
container that have not participated in page pinning, then we consider
all DMA mappings within the container as persistently dirty.  Once all
of the participants subscribe to page pinning, the dirty scope is
reduced to the pinned pages.  IOMMU support for dirty page logging would
introduce finer granularity yet, which we would probably prefer over
page pinning, but interfaces for this have not been devised.

Ideally userspace should be unaware of any of this, the benefit would
be seen transparently by having a more sparsely filled dirty bitmap,
which more accurately reflects how memory is actually being dirtied.
Thanks,

Alex

Re: [REPORT] [GSoC - TCG Continuous Benchmarking] [#5] Finding Commits Affecting QEMU Performance

2020-07-21 Thread Ahmed Karaman

On Tue, Jul 21, 2020 at 1:54 PM Alex Bennée  wrote:
>
>
> Ahmed Karaman  writes:
>
> > Hi,
> >
> > The fifth report of the TCG Continuous Benchmarking project concludes
> > a mini-series of three reports that dealt with the performance
> > comparison and analysis of QEMU 5.0 and 5.1-pre-soft-freeze.
> >
> > The report presents a new Python script that utilizes "git bisect" for
> > running a binary search within a specified range of commits to
> > automatically detect the commit causing a performance improvement or
> > degradation.
>
> Excellent stuff.

Thanks for your continued support!

>
> > The new script is then used to find the commit introducing the PowerPC
> > performance degradation as well as that introducing the performance
> > improvement in MIPS. The results obtained for both commits proves the
> > correctness of the conclusions and analyses presented in the two
> > previous reports.
>
> I can certainly envision a mechanism where 0673ec slows things down. I
> wonder if it would come back if instead of inline function calls we
> ended up making concrete flattend versions, e.g.:
>
> bool QEMU_FLATTEN float64_eq(float64 a, float64 b, float_status *s)
> {
> return float64_compare(a, b, s) == float_relation_equal;
> }
>
> PPC is of course more affected by these changes than others because
> HARDFLOAT never gets a chance to kick in. Looking at the objdump of
> f64_compare there should surely be an opportunity to loose some of the
> branches when looking for a certain test result?

Interesting, I will try to tinker a little bit with the float64
functions and will let you know if I find anything interesting.

>
> >
> > Report link:
> > https://ahmedkrmn.github.io/TCG-Continuous-Benchmarking/Finding-Commits-Affecting-QEMU-Performance/
> >
> > Previous reports:
> > Report 1 - Measuring Basic Performance Metrics of QEMU:
> > https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg06692.html
> > Report 2 - Dissecting QEMU Into Three Main Parts:
> > https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09441.html
> > Report 3 - QEMU 5.0 and 5.1-pre-soft-freeze Dissect Comparison:
> > https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg01978.html
> > Report 4 - Listing QEMU Helpers and Function Callees:
> > https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04227.html
> >
> > Best regards,
> > Ahmed Karaman
>
>
> --
> Alex Bennée

Best regards,
Ahmed Karaman

Re: [PATCH v4 2/2] nvme: allow cmb and pmr to be enabled on same device

2020-07-21 Thread Andrzej Jakowski

On 7/15/20 1:06 AM, Klaus Jensen wrote:
> Hi Andrzej,
> 
> I've not been ignoring this, but sorry for not following up earlier.
> 
> I'm hesitent to merge anything that very obviously breaks an OS that we
> know is used a lot to this using this device. Also because the issue has
> not been analyzed well enough to actually know if this is a QEMU or
> kernel issue.

Hi Klaus,

Thx for your response! I understand your hesitance on merging stuff that
obviously breaks guest OS. 

> 
> Now, as far as I can test, having the MSI-X vector table and PBA in BAR
> 0, PMR in BAR 2 and CMB in BAR 4 seems to make everyone happy
> (irregardless of IOMMU on/off).
> 
> Later, when the issue is better understood, we can add options to set
> offsets, BIRs etc.
> 
> The patch below replaces your "[PATCH v4 2/2] nvme: allow cmb and pmr to
> be enabled" (but still requires "[PATCH v4 1/2] ...") and applies to
> git://git.infradead.org/qemu-nvme.git nvme-next branch.
> 
> Can you reproduce the issues with that patch? I can't on a stock Arch
> Linux 5.7.5-arch1-1 kernel.

While I'm happy that approach with MSIX and PBA in BAR0 works fine, I
feel that investigation part why it works while mine doesn't is
missing. It looks to me that both patches are basically following same 
approach: create memory subregion and overlay on top of other memory
region. Why one works and the other doesn't then?

Having in mind that, I have recently focused on understanding problem.
I observed that when guest assigns address to BAR4, addr field in
nvme-bar4 memory region gets populated, but it doesn't get automatically
populated in ctrl_mem (cmb) memory subregion, so later when nvme_addr_is_cmb() 
is called address check works incorrectly and as a consequence vmm does dma 
read instead of memcpy.
I created a patch that sets correct address on ctrl_mem subregion and guest 
OS boots up correctly.

When I looked into pci and memory region code I noticed that indeed address
is only assigned to top level memory region but not to contained subregions.
I think that because in your approach cmb grabs whole bar exclusively it works
fine.

Here is my question (perhaps pci folks can help answer :)): if we consider 
memory region overlapping for pci devices as valid use case should pci 
code on configuration cycles walk through all contained subregion and
update addr field accordingly?

Thx!

Re: please try to avoid sending pullreqs late on release-candidate day

2020-07-21 Thread Gerd Hoffmann

On Tue, Jul 21, 2020 at 04:56:25PM +0100, Peter Maydell wrote:
> It is not helpful if everybody sends their pullrequests late
> on the Tuesday afternoon, as there just isn't enough time in the
> day to merge test and apply them all before I have to cut the tag.
> Please, if you can, try to send pullrequests earlier, eg Monday.

I usually try, but it didn't work out this time due to patches coming
in late ...

Speaking of testing:  What is the state of gitlab ci?  How much of the
testing has been migrated over?  I've noticed I can push branches and
tags to a qemu fork @ gitlab.com and gitlab ci runs a bunch of tests.

What is the best way to indicate that the tag did pass gitlab ci
already?  Maybe simply send a pull request with gitlab.com url?

take care,
  Gerd

[Bug 1888431] [NEW] v5.1.0-rc1 build fails on Mac OS X 10.11.6

2020-07-21 Thread Robert Ball

Public bug reported:

Hi all,

build of tag v5.1.0-rc1 fails on Mac OS X 10.11.6 (El Capitan) with the
following error:

git clone https://git.qemu.org/git/qemu.git

cd qemu
git submodule init

git submodule update --recursive

./configure

make


  CC  trace/control.o
In file included from trace/control.c:29:
In file included from /Users/rtb/src/qemu/include/monitor/monitor.h:4:
In file included from /Users/rtb/src/qemu/include/block/block.h:4:
In file included from /Users/rtb/src/qemu/include/block/aio.h:23:
/Users/rtb/src/qemu/include/qemu/timer.h:843:9: warning: implicit declaration 
of function 'clock_gettime' is invalid in C99
  [-Wimplicit-function-declaration]
clock_gettime(CLOCK_MONOTONIC, );
^
/Users/rtb/src/qemu/include/qemu/timer.h:843:23: error: use of undeclared 
identifier 'CLOCK_MONOTONIC'
clock_gettime(CLOCK_MONOTONIC, );
  ^
1 warning and 1 error generated.
make: *** [trace/control.o] Error 1


rtb:qemu rtb$ git log -n1
commit c8004fe6bbfc0d9c2e7b942c418a85efb3ac4b00 (HEAD -> master, tag: 
v5.1.0-rc1, origin/master, origin/HEAD)
Author: Peter Maydell 
Date:   Tue Jul 21 20:28:59 2020 +0100

Update version for v5.1.0-rc1 release

Signed-off-by: Peter Maydell 
rtb:qemu rtb$ 


Please find the full output of all the commands (from git clone of the repo, to 
the make) in the attached file "buildfail.txt".

Thank you!

Best regards,

Robert Ball

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: build-failed macosx-10.11.6 v5.1.0-rc1

** Attachment added: "full output of git clone through make"
   
https://bugs.launchpad.net/bugs/1888431/+attachment/5394703/+files/buildfail.txt

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1888431

Title:
  v5.1.0-rc1 build fails on Mac OS X 10.11.6

Status in QEMU:
  New

Bug description:
  Hi all,

  build of tag v5.1.0-rc1 fails on Mac OS X 10.11.6 (El Capitan) with
  the following error:

  git clone https://git.qemu.org/git/qemu.git
  
  cd qemu
  git submodule init
  
  git submodule update --recursive
  
  ./configure
  
  make
  

CC  trace/control.o
  In file included from trace/control.c:29:
  In file included from /Users/rtb/src/qemu/include/monitor/monitor.h:4:
  In file included from /Users/rtb/src/qemu/include/block/block.h:4:
  In file included from /Users/rtb/src/qemu/include/block/aio.h:23:
  /Users/rtb/src/qemu/include/qemu/timer.h:843:9: warning: implicit declaration 
of function 'clock_gettime' is invalid in C99
[-Wimplicit-function-declaration]
  clock_gettime(CLOCK_MONOTONIC, );
  ^
  /Users/rtb/src/qemu/include/qemu/timer.h:843:23: error: use of undeclared 
identifier 'CLOCK_MONOTONIC'
  clock_gettime(CLOCK_MONOTONIC, );
^
  1 warning and 1 error generated.
  make: *** [trace/control.o] Error 1

  
  rtb:qemu rtb$ git log -n1
  commit c8004fe6bbfc0d9c2e7b942c418a85efb3ac4b00 (HEAD -> master, tag: 
v5.1.0-rc1, origin/master, origin/HEAD)
  Author: Peter Maydell 
  Date:   Tue Jul 21 20:28:59 2020 +0100

  Update version for v5.1.0-rc1 release
  
  Signed-off-by: Peter Maydell 
  rtb:qemu rtb$ 

  
  Please find the full output of all the commands (from git clone of the repo, 
to the make) in the attached file "buildfail.txt".

  Thank you!

  Best regards,

  Robert Ball

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1888431/+subscriptions

Re: [PATCH] gdbstub: add support to Xfer:auxv:read: packet

2020-07-21 Thread Lirong Yuan

On Tue, Jul 21, 2020 at 12:50 PM Alex Bennée  wrote:
>
>
> Lirong Yuan  writes:
>
> > On Tue, Mar 31, 2020 at 12:20 AM Alex Bennée  wrote:
> >>
> >>
> >> Lirong Yuan  writes:
> >>
> >> > On Mon, Mar 30, 2020 at 12:47 PM Alex Bennée  
> >> > wrote:
> >> >
> >> >>
> >> >> Lirong Yuan  writes:
> >> >>
> >> >> > On Sat, Mar 21, 2020 at 6:56 AM Alex Bennée 
> >> >> wrote:
> >> >> >
> >> >> >>
> >> >> >> Lirong Yuan  writes:
> >> >> >>
> >> >> >> > On Fri, Mar 20, 2020 at 2:17 AM Alex Bennée 
> >> >> >> > 
> >> >> >> wrote:
> >> >> >> 
> >> >> >> >>
> >> >> >> >> Sorry I missed this on my radar. There was a minor re-factor of
> >> >> gdbstub
> >> >> >> >> that was just merged which will mean this patch needs a re-base to
> >> >> use
> >> >> >> >> g_string_* functions to expand stings.
> >> >> >> >>
> >> >> >> >> Also we have some simple gdbstub tests now - could we come up 
> >> >> >> >> with a
> >> >> >> >> multiarch gdbstub test to verify this is working properly?
> >> >> >> >>
> >> >> >> 
> >> >> >> > For sure, I will re-base this patch to use g_string_* functions.
> >> >> >> >
> >> >> >> > Currently we are using qemu aarch64. I am not sure how to do this 
> >> >> >> > yet,
> >> >> >> but
> >> >> >> > I could try to add something to
> >> >> >> > https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub
> >> >> >>
> >> >> >> If the auxv support is appropriate to all linux-user targets you can
> >> >> >> plumb it into the multiarch tests - you can even use the existing
> >> >> >> binaries.
> >> >> >>
> >> >> >> So you need:
> >> >> >>
> >> >> >>   - a stanza in the makefiles to launch the test (see
> >> >> >> tests/tcg/aarch64/Makefile.target)
> >> >> >>
> >> >> >>   - a .py test script that manipulates gdbstub to check things are
> >> >> working
> >> >> >>
> >> >> >> So something like:
> >> >> >>
> >> >> >> .PHONY: gdbstub-foo-binary
> >> >> >> run-gdbstub-foo-binary: foo-binary
> >> >> >> $(call run-test, $@, $(GDB_SCRIPT) \
> >> >> >> --gdb $(HAVE_GDB_BIN) \
> >> >> >> --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> >> >> >> --bin $< --test 
> >> >> >> $(MULTIARCH_SRC)/gdbstub/test-foo.py, \
> >> >> >> "basic gdbstub FOO support")
> >> >> >>
> >> >> >>
> >> >> >> >
> >> >> >> > Does this sound good?
> >> >> >>
> >> >> >> Hope that helps.
> >> >> >>
> >> >> >> >
> >> >> >> > Thanks!
> >> >> >> > Lirong
> >> >> >>
> >> >> >>
> >> >> >> --
> >> >> >> Alex Bennée
> >> >> >>
> >> >> >
> >> >> > Hi Alex,
> >> >> >
> >> >> > Thanks for the instructions, very helpful!
> >> >> >
> >> >> > I rebased this patch to use g_string_* functions, and the link to
> >> >> patchwork
> >> >> > is:
> >> >> > http://patchwork.ozlabs.org/patch/1264125/
> >> >> > Could you help take another look?
> >> >> >
> >> >> > Regarding testing, I looked at some instructions for running tests, 
> >> >> > e.g.
> >> >> > https://github.com/qemu/qemu/blob/master/docs/devel/testing.rst
> >> >> > https://wiki.qemu.org/Testing
> >> >> > However I still could not get the tests for aarch64 to run. Do you 
> >> >> > know
> >> >> how
> >> >> > to run the aarch64 or multi-arch tests?
> >> >>
> >> >> The aarch64 ones run with "make run-tcg-tests-aarch64-linux-user" add
> >> >> V=1 to see the details.
> >> >>
> >> >> > Also there aren't any existing gdb stub tests that try to read
> >> >> > uninterpreted bytes from the target’s special data area identified by 
> >> >> > a
> >> >> > keyword:
> >> >> >
> >> >> https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#qXfer-auxiliary-vector-read
> >> >> > I looked at some other gdb stub tests, but they did not seem to send 
> >> >> > any
> >> >> > queries:
> >> >> > https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub
> >> >> > So I am not sure how to set up one for "Xfer:auxv:read:" packets...
> >> >> > Are there plans to add more tests for other packets like
> >> >> > "Xfer:features:read:"?
> >> >> > I'd be happy to add a test if there is an example of how to do it. :)
> >> >>
> >> >> What would you do from a normal gdb command line. At the very least you
> >> >> run the same command with gdb.execute(), e.g.:
> >> >>
> >> >>   gdb.execute("set confirm off")
> >> >>
> >> >> is the same as typing
> >> >>
> >> >>   set confirm off
> >> >>
> >> >> at the gdb command prompt.
> >> >>
> >> >> >
> >> >> > Thanks,
> >> >> > Lirong
> >> >>
> >> >>
> >> >> --
> >> >> Alex Bennée
> >> >>
> >> >
> >> > Hey Alex,
> >> >
> >> > I tried to run the test but they were skipped. Do you know if there's any
> >> > other flag that needs to be set?
> >> >
> >> > $ make run-tcg-tests-aarch64-linux-user
> >> > make[1]: Entering directory '/usr/local/google/home/yuanzi/qemu/slirp'
> >> > make[1]: Nothing to be done for 'all'.
> >> > make[1]: Leaving directory '/usr/local/google/home/yuanzi/qemu/slirp'
> >> >   BUILD   TCG tests for aarch64-linux-user
> >> >   BUILD   aarch64-linux-user guest-tests SKIPPED
> >> >   RUN TCG tests

Re: [PATCH] linux-user: fix clock_nanosleep()



Laurent Vivier  writes:

> If clock_nanosleep() encounters an error, it returns one of the positive
> error number.
>
> If the call is interrupted by a signal handler, it fails with error EINTR
> and if "remain" is not NULL and "flags" is not TIMER_ABSTIME, it returns
> the remaining unslept time in "remain".
>
> Update linux-user to not overwrite the "remain" structure if there is no
> error.
>
> Found with "make check-tcg", linux-test fails on nanosleep test:

Queued to for-5.1/fixes-for-rc1-v2, thanks.

-- 
Alex Bennée

[PATCH] linux-user: fix clock_nanosleep()

2020-07-21 Thread Laurent Vivier

If clock_nanosleep() encounters an error, it returns one of the positive
error number.

If the call is interrupted by a signal handler, it fails with error EINTR
and if "remain" is not NULL and "flags" is not TIMER_ABSTIME, it returns
the remaining unslept time in "remain".

Update linux-user to not overwrite the "remain" structure if there is no
error.

Found with "make check-tcg", linux-test fails on nanosleep test:

  TESTlinux-test on x86_64
.../tests/tcg/multiarch/linux-test.c:242: nanosleep
make[2]: *** [../Makefile.target:153: run-linux-test] Error 1
make[1]: *** [.../tests/tcg/Makefile.qemu:76: run-guest-tests] Error 2
make: *** [.../tests/Makefile.include:857: run-tcg-tests-x86_64-linux-user] 
Error 2

Reported-by: Philippe Mathieu-Daudé 
Signed-off-by: Laurent Vivier 
---
 linux-user/syscall.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1211e759c26c..caa7cd3cab94 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11829,10 +11829,19 @@ static abi_long do_syscall1(void *cpu_env, int num, 
abi_long arg1,
 {
 struct timespec ts;
 target_to_host_timespec(, arg3);
-ret = get_errno(safe_clock_nanosleep(arg1, arg2,
- , arg4 ?  : NULL));
-if (arg4)
+/*
+ * clock_nanosleep() returns 0 or one of the *positive* error number.
+ */
+ret = host_to_target_errno(safe_clock_nanosleep(arg1, arg2, ,
+arg4 ?  : NULL));
+/*
+ * if the call is interrupted by a signal handler, it fails
+ * with error TARGET_EINTR and if arg4 is not NULL and arg2 is not
+ * TIMER_ABSTIME, it returns the remaining unslept time in arg4.
+ */
+if (ret == TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
 host_to_target_timespec(arg4, );
+}
 
 #if defined(TARGET_PPC)
 /* clock_nanosleep is odd in that it returns positive errno values.
-- 
2.26.2

[Bug 1805256] Re: qemu-img hangs on rcu_call_ready_event logic in Aarch64 when converting images

2020-07-21 Thread Rafael David Tinoco

Status from old attempts to solve same nature issues:



Older (2018) merge request from @raharper:

https://github.com/koverstreet/bcache-tools/pull/1

addressing the fact that kernel uevents would not always emit 
CACHED_UUID parameters, making udev to delete (whenever that happens) 
/dev/bcache/{by-uuid,by-label} symlinks.

This last MR pointed to previous related bugs:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890446
https://bugs.launchpad.net/curtin/+bug/1728742

And to an upstream kernel patch:

https://lore.kernel.org/patchwork/patch/921298/

to

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1729145

that wasn't accepted upstream.

Even not being accepted upstream, the SRU was attempted:

LP: #1729145

https://lists.ubuntu.com/archives/kernel-team/2017-December/088680.html
https://lists.ubuntu.com/archives/kernel-team/2017-December/088679.html

Both were NACKED.

Attempted again:

https://lists.ubuntu.com/archives/kernel-team/2017-December/088682.html
https://lists.ubuntu.com/archives/kernel-team/2017-December/088683.html

NACKED again.

And a v2 was sent:

https://lists.ubuntu.com/archives/kernel-team/2017-December/088751.html
https://lists.ubuntu.com/archives/kernel-team/2017-December/088750.html
https://lists.ubuntu.com/archives/kernel-team/2017-December/088749.html

and acked in January 2018 by Coling:

https://lists.ubuntu.com/archives/kernel-team/2018-January/089492.html

but not upstreamed.

BIONIC contains the fix:

commit ed9333e1b583
Author: Ryan Harper 
Date:   Mon Dec 11 12:12:01 2017

UBUNTU: SAUCE: (no-up) bcache: decouple emitting a cached_dev CHANGE uevent

BugLink: http://bugs.launchpad.net/bugs/1729145

- decouple emitting a cached_dev CHANGE uevent which includes dev.uuid
  and dev.label from bch_cached_dev_run() which only happens when a
  bcacheX device is bound to the actual backing block device (bcache0 -> 
vdb)

- update bch_cached_dev_run() to invoke bch_cached_dev_emit_change() as
  needed; no functional code path changes here

- Modify register_bcache to detect a re-registering of a bcache
  cached_dev, and in that case call bcache_cached_dev_emit_change() to

Signed-off-by: Ryan Harper 
Signed-off-by: Joseph Salisbury 
Acked-by: Colin Ian King 
Acked-by: Stefan Bader 
Signed-off-by: Khalid Elmously 
[ saf: fix incorrect indentation ]
Signed-off-by: Seth Forshee 

FOCAL contains the fix:

commit 67553dcd7905
Author: Ryan Harper 
Date:   Mon Dec 11 12:12:01 2017

UBUNTU: SAUCE: (no-up) bcache: decouple emitting a cached_dev CHANGE
uevent

GROOVY contains the fix:

commit 67553dcd7905
Author: Ryan Harper 
Date:   Mon Dec 11 12:12:01 2017

UBUNTU: SAUCE: (no-up) bcache: decouple emitting a cached_dev CHANGE
uevent



So, the kernel patch wasn't accepted, nor bcache-tools patch by 
@raharper, the bcache-export-cached.



New Upstream summary from @raharper:

https://github.com/systemd/systemd/pull/16317#issuecomment-655647313

in the upstream merge request made by @rbalint.


** Bug watch added: Debian Bug tracker #890446
   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890446

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1805256

Title:
  qemu-img hangs on rcu_call_ready_event logic in Aarch64 when
  converting images

Status in kunpeng920:
  Triaged
Status in kunpeng920 ubuntu-18.04 series:
  Triaged
Status in kunpeng920 ubuntu-18.04-hwe series:
  Triaged
Status in kunpeng920 ubuntu-19.10 series:
  Fix Released
Status in kunpeng920 ubuntu-20.04 series:
  Fix Released
Status in kunpeng920 upstream-kernel series:
  Invalid
Status in QEMU:
  Fix Released
Status in qemu package in Ubuntu:
  Fix Released
Status in qemu source package in Bionic:
  In Progress
Status in qemu source package in Eoan:
  Fix Released
Status in qemu source package in Focal:
  Fix Released

Bug description:
  [Impact]

  * QEMU locking primitives might face a race condition in QEMU Async
  I/O bottom halves scheduling. This leads to a dead lock making either
  QEMU or one of its tools to hang indefinitely.

  [Test Case]

  * qemu-img convert -f qcow2 -O qcow2 ./disk01.qcow2 ./output.qcow2

  Hangs indefinitely approximately 30% of the runs in Aarch64.

  [Regression Potential]

  * This is a change to a core part of QEMU: The AIO scheduling. It
  works like a "kernel" scheduler, whereas kernel schedules OS tasks,
  the QEMU AIO code is responsible to schedule QEMU coroutines or event
  listeners callbacks.

  * There was a long discussion upstream about primitives and Aarch64.
  After quite sometime Paolo released this patch and it solves the
  issue. Tested platforms were: amd64 and aarch64 based on his commit
  log.

  * Christian suggests that this fix stay little longer in -proposed to
  make sure it won't cause any regressions.

  * dannf suggests we also check for performance

[Bug 1805256] Re: qemu-img hangs on rcu_call_ready_event logic in Aarch64 when converting images

2020-07-21 Thread Rafael David Tinoco

I've hidden last post as it was posted in the wrong bug.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1805256

Title:
  qemu-img hangs on rcu_call_ready_event logic in Aarch64 when
  converting images

Status in kunpeng920:
  Triaged
Status in kunpeng920 ubuntu-18.04 series:
  Triaged
Status in kunpeng920 ubuntu-18.04-hwe series:
  Triaged
Status in kunpeng920 ubuntu-19.10 series:
  Fix Released
Status in kunpeng920 ubuntu-20.04 series:
  Fix Released
Status in kunpeng920 upstream-kernel series:
  Invalid
Status in QEMU:
  Fix Released
Status in qemu package in Ubuntu:
  Fix Released
Status in qemu source package in Bionic:
  In Progress
Status in qemu source package in Eoan:
  Fix Released
Status in qemu source package in Focal:
  Fix Released

Bug description:
  [Impact]

  * QEMU locking primitives might face a race condition in QEMU Async
  I/O bottom halves scheduling. This leads to a dead lock making either
  QEMU or one of its tools to hang indefinitely.

  [Test Case]

  * qemu-img convert -f qcow2 -O qcow2 ./disk01.qcow2 ./output.qcow2

  Hangs indefinitely approximately 30% of the runs in Aarch64.

  [Regression Potential]

  * This is a change to a core part of QEMU: The AIO scheduling. It
  works like a "kernel" scheduler, whereas kernel schedules OS tasks,
  the QEMU AIO code is responsible to schedule QEMU coroutines or event
  listeners callbacks.

  * There was a long discussion upstream about primitives and Aarch64.
  After quite sometime Paolo released this patch and it solves the
  issue. Tested platforms were: amd64 and aarch64 based on his commit
  log.

  * Christian suggests that this fix stay little longer in -proposed to
  make sure it won't cause any regressions.

  * dannf suggests we also check for performance regressions; e.g. how
  long it takes to convert a cloud image on high-core systems.

  [Other Info]

   * Original Description bellow:

  Command:

  qemu-img convert -f qcow2 -O qcow2 ./disk01.qcow2 ./output.qcow2

  Hangs indefinitely approximately 30% of the runs.

  

  Workaround:

  qemu-img convert -m 1 -f qcow2 -O qcow2 ./disk01.qcow2 ./output.qcow2

  Run "qemu-img convert" with "a single coroutine" to avoid this issue.

  

  (gdb) thread 1
  ...
  (gdb) bt
  #0 0xbf1ad81c in __GI_ppoll
  #1 0xaabcf73c in ppoll
  #2 qemu_poll_ns
  #3 0xaabd0764 in os_host_main_loop_wait
  #4 main_loop_wait
  ...

  (gdb) thread 2
  ...
  (gdb) bt
  #0 syscall ()
  #1 0xaabd41cc in qemu_futex_wait
  #2 qemu_event_wait (ev=ev@entry=0xaac86ce8 )
  #3 0xaabed05c in call_rcu_thread
  #4 0xaabd34c8 in qemu_thread_start
  #5 0xbf25c880 in start_thread
  #6 0xbf1b6b9c in thread_start ()

  (gdb) thread 3
  ...
  (gdb) bt
  #0 0xbf11aa20 in __GI___sigtimedwait
  #1 0xbf2671b4 in __sigwait
  #2 0xaabd1ddc in sigwait_compat
  #3 0xaabd34c8 in qemu_thread_start
  #4 0xbf25c880 in start_thread
  #5 0xbf1b6b9c in thread_start

  

  (gdb) run
  Starting program: /usr/bin/qemu-img convert -f qcow2 -O qcow2
  ./disk01.ext4.qcow2 ./output.qcow2

  [New Thread 0xbec5ad90 (LWP 72839)]
  [New Thread 0xbe459d90 (LWP 72840)]
  [New Thread 0xbdb57d90 (LWP 72841)]
  [New Thread 0xacac9d90 (LWP 72859)]
  [New Thread 0xa7ffed90 (LWP 72860)]
  [New Thread 0xa77fdd90 (LWP 72861)]
  [New Thread 0xa6ffcd90 (LWP 72862)]
  [New Thread 0xa67fbd90 (LWP 72863)]
  [New Thread 0xa5ffad90 (LWP 72864)]

  [Thread 0xa5ffad90 (LWP 72864) exited]
  [Thread 0xa6ffcd90 (LWP 72862) exited]
  [Thread 0xa77fdd90 (LWP 72861) exited]
  [Thread 0xbdb57d90 (LWP 72841) exited]
  [Thread 0xa67fbd90 (LWP 72863) exited]
  [Thread 0xacac9d90 (LWP 72859) exited]
  [Thread 0xa7ffed90 (LWP 72860) exited]

  
  """

  All the tasks left are blocked in a system call, so no task left to call
  qemu_futex_wake() to unblock thread #2 (in futex()), which would unblock
  thread #1 (doing poll() in a pipe with thread #2).

  Those 7 threads exit before disk conversion is complete (sometimes in
  the beginning, sometimes at the end).

  

  On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img
  frequently hangs (~50% of the time) with this command:

  qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2

  Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This
  qcow2->qcow2 conversion happens to be something uvtool does every time
  it fetches images.

  Once hung, attaching gdb gives the following backtrace:

  (gdb) bt
  #0  0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, 
nfds=187650274213760,
  timeout=, timeout@entry=0x0, sigmask=0xc123b950)
  at ../sysdeps/unix/sysv/linux/ppoll.c:39
  #1  0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=,
  __fds=) at

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-07-21 Thread Guenter Roeck

On 7/21/20 10:36 AM, Cédric Le Goater wrote:
> Hello,
> 
> On 2/6/20 7:32 PM, Guenter Roeck wrote:
>> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
>> always requests 6 bytes. The current implementation only returns three
>> bytes, and interprets the remaining three bytes as new commands.
>> While this does not matter most of the time, it is at the very least
>> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
>> data. Fill remaining data with 0.
>>
>> Signed-off-by: Guenter Roeck 
>> ---
>> v2: Split patch into two parts; improved decription
>>
>>  hw/block/m25p80.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 5ff8d270c4..53bf63856f 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  for (i = 0; i < s->pi->id_len; i++) {
>>  s->data[i] = s->pi->id[i];
>>  }
>> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
>> +s->data[i] = 0;
>> +}
> 
> This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
> board : 
> 
>   https://www.supermicro.com/en/products/motherboard/X11SSL-F 
> 
> which expects the flash ID to repeat. Erik solved the problem with the patch 
> below and I think it makes sense to wrap around. Anyone knows what should be 
> the expected behavior ? 
> 

No idea what the expected behavior is, but I am fine with the code below
if it works.

Thanks,
Guenter

> Thanks,
> 
> C. 
> 
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8227088441..5000930800 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  s->data[i] = s->pi->id[i];
>  }
>  for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> -s->data[i] = 0;
> +s->data[i] = s->pi->id[i % s->pi->id_len];
>  }
> 
>  s->len = SPI_NOR_MAX_ID_LEN;
>

Re: [PATCH] gdbstub: add support to Xfer:auxv:read: packet



Lirong Yuan  writes:

> On Tue, Mar 31, 2020 at 12:20 AM Alex Bennée  wrote:
>>
>>
>> Lirong Yuan  writes:
>>
>> > On Mon, Mar 30, 2020 at 12:47 PM Alex Bennée  
>> > wrote:
>> >
>> >>
>> >> Lirong Yuan  writes:
>> >>
>> >> > On Sat, Mar 21, 2020 at 6:56 AM Alex Bennée 
>> >> wrote:
>> >> >
>> >> >>
>> >> >> Lirong Yuan  writes:
>> >> >>
>> >> >> > On Fri, Mar 20, 2020 at 2:17 AM Alex Bennée 
>> >> >> wrote:
>> >> >> 
>> >> >> >>
>> >> >> >> Sorry I missed this on my radar. There was a minor re-factor of
>> >> gdbstub
>> >> >> >> that was just merged which will mean this patch needs a re-base to
>> >> use
>> >> >> >> g_string_* functions to expand stings.
>> >> >> >>
>> >> >> >> Also we have some simple gdbstub tests now - could we come up with a
>> >> >> >> multiarch gdbstub test to verify this is working properly?
>> >> >> >>
>> >> >> 
>> >> >> > For sure, I will re-base this patch to use g_string_* functions.
>> >> >> >
>> >> >> > Currently we are using qemu aarch64. I am not sure how to do this 
>> >> >> > yet,
>> >> >> but
>> >> >> > I could try to add something to
>> >> >> > https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub
>> >> >>
>> >> >> If the auxv support is appropriate to all linux-user targets you can
>> >> >> plumb it into the multiarch tests - you can even use the existing
>> >> >> binaries.
>> >> >>
>> >> >> So you need:
>> >> >>
>> >> >>   - a stanza in the makefiles to launch the test (see
>> >> >> tests/tcg/aarch64/Makefile.target)
>> >> >>
>> >> >>   - a .py test script that manipulates gdbstub to check things are
>> >> working
>> >> >>
>> >> >> So something like:
>> >> >>
>> >> >> .PHONY: gdbstub-foo-binary
>> >> >> run-gdbstub-foo-binary: foo-binary
>> >> >> $(call run-test, $@, $(GDB_SCRIPT) \
>> >> >> --gdb $(HAVE_GDB_BIN) \
>> >> >> --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>> >> >> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-foo.py, \
>> >> >> "basic gdbstub FOO support")
>> >> >>
>> >> >>
>> >> >> >
>> >> >> > Does this sound good?
>> >> >>
>> >> >> Hope that helps.
>> >> >>
>> >> >> >
>> >> >> > Thanks!
>> >> >> > Lirong
>> >> >>
>> >> >>
>> >> >> --
>> >> >> Alex Bennée
>> >> >>
>> >> >
>> >> > Hi Alex,
>> >> >
>> >> > Thanks for the instructions, very helpful!
>> >> >
>> >> > I rebased this patch to use g_string_* functions, and the link to
>> >> patchwork
>> >> > is:
>> >> > http://patchwork.ozlabs.org/patch/1264125/
>> >> > Could you help take another look?
>> >> >
>> >> > Regarding testing, I looked at some instructions for running tests, e.g.
>> >> > https://github.com/qemu/qemu/blob/master/docs/devel/testing.rst
>> >> > https://wiki.qemu.org/Testing
>> >> > However I still could not get the tests for aarch64 to run. Do you know
>> >> how
>> >> > to run the aarch64 or multi-arch tests?
>> >>
>> >> The aarch64 ones run with "make run-tcg-tests-aarch64-linux-user" add
>> >> V=1 to see the details.
>> >>
>> >> > Also there aren't any existing gdb stub tests that try to read
>> >> > uninterpreted bytes from the target’s special data area identified by a
>> >> > keyword:
>> >> >
>> >> https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#qXfer-auxiliary-vector-read
>> >> > I looked at some other gdb stub tests, but they did not seem to send any
>> >> > queries:
>> >> > https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub
>> >> > So I am not sure how to set up one for "Xfer:auxv:read:" packets...
>> >> > Are there plans to add more tests for other packets like
>> >> > "Xfer:features:read:"?
>> >> > I'd be happy to add a test if there is an example of how to do it. :)
>> >>
>> >> What would you do from a normal gdb command line. At the very least you
>> >> run the same command with gdb.execute(), e.g.:
>> >>
>> >>   gdb.execute("set confirm off")
>> >>
>> >> is the same as typing
>> >>
>> >>   set confirm off
>> >>
>> >> at the gdb command prompt.
>> >>
>> >> >
>> >> > Thanks,
>> >> > Lirong
>> >>
>> >>
>> >> --
>> >> Alex Bennée
>> >>
>> >
>> > Hey Alex,
>> >
>> > I tried to run the test but they were skipped. Do you know if there's any
>> > other flag that needs to be set?
>> >
>> > $ make run-tcg-tests-aarch64-linux-user
>> > make[1]: Entering directory '/usr/local/google/home/yuanzi/qemu/slirp'
>> > make[1]: Nothing to be done for 'all'.
>> > make[1]: Leaving directory '/usr/local/google/home/yuanzi/qemu/slirp'
>> >   BUILD   TCG tests for aarch64-linux-user
>> >   BUILD   aarch64-linux-user guest-tests SKIPPED
>> >   RUN TCG tests for aarch64-linux-user
>> >   RUN tests for aarch64-linux-user SKIPPED
>>
>> Ahh you either need to have docker enabled or the aarch64 compilers in
>> your path when you run configure so the test programs can be built. The
>> details are in docs/devel/testing.rst
>>
>> > I don't think any command needs to be run. It should just send the query
>> > automatically.
>> > Could we assume that it will work the same in the

Re: [PULL 0/1] Monitor patches for 2020-07-21

On Tue, 21 Jul 2020 at 16:25, Markus Armbruster  wrote:
>
> The following changes since commit af3d69058e09bede9900f266a618ed11f76f49f3:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20200720' into staging (2020-07-20 
> 15:58:07 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-monitor-2020-07-21
>
> for you to fetch changes up to 029afc4e76041e1a320530d97f99122a1b3d5da2:
>
>   qdev: Fix device_add DRIVER,help to print to monitor (2020-07-21 17:22:44 
> +0200)
>
> 
> Monitor patches for 2020-07-21
>
> 
> Markus Armbruster (1):
>   qdev: Fix device_add DRIVER,help to print to monitor
>
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This missed rc1, I'm afraid; will go in for rc2.

thanks
-- PMM

Re: [PULL 0/3] Block layer patches for 5.1.0-rc1

On Tue, 21 Jul 2020 at 16:46, Kevin Wolf  wrote:
>
> The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:
>
>   Merge remote-tracking branch 
> 'remotes/huth-gitlab/tags/pull-request-2020-07-21' into staging (2020-07-21 
> 10:24:38 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 4a01e27ddcb5921efd68820d05d85ba71293fea6:
>
>   iotests: Test sparseness for qemu-img convert -n (2020-07-21 17:44:35 +0200)
>
> 
> Block layer patches:
>
> - file-posix: Handle `EINVAL` fallocate return value
> - qemu-img convert -n: Keep qcow2 v2 target sparse
>
> 
> Antoine Damhet (1):
>   file-posix: Handle `EINVAL` fallocate return value
>
> Kevin Wolf (2):
>   qcow2: Implement v2 zero writes with discard if possible
>   iotests: Test sparseness for qemu-img convert -n


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

Re: Inter-VM device emulation (call on Mon 20th July 2020)

2020-07-21 Thread Jan Kiszka


On 21.07.20 12:49, Alex Bennée wrote:


Stefan Hajnoczi  writes:


Thank you everyone who joined!

I didn't take notes but two things stood out:

1. The ivshmem v2 and virtio-vhost-user use cases are quite different
so combining them does not seem realistic. ivshmem v2 needs to be as
simple for the hypervisor to implement as possible even if this
involves some sacrifices (e.g. not transparent to the Driver VM that
is accessing the device, performance). virtio-vhost-user is more aimed
at general-purpose device emulation although support for arbitrary
devices (e.g. PCI) would be important to serve all use cases.


I believe my phone gave up on the last few minutes of the call so I'll
just say we are interested in being able to implement arbitrary devices
in the inter-VM silos. Devices we are looking at:

   virtio-audio
   virtio-video

these are performance sensitive devices which provide a HAL abstraction
to a common software core.

   virtio-rpmb

this is a secure device where the backend may need to reside in a secure
virtualised world.

   virtio-scmi

this is a more complex device which allows the guest to make power and
clock demands from the firmware. Needless to say this starts to become
complex with multiple moving parts.

The flexibility of vhost-user seems to match up quite well with wanting
to have a reasonably portable backend that just needs to be fed signals
and a memory mapping. However we don't want daemons to automatically
have a full view of the whole of the guests system memory.


2. Alexander Graf's idea for a new Linux driver that provides an
enforcing software IOMMU. This would be a character device driver that
is mmapped by the device emulation process (either vhost-user-style on
the host or another VMM for inter-VM device emulation). The Driver VMM
can program mappings into the device and the page tables in the device
emulation process will be updated. This way the Driver VMM can share
memory specific regions of guest RAM with the device emulation process
and revoke those mappings later.


I'm wondering if there is enough plumbing on the guest side so a guest
can use the virtio-iommu to mark out exactly which bits of memory the
virtual device can have access to? At a minimum the virtqueues need to
be accessible and for larger transfers maybe a bounce buffer. However
for speed you want as wide as possible mapping but no more. It would be
nice for example if a block device could load data directly into the
guests block cache (zero-copy) but without getting a view of the kernels
internal data structures.


Welcome to a classic optimization triangle:

 - speed -> direct mappings
 - security -> restricted mapping
 - simplicity -> static mapping

Pick two, you can't have them all. Well, you could try a little bit more 
of one, at the price of losing on another. But that's it.


We chose the last two, ending up with probably the simplest but not 
fastest solution for type-1 hypervisors like Jailhouse. Specifically for 
non-Linux use cases, legacy RTOSes, often with limited driver stacks, 
having not only virtio but also even simpler channels over 
application-defined shared memory layouts is a requirement.




Another thing that came across in the call was quite a lot of
assumptions about QEMU and Linux w.r.t virtio. While our project will
likely have Linux as a guest OS we are looking specifically at enabling
virtio for Type-1 hypervisors like Xen and the various safety certified
proprietary ones. It is unlikely that QEMU would be used as the VMM for
these deployments. We want to work out what sort of common facilities
hypervisors need to support to enable virtio so the daemons can be
re-usable and maybe setup with a minimal shim for the particular
hypervisor in question.



I'm with you regarding stacks that are mappable not only on QEMU/Linux. 
And also one that does not let the certification costs sky-rocket 
because of its mandated implementation complexity.


I'm not sure anymore if there will be only one device model. Maybe we 
should eventually think about a backend layer that can sit on something 
like virtio-vhost-user as well as on ivshmem-virtio, allowing the same 
device backend code to be plumbed into both transports. Why shouldn't 
work what already works well under Linux with the frontend device 
drivers vs. virtio transports?


Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

2020-07-21 Thread Collin Walling

On 7/21/20 4:41 AM, David Hildenbrand wrote:
> [...]
> 
 +switch (code & SCLP_CMD_CODE_MASK) {
 +default:
 +if (sccb_max_addr < sccb_boundary) {
 +return true;
 +}
 +}
>>>
>>> ^ what is that?
>>>
>>> if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>>> return true;
>>> }
> 
> Oh, my tired eyes missed that it's actually only
> 
> if (sccb_max_addr < sccb_boundary) :)
> 
>>>
>>
>> I agree it looks pointless in this patch, but it makes more sense in
>> patch #6 where we introduce cases for the SCLP commands that bypass
>> these checks if the extended-length sccb feature is enabled.
> 
> I am really a friend of introducing stuff where needed. Just use a
> simple "if" here and convert to the switch in patch #6.
> 

I can understand that. This follows the whole "each patch should be
sufficient on its own" way of thinking. A switch with no cases and a
default _does_ look silly.

>>
 +header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
 +return false;
>>>
>>> So we return "false" on success? At least I consider that weird when
>>> returning the bool type. Maybe make it clearer what the function indicates
>>>
>>
>> Hmmm... I figured since there were more paths that can lead to success
>> (i.e. when I introduce the feat check in a later patch), then it made
>> more sense to to return false at the end. sclp_command_code_valid has
>> similar logic.
>>
>> But if boolean functions traditionally return true as the last return
>> value, I can rework it to align to coding preferences / standards.
>>
>>> "sccb_boundary_is_invalid"
>>>
>>
>> Unless it's simply the name that is confusing?
> 
> The options I would support are
> 
> 1. "sccb_boundary_is_valid" which returns "true" if valid
> 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> 
> Which makes reading this code a bit easier.
> 

Sounds good. I'll takes this into consideration for the next round. (I
may wait just a little longer for that to allow more reviews to come in
from whoever has the time, if that's okay.)

>>
>>> or leave it named as is and switch from return value "bool" to "int",
>>> using "0" on success and "-EINVAL" on error.
>>>
>>
>> Is the switch statement an overkill? I thought of it as a cleaner way to
>> later show which commands have a special conditions (introduced in patch
>> 6 for the ELS stuff) instead of a nasty long if statement.
> 
> I think the switch make sense in patch #6.
> 


-- 
Regards,
Collin

Stay safe and stay healthy

QEMU | Pipeline #169168442 has failed for master | b50dab9e

2020-07-21 Thread GitLab via



Your pipeline has failed.

Project: QEMU ( https://gitlab.com/qemu-project/qemu )
Branch: master ( https://gitlab.com/qemu-project/qemu/-/commits/master )

Commit: b50dab9e ( 
https://gitlab.com/qemu-project/qemu/-/commit/b50dab9ecac442acc1b316e4749dae36a2fe7d61
 )
Commit Message: Merge remote-tracking branch 'remotes/armbru/ta...
Commit Author: Peter Maydell ( https://gitlab.com/pm215 )

Pipeline #169168442 ( 
https://gitlab.com/qemu-project/qemu/-/pipelines/169168442 ) triggered by Alex 
Bennée ( https://gitlab.com/stsquad )
had 1 failed build.

Job #649378246 ( https://gitlab.com/qemu-project/qemu/-/jobs/649378246/raw )

Stage: containers
Name: amd64-debian9-container
Trace: #7 DONE 0.0s

#4 [1/3] FROM docker.io/library/debian:stretch-slim@sha256:583e0a6588d4eb8c...
#4 DONE 0.0s

#5 [2/3] RUN cat /etc/apt/sources.list | sed "s/^deb\ /deb-src /" >> /etc/a...
#5 CACHED

#6 [3/3] RUN apt update && DEBIAN_FRONTEND=noninteractive apt install -...
#6 CACHED

#8 exporting to image
#8 exporting layers done
#8 writing image 
sha256:cf70ed90c67dd6435b3c24394ca444257391cbdda7514708f7d89a649b3a2f2a done
#8 naming to docker.io/qemu/debian9 done
#8 DONE 0.0s

#9 exporting cache
#9 preparing build cache for export done
#9 DONE 0.0s
[32;1m$ docker tag "qemu/$NAME" "$TAG"[0;m
[32;1m$ docker push "$TAG"[0;m
The push refers to repository 
[registry.gitlab.com/qemu-project/qemu/qemu/debian9]
Get https://registry.gitlab.com/v2/: EOF
section_end:1595356601:step_script
[0Ksection_start:1595356601:after_script
[0K[0K[36;1mRunning after_script[0;m
[0;m[32;1mRunning after script...[0;m
[32;1m$ docker logout[0;m
Removing login credentials for https://index.docker.io/v1/
section_end:1595356602:after_script
[0K[31;1mERROR: Job failed: exit code 1
[0;m


-- 
You're receiving this email because of your account on gitlab.com.

Re: [PULL 0/3] QOM patches for 2020-07-21

On Tue, 21 Jul 2020 at 16:41, Markus Armbruster  wrote:
>
> The following changes since commit af3d69058e09bede9900f266a618ed11f76f49f3:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20200720' into staging (2020-07-20 
> 15:58:07 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qom-2020-07-21
>
> for you to fetch changes up to 0dde9fd12fd39762ff68fca80d2f0a735d66e7bd:
>
>   qom: Make info qom-tree sort children more efficiently (2020-07-21 17:39:37 
> +0200)
>
> 
> QOM patches for 2020-07-21
>
> 
> Markus Armbruster (3):
>   qom: Change object_get_canonical_path_component() not to malloc
>   qom: Document object_get_canonical_path() returns malloced string
>   qom: Make info qom-tree sort children more efficiently


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

[PULL 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware()

The value returned by qemu_find_file() must be freed.

This fixes Coverity issue CID 1430449, which points out
that the memory returned by qemu_find_file() is leaked.

Fixes: Coverity CID 1430449 (RESOURCE_LEAK)
Fixes: 7dd8f6fde4 ('hw/avr: Add support for loading ELF/raw binaries')
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael Rolnik 
Tested-by: Michael Rolnik 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20200714164257.23330-5-f4...@amsat.org>
---
 hw/avr/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/avr/boot.c b/hw/avr/boot.c
index 6fbcde4061..d16bb3dbe1 100644
--- a/hw/avr/boot.c
+++ b/hw/avr/boot.c
@@ -60,7 +60,7 @@ static const char *avr_elf_e_flags_to_cpu_type(uint32_t flags)
 bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
MemoryRegion *program_mr, const char *firmware)
 {
-const char *filename;
+g_autofree char *filename = NULL;
 int bytes_loaded;
 uint64_t entry;
 uint32_t e_flags;
-- 
2.21.3

[PULL 2/4] qemu/osdep: Reword qemu_get_exec_dir() documentation

This comment is confuse, reword it a bit.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael Rolnik 
Tested-by: Michael Rolnik 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20200714164257.23330-3-f4...@amsat.org>
---
 include/qemu/osdep.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4841b5c6b5..45c217aa28 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -588,7 +588,10 @@ char *qemu_get_local_state_pathname(const char 
*relative_pathname);
 void qemu_init_exec_dir(const char *argv0);
 
 /* Get the saved exec dir.
- * Caller needs to release the returned string by g_free() */
+ *
+ * The caller is responsible for releasing the value returned with g_free()
+ * after use.
+ */
 char *qemu_get_exec_dir(void);
 
 /**
-- 
2.21.3

[PULL 1/4] qemu/osdep: Document os_find_datadir() return value

Document os_find_datadir() returned data must be freed.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael Rolnik 
Tested-by: Michael Rolnik 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20200714164257.23330-2-f4...@amsat.org>
---
 os-posix.c | 3 +++
 os-win32.c | 7 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/os-posix.c b/os-posix.c
index b674b20b1b..3572db3f44 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -84,6 +84,9 @@ void os_setup_signal_handling(void)
  * Find a likely location for support files using the location of the binary.
  * When running from the build tree this will be "$bindir/../pc-bios".
  * Otherwise, this is CONFIG_QEMU_DATADIR.
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
  */
 char *os_find_datadir(void)
 {
diff --git a/os-win32.c b/os-win32.c
index 6b86e022f0..c9c3afe648 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -57,7 +57,12 @@ void os_setup_early_signal_handling(void)
 atexit(os_undo_timer_resolution);
 }
 
-/* Look for support files in the same directory as the executable.  */
+/*
+ * Look for support files in the same directory as the executable.
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
+ */
 char *os_find_datadir(void)
 {
 return qemu_get_exec_dir();
-- 
2.21.3

[PULL 0/4] AVR patches for 2020-07-21

The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:

  Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07=
-21' into staging (2020-07-21 10:24:38 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/avr-20200721

for you to fetch changes up to 5e29521a82e540552880c3572cb8274bcaa1002c:

  hw/avr/boot: Fix memory leak in avr_load_firmware() (2020-07-21 16:13:04 +0=
200)


AVR patches

Fixes a memory leak reported by Coverity (CID 1430449).

CI jobs result:
. https://gitlab.com/philmd/qemu/-/pipelines/168722631



Philippe Mathieu-Daud=C3=A9 (4):
  qemu/osdep: Document os_find_datadir() return value
  qemu/osdep: Reword qemu_get_exec_dir() documentation
  qemu-common: Document qemu_find_file()
  hw/avr/boot: Fix memory leak in avr_load_firmware()

 include/qemu-common.h | 17 +
 include/qemu/osdep.h  |  5 -
 hw/avr/boot.c |  2 +-
 os-posix.c|  3 +++
 os-win32.c|  7 ++-
 5 files changed, 31 insertions(+), 3 deletions(-)

--=20
2.21.3

[PULL 3/4] qemu-common: Document qemu_find_file()

Document qemu_find_file(), in particular the returned
value which must be freed.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Maydell 
Reviewed-by: Li Qiang 
Reviewed-by: Michael Rolnik 
Tested-by: Michael Rolnik 
Reviewed-by: Daniel P. Berrangé 
Message-Id: <20200714164257.23330-4-f4...@amsat.org>
---
 include/qemu-common.h | 17 +
 1 file changed, 17 insertions(+)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index d0142f29ac..bb9496bd80 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -110,6 +110,23 @@ const char *qemu_get_vm_name(void);
 
 #define QEMU_FILE_TYPE_BIOS   0
 #define QEMU_FILE_TYPE_KEYMAP 1
+/**
+ * qemu_find_file:
+ * @type: QEMU_FILE_TYPE_BIOS (for BIOS, VGA BIOS)
+ *or QEMU_FILE_TYPE_KEYMAP (for keymaps).
+ * @name: Relative or absolute file name
+ *
+ * If @name exists on disk as an absolute path, or a path relative
+ * to the current directory, then returns @name unchanged.
+ * Otherwise searches for @name file in the data directories, either
+ * configured at build time (DATADIR) or registered with the -L command
+ * line option.
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
+ *
+ * Returns: a path that can access @name, or NULL if no matching file exists.
+ */
 char *qemu_find_file(int type, const char *name);
 
 /* OS specific functions */
-- 
2.21.3

Re: [PATCH-for-5.1] hw/ide/ahci: Do not dma_memory_unmap(NULL)


On 7/18/20 3:28 AM, Philippe Mathieu-Daudé wrote:

libFuzzer triggered the following assertion:

   cat << EOF | qemu-system-i386 -M pc-q35-5.0 \
 -nographic -monitor none -serial none -qtest stdio
   outl 0xcf8 0x8000fa24
   outl 0xcfc 0xe1068000
   outl 0xcf8 0x8000fa04
   outw 0xcfc 0x7
   outl 0xcf8 0x8000fb20
   write 0xe1068304 0x1 0x21
   write 0xe1068318 0x1 0x21
   write 0xe1068384 0x1 0x21
   write 0xe1068398 0x2 0x21
   EOF
   qemu-system-i386: exec.c:3621: address_space_unmap: Assertion `mr != NULL' 
failed.
   Aborted (core dumped)

This is because we don't check the return value from dma_memory_map()
which can return NULL, then we call dma_memory_unmap(NULL) which is
illegal. Fix by only unmap if the value is not NULL (and the size is
not the expected one).

Cc: qemu-sta...@nongnu.org
Reported-by: Alexander Bulekov 
Fixes: f6ad2e32f8 ("ahci: add ahci emulation")
BugLink: https://bugs.launchpad.net/qemu/+bug/1884693
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/ahci.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 009120f88b..4f596cb9ce 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -250,7 +250,7 @@ static void map_page(AddressSpace *as, uint8_t **ptr, 
uint64_t addr,
  }
  
  *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);

-if (len < wanted) {
+if (len < wanted && *ptr) {
  dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
  *ptr = NULL;
  }



Staged @ gitlab

https://gitlab.com/jsnow/qemu/-/commits/ide

[PULL 2/2] hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value

Commits b6d7e9b66f..a43770df5d simplified the error propagation.
Similarly to commit 6fd5bef10b "qom: Make functions taking Error**
return bool, not void", let fw_cfg_add_from_generator() return a
boolean value, not void.
This allow to simplify parse_fw_cfg() and fixes the error handling
issue reported by Coverity (CID 1430396):

  In parse_fw_cfg():

Variable assigned once to a constant guards dead code.

Local variable local_err is assigned only once, to a constant
value, making it effectively constant throughout its scope.
If this is not the intent, examine the logic to see if there
is a missing assignment that would make local_err not remain
constant.

It's the call of fw_cfg_add_from_generator():

Error *local_err = NULL;

fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
if (local_err) {
error_propagate(errp, local_err);
return -1;
}
return 0;

If it fails, parse_fw_cfg() sets an error and returns 0, which is
wrong. Harmless, because the only caller passes _fatal.

Reported-by: Peter Maydell 
Fixes: Coverity CID 1430396: 'Constant' variable guards dead code (DEADCODE)
Fixes: 6552d87c48 ("softmmu/vl: Let -fw_cfg option take a 'gen_id' argument")
Reviewed-by: Laszlo Ersek 
Reviewed-by: Markus Armbruster 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200721131911.27380-3-phi...@redhat.com>
---
 include/hw/nvram/fw_cfg.h |  4 +++-
 hw/nvram/fw_cfg.c | 10 ++
 softmmu/vl.c  |  6 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index bbcf405649..f190c428e8 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -304,8 +304,10 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
*filename, void *data,
  * will be used; also, a new entry will be added to the file directory
  * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
  * data size, and assigned selector key value.
+ *
+ * Returns: %true on success, %false on error.
  */
-void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
const char *gen_id, Error **errp);
 
 FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index dfa1f2012a..f3a4728288 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1032,7 +1032,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
*filename,
 return NULL;
 }
 
-void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
const char *gen_id, Error **errp)
 {
 FWCfgDataGeneratorClass *klass;
@@ -1043,20 +1043,22 @@ void fw_cfg_add_from_generator(FWCfgState *s, const 
char *filename,
 obj = object_resolve_path_component(object_get_objects_root(), gen_id);
 if (!obj) {
 error_setg(errp, "Cannot find object ID '%s'", gen_id);
-return;
+return false;
 }
 if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
 error_setg(errp, "Object ID '%s' is not a '%s' subclass",
gen_id, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE);
-return;
+return false;
 }
 klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
 array = klass->get_data(obj, errp);
 if (!array) {
-return;
+return false;
 }
 size = array->len;
 fw_cfg_add_file(s, filename, g_byte_array_free(array, TRUE), size);
+
+return true;
 }
 
 static void fw_cfg_machine_reset(void *opaque)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f476ef89ed..3416241557 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2070,11 +2070,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
Error **errp)
 size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
 buf = g_memdup(str, size);
 } else if (nonempty_str(gen_id)) {
-Error *local_err = NULL;
-
-fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp)) {
 return -1;
 }
 return 0;
-- 
2.21.3

[PULL 1/2] hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error propagation

Document FWCfgDataGeneratorClass::get_data() return NULL
on error, and non-NULL on success. This allow us to simplify
fw_cfg_add_from_generator(). Since we don't need a local
variable to propagate the error, we can remove the ERRP_GUARD()
macro.

Suggested-by: Markus Armbruster 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Markus Armbruster 
Message-Id: <20200721131911.27380-2-phi...@redhat.com>
---
 include/hw/nvram/fw_cfg.h | 4 +++-
 hw/nvram/fw_cfg.c | 3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 11feae3177..bbcf405649 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -32,7 +32,9 @@ typedef struct FWCfgDataGeneratorClass {
  * @obj: the object implementing this interface
  * @errp: pointer to a NULL-initialized error object
  *
- * Returns: reference to a byte array containing the data.
+ * Returns: reference to a byte array containing the data on success,
+ *  or NULL on error.
+ *
  * The caller should release the reference when no longer
  * required.
  */
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3b1811d3bf..dfa1f2012a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1035,7 +1035,6 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
*filename,
 void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
const char *gen_id, Error **errp)
 {
-ERRP_GUARD();
 FWCfgDataGeneratorClass *klass;
 GByteArray *array;
 Object *obj;
@@ -1053,7 +1052,7 @@ void fw_cfg_add_from_generator(FWCfgState *s, const char 
*filename,
 }
 klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
 array = klass->get_data(obj, errp);
-if (*errp) {
+if (!array) {
 return;
 }
 size = array->len;
-- 
2.21.3

[PULL 0/2] fw_cfg patches for 2020-07-21

The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:

  Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07=
-21' into staging (2020-07-21 10:24:38 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/fw_cfg-20200721

for you to fetch changes up to 077195187b47d83418e5fb521c89d7881fab3049:

  hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value (2020=
-07-21 16:47:54 +0200)


fw_cfg patches

Fixes the DEADCODE issue reported by Coverity (CID 1430396).

CI jobs result:
. https://gitlab.com/philmd/qemu/-/pipelines/169086301



Philippe Mathieu-Daud=C3=A9 (2):
  hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error
propagation
  hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value

 include/hw/nvram/fw_cfg.h |  8 ++--
 hw/nvram/fw_cfg.c | 13 +++--
 softmmu/vl.c  |  6 +-
 3 files changed, 14 insertions(+), 13 deletions(-)

--=20
2.21.3

Re: [PATCH] gdbstub: add support to Xfer:auxv:read: packet

2020-07-21 Thread Lirong Yuan

On Tue, Mar 31, 2020 at 12:20 AM Alex Bennée  wrote:
>
>
> Lirong Yuan  writes:
>
> > On Mon, Mar 30, 2020 at 12:47 PM Alex Bennée  wrote:
> >
> >>
> >> Lirong Yuan  writes:
> >>
> >> > On Sat, Mar 21, 2020 at 6:56 AM Alex Bennée 
> >> wrote:
> >> >
> >> >>
> >> >> Lirong Yuan  writes:
> >> >>
> >> >> > On Fri, Mar 20, 2020 at 2:17 AM Alex Bennée 
> >> >> wrote:
> >> >> 
> >> >> >>
> >> >> >> Sorry I missed this on my radar. There was a minor re-factor of
> >> gdbstub
> >> >> >> that was just merged which will mean this patch needs a re-base to
> >> use
> >> >> >> g_string_* functions to expand stings.
> >> >> >>
> >> >> >> Also we have some simple gdbstub tests now - could we come up with a
> >> >> >> multiarch gdbstub test to verify this is working properly?
> >> >> >>
> >> >> 
> >> >> > For sure, I will re-base this patch to use g_string_* functions.
> >> >> >
> >> >> > Currently we are using qemu aarch64. I am not sure how to do this yet,
> >> >> but
> >> >> > I could try to add something to
> >> >> > https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub
> >> >>
> >> >> If the auxv support is appropriate to all linux-user targets you can
> >> >> plumb it into the multiarch tests - you can even use the existing
> >> >> binaries.
> >> >>
> >> >> So you need:
> >> >>
> >> >>   - a stanza in the makefiles to launch the test (see
> >> >> tests/tcg/aarch64/Makefile.target)
> >> >>
> >> >>   - a .py test script that manipulates gdbstub to check things are
> >> working
> >> >>
> >> >> So something like:
> >> >>
> >> >> .PHONY: gdbstub-foo-binary
> >> >> run-gdbstub-foo-binary: foo-binary
> >> >> $(call run-test, $@, $(GDB_SCRIPT) \
> >> >> --gdb $(HAVE_GDB_BIN) \
> >> >> --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> >> >> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-foo.py, \
> >> >> "basic gdbstub FOO support")
> >> >>
> >> >>
> >> >> >
> >> >> > Does this sound good?
> >> >>
> >> >> Hope that helps.
> >> >>
> >> >> >
> >> >> > Thanks!
> >> >> > Lirong
> >> >>
> >> >>
> >> >> --
> >> >> Alex Bennée
> >> >>
> >> >
> >> > Hi Alex,
> >> >
> >> > Thanks for the instructions, very helpful!
> >> >
> >> > I rebased this patch to use g_string_* functions, and the link to
> >> patchwork
> >> > is:
> >> > http://patchwork.ozlabs.org/patch/1264125/
> >> > Could you help take another look?
> >> >
> >> > Regarding testing, I looked at some instructions for running tests, e.g.
> >> > https://github.com/qemu/qemu/blob/master/docs/devel/testing.rst
> >> > https://wiki.qemu.org/Testing
> >> > However I still could not get the tests for aarch64 to run. Do you know
> >> how
> >> > to run the aarch64 or multi-arch tests?
> >>
> >> The aarch64 ones run with "make run-tcg-tests-aarch64-linux-user" add
> >> V=1 to see the details.
> >>
> >> > Also there aren't any existing gdb stub tests that try to read
> >> > uninterpreted bytes from the target’s special data area identified by a
> >> > keyword:
> >> >
> >> https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#qXfer-auxiliary-vector-read
> >> > I looked at some other gdb stub tests, but they did not seem to send any
> >> > queries:
> >> > https://github.com/qemu/qemu/tree/master/tests/tcg/aarch64/gdbstub
> >> > So I am not sure how to set up one for "Xfer:auxv:read:" packets...
> >> > Are there plans to add more tests for other packets like
> >> > "Xfer:features:read:"?
> >> > I'd be happy to add a test if there is an example of how to do it. :)
> >>
> >> What would you do from a normal gdb command line. At the very least you
> >> run the same command with gdb.execute(), e.g.:
> >>
> >>   gdb.execute("set confirm off")
> >>
> >> is the same as typing
> >>
> >>   set confirm off
> >>
> >> at the gdb command prompt.
> >>
> >> >
> >> > Thanks,
> >> > Lirong
> >>
> >>
> >> --
> >> Alex Bennée
> >>
> >
> > Hey Alex,
> >
> > I tried to run the test but they were skipped. Do you know if there's any
> > other flag that needs to be set?
> >
> > $ make run-tcg-tests-aarch64-linux-user
> > make[1]: Entering directory '/usr/local/google/home/yuanzi/qemu/slirp'
> > make[1]: Nothing to be done for 'all'.
> > make[1]: Leaving directory '/usr/local/google/home/yuanzi/qemu/slirp'
> >   BUILD   TCG tests for aarch64-linux-user
> >   BUILD   aarch64-linux-user guest-tests SKIPPED
> >   RUN TCG tests for aarch64-linux-user
> >   RUN tests for aarch64-linux-user SKIPPED
>
> Ahh you either need to have docker enabled or the aarch64 compilers in
> your path when you run configure so the test programs can be built. The
> details are in docs/devel/testing.rst
>
> > I don't think any command needs to be run. It should just send the query
> > automatically.
> > Could we assume that it will work the same in the test?
>
> If that is enough to exercise the code. Can we not validate the data somehow?
>
>
> --
> Alex Bennée

Hi Alex,

Thanks for the suggestions! I just installed docker and ran the

[Bug 1888417] [NEW] Latest QEMU git build on Arch linux causes PCI Passthrough host to hang on guest reboot.

2020-07-21 Thread Jason

Public bug reported:

Current Arch linux release, up-to-date as of 7/21/2020.

Running a windows 7 virtual machine (also happens with windows 10,
possibly more OSes), with an nvidia GTX 1060 passthrough, if the VM is
attempted to be restarted, either through the guest interface, or by
libvirt's gui interface "Virtual Machine Manager", it hangs in a
"paused" state once the VM shutsdown, and just before the reboot can
take place.  A force-stop of the VM allows the VM to be properly booted
without any disk error checks, alluding to a clean shutdown, but failed
reboot.  The VM can be properly shutdown using the guests shutdown
function, or the libvirt manager shutdown, without any hangs.  Reverting
to Arch stable build QEMU 5.0.0-7 fixes the issue.

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: force-stop hangs nvidia pci reboot

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1888417

Title:
  Latest QEMU git build on Arch linux causes PCI Passthrough host to
  hang on guest reboot.

Status in QEMU:
  New

Bug description:
  Current Arch linux release, up-to-date as of 7/21/2020.

  Running a windows 7 virtual machine (also happens with windows 10,
  possibly more OSes), with an nvidia GTX 1060 passthrough, if the VM is
  attempted to be restarted, either through the guest interface, or by
  libvirt's gui interface "Virtual Machine Manager", it hangs in a
  "paused" state once the VM shutsdown, and just before the reboot can
  take place.  A force-stop of the VM allows the VM to be properly
  booted without any disk error checks, alluding to a clean shutdown,
  but failed reboot.  The VM can be properly shutdown using the guests
  shutdown function, or the libvirt manager shutdown, without any hangs.
  Reverting to Arch stable build QEMU 5.0.0-7 fixes the issue.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1888417/+subscriptions

Re: [PATCH-for-5.1] hw/ide/ahci: Do not dma_memory_unmap(NULL)


On 7/18/20 3:28 AM, Philippe Mathieu-Daudé wrote:

libFuzzer triggered the following assertion:

   cat << EOF | qemu-system-i386 -M pc-q35-5.0 \
 -nographic -monitor none -serial none -qtest stdio
   outl 0xcf8 0x8000fa24
   outl 0xcfc 0xe1068000
   outl 0xcf8 0x8000fa04
   outw 0xcfc 0x7
   outl 0xcf8 0x8000fb20
   write 0xe1068304 0x1 0x21
   write 0xe1068318 0x1 0x21
   write 0xe1068384 0x1 0x21
   write 0xe1068398 0x2 0x21
   EOF
   qemu-system-i386: exec.c:3621: address_space_unmap: Assertion `mr != NULL' 
failed.
   Aborted (core dumped)

This is because we don't check the return value from dma_memory_map()
which can return NULL, then we call dma_memory_unmap(NULL) which is
illegal. Fix by only unmap if the value is not NULL (and the size is
not the expected one).

Cc: qemu-sta...@nongnu.org
Reported-by: Alexander Bulekov 
Fixes: f6ad2e32f8 ("ahci: add ahci emulation")
BugLink: https://bugs.launchpad.net/qemu/+bug/1884693
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/ahci.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 009120f88b..4f596cb9ce 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -250,7 +250,7 @@ static void map_page(AddressSpace *as, uint8_t **ptr, 
uint64_t addr,
  }
  
  *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);

-if (len < wanted) {
+if (len < wanted && *ptr) {
  dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
  *ptr = NULL;
  }



Reviewed-by: John Snow 

Thanks! this one wound up being easy.

Re: [PATCH v2 2/4] m25p80: Improve command handling for Jedec commands

2020-07-21 Thread Cédric Le Goater

Hello,

On 2/6/20 7:32 PM, Guenter Roeck wrote:
> When requesting JEDEC data using the JEDEC_READ command, the Linux kernel
> always requests 6 bytes. The current implementation only returns three
> bytes, and interprets the remaining three bytes as new commands.
> While this does not matter most of the time, it is at the very least
> confusing. To avoid the problem, always report up to 6 bytes of JEDEC
> data. Fill remaining data with 0.
> 
> Signed-off-by: Guenter Roeck 
> ---
> v2: Split patch into two parts; improved decription
> 
>  hw/block/m25p80.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 5ff8d270c4..53bf63856f 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1040,8 +1040,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  for (i = 0; i < s->pi->id_len; i++) {
>  s->data[i] = s->pi->id[i];
>  }
> +for (; i < SPI_NOR_MAX_ID_LEN; i++) {
> +s->data[i] = 0;
> +}

This is breaking an old firmware (Linux version 2.6.28.9) for a SuperMicro
board : 

https://www.supermicro.com/en/products/motherboard/X11SSL-F 

which expects the flash ID to repeat. Erik solved the problem with the patch 
below and I think it makes sense to wrap around. Anyone knows what should be 
the expected behavior ? 

Thanks,

C. 


diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8227088441..5000930800 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1041,7 +1041,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 s->data[i] = s->pi->id[i];
 }
 for (; i < SPI_NOR_MAX_ID_LEN; i++) {
-s->data[i] = 0;
+s->data[i] = s->pi->id[i % s->pi->id_len];
 }

 s->len = SPI_NOR_MAX_ID_LEN;

Re: [PATCH-for-5.1] hw/ide/ahci: Do not dma_memory_unmap(NULL)

On 7/18/20 9:28 AM, Philippe Mathieu-Daudé wrote:
> libFuzzer triggered the following assertion:
> 
>   cat << EOF | qemu-system-i386 -M pc-q35-5.0 \
> -nographic -monitor none -serial none -qtest stdio
>   outl 0xcf8 0x8000fa24
>   outl 0xcfc 0xe1068000
>   outl 0xcf8 0x8000fa04
>   outw 0xcfc 0x7
>   outl 0xcf8 0x8000fb20
>   write 0xe1068304 0x1 0x21
>   write 0xe1068318 0x1 0x21
>   write 0xe1068384 0x1 0x21
>   write 0xe1068398 0x2 0x21
>   EOF
>   qemu-system-i386: exec.c:3621: address_space_unmap: Assertion `mr != NULL' 
> failed.
>   Aborted (core dumped)
> 
> This is because we don't check the return value from dma_memory_map()
> which can return NULL, then we call dma_memory_unmap(NULL) which is
> illegal. Fix by only unmap if the value is not NULL (and the size is
> not the expected one).

Maybe worth mentioning it was hidden before but got revealed by commit
77f55eac6c ("exec: set map length to zero when returning NULL").

Cc'ing commit 77f55eac6c's reviewers.

> Cc: qemu-sta...@nongnu.org
> Reported-by: Alexander Bulekov 
> Fixes: f6ad2e32f8 ("ahci: add ahci emulation")
> BugLink: https://bugs.launchpad.net/qemu/+bug/1884693
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ide/ahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 009120f88b..4f596cb9ce 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -250,7 +250,7 @@ static void map_page(AddressSpace *as, uint8_t **ptr, 
> uint64_t addr,
>  }
>  
>  *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> -if (len < wanted) {
> +if (len < wanted && *ptr) {
>  dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
>  *ptr = NULL;
>  }
>

Re: [PULL 0/3] Fixes 20200721 patches

On Tue, 21 Jul 2020 at 15:05, Gerd Hoffmann  wrote:
>
> The following changes since commit af3d69058e09bede9900f266a618ed11f76f49f3:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20200720' into staging (2020-07-20 
> 15:58:07 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/fixes-20200721-pull-request
>
> for you to fetch changes up to d87350b065128e8156e7aca93e89a1ab9e5fa63d:
>
>   module: ignore NULL type (2020-07-21 10:56:51 +0200)
>
> 
> fixes for xhci and modular builds.
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

Re: [PATCH for-5.1] docs/system/arm/virt: Document mte machine option

2020-07-21 Thread Richard Henderson

On 7/21/20 7:35 AM, Peter Maydell wrote:
> Commit 6a0b7505f1fd6769c which added documentation of the virt board
> crossed in the post with commit 6f4e1405b91da0d0 which added a new
> 'mte' machine option. Update the docs to include the new option.
> 
> Signed-off-by: Peter Maydell 
> ---
>  docs/system/arm/virt.rst | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH for-5.1] target/arm: Always pass cacheattr in S1_ptw_translate

2020-07-21 Thread Jan Kiszka


On 21.07.20 18:35, Richard Henderson wrote:

When we changed the interface of get_phys_addr_lpae to require
the cacheattr parameter, this spot was missed.  The compiler is
unable to detect the use of NULL vs the nonnull attribute here.

Fixes: 7e98e21c098
Reported-by: Jan Kiszka 
Signed-off-by: Richard Henderson 
---
  target/arm/helper.c | 19 ++-
  1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c69a2baf1d..8ef0fb478f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10204,21 +10204,11 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
ARMMMUIdx mmu_idx,
  int s2prot;
  int ret;
  ARMCacheAttrs cacheattrs = {};
-ARMCacheAttrs *pcacheattrs = NULL;
-
-if (env->cp15.hcr_el2 & HCR_PTW) {
-/*
- * PTW means we must fault if this S1 walk touches S2 Device
- * memory; otherwise we don't care about the attributes and can
- * save the S2 translation the effort of computing them.
- */
-pcacheattrs = 
-}
  
  ret = get_phys_addr_lpae(env, addr, MMU_DATA_LOAD, ARMMMUIdx_Stage2,

   false,
   , , , , fi,
- pcacheattrs);
+ );
  if (ret) {
  assert(fi->type != ARMFault_None);
  fi->s2addr = addr;
@@ -10226,8 +10216,11 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
ARMMMUIdx mmu_idx,
  fi->s1ptw = true;
  return ~0;
  }
-if (pcacheattrs && (pcacheattrs->attrs & 0xf0) == 0) {
-/* Access was to Device memory: generate Permission fault */
+if ((env->cp15.hcr_el2 & HCR_PTW) && (cacheattrs.attrs & 0xf0) == 0) {
+/*
+ * PTW set and S1 walk touched S2 Device memory:
+ * generate Permission fault.
+ */
  fi->type = ARMFault_Permission;
  fi->s2addr = addr;
  fi->stage2 = true;



Jup:

Tested-by: Jan Kiszka 

Thanks,
Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [RFC v2 2/3] libvhost-user: handle endianness as mandated by the spec

2020-07-21 Thread Halil Pasic

On Tue, 21 Jul 2020 09:40:10 -0400
"Michael S. Tsirkin"  wrote:

> On Fri, Jul 17, 2020 at 11:29:28AM +0200, Marc Hartmayer wrote:
> > Since virtio existed even before it got standardized, the virtio
> > standard defines the following types of virtio devices:
> > 
> >  + legacy device (pre-virtio 1.0)
> >  + non-legacy or VIRTIO 1.0 device
> >  + transitional device (which can act both as legacy and non-legacy)
> > 
> > Virtio 1.0 defines the fields of the virtqueues as little endian,
> > while legacy uses guest's native endian [1]. Currently libvhost-user
> > does not handle virtio endianness at all, i.e. it works only if the
> > native endianness matches with whatever is actually needed. That means
> > things break spectacularly on big-endian targets. Let us handle virtio
> > endianness for non-legacy as required by the virtio specification
> > [1]. We will fence non-legacy virtio devices with the upcoming patch.
> > 
> > [1] 
> > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-210003
> > 
> > Signed-off-by: Marc Hartmayer 
> > 
> > ---
> > Note: As we don't support legacy virtio devices  
> 
> Who's "we" in this sentence? vhost user supports legacy generally ...

In that sentence "we" is the library "libvhost-user". I would like
to avoid s390x being an oddball regarding this. Marc's previous
version made an attempt at correct endianness handling for legacy
and non-legacy. That spawned a discussion on how we don't want
legacy devices in this context. This series makes what seemed to be the
consensus reached in that discussion explicit: namely that libvhost-user
does not support legacy-virtio.

Regards,
Halil

Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

On Tue, 21 Jul 2020 at 17:07, Philippe Mathieu-Daudé  wrote:
>
> Hi Stefan,
>
> I'm trying to understand what is modelling the
> TYPE_TPM_TIS_ISA device.
>
> It inherits from TYPE_ISA_DEVICE, so I expected
> to see an ISA device, but then I noticed:
>
> 1/ it doesn't use the ISA I/O space, it directly
> maps the device in the system memory at a fixed
> address that is not addressable by the ISA bus:
>
> #define TPM_TIS_ADDR_BASE   0xFED4

Why do you think this is mapping to the system memory?
tpm_tis_isa_realizefn() does:

memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
TPM_TIS_ADDR_BASE, >mmio);

which puts it into the ISA memory address space.

The weird thing about this is not which AS it's
going in but the fact that the TPM_TIS_ADDR_BASE
is way higher than an actual ISA bus can address
(so for instance it's out of range of the size of
the ISA memory window on the Jazz board).

> 2/ it is not plugged to an ISA BUS (ISABus*)

Won't it autoplug into the ISA bus if you say "-device tpm-tis",
the same as any other ISA device ?

> 3/ no machine plug it using isa_register_ioport()
>(it is not registered to the ISA memory space)

There's no requirement for an ISA device to have IO ports...

thanks
-- PMM

Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

2020-07-21 Thread Stefan Berger


On 7/21/20 12:02 PM, Philippe Mathieu-Daudé wrote:

Hi Stefan,

I'm trying to understand what is modelling the
TYPE_TPM_TIS_ISA device.

It inherits from TYPE_ISA_DEVICE, so I expected
to see an ISA device, but then I noticed:

1/ it doesn't use the ISA I/O space, it directly
maps the device in the system memory at a fixed
address that is not addressable by the ISA bus:

#define TPM_TIS_ADDR_BASE   0xFED4

2/ it is not plugged to an ISA BUS (ISABus*)

3/ no machine plug it using isa_register_ioport()
(it is not registered to the ISA memory space)

4/ the only thing slightly related to ISA is it
checks the IRQ number is < ISA_NUM_IRQS


So it seems this is a plain SysBusDevice. But then
there is TYPE_TPM_TIS_SYSBUS... What is the difference?


The TIS is modeled as an ISA device as an alternative to being connected 
to the LPC bus, to which it would typically be connected to. I believe 
we also have the ISA.TPM because of Windows only accepting it. Maybe 
this was not quite correct, but does it cause issues?


    } else {
    dev = aml_device("ISA.TPM");
    aml_append(dev, aml_name_decl("_HID",
aml_eisaid("PNP0C31")));
    }






Thanks,

Phil.

Re: [RFC PATCH-for-5.1] hw/ide: Do not block for AIO while resetting a drive


On 7/20/20 6:02 AM, Stefan Hajnoczi wrote:

On Fri, Jul 17, 2020 at 07:19:38PM +0200, Philippe Mathieu-DaudÃ© wrote:

Last minute chat:
19:01  f4bug: use bdrv_aio_cancel_async() if possible because it 
won't block the current thread.
19:02  f4bug: For example, in device emulation code where the guest 
has requested to cancel an I/O request it's often possible to use the async version.
19:02  f4bug: But in synchronous code like device reset it may be 
necessary to use the synchronous (blocking) bdrv_aio_cancel() API instead. :(
19:14  f4bug: The way to decide is: will the current function return 
to the event loop and is there someone who will handle the request completion 
callback when cancel finishes?
19:14  f4bug: If the next line of code requires the request to 
finished then async cancel cannot be used.
19:15  f4bug: On the other hand, if the function can return and it's 
okay for the request to cancel at some future time then you can use async.

So I'll revisit this patch :)
---
  hw/ide/core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..e3a9ce7d25 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1315,7 +1315,8 @@ static void ide_reset(IDEState *s)
  trace_ide_reset(s);
  
  if (s->pio_aiocb) {

-blk_aio_cancel(s->pio_aiocb);
+/* If there is a pending AIO callback, invoke it now. */
+blk_aio_cancel_async(s->pio_aiocb);


This is a place where an async call is not allowed. The completion
function must be called right away (synchronously) before we can
continue resetting the device.

I sent a patch that allows bdrv_aio_cancel() to find the AioContext so
it can call aio_poll().

Stefan



OK, dropping Phil's patch here.

--js

Re: [PULL 0/2] Net patches

On Tue, 21 Jul 2020 at 14:34, Jason Wang  wrote:
>
> The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:
>
>   Merge remote-tracking branch 
> 'remotes/huth-gitlab/tags/pull-request-2020-07-21' into staging (2020-07-21 
> 10:24:38 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jasowang/qemu.git tags/net-pull-request
>
> for you to fetch changes up to 5519724a13664b43e225ca05351c60b4468e4555:
>
>   hw/net/xgmac: Fix buffer overflow in xgmac_enet_send() (2020-07-21 21:30:39 
> +0800)
>
> 
>
> 
> Andrew (1):
>   hw/net: Added plen fix for IPv6
>
> Mauro Matteo Cascella (1):
>   hw/net/xgmac: Fix buffer overflow in xgmac_enet_send()
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

[PATCH for-5.1] target/arm: Always pass cacheattr in S1_ptw_translate

2020-07-21 Thread Richard Henderson

When we changed the interface of get_phys_addr_lpae to require
the cacheattr parameter, this spot was missed.  The compiler is
unable to detect the use of NULL vs the nonnull attribute here.

Fixes: 7e98e21c098
Reported-by: Jan Kiszka 
Signed-off-by: Richard Henderson 
---
 target/arm/helper.c | 19 ++-
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c69a2baf1d..8ef0fb478f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10204,21 +10204,11 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
ARMMMUIdx mmu_idx,
 int s2prot;
 int ret;
 ARMCacheAttrs cacheattrs = {};
-ARMCacheAttrs *pcacheattrs = NULL;
-
-if (env->cp15.hcr_el2 & HCR_PTW) {
-/*
- * PTW means we must fault if this S1 walk touches S2 Device
- * memory; otherwise we don't care about the attributes and can
- * save the S2 translation the effort of computing them.
- */
-pcacheattrs = 
-}
 
 ret = get_phys_addr_lpae(env, addr, MMU_DATA_LOAD, ARMMMUIdx_Stage2,
  false,
  , , , , fi,
- pcacheattrs);
+ );
 if (ret) {
 assert(fi->type != ARMFault_None);
 fi->s2addr = addr;
@@ -10226,8 +10216,11 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
ARMMMUIdx mmu_idx,
 fi->s1ptw = true;
 return ~0;
 }
-if (pcacheattrs && (pcacheattrs->attrs & 0xf0) == 0) {
-/* Access was to Device memory: generate Permission fault */
+if ((env->cp15.hcr_el2 & HCR_PTW) && (cacheattrs.attrs & 0xf0) == 0) {
+/*
+ * PTW set and S1 walk touched S2 Device memory:
+ * generate Permission fault.
+ */
 fi->type = ARMFault_Permission;
 fi->s2addr = addr;
 fi->stage2 = true;
-- 
2.25.1

Re: [PATCH v3] introduce VFIO-over-socket protocol specificaion

2020-07-21 Thread Nikos Dragazis


Hi Thanos,

I had a quick look on the spec. Leaving some comments inline.

On 17/7/20 2:20 μ.μ., Thanos Makatos wrote:


This patch introduces the VFIO-over-socket protocol specification, which
is designed to allow devices to be emulated outside QEMU, in a separate
process. VFIO-over-socket reuses the existing VFIO defines, structs and
concepts.

It has been earlier discussed as an RFC in:
"RFC: use VFIO over a UNIX domain socket to implement device offloading"

Signed-off-by: John G Johnson 
Signed-off-by: Thanos Makatos 

---

Changed since v1:
   * fix coding style issues
   * update MAINTAINERS for VFIO-over-socket
   * add vfio-over-socket to ToC

Changed since v2:
   * fix whitespace

Regarding the build failure, I have not been able to reproduce it locally
using the docker image on my Debian 10.4 machine.
---
  MAINTAINERS |6 +
  docs/devel/index.rst|1 +
  docs/devel/vfio-over-socket.rst | 1135 +++
  3 files changed, 1142 insertions(+)
  create mode 100644 docs/devel/vfio-over-socket.rst

diff --git a/MAINTAINERS b/MAINTAINERS
index 030faf0..bb81590 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1732,6 +1732,12 @@ F: hw/vfio/ap.c
  F: docs/system/s390x/vfio-ap.rst
  L: qemu-s3...@nongnu.org
  
+VFIO-over-socket

+M: John G Johnson 
+M: Thanos Makatos 
+S: Supported
+F: docs/devel/vfio-over-socket.rst
+
  vhost
  M: Michael S. Tsirkin 
  S: Supported
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index ae6eac7..0439460 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -30,3 +30,4 @@ Contents:
 reset
 s390-dasd-ipl
 clocks
+   vfio-over-socket
diff --git a/docs/devel/vfio-over-socket.rst b/docs/devel/vfio-over-socket.rst
new file mode 100644
index 000..b474f23
--- /dev/null
+++ b/docs/devel/vfio-over-socket.rst
@@ -0,0 +1,1135 @@
+***
+VFIO-over-socket Protocol Specification
+***
+
+Version 0.1
+
+Introduction
+
+VFIO-over-socket, also known as vfio-user, is a protocol that allows a device


I think there is no point in having two names for the same protocol,
"vfio-over-socket" and "vfio-user".


+to be virtualized in a separate process outside of QEMU. VFIO-over-socket
+devices consist of a generic VFIO device type, living inside QEMU, which we
+call the client, and the core device implementation, living outside QEMU, which
+we call the server. VFIO-over-socket can be the main transport mechanism for
+multi-process QEMU, however it can be used by other applications offering
+device virtualization. Explaining the advantages of a
+disaggregated/multi-process QEMU, and device virtualization outside QEMU in
+general, is beyond the scope of this document.
+
+This document focuses on specifying the VFIO-over-socket protocol. VFIO has
+been chosen for the following reasons:
+
+1) It is a mature and stable API, backed by an extensively used framework.
+2) The existing VFIO client implementation (qemu/hw/vfio/) can be largely
+   reused.
+
+In a proof of concept implementation it has been demonstrated that using VFIO
+over a UNIX domain socket is a viable option. VFIO-over-socket is designed with
+QEMU in mind, however it could be used by other client applications. The
+VFIO-over-socket protocol does not require that QEMU's VFIO client
+implementation is used in QEMU. None of the VFIO kernel modules are required
+for supporting the protocol, neither in the client nor the server, only the
+source header files are used.
+
+The main idea is to allow a virtual device to function in a separate process in
+the same host over a UNIX domain socket. A UNIX domain socket (AF_UNIX) is
+chosen because we can trivially send file descriptors over it, which in turn
+allows:
+
+* Sharing of guest memory for DMA with the virtual device process.
+* Sharing of virtual device memory with the guest for fast MMIO.
+* Efficient sharing of eventfd's for triggering interrupts.
+
+However, other socket types could be used which allows the virtual device
+process to run in a separate guest in the same host (AF_VSOCK) or remotely
+(AF_INET). Theoretically the underlying transport doesn't necessarily have to
+be a socket, however we don't examine such alternatives. In this document we
+focus on using a UNIX domain socket and introduce basic support for the other
+two types of sockets without considering performance implications.
+
+This document does not yet describe any internal details of the server-side
+implementation, however QEMU's VFIO client implementation will have to be
+adapted according to this protocol in order to support VFIO-over-socket virtual
+devices.
+
+VFIO
+
+VFIO is a framework that allows a physical device to be securely passed through
+to a user space process; the kernel does not drive the device at all.


I would remove the last part: "the kernel does not drive the device at
all". Isn't that quite inaccurate?

What is TYPE_TPM_TIS_ISA? (Not an ISA Device)

Hi Stefan,

I'm trying to understand what is modelling the
TYPE_TPM_TIS_ISA device.

It inherits from TYPE_ISA_DEVICE, so I expected
to see an ISA device, but then I noticed:

1/ it doesn't use the ISA I/O space, it directly
maps the device in the system memory at a fixed
address that is not addressable by the ISA bus:

#define TPM_TIS_ADDR_BASE   0xFED4

2/ it is not plugged to an ISA BUS (ISABus*)

3/ no machine plug it using isa_register_ioport()
   (it is not registered to the ISA memory space)

4/ the only thing slightly related to ISA is it
checks the IRQ number is < ISA_NUM_IRQS


So it seems this is a plain SysBusDevice. But then
there is TYPE_TPM_TIS_SYSBUS... What is the difference?

Thanks,

Phil.

Re: [PATCH] target/i386: floatx80: avoid compound literals in static initializers



Philippe Mathieu-Daudé  writes:

> On 7/17/20 6:46 PM, Laszlo Ersek wrote:
>> On 07/17/20 11:26, Laszlo Ersek wrote:
>>> On 07/16/20 17:09, Philippe Mathieu-Daudé wrote:
 On 7/16/20 4:42 PM, Laszlo Ersek wrote:
> Quoting ISO C99 6.7.8p4, "All the expressions in an initializer for an
> object that has static storage duration shall be constant expressions or
> string literals".
>
> The compound literal produced by the make_floatx80() macro is not such a
> constant expression, per 6.6p7-9. (An implementation may accept it,
> according to 6.6p10, but is not required to.)
>
> Therefore using "floatx80_zero" and make_floatx80() for initializing
> "f2xm1_table" and "fpatan_table" is not portable. And gcc-4.8 in RHEL-7.6
> actually chokes on them:
>
>> target/i386/fpu_helper.c:871:5: error: initializer element is not 
>> constant
>>  { make_floatx80(0xbfff, 0x8000ULL),
>>  ^

 This reminds me of:

 commit 6fa9ba09dbf4eb8b52bcb47d6820957f1b77ee0b
 Author: Kamil Rytarowski 
 Date:   Mon Sep 4 23:23:06 2017 +0200

 target/m68k: Switch fpu_rom from make_floatx80() to 
 make_floatx80_init()

 GCC 4.7.2 on SunOS reports that the values assigned to array members
 are not
 real constants:

 target/m68k/fpu_helper.c:32:5: error: initializer element is not
 constant
 target/m68k/fpu_helper.c:32:5: error: (near initialization for
 'fpu_rom[0]')
 rules.mak:66: recipe for target 'target/m68k/fpu_helper.o' failed

 Convert the array to make_floatx80_init() to fix it.
 Replace floatx80_pi-like constants with make_floatx80_init() as they 
 are
 defined as make_floatx80().

 Reviewed-by: Philippe Mathieu-Daudé 

>
> We've had the make_floatx80_init() macro for this purpose since commit
> 3bf7e40ab914 ("softfloat: fix for C99", 2012-03-17), so let's use that
> macro again.
>
> Fixes: eca30647fc07
> Fixes: ff57bb7b6326
> Link: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06566.html
> Link: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04714.html
> Cc: Alex Bennée 
> Cc: Aurelien Jarno 
> Cc: Eduardo Habkost 
> Cc: Joseph Myers 
> Cc: Paolo Bonzini 
> Cc: Peter Maydell 
> Cc: Richard Henderson 
> Signed-off-by: Laszlo Ersek 
> ---
>
> Notes:
> I can see that there are test cases under "tests/tcg/i386", but I 
> don't
> know how to run them.

 Yeah it is not easy to figure...

 Try 'make run-tcg-tests-i386-softmmu'
 but you need docker :^)
>>>
>>> That worked, thanks! Even without Docker: I just had to add
>>>
>>>   --cross-cc-i386=gcc
>>>
>>> to my ./configure flags.
>>>
>> 
>> Also -- I meant to, but I forgot to put "for-5.1" in the subject prefix;
>> sorry about that.
>
> Alex, as Paolo is not available, can this go via your tree?

Ok queued to for-5.1/fixes-for-rc1-v2, thanks.

>
>> 
>> Laszlo
>> 


-- 
Alex Bennée

Re: Testing the virtio-vhost-user QEMU patch

2020-07-21 Thread Alyssa Ross

Stefan Hajnoczi  writes:

> On Tue, Jul 21, 2020 at 07:14:38AM +, Alyssa Ross wrote:
>> Hi -- I hope it's okay me reaching out like this.
>> 
>> I've been trying to test out the virtio-vhost-user implementation that's
>> been posted to this list a couple of times, but have been unable to get
>> it to boot a kernel following the steps listed either on
>>  or
>> .
>> 
>> Specifically, the kernel appears to be unable to write to the
>> virtio-vhost-user device's PCI registers.  I've included the full panic
>> output from the kernel at the end of this message.  The panic is
>> reproducible with two different kernels I tried (with different configs
>> and versions).  I tried both versions of the virtio-vhost-user I was
>> able to find[1][2], and both exhibited the same behaviour.
>> 
>> Is this a known issue?  Am I doing something wrong?
>
> Hi,
> Unfortunately I'm not sure what the issue is. This is an early
> virtio-pci register access before a driver for any specific device type
> (net, blk, vhost-user, etc) comes into play.
>
> Did you test the git trees linked below or did you rebase the commits
> on top of your own QEMU tree?

I tested the git trees.  For your one I had to make a slight
modification to delete the memfd syscall wrapper in util/memfd.c, since
it conflicted with the one that is now provided by Glibc.  Nikos's tree
I used totally unmodified.

> Is your guest kernel a stock kernel.org/distro kernel or has it been
> modified (especially with security patches)?

I tried a slightly modified Chromium OS kernel (5.4.23), and a stock
Ubuntu 18.10 kernel (4.15.0).  I think the most "normal" setup I tried
was building QEMU on Fedora 32, and then attempting to boot a freshly
installed Ubuntu Server 18.10 VM with

-chardev socket,id=chardev0,path=vhost-user.sock,server,nowait \
-device virtio-vhost-user-pci,chardev=chardev0

(The crash was reproducible with the full QEMU command lines in the
write-ups, but these seemed to be the load-bearing bits.)

> If no one else knows what is wrong here then it will be necessary to
> check the Intel manuals to figure out the exact meaning of
> "error_code(0x000b) - reserved bit violation" and why Linux triggers it
> with "PGD 3b128067 P4D 3b128067 PUD 3b129067 PMD 3b12a067 PTE
> 80200073".

Thanks for your insight.  Now I at least have a place to start if nobody
else knows what's up. :)

Re: [PATCH v1 4/5] util: add qemu_get_host_physmem utility function



Richard Henderson  writes:

> On 7/17/20 3:51 AM, Alex Bennée wrote:
>> +size_t qemu_get_host_physmem(void)
>> +{
>> +#ifdef _SC_PHYS_PAGES
>> +long pages = sysconf(_SC_PHYS_PAGES);
>> +if (pages > 0) {
>> +return pages * qemu_real_host_page_size;
>> +}
>> +#endif
>> +return 0;
>> +}
>
> Is it worth examining our own RLIMIT_{AS,DATA} as well?

We don't anywhere else in the code so for now I'd say not.

>
> I suppose that's not usually what is tweaked in the example of a ram-limited
> container...
>
>
> r~


-- 
Alex Bennée

please try to avoid sending pullreqs late on release-candidate day

It is not helpful if everybody sends their pullrequests late
on the Tuesday afternoon, as there just isn't enough time in the
day to merge test and apply them all before I have to cut the tag.
Please, if you can, try to send pullrequests earlier, eg Monday.

thanks
-- PMM

Re: [PULL 0/1] QAPI patches patches for 2020-07-21

On Tue, 21 Jul 2020 at 14:07, Markus Armbruster  wrote:
>
> The following changes since commit af3d69058e09bede9900f266a618ed11f76f49f3:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20200720' into staging (2020-07-20 
> 15:58:07 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2020-07-21
>
> for you to fetch changes up to cbf97d5b799f4bc47b9e825100d1a98d3cf77c80:
>
>   qapi: Fix visit_type_STRUCT() not to fail for null object (2020-07-21 
> 14:38:23 +0200)
>
> 
> QAPI patches patches for 2020-07-21
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

[PULL 3/3] iotests: Test sparseness for qemu-img convert -n

Signed-off-by: Kevin Wolf 
Message-Id: <20200721135520.72355-3-kw...@redhat.com>
Reviewed-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/122 | 30 ++
 tests/qemu-iotests/122.out | 17 +
 2 files changed, 47 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index dfd1cd05d6..0f3d4ca851 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -281,6 +281,36 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" 
"$TEST_IMG".orig
 
 $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
 
+echo
+echo '=== -n to an empty image ==='
+echo
+
+TEST_IMG="$TEST_IMG".orig _make_test_img 64M
+
+# Convert with -n, which should not result in a fully allocated image, not even
+# with compat=0.10 (because the target doesn't have a backing file)
+for compat in "1.1" "0.10"; do
+IMGOPTS="compat=$compat" _make_test_img 64M
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG".orig "$TEST_IMG"
+$QEMU_IMG map --output=json "$TEST_IMG"
+done
+
+echo
+echo '=== -n to an empty image with a backing file ==='
+echo
+
+TEST_IMG="$TEST_IMG".orig _make_test_img 64M
+TEST_IMG="$TEST_IMG".base _make_test_img 64M
+
+# Convert with -n, which should still not result in a fully allocated image for
+# compat=1.1 (because it can use zero clusters), but it should be fully
+# allocated with compat=0.10
+for compat in "1.1" "0.10"; do
+IMGOPTS="compat=$compat" _make_test_img -b "$TEST_IMG".base -F $IMGFMT 64M
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG".orig "$TEST_IMG"
+$QEMU_IMG map --output=json "$TEST_IMG"
+done
+
 echo
 echo '=== -n -B to an image without a backing file ==='
 echo
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index f1f195ed77..3a3e121d57 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -229,6 +229,23 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Images are identical.
 
+=== -n to an empty image ===
+
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
+
+=== -n to an empty image with a backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, 
"offset": 327680}]
+
 === -n -B to an image without a backing file ===
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
-- 
2.25.4

[PULL 0/3] Block layer patches for 5.1.0-rc1

The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:

  Merge remote-tracking branch 
'remotes/huth-gitlab/tags/pull-request-2020-07-21' into staging (2020-07-21 
10:24:38 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 4a01e27ddcb5921efd68820d05d85ba71293fea6:

  iotests: Test sparseness for qemu-img convert -n (2020-07-21 17:44:35 +0200)


Block layer patches:

- file-posix: Handle `EINVAL` fallocate return value
- qemu-img convert -n: Keep qcow2 v2 target sparse


Antoine Damhet (1):
  file-posix: Handle `EINVAL` fallocate return value

Kevin Wolf (2):
  qcow2: Implement v2 zero writes with discard if possible
  iotests: Test sparseness for qemu-img convert -n

 block/file-posix.c |  6 +-
 block/qcow2-cluster.c  |  9 -
 tests/qemu-iotests/122 | 30 ++
 tests/qemu-iotests/122.out | 17 +
 4 files changed, 60 insertions(+), 2 deletions(-)

[PULL 2/3] qcow2: Implement v2 zero writes with discard if possible

qcow2 version 2 images don't support the zero flag for clusters, so for
write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
writes. If the image doesn't have a backing file, we can do better: Just
discard the respective clusters.

This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
to assume that the existing target image may contain any data, so it has
to write zeroes. Without this patch, this results in a fully allocated
target image, even if the source image was empty.

Reported-by: Nir Soffer 
Signed-off-by: Kevin Wolf 
Message-Id: <20200721135520.72355-2-kw...@redhat.com>
Reviewed-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 block/qcow2-cluster.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4b5fc8c4a7..a677ba9f5c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1797,8 +1797,15 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t 
offset,
 assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
 
-/* The zero flag is only supported by version 3 and newer */
+/*
+ * The zero flag is only supported by version 3 and newer. However, if we
+ * have no backing file, we can resort to discard in version 2.
+ */
 if (s->qcow_version < 3) {
+if (!bs->backing) {
+return qcow2_cluster_discard(bs, offset, bytes,
+ QCOW2_DISCARD_REQUEST, false);
+}
 return -ENOTSUP;
 }
 
-- 
2.25.4

[PULL 1/3] file-posix: Handle `EINVAL` fallocate return value

From: Antoine Damhet 

The `detect-zeroes=unmap` option may issue unaligned
`FALLOC_FL_PUNCH_HOLE` requests, raw block devices can (and will) return
`EINVAL`, qemu should then write the zeroes to the blockdev instead of
issuing an `IO_ERROR`.

The problem can be reprodced like this:

$ qemu-io -c 'write -P 0 42 1234' --image-opts 
driver=host_device,filename=/dev/loop0,detect-zeroes=unmap
write failed: Invalid argument

Signed-off-by: Antoine Damhet 
Message-Id: <20200717135603.51180-1-antoine.dam...@blade-group.com>
Signed-off-by: Kevin Wolf 
---
 block/file-posix.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 8cc39a1ef6..9a00d4190a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1698,7 +1698,11 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque)
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
 int ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
aiocb->aio_offset, aiocb->aio_nbytes);
-if (ret != -ENOTSUP) {
+switch (ret) {
+case -ENOTSUP:
+case -EINVAL:
+break;
+default:
 return ret;
 }
 #endif
-- 
2.25.4

[PULL 3/3] qom: Make info qom-tree sort children more efficiently

Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
sorts children the simple, stupid, quadratic way.  I thought the
number of children would be small enough for this not to matter.  I
was wrong: there are outliers with several hundred children, e.g ARM
machines nuri and smdkc210 each have a node with 513 children.

While n^2 sorting isn't noticeable in normal, human usage even for
n=513, it can be quite noticeable in certain automated tests.  In
particular, the sort made device-introspect-test even slower.  Commit
3e7b80f84d "tests: improve performance of device-introspect-test" just
fixed that by cutting back its excessive use of "info qom-tree".
Sorting more efficiently makes sense regardless, so do it.

Signed-off-by: Markus Armbruster 
Message-Id: <20200714160202.3121879-6-arm...@redhat.com>
Reviewed-by: Dr. David Alan Gilbert 
---
 qom/qom-hmp-cmds.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index 4032c96089..8861a109d5 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -94,25 +94,23 @@ typedef struct QOMCompositionState {
 
 static void print_qom_composition(Monitor *mon, Object *obj, int indent);
 
-static int qom_composition_compare(const void *a, const void *b, void *ignore)
+static int qom_composition_compare(const void *a, const void *b)
 {
-return g_strcmp0(object_get_canonical_path_component(a),
- object_get_canonical_path_component(b));
+return g_strcmp0(object_get_canonical_path_component(*(Object **)a),
+ object_get_canonical_path_component(*(Object **)b));
 }
 
 static int insert_qom_composition_child(Object *obj, void *opaque)
 {
-GQueue *children = opaque;
-
-g_queue_insert_sorted(children, obj, qom_composition_compare, NULL);
+g_array_append_val(opaque, obj);
 return 0;
 }
 
 static void print_qom_composition(Monitor *mon, Object *obj, int indent)
 {
+GArray *children = g_array_new(false, false, sizeof(Object *));
 const char *name;
-GQueue children;
-Object *child;
+int i;
 
 if (obj == object_get_root()) {
 name = "";
@@ -122,11 +120,14 @@ static void print_qom_composition(Monitor *mon, Object 
*obj, int indent)
 monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
object_get_typename(obj));
 
-g_queue_init();
-object_child_foreach(obj, insert_qom_composition_child, );
-while ((child = g_queue_pop_head())) {
-print_qom_composition(mon, child, indent + 2);
+object_child_foreach(obj, insert_qom_composition_child, children);
+g_array_sort(children, qom_composition_compare);
+
+for (i = 0; i < children->len; i++) {
+print_qom_composition(mon, g_array_index(children, Object *, i),
+  indent + 2);
 }
+g_array_free(children, TRUE);
 }
 
 void hmp_info_qom_tree(Monitor *mon, const QDict *dict)
-- 
2.26.2

[PULL 1/3] qom: Change object_get_canonical_path_component() not to malloc

object_get_canonical_path_component() returns a malloced copy of a
property name on success, null on failure.

19 of its 25 callers immediately free the returned copy.

Change object_get_canonical_path_component() to return the property
name directly.  Since modifying the name would be wrong, adjust the
return type to const char *.

Drop the free from the 19 callers become simpler, add the g_strdup()
to the other six.

Signed-off-by: Markus Armbruster 
Message-Id: <20200714160202.3121879-4-arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Li Qiang 
---
 include/qom/object.h   |  2 +-
 backends/hostmem.c |  2 +-
 block/throttle-groups.c|  2 +-
 gdbstub.c  |  2 +-
 hw/arm/xlnx-zynqmp.c   |  6 ++
 hw/block/nvme.c|  5 ++---
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c  |  5 ++---
 hw/mem/nvdimm.c|  5 ++---
 hw/mem/pc-dimm.c   |  5 ++---
 hw/misc/ivshmem.c  |  5 ++---
 hw/ppc/spapr_drc.c |  3 +--
 hw/virtio/virtio-crypto.c  |  5 ++---
 hw/virtio/virtio-mem.c |  6 ++
 hw/virtio/virtio-pmem.c|  5 ++---
 iothread.c |  9 -
 net/net.c  |  6 ++
 qom/object.c   |  7 +++
 qom/qom-hmp-cmds.c | 11 ---
 scsi/pr-manager-helper.c   |  3 +--
 scsi/pr-manager.c  |  2 +-
 softmmu/memory.c   |  2 +-
 hw/ppc/trace-events|  2 +-
 23 files changed, 41 insertions(+), 61 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 79c8f838b6..55d925d2c8 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1462,7 +1462,7 @@ Object *object_get_internal_root(void);
  * path is the path within the composition tree starting from the root.
  * %NULL if the object doesn't have a parent (and thus a canonical path).
  */
-char *object_get_canonical_path_component(const Object *obj);
+const char *object_get_canonical_path_component(const Object *obj);
 
 /**
  * object_get_canonical_path:
diff --git a/backends/hostmem.c b/backends/hostmem.c
index c614f1bdc1..4bde00e8e7 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -33,7 +33,7 @@ char *
 host_memory_backend_get_name(HostMemoryBackend *backend)
 {
 if (!backend->use_canonical_path) {
-return object_get_canonical_path_component(OBJECT(backend));
+return g_strdup(object_get_canonical_path_component(OBJECT(backend)));
 }
 
 return object_get_canonical_path(OBJECT(backend));
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 03a53c89ea..98fea7fd47 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -771,7 +771,7 @@ static void throttle_group_obj_complete(UserCreatable *obj, 
Error **errp)
 
 /* set group name to object id if it exists */
 if (!tg->name && tg->parent_obj.parent) {
-tg->name = object_get_canonical_path_component(OBJECT(obj));
+tg->name = g_strdup(object_get_canonical_path_component(OBJECT(obj)));
 }
 /* We must have a group name at this point */
 assert(tg->name);
diff --git a/gdbstub.c b/gdbstub.c
index 6950fd243f..f3a318cd7f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2059,7 +2059,7 @@ static void handle_query_thread_extra(GdbCmdContext 
*gdb_ctx, void *user_ctx)
 /* Print the CPU model and name in multiprocess mode */
 ObjectClass *oc = object_get_class(OBJECT(cpu));
 const char *cpu_model = object_class_get_name(oc);
-g_autofree char *cpu_name =
+const char *cpu_name =
 object_get_canonical_path_component(OBJECT(cpu));
 g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
 cpu->halted ? "halted " : "running");
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 772cfa3771..e14323c991 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -190,7 +190,7 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, 
XlnxZynqMPState *s,
 qdev_prop_set_uint32(DEVICE(>rpu_cluster), "cluster-id", 1);
 
 for (i = 0; i < num_rpus; i++) {
-char *name;
+const char *name;
 
 object_initialize_child(OBJECT(>rpu_cluster), "rpu-cpu[*]",
 >rpu_cpu[i],
@@ -204,7 +204,6 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, 
XlnxZynqMPState *s,
 } else {
 s->boot_cpu_ptr = >rpu_cpu[i];
 }
-g_free(name);
 
 object_property_set_bool(OBJECT(>rpu_cpu[i]), "reset-hivecs", true,
  _abort);
@@ -341,7 +340,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
**errp)
 
 /* Realize APUs before realizing the GIC. KVM requires this.  */
 for (i = 0; i < num_apus; i++) {
-char *name;
+const char *name;
 
 object_property_set_int(OBJECT(>apu_cpu[i]), "psci-conduit",
 QEMU_PSCI_CONDUIT_SMC, _abort);
@@ -354,7 +353,6 @@ static void

[PATCH v10] qga: add command guest-get-devices for reporting VirtIO devices

2020-07-21 Thread Tomáš Golembiovský

Add command for reporting devices on Windows guest. The intent is not so
much to report the devices but more importantly the driver (and its
version) that is assigned to the device. This gives caller the
information whether VirtIO drivers are installed and/or whether
inadequate driver is used on a device (e.g. QXL device with base VGA
driver).

Example:
[
{
  "driver-date": "2019-08-12",
  "driver-name": "Red Hat VirtIO SCSI controller",
  "driver-version": "100.80.104.17300",
  "address": {
"type": "pci",
"data": {
  "device-id": 4162,
  "vendor-id": 6900
}
  }
},
...
]

Signed-off-by: Tomáš Golembiovský 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---

Changes in v10:
- rebased to current master
- changed `since` tag in schema to 5.2

 qga/commands-posix.c |   9 ++
 qga/commands-win32.c | 212 ++-
 qga/qapi-schema.json |  51 +++
 3 files changed, 271 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1a62a3a70d..f509a1f525 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2761,6 +2761,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
 blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
 #endif
 
+blacklist = g_list_append(blacklist, g_strdup("guest-get-devices"));
+
 return blacklist;
 }
 
@@ -2981,3 +2983,10 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
 return info;
 }
+
+GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
+{
+error_setg(errp, QERR_UNSUPPORTED);
+
+return NULL;
+}
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index aaa71f147b..1302bae9eb 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -21,10 +21,11 @@
 #ifdef CONFIG_QGA_NTDDSCSI
 #include 
 #include 
+#endif
 #include 
 #include 
 #include 
-#endif
+#include 
 #include 
 #include 
 #include 
@@ -39,6 +40,36 @@
 #include "qemu/base64.h"
 #include "commands-common.h"
 
+/*
+ * The following should be in devpkey.h, but it isn't. The key names were
+ * prefixed to avoid (future) name clashes. Once the definitions get into
+ * mingw the following lines can be removed.
+ */
+DEFINE_DEVPROPKEY(qga_DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5,
+0xf1, 0x02, 0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);
+/* DEVPROP_TYPE_STRING */
+DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c,
+0x4efd, 0x80, 0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
+/* DEVPROP_TYPE_STRING_LIST */
+DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d,
+0x4094, 0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);
+/* DEVPROP_TYPE_FILETIME */
+DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d,
+0x4094, 0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
+/* DEVPROP_TYPE_STRING */
+/* The following shoud be in cfgmgr32.h, but it isn't */
+#ifndef CM_Get_DevNode_Property
+CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
+DEVINST  dnDevInst,
+CONST DEVPROPKEY * PropertyKey,
+DEVPROPTYPE  * PropertyType,
+PBYTEPropertyBuffer,
+PULONG   PropertyBufferSize,
+ULONGulFlags
+);
+#define CM_Get_DevNode_Property CM_Get_DevNode_PropertyW
+#endif
+
 #ifndef SHTDN_REASON_FLAG_PLANNED
 #define SHTDN_REASON_FLAG_PLANNED 0x8000
 #endif
@@ -93,6 +124,8 @@ static OpenFlags guest_file_open_modes[] = {
 g_free(suffix); \
 } while (0)
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestDeviceInfo, qapi_free_GuestDeviceInfo)
+
 static OpenFlags *find_open_flag(const char *mode_str)
 {
 int mode;
@@ -2229,3 +2262,180 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
 return info;
 }
+
+/*
+ * Safely get device property. Returned strings are using wide characters.
+ * Caller is responsible for freeing the buffer.
+ */
+static LPBYTE cm_get_property(DEVINST devInst, const DEVPROPKEY *propName,
+PDEVPROPTYPE propType)
+{
+CONFIGRET cr;
+g_autofree LPBYTE buffer = NULL;
+ULONG buffer_len = 0;
+
+/* First query for needed space */
+cr = CM_Get_DevNode_PropertyW(devInst, propName, propType,
+buffer, _len, 0);
+if (cr != CR_SUCCESS && cr != CR_BUFFER_SMALL) {
+
+slog("failed to get property size, error=0x%lx", cr);
+return NULL;
+}
+buffer = g_new0(BYTE, buffer_len + 1);
+cr = CM_Get_DevNode_PropertyW(devInst, propName, propType,
+buffer, _len, 0);
+if (cr != CR_SUCCESS) {
+slog("failed to get device property, error=0x%lx", cr);
+return NULL;
+}
+return g_steal_pointer();
+}
+
+static GStrv ga_get_hardware_ids(DEVINST devInstance)
+{
+GArray *values = NULL;
+DEVPROPTYPE cm_type;
+LPWSTR id;
+g_autofree LPWSTR property = (LPWSTR)cm_get_property(devInstance,
+_DEVPKEY_Device_HardwareIds, _type);
+if (property == NULL) {
+slog("failed to get hardware

[PULL 2/3] qom: Document object_get_canonical_path() returns malloced string

Signed-off-by: Markus Armbruster 
Message-Id: <20200714160202.3121879-5-arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
---
 include/qom/object.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 55d925d2c8..0f3a60617c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1467,8 +1467,9 @@ const char *object_get_canonical_path_component(const 
Object *obj);
 /**
  * object_get_canonical_path:
  *
- * Returns: The canonical path for a object.  This is the path within the
- * composition tree starting from the root.
+ * Returns: The canonical path for a object, newly allocated.  This is
+ * the path within the composition tree starting from the root.  Use
+ * g_free() to free it.
  */
 char *object_get_canonical_path(const Object *obj);
 
-- 
2.26.2

[PULL 0/3] QOM patches for 2020-07-21

The following changes since commit af3d69058e09bede9900f266a618ed11f76f49f3:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200720' 
into staging (2020-07-20 15:58:07 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qom-2020-07-21

for you to fetch changes up to 0dde9fd12fd39762ff68fca80d2f0a735d66e7bd:

  qom: Make info qom-tree sort children more efficiently (2020-07-21 17:39:37 
+0200)


QOM patches for 2020-07-21


Markus Armbruster (3):
  qom: Change object_get_canonical_path_component() not to malloc
  qom: Document object_get_canonical_path() returns malloced string
  qom: Make info qom-tree sort children more efficiently

 include/qom/object.h   |  7 ---
 backends/hostmem.c |  2 +-
 block/throttle-groups.c|  2 +-
 gdbstub.c  |  2 +-
 hw/arm/xlnx-zynqmp.c   |  6 ++
 hw/block/nvme.c|  5 ++---
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c  |  5 ++---
 hw/mem/nvdimm.c|  5 ++---
 hw/mem/pc-dimm.c   |  5 ++---
 hw/misc/ivshmem.c  |  5 ++---
 hw/ppc/spapr_drc.c |  3 +--
 hw/virtio/virtio-crypto.c  |  5 ++---
 hw/virtio/virtio-mem.c |  6 ++
 hw/virtio/virtio-pmem.c|  5 ++---
 iothread.c |  9 -
 net/net.c  |  6 ++
 qom/object.c   |  7 +++
 qom/qom-hmp-cmds.c | 32 +++-
 scsi/pr-manager-helper.c   |  3 +--
 scsi/pr-manager.c  |  2 +-
 softmmu/memory.c   |  2 +-
 hw/ppc/trace-events|  2 +-
 23 files changed, 55 insertions(+), 73 deletions(-)

-- 
2.26.2

Re: [PATCH for-5.1 5/5] qom: Make info qom-tree sort children more efficiently

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
>> sorts children the simple, stupid, quadratic way.  I thought the
>> number of children would be small enough for this not to matter.  I
>> was wrong: there are outliers with several hundred children, e.g ARM
>> machines nuri and smdkc210 each have a node with 513 children.
>
> Big Power systems can have thousands.
>
>> While n^2 sorting isn't noticeable in normal, human usage even for
>> n=513, it can be quite noticeable in certain automated tests.  In
>> particular, the sort made device-introspect-test even slower.  Commit
>> 3e7b80f84d "tests: improve performance of device-introspect-test" just
>> fixed that by cutting back its excessive use of "info qom-tree".
>> Sorting more efficiently makes sense regardless, so do it.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  qom/qom-hmp-cmds.c | 25 +
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
>> index 4032c96089..8861a109d5 100644
>> --- a/qom/qom-hmp-cmds.c
>> +++ b/qom/qom-hmp-cmds.c
>> @@ -94,25 +94,23 @@ typedef struct QOMCompositionState {
>>  
>>  static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>>  
>> -static int qom_composition_compare(const void *a, const void *b, void 
>> *ignore)
>> +static int qom_composition_compare(const void *a, const void *b)
>>  {
>> -return g_strcmp0(object_get_canonical_path_component(a),
>> - object_get_canonical_path_component(b));
>> +return g_strcmp0(object_get_canonical_path_component(*(Object **)a),
>> + object_get_canonical_path_component(*(Object **)b));
>>  }
>>  
>>  static int insert_qom_composition_child(Object *obj, void *opaque)
>>  {
>> -GQueue *children = opaque;
>> -
>> -g_queue_insert_sorted(children, obj, qom_composition_compare, NULL);
>> +g_array_append_val(opaque, obj);
>>  return 0;
>>  }
>>  
>>  static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>>  {
>> +GArray *children = g_array_new(false, false, sizeof(Object *));
>>  const char *name;
>> -GQueue children;
>> -Object *child;
>> +int i;
>>  
>>  if (obj == object_get_root()) {
>>  name = "";
>> @@ -122,11 +120,14 @@ static void print_qom_composition(Monitor *mon, Object 
>> *obj, int indent)
>>  monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
>> object_get_typename(obj));
>>  
>> -g_queue_init();
>> -object_child_foreach(obj, insert_qom_composition_child, );
>> -while ((child = g_queue_pop_head())) {
>> -print_qom_composition(mon, child, indent + 2);
>> +object_child_foreach(obj, insert_qom_composition_child, children);
>> +g_array_sort(children, qom_composition_compare);
>> +
>> +for (i = 0; i < children->len; i++) {
>> +print_qom_composition(mon, g_array_index(children, Object *, i),
>> +  indent + 2);
>>  }
>> +g_array_free(children, TRUE);
>
> So I think that's OK, so :
>
> Reviewed-by: Dr. David Alan Gilbert 
>
> Can you just convince me that 'TRUE' in the array_free?

g_array_free(children, TRUE) frees both children and children->data.  It
returns null.  This is what we want here.

g_array_free(children, FALSE) frees only children, and returns
children->data.  Occasionally useful.

https://developer.gnome.org/glib/2.62/glib-Arrays.html#g-array-free

I definitely would have made this two separate functions.

Thanks!

Re: [PATCH for-5.1 5/5] qom: Make info qom-tree sort children more efficiently

2020-07-21 Thread Dr. David Alan Gilbert

* Markus Armbruster (arm...@redhat.com) wrote:
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> sorts children the simple, stupid, quadratic way.  I thought the
> number of children would be small enough for this not to matter.  I
> was wrong: there are outliers with several hundred children, e.g ARM
> machines nuri and smdkc210 each have a node with 513 children.

Big Power systems can have thousands.

> While n^2 sorting isn't noticeable in normal, human usage even for
> n=513, it can be quite noticeable in certain automated tests.  In
> particular, the sort made device-introspect-test even slower.  Commit
> 3e7b80f84d "tests: improve performance of device-introspect-test" just
> fixed that by cutting back its excessive use of "info qom-tree".
> Sorting more efficiently makes sense regardless, so do it.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qom/qom-hmp-cmds.c | 25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 4032c96089..8861a109d5 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -94,25 +94,23 @@ typedef struct QOMCompositionState {
>  
>  static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>  
> -static int qom_composition_compare(const void *a, const void *b, void 
> *ignore)
> +static int qom_composition_compare(const void *a, const void *b)
>  {
> -return g_strcmp0(object_get_canonical_path_component(a),
> - object_get_canonical_path_component(b));
> +return g_strcmp0(object_get_canonical_path_component(*(Object **)a),
> + object_get_canonical_path_component(*(Object **)b));
>  }
>  
>  static int insert_qom_composition_child(Object *obj, void *opaque)
>  {
> -GQueue *children = opaque;
> -
> -g_queue_insert_sorted(children, obj, qom_composition_compare, NULL);
> +g_array_append_val(opaque, obj);
>  return 0;
>  }
>  
>  static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>  {
> +GArray *children = g_array_new(false, false, sizeof(Object *));
>  const char *name;
> -GQueue children;
> -Object *child;
> +int i;
>  
>  if (obj == object_get_root()) {
>  name = "";
> @@ -122,11 +120,14 @@ static void print_qom_composition(Monitor *mon, Object 
> *obj, int indent)
>  monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
> object_get_typename(obj));
>  
> -g_queue_init();
> -object_child_foreach(obj, insert_qom_composition_child, );
> -while ((child = g_queue_pop_head())) {
> -print_qom_composition(mon, child, indent + 2);
> +object_child_foreach(obj, insert_qom_composition_child, children);
> +g_array_sort(children, qom_composition_compare);
> +
> +for (i = 0; i < children->len; i++) {
> +print_qom_composition(mon, g_array_index(children, Object *, i),
> +  indent + 2);
>  }
> +g_array_free(children, TRUE);

So I think that's OK, so :

Reviewed-by: Dr. David Alan Gilbert 

Can you just convince me that 'TRUE' in the array_free?

Dave

>  }
>  
>  void hmp_info_qom_tree(Monitor *mon, const QDict *dict)
> -- 
> 2.26.2
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 1/2] target/riscv: Quiet Coverity complains about vamo*

On Tue, 21 Jul 2020 at 16:19, Alistair Francis  wrote:
>
> On Tue, Jul 21, 2020 at 6:38 AM LIU Zhiwei  wrote:
> >
> > Signed-off-by: LIU Zhiwei 
>
> Reviewed-by: Alistair Francis 

Interestingly Coverity's latest scan has decided all these
issues are 'fixed', even without the assert. I guess that the
online version of Coverity has had an improvement to its
checking and so is now able to figure out that it's not going
to overrun the array? Still I think the assert is worth having.

thanks
-- PMM

Re: [PATCH v9] qga: add command guest-get-devices for reporting VirtIO devices

2020-07-21 Thread Tomáš Golembiovský

On Tue, 21 Jul 2020 12:43:35 +0100
Daniel P. Berrangé  wrote:

> On Tue, Jul 21, 2020 at 03:31:52PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Jul 21, 2020 at 12:03 PM Tomáš Golembiovský 
> > wrote:
> >   
> > > Ping. Can we get this merged finally?
> > >  
> > 
> > We missed the feature deadline by a week:
> > https://wiki.qemu.org/Planning/5.1  
> 
> Note, if a patch series from a contributor is ready, the subsystem
> maintainer should queue it in their subsystem tree, so it is ready
> for a pull request when the main tree opens up again. ie the subsys
> maintainer is the one responsible for dealing with feature freeze
> coordination, not the individual contriubutors.
> 
> In this case the "Since 5.0" tags need updating to 5.2 now. The
> maintainer could handle this, but since this series is over 6 months
> old now, it is probably worth rebasing & reposting.

Good point. I will rebase and post again.

Tomas

> 
> > > Thanks,
> > >
> > > Tomas
> > >
> > > On Thu,  9 Jan 2020 13:39:36 +0100
> > > Tomáš Golembiovský  wrote:
> > >  
> > > > Add command for reporting devices on Windows guest. The intent is not so
> > > > much to report the devices but more importantly the driver (and its
> > > > version) that is assigned to the device. This gives caller the
> > > > information whether VirtIO drivers are installed and/or whether
> > > > inadequate driver is used on a device (e.g. QXL device with base VGA
> > > > driver).
> > > >
> > > > Example:
> > > > [
> > > > {
> > > >   "driver-date": "2019-08-12",
> > > >   "driver-name": "Red Hat VirtIO SCSI controller",
> > > >   "driver-version": "100.80.104.17300",
> > > >   "address": {
> > > > "type": "pci",
> > > > "data": {
> > > >   "device-id": 4162,
> > > >   "vendor-id": 6900
> > > > }
> > > >   }
> > > > },
> > > > ...
> > > > ]
> > > >
> > > > Signed-off-by: Tomáš Golembiovský 
> > > > ---
> > > >
> > > > Changes in v9: fixed compilation errors
> > > >
> > > >  qga/commands-posix.c |   9 ++
> > > >  qga/commands-win32.c | 212 ++-
> > > >  qga/qapi-schema.json |  51 +++
> > > >  3 files changed, 271 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > > index 93474ff770..bffee8ce48 100644
> > > > --- a/qga/commands-posix.c
> > > > +++ b/qga/commands-posix.c
> > > > @@ -2771,6 +2771,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
> > > >  blacklist = g_list_append(blacklist, g_strdup("guest-fstrim"));
> > > >  #endif
> > > >
> > > > +blacklist = g_list_append(blacklist, 
> > > > g_strdup("guest-get-devices"));
> > > > +
> > > >  return blacklist;
> > > >  }
> > > >
> > > > @@ -2991,3 +2993,10 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > > >
> > > >  return info;
> > > >  }
> > > > +
> > > > +GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
> > > > +{
> > > > +error_setg(errp, QERR_UNSUPPORTED);
> > > > +
> > > > +return NULL;
> > > > +}
> > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > > > index 2461fd19bf..b18d89d7ad 100644
> > > > --- a/qga/commands-win32.c
> > > > +++ b/qga/commands-win32.c
> > > > @@ -21,10 +21,11 @@
> > > >  #ifdef CONFIG_QGA_NTDDSCSI
> > > >  #include 
> > > >  #include 
> > > > +#endif
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > -#endif
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -38,6 +39,36 @@
> > > >  #include "qemu/host-utils.h"
> > > >  #include "qemu/base64.h"
> > > >
> > > > +/*
> > > > + * The following should be in devpkey.h, but it isn't. The key names  
> > > were  
> > > > + * prefixed to avoid (future) name clashes. Once the definitions get  
> > > into  
> > > > + * mingw the following lines can be removed.
> > > > + */
> > > > +DEFINE_DEVPROPKEY(qga_DEVPKEY_NAME, 0xb725f130, 0x47ef, 0x101a, 0xa5,
> > > > +0xf1, 0x02, 0x60, 0x8c, 0x9e, 0xeb, 0xac, 10);
> > > > +/* DEVPROP_TYPE_STRING */
> > > > +DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_HardwareIds, 0xa45c254e, 0xdf1c,
> > > > +0x4efd, 0x80, 0x20, 0x67, 0xd1, 0x46, 0xa8, 0x50, 0xe0, 3);
> > > > +/* DEVPROP_TYPE_STRING_LIST */
> > > > +DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverDate, 0xa8b865dd, 0x2e3d,
> > > > +0x4094, 0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 2);
> > > > +/* DEVPROP_TYPE_FILETIME */
> > > > +DEFINE_DEVPROPKEY(qga_DEVPKEY_Device_DriverVersion, 0xa8b865dd, 0x2e3d,
> > > > +0x4094, 0xad, 0x97, 0xe5, 0x93, 0xa7, 0xc, 0x75, 0xd6, 3);
> > > > +/* DEVPROP_TYPE_STRING */
> > > > +/* The following shoud be in cfgmgr32.h, but it isn't */
> > > > +#ifndef CM_Get_DevNode_Property
> > > > +CMAPI CONFIGRET WINAPI CM_Get_DevNode_PropertyW(
> > > > +DEVINST  dnDevInst,
> > > > +CONST DEVPROPKEY * PropertyKey,
> > > > +DEVPROPTYPE  * PropertyType,
> > > > +PBYTEPropertyBuffer,

Re: [RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

On 7/21/20 2:24 PM, Laurent Vivier wrote:
> Le 21/07/2020 à 13:38, Laurent Vivier a écrit :
>> Le 21/07/2020 à 10:57, Philippe Mathieu-Daudé a écrit :
>>> The time test sometimes fails on Travis-CI [*]:
>>>
>>> TESTlinux-test on aarch64
>>>   tests/tcg/multiarch/linux-test.c:237: nanosleep
>>>   make[2]: *** [run-linux-test] Error 1
>>>   make: *** [run-tcg-tests-aarch64-linux-user] Error 2
>>>
>>> As this seems due to a container limitation on Travis-CI,
>>> simply skip the test there.
>>>
>>> [*] https://travis-ci.org/github/qemu/qemu/jobs/710005078#L3706
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>> RFC because per Laurent Vivier we are not using the correct libc
>>> while cross-linking the test (maybe change in the container
>>> packages?)
>>> ---
>>>  tests/tcg/multiarch/linux-test.c | 7 ++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/tcg/multiarch/linux-test.c 
>>> b/tests/tcg/multiarch/linux-test.c
>>> index 8a7c15cd31..c7dfdec9ec 100644
>>> --- a/tests/tcg/multiarch/linux-test.c
>>> +++ b/tests/tcg/multiarch/linux-test.c
>>> @@ -233,8 +233,13 @@ static void test_time(void)
>>>  ts.tv_sec = 0;
>>>  ts.tv_nsec = 20 * 100;
>>>  chk_error(nanosleep(, ));
>>> -if (rem.tv_sec != 1)
>>> +if (rem.tv_sec != 1) {
>>> +if (getenv("TRAVIS_ARCH")) {
>>> +printf("nanosleep missing? skipping 'time' test\n");
>>> +return;
>>> +}
>>>  error("nanosleep");
>>> +}
>>>  chk_error(gettimeofday(, NULL));
>>>  ti = tv2.tv_sec - tv.tv_sec;
>>>  if (ti >= 2)
>>>
>>
>> Well, in the end I think the problem is in linux-user:
>>
>> We copy the "rem" structure even if there is no error, so "1" is
>> overwritten.
>>
>> We don't have the problem on all architectures because some use
>> nanosleep() syscall (that is correct) others use clock_nanosleep()
>> syscall that is not correct.
>>
>> This should fix the problem:
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 1211e759c26c..130005716ece 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -11831,7 +11831,7 @@ static abi_long do_syscall1(void *cpu_env, int
>> num, abi_long arg1,
>>  target_to_host_timespec(, arg3);
>>  ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>>   , arg4 ?  : NULL));
>> -if (arg4)
>> +if (is_error(ret) && arg4)
>>  host_to_target_timespec(arg4, );
>>
>>  #if defined(TARGET_PPC)
> 
> According to clock_nanosleep(2) it should be in fact:
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c26c..63e7cd8947e5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11831,8 +11831,9 @@ static abi_long do_syscall1(void *cpu_env, int
> num, abi_long arg1,
>  target_to_host_timespec(, arg3);
>  ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>   , arg4 ?  : NULL));
> -if (arg4)
> +if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
>  host_to_target_timespec(arg4, );
> +}

I tested this hunk and couldn't reproduce it on Travis-CI. Since it is
intermittent, that doesn't mean much, but still if you send a proper
path you can add "Tested-by: Philippe Mathieu-Daudé ".

> 
>  #if defined(TARGET_PPC)
>  /* clock_nanosleep is odd in that it returns positive errno values.
> 
>

[PULL 1/1] qdev: Fix device_add DRIVER,help to print to monitor