Re: [PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Philippe Mathieu-Daudé]

On 19/2/24 20:17, Thomas Huth wrote:

On 19/02/2024 12.32, Philippe Mathieu-Daudé wrote:

On 19/2/24 11:49, Thomas Huth wrote:

Let's start to unentangle internal.h by moving public IDE device
related definitions to ide-dev.h.

Signed-off-by: Thomas Huth 
---
  include/hw/ide/ide-dev.h  | 145 +-
  include/hw/ide/internal.h | 145 +-
  hw/ide/ide-dev.c  |   1 +
  3 files changed, 146 insertions(+), 145 deletions(-)

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index 7e9663cda9..de88784a25 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -20,9 +20,152 @@
  #ifndef IDE_DEV_H
  #define IDE_DEV_H
+#include "sysemu/dma.h"


Not required.


It's required for QEMUSGList that is used in struct IDEState.


Oh, OK.


  #include "hw/qdev-properties.h"
  #include "hw/block/block.h"
-#include "hw/ide/internal.h"
+
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEState IDEState;



+typedef struct IDEDMA IDEDMA;
+typedef struct IDEDMAOps IDEDMAOps;
+typedef struct IDEBus IDEBus;


Looking at next patches, better forward-declare IDEBus and
IDEDMA in "qemu/typedefs.h".


I really dislike using qemu/typedefs.h for things that are not really 
part of the core framework, since it's a 
touch-it-once-and-everything-gets-recompiled header. So IMHO the 
typedefs here are the lesser evil.


OK then.


IDEDMAOps and "sysemu/dma.h" belong to "hw/ide/ide-dma.h.


Ok, I can move the typedef for IDEDMAOps to ide-dma.h instead.

  Thomas

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Damien Le Moal]

On 2/20/24 06:15, Markus Armbruster wrote:
> Making this member @size mandatory means we must specify it when
> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
> @mode is "host-managed".  Feels right to me.  Am I missing anything?

 That's right. And the checks when creating such an img can help do
 that. It's not specified in the .json file directly.
>>>
>>> What would break if we did specify it in the QAPI schema directly?
>>
>> Nothing I think. We can keep the current schema and add a default zone
>> size like 131072.
> 
> I believe making the member mandatory makes a lot more sense.
> 
> I guess we can keep @capacity and @max-append-bytes keep optional *if*
> we can come up with sensible defaults.

Yes, @capacity can be optional and default to the zone size.  @max-append-bytes
can also be optional and default to the regular read/write max size.

However, defining a "sensible" default for the zone size is rather tricky. The
reason is that zones are generally sized according to the device speed. Zones
are 256MB on HDDs (which takes about 1s to fully write sequentially) while NVMe
ZNS devices will more likely have a zone size of 1-2 GB (because the device is
much faster). If we stick with such approach, a sensible zone size would depend
on how fast the qcow2 image backing is. I.e. 256 MB would be more appropriate
for a qcow2 image on a file stored on HDD while larger sizes may be better for
SSD backed images. But that is also not accounting for the host page caching
which can "show" the qcow2 image as being much faster than its backing storage
really is.

So I would be tempted to say that defaulting to 0 to force the user to specify a
zone size would be safer. But if you really want a non-0 default, then maybe
256MB or so may be OK as that is the most commonly used value out there for
zoned storage (there are literally millions of SMR drives running in production
systems out there and NVMe ZNS devices are not widely used yet).


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster]

Sam Li  writes:

> Markus Armbruster  于2024年2月19日周一 21:42写道：
>>
>> Sam Li  writes:
>>
>> > Markus Armbruster  于2024年2月19日周一 16:56写道：
>> >>
>> >> Sam Li  writes:
>> >>
>> >> > Markus Armbruster  于2024年2月19日周一 15:40写道：
>> >> >>
>> >> >> Sam Li  writes:
>> >> >>
>> >> >> > Markus Armbruster  于2024年2月19日周一 13:05写道：
>> >> >> >>
>> >> >> >> One more thing...
>> >> >> >>
>> >> >> >> Markus Armbruster  writes:
>> >> >> >>
>> >> >> >> > I apologize for the delayed review.
>> >> >> >
>> >> >> > No problems. Thanks for reviewing!
>> >> >> >
>> >> >> >> >
>> >> >> >> > Sam Li  writes:
>> >> >> >> >
>> >> >> >> >> To configure the zoned format feature on the qcow2 driver, it
>> >> >> >> >> requires settings as: the device size, zone model, zone size,
>> >> >> >> >> zone capacity, number of conventional zones, limits on zone
>> >> >> >> >> resources (max append bytes, max open zones, and 
>> >> >> >> >> max_active_zones).
>> >> >> >> >>
>> >> >> >> >> To create a qcow2 image with zoned format feature, use command 
>> >> >> >> >> like
>> >> >> >> >> this:
>> >> >> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> >> >> >> -o zone.size=64M -o zone.capacity=64M -o 
>> >> >> >> >> zone.conventional_zones=0 \
>> >> >> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> >> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >> >> >> >>
>> >> >> >> >> Signed-off-by: Sam Li 
>> >> >> >> >
>> >> >> >> > [...]
>> >> >> >> >
>> >> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> >> >> index ca390c5700..e2e0ec21a5 100644
>> >> >> >> >> --- a/qapi/block-core.json
>> >> >> >> >> +++ b/qapi/block-core.json
>> >> >> >> >> @@ -5038,6 +5038,67 @@
>> >> >> >> >>  { 'enum': 'Qcow2CompressionType',
>> >> >> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >> >> >> >>
>> >> >> >> >> +##
>> >> >> >> >> +# @Qcow2ZoneModel:
>> >> >> >> >> +#
>> >> >> >> >> +# Zoned device model used in qcow2 image file
>> >> >> >> >> +#
>> >> >> >> >> +# @host-managed: The host-managed model only allows sequential 
>> >> >> >> >> write over the
>> >> >> >> >> +# device zones.
>> >> >> >> >> +#
>> >> >> >> >> +# Since 8.2
>> >> >> >> >> +##
>> >> >> >> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> >> >> >> +  'data': [ 'host-managed'] }
>> >> >> >> >> +
>> >> >> >> >> +##
>> >> >> >> >> +# @Qcow2ZoneHostManaged:
>> >> >> >> >> +#
>> >> >> >> >> +# The host-managed zone model.  It only allows sequential 
>> >> >> >> >> writes.
>> >> >> >> >> +#
>> >> >> >> >> +# @size: Total number of bytes within zones.
>> >> >> >> >
>> >> >> >> > Default?
>> >> >> >
>> >> >> > It should be set by users. No default value provided. If it's unset
>> >> >> > then it is zero and an error will be returned.
>> >> >>
>> >> >> If the user must provide @size, why is it optional then?
>> >> >
>> >> > It is not optional when the zone model is host-managed. If it's
>> >> > non-zoned, then we don't care about zone info. I am not sure how to
>> >> > make it unoptional.
>> >>
>> >> We have:
>> >>
>> >>blockdev-create argument @options of type BlockdevCreateOptions
>> >>
>> >>BlockdevCreateOptions union branch @qcow2 of type
>> >>BlockdevCreateOptionsQcow2, union tag member is @driver
>> >>
>> >>BlockdevCreateOptionsQcow2 optional member @zone of type
>> >>Qcow2ZoneCreateOptions, default not zoned
>> >>
>> >>Qcow2ZoneCreateOptions union branch @host-managed of type
>> >>Qcow2ZoneHostManaged, union tag member is @mode
>> >>
>> >>Qcow2ZoneHostManaged optional member @size of type size.
>> >>
>> >> Making this member @size mandatory means we must specify it when
>> >> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
>> >> @mode is "host-managed".  Feels right to me.  Am I missing anything?
>> >
>> > That's right. And the checks when creating such an img can help do
>> > that. It's not specified in the .json file directly.
>>
>> What would break if we did specify it in the QAPI schema directly?
>
> Nothing I think. We can keep the current schema and add a default zone
> size like 131072.

I believe making the member mandatory makes a lot more sense.

I guess we can keep @capacity and @max-append-bytes keep optional *if*
we can come up with sensible defaults.

>> [...]

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Sam Li]

Markus Armbruster  于2024年2月19日周一 21:42写道：
>
> Sam Li  writes:
>
> > Markus Armbruster  于2024年2月19日周一 16:56写道：
> >>
> >> Sam Li  writes:
> >>
> >> > Markus Armbruster  于2024年2月19日周一 15:40写道：
> >> >>
> >> >> Sam Li  writes:
> >> >>
> >> >> > Markus Armbruster  于2024年2月19日周一 13:05写道：
> >> >> >>
> >> >> >> One more thing...
> >> >> >>
> >> >> >> Markus Armbruster  writes:
> >> >> >>
> >> >> >> > I apologize for the delayed review.
> >> >> >
> >> >> > No problems. Thanks for reviewing!
> >> >> >
> >> >> >> >
> >> >> >> > Sam Li  writes:
> >> >> >> >
> >> >> >> >> To configure the zoned format feature on the qcow2 driver, it
> >> >> >> >> requires settings as: the device size, zone model, zone size,
> >> >> >> >> zone capacity, number of conventional zones, limits on zone
> >> >> >> >> resources (max append bytes, max open zones, and 
> >> >> >> >> max_active_zones).
> >> >> >> >>
> >> >> >> >> To create a qcow2 image with zoned format feature, use command 
> >> >> >> >> like
> >> >> >> >> this:
> >> >> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
> >> >> >> >> -o zone.size=64M -o zone.capacity=64M -o 
> >> >> >> >> zone.conventional_zones=0 \
> >> >> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
> >> >> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
> >> >> >> >>
> >> >> >> >> Signed-off-by: Sam Li 
> >> >> >> >
> >> >> >> > [...]
> >> >> >> >
> >> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> >> >> index ca390c5700..e2e0ec21a5 100644
> >> >> >> >> --- a/qapi/block-core.json
> >> >> >> >> +++ b/qapi/block-core.json
> >> >> >> >> @@ -5038,6 +5038,67 @@
> >> >> >> >>  { 'enum': 'Qcow2CompressionType',
> >> >> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
> >> >> >> >>
> >> >> >> >> +##
> >> >> >> >> +# @Qcow2ZoneModel:
> >> >> >> >> +#
> >> >> >> >> +# Zoned device model used in qcow2 image file
> >> >> >> >> +#
> >> >> >> >> +# @host-managed: The host-managed model only allows sequential 
> >> >> >> >> write over the
> >> >> >> >> +# device zones.
> >> >> >> >> +#
> >> >> >> >> +# Since 8.2
> >> >> >> >> +##
> >> >> >> >> +{ 'enum': 'Qcow2ZoneModel',
> >> >> >> >> +  'data': [ 'host-managed'] }
> >> >> >> >> +
> >> >> >> >> +##
> >> >> >> >> +# @Qcow2ZoneHostManaged:
> >> >> >> >> +#
> >> >> >> >> +# The host-managed zone model.  It only allows sequential writes.
> >> >> >> >> +#
> >> >> >> >> +# @size: Total number of bytes within zones.
> >> >> >> >
> >> >> >> > Default?
> >> >> >
> >> >> > It should be set by users. No default value provided. If it's unset
> >> >> > then it is zero and an error will be returned.
> >> >>
> >> >> If the user must provide @size, why is it optional then?
> >> >
> >> > It is not optional when the zone model is host-managed. If it's
> >> > non-zoned, then we don't care about zone info. I am not sure how to
> >> > make it unoptional.
> >>
> >> We have:
> >>
> >>blockdev-create argument @options of type BlockdevCreateOptions
> >>
> >>BlockdevCreateOptions union branch @qcow2 of type
> >>BlockdevCreateOptionsQcow2, union tag member is @driver
> >>
> >>BlockdevCreateOptionsQcow2 optional member @zone of type
> >>Qcow2ZoneCreateOptions, default not zoned
> >>
> >>Qcow2ZoneCreateOptions union branch @host-managed of type
> >>Qcow2ZoneHostManaged, union tag member is @mode
> >>
> >>Qcow2ZoneHostManaged optional member @size of type size.
> >>
> >> Making this member @size mandatory means we must specify it when
> >> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
> >> @mode is "host-managed".  Feels right to me.  Am I missing anything?
> >
> > That's right. And the checks when creating such an img can help do
> > that. It's not specified in the .json file directly.
>
> What would break if we did specify it in the QAPI schema directly?

Nothing I think. We can keep the current schema and add a default zone
size like 131072.

>
> [...]
>

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster]

Sam Li  writes:

> Markus Armbruster  于2024年2月19日周一 16:56写道：
>>
>> Sam Li  writes:
>>
>> > Markus Armbruster  于2024年2月19日周一 15:40写道：
>> >>
>> >> Sam Li  writes:
>> >>
>> >> > Markus Armbruster  于2024年2月19日周一 13:05写道：
>> >> >>
>> >> >> One more thing...
>> >> >>
>> >> >> Markus Armbruster  writes:
>> >> >>
>> >> >> > I apologize for the delayed review.
>> >> >
>> >> > No problems. Thanks for reviewing!
>> >> >
>> >> >> >
>> >> >> > Sam Li  writes:
>> >> >> >
>> >> >> >> To configure the zoned format feature on the qcow2 driver, it
>> >> >> >> requires settings as: the device size, zone model, zone size,
>> >> >> >> zone capacity, number of conventional zones, limits on zone
>> >> >> >> resources (max append bytes, max open zones, and max_active_zones).
>> >> >> >>
>> >> >> >> To create a qcow2 image with zoned format feature, use command like
>> >> >> >> this:
>> >> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> >> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> >> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >> >> >>
>> >> >> >> Signed-off-by: Sam Li 
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> >> index ca390c5700..e2e0ec21a5 100644
>> >> >> >> --- a/qapi/block-core.json
>> >> >> >> +++ b/qapi/block-core.json
>> >> >> >> @@ -5038,6 +5038,67 @@
>> >> >> >>  { 'enum': 'Qcow2CompressionType',
>> >> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >> >> >>
>> >> >> >> +##
>> >> >> >> +# @Qcow2ZoneModel:
>> >> >> >> +#
>> >> >> >> +# Zoned device model used in qcow2 image file
>> >> >> >> +#
>> >> >> >> +# @host-managed: The host-managed model only allows sequential 
>> >> >> >> write over the
>> >> >> >> +# device zones.
>> >> >> >> +#
>> >> >> >> +# Since 8.2
>> >> >> >> +##
>> >> >> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> >> >> +  'data': [ 'host-managed'] }
>> >> >> >> +
>> >> >> >> +##
>> >> >> >> +# @Qcow2ZoneHostManaged:
>> >> >> >> +#
>> >> >> >> +# The host-managed zone model.  It only allows sequential writes.
>> >> >> >> +#
>> >> >> >> +# @size: Total number of bytes within zones.
>> >> >> >
>> >> >> > Default?
>> >> >
>> >> > It should be set by users. No default value provided. If it's unset
>> >> > then it is zero and an error will be returned.
>> >>
>> >> If the user must provide @size, why is it optional then?
>> >
>> > It is not optional when the zone model is host-managed. If it's
>> > non-zoned, then we don't care about zone info. I am not sure how to
>> > make it unoptional.
>>
>> We have:
>>
>>blockdev-create argument @options of type BlockdevCreateOptions
>>
>>BlockdevCreateOptions union branch @qcow2 of type
>>BlockdevCreateOptionsQcow2, union tag member is @driver
>>
>>BlockdevCreateOptionsQcow2 optional member @zone of type
>>Qcow2ZoneCreateOptions, default not zoned
>>
>>Qcow2ZoneCreateOptions union branch @host-managed of type
>>Qcow2ZoneHostManaged, union tag member is @mode
>>
>>Qcow2ZoneHostManaged optional member @size of type size.
>>
>> Making this member @size mandatory means we must specify it when
>> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
>> @mode is "host-managed".  Feels right to me.  Am I missing anything?
>
> That's right. And the checks when creating such an img can help do
> that. It's not specified in the .json file directly.

What would break if we did specify it in the QAPI schema directly?

[...]

Re: [PATCH 5/7] hw/ide: Move IDE DMA related definitions to a separate header ide-dma.h

[Thomas Huth]

On 19/02/2024 12.53, BALATON Zoltan wrote:

On Mon, 19 Feb 2024, Thomas Huth wrote:

These definitions are required outside of the hw/ide/ code, too,
so lets's move them from internal.h to a new header called ide-dma.h.

Signed-off-by: Thomas Huth 
---
include/hw/ide/ide-dma.h  | 30 ++
include/hw/ide/internal.h | 27 +--
2 files changed, 31 insertions(+), 26 deletions(-)
create mode 100644 include/hw/ide/ide-dma.h

diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
new file mode 100644
index 00..fb82966bdd
--- /dev/null
+++ b/include/hw/ide/ide-dma.h
@@ -0,0 +1,30 @@
+#ifndef HW_IDE_DMA_H
+#define HW_IDE_DMA_H
+
+typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc 
*);

+typedef void DMAVoidFunc(const IDEDMA *);
+typedef int DMAIntFunc(const IDEDMA *, bool);
+typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
+typedef void DMAu32Func(const IDEDMA *, uint32_t);
+typedef void DMAStopFunc(const IDEDMA *, bool);
+
+struct IDEDMAOps {
+    DMAStartFunc *start_dma;
+    DMAVoidFunc *pio_transfer;
+    DMAInt32Func *prepare_buf;
+    DMAu32Func *commit_buf;
+    DMAIntFunc *rw_buf;
+    DMAVoidFunc *restart;
+    DMAVoidFunc *restart_dma;
+    DMAStopFunc *set_inactive;
+    DMAVoidFunc *cmd_done;
+    DMAVoidFunc *reset;
+};
+
+struct IDEDMA {
+    const struct IDEDMAOps *ops;
+    QEMUIOVector qiov;
+    BlockAIOCB *aiocb;


Doesn't this need to #include something to define QEMUIOVector and 
BlockAIOCB and some of the DMA and IDE types not defined above?


Yes, it currently works by accident since the header is only included in 
spots where those things are already defined, but I'll better add some 
#include statements here so that this header can also be used stand-alone in 
other spots.


 Thomas

Re: [PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Thomas Huth]

On 19/02/2024 12.32, Philippe Mathieu-Daudé wrote:

On 19/2/24 11:49, Thomas Huth wrote:

Let's start to unentangle internal.h by moving public IDE device
related definitions to ide-dev.h.

Signed-off-by: Thomas Huth 
---
  include/hw/ide/ide-dev.h  | 145 +-
  include/hw/ide/internal.h | 145 +-
  hw/ide/ide-dev.c  |   1 +
  3 files changed, 146 insertions(+), 145 deletions(-)

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index 7e9663cda9..de88784a25 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -20,9 +20,152 @@
  #ifndef IDE_DEV_H
  #define IDE_DEV_H
+#include "sysemu/dma.h"


Not required.


It's required for QEMUSGList that is used in struct IDEState.


  #include "hw/qdev-properties.h"
  #include "hw/block/block.h"
-#include "hw/ide/internal.h"
+
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEState IDEState;



+typedef struct IDEDMA IDEDMA;
+typedef struct IDEDMAOps IDEDMAOps;
+typedef struct IDEBus IDEBus;


Looking at next patches, better forward-declare IDEBus and
IDEDMA in "qemu/typedefs.h".


I really dislike using qemu/typedefs.h for things that are not really part 
of the core framework, since it's a 
touch-it-once-and-everything-gets-recompiled header. So IMHO the typedefs 
here are the lesser evil.



IDEDMAOps and "sysemu/dma.h" belong to "hw/ide/ide-dma.h.


Ok, I can move the typedef for IDEDMAOps to ide-dma.h instead.

 Thomas

Re: [PATCH 2/7] hw/ide: Split qdev.c into ide-bus.c and ide-dev.c

[Thomas Huth]

On 19/02/2024 12.45, BALATON Zoltan wrote:

On Mon, 19 Feb 2024, Thomas Huth wrote:

qdev.c is a mixture between IDE bus specific functions and IDE device
functions. Let's split it up to make it more obvious which part is
related to bus handling and which part is related to device handling.

Signed-off-by: Thomas Huth 
---
hw/ide/ide-bus.c | 111 +++
hw/ide/{qdev.c => ide-dev.c} |  87 +--
hw/arm/Kconfig   |   2 +
hw/ide/Kconfig   |  30 ++
hw/ide/meson.build   |   3 +-
5 files changed, 134 insertions(+), 99 deletions(-)
create mode 100644 hw/ide/ide-bus.c
rename hw/ide/{qdev.c => ide-dev.c} (78%)

[...]

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 29abe1da29..b372b819a4 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -275,6 +275,8 @@ config SBSA_REF
    select USB_XHCI_SYSBUS
    select WDT_SBSA
    select BOCHS_DISPLAY
+    select IDE_BUS
+    select IDE_DEV

config SABRELITE
    bool
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index b93d6743d5..6dfc5a2129 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -1,51 +1,58 @@
config IDE_CORE
    bool

-config IDE_QDEV
+config IDE_BUS
    bool
    select IDE_CORE


Maybe we can assume if something has an IDE bus it also wants to connect IDE 
devices to it so just select IDE_DEV here and not at every place IDE_BUS is 
selected? Or is there a place that only wants IDE_BUS?


Currently not, but I think it's conceptually much cleaner if we are explicit 
here.


 Thomas

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Sam Li]

