Patchwork report serial devices created with -device in the PIIX4 config space

login
register
mail settings
Submitter Paolo Bonzini
Date July 15, 2011, 3:10 p.m.
Message ID <1310742615-23901-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/104853/
State New
Headers show

Comments

Paolo Bonzini - July 15, 2011, 3:10 p.m.
Serial and parallel devices created with -device are not reported in
the PIIX4 configuration space, and are hence not picked up by the DSDT.
This upsets Windows, which hides them altogether from the guest.

To avoid this, check at the end of machine initialization whether the
corresponding I/O ports have been registered.  The new function in
ioport.c does this; this also requires a tweak to isa_unassign_ioport.

I left the comment in piix4_pm_initfn since the registers I moved do
seem to match the 82371AB datasheet.  There are some quirks though.
We are setting this bit:

    "Device 8 EIO Enable (EIO_EN_DEV8)—R/W. 1=Enable PCI access to the
    device 8 enabled I/O ranges to be claimed by PIIX4 and forwarded
    to the ISA/EIO bus. 0=Disable. The LPT_MON_EN must be set to enable
    the decode."

but not LPT_MON_EN (bit 18 at 50h):

    LPT Port Enable (LPT_MON_EN)—R/W. 1=Enable accesses to parallel
    port address range (LPT_DEC_SEL) to generate a device 8 (parallel
    port) decode event. 0=Disable.

We're also setting the LPT_DEC_SEL field (that's the 0x60 written to
63h) to 11, which means reserved, rather than to 01 (378h-37Fh).

Likewise we're not setting SA_MON_EN, SB_MON_EN (respectively bit 14
and bit 16 at address 50h) for the serial ports.  However, we're setting
COMA_DEC_SEL and COMB_DEC_SEL correctly, unlike the corresponding register
for the parallel port.

All these fields are left as they are, since they are probably only
meant to be used in the DSDT.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/acpi_piix4.c |   23 ++++++++++++++++++-----
 ioport.c        |   19 +++++++++++++------
 ioport.h        |    2 +-
 3 files changed, 32 insertions(+), 12 deletions(-)
Andreas Färber - July 15, 2011, 9 p.m.
Am 15.07.2011 um 17:10 schrieb Paolo Bonzini:

> Serial and parallel devices created with -device are not reported in
> the PIIX4 configuration space, and are hence not picked up by the  
> DSDT.
> This upsets Windows, which hides them altogether from the guest.
>
> To avoid this, check at the end of machine initialization whether the
> corresponding I/O ports have been registered.  The new function in
> ioport.c does this; this also requires a tweak to isa_unassign_ioport.
>
> I left the comment in piix4_pm_initfn since the registers I moved do
> seem to match the 82371AB datasheet.  There are some quirks though.
> We are setting this bit:
>
>    "Device 8 EIO Enable (EIO_EN_DEV8)—R/W. 1=Enable PCI access to the
>    device 8 enabled I/O ranges to be claimed by PIIX4 and forwarded
>    to the ISA/EIO bus. 0=Disable. The LPT_MON_EN must be set to enable
>    the decode."
>
> but not LPT_MON_EN (bit 18 at 50h):
>
>    LPT Port Enable (LPT_MON_EN)—R/W. 1=Enable accesses to parallel
>    port address range (LPT_DEC_SEL) to generate a device 8 (parallel
>    port) decode event. 0=Disable.
>
> We're also setting the LPT_DEC_SEL field (that's the 0x60 written to
> 63h) to 11, which means reserved, rather than to 01 (378h-37Fh).
>
> Likewise we're not setting SA_MON_EN, SB_MON_EN (respectively bit 14
> and bit 16 at address 50h) for the serial ports.  However, we're  
> setting
> COMA_DEC_SEL and COMB_DEC_SEL correctly, unlike the corresponding  
> register
> for the parallel port.
>
> All these fields are left as they are, since they are probably only
> meant to be used in the DSDT.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/acpi_piix4.c |   23 ++++++++++++++++++-----
> ioport.c        |   19 +++++++++++++------
> ioport.h        |    2 +-
> 3 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 350558b..03de3ad 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c

