Re: [PATCH 07/13] ppc/pnv: Introduce PnvChipClass::intc_print_info() method

[Cédric Le Goater]

>> May be we should introduce a helper such as : 
>>
>> int pnv_chip_cpu_foreach(PnvChip *chip,
>>int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void 
>> *opaque)
>> {
>> int i, j;
>> int ret = 0;
>>
>> for (i = 0; i < chip->nr_cores; i++) {
>> PnvCore *pc = chip->cores[i];
>> CPUCore *cc = CPU_CORE(pc);
>>
>> for (j = 0; j < cc->nr_threads; j++) {
>> PowerPCCPU *cpu = pc->threads[j];
>> ret = doit(chip, cpu, opaque);
>> if (ret) { 
>> break;
>> }
>> }
>> }
>> return ret;
>> }
> 
> What I'd actually like to work towards is just having the interrupt
> controllers themselves advertize TYPE_INTERRUPT_STATS_PROVIDER and not
> needing anything specific at the machine level to locate them, just
> let the generic code in hmp_info_pic handle it.

OK. It would good to at least loop on the chips, so that the output
of the possible TYPE_INTERRUPT_STATS_PROVIDER (IC, PSIHB, PHB, NPU) 
are ordered. 

C.

Re: [PATCH v6 5/8] Add dbus-vmstate object

[Marc-André Lureau]

Hi

