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

[Bernhard Beschow]

Am 15. Februar 2023 21:09:10 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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.

Yes, that would make sense and is indeed the "phantom" behavior I would be 
expecting.

>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.

Interesting read... Thanks for sharing!

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan