Re: [PATCH 02/13] qcrypto-luks: implement encryption key management

[Markus Armbruster]

Reviewing just the QAPI schema.

Maxim Levitsky  writes:

> Next few patches will expose that functionality
> to the user.
>
> Signed-off-by: Maxim Levitsky 
> ---
>  crypto/block-luks.c | 374 +++-
>  qapi/crypto.json|  50 +-
>  2 files changed, 421 insertions(+), 3 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 4861db810c..349e95fed3 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -32,6 +32,7 @@
>  #include "qemu/uuid.h"
>  
>  #include "qemu/coroutine.h"
> +#include "qemu/bitmap.h"
>  
>  /*
>   * Reference for the LUKS format implemented here is
> @@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot 
> QCryptoBlockLUKSKeySlot;
>  
>  #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
>  
> +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000
> +#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
> +
>  static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = {
>  'L', 'U', 'K', 'S', 0xBA, 0xBE
>  };
> @@ -219,6 +223,9 @@ struct QCryptoBlockLUKS {
>  
>  /* Hash algorithm used in pbkdf2 function */
>  QCryptoHashAlgorithm hash_alg;
> +
> +/* Name of the secret that was used to open the image */
> +char *secret;
>  };
>  
>  
> @@ -1069,6 +1076,112 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
>  return -1;
>  }
>  
> +/*
> + * Returns true if a slot i is marked as active
> + * (contains encrypted copy of the master key)
> + */
> +static bool
> +qcrypto_block_luks_slot_active(const QCryptoBlockLUKS *luks,
> +   unsigned int slot_idx)
> +{
> +uint32_t val = luks->header.key_slots[slot_idx].active;
> +return val ==  QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> +}
> +
> +/*
> + * Returns the number of slots that are marked as active
> + * (slots that contain encrypted copy of the master key)
> + */
> +static unsigned int
> +qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks)
> +{
> +size_t i = 0;
> +unsigned int ret = 0;
> +
> +for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +if (qcrypto_block_luks_slot_active(luks, i)) {
> +ret++;
> +}
> +}
> +return ret;
> +}
> +
> +/*
> + * Finds first key slot which is not active
> + * Returns the key slot index, or -1 if it doesn't exist
> + */
> +static int
> +qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks)
> +{
> +size_t i;
> +
> +for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +if (!qcrypto_block_luks_slot_active(luks, i)) {
> +return i;
> +}
> +}
> +return -1;
> +
> +}
> +
> +/*
> + * Erases an keyslot given its index
> + * Returns:
> + *0 if the keyslot was erased successfully
> + *   -1 if a error occurred while erasing the keyslot
> + *
> + */
> +static int
> +qcrypto_block_luks_erase_key(QCryptoBlock *block,
> + unsigned int slot_idx,
> + QCryptoBlockWriteFunc writefunc,
> + void *opaque,
> + Error **errp)
> +{
> +QCryptoBlockLUKS *luks = block->opaque;
> +QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
> +g_autofree uint8_t *garbagesplitkey = NULL;
> +size_t splitkeylen = luks->header.master_key_len * slot->stripes;
> +size_t i;
> +
> +assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +assert(splitkeylen > 0);
> +
> +garbagesplitkey = g_new0(uint8_t, splitkeylen);
> +
> +/* Reset the key slot header */
> +memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
> +slot->iterations = 0;
> +slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> +
> +qcrypto_block_luks_store_header(block,  writefunc, opaque, errp);
> +
> +/*
> + * Now try to erase the key material, even if the header
> + * update failed
> + */
> +for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
> +if (qcrypto_random_bytes(garbagesplitkey, splitkeylen, errp) < 0) {
> +/*
> + * If we failed to get the random data, still write
> + * at least zeros to the key slot at least once
> + */
> +if (i > 0) {
> +return -1;
> +}
> +}
> +
> +if (writefunc(block,
> +  slot->key_offset_sector * 
> QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
> +  garbagesplitkey,
> +  splitkeylen,
> +  opaque,
> +  errp) != splitkeylen) {
> +return -1;
> +}
> +}
> +return 0;
> +}
>  
>  static int
>  qcrypto_block_luks_open(QCryptoBlock *block,
> @@ -1099,6 +1212,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>  
>  luks = g_new0(QCryptoBlockLUKS, 1);
>  block->opaque = luks;
> +luks->secret = g_strdup(options->u.luks.key_secret);
>  
>  if 

Re: [PATCH] spapr: Migrate CAS reboot flag

[Greg Kurz]

On Tue, 21 Jan 2020 14:41:26 +1100
David Gibson  wrote:

> On Wed, Jan 15, 2020 at 07:10:47PM +0100, Cédric Le Goater wrote:
> > On 1/15/20 6:48 PM, Greg Kurz wrote:
> > > Migration can potentially race with CAS reboot. If the migration thread
> > > completes migration after CAS has set spapr->cas_reboot but before the
> > > mainloop could pick up the reset request and reset the machine, the
> > > guest is migrated unrebooted and the destination doesn't reboot it
> > > either because it isn't aware a CAS reboot was needed (eg, because a
> > > device was added before CAS). This likely result in a broken or hung
> > > guest.
> > > 
> > > Even if it is small, the window between CAS and CAS reboot is enough to
> > > re-qualify spapr->cas_reboot as state that we should migrate. Add a new
> > > subsection for that and always send it when a CAS reboot is pending.
> > > This may cause migration to older QEMUs to fail but it is still better
> > > than end up with a broken guest.
> > > 
> > > The destination cannot honour the CAS reboot request from a post load
> > > handler because this must be done after the guest is fully restored.
> > > It is thus done from a VM change state handler.
> > > 
> > > Reported-by: Lukáš Doktor 
> > > Signed-off-by: Greg Kurz 
> > 
> > Cédric Le Goater 
> > 
> > Nice work ! That was quite complex to catch !
> 
> It is a very nice analysis.  However, I'm disinclined to merge this
> for the time being.
> 
> My preferred approach would be to just eliminate CAS reboots
> altogether, since that has other benefits.  I'm feeling like this
> isn't super-urgent, since CAS reboots are extremely rare in practice,
> now that we've eliminated the one for the irq switchover.
> 

Yeah. The only _true_ need for CAS rebooting now seems to be hotplug
before CAS, which is likely not something frequent.

> However, if it's not looking like we'll be ready to do that as the
> qemu-5.0 release approaches, then I'll be more than willing to
> reconsider this.
> 

I hope we can drop CAS reboot in time.


pgp3KJKQ24sp6.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/2] qdev: Introduce qdev_get_bus_device

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> Hi Julia,
>
> Cc'ing Markus for the qdev/qbus analysis.
>
> On 1/15/20 11:40 PM, Julia Suvorova wrote:
>> For bus devices, it is useful to be able to handle the parent device.
>>
>> Signed-off-by: Julia Suvorova 
>> ---
>>   hw/core/qdev.c  |  5 +
>>   hw/pci-bridge/pci_expander_bridge.c |  4 +++-
>>   hw/scsi/scsi-bus.c  |  4 +++-
>>   hw/usb/bus.c|  4 +++-
>>   hw/usb/dev-smartcard-reader.c   | 32 +
>>   hw/virtio/virtio-pci.c  | 16 +--
>>   include/hw/qdev-core.h  |  2 ++
>
> Please consider using the scripts/git.orderfile config.
>
>>   7 files changed, 54 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 9f1753f5cf..ad8226e240 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -114,6 +114,11 @@ void qdev_set_parent_bus(DeviceState *dev, BusState 
>> *bus)
>>   }
>>   }
>>   +DeviceState *qdev_get_bus_device(const DeviceState *dev)
>
> We have qdev_get_bus_hotplug_handler(), this follow the naming, OK.
>
>> +{
>> +return dev->parent_bus ? dev->parent_bus->parent : NULL;
>> +}
>> +
>>   /* Create a new device.  This only initializes the device state
>>  structure and allows properties to be set.  The device still needs
>>  to be realized.  See qdev-core.h.  */
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c 
>> b/hw/pci-bridge/pci_expander_bridge.c
>> index 0592818447..63a6c07406 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -125,9 +125,11 @@ static char *pxb_host_ofw_unit_address(const 
>> SysBusDevice *dev)
>>   assert(position >= 0);
>> pxb_dev_base = DEVICE(pxb_dev);
>> -main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
>> +main_host = PCI_HOST_BRIDGE(qdev_get_bus_device(pxb_dev_base));
>>   main_host_sbd = SYS_BUS_DEVICE(main_host);
>>   +g_assert(main_host);
>
> I found myself stuck reviewing this patch for 25min, I'm not sure
> what's bugging me yet, so I'll take notes a-la-Markus-style.
>
> We have the qdev API, with DeviceState.
>
>
> We have the qbus API, with BusState.
>
> A BusState is not a DeviceState but a raw Object.

It's a completely separate kind of Object.

> It keeps a pointer to the a DeviceState parent, a HotplugHandler, and
> a list of BusChild.
>
>
> BusChild are neither DeviceState nor Object, but keep a pointer the a
> DeviceState.

It's a thin wrapper around DeviceState to support collecting the
DeviceState into a list.

> TYPE_HOTPLUG_HANDLER is an interface. It can be implemented by any
> object, but its API seems expects a DeviceState as argument.

What do you mean by "interface expects an argument"?

The interface methods all take a HotplugHandler * and a DeviceState *.
The latter is the device being plugged / unplugged, the former is its
hotplug handler.  In the generic case, @dev's hotplug handler is
qdev_get_hotplug_handler(dev).

> Looking at examples implementing TYPE_HOTPLUG_HANDLER:
>
> - TYPE_USB_BUS. It inherits TYPE_BUS. Handlers will be called with
> USBDevice as argument (TYPE_USB_DEVICE -> TYPE_DEVICE).
>
> - TYPE_PCI_BRIDGE_DEV. Inherits TYPE_PCI_BRIDGE -> TYPE_PCI_DEVICE -> 
> TYPE_DEVICE. Handlers expects PCIDevice (TYPE_PCI_DEVICE).
>
> - TYPE_PC_MACHINE. It inherits TYPE_X86_MACHINE -> TYPE_MACHINE -> 
> TYPE_OBJECT. Not a TYPE_BUS. Handlers for TYPE_PC_DIMM, TYPE_CPU and
> TYPE_VIRTIO_PMEM_PCI. Complex... TYPE_PC_DIMM/TYPE_CPU are
> TYPE_DEVICE.
> For TYPE_VIRTIO_PMEM_PCI we have VirtIOPMEMPCI -> VirtIOPCIProxy -> 
> PCIDevice.
>
> - USB_CCID_DEV. Inherits TYPE_USB_DEVICE -> TYPE_DEVICE. Only one
> 'unplug' handler which likely expects USBCCIDState.
>
> - TYPE_SCSI_BUS. Inherits TYPE_BUS. Also a single 'unplug' handler
> expecting SCSIDevice.
>
> - TYPE_VIRTIO_SCSI. Inherits TYPE_VIRTIO_SCSI_COMMON -> 
> TYPE_VIRTIO_DEVICE -> TYPE_DEVICE. Handlers expect VirtIOSCSI.
>
>
> No simple pattern so far.
>
>
> Looking back at qbus. qbus_initfn() enforces a TYPE_HOTPLUG_HANDLER
> property on BusState (which is not a DeviceState). So IIUC TYPE_BUS
> also implements TYPE_HOTPLUG_HANDLER.

I think this merely creates a reference to the concrete bus's hotplug
handler.  TYPE_BUS is abstract, and doesn't implement any interfaces
(its .interfaces is empty).

Anything else you'd like me to check for you?

[...]

Re: [PATCH qemu v5] spapr: Kill SLOF

[Alexey Kardashevskiy]

On 21/01/2020 16:11, David Gibson wrote:
> On Fri, Jan 10, 2020 at 01:09:25PM +1100, Alexey Kardashevskiy wrote:
>> The Petitboot bootloader is way more advanced than SLOF is ever going to
>> be as Petitboot comes with the full-featured Linux kernel with all
>> the drivers, and initramdisk with quite user friendly interface.
>> The problem with ditching SLOF is that an unmodified pseries kernel can
>> either start via:
>> 1. kexec, this requires presence of RTAS and skips
>> ibm,client-architecture-support entirely;
>> 2. normal boot, this heavily relies on the OF1275 client interface to
>> fetch the device tree and do early setup (claim memory).
>>
>> This adds a new bios-less mode to the pseries machine: "bios=on|off".
>> When enabled, QEMU does not load SLOF and jumps to the kernel from
>> "-kernel".
> 
> I don't love the name "bios" for this flag, since BIOS tends to refer
> to old-school x86 firmware.  Given the various plans we're considering
> the future, I'd suggest "firmware=slof" for the current in-guest SLOF
> mode, and say "firmware=vof" (Virtual Open Firmware) for the new
> model.  We can consider firmware=petitboot or firmware=none (for
> direct kexec-style boot into -kernel) or whatever in the future

Ok. We could also enforce default loading addresses for SLOF/kernel/grub
and drop "kernel-addr", although it is going to be confusing if it
changes in not so obvious way...

In fact, I will ideally need 3 flags:
-bios: on|off to stop loading SLOF;
-kernel-addr: 0x0 for slof/kernel; 0x2 for grub;
-kernel-translate-hack: on|off - as grub is linked to run from 0x2
and it only works when placed there, the hack breaks it.

Or we can pass grub via -bios and not via -kernel but strictly speaking
there is still a firmware - that new 20 bytes blob so it would not be
accurate.

We can put this all into a single
-firmware slof|vof|grub|linux. Not sure.


>> The client interface is implemented exactly as RTAS - a 20 bytes blob,
>> right next after the RTAS blob. The entry point is passed to the kernel
>> via GPR5.
>>
>> This implements a handful of client interface methods just to get going.
>> In particular, this implements the device tree fetching,
>> ibm,client-architecture-support and instantiate-rtas.
>>
>> This implements changing FDT properties for RTAS (for vmlinux and zImage)
>> and initramdisk location (for zImage). To make this work, this skips
>> fdt_pack() when bios=off as not packing the blob leaves some room for
>> appending.
>>
>> This assigns "phandles" to device tree nodes as there is no more SLOF
>> and OF nodes addresses of which served as phandle values.
>> This keeps predefined nodes (such as XICS/NVLINK/...) unchanged.
>> phandles are regenerated at every FDT rebuild.
>>
>> When bios=off, this adds "/chosen" every time QEMU builds a tree.
>>
>> This implements "claim" which the client (Linux) uses for memory
>> allocation; this is also  used by QEMU for claiming kernel/initrd images,
>> client interface entry point, RTAS and the initial stack.
>>
>> While at this, add a "kernel-addr" machine parameter to allow moving
>> the kernel in memory. This is useful for debugging if the kernel is
>> loaded at @0, although not necessary.
>>
>> This adds basic instances support which are managed by a hashmap
>> ihandle->[phandle, DeviceState, Chardev].
>>
>> Note that a 64bit PCI fix is required for Linux:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df5be5be8735e
>>
>> The test command line:
>>
>> qemu-system-ppc64 \
>> -nodefaults \
>> -chardev stdio,id=STDIO0,signal=off,mux=on \
>> -device spapr-vty,id=svty0,reg=0x71000110,chardev=STDIO0 \
>> -mon id=MON0,chardev=STDIO0,mode=readline \
>> -nographic \
>> -vga none \
>> -kernel pbuild/kernel-le-guest/arch/powerpc/boot/zImage.pseries \
>> -machine pseries,bios=off,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken \
>> -m 4G \
>> -enable-kvm \
>> -initrd pb/rootfs.cpio.xz \
>> -device nec-usb-xhci,id=nec-usb-xhci0 \
>> -netdev tap,id=TAP0,helper=/home/aik/qemu-bridge-helper --br=br0 \
>> -device virtio-net-pci,id=vnet0,netdev=TAP0 img/f30le.qcow2 \
>> -snapshot \
>> -smp 8,threads=8 \
>> -trace events=qemu_trace_events \
>> -d guest_errors \
>> -chardev socket,id=SOCKET0,server,nowait,path=qemu.mon.ssh54088 \
>> -mon chardev=SOCKET0,mode=control
>>
>> Signed-off-by: Alexey Kardashevskiy 
> 
> It'd be nice to split this patch up a bit, though I'll admit it's not
> very obvious where to do so.


v6 is a patchset.

>> ---
>> Changes:
>> v5:
>> * made instances keep device and chardev pointers
>> * removed VIO dependencies
>> * print error if RTAS memory is not claimed as it should have been
>> * pack FDT as "quiesce"
>>
>> v4:
>> * fixed open
>> * validate ihandles in "call-method"
>>
>> v3:
>> * fixed phandles allocation
>> * s/__be32/uint32_t/ as we do not normally have __be32 type in qemu
>> * fixed size of /chosen/stdout
>> * bunch of renames
>> * do not create rtas properties at all, let the client 

Re: [PATCH 0/2] aspeed/scu: Implement chip id register

[Cédric Le Goater]

On 1/21/20 2:33 AM, Joel Stanley wrote:
> This implements the chip id register in the SCU for the ast2500 and
> ast2600. The first patch is a cleanup to separate out ast2400 and
> ast2500 functionality.

These patches apply cleanly on top of :

[v3,0/5] aspeed: extensions and fixes 
http://patchwork.ozlabs.org/cover/1222667/

Thanks,

C.

> 
> Joel Stanley (2):
>   aspeed/scu: Create separate write callbacks
>   aspeed/scu: Implement chip ID register
> 
>  hw/misc/aspeed_scu.c | 93 +---
>  1 file changed, 70 insertions(+), 23 deletions(-)
> 

Re: [PATCH 1/2] aspeed/scu: Create separate write callbacks

[Cédric Le Goater]

On 1/21/20 2:33 AM, Joel Stanley wrote:
> This splits the common write callback into separate ast2400 and ast2500
> implementations. This makes it clearer when implementing differing
> behaviour.
> 
> Signed-off-by: Joel Stanley 

Reviewed-by: Cédric Le Goater 