On Thu, Dec 12, 2019 at 4:03 PM Daniel P. Berrangé  wrote:
>
> On Wed, Dec 11, 2019 at 05:45:03PM +0400, Marc-André Lureau wrote:
> > When instantiated, this object will connect to the given D-Bus bus
> > "addr". During migration, it will take/restore the data from
> > org.qemu.VMState1 instances. See documentation for details.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  MAINTAINERS   |   2 +
> >  backends/Makefile.objs|   4 +
> >  backends/dbus-vmstate.c   | 496 ++
> >  docs/interop/dbus-vmstate.rst |  74 +
> >  docs/interop/dbus.rst |   5 +
> >  docs/interop/index.rst|   1 +
> >  6 files changed, 582 insertions(+)
> >  create mode 100644 backends/dbus-vmstate.c
> >  create mode 100644 docs/interop/dbus-vmstate.rst
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f08fb4f24e..7af80d0c1d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2202,9 +2202,11 @@ F: qapi/migration.json
> >  D-Bus
> >  M: Marc-André Lureau 
> >  S: Maintained
> > +F: backends/dbus-vmstate.c
> >  F: util/dbus.c
> >  F: include/qemu/dbus.h
> >  F: docs/interop/dbus.rst
> > +F: docs/interop/dbus-vmstate.rst
> >
> >  Seccomp
> >  M: Eduardo Otubo 
> > diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> > index f0691116e8..28a847cd57 100644
> > --- a/backends/Makefile.objs
> > +++ b/backends/Makefile.objs
> > @@ -17,3 +17,7 @@ endif
> >  common-obj-$(call land,$(CONFIG_VHOST_USER),$(CONFIG_VIRTIO)) += 
> > vhost-user.o
> >
> >  common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
> > +
> > +common-obj-$(CONFIG_GIO) += dbus-vmstate.o
> > +dbus-vmstate.o-cflags = $(GIO_CFLAGS)
> > +dbus-vmstate.o-libs = $(GIO_LIBS)
> > diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> > new file mode 100644
> > index 00..059dd420b8
> > --- /dev/null
> > +++ b/backends/dbus-vmstate.c
> > @@ -0,0 +1,496 @@
> > +/*
> > + * QEMU dbus-vmstate
> > + *
> > + * Copyright (C) 2019 Red Hat Inc
> > + *
> > + * Authors:
> > + *  Marc-André Lureau 
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qemu/dbus.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/error.h"
> > +#include "qom/object_interfaces.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "migration/vmstate.h"
> > +
> > +typedef struct DBusVMState DBusVMState;
> > +typedef struct DBusVMStateClass DBusVMStateClass;
> > +
> > +#define TYPE_DBUS_VMSTATE "dbus-vmstate"
> > +#define DBUS_VMSTATE(obj)\
> > +OBJECT_CHECK(DBusVMState, (obj), TYPE_DBUS_VMSTATE)
> > +#define DBUS_VMSTATE_GET_CLASS(obj)  \
> > +OBJECT_GET_CLASS(DBusVMStateClass, (obj), TYPE_DBUS_VMSTATE)
> > +#define DBUS_VMSTATE_CLASS(klass)\
> > +OBJECT_CLASS_CHECK(DBusVMStateClass, (klass), TYPE_DBUS_VMSTATE)
> > +
> > +struct DBusVMStateClass {
> > +ObjectClass parent_class;
> > +};
> > +
>
> Not an objection to your patch here. This just reminds me that we
> ought to follow GLib's lead and implement some helper macros to
> automate all this tedious boilerplate. So we can just do something
> simple like:
>
>  QOM_DECLARE_FINAL_TYPE(DBusVMState, dbus_vmstate, DBUS_VMSATE, Object)
>
> and an equiv to do the  TypeInfo declaration & registration.
>
> > +struct DBusVMState {
> > +Object parent;
> > +
> > +GDBusConnection *bus;
> > +char *dbus_addr;
> > +char *id_list;
> > +
> > +uint32_t data_size;
> > +uint8_t *data;
> > +};
> > +
> > +static const GDBusPropertyInfo vmstate_property_info[] = {
> > +{ -1, (char *) "Id", (char *) "s",
> > +  G_DBUS_PROPERTY_INFO_FLAGS_READABLE, NULL },
> > +};
> > +
> > +static const GDBusPropertyInfo * const vmstate_property_info_pointers[] = {
> > +_property_info[0],
> > +NULL
> > +};
> > +
> > +static const GDBusInterfaceInfo vmstate1_interface_info = {
> > +-1,
> > +(char *) "org.qemu.VMState1",
> > +(GDBusMethodInfo **) NULL,
> > +(GDBusSignalInfo **) NULL,
> > +(GDBusPropertyInfo **) _property_info_pointers,
> > +NULL,
> > +};
> > +
> > +#define DBUS_VMSTATE_SIZE_LIMIT (1 * MiB)
> > +
> > +static GHashTable *
> > +get_id_list_set(DBusVMState *self)
> > +{
> > +g_auto(GStrv) ids = NULL;
> > +g_autoptr(GHashTable) set = NULL;
> > +int i;
> > +
> > +if (!self->id_list) {
> > +return NULL;
> > +}
> > +
> > +ids = g_strsplit(self->id_list, ",", -1);
> > +set = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL);
> > +for (i = 0; ids[i]; i++) {
> > +g_hash_table_add(set, ids[i]);
> > +ids[i] = NULL;
> > +}
> > +
> > +return g_steal_pointer();
> > +}
> > +
> > +static GHashTable *
> > +dbus_get_proxies(DBusVMState *self, GError **err)
> > +{
> 

Re: [PATCH 1/5] arm64: zynqmp: Add firmware DT node

[Michal Simek]

On 15. 12. 19 6:28, Guenter Roeck wrote:
> On 12/9/19 7:02 AM, Michal Simek wrote:
>> On 09. 12. 19 15:32, Guenter Roeck wrote:
>>> On 12/8/19 11:48 PM, Edgar E. Iglesias wrote:
 On Sun, Dec 08, 2019 at 11:19:33PM -0800, Guenter Roeck wrote:
> On 12/8/19 10:42 PM, Michal Simek wrote:
>> Hi, +Edgar
>>
>>
>> On 08. 12. 19 23:38, Guenter Roeck wrote:
>>> On Fri, Oct 18, 2019 at 06:07:31PM +0200, Michael Tretter wrote:
 From: Rajan Vaja 

 Add firmware DT node in ZynqMP device tree. This node
 uses bindings as per new firmware interface driver.

 Signed-off-by: Rajan Vaja 
 Signed-off-by: Michal Simek 
 Signed-off-by: Michael Tretter 
>>>
>>> With this patch applied in the mainline kernel, the qemu xlnx-zcu102
>>> emulation crashes (see below). Any idea what it might take to get
>>> qemu back to working ?
>>
>> Driver talks through ATF to PMU unit(microblaze). I don't think
>> A53+MB
>> concept is working with mainline qemu. But crash is too hard. It
>> should

 Yes, QEMU doesn't support the Cortex-A53s along with the PMU
 MicroBlaze.

 My workaround when using upstream QEMU is a modified DT without the
 PMU firmware
 and with fixed-clock nodes.

>>>
>>> I can't do that for my boot tests. Normally I would just disable
>>> ZYNQMP_FIRMWARE,
>>> but that is hard enabled with ARCH_ZYNQMP. I'll have to drop those
>>> tests,
>>> unfortunately, if the firmware driver is considered mandatory.
>>
>> We can make it optional.
>> Rajan: please send a patch for it.
>>
> 
> I'll disable the related boot tests for now. If/when this is fixed, let
> me know,
> and I'll re-enable it.

ok. Sure.

Thanks,
Michal

Re: [PATCH v2 0/6] migration/multifd: a new mechanism for send thread sync

[Wei Yang]

Ping for comment.

On Sat, Oct 26, 2019 at 08:45:14AM +0800, Wei Yang wrote:
>Current send thread could work while the sync mechanism has some problem:
>
>  * has spuriously wakeup
>  * number of channels_ready will *overflow* the number of real channels
>
>The reason is:
>
>  * if MULTIFD_FLAG_SYNC is set in the middle of send thread running, there
>is one more spurious wakeup
>  * if MULTIFD_FLAG_SYNC is set when send thread is not running, there is one
>more channels_ready be triggered
>
>To solve this situation, one new mechanism is introduced to synchronize send
>threads. The idea is simple, a new field *sync* is introduced to indicate a
>synchronization is required.
>
>---
>v2: rebase on latest code
>
>Wei Yang (6):
>  migration/multifd: move Params update and pages cleanup into
>multifd_send_fill_packet()
>  migration/multifd: notify channels_ready when send thread starts
>  migration/multifd: use sync field to synchronize send threads
>  migration/multifd: used must not be 0 for a pending job
>  migration/multifd: use boolean for pending_job is enough
>  migration/multifd: there is no spurious wakeup now
>
> migration/ram.c | 74 +++--
> 1 file changed, 47 insertions(+), 27 deletions(-)
>
>-- 
>2.17.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/2] not use multifd during postcopy

[Wei Yang]

Would this one be picked up this time?

On Sat, Oct 26, 2019 at 07:19:58AM +0800, Wei Yang wrote:
>We don't support multifd during postcopy, but user still could enable
>both multifd and postcopy. This leads to migration failure.
>
>Patch 1 does proper cleanup, otherwise we may have data corruption.
>Patch 2 does the main job.
>
>BTW, current multifd synchronization method needs a cleanup. Will send another
>patch set.
>
>Wei Yang (2):
>  migration/multifd: clean pages after filling packet
>  migration/multifd: not use multifd during postcopy
>
> migration/ram.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
>-- 
>2.17.1

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 0/6] migration/postcopy: enable compress during postcopy

[Wei Yang]

Would this one be picked up in this version?

On Thu, Nov 07, 2019 at 08:39:01PM +0800, Wei Yang wrote:
>This patch set tries enable compress during postcopy.
>
>postcopy requires to place a whole host page, while migration thread migrate
>memory in target page size. This makes postcopy need to collect all target
>pages in one host page before placing via userfaultfd.
>
>To enable compress during postcopy, there are two problems to solve:
>
>1. Random order for target page arrival
>2. Target pages in one host page arrives without interrupt by target
>   page from other host page
>
>The first one is handled by counting the number of target pages arrived
>instead of the last target page arrived.
>
>The second one is handled by:
>
>1. Flush compress thread for each host page
>2. Wait for decompress thread for before placing host page
>
>With the combination of these two changes, compress is enabled during
>postcopy.
>
>---
>v2:
> * use uintptr_t to calculate place_dest
> * check target pages belongs to the same host page
>
>Wei Yang (6):
>  migration/postcopy: reduce memset when it is zero page and
>matches_target_page_size
>  migration/postcopy: wait for decompress thread in precopy
>  migration/postcopy: count target page number to decide the
>place_needed
>  migration/postcopy: set all_zero to true on the first target page
>  migration/postcopy: enable random order target page arrival
>  migration/postcopy: enable compress during postcopy
>
> migration/migration.c | 11 ---
> migration/ram.c   | 67 +--
> 2 files changed, 52 insertions(+), 26 deletions(-)
>
>-- 
>2.17.1

-- 
Wei Yang
Help you, Help me

[PATCH] virtio-blk: fix out-of-bounds access to bitmap in notify_guest_bh

[Li hangjing]

From: Li Hangjing 

When the number of a virtio-blk device's virtqueues is larger than
BITS_PER_LONG, the out-of-bounds access to bitmap[ ] will occur.

Fixes: e21737ab15 ("virtio-blk: multiqueue batch notify")
Cc: qemu-sta...@nongnu.org
Cc: Stefan Hajnoczi 
Signed-off-by: Li Hangjing 
Reviewed-by: Xie Yongji 
Reviewed-by: Chai Wen 
---
 hw/block/dataplane/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 119906a5fe..1b52e8159c 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -67,7 +67,7 @@ static void notify_guest_bh(void *opaque)
 memset(s->batch_notify_vqs, 0, sizeof(bitmap));
 
 for (j = 0; j < nvqs; j += BITS_PER_LONG) {
-unsigned long bits = bitmap[j];
+unsigned long bits = bitmap[j / BITS_PER_LONG];
 
 while (bits != 0) {
 unsigned i = j + ctzl(bits);
-- 
2.15.1.windows.2

Re: [PATCH 07/13] ppc/pnv: Introduce PnvChipClass::intc_print_info() method

[David Gibson]

On Fri, Dec 13, 2019 at 02:00:53PM +0100, Cédric Le Goater wrote:
> On 13/12/2019 13:00, Greg Kurz wrote:
> > The pnv_pic_print_info() callback checks the type of the chip in order
> > to forward to the request appropriate interrupt controller. This can
> > be achieved with QOM. Introduce a method for this in the base chip class
> > and implement it in child classes.
> > 
> > This also prepares ground for the upcoming interrupt controller of POWER10
> > chips.
> > 
> > Signed-off-by: Greg Kurz 
> 
> 
> Reviewed-by: Cédric Le Goater 
> 
> One comment below.
> 
> > ---
> >  hw/ppc/pnv.c |   30 +-
> >  include/hw/ppc/pnv.h |1 +
> >  2 files changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index efc00f4cb67a..2a53e99bda2e 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -832,6 +832,12 @@ static void pnv_chip_power8_intc_destroy(PnvChip 
> > *chip, PowerPCCPU *cpu)
> >  pnv_cpu->intc = NULL;
> >  }
> >  
> > +static void pnv_chip_power8_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> > +Monitor *mon)
> > +{
> > +icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
> > +}
> > +
> >  /*
> >   *0:48  Reserved - Read as zeroes
> >   *   49:52  Node ID
> > @@ -889,6 +895,12 @@ static void pnv_chip_power9_intc_destroy(PnvChip 
> > *chip, PowerPCCPU *cpu)
> >  pnv_cpu->intc = NULL;
> >  }
> >  
> > +static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> > +Monitor *mon)
> > +{
> > +xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
> > +}
> > +
> >  static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> >  Error **errp)
> >  {
> > @@ -910,6 +922,11 @@ static void pnv_chip_power10_intc_destroy(PnvChip 
> > *chip, PowerPCCPU *cpu)
> >  pnv_cpu->intc = NULL;
> >  }
> >  
> > +static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU 
> > *cpu,
> > + Monitor *mon)
> > +{
> > +}
> > +
> >  /*
> >   * Allowed core identifiers on a POWER8 Processor Chip :
> >   *
> > @@ -1086,6 +1103,7 @@ static void pnv_chip_power8e_class_init(ObjectClass 
> > *klass, void *data)
> >  k->intc_create = pnv_chip_power8_intc_create;
> >  k->intc_reset = pnv_chip_power8_intc_reset;
> >  k->intc_destroy = pnv_chip_power8_intc_destroy;
> > +k->intc_print_info = pnv_chip_power8_intc_print_info;
> >  k->isa_create = pnv_chip_power8_isa_create;
> >  k->dt_populate = pnv_chip_power8_dt_populate;
> >  k->pic_print_info = pnv_chip_power8_pic_print_info;
> > @@ -1107,6 +1125,7 @@ static void pnv_chip_power8_class_init(ObjectClass 
> > *klass, void *data)
> >  k->intc_create = pnv_chip_power8_intc_create;
> >  k->intc_reset = pnv_chip_power8_intc_reset;
> >  k->intc_destroy = pnv_chip_power8_intc_destroy;
> > +k->intc_print_info = pnv_chip_power8_intc_print_info;
> >  k->isa_create = pnv_chip_power8_isa_create;
> >  k->dt_populate = pnv_chip_power8_dt_populate;
> >  k->pic_print_info = pnv_chip_power8_pic_print_info;
> > @@ -1128,6 +1147,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass 
> > *klass, void *data)
> >  k->intc_create = pnv_chip_power8_intc_create;
> >  k->intc_reset = pnv_chip_power8_intc_reset;
> >  k->intc_destroy = pnv_chip_power8_intc_destroy;
> > +k->intc_print_info = pnv_chip_power8_intc_print_info;
> >  k->isa_create = pnv_chip_power8nvl_isa_create;
> >  k->dt_populate = pnv_chip_power8_dt_populate;
> >  k->pic_print_info = pnv_chip_power8_pic_print_info;
> > @@ -1299,6 +1319,7 @@ static void pnv_chip_power9_class_init(ObjectClass 
> > *klass, void *data)
> >  k->intc_create = pnv_chip_power9_intc_create;
> >  k->intc_reset = pnv_chip_power9_intc_reset;
> >  k->intc_destroy = pnv_chip_power9_intc_destroy;
> > +k->intc_print_info = pnv_chip_power9_intc_print_info;
> >  k->isa_create = pnv_chip_power9_isa_create;
> >  k->dt_populate = pnv_chip_power9_dt_populate;
> >  k->pic_print_info = pnv_chip_power9_pic_print_info;
> > @@ -1379,6 +1400,7 @@ static void pnv_chip_power10_class_init(ObjectClass 
> > *klass, void *data)
> >  k->intc_create = pnv_chip_power10_intc_create;
> >  k->intc_reset = pnv_chip_power10_intc_reset;
> >  k->intc_destroy = pnv_chip_power10_intc_destroy;
> > +k->intc_print_info = pnv_chip_power10_intc_print_info;
> >  k->isa_create = pnv_chip_power10_isa_create;
> >  k->dt_populate = pnv_chip_power10_dt_populate;
> >  k->pic_print_info = pnv_chip_power10_pic_print_info;
> > @@ -1575,11 +1597,9 @@ static void 
> > pnv_pic_print_info(InterruptStatsProvider *obj,
> >  CPU_FOREACH(cs) {
> >  PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > -if (pnv_chip_is_power9(pnv->chips[0])) {
> > -

Re: [PATCH 00/13] ppc/pnv: Get rid of chip_type attributes

[David Gibson]

On Fri, Dec 13, 2019 at 12:59:28PM +0100, Greg Kurz wrote:
> The PnvChipClass type has a chip_type attribute which identifies various
> POWER CPU chip types that can be used in a powernv machine.
> 
> typedef enum PnvChipType {
> PNV_CHIP_POWER8E, /* AKA Murano (default) */
> PNV_CHIP_POWER8,  /* AKA Venice */
> PNV_CHIP_POWER8NVL,   /* AKA Naples */
> PNV_CHIP_POWER9,  /* AKA Nimbus */
> PNV_CHIP_POWER10, /* AKA TBD */
> } PnvChipType;
> 
> This attribute is used in many places where we want a different behaviour
> depending on the CPU type, either directly like:
> 
> switch (PNV_CHIP_GET_CLASS(chip)->chip_type) {
> case PNV_CHIP_POWER8E:
> case PNV_CHIP_POWER8:
> case PNV_CHIP_POWER8NVL:
> return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> case PNV_CHIP_POWER9:
> case PNV_CHIP_POWER10:
> return addr >> 3;
> default:
> g_assert_not_reached();
> }
> 
> or through various helpers that rely on it:
> 
> /* Each core has an XSCOM MMIO region */
> if (pnv_chip_is_power10(chip)) {
> xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid);
> } else if (pnv_chip_is_power9(chip)) {
> xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid);
> } else {
> xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid);
> }
> 
> The chip_type is also duplicated in the PnvPsiClass type.
> 
> It looks a bit unfortunate to implement manually something that falls into
> the scope of QOM. Especially since we don't seem to need a finer grain than
> the CPU familly, ie. POWER8, POWER9, POWER10, ..., and we already have
> specialized versions of PnvChipClass and PnvPsiClass for these.
> 
> This series basically QOM-ifies all the places where we check on the chip
> type, and gets rid of the chip_type attributes and the is_powerXX() helpers.
> 
> Patch 1 was recently posted to the list but it isn't available in David's
> ppc-for-5.0 tree yet, so I include it in this series for convenience.

