Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

[BALATON Zoltan]

On Wed, 26 Apr 2023, Bernhard Beschow wrote:

Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  -extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  -const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  -const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
/* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >mmio, 0xc0, 8);
-pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
+

Re: [PATCH v4 3/5] parallels: Add checking and repairing duplicate offsets in BAT

[Mike Maslenkin]

On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov
 wrote:
>
> Cluster offsets must be unique among all the BAT entries. Find duplicate
> offsets in the BAT and fix it by copying the content of the relevant
> cluster to a newly allocated cluster and set the new cluster offset to the
> duplicated entry.
>
> Add host_cluster_index() helper to deduplicate the code.
>
> Move parallels_fix_leak() call to parallels_co_check() to fix both types
> of leak: real corruption and a leak produced by allocate_clusters()
> during deduplication.
>
> Signed-off-by: Alexander Ivanov 
> ---
>  block/parallels.c | 134 --
>  1 file changed, 129 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index ec89ed894b..3b992e8173 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, 
> int64_t sector_num,
>  return MIN(nb_sectors, ret);
>  }
>
> +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> +{
> +off -= s->header->data_off << BDRV_SECTOR_BITS;
> +return off / s->cluster_size;
> +}
> +

I guess  there should be:
off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS

Regards,
Mike.

[PATCH] aio-posix: zero out io_uring sqe user_data

[Stefan Hajnoczi]

liburing does not clear sqe->user_data. We must do it ourselves to avoid
undefined behavior in process_cqe() when user_data is used.

Note that fdmon-io_uring is currently disabled, so this is a latent bug
that does not affect users. Let's merge this fix now to make it easier
to enable fdmon-io_uring in the future (and I'm working on that).

Signed-off-by: Stefan Hajnoczi 
---
 util/fdmon-io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index ab43052dd7..35165bcb46 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -184,6 +184,7 @@ static void add_poll_remove_sqe(AioContext *ctx, AioHandler 
*node)
 #else
 io_uring_prep_poll_remove(sqe, node);
 #endif
+io_uring_sqe_set_data(sqe, NULL);
 }
 
 /* Add a timeout that self-cancels when another cqe becomes ready */
@@ -197,6 +198,7 @@ static void add_timeout_sqe(AioContext *ctx, int64_t ns)
 
 sqe = get_sqe(ctx);
 io_uring_prep_timeout(sqe, , 1, 0);
+io_uring_sqe_set_data(sqe, NULL);
 }
 
 /* Add sqes from ctx->submit_list for submission */
-- 
2.40.0

Re: [PATCH 12/13] hw/ide/sii3112: Reuse PCIIDEState::bmdma_ops

[Bernhard Beschow]

Am 26. April 2023 11:44:29 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Allows to unexport bmdma_addr_ioport_ops and models TYPE_SII3112_PCI as a
>> standard-compliant PCI IDE device.
>
>Nice! I think it's worth adding a brief mention that you've added BMDMA 
>trace-events, but otherwise looks sensible to me.

Will do. I'd also split this patch into two.

Best regards,
Bernhard

>
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  1 -
>>   hw/ide/pci.c |  2 +-
>>   hw/ide/sii3112.c | 94 ++--
>>   hw/ide/trace-events  |  6 ++-
>>   4 files changed, 60 insertions(+), 43 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index dbb4b13161..81e0370202 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -59,7 +59,6 @@ struct PCIIDEState {
>>   void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
>>   void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
>>   void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>> -extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>> #endif
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 97ccc75aa6..3539b162b7 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -342,7 +342,7 @@ static void bmdma_addr_write(void *opaque, hwaddr addr,
>>   bm->addr |= ((data & mask) << shift) & ~3;
>>   }
>>   -MemoryRegionOps bmdma_addr_ioport_ops = {
>> +static MemoryRegionOps bmdma_addr_ioport_ops = {
>>   .read = bmdma_addr_read,
>>   .write = bmdma_addr_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 9cf920369f..373c0dd1ee 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -34,47 +34,73 @@ struct SiI3112PCIState {
>>   SiI3112Regs regs[2];
>>   };
>>   -/* The sii3112_reg_read and sii3112_reg_write functions implement the
>> - * Internal Register Space - BAR5 (section 6.7 of the data sheet).
>> - */
>> -
>> -static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
>> -unsigned int size)
>> +static uint64_t sii3112_bmdma_read(void *opaque, hwaddr addr, unsigned int 
>> size)
>>   {
>> -SiI3112PCIState *d = opaque;
>> +BMDMAState *bm = opaque;
>> +SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
>> +int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
>>   uint64_t val;
>> switch (addr) {
>>   case 0x00:
>> -val = d->i.bmdma[0].cmd;
>> +val = bm->cmd;
>>   break;
>>   case 0x01:
>> -val = d->regs[0].swdata;
>> +val = d->regs[i].swdata;
>>   break;
>>   case 0x02:
>> -val = d->i.bmdma[0].status;
>> +val = bm->status;
>>   break;
>>   case 0x03:
>>   val = 0;
>>   break;
>> -case 0x04 ... 0x07:
>> -val = bmdma_addr_ioport_ops.read(>i.bmdma[0], addr - 4, size);
>> -break;
>> -case 0x08:
>> -val = d->i.bmdma[1].cmd;
>> +default:
>> +val = 0;
>>   break;
>> -case 0x09:
>> -val = d->regs[1].swdata;
>> +}
>> +trace_sii3112_bmdma_read(size, addr, val);
>> +return val;
>> +}
>> +
>> +static void sii3112_bmdma_write(void *opaque, hwaddr addr,
>> +uint64_t val, unsigned int size)
>> +{
>> +BMDMAState *bm = opaque;
>> +SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
>> +int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
>> +
>> +trace_sii3112_bmdma_write(size, addr, val);
>> +switch (addr) {
>> +case 0x00:
>> +bmdma_cmd_writeb(bm, val);
>>   break;
>> -case 0x0a:
>> -val = d->i.bmdma[1].status;
>> +case 0x01:
>> +d->regs[i].swdata = val & 0x3f;
>>   break;
>> -case 0x0b:
>> -val = 0;
>> +case 0x02:
>> +bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 
>> 6);
>>   break;
>> -case 0x0c ... 0x0f:
>> -val = bmdma_addr_ioport_ops.read(>i.bmdma[1], addr - 12, size);
>> +default:
>>   break;
>> +}
>> +}
>> +
>> +static const MemoryRegionOps sii3112_bmdma_ops = {
>> +.read = sii3112_bmdma_read,
>> +.write = sii3112_bmdma_write,
>> +};
>> +
>> +/* The sii3112_reg_read and sii3112_reg_write functions implement the
>> + * Internal Register Space - BAR5 (section 6.7 of the data sheet).
>> + */
>> +
>> +static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
>> +unsigned int size)
>> +{
>> +SiI3112PCIState *d = opaque;
>> +uint64_t val;
>> +
>> +switch (addr) {
>>   case 0x10:
>>   val = d->i.bmdma[0].cmd;
>>   val |= (d->regs[0].confstat & (1UL << 11) ? (1 << 4) : 0); 
>> /*SATAINT0*/
>> @@ -127,38 +153,26 @@ static void sii3112_reg_write(void *opaque, hwaddr 
>> addr,
>> trace_sii3112_write(size, 

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
>> standard-compliant PCI IDE device.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  2 --
>>   hw/ide/pci.c |  4 ++--
>>   hw/ide/sii3112.c | 50 
>>   3 files changed, 20 insertions(+), 36 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 5025df5b82..dbb4b13161 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>>   extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>>   -extern const MemoryRegionOps pci_ide_cmd_le_ops;
>> -extern const MemoryRegionOps pci_ide_data_le_ops;
>>   #endif
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index b2fcc00a64..97ccc75aa6 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
>>   ide_ctrl_write(bus, addr + 2, data);
>>   }
>>   -const MemoryRegionOps pci_ide_cmd_le_ops = {
>> +static const MemoryRegionOps pci_ide_cmd_le_ops = {
>>   .read = pci_ide_status_read,
>>   .write = pci_ide_ctrl_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
>>   }
>>   }
>>   -const MemoryRegionOps pci_ide_data_le_ops = {
>> +static const MemoryRegionOps pci_ide_data_le_ops = {
>>   .read = pci_ide_data_read,
>>   .write = pci_ide_data_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 0af897a9ef..9cf920369f 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr 
>> addr,
>>   val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
>>   val |= (uint32_t)d->i.bmdma[1].status << 16;
>>   break;
>> -case 0x80 ... 0x87:
>> -val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
>> -break;
>> -case 0x8a:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
>> -break;
>>   case 0xa0:
>>   val = d->regs[0].confstat;
>>   break;
>> -case 0xc0 ... 0xc7:
>> -val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
>> -break;
>> -case 0xca:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
>> -break;
>>   case 0xe0:
>>   val = d->regs[1].confstat;
>>   break;
>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>>   case 0x0c ... 0x0f:
>>   bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
>>   break;
>> -case 0x80 ... 0x87:
>> -pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
>> -break;
>> -case 0x8a:
>> -pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
>> -break;
>> -case 0xc0 ... 0xc7:
>> -pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
>> -break;
>> -case 0xca:
>> -pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
>> -break;
>>   case 0x100:
>>   d->regs[0].scontrol = val & 0xfff;
>>   if (val & 1) {
>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>   pci_config_set_interrupt_pin(dev->config, 1);
>>   pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
>>   +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
>> +pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
>> +pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
>> +pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
>> +
>>   /* BAR5 is in PCI memory space */
>>   memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
>>"sii3112.bar5", 0x200);
>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
>>   mr = g_new(MemoryRegion, 1);
>> -memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 
>> 8);
>> -pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
>> +memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", 
>> >data_ops[0], 0,
>> + memory_region_size(>data_ops[0]));
>> +memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
>>   mr = g_new(MemoryRegion, 1);
>> -memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 
>> 4);
>> -pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
>> +memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 
>> 0,
>> +   

Re: [PATCH 10/13] hw/ide/piix: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

Am 26. April 2023 18:18:35 UTC schrieb Bernhard Beschow :
>
>
>Am 26. April 2023 11:37:48 UTC schrieb Mark Cave-Ayland 
>:
>>On 22/04/2023 16:07, Bernhard Beschow wrote:
>>
>>> Now that PCIIDEState::{cmd,data}_ops are initialized in the base class
>>> constructor there is an opportunity for PIIX to reuse these attributes. This
>>> resolves usage of ide_init_ioport() which would fall back internally to 
>>> using
>>> the isabus global due to NULL being passed as ISADevice by PIIX.
>>> 
>>> Signed-off-by: Bernhard Beschow 
>>> ---
>>>   hw/ide/piix.c | 30 +-
>>>   1 file changed, 13 insertions(+), 17 deletions(-)
>>> 
>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>> index a3a15dc7db..406a67fa0f 100644
>>> --- a/hw/ide/piix.c
>>> +++ b/hw/ide/piix.c
>>> @@ -104,34 +104,32 @@ static void piix_ide_reset(DeviceState *dev)
>>>   pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>>   }
>>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus 
>>> *isa_bus,
>>> -  Error **errp)
>>> +static void pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus)
>>>   {
>>>   static const struct {
>>>   int iobase;
>>>   int iobase2;
>>>   int isairq;
>>>   } port_info[] = {
>>> -{0x1f0, 0x3f6, 14},
>>> -{0x170, 0x376, 15},
>>> +{0x1f0, 0x3f4, 14},
>>> +{0x170, 0x374, 15},
>>>   };
>>> -int ret;
>>> +MemoryRegion *address_space_io = pci_address_space_io(PCI_DEVICE(d));
>>> ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>> -ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
>>> -  port_info[i].iobase2);
>>> -if (ret) {
>>> -error_setg_errno(errp, -ret, "Failed to realize %s port %u",
>>> - object_get_typename(OBJECT(d)), i);
>>> -return false;
>>> -}
>>> +memory_region_add_subregion(address_space_io, port_info[i].iobase,
>>> +>data_ops[i]);
>>> +/*
>>> + * PIIX forwards the last byte of cmd_ops to ISA. Model this using a 
>>> low
>>> + * prio so competing memory regions take precedence.
>>> + */
>>> +memory_region_add_subregion_overlap(address_space_io, 
>>> port_info[i].iobase2,
>>> +>cmd_ops[i], -1);
>>
>>Interesting. Is this behaviour documented somewhere and/or used in one of 
>>your test images at all? If I'd have seen this myself, I probably thought 
>>that the addresses were a typo...
>
>I first  stumbled upon this and wondered why this code was working with 
>VIA_IDE (through my pc-via branch). Then I found the correct offsets there 
>which are confirmed in the piix datasheet, e.g.: "Secondary Control Block 
>Offset: 0374h"

In case you were wondering about the forwarding of the last byte the datasheet 
says: "Accesses to byte 3 of the Control Block are forwarded to ISA where the 
floppy disk controller responds."

>
>>
>>>   ide_bus_init_output_irq(>bus[i],
>>>   isa_bus_get_irq(isa_bus, 
>>> port_info[i].isairq));
>>> bmdma_init(>bus[i], >bmdma[i], d);
>>>   ide_bus_register_restart_cb(>bus[i]);
>>> -
>>> -return true;
>>>   }
>>> static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>>> @@ -160,9 +158,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>>> **errp)
>>>   }
>>> for (unsigned i = 0; i < 2; i++) {
>>> -if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>>> -return;
>>> -}
>>> +pci_piix_init_bus(d, i, isa_bus);
>>>   }
>>>   }
>>>   
>>
>>
>>ATB,
>>
>>Mark.

Re: [PATCH v3 09/13] migration: Create migrate_tls_creds() function

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> On 24.04.23 21:32, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela 
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

> could be stricter "const char *"

I change changed the patch just to reflect this.

>
>> @@ -34,20 +34,19 @@ migration_tls_get_creds(MigrationState *s,
>>   Error **errp)
>
> "s" argument becomes unused, may be dropped.

Good catch!

I created the patches for this, will send after I send the PULL request.

Thanks, Juan.

Re: [PATCH 01/13] hw/ide/pci: Expose legacy interrupts as GPIOs

[Bernhard Beschow]

Am 26. April 2023 10:41:30 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Exposing the legacy IDE interrupts as GPIOs allows them to be connected in 
>> the
>> parent device through qdev_connect_gpio_out(), i.e. without accessing private
>> data of TYPE_PCI_IDE.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   hw/ide/pci.c | 8 
>>   1 file changed, 8 insertions(+)
>> 
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index fc9224bbc9..942e216b9b 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -522,10 +522,18 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, 
>> PCIIDEState *d)
>>   bm->pci_dev = d;
>>   }
>>   +static void pci_ide_init(Object *obj)
>> +{
>> +PCIIDEState *d = PCI_IDE(obj);
>> +
>> +qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
>
>Just one minor nit: can we make this qdev_init_gpio_out_named() and call it 
>"isa-irq" to match? This is for 2 reasons: firstly these are PCI devices and 
>so an unnamed IRQ/gpio could be considered to belong to PCI, and secondly it 
>gives the gpio the same name as the struct field.

Yes, makes sense.

>
>From my previous email I think this should supercede Phil's patch at 
>https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-2-phi...@linaro.org/.
>
>> +}
>> +
>>   static const TypeInfo pci_ide_type_info = {
>>   .name = TYPE_PCI_IDE,
>>   .parent = TYPE_PCI_DEVICE,
>>   .instance_size = sizeof(PCIIDEState),
>> +.instance_init = pci_ide_init,
>>   .abstract = true,
>>   .interfaces = (InterfaceInfo[]) {
>>   { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>
>Otherwise:
>
>Reviewed-by: Mark Cave-Ayland 
>
>
>ATB,
>
>Mark.

Re: [PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug

[Alexander Ivanov]

The patchests are the same, but the first one has incorrect receivers. 
Please just ignore it.
I've sent a email about it, but i mistook twice and sent it to 
qemu-de...@nongnu.org only.



On 4/26/23 17:07, Denis V. Lunev wrote:

On 4/24/23 11:31, Alexander Ivanov wrote:

Fix image inflation when offset in BAT is out of image.

Replace whole BAT syncing by flushing only dirty blocks.

Move all the checks outside the main check function in
separate functions

Use WITH_QEMU_LOCK_GUARD for simplier code.

Fix incorrect condition in out-of-image check.

v11:
1: Use bdrv_nb_sectors() instead of bdrv_getlength() to get image 
size in sectors.

7,9: Add coroutine_fn and GRAPH_RDLOCK annotations.

v10:
8: Add a comment.
9: Exclude unrelated changes.

v9:
3: Add (high_off == 0) case handling.
7: Move res->image_end_offset setting to 
parallels_check_outside_image().

8: Add a patch with a statistics calculation fix.
9: Remove redundant high_off calculation.
12: Change the condition to (off + s->cluster_size > size).

v8: Rebase on the top of the current master branch.

v7:
1,2: Fix string lengths in the commit messages.
3: Fix a typo in the commit message.

v6:
1: Move the error check inside the loop. Move file size getting
    to the function beginning. Skip out-of-image offsets.
2: A new patch - don't let high_off be more than the end of the last 
cluster.

3: Set data_end without any condition.
7: Move data_end setting to parallels_check_outside_image().
8: Remove s->data_end setting from parallels_check_leak().
    Fix 'i' type.

v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().

v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
    in parallels_open() and the fix in parallels_co_check() to two 
patches.

2: A new patch - a part of the patch 1.
    Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.

v3:

1-8: Fix commit message.

v2:

2: A new patch - a part of the splitted patch 2.
3: Patch order was changed so the replacement is done in 
parallels_co_check.

    Now we use a helper to set BAT entry and mark the block dirty.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a 
separate patch.

7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.

Alexander Ivanov (12):
   parallels: Out of image offset in BAT leads to image inflation
   parallels: Fix high_off calculation in parallels_co_check()
   parallels: Fix image_end_offset and data_end after out-of-image check
   parallels: create parallels_set_bat_entry_helper() to assign BAT 
value

   parallels: Use generic infrastructure for BAT writing in
 parallels_co_check()
   parallels: Move check of unclean image to a separate function
   parallels: Move check of cluster outside image to a separate function
   parallels: Fix statistics calculation
   parallels: Move check of leaks to a separate function
   parallels: Move statistic collection to a separate function
   parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
   parallels: Incorrect condition in out-of-image check

  block/parallels.c | 190 +-
  1 file changed, 136 insertions(+), 54 deletions(-)


I am a little bit puzzled - there are 2 series v11 sent within
15 minutes. Which one is correct?

Den

Re: [PATCH v3 13/13] migration: Move migration_properties to options.c

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 08/13] migration: Remove MigrationState from block_cleanup_parameters()

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> On 24.04.23 21:32, Juan Quintela wrote:
>> This makes the function more regular with everything else.
>> Signed-off-by: Juan Quintela 
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Thanks.

>> ---
>>   migration/migration.c | 4 ++--
>>   migration/options.c   | 4 +++-
>>   migration/options.h   | 2 +-
>>   3 files changed, 6 insertions(+), 4 deletions(-)
>> diff --git a/migration/migration.c b/migration/migration.c
>> index cefe6da2b8..ef8caa79b9 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1218,7 +1218,7 @@ static void migrate_fd_cleanup(MigrationState *s)
>>   error_report_err(error_copy(s->error));
>>   }
>>   notifier_list_notify(_state_notifiers, s);
>> -block_cleanup_parameters(s);
>> +block_cleanup_parameters();
>>   yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>   }
>>   @@ -1712,7 +1712,7 @@ void qmp_migrate(const char *uri, bool
>> has_blk, bool blk,
>>  "a valid migration protocol");
>>   migrate_set_state(>state, MIGRATION_STATUS_SETUP,
>> MIGRATION_STATUS_FAILED);
>> -block_cleanup_parameters(s);
>> +block_cleanup_parameters();
>>   return;
>>   }
>>   diff --git a/migration/options.c b/migration/options.c
>> index 26fe00799b..f65b7babef 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -597,8 +597,10 @@ void migrate_set_block_incremental(bool value)
>> /* parameters helpers */
>>   -void block_cleanup_parameters(MigrationState *s)
>> +void block_cleanup_parameters(void)
>>   {
>> +MigrationState *s = migrate_get_current();
>> +
>>   if (s->must_remove_block_options) {
>>   /* setting to false can never fail */
>>   migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, _abort);
>> diff --git a/migration/options.h b/migration/options.h
>> index 1fc8d341dd..3948218dbe 100644
>> --- a/migration/options.h
>> +++ b/migration/options.h
>> @@ -90,6 +90,6 @@ void migrate_set_block_incremental(bool value);
>> bool migrate_params_check(MigrationParameters *params, Error
>> **errp);
>>   void migrate_params_init(MigrationParameters *params);
>> -void block_cleanup_parameters(MigrationState *s);
>> +void block_cleanup_parameters(void);
>
> Don't you want to rename it to migrate_* ?

The idea is to deprecate block migration.  There are much better things
on the block layer to migrate disks.  So I think we can let it from now
there.


Later, Juan.

Re: [PATCH v3 06/13] migration: Move migrate_set_block_incremental() to options.c

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> On 24.04.23 21:32, Juan Quintela wrote:
>> Once there, make it more regular and remove th eneed for
>
> some type here
Thanks, fixed.


>
>> MigrationState parameter.
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Re: [PATCH v3 12/13] migration: Create migrate_block_bitmap_mapping() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Notice that we changed the test of ->has_block_bitmap_mapping
for the test that block_bitmap_mapping is not NULL.

Signed-off-by: Juan Quintela 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  migration/block-dirty-bitmap.c | 14 --
  migration/options.c|  7 +++
  migration/options.h|  1 +
  3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index a6ffae0002..62b2352bbb 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -605,11 +605,12 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
  SaveBitmapState *dbms;
  GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
  BlockBackend *blk;
-const MigrationParameters *mig_params = _get_current()->parameters;
  GHashTable *alias_map = NULL;
+BitmapMigrationNodeAliasList *block_bitmap_mapping =
+migrate_block_bitmap_mapping();
  
-if (mig_params->has_block_bitmap_mapping) {

-alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
+if (block_bitmap_mapping) {
+alias_map = construct_alias_map(block_bitmap_mapping, true,
  _abort);
  }
  
@@ -1158,7 +1159,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,

  static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
  {
  GHashTable *alias_map = NULL;
-const MigrationParameters *mig_params = _get_current()->parameters;
+BitmapMigrationNodeAliasList *block_bitmap_mapping =
+migrate_block_bitmap_mapping();
  DBMLoadState *s = &((DBMState *)opaque)->load;
  int ret = 0;
  
@@ -1170,8 +1172,8 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)

  return -EINVAL;
  }
  
-if (mig_params->has_block_bitmap_mapping) {

-alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
+if (block_bitmap_mapping) {
+alias_map = construct_alias_map(block_bitmap_mapping,
  false, _abort);
  }
  
diff --git a/migration/options.c b/migration/options.c

index 9fbba84b9a..ec234bf3ff 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -452,6 +452,13 @@ void 
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
  
  /* parameters */
  
+BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void)


as well, this could return constant pointer. Even construct_alias_map() is 
already prepared for this.


+{
+MigrationState *s = migrate_get_current();
+
+return s->parameters.block_bitmap_mapping;
+}
+
  bool migrate_block_incremental(void)
  {
  MigrationState *s = migrate_get_current();
diff --git a/migration/options.h b/migration/options.h
index 9123fdb5f4..43e8e9cd8f 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -62,6 +62,7 @@ bool migrate_cap_set(int cap, bool value, Error **errp);
  
  /* parameters */
  
+BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void);

  bool migrate_block_incremental(void);
  uint32_t migrate_checkpoint_delay(void);
  int migrate_compress_level(void);


--
Best regards,
Vladimir

Re: [PATCH v3 11/13] migration: Create migrate_tls_hostname() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

[same recommendations]

--
Best regards,
Vladimir

Re: [PATCH v3 10/13] migration: Create migrate_tls_authz() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela


Reviewed-by: Vladimir Sementsov-Ogievskiy 

Still same recommendations: "const char *" and "s" becomes unused in 
migration_tls_channel_process_incoming()

--
Best regards,
Vladimir

Re: [PATCH v3 09/13] migration: Create migrate_tls_creds() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela 


Reviewed-by: Vladimir Sementsov-Ogievskiy 



---
  migration/options.c | 7 +++
  migration/options.h | 1 +
  migration/tls.c | 9 -
  3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index f65b7babef..9eabb4c25d 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -579,6 +579,13 @@ uint8_t migrate_throttle_trigger_threshold(void)
  return s->parameters.throttle_trigger_threshold;
  }
  
+char *migrate_tls_creds(void)


could be stricter "const char *"


+{
+MigrationState *s = migrate_get_current();
+
+return s->parameters.tls_creds;
+}
+
  uint64_t migrate_xbzrle_cache_size(void)
  {
  MigrationState *s = migrate_get_current();
diff --git a/migration/options.h b/migration/options.h
index 3948218dbe..47cc24585b 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -80,6 +80,7 @@ MultiFDCompression migrate_multifd_compression(void);
  int migrate_multifd_zlib_level(void);
  int migrate_multifd_zstd_level(void);
  uint8_t migrate_throttle_trigger_threshold(void);
+char *migrate_tls_creds(void);
  uint64_t migrate_xbzrle_cache_size(void);
  
  /* parameters setters */

diff --git a/migration/tls.c b/migration/tls.c
index acd38e0b62..0d318516de 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -34,20 +34,19 @@ migration_tls_get_creds(MigrationState *s,
  Error **errp)


"s" argument becomes unused, may be dropped.


  {
  Object *creds;
+char *tls_creds = migrate_tls_creds();
  QCryptoTLSCreds *ret;
  
-creds = object_resolve_path_component(

-object_get_objects_root(), s->parameters.tls_creds);
+creds = object_resolve_path_component(object_get_objects_root(), 
tls_creds);
  if (!creds) {
-error_setg(errp, "No TLS credentials with id '%s'",
-   s->parameters.tls_creds);
+error_setg(errp, "No TLS credentials with id '%s'", tls_creds);
  return NULL;
  }
  ret = (QCryptoTLSCreds *)object_dynamic_cast(
  creds, TYPE_QCRYPTO_TLS_CREDS);
  if (!ret) {
  error_setg(errp, "Object with id '%s' is not TLS credentials",
-   s->parameters.tls_creds);
+   tls_creds);
  return NULL;
  }
  if (!qcrypto_tls_creds_check_endpoint(ret, endpoint, errp)) {


--
Best regards,
Vladimir

Re: [PATCH v3 08/13] migration: Remove MigrationState from block_cleanup_parameters()

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

This makes the function more regular with everything else.

Signed-off-by: Juan Quintela 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


---
  migration/migration.c | 4 ++--
  migration/options.c   | 4 +++-
  migration/options.h   | 2 +-
  3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index cefe6da2b8..ef8caa79b9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1218,7 +1218,7 @@ static void migrate_fd_cleanup(MigrationState *s)
  error_report_err(error_copy(s->error));
  }
  notifier_list_notify(_state_notifiers, s);
-block_cleanup_parameters(s);
+block_cleanup_parameters();
  yank_unregister_instance(MIGRATION_YANK_INSTANCE);
  }
  
@@ -1712,7 +1712,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,

 "a valid migration protocol");
  migrate_set_state(>state, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_FAILED);
-block_cleanup_parameters(s);
+block_cleanup_parameters();
  return;
  }
  
diff --git a/migration/options.c b/migration/options.c

index 26fe00799b..f65b7babef 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -597,8 +597,10 @@ void migrate_set_block_incremental(bool value)
  
  /* parameters helpers */
  
-void block_cleanup_parameters(MigrationState *s)

+void block_cleanup_parameters(void)
  {
+MigrationState *s = migrate_get_current();
+
  if (s->must_remove_block_options) {
  /* setting to false can never fail */
  migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, false, _abort);
diff --git a/migration/options.h b/migration/options.h
index 1fc8d341dd..3948218dbe 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -90,6 +90,6 @@ void migrate_set_block_incremental(bool value);
  
  bool migrate_params_check(MigrationParameters *params, Error **errp);

  void migrate_params_init(MigrationParameters *params);
-void block_cleanup_parameters(MigrationState *s);
+void block_cleanup_parameters(void);


Don't you want to rename it to migrate_* ?

--
Best regards,
Vladimir

Re: [PATCH v3 07/13] migration: Move block_cleanup_parameters() to options.c

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 06/13] migration: Move migrate_set_block_incremental() to options.c

[Vladimir Sementsov-Ogievskiy]

On 26.04.23 21:45, Vladimir Sementsov-Ogievskiy wrote:

On 24.04.23 21:32, Juan Quintela wrote:

Once there, make it more regular and remove th eneed for


some type here


haha, and my typo here




MigrationState parameter.


Reviewed-by: Vladimir Sementsov-Ogievskiy 



--
Best regards,
Vladimir

Re: [PATCH v3 06/13] migration: Move migrate_set_block_incremental() to options.c

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Once there, make it more regular and remove th eneed for


some type here


MigrationState parameter.


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 05/13] hw/ide: Extract pci_ide_class_init()

[Bernhard Beschow]

Am 26. April 2023 11:04:30 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Resolves redundant code in every PCI IDE device model.
>
>I think this needs to mention that it's moving the PCIDeviceClass::exit() 
>function from all of the PCI IDE controller implementations to a common 
>implementation in the parent PCI_IDE type.

I'll completely rework this patch.