Markus Armbruster  于2024年2月19日周一 16:56写道：
>
> Sam Li  writes:
>
> > Markus Armbruster  于2024年2月19日周一 15:40写道：
> >>
> >> Sam Li  writes:
> >>
> >> > Markus Armbruster  于2024年2月19日周一 13:05写道：
> >> >>
> >> >> One more thing...
> >> >>
> >> >> Markus Armbruster  writes:
> >> >>
> >> >> > I apologize for the delayed review.
> >> >
> >> > No problems. Thanks for reviewing!
> >> >
> >> >> >
> >> >> > Sam Li  writes:
> >> >> >
> >> >> >> To configure the zoned format feature on the qcow2 driver, it
> >> >> >> requires settings as: the device size, zone model, zone size,
> >> >> >> zone capacity, number of conventional zones, limits on zone
> >> >> >> resources (max append bytes, max open zones, and max_active_zones).
> >> >> >>
> >> >> >> To create a qcow2 image with zoned format feature, use command like
> >> >> >> this:
> >> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
> >> >> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
> >> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
> >> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
> >> >> >>
> >> >> >> Signed-off-by: Sam Li 
> >> >> >
> >> >> > [...]
> >> >> >
> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> >> index ca390c5700..e2e0ec21a5 100644
> >> >> >> --- a/qapi/block-core.json
> >> >> >> +++ b/qapi/block-core.json
> >> >> >> @@ -5038,6 +5038,67 @@
> >> >> >>  { 'enum': 'Qcow2CompressionType',
> >> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
> >> >> >>
> >> >> >> +##
> >> >> >> +# @Qcow2ZoneModel:
> >> >> >> +#
> >> >> >> +# Zoned device model used in qcow2 image file
> >> >> >> +#
> >> >> >> +# @host-managed: The host-managed model only allows sequential 
> >> >> >> write over the
> >> >> >> +# device zones.
> >> >> >> +#
> >> >> >> +# Since 8.2
> >> >> >> +##
> >> >> >> +{ 'enum': 'Qcow2ZoneModel',
> >> >> >> +  'data': [ 'host-managed'] }
> >> >> >> +
> >> >> >> +##
> >> >> >> +# @Qcow2ZoneHostManaged:
> >> >> >> +#
> >> >> >> +# The host-managed zone model.  It only allows sequential writes.
> >> >> >> +#
> >> >> >> +# @size: Total number of bytes within zones.
> >> >> >
> >> >> > Default?
> >> >
> >> > It should be set by users. No default value provided. If it's unset
> >> > then it is zero and an error will be returned.
> >>
> >> If the user must provide @size, why is it optional then?
> >
> > It is not optional when the zone model is host-managed. If it's
> > non-zoned, then we don't care about zone info. I am not sure how to
> > make it unoptional.
>
> We have:
>
>blockdev-create argument @options of type BlockdevCreateOptions
>
>BlockdevCreateOptions union branch @qcow2 of type
>BlockdevCreateOptionsQcow2, union tag member is @driver
>
>BlockdevCreateOptionsQcow2 optional member @zone of type
>Qcow2ZoneCreateOptions, default not zoned
>
>Qcow2ZoneCreateOptions union branch @host-managed of type
>Qcow2ZoneHostManaged, union tag member is @mode
>
>Qcow2ZoneHostManaged optional member @size of type size.
>
> Making this member @size mandatory means we must specify it when
> BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
> @mode is "host-managed".  Feels right to me.  Am I missing anything?

That's right. And the checks when creating such an img can help do
that. It's not specified in the .json file directly.

>
> >>
> >> >> >
> >> >> >> +#
> >> >> >> +# @capacity: The number of usable logical blocks within zones
> >> >> >> +# in bytes.  A zone capacity is always smaller or equal to the
> >> >> >> +# zone size.
> >> >> >
> >> >> > Default?
> >> >
> >> > Same.
> >> >
> >> >> >
> >> >> >> +# @max-append-bytes: The maximal number of bytes of a zone
> >> >> >> +# append request that can be issued to the device.  It must be
> >> >> >> +# 512-byte aligned and less than the zone capacity.
> >> >> >
> >> >> > Default?
> >> >
> >> > Same.
> >> >
> >> > For those values, I guess it could be set when users provide no
> >> > information and still want a workable emulated zoned block device.
> >> >
> >> >> >
> >> >> >> +#
> >> >> >> +# Since 8.2
> >> >> >> +##
> >> >> >> +{ 'struct': 'Qcow2ZoneHostManaged',
> >> >> >> +  'data': { '*size':  'size',
> >> >> >> +'*capacity':  'size',
> >> >> >> +'*conventional-zones': 'uint32',
> >> >> >> +'*max-open-zones': 'uint32',
> >> >> >> +'*max-active-zones':   'uint32',
> >> >> >> +'*max-append-bytes':   'size' } }
> >>
> >> [...]
> >>
>

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster]

Sam Li  writes:

> Markus Armbruster  于2024年2月19日周一 15:40写道：
>>
>> Sam Li  writes:
>>
>> > Markus Armbruster  于2024年2月19日周一 13:05写道：
>> >>
>> >> One more thing...
>> >>
>> >> Markus Armbruster  writes:
>> >>
>> >> > I apologize for the delayed review.
>> >
>> > No problems. Thanks for reviewing!
>> >
>> >> >
>> >> > Sam Li  writes:
>> >> >
>> >> >> To configure the zoned format feature on the qcow2 driver, it
>> >> >> requires settings as: the device size, zone model, zone size,
>> >> >> zone capacity, number of conventional zones, limits on zone
>> >> >> resources (max append bytes, max open zones, and max_active_zones).
>> >> >>
>> >> >> To create a qcow2 image with zoned format feature, use command like
>> >> >> this:
>> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >> >>
>> >> >> Signed-off-by: Sam Li 
>> >> >
>> >> > [...]
>> >> >
>> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> >> index ca390c5700..e2e0ec21a5 100644
>> >> >> --- a/qapi/block-core.json
>> >> >> +++ b/qapi/block-core.json
>> >> >> @@ -5038,6 +5038,67 @@
>> >> >>  { 'enum': 'Qcow2CompressionType',
>> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >> >>
>> >> >> +##
>> >> >> +# @Qcow2ZoneModel:
>> >> >> +#
>> >> >> +# Zoned device model used in qcow2 image file
>> >> >> +#
>> >> >> +# @host-managed: The host-managed model only allows sequential write 
>> >> >> over the
>> >> >> +# device zones.
>> >> >> +#
>> >> >> +# Since 8.2
>> >> >> +##
>> >> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> >> +  'data': [ 'host-managed'] }
>> >> >> +
>> >> >> +##
>> >> >> +# @Qcow2ZoneHostManaged:
>> >> >> +#
>> >> >> +# The host-managed zone model.  It only allows sequential writes.
>> >> >> +#
>> >> >> +# @size: Total number of bytes within zones.
>> >> >
>> >> > Default?
>> >
>> > It should be set by users. No default value provided. If it's unset
>> > then it is zero and an error will be returned.
>>
>> If the user must provide @size, why is it optional then?
>
> It is not optional when the zone model is host-managed. If it's
> non-zoned, then we don't care about zone info. I am not sure how to
> make it unoptional.

We have:

   blockdev-create argument @options of type BlockdevCreateOptions

   BlockdevCreateOptions union branch @qcow2 of type
   BlockdevCreateOptionsQcow2, union tag member is @driver

   BlockdevCreateOptionsQcow2 optional member @zone of type
   Qcow2ZoneCreateOptions, default not zoned

   Qcow2ZoneCreateOptions union branch @host-managed of type
   Qcow2ZoneHostManaged, union tag member is @mode

   Qcow2ZoneHostManaged optional member @size of type size.

Making this member @size mandatory means we must specify it when
BlockdevCreateOptionsQcow2 member @zone is present and @zone's member
@mode is "host-managed".  Feels right to me.  Am I missing anything?

>>
>> >> >
>> >> >> +#
>> >> >> +# @capacity: The number of usable logical blocks within zones
>> >> >> +# in bytes.  A zone capacity is always smaller or equal to the
>> >> >> +# zone size.
>> >> >
>> >> > Default?
>> >
>> > Same.
>> >
>> >> >
>> >> >> +# @max-append-bytes: The maximal number of bytes of a zone
>> >> >> +# append request that can be issued to the device.  It must be
>> >> >> +# 512-byte aligned and less than the zone capacity.
>> >> >
>> >> > Default?
>> >
>> > Same.
>> >
>> > For those values, I guess it could be set when users provide no
>> > information and still want a workable emulated zoned block device.
>> >
>> >> >
>> >> >> +#
>> >> >> +# Since 8.2
>> >> >> +##
>> >> >> +{ 'struct': 'Qcow2ZoneHostManaged',
>> >> >> +  'data': { '*size':  'size',
>> >> >> +'*capacity':  'size',
>> >> >> +'*conventional-zones': 'uint32',
>> >> >> +'*max-open-zones': 'uint32',
>> >> >> +'*max-active-zones':   'uint32',
>> >> >> +'*max-append-bytes':   'size' } }
>>
>> [...]
>>

Re: [PATCH] iotests: adapt to output change for recently introduced 'detached header' field

[Daniel P . Berrangé]

On Fri, Feb 16, 2024 at 11:14:15AM +0100, Fiona Ebner wrote:
> Failure was noticed when running the tests for the qcow2 image format.
> 
> Fixes: 0bd779e27e ("crypto: Introduce 'detached-header' field in 
> QCryptoBlockInfoLUKS")
> Signed-off-by: Fiona Ebner 
> ---
>  tests/qemu-iotests/198.out | 2 ++
>  tests/qemu-iotests/206.out | 1 +
>  2 files changed, 3 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Sam Li]

Markus Armbruster  于2024年2月19日周一 15:40写道：
>
> Sam Li  writes:
>
> > Markus Armbruster  于2024年2月19日周一 13:05写道：
> >>
> >> One more thing...
> >>
> >> Markus Armbruster  writes:
> >>
> >> > I apologize for the delayed review.
> >
> > No problems. Thanks for reviewing!
> >
> >> >
> >> > Sam Li  writes:
> >> >
> >> >> To configure the zoned format feature on the qcow2 driver, it
> >> >> requires settings as: the device size, zone model, zone size,
> >> >> zone capacity, number of conventional zones, limits on zone
> >> >> resources (max append bytes, max open zones, and max_active_zones).
> >> >>
> >> >> To create a qcow2 image with zoned format feature, use command like
> >> >> this:
> >> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
> >> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
> >> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
> >> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
> >> >>
> >> >> Signed-off-by: Sam Li 
> >> >
> >> > [...]
> >> >
> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >> index ca390c5700..e2e0ec21a5 100644
> >> >> --- a/qapi/block-core.json
> >> >> +++ b/qapi/block-core.json
> >> >> @@ -5038,6 +5038,67 @@
> >> >>  { 'enum': 'Qcow2CompressionType',
> >> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
> >> >>
> >> >> +##
> >> >> +# @Qcow2ZoneModel:
> >> >> +#
> >> >> +# Zoned device model used in qcow2 image file
> >> >> +#
> >> >> +# @host-managed: The host-managed model only allows sequential write 
> >> >> over the
> >> >> +# device zones.
> >> >> +#
> >> >> +# Since 8.2
> >> >> +##
> >> >> +{ 'enum': 'Qcow2ZoneModel',
> >> >> +  'data': [ 'host-managed'] }
> >> >> +
> >> >> +##
> >> >> +# @Qcow2ZoneHostManaged:
> >> >> +#
> >> >> +# The host-managed zone model.  It only allows sequential writes.
> >> >> +#
> >> >> +# @size: Total number of bytes within zones.
> >> >
> >> > Default?
> >
> > It should be set by users. No default value provided. If it's unset
> > then it is zero and an error will be returned.
>
> If the user must provide @size, why is it optional then?

It is not optional when the zone model is host-managed. If it's
non-zoned, then we don't care about zone info. I am not sure how to
make it unoptional.

>
> >> >
> >> >> +#
> >> >> +# @capacity: The number of usable logical blocks within zones
> >> >> +# in bytes.  A zone capacity is always smaller or equal to the
> >> >> +# zone size.
> >> >
> >> > Default?
> >
> > Same.
> >
> >> >
> >> >> +# @max-append-bytes: The maximal number of bytes of a zone
> >> >> +# append request that can be issued to the device.  It must be
> >> >> +# 512-byte aligned and less than the zone capacity.
> >> >
> >> > Default?
> >
> > Same.
> >
> > For those values, I guess it could be set when users provide no
> > information and still want a workable emulated zoned block device.
> >
> >> >
> >> >> +#
> >> >> +# Since 8.2
> >> >> +##
> >> >> +{ 'struct': 'Qcow2ZoneHostManaged',
> >> >> +  'data': { '*size':  'size',
> >> >> +'*capacity':  'size',
> >> >> +'*conventional-zones': 'uint32',
> >> >> +'*max-open-zones': 'uint32',
> >> >> +'*max-active-zones':   'uint32',
> >> >> +'*max-append-bytes':   'size' } }
>
> [...]
>

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster]

Sam Li  writes:

