diff mbox

In "pci_fixup_video" check if this is or should be the primary video devi

Message ID 1524137311.20140115232528@eikelenboom.it (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Sander Eikelenboom Jan. 15, 2014, 10:25 p.m. UTC 
Wednesday, January 15, 2014, 10:50:09 PM, you wrote:

> On Wed, Jan 15, 2014 at 12:36 PM, Sander Eikelenboom
> <linux@eikelenboom.it> wrote:
>> ...
>> And that's just what my patch does ..
>>
>> +       if (!vga_default_device() || pdev == vga_default_device()) {
>>
>> If we don't know the vga_default_device ... because we don't have that knowlegde
>> or
>> if this is actually the vga_default_device  ... because we do have that knowledge ..
>>
>> and only then .. run the fixup code and set this device as the vga_default_device
>>
>>
>> So this change actually makes the code adhere to the comment already above it .. saying it should only be applied to the vga_default_device aka
>> boot video device.
>>
>> Also added Bjorn and linux-pci to the CC .. should have done that right away ..
>> sorry for that.

> Can you resend your patch to linux-pci?  I don't think it made it there.

> Bjorn


Sure .. also attached for the case it gets mangled ..

Date: Sun, 12 Jan 2014 04:49:44 +0100
Subject: [PATCH] In "pci_fixup_video" check if this is or should be the
 primary video device to prevent setting the
 IORESOURCE_ROM_SHADOW flag on a secondary VGA card
To: Dave Airlie <airlied@redhat.com>,
    Eiichiro Oiwa <eiichiro.oiwa.nm@hitachi.com>,
    Greg Kroah-Hartman <gregkh@suse.de>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
    linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>

Setting the IORESOURCE_ROM_SHADOW flag on a secondary VGA card prevents if from
reading it's own rom. It will get the content of the shadowrom at C000 instead,
which is of the primary VGA card and the driver of the secondary card will bail
out.

Fix this by checking if this is or should be the primary video device before
applying the fix and let the comment reflect this.

Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>
---
 arch/x86/pci/fixup.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Bjorn Helgaas Jan. 16, 2014, 1:19 p.m. UTC | #1 
[+cc Michael, Jesse, David, qemu-devel]

On Wed, Jan 15, 2014 at 8:58 PM,  <eiichiro.oiwa.nm@hitachi.com> wrote:
> I suggest you should not break the PCI specification, as a developer of proprietary
> hypervisor, but I think your patch is no problem.
> Your PCI structure is specialized structure for your virtual machine.
> Maybe, your virtual machine will be causing another problem on Linux or other kernels
> because of breaking the PCI specification.

I assume you think qemu is breaking the PCI spec.  What exactly do you
think is broken?  Please give specific references to the spec.  This
conversation is pretty fragmented, and I came in late, so I apologize
if I missed this.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Jan. 30, 2014, 12:08 a.m. UTC | #2 
On Wed, Jan 15, 2014 at 11:25:28PM +0100, Sander Eikelenboom wrote:
> Date: Sun, 12 Jan 2014 04:49:44 +0100
> Subject: [PATCH] In "pci_fixup_video" check if this is or should be the
>  primary video device to prevent setting the
>  IORESOURCE_ROM_SHADOW flag on a secondary VGA card
> To: Dave Airlie <airlied@redhat.com>,
>     Eiichiro Oiwa <eiichiro.oiwa.nm@hitachi.com>,
>     Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
>     linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> 
> Setting the IORESOURCE_ROM_SHADOW flag on a secondary VGA card prevents if from
> reading it's own rom. It will get the content of the shadowrom at C000 instead,
> which is of the primary VGA card and the driver of the secondary card will bail
> out.
> 
> Fix this by checking if this is or should be the primary video device before
> applying the fix and let the comment reflect this.
> 
> Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>
> ---
>  arch/x86/pci/fixup.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index b046e07..525e49a 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -314,9 +314,9 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,        PCI_DEVICE_ID_INTEL_MCH_PC1,    pcie_r
>   * IORESOURCE_ROM_SHADOW is used to associate the boot video
>   * card with this copy. On laptops this copy has to be used since
>   * the main ROM may be compressed or combined with another image.
> - * See pci_map_rom() for use of this flag. IORESOURCE_ROM_SHADOW
> - * is marked here since the boot video device will be the only enabled
> - * video device at this point.
> + * See pci_map_rom() for use of this flag. Before we mark the device
> + * with IORESOURCE_ROM_SHADOW we have to check if this is or should become
> + * the primary video card, since this quirk is ran for all video devices.
>   */
>  
>  static void pci_fixup_video(struct pci_dev *pdev)
> @@ -347,12 +347,13 @@ static void pci_fixup_video(struct pci_dev *pdev)
>                 }
>                 bus = bus->parent;
>         }
> -       pci_read_config_word(pdev, PCI_COMMAND, &config);
> -       if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> -               pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
> -               dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
> -               if (!vga_default_device())
> +       if (!vga_default_device() || pdev == vga_default_device()) {
> +               pci_read_config_word(pdev, PCI_COMMAND, &config);
> +               if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> +                       pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
> +                       dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
>                         vga_set_default_device(pdev);
> +               }

