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

Philippe Mathieu-Daudé posted 20 patches 2 years, 11 months ago
Maintainers: Radoslaw Biernacki <rad@semihalf.com>, Peter Maydell <peter.maydell@linaro.org>, Leif Lindholm <quic_llindhol@quicinc.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, John Snow <jsnow@redhat.com>, BALATON Zoltan <balaton@eik.bme.hu>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Yoshinori Sato <ysato@users.sourceforge.jp>, Magnus Damm <magnus.damm@gmail.com>, Artyom Tarasenko <atar4qemu@gmail.com>
There is a newer version of this series
[PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
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;
-- 
2.38.1


Re: [PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()
Posted by Richard Henderson 2 years, 11 months ago
On 2/15/23 01:27, Philippe Mathieu-Daudé wrote:
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()
Posted by Bernhard Beschow 2 years, 11 months ago

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

Best regards,
Bernhard
Re: [PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()
Posted by BALATON Zoltan 2 years, 11 months ago
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
Re: [PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()
Posted by Bernhard Beschow 2 years, 11 months ago

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
Re: [PATCH 17/20] hw/ide/pci: Unexport bmdma_active_if()
Posted by Bernhard Beschow 2 years, 11 months ago

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