> Markus Armbruster  于2024年2月19日周一 13:05写道：
>>
>> One more thing...
>>
>> Markus Armbruster  writes:
>>
>> > I apologize for the delayed review.
>
> No problems. Thanks for reviewing!
>
>> >
>> > Sam Li  writes:
>> >
>> >> To configure the zoned format feature on the qcow2 driver, it
>> >> requires settings as: the device size, zone model, zone size,
>> >> zone capacity, number of conventional zones, limits on zone
>> >> resources (max append bytes, max open zones, and max_active_zones).
>> >>
>> >> To create a qcow2 image with zoned format feature, use command like
>> >> this:
>> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
>> >>
>> >> Signed-off-by: Sam Li 
>> >
>> > [...]
>> >
>> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >> index ca390c5700..e2e0ec21a5 100644
>> >> --- a/qapi/block-core.json
>> >> +++ b/qapi/block-core.json
>> >> @@ -5038,6 +5038,67 @@
>> >>  { 'enum': 'Qcow2CompressionType',
>> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>> >>
>> >> +##
>> >> +# @Qcow2ZoneModel:
>> >> +#
>> >> +# Zoned device model used in qcow2 image file
>> >> +#
>> >> +# @host-managed: The host-managed model only allows sequential write 
>> >> over the
>> >> +# device zones.
>> >> +#
>> >> +# Since 8.2
>> >> +##
>> >> +{ 'enum': 'Qcow2ZoneModel',
>> >> +  'data': [ 'host-managed'] }
>> >> +
>> >> +##
>> >> +# @Qcow2ZoneHostManaged:
>> >> +#
>> >> +# The host-managed zone model.  It only allows sequential writes.
>> >> +#
>> >> +# @size: Total number of bytes within zones.
>> >
>> > Default?
>
> It should be set by users. No default value provided. If it's unset
> then it is zero and an error will be returned.

If the user must provide @size, why is it optional then?

>> >
>> >> +#
>> >> +# @capacity: The number of usable logical blocks within zones
>> >> +# in bytes.  A zone capacity is always smaller or equal to the
>> >> +# zone size.
>> >
>> > Default?
>
> Same.
>
>> >
>> >> +# @max-append-bytes: The maximal number of bytes of a zone
>> >> +# append request that can be issued to the device.  It must be
>> >> +# 512-byte aligned and less than the zone capacity.
>> >
>> > Default?
>
> Same.
>
> For those values, I guess it could be set when users provide no
> information and still want a workable emulated zoned block device.
>
>> >
>> >> +#
>> >> +# Since 8.2
>> >> +##
>> >> +{ 'struct': 'Qcow2ZoneHostManaged',
>> >> +  'data': { '*size':  'size',
>> >> +'*capacity':  'size',
>> >> +'*conventional-zones': 'uint32',
>> >> +'*max-open-zones': 'uint32',
>> >> +'*max-active-zones':   'uint32',
>> >> +'*max-append-bytes':   'size' } }

[...]

Re: [PATCH 19/21] hw/s390x/zpci-bus: Add QOM parentship relation with zPCI devices

[Philippe Mathieu-Daudé]

On 19/2/24 14:38, Thomas Huth wrote:

On 16/02/2024 12.03, Philippe Mathieu-Daudé wrote:

QDev objects created with qdev_*new() need to manually add
their parent relationship with object_property_add_child().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/s390x/s390-pci-bus.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3e57d5faca..6d07a7b530 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -934,6 +934,7 @@ static S390PCIBusDevice 
*s390_pci_device_new(S390pciState *s,

  "zPCI device could not be created: ");
  return NULL;
  }
+    object_property_add_child(OBJECT(s), "zpci[*]", OBJECT(dev));


I think there can only be one zpci device per PCI device, so do we need 
the "[*]" here?


Oh I missed that, I'll change, thanks.

Re: [PATCH 19/21] hw/s390x/zpci-bus: Add QOM parentship relation with zPCI devices

[Thomas Huth]

On 16/02/2024 12.03, Philippe Mathieu-Daudé wrote:

QDev objects created with qdev_*new() need to manually add
their parent relationship with object_property_add_child().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/s390x/s390-pci-bus.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 3e57d5faca..6d07a7b530 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -934,6 +934,7 @@ static S390PCIBusDevice *s390_pci_device_new(S390pciState 
*s,
  "zPCI device could not be created: ");
  return NULL;
  }
+object_property_add_child(OBJECT(s), "zpci[*]", OBJECT(dev));


I think there can only be one zpci device per PCI device, so do we need the 
"[*]" here?


 Thomas


  if (!qdev_realize_and_unref(dev, BUS(s->bus), _err)) {
  object_unparent(OBJECT(dev));
  error_propagate_prepend(errp, local_err,

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Sam Li]

Markus Armbruster  于2024年2月19日周一 13:05写道：
>
> One more thing...
>
> Markus Armbruster  writes:
>
> > I apologize for the delayed review.

No problems. Thanks for reviewing!

> >
> > Sam Li  writes:
> >
> >> To configure the zoned format feature on the qcow2 driver, it
> >> requires settings as: the device size, zone model, zone size,
> >> zone capacity, number of conventional zones, limits on zone
> >> resources (max append bytes, max open zones, and max_active_zones).
> >>
> >> To create a qcow2 image with zoned format feature, use command like
> >> this:
> >> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
> >> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
> >> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
> >> -o zone.max_active_zones=8 -o zone.mode=host-managed
> >>
> >> Signed-off-by: Sam Li 
> >
> > [...]
> >
> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> index ca390c5700..e2e0ec21a5 100644
> >> --- a/qapi/block-core.json
> >> +++ b/qapi/block-core.json
> >> @@ -5038,6 +5038,67 @@
> >>  { 'enum': 'Qcow2CompressionType',
> >>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
> >>
> >> +##
> >> +# @Qcow2ZoneModel:
> >> +#
> >> +# Zoned device model used in qcow2 image file
> >> +#
> >> +# @host-managed: The host-managed model only allows sequential write over 
> >> the
> >> +# device zones.
> >> +#
> >> +# Since 8.2
> >> +##
> >> +{ 'enum': 'Qcow2ZoneModel',
> >> +  'data': [ 'host-managed'] }
> >> +
> >> +##
> >> +# @Qcow2ZoneHostManaged:
> >> +#
> >> +# The host-managed zone model.  It only allows sequential writes.
> >> +#
> >> +# @size: Total number of bytes within zones.
> >
> > Default?

It should be set by users. No default value provided. If it's unset
then it is zero and an error will be returned.

> >
> >> +#
> >> +# @capacity: The number of usable logical blocks within zones
> >> +# in bytes.  A zone capacity is always smaller or equal to the
> >> +# zone size.
> >
> > Default?

Same.

> >
> >> +# @max-append-bytes: The maximal number of bytes of a zone
> >> +# append request that can be issued to the device.  It must be
> >> +# 512-byte aligned and less than the zone capacity.
> >
> > Default?

Same.

For those values, I guess it could be set when users provide no
information and still want a workable emulated zoned block device.

> >
> >> +#
> >> +# Since 8.2
> >> +##
> >> +{ 'struct': 'Qcow2ZoneHostManaged',
> >> +  'data': { '*size':  'size',
> >> +'*capacity':  'size',
> >> +'*conventional-zones': 'uint32',
> >> +'*max-open-zones': 'uint32',
> >> +'*max-active-zones':   'uint32',
> >> +'*max-append-bytes':   'size' } }
> >> +
> >> +##
> >> +# @Qcow2ZoneCreateOptions:
> >> +#
> >> +# The zone device model for the qcow2 image.
>
> Please document member @mode.
>
> Fails to build since merge commit 61e7a0d27c1:
>
> qapi/block-core.json: In union 'Qcow2ZoneCreateOptions':
> qapi/block-core.json:5135: member 'mode' lacks documentation
>

I see. Will update to the latest commit.

> >> +#
> >> +# Since 8.2
> >> +##
> >> +{ 'union': 'Qcow2ZoneCreateOptions',
> >> +  'base': { 'mode': 'Qcow2ZoneModel' },
> >> +  'discriminator': 'mode',
> >> +  'data': { 'host-managed': 'Qcow2ZoneHostManaged' } }
> >> +
> >>  ##
> >>  # @BlockdevCreateOptionsQcow2:
> >>  #
> >> @@ -5080,6 +5141,9 @@
> >>  # @compression-type: The image cluster compression method
> >>  # (default: zlib, since 5.1)
> >>  #
> >> +# @zone: The zone device model modes.  The default is that the device is
> >> +# not zoned.  (since 8.2)
> >> +#
> >>  # Since: 2.12
> >>  ##
> >>  { 'struct': 'BlockdevCreateOptionsQcow2',
> >> @@ -5096,7 +5160,8 @@
> >>  '*preallocation':   'PreallocMode',
> >>  '*lazy-refcounts':  'bool',
> >>  '*refcount-bits':   'int',
> >> -'*compression-type':'Qcow2CompressionType' } }
> >> +'*compression-type':'Qcow2CompressionType',
> >> +'*zone':'Qcow2ZoneCreateOptions' } }
> >>
> >>  ##
> >>  # @BlockdevCreateOptionsQed:
>

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster]

One more thing...

Markus Armbruster  writes:

> I apologize for the delayed review.
>
> Sam Li  writes:
>
>> To configure the zoned format feature on the qcow2 driver, it
>> requires settings as: the device size, zone model, zone size,
>> zone capacity, number of conventional zones, limits on zone
>> resources (max append bytes, max open zones, and max_active_zones).
>>
>> To create a qcow2 image with zoned format feature, use command like
>> this:
>> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
>> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
>> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
>> -o zone.max_active_zones=8 -o zone.mode=host-managed
>>
>> Signed-off-by: Sam Li 
>
> [...]
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ca390c5700..e2e0ec21a5 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -5038,6 +5038,67 @@
>>  { 'enum': 'Qcow2CompressionType',
>>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>>  
>> +##
>> +# @Qcow2ZoneModel:
>> +#
>> +# Zoned device model used in qcow2 image file
>> +#
>> +# @host-managed: The host-managed model only allows sequential write over 
>> the
>> +# device zones.
>> +#
>> +# Since 8.2
>> +##
>> +{ 'enum': 'Qcow2ZoneModel',
>> +  'data': [ 'host-managed'] }
>> +
>> +##
>> +# @Qcow2ZoneHostManaged:
>> +#
>> +# The host-managed zone model.  It only allows sequential writes.
>> +#
>> +# @size: Total number of bytes within zones.
>
> Default?
>
>> +#
>> +# @capacity: The number of usable logical blocks within zones
>> +# in bytes.  A zone capacity is always smaller or equal to the
>> +# zone size.
>
> Default?
>
>> +#
>> +# @conventional-zones: The number of conventional zones of the
>> +# zoned device (default 0).
>> +#
>> +# @max-open-zones: The maximal number of open zones.  It is less than
>> +# or equal to the number of sequential write required zones of
>> +# the device (default 0).
>> +#
>> +# @max-active-zones: The maximal number of zones in the implicit
>> +# open, explicit open or closed state.  It is less than or equal
>> +# to the max open zones (default 0).
>> +#
>> +# @max-append-bytes: The maximal number of bytes of a zone
>> +# append request that can be issued to the device.  It must be
>> +# 512-byte aligned and less than the zone capacity.
>
> Default?
>
>> +#
>> +# Since 8.2
>> +##
>> +{ 'struct': 'Qcow2ZoneHostManaged',
>> +  'data': { '*size':  'size',
>> +'*capacity':  'size',
>> +'*conventional-zones': 'uint32',
>> +'*max-open-zones': 'uint32',
>> +'*max-active-zones':   'uint32',
>> +'*max-append-bytes':   'size' } }
>> +
>> +##
>> +# @Qcow2ZoneCreateOptions:
>> +#
>> +# The zone device model for the qcow2 image.

Please document member @mode.

Fails to build since merge commit 61e7a0d27c1:

qapi/block-core.json: In union 'Qcow2ZoneCreateOptions':
qapi/block-core.json:5135: member 'mode' lacks documentation

>> +#
>> +# Since 8.2
>> +##
>> +{ 'union': 'Qcow2ZoneCreateOptions',
>> +  'base': { 'mode': 'Qcow2ZoneModel' },
>> +  'discriminator': 'mode',
>> +  'data': { 'host-managed': 'Qcow2ZoneHostManaged' } }
>> +
>>  ##
>>  # @BlockdevCreateOptionsQcow2:
>>  #
>> @@ -5080,6 +5141,9 @@
>>  # @compression-type: The image cluster compression method
>>  # (default: zlib, since 5.1)
>>  #
>> +# @zone: The zone device model modes.  The default is that the device is
>> +# not zoned.  (since 8.2)
>> +#
>>  # Since: 2.12
>>  ##
>>  { 'struct': 'BlockdevCreateOptionsQcow2',
>> @@ -5096,7 +5160,8 @@
>>  '*preallocation':   'PreallocMode',
>>  '*lazy-refcounts':  'bool',
>>  '*refcount-bits':   'int',
>> -'*compression-type':'Qcow2CompressionType' } }
>> +'*compression-type':'Qcow2CompressionType',
>> +'*zone':'Qcow2ZoneCreateOptions' } }
>>  
>>  ##
>>  # @BlockdevCreateOptionsQed:

