vgaarb, and bridges not capable of VGA forwarding

vgaarb, and bridges not capable of VGA forwarding

[Simon Richter]

[-- Attachment #1.1: Type: text/plain, Size: 2541 bytes --]

Hi,

(cc Steve Wahl because UV BIOS is tangentially involved)
(cc Ville Syrjala because of the recent thread about VGA arbiter)

I'm looking into

     https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1824

again because I have a similar problem.

In pci_set_vga_state, we traverse the PCI bridges upwards, and set the 
VGA forwarding bit on all of them up to the root. Now I happen to have a 
root bridge that refuses to set this bit, so if I read it back after 
setting it, it is still unset.

TTBOMK, that is allowed in the PCI specification, and the correct way to 
indicate that a bridge cannot forward VGA accesses.

The smallest possible change is to read back the bridge control 
register, and if the bit is still unset, terminate the loop and return 
an error.

I'm trying to find out now if this is a good idea, and am a bit puzzled:

It appears the only place that can generate errors is 
pci_set_vga_state_arch, which is a dispatch mechanism with a single 
user, uv_bios_set_legacy_vga_target.

According to the comment, the errors generated here are -EINVAL, -ENOSYS 
and -EBUSY. The implementation returns the return value from an EFI 
call, which appears to be defined to be a negated POSIX errno number, so 
this has a high chance of being correct.

If pci_set_vga_state_arch does not return an error, the normal 
pci_set_vga_state function runs, and there is no way for the arch 
specific function to indicate that the rest of the function should not 
run. Is that intentional, or should the BIOS call replace the normal 
implementation?

It also seems that vgaarb is the only caller, and it does not look at 
the return value from this function. So if I fix that, and propagate the 
error upwards, I first need a way to indicate that __try_vga_get failed 
without there being a conflict, and then I need all vga_get callers to 
have error handling. The latter sounds doable.

What I'm unsure about:

1. Does this seem like a viable approach?

2. What interface makes sense for returning an error here? This function 
is supposed to return the conflicting device -- should I just make this 
a PTR_ERR?

3. What error would be appropriate for a bridge refusing to activate the 
VGA bit? It's not EIO, it's not EACCES, it's not EINVAL, the closest is 
probably ENOSYS, but even that feels wrong.

4. How likely is it for this to break something else? Are there PCI 
bridges that forward VGA but implement the bridge control register 
incorrectly?

    Simon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: vgaarb, and bridges not capable of VGA forwarding

[Ville Syrjälä]

On Thu, Dec 11, 2025 at 11:43:13PM +0900, Simon Richter wrote:
> Hi,
> 
> (cc Steve Wahl because UV BIOS is tangentially involved)
> (cc Ville Syrjala because of the recent thread about VGA arbiter)
> 
> I'm looking into
> 
>      https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1824
> 
> again because I have a similar problem.
> 
> In pci_set_vga_state, we traverse the PCI bridges upwards, and set the 
> VGA forwarding bit on all of them up to the root. Now I happen to have a 
> root bridge that refuses to set this bit, so if I read it back after 
> setting it, it is still unset.
> 
> TTBOMK, that is allowed in the PCI specification, and the correct way to 
> indicate that a bridge cannot forward VGA accesses.
> 
> The smallest possible change is to read back the bridge control 
> register, and if the bit is still unset, terminate the loop and return 
> an error.
> 
> I'm trying to find out now if this is a good idea, and am a bit puzzled:
> 
> It appears the only place that can generate errors is 
> pci_set_vga_state_arch, which is a dispatch mechanism with a single 
> user, uv_bios_set_legacy_vga_target.
> 
> According to the comment, the errors generated here are -EINVAL, -ENOSYS 
> and -EBUSY. The implementation returns the return value from an EFI 
> call, which appears to be defined to be a negated POSIX errno number, so 
> this has a high chance of being correct.
> 
> If pci_set_vga_state_arch does not return an error, the normal 
> pci_set_vga_state function runs, and there is no way for the arch 
> specific function to indicate that the rest of the function should not 
> run. Is that intentional, or should the BIOS call replace the normal 
> implementation?
> 
> It also seems that vgaarb is the only caller, and it does not look at 
> the return value from this function. So if I fix that, and propagate the 
> error upwards, I first need a way to indicate that __try_vga_get failed 
> without there being a conflict, and then I need all vga_get callers to 
> have error handling. The latter sounds doable.
> 
> What I'm unsure about:
> 
> 1. Does this seem like a viable approach?
> 
> 2. What interface makes sense for returning an error here? This function 
> is supposed to return the conflicting device -- should I just make this 
> a PTR_ERR?
> 
> 3. What error would be appropriate for a bridge refusing to activate the 
> VGA bit? It's not EIO, it's not EACCES, it's not EINVAL, the closest is 
> probably ENOSYS, but even that feels wrong.
> 
> 4. How likely is it for this to break something else? Are there PCI 
> bridges that forward VGA but implement the bridge control register 
> incorrectly?
> 
>     Simon

I wonder how bad it would be to just tickle the VGA bit
when the bridge is added. Basically something like:

if (BRIDGECTL.VGA) {
	bridge.has_vga = true;
} else {
	BRIDGECTL.VGA = 1
	if (BRIDGECTL.VGA)
		bridge.has_vga = true;
	BRIDGECTL.VGA = 0
}

Obviously there is a tiny race there where the bridge might
temporarily forward VGA accesses to the wrong device. But
maybe the race is small enough to not really matter? Or I
guess if you're really worried about the race you could do
this check under stop_machine() *shudder*.

BTW I recently posted this:
https://lore.kernel.org/intel-gfx/20251208182637.334-1-ville.syrjala@linux.intel.com/
where I'm trying to fix the mess around VGA accesses in i915/xe.
Some of the more hacky things there should probably be properly
fixed in vgaarb, but I don't really have the time/energy to
deal with that right now.

-- 
Ville Syrjälä
Intel

Re: vgaarb, and bridges not capable of VGA forwarding

[Steve Wahl]

Hi,

I somehow missed the first message, but saw Ville's reply.  (HPE email
problem, possibly?  Corporate is finicky about some email things.)