> @@ -311,6 +313,19 @@ static void piix4_powerdown(void *opaque, int  
> irq, int power_failing)
>     acpi_pm1_evt_power_down(pm1a, tmr);
> }
>
> +static void piix4_pm_machine_ready(struct Notifier* n)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);

DO_UPCAST()? I assume we have it for a reason.

> diff --git a/ioport.c b/ioport.c
> index 2e971fa..0d2611d 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -245,18 +245,25 @@ void isa_unassign_ioport(pio_addr_t start, int  
> length)
>     int i;
>
>     for(i = start; i < start + length; i++) {
> -        ioport_read_table[0][i] = default_ioport_readb;
> -        ioport_read_table[1][i] = default_ioport_readw;
> -        ioport_read_table[2][i] = default_ioport_readl;
> +        ioport_read_table[0][i] = NULL;
> +        ioport_read_table[1][i] = NULL;
> +        ioport_read_table[2][i] = NULL;
>
> -        ioport_write_table[0][i] = default_ioport_writeb;
> -        ioport_write_table[1][i] = default_ioport_writew;
> -        ioport_write_table[2][i] = default_ioport_writel;
> +        ioport_write_table[0][i] = NULL;
> +        ioport_write_table[1][i] = NULL;
> +        ioport_write_table[2][i] = NULL;

Does this make a change as to whether unhandled I/O ports are reported  
in debug mode?

What do you think of Gleb's recent request to convert all ioports to  
IORanges? I briefly looked into it but feared that needlessly using  
uint64_t for all parameters might damage performance on 32-bit hosts.

The ioport changes look okay otherwise.

Andreas
Anthony Liguori - July 16, 2011, 2:39 p.m.
On 07/15/2011 10:10 AM, Paolo Bonzini wrote:
> Serial and parallel devices created with -device are not reported in
> the PIIX4 configuration space, and are hence not picked up by the DSDT.
> This upsets Windows, which hides them altogether from the guest.
>
> To avoid this, check at the end of machine initialization whether the
> corresponding I/O ports have been registered.  The new function in
> ioport.c does this; this also requires a tweak to isa_unassign_ioport.
>
> I left the comment in piix4_pm_initfn since the registers I moved do
> seem to match the 82371AB datasheet.  There are some quirks though.
> We are setting this bit:
>
>      "Device 8 EIO Enable (EIO_EN_DEV8)—R/W. 1=Enable PCI access to the
>      device 8 enabled I/O ranges to be claimed by PIIX4 and forwarded
>      to the ISA/EIO bus. 0=Disable. The LPT_MON_EN must be set to enable
>      the decode."
>
> but not LPT_MON_EN (bit 18 at 50h):
>
>      LPT Port Enable (LPT_MON_EN)—R/W. 1=Enable accesses to parallel
>      port address range (LPT_DEC_SEL) to generate a device 8 (parallel
>      port) decode event. 0=Disable.
>
> We're also setting the LPT_DEC_SEL field (that's the 0x60 written to
> 63h) to 11, which means reserved, rather than to 01 (378h-37Fh).
>
> Likewise we're not setting SA_MON_EN, SB_MON_EN (respectively bit 14
> and bit 16 at address 50h) for the serial ports.  However, we're setting
> COMA_DEC_SEL and COMB_DEC_SEL correctly, unlike the corresponding register
> for the parallel port.
>
> All these fields are left as they are, since they are probably only
> meant to be used in the DSDT.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Instead of checking for a port assignment, couldn't we do a device tree 
transversal and look for isa-serial devices?  We could then look at the 
iobase property to figure out which serial device is configured.

Regards,

Anthony Liguori

> ---
>   hw/acpi_piix4.c |   23 ++++++++++++++++++-----
>   ioport.c        |   19 +++++++++++++------
>   ioport.h        |    2 +-
>   3 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 350558b..03de3ad 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -23,6 +23,7 @@
>   #include "acpi.h"
>   #include "sysemu.h"
>   #include "range.h"
> +#include "ioport.h"
>
>   //#define DEBUG
>
> @@ -63,6 +64,7 @@ typedef struct PIIX4PMState {
>       qemu_irq irq;
>       qemu_irq smi_irq;
>       int kvm_enabled;
> +    Notifier machine_ready;
>
>       /* for pci hotplug */
>       ACPIGPE gpe;
> @@ -311,6 +313,19 @@ static void piix4_powerdown(void *opaque, int irq, int power_failing)
>       acpi_pm1_evt_power_down(pm1a, tmr);
>   }
>
> +static void piix4_pm_machine_ready(struct Notifier* n)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
> +    uint8_t *pci_conf;
> +
> +    pci_conf = s->dev.config;
> +    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
> +    pci_conf[0x63] = 0x60;
> +    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
> +	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
> +
> +}
> +
>   static int piix4_pm_initfn(PCIDevice *dev)
>   {
>       PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
> @@ -337,11 +352,6 @@ static int piix4_pm_initfn(PCIDevice *dev)
>
>       /* XXX: which specification is used ? The i82731AB has different
>          mappings */
> -    pci_conf[0x5f] = (parallel_hds[0] != NULL ? 0x80 : 0) | 0x10;
> -    pci_conf[0x63] = 0x60;
> -    pci_conf[0x67] = (serial_hds[0] != NULL ? 0x08 : 0) |
> -	(serial_hds[1] != NULL ? 0x90 : 0);
> -
>       pci_conf[0x90] = s->smb_io_base | 1;
>       pci_conf[0x91] = s->smb_io_base>>  8;
>       pci_conf[0xd2] = 0x09;
> @@ -354,12 +364,14 @@ static int piix4_pm_initfn(PCIDevice *dev)
>       qemu_system_powerdown = *qemu_allocate_irqs(piix4_powerdown, s, 1);
>
>       pm_smbus_init(&s->dev.qdev,&s->smb);
> +    s->machine_ready.notify = piix4_pm_machine_ready;
> +    qemu_add_machine_init_done_notifier(&s->machine_ready);
>       qemu_register_reset(piix4_reset, s);
>       piix4_acpi_system_hot_add_init(dev->bus, s);
>
>       return 0;
>   }
>
>   i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                          qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
>                          int kvm_enabled)
> diff --git a/ioport.c b/ioport.c
> index 2e971fa..0d2611d 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -245,18 +245,25 @@ void isa_unassign_ioport(pio_addr_t start, int length)
>       int i;
>
>       for(i = start; i<  start + length; i++) {
> -        ioport_read_table[0][i] = default_ioport_readb;
> -        ioport_read_table[1][i] = default_ioport_readw;
> -        ioport_read_table[2][i] = default_ioport_readl;
> +        ioport_read_table[0][i] = NULL;
> +        ioport_read_table[1][i] = NULL;
> +        ioport_read_table[2][i] = NULL;
>
> -        ioport_write_table[0][i] = default_ioport_writeb;
> -        ioport_write_table[1][i] = default_ioport_writew;
> -        ioport_write_table[2][i] = default_ioport_writel;
> +        ioport_write_table[0][i] = NULL;
> +        ioport_write_table[1][i] = NULL;
> +        ioport_write_table[2][i] = NULL;
>
>           ioport_opaque[i] = NULL;
>       }
>   }
>
> +bool isa_is_ioport_assigned(pio_addr_t start)
> +{
> +    return (ioport_read_table[0][start] || ioport_write_table[0][start] ||
> +	    ioport_read_table[1][start] || ioport_write_table[1][start] ||
> +	    ioport_read_table[2][start] || ioport_write_table[2][start]);
> +}
> +
>   /***********************************************************/
>
>   void cpu_outb(pio_addr_t addr, uint8_t val)
> diff --git a/ioport.h b/ioport.h
> index 5ae62a3..82ffd9d 100644
> --- a/ioport.h
> +++ b/ioport.h
> @@ -43,7 +43,7 @@ int register_ioport_read(pio_addr_t start, int length, int size,
>   int register_ioport_write(pio_addr_t start, int length, int size,
>                             IOPortWriteFunc *func, void *opaque);
>   void isa_unassign_ioport(pio_addr_t start, int length);
> -
> +bool isa_is_ioport_assigned(pio_addr_t start);
>
>   void cpu_outb(pio_addr_t addr, uint8_t val);
>   void cpu_outw(pio_addr_t addr, uint16_t val);
Anthony Liguori - July 23, 2011, 3:56 p.m.
On 07/15/2011 04:00 PM, Andreas Färber wrote:
> Am 15.07.2011 um 17:10 schrieb Paolo Bonzini:
>
>> Serial and parallel devices created with -device are not reported in
>> the PIIX4 configuration space, and are hence not picked up by the DSDT.
>> This upsets Windows, which hides them altogether from the guest.
>>
>> To avoid this, check at the end of machine initialization whether the
>> corresponding I/O ports have been registered. The new function in
>> ioport.c does this; this also requires a tweak to isa_unassign_ioport.
>>
>> I left the comment in piix4_pm_initfn since the registers I moved do
>> seem to match the 82371AB datasheet. There are some quirks though.
>> We are setting this bit:
>>
>> "Device 8 EIO Enable (EIO_EN_DEV8)—R/W. 1=Enable PCI access to the
>> device 8 enabled I/O ranges to be claimed by PIIX4 and forwarded
>> to the ISA/EIO bus. 0=Disable. The LPT_MON_EN must be set to enable
>> the decode."
>>
>> but not LPT_MON_EN (bit 18 at 50h):
>>
>> LPT Port Enable (LPT_MON_EN)—R/W. 1=Enable accesses to parallel
>> port address range (LPT_DEC_SEL) to generate a device 8 (parallel
>> port) decode event. 0=Disable.
>>
>> We're also setting the LPT_DEC_SEL field (that's the 0x60 written to
>> 63h) to 11, which means reserved, rather than to 01 (378h-37Fh).
>>
>> Likewise we're not setting SA_MON_EN, SB_MON_EN (respectively bit 14
>> and bit 16 at address 50h) for the serial ports. However, we're setting
>> COMA_DEC_SEL and COMB_DEC_SEL correctly, unlike the corresponding
>> register
>> for the parallel port.
>>
>> All these fields are left as they are, since they are probably only
>> meant to be used in the DSDT.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> hw/acpi_piix4.c | 23 ++++++++++++++++++-----
>> ioport.c | 19 +++++++++++++------
>> ioport.h | 2 +-
>> 3 files changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index 350558b..03de3ad 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>
>> @@ -311,6 +313,19 @@ static void piix4_powerdown(void *opaque, int
>> irq, int power_failing)
>> acpi_pm1_evt_power_down(pm1a, tmr);
>> }
>>
>> +static void piix4_pm_machine_ready(struct Notifier* n)
>> +{
>> + PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
>
> DO_UPCAST()? I assume we have it for a reason.

NIH is the reason we have it.

Regards,

Anthony Liguori
Anthony Liguori - July 23, 2011, 4:14 p.m.
On 07/15/2011 10:10 AM, Paolo Bonzini wrote:
> Serial and parallel devices created with -device are not reported in
> the PIIX4 configuration space, and are hence not picked up by the DSDT.
> This upsets Windows, which hides them altogether from the guest.
>
> To avoid this, check at the end of machine initialization whether the
> corresponding I/O ports have been registered.  The new function in
> ioport.c does this; this also requires a tweak to isa_unassign_ioport.
>
> I left the comment in piix4_pm_initfn since the registers I moved do
> seem to match the 82371AB datasheet.  There are some quirks though.
> We are setting this bit:
>
>      "Device 8 EIO Enable (EIO_EN_DEV8)—R/W. 1=Enable PCI access to the
>      device 8 enabled I/O ranges to be claimed by PIIX4 and forwarded
>      to the ISA/EIO bus. 0=Disable. The LPT_MON_EN must be set to enable
>      the decode."
>
> but not LPT_MON_EN (bit 18 at 50h):
>
>      LPT Port Enable (LPT_MON_EN)—R/W. 1=Enable accesses to parallel
>      port address range (LPT_DEC_SEL) to generate a device 8 (parallel
>      port) decode event. 0=Disable.
>
> We're also setting the LPT_DEC_SEL field (that's the 0x60 written to
> 63h) to 11, which means reserved, rather than to 01 (378h-37Fh).
>
> Likewise we're not setting SA_MON_EN, SB_MON_EN (respectively bit 14
> and bit 16 at address 50h) for the serial ports.  However, we're setting
> COMA_DEC_SEL and COMB_DEC_SEL correctly, unlike the corresponding register
> for the parallel port.
>
> All these fields are left as they are, since they are probably only
> meant to be used in the DSDT.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   hw/acpi_piix4.c |   23 ++++++++++++++++++-----
>   ioport.c        |   19 +++++++++++++------
>   ioport.h        |    2 +-
>   3 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 350558b..03de3ad 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -23,6 +23,7 @@
>   #include "acpi.h"
>   #include "sysemu.h"
>   #include "range.h"
> +#include "ioport.h"
>
>   //#define DEBUG
>
> @@ -63,6 +64,7 @@ typedef struct PIIX4PMState {
>       qemu_irq irq;
>       qemu_irq smi_irq;
>       int kvm_enabled;
> +    Notifier machine_ready;
>
>       /* for pci hotplug */
>       ACPIGPE gpe;
> @@ -311,6 +313,19 @@ static void piix4_powerdown(void *opaque, int irq, int power_failing)
>       acpi_pm1_evt_power_down(pm1a, tmr);
>   }
>
> +static void piix4_pm_machine_ready(struct Notifier* n)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
> +    uint8_t *pci_conf;
> +
> +    pci_conf = s->dev.config;
> +    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
> +    pci_conf[0x63] = 0x60;
> +    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
> +	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
> +
> +}
> +
>   static int piix4_pm_initfn(PCIDevice *dev)
>   {
>       PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
> @@ -337,11 +352,6 @@ static int piix4_pm_initfn(PCIDevice *dev)
>
>       /* XXX: which specification is used ? The i82731AB has different
>          mappings */
> -    pci_conf[0x5f] = (parallel_hds[0] != NULL ? 0x80 : 0) | 0x10;
> -    pci_conf[0x63] = 0x60;
> -    pci_conf[0x67] = (serial_hds[0] != NULL ? 0x08 : 0) |
> -	(serial_hds[1] != NULL ? 0x90 : 0);
> -
>       pci_conf[0x90] = s->smb_io_base | 1;
>       pci_conf[0x91] = s->smb_io_base>>  8;
>       pci_conf[0xd2] = 0x09;
> @@ -354,12 +364,14 @@ static int piix4_pm_initfn(PCIDevice *dev)
>       qemu_system_powerdown = *qemu_allocate_irqs(piix4_powerdown, s, 1);
>
>       pm_smbus_init(&s->dev.qdev,&s->smb);
> +    s->machine_ready.notify = piix4_pm_machine_ready;
> +    qemu_add_machine_init_done_notifier(&s->machine_ready);
>       qemu_register_reset(piix4_reset, s);
>       piix4_acpi_system_hot_add_init(dev->bus, s);
>
>       return 0;
>   }
>
>   i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                          qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
>                          int kvm_enabled)
> diff --git a/ioport.c b/ioport.c
> index 2e971fa..0d2611d 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -245,18 +245,25 @@ void isa_unassign_ioport(pio_addr_t start, int length)
>       int i;
>
>       for(i = start; i<  start + length; i++) {
> -        ioport_read_table[0][i] = default_ioport_readb;
> -        ioport_read_table[1][i] = default_ioport_readw;
> -        ioport_read_table[2][i] = default_ioport_readl;
> +        ioport_read_table[0][i] = NULL;
> +        ioport_read_table[1][i] = NULL;
> +        ioport_read_table[2][i] = NULL;
>
> -        ioport_write_table[0][i] = default_ioport_writeb;
> -        ioport_write_table[1][i] = default_ioport_writew;
> -        ioport_write_table[2][i] = default_ioport_writel;
> +        ioport_write_table[0][i] = NULL;
> +        ioport_write_table[1][i] = NULL;
> +        ioport_write_table[2][i] = NULL;
>
>           ioport_opaque[i] = NULL;
>       }
>   }
>
> +bool isa_is_ioport_assigned(pio_addr_t start)
> +{
> +    return (ioport_read_table[0][start] || ioport_write_table[0][start] ||
> +	    ioport_read_table[1][start] || ioport_write_table[1][start] ||
> +	    ioport_read_table[2][start] || ioport_write_table[2][start]);
> +}
> +
>   /***********************************************************/
>
>   void cpu_outb(pio_addr_t addr, uint8_t val)
> diff --git a/ioport.h b/ioport.h
> index 5ae62a3..82ffd9d 100644
> --- a/ioport.h
> +++ b/ioport.h
> @@ -43,7 +43,7 @@ int register_ioport_read(pio_addr_t start, int length, int size,
>   int register_ioport_write(pio_addr_t start, int length, int size,
>                             IOPortWriteFunc *func, void *opaque);
>   void isa_unassign_ioport(pio_addr_t start, int length);
> -
> +bool isa_is_ioport_assigned(pio_addr_t start);
>
>   void cpu_outb(pio_addr_t addr, uint8_t val);
>   void cpu_outw(pio_addr_t addr, uint16_t val);
Paolo Bonzini - July 24, 2011, 6:45 p.m.
On 07/16/2011 04:39 PM, Anthony Liguori wrote:
>
> Instead of checking for a port assignment, couldn't we do a device tree
> transversal and look for isa-serial devices?  We could then look at the
> iobase property to figure out which serial device is configured.

Thanks for applying it as is---BTW, I set to do this first but changed 
plans after I noticed that there are no qdev property getters.

Paolo
Anthony Liguori - July 24, 2011, 6:57 p.m.
On 07/24/2011 01:45 PM, Paolo Bonzini wrote:
> On 07/16/2011 04:39 PM, Anthony Liguori wrote:
>>
>> Instead of checking for a port assignment, couldn't we do a device tree
>> transversal and look for isa-serial devices? We could then look at the
>> iobase property to figure out which serial device is configured.
>
> Thanks for applying it as is

It fixes a broken guest so I'm okay with a non-optimal solution.

>---BTW, I set to do this first but changed
> plans after I noticed that there are no qdev property getters.

:-/

I seem to recall a patch to add them but it's not a very nice solution 
either.

Regards,

Anthony Liguori

> Paolo
>
Andreas Färber - July 24, 2011, 9:35 p.m.
Am 24.07.2011 um 20:57 schrieb Anthony Liguori:

> On 07/24/2011 01:45 PM, Paolo Bonzini wrote:
>> On 07/16/2011 04:39 PM, Anthony Liguori wrote:
>>>
>>> Instead of checking for a port assignment, couldn't we do a device  
>>> tree
>>> transversal and look for isa-serial devices? We could then look at  
>>> the
>>> iobase property to figure out which serial device is configured.
>>
>> ---BTW, I set to do this first but changed
>> plans after I noticed that there are no qdev property getters.
>
> :-/
>
> I seem to recall a patch to add them but it's not a very nice  
> solution either.

Mine, in the ISA/PReP series. The alternative is moving lots of device  
state to header files.
Adding the getters avoided patch conflicts and seemed cleaner symmetry- 
wise.

Regards,
Andreas
Anthony Liguori - July 25, 2011, 1:45 a.m.
On 07/24/2011 04:35 PM, Andreas Färber wrote:
> Am 24.07.2011 um 20:57 schrieb Anthony Liguori:
>
>> On 07/24/2011 01:45 PM, Paolo Bonzini wrote:
>>> On 07/16/2011 04:39 PM, Anthony Liguori wrote:
>>>>
>>>> Instead of checking for a port assignment, couldn't we do a device tree
>>>> transversal and look for isa-serial devices? We could then look at the
>>>> iobase property to figure out which serial device is configured.
>>>
>>> ---BTW, I set to do this first but changed
>>> plans after I noticed that there are no qdev property getters.
>>
>> :-/
>>
>> I seem to recall a patch to add them but it's not a very nice solution
>> either.
>
> Mine, in the ISA/PReP series. The alternative is moving lots of device
> state to header files.
> Adding the getters avoided patch conflicts and seemed cleaner
> symmetry-wise.

The getters lack type safety though which is unfortunate.  That said, I 
guess it's the best we can do with qdev.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
Paolo Bonzini - July 25, 2011, 6:33 a.m.
On 07/25/2011 03:45 AM, Anthony Liguori wrote:
> The getters lack type safety though which is unfortunate. That said, I
> guess it's the best we can do with qdev.

You can assert on type mismatch though, just like with setters.

Paolo
Paolo Bonzini - July 25, 2011, 9:11 a.m.
>>> +static void piix4_pm_machine_ready(struct Notifier* n)
>>> +{
>>> + PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
>>
>> DO_UPCAST()? I assume we have it for a reason.
>
> NIH is the reason we have it.

DO_UPCAST checks that the offset of the field is zero:

#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
     char __attribute__((unused)) offset_must_be_zero[ \
         -offsetof(type, field)]; \
     container_of(dev, type, field);}))
