[Qemu-devel] [PATCHv2] seabios: enable io/memory unconditionally

[Qemu-devel] [PATCHv2] seabios: enable io/memory unconditionally

[Michael S. Tsirkin]

VGA adapters need to claim memory and i/o
transactions even if they do not have any
i/o or memory bars. E.g. PCI spec, page 297,
gives an example of such a device:

    Programming interface 0000 0000b
    VGA-compatible controller. Memory
    addresses 0A 0000h through 0B
    FFFFh. I/O addresses 3B0h to 3BBh
    and 3C0h to 3DFh and all aliases of
    these addresses.

While we could check for these devices and special-case them, it is
easier to fix this by enabling i/o and memory space unconditionally:
devices that do not support it will just ignore this setting.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Reposting for qemu tree.

 src/pciinit.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 0d558a9..eab082a 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -28,7 +28,6 @@ static u8 pci_irqs[4] = {
 
 static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
 {
-    u16 cmd;
     u32 ofs, old_addr;
 
     if (region_num == PCI_ROM_SLOT) {
@@ -41,16 +40,6 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
 
     pci_config_writel(bdf, ofs, addr);
     dprintf(1, "region %d: 0x%08x\n", region_num, addr);
-
-    /* enable memory mappings */
-    cmd = pci_config_readw(bdf, PCI_COMMAND);
-    if (region_num == PCI_ROM_SLOT)
-        cmd |= PCI_COMMAND_MEMORY;
-    else if (old_addr & PCI_BASE_ADDRESS_SPACE_IO)
-        cmd |= PCI_COMMAND_IO;
-    else
-        cmd |= PCI_COMMAND_MEMORY;
-    pci_config_writew(bdf, PCI_COMMAND, cmd);
 }
 
 /* return the global irq number corresponding to a given device irq
@@ -95,6 +84,7 @@ static void pci_bios_init_device(u16 bdf)
 {
     int class;
     u32 *paddr;
+    uint16_t cmd;
     int i, pin, pic_irq, vendor_id, device_id;
 
     class = pci_config_readw(bdf, PCI_CLASS_DEVICE);
@@ -165,6 +155,11 @@ static void pci_bios_init_device(u16 bdf)
         break;
     }
 
+    /* enable memory mappings */
+    cmd = pci_config_readw(d, PCI_COMMAND);
+    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
+    pci_config_writew(d, PCI_COMMAND, cmd);
+
     /* map the interrupt */
     pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
     if (pin != 0) {
-- 
1.6.5.rc2

[Qemu-devel] Re: [PATCHv2] seabios: enable io/memory unconditionally

[Gleb Natapov]

Add  seabios maintainer to CC.

On Thu, Oct 08, 2009 at 05:53:46PM +0200, Michael S. Tsirkin wrote:
> VGA adapters need to claim memory and i/o
> transactions even if they do not have any
> i/o or memory bars. E.g. PCI spec, page 297,
> gives an example of such a device:
> 
>     Programming interface 0000 0000b
>     VGA-compatible controller. Memory
>     addresses 0A 0000h through 0B
>     FFFFh. I/O addresses 3B0h to 3BBh
>     and 3C0h to 3DFh and all aliases of
>     these addresses.
> 
> While we could check for these devices and special-case them, it is
> easier to fix this by enabling i/o and memory space unconditionally:
> devices that do not support it will just ignore this setting.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Reposting for qemu tree.
> 
>  src/pciinit.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/pciinit.c b/src/pciinit.c
> index 0d558a9..eab082a 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -28,7 +28,6 @@ static u8 pci_irqs[4] = {
>  
>  static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
>  {
> -    u16 cmd;
>      u32 ofs, old_addr;
>  
>      if (region_num == PCI_ROM_SLOT) {
> @@ -41,16 +40,6 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
>  
>      pci_config_writel(bdf, ofs, addr);
>      dprintf(1, "region %d: 0x%08x\n", region_num, addr);
> -
> -    /* enable memory mappings */
> -    cmd = pci_config_readw(bdf, PCI_COMMAND);
> -    if (region_num == PCI_ROM_SLOT)
> -        cmd |= PCI_COMMAND_MEMORY;
> -    else if (old_addr & PCI_BASE_ADDRESS_SPACE_IO)
> -        cmd |= PCI_COMMAND_IO;
> -    else
> -        cmd |= PCI_COMMAND_MEMORY;
> -    pci_config_writew(bdf, PCI_COMMAND, cmd);
>  }
>  
>  /* return the global irq number corresponding to a given device irq
> @@ -95,6 +84,7 @@ static void pci_bios_init_device(u16 bdf)
>  {
>      int class;
>      u32 *paddr;
> +    uint16_t cmd;
>      int i, pin, pic_irq, vendor_id, device_id;
>  
>      class = pci_config_readw(bdf, PCI_CLASS_DEVICE);
> @@ -165,6 +155,11 @@ static void pci_bios_init_device(u16 bdf)
>          break;
>      }
>  
> +    /* enable memory mappings */
> +    cmd = pci_config_readw(d, PCI_COMMAND);
> +    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
> +    pci_config_writew(d, PCI_COMMAND, cmd);
> +
>      /* map the interrupt */
>      pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
>      if (pin != 0) {
> -- 
> 1.6.5.rc2
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [Qemu-devel] [PATCHv2] seabios: enable io/memory unconditionally

[Kevin O'Connor]

Hi Michael,

On Thu, Oct 08, 2009 at 05:53:46PM +0200, Michael S. Tsirkin wrote:
> VGA adapters need to claim memory and i/o
> transactions even if they do not have any
> i/o or memory bars. E.g. PCI spec, page 297,
> gives an example of such a device:
> 
>     Programming interface 0000 0000b
>     VGA-compatible controller. Memory
>     addresses 0A 0000h through 0B
>     FFFFh. I/O addresses 3B0h to 3BBh
>     and 3C0h to 3DFh and all aliases of
>     these addresses.
> 
> While we could check for these devices and special-case them, it is
> easier to fix this by enabling i/o and memory space unconditionally:
> devices that do not support it will just ignore this setting.

This doesn't sound correct to me - I would think the vga option rom
should enable the memory and io bars.  I don't have enough knowledge
to say for sure though - can someone else with knowledge in this area
confirm this approach?

-Kevin

Re: [Qemu-devel] [PATCHv2] seabios: enable io/memory unconditionally

[Avi Kivity]

On 10/09/2009 04:29 AM, Kevin O'Connor wrote:
> Hi Michael,
>
> On Thu, Oct 08, 2009 at 05:53:46PM +0200, Michael S. Tsirkin wrote:
>    
>> VGA adapters need to claim memory and i/o
>> transactions even if they do not have any
>> i/o or memory bars. E.g. PCI spec, page 297,
>> gives an example of such a device:
>>
>>      Programming interface 0000 0000b
>>      VGA-compatible controller. Memory
>>      addresses 0A 0000h through 0B
>>      FFFFh. I/O addresses 3B0h to 3BBh
>>      and 3C0h to 3DFh and all aliases of
>>      these addresses.
>>
>> While we could check for these devices and special-case them, it is
>> easier to fix this by enabling i/o and memory space unconditionally:
>> devices that do not support it will just ignore this setting.
>>      
> This doesn't sound correct to me - I would think the vga option rom
> should enable the memory and io bars.  I don't have enough knowledge
> to say for sure though - can someone else with knowledge in this area
> confirm this approach?
>
>    

The vga option rom is often itself in a BAR, so it cannot enable memory.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] [PATCHv2] seabios: enable io/memory unconditionally

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 10:29:53PM -0400, Kevin O'Connor wrote:
> Hi Michael,
> 
> On Thu, Oct 08, 2009 at 05:53:46PM +0200, Michael S. Tsirkin wrote:
> > VGA adapters need to claim memory and i/o
> > transactions even if they do not have any
> > i/o or memory bars. E.g. PCI spec, page 297,
> > gives an example of such a device:
> > 
> >     Programming interface 0000 0000b
> >     VGA-compatible controller. Memory
> >     addresses 0A 0000h through 0B
> >     FFFFh. I/O addresses 3B0h to 3BBh
> >     and 3C0h to 3DFh and all aliases of
> >     these addresses.
> > 
> > While we could check for these devices and special-case them, it is
> > easier to fix this by enabling i/o and memory space unconditionally:
> > devices that do not support it will just ignore this setting.
> 
> This doesn't sound correct to me - I would think the vga option rom
> should enable the memory and io bars.

You can do this, but in real systems, BIOS enables memory on all
devices. Just take a real system and do lspci -v on a device that has no
driver loaded.
And note how seabios does this *already* - it just doesn't
enable I/O for VGA device unless it has I/O bar, which is wrong.

>  I don't have enough knowledge
> to say for sure though - can someone else with knowledge in this area
> confirm this approach?
> 
> -Kevin

[Qemu-devel] [PATCHv2] seabios: enable io/memory unconditionally

[Michael S. Tsirkin]

VGA adapters need to claim memory and i/o
transactions even if they do not have any
i/o or memory bars. E.g. PCI spec, page 297,
gives an example of such a device:

    Programming interface 0000 0000b
    VGA-compatible controller. Memory
    addresses 0A 0000h through 0B
    FFFFh. I/O addresses 3B0h to 3BBh
    and 3C0h to 3DFh and all aliases of
    these addresses.

While we could check for these devices and special-case them, it is
easier to fix this by enabling i/o and memory space unconditionally:
devices that do not support it will just ignore this setting.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1: build fixes

 src/pciinit.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/src/pciinit.c b/src/pciinit.c
index 0d558a9..db70560 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -28,7 +28,6 @@ static u8 pci_irqs[4] = {
 
 static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
 {
-    u16 cmd;
     u32 ofs, old_addr;
 
     if (region_num == PCI_ROM_SLOT) {
@@ -41,16 +40,6 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
 
     pci_config_writel(bdf, ofs, addr);
     dprintf(1, "region %d: 0x%08x\n", region_num, addr);
-
-    /* enable memory mappings */
-    cmd = pci_config_readw(bdf, PCI_COMMAND);
-    if (region_num == PCI_ROM_SLOT)
-        cmd |= PCI_COMMAND_MEMORY;
-    else if (old_addr & PCI_BASE_ADDRESS_SPACE_IO)
-        cmd |= PCI_COMMAND_IO;
-    else
-        cmd |= PCI_COMMAND_MEMORY;
-    pci_config_writew(bdf, PCI_COMMAND, cmd);
 }
 
 /* return the global irq number corresponding to a given device irq
@@ -95,6 +84,7 @@ static void pci_bios_init_device(u16 bdf)
 {
     int class;
     u32 *paddr;
+    u16 cmd;
     int i, pin, pic_irq, vendor_id, device_id;
 
     class = pci_config_readw(bdf, PCI_CLASS_DEVICE);
@@ -165,6 +155,11 @@ static void pci_bios_init_device(u16 bdf)
         break;
     }
 
+    /* enable memory mappings */
+    cmd = pci_config_readw(bdf, PCI_COMMAND);
+    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
+    pci_config_writew(bdf, PCI_COMMAND, cmd);
+
     /* map the interrupt */
     pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
     if (pin != 0) {
-- 
1.6.5.rc2

[Qemu-devel] Re: [PATCHv2] seabios: enable io/memory unconditionally

[Kevin O'Connor]

On Mon, Oct 12, 2009 at 11:59:29AM +0200, Michael S. Tsirkin wrote:
> VGA adapters need to claim memory and i/o
> transactions even if they do not have any
> i/o or memory bars. E.g. PCI spec, page 297,
> gives an example of such a device:

I've modified your patch slightly (to use new pci_config_maskw helper)
and have committed it.

Thanks,
-Kevin

[Qemu-devel] Re: [PATCHv2] seabios: enable io/memory unconditionally

[Michael S. Tsirkin]

On Mon, Oct 12, 2009 at 10:38:06AM -0400, Kevin O'Connor wrote:
> On Mon, Oct 12, 2009 at 11:59:29AM +0200, Michael S. Tsirkin wrote:
> > VGA adapters need to claim memory and i/o
> > transactions even if they do not have any
> > i/o or memory bars. E.g. PCI spec, page 297,
> > gives an example of such a device:
> 
> I've modified your patch slightly (to use new pci_config_maskw helper)
> and have committed it.

Cool. Anthony, could you merge into savannah tree please?

> Thanks,
> -Kevin