> ---
>  hw/misc/aspeed_scu.c | 80 +++-
>  1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index f62fa25e3474..7108cad8c6a7 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -232,8 +232,47 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr 
> offset, unsigned size)
>  return s->regs[reg];
>  }
>  
> -static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
> - unsigned size)
> +static void aspeed_ast2400_scu_write(void *opaque, hwaddr offset,
> + uint64_t data, unsigned size)
> +{
> +AspeedSCUState *s = ASPEED_SCU(opaque);
> +int reg = TO_REG(offset);
> +
> +if (reg >= ASPEED_SCU_NR_REGS) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx 
> "\n",
> +  __func__, offset);
> +return;
> +}
> +
> +if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
> +!s->regs[PROT_KEY]) {
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);
> +}
> +
> +trace_aspeed_scu_write(offset, size, data);
> +
> +switch (reg) {
> +case PROT_KEY:
> +s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
> +return;
> +case SILICON_REV:
> +case FREQ_CNTR_EVAL:
> +case VGA_SCRATCH1 ... VGA_SCRATCH8:
> +case RNG_DATA:
> +case FREE_CNTR4:
> +case FREE_CNTR4_EXT:
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
> +  __func__, offset);
> +return;
> +}
> +
> +s->regs[reg] = data;
> +}
> +
> +static void aspeed_ast2500_scu_write(void *opaque, hwaddr offset,
> + uint64_t data, unsigned size)
>  {
>  AspeedSCUState *s = ASPEED_SCU(opaque);
>  int reg = TO_REG(offset);
> @@ -257,25 +296,11 @@ static void aspeed_scu_write(void *opaque, hwaddr 
> offset, uint64_t data,
>  case PROT_KEY:
>  s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
>  return;
> -case CLK_SEL:
> -s->regs[reg] = data;
> -break;
>  case HW_STRAP1:
> -if (ASPEED_IS_AST2500(s->regs[SILICON_REV])) {
> -s->regs[HW_STRAP1] |= data;
> -return;
> -}
> -/* Jump to assignment below */
> -break;
> +s->regs[HW_STRAP1] |= data;
> +return;
>  case SILICON_REV:
> -if (ASPEED_IS_AST2500(s->regs[SILICON_REV])) {
> -s->regs[HW_STRAP1] &= ~data;
> -} else {
> -qemu_log_mask(LOG_GUEST_ERROR,
> -  "%s: Write to read-only offset 0x%" HWADDR_PRIx 
> "\n",
> -  __func__, offset);
> -}
> -/* Avoid assignment below, we've handled everything */
> +s->regs[HW_STRAP1] &= ~data;
>  return;
>  case FREQ_CNTR_EVAL:
>  case VGA_SCRATCH1 ... VGA_SCRATCH8:
> @@ -291,9 +316,18 @@ static void aspeed_scu_write(void *opaque, hwaddr 
> offset, uint64_t data,
>  s->regs[reg] = data;
>  }
>  
> -static const MemoryRegionOps aspeed_scu_ops = {
> +static const MemoryRegionOps aspeed_ast2400_scu_ops = {
> +.read = aspeed_scu_read,
> +.write = aspeed_ast2400_scu_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +.valid.min_access_size = 4,
> +.valid.max_access_size = 4,
> +.valid.unaligned = false,
> +};
> +
> +static const MemoryRegionOps aspeed_ast2500_scu_ops = {
>  .read = aspeed_scu_read,
> -.write = aspeed_scu_write,
> +.write = aspeed_ast2500_scu_write,
>  .endianness = DEVICE_LITTLE_ENDIAN,
>  .valid.min_access_size = 4,
>  .valid.max_access_size = 4,
> @@ -469,7 +503,7 @@ static void aspeed_2400_scu_class_init(ObjectClass 
> *klass, void *data)
>  asc->calc_hpll = aspeed_2400_scu_calc_hpll;
>  asc->apb_divider = 2;
>  asc->nr_regs = ASPEED_SCU_NR_REGS;
> -asc->ops = _scu_ops;
> +asc->ops = _ast2400_scu_ops;
>  }
>  
>  static const TypeInfo aspeed_2400_scu_info = {
> @@ -489,7 +523,7 @@ static void aspeed_2500_scu_class_init(ObjectClass 
> *klass, void *data)
>  asc->calc_hpll = aspeed_2500_scu_calc_hpll;
>  asc->apb_divider = 4;
>  asc->nr_regs = ASPEED_SCU_NR_REGS;
> -asc->ops = _scu_ops;
> +asc->ops = _ast2500_scu_ops;
>  }
>  
>  static const TypeInfo aspeed_2500_scu_info = {
> 

Re: [PATCH 2/2] aspeed/scu: Implement chip ID register

[Cédric Le Goater]

On 1/21/20 2:33 AM, Joel Stanley wrote:
> This returns a fixed but non-zero value for the chip id.
> 
> Signed-off-by: Joel Stanley 

Reviewed-by: Cédric Le Goater 

> ---
>  hw/misc/aspeed_scu.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 7108cad8c6a7..19d1780a40da 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -77,6 +77,8 @@
>  #define CPU2_BASE_SEG4   TO_REG(0x110)
>  #define CPU2_BASE_SEG5   TO_REG(0x114)
>  #define CPU2_CACHE_CTRL  TO_REG(0x118)
> +#define CHIP_ID0 TO_REG(0x150)
> +#define CHIP_ID1 TO_REG(0x154)
>  #define UART_HPLL_CLKTO_REG(0x160)
>  #define PCIE_CTRLTO_REG(0x180)
>  #define BMC_MMIO_CTRLTO_REG(0x184)
> @@ -115,6 +117,8 @@
>  #define AST2600_HW_STRAP2_PROTTO_REG(0x518)
>  #define AST2600_RNG_CTRL  TO_REG(0x524)
>  #define AST2600_RNG_DATA  TO_REG(0x540)
> +#define AST2600_CHIP_ID0  TO_REG(0x5B0)
> +#define AST2600_CHIP_ID1  TO_REG(0x5B4)
>  
>  #define AST2600_CLK TO_REG(0x40)
>  
> @@ -182,6 +186,8 @@ static const uint32_t 
> ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
>   [CPU2_BASE_SEG1]  = 0x8000U,
>   [CPU2_BASE_SEG4]  = 0x1E60U,
>   [CPU2_BASE_SEG5]  = 0xC000U,
> + [CHIP_ID0]= 0x1234ABCDU,
> + [CHIP_ID1]= 0xU,
>   [UART_HPLL_CLK]   = 0x1903U,
>   [PCIE_CTRL]   = 0x007BU,
>   [BMC_DEV_ID]  = 0x2402U
> @@ -307,6 +313,8 @@ static void aspeed_ast2500_scu_write(void *opaque, hwaddr 
> offset,
>  case RNG_DATA:
>  case FREE_CNTR4:
>  case FREE_CNTR4_EXT:
> +case CHIP_ID0:
> +case CHIP_ID1:
>  qemu_log_mask(LOG_GUEST_ERROR,
>"%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
>__func__, offset);
> @@ -620,6 +628,8 @@ static void aspeed_ast2600_scu_write(void *opaque, hwaddr 
> offset,
>  case AST2600_RNG_DATA:
>  case AST2600_SILICON_REV:
>  case AST2600_SILICON_REV2:
> +case AST2600_CHIP_ID0:
> +case AST2600_CHIP_ID1:
>  /* Add read only registers here */
>  qemu_log_mask(LOG_GUEST_ERROR,
>"%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
> @@ -648,6 +658,9 @@ static const uint32_t 
> ast2600_a0_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>  [AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
>  [AST2600_SDRAM_HANDSHAKE]   = 0x0040,  /* SoC completed DRAM init */
>  [AST2600_HPLL_PARAM]= 0x1000405F,
> +[AST2600_CHIP_ID0]  = 0x1234ABCD,
> +[AST2600_CHIP_ID1]  = 0x,
> +
>  };
>  
>  static void aspeed_ast2600_scu_reset(DeviceState *dev)
> 

Re: [PATCH] spapr: Migrate CAS reboot flag

[Cédric Le Goater]

On 1/21/20 4:41 AM, David Gibson wrote:
> On Wed, Jan 15, 2020 at 07:10:47PM +0100, Cédric Le Goater wrote:
>> On 1/15/20 6:48 PM, Greg Kurz wrote:
>>> Migration can potentially race with CAS reboot. If the migration thread
>>> completes migration after CAS has set spapr->cas_reboot but before the
>>> mainloop could pick up the reset request and reset the machine, the
>>> guest is migrated unrebooted and the destination doesn't reboot it
>>> either because it isn't aware a CAS reboot was needed (eg, because a
>>> device was added before CAS). This likely result in a broken or hung
>>> guest.
>>>
>>> Even if it is small, the window between CAS and CAS reboot is enough to
>>> re-qualify spapr->cas_reboot as state that we should migrate. Add a new
>>> subsection for that and always send it when a CAS reboot is pending.
>>> This may cause migration to older QEMUs to fail but it is still better
>>> than end up with a broken guest.
>>>
>>> The destination cannot honour the CAS reboot request from a post load
>>> handler because this must be done after the guest is fully restored.
>>> It is thus done from a VM change state handler.
>>>
>>> Reported-by: Lukáš Doktor 
>>> Signed-off-by: Greg Kurz 
>>
>> Cédric Le Goater 
>>
>> Nice work ! That was quite complex to catch !
> 
> It is a very nice analysis.  However, I'm disinclined to merge this
> for the time being.
> 
> My preferred approach would be to just eliminate CAS reboots
> altogether, since that has other benefits.  I'm feeling like this
> isn't super-urgent, since CAS reboots are extremely rare in practice,
> now that we've eliminated the one for the irq switchover.

Yes. The possibility of a migration in the window between CAS and 
CAS reboot must be even more rare.

C.

 
> However, if it's not looking like we'll be ready to do that as the
> qemu-5.0 release approaches, then I'll be more than willing to
> reconsider this.

Re: [PATCH v4 5/7] tests/boot_linux_console: Test booting U-Boot on the Raspberry Pi 2

[Gerd Hoffmann]

On Tue, Jan 21, 2020 at 12:51:57AM +0100, Philippe Mathieu-Daudé wrote:
> This test runs U-Boot on the Raspberry Pi 2.

> U-Boot is built by the Debian project, see:
> https://wiki.debian.org/InstallingDebianOn/Allwinner#Creating_a_bootable_SD_Card_with_u-boot

We already have a u-boot submodule in roms/

I guess it makes sense to just build & ship our own binaries
instead of downloading them from Debian?

cheers,
  Gerd

Re: [PATCH 016/104] virtiofsd: Open vhost connection instead of mounting

[Misono Tomohiro]

> From: "Dr. David Alan Gilbert" 
> 
> When run with vhost-user options we conect to the QEMU instead
> via a socket.  Start this off by creating the socket.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---


> +/*
> + * Poison the fuse FD so we spot if we accidentally use it;
> + * DO NOT check for this value, check for fuse_lowlevel_is_virtio()
> + */
> +se->fd = 0xdaff0d11;


As a result of this, se->fd now holds dummy fd.
So we should remove close(se->fd) in fuse_session_destroy():
https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/tools/virtiofsd/fuse_lowlevel.c#L2663

Reviewed-by: Misono Tomohiro 

Re: Proposal for handling .hx files with Sphinx

[Markus Armbruster]

John Snow  writes:

> On 1/17/20 12:30 PM, Peter Maydell wrote:
>> Currently our manual creation includes some .texi files which
>> are autogenerated from .hx files by running scripts/hxtool.
>> .hx files are a simple format, where where a line is either a
>> directive or literal text to be output:
>> 
>> HXCOMM
>>  -- comment lines, ignored
>> STEXI/ETEXI
>>  -- mark start/end of chunks of text to put into the texinfo output only
>> DEFHEADING/ARCHHEADING
>>  -- appear in the .h file output verbatim (they are defined as C macros);
>> for texi output they are parsed to add in header sections
>> 
>> For Sphinx, rather than creating a file to include, the most
>> natural way to handle this is to have a small custom Sphinx
>> extension which will read the .hx file and process it. So
>> instead of "makefile produces foo.texi from foo.hx, qemu-doc.texi
>> says '@include foo.texi'", we have "qemu-doc.rst says
>> 'hxtool-doc:: foo.hx', the Sphinx extension for hxtool has
>> code that runs to handle that Sphinx directive, it reads the .hx
>> file and emits the appropriate documentation contents". (This is
>> pretty much the same way the kerneldoc extension works right now.
>> It also has the advantage that it should work for third-party
>> services like readthedocs that expect to build the docs directly
>> with sphinx rather than by invoking our makefiles.)
>> 
>> We'll need to update what the markup is to handle having rST
>> fragments in it. A very minimalist approach to this would
>> simply define a new pair of SRST/ERST directives marking the
>> start/end of chunks of rST text to go into the rST only.
>> (We might be able to do better than that later, as there's
>> some repetition currently going on. But we'll probably get
>> a better idea of how easy it is to avoid the repetition if
>> we start with a simple conversion.)
>> 
>> Here's what we do with hx files at the moment. We have four:
>> 
>>  hmp-commands.hx
>>-- defines monitor commands used by monitor.c; generates
>>   qemu-monitor.texi, used by qemu-doc.texi
>>  hmp-commands-info.hx
>>-- ditto, for the "info" command's subcommand;
>>   generates qemu-monitor-info.texi, used by qemu-doc.texi
>> 
>> These two use only the "put this in the texi or in the .h file"
>> functionality, alternating "raw C code defining an entry for the
>> monitor command array" with "lump of raw texi for the docs".
>> 
>>  qemu-img-cmds.hx
>>-- defines options for qemu-img, used by qemu-img.texi
>> 
>> This uses the STEXI/ETEXI directives to alternate C and texi.
>> In the for-the-h-file section the only content is always a DEF()
>> macro invocation defining the option; in the texi is only the
>> synopsis of the command. This means there's a lot of repetition,
>> as the DEF macro includes an argument giving the text of the
>> option synopsis, and then the texi also has that synopsis with
>> some extra markup. Finally the main qemu-img.texi repeats the
>> marked-up synopsis later on when it has the actual main documentation
>> of each command.
>> 
>>  qemu-options.hx
>>-- options for qemu proper, used by qemu-doc.texi
>> 
>> This uses only the DEF, DEFHEADING, ARCHHEADING macros
>> in the for-the-h-file sections (and the DEFHEADING/ARCHHEADING
>> are read also for texi generation). This also repeats the
>> synopsis in the DEF macro and in the texi fragment.
>> 
>> So I think my current view is that we should do the very
>> simple "add SRST/ERST directives" to start with:
>>  * scripts/hxtool needs to recognize them and just ignore
>>the text inside them
>>  * write the hxtool sphinx extension (shouldn't be too hard)
>>  * conversion of any particular .hx file then involves
>>replacing the STEXI ...texi stuff... ETEXI sections with
>>SRST ...rst stuff... ERST. There's no need for any
>>particular .hx file to support both texi and rst output
>>at the same time
>> 
>> I would initially start with qemu-img-cmds.hx, since that's
>> pulled in by qemu-img.texi, which we can convert in the
>> same way I've been doing qemu-nbd and other standalone-ish
>> manpages. The others are part of the big fat qemu-doc.texi,
>> which is probably going to be the very last thing we convert...
>> 
>
> At one point I did a quick mockup of turning qemu-img-cmds.hx into json
> and wrote a small tool I called "pxtool" that was used for generating
> all the rest of the subsequent information -- an attempt at getting rid
> of .hx files *entirely*.
>
> The idea at heart was: "Can we remove .hx files and describe everything
> in terms of the QAPI schema instead?"
>
> I'm still a bit partial to that idea, but realize there are some nasty
> complexities when it comes to describing the QEMU CLI as a schema. One
> of those is that I doubt we even have a full understanding of what the
> CLI syntax is at all.

My CLI QAPIfication prototype[*] gets rid of qemu-options.hx.  Three
more are left: hmp-commands.hx, hmp-commands-info.hx, qemu-img-cmds.hx.
No idea whether 

Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0

[Michael S. Tsirkin]

On Mon, Jan 13, 2020 at 01:37:02PM +0100, Igor Mammedov wrote:
> On Thu, 19 Dec 2019 14:47:58 +0800
> Heyi Guo  wrote:
> 
> > The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
> > so simply remote it.
> Could you make commit message more concrete so it would say
> why it doesn't make any sense.
> 
> It seems to be there to describe root port,
> I'd rather have PCI folk ack if it's ok to remove it.

An empty device like this doesn't really do anything useful I think.
commit log needs to be fixed up though.


> > 
> > Signed-off-by: Heyi Guo 
> > 
> > ---
> > Cc: Peter Maydell 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Igor Mammedov 
> > Cc: Shannon Zhao 
> > Cc: qemu-...@nongnu.org
> > Cc: qemu-devel@nongnu.org
> > ---
> >  hw/arm/virt-acpi-build.c  |   4 
> >  tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
> >  tests/data/acpi/virt/DSDT.memhp   | Bin 19799 -> 19786 bytes
> >  tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
> >  4 files changed, 4 deletions(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index bd5f771e9b..9f4c7d1889 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
> > MemMapEntry *memmap,
> >  aml_append(method, aml_return(buf));
> >  aml_append(dev, method);
> >  
> > -Aml *dev_rp0 = aml_device("%s", "RP0");
> > -aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> > -aml_append(dev, dev_rp0);
> > -
> >  Aml *dev_res0 = aml_device("%s", "RES0");
> >  aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
> >  crs = aml_resource_template();
> > diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> > index 
> > d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c
> >  100644
> > GIT binary patch
> > delta 39
> > vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)  
> > 
> > delta 50
> > zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?^tF5;R%I  
> > G{V4!>hYx%J  
> > 
> > diff --git a/tests/data/acpi/virt/DSDT.memhp 
> > b/tests/data/acpi/virt/DSDT.memhp
> > index 
> > 41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce
> >  100644
> > GIT binary patch
> > delta 40
> > wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s
> > 
> > delta 51
> > zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb  
> > HyBr$;t7;Fc
> > 
> > diff --git a/tests/data/acpi/virt/DSDT.numamem 
> > b/tests/data/acpi/virt/DSDT.numamem
> > index 
> > d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c
> >  100644
> > GIT binary patch
> > delta 39
> > vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)  
> > 
> > delta 50
> > zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?^tF5;R%I  
> > G{V4!>hYx%J  
> > 

Re: [PATCH] ui/console: Display the 'none' backend in '-display help'

[Gerd Hoffmann]

On Mon, Jan 20, 2020 at 08:29:47PM +0100, Philippe Mathieu-Daudé wrote:
> Commit c388f408b5 added the possibility to list the display
> backends using '-display help'. Since the 'none' backend is
> is not implemented as a DisplayChangeListenerOps, it is not
> registered to the dpys[] array with qemu_display_register(),
> and is not listed in the help output.
> 
> This might be confusing, as we list it in the man page:
> 
>   -display type
>   Select type of display to use. This option is a replacement for
>   the old style -sdl/-curses/... options. Valid values for type are
> 
>   none
>   Do not display video output. The guest will still see an
>   emulated graphics card, but its output will not be displayed
>   to the QEMU user. This option differs from the -nographic
>   option in that it only affects what is done with video
>   output; -nographic also changes the destination of the serial
>   and parallel port data.
> 
> Fix by manually listing the special 'none' backend in the help.
> 
> Suggested-by: Thomas Huth 
> Signed-off-by: Philippe Mathieu-Daudé 

Added to ui queue.

thanks,
  Gerd

Re: [PATCH v2 1/2] vnc: fix VNC artifacts

[Gerd Hoffmann]

On Mon, Jan 20, 2020 at 09:00:51PM -0800, Cameron Esfahani wrote:
> Patch de3f7de7f4e257ce44cdabb90f5f17ee99624557 was too simplistic in its
> implementation: it didn't account for the ZLIB z_stream mutating with
> each compression.  Because of the mutation, simply resetting the output
> buffer's offset wasn't sufficient to "rewind" the operation.  The mutated
> z_stream would generate future zlib blocks which referred to symbols in
> past blocks which weren't sent.  This would lead to artifacting.
> 
> This reverts commit de3f7de7f4e257ce44cdabb90f5f17ee99624557.
> 
> Fixes:  ("vnc: allow fall back to RAW encoding")
> Signed-off-by: Cameron Esfahani 

Looks like you didn't realize that "revert" was meant literally.  Git has a
revert subcommand, i.e. you can simply run "git revert de3f7de7f4e257" to
create a commit undoing the changes, with a commit message saying so.  The
generated text should be left intact, to make the job for tools analyzing git
commits easier.  The commit message (for reverts typically explaining why the
reverted commit was buggy) can go below the generated text.

Also note that only the patch commit messages end up in the commit log, the
cover letter text doesn't.  So any important details should (also) be in the
commit messages so they are recorded in the log.

Reworked the commit message, looks like this now:

-
commit 0780ec7be82dd4781e9fd216b5d99a125882ff5a (HEAD -> queue/ui)
Author: Gerd Hoffmann 
Date:   Tue Jan 21 07:02:10 2020 +0100

Revert "vnc: allow fall back to RAW encoding"

This reverts commit de3f7de7f4e257ce44cdabb90f5f17ee99624557.

Remove VNC optimization to reencode framebuffer update as raw if it's
smaller than the default encoding.

QEMU's implementation was naive and didn't account for the ZLIB z_stream
mutating with each compression.  Because of the mutation, simply
resetting the output buffer's offset wasn't sufficient to "rewind" the
operation.  The mutated z_stream would generate future zlib blocks which
referred to symbols in past blocks which weren't sent.  This would lead
to artifacting.

Considering that ZRLE is never larger than raw and even though ZLIB can
occasionally be fractionally larger than raw, the overhead of
implementing this optimization correctly isn't worth it.

Signed-off-by: Cameron Esfahani 
Signed-off-by: Gerd Hoffmann 
-

Modified patch queued up.
Patch 2/2 is fine as-is.

thanks,
  Gerd

[virtio-dev] [PATCH v2 4/5] virtio-mmio: Introduce MSI details

[Jing Liu]

With VIRTIO_F_MMIO_MSI feature bit offered, the Message Signal
Interrupts (MSI) is supported as first priority. For any reason it
fails to use MSI, it need use the single dedicated interrupt as before.

For MSI vectors and events mapping relationship, introduce in next patch.

Co-developed-by: Chao Peng 
Signed-off-by: Chao Peng 
Co-developed-by: Liu Jiang 
Signed-off-by: Liu Jiang 
Co-developed-by: Zha Bin 
Signed-off-by: Zha Bin 
Signed-off-by: Jing Liu 
---
 content.tex | 171 ++--
 msi-state.c |   4 ++
 2 files changed, 159 insertions(+), 16 deletions(-)
 create mode 100644 msi-state.c

diff --git a/content.tex b/content.tex
index ff151ba..dcf6c71 100644
--- a/content.tex
+++ b/content.tex
@@ -1687,7 +1687,8 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio 
Transport Options / Vi
   \hline 
   \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
 Reading from this register returns a bit mask of events that
-caused the device interrupt to be asserted.
+caused the device interrupt to be asserted. This is only used
+when MSI is not enabled.
 The following events are possible:
 \begin{description}
   \item[Used Buffer Notification] - bit 0 - the interrupt was asserted
@@ -1701,7 +1702,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio 
Transport Options / Vi
   \mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
 Writing a value with bits set as defined in \field{InterruptStatus}
 to this register notifies the device that events causing
-the interrupt have been handled.
+the interrupt have been handled. This is only used when MSI is not enabled.
   }
   \hline 
   \mmioreg{Status}{Device status}{0x070}{RW}{%
@@ -1760,6 +1761,47 @@ \subsection{MMIO Device Register 
Layout}\label{sec:Virtio Transport Options / Vi
 \field{SHMSel} is unused) results in a base address of
 0x.
   }
+  \hline
+  \mmioreg{MsiVecNum}{MSI max vector number}{0x0c0}{R}{%
+When VIRTIO_F_MMIO_MSI has been negotiated, reading
+from this register returns the maximum MSI vector number
+that device supports.
+  }
+  \hline
+  \mmioreg{MsiState}{MSI state}{0x0c4}{R}{%
+When VIRTIO_F_MMIO_MSI has been negotiated, reading
+from this register returns the global MSI enable/disable status.
+\lstinputlisting{msi-state.c}
+  }
+  \hline
+  \mmioreg{MsiCmd}{MSI command}{0x0c8}{W}{%
+When VIRTIO_F_MMIO_MSI has been negotiated, writing
+to this register executes the corresponding command to device.
+Part of this applies to the MSI vector selected by writing to 
\field{MsiVecSel}.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific 
Initialization And Device Operation / Device Initialization / MSI Vector 
Configuration}
+for using details.
+  }
+  \hline
+  \mmioreg{MsiVecSel}{MSI vector index}{0x0d0}{W}{%
+When VIRTIO_F_MMIO_MSI has been negotiated, writing
+to this register selects the MSI vector index that the following operations
+on \field{MsiAddrLow}, \field{MsiAddrHigh}, \field{MsiData} and part of
+\field{MsiCmd} commands specified in \ref{sec:Virtio Transport Options / 
Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device 
Initialization / MSI Vector Configuration}
+apply to. The index number of the first vector is zero (0x0).
+  }
+  \hline
+  \mmiodreg{MsiAddrLow}{MsiAddrHigh}{MSI 64 bit address}{0x0d4}{0x0d8}{W}{%
+When VIRTIO_F_MMIO_MSI has been negotiated, writing
+to these two registers (lower 32 bits of the address to \field{MsiAddrLow},
+higher 32 bits to \field{MsiAddrHigh}) notifies the device about the
+MSI address. This applies to the MSI vector selected by writing to 
\field{MsiVecSel}.
+  }
+  \hline
+  \mmioreg{MsiData}{MSI 32 bit data}{0x0dc}{W}{%
+When VIRTIO_F_MMIO_MSI has been negotiated, writing
+to this register notifies the device about the MSI data.
+This applies to the MSI vector selected by writing to \field{MsiVecSel}.
+  }
   \hline 
   \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
 Reading from this register returns a value describing a version of the 
device-specific configuration space (see \field{Config}).
@@ -1783,10 +1825,16 @@ \subsection{MMIO Device Register 
Layout}\label{sec:Virtio Transport Options / Vi
 
 The device MUST return value 0x2 in \field{Version}.
 
-The device MUST present each event by setting the corresponding bit in 
\field{InterruptStatus} from the
+When MSI is disabled, the device MUST present each event by setting the
+corresponding bit in \field{InterruptStatus} from the
 moment it takes place, until the driver acknowledges the interrupt
-by writing a corresponding bit mask to the \field{InterruptACK} register.  
Bits which
-do not represent events which took place MUST be zero.
+by writing a corresponding bit mask to the \field{InterruptACK} register.
+Bits which do not 

[virtio-dev] [PATCH v2 5/5] virtio-mmio: MSI vector and event mapping

[Jing Liu]

Bit 1 msi_sharing reported in the MsiState register indicates the mapping mode
device uses.

Bit 1 is 0 - device uses MSI non-sharing mode. This indicates vector per event 
and
fixed static vectors and events relationship. This fits for devices with a high 
interrupt
rate and best performance;
Bit 1 is 1 - device uses MSI sharing mode. This indicates vectors and events
dynamic mapping and fits for devices not requiring a high interrupt rate.

Co-developed-by: Chao Peng 
Signed-off-by: Chao Peng 
Co-developed-by: Liu Jiang 
Signed-off-by: Liu Jiang 
Co-developed-by: Zha Bin 
Signed-off-by: Zha Bin 
Signed-off-by: Jing Liu 
---
 content.tex | 48 +++-
 msi-state.c |  3 ++-
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/content.tex b/content.tex
index dcf6c71..2fd1686 100644
--- a/content.tex
+++ b/content.tex
@@ -1770,7 +1770,8 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio 
Transport Options / Vi
   \hline
   \mmioreg{MsiState}{MSI state}{0x0c4}{R}{%
 When VIRTIO_F_MMIO_MSI has been negotiated, reading
-from this register returns the global MSI enable/disable status.
+from this register returns the global MSI enable/disable status
+and whether device uses MSI sharing mode.
 \lstinputlisting{msi-state.c}
   }
   \hline
@@ -1926,12 +1927,18 @@ \subsubsection{Device Initialization}\label{sec:Virtio 
Transport Options / Virti
 mask and unmask the MSI vector applying to the one selected by writing
 to \field{MsiVecSel}.
 
+VIRTIO_MMIO_MSI_CMD_MAP_CONFIG command is to set the configuration event and 
MSI vector
+mapping. VIRTIO_MMIO_MSI_CMD_MAP_QUEUE is to set the queue event and MSI vector
+mapping. They SHOULD only be used in MSI sharing mode.
+
 \begin{lstlisting}
 #define  VIRTIO_MMIO_MSI_CMD_ENABLE   0x1
 #define  VIRTIO_MMIO_MSI_CMD_DISABLE  0x2
 #define  VIRTIO_MMIO_MSI_CMD_CONFIGURE0x3
 #define  VIRTIO_MMIO_MSI_CMD_MASK 0x4
 #define  VIRTIO_MMIO_MSI_CMD_UNMASK   0x5
+#define  VIRTIO_MMIO_MSI_CMD_MAP_CONFIG   0x6
+#define  VIRTIO_MMIO_MSI_CMD_MAP_QUEUE0x7
 \end{lstlisting}
 
 Setting a special NO_VECTOR value means disabling an interrupt for an event 
type.
@@ -1941,10 +1948,49 @@ \subsubsection{Device Initialization}\label{sec:Virtio 
Transport Options / Virti
 #define VIRTIO_MMIO_MSI_NO_VECTOR 0x
 \end{lstlisting}
 
+\subparagraph{MSI Vector and Event Mapping}\label{sec:Virtio Transport Options 
/ Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device 
Initialization / MSI Vector Configuration}
+The reported \field{msi_sharing} bit in the \field{MsiState} return value shows
+the MSI sharing mode that device uses.
+
+When \field{msi_sharing} bit is 0, it indicates the device uses non-sharing 
mode
+and vector per event fixed static relationship is used. The first vector is 
for device
+configuraiton change event, the second vector is for virtqueue 1, the third 
vector
+is for virtqueue 2 and so on.
+
+When \field{msi_sharing} bit is 1, it indicates the device uses MSI sharing 
mode,
+and the vector and event mapping is dynamic. Writing \field{MsiVecSel}
+followed by writing 
VIRTIO_MMIO_MSI_CMD_MAP_CONFIG/VIRTIO_MMIO_MSI_CMD_MAP_QUEUE command
+maps interrupts triggered by the configuration change/selected queue events 
respectively
+to the corresponding MSI vector.
+
+\devicenormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport 
Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation 
/ MSI Vector Configuration}
+
+When the device reports \field{msi_sharing} bit as 0, it SHOULD support a 
number of
+vectors that greater than the maximum number of virtqueues.
+Device MUST report the number of vectors supported in \field{MsiVecNum}.
+
+When the device reports \field{msi_sharing} bit as 1, it SHOULD support at 
least
+2 MSI vectors and MUST report in \field{MsiVecNum}. Device SHOULD support 
mapping any
+event type to any vector under \field{MsiVecNum}.
+
+Device MUST support unmapping any event type (NO_VECTOR).
+
+The device SHOULD restrict the reported \field{msi_sharing} and 
\field{MsiVecNum}
+to a value that might benefit system performance.
+
+\begin{note}
+For example, a device which does not expect to send interrupts at a high rate 
might
+return \field{msi_sharing} bit as 1.
+\end{note}
+
 \drivernormative{\subparagraph}{MSI Vector Configuration}{Virtio Transport 
Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation 
/ MSI Vector Configuration}
 When VIRTIO_F_MMIO_MSI has been negotiated, driver should try to configure
 and enable MSI.
 
+To set up the event and vector mapping for MSI sharing mode, driver SHOULD
+write a valid \field{MsiVecSel} followed by 
VIRTIO_MMIO_MSI_CMD_MAP_CONFIG/VIRTIO_MMIO_MSI_CMD_MAP_QUEUE
+command to map the configuration change/selected queue events respectively.
+
 To configure MSI vector, driver SHOULD firstly 

[virtio-dev] [PATCH v2 2/5] virtio-mmio: Enhance queue notification support

[Jing Liu]

With VIRTIO_F_MMIO_NOTIFICATION feature bit offered, the notification
mechanism is enhanced. Driver reads QueueNotify register to get
notification structure and calculate notification addresses of
each virtqueue.

Co-developed-by: Chao Peng 
Signed-off-by: Chao Peng 
Co-developed-by: Liu Jiang 
Signed-off-by: Liu Jiang 
Co-developed-by: Zha Bin 
Signed-off-by: Zha Bin 
Signed-off-by: Jing Liu 
---
 content.tex | 53 -
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/content.tex b/content.tex
index 826bc7d..5881253 100644
--- a/content.tex
+++ b/content.tex
@@ -1671,20 +1671,18 @@ \subsection{MMIO Device Register 
Layout}\label{sec:Virtio Transport Options / Vi
 accesses apply to the queue selected by writing to \field{QueueSel}.
   }
   \hline 
-  \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
-Writing a value to this register notifies the device that
-there are new buffers to process in a queue.
+  \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
+When VIRTIO_F_MMIO_NOTIFICATION has not been negotiated, writing to this
+register notifies the device that there are new buffers to process in a 
queue.
 
-When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-the value written is the queue index.
+When VIRTIO_F_MMIO_NOTIFICATION has been negotiated, reading this register
+returns the virtqueue notification structure for calculating notification 
location.
 
-When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
-the \field{Notification data} value has the following format:
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific 
Initialization And Device Operation / Notification Structure Layout}
+for the notification structure format.
 
-\lstinputlisting{notifications-le.c}
-
-See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / 
Driver notifications}
-for the definition of the components.
+See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific 
Initialization And Device Operation / Available Buffer Notifications}
+for the notification data format.
   }
   \hline 
   \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
@@ -1858,6 +1856,31 @@ \subsubsection{Device Initialization}\label{sec:Virtio 
Transport Options / Virti
 Further initialization MUST follow the procedure described in
 \ref{sec:General Initialization And Device Operation / Device 
Initialization}~\nameref{sec:General Initialization And Device Operation / 
Device Initialization}.
 
+\subsubsection{Notification Structure Layout}\label{sec:Virtio Transport 
Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation 
/ Notification Structure Layout}
+
+When VIRTIO_F_MMIO_NOTIFICATION has been negotiated, the notification location 
is calculated
+by notification structure. Driver reads \field{QueueNotify} to get this 
structure formatted
+as follows.
+
+\begin{lstlisting}
+le32 {
+notify_base : 16;
+notify_multiplier : 16;
+};
+\end{lstlisting}
+
+\field{notify_multiplier} is combined with virtqueue index to derive the Queue 
Notify address
+within a memory mapped control registers for a virtqueue:
+
+\begin{lstlisting}
+notify_base + queue_index * notify_multiplier
+\end{lstlisting}
+
+\begin{note}
+For example, if notify_multiplier is 0, the device uses the same Queue Notify 
address for all
+queues.
+\end{note}
+
 \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / 
Virtio Over MMIO / MMIO-specific Initialization And Device Operation / 
Virtqueue Configuration}
 
 The driver will typically initialize the virtual queue in the following way:
@@ -1893,16 +1916,20 @@ \subsubsection{Available Buffer 
Notifications}\label{sec:Virtio Transport Option
 When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
 the driver sends an available buffer notification to the device by writing
 the 16-bit virtqueue index
-of the queue to be notified to \field{QueueNotify}.
+of the queue to be notified to Queue Notify address.
 
 When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
 the driver sends an available buffer notification to the device by writing
-the following 32-bit value to \field{QueueNotify}:
+the following 32-bit value to Queue Notify address:
 \lstinputlisting{notifications-le.c}
 
 See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / 
Driver notifications}
 for the definition of the components.
 
+For device not offering VIRTIO_F_MMIO_NOTIFICATION, the Queue Notify address 
is \field{QueueNotify}.
+For device offering VIRTIO_F_MMIO_NOTIFICATION, see \ref{sec:Virtio Transport 
Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation 
/ Notification Structure Layout}
+for how to calculate the Queue Notify address.
+
 \subsubsection{Notifications From The Device}\label{sec:Virtio Transport 
Options / Virtio Over MMIO / MMIO-specific 

[virtio-dev] [PATCH v2 3/5] virtio-mmio: Add feature bit for MMIO MSI

[Jing Liu]

The current MMIO transport layer uses a single, dedicated interrupt
signal, which brings performance penalty. Add a feature bit (40)
for introducing MSI capability.

Co-developed-by: Chao Peng 
Signed-off-by: Chao Peng 
Co-developed-by: Liu Jiang 
Signed-off-by: Liu Jiang 
Co-developed-by: Zha Bin 
Signed-off-by: Zha Bin 
Signed-off-by: Jing Liu 
---
 content.tex | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/content.tex b/content.tex
index 5881253..ff151ba 100644
--- a/content.tex
+++ b/content.tex
@@ -5840,6 +5840,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved 
Feature Bits}
   \item[VIRTIO_F_MMIO_NOTIFICATION(39)] This feature indicates
   that the device supports enhanced notification mechanism on
   MMIO transport layer.
+  \item[VIRTIO_F_MMIO_MSI(40)] This feature indicates that the
+  device supports Message Signal Interrupts (MSI) mechanism on
+  MMIO transport layer.
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
 
@@ -5875,6 +5878,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved 
Feature Bits}
 
 A driver SHOULD accept VIRTIO_F_MMIO_NOTIFICATION if it is offered.
 
+A driver SHOULD accept VIRTIO_F_MMIO_MSI if it is offered.
+If VIRTIO_F_MMIO_MSI has been negotiated, a driver MUST try to
+set up MSI at first priority.
+
 \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
 
 A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further
-- 
2.7.4

[virtio-dev] [PATCH v2 0/5] virtio-mmio enhancement

[Jing Liu]

The current virtio over MMIO has some limitations that impact the performance.
It only supports single legacy, dedicated interrupt and one virtqueue
notification register for all virtqueues which cause performance penalties.

To address such limitations, we proposed to update virtio-mmio spec with
two new feature bits to support MSI interrupt and enhancing notification 
mechanism.

For keeping virtio-mmio simple as it was, and considering practical usages, this
provides two kinds of mapping mode for device: MSI non-sharing mode and MSI 
sharing mode.
MSI non-sharng mode indicates a fixed static vector and event relationship 
specified
in spec, which can simplify the setup process and reduce vmexit, fitting for
a high interrupt rate request.
MSI sharing mode indicates a dynamic mapping, which is more like PCI does, 
fitting
for a non-high interrupt rate request.

Change Log:
v1->v2:
* Change version update to feature bit
* Add mask/unmask support
* Add two MSI sharing/non-sharing modes
* Change MSI registers layout and bits

Jing Liu (5):
  virtio-mmio: Add feature bit for MMIO notification
  virtio-mmio: Enhance queue notification support
  virtio-mmio: Add feature bit for MMIO MSI
  virtio-mmio: Introduce MSI details
  virtio-mmio: MSI vector and event mapping

 content.tex | 286 ++--
 msi-state.c |   5 ++
 2 files changed, 262 insertions(+), 29 deletions(-)
 create mode 100644 msi-state.c

-- 
2.7.4

[virtio-dev] [PATCH v2 1/5] virtio-mmio: Add feature bit for MMIO notification

[Jing Liu]

All the queues notifications use the same register on MMIO transport
layer. Add a feature bit (39) for enhancing the notification capability.
The detailed mechanism would be in next patch.

Co-developed-by: Chao Peng 
Signed-off-by: Chao Peng 
Co-developed-by: Liu Jiang 
Signed-off-by: Liu Jiang 
Co-developed-by: Zha Bin 
Signed-off-by: Zha Bin 
Signed-off-by: Jing Liu 
---
 content.tex | 9 +
 1 file changed, 9 insertions(+)

diff --git a/content.tex b/content.tex
index d68cfaf..826bc7d 100644
--- a/content.tex
+++ b/content.tex
@@ -5810,6 +5810,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved 
Feature Bits}
   in its device notifications.
   See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / 
Driver notifications}.
 \end{description}
+  \item[VIRTIO_F_MMIO_NOTIFICATION(39)] This feature indicates
+  that the device supports enhanced notification mechanism on
+  MMIO transport layer.
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
 
@@ -5843,6 +5846,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved 
Feature Bits}
 or partially reset, and even without re-negotiating
 VIRTIO_F_SR_IOV after the reset.
 
+A driver SHOULD accept VIRTIO_F_MMIO_NOTIFICATION if it is offered.
+
 \devicenormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
 
 A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further
@@ -5872,6 +5877,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved 
Feature Bits}
 and presents a PCI SR-IOV capability structure, otherwise
 it MUST NOT offer VIRTIO_F_SR_IOV.
 
+If VIRTIO_F_MMIO_NOTIFICATION has been negotiated, a device
+MUST support handling the notification from driver at the
+calculated location.
+
 \section{Legacy Interface: Reserved Feature Bits}\label{sec:Reserved Feature 
Bits / Legacy Interface: Reserved Feature Bits}
 
 Transitional devices MAY offer the following:
-- 
2.7.4

Re: [PATCH v4 15/18] tests/boot-serial-test: Test some Arduino boards (AVR based)

[Thomas Huth]

