Re: [PATCH v2 0/4] bulk: Replace TARGET_FMT_plx by HWADDR_PRIx

[Thomas Huth]

On 11/01/2023 09.39, Philippe Mathieu-Daudé wrote:

Since v1:
- Fix checkpatch style violations
- Use HWADDR_PRIx instead of HWADDR_FMT_plx (Zoltan)

Supersedes: <20230110212947.34557-1-phi...@linaro.org>
 "bulk: Rename TARGET_FMT_plx -> HWADDR_FMT_plx"

Philippe Mathieu-Daudé (4):
   hw: Remove hardcoded tabs (code style)
   bulk: Coding style fixes
   bulk: Replace TARGET_FMT_plx -> HWADDR_PRIx
   bulk: Prefix '0x' to hex values displayed with HWADDR_PRIx format


Big sorry, I picked up v1 for my last pull request before I saw that there 
is a v2. But IMHO it's ok to have a separate macro with a %016 included, so 
I'd rather tend to keep HWADDR_FMT_plx. Anyway, if you consider the other 
changes in your series important enough, please rebase them. Sorry again for 
the additional work that this might cause.


 Thomas

Re: [PATCH v4 12/19] target/hexagon: Clean up includes

[Markus Armbruster]

Taylor Simpson  writes:

>> -Original Message-
>> From: Markus Armbruster 
>> Sent: Thursday, January 19, 2023 1:00 AM
>> To: qemu-de...@nongnu.org
>> Cc: richard.hender...@linaro.org; pbonz...@redhat.com;
>> kw...@redhat.com; hre...@redhat.com; i...@bsdimp.com;
>> kev...@freebsd.org; berra...@redhat.com; gr...@kaod.org;
>> qemu_...@crudebyte.com; m...@redhat.com; phi...@linaro.org;
>> peter.mayd...@linaro.org; alist...@alistair23.me; jasow...@redhat.com;
>> jonathan.came...@huawei.com; kbast...@mail.uni-paderborn.de;
>> quint...@redhat.com; dgilb...@redhat.com; michael.r...@amd.com;
>> kkost...@redhat.com; Taylor Simpson ;
>> pal...@dabbelt.com; bin.m...@windriver.com; qemu-block@nongnu.org;
>> qemu-...@nongnu.org; qemu-ri...@nongnu.org
>> Subject: [PATCH v4 12/19] target/hexagon: Clean up includes
>> 
>> Clean up includes so that osdep.h is included first and headers which it
>> implies are not included manually.
>> 
>> This commit was created with scripts/clean-includes.
>> 
>> Changes to standalone programs dropped, because I can't tell whether them
>> not using qemu/osdep.h is intentional:
>> 
>> target/hexagon/gen_dectree_import.c
>> target/hexagon/gen_semantics.c
>> target/hexagon/idef-parser/idef-parser.h
>> target/hexagon/idef-parser/parser-helpers.c
>> target/hexagon/idef-parser/parser-helpers.h
>
> Correct.  These are standalone programs not built with the full QEMU context.

I'll tweak the commit message like this:

   Changes to standalone programs dropped, because these intentionally
   don't use qemu/osdep.h:

>> Signed-off-by: Markus Armbruster 
>> ---
>>  target/hexagon/hex_arch_types.h | 1 -
>>  target/hexagon/mmvec/macros.h   | 1 -
>>  2 files changed, 2 deletions(-)
>
> Reviewed-by: Taylor Simpson 

Thanks!

Re: [PATCH v4 10/19] migration: Clean up includes

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Dr. David Alan Gilbert"  writes:
>> 
>> > * Markus Armbruster (arm...@redhat.com) wrote:
>> >> Clean up includes so that osdep.h is included first and headers
>> >> which it implies are not included manually.
>> >
>> > That change doesn't seem to match the message; the patch is removing the
>> > osdep.h include.
>> 
>> It's the commit message scripts/clean-includes creates :)
>> 
>> I can throw in another patch to the script so it mentions it also
>> removes qemu/osdep.h from headers.
>
> Oh hmm it would be clearer;

What about

$GITSUBJ: Clean up includes

Clean up includes so that osdep.h is included first in .c and not in
.h, and headers which it implies are not included manually.

This commit was created with scripts/clean-includes.


> but OK then, so 
>
> Reviewed-by: Dr. David Alan Gilbert 

Thanks!

Re: [PATCH v4 13/19] riscv: Clean up includes

[Alistair Francis]

On Thu, Jan 19, 2023 at 5:10 PM Markus Armbruster  wrote:
>
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
>
> This commit was created with scripts/clean-includes.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/pmu.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 3004ce37b6..0c819ca983 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -16,7 +16,6 @@
>   * this program.  If not, see .
>   */
>
> -#include "qemu/osdep.h"
>  #include "qemu/log.h"
>  #include "cpu.h"
>  #include "qemu/main-loop.h"
> --
> 2.39.0
>
>

RE: [PATCH v4 12/19] target/hexagon: Clean up includes

[Taylor Simpson]

> -Original Message-
> From: Markus Armbruster 
> Sent: Thursday, January 19, 2023 1:00 AM
> To: qemu-de...@nongnu.org
> Cc: richard.hender...@linaro.org; pbonz...@redhat.com;
> kw...@redhat.com; hre...@redhat.com; i...@bsdimp.com;
> kev...@freebsd.org; berra...@redhat.com; gr...@kaod.org;
> qemu_...@crudebyte.com; m...@redhat.com; phi...@linaro.org;
> peter.mayd...@linaro.org; alist...@alistair23.me; jasow...@redhat.com;
> jonathan.came...@huawei.com; kbast...@mail.uni-paderborn.de;
> quint...@redhat.com; dgilb...@redhat.com; michael.r...@amd.com;
> kkost...@redhat.com; Taylor Simpson ;
> pal...@dabbelt.com; bin.m...@windriver.com; qemu-block@nongnu.org;
> qemu-...@nongnu.org; qemu-ri...@nongnu.org
> Subject: [PATCH v4 12/19] target/hexagon: Clean up includes
> 
> Clean up includes so that osdep.h is included first and headers which it
> implies are not included manually.
> 
> This commit was created with scripts/clean-includes.
> 
> Changes to standalone programs dropped, because I can't tell whether them
> not using qemu/osdep.h is intentional:
> 
> target/hexagon/gen_dectree_import.c
> target/hexagon/gen_semantics.c
> target/hexagon/idef-parser/idef-parser.h
> target/hexagon/idef-parser/parser-helpers.c
> target/hexagon/idef-parser/parser-helpers.h

Correct.  These are standalone programs not built with the full QEMU context.

> 
> Signed-off-by: Markus Armbruster 
> ---
>  target/hexagon/hex_arch_types.h | 1 -
>  target/hexagon/mmvec/macros.h   | 1 -
>  2 files changed, 2 deletions(-)

Reviewed-by: Taylor Simpson 

[PATCH] virtio-scsi: reset SCSI devices from main loop thread

[Stefan Hajnoczi]

When an IOThread is configured, the ctrl virtqueue is processed in the
IOThread. TMFs that reset SCSI devices are currently called directly
from the IOThread and trigger an assertion failure in blk_drain():

  ../block/block-backend.c:1780: void blk_drain(BlockBackend *): Assertion 
