Patchwork [RFC,v4,03/12] isa: Provide set_state callback

login
register
mail settings
Submitter Andreas Färber
Date June 8, 2011, 6:55 p.m.
Message ID <1307559319-16183-4-git-send-email-andreas.faerber@web.de>
Download mbox | patch
Permalink /patch/99537/
State New
Headers show

Comments

Andreas Färber - June 8, 2011, 6:55 p.m.
To allow enabling/disabling present ISA devices without hotplug,
keep track of state and add a helper to avoid enabling twice.
Since the properties to be configured are defined at device level,
delegate the actual work to a callback function.

If no callback is supplied, the device can't be disabled.

Prepare VMSTATE_ISA_DEVICE for devices that support disabling.
Legacy devices never change their state and won't need this yet.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/hw.h      |   15 +++++++++++++++
 hw/isa-bus.c |   29 +++++++++++++++++++++++++++++
 hw/isa.h     |    4 ++++
 3 files changed, 48 insertions(+), 0 deletions(-)
Gerd Hoffmann - June 9, 2011, 10:39 a.m.
Hi,

> +const VMStateDescription vmstate_isa_device = {
> +    .name = "ISADevice",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_BOOL(enabled, ISADevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

That alone isn't enougth.  You also need to save iobase and irq.  And 
have a pre-load callback which disables the device + a post-load 
callback which re-enables them with the new settings.

I get the feeling that doing all this in the pc87312 emulation is easier 
as it needs to have this logic anyway for config register writes and you 
can probably reuse the code for loadvm pre- and postprocessing.

cheers,
   Gerd
Andreas Färber - June 9, 2011, 11:37 a.m.
Hi,

Am 09.06.2011 um 12:39 schrieb Gerd Hoffmann:

>> +const VMStateDescription vmstate_isa_device = {
>> +    .name = "ISADevice",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField []) {
>> +        VMSTATE_BOOL(enabled, ISADevice),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>
> That alone isn't enougth.  You also need to save iobase and irq.

Here it becomes tricky: the enabling was done at ISA level on your  
suggestion, but the iobase and isairq are kept in the ISADevices'  
state. Duplicates are however kept at ISA level, it's fairly ugly.

I would prefer to save the state where it is stored. In this case  
enabled is store at ISADevice level, so I added the VMState for it here.

Btw is 1 correct here or should an initial version be 0?

> And have a pre-load callback which disables the device + a post-load  
> callback which re-enables them with the new settings.

Right.

> I get the feeling that doing all this in the pc87312 emulation is  
> easier as it needs to have this logic anyway for config register  
> writes and you can probably reuse the code for loadvm pre- and  
> postprocessing.


Well, I wasn't looking for the easiest way but for the proper way. I  
don't want to let pc87312-internal state get out-of-sync with that of  
the aggregated devices. So we still need the qdev getters, and we  
still need each device to handle enabling/disabling itself. Are you  
okay with those parts if we move just the VMState to pc87312? That  
feels wrong.

Do we have any ordering guarantee that the isa devices were loaded  
prior to the pc87312 post hook?

How do BIOS config changes work on a PC? Which qdev would be  
responsible for saving their state?

Andreas
Gerd Hoffmann - June 9, 2011, 12:23 p.m.
Hi,

> Btw is 1 correct here or should an initial version be 0?

I think for sub-structs it doesn't matter at all, only for top-level 
vmstates.

>> I get the feeling that doing all this in the pc87312 emulation is
>> easier as it needs to have this logic anyway for config register
>> writes and you can probably reuse the code for loadvm pre- and
>> postprocessing.
>
> Well, I wasn't looking for the easiest way but for the proper way. I
> don't want to let pc87312-internal state get out-of-sync with that of
> the aggregated devices. So we still need the qdev getters, and we still
> need each device to handle enabling/disabling itself.

Do we?  The pc87312 is the only instance which changes those settings at 
runtime, so they should not get out-of-sync even if they are write-only 
for the pc87312.

> Are you okay with
> those parts if we move just the VMState to pc87312? That feels wrong.

I'd keep everything (iobase, irq, enabled state) in pc87312, so the isa 
vmstate doesn't need any updates.  The pc87312 actually applies the 
configuration, so this doesn't feel wrong to me ...

> Do we have any ordering guarantee that the isa devices were loaded prior
> to the pc87312 post hook?

I don't think so.  Juan?

> How do BIOS config changes work on a PC? Which qdev would be responsible
> for saving their state?

They are not re-configurable at runtime.  I think even on real hardware 
you usually can only enable/disable devices, not change the configuration.

cheers,
   Gerd
Andreas Färber - June 9, 2011, 2:07 p.m.
Hi,

Am 09.06.2011 um 14:23 schrieb Gerd Hoffmann:

>>> I get the feeling that doing all this in the pc87312 emulation is
>>> easier as it needs to have this logic anyway for config register
>>> writes and you can probably reuse the code for loadvm pre- and
>>> postprocessing.
>>
>> Well, I wasn't looking for the easiest way but for the proper way. I
>> don't want to let pc87312-internal state get out-of-sync with that of
>> the aggregated devices. So we still need the qdev getters, and we  
>> still
>> need each device to handle enabling/disabling itself.
>
> Do we?  The pc87312 is the only instance which changes those  
> settings at runtime, so they should not get out-of-sync even if they  
> are write-only for the pc87312.

The devices need to register the I/O ports for sure. No one else knows  
what functions to use.
So we either need your generic set_state callback or my explicit  
helper functions to invoke that.

>> Are you okay with
>> those parts if we move just the VMState to pc87312? That feels wrong.
>
> I'd keep everything (iobase, irq, enabled state) in pc87312, so the  
> isa vmstate doesn't need any updates.  The pc87312 actually applies  
> the configuration, so this doesn't feel wrong to me ...
>
>> Do we have any ordering guarantee that the isa devices were loaded  
>> prior
>> to the pc87312 post hook?
>
> I don't think so.  Juan?

In that case it won't work (out-of-sync) and we shouldn't introduce  
VMState for pc87312 at all imo. In theory we'd probably need a pc87312- 
owned bus to put the devices on but then I don't see how to reuse the  
existing isa devices.

>> How do BIOS config changes work on a PC? Which qdev would be  
>> responsible
>> for saving their state?
>
> They are not re-configurable at runtime.  I think even on real  
> hardware you usually can only enable/disable devices, not change the  
> configuration.

I'm positive they are configurable by the BIOS, that's why I called it  
"ISA reconfigurability" (not "Weird workarounds for PReP") and thought  
about VMState in the first place.

SystemSoft MobilePRO BIOS, PhoenixBIOS, AMI BIOS, Award BIOS all allow  
to reconfigure I/O base (same also IRQ) of both serial ports and  
parallel port. fdc and ide however I've seen nowhere before.
Some newer ones have an "Auto" setting that hides this, and when  
choosing "Enabled" a new field shows up.
Configuration options are a set of fixed options, similar to pc87312.

DOS, Windows and Linux were able to apply such driver configuration  
changes after a reboot iirc. For example, ISA SoundBlaster or NE2000  
cards. I also remember 3Com having special DOS boot discs with a tool  
to change on-card config for some ISA network cards.

Andreas
Gerd Hoffmann - June 9, 2011, 2:19 p.m.
Hi,

> In that case it won't work (out-of-sync) and we shouldn't introduce
> VMState for pc87312 at all imo. In theory we'd probably need a
> pc87312-owned bus to put the devices on but then I don't see how to
> reuse the existing isa devices.

Oh, that should actually work just fine and is maybe the best idea.
The bus interface and the bus implementation are independant from each 
other.  USB for example has UHCI and OHCI, both implement a usb bus and 
you can hook the USB devices to both of them just fine.  Might be the 
ISA bus is a bit sloppy because a single implementation exists and needs 
some cleanups to make this actually work.  But worth investigating I think.

cheers,
   Gerd
Markus Armbruster - June 9, 2011, 2:53 p.m.
Andreas Färber <andreas.faerber@web.de> writes:

> To allow enabling/disabling present ISA devices without hotplug,
> keep track of state and add a helper to avoid enabling twice.
> Since the properties to be configured are defined at device level,
> delegate the actual work to a callback function.
>
> If no callback is supplied, the device can't be disabled.
>
> Prepare VMSTATE_ISA_DEVICE for devices that support disabling.
> Legacy devices never change their state and won't need this yet.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/hw.h      |   15 +++++++++++++++
>  hw/isa-bus.c |   29 +++++++++++++++++++++++++++++
>  hw/isa.h     |    4 ++++
>  3 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/hw/hw.h b/hw/hw.h
> index 56447a7..07b1e2e 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -628,6 +628,21 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>      .info         = &vmstate_info_unused_buffer,                     \
>      .flags        = VMS_BUFFER,                                      \
>  }
> +
> +extern const VMStateDescription vmstate_isa_device;
> +
> +#define VMSTATE_ISA_DEVICE_V(_field, _state, _version) {             \
> +    .name       = (stringify(_field)),                               \
> +    .version_id   = (_version),                                      \
> +    .size       = sizeof(ISADevice),                                 \
> +    .vmsd       = &vmstate_isa_device,                               \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, ISADevice),   \
> +}
> +
> +#define VMSTATE_ISA_DEVICE(_field, _state) {                         \
> +    VMSTATE_ISA_DEVICE_V(_field, _state, 0)
> +
>  extern const VMStateDescription vmstate_pci_device;
>  
>  #define VMSTATE_PCI_DEVICE(_field, _state) {                         \
> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> index 2765543..d258932 100644
> --- a/hw/isa-bus.c
> +++ b/hw/isa-bus.c
> @@ -112,6 +112,7 @@ static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base)
>  
>      dev->isairq[0] = -1;
>      dev->isairq[1] = -1;
> +    dev->enabled = true;
>  
>      return info->init(dev);
>  }
> @@ -156,6 +157,34 @@ ISADevice *isa_create_simple(const char *name)
>      return dev;
>  }
>  
> +const VMStateDescription vmstate_isa_device = {
> +    .name = "ISADevice",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_BOOL(enabled, ISADevice),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +int isa_set_state(ISADevice *dev, bool enabled)
> +{
> +    ISADeviceInfo *info = DO_UPCAST(ISADeviceInfo, qdev, dev->qdev.info);
> +    int err;
> +
> +    if (dev->enabled == enabled) {
> +        return 42;

Sorry, too clever.  Make it return 0 for the benefit of future
maintenance programmers.

> +    } else if (info->set_state == NULL) {
> +        return -1;
> +    }
> +    err = info->set_state(dev, enabled);
> +    if (err < 0) {
> +        return err;
> +    }
> +    dev->enabled = enabled;
> +    return err;
> +}
> +
[...]
Markus Armbruster - June 9, 2011, 4:04 p.m.
Andreas Färber <andreas.faerber@web.de> writes:

> Hi,
>
> Am 09.06.2011 um 14:23 schrieb Gerd Hoffmann:
>
>>>> I get the feeling that doing all this in the pc87312 emulation is
>>>> easier as it needs to have this logic anyway for config register
>>>> writes and you can probably reuse the code for loadvm pre- and
>>>> postprocessing.
>>>
>>> Well, I wasn't looking for the easiest way but for the proper way. I
>>> don't want to let pc87312-internal state get out-of-sync with that of
>>> the aggregated devices. So we still need the qdev getters, and we
>>> still
>>> need each device to handle enabling/disabling itself.
>>
>> Do we?  The pc87312 is the only instance which changes those
>> settings at runtime, so they should not get out-of-sync even if they
>> are write-only for the pc87312.
>
> The devices need to register the I/O ports for sure. No one else knows
> what functions to use.
> So we either need your generic set_state callback or my explicit
> helper functions to invoke that.
>
>>> Are you okay with
>>> those parts if we move just the VMState to pc87312? That feels wrong.
>>
>> I'd keep everything (iobase, irq, enabled state) in pc87312, so the
>> isa vmstate doesn't need any updates.  The pc87312 actually applies
>> the configuration, so this doesn't feel wrong to me ...

Feels weird to me.

A device on a real ISA bus decodes whatever addresses it likes, and
raises whatever interrupt lines it likes.

Of course, the thing within the Super I/O isn't a real ISA bus, it just
looks like one to software.  Not that there's much to look at with ISA.

>>> Do we have any ordering guarantee that the isa devices were loaded
>>> prior
>>> to the pc87312 post hook?
>>
>> I don't think so.  Juan?
>
> In that case it won't work (out-of-sync) and we shouldn't introduce
> VMState for pc87312 at all imo. In theory we'd probably need a
> pc87312- 
> owned bus to put the devices on but then I don't see how to reuse the
> existing isa devices.

Err, which device provides your ISA bus if not pc87312?

>>> How do BIOS config changes work on a PC? Which qdev would be
>>> responsible
>>> for saving their state?
>>
>> They are not re-configurable at runtime.  I think even on real
>> hardware you usually can only enable/disable devices, not change the
>> configuration.
>
> I'm positive they are configurable by the BIOS, that's why I called it
> "ISA reconfigurability" (not "Weird workarounds for PReP") and thought
> about VMState in the first place.

Yes, software-configurable ISA devices are fairly common.

[...]
Andreas Färber - June 12, 2011, 11:46 a.m.
Am 09.06.2011 um 16:53 schrieb Markus Armbruster:

> Andreas Färber <andreas.faerber@web.de> writes:
>
>> To allow enabling/disabling present ISA devices without hotplug,
>> keep track of state and add a helper to avoid enabling twice.
>> Since the properties to be configured are defined at device level,
>> delegate the actual work to a callback function.
>>
>> If no callback is supplied, the device can't be disabled.
>>
>> Prepare VMSTATE_ISA_DEVICE for devices that support disabling.
>> Legacy devices never change their state and won't need this yet.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>> hw/hw.h      |   15 +++++++++++++++
>> hw/isa-bus.c |   29 +++++++++++++++++++++++++++++
>> hw/isa.h     |    4 ++++
>> 3 files changed, 48 insertions(+), 0 deletions(-)

>> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
>> index 2765543..d258932 100644
>> --- a/hw/isa-bus.c
>> +++ b/hw/isa-bus.c

>> +int isa_set_state(ISADevice *dev, bool enabled)
>> +{
>> +    ISADeviceInfo *info = DO_UPCAST(ISADeviceInfo, qdev, dev- 
>> >qdev.info);
>> +    int err;
>> +
>> +    if (dev->enabled == enabled) {
>> +        return 42;
>
> Sorry, too clever.  Make it return 0 for the benefit of future
> maintenance programmers.

Done.
Andreas Färber - June 12, 2011, 12:48 p.m.
Am 09.06.2011 um 18:04 schrieb Markus Armbruster:

> Andreas Färber <andreas.faerber@web.de> writes:
>
>> Am 09.06.2011 um 14:23 schrieb Gerd Hoffmann:
>>
>>>>> I get the feeling that doing all this in the pc87312 emulation is
>>>>> easier as it needs to have this logic anyway for config register
>>>>> writes and you can probably reuse the code for loadvm pre- and
>>>>> postprocessing.
>>>>
>>>> Well, I wasn't looking for the easiest way but for the proper  
>>>> way. I
>>>> don't want to let pc87312-internal state get out-of-sync with  
>>>> that of
>>>> the aggregated devices. So we still need the qdev getters, and we
>>>> still
>>>> need each device to handle enabling/disabling itself.
>>>
>>> Do we?  The pc87312 is the only instance which changes those
>>> settings at runtime, so they should not get out-of-sync even if they
>>> are write-only for the pc87312.
>>
>> The devices need to register the I/O ports for sure. No one else  
>> knows
>> what functions to use.
>> So we either need your generic set_state callback or my explicit
>> helper functions to invoke that.
>>
>>>> Are you okay with
>>>> those parts if we move just the VMState to pc87312? That feels  
>>>> wrong.
>>>
>>> I'd keep everything (iobase, irq, enabled state) in pc87312, so the
>>> isa vmstate doesn't need any updates.  The pc87312 actually applies
>>> the configuration, so this doesn't feel wrong to me ...
>
> Feels weird to me.
>
> A device on a real ISA bus decodes whatever addresses it likes, and
> raises whatever interrupt lines it likes.
>
> Of course, the thing within the Super I/O isn't a real ISA bus, it  
> just
> looks like one to software.  Not that there's much to look at with  
> ISA.
>
>>>> Do we have any ordering guarantee that the isa devices were loaded
>>>> prior
>>>> to the pc87312 post hook?
>>>
>>> I don't think so.  Juan?
>>
>> In that case it won't work (out-of-sync) and we shouldn't introduce
>> VMState for pc87312 at all imo. In theory we'd probably need a
>> pc87312-
>> owned bus to put the devices on but then I don't see how to reuse the
>> existing isa devices.
>
> Err, which device provides your ISA bus if not pc87312?

As discussed on IRC, I'm now creating the ISA bus on the upcoming  
i82378 PCI-ISA bridge, as done for PIIX3. Makes no change for the  
pc87312 Super I/O situation.

Since the whole PReP series goes to ~25 patches, I'd rather postpone  
the pc87312 VMState issues.

Andreas

Patch

diff --git a/hw/hw.h b/hw/hw.h
index 56447a7..07b1e2e 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -628,6 +628,21 @@  extern const VMStateInfo vmstate_info_unused_buffer;
     .info         = &vmstate_info_unused_buffer,                     \
     .flags        = VMS_BUFFER,                                      \
 }
+
+extern const VMStateDescription vmstate_isa_device;
+
+#define VMSTATE_ISA_DEVICE_V(_field, _state, _version) {             \
+    .name       = (stringify(_field)),                               \
+    .version_id   = (_version),                                      \
+    .size       = sizeof(ISADevice),                                 \
+    .vmsd       = &vmstate_isa_device,                               \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, ISADevice),   \
+}
+
+#define VMSTATE_ISA_DEVICE(_field, _state) {                         \
+    VMSTATE_ISA_DEVICE_V(_field, _state, 0)
+
 extern const VMStateDescription vmstate_pci_device;
 
 #define VMSTATE_PCI_DEVICE(_field, _state) {                         \
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 2765543..d258932 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -112,6 +112,7 @@  static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base)
 
     dev->isairq[0] = -1;
     dev->isairq[1] = -1;