On 20/01/2020 23.01, Philippe Mathieu-Daudé wrote:
> The Arduino Duemilanove is based on a AVR5 CPU, while the
> Arduino MEGA2560 on a AVR6 CPU.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/qtest/boot-serial-test.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index e556f09db8..582a497963 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -113,6 +113,8 @@ typedef struct testdef {
>  static testdef_t tests[] = {
>  { "alpha", "clipper", "", "PCI:" },
>  { "avr", "sample", "", "T", sizeof(bios_avr), NULL, bios_avr },
> +{ "avr", "arduino-duemilanove", "", "T", sizeof(bios_avr), NULL, 
> bios_avr },
> +{ "avr", "arduino-mega-2560-v3", "", "T", sizeof(bios_avr), NULL, 
> bios_avr},
>  { "ppc", "ppce500", "", "U-Boot" },
>  { "ppc", "40p", "-vga none -boot d", "Trying cd:," },
>  { "ppc", "g3beige", "", "PowerPC,750" },
> 

Acked-by: Thomas Huth 

Re: [PATCH 3/3] hw/display/qxl.c: Use trace_event_get_state_backends()

[Gerd Hoffmann]

On Mon, Jan 20, 2020 at 03:11:42PM +, Peter Maydell wrote:
> The preferred way to test whether a trace event is enabled is to
> use trace_event_get_state_backends(), because this will give the
> correct answer (allowing expensive computations to be skipped)
> whether the trace event is compile-time or run-time disabled.
> Convert the old-style direct use of TRACE_FOO_ENABLED.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/display/qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 6d43b7433cf..80a4dcc40e4 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1764,7 +1764,7 @@ async_common:
>  qxl_set_mode(d, val, 0);
>  break;
>  case QXL_IO_LOG:
> -if (TRACE_QXL_IO_LOG_ENABLED || d->guestdebug) {
> +if (trace_event_get_state_backends(TRACE_QXL_IO_LOG) || 
> d->guestdebug) {

Acked-by: Gerd Hoffmann 

>  /* We cannot trust the guest to NUL terminate d->ram->log_buf */
>  char *log_buf = g_strndup((const char *)d->ram->log_buf,
>sizeof(d->ram->log_buf));
> -- 
> 2.20.1
> 

Re: [RFC PATCH] qapi: Incorrect attempt to fix building with MC146818RTC=n

[Markus Armbruster]

Paolo Bonzini  writes:

> On 13/01/20 15:01, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>>> When configured with --without-default-devices and setting
>>> MC146818RTC=n, the build fails:
>>>
>>> LINKx86_64-softmmu/qemu-system-x86_64
>>>   /usr/bin/ld: qapi/qapi-commands-misc-target.o: in function 
>>> `qmp_marshal_rtc_reset_reinjection':
>>>   qapi/qapi-commands-misc-target.c:46: undefined reference to 
>>> `qmp_rtc_reset_reinjection'
>>>   /usr/bin/ld: qapi/qapi-commands-misc-target.c:46: undefined reference to 
>>> `qmp_rtc_reset_reinjection'
>>>   collect2: error: ld returned 1 exit status
>>>   make[1]: *** [Makefile:206: qemu-system-x86_64] Error 1
>>>   make: *** [Makefile:483: x86_64-softmmu/all] Error 2
>>>
>>> This patch tries to fix this, but this is incorrect because QAPI
>>> scripts only provide TARGET definitions, so with MC146818RTC=y we
>>> get:
>>>
>>>   hw/rtc/mc146818rtc.c:113:6: error: no previous prototype for 
>>> ‘qmp_rtc_reset_reinjection’ [-Werror=missing-prototypes]
>>> 113 | void qmp_rtc_reset_reinjection(Error **errp)
>>> |  ^
>>>   cc1: all warnings being treated as errors
>>>   make[1]: *** [rules.mak:69: hw/rtc/mc146818rtc.o] Error 1
>>>
>>> Any idea? :)
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>>  qapi/misc-target.json | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>>> index a00fd821eb..8e49c113d1 100644
>>> --- a/qapi/misc-target.json
>>> +++ b/qapi/misc-target.json
>>> @@ -41,7 +41,7 @@
>>>  #
>>>  ##
>>>  { 'command': 'rtc-reset-reinjection',
>>> -  'if': 'defined(TARGET_I386)' }
>>> +  'if': 'defined(TARGET_I386) && defined(CONFIG_MC146818RTC)' }
>>>  
>>>  
>>>  ##
>> 
>> The generated qapi-commands-misc-target.h duly has
>> 
>> #if defined(TARGET_I386) && defined(CONFIG_MC146818RTC)
>> void qmp_rtc_reset_reinjection(Error **errp);
>> void qmp_marshal_rtc_reset_reinjection(QDict *args, QObject **ret, Error 
>> **errp);
>> #endif /* defined(TARGET_I386) && defined(CONFIG_MC146818RTC) */
>> 
>> mc146818rtc.c includes it.  But since it doesn't include
>> config-devices.h, CONFIG_MC146818RTC remains undefined, and the
>> prototype gets suppressed.
>> 
>> Crude fix: make mc146818rtc.c #include "config-devices.h".
>
> Can we modify the code generator to leave out the #if from the header,
> and only include it in the .c file?  An extra prototype is harmless.

Is *everything* we generate into headers just as harmless?

Another idea: provide a hook to make the generator insert the #include
necessary to get the macros used in 'if'.

Yet another idea: include it always in target-specific code, just like
config-target.h.

[PATCH v1 1/1] target/riscv: Correctly implement TSR trap

[Alistair Francis]

As reported in: https://bugs.launchpad.net/qemu/+bug/1851939 we weren't
correctly handling illegal instructions based on the value of MSTATUS_TSR
and the current privledge level.

This patch fixes the issue raised in the bug by raising an illegal
instruction if TSR is set and we are in S-Mode.

Signed-off-by: Alistair Francis 
---
 target/riscv/op_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 331cc36232..eed8eea6f2 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -83,7 +83,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong 
cpu_pc_deb)
 }
 
 if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
-get_field(env->mstatus, MSTATUS_TSR)) {
+get_field(env->mstatus, MSTATUS_TSR) && !(env->priv >= PRV_M)) {
 riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
 }
 
-- 
2.24.1

Re: Making QEMU easier for management tools and applications

[Markus Armbruster]

Stefan Hajnoczi  writes:

> On Wed, Jan 15, 2020 at 01:15:17PM +0100, Markus Armbruster wrote:
>> Christophe de Dinechin  writes:
>> >> On 15 Jan 2020, at 10:20, Markus Armbruster  wrote:
>> * qemuMonitorJSONSetIOThread() uses it to control iothread's properties
>>   poll-max-ns, poll-grow, poll-shrink.  Their use with -object is
>>   documented (in qemu-options.hx), their use with qom-set is not.
>
> I'm happy to use a different interface.
>
> Writing a boilerplate "iothread-set-poll-params" QMP command in C would
> be a step backwards.

No argument.

> Maybe the QAPI code generator could map something like this:
>
>   { 'command': 'iothread-set-poll-params',
> 'data': {
> 'id': 'str',
>   '*max-ns': 'uint64',
>   '*grow': 'uint64',
>   '*shrink': 'uint64'
> },
> 'map-to-qom-set': 'IOThread'
>   }
>
> And turn it into QOM accessors on the IOThread object.

I think a generic "set this configuration to that value" command is just
fine.  qom-set fails on several counts, though:

* Tolerable: qom-set is not actually generic, it applies only to QOM.

* qom-set lets you set tons of stuff that is not meant to be changed at
  run time.  If it breaks your guest, you get to keep the pieces.

* There is virtually no documentation on what can be set to what values,
  and their semantics.

In its current state, QOM is a user interface superfund site.

Re: [Qemu-devel] What should a virtual board emulate?

[Markus Armbruster]

Paolo Bonzini  writes:

> On 04/01/20 22:16, Philippe Mathieu-Daudé wrote:
>> 1/ the Radeon chip is soldered on the motherboard,
>> 
>> 2/ the default BIOS expects the Radeon chip to be
>>    unconditionally present,
>> 
>> I insist this patch is incorrect for the particular case of the
>> Fuloong2e board. I plan to revert it when I post the test.
>> 
>> BTW I'm not using --nodefault, I'm running default ./configure:
>> 
>> qemu-system-mips64el -M fulong2e -bios pmon_2e.bin \
>> -display none -vga none -serial stdio
>
> But if you're not specifying -nodefaults, why are you specifying a
> configuration that your BIOS does not support?  You should just remove
> -vga none and leave in -display none.

Is there any use for -vga none with this machine?  If no, then rejecting
it cleanly would be nicer than having the machine hang.

Re: [PATCH] riscv: Fix defination of TW bits in mstatus CSR

[Alistair Francis]

On Mon, Jan 20, 2020 at 6:59 PM Ian Jiang  wrote:
>
> The origin defination of TW bits in mstatus is not correct.
> This patch fixes the problem.
>
> Signed-off-by: Ian Jiang 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu_bits.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e99834856c..fb2e0b340e 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -349,7 +349,7 @@
>  #define MSTATUS_MXR 0x0008
>  #define MSTATUS_VM  0x1F00 /* until: priv-1.9.1 */
>  #define MSTATUS_TVM 0x0010 /* since: priv-1.10 */
> -#define MSTATUS_TW  0x2000 /* since: priv-1.10 */
> +#define MSTATUS_TW  0x0020 /* since: priv-1.10 */
>  #define MSTATUS_TSR 0x4000 /* since: priv-1.10 */
>  #define MSTATUS_MTL 0x40ULL
>  #define MSTATUS_MPV 0x80ULL
> --
> 2.17.1
>
>

Re: [PATCH qemu v5] spapr: Kill SLOF

[David Gibson]

On Fri, Jan 10, 2020 at 01:09:25PM +1100, Alexey Kardashevskiy wrote:
> The Petitboot bootloader is way more advanced than SLOF is ever going to
> be as Petitboot comes with the full-featured Linux kernel with all
> the drivers, and initramdisk with quite user friendly interface.
> The problem with ditching SLOF is that an unmodified pseries kernel can
> either start via:
> 1. kexec, this requires presence of RTAS and skips
> ibm,client-architecture-support entirely;
> 2. normal boot, this heavily relies on the OF1275 client interface to
> fetch the device tree and do early setup (claim memory).
> 
> This adds a new bios-less mode to the pseries machine: "bios=on|off".
> When enabled, QEMU does not load SLOF and jumps to the kernel from
> "-kernel".

I don't love the name "bios" for this flag, since BIOS tends to refer
to old-school x86 firmware.  Given the various plans we're considering
the future, I'd suggest "firmware=slof" for the current in-guest SLOF
mode, and say "firmware=vof" (Virtual Open Firmware) for the new
model.  We can consider firmware=petitboot or firmware=none (for
direct kexec-style boot into -kernel) or whatever in the future

> The client interface is implemented exactly as RTAS - a 20 bytes blob,
> right next after the RTAS blob. The entry point is passed to the kernel
> via GPR5.
> 
> This implements a handful of client interface methods just to get going.
> In particular, this implements the device tree fetching,
> ibm,client-architecture-support and instantiate-rtas.
> 
> This implements changing FDT properties for RTAS (for vmlinux and zImage)
> and initramdisk location (for zImage). To make this work, this skips
> fdt_pack() when bios=off as not packing the blob leaves some room for
> appending.
> 
> This assigns "phandles" to device tree nodes as there is no more SLOF
> and OF nodes addresses of which served as phandle values.
> This keeps predefined nodes (such as XICS/NVLINK/...) unchanged.
> phandles are regenerated at every FDT rebuild.
> 
> When bios=off, this adds "/chosen" every time QEMU builds a tree.
> 
> This implements "claim" which the client (Linux) uses for memory
> allocation; this is also  used by QEMU for claiming kernel/initrd images,
> client interface entry point, RTAS and the initial stack.
> 
> While at this, add a "kernel-addr" machine parameter to allow moving
> the kernel in memory. This is useful for debugging if the kernel is
> loaded at @0, although not necessary.
> 
> This adds basic instances support which are managed by a hashmap
> ihandle->[phandle, DeviceState, Chardev].
> 
> Note that a 64bit PCI fix is required for Linux:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df5be5be8735e
> 
> The test command line:
> 
> qemu-system-ppc64 \
> -nodefaults \
> -chardev stdio,id=STDIO0,signal=off,mux=on \
> -device spapr-vty,id=svty0,reg=0x71000110,chardev=STDIO0 \
> -mon id=MON0,chardev=STDIO0,mode=readline \
> -nographic \
> -vga none \
> -kernel pbuild/kernel-le-guest/arch/powerpc/boot/zImage.pseries \
> -machine pseries,bios=off,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken \
> -m 4G \
> -enable-kvm \
> -initrd pb/rootfs.cpio.xz \
> -device nec-usb-xhci,id=nec-usb-xhci0 \
> -netdev tap,id=TAP0,helper=/home/aik/qemu-bridge-helper --br=br0 \
> -device virtio-net-pci,id=vnet0,netdev=TAP0 img/f30le.qcow2 \
> -snapshot \
> -smp 8,threads=8 \
> -trace events=qemu_trace_events \
> -d guest_errors \
> -chardev socket,id=SOCKET0,server,nowait,path=qemu.mon.ssh54088 \
> -mon chardev=SOCKET0,mode=control
> 
> Signed-off-by: Alexey Kardashevskiy 

It'd be nice to split this patch up a bit, though I'll admit it's not
very obvious where to do so.

> ---
> Changes:
> v5:
> * made instances keep device and chardev pointers
> * removed VIO dependencies
> * print error if RTAS memory is not claimed as it should have been
> * pack FDT as "quiesce"
> 
> v4:
> * fixed open
> * validate ihandles in "call-method"
> 
> v3:
> * fixed phandles allocation
> * s/__be32/uint32_t/ as we do not normally have __be32 type in qemu
> * fixed size of /chosen/stdout
> * bunch of renames
> * do not create rtas properties at all, let the client deal with it;
> instead setprop allows changing these in the FDT
> * no more packing FDT when bios=off - nobody needs it and getprop does not
> work otherwise
> * allow updating initramdisk device tree properties (for zImage)
> * added instances
> * fixed stdout on OF's "write"
> * removed special handling for stdout in OF client, spapr-vty handles it
> instead
> 
> v2:
> * fixed claim()
> * added "setprop"
> * cleaner client interface and RTAS blobs management
> * boots to petitboot and further to the target system
> * more trace points
> ---
>  hw/ppc/Makefile.objs |   1 +
>  include/hw/ppc/spapr.h   |  28 +-
>  hw/ppc/spapr.c   | 266 ++--
>  hw/ppc/spapr_hcall.c |  74 +++--
>  hw/ppc/spapr_of_client.c | 633 +++
>  hw/ppc/trace-events  |  12 

[PATCH v2 2/2] vnc: prioritize ZRLE compression over ZLIB

[Cameron Esfahani via]

In my investigation, ZRLE always compresses better than ZLIB so
prioritize ZRLE over ZLIB, even if the client hints that ZLIB is
preferred.

zlib buffer is always reset in zrle_compress_data(), so using offset to
calculate next_out and avail_out is useless.

Signed-off-by: Cameron Esfahani 
---
 ui/vnc-enc-zrle.c |  4 ++--
 ui/vnc.c  | 11 +--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-enc-zrle.c b/ui/vnc-enc-zrle.c
index 17fd28a2e2..b4f71e32cf 100644
--- a/ui/vnc-enc-zrle.c
+++ b/ui/vnc-enc-zrle.c
@@ -98,8 +98,8 @@ static int zrle_compress_data(VncState *vs, int level)
 /* set pointers */
 zstream->next_in = vs->zrle->zrle.buffer;
 zstream->avail_in = vs->zrle->zrle.offset;
-zstream->next_out = vs->zrle->zlib.buffer + vs->zrle->zlib.offset;
-zstream->avail_out = vs->zrle->zlib.capacity - vs->zrle->zlib.offset;
+zstream->next_out = vs->zrle->zlib.buffer;
+zstream->avail_out = vs->zrle->zlib.capacity;
 zstream->data_type = Z_BINARY;
 
 /* start encoding */
diff --git a/ui/vnc.c b/ui/vnc.c
index 3e8d1f1207..1d7138a3a0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2071,8 +2071,15 @@ static void set_encodings(VncState *vs, int32_t 
*encodings, size_t n_encodings)
 break;
 #endif
 case VNC_ENCODING_ZLIB:
-vs->features |= VNC_FEATURE_ZLIB_MASK;
-vs->vnc_encoding = enc;
+/*
+ * VNC_ENCODING_ZRLE compresses better than VNC_ENCODING_ZLIB.
+ * So prioritize ZRLE, even if the client hints that it prefers
+ * ZLIB.
+ */
+if ((vs->features & VNC_FEATURE_ZRLE_MASK) == 0) {
+vs->features |= VNC_FEATURE_ZLIB_MASK;
+vs->vnc_encoding = enc;
+}
 break;
 case VNC_ENCODING_ZRLE:
 vs->features |= VNC_FEATURE_ZRLE_MASK;
-- 
2.24.0

[PATCH v2 0/2] vnc: fix VNC artifacts

[Cameron Esfahani via]

Remove VNC optimization to reencode framebuffer update as raw if it's
smaller than the default encoding.  QEMU's implementation was naive and
didn't account for the ZLIB z_stream mutating with each compression.  Just
saving and restoring the output buffer offset wasn't sufficient to "rewind"
the previous encoding.  Considering that ZRLE is never larger than raw and
even though ZLIB can occasionally be fractionally larger than raw, the
overhead of implementing this optimization correctly isn't worth it.

While debugging this, I noticed ZRLE always compresses better than ZLIB.
Prioritize ZRLE over ZLIB, even if the client hints that ZLIB is preferred.

Cameron Esfahani (2):
  vnc: fix VNC artifacts
  vnc: prioritize ZRLE compression over ZLIB

 ui/vnc-enc-zrle.c |  4 ++--
 ui/vnc.c  | 31 +++
 2 files changed, 13 insertions(+), 22 deletions(-)

-- 
2.24.0

[PATCH v2 1/2] vnc: fix VNC artifacts

[Cameron Esfahani via]

Patch de3f7de7f4e257ce44cdabb90f5f17ee99624557 was too simplistic in its
implementation: it didn't account for the ZLIB z_stream mutating with
each compression.  Because of the mutation, simply resetting the output
buffer's offset wasn't sufficient to "rewind" the operation.  The mutated
z_stream would generate future zlib blocks which referred to symbols in
past blocks which weren't sent.  This would lead to artifacting.

This reverts commit de3f7de7f4e257ce44cdabb90f5f17ee99624557.

Fixes:  ("vnc: allow fall back to RAW encoding")
Signed-off-by: Cameron Esfahani 
---
 ui/vnc.c | 20 ++--
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 4100d6e404..3e8d1f1207 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -898,8 +898,6 @@ int vnc_raw_send_framebuffer_update(VncState *vs, int x, 
int y, int w, int h)
 int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
 {
 int n = 0;
-bool encode_raw = false;
-size_t saved_offs = vs->output.offset;
 
 switch(vs->vnc_encoding) {
 case VNC_ENCODING_ZLIB:
@@ -922,24 +920,10 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int 
y, int w, int h)
 n = vnc_zywrle_send_framebuffer_update(vs, x, y, w, h);
 break;
 default:
-encode_raw = true;
+vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
+n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
 break;
 }
-
-/* If the client has the same pixel format as our internal buffer and
- * a RAW encoding would need less space fall back to RAW encoding to
- * save bandwidth and processing power in the client. */
-if (!encode_raw && vs->write_pixels == vnc_write_pixels_copy &&
-12 + h * w * VNC_SERVER_FB_BYTES <= (vs->output.offset - saved_offs)) {
-vs->output.offset = saved_offs;
-encode_raw = true;
-}
-
-if (encode_raw) {
-vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
-n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
-}
-
 return n;
 }
 
-- 
2.24.0

Re: [PATCH 2/2] aspeed/scu: Implement chip ID register

[Andrew Jeffery]

On Tue, 21 Jan 2020, at 12:03, Joel Stanley wrote:
> This returns a fixed but non-zero value for the chip id.
> 
> Signed-off-by: Joel Stanley 

Reviewed-by: Andrew Jeffery 
> ---
>  hw/misc/aspeed_scu.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 7108cad8c6a7..19d1780a40da 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -77,6 +77,8 @@
>  #define CPU2_BASE_SEG4   TO_REG(0x110)
>  #define CPU2_BASE_SEG5   TO_REG(0x114)
>  #define CPU2_CACHE_CTRL  TO_REG(0x118)
> +#define CHIP_ID0 TO_REG(0x150)
> +#define CHIP_ID1 TO_REG(0x154)
>  #define UART_HPLL_CLKTO_REG(0x160)
>  #define PCIE_CTRLTO_REG(0x180)
>  #define BMC_MMIO_CTRLTO_REG(0x184)
> @@ -115,6 +117,8 @@
>  #define AST2600_HW_STRAP2_PROTTO_REG(0x518)
>  #define AST2600_RNG_CTRL  TO_REG(0x524)
>  #define AST2600_RNG_DATA  TO_REG(0x540)
> +#define AST2600_CHIP_ID0  TO_REG(0x5B0)
> +#define AST2600_CHIP_ID1  TO_REG(0x5B4)
>  
>  #define AST2600_CLK TO_REG(0x40)
>  
> @@ -182,6 +186,8 @@ static const uint32_t 
> ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
>   [CPU2_BASE_SEG1]  = 0x8000U,
>   [CPU2_BASE_SEG4]  = 0x1E60U,
>   [CPU2_BASE_SEG5]  = 0xC000U,
> + [CHIP_ID0]= 0x1234ABCDU,
> + [CHIP_ID1]= 0xU,
>   [UART_HPLL_CLK]   = 0x1903U,
>   [PCIE_CTRL]   = 0x007BU,
>   [BMC_DEV_ID]  = 0x2402U
> @@ -307,6 +313,8 @@ static void aspeed_ast2500_scu_write(void *opaque, 
> hwaddr offset,
>  case RNG_DATA:
>  case FREE_CNTR4:
>  case FREE_CNTR4_EXT:
> +case CHIP_ID0:
> +case CHIP_ID1:
>  qemu_log_mask(LOG_GUEST_ERROR,
>"%s: Write to read-only offset 0x%" HWADDR_PRIx 
> "\n",
>__func__, offset);
> @@ -620,6 +628,8 @@ static void aspeed_ast2600_scu_write(void *opaque, 
> hwaddr offset,
>  case AST2600_RNG_DATA:
>  case AST2600_SILICON_REV:
>  case AST2600_SILICON_REV2:
> +case AST2600_CHIP_ID0:
> +case AST2600_CHIP_ID1:
>  /* Add read only registers here */
>  qemu_log_mask(LOG_GUEST_ERROR,
>"%s: Write to read-only offset 0x%" HWADDR_PRIx 
> "\n",
> @@ -648,6 +658,9 @@ static const uint32_t 
> ast2600_a0_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>  [AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
>  [AST2600_SDRAM_HANDSHAKE]   = 0x0040,  /* SoC completed DRAM 
> init */
>  [AST2600_HPLL_PARAM]= 0x1000405F,
> +[AST2600_CHIP_ID0]  = 0x1234ABCD,
> +[AST2600_CHIP_ID1]  = 0x,

Probably should add the explicit trailing 'U' to the constants at some point.

Reviewed-by: Andrew Jeffery 

Re: [PATCH 1/2] aspeed/scu: Create separate write callbacks

[Andrew Jeffery]

On Tue, 21 Jan 2020, at 12:03, Joel Stanley wrote:
> This splits the common write callback into separate ast2400 and ast2500
> implementations. This makes it clearer when implementing differing
> behaviour.
> 
> Signed-off-by: Joel Stanley 

Reviewed-by: Andrew Jeffery 

Re: [PATCH v8 5/6] hw/ppc/Kconfig: Enable TPM_SPAPR as part of PSERIES config

[David Gibson]

On Wed, Jan 08, 2020 at 11:10:11AM -0500, Stefan Berger wrote:
> From: Stefan Berger 
> 
> Signed-off-by: Stefan Berger 

Reviewed-by: David Gibson 

> ---
>  hw/ppc/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index f927ec9c74..b5b3519158 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -10,6 +10,7 @@ config PSERIES
>  select XICS_SPAPR
>  select XIVE_SPAPR
>  select MSI_NONBROKEN
> +select TPM_SPAPR
>  
>  config SPAPR_RNG
>  bool

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v8 4/6] tpm_spapr: Support suspend and resume

[David Gibson]

On Fri, Jan 17, 2020 at 05:46:21PM +0400, Marc-André Lureau wrote:
> On Fri, Jan 17, 2020 at 5:41 PM Stefan Berger  wrote:
> >
> > On 1/17/20 8:31 AM, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Wed, Jan 8, 2020 at 8:14 PM Stefan Berger  
> > > wrote:
> > >> From: Stefan Berger 
> > >>
> > >> Extend the tpm_spapr frontend with VM suspend and resume support.
> > >>
> > >> Signed-off-by: Stefan Berger 
> > >> ---
> > >>   hw/tpm/tpm_spapr.c  | 67 -
> > >>   hw/tpm/trace-events |  2 ++
> > >>   2 files changed, 68 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> > >> index ab184fbb82..cf5c7851e7 100644
> > >> --- a/hw/tpm/tpm_spapr.c
> > >> +++ b/hw/tpm/tpm_spapr.c
> > >> @@ -76,6 +76,9 @@ typedef struct {
> > >>
> > >>   unsigned char buffer[TPM_SPAPR_BUFFER_MAX];
> > >>
> > >> +uint32_t numbytes; /* number of bytes in suspend_buffer */
> > >> +unsigned char *suspend_buffer;
> > > Why do you need a copy suspend_buffer? Why not use and save buffer[] 
> > > directly?
> >
> >
> > This addresses David's comment:
> >
> > "Transferring the whole 4kiB buffer unconditionally when it mostly
> > won't have anything useful in it doesn't seem like a great idea."
> >
> > https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg02601.html
> 
> Oh ok.. (well really I don't think 4k (usually compressed) will really
> matter much in multi-gigabytes streams ;)

Probably not - though it is in the downtime portion of the stream.

But more to the point you can still make the size / whether you send
conditional on numbytes without having a whole separate buffer for it.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v8 2/6] spapr: Implement get_dt_compatible() callback

[David Gibson]

On Wed, Jan 08, 2020 at 11:10:08AM -0500, Stefan Berger wrote:
> From: Stefan Berger 
> 
> For devices that cannot be statically initialized, implement a
> get_dt_compatible() callback that allows us to ask the device for
> the 'compatible' value.
> 
> Signed-off-by: Stefan Berger 

Reviewed-by: David Gibson 

> ---
>  hw/ppc/spapr_vio.c | 11 +--
>  include/hw/ppc/spapr_vio.h |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 554de9930d..4b24b81797 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -87,6 +87,7 @@ static int vio_make_devnode(SpaprVioDevice *dev,
>  SpaprVioDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>  int vdevice_off, node_off, ret;
>  char *dt_name;
> +const char *dt_compatible;
>  
>  vdevice_off = fdt_path_offset(fdt, "/vdevice");
>  if (vdevice_off < 0) {
> @@ -113,9 +114,15 @@ static int vio_make_devnode(SpaprVioDevice *dev,
>  }
>  }
>  
> -if (pc->dt_compatible) {
> +if (pc->get_dt_compatible) {
> +dt_compatible = pc->get_dt_compatible(dev);
> +} else {
> +dt_compatible = pc->dt_compatible;
> +}
> +
> +if (dt_compatible) {
>  ret = fdt_setprop_string(fdt, node_off, "compatible",
> - pc->dt_compatible);
> + dt_compatible);
>  if (ret < 0) {
>  return ret;
>  }
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 72762ed16b..67f58b7f98 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -58,6 +58,7 @@ typedef struct SpaprVioDeviceClass {
>  void (*realize)(SpaprVioDevice *dev, Error **errp);
>  void (*reset)(SpaprVioDevice *dev);
>  int (*devnode)(SpaprVioDevice *dev, void *fdt, int node_off);
> +const char *(*get_dt_compatible)(SpaprVioDevice *dev);
>  } SpaprVioDeviceClass;
>  
>  struct SpaprVioDevice {

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 1/2] arm/virt/acpi: remove meaningless sub device "PR0" from PCI0

[Guoheyi]

Hi Julia,

Could you provide some comments or advice?

Thanks,

Heyi

在 2020/1/13 20:37, Igor Mammedov 写道:

On Thu, 19 Dec 2019 14:47:58 +0800
Heyi Guo  wrote:


The sub device "PR0" under PCI0 in ACPI/DSDT does not make any sense,
so simply remote it.

Could you make commit message more concrete so it would say
why it doesn't make any sense.

It seems to be there to describe root port,
I'd rather have PCI folk ack if it's ok to remove it.


Signed-off-by: Heyi Guo 

---
Cc: Peter Maydell 
Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 
Cc: Shannon Zhao 
Cc: qemu-...@nongnu.org
Cc: qemu-devel@nongnu.org
---
  hw/arm/virt-acpi-build.c  |   4 
  tests/data/acpi/virt/DSDT | Bin 18462 -> 18449 bytes
  tests/data/acpi/virt/DSDT.memhp   | Bin 19799 -> 19786 bytes
  tests/data/acpi/virt/DSDT.numamem | Bin 18462 -> 18449 bytes
  4 files changed, 4 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index bd5f771e9b..9f4c7d1889 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -317,10 +317,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
MemMapEntry *memmap,
  aml_append(method, aml_return(buf));
  aml_append(dev, method);
  
-Aml *dev_rp0 = aml_device("%s", "RP0");

-aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
-aml_append(dev, dev_rp0);
-
  Aml *dev_res0 = aml_device("%s", "RES0");
  aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
  crs = aml_resource_template();
diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index 
d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c
 100644
GIT binary patch
delta 39
vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)

delta 50
zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?^tF5;R%I
G{V4!>hYx%J

diff --git a/tests/data/acpi/virt/DSDT.memhp b/tests/data/acpi/virt/DSDT.memhp
index 
41ccc6431b917252bcbaac86c33b340c796be5ce..69ad844f65d047973a3e55198beecd45a35b8fce
 100644
GIT binary patch
delta 40
wcmcaUi}BPfMlP3Nmk=*s1_q}3iCof5t(P{ccXBfI+}XT|v(|RAjk`1(02g)*ivR!s

delta 51
zcmX>#i}Cs_MlP3NmymE@1_mbiiCof5O_w*ScXBdy-rc;3v(}c2J1D>)o+IATC1|sb
HyBr$;t7;Fc

diff --git a/tests/data/acpi/virt/DSDT.numamem 
b/tests/data/acpi/virt/DSDT.numamem
index 
d0f3afeb134fdf1c11f64cd06dbcdd30be603b80..b5895cb22446860a0b9be3d32ec856feb388be4c
 100644
GIT binary patch
delta 39
vcmbO?fpOvlMlP3Nmk>b@1_q`B6S<_Bdg?Z+cXBfI+}XT|v(|R9jr$`2@RSW)

delta 50
zcmbO@fpOjhMlP3Nmk>D*1_q{tiCof5o%I{lJ2{y;?{412S!>J19TZ>?^tF5;R%I
G{V4!>hYx%J



.

Re: [PATCH] spapr: Migrate CAS reboot flag

[David Gibson]

On Wed, Jan 15, 2020 at 07:10:47PM +0100, Cédric Le Goater wrote:
> On 1/15/20 6:48 PM, Greg Kurz wrote:
> > Migration can potentially race with CAS reboot. If the migration thread
> > completes migration after CAS has set spapr->cas_reboot but before the
> > mainloop could pick up the reset request and reset the machine, the
> > guest is migrated unrebooted and the destination doesn't reboot it
> > either because it isn't aware a CAS reboot was needed (eg, because a
> > device was added before CAS). This likely result in a broken or hung
> > guest.
> > 
> > Even if it is small, the window between CAS and CAS reboot is enough to
> > re-qualify spapr->cas_reboot as state that we should migrate. Add a new
> > subsection for that and always send it when a CAS reboot is pending.
> > This may cause migration to older QEMUs to fail but it is still better
> > than end up with a broken guest.
> > 
> > The destination cannot honour the CAS reboot request from a post load
> > handler because this must be done after the guest is fully restored.
> > It is thus done from a VM change state handler.
> > 
> > Reported-by: Lukáš Doktor 
> > Signed-off-by: Greg Kurz 
> 
> Cédric Le Goater 
> 
> Nice work ! That was quite complex to catch !

It is a very nice analysis.  However, I'm disinclined to merge this
for the time being.

My preferred approach would be to just eliminate CAS reboots
altogether, since that has other benefits.  I'm feeling like this
isn't super-urgent, since CAS reboots are extremely rare in practice,
now that we've eliminated the one for the irq switchover.

However, if it's not looking like we'll be ready to do that as the
qemu-5.0 release approaches, then I'll be more than willing to
reconsider this.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] spapr: Migrate CAS reboot flag

[David Gibson]

On Mon, Jan 20, 2020 at 09:04:38AM +0100, Greg Kurz wrote:
> On Fri, 17 Jan 2020 16:44:27 +0100
> Greg Kurz  wrote:
> 
> > On Fri, 17 Jan 2020 19:16:08 +1000
> > David Gibson  wrote:
> > 
> > > On Thu, Jan 16, 2020 at 07:29:02PM +0100, Greg Kurz wrote:
> > > > On Thu, 16 Jan 2020 13:14:35 +0100
> > > > Greg Kurz  wrote:
> > > > 
> > > > > On Thu, 16 Jan 2020 11:37:24 +0100
> > > > > Laurent Vivier  wrote:
> > > > > 
> > > > > > On 16/01/2020 09:48, Greg Kurz wrote:
> > > > > > > On Wed, 15 Jan 2020 19:10:37 +0100
> > > > > > > Laurent Vivier  wrote:
> > > > > > > 
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> On 15/01/2020 18:48, Greg Kurz wrote:
> > > > > > >>> Migration can potentially race with CAS reboot. If the 
> > > > > > >>> migration thread
> > > > > > >>> completes migration after CAS has set spapr->cas_reboot but 
> > > > > > >>> before the
> > > > > > >>> mainloop could pick up the reset request and reset the machine, 
> > > > > > >>> the
> > > > > > >>> guest is migrated unrebooted and the destination doesn't reboot 
> > > > > > >>> it
> > > > > > >>> either because it isn't aware a CAS reboot was needed (eg, 
> > > > > > >>> because a
> > > > > > >>> device was added before CAS). This likely result in a broken or 
> > > > > > >>> hung
> > > > > > >>> guest.
> > > > > > >>>
> > > > > > >>> Even if it is small, the window between CAS and CAS reboot is 
> > > > > > >>> enough to
> > > > > > >>> re-qualify spapr->cas_reboot as state that we should migrate. 
> > > > > > >>> Add a new
> > > > > > >>> subsection for that and always send it when a CAS reboot is 
> > > > > > >>> pending.
> > > > > > >>> This may cause migration to older QEMUs to fail but it is still 
> > > > > > >>> better
> > > > > > >>> than end up with a broken guest.
> > > > > > >>>
> > > > > > >>> The destination cannot honour the CAS reboot request from a 
> > > > > > >>> post load
> > > > > > >>> handler because this must be done after the guest is fully 
> > > > > > >>> restored.
> > > > > > >>> It is thus done from a VM change state handler.
> > > > > > >>>
> > > > > > >>> Reported-by: Lukáš Doktor 
> > > > > > >>> Signed-off-by: Greg Kurz 
> > > > > > >>> ---
> > > > > > >>>
> > > > > > >>
> > > > > > >> I'm wondering if the problem can be related with the fact that
> > > > > > >> main_loop_should_exit() could release qemu_global_mutex in
> > > > > > >> pause_all_vcpus() in the reset case?
> > > > > > >>
> > > > > > >> 1602 static bool main_loop_should_exit(void)
> > > > > > >> 1603 {
> > > > > > >> ...
> > > > > > >> 1633 request = qemu_reset_requested();
> > > > > > >> 1634 if (request) {
> > > > > > >> 1635 pause_all_vcpus();
> > > > > > >> 1636 qemu_system_reset(request);
> > > > > > >> 1637 resume_all_vcpus();
> > > > > > >> 1638 if (!runstate_check(RUN_STATE_RUNNING) &&
> > > > > > >> 1639 !runstate_check(RUN_STATE_INMIGRATE)) {
> > > > > > >> 1640 runstate_set(RUN_STATE_PRELAUNCH);
> > > > > > >> 1641 }
> > > > > > >> 1642 }
> > > > > > >> ...
> > > > > > >>
> > > > > > >> I already sent a patch for this kind of problem (in current Juan 
> > > > > > >> pull
> > > > > > >> request):
> > > > > > >>
> > > > > > >> "runstate: ignore finishmigrate -> prelaunch transition"
> > > > > > >>
> > > > > > > 
> > > > > > > IIUC your patch avoids an invalid 'prelaunch' -> 'postmigrate' 
> > > > > > > runstate
> > > > > > > transition that can happen if the migration thread sets the 
> > > > > > > runstate to
> > > > > > > 'finishmigrate' when pause_all_vcpus() releases the main loop 
> > > > > > > mutex.
> > > > > > > 
> > > > > > > ie. symptom of the problem is QEMU aborting, correct ? The issue 
> > > > > > > I'm
> > > > > > > trying to fix is a guest breakage caused by a discrepancy between
> > > > > > > QEMU and the guest after migration has succeeded.
> > > > > > > 
> > > > > > >> but I don't know if it could fix this one.
> > > > > > >>
> > > > > > > 
> > > > > > > I don't think so and your patch kinda illustrates it. If the 
> > > > > > > runstate
> > > > > > > is 'finishmigrate' when returning from pause_all_vcpus(), this 
> > > > > > > means
> > > > > > > that state was sent to the destination before we could actually 
> > > > > > > reset
> > > > > > > the machine.
> > > > > > 
> > > > > > Yes, you're right.
> > > > > > 
> > > > > > But the question behind my comment was: is it expected to have a 
> > > > > > pending
> > > > > > reset while we are migrating?
> > > > > > 
> > > > > 
> > > > > Nothing prevents qemu_system_reset_request() to be called when 
> > > > > migration
> > > > > is active. 
> > > > > 
> > > > > > Perhaps H_CAS can return H_BUSY and wait the end of the migration 
> > > > > > and
> > > > > > then be fully executed on destination?
> > > > > > 
> > > > > 
> > > > > And so we would need to teach SLOF to try H_CAS again until it stops
> > > > > returning H_BUSY ? It seems safer to migrate the CAS reboot flag IMHO.
> > > > > 

Re: [PATCH v2 0/2] ppc: add support for Directed Privileged Doorbell (non-hypervisor)

[David Gibson]

On Mon, Jan 20, 2020 at 11:49:33AM +0100, Cédric Le Goater wrote:
> Hello,
> 
> The Processor Control facility for POWER8 processors and later
> provides a mechanism for the hypervisor to send messages to other
> threads in the system (msgsnd instruction) and cause hypervisor-level
> exceptions.
> 
> Privileged non-hypervisor programs can also send messages (msgsndp
> instruction) but are restricted to the threads of the same
> subprocessor and cause privileged-level exceptions. The Directed
> Privileged Doorbell Exception State (DPDES) register reflects the
> state of pending privileged-level doorbell exceptions for all threads
> and can be used to modify that state.
> 
> If the MSGP facility is not in the HFSCR, a hypervisor facility
> unavailable exception is generated when these instructions are used or
> when the DPDES register is accessed by the supervisor.
> 
> Based on previous work from Suraj Jitindar Singh.

Applied to ppc-for-5.0, thanks.

> 
> Thanks,
> 
> C.
> 
> Changes since v1:
> 
>  - removed DBELL_TIRTAG_MASK and simplified helpers as QEMU TCG
>doesn't support more than on thread per core   
>  - simplified book3s_dbell2irq() and renamed it to dbell_type_server() 
>  - replaced mask LOG_GUEST_ERROR by CPU_LOG_INT to track HV Facility
>errors
>  
> Cédric Le Goater (2):
>   target/ppc: Add privileged message send facilities
>   target/ppc: add support for Hypervisor Facility Unavailable Exception
> 
>  target/ppc/cpu.h|  6 +++
>  target/ppc/helper.h |  4 ++
>  target/ppc/excp_helper.c| 79 ++---
>  target/ppc/misc_helper.c| 63 ++
>  target/ppc/translate.c  | 26 +++
>  target/ppc/translate_init.inc.c | 20 +++--
>  6 files changed, 178 insertions(+), 20 deletions(-)
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v6 3/3] target/ppc: support single stepping with KVM HV

[David Gibson]

On Mon, Jan 20, 2020 at 05:11:50PM -0300, Fabiano Rosas wrote:
> David Gibson  writes:
> 
> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >> index 103bfe9dc2..b69f8565aa 100644
> >> --- a/target/ppc/cpu.h
> >> +++ b/target/ppc/cpu.h
> >> @@ -440,6 +440,10 @@ typedef struct ppc_v3_pate_t {
> >>  #define msr_ts   ((env->msr >> MSR_TS1)  & 3)
> >>  #define msr_tm   ((env->msr >> MSR_TM)   & 1)
> >>  
> >> +#define srr1_ir ((env->spr[SPR_SRR1] >> MSR_IR) & 1)
> >> +#define srr1_dr ((env->spr[SPR_SRR1] >> MSR_DR) & 1)
> >> +#define srr1_se ((env->spr[SPR_SRR1] >> MSR_SE) & 1)
> >
> > I'd prefer not to introduce these.  The msr_xx macros are already kind
> > of dubious because they assume the meaning of 'env' in the context
> > they're used.  I'm ok to use them because they're so useful, so
> > often.  These srr1 variants however are used in far fewer situations.
> > So, I'd prefer to just open code these as (env->spr[SPR_SRR1] &
> > MSR_IR) in the relatively few places they're used for now.
> >
> 
> Ok. No problem.
> 
> >>  #define DBCR0_ICMP (1 << 27)
> >>  #define DBCR0_BRT (1 << 26)
> >>  #define DBSR_ICMP (1 << 27)
> >> @@ -1158,6 +1162,16 @@ struct CPUPPCState {
> >>  uint32_t tm_vscr;
> >>  uint64_t tm_dscr;
> >>  uint64_t tm_tar;
> >> +
> >> +/* Used for software single step */
> >> +target_ulong sstep_srr0;
> >> +target_ulong sstep_srr1;
> >> +target_ulong sstep_insn;
> >> +target_ulong trace_handler_addr;
> >> +int sstep_kind;
> >
> > Won't you need to migrate this extra state, at least some of the time?
> >
> 
> Ah. I haven't looked into this yet. Will do that for the next
> version. Need to learn a bit about migration first.
> 
> >> +#define SSTEP_REGULAR 0
> >> +#define SSTEP_PENDING 1
> >> +#define SSTEP_GUEST   2
> >
> > Some comments on what these modes mean would be useful.
> >
> 
> Ok.
> 
> >> +static uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr)
> >> +{
> >> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +CPUPPCState *env = >env;
> >> +uint32_t insn;
> >> +
> >> +cpu_memory_rw_debug(cs, addr, (uint8_t *), sizeof(insn), 0);
> >> +
> >> +if (msr_le) {
> >> +return ldl_le_p();
> >> +} else {
> >> +return ldl_be_p();
> >> +}
> >
> > I think you can just use cpu_ldl_code() for this.
> >
> 
> I'll look into it.
> 
> >> +static void kvm_insert_singlestep_breakpoint(CPUState *cs, bool mmu_on)
> >> +{
> >> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +CPUPPCState *env = >env;
> >> +target_ulong bp_addr;
> >> +target_ulong saved_msr = env->msr;
> >> +
> >> +bp_addr = ppc_get_trace_int_handler_addr(cs, mmu_on);
> >> +if (env->nip == bp_addr) {
> >> +/*
> >> + * We are trying to step the interrupt handler address itself;
> >> + * move the breakpoint to the next instruction.
> >> + */
> >> +bp_addr += 4;
> >
> > What if the first instruction of the interrupt handler is a branch?
> >
> 
> Well, I need to displace the breakpoint somehow. I don't think I can
> handle this particular case without having _some_ knowledge of the
> handler's code. The ones I've seen so far don't have a branch as first
> instruction.

So, at least for unconditional branches it's possible - you could
parse the instruction and place your breakpoint on the target.  For
conditional branches it would be much harder, but they are much less
likely as the very first instruction on the interrupt vector.

I do think we should at least detect and flag some sort of error in
this situation though.

> >> +}
> >> +
> >> +/*
> >> + * The actual access by the guest might be made in a mode
> >> + * different than we are now (due to rfid) so temporarily set the
> >> + * MSR to reflect that during the breakpoint placement.
> >> + *
> >> + * Note: we need this because breakpoints currently use
> >> + * cpu_memory_rw_debug, which does the memory accesses from the
> >> + * guest point of view.
> >> + */
> >> +if ((msr_ir & msr_dr) != mmu_on) {
> >
> > Should be msr_ir && msr_dr - you only get away with bitwise and by
> > accident.
> >
> 
> Ack.
> 
> >> +if (mmu_on) {
> >> +env->msr |= (1ULL << MSR_IR | 1ULL << MSR_DR);
> >> +} else {
> >> +env->msr &= ~(1ULL << MSR_IR | 1ULL << MSR_DR);
> >> +}
> >> +}
> >
> > Wouldn't it be simpler to unconditionally set env->msr based on
> > mmu_on.
> >
> 
> Yes.
> 
> >> +
> >> +kvm_insert_breakpoint(cs, bp_addr, 4, GDB_BREAKPOINT_SW);
> >
> > Hrm I don't actually see how changing env->msr helps you here.
> > AFAICT if kvm_insert_breakpoint() resolves to kvm_arch_sw_breakpoint()
> > it doesn't rely on the MSR value at all.  If it resolves to
> > kvm_arch_hw_breakpoint(), then it looks like it just stashes
> > information to be pushed into KVM when we re-enter the guest.  None of
> > the information stashed appears to depend on the current MSR, and once
> > we 

Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls

[Richard Henderson]

On 1/20/20 1:48 AM, Alex Bennée wrote:
>> +default:
>> +sigsegv:
> 
> this label looks a little extraneous.
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée 
> 

Look a little further down:

> +default:
> +sigsegv:
> +/* Like force_sig(SIGSEGV).  */
> +gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
> +return;
> +}
> +
> +/*
> + * Validate the return address.
> + * Note that the kernel treats this the same as an invalid entry point.
> + */
> +if (get_user_u64(caller, env->regs[R_ESP])) {
> +goto sigsegv;
> +}


r~

Re: [PATCH v4 1/7] hw/arm/raspi: Remove obsolete use of -smp to set the soc 'enabled-cpus'

[Alistair Francis]

On Tue, Jan 21, 2020 at 9:53 AM Philippe Mathieu-Daudé  wrote:
>
> Since we enabled parallel TCG code generation for softmmu (see
> commit 3468b59 "tcg: enable multiple TCG contexts in softmmu")
> and its subsequent fix (commit 72649619 "add .min_cpus and
> .default_cpus fields to machine_class"), the raspi machines are
> restricted to always use their 4 cores:
>
> See in hw/arm/raspi2 (with BCM283X_NCPUS set to 4):
>
>   222 static void raspi2_machine_init(MachineClass *mc)
>   223 {
>   224 mc->desc = "Raspberry Pi 2";
>   230 mc->max_cpus = BCM283X_NCPUS;
>   231 mc->min_cpus = BCM283X_NCPUS;
>   232 mc->default_cpus = BCM283X_NCPUS;
>   235 };
>   236 DEFINE_MACHINE("raspi2", raspi2_machine_init)
>
> We can no longer use the -smp option, as we get:
>
>   $ qemu-system-arm -M raspi2 -smp 1
>   qemu-system-arm: Invalid SMP CPUs 1. The min CPUs supported by machine 
> 'raspi2' is 4
>
> Since we can not set the TYPE_BCM283x SOC "enabled-cpus" with -smp,
> remove the unuseful code.
>
> We can achieve the same by using the '-global bcm2836.enabled-cpus=1'
> option.
>
> Reported-by: Laurent Bonnans 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
> Cc: Emilio G. Cota 
> Cc: Richard Henderson 
> Cc: Andrew Baumann 
> Cc: Eduardo Habkost 
> ---
>  hw/arm/raspi.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 6a510aafc1..3996f6c63a 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -192,8 +192,6 @@ static void raspi_init(MachineState *machine, int version)
>  /* Setup the SOC */
>  object_property_add_const_link(OBJECT(>soc), "ram", OBJECT(>ram),
> _abort);
> -object_property_set_int(OBJECT(>soc), machine->smp.cpus, 
> "enabled-cpus",
> -_abort);
>  int board_rev = version == 3 ? 0xa02082 : 0xa21041;
>  object_property_set_int(OBJECT(>soc), board_rev, "board-rev",
>  _abort);
> --
> 2.21.1
>
>

[PATCH v4 08/11] 9pfs: readdir benchmark

[Christian Schoenebeck]

This patch is not intended to be merged. It just provides a
temporary benchmark foundation for coneniently A/B comparison
of the subsequent 9p readdir optimization patches:

* hw/9pfs/9p-synth: increase amount of simulated files for
  readdir test to 2000 files.

* tests/virtio-9p: measure wall time that elapsed between
  sending T_readdir request and arrival of R_readdir response
  and print out that measured duration, as well as amount of
  directory entries received, and the amount of bytes of the
  response message.

* tests/virtio-9p: increased msize to 256kiB to allow
  retrieving all 2000 files (simulated by 9pfs synth driver)
  with only one T_readdir request.

Running this benchmark is fairly quick & simple and does not
require any guest OS installation or other prerequisites:

cd build
make && make tests/qtest/qos-test
export QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
tests/qtest/qos-test -p $(tests/qtest/qos-test -l | grep readdir/basic)

Since this benchmark uses the 9pfs synth driver, the host
machine's I/O hardware (SSDs/HDDs) is not relevant for the
benchmark result, because the synth backend's readdir
implementation returns immediately (without any blocking I/O
that would incur with a real-life fs driver) and just returns
already prepared, simulated directory entries directly from RAM.
So this benchmark focuses on the efficiency of the 9pfs
controller code (or top half) for readdir request handling.

Signed-off-by: Christian Schoenebeck 
---
 hw/9pfs/9p-synth.h   |  2 +-
 tests/qtest/virtio-9p-test.c | 37 +++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-synth.h b/hw/9pfs/9p-synth.h
index 036d7e4a5b..7d6cedcdac 100644
--- a/hw/9pfs/9p-synth.h
+++ b/hw/9pfs/9p-synth.h
@@ -58,7 +58,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
 /* for READDIR test */
 #define QTEST_V9FS_SYNTH_READDIR_DIR "ReadDirDir"
 #define QTEST_V9FS_SYNTH_READDIR_FILE "ReadDirFile%d"
-#define QTEST_V9FS_SYNTH_READDIR_NFILES 100
+#define QTEST_V9FS_SYNTH_READDIR_NFILES 2000
 
 /* Any write to the "FLUSH" file is handled one byte at a time by the
  * backend. If the byte is zero, the backend returns success (ie, 1),
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index e47b286340..d71b37aa6c 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -15,6 +15,18 @@
 #include "libqos/virtio-9p.h"
 #include "libqos/qgraph.h"
 
+/*
+ * to benchmark the real time (not CPU time) that elapsed between start of
+ * a request and arrival of its response
+ */
+static double wall_time(void)
+{
+struct timeval t;
+struct timezone tz;
+gettimeofday(, );
+return t.tv_sec + t.tv_usec * 0.01;
+}
+
 #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
 static QGuestAllocator *alloc;
 
@@ -36,7 +48,7 @@ static void pci_config(void *obj, void *data, QGuestAllocator 
*t_alloc)
 g_free(tag);
 }
 
-#define P9_MAX_SIZE 4096 /* Max size of a T-message or R-message */
+#define P9_MAX_SIZE (256 * 1024) /* Max size of a T-message or R-message */
 
 typedef struct {
 QTestState *qts;
@@ -600,12 +612,35 @@ static void fs_readdir(void *obj, void *data, 
QGuestAllocator *t_alloc)
 v9fs_req_wait_for_reply(req, NULL);
 v9fs_rlopen(req, , NULL);
 
+const double start = wall_time();
+
 /*
  * submit count = msize - 11, because 11 is the header size of Rreaddir
  */
 req = v9fs_treaddir(v9p, 1, 0, P9_MAX_SIZE - 11, 0);
+const double treaddir = wall_time();
 v9fs_req_wait_for_reply(req, NULL);
+const double waitforreply = wall_time();
 v9fs_rreaddir(req, , , );
+const double end = wall_time();
+
+printf("\nTime client spent on sending T_readdir: %fs\n\n",
+   treaddir - start);
+
+printf("Time client spent for waiting for reply from server: %fs "
+   "[MOST IMPORTANT]\n", waitforreply - start);
+printf("(This is the most important value, because it reflects the time\n"
+   "the 9p server required to process and return the result of the\n"
+   "T_readdir request.)\n\n");
+
+printf("Total client time: %fs\n", end - start);
+printf("(NOTE: this time is not relevant; this huge time comes from\n"
+   "inefficient qtest_memread() calls. So you can discard this\n"
+   "value as a problem of this test client implementation while\n"
+   "processing the received server T_readdir reply.)\n\n");
+
+printf("Details of response message data: R_readddir nentries=%d "
+   "rbytes=%d\n", nentries, count);
 
 /*
  * Assuming msize (P9_MAX_SIZE) is large enough so we can retrieve all
-- 
2.20.1

[PATCH v4 07/11] tests/virtio-9p: failing splitted readdir test

[Christian Schoenebeck]

This patch is not intended to be merged. It resembles
an issue (with debug messages) where the splitted
readdir test fails because server is interrupted with
transport error "Failed to decode VirtFS request type 40",
which BTW fails both with the unoptimized and with the
optimized 9p readdir code.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 8b0d94546e..e47b286340 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -647,13 +647,14 @@ static void fs_readdir_split(void *obj, void *data, 
QGuestAllocator *t_alloc)
 int fid;
 uint64_t offset;
 /* the Treaddir 'count' parameter values to be tested */
-const uint32_t vcount[] = { 512, 256 };
+const uint32_t vcount[] = { 512, 256, 128 };
 const int nvcount = sizeof(vcount) / sizeof(uint32_t);
 
 fs_attach(v9p, NULL, t_alloc);
 
 /* iterate over all 'count' parameter values to be tested with Treaddir */
 for (subtest = 0; subtest < nvcount; ++subtest) {
+printf("\nsubtest[%d] with count=%d\n", subtest, vcount[subtest]);
 fid = subtest + 1;
 offset = 0;
 entries = NULL;
@@ -674,12 +675,16 @@ static void fs_readdir_split(void *obj, void *data, 
QGuestAllocator *t_alloc)
  * entries
  */
 while (true) {
+printf("\toffset=%ld\n", offset);
 npartialentries = 0;
 partialentries = NULL;
 
+printf("Treaddir fid=%d offset=%ld count=%d\n",
+   fid, offset, vcount[subtest]);
 req = v9fs_treaddir(v9p, fid, offset, vcount[subtest], 0);
 v9fs_req_wait_for_reply(req, NULL);
 v9fs_rreaddir(req, , , );
+printf("\t\tnpartial=%d nentries=%d\n", npartialentries, nentries);
 if (npartialentries > 0 && partialentries) {
 if (!entries) {
 entries = partialentries;
@@ -716,6 +721,8 @@ static void fs_readdir_split(void *obj, void *data, 
QGuestAllocator *t_alloc)
 }
 
 v9fs_free_dirents(entries);
+
+printf("PASSED subtest[%d]\n", subtest);
 }
 
 g_free(wnames[0]);
-- 
2.20.1

[PATCH v4 03/11] 9pfs: validate count sent by client with T_readdir

[Christian Schoenebeck]

A good 9p client sends T_readdir with "count" parameter that's sufficiently
smaller than client's initially negotiated msize (maximum message size).
We perform a check for that though to avoid the server to be interrupted
with a "Failed to encode VirtFS reply type 41" transport error message by
bad clients. This count value constraint uses msize - 11, because 11 is the
header size of R_readdir.

Signed-off-by: Christian Schoenebeck 
---
 hw/9pfs/9p.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a5fbe821d4..18370183c4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2426,6 +2426,7 @@ static void coroutine_fn v9fs_readdir(void *opaque)
 int32_t count;
 uint32_t max_count;
 V9fsPDU *pdu = opaque;
+V9fsState *s = pdu->s;
 
 retval = pdu_unmarshal(pdu, offset, "dqd", ,
_offset, _count);
@@ -2434,6 +2435,13 @@ static void coroutine_fn v9fs_readdir(void *opaque)
 }
 trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset, max_count);
 
+if (max_count > s->msize - 11) {
+max_count = s->msize - 11;
+warn_report_once(
+"9p: bad client: T_readdir with count > msize - 11"
+);
+}
+
 fidp = get_fid(pdu, fid);
 if (fidp == NULL) {
 retval = -EINVAL;
-- 
2.20.1

[PATCH v4 02/11] 9pfs: require msize >= 4096

[Christian Schoenebeck]

A client establishes a session by sending a Tversion request along with a
'msize' parameter which client uses to suggest server a maximum message
size ever to be used for communication (for both requests and replies)
between client and server during that session. If client suggests a 'msize'
smaller than 4096 then deny session by server immediately with an error
response (Rlerror for "9P2000.L" clients or Rerror for "9P2000.u" clients)
instead of replying with Rversion.

So far any msize submitted by client with Tversion was simply accepted by
server without any check. Introduction of some minimum msize makes sense,
because e.g. a msize < 7 would not allow any subsequent 9p operation at
all, because 7 is the size of the header section common by all 9p message
types.

A substantial higher value of 4096 was chosen though to prevent potential
issues with some message types. E.g. Rreadlink may yield up to a size of
PATH_MAX which is usually 4096, and like almost all 9p message types,
Rreadlink is not allowed to be truncated by the 9p protocol. This chosen
size also prevents a similar issue with Rreaddir responses (provided client
always sends adequate 'count' parameter with Treaddir), because even though
directory entries retrieval may be split up over several T/Rreaddir
messages; a Rreaddir response must not truncate individual directory entries
though. So msize should be large enough to return at least one directory
entry with the longest possible file name supported by host. Most file
systems support a max. file name length of 255. Largest known file name
lenght limit would be currently ReiserFS with max. 4032 bytes, which is
also covered by this min. msize value because 4032 + 35 < 4096.

Furthermore 4096 is already the minimum msize of the Linux kernel's 9pfs
client.

Signed-off-by: Christian Schoenebeck 
Reviewed-by: Greg Kurz 
---
 hw/9pfs/9p.c | 12 
 hw/9pfs/9p.h | 11 +++
 2 files changed, 23 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 520177f40c..a5fbe821d4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1363,8 +1363,20 @@ static void coroutine_fn v9fs_version(void *opaque)
 s->proto_version = V9FS_PROTO_2000L;
 } else {
 v9fs_string_sprintf(, "unknown");
+/* skip min. msize check, reporting invalid version has priority */
+goto marshal;
 }
 
+if (s->msize < P9_MIN_MSIZE) {
+err = -EMSGSIZE;
+error_report(
+"9pfs: Client requested msize < minimum msize ("
+stringify(P9_MIN_MSIZE) ") supported by this server."
+);
+goto out;
+}
+
+marshal:
 err = pdu_marshal(pdu, offset, "ds", s->msize, );
 if (err < 0) {
 goto out;
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 3904f82901..6fffe44f5a 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -100,6 +100,17 @@ typedef enum P9ProtoVersion {
 V9FS_PROTO_2000L = 0x02,
 } P9ProtoVersion;
 
+/**
+ * @brief Minimum message size supported by this 9pfs server.
+ *
+ * A client establishes a session by sending a Tversion request along with a
+ * 'msize' parameter which suggests the server a maximum message size ever to 
be
+ * used for communication (for both requests and replies) between client and
+ * server during that session. If client suggests a 'msize' smaller than this
+ * value then session is denied by server with an error response.
+ */
+#define P9_MIN_MSIZE4096
+
 #define P9_NOTAGUINT16_MAX
 #define P9_NOFIDUINT32_MAX
 #define P9_MAXWELEM 16
-- 
2.20.1

[PATCH v4 06/11] tests/virtio-9p: added splitted readdir test

[Christian Schoenebeck]

The previous, already existing readdir test simply used a 'count'
parameter big enough to retrieve all directory entries with a
single Treaddir request.

In this new 'splitted' readdir test, directory entries are
retrieved, splitted over several Treaddir requests by picking small
'count' parameters which force the server to truncate the response.
So the test client sends as many Treaddir requests as necessary to
get all directory entries. Currently this test covers actually two
tests: a sequence of Treaddir requests with count=512 and then a
subsequent test with a sequence of Treaddir requests with count=256.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 91 
 1 file changed, 91 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 2167322985..8b0d94546e 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -578,6 +578,7 @@ static bool fs_dirents_contain_name(struct V9fsDirent *e, 
const char* name)
 return false;
 }
 
+/* basic readdir test where reply fits into a single response message */
 static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 {
 QVirtio9P *v9p = obj;
@@ -631,6 +632,95 @@ static void fs_readdir(void *obj, void *data, 
QGuestAllocator *t_alloc)
 g_free(wnames[0]);
 }
 
+/* readdir test where overall request is splitted over several messages */
+static void fs_readdir_split(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
+uint16_t nqid;
+v9fs_qid qid;
+uint32_t count, nentries, npartialentries;
+struct V9fsDirent *entries, *tail, *partialentries;
+P9Req *req;
+int subtest;
+int fid;
+uint64_t offset;
+/* the Treaddir 'count' parameter values to be tested */
+const uint32_t vcount[] = { 512, 256 };
+const int nvcount = sizeof(vcount) / sizeof(uint32_t);
+
+fs_attach(v9p, NULL, t_alloc);
+
+/* iterate over all 'count' parameter values to be tested with Treaddir */
+for (subtest = 0; subtest < nvcount; ++subtest) {
+fid = subtest + 1;
+offset = 0;
+entries = NULL;
+nentries = 0;
+tail = NULL;
+
+req = v9fs_twalk(v9p, 0, fid, 1, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, , NULL);
+g_assert_cmpint(nqid, ==, 1);
+
+req = v9fs_tlopen(v9p, fid, O_DIRECTORY, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlopen(req, , NULL);
+
+/*
+ * send as many Treaddir requests as required to get all directory
+ * entries
+ */
+while (true) {
+npartialentries = 0;
+partialentries = NULL;
+
+req = v9fs_treaddir(v9p, fid, offset, vcount[subtest], 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rreaddir(req, , , );
+if (npartialentries > 0 && partialentries) {
+if (!entries) {
+entries = partialentries;
+nentries = npartialentries;
+tail = partialentries;
+} else {
+tail->next = partialentries;
+nentries += npartialentries;
+}
+while (tail->next) {
+tail = tail->next;
+}
+offset = tail->offset;
+} else {
+break;
+}
+}
+
+g_assert_cmpint(
+nentries, ==,
+QTEST_V9FS_SYNTH_READDIR_NFILES + 2 /* "." and ".." */
+);
+
+/*
+ * Check all file names exist in returned entries, ignore their order
+ * though.
+ */
+g_assert_cmpint(fs_dirents_contain_name(entries, "."), ==, true);
+g_assert_cmpint(fs_dirents_contain_name(entries, ".."), ==, true);
+for (int i = 0; i < QTEST_V9FS_SYNTH_READDIR_NFILES; ++i) {
+char *name = g_strdup_printf(QTEST_V9FS_SYNTH_READDIR_FILE, i);
+g_assert_cmpint(fs_dirents_contain_name(entries, name), ==, true);
+g_free(name);
+}
+
+v9fs_free_dirents(entries);
+}
+
+g_free(wnames[0]);
+}
+
 static void fs_walk_no_slash(void *obj, void *data, QGuestAllocator *t_alloc)
 {
 QVirtio9P *v9p = obj;
@@ -810,6 +900,7 @@ static void register_virtio_9p_test(void)
 qos_add_test("fs/flush/ignored", "virtio-9p", fs_flush_ignored,
  NULL);
 qos_add_test("fs/readdir/basic", "virtio-9p", fs_readdir, NULL);
+qos_add_test("fs/readdir/split", "virtio-9p", fs_readdir_split, NULL);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.20.1

[PATCH v4 09/11] hw/9pfs/9p-synth: avoid n-square issue in synth_readdir()

[Christian Schoenebeck]

This patch is just a temporary benchmark hack, not intended
to be merged!

9pfs synth driver's readdir() implementation has a severe
n-square performance problem. This patch is a quick and dirty
hack to prevent that performance problem from tainting the
readdir() benchmark results. In its current form, this patch
is not useful for anything else than for an isolated readdir
benchmark.

NOTE: This patch would break the new readdir/split test,
because it would alter the behaviour of seekdir() required
for retrieving directory entries splitted over several
requests.

Signed-off-by: Christian Schoenebeck 
---
 hw/9pfs/9p-synth.c | 29 ++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 7eb210ffa8..54dc30f37b 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -225,7 +225,8 @@ static void synth_direntry(V9fsSynthNode *node,
 }
 
 static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
-struct dirent *entry, off_t off)
+   struct dirent *entry, off_t off,
+   V9fsSynthNode **hack)
 {
 int i = 0;
 V9fsSynthNode *node;
@@ -243,16 +244,38 @@ static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
 /* end of directory */
 return NULL;
 }
+*hack = node;
 synth_direntry(node, entry, off);
 return entry;
 }
 
 static struct dirent *synth_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-struct dirent *entry;
+struct dirent *entry = NULL;
 V9fsSynthOpenState *synth_open = fs->private;
 V9fsSynthNode *node = synth_open->node;
-entry = synth_get_dentry(node, _open->dent, synth_open->offset);
+
+/*
+ * HACK: This is just intended for benchmark, to avoid severe n-square
+ * performance problem of synth driver's readdir implementation here which
+ * would otherwise unncessarily taint the benchmark results. By simply
+ * caching (globally) the previous node (of the previous synth_readdir()
+ * call) we can simply proceed to next node in chained list efficiently.
+ *
+ * not a good idea for any production code ;-)
+ */
+static struct V9fsSynthNode *cachedNode;
+
+if (!cachedNode) {
+entry = synth_get_dentry(node, _open->dent, synth_open->offset,
+ );
+} else {
+cachedNode = cachedNode->sibling.le_next;
+if (cachedNode) {
+entry = _open->dent;
+synth_direntry(cachedNode, entry, synth_open->offset + 1);
+}
+}
 if (entry) {
 synth_open->offset++;
 }
-- 
2.20.1

[PATCH v4 01/11] tests/virtio-9p: add terminating null in v9fs_string_read()

[Christian Schoenebeck]

The 9p protocol sends strings in general without null termination
over the wire. However for future use of this functions it is
beneficial for the delivered string to be null terminated though
for being able to use the string with standard C functions which
often rely on strings being null terminated.

Signed-off-by: Christian Schoenebeck 
Reviewed-by: Greg Kurz 
---
 tests/qtest/virtio-9p-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index e7b58e3a0c..06263edb53 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -130,8 +130,9 @@ static void v9fs_string_read(P9Req *req, uint16_t *len, 
char **string)
 *len = local_len;
 }
 if (string) {
-*string = g_malloc(local_len);
+*string = g_malloc(local_len + 1);
 v9fs_memread(req, *string, local_len);
+(*string)[local_len] = 0;
 } else {
 v9fs_memskip(req, local_len);
 }
-- 
2.20.1

[PATCH v4 05/11] tests/virtio-9p: added readdir test

[Christian Schoenebeck]

The first readdir test simply checks the amount of directory
entries returned by 9pfs server, according to the created amount
of virtual files on 9pfs synth driver side. Then the subsequent
readdir test also checks whether all directory entries have the
expected file names (as created on 9pfs synth driver side),
ignoring their precise order in result list though.

Signed-off-by: Christian Schoenebeck 
---
 tests/qtest/virtio-9p-test.c | 152 +++
 1 file changed, 152 insertions(+)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 06263edb53..2167322985 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -68,6 +68,11 @@ static void v9fs_memread(P9Req *req, void *addr, size_t len)
 req->r_off += len;
 }
 
+static void v9fs_uint8_read(P9Req *req, uint8_t *val)
+{
+v9fs_memread(req, val, 1);
+}
+
 static void v9fs_uint16_write(P9Req *req, uint16_t val)
 {
 uint16_t le_val = cpu_to_le16(val);
@@ -101,6 +106,12 @@ static void v9fs_uint32_read(P9Req *req, uint32_t *val)
 le32_to_cpus(val);
 }
 
+static void v9fs_uint64_read(P9Req *req, uint64_t *val)
+{
+v9fs_memread(req, val, 8);
+le64_to_cpus(val);
+}
+
 /* len[2] string[len] */
 static uint16_t v9fs_string_size(const char *string)
 {
@@ -191,6 +202,7 @@ static const char *rmessage_name(uint8_t id)
 id == P9_RLOPEN ? "RLOPEN" :
 id == P9_RWRITE ? "RWRITE" :
 id == P9_RFLUSH ? "RFLUSH" :
+id == P9_RREADDIR ? "READDIR" :
 "";
 }
 
@@ -348,6 +360,82 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid, 
v9fs_qid **wqid)
 v9fs_req_free(req);
 }
 
+/* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
+static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t offset,
+uint32_t count, uint16_t tag)
+{
+P9Req *req;
+
+req = v9fs_req_init(v9p, 4 + 8 + 4, P9_TREADDIR, tag);
+v9fs_uint32_write(req, fid);
+v9fs_uint64_write(req, offset);
+v9fs_uint32_write(req, count);
+v9fs_req_send(req);
+return req;
+}
+
+struct V9fsDirent {
+v9fs_qid qid;
+uint64_t offset;
+uint8_t type;
+char *name;
+struct V9fsDirent *next;
+};
+
+/* size[4] Rreaddir tag[2] count[4] data[count] */
+static void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t *nentries,
+  struct V9fsDirent **entries)
+{
+uint32_t local_count;
+struct V9fsDirent *e = NULL;
+uint16_t slen;
+uint32_t n = 0;
+
+v9fs_req_recv(req, P9_RREADDIR);
+v9fs_uint32_read(req, _count);
+
+if (count) {
+*count = local_count;
+}
+
+for (int32_t togo = (int32_t)local_count;
+ togo >= 13 + 8 + 1 + 2;
+ togo -= 13 + 8 + 1 + 2 + slen, ++n)
+{
+if (!e) {
+e = g_malloc(sizeof(struct V9fsDirent));
+if (entries) {
+*entries = e;
+}
+} else {
+e = e->next = g_malloc(sizeof(struct V9fsDirent));
+}
+e->next = NULL;
+/* qid[13] offset[8] type[1] name[s] */
+v9fs_memread(req, >qid, 13);
+v9fs_uint64_read(req, >offset);
+v9fs_uint8_read(req, >type);
+v9fs_string_read(req, , >name);
+}
+
+if (nentries) {
+*nentries = n;
+}
+
+v9fs_req_free(req);
+}
+
+static void v9fs_free_dirents(struct V9fsDirent *e)
+{
+struct V9fsDirent *next = NULL;
+
+for (; e; e = next) {
+next = e->next;
+g_free(e->name);
+g_free(e);
+}
+}
+
 /* size[4] Tlopen tag[2] fid[4] flags[4] */
 static P9Req *v9fs_tlopen(QVirtio9P *v9p, uint32_t fid, uint32_t flags,
   uint16_t tag)
@@ -480,6 +568,69 @@ static void fs_walk(void *obj, void *data, QGuestAllocator 
*t_alloc)
 g_free(wqid);
 }
 
+static bool fs_dirents_contain_name(struct V9fsDirent *e, const char* name)
+{
+for (; e; e = e->next) {
+if (!strcmp(e->name, name)) {
+return true;
+}
+}
+return false;
+}
+
+static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+QVirtio9P *v9p = obj;
+alloc = t_alloc;
+char *const wnames[] = { g_strdup(QTEST_V9FS_SYNTH_READDIR_DIR) };
+uint16_t nqid;
+v9fs_qid qid;
+uint32_t count, nentries;
+struct V9fsDirent *entries = NULL;
+P9Req *req;
+
+fs_attach(v9p, NULL, t_alloc);
+req = v9fs_twalk(v9p, 0, 1, 1, wnames, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rwalk(req, , NULL);
+g_assert_cmpint(nqid, ==, 1);
+
+req = v9fs_tlopen(v9p, 1, O_DIRECTORY, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rlopen(req, , NULL);
+
+/*
+ * submit count = msize - 11, because 11 is the header size of Rreaddir
+ */
+req = v9fs_treaddir(v9p, 1, 0, P9_MAX_SIZE - 11, 0);
+v9fs_req_wait_for_reply(req, NULL);
+v9fs_rreaddir(req, , , );
+
+/*
+ * Assuming msize (P9_MAX_SIZE) is large enough so we can retrieve 

[PATCH v4 10/11] 9pfs: T_readdir latency optimization

[Christian Schoenebeck]

Make top half really top half and bottom half really bottom half:

Each T_readdir request handling is hopping between threads (main
I/O thread and background I/O driver threads) several times for
every individual directory entry, which sums up to huge latencies
for handling just a single T_readdir request.

Instead of doing that, collect now all required directory entries
(including all potentially required stat buffers for each entry) in
one rush on a background I/O thread from fs driver, then assemble
the entire resulting network response message for the readdir
request on main I/O thread. The fs driver is still aborting the
directory entry retrieval loop (on the background I/O thread) as
soon as it would exceed the client's requested maximum R_readdir
response size. So we should not have any performance penalty by
doing this.

Signed-off-by: Christian Schoenebeck 
---
 hw/9pfs/9p.c| 124 +++-
 hw/9pfs/9p.h|  23 ++
 hw/9pfs/codir.c | 183 +---
 hw/9pfs/coth.h  |   3 +
 4 files changed, 254 insertions(+), 79 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 18370183c4..e0ca45d46b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -971,30 +971,6 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, 
V9fsFidState *fidp,
 return 0;
 }
 
-static int coroutine_fn dirent_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
-  struct dirent *dent, V9fsQID *qidp)
-{
-struct stat stbuf;
-V9fsPath path;
-int err;
-
-v9fs_path_init();
-
-err = v9fs_co_name_to_path(pdu, >path, dent->d_name, );
-if (err < 0) {
-goto out;
-}
-err = v9fs_co_lstat(pdu, , );
-if (err < 0) {
-goto out;
-}
-err = stat_to_qid(pdu, , qidp);
-
-out:
-v9fs_path_free();
-return err;
-}
-
 V9fsPDU *pdu_alloc(V9fsState *s)
 {
 V9fsPDU *pdu = NULL;
@@ -2314,7 +2290,7 @@ out_nofid:
 pdu_complete(pdu, err);
 }
 
-static size_t v9fs_readdir_data_size(V9fsString *name)
+size_t v9fs_readdir_response_size(V9fsString *name)
 {
 /*
  * Size of each dirent on the wire: size of qid (13) + size of offset (8)
@@ -2323,6 +2299,18 @@ static size_t v9fs_readdir_data_size(V9fsString *name)
 return 24 + v9fs_string_size(name);
 }
 
+static void v9fs_free_dirents(struct V9fsDirEnt *e)
+{
+struct V9fsDirEnt *next = NULL;
+
+for (; e; e = next) {
+next = e->next;
+g_free(e->dent);
+g_free(e->st);
+g_free(e);
+}
+}
+
 static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
 int32_t max_count)
 {
@@ -2331,54 +2319,53 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 V9fsString name;
 int len, err = 0;
 int32_t count = 0;
-off_t saved_dir_pos;
 struct dirent *dent;
+struct stat *st;
+struct V9fsDirEnt *entries = NULL;
 
-/* save the directory position */
-saved_dir_pos = v9fs_co_telldir(pdu, fidp);
-if (saved_dir_pos < 0) {
-return saved_dir_pos;
-}
-
-while (1) {
-v9fs_readdir_lock(>fs.dir);
+/*
+ * inode remapping requires the device id, which in turn might be
+ * different for different directory entries, so if inode remapping is
+ * enabled we have to make a full stat for each directory entry
+ */
+const bool dostat = pdu->s->ctx.export_flags & V9FS_REMAP_INODES;
 
-err = v9fs_co_readdir(pdu, fidp, );
-if (err || !dent) {
-break;
-}
-v9fs_string_init();
-v9fs_string_sprintf(, "%s", dent->d_name);
-if ((count + v9fs_readdir_data_size()) > max_count) {
-v9fs_readdir_unlock(>fs.dir);
+/*
+ * Fetch all required directory entries altogether on a background IO
+ * thread from fs driver. We don't want to do that for each entry
+ * individually, because hopping between threads (this main IO thread
+ * and background IO driver thread) would sum up to huge latencies.
+ */
+count = v9fs_co_readdir_lowlat(pdu, fidp, , max_count, dostat);
+if (count < 0) {
+err = count;
+count = 0;
+goto out;
+}
+count = 0;
 
-/* Ran out of buffer. Set dir back to old position and return */
-v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
-v9fs_string_free();
-return count;
-}
+for (struct V9fsDirEnt *e = entries; e; e = e->next) {
+dent = e->dent;
 
 if (pdu->s->ctx.export_flags & V9FS_REMAP_INODES) {
-/*
- * dirent_to_qid() implies expensive stat call for each entry,
- * we must do that here though since inode remapping requires
- * the device id, which in turn might be different for
- * different entries; we cannot make any assumption to avoid
- * that here.
- */
-err = 

[PATCH v4 04/11] hw/9pfs/9p-synth: added directory for readdir test

[Christian Schoenebeck]

This will provide the following virtual files by the 9pfs
synth driver:

  - /ReadDirDir/ReadDirFile99
  - /ReadDirDir/ReadDirFile98
  ...
  - /ReadDirDir/ReadDirFile1
  - /ReadDirDir/ReadDirFile0

This virtual directory and its virtual 100 files will be
used by the upcoming 9pfs readdir tests.

Signed-off-by: Christian Schoenebeck 
Reviewed-by: Greg Kurz 
---
 hw/9pfs/9p-synth.c | 19 +++
 hw/9pfs/9p-synth.h |  5 +
 2 files changed, 24 insertions(+)

diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 54239c9bbf..7eb210ffa8 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -578,6 +578,25 @@ static int synth_init(FsContext *ctx, Error **errp)
NULL, v9fs_synth_qtest_flush_write,
ctx);
 assert(!ret);
+
+/* Directory for READDIR test */
+{
+V9fsSynthNode *dir = NULL;
+ret = qemu_v9fs_synth_mkdir(
+NULL, 0700, QTEST_V9FS_SYNTH_READDIR_DIR, 
+);
+assert(!ret);
+for (i = 0; i < QTEST_V9FS_SYNTH_READDIR_NFILES; ++i) {
+char *name = g_strdup_printf(
+QTEST_V9FS_SYNTH_READDIR_FILE, i
+);
+ret = qemu_v9fs_synth_add_file(
+dir, 0, name, NULL, NULL, ctx
+);
+assert(!ret);
+g_free(name);
+}
+}
 }
 
 return 0;
diff --git a/hw/9pfs/9p-synth.h b/hw/9pfs/9p-synth.h
index af7a993a1e..036d7e4a5b 100644
--- a/hw/9pfs/9p-synth.h
+++ b/hw/9pfs/9p-synth.h
@@ -55,6 +55,11 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
 #define QTEST_V9FS_SYNTH_LOPEN_FILE "LOPEN"
 #define QTEST_V9FS_SYNTH_WRITE_FILE "WRITE"
 
+/* for READDIR test */
+#define QTEST_V9FS_SYNTH_READDIR_DIR "ReadDirDir"
+#define QTEST_V9FS_SYNTH_READDIR_FILE "ReadDirFile%d"
+#define QTEST_V9FS_SYNTH_READDIR_NFILES 100
+
 /* Any write to the "FLUSH" file is handled one byte at a time by the
  * backend. If the byte is zero, the backend returns success (ie, 1),
  * otherwise it forces the server to try again forever. Thus allowing
-- 
2.20.1

[PATCH v4 11/11] hw/9pfs/9p.c: benchmark time on T_readdir request

[Christian Schoenebeck]

This patch is not intended to be merged, it measures
and prints the time the 9p server spends on handling
a T_readdir request. It prints the total time it spent
on handling the request, and also the time it spent
on I/O (fs driver) only.

Signed-off-by: Christian Schoenebeck 
---
 hw/9pfs/9p.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e0ca45d46b..9cd494edd7 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2311,6 +2311,15 @@ static void v9fs_free_dirents(struct V9fsDirEnt *e)
 }
 }
 
+static double wall_time(void)
+{
+struct timeval t;
+struct timezone tz;
+gettimeofday(, );
+return t.tv_sec + t.tv_usec * 0.01;
+}
+
+
 static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
 int32_t max_count)
 {
@@ -2330,6 +2339,8 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
  */
 const bool dostat = pdu->s->ctx.export_flags & V9FS_REMAP_INODES;
 
+const double start = wall_time();
+
 /*
  * Fetch all required directory entries altogether on a background IO
  * thread from fs driver. We don't want to do that for each entry
@@ -2344,6 +2355,10 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
V9fsFidState *fidp,
 }
 count = 0;
 
+const double end = wall_time();
+printf("\n\nTime 9p server spent on synth_readdir() I/O only (synth "
+   "driver): %fs\n", end - start);
+
 for (struct V9fsDirEnt *e = entries; e; e = e->next) {
 dent = e->dent;
 
@@ -2416,6 +2431,8 @@ static void coroutine_fn v9fs_readdir(void *opaque)
 V9fsPDU *pdu = opaque;
 V9fsState *s = pdu->s;
 
+const double start = wall_time();
+
 retval = pdu_unmarshal(pdu, offset, "dqd", ,
_offset, _count);
 if (retval < 0) {
@@ -2459,6 +2476,10 @@ out:
 put_fid(pdu, fidp);
 out_nofid:
 pdu_complete(pdu, retval);
+
+const double end = wall_time();
+printf("Time 9p server spent on entire T_readdir request: %fs "
+   "[IMPORTANT]\n", end - start);
 }
 
 static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
-- 
2.20.1

[PATCH v4 00/11] 9pfs: readdir optimization

[Christian Schoenebeck]

As previously mentioned, I was investigating performance issues with 9pfs.
Raw file read/write of 9pfs is actually quite good, provided that client
picked a reasonable high msize (maximum message size). I would recommend
to log a warning on 9p server side if a client attached with a small msize
that would cause performance issues for that reason.

However there are other aspects where 9pfs currently performs suboptimally,
especially readdir handling of 9pfs is extremely slow, a simple readdir
request of a guest typically blocks for several hundred milliseconds or
even several seconds, no matter how powerful the underlying hardware is.
The reason for this performance issue: latency.
Currently 9pfs is heavily dispatching a T_readdir request numerous times
between main I/O thread and a background I/O thread back and forth; in fact
it is actually hopping between threads even multiple times for every single
directory entry during T_readdir request handling which leads in total to
huge latencies for a single T_readdir request.

This patch series aims to address this severe performance issue of 9pfs
T_readdir request handling. The actual performance fix is patch 10. I also
provided a convenient benchmark for comparing the performance improvements
by using the 9pfs "synth" driver (see patch 8 for instructions how to run
the benchmark), so no guest OS installation is required to peform this
benchmark A/B comparison. With patch 10 I achieved a performance improvement
of factor 40 on my test machine.

** NOTE: ** As outlined by patch 7 there seems to be an outstanding issue
(both with current, unoptimized readdir code, as well as with new, optimized
readdir code) causing a transport error with splitted readdir requests. This
issue only occurs if patch 7 is applied. I haven't investigated the cause of
this issue yet, it looks like a memory issue though. I am not sure if it is a
problem with the actual 9p server or rather "just" with the test environment.
Apart from that issue, the actual splitted readdir seems to work well with the
new performance optimized readdir code as well though.

v3->v4:

  * Rebased to master (SHA-1 43d1455c).

  * Adjusted commit log message [patch 2], [patch 3], [patch 8].

  * Fixed using Rreaddir header size of 11 (instead of P9_IOHDRSZ) for
limiting 'count' parameter of Treaddir [patch 3], [patch 5].

Christian Schoenebeck (11):
  tests/virtio-9p: add terminating null in v9fs_string_read()
  9pfs: require msize >= 4096
  9pfs: validate count sent by client with T_readdir
  hw/9pfs/9p-synth: added directory for readdir test
  tests/virtio-9p: added readdir test
  tests/virtio-9p: added splitted readdir test
  tests/virtio-9p: failing splitted readdir test
  9pfs: readdir benchmark
  hw/9pfs/9p-synth: avoid n-square issue in synth_readdir()
  9pfs: T_readdir latency optimization
  hw/9pfs/9p.c: benchmark time on T_readdir request

 hw/9pfs/9p-synth.c   |  48 +-
 hw/9pfs/9p-synth.h   |   5 +
 hw/9pfs/9p.c | 163 
 hw/9pfs/9p.h |  34 
 hw/9pfs/codir.c  | 183 --
 hw/9pfs/coth.h   |   3 +
 tests/qtest/virtio-9p-test.c | 290 ++-
 7 files changed, 643 insertions(+), 83 deletions(-)

-- 
2.20.1

[PATCH 2/2] aspeed/scu: Implement chip ID register

[Joel Stanley]

This returns a fixed but non-zero value for the chip id.

Signed-off-by: Joel Stanley 
---
 hw/misc/aspeed_scu.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 7108cad8c6a7..19d1780a40da 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -77,6 +77,8 @@
 #define CPU2_BASE_SEG4   TO_REG(0x110)
 #define CPU2_BASE_SEG5   TO_REG(0x114)
 #define CPU2_CACHE_CTRL  TO_REG(0x118)
+#define CHIP_ID0 TO_REG(0x150)
+#define CHIP_ID1 TO_REG(0x154)
 #define UART_HPLL_CLKTO_REG(0x160)
 #define PCIE_CTRLTO_REG(0x180)
 #define BMC_MMIO_CTRLTO_REG(0x184)
@@ -115,6 +117,8 @@
 #define AST2600_HW_STRAP2_PROTTO_REG(0x518)
 #define AST2600_RNG_CTRL  TO_REG(0x524)
 #define AST2600_RNG_DATA  TO_REG(0x540)
+#define AST2600_CHIP_ID0  TO_REG(0x5B0)
+#define AST2600_CHIP_ID1  TO_REG(0x5B4)
 
 #define AST2600_CLK TO_REG(0x40)
 
@@ -182,6 +186,8 @@ static const uint32_t ast2500_a1_resets[ASPEED_SCU_NR_REGS] 
= {
  [CPU2_BASE_SEG1]  = 0x8000U,
  [CPU2_BASE_SEG4]  = 0x1E60U,
  [CPU2_BASE_SEG5]  = 0xC000U,
+ [CHIP_ID0]= 0x1234ABCDU,
+ [CHIP_ID1]= 0xU,
  [UART_HPLL_CLK]   = 0x1903U,
  [PCIE_CTRL]   = 0x007BU,
  [BMC_DEV_ID]  = 0x2402U
@@ -307,6 +313,8 @@ static void aspeed_ast2500_scu_write(void *opaque, hwaddr 
offset,
 case RNG_DATA:
 case FREE_CNTR4:
 case FREE_CNTR4_EXT:
+case CHIP_ID0:
+case CHIP_ID1:
 qemu_log_mask(LOG_GUEST_ERROR,
   "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
   __func__, offset);
@@ -620,6 +628,8 @@ static void aspeed_ast2600_scu_write(void *opaque, hwaddr 
offset,
 case AST2600_RNG_DATA:
 case AST2600_SILICON_REV:
 case AST2600_SILICON_REV2:
+case AST2600_CHIP_ID0:
+case AST2600_CHIP_ID1:
 /* Add read only registers here */
 qemu_log_mask(LOG_GUEST_ERROR,
   "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
@@ -648,6 +658,9 @@ static const uint32_t 
ast2600_a0_resets[ASPEED_AST2600_SCU_NR_REGS] = {
 [AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
 [AST2600_SDRAM_HANDSHAKE]   = 0x0040,  /* SoC completed DRAM init */
 [AST2600_HPLL_PARAM]= 0x1000405F,
+[AST2600_CHIP_ID0]  = 0x1234ABCD,
+[AST2600_CHIP_ID1]  = 0x,
+
 };
 
 static void aspeed_ast2600_scu_reset(DeviceState *dev)
-- 
2.24.1

[PATCH 0/2] aspeed/scu: Implement chip id register

[Joel Stanley]

This implements the chip id register in the SCU for the ast2500 and
ast2600. The first patch is a cleanup to separate out ast2400 and
ast2500 functionality.

Joel Stanley (2):
  aspeed/scu: Create separate write callbacks
  aspeed/scu: Implement chip ID register

 hw/misc/aspeed_scu.c | 93 +---
 1 file changed, 70 insertions(+), 23 deletions(-)

-- 
2.24.1

[PATCH 1/2] aspeed/scu: Create separate write callbacks

[Joel Stanley]

This splits the common write callback into separate ast2400 and ast2500
implementations. This makes it clearer when implementing differing
behaviour.

Signed-off-by: Joel Stanley 
---
 hw/misc/aspeed_scu.c | 80 +++-
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index f62fa25e3474..7108cad8c6a7 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -232,8 +232,47 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr 
offset, unsigned size)
 return s->regs[reg];
 }
 
-static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
- unsigned size)
+static void aspeed_ast2400_scu_write(void *opaque, hwaddr offset,
+ uint64_t data, unsigned size)
+{
+AspeedSCUState *s = ASPEED_SCU(opaque);
+int reg = TO_REG(offset);
+
+if (reg >= ASPEED_SCU_NR_REGS) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+  __func__, offset);
+return;
+}
+
+if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
+!s->regs[PROT_KEY]) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);
+}
+
+trace_aspeed_scu_write(offset, size, data);
+
+switch (reg) {
+case PROT_KEY:
+s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
+return;
+case SILICON_REV:
+case FREQ_CNTR_EVAL:
+case VGA_SCRATCH1 ... VGA_SCRATCH8:
+case RNG_DATA:
+case FREE_CNTR4:
+case FREE_CNTR4_EXT:
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
+  __func__, offset);
+return;
+}
+
+s->regs[reg] = data;
+}
+
+static void aspeed_ast2500_scu_write(void *opaque, hwaddr offset,
+ uint64_t data, unsigned size)
 {
 AspeedSCUState *s = ASPEED_SCU(opaque);
 int reg = TO_REG(offset);
@@ -257,25 +296,11 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, 
uint64_t data,
 case PROT_KEY:
 s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
 return;
-case CLK_SEL:
-s->regs[reg] = data;
-break;
 case HW_STRAP1:
-if (ASPEED_IS_AST2500(s->regs[SILICON_REV])) {
-s->regs[HW_STRAP1] |= data;
-return;
-}
-/* Jump to assignment below */
-break;
+s->regs[HW_STRAP1] |= data;
+return;
 case SILICON_REV:
-if (ASPEED_IS_AST2500(s->regs[SILICON_REV])) {
-s->regs[HW_STRAP1] &= ~data;
-} else {
-qemu_log_mask(LOG_GUEST_ERROR,
-  "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n",
-  __func__, offset);
-}
-/* Avoid assignment below, we've handled everything */
+s->regs[HW_STRAP1] &= ~data;
 return;
 case FREQ_CNTR_EVAL:
 case VGA_SCRATCH1 ... VGA_SCRATCH8:
@@ -291,9 +316,18 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, 
uint64_t data,
 s->regs[reg] = data;
 }
 