Re: [PATCH v5 01/11] hw/nvme: Use pcie_sriov_num_vfs()

[Klaus Jensen]

On Feb 18 13:56, Akihiko Odaki wrote:
> nvme_sriov_pre_write_ctrl() used to directly inspect SR-IOV
> configurations to know the number of VFs being disabled due to SR-IOV
> configuration writes, but the logic was flawed and resulted in
> out-of-bound memory access.
> 
> It assumed PCI_SRIOV_NUM_VF always has the number of currently enabled
> VFs, but it actually doesn't in the following cases:
> - PCI_SRIOV_NUM_VF has been set but PCI_SRIOV_CTRL_VFE has never been.
> - PCI_SRIOV_NUM_VF was written after PCI_SRIOV_CTRL_VFE was set.
> - VFs were only partially enabled because of realization failure.
> 
> It is a responsibility of pcie_sriov to interpret SR-IOV configurations
> and pcie_sriov does it correctly, so use pcie_sriov_num_vfs(), which it
> provides, to get the number of enabled VFs before and after SR-IOV
> configuration writes.
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: 11871f53ef8e ("hw/nvme: Add support for the Virtualization Management 
> command")
> Suggested-by: Michael S. Tsirkin 
> Signed-off-by: Akihiko Odaki 

Thanks Akihiko,

I'll pick this up for hw/nvme nvme-next as-is.

Reviewed-by: Klaus Jensen 

> ---
>  hw/nvme/ctrl.c | 26 --
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f026245d1e9e..7a56e7b79b4d 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -8466,36 +8466,26 @@ static void nvme_pci_reset(DeviceState *qdev)
>  nvme_ctrl_reset(n, NVME_RESET_FUNCTION);
>  }
>  
> -static void nvme_sriov_pre_write_ctrl(PCIDevice *dev, uint32_t address,
> -  uint32_t val, int len)
> +static void nvme_sriov_post_write_config(PCIDevice *dev, uint16_t 
> old_num_vfs)
>  {
>  NvmeCtrl *n = NVME(dev);
>  NvmeSecCtrlEntry *sctrl;
> -uint16_t sriov_cap = dev->exp.sriov_cap;
> -uint32_t off = address - sriov_cap;
> -int i, num_vfs;
> +int i;
>  
> -if (!sriov_cap) {
> -return;
> -}
> -
> -if (range_covers_byte(off, len, PCI_SRIOV_CTRL)) {
> -if (!(val & PCI_SRIOV_CTRL_VFE)) {
> -num_vfs = pci_get_word(dev->config + sriov_cap + 
> PCI_SRIOV_NUM_VF);
> -for (i = 0; i < num_vfs; i++) {
> -sctrl = >sec_ctrl_list.sec[i];
> -nvme_virt_set_state(n, le16_to_cpu(sctrl->scid), false);
> -}
> -}
> +for (i = pcie_sriov_num_vfs(dev); i < old_num_vfs; i++) {
> +sctrl = >sec_ctrl_list.sec[i];
> +nvme_virt_set_state(n, le16_to_cpu(sctrl->scid), false);
>  }
>  }
>  
>  static void nvme_pci_write_config(PCIDevice *dev, uint32_t address,
>uint32_t val, int len)
>  {
> -nvme_sriov_pre_write_ctrl(dev, address, val, len);
> +uint16_t old_num_vfs = pcie_sriov_num_vfs(dev);
> +
>  pci_default_write_config(dev, address, val, len);
>  pcie_cap_flr_write_config(dev, address, val, len);
> +nvme_sriov_post_write_config(dev, old_num_vfs);
>  }
>  
>  static const VMStateDescription nvme_vmstate = {
> 
> -- 
> 2.43.1
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.


signature.asc
Description: PGP signature

Re: [PATCH 04/21] hw/tricore/testboard: Use qdev_new() instead of QOM basic API

[Bastian Koppelmann]

On Fri, Feb 16, 2024 at 12:02:55PM +0100, Philippe Mathieu-Daudé wrote:
> Prefer QDev API for QDev objects, avoid the underlying QOM layer.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/tricore/tricore_testdevice.h | 3 ---
>  hw/tricore/tricore_testboard.c  | 4 +---
>  2 files changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Bastian Koppelmann 

Cheers,
Bastian

Re: [PATCH v7 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster]

I apologize for the delayed review.

Sam Li  writes:

> To configure the zoned format feature on the qcow2 driver, it
> requires settings as: the device size, zone model, zone size,
> zone capacity, number of conventional zones, limits on zone
> resources (max append bytes, max open zones, and max_active_zones).
>
> To create a qcow2 image with zoned format feature, use command like
> this:
> qemu-img create -f qcow2 zbc.qcow2 -o size=768M \
> -o zone.size=64M -o zone.capacity=64M -o zone.conventional_zones=0 \
> -o zone.max_append_bytes=4096 -o zone.max_open_zones=6 \
> -o zone.max_active_zones=8 -o zone.mode=host-managed
>
> Signed-off-by: Sam Li 

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ca390c5700..e2e0ec21a5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5038,6 +5038,67 @@
>  { 'enum': 'Qcow2CompressionType',
>'data': [ 'zlib', { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>  
> +##
> +# @Qcow2ZoneModel:
> +#
> +# Zoned device model used in qcow2 image file
> +#
> +# @host-managed: The host-managed model only allows sequential write over the
> +# device zones.
> +#
> +# Since 8.2
> +##
> +{ 'enum': 'Qcow2ZoneModel',
> +  'data': [ 'host-managed'] }
> +
> +##
> +# @Qcow2ZoneHostManaged:
> +#
> +# The host-managed zone model.  It only allows sequential writes.
> +#
> +# @size: Total number of bytes within zones.

Default?

> +#
> +# @capacity: The number of usable logical blocks within zones
> +# in bytes.  A zone capacity is always smaller or equal to the
> +# zone size.

Default?

> +#
> +# @conventional-zones: The number of conventional zones of the
> +# zoned device (default 0).
> +#
> +# @max-open-zones: The maximal number of open zones.  It is less than
> +# or equal to the number of sequential write required zones of
> +# the device (default 0).
> +#
> +# @max-active-zones: The maximal number of zones in the implicit
> +# open, explicit open or closed state.  It is less than or equal
> +# to the max open zones (default 0).
> +#
> +# @max-append-bytes: The maximal number of bytes of a zone
> +# append request that can be issued to the device.  It must be
> +# 512-byte aligned and less than the zone capacity.

Default?

> +#
> +# Since 8.2
> +##
> +{ 'struct': 'Qcow2ZoneHostManaged',
> +  'data': { '*size':  'size',
> +'*capacity':  'size',
> +'*conventional-zones': 'uint32',
> +'*max-open-zones': 'uint32',
> +'*max-active-zones':   'uint32',
> +'*max-append-bytes':   'size' } }
> +
> +##
> +# @Qcow2ZoneCreateOptions:
> +#
> +# The zone device model for the qcow2 image.
> +#
> +# Since 8.2
> +##
> +{ 'union': 'Qcow2ZoneCreateOptions',
> +  'base': { 'mode': 'Qcow2ZoneModel' },
> +  'discriminator': 'mode',
> +  'data': { 'host-managed': 'Qcow2ZoneHostManaged' } }
> +
>  ##
>  # @BlockdevCreateOptionsQcow2:
>  #
> @@ -5080,6 +5141,9 @@
>  # @compression-type: The image cluster compression method
>  # (default: zlib, since 5.1)
>  #
> +# @zone: The zone device model modes.  The default is that the device is
> +# not zoned.  (since 8.2)
> +#
>  # Since: 2.12
>  ##
>  { 'struct': 'BlockdevCreateOptionsQcow2',
> @@ -5096,7 +5160,8 @@
>  '*preallocation':   'PreallocMode',
>  '*lazy-refcounts':  'bool',
>  '*refcount-bits':   'int',
> -'*compression-type':'Qcow2CompressionType' } }
> +'*compression-type':'Qcow2CompressionType',
> +'*zone':'Qcow2ZoneCreateOptions' } }
>  
>  ##
>  # @BlockdevCreateOptionsQed:

Re: [PATCH 5/7] hw/ide: Move IDE DMA related definitions to a separate header ide-dma.h

[BALATON Zoltan]

On Mon, 19 Feb 2024, Thomas Huth wrote:

These definitions are required outside of the hw/ide/ code, too,
so lets's move them from internal.h to a new header called ide-dma.h.

Signed-off-by: Thomas Huth 
---
include/hw/ide/ide-dma.h  | 30 ++
include/hw/ide/internal.h | 27 +--
2 files changed, 31 insertions(+), 26 deletions(-)
create mode 100644 include/hw/ide/ide-dma.h

diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
new file mode 100644
index 00..fb82966bdd
--- /dev/null
+++ b/include/hw/ide/ide-dma.h
@@ -0,0 +1,30 @@
+#ifndef HW_IDE_DMA_H
+#define HW_IDE_DMA_H
+
+typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
+typedef void DMAVoidFunc(const IDEDMA *);
+typedef int DMAIntFunc(const IDEDMA *, bool);
+typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
+typedef void DMAu32Func(const IDEDMA *, uint32_t);
+typedef void DMAStopFunc(const IDEDMA *, bool);
+
+struct IDEDMAOps {
+DMAStartFunc *start_dma;
+DMAVoidFunc *pio_transfer;
+DMAInt32Func *prepare_buf;
+DMAu32Func *commit_buf;
+DMAIntFunc *rw_buf;
+DMAVoidFunc *restart;
+DMAVoidFunc *restart_dma;
+DMAStopFunc *set_inactive;
+DMAVoidFunc *cmd_done;
+DMAVoidFunc *reset;
+};
+
+struct IDEDMA {
+const struct IDEDMAOps *ops;
+QEMUIOVector qiov;
+BlockAIOCB *aiocb;


Doesn't this need to #include something to define QEMUIOVector and 
BlockAIOCB and some of the DMA and IDE types not defined above?


Regards,
BALATON Zoltan


+};
+
+#endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 642bd1a979..d1d3fcd23a 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -9,6 +9,7 @@

#include "hw/ide.h"
#include "hw/ide/ide-bus.h"
+#include "hw/ide/ide-dma.h"

/* debug IDE devices */
#define USE_DMA_CDROM
@@ -316,13 +317,6 @@
#define SMART_DISABLE 0xd9
#define SMART_STATUS  0xda

-typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
-typedef void DMAVoidFunc(const IDEDMA *);
-typedef int DMAIntFunc(const IDEDMA *, bool);
-typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
-typedef void DMAu32Func(const IDEDMA *, uint32_t);
-typedef void DMAStopFunc(const IDEDMA *, bool);
-
extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];

extern const MemoryRegionPortio ide_portio_list[];
@@ -340,25 +334,6 @@ typedef struct IDEBufferedRequest {
bool orphaned;
} IDEBufferedRequest;

-struct IDEDMAOps {
-DMAStartFunc *start_dma;
-DMAVoidFunc *pio_transfer;
-DMAInt32Func *prepare_buf;
-DMAu32Func *commit_buf;
-DMAIntFunc *rw_buf;
-DMAVoidFunc *restart;
-DMAVoidFunc *restart_dma;
-DMAStopFunc *set_inactive;
-DMAVoidFunc *cmd_done;
-DMAVoidFunc *reset;
-};
-
-struct IDEDMA {
-const struct IDEDMAOps *ops;
-QEMUIOVector qiov;
-BlockAIOCB *aiocb;
-};
-
/* These are used for the error_status field of IDEBus */
#define IDE_RETRY_MASK 0xf8
#define IDE_RETRY_DMA  0x08

Re: [PATCH 2/7] hw/ide: Split qdev.c into ide-bus.c and ide-dev.c

[BALATON Zoltan]

On Mon, 19 Feb 2024, Thomas Huth wrote:

qdev.c is a mixture between IDE bus specific functions and IDE device
functions. Let's split it up to make it more obvious which part is
related to bus handling and which part is related to device handling.

Signed-off-by: Thomas Huth 
---
hw/ide/ide-bus.c | 111 +++
hw/ide/{qdev.c => ide-dev.c} |  87 +--
hw/arm/Kconfig   |   2 +
hw/ide/Kconfig   |  30 ++
hw/ide/meson.build   |   3 +-
5 files changed, 134 insertions(+), 99 deletions(-)
create mode 100644 hw/ide/ide-bus.c
rename hw/ide/{qdev.c => ide-dev.c} (78%)

[...]

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 29abe1da29..b372b819a4 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -275,6 +275,8 @@ config SBSA_REF
select USB_XHCI_SYSBUS
select WDT_SBSA
select BOCHS_DISPLAY
+select IDE_BUS
+select IDE_DEV

config SABRELITE
bool
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index b93d6743d5..6dfc5a2129 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -1,51 +1,58 @@
config IDE_CORE
bool

-config IDE_QDEV
+config IDE_BUS
bool
select IDE_CORE


Maybe we can assume if something has an IDE bus it also wants to connect 
IDE devices to it so just select IDE_DEV here and not at every place 
IDE_BUS is selected? Or is there a place that only wants IDE_BUS?


Regards,
BALATON Zoltan


+config IDE_DEV
+bool
+depends on IDE_BUS
+
config IDE_PCI
bool
depends on PCI
-select IDE_QDEV
+select IDE_BUS
+select IDE_DEV

config IDE_ISA
bool
depends on ISA_BUS
-select IDE_QDEV
+select IDE_BUS
+select IDE_DEV

config IDE_PIIX
bool
select IDE_PCI
-select IDE_QDEV

config IDE_CMD646
bool
select IDE_PCI
-select IDE_QDEV

config IDE_MACIO
bool
-select IDE_QDEV
+select IDE_BUS
+select IDE_DEV

config IDE_MMIO
bool
-select IDE_QDEV
+select IDE_BUS
+select IDE_DEV

config IDE_VIA
bool
select IDE_PCI
-select IDE_QDEV

config MICRODRIVE
bool
-select IDE_QDEV
+select IDE_BUS
+select IDE_DEV
depends on PCMCIA

config AHCI
bool
-select IDE_QDEV
+select IDE_BUS
+select IDE_DEV

config AHCI_ICH9
bool
@@ -56,8 +63,7 @@ config AHCI_ICH9
config IDE_SII3112
bool
select IDE_PCI
-select IDE_QDEV

config IDE_CF
bool
-default y if IDE_QDEV
+default y if IDE_BUS
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index d2e5b45c9e..d09705cac0 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -1,15 +1,16 @@
system_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
system_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
+system_ss.add(when: 'CONFIG_IDE_BUS', if_true: files('ide-bus.c'))
system_ss.add(when: 'CONFIG_IDE_CF', if_true: files('cf.c'))
system_ss.add(when: 'CONFIG_IDE_CMD646', if_true: files('cmd646.c'))
system_ss.add(when: 'CONFIG_IDE_CORE', if_true: files('core.c', 'atapi.c'))
+system_ss.add(when: 'CONFIG_IDE_DEV', if_true: files('ide-dev.c'))
system_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c', 'ioport.c'))
system_ss.add(when: 'CONFIG_IDE_MACIO', if_true: files('macio.c'))
system_ss.add(when: 'CONFIG_IDE_MMIO', if_true: files('mmio.c'))
system_ss.add(when: 'CONFIG_IDE_PCI', if_true: files('pci.c'))
system_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c'))
-system_ss.add(when: 'CONFIG_IDE_QDEV', if_true: files('qdev.c'))
system_ss.add(when: 'CONFIG_IDE_SII3112', if_true: files('sii3112.c'))
system_ss.add(when: 'CONFIG_IDE_VIA', if_true: files('via.c'))
system_ss.add(when: 'CONFIG_MICRODRIVE', if_true: files('microdrive.c'))

Re: [PATCH 0/7] hw/ide: Clean up hw/ide/qdev.c and include/hw/ide/internal.h

[Philippe Mathieu-Daudé]

On 19/2/24 11:49, Thomas Huth wrote:

While trying to make it possible to compile-out the CompactFlash IDE device
in downstream distributions (first patch), we noticed that there are more
things in the IDE code that could use a proper clean up:

First, hw/ide/qdev.c is quite a mix between IDE BUS specific functions
and (disk) device specific functions. Thus the second patch splits qdev.c
into two new separate files to make it more obvious which part belongs
to which kind of devices.

The remaining patches unentangle include/hw/ide/internal.h, which is meant
as a header that should only be used internally to the IDE subsystem, but
which is currently exposed to the world since include/hw/ide/pci.h includes
this header, too. Thus we move the definitions that are also required for
non-IDE code to other new header files, so we can finally change pci.h to
stop including internal.h. After these changes, internal.h is only included
by files in hw/ide/ as it should be.

Thomas Huth (7):
   hw/ide: Add the possibility to disable the CompactFlash device in the
 build
   hw/ide: Split qdev.c into ide-bus.c and ide-dev.c
   hw/ide: Move IDE device related definitions to ide-dev.h


Modulo comments in "hw/ide/ide-dev.h", series:
Reviewed-by: Philippe Mathieu-Daudé 


   hw/ide: Move IDE bus related definitions to a new header ide-bus.h
   hw/ide: Move IDE DMA related definitions to a separate header
 ide-dma.h
   hw/ide: Remove the include/hw/ide.h legacy file
   hw/ide: Stop exposing internal.h to non-IDE files

Re: [PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Philippe Mathieu-Daudé]

On 19/2/24 11:49, Thomas Huth wrote:

Let's start to unentangle internal.h by moving public IDE device
related definitions to ide-dev.h.

Signed-off-by: Thomas Huth 
---
  include/hw/ide/ide-dev.h  | 145 +-
  include/hw/ide/internal.h | 145 +-
  hw/ide/ide-dev.c  |   1 +
  3 files changed, 146 insertions(+), 145 deletions(-)

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index 7e9663cda9..de88784a25 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -20,9 +20,152 @@
  #ifndef IDE_DEV_H
  #define IDE_DEV_H
  
+#include "sysemu/dma.h"


Not required.


  #include "hw/qdev-properties.h"
  #include "hw/block/block.h"
-#include "hw/ide/internal.h"
+
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEState IDEState;



+typedef struct IDEDMA IDEDMA;
+typedef struct IDEDMAOps IDEDMAOps;
+typedef struct IDEBus IDEBus;


Looking at next patches, better forward-declare IDEBus and
IDEDMA in "qemu/typedefs.h".

IDEDMAOps and "sysemu/dma.h" belong to "hw/ide/ide-dma.h.

[PATCH v2] xlnx-versal-ospi: disable reentrancy detection for iomem_dac

[Sai Pavan Boddu]

The OSPI DMA reads flash data through the OSPI linear address space (the
iomem_dac region), because of this the reentrancy guard introduced in
commit a2e1753b ("memory: prevent dma-reentracy issues") is disabled for
the memory region.

Signed-off-by: Sai Pavan Boddu 
---
Changes for V2:
Added code comments.

 hw/ssi/xlnx-versal-ospi.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
index c7b95b1f37..c479138ec1 100644
--- a/hw/ssi/xlnx-versal-ospi.c
+++ b/hw/ssi/xlnx-versal-ospi.c
@@ -1772,6 +1772,12 @@ static void xlnx_versal_ospi_init(Object *obj)
 memory_region_init_io(>iomem_dac, obj, _dac_ops, s,
   TYPE_XILINX_VERSAL_OSPI "-dac", 0x2000);
 sysbus_init_mmio(sbd, >iomem_dac);
+/*
+ * The OSPI DMA reads flash data through the OSPI linear address space (the
+ * iomem_dac region), because of this the reentrancy guard needs to be
+ * disabled.
+ */
+s->iomem_dac.disable_reentrancy_guard = true;
 
 sysbus_init_irq(sbd, >irq);
 
-- 
2.25.1

[PATCH 6/7] hw/ide: Remove the include/hw/ide.h legacy file

[Thomas Huth]

There was only one prototype left in this legacy file. Move it to
ide-dev.h to finally get rid of it.

Signed-off-by: Thomas Huth 
---
 include/hw/ide.h  | 9 -
 include/hw/ide/ide-dev.h  | 2 ++
 include/hw/ide/internal.h | 1 -
 3 files changed, 2 insertions(+), 10 deletions(-)
 delete mode 100644 include/hw/ide.h

diff --git a/include/hw/ide.h b/include/hw/ide.h
deleted file mode 100644
index db963bdb77..00
--- a/include/hw/ide.h
+++ /dev/null
@@ -1,9 +0,0 @@
-#ifndef HW_IDE_H
-#define HW_IDE_H
-
-#include "exec/memory.h"
-
-/* ide/core.c */
-void ide_drive_get(DriveInfo **hd, int max_bus);
-
-#endif /* HW_IDE_H */
diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index de88784a25..ad55997442 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -181,4 +181,6 @@ typedef struct IDEDrive {
 
 void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
 
+void ide_drive_get(DriveInfo **hd, int max_bus);
+
 #endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d1d3fcd23a..0fc2013374 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -7,7 +7,6 @@
  * non-internal declarations are in hw/ide.h
  */
 
-#include "hw/ide.h"
 #include "hw/ide/ide-bus.h"
 #include "hw/ide/ide-dma.h"
 
-- 
2.43.2

[PATCH 5/7] hw/ide: Move IDE DMA related definitions to a separate header ide-dma.h

[Thomas Huth]

These definitions are required outside of the hw/ide/ code, too,
so lets's move them from internal.h to a new header called ide-dma.h.

Signed-off-by: Thomas Huth 
---
 include/hw/ide/ide-dma.h  | 30 ++
 include/hw/ide/internal.h | 27 +--
 2 files changed, 31 insertions(+), 26 deletions(-)
 create mode 100644 include/hw/ide/ide-dma.h

diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
new file mode 100644
index 00..fb82966bdd
--- /dev/null
+++ b/include/hw/ide/ide-dma.h
@@ -0,0 +1,30 @@
+#ifndef HW_IDE_DMA_H
+#define HW_IDE_DMA_H
+
+typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
+typedef void DMAVoidFunc(const IDEDMA *);
+typedef int DMAIntFunc(const IDEDMA *, bool);
+typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
+typedef void DMAu32Func(const IDEDMA *, uint32_t);
+typedef void DMAStopFunc(const IDEDMA *, bool);
+
+struct IDEDMAOps {
+DMAStartFunc *start_dma;
+DMAVoidFunc *pio_transfer;
+DMAInt32Func *prepare_buf;
+DMAu32Func *commit_buf;
+DMAIntFunc *rw_buf;
+DMAVoidFunc *restart;
+DMAVoidFunc *restart_dma;
+DMAStopFunc *set_inactive;
+DMAVoidFunc *cmd_done;
+DMAVoidFunc *reset;
+};
+
+struct IDEDMA {
+const struct IDEDMAOps *ops;
+QEMUIOVector qiov;
+BlockAIOCB *aiocb;
+};
+
+#endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 642bd1a979..d1d3fcd23a 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -9,6 +9,7 @@
 
 #include "hw/ide.h"
 #include "hw/ide/ide-bus.h"
+#include "hw/ide/ide-dma.h"
 
 /* debug IDE devices */
 #define USE_DMA_CDROM
@@ -316,13 +317,6 @@
 #define SMART_DISABLE 0xd9
 #define SMART_STATUS  0xda
 
-typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
-typedef void DMAVoidFunc(const IDEDMA *);
-typedef int DMAIntFunc(const IDEDMA *, bool);
-typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
-typedef void DMAu32Func(const IDEDMA *, uint32_t);
-typedef void DMAStopFunc(const IDEDMA *, bool);
-
 extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
 
 extern const MemoryRegionPortio ide_portio_list[];
@@ -340,25 +334,6 @@ typedef struct IDEBufferedRequest {
 bool orphaned;
 } IDEBufferedRequest;
 
-struct IDEDMAOps {
-DMAStartFunc *start_dma;
-DMAVoidFunc *pio_transfer;
-DMAInt32Func *prepare_buf;
-DMAu32Func *commit_buf;
-DMAIntFunc *rw_buf;
-DMAVoidFunc *restart;
-DMAVoidFunc *restart_dma;
-DMAStopFunc *set_inactive;
-DMAVoidFunc *cmd_done;
-DMAVoidFunc *reset;
-};
-
-struct IDEDMA {
-const struct IDEDMAOps *ops;
-QEMUIOVector qiov;
-BlockAIOCB *aiocb;
-};
-
 /* These are used for the error_status field of IDEBus */
 #define IDE_RETRY_MASK 0xf8
 #define IDE_RETRY_DMA  0x08
-- 
2.43.2

[PATCH 7/7] hw/ide: Stop exposing internal.h to non-IDE files

[Thomas Huth]

include/hw/ide/internal.h is currently included by include/hw/ide/pci.h
and thus exposed to a lot of files that are not part of the IDE subsystem.
Stop including internal.h there and use the appropriate new headers
ide-bus.h and ide-dma.h instead.

Signed-off-by: Thomas Huth 
---
 include/hw/ide/pci.h | 3 ++-
 hw/i386/pc.c | 2 +-
 hw/ide/cmd646.c  | 1 +
 hw/ide/pci.c | 1 +
 hw/ide/piix.c| 1 +
 hw/ide/sii3112.c | 1 +
 hw/ide/via.c | 1 +
 7 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index a814a0a7c3..e1e012c387 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -1,7 +1,8 @@
 #ifndef HW_IDE_PCI_H
 #define HW_IDE_PCI_H
 
-#include "hw/ide/internal.h"
+#include "hw/ide/ide-bus.h"
+#include "hw/ide/ide-dma.h"
 #include "hw/pci/pci_device.h"
 #include "qom/object.h"
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 196827531a..22d0c29575 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -31,7 +31,7 @@
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/vmport.h"
 #include "sysemu/cpus.h"
-#include "hw/ide/internal.h"
+#include "hw/ide/ide-bus.h"
 #include "hw/timer/hpet.h"
 #include "hw/loader.h"
 #include "hw/rtc/mc146818rtc.h"
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c0bcfa4414..23d213ff01 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -33,6 +33,7 @@
 #include "sysemu/reset.h"
 
 #include "hw/ide/pci.h"
+#include "hw/ide/internal.h"
 #include "trace.h"
 
 /* CMD646 specific */
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index ca85d8474c..73efeec7f4 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -30,6 +30,7 @@
 #include "sysemu/dma.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "hw/ide/internal.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 4e5e12935f..1773a068c3 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -30,6 +30,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/pci/pci.h"
+#include "hw/ide/internal.h"
 #include "hw/ide/piix.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 63dc4a0494..321b9e46a1 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/ide/internal.h"
 #include "hw/ide/pci.h"
 #include "qemu/module.h"
 #include "trace.h"
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 3f3c484253..cf151e70ec 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -25,6 +25,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/ide/internal.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
-- 
2.43.2

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

[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. Let's also introduce a new header ide-dev.h which can be used to
collect definitions related to IDE devices.

Signed-off-by: Thomas Huth 
---
 include/hw/ide/ide-dev.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 include/hw/ide/ide-dev.h
 create mode 100644 hw/ide/cf.c

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
new file mode 100644
index 00..7e9663cda9
--- /dev/null
+++ b/include/hw/ide/ide-dev.h
@@ -0,0 +1,41 @@
+/*
+ * ide device definitions
+ *
+ * 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 IDE_DEV_H
+#define IDE_DEV_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..2a425cb0f2
--- /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/ide-dev.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..4189313d30 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/ide-dev.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)
 
 /* 

[PATCH 4/7] hw/ide: Move IDE bus related definitions to a new header ide-bus.h

[Thomas Huth]

Let's consolidate the public IDE bus related functions in a separate
header.

Signed-off-by: Thomas Huth 
---
 include/hw/ide/ide-bus.h  | 41 +++
 include/hw/ide/internal.h | 38 +---
 2 files changed, 42 insertions(+), 37 deletions(-)
 create mode 100644 include/hw/ide/ide-bus.h

diff --git a/include/hw/ide/ide-bus.h b/include/hw/ide/ide-bus.h
new file mode 100644
index 00..e0460700ed
--- /dev/null
+++ b/include/hw/ide/ide-bus.h
@@ -0,0 +1,41 @@
+#ifndef HW_IDE_BUS_H
+#define HW_IDE_BUS_H
+
+#include "exec/ioport.h"
+#include "hw/ide/ide-dev.h"
+
+struct IDEBus {
+BusState qbus;
+IDEDevice *master;
+IDEDevice *slave;
+IDEState ifs[2];
+QEMUBH *bh;
+
+int bus_id;
+int max_units;
+IDEDMA *dma;
+uint8_t unit;
+uint8_t cmd;
+qemu_irq irq; /* bus output */
+
+int error_status;
+uint8_t retry_unit;
+int64_t retry_sector_num;
+uint32_t retry_nsector;
+PortioList portio_list;
+PortioList portio2_list;
+VMChangeStateEntry *vmstate;
+};
+
+#define TYPE_IDE_BUS "IDE"
+OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
+
+void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
+  int bus_id, int max_units);
+IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
+
+int ide_get_geometry(BusState *bus, int unit,
+ int16_t *cyls, int8_t *heads, int8_t *secs);
+int ide_get_bios_chs_trans(BusState *bus, int unit);
+
+#endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 5cc109fe82..642bd1a979 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -8,16 +8,12 @@
  */
 
 #include "hw/ide.h"
-#include "exec/ioport.h"
-#include "hw/ide/ide-dev.h"
+#include "hw/ide/ide-bus.h"
 
 /* debug IDE devices */
 #define USE_DMA_CDROM
 #include "qom/object.h"
 
-#define TYPE_IDE_BUS "IDE"
-OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
-
 /* Device/Head ("select") Register */
 #define ATA_DEV_SELECT  0x10
 /* ATA1,3: Defined as '1'.
@@ -363,29 +359,6 @@ struct IDEDMA {
 BlockAIOCB *aiocb;
 };
 
-struct IDEBus {
-BusState qbus;
-IDEDevice *master;
-IDEDevice *slave;
-IDEState ifs[2];
-QEMUBH *bh;
-
-int bus_id;
-int max_units;
-IDEDMA *dma;
-uint8_t unit;
-uint8_t cmd;
-qemu_irq irq; /* bus output */
-
-int error_status;
-uint8_t retry_unit;
-int64_t retry_sector_num;
-uint32_t retry_nsector;
-PortioList portio_list;
-PortioList portio2_list;
-VMChangeStateEntry *vmstate;
-};
-
 /* These are used for the error_status field of IDEBus */
 #define IDE_RETRY_MASK 0xf8
 #define IDE_RETRY_DMA  0x08