>
>> ---
>>   include/hw/ide/pci.h |  1 -
>>   hw/ide/cmd646.c  | 15 ---
>>   hw/ide/pci.c | 25 -
>>   hw/ide/piix.c| 19 ---
>>   hw/ide/sii3112.c |  3 ++-
>>   hw/ide/via.c | 15 ---
>>   6 files changed, 26 insertions(+), 52 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 74c127e32f..7bc4e53d02 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -61,7 +61,6 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>>   extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>>   -extern const VMStateDescription vmstate_ide_pci;
>>   extern const MemoryRegionOps pci_ide_cmd_le_ops;
>>   extern const MemoryRegionOps pci_ide_data_le_ops;
>>   #endif
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index a094a6e12a..9aabf80e52 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -301,17 +301,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, 
>> Error **errp)
>>   }
>>   }
>>   -static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>> -{
>> -PCIIDEState *d = PCI_IDE(dev);
>> -unsigned i;
>> -
>> -for (i = 0; i < 2; ++i) {
>> -memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
>> -memory_region_del_subregion(>bmdma_bar, 
>> >bmdma[i].addr_ioport);
>> -}
>> -}
>> -
>>   static Property cmd646_ide_properties[] = {
>>   DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
>>   DEFINE_PROP_END_OF_LIST(),
>> @@ -323,17 +312,13 @@ static void cmd646_ide_class_init(ObjectClass *klass, 
>> void *data)
>>   PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> dc->reset = cmd646_reset;
>> -dc->vmsd = _ide_pci;
>>   k->realize = pci_cmd646_ide_realize;
>> -k->exit = pci_cmd646_ide_exitfn;
>>   k->vendor_id = PCI_VENDOR_ID_CMD;
>>   k->device_id = PCI_DEVICE_ID_CMD_646;
>>   k->revision = 0x07;
>> -k->class_id = PCI_CLASS_STORAGE_IDE;
>>   k->config_read = cmd646_pci_config_read;
>>   k->config_write = cmd646_pci_config_write;
>>   device_class_set_props(dc, cmd646_ide_properties);
>> -set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>   }
>> static const TypeInfo cmd646_ide_info = {
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 67e0998ff0..8bea92e394 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -467,7 +467,7 @@ static int ide_pci_post_load(void *opaque, int 
>> version_id)
>>   return 0;
>>   }
>>   -const VMStateDescription vmstate_ide_pci = {
>> +static const VMStateDescription vmstate_ide_pci = {
>>   .name = "ide",
>>   .version_id = 3,
>>   .minimum_version_id = 0,
>> @@ -530,11 +530,34 @@ static void pci_ide_init(Object *obj)
>>   qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
>>   }
>>   +static void pci_ide_exitfn(PCIDevice *dev)
>> +{
>> +PCIIDEState *d = PCI_IDE(dev);
>> +unsigned i;
>> +
>> +for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
>> +memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
>> +memory_region_del_subregion(>bmdma_bar, 
>> >bmdma[i].addr_ioport);
>> +}
>> +}
>> +
>> +static void pci_ide_class_init(ObjectClass *klass, void *data)
>> +{
>> +DeviceClass *dc = DEVICE_CLASS(klass);
>> +PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +dc->vmsd = _ide_pci;
>> +k->exit = pci_ide_exitfn;
>> +k->class_id = PCI_CLASS_STORAGE_IDE;
>> +set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> +}
>> +
>>   static const TypeInfo pci_ide_type_info = {
>>   .name = TYPE_PCI_IDE,
>>   .parent = TYPE_PCI_DEVICE,
>>   .instance_size = sizeof(PCIIDEState),
>>   .instance_init = pci_ide_init,
>> +.class_init = pci_ide_class_init,
>>   .abstract = true,
>>   .interfaces = (InterfaceInfo[]) {
>>   { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index a32f7ccece..4e6ca99123 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -159,8 +159,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>   bmdma_setup_bar(d);
>>   pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
>>   -vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);
>> -
>
>Presumably this still survives migration between a pre-series and post-series 
>QEMU using the PIIX IDE controller?
>
>>   for (unsigned i = 0; i < 2; i++) {
>>   if (!pci_piix_init_bus(d, i, errp)) {
>>   

Re: [PATCH v3 05/13] migration: Create migrate_downtime_limit() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 08/13] hw/ide: Rename PCIIDEState::*_bar attributes

[Bernhard Beschow]

Am 26. April 2023 11:21:28 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> The attributes represent memory regions containing operations which are 
>> mapped
>> by the device models into PCI BARs. Reflect this by changing the suffic into
>> "_ops".
>> 
>> Note that in a few commits piix will also use the {cmd,data}_ops but won't 
>> map
>> them into BARs. This further suggests that the "_bar" suffix doesn't match
>> very well.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  6 +++---
>>   hw/ide/cmd646.c  | 10 +-
>>   hw/ide/pci.c | 18 +-
>>   hw/ide/piix.c|  2 +-
>>   hw/ide/via.c | 10 +-
>>   5 files changed, 23 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 597c77c7ad..5025df5b82 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -51,9 +51,9 @@ struct PCIIDEState {
>>   BMDMAState bmdma[2];
>>   qemu_irq isa_irq[2];
>>   uint32_t secondary; /* used only for cmd646 */
>> -MemoryRegion bmdma_bar;
>> -MemoryRegion cmd_bar[2];
>> -MemoryRegion data_bar[2];
>> +MemoryRegion bmdma_ops;
>> +MemoryRegion cmd_ops[2];
>> +MemoryRegion data_ops[2];
>>   };
>> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 85716aaf17..b9d005a357 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, 
>> Error **errp)
>>   dev->wmask[MRDMODE] = 0x0;
>>   dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
>>   -pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[0]);
>> -pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[0]);
>> -pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[1]);
>> -pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[1]);
>> +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
>> +pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
>> +pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
>> +pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
>> bmdma_init_ops(d, _bmdma_ops);
>> -pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
>> +pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_ops);
>> /* TODO: RST# value should be 0 */
>>   pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index a9194313bd..b2fcc00a64 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const 
>> MemoryRegionOps *bmdma_ops)
>>   {
>>   size_t i;
>>   -memory_region_init(>bmdma_bar, OBJECT(d), "bmdma-container", 16);
>> +memory_region_init(>bmdma_ops, OBJECT(d), "bmdma-container", 16);
>>   for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) {
>>   BMDMAState *bm = >bmdma[i];
>> memory_region_init_io(>extra_io, OBJECT(d), bmdma_ops, bm, 
>> "bmdma-ops", 4);
>> -memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
>> +memory_region_add_subregion(>bmdma_ops, i * 8, >extra_io);
>>   memory_region_init_io(>addr_ioport, OBJECT(d), 
>> _addr_ioport_ops, bm,
>> "bmdma-ioport-ops", 4);
>> -memory_region_add_subregion(>bmdma_bar, i * 8 + 4, 
>> >addr_ioport);
>> +memory_region_add_subregion(>bmdma_ops, i * 8 + 4, 
>> >addr_ioport);
>>   }
>>   }
>>   @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj)
>>   {
>>   PCIIDEState *d = PCI_IDE(obj);
>>   -memory_region_init_io(>data_bar[0], OBJECT(d), 
>> _ide_data_le_ops,
>> +memory_region_init_io(>data_ops[0], OBJECT(d), _ide_data_le_ops,
>> >bus[0], "pci-ide0-data-ops", 8);
>> -memory_region_init_io(>cmd_bar[0], OBJECT(d), _ide_cmd_le_ops,
>> +memory_region_init_io(>cmd_ops[0], OBJECT(d), _ide_cmd_le_ops,
>> >bus[0], "pci-ide0-cmd-ops", 4);
>>   -memory_region_init_io(>data_bar[1], OBJECT(d), 
>> _ide_data_le_ops,
>> +memory_region_init_io(>data_ops[1], OBJECT(d), _ide_data_le_ops,
>> >bus[1], "pci-ide1-data-ops", 8);
>> -memory_region_init_io(>cmd_bar[1], OBJECT(d), _ide_cmd_le_ops,
>> +memory_region_init_io(>cmd_ops[1], OBJECT(d), _ide_cmd_le_ops,
>> >bus[1], "pci-ide1-cmd-ops", 4);
>> qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
>> @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev)
>>   unsigned i;
>> for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
>> -memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
>> -memory_region_del_subregion(>bmdma_bar, 
>> 

Re: [PATCH 10/13] hw/ide/piix: Reuse PCIIDEState::{cmd,data}_ops

[Bernhard Beschow]

Am 26. April 2023 11:37:48 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Now that PCIIDEState::{cmd,data}_ops are initialized in the base class
>> constructor there is an opportunity for PIIX to reuse these attributes. This
>> resolves usage of ide_init_ioport() which would fall back internally to using
>> the isabus global due to NULL being passed as ISADevice by PIIX.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   hw/ide/piix.c | 30 +-
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index a3a15dc7db..406a67fa0f 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -104,34 +104,32 @@ static void piix_ide_reset(DeviceState *dev)
>>   pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>   }
>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
>> -  Error **errp)
>> +static void pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus)
>>   {
>>   static const struct {
>>   int iobase;
>>   int iobase2;
>>   int isairq;
>>   } port_info[] = {
>> -{0x1f0, 0x3f6, 14},
>> -{0x170, 0x376, 15},
>> +{0x1f0, 0x3f4, 14},
>> +{0x170, 0x374, 15},
>>   };
>> -int ret;
>> +MemoryRegion *address_space_io = pci_address_space_io(PCI_DEVICE(d));
>> ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> -ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
>> -  port_info[i].iobase2);
>> -if (ret) {
>> -error_setg_errno(errp, -ret, "Failed to realize %s port %u",
>> - object_get_typename(OBJECT(d)), i);
>> -return false;
>> -}
>> +memory_region_add_subregion(address_space_io, port_info[i].iobase,
>> +>data_ops[i]);
>> +/*
>> + * PIIX forwards the last byte of cmd_ops to ISA. Model this using a low
>> + * prio so competing memory regions take precedence.
>> + */
>> +memory_region_add_subregion_overlap(address_space_io, 
>> port_info[i].iobase2,
>> +>cmd_ops[i], -1);
>
>Interesting. Is this behaviour documented somewhere and/or used in one of your 
>test images at all? If I'd have seen this myself, I probably thought that the 
>addresses were a typo...

I first  stumbled upon this and wondered why this code was working with VIA_IDE 
(through my pc-via branch). Then I found the correct offsets there which are 
confirmed in the piix datasheet, e.g.: "Secondary Control Block Offset: 0374h"

>
>>   ide_bus_init_output_irq(>bus[i],
>>   isa_bus_get_irq(isa_bus, port_info[i].isairq));
>> bmdma_init(>bus[i], >bmdma[i], d);
>>   ide_bus_register_restart_cb(>bus[i]);
>> -
>> -return true;
>>   }
>> static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>> @@ -160,9 +158,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>   }
>> for (unsigned i = 0; i < 2; i++) {
>> -if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>> -return;
>> -}
>> +pci_piix_init_bus(d, i, isa_bus);
>>   }
>>   }
>>   
>
>
>ATB,
>
>Mark.

Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq()

[Bernhard Beschow]

Am 26. April 2023 11:33:40 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
>> Passing a NULL pointer works but causes the isabus global to be used
>> then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
>> achieve the same as using isa_get_irq().
>> 
>> This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
>> crash when plugging a piix3-ide device into the x-remote machine' which
>> allows for cleaning up the ISA API while keeping PIIX IDE functions
>> user-createable.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   hw/ide/piix.c | 23 ---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 6942b484f9..a3a15dc7db 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -104,7 +104,8 @@ static void piix_ide_reset(DeviceState *dev)
>>   pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>   }
>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>> +static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
>> +  Error **errp)
>>   {
>>   static const struct {
>>   int iobase;
>> @@ -124,7 +125,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned 
>> i, Error **errp)
>>object_get_typename(OBJECT(d)), i);
>>   return false;
>>   }
>> -ide_bus_init_output_irq(>bus[i], isa_get_irq(NULL, 
>> port_info[i].isairq));
>> +ide_bus_init_output_irq(>bus[i],
>> +isa_bus_get_irq(isa_bus, port_info[i].isairq));
>
>I don't think is the right solution here, since ultimately we want to move the 
>IRQ routing out of the device itself and into the PCI-ISA bridge. I'd go for 
>the same solution as you've done for VIA IDE in patch 2, i.e. update the PIIX 
>interrupt handler to set the legacy_irqs in PCIIDEState, and then wire them up 
>to the ISA IRQs 14 and 15 similar to as Phil as done in his patches:

The problem is user-creatable PIIX-IDE. IMO we should stick to our deprecation 
process before going this step because this will break it.

>
>https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-4-phi...@linaro.org/
>
>https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-5-phi...@linaro.org/
>
>This also reminds me, given that the first patch above is doing wiring in 
>pc_init1() then we are still missing part of your tidy-up series :/
>
>>   bmdma_init(>bus[i], >bmdma[i], d);
>>   ide_bus_register_restart_cb(>bus[i]);
>> @@ -136,14 +138,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>   {
>>   PCIIDEState *d = PCI_IDE(dev);
>>   uint8_t *pci_conf = dev->config;
>> +ISABus *isa_bus;
>> +bool ambiguous;
>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>> bmdma_init_ops(d, _bmdma_ops);
>>   pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_ops);
>>   +isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS, 
>> ));
>> +if (ambiguous) {
>> +error_setg(errp,
>> +   "More than one ISA bus found while %s supports only one",
>> +   object_get_typename(OBJECT(d)));
>> +return;
>> +}
>> +if (!isa_bus) {
>> +error_setg(errp, "No ISA bus found while %s requires one",
>> +   object_get_typename(OBJECT(d)));
>> +return;
>> +}
>
>Again I think this should go away with using PCIIDEState's legacy_irqs, since 
>you simply let the board wire them up to the ISABus (or not) as required.

Same here: This breaks user-creatable PIIX-IDE.

>
>>   for (unsigned i = 0; i < 2; i++) {
>> -if (!pci_piix_init_bus(d, i, errp)) {
>> +if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>>   return;
>>   }
>>   }
>
>
>ATB,
>
>Mark.

Re: [PATCH v3 04/13] migration: Make all functions check have the same format

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 03/13] migration: Create migrate_params_init() function

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 21:32, Juan Quintela wrote:

Signed-off-by: Juan Quintela


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

[PATCH v9 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Alexander Bulekov]

This protects devices from bh->mmio reentrancy issues.

Thanks: Thomas Huth  for diagnosing OS X test failure.
Reviewed-by: Darren Kenny 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Paul Durrant 
Signed-off-by: Alexander Bulekov 
Reviewed-by: Thomas Huth 
---
 hw/9pfs/xen-9p-backend.c| 5 -
 hw/block/dataplane/virtio-blk.c | 3 ++-
 hw/block/dataplane/xen-block.c  | 5 +++--
 hw/char/virtio-serial-bus.c | 3 ++-
 hw/display/qxl.c| 9 ++---
 hw/display/virtio-gpu.c | 6 --
 hw/ide/ahci.c   | 3 ++-
 hw/ide/ahci_internal.h  | 1 +
 hw/ide/core.c   | 4 +++-
 hw/misc/imx_rngc.c  | 6 --
 hw/misc/macio/mac_dbdma.c   | 2 +-
 hw/net/virtio-net.c | 3 ++-
 hw/nvme/ctrl.c  | 6 --
 hw/scsi/mptsas.c| 3 ++-
 hw/scsi/scsi-bus.c  | 3 ++-
 hw/scsi/vmw_pvscsi.c| 3 ++-
 hw/usb/dev-uas.c| 3 ++-
 hw/usb/hcd-dwc2.c   | 3 ++-
 hw/usb/hcd-ehci.c   | 3 ++-
 hw/usb/hcd-uhci.c   | 2 +-
 hw/usb/host-libusb.c| 6 --
 hw/usb/redirect.c   | 6 --
 hw/usb/xen-usb.c| 3 ++-
 hw/virtio/virtio-balloon.c  | 5 +++--
 hw/virtio/virtio-crypto.c   | 3 ++-
 25 files changed, 66 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
 
 int num_rings;
 Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
 } Xen9pfsDev;
 
 static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
 xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
 
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, 
_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ _9pdev->rings[i],
+ 
_9pdev->mem_reentrancy_guard);
 xen_9pdev->rings[i].out_cons = 0;
 xen_9pdev->rings[i].out_size = 0;
 xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 } else {
 s->ctx = qemu_get_aio_context();
 }
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+   (vdev)->mem_reentrancy_guard);
 s->batch_notify_vqs = bitmap_new(conf->num_queues);
 
 *dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice 
*xendev,
 } else {
 dataplane->ctx = qemu_get_aio_context();
 }
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
-   dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+   dataplane,
+   (xendev)->mem_reentrancy_guard);
 
 return dataplane;
 }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+   >mem_reentrancy_guard);
 port->elem = NULL;
 }
 
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error 
**errp)
 
 qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
 
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+  (qxl)->mem_reentrancy_guard);
 qxl_reset_state(qxl);
 
-qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
-qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, >ssd);
+qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
+

[PATCH v9 2/8] async: Add an optional reentrancy guard to the BH API

[Alexander Bulekov]

Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.

Reviewed-by: Darren Kenny 
Signed-off-by: Alexander Bulekov 
---
 docs/devel/multiple-iothreads.txt |  7 +++
 include/block/aio.h   | 18 --
 include/qemu/main-loop.h  |  7 +--
 tests/unit/ptimer-test-stubs.c|  3 ++-
 util/async.c  | 18 +-
 util/main-loop.c  |  5 +++--
 util/trace-events |  1 +
 7 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/docs/devel/multiple-iothreads.txt 
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
  * LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
  * LEGACY timer_new_ms() - create a timer
  * LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
  * LEGACY qemu_aio_wait() - run an event loop iteration
 
 Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see 
include/block/aio.h):
  * aio_set_event_notifier() - monitor an event notifier
  * aio_timer_new() - create a timer
  * aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
  * aio_poll() - run an event loop iteration
 
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
 The AioContext can be obtained from the IOThread using
 iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
 Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
 #include "qemu/thread.h"
 #include "qemu/timer.h"
 #include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
 
 typedef struct BlockAIOCB BlockAIOCB;
 typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, 
QEMUBHFunc *cb, void *opaque,
  * is opaque and must be allocated prior to its use.
  *
  * @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
  */
 QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard 
*reentrancy_guard);
 
 /**
  * aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, 
void *opaque,
  * string.
  */
 #define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
 
 /**
  * aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
 
 /* internal interfaces */
 
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
 #define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
 void qemu_bh_schedule_idle(QEMUBH *bh);
 
 enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int 
attr_mask)
 return deadline;
 }
 
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void 

[PATCH v9 7/8] memory: abort on re-entrancy in debug builds

[Alexander Bulekov]

This is useful for using unit-tests/fuzzing to detect bugs introduced by
the re-entrancy guard mechanism into devices that are intentionally
re-entrant.

Signed-off-by: Alexander Bulekov 
Reviewed-by: Thomas Huth 
---
 softmmu/memory.c | 3 +++
 util/async.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index af9365bb81..d038633a6c 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -547,6 +547,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
 !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
 if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
 trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+#ifdef DEBUG
+abort();
+#endif
 return MEMTX_ACCESS_ERROR;
 }
 mr->dev->mem_reentrancy_guard.engaged_in_io = true;
diff --git a/util/async.c b/util/async.c
index a9b528c370..2dc9389e0d 100644
--- a/util/async.c
+++ b/util/async.c
@@ -160,6 +160,9 @@ void aio_bh_call(QEMUBH *bh)
 last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
 if (bh->reentrancy_guard->engaged_in_io) {
 trace_reentrant_aio(bh->ctx, bh->name);
+#ifdef DEBUG
+abort();
+#endif
 }
 bh->reentrancy_guard->engaged_in_io = true;
 }
-- 
2.39.0

