date:20210223

Re: [PATCH] virtio-net: Add check for mac address while peer is vdpa

2021-02-23 Thread Michael S. Tsirkin

> [PATCH] virtio-net: Add check for mac address while peer is vdpa
please do keep numbering patch versions.

On Wed, Feb 24, 2021 at 03:33:33PM +0800, Cindy Lu wrote:
> Sometime vdpa get an all zero mac address from the hardware, this will cause 
> the
> traffic down, So we add the check for in part.if we get an zero mac address 
> we will
> use the default/cmdline mac address instead

How does this differ from v4 of same patch?

Lots of typos above. I guess I can just rewrite the whole log ...

> 
> Signed-off-by: Cindy Lu 
> ---
>  hw/net/virtio-net.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9179013ac4..f1648fc47d 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> uint8_t *config)
>  VirtIONet *n = VIRTIO_NET(vdev);
>  struct virtio_net_config netcfg;
>  NetClientState *nc = qemu_get_queue(n->nic);
> +static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
>  
>  int ret = 0;
>  memset(, 0 , sizeof(struct virtio_net_config));
> @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> uint8_t *config)
>  ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t 
> *),
> n->config_size);
>  if (ret != -1) {
> -memcpy(config, , n->config_size);

Below needs a huge code comment explaining what is going on.

E.g.  if we get a valid mac from peer, use it.

If we get 0 instead
then we don't know the peer mac but since the
mac is 0 and that is not a legal value,
try to proceed assume something else (management, hardware) sets up the mac
correctly and consistently with the qemu configuration.

> +if (memcmp(, , sizeof(zero)) != 0) {

!= 0 is not necessary

> +memcpy(config, , n->config_size);

> +} else {
> +error_report("Get an all zero mac address from hardware");

So why are you skipping the copying of the whole config space? Why not
just skip the mac? Seems quite risky e.g. looks like it will break
things like reporting the link status, right?

> +}
>  }
>  }
>  }
> -- 
> 2.21.3

Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism

2021-02-23 Thread Paolo Bonzini


On 23/02/21 20:34, Daniele Buono wrote:

This works, but setting this value to 1 for everybody seems a bit too
restrictive. While the gitlab ci runners don't have enough memory for
this, that's not necessarily true for every build platform, and linking
multiple targets in parallel with LTO can result in a big save in time,
so I'd prefer a customizable way.

How about adding a flag `--max-ld-procs` to configure to manually set
backend_max_links?


Another possibility is to invoke "meson configure build 
-Dbackend_max_links=1" after configure.


Paolo


This would also allow setting it up to any specific number above 1,
which looking at the Makefile seems to not be possible now: because of
how the -j flag is passed from make to ninja, a compilation is either
sequential or parallel based on #cpus

Re: [PATCH] disas: Fix build with glib2.0 >=2.67.3

2021-02-23 Thread Christian Ehrhardt

On Tue, Feb 23, 2021 at 5:12 PM Daniel P. Berrangé  wrote:
>
> On Tue, Feb 23, 2021 at 03:43:48PM +, Peter Maydell wrote:
> > On Tue, 23 Feb 2021 at 15:03, Christian Ehrhardt
> >  wrote:
> > >
> > > glib2.0 introduced a change in 2.67.3 and later which triggers an
> > > issue [1] for anyone including it's headers in a "extern C" context
> > > which a few files in disas/* do. An example of such an include chain
> > > and error look like:
> > >
> > > ../../disas/arm-a64.cc
> > > In file included from /usr/include/glib-2.0/glib/gmacros.h:241,
> > >  from 
> > > /usr/lib/x86_64-linux-gnu/glib-2.0/include/glibconfig.h:9,
> > >  from /usr/include/glib-2.0/glib/gtypes.h:32,
> > >  from /usr/include/glib-2.0/glib/galloca.h:32,
> > >  from /usr/include/glib-2.0/glib.h:30,
> > >  from 
> > > /<>/qemu-5.2+dfsg/include/glib-compat.h:32,
> > >  from 
> > > /<>/qemu-5.2+dfsg/include/qemu/osdep.h:126,
> > >  from ../../disas/arm-a64.cc:21:
> > > /usr/include/c++/10/type_traits:56:3: error: template with C linkage
> > >56 |   template
> > >   |   ^~~~
> > > ../../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here
> > >20 | extern "C" {
> > >   | ^~
> > >
> > > To fix that move the include of osdep.h out of that section. It was added
> > > already as C++ fixup by e78490c44: "disas/arm-a64.cc: Include osdep.h 
> > > first".
> > >
> > > [1]: https://gitlab.gnome.org/GNOME/glib/-/issues/2331
> >
> > I'm not convinced by this as a fix, though I'm happy to be corrected
> > by somebody with a fuller understanding of C++. glib.h may be supposed
> > to work as a C++ header, but osdep.h as a whole is definitely a C header,
> > so I think it ought to be inside 'extern C'; and it has to be
> > the first header included; and it happens to want to include glib.h.
> >
> > Fixing glib.h seems like it would be nicer, assuming they haven't
> > already shipped this breakage.

They have already shipped this. And in the linked issue and from there
MR discussions
you'll find that everyone acknowledges that the "error" is in the
consuming projects
and that glib upstream is rejecting to fix it for e.g. the argument of
backward compatibility.

> Failing that, does it work to do:
>
> This was raised in Fedora and upstream GLib already
>
> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1935
> https://lists.fedoraproject.org/archives/list/de...@lists.fedoraproject.org/thread/J3P4TRHLWNDIKXF76OLYZNAPTABCZ3U5/#7LXFUDBBBIT23FE44QJYWX3I7U4EHW6M
>
> The key comment there is this one:
>
>   "Note that wrapping the header include in an extern declaration violates
>C++ standard requirements.  ("A translation unit shall include a header
>only outside of any declaration or definition", [using.headers]/3)"
>
> IOW, if we need to make osdep.h safe to use from C++, then we
> need to put the 'extern "C" { ... }'  bit in osdep.h itself,
> not in the things which include it.
>
> >
> > /*
> >  * glib.h expects to be included as a C++ header if we're
> >  * building a C++ file, but osdep.h and thus glib-compat.h are
> >  * C headers and should be inside an "extern C" block.
> >  */
> > #ifdef __cplusplus
> > extern "C++" {
> > #include 
> > #if defined(G_OS_UNIX)
> > #include 
> > #endif
> > }
> >
> > in include/glib-compat.h ?
>
> That'd be even worse.
>
> We need to make headers that need to be used from C++ code follow
> the pattern:
>
> #include 
> #include 
> #include 
> ...all other includs..
>
> extern "C" {
> ..
> only the declarations, no #includes
> ...
> };

