Change in coreboot[master]: device/pci: Don't enabled unassigned resources

[Nico Huber (Code Review)]

Hello Furquan Shaikh, Arthur Heymans, Kyösti Mälkki, Aaron Durbin,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/41553

to review the following change.

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

device/pci: Don't enabled unassigned resources

We only have a single bit in the PCI_COMMAND register to enable
or disable resources of one type. If any resource of a type is
unassigned, we have to disable all of them.

Change-Id: I7a7e9c5c382358446b60a7bd5b29954f80cce07e
Signed-off-by: Nico Huber 
---
M src/device/pci_device.c
1 file changed, 12 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/41553/1

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 5afbfa9..8df44df 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -506,7 +506,8 @@
    }
 }

-static void pci_set_resource(struct device *dev, struct resource *resource)
+static void pci_set_resource(struct device *const dev, struct resource *const resource,
+			     uint16_t *const command_mask)
 {
    /* Make certain the resource has actually been assigned a value. */
    if (!(resource->flags & IORESOURCE_ASSIGNED)) {
@@ -518,6 +519,11 @@
    		printk(BIOS_ERR, "ERROR: %s %02lx %s size: 0x%010llx not "
    		       "assigned\n", dev_path(dev), resource->index,
    		       resource_type(resource), resource->size);
+			/* We'll have to disable all resources of this type. */
+			if (resource->flags & IORESOURCE_MEM)
+				*command_mask &= ~PCI_COMMAND_MEMORY;
+			if (resource->flags & IORESOURCE_IO)
+				*command_mask &= ~PCI_COMMAND_IO;
    		return;
    	}
    }
@@ -561,12 +567,16 @@

void pci_dev_set_resources(struct device *dev)
 {
+	uint16_t command_mask = 0xffff;
    struct resource *res;
    struct bus *bus;
    u8 line;

for (res = dev->resource_list; res; res = res->next)
-		pci_set_resource(dev, res);
+		pci_set_resource(dev, res, &command_mask);
+	/* If there are unassigned resources, we might
+	   have to disable others of the same type. */
+	dev->command &= command_mask;

for (bus = dev->link_list; bus; bus = bus->next) {
    	if (bus->children)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@575 
PS1, Line 575: for (res = dev->resource_list; res; res = res->next)
             : 		pci_set_resource(dev, res, &command_mask);
             : 	/* If there are unassigned resources, we might
             : 	   have to disable others of the same type. */
             : 	dev->command &= command_mask;
             : 
I am thinking if we should just split bridge and non-bridge resource setting completely. That will also allow us to keep the entire logic in one place for each type:

void pci_dev_set_bridge_resources(void)
{
    for (res = dev->resource_list; res; res = res->next) {
          if (!(res->flags & IORESOURCE_PCI_BRIDGE))
              continue;

if (!resource_needs_storing(res))
              continue;

if (!(res->flags & IORESOURCE_ASSIGNED))
              res->size = 0;

if (res->size == 0) {
               base = res->limit;
               end = res->limit - (1 << res->gran);
               res->base = base;
          } else {
               base = res->base; 
               end = resource_end(res);
          }

dev->command |= PCI_COMMAND_MASTER;
          pci_dev_write_bridge_resource(dev, res->index, base, limit);
    }
}

void pci_dev_set_nonbridge_resources_by_type(unsigned long type_match, uint32_t command_mask, uint32_t bar_mask)
{
    for (res = dev->resource_list; res; res = res->next) {
          if ((res->flags & type_match) != type_match)
              continue;

if (!resource_needs_storing(res))
               continue;

if (!(res->flags & IORESOURCE_ASSIGNED) || !res->size) {
                printk(BIOS_ERR, "ERROR: %s %02lx %s size: 0x%010llx not "
    		       "assigned\n", dev_path(dev), res->index,
    		       resource_type(res), res->size);
                dev->command &= ~command_mask;
                return;
           }
           dev->command |= command_mask;
           pci_dev_write_nonbridge_resource(dev, res, bar_mask);
    }
}

static void pci_dev_set_nonbridge_resources(void)
{
    pci_dev_set_nonbridge_resources_by_type(IORESOURCE_IO, PCI_COMMAND_IO, PCI_BASE_ADDRESS_SPACE_IO);
    pci_dev_set_nonbridge_resources_by_type(IORESOURCE_MEM, PCI_COMMAND_MEM, 0);
}

pci_dev_set_resources(struct device *dev)
{
    pci_dev_set_bridge_resources();
    pci_dev_set_nonbridge_resources();

...
}

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41553/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41553/1//COMMIT_MSG@7 
PS1, Line 7: enabled
enable

https://review.coreboot.org/c/coreboot/+/41553/1//COMMIT_MSG@12 
PS1, Line 12: 
Does this fix an error, or is it for correctness?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@575 
PS1, Line 575: for (res = dev->resource_list; res; res = res->next)
             : 		pci_set_resource(dev, res, &command_mask);
             : 	/* If there are unassigned resources, we might
             : 	   have to disable others of the same type. */
             : 	dev->command &= command_mask;
             :
> I am thinking if we should just split bridge and non-bridge resource setting completely.

I thought about that too, but the bridge and non-bridge resources share
the command register bits. That makes it more delicate so I went for an
less invasive change. Basically we have 4 cases:

* No resources => don't set command bit
* Any non-bridge resource is invalid => don't set command bit
* All resources are valid => set command bit
* All non-bridge resources are valid, but a bridge one is not => we can disable bridge resources by programming limit < base, and set command bit

So we need to keep track of all the good and all the bad cases and will
have to decide in the end. With this change, `dev->command` would gather
the good and `command_mask` the bad cases.

I'll have a look into the split. If we also factor set_resource_command_bits()
out, we could get away without redundancy.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@575 
PS1, Line 575: for (res = dev->resource_list; res; res = res->next)
             : 		pci_set_resource(dev, res, &command_mask);
             : 	/* If there are unassigned resources, we might
             : 	   have to disable others of the same type. */
             : 	dev->command &= command_mask;
             :
> > I am thinking if we should just split bridge and non-bridge resource setting completely. […]

Agreed. We need to be careful about the command bits. Having a helper function get_command_bits(res) can return the right mask to set / clear.

1) Bridge --> a) Always set the mask using get_command_bits(res)
2) Non-bridge --> Get mask using get_command_bits(res), a) Set if resource is valid, b) Clear if resource is invalid and exit.