#else
#define DO_UPCAST(type, field, dev) container_of(dev, type, field)
#endif

This isn't the case here, we really want container_of.

BTW, DO_UPCAST actually is used to do a _down_cast (base to derived).  A 
compile-time checked upcast (derived to base) could be done like this:

#ifdef __GNUC__
#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
     char __attribute__((unused)) offset_must_be_zero[ \
         -offsetof(type, field)]; \
     char __attribute__((unused)) type_matches = \
         type_check(type, __typeof__(dev));
     &(dev)->field);}))
#else
#define DO_UPCAST(type, field, dev) &(dev)->field
#endif

Paolo

Patch

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 350558b..03de3ad 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -23,6 +23,7 @@ 
 #include "acpi.h"
 #include "sysemu.h"
 #include "range.h"
+#include "ioport.h"
 
 //#define DEBUG
 
@@ -63,6 +64,7 @@  typedef struct PIIX4PMState {
     qemu_irq irq;
     qemu_irq smi_irq;
     int kvm_enabled;
+    Notifier machine_ready;
 
     /* for pci hotplug */
     ACPIGPE gpe;
@@ -311,6 +313,19 @@  static void piix4_powerdown(void *opaque, int irq, int power_failing)
     acpi_pm1_evt_power_down(pm1a, tmr);
 }
 
