Re: [PATCH v3 2/9] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Dan Williams <dan.j.williams@intel.com>]

Robert Richter wrote:
> On 17.11.22 09:20:55, Dan Williams wrote:
> > Robert Richter wrote:
> > > On 15.11.22 09:54:16, Dan Williams wrote:
> > > > Robert Richter wrote:
> > > > > On 14.11.22 13:30:01, Dan Williams wrote:
> > > > > > Robert Richter wrote:
> > > > > 
> > > > > > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > > > > > > index ec178e69b18f..7a5bde81e949 100644
> > > > > > > --- a/drivers/cxl/core/regs.c
> > > > > > > +++ b/drivers/cxl/core/regs.c
> > > > > > > @@ -307,3 +307,49 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> > > > > > >  	return -ENODEV;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> > > > > > > +
> > > > > > > +resource_size_t cxl_rcrb_to_component(struct device *dev,
> > > > > > > +				      resource_size_t rcrb,
> > > > > > > +				      enum cxl_rcrb which)
> > > > > > > +{
> > > > > > > +	resource_size_t component_reg_phys;
> > > > > > > +	u32 bar0, bar1;
> > > > > > > +	void *addr;
> > > > > > > +
> > > > > > > +	if (which == CXL_RCRB_UPSTREAM)
> > > > > > > +		rcrb += SZ_4K;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * RCRB's BAR[0..1] point to component block containing CXL
> > > > > > > +	 * subsystem component registers. MEMBAR extraction follows
> > > > > > > +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> > > > > > > +	 * ranges alignment (6.0, 7.5.1.2.1).
> > > > > > > +	 */
> > > > > > 
> > > > > > A request_mem_region() is needed here to ensure ownership and expected
> > > > > > sequencing of accessing the RCRB to locate the component registers, and
> > > > > > accessing the RCRB to manipulate the component registers. It also helps
> > > > > > to sanity check that the BIOS mapped an exclusive range for the RCRB.
> > > > > 
> > > > > Right, that is missing.
> > > > > 
> > > > > > 
> > > > > > > +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);
> > > > > > 
> > > > > > That PCI_BASE_ADDRESS_0 does not belong there. It ends up being benign
> > > > > > and forcing ioremap to map 12K instead of 8K, but it is a
> > > > > > config-register offset, not part of the RCRB size.
> > > > > 
> > > > > Note this is BAR0 + 8 bytes, not 8k, and it does not map the whole
> > > > > RCRB region but instead the first part of the config space up to
> > > > > including the 64 bit BAR.
> > > > 
> > > > Oh, sorry, yes, my mistake. However, there is not much value in mapping
> > > > less than 4K since all ioremap requests are rounded up to PAGE_SIZE.
> > > > Since an RCRB is only 4K per port lets just map the whole thing.
> > > 
> > > I was going to keep the ranges small to avoid conflicts with other
> > > requests for the same page (though request_mem_region() was missing
> > > yet).
> > 
> > What else will be conflicting the RCRB? Linux has never accessed an RCRB
> > in the past as far as I can see. If there is a conflict then we may need
> > to move this mapping to the PCI core so that it is managed like other
> > mmconf space.
> 
> The capabilities (PCIe and DVSEC) could be used by various subsystems
> and parts of the driver. I am thinking of the various RAS caps (UP,
> DP, CXL, AER variants) that are accessed from different parts of the
> driver. Of curse, access could be delegated but else there is the
> option to directly map and access that parts. In the component reg
> block we already see issues with that broad mappings.

Sure, but lets cross that bridge when we get to that point. Something is
broken if these competing usages can not at least have their own page
mapping since that limits being able to hand out control across security
boundaries (like VMs or userspace). Any ioremap less than PAGE_SIZE is
somewhat suspect.

The cxl_port driver so far seems to be sufficient for owning the entire
component register space.