@@ -502,15 +475,6 @@ void ide_cancel_dma_sync(IDEState *s);
 void ide_atapi_cmd(IDEState *s);
 void ide_atapi_cmd_reply_end(IDEState *s);
 
-/* hw/ide/qdev.c */
-void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
-  int bus_id, int max_units);
-IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
-
-int ide_get_geometry(BusState *bus, int unit,
- int16_t *cyls, int8_t *heads, int8_t *secs);
-int ide_get_bios_chs_trans(BusState *bus, int unit);
-
 int ide_handle_rw_error(IDEState *s, int error, int op);
 
 #endif /* HW_IDE_INTERNAL_H */
-- 
2.43.2

[PATCH 2/7] hw/ide: Split qdev.c into ide-bus.c and ide-dev.c

[Thomas Huth]

qdev.c is a mixture between IDE bus specific functions and IDE device
functions. Let's split it up to make it more obvious which part is
related to bus handling and which part is related to device handling.

Signed-off-by: Thomas Huth 
---
 hw/ide/ide-bus.c | 111 +++
 hw/ide/{qdev.c => ide-dev.c} |  87 +--
 hw/arm/Kconfig   |   2 +
 hw/ide/Kconfig   |  30 ++
 hw/ide/meson.build   |   3 +-
 5 files changed, 134 insertions(+), 99 deletions(-)
 create mode 100644 hw/ide/ide-bus.c
 rename hw/ide/{qdev.c => ide-dev.c} (78%)