Re: [PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug

[Denis V. Lunev]

On 4/24/23 11:31, Alexander Ivanov wrote:

Fix image inflation when offset in BAT is out of image.

Replace whole BAT syncing by flushing only dirty blocks.

Move all the checks outside the main check function in
separate functions

Use WITH_QEMU_LOCK_GUARD for simplier code.

Fix incorrect condition in out-of-image check.

v11:
1: Use bdrv_nb_sectors() instead of bdrv_getlength() to get image size in 
sectors.
7,9: Add coroutine_fn and GRAPH_RDLOCK annotations.

v10:
8: Add a comment.
9: Exclude unrelated changes.

v9:
3: Add (high_off == 0) case handling.
7: Move res->image_end_offset setting to parallels_check_outside_image().
8: Add a patch with a statistics calculation fix.
9: Remove redundant high_off calculation.
12: Change the condition to (off + s->cluster_size > size).

v8: Rebase on the top of the current master branch.

v7:
1,2: Fix string lengths in the commit messages.
3: Fix a typo in the commit message.

v6:
1: Move the error check inside the loop. Move file size getting
to the function beginning. Skip out-of-image offsets.
2: A new patch - don't let high_off be more than the end of the last cluster.
3: Set data_end without any condition.
7: Move data_end setting to parallels_check_outside_image().
8: Remove s->data_end setting from parallels_check_leak().
Fix 'i' type.

v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().

v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
in parallels_open() and the fix in parallels_co_check() to two patches.
2: A new patch - a part of the patch 1.
Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.

v3:

1-8: Fix commit message.

v2:

2: A new patch - a part of the splitted patch 2.
3: Patch order was changed so the replacement is done in parallels_co_check.
Now we use a helper to set BAT entry and mark the block dirty.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a separate patch.
7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.

Alexander Ivanov (12):
   parallels: Out of image offset in BAT leads to image inflation
   parallels: Fix high_off calculation in parallels_co_check()
   parallels: Fix image_end_offset and data_end after out-of-image check
   parallels: create parallels_set_bat_entry_helper() to assign BAT value
   parallels: Use generic infrastructure for BAT writing in
 parallels_co_check()
   parallels: Move check of unclean image to a separate function
   parallels: Move check of cluster outside image to a separate function
   parallels: Fix statistics calculation
   parallels: Move check of leaks to a separate function
   parallels: Move statistic collection to a separate function
   parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
   parallels: Incorrect condition in out-of-image check

  block/parallels.c | 190 +-
  1 file changed, 136 insertions(+), 54 deletions(-)


I am a little bit puzzled - there are 2 series v11 sent within
15 minutes. Which one is correct?

Den

Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll

[Fiona Ebner]

Am 20.04.23 um 08:55 schrieb Paolo Bonzini:
> 
> 
> Il gio 20 apr 2023, 08:11 Markus Armbruster  > ha scritto:
> 
> So, splicing in a bottom half unmoored monitor commands from the main
> loop.  We weren't aware of that, as our commit messages show.
> 
> I guess the commands themselves don't care; all they need is the BQL.
> 
> However, did we unwittingly change what can get blocked?  Before,
> monitor commands could block only the main thread.  Now they can also
> block vCPU threads.  Impact?
> 
> 
> Monitor commands could always block vCPU threads through the BQL(*).
> However, aio_poll() only runs in the vCPU threads in very special cases;
> typically associated to resetting a device which causes a blk_drain() on
> the device's BlockBackend. So it is not a performance issue.
> 

AFAIU, all generated coroutine wrappers use aio_poll. In my backtrace
aio_poll happens via blk_pwrite for a pflash device. So a bit more often
than "very special cases" ;)

> However, liberal reuse of the main block layer AioContext could indeed
> be a *correctness* issue. I need to re-read Fiona's report instead of
> stopping at the first three lines because it's the evening. :)

For me, being called in a vCPU thread caused problems with a custom QMP
function patched in by Proxmox. The function uses a newly opened
BlockBackend and calls qemu_mutex_unlock_iothread() after which
qemu_get_current_aio_context() returns 0x0 (when running in the main
thread, it still returns the main thread's AioContext). It then calls
blk_pwritev which is also a generated coroutine wrapper and the
assert(qemu_get_current_aio_context() == qemu_get_aio_context());
in the else branch of the AIO_WAIT_WHILE_INTERNAL macro fails.

Sounds like there's room for improvement in our code :/ I'm not aware of
something similar in upstream QEMU.

Thanks to Markus for the detailed history lesson!

Best Regards,
Fiona

Re: [PATCH v3 14/18] exec/ioport: Factor portio_list_register() out

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:


We always follow the same pattern when registering
non-coalesced portio:

   - portio_list_init()
   - portio_list_add()

Factor these 2 operations in a single helper named
portio_list_register(). Since both calls become local
to ioport.c, reduce their scope by declaring them static.

Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20230207234615.77300-3-phi...@linaro.org>
---
  hw/audio/adlib.c|  4 ++--
  hw/display/vga.c|  4 ++--
  hw/dma/i82374.c |  7 +++
  hw/ide/ioport.c |  9 -
  hw/isa/isa-bus.c|  5 ++---
  hw/watchdog/wdt_ib700.c |  4 ++--
  include/exec/ioport.h   | 10 --
  softmmu/ioport.c| 21 ++---
  8 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
index 5f979b1487..cc03c99306 100644
--- a/hw/audio/adlib.c
+++ b/hw/audio/adlib.c
@@ -291,8 +291,8 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
  
  adlib_portio_list[0].offset = s->port;

  adlib_portio_list[1].offset = s->port + 8;
-portio_list_init (>port_list, OBJECT(s), adlib_portio_list, s, "adlib");
-portio_list_add (>port_list, isa_address_space_io(>parent_obj), 0);
+portio_list_register(>port_list, OBJECT(s), adlib_portio_list, s,
+ "adlib", isa_address_space_io(>parent_obj), 0);
  }
  
  static Property adlib_properties[] = {

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 98d644922e..aa899fddc3 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2313,7 +2313,7 @@ void vga_init(VGACommonState *s, Object *obj, 
MemoryRegion *address_space,
   s, "vga", address_space_io, 
0x3b0);
  }
  if (vbe_ports) {
-portio_list_init(>vbe_port_list, obj, vbe_ports, s, "vbe");
-portio_list_add(>vbe_port_list, address_space_io, 0x1ce);
+portio_list_register(>vbe_port_list, obj, vbe_ports, s,
+ "vbe", address_space_io, 0x1ce);
  }
  }
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 63734c22c9..aeca0e8323 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -131,10 +131,9 @@ static void i82374_realize(DeviceState *dev, Error **errp)
  }
  i8257_dma_init(isa_bus, true);
  
-portio_list_init(>port_list, OBJECT(s), i82374_portio_list, s,

- "i82374");
-portio_list_add(>port_list, isa_address_space_io(>parent_obj),
-s->iobase);
+portio_list_register(>port_list, OBJECT(s), i82374_portio_list, s,
+ "i82374", isa_address_space_io(>parent_obj),
+ s->iobase);
  
  memset(s->commands, 0, sizeof(s->commands));

  }
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index ed7957dbae..7a6f29955f 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -60,9 +60,8 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
  void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
   int iobase, int iobase2)
  {
-portio_list_init(>portio_list, owner, ide_portio_list, bus, "ide");
-portio_list_add(>portio_list, io, iobase);
-
-portio_list_init(>portio2_list, owner, ide_portio2_list, bus, "ide");
-portio_list_add(>portio_list, io, iobase2);
+portio_list_register(>portio_list, owner, ide_portio_list,
+ bus, "ide", io, iobase);
+portio_list_register(>portio2_list, owner, ide_portio2_list,
+ bus, "ide", io, iobase2);
  }
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 8e3ca3785e..087293108e 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -130,15 +130,14 @@ int isa_register_portio_list(ISADevice *dev,
   void *opaque, const char *name)
  {
  assert(dev);
-assert(piolist && !piolist->owner);
  
  /* START is how we should treat DEV, regardless of the actual

 contents of the portio array.  This is how the old code
 actually handled e.g. the FDC device.  */
  isa_init_ioport(dev, start);
  
-portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);

-portio_list_add(piolist, isa_address_space_io(dev), start);
+portio_list_register(piolist, OBJECT(dev), pio_start, opaque, name,
+ isa_address_space_io(dev), start);
  
  return 0;

  }
diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
index b116c3a3aa..ac4f0be7d8 100644
--- a/hw/watchdog/wdt_ib700.c
+++ b/hw/watchdog/wdt_ib700.c
@@ -115,8 +115,8 @@ static void wdt_ib700_realize(DeviceState *dev, Error 
**errp)
  
  s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ib700_timer_expired, s);
  
-portio_list_init(>port_list, OBJECT(s), wdt_portio_list, s, "ib700");

-portio_list_add(>port_list, isa_address_space_io(>parent_obj), 0);
+portio_list_register(>port_list, OBJECT(s), 

Re: [PATCH v3 08/18] hw/ide: Introduce generic ide_init_ioport()

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:


Add ide_init_ioport() which is not restricted to the ISA bus.
(Next commit will use it for a PCI device).

Inspired-by: Mark Cave-Ayland 
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/ioport.c   | 12 ++--
  include/hw/ide/internal.h |  2 ++
  2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index d869f8018a..ed7957dbae 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -46,8 +46,6 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
  {
  int ret;
  
-/* ??? Assume only ISA and PCI configurations, and that the PCI-ISA

-   bridge has been setup properly to always register with ISA.  */
  ret = isa_register_portio_list(dev, >portio_list,
 iobase, ide_portio_list, bus, "ide");
  
@@ -58,3 +56,13 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
  
  return ret;

  }
+
+void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+ int iobase, int iobase2)
+{
+portio_list_init(>portio_list, owner, ide_portio_list, bus, "ide");
+portio_list_add(>portio_list, io, iobase);
+
+portio_list_init(>portio2_list, owner, ide_portio2_list, bus, "ide");
+portio_list_add(>portio_list, io, iobase2);
+}
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d3b7fdc504..6967ca13e0 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -617,6 +617,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
IDEDriveKind kind,
 uint32_t cylinders, uint32_t heads, uint32_t secs,
 int chs_trans, Error **errp);
  void ide_exit(IDEState *s);
+void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+ int iobase, int iobase2);
  void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
  void ide_bus_set_irq(IDEBus *bus);
  void ide_bus_register_restart_cb(IDEBus *bus);


Since I took the opposite approach with regard to the ISA IDE ioports, I have a very 
similar patch locally except that as all the remaining users are PCIDevices, I moved 
the logic into a function called pci_ide_register_legacy_ioports() in hw/ide/pci.c 
(see https://github.com/mcayland/qemu/commit/c2aa28fd6306eeeb60b3ec21be48dd9c8841e20b).


Thoughts?


ATB,

Mark.

Re: [PATCH v3 06/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:


Ensure both IDE output IRQ lines are wired.

We can remove the last use of isa_get_irq(NULL).

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/piix.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index cb527553e2..91424e5249 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -134,14 +134,17 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
  static const struct {
  int iobase;
  int iobase2;
-int isairq;
  } port_info[] = {
-{0x1f0, 0x3f6, 14},
-{0x170, 0x376, 15},
+{0x1f0, 0x3f6},
+{0x170, 0x376},
  };
  int ret;
  
-qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);

+if (!d->isa_irq[i]) {
+error_setg(errp, "output IDE IRQ %u not connected", i);
+return false;
+}
+
  ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
  ret = ide_bus_init_ioport_isa(>bus[i], NULL, port_info[i].iobase,
port_info[i].iobase2);
@@ -150,7 +153,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
   object_get_typename(OBJECT(d)), i);
  return false;
  }
-ide_bus_init_output_irq(>bus[i], irq_out);
+ide_bus_init_output_irq(>bus[i], d->isa_irq[i]);
  
  bmdma_init(>bus[i], >bmdma[i], d);

  d->bmdma[i].bus = >bus[i];


I'm not sure I agree with this, since an unwired IRQ/gpio is normally a no-op rather 
than an error - do we really want to change this just for one case? Plus wiring with 
legacy_irqs you should already have eliminated isa_get_irq() in the device itself.



ATB,

Mark.

Re: [PATCH v3 05/18] hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:


Rename ide_init_ioport() as ide_bus_init_ioport_isa() to make
explicit it expects an ISA device. Move the declaration to
"hw/ide/isa.h" where it belongs.

Message-Id: <20230215161641.32663-13-phi...@linaro.org>
Reviewed-by: Richard Henderson 
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/ioport.c   | 4 +++-
  hw/ide/isa.c  | 2 +-
  hw/ide/piix.c | 5 +++--
  include/hw/ide/internal.h | 1 -
  include/hw/ide/isa.h  | 3 +++
  5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e2ecc6230c..d869f8018a 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -25,6 +25,7 @@
  
  #include "qemu/osdep.h"

  #include "hw/isa/isa.h"
+#include "hw/ide/isa.h"
  #include "hw/ide/internal.h"
  #include "trace.h"
  
@@ -40,7 +41,8 @@ static const MemoryRegionPortio ide_portio2_list[] = {

  PORTIO_END_OF_LIST(),
  };
  
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)

+int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
+int iobase, int iobase2)
  {
  int ret;
  
diff --git a/hw/ide/isa.c b/hw/ide/isa.c

index 95053e026f..6eed16bf87 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -71,7 +71,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
  ISAIDEState *s = ISA_IDE(dev);
  
  ide_bus_init(>bus, sizeof(s->bus), dev, 0, 2);

-ide_init_ioport(>bus, isadev, s->iobase, s->iobase2);
+ide_bus_init_ioport_isa(>bus, isadev, s->iobase, s->iobase2);
  ide_bus_init_output_irq(>bus, isa_get_irq(isadev, s->irqnum));
  vmstate_register(VMSTATE_IF(dev), 0, _ide_isa, s);
  ide_bus_register_restart_cb(>bus);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 7cb96ef67f..cb527553e2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -33,6 +33,7 @@
  #include "hw/pci/pci.h"
  #include "hw/ide/piix.h"
  #include "hw/ide/pci.h"
+#include "hw/ide/isa.h"
  #include "trace.h"
  
  static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)

@@ -142,8 +143,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
  
  qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);

  ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
-  port_info[i].iobase2);
+ret = ide_bus_init_ioport_isa(>bus[i], NULL, port_info[i].iobase,
+  port_info[i].iobase2);
  if (ret) {
  error_setg_errno(errp, -ret, "Failed to realize %s port %u",
   object_get_typename(OBJECT(d)), i);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d9f1f77dd5..d3b7fdc504 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -618,7 +618,6 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
IDEDriveKind kind,
 int chs_trans, Error **errp);
  void ide_exit(IDEState *s);
  void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
-int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
  void ide_bus_set_irq(IDEBus *bus);
  void ide_bus_register_restart_cb(IDEBus *bus);
  
diff --git a/include/hw/ide/isa.h b/include/hw/ide/isa.h

index 1cd0ff1fa6..7f7a850265 100644
--- a/include/hw/ide/isa.h
+++ b/include/hw/ide/isa.h
@@ -10,11 +10,14 @@
  #define HW_IDE_ISA_H
  
  #include "qom/object.h"

+#include "hw/ide/internal.h"
  
  #define TYPE_ISA_IDE "isa-ide"

  OBJECT_DECLARE_SIMPLE_TYPE(ISAIDEState, ISA_IDE)
  
  ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int irqnum,

  DriveInfo *hd0, DriveInfo *hd1);
+int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *isa,
+int iobase, int iobase2);
  
  #endif


I have a similar, but opposite patch to this in one of my branches where I have a PCI 
IDE controller that can switch between legacy and native modes :)


From my perspective the use of ide_init_ioport() in hw/ide/isa.c is the outlier 
here, because that is the only instance that works on ISADevice, all the other uses 
are PCIDevices. Hence I've gone the other way which is to inline the ISA ioport 
initialisation into isa_ide_realizefn(): 
https://github.com/mcayland/qemu/commit/e94b004d259e5831beadface100e6bb41beca92c.



ATB,

Mark.

Re: [PATCH v3 02/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:


In order to allow Frankenstein uses such plugging a PIIX3
IDE function on a ICH9 chipset (which already exposes AHCI
ports...) as:

   $ qemu-system-x86_64 -M q35 -device piix3-ide

add a kludge to automatically wires the IDE IRQs on an ISA
bus exposed by a PCI-to-ISA bridge (usually function #0).
Restrict this kludge to the PIIX3.

Reported-by: Bernhard Beschow 
Signed-off-by: Philippe Mathieu-Daudé 
---
TODO: describe why this configuration is broken (multiple
output IRQs wired to the same input IRQ can lead to various
IRQ level changed in the iothread, thus missed by the vCPUs).
---
  hw/ide/piix.c | 21 +
  1 file changed, 21 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a36dac8469..7cb96ef67f 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -170,6 +170,18 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  
  vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);
  
+if (!d->isa_irq[0] && !d->isa_irq[1]

+   && DEVICE_GET_CLASS(d)->user_creatable) {
+/* kludge specific to TYPE_PIIX3_IDE */
+Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL);
+
+if (!isabus) {
+error_setg(errp, "Unable to find a single ISA bus");
+return;
+}
+d->isa_irq[0] = isa_bus_get_irq(ISA_BUS(isabus), 14);
+d->isa_irq[1] = isa_bus_get_irq(ISA_BUS(isabus), 15);
+}
  for (unsigned i = 0; i < ARRAY_SIZE(d->isa_irq); i++) {
  if (!pci_piix_init_bus(d, i, errp)) {
  return;
@@ -202,6 +214,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void 
*data)
  k->class_id = PCI_CLASS_STORAGE_IDE;
  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
  dc->hotpluggable = false;
+/*
+ * This function is part of a Super I/O chip and shouldn't be user
+ * creatable. However QEMU accepts impossible hardware setups such
+ * plugging a PIIX IDE function on a ICH ISA bridge.
+ * Keep this Frankenstein (ab)use working.
+ */
+dc->user_creatable = true;
  }
  
  static const TypeInfo piix3_ide_info = {

@@ -225,6 +244,8 @@ static void piix4_ide_class_init(ObjectClass *klass, void 
*data)
  k->class_id = PCI_CLASS_STORAGE_IDE;
  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
  dc->hotpluggable = false;
+/* Reason: Part of a Super I/O chip */
+dc->user_creatable = false;
  }
  
  static const TypeInfo piix4_ide_info = {


Heh. Do we really want to support this configuration? :O  If PIIX is hard-wired to 
use legacy IRQs then our options are limited, since the device will always require 
separate routing to an ISABus which can only be present in the PIIX's own PCI-ISA 
bridge (i.e. it cannot exist standalone).


Having said that, it should be possible to do this with the VIA IDE since that can 
simply be switched to native mode.



ATB,

Mark.

Re: [PATCH v2 1/3] hw/i2c: add mctp core

[Corey Minyard]

On Wed, Apr 26, 2023 at 09:11:16AM +0200, Klaus Jensen wrote:
> On Apr 25 10:19, Corey Minyard wrote:
> > On Tue, Apr 25, 2023 at 08:35:38AM +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen 
> > > 
> > > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > > control message handling as well as handling the actual I2C transport
> > > (packetization).
> > > 
> > > Devices are intended to derive from this and implement the class
> > > methods.
> > > 
> > > Parts of this implementation is inspired by code[1] previously posted by
> > > Jonathan Cameron.
> > 
> > All in all this looks good.  Two comments:
> > 
> > I would like to see the buffer handling consolidated into one function
> > and the length checked, even for (especially for) the outside users of
> > this code, like the nvme code.  Best to avoid future issues with buffer
> > overruns.  This will require reworking the get_message_types function,
> > unfortunately.
> > 
> 
> Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly
> into the mctp core buffer for get_message_bytes(). The contract is that
> it must not write more than the `maxlen` parameter. Is that bad? Would
> it be better that get_message_bytes() returned a pointer to its own
> buffer that hw/mctp can then copy from?

qemu has had several instances of unchecked writing into a buffer
eventually getting it into trouble.  It might be ok in the beginning,
but as things change and code is added, something might come in that is
not ok.

nmi_get_message_types(), for instance, does not check maxlen.

It may be borderline paranoia, but I've seen too many instances where
paranoia was warranted :).

