1. Patchew
  2. QEMU
  3. [PATCH 0/2] ide: implement simple legacy/native mode switching for PCI IDE controllers
  4. View patch

[PATCH 2/2] hw/ide/via: implement legacy/native mode switching

Mark Cave-Ayland posted 2 patches 1 year, 1 month ago
Maintainers: John Snow <jsnow@redhat.com>
There is a newer version of this series
[PATCH 2/2] hw/ide/via: implement legacy/native mode switching
Posted by Mark Cave-Ayland 1 year, 1 month ago 
Allow the VIA IDE controller to switch between both legacy and native modes by
calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG
is updated.

This patch also moves the setting of PCI_CLASS_PROG from via_ide_realize() to
via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI device
reset occurs then the device configuration is always consistent.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/via.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..e6278dd419 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -28,6 +28,7 @@
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
+#include "qemu/range.h"
 #include "sysemu/dma.h"
 #include "hw/isa/vt82c686.h"
 #include "hw/ide/pci.h"
@@ -128,6 +129,9 @@ static void via_ide_reset(DeviceState *dev)
         ide_bus_reset(&d->bus[i]);
     }
 
+    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
+    pci_ide_update_mode(d);
+
     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
@@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
-    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
+    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
 
     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
     pci_set_long(pci_conf + 0x40, 0x0a090600);
@@ -159,6 +163,18 @@ static void via_ide_reset(DeviceState *dev)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+                              uint32_t val, int len)
+{
+    PCIIDEState *d = PCI_IDE(pd);
+
+    pci_default_write_config(pd, addr, val, len);
+
+    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
+        pci_ide_update_mode(d);
+    }
+}
+
 static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -166,7 +182,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     uint8_t *pci_conf = dev->config;
     int i;
 
-    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
     dev->wmask[PCI_INTERRUPT_LINE] = 0;
     dev->wmask[PCI_CLASS_PROG] = 5;
@@ -221,6 +236,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     /* Reason: only works as function of VIA southbridge */
     dc->user_creatable = false;
 
+    k->config_write = via_ide_cfg_write;
     k->realize = via_ide_realize;
     k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
-- 
2.39.2
Re: [PATCH 2/2] hw/ide/via: implement legacy/native mode switching
Posted by BALATON Zoltan 1 year, 1 month ago 
On Thu, 19 Oct 2023, Mark Cave-Ayland wrote:
> Allow the VIA IDE controller to switch between both legacy and native modes by
> calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG
> is updated.
>
> This patch also moves the setting of PCI_CLASS_PROG from via_ide_realize() to
> via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI device
> reset occurs then the device configuration is always consistent.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/ide/via.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index fff23803a6..e6278dd419 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -28,6 +28,7 @@
> #include "hw/pci/pci.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> +#include "qemu/range.h"
> #include "sysemu/dma.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/ide/pci.h"
> @@ -128,6 +129,9 @@ static void via_ide_reset(DeviceState *dev)
>         ide_bus_reset(&d->bus[i]);
>     }
>
> +    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
> +    pci_ide_update_mode(d);
> +
>     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
>     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>                  PCI_STATUS_DEVSEL_MEDIUM);
> @@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */

I'd remove these BAR defaults as they are not effective and aren't valid 
BAR values (missing IO bit) so likely would not work even if they weren't 
cleared but if you want to keep them maybe add a comment about that they 
will be zeroed by PCI reset anyway so only here for reference.

> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);

I did not get the commit message why it needs to 0 the intrerrupt pin 
because pci_ide_update_mode will set it above so I think this line could 
just use pci_set_byte() to set the PCI_INTERRUPT_LINE only now. (Although 
it still contradicts the VT8231 data sheet about the interrupt pin default 
value but I don't care as long as it works.)

Regards,
BALATON Zoltan

>
>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>     pci_set_long(pci_conf + 0x40, 0x0a090600);
> @@ -159,6 +163,18 @@ static void via_ide_reset(DeviceState *dev)
>     pci_set_long(pci_conf + 0xc0, 0x00020001);
> }
>
> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
> +                              uint32_t val, int len)
> +{
> +    PCIIDEState *d = PCI_IDE(pd);
> +
> +    pci_default_write_config(pd, addr, val, len);
> +
> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
> +        pci_ide_update_mode(d);
> +    }
> +}
> +
> static void via_ide_realize(PCIDevice *dev, Error **errp)
> {
>     PCIIDEState *d = PCI_IDE(dev);
> @@ -166,7 +182,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>     uint8_t *pci_conf = dev->config;
>     int i;
>
> -    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
>     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
>     dev->wmask[PCI_INTERRUPT_LINE] = 0;
>     dev->wmask[PCI_CLASS_PROG] = 5;
> @@ -221,6 +236,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>     /* Reason: only works as function of VIA southbridge */
>     dc->user_creatable = false;
>
> +    k->config_write = via_ide_cfg_write;
>     k->realize = via_ide_realize;
>     k->exit = via_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>
Re: [PATCH 2/2] hw/ide/via: implement legacy/native mode switching
Posted by Mark Cave-Ayland 1 year, 1 month ago 
On 20/10/2023 00:09, BALATON Zoltan wrote:

> On Thu, 19 Oct 2023, Mark Cave-Ayland wrote:
>> Allow the VIA IDE controller to switch between both legacy and native modes by
>> calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG
>> is updated.
>>
>> This patch also moves the setting of PCI_CLASS_PROG from via_ide_realize() to
>> via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI device
>> reset occurs then the device configuration is always consistent.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/ide/via.c | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index fff23803a6..e6278dd419 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -28,6 +28,7 @@
>> #include "hw/pci/pci.h"
>> #include "migration/vmstate.h"
>> #include "qemu/module.h"
>> +#include "qemu/range.h"
>> #include "sysemu/dma.h"
>> #include "hw/isa/vt82c686.h"
>> #include "hw/ide/pci.h"
>> @@ -128,6 +129,9 @@ static void via_ide_reset(DeviceState *dev)
>>         ide_bus_reset(&d->bus[i]);
>>     }
>>
>> +    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
>> +    pci_ide_update_mode(d);
>> +
>>     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
>>     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>                  PCI_STATUS_DEVSEL_MEDIUM);
>> @@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */
> 
> I'd remove these BAR defaults as they are not effective and aren't valid BAR values 
> (missing IO bit) so likely would not work even if they weren't cleared but if you 
> want to keep them maybe add a comment about that they will be zeroed by PCI reset 
> anyway so only here for reference.

I've had a look again at the other PCI IDE controllers and none of the others attempt 
to set default BAR addresses except for PIIX-IDE, and that is simply to indicate the 
BMDMA BAR is an I/O BAR. So in retrospect, I'll add a commit to remove these BAR 
addresses as you've suggested above.

>> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
> 
> I did not get the commit message why it needs to 0 the intrerrupt pin because 
> pci_ide_update_mode will set it above so I think this line could just use 
> pci_set_byte() to set the PCI_INTERRUPT_LINE only now. (Although it still contradicts 
> the VT8231 data sheet about the interrupt pin default value but I don't care as long 
> as it works.)

If you're happy then I'll make the change to use pci_set_byte().

> Regards,
> BALATON Zoltan
> 
>>
>>     /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
>>     pci_set_long(pci_conf + 0x40, 0x0a090600);
>> @@ -159,6 +163,18 @@ static void via_ide_reset(DeviceState *dev)
>>     pci_set_long(pci_conf + 0xc0, 0x00020001);
>> }
>>
>> +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
>> +                              uint32_t val, int len)
>> +{
>> +    PCIIDEState *d = PCI_IDE(pd);
>> +
>> +    pci_default_write_config(pd, addr, val, len);
>> +
>> +    if (range_covers_byte(addr, len, PCI_CLASS_PROG)) {
>> +        pci_ide_update_mode(d);
>> +    }
>> +}
>> +
>> static void via_ide_realize(PCIDevice *dev, Error **errp)
>> {
>>     PCIIDEState *d = PCI_IDE(dev);
>> @@ -166,7 +182,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>>     uint8_t *pci_conf = dev->config;
>>     int i;
>>
>> -    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
>>     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
>>     dev->wmask[PCI_INTERRUPT_LINE] = 0;
>>     dev->wmask[PCI_CLASS_PROG] = 5;
>> @@ -221,6 +236,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>>     /* Reason: only works as function of VIA southbridge */
>>     dc->user_creatable = false;
>>
>> +    k->config_write = via_ide_cfg_write;
>>     k->realize = via_ide_realize;
>>     k->exit = via_ide_exitfn;
>>     k->vendor_id = PCI_VENDOR_ID_VIA;


ATB,

Mark.
Re: [PATCH 2/2] hw/ide/via: implement legacy/native mode switching
Posted by BALATON Zoltan 1 year, 1 month ago 
On Mon, 23 Oct 2023, Mark Cave-Ayland wrote:
> On 20/10/2023 00:09, BALATON Zoltan wrote:
>> On Thu, 19 Oct 2023, Mark Cave-Ayland wrote:
>>> Allow the VIA IDE controller to switch between both legacy and native 
>>> modes by
>>> calling pci_ide_update_mode() to reconfigure the device whenever 
>>> PCI_CLASS_PROG
>>> is updated.
>>> 
>>> This patch also moves the setting of PCI_CLASS_PROG from via_ide_realize() 
>>> to
>>> via_ide_reset() and clears PCI_INTERRUPT_PIN to ensure that if a PCI 
>>> device
>>> reset occurs then the device configuration is always consistent.
>>> 
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hw/ide/via.c | 20 ++++++++++++++++++--
>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index fff23803a6..e6278dd419 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -28,6 +28,7 @@
>>> #include "hw/pci/pci.h"
>>> #include "migration/vmstate.h"
>>> #include "qemu/module.h"
>>> +#include "qemu/range.h"
>>> #include "sysemu/dma.h"
>>> #include "hw/isa/vt82c686.h"
>>> #include "hw/ide/pci.h"
>>> @@ -128,6 +129,9 @@ static void via_ide_reset(DeviceState *dev)
>>>         ide_bus_reset(&d->bus[i]);
>>>     }
>>> 
>>> +    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
>>> +    pci_ide_update_mode(d);
>>> +
>>>     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | 
>>> PCI_COMMAND_WAIT);
>>>     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>>                  PCI_STATUS_DEVSEL_MEDIUM);
>>> @@ -137,7 +141,7 @@ static void via_ide_reset(DeviceState *dev)
>>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170);
>>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374);
>>>     pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 
>>> 20-23h */
>> 
>> I'd remove these BAR defaults as they are not effective and aren't valid 
>> BAR values (missing IO bit) so likely would not work even if they weren't 
>> cleared but if you want to keep them maybe add a comment about that they 
>> will be zeroed by PCI reset anyway so only here for reference.
>
> I've had a look again at the other PCI IDE controllers and none of the others 
> attempt to set default BAR addresses except for PIIX-IDE, and that is simply 
> to indicate the BMDMA BAR is an I/O BAR. So in retrospect, I'll add a commit 
> to remove these BAR addresses as you've suggested above.
>
>>> -    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e);
>>> +    pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e);
>> 
>> I did not get the commit message why it needs to 0 the intrerrupt pin 
>> because pci_ide_update_mode will set it above so I think this line could 
>> just use pci_set_byte() to set the PCI_INTERRUPT_LINE only now. (Although 
>> it still contradicts the VT8231 data sheet about the interrupt pin default 
>> value but I don't care as long as it works.)
>
> If you're happy then I'll make the change to use pci_set_byte().

I think only setting the interrupt line here is enough but as I said I 
don't care that much as long as it works. Although the vt8231 data sheet 
says interrupt pin should be 1 the vt82c686b data sheet is not clear about 
that and your new mode setting func will override it anyway so maybe it's 
not needed to touch other values than the interuupt line here.

Regards,
BALATON Zoltan

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.