Applied to ppc-for-5.0.  I had been anticipating just replacing a
bunch of tests on ->chip_type with object_dynamic_cast() checks, but
this is better still.

-- 
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 12/13] ppc/pnv: Introduce PnvChipClass::xscom_pcba() method

[David Gibson]

On Fri, Dec 13, 2019 at 02:06:31PM +0100, Cédric Le Goater wrote:
> On 13/12/2019 13:00, Greg Kurz wrote:
> > The XSCOM bus is implemented with a QOM interface, which is mostly
> > generic from a CPU type standpoint, except for the computation of
> > addresses on the Pervasize Connect Bus (PCB) network. This is handled
> 
> Pervasive

I've fixed this typo in transit.

> 
> > by the pnv_xscom_pcba() function with a switch statement based on
> > the chip_type class level attribute of the CPU chip.
> > 
> > This can be achieved using QOM. Also the address argument is masked with
> > PNV_XSCOM_SIZE - 1, which is for POWER8 only. Addresses may have different
> > sizes with other CPU types. Have each CPU chip type handle the appropriate
> > computation with a QOM xscom_pcba() method.
> 
> PnvXscom model ? :)
> 
> > Signed-off-by: Greg Kurz 
> 
> Reviewed-by: Cédric Le Goater 
> 
> > ---
> >  hw/ppc/pnv.c |   23 +++
> >  hw/ppc/pnv_xscom.c   |   14 +-
> >  include/hw/ppc/pnv.h |1 +
> >  3 files changed, 25 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0447b534b8c5..cc40b90e9cd2 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1121,6 +1121,12 @@ static void pnv_chip_power8_realize(DeviceState 
> > *dev, Error **errp)
> >  >homer.regs);
> >  }
> >  
> > +static uint32_t pnv_chip_power8_xscom_pcba(PnvChip *chip, uint64_t addr)
> > +{
> > +addr &= (PNV_XSCOM_SIZE - 1);
> > +return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> > +}
> > +
> >  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >  {
> >  DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -1138,6 +1144,7 @@ static void pnv_chip_power8e_class_init(ObjectClass 
> > *klass, void *data)
> >  k->dt_populate = pnv_chip_power8_dt_populate;
> >  k->pic_print_info = pnv_chip_power8_pic_print_info;
> >  k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> > +k->xscom_pcba = pnv_chip_power8_xscom_pcba;
> >  dc->desc = "PowerNV Chip POWER8E";
> >  
> >  device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> > @@ -1161,6 +1168,7 @@ static void pnv_chip_power8_class_init(ObjectClass 
> > *klass, void *data)
> >  k->dt_populate = pnv_chip_power8_dt_populate;
> >  k->pic_print_info = pnv_chip_power8_pic_print_info;
> >  k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> > +k->xscom_pcba = pnv_chip_power8_xscom_pcba;
> >  dc->desc = "PowerNV Chip POWER8";
> >  
> >  device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> > @@ -1184,6 +1192,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass 
> > *klass, void *data)
> >  k->dt_populate = pnv_chip_power8_dt_populate;
> >  k->pic_print_info = pnv_chip_power8_pic_print_info;
> >  k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> > +k->xscom_pcba = pnv_chip_power8_xscom_pcba;
> >  dc->desc = "PowerNV Chip POWER8NVL";
> >  
> >  device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> > @@ -1340,6 +1349,12 @@ static void pnv_chip_power9_realize(DeviceState 
> > *dev, Error **errp)
> >  >homer.regs);
> >  }
> >  
> > +static uint32_t pnv_chip_power9_xscom_pcba(PnvChip *chip, uint64_t addr)
> > +{
> > +addr &= (PNV9_XSCOM_SIZE - 1);
> > +return addr >> 3;
> > +}
> > +
> >  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >  {
> >  DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -1357,6 +1372,7 @@ static void pnv_chip_power9_class_init(ObjectClass 
> > *klass, void *data)
> >  k->dt_populate = pnv_chip_power9_dt_populate;
> >  k->pic_print_info = pnv_chip_power9_pic_print_info;
> >  k->xscom_core_base = pnv_chip_power9_xscom_core_base;
> > +k->xscom_pcba = pnv_chip_power9_xscom_pcba;
> >  dc->desc = "PowerNV Chip POWER9";
> >  
> >  device_class_set_parent_realize(dc, pnv_chip_power9_realize,
> > @@ -1422,6 +1438,12 @@ static void pnv_chip_power10_realize(DeviceState 
> > *dev, Error **errp)
> >  (uint64_t) 
> > PNV10_LPCM_BASE(chip));
> >  }
> >  
> > +static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
> > +{
> > +addr &= (PNV10_XSCOM_SIZE - 1);
> > +return addr >> 3;
> > +}
> > +
> >  static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
> >  {
> >  DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -1439,6 +1461,7 @@ static void pnv_chip_power10_class_init(ObjectClass 
> > *klass, void *data)
> >  k->dt_populate = pnv_chip_power10_dt_populate;
> >  k->pic_print_info = pnv_chip_power10_pic_print_info;
> >  k->xscom_core_base = pnv_chip_power10_xscom_core_base;
> > +k->xscom_pcba = pnv_chip_power10_xscom_pcba;
> >  dc->desc = "PowerNV Chip POWER10";
> >  
> >  device_class_set_parent_realize(dc, pnv_chip_power10_realize,
> > diff --git 

Re: [PATCH 2/2] numa: properly check if numa is supported

[Tao Xu]

On 12/13/2019 5:12 PM, Igor Mammedov wrote:

On Fri, 13 Dec 2019 09:33:10 +0800
Tao Xu  wrote:


On 12/12/2019 8:48 PM, Igor Mammedov wrote:

Commit aa57020774b, by mistake used MachineClass::numa_mem_supported
to check if NUMA is supported by machine and also as unrelated change
set it to true for sbsa-ref board.

Luckily change didn't break machines that support NUMA, as the field
is set to true for them.

But the field is not intended for checking if NUMA is supported and
will be flipped to false within this release for new machine types.

Fix it:
   - by using previously used condition
!mc->cpu_index_to_instance_props || !mc->get_default_cpu_node_id
 the first time and then use MachineState::numa_state down the road
 to check if NUMA is supported
   - dropping stray sbsa-ref chunk

Fixes: aa57020774b690a22be72453b8e91c9b5a68c516
Signed-off-by: Igor Mammedov 
---
CC: Radoslaw Biernacki 
CC: Peter Maydell 
CC: Leif Lindholm 
CC: qemu-...@nongnu.org
CC: qemu-sta...@nongnu.org


   hw/arm/sbsa-ref.c | 1 -
   hw/core/machine.c | 4 ++--
   2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 27046cc..c6261d4 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -791,7 +791,6 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
   mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
   mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
   mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
-mc->numa_mem_supported = true;
   }
   
   static const TypeInfo sbsa_ref_info = {

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3..aa63231 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -958,7 +958,7 @@ static void machine_initfn(Object *obj)
   NULL);
   }
   