ia64 also has a pci_fixup_video() that is essentially the same.  Can you
fix that one as well, unless there is a reason why the fix applies only to
x86 and not to ia64?

>         }
>  }
>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> -- 
> 1.7.10.4


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sander Eikelenboom Jan. 31, 2014, 9:28 a.m. UTC | #3 
Hi Bjorn / Tony,

I fixed up ia64 as well and brought it inline again with the x86 code,
but i don't have a ia64 machine, so that part is untested.
Perhaps Tony is able to review/test it ?

Sander



Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the primary
prevents it from reading it's own rom. It will get the content of the shadowrom
at C000 instead, which is of the primary VGA card and the driver of the
secondary card will bail out.

Fix this by checking if the arch code or vga-arbitration has already
determined the vga_default_device, if so only apply the fix to this
primary video device and let the comment reflect this.

v2:
    - Fix pci_fixup_video both in x86 and ia64


Sander Eikelenboom (1):
  Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the
    primary     prevents it from reading it's own rom. It will get the
    content of the shadowrom     at C000 instead, which is of the
    primary VGA card and the driver of the     secondary card will bail
    out.

 arch/ia64/pci/fixup.c |   24 +++++++++++++-----------
 arch/x86/pci/fixup.c  |   18 ++++++++++--------
 2 files changed, 23 insertions(+), 19 deletions(-)
Konrad Rzeszutek Wilk Feb. 3, 2014, 2:52 p.m. UTC | #4 
On Fri, Jan 31, 2014 at 10:28:22AM +0100, Sander Eikelenboom wrote:
> Hi Bjorn / Tony,
> 
> I fixed up ia64 as well and brought it inline again with the x86 code,
> but i don't have a ia64 machine, so that part is untested.
> Perhaps Tony is able to review/test it ?
> 
> Sander
> 
> 
> 
> Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the primary
> prevents it from reading it's own rom. It will get the content of the shadowrom
> at C000 instead, which is of the primary VGA card and the driver of the
> secondary card will bail out.
> 
> Fix this by checking if the arch code or vga-arbitration has already
> determined the vga_default_device, if so only apply the fix to this
> primary video device and let the comment reflect this.
> 
> v2:
>     - Fix pci_fixup_video both in x86 and ia64
> 
> 
> Sander Eikelenboom (1):
>   Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the
>     primary     prevents it from reading it's own rom. It will get the
>     content of the shadowrom     at C000 instead, which is of the
>     primary VGA card and the driver of the     secondary card will bail
>     out.

Your editor mutilated your subject line. It ought to have been just
one line.

Anyhow, you can also add 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com'
on the patch for the x86 part.

The ia64 "looks" OK to me, but my ia64 box won't boot v3.11 or later
so I can't give it a 'Tested-by' yet.

> 
>  arch/ia64/pci/fixup.c |   24 +++++++++++++-----------
>  arch/x86/pci/fixup.c  |   18 ++++++++++--------
>  2 files changed, 23 insertions(+), 19 deletions(-)
> 
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sander Eikelenboom Feb. 7, 2014, 11:03 a.m. UTC | #5 
Hi All,

Still trying to collect tested and acked-by's ..

Dave,
          In commit 6cf20beec4b91c240cf759b4db72669e251f1fc4 you changed pci_fixup_video saying:
          "This fixes the switcheroo on my T410s."
          Do you still have the machine and could you verify if it still works with this patch applied ?