Plus I think it would make the code a little cleaner and easier to
maintain.  If you wanted to change how the buffer worked, trace data put
into the buffer, or something like that, all the code to handle that is
in one place.

> 
> > You have one trace function on a bad receive message check, but lots of
> > other bad receive message checks with no trace.  Just a suggestion, but
> > it might be nice for tracking down issues to trace all the reasons a
> > message is dropped.
> > 
> 
> Sounds reasonable! :)
> 
> Thanks for the review!

Thank you for the submission :).

-corey

Re: [PATCH 13/13] hw/ide: Extract bmdma_clear_status()

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Extract bmdma_clear_status() mirroring bmdma_cmd_writeb().

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  1 +
  hw/ide/cmd646.c  |  2 +-
  hw/ide/pci.c |  7 +++
  hw/ide/piix.c|  2 +-
  hw/ide/sii3112.c | 12 +---
  hw/ide/via.c |  2 +-
  hw/ide/trace-events  |  1 +
  7 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 81e0370202..6a286ad307 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -59,6 +59,7 @@ struct PCIIDEState {
  void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
  void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
  void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
+void bmdma_clear_status(BMDMAState *bm, uint32_t val);
  void pci_ide_create_devs(PCIDevice *dev);
  
  #endif

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index b9d005a357..973c3ff0dc 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
  cmd646_update_irq(pci_dev);
  break;
  case 2:
-bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 
0x06);
+bmdma_clear_status(bm, val);
  break;
  case 3:
  if (bm == >pci_dev->bmdma[0]) {
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 3539b162b7..4aa06be7c6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
  bm->cmd = val & 0x09;
  }
  
+void bmdma_clear_status(BMDMAState *bm, uint32_t val)

+{
+trace_bmdma_update_status(val);
+
+bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & 
~val & 0x06);
+}
+
  static uint64_t bmdma_addr_read(void *opaque, hwaddr addr,
  unsigned width)
  {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 406a67fa0f..9eab615e35 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
  bmdma_cmd_writeb(bm, val);
  break;
  case 2:
-bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 
0x06);
+bmdma_clear_status(bm, val);
  break;
  }
  }
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 373c0dd1ee..1180ff55e7 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr,
  uint64_t val, unsigned int size)
  {
  BMDMAState *bm = opaque;
-SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
+SiI3112PCIState *s = SII3112_PCI(bm->pci_dev);
  int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
  
  trace_sii3112_bmdma_write(size, addr, val);

@@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr,
  bmdma_cmd_writeb(bm, val);
  break;
  case 0x01:
-d->regs[i].swdata = val & 0x3f;
+s->regs[i].swdata = val & 0x3f;
  break;
  case 0x02:
-bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6);
+bmdma_clear_status(bm, val);
  break;
  default:
  break;
@@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  d->regs[0].swdata = val & 0x3f;
  break;
  case 0x12:
-d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) |
-   (d->i.bmdma[0].status & ~val & 6);
+bmdma_clear_status(>i.bmdma[0], val);
  break;
  case 0x18:
  bmdma_cmd_writeb(>i.bmdma[1], val);
@@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  d->regs[1].swdata = val & 0x3f;
  break;
  case 0x1a:
-d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) |
-   (d->i.bmdma[1].status & ~val & 6);
+bmdma_clear_status(>i.bmdma[1], val);
  break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 35dd97e49b..afb97f302a 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
  bmdma_cmd_writeb(bm, val);
  break;
  case 2:
-bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 
0x06);
+bmdma_clear_status(bm, val);
  break;
  default:;
  }
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index a479525e38..d219c64b61 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 
0x%"PRIx64" : 0x%
  # pci.c
  bmdma_reset(void) ""
  bmdma_cmd_writeb(uint32_t val) "val: 0x%08x"
+bmdma_update_status(uint32_t val) "val: 0x%08x"
  bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64
  bmdma_addr_write(uint64_t 

Re: [PATCH 12/13] hw/ide/sii3112: Reuse PCIIDEState::bmdma_ops

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport bmdma_addr_ioport_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.


Nice! I think it's worth adding a brief mention that you've added BMDMA trace-events, 
but otherwise looks sensible to me.



Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  1 -
  hw/ide/pci.c |  2 +-
  hw/ide/sii3112.c | 94 ++--
  hw/ide/trace-events  |  6 ++-
  4 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index dbb4b13161..81e0370202 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -59,7 +59,6 @@ struct PCIIDEState {
  void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
  void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
  void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
-extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  
  #endif

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 97ccc75aa6..3539b162b7 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -342,7 +342,7 @@ static void bmdma_addr_write(void *opaque, hwaddr addr,
  bm->addr |= ((data & mask) << shift) & ~3;
  }
  
-MemoryRegionOps bmdma_addr_ioport_ops = {

+static MemoryRegionOps bmdma_addr_ioport_ops = {
  .read = bmdma_addr_read,
  .write = bmdma_addr_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 9cf920369f..373c0dd1ee 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -34,47 +34,73 @@ struct SiI3112PCIState {
  SiI3112Regs regs[2];
  };
  
-/* The sii3112_reg_read and sii3112_reg_write functions implement the

- * Internal Register Space - BAR5 (section 6.7 of the data sheet).
- */
-
-static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
-unsigned int size)
+static uint64_t sii3112_bmdma_read(void *opaque, hwaddr addr, unsigned int 
size)
  {
-SiI3112PCIState *d = opaque;
+BMDMAState *bm = opaque;
+SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
+int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
  uint64_t val;
  
  switch (addr) {

  case 0x00:
-val = d->i.bmdma[0].cmd;
+val = bm->cmd;
  break;
  case 0x01:
-val = d->regs[0].swdata;
+val = d->regs[i].swdata;
  break;
  case 0x02:
-val = d->i.bmdma[0].status;
+val = bm->status;
  break;
  case 0x03:
  val = 0;
  break;
-case 0x04 ... 0x07:
-val = bmdma_addr_ioport_ops.read(>i.bmdma[0], addr - 4, size);
-break;
-case 0x08:
-val = d->i.bmdma[1].cmd;
+default:
+val = 0;
  break;
-case 0x09:
-val = d->regs[1].swdata;
+}
+trace_sii3112_bmdma_read(size, addr, val);
+return val;
+}
+
+static void sii3112_bmdma_write(void *opaque, hwaddr addr,
+uint64_t val, unsigned int size)
+{
+BMDMAState *bm = opaque;
+SiI3112PCIState *d = SII3112_PCI(bm->pci_dev);
+int i = (bm == >pci_dev->bmdma[0]) ? 0 : 1;
+
+trace_sii3112_bmdma_write(size, addr, val);
+switch (addr) {
+case 0x00:
+bmdma_cmd_writeb(bm, val);
  break;
-case 0x0a:
-val = d->i.bmdma[1].status;
+case 0x01:
+d->regs[i].swdata = val & 0x3f;
  break;
-case 0x0b:
-val = 0;
+case 0x02:
+bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6);
  break;
-case 0x0c ... 0x0f:
-val = bmdma_addr_ioport_ops.read(>i.bmdma[1], addr - 12, size);
+default:
  break;
+}
+}
+
+static const MemoryRegionOps sii3112_bmdma_ops = {
+.read = sii3112_bmdma_read,
+.write = sii3112_bmdma_write,
+};
+
+/* The sii3112_reg_read and sii3112_reg_write functions implement the
+ * Internal Register Space - BAR5 (section 6.7 of the data sheet).
+ */
+
+static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
+unsigned int size)
+{
+SiI3112PCIState *d = opaque;
+uint64_t val;
+
+switch (addr) {
  case 0x10:
  val = d->i.bmdma[0].cmd;
  val |= (d->regs[0].confstat & (1UL << 11) ? (1 << 4) : 0); 
/*SATAINT0*/
@@ -127,38 +153,26 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  
  trace_sii3112_write(size, addr, val);

  switch (addr) {
-case 0x00:
  case 0x10:
  bmdma_cmd_writeb(>i.bmdma[0], val);
  break;
-case 0x01:
  case 0x11:
  d->regs[0].swdata = val & 0x3f;
  break;
-case 0x02:
  case 0x12:
  d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) |
 (d->i.bmdma[0].status & ~val & 6);
  break;
-case 0x04 ... 0x07:
-bmdma_addr_ioport_ops.write(>i.bmdma[0], addr - 4, val, size);
-break;
-case 

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  
-extern const MemoryRegionOps pci_ide_cmd_le_ops;

-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  
-const MemoryRegionOps pci_ide_cmd_le_ops = {

+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  
-const MemoryRegionOps pci_ide_data_le_ops = {

+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  
+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);

+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  
  /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */

  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >mmio, 0xc0, 8);
-pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >data_ops[1], 0,
+ 

Re: [PATCH 10/13] hw/ide/piix: Reuse PCIIDEState::{cmd,data}_ops

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Now that PCIIDEState::{cmd,data}_ops are initialized in the base class
constructor there is an opportunity for PIIX to reuse these attributes. This
resolves usage of ide_init_ioport() which would fall back internally to using
the isabus global due to NULL being passed as ISADevice by PIIX.

Signed-off-by: Bernhard Beschow 
---
  hw/ide/piix.c | 30 +-
  1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a3a15dc7db..406a67fa0f 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -104,34 +104,32 @@ static void piix_ide_reset(DeviceState *dev)
  pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
  }
  
-static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,

-  Error **errp)
+static void pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus)
  {
  static const struct {
  int iobase;
  int iobase2;
  int isairq;
  } port_info[] = {
-{0x1f0, 0x3f6, 14},
-{0x170, 0x376, 15},
+{0x1f0, 0x3f4, 14},
+{0x170, 0x374, 15},
  };
-int ret;
+MemoryRegion *address_space_io = pci_address_space_io(PCI_DEVICE(d));
  
  ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);

-ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
-  port_info[i].iobase2);
-if (ret) {
-error_setg_errno(errp, -ret, "Failed to realize %s port %u",
- object_get_typename(OBJECT(d)), i);
-return false;
-}
+memory_region_add_subregion(address_space_io, port_info[i].iobase,
+>data_ops[i]);
+/*
+ * PIIX forwards the last byte of cmd_ops to ISA. Model this using a low
+ * prio so competing memory regions take precedence.
+ */
+memory_region_add_subregion_overlap(address_space_io, port_info[i].iobase2,
+>cmd_ops[i], -1);


Interesting. Is this behaviour documented somewhere and/or used in one of your test 
images at all? If I'd have seen this myself, I probably thought that the addresses 
were a typo...



  ide_bus_init_output_irq(>bus[i],
  isa_bus_get_irq(isa_bus, port_info[i].isairq));
  
  bmdma_init(>bus[i], >bmdma[i], d);

  ide_bus_register_restart_cb(>bus[i]);
-
-return true;
  }
  
  static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)

@@ -160,9 +158,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  }
  
  for (unsigned i = 0; i < 2; i++) {

-if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
-return;
-}
+pci_piix_init_bus(d, i, isa_bus);
  }
  }
  



ATB,

Mark.

Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq()

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
Passing a NULL pointer works but causes the isabus global to be used
then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
achieve the same as using isa_get_irq().

This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
crash when plugging a piix3-ide device into the x-remote machine' which
allows for cleaning up the ISA API while keeping PIIX IDE functions
user-createable.

Signed-off-by: Bernhard Beschow 
---
  hw/ide/piix.c | 23 ---
  1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 6942b484f9..a3a15dc7db 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -104,7 +104,8 @@ static void piix_ide_reset(DeviceState *dev)
  pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
  }
  
-static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)

+static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
+  Error **errp)
  {
  static const struct {
  int iobase;
@@ -124,7 +125,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
   object_get_typename(OBJECT(d)), i);
  return false;
  }
-ide_bus_init_output_irq(>bus[i], isa_get_irq(NULL, 
port_info[i].isairq));
+ide_bus_init_output_irq(>bus[i],
+isa_bus_get_irq(isa_bus, port_info[i].isairq));


I don't think is the right solution here, since ultimately we want to move the IRQ 
routing out of the device itself and into the PCI-ISA bridge. I'd go for the same 
solution as you've done for VIA IDE in patch 2, i.e. update the PIIX interrupt 
handler to set the legacy_irqs in PCIIDEState, and then wire them up to the ISA IRQs 
14 and 15 similar to as Phil as done in his patches:


https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-4-phi...@linaro.org/

https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-5-phi...@linaro.org/

This also reminds me, given that the first patch above is doing wiring in pc_init1() 
then we are still missing part of your tidy-up series :/



  bmdma_init(>bus[i], >bmdma[i], d);
  ide_bus_register_restart_cb(>bus[i]);
@@ -136,14 +138,29 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  {
  PCIIDEState *d = PCI_IDE(dev);
  uint8_t *pci_conf = dev->config;
+ISABus *isa_bus;
+bool ambiguous;
  
  pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
  
  bmdma_init_ops(d, _bmdma_ops);

  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_ops);
  
+isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS, ));

+if (ambiguous) {
+error_setg(errp,
+   "More than one ISA bus found while %s supports only one",
+   object_get_typename(OBJECT(d)));
+return;
+}
+if (!isa_bus) {
+error_setg(errp, "No ISA bus found while %s requires one",
+   object_get_typename(OBJECT(d)));
+return;
+}


Again I think this should go away with using PCIIDEState's legacy_irqs, since you 
simply let the board wire them up to the ISABus (or not) as required.



  for (unsigned i = 0; i < 2; i++) {
-if (!pci_piix_init_bus(d, i, errp)) {
+if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
  return;
  }
  }



ATB,

Mark.

Re: [PATCH 08/13] hw/ide: Rename PCIIDEState::*_bar attributes

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


The attributes represent memory regions containing operations which are mapped
by the device models into PCI BARs. Reflect this by changing the suffic into
"_ops".

Note that in a few commits piix will also use the {cmd,data}_ops but won't map
them into BARs. This further suggests that the "_bar" suffix doesn't match
very well.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  6 +++---
  hw/ide/cmd646.c  | 10 +-
  hw/ide/pci.c | 18 +-
  hw/ide/piix.c|  2 +-
  hw/ide/via.c | 10 +-
  5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 597c77c7ad..5025df5b82 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -51,9 +51,9 @@ struct PCIIDEState {
  BMDMAState bmdma[2];
  qemu_irq isa_irq[2];
  uint32_t secondary; /* used only for cmd646 */
-MemoryRegion bmdma_bar;
-MemoryRegion cmd_bar[2];
-MemoryRegion data_bar[2];
+MemoryRegion bmdma_ops;
+MemoryRegion cmd_ops[2];
+MemoryRegion data_ops[2];
  };
  
  void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 85716aaf17..b9d005a357 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
  dev->wmask[MRDMODE] = 0x0;
  dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
  
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[0]);

-pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[0]);
-pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[1]);
-pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[1]);
+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
  
  bmdma_init_ops(d, _bmdma_ops);

-pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
+pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_ops);
  
  /* TODO: RST# value should be 0 */

  pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a9194313bd..b2fcc00a64 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps 
