Re: [PATCH V6 1/7] ACPI: I/O Remapping Table (IORT) initial support

[Lorenzo Pieralisi]

On Wed, Jun 15, 2016 at 09:46:29AM -0400, Sinan Kaya wrote:
> On 6/15/2016 9:34 AM, Lorenzo Pieralisi wrote:
> > On Wed, Jun 15, 2016 at 09:19:54AM -0400, Sinan Kaya wrote:
> >> Hi Tomasz,
> >>
> >>> +static acpi_status
> >>> +iort_match_node_callback(struct acpi_iort_node *node, void *context)
> >>> +{
> >>> +	case ACPI_IORT_NODE_PCI_ROOT_COMPLEX: {
> >>> +		struct acpi_iort_root_complex *pci_rc;
> >>> +		struct pci_bus *bus;
> >>> +
> >>> +		bus = to_pci_bus(dev);
> >>> +		pci_rc = (struct acpi_iort_root_complex *)node->node_data;
> >>> +
> >>> +		/*
> >>> +		 * It is assumed that PCI segment numbers maps one-to-one
> >>> +		 * with root complexes. Each segment number can represent only
> >>> +		 * one root complex.
> >>> +		 */
> >>> +		if (pci_rc->pci_segment_number == pci_domain_nr(bus))
> >>> +			return AE_OK;
> >>> +
> >>
> >> There is problem with the find_dev_node and callback for PCIe here. It assumes
> >> a one-to-one relationship between an SMMU and root complex. 
> >>
> >> Just checked with Charles offline to see if there is anything in the IORT spec that forces
> >> this. And, the answer was no. 
> >>
> >> Pasting the IORT requirements for you below.
> >>
> >> ?The IORT was intended to be flexible enough to define static RID to SID mappings, which should cover 
> >> the following configurations:
> >> -	Dedicated SMMU per RC
> >> -	Multiple RC?s per SMMU (as you described)
> >> -	Multiple SMMU?s per RC (with static RID:SID range per SMMU)
> >>
> >> The SMMU instance must be identified by either a device ID *or* a combination of 
> >> segment ID *and* Requestor ID. ?
> >>
> >> If a root complex has multiple SMMUs, this code is going to return the
> >> first SMMU. This needs to be corrected.
> > 
> > What you say above is correct, but the problem is not here. This
> > callback returns either a named component IORT node or a root complex
> > IORT node corresponding to a device, the problem you are referring to is
> > related to detecting which SMMU a given named component or root
> > complex refers too, which is not done here, I will take care of that
> > on my SMMU series.
> > 
> > When we look for the SMMU a PCI device is connected to, we must first
> > retrieve the IORT node of its root complex and walk its list of
> > mappings and match through RID range instead of picking the first
> > one, as I assumed, wrongly.
> > 
> > Lorenzo
> > 
> 
> Thanks for posting. I was trying to be more explicit by a follow up
> email.  You sent before me. 
> 
> The summary is that iort_find_dev_node function below will locate the
> wrong IORT root complex node in a multiple root port inside the same
> root complex configuration. 
> 
> I wish I could share the picture Harb drew here. Let me put it in text.
> 
> You can have a use case where you have two root ports in a single root complex.
> 
> Each root port has its own SMMU. Root ports are described in the MCFG
> table and in the DSDT table as root bridge with their respective bus
> start and end addresses. They both participate in the same root
> complex with the same segment number.
> 
> First root port requester id range (0x0-0x3ff) and second root port
> requester id range (0x400-0x7ff).

Ok, so why a single IORT node root complex entry with multiple node
mappings (with different RID ranges AND SMMU output references)
would not do here ?

Sorry for being blunt but I would like to understand where the
problem is here.

> The IORT table has two root complex entries for each root port. The
> first entry describes the requester id range (0x0-0x3ff) and points to
> first smmu behind id.

I lost you here. Do you mean the IORT table has one root complex IORT
node with two node mappings ?

> The second entry also describes the id range (0x400-0x7ff) and points to second smmu id.
> 
> The iort_find_dev_node function tries to locate an IORT node for a given PCIe device id.
> 
> If the requester id is 0x400 and segment id is 0, then this function
> will stop searching as soon as it finds the first node with segment id
> 0 as it only uses the segment id as a qualifier. 

Well yes. The question is whether we should have two root complexes
IORT nodes with the same segment id or a single root complex IORT node
with multiple mappings.

If we have one PCI root complex IORT node with multiple node mappings,
where is the problem ?

Thanks !
Lorenzo

> 
> It will locate the PCIe root complex node with requester id range (0x0-0x3ff) and use the 
> wrong smmu to do the ITS device id mapping.
> 
> "The SMMU instance must be identified by either a device ID *or* a combination of 
>   segment ID *and* Requestor ID. ?"
> 
> 
> >>
> >>> +		break;
> >>> +	}
> >>> +	}
> >>> +
> >>> +	return AE_NOT_FOUND;
> >>> +}
> >>> +
> >>
> >>> +
> >>> +static struct acpi_iort_node *
> >>> +iort_find_dev_node(struct device *dev)
> >>> +{
> >>> +	struct pci_bus *pbus;
> >>> +
> >>> +	if (!dev_is_pci(dev))
> >>> +		return iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> >>> +				      iort_match_node_callback, dev);
> >>> +
> >>> +	/* Find a PCI root bus */
> >>> +	pbus = to_pci_dev(dev)->bus;
> >>> +	while (!pci_is_root_bus(pbus))
> >>> +		pbus = pbus->parent;
> >>> +
> >>> +	return iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> >>> +			      iort_match_node_callback, &pbus->dev);
> >>> +}
> >>> +
> >>
> >> -- 
> >> Sinan Kaya
> >> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> >>
> 
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>