[PULL 5/5] qapi: More complex uses of QAPI_LIST_APPEND
From: Eric Blake
These cases require a bit more thought to review; in each case, the
code was appending to a list, but not with a FOOList **tail variable.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20210113221013.390592-6-ebl...@redhat.com>
Reviewed-by: Markus Armbruster
[Flawed change to qmp_guest_network_get_interfaces() dropped]
Signed-off-by: Markus Armbruster
---
block/gluster.c| 13 ++
block/qapi.c | 14 +-
dump/dump.c| 22 +++--
hw/core/machine-qmp-cmds.c | 93 --
hw/mem/memory-device.c | 12 +
hw/pci/pci.c | 60
migration/migration.c | 20 +++-
monitor/hmp-cmds.c | 25 --
net/net.c | 13 +-
qga/commands-posix.c | 19 +++-
qga/commands-win32.c | 88
softmmu/tpm.c | 38 +++-
ui/spice-core.c| 27 ---
13 files changed, 141 insertions(+), 303 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 1f8699b938..e8ee14c8e9 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -514,7 +514,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
{
QemuOpts *opts;
SocketAddress *gsconf = NULL;
-SocketAddressList *curr = NULL;
+SocketAddressList **tail;
QDict *backing_options = NULL;
Error *local_err = NULL;
char *str = NULL;
@@ -547,6 +547,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
}
gconf->path = g_strdup(ptr);
qemu_opts_del(opts);
+tail = >server;
for (i = 0; i < num_servers; i++) {
str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
@@ -655,15 +656,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
qemu_opts_del(opts);
}
-if (gconf->server == NULL) {
-gconf->server = g_new0(SocketAddressList, 1);
-gconf->server->value = gsconf;
-curr = gconf->server;
-} else {
-curr->next = g_new0(SocketAddressList, 1);
-curr->next->value = gsconf;
-curr = curr->next;
-}
+QAPI_LIST_APPEND(tail, gsconf);
gsconf = NULL;
qobject_unref(backing_options);
diff --git a/block/qapi.c b/block/qapi.c
index 3a1186fdcc..0a96099e36 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -198,7 +198,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
{
int i, sn_count;
QEMUSnapshotInfo *sn_tab = NULL;
-SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
+SnapshotInfoList *head = NULL, **tail =
SnapshotInfo *info;
sn_count = bdrv_snapshot_list(bs, _tab);
@@ -233,17 +233,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
info->icount= sn_tab[i].icount;
info->has_icount= sn_tab[i].icount != -1ULL;
-info_list = g_new0(SnapshotInfoList, 1);
-info_list->value = info;
-
-/* XXX: waiting for the qapi to support qemu-queue.h types */
-if (!cur_item) {
-head = cur_item = info_list;
-} else {
-cur_item->next = info_list;
-cur_item = info_list;
-}
-
+QAPI_LIST_APPEND(tail, info);
}
g_free(sn_tab);
diff --git a/dump/dump.c b/dump/dump.c
index dec32468d9..929138e91d 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2030,39 +2030,29 @@ void qmp_dump_guest_memory(bool paging, const char
*file,
DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
{
-DumpGuestMemoryFormatList *item;
DumpGuestMemoryCapability *cap =
g_malloc0(sizeof(DumpGuestMemoryCapability));
+DumpGuestMemoryFormatList **tail = >formats;
/* elf is always available */
-item = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-cap->formats = item;
-item->value = DUMP_GUEST_MEMORY_FORMAT_ELF;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
/* kdump-zlib is always available */
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB);
/* add new item if kdump-lzo is available */
#ifdef CONFIG_LZO
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO);
#endif
/* add new item if kdump-snappy is available */
#ifdef CONFIG_SNAPPY
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY);
#endif
/* Windows dump is
Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()
On 1/28/21 1:46 AM, Joel Stanley wrote: > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater wrote: >> >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. >> >> Signed-off-by: Cédric Le Goater >> --- >> hw/ppc/pnv_bmc.c | 7 +-- >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c >> index 67ebb16c4d5f..86d16b493539 100644 >> --- a/hw/ppc/pnv_bmc.c >> +++ b/hw/ppc/pnv_bmc.c >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) >> Object *obj; >> >> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); >> -object_ref(OBJECT(pnor)); >> -object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > > I assume it's ok to move the link set to after the realise of the BMC object? When 2 objects need to be linked, one has to be realized first. I suppose this is why it is allowed but I am not expert in that area. Greg ? That was the case already when defining a "ipmi-bmc-sim" device on the command line. C. >> qdev_realize(DEVICE(obj), NULL, _fatal); >> - >> -/* Install the HIOMAP protocol handlers to access the PNOR */ >> -ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, >> -_netfn); >> +pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); >> >> return IPMI_BMC(obj); >> } >> -- >> 2.26.2 >> >>
[PULL 4/5] qapi: Use QAPI_LIST_APPEND in trivial cases
From: Eric Blake
The easiest spots to use QAPI_LIST_APPEND are where we already have an
obvious pointer to the tail of a list. While at it, consistently use
the variable name 'tail' for that purpose.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Markus Armbruster
Message-Id: <20210113221013.390592-5-ebl...@redhat.com>
Signed-off-by: Markus Armbruster
---
backends/hostmem.c | 10 ++--
block/dirty-bitmap.c| 8 ++-
block/export/export.c | 7 +--
block/qapi.c| 23 ++--
block/qcow2-bitmap.c| 15 ++
block/vmdk.c| 9 ++--
blockdev.c | 13 ++---
crypto/block-luks.c | 9 ++--
hw/acpi/cpu.c | 7 +--
hw/acpi/memory_hotplug.c| 8 ++-
iothread.c | 12 ++---
job-qmp.c | 13 ++---
monitor/hmp-cmds.c | 10 ++--
qemu-img.c | 8 +--
qga/commands-posix.c| 31 ---
qga/commands-win32.c| 11 ++--
scsi/pr-manager.c | 10 +---
target/i386/cpu.c | 24 +++--
tests/test-qobject-output-visitor.c | 84 +
tests/test-string-output-visitor.c | 6 +--
20 files changed, 96 insertions(+), 222 deletions(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 9f9ac95edd..be0c3b079f 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -80,7 +80,7 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v,
const char *name,
{
HostMemoryBackend *backend = MEMORY_BACKEND(obj);
uint16List *host_nodes = NULL;
-uint16List **node = _nodes;
+uint16List **tail = _nodes;
unsigned long value;
value = find_first_bit(backend->host_nodes, MAX_NODES);
@@ -88,9 +88,7 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v,
const char *name,
goto ret;
}
-*node = g_malloc0(sizeof(**node));
-(*node)->value = value;
-node = &(*node)->next;
+QAPI_LIST_APPEND(tail, value);
do {
value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
@@ -98,9 +96,7 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v,
const char *name,
break;
}
-*node = g_malloc0(sizeof(**node));
-(*node)->value = value;
-node = &(*node)->next;
+QAPI_LIST_APPEND(tail, value);
} while (true);
ret:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c01319b188..9b9cd71238 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -572,12 +572,12 @@ BlockDirtyInfoList
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
{
BdrvDirtyBitmap *bm;
BlockDirtyInfoList *list = NULL;
-BlockDirtyInfoList **plist =
+BlockDirtyInfoList **tail =
bdrv_dirty_bitmaps_lock(bs);
QLIST_FOREACH(bm, >dirty_bitmaps, list) {
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
-BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
+
info->count = bdrv_get_dirty_count(bm);
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
@@ -588,9 +588,7 @@ BlockDirtyInfoList
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->persistent = bm->persistent;
info->has_inconsistent = bm->inconsistent;
info->inconsistent = bm->inconsistent;
-entry->value = info;
-*plist = entry;
-plist = >next;
+QAPI_LIST_APPEND(tail, info);
}
bdrv_dirty_bitmaps_unlock(bs);
diff --git a/block/export/export.c b/block/export/export.c
index b716c1522c..fec7d9f738 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -342,11 +342,10 @@ void qmp_block_export_del(const char *id,
BlockExportInfoList *qmp_query_block_exports(Error **errp)
{
-BlockExportInfoList *head = NULL, **p_next =
+BlockExportInfoList *head = NULL, **tail =
BlockExport *exp;
QLIST_FOREACH(exp, _exports, next) {
-BlockExportInfoList *entry = g_new0(BlockExportInfoList, 1);
BlockExportInfo *info = g_new(BlockExportInfo, 1);
*info = (BlockExportInfo) {
.id = g_strdup(exp->id),
@@ -355,9 +354,7 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp)
.shutting_down = !exp->user_owned,
};
-entry->value = info;
-*p_next = entry;
-p_next = >next;
+QAPI_LIST_APPEND(tail, info);
}
return head;
diff --git a/block/qapi.c b/block/qapi.c
index 0ca206f559..3a1186fdcc 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -418,17 +418,12 @@ static uint64List *uint64_list(uint64_t *list, int size)
{
int i;
uint64List *out_list = NULL;
-uint64List **pout_list = _list;
+uint64List **tail = _list;
for (i = 0; i < size;
Re: [PATCH v4 5/5] qapi: More complex uses of QAPI_LIST_APPEND
Eric Blake writes: > On 1/26/21 3:31 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> These cases require a bit more thought to review; in each case, the >>> code was appending to a list, but not with a FOOList **tail variable. >>> >>> Signed-off-by: Eric Blake >>> Reviewed-by: Vladimir Sementsov-Ogievskiy >>> >>> --- >>> fix qmp_guest_network_get_interfaces [Vladimir] >> >> Fails tests/test-qga. I should've double-checked earlier. > > I ran 'make check', but I'll check again. In the meantime, > > >> >> Dropping this part unbreaks the test. >> >> I'd like to drop just this part, and merge the rest. You can then >> respin just this part as a follow-up patch. Okay? > > this plan is okay with me. Done. Thanks!
[PULL 2/5] qapi: A couple more QAPI_LIST_PREPEND() stragglers
From: Eric Blake
Commit 54aa3de72e switched multiple sites to use QAPI_LIST_PREPEND
instead of open-coding, but missed a couple of spots.
Signed-off-by: Eric Blake
Message-Id: <20210113221013.390592-3-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Markus Armbruster
---
hw/core/machine-qmp-cmds.c | 32 +++-
monitor/qmp-cmds-control.c | 9 -
2 files changed, 15 insertions(+), 26 deletions(-)
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index ae0c4a..156223a344 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -293,41 +293,31 @@ void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
static int query_memdev(Object *obj, void *opaque)
{
MemdevList **list = opaque;
-MemdevList *m = NULL;
+Memdev *m;
QObject *host_nodes;
Visitor *v;
if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
m = g_malloc0(sizeof(*m));
-m->value = g_malloc0(sizeof(*m->value));
+m->id = g_strdup(object_get_canonical_path_component(obj));
+m->has_id = !!m->id;
-m->value->id = g_strdup(object_get_canonical_path_component(obj));
-m->value->has_id = !!m->value->id;
-
-m->value->size = object_property_get_uint(obj, "size",
- _abort);
-m->value->merge = object_property_get_bool(obj, "merge",
- _abort);
-m->value->dump = object_property_get_bool(obj, "dump",
- _abort);
-m->value->prealloc = object_property_get_bool(obj,
- "prealloc",
- _abort);
-m->value->policy = object_property_get_enum(obj,
-"policy",
-"HostMemPolicy",
-_abort);
+m->size = object_property_get_uint(obj, "size", _abort);
+m->merge = object_property_get_bool(obj, "merge", _abort);
+m->dump = object_property_get_bool(obj, "dump", _abort);
+m->prealloc = object_property_get_bool(obj, "prealloc", _abort);
+m->policy = object_property_get_enum(obj, "policy", "HostMemPolicy",
+ _abort);
host_nodes = object_property_get_qobject(obj,
"host-nodes",
_abort);
v = qobject_input_visitor_new(host_nodes);
-visit_type_uint16List(v, NULL, >value->host_nodes, _abort);
+visit_type_uint16List(v, NULL, >host_nodes, _abort);
visit_free(v);
qobject_unref(host_nodes);
-m->next = *list;
-*list = m;
+QAPI_LIST_PREPEND(*list, m);
}
return 0;
diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
index 17514f4959..509ae870bd 100644
--- a/monitor/qmp-cmds-control.c
+++ b/monitor/qmp-cmds-control.c
@@ -104,17 +104,16 @@ VersionInfo *qmp_query_version(Error **errp)
static void query_commands_cb(const QmpCommand *cmd, void *opaque)
{
-CommandInfoList *info, **list = opaque;
+CommandInfo *info;
+CommandInfoList **list = opaque;
if (!cmd->enabled) {
return;
}
info = g_malloc0(sizeof(*info));
-info->value = g_malloc0(sizeof(*info->value));
-info->value->name = g_strdup(cmd->name);
-info->next = *list;
-*list = info;
+info->name = g_strdup(cmd->name);
+QAPI_LIST_PREPEND(*list, info);
}
CommandInfoList *qmp_query_commands(Error **errp)
--
2.26.2
[PULL 0/5] QAPI patches patches for 2021-01-28
The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612: Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +) are available in the Git repository at: git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2021-01-28 for you to fetch changes up to 95b3a8c8a82a34ca874ac0d4a9bbbdb38034acf3: qapi: More complex uses of QAPI_LIST_APPEND (2021-01-28 08:08:45 +0100) QAPI patches patches for 2021-01-28 Eric Blake (5): net: Clarify early exit condition qapi: A couple more QAPI_LIST_PREPEND() stragglers qapi: Introduce QAPI_LIST_APPEND qapi: Use QAPI_LIST_APPEND in trivial cases qapi: More complex uses of QAPI_LIST_APPEND include/qapi/util.h | 13 backends/hostmem.c | 10 +-- block/dirty-bitmap.c| 8 +-- block/export/export.c | 7 +- block/gluster.c | 13 +--- block/qapi.c| 37 ++- block/qcow2-bitmap.c| 15 ++--- block/vmdk.c| 9 +-- blockdev.c | 13 ++-- crypto/block-luks.c | 9 +-- dump/dump.c | 22 ++- hw/acpi/cpu.c | 7 +- hw/acpi/memory_hotplug.c| 8 +-- hw/core/machine-qmp-cmds.c | 125 +++- hw/mem/memory-device.c | 12 +--- hw/pci/pci.c| 60 ++--- iothread.c | 12 +--- job-qmp.c | 13 ++-- migration/migration.c | 20 ++ monitor/hmp-cmds.c | 35 +- monitor/qmp-cmds-control.c | 9 ++- net/net.c | 15 ++--- qemu-img.c | 8 +-- qga/commands-posix.c| 50 +-- qga/commands-win32.c| 99 ++-- scsi/pr-manager.c | 10 +-- softmmu/tpm.c | 38 ++- target/i386/cpu.c | 24 +++ tests/test-qobject-output-visitor.c | 84 +++- tests/test-string-output-visitor.c | 6 +- ui/spice-core.c | 27 +++- 31 files changed, 267 insertions(+), 551 deletions(-) -- 2.26.2
[PULL 1/5] net: Clarify early exit condition
From: Eric Blake
On first glance, the loop in qmp_query_rx_filter() has early return
paths that could leak any allocation of filter_list from a previous
iteration. But on closer inspection, it is obvious that all of the
early exits are guarded by has_name, and that the bulk of the loop
body can be executed at most once if the user is filtering by name,
thus, any early exit coincides with an empty list. Add asserts to
make this obvious.
Signed-off-by: Eric Blake
Reviewed-by: Markus Armbruster
Message-Id: <20210113221013.390592-2-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Markus Armbruster
---
net/net.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/net.c b/net/net.c
index c1cd9c75f6..2afac24b79 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1227,6 +1227,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name,
const char *name,
if (nc->info->type != NET_CLIENT_DRIVER_NIC) {
if (has_name) {
error_setg(errp, "net client(%s) isn't a NIC", name);
+assert(!filter_list);
return NULL;
}
continue;
@@ -1252,6 +1253,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name,
const char *name,
} else if (has_name) {
error_setg(errp, "net client(%s) doesn't support"
" rx-filter querying", name);
+assert(!filter_list);
return NULL;
}
--
2.26.2
[PULL 3/5] qapi: Introduce QAPI_LIST_APPEND
From: Eric Blake
Similar to the existing QAPI_LIST_PREPEND, but designed for use where
we want to preserve insertion order. Callers will be added in
upcoming patches. Note the difference in signature: PREPEND takes
List*, APPEND takes List**.
Signed-off-by: Eric Blake
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20210113221013.390592-4-ebl...@redhat.com>
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
include/qapi/util.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/qapi/util.h b/include/qapi/util.h
index 6178e98e97..d7bfb30e25 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -37,4 +37,17 @@ int parse_qapi_name(const char *name, bool complete);
(list) = _tmp; \
} while (0)
+/*
+ * For any pointer to a GenericList @tail (usually the 'next' member of a
+ * list element), insert @element at the back and update the tail.
+ *
+ * Note that this macro evaluates @element exactly once, so it is safe
+ * to have side-effects with that argument.
+ */
+#define QAPI_LIST_APPEND(tail, element) do { \
+*(tail) = g_malloc0(sizeof(**(tail))); \
+(*(tail))->value = (element); \
+(tail) = &(*(tail))->next; \
+} while (0)
+
#endif
--
2.26.2
Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
Kevin Wolf writes:
> Am 27.01.2021 um 14:56 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Instead of counting how many elements from the top of the stack we need
>> > to ignore until we find the thing we're interested in, we can just
>> > directly pass the StackObject pointer because all callers already know
>> > it.
>> >
>> > We only need a different way now to tell if we want to know the name of
>> > something contained in the given StackObject or of the StackObject
>> > itself. Passing name = NULL is the obvious way to request the latter.
>> >
>> > This simplifies the interface and makes it easier to use in cases where
>> > we have the StackObject, but don't know how many steps down the stack it
>> > is.
>> >
>> > Signed-off-by: Kevin Wolf
>> > ---
>> > qapi/qobject-input-visitor.c | 38 ++--
>> > 1 file changed, 19 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> > index a00ac32682..1415561828 100644
>> > --- a/qapi/qobject-input-visitor.c
>> > +++ b/qapi/qobject-input-visitor.c
>> > @@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>> > }
>> >
>> > /*
>> > - * Find the full name of something @qiv is currently visiting.
>> > - * @qiv is visiting something named @name in the stack of containers
>> > - * @qiv->stack.
>> > - * If @n is zero, return its full name.
>> > - * If @n is positive, return the full name of the @n-th container
>> > - * counting from the top. The stack of containers must have at least
>> > - * @n elements.
>> > - * The returned string is valid until the next full_name_nth(@v) or
>> > - * destruction of @v.
>> > + * Find the full name of something named @name in @so which @qiv is
>> > + * currently visiting. If @name is NULL, find the full name of @so
>> > + * itself.
>> > + *
>> > + * The returned string is valid until the next full_name_so(@qiv) or
>> > + * destruction of @qiv.
>>
>> How can this distinguish between a list and its member?
>>
>> Before the patch:
>>
>> * list member: n = 0, name = NULL
>> * list: n = 1, name = NULL
>
> Oh. These two lines were more helpful than the whole function comment
> before this patch (which doesn't talk about name = NULL at all).
See, I can write impenetrable comments with the best of them!
The spot that talks about @name is in visitor.h:
* The @name parameter of visit_type_FOO() describes the relation
* between this QAPI value and its parent container. When visiting
* the root of a tree, @name is ignored; when visiting a member of an
* object, @name is the key associated with the value; when visiting a
* member of a list, @name is NULL; and when visiting the member of an
* alternate, @name should equal the name used for visiting the
* alternate.
Many contracts in the same file refer back to it like this:
* @name expresses the relationship of this object to its parent
* container; see the general description of @name above.
The contract here doesn't.
>> Afterwards?
>>
>> Checking... yes, regression. Test case:
>>
>> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name":
>> "blk0", "filename": "tmp.img"}}
>> {"return": {}}
>> {"execute": "blockdev-add", "arguments": {"driver": "blkdebug",
>> "node-name": "blk1", "image": "blk0", "take-child-perms": [0]}}
>> {"error": {"class": "GenericError", "desc": "Invalid parameter type for
>> 'take-child-perms', expected: string"}}
>>
>> The second command's reply changes from
>>
>> {"error": {"class": "GenericError", "desc": "Invalid parameter type for
>> 'take-child-perms[0]', expected: string"}}
>>
>> to
>>
>> {"error": {"class": "GenericError", "desc": "Invalid parameter type for
>> 'take-child-perms', expected: string"}}
>>
>> The idea of using @so instead of @n may be salvagable.
>
> I can always add a bool parameter that tells (independently from @name)
> whether we want the name of a member or of the container.
>
> Though do we really need the name of the container anywhere? The n = 1
> case exists in qobject_input_check_list(), but is this a case that can
> fail? The pattern how lists are intended to be visited seems to be
> calling visit_next_list() until it returns NULL.
Yes, the generated visitors always exhaust the list. But virtual walks
needn't. There's one in hw/ppc/spapr_drc.c:
case FDT_PROP: {
int i;
prop = fdt_get_property_by_offset(fdt, fdt_offset, _len);
name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
if (!visit_start_list(v, name, NULL, 0, errp)) {
return;
}
for (i = 0; i < prop_len; i++) {
if (!visit_type_uint8(v, NULL, (uint8_t *)>data[i],
errp)) {
return;
}
}
ok = visit_check_list(v, errp);
visit_end_list(v, NULL);
if
Re: [PATCH v8 00/10] hw/ssi: imx_spi: Fix various bugs in the imx_spi model
On Fri, Jan 22, 2021 at 9:36 PM Bin Meng wrote: > > On Tue, Jan 19, 2021 at 9:40 PM Bin Meng wrote: > > > > From: Bin Meng > > > > This v8 series is based on the following 2 versions: > > > > - v5 series sent from Bin > > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223919 > > - v7 series sent from Philippe > > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=224612 > > > > This series fixes a bunch of bugs in current implementation of the imx > > spi controller, including the following issues: > > > > - remove imx_spi_update_irq() in imx_spi_reset() > > - chip select signal was not lower down when spi controller is disabled > > - round up the tx burst length to be multiple of 8 > > - transfer incorrect data when the burst length is larger than 32 bit > > - spi controller tx and rx fifo endianness is incorrect > > - remove pointless variable (s->burst_length) initialization (Philippe) > > - rework imx_spi_reset() to keep CONREG register value (Philippe) > > - rework imx_spi_read() to handle block disabled (Philippe) > > - rework imx_spi_write() to handle block disabled (Philippe) > > > > Tested with upstream U-Boot v2020.10 (polling mode) and VxWorks 7 > > (interrupt mode). > > > > Changes in v8: > > - keep the controller disable logic in the ECSPI_CONREG case > > in imx_spi_write() > > Ping? Could we get this applied soon if no more comments? Regards, Bin
Re: [PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external
On 1/28/21 1:48 AM, Joel Stanley wrote:
> On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater wrote:
>>
>> The PowerNV machine can be run with an external IPMI BMC device
>> connected to a remote QEMU machine acting as BMC, using these options :
>>
>> -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
>> -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
>> -device isa-ipmi-bt,bmc=bmc0,irq=10 \
>> -nodefaults
>
> Should this information also go in docs/system/ppc similar to the
> descriptions we have in docs/system/arm?
yes.
Do you think we could reference :
https://openpower.xyz/job/openpower/job/openpower-op-build/
?
Thanks,
C.
>>
>> In that case, some aspects of the BMC initialization should be
>> skipped, since they rely on the simulator interface.
>>
>> Signed-off-by: Cédric Le Goater
>
> Reviewed-by: Joel Stanley
>
>
>> ---
>> hw/ppc/pnv_bmc.c | 15 ++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
>> index 86d16b493539..b9bf5735ea0f 100644
>> --- a/hw/ppc/pnv_bmc.c
>> +++ b/hw/ppc/pnv_bmc.c
>> @@ -51,6 +51,11 @@ typedef struct OemSel {
>> #define SOFT_OFF0x00
>> #define SOFT_REBOOT 0x01
>>
>> +static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
>> +{
>> +return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
>> +}
>> +
>> static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
>> {
>> /* IPMI SEL Event are 16 bytes long */
>> @@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
>> const struct ipmi_sdr_compact *sdr;
>> uint16_t nextrec;
>>
>> +if (!pnv_bmc_is_simulator(bmc)) {
>> +return;
>> +}
>> +
>> offset = fdt_add_subnode(fdt, 0, "bmc");
>> _FDT(offset);
>>
>> @@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
>>
>> void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
>> {
>> +if (!pnv_bmc_is_simulator(bmc)) {
>> +return;
>> +}
>> +
>> object_ref(OBJECT(pnor));
>> object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
>>
>> @@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
>>
>> IPMIBmc *pnv_bmc_find(Error **errp)
>> {
>> -ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
>> +ForeachArgs args = { TYPE_IPMI_BMC, NULL };
>> int ret;
>>
>> ret = object_child_foreach_recursive(object_get_root(), bmc_find,
>> );
>> --
>> 2.26.2
>>
>>
Re: [PATCH 2/3] hw/sd: sd: Move the sd_block_{read, write} and macros ahead
Hello Bin,
On 1/28/21 7:43 AM, Bin Meng wrote:
> From: Bin Meng
>
> These APIs and macros may be referenced by functions that are
> currently before them. Move them ahead a little bit.
We could also change fprintf() by qemu_log_mask()
Thanks,
C.
> Signed-off-by: Bin Meng
> ---
>
> hw/sd/sd.c | 42 +-
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a6a0b3dcc6..1886d4b30b 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -739,6 +739,27 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq
> insert)
> qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
> }
>
> +static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
> +{
> +trace_sdcard_read_block(addr, len);
> +if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
> +fprintf(stderr, "sd_blk_read: read error on host side\n");
> +}
> +}
> +
> +static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
> +{
> +trace_sdcard_write_block(addr, len);
> +if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
> +fprintf(stderr, "sd_blk_write: write error on host side\n");
> +}
> +}
> +
> +#define BLK_READ_BLOCK(a, len) sd_blk_read(sd, a, len)
> +#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
> +#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
> +#define APP_WRITE_BLOCK(a, len)
> +
> static void sd_erase(SDState *sd)
> {
> int i;
> @@ -1742,27 +1763,6 @@ send_response:
> return rsplen;
> }
>
> -static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
> -{
> -trace_sdcard_read_block(addr, len);
> -if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
> -fprintf(stderr, "sd_blk_read: read error on host side\n");
> -}
> -}
> -
> -static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
> -{
> -trace_sdcard_write_block(addr, len);
> -if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
> -fprintf(stderr, "sd_blk_write: write error on host side\n");
> -}
> -}
> -
> -#define BLK_READ_BLOCK(a, len) sd_blk_read(sd, a, len)
> -#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
> -#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
> -#define APP_WRITE_BLOCK(a, len)
> -
> void sd_write_byte(SDState *sd, uint8_t value)
> {
> int i;
>
Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs
On 1/28/21 1:45 AM, Joel Stanley wrote:
> On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater wrote:
>>
>> The current settings are useful to load large kernels (with debug) but
>> it moves the initrd image in a memory region not protected by
>> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
>> corrupt the initrd.
>>
>> Cc: Murilo Opsfelder Araujo
>> Signed-off-by: Cédric Le Goater
>
> Reviewed-by: Joel Stanley
>
> Note that the machine's default ram size will change with this patch:
>
> mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
Ah yes. I missed that.
> So we will go from 1.75GB to 768MB. Does anything break when the
> machine has less than 1GB of ram?
There is a warning if the machine has less than 1GB but we should
also change the default RAM size to 1G to be on the safe side.
Thanks,
C.
>
>> ---
>>
>> If we want to increase the kernel size limit as commit b45b56baeecd
>> ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
>> think we should add a machine option.
>>
>> hw/ppc/pnv.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 14fc9758a973..e500c2e2437e 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -65,9 +65,9 @@
>> #define FW_MAX_SIZE (16 * MiB)
>>
>> #define KERNEL_LOAD_ADDR0x2000
>> -#define KERNEL_MAX_SIZE (256 * MiB)
>> -#define INITRD_LOAD_ADDR0x6000
>> -#define INITRD_MAX_SIZE (256 * MiB)
>> +#define KERNEL_MAX_SIZE (128 * MiB)
>> +#define INITRD_LOAD_ADDR0x2800
>> +#define INITRD_MAX_SIZE (128 * MiB)
>>
>> static const char *pnv_chip_core_typename(const PnvChip *o)
>> {
>> --
>> 2.26.2
>>
>>
[PATCH 3/3] hw/sd: sd: Actually perform the erase operation
From: Bin Meng
At present the sd_erase() does not erase the requested range of card
data to 0xFFs. Let's make the erase operation actually happen.
Signed-off-by: Bin Meng
---
hw/sd/sd.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1886d4b30b..8c397d4ad7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -765,6 +765,8 @@ static void sd_erase(SDState *sd)
int i;
uint64_t erase_start = sd->erase_start;
uint64_t erase_end = sd->erase_end;
+uint64_t erase_addr;
+int erase_len = 1 << HWBLOCK_SHIFT;
trace_sdcard_erase(sd->erase_start, sd->erase_end);
if (sd->erase_start == INVALID_ADDRESS
@@ -788,6 +790,13 @@ static void sd_erase(SDState *sd)
return;
}
+memset(sd->data, 0xff, erase_len);
+erase_addr = erase_start;
+for (i = 0; i < (erase_end - erase_start) / erase_len + 1; i++) {
+BLK_WRITE_BLOCK(erase_addr, erase_len);
+erase_addr += erase_len;
+}
+
erase_start = sd_addr_to_wpnum(erase_start);
erase_end = sd_addr_to_wpnum(erase_end);
sd->erase_start = INVALID_ADDRESS;
--
2.25.1
qemu user mode fails to run programs with large VM / built with address sanitizer (was: Re: [PATCH v4 4/4] meson: Warn when TCI is selected but TCG backend is available)
Am 27.01.21 um 22:47 schrieb Alex Bennée: Stefan Weil writes: I recently tried running tesseract with qemu-x86_64 because I had expected that it might trigger some unimplemented TCG opcodes. qemu-x86-64 is a poor choice as a relatively under maintained front-end doesn't emulate a particularly new CPU or take advantage of the new TCG features. ARM64 is pretty good because the default cpu for linux-user is CPU max which a) enables all ISA features we have and b) exposes them fairly easily to guest detection routines which probe feature registers. Instead it showed a general problem for native TCG: qemu-x86_64 allocates too much memory for tesseract and gets killed by the Linux kernel OOM handler. Do you have a command line? That sounds like something that should be fixed. The problem occurred with a locally built tesseract, but I now found that it is more general. Any program which was compiled with address sanitizer uses huge virtual memory (TB) right at the start. QEMU user mode tries to allocate that memory until it is killed by the Linux kernel OOM handler. A simple hello program compiled with "gcc -fsanitize=address hello.c" is sufficient to show the problem. Just run it with "qemu-x86_64 a.out". I did not test but expect the same problem for other architectures, too, unless their VM is more limited. Regards, Stefan
[PATCH 1/3] hw/sd: sd: Fix address check in sd_erase()
From: Bin Meng
For high capacity memory cards, the erase start address and end
address are multiplied by 512, but the address check is still
based on the original block number in sd->erase_{start, end}.
Fixes: 1bd6fd8ed593 ("hw/sd/sdcard: Do not attempt to erase out of range
addresses")
Signed-off-by: Bin Meng
---
hw/sd/sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c99c0e93bb..a6a0b3dcc6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -760,7 +760,7 @@ static void sd_erase(SDState *sd)
erase_end *= 512;
}
-if (sd->erase_start > sd->size || sd->erase_end > sd->size) {
+if (erase_start > sd->size || erase_end > sd->size) {
sd->card_status |= OUT_OF_RANGE;
sd->erase_start = INVALID_ADDRESS;
sd->erase_end = INVALID_ADDRESS;
--
2.25.1
[PATCH 2/3] hw/sd: sd: Move the sd_block_{read, write} and macros ahead
From: Bin Meng
These APIs and macros may be referenced by functions that are
currently before them. Move them ahead a little bit.
Signed-off-by: Bin Meng
---
hw/sd/sd.c | 42 +-
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a6a0b3dcc6..1886d4b30b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -739,6 +739,27 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq
insert)
qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
}
+static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+{
+trace_sdcard_read_block(addr, len);
+if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
+fprintf(stderr, "sd_blk_read: read error on host side\n");
+}
+}
+
+static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+{
+trace_sdcard_write_block(addr, len);
+if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
+fprintf(stderr, "sd_blk_write: write error on host side\n");
+}
+}
+
+#define BLK_READ_BLOCK(a, len) sd_blk_read(sd, a, len)
+#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
+#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
+#define APP_WRITE_BLOCK(a, len)
+
static void sd_erase(SDState *sd)
{
int i;
@@ -1742,27 +1763,6 @@ send_response:
return rsplen;
}
-static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
-{
-trace_sdcard_read_block(addr, len);
-if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
-fprintf(stderr, "sd_blk_read: read error on host side\n");
-}
-}
-
-static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
-{
-trace_sdcard_write_block(addr, len);
-if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
-fprintf(stderr, "sd_blk_write: write error on host side\n");
-}
-}
-
-#define BLK_READ_BLOCK(a, len) sd_blk_read(sd, a, len)
-#define BLK_WRITE_BLOCK(a, len)sd_blk_write(sd, a, len)
-#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
-#define APP_WRITE_BLOCK(a, len)
-
void sd_write_byte(SDState *sd, uint8_t value)
{
int i;
--
2.25.1
[PATCH 0/3] hw/sd: sd: erase operation fixes
From: Bin Meng
This includes several fixes related to erase operation of a SD card.
Based-on:
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226785
Bin Meng (3):
hw/sd: sd: Fix address check in sd_erase()
hw/sd: sd: Move the sd_block_{read,write} and macros ahead
hw/sd: sd: Actually perform the erase operation
hw/sd/sd.c | 53 +++--
1 file changed, 31 insertions(+), 22 deletions(-)
--
2.25.1
[PATCH v4 7/9] hw/sd: ssi-sd: Fix SEND_IF_COND (CMD8) response
From: Bin Meng
The SEND_IF_COND command (CMD8) response is of format R7, but
current code returns R1 for CMD8. Fix it.
Fixes: 775616c3ae8c ("Partial SD card SPI mode support")
Signed-off-by: Bin Meng
Reviewed-by: Philippe Mathieu-Daudé
---
When testing with VxWorks driver, this additional issue was exposed.
It looks like VxWorks has stricter parsing on command responses while
U-Boot/Linux drivers are all happy with exising QEMU CMD8 response.
(no changes since v1)
hw/sd/ssi-sd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 200e885225..84c873b3fd 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -176,9 +176,9 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
s->arglen = 1;
s->response[0] = 4;
DPRINTF("SD command failed\n");
-} else if (s->cmd == 58) {
-/* CMD58 returns R3 response (OCR) */
-DPRINTF("Returned OCR\n");
+} else if (s->cmd == 8 || s->cmd == 58) {
+/* CMD8/CMD58 returns R3/R7 response */
+DPRINTF("Returned R3/R7\n");
s->arglen = 5;
s->response[0] = 1;
memcpy(>response[1], longresp, 4);
--
2.25.1
[PATCH v4 9/9] hw/sd: ssi-sd: Handle the rest commands with R1b response type
From: Bin Meng
Besides CMD12, the following command's reponse type is R1b:
- SET_WRITE_PROT (CMD28)
- CLR_WRITE_PROT (CMD29)
- ERASE (CMD38)
Reuse the same s->stopping to indicate a R1b reponse is needed.
Signed-off-by: Bin Meng
---
Changes in v4:
- new patch: handle the rest commands with R1b response type
hw/sd/ssi-sd.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 907d681d19..97ee58e20c 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
/* CMD13 returns a 2-byte statuse work. Other commands
only return the first byte. */
s->arglen = (s->cmd == 13) ? 2 : 1;
+
+/* handle R1b */
+if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {
+s->stopping = 1;
+}
+
cardstatus = ldl_be_p(longresp);
status = 0;
if (((cardstatus >> 9) & 0xf) < 4)
--
2.25.1
[PATCH v4 6/9] hw/sd: ssi-sd: Support multiple block write
From: Bin Meng
For a multiple block write operation, each block begins with a multi
write start token. Unlike the SD mode that the multiple block write
ends when receiving a STOP_TRAN command (CMD12), a special stop tran
token is used to signal the card.
Emulating this by manually sending a CMD12 to the SD card core, to
bring it out of the receiving data state.
Signed-off-by: Bin Meng
Acked-by: Alistair Francis
Tested-by: Philippe Mathieu-Daudé
---
Changes in v4:
- Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
receiving the STOP_TRAN token per the spec
hw/sd/ssi-sd.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 1205ad8b52..200e885225 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -4,6 +4,11 @@
* Copyright (c) 2007-2009 CodeSourcery.
* Written by Paul Brook
*
+ * Copyright (c) 2021 Wind River Systems, Inc.
+ * Improved by Bin Meng
+ *
+ * Validated with U-Boot v2021.01 and Linux v5.10 mmc_spi driver
+ *
* This code is licensed under the GNU GPL v2.
*
* Contributions after 2012-01-13 are licensed under the terms of the
@@ -82,6 +87,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
#define SSI_SDR_ADDRESS_ERROR 0x2000
#define SSI_SDR_PARAMETER_ERROR 0x4000
+/* multiple block write */
+#define SSI_TOKEN_MULTI_WRITE 0xfc
+/* terminate multiple block write */
+#define SSI_TOKEN_STOP_TRAN 0xfd
/* single block read/write, multiple block read */
#define SSI_TOKEN_SINGLE0xfe
@@ -94,6 +103,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
{
ssi_sd_state *s = SSI_SD(dev);
+SDRequest request;
+uint8_t longresp[16];
/*
* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.
@@ -125,8 +136,28 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
return SSI_DUMMY;
break;
case SSI_TOKEN_SINGLE:
+case SSI_TOKEN_MULTI_WRITE:
DPRINTF("Start write block\n");
s->mode = SSI_SD_DATA_WRITE;
+return SSI_DUMMY;
+case SSI_TOKEN_STOP_TRAN:
+DPRINTF("Stop multiple write\n");
+
+/* manually issue cmd12 to stop the transfer */
+request.cmd = 12;
+request.arg = 0;
+s->arglen = sdbus_do_command(>sdbus, , longresp);
+if (s->arglen <= 0) {
+s->arglen = 1;
+/* a zero value indicates the card is busy */
+s->response[0] = 0;
+DPRINTF("SD card busy\n");
+} else {
+s->arglen = 1;
+/* a non-zero value indicates the card is ready */
+s->response[0] = SSI_DUMMY;
+}
+
return SSI_DUMMY;
}
@@ -136,8 +167,6 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
return SSI_DUMMY;
case SSI_SD_CMDARG:
if (s->arglen == 4) {
-SDRequest request;
-uint8_t longresp[16];
/* FIXME: Check CRC. */
request.cmd = s->cmd;
request.arg = ldl_be_p(s->cmdarg);
--
2.25.1
[PATCH v4 8/9] hw/sd: ssi-sd: Fix STOP_TRANSMISSION (CMD12) response
From: Bin Meng
CMD12's response type is R1b, which is basically a R1 plus optional
addition of the busy signal token that can be any number of bytes.
A zero value indicates card is busy and a non-zero value indicates
the card is ready for the next command.
Current implementation sends the busy signal token without sending
the R1 first. This does not break the U-Boot/Linux mmc_spi driver,
but it does not make the VxWorks driver happy.
Move the testing logic of s->stopping in the SSI_SD_RESPONSE state
a bit later, after the first byte of the card reponse is sent out,
to conform with the spec. After the busy signal token is sent, the
state should be transferred to SSI_SD_CMD.
Fixes: 775616c3ae8c ("Partial SD card SPI mode support")
Signed-off-by: Bin Meng
---
Changes in v4:
- new patch: fix STOP_TRANSMISSION (CMD12) response
hw/sd/ssi-sd.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 84c873b3fd..907d681d19 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -243,14 +243,15 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
s->mode = SSI_SD_RESPONSE;
return SSI_DUMMY;
case SSI_SD_RESPONSE:
-if (s->stopping) {
-s->stopping = 0;
-return SSI_DUMMY;
-}
if (s->response_pos < s->arglen) {
DPRINTF("Response 0x%02x\n", s->response[s->response_pos]);
return s->response[s->response_pos++];
}
+if (s->stopping) {
+s->stopping = 0;
+s->mode = SSI_SD_CMD;
+return SSI_DUMMY;
+}
if (sdbus_data_ready(>sdbus)) {
DPRINTF("Data read\n");
s->mode = SSI_SD_DATA_START;
--
2.25.1
[PATCH v4 4/9] hw/sd: Introduce receive_ready() callback
From: Bin Meng
At present there is a data_ready() callback for the SD data read
path. Let's add a receive_ready() for the SD data write path.
Signed-off-by: Bin Meng
Reviewed-by: Philippe Mathieu-Daudé
Acked-by: Alistair Francis
Tested-by: Philippe Mathieu-Daudé
---
(no changes since v1)
include/hw/sd/sd.h | 2 ++
hw/sd/core.c | 13 +
hw/sd/sd.c | 6 ++
3 files changed, 21 insertions(+)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 05ef9b73e5..47360ba4ee 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -116,6 +116,7 @@ struct SDCardClass {
* Return: byte value read
*/
uint8_t (*read_byte)(SDState *sd);
+bool (*receive_ready)(SDState *sd);
bool (*data_ready)(SDState *sd);
void (*set_voltage)(SDState *sd, uint16_t millivolts);
uint8_t (*get_dat_lines)(SDState *sd);
@@ -187,6 +188,7 @@ void sdbus_write_data(SDBus *sdbus, const void *buf, size_t
length);
* Read multiple bytes of data on the data lines of a SD bus.
*/
void sdbus_read_data(SDBus *sdbus, void *buf, size_t length);
+bool sdbus_receive_ready(SDBus *sd);
bool sdbus_data_ready(SDBus *sd);
bool sdbus_get_inserted(SDBus *sd);
bool sdbus_get_readonly(SDBus *sd);
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 08c93b5903..30ee62c510 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -160,6 +160,19 @@ void sdbus_read_data(SDBus *sdbus, void *buf, size_t
length)
}
}
+bool sdbus_receive_ready(SDBus *sdbus)
+{
+SDState *card = get_card(sdbus);
+
+if (card) {
+SDCardClass *sc = SD_CARD_GET_CLASS(card);
+
+return sc->receive_ready(card);
+}
+
+return false;
+}
+
bool sdbus_data_ready(SDBus *sdbus)
{
SDState *card = get_card(sdbus);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 946036d87c..c99c0e93bb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2037,6 +2037,11 @@ uint8_t sd_read_byte(SDState *sd)
return ret;
}
+static bool sd_receive_ready(SDState *sd)
+{
+return sd->state == sd_receivingdata_state;
+}
+
static bool sd_data_ready(SDState *sd)
{
return sd->state == sd_sendingdata_state;
@@ -2147,6 +2152,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
sc->do_command = sd_do_command;
sc->write_byte = sd_write_byte;
sc->read_byte = sd_read_byte;
+sc->receive_ready = sd_receive_ready;
sc->data_ready = sd_data_ready;
sc->enable = sd_enable;
sc->get_inserted = sd_get_inserted;
--
2.25.1
[PATCH v4 5/9] hw/sd: ssi-sd: Support single block write
From: Bin Meng
Add 2 more states for the block write operation. The SPI host needs
to send a data start token to start the transfer, and the data block
written to the card will be acknowledged by a data response token.
Signed-off-by: Bin Meng
Acked-by: Alistair Francis
[PMD: Change VMState version id 6 -> 7]
Signed-off-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
---
(no changes since v1)
hw/sd/ssi-sd.c | 44
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 6d20a240c6..1205ad8b52 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -43,6 +43,8 @@ typedef enum {
SSI_SD_DATA_START,
SSI_SD_DATA_READ,
SSI_SD_DATA_CRC16,
+SSI_SD_DATA_WRITE,
+SSI_SD_SKIP_CRC16,
} ssi_sd_mode;
struct ssi_sd_state {
@@ -53,6 +55,7 @@ struct ssi_sd_state {
uint8_t response[5];
uint16_t crc16;
int32_t read_bytes;
+int32_t write_bytes;
int32_t arglen;
int32_t response_pos;
int32_t stopping;
@@ -85,6 +88,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
/* dummy value - don't care */
#define SSI_DUMMY 0xff
+/* data accepted */
+#define DATA_RESPONSE_ACCEPTED 0x05
+
static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
{
ssi_sd_state *s = SSI_SD(dev);
@@ -113,10 +119,17 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
switch (s->mode) {
case SSI_SD_CMD:
-if (val == SSI_DUMMY) {
+switch (val) {
+case SSI_DUMMY:
DPRINTF("NULL command\n");
return SSI_DUMMY;
+break;
+case SSI_TOKEN_SINGLE:
+DPRINTF("Start write block\n");
+s->mode = SSI_SD_DATA_WRITE;
+return SSI_DUMMY;
}
+
s->cmd = val & 0x3f;
s->mode = SSI_SD_CMDARG;
s->arglen = 0;
@@ -250,6 +263,27 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
s->response_pos = 0;
}
return val;
+case SSI_SD_DATA_WRITE:
+sdbus_write_byte(>sdbus, val);
+s->write_bytes++;
+if (!sdbus_receive_ready(>sdbus) || s->write_bytes == 512) {
+DPRINTF("Data write end\n");
+s->mode = SSI_SD_SKIP_CRC16;
+s->response_pos = 0;
+}
+return val;
+case SSI_SD_SKIP_CRC16:
+/* we don't verify the crc16 */
+s->response_pos++;
+if (s->response_pos == 2) {
+DPRINTF("CRC16 receive end\n");
+s->mode = SSI_SD_RESPONSE;
+s->write_bytes = 0;
+s->arglen = 1;
+s->response[0] = DATA_RESPONSE_ACCEPTED;
+s->response_pos = 0;
+}
+return SSI_DUMMY;
}
/* Should never happen. */
return SSI_DUMMY;
@@ -259,7 +293,7 @@ static int ssi_sd_post_load(void *opaque, int version_id)
{
ssi_sd_state *s = (ssi_sd_state *)opaque;
-if (s->mode > SSI_SD_DATA_CRC16) {
+if (s->mode > SSI_SD_SKIP_CRC16) {
return -EINVAL;
}
if (s->mode == SSI_SD_CMDARG &&
@@ -277,8 +311,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
static const VMStateDescription vmstate_ssi_sd = {
.name = "ssi_sd",
-.version_id = 6,
-.minimum_version_id = 6,
+.version_id = 7,
+.minimum_version_id = 7,
.post_load = ssi_sd_post_load,
.fields = (VMStateField []) {
VMSTATE_UINT32(mode, ssi_sd_state),
@@ -287,6 +321,7 @@ static const VMStateDescription vmstate_ssi_sd = {
VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5),
VMSTATE_UINT16(crc16, ssi_sd_state),
VMSTATE_INT32(read_bytes, ssi_sd_state),
+VMSTATE_INT32(write_bytes, ssi_sd_state),
VMSTATE_INT32(arglen, ssi_sd_state),
VMSTATE_INT32(response_pos, ssi_sd_state),
VMSTATE_INT32(stopping, ssi_sd_state),
@@ -340,6 +375,7 @@ static void ssi_sd_reset(DeviceState *dev)
memset(s->response, 0, sizeof(s->response));
s->crc16 = 0;
s->read_bytes = 0;
+s->write_bytes = 0;
s->arglen = 0;
s->response_pos = 0;
s->stopping = 0;
--
2.25.1
[PATCH v4 0/9] hw/sd: Support block read/write in SPI mode
From: Bin Meng This includes the previously v3 series [1], and one single patch [2]. Compared to v3, this fixed the following issue in patch [v3,6/6]: - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after receiving the STOP_TRAN token per the spec All software tested so far (U-Boot/Linux/VxWorks) do work without the fix, but it is better to comform with the spec. In addition to [2], one more issue was exposed when testing with VxWorks driver related to STOP_TRANSMISSION (CMD12) response. [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136 [2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng...@gmail.com/ Changes in v4: - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after receiving the STOP_TRAN token per the spec - new patch: fix STOP_TRANSMISSION (CMD12) response - new patch: handle the rest commands with R1b response type Bin Meng (9): hw/sd: ssi-sd: Support multiple block read hw/sd: sd: Remove duplicated codes in single/multiple block read/write hw/sd: sd: Allow single/multiple block write for SPI mode hw/sd: Introduce receive_ready() callback hw/sd: ssi-sd: Support single block write hw/sd: ssi-sd: Support multiple block write hw/sd: ssi-sd: Fix SEND_IF_COND (CMD8) response hw/sd: ssi-sd: Fix STOP_TRANSMISSION (CMD12) response hw/sd: ssi-sd: Handle the rest commands with R1b response type include/hw/sd/sd.h | 2 + hw/sd/core.c | 13 + hw/sd/sd.c | 56 ++- hw/sd/ssi-sd.c | 136 ++--- 4 files changed, 137 insertions(+), 70 deletions(-) -- 2.25.1
[PATCH v4 2/9] hw/sd: sd: Remove duplicated codes in single/multiple block read/write
From: Bin Meng
The single block read (CMD17) codes are the same as the multiple
block read (CMD18). Merge them into one. The same applies to single
block write (CMD24) and multiple block write (CMD25).
Signed-off-by: Bin Meng
Acked-by: Alistair Francis
Tested-by: Philippe Mathieu-Daudé
---
(no changes since v1)
hw/sd/sd.c | 47 ---
1 file changed, 47 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b3952514fe..09753359bb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1181,24 +1181,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 17: /* CMD17: READ_SINGLE_BLOCK */
-switch (sd->state) {
-case sd_transfer_state:
-
-if (addr + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
-return sd_r1;
-}
-
-sd->state = sd_sendingdata_state;
-sd->data_start = addr;
-sd->data_offset = 0;
-return sd_r1;
-
-default:
-break;
-}
-break;
-
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
switch (sd->state) {
case sd_transfer_state:
@@ -1245,35 +1227,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
/* Block write commands (Class 4) */
case 24: /* CMD24: WRITE_SINGLE_BLOCK */
-switch (sd->state) {
-case sd_transfer_state:
-/* Writing in SPI mode not implemented. */
-if (sd->spi)
-break;
-
-if (addr + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
-return sd_r1;
-}
-
-sd->state = sd_receivingdata_state;
-sd->data_start = addr;
-sd->data_offset = 0;
-sd->blk_written = 0;
-
-if (sd_wp_addr(sd, sd->data_start)) {
-sd->card_status |= WP_VIOLATION;
-}
-if (sd->csd[14] & 0x30) {
-sd->card_status |= WP_VIOLATION;
-}
-return sd_r1;
-
-default:
-break;
-}
-break;
-
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
switch (sd->state) {
case sd_transfer_state:
--
2.25.1
[PATCH v4 1/9] hw/sd: ssi-sd: Support multiple block read
From: Bin Meng
In the case of a multiple block read operation every transferred
block has its suffix of CRC16. Update the state machine logic to
handle multiple block read.
Signed-off-by: Bin Meng
Acked-by: Alistair Francis
[PMD: Change VMState version id 5 -> 6]
Signed-off-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
---
(no changes since v1)
hw/sd/ssi-sd.c | 42 +-
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index be1bb10164..6d20a240c6 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -52,6 +52,7 @@ struct ssi_sd_state {
uint8_t cmdarg[4];
uint8_t response[5];
uint16_t crc16;
+int32_t read_bytes;
int32_t arglen;
int32_t response_pos;
int32_t stopping;
@@ -88,11 +89,26 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
{
ssi_sd_state *s = SSI_SD(dev);
-/* Special case: allow CMD12 (STOP TRANSMISSION) while reading data. */
-if (s->mode == SSI_SD_DATA_READ && val == 0x4c) {
-s->mode = SSI_SD_CMD;
-/* There must be at least one byte delay before the card responds. */
-s->stopping = 1;
+/*
+ * Special case: allow CMD12 (STOP TRANSMISSION) while reading data.
+ *
+ * See "Physical Layer Specification Version 8.00" chapter 7.5.2.2,
+ * to avoid conflict between CMD12 response and next data block,
+ * timing of CMD12 should be controlled as follows:
+ *
+ * - CMD12 issued at the timing that end bit of CMD12 and end bit of
+ * data block is overlapped
+ * - CMD12 issued after one clock cycle after host receives a token
+ * (either Start Block token or Data Error token)
+ *
+ * We need to catch CMD12 in all of the data read states.
+ */
+if (s->mode >= SSI_SD_PREP_DATA && s->mode <= SSI_SD_DATA_CRC16) {
+if (val == 0x4c) {
+s->mode = SSI_SD_CMD;
+/* There must be at least one byte delay before the card responds
*/
+s->stopping = 1;
+}
}
switch (s->mode) {
@@ -212,8 +228,9 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
return SSI_TOKEN_SINGLE;
case SSI_SD_DATA_READ:
val = sdbus_read_byte(>sdbus);
+s->read_bytes++;
s->crc16 = crc_ccitt_false(s->crc16, (uint8_t *), 1);
-if (!sdbus_data_ready(>sdbus)) {
+if (!sdbus_data_ready(>sdbus) || s->read_bytes == 512) {
DPRINTF("Data read end\n");
s->mode = SSI_SD_DATA_CRC16;
}
@@ -224,7 +241,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev,
uint32_t val)
s->response_pos++;
if (s->response_pos == 2) {
DPRINTF("CRC16 read end\n");
-s->mode = SSI_SD_CMD;
+if (s->read_bytes == 512 && s->cmd != 17) {
+s->mode = SSI_SD_PREP_DATA;
+} else {
+s->mode = SSI_SD_CMD;
+}
+s->read_bytes = 0;
s->response_pos = 0;
}
return val;
@@ -255,8 +277,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
static const VMStateDescription vmstate_ssi_sd = {
.name = "ssi_sd",
-.version_id = 5,
-.minimum_version_id = 5,
+.version_id = 6,
+.minimum_version_id = 6,
.post_load = ssi_sd_post_load,
.fields = (VMStateField []) {
VMSTATE_UINT32(mode, ssi_sd_state),
@@ -264,6 +286,7 @@ static const VMStateDescription vmstate_ssi_sd = {
VMSTATE_UINT8_ARRAY(cmdarg, ssi_sd_state, 4),
VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5),
VMSTATE_UINT16(crc16, ssi_sd_state),
+VMSTATE_INT32(read_bytes, ssi_sd_state),
VMSTATE_INT32(arglen, ssi_sd_state),
VMSTATE_INT32(response_pos, ssi_sd_state),
VMSTATE_INT32(stopping, ssi_sd_state),
@@ -316,6 +339,7 @@ static void ssi_sd_reset(DeviceState *dev)
memset(s->cmdarg, 0, sizeof(s->cmdarg));
memset(s->response, 0, sizeof(s->response));
s->crc16 = 0;
+s->read_bytes = 0;
s->arglen = 0;
s->response_pos = 0;
s->stopping = 0;
--
2.25.1
[PATCH v4 3/9] hw/sd: sd: Allow single/multiple block write for SPI mode
From: Bin Meng
At present the single/multiple block write in SPI mode is blocked
by sd_normal_command(). Remove the limitation.
Signed-off-by: Bin Meng
Acked-by: Alistair Francis
Tested-by: Philippe Mathieu-Daudé
---
(no changes since v1)
hw/sd/sd.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 09753359bb..946036d87c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1230,9 +1230,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
switch (sd->state) {
case sd_transfer_state:
-/* Writing in SPI mode not implemented. */
-if (sd->spi)
-break;
if (addr + sd->blk_len > sd->size) {
sd->card_status |= ADDRESS_ERROR;
--
2.25.1
Re: [PATCH 0/2] hw/block/nvme: zoned fixes
On Jan 19 14:54, Klaus Jensen wrote: > From: Klaus Jensen > > Patch [1/2] fixes the zone append bug reported by Niklas. [2/2] > refactors the zone write check function to return status codes in a > different order if there are multiple zone write violations that apply. > > Klaus Jensen (2): > hw/block/nvme: fix zone boundary check for append > hw/block/nvme: refactor the logic for zone write checks > > hw/block/nvme.c | 89 +-- > hw/block/trace-events | 5 +++ > 2 files changed, 48 insertions(+), 46 deletions(-) > > -- > 2.30.0 > > Thanks all, applied to nvme-next. signature.asc Description: PGP signature
Re: [PATCH 0/4] Introduce a battery, AC adapter, and lid button
On Tue, Jan 26, 2021 at 4:40 PM Michael S. Tsirkin wrote: > > Poking at sysfs from QEMU poses a bunch of issues, for example, > security, migration, etc. Running timers on the host is also not nice > since it causes exits from VM ... > > So I agree, as a starting point let's just let user > control the battery level through QMP. > Thanks for the review, Michael. Yep, I fully agree - indeed looking at the host's battery is better to be left for the user, and QEMU should just set what the guest sees. Will modify accordingly, and send a v2. Cheers, Leonid.
Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
On Wed, Jan 27, 2021 at 05:37:25PM -0600, Corey Minyard wrote: > On Wed, Jan 27, 2021 at 01:59:07PM -0800, Hao Wu wrote: > > > > > > There is also the question about who takes these patches in. I'm the > > > I2C maintainer, but there's other code in this series. Once everything > > > is ready, I can ack them if we take it through the ARM tree. Or I can > > > take it through my tree with the proper acks. > > > > > I think either way is fine. Previous NPCM7XX patch series were taken in > > the ARM tree. But as i2c code taking into your tree is also fine. > > Let's go through the ARM tree, then. So you have an: Acked-by: Corey Minyard For patches 2 and 6. Patch 4 still has the issue with the eeprom size. If you are expecting a 32K eeprom to work, it's not going to, you are only going to get 256 bytes. -corey
Re: [RFC] Set addresses for memory devices [CXL]
On Wed, Jan 27, 2021 at 7:52 PM Ben Widawsky wrote: > > Hi list, Igor. > > I wanted to get some ideas on how to better handle this. Per the recent > discussion [1], it's become clear that there needs to be more thought put into > how to manage the address space for CXL memory devices. If you see the > discussion on interleave [2] there's a decent diagram for the problem > statement. > > A CXL topology looks just like a PCIe topology. A CXL memory device is a > memory > expander. It's a byte addressable address range with a combination of > persistent > and volatile memory. In a CXL capable system, you can effectively think of > these > things as more configurable NVDIMMs. The memory devices have an interface that > allows the OS to program the base physical address range it claims called an > HDM > (Host Defined Memory) decoder. A larger address range is claimed by a host > bridge (or a combination of host bridges in the interleaved case) which is > platform specific. > > Originally, my plan was to create a single memory backend for a "window" and > subregion the devices in there. So for example, if you had two devices under a > hostbridge, each of 256M size, the window would be some fixed GPA of 512M+ > size > memory backend, and those memory devices would be a subregion of the > hostbridge's window. I thought this was working in my patch series, but as it > turns out, this doesn't actually work as I intended. `info mtree` looks good, > but `info memory-devices` doesn't. > A couple clarifying questions... > So let me list the requirements and hopefully get some feedback on the best > way > to handle it. > 1. A PCIe like device has a persistent memory region (I don't care about > volatile at the moment). What do you mean by "PCIe" like? If it is PCI enumerable by the guest it has no business being treated as proper memory because the OS rightly assumes that PCIe address space is not I/O coherent to other initiators. > 2. The physical address base for the memory region is programmable. > 3. Memory accesses will support interleaving across multiple host bridges. So, per 1. it would look like a PCIe address space inside QEMU but advertised as an I/O coherent platform resource in the guest?
Re: [PATCH v3 4/4] target/arm: Set ID_PFR0.DIT to 1 for "max" 32-bit CPU
On 1/27/21 6:45 PM, Rebecca Cran wrote: > Enable FEAT_DIT for the "max" 32-bit CPU. > > Signed-off-by: Rebecca Cran > --- > target/arm/cpu.c | 4 > 1 file changed, 4 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 2/4] target/arm: Add support for FEAT_DIT, Data Independent Timing
On 1/27/21 6:45 PM, Rebecca Cran wrote:
> Add support for FEAT_DIT. DIT (Data Independent Timing) is a required
> feature for ARMv8.4. Since virtual machine execution is largely
> nondeterministic and TCG is outside of the security domain, it's
> implemented as a NOP.
>
> Signed-off-by: Rebecca Cran
> Reviewed-by: Richard Henderson
This misses the convert from AA32 CPSR to AA64 SPSR on exception entry (and
vice-versa on return).
In particular: CPSR.DIT (bit 21) -> SPSR_EL1.DIT (bit 24), and merging
PSTATE.SS into SPSR_EL1.SS (bit 21).
r~
> ---
> target/arm/cpu.h | 12 +++
> target/arm/helper.c| 22
> target/arm/internals.h | 6 ++
> target/arm/translate-a64.c | 12 +++
> 4 files changed, 52 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index df0d6778330f..56b1053dfdec 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1242,6 +1242,7 @@ void pmu_init(ARMCPU *cpu);
> #define CPSR_IT_2_7 (0xfc00U)
> #define CPSR_GE (0xfU << 16)
> #define CPSR_IL (1U << 20)
> +#define CPSR_DIT (1U << 21)
> #define CPSR_PAN (1U << 22)
> #define CPSR_J (1U << 24)
> #define CPSR_IT_0_1 (3U << 25)
> @@ -1309,6 +1310,7 @@ void pmu_init(ARMCPU *cpu);
> #define PSTATE_SS (1U << 21)
> #define PSTATE_PAN (1U << 22)
> #define PSTATE_UAO (1U << 23)
> +#define PSTATE_DIT (1U << 24)
> #define PSTATE_TCO (1U << 25)
> #define PSTATE_V (1U << 28)
> #define PSTATE_C (1U << 29)
> @@ -3875,6 +3877,11 @@ static inline bool isar_feature_aa32_tts2uxn(const
> ARMISARegisters *id)
> return FIELD_EX32(id->id_mmfr4, ID_MMFR4, XNX) != 0;
> }
>
> +static inline bool isar_feature_aa32_dit(const ARMISARegisters *id)
> +{
> +return FIELD_EX32(id->id_pfr0, ID_PFR0, DIT) != 0;
> +}
> +
> /*
> * 64-bit feature tests via id registers.
> */
> @@ -4119,6 +4126,11 @@ static inline bool isar_feature_aa64_tts2uxn(const
> ARMISARegisters *id)
> return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, XNX) != 0;
> }
>
> +static inline bool isar_feature_aa64_dit(const ARMISARegisters *id)
> +{
> +return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, DIT) != 0;
> +}
> +
> /*
> * Feature tests for "does this exist in either 32-bit or 64-bit?"
> */
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7b7e72ba878c..435208121e9f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4419,6 +4419,24 @@ static const ARMCPRegInfo uao_reginfo = {
> .readfn = aa64_uao_read, .writefn = aa64_uao_write
> };
>
> +static uint64_t aa64_dit_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +return env->pstate & PSTATE_DIT;
> +}
> +
> +static void aa64_dit_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> +env->pstate = (env->pstate & ~PSTATE_DIT) | (value & PSTATE_DIT);
> +}
> +
> +static const ARMCPRegInfo dit_reginfo = {
> +.name = "DIT", .state = ARM_CP_STATE_AA64,
> +.opc0 = 3, .opc1 = 3, .crn = 4, .crm = 2, .opc2 = 5,
> +.type = ARM_CP_NO_RAW, .access = PL0_RW,
> +.readfn = aa64_dit_read, .writefn = aa64_dit_write
> +};
> +
> static CPAccessResult aa64_cacheop_poc_access(CPUARMState *env,
>const ARMCPRegInfo *ri,
>bool isread)
> @@ -8203,6 +8221,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> define_one_arm_cp_reg(cpu, _reginfo);
> }
>
> +if (cpu_isar_feature(aa64_dit, cpu)) {
> +define_one_arm_cp_reg(cpu, _reginfo);
> +}
> +
> if (arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu))
> {
> define_arm_cp_regs(cpu, vhe_reginfo);
> }
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 853fa88fd616..3d11e42d8e1b 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1222,6 +1222,9 @@ static inline uint32_t aarch32_cpsr_valid_mask(uint64_t
> features,
> if (isar_feature_aa32_pan(id)) {
> valid |= CPSR_PAN;
> }
> +if (isar_feature_aa32_dit(id)) {
> +valid |= CPSR_DIT;
> +}
>
> return valid;
> }
> @@ -1240,6 +1243,9 @@ static inline uint32_t aarch64_pstate_valid_mask(const
> ARMISARegisters *id)
> if (isar_feature_aa64_uao(id)) {
> valid |= PSTATE_UAO;
> }
> +if (isar_feature_aa64_dit(id)) {
> +valid |= PSTATE_DIT;
> +}
> if (isar_feature_aa64_mte(id)) {
> valid |= PSTATE_TCO;
> }
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index ffc060e5d70c..1c4b8d02f3b8 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1700,6 +1700,18 @@ static void handle_msr_i(DisasContext *s, uint32_t
> insn,
> tcg_temp_free_i32(t1);
> break;
>
> +case 0x1a: /* DIT */
> +if (!dc_isar_feature(aa64_dit, s)) {
> +goto do_unallocated;
> +}
> +
[PATCH] tcg/tci: Implement INDEX_op_ld16s_i32
That TCG opcode is used by debian-buster (arm64) running ffmpeg: qemu-aarch64 /usr/bin/ffmpeg -i theora.mkv theora.webm Reported-by: Alex Bennée Signed-off-by: Stefan Weil --- tcg/tci.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tcg/tci.c b/tcg/tci.c index 63d79dac87..615423b06c 100644 --- a/tcg/tci.c +++ b/tcg/tci.c @@ -621,7 +621,10 @@ uintptr_t QEMU_DISABLE_CFI tcg_qemu_tb_exec(CPUArchState *env, tci_write_reg16(regs, t0, *(uint16_t *)(t1 + t2)); break; case INDEX_op_ld16s_i32: -TODO(); +t0 = *tb_ptr++; +t1 = tci_read_r(regs, _ptr); +t2 = tci_read_s32(_ptr); +tci_write_reg(regs, t0, *(int16_t *)(t1 + t2)); break; case INDEX_op_ld_i32: t0 = *tb_ptr++; -- 2.29.2
Re: [PATCH v3 1/4] target/arm: Remove PSTATE_SS from cpsr and move it into env->pstate.
On 1/27/21 6:45 PM, Rebecca Cran wrote: > cpsr has been treated as being the same as spsr, but it isn't. > Since PSTATE_SS isn't in cpsr, remove it and move it into env->pstate. > > Signed-off-by: Rebecca Cran > --- > target/arm/helper-a64.c | 2 +- > target/arm/helper.c | 2 +- > target/arm/op_helper.c | 9 + > 3 files changed, 3 insertions(+), 10 deletions(-) Missing a change to cpu_get_tb_cpu_state to remove pstate_for_ss and simply use env->pstate. r~
[PATCH v3 3/4] target/arm: Set ID_AA64PFR0.DIT and ID_PFR0.DIT to 1 for "max" AA64 CPU
Enable FEAT_DIT for the "max" AARCH64 CPU. Signed-off-by: Rebecca Cran Reviewed-by: Richard Henderson --- target/arm/cpu64.c | 5 + 1 file changed, 5 insertions(+) diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 5e851028c592..9a5cfd4fc632 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -666,6 +666,7 @@ static void aarch64_max_initfn(Object *obj) t = FIELD_DP64(t, ID_AA64PFR0, FP, 1); t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1); t = FIELD_DP64(t, ID_AA64PFR0, SEL2, 1); +t = FIELD_DP64(t, ID_AA64PFR0, DIT, 1); cpu->isar.id_aa64pfr0 = t; t = cpu->isar.id_aa64pfr1; @@ -715,6 +716,10 @@ static void aarch64_max_initfn(Object *obj) u = FIELD_DP32(u, ID_ISAR6, SPECRES, 1); cpu->isar.id_isar6 = u; +u = cpu->isar.id_pfr0; +u = FIELD_DP32(u, ID_PFR0, DIT, 1); +cpu->isar.id_pfr0 = u; + u = cpu->isar.id_mmfr3; u = FIELD_DP32(u, ID_MMFR3, PAN, 2); /* ATS1E1 */ cpu->isar.id_mmfr3 = u; -- 2.26.2
[PATCH v3 2/4] target/arm: Add support for FEAT_DIT, Data Independent Timing
Add support for FEAT_DIT. DIT (Data Independent Timing) is a required
feature for ARMv8.4. Since virtual machine execution is largely
nondeterministic and TCG is outside of the security domain, it's
implemented as a NOP.
Signed-off-by: Rebecca Cran
Reviewed-by: Richard Henderson
---
target/arm/cpu.h | 12 +++
target/arm/helper.c| 22
target/arm/internals.h | 6 ++
target/arm/translate-a64.c | 12 +++
4 files changed, 52 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index df0d6778330f..56b1053dfdec 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1242,6 +1242,7 @@ void pmu_init(ARMCPU *cpu);
#define CPSR_IT_2_7 (0xfc00U)
#define CPSR_GE (0xfU << 16)
#define CPSR_IL (1U << 20)
+#define CPSR_DIT (1U << 21)
#define CPSR_PAN (1U << 22)
#define CPSR_J (1U << 24)
#define CPSR_IT_0_1 (3U << 25)
@@ -1309,6 +1310,7 @@ void pmu_init(ARMCPU *cpu);
#define PSTATE_SS (1U << 21)
#define PSTATE_PAN (1U << 22)
#define PSTATE_UAO (1U << 23)
+#define PSTATE_DIT (1U << 24)
#define PSTATE_TCO (1U << 25)
#define PSTATE_V (1U << 28)
#define PSTATE_C (1U << 29)
@@ -3875,6 +3877,11 @@ static inline bool isar_feature_aa32_tts2uxn(const
ARMISARegisters *id)
return FIELD_EX32(id->id_mmfr4, ID_MMFR4, XNX) != 0;
}
+static inline bool isar_feature_aa32_dit(const ARMISARegisters *id)
+{
+return FIELD_EX32(id->id_pfr0, ID_PFR0, DIT) != 0;
+}
+
/*
* 64-bit feature tests via id registers.
*/
@@ -4119,6 +4126,11 @@ static inline bool isar_feature_aa64_tts2uxn(const
ARMISARegisters *id)
return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, XNX) != 0;
}
+static inline bool isar_feature_aa64_dit(const ARMISARegisters *id)
+{
+return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, DIT) != 0;
+}
+
/*
* Feature tests for "does this exist in either 32-bit or 64-bit?"
*/
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7b7e72ba878c..435208121e9f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4419,6 +4419,24 @@ static const ARMCPRegInfo uao_reginfo = {
.readfn = aa64_uao_read, .writefn = aa64_uao_write
};
+static uint64_t aa64_dit_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+return env->pstate & PSTATE_DIT;
+}
+
+static void aa64_dit_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+env->pstate = (env->pstate & ~PSTATE_DIT) | (value & PSTATE_DIT);
+}
+
+static const ARMCPRegInfo dit_reginfo = {
+.name = "DIT", .state = ARM_CP_STATE_AA64,
+.opc0 = 3, .opc1 = 3, .crn = 4, .crm = 2, .opc2 = 5,
+.type = ARM_CP_NO_RAW, .access = PL0_RW,
+.readfn = aa64_dit_read, .writefn = aa64_dit_write
+};
+
static CPAccessResult aa64_cacheop_poc_access(CPUARMState *env,
const ARMCPRegInfo *ri,
bool isread)
@@ -8203,6 +8221,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
define_one_arm_cp_reg(cpu, _reginfo);
}
+if (cpu_isar_feature(aa64_dit, cpu)) {
+define_one_arm_cp_reg(cpu, _reginfo);
+}
+
if (arm_feature(env, ARM_FEATURE_EL2) && cpu_isar_feature(aa64_vh, cpu)) {
define_arm_cp_regs(cpu, vhe_reginfo);
}
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 853fa88fd616..3d11e42d8e1b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1222,6 +1222,9 @@ static inline uint32_t aarch32_cpsr_valid_mask(uint64_t
features,
if (isar_feature_aa32_pan(id)) {
valid |= CPSR_PAN;
}
+if (isar_feature_aa32_dit(id)) {
+valid |= CPSR_DIT;
+}
return valid;
}
@@ -1240,6 +1243,9 @@ static inline uint32_t aarch64_pstate_valid_mask(const
ARMISARegisters *id)
if (isar_feature_aa64_uao(id)) {
valid |= PSTATE_UAO;
}
+if (isar_feature_aa64_dit(id)) {
+valid |= PSTATE_DIT;
+}
if (isar_feature_aa64_mte(id)) {
valid |= PSTATE_TCO;
}
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index ffc060e5d70c..1c4b8d02f3b8 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1700,6 +1700,18 @@ static void handle_msr_i(DisasContext *s, uint32_t insn,
tcg_temp_free_i32(t1);
break;
+case 0x1a: /* DIT */
+if (!dc_isar_feature(aa64_dit, s)) {
+goto do_unallocated;
+}
+if (crm & 1) {
+set_pstate_bits(PSTATE_DIT);
+} else {
+clear_pstate_bits(PSTATE_DIT);
+}
+/* There's no need to rebuild hflags because DIT is a nop */
+break;
+
case 0x1e: /* DAIFSet */
t1 = tcg_const_i32(crm);
gen_helper_msr_i_daifset(cpu_env, t1);
--
2.26.2
[PATCH v3 4/4] target/arm: Set ID_PFR0.DIT to 1 for "max" 32-bit CPU
Enable FEAT_DIT for the "max" 32-bit CPU. Signed-off-by: Rebecca Cran --- target/arm/cpu.c | 4 1 file changed, 4 insertions(+) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 40142ac141e5..c98f44624423 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -2197,6 +2197,10 @@ static void arm_max_initfn(Object *obj) t = FIELD_DP32(t, ID_MMFR4, CNP, 1); /* TTCNP */ t = FIELD_DP32(t, ID_MMFR4, XNX, 1); /* TTS2UXN */ cpu->isar.id_mmfr4 = t; + +t = cpu->isar.id_pfr0; +t = FIELD_DP32(t, ID_PFR0, DIT, 1); +cpu->isar.id_pfr0 = t; } #endif } -- 2.26.2
[PATCH v3 0/4] target/arm: Add support for FEAT_DIT, Data Independent Timing
Add support for FEAT_DIT. DIT (Data Independent Timing) is a required feature for ARMv8.4. Changes from v2 to v3: o Fixed PSTATE_SS patch: - Reverted the singlestep removal. - Fixed saving cpsr into spsr. o Added DIT to the max 32-bit CPU. Rebecca Cran (4): target/arm: Remove PSTATE_SS from cpsr and move it into env->pstate. target/arm: Add support for FEAT_DIT, Data Independent Timing target/arm: Set ID_AA64PFR0.DIT and ID_PFR0.DIT to 1 for "max" AA64 CPU target/arm: Set ID_PFR0.DIT to 1 for "max" 32-bit CPU target/arm/cpu.c | 4 target/arm/cpu.h | 12 ++ target/arm/cpu64.c | 5 target/arm/helper-a64.c| 2 +- target/arm/helper.c| 24 +++- target/arm/internals.h | 6 + target/arm/op_helper.c | 9 +--- target/arm/translate-a64.c | 12 ++ 8 files changed, 64 insertions(+), 10 deletions(-) -- 2.26.2
[PATCH v3 1/4] target/arm: Remove PSTATE_SS from cpsr and move it into env->pstate.
cpsr has been treated as being the same as spsr, but it isn't.
Since PSTATE_SS isn't in cpsr, remove it and move it into env->pstate.
Signed-off-by: Rebecca Cran
---
target/arm/helper-a64.c | 2 +-
target/arm/helper.c | 2 +-
target/arm/op_helper.c | 9 +
3 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index c426c23d2c4e..a6b162049806 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -1001,7 +1001,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t
new_pc)
mask = aarch32_cpsr_valid_mask(env->features, _archcpu(env)->isar);
cpsr_write(env, spsr, mask, CPSRWriteRaw);
if (!arm_singlestep_active(env)) {
-env->uncached_cpsr &= ~PSTATE_SS;
+env->pstate &= ~PSTATE_SS;
}
aarch64_sync_64_to_32(env);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d2ead3fcbdbd..7b7e72ba878c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9402,7 +9402,7 @@ static void take_aarch32_exception(CPUARMState *env, int
new_mode,
* For exceptions taken to AArch32 we must clear the SS bit in both
* PSTATE and in the old-state value we save to SPSR_, so zero it
now.
*/
-env->uncached_cpsr &= ~PSTATE_SS;
+env->pstate &= ~PSTATE_SS;
env->spsr = cpsr_read(env);
/* Clear IT bits. */
env->condexec_bits = 0;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 5e0f123043b5..65cb37d088f8 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -389,14 +389,7 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env,
uint32_t syndrome)
uint32_t HELPER(cpsr_read)(CPUARMState *env)
{
-/*
- * We store the ARMv8 PSTATE.SS bit in env->uncached_cpsr.
- * This is convenient for populating SPSR_ELx, but must be
- * hidden from aarch32 mode, where it is not visible.
- *
- * TODO: ARMv8.4-DIT -- need to move SS somewhere else.
- */
-return cpsr_read(env) & ~(CPSR_EXEC | PSTATE_SS);
+return cpsr_read(env) & ~CPSR_EXEC;
}
void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
--
2.26.2
Re: Fwd: VirtioSound device emulation implementation
Thanks a lot Alex! > All QEMU devices have two parts, a frontend (which the guest sees) and a > backend (which is how the data gets to somewhere in the host). Some of > the command line options in QEMU elide the details for convenience (-nic > and -drive are examples). The -netdev device is all about configuring > the backend of the network part for a paired -device front end. There is > a similar setup for audio (-audiodev) although I'll defer to Gerd's > expertise on how the two interact. This helped me understand a bit more about how the devices work. In the source code, I found the `virtio-net-pci.c` and `virtio-net.c` files, I think the pci device is what is visible to the guest. So `virtio-net-pci.c` should be the front end? For the realize function, I saw that the `virtio_net_device_realize` function initializes the `net_conf` for the device. So I am guessing that the `virtio_snd_device_realize` function should initialize the number of jacks and streams a device has, along with the configuration for all these jacks and streams? The thing is I do not understand `net` devices all that well so I get a bit confused with what is specific to a net device and what should I actually be worried about :) The device initalization step mentions that the jack and streams should be read and a query should be made for the config of all jacks and streams. What should be the default values of these configurations? I think the realize function is responsible for setting these up.
[Bug 1913510] [NEW] [Fuzz] qemu-system-i386 virtio-mouse: Assertion in address_space_lduw_le_cached failed
Public bug reported: --[ Reproducer cat << EOF | ./build/qemu-system-i386 -machine q35,accel=qtest -nodefaults \ -device virtio-mouse -display none -qtest stdio outl 0xcf8 0x8820 outl 0xcfc 0xe0004000 outl 0xcf8 0x8804 outb 0xcfc 0x02 write 0xe000400c 0x4 0x003fe62e write 0xe0004016 0x1 0x01 write 0xe0004024 0x1 0x01 write 0xe000401c 0x1 0x01 write 0xe0007007 0x1 0x00 write 0xe0004018 0x1 0x41 write 0xe0007007 0x1 0x00 EOF --[ Output [I 1611805425.711054] OPENED [R +0.040080] outl 0xcf8 0x8820 OK [S +0.040117] OK [R +0.040136] outl 0xcfc 0xe0004000 OK [S +0.040155] OK [R +0.040165] outl 0xcf8 0x8804 OK [S +0.040172] OK [R +0.040184] outb 0xcfc 0x02 OK [S +0.040683] OK [R +0.040702] write 0xe000400c 0x4 0x003fe62e OK [S +0.040735] OK [R +0.040743] write 0xe0004016 0x1 0x01 OK [S +0.040748] OK [R +0.040755] write 0xe0004024 0x1 0x01 OK [S +0.040760] OK [R +0.040767] write 0xe000401c 0x1 0x01 OK [S +0.040785] OK [R +0.040792] write 0xe0007007 0x1 0x00 OK [S +0.040810] OK [R +0.040817] write 0xe0004018 0x1 0x41 OK [S +0.040822] OK [R +0.040839] write 0xe0007007 0x1 0x00 qemu-system-i386: /home/ubuntu/qemu/include/exec/memory_ldst_cached.h.inc:54: uint32_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): Assertion `addr < cache->len && 2 <= cache->len - addr' failed. -- [ Original ASAN report qemu-fuzz-i386: /home/ubuntu/qemu/include/exec/memory_ldst_cached.h.inc:54: uint32_t address_space_lduw_le_cached(MemoryRegionCache *, hwaddr, MemTxAttrs, MemTxResult *): Assertion `addr < cache->len && 2 <= cache->len - addr' failed. ==3406167== ERROR: libFuzzer: deadly signal #0 0x5644e4ae0f21 in __sanitizer_print_stack_trace (/home/ubuntu/qemu/build/qemu-fuzz-i386+0x2a47f21) #1 0x5644e4a29fe8 in fuzzer::PrintStackTrace() (/home/ubuntu/qemu/build/qemu-fuzz-i386+0x2990fe8) #2 0x5644e4a10023 in fuzzer::Fuzzer::CrashCallback() (/home/ubuntu/qemu/build/qemu-fuzz-i386+0x2977023) #3 0x7f77e2a4b3bf (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf) #4 0x7f77e285c18a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a) #5 0x7f77e283b858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858) #6 0x7f77e283b728 (/lib/x86_64-linux-gnu/libc.so.6+0x25728) #7 0x7f77e284cf35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35) #8 0x5644e60051b2 in address_space_lduw_le_cached /home/ubuntu/qemu/include/exec/memory_ldst_cached.h.inc:54:5 #9 0x5644e60051b2 in lduw_le_phys_cached /home/ubuntu/qemu/include/exec/memory_ldst_phys.h.inc:91:12 #10 0x5644e60051b2 in virtio_lduw_phys_cached /home/ubuntu/qemu/include/hw/virtio/virtio-access.h:166:12 #11 0x5644e5ff476d in vring_avail_ring /home/ubuntu/qemu/build/../hw/virtio/virtio.c:327:12 #12 0x5644e5ff476d in vring_get_used_event /home/ubuntu/qemu/build/../hw/virtio/virtio.c:333:12 #13 0x5644e5ff476d in virtio_split_should_notify /home/ubuntu/qemu/build/../hw/virtio/virtio.c:2473:35 #14 0x5644e5ff476d in virtio_should_notify /home/ubuntu/qemu/build/../hw/virtio/virtio.c:2524:16 #15 0x5644e5ff5556 in virtio_notify /home/ubuntu/qemu/build/../hw/virtio/virtio.c:2566:14 #16 0x5644e5571d2a in virtio_input_handle_sts /home/ubuntu/qemu/build/../hw/input/virtio-input.c:100:5 #17 0x5644e5ff20ec in virtio_queue_notify /home/ubuntu/qemu/build/../hw/virtio/virtio.c:2366:9 #18 0x5644e60908fb in memory_region_write_accessor /home/ubuntu/qemu/build/../softmmu/memory.c:491:5 #19 0x5644e6090363 in access_with_adjusted_size /home/ubuntu/qemu/build/../softmmu/memory.c:552:18 #20 0x5644e608fbc0 in memory_region_dispatch_write /home/ubuntu/qemu/build/../softmmu/memory.c #21 0x5644e5b97bc6 in flatview_write_continue /home/ubuntu/qemu/build/../softmmu/physmem.c:2759:23 #22 0x5644e5b8d328 in flatview_write /home/ubuntu/qemu/build/../softmmu/physmem.c:2799:14 #23 0x5644e5b8d328 in address_space_write /home/ubuntu/qemu/build/../softmmu/physmem.c:2891:18 #24 0x5644e6018906 in qtest_process_command /home/ubuntu/qemu/build/../softmmu/qtest.c:539:13 #25 0x5644e60159df in qtest_process_inbuf /home/ubuntu/qemu/build/../softmmu/qtest.c:797:9 #26 0x5644e6015735 in qtest_server_inproc_recv /home/ubuntu/qemu/build/../softmmu/qtest.c:904:9 #27 0x5644e667cf68 in qtest_sendf /home/ubuntu/qemu/build/../tests/qtest/libqtest.c:438:5 #28 0x5644e667e54e in qtest_write /home/ubuntu/qemu/build/../tests/qtest/libqtest.c:1002:5 #29 0x5644e667e54e in qtest_writeq /home/ubuntu/qemu/build/../tests/qtest/libqtest.c:1023:5 #30 0x5644e4b1037e in __wrap_qtest_writeq /home/ubuntu/qemu/build/../tests/qtest/fuzz/qtest_wrappers.c:190:9 #31 0x5644e4b1c33d in op_write /home/ubuntu/qemu/build/../tests/qtest/fuzz/generic_fuzz.c:479:13 #32 0x5644e4b1a259 in generic_fuzz /home/ubuntu/qemu/build/../tests/qtest/fuzz/generic_fuzz.c:681:17 #33 0x5644e4b0b333 in LLVMFuzzerTestOneInput
[PATCH] fuzz: fix wrong index in clear_bits
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 4cba96dee2..20825768c2 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -261,7 +261,7 @@ def clear_bits(newtrace, outpath):
data_try = hex(int("".join(data_bin_list), 2))
# It seems qtest only accepts padded hex-values.
if len(data_try) % 2 == 1:
-data_try = data_try[:2] + "0" + data_try[2:-1]
+data_try = data_try[:2] + "0" + data_try[2:]
newtrace[i] = "{prefix} {data_try}\n".format(
prefix=prefix,
--
2.25.1
[RFC] Set addresses for memory devices [CXL]
Hi list, Igor. I wanted to get some ideas on how to better handle this. Per the recent discussion [1], it's become clear that there needs to be more thought put into how to manage the address space for CXL memory devices. If you see the discussion on interleave [2] there's a decent diagram for the problem statement. A CXL topology looks just like a PCIe topology. A CXL memory device is a memory expander. It's a byte addressable address range with a combination of persistent and volatile memory. In a CXL capable system, you can effectively think of these things as more configurable NVDIMMs. The memory devices have an interface that allows the OS to program the base physical address range it claims called an HDM (Host Defined Memory) decoder. A larger address range is claimed by a host bridge (or a combination of host bridges in the interleaved case) which is platform specific. Originally, my plan was to create a single memory backend for a "window" and subregion the devices in there. So for example, if you had two devices under a hostbridge, each of 256M size, the window would be some fixed GPA of 512M+ size memory backend, and those memory devices would be a subregion of the hostbridge's window. I thought this was working in my patch series, but as it turns out, this doesn't actually work as I intended. `info mtree` looks good, but `info memory-devices` doesn't. So let me list the requirements and hopefully get some feedback on the best way to handle it. 1. A PCIe like device has a persistent memory region (I don't care about volatile at the moment). 2. The physical address base for the memory region is programmable. 3. Memory accesses will support interleaving across multiple host bridges. As far as I can tell, there isn't anything that works quite like this today, and, my attempts so far haven't been correct. Thanks. Ben References: [1] https://lore.kernel.org/qemu-devel/20210126213013.6v24im4sler3q...@mail.bwidawsk.net/ [2] https://lore.kernel.org/qemu-devel/c51b000e-80db-40e9-d878-f260c49e4...@amsat.org/ Other: https://lore.kernel.org/qemu-devel/20210105165323.783725-23-ben.widaw...@intel.com/ https://lore.kernel.org/qemu-devel/20210105165323.783725-26-ben.widaw...@intel.com/
[PATCH] hw/char/exynos4210_uart: Fix buffer size reporting with FIFO disabled
Currently the Exynos 4210 UART code always reports available FIFO space
when the backend checks for buffer space. When the FIFO is disabled this
is behavior causes the backend chardev code to replace the data before the
guest can read it.
This patch changes adds the logic to report the capacity properly when the
FIFO is not being used.
Buglink: https://bugs.launchpad.net/qemu/+bug/1913344
Signed-off-by: Iris Johnson
---
hw/char/exynos4210_uart.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 6361df2ad3..9b21d201b3 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -553,7 +553,11 @@ static int exynos4210_uart_can_receive(void *opaque)
{
Exynos4210UartState *s = (Exynos4210UartState *)opaque;
-return fifo_empty_elements_number(>rx);
+if (s->reg[I_(UFCON)] & UFCON_FIFO_ENABLE) {
+return fifo_empty_elements_number(>rx);
+} else {
+return !(s->reg[I_(UTRSTAT)] & UTRSTAT_Rx_BUFFER_DATA_READY);
+}
}
static void exynos4210_uart_receive(void *opaque, const uint8_t *buf, int size)
--
2.25.1
[PATCH] tcg/tci: Implement INDEX_op_ld16s_i32
That TCG opcode is used by debian-buster (arm64) running ffmpeg: qemu-aarch64 /usr/bin/ffmpeg -i theora.mkv theora.webm Reported-by: Alex Bennée Signed-off-by: Stefan Weil --- tcg/tci.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tcg/tci.c b/tcg/tci.c index 63d79dac87..615423b06c 100644 --- a/tcg/tci.c +++ b/tcg/tci.c @@ -621,7 +621,10 @@ uintptr_t QEMU_DISABLE_CFI tcg_qemu_tb_exec(CPUArchState *env, tci_write_reg16(regs, t0, *(uint16_t *)(t1 + t2)); break; case INDEX_op_ld16s_i32: -TODO(); +t0 = *tb_ptr++; +t1 = tci_read_r(regs, _ptr); +t2 = tci_read_s32(_ptr); +tci_write_reg(regs, t0, *(int16_t *)(t1 + t2)); break; case INDEX_op_ld_i32: t0 = *tb_ptr++; -- 2.29.2
Re: [PATCH v4 4/4] meson: Warn when TCI is selected but TCG backend is available
Am 27.01.21 um 22:47 schrieb Alex Bennée: Your just going to end up playing wack-a-mole: TODO ../../tcg/tci.c:620: tcg_qemu_tb_exec()me=00:00:00.00 bitrate=N/A speed= 0x ../../tcg/tci.c:620: tcg fatal error qemu: uncaught target signal 11 (Segmentation fault) - core dumped Segmentation fault (core dumped) That's INDEX_op_ld16s_i32. Only five levels left in the play as soon as that is implemented, too. Thanks, Stefan
[Bug 1913505] [NEW] Windows XP slow on Apple M1
Public bug reported: Qemu installed by using brew install qemu -s on M1 QEMU emulator version 5.2.0 XP image from: https://archive.org/details/WinXPProSP3x86 Commands run: $ qemu-img create -f qcow2 xpsp3.img 10G $ qemu-system-i386 -m 512 -hda xpsp3.img -cdrom WinXPProSP3x86/en_windows_xp_professional_with_service_pack_3_x86_cd_vl_x14-73974.iso -boot d It's taken 3 days now with qemu running at around 94% CPU and installation hasn't finished. The mouse pointer moves and occasionally changes between the pointer and hourglass so it doesn't seem to have frozen. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1913505 Title: Windows XP slow on Apple M1 Status in QEMU: New Bug description: Qemu installed by using brew install qemu -s on M1 QEMU emulator version 5.2.0 XP image from: https://archive.org/details/WinXPProSP3x86 Commands run: $ qemu-img create -f qcow2 xpsp3.img 10G $ qemu-system-i386 -m 512 -hda xpsp3.img -cdrom WinXPProSP3x86/en_windows_xp_professional_with_service_pack_3_x86_cd_vl_x14-73974.iso -boot d It's taken 3 days now with qemu running at around 94% CPU and installation hasn't finished. The mouse pointer moves and occasionally changes between the pointer and hourglass so it doesn't seem to have frozen. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1913505/+subscriptions
Re: [RFC PATCH v2 2/3] vfio: Set the priority of the VFIO VM state change handler explicitly
On 2021/1/27 22:20, Alex Williamson wrote: > On Wed, 27 Jan 2021 19:20:06 +0800 > Shenming Lu wrote: > >> On 2021/1/27 5:36, Alex Williamson wrote: >>> On Wed, 9 Dec 2020 16:09:18 +0800 >>> Shenming Lu wrote: >>> In the VFIO VM state change handler, VFIO devices are transitioned in the _SAVING state, which should keep them from sending interrupts. >>> >>> Is this comment accurate? It's my expectation that _SAVING has no >>> bearing on a device generating interrupts. Interrupt generation must >>> be allowed to continue so long as the device is _RUNNING. Thanks, >>> >> >> To be more accurate, the _RUNNING bit in device_state is cleared in the >> VFIO VM state change handler when stopping the VM. And if the device >> continues >> to send interrupts after this, how can we save the states of device >> interrupts >> in the stop-and-copy phase?... > > Exactly, it's clearing the _RUNNING bit that makes the device stop, > including no longer generating interrupts. Perhaps I incorrectly > inferred "_SAVING state" as referring to the _SAVING bit when you > actually intended: > >* +--- _RESUMING >* |+-- _SAVING >* ||+- _RUNNING >* ||| >* 000b => Device Stopped, not saving or resuming >* 001b => Device running, which is the default state > -> * 010b => Stop the device & save the device state, stop-and-copy state > > ie. the full state when only _SAVING is set. > > Could we make the comment more clear to avoid this confusion? Thanks, > OK, sorry for the confusion. I will modify the comment to: In the VFIO VM state change handler when stopping the VM, the _RUNNING bit in device_state is cleared which makes the VFIO device stop, including no longer generating interrupts. Thanks, Shenming > Alex > Then we can save the pending states of all interrupts in the GIC VM state change handler (on ARM). So we have to set the priority of the VFIO VM state change handler explicitly (like virtio devices) to ensure it is called before the GIC's in saving. Signed-off-by: Shenming Lu Reviewed-by: Kirti Wankhede --- hw/vfio/migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 3b9de1353a..97ea82b100 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -862,7 +862,8 @@ static int vfio_migration_init(VFIODevice *vbasedev, register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, _vfio_handlers, vbasedev); -migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change, +migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev, + vfio_vmstate_change, vbasedev); migration->migration_state.notify = vfio_migration_state_notifier; add_migration_state_change_notifier(>migration_state); >>> >>> . >>> >> > > . >
Re: [PATCH] vhost-user: Check for iotlb callback in iotlb_miss
On 2021/1/28 上午4:44, Eugenio Pérez wrote:
Not registering this can lead to vhost_backend_handle_iotlb_msg and
vhost_device_iotlb_miss if backend issue a miss after qemu vhost device
stop.
This causes a try to access dev->vdev->dma_as with vdev == NULL.
Hi Eugenio:
What condition can we get this? Did you mean we receive IOTLB_MISS
before vhost_dev_start()?
If yes, it looks to me a bug somewhere else.
Thanks
Reproduced rebooting a guest with testpmd in txonly forward mode.
#0 0x55994394 in vhost_device_iotlb_miss (
dev=dev@entry=0x55a0012f6680, iova=10245279744, write=1)
at ../hw/virtio/vhost.c:1013
#1 0x5599ac31 in vhost_backend_handle_iotlb_msg (
imsg=0x7ffddcfd32c0, dev=0x55a0012f6680)
at ../hw/virtio/vhost-backend.c:411
#2 vhost_backend_handle_iotlb_msg (dev=dev@entry=0x55a0012f6680,
imsg=imsg@entry=0x7ffddcfd32c0)
at ../hw/virtio/vhost-backend.c:404
#3 0x559fffeded7b in slave_read (opaque=0x55a0012f6680)
at ../hw/virtio/vhost-user.c:1464
#4 0x55ac541b in aio_dispatch_handler (
ctx=ctx@entry=0x55a0010a2120, node=0x55a0012d9e00)
at ../util/aio-posix.c:329
Fixes: 6dcdd06e3b ("spec/vhost-user spec: Add IOMMU support")
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-user.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2fdd5daf74..a49b2229fb 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -238,6 +238,7 @@ struct vhost_user {
/* Shared between vhost devs of the same virtio device */
VhostUserState *user;
int slave_fd;
+bool iotlb_enabled;
NotifierWithReturn postcopy_notifier;
struct PostCopyFD postcopy_fd;
uint64_t postcopy_client_bases[VHOST_USER_MAX_RAM_SLOTS];
@@ -1461,7 +1462,11 @@ static void slave_read(void *opaque)
switch (hdr.request) {
case VHOST_USER_SLAVE_IOTLB_MSG:
-ret = vhost_backend_handle_iotlb_msg(dev, );
+if (likely(u->iotlb_enabled)) {
+ret = vhost_backend_handle_iotlb_msg(dev, );
+} else {
+ret = -EFAULT;
+}
break;
case VHOST_USER_SLAVE_CONFIG_CHANGE_MSG :
ret = vhost_user_slave_handle_config_change(dev);
@@ -2044,7 +2049,8 @@ static int vhost_user_send_device_iotlb_msg(struct
vhost_dev *dev,
static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
{
-/* No-op as the receive channel is not dedicated to IOTLB messages. */
+struct vhost_user *u = dev->opaque;
+u->iotlb_enabled = enabled;
}
static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
[PATCH v2] tcg/tci: Implement INDEX_op_ld8s_i64
That TCG opcode is used by debian-buster (arm64) running ffmpeg: qemu-aarch64 /usr/bin/ffmpeg -i theora.mkv theora.webm Reported-by: Alex Bennée Signed-off-by: Stefan Weil --- v2: Fixed as suggested by Richard Henderson Thank you, Stefan tcg/tci.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tcg/tci.c b/tcg/tci.c index ddbb259e1d..63d79dac87 100644 --- a/tcg/tci.c +++ b/tcg/tci.c @@ -886,7 +886,10 @@ uintptr_t QEMU_DISABLE_CFI tcg_qemu_tb_exec(CPUArchState *env, tci_write_reg8(regs, t0, *(uint8_t *)(t1 + t2)); break; case INDEX_op_ld8s_i64: -TODO(); +t0 = *tb_ptr++; +t1 = tci_read_r(regs, _ptr); +t2 = tci_read_s32(_ptr); +tci_write_reg(regs, t0, *(int8_t *)(t1 + t2)); break; case INDEX_op_ld16u_i64: t0 = *tb_ptr++; -- 2.29.2
Re: [PATCH 7/7] ppc/pnv: Introduce a LPC FW memory region attribute to map the PNOR
On Tue, Jan 26, 2021 at 06:10:59PM +0100, Cédric Le Goater wrote:
> This to map the PNOR from the machine init handler directly and finish
> the cleanup of the LPC model.
>
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.0, thanks.
> ---
> include/hw/ppc/pnv.h | 1 +
> hw/ppc/pnv.c | 11 +++
> hw/ppc/pnv_lpc.c | 7 ---
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index ee7eda3e0102..d69cee17b232 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -58,6 +58,7 @@ struct PnvChip {
> MemoryRegion xscom;
> AddressSpace xscom_as;
>
> +MemoryRegion *fw_mr;
> gchar*dt_isa_nodename;
> };
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e500c2e2437e..50810df83815 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -871,6 +871,14 @@ static void pnv_init(MachineState *machine)
> pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
> }
>
> +/*
> + * The PNOR is mapped on the LPC FW address space by the BMC.
> + * Since we can not reach the remote BMC machine with LPC memops,
> + * map it always for now.
> + */
> +memory_region_add_subregion(pnv->chips[0]->fw_mr, PNOR_SPI_OFFSET,
> +>pnor->mmio);
> +
> /*
> * OpenPOWER systems use a IPMI SEL Event message to notify the
> * host to powerdown
> @@ -1150,6 +1158,7 @@ static void pnv_chip_power8_realize(DeviceState *dev,
> Error **errp)
> qdev_realize(DEVICE(>lpc), NULL, _fatal);
> pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE,
> >lpc.xscom_regs);
>
> +chip->fw_mr = >lpc.isa_fw;
> chip->dt_isa_nodename = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
> (uint64_t) PNV_XSCOM_BASE(chip),
> PNV_XSCOM_LPC_BASE);
> @@ -1479,6 +1488,7 @@ static void pnv_chip_power9_realize(DeviceState *dev,
> Error **errp)
> memory_region_add_subregion(get_system_memory(), PNV9_LPCM_BASE(chip),
> >lpc.xscom_regs);
>
> +chip->fw_mr = >lpc.isa_fw;
> chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
> (uint64_t) PNV9_LPCM_BASE(chip));
>
> @@ -1592,6 +1602,7 @@ static void pnv_chip_power10_realize(DeviceState *dev,
> Error **errp)
> memory_region_add_subregion(get_system_memory(), PNV10_LPCM_BASE(chip),
> >lpc.xscom_regs);
>
> +chip->fw_mr = >lpc.isa_fw;
> chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
> (uint64_t)
> PNV10_LPCM_BASE(chip));
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 11739e397b27..bcbca3db9743 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -824,7 +824,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
> ISABus *isa_bus;
> qemu_irq *irqs;
> qemu_irq_handler handler;
> -PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>
> /* let isa_bus_new() create its own bridge on SysBus otherwise
> * devices specified on the command line won't find the bus and
> @@ -850,11 +849,5 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
>
> isa_bus_irqs(isa_bus, irqs);
>
> -/*
> - * TODO: Map PNOR on the LPC FW address space on demand ?
> - */
> -memory_region_add_subregion(>isa_fw, PNOR_SPI_OFFSET,
> ->pnor->mmio);
> -
> return isa_bus;
> }
--
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 6/7] ppc/pnv: Remove default disablement of the PNOR contents
On Tue, Jan 26, 2021 at 06:10:58PM +0100, Cédric Le Goater wrote:
> On PowerNV systems, the BMC is in charge of mapping the PNOR contents
> on the LPC FW address space using the HIOMAP protocol. Under QEMU, we
> emulate this behavior and we also add an extra control on the flash
> accesses by letting the HIOMAP command handler decide whether the
> memory region is accessible or not depending on the firmware requests.
>
> However, this behavior is not compatible with hostboot like firmwares
> which need this mapping to be always available. For this reason, the
> PNOR memory region is initially disabled for skiboot mode only.
>
> This is badly placed under the LPC model and requires the use of the
> machine. Since it doesn't add much, simply remove the initial setting.
> The extra control in the HIOMAP command handler will still be performed.
>
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.0, thanks.
> ---
> hw/ppc/pnv_lpc.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 590359022084..11739e397b27 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -825,7 +825,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
> qemu_irq *irqs;
> qemu_irq_handler handler;
> PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> -bool hostboot_mode = !!pnv->fw_load_addr;
>
> /* let isa_bus_new() create its own bridge on SysBus otherwise
> * devices specified on the command line won't find the bus and
> @@ -856,13 +855,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
> */
> memory_region_add_subregion(>isa_fw, PNOR_SPI_OFFSET,
> >pnor->mmio);
> -/*
> - * Start disabled. The HIOMAP protocol will activate the mapping
> - * with HIOMAP_C_CREATE_WRITE_WINDOW
> - */
> -if (!hostboot_mode) {
> -memory_region_set_enabled(>pnor->mmio, false);
> -}
>
> return isa_bus;
> }
--
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 4/7] ppc/pnv: Simplify pnv_bmc_create()
On Tue, Jan 26, 2021 at 06:10:56PM +0100, Cédric Le Goater wrote: > and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > > Signed-off-by: Cédric Le Goater Applied to ppc-for-6.0, thanks. > --- > hw/ppc/pnv_bmc.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > index 67ebb16c4d5f..86d16b493539 100644 > --- a/hw/ppc/pnv_bmc.c > +++ b/hw/ppc/pnv_bmc.c > @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > Object *obj; > > obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > -object_ref(OBJECT(pnor)); > -object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > qdev_realize(DEVICE(obj), NULL, _fatal); > - > -/* Install the HIOMAP protocol handlers to access the PNOR */ > -ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > -_netfn); > +pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > > return IPMI_BMC(obj); > } -- 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 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external
On Tue, Jan 26, 2021 at 06:10:57PM +0100, Cédric Le Goater wrote:
> The PowerNV machine can be run with an external IPMI BMC device
> connected to a remote QEMU machine acting as BMC, using these options :
>
> -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
> -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
> -device isa-ipmi-bt,bmc=bmc0,irq=10 \
> -nodefaults
>
> In that case, some aspects of the BMC initialization should be
> skipped, since they rely on the simulator interface.
>
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.0, thanks.
> ---
> hw/ppc/pnv_bmc.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> index 86d16b493539..b9bf5735ea0f 100644
> --- a/hw/ppc/pnv_bmc.c
> +++ b/hw/ppc/pnv_bmc.c
> @@ -51,6 +51,11 @@ typedef struct OemSel {
> #define SOFT_OFF0x00
> #define SOFT_REBOOT 0x01
>
> +static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
> +{
> +return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
> +}
> +
> static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
> {
> /* IPMI SEL Event are 16 bytes long */
> @@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
> const struct ipmi_sdr_compact *sdr;
> uint16_t nextrec;
>
> +if (!pnv_bmc_is_simulator(bmc)) {
> +return;
> +}
> +
> offset = fdt_add_subnode(fdt, 0, "bmc");
> _FDT(offset);
>
> @@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
>
> void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
> {
> +if (!pnv_bmc_is_simulator(bmc)) {
> +return;
> +}
> +
> object_ref(OBJECT(pnor));
> object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
>
> @@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
>
> IPMIBmc *pnv_bmc_find(Error **errp)
> {
> -ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
> +ForeachArgs args = { TYPE_IPMI_BMC, NULL };
> int ret;
>
> ret = object_child_foreach_recursive(object_get_root(), bmc_find, );
--
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 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs
On Tue, Jan 26, 2021 at 06:10:55PM +0100, Cédric Le Goater wrote:
> The current settings are useful to load large kernels (with debug) but
> it moves the initrd image in a memory region not protected by
> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> corrupt the initrd.
>
> Cc: Murilo Opsfelder Araujo
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.0, thanks.
> ---
>
> If we want to increase the kernel size limit as commit b45b56baeecd
> ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
> think we should add a machine option.
>
> hw/ppc/pnv.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 14fc9758a973..e500c2e2437e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -65,9 +65,9 @@
> #define FW_MAX_SIZE (16 * MiB)
>
> #define KERNEL_LOAD_ADDR0x2000
> -#define KERNEL_MAX_SIZE (256 * MiB)
> -#define INITRD_LOAD_ADDR0x6000
> -#define INITRD_MAX_SIZE (256 * MiB)
> +#define KERNEL_MAX_SIZE (128 * MiB)
> +#define INITRD_LOAD_ADDR0x2800
> +#define INITRD_MAX_SIZE (128 * MiB)
>
> static const char *pnv_chip_core_typename(const PnvChip *o)
> {
--
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/7] ppc/pnv: Add trace events for PCI event notification
On Tue, Jan 26, 2021 at 06:10:53PM +0100, Cédric Le Goater wrote:
> On POWER9 systems, PHB controllers signal the XIVE interrupt controller
> of a source interrupt notification using a store on a MMIO region. Add
> traces for such events.
>
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.0, thanks.
> ---
> hw/intc/pnv_xive.c | 3 +++
> hw/pci-host/pnv_phb4.c | 3 +++
> hw/intc/trace-events | 3 +++
> hw/pci-host/trace-events | 3 +++
> 4 files changed, 12 insertions(+)
>
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 5f69626b3a8d..ad43483612e5 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -24,6 +24,7 @@
> #include "hw/ppc/xive_regs.h"
> #include "hw/qdev-properties.h"
> #include "hw/ppc/ppc.h"
> +#include "trace.h"
>
> #include
>
> @@ -1319,6 +1320,8 @@ static void pnv_xive_ic_hw_trigger(PnvXive *xive,
> hwaddr addr, uint64_t val)
> uint8_t blk;
> uint32_t idx;
>
> +trace_pnv_xive_ic_hw_trigger(addr, val);
> +
> if (val & XIVE_TRIGGER_END) {
> xive_error(xive, "IC: END trigger at @0x%"HWADDR_PRIx" data
> 0x%"PRIx64,
> addr, val);
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 6328e985f81c..54f57c660a94 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -22,6 +22,7 @@
> #include "hw/irq.h"
> #include "hw/qdev-properties.h"
> #include "qom/object.h"
> +#include "trace.h"
>
> #define phb_error(phb, fmt, ...)\
> qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",\
> @@ -1257,6 +1258,8 @@ static void pnv_phb4_xive_notify(XiveNotifier *xf,
> uint32_t srcno)
> uint64_t data = XIVE_TRIGGER_PQ | offset | srcno;
> MemTxResult result;
>
> +trace_pnv_phb4_xive_notify(notif_port, data);
> +
> address_space_stq_be(_space_memory, notif_port, data,
> MEMTXATTRS_UNSPECIFIED, );
> if (result != MEMTX_OK) {
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 8ed397a0d587..45ddaf48df8e 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -236,3 +236,6 @@ xive_tctx_tm_write(uint64_t offset, unsigned int size,
> uint64_t value) "@0x0x%"P
> xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value)
> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring)
> "found NVT 0x%x/0x%x ring=0x%x"
> xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END
> 0x%x/0x%x @0x0x%"PRIx64
> +
> +# pnv_xive.c
> +pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64"
> val=0x%"PRIx64
> diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
> index d19ca9aef6f7..7d8063ac4212 100644
> --- a/hw/pci-host/trace-events
> +++ b/hw/pci-host/trace-events
> @@ -20,3 +20,6 @@ unin_data_write(uint64_t addr, unsigned len, uint64_t val)
> "write addr 0x%"PRIx6
> unin_data_read(uint64_t addr, unsigned len, uint64_t val) "read addr
> 0x%"PRIx64 " len %d val 0x%"PRIx64
> unin_write(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
> unin_read(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
> +
> +# pnv_phb4.c
> +pnv_phb4_xive_notify(uint64_t notif_port, uint64_t data) "notif=@0x%"PRIx64"
> data=0x%"PRIx64
--
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 2/7] ppc/xive: Add firmware bit when dumping the ENDs
On Tue, Jan 26, 2021 at 06:10:54PM +0100, Cédric Le Goater wrote:
> ENDs allocated by OPAL for the HW thread VPs are tagged as owned by FW.
> Dump the state in 'info pic'.
>
> Signed-off-by: Cédric Le Goater
Applied to ppc-for-6.0, thanks.
> ---
> include/hw/ppc/xive_regs.h | 2 ++
> hw/intc/xive.c | 3 ++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
> index 787969282593..b7fde2354e31 100644
> --- a/include/hw/ppc/xive_regs.h
> +++ b/include/hw/ppc/xive_regs.h
> @@ -236,6 +236,8 @@ typedef struct XiveEND {
> (be32_to_cpu((end)->w0) & END_W0_UNCOND_ESCALATE)
> #define xive_end_is_silent_escalation(end) \
> (be32_to_cpu((end)->w0) & END_W0_SILENT_ESCALATE)
> +#define xive_end_is_firmware(end) \
> +(be32_to_cpu((end)->w0) & END_W0_FIRMWARE)
>
> static inline uint64_t xive_end_qaddr(XiveEND *end)
> {
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index fa8c3d82877f..eeb4e62ba954 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1294,7 +1294,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t
> end_idx, Monitor *mon)
>
> pq = xive_get_field32(END_W1_ESn, end->w1);
>
> -monitor_printf(mon, " %08x %c%c %c%c%c%c%c%c%c prio:%d nvt:%02x/%04x",
> +monitor_printf(mon, " %08x %c%c %c%c%c%c%c%c%c%c prio:%d nvt:%02x/%04x",
> end_idx,
> pq & XIVE_ESB_VAL_P ? 'P' : '-',
> pq & XIVE_ESB_VAL_Q ? 'Q' : '-',
> @@ -1305,6 +1305,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t
> end_idx, Monitor *mon)
> xive_end_is_escalate(end) ? 'e' : '-',
> xive_end_is_uncond_escalation(end) ? 'u' : '-',
> xive_end_is_silent_escalation(end) ? 's' : '-',
> + xive_end_is_firmware(end) ? 'f' : '-',
> priority, nvt_blk, nvt_idx);
>
> if (qaddr_base) {
--
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] target/ppc: Remove unused MMU definitions
On Thu, Jan 28, 2021 at 12:24:01AM +0100, Philippe Mathieu-Daudé wrote:
> Remove these confusing and unused definitions.
>
> Signed-off-by: Philippe Mathieu-Daudé
Applied to ppc-for-6.0.
> ---
> target/ppc/cpu.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2609e4082ed..cb002102888 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2205,9 +2205,6 @@ enum {
> * may be needed for precise access rights control and precise exceptions.
> */
> enum {
> -/* 1 bit to define user level / supervisor access */
> -ACCESS_USER = 0x00,
> -ACCESS_SUPER = 0x01,
> /* Type of instruction that generated the access */
> ACCESS_CODE = 0x10, /* Code fetch access*/
> ACCESS_INT = 0x20, /* Integer load/store access*/
--
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
[PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot
From: Michael Qiu
v4: rebase to latest code
v3: reformat the commit log, remove duplicate content
v2: modify the coredump backtrace within commit log with the newest
qemu with master branch
Currently, if guest has workloads, IO thread will acquire aio_context
lock before do io_submit, it leads to segmentfault when do block commit
after snapshot. Just like below:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f7c7d91f700 (LWP 99907)]
0x5576d0f65aab in bdrv_mirror_top_pwritev at ../block/mirror.c:1437
1437../block/mirror.c: No such file or directory.
(gdb) p s->job
$17 = (MirrorBlockJob *) 0x0
(gdb) p s->stop
$18 = false
(gdb) bt
Switch to qemu main thread:
/lib/../lib64/libpthread.so.0
/lib/../lib64/libpthread.so.0
../util/qemu-thread-posix.c:79
qapi/qapi-commands-block-core.c:346
../qapi/qmp-dispatch.c:110
/lib/../lib64/libglib-2.0.so.0
In IO thread when do bdrv_mirror_top_pwritev, the job is NULL, and stop field
is false, this means the MirrorBDSOpaque "s" object has not been initialized
yet,
and this object is initialized by block_job_create(), but the initialize
process is stuck in acquiring the lock.
The rootcause is that qemu do release/acquire when hold the lock,
at the same time, IO thread get the lock after release stage, and the crash
occured.
Actually, in this situation, job->job.aio_context will not equal to
qemu_get_aio_context(), and will be the same as bs->aio_context,
thus, no need to release the lock, becasue bdrv_root_attach_child()
will not change the context.
This patch fix this issue.
Signed-off-by: Michael Qiu
---
blockjob.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 98ac8af982..51a09f3b60 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -214,13 +214,15 @@ int block_job_add_bdrv(BlockJob *job, const char *name,
BlockDriverState *bs,
BdrvChild *c;
bdrv_ref(bs);
-if (job->job.aio_context != qemu_get_aio_context()) {
+if (bdrv_get_aio_context(bs) != job->job.aio_context &&
+job->job.aio_context != qemu_get_aio_context()) {
aio_context_release(job->job.aio_context);
}
c = bdrv_root_attach_child(bs, name, _job, 0,
job->job.aio_context, perm, shared_perm, job,
errp);
-if (job->job.aio_context != qemu_get_aio_context()) {
+if (bdrv_get_aio_context(bs) != job->job.aio_context &&
+job->job.aio_context != qemu_get_aio_context()) {
aio_context_acquire(job->job.aio_context);
}
if (c == NULL) {
--
2.22.0
Re: [PATCH 7/7] ppc/pnv: Introduce a LPC FW memory region attribute to map the PNOR
On Tue, 26 Jan 2021 at 17:19, Cédric Le Goater wrote:
>
> This to map the PNOR from the machine init handler directly and finish
> the cleanup of the LPC model.
>
> Signed-off-by: Cédric Le Goater
Reviewed-by: Joel Stanley
> ---
> include/hw/ppc/pnv.h | 1 +
> hw/ppc/pnv.c | 11 +++
> hw/ppc/pnv_lpc.c | 7 ---
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index ee7eda3e0102..d69cee17b232 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -58,6 +58,7 @@ struct PnvChip {
> MemoryRegion xscom;
> AddressSpace xscom_as;
>
> +MemoryRegion *fw_mr;
> gchar*dt_isa_nodename;
> };
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e500c2e2437e..50810df83815 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -871,6 +871,14 @@ static void pnv_init(MachineState *machine)
> pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
> }
>
> +/*
> + * The PNOR is mapped on the LPC FW address space by the BMC.
> + * Since we can not reach the remote BMC machine with LPC memops,
> + * map it always for now.
> + */
> +memory_region_add_subregion(pnv->chips[0]->fw_mr, PNOR_SPI_OFFSET,
> +>pnor->mmio);
> +
> /*
> * OpenPOWER systems use a IPMI SEL Event message to notify the
> * host to powerdown
> @@ -1150,6 +1158,7 @@ static void pnv_chip_power8_realize(DeviceState *dev,
> Error **errp)
> qdev_realize(DEVICE(>lpc), NULL, _fatal);
> pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE,
> >lpc.xscom_regs);
>
> +chip->fw_mr = >lpc.isa_fw;
> chip->dt_isa_nodename = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
> (uint64_t) PNV_XSCOM_BASE(chip),
> PNV_XSCOM_LPC_BASE);
> @@ -1479,6 +1488,7 @@ static void pnv_chip_power9_realize(DeviceState *dev,
> Error **errp)
> memory_region_add_subregion(get_system_memory(), PNV9_LPCM_BASE(chip),
> >lpc.xscom_regs);
>
> +chip->fw_mr = >lpc.isa_fw;
> chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
> (uint64_t) PNV9_LPCM_BASE(chip));
>
> @@ -1592,6 +1602,7 @@ static void pnv_chip_power10_realize(DeviceState *dev,
> Error **errp)
> memory_region_add_subregion(get_system_memory(), PNV10_LPCM_BASE(chip),
> >lpc.xscom_regs);
>
> +chip->fw_mr = >lpc.isa_fw;
> chip->dt_isa_nodename = g_strdup_printf("/lpcm-opb@%" PRIx64 "/lpc@0",
> (uint64_t)
> PNV10_LPCM_BASE(chip));
> }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 11739e397b27..bcbca3db9743 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -824,7 +824,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
> ISABus *isa_bus;
> qemu_irq *irqs;
> qemu_irq_handler handler;
> -PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>
> /* let isa_bus_new() create its own bridge on SysBus otherwise
> * devices specified on the command line won't find the bus and
> @@ -850,11 +849,5 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
>
> isa_bus_irqs(isa_bus, irqs);
>
> -/*
> - * TODO: Map PNOR on the LPC FW address space on demand ?
> - */
> -memory_region_add_subregion(>isa_fw, PNOR_SPI_OFFSET,
> ->pnor->mmio);
> -
> return isa_bus;
> }
> --
> 2.26.2
>
>
Re: [PATCH 6/7] ppc/pnv: Remove default disablement of the PNOR contents
On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater wrote:
>
> On PowerNV systems, the BMC is in charge of mapping the PNOR contents
> on the LPC FW address space using the HIOMAP protocol. Under QEMU, we
> emulate this behavior and we also add an extra control on the flash
> accesses by letting the HIOMAP command handler decide whether the
> memory region is accessible or not depending on the firmware requests.
>
> However, this behavior is not compatible with hostboot like firmwares
> which need this mapping to be always available. For this reason, the
> PNOR memory region is initially disabled for skiboot mode only.
>
> This is badly placed under the LPC model and requires the use of the
> machine. Since it doesn't add much, simply remove the initial setting.
> The extra control in the HIOMAP command handler will still be performed.
>
> Signed-off-by: Cédric Le Goater
Reviewed-by: Joel Stanley
> ---
> hw/ppc/pnv_lpc.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 590359022084..11739e397b27 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -825,7 +825,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
> qemu_irq *irqs;
> qemu_irq_handler handler;
> PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> -bool hostboot_mode = !!pnv->fw_load_addr;
>
> /* let isa_bus_new() create its own bridge on SysBus otherwise
> * devices specified on the command line won't find the bus and
> @@ -856,13 +855,6 @@ ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool
> use_cpld, Error **errp)
> */
> memory_region_add_subregion(>isa_fw, PNOR_SPI_OFFSET,
> >pnor->mmio);
> -/*
> - * Start disabled. The HIOMAP protocol will activate the mapping
> - * with HIOMAP_C_CREATE_WRITE_WINDOW
> - */
> -if (!hostboot_mode) {
> -memory_region_set_enabled(>pnor->mmio, false);
> -}
>
> return isa_bus;
> }
> --
> 2.26.2
>
>
Re: [PATCH 5/7] ppc/pnv: Discard internal BMC initialization when BMC is external
On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater wrote:
>
> The PowerNV machine can be run with an external IPMI BMC device
> connected to a remote QEMU machine acting as BMC, using these options :
>
> -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
> -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
> -device isa-ipmi-bt,bmc=bmc0,irq=10 \
> -nodefaults
Should this information also go in docs/system/ppc similar to the
descriptions we have in docs/system/arm?
>
> In that case, some aspects of the BMC initialization should be
> skipped, since they rely on the simulator interface.
>
> Signed-off-by: Cédric Le Goater
Reviewed-by: Joel Stanley
> ---
> hw/ppc/pnv_bmc.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> index 86d16b493539..b9bf5735ea0f 100644
> --- a/hw/ppc/pnv_bmc.c
> +++ b/hw/ppc/pnv_bmc.c
> @@ -51,6 +51,11 @@ typedef struct OemSel {
> #define SOFT_OFF0x00
> #define SOFT_REBOOT 0x01
>
> +static bool pnv_bmc_is_simulator(IPMIBmc *bmc)
> +{
> +return object_dynamic_cast(OBJECT(bmc), TYPE_IPMI_BMC_SIMULATOR);
> +}
> +
> static void pnv_gen_oem_sel(IPMIBmc *bmc, uint8_t reboot)
> {
> /* IPMI SEL Event are 16 bytes long */
> @@ -79,6 +84,10 @@ void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt)
> const struct ipmi_sdr_compact *sdr;
> uint16_t nextrec;
>
> +if (!pnv_bmc_is_simulator(bmc)) {
> +return;
> +}
> +
> offset = fdt_add_subnode(fdt, 0, "bmc");
> _FDT(offset);
>
> @@ -243,6 +252,10 @@ static const IPMINetfn hiomap_netfn = {
>
> void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
> {
> +if (!pnv_bmc_is_simulator(bmc)) {
> +return;
> +}
> +
> object_ref(OBJECT(pnor));
> object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor));
>
> @@ -286,7 +299,7 @@ static int bmc_find(Object *child, void *opaque)
>
> IPMIBmc *pnv_bmc_find(Error **errp)
> {
> -ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
> +ForeachArgs args = { TYPE_IPMI_BMC, NULL };
> int ret;
>
> ret = object_child_foreach_recursive(object_get_root(), bmc_find, );
> --
> 2.26.2
>
>
Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()
On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater wrote: > > and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > > Signed-off-by: Cédric Le Goater > --- > hw/ppc/pnv_bmc.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > index 67ebb16c4d5f..86d16b493539 100644 > --- a/hw/ppc/pnv_bmc.c > +++ b/hw/ppc/pnv_bmc.c > @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > Object *obj; > > obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > -object_ref(OBJECT(pnor)); > -object_property_add_const_link(obj, "pnor", OBJECT(pnor)); I assume it's ok to move the link set to after the realise of the BMC object? > qdev_realize(DEVICE(obj), NULL, _fatal); > - > -/* Install the HIOMAP protocol handlers to access the PNOR */ > -ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > -_netfn); > +pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > > return IPMI_BMC(obj); > } > -- > 2.26.2 > >
Re: [PATCH 3/7] ppc/pnv: Use skiboot addresses to load kernel and ramfs
On Tue, 26 Jan 2021 at 17:11, Cédric Le Goater wrote:
>
> The current settings are useful to load large kernels (with debug) but
> it moves the initrd image in a memory region not protected by
> skiboot. If skiboot is compiled with DEBUG=1, memory poisoning will
> corrupt the initrd.
>
> Cc: Murilo Opsfelder Araujo
> Signed-off-by: Cédric Le Goater
Reviewed-by: Joel Stanley
Note that the machine's default ram size will change with this patch:
mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
So we will go from 1.75GB to 768MB. Does anything break when the
machine has less than 1GB of ram?
> ---
>
> If we want to increase the kernel size limit as commit b45b56baeecd
> ("ppc/pnv: increase kernel size limit to 256MiB") intented to do, I
> think we should add a machine option.
>
> hw/ppc/pnv.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 14fc9758a973..e500c2e2437e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -65,9 +65,9 @@
> #define FW_MAX_SIZE (16 * MiB)
>
> #define KERNEL_LOAD_ADDR0x2000
> -#define KERNEL_MAX_SIZE (256 * MiB)
> -#define INITRD_LOAD_ADDR0x6000
> -#define INITRD_MAX_SIZE (256 * MiB)
> +#define KERNEL_MAX_SIZE (128 * MiB)
> +#define INITRD_LOAD_ADDR0x2800
> +#define INITRD_MAX_SIZE (128 * MiB)
>
> static const char *pnv_chip_core_typename(const PnvChip *o)
> {
> --
> 2.26.2
>
>
Re: [PATCH v4 00/12] Support disabling TCG on ARM (part 2)
Philippe Mathieu-Daudé writes:
> Cover from Samuel Ortiz from (part 1) [1]:
>
> This patchset allows for building and running ARM targets with TCG
> disabled. [...]
>
> The rationale behind this work comes from the NEMU project where we're
> trying to only support x86 and ARM 64-bit architectures, without
> including the TCG code base. We can only do so if we can build and run
> ARM binaries with TCG disabled.
>
> v4 almost 2 years later... [2]:
> - Rebased on Meson
> - Addressed Richard review comments
> - Addressed Claudio review comments
Have you re-based recently because I was having a look but ran into
merge conflicts. I'd like to get the merged at some point because I ran
into similar issues with the Xen only build without TCG.
>
> v3 almost 18 months later [3]:
> - Rebased
> - Addressed Thomas review comments
> - Added Travis-CI job to keep building --disable-tcg on ARM
>
> v2 [4]:
> - Addressed review comments from Richard and Thomas from v1 [1]
>
> Regards,
>
> Phil.
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02451.html
> [2]: https://www.mail-archive.com/qemu-devel@nongnu.org/msg689168.html
> [3]: https://www.mail-archive.com/qemu-devel@nongnu.org/msg641796.html
> [4]: https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg05003.html
>
> Green CI:
> - https://cirrus-ci.com/build/4572961761918976
> - https://gitlab.com/philmd/qemu/-/pipelines/196047779
> - https://travis-ci.org/github/philmd/qemu/builds/731370972
>
> Based-on: <20200929125609.1088330-1-phi...@redhat.com>
> "hw/arm: Restrict APEI tables generation to the 'virt' machine"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg745792.html
>
> Philippe Mathieu-Daudé (10):
> accel/tcg: Add stub for cpu_loop_exit()
> meson: Allow optional target/${ARCH}/Kconfig
> target/arm: Select SEMIHOSTING if TCG is available
> target/arm: Restrict ARMv4 cpus to TCG accel
> target/arm: Restrict ARMv5 cpus to TCG accel
> target/arm: Restrict ARMv6 cpus to TCG accel
> target/arm: Restrict ARMv7 R-profile cpus to TCG accel
> target/arm: Restrict ARMv7 M-profile cpus to TCG accel
> target/arm: Reorder meson.build rules
> .travis.yml: Add a KVM-only Aarch64 job
>
> Samuel Ortiz (1):
> target/arm: Do not build TCG objects when TCG is off
>
> Thomas Huth (1):
> target/arm: Make m_helper.c optional via CONFIG_ARM_V7M
>
> default-configs/arm-softmmu.mak | 3 --
> meson.build | 8 +++-
> target/arm/cpu.h| 12 --
> accel/stubs/tcg-stub.c | 5 +++
> target/arm/cpu_tcg.c| 4 +-
> target/arm/helper.c | 7
> target/arm/m_helper-stub.c | 73 +
> .travis.yml | 35
> hw/arm/Kconfig | 32 +++
> target/arm/Kconfig | 4 ++
> target/arm/meson.build | 40 +++---
> 11 files changed, 184 insertions(+), 39 deletions(-)
> create mode 100644 target/arm/m_helper-stub.c
> create mode 100644 target/arm/Kconfig
--
Alex Bennée
[PATCH] target/moxie: Let moxie_mmu_translate() use MMUAccessType access_type
moxie_mmu_translate() doesn't do much work. Still keep its
prototype similar to the other targets, by taking a MMUAccessType
argument.
Signed-off-by: Philippe Mathieu-Daudé
---
target/moxie/mmu.h| 2 +-
target/moxie/helper.c | 2 +-
target/moxie/mmu.c| 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/target/moxie/mmu.h b/target/moxie/mmu.h
index d80690f4d2f..9fddc336746 100644
--- a/target/moxie/mmu.h
+++ b/target/moxie/mmu.h
@@ -14,6 +14,6 @@ typedef struct {
int moxie_mmu_translate(MoxieMMUResult *res,
CPUMoxieState *env, uint32_t vaddr,
-int rw, int mmu_idx);
+MMUAccessType access_type, int mmu_idx);
#endif
diff --git a/target/moxie/helper.c b/target/moxie/helper.c
index b1919f62b34..0e72b1726e3 100644
--- a/target/moxie/helper.c
+++ b/target/moxie/helper.c
@@ -112,7 +112,7 @@ hwaddr moxie_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
MoxieMMUResult res;
int miss;
-miss = moxie_mmu_translate(, >env, addr, 0, 0);
+miss = moxie_mmu_translate(, >env, addr, MMU_DATA_LOAD, 0);
if (!miss) {
phy = res.phy;
}
diff --git a/target/moxie/mmu.c b/target/moxie/mmu.c
index 87783a36f82..228d4c42ff4 100644
--- a/target/moxie/mmu.c
+++ b/target/moxie/mmu.c
@@ -24,7 +24,7 @@
int moxie_mmu_translate(MoxieMMUResult *res,
CPUMoxieState *env, uint32_t vaddr,
- int rw, int mmu_idx)
+ MMUAccessType access_type, int mmu_idx)
{
/* Perform no translation yet. */
res->phy = vaddr;
--
2.26.2
[PATCH 2/2] target/cris: Let cris_mmu_translate() use MMUAccessType access_type
All callers of cris_mmu_translate() provide a MMUAccessType
type. Let the prototype use it as argument, as it is stricter
than an integer. We can remove the documentation as enum
names are self explicit.
Signed-off-by: Philippe Mathieu-Daudé
---
target/cris/mmu.h | 2 +-
target/cris/mmu.c | 24
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/target/cris/mmu.h b/target/cris/mmu.h
index 9ab1642b964..d57386ec6cd 100644
--- a/target/cris/mmu.h
+++ b/target/cris/mmu.h
@@ -17,6 +17,6 @@ void cris_mmu_init(CPUCRISState *env);
void cris_mmu_flush_pid(CPUCRISState *env, uint32_t pid);
int cris_mmu_translate(struct cris_mmu_result *res,
CPUCRISState *env, uint32_t vaddr,
- int rw, int mmu_idx, int debug);
+ MMUAccessType access_type, int mmu_idx, int debug);
#endif
diff --git a/target/cris/mmu.c b/target/cris/mmu.c
index 294de7dffd5..b574ec6e5b9 100644
--- a/target/cris/mmu.c
+++ b/target/cris/mmu.c
@@ -129,10 +129,10 @@ static void dump_tlb(CPUCRISState *env, int mmu)
}
#endif
-/* rw 0 = read, 1 = write, 2 = exec. */
static int cris_mmu_translate_page(struct cris_mmu_result *res,
- CPUCRISState *env, uint32_t vaddr,
- int rw, int usermode, int debug)
+ CPUCRISState *env, uint32_t vaddr,
+ MMUAccessType access_type,
+ int usermode, int debug)
{
unsigned int vpage;
unsigned int idx;
@@ -151,7 +151,7 @@ static int cris_mmu_translate_page(struct cris_mmu_result
*res,
r_cfg = env->sregs[SFR_RW_MM_CFG];
pid = env->pregs[PR_PID] & 0xff;
-switch (rw) {
+switch (access_type) {
case MMU_INST_FETCH:
rwcause = CRIS_MMU_ERR_EXEC;
mmu = 0;
@@ -219,13 +219,13 @@ static int cris_mmu_translate_page(struct cris_mmu_result
*res,
vaddr, lo, env->pc));
match = 0;
res->bf_vec = vect_base + 2;
-} else if (rw == MMU_DATA_STORE && cfg_w && !tlb_w) {
+} else if (access_type == MMU_DATA_STORE && cfg_w && !tlb_w) {
D(printf("tlb: write protected %x lo=%x pc=%x\n",
vaddr, lo, env->pc));
match = 0;
/* write accesses never go through the I mmu. */
res->bf_vec = vect_base + 3;
-} else if (rw == MMU_INST_FETCH && cfg_x && !tlb_x) {
+} else if (access_type == MMU_INST_FETCH && cfg_x && !tlb_x) {
D(printf("tlb: exec protected %x lo=%x pc=%x\n",
vaddr, lo, env->pc));
match = 0;
@@ -272,9 +272,9 @@ static int cris_mmu_translate_page(struct cris_mmu_result
*res,
D(printf("refill vaddr=%x pc=%x\n", vaddr, env->pc));
}
-D(printf("%s rw=%d mtch=%d pc=%x va=%x vpn=%x tlbvpn=%x pfn=%x pid=%x"
+D(printf("%s access=%u mtch=%d pc=%x va=%x vpn=%x tlbvpn=%x pfn=%x pid=%x"
" %x cause=%x sel=%x sp=%x %x %x\n",
- __func__, rw, match, env->pc,
+ __func__, access_type, match, env->pc,
vaddr, vpage,
tlb_vpn, tlb_pfn, tlb_pid,
pid,
@@ -319,8 +319,8 @@ void cris_mmu_flush_pid(CPUCRISState *env, uint32_t pid)
}
int cris_mmu_translate(struct cris_mmu_result *res,
- CPUCRISState *env, uint32_t vaddr,
- int rw, int mmu_idx, int debug)
+ CPUCRISState *env, uint32_t vaddr,
+ MMUAccessType access_type, int mmu_idx, int debug)
{
int seg;
int miss = 0;
@@ -329,7 +329,7 @@ int cris_mmu_translate(struct cris_mmu_result *res,
old_srs = env->pregs[PR_SRS];
-env->pregs[PR_SRS] = rw == MMU_INST_FETCH ? 1 : 2;
+env->pregs[PR_SRS] = access_type == MMU_INST_FETCH ? 1 : 2;
if (!cris_mmu_enabled(env->sregs[SFR_RW_GC_CFG])) {
res->phy = vaddr;
@@ -346,7 +346,7 @@ int cris_mmu_translate(struct cris_mmu_result *res,
res->phy = base | (0x0fff & vaddr);
res->prot = PAGE_BITS;
} else {
-miss = cris_mmu_translate_page(res, env, vaddr, rw,
+miss = cris_mmu_translate_page(res, env, vaddr, access_type,
is_user, debug);
}
done:
--
2.26.2
[PATCH 1/2] target/cris: Use MMUAccessType enum type when possible
Replace the 0/1/2 magic values by the corresponding MMUAccessType.
We can remove a comment as enum names are self explicit.
Signed-off-by: Philippe Mathieu-Daudé
---
target/cris/helper.c | 4 ++--
target/cris/mmu.c| 13 ++---
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/target/cris/helper.c b/target/cris/helper.c
index ed45c3d9b70..cc110f1154d 100644
--- a/target/cris/helper.c
+++ b/target/cris/helper.c
@@ -274,10 +274,10 @@ hwaddr cris_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
struct cris_mmu_result res;
int miss;
-miss = cris_mmu_translate(, >env, addr, 0, 0, 1);
+miss = cris_mmu_translate(, >env, addr, MMU_DATA_LOAD, 0, 1);
/* If D TLB misses, try I TLB. */
if (miss) {
-miss = cris_mmu_translate(, >env, addr, 2, 0, 1);
+miss = cris_mmu_translate(, >env, addr, MMU_INST_FETCH, 0, 1);
}
if (!miss) {
diff --git a/target/cris/mmu.c b/target/cris/mmu.c
index a279b7f1b60..294de7dffd5 100644
--- a/target/cris/mmu.c
+++ b/target/cris/mmu.c
@@ -152,15 +152,15 @@ static int cris_mmu_translate_page(struct cris_mmu_result
*res,
pid = env->pregs[PR_PID] & 0xff;
switch (rw) {
-case 2:
+case MMU_INST_FETCH:
rwcause = CRIS_MMU_ERR_EXEC;
mmu = 0;
break;
-case 1:
+case MMU_DATA_STORE:
rwcause = CRIS_MMU_ERR_WRITE;
break;
default:
-case 0:
+case MMU_DATA_LOAD:
rwcause = CRIS_MMU_ERR_READ;
break;
}
@@ -219,13 +219,13 @@ static int cris_mmu_translate_page(struct cris_mmu_result
*res,
vaddr, lo, env->pc));
match = 0;
res->bf_vec = vect_base + 2;
-} else if (rw == 1 && cfg_w && !tlb_w) {
+} else if (rw == MMU_DATA_STORE && cfg_w && !tlb_w) {
D(printf("tlb: write protected %x lo=%x pc=%x\n",
vaddr, lo, env->pc));
match = 0;
/* write accesses never go through the I mmu. */
res->bf_vec = vect_base + 3;
-} else if (rw == 2 && cfg_x && !tlb_x) {
+} else if (rw == MMU_INST_FETCH && cfg_x && !tlb_x) {
D(printf("tlb: exec protected %x lo=%x pc=%x\n",
vaddr, lo, env->pc));
match = 0;
@@ -329,8 +329,7 @@ int cris_mmu_translate(struct cris_mmu_result *res,
old_srs = env->pregs[PR_SRS];
-/* rw == 2 means exec, map the access to the insn mmu. */
-env->pregs[PR_SRS] = rw == 2 ? 1 : 2;
+env->pregs[PR_SRS] = rw == MMU_INST_FETCH ? 1 : 2;
if (!cris_mmu_enabled(env->sregs[SFR_RW_GC_CFG])) {
res->phy = vaddr;
--
2.26.2
[PATCH 0/2] target/cris: Pass MMUAccessType to cris_mmu_translate()
Taking notes while reviewing commit 671a0a1265a
("use MMUAccessType instead of int in mmu_translate").
Philippe Mathieu-Daudé (2):
target/cris: Use MMUAccessType enum type when possible
target/cris: Let cris_mmu_translate() use MMUAccessType access_type
target/cris/mmu.h| 2 +-
target/cris/helper.c | 4 ++--
target/cris/mmu.c| 31 +++
3 files changed, 18 insertions(+), 19 deletions(-)
--
2.26.2
[PATCH 2/2] target/nios2: Use MMUAccessType enum type when possible
All callers of mmu_translate() provide it a MMUAccessType
type. Let the prototype use it as argument, as it is stricter
than an integer. We can remove the documentation as enum
names are self explicit.
Signed-off-by: Philippe Mathieu-Daudé
---
target/nios2/mmu.h | 3 ++-
target/nios2/mmu.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/target/nios2/mmu.h b/target/nios2/mmu.h
index 4f46fbb82e2..2f4e325a81e 100644
--- a/target/nios2/mmu.h
+++ b/target/nios2/mmu.h
@@ -43,7 +43,8 @@ typedef struct Nios2MMULookup {
void mmu_flip_um(CPUNios2State *env, unsigned int um);
unsigned int mmu_translate(CPUNios2State *env,
Nios2MMULookup *lu,
- target_ulong vaddr, int rw, int mmu_idx);
+ target_ulong vaddr,
+ MMUAccessType access_type, int mmu_idx);
void mmu_read_debug(CPUNios2State *env, uint32_t rn);
void mmu_write(CPUNios2State *env, uint32_t rn, uint32_t v);
void mmu_init(CPUNios2State *env);
diff --git a/target/nios2/mmu.c b/target/nios2/mmu.c
index 2545c06761f..ebe13b89f68 100644
--- a/target/nios2/mmu.c
+++ b/target/nios2/mmu.c
@@ -55,10 +55,10 @@ void mmu_read_debug(CPUNios2State *env, uint32_t rn)
}
}
-/* rw - 0 = read, 1 = write, 2 = fetch. */
unsigned int mmu_translate(CPUNios2State *env,
Nios2MMULookup *lu,
- target_ulong vaddr, int rw, int mmu_idx)
+ target_ulong vaddr,
+ MMUAccessType access_type, int mmu_idx)
{
Nios2CPU *cpu = env_archcpu(env);
int pid = (env->mmu.tlbmisc_wr & CR_TLBMISC_PID_MASK) >> 4;
--
2.26.2
[PATCH 1/2] target/nios2: Replace magic value by MMU definitions
cpu_get_phys_page_debug() uses 'DATA LOAD' MMU access type.
The first MMU is the supervisor one.
Signed-off-by: Philippe Mathieu-Daudé
---
target/nios2/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/nios2/helper.c b/target/nios2/helper.c
index 57c97bde3c6..fea34c957d9 100644
--- a/target/nios2/helper.c
+++ b/target/nios2/helper.c
@@ -209,7 +209,7 @@ hwaddr nios2_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
unsigned int hit;
if (cpu->mmu_present && (addr < 0xC000)) {
-hit = mmu_translate(env, , addr, 0, 0);
+hit = mmu_translate(env, , addr, MMU_DATA_LOAD, MMU_SUPERVISOR_IDX);
if (hit) {
vaddr = addr & TARGET_PAGE_MASK;
paddr = lu.paddr + vaddr - lu.vaddr;
--
2.26.2
[PATCH 0/2] target/nios2: Pass MMUAccessType to mmu_translate()
Taking notes while reviewing commit 671a0a1265a
("use MMUAccessType instead of int in mmu_translate").
Philippe Mathieu-Daudé (2):
target/nios2: Replace magic value by MMU definitions
target/nios2: Use MMUAccessType enum type when possible
target/nios2/mmu.h| 3 ++-
target/nios2/helper.c | 2 +-
target/nios2/mmu.c| 4 ++--
3 files changed, 5 insertions(+), 4 deletions(-)
--
2.26.2
Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
On Wed, Jan 27, 2021 at 01:59:07PM -0800, Hao Wu wrote:
> On Wed, Jan 27, 2021 at 1:42 PM Corey Minyard wrote:
>
> > On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> > > On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard wrote:
> > >
> > > > On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > > > > +
> > > > > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > > > > +{
> > > > > +uint8_t received_bytes =
> > NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > > > > +
> > > > > +if (received_bytes == 0) {
> > > > > +npcm7xx_smbus_recv_fifo(s);
> > > > > +return;
> > > > > +}
> > > > > +
> > > > > +s->sda = s->rx_fifo[s->rx_cur];
> > > > > +s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > > > > +--s->rxf_sts;
> > > >
> > > > This open-coded decrement seems a little risky. Are you sure in every
> > > > case that s->rxf_sts > 0? There's no way what's running in the VM can
> > > > game this and cause a buffer overrun? One caller to this function
> > seems
> > > > to protect against this, and another does not.
> > > >
> > > s->rxf_sts is uint8_t so it's guaranteed to be >=0.
> > > In the case s->rxf_sts == 0, NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
> > > also 0, so it'll take the if-branch and return without running
> > --s->rxf_sts.
> >
> > That is true if called from the
> > NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case. There is no such check
> > in the NPCM7XX_SMBUS_STATUS_RECEIVING case.
> >
> I don't understand the reasoning here. The caller doesn't matter.
> Previous code has:
> #define NPCM7XX_SMBRXF_STS_RX_BYTES(rv) extract8((rv), 0, 5)
> So
> uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> is guaranteed to be 0 if s->rxf_sts == 0.
> As a result the code will take the following branch and returns:
> if (received_bytes == 0) {
> npcm7xx_smbus_recv_fifo(s);
> return;
> }
> And will not execute the --s->rxf_sts sentence.
> Please let me know if I missed anything here.
Ah, sorry, I missed that. Yes, this is ok. So...
Reviewed-by: Corey Minyard
>
> >
> > > I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.
> >
> > You never want to do an assert if the hosted system can do something to
> > cause it. If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
> > case, it would be ok, but really unnecessary.
> >
> > If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
> > but you want to add a comment to that effect if so. These sorts of
> > things look dangerous.
> >
> > There is also the question about who takes these patches in. I'm the
> > I2C maintainer, but there's other code in this series. Once everything
> > is ready, I can ack them if we take it through the ARM tree. Or I can
> > take it through my tree with the proper acks.
> >
> I think either way is fine. Previous NPCM7XX patch series were taken in
> the ARM tree. But as i2c code taking into your tree is also fine.
>
> >
> > -corey
> >
> > >
> > > >
> > > > Other than this, I didn't see any issues with this patch.
> > > >
> > > > -corey
> > > >
> >
RE: [PATCH 0/2] hw/block/nvme: zoned fixes
> -Original Message- > From: Keith Busch > Sent: Wednesday, January 27, 2021 12:42 PM > To: Klaus Jensen > Cc: qemu-devel@nongnu.org; Kevin Wolf ; Max Reitz > ; qemu-bl...@nongnu.org; Dmitry Fomichev > ; Klaus Jensen > Subject: Re: [PATCH 0/2] hw/block/nvme: zoned fixes > > On Tue, Jan 19, 2021 at 02:54:58PM +0100, Klaus Jensen wrote: > > From: Klaus Jensen > > > > Patch [1/2] fixes the zone append bug reported by Niklas. [2/2] > > refactors the zone write check function to return status codes in a > > different order if there are multiple zone write violations that apply. > > Looks good to me. > > Reviewed-by: Keith Busch Besides aesthetics, I don't have any complaints about this series :) Tested-by: Dmitry Fomichev Reviewed-by: Dmitry Fomichev
[PATCH] target/arm: Replace magic value by MMU_DATA_LOAD definition
cpu_get_phys_page_debug() uses 'DATA LOAD' MMU access type.
Signed-off-by: Philippe Mathieu-Daudé
---
target/arm/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d2ead3fcbdb..766ca8b5c78 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12409,7 +12409,7 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs,
vaddr addr,
*attrs = (MemTxAttrs) {};
-ret = get_phys_addr(env, addr, 0, mmu_idx, _addr,
+ret = get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, _addr,
attrs, , _size, , );
if (ret) {
--
2.26.2
[PATCH] target/ppc: Remove unused MMU definitions
Remove these confusing and unused definitions.
Signed-off-by: Philippe Mathieu-Daudé
---
target/ppc/cpu.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2609e4082ed..cb002102888 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2205,9 +2205,6 @@ enum {
* may be needed for precise access rights control and precise exceptions.
*/
enum {
-/* 1 bit to define user level / supervisor access */
-ACCESS_USER = 0x00,
-ACCESS_SUPER = 0x01,
/* Type of instruction that generated the access */
ACCESS_CODE = 0x10, /* Code fetch access*/
ACCESS_INT = 0x20, /* Integer load/store access*/
--
2.26.2
[PATCH 4/5] target/sh4: Let get_physical_address() use MMUAccessType access_type
superh_cpu_tlb_fill() already provides a access_type variable of
type MMUAccessType, and it is passed along, but casted as integer
and renamed 'rw'.
Simply replace 'int rw' by 'MMUAccessType access_type'.
Signed-off-by: Philippe Mathieu-Daudé
---
target/sh4/helper.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 737938d2fd1..fad5f906ef6 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -331,14 +331,14 @@ static int find_utlb_entry(CPUSH4State * env,
target_ulong address, int use_asid
*/
static int get_mmu_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
- int rw, int mmu_idx)
+ MMUAccessType access_type, int mmu_idx)
{
int use_asid, n;
tlb_t *matching = NULL;
use_asid = !(env->mmucr & MMUCR_SV) || !(env->sr & (1u << SR_MD));
-if (rw == MMU_INST_FETCH) {
+if (access_type == MMU_INST_FETCH) {
n = find_itlb_entry(env, address, use_asid);
if (n >= 0) {
matching = >itlb[n];
@@ -371,11 +371,11 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
if (n >= 0) {
matching = >utlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
-n = (rw == MMU_DATA_STORE)
+n = (access_type == MMU_DATA_STORE)
? MMU_DTLB_VIOLATION_WRITE : MMU_DTLB_VIOLATION_READ;
-} else if ((rw == MMU_DATA_STORE) && !(matching->pr & 1)) {
+} else if ((access_type == MMU_DATA_STORE) && !(matching->pr & 1))
{
n = MMU_DTLB_VIOLATION_WRITE;
-} else if ((rw == MMU_DATA_STORE) && !matching->d) {
+} else if ((access_type == MMU_DATA_STORE) && !matching->d) {
n = MMU_DTLB_INITIAL_WRITE;
} else {
*prot = PAGE_READ;
@@ -384,7 +384,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
}
}
} else if (n == MMU_DTLB_MISS) {
-n = (rw == MMU_DATA_STORE)
+n = (access_type == MMU_DATA_STORE)
? MMU_DTLB_MISS_WRITE : MMU_DTLB_MISS_READ;
}
}
@@ -398,7 +398,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
static int get_physical_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
-int rw, int mmu_idx)
+MMUAccessType access_type, int mmu_idx)
{
/* P1, P2 and P4 areas do not use translation */
if ((address >= 0x8000 && address < 0xc000) || address >=
0xe000) {
@@ -406,9 +406,9 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
&& (address < 0xe000 || address >= 0xe400)) {
/* Unauthorized access in user mode (only store queues are
available) */
qemu_log_mask(LOG_GUEST_ERROR, "Unauthorized access\n");
-if (rw == MMU_DATA_LOAD) {
+if (access_type == MMU_DATA_LOAD) {
return MMU_DADDR_ERROR_READ;
-} else if (rw == MMU_DATA_STORE) {
+} else if (access_type == MMU_DATA_STORE) {
return MMU_DADDR_ERROR_WRITE;
} else {
return MMU_IADDR_ERROR;
@@ -432,7 +432,7 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
}
/* We need to resort to the MMU */
-return get_mmu_address(env, physical, prot, address, rw, mmu_idx);
+return get_mmu_address(env, physical, prot, address, access_type, mmu_idx);
}
hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
--
2.26.2
[PATCH 2/5] target/sh4: Replace magic value by MMUAccessType definitions
Replace the 0/1/2 magic values by the corresponding MMUAccessType.
Signed-off-by: Philippe Mathieu-Daudé
---
target/sh4/helper.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index fc816137766..4303ebf018b 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -338,7 +338,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
use_asid = !(env->mmucr & MMUCR_SV) || !(env->sr & (1u << SR_MD));
-if (rw == 2) {
+if (rw == MMU_INST_FETCH) {
n = find_itlb_entry(env, address, use_asid);
if (n >= 0) {
matching = >itlb[n];
@@ -371,11 +371,11 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
if (n >= 0) {
matching = >utlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
-n = (rw == 1)
+n = (rw == MMU_DATA_STORE)
? MMU_DTLB_VIOLATION_WRITE : MMU_DTLB_VIOLATION_READ;
-} else if ((rw == 1) && !(matching->pr & 1)) {
+} else if ((rw == MMU_DATA_STORE) && !(matching->pr & 1)) {
n = MMU_DTLB_VIOLATION_WRITE;
-} else if ((rw == 1) && !matching->d) {
+} else if ((rw == MMU_DATA_STORE) && !matching->d) {
n = MMU_DTLB_INITIAL_WRITE;
} else {
*prot = PAGE_READ;
@@ -384,7 +384,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
}
}
} else if (n == MMU_DTLB_MISS) {
-n = (rw == 1)
+n = (rw == MMU_DATA_STORE)
? MMU_DTLB_MISS_WRITE : MMU_DTLB_MISS_READ;
}
}
@@ -406,9 +406,9 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
&& (address < 0xe000 || address >= 0xe400)) {
/* Unauthorized access in user mode (only store queues are
available) */
qemu_log_mask(LOG_GUEST_ERROR, "Unauthorized access\n");
-if (rw == 0) {
+if (rw == MMU_DATA_LOAD) {
return MMU_DADDR_ERROR_READ;
-} else if (rw == 1) {
+} else if (rw == MMU_DATA_STORE) {
return MMU_DADDR_ERROR_WRITE;
} else {
return MMU_IADDR_ERROR;
@@ -441,7 +441,7 @@ hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
target_ulong physical;
int prot;
-get_physical_address(>env, , , addr, 0, 0);
+get_physical_address(>env, , , addr, MMU_DATA_LOAD, 0);
return physical;
}
--
2.26.2
[PATCH 5/5] target/sh4: Remove unused definitions
Remove these confusing and unused definitions.
Signed-off-by: Philippe Mathieu-Daudé
---
target/sh4/cpu.h | 11 ---
1 file changed, 11 deletions(-)
diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h
index 714e3b56413..01c43440822 100644
--- a/target/sh4/cpu.h
+++ b/target/sh4/cpu.h
@@ -271,17 +271,6 @@ typedef SuperHCPU ArchCPU;
#include "exec/cpu-all.h"
-/* Memory access type */
-enum {
-/* Privilege */
-ACCESS_PRIV = 0x01,
-/* Direction */
-ACCESS_WRITE = 0x02,
-/* Type of instruction */
-ACCESS_CODE = 0x10,
-ACCESS_INT = 0x20
-};
-
/* MMU control register */
#define MMUCR0x1F10
#define MMUCR_AT (1<<0)
--
2.26.2
[PATCH 3/5] target/sh4: Pass mmu_idx to get_physical_address()
get_mmu_address() and get_physical_address() don't use their
'int access_type' argument: remove it along with ACCESS_INT
in superh_cpu_tlb_fill().
Pass the MMU index along, as other targets do.
Signed-off-by: Philippe Mathieu-Daudé
---
target/sh4/helper.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 4303ebf018b..737938d2fd1 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -331,7 +331,7 @@ static int find_utlb_entry(CPUSH4State * env, target_ulong
address, int use_asid
*/
static int get_mmu_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
- int rw, int access_type)
+ int rw, int mmu_idx)
{
int use_asid, n;
tlb_t *matching = NULL;
@@ -398,7 +398,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
static int get_physical_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
-int rw, int access_type)
+int rw, int mmu_idx)
{
/* P1, P2 and P4 areas do not use translation */
if ((address >= 0x8000 && address < 0xc000) || address >=
0xe000) {
@@ -432,7 +432,7 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
}
/* We need to resort to the MMU */
-return get_mmu_address(env, physical, prot, address, rw, access_type);
+return get_mmu_address(env, physical, prot, address, rw, mmu_idx);
}
hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -813,11 +813,10 @@ bool superh_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
MMU_DTLB_VIOLATION_READ);
#else
target_ulong physical;
-int prot, sh_access_type;
+int prot;
-sh_access_type = ACCESS_INT;
ret = get_physical_address(env, , , address,
- access_type, sh_access_type);
+ access_type, mmu_idx);
if (ret == MMU_OK) {
address &= TARGET_PAGE_MASK;
--
2.26.2
[PATCH 1/5] target/sh4: Fix code style for checkpatch.pl
We are going to move this code, fix its style first.
Signed-off-by: Philippe Mathieu-Daudé
---
Easier to review using 'git-diff -w -b'
---
target/sh4/helper.c | 82 ++---
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 408478ce5dc..fc816137766 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -330,8 +330,8 @@ static int find_utlb_entry(CPUSH4State * env, target_ulong
address, int use_asid
MMU_IADDR_ERROR, MMU_DADDR_ERROR_READ, MMU_DADDR_ERROR_WRITE.
*/
static int get_mmu_address(CPUSH4State * env, target_ulong * physical,
- int *prot, target_ulong address,
- int rw, int access_type)
+ int *prot, target_ulong address,
+ int rw, int access_type)
{
int use_asid, n;
tlb_t *matching = NULL;
@@ -340,12 +340,12 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
if (rw == 2) {
n = find_itlb_entry(env, address, use_asid);
- if (n >= 0) {
- matching = >itlb[n];
+if (n >= 0) {
+matching = >itlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
- n = MMU_ITLB_VIOLATION;
+n = MMU_ITLB_VIOLATION;
} else {
- *prot = PAGE_EXEC;
+*prot = PAGE_EXEC;
}
} else {
n = find_utlb_entry(env, address, use_asid);
@@ -365,14 +365,14 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
} else if (n == MMU_DTLB_MISS) {
n = MMU_ITLB_MISS;
}
- }
+}
} else {
- n = find_utlb_entry(env, address, use_asid);
- if (n >= 0) {
- matching = >utlb[n];
+n = find_utlb_entry(env, address, use_asid);
+if (n >= 0) {
+matching = >utlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
-n = (rw == 1) ? MMU_DTLB_VIOLATION_WRITE :
-MMU_DTLB_VIOLATION_READ;
+n = (rw == 1)
+? MMU_DTLB_VIOLATION_WRITE : MMU_DTLB_VIOLATION_READ;
} else if ((rw == 1) && !(matching->pr & 1)) {
n = MMU_DTLB_VIOLATION_WRITE;
} else if ((rw == 1) && !matching->d) {
@@ -383,15 +383,15 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
*prot |= PAGE_WRITE;
}
}
- } else if (n == MMU_DTLB_MISS) {
- n = (rw == 1) ? MMU_DTLB_MISS_WRITE :
- MMU_DTLB_MISS_READ;
- }
+} else if (n == MMU_DTLB_MISS) {
+n = (rw == 1)
+? MMU_DTLB_MISS_WRITE : MMU_DTLB_MISS_READ;
+}
}
if (n >= 0) {
- n = MMU_OK;
- *physical = ((matching->ppn << 10) & ~(matching->size - 1)) |
- (address & (matching->size - 1));
+n = MMU_OK;
+*physical = ((matching->ppn << 10) & ~(matching->size - 1))
+| (address & (matching->size - 1));
}
return n;
}
@@ -401,34 +401,34 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
int rw, int access_type)
{
/* P1, P2 and P4 areas do not use translation */
-if ((address >= 0x8000 && address < 0xc000) ||
- address >= 0xe000) {
+if ((address >= 0x8000 && address < 0xc000) || address >=
0xe000) {
if (!(env->sr & (1u << SR_MD))
- && (address < 0xe000 || address >= 0xe400)) {
- /* Unauthorized access in user mode (only store queues are
available) */
+&& (address < 0xe000 || address >= 0xe400)) {
+/* Unauthorized access in user mode (only store queues are
available) */
qemu_log_mask(LOG_GUEST_ERROR, "Unauthorized access\n");
- if (rw == 0)
- return MMU_DADDR_ERROR_READ;
- else if (rw == 1)
- return MMU_DADDR_ERROR_WRITE;
- else
- return MMU_IADDR_ERROR;
- }
- if (address >= 0x8000 && address < 0xc000) {
- /* Mask upper 3 bits for P1 and P2 areas */
- *physical = address & 0x1fff;
- } else {
- *physical = address;
- }
- *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
- return MMU_OK;
+if (rw == 0) {
+return MMU_DADDR_ERROR_READ;
+} else if (rw == 1) {
+return MMU_DADDR_ERROR_WRITE;
+} else {
+return MMU_IADDR_ERROR;
+}
+}
+if (address >= 0x8000 && address < 0xc000) {
+/* Mask upper 3 bits for P1 and P2 areas */
+*physical = address & 0x1fff;
+} else {
+*physical =
[PATCH 0/5] target/sh4: Pass MMUAccessType to get_physical_address()
Taking notes while reviewing commit 671a0a1265a
("use MMUAccessType instead of int in mmu_translate").
Philippe Mathieu-Daudé (5):
target/sh4: Fix code style for checkpatch.pl
target/sh4: Replace magic value by MMUAccessType definitions
target/sh4: Pass mmu_idx to get_physical_address()
target/sh4: Let get_physical_address() use MMUAccessType access_type
target/sh4: Remove unused definitions
target/sh4/cpu.h| 11 -
target/sh4/helper.c | 99 ++---
2 files changed, 49 insertions(+), 61 deletions(-)
--
2.26.2
Re: [PATCH] tcg/tci: Implement INDEX_op_ld8s_i64
On 1/27/21 11:07 AM, Stefan Weil wrote: > case INDEX_op_ld8s_i64: > -TODO(); > +t0 = *tb_ptr++; > +t1 = tci_read_r(regs, _ptr); > +t2 = tci_read_s32(_ptr); > +tci_write_reg8(regs, t0, *(int8_t *)(t1 + t2)); > break; This doesn't work, because tci_write_reg8 truncates to uint8_t again. You need to use tci_write_reg(). r~
Re: [PATCH] target/i386: Fix decoding of certain BMI instructions
On 14 Jan 2021, David Greenaway wrote: > This patch fixes a translation bug for a subset of x86 BMI instructions > such as the following: [...] Gentle ping. The patch is up at: https://patchwork.kernel.org/project/qemu-devel/patch/20210114063958.1508050-1-dgreena...@google.com/ if that helps. Cheers, David
[PATCH 1/3] target/tricore: Replace magic value by MMU_DATA_LOAD definition
cpu_get_phys_page_debug() uses 'DATA LOAD' MMU access type.
Signed-off-by: Philippe Mathieu-Daudé
---
target/tricore/helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/tricore/helper.c b/target/tricore/helper.c
index 77152932630..81171db833b 100644
--- a/target/tricore/helper.c
+++ b/target/tricore/helper.c
@@ -50,7 +50,8 @@ hwaddr tricore_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
int prot;
int mmu_idx = cpu_mmu_index(>env, false);
-if (get_physical_address(>env, _addr, , addr, 0, mmu_idx)) {
+if (get_physical_address(>env, _addr, , addr,
+ MMU_DATA_LOAD, mmu_idx)) {
return -1;
}
return phys_addr;
--
2.26.2
[PATCH 3/3] target/tricore: Remove unused definitions
Remove these confusing and unused definitions.
Signed-off-by: Philippe Mathieu-Daudé
---
target/tricore/cpu.h | 12
1 file changed, 12 deletions(-)
diff --git a/target/tricore/cpu.h b/target/tricore/cpu.h
index b82349d1b10..4b61a2c03f8 100644
--- a/target/tricore/cpu.h
+++ b/target/tricore/cpu.h
@@ -375,18 +375,6 @@ typedef TriCoreCPU ArchCPU;
#include "exec/cpu-all.h"
-enum {
-/* 1 bit to define user level / supervisor access */
-ACCESS_USER = 0x00,
-ACCESS_SUPER = 0x01,
-/* 1 bit to indicate direction */
-ACCESS_STORE = 0x02,
-/* Type of instruction that generated the access */
-ACCESS_CODE = 0x10, /* Code fetch access*/
-ACCESS_INT = 0x20, /* Integer load/store access*/
-ACCESS_FLOAT = 0x30, /* floating point load/store access */
-};
-
void cpu_state_reset(CPUTriCoreState *s);
void tricore_tcg_init(void);
int cpu_tricore_signal_handler(int host_signum, void *pinfo, void *puc);
--
2.26.2
[PATCH 2/3] target/tricore: Pass MMUAccessType to get_physical_address()
'int access_type' and ACCESS_INT are unused, drop them.
Provide the mmu_idx argument to match other targets.
'int rw' is actually the MMUAccessType, rename it.
Signed-off-by: Philippe Mathieu-Daudé
---
target/tricore/helper.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/tricore/helper.c b/target/tricore/helper.c
index 81171db833b..c5e997f3215 100644
--- a/target/tricore/helper.c
+++ b/target/tricore/helper.c
@@ -33,7 +33,7 @@ enum {
#if defined(CONFIG_SOFTMMU)
static int get_physical_address(CPUTriCoreState *env, hwaddr *physical,
int *prot, target_ulong address,
-int rw, int access_type)
+MMUAccessType access_type, int mmu_idx)
{
int ret = TLBRET_MATCH;
@@ -72,13 +72,11 @@ bool tricore_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
CPUTriCoreState *env = >env;
hwaddr physical;
int prot;
-int access_type;
int ret = 0;
rw &= 1;
-access_type = ACCESS_INT;
ret = get_physical_address(env, , ,
- address, rw, access_type);
+ address, rw, mmu_idx);
qemu_log_mask(CPU_LOG_MMU, "%s address=" TARGET_FMT_lx " ret %d physical "
TARGET_FMT_plx " prot %d\n",
--
2.26.2
[PATCH 0/3] target/tricore: Pass MMUAccessType to get_physical_address()
Taking notes while reviewing commit 671a0a1265a
("use MMUAccessType instead of int in mmu_translate").
Philippe Mathieu-Daudé (3):
target/tricore: Replace magic value by MMU_DATA_LOAD definition
target/tricore: Pass MMUAccessType to get_physical_address()
target/tricore: Remove unused definitions
target/tricore/cpu.h| 12
target/tricore/helper.c | 9 -
2 files changed, 4 insertions(+), 17 deletions(-)
--
2.26.2
Re: [PATCH 17/25] hw/arm/stellaris: Create Clock input for watchdog
On 1/21/21 10:59 PM, Philippe Mathieu-Daudé wrote:
> On 1/21/21 8:06 PM, Peter Maydell wrote:
>> Create and connect the Clock input for the watchdog device on the
>> Stellaris boards. Because the Stellaris boards model the ability to
>> change the clock rate by programming PLL registers, we have to create
>> an output Clock on the ssys_state device and wire it up to the
>> watchdog.
>>
>> Note that the old comment on ssys_calculate_system_clock() got the
>> units wrong -- system_clock_scale is in nanoseconds, not
>> milliseconds. Improve the commentary to clarify how we are
>> calculating the period.
>>
>> Signed-off-by: Peter Maydell
>> ---
>> hw/arm/stellaris.c | 43 +++
>> 1 file changed, 31 insertions(+), 12 deletions(-)
> ...
>
>> /*
>> - * Caculate the sys. clock period in ms.
>> + * Calculate the system clock period. We only want to propagate
>> + * this change to the rest of the system if we're not being called
>> + * from migration post-load.
>
> This part was not trivial to understand. I read the Clock API
> doc again then found:
>
> Care should be taken not to use ``clock_update[_ns|_hz]()`` or
> ``clock_propagate()`` during the whole migration procedure because it
> will trigger side effects to other devices in an unknown state.
>
>> */
>> -static void ssys_calculate_system_clock(ssys_state *s)
>> +static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock)
>> {
>> +/*
>> + * SYSDIV field specifies divisor: 0 == /1, 1 == /2, etc. Input
>> + * clock is 200MHz, which is a period of 5 ns. Dividing the clock
>> + * frequency by X is the same as multiplying the period by X.
>> + */
>> if (ssys_use_rcc2(s)) {
>> system_clock_scale = 5 * (((s->rcc2 >> 23) & 0x3f) + 1);
>> } else {
>> system_clock_scale = 5 * (((s->rcc >> 23) & 0xf) + 1);
>> }
>> +clock_set_ns(s->sysclk, system_clock_scale);
>> +if (propagate_clock) {
>> +clock_propagate(s->sysclk);
>> +}
>> }
> ...
>> static void stellaris_sys_reset_exit(Object *obj)
>> @@ -690,7 +704,7 @@ static int stellaris_sys_post_load(void *opaque, int
>> version_id)
>> {
>> ssys_state *s = opaque;
>>
>> -ssys_calculate_system_clock(s);
>> +ssys_calculate_system_clock(s, false);
>
> So this makes sense.
>
> I'll keep reviewing and come back to this patch later.
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v4 1/2] drivers/misc: sysgenid: add system generation id driver
Hi! > - Solution > > The System Generation ID is a simple concept meant to alleviate the > issue by providing a monotonically increasing u32 counter that changes > each time the VM or container is restored from a snapshot. I'd make it u64. But as people explained, this has race problems that may be impossible to solve? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek signature.asc Description: Digital signature
Re: [PATCH 25/25] hw/arm/stellaris: Remove board-creation reset of STELLARIS_SYS
On 1/21/21 8:06 PM, Peter Maydell wrote: > Now that the watchdog device uses its Clock input rather than being > passed the value of system_clock_scale at creation time, we can > remove the hack where we reset the STELLARIS_SYS at board creation > time to force it to set system_clock_scale. Instead it will be reset > at the usual point in startup and will inform the watchdog of the > clock frequency at that point. > > Signed-off-by: Peter Maydell > --- > hw/arm/stellaris.c | 10 -- > 1 file changed, 10 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 16/25] hw/arm/stellaris: Convert SSYS to QOM device
On 1/25/21 12:48 PM, Peter Maydell wrote:
> On Thu, 21 Jan 2021 at 22:13, Philippe Mathieu-Daudé wrote:
>> On 1/21/21 8:06 PM, Peter Maydell wrote:
>>> Convert the SSYS code in the Stellaris boards (which encapsulates the
>>> system registers) to a proper QOM device. This will provide us with
>>> somewhere to put the output Clock whose frequency depends on the
>>> setting of the PLL configuration registers.
>>>
>>> This is a migration compatibility break for lm3s811evb, lm3s6965evb.
>>>
>>> We use 3-phase reset here because the Clock will need to propagate
>>> its value in the hold phase.
>>>
>>> For the moment we reset the device during the board creation so that
>>> the system_clock_scale global gets set; this will be removed in a
>>> subsequent commit.
>
>>> +
>>> +struct ssys_state {
>>> +SysBusDevice parent_obj;
>>> +
>>> MemoryRegion iomem;
>>> uint32_t pborctl;
>>> uint32_t ldopctl;
>>> @@ -371,11 +376,18 @@ typedef struct {
>>> uint32_t dcgc[3];
>>> uint32_t clkvclr;
>>> uint32_t ldoarst;
>>> +qemu_irq irq;
>>> +/* Properties (all read-only registers) */
>>> uint32_t user0;
>>> uint32_t user1;
>>> -qemu_irq irq;
>>> -stellaris_board_info *board;
>>> -} ssys_state;
>>> +uint32_t did0;
>>> +uint32_t did1;
>>> +uint32_t dc0;
>>> +uint32_t dc1;
>>> +uint32_t dc2;
>>> +uint32_t dc3;
>>> +uint32_t dc4;
>>
>> Shouldn't these be class properties?
>
> Could you elaborate on what you think the code ought to look like?
I am thinking something similar how Igor asked me to implement
RaspiMachineClass::board_rev in hw/arm/raspi.c, as the did/dc registers
are read-only. Anyhow this is 1/ probably not necessary and 2/ out of
the scope of this series, this patch is already complex enough, and
the work is done.
> I just used the usual thing of defining uint32 qdev properties so we
> can set these values when we create the device, as a replacement
> for the existing code which either reaches directly into the
> state struct to set the user0/user1 values or sets the
> stellaris_board_info pointer in the state struct.
No problem.
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 19/25] hw/timer/cmsdk-apb-dualtimer: Convert to use Clock input
On 1/21/21 8:06 PM, Peter Maydell wrote:
> Switch the CMSDK APB dualtimer device over to using its Clock input;
> the pclk-frq property is now ignored.
>
> Signed-off-by: Peter Maydell
> ---
> hw/timer/cmsdk-apb-dualtimer.c | 42 ++
> 1 file changed, 37 insertions(+), 5 deletions(-)
...
> @@ -454,8 +486,8 @@ static void cmsdk_apb_dualtimer_realize(DeviceState *dev,
> Error **errp)
> CMSDKAPBDualTimer *s = CMSDK_APB_DUALTIMER(dev);
> int i;
>
> -if (s->pclk_frq == 0) {
> -error_setg(errp, "CMSDK APB timer: pclk-frq property must be set");
> +if (!clock_has_source(s->timclk)) {
> +error_setg(errp, "CMSDK APB dualtimer: TIMCLK clock must be
> connected");
> return;
> }
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode
On Wed, Jan 27, 2021 at 1:42 PM Corey Minyard wrote:
> On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> > On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard wrote:
> >
> > > On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > > > +
> > > > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > > > +{
> > > > +uint8_t received_bytes =
> NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > > > +
> > > > +if (received_bytes == 0) {
> > > > +npcm7xx_smbus_recv_fifo(s);
> > > > +return;
> > > > +}
> > > > +
> > > > +s->sda = s->rx_fifo[s->rx_cur];
> > > > +s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > > > +--s->rxf_sts;
> > >
> > > This open-coded decrement seems a little risky. Are you sure in every
> > > case that s->rxf_sts > 0? There's no way what's running in the VM can
> > > game this and cause a buffer overrun? One caller to this function
> seems
> > > to protect against this, and another does not.
> > >
> > s->rxf_sts is uint8_t so it's guaranteed to be >=0.
> > In the case s->rxf_sts == 0, NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
> > also 0, so it'll take the if-branch and return without running
> --s->rxf_sts.
>
> That is true if called from the
> NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case. There is no such check
> in the NPCM7XX_SMBUS_STATUS_RECEIVING case.
>
I don't understand the reasoning here. The caller doesn't matter.
Previous code has:
#define NPCM7XX_SMBRXF_STS_RX_BYTES(rv) extract8((rv), 0, 5)
So
uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
is guaranteed to be 0 if s->rxf_sts == 0.
As a result the code will take the following branch and returns:
if (received_bytes == 0) {
npcm7xx_smbus_recv_fifo(s);
return;
}
And will not execute the --s->rxf_sts sentence.
Please let me know if I missed anything here.
>
> > I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.
>
> You never want to do an assert if the hosted system can do something to
> cause it. If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
> case, it would be ok, but really unnecessary.
>
> If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
> but you want to add a comment to that effect if so. These sorts of
> things look dangerous.
>
> There is also the question about who takes these patches in. I'm the
> I2C maintainer, but there's other code in this series. Once everything
> is ready, I can ack them if we take it through the ARM tree. Or I can
> take it through my tree with the proper acks.
>
I think either way is fine. Previous NPCM7XX patch series were taken in
the ARM tree. But as i2c code taking into your tree is also fine.
>
> -corey
>
> >
> > >
> > > Other than this, I didn't see any issues with this patch.
> > >
> > > -corey
> > >
>
Re: [PATCH 02/25] clock: Add new clock_has_source() function
On 1/21/21 8:05 PM, Peter Maydell wrote: > Add a function for checking whether a clock has a source. This is > useful for devices which have input clocks that must be wired up by > the board as it allows them to fail in realize rather than ploughing > on with a zero-period clock. > > Signed-off-by: Peter Maydell > --- > docs/devel/clocks.rst | 16 > include/hw/clock.h| 15 +++ > 2 files changed, 31 insertions(+) Reviewed-by: Philippe Mathieu-Daudé
Re: [RFC] Change default ipv6 network from fec0/10 (site local) to fe80/10 (link local)
Hello, Philippe Mathieu-Daudé, le mer. 27 janv. 2021 22:46:13 +0100, a ecrit: > On 1/27/21 8:13 PM, Doug Evans wrote: > > I happened to notice QEMU's default for the ipv6 network is fec0::/10 > > which is deprecated (RFC3879). > > I think(!) an obvious replacement is fe80::/10, link local. fe80::/10 is really a different thing, I don't think we want to use it. We can use some prefix in fc00::/7, such as fd00::/8. It "just" needs checking with various guest OS, to check that it doesn't break the default behavior. Samuel
Re: [PATCH 04/25] tests: Add a simple test of the CMSDK APB watchdog
On 1/21/21 8:06 PM, Peter Maydell wrote: > Add a simple test of the CMSDK watchdog, since we're about to do some > refactoring of how it is clocked. > > Signed-off-by: Peter Maydell > --- > tests/qtest/cmsdk-apb-watchdog-test.c | 80 +++ > MAINTAINERS | 1 + > tests/qtest/meson.build | 1 + > 3 files changed, 82 insertions(+) > create mode 100644 tests/qtest/cmsdk-apb-watchdog-test.c Reviewed-by: Philippe Mathieu-Daudé
Re: [RFC PATCH v2 1/3] vfio: Move the saving of the config space to the right place in VFIO migration
On 1/27/2021 3:06 AM, Alex Williamson wrote: On Thu, 10 Dec 2020 10:21:21 +0800 Shenming Lu wrote: On 2020/12/10 2:34, Alex Williamson wrote: On Wed, 9 Dec 2020 13:29:47 +0100 Cornelia Huck wrote: On Wed, 9 Dec 2020 16:09:17 +0800 Shenming Lu wrote: On ARM64 the VFIO SET_IRQS ioctl is dependent on the VM interrupt setup, if the restoring of the VFIO PCI device config space is before the VGIC, an error might occur in the kernel. So we move the saving of the config space to the non-iterable process, so that it will be called after the VGIC according to their priorities. As for the possible dependence of the device specific migration data on it's config space, we can let the vendor driver to include any config info it needs in its own data stream. (Should we note this in the header file linux-headers/linux/vfio.h?) Given that the header is our primary source about how this interface should act, we need to properly document expectations about what will be saved/restored when there (well, in the source file in the kernel.) That goes in both directions: what a userspace must implement, and what a vendor driver can rely on. Yeah, in order to make the vendor driver and QEMU cooperate better, we might need to document some expectations about the data section in the migration region... [Related, but not a todo for you: I think we're still missing proper documentation of the whole migration feature.] Yes, we never saw anything past v1 of the documentation patch. Thanks, By the way, is there anything unproper with this patch? Wish your suggestion. :-) I'm really hoping for some feedback from Kirti, I understand the NVIDIA vGPU driver to have some dependency on this. Thanks, I need to verify this patch. Spare me a day to verify this. Thanks, Kirti Alex Signed-off-by: Shenming Lu --- hw/vfio/migration.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) .
