Re: [PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()

[BALATON Zoltan]

On Wed, 15 Feb 2023, Bernhard Beschow wrote:
> Am 15. Februar 2023 11:27:09 UTC schrieb "Philippe Mathieu-Daudé" 
> <philmd@linaro.org>:
> > From: Bernhard Beschow <shentey@gmail.com>
> >
> > The function is only used inside ide/pci.c, so doesn't need to be exported.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > hw/ide/pci.c         | 6 ++++++
> > include/hw/ide/pci.h | 6 ------
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> > index 2ddcb49b27..fc9224bbc9 100644
> > --- a/hw/ide/pci.c
> > +++ b/hw/ide/pci.c
> > @@ -104,6 +104,12 @@ const MemoryRegionOps pci_ide_data_le_ops = {
> >     .endianness = DEVICE_LITTLE_ENDIAN,
> > };
> >
> > +static IDEState *bmdma_active_if(BMDMAState *bmdma)
> > +{
> > +    assert(bmdma->bus->retry_unit != (uint8_t)-1);
> > +    return bmdma->bus->ifs + bmdma->bus->retry_unit;
> > +}
> > +
> > static void bmdma_start_dma(const IDEDMA *dma, IDEState *s,
> >                             BlockCompletionFunc *dma_cb)
> > {
> > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> > index 2a6284acac..7b5e3f6e1c 100644
> > --- a/include/hw/ide/pci.h
> > +++ b/include/hw/ide/pci.h
> > @@ -55,12 +55,6 @@ struct PCIIDEState {
> >     MemoryRegion data_bar[2];
> > };
> >
> > -static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> > -{
> > -    assert(bmdma->bus->retry_unit != (uint8_t)-1);
> > -    return bmdma->bus->ifs + bmdma->bus->retry_unit;
> > -}
> > -
> > void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
> > void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
> > extern MemoryRegionOps bmdma_addr_ioport_ops;
>
> Cool, where did you find this? ;)
>
> This patch, your other patches doing s/ide/ide_bus/, and the fact that 
> IDEBus instantiates two IDE devices itself make me wonder whether the 
> IDE devices should really be instantiated in the usual QOM way. Then 
> perhaps it could turn out that all the s/ide/ide_bus/ patches aren't 
> really needed since the functions could operate on the IDE device 
> directly. Not sure though...
>
> This might all be a rabbit hole, but since you already started looking 
> into it... ;)

If you want some more, we've also found another problem with ports that 
should not have anythnig connected but still appear to have a non-working 
device that causes some delay during guest boot (and I think this is the 
reason the pegasos2 firmware prints an "ATA device not present or not 
responding" error while detecting IDE devices. This was discussed here:

https://lists.nongnu.org/archive/html/qemu-devel/2020-02/msg02468.html

Just to spread knowledge about it. I don't exoect this to be fixed as it 
does not cause much trouble just if you wanted to dig into it in the 
future I thought I'd let you know.

Regards,
BALATON Zoltan