[Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Gerd Hoffmann]

qemu uses "pci" as name for pci bridges in the firmware device path.
seabios expects "pci-bridge".  Result is that bootorder is broken for
devices behind pci bridges.

Some googling suggests that "pci-bridge" is the correct one.  At least
PPC-based Apple machines are using this.  See question "How do I boot
from a device attached to a PCI card" here:
	http://www.netbsd.org/ports/macppc/faq.html

So lets change qemu to use "pci-bridge" too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 49eca95..82c11ec 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1330,7 +1330,7 @@ static const pci_class_desc pci_class_descriptions[] =
     { 0x0601, "ISA bridge", "isa"},
     { 0x0602, "EISA bridge", "eisa"},
     { 0x0603, "MC bridge", "mca"},
-    { 0x0604, "PCI bridge", "pci"},
+    { 0x0604, "PCI bridge", "pci-bridge"},
     { 0x0605, "PCMCIA bridge", "pcmcia"},
     { 0x0606, "NUBUS bridge", "nubus"},
     { 0x0607, "CARDBUS bridge", "cardbus"},
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Michael S. Tsirkin]

On Fri, Dec 06, 2013 at 12:24:40PM +0100, Gerd Hoffmann wrote:
> qemu uses "pci" as name for pci bridges in the firmware device path.
> seabios expects "pci-bridge".  Result is that bootorder is broken for
> devices behind pci bridges.
> 
> Some googling suggests that "pci-bridge" is the correct one.  At least
> PPC-based Apple machines are using this.  See question "How do I boot
> from a device attached to a PCI card" here:
> 	http://www.netbsd.org/ports/macppc/faq.html
> 
> So lets change qemu to use "pci-bridge" too.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Sounds good but this needs to be enabled for
new machine types only - otherwise guest will read
corrupted data since bootindex is in FW CFG.

Alternatively, extend
04920fc0faa4760f9c4fc0e73b992b768099be70
to work for all fw cfg file entries.

I think the whole boot index thing needs
to be reworked: we also have the long-standing feature request to allow
booting from a device added by hot add.

> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 49eca95..82c11ec 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1330,7 +1330,7 @@ static const pci_class_desc pci_class_descriptions[] =
>      { 0x0601, "ISA bridge", "isa"},
>      { 0x0602, "EISA bridge", "eisa"},
>      { 0x0603, "MC bridge", "mca"},
> -    { 0x0604, "PCI bridge", "pci"},
> +    { 0x0604, "PCI bridge", "pci-bridge"},
>      { 0x0605, "PCMCIA bridge", "pcmcia"},
>      { 0x0606, "NUBUS bridge", "nubus"},
>      { 0x0607, "CARDBUS bridge", "cardbus"},
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Gerd Hoffmann]

On So, 2013-12-08 at 13:31 +0200, Michael S. Tsirkin wrote:
> On Fri, Dec 06, 2013 at 12:24:40PM +0100, Gerd Hoffmann wrote:
> > qemu uses "pci" as name for pci bridges in the firmware device path.
> > seabios expects "pci-bridge".  Result is that bootorder is broken for
> > devices behind pci bridges.
> > 
> > Some googling suggests that "pci-bridge" is the correct one.  At least
> > PPC-based Apple machines are using this.  See question "How do I boot
> > from a device attached to a PCI card" here:
> > 	http://www.netbsd.org/ports/macppc/faq.html
> > 
> > So lets change qemu to use "pci-bridge" too.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Sounds good but this needs to be enabled for
> new machine types only - otherwise guest will read
> corrupted data since bootindex is in FW CFG.

Come on, that is seriously over-engineering.

First, the chance that you'll find setup in the field where this can
actually happen in practice is very low as bootorder for devices behind
pci bridges does not work at all without this patch.

Second, the race window is so tiny that nobody has ever seen that happen
in testing.

> Alternatively, extend
> 04920fc0faa4760f9c4fc0e73b992b768099be70
> to work for all fw cfg file entries.
> 
> I think the whole boot index thing needs
> to be reworked: we also have the long-standing feature request to allow
> booting from a device added by hot add.

I'm not going to rewrite bootorder just to get a one-liner bugfix
merged.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Marcel Apfelbaum]

On Mon, 2013-12-09 at 08:14 +0100, Gerd Hoffmann wrote:
> On So, 2013-12-08 at 13:31 +0200, Michael S. Tsirkin wrote:
> > On Fri, Dec 06, 2013 at 12:24:40PM +0100, Gerd Hoffmann wrote:
> > > qemu uses "pci" as name for pci bridges in the firmware device path.
> > > seabios expects "pci-bridge".  Result is that bootorder is broken for
> > > devices behind pci bridges.
> > > 
> > > Some googling suggests that "pci-bridge" is the correct one.  At least
> > > PPC-based Apple machines are using this.  See question "How do I boot
> > > from a device attached to a PCI card" here:
> > > 	http://www.netbsd.org/ports/macppc/faq.html
> > > 
> > > So lets change qemu to use "pci-bridge" too.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > Sounds good but this needs to be enabled for
> > new machine types only - otherwise guest will read
> > corrupted data since bootindex is in FW CFG.
> 
> Come on, that is seriously over-engineering.
Does this change affects migration in any way?