+static void piix4_pm_machine_ready(struct Notifier* n)
+{
+    PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
+    uint8_t *pci_conf;
+
+    pci_conf = s->dev.config;
+    pci_conf[0x5f] = (isa_is_ioport_assigned(0x378) ? 0x80 : 0) | 0x10;
+    pci_conf[0x63] = 0x60;
+    pci_conf[0x67] = (isa_is_ioport_assigned(0x3f8) ? 0x08 : 0) |
+	(isa_is_ioport_assigned(0x2f8) ? 0x90 : 0);
+
+}
+
 static int piix4_pm_initfn(PCIDevice *dev)
 {
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, dev);
@@ -337,11 +352,6 @@  static int piix4_pm_initfn(PCIDevice *dev)
 
     /* XXX: which specification is used ? The i82731AB has different
        mappings */
-    pci_conf[0x5f] = (parallel_hds[0] != NULL ? 0x80 : 0) | 0x10;
-    pci_conf[0x63] = 0x60;
-    pci_conf[0x67] = (serial_hds[0] != NULL ? 0x08 : 0) |
-	(serial_hds[1] != NULL ? 0x90 : 0);
-
     pci_conf[0x90] = s->smb_io_base | 1;
     pci_conf[0x91] = s->smb_io_base >> 8;
     pci_conf[0xd2] = 0x09;