-if (mc->numa_mem_supported) {

+if (mc->cpu_index_to_instance_props && mc->get_default_cpu_node_id) {
   ms->numa_state = g_new0(NumaState, 1);
   }


I am wondering if @numa_mem_supported is unused here, it is unused for
QEMU, because the only usage of @numa_mem_supported is to initialize
@numa_state. Or there is other usage? So should it be removed from
struct MachineClass?

You are wrong, it's not intended for numa_state initialization,
read doc comment for it in include/hw/boards.h
(for full story look at commit cd5ff8333a3)


I understand.

Re: [PATCH RESEND v20 0/8] Build ACPI Heterogeneous Memory Attribute Table (HMAT)

[Tao Xu]

On 12/13/2019 6:06 PM, Michael S. Tsirkin wrote:

On Fri, Dec 13, 2019 at 09:19:21AM +0800, Tao Xu wrote:

This series of patches will build Heterogeneous Memory Attribute Table (HMAT)
according to the command line. The ACPI HMAT describes the memory attributes,
such as memory side cache attributes and bandwidth and latency details,
related to the Memory Proximity Domain.
The software is expected to use HMAT information as hint for optimization.

In the linux kernel, the codes in drivers/acpi/hmat/hmat.c parse and report
the platform's HMAT tables.

The V19 patches link:
https://patchwork.kernel.org/cover/11265525/


Looks good to me, I'll queue it for merge after the release. If possible
please ping me after the release to help make sure it didn't get
dropped.



Thank you!




Changelog:
v20:
 - Resend to fix the wrong target in pc_hmat_erange_cfg()
 - Use g_assert_true and g_assert_false to replace g_assert
   (Thomas and Markus)
 - Rename assoc as associativity, update the QAPI description (Markus)
 - Disable cache level 0 in hmat-cache option (Igor)
 - Keep base and bitmap unchanged when latency or bandwidth
   out of range
 - Fix the broken CI case when user input latency or bandwidth
   less than required.
v19:
 - Add description about the machine property 'hmat' in commit
   message (Markus)
 - Update the QAPI comments
 - Add a check for no memory side cache
 - Add some fail cases for hmat-cache when level=0
v18:
 - Defer patches 01/14~06/14 of V17, use qapi type uint64 and
   only nanosecond for latency (Markus)
 - Rewrite the lines over 80 characters(Igor)
v17:
 - Add check when user input latency or bandwidth 0, the
   lb_info_provided should also be 0. Because in ACPI 6.3 5.2.27.4,
   0 means the corresponding latency or bandwidth information is
   not provided.
 - Fix the infinite loop when node->latency is 0.
 - Use NumaHmatCacheOptions to replace HMAT_Cache_Info (Igor)
 - Add check for unordered cache level input (Igor)
 - Add some fail test cases (Igor)
v16:
 - Add and use qemu_strtold_finite to parse size, support full
   64bit precision, modify related test cases (Eduardo and Markus)
 - Simplify struct HMAT_LB_Info and related code, unify latency
   and bandwidth (Igor)
 - Add cross check with hmat_lb data (Igor)
 - Fields in Cache Attributes are promoted to uint32_t before
   shifting (Igor)
 - Add case for QMP build HMAT (Igor)
v15:
 - Add a new patch to refactor do_strtosz() (Eduardo)
 - Make tests without breaking CI (Michael)
v14:
 - Reuse the codes of do_strtosz to build qemu_strtotime_ns
   (Eduardo)
 - Squash patch v13 01/12 and 02/12 together (Daniel and Eduardo)
 - Drop time unit picosecond (Eric)
 - Use qemu ctz64 and clz64 instead of builtin function
v13:
 - Modify some text description
 - Drop "initiator_valid" field in struct NodeInfo
 - Reuse Garray to store the raw bandwidth and bandwidth data
 - Calculate common base unit using range bitmap
 - Add a patch to alculate hmat latency and bandwidth entry list
 - Drop the total_levels option and use readable cache size
 - Remove the unnecessary head file
 - Use decimal notation with appropriate suffix for cache size

Liu Jingqi (5):
   numa: Extend CLI to provide memory latency and bandwidth information
   numa: Extend CLI to provide memory side cache information
   hmat acpi: Build Memory Proximity Domain Attributes Structure(s)
   hmat acpi: Build System Locality Latency and Bandwidth Information
 Structure(s)
   hmat acpi: Build Memory Side Cache Information Structure(s)

Tao Xu (3):
   numa: Extend CLI to provide initiator information for numa nodes
   tests/numa: Add case for QMP build HMAT
   tests/bios-tables-test: add test cases for ACPI HMAT

  hw/acpi/Kconfig   |   7 +-
  hw/acpi/Makefile.objs |   1 +
  hw/acpi/hmat.c| 268 +++
  hw/acpi/hmat.h|  42 
  hw/core/machine.c |  64 ++
  hw/core/numa.c| 297 ++
  hw/i386/acpi-build.c  |   5 +
  include/sysemu/numa.h |  63 ++
  qapi/machine.json | 180 +++-
  qemu-options.hx   |  95 +++-
  tests/bios-tables-test-allowed-diff.h |   8 +
  tests/bios-tables-test.c  |  44 
  tests/data/acpi/pc/APIC.acpihmat  |   0
  tests/data/acpi/pc/DSDT.acpihmat  |   0
  tests/data/acpi/pc/HMAT.acpihmat  |   0
  tests/data/acpi/pc/SRAT.acpihmat  |   0
  tests/data/acpi/q35/APIC.acpihmat |   0
  tests/data/acpi/q35/DSDT.acpihmat |   0
  tests/data/acpi/q35/HMAT.acpihmat |   0
  tests/data/acpi/q35/SRAT.acpihmat |   0
  tests/numa-test.c | 213 ++
  21 

Re: [PATCH 00/10] Fixes for DP8393X SONIC device emulation

[Finn Thain]

On Sun, 15 Dec 2019, Finn Thain wrote:

> I test the qemu build like this,
> 
> qemu-system-m68k -M q800 -m 512M -serial none -serial mon:stdio -g 800x600x4
> -net nic,model=dp83932,addr=00:00:00:01:02:03
> -net bridge,helper=/opt/qemu/libexec/qemu-bridge-helper,br=br0
> -append "fbcon=font:ProFont6x11 console=tty0 console=ttyS0 ignore_loglevel"
> -kernel vmlinux-4.14.157-mac-backport+
> -initrd /mnt/loop/install/cdrom/initrd.gz
> 
> You can obtain this kernel binary from the linux-mac68k project on 
> sourceforge. (I usually use a mainline Linux build but it makes no 
> difference.)
> 

One difficulty with testing these patches with Linux guests is some old 
bugs in drivers/net/ethernet/natsemi/sonic.c that can cause tx watchdog 
timeouts on real hardware.

I have some patches for that driver which may be useful when testing 
QEMU's hw/net/dp8393x.c device. (I've pushed those patches to my github 
repo.)

The second obstacle I have involves testing the dp8393x device with a 
bridge device on a Linux/i686 host.

Running tcpdump in the Linux/m68k guest showed these two ping packets from 
the host,

00:15:28.480164 IP 192.168.66.1 > 192.168.66.111: ICMP echo request, id 23957, 
seq 11, length 64
0x:  0800 0702 0304 fe16 d9ae 6943 0800 4500  ..iC..E.
0x0010:  0054 ff4d 4000 4001 359a c0a8 4201 c0a8  .T.M@.@.5...B...
0x0020:  426f 0800 4243 5d95 000b a0cc f65d cfee  Bo..BC]..]..
0x0030:  0600 0809 0a0b 0c0d 0e0f 1011 1213 1415  
0x0040:  1617 1819 1a1b 1c1d 1e1f 2021 2223 2425  ...!"#$%
0x0050:  2627 2829 2a2b 2c2d 2e2f 3031 3233 3435  &'()*+,-./012345
0x0060:  3637 33e0 14c7   673...
00:15:29.341601 IP truncated-ip - 52 bytes missing! 192.168.66.1 > 
192.168.66.111: ICMP echo request, id 23957, seq 12, length 64
0x:  0800 0702 0304 fe16 d9ae 6943 0800 4500  ..iC..E.
0x0010:  0054 ff4e 4000 4001 3599 c0a8 4201 c0a8  .T.N@.@.5...B...
0x0020:  426f 0800 d61a 5d95 000c a0cc f65dBo]..]

Sniffing br0 on the host shows no sign of the truncated packet at all 
which leaves a gap in the packet sequence numbers captured on the host. 
Weird.

When I log the calls to,

static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
   size_t pkt_size)

the corresponding pkt_size values look like this,

pkt_size 98
pkt_size 42

So this seems to show that the bug is not in dp8393x. Possibly not in 
QEMU?

I don't see any options in 'man brctl' that might explain why the host and 
guest see different packets. I guess I'll have to find a way to avoid 
using bridge interfaces (?)

[PATCH 1/2] hw/pci/pci_host: Remove redundant PCI_DPRINTF()

[Philippe Mathieu-Daudé]

In commit 3bf4dfdd111 we introduced the pci_cfg_[read/write]
trace events in pci_host_config_[read/write]_common().
We have the following call trace:

  pci_host_data_[read/write]()
- PCI_DPRINTF()
- pci_data_[read/write]()
- PCI_DPRINTF()
- pci_host_config_[read/write]_common()
trace_pci_cfg_[read/write]()

Since the PCI_DPRINTF() calls are redundant with the trace
events, remove them.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/pci/pci_host.c | 21 +
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index c5f9244934..0958d157de 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -115,8 +115,6 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, 
int len)
 return;
 }
 
-PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
-__func__, pci_dev->name, config_addr, val, len);
 pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
  val, len);
 }
@@ -125,18 +123,13 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
 PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
 uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
-uint32_t val;
 
 if (!pci_dev) {
 return ~0x0;
 }
 
-val = pci_host_config_read_common(pci_dev, config_addr,
-  PCI_CONFIG_SPACE_SIZE, len);
-PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-__func__, pci_dev->name, config_addr, val, len);
-
-return val;
+return pci_host_config_read_common(pci_dev, config_addr,
+   PCI_CONFIG_SPACE_SIZE, len);
 }
 
 static void pci_host_config_write(void *opaque, hwaddr addr,
@@ -167,8 +160,7 @@ static void pci_host_data_write(void *opaque, hwaddr addr,
 uint64_t val, unsigned len)
 {
 PCIHostState *s = opaque;
-PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n",
-addr, len, (unsigned)val);
+
 if (s->config_reg & (1u << 31))
 pci_data_write(s->bus, s->config_reg | (addr & 3), val, len);
 }
