Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread BALATON Zoltan 

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.54, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:
For distros like downstream RHEL, it would be helpful to allow to 
disable

the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but 
that header is in inlcude because some files outside hw/ide include it. 
I've found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
IDE parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow 
make this a local header where non-public parts of hw/ide/internal.h 
could be moved in the future. Such as rename include/hw/ide/internal.h to 
ide.h and name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to 
unentangle the stuff one day. So what's wrong with having a dedicated 
header for the stuff in hw/ide/qdev.c ?


Maybe that it's not obvious from the name that it belongs to qdev.c as the 
names are not the same.


I didn't want to just name the header "qdev.h" since that could easily be 
confused in #include statements...
IMHO qdev.c is already a very bad idea for a file name here... maybe 
something like ide-dev.c and ide-dev.h would be better?


The comment at the beginning of qdev.c says "ide bus support" but a lot of 
functions in it have ide_dev prefix. Maybe this comes from the original 
qdev-ification of this code and then adding some more unrelated parts in 
this file. It's clearly beyond scope of this patch to clean all this but 
since you're changing it maybe this is a good opportunity to reduce the 
mess a bit. I'm OK with renaming qdev.c to ide-dev.c or qdev-ide.c or 
whatever else as long as it's clear that the header belongs to the c file.


Also some of the qdev stuff that should be in this header are in 
include/hw/ide/internal.h so these will still be split arbitrarily.


Oh, well, it seems to be a mess already... hw/ide/pci.h includes the 
internal.h header and thus opens the internal definitions to all PCI-based 
IDE devices :-/


I've missed that so besides files directly including internal.h there 
could be others using some stuff from it through pci.h so this makes it 
more difficult to untangle.


If we can agree on a better name for qdev-ide.h, I can try to clean that mess 
up a little bit...


My comment was only that if you make changes then do it in a way that 
leaves the possibility to move stuff here to clean the current situation 
but if you can also do some of it now that's even better but not required.


Regards,
BALATON Zoltan

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread Thomas Huth 

On 01/02/2024 13.54, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but 
that header is in inlcude because some files outside hw/ide include it. 
I've found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
IDE parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow 
make this a local header where non-public parts of hw/ide/internal.h 
could be moved in the future. Such as rename include/hw/ide/internal.h to 
ide.h and name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to 
unentangle the stuff one day. So what's wrong with having a dedicated 
header for the stuff in hw/ide/qdev.c ?


Maybe that it's not obvious from the name that it belongs to qdev.c as the 
names are not the same.


I didn't want to just name the header "qdev.h" since that could easily be 
confused in #include statements...
IMHO qdev.c is already a very bad idea for a file name here... maybe 
something like ide-dev.c and ide-dev.h would be better?


Also some of the qdev stuff that should be in this 
header are in include/hw/ide/internal.h so these will still be split 
arbitrarily.


Oh, well, it seems to be a mess already... hw/ide/pci.h includes the 
internal.h header and thus opens the internal definitions to all PCI-based 
IDE devices :-/


If we can agree on a better name for qdev-ide.h, I can try to clean that 
mess up a little bit...


 Thomas

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Hanna Czenczek 

On 01.02.24 16:25, Hanna Czenczek wrote:

[...]


It just seems simpler to me to not rely on the BB's context at all.


Hm, I now see the problem is that the processing (and scheduling) is 
largely done in generic SCSI code, which doesn’t have access to 
virtio-scsi’s context, only to that of the BB.  That makes my idea quite 
impossible. :/

Re: [PATCH v3 0/6] hw/{arm,xen} convert printfs to trace/reports

2024-02-01 Thread Peter Maydell 
On Mon, 29 Jan 2024 at 16:10, Manos Pitsidianakis
 wrote:
>
> This series changes some printfs to use the trace event framework.
> Additionally, it converts some error/warning reporting fprintfs to
> error_report/warn_report.
>
> v2 -> v3
> :
> - addressed Peter Maydells's review
>
> v1 -> v2
> :
> - addressed Alex's review
>
>
> Manos Pitsidianakis (6):
>   hw/arm/strongarm.c: convert DPRINTF to trace events and guest errors
>   hw/arm/z2: convert DPRINTF to trace events and guest errors
>   hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn
> reports
>   hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints
>   hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints
>   hw/xen: convert stderr prints to error/warn reports


