| Message ID | 20230215112712.23110-18-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/ide: QOM/QDev housekeeping | expand |
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
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~
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
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
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
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;