While I can follow the words and always awesome explanations by Daniel,
I must admit that I'm a bit lost at what a v2 of this could look like.

osdep.h as of today unfortunately isn't as trivial as 1. include 2.
declarations.
There are late includes deep in cascading ifdef's and we all know that "just
moving includes around for the above fix to work in an easy way" in headers
will likely (maybe even silently) break things.

So I wonder is this going to become a massive patch either moving a lot or
adding many extern C declarations all over the place in osdep-h? Or did I
just fail to see that there is an obviously better approach to this?

>
> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH] virtio-net: Add check for mac address while peer is vdpa

2021-02-23 Thread Cindy Lu

Sometime vdpa get an all zero mac address from the hardware, this will cause the
traffic down, So we add the check for in part.if we get an zero mac address we 
will
use the default/cmdline mac address instead

Signed-off-by: Cindy Lu 
---
 hw/net/virtio-net.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9179013ac4..f1648fc47d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
uint8_t *config)
 VirtIONet *n = VIRTIO_NET(vdev);
 struct virtio_net_config netcfg;
 NetClientState *nc = qemu_get_queue(n->nic);
+static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
 
 int ret = 0;
 memset(, 0 , sizeof(struct virtio_net_config));
@@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
uint8_t *config)
 ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *),
n->config_size);
 if (ret != -1) {
-memcpy(config, , n->config_size);
+if (memcmp(, , sizeof(zero)) != 0) {
+memcpy(config, , n->config_size);
+} else {
+error_report("Get an all zero mac address from hardware");
+}
 }
 }
 }
-- 
2.21.3

Re: [PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

2021-02-23 Thread Stefan Weil


Am 24.02.21 um 06:53 schrieb Jason Wang:


This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang 
---
  hw/net/dp8393x.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 205c0decc5..019d4fe435 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -506,7 +506,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
  s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
  if (nc->info->can_receive(nc)) {
  s->loopback_packet = 1;
-nc->info->receive(nc, s->tx_buffer, tx_len);
+qemu_receice_packet(nc, s->tx_buffer, tx_Len);



Did you test compilation of that code? It looks like a typo ...

Regards

Stefan W.

Re: What prevents discarding a cluster during rewrite?

2021-02-23 Thread Vladimir Sementsov-Ogievskiy


23.02.2021 19:29, Kevin Wolf wrote:

Am 23.02.2021 um 16:23 hat Vladimir Sementsov-Ogievskiy geschrieben:



On 23.02.2021 13:35, Kevin Wolf wrote:

Am 22.02.2021 um 22:42 hat Vladimir Sementsov-Ogievskiy geschrieben:

23.02.2021 00:30, Vladimir Sementsov-Ogievskiy wrote:

Thinking of how to prevent dereferencing to zero (and discard) of
host cluster during flush of compressed cache (which I'm working on
now), I have a question.. What prevents it for normal writes?


I have no idea about why didn't it fail for years.. May be, I'm
missing something?


Ouch. I suppose the reason why we never ran into it is that afaik Linux
drains the queue before sending discard requests.

Of course, we still need to protect against this in QEMU. We can't just
unref a cluster that is still in use.


I have idea of fixing: increase the refcount of host cluster before
write to data_file (it's important to increase refacount in same
s->lock critical section where we get host_offset) and dereference it
after write.. It should help. Any thoughts?


It would cause metadata updates for rewrites. I don't know whether the
intermediate value would ever be written to disk, but at least we'd
rewrite the same refcounts as before. I don't think we want that.


Hmm, if that can provoke extra refcount cache flush that's bad..

May be we need something like of additional "dynamic" refcounts, so
that total_refcount = normal_refcount + dynamic_refcount.. And for
in-flight clusters dynamic_refcount is > 0. We can store dynamic
refcounts in GHashTable(cluster -> refcount).


Do you think it's worth the complexity? The qcow2 driver is already
relatively complicated today.


I started to implement it and actually it doesn't seem much more complicated
than additional CoRwLock. So, I think, I will make two patches (refcounts vs
CoRwMutex) and we'll compare.




Discard requests might be rare enough that not considering the cluster
at all could work. We could then take a reader CoRwlock during most
operations, and a writer lock for discard.

Actually, maybe s->lock should be this CoRwlock, and instead of dropping
it temporarily like we do now we would just upgrade and downgrade it as
needed. Maybe this would allow finer grained locking in other places,
too.


In this case many operations will be blocked during data writing, like
allocation of another cluster.. That doesn't seem good.


You're right, that would be too restrictive.


Separate CoRwLock looks more feasible.


Maybe that then.

Kevin




--
Best regards,
Vladimir

Re: [PATCH 0/6] Detect reentrant RX casue by loopback

2021-02-23 Thread no-reply

Patchew URL: 
https://patchew.org/QEMU/20210224055401.492407-1-jasow...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210224055401.492407-1-jasow...@redhat.com
Subject: [PATCH 0/6] Detect reentrant RX casue by loopback

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20210224055401.492407-1-jasow...@redhat.com -> 
patchew/20210224055401.492407-1-jasow...@redhat.com
Switched to a new branch 'test'
6a41d09 tx_pkt: switch to use qemu_receive_packet_iov() for loopback
5480e55 sungem: switch to use qemu_receive_packet() for loopback
f35fab6 msf2-mac: switch to use qemu_receive_packet() for loopback
1dc29cf dp8393x: switch to use qemu_receive_packet() for loopback packet
6c79834 e1000: switch to use qemu_receive_packet() for loopback
04660c0 net: introduce qemu_receive_packet()

=== OUTPUT BEGIN ===
1/6 Checking commit 04660c06d88e (net: introduce qemu_receive_packet())
ERROR: space required after that ',' (ctx:VxV)
#42: FILE: include/net/net.h:154:
+ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf,int size);
   ^

total: 1 errors, 0 warnings, 115 lines checked

Patch 1/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/6 Checking commit 6c7983468e94 (e1000: switch to use qemu_receive_packet() 
for loopback)
3/6 Checking commit 1dc29cfa3117 (dp8393x: switch to use qemu_receive_packet() 
for loopback packet)
4/6 Checking commit f35fab6cb737 (msf2-mac: switch to use qemu_receive_packet() 
for loopback)
5/6 Checking commit 5480e5557678 (sungem: switch to use qemu_receive_packet() 
for loopback)
6/6 Checking commit 6a41d093a705 (tx_pkt: switch to use 
qemu_receive_packet_iov() for loopback)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210224055401.492407-1-jasow...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH 5/6] sungem: switch to use qemu_receive_packet() for loopback