Applied to target-arm.next, thanks.

-- PMM

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Hanna Czenczek 

On 01.02.24 15:28, Stefan Hajnoczi wrote:

On Thu, Feb 01, 2024 at 03:10:12PM +0100, Hanna Czenczek wrote:

On 31.01.24 21:35, Stefan Hajnoczi wrote:

On Fri, Jan 26, 2024 at 04:24:49PM +0100, Hanna Czenczek wrote:

On 26.01.24 14:18, Kevin Wolf wrote:

Am 25.01.2024 um 18:32 hat Hanna Czenczek geschrieben:

On 23.01.24 18:10, Kevin Wolf wrote:

Am 23.01.2024 um 17:40 hat Hanna Czenczek geschrieben:

On 21.12.23 22:23, Kevin Wolf wrote:

From: Stefan Hajnoczi

Stop depending on the AioContext lock and instead access
SCSIDevice->requests from only one thread at a time:
- When the VM is running only the BlockBackend's AioContext may access
   the requests list.
- When the VM is stopped only the main loop may access the requests
   list.

These constraints protect the requests list without the need for locking
in the I/O code path.

Note that multiple IOThreads are not supported yet because the code
assumes all SCSIRequests are executed from a single AioContext. Leave
that as future work.

Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Message-ID:<20231204164259.1515217-2-stefa...@redhat.com>
Signed-off-by: Kevin Wolf
---
  include/hw/scsi/scsi.h |   7 +-
  hw/scsi/scsi-bus.c | 181 -
  2 files changed, 131 insertions(+), 57 deletions(-)

My reproducer forhttps://issues.redhat.com/browse/RHEL-3934  now breaks more
often because of this commit than because of the original bug, i.e. when
repeatedly hot-plugging and unplugging a virtio-scsi and a scsi-hd device,
this tends to happen when unplugging the scsi-hd:

Note: We (on issues.redhat.com) have a separate report that seems to be
concerning this very problem: https://issues.redhat.com/browse/RHEL-19381


