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

[Bernhard Beschow]

Am 16. Februar 2023 00:18:47 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>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.

Just an idea: Perhaps pci_ide_create_devs() rather than ide_init2() could 
create the IDEState objects -- of course only if the respective drives are 
configured.

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