2021-02-23 Thread Jason Wang

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang 
---
 hw/net/sungem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/sungem.c b/hw/net/sungem.c
index 33c3722df6..3684a4d733 100644
--- a/hw/net/sungem.c
+++ b/hw/net/sungem.c
@@ -306,7 +306,7 @@ static void sungem_send_packet(SunGEMState *s, const 
uint8_t *buf,
 NetClientState *nc = qemu_get_queue(s->nic);
 
 if (s->macregs[MAC_XIFCFG >> 2] & MAC_XIFCFG_LBCK) {
-nc->info->receive(nc, buf, size);
+qemu_receive_packet(nc, buf, size);
 } else {
 qemu_send_packet(nc, buf, size);
 }
-- 
2.25.1

[PATCH 4/6] msf2-mac: switch to use qemu_receive_packet() for loopback

2021-02-23 Thread Jason Wang

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang 
---
 hw/net/msf2-emac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c
index 32ba9e8412..3e6206044f 100644
--- a/hw/net/msf2-emac.c
+++ b/hw/net/msf2-emac.c
@@ -158,7 +158,7 @@ static void msf2_dma_tx(MSF2EmacState *s)
  * R_CFG1 bit 0 is set.
  */
 if (s->regs[R_CFG1] & R_CFG1_LB_EN_MASK) {
-nc->info->receive(nc, buf, size);
+qemu_receive_packet(nc, buf, size);
 } else {
 qemu_send_packet(nc, buf, size);
 }
-- 
2.25.1

[PATCH 1/6] net: introduce qemu_receive_packet()

2021-02-23 Thread Jason Wang

Some NIC supports loopback mode and this is done by calling
nc->info->receive() directly which in fact suppresses the effort of
reentrancy check that is done in qemu_net_queue_send().

Unfortunately we can use qemu_net_queue_send() here since for loop
back there's no sender as peer, so this patch introduce a
qemu_receive_packet() which is used for implementing loopback mode
for a NIC with this check.

NIC that supports loopback mode will be converted to this helper.

Signed-off-by: Jason Wang 
---
 include/net/net.h   |  5 +
 include/net/queue.h |  8 
 net/net.c   | 38 +++---
 net/queue.c | 22 ++
 4 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 919facaad2..65eb8a58c5 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -144,12 +144,17 @@ void *qemu_get_nic_opaque(NetClientState *nc);
 void qemu_del_net_client(NetClientState *nc);
 typedef void (*qemu_nic_foreach)(NICState *nic, void *opaque);
 void qemu_foreach_nic(qemu_nic_foreach func, void *opaque);
+int qemu_can_receive_packet(NetClientState *nc);
 int qemu_can_send_packet(NetClientState *nc);
 ssize_t qemu_sendv_packet(NetClientState *nc, const struct iovec *iov,
   int iovcnt);
 ssize_t qemu_sendv_packet_async(NetClientState *nc, const struct iovec *iov,
 int iovcnt, NetPacketSent *sent_cb);
 ssize_t qemu_send_packet(NetClientState *nc, const uint8_t *buf, int size);
+ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf,int size);
+ssize_t qemu_receive_packet_iov(NetClientState *nc,
+const struct iovec *iov,
+int iovcnt);
 ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size);
 ssize_t qemu_send_packet_async(NetClientState *nc, const uint8_t *buf,
int size, NetPacketSent *sent_cb);
diff --git a/include/net/queue.h b/include/net/queue.h
index c0269bb1dc..9f2f289d77 100644
--- a/include/net/queue.h
+++ b/include/net/queue.h
@@ -55,6 +55,14 @@ void qemu_net_queue_append_iov(NetQueue *queue,
 
 void qemu_del_net_queue(NetQueue *queue);
 
+ssize_t qemu_net_queue_receive(NetQueue *queue,
+   const uint8_t *data,
+   size_t size);
+
+ssize_t qemu_net_queue_receive_iov(NetQueue *queue,
+   const struct iovec *iov,
+   int iovcnt);
+
 ssize_t qemu_net_queue_send(NetQueue *queue,
 NetClientState *sender,
 unsigned flags,
diff --git a/net/net.c b/net/net.c
index e1035f21d1..6e470133ad 100644
--- a/net/net.c
+++ b/net/net.c
@@ -528,6 +528,17 @@ int qemu_set_vnet_be(NetClientState *nc, bool is_be)
 #endif
 }
 
+int qemu_can_receive_packet(NetClientState *nc)
+{
+if (nc->receive_disabled) {
+return 0;
+} else if (nc->info->can_receive &&
+   !nc->info->can_receive(nc)) {
+return 0;
+}
+return 1;
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
 int vm_running = runstate_is_running();
@@ -540,13 +551,7 @@ int qemu_can_send_packet(NetClientState *sender)
 return 1;
 }
 
-if (sender->peer->receive_disabled) {
-return 0;
-} else if (sender->peer->info->can_receive &&
-   !sender->peer->info->can_receive(sender->peer)) {
-return 0;
-}
-return 1;
+return qemu_can_receive_packet(sender->peer);
 }
 
 static ssize_t filter_receive_iov(NetClientState *nc,
@@ -679,6 +684,25 @@ ssize_t qemu_send_packet(NetClientState *nc, const uint8_t 
*buf, int size)
 return qemu_send_packet_async(nc, buf, size, NULL);
 }
 
+ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf, int size)
+{
+if (!qemu_can_receive_packet(nc)) {
+return 0;
+}
+
+return qemu_net_queue_receive(nc->incoming_queue, buf, size);
+}
+
+ssize_t qemu_receive_packet_iov(NetClientState *nc, const struct iovec *iov,
+int iovcnt)
+{
+if (!qemu_can_receive_packet(nc)) {
+return 0;
+}
+
+return qemu_net_queue_receive_iov(nc->incoming_queue, iov, iovcnt);
+}
+
 ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
 {
 return qemu_send_packet_async_with_flags(nc, QEMU_NET_PACKET_FLAG_RAW,
diff --git a/net/queue.c b/net/queue.c
index 19e32c80fd..c872d51df8 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -182,6 +182,28 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
 return ret;
 }
 
+ssize_t qemu_net_queue_receive(NetQueue *queue,
+   const uint8_t *data,
+   size_t size)
+{
+if (queue->delivering) {
+return 0;
+}
+
+return qemu_net_queue_deliver(queue, NULL, 0, data, size);
+}
+
+ssize_t

[PATCH 6/6] tx_pkt: switch to use qemu_receive_packet_iov() for loopback

2021-02-23 Thread Jason Wang

This patch switches to use qemu_receive_receive_iov() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang 
---
 hw/net/net_tx_pkt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index da262edc3e..1f9aa59eca 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -553,7 +553,7 @@ static inline void net_tx_pkt_sendv(struct NetTxPkt *pkt,
 NetClientState *nc, const struct iovec *iov, int iov_cnt)
 {
 if (pkt->is_loopback) {
-nc->info->receive_iov(nc, iov, iov_cnt);
+qemu_receive_packet_iov(nc, iov, iov_cnt);
 } else {
 qemu_sendv_packet(nc, iov, iov_cnt);
 }
-- 
2.25.1

[PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

2021-02-23 Thread Jason Wang

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang 
---
 hw/net/dp8393x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 205c0decc5..019d4fe435 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -506,7 +506,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
 s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
 if (nc->info->can_receive(nc)) {
 s->loopback_packet = 1;
-nc->info->receive(nc, s->tx_buffer, tx_len);
+qemu_receice_packet(nc, s->tx_buffer, tx_Len);
 }
 } else {
 /* Transmit packet */
-- 
2.25.1

[PATCH 2/6] e1000: switch to use qemu_receive_packet() for loopback

2021-02-23 Thread Jason Wang

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang 
---
 hw/net/e1000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 4345d863e6..4f75b44cfc 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -546,7 +546,7 @@ e1000_send_packet(E1000State *s, const uint8_t *buf, int 
size)
 
 NetClientState *nc = qemu_get_queue(s->nic);
 if (s->phy_reg[PHY_CTRL] & MII_CR_LOOPBACK) {
-nc->info->receive(nc, buf, size);
+qemu_receive_packet(nc, buf, size);
 } else {
 qemu_send_packet(nc, buf, size);
 }
-- 
2.25.1

[PATCH 0/6] Detect reentrant RX casue by loopback

2021-02-23 Thread Jason Wang

Hi All:

Followed by commit 22dc8663d9 ("net: forbid the reentrant RX"), we
still need to fix the issues casued by loopback mode where the NIC
usually it via calling nc->info->receive() directly.

The fix is to introduce new network helper and check the
queue->delivering.

Thanks

Jason Wang (6):
  net: introduce qemu_receive_packet()
  e1000: switch to use qemu_receive_packet() for loopback
  dp8393x: switch to use qemu_receive_packet() for loopback packet
  msf2-mac: switch to use qemu_receive_packet() for loopback
  sungem: switch to use qemu_receive_packet() for loopback
  tx_pkt: switch to use qemu_receive_packet_iov() for loopback

 hw/net/dp8393x.c|  2 +-
 hw/net/e1000.c  |  2 +-
 hw/net/msf2-emac.c  |  2 +-
 hw/net/net_tx_pkt.c |  2 +-
 hw/net/sungem.c |  2 +-
 include/net/net.h   |  5 +
 include/net/queue.h |  8 
 net/net.c   | 38 +++---
 net/queue.c | 22 ++
 9 files changed, 71 insertions(+), 12 deletions(-)

-- 
2.25.1

[Bug 1914535] Re: PL110 8-bit mode is not emulated correctly

2021-02-23 Thread Vadim Averin

Well... I don't have any ARM Versatile Express board. Also,
documentation and manuals are not saying much about it

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

Title:
  PL110 8-bit mode is not emulated correctly

Status in QEMU:
  New

Bug description:
  When the emulated pl110/pl111 is switched programmatically to 8-bit
  color depth mode, the display is drawn green and blue, but the real
  PL110 displays grayscale in 8-bit mode.

  The bug appears in qemu-system-arm version 3.1.0 (Debian
  1:3.1+dfsg-8+deb10u8) and qemu-system-arm version 5.2.50
  (v5.2.0-1579-g99ae0cd90d).

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

Re: [PATCH qemu v14] spapr: Implement Open Firmware client interface

2021-02-23 Thread no-reply

Patchew URL: https://patchew.org/QEMU/20210224054130.4540-1-...@ozlabs.ru/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210224054130.4540-1-...@ozlabs.ru
Subject: [PATCH qemu v14] spapr: Implement Open Firmware client interface

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20210224054130.4540-1-...@ozlabs.ru -> 
patchew/20210224054130.4540-1-...@ozlabs.ru
Switched to a new branch 'test'
3fc539b spapr: Implement Open Firmware client interface

=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#268: FILE: hw/ppc/spapr.c:4463:
+ClientArchitectureSupportClass *casc = 
CLIENT_ARCHITECTURE_SUPPORT_CLASS(oc);

WARNING: line over 80 characters
#1431: FILE: hw/ppc/vof.h:29:
+INTERFACE_CHECK(ClientArchitectureSupport, (obj), 
TYPE_CLIENT_ARCHITECTURE_SUPPORT)

ERROR: code indent should never use tabs
#1548: FILE: pc-bios/vof/bootmem.c:5:
+^Iuint64_t kern[2];$

ERROR: code indent should never use tabs
#1549: FILE: pc-bios/vof/bootmem.c:6:
+^Iphandle chosen = ci_finddevice("/chosen");$

ERROR: code indent should never use tabs
#1551: FILE: pc-bios/vof/bootmem.c:8:
+^Iif (ci_getprop(chosen, "qemu,boot-kernel", kern, sizeof(kern)) !=$

ERROR: code indent should never use tabs
#1552: FILE: pc-bios/vof/bootmem.c:9:
+^I^I^Isizeof(kern))$

ERROR: code indent should never use tabs
#1553: FILE: pc-bios/vof/bootmem.c:10:
+^I^Ireturn;$

ERROR: code indent should never use tabs
#1555: FILE: pc-bios/vof/bootmem.c:12:
+^Ido_boot(kern[0], initrd, initrdsize);$

ERROR: externs should be avoided in .c files
#1574: FILE: pc-bios/vof/ci.c:12:
+extern uint32_t ci_entry(uint32_t params);

ERROR: externs should be avoided in .c files
#1576: FILE: pc-bios/vof/ci.c:14:
+extern unsigned long hv_rtas(unsigned long params);

ERROR: externs should be avoided in .c files
#1577: FILE: pc-bios/vof/ci.c:15:
+extern unsigned int hv_rtas_size;

ERROR: code indent should never use tabs
#1581: FILE: pc-bios/vof/ci.c:19:
+^Ivoid *rtasbase;$

ERROR: code indent should never use tabs
#1582: FILE: pc-bios/vof/ci.c:20:
+^Iuint32_t rtassize = 0;$

ERROR: code indent should never use tabs
#1583: FILE: pc-bios/vof/ci.c:21:
+^Iphandle rtas;$

ERROR: code indent should never use tabs
#1585: FILE: pc-bios/vof/ci.c:23:
+^Iif (strcmp("call-method", (void *)(unsigned long) pargs->service))$

ERROR: braces {} are necessary for all arms of this statement
#1585: FILE: pc-bios/vof/ci.c:23:
+   if (strcmp("call-method", (void *)(unsigned long) pargs->service))
[...]

ERROR: code indent should never use tabs
#1586: FILE: pc-bios/vof/ci.c:24:
+^I^Ireturn false;$

ERROR: code indent should never use tabs
#1588: FILE: pc-bios/vof/ci.c:26:
+^Iif (strcmp("instantiate-rtas", (void *)(unsigned long) pargs->args[0]))$

ERROR: braces {} are necessary for all arms of this statement
#1588: FILE: pc-bios/vof/ci.c:26:
+   if (strcmp("instantiate-rtas", (void *)(unsigned long) pargs->args[0]))
[...]

ERROR: code indent should never use tabs
#1589: FILE: pc-bios/vof/ci.c:27:
+^I^Ireturn false;$

ERROR: code indent should never use tabs
#1591: FILE: pc-bios/vof/ci.c:29:
+^Irtas = ci_finddevice("/rtas");$

ERROR: code indent should never use tabs
#1592: FILE: pc-bios/vof/ci.c:30:
+^Ici_getprop(rtas, "rtas-size", , sizeof(rtassize));$

ERROR: code indent should never use tabs
#1593: FILE: pc-bios/vof/ci.c:31:
+^Iif (rtassize < hv_rtas_size)$

ERROR: braces {} are necessary for all arms of this statement
#1593: FILE: pc-bios/vof/ci.c:31:
+   if (rtassize < hv_rtas_size)
[...]

ERROR: code indent should never use tabs
#1594: FILE: pc-bios/vof/ci.c:32:
+^I^Ireturn false;$

ERROR: code indent should never use tabs
#1596: FILE: pc-bios/vof/ci.c:34:
+^Irtasbase = (void *)(unsigned long) pargs->args[2];$

ERROR: code indent should never use tabs
#1598: FILE: pc-bios/vof/ci.c:36:
+^Imemcpy(rtasbase, hv_rtas, hv_rtas_size);$

ERROR: code indent should never use tabs
#1599: FILE: pc-bios/vof/ci.c:37:
+^Ipargs->args[pargs->nargs] = 0;$

ERROR: code indent should never use tabs
#1600: FILE: pc-bios/vof/ci.c:38:
+^Ipargs->args[pargs->nargs + 1] = pargs->args[2];$

ERROR: code indent should never use tabs
#1602: FILE: pc-bios/vof/ci.c:40:
+^Ireturn true;$

ERROR: code indent should never use tabs
#1607: FILE: pc-bios/vof/ci.c:45:
+^Iif (!prom_handle((void *)(unsigned long) args))$

ERROR: braces {} are necessary for all arms of this statement
#1607: FILE: pc-bios/vof/ci.c:45:
+   if (!prom_handle((void *)(unsigned long) args))
[...]

ERROR: code indent should never use tabs
#1608: FILE: pc-bios/vof/ci.c:46:
+^I^Ici_entry(args);$

ERROR: braces {} are

[PATCH] e1000: fail early for evil descriptor

2021-02-23 Thread Jason Wang

During procss_tx_desc(), driver can try to chain data descriptor with
legacy descriptor, when will lead underflow for the following
calculation in process_tx_desc() for bytes:

if (tp->size + bytes > msh)
bytes = msh - tp->size;

This will lead a infinite loop. So check and fail early if tp->size if
greater or equal to msh.

Reported-by: Alexander Bulekov 
Reported-by: Cheolwoo Myung 
Reported-by: Ruhr-University Bochum 
Cc: Prasad J Pandit 
Signed-off-by: Jason Wang 
---
 hw/net/e1000.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index d8da2f6528..4345d863e6 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -670,6 +670,9 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
 msh = tp->tso_props.hdr_len + tp->tso_props.mss;
 do {
 bytes = split_size;
+if (tp->size >= msh) {
+goto eop;
+}
 if (tp->size + bytes > msh)
 bytes = msh - tp->size;
 
@@ -695,6 +698,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
 tp->size += split_size;
 }
 
+eop:
 if (!(txd_lower & E1000_TXD_CMD_EOP))
 return;
 if (!(tp->cptse && tp->size < tp->tso_props.hdr_len)) {
-- 
2.25.1

[PATCH qemu v14] spapr: Implement Open Firmware client interface

2021-02-23 Thread Alexey Kardashevskiy

The PAPR platform which describes an OS environment that's presented by
a combination of a hypervisor and firmware. The features it specifies
require collaboration between the firmware and the hypervisor.

Since the beginning, the runtime component of the firmware (RTAS) has
been implemented as a 20 byte shim which simply forwards it to
a hypercall implemented in qemu. The boot time firmware component is
SLOF - but a build that's specific to qemu, and has always needed to be
updated in sync with it. Even though we've managed to limit the amount
of runtime communication we need between qemu and SLOF, there's some,
and it has become increasingly awkward to handle as we've implemented
new features.

This implements a boot time OF client interface (CI) which is
enabled by a new "x-vof" pseries machine option (stands for "Virtual Open
Firmware). When enabled, QEMU implements the custom H_OF_CLIENT hcall
which implements Open Firmware Client Interface (OF CI). This allows
using a smaller stateless firmware which does not have to manage
the device tree.

The new "vof.bin" firmware image is included with source code under
pc-bios/. It also includes RTAS blob.

This implements a handful of CI methods just to get -kernel/-initrd
working. In particular, this implements the device tree fetching and
simple memory allocator - "claim" (an OF CI memory allocator) and updates
"/memory@0/available" to report the client about available memory.

This implements changing some device tree properties which we know how
to deal with, the rest is ignored. To allow changes, this skips
fdt_pack() when x-vof=on as not packing the blob leaves some room for
appending.

In absence of SLOF, this assigns phandles to device tree nodes to make
device tree traversing work.

When x-vof=on, this adds "/chosen" every time QEMU (re)builds a tree.

This adds basic instances support which are managed by a hash map
ihandle -> [phandle].

Before the guest started, the used memory is:
0..4000 - the initial firmware
1..18 - stack

This OF CI does not implement "interpret".

Unlike SLOF, this does not format uninitialized nvram. Instead, this
includes a disk image with pre-formatted nvram.

With this basic support, this can only boot into kernel directly.
However this is just enough for the petitboot kernel and initradmdisk to
boot from any possible source. Note this requires reasonably recent guest
kernel with:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df5be5be8735

The immediate benefit is much faster booting time which especially
crucial with fully emulated early CPU bring up environments. Also this
may come handy when/if GRUB-in-the-userspace sees light of the day.

This separates VOF and sPAPR in a hope that VOF bits may be reused by
other POWERPC boards which do not support pSeries.

This is coded in assumption that later on we might be adding support for
booting from QEMU backends (blockdev is the first candidate) without
devices/drivers in between as OF1275 does not require that and
it is quite easy to so.

Signed-off-by: Alexey Kardashevskiy
---

The example command line is:

/home/aik/pbuild/qemu-killslof-localhost-ppc64/qemu-system-ppc64 \
-nodefaults \
-chardev stdio,id=STDIO0,signal=off,mux=on \
-device spapr-vty,id=svty0,reg=0x71000110,chardev=STDIO0 \
-mon id=MON0,chardev=STDIO0,mode=readline \
-nographic \
-vga none \
-enable-kvm \
-m 2G \
-machine
pseries,x-vof=on,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off
\
-kernel pbuild/kernel-le-guest/vmlinux \
-initrd pb/rootfs.cpio.xz \
-drive
id=DRIVE0,if=none,file=./p/qemu-killslof/pc-bios/vof/nvram.bin,format=raw \
-global spapr-nvram.drive=DRIVE0 \
-snapshot \
-smp 8,threads=8 \
-L /home/aik/t/qemu-ppc64-bios/ \
-trace events=qemu_trace_events \
-d guest_errors \
-chardev socket,id=SOCKET0,server,nowait,path=qemu.mon.tmux26 \
-mon chardev=SOCKET0,mode=control

---
Changes:
v14:
* check for truncates in readstr()
* ditched a separate vof_reset()
* spapr->vof is a pointer now, dropped the "on" field
* removed rtas_base from vof and updated comment why we allow setting it
* added myself to maintainers
* updated commit log about blockdev and other possible platforms
* added a note why new hcall is 0x5
* no in place endianness convertion in spapr_h_vof_client
* converted all cpu_physical_memory_read/write to address_space_rw
* git mv hw/ppc/spapr_vof_client.c hw/ppc/spapr_vof.c

v13:
* rebase on latest ppc-for-6.0
* shuffled code around to touch spapr.c less

v12:
* split VOF and SPAPR

v11:
* added g_autofree
* fixed gcc warnings
* fixed few leaks
* added nvram image to make "nvram --print-config" not crash;
Note that contrary to MIN_NVRAM_SIZE (8 * KiB), the actual minimum size
is 16K, or it just does not work (empty output from "nvram")

v10:
* now rebased to compile with meson

v9:
* remove special handling of /rtas/rtas-size as now we always add it in QEMU
* removed leftovers from scsi/grub/stdout/stdin/...

v8:
* no

[REBASE] [PATCH v5] qga: Correct loop count in qmp_guest_get_vcpus()

2021-02-23 Thread Lin Ma

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
Reviewed-by: Marc-André Lureau 
---
 qga/commands-posix.c | 43 ++-
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8dd94a3314..530c98344c 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2385,24 +2385,6 @@ error:
 return NULL;
 }
 
-#define SYSCONF_EXACT(name, errp) sysconf_exact((name), #name, (errp))
-
-static long sysconf_exact(int name, const char *name_str, Error **errp)
-{
-long ret;
-
-errno = 0;
-ret = sysconf(name);
-if (ret == -1) {
-if (errno == 0) {
-error_setg(errp, "sysconf(%s): value indefinite", name_str);
-} else {
-error_setg_errno(errp, errno, "sysconf(%s)", name_str);
-}
-}
-return ret;
-}
-
 /* Transfer online/offline status between @vcpu and the guest system.
  *
  * On input either @errp or *@errp must be NULL.
@@ -2473,30 +2455,33 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
bool sys2vcpu,
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
-int64_t current;
 GuestLogicalProcessorList *head, **tail;
-long sc_max;
+const char *cpu_dir = "/sys/devices/system/cpu";
+const gchar *line;
+g_autoptr(GDir) cpu_gdir = NULL;
 Error *local_err = NULL;
 
-current = 0;
 head = NULL;
 tail = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
 
-while (local_err == NULL && current < sc_max) {
-GuestLogicalProcessor *vcpu;
-int64_t id = current++;
-char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
- id);
+if (cpu_gdir == NULL) {
+error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
+return NULL;
+}
 
-if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
+GuestLogicalProcessor *vcpu;
+int64_t id;
+if (sscanf(line, "cpu%ld", )) {
+g_autofree char *path = g_strdup_printf("/sys/devices/system/cpu/"
+"cpu%" PRId64 "/", id);
 vcpu = g_malloc0(sizeof *vcpu);
 vcpu->logical_id = id;
 vcpu->has_can_offline = true; /* lolspeak ftw */
 transfer_vcpu(vcpu, true, path, _err);
 QAPI_LIST_APPEND(tail, vcpu);
 }
-g_free(path);
 }
 
 if (local_err == NULL) {
-- 
2.26.2

Re: [PATCH v2 0/6] testing/next (meson check-tcg, fedora bump, docs)

2021-02-23 Thread Richard Henderson

On 2/22/21 2:14 AM, Alex Bennée wrote:
> Alex Bennée (5):
>   meson.build: expose TCG cross compiler information in summary
>   tests/acceptance: allow a "graceful" failing for virtio-gpu test
>   docs/devel: expand on use of containers to build tests
>   docs/devel: update the container based tests
>   docs/devel: add forward reference to check-tcg
> 
> Philippe Mathieu-Daudé (1):
>   docker: Bump Fedora images to release 33

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 10/10] target/mips: Extract MXU code to new mxu_translate.c file

2021-02-23 Thread Richard Henderson

On 2/22/21 2:39 PM, Philippe Mathieu-Daudé wrote:
> Extract 1600+ lines from the big translate.c into a new file.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/mxu_translate.c | 1625 +++
>  target/mips/translate.c | 1613 --
>  target/mips/meson.build |1 +
>  3 files changed, 1626 insertions(+), 1613 deletions(-)
>  create mode 100644 target/mips/mxu_translate.c


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 09/10] target/mips: Simplify 64-bit ifdef'ry of MXU code

2021-02-23 Thread Richard Henderson

On 2/22/21 2:39 PM, Philippe Mathieu-Daudé wrote:
> +#else /* !defined(TARGET_MIPS64) */
> +
> +bool decode_ase_mxu(DisasContext *ctx, uint32_t insn)
> +{
> +return false;

Also seems suspect, but harmless.

> -#if !defined(TARGET_MIPS64)
> -if (ctx->insn_flags & ASE_MXU) {
> +if ((TARGET_LONG_BITS == 32) && (ctx->insn_flags & ASE_MXU)) {
>  decode_opc_mxu(ctx, ctx->opcode);

(1) Unnecessary () around ==.

(2) The call to decode_opc_mxu should be eliminated by the compiler because of
the constant false test.  You can (a) retain the function above and omit the
new test, (b) add the new test and leave the function undefined, a diagnostic
link error, or you can re-declare the function with QEMU_ERROR.

> @@ -28081,9 +28085,7 @@ void mips_tcg_init(void)
>  cpu_llval = tcg_global_mem_new(cpu_env, offsetof(CPUMIPSState, llval),
> "llval");
>  
> -#if !defined(TARGET_MIPS64)
>  mxu_translate_init();
> -#endif /* !TARGET_MIPS64 */

This one won't be eliminated, and is an abort for MIPS64 per patch 8, so all
mips64 now aborts on startup.

r~

Re: [PATCH v3 08/10] target/mips: Make mxu_translate_init() / decode_ase_mxu() proto public

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> +#else /* !defined(TARGET_MIPS64) */
> +void mxu_translate_init(void)
> +{
> +g_assert_not_reached();
> +}

This is suspect, see next patch.
The rest of it seems ok.

r~

Re: [PATCH v3 07/10] target/mips: Introduce mxu_translate_init() helper

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> Extract the MXU register initialization code from mips_tcg_init()
> as a new mxu_translate_init() helper
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/translate.c | 26 +++---
>  1 file changed, 15 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 06/10] target/mips: Use OPC_MUL instead of OPC__MXU_MUL

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> We already have a macro and definition to extract / check
> the Special2 MUL opcode. Use it instead of the unnecessary
> OPC__MXU_MUL macro.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/translate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 05/10] target/mips: Extract decode_ase_mxu() from decode_opc_mxu()

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> To easily convert MXU code to decodetree, extract decode_ase_mxu()
> from decode_opc_mxu(), making it return a boolean.
> We will keep decode_opc_mxu() in the translate.c unit because it
> calls gen_arith().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/translate.c | 45 -
>  1 file changed, 26 insertions(+), 19 deletions(-)

I guess the split is fine until you can just call gen_mul() from a trans() from
the mxu decodetree.

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 04/10] target/mips: Pass instruction opcode to decode_opc_mxu()

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> In the next commit we'll make decode_opc_mxu() match decodetree
> prototype by returning a boolean. First pass ctx->opcode as an
> argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/translate.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 03/10] target/mips: Remove unused CPUMIPSState* from MXU functions

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> None of these MXU functions use their CPUMIPSState* env argument,
> remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/translate.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 01/10] target/mips: Rewrite complex ifdef'ry