@@ -354,12 +364,14 @@  static int piix4_pm_initfn(PCIDevice *dev)
     qemu_system_powerdown = *qemu_allocate_irqs(piix4_powerdown, s, 1);
 
     pm_smbus_init(&s->dev.qdev, &s->smb);
+    s->machine_ready.notify = piix4_pm_machine_ready;
+    qemu_add_machine_init_done_notifier(&s->machine_ready);
     qemu_register_reset(piix4_reset, s);
     piix4_acpi_system_hot_add_init(dev->bus, s);
 
     return 0;
 }
 
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                        qemu_irq sci_irq, qemu_irq cmos_s3, qemu_irq smi_irq,
                        int kvm_enabled)
diff --git a/ioport.c b/ioport.c
index 2e971fa..0d2611d 100644
--- a/ioport.c
+++ b/ioport.c
@@ -245,18 +245,25 @@  void isa_unassign_ioport(pio_addr_t start, int length)
     int i;
 
     for(i = start; i < start + length; i++) {
-        ioport_read_table[0][i] = default_ioport_readb;
-        ioport_read_table[1][i] = default_ioport_readw;
-        ioport_read_table[2][i] = default_ioport_readl;
+        ioport_read_table[0][i] = NULL;
+        ioport_read_table[1][i] = NULL;
+        ioport_read_table[2][i] = NULL;
 
-        ioport_write_table[0][i] = default_ioport_writeb;
-        ioport_write_table[1][i] = default_ioport_writew;
-        ioport_write_table[2][i] = default_ioport_writel;
+        ioport_write_table[0][i] = NULL;
+        ioport_write_table[1][i] = NULL;
+        ioport_write_table[2][i] = NULL;
 
         ioport_opaque[i] = NULL;
     }
 }
 
+bool isa_is_ioport_assigned(pio_addr_t start)
+{
+    return (ioport_read_table[0][start] || ioport_write_table[0][start] ||
+	    ioport_read_table[1][start] || ioport_write_table[1][start] ||
+	    ioport_read_table[2][start] || ioport_write_table[2][start]);
+}
+
 /***********************************************************/
 
 void cpu_outb(pio_addr_t addr, uint8_t val)
diff --git a/ioport.h b/ioport.h
index 5ae62a3..82ffd9d 100644
--- a/ioport.h
+++ b/ioport.h
@@ -43,7 +43,7 @@  int register_ioport_read(pio_addr_t start, int length, int size,
 int register_ioport_write(pio_addr_t start, int length, int size,
                           IOPortWriteFunc *func, void *opaque);
 void isa_unassign_ioport(pio_addr_t start, int length);
-
+bool isa_is_ioport_assigned(pio_addr_t start);
 
 void cpu_outb(pio_addr_t addr, uint8_t val);
 void cpu_outw(pio_addr_t addr, uint16_t val);