*bmdma_ops)
  {
  size_t i;
  
-memory_region_init(>bmdma_bar, OBJECT(d), "bmdma-container", 16);

+memory_region_init(>bmdma_ops, OBJECT(d), "bmdma-container", 16);
  for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) {
  BMDMAState *bm = >bmdma[i];
  
  memory_region_init_io(>extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4);

-memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
+memory_region_add_subregion(>bmdma_ops, i * 8, >extra_io);
  memory_region_init_io(>addr_ioport, OBJECT(d), 
_addr_ioport_ops, bm,
"bmdma-ioport-ops", 4);
-memory_region_add_subregion(>bmdma_bar, i * 8 + 4, 
>addr_ioport);
+memory_region_add_subregion(>bmdma_ops, i * 8 + 4, 
>addr_ioport);
  }
  }
  
@@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj)

  {
  PCIIDEState *d = PCI_IDE(obj);
  
-memory_region_init_io(>data_bar[0], OBJECT(d), _ide_data_le_ops,

+memory_region_init_io(>data_ops[0], OBJECT(d), _ide_data_le_ops,
>bus[0], "pci-ide0-data-ops", 8);
-memory_region_init_io(>cmd_bar[0], OBJECT(d), _ide_cmd_le_ops,
+memory_region_init_io(>cmd_ops[0], OBJECT(d), _ide_cmd_le_ops,
>bus[0], "pci-ide0-cmd-ops", 4);
  
-memory_region_init_io(>data_bar[1], OBJECT(d), _ide_data_le_ops,

+memory_region_init_io(>data_ops[1], OBJECT(d), _ide_data_le_ops,
>bus[1], "pci-ide1-data-ops", 8);
-memory_region_init_io(>cmd_bar[1], OBJECT(d), _ide_cmd_le_ops,
+memory_region_init_io(>cmd_ops[1], OBJECT(d), _ide_cmd_le_ops,
>bus[1], "pci-ide1-cmd-ops", 4);
  
  qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));

@@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev)
  unsigned i;
  
  for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {

-memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].addr_ioport);
+memory_region_del_subregion(>bmdma_ops, >bmdma[i].extra_io);
+memory_region_del_subregion(>bmdma_ops, >bmdma[i].addr_ioport);
  }
  }
  
diff --git a/hw/ide/piix.c b/hw/ide/piix.c

index 5611473d37..6942b484f9 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -140,7 +140,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 

Re: [PATCH 07/13] hw/ide: Extract pci_ide_{cmd, data}_le_ops initialization into base class constructor

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


There is redundant code in cmd646 and via which can be extracted into the base
class. In case of piix and sii3112 this is currently unneccessary but shouldn't
interfere since the memory regions aren't mapped by those devices. In few
commits later this will be changed, i.e. those device models will also make use
of these memory regions.

Signed-off-by: Bernhard Beschow 
---
  hw/ide/cmd646.c | 11 ---
  hw/ide/pci.c| 10 ++
  hw/ide/via.c| 11 ---
  3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 6fd09fe74e..85716aaf17 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -251,20 +251,9 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
  dev->wmask[MRDMODE] = 0x0;
  dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
  
-memory_region_init_io(>data_bar[0], OBJECT(d), _ide_data_le_ops,

-  >bus[0], "cmd646-data0", 8);
  pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[0]);
-
-memory_region_init_io(>cmd_bar[0], OBJECT(d), _ide_cmd_le_ops,
-  >bus[0], "cmd646-cmd0", 4);
  pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[0]);
-
-memory_region_init_io(>data_bar[1], OBJECT(d), _ide_data_le_ops,
-  >bus[1], "cmd646-data1", 8);
  pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[1]);
-
-memory_region_init_io(>cmd_bar[1], OBJECT(d), _ide_cmd_le_ops,
-  >bus[1], "cmd646-cmd1", 4);
  pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[1]);
  
  bmdma_init_ops(d, _bmdma_ops);

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 65ed6f7f72..a9194313bd 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -543,6 +543,16 @@ static void pci_ide_init(Object *obj)
  {
  PCIIDEState *d = PCI_IDE(obj);
  
+memory_region_init_io(>data_bar[0], OBJECT(d), _ide_data_le_ops,

+  >bus[0], "pci-ide0-data-ops", 8);
+memory_region_init_io(>cmd_bar[0], OBJECT(d), _ide_cmd_le_ops,
+  >bus[0], "pci-ide0-cmd-ops", 4);
+
+memory_region_init_io(>data_bar[1], OBJECT(d), _ide_data_le_ops,
+  >bus[1], "pci-ide1-data-ops", 8);
+memory_region_init_io(>cmd_bar[1], OBJECT(d), _ide_cmd_le_ops,
+  >bus[1], "pci-ide1-cmd-ops", 4);
+
  qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
  }
  
diff --git a/hw/ide/via.c b/hw/ide/via.c

index 40704e2857..704a8024cb 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -154,20 +154,9 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
  dev->wmask[PCI_INTERRUPT_LINE] = 0;
  dev->wmask[PCI_CLASS_PROG] = 5;
  
-memory_region_init_io(>data_bar[0], OBJECT(d), _ide_data_le_ops,

-  >bus[0], "via-ide0-data", 8);
  pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[0]);
-
-memory_region_init_io(>cmd_bar[0], OBJECT(d), _ide_cmd_le_ops,
-  >bus[0], "via-ide0-cmd", 4);
  pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[0]);
-
-memory_region_init_io(>data_bar[1], OBJECT(d), _ide_data_le_ops,
-  >bus[1], "via-ide1-data", 8);
  pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[1]);
-
-memory_region_init_io(>cmd_bar[1], OBJECT(d), _ide_cmd_le_ops,
-  >bus[1], "via-ide1-cmd", 4);
  pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[1]);
  
  bmdma_init_ops(d, _bmdma_ops);


I'd also be inclined to agree with Phil/Zoltan re: dropping the trailing "-ops" in 
the name, otherwise:


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 06/13] hw/ide: Extract bmdma_init_ops()

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


There are three private copies of bmdma_setup_bar() with small adaptions.
Consolidate them into one public implementation.

While at it rename the function to bmdma_init_ops() to reflect that the memory
regions being initialized represent BMDMA operations. The actual mapping as a
PCI BAR is still performed separately in each device.

Note that the bmdma_bar attribute will be renamed in a separate commit.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  1 +
  hw/ide/cmd646.c  | 20 +---
  hw/ide/pci.c | 16 
  hw/ide/piix.c| 19 +--
  hw/ide/via.c | 19 +--
  5 files changed, 20 insertions(+), 55 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 7bc4e53d02..597c77c7ad 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -57,6 +57,7 @@ struct PCIIDEState {
  };
  
  void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);

+void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
  void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 9aabf80e52..6fd09fe74e 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -161,24 +161,6 @@ static const MemoryRegionOps cmd646_bmdma_ops = {
  .write = bmdma_write,
  };
  
-static void bmdma_setup_bar(PCIIDEState *d)

-{
-BMDMAState *bm;
-int i;
-
-memory_region_init(>bmdma_bar, OBJECT(d), "cmd646-bmdma", 16);
-for(i = 0;i < 2; i++) {
-bm = >bmdma[i];
-memory_region_init_io(>extra_io, OBJECT(d), _bmdma_ops, bm,
-  "cmd646-bmdma-bus", 4);
-memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
-memory_region_init_io(>addr_ioport, OBJECT(d),
-  _addr_ioport_ops, bm,
-  "cmd646-bmdma-ioport", 4);
-memory_region_add_subregion(>bmdma_bar, i * 8 + 4, 
>addr_ioport);
-}
-}
-
  static void cmd646_update_irq(PCIDevice *pd)
  {
  int pci_level;
@@ -285,7 +267,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
>bus[1], "cmd646-cmd1", 4);
  pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_bar[1]);
  
-bmdma_setup_bar(d);

+bmdma_init_ops(d, _bmdma_ops);
  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
  
  /* TODO: RST# value should be 0 */

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 8bea92e394..65ed6f7f72 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -523,6 +523,22 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState 
*d)
  bm->pci_dev = d;
  }
  
+void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops)

+{
+size_t i;
+
+memory_region_init(>bmdma_bar, OBJECT(d), "bmdma-container", 16);
+for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) {
+BMDMAState *bm = >bmdma[i];
+
+memory_region_init_io(>extra_io, OBJECT(d), bmdma_ops, bm, 
"bmdma-ops", 4);
+memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
+memory_region_init_io(>addr_ioport, OBJECT(d), 
_addr_ioport_ops, bm,
+  "bmdma-ioport-ops", 4);
+memory_region_add_subregion(>bmdma_bar, i * 8 + 4, 
>addr_ioport);
+}
+}
+
  static void pci_ide_init(Object *obj)
  {
  PCIIDEState *d = PCI_IDE(obj);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 4e6ca99123..5611473d37 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -86,23 +86,6 @@ static const MemoryRegionOps piix_bmdma_ops = {
  .write = bmdma_write,
  };
  
-static void bmdma_setup_bar(PCIIDEState *d)

-{
-int i;
-
-memory_region_init(>bmdma_bar, OBJECT(d), "piix-bmdma-container", 16);
-for(i = 0;i < 2; i++) {
-BMDMAState *bm = >bmdma[i];
-
-memory_region_init_io(>extra_io, OBJECT(d), _bmdma_ops, bm,
-  "piix-bmdma", 4);
-memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
-memory_region_init_io(>addr_ioport, OBJECT(d),
-  _addr_ioport_ops, bm, "bmdma", 4);
-memory_region_add_subregion(>bmdma_bar, i * 8 + 4, 
>addr_ioport);
-}
-}
-
  static void piix_ide_reset(DeviceState *dev)
  {
  PCIIDEState *d = PCI_IDE(dev);
@@ -156,7 +139,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  
  pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
  
-bmdma_setup_bar(d);

+bmdma_init_ops(d, _bmdma_ops);
  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
  
  for (unsigned i = 0; i < 2; i++) {

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 287143a005..40704e2857 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -86,23 +86,6 @@ static const MemoryRegionOps via_bmdma_ops = {
  .write = bmdma_write,
  };
  

Re: [PULL 00/25] Block layer patches

[Richard Henderson]

On 4/25/23 14:13, Kevin Wolf wrote:

The following changes since commit ac5f7bf8e208cd7893dbb1a9520559e569a4677c:

   Merge tag 'migration-20230424-pull-request' 
ofhttps://gitlab.com/juan.quintela/qemu  into staging (2023-04-24 15:00:39 
+0100)

are available in the Git repository at:

   https://repo.or.cz/qemu/kevin.git  tags/for-upstream

for you to fetch changes up to 8c1e8fb2e7fc2cbeb57703e143965a4cd3ad301a:

   block/monitor: Fix crash when executing HMP commit (2023-04-25 15:11:57 
+0200)


Block layer patches

- Protect BlockBackend.queued_requests with its own lock
- Switch to AIO_WAIT_WHILE_UNLOCKED() where possible
- AioContext removal: LinuxAioState/LuringState/ThreadPool
- Add more coroutine_fn annotations, use bdrv/blk_co_*
- Fix crash when execute hmp_commit


Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as 
appropriate.


r~

Re: [PATCH 05/13] hw/ide: Extract pci_ide_class_init()

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Resolves redundant code in every PCI IDE device model.


I think this needs to mention that it's moving the PCIDeviceClass::exit() function 
from all of the PCI IDE controller implementations to a common implementation in the 
parent PCI_IDE type.



---
  include/hw/ide/pci.h |  1 -
  hw/ide/cmd646.c  | 15 ---
  hw/ide/pci.c | 25 -
  hw/ide/piix.c| 19 ---
  hw/ide/sii3112.c |  3 ++-
  hw/ide/via.c | 15 ---
  6 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 74c127e32f..7bc4e53d02 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -61,7 +61,6 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  
-extern const VMStateDescription vmstate_ide_pci;

  extern const MemoryRegionOps pci_ide_cmd_le_ops;
  extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a094a6e12a..9aabf80e52 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -301,17 +301,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
  }
  }
  
-static void pci_cmd646_ide_exitfn(PCIDevice *dev)

-{
-PCIIDEState *d = PCI_IDE(dev);
-unsigned i;
-
-for (i = 0; i < 2; ++i) {
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].addr_ioport);
-}
-}
-
  static Property cmd646_ide_properties[] = {
  DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
  DEFINE_PROP_END_OF_LIST(),
@@ -323,17 +312,13 @@ static void cmd646_ide_class_init(ObjectClass *klass, 
void *data)
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
  dc->reset = cmd646_reset;

-dc->vmsd = _ide_pci;
  k->realize = pci_cmd646_ide_realize;
-k->exit = pci_cmd646_ide_exitfn;
  k->vendor_id = PCI_VENDOR_ID_CMD;
  k->device_id = PCI_DEVICE_ID_CMD_646;
  k->revision = 0x07;
-k->class_id = PCI_CLASS_STORAGE_IDE;
  k->config_read = cmd646_pci_config_read;
  k->config_write = cmd646_pci_config_write;
  device_class_set_props(dc, cmd646_ide_properties);
-set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
  }
  
  static const TypeInfo cmd646_ide_info = {

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 67e0998ff0..8bea92e394 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -467,7 +467,7 @@ static int ide_pci_post_load(void *opaque, int version_id)
  return 0;
  }
  
-const VMStateDescription vmstate_ide_pci = {

+static const VMStateDescription vmstate_ide_pci = {
  .name = "ide",
  .version_id = 3,
  .minimum_version_id = 0,
@@ -530,11 +530,34 @@ static void pci_ide_init(Object *obj)
  qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
  }
  
+static void pci_ide_exitfn(PCIDevice *dev)

+{
+PCIIDEState *d = PCI_IDE(dev);
+unsigned i;
+
+for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
+memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
+memory_region_del_subregion(>bmdma_bar, >bmdma[i].addr_ioport);
+}
+}
+
+static void pci_ide_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+dc->vmsd = _ide_pci;
+k->exit = pci_ide_exitfn;
+k->class_id = PCI_CLASS_STORAGE_IDE;
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
  static const TypeInfo pci_ide_type_info = {
  .name = TYPE_PCI_IDE,
  .parent = TYPE_PCI_DEVICE,
  .instance_size = sizeof(PCIIDEState),
  .instance_init = pci_ide_init,
+.class_init = pci_ide_class_init,
  .abstract = true,
  .interfaces = (InterfaceInfo[]) {
  { INTERFACE_CONVENTIONAL_PCI_DEVICE },
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a32f7ccece..4e6ca99123 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -159,8 +159,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  bmdma_setup_bar(d);
  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
  
-vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);

-


Presumably this still survives migration between a pre-series and post-series QEMU 
using the PIIX IDE controller?



  for (unsigned i = 0; i < 2; i++) {
  if (!pci_piix_init_bus(d, i, errp)) {
  return;
@@ -168,17 +166,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  }
  }
  
-static void pci_piix_ide_exitfn(PCIDevice *dev)

-{
-PCIIDEState *d = PCI_IDE(dev);
-unsigned i;
-
-for (i = 0; i < 2; ++i) {
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].addr_ioport);
-}
-}
-
  /* NOTE: for the PIIX3, the IRQs and IOports are 

Re: [PATCH 04/13] hw/ide: Extract IDEBus assignment into bmdma_init()

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Every invocation of bmdma_init() is followed by `d->bmdma[i].bus = >bus[i]`.
Resolve this redundancy by extracting it into bmdma_init().

Signed-off-by: Bernhard Beschow 
---
  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 -
  5 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a68357c1c5..a094a6e12a 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -297,7 +297,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
  ide_bus_init_output_irq(>bus[i], qdev_get_gpio_in(ds, i));
  
  bmdma_init(>bus[i], >bmdma[i], d);

-d->bmdma[i].bus = >bus[i];
  ide_bus_register_restart_cb(>bus[i]);
  }
  }
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 942e216b9b..67e0998ff0 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -519,6 +519,7 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
  bus->dma = >dma;
  bm->irq = bus->irq;
  bus->irq = qemu_allocate_irq(bmdma_irq, bm, 0);
+bm->bus = bus;
  bm->pci_dev = d;
  }
  
diff --git a/hw/ide/piix.c b/hw/ide/piix.c

index 41d60921e3..a32f7ccece 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -144,7 +144,6 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
  ide_bus_init_output_irq(>bus[i], isa_get_irq(NULL, 
port_info[i].isairq));
  
  bmdma_init(>bus[i], >bmdma[i], d);

-d->bmdma[i].bus = >bus[i];
  ide_bus_register_restart_cb(>bus[i]);
  
  return true;

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index f9becdff8e..5dd3d03c29 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -287,7 +287,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  ide_bus_init_output_irq(>bus[i], qdev_get_gpio_in(ds, i));
  
  bmdma_init(>bus[i], >bmdma[i], s);

-s->bmdma[i].bus = >bus[i];
  ide_bus_register_restart_cb(>bus[i]);
  }
  }
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 0caae52276..91253fa4ef 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -196,7 +196,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
  ide_bus_init_output_irq(>bus[i], qdev_get_gpio_in(ds, i));
  
  bmdma_init(>bus[i], >bmdma[i], d);

-d->bmdma[i].bus = >bus[i];
  ide_bus_register_restart_cb(>bus[i]);
  }
  }


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 03/13] hw/isa/vt82c686: Remove via_isa_set_irq()

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Now that via_isa_set_irq() is unused it can be removed.

Signed-off-by: Bernhard Beschow 
---
  include/hw/isa/vt82c686.h | 2 --
  hw/isa/vt82c686.c | 6 --
  2 files changed, 8 deletions(-)

diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index da1722daf2..b6e95b2851 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -34,6 +34,4 @@ struct ViaAC97State {
  uint32_t ac97_cmd;
  };
  
-void via_isa_set_irq(PCIDevice *d, int n, int level);

-
  #endif
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index c7e29bb46a..a69888a396 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -604,12 +604,6 @@ static const TypeInfo via_isa_info = {
  },
  };
  
-void via_isa_set_irq(PCIDevice *d, int n, int level)

-{
-ViaISAState *s = VIA_ISA(d);
-qemu_set_irq(s->isa_irqs_in[n], level);
-}
-
  static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
  {
  ViaISAState *s = opaque;


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 02/13] hw/ide/via: Implement ISA IRQ routing

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


The VIA south bridge allows the legacy IDE interrupts to be routed to four
different ISA interrupts. This can be configured through the 0x4a register in
the PCI configuration space of the ISA function. The default routing matches
the legacy ISA IRQs, that is 14 and 15.

Implement this missing piece of the VIA south bridge.

Signed-off-by: Bernhard Beschow 
---
  hw/ide/via.c  |  6 --
  hw/isa/vt82c686.c | 17 +
  2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 177baea9a7..0caae52276 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -31,6 +31,7 @@
  #include "sysemu/dma.h"
  #include "hw/isa/vt82c686.h"
  #include "hw/ide/pci.h"
+#include "hw/irq.h"
  #include "trace.h"
  
  static uint64_t bmdma_read(void *opaque, hwaddr addr,

@@ -104,7 +105,8 @@ static void bmdma_setup_bar(PCIIDEState *d)
  
  static void via_ide_set_irq(void *opaque, int n, int level)

  {
-PCIDevice *d = PCI_DEVICE(opaque);
+PCIIDEState *s = opaque;
+PCIDevice *d = PCI_DEVICE(s);
  
  if (level) {

  d->config[0x70 + n * 8] |= 0x80;
@@ -112,7 +114,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
  d->config[0x70 + n * 8] &= ~0x80;
  }
  
-via_isa_set_irq(pci_get_function_0(d), 14 + n, level);

+qemu_set_irq(s->isa_irq[n], level);
  }
  
  static void via_ide_reset(DeviceState *dev)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index ca89119ce0..c7e29bb46a 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -568,9 +568,19 @@ static const VMStateDescription vmstate_via = {
  }
  };
  
+static void via_isa_set_ide_irq(void *opaque, int n, int level)

+{
+static const uint8_t irqs[] = { 14, 15, 10, 11 };
+ViaISAState *s = opaque;
+uint8_t irq = irqs[(s->dev.config[0x4a] >> (n * 2)) & 0x3];
+
+qemu_set_irq(s->isa_irqs_in[irq], level);
+}
+
  static void via_isa_init(Object *obj)
  {
  ViaISAState *s = VIA_ISA(obj);
+DeviceState *dev = DEVICE(s);
  
  object_initialize_child(obj, "rtc", >rtc, TYPE_MC146818_RTC);

  object_initialize_child(obj, "ide", >ide, TYPE_VIA_IDE);
@@ -578,6 +588,8 @@ static void via_isa_init(Object *obj)
  object_initialize_child(obj, "uhci2", >uhci[1], 
TYPE_VT82C686B_USB_UHCI);
  object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
  object_initialize_child(obj, "mc97", >mc97, TYPE_VIA_MC97);
+
+qdev_init_gpio_in_named(dev, via_isa_set_ide_irq, "ide", 
ARRAY_SIZE(s->ide.isa_irq));
  }
  
  static const TypeInfo via_isa_info = {

@@ -692,6 +704,10 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
  if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
  return;
  }
+for (i = 0; i < 2; i++) {
+qdev_connect_gpio_out(DEVICE(>ide), i,
+  qdev_get_gpio_in_named(DEVICE(s), "ide", i));
+}
  
  /* Functions 2-3: USB Ports */

  for (i = 0; i < ARRAY_SIZE(s->uhci); i++) {
@@ -814,6 +830,7 @@ static void vt8231_isa_reset(DeviceState *dev)
   PCI_COMMAND_MASTER | PCI_COMMAND_SPECIAL);
  pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
  
+pci_conf[0x4a] = 0x04; /* IDE interrupt Routing */

  pci_conf[0x58] = 0x40; /* Miscellaneous Control 0 */
  pci_conf[0x67] = 0x08; /* Fast IR Config */
  pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */


I see there is still some further discussion on the exact datasheet being used, 
however the basic mechanism of wiring up the IDE IRQs using 
qdev_connect_gpio_out{_named}() in via_isa_realize():


Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH 01/13] hw/ide/pci: Expose legacy interrupts as GPIOs

[Mark Cave-Ayland]

On 22/04/2023 16:07, Bernhard Beschow wrote:


Exposing the legacy IDE interrupts as GPIOs allows them to be connected in the
parent device through qdev_connect_gpio_out(), i.e. without accessing private
data of TYPE_PCI_IDE.

Signed-off-by: Bernhard Beschow 
---
  hw/ide/pci.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index fc9224bbc9..942e216b9b 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -522,10 +522,18 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState 
*d)
  bm->pci_dev = d;
  }
  
+static void pci_ide_init(Object *obj)

+{
+PCIIDEState *d = PCI_IDE(obj);
+
+qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));


Just one minor nit: can we make this qdev_init_gpio_out_named() and call it "isa-irq" 
to match? This is for 2 reasons: firstly these are PCI devices and so an unnamed 
IRQ/gpio could be considered to belong to PCI, and secondly it gives the gpio the 
same name as the struct field.


From my previous email I think this should supercede Phil's patch at 
https://patchew.org/QEMU/20230302224058.43315-1-phi...@linaro.org/20230302224058.43315-2-phi...@linaro.org/.



+}
+
  static const TypeInfo pci_ide_type_info = {
  .name = TYPE_PCI_IDE,
  .parent = TYPE_PCI_DEVICE,
  .instance_size = sizeof(PCIIDEState),
+.instance_init = pci_ide_init,
  .abstract = true,
  .interfaces = (InterfaceInfo[]) {
  { INTERFACE_CONVENTIONAL_PCI_DEVICE },


Otherwise:

Reviewed-by: Mark Cave-Ayland 


ATB,

Mark.

Re: [PATCH v3 01/18] hw/ide/piix: Expose output IRQ as properties for late object population

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:


Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/ide/piix.c | 14 --
  include/hw/ide/piix.h |  4 
  2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 41d60921e3..a36dac8469 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -121,6 +121,13 @@ static void piix_ide_reset(DeviceState *dev)
  pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
  }
  
+static void piix_ide_initfn(Object *obj)

+{
+PCIIDEState *dev = PCI_IDE(obj);
+
+qdev_init_gpio_out_named(DEVICE(obj), dev->isa_irq, "ide-irq", 2);
+}
+
  static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
  {
  static const struct {
@@ -133,6 +140,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
  };
  int ret;
  
+qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);

  ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
  ret = ide_init_ioport(>bus[i], NULL, port_info[i].iobase,
port_info[i].iobase2);
@@ -141,7 +149,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, 
Error **errp)
   object_get_typename(OBJECT(d)), i);
  return false;
  }
-ide_bus_init_output_irq(>bus[i], isa_get_irq(NULL, 
port_info[i].isairq));
+ide_bus_init_output_irq(>bus[i], irq_out);
  
  bmdma_init(>bus[i], >bmdma[i], d);

  d->bmdma[i].bus = >bus[i];
@@ -162,7 +170,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
  
  vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);
  
-for (unsigned i = 0; i < 2; i++) {

+for (unsigned i = 0; i < ARRAY_SIZE(d->isa_irq); i++) {
  if (!pci_piix_init_bus(d, i, errp)) {
  return;
  }
@@ -199,6 +207,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void 
*data)
  static const TypeInfo piix3_ide_info = {
  .name  = TYPE_PIIX3_IDE,
  .parent= TYPE_PCI_IDE,
+.instance_init = piix_ide_initfn,
  .class_init= piix3_ide_class_init,
  };
  
@@ -221,6 +230,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)

  static const TypeInfo piix4_ide_info = {
  .name  = TYPE_PIIX4_IDE,
  .parent= TYPE_PCI_IDE,
+.instance_init = piix_ide_initfn,
  .class_init= piix4_ide_class_init,
  };
  
diff --git a/include/hw/ide/piix.h b/include/hw/ide/piix.h

index ef3ef3d62d..533d24d408 100644
--- a/include/hw/ide/piix.h
+++ b/include/hw/ide/piix.h
@@ -1,6 +1,10 @@
  #ifndef HW_IDE_PIIX_H
  #define HW_IDE_PIIX_H
  
+/*

+ * QEMU interface:
+ *  + named GPIO outputs "ide-irq": asserted by each IDE channel
+ */
  #define TYPE_PIIX3_IDE "piix3-ide"
  #define TYPE_PIIX4_IDE "piix4-ide"


Comparing this with Bernhard's latest series, Bernhard's patch at 
https://patchew.org/QEMU/20230422150728.176512-1-shen...@gmail.com/20230422150728.176512-2-shen...@gmail.com/ 
(with a small change) is the version we should use, since legacy IRQs are a feature 
of all PCI IDE controllers and not just the PIIX controllers.


If we do it this way then it is possible for all PCI IDE controllers to share the 
same logic for BDMA and legacy/native mode switching moving forward: if a PCI IDE 
controller doesn't implement legacy IRQ routing then the board can leave the IRQs 
disconnected.



ATB,

Mark.

Re: [PATCH v2 2/6] tests/qtests: remove migration test iterations config

[Daniel P . Berrangé]

On Wed, Apr 26, 2023 at 11:42:51AM +0200, Juan Quintela wrote:
> Daniel P. Berrangé  wrote:
> > On Fri, Apr 21, 2023 at 11:54:55PM +0200, Juan Quintela wrote:
> >> Daniel P. Berrangé  wrote:
> >> > The 'unsigned int interations' config for migration is somewhat
> >> > overkill. Most tests don't set it, and a value of '0' is treated
> >> > as equivalent to '1'. The only test that does set it, xbzrle,
> >> > used a value of '2'.
> >> >
> >> > This setting, however, only relates to the migration iterations
> >> > that take place prior to allowing convergence. IOW, on top of
> >> > this iteration count, there is always at least 1 further migration
> >> > iteration done to deal with pages that are dirtied during the
> >> > previous iteration(s).
> >> >
> >> > IOW, even with iterations==1, the xbzrle test will be running for
> >> > a minimum of 2 iterations. With this in mind we can simplify the
> >> > code and just get rid of the special case.
> >> 
> >> Perhaps the old code was already wrong, but we need at least three
> >> iterations for the xbzrle test:
> >> - 1st iteration: xbzrle is not used, nothing is on cache.
> >
> > Are you sure about this ?  I see ram_save_page() calling
> > save_xbzrle_page() and unless I'm mis-understanding the
> > code, it doesn't appear to skip anything on the 1st
> > iteration.
> 
> I will admit that code is convoluted as hell.
> And I confuse myself a lot here O:-)
> 
> struct RAM_STATE {
> ...
> /* Start using XBZRLE (e.g., after the first round). */
> bool xbzrle_enabled;
> }
> 
> I.e. xbzrle_enabled() and m->xbzrle_enabled are two completely different 
> things.

Aie !  That's confusing indeed :-)

Lets rename that struct field to 'xbzrle_started', to better
distinguish active state from enabled state.