Eiichiro,
          Most of last major overhaul of pci_fixup_video was in commit 6b5c76b8e2ff204fa8d7201acce461188873bf2b saying:
          "PCI: fix pci_fixup_video as it blows up on sparc64"
          Is it possible for you to verify you can still boot on sparc64 with this patch applied ?

Tony,
          Since ia64's pci_fixup_video is also changed to be inline with x86, could you verify it still compiles and boots for you ?


Konrad,
          Thanks for reviewing and testing !

--
Sander




Monday, February 3, 2014, 3:52:05 PM, you wrote:

> On Fri, Jan 31, 2014 at 10:28:22AM +0100, Sander Eikelenboom wrote:
>> Hi Bjorn / Tony,
>> 
>> I fixed up ia64 as well and brought it inline again with the x86 code,
>> but i don't have a ia64 machine, so that part is untested.
>> Perhaps Tony is able to review/test it ?
>> 
>> Sander
>> 
>> 
>> 
>> Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the primary
>> prevents it from reading it's own rom. It will get the content of the shadowrom
>> at C000 instead, which is of the primary VGA card and the driver of the
>> secondary card will bail out.
>> 
>> Fix this by checking if the arch code or vga-arbitration has already
>> determined the vga_default_device, if so only apply the fix to this
>> primary video device and let the comment reflect this.
>> 
>> v2:
>>     - Fix pci_fixup_video both in x86 and ia64
>> 
>> 
>> Sander Eikelenboom (1):
>>   Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the
>>     primary     prevents it from reading it's own rom. It will get the
>>     content of the shadowrom     at C000 instead, which is of the
>>     primary VGA card and the driver of the     secondary card will bail
>>     out.

> Your editor mutilated your subject line. It ought to have been just
> one line.

> Anyhow, you can also add 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com'
> on the patch for the x86 part.

> The ia64 "looks" OK to me, but my ia64 box won't boot v3.11 or later
> so I can't give it a 'Tested-by' yet.

>> 
>>  arch/ia64/pci/fixup.c |   24 +++++++++++++-----------
>>  arch/x86/pci/fixup.c  |   18 ++++++++++--------
>>  2 files changed, 23 insertions(+), 19 deletions(-)
>> 
>> -- 
>> 1.7.10.4
>> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sander Eikelenboom Feb. 13, 2014, 9:48 a.m. UTC | #6 
Hi Bjorn,

I have given it another email and another week, but without gaining any reviewed or acked-by's.
It seems the only way forward is to shovel it in linux-next earlier, give it a good soak and see if
anyone starts to squeal .. or that everything seems to be ok :-)

Would you need a v3 with the acked and reviewed-by from Konrad for x86 in it ?

--

Sander


Monday, February 3, 2014, 3:52:05 PM, you wrote:

> On Fri, Jan 31, 2014 at 10:28:22AM +0100, Sander Eikelenboom wrote:
>> Hi Bjorn / Tony,
>> 
>> I fixed up ia64 as well and brought it inline again with the x86 code,
>> but i don't have a ia64 machine, so that part is untested.
>> Perhaps Tony is able to review/test it ?
>> 
>> Sander
>> 
>> 
>> 
>> Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the primary
>> prevents it from reading it's own rom. It will get the content of the shadowrom
>> at C000 instead, which is of the primary VGA card and the driver of the
>> secondary card will bail out.
>> 
>> Fix this by checking if the arch code or vga-arbitration has already
>> determined the vga_default_device, if so only apply the fix to this
>> primary video device and let the comment reflect this.
>> 
>> v2:
>>     - Fix pci_fixup_video both in x86 and ia64
>> 
>> 
>> Sander Eikelenboom (1):
>>   Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the
>>     primary     prevents it from reading it's own rom. It will get the
>>     content of the shadowrom     at C000 instead, which is of the
>>     primary VGA card and the driver of the     secondary card will bail
>>     out.

> Your editor mutilated your subject line. It ought to have been just
> one line.

> Anyhow, you can also add 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com'
> on the patch for the x86 part.

> The ia64 "looks" OK to me, but my ia64 box won't boot v3.11 or later
> so I can't give it a 'Tested-by' yet.