{"execute":"device_del","arguments":{"id":"stg0"}}
{"return": {}}
qemu-system-x86_64: ../block/block-backend.c:2429: blk_get_aio_context:
Assertion `ctx == blk->ctx' failed.

[...]


I don’t know anything about the problem yet, but as usual, I like
speculation and discovering how wrong I was later on, so one thing I came
across that’s funny about virtio-scsi is that requests can happen even while
a disk is being attached or detached.  That is, Linux seems to probe all
LUNs when a new virtio-scsi device is being attached, and it won’t stop just
because a disk is being attached or removed.  So maybe that’s part of the
problem, that we get a request while the BB is being detached, and
temporarily in an inconsistent state (BDS context differs from BB context).

I don't know anything about the problem either, but since you already
speculated about the cause, let me speculate about the solution:
Can we hold the graph writer lock for the tran_commit() call in
bdrv_try_change_aio_context()? And of course take the reader lock for
blk_get_aio_context(), but that should be completely unproblematic.

Actually, now that completely unproblematic part is giving me trouble.  I
wanted to just put a graph lock into blk_get_aio_context() (making it a
coroutine with a wrapper)

Which is the first thing I neglected and already not great. We have
calls of blk_get_aio_context() in the SCSI I/O path, and creating a
coroutine and doing at least two context switches simply for this call
is a lot of overhead...


but callers of blk_get_aio_context() generally assume the context is
going to stay the BB’s context for as long as their AioContext *
variable is in scope.

I'm not so sure about that. And taking another step back, I'm actually
also not sure how much it still matters now that they can submit I/O
from any thread.

That’s my impression, too, but “not sure” doesn’t feel great. :)
scsi_device_for_each_req_async_bh() specifically double-checks whether it’s
still in the right context before invoking the specified function, so it
seems there was some intention to continue to run in the context associated
with the BB.

(Not judging whether that intent makes sense or not, yet.)


Maybe the correct solution is to remove the assertion from
blk_get_aio_context() and just always return blk->ctx. If it's in the
middle of a change, you'll either get the old one or the new one. Either
one is fine to submit I/O from, and if you care about changes for other
reasons (like SCSI does), then you need explicit code to protect it
anyway (which SCSI apparently has, but it doesn't work).

I think most callers do just assume the BB stays in the context they got
(without any proof, admittedly), but I agree that under re-evaluation, it
probably doesn’t actually matter to them, really. And yes, basically, if the
caller doesn’t need to take a lock because it doesn’t really matter whether
blk->ctx changes while its still using the old value, blk_get_aio_context()
in turn doesn’t need to double-check blk->ctx against the root node’s
context either, and nobody needs a lock.

So I agree, it’s on the caller to protect against a potentially changing
context, blk_get_aio_context() should just return blk->ctx and not check
against the root

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Stefan Hajnoczi 
On Thu, Feb 01, 2024 at 03:10:12PM +0100, Hanna Czenczek wrote:
> On 31.01.24 21:35, Stefan Hajnoczi wrote:
> > On Fri, Jan 26, 2024 at 04:24:49PM +0100, Hanna Czenczek wrote:
> > > On 26.01.24 14:18, Kevin Wolf wrote:
> > > > Am 25.01.2024 um 18:32 hat Hanna Czenczek geschrieben:
> > > > > On 23.01.24 18:10, Kevin Wolf wrote:
> > > > > > Am 23.01.2024 um 17:40 hat Hanna Czenczek geschrieben:
> > > > > > > On 21.12.23 22:23, Kevin Wolf wrote:
> > > > > > > > From: Stefan Hajnoczi
> > > > > > > > 
> > > > > > > > Stop depending on the AioContext lock and instead access
> > > > > > > > SCSIDevice->requests from only one thread at a time:
> > > > > > > > - When the VM is running only the BlockBackend's AioContext may 
> > > > > > > > access
> > > > > > > >   the requests list.
> > > > > > > > - When the VM is stopped only the main loop may access the 
> > > > > > > > requests
> > > > > > > >   list.
> > > > > > > > 
> > > > > > > > These constraints protect the requests list without the need 
> > > > > > > > for locking
> > > > > > > > in the I/O code path.
> > > > > > > > 
> > > > > > > > Note that multiple IOThreads are not supported yet because the 
> > > > > > > > code
> > > > > > > > assumes all SCSIRequests are executed from a single AioContext. 
> > > > > > > > Leave
> > > > > > > > that as future work.
> > > > > > > > 
> > > > > > > > Signed-off-by: Stefan Hajnoczi
> > > > > > > > Reviewed-by: Eric Blake
> > > > > > > > Message-ID:<20231204164259.1515217-2-stefa...@redhat.com>
> > > > > > > > Signed-off-by: Kevin Wolf
> > > > > > > > ---
> > > > > > > >  include/hw/scsi/scsi.h |   7 +-
> > > > > > > >  hw/scsi/scsi-bus.c | 181 
> > > > > > > > -
> > > > > > > >  2 files changed, 131 insertions(+), 57 deletions(-)
> > > > > > > My reproducer forhttps://issues.redhat.com/browse/RHEL-3934  now 
> > > > > > > breaks more
> > > > > > > often because of this commit than because of the original bug, 
> > > > > > > i.e. when
> > > > > > > repeatedly hot-plugging and unplugging a virtio-scsi and a 
> > > > > > > scsi-hd device,
> > > > > > > this tends to happen when unplugging the scsi-hd:
> > > Note: We (on issues.redhat.com) have a separate report that seems to be
> > > concerning this very problem: https://issues.redhat.com/browse/RHEL-19381
> > > 
> > > > > > > {"execute":"device_del","arguments":{"id":"stg0"}}
> > > > > > > {"return": {}}
> > > > > > > qemu-system-x86_64: ../block/block-backend.c:2429: 
> > > > > > > blk_get_aio_context:
> > > > > > > Assertion `ctx == blk->ctx' failed.
> > > > > [...]
> > > > > 
> > > > > > > I don’t know anything about the problem yet, but as usual, I like
> > > > > > > speculation and discovering how wrong I was later on, so one 
> > > > > > > thing I came
> > > > > > > across that’s funny about virtio-scsi is that requests can happen 
> > > > > > > even while
> > > > > > > a disk is being attached or detached.  That is, Linux seems to 
> > > > > > > probe all
> > > > > > > LUNs when a new virtio-scsi device is being attached, and it 
> > > > > > > won’t stop just
> > > > > > > because a disk is being attached or removed.  So maybe that’s 
> > > > > > > part of the
> > > > > > > problem, that we get a request while the BB is being detached, and
> > > > > > > temporarily in an inconsistent state (BDS context differs from BB 
> > > > > > > context).
> > > > > > I don't know anything about the problem either, but since you 
> > > > > > already
> > > > > > speculated about the cause, let me speculate about the solution:
> > > > > > Can we hold the graph writer lock for the tran_commit() call in
> > > > > > bdrv_try_change_aio_context()? And of course take the reader lock 
> > > > > > for
> > > > > > blk_get_aio_context(), but that should be completely unproblematic.
> > > > > Actually, now that completely unproblematic part is giving me 
> > > > > trouble.  I
> > > > > wanted to just put a graph lock into blk_get_aio_context() (making it 
> > > > > a
> > > > > coroutine with a wrapper)
> > > > Which is the first thing I neglected and already not great. We have
> > > > calls of blk_get_aio_context() in the SCSI I/O path, and creating a
> > > > coroutine and doing at least two context switches simply for this call
> > > > is a lot of overhead...
> > > > 
> > > > > but callers of blk_get_aio_context() generally assume the context is
> > > > > going to stay the BB’s context for as long as their AioContext *
> > > > > variable is in scope.
> > > > I'm not so sure about that. And taking another step back, I'm actually
> > > > also not sure how much it still matters now that they can submit I/O
> > > > from any thread.
> > > That’s my impression, too, but “not sure” doesn’t feel great. :)
> > > scsi_device_for_each_req_async_bh() specifically double-checks whether 
> > > it’s
> > > still in the right context before invoking the specified function, so it
> > > seems there was some intention to continue to run in the

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Hanna Czenczek 