-static const MemoryRegionOps aspeed_scu_ops = {
+static const MemoryRegionOps aspeed_ast2400_scu_ops = {
+.read = aspeed_scu_read,
+.write = aspeed_ast2400_scu_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.valid.unaligned = false,
+};
+
+static const MemoryRegionOps aspeed_ast2500_scu_ops = {
 .read = aspeed_scu_read,
-.write = aspeed_scu_write,
+.write = aspeed_ast2500_scu_write,
 .endianness = DEVICE_LITTLE_ENDIAN,
 .valid.min_access_size = 4,
 .valid.max_access_size = 4,
@@ -469,7 +503,7 @@ static void aspeed_2400_scu_class_init(ObjectClass *klass, 
void *data)
 asc->calc_hpll = aspeed_2400_scu_calc_hpll;
 asc->apb_divider = 2;
 asc->nr_regs = ASPEED_SCU_NR_REGS;
-asc->ops = _scu_ops;
+asc->ops = _ast2400_scu_ops;
 }
 
 static const TypeInfo aspeed_2400_scu_info = {
@@ -489,7 +523,7 @@ static void aspeed_2500_scu_class_init(ObjectClass *klass, 
void *data)
 asc->calc_hpll = aspeed_2500_scu_calc_hpll;
 asc->apb_divider = 4;
 asc->nr_regs = ASPEED_SCU_NR_REGS;
-asc->ops = _scu_ops;
+asc->ops = _ast2500_scu_ops;
 }
 
 static const TypeInfo aspeed_2500_scu_info = {
-- 
2.24.1

RE: [PATCH RFC 12/12] migration/rdma: only register the virt-ram block for MultiRDMA

[fengzhimin]

The performance increase is due to the multiple RDMA channels instead of 
multiple threads, so we must register RAM blocks for the multiple RDMA channels.

Zhimin Feng

-Original Message-
From: Dr. David Alan Gilbert [mailto:dgilb...@redhat.com] 
Sent: Monday, January 20, 2020 5:05 PM
To: fengzhimin 
Cc: quint...@redhat.com; arm...@redhat.com; ebl...@redhat.com; 
qemu-devel@nongnu.org; Zhanghailiang ; 
jemmy858...@gmail.com
Subject: Re: [PATCH RFC 12/12] migration/rdma: only register the virt-ram block 
for MultiRDMA

* fengzhimin (fengzhim...@huawei.com) wrote:
> OK, I will modify it.
> 
> Due to the mach-virt.ram is sent by the multiRDMA channels instead of the 
> main channel, it don't to register on the main channel.

You might be OK if instead  of using the name, you use a size threshold; e.g. 
you use the multirdma threads for any RAM block larger than say 128MB.

> It takes a long time to register the mach-virt.ram for VM with large capacity 
> memory, so we shall try our best not to register it.

I'm curious why, I know it's expensive to register RAM blocks with rdma code; 
but I thought that would just be the first time; it surprises me that 
registering it with a 2nd RDMA channel is as expensive.

But then that makes me ask a 2nd question; is your performance increase due to 
multiple threads or is it due to the multiple RDMA channels?
COuld you have multiple threads but still a single RDMA channel (and with 
sufficient locking) still get the performance?

Dave

> Thanks for your review.
> 
> Zhimin Feng
> 
> -Original Message-
> From: Dr. David Alan Gilbert [mailto:dgilb...@redhat.com]
> Sent: Saturday, January 18, 2020 2:52 AM
> To: fengzhimin 
> Cc: quint...@redhat.com; arm...@redhat.com; ebl...@redhat.com; 
> qemu-devel@nongnu.org; Zhanghailiang ; 
> jemmy858...@gmail.com
> Subject: Re: [PATCH RFC 12/12] migration/rdma: only register the 
> virt-ram block for MultiRDMA
> 
> * Zhimin Feng (fengzhim...@huawei.com) wrote:
> > From: fengzhimin 
> > 
> > The virt-ram block is sent by MultiRDMA, so we only register it for 
> > MultiRDMA channels and main channel don't register the virt-ram block.
> > 
> > Signed-off-by: fengzhimin 
> 
> You can't specialise on the name of the RAMBlock like that.
> 'mach-virt.ram' is the name specific to just the main ram on just aarch's 
> machine type;  for example the name on x86 is completely different and if you 
> use NUMA or hotplug etc it would also be different on aarch.
> 
> Is there a downside to also registering the mach-virt.ram on the main channel?
> 
> Dave
> 
> > ---
> >  migration/rdma.c | 140
> > +--
> >  1 file changed, 112 insertions(+), 28 deletions(-)
> > 
> > diff --git a/migration/rdma.c b/migration/rdma.c index 
> > 0a150099e2..1477fd509b 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -618,7 +618,9 @@ const char *print_wrid(int wrid);  static int 
> > qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head,
> > uint8_t *data, RDMAControlHeader *resp,
> > int *resp_idx,
> > -   int (*callback)(RDMAContext *rdma));
> > +   int (*callback)(RDMAContext *rdma,
> > +   uint8_t id),
> > +   uint8_t id);
> >  
> >  static inline uint64_t ram_chunk_index(const uint8_t *start,
> > const uint8_t *host) @@
> > -1198,24 +1200,81 @@ static int qemu_rdma_alloc_qp(RDMAContext *rdma)
> >  return 0;
> >  }
> >  
> > -static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
> > +/*
> > + * Parameters:
> > + *@id == UNUSED_ID :
> > + *This means that we register memory for the main RDMA channel,
> > + *the main RDMA channel don't register the mach-virt.ram block
> > + *when we use multiRDMA method to migrate.
> > + *
> > + *@id == 0 or id == 1 or ... :
> > + *This means that we register memory for the multiRDMA channels,
> > + *the multiRDMA channels only register memory for the mach-virt.ram
> > + *block when we use multiRDAM method to migrate.
> > + */
> > +static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma, 
> > +uint8_t
> > +id)
> >  {
> >  int i;
> >  RDMALocalBlocks *local = >local_ram_blocks;
> >  
> > -for (i = 0; i < local->nb_blocks; i++) {
> > -local->block[i].mr =
> > -ibv_reg_mr(rdma->pd,
> > -local->block[i].local_host_addr,
> > -local->block[i].length,
> > -IBV_ACCESS_LOCAL_WRITE |
> > -IBV_ACCESS_REMOTE_WRITE
> > -);
> > -if (!local->block[i].mr) {
> > -perror("Failed to register local dest ram block!\n");
> > -break;
> > +if (migrate_use_multiRDMA()) {
> > +if (id == UNUSED_ID) {
> > +

riscv: How to check floating point support is currently enabled?

[Ian Jiang]

The function riscv_cpu_fp_enabled() is used for checking whether floating
point support is currently enabled. In fact it checks the FS field in the
mstatus MSR.

target/riscv/cpu_helper.c
 76 bool riscv_cpu_fp_enabled(CPURISCVState *env)
 77 {
 78 if (env->mstatus & MSTATUS_FS) {
 79 return true;
 80 }
 81
 82 return false;
 83 }

This will cause a problem that the SD bit in mstatus is not set to 1 when
FS in mstatus is modified from '00'b to '11'b with write_mstatus().

file target/riscv/csr.c, func write_mstatus():
350 dirty = (riscv_cpu_fp_enabled(env) &&
351  ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
352 ((mstatus & MSTATUS_XS) == MSTATUS_XS);
353 mstatus = set_field(mstatus, MSTATUS_SD, dirty);
354 env->mstatus = mstatus;

So checking fields D and F in the misa MSR (bit 3 and bit 5) may be an
better way. That is
bool riscv_cpu_fp_enabled(CPURISCVState *env)
if (env->misa & (MISA_F | MISA_F) {
return true;
}
return false;
}


--
Ian Jiang

Re: [PATCH] qemu-img: Add --target-is-zero to convert

[John Snow]

CC qemu-block and block maintainers

On 1/17/20 5:34 AM, David Edmondson wrote:
> In many cases the target of a convert operation is a newly provisioned
> target that the user knows is blank (filled with zeroes). In this
> situation there is no requirement for qemu-img to wastefully zero out
> the entire device.
> 

Is there no way to convince bdrv_has_zero_init to return what we want
already in this case? I cannot recall off hand, but wonder if there's an
advanced syntax method of specifying the target image that can set this
flag already.

> Add a new option, --target-is-zero, allowing the user to indicate that
> an existing target device is already zero filled.
> ---
>  qemu-img.c | 19 ---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 95a24b9762..56ca727e8c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -70,6 +70,7 @@ enum {
>  OPTION_PREALLOCATION = 265,
>  OPTION_SHRINK = 266,
>  OPTION_SALVAGE = 267,
> +OPTION_TARGET_IS_ZERO = 268,
>  };
>  
>  typedef enum OutputFormat {
> @@ -1593,6 +1594,7 @@ typedef struct ImgConvertState {
>  bool copy_range;
>  bool salvage;
>  bool quiet;
> +bool target_is_zero;
>  int min_sparse;
>  int alignment;
>  size_t cluster_sectors;
> @@ -1984,10 +1986,11 @@ static int convert_do_copy(ImgConvertState *s)
>  int64_t sector_num = 0;
>  
>  /* Check whether we have zero initialisation or can get it efficiently */
> -if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +s->has_zero_init = s->target_is_zero;
> +
> +if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
> +!s->target_has_backing) {
>  s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> -} else {
> -s->has_zero_init = false;
>  }
>  
>  if (!s->has_zero_init && !s->target_has_backing &&
> @@ -2076,6 +2079,7 @@ static int img_convert(int argc, char **argv)
>  .buf_sectors= IO_BUF_SIZE / BDRV_SECTOR_SIZE,
>  .wr_in_order= true,
>  .num_coroutines = 8,
> +.target_is_zero = false,
>  };
>  
>  for(;;) {
> @@ -2086,6 +2090,7 @@ static int img_convert(int argc, char **argv)
>  {"force-share", no_argument, 0, 'U'},
>  {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>  {"salvage", no_argument, 0, OPTION_SALVAGE},
> +{"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
>  {0, 0, 0, 0}
>  };
>  c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> @@ -2209,6 +2214,9 @@ static int img_convert(int argc, char **argv)
>  case OPTION_TARGET_IMAGE_OPTS:
>  tgt_image_opts = true;
>  break;
> +case OPTION_TARGET_IS_ZERO:
> +s.target_is_zero = true;
> +break;
>  }
>  }
>  
> @@ -2247,6 +2255,11 @@ static int img_convert(int argc, char **argv)
>  warn_report("This will become an error in future QEMU versions.");
>  }
>  
> +if (s.target_is_zero && !skip_create) {
> +error_report("--target-is-zero requires use of -n flag");
> +goto fail_getopt;
> +}
> +
>  s.src_num = argc - optind - 1;
>  out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>  
> 

Re: Proposal for handling .hx files with Sphinx

[John Snow]

On 1/17/20 12:30 PM, Peter Maydell wrote:
> Currently our manual creation includes some .texi files which
> are autogenerated from .hx files by running scripts/hxtool.
> .hx files are a simple format, where where a line is either a
> directive or literal text to be output:
> 
> HXCOMM
>  -- comment lines, ignored
> STEXI/ETEXI
>  -- mark start/end of chunks of text to put into the texinfo output only
> DEFHEADING/ARCHHEADING
>  -- appear in the .h file output verbatim (they are defined as C macros);
> for texi output they are parsed to add in header sections
> 
> For Sphinx, rather than creating a file to include, the most
> natural way to handle this is to have a small custom Sphinx
> extension which will read the .hx file and process it. So
> instead of "makefile produces foo.texi from foo.hx, qemu-doc.texi
> says '@include foo.texi'", we have "qemu-doc.rst says
> 'hxtool-doc:: foo.hx', the Sphinx extension for hxtool has
> code that runs to handle that Sphinx directive, it reads the .hx
> file and emits the appropriate documentation contents". (This is
> pretty much the same way the kerneldoc extension works right now.
> It also has the advantage that it should work for third-party
> services like readthedocs that expect to build the docs directly
> with sphinx rather than by invoking our makefiles.)
> 
> We'll need to update what the markup is to handle having rST
> fragments in it. A very minimalist approach to this would
> simply define a new pair of SRST/ERST directives marking the
> start/end of chunks of rST text to go into the rST only.
> (We might be able to do better than that later, as there's
> some repetition currently going on. But we'll probably get
> a better idea of how easy it is to avoid the repetition if
> we start with a simple conversion.)
> 
> Here's what we do with hx files at the moment. We have four:
> 
>  hmp-commands.hx
>-- defines monitor commands used by monitor.c; generates
>   qemu-monitor.texi, used by qemu-doc.texi
>  hmp-commands-info.hx
>-- ditto, for the "info" command's subcommand;
>   generates qemu-monitor-info.texi, used by qemu-doc.texi
> 
> These two use only the "put this in the texi or in the .h file"
> functionality, alternating "raw C code defining an entry for the
> monitor command array" with "lump of raw texi for the docs".
> 
>  qemu-img-cmds.hx
>-- defines options for qemu-img, used by qemu-img.texi
> 
> This uses the STEXI/ETEXI directives to alternate C and texi.
> In the for-the-h-file section the only content is always a DEF()
> macro invocation defining the option; in the texi is only the
> synopsis of the command. This means there's a lot of repetition,
> as the DEF macro includes an argument giving the text of the
> option synopsis, and then the texi also has that synopsis with
> some extra markup. Finally the main qemu-img.texi repeats the
> marked-up synopsis later on when it has the actual main documentation
> of each command.
> 
>  qemu-options.hx
>-- options for qemu proper, used by qemu-doc.texi
> 
> This uses only the DEF, DEFHEADING, ARCHHEADING macros
> in the for-the-h-file sections (and the DEFHEADING/ARCHHEADING
> are read also for texi generation). This also repeats the
> synopsis in the DEF macro and in the texi fragment.
> 
> So I think my current view is that we should do the very
> simple "add SRST/ERST directives" to start with:
>  * scripts/hxtool needs to recognize them and just ignore
>the text inside them
>  * write the hxtool sphinx extension (shouldn't be too hard)
>  * conversion of any particular .hx file then involves
>replacing the STEXI ...texi stuff... ETEXI sections with
>SRST ...rst stuff... ERST. There's no need for any
>particular .hx file to support both texi and rst output
>at the same time
> 
> I would initially start with qemu-img-cmds.hx, since that's
> pulled in by qemu-img.texi, which we can convert in the
> same way I've been doing qemu-nbd and other standalone-ish
> manpages. The others are part of the big fat qemu-doc.texi,
> which is probably going to be the very last thing we convert...
> 

At one point I did a quick mockup of turning qemu-img-cmds.hx into json
and wrote a small tool I called "pxtool" that was used for generating
all the rest of the subsequent information -- an attempt at getting rid
of .hx files *entirely*.

The idea at heart was: "Can we remove .hx files and describe everything
in terms of the QAPI schema instead?"

I'm still a bit partial to that idea, but realize there are some nasty
complexities when it comes to describing the QEMU CLI as a schema. One
of those is that I doubt we even have a full understanding of what the
CLI syntax is at all.

Still, I do want to ask: Are we sure we want to double-down on keeping
the .hx files around instead of trying to move to a more generic data
format?

[PATCH v4 7/7] tests/boot_linux_console: Tag Emcraft Smartfusion2 as running 'u-boot'

[Philippe Mathieu-Daudé]

Avocado tags are handy to automatically select tests matching
the tags. Since this test also runs U-Boot, tag it.

We can run all the tests using U-Boot as once with:

  $ avocado --show=app run -t u-boot tests/acceptance/
  JOB LOG: avocado/job-results/job-2020-01-21T00.16-ee9344e/job.log
   (1/3) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_emcraft_sf2: 
PASS (16.59 s)
   (2/3) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_uboot: 
PASS (0.47 s)
   (3/3) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_uboot:
 PASS (2.43 s)
  RESULTS: PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
  JOB TIME   : 19.78 s

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/boot_linux_console.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index 22b360118d..4a4cf9d0ea 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -305,6 +305,7 @@ class BootLinuxConsole(Test):
 :avocado: tags=arch:arm
 :avocado: tags=machine:emcraft-sf2
 :avocado: tags=endian:little
+:avocado: tags=u-boot
 """
 uboot_url = ('https://raw.githubusercontent.com/'
  'Subbaraya-Sundeep/qemu-test-binaries/'
-- 
2.21.1

[PATCH v4 5/7] tests/boot_linux_console: Test booting U-Boot on the Raspberry Pi 2

[Philippe Mathieu-Daudé]

This test runs U-Boot on the Raspberry Pi 2.
It is very simple and fast:

  $ avocado --show=app,console run -t raspi2 -t u-boot tests/acceptance/
  JOB LOG: avocado/job-results/job-2020-01-20T23.40-2424777/job.log
   (1/1) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_uboot:
  console: MMC:   sdhci@7e30: 0
  console: Loading Environment from FAT... Card did not respond to voltage 
select!
  console: In:serial
  console: Out:   vidconsole
  console: Err:   vidconsole
  console: Net:   No ethernet found.
  console: starting USB...
  console: USB0:   Port not available.
  console: Hit any key to stop autoboot:  0
  console: U-Boot>
  console: U-Boot> bdinfo
  console: arch_number = 0x
  console: boot_params = 0x0100
  console: DRAM bank   = 0x
  console: -> start= 0x
  console: -> size = 0x3c00
  console: baudrate= 115200 bps
  console: TLB addr= 0x3bff
  console: relocaddr   = 0x3bf64000
  console: reloc off   = 0x3bf5c000
  console: irq_sp  = 0x3bb5fec0
  console: sp start= 0x3bb5feb0
  console: Early malloc usage: 2a4 / 400
  console: fdt_blob= 0x3bfbdfb0
  console: U-Boot> version
  console: U-Boot 2019.01+dfsg-7 (May 14 2019 - 02:07:44 +)
  console: gcc (Debian 8.3.0-7) 8.3.0
  console: GNU ld (GNU Binutils for Debian) 2.31.1
  console: U-Boot> reset
  console: resetting ...
  PASS (0.46 s)

U-Boot is built by the Debian project, see:
https://wiki.debian.org/InstallingDebianOn/Allwinner#Creating_a_bootable_SD_Card_with_u-boot

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/boot_linux_console.py | 28 ++
 1 file changed, 28 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index e40b84651b..682b801b4f 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -16,6 +16,7 @@ import shutil
 from avocado import skipUnless
 from avocado_qemu import Test
 from avocado_qemu import exec_command_and_wait_for_pattern
+from avocado_qemu import interrupt_interactive_console_until_pattern
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import process
 from avocado.utils import archive
@@ -485,6 +486,33 @@ class BootLinuxConsole(Test):
 exec_command_and_wait_for_pattern(self, 'reboot',
 'reboot: Restarting system')
 
+def test_arm_raspi2_uboot(self):
+"""
+:avocado: tags=arch:arm
+:avocado: tags=machine:raspi2
+:avocado: tags=u-boot
+"""
+deb_url = ('https://snapshot.debian.org/archive/debian/'
+   '20190514T084354Z/pool/main/u/u-boot/'
+   'u-boot-rpi_2019.01%2Bdfsg-7_armhf.deb')
+deb_hash = 'ad858cf3afe623b6c3fa2e20dcdd1768fcb9ae83'
+deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
+uboot_path = '/usr/lib/u-boot/rpi_2/uboot.elf'
+uboot_path = self.extract_from_deb(deb_path, uboot_path)
+
+self.vm.set_console()
+self.vm.add_args('-kernel', uboot_path,
+ # VideoCore starts CPU with only 1 core enabled
+ '-global', 'bcm2836.enabled-cpus=1',
+ '-no-reboot')
+self.vm.launch()
+interrupt_interactive_console_until_pattern(self,
+   'Hit any key to stop autoboot:',
+   'Config file not found')
+exec_command_and_wait_for_pattern(self, 'bdinfo', 'U-Boot')
+exec_command_and_wait_for_pattern(self, 'version', 'U-Boot')
+exec_command_and_wait_for_pattern(self, 'reset', 'resetting ...')
+
 def test_s390x_s390_ccw_virtio(self):
 """
 :avocado: tags=arch:s390x
-- 
2.21.1

[PATCH v4 6/7] tests/boot_linux_console: Test booting U-Boot on the Raspberry Pi 3

[Philippe Mathieu-Daudé]

This test runs U-Boot on the Raspberry Pi 3.
It is very simple and fast:

  $ avocado --show=app,console run -t raspi3 -t u-boot tests/acceptance/
  JOB LOG: avocado/job-results/job-2020-01-20T23.40-2424777/job.log
   (1/1) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_uboot:
  console: MMC:   mmc@7e202000: 0, sdhci@7e30: 1
  console: Loading Environment from FAT... WARNING at 
drivers/mmc/bcm2835_sdhost.c:410/bcm2835_send_command()!
  console: WARNING at drivers/mmc/bcm2835_sdhost.c:410/bcm2835_send_command()!
  console: Card did not respond to voltage select!
  console: In:serial
  console: Out:   vidconsole
  console: Err:   vidconsole
  console: Net:   No ethernet found.
  console: starting USB...
  console: Bus usb@7e98: Port not available.
  console: Hit any key to stop autoboot:  0
  console: U-Boot>
  console: U-Boot>
  console: U-Boot> bdinfo
  console: arch_number = 0x
  console: boot_params = 0x0100
  console: DRAM bank   = 0x
  console: -> start= 0x
  console: -> size = 0x3c00
  console: baudrate= 115200 bps
  console: TLB addr= 0x3bff
  console: relocaddr   = 0x3bf57000
  console: reloc off   = 0x3bed7000
  console: irq_sp  = 0x3bb52dd0
  console: sp start= 0x3bb52dd0
  console: FB base = 0x
  console: Early malloc usage: 7b0 / 2000
  console: fdt_blob= 0x3bfbf200
  console: U-Boot> version
  console: U-Boot 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +)
  console: gcc (Debian 9.2.1-22) 9.2.1 20200104
  console: GNU ld (GNU Binutils for Debian) 2.33.1
  console: U-Boot> reset
  console: resetting ...
  PASS (1.79 s)

U-Boot is built by the Debian project, see:
https://wiki.debian.org/InstallingDebianOn/Allwinner#Creating_a_bootable_SD_Card_with_u-boot

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/boot_linux_console.py | 25 +
 1 file changed, 25 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index 682b801b4f..22b360118d 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -513,6 +513,31 @@ class BootLinuxConsole(Test):
 exec_command_and_wait_for_pattern(self, 'version', 'U-Boot')
 exec_command_and_wait_for_pattern(self, 'reset', 'resetting ...')
 
+def test_aarch64_raspi3_uboot(self):
+"""
+:avocado: tags=arch:aarch64
+:avocado: tags=machine:raspi3
+:avocado: tags=u-boot
+"""
+deb_url = ('https://snapshot.debian.org/archive/debian/'
+   '20200108T145233Z/pool/main/u/u-boot/'
+   'u-boot-rpi_2020.01%2Bdfsg-1_arm64.deb')
+deb_hash = 'f394386e02469d52f2eb3c07a2325b1c95aeb00b'
+deb_path = self.fetch_asset(deb_url, asset_hash=deb_hash)
+uboot_path = '/usr/lib/u-boot/rpi_3/u-boot.bin'
+uboot_path = self.extract_from_deb(deb_path, uboot_path)
+
+self.vm.set_console(console_index=1)
+self.vm.add_args('-kernel', uboot_path,
+ '-no-reboot')
+self.vm.launch()
+interrupt_interactive_console_until_pattern(self,
+   'Hit any key to stop autoboot:',
+   'Config file not found')
+exec_command_and_wait_for_pattern(self, 'bdinfo', 'U-Boot')
+exec_command_and_wait_for_pattern(self, 'version', 'U-Boot')
+exec_command_and_wait_for_pattern(self, 'reset', 'resetting ...')
+
 def test_s390x_s390_ccw_virtio(self):
 """
 :avocado: tags=arch:s390x
-- 
2.21.1

[PATCH v4 2/7] Acceptance tests: Extract _console_interaction()

[Philippe Mathieu-Daudé]

Since we are going to re-use the code shared between
wait_for_console_pattern() and exec_command_and_wait_for_pattern(),
extract the common part into a local function.

Tested-by: Niek Linnenbank 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/avocado_qemu/__init__.py | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 6618ea67c1..0a50fcf2be 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -55,19 +55,14 @@ def pick_default_qemu_bin(arch=None):
 return qemu_bin_from_src_dir_path
 
 
-def wait_for_console_pattern(test, success_message, failure_message=None):
-"""
-Waits for messages to appear on the console, while logging the content
-
-:param test: an Avocado test containing a VM that will have its console
- read and probed for a success or failure message
-:type test: :class:`avocado_qemu.Test`
-:param success_message: if this message appears, test succeeds
-:param failure_message: if this message appears, test fails
-"""
+def _console_interaction(test, success_message, failure_message,
+ send_string):
 console = test.vm.console_socket.makefile()
 console_logger = logging.getLogger('console')
 while True:
+if send_string:
+test.vm.console_socket.sendall(send_string.encode())
+send_string = None # send only once
 msg = console.readline().strip()
 if not msg:
 continue
@@ -79,6 +74,17 @@ def wait_for_console_pattern(test, success_message, 
failure_message=None):
 fail = 'Failure message found in console: %s' % failure_message
 test.fail(fail)
 
+def wait_for_console_pattern(test, success_message, failure_message=None):
+"""
+Waits for messages to appear on the console, while logging the content
+
+:param test: an Avocado test containing a VM that will have its console
+ read and probed for a success or failure message
+:type test: :class:`avocado_qemu.Test`
+:param success_message: if this message appears, test succeeds
+:param failure_message: if this message appears, test fails
+"""
+_console_interaction(test, success_message, failure_message, None)
 
 def exec_command_and_wait_for_pattern(test, command,
   success_message, failure_message=None):
@@ -94,10 +100,7 @@ def exec_command_and_wait_for_pattern(test, command,
 :param success_message: if this message appears, test succeeds
 :param failure_message: if this message appears, test fails
 """
-command += '\r'
-test.vm.console_socket.sendall(command.encode())
-wait_for_console_pattern(test, success_message, failure_message)
-
+_console_interaction(test, success_message, failure_message, command + 
'\r')
 
 class Test(avocado.Test):
 def _get_unique_tag_val(self, tag_name):
-- 
2.21.1

[PATCH v4 3/7] Acceptance tests: Add interrupt_interactive_console_until_pattern()

[Philippe Mathieu-Daudé]

We need a function to interrupt interactive consoles.

Example: Interrupt U-Boot to set different environment values.

Tested-by: Niek Linnenbank 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/avocado_qemu/__init__.py | 32 +--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 0a50fcf2be..d4358eb431 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -56,13 +56,15 @@ def pick_default_qemu_bin(arch=None):
 
 
 def _console_interaction(test, success_message, failure_message,
- send_string):
+ send_string, keep_sending=False):
+assert not keep_sending or send_string
 console = test.vm.console_socket.makefile()
 console_logger = logging.getLogger('console')
 while True:
 if send_string:
 test.vm.console_socket.sendall(send_string.encode())
-send_string = None # send only once
+if not keep_sending:
+send_string = None # send only once
 msg = console.readline().strip()
 if not msg:
 continue
@@ -74,6 +76,32 @@ def _console_interaction(test, success_message, 
failure_message,
 fail = 'Failure message found in console: %s' % failure_message
 test.fail(fail)
 
+def interrupt_interactive_console_until_pattern(test, success_message,
+failure_message=None,
+interrupt_string='\r'):
+"""
+Keep sending a string to interrupt a console prompt, while logging the
+console output. Typical use case is to break a boot loader prompt, such:
+
+Press a key within 5 seconds to interrupt boot process.
+5
+4
+3
+2
+1
+Booting default image...
+
+:param test: an Avocado test containing a VM that will have its console
+ read and probed for a success or failure message
+:type test: :class:`avocado_qemu.Test`
+:param success_message: if this message appears, test succeeds
+:param failure_message: if this message appears, test fails
+:param interrupt_string: a string to send to the console before trying
+ to read a new line
+"""
+_console_interaction(test, success_message, failure_message,
+ interrupt_string, True)
+
 def wait_for_console_pattern(test, success_message, failure_message=None):
 """
 Waits for messages to appear on the console, while logging the content
-- 
2.21.1

[PATCH v4 4/7] python/qemu/machine: Allow to use other serial consoles than default

[Philippe Mathieu-Daudé]

Currently the QEMU Python module limits the QEMUMachine class to
use the first serial console.

Some machines/guest might use another console than the first one as
the 'boot console'. For example the Raspberry Pi uses the second
(AUX) console.

To be able to use the Nth console as default, we simply need to
connect all the N - 1 consoles to the null chardev.

Add an index argument, so we can use a specific serial console as
default.

Signed-off-by: Philippe Mathieu-Daudé 
---
v2:
- renamed 'console_index', added docstring (Cleber)
- reworded description (pm215)
---
 python/qemu/machine.py | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 734efd8536..ef9f5b213f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -241,6 +241,8 @@ class QEMUMachine(object):
  'chardev=mon,mode=control'])
 if self._machine is not None:
 args.extend(['-machine', self._machine])
+for i in range(self._console_index):
+args.extend(['-serial', 'null'])
 if self._console_set:
 self._console_address = os.path.join(self._sock_dir,
  self._name + "-console.sock")
@@ -527,7 +529,7 @@ class QEMUMachine(object):
 """
 self._machine = machine_type
 
-def set_console(self, device_type=None):
+def set_console(self, device_type=None, console_index=0):
 """
 Sets the device type for a console device
 
@@ -548,9 +550,14 @@ class QEMUMachine(object):
 chardev:console" command line argument will
 be used instead, resorting to the machine's
 default device type.
+@param console_index: the index of the console device to use.
+  If not zero, the command line will create
+  'index - 1' consoles and connect them to
+  the 'null' backing character device.
 """
 self._console_set = True
 self._console_device_type = device_type
+self._console_index = console_index
 
 @property
 def console_socket(self):
-- 
2.21.1

[PATCH v4 1/7] hw/arm/raspi: Remove obsolete use of -smp to set the soc 'enabled-cpus'

[Philippe Mathieu-Daudé]

Since we enabled parallel TCG code generation for softmmu (see
commit 3468b59 "tcg: enable multiple TCG contexts in softmmu")
and its subsequent fix (commit 72649619 "add .min_cpus and
.default_cpus fields to machine_class"), the raspi machines are
restricted to always use their 4 cores:

See in hw/arm/raspi2 (with BCM283X_NCPUS set to 4):

  222 static void raspi2_machine_init(MachineClass *mc)
  223 {
  224 mc->desc = "Raspberry Pi 2";
  230 mc->max_cpus = BCM283X_NCPUS;
  231 mc->min_cpus = BCM283X_NCPUS;
  232 mc->default_cpus = BCM283X_NCPUS;
  235 };
  236 DEFINE_MACHINE("raspi2", raspi2_machine_init)

We can no longer use the -smp option, as we get:

  $ qemu-system-arm -M raspi2 -smp 1
  qemu-system-arm: Invalid SMP CPUs 1. The min CPUs supported by machine 
'raspi2' is 4

Since we can not set the TYPE_BCM283x SOC "enabled-cpus" with -smp,
remove the unuseful code.

We can achieve the same by using the '-global bcm2836.enabled-cpus=1'
option.

Reported-by: Laurent Bonnans 
Signed-off-by: Philippe Mathieu-Daudé 
---
Cc: Emilio G. Cota 
Cc: Richard Henderson 
Cc: Andrew Baumann 
Cc: Eduardo Habkost 
---
 hw/arm/raspi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 6a510aafc1..3996f6c63a 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -192,8 +192,6 @@ static void raspi_init(MachineState *machine, int version)
 /* Setup the SOC */
 object_property_add_const_link(OBJECT(>soc), "ram", OBJECT(>ram),
_abort);
-object_property_set_int(OBJECT(>soc), machine->smp.cpus, "enabled-cpus",
-_abort);
 int board_rev = version == 3 ? 0xa02082 : 0xa21041;
 object_property_set_int(OBJECT(>soc), board_rev, "board-rev",
 _abort);
-- 
2.21.1

[PATCH v4 0/7] hw/arm/raspi: Run U-Boot on the raspi machines

[Philippe Mathieu-Daudé]

Following Laurent report:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg639950.html

The SYS_timer is already merged, see:
https://git.qemu.org/?p=qemu.git;a=commit;h=d05be883fc
"hw/timer/bcm2835: Add the BCM2835 SYS_timer"

The first patch should fix Laurent other issue.
Then few python patches are require to break into U-Boot console,
and the last patches add U-Boot tests for Raspi2 and Raspi3.

Laurent, if you successfully test U-Boot with this patchset again,
do you mind replying with a "Tested-by:" tag?

Regards,

Phil.

Since v3:
- rewrote '-smp' fix.
- tests use Debian 'trustable' u-boot.elf

previous feedbacks from Peter on v3:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg655415.html

v3: https://www.mail-archive.com/qemu-devel@nongnu.org/msg653807.html
Supersedes: <20191019234715.25750-1-f4...@amsat.org>

Philippe Mathieu-Daudé (7):
  hw/arm/raspi: Remove obsolete use of -smp to set the soc
'enabled-cpus'
  Acceptance tests: Extract _console_interaction()
  Acceptance tests: Add interrupt_interactive_console_until_pattern()
  python/qemu/machine: Allow to use other serial consoles than default
  tests/boot_linux_console: Test booting U-Boot on the Raspberry Pi 2
  tests/boot_linux_console: Test booting U-Boot on the Raspberry Pi 3
  tests/boot_linux_console: Tag Emcraft Smartfusion2 as running 'u-boot'

 hw/arm/raspi.c|  2 -
 python/qemu/machine.py|  9 +++-
 tests/acceptance/avocado_qemu/__init__.py | 59 +--
 tests/acceptance/boot_linux_console.py| 54 +
 4 files changed, 107 insertions(+), 17 deletions(-)

-- 
2.21.1

Re: [PATCH v4 00/18] hw/avr: Introduce few Arduino boards

[Philippe Mathieu-Daudé]

On Mon, Jan 20, 2020 at 11:01 PM Philippe Mathieu-Daudé  wrote:
>
> Hi,
>
> This series add the arduino boards, aiming at removing the
> 'sample' board that doesn't follow any specification.
>
> Since v3:
> - Rebased on Michael's v41
> - Drop 'extram' unused field (Igor)
> - Renamed devices AVR -> Atmel (Aleksandar)
>   (I haven't renamed structure names to ease review)
>
> Since v2:
> - rebased on Michael's v40
>
> Since v1:
> - Addressed Igor comments
> - Addressed Aleksandar comments
> - Fixed UART issue (was due to IRQ shifted by 2 in CPU)
>
> Since Michael's work is not yet merged, Various of my patches
> - which are trivials or simple renames - could be squashed
> directly on his patches, if we ever care.
> [I believe sending this patches is easier/quicker than keeping
> asking Michael to respin his series infinitely].
>
> Michael, do you mind testing it? The full series is available
> here: https://gitlab.com/philmd/qemu/commits/arduino-v4
>
> Regards,
>
> Phil.
>
> Obsoletes: <20191229224505.24466-1-f4...@amsat.org>

Argh the correct tag is:
Supersedes: <20191229224505.24466-1-f4...@amsat.org>

Paolo: will this work? (replying to the cover)

> Based-on: <20200118191416.19934-1-mrol...@gmail.com>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg671707.html
>
> Philippe Mathieu-Daudé (18):
>   MAINTAINERS: Move machine test to the machine section (not ARCH one)
>   MAINTAINERS: Move the AVR machines in new section (not within ARM)
>   tests/acceptance: Do not set the machine type manually
>   tests/acceptance: Keep multilines comment consistent with other tests
>   hw/char/avr: Reduce USART I/O size
>   hw/timer/avr_timer16: Rename memory region debugging name
>   hw/misc/avr_mask: Remove unused include
>   hw/avr/Makefile: Use CONFIG_AVR_SAMPLE variable
>   hw/char: Rename avr_usart -> atmel_usart
>   hw/timer: Rename avr_timer16 -> atmel_timer16
>   hw/misc: Rename avr_mask -> atmel_power
>   hw/avr: Introduce ATMEL_ATMEGA_MCU config
>   hw/avr: Add some ATmega microcontrollers
>   hw/avr: Add some Arduino boards
>   tests/boot-serial-test: Test some Arduino boards (AVR based)
>   tests/acceptance: Test the Arduino MEGA2560 board
>   hw/avr: Remove the unrealistic AVR 'sample' board
>   .travis.yml: Run the AVR acceptance tests

Re: [PATCH 7/9] cputlb: Partially merge tlb_dyn_init into tlb_init

[Alistair Francis]

On Thu, Jan 9, 2020 at 12:52 PM Richard Henderson
 wrote:
>
> Merge into the only caller, but at the same time split
> out tlb_mmu_init to initialize a single tlb entry.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  accel/tcg/cputlb.c | 33 -
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index e60e501334..c7c34b185b 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -97,22 +97,6 @@ static void tlb_window_reset(CPUTLBDesc *desc, int64_t ns,
>  desc->window_max_entries = max_entries;
>  }
>
> -static void tlb_dyn_init(CPUArchState *env)
> -{
> -int i;
> -
> -for (i = 0; i < NB_MMU_MODES; i++) {
> -CPUTLBDesc *desc = _tlb(env)->d[i];
> -size_t n_entries = 1 << CPU_TLB_DYN_DEFAULT_BITS;
> -
> -tlb_window_reset(desc, get_clock_realtime(), 0);
> -desc->n_used_entries = 0;
> -env_tlb(env)->f[i].mask = (n_entries - 1) << CPU_TLB_ENTRY_BITS;
> -env_tlb(env)->f[i].table = g_new(CPUTLBEntry, n_entries);
> -env_tlb(env)->d[i].iotlb = g_new(CPUIOTLBEntry, n_entries);
> -}
> -}
> -
>  /**
>   * tlb_mmu_resize_locked() - perform TLB resize bookkeeping; resize if 
> necessary
>   * @desc: The CPUTLBDesc portion of the TLB
> @@ -247,6 +231,17 @@ static void tlb_flush_one_mmuidx_locked(CPUArchState 
> *env, int mmu_idx)
>  tlb_mmu_flush_locked(desc, fast);
>  }
>
> +static void tlb_mmu_init(CPUTLBDesc *desc, CPUTLBDescFast *fast, int64_t now)
> +{
> +size_t n_entries = 1 << CPU_TLB_DYN_DEFAULT_BITS;
> +
> +tlb_window_reset(desc, now, 0);
> +desc->n_used_entries = 0;
> +fast->mask = (n_entries - 1) << CPU_TLB_ENTRY_BITS;
> +fast->table = g_new(CPUTLBEntry, n_entries);
> +desc->iotlb = g_new(CPUIOTLBEntry, n_entries);
> +}
> +
>  static inline void tlb_n_used_entries_inc(CPUArchState *env, uintptr_t 
> mmu_idx)
>  {
>  env_tlb(env)->d[mmu_idx].n_used_entries++;
> @@ -260,13 +255,17 @@ static inline void tlb_n_used_entries_dec(CPUArchState 
> *env, uintptr_t mmu_idx)
>  void tlb_init(CPUState *cpu)
>  {
>  CPUArchState *env = cpu->env_ptr;
> +int64_t now = get_clock_realtime();
> +int i;
>
>  qemu_spin_init(_tlb(env)->c.lock);
>
>  /* Ensure that cpu_reset performs a full flush.  */
>  env_tlb(env)->c.dirty = ALL_MMUIDX_BITS;
>
> -tlb_dyn_init(env);
> +for (i = 0; i < NB_MMU_MODES; i++) {
> +tlb_mmu_init(_tlb(env)->d[i], _tlb(env)->f[i], now);
> +}
>  }
>
>  /* flush_all_helper: run fn across all cpus
> --
> 2.20.1
>
>

Re: [PATCH] riscv: Fix defination of csr operations

[Alistair Francis]

On Sun, Jan 19, 2020 at 11:55 PM  wrote:
>
> From: Ian Jiang 
>
> There is a mistake in defining CSR operations for pmpcfg registers.
> This patch fixes the bug.
>
> Signed-off-by: Ian Jiang 

Looks good! Thanks for the patch.

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index da02f9f0b1..e07b5267be 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -948,7 +948,7 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>  [CSR_SATP] ={ smode, read_satp,write_satp
> },
>
>  /* Physical Memory Protection */
> -[CSR_PMPCFG0  ... CSR_PMPADDR9] =  { pmp,   read_pmpcfg,  write_pmpcfg   
> },
> +[CSR_PMPCFG0  ... CSR_PMPCFG3] =  { pmp,   read_pmpcfg,  write_pmpcfg   
> },
>  [CSR_PMPADDR0 ... CSR_PMPADDR15] = { pmp,   read_pmpaddr, write_pmpaddr  
> },
>
>  /* Performance Counters */
> --
> 2.17.1
>
>

Re: riscv: How to get more CSR information in debug trace?

[Alistair Francis]

On Fri, Jan 17, 2020 at 11:36 PM Ian Jiang  wrote:
>
> The following registers are given in QEMU debug trace with "-d cpu" parameter.
> pc   1000
> mhartid  
> mstatus  
> mip  0x0
> mie  
> mideleg  
> medeleg  
> mtvec
> mepc 
> mcause   
>
> I want more information of other CSRs, such as sstatus, misa, pmpconfig0.
> How to get debug trace on all CSRs defined in RISC-V specification?

You can use the `info registers` command from the QEMU monitor.
Otherwise you could edit the source code to trace the registers you
are interested in.

The problem is there are so many CSRs we can't print them all.

Alistair

> Thanks!
>
> --
> Ian Jiang

Re: [PATCH 0/9] cputlb: Various cleanups

[Alistair Francis]

On Thu, Jan 9, 2020 at 12:49 PM Richard Henderson
 wrote:
>
> I had a conversation with Alistair Francis at KVM forum about
> being able to represent ASIDs "properly".  This lead to the idea
> that target-specific code might be able to cache TLBs outside of
> the "main" NB_MMU_MODES -- possibly thousands of them.
>
> This goes nowhere near that far.  But it does begin edging toward
> the possibility of having a
>
> struct CPUTLBSaved {
> CPUTLBDesc d;
> CPUTLBDescFast f;
> };
>
> by moving some of the most basic routines to use CPUTLBDesc and
> CPUTLBDescFast directly instead of always using an mmu_idx.
>
> I'm not sure how much time I'll have to go further along these
> lines, but what I have so far still looks like a cleanup.

Thanks for helping with this!

Unfortunately I haven't had a chance to dig into this myself.

Alistair

>
>
> r~
>
>
> Richard Henderson (9):
>   cputlb: Merge tlb_table_flush_by_mmuidx into
> tlb_flush_one_mmuidx_locked
>   cputlb: Make tlb_n_entries private to cputlb.c
>   cputlb: Pass CPUTLBDescFast to tlb_n_entries and sizeof_tlb
>   cputlb: Hoist tlb portions in tlb_mmu_resize_locked
>   cputlb: Hoist tlb portions in tlb_flush_one_mmuidx_locked
>   cputlb: Split out tlb_mmu_flush_locked
>   cputlb: Partially merge tlb_dyn_init into tlb_init
>   cputlb: Initialize tlbs as flushed
>   cputlb: Hoist timestamp outside of loops over tlbs
>
>  include/exec/cpu_ldst.h |   5 --
>  accel/tcg/cputlb.c  | 120 +---
>  2 files changed, 64 insertions(+), 61 deletions(-)
>
> --
> 2.20.1
>
>

Re: [PATCH 9/9] cputlb: Hoist timestamp outside of loops over tlbs

[Alistair Francis]

On Thu, Jan 9, 2020 at 12:55 PM Richard Henderson
 wrote:
>
> Do not call get_clock_realtime() in tlb_mmu_resize_locked,
> but hoist outside of any loop over a set of tlbs.  This is
> only two (indirect) callers, tlb_flush_by_mmuidx_async_work
> and tlb_flush_page_locked, so not onerous.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  accel/tcg/cputlb.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 761e9d44d7..9f6cb36921 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -137,12 +137,12 @@ static void tlb_window_reset(CPUTLBDesc *desc, int64_t 
> ns,
>   * high), since otherwise we are likely to have a significant amount of
>   * conflict misses.
>   */
> -static void tlb_mmu_resize_locked(CPUTLBDesc *desc, CPUTLBDescFast *fast)
> +static void tlb_mmu_resize_locked(CPUTLBDesc *desc, CPUTLBDescFast *fast,
> +  int64_t now)
>  {
>  size_t old_size = tlb_n_entries(fast);
>  size_t rate;
>  size_t new_size = old_size;
> -int64_t now = get_clock_realtime();
>  int64_t window_len_ms = 100;
>  int64_t window_len_ns = window_len_ms * 1000 * 1000;
>  bool window_expired = now > desc->window_begin_ns + window_len_ns;
> @@ -222,12 +222,13 @@ static void tlb_mmu_flush_locked(CPUTLBDesc *desc, 
> CPUTLBDescFast *fast)
>  memset(desc->vtable, -1, sizeof(desc->vtable));
>  }
>
> -static void tlb_flush_one_mmuidx_locked(CPUArchState *env, int mmu_idx)
> +static void tlb_flush_one_mmuidx_locked(CPUArchState *env, int mmu_idx,
> +int64_t now)
>  {
>  CPUTLBDesc *desc = _tlb(env)->d[mmu_idx];
>  CPUTLBDescFast *fast = _tlb(env)->f[mmu_idx];
>
> -tlb_mmu_resize_locked(desc, fast);
> +tlb_mmu_resize_locked(desc, fast, now);
>  tlb_mmu_flush_locked(desc, fast);
>  }
>
> @@ -310,6 +311,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, 
> run_on_cpu_data data)
>  CPUArchState *env = cpu->env_ptr;
>  uint16_t asked = data.host_int;
>  uint16_t all_dirty, work, to_clean;
> +int64_t now = get_clock_realtime();
>
>  assert_cpu_is_self(cpu);
>
> @@ -324,7 +326,7 @@ static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, 
> run_on_cpu_data data)
>
>  for (work = to_clean; work != 0; work &= work - 1) {
>  int mmu_idx = ctz32(work);
> -tlb_flush_one_mmuidx_locked(env, mmu_idx);
> +tlb_flush_one_mmuidx_locked(env, mmu_idx, now);
>  }
>
>  qemu_spin_unlock(_tlb(env)->c.lock);
> @@ -446,7 +448,7 @@ static void tlb_flush_page_locked(CPUArchState *env, int 
> midx,
>  tlb_debug("forcing full flush midx %d ("
>TARGET_FMT_lx "/" TARGET_FMT_lx ")\n",
>midx, lp_addr, lp_mask);
> -tlb_flush_one_mmuidx_locked(env, midx);
> +tlb_flush_one_mmuidx_locked(env, midx, get_clock_realtime());
>  } else {
>  if (tlb_flush_entry_locked(tlb_entry(env, midx, page), page)) {
>  tlb_n_used_entries_dec(env, midx);
> --
> 2.20.1
>
>

Re: [PATCH 8/9] cputlb: Initialize tlbs as flushed

[Alistair Francis]

On Thu, Jan 9, 2020 at 12:57 PM Richard Henderson
 wrote:
>
> There's little point in leaving these data structures half initialized,
> and relying on a flush to be done during reset.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  accel/tcg/cputlb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c7c34b185b..761e9d44d7 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -240,6 +240,7 @@ static void tlb_mmu_init(CPUTLBDesc *desc, CPUTLBDescFast 
> *fast, int64_t now)
>  fast->mask = (n_entries - 1) << CPU_TLB_ENTRY_BITS;
>  fast->table = g_new(CPUTLBEntry, n_entries);
>  desc->iotlb = g_new(CPUIOTLBEntry, n_entries);
> +tlb_mmu_flush_locked(desc, fast);
>  }
>
>  static inline void tlb_n_used_entries_inc(CPUArchState *env, uintptr_t 
> mmu_idx)
> @@ -260,8 +261,8 @@ void tlb_init(CPUState *cpu)
>
>  qemu_spin_init(_tlb(env)->c.lock);
>
> -/* Ensure that cpu_reset performs a full flush.  */
> -env_tlb(env)->c.dirty = ALL_MMUIDX_BITS;
> +/* All tlbs are initialized flushed. */
> +env_tlb(env)->c.dirty = 0;
>
>  for (i = 0; i < NB_MMU_MODES; i++) {
>  tlb_mmu_init(_tlb(env)->d[i], _tlb(env)->f[i], now);
> --
> 2.20.1
>
>

Re: [PATCH 6/9] cputlb: Split out tlb_mmu_flush_locked

[Alistair Francis]

On Thu, Jan 9, 2020 at 12:52 PM Richard Henderson
 wrote:
>
> We will want to be able to flush a tlb without resizing.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  accel/tcg/cputlb.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index eff427f137..e60e501334 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -228,12 +228,8 @@ static void tlb_mmu_resize_locked(CPUTLBDesc *desc, 
> CPUTLBDescFast *fast)
>  }
>  }
>
> -static void tlb_flush_one_mmuidx_locked(CPUArchState *env, int mmu_idx)
> +static void tlb_mmu_flush_locked(CPUTLBDesc *desc, CPUTLBDescFast *fast)
>  {
> -CPUTLBDesc *desc = _tlb(env)->d[mmu_idx];
> -CPUTLBDescFast *fast = _tlb(env)->f[mmu_idx];
> -
> -tlb_mmu_resize_locked(desc, fast);
>  desc->n_used_entries = 0;
>  desc->large_page_addr = -1;
>  desc->large_page_mask = -1;
> @@ -242,6 +238,15 @@ static void tlb_flush_one_mmuidx_locked(CPUArchState 
> *env, int mmu_idx)
>  memset(desc->vtable, -1, sizeof(desc->vtable));
>  }
>
> +static void tlb_flush_one_mmuidx_locked(CPUArchState *env, int mmu_idx)
> +{
> +CPUTLBDesc *desc = _tlb(env)->d[mmu_idx];
> +CPUTLBDescFast *fast = _tlb(env)->f[mmu_idx];
> +
> +tlb_mmu_resize_locked(desc, fast);
> +tlb_mmu_flush_locked(desc, fast);
> +}
> +
>  static inline void tlb_n_used_entries_inc(CPUArchState *env, uintptr_t 
> mmu_idx)
>  {
>  env_tlb(env)->d[mmu_idx].n_used_entries++;
> --
> 2.20.1
>
>

Re: [PATCH 5/9] cputlb: Hoist tlb portions in tlb_flush_one_mmuidx_locked

[Alistair Francis]

On Thu, Jan 9, 2020 at 12:55 PM Richard Henderson
 wrote:
>
> No functional change, but the smaller expressions make
> the code easier to read.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  accel/tcg/cputlb.c | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c7dc1dc85a..eff427f137 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -230,15 +230,16 @@ static void tlb_mmu_resize_locked(CPUTLBDesc *desc, 
> CPUTLBDescFast *fast)
>
>  static void tlb_flush_one_mmuidx_locked(CPUArchState *env, int mmu_idx)
>  {
> -tlb_mmu_resize_locked(_tlb(env)->d[mmu_idx], 
> _tlb(env)->f[mmu_idx]);
> -env_tlb(env)->d[mmu_idx].n_used_entries = 0;
> -env_tlb(env)->d[mmu_idx].large_page_addr = -1;
> -env_tlb(env)->d[mmu_idx].large_page_mask = -1;
> -env_tlb(env)->d[mmu_idx].vindex = 0;
> -memset(env_tlb(env)->f[mmu_idx].table, -1,
> -   sizeof_tlb(_tlb(env)->f[mmu_idx]));
> -memset(env_tlb(env)->d[mmu_idx].vtable, -1,
> -   sizeof(env_tlb(env)->d[0].vtable));
> +CPUTLBDesc *desc = _tlb(env)->d[mmu_idx];
> +CPUTLBDescFast *fast = _tlb(env)->f[mmu_idx];
> +
> +tlb_mmu_resize_locked(desc, fast);
> +desc->n_used_entries = 0;
> +desc->large_page_addr = -1;
> +desc->large_page_mask = -1;
> +desc->vindex = 0;
> +memset(fast->table, -1, sizeof_tlb(fast));
> +memset(desc->vtable, -1, sizeof(desc->vtable));
>  }
>
>  static inline void tlb_n_used_entries_inc(CPUArchState *env, uintptr_t 
> mmu_idx)
> --
> 2.20.1
>
>

Re: [PATCH v41 00/21] QEMU AVR 8 bit cores

[Philippe Mathieu-Daudé]

On Sat, Jan 18, 2020 at 8:21 PM Michael Rolnik  wrote:
>
> This series of patches adds 8bit AVR cores to QEMU.
> All instruction, except BREAK/DES/SPM/SPMX, are implemented. Not fully tested 
> yet.
> However I was able to execute simple code with functions. e.g fibonacci 
> calculation.
> This series of patches include a non real, sample board.
> No fuses support yet. PC is set to 0 at reset.
>
[...]
> Michael Rolnik (21):
>   target/avr: Add outward facing interfaces and core CPU logic
>   target/avr: Add instruction helpers
>   target/avr: Add instruction translation - Registers definition
>   target/avr: Add instruction translation - Arithmetic and Logic
> Instructions
>   target/avr: Add instruction translation - Branch Instructions
>   target/avr: Add instruction translation - Data Transfer Instructions
>   target/avr: Add instruction translation - Bit and Bit-test
> Instructions
>   target/avr: Add instruction translation - MCU Control Instructions
>   target/avr: Add instruction translation - CPU main translation
> function
>   target/avr: Add instruction disassembly function
>   hw/avr: Add limited support for USART peripheral
>   hw/avr: Add limited support for 16 bit timer peripheral
>   hw/avr: Add dummy mask device
>   hw/avr: Add example board configuration
>   target/avr: Add section about AVR into QEMU documentation
>   target/avr: Register AVR support with the rest of QEMU
>   target/avr: Add machine none test
>   target/avr: Update build system
>   target/avr: Add boot serial test
>   target/avr: Add Avocado test
>   target/avr: Update MAINTAINERS file
>
>  qemu-doc.texi|   51 +
>  configure|7 +
>  default-configs/avr-softmmu.mak  |5 +
>  qapi/machine.json|3 +-
>  include/disas/dis-asm.h  |   19 +
>  include/elf.h|2 +
>  include/hw/char/avr_usart.h  |   93 +
>  include/hw/elf_ops.h |6 +-
>  include/hw/loader.h  |6 +-
>  include/hw/misc/avr_mask.h   |   47 +
>  include/hw/timer/avr_timer16.h   |   94 +
>  include/sysemu/arch_init.h   |1 +
>  target/avr/cpu-param.h   |   37 +
>  target/avr/cpu-qom.h |   54 +
>  target/avr/cpu.h |  259 +++
>  target/avr/helper.h  |   29 +
>  arch_init.c  |2 +
>  hw/avr/sample.c  |  295 +++
>  hw/char/avr_usart.c  |  320 
>  hw/core/loader.c |   15 +-
>  hw/misc/avr_mask.c   |  112 ++
>  hw/riscv/boot.c  |2 +-
>  hw/timer/avr_timer16.c   |  602 ++
>  target/avr/cpu.c |  826 
>  target/avr/disas.c   |  245 +++
>  target/avr/gdbstub.c |   84 +
>  target/avr/helper.c  |  347 
>  target/avr/machine.c |  121 ++
>  target/avr/translate.c   | 2997 ++
>  tests/qtest/boot-serial-test.c   |   10 +
>  tests/qtest/machine-none-test.c  |1 +
>  MAINTAINERS  |   21 +
>  gdb-xml/avr-cpu.xml  |   49 +
>  hw/Kconfig   |1 +
>  hw/avr/Kconfig   |6 +
>  hw/avr/Makefile.objs |1 +
>  hw/char/Kconfig  |3 +
>  hw/char/Makefile.objs|1 +
>  hw/misc/Kconfig  |3 +
>  hw/misc/Makefile.objs|2 +
>  hw/timer/Kconfig |3 +
>  hw/timer/Makefile.objs   |2 +
>  target/avr/Makefile.objs |   34 +
>  target/avr/insn.decode   |  183 ++
>  tests/acceptance/machine_avr6.py |   53 +
>  tests/qtest/Makefile.include |2 +
>  46 files changed, 7044 insertions(+), 12 deletions(-)

Series:
Tested-by: Philippe Mathieu-Daudé 

[PATCH v4 13/18] hw/avr: Add some ATmega microcontrollers

[Philippe Mathieu-Daudé]

Add some microcontrollers from the megaAVR family (ATmega series):

- middle range: ATmega168 and ATmega328
- high range: ATmega1280 and ATmega2560

For product comparison:
  https://www.microchip.com/wwwproducts/ProductCompare/ATmega168P/ATmega328P
  https://www.microchip.com/wwwproducts/ProductCompare/ATmega1280/ATmega2560

Datasheets:
  
http://ww1.microchip.com/downloads/en/DeviceDoc/ATmega48A-PA-88A-PA-168A-PA-328-P-DS-DS40002061A.pdf
  
http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-2549-8-bit-AVR-Microcontroller-ATmega640-1280-1281-2560-2561_datasheet.pdf

Signed-off-by: Philippe Mathieu-Daudé 
---
v2:
- Reword description adding more information (Aleksandar)
- Use DEFINE_TYPES and memory_region_init_ram (Igor)

Cc: Igor Mammedov 
---
 hw/avr/atmel_atmega.h |  48 +
 hw/avr/atmel_atmega.c | 464 ++
 hw/avr/Makefile.objs  |   1 +
 3 files changed, 513 insertions(+)
 create mode 100644 hw/avr/atmel_atmega.h
 create mode 100644 hw/avr/atmel_atmega.c

diff --git a/hw/avr/atmel_atmega.h b/hw/avr/atmel_atmega.h
new file mode 100644
index 00..391b8b1bf8
--- /dev/null
+++ b/hw/avr/atmel_atmega.h
@@ -0,0 +1,48 @@
+/*
+ * QEMU ATmega MCU
+ *
+ * Copyright (c) 2019 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_AVR_ATMEL_ATMEGA_H
+#define HW_AVR_ATMEL_ATMEGA_H
+
+#include "hw/char/atmel_usart.h"
+#include "hw/timer/atmel_timer16.h"
+#include "hw/misc/atmel_power.h"
+#include "target/avr/cpu.h"
+
+#define TYPE_ATMEGA_MCU "ATmega"
+#define TYPE_ATMEGA168_MCU  "ATmega168"
+#define TYPE_ATMEGA328_MCU  "ATmega328"
+#define TYPE_ATMEGA1280_MCU "ATmega1280"
+#define TYPE_ATMEGA2560_MCU "ATmega2560"
+
+#define ATMEGA_MCU(obj) OBJECT_CHECK(AtmegaMcuState, (obj), TYPE_ATMEGA_MCU)
+
+#define POWER_MAX 2
+#define USART_MAX 4
+#define TIMER_MAX 6
+#define GPIO_MAX 12
+
+typedef struct AtmegaMcuState {
+/*< private >*/
+SysBusDevice parent_obj;
+/*< public >*/
+
+AVRCPU cpu;
+MemoryRegion flash;
+MemoryRegion eeprom;
+MemoryRegion sram;
+DeviceState *io;
+AVRMaskState pwr[POWER_MAX];
+AVRUsartState usart[USART_MAX];
+AVRTimer16State timer[TIMER_MAX];
+uint64_t xtal_freq_hz;
+} AtmegaMcuState;
+
+#endif /* HW_AVR_ATMEL_ATMEGA_H */
diff --git a/hw/avr/atmel_atmega.c b/hw/avr/atmel_atmega.c
new file mode 100644
index 00..18ab0b7906
--- /dev/null
+++ b/hw/avr/atmel_atmega.c
@@ -0,0 +1,464 @@
+/*
+ * QEMU ATmega MCU
+ *
+ * Copyright (c) 2019 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "exec/memory.h"
+#include "exec/address-spaces.h"
+#include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+#include "hw/boards.h" /* FIXME memory_region_allocate_system_memory for sram 
*/
+#include "hw/misc/unimp.h"
+#include "atmel_atmega.h"
+
+enum AtmegaPeripheral {
+POWER0, POWER1,
+GPIOA, GPIOB, GPIOC, GPIOD, GPIOE, GPIOF,
+GPIOG, GPIOH, GPIOI, GPIOJ, GPIOK, GPIOL,
+USART0, USART1, USART2, USART3,
+TIMER0, TIMER1, TIMER2, TIMER3, TIMER4, TIMER5,
+PERIFMAX
+};
+
+#define GPIO(n) (n + GPIOA)
+#define USART(n)(n + USART0)
+#define TIMER(n)(n + TIMER0)
+#define POWER(n)(n + POWER0)
+
+typedef struct {
+uint16_t addr;
+enum AtmegaPeripheral power_index;
+uint8_t power_bit;
+/* timer specific */
+uint16_t intmask_addr;
+uint16_t intflag_addr;
+bool is_timer16;
+} peripheral_cfg;
+
+typedef struct AtmegaMcuClass {
+/*< private >*/
+SysBusDeviceClass parent_class;
+/*< public >*/
+const char *uc_name;
+const char *cpu_type;
+size_t flash_size;
+size_t eeprom_size;
+size_t sram_size;
+size_t io_size;
+size_t gpio_count;
+size_t adc_count;
+const uint8_t *irq;
+const peripheral_cfg *dev;
+} AtmegaMcuClass;
+
+#define ATMEGA_MCU_CLASS(klass) \
+OBJECT_CLASS_CHECK(AtmegaMcuClass, (klass), TYPE_ATMEGA_MCU)
+#define ATMEGA_MCU_GET_CLASS(obj) \
+OBJECT_GET_CLASS(AtmegaMcuClass, (obj), TYPE_ATMEGA_MCU)
+
+static const peripheral_cfg dev168_328[PERIFMAX] = {
+[USART0]= {  0xc0, POWER0, 1 },
+[TIMER2]= {  0xb0, POWER0, 6, 0x70, 0x37, false },
+[TIMER1]= {  0x80, POWER0, 3, 0x6f, 0x36, true },
+[POWER0]= {  0x64 },
+[TIMER0]= {  0x44, POWER0, 5, 0x6e, 0x35, false },
+[GPIOD] = {  0x29 },
+[GPIOC] = {  0x26 },
+[GPIOB] = {  0x23 },
+}, dev1280_2560[PERIFMAX] = {
+[USART3]= { 0x130, POWER1, 2 },
+[TIMER5]= { 0x120, POWER1, 5, 0x73, 0x3a, true },
+ 

[PATCH v4 18/18] .travis.yml: Run the AVR acceptance tests

[Philippe Mathieu-Daudé]

We have one test so far, and it is very fast:

  $ avocado --show=app run -t arch:avr tests/acceptance/
  (1/1) tests/acceptance/machine_avr6.py:AVR6Machine.test_freertos: PASS (2.13 
s)
  RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
  JOB TIME   : 2.30 s

Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a0f1..2301c9221e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -268,7 +268,7 @@ matrix:
 
 # Acceptance (Functional) tests
 - env:
-- CONFIG="--python=/usr/bin/python3 
--target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
+- CONFIG="--python=/usr/bin/python3 
--target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu,avr-softmmu"
 - TEST_CMD="make check-acceptance"
   after_script:
 - python3 -c 'import json; r = 
json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for 
t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
-- 
2.21.1

[PATCH v4 17/18] hw/avr: Remove the unrealistic AVR 'sample' board

[Philippe Mathieu-Daudé]

The 'arduino-mega-2560-v3' board fully replace the 'sample'
board.

Signed-off-by: Philippe Mathieu-Daudé 
---
!squash?
---
 default-configs/avr-softmmu.mak |   1 -
 hw/avr/sample.c | 295 
 tests/qtest/boot-serial-test.c  |   1 -
 hw/avr/Kconfig  |   4 -
 hw/avr/Makefile.objs|   1 -
 5 files changed, 302 deletions(-)
 delete mode 100644 hw/avr/sample.c

diff --git a/default-configs/avr-softmmu.mak b/default-configs/avr-softmmu.mak
index d8d1d8e6fe..80218add98 100644
--- a/default-configs/avr-softmmu.mak
+++ b/default-configs/avr-softmmu.mak
@@ -3,4 +3,3 @@
 # Boards:
 #
 CONFIG_ARDUINO=y
-CONFIG_AVR_SAMPLE=y
diff --git a/hw/avr/sample.c b/hw/avr/sample.c
deleted file mode 100644
index 19b8c067e5..00
--- a/hw/avr/sample.c
+++ /dev/null
@@ -1,295 +0,0 @@
-/*
- * QEMU AVR CPU
- *
- * Copyright (c) 2019 Michael Rolnik
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library 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
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see
- * 
- */
-
-/*
- *  NOTE:
- *  This is not a real AVR board, this is an example!
- *  The CPU is an approximation of an ATmega2560, but is missing various
- *  built-in peripherals.
- *
- *  This example board loads provided binary file into flash memory and
- *  executes it from 0x address in the code memory space.
- *
- *  Currently used for AVR CPU validation
- *
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "qemu-common.h"
-#include "cpu.h"
-#include "hw/hw.h"
-#include "sysemu/sysemu.h"
-#include "sysemu/qtest.h"
-#include "ui/console.h"
-#include "hw/boards.h"
-#include "hw/loader.h"
-#include "qemu/error-report.h"
-#include "exec/address-spaces.h"
-#include "include/hw/sysbus.h"
-#include "include/hw/char/atmel_usart.h"
-#include "include/hw/timer/atmel_timer16.h"
-#include "include/hw/misc/atmel_power.h"
-#include "elf.h"
-#include "hw/misc/unimp.h"
-
-#define SIZE_FLASH 0x0004
-#define SIZE_SRAM 0x2000
-/*
- * Size of additional "external" memory, as if the AVR were configured to use
- * an external RAM chip.
- * Note that the configuration registers that normally enable this feature are
- * unimplemented.
- */
-#define SIZE_EXMEM 0x
-
-/* Offsets of peripherals in emulated memory space (i.e. not host addresses)  
*/
-#define PRR0_BASE 0x64
-#define PRR1_BASE 0x65
-#define USART_BASE 0xc0
-#define TIMER1_BASE 0x80
-#define TIMER1_IMSK_BASE 0x6f
-#define TIMER1_IFR_BASE 0x36
-
-/* Interrupt numbers used by peripherals */
-#define USART_RXC_IRQ 24
-#define USART_DRE_IRQ 25
-#define USART_TXC_IRQ 26
-
-#define TIMER1_CAPT_IRQ 15
-#define TIMER1_COMPA_IRQ 16
-#define TIMER1_COMPB_IRQ 17
-#define TIMER1_COMPC_IRQ 18
-#define TIMER1_OVF_IRQ 19
-
-/*  Power reduction */
-#define PRR1_BIT_PRTIM5 0x05/*  Timer/Counter5  */
-#define PRR1_BIT_PRTIM4 0x04/*  Timer/Counter4  */
-#define PRR1_BIT_PRTIM3 0x03/*  Timer/Counter3  */
-#define PRR1_BIT_PRUSART3   0x02/*  USART3  */
-#define PRR1_BIT_PRUSART2   0x01/*  USART2  */
-#define PRR1_BIT_PRUSART1   0x00/*  USART1  */
-
-#define PRR0_BIT_PRTWI  0x06/*  TWI */
-#define PRR0_BIT_PRTIM2 0x05/*  Timer/Counter2  */
-#define PRR0_BIT_PRTIM0 0x04/*  Timer/Counter0  */
-#define PRR0_BIT_PRTIM1 0x03/*  Timer/Counter1  */
-#define PRR0_BIT_PRSPI  0x02/*  Serial Peripheral Interface */
-#define PRR0_BIT_PRUSART0   0x01/*  USART0  */
-#define PRR0_BIT_PRADC  0x00/*  ADC */
-
-#define configCPU_CLOCK_HZ ((unsigned long)1600)
-
-typedef struct {
-MachineClass parent;
-} SampleMachineClass;
-
-typedef struct {
-MachineState parent;
-MemoryRegion *ram;
-MemoryRegion *flash;
-AVRUsartState *usart0;
-AVRTimer16State *timer1;
-AVRMaskState *prr[2];
-} SampleMachineState;
-
-#define TYPE_SAMPLE_MACHINE MACHINE_TYPE_NAME("sample")
-
-#define SAMPLE_MACHINE(obj) \
-OBJECT_CHECK(SampleMachineState, obj, TYPE_SAMPLE_MACHINE)
-#define SAMPLE_MACHINE_GET_CLASS(obj) \
-OBJECT_GET_CLASS(SampleMachineClass, obj, TYPE_SAMPLE_MACHINE)
-#define SAMPLE_MACHINE_CLASS(klass) \
-OBJECT_CLASS_CHECK(SampleMachineClass, klass, TYPE_SAMPLE_MACHINE)
-
-static void sample_init(MachineState *machine)
-{
-SampleMachineState *sms = SAMPLE_MACHINE(machine);
-MemoryRegion *system_memory = get_system_memory();
-

[PATCH v4 16/18] tests/acceptance: Test the Arduino MEGA2560 board

[Philippe Mathieu-Daudé]

As the path name demonstrates, the FreeRTOS tests target a
board based on a ATMega2560 MCU. We have one, the Arduino
MEGA2560.

Complementary documentation:

https://feilipu.me/2012/01/15/ethermega-arduino-mega-2560-and-freertos/
https://feilipu.me/2015/11/24/arduino_freertos/ (see 'Compatibility')

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/machine_avr6.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/machine_avr6.py b/tests/acceptance/machine_avr6.py
index 611f6a62a4..b644d2a81c 100644
--- a/tests/acceptance/machine_avr6.py
+++ b/tests/acceptance/machine_avr6.py
@@ -27,7 +27,7 @@ class AVR6Machine(Test):
 def test_freertos(self):
 """
 :avocado: tags=arch:avr
-:avocado: tags=machine:sample
+:avocado: tags=machine:arduino-mega-2560-v3
 """
 """
 
https://github.com/seharris/qemu-avr-tests/raw/master/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf
-- 
2.21.1

[PATCH v4 10/18] hw/timer: Rename avr_timer16 -> atmel_timer16

[Philippe Mathieu-Daudé]

AVR is the architecture, Atmel the manufacturer.

Suggested-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
!squash
---
 include/hw/timer/{avr_timer16.h => atmel_timer16.h} | 10 +-
 hw/avr/sample.c |  2 +-
 hw/timer/{avr_timer16.c => atmel_timer16.c} |  4 ++--
 MAINTAINERS |  4 ++--
 hw/avr/Kconfig  |  2 +-
 hw/timer/Kconfig|  2 +-
 hw/timer/Makefile.objs  |  2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)
 rename include/hw/timer/{avr_timer16.h => atmel_timer16.h} (92%)
 rename hw/timer/{avr_timer16.c => atmel_timer16.c} (99%)

diff --git a/include/hw/timer/avr_timer16.h b/include/hw/timer/atmel_timer16.h
similarity index 92%
rename from include/hw/timer/avr_timer16.h
rename to include/hw/timer/atmel_timer16.h
index 4ae0c64a34..f0516c41cf 100644
--- a/include/hw/timer/avr_timer16.h
+++ b/include/hw/timer/atmel_timer16.h
@@ -1,5 +1,5 @@
 /*
- * AVR 16 bit timer
+ * Atmel AVR 16 bit timer
  *
  * Copyright (c) 2018 University of Kent
  * Author: Ed Robbins
@@ -25,8 +25,8 @@
  * On ATmega640/V-1280/V-1281/V-2560/V-2561/V timers 1, 3, 4 and 5 are 16 bit
  */
 
-#ifndef AVR_TIMER16_H
-#define AVR_TIMER16_H
+#ifndef HW_TIMER_ATMEL_TIMER16_H
+#define HW_TIMER_ATMEL_TIMER16_H
 
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
@@ -40,7 +40,7 @@ enum NextInterrupt {
 CAPT
 };
 
-#define TYPE_AVR_TIMER16 "avr-timer16"
+#define TYPE_AVR_TIMER16 "atmel-timer16"
 #define AVR_TIMER16(obj) \
 OBJECT_CHECK(AVRTimer16State, (obj), TYPE_AVR_TIMER16)
 
@@ -91,4 +91,4 @@ typedef struct AVRTimer16State {
 enum NextInterrupt next_interrupt;
 } AVRTimer16State;
 
-#endif /* AVR_TIMER16_H */
+#endif /* HW_TIMER_ATMEL_TIMER16_H */
diff --git a/hw/avr/sample.c b/hw/avr/sample.c
index ca67c11233..19e9f56f3b 100644
--- a/hw/avr/sample.c
+++ b/hw/avr/sample.c
@@ -45,7 +45,7 @@
 #include "exec/address-spaces.h"
 #include "include/hw/sysbus.h"
 #include "include/hw/char/atmel_usart.h"
-#include "include/hw/timer/avr_timer16.h"
+#include "include/hw/timer/atmel_timer16.h"
 #include "include/hw/misc/avr_mask.h"
 #include "elf.h"
 #include "hw/misc/unimp.h"
diff --git a/hw/timer/avr_timer16.c b/hw/timer/atmel_timer16.c
similarity index 99%
rename from hw/timer/avr_timer16.c
rename to hw/timer/atmel_timer16.c
index a27933a18a..3b06d6e4e5 100644
--- a/hw/timer/avr_timer16.c
+++ b/hw/timer/atmel_timer16.c
@@ -1,5 +1,5 @@
 /*
- * AVR 16 bit timer
+ * Atmel AVR 16 bit timer
  *
  * Copyright (c) 2018 University of Kent
  * Author: Ed Robbins
@@ -32,7 +32,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/timer/avr_timer16.h"
+#include "hw/timer/atmel_timer16.h"
 #include "qemu/log.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index f2e01a6d16..a98d164bc1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -898,8 +898,8 @@ S: Maintained
 F: hw/avr/
 F: hw/char/atmel_usart.c
 F: include/hw/char/atmel_usart.h
-F: hw/timer/avr_timer16.c
-F: include/hw/timer/avr_timer16.h
+F: hw/timer/atmel_timer16.c
+F: include/hw/timer/atmel_timer16.h
 F: hw/misc/avr_mask.c
 F: include/hw/misc/avr_mask.h
 F: tests/acceptance/machine_avr6.py
diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
index 8084a73176..45c7025f09 100644
--- a/hw/avr/Kconfig
+++ b/hw/avr/Kconfig
@@ -1,6 +1,6 @@
 config AVR_SAMPLE
 bool
-select AVR_TIMER16
+select ATMEL_TIMER16
 select ATMEL_USART
 select AVR_MASK
 select UNIMP
diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig
index 2521056dc8..cc66b60ef1 100644
--- a/hw/timer/Kconfig
+++ b/hw/timer/Kconfig
@@ -36,5 +36,5 @@ config CMSDK_APB_DUALTIMER
 bool
 select PTIMER
 
-config AVR_TIMER16
+config ATMEL_TIMER16
 bool
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index af0913ca3b..08a8a5cee9 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -36,4 +36,4 @@ common-obj-$(CONFIG_CMSDK_APB_DUALTIMER) += 
cmsdk-apb-dualtimer.o
 common-obj-$(CONFIG_MSF2) += mss-timer.o
 common-obj-$(CONFIG_RASPI) += bcm2835_systmr.o
 
-obj-$(CONFIG_AVR_TIMER16) += avr_timer16.o
+obj-$(CONFIG_ATMEL_TIMER16) += atmel_timer16.o
-- 
2.21.1

[PATCH v4 14/18] hw/avr: Add some Arduino boards

[Philippe Mathieu-Daudé]

Arduino boards are build with AVR chipsets.
Add some of the popular boards:

- Arduino Duemilanove
- Arduino Uno
- Arduino Mega

For more information:
  https://www.arduino.cc/en/Main/Products
  https://store.arduino.cc/arduino-genuino/most-popular

Reviewed-by: Igor Mammedov 
Signed-off-by: Philippe Mathieu-Daudé 
---
v4:
- Remove unused 'extram*' fields (Igor)

v2:
- Reword description adding more information (Aleksandar)
- Use DEFINE_TYPES (Igor)
---
 default-configs/avr-softmmu.mak |   1 +
 hw/avr/arduino.c| 175 
 hw/avr/Kconfig  |   4 +
 hw/avr/Makefile.objs|   1 +
 4 files changed, 181 insertions(+)
 create mode 100644 hw/avr/arduino.c

diff --git a/default-configs/avr-softmmu.mak b/default-configs/avr-softmmu.mak
index d1e1c28118..d8d1d8e6fe 100644
--- a/default-configs/avr-softmmu.mak
+++ b/default-configs/avr-softmmu.mak
@@ -2,4 +2,5 @@
 
 # Boards:
 #
+CONFIG_ARDUINO=y
 CONFIG_AVR_SAMPLE=y
diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
new file mode 100644
index 00..784246baba
--- /dev/null
+++ b/hw/avr/arduino.c
@@ -0,0 +1,175 @@
+/*
+ * QEMU Arduino boards
+ *
+ * Copyright (c) 2019 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPLv2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+/* TODO: Implement the use of EXTRAM */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "hw/boards.h"
+#include "hw/loader.h"
+#include "elf.h"
+#include "atmel_atmega.h"
+
+typedef struct ArduinoMachineState {
+/*< private >*/
+MachineState parent_obj;
+/*< public >*/
+AtmegaMcuState mcu;
+} ArduinoMachineState;
+
+typedef struct ArduinoMachineClass {
+/*< private >*/
+MachineClass parent_class;
+/*< public >*/
+const char *mcu_type;
+uint64_t xtal_hz;
+} ArduinoMachineClass;
+
+#define TYPE_ARDUINO_MACHINE \
+MACHINE_TYPE_NAME("arduino")
+#define ARDUINO_MACHINE(obj) \
+OBJECT_CHECK(ArduinoMachineState, (obj), TYPE_ARDUINO_MACHINE)
+#define ARDUINO_MACHINE_CLASS(klass) \
+OBJECT_CLASS_CHECK(ArduinoMachineClass, (klass), TYPE_ARDUINO_MACHINE)
+#define ARDUINO_MACHINE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(ArduinoMachineClass, (obj), TYPE_ARDUINO_MACHINE)
+
+static void load_firmware(const char *firmware, uint64_t flash_size)
+{
+const char *filename;
+int bytes_loaded;
+
+/* Load firmware (contents of flash) trying to auto-detect format */
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
+if (filename == NULL) {
+error_report("Unable to find %s", firmware);
+exit(1);
+}
+
+bytes_loaded = load_elf(filename, NULL, NULL, NULL, NULL, NULL, NULL,
+0, EM_NONE, 0, 0);
+if (bytes_loaded < 0) {
+bytes_loaded = load_image_targphys(filename, OFFSET_CODE, flash_size);
+}
+if (bytes_loaded < 0) {
+error_report("Unable to load firmware image %s as ELF or raw binary",
+ firmware);
+exit(1);
+}
+}
+
+static void arduino_machine_init(MachineState *machine)
+{
+ArduinoMachineClass *amc = ARDUINO_MACHINE_GET_CLASS(machine);
+ArduinoMachineState *ams = ARDUINO_MACHINE(machine);
+
+sysbus_init_child_obj(OBJECT(machine), "mcu", >mcu, sizeof(ams->mcu),
+  amc->mcu_type);
+object_property_set_uint(OBJECT(>mcu), amc->xtal_hz,
+ "xtal-frequency-hz", _abort);
+object_property_set_bool(OBJECT(>mcu), true, "realized",
+ _abort);
+
+if (machine->firmware) {
+load_firmware(machine->firmware, memory_region_size(>mcu.flash));
+}
+}
+
+static void arduino_machine_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+
+mc->init = arduino_machine_init;
+mc->default_cpus = 1;
+mc->min_cpus = mc->default_cpus;
+mc->max_cpus = mc->default_cpus;
+mc->no_floppy = 1;
+mc->no_cdrom = 1;
+mc->no_parallel = 1;
+}
+
+static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
+
+/* https://www.arduino.cc/en/Main/ArduinoBoardDuemilanove */
+mc->desc= "Arduino Duemilanove (ATmega168)",
+mc->alias   = "2009";
+amc->mcu_type   = TYPE_ATMEGA168_MCU;
+amc->xtal_hz= 16 * 1000 * 1000;
+};
+
+static void arduino_uno_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
+
+/* https://store.arduino.cc/arduino-uno-rev3 */
+mc->desc= "Arduino UNO (ATmega328P)";
+mc->alias   = "uno";
+amc->mcu_type   = TYPE_ATMEGA328_MCU;
+amc->xtal_hz= 16 * 1000 * 1000;
+};
+
+static void 

[PATCH v4 12/18] hw/avr: Introduce ATMEL_ATMEGA_MCU config

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/avr/Kconfig | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
index 516b89c849..228eae7582 100644
--- a/hw/avr/Kconfig
+++ b/hw/avr/Kconfig
@@ -1,6 +1,9 @@
-config AVR_SAMPLE
+config ATMEL_ATMEGA_MCU
 bool
 select ATMEL_TIMER16
 select ATMEL_USART
 select ATMEL_POWER
+
+config AVR_SAMPLE
+select ATMEL_ATMEGA_MCU
 select UNIMP
-- 
2.21.1

[PATCH v4 15/18] tests/boot-serial-test: Test some Arduino boards (AVR based)

[Philippe Mathieu-Daudé]

The Arduino Duemilanove is based on a AVR5 CPU, while the
Arduino MEGA2560 on a AVR6 CPU.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/qtest/boot-serial-test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index e556f09db8..582a497963 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -113,6 +113,8 @@ typedef struct testdef {
 static testdef_t tests[] = {
 { "alpha", "clipper", "", "PCI:" },
 { "avr", "sample", "", "T", sizeof(bios_avr), NULL, bios_avr },
+{ "avr", "arduino-duemilanove", "", "T", sizeof(bios_avr), NULL, bios_avr 
},
+{ "avr", "arduino-mega-2560-v3", "", "T", sizeof(bios_avr), NULL, 
bios_avr},
 { "ppc", "ppce500", "", "U-Boot" },
 { "ppc", "40p", "-vga none -boot d", "Trying cd:," },
 { "ppc", "g3beige", "", "PowerPC,750" },
-- 
2.21.1

[PATCH v4 09/18] hw/char: Rename avr_usart -> atmel_usart

[Philippe Mathieu-Daudé]

AVR is the architecture, Atmel the manufacturer.

Suggested-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
!squash
---
 include/hw/char/{avr_usart.h => atmel_usart.h} | 10 +-
 hw/avr/sample.c|  2 +-
 hw/char/{avr_usart.c => atmel_usart.c} |  4 ++--
 MAINTAINERS|  4 ++--
 hw/avr/Kconfig |  2 +-
 hw/char/Kconfig|  2 +-
 hw/char/Makefile.objs  |  2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)
 rename include/hw/char/{avr_usart.h => atmel_usart.h} (93%)
 rename hw/char/{avr_usart.c => atmel_usart.c} (99%)

diff --git a/include/hw/char/avr_usart.h b/include/hw/char/atmel_usart.h
similarity index 93%
rename from include/hw/char/avr_usart.h
rename to include/hw/char/atmel_usart.h
index 467e97e8c0..fd35feac60 100644
--- a/include/hw/char/avr_usart.h
+++ b/include/hw/char/atmel_usart.h
@@ -1,5 +1,5 @@
 /*
- * AVR USART
+ * Atmel AVR USART
  *
  * Copyright (c) 2018 University of Kent
  * Author: Sarah Harris
@@ -19,8 +19,8 @@
  * 
  */
 
-#ifndef HW_AVR_USART_H
-#define HW_AVR_USART_H
+#ifndef HW_CHAR_ATMEL_USART_H
+#define HW_CHAR_ATMEL_USART_H
 
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
@@ -56,7 +56,7 @@
 #define USART_CSRC_CSZ1   (1 << 2)
 #define USART_CSRC_CSZ0   (1 << 1)
 
-#define TYPE_AVR_USART "avr-usart"
+#define TYPE_AVR_USART "atmel-usart"
 #define AVR_USART(obj) \
 OBJECT_CHECK(AVRUsartState, (obj), TYPE_AVR_USART)
 
@@ -90,4 +90,4 @@ typedef struct {
 qemu_irq dre_irq;
 } AVRUsartState;
 
-#endif /* HW_AVR_USART_H */
+#endif /* HW_CHAR_ATMEL_USART_H */
diff --git a/hw/avr/sample.c b/hw/avr/sample.c
index 95094a8d6c..ca67c11233 100644
--- a/hw/avr/sample.c
+++ b/hw/avr/sample.c
@@ -44,7 +44,7 @@
 #include "qemu/error-report.h"
 #include "exec/address-spaces.h"
 #include "include/hw/sysbus.h"
-#include "include/hw/char/avr_usart.h"
+#include "include/hw/char/atmel_usart.h"
 #include "include/hw/timer/avr_timer16.h"
 #include "include/hw/misc/avr_mask.h"
 #include "elf.h"
diff --git a/hw/char/avr_usart.c b/hw/char/atmel_usart.c
similarity index 99%
rename from hw/char/avr_usart.c
rename to hw/char/atmel_usart.c
index becdb87847..a7004c212a 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/atmel_usart.c
@@ -1,5 +1,5 @@
 /*
- * AVR USART
+ * Atmel AVR USART
  *
  * Copyright (c) 2018 University of Kent
  * Author: Sarah Harris
@@ -20,7 +20,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/char/avr_usart.h"
+#include "hw/char/atmel_usart.h"
 #include "qemu/log.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index 4998fee99a..f2e01a6d16 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -896,8 +896,8 @@ M: Michael Rolnik 
 R: Sarah Harris 
 S: Maintained
 F: hw/avr/
-F: hw/char/avr_usart.c
-F: include/hw/char/avr_usart.h
+F: hw/char/atmel_usart.c
+F: include/hw/char/atmel_usart.h
 F: hw/timer/avr_timer16.c
 F: include/hw/timer/avr_timer16.h
 F: hw/misc/avr_mask.c
diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
index 92aa1e6afb..8084a73176 100644
--- a/hw/avr/Kconfig
+++ b/hw/avr/Kconfig
@@ -1,6 +1,6 @@
 config AVR_SAMPLE
 bool
 select AVR_TIMER16
-select AVR_USART
+select ATMEL_USART
 select AVR_MASK
 select UNIMP
diff --git a/hw/char/Kconfig b/hw/char/Kconfig
index 331b20983f..5a27681884 100644
--- a/hw/char/Kconfig
+++ b/hw/char/Kconfig
@@ -47,5 +47,5 @@ config SCLPCONSOLE
 config TERMINAL3270
 bool
 
-config AVR_USART
+config ATMEL_USART
 bool
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index f05c1f5667..c23ad3b4a7 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -21,7 +21,7 @@ obj-$(CONFIG_PSERIES) += spapr_vty.o
 obj-$(CONFIG_DIGIC) += digic-uart.o
 obj-$(CONFIG_STM32F2XX_USART) += stm32f2xx_usart.o
 obj-$(CONFIG_RASPI) += bcm2835_aux.o
-common-obj-$(CONFIG_AVR_USART) += avr_usart.o
+common-obj-$(CONFIG_ATMEL_USART) += atmel_usart.o
 
 common-obj-$(CONFIG_CMSDK_APB_UART) += cmsdk-apb-uart.o
 common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
-- 
2.21.1

[PATCH v4 07/18] hw/misc/avr_mask: Remove unused include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/misc/avr_mask.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/misc/avr_mask.h b/include/hw/misc/avr_mask.h
index d3e21972d8..5f95e1081d 100644
--- a/include/hw/misc/avr_mask.h
+++ b/include/hw/misc/avr_mask.h
@@ -26,7 +26,6 @@
 #define HW_avr_mask_H
 
 #include "hw/sysbus.h"
-#include "chardev/char-fe.h"
 #include "hw/hw.h"
 
 
-- 
2.21.1

[PATCH v4 06/18] hw/timer/avr_timer16: Rename memory region debugging name

[Philippe Mathieu-Daudé]

This device expose 3 different I/O regions. Name them differently
to have a clearer 'info mtree' output.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/avr_timer16.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/timer/avr_timer16.c b/hw/timer/avr_timer16.c
index aea1bf009e..a27933a18a 100644
--- a/hw/timer/avr_timer16.c
+++ b/hw/timer/avr_timer16.c
@@ -563,11 +563,11 @@ static void avr_timer16_init(Object *obj)
 sysbus_init_irq(SYS_BUS_DEVICE(obj), >ovf_irq);
 
 memory_region_init_io(>iomem, obj, _timer16_ops,
-  s, TYPE_AVR_TIMER16, 0xe);
+  s, "avr-timer16", 0xe);
 memory_region_init_io(>imsk_iomem, obj, _timer16_imsk_ops,
-  s, TYPE_AVR_TIMER16, 0x1);
+  s, "avr-timer16-intmask", 0x1);
 memory_region_init_io(>ifr_iomem, obj, _timer16_ifr_ops,
-  s, TYPE_AVR_TIMER16, 0x1);
+  s, "avr-timer16-intflag", 0x1);
 
 sysbus_init_mmio(SYS_BUS_DEVICE(obj), >iomem);
 sysbus_init_mmio(SYS_BUS_DEVICE(obj), >imsk_iomem);
-- 
2.21.1

[PATCH v4 11/18] hw/misc: Rename avr_mask -> atmel_power

[Philippe Mathieu-Daudé]

AVR is the architecture, Atmel the manufacturer.

Suggested-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
!squash
---
 include/hw/misc/{avr_mask.h => atmel_power.h} | 10 +-
 hw/avr/sample.c   |  2 +-
 hw/misc/{avr_mask.c => atmel_power.c} |  4 ++--
 MAINTAINERS   |  4 ++--
 hw/avr/Kconfig|  2 +-
 hw/misc/Kconfig   |  2 +-
 hw/misc/Makefile.objs |  2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)
 rename include/hw/misc/{avr_mask.h => atmel_power.h} (89%)
 rename hw/misc/{avr_mask.c => atmel_power.c} (97%)

diff --git a/include/hw/misc/avr_mask.h b/include/hw/misc/atmel_power.h
similarity index 89%
rename from include/hw/misc/avr_mask.h
rename to include/hw/misc/atmel_power.h
index 5f95e1081d..5366e9693f 100644
--- a/include/hw/misc/avr_mask.h
+++ b/include/hw/misc/atmel_power.h
@@ -1,5 +1,5 @@
 /*
- * AVR Power Reduction
+ * Atmel AVR Power Reduction Management
  *
  * Copyright (c) 2019 Michael Rolnik
  *
@@ -22,14 +22,14 @@
  * THE SOFTWARE.
  */
 
-#ifndef HW_avr_mask_H
-#define HW_avr_mask_H
+#ifndef HW_MISC_ATMEL_POWER_H
+#define HW_MISC_ATMEL_POWER_H
 
 #include "hw/sysbus.h"
 #include "hw/hw.h"
 
 
-#define TYPE_AVR_MASK "avr-mask"
+#define TYPE_AVR_MASK "atmel-power"
 #define AVR_MASK(obj) OBJECT_CHECK(AVRMaskState, (obj), TYPE_AVR_MASK)
 
 typedef struct {
@@ -43,4 +43,4 @@ typedef struct {
 qemu_irq irq[8];
 } AVRMaskState;
 
-#endif /* HW_avr_mask_H */
+#endif /* HW_MISC_ATMEL_POWER_H */
diff --git a/hw/avr/sample.c b/hw/avr/sample.c
index 19e9f56f3b..19b8c067e5 100644
--- a/hw/avr/sample.c
+++ b/hw/avr/sample.c
@@ -46,7 +46,7 @@
 #include "include/hw/sysbus.h"
 #include "include/hw/char/atmel_usart.h"
 #include "include/hw/timer/atmel_timer16.h"
-#include "include/hw/misc/avr_mask.h"
+#include "include/hw/misc/atmel_power.h"
 #include "elf.h"
 #include "hw/misc/unimp.h"
 
diff --git a/hw/misc/avr_mask.c b/hw/misc/atmel_power.c
similarity index 97%
rename from hw/misc/avr_mask.c
rename to hw/misc/atmel_power.c
index 3af82ed9c1..adab729f66 100644
--- a/hw/misc/avr_mask.c
+++ b/hw/misc/atmel_power.c
@@ -1,5 +1,5 @@
 /*
- * AVR Power Reduction
+ * Atmel AVR Power Reduction Management
  *
  * Copyright (c) 2019 Michael Rolnik
  *
@@ -23,7 +23,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/misc/avr_mask.h"
+#include "hw/misc/atmel_power.h"
 #include "qemu/log.h"
 #include "hw/qdev-properties.h"
 #include "hw/irq.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index a98d164bc1..60634cebf4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -900,8 +900,8 @@ F: hw/char/atmel_usart.c
 F: include/hw/char/atmel_usart.h
 F: hw/timer/atmel_timer16.c
 F: include/hw/timer/atmel_timer16.h
-F: hw/misc/avr_mask.c
-F: include/hw/misc/avr_mask.h
+F: hw/misc/atmel_power.c
+F: include/hw/misc/atmel_power.h
 F: tests/acceptance/machine_avr6.py
 
 CRIS Machines
diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
index 45c7025f09..516b89c849 100644
--- a/hw/avr/Kconfig
+++ b/hw/avr/Kconfig
@@ -2,5 +2,5 @@ config AVR_SAMPLE
 bool
 select ATMEL_TIMER16
 select ATMEL_USART
-select AVR_MASK
+select ATMEL_POWER
 select UNIMP
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 74a1e9a241..3a3c32e1b0 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -131,7 +131,7 @@ config MAC_VIA
 select MOS6522
 select ADB
 
-config AVR_MASK
+config ATMEL_POWER
 bool
 
 source macio/Kconfig
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index bbf17f651b..e605964f4b 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -86,4 +86,4 @@ obj-$(CONFIG_MAC_VIA) += mac_via.o
 
 common-obj-$(CONFIG_GRLIB) += grlib_ahb_apb_pnp.o
 
-obj-$(CONFIG_AVR_MASK) += avr_mask.o
+obj-$(CONFIG_ATMEL_POWER) += atmel_power.o
-- 
2.21.1

[PATCH v4 04/18] tests/acceptance: Keep multilines comment consistent with other tests

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
note to maintainer: squash before merge?
---
 tests/acceptance/machine_avr6.py | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/acceptance/machine_avr6.py b/tests/acceptance/machine_avr6.py
index 784c287d0b..611f6a62a4 100644
--- a/tests/acceptance/machine_avr6.py
+++ b/tests/acceptance/machine_avr6.py
@@ -33,11 +33,9 @@ class AVR6Machine(Test):
 
https://github.com/seharris/qemu-avr-tests/raw/master/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf
 constantly prints out 
'ABCDEFGHIJKLMNOPQRSTUVWXABCDEFGHIJKLMNOPQRSTUVWX'
 """
-rom_url = 'https://github.com/seharris/qemu-avr-tests'
-rom_sha1= '36c3e67b8755dcf37e06af6730ef5d477b8ed16d'
-rom_url += '/raw/'
-rom_url += rom_sha1
-rom_url += '/free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf'
+rom_url = ('https://github.com/seharris/qemu-avr-tests'
+   '/raw/36c3e67b8755dcf/free-rtos/Demo'
+   '/AVR_ATMega2560_GCC/demo.elf')
 rom_hash = '7eb521f511ca8f2622e0a3c5e8dd686efbb911d4'
 rom_path = self.fetch_asset(rom_url, asset_hash=rom_hash)
 
-- 
2.21.1

[PATCH v4 08/18] hw/avr/Makefile: Use CONFIG_AVR_SAMPLE variable

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/avr/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
index 626b7064b3..1eb4b53be6 100644
--- a/hw/avr/Makefile.objs
+++ b/hw/avr/Makefile.objs
@@ -1 +1 @@
-obj-y += sample.o
+obj-$(CONFIG_AVR_SAMPLE) += sample.o
-- 
2.21.1

[PATCH v4 05/18] hw/char/avr: Reduce USART I/O size

[Philippe Mathieu-Daudé]

Per the datasheet the USART uses 7 consecutive 8-bit registers.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/char/avr_usart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
index cb307fe23d..becdb87847 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/avr_usart.c
@@ -280,7 +280,7 @@ static void avr_usart_init(Object *obj)
 sysbus_init_irq(SYS_BUS_DEVICE(obj), >rxc_irq);
 sysbus_init_irq(SYS_BUS_DEVICE(obj), >dre_irq);
 sysbus_init_irq(SYS_BUS_DEVICE(obj), >txc_irq);
-memory_region_init_io(>mmio, obj, _usart_ops, s, TYPE_AVR_USART, 8);
+memory_region_init_io(>mmio, obj, _usart_ops, s, TYPE_AVR_USART, 7);
 sysbus_init_mmio(SYS_BUS_DEVICE(obj), >mmio);
 qdev_init_gpio_in(DEVICE(s), avr_usart_pr, 1);
 s->enabled = true;
-- 
2.21.1

[PATCH v4 02/18] MAINTAINERS: Move the AVR machines in new section (not within ARM)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
!squash
---
 MAINTAINERS | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3fbac64b31..4998fee99a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -489,19 +489,6 @@ F: hw/*/allwinner*
 F: include/hw/*/allwinner*
 F: hw/arm/cubieboard.c
 
-AVR Machines
-M: Michael Rolnik 
-R: Sarah Harris 
-S: Maintained
-F: hw/avr/
-F: hw/char/avr_usart.c
-F: include/hw/char/avr_usart.h
-F: hw/timer/avr_timer16.c
-F: include/hw/timer/avr_timer16.h
-F: hw/misc/avr_mask.c
-F: include/hw/misc/avr_mask.h
-F: tests/acceptance/machine_avr6.py
-
 ARM PrimeCell and CMSDK devices
 M: Peter Maydell 
 L: qemu-...@nongnu.org
@@ -901,6 +888,22 @@ F: include/hw/*/nrf51*.h
 F: include/hw/*/microbit*.h
 F: tests/qtest/microbit-test.c
 
+AVR Machines
+-
+
+Atmel MCU
+M: Michael Rolnik 
+R: Sarah Harris 
+S: Maintained
+F: hw/avr/
+F: hw/char/avr_usart.c
+F: include/hw/char/avr_usart.h
+F: hw/timer/avr_timer16.c
+F: include/hw/timer/avr_timer16.h
+F: hw/misc/avr_mask.c
+F: include/hw/misc/avr_mask.h
+F: tests/acceptance/machine_avr6.py
+
 CRIS Machines
 -
 Axis Dev88
-- 
2.21.1

[PATCH v4 03/18] tests/acceptance: Do not set the machine type manually

[Philippe Mathieu-Daudé]

Since commit ba21bde93 we don't need to set the machine type
manually, the one set by the ":avocado: tags=machine" will be used.

Suggested-by: Cleber Rosa 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/acceptance/machine_avr6.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/acceptance/machine_avr6.py b/tests/acceptance/machine_avr6.py
index 43501b26a3..784c287d0b 100644
--- a/tests/acceptance/machine_avr6.py
+++ b/tests/acceptance/machine_avr6.py
@@ -41,7 +41,6 @@ class AVR6Machine(Test):
 rom_hash = '7eb521f511ca8f2622e0a3c5e8dd686efbb911d4'
 rom_path = self.fetch_asset(rom_url, asset_hash=rom_hash)
 
-self.vm.set_machine('sample')
 self.vm.add_args('-bios', rom_path)
 self.vm.add_args('-nographic')
 self.vm.launch()
-- 
2.21.1

[PATCH v4 00/18] hw/avr: Introduce few Arduino boards

[Philippe Mathieu-Daudé]

Hi,

This series add the arduino boards, aiming at removing the
'sample' board that doesn't follow any specification.

Since v3:
- Rebased on Michael's v41
- Drop 'extram' unused field (Igor)
- Renamed devices AVR -> Atmel (Aleksandar)
  (I haven't renamed structure names to ease review)

Since v2:
- rebased on Michael's v40

Since v1:
- Addressed Igor comments
- Addressed Aleksandar comments
- Fixed UART issue (was due to IRQ shifted by 2 in CPU)

Since Michael's work is not yet merged, Various of my patches
- which are trivials or simple renames - could be squashed
directly on his patches, if we ever care.
[I believe sending this patches is easier/quicker than keeping
asking Michael to respin his series infinitely].

Michael, do you mind testing it? The full series is available
here: https://gitlab.com/philmd/qemu/commits/arduino-v4

Regards,

Phil.

Obsoletes: <20191229224505.24466-1-f4...@amsat.org>
Based-on: <20200118191416.19934-1-mrol...@gmail.com>
https://www.mail-archive.com/qemu-devel@nongnu.org/msg671707.html

Philippe Mathieu-Daudé (18):
  MAINTAINERS: Move machine test to the machine section (not ARCH one)
  MAINTAINERS: Move the AVR machines in new section (not within ARM)
  tests/acceptance: Do not set the machine type manually
  tests/acceptance: Keep multilines comment consistent with other tests
  hw/char/avr: Reduce USART I/O size
  hw/timer/avr_timer16: Rename memory region debugging name
  hw/misc/avr_mask: Remove unused include
  hw/avr/Makefile: Use CONFIG_AVR_SAMPLE variable
  hw/char: Rename avr_usart -> atmel_usart
  hw/timer: Rename avr_timer16 -> atmel_timer16
  hw/misc: Rename avr_mask -> atmel_power
  hw/avr: Introduce ATMEL_ATMEGA_MCU config
  hw/avr: Add some ATmega microcontrollers
  hw/avr: Add some Arduino boards
  tests/boot-serial-test: Test some Arduino boards (AVR based)
  tests/acceptance: Test the Arduino MEGA2560 board
  hw/avr: Remove the unrealistic AVR 'sample' board
  .travis.yml: Run the AVR acceptance tests

 default-configs/avr-softmmu.mak   |   2 +-
 hw/avr/atmel_atmega.h |  48 ++
 .../hw/char/{avr_usart.h => atmel_usart.h}|  10 +-
 include/hw/misc/{avr_mask.h => atmel_power.h} |  11 +-
 .../timer/{avr_timer16.h => atmel_timer16.h}  |  10 +-
 hw/avr/arduino.c  | 175 +++
 hw/avr/atmel_atmega.c | 464 ++
 hw/avr/sample.c   | 295 ---
 hw/char/{avr_usart.c => atmel_usart.c}|   6 +-
 hw/misc/{avr_mask.c => atmel_power.c} |   4 +-
 hw/timer/{avr_timer16.c => atmel_timer16.c}   |  10 +-
 tests/qtest/boot-serial-test.c|   3 +-
 .travis.yml   |   2 +-
 MAINTAINERS   |  29 +-
 hw/avr/Kconfig|  11 +-
 hw/avr/Makefile.objs  |   3 +-
 hw/char/Kconfig   |   2 +-
 hw/char/Makefile.objs |   2 +-
 hw/misc/Kconfig   |   2 +-
 hw/misc/Makefile.objs |   2 +-
 hw/timer/Kconfig  |   2 +-
 hw/timer/Makefile.objs|   2 +-
 tests/acceptance/machine_avr6.py  |  11 +-
 23 files changed, 751 insertions(+), 355 deletions(-)
 create mode 100644 hw/avr/atmel_atmega.h
 rename include/hw/char/{avr_usart.h => atmel_usart.h} (93%)
 rename include/hw/misc/{avr_mask.h => atmel_power.h} (89%)
 rename include/hw/timer/{avr_timer16.h => atmel_timer16.h} (92%)
 create mode 100644 hw/avr/arduino.c
 create mode 100644 hw/avr/atmel_atmega.c
 delete mode 100644 hw/avr/sample.c
 rename hw/char/{avr_usart.c => atmel_usart.c} (99%)
 rename hw/misc/{avr_mask.c => atmel_power.c} (97%)
 rename hw/timer/{avr_timer16.c => atmel_timer16.c} (98%)

-- 
2.21.1

[PATCH v4 01/18] MAINTAINERS: Move machine test to the machine section (not ARCH one)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
!squash
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index c70d77b1ae..3fbac64b31 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -168,7 +168,6 @@ M: Michael Rolnik 
 R: Sarah Harris 
 S: Maintained
 F: target/avr/
-F: tests/acceptance/machine_avr6.py
 F: default-configs/avr-softmmu.mak
 F: gdb-xml/avr-cpu.xml
 
@@ -501,6 +500,7 @@ F: hw/timer/avr_timer16.c
 F: include/hw/timer/avr_timer16.h
 F: hw/misc/avr_mask.c
 F: include/hw/misc/avr_mask.h
+F: tests/acceptance/machine_avr6.py
 
 ARM PrimeCell and CMSDK devices
 M: Peter Maydell 
-- 
2.21.1

Re: [PATCH v3 00/10] Further bitmaps improvements

[Eric Blake]

On 1/20/20 10:33 AM, Vladimir Sementsov-Ogievskiy wrote:

20.01.2020 17:20, Max Reitz wrote:

On 19.12.19 11:03, Vladimir Sementsov-Ogievskiy wrote:

Hi!

The main feature here is improvement of _next_dirty_area API, which I'm
going to use then for backup / block-copy.


Looks good to me overall, with a few minor changes.  I’d rather leave
patches 8 and 9 to Eric, though.  (Even though I’m not exactly the
maintainer for the rest of the patches either...)

Max



Thanks for reviewing! Let's wait for Eric.


I had enough concerns over 8/10 to probably warrant a v4; but I'm also 
fine taking the entire series through my NBD tree once everyone is happy 
with it.


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