[PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[mike.travis]

[-- Attachment #1: skip-non-vga-devices --]
[-- Type: text/plain, Size: 1622 bytes --]

A GPU inserted into a PCIe I/O slot disappears during system startup.
The problem centers around GRUB and a specific VGA init function in
efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
memory has not been enabled before attempting access.

The fix is to add the same coding used in other VGA drivers, specifically
to add a check to insure that it is indeed a VGA controller.  And then
enable the MMIO address space with the specific bits.

Signed-off-by: Mike Travis <mike.travis@hpe.com>
Reviewed-by: Michael Chang <mchang@suse.com>
Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
---
v1:change class to subclass, remove parens around "enable mem" code
---
 grub-2.02/grub-core/video/efi_uga.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Index: grub-2.02/grub-core/video/efi_uga.c
===================================================================
--- grub-2.02.orig/grub-core/video/efi_uga.c
+++ grub-2.02/grub-core/video/efi_uga.c
@@ -95,9 +95,18 @@ find_card (grub_pci_device_t dev, grub_p
 {
   struct find_framebuf_ctx *ctx = data;
   grub_pci_address_t addr;
+  grub_pci_address_t rcaddr;
+  grub_uint32_t subclass;
 
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-  if (grub_pci_read (addr) >> 24 == 0x3)
+  subclass = (grub_pci_read (addr) >> 16) & 0xffff;
+  if (subclass != GRUB_PCI_CLASS_SUBCLASS_VGA)
+    return 0;
+
+  /* Enable MEM address spaces */
+  rcaddr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+  grub_pci_write_word (rcaddr, grub_pci_read_word (rcaddr) | GRUB_PCI_COMMAND_MEM_ENABLED);
+
     {
       int i;
 

-- 

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Daniel Kiper]

On Wed, Mar 28, 2018 at 11:42:18AM -0500, mike.travis@hpe.com wrote:
> A GPU inserted into a PCIe I/O slot disappears during system startup.
> The problem centers around GRUB and a specific VGA init function in
> efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
> memory has not been enabled before attempting access.
>
> The fix is to add the same coding used in other VGA drivers, specifically
> to add a check to insure that it is indeed a VGA controller.  And then
> enable the MMIO address space with the specific bits.
>
> Signed-off-by: Mike Travis <mike.travis@hpe.com>
> Reviewed-by: Michael Chang <mchang@suse.com>
> Reviewed-by: Daniel Kiper <dkiper@net-space.pl>

Well, please do not add somebody RB tag if he/she did not explicitly
asked you to do that. And even in that case I was not able to look at
this patch in advance. So, my RB should not be here. Additionally, in
this situation I would like to ask if Michael approved his RB?

Anyway, patch LGTM except one nitpick. I will apply the patch, in a week or
so, with Michael's RB if I get confirmation that he approved it earlier.

> ---
> v1:change class to subclass, remove parens around "enable mem" code
> ---
>  grub-2.02/grub-core/video/efi_uga.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> Index: grub-2.02/grub-core/video/efi_uga.c
> ===================================================================
> --- grub-2.02.orig/grub-core/video/efi_uga.c
> +++ grub-2.02/grub-core/video/efi_uga.c
> @@ -95,9 +95,18 @@ find_card (grub_pci_device_t dev, grub_p
>  {
>    struct find_framebuf_ctx *ctx = data;
>    grub_pci_address_t addr;
> +  grub_pci_address_t rcaddr;

This line should be together with the addr. I will fix it before
committing the patch.

Thank you for doing the work.

Daniel

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Michael Chang]

On Thu, Mar 29, 2018 at 11:02:51AM +0200, Daniel Kiper wrote:
> On Wed, Mar 28, 2018 at 11:42:18AM -0500, mike.travis@hpe.com wrote:
> > A GPU inserted into a PCIe I/O slot disappears during system startup.
> > The problem centers around GRUB and a specific VGA init function in
> > efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
> > memory has not been enabled before attempting access.
> >
> > The fix is to add the same coding used in other VGA drivers, specifically
> > to add a check to insure that it is indeed a VGA controller.  And then
> > enable the MMIO address space with the specific bits.
> >
> > Signed-off-by: Mike Travis <mike.travis@hpe.com>
> > Reviewed-by: Michael Chang <mchang@suse.com>
> > Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
> 
> Well, please do not add somebody RB tag if he/she did not explicitly
> asked you to do that. And even in that case I was not able to look at
> this patch in advance. So, my RB should not be here. Additionally, in
> this situation I would like to ask if Michael approved his RB?

We did have discussion about the patch before it was submitted upstream but I
did not ask for RB as well. 

> 
> Anyway, patch LGTM except one nitpick. I will apply the patch, in a week or
> so, with Michael's RB if I get confirmation that he approved it earlier.

As I did not ask for it, it has to be removed.

Thanks for notifying this.
Michael

> 
> > ---
> > v1:change class to subclass, remove parens around "enable mem" code
> > ---
> >  grub-2.02/grub-core/video/efi_uga.c |   15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > Index: grub-2.02/grub-core/video/efi_uga.c
> > ===================================================================
> > --- grub-2.02.orig/grub-core/video/efi_uga.c
> > +++ grub-2.02/grub-core/video/efi_uga.c
> > @@ -95,9 +95,18 @@ find_card (grub_pci_device_t dev, grub_p
> >  {
> >    struct find_framebuf_ctx *ctx = data;
> >    grub_pci_address_t addr;
> > +  grub_pci_address_t rcaddr;
> 
> This line should be together with the addr. I will fix it before
> committing the patch.
> 
> Thank you for doing the work.
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Daniel Kiper]

On Thu, Mar 29, 2018 at 05:29:01PM +0800, Michael Chang wrote:
> On Thu, Mar 29, 2018 at 11:02:51AM +0200, Daniel Kiper wrote:
> > On Wed, Mar 28, 2018 at 11:42:18AM -0500, mike.travis@hpe.com wrote:
> > > A GPU inserted into a PCIe I/O slot disappears during system startup.
> > > The problem centers around GRUB and a specific VGA init function in
> > > efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
> > > memory has not been enabled before attempting access.
> > >
> > > The fix is to add the same coding used in other VGA drivers, specifically
> > > to add a check to insure that it is indeed a VGA controller.  And then
> > > enable the MMIO address space with the specific bits.
> > >
> > > Signed-off-by: Mike Travis <mike.travis@hpe.com>
> > > Reviewed-by: Michael Chang <mchang@suse.com>
> > > Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
> >
> > Well, please do not add somebody RB tag if he/she did not explicitly
> > asked you to do that. And even in that case I was not able to look at
> > this patch in advance. So, my RB should not be here. Additionally, in
> > this situation I would like to ask if Michael approved his RB?
>
> We did have discussion about the patch before it was submitted upstream but I
> did not ask for RB as well.
>
> > Anyway, patch LGTM except one nitpick. I will apply the patch, in a week or
> > so, with Michael's RB if I get confirmation that he approved it earlier.
>
> As I did not ask for it, it has to be removed.

OK, I will commit this patch without your RB.

Daniel

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Mike Travis]

On 3/29/2018 2:36 AM, Daniel Kiper wrote:
> On Thu, Mar 29, 2018 at 05:29:01PM +0800, Michael Chang wrote:
>> On Thu, Mar 29, 2018 at 11:02:51AM +0200, Daniel Kiper wrote:
>>> On Wed, Mar 28, 2018 at 11:42:18AM -0500, mike.travis@hpe.com wrote:
>>>> A GPU inserted into a PCIe I/O slot disappears during system startup.
>>>> The problem centers around GRUB and a specific VGA init function in
>>>> efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
>>>> memory has not been enabled before attempting access.
>>>>
>>>> The fix is to add the same coding used in other VGA drivers, specifically
>>>> to add a check to insure that it is indeed a VGA controller.  And then
>>>> enable the MMIO address space with the specific bits.
>>>>
>>>> Signed-off-by: Mike Travis <mike.travis@hpe.com>
>>>> Reviewed-by: Michael Chang <mchang@suse.com>
>>>> Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
>>>
>>> Well, please do not add somebody RB tag if he/she did not explicitly
>>> asked you to do that. And even in that case I was not able to look at
>>> this patch in advance. So, my RB should not be here. Additionally, in
>>> this situation I would like to ask if Michael approved his RB?
>>
>> We did have discussion about the patch before it was submitted upstream but I
>> did not ask for RB as well.
>>
>>> Anyway, patch LGTM except one nitpick. I will apply the patch, in a week or
>>> so, with Michael's RB if I get confirmation that he approved it earlier.
>>
>> As I did not ask for it, it has to be removed.
> 
> OK, I will commit this patch without your RB.
> 
> Daniel
> 

Sorry, I was not aware of this custom.  I thought that if someone 
reviewed the code then that was an indication that it should be noted. 
I am aware of the more restrictive Acked-by which is normally sent by 
the "acker" to the submitter.  I will be more mindful of this in the 
future.  (This is my first posting to any of the GNU mail lists).

Thanks,
Mike

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Hans de Goede]

Hi,

On 29-03-18 16:27, Mike Travis wrote:
> 
> 
> On 3/29/2018 2:36 AM, Daniel Kiper wrote:
>> On Thu, Mar 29, 2018 at 05:29:01PM +0800, Michael Chang wrote:
>>> On Thu, Mar 29, 2018 at 11:02:51AM +0200, Daniel Kiper wrote:
>>>> On Wed, Mar 28, 2018 at 11:42:18AM -0500, mike.travis@hpe.com wrote:
>>>>> A GPU inserted into a PCIe I/O slot disappears during system startup.
>>>>> The problem centers around GRUB and a specific VGA init function in
>>>>> efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
>>>>> memory has not been enabled before attempting access.
>>>>>
>>>>> The fix is to add the same coding used in other VGA drivers, specifically
>>>>> to add a check to insure that it is indeed a VGA controller.  And then
>>>>> enable the MMIO address space with the specific bits.
>>>>>
>>>>> Signed-off-by: Mike Travis <mike.travis@hpe.com>
>>>>> Reviewed-by: Michael Chang <mchang@suse.com>
>>>>> Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
>>>>
>>>> Well, please do not add somebody RB tag if he/she did not explicitly
>>>> asked you to do that. And even in that case I was not able to look at
>>>> this patch in advance. So, my RB should not be here. Additionally, in
>>>> this situation I would like to ask if Michael approved his RB?
>>>
>>> We did have discussion about the patch before it was submitted upstream but I
>>> did not ask for RB as well.
>>>
>>>> Anyway, patch LGTM except one nitpick. I will apply the patch, in a week or
>>>> so, with Michael's RB if I get confirmation that he approved it earlier.
>>>
>>> As I did not ask for it, it has to be removed.
>>
>> OK, I will commit this patch without your RB.
>>
>> Daniel
>>
> 
> Sorry, I was not aware of this custom.  I thought that if someone reviewed the code then that was an indication that it should be noted. I am aware of the more restrictive Acked-by which is normally sent by the "acker" to the submitter.  I will be more mindful of this in the future.  (This is my first posting to any of the GNU mail lists).

Reviewed-by actually is stronger then Acked-by at least that is
what I believe, I've always interpreted these 2 as:

Acked-by: foo:     Foo took a quick glance at the proposed changes and thinks this is a sensible change
Reviewed-by: foo:  Foo did a detailed review of the code and did not find any issues

Regards,

Hans

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Daniel Kiper]

On Thu, Mar 29, 2018 at 05:00:56PM +0200, Hans de Goede wrote:
> Hi,
>
> On 29-03-18 16:27, Mike Travis wrote:
> >
> >
> >On 3/29/2018 2:36 AM, Daniel Kiper wrote:
> >>On Thu, Mar 29, 2018 at 05:29:01PM +0800, Michael Chang wrote:
> >>>On Thu, Mar 29, 2018 at 11:02:51AM +0200, Daniel Kiper wrote:
> >>>>On Wed, Mar 28, 2018 at 11:42:18AM -0500, mike.travis@hpe.com wrote:
> >>>>>A GPU inserted into a PCIe I/O slot disappears during system startup.
> >>>>>The problem centers around GRUB and a specific VGA init function in
> >>>>>efi_uga.c.?? This causes an LER (link error recorvery) because the MMIO
> >>>>>memory has not been enabled before attempting access.
> >>>>>
> >>>>>The fix is to add the same coding used in other VGA drivers,
> >>>>>specifically
> >>>>>to add a check to insure that it is indeed a VGA controller.?? And then
> >>>>>enable the MMIO address space with the specific bits.
> >>>>>
> >>>>>Signed-off-by: Mike Travis <mike.travis@hpe.com>
> >>>>>Reviewed-by: Michael Chang <mchang@suse.com>
> >>>>>Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
> >>>>
> >>>>Well, please do not add somebody RB tag if he/she did not explicitly
> >>>>asked you to do that. And even in that case I was not able to look at
> >>>>this patch in advance. So, my RB should not be here. Additionally, in
> >>>>this situation I would like to ask if Michael approved his RB?
> >>>
> >>>We did have discussion about the patch before it was submitted upstream
> >>>but I
> >>>did not ask for RB as well.
> >>>
> >>>>Anyway, patch LGTM except one nitpick. I will apply the patch, in a
> >>>>week or
> >>>>so, with Michael's RB if I get confirmation that he approved it earlier.
> >>>
> >>>As I did not ask for it, it has to be removed.
> >>
> >>OK, I will commit this patch without your RB.
> >>
> >>Daniel
> >>
> >
> >Sorry, I was not aware of this custom.?? I thought that if someone
> >reviewed the code then that was an indication that it should be noted. I
> >am aware of the more restrictive Acked-by which is normally sent by the
> >"acker" to the submitter.?? I will be more mindful of this in the
> >future.?? (This is my first posting to any of the GNU mail lists).

No problem.

> Reviewed-by actually is stronger then Acked-by at least that is
> what I believe, I've always interpreted these 2 as:
>
> Acked-by: foo:     Foo took a quick glance at the proposed changes and
> thinks this is a sensible change
> Reviewed-by: foo:  Foo did a detailed review of the code and did not find
> any issues

I think that you can take look at Documentation/process/submitting-patches.rst
in Linux kernel source. Most of us more or less adhere to this. Well, I think
that I should put something similar into GRUB2 tree.

Daniel

Re: [PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Daniel Kiper]

On Fri, Mar 23, 2018 at 02:11:33PM -0500, Mike Travis wrote:
> A GPU inserted into a PCIe I/O slot disappears during system startup.
> The problem centers around GRUB and a specific VGA init function in
> efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
> memory has not been enabled before attempting access.
>
> The fix is to add the same coding used in other VGA drivers, specifically
> to add a check to insure that it is indeed a VGA controller.  And then
> enable the MMIO address space with the specific bits.
>
> Signed-off-by: Mike Travis <mike.travis@hpe.com>
> Reviewed-by: Michael Chang <mchang@suse.com>
> ---
>  grub-2.02/grub-core/video/efi_uga.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> Index: grub-2.02/grub-core/video/efi_uga.c
> ===================================================================
> --- grub-2.02/grub-core/video/efi_uga.c.orig
> +++ grub-2.02/grub-core/video/efi_uga.c
> @@ -95,9 +95,19 @@ find_card (grub_pci_device_t dev, grub_p
>  {
>    struct find_framebuf_ctx *ctx = data;
>    grub_pci_address_t addr;
> +  grub_uint32_t class;

s/class/subclass/

>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
> -  if (grub_pci_read (addr) >> 24 == 0x3)
> +  class = grub_pci_read (addr);

subclass = (grub_pci_read (addr) >> 16) & 0xffff;

> +  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA)

if (subclass != GRUB_PCI_CLASS_SUBCLASS_VGA)

> +    return 0;
> +
> +  /* Enable MEM address spaces */
> +  {
> +    grub_pci_address_t rcaddr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +    grub_pci_write_word (rcaddr, grub_pci_read_word (rcaddr) | GRUB_PCI_COMMAND_MEM_ENABLED);
> +  }

Please drop these curly brackets.

Daniel

[PATCH 1/1] Fix PCIe LER when GRUB2 accesses non-enabled MMIO data from VGA

[Mike Travis]

[-- Attachment #1: skip-non-vga-devices --]
[-- Type: text/plain, Size: 1498 bytes --]

A GPU inserted into a PCIe I/O slot disappears during system startup.
The problem centers around GRUB and a specific VGA init function in
efi_uga.c.  This causes an LER (link error recorvery) because the MMIO
memory has not been enabled before attempting access.

The fix is to add the same coding used in other VGA drivers, specifically
to add a check to insure that it is indeed a VGA controller.  And then
enable the MMIO address space with the specific bits.

Signed-off-by: Mike Travis <mike.travis@hpe.com>
Reviewed-by: Michael Chang <mchang@suse.com>
---
 grub-2.02/grub-core/video/efi_uga.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Index: grub-2.02/grub-core/video/efi_uga.c
===================================================================
--- grub-2.02/grub-core/video/efi_uga.c.orig
+++ grub-2.02/grub-core/video/efi_uga.c
@@ -95,9 +95,19 @@ find_card (grub_pci_device_t dev, grub_p
 {
   struct find_framebuf_ctx *ctx = data;
   grub_pci_address_t addr;
+  grub_uint32_t class;
 
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-  if (grub_pci_read (addr) >> 24 == 0x3)
+  class = grub_pci_read (addr);
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA)
+    return 0;
+
+  /* Enable MEM address spaces */
+  {
+    grub_pci_address_t rcaddr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+    grub_pci_write_word (rcaddr, grub_pci_read_word (rcaddr) | GRUB_PCI_COMMAND_MEM_ENABLED);
+  }
+
     {
       int i;
 

--