On 31.01.24 21:35, Stefan Hajnoczi wrote:

On Fri, Jan 26, 2024 at 04:24:49PM +0100, Hanna Czenczek wrote:

On 26.01.24 14:18, Kevin Wolf wrote:

Am 25.01.2024 um 18:32 hat Hanna Czenczek geschrieben:

On 23.01.24 18:10, Kevin Wolf wrote:

Am 23.01.2024 um 17:40 hat Hanna Czenczek geschrieben:

On 21.12.23 22:23, Kevin Wolf wrote:

From: Stefan Hajnoczi

Stop depending on the AioContext lock and instead access
SCSIDevice->requests from only one thread at a time:
- When the VM is running only the BlockBackend's AioContext may access
  the requests list.
- When the VM is stopped only the main loop may access the requests
  list.

These constraints protect the requests list without the need for locking
in the I/O code path.

Note that multiple IOThreads are not supported yet because the code
assumes all SCSIRequests are executed from a single AioContext. Leave
that as future work.

Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Message-ID:<20231204164259.1515217-2-stefa...@redhat.com>
Signed-off-by: Kevin Wolf
---
 include/hw/scsi/scsi.h |   7 +-
 hw/scsi/scsi-bus.c | 181 -
 2 files changed, 131 insertions(+), 57 deletions(-)

My reproducer forhttps://issues.redhat.com/browse/RHEL-3934  now breaks more
often because of this commit than because of the original bug, i.e. when
repeatedly hot-plugging and unplugging a virtio-scsi and a scsi-hd device,
this tends to happen when unplugging the scsi-hd:

Note: We (on issues.redhat.com) have a separate report that seems to be
concerning this very problem: https://issues.redhat.com/browse/RHEL-19381


