Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space

[Anthony Liguori <anthony@codemonkey.ws>]

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);