2021-02-23 Thread Richard Henderson

On 2/22/21 2:38 PM, Philippe Mathieu-Daudé wrote:
> No need for this obfuscated ifdef'ry, KISS.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/translate.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2] target/riscv: fix TB_FLAGS bits overlapping bug for rvv/rvh

2021-02-23 Thread Frank Chang

On Wed, Feb 24, 2021 at 2:24 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 2/23/21 12:19 AM, frank.ch...@sifive.com wrote:
> > From: Frank Chang 
> >
> > TB_FLAGS mem_idx bits was extended from 2 bits to 3 bits in
> > commit: c445593, but other TB_FLAGS bits for rvv and rvh were
> > not shift as well so these bits may overlap with each other when
> > rvv is enabled.
> >
> > Signed-off-by: Frank Chang 
>
> Reviewed-by: Richard Henderson 
>
> > -#define TB_FLAGS_MMU_MASK   7
> >  #define TB_FLAGS_PRIV_MMU_MASK3
> >  #define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
> >  #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
> ...
> > +FIELD(TB_FLAGS, MEM_IDX, 0, 3)
> > +FIELD(TB_FLAGS, VL_EQ_VLMAX, 3, 1)
> > +FIELD(TB_FLAGS, LMUL, 4, 2)
> > +FIELD(TB_FLAGS, SEW, 6, 3)
> > +FIELD(TB_FLAGS, VILL, 9, 1)
> >  /* Is a Hypervisor instruction load/store allowed? */
> > -FIELD(TB_FLAGS, HLSX, 9, 1)
> > +FIELD(TB_FLAGS, HLSX, 10, 1)
>
> The only other thing that I'd add at this point is a comment about
> MSTATUS_FS
> -- a 2-bit field at bit 13 -- for the benefit of the next person that adds
> something to TB_FLAGS.
>
>
In fact, in RVV patchset, both MSTATUS_FS and MSTATUS_VS are skipped
and I also add the comments to state that.
The bits are also rearranged to fill the empty bit holes in RVV patchset on
my local branch:

  FIELD(TB_FLAGS, MEM_IDX, 0, 3)
  FIELD(TB_FLAGS, LMUL, 3, 3)
  FIELD(TB_FLAGS, SEW, 6, 3)
  /* Skip MSTATUS_VS (0x600) bits */
  FIELD(TB_FLAGS, VL_EQ_VLMAX, 11, 1)
  FIELD(TB_FLAGS, VILL, 12, 1)
  /* Skip MSTATUS_FS (0x6000) bits */
  /* Is a Hypervisor instruction load/store allowed? */
  FIELD(TB_FLAGS, HLSX, 15, 1)