`qemu_in_main_thread()' failed.

The blk_drain() function is not designed to be called from an IOThread
because it needs the Big QEMU Lock (BQL).

This patch defers TMFs that reset SCSI devices to a Bottom Half (BH)
that runs in the main loop thread under the BQL. This way it's safe to
call blk_drain() and the assertion failure is avoided.

Introduce s->tmf_bh_list for tracking TMF requests that have been
deferred to the BH. When the BH runs it will grab the entire list and
process all requests. Care must be taken to clear the list when the
virtio-scsi device is reset or unrealized. Otherwise deferred TMF
requests could execute later and lead to use-after-free or other
undefined behavior.

The s->resetting counter that's used by TMFs that reset SCSI devices is
accessed from multiple threads. This patch makes that explicit by using
atomic accessor functions. With this patch applied the counter is only
modified by the main loop thread under the BQL but can be read by any
thread.

Reported-by: Qing Wang 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/virtio/virtio-scsi.h |  11 ++-
 hw/scsi/virtio-scsi.c   | 169 +---
 2 files changed, 143 insertions(+), 37 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 37b75e15e3..779568ab5d 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -74,13 +74,22 @@ struct VirtIOSCSICommon {
 VirtQueue **cmd_vqs;
 };
 
+struct VirtIOSCSIReq;
+
 struct VirtIOSCSI {
 VirtIOSCSICommon parent_obj;
 
 SCSIBus bus;
-int resetting;
+int resetting; /* written from main loop thread, read from any thread */
 bool events_dropped;
 
+/*
+ * TMFs deferred to main loop BH. These fields are protected by
+ * virtio_scsi_acquire().
+ */
+QEMUBH *tmf_bh;
+QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list;
+
 /* Fields for dataplane below */
 AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 2b649ca976..612c525d9d 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -43,13 +43,11 @@ typedef struct VirtIOSCSIReq {
 QEMUSGList qsgl;
 QEMUIOVector resp_iov;
 
-union {
-/* Used for two-stage request submission */
-QTAILQ_ENTRY(VirtIOSCSIReq) next;
+/* Used for two-stage request submission and TMFs deferred to BH */
+QTAILQ_ENTRY(VirtIOSCSIReq) next;
 
-/* Used for cancellation of request during TMFs */
-int remaining;
-};
+/* Used for cancellation of request during TMFs */
+int remaining;
 
 SCSIRequest *sreq;
 size_t resp_size;
@@ -294,6 +292,122 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, 
SCSIDevice *d)
 }
 }
 
+static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req)
+{
+VirtIOSCSI *s = req->dev;
+SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
+BusChild *kid;
+int target;
+
+switch (req->req.tmf.subtype) {
+case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET:
+if (!d) {
+req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+goto out;
+}
+if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
+req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+goto out;
+}
+qatomic_inc(>resetting);
+device_cold_reset(>qdev);
+qatomic_dec(>resetting);
+break;
+
+case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
+target = req->req.tmf.lun[1];
+qatomic_inc(>resetting);
+
+rcu_read_lock();
+QTAILQ_FOREACH_RCU(kid, >bus.qbus.children, sibling) {
+SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+if (d1->channel == 0 && d1->id == target) {
+device_cold_reset(>qdev);
+}
+}
+rcu_read_unlock();
+
+qatomic_dec(>resetting);
+break;
+
+default:
+g_assert_not_reached();
+break;
+}
+
+out:
+object_unref(OBJECT(d));
+
+virtio_scsi_acquire(s);
+virtio_scsi_complete_req(req);
+virtio_scsi_release(s);
+}
+
+/* Some TMFs must be processed from the main loop thread */
+static void virtio_scsi_do_tmf_bh(void *opaque)
+{
+VirtIOSCSI *s = opaque;
+QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
+VirtIOSCSIReq *req;
+VirtIOSCSIReq *tmp;
+
+GLOBAL_STATE_CODE();
+
+virtio_scsi_acquire(s);
+
+QTAILQ_FOREACH_SAFE(req, >tmf_bh_list, next, tmp) {
+QTAILQ_REMOVE(>tmf_bh_list, req, next);
+QTAILQ_INSERT_TAIL(, req, next);
+}
+
+qemu_bh_delete(s->tmf_bh);

Re: [PATCH v2 00/12] qemu-img info: Show protocol-level information

[Kevin Wolf]

Am 08.12.2022 um 13:24 hat Hanna Reitz geschrieben:
> On 20.06.22 18:26, Hanna Reitz wrote:
> > Hi,
> > 
> > This series is a v2 to:
> > 
> > https://lists.nongnu.org/archive/html/qemu-block/2022-05/msg00042.html
> 
> Ping, it looks like this still applies (to the master branch and kevin’s
> block-next branch at least).

Not any more. :-)

But the conflicts seemed obvious enough, so I rebased it (including
changing the "Since: 7.1" occurrences to 8.0) and applied it to my block
branch. Thanks!

Kevin

Re: [PATCH v4 04/19] bsd-user: Clean up includes

[Warner Losh]

On Thu, Jan 19, 2023 at 9:42 AM Markus Armbruster  wrote:

> Warner Losh  writes:
>
> > On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster 
> > wrote:
> >
> >> Clean up includes so that osdep.h is included first and headers
> >> which it implies are not included manually.
> >>
> >> This commit was created with scripts/clean-includes.
> >>
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  bsd-user/bsd-proc.h   | 4 
> >>  bsd-user/qemu.h   | 1 -
> >>  bsd-user/arm/signal.c | 1 +
> >>  bsd-user/arm/target_arch_cpu.c| 2 ++
> >>  bsd-user/freebsd/os-sys.c | 1 +
> >>  bsd-user/i386/signal.c| 1 +
> >>  bsd-user/i386/target_arch_cpu.c   | 3 +--
> >>  bsd-user/main.c   | 4 +---
> >>  bsd-user/strace.c | 1 -
> >>  bsd-user/x86_64/signal.c  | 1 +
> >>  bsd-user/x86_64/target_arch_cpu.c | 3 +--
> >>  11 files changed, 9 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
> >> index 68b66e571d..a1061bffb8 100644
> >> --- a/bsd-user/bsd-proc.h
> >> +++ b/bsd-user/bsd-proc.h
> >> @@ -20,11 +20,7 @@
> >>  #ifndef BSD_PROC_H_
> >>  #define BSD_PROC_H_
> >>
> >> -#include 
> >> -#include 
> >> -#include 
> >>  #include 
> >>
> >
> > Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill
> > where all includes are independent, but much of that hasn't been merged
> to
> > 12 yet. sys/types.h, at least, is documented as a prereq for both
> > sys/stat.h and sys/resource.h.
> >
> > I know many of these are in os-dep.h, and I've had trouble in the
> bsd-user
> > fork with that and the layout of the code which has arguably way too much
> > in the .h files.
>
> No, I didn't test on FreeBSD 12.
>

OK. Fair enough. If it passes the CI tests, then it's likely fine (though
if I hit issues, I'll submit patches).


> Any .c must include qemu/osdep.h *first*.  Any further inclusions of
> headers qemu/osdep.h includes already are no-ops unless the headers in
> question lack multiple inclusion guards.
>

OK.


> > Also, why didn't you move sys/resource.h and other such files to
> os-dep.h?
> > I'm struggling to understand the rules around what is or isn't included
> > where?
>
> This series is "run scripts/clean-includes, split it into reviewable
> chunks, tidy up blank lines".  Moving more includes into qemu/osdep.h is
> out of scope.
>

OK. Fair point. I'm happy with that answer since it tells me I could move
things there too, if need be.


> I sympathize with your complaint that QEMU does too much in headers in
> general, and in qemu/osdep.h in particular.
>

Yea, I'm not entirely sure  it's a complaint, or if it's an observation of
difficulties relative to other code bases... I go back and forth on my
opinion of it...


> >> -#include 
> >>
> >>  /* exit(2) */
> >>  static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
> >> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> >> index be6105385e..0ceecfb6df 100644
> >> --- a/bsd-user/qemu.h
> >> +++ b/bsd-user/qemu.h
> >> @@ -17,7 +17,6 @@
> >>  #ifndef QEMU_H
> >>  #define QEMU_H
> >>
> >> -#include "qemu/osdep.h"
> >>  #include "cpu.h"
> >>  #include "qemu/units.h"
> >>  #include "exec/cpu_ldst.h"
> >> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> >> index 2b1dd745d1..9734407543 100644
> >> --- a/bsd-user/arm/signal.c
> >> +++ b/bsd-user/arm/signal.c
> >> @@ -17,6 +17,7 @@
> >>   *  along with this program; if not, see  >.
> >>   */
> >>
> >> +#include "qemu/osdep.h"
> >>  #include "qemu.h"
> >>
> >>  /*
> >> diff --git a/bsd-user/arm/target_arch_cpu.c
> >> b/bsd-user/arm/target_arch_cpu.c
> >> index 02bf9149d5..fe38ae2210 100644
> >> --- a/bsd-user/arm/target_arch_cpu.c
> >> +++ b/bsd-user/arm/target_arch_cpu.c
> >> @@ -16,6 +16,8 @@
> >>   *  You should have received a copy of the GNU General Public License
> >>   *  along with this program; if not, see  >.
> >>   */
> >> +
> >> +#include "qemu/osdep.h"
> >>  #include "target_arch.h"
> >>
> >>  void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
> >> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> >> index 309e27b9d6..1676ec10f8 100644
> >> --- a/bsd-user/freebsd/os-sys.c
> >> +++ b/bsd-user/freebsd/os-sys.c
> >> @@ -17,6 +17,7 @@
> >>   *  along with this program; if not, see  >.
> >>   */
> >>
> >> +#include "qemu/osdep.h"
> >>  #include "qemu.h"
> >>  #include "target_arch_sysarch.h"
> >>
> >> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
> >> index 5dd975ce56..a3131047b8 100644
> >> --- a/bsd-user/i386/signal.c
> >> +++ b/bsd-user/i386/signal.c
> >> @@ -17,6 +17,7 @@
> >>   *  along with this program; if not, see  >.
> >>   */
> >>
> >> +#include "qemu/osdep.h"
> >>  #include "qemu.h"
> >>
> >>  /*
> >> diff --git a/bsd-user/i386/target_arch_cpu.c
> >> 

Re: [PATCH v4 04/19] bsd-user: Clean up includes

[Markus Armbruster]

Warner Losh  writes:

> On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster 
> wrote:
>
>> Clean up includes so that osdep.h is included first and headers
>> which it implies are not included manually.
>>
>> This commit was created with scripts/clean-includes.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  bsd-user/bsd-proc.h   | 4 
>>  bsd-user/qemu.h   | 1 -
>>  bsd-user/arm/signal.c | 1 +
>>  bsd-user/arm/target_arch_cpu.c| 2 ++
>>  bsd-user/freebsd/os-sys.c | 1 +
>>  bsd-user/i386/signal.c| 1 +
>>  bsd-user/i386/target_arch_cpu.c   | 3 +--
>>  bsd-user/main.c   | 4 +---
>>  bsd-user/strace.c | 1 -
>>  bsd-user/x86_64/signal.c  | 1 +
>>  bsd-user/x86_64/target_arch_cpu.c | 3 +--
>>  11 files changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
>> index 68b66e571d..a1061bffb8 100644
>> --- a/bsd-user/bsd-proc.h
>> +++ b/bsd-user/bsd-proc.h
>> @@ -20,11 +20,7 @@
>>  #ifndef BSD_PROC_H_
>>  #define BSD_PROC_H_
>>
>> -#include 
>> -#include 
>> -#include 
>>  #include 
>>
>
> Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill
> where all includes are independent, but much of that hasn't been merged to
> 12 yet. sys/types.h, at least, is documented as a prereq for both
> sys/stat.h and sys/resource.h.
>
> I know many of these are in os-dep.h, and I've had trouble in the bsd-user
> fork with that and the layout of the code which has arguably way too much
> in the .h files.

No, I didn't test on FreeBSD 12.

Any .c must include qemu/osdep.h *first*.  Any further inclusions of
headers qemu/osdep.h includes already are no-ops unless the headers in
question lack multiple inclusion guards.

> Also, why didn't you move sys/resource.h and other such files to os-dep.h?
> I'm struggling to understand the rules around what is or isn't included
> where?

This series is "run scripts/clean-includes, split it into reviewable
chunks, tidy up blank lines".  Moving more includes into qemu/osdep.h is
out of scope.

I sympathize with your complaint that QEMU does too much in headers in
general, and in qemu/osdep.h in particular.

>> -#include 
>>
>>  /* exit(2) */
>>  static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
>> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
>> index be6105385e..0ceecfb6df 100644
>> --- a/bsd-user/qemu.h
>> +++ b/bsd-user/qemu.h
>> @@ -17,7 +17,6 @@
>>  #ifndef QEMU_H
>>  #define QEMU_H
>>
>> -#include "qemu/osdep.h"
>>  #include "cpu.h"
>>  #include "qemu/units.h"
>>  #include "exec/cpu_ldst.h"
>> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
>> index 2b1dd745d1..9734407543 100644
>> --- a/bsd-user/arm/signal.c
>> +++ b/bsd-user/arm/signal.c
>> @@ -17,6 +17,7 @@
>>   *  along with this program; if not, see .
>>   */
>>
>> +#include "qemu/osdep.h"
>>  #include "qemu.h"
>>
>>  /*
>> diff --git a/bsd-user/arm/target_arch_cpu.c
>> b/bsd-user/arm/target_arch_cpu.c
>> index 02bf9149d5..fe38ae2210 100644
>> --- a/bsd-user/arm/target_arch_cpu.c
>> +++ b/bsd-user/arm/target_arch_cpu.c
>> @@ -16,6 +16,8 @@
>>   *  You should have received a copy of the GNU General Public License
>>   *  along with this program; if not, see .
>>   */
>> +
>> +#include "qemu/osdep.h"
>>  #include "target_arch.h"
>>
>>  void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
>> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
>> index 309e27b9d6..1676ec10f8 100644
>> --- a/bsd-user/freebsd/os-sys.c
>> +++ b/bsd-user/freebsd/os-sys.c
>> @@ -17,6 +17,7 @@
>>   *  along with this program; if not, see .
>>   */
>>
>> +#include "qemu/osdep.h"
>>  #include "qemu.h"
>>  #include "target_arch_sysarch.h"
>>
>> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
>> index 5dd975ce56..a3131047b8 100644
>> --- a/bsd-user/i386/signal.c
>> +++ b/bsd-user/i386/signal.c
>> @@ -17,6 +17,7 @@
>>   *  along with this program; if not, see .
>>   */
>>
>> +#include "qemu/osdep.h"
>>  #include "qemu.h"
>>
>>  /*
>> diff --git a/bsd-user/i386/target_arch_cpu.c
>> b/bsd-user/i386/target_arch_cpu.c
>> index d349e45299..2a3af2ddef 100644
>> --- a/bsd-user/i386/target_arch_cpu.c
>> +++ b/bsd-user/i386/target_arch_cpu.c
>> @@ -17,9 +17,8 @@
>>   *  along with this program; if not, see .
>>   */
>>
>> -#include 
>> -
>>  #include "qemu/osdep.h"
>> +
>>  #include "cpu.h"
>>  #include "qemu.h"
>>  #include "qemu/timer.h"
>> diff --git a/bsd-user/main.c b/bsd-user/main.c
>> index 6f09180d65..41290e16f9 100644
>> --- a/bsd-user/main.c
>> +++ b/bsd-user/main.c
>> @@ -18,12 +18,10 @@
>>   *  along with this program; if not, see .
>>   */
>>
>> -#include 
>> -#include 
>> +#include "qemu/osdep.h"
>>  #include 
>>  #include 
>>
>> -#include 

Re: [PATCH v2 03/12] block/vmdk: Change extent info type

[Kevin Wolf]

Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> VMDK's implementation of .bdrv_get_specific_info() returns information
> about its extent files, ostensibly in the form of ImageInfo objects.
> However, it does not get this information through
> bdrv_query_image_info(), but fills only a select few fields with custom
> information that does not always match the fields' purposes.
> 
> For example, @format, which is supposed to be a block driver name, is
> filled with the extent type, e.g. SPARSE or FLAT.
> 
> In ImageInfo, @compressed shows whether the data that can be seen in the
> image is stored in compressed form or not.  For example, a compressed
> qcow2 image will store compressed data in its data file, but when
> accessing the qcow2 node, you will see normal data.  This is not how
> VMDK uses the @compressed field for its extent files: Instead, it
> signifies whether accessing the extent file will yield compressed data
> (which the VMDK driver then (de-)compresses).

Actually, VMDK was the only user of the field in ImageInfo. qcow2
doesn't set the field at all because it would have to parse all L2
tables in order to set it.

So I suppose @compressed can now be removed from ImageInfo?

Kevin

Re: [PATCH v4 04/19] bsd-user: Clean up includes

[Warner Losh]

On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster 
wrote:

> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
>
> This commit was created with scripts/clean-includes.
>
> Signed-off-by: Markus Armbruster 
> ---
>  bsd-user/bsd-proc.h   | 4 
>  bsd-user/qemu.h   | 1 -
>  bsd-user/arm/signal.c | 1 +
>  bsd-user/arm/target_arch_cpu.c| 2 ++
>  bsd-user/freebsd/os-sys.c | 1 +
>  bsd-user/i386/signal.c| 1 +
>  bsd-user/i386/target_arch_cpu.c   | 3 +--
>  bsd-user/main.c   | 4 +---
>  bsd-user/strace.c | 1 -
>  bsd-user/x86_64/signal.c  | 1 +
>  bsd-user/x86_64/target_arch_cpu.c | 3 +--
>  11 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
> index 68b66e571d..a1061bffb8 100644
> --- a/bsd-user/bsd-proc.h
> +++ b/bsd-user/bsd-proc.h
> @@ -20,11 +20,7 @@
>  #ifndef BSD_PROC_H_
>  #define BSD_PROC_H_
>
> -#include 
> -#include 
> -#include 
>  #include 
>

Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill
where all includes are independent, but much of that hasn't been merged to
12 yet. sys/types.h, at least, is documented as a prereq for both
sys/stat.h and sys/resource.h.

I know many of these are in os-dep.h, and I've had trouble in the bsd-user
fork with that and the layout of the code which has arguably way too much
in the .h files.

Also, why didn't you move sys/resource.h and other such files to os-dep.h?
I'm struggling to understand the rules around what is or isn't included
where?


> -#include 
>
>  /* exit(2) */
>  static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index be6105385e..0ceecfb6df 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -17,7 +17,6 @@
>  #ifndef QEMU_H
>  #define QEMU_H
>
> -#include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "qemu/units.h"
>  #include "exec/cpu_ldst.h"
> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> index 2b1dd745d1..9734407543 100644
> --- a/bsd-user/arm/signal.c
> +++ b/bsd-user/arm/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>
>  /*
> diff --git a/bsd-user/arm/target_arch_cpu.c
> b/bsd-user/arm/target_arch_cpu.c
> index 02bf9149d5..fe38ae2210 100644
> --- a/bsd-user/arm/target_arch_cpu.c
> +++ b/bsd-user/arm/target_arch_cpu.c
> @@ -16,6 +16,8 @@
>   *  You should have received a copy of the GNU General Public License
>   *  along with this program; if not, see .
>   */
> +
> +#include "qemu/osdep.h"
>  #include "target_arch.h"
>
>  void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> index 309e27b9d6..1676ec10f8 100644
> --- a/bsd-user/freebsd/os-sys.c
> +++ b/bsd-user/freebsd/os-sys.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  #include "target_arch_sysarch.h"
>
> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
> index 5dd975ce56..a3131047b8 100644
> --- a/bsd-user/i386/signal.c
> +++ b/bsd-user/i386/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>
>  /*
> diff --git a/bsd-user/i386/target_arch_cpu.c
> b/bsd-user/i386/target_arch_cpu.c
> index d349e45299..2a3af2ddef 100644
> --- a/bsd-user/i386/target_arch_cpu.c
> +++ b/bsd-user/i386/target_arch_cpu.c
> @@ -17,9 +17,8 @@
>   *  along with this program; if not, see .
>   */
>
> -#include 
> -
>  #include "qemu/osdep.h"
> +
>  #include "cpu.h"
>  #include "qemu.h"
>  #include "qemu/timer.h"
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 6f09180d65..41290e16f9 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -18,12 +18,10 @@
>   *  along with this program; if not, see .
>   */
>
> -#include 
> -#include 
> +#include "qemu/osdep.h"
>  #include 
>  #include 
>
> -#include "qemu/osdep.h"
>  #include "qemu/help-texts.h"
>  #include "qemu/units.h"
>  #include "qemu/accel.h"
> diff --git a/bsd-user/strace.c b/bsd-user/strace.c
> index a77d10dd6b..96499751eb 100644
> --- a/bsd-user/strace.c
> +++ b/bsd-user/strace.c
> @@ -20,7 +20,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>
>  #include "qemu.h"
>
> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
> index c3875bc4c6..46cb865180 100644
> --- a/bsd-user/x86_64/signal.c
> +++ b/bsd-user/x86_64/signal.c
> @@ -16,6 +16,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>
>  /*
> 

Re: completion timeouts with pin-based interrupts in QEMU hw/nvme

[Guenter Roeck]

On 1/19/23 00:02, Klaus Jensen wrote:

On Jan 19 08:28, Klaus Jensen wrote:

On Jan 18 21:03, Keith Busch wrote:

On Thu, Jan 19, 2023 at 01:10:57PM +1000, Alistair Francis wrote:

On Thu, Jan 19, 2023 at 12:44 PM Keith Busch  wrote:


Further up, it says the "interrupt gateway" is responsible for
forwarding new interrupt requests while the level remains asserted, but
it doesn't look like anything is handling that, which essentially turns
this into an edge interrupt. Am I missing something, or is this really
not being handled?


Yeah, that wouldn't be handled. In QEMU the PLIC relies on QEMUs
internal GPIO lines to trigger an interrupt. So with the current setup
we only support edge triggered interrupts.


Thanks for confirming!

Klaus,
I think we can justify introducing a work-around in the emulated device
now. My previous proposal with pci_irq_pulse() is no good since it does
assert+deassert, but it needs to be the other way around, so please
don't considert that one.

Also, we ought to revisit the intms/intmc usage in the linux driver for
threaded interrupts.


+CC: qemu-riscv

Keith,

Thanks for digging into this!

Yeah, you are probably right that we should only use the intms/intmc
changes in the use_threaded_interrupts case, not in general. While my
RFC patch does seem to "fix" this, it is just a workaround as your
analysis indicate. >

+CC: Philippe,

I am observing these timeouts/aborts on mips as well, so I guess that
emulation could suffer from the same issue?


I suspect it does. In my case, I have an Ethernet controller on the same
interrupt line as the NVME controller, and it looks like one of the
interrupts gets lost if both controllers raise an interrupt at the same
time. The suggested workarounds "fix" the problem, but that doesn't seem
to be the correct solution.

Guenter

Re: [PATCH v4 19/19] Drop duplicate #include

[Greg Kurz]

On Thu, 19 Jan 2023 07:59:59 +0100
Markus Armbruster  wrote:

> Tracked down with the help of scripts/clean-includes.
> 
> Signed-off-by: Markus Armbruster 
> ---

For ppc changes.

Reviewed-by: Greg Kurz 

>  include/hw/arm/fsl-imx6ul.h   | 1 -
>  include/hw/arm/fsl-imx7.h | 1 -
>  backends/tpm/tpm_emulator.c   | 1 -
>  hw/acpi/piix4.c   | 1 -
>  hw/alpha/dp264.c  | 1 -
>  hw/arm/virt.c | 1 -
>  hw/arm/xlnx-versal.c  | 1 -
>  hw/block/pflash_cfi01.c   | 1 -
>  hw/core/machine.c | 1 -
>  hw/hppa/machine.c | 1 -
>  hw/i386/acpi-build.c  | 1 -
>  hw/loongarch/acpi-build.c | 1 -
>  hw/misc/macio/cuda.c  | 1 -
>  hw/misc/macio/pmu.c   | 1 -
>  hw/net/xilinx_axienet.c   | 1 -
>  hw/ppc/ppc405_uc.c| 2 --
>  hw/ppc/ppc440_bamboo.c| 1 -
>  hw/ppc/spapr_drc.c| 1 -
>  hw/rdma/vmw/pvrdma_dev_ring.c | 1 -
>  hw/remote/machine.c   | 1 -
>  hw/remote/remote-obj.c| 1 -
>  hw/rtc/mc146818rtc.c  | 1 -
>  hw/s390x/virtio-ccw-serial.c  | 1 -
>  migration/postcopy-ram.c  | 2 --
>  softmmu/dirtylimit.c  | 1 -
>  softmmu/runstate.c| 1 -
>  softmmu/vl.c  | 1 -
>  target/loongarch/translate.c  | 1 -
>  target/mips/tcg/translate.c   | 1 -
>  target/nios2/translate.c  | 2 --
>  tests/unit/test-cutils.c  | 1 -
>  ui/gtk.c  | 1 -
>  util/oslib-posix.c| 4 
>  33 files changed, 39 deletions(-)
> 
> diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
> index 7812e516a5..1952cb984d 100644
> --- a/include/hw/arm/fsl-imx6ul.h
> +++ b/include/hw/arm/fsl-imx6ul.h
> @@ -30,7 +30,6 @@
>  #include "hw/timer/imx_gpt.h"
>  #include "hw/timer/imx_epit.h"
>  #include "hw/i2c/imx_i2c.h"
> -#include "hw/gpio/imx_gpio.h"
>  #include "hw/sd/sdhci.h"
>  #include "hw/ssi/imx_spi.h"
>  #include "hw/net/imx_fec.h"
> diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
> index 4e5e071864..355bd8ea83 100644
> --- a/include/hw/arm/fsl-imx7.h
> +++ b/include/hw/arm/fsl-imx7.h
> @@ -32,7 +32,6 @@
>  #include "hw/timer/imx_gpt.h"
>  #include "hw/timer/imx_epit.h"
>  #include "hw/i2c/imx_i2c.h"
> -#include "hw/gpio/imx_gpio.h"
>  #include "hw/sd/sdhci.h"
>  #include "hw/ssi/imx_spi.h"
>  #include "hw/net/imx_fec.h"
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 49cc3d749d..2b440d2c9a 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -35,7 +35,6 @@
>  #include "sysemu/runstate.h"
>  #include "sysemu/tpm_backend.h"
>  #include "sysemu/tpm_util.h"
> -#include "sysemu/runstate.h"
>  #include "tpm_int.h"
>  #include "tpm_ioctl.h"
>  #include "migration/blocker.h"
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 0a81f1ad93..df39f91294 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -35,7 +35,6 @@
>  #include "sysemu/xen.h"
>  #include "qapi/error.h"
>  #include "qemu/range.h"
> -#include "hw/acpi/pcihp.h"
>  #include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/cpu.h"
>  #include "hw/hotplug.h"
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index c502c8c62a..4161f559a7 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -18,7 +18,6 @@
>  #include "net/net.h"
>  #include "qemu/cutils.h"
>  #include "qemu/datadir.h"
> -#include "net/net.h"
>  
>  static uint64_t cpu_alpha_superpage_to_phys(void *opaque, uint64_t addr)
>  {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ea2413a0ba..d3849d7233 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -33,7 +33,6 @@
>  #include "qemu/units.h"
>  #include "qemu/option.h"
>  #include "monitor/qdev.h"
> -#include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/boot.h"
>  #include "hw/arm/primecell.h"
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index 57276e1506..69b1b99e93 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -22,7 +22,6 @@
>  #include "hw/misc/unimp.h"
>  #include "hw/arm/xlnx-versal.h"
>  #include "qemu/log.h"
> -#include "hw/sysbus.h"
>  
>  #define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
>  #define XLNX_VERSAL_RCPU_TYPE ARM_CPU_TYPE_NAME("cortex-r5f")
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 0cbc2fb4cb..d11406eada 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -45,7 +45,6 @@
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qemu/bitops.h"
> -#include "qemu/error-report.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 616f3a207c..67cf9f9dcd 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -39,7 +39,6 @@
>  #include "exec/confidential-guest-support.h"
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-pci.h"
> -#include "qom/object_interfaces.h"
>  
>  

Re: [PATCH v2 01/12] block: Improve empty format-specific info dump

[Kevin Wolf]

Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> When a block driver supports obtaining format-specific information, but
> that object only contains optional fields, it is possible that none of
> them are present, so that dump_qobject() (called by
> bdrv_image_info_specific_dump()) will not print anything.
> 
> The callers of bdrv_image_info_specific_dump() put a header above this
> information ("Format specific information:\n"), which will look strange
> when there is nothing below.  Modify bdrv_image_info_specific_dump() to
> print this header instead of its callers, and only if there is indeed
> something to be printed.
> 
> Signed-off-by: Hanna Reitz 

> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 2f0d8ac25a..084ec44d3b 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1819,8 +1819,8 @@ static int info_f(BlockBackend *blk, int argc, char 
> **argv)
>  return -EIO;
>  }
>  if (spec_info) {
> -printf("Format specific information:\n");
> -bdrv_image_info_specific_dump(spec_info);
> +bdrv_image_info_specific_dump(spec_info,
> +  "Format specific information:\n");
>  qapi_free_ImageInfoSpecific(spec_info);
>  }

Interesting observation here: That qemu-io uses printf() instead of
qemu_printf() for the top level, but then dump_qobject() (which results
in qemu_printf()) for the format specific information, means that if you
use the 'qemu-io' HMP command, you'll get half of the output on stdout
and the other half in the monitor.

This series doesn't fix this, but the split makes a little more sense
after this patch at least...

Kevin

Re: [PATCH 2/3] block: do not check bdrv_file_open

[Kevin Wolf]

Am 12.12.2022 um 14:16 hat Paolo Bonzini geschrieben:
> The set of BlockDrivers that have .bdrv_file_open coincides with those
> that have .protocol_name and guess what---checking drv->bdrv_file_open
> is done to see if the driver is a protocol.  So check drv->protocol_name
> instead.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0a625a489a6e..7a66cc2ea23a 100644
> --- a/block.c
> +++ b/block.c
> @@ -911,7 +911,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>  int i;
>  
>  GLOBAL_STATE_CODE();
> -/* TODO Drivers without bdrv_file_open must be specified explicitly */
>  
>  /*
>   * XXX(hch): we really should not let host device detection
> @@ -1618,7 +1617,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> BlockDriver *drv,
>  bs->opaque = g_malloc0(drv->instance_size);
>  
>  assert(!drv->bdrv_needs_filename || bs->filename[0]);
> -if (drv->bdrv_file_open) {
> +if (drv->bdrv_open) {
>  ret = drv->bdrv_file_open(bs, options, open_flags, _err);
>  } else if (drv->bdrv_open) {
>  ret = drv->bdrv_open(bs, options, open_flags, _err);

I suppose you mean drv->protocol_name for the first if condition?

The bug will disappear again after patch 3, but this intermediate state
is very broken.

Kevin

Re: [PATCH-for-8.0] block/nbd: Add missing include

[Kevin Wolf]

Am 25.11.2022 um 18:53 hat Philippe Mathieu-Daudé geschrieben:
> The inlined nbd_readXX() functions call beXX_to_cpu(), themselves
> declared in . This fixes when refactoring:
> 
>   In file included from ../../block/nbd.c:44:
>   include/block/nbd.h: In function 'nbd_read16':
>   include/block/nbd.h:383:12: error: implicit declaration of function 
> 'be16_to_cpu' [-Werror=implicit-function-declaration]
> 383 | *val = be##bits##_to_cpu(*val); 
> \
> |^~
>   include/block/nbd.h:387:1: note: in expansion of macro 'DEF_NBD_READ_N'
> 387 | DEF_NBD_READ_N(16) /* Defines nbd_read16(). */
> | ^~
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Thanks, applied to the block branch.

Kevin

Re: [PATCH v4 16/19] Fix non-first inclusions of qemu/osdep.h

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 19/1/23 12:41, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>>> On 19/1/23 07:59, Markus Armbruster wrote:
 This commit was created with scripts/clean-includes.
 Signed-off-by: Markus Armbruster 
>> [...]
>> 
>>> Up to here:
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé 
>>>
 diff --git a/util/async-teardown.c b/util/async-teardown.c
 index 62bfce1b3c..62cdeb0f20 100644
 --- a/util/async-teardown.c
 +++ b/util/async-teardown.c
 @@ -10,16 +10,12 @@
 * option) any later version.  See the COPYING file in the top-level 
 directory.
 *
 */
 -#include 
 -#include 
 -#include 
 -#include 
 -#include 
 -#include 
 -#include 
 -#include 
  #include "qemu/osdep.h"
 +#include 
 +#include 
 +#include 
 +
#include "qemu/async-teardown.h"
>>>
>>> This file has more changes.
>> I'm not sure I understand.
>> The patch does two related things:
>> 1. It puts qemu/osdep.h first.  The diff makes it look like we leave it
>> in place and move other stuff across, but that's the same.
>> 2. It deletes inclusions of headers qemu/osdep.h already includes:
>>  
>>  
>>  
>>  
>>  
>
> Ah, the other files get this done in the "Drop duplicate #include" patch.

Right!

Re: [PATCH v4 17/19] Don't include headers already included by qemu/osdep.h

[Christian Schoenebeck]

On Thursday, January 19, 2023 7:59:57 AM CET Markus Armbruster wrote:
> This commit was created with scripts/clean-includes.
> 
> Signed-off-by: Markus Armbruster 
> ---

For 9p changes:

Acked-by: Christian Schoenebeck 

>  backends/tpm/tpm_ioctl.h  | 2 --
>  fsdev/p9array.h   | 2 --
>  include/hw/misc/aspeed_lpc.h  | 2 --
>  include/hw/pci/pcie_doe.h | 1 -
>  include/qemu/async-teardown.h | 2 --
>  include/qemu/dbus.h   | 1 -
>  include/qemu/host-utils.h | 1 -
>  include/sysemu/event-loop-base.h  | 1 -
>  accel/tcg/cpu-exec.c  | 1 -
>  hw/9pfs/9p.c  | 2 --
>  hw/display/virtio-gpu-udmabuf.c   | 1 -
>  hw/i2c/pmbus_device.c | 1 -
>  hw/remote/proxy-memory-listener.c | 1 -
>  hw/sensor/adm1272.c   | 1 -
>  hw/usb/dev-storage-bot.c  | 1 -
>  hw/usb/dev-storage-classic.c  | 1 -
>  softmmu/vl.c  | 2 --
>  tcg/tci.c | 1 -
>  tests/unit/test-seccomp.c | 1 -
>  ui/udmabuf.c  | 1 -
>  util/main-loop.c  | 1 -
>  util/oslib-posix.c| 2 --
>  22 files changed, 29 deletions(-)
> 
> diff --git a/backends/tpm/tpm_ioctl.h b/backends/tpm/tpm_ioctl.h
> index e506ef5160..b1d31768a6 100644
> --- a/backends/tpm/tpm_ioctl.h
> +++ b/backends/tpm/tpm_ioctl.h
> @@ -12,8 +12,6 @@
>  # define __USE_LINUX_IOCTL_DEFS
>  #endif
>  
> -#include 
> -#include 
>  #ifndef _WIN32
>  #include 
>  #include 
> diff --git a/fsdev/p9array.h b/fsdev/p9array.h
> index 90e83a7c7b..50a1b15fe9 100644
> --- a/fsdev/p9array.h
> +++ b/fsdev/p9array.h
> @@ -27,8 +27,6 @@
>  #ifndef QEMU_P9ARRAY_H
>  #define QEMU_P9ARRAY_H
>  
> -#include "qemu/compiler.h"
> -
>  /**
>   * P9Array provides a mechanism to access arrays in common C-style (e.g. by
>   * square bracket [] operator) in conjunction with reference variables that
> diff --git a/include/hw/misc/aspeed_lpc.h b/include/hw/misc/aspeed_lpc.h
> index fd228731d2..fa398959af 100644
> --- a/include/hw/misc/aspeed_lpc.h
> +++ b/include/hw/misc/aspeed_lpc.h
> @@ -12,8 +12,6 @@
>  
>  #include "hw/sysbus.h"
>  
> -#include 
> -
>  #define TYPE_ASPEED_LPC "aspeed.lpc"
>  #define ASPEED_LPC(obj) OBJECT_CHECK(AspeedLPCState, (obj), TYPE_ASPEED_LPC)
>  
> diff --git a/include/hw/pci/pcie_doe.h b/include/hw/pci/pcie_doe.h
> index ba4d8b03bd..87dc17dcef 100644
> --- a/include/hw/pci/pcie_doe.h
> +++ b/include/hw/pci/pcie_doe.h
> @@ -11,7 +11,6 @@
>  #define PCIE_DOE_H
>  
>  #include "qemu/range.h"
> -#include "qemu/typedefs.h"
>  #include "hw/register.h"
>  
>  /*
> diff --git a/include/qemu/async-teardown.h b/include/qemu/async-teardown.h
> index 092e7a37e7..b281da005b 100644
> --- a/include/qemu/async-teardown.h
> +++ b/include/qemu/async-teardown.h
> @@ -13,8 +13,6 @@
>  #ifndef QEMU_ASYNC_TEARDOWN_H
>  #define QEMU_ASYNC_TEARDOWN_H
>  
> -#include "config-host.h"
> -
>  #ifdef CONFIG_LINUX
>  void init_async_teardown(void);
>  #endif
> diff --git a/include/qemu/dbus.h b/include/qemu/dbus.h
> index 08f00dfd53..81d3de8a5a 100644
> --- a/include/qemu/dbus.h
> +++ b/include/qemu/dbus.h
> @@ -15,7 +15,6 @@
>  #include "qom/object.h"
>  #include "chardev/char.h"
>  #include "qemu/notify.h"
> -#include "qemu/typedefs.h"
>  
>  /* glib/gio 2.68 */
>  #define DBUS_METHOD_INVOCATION_HANDLED TRUE
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index 88d476161c..3ce62bf4a5 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -30,7 +30,6 @@
>  #ifndef HOST_UTILS_H
>  #define HOST_UTILS_H
>  
> -#include "qemu/compiler.h"
>  #include "qemu/bswap.h"
>  #include "qemu/int128.h"
>  
> diff --git a/include/sysemu/event-loop-base.h 
> b/include/sysemu/event-loop-base.h
> index 2748bf6ae1..a6c24f1351 100644
> --- a/include/sysemu/event-loop-base.h
> +++ b/include/sysemu/event-loop-base.h
> @@ -14,7 +14,6 @@
>  
>  #include "qom/object.h"
>  #include "block/aio.h"
> -#include "qemu/typedefs.h"
>  
>  #define TYPE_EVENT_LOOP_BASE "event-loop-base"
>  OBJECT_DECLARE_TYPE(EventLoopBase, EventLoopBaseClass,
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 8927092537..dd8f54a415 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -28,7 +28,6 @@
>  #include "exec/exec-all.h"
>  #include "tcg/tcg.h"
>  #include "qemu/atomic.h"
> -#include "qemu/compiler.h"
>  #include "qemu/timer.h"
>  #include "qemu/rcu.h"
>  #include "exec/log.h"
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 072cf67956..9621ec1341 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -19,8 +19,6 @@
>  #include "qemu/osdep.h"
>  #ifdef CONFIG_LINUX
>  #include 
> -#else
> -#include 
>  #endif
>  #include 
>  #include "hw/virtio/virtio.h"
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> index 8bdf4bac6e..847fa4c0cc 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ 

Re: [PATCH v4 18/19] 9p: Drop superfluous include of linux/limits.h

[Christian Schoenebeck]

On Thursday, January 19, 2023 11:37:00 AM CET Markus Armbruster wrote:
> Christian Schoenebeck  writes:
> 
> > On Thursday, January 19, 2023 7:59:58 AM CET Markus Armbruster wrote:
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  hw/9pfs/9p.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >> 
> >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> >> index 9621ec1341..aa736af380 100644
> >> --- a/hw/9pfs/9p.c
> >> +++ b/hw/9pfs/9p.c
> >> @@ -17,9 +17,6 @@
> >>   */
> >>  
> >>  #include "qemu/osdep.h"
> >> -#ifdef CONFIG_LINUX
> >> -#include 
> >> -#endif
> >>  #include 
> >>  #include "hw/virtio/virtio.h"
> >>  #include "qapi/error.h"
> >> 
> >
> > Where did that base version come from? I don't see it anywhere in history. 
> > Last relevant change in context was a136d17590a.
> 
> Current master (7ec8aeb6048) has
> 
> #include "qemu/osdep.h"
> #ifdef CONFIG_LINUX
> #include 
> #else
> #include 
> #endif
> #include 
> 
> The previous commit changes it to
> 
> #include "qemu/osdep.h"
> #ifdef CONFIG_LINUX
> #include 
> #endif
> #include 
> 
> because "qemu/osdep.h" already includes .
> 
> Clearer now?

Ah, right I missed that in your previous patch. Thanks!

Reviewed-by: Christian Schoenebeck 

Best regards,
Christian Schoenebeck

Re: [PATCH v4 16/19] Fix non-first inclusions of qemu/osdep.h

[Philippe Mathieu-Daudé]

On 19/1/23 12:41, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


On 19/1/23 07:59, Markus Armbruster wrote:

This commit was created with scripts/clean-includes.
Signed-off-by: Markus Armbruster 


[...]


Up to here:

Reviewed-by: Philippe Mathieu-Daudé 


diff --git a/util/async-teardown.c b/util/async-teardown.c
index 62bfce1b3c..62cdeb0f20 100644
--- a/util/async-teardown.c
+++ b/util/async-teardown.c
@@ -10,16 +10,12 @@
* option) any later version.  See the COPYING file in the top-level 
directory.
*
*/
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
  
  #include "qemu/osdep.h"

+#include 
+#include 
+#include 
+
   #include "qemu/async-teardown.h"


This file has more changes.


I'm not sure I understand.

The patch does two related things:

1. It puts qemu/osdep.h first.  The diff makes it look like we leave it
in place and move other stuff across, but that's the same.

2. It deletes inclusions of headers qemu/osdep.h already includes:

 
 
 
 
 


Ah, the other files get this done in the "Drop duplicate #include" patch.

Re: [PATCH v4 00/19] Clean up includes

[Michael S. Tsirkin]

On Thu, Jan 19, 2023 at 07:59:40AM +0100, Markus Armbruster wrote:
> Back in 2016, we discussed[1] rules for headers, and these were
> generally liked:
> 
> 1. Have a carefully curated header that's included everywhere first.  We
>got that already thanks to Peter: osdep.h.
> 
> 2. Headers should normally include everything they need beyond osdep.h.
>If exceptions are needed for some reason, they must be documented in
>the header.  If all that's needed from a header is typedefs, put
>those into qemu/typedefs.h instead of including the header.
> 
> 3. Cyclic inclusion is forbidden.
> 
> This series fixes violations of rule 2.  I may have split patches too
> aggressively.  Let me know if you want some squashed together.

series
Reviewed-by: Michael S. Tsirkin 

feel free to merge.

> v4:
> * PATCH 01-03: New
> * PATCH 04-15: Previous version redone with scripts/clean-includes,
>  result split up for review
> * PATCH 16-19: New
> 
> v3:
> * Rebased, old PATCH 1+2+4 are in master as commit
>   881e019770..f07ceffdf5
> * PATCH 1: Fix bsd-user
> 
> v2:
> * Rebased
> * PATCH 3: v1 posted separately
> * PATCH 4: New
> 
> [1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html
> 
> Markus Armbruster (19):
>   scripts/clean-includes: Fully skip / ignore files
>   scripts/clean-includes: Don't claim duplicate headers found when not
>   scripts/clean-includes: Skip symbolic links
>   bsd-user: Clean up includes
>   crypto: Clean up includes
>   hw/cxl: Clean up includes
>   hw/input: Clean up includes
>   hw/tricore: Clean up includes
>   qga: Clean up includes
>   migration: Clean up includes
>   net: Clean up includes
>   target/hexagon: Clean up includes
>   riscv: Clean up includes
>   block: Clean up includes
>   accel: Clean up includes
>   Fix non-first inclusions of qemu/osdep.h
>   Don't include headers already included by qemu/osdep.h
>   9p: Drop superfluous include of linux/limits.h
>   Drop duplicate #include
> 
>  backends/tpm/tpm_ioctl.h  |  2 --
>  bsd-user/bsd-proc.h   |  4 
>  bsd-user/qemu.h   |  1 -
>  crypto/block-luks-priv.h  |  1 -
>  fsdev/p9array.h   |  2 --
>  include/block/graph-lock.h|  1 -
>  include/block/write-threshold.h   |  2 --
>  include/hw/arm/fsl-imx6ul.h   |  1 -
>  include/hw/arm/fsl-imx7.h |  1 -
>  include/hw/cxl/cxl_component.h|  2 --
>  include/hw/cxl/cxl_host.h |  1 -
>  include/hw/cxl/cxl_pci.h  |  1 -
>  include/hw/input/pl050.h  |  1 -
>  include/hw/misc/aspeed_lpc.h  |  2 --
>  include/hw/pci/pcie_doe.h |  1 -
>  include/hw/tricore/triboard.h |  1 -
>  include/qemu/async-teardown.h |  2 --
>  include/qemu/dbus.h   |  1 -
>  include/qemu/host-utils.h |  1 -
>  include/qemu/userfaultfd.h|  1 -
>  include/sysemu/accel-blocker.h|  1 -
>  include/sysemu/event-loop-base.h  |  1 -
>  net/vmnet_int.h   |  1 -
>  qga/cutils.h  |  2 --
>  target/hexagon/hex_arch_types.h   |  1 -
>  target/hexagon/mmvec/macros.h |  1 -
>  target/riscv/pmu.h|  1 -
>  accel/tcg/cpu-exec.c  |  1 -
>  audio/sndioaudio.c|  2 +-
>  backends/hostmem-epc.c|  2 +-
>  backends/tpm/tpm_emulator.c   |  1 -
>  block/export/vduse-blk.c  |  2 +-
>  block/qapi.c  |  1 -
>  bsd-user/arm/signal.c |  1 +
>  bsd-user/arm/target_arch_cpu.c|  2 ++
>  bsd-user/freebsd/os-sys.c |  1 +
>  bsd-user/i386/signal.c|  1 +
>  bsd-user/i386/target_arch_cpu.c   |  3 +--
>  bsd-user/main.c   |  4 +---
>  bsd-user/strace.c |  1 -
>  bsd-user/x86_64/signal.c  |  1 +
>  bsd-user/x86_64/target_arch_cpu.c |  3 +--
>  hw/9pfs/9p.c  |  5 -
>  hw/acpi/piix4.c   |  1 -
>  hw/alpha/dp264.c  |  1 -
>  hw/arm/virt.c |  1 -
>  hw/arm/xlnx-versal.c  |  1 -
>  hw/block/pflash_cfi01.c   |  1 -
>  hw/core/machine.c |  1 -
>  hw/display/virtio-gpu-udmabuf.c   |  1 -
>  hw/hppa/machine.c |  1 -
>  hw/hyperv/syndbg.c|  2 +-
>  hw/i2c/pmbus_device.c |  1 -
>  hw/i386/acpi-build.c  |  1 -
>  hw/input/tsc210x.c|  1 -
>  hw/loongarch/acpi-build.c |  1 -
>  hw/misc/macio/cuda.c  |  1 -
>  hw/misc/macio/pmu.c   |  1 -
>  hw/net/xilinx_axienet.c   |  1 -
>  hw/ppc/ppc405_uc.c|  2 --
>  hw/ppc/ppc440_bamboo.c|  1 -
>  hw/ppc/spapr_drc.c|  1 -
>  hw/rdma/vmw/pvrdma_dev_ring.c |  1 -
>  hw/remote/machine.c   |  1 -
>  hw/remote/proxy-memory-listener.c |  1 -
>  hw/remote/remote-obj.c|  1 -
>  

Re: [PATCH v4 10/19] migration: Clean up includes

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> Clean up includes so that osdep.h is included first and headers
> >> which it implies are not included manually.
> >
> > That change doesn't seem to match the message; the patch is removing the
> > osdep.h include.
> 
> It's the commit message scripts/clean-includes creates :)
> 
> I can throw in another patch to the script so it mentions it also
> removes qemu/osdep.h from headers.

Oh hmm it would be clearer; but OK then, so 

Reviewed-by: Dr. David Alan Gilbert 

> >> This commit was created with scripts/clean-includes.
> >> 
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  include/qemu/userfaultfd.h | 1 -
> >>  1 file changed, 1 deletion(-)
> >> 
> >> diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
> >> index 6b74f92792..55c95998e8 100644
> >> --- a/include/qemu/userfaultfd.h
> >> +++ b/include/qemu/userfaultfd.h
> >> @@ -13,7 +13,6 @@
> >>  #ifndef USERFAULTFD_H
> >>  #define USERFAULTFD_H
> >>  
> >> -#include "qemu/osdep.h"
> >>  #include "exec/hwaddr.h"
> >>  #include 
> >>  
> >> -- 
> >> 2.39.0
> >> 
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v4 16/19] Fix non-first inclusions of qemu/osdep.h

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 19/1/23 07:59, Markus Armbruster wrote:
>> This commit was created with scripts/clean-includes.
>> Signed-off-by: Markus Armbruster 

[...]

> Up to here:
>
> Reviewed-by: Philippe Mathieu-Daudé 
>
>> diff --git a/util/async-teardown.c b/util/async-teardown.c
>> index 62bfce1b3c..62cdeb0f20 100644
>> --- a/util/async-teardown.c
>> +++ b/util/async-teardown.c
>> @@ -10,16 +10,12 @@
>>* option) any later version.  See the COPYING file in the top-level 
>> directory.
>>*
>>*/
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>>  
>>  #include "qemu/osdep.h"
>> +#include 
>> +#include 
>> +#include 
>> +
>>   #include "qemu/async-teardown.h"
>
> This file has more changes.

I'm not sure I understand.

The patch does two related things:

1. It puts qemu/osdep.h first.  The diff makes it look like we leave it
   in place and move other stuff across, but that's the same.

2. It deletes inclusions of headers qemu/osdep.h already includes:







What would you like me to change or explain?

Re: [PATCH v4 16/19] Fix non-first inclusions of qemu/osdep.h

[Philippe Mathieu-Daudé]

On 19/1/23 07:59, Markus Armbruster wrote:

This commit was created with scripts/clean-includes.

Signed-off-by: Markus Armbruster 
---
  audio/sndioaudio.c   |  2 +-
  backends/hostmem-epc.c   |  2 +-
  block/export/vduse-blk.c |  2 +-
  hw/hyperv/syndbg.c   |  2 +-
  util/async-teardown.c| 12 
  5 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
index 632b0e3825..3fde01fdbd 100644
--- a/audio/sndioaudio.c
+++ b/audio/sndioaudio.c
@@ -14,9 +14,9 @@
   * to recording, which is what guest systems expect.
   */
  
+#include "qemu/osdep.h"

  #include 
  #include 
-#include "qemu/osdep.h"
  #include "qemu/main-loop.h"
  #include "audio.h"
  #include "trace.h"
diff --git a/backends/hostmem-epc.c b/backends/hostmem-epc.c
index 037292d267..4e162d6789 100644
--- a/backends/hostmem-epc.c
+++ b/backends/hostmem-epc.c
@@ -9,9 +9,9 @@
   * This work is licensed under the terms of the GNU GPL, version 2 or later.
   * See the COPYING file in the top-level directory.
   */
-#include 
  
  #include "qemu/osdep.h"

+#include 
  #include "qom/object_interfaces.h"
  #include "qapi/error.h"
  #include "sysemu/hostmem.h"
diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index 350d6fdaf0..f7ae44e3ce 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -10,9 +10,9 @@
   * later.  See the COPYING file in the top-level directory.
   */
  
+#include "qemu/osdep.h"

  #include 
  
-#include "qemu/osdep.h"

  #include "qapi/error.h"
  #include "block/export.h"
  #include "qemu/error-report.h"
diff --git a/hw/hyperv/syndbg.c b/hw/hyperv/syndbg.c
index 16d04cfdc6..94fe1b534b 100644
--- a/hw/hyperv/syndbg.c
+++ b/hw/hyperv/syndbg.c
@@ -5,8 +5,8 @@
   * See the COPYING file in the top-level directory.
   */
  
-#include "qemu/ctype.h"

  #include "qemu/osdep.h"
+#include "qemu/ctype.h"
  #include "qemu/error-report.h"
  #include "qemu/main-loop.h"
  #include "qemu/sockets.h"


Up to here:

Reviewed-by: Philippe Mathieu-Daudé 


diff --git a/util/async-teardown.c b/util/async-teardown.c
index 62bfce1b3c..62cdeb0f20 100644
--- a/util/async-teardown.c
+++ b/util/async-teardown.c
@@ -10,16 +10,12 @@
   * option) any later version.  See the COPYING file in the top-level 
directory.
   *
   */
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
  
  #include "qemu/osdep.h"

+#include 
+#include 
+#include 
+
  #include "qemu/async-teardown.h"


This file has more changes.

Re: [PATCH v4 13/19] riscv: Clean up includes

[Philippe Mathieu-Daudé]

On 19/1/23 07:59, Markus Armbruster wrote:

Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes.

Signed-off-by: Markus Armbruster 
---
  target/riscv/pmu.h | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v4 15/19] accel: Clean up includes

[Philippe Mathieu-Daudé]

On 19/1/23 07:59, Markus Armbruster wrote:

Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes.

Signed-off-by: Markus Armbruster 
---
  include/sysemu/accel-blocker.h | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v4 18/19] 9p: Drop superfluous include of linux/limits.h

[Markus Armbruster]

Christian Schoenebeck  writes:

> On Thursday, January 19, 2023 7:59:58 AM CET Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/9pfs/9p.c | 3 ---
>>  1 file changed, 3 deletions(-)
>> 
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 9621ec1341..aa736af380 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -17,9 +17,6 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> -#ifdef CONFIG_LINUX
>> -#include 
>> -#endif
>>  #include 
>>  #include "hw/virtio/virtio.h"
>>  #include "qapi/error.h"
>> 
>
> Where did that base version come from? I don't see it anywhere in history. 
> Last relevant change in context was a136d17590a.

Current master (7ec8aeb6048) has

#include "qemu/osdep.h"
#ifdef CONFIG_LINUX
#include 
#else
#include 
#endif
#include 

The previous commit changes it to

#include "qemu/osdep.h"
#ifdef CONFIG_LINUX
#include 
#endif
#include 

because "qemu/osdep.h" already includes .

Clearer now?

Re: [PATCH v4 11/19] net: Clean up includes

[Philippe Mathieu-Daudé]

On 19/1/23 07:59, Markus Armbruster wrote:

Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes.

Signed-off-by: Markus Armbruster 
---
  net/vmnet_int.h | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: completion timeouts with pin-based interrupts in QEMU hw/nvme

[Peter Maydell]

On Thu, 19 Jan 2023 at 04:03, Keith Busch  wrote:
>
> On Thu, Jan 19, 2023 at 01:10:57PM +1000, Alistair Francis wrote:
> > On Thu, Jan 19, 2023 at 12:44 PM Keith Busch  wrote:
> > >
> > > Further up, it says the "interrupt gateway" is responsible for
> > > forwarding new interrupt requests while the level remains asserted, but
> > > it doesn't look like anything is handling that, which essentially turns
> > > this into an edge interrupt. Am I missing something, or is this really
> > > not being handled?
> >
> > Yeah, that wouldn't be handled. In QEMU the PLIC relies on QEMUs
> > internal GPIO lines to trigger an interrupt. So with the current setup
> > we only support edge triggered interrupts.
>
> Thanks for confirming!
>
> Klaus,
> I think we can justify introducing a work-around in the emulated device
> now. My previous proposal with pci_irq_pulse() is no good since it does
> assert+deassert, but it needs to be the other way around, so please
> don't considert that one.

No, please don't. The bug is in the risc-v interrupt controller,
so fix the risc-v interrupt controller. It shouldn't be too difficult
(you probably have to do something like what the Arm GIC implementation
does, where when the guest dismisses the interrupt you look at the level
to see if it needs to be re-pended.)

Once "workarounds" go into QEMU device emulation that make it
deviate from hardware behaviour, it's hard to get rid of them
again, because nobody knows whether deployed guests now accidentally
rely on the wrong behaviour. So the correct thing is to never
put in the workaround in the first place.

thanks
-- PMM

Re: [PATCH v4 05/19] crypto: Clean up includes

[Philippe Mathieu-Daudé]

On 19/1/23 07:59, Markus Armbruster wrote:

Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes.

Signed-off-by: Markus Armbruster 
---
  crypto/block-luks-priv.h | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v4 08/19] hw/tricore: Clean up includes

[Philippe Mathieu-Daudé]

On 19/1/23 07:59, Markus Armbruster wrote:

Clean up includes so that osdep.h is included first and headers
which it implies are not included manually.

This commit was created with scripts/clean-includes.

Signed-off-by: Markus Armbruster 
---
  include/hw/tricore/triboard.h | 1 -
  1 file changed, 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v4 10/19] migration: Clean up includes

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> Clean up includes so that osdep.h is included first and headers
>> which it implies are not included manually.
>
> That change doesn't seem to match the message; the patch is removing the
> osdep.h include.

It's the commit message scripts/clean-includes creates :)

I can throw in another patch to the script so it mentions it also
removes qemu/osdep.h from headers.

>> This commit was created with scripts/clean-includes.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  include/qemu/userfaultfd.h | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
>> index 6b74f92792..55c95998e8 100644
>> --- a/include/qemu/userfaultfd.h
>> +++ b/include/qemu/userfaultfd.h
>> @@ -13,7 +13,6 @@
>>  #ifndef USERFAULTFD_H
>>  #define USERFAULTFD_H
>>  
>> -#include "qemu/osdep.h"
>>  #include "exec/hwaddr.h"
>>  #include 
>>  
>> -- 
>> 2.39.0
>> 

[PATCH] MAINTAINERS: Cover RCU documentation

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0fe50d01e3..73e9cb33f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2812,6 +2812,8 @@ F: qapi/run-state.json
 Read, Copy, Update (RCU)
 M: Paolo Bonzini 
 S: Maintained
+F: docs/devel/lockcnt.txt
+F: docs/devel/rcu.txt
 F: include/qemu/rcu*.h
 F: tests/unit/rcutorture.c
 F: tests/unit/test-rcu-*.c
-- 
2.38.1

Re: [PATCH v4 19/19] Drop duplicate #include

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Tracked down with the help of scripts/clean-includes.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/hw/arm/fsl-imx6ul.h   | 1 -
>  include/hw/arm/fsl-imx7.h | 1 -
>  backends/tpm/tpm_emulator.c   | 1 -
>  hw/acpi/piix4.c   | 1 -
>  hw/alpha/dp264.c  | 1 -
>  hw/arm/virt.c | 1 -
>  hw/arm/xlnx-versal.c  | 1 -
>  hw/block/pflash_cfi01.c   | 1 -
>  hw/core/machine.c | 1 -
>  hw/hppa/machine.c | 1 -
>  hw/i386/acpi-build.c  | 1 -
>  hw/loongarch/acpi-build.c | 1 -
>  hw/misc/macio/cuda.c  | 1 -
>  hw/misc/macio/pmu.c   | 1 -
>  hw/net/xilinx_axienet.c   | 1 -
>  hw/ppc/ppc405_uc.c| 2 --
>  hw/ppc/ppc440_bamboo.c| 1 -
>  hw/ppc/spapr_drc.c| 1 -
>  hw/rdma/vmw/pvrdma_dev_ring.c | 1 -
>  hw/remote/machine.c   | 1 -
>  hw/remote/remote-obj.c| 1 -
>  hw/rtc/mc146818rtc.c  | 1 -
>  hw/s390x/virtio-ccw-serial.c  | 1 -
>  migration/postcopy-ram.c  | 2 --
>  softmmu/dirtylimit.c  | 1 -
>  softmmu/runstate.c| 1 -
>  softmmu/vl.c  | 1 -
>  target/loongarch/translate.c  | 1 -
>  target/mips/tcg/translate.c   | 1 -
>  target/nios2/translate.c  | 2 --
>  tests/unit/test-cutils.c  | 1 -
>  ui/gtk.c  | 1 -
>  util/oslib-posix.c| 4 
>  33 files changed, 39 deletions(-)
> 
> diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
> index 7812e516a5..1952cb984d 100644
> --- a/include/hw/arm/fsl-imx6ul.h
> +++ b/include/hw/arm/fsl-imx6ul.h
> @@ -30,7 +30,6 @@
>  #include "hw/timer/imx_gpt.h"
>  #include "hw/timer/imx_epit.h"
>  #include "hw/i2c/imx_i2c.h"
> -#include "hw/gpio/imx_gpio.h"
>  #include "hw/sd/sdhci.h"
>  #include "hw/ssi/imx_spi.h"
>  #include "hw/net/imx_fec.h"
> diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
> index 4e5e071864..355bd8ea83 100644
> --- a/include/hw/arm/fsl-imx7.h
> +++ b/include/hw/arm/fsl-imx7.h
> @@ -32,7 +32,6 @@
>  #include "hw/timer/imx_gpt.h"
>  #include "hw/timer/imx_epit.h"
>  #include "hw/i2c/imx_i2c.h"
> -#include "hw/gpio/imx_gpio.h"
>  #include "hw/sd/sdhci.h"
>  #include "hw/ssi/imx_spi.h"
>  #include "hw/net/imx_fec.h"
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 49cc3d749d..2b440d2c9a 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -35,7 +35,6 @@
>  #include "sysemu/runstate.h"
>  #include "sysemu/tpm_backend.h"
>  #include "sysemu/tpm_util.h"
> -#include "sysemu/runstate.h"
>  #include "tpm_int.h"
>  #include "tpm_ioctl.h"
>  #include "migration/blocker.h"
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 0a81f1ad93..df39f91294 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -35,7 +35,6 @@
>  #include "sysemu/xen.h"
>  #include "qapi/error.h"
>  #include "qemu/range.h"
> -#include "hw/acpi/pcihp.h"
>  #include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/cpu.h"
>  #include "hw/hotplug.h"
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index c502c8c62a..4161f559a7 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -18,7 +18,6 @@
>  #include "net/net.h"
>  #include "qemu/cutils.h"
>  #include "qemu/datadir.h"
> -#include "net/net.h"
>  
>  static uint64_t cpu_alpha_superpage_to_phys(void *opaque, uint64_t addr)
>  {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ea2413a0ba..d3849d7233 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -33,7 +33,6 @@
>  #include "qemu/units.h"
>  #include "qemu/option.h"
>  #include "monitor/qdev.h"
> -#include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/boot.h"
>  #include "hw/arm/primecell.h"
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index 57276e1506..69b1b99e93 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -22,7 +22,6 @@
>  #include "hw/misc/unimp.h"
>  #include "hw/arm/xlnx-versal.h"
>  #include "qemu/log.h"
> -#include "hw/sysbus.h"
>  
>  #define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
>  #define XLNX_VERSAL_RCPU_TYPE ARM_CPU_TYPE_NAME("cortex-r5f")
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 0cbc2fb4cb..d11406eada 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -45,7 +45,6 @@
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qemu/bitops.h"
> -#include "qemu/error-report.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 616f3a207c..67cf9f9dcd 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -39,7 +39,6 @@
>  #include "exec/confidential-guest-support.h"
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-pci.h"
> -#include "qom/object_interfaces.h"
>  
>  GlobalProperty hw_compat_7_2[] = {};
>  const size_t 

Re: [PATCH v4 18/19] 9p: Drop superfluous include of linux/limits.h

[Christian Schoenebeck]

On Thursday, January 19, 2023 7:59:58 AM CET Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  hw/9pfs/9p.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 9621ec1341..aa736af380 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -17,9 +17,6 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#ifdef CONFIG_LINUX
> -#include 
> -#endif
>  #include 
>  #include "hw/virtio/virtio.h"
>  #include "qapi/error.h"
> 

Where did that base version come from? I don't see it anywhere in history. 
Last relevant change in context was a136d17590a.

Best regards,
Christian Schoenebeck

Re: [PATCH v4 10/19] migration: Clean up includes

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.

That change doesn't seem to match the message; the patch is removing the
osdep.h include.

Dave

> This commit was created with scripts/clean-includes.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qemu/userfaultfd.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
> index 6b74f92792..55c95998e8 100644
> --- a/include/qemu/userfaultfd.h
> +++ b/include/qemu/userfaultfd.h
> @@ -13,7 +13,6 @@
>  #ifndef USERFAULTFD_H
>  #define USERFAULTFD_H
>  
> -#include "qemu/osdep.h"
>  #include "exec/hwaddr.h"
>  #include 
>  
> -- 
> 2.39.0
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v4 00/19] Clean up includes

[Markus Armbruster]

Markus Armbruster  writes:

> Back in 2016, we discussed[1] rules for headers, and these were
> generally liked:
>
> 1. Have a carefully curated header that's included everywhere first.  We
>got that already thanks to Peter: osdep.h.
>
> 2. Headers should normally include everything they need beyond osdep.h.
>If exceptions are needed for some reason, they must be documented in
>the header.  If all that's needed from a header is typedefs, put
>those into qemu/typedefs.h instead of including the header.
>
> 3. Cyclic inclusion is forbidden.
>
> This series fixes violations of rule 2.  I may have split patches too
> aggressively.  Let me know if you want some squashed together.
>
> v4:
> * PATCH 01-03: New
> * PATCH 04-15: Previous version redone with scripts/clean-includes,
>  result split up for review

Copying the R-bys for v3 to these patches is tempting.  But I didn't.

> * PATCH 16-19: New
>
> v3:
> * Rebased, old PATCH 1+2+4 are in master as commit
>   881e019770..f07ceffdf5
> * PATCH 1: Fix bsd-user
>
> v2:
> * Rebased
> * PATCH 3: v1 posted separately
> * PATCH 4: New
>
> [1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html

Re: [PATCH v4 09/19] qga: Clean up includes

[Konstantin Kostiuk]

Reviewed-by: Konstantin Kostiuk 



On Thu, Jan 19, 2023 at 9:00 AM Markus Armbruster  wrote:

> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
>
> This commit was created with scripts/clean-includes.
>
> Signed-off-by: Markus Armbruster 
> ---
>  qga/cutils.h | 2 --
>  qga/commands-posix.c | 1 -
>  qga/cutils.c | 3 ++-
>  3 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/qga/cutils.h b/qga/cutils.h
> index f0f30a7d28..c1f2f4b17a 100644
> --- a/qga/cutils.h
> +++ b/qga/cutils.h
> @@ -1,8 +1,6 @@
>  #ifndef CUTILS_H_
>  #define CUTILS_H_
>
> -#include "qemu/osdep.h"
> -
>  int qga_open_cloexec(const char *name, int flags, mode_t mode);
>
>  #endif /* CUTILS_H_ */
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index ebd33a643c..079689d79a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -51,7 +51,6 @@
>  #else
>  #include 
>  #endif
> -#include 
>  #ifdef CONFIG_SOLARIS
>  #include 
>  #endif
> diff --git a/qga/cutils.c b/qga/cutils.c
> index b8e142ef64..b21bcf3683 100644
> --- a/qga/cutils.c
> +++ b/qga/cutils.c
> @@ -2,8 +2,9 @@
>   * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
>   * See the COPYING file in the top-level directory.
>   */
> -#include "cutils.h"
>
> +#include "qemu/osdep.h"
> +#include "cutils.h"
>  #include "qapi/error.h"
>
>  /**
> --
> 2.39.0
>
>

Re: completion timeouts with pin-based interrupts in QEMU hw/nvme

[Klaus Jensen]

On Jan 19 08:28, Klaus Jensen wrote:
> On Jan 18 21:03, Keith Busch wrote:
> > On Thu, Jan 19, 2023 at 01:10:57PM +1000, Alistair Francis wrote:
> > > On Thu, Jan 19, 2023 at 12:44 PM Keith Busch  wrote:
> > > >
> > > > Further up, it says the "interrupt gateway" is responsible for
> > > > forwarding new interrupt requests while the level remains asserted, but
> > > > it doesn't look like anything is handling that, which essentially turns
> > > > this into an edge interrupt. Am I missing something, or is this really
> > > > not being handled?
> > > 
> > > Yeah, that wouldn't be handled. In QEMU the PLIC relies on QEMUs
> > > internal GPIO lines to trigger an interrupt. So with the current setup
> > > we only support edge triggered interrupts.
> > 
> > Thanks for confirming!
> > 
> > Klaus,
> > I think we can justify introducing a work-around in the emulated device
> > now. My previous proposal with pci_irq_pulse() is no good since it does
> > assert+deassert, but it needs to be the other way around, so please
> > don't considert that one.
> > 
> > Also, we ought to revisit the intms/intmc usage in the linux driver for
> > threaded interrupts.
> 
> +CC: qemu-riscv
> 
> Keith,
> 
> Thanks for digging into this!
> 
> Yeah, you are probably right that we should only use the intms/intmc
> changes in the use_threaded_interrupts case, not in general. While my
> RFC patch does seem to "fix" this, it is just a workaround as your
> analysis indicate.

+CC: Philippe,

I am observing these timeouts/aborts on mips as well, so I guess that
emulation could suffer from the same issue?


signature.asc
Description: PGP signature