+    dev->enabled = true;
 
     return info->init(dev);
 }
@@ -156,6 +157,34 @@  ISADevice *isa_create_simple(const char *name)
     return dev;
 }
 
+const VMStateDescription vmstate_isa_device = {
+    .name = "ISADevice",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_BOOL(enabled, ISADevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+int isa_set_state(ISADevice *dev, bool enabled)
+{
+    ISADeviceInfo *info = DO_UPCAST(ISADeviceInfo, qdev, dev->qdev.info);
+    int err;
+
+    if (dev->enabled == enabled) {
+        return 42;
+    } else if (info->set_state == NULL) {
+        return -1;
+    }
+    err = info->set_state(dev, enabled);
+    if (err < 0) {
+        return err;
+    }
+    dev->enabled = enabled;
+    return err;
+}
+
 static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     ISADevice *d = DO_UPCAST(ISADevice, qdev, dev);
diff --git a/hw/isa.h b/hw/isa.h
index d2b6126..5d460ab 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -16,12 +16,15 @@  struct ISADevice {
     int nirqs;
     uint16_t ioports[32];
     int nioports;
+    bool enabled;
 };
 
 typedef int (*isa_qdev_initfn)(ISADevice *dev);
+typedef int (*isa_qdev_statefn)(ISADevice *dev, bool enabled);
 struct ISADeviceInfo {
     DeviceInfo qdev;
     isa_qdev_initfn init;
+    isa_qdev_statefn set_state;
 };
 
 ISABus *isa_bus_new(DeviceState *dev);
@@ -34,6 +37,7 @@  void isa_qdev_register(ISADeviceInfo *info);
 ISADevice *isa_create(const char *name);
 ISADevice *isa_try_create(const char *name);
 ISADevice *isa_create_simple(const char *name);
+int isa_set_state(ISADevice *dev, bool enabled);
 
 extern target_phys_addr_t isa_mem_base;