hw/ide/core.c | 12 ++++++ hw/ide/ioport.c | 12 ------ hw/ide/pci.c | 84 +++++++++++++++++++++++++++++++++++++++ hw/ide/via.c | 44 ++++++++++++++++---- include/hw/ide/internal.h | 3 ++ include/hw/ide/pci.h | 1 + 6 files changed, 137 insertions(+), 19 deletions(-)
This series adds a simple implementation of legacy/native mode switching for PCI
IDE controllers and updates the via-ide device to use it.
The approach I take here is to add a new pci_ide_update_mode() function which handles
management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing
details of the internal logic to individual PCI IDE controllers.
As noted in [1] this is extracted from a local WIP branch I have which contains
further work in this area. However for the moment I've kept it simple (and
restricted it to the via-ide device) which is good enough for Zoltan's PPC
images whilst paving the way for future improvements after 8.2.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
[1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
v3:
- Rebase onto master
- Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent duplication in
hw/ide/pci.c
- Don't zero BARs when switching from native mode to legacy mode, instead always force
them to read zero as suggested in the PCI IDE specification (note: this also appears
to fix the fuloong2e machine booting from IDE)
- Add comments in pci_ide_update_mode() suggested by Kevin
- Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
around zero BARs feels different enough here
v2:
- Rebase onto master
- Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in patch 1
- Add patch 2 to remove the default BAR addresses to avoid confusion
- Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already set
by pci_ide_update_mode() in patch 3, and reword the commit message accordingly
- Add Tested-By tags from Zoltan and Bernhard
Mark Cave-Ayland (4):
ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
to IDE core
ide/pci: introduce pci_ide_update_mode() function
ide/via: don't attempt to set default BAR addresses
hw/ide/via: implement legacy/native mode switching
hw/ide/core.c | 12 ++++++
hw/ide/ioport.c | 12 ------
hw/ide/pci.c | 84 +++++++++++++++++++++++++++++++++++++++
hw/ide/via.c | 44 ++++++++++++++++----
include/hw/ide/internal.h | 3 ++
include/hw/ide/pci.h | 1 +
6 files changed, 137 insertions(+), 19 deletions(-)
--
2.39.2
On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: > This series adds a simple implementation of legacy/native mode switching for PCI > IDE controllers and updates the via-ide device to use it. > > The approach I take here is to add a new pci_ide_update_mode() function which handles > management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing > details of the internal logic to individual PCI IDE controllers. > > As noted in [1] this is extracted from a local WIP branch I have which contains > further work in this area. However for the moment I've kept it simple (and > restricted it to the via-ide device) which is good enough for Zoltan's PPC > images whilst paving the way for future improvements after 8.2. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html > > v3: > - Rebase onto master > - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent duplication in > hw/ide/pci.c > - Don't zero BARs when switching from native mode to legacy mode, instead always force > them to read zero as suggested in the PCI IDE specification (note: this also appears > to fix the fuloong2e machine booting from IDE) Not sure you're getting this, see also: https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html but this seems to break latest version of the AmigaOS driver for some reason. I assume this is the BAR zeroing that causes this as it works with v2 series and nothing else changed in v3 that could cause this. Testing was done by Rene Engel, cc'd so maybe he can add more info. It seems to work with my patch that sets BARs to legacy values and with v2 that sets them to 0 but not with v3 which should also read 0 but maybe something is off here. Regards, BALATON Zoltan > - Add comments in pci_ide_update_mode() suggested by Kevin > - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour > around zero BARs feels different enough here > > v2: > - Rebase onto master > - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in patch 1 > - Add patch 2 to remove the default BAR addresses to avoid confusion > - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already set > by pci_ide_update_mode() in patch 3, and reword the commit message accordingly > - Add Tested-By tags from Zoltan and Bernhard > > > Mark Cave-Ayland (4): > ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions > to IDE core > ide/pci: introduce pci_ide_update_mode() function > ide/via: don't attempt to set default BAR addresses > hw/ide/via: implement legacy/native mode switching > > hw/ide/core.c | 12 ++++++ > hw/ide/ioport.c | 12 ------ > hw/ide/pci.c | 84 +++++++++++++++++++++++++++++++++++++++ > hw/ide/via.c | 44 ++++++++++++++++---- > include/hw/ide/internal.h | 3 ++ > include/hw/ide/pci.h | 1 + > 6 files changed, 137 insertions(+), 19 deletions(-) > >
On 19/11/2023 21:43, BALATON Zoltan wrote:
> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:
>> This series adds a simple implementation of legacy/native mode switching for PCI
>> IDE controllers and updates the via-ide device to use it.
>>
>> The approach I take here is to add a new pci_ide_update_mode() function which handles
>> management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing
>> details of the internal logic to individual PCI IDE controllers.
>>
>> As noted in [1] this is extracted from a local WIP branch I have which contains
>> further work in this area. However for the moment I've kept it simple (and
>> restricted it to the via-ide device) which is good enough for Zoltan's PPC
>> images whilst paving the way for future improvements after 8.2.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>>
>> v3:
>> - Rebase onto master
>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent duplication in
>> hw/ide/pci.c
>> - Don't zero BARs when switching from native mode to legacy mode, instead always force
>> them to read zero as suggested in the PCI IDE specification (note: this also appears
>> to fix the fuloong2e machine booting from IDE)
>
> Not sure you're getting this, see also:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html
> but this seems to break latest version of the AmigaOS driver for some reason. I
> assume this is the BAR zeroing that causes this as it works with v2 series and
> nothing else changed in v3 that could cause this. Testing was done by Rene Engel,
> cc'd so maybe he can add more info. It seems to work with my patch that sets BARs to
> legacy values and with v2 that sets them to 0 but not with v3 which should also read
> 0 but maybe something is off here.
I've been AFK for a few days, so just starting to catch up on various bits and pieces.
The only difference I can think of regarding the BAR zeroing is that the BMDMA BAR is
zeroed here. Does the following diff fix things?
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..2d3124ebd7 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -164,10 +164,10 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr,
int len)
uint8_t mode = pd->config[PCI_CLASS_PROG];
if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
- PCI_BASE_ADDRESS_0, 24)) {
+ PCI_BASE_ADDRESS_0, 16)) {
/* BARs always read back zero in legacy mode */
for (int i = addr; i < addr + len; i++) {
- if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
+ if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
val &= ~(0xffULL << ((i - addr) << 3));
}
}
>> - Add comments in pci_ide_update_mode() suggested by Kevin
>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour
>> around zero BARs feels different enough here
>>
>> v2:
>> - Rebase onto master
>> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in patch 1
>> - Add patch 2 to remove the default BAR addresses to avoid confusion
>> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already set
>> by pci_ide_update_mode() in patch 3, and reword the commit message accordingly
>> - Add Tested-By tags from Zoltan and Bernhard
>>
>>
>> Mark Cave-Ayland (4):
>> ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
>> to IDE core
>> ide/pci: introduce pci_ide_update_mode() function
>> ide/via: don't attempt to set default BAR addresses
>> hw/ide/via: implement legacy/native mode switching
>>
>> hw/ide/core.c | 12 ++++++
>> hw/ide/ioport.c | 12 ------
>> hw/ide/pci.c | 84 +++++++++++++++++++++++++++++++++++++++
>> hw/ide/via.c | 44 ++++++++++++++++----
>> include/hw/ide/internal.h | 3 ++
>> include/hw/ide/pci.h | 1 +
>> 6 files changed, 137 insertions(+), 19 deletions(-)
ATB,
Mark.
On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
> On 19/11/2023 21:43, BALATON Zoltan wrote:
>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:
>>> This series adds a simple implementation of legacy/native mode switching
>>> for PCI
>>> IDE controllers and updates the via-ide device to use it.
>>>
>>> The approach I take here is to add a new pci_ide_update_mode() function
>>> which handles
>>> management of the PCI BARs and legacy IDE ioports for each mode to avoid
>>> exposing
>>> details of the internal logic to individual PCI IDE controllers.
>>>
>>> As noted in [1] this is extracted from a local WIP branch I have which
>>> contains
>>> further work in this area. However for the moment I've kept it simple (and
>>> restricted it to the via-ide device) which is good enough for Zoltan's PPC
>>> images whilst paving the way for future improvements after 8.2.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>>>
>>> v3:
>>> - Rebase onto master
>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent
>>> duplication in
>>> hw/ide/pci.c
>>> - Don't zero BARs when switching from native mode to legacy mode, instead
>>> always force
>>> them to read zero as suggested in the PCI IDE specification (note: this
>>> also appears
>>> to fix the fuloong2e machine booting from IDE)
>>
>> Not sure you're getting this, see also:
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html
>> but this seems to break latest version of the AmigaOS driver for some
>> reason. I assume this is the BAR zeroing that causes this as it works with
>> v2 series and nothing else changed in v3 that could cause this. Testing was
>> done by Rene Engel, cc'd so maybe he can add more info. It seems to work
>> with my patch that sets BARs to legacy values and with v2 that sets them to
>> 0 but not with v3 which should also read 0 but maybe something is off here.
>
> I've been AFK for a few days, so just starting to catch up on various bits
> and pieces.
OK just wasn't sure if you saw my emails at all as it happened before that
some spam filters disliked my mail server and put messages in the spam
folder.
> The only difference I can think of regarding the BAR zeroing is that the
> BMDMA BAR is zeroed here. Does the following diff fix things?
This helps, with this the latest driver does not crash but still reads
BAR4 as 0 instead of 0xcc00 so UDMA won't work but at least it boots.
Regards,
BALATON Zoltan
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 47223b1268..2d3124ebd7 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -164,10 +164,10 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd,
> uint32_t addr, int len)
> uint8_t mode = pd->config[PCI_CLASS_PROG];
>
> if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
> - PCI_BASE_ADDRESS_0, 24)) {
> + PCI_BASE_ADDRESS_0, 16)) {
> /* BARs always read back zero in legacy mode */
> for (int i = addr; i < addr + len; i++) {
> - if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
> + if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
> val &= ~(0xffULL << ((i - addr) << 3));
> }
> }
>
>>> - Add comments in pci_ide_update_mode() suggested by Kevin
>>> - Drop the existing R-B and T-B tags: whilst this passes my local tests,
>>> the behaviour
>>> around zero BARs feels different enough here
>>>
>>> v2:
>>> - Rebase onto master
>>> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in
>>> patch 1
>>> - Add patch 2 to remove the default BAR addresses to avoid confusion
>>> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already
>>> set
>>> by pci_ide_update_mode() in patch 3, and reword the commit message
>>> accordingly
>>> - Add Tested-By tags from Zoltan and Bernhard
>>>
>>>
>>> Mark Cave-Ayland (4):
>>> ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions
>>> to IDE core
>>> ide/pci: introduce pci_ide_update_mode() function
>>> ide/via: don't attempt to set default BAR addresses
>>> hw/ide/via: implement legacy/native mode switching
>>>
>>> hw/ide/core.c | 12 ++++++
>>> hw/ide/ioport.c | 12 ------
>>> hw/ide/pci.c | 84 +++++++++++++++++++++++++++++++++++++++
>>> hw/ide/via.c | 44 ++++++++++++++++----
>>> include/hw/ide/internal.h | 3 ++
>>> include/hw/ide/pci.h | 1 +
>>> 6 files changed, 137 insertions(+), 19 deletions(-)
>
>
> ATB,
>
> Mark.
>
>
Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > > On 19/11/2023 21:43, BALATON Zoltan wrote: > > > On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: > > > > This series adds a simple implementation of legacy/native mode > > > > switching for PCI > > > > IDE controllers and updates the via-ide device to use it. > > > > > > > > The approach I take here is to add a new pci_ide_update_mode() > > > > function which handles > > > > management of the PCI BARs and legacy IDE ioports for each mode > > > > to avoid exposing > > > > details of the internal logic to individual PCI IDE controllers. > > > > > > > > As noted in [1] this is extracted from a local WIP branch I have > > > > which contains > > > > further work in this area. However for the moment I've kept it simple (and > > > > restricted it to the via-ide device) which is good enough for Zoltan's PPC > > > > images whilst paving the way for future improvements after 8.2. > > > > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > > > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html > > > > > > > > v3: > > > > - Rebase onto master > > > > - Move ide_portio_list[] and ide_portio_list2[] to IDE core to > > > > prevent duplication in > > > > hw/ide/pci.c > > > > - Don't zero BARs when switching from native mode to legacy > > > > mode, instead always force > > > > them to read zero as suggested in the PCI IDE specification > > > > (note: this also appears > > > > to fix the fuloong2e machine booting from IDE) > > > > > > Not sure you're getting this, see also: > > > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html > > > but this seems to break latest version of the AmigaOS driver for > > > some reason. I assume this is the BAR zeroing that causes this as it > > > works with v2 series and nothing else changed in v3 that could cause > > > this. Testing was done by Rene Engel, cc'd so maybe he can add more > > > info. It seems to work with my patch that sets BARs to legacy values > > > and with v2 that sets them to 0 but not with v3 which should also > > > read 0 but maybe something is off here. > > > > I've been AFK for a few days, so just starting to catch up on various > > bits and pieces. > > OK just wasn't sure if you saw my emails at all as it happened before that > some spam filters disliked my mail server and put messages in the spam > folder. > > > The only difference I can think of regarding the BAR zeroing is that the > > BMDMA BAR is zeroed here. Does the following diff fix things? > > This helps, with this the latest driver does not crash but still reads BAR4 > as 0 instead of 0xcc00 so UDMA won't work but at least it boots. And disabling only the first four BARs is actually what the spec says, too. So I'll make this change to the queued patches. If I understand correctly, UDMA didn't work before this series either, so it's a separate goal and doing it in its own patch is best anyway. As we don't seem to have a good place to set a default, maybe just overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in compatibility mode is enough? Kevin
On 20/11/2023 13:42, Kevin Wolf wrote: > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: >> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >>> On 19/11/2023 21:43, BALATON Zoltan wrote: >>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>>>> This series adds a simple implementation of legacy/native mode >>>>> switching for PCI >>>>> IDE controllers and updates the via-ide device to use it. >>>>> >>>>> The approach I take here is to add a new pci_ide_update_mode() >>>>> function which handles >>>>> management of the PCI BARs and legacy IDE ioports for each mode >>>>> to avoid exposing >>>>> details of the internal logic to individual PCI IDE controllers. >>>>> >>>>> As noted in [1] this is extracted from a local WIP branch I have >>>>> which contains >>>>> further work in this area. However for the moment I've kept it simple (and >>>>> restricted it to the via-ide device) which is good enough for Zoltan's PPC >>>>> images whilst paving the way for future improvements after 8.2. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> >>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>>> >>>>> v3: >>>>> - Rebase onto master >>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to >>>>> prevent duplication in >>>>> hw/ide/pci.c >>>>> - Don't zero BARs when switching from native mode to legacy >>>>> mode, instead always force >>>>> them to read zero as suggested in the PCI IDE specification >>>>> (note: this also appears >>>>> to fix the fuloong2e machine booting from IDE) >>>> >>>> Not sure you're getting this, see also: >>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >>>> but this seems to break latest version of the AmigaOS driver for >>>> some reason. I assume this is the BAR zeroing that causes this as it >>>> works with v2 series and nothing else changed in v3 that could cause >>>> this. Testing was done by Rene Engel, cc'd so maybe he can add more >>>> info. It seems to work with my patch that sets BARs to legacy values >>>> and with v2 that sets them to 0 but not with v3 which should also >>>> read 0 but maybe something is off here. >>> >>> I've been AFK for a few days, so just starting to catch up on various >>> bits and pieces. >> >> OK just wasn't sure if you saw my emails at all as it happened before that >> some spam filters disliked my mail server and put messages in the spam >> folder. >> >>> The only difference I can think of regarding the BAR zeroing is that the >>> BMDMA BAR is zeroed here. Does the following diff fix things? >> >> This helps, with this the latest driver does not crash but still reads BAR4 >> as 0 instead of 0xcc00 so UDMA won't work but at least it boots. > > And disabling only the first four BARs is actually what the spec says, > too. So I'll make this change to the queued patches. That was definitely something that I thought about: what should happen to BARs outside of the ones mentioned in the PCI IDE controller specification? It seems reasonable to me just to consider BARS 0-3 for zeroing here. > If I understand correctly, UDMA didn't work before this series either, > so it's a separate goal and doing it in its own patch is best anyway. > > As we don't seem to have a good place to set a default, maybe just > overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in > compatibility mode is enough? It's difficult to know whether switching to legacy mode on the via-ide device resets BAR4 to its default value, or whether it is simply left unaltered. For 8.2 I don't mind too much as long as the logic is separate from the BAR zeroing logic (which will eventually be lifted up into hw/ide/pci.c). ATB, Mark.
On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > On 20/11/2023 13:42, Kevin Wolf wrote: >> Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: >>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >>>> On 19/11/2023 21:43, BALATON Zoltan wrote: >>>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>>>>> This series adds a simple implementation of legacy/native mode >>>>>> switching for PCI >>>>>> IDE controllers and updates the via-ide device to use it. >>>>>> >>>>>> The approach I take here is to add a new pci_ide_update_mode() >>>>>> function which handles >>>>>> management of the PCI BARs and legacy IDE ioports for each mode >>>>>> to avoid exposing >>>>>> details of the internal logic to individual PCI IDE controllers. >>>>>> >>>>>> As noted in [1] this is extracted from a local WIP branch I have >>>>>> which contains >>>>>> further work in this area. However for the moment I've kept it simple >>>>>> (and >>>>>> restricted it to the via-ide device) which is good enough for Zoltan's >>>>>> PPC >>>>>> images whilst paving the way for future improvements after 8.2. >>>>>> >>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>> >>>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>>>> >>>>>> v3: >>>>>> - Rebase onto master >>>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to >>>>>> prevent duplication in >>>>>> hw/ide/pci.c >>>>>> - Don't zero BARs when switching from native mode to legacy >>>>>> mode, instead always force >>>>>> them to read zero as suggested in the PCI IDE specification >>>>>> (note: this also appears >>>>>> to fix the fuloong2e machine booting from IDE) >>>>> >>>>> Not sure you're getting this, see also: >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >>>>> but this seems to break latest version of the AmigaOS driver for >>>>> some reason. I assume this is the BAR zeroing that causes this as it >>>>> works with v2 series and nothing else changed in v3 that could cause >>>>> this. Testing was done by Rene Engel, cc'd so maybe he can add more >>>>> info. It seems to work with my patch that sets BARs to legacy values >>>>> and with v2 that sets them to 0 but not with v3 which should also >>>>> read 0 but maybe something is off here. >>>> >>>> I've been AFK for a few days, so just starting to catch up on various >>>> bits and pieces. >>> >>> OK just wasn't sure if you saw my emails at all as it happened before that >>> some spam filters disliked my mail server and put messages in the spam >>> folder. >>> >>>> The only difference I can think of regarding the BAR zeroing is that the >>>> BMDMA BAR is zeroed here. Does the following diff fix things? >>> >>> This helps, with this the latest driver does not crash but still reads >>> BAR4 >>> as 0 instead of 0xcc00 so UDMA won't work but at least it boots. >> >> And disabling only the first four BARs is actually what the spec says, >> too. So I'll make this change to the queued patches. > > That was definitely something that I thought about: what should happen to > BARs outside of the ones mentioned in the PCI IDE controller specification? > It seems reasonable to me just to consider BARS 0-3 for zeroing here. > >> If I understand correctly, UDMA didn't work before this series either, >> so it's a separate goal and doing it in its own patch is best anyway. >> >> As we don't seem to have a good place to set a default, maybe just >> overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in >> compatibility mode is enough? > > It's difficult to know whether switching to legacy mode on the via-ide device > resets BAR4 to its default value, or whether it is simply left unaltered. For > 8.2 I don't mind too much as long as the logic is separate from the BAR > zeroing logic (which will eventually be lifted up into hw/ide/pci.c). My original patch checked for BAR being unset and only reset to defailt in that case so it won't clobber a value set by something (like pegasos2 firmware) but will set default for amigaone which does not program the BAR just uses the default legacy mode (which is the default on real chip but we have to make that happen on QEMU after reset). So setting it to default if it's unset when switching to legacy seems like a safe bet and testing with my patch did not find problem with that. Regards, BALATON Zoltan
Am 20.11.2023 um 16:02 hat BALATON Zoltan geschrieben: > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > > On 20/11/2023 13:42, Kevin Wolf wrote: > > > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: > > > > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > > > > > On 19/11/2023 21:43, BALATON Zoltan wrote: > > > > > > On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: > > > > > > > This series adds a simple implementation of legacy/native mode > > > > > > > switching for PCI > > > > > > > IDE controllers and updates the via-ide device to use it. > > > > > > > > > > > > > > The approach I take here is to add a new pci_ide_update_mode() > > > > > > > function which handles > > > > > > > management of the PCI BARs and legacy IDE ioports for each mode > > > > > > > to avoid exposing > > > > > > > details of the internal logic to individual PCI IDE controllers. > > > > > > > > > > > > > > As noted in [1] this is extracted from a local WIP branch I have > > > > > > > which contains > > > > > > > further work in this area. However for the moment > > > > > > > I've kept it simple (and > > > > > > > restricted it to the via-ide device) which is good > > > > > > > enough for Zoltan's PPC > > > > > > > images whilst paving the way for future improvements after 8.2. > > > > > > > > > > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > > > > > > > > > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html > > > > > > > > > > > > > > v3: > > > > > > > - Rebase onto master > > > > > > > - Move ide_portio_list[] and ide_portio_list2[] to IDE core to > > > > > > > prevent duplication in > > > > > > > hw/ide/pci.c > > > > > > > - Don't zero BARs when switching from native mode to legacy > > > > > > > mode, instead always force > > > > > > > them to read zero as suggested in the PCI IDE specification > > > > > > > (note: this also appears > > > > > > > to fix the fuloong2e machine booting from IDE) > > > > > > > > > > > > Not sure you're getting this, see also: > > > > > > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html > > > > > > but this seems to break latest version of the AmigaOS driver for > > > > > > some reason. I assume this is the BAR zeroing that causes this as it > > > > > > works with v2 series and nothing else changed in v3 that could cause > > > > > > this. Testing was done by Rene Engel, cc'd so maybe he can add more > > > > > > info. It seems to work with my patch that sets BARs to legacy values > > > > > > and with v2 that sets them to 0 but not with v3 which should also > > > > > > read 0 but maybe something is off here. > > > > > > > > > > I've been AFK for a few days, so just starting to catch up on various > > > > > bits and pieces. > > > > > > > > OK just wasn't sure if you saw my emails at all as it happened before that > > > > some spam filters disliked my mail server and put messages in the spam > > > > folder. > > > > > > > > > The only difference I can think of regarding the BAR zeroing is that the > > > > > BMDMA BAR is zeroed here. Does the following diff fix things? > > > > > > > > This helps, with this the latest driver does not crash but still > > > > reads BAR4 > > > > as 0 instead of 0xcc00 so UDMA won't work but at least it boots. > > > > > > And disabling only the first four BARs is actually what the spec says, > > > too. So I'll make this change to the queued patches. > > > > That was definitely something that I thought about: what should happen > > to BARs outside of the ones mentioned in the PCI IDE controller > > specification? It seems reasonable to me just to consider BARS 0-3 for > > zeroing here. > > > > > If I understand correctly, UDMA didn't work before this series either, > > > so it's a separate goal and doing it in its own patch is best anyway. > > > > > > As we don't seem to have a good place to set a default, maybe just > > > overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in > > > compatibility mode is enough? > > > > It's difficult to know whether switching to legacy mode on the via-ide > > device resets BAR4 to its default value, or whether it is simply left > > unaltered. For 8.2 I don't mind too much as long as the logic is > > separate from the BAR zeroing logic (which will eventually be lifted up > > into hw/ide/pci.c). > > My original patch checked for BAR being unset and only reset to defailt in > that case so it won't clobber a value set by something (like pegasos2 > firmware) but will set default for amigaone which does not program the BAR > just uses the default legacy mode (which is the default on real chip but we > have to make that happen on QEMU after reset). So setting it to default if > it's unset when switching to legacy seems like a safe bet and testing with > my patch did not find problem with that. How about setting the default if it's unset on the first read after reset instead of only on switching modes? I'd like to avoid that the guest could observe surprising state changes, even if the OSes we're currently looking at don't do that. It just seems more robust than introducing random magic at arbitrary points. Kevin
On 21/11/2023 09:12, Kevin Wolf wrote:
> Am 20.11.2023 um 16:02 hat BALATON Zoltan geschrieben:
>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
>>> On 20/11/2023 13:42, Kevin Wolf wrote:
>>>> Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:
>>>>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
>>>>>> On 19/11/2023 21:43, BALATON Zoltan wrote:
>>>>>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:
>>>>>>>> This series adds a simple implementation of legacy/native mode
>>>>>>>> switching for PCI
>>>>>>>> IDE controllers and updates the via-ide device to use it.
>>>>>>>>
>>>>>>>> The approach I take here is to add a new pci_ide_update_mode()
>>>>>>>> function which handles
>>>>>>>> management of the PCI BARs and legacy IDE ioports for each mode
>>>>>>>> to avoid exposing
>>>>>>>> details of the internal logic to individual PCI IDE controllers.
>>>>>>>>
>>>>>>>> As noted in [1] this is extracted from a local WIP branch I have
>>>>>>>> which contains
>>>>>>>> further work in this area. However for the moment
>>>>>>>> I've kept it simple (and
>>>>>>>> restricted it to the via-ide device) which is good
>>>>>>>> enough for Zoltan's PPC
>>>>>>>> images whilst paving the way for future improvements after 8.2.
>>>>>>>>
>>>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>>>>
>>>>>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>>>>>>>>
>>>>>>>> v3:
>>>>>>>> - Rebase onto master
>>>>>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to
>>>>>>>> prevent duplication in
>>>>>>>> hw/ide/pci.c
>>>>>>>> - Don't zero BARs when switching from native mode to legacy
>>>>>>>> mode, instead always force
>>>>>>>> them to read zero as suggested in the PCI IDE specification
>>>>>>>> (note: this also appears
>>>>>>>> to fix the fuloong2e machine booting from IDE)
>>>>>>>
>>>>>>> Not sure you're getting this, see also:
>>>>>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html
>>>>>>> but this seems to break latest version of the AmigaOS driver for
>>>>>>> some reason. I assume this is the BAR zeroing that causes this as it
>>>>>>> works with v2 series and nothing else changed in v3 that could cause
>>>>>>> this. Testing was done by Rene Engel, cc'd so maybe he can add more
>>>>>>> info. It seems to work with my patch that sets BARs to legacy values
>>>>>>> and with v2 that sets them to 0 but not with v3 which should also
>>>>>>> read 0 but maybe something is off here.
>>>>>>
>>>>>> I've been AFK for a few days, so just starting to catch up on various
>>>>>> bits and pieces.
>>>>>
>>>>> OK just wasn't sure if you saw my emails at all as it happened before that
>>>>> some spam filters disliked my mail server and put messages in the spam
>>>>> folder.
>>>>>
>>>>>> The only difference I can think of regarding the BAR zeroing is that the
>>>>>> BMDMA BAR is zeroed here. Does the following diff fix things?
>>>>>
>>>>> This helps, with this the latest driver does not crash but still
>>>>> reads BAR4
>>>>> as 0 instead of 0xcc00 so UDMA won't work but at least it boots.
>>>>
>>>> And disabling only the first four BARs is actually what the spec says,
>>>> too. So I'll make this change to the queued patches.
>>>
>>> That was definitely something that I thought about: what should happen
>>> to BARs outside of the ones mentioned in the PCI IDE controller
>>> specification? It seems reasonable to me just to consider BARS 0-3 for
>>> zeroing here.
>>>
>>>> If I understand correctly, UDMA didn't work before this series either,
>>>> so it's a separate goal and doing it in its own patch is best anyway.
>>>>
>>>> As we don't seem to have a good place to set a default, maybe just
>>>> overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in
>>>> compatibility mode is enough?
>>>
>>> It's difficult to know whether switching to legacy mode on the via-ide
>>> device resets BAR4 to its default value, or whether it is simply left
>>> unaltered. For 8.2 I don't mind too much as long as the logic is
>>> separate from the BAR zeroing logic (which will eventually be lifted up
>>> into hw/ide/pci.c).
>>
>> My original patch checked for BAR being unset and only reset to defailt in
>> that case so it won't clobber a value set by something (like pegasos2
>> firmware) but will set default for amigaone which does not program the BAR
>> just uses the default legacy mode (which is the default on real chip but we
>> have to make that happen on QEMU after reset). So setting it to default if
>> it's unset when switching to legacy seems like a safe bet and testing with
>> my patch did not find problem with that.
>
> How about setting the default if it's unset on the first read after
> reset instead of only on switching modes? I'd like to avoid that the
> guest could observe surprising state changes, even if the OSes we're
> currently looking at don't do that. It just seems more robust than
> introducing random magic at arbitrary points.
Note that Zoltan's image will work with just the change suggested in
https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg04331.html, but without
BMDMA so any discussion re: default BAR addresses can be handled separately from the
currently queued patches.
On real AmigaOne hardware BMDMA isn't well tested because it needs a hardware
modification and configuration changes for U-Boot [1], and given that BAR4 isn't
programmed by either the OS or the firmware, I'd be inclined to say that the fact it
even works at all is because of a happy coincidence of bugs.
In the meantime the department of hacks has been looking at ways of trying to set BAR
addresses during reset, and humbly submits the following for consideration:
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..bec4a3d09b 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -30,6 +30,7 @@
#include "qemu/module.h"
#include "qemu/range.h"
#include "sysemu/dma.h"
+#include "sysemu/reset.h"
#include "hw/isa/vt82c686.h"
#include "hw/ide/pci.h"
#include "hw/irq.h"
@@ -118,6 +119,23 @@ static void via_ide_set_irq(void *opaque, int n, int level)
qemu_set_irq(s->isa_irq[n], level);
}
#include "hw/irq.h"
@@ -118,6 +119,23 @@ static void via_ide_set_irq(void *opaque, int n, int level)
qemu_set_irq(s->isa_irq[n], level);
}
+static void via_ide_bar_reset(void *opaque)
+{
+ PCIIDEState *d = PCI_IDE(opaque);
+ PCIDevice *pd = PCI_DEVICE(d);
+ uint8_t *pci_conf = pd->config;
+
+ /*
+ * Some OSs e.g. AmigaOS rely on the default BMDMA BAR value being present
+ * to initialise correctly, even in legacy mode(!)
+ */
+ pci_set_long(pci_conf + PCI_BASE_ADDRESS_4,
+ 0xcc00 | PCI_BASE_ADDRESS_SPACE_IO);
+
+ /* Unregister reset function */
+ qemu_unregister_reset(via_ide_bar_reset, opaque);
+}
+
static void via_ide_reset(DeviceState *dev)
{
PCIIDEState *d = PCI_IDE(dev);
@@ -156,6 +174,9 @@ static void via_ide_reset(DeviceState *dev)
pci_set_long(pci_conf + 0x68, 0x00000200);
/* PCI PM Block */
pci_set_long(pci_conf + 0xc0, 0x00020001);
+
+ /* Register separate function to set BAR values after PCI bus reset */
+ qemu_register_reset(via_ide_bar_reset, d);
}
static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, int len)
This works because the reset handlers are stored in an ordered list, so
via_ide_bar_reset() is guaranteed to be called after the PCI bus reset occurs.
ATB,
Mark.
[1] https://intuitionbase.com/hints.php
On Tue, 21 Nov 2023 at 10:18, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> In the meantime the department of hacks has been looking at ways of trying to set BAR
> addresses during reset, and humbly submits the following for consideration:
> +static void via_ide_bar_reset(void *opaque)
> +{
> + PCIIDEState *d = PCI_IDE(opaque);
> + PCIDevice *pd = PCI_DEVICE(d);
> + uint8_t *pci_conf = pd->config;
> +
> + /*
> + * Some OSs e.g. AmigaOS rely on the default BMDMA BAR value being present
> + * to initialise correctly, even in legacy mode(!)
> + */
> + pci_set_long(pci_conf + PCI_BASE_ADDRESS_4,
> + 0xcc00 | PCI_BASE_ADDRESS_SPACE_IO);
> +
> + /* Unregister reset function */
> + qemu_unregister_reset(via_ide_bar_reset, opaque);
> +}
> +
> static void via_ide_reset(DeviceState *dev)
> {
> PCIIDEState *d = PCI_IDE(dev);
> @@ -156,6 +174,9 @@ static void via_ide_reset(DeviceState *dev)
> pci_set_long(pci_conf + 0x68, 0x00000200);
> /* PCI PM Block */
> pci_set_long(pci_conf + 0xc0, 0x00020001);
> +
> + /* Register separate function to set BAR values after PCI bus reset */
> + qemu_register_reset(via_ide_bar_reset, d);
> }
I'm definitely not very enthusiastic about hacks which
increase the usage of qemu_register_reset() and rely
on reset-hook call order. We really need to try to have
another go at cleaning up the reset mess and this would be
yet another thing somebody's going to have to undo some day.
Unregistering your reset function in the reset function is
also rather curious...
thanks
-- PMM
On Mon, 20 Nov 2023, Kevin Wolf wrote: > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: >> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >>> On 19/11/2023 21:43, BALATON Zoltan wrote: >>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>>>> This series adds a simple implementation of legacy/native mode >>>>> switching for PCI >>>>> IDE controllers and updates the via-ide device to use it. >>>>> >>>>> The approach I take here is to add a new pci_ide_update_mode() >>>>> function which handles >>>>> management of the PCI BARs and legacy IDE ioports for each mode >>>>> to avoid exposing >>>>> details of the internal logic to individual PCI IDE controllers. >>>>> >>>>> As noted in [1] this is extracted from a local WIP branch I have >>>>> which contains >>>>> further work in this area. However for the moment I've kept it simple (and >>>>> restricted it to the via-ide device) which is good enough for Zoltan's PPC >>>>> images whilst paving the way for future improvements after 8.2. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> >>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>>> >>>>> v3: >>>>> - Rebase onto master >>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to >>>>> prevent duplication in >>>>> hw/ide/pci.c >>>>> - Don't zero BARs when switching from native mode to legacy >>>>> mode, instead always force >>>>> them to read zero as suggested in the PCI IDE specification >>>>> (note: this also appears >>>>> to fix the fuloong2e machine booting from IDE) >>>> >>>> Not sure you're getting this, see also: >>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >>>> but this seems to break latest version of the AmigaOS driver for >>>> some reason. I assume this is the BAR zeroing that causes this as it >>>> works with v2 series and nothing else changed in v3 that could cause >>>> this. Testing was done by Rene Engel, cc'd so maybe he can add more >>>> info. It seems to work with my patch that sets BARs to legacy values >>>> and with v2 that sets them to 0 but not with v3 which should also >>>> read 0 but maybe something is off here. >>> >>> I've been AFK for a few days, so just starting to catch up on various >>> bits and pieces. >> >> OK just wasn't sure if you saw my emails at all as it happened before that >> some spam filters disliked my mail server and put messages in the spam >> folder. >> >>> The only difference I can think of regarding the BAR zeroing is that the >>> BMDMA BAR is zeroed here. Does the following diff fix things? >> >> This helps, with this the latest driver does not crash but still reads BAR4 >> as 0 instead of 0xcc00 so UDMA won't work but at least it boots. > > And disabling only the first four BARs is actually what the spec says, > too. So I'll make this change to the queued patches. > > If I understand correctly, UDMA didn't work before this series either, > so it's a separate goal and doing it in its own patch is best anyway. UDMA works with my original series, did not work with earlier versions of this alternative from Mark but could be fixed up on top unless Mark can send a v4 now. > As we don't seem to have a good place to set a default, maybe just > overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in > compatibility mode is enough? I could give that a try and see if that helps but all this via_ide_cfg_read() seems like an unnecessary complication to me. Why can't we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need to override config read? Regards, BALATON Zoltan
Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben: > On Mon, 20 Nov 2023, Kevin Wolf wrote: > > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: > > > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > > > > The only difference I can think of regarding the BAR zeroing is that the > > > > BMDMA BAR is zeroed here. Does the following diff fix things? > > > > > > This helps, with this the latest driver does not crash but still reads BAR4 > > > as 0 instead of 0xcc00 so UDMA won't work but at least it boots. > > > > And disabling only the first four BARs is actually what the spec says, > > too. So I'll make this change to the queued patches. > > > > If I understand correctly, UDMA didn't work before this series either, > > so it's a separate goal and doing it in its own patch is best anyway. > > UDMA works with my original series, did not work with earlier versions of > this alternative from Mark but could be fixed up on top unless Mark can send > a v4 now. > > > As we don't seem to have a good place to set a default, maybe just > > overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in > > compatibility mode is enough? > > I could give that a try and see if that helps but all this > via_ide_cfg_read() seems like an unnecessary complication to me. Why can't > we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need > to override config read? I would be fine with setting 0xcc00 as the default value for BAR 4, but as you said yourself, we can't do that in reset because it will be overwritten by the PCI core code. Where else could we meaningfully do that? As far as I understand, we don't have any hint that the native/compatibility mode switch resets it on real hardware, so I'm hesitant to do it there (and if the guest OS doesn't even switch, it would never get set). As for BAR 0-3, didn't we conclude that the via device still accepts I/O to the configured addresses even though they read as zeros? Having inconsistent config space and PCIIORegion seems like a bad idea, the next call to pci_update_mappings() would break it. Kevin
On Mon, 20 Nov 2023, Kevin Wolf wrote: > Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben: >> On Mon, 20 Nov 2023, Kevin Wolf wrote: >>> Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: >>>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >>>>> The only difference I can think of regarding the BAR zeroing is that the >>>>> BMDMA BAR is zeroed here. Does the following diff fix things? >>>> >>>> This helps, with this the latest driver does not crash but still reads BAR4 >>>> as 0 instead of 0xcc00 so UDMA won't work but at least it boots. >>> >>> And disabling only the first four BARs is actually what the spec says, >>> too. So I'll make this change to the queued patches. >>> >>> If I understand correctly, UDMA didn't work before this series either, >>> so it's a separate goal and doing it in its own patch is best anyway. >> >> UDMA works with my original series, did not work with earlier versions of >> this alternative from Mark but could be fixed up on top unless Mark can send >> a v4 now. >> >>> As we don't seem to have a good place to set a default, maybe just >>> overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in >>> compatibility mode is enough? >> >> I could give that a try and see if that helps but all this >> via_ide_cfg_read() seems like an unnecessary complication to me. Why can't >> we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need >> to override config read? > > I would be fine with setting 0xcc00 as the default value for BAR 4, but > as you said yourself, we can't do that in reset because it will be > overwritten by the PCI core code. Where else could we meaningfully do > that? As far as I understand, we don't have any hint that the > native/compatibility mode switch resets it on real hardware, so I'm > hesitant to do it there (and if the guest OS doesn't even switch, it > would never get set). Luckily machines which need legacy mode also seem to set it explicitly on startup so we can set the defaults there. The check to see if something changed the BARs before is enough to avoid breaking it when legacy mode is set after native mode which does not seem to reset BARs according to how pegasos2 Linux behaves that sets legacy mode after firmware set native and proframmed BARs but the keep using BAR addresses. The AmigaOne I think just uses the default values with setting legacy mode doing nothing as that's the default but we can detect this as setting legacy mode with BARs unset so that's a good place to set default values which is what my patch did and I added a lot of comments trying to explain this. > As for BAR 0-3, didn't we conclude that the via device still accepts I/O > to the configured addresses even though they read as zeros? Having > inconsistent config space and PCIIORegion seems like a bad idea, the > next call to pci_update_mappings() would break it. I don't quite get this but then we could also just leave BARs alone and it would still work. It probably does not matter what it reads back when the device is in legacy mode. What would call pci_update_mappings() if device is in legacy and if something switches it to native it will very likely also program BARs. I can't imagine what would want to turn on native mode without trying to use a PCI driver and program BARs. Regards, BALATON Zoltan
Am 20.11.2023 um 16:11 hat BALATON Zoltan geschrieben: > On Mon, 20 Nov 2023, Kevin Wolf wrote: > > Am 20.11.2023 um 14:47 hat BALATON Zoltan geschrieben: > > > On Mon, 20 Nov 2023, Kevin Wolf wrote: > > > > Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben: > > > > > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > > > > > > The only difference I can think of regarding the BAR zeroing is that the > > > > > > BMDMA BAR is zeroed here. Does the following diff fix things? > > > > > > > > > > This helps, with this the latest driver does not crash but still reads BAR4 > > > > > as 0 instead of 0xcc00 so UDMA won't work but at least it boots. > > > > > > > > And disabling only the first four BARs is actually what the spec says, > > > > too. So I'll make this change to the queued patches. > > > > > > > > If I understand correctly, UDMA didn't work before this series either, > > > > so it's a separate goal and doing it in its own patch is best anyway. > > > > > > UDMA works with my original series, did not work with earlier versions of > > > this alternative from Mark but could be fixed up on top unless Mark can send > > > a v4 now. > > > > > > > As we don't seem to have a good place to set a default, maybe just > > > > overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in > > > > compatibility mode is enough? > > > > > > I could give that a try and see if that helps but all this > > > via_ide_cfg_read() seems like an unnecessary complication to me. Why can't > > > we just set the BARs (o for BAR1-3 and default for BAR4) then we don't need > > > to override config read? > > > > I would be fine with setting 0xcc00 as the default value for BAR 4, but > > as you said yourself, we can't do that in reset because it will be > > overwritten by the PCI core code. Where else could we meaningfully do > > that? As far as I understand, we don't have any hint that the > > native/compatibility mode switch resets it on real hardware, so I'm > > hesitant to do it there (and if the guest OS doesn't even switch, it > > would never get set). > > Luckily machines which need legacy mode also seem to set it explicitly > on startup so we can set the defaults there. With machines, you mean the QEMU board code? Where does this happen? Or just what guests usually do? > The check to see if something changed the BARs before is enough to > avoid breaking it when legacy mode is set after native mode which does > not seem to reset BARs according to how pegasos2 Linux behaves that > sets legacy mode after firmware set native and proframmed BARs but the > keep using BAR addresses. The AmigaOne I think just uses the default > values with setting legacy mode doing nothing as that's the default > but we can detect this as setting legacy mode with BARs unset so > that's a good place to set default values which is what my patch did > and I added a lot of comments trying to explain this. I guess it's a question of your philosophy if you focus on just making a specific set of OSes work, or if you focus on trying to do what hardware actually does. Of course, figuring out what hardware actually does proved a bit harder than I would have hoped in this case. > > As for BAR 0-3, didn't we conclude that the via device still accepts I/O > > to the configured addresses even though they read as zeros? Having > > inconsistent config space and PCIIORegion seems like a bad idea, the > > next call to pci_update_mappings() would break it. > > I don't quite get this but then we could also just leave BARs alone > and it would still work. Didn't we have config space dumps from real hardware that showed that this isn't what it does? Otherwise, this would be simpler indeed. Of course, the spec also mandates that the values in the first four BARs are ignored in compatibility mode (i.e. we would have to unregister the memory regions), but we already figured that the via controller doesn't do this. So that's something we would only have to solve if we want pci.c code to be actually generic, but not for via. > It probably does not matter what it reads back when the device is in > legacy mode. What would call pci_update_mappings() if device is in > legacy and if something switches it to native it will very likely also > program BARs. I can't imagine what would want to turn on native mode > without trying to use a PCI driver and program BARs. For example, I see vmstate_info_pci_config -> get_pci_config_device -> pci_update_mappings. Would live migration be enough to trigger this? Maybe the relevant machines don't even support live migration (I haven't checked), but it just doesn't feel robust to have inconsistent values in data structures that are supposed to be in sync. Kevin
On Mon, 20 Nov 2023, BALATON Zoltan wrote:
> On Mon, 20 Nov 2023, Kevin Wolf wrote:
>> Am 20.11.2023 um 14:09 hat BALATON Zoltan geschrieben:
>>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote:
>>>> On 19/11/2023 21:43, BALATON Zoltan wrote:
>>>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote:
>>>>>> This series adds a simple implementation of legacy/native mode
>>>>>> switching for PCI
>>>>>> IDE controllers and updates the via-ide device to use it.
>>>>>>
>>>>>> The approach I take here is to add a new pci_ide_update_mode()
>>>>>> function which handles
>>>>>> management of the PCI BARs and legacy IDE ioports for each mode
>>>>>> to avoid exposing
>>>>>> details of the internal logic to individual PCI IDE controllers.
>>>>>>
>>>>>> As noted in [1] this is extracted from a local WIP branch I have
>>>>>> which contains
>>>>>> further work in this area. However for the moment I've kept it simple
>>>>>> (and
>>>>>> restricted it to the via-ide device) which is good enough for Zoltan's
>>>>>> PPC
>>>>>> images whilst paving the way for future improvements after 8.2.
>>>>>>
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>>
>>>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html
>>>>>>
>>>>>> v3:
>>>>>> - Rebase onto master
>>>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to
>>>>>> prevent duplication in
>>>>>> hw/ide/pci.c
>>>>>> - Don't zero BARs when switching from native mode to legacy
>>>>>> mode, instead always force
>>>>>> them to read zero as suggested in the PCI IDE specification
>>>>>> (note: this also appears
>>>>>> to fix the fuloong2e machine booting from IDE)
>>>>>
>>>>> Not sure you're getting this, see also:
>>>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html
>>>>> but this seems to break latest version of the AmigaOS driver for
>>>>> some reason. I assume this is the BAR zeroing that causes this as it
>>>>> works with v2 series and nothing else changed in v3 that could cause
>>>>> this. Testing was done by Rene Engel, cc'd so maybe he can add more
>>>>> info. It seems to work with my patch that sets BARs to legacy values
>>>>> and with v2 that sets them to 0 but not with v3 which should also
>>>>> read 0 but maybe something is off here.
>>>>
>>>> I've been AFK for a few days, so just starting to catch up on various
>>>> bits and pieces.
>>>
>>> OK just wasn't sure if you saw my emails at all as it happened before that
>>> some spam filters disliked my mail server and put messages in the spam
>>> folder.
>>>
>>>> The only difference I can think of regarding the BAR zeroing is that the
>>>> BMDMA BAR is zeroed here. Does the following diff fix things?
>>>
>>> This helps, with this the latest driver does not crash but still reads
>>> BAR4
>>> as 0 instead of 0xcc00 so UDMA won't work but at least it boots.
>>
>> And disabling only the first four BARs is actually what the spec says,
>> too. So I'll make this change to the queued patches.
>>
>> If I understand correctly, UDMA didn't work before this series either,
>> so it's a separate goal and doing it in its own patch is best anyway.
>
> UDMA works with my original series, did not work with earlier versions of
> this alternative from Mark but could be fixed up on top unless Mark can send
> a v4 now.
>
>> As we don't seem to have a good place to set a default, maybe just
>> overriding it in via_ide_cfg_read(), too, and making it return 0xcc01 in
>> compatibility mode is enough?
>
> I could give that a try and see if that helps but all this via_ide_cfg_read()
> seems like an unnecessary complication to me. Why can't we just set the BARs
> (o for BAR1-3 and default for BAR4) then we don't need to override config
> read?
Seems to work with this:
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 47223b1268..214ebe48a1 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -164,14 +164,17 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t addr, int len)
uint8_t mode = pd->config[PCI_CLASS_PROG];
if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
- PCI_BASE_ADDRESS_0, 24)) {
+ PCI_BASE_ADDRESS_0, 16)) {
/* BARs always read back zero in legacy mode */
for (int i = addr; i < addr + len; i++) {
- if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 24) {
+ if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
val &= ~(0xffULL << ((i - addr) << 3));
}
}
}
+ if (addr == PCI_BASE_ADDRESS_4) {
+ val = 0xcc01;
+ }
return val;
}
But I still think all the above mess could be avoided and just set the BAR
value to 0 for BAR0-3 and to 0xcc01 for BAR4 instead should be enough and
would not mess up the device model unnecessarily.
Regards,
BALATON Zoltan
On 19/11/2023 21:43, BALATON Zoltan wrote: > On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >> This series adds a simple implementation of legacy/native mode switching for PCI >> IDE controllers and updates the via-ide device to use it. >> >> The approach I take here is to add a new pci_ide_update_mode() function which handles >> management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing >> details of the internal logic to individual PCI IDE controllers. >> >> As noted in [1] this is extracted from a local WIP branch I have which contains >> further work in this area. However for the moment I've kept it simple (and >> restricted it to the via-ide device) which is good enough for Zoltan's PPC >> images whilst paving the way for future improvements after 8.2. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >> >> v3: >> - Rebase onto master >> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent duplication in >> hw/ide/pci.c >> - Don't zero BARs when switching from native mode to legacy mode, instead always force >> them to read zero as suggested in the PCI IDE specification (note: this also appears >> to fix the fuloong2e machine booting from IDE) > > Not sure you're getting this, see also: > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html > but this seems to break latest version of the AmigaOS driver for some reason. I > assume this is the BAR zeroing that causes this as it works with v2 series and > nothing else changed in v3 that could cause this. Testing was done by Rene Engel, > cc'd so maybe he can add more info. It seems to work with my patch that sets BARs to > legacy values and with v2 that sets them to 0 but not with v3 which should also read > 0 but maybe something is off here. Is this document here accurate as to how it works on real hardware? https://intuitionbase.com/hints.php. I can't understand why the base OS is attempting any access to BAR 4 if BMDMA isn't enabled by default on real hardware due to hardware bugs. Are we sure that the idetool hacks given in the link above to enable BMDMA haven't already been run on the AmigaOS install when testing an earlier version of the patches? Finally is there a bootable AmigaOS demo ISO somewhere that can be used for testing? ATB, Mark.
On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > On 19/11/2023 21:43, BALATON Zoltan wrote: >> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>> This series adds a simple implementation of legacy/native mode switching >>> for PCI >>> IDE controllers and updates the via-ide device to use it. >>> >>> The approach I take here is to add a new pci_ide_update_mode() function >>> which handles >>> management of the PCI BARs and legacy IDE ioports for each mode to avoid >>> exposing >>> details of the internal logic to individual PCI IDE controllers. >>> >>> As noted in [1] this is extracted from a local WIP branch I have which >>> contains >>> further work in this area. However for the moment I've kept it simple (and >>> restricted it to the via-ide device) which is good enough for Zoltan's PPC >>> images whilst paving the way for future improvements after 8.2. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> >>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>> >>> v3: >>> - Rebase onto master >>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent >>> duplication in >>> hw/ide/pci.c >>> - Don't zero BARs when switching from native mode to legacy mode, instead >>> always force >>> them to read zero as suggested in the PCI IDE specification (note: this >>> also appears >>> to fix the fuloong2e machine booting from IDE) >> >> Not sure you're getting this, see also: >> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >> but this seems to break latest version of the AmigaOS driver for some >> reason. I assume this is the BAR zeroing that causes this as it works with >> v2 series and nothing else changed in v3 that could cause this. Testing was >> done by Rene Engel, cc'd so maybe he can add more info. It seems to work >> with my patch that sets BARs to legacy values and with v2 that sets them to >> 0 but not with v3 which should also read 0 but maybe something is off here. > > Is this document here accurate as to how it works on real hardware? > https://intuitionbase.com/hints.php. That should be about right. On QEMU the U-boot env vars won't work because NVRAM is not emulated yet so they can't be saved but you can call idetool from startup-sequence boot script instead for same effect (UDMA is enabled a bit later with that but after that it's the same). > I can't understand why the base OS is attempting any access to BAR 4 if BMDMA > isn't enabled by default on real hardware due to hardware bugs. Real hardware had problems with DMA (the VIA chip was also infamous for it on PC hardware and later also the ArticiaS was found to have its own problems) so the default is to use IDE in PIO mode and UDMA has to be enabled manually. But if it works (and it should on QEMU) it's much faster so we want to enable it. > Are we sure > that the idetool hacks given in the link above to enable BMDMA haven't > already been run on the AmigaOS install when testing an earlier version of > the patches? It was tested with my original series and works with that as my patch sets the default vaules for BARs and the driver reads it correctly. Then we tested your series too and I've noted for v2 already that it misses the degault value for BAR4. Other BARs don't matter as it will apparently use ISA IDE ports when it gets 0 or it knows that in legacy mode these shoud be the port values but seems to read BAR4 for UDMA and only works if the right default value is there, otherwise it lists DMA BAR 0 on start. > Finally is there a bootable AmigaOS demo ISO somewhere that can be used for > testing? Unfortunately AmigaOS4 is only available commercially, no free demo is avaialable. Regards, BALATON Zoltan
On Mon, 20 Nov 2023, BALATON Zoltan wrote: > On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >> On 19/11/2023 21:43, BALATON Zoltan wrote: >>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>>> This series adds a simple implementation of legacy/native mode switching >>>> for PCI >>>> IDE controllers and updates the via-ide device to use it. >>>> >>>> The approach I take here is to add a new pci_ide_update_mode() function >>>> which handles >>>> management of the PCI BARs and legacy IDE ioports for each mode to avoid >>>> exposing >>>> details of the internal logic to individual PCI IDE controllers. >>>> >>>> As noted in [1] this is extracted from a local WIP branch I have which >>>> contains >>>> further work in this area. However for the moment I've kept it simple >>>> (and >>>> restricted it to the via-ide device) which is good enough for Zoltan's >>>> PPC >>>> images whilst paving the way for future improvements after 8.2. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> >>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>> >>>> v3: >>>> - Rebase onto master >>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent >>>> duplication in >>>> hw/ide/pci.c >>>> - Don't zero BARs when switching from native mode to legacy mode, instead >>>> always force >>>> them to read zero as suggested in the PCI IDE specification (note: this >>>> also appears >>>> to fix the fuloong2e machine booting from IDE) >>> >>> Not sure you're getting this, see also: >>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >>> but this seems to break latest version of the AmigaOS driver for some >>> reason. I assume this is the BAR zeroing that causes this as it works with >>> v2 series and nothing else changed in v3 that could cause this. Testing >>> was done by Rene Engel, cc'd so maybe he can add more info. It seems to >>> work with my patch that sets BARs to legacy values and with v2 that sets >>> them to 0 but not with v3 which should also read 0 but maybe something is >>> off here. >> >> Is this document here accurate as to how it works on real hardware? >> https://intuitionbase.com/hints.php. > > That should be about right. On QEMU the U-boot env vars won't work because > NVRAM is not emulated yet so they can't be saved but you can call idetool > from startup-sequence boot script instead for same effect (UDMA is enabled a > bit later with that but after that it's the same). > >> I can't understand why the base OS is attempting any access to BAR 4 if >> BMDMA isn't enabled by default on real hardware due to hardware bugs. > > Real hardware had problems with DMA (the VIA chip was also infamous for it on > PC hardware and later also the ArticiaS was found to have its own problems) > so the default is to use IDE in PIO mode and UDMA has to be enabled manually. > But if it works (and it should on QEMU) it's much faster so we want to enable > it. > >> Are we sure that the idetool hacks given in the link above to enable BMDMA >> haven't already been run on the AmigaOS install when testing an earlier >> version of the patches? > > It was tested with my original series and works with that as my patch sets > the default vaules for BARs and the driver reads it correctly. Then we tested > your series too and I've noted for v2 already that it misses the degault > value for BAR4. Other BARs don't matter as it will apparently use ISA IDE > ports when it gets 0 or it knows that in legacy mode these shoud be the port > values but seems to read BAR4 for UDMA and only works if the right default > value is there, otherwise it lists DMA BAR 0 on start. This probably wasn't clear so what I mean is: a1ide.device 53.22 (28.6.2017) [a1ide/dev_init] Found chip #0 [a1ide/init_port] ---> Port 0 [a1ide/init_port] IOBase 000001F0, AltBase 000003F6 [a1ide/init_port] bmcr_base 0000CC00 [a1ide/init_port] MMIOBase 00000000 This is with my patch: https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ With yours bmcr_base is 0 and then when enabling UDMA with idetool it hangs as it needs this value from BAR4. Regards, BALATON Zoltan >> Finally is there a bootable AmigaOS demo ISO somewhere that can be used for >> testing? > > Unfortunately AmigaOS4 is only available commercially, no free demo is > avaialable. > > Regards, > BALATON Zoltan
On 20/11/2023 13:30, BALATON Zoltan wrote: > On Mon, 20 Nov 2023, BALATON Zoltan wrote: >> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >>> On 19/11/2023 21:43, BALATON Zoltan wrote: >>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>>>> This series adds a simple implementation of legacy/native mode switching for PCI >>>>> IDE controllers and updates the via-ide device to use it. >>>>> >>>>> The approach I take here is to add a new pci_ide_update_mode() function which >>>>> handles >>>>> management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing >>>>> details of the internal logic to individual PCI IDE controllers. >>>>> >>>>> As noted in [1] this is extracted from a local WIP branch I have which contains >>>>> further work in this area. However for the moment I've kept it simple (and >>>>> restricted it to the via-ide device) which is good enough for Zoltan's PPC >>>>> images whilst paving the way for future improvements after 8.2. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> >>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>>> >>>>> v3: >>>>> - Rebase onto master >>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent >>>>> duplication in >>>>> hw/ide/pci.c >>>>> - Don't zero BARs when switching from native mode to legacy mode, instead always >>>>> force >>>>> them to read zero as suggested in the PCI IDE specification (note: this also >>>>> appears >>>>> to fix the fuloong2e machine booting from IDE) >>>> >>>> Not sure you're getting this, see also: >>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >>>> but this seems to break latest version of the AmigaOS driver for some reason. I >>>> assume this is the BAR zeroing that causes this as it works with v2 series and >>>> nothing else changed in v3 that could cause this. Testing was done by Rene Engel, >>>> cc'd so maybe he can add more info. It seems to work with my patch that sets BARs >>>> to legacy values and with v2 that sets them to 0 but not with v3 which should >>>> also read 0 but maybe something is off here. >>> >>> Is this document here accurate as to how it works on real hardware? >>> https://intuitionbase.com/hints.php. >> >> That should be about right. On QEMU the U-boot env vars won't work because NVRAM is >> not emulated yet so they can't be saved but you can call idetool from >> startup-sequence boot script instead for same effect (UDMA is enabled a bit later >> with that but after that it's the same). >> >>> I can't understand why the base OS is attempting any access to BAR 4 if BMDMA >>> isn't enabled by default on real hardware due to hardware bugs. >> >> Real hardware had problems with DMA (the VIA chip was also infamous for it on PC >> hardware and later also the ArticiaS was found to have its own problems) so the >> default is to use IDE in PIO mode and UDMA has to be enabled manually. But if it >> works (and it should on QEMU) it's much faster so we want to enable it. >> >>> Are we sure that the idetool hacks given in the link above to enable BMDMA haven't >>> already been run on the AmigaOS install when testing an earlier version of the >>> patches? >> >> It was tested with my original series and works with that as my patch sets the >> default vaules for BARs and the driver reads it correctly. Then we tested your >> series too and I've noted for v2 already that it misses the degault value for BAR4. >> Other BARs don't matter as it will apparently use ISA IDE ports when it gets 0 or >> it knows that in legacy mode these shoud be the port values but seems to read BAR4 >> for UDMA and only works if the right default value is there, otherwise it lists DMA >> BAR 0 on start. > > This probably wasn't clear so what I mean is: > > a1ide.device 53.22 (28.6.2017) > [a1ide/dev_init] Found chip #0 > [a1ide/init_port] ---> Port 0 > [a1ide/init_port] IOBase 000001F0, AltBase 000003F6 > [a1ide/init_port] bmcr_base 0000CC00 > [a1ide/init_port] MMIOBase 00000000 > > This is with my patch: > https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ With yours bmcr_base is 0 and then when enabling UDMA with idetool it hangs as it needs this value from BAR4. Right, so what you're saying is that AmigaOS will run fine with the v3 series out-of-the-box, but only fails when you try and enable UDMA with idetool (which is explicitly changing the configuration from the AmigaOS default)? ATB, Mark.
On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: > On 20/11/2023 13:30, BALATON Zoltan wrote: >> On Mon, 20 Nov 2023, BALATON Zoltan wrote: >>> On Mon, 20 Nov 2023, Mark Cave-Ayland wrote: >>>> On 19/11/2023 21:43, BALATON Zoltan wrote: >>>>> On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >>>>>> This series adds a simple implementation of legacy/native mode >>>>>> switching for PCI >>>>>> IDE controllers and updates the via-ide device to use it. >>>>>> >>>>>> The approach I take here is to add a new pci_ide_update_mode() function >>>>>> which handles >>>>>> management of the PCI BARs and legacy IDE ioports for each mode to >>>>>> avoid exposing >>>>>> details of the internal logic to individual PCI IDE controllers. >>>>>> >>>>>> As noted in [1] this is extracted from a local WIP branch I have which >>>>>> contains >>>>>> further work in this area. However for the moment I've kept it simple >>>>>> (and >>>>>> restricted it to the via-ide device) which is good enough for Zoltan's >>>>>> PPC >>>>>> images whilst paving the way for future improvements after 8.2. >>>>>> >>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>> >>>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>>>> >>>>>> v3: >>>>>> - Rebase onto master >>>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent >>>>>> duplication in >>>>>> hw/ide/pci.c >>>>>> - Don't zero BARs when switching from native mode to legacy mode, >>>>>> instead always force >>>>>> them to read zero as suggested in the PCI IDE specification (note: >>>>>> this also appears >>>>>> to fix the fuloong2e machine booting from IDE) >>>>> >>>>> Not sure you're getting this, see also: >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html >>>>> but this seems to break latest version of the AmigaOS driver for some >>>>> reason. I assume this is the BAR zeroing that causes this as it works >>>>> with v2 series and nothing else changed in v3 that could cause this. >>>>> Testing was done by Rene Engel, cc'd so maybe he can add more info. It >>>>> seems to work with my patch that sets BARs to legacy values and with v2 >>>>> that sets them to 0 but not with v3 which should also read 0 but maybe >>>>> something is off here. >>>> >>>> Is this document here accurate as to how it works on real hardware? >>>> https://intuitionbase.com/hints.php. >>> >>> That should be about right. On QEMU the U-boot env vars won't work because >>> NVRAM is not emulated yet so they can't be saved but you can call idetool >>> from startup-sequence boot script instead for same effect (UDMA is enabled >>> a bit later with that but after that it's the same). >>> >>>> I can't understand why the base OS is attempting any access to BAR 4 if >>>> BMDMA isn't enabled by default on real hardware due to hardware bugs. >>> >>> Real hardware had problems with DMA (the VIA chip was also infamous for it >>> on PC hardware and later also the ArticiaS was found to have its own >>> problems) so the default is to use IDE in PIO mode and UDMA has to be >>> enabled manually. But if it works (and it should on QEMU) it's much faster >>> so we want to enable it. >>> >>>> Are we sure that the idetool hacks given in the link above to enable >>>> BMDMA haven't already been run on the AmigaOS install when testing an >>>> earlier version of the patches? >>> >>> It was tested with my original series and works with that as my patch sets >>> the default vaules for BARs and the driver reads it correctly. Then we >>> tested your series too and I've noted for v2 already that it misses the >>> degault value for BAR4. Other BARs don't matter as it will apparently use >>> ISA IDE ports when it gets 0 or it knows that in legacy mode these shoud >>> be the port values but seems to read BAR4 for UDMA and only works if the >>> right default value is there, otherwise it lists DMA BAR 0 on start. >> >> This probably wasn't clear so what I mean is: >> >> a1ide.device 53.22 (28.6.2017) >> [a1ide/dev_init] Found chip #0 >> [a1ide/init_port] ---> Port 0 >> [a1ide/init_port] IOBase 000001F0, AltBase 000003F6 >> [a1ide/init_port] bmcr_base 0000CC00 >> [a1ide/init_port] MMIOBase 00000000 >> >> This is with my patch: >> https://patchew.org/QEMU/cover.1697661160.git.balaton@eik.bme.hu/4095e01f4596e77a478759161ae736f0c398600a.1697661160.git.balaton@eik.bme.hu/ >> With yours bmcr_base is 0 and then when enabling UDMA with idetool it hangs >> as it needs this value from BAR4. > > Right, so what you're saying is that AmigaOS will run fine with the v3 series > out-of-the-box, but only fails when you try and enable UDMA with idetool > (which is explicitly changing the configuration from the AmigaOS default)? No, what I said is it boots with v2 but fails when enabling UDMA but does not even boot with v3 as it crashes even before printing the above debug logs with v3. The changes you've and Kevin proposed fix v3 to boot and allow UDMA too which I've tested and sent another reply about. Regards, BALATON Zoltan
On Sun, 19 Nov 2023, BALATON Zoltan wrote: > On Thu, 16 Nov 2023, Mark Cave-Ayland wrote: >> This series adds a simple implementation of legacy/native mode switching >> for PCI >> IDE controllers and updates the via-ide device to use it. >> >> The approach I take here is to add a new pci_ide_update_mode() function >> which handles >> management of the PCI BARs and legacy IDE ioports for each mode to avoid >> exposing >> details of the internal logic to individual PCI IDE controllers. >> >> As noted in [1] this is extracted from a local WIP branch I have which >> contains >> further work in this area. However for the moment I've kept it simple (and >> restricted it to the via-ide device) which is good enough for Zoltan's PPC >> images whilst paving the way for future improvements after 8.2. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >> >> v3: >> - Rebase onto master >> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent >> duplication in >> hw/ide/pci.c >> - Don't zero BARs when switching from native mode to legacy mode, instead >> always force >> them to read zero as suggested in the PCI IDE specification (note: this >> also appears >> to fix the fuloong2e machine booting from IDE) > > Not sure you're getting this, see also: > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg04167.html > but this seems to break latest version of the AmigaOS driver for some reason. > I assume this is the BAR zeroing that causes this as it works with v2 series > and nothing else changed in v3 that could cause this. Testing was done by > Rene Engel, cc'd so maybe he can add more info. It seems to work with my > patch that sets BARs to legacy values and with v2 that sets them to 0 but not > with v3 which should also read 0 but maybe something is off here. The change log of the AmigaOS driver version which only works with v2 of this series says: "Replaced ReadConfigLong(PCI_BASE_ADDRESS_x) calls with their PCI interface variant GetResourceRange(x)->BaseAddress to get the correct base address instead of the *bus* base address." but not sure what that means. Apparently it reads BAR values differently and crashes before finding the IDE ports with v3 but not with v2. As it crashes it does not even print debug logs to check what it read. (Additionally would also need the default value for BMDMA BAR4 as I wrote before but that's disabled by default so only an issue if one tries to enable it. Would be nics to have though as UDMA is faster.) Regards, BALATON Zoltan > >> - Add comments in pci_ide_update_mode() suggested by Kevin >> - Drop the existing R-B and T-B tags: whilst this passes my local tests, >> the behaviour >> around zero BARs feels different enough here >> >> v2: >> - Rebase onto master >> - Mask the bottom 4 bits of PCI_CLASS_PROG in pci_ide_update_mode() in >> patch 1 >> - Add patch 2 to remove the default BAR addresses to avoid confusion >> - Don't set PCI_INTERRUPT_PIN directly in via_ide_reset() as it is already >> set >> by pci_ide_update_mode() in patch 3, and reword the commit message >> accordingly >> - Add Tested-By tags from Zoltan and Bernhard >> >> >> Mark Cave-Ayland (4): >> ide/ioport: move ide_portio_list[] and ide_portio_list2[] definitions >> to IDE core >> ide/pci: introduce pci_ide_update_mode() function >> ide/via: don't attempt to set default BAR addresses >> hw/ide/via: implement legacy/native mode switching >> >> hw/ide/core.c | 12 ++++++ >> hw/ide/ioport.c | 12 ------ >> hw/ide/pci.c | 84 +++++++++++++++++++++++++++++++++++++++ >> hw/ide/via.c | 44 ++++++++++++++++---- >> include/hw/ide/internal.h | 3 ++ >> include/hw/ide/pci.h | 1 + >> 6 files changed, 137 insertions(+), 19 deletions(-) >> >> > >
Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben: > This series adds a simple implementation of legacy/native mode switching for PCI > IDE controllers and updates the via-ide device to use it. > > The approach I take here is to add a new pci_ide_update_mode() function which handles > management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing > details of the internal logic to individual PCI IDE controllers. > > As noted in [1] this is extracted from a local WIP branch I have which contains > further work in this area. However for the moment I've kept it simple (and > restricted it to the via-ide device) which is good enough for Zoltan's PPC > images whilst paving the way for future improvements after 8.2. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html > > v3: > - Rebase onto master > - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent duplication in > hw/ide/pci.c > - Don't zero BARs when switching from native mode to legacy mode, instead always force > them to read zero as suggested in the PCI IDE specification (note: this also appears > to fix the fuloong2e machine booting from IDE) > - Add comments in pci_ide_update_mode() suggested by Kevin > - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour > around zero BARs feels different enough here Thanks, applied to the block branch. Kevin
On Thu, 16 Nov 2023, Kevin Wolf wrote: > Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben: >> This series adds a simple implementation of legacy/native mode switching for PCI >> IDE controllers and updates the via-ide device to use it. >> >> The approach I take here is to add a new pci_ide_update_mode() function which handles >> management of the PCI BARs and legacy IDE ioports for each mode to avoid exposing >> details of the internal logic to individual PCI IDE controllers. >> >> As noted in [1] this is extracted from a local WIP branch I have which contains >> further work in this area. However for the moment I've kept it simple (and >> restricted it to the via-ide device) which is good enough for Zoltan's PPC >> images whilst paving the way for future improvements after 8.2. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >> >> v3: >> - Rebase onto master >> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent duplication in >> hw/ide/pci.c >> - Don't zero BARs when switching from native mode to legacy mode, instead always force >> them to read zero as suggested in the PCI IDE specification (note: this also appears >> to fix the fuloong2e machine booting from IDE) >> - Add comments in pci_ide_update_mode() suggested by Kevin >> - Drop the existing R-B and T-B tags: whilst this passes my local tests, the behaviour >> around zero BARs feels different enough here > > Thanks, applied to the block branch. I feel a bit left out of this conversation... Did Google or some other spam filter decide again to filter my messages so you did not see them at all? Could you confitm that you've got my previous two replies in this thread so I know I'm not sending comments to /dev/null please? Regards, BALATON Zoltan
On Thu, 16 Nov 2023, BALATON Zoltan wrote: > On Thu, 16 Nov 2023, Kevin Wolf wrote: >> Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben: >>> This series adds a simple implementation of legacy/native mode switching >>> for PCI >>> IDE controllers and updates the via-ide device to use it. >>> >>> The approach I take here is to add a new pci_ide_update_mode() function >>> which handles >>> management of the PCI BARs and legacy IDE ioports for each mode to avoid >>> exposing >>> details of the internal logic to individual PCI IDE controllers. >>> >>> As noted in [1] this is extracted from a local WIP branch I have which >>> contains >>> further work in this area. However for the moment I've kept it simple (and >>> restricted it to the via-ide device) which is good enough for Zoltan's PPC >>> images whilst paving the way for future improvements after 8.2. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> >>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>> >>> v3: >>> - Rebase onto master >>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to prevent >>> duplication in >>> hw/ide/pci.c >>> - Don't zero BARs when switching from native mode to legacy mode, instead >>> always force >>> them to read zero as suggested in the PCI IDE specification (note: this >>> also appears >>> to fix the fuloong2e machine booting from IDE) >>> - Add comments in pci_ide_update_mode() suggested by Kevin >>> - Drop the existing R-B and T-B tags: whilst this passes my local tests, >>> the behaviour >>> around zero BARs feels different enough here >> >> Thanks, applied to the block branch. > > I feel a bit left out of this conversation... Did Google or some other spam > filter decide again to filter my messages so you did not see them at all? > Could you confitm that you've got my previous two replies in this thread so I > know I'm not sending comments to /dev/null please? Looks like there's some issue with these mails. They appear in the list archive but maybe not in people's mailboxes? Did any of you got this message and previous ones I've sent? https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03983.html Regards, BALATON Zoltan
Am 17.11.2023 um 15:23 hat BALATON Zoltan geschrieben: > On Thu, 16 Nov 2023, BALATON Zoltan wrote: > > On Thu, 16 Nov 2023, Kevin Wolf wrote: > > > Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben: > > > > This series adds a simple implementation of legacy/native mode > > > > switching for PCI > > > > IDE controllers and updates the via-ide device to use it. > > > > > > > > The approach I take here is to add a new pci_ide_update_mode() > > > > function which handles > > > > management of the PCI BARs and legacy IDE ioports for each mode > > > > to avoid exposing > > > > details of the internal logic to individual PCI IDE controllers. > > > > > > > > As noted in [1] this is extracted from a local WIP branch I have > > > > which contains > > > > further work in this area. However for the moment I've kept it simple (and > > > > restricted it to the via-ide device) which is good enough for Zoltan's PPC > > > > images whilst paving the way for future improvements after 8.2. > > > > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > > > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html > > > > > > > > v3: > > > > - Rebase onto master > > > > - Move ide_portio_list[] and ide_portio_list2[] to IDE core to > > > > prevent duplication in > > > > hw/ide/pci.c > > > > - Don't zero BARs when switching from native mode to legacy > > > > mode, instead always force > > > > them to read zero as suggested in the PCI IDE specification > > > > (note: this also appears > > > > to fix the fuloong2e machine booting from IDE) > > > > - Add comments in pci_ide_update_mode() suggested by Kevin > > > > - Drop the existing R-B and T-B tags: whilst this passes my > > > > local tests, the behaviour > > > > around zero BARs feels different enough here > > > > > > Thanks, applied to the block branch. > > > > I feel a bit left out of this conversation... Did Google or some other > > spam filter decide again to filter my messages so you did not see them > > at all? Could you confitm that you've got my previous two replies in > > this thread so I know I'm not sending comments to /dev/null please? > > Looks like there's some issue with these mails. They appear in the list > archive but maybe not in people's mailboxes? Did any of you got this message > and previous ones I've sent? > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html > https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03983.html I received it, but at least the second one only after sending the mail you replied to here. For your specific concern, it seems to me that resetting BAR 4 on a switch between compatibility mode and native mode is another via quirk and should therefore be implemented in via.c rather than pci.c? If I get a new version of the patches in the next few hours, I can update the queued patches before sending a pull request, but otherwise this can still be done in a separate patch. Kevin
On Mon, 20 Nov 2023, Kevin Wolf wrote: > Am 17.11.2023 um 15:23 hat BALATON Zoltan geschrieben: >> On Thu, 16 Nov 2023, BALATON Zoltan wrote: >>> On Thu, 16 Nov 2023, Kevin Wolf wrote: >>>> Am 16.11.2023 um 11:33 hat Mark Cave-Ayland geschrieben: >>>>> This series adds a simple implementation of legacy/native mode >>>>> switching for PCI >>>>> IDE controllers and updates the via-ide device to use it. >>>>> >>>>> The approach I take here is to add a new pci_ide_update_mode() >>>>> function which handles >>>>> management of the PCI BARs and legacy IDE ioports for each mode >>>>> to avoid exposing >>>>> details of the internal logic to individual PCI IDE controllers. >>>>> >>>>> As noted in [1] this is extracted from a local WIP branch I have >>>>> which contains >>>>> further work in this area. However for the moment I've kept it simple (and >>>>> restricted it to the via-ide device) which is good enough for Zoltan's PPC >>>>> images whilst paving the way for future improvements after 8.2. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> >>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg05403.html >>>>> >>>>> v3: >>>>> - Rebase onto master >>>>> - Move ide_portio_list[] and ide_portio_list2[] to IDE core to >>>>> prevent duplication in >>>>> hw/ide/pci.c >>>>> - Don't zero BARs when switching from native mode to legacy >>>>> mode, instead always force >>>>> them to read zero as suggested in the PCI IDE specification >>>>> (note: this also appears >>>>> to fix the fuloong2e machine booting from IDE) >>>>> - Add comments in pci_ide_update_mode() suggested by Kevin >>>>> - Drop the existing R-B and T-B tags: whilst this passes my >>>>> local tests, the behaviour >>>>> around zero BARs feels different enough here >>>> >>>> Thanks, applied to the block branch. >>> >>> I feel a bit left out of this conversation... Did Google or some other >>> spam filter decide again to filter my messages so you did not see them >>> at all? Could you confitm that you've got my previous two replies in >>> this thread so I know I'm not sending comments to /dev/null please? >> >> Looks like there's some issue with these mails. They appear in the list >> archive but maybe not in people's mailboxes? Did any of you got this message >> and previous ones I've sent? >> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03180.html >> https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03983.html > > I received it, but at least the second one only after sending the mail > you replied to here. > > For your specific concern, it seems to me that resetting BAR 4 on a > switch between compatibility mode and native mode is another via quirk > and should therefore be implemented in via.c rather than pci.c? I think in real chip the default value is there so no need to reset it when switching mode. Also the defeult mode is legacy so writing 0x8a first does nothing on real chip but we can't do that in QEMU becaise PCI code resets BARs after calling device reset method so we can't set reset degaults there but have to establish them on mode switch instead when the BARs are yet unset. Setting the BMDMA BAR4 in via.c is fine as well and could be done as a follow up so that's OK. > If I get a new version of the patches in the next few hours, I can > update the queued patches before sending a pull request, but otherwise > this can still be done in a separate patch. This v3 series does not work with latest AmigaOS driver as I wrote in my last reply. So maybe wait for a v4 or merge either v2 from Mark or my single patch fix for now. Those are better than v3 that's queued now. Regards, BALATON Zoltan
© 2016 - 2024 Red Hat, Inc.