@@ -177,14 +169,11 @@ static uint64_t pci_host_data_read(void *opaque,
hwaddr addr, unsigned len)
 {
 PCIHostState *s = opaque;
-uint32_t val;
+
 if (!(s->config_reg & (1U << 31))) {
 return 0x;
 }
-val = pci_data_read(s->bus, s->config_reg | (addr & 3), len);
-PCI_DPRINTF("read addr " TARGET_FMT_plx " len %d val %x\n",
-addr, len, val);
-return val;
+return pci_data_read(s->bus, s->config_reg | (addr & 3), len);
 }
 
 const MemoryRegionOps pci_host_conf_le_ops = {
-- 
2.21.0

[PATCH 2/2] hw/pci/pci_host: Let pci_data_[read/write] use unsigned 'size' argument

[Philippe Mathieu-Daudé]

Both functions are called by MemoryRegionOps.[read/write] handlers
with unsigned 'size' argument. Both functions call
pci_host_config_[read/write]_common() which expect a uint32_t 'len'
parameter (also unsigned).
Since it is pointless (and confuse) to use a signed value, use a
unsigned type.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/pci/pci_host.h | 4 ++--
 hw/pci/pci_host.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index ba31595fc7..9ce088bd13 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -62,8 +62,8 @@ void pci_host_config_write_common(PCIDevice *pci_dev, 
uint32_t addr,
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
  uint32_t limit, uint32_t len);
 
-void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
-uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
+void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, unsigned len);
+uint32_t pci_data_read(PCIBus *s, uint32_t addr, unsigned len);
 
 extern const MemoryRegionOps pci_host_conf_le_ops;
 extern const MemoryRegionOps pci_host_conf_be_ops;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 0958d157de..ce7bcdb1d5 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -106,7 +106,7 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, 
uint32_t addr,
 return ret;
 }
 
-void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
+void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, unsigned len)
 {
 PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
 uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
@@ -119,7 +119,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, 
int len)
  val, len);
 }
 
-uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
+uint32_t pci_data_read(PCIBus *s, uint32_t addr, unsigned len)
 {
 PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
 uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
-- 
2.21.0

[PATCH 0/2] hw/pci/pci_host: Small cleanups

[Philippe Mathieu-Daudé]

- Use unsigned 'size' argument
- Remove unuseful DPRINTF()

Philippe Mathieu-Daudé (2):
  hw/pci/pci_host: Remove redundant PCI_DPRINTF()
  hw/pci/pci_host: Let pci_data_[read/write] use unsigned 'size'
argument

 include/hw/pci/pci_host.h |  4 ++--
 hw/pci/pci_host.c | 25 +++--
 2 files changed, 9 insertions(+), 20 deletions(-)

-- 
2.21.0

Re: [PATCH 05/10] arm: allwinner-h3: add System Control module

[Philippe Mathieu-Daudé]

On 12/16/19 12:27 AM, Niek Linnenbank wrote:
On Fri, Dec 13, 2019 at 1:09 AM Philippe Mathieu-Daudé 
mailto:phi...@redhat.com>> wrote:


On 12/2/19 10:09 PM, Niek Linnenbank wrote:

[...]

 > +static const MemoryRegionOps allwinner_h3_syscon_ops = {
 > +    .read = allwinner_h3_syscon_read,
 > +    .write = allwinner_h3_syscon_write,
 > +    .endianness = DEVICE_NATIVE_ENDIAN,
 > +    .valid = {
 > +        .min_access_size = 4,
 > +        .max_access_size = 4,

Can you point me to the datasheet page that says this region is
restricted to 32-bit accesses? Maybe you want .valid -> .impl instead?

Hehe well here I can only give the same answer as for the SD/MMC driver: 
the datasheet
only provides the base address and register offsets, but nothing 
explicitely mentioned about alignment.

I do see that also for this device the registers are 32-bit aligned.

Does that mean I should change MemoryRegionOps to . impl instead?


No, keep them, but add ".impl.min_access_size = 4" (see answer to SD/MMC 
model patch).

Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller

[Philippe Mathieu-Daudé]

On 12/16/19 12:07 AM, Niek Linnenbank wrote:



On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
mailto:phi...@redhat.com>> wrote:


Hi Niek,

On 12/11/19 11:34 PM, Niek Linnenbank wrote:

[...]

 >     +static uint32_t aw_h3_sdhost_process_desc(AwH3SDHostState *s,
 >     +                                          hwaddr desc_addr,
 >     +                                          TransferDescriptor
*desc,
 >     +                                          bool is_write,
uint32_t
 >     max_bytes)
 >     +{
 >     +    uint32_t num_done = 0;
 >     +    uint32_t num_bytes = max_bytes;
 >     +    uint8_t buf[1024];
 >     +
 >     +    /* Read descriptor */
 >     +    cpu_physical_memory_read(desc_addr, desc, sizeof(*desc));

Should we worry about endianess here?


I tried to figure out what is expected, but the 
Allwinner_H3_Datasheet_V1.2.pdf does not
explicitly mention endianness for any of its I/O devices. Currently it 
seems all devices are
happy with using the same endianness as the CPUs. In the MemoryRegionOps 
has DEVICE_NATIVE_ENDIAN

set to match the behavior seen.


OK.

[...]

 >     +static const MemoryRegionOps aw_h3_sdhost_ops = {
 >     +    .read = aw_h3_sdhost_read,
 >     +    .write = aw_h3_sdhost_write,
 >     +    .endianness = DEVICE_NATIVE_ENDIAN,

I haven't checked .valid accesses from the datasheet.

However due to:

    res = s->data_crc[((offset - REG_SD_DATA7_CRC) / sizeof(uint32_t))];

You seem to expect:

             .impl.min_access_size = 4,

.impl.max_access_size unset is 8, which should works.

It seems that all registers are aligned on at least 32-bit boundaries. 
And the section 5.3.5.1 mentions
that the DMA descriptors must be stored in memory 32-bit aligned. So 
based on that information,


So you are describing ".valid.min_access_size = 4", which is the minimum 
access size on the bus.
".impl.min_access_size" is different, it is what access sizes is ready 
to handle your model.
Your model read/write handlers expect addresses aligned on 32-bit 
boundary, this is why I suggested to use ".impl.min_access_size = 4". If 
the guest were using a 16-bit access, your model would be buggy. If you 
describe your implementation to accept minimum 32-bit and the guest is 
allowed to use smaller accesses, QEMU will do a 32-bit access to the 
device, and return the 16-bit part to the guest. This way your model is 
safe. This is done by access_with_adjusted_size() in memory.c.
If you restrict with ".valid.min_access_size = 4", you might think we 
don't need ".valid.min_access_size = 4" because all access from guest 
will be at least 32-bit. However keep in mind someone might find this 
device in another datasheet not limited to 32-bit, and let's say change 
to ".valid.min_access_size = 2". Without ".impl.min_access_size = 4" 
your model is buggy. So to be safe I'd use:


  .impl.min_access_size = 4,
  .valid.min_access_size = 4,


I think 32-bit is a safe choice. I've verified this with Linux mainline 
and U-Boot drivers and both are still working.


Regards,

Phil.

Re: [PATCH 05/10] arm: allwinner-h3: add System Control module

[Niek Linnenbank]

On Fri, Dec 13, 2019 at 1:09 AM Philippe Mathieu-Daudé 
wrote:

> On 12/2/19 10:09 PM, Niek Linnenbank wrote:
> > The Allwinner H3 System on Chip has an System Control
> > module that provides system wide generic controls and
> > device information. This commit adds support for the
> > Allwinner H3 System Control module.
> >
> > Signed-off-by: Niek Linnenbank 
> > ---
> >   hw/arm/allwinner-h3.c |  11 ++
> >   hw/misc/Makefile.objs |   1 +
> >   hw/misc/allwinner-h3-syscon.c | 139 ++
> >   include/hw/arm/allwinner-h3.h |   2 +
> >   include/hw/misc/allwinner-h3-syscon.h |  43 
> >   5 files changed, 196 insertions(+)
> >   create mode 100644 hw/misc/allwinner-h3-syscon.c
> >   create mode 100644 include/hw/misc/allwinner-h3-syscon.h
> >
> > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> > index afeb49c0ac..ebd8fde412 100644
> > --- a/hw/arm/allwinner-h3.c
> > +++ b/hw/arm/allwinner-h3.c
> > @@ -41,6 +41,9 @@ static void aw_h3_init(Object *obj)
> >
> >   sysbus_init_child_obj(obj, "ccu", >ccu, sizeof(s->ccu),
> > TYPE_AW_H3_CLK);
> > +
> > +sysbus_init_child_obj(obj, "syscon", >syscon, sizeof(s->syscon),
> > +  TYPE_AW_H3_SYSCON);
> >   }
> >
> >   static void aw_h3_realize(DeviceState *dev, Error **errp)
> > @@ -184,6 +187,14 @@ static void aw_h3_realize(DeviceState *dev, Error
> **errp)
> >   }
> >   sysbus_mmio_map(SYS_BUS_DEVICE(>ccu), 0, AW_H3_CCU_BASE);
> >
> > +/* System Control */
> > +object_property_set_bool(OBJECT(>syscon), true, "realized",
> );
> > +if (err) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +sysbus_mmio_map(SYS_BUS_DEVICE(>syscon), 0, AW_H3_SYSCON_BASE);
> > +
> >   /* Universal Serial Bus */
> >   sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
> >s->irq[AW_H3_GIC_SPI_EHCI0]);
> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> > index 200ed44ce1..b234aefba5 100644
> > --- a/hw/misc/Makefile.objs
> > +++ b/hw/misc/Makefile.objs
> > @@ -29,6 +29,7 @@ common-obj-$(CONFIG_MACIO) += macio/
> >   common-obj-$(CONFIG_IVSHMEM_DEVICE) += ivshmem.o
> >
> >   common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-clk.o
> > +common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-syscon.o
> >   common-obj-$(CONFIG_REALVIEW) += arm_sysctl.o
> >   common-obj-$(CONFIG_NSERIES) += cbus.o
> >   common-obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
> > diff --git a/hw/misc/allwinner-h3-syscon.c
> b/hw/misc/allwinner-h3-syscon.c
> > new file mode 100644
> > index 00..66bd518a05
> > --- /dev/null
> > +++ b/hw/misc/allwinner-h3-syscon.c
> > @@ -0,0 +1,139 @@
> > +/*
> > + * Allwinner H3 System Control emulation
> > + *
> > + * Copyright (C) 2019 Niek Linnenbank 
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see  >.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/sysbus.h"
> > +#include "migration/vmstate.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "hw/misc/allwinner-h3-syscon.h"
> > +
> > +/* SYSCON register offsets */
> > +#define REG_VER (0x24)  /* Version */
> > +#define REG_EMAC_PHY_CLK(0x30)  /* EMAC PHY Clock */
> > +#define REG_INDEX(offset)   (offset / sizeof(uint32_t))
> > +
> > +/* SYSCON register reset values */
> > +#define REG_VER_RST (0x0)
> > +#define REG_EMAC_PHY_CLK_RST(0x58000)
> > +
> > +static uint64_t allwinner_h3_syscon_read(void *opaque, hwaddr offset,
> > + unsigned size)
> > +{
> > +const AwH3SysconState *s = (AwH3SysconState *)opaque;
> > +const uint32_t idx = REG_INDEX(offset);
> > +
> > +if (idx >= AW_H3_SYSCON_REGS_NUM) {
> > +qemu_log_mask(LOG_GUEST_ERROR, "%s: bad read offset 0x%04x\n",
> > +  __func__, (uint32_t)offset);
> > +return 0;
> > +}
> > +
> > +return s->regs[idx];
> > +}
> > +
> > +static void allwinner_h3_syscon_write(void *opaque, hwaddr offset,
> > +  uint64_t val, unsigned size)
> > +{
> > +AwH3SysconState *s = (AwH3SysconState *)opaque;
> > +const uint32_t idx = REG_INDEX(offset);
> > +
> > +if (idx >= 

Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller

[Niek Linnenbank]

On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé 
wrote:

> Hi Niek,
>
> On 12/11/19 11:34 PM, Niek Linnenbank wrote:
> > Ping!
> >
> > Anyone would like to comment on this driver?
> >
> > I finished the rework on all previous comments in this series.
> >
> > Currently debugging the hflags error reported by Philippe.
> > After that, I'm ready to send out v2 of these patches.
> >
> > Regards,
> > Niek
> >
> > On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank
> > mailto:nieklinnenb...@gmail.com>> wrote:
> >
> > The Allwinner H3 System on Chip contains an integrated storage
> > controller for Secure Digital (SD) and Multi Media Card (MMC)
> > interfaces. This commit adds support for the Allwinner H3
> > SD/MMC storage controller with the following emulated features:
> >
> >   * DMA transfers
> >   * Direct FIFO I/O
> >   * Short/Long format command responses
> >   * Auto-Stop command (CMD12)
> >   * Insert & remove card detection
> >
> > Signed-off-by: Niek Linnenbank  > >
> > ---
> >   hw/arm/allwinner-h3.c   |  20 +
> >   hw/arm/orangepi.c   |  17 +
> >   hw/sd/Makefile.objs |   1 +
> >   hw/sd/allwinner-h3-sdhost.c | 791
> 
> >   hw/sd/trace-events  |   7 +
> >   include/hw/arm/allwinner-h3.h   |   2 +
> >   include/hw/sd/allwinner-h3-sdhost.h |  73 +++
> >   7 files changed, 911 insertions(+)
> >   create mode 100644 hw/sd/allwinner-h3-sdhost.c
> >   create mode 100644 include/hw/sd/allwinner-h3-sdhost.h
> >
> > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> > index 4fc4c8c725..c2972caf88 100644
> > --- a/hw/arm/allwinner-h3.c
> > +++ b/hw/arm/allwinner-h3.c
> > @@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)
> >
> >   sysbus_init_child_obj(obj, "sid", >sid, sizeof(s->sid),
> > TYPE_AW_H3_SID);
> > +
> > +sysbus_init_child_obj(obj, "mmc0", >mmc0, sizeof(s->mmc0),
> > +  TYPE_AW_H3_SDHOST);
> >   }
> >
> >   static void aw_h3_realize(DeviceState *dev, Error **errp)
> > @@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev,
> > Error **errp)
> >   }
> >   sysbus_mmio_map(SYS_BUS_DEVICE(>sid), 0, AW_H3_SID_BASE);
> >
> > +/* SD/MMC */
> > +object_property_set_bool(OBJECT(>mmc0), true, "realized",
> );
> > +if (err != NULL) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +sysbusdev = SYS_BUS_DEVICE(>mmc0);
> > +sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
> > +sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
> > +
> > +object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(>mmc0),
> > +  "sd-bus", );
> > +if (err) {
> > +error_propagate(errp, err);
> > +return;
> > +}
> > +
> >   /* Universal Serial Bus */
> >   sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
> >s->irq[AW_H3_GIC_SPI_EHCI0]);
> > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> > index 5ef2735f81..dee3efaf08 100644
> > --- a/hw/arm/orangepi.c
> > +++ b/hw/arm/orangepi.c
> > @@ -39,6 +39,10 @@ typedef struct OrangePiState {
> >   static void orangepi_init(MachineState *machine)
> >   {
> >   OrangePiState *s = g_new(OrangePiState, 1);
> > +DriveInfo *di;
> > +BlockBackend *blk;
> > +BusState *bus;
> > +DeviceState *carddev;
> >   Error *err = NULL;
> >
> >   s->h3 = AW_H3(object_new(TYPE_AW_H3));
> > @@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
> >   exit(1);
> >   }
> >
> > +/* Create and plug in the SD card */
> > +di = drive_get_next(IF_SD);
> > +blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
> > +if (bus == NULL) {
> > +error_report("No SD/MMC found in H3 object");
> > +exit(1);
> > +}
>
> Your device always creates a bus, so I don't think the if(bus) check is
> worthwhile. Eventually use an assert(bus)?
>
> > +carddev = qdev_create(bus, TYPE_SD_CARD);
> > +qdev_prop_set_drive(carddev, "drive", blk, _fatal);
> > +object_property_set_bool(OBJECT(carddev), true, "realized",
> > _fatal);
> > +
> >   /* RAM */
> >   memory_region_allocate_system_memory(>sdram, NULL,
> > "orangepi.ram",
> >machine->ram_size);
> > @@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
> >   {
> >   mc->desc = "Orange Pi PC";
> >   mc->init = 

Re: [PATCH v3 5/5] python/qemu: Remove unneeded imports in __init__

[Cleber Rosa]

On Thu, Dec 12, 2019 at 07:58:31AM -0500, Wainer dos Santos Moschetta wrote:
> __init_.py import some sub-modules unnecessarily. So let's
> clean it up.
> 
> Signed-off-by: Wainer dos Santos Moschetta 
> Suggested-by: Cleber Rosa 
> ---
>  python/qemu/__init__.py | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index eff17a306e..4ca06c34a4 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -9,9 +9,3 @@
>  # This work is licensed under the terms of the GNU GPL, version 2.  See
>  # the COPYING file in the top-level directory.
>  #
> -# Based on qmp.py.
> -#
> -
> -from . import qmp
> -from . import machine
> -from . import accel
> -- 
> 2.21.0
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH] ppc: remove excessive logging

[Joakim Tjernlund]

On Sun, 2019-12-15 at 11:59 +0100, BALATON Zoltan wrote:
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open attachments unless you recognize the sender and know the 
> content is safe.
> 
> 
> On Sun, 15 Dec 2019, Philippe Mathieu-Daudé wrote:
> > Hi Joakim,
> > 
> > I'm cc'ing the PPC maintainers for you, so they won't miss your patch (see
> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.qemu.org%2FContribute%2FSubmitAPatch%23CC_the_relevant_maintainerdata=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C8fd615a611ec4bd9cce408d7814dda1a%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637120043688298205sdata=d433DXO8SEaFJqAu73VTQwkZptmvK2eMAxivELGMcMI%3Dreserved=0
> >  and
> > the output of ./scripts/get_maintainer.pl -f target/ppc/excp_helper.c).
> > 
> > On 12/14/19 1:13 PM, Joakim Tjernlund wrote:
> > > From: Joakim Tjernlund 
> > > 
> > > ppc logs every type of Invalid instruction. This generates a lot
> > > of garbage on console when sshd/ssh_keygen executes as
> > > they try various insn to optimize its performance.
> > > The invalid operation log is still there so an unknown insn
> > > will still be logged.
> > > 
> > > Signed-off-by: Joakim Tjernlund 
> > > ---
> > > 
> > >  As far as I can see, ppc is the only one emiting thsi
> > >  debug msg for Invalid insns.
> > > 
> > > target/ppc/excp_helper.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > > index 50b004d00d..45c2fa3ff9 100644
> > > --- a/target/ppc/excp_helper.c
> > > +++ b/target/ppc/excp_helper.c
> > > @@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> > > excp_model, int excp)
> > >   env->spr[SPR_BOOKE_ESR] = ESR_FP;
> > >   break;
> > >   case POWERPC_EXCP_INVAL:
> > > -LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n",
> > > env->nip);
> > 
> > I don't think we want to remove a such important log. Since it make sense to
> > not disturb the console, maybe we can replace some of the LOG_EXCP() calls 
> > by
> > qemu_log_mask(LOG_GUEST_ERROR,...) instead?

Why is that so important? As far as I can tell ppc is the only arch doing this?

> 
> I don't think that's a good idea. That would flood the -d guest_errors log
> with unwanted messages that are normal not really due to guest errors. The

It not OK to flood some log but OK to flood the console?

> LOG_EXCP() is not enabled by default, you have to edit source to enable it

LOG_EXCP is enabled on Gentoo, what about other distros?