>> 
>>  arch/ia64/pci/fixup.c |   24 +++++++++++++-----------
>>  arch/x86/pci/fixup.c  |   18 ++++++++++--------
>>  2 files changed, 23 insertions(+), 19 deletions(-)
>> 
>> -- 
>> 1.7.10.4
>> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 14, 2014, 8:26 p.m. UTC | #7 
On Thu, Feb 13, 2014 at 10:48:24AM +0100, Sander Eikelenboom wrote:
> Hi Bjorn,
> 
> I have given it another email and another week, but without gaining any reviewed or acked-by's.
> It seems the only way forward is to shovel it in linux-next earlier, give it a good soak and see if
> anyone starts to squeal .. or that everything seems to be ok :-)
> 
> Would you need a v3 with the acked and reviewed-by from Konrad for x86 in it ?

No need, I applied this to my pci/misc branch for v3.15.

Tony, let me know if you have any issues with this.

Bjorn

> Monday, February 3, 2014, 3:52:05 PM, you wrote:
> 
> > On Fri, Jan 31, 2014 at 10:28:22AM +0100, Sander Eikelenboom wrote:
> >> Hi Bjorn / Tony,
> >> 
> >> I fixed up ia64 as well and brought it inline again with the x86 code,
> >> but i don't have a ia64 machine, so that part is untested.
> >> Perhaps Tony is able to review/test it ?
> >> 
> >> Sander
> >> 
> >> 
> >> 
> >> Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the primary
> >> prevents it from reading it's own rom. It will get the content of the shadowrom
> >> at C000 instead, which is of the primary VGA card and the driver of the
> >> secondary card will bail out.
> >> 
> >> Fix this by checking if the arch code or vga-arbitration has already
> >> determined the vga_default_device, if so only apply the fix to this
> >> primary video device and let the comment reflect this.
> >> 
> >> v2:
> >>     - Fix pci_fixup_video both in x86 and ia64
> >> 
> >> 
> >> Sander Eikelenboom (1):
> >>   Setting the IORESOURCE_ROM_SHADOW flag on a VGA card other than the
> >>     primary     prevents it from reading it's own rom. It will get the
> >>     content of the shadowrom     at C000 instead, which is of the
> >>     primary VGA card and the driver of the     secondary card will bail
> >>     out.
> 
> > Your editor mutilated your subject line. It ought to have been just
> > one line.
> 
> > Anyhow, you can also add 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com'
> > on the patch for the x86 part.
> 
> > The ia64 "looks" OK to me, but my ia64 box won't boot v3.11 or later
> > so I can't give it a 'Tested-by' yet.
> 
> >> 
> >>  arch/ia64/pci/fixup.c |   24 +++++++++++++-----------
> >>  arch/x86/pci/fixup.c  |   18 ++++++++++--------
> >>  2 files changed, 23 insertions(+), 19 deletions(-)
> >> 
> >> -- 
> >> 1.7.10.4
> >> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index b046e07..525e49a 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -314,9 +314,9 @@  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,        PCI_DEVICE_ID_INTEL_MCH_PC1,    pcie_r
  * IORESOURCE_ROM_SHADOW is used to associate the boot video
  * card with this copy. On laptops this copy has to be used since
  * the main ROM may be compressed or combined with another image.
- * See pci_map_rom() for use of this flag. IORESOURCE_ROM_SHADOW
- * is marked here since the boot video device will be the only enabled
- * video device at this point.
+ * See pci_map_rom() for use of this flag. Before we mark the device
+ * with IORESOURCE_ROM_SHADOW we have to check if this is or should become
+ * the primary video card, since this quirk is ran for all video devices.
  */
 
 static void pci_fixup_video(struct pci_dev *pdev)
@@ -347,12 +347,13 @@  static void pci_fixup_video(struct pci_dev *pdev)
                }
                bus = bus->parent;
        }
-       pci_read_config_word(pdev, PCI_COMMAND, &config);
-       if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
-               pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
-               dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
-               if (!vga_default_device())
+       if (!vga_default_device() || pdev == vga_default_device()) {
+               pci_read_config_word(pdev, PCI_COMMAND, &config);
+               if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+                       pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
+                       dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
                        vga_set_default_device(pdev);
+               }
        }
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,