I'm placing the one question I think I can answer inline below:

On Fri, Dec 12, 2025 at 01:34:53PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 11, 2025 at 11:43:13PM +0900, Simon Richter wrote:
> > Hi,
> > 
> > (cc Steve Wahl because UV BIOS is tangentially involved)
> > (cc Ville Syrjala because of the recent thread about VGA arbiter)
> > 
> > I'm looking into
> > 
> >      https://urldefense.com/v3/__https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1824__;!!NpxR!gfuCvZx88l1D_2aox3_me1EFGcTVvba2mVU-c-8rLBWcQ2FLmMWTl4h8c7zasCBM-sAYW9EuoMXEyyVeedabAZEq2cQ$ 
> > 
> > again because I have a similar problem.
> > 
> > In pci_set_vga_state, we traverse the PCI bridges upwards, and set the 
> > VGA forwarding bit on all of them up to the root. Now I happen to have a 
> > root bridge that refuses to set this bit, so if I read it back after 
> > setting it, it is still unset.
> > 
> > TTBOMK, that is allowed in the PCI specification, and the correct way to 
> > indicate that a bridge cannot forward VGA accesses.
> > 
> > The smallest possible change is to read back the bridge control 
> > register, and if the bit is still unset, terminate the loop and return 
> > an error.
> > 
> > I'm trying to find out now if this is a good idea, and am a bit puzzled:
> > 
> > It appears the only place that can generate errors is 
> > pci_set_vga_state_arch, which is a dispatch mechanism with a single 
> > user, uv_bios_set_legacy_vga_target.
> > 
> > According to the comment, the errors generated here are -EINVAL, -ENOSYS 
> > and -EBUSY. The implementation returns the return value from an EFI 
> > call, which appears to be defined to be a negated POSIX errno number, so 
> > this has a high chance of being correct.
> > 
> > If pci_set_vga_state_arch does not return an error, the normal 
> > pci_set_vga_state function runs, and there is no way for the arch 
> > specific function to indicate that the rest of the function should not 
> > run. Is that intentional, or should the BIOS call replace the normal 
> > implementation?

Unfortunately, I have very little experience with VGA usage on our UV
platform, the majority of our kernel development work is done using a
serial console setup, so I don't know if I've even seen this function
exercised.  System test and customers may be different, however.

But it does seem obvious that for the UV platform, this BIOS call is
intended to be called in addition to, not instead of, the normal
implementation.  The original checkin comment for the uv_set_vga_state
comment:

    x86, uv: Update UV arch to target Legacy VGA I/O correctly.
    
    Add function to direct Legacy VGA I/O traffic to correct I/O Hub.
    
Most members of the UV platform have external node controllers (XNC,
aka Hubs) that essentially expand the number of sockets connected via
UPI.  So I expect that under these conditions we need the BIOS to set
up some routing in the Hubs, in addition to what the existing kernel
code does.

I think others on this list are probably better equipped to handle
the rest of the questions.

Thanks,

--> Steve Wahl, HPE

> > It also seems that vgaarb is the only caller, and it does not look at 
> > the return value from this function. So if I fix that, and propagate the 
> > error upwards, I first need a way to indicate that __try_vga_get failed 
> > without there being a conflict, and then I need all vga_get callers to 
> > have error handling. The latter sounds doable.
> > 
> > What I'm unsure about:
> > 
> > 1. Does this seem like a viable approach?
> > 
> > 2. What interface makes sense for returning an error here? This function 
> > is supposed to return the conflicting device -- should I just make this 
> > a PTR_ERR?
> > 
> > 3. What error would be appropriate for a bridge refusing to activate the 
> > VGA bit? It's not EIO, it's not EACCES, it's not EINVAL, the closest is 
> > probably ENOSYS, but even that feels wrong.
> > 
> > 4. How likely is it for this to break something else? Are there PCI 
> > bridges that forward VGA but implement the bridge control register 
> > incorrectly?
> > 
> >     Simon
> 
> I wonder how bad it would be to just tickle the VGA bit
> when the bridge is added. Basically something like:
> 
> if (BRIDGECTL.VGA) {
> 	bridge.has_vga = true;
> } else {
> 	BRIDGECTL.VGA = 1
> 	if (BRIDGECTL.VGA)
> 		bridge.has_vga = true;
> 	BRIDGECTL.VGA = 0
> }
> 
> Obviously there is a tiny race there where the bridge might
> temporarily forward VGA accesses to the wrong device. But
> maybe the race is small enough to not really matter? Or I
> guess if you're really worried about the race you could do
> this check under stop_machine() *shudder*.
> 
> BTW I recently posted this:
> https://lore.kernel.org/intel-gfx/20251208182637.334-1-ville.syrjala@linux.intel.com/ 
> where I'm trying to fix the mess around VGA accesses in i915/xe.
> Some of the more hacky things there should probably be properly
> fixed in vgaarb, but I don't really have the time/energy to
> deal with that right now.
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Steve Wahl, Hewlett Packard Enterprise