> so you can also then edit the unwanted messages as well (like commenting
> this one out if you don't like it). If this is removed, invalid opcodes
> are still logged from translate.c but the exception generated for them is
> not logged. I don't know if that's useful or not but these are debug logs
> so depends on what do you want to debug. I don't mind this being removed
> but would be also happy leaving it as it is as it's only shown for
> developers who enable it and might help debugging. Or maybe these could be
> converted to traces (although I generally find traces to be less
> convenient to work with than debug logs and not sure about potential
> performance impact). So my preferences would be in order: 1. leave it
> alone, 2. remove it, 3. convert to traces.
> 
> Regards,
> BALATON Zoltan
> 
> > >   msr |= 0x0008;
> > >   env->spr[SPR_BOOKE_ESR] = ESR_PIL;
> > >   break;

Re: [PATCH] linux-user: make binfmt flag O require P

[Joakim Tjernlund]

On Sun, 2019-12-15 at 17:54 +0100, Laurent Vivier wrote:
> 
> Le 14/12/2019 à 13:20, Joakim Tjernlund a écrit :
> > From: Joakim Tjernlund 
> > 
> > QEMU can autodetect if it is started from Linux binfmt loader
> > when binfmt flag O is on.
> > Use that and require binfmt flag P as well which will enable QEMU
> > to pass in correct argv0 to the application.
> 
> I agree it's a good idea to try to detect the P flag if we use the O but
> it changes the current behavior and breaks the compatibility with
> previous version. This will prevent to run old and new version of QEMU
> on the same system.

Only if you already are using O flag only. Distributions can adjust so that O 
and P
are used in tandem.

> 
> I already try to find a solution to this problem.
> 
> The first one is at QEMU level:
> 
> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchew.org%2FQEMU%2F20191024153847.31815-1-laurent%40vivier.eu%2Fdata=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C5c4859947f4949d8824d08d7817f7aef%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C637120256850791761sdata=w6YgDlrUPxk7BSPdOVRyyWzb64usUiF9EMSOMonskPY%3Dreserved=0
> 
> Another would be at linux level to provide a way to detect binfmt flags
> (like you try to do with AT_EXECFD):

The kernel way to report flags are really the way forward here. There seems
to be little interest from QEMU community to fix this though, why is that?


 Jocke
> 
> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1158151%2Fdata=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C5c4859947f4949d8824d08d7817f7aef%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C637120256850791761sdata=abV%2BELktrjTOCS6gEp38%2BYuj17HQfCclj0YhyN0X7Bg%3Dreserved=0
> 
> I also found another one from another author:
> 
> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F10902935%2Fdata=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C5c4859947f4949d8824d08d7817f7aef%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C1%7C637120256850791761sdata=TftL59%2Baj2IAAoQ6Femfb8w%2F6%2FGc%2FjrM49iQvw4b1MM%3Dreserved=0
> 
> Thanks,
> Laurent
> 
> 
> > Signed-off-by: Joakim Tjernlund 
> > ---
> >  linux-user/main.c | 18 ++
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 6ff7851e86..1b626e5762 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -544,7 +544,7 @@ static void usage(int exitcode)
> >  exit(exitcode);
> >  }
> > 
> > -static int parse_args(int argc, char **argv)
> > +static int parse_args(int argc, char **argv, int assume_P_flag)
> >  {
> >  const char *r;
> >  int optind;
> > @@ -560,7 +560,17 @@ static int parse_args(int argc, char **argv)
> >  arginfo->handle_opt(r);
> >  }
> >  }
> > -
> > +if (assume_P_flag) {
> > +/* Assume started by binmisc and binfmt P flag is set */
> > +if (argc < 3) {
> > +fprintf(stderr, "%s: Please use me through binfmt with P 
> > flag\n",
> > +argv[0]);
> > +exit(1);
> > +}
> > +exec_path = argv[1];
> > +/* Next argv must be argv0 for the app */
> > +return 2;
> > +}
> >  optind = 1;
> >  for (;;) {
> >  if (optind >= argc) {
> > @@ -659,7 +669,8 @@ int main(int argc, char **argv, char **envp)
> >  qemu_add_opts(_trace_opts);
> >  qemu_plugin_add_opts();
> > 
> > -optind = parse_args(argc, argv);
> > +execfd = qemu_getauxval(AT_EXECFD);
> > +optind = parse_args(argc, argv,  execfd > 0);
> > 
> >  if (!trace_init_backends()) {
> >  exit(1);
> > @@ -682,7 +693,6 @@ int main(int argc, char **argv, char **envp)
> > 
> >  init_qemu_uname_release();
> > 
> > -execfd = qemu_getauxval(AT_EXECFD);
> >  if (execfd == 0) {
> >  execfd = open(exec_path, O_RDONLY);
> >  if (execfd < 0) {
> > 

Re: [PATCH v3 2/5] python/qemu: accel: Add list_accel() method

[Cleber Rosa]

On Thu, Dec 12, 2019 at 07:58:28AM -0500, Wainer dos Santos Moschetta wrote:
> Since commit cbe6d6365a48 the command `qemu -accel help` returns
> the list of accelerators enabled in the QEMU binary. This adds
> the list_accel() method which return that same list.
> 
> Signed-off-by: Wainer dos Santos Moschetta 
> Reviewed-by: Alex Bennée 
> Reviewed-by: Cleber Rosa 
> Reviewed-by: Philippe Mathieu-Daudé 

The degree of changes is certainly subjective, but consider clearing
"Reviewed-by"s, according to:

https://wiki.qemu.org/Contribute/SubmitAPatch#Proper_use_of_Reviewed-by:_tags_can_aid_review

Alex, Phillipe,

You re-review on this patch is highly appreciated anyway.

> ---
>  python/qemu/accel.py | 23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index cbeac10dd1..ddcdbfd9ae 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -14,7 +14,11 @@ accelerators.
>  # the COPYING file in the top-level directory.
>  #
>  
> +import logging
>  import os
> +import subprocess
> +
> +LOG = logging.getLogger(__name__)
>  
>  # Mapping host architecture to any additional architectures it can
>  # support which often includes its 32 bit cousin.
> @@ -23,6 +27,25 @@ ADDITIONAL_ARCHES = {
>  "aarch64" : "armhf"
>  }
>  
> +def list_accel(qemu_bin):
> +"""
> +List accelerators enabled in the QEMU binary.
> +
> +@param qemu_bin (str): path to the QEMU binary.
> +@raise Exception: if failed to run `qemu -accel help`
> +@return a list of accelerator names.
> +"""
> +if not qemu_bin:
> +return []
> +try:
> +out = subprocess.check_output([qemu_bin, '-accel', 'help'],
> +  universal_newlines=True)
> +except:

This is a "generally frowned upon" naked except, but given that its
goal is to present the error to the user, and that it re-raises the
exception, it's much less frowned upon, so it LGTM.

> +LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)

The ideal use of the logging module log functions is to let them
format the output, passing those values as arguments, ie:

   LOG.debug("Failed to get the list of accelerators in %s", qemu_bin)

See https://docs.python.org/3.7/library/logging.html#logging.Logger.debug

And sorry for failing to catch this on v2.

- Cleber.

> +raise
> +# Skip the first line which is the header.
> +return [acc.strip() for acc in out.splitlines()[1:]]
> +
>  def kvm_available(target_arch=None):
>  host_arch = os.uname()[4]
>  if target_arch and target_arch != host_arch:
> -- 
> 2.21.0
> 


signature.asc
Description: PGP signature

Re: [PATCH v3 1/5] python/qemu: Move kvm_available() to its own module

[Cleber Rosa]

On Thu, Dec 12, 2019 at 07:58:27AM -0500, Wainer dos Santos Moschetta wrote:
> This creates the 'accel' Python module to be the home for
> utilities that deal with accelerators. Also moved kvm_available()
> from __init__.py to this new module.
> 
> Signed-off-by: Wainer dos Santos Moschetta 
> Reviewed-by: Alex Bennée 
> Reviewed-by: Philippe Mathieu-Daudé 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] tests/acceptance: Makes linux_initrd and empty_cpu_model use QEMUMachine

[Cleber Rosa]

On Wed, Dec 11, 2019 at 01:55:36PM -0500, Wainer dos Santos Moschetta wrote:
> On linux_initrd and empty_cpu_model tests the same effect of
> calling QEMU through run() to inspect the terminated process is
> achieved with a sequence of set_qmp_monitor() / launch() / wait()
> commands on an QEMUMachine object. This patch changes those
> tests to use QEMUMachine instead, so they follow the same pattern
> to launch QEMU found on other acceptance tests.
> 
> Signed-off-by: Wainer dos Santos Moschetta 
> Reviewed-by: Cleber Rosa 
> Tested-by: Cleber Rosa 
> ---

FIY, queued on my python-next branch.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] python/qemu: Add set_qmp_monitor() to QEMUMachine

[Cleber Rosa]

On Fri, Dec 13, 2019 at 10:46:17AM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 12/12/19 12:13 PM, Cleber Rosa wrote:
> > On Wed, Dec 11, 2019 at 01:55:35PM -0500, Wainer dos Santos Moschetta wrote:
> > > The QEMUMachine VM has a monitor setup on which an QMP
> > > connection is always attempted on _post_launch() (executed
> > > by launch()). In case the QEMU process immediatly exits
> > > then the qmp.accept() (used to establish the connection) stalls
> > > until it reaches timeout and consequently an exception raises.
> > > 
> > > That behavior is undesirable when, for instance, it needs to
> > > gather information from the QEMU binary ($ qemu -cpu list) or a
> > > test which launches the VM expecting its failure.
> > > 
> > > This patch adds the set_qmp_monitor() method to QEMUMachine that
> > > allows turn off the creation of the monitor machinery on VM launch.
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta 
> > > Reviewed-by: Cleber Rosa 
> > > ---

FIY, queued on my python-next branch.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH] linux-user: make binfmt flag O require P

[Laurent Vivier]

Le 14/12/2019 à 13:20, Joakim Tjernlund a écrit :
> From: Joakim Tjernlund 
> 
> QEMU can autodetect if it is started from Linux binfmt loader
> when binfmt flag O is on.
> Use that and require binfmt flag P as well which will enable QEMU
> to pass in correct argv0 to the application.

I agree it's a good idea to try to detect the P flag if we use the O but
it changes the current behavior and breaks the compatibility with
previous version. This will prevent to run old and new version of QEMU
on the same system.

I already try to find a solution to this problem.

The first one is at QEMU level:

https://patchew.org/QEMU/20191024153847.31815-1-laur...@vivier.eu/

Another would be at linux level to provide a way to detect binfmt flags
(like you try to do with AT_EXECFD):

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

I also found another one from another author:

https://patchwork.kernel.org/patch/10902935/

Thanks,
Laurent


> Signed-off-by: Joakim Tjernlund 
> ---
>  linux-user/main.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 6ff7851e86..1b626e5762 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -544,7 +544,7 @@ static void usage(int exitcode)
>  exit(exitcode);
>  }
>  
> -static int parse_args(int argc, char **argv)
> +static int parse_args(int argc, char **argv, int assume_P_flag)
>  {
>  const char *r;
>  int optind;
> @@ -560,7 +560,17 @@ static int parse_args(int argc, char **argv)
>  arginfo->handle_opt(r);
>  }
>  }
> -
> +if (assume_P_flag) {
> +/* Assume started by binmisc and binfmt P flag is set */
> +if (argc < 3) {
> +fprintf(stderr, "%s: Please use me through binfmt with P flag\n",
> +argv[0]);
> +exit(1);
> +}
> +exec_path = argv[1];
> +/* Next argv must be argv0 for the app */
> +return 2;
> +}
>  optind = 1;
>  for (;;) {
>  if (optind >= argc) {
> @@ -659,7 +669,8 @@ int main(int argc, char **argv, char **envp)
>  qemu_add_opts(_trace_opts);
>  qemu_plugin_add_opts();
>  
> -optind = parse_args(argc, argv);
> +execfd = qemu_getauxval(AT_EXECFD);
> +optind = parse_args(argc, argv,  execfd > 0);
>  
>  if (!trace_init_backends()) {
>  exit(1);
> @@ -682,7 +693,6 @@ int main(int argc, char **argv, char **envp)
>  
>  init_qemu_uname_release();
>  
> -execfd = qemu_getauxval(AT_EXECFD);
>  if (execfd == 0) {
>  execfd = open(exec_path, O_RDONLY);
>  if (execfd < 0) {
> 

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Peter Maydell]

On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin  wrote:
>
> On Sat, Dec 14, 2019 at 04:28:08PM +, Peter Maydell wrote:
> > (It doesn't actually assert that it doesn't
> > overlap because we have some legacy uses, notably
> > in the x86 PC machines, which do overlap without using
> > the right function, which we've never tried to tidy up.)
>
> It's not exactly legacy uses.
>
> To be more exact, the way the non overlap versions
> are *used* is to mean "I don't care what happens when they overlap"
> as opposed to "will never overlap".

Almost all of the use of the non-overlap versions is
for "these are never going to overlap" -- devices or ram at
fixed addresses in the address space that can't
ever be mapped over by anything else. If you want
"can overlap but I don't care which one wins" then
that would be more clearly expressed by using the _overlap()
version but just giving everything that can overlap there
the same priority.

> There are lots of regions where guest can make things overlapping
> but doesn't, e.g. PCI BARs can be programmed to overlap
> almost anything.
>
> What happens on real hardware if you then access one of
> the BARs is undefined, but programming itself is harmless.
> That's why we can't assert.

Yeah, good point, for the special case where it's the
guest that's determining the addresses where something's
mapped we might want to allow the behaviour to fall out
of the implementation. (You could instead specify set of
priorities that makes the undefined-behaviour something
specific, rather than just an emergent property of
the implementation QEMU happens to have, but it seems
a bit hard to justify.)

thanks
-- PMM

Re: [PATCH] ppc: remove excessive logging

[BALATON Zoltan]

On Sun, 15 Dec 2019, Philippe Mathieu-Daudé wrote:

Hi Joakim,

I'm cc'ing the PPC maintainers for you, so they won't miss your patch (see 
https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer and 
the output of ./scripts/get_maintainer.pl -f target/ppc/excp_helper.c).


On 12/14/19 1:13 PM, Joakim Tjernlund wrote:

From: Joakim Tjernlund 

ppc logs every type of Invalid instruction. This generates a lot
of garbage on console when sshd/ssh_keygen executes as
they try various insn to optimize its performance.
The invalid operation log is still there so an unknown insn
will still be logged.

Signed-off-by: Joakim Tjernlund 
---

As far as I can see, ppc is the only one emiting thsi
debug msg for Invalid insns.

target/ppc/excp_helper.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 50b004d00d..45c2fa3ff9 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
excp_model, int excp)

  env->spr[SPR_BOOKE_ESR] = ESR_FP;
  break;
  case POWERPC_EXCP_INVAL:
-LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n", 
env->nip);


I don't think we want to remove a such important log. Since it make sense to 
not disturb the console, maybe we can replace some of the LOG_EXCP() calls by 
qemu_log_mask(LOG_GUEST_ERROR,...) instead?


I don't think that's a good idea. That would flood the -d guest_errors log 
with unwanted messages that are normal not really due to guest errors. The 
LOG_EXCP() is not enabled by default, you have to edit source to enable it 
so you can also then edit the unwanted messages as well (like commenting 
this one out if you don't like it). If this is removed, invalid opcodes 
are still logged from translate.c but the exception generated for them is 
not logged. I don't know if that's useful or not but these are debug logs 
so depends on what do you want to debug. I don't mind this being removed 
but would be also happy leaving it as it is as it's only shown for 
developers who enable it and might help debugging. Or maybe these could be 
converted to traces (although I generally find traces to be less 
convenient to work with than debug logs and not sure about potential 
performance impact). So my preferences would be in order: 1. leave it 
alone, 2. remove it, 3. convert to traces.


Regards,
BALATON Zoltan




  msr |= 0x0008;
  env->spr[SPR_BOOKE_ESR] = ESR_PIL;
  break;

Re: [PATCH 12/12] hw/i386/pc: Move PC-machine specific declarations to 'pc_internal.h'

[Michael S. Tsirkin]

On Fri, Dec 13, 2019 at 05:47:28PM +0100, Philippe Mathieu-Daudé wrote:
> On 12/13/19 5:17 PM, Philippe Mathieu-Daudé wrote:
> > Historically, QEMU started with only one X86 machine: the PC.
> > The 'hw/i386/pc.h' header was used to store all X86 and PC
> > declarations. Since we have now multiple machines based on the
> > X86 architecture, move the PC-specific declarations in a new
> > header.
> > We use 'internal' in the name to explicit this header is restricted
> > to the X86 architecture. Other architecture can not access it.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> > Maybe name it 'pc_machine.h'?
> 
> I forgot to describe here (and in the cover), what's follow after this
> patch.
> 
> Patch #13 moves PCMachineClass to
> 
> If you ignore PCMachineState, "hw/i386/pc.h" now only contains 76 lines, and
> it is easier to see what is PC machine specific, what is X86 specific, and
> what is device generic (not X86 related at all):
> 
> - GSI is common to X86 (Paolo sent [3], [6])
> - IOAPIC is common to X86
> - i8259 is multiarch (Paolo [2])
> - PCI_HOST definitions and pc_pci_hole64_start() are X86
> - pc_machine_is_smm_enabled() is X86 (Paolo sent [5])
> - hpet
> - tsc (Paolo sent [3])
> - 3 more functions
> 
> So we can move half of this file to "pc_internal.h" and the other to
> 
> One problem is the Q35 MCH north bridge which directly sets the PCI
> PCMachineState->bus in q35_host_realize(). This seems a QOM violation and is
> probably easily fixable.
> 
> Maybe I can apply Paolo's patches instead of this #12, move X86-generic
> declarations to "hw/i386/x86.h", and directly git-move what's left of
> "hw/i386/pc.h" to "pc_internal.h".

Yea that sounds a bit better.

> [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg664627.html
> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg664765.html
> [5] https://www.mail-archive.com/qemu-devel@nongnu.org/msg664754.html
> [6] https://www.mail-archive.com/qemu-devel@nongnu.org/msg664766.html
> 
> > ---
> >   hw/i386/pc_internal.h | 144 ++
> >   include/hw/i386/pc.h  | 128 -
> >   hw/i386/acpi-build.c  |   1 +
> >   hw/i386/pc.c  |   1 +
> >   hw/i386/pc_piix.c |   1 +
> >   hw/i386/pc_q35.c  |   1 +
> >   hw/i386/pc_sysfw.c|   1 +
> >   hw/i386/xen/xen-hvm.c |   1 +
> >   8 files changed, 150 insertions(+), 128 deletions(-)
> >   create mode 100644 hw/i386/pc_internal.h

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Michael S. Tsirkin]

On Sat, Dec 14, 2019 at 08:01:46PM +, Peter Maydell wrote:
> On Sat, 14 Dec 2019 at 18:17, Philippe Mathieu-Daudé  
> wrote:
> > Maybe we can a warning if priority=0, to force board designers to use
> > explicit priority (explicit overlap).
> 
> Priority 0 is fine, it's just one of the possible positive and
> negative values. I think what ideally we would complain about
> is where we see an overlap and both the regions involved
> have the same priority value, because in that case which
> one the guest sees is implicitly dependent on (I think) which
> order the subregions were added, which is fragile if we move
> code around. I'm not sure how easy that is to test for or how
> much of our existing code violates it, though.
> 
> thanks
> -- PMM

Problem is it's not uncommon for guests to create such
configs, and then just never access them.
So the thing to do would be to complain *on access*.

-- 
MST

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Michael S. Tsirkin]

On Sat, Dec 14, 2019 at 04:28:08PM +, Peter Maydell wrote:
> (It doesn't actually assert that it doesn't
> overlap because we have some legacy uses, notably
> in the x86 PC machines, which do overlap without using
> the right function, which we've never tried to tidy up.)

It's not exactly legacy uses.

To be more exact, the way the non overlap versions
are *used* is to mean "I don't care what happens when they overlap"
as opposed to "will never overlap".

There are lots of regions where guest can make things overlapping
but doesn't, e.g. PCI BARs can be programmed to overlap
almost anything.

What happens on real hardware if you then access one of
the BARs is undefined, but programming itself is harmless.
That's why we can't assert.

-- 
MST