> static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
> {
> ...
> if (rs->xbzrle_enabled && !migration_in_postcopy()) {
> pages = save_xbzrle_page(rs, pss, , current_addr,
>  block, offset);
> 
> }
> 
> }
> 
> and
> 
> static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
> {
> /* Update pss->page for the next dirty bit in ramblock */
> pss_find_next_dirty(pss);
> 
> if (pss->complete_round && pss->block == rs->last_seen_block &&
> ...
> return PAGE_ALL_CLEAN;
> }
> if (!offset_in_ramblock(pss->block,
> ((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {
> 
> if (!pss->block) {
> 
> if (migrate_use_xbzrle()) {
> rs->xbzrle_enabled = true;
> }
> }
> ...
> } else {
> /* We've found something */
> return PAGE_DIRTY_FOUND;
> }
> }
> 
> 
> 
> > IIUC save_xbzrle_page will add pages into the cache on
> > the first iteration, so the second iteration will get
> > cache hits
> >
> >> - 2nd iteration: pages are put into cache, no xbzrle is used because
> >>   there is no previous page.
> >> - 3rd iteration: We really use xbzrle now against the copy of the
> >>   previous iterations.
> >> 
> >> And yes, this should be commented somewhere.
> 
> Seeing that it has been able to confuse you, a single comment will not
> make the trick O:-)
> 
> Later, Juan.
> 

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 v2 2/6] tests/qtests: remove migration test iterations config

[Juan Quintela]

Daniel P. Berrangé  wrote:
> On Fri, Apr 21, 2023 at 11:54:55PM +0200, Juan Quintela wrote:
>> Daniel P. Berrangé  wrote:
>> > The 'unsigned int interations' config for migration is somewhat
>> > overkill. Most tests don't set it, and a value of '0' is treated
>> > as equivalent to '1'. The only test that does set it, xbzrle,
>> > used a value of '2'.
>> >
>> > This setting, however, only relates to the migration iterations
>> > that take place prior to allowing convergence. IOW, on top of
>> > this iteration count, there is always at least 1 further migration
>> > iteration done to deal with pages that are dirtied during the
>> > previous iteration(s).
>> >
>> > IOW, even with iterations==1, the xbzrle test will be running for
>> > a minimum of 2 iterations. With this in mind we can simplify the
>> > code and just get rid of the special case.
>> 
>> Perhaps the old code was already wrong, but we need at least three
>> iterations for the xbzrle test:
>> - 1st iteration: xbzrle is not used, nothing is on cache.
>
> Are you sure about this ?  I see ram_save_page() calling
> save_xbzrle_page() and unless I'm mis-understanding the
> code, it doesn't appear to skip anything on the 1st
> iteration.

I will admit that code is convoluted as hell.
And I confuse myself a lot here O:-)

struct RAM_STATE {
...
/* Start using XBZRLE (e.g., after the first round). */
bool xbzrle_enabled;
}

I.e. xbzrle_enabled() and m->xbzrle_enabled are two completely different things.

static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
{
...
if (rs->xbzrle_enabled && !migration_in_postcopy()) {
pages = save_xbzrle_page(rs, pss, , current_addr,
 block, offset);

}

}

and

static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
{
/* Update pss->page for the next dirty bit in ramblock */
pss_find_next_dirty(pss);

if (pss->complete_round && pss->block == rs->last_seen_block &&
...
return PAGE_ALL_CLEAN;
}
if (!offset_in_ramblock(pss->block,
((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {

if (!pss->block) {

if (migrate_use_xbzrle()) {
rs->xbzrle_enabled = true;
}
}
...
} else {
/* We've found something */
return PAGE_DIRTY_FOUND;
}
}



> IIUC save_xbzrle_page will add pages into the cache on
> the first iteration, so the second iteration will get
> cache hits
>
>> - 2nd iteration: pages are put into cache, no xbzrle is used because
>>   there is no previous page.
>> - 3rd iteration: We really use xbzrle now against the copy of the
>>   previous iterations.
>> 
>> And yes, this should be commented somewhere.

Seeing that it has been able to confuse you, a single comment will not
make the trick O:-)

Later, Juan.

Re: [PATCH v2 2/6] tests/qtests: remove migration test iterations config

[Daniel P . Berrangé]

On Fri, Apr 21, 2023 at 11:54:55PM +0200, Juan Quintela wrote:
> Daniel P. Berrangé  wrote:
> > The 'unsigned int interations' config for migration is somewhat
> > overkill. Most tests don't set it, and a value of '0' is treated
> > as equivalent to '1'. The only test that does set it, xbzrle,
> > used a value of '2'.
> >
> > This setting, however, only relates to the migration iterations
> > that take place prior to allowing convergence. IOW, on top of
> > this iteration count, there is always at least 1 further migration
> > iteration done to deal with pages that are dirtied during the
> > previous iteration(s).
> >
> > IOW, even with iterations==1, the xbzrle test will be running for
> > a minimum of 2 iterations. With this in mind we can simplify the
> > code and just get rid of the special case.
> 
> Perhaps the old code was already wrong, but we need at least three
> iterations for the xbzrle test:
> - 1st iteration: xbzrle is not used, nothing is on cache.

Are you sure about this ?  I see ram_save_page() calling
save_xbzrle_page() and unless I'm mis-understanding the
code, it doesn't appear to skip anything on the 1st
iteration.

IIUC save_xbzrle_page will add pages into the cache on
the first iteration, so the second iteration will get
cache hits

> - 2nd iteration: pages are put into cache, no xbzrle is used because
>   there is no previous page.
> - 3rd iteration: We really use xbzrle now against the copy of the
>   previous iterations.
> 
> And yes, this should be commented somewhere.

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: test-blockjob: intermittent CI failures in msys2-64bit job

[Emanuele Giuseppe Esposito]

Am 26/04/2023 um 10:03 schrieb Hanna Czenczek:
> On 26.04.23 09:38, Emanuele Giuseppe Esposito wrote:
>>
>> Am 25/04/2023 um 18:48 schrieb Hanna Czenczek:
>>> On 24.04.23 20:32, Vladimir Sementsov-Ogievskiy wrote:
 On 24.04.23 16:36, Emanuele Giuseppe Esposito wrote:
>
> Am 21/04/2023 um 12:13 schrieb Vladimir Sementsov-Ogievskiy:
>> On 17.03.23 15:35, Thomas Huth wrote:
>>> On 17/03/2023 11.17, Peter Maydell wrote:
 On Mon, 6 Mar 2023 at 11:16, Peter Maydell
 
 wrote:
> On Fri, 3 Mar 2023 at 18:36, Peter Maydell
>  wrote:
>> I've noticed that test-blockjob seems to fail intermittently
>> on the msys2-64bit job:
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/3872508803
>> https://gitlab.com/qemu-project/qemu/-/jobs/3871061024
>> https://gitlab.com/qemu-project/qemu/-/jobs/3865312440
>>
>> Sample output:
>> | 53/89
>> ERROR:../tests/unit/test-blockjob.c:499:test_complete_in_standby:
>> assertion failed: (job->status == JOB_STATUS_STANDBY) ERROR
>> 53/89 qemu:unit / test-blockjob ERROR 0.08s exit status 3
> Here's an intermittent failure from my macos x86 machine:
>
> 172/621 qemu:unit / test-blockjob
>   ERROR   0.26s   killed by signal 6 SIGABRT
 And an intermittent on the freebsd 13 CI job:
 https://gitlab.com/qemu-project/qemu/-/jobs/3950667240

>>> MALLOC_PERTURB_=197
>>> G_TEST_BUILDDIR=/tmp/cirrus-ci-build/build/tests/unit
>>> G_TEST_SRCDIR=/tmp/cirrus-ci-build/tests/unit
>>> /tmp/cirrus-ci-build/build/tests/unit/test-blockjob --tap -k
 ▶ 178/650 /blockjob/ids
   OK
 178/650 qemu:unit / test-blockjob
   ERROR   0.31s   killed by signal 6 SIGABRT
 ― ✀
 ―
 stderr:
 Assertion failed: (job->status == JOB_STATUS_STANDBY), function
 test_complete_in_standby, file ../tests/unit/test-blockjob.c, line
 499.


 TAP parsing error: Too few tests run (expected 9, got 1)
 (test program exited with status code -6)
 ――

 Anybody in the block team looking at these, or shall we just
 disable this test entirely ?
>>> I ran into this issue today, too:
>>>
>>> https://gitlab.com/thuth/qemu/-/jobs/3954367101
>>>
>>> ... if nobody is interested in fixing this test, I think we should
>>> disable it...
>>>
>>> Thomas
>>>
>> I'm looking at this now, and seems that it's broken since
>> 6f592e5aca1a27fe1c1f6 "job.c: enable job lock/unlock and remove
>> Aiocontext locks", as it stops critical section by new
>> aio_context_release() call exactly after bdrv_drain_all_and(), so
>> it's
>> not a surprise that job may start at that moment and the following
>> assertion fires.
>>
>> Emanuele could please look at it?
>>
> Well if I understood correctly, the only thing that was preventing the
> job from continuing was the aiocontext lock held.
>
> The failing assertion even mentions that:
> /* Lock the IO thread to prevent the job from being run */
> [...]
> /* But the job cannot run, so it will remain on standby */
> assert(job->status == JOB_STATUS_STANDBY);
>
> Essentially bdrv_drain_all_end() would wake up the job coroutine,
> but it
> would anyways block somewhere after since the aiocontext lock was
> taken
> by the test.
>
> Now that we got rid of aiocontext lock in job code, nothing
> prevents the
> test from resuming.
> Mixing job lock and aiocontext acquire/release is not a good idea, and
> it would anyways block job_resume() called by bdrv_drain_all_end(), so
> the test itself would deadlock.
>
> So unless @Kevin has a better idea, this seems to be just an "hack" to
> test stuff that is not possible to do now anymore. What I would
> suggest
> is to get rid of that test, or at least of that assert function. I
> unfortunately cannot reproduce the failure, but I think the remaining
> functions called by the test should run as expected.
>
 Thanks! I agree. Probably, alternatively we could just expand the
 drained section, like

 @@ -488,12 +488,6 @@ static void test_complete_in_standby(void)
   bdrv_drain_all_begin();
   assert_job_status_is(job, JOB_STATUS_STANDBY);

 -    /* Lock the IO thread to prevent the job from being run */
 -    aio_context_acquire(ctx);
 -    /* This will schedule the job to resume it */
 -    bdrv_drain_all_end();
 -    aio_context_release(ctx);
 -
   

Re: test-blockjob: intermittent CI failures in msys2-64bit job

[Hanna Czenczek]

On 26.04.23 09:38, Emanuele Giuseppe Esposito wrote:


Am 25/04/2023 um 18:48 schrieb Hanna Czenczek:

On 24.04.23 20:32, Vladimir Sementsov-Ogievskiy wrote:

On 24.04.23 16:36, Emanuele Giuseppe Esposito wrote:


Am 21/04/2023 um 12:13 schrieb Vladimir Sementsov-Ogievskiy:

On 17.03.23 15:35, Thomas Huth wrote:

On 17/03/2023 11.17, Peter Maydell wrote:

On Mon, 6 Mar 2023 at 11:16, Peter Maydell 
wrote:

On Fri, 3 Mar 2023 at 18:36, Peter Maydell
 wrote:

I've noticed that test-blockjob seems to fail intermittently
on the msys2-64bit job:

https://gitlab.com/qemu-project/qemu/-/jobs/3872508803
https://gitlab.com/qemu-project/qemu/-/jobs/3871061024
https://gitlab.com/qemu-project/qemu/-/jobs/3865312440

Sample output:
| 53/89
ERROR:../tests/unit/test-blockjob.c:499:test_complete_in_standby:
assertion failed: (job->status == JOB_STATUS_STANDBY) ERROR
53/89 qemu:unit / test-blockjob ERROR 0.08s exit status 3

Here's an intermittent failure from my macos x86 machine:

172/621 qemu:unit / test-blockjob
  ERROR   0.26s   killed by signal 6 SIGABRT

And an intermittent on the freebsd 13 CI job:
https://gitlab.com/qemu-project/qemu/-/jobs/3950667240


MALLOC_PERTURB_=197
G_TEST_BUILDDIR=/tmp/cirrus-ci-build/build/tests/unit
G_TEST_SRCDIR=/tmp/cirrus-ci-build/tests/unit
/tmp/cirrus-ci-build/build/tests/unit/test-blockjob --tap -k

▶ 178/650 /blockjob/ids
  OK
178/650 qemu:unit / test-blockjob
  ERROR   0.31s   killed by signal 6 SIGABRT
― ✀
―
stderr:
Assertion failed: (job->status == JOB_STATUS_STANDBY), function
test_complete_in_standby, file ../tests/unit/test-blockjob.c, line
499.


TAP parsing error: Too few tests run (expected 9, got 1)
(test program exited with status code -6)
――

Anybody in the block team looking at these, or shall we just
disable this test entirely ?

I ran into this issue today, too:

    https://gitlab.com/thuth/qemu/-/jobs/3954367101

... if nobody is interested in fixing this test, I think we should
disable it...

    Thomas


I'm looking at this now, and seems that it's broken since
6f592e5aca1a27fe1c1f6 "job.c: enable job lock/unlock and remove
Aiocontext locks", as it stops critical section by new
aio_context_release() call exactly after bdrv_drain_all_and(), so it's
not a surprise that job may start at that moment and the following
assertion fires.

Emanuele could please look at it?


Well if I understood correctly, the only thing that was preventing the
job from continuing was the aiocontext lock held.

The failing assertion even mentions that:
/* Lock the IO thread to prevent the job from being run */
[...]
/* But the job cannot run, so it will remain on standby */
assert(job->status == JOB_STATUS_STANDBY);

Essentially bdrv_drain_all_end() would wake up the job coroutine, but it
would anyways block somewhere after since the aiocontext lock was taken
by the test.

Now that we got rid of aiocontext lock in job code, nothing prevents the
test from resuming.
Mixing job lock and aiocontext acquire/release is not a good idea, and
it would anyways block job_resume() called by bdrv_drain_all_end(), so
the test itself would deadlock.

So unless @Kevin has a better idea, this seems to be just an "hack" to
test stuff that is not possible to do now anymore. What I would suggest
is to get rid of that test, or at least of that assert function. I
unfortunately cannot reproduce the failure, but I think the remaining
functions called by the test should run as expected.


Thanks! I agree. Probably, alternatively we could just expand the
drained section, like

@@ -488,12 +488,6 @@ static void test_complete_in_standby(void)
  bdrv_drain_all_begin();
  assert_job_status_is(job, JOB_STATUS_STANDBY);

-    /* Lock the IO thread to prevent the job from being run */
-    aio_context_acquire(ctx);
-    /* This will schedule the job to resume it */
-    bdrv_drain_all_end();
-    aio_context_release(ctx);
-
  WITH_JOB_LOCK_GUARD() {
  /* But the job cannot run, so it will remain on standby */
  assert(job->status == JOB_STATUS_STANDBY);
@@ -511,6 +505,7 @@ static void test_complete_in_standby(void)
  job_dismiss_locked(, _abort);
  }

+    bdrv_drain_all_end();
  aio_context_acquire(ctx);
  destroy_blk(blk);
  aio_context_release(ctx);


But, seems that test wanted to specifically test job, that still in
STANDBY exactly after drained section...

[cc: Hanna]

Hanna, it was your test (added in
c2c731a4d35062295cd3260e66b3754588a2fad4, two years ago). Don't you
remember was important to catch STANDBY job *after* a drained section?

I’m not quite sure, but I think the idea was that we basically try to
get as close to something that might come in over QMP.  Over QMP, you
can’t issue a job-complete while keeping everything drained, so I
wouldn’t just extend the 

Re: test-blockjob: intermittent CI failures in msys2-64bit job

[Emanuele Giuseppe Esposito]

Am 25/04/2023 um 18:48 schrieb Hanna Czenczek:
> On 24.04.23 20:32, Vladimir Sementsov-Ogievskiy wrote:
>> On 24.04.23 16:36, Emanuele Giuseppe Esposito wrote:
>>>
>>>
>>> Am 21/04/2023 um 12:13 schrieb Vladimir Sementsov-Ogievskiy:
 On 17.03.23 15:35, Thomas Huth wrote:
> On 17/03/2023 11.17, Peter Maydell wrote:
>> On Mon, 6 Mar 2023 at 11:16, Peter Maydell 
>> wrote:
>>>
>>> On Fri, 3 Mar 2023 at 18:36, Peter Maydell
>>>  wrote:

 I've noticed that test-blockjob seems to fail intermittently
 on the msys2-64bit job:

 https://gitlab.com/qemu-project/qemu/-/jobs/3872508803
 https://gitlab.com/qemu-project/qemu/-/jobs/3871061024
 https://gitlab.com/qemu-project/qemu/-/jobs/3865312440

 Sample output:
 | 53/89
 ERROR:../tests/unit/test-blockjob.c:499:test_complete_in_standby:
 assertion failed: (job->status == JOB_STATUS_STANDBY) ERROR
 53/89 qemu:unit / test-blockjob ERROR 0.08s exit status 3
>>
>>> Here's an intermittent failure from my macos x86 machine:
>>>
>>> 172/621 qemu:unit / test-blockjob
>>>  ERROR   0.26s   killed by signal 6 SIGABRT
>>
>> And an intermittent on the freebsd 13 CI job:
>> https://gitlab.com/qemu-project/qemu/-/jobs/3950667240
>>
> MALLOC_PERTURB_=197
> G_TEST_BUILDDIR=/tmp/cirrus-ci-build/build/tests/unit
> G_TEST_SRCDIR=/tmp/cirrus-ci-build/tests/unit
> /tmp/cirrus-ci-build/build/tests/unit/test-blockjob --tap -k
>> ▶ 178/650 /blockjob/ids
>>  OK
>> 178/650 qemu:unit / test-blockjob
>>  ERROR   0.31s   killed by signal 6 SIGABRT
>> ― ✀
>> ―
>> stderr:
>> Assertion failed: (job->status == JOB_STATUS_STANDBY), function
>> test_complete_in_standby, file ../tests/unit/test-blockjob.c, line
>> 499.
>>
>>
>> TAP parsing error: Too few tests run (expected 9, got 1)
>> (test program exited with status code -6)
>> ――
>>
>> Anybody in the block team looking at these, or shall we just
>> disable this test entirely ?
>
> I ran into this issue today, too:
>
>    https://gitlab.com/thuth/qemu/-/jobs/3954367101
>
> ... if nobody is interested in fixing this test, I think we should
> disable it...
>
>    Thomas
>

 I'm looking at this now, and seems that it's broken since
 6f592e5aca1a27fe1c1f6 "job.c: enable job lock/unlock and remove
 Aiocontext locks", as it stops critical section by new
 aio_context_release() call exactly after bdrv_drain_all_and(), so it's
 not a surprise that job may start at that moment and the following
 assertion fires.

 Emanuele could please look at it?

>>> Well if I understood correctly, the only thing that was preventing the
>>> job from continuing was the aiocontext lock held.
>>>
>>> The failing assertion even mentions that:
>>> /* Lock the IO thread to prevent the job from being run */
>>> [...]
>>> /* But the job cannot run, so it will remain on standby */
>>> assert(job->status == JOB_STATUS_STANDBY);
>>>
>>> Essentially bdrv_drain_all_end() would wake up the job coroutine, but it
>>> would anyways block somewhere after since the aiocontext lock was taken
>>> by the test.
>>>
>>> Now that we got rid of aiocontext lock in job code, nothing prevents the
>>> test from resuming.
>>> Mixing job lock and aiocontext acquire/release is not a good idea, and
>>> it would anyways block job_resume() called by bdrv_drain_all_end(), so
>>> the test itself would deadlock.
>>>
>>> So unless @Kevin has a better idea, this seems to be just an "hack" to
>>> test stuff that is not possible to do now anymore. What I would suggest
>>> is to get rid of that test, or at least of that assert function. I
>>> unfortunately cannot reproduce the failure, but I think the remaining
>>> functions called by the test should run as expected.
>>>
>>
>> Thanks! I agree. Probably, alternatively we could just expand the
>> drained section, like
>>
>> @@ -488,12 +488,6 @@ static void test_complete_in_standby(void)
>>  bdrv_drain_all_begin();
>>  assert_job_status_is(job, JOB_STATUS_STANDBY);
>>
>> -    /* Lock the IO thread to prevent the job from being run */
>> -    aio_context_acquire(ctx);
>> -    /* This will schedule the job to resume it */
>> -    bdrv_drain_all_end();
>> -    aio_context_release(ctx);
>> -
>>  WITH_JOB_LOCK_GUARD() {
>>  /* But the job cannot run, so it will remain on standby */
>>  assert(job->status == JOB_STATUS_STANDBY);
>> @@ -511,6 +505,7 @@ static void test_complete_in_standby(void)
>>  job_dismiss_locked(, _abort);
>>  }
>>
>> +    bdrv_drain_all_end();
>>  

Re: [PATCH v2 1/3] hw/i2c: add mctp core

[Klaus Jensen]

On Apr 25 10:19, Corey Minyard wrote:
> On Tue, Apr 25, 2023 at 08:35:38AM +0200, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > control message handling as well as handling the actual I2C transport
> > (packetization).
> > 
> > Devices are intended to derive from this and implement the class
> > methods.
> > 
> > Parts of this implementation is inspired by code[1] previously posted by
> > Jonathan Cameron.
> 
> All in all this looks good.  Two comments:
> 
> I would like to see the buffer handling consolidated into one function
> and the length checked, even for (especially for) the outside users of
> this code, like the nvme code.  Best to avoid future issues with buffer
> overruns.  This will require reworking the get_message_types function,
> unfortunately.
> 

Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly
into the mctp core buffer for get_message_bytes(). The contract is that
it must not write more than the `maxlen` parameter. Is that bad? Would
it be better that get_message_bytes() returned a pointer to its own
buffer that hw/mctp can then copy from?

> You have one trace function on a bad receive message check, but lots of
> other bad receive message checks with no trace.  Just a suggestion, but
> it might be nice for tracking down issues to trace all the reasons a
> message is dropped.
> 

Sounds reasonable! :)

Thanks for the review!


signature.asc
Description: PGP signature