Hence, the order above is important where bridge resources are considered first and then the non-bridge resources.

With that the 4 cases would be:
* No resources => don't set command bit ==> Never sets any mask for the resource type
* Any non-bridge resource is invalid => don't set command bit ==> 2b
* All resources are valid => set command bit ==> 2a
* All non-bridge resources are valid, but a bridge one is not => we can disable bridge resources by programming limit < base, and set command bit ==> 1a + 2a

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

(2 comments)

This change is ready for review.

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@517 
PS1, Line 517: 			resource->size = 0;
can we just call pci_store_bridge_resource() here and return?

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@558 
PS1, Line 558: 	resource->flags |= IORESOURCE_STORED;
Shouldn't we set this in the respective functions (pci_store_bridge_resource() and pci_store_resource())  as well as the accompanying report_resource_stored()?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@517 
PS1, Line 517: 			resource->size = 0;
> can we just call pci_store_bridge_resource() here and return?

Hmmm, yes, if we'd move the ASSIGNED check below all the other ones. In
other words, the only thing we want to skip is the COMMAND bits, right?

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@558 
PS1, Line 558: 	resource->flags |= IORESOURCE_STORED;
> Shouldn't we set this in the respective functions (pci_store_bridge_resource() and pci_store_resourc […]

Uff, no feelings here. IIRC, report_resource_stored() actually checks
IORESOURCE_STORED by itself. So it expects to be always called, which
made it weird enough to me to not change the flow ;)

https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@575 
PS1, Line 575: for (res = dev->resource_list; res; res = res->next)
             : 		pci_set_resource(dev, res, &command_mask);
             : 	/* If there are unassigned resources, we might
             : 	   have to disable others of the same type. */
             : 	dev->command &= command_mask;
             :
> > We have to gather information from all resource first and then make the final decision
>
> Bridge resources never result in clearing of command bits. Thus, if non-bridge resources are considered after bridge resources, you can rely on the state provided by non-bridge resource. Also, we don't have to set the command bit in the loop and instead we can use a flag in pci_dev_set_nonbridge_resources_by_type() to decide if command bit should be set.

Not sure where we are talking past each other. I'll give an example of what
we might have to expect from a random PCI device:

{
            {
                    .base = 0,
                    .size = 0x10000,
                    .flags = IORESOURCE_MEM,
                    .index = 0x10,
            },
            {
                    .base = 0xc32fc000,
                    .size = 0x4000,
                    .flags = IORESOURCE_ASSIGNED | IORESOURCE_MEM,
                    .index = 0x14,
            },
    }

This can happen. A PCI device can have multiple resources of the same type
and if a later one is smaller, the allocator might have found space for it
even if it failed for an earlier, bigger resource. But in the PCI case, we
must either have all resources of a type assigned or not set the COMMAND
bit.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Patch Set 1:

Aaaron, Furquan, it’d be great, if you could respond to Nico’s comments (answers and questions).

[S] Change in coreboot[master]: device/pci: Don't enabled unassigned resources

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41553?usp=email )

Change subject: device/pci: Don't enabled unassigned resources
......................................................................

Abandoned