{"execute":"device_del","arguments":{"id":"stg0"}}
{"return": {}}
qemu-system-x86_64: ../block/block-backend.c:2429: blk_get_aio_context:
Assertion `ctx == blk->ctx' failed.

[...]


I don’t know anything about the problem yet, but as usual, I like
speculation and discovering how wrong I was later on, so one thing I came
across that’s funny about virtio-scsi is that requests can happen even while
a disk is being attached or detached.  That is, Linux seems to probe all
LUNs when a new virtio-scsi device is being attached, and it won’t stop just
because a disk is being attached or removed.  So maybe that’s part of the
problem, that we get a request while the BB is being detached, and
temporarily in an inconsistent state (BDS context differs from BB context).

I don't know anything about the problem either, but since you already
speculated about the cause, let me speculate about the solution:
Can we hold the graph writer lock for the tran_commit() call in
bdrv_try_change_aio_context()? And of course take the reader lock for
blk_get_aio_context(), but that should be completely unproblematic.

Actually, now that completely unproblematic part is giving me trouble.  I
wanted to just put a graph lock into blk_get_aio_context() (making it a
coroutine with a wrapper)

Which is the first thing I neglected and already not great. We have
calls of blk_get_aio_context() in the SCSI I/O path, and creating a
coroutine and doing at least two context switches simply for this call
is a lot of overhead...


but callers of blk_get_aio_context() generally assume the context is
going to stay the BB’s context for as long as their AioContext *
variable is in scope.

I'm not so sure about that. And taking another step back, I'm actually
also not sure how much it still matters now that they can submit I/O
from any thread.

That’s my impression, too, but “not sure” doesn’t feel great. :)
scsi_device_for_each_req_async_bh() specifically double-checks whether it’s
still in the right context before invoking the specified function, so it
seems there was some intention to continue to run in the context associated
with the BB.

(Not judging whether that intent makes sense or not, yet.)


Maybe the correct solution is to remove the assertion from
blk_get_aio_context() and just always return blk->ctx. If it's in the
middle of a change, you'll either get the old one or the new one. Either
one is fine to submit I/O from, and if you care about changes for other
reasons (like SCSI does), then you need explicit code to protect it
anyway (which SCSI apparently has, but it doesn't work).

I think most callers do just assume the BB stays in the context they got
(without any proof, admittedly), but I agree that under re-evaluation, it
probably doesn’t actually matter to them, really. And yes, basically, if the
caller doesn’t need to take a lock because it doesn’t really matter whether
blk->ctx changes while its still using the old value, blk_get_aio_context()
in turn doesn’t need to double-check blk->ctx against the root node’s
context either, and nobody needs a lock.

So I agree, it’s on the caller to protect against a potentially changing
context, blk_get_aio_context() should just return blk->ctx and not check
against the root node.

(On a tangent: blk_drain() is a caller of blk_get_aio_context(), and it
polls that context.  Why does it need

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread BALATON Zoltan 

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but that 
header is in inlcude because some files outside hw/ide include it. I've 
found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal IDE 
parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow make 
this a local header where non-public parts of hw/ide/internal.h could be 
moved in the future. Such as rename include/hw/ide/internal.h to ide.h and 
name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to unentangle 
the stuff one day. So what's wrong with having a dedicated header for the 
stuff in hw/ide/qdev.c ?


Maybe that it's not obvious from the name that it belongs to qdev.c as the 
names are not the same. Also some of the qdev stuff that should be in this 
header are in include/hw/ide/internal.h so these will still be split 
arbitrarily.


Regards,
BALATON Zoltan

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread Thomas Huth 

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but that 
header is in inlcude because some files outside hw/ide include it. I've 
found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal IDE 
parts the other two just uses some functions so macio is probably the reason 
this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow make 
this a local header where non-public parts of hw/ide/internal.h could be 
moved in the future. Such as rename include/hw/ide/internal.h to ide.h and 
name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to unentangle 
the stuff one day. So what's wrong with having a dedicated header for the 
stuff in hw/ide/qdev.c ?


 Thomas

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread BALATON Zoltan 

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c| 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but that 
header is in inlcude because some files outside hw/ide include it. I've 
found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
IDE parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow 
make this a local header where non-public parts of hw/ide/internal.h could 
be moved in the future. Such as rename include/hw/ide/internal.h to ide.h 
and name this one internal.h maybe?


Regards,
BALATON Zoltan


@@ -0,0 +1,41 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann 
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#ifndef QDEV_IDE_H
+#define QDEV_IDE_H
+
+#include "hw/qdev-properties.h"
+#include "hw/block/block.h"
+#include "hw/ide/internal.h"
+
+typedef struct IDEDrive {
+IDEDevice dev;
+} IDEDrive;
+
+#define DEFINE_IDE_DEV_PROPERTIES() \
+DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
+DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
+DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
+DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+
+void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
+
+#endif
diff --git a/hw/ide/cf.c b/hw/ide/cf.c
new file mode 100644
index 00..0b4bb57591
--- /dev/null
+++ b/hw/ide/cf.c
@@ -0,0 +1,58 @@
+/*
+ * ide CompactFlash support
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ide/qdev-ide.h"
+#include "qapi/qapi-types-block.h"
+
+static void ide_cf_realize(IDEDevice *dev, Error **errp)
+{
+ide_dev_initfn(dev, IDE_CFATA, errp);
+}
+
+static Property ide_cf_properties[] = {
+DEFINE_IDE_DEV_PROPERTIES(),
+DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
+IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ide_cf_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
+
+k->realize  = ide_cf_realize;
+dc->fw_name = "drive";
+dc->desc= "virtual CompactFlash card";
+device_class_set_props(dc, ide_cf_properties);
+}
+
+static const TypeInfo ide_cf_info = {
+.name  = "ide-cf",
+.parent= TYPE_IDE_DEVICE,
+.instance_size = sizeof(IDEDrive),
+.class_init= ide_cf_class_init,
+};
+
+static void ide_cf_register_type(void)
+{
+

Re: [PATCH] pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes()

2024-02-01 Thread Michael Tokarev 

30.01.2024 03:27, Stefan Hajnoczi wrote:

The following expression is incorrect because blk_pread_nonzeroes()
deals in units of bytes, not sectors:

   bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS)
   ^^^

BDRV_REQUEST_MAX_BYTES is the appropriate constant.

Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image")
Cc: Xiang Zheng 
Signed-off-by: Stefan Hajnoczi 
---
  hw/block/block.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..ff503002aa 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr 
size, void *buf)
  BlockDriverState *bs = blk_bs(blk);
  
  for (;;) {

-bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS);
+bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES);


Hmm.  This smells like a -stable material, but you know better not
to Cc: qemu-stable@ for unrelated stuff...  Is it not for stable?

Thanks,

/mjt

Re: [PATCH v2] block/blkio: Make s->mem_region_alignment be 64 bits

2024-02-01 Thread Michael Tokarev 

30.01.2024 15:20, Richard W.M. Jones :

With GCC 14 the code failed to compile on i686 (and was wrong for any
version of GCC):

../block/blkio.c: In function ‘blkio_file_open’:
../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from 
incompatible pointer type [-Wincompatible-pointer-types]
   857 |>mem_region_alignment);
   |^~~~
   ||
   |size_t * {aka unsigned int *}
In file included from ../block/blkio.c:12:
/usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long 
unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t 
*value);
   | ~~^

Signed-off-by: Richard W.M. Jones 
---
  block/blkio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blkio.c b/block/blkio.c
index 0a0a6c0f5fd..bc2f21784c7 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -68,7 +68,7 @@ typedef struct {
  CoQueue bounce_available;
  
  /* The value of the "mem-region-alignment" property */

-size_t mem_region_alignment;
+uint64_t mem_region_alignment;


This seems like a stable material, is it not?

It's interesting gcc didn't warn about this one before, while it is
clearly a type mismatch..

/mjt

Re: [PULL 0/5] Block patches

2024-02-01 Thread Peter Maydell 
On Tue, 30 Jan 2024 at 21:51, Stefan Hajnoczi  wrote:
>
> The following changes since commit 11be70677c70fdccd452a3233653949b79e97908:
>
>   Merge tag 'pull-vfio-20240129' of https://github.com/legoater/qemu into 
> staging (2024-01-29 10:53:56 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 954b33daee83fe79293fd81c2f7371db48e7d6bd:
>
>   hw/block/block.c: improve confusing blk_check_size_and_read_all() error 
> (2024-01-30 16:19:00 -0500)
>
> 
> Pull request
>
> 

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0
for any user-visible changes.

-- PMM

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Hanna Czenczek 

On 01.02.24 11:21, Kevin Wolf wrote:

Am 01.02.2024 um 10:43 hat Hanna Czenczek geschrieben:

On 31.01.24 11:17, Kevin Wolf wrote:

Am 29.01.2024 um 17:30 hat Hanna Czenczek geschrieben:

I don’t like using drain as a form of lock specifically against AioContext
changes, but maybe Stefan is right, and we should use it in this specific
case to get just the single problem fixed.  (Though it’s not quite trivial
either.  We’d probably still want to remove the assertion from
blk_get_aio_context(), so we don’t have to require all of its callers to
hold a count in the in-flight counter.)

Okay, fair, maybe fixing the specific problem is more important that
solving the more generic blk_get_aio_context() race.

In this case, wouldn't it be enough to increase the in-flight counter so
that the drain before switching AioContexts would run the BH before
anything bad can happen? Does the following work?

Yes, that’s what I had in mind (Stefan, too, I think), and in testing,
it looks good.

Oh, sorry, I completely misunderstood then. I thought you were talking
about adding a new drained section somewhere and that sounded a bit more
complicated. :-)

If it works, let's do this. Would you like to pick this up and send it
as a formal patch (possibly in a more polished form), or should I do
that?


Sure, I can do it.

Hanna

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Kevin Wolf 
Am 01.02.2024 um 10:43 hat Hanna Czenczek geschrieben:
> On 31.01.24 11:17, Kevin Wolf wrote:
> > Am 29.01.2024 um 17:30 hat Hanna Czenczek geschrieben:
> > > I don’t like using drain as a form of lock specifically against AioContext
> > > changes, but maybe Stefan is right, and we should use it in this specific
> > > case to get just the single problem fixed.  (Though it’s not quite trivial
> > > either.  We’d probably still want to remove the assertion from
> > > blk_get_aio_context(), so we don’t have to require all of its callers to
> > > hold a count in the in-flight counter.)
> > Okay, fair, maybe fixing the specific problem is more important that
> > solving the more generic blk_get_aio_context() race.
> > 
> > In this case, wouldn't it be enough to increase the in-flight counter so
> > that the drain before switching AioContexts would run the BH before
> > anything bad can happen? Does the following work?
> 
> Yes, that’s what I had in mind (Stefan, too, I think), and in testing,
> it looks good.

Oh, sorry, I completely misunderstood then. I thought you were talking
about adding a new drained section somewhere and that sounded a bit more
complicated. :-)

If it works, let's do this. Would you like to pick this up and send it
as a formal patch (possibly in a more polished form), or should I do
that?

Kevin

Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

2024-02-01 Thread Hanna Czenczek 

On 31.01.24 11:17, Kevin Wolf wrote:

Am 29.01.2024 um 17:30 hat Hanna Czenczek geschrieben:

I don’t like using drain as a form of lock specifically against AioContext
changes, but maybe Stefan is right, and we should use it in this specific
case to get just the single problem fixed.  (Though it’s not quite trivial
either.  We’d probably still want to remove the assertion from
blk_get_aio_context(), so we don’t have to require all of its callers to
hold a count in the in-flight counter.)

Okay, fair, maybe fixing the specific problem is more important that
solving the more generic blk_get_aio_context() race.

In this case, wouldn't it be enough to increase the in-flight counter so
that the drain before switching AioContexts would run the BH before
anything bad can happen? Does the following work?


Yes, that’s what I had in mind (Stefan, too, I think), and in testing, 
it looks good.


Hanna



Kevin

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 0a2eb11c56..dc09eb8024 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -120,17 +120,11 @@ static void scsi_device_for_each_req_async_bh(void 
*opaque)
  SCSIRequest *next;
  
  /*

- * If the AioContext changed before this BH was called then reschedule into
- * the new AioContext before accessing ->requests. This can happen when
- * scsi_device_for_each_req_async() is called and then the AioContext is
- * changed before BHs are run.
+ * The AioContext can't have changed because we increased the in-flight
+ * counter for s->conf.blk.
   */
  ctx = blk_get_aio_context(s->conf.blk);
-if (ctx != qemu_get_current_aio_context()) {
-aio_bh_schedule_oneshot(ctx, scsi_device_for_each_req_async_bh,
-g_steal_pointer());
-return;
-}
+assert(ctx == qemu_get_current_aio_context());
  
  QTAILQ_FOREACH_SAFE(req, >requests, next, next) {

  data->fn(req, data->fn_opaque);
@@ -138,6 +132,7 @@ static void scsi_device_for_each_req_async_bh(void *opaque)
  
  /* Drop the reference taken by scsi_device_for_each_req_async() */

  object_unref(OBJECT(s));
+blk_dec_in_flight(s->conf.blk);
  }
  
  /*

@@ -163,6 +158,7 @@ static void scsi_device_for_each_req_async(SCSIDevice *s,
   */
  object_ref(OBJECT(s));
  
+blk_inc_in_flight(s->conf.blk);

  aio_bh_schedule_oneshot(blk_get_aio_context(s->conf.blk),
  scsi_device_for_each_req_async_bh,
  data);

[PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread Thomas Huth 
For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
 hw/ide/qdev-ide.h  | 41 
 hw/ide/cf.c| 58 ++
 hw/ide/qdev.c  | 51 ++--
 hw/ide/Kconfig |  4 
 hw/ide/meson.build |  1 +
 5 files changed, 106 insertions(+), 49 deletions(-)
 create mode 100644 hw/ide/qdev-ide.h
 create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h
@@ -0,0 +1,41 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann 
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#ifndef QDEV_IDE_H
+#define QDEV_IDE_H
+
+#include "hw/qdev-properties.h"
+#include "hw/block/block.h"
+#include "hw/ide/internal.h"
+
+typedef struct IDEDrive {
+IDEDevice dev;
+} IDEDrive;
+
+#define DEFINE_IDE_DEV_PROPERTIES() \
+DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
+DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
+DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
+DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+
+void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
+
+#endif
diff --git a/hw/ide/cf.c b/hw/ide/cf.c
new file mode 100644
index 00..0b4bb57591
--- /dev/null
+++ b/hw/ide/cf.c
@@ -0,0 +1,58 @@
+/*
+ * ide CompactFlash support
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ide/qdev-ide.h"
+#include "qapi/qapi-types-block.h"
+
+static void ide_cf_realize(IDEDevice *dev, Error **errp)
+{
+ide_dev_initfn(dev, IDE_CFATA, errp);
+}
+
+static Property ide_cf_properties[] = {
+DEFINE_IDE_DEV_PROPERTIES(),
+DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
+IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ide_cf_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
+
+k->realize  = ide_cf_realize;
+dc->fw_name = "drive";
+dc->desc= "virtual CompactFlash card";
+device_class_set_props(dc, ide_cf_properties);
+}
+
+static const TypeInfo ide_cf_info = {
+.name  = "ide-cf",
+.parent= TYPE_IDE_DEVICE,
+.instance_size = sizeof(IDEDrive),
+.class_init= ide_cf_class_init,
+};
+
+static void ide_cf_register_type(void)
+{
+type_register_static(_cf_info);
+}
+
+type_init(ide_cf_register_type)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 1b3b4da01d..a2f2d0ea08 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -24,12 +24,9 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
-#include "hw/ide/internal.h"
-#include "hw/qdev-properties.h"
-#include "hw/qdev-properties-system.h"
+#include "hw/ide/qdev-ide.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
-#include "hw/block/block.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 #include "qapi/visitor.h"
@@ -158,11 +155,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
 
 /* - */
 
-typedef struct IDEDrive {
-IDEDevice dev;
-} IDEDrive;
-
-static void ide_dev_initfn(IDEDevice *dev,