Thanks for the review.
Frank Chang


>
> r~
>

Re: [PATCH v2 2/2] target/riscv: Call check_access() before tcg_temp_new()

2021-02-23 Thread Richard Henderson

On 2/22/21 10:49 AM, Alex Richardson wrote:
> ---
>  target/riscv/insn_trans/trans_rvh.c.inc | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

Any particular reason?

At first I thought this was a macro with a hidden return, but it isn't.  I
would understand a cleanup like

if (check_access(ctx)) {
   TCGv t0 = tcg_temp_new();
   TCGv t1 = tcg_temp_new();
   ...

where we do not generate the rest of the code if we raised an exception.
Alternately, if the nesting seems ugly,

if (!check_access(ctx)) {
return true;
}
...
return true;

r~

Re: who's using the ozlabs patchwork install for QEMU patches ?

2021-02-23 Thread Jeremy Kerr

Hi Alexey,
> Jeremy or Stephen (cc-ing) do definitely know if there is a better
> way.

The standard process is to send me an email :)

You're wanting to add user 'groug' to the qemu project, is that
correct?

Cheers,


Jeremy

Re: [PATCH v2 1/2] target/riscv: Reduce duplicated code in trans_rvh.c.inc

2021-02-23 Thread Richard Henderson

On 2/22/21 10:49 AM, Alex Richardson wrote:
> I rencently merged CHERI QEMU up to 5.2, and we have to modify all
> functions that perform memory accesses. Factoring these almost-indentical
> functions into two shared helpers makes our changes a lot smaller and
> should also make this code easier to maintain.
> ---
>  target/riscv/insn_trans/trans_rvh.c.inc | 199 
>  1 file changed, 29 insertions(+), 170 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

2021-02-23 Thread Richard Henderson

On 2/23/21 3:43 PM, Philippe Mathieu-Daudé wrote:
> On 2/23/21 7:51 PM, Richard Henderson wrote:
>> I just want the file naming done correctly, while you're renaming.  That is
>> something you are actively changing in this patch set, so we should get it 
>> right.
> 
> So what is the new directory structure?
> 
> - user/
> - sysemu/{tcg,kvm,}/
> 
> or
> 
> - tcg/a-user.c
> - tcg/b-sysemu.c
> - kvm/kvm.c

Personally I think this second one makes more sense, focused primarily on the
accelerator and secondarily on the kind of emulation.


r~

1 2 3 >

1 - 100 of 278 matches

Mail list logo