diff --git a/hw/ide/ide-bus.c b/hw/ide/ide-bus.c
new file mode 100644
index 00..57fe67b29c
--- /dev/null
+++ b/hw/ide/ide-bus.c
@@ -0,0 +1,111 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann 
+ *
+ * This library 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 "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/module.h"
+#include "hw/ide/internal.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/runstate.h"
+
+static char *idebus_get_fw_dev_path(DeviceState *dev);
+static void idebus_unrealize(BusState *qdev);
+
+static void ide_bus_class_init(ObjectClass *klass, void *data)
+{
+BusClass *k = BUS_CLASS(klass);
+
+k->get_fw_dev_path = idebus_get_fw_dev_path;
+k->unrealize = idebus_unrealize;
+}
+
+static void idebus_unrealize(BusState *bus)
+{
+IDEBus *ibus = IDE_BUS(bus);
+
+if (ibus->vmstate) {
+qemu_del_vm_change_state_handler(ibus->vmstate);
+}
+}
+
+static const TypeInfo ide_bus_info = {
+.name = TYPE_IDE_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(IDEBus),
+.class_init = ide_bus_class_init,
+};
+
+void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
+ int bus_id, int max_units)
+{
+qbus_init(idebus, idebus_size, TYPE_IDE_BUS, dev, NULL);
+idebus->bus_id = bus_id;
+idebus->max_units = max_units;
+}
+
+static char *idebus_get_fw_dev_path(DeviceState *dev)
+{
+char path[30];
+
+snprintf(path, sizeof(path), "%s@%x", qdev_fw_name(dev),
+ ((IDEBus *)dev->parent_bus)->bus_id);
+
+return g_strdup(path);
+}
+
+IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
+{
+DeviceState *dev;
+
+dev = qdev_new(drive->media_cd ? "ide-cd" : "ide-hd");
+qdev_prop_set_uint32(dev, "unit", unit);
+qdev_prop_set_drive_err(dev, "drive", blk_by_legacy_dinfo(drive),
+_fatal);
+qdev_realize_and_unref(dev, >qbus, _fatal);
+return DO_UPCAST(IDEDevice, qdev, dev);
+}
+
+int ide_get_geometry(BusState *bus, int unit,
+ int16_t *cyls, int8_t *heads, int8_t *secs)
+{
+IDEState *s = _UPCAST(IDEBus, qbus, bus)->ifs[unit];
+
+if (s->drive_kind != IDE_HD || !s->blk) {
+return -1;
+}
+
+*cyls = s->cylinders;
+*heads = s->heads;
+*secs = s->sectors;
+return 0;
+}
+
+int ide_get_bios_chs_trans(BusState *bus, int unit)
+{
+return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
+}
+
+static void ide_bus_register_type(void)
+{
+type_register_static(_bus_info);
+}
+
+type_init(ide_bus_register_type)
diff --git a/hw/ide/qdev.c b/hw/ide/ide-dev.c
similarity index 78%
rename from hw/ide/qdev.c
rename to hw/ide/ide-dev.c
index 4189313d30..15d088fd06 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/ide-dev.c
@@ -1,5 +1,5 @@
 /*
- * ide bus support for qdev.
+ * IDE device functions
  *
  * Copyright (c) 2009 Gerd Hoffmann 
  *
@@ -18,71 +18,21 @@
  */
 
 #include "qemu/osdep.h"