> 
> First, the chance that you'll find setup in the field where this can
> actually happen in practice is very low as bootorder for devices behind
> pci bridges does not work at all without this patch.
> 
> Second, the race window is so tiny that nobody has ever seen that happen
> in testing.
> 
> > Alternatively, extend
> > 04920fc0faa4760f9c4fc0e73b992b768099be70
> > to work for all fw cfg file entries.
> > 
> > I think the whole boot index thing needs
> > to be reworked: we also have the long-standing feature request to allow
> > booting from a device added by hot add.
> 
> I'm not going to rewrite bootorder just to get a one-liner bugfix
> merged.
Why? You could rewrite Qemu in C++, then rewrite the 
bootorder (easier, because of C++) and finally get your bugfix in...
but again, your patch would look exactly the same :)

Thanks,
Marcel

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Gerd Hoffmann]

On Mo, 2013-12-09 at 11:24 +0200, Marcel Apfelbaum wrote:
> On Mon, 2013-12-09 at 08:14 +0100, Gerd Hoffmann wrote:
> > On So, 2013-12-08 at 13:31 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Dec 06, 2013 at 12:24:40PM +0100, Gerd Hoffmann wrote:
> > > > qemu uses "pci" as name for pci bridges in the firmware device path.
> > > > seabios expects "pci-bridge".  Result is that bootorder is broken for
> > > > devices behind pci bridges.
> > > > 
> > > > Some googling suggests that "pci-bridge" is the correct one.  At least
> > > > PPC-based Apple machines are using this.  See question "How do I boot
> > > > from a device attached to a PCI card" here:
> > > > 	http://www.netbsd.org/ports/macppc/faq.html
> > > > 
> > > > So lets change qemu to use "pci-bridge" too.
> > > > 
> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > 
> > > Sounds good but this needs to be enabled for
> > > new machine types only - otherwise guest will read
> > > corrupted data since bootindex is in FW CFG.
> > 
> > Come on, that is seriously over-engineering.
> Does this change affects migration in any way?

If you live-migrate from a machine without the fix to a machine with the
fix *and* you have bootorder configured for a device behind the pci
bridge then size of the etc/bootorder fw_cfg file will change.

So, in case of the following event sequence ...

  (a) firmware reads fw_fg directory index (with file sizes)
  (b) live migration
  (c) firmware reads etc/bootorder

... the firmware will read the new content with old size, i.e. it will
miss a few bytes or have trailing garbage (or zeros?  Not sure what qemu
emulation does if you read over the end of a fw_cfg files).

Result might be that bootorder isn't applied correctly.  But that is the
case _anyway_ on the machine with the unfixed qemu version.  So making
this machine type dependent buys us nothing but churn for no reason.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Michael S. Tsirkin]

On Mon, Dec 09, 2013 at 08:14:30AM +0100, Gerd Hoffmann wrote:
> On So, 2013-12-08 at 13:31 +0200, Michael S. Tsirkin wrote:
> > On Fri, Dec 06, 2013 at 12:24:40PM +0100, Gerd Hoffmann wrote:
> > > qemu uses "pci" as name for pci bridges in the firmware device path.
> > > seabios expects "pci-bridge".  Result is that bootorder is broken for
> > > devices behind pci bridges.
> > > 
> > > Some googling suggests that "pci-bridge" is the correct one.  At least
> > > PPC-based Apple machines are using this.  See question "How do I boot
> > > from a device attached to a PCI card" here:
> > > 	http://www.netbsd.org/ports/macppc/faq.html
> > > 
> > > So lets change qemu to use "pci-bridge" too.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > Sounds good but this needs to be enabled for
> > new machine types only - otherwise guest will read
> > corrupted data since bootindex is in FW CFG.
> 
> Come on, that is seriously over-engineering.
> 
> First, the chance that you'll find setup in the field where this can
> actually happen in practice is very low as bootorder for devices behind
> pci bridges does not work at all without this patch.

That's a good point, thanks.

> Second, the race window is so tiny that nobody has ever seen that happen
> in testing.
> 
> > Alternatively, extend
> > 04920fc0faa4760f9c4fc0e73b992b768099be70
> > to work for all fw cfg file entries.
> > 
> > I think the whole boot index thing needs
> > to be reworked: we also have the long-standing feature request to allow
> > booting from a device added by hot add.
> 
> I'm not going to rewrite bootorder just to get a one-liner bugfix
> merged.
> 
> cheers,
>   Gerd

-- 
MST

Re: [Qemu-devel] [PATCH] pci: fix pci bridge fw path

[Michael S. Tsirkin]

On Fri, Dec 06, 2013 at 12:24:40PM +0100, Gerd Hoffmann wrote:
> qemu uses "pci" as name for pci bridges in the firmware device path.
> seabios expects "pci-bridge".  Result is that bootorder is broken for
> devices behind pci bridges.
> 
> Some googling suggests that "pci-bridge" is the correct one.  At least
> PPC-based Apple machines are using this.  See question "How do I boot
> from a device attached to a PCI card" here:
> 	http://www.netbsd.org/ports/macppc/faq.html
> 
> So lets change qemu to use "pci-bridge" too.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Applied, thanks.

> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 49eca95..82c11ec 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1330,7 +1330,7 @@ static const pci_class_desc pci_class_descriptions[] =
>      { 0x0601, "ISA bridge", "isa"},
>      { 0x0602, "EISA bridge", "eisa"},
>      { 0x0603, "MC bridge", "mca"},
> -    { 0x0604, "PCI bridge", "pci"},
> +    { 0x0604, "PCI bridge", "pci-bridge"},
>      { 0x0605, "PCMCIA bridge", "pcmcia"},
>      { 0x0606, "NUBUS bridge", "nubus"},
>      { 0x0607, "CARDBUS bridge", "cardbus"},
> -- 
> 1.8.3.1