* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-15 15:10 [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space Paolo Bonzini
@ 2011-07-15 21:00 ` Andreas Färber
2011-07-23 15:56 ` Anthony Liguori
2011-07-16 14:39 ` Anthony Liguori
2011-07-23 16:14 ` Anthony Liguori
2 siblings, 1 reply; 11+ messages in thread
From: Andreas Färber @ 2011-07-15 21:00 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: QEMU Developers, Gleb Natapov, Markus Armbruster
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-15 21:00 ` Andreas Färber
@ 2011-07-23 15:56 ` Anthony Liguori
2011-07-25 9:11 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Anthony Liguori @ 2011-07-23 15:56 UTC (permalink / raw)
To: Andreas Färber
Cc: Paolo Bonzini, QEMU Developers, Gleb Natapov, Markus Armbruster
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-23 15:56 ` Anthony Liguori
@ 2011-07-25 9:11 ` Paolo Bonzini
0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2011-07-25 9:11 UTC (permalink / raw)
To: Anthony Liguori
Cc: Andreas Färber, QEMU Developers, Gleb Natapov, Markus Armbruster
>>> +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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-15 15:10 [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space Paolo Bonzini
2011-07-15 21:00 ` Andreas Färber
@ 2011-07-16 14:39 ` Anthony Liguori
2011-07-24 18:45 ` Paolo Bonzini
2011-07-23 16:14 ` Anthony Liguori
2 siblings, 1 reply; 11+ messages in thread
From: Anthony Liguori @ 2011-07-16 14:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
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);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-16 14:39 ` Anthony Liguori
@ 2011-07-24 18:45 ` Paolo Bonzini
2011-07-24 18:57 ` Anthony Liguori
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2011-07-24 18:45 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, armbru
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-24 18:45 ` Paolo Bonzini
@ 2011-07-24 18:57 ` Anthony Liguori
2011-07-24 21:35 ` Andreas Färber
0 siblings, 1 reply; 11+ messages in thread
From: Anthony Liguori @ 2011-07-24 18:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-24 18:57 ` Anthony Liguori
@ 2011-07-24 21:35 ` Andreas Färber
2011-07-25 1:45 ` Anthony Liguori
0 siblings, 1 reply; 11+ messages in thread
From: Andreas Färber @ 2011-07-24 21:35 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Paolo Bonzini, qemu-devel, armbru
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-24 21:35 ` Andreas Färber
@ 2011-07-25 1:45 ` Anthony Liguori
2011-07-25 6:33 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Anthony Liguori @ 2011-07-25 1:45 UTC (permalink / raw)
To: Andreas Färber; +Cc: Paolo Bonzini, qemu-devel, armbru
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-25 1:45 ` Anthony Liguori
@ 2011-07-25 6:33 ` Paolo Bonzini
0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2011-07-25 6:33 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Andreas Färber, qemu-devel, armbru
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space
2011-07-15 15:10 [Qemu-devel] [PATCH] report serial devices created with -device in the PIIX4 config space Paolo Bonzini
2011-07-15 21:00 ` Andreas Färber
2011-07-16 14:39 ` Anthony Liguori
@ 2011-07-23 16:14 ` Anthony Liguori
2 siblings, 0 replies; 11+ messages in thread
From: Anthony Liguori @ 2011-07-23 16:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
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);
^ permalink raw reply [flat|nested] 11+ messages in thread