-#include "sysemu/dma.h"
 #include "qapi/error.h"
 #include "qapi/qapi-types-block.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "hw/ide/ide-dev.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
-#include "sysemu/runstate.h"
 #include "qapi/visitor.h"
 
-/* - */
-
-static char *idebus_get_fw_dev_path(DeviceState *dev);
-static void idebus_unrealize(BusState *qdev);
-
 static Property ide_props[] = {
 DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-static void ide_bus_class_init(ObjectClass *klass, void 

[PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Thomas Huth]

Let's start to unentangle internal.h by moving public IDE device
related definitions to ide-dev.h.

Signed-off-by: Thomas Huth 
---
 include/hw/ide/ide-dev.h  | 145 +-
 include/hw/ide/internal.h | 145 +-
 hw/ide/ide-dev.c  |   1 +
 3 files changed, 146 insertions(+), 145 deletions(-)

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index 7e9663cda9..de88784a25 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -20,9 +20,152 @@
 #ifndef IDE_DEV_H
 #define IDE_DEV_H
 
+#include "sysemu/dma.h"
 #include "hw/qdev-properties.h"
 #include "hw/block/block.h"
-#include "hw/ide/internal.h"
+
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEState IDEState;
+typedef struct IDEDMA IDEDMA;
+typedef struct IDEDMAOps IDEDMAOps;
+typedef struct IDEBus IDEBus;
+
+typedef void EndTransferFunc(IDEState *);
+
+#define MAX_IDE_DEVS 2
+
+#define TYPE_IDE_DEVICE "ide-device"
+OBJECT_DECLARE_TYPE(IDEDevice, IDEDeviceClass, IDE_DEVICE)
+
+typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
+
+struct unreported_events {
+bool eject_request;
+bool new_media;
+};
+
+enum ide_dma_cmd {
+IDE_DMA_READ = 0,
+IDE_DMA_WRITE,
+IDE_DMA_TRIM,
+IDE_DMA_ATAPI,
+IDE_DMA__COUNT
+};
+
+/* NOTE: IDEState represents in fact one drive */
+struct IDEState {
+IDEBus *bus;
+uint8_t unit;
+/* ide config */
+IDEDriveKind drive_kind;
+int drive_heads, drive_sectors;
+int cylinders, heads, sectors, chs_trans;
+int64_t nb_sectors;
+int mult_sectors;
+int identify_set;
+uint8_t identify_data[512];
+int drive_serial;
+char drive_serial_str[21];
+char drive_model_str[41];
+uint64_t wwn;
+/* ide regs */
+uint8_t feature;
+uint8_t error;
+uint32_t nsector;
+uint8_t sector;
+uint8_t lcyl;
+uint8_t hcyl;
+/* other part of tf for lba48 support */
+uint8_t hob_feature;
+uint8_t hob_nsector;
+uint8_t hob_sector;
+uint8_t hob_lcyl;
+uint8_t hob_hcyl;
+
+uint8_t select;
+uint8_t status;
+
+bool io8;
+bool reset_reverts;
+
+/* set for lba48 access */
+uint8_t lba48;
+BlockBackend *blk;
+char version[9];
+/* ATAPI specific */
+struct unreported_events events;
+uint8_t sense_key;
+uint8_t asc;
+bool tray_open;
+bool tray_locked;
+uint8_t cdrom_changed;
+int packet_transfer_size;
+int elementary_transfer_size;
+int32_t io_buffer_index;
+int lba;
+int cd_sector_size;
+int atapi_dma; /* true if dma is requested for the packet cmd */
+BlockAcctCookie acct;
+BlockAIOCB *pio_aiocb;
+QEMUIOVector qiov;
+QLIST_HEAD(, IDEBufferedRequest) buffered_requests;
+/* ATA DMA state */
+uint64_t io_buffer_offset;
+int32_t io_buffer_size;
+QEMUSGList sg;
+/* PIO transfer handling */
+int req_nb_sectors; /* number of sectors per interrupt */
+EndTransferFunc *end_transfer_func;
+uint8_t *data_ptr;
+uint8_t *data_end;
+uint8_t *io_buffer;
+/* PIO save/restore */
+int32_t io_buffer_total_len;
+int32_t cur_io_buffer_offset;
+int32_t cur_io_buffer_len;
+uint8_t end_transfer_fn_idx;
+QEMUTimer *sector_write_timer; /* only used for win2k install hack */
+uint32_t irq_count; /* counts IRQs when using win2k install hack */
+/* CF-ATA extended error */
+uint8_t ext_error;
+/* CF-ATA metadata storage */
+uint32_t mdata_size;
+uint8_t *mdata_storage;
+int media_changed;
+enum ide_dma_cmd dma_cmd;
+/* SMART */
+uint8_t smart_enabled;
+uint8_t smart_autosave;
+int smart_errors;
+uint8_t smart_selftest_count;
+uint8_t *smart_selftest_data;
+/* AHCI */
+int ncq_queues;
+};
+
+struct IDEDeviceClass {
+DeviceClass parent_class;
+void (*realize)(IDEDevice *dev, Error **errp);
+};
+
+struct IDEDevice {
+DeviceState qdev;
+uint32_t unit;
+BlockConf conf;
+int chs_trans;
+char *version;
+char *serial;
+char *model;
+uint64_t wwn;
+/*
+ * 0x- rotation rate not reported
+ * 0x0001- non-rotating medium (SSD)
+ * 0x0002-0x0400 - reserved
+ * 0x0401-0xffe  - rotations per minute
+ * 0x- reserved
+ */
+uint16_t rotation_rate;
+};
 
 typedef struct IDEDrive {
 IDEDevice dev;
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 3bdcc75597..5cc109fe82 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -8,24 +8,16 @@
  */
 
 #include "hw/ide.h"
-#include "sysemu/dma.h"
-#include "hw/block/block.h"
 #include "exec/ioport.h"
+#include "hw/ide/ide-dev.h"
 
 /* debug IDE devices */
 #define USE_DMA_CDROM
 #include "qom/object.h"
 
-typedef struct IDEDevice IDEDevice;
-typedef struct IDEState IDEState;
-typedef struct IDEDMA IDEDMA;
-typedef struct IDEDMAOps IDEDMAOps;
-
 #define TYPE_IDE_BUS 

[PATCH 0/7] hw/ide: Clean up hw/ide/qdev.c and include/hw/ide/internal.h

[Thomas Huth]

While trying to make it possible to compile-out the CompactFlash IDE device
in downstream distributions (first patch), we noticed that there are more
things in the IDE code that could use a proper clean up:

First, hw/ide/qdev.c is quite a mix between IDE BUS specific functions
and (disk) device specific functions. Thus the second patch splits qdev.c
into two new separate files to make it more obvious which part belongs
to which kind of devices.

The remaining patches unentangle include/hw/ide/internal.h, which is meant
as a header that should only be used internally to the IDE subsystem, but
which is currently exposed to the world since include/hw/ide/pci.h includes
this header, too. Thus we move the definitions that are also required for
non-IDE code to other new header files, so we can finally change pci.h to
stop including internal.h. After these changes, internal.h is only included
by files in hw/ide/ as it should be.

Thomas Huth (7):
  hw/ide: Add the possibility to disable the CompactFlash device in the
build
  hw/ide: Split qdev.c into ide-bus.c and ide-dev.c
  hw/ide: Move IDE device related definitions to ide-dev.h
  hw/ide: Move IDE bus related definitions to a new header ide-bus.h
  hw/ide: Move IDE DMA related definitions to a separate header
ide-dma.h
  hw/ide: Remove the include/hw/ide.h legacy file
  hw/ide: Stop exposing internal.h to non-IDE files

 include/hw/ide.h |   9 --
 include/hw/ide/ide-bus.h |  41 +++
 include/hw/ide/ide-dev.h | 186 +++
 include/hw/ide/ide-dma.h |  30 +
 include/hw/ide/internal.h| 209 +--
 include/hw/ide/pci.h |   3 +-
 hw/i386/pc.c |   2 +-
 hw/ide/cf.c  |  58 ++
 hw/ide/cmd646.c  |   1 +
 hw/ide/ide-bus.c | 111 +++
 hw/ide/{qdev.c => ide-dev.c} | 137 +--
 hw/ide/pci.c |   1 +
 hw/ide/piix.c|   1 +
 hw/ide/sii3112.c |   1 +
 hw/ide/via.c |   1 +
 hw/arm/Kconfig   |   2 +
 hw/ide/Kconfig   |  32 --
 hw/ide/meson.build   |   4 +-
 18 files changed, 465 insertions(+), 364 deletions(-)
 delete mode 100644 include/hw/ide.h
 create mode 100644 include/hw/ide/ide-bus.h
 create mode 100644 include/hw/ide/ide-dev.h
 create mode 100644 include/hw/ide/ide-dma.h
 create mode 100644 hw/ide/cf.c
 create mode 100644 hw/ide/ide-bus.c
 rename hw/ide/{qdev.c => ide-dev.c} (67%)

-- 
2.43.2

Re: [PATCH v5 02/11] pcie_sriov: Validate NumVFs

[Akihiko Odaki]

On 2024/02/19 2:36, Michael S. Tsirkin wrote:

On Sun, Feb 18, 2024 at 01:56:07PM +0900, Akihiko Odaki wrote:

The guest may write NumVFs greater than TotalVFs and that can lead
to buffer overflow in VF implementations.

Cc: qemu-sta...@nongnu.org
Fixes: 7c0fa8dff811 ("pcie: Add support for Single Root I/O Virtualization 
(SR/IOV)")
Signed-off-by: Akihiko Odaki 
---
  hw/pci/pcie_sriov.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index a1fe65f5d801..da209b7f47fd 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -176,6 +176,9 @@ static void register_vfs(PCIDevice *dev)
  
  assert(sriov_cap > 0);

  num_vfs = pci_get_word(dev->config + sriov_cap + PCI_SRIOV_NUM_VF);
+if (num_vfs > pci_get_word(dev->config + sriov_cap + PCI_SRIOV_TOTAL_VF)) {
+return;
+}
  
  dev->exp.sriov_pf.vf = g_new(PCIDevice *, num_vfs);



This reminds me: how is this num_vfs value set on migration?


That's a good point... Actually no consideration of migration is made 
and SR-IOV is completely broken with it.