diff mbox series

[v7,16/27] cxl/pci: Store the endpoint's Component Register mappings in struct cxl_dev_state

Message ID 20230622205523.85375-17-terry.bowman@amd.com
State Superseded
Headers show
Series cxl/pci: Add support for RCH RAS error handling | expand

Commit Message

Bowman, Terry June 22, 2023, 8:55 p.m. UTC
From: Robert Richter <rrichter@amd.com>

Same as for ports and dports, also store the endpoint's Component
Register mappings, use struct cxl_dev_state for that.

The Component Register base address @component_reg_phys is no longer
used after the rework of the Component Register setup which now uses
struct member @comp_map instead. Remove the base address.

Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/cxl/cxlmem.h         |  5 ++---
 drivers/cxl/mem.c            |  4 ++--
 drivers/cxl/pci.c            | 10 ++++------
 tools/testing/cxl/test/mem.c |  1 -
 4 files changed, 8 insertions(+), 12 deletions(-)

Comments

Dan Williams June 25, 2023, 5:38 p.m. UTC | #1
Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> Same as for ports and dports, also store the endpoint's Component
> Register mappings, use struct cxl_dev_state for that.
> 
> The Component Register base address @component_reg_phys is no longer
> used after the rework of the Component Register setup which now uses
> struct member @comp_map instead. Remove the base address.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Starting with this patch my QEMU cxl configuration fails to probe the
device:

# cxl list -Miu
{
  "memdev":"mem0",
  "pmem_size":"512.00 MiB (536.87 MB)",
  "serial":"0",
  "host":"0000:35:00.0",
  "state":"disabled"
}

...the next patch changes where the failure occurs, but it still fails
and applying the rest of the patches does not fix the issue either. I
have not had the time to debug it. Please have a look and make sure that
at every step of this patch set the driver is still operational. I.e.
'git bisect' should never find this conversion in a broken state.

For now I will look at proceeding with patch 1-15.

The cxl bits of my QEMU config are:

  -object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxl-window1,size=$cxl_backend_size
  -object memory-backend-file,id=cxl-label1,share=on,mem-path=cxl-label1,size=$cxl_label_size
  -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52
  -device cxl-rp,id=rp0,bus=cxl.0,addr=0.0,chassis=0,slot=0,port=0
  -device cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,lsa=cxl-label1
  -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G
Bowman, Terry June 26, 2023, 2:16 p.m. UTC | #2
Hi Dan,

On 6/25/23 12:38, Dan Williams wrote:
> Terry Bowman wrote:
>> From: Robert Richter <rrichter@amd.com>
>>
>> Same as for ports and dports, also store the endpoint's Component
>> Register mappings, use struct cxl_dev_state for that.
>>
>> The Component Register base address @component_reg_phys is no longer
>> used after the rework of the Component Register setup which now uses
>> struct member @comp_map instead. Remove the base address.
>>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Starting with this patch my QEMU cxl configuration fails to probe the
> device:
> 
> # cxl list -Miu
> {
>   "memdev":"mem0",
>   "pmem_size":"512.00 MiB (536.87 MB)",
>   "serial":"0",
>   "host":"0000:35:00.0",
>   "state":"disabled"
> }
> 
> ...the next patch changes where the failure occurs, but it still fails
> and applying the rest of the patches does not fix the issue either. I
> have not had the time to debug it. Please have a look and make sure that
> at every step of this patch set the driver is still operational. I.e.
> 'git bisect' should never find this conversion in a broken state.
> 

Yes, we will look into this issue now. We will verify each patch 
builds and runs without degrading. Can you share which Qemu release/branch
to test against?

> For now I will look at proceeding with patch 1-15.
> 

Ok

Regards,
Terry

> The cxl bits of my QEMU config are:
> 
>   -object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxl-window1,size=$cxl_backend_size
>   -object memory-backend-file,id=cxl-label1,share=on,mem-path=cxl-label1,size=$cxl_label_size
>   -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52
>   -device cxl-rp,id=rp0,bus=cxl.0,addr=0.0,chassis=0,slot=0,port=0
>   -device cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,lsa=cxl-label1
>   -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G
Robert Richter June 30, 2023, 7:56 p.m. UTC | #3
(resending without logs attached to make vger.kernel.org happy)

Dan, Jonathan,

On 25.06.23 10:38:26, Dan Williams wrote:
> Terry Bowman wrote:
> > From: Robert Richter <rrichter@amd.com>
> > 
> > Same as for ports and dports, also store the endpoint's Component
> > Register mappings, use struct cxl_dev_state for that.
> > 
> > The Component Register base address @component_reg_phys is no longer
> > used after the rework of the Component Register setup which now uses
> > struct member @comp_map instead. Remove the base address.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I could fix the HDM initialization failure. The check in "cxl/hdm: Use
stored Component Register mappings to map HDM decoder capability" was
too strict and did not return -ENODEV as the previous implementation.

We will send an updated version of the remaining patches.

However, while analyzing this I found the following:

> 
> Starting with this patch my QEMU cxl configuration fails to probe the
> device:
> 
> # cxl list -Miu
> {
>   "memdev":"mem0",
>   "pmem_size":"512.00 MiB (536.87 MB)",
>   "serial":"0",
>   "host":"0000:35:00.0",
>   "state":"disabled"
> }

I have looked into this and it looks to me that the qemu model is
wrong. I am referring esp. to those sections in the CXL 3.0 spec (Aug
1, 2022):

 Figure 9-14. CXL Link/Protocol Register Mapping in a CXL VH
 Figure 9-15. CXL Link/Protocol Registers in a CXL Switch
 Table 8-22. CXL_Capability_ID Assignment

In short, the CXL capabilities are wrongly assigned to the various
component registers (component abbr. taken from Table 8-22):

ACPI CHBCR (RC): 
 * includes RAS cap, misses HDM cap
 * expected: no RAS cap, includes HDM cap

RAS cap does not make sense here and is unused.

logs:

> [  468.239319] cxl port1: found RAS capability (0x80)
> [  468.240186] cxl port1: Unknown CM cap ID: 4 (0xd8)
> [  468.241083] cxl port1: Unknown CM cap ID: 0 (0x110)
> [  468.241974] cxl port1: Unknown CM cap ID: 6 (0x260)
> [  468.242876] cxl port1: Unknown CM cap ID: 8 (0xa84)

CXL root port (R):
 * includes RAS and HDM cap
 * expected: includes RAS cap, misses HDM cap

This causes the HDM for the root ports being not properly initialized,
kernel switching to fallback mode.

logs:

> [  468.250182] pcieport 0000:34:00.0: Mapped CXL Memory Device resource 0x00000000fea00000
> [  468.250258] pcieport 0000:34:00.0: found RAS capability (0x80)
> [  468.250371] pcieport 0000:34:00.0: Unknown CM cap ID: 4 (0xd8)
> [  468.250413] pcieport 0000:34:00.0: found HDM decoder capability (0x110)
> [  468.256022] pcieport 0000:34:00.0: Unknown CM cap ID: 6 (0x260)
> [  468.256058] pcieport 0000:34:00.0: Unknown CM cap ID: 8 (0xa84)

CXL 2.0 device (D2)
 * includes RAS and HDM cap
 * works as expected

logs:

> [ 1396.813184] cxl_pci 0000:35:00.0: found RAS capability (0x80)
> [ 1396.814133] cxl_pci 0000:35:00.0: Unknown CM cap ID: 4 (0xd8)
> [ 1396.815081] cxl_pci 0000:35:00.0: found HDM decoder capability (0x110)

No issues here.

Details below.

Kernel:

version: 0c0df63177e3 Merge branch 'for-6.5/cxl-rch-eh' into for-6.5/cxl
cmdline: 
[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-6.4.0-rc6-rric-00055-g0c0df63177e3 root=UUID=8b447531-798d-41cb-a3df-ae1d9bb1eaf9 ro debug ignore_loglevel "dyndbg=file drivers/cxl/* +p" systemd.log_target=journal

Qemu:

version: QEMU emulator version 8.0.50 (v6.2.0-12087-g62c0c95799)
cmdline:
/usr/local/bin/qemu-system-x86_64 -smp 6 -machine type=q35,accel=kvm,nvdimm=on,cxl=on -hda debian.img -m 4096 -enable-kvm -object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxl-window1,size=512M -object memory-backend-file,id=cxl-label1,share=on,mem-path=cxl-label1,size=512M -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 -device cxl-rp,id=rp0,bus=cxl.0,addr=0.0,chassis=0,slot=0,port=0 -device cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,lsa=cxl-label1 -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G -vnc :0 -nic none

Annotated qemu kernel boot log after loading the cxl_acpi module
(including deps):

> [  468.229977] cxl_acpi ACPI0017:00: root0 added (root port)

root0 added.

> [  468.231055] acpi ACPI0016:00: UID found: 52
> [  468.231959]  pci0000:34: dport added to root0

add_host_bridge_dport() found the CXL host bridge and added it as
dport to root0..

> [  468.232867] Unknown target node for memory at 0x190000000, assuming node 0
> [  468.233901] cxl_acpi ACPI0017:00: add: decoder0.0 node: 0 range [0x190000000 - 0x28fffffff]
> [  468.235394] acpi ACPI0016:00: UID found: 52
> [  468.236171] acpi ACPI0016:00: CHBCR found for UID 52: 0x0000000180000000
> [  468.237193]  pci0000:34: host-bridge: pci0000:34
> [  468.238132] cxl port1: Mapped CXL Memory Device resource 0x0000000180000000

CHBCR for the host bridge was found at 0x0000000180000000.

In this kernel version it is bound to port1 (not the dport), some
comments on this later.

Component registers and capabilities are extracted from the CHBCR:

> [  468.239319] cxl port1: found RAS capability (0x80)
> [  468.240186] cxl port1: Unknown CM cap ID: 4 (0xd8)
> [  468.241083] cxl port1: Unknown CM cap ID: 0 (0x110)
> [  468.241974] cxl port1: Unknown CM cap ID: 6 (0x260)
> [  468.242876] cxl port1: Unknown CM cap ID: 8 (0xa84)

The CHBCR does not contain the HDM registers for the root ports.

It (wrongly) contains the RAS caps.

I would expect HDM here instead.

> [  468.243767] cxl port1: Set up component registers
> [  468.244959]  pci0000:34: port1 added to root0

port1 added to root0 with the component regs from the CHBCR bound.

> [  468.245807]  pci0000:34: host supports CXL
> [  468.246711] cxl_acpi ACPI0017:00: root0: add: nvdimm-bridge0
> [  468.247792] CXL bus rescan result: 0

port 1 added to the cxl bus which triggers cxl_port_probe:

Running cxl_switch_port_probe() for port1 here:

> [  468.250149] pcieport 0000:34:00.0: Component Registers found for dport: 0x00000000fea00000

The CXL root port was found by devm_cxl_port_enumerate_dports().

Its component regs are extracted:

> [  468.250182] pcieport 0000:34:00.0: Mapped CXL Memory Device resource 0x00000000fea00000
> [  468.250258] pcieport 0000:34:00.0: found RAS capability (0x80)
> [  468.250371] pcieport 0000:34:00.0: Unknown CM cap ID: 4 (0xd8)
> [  468.250413] pcieport 0000:34:00.0: found HDM decoder capability (0x110)
> [  468.256022] pcieport 0000:34:00.0: Unknown CM cap ID: 6 (0x260)
> [  468.256058] pcieport 0000:34:00.0: Unknown CM cap ID: 8 (0xa84)

The CXL root port's comp regs contain the RAS caps but also (wrongly)
the HDM caps.

> [  468.256065] pcieport 0000:34:00.0: Set up component registers
> [  468.256089] pcieport 0000:34:00.0: dport added to port1

CXL root port added as dport to port1.

devm_cxl_setup_hdm() is run for port1:

> [  468.256157] cxl_port port1: found RAS capability (0x80)
> [  468.260800] cxl_port port1: Unknown CM cap ID: 4 (0xd8)
> [  468.260833] cxl_port port1: Unknown CM cap ID: 0 (0x110)
> [  468.260865] cxl_port port1: Unknown CM cap ID: 6 (0x260)
> [  468.260898] cxl_port port1: Unknown CM cap ID: 8 (0xa84)

port1's comp regs from above extracted again for HDM setup, but...

> [  468.260904] cxl_port port1: HDM decoder registers not implemented

... the HDM decoder could not be found. map_hdm_decoder_regs() returns
a -ENODEV.

> [  468.260915] cxl_port port1: Fallback to passthrough decoder

cxl_switch_port_probe() continues with
devm_cxl_add_passthrough_decoder() as there was only one dport found.

> [  468.261024] cxl decoder1.0: Added to port port1
> [  468.267186] cxl_port port1: probe: 0

Probe success.

> [  468.298485] nd_bus ndbus0: START: nd_bus.probe(ndbus0)
> [  468.299859] nd_bus ndbus0: END: nd_bus.probe(ndbus0) = 0
> [  468.300840] cxl_nvdimm_bridge nvdimm-bridge0: probe: 0

End of log. Full log attached.

Loading the cxl_pci driver later succeeds, cxl list shows:

[
  {
    "memdev":"mem0",
    "pmem_size":536870912,
    "serial":0,
    "host":"0000:35:00.0"
  }
]

I am not sure if the issue is a qemu config or implementation issue.

Please take a look. Am I missing something here?

Thanks,

-Robert
Jonathan Cameron July 3, 2023, 3:55 a.m. UTC | #4
On Fri, 30 Jun 2023 21:56:44 +0200
Robert Richter <rrichter@amd.com> wrote:

> (resending without logs attached to make vger.kernel.org happy)
> 
> Dan, Jonathan,
> 
> On 25.06.23 10:38:26, Dan Williams wrote:
> > Terry Bowman wrote:  
> > > From: Robert Richter <rrichter@amd.com>
> > > 
> > > Same as for ports and dports, also store the endpoint's Component
> > > Register mappings, use struct cxl_dev_state for that.
> > > 
> > > The Component Register base address @component_reg_phys is no longer
> > > used after the rework of the Component Register setup which now uses
> > > struct member @comp_map instead. Remove the base address.
> > > 
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> I could fix the HDM initialization failure. The check in "cxl/hdm: Use
> stored Component Register mappings to map HDM decoder capability" was
> too strict and did not return -ENODEV as the previous implementation.
> 
> We will send an updated version of the remaining patches.
> 
> However, while analyzing this I found the following:
> 
> > 
> > Starting with this patch my QEMU cxl configuration fails to probe the
> > device:
> > 
> > # cxl list -Miu
> > {
> >   "memdev":"mem0",
> >   "pmem_size":"512.00 MiB (536.87 MB)",
> >   "serial":"0",
> >   "host":"0000:35:00.0",
> >   "state":"disabled"
> > }  
> 
> I have looked into this and it looks to me that the qemu model is
> wrong. I am referring esp. to those sections in the CXL 3.0 spec (Aug
> 1, 2022):

Indeed looks dodgy.  Ira recently highlighted that this was a mess
and proposed some fixes.. (going way back, I moaned about the way this
was being done being inflexible to Ben but we never cleaned it up :(
https://lore.kernel.org/linux-cxl/20230517-rfc-type2-dev-v1-2-6eb2e470981b@intel.com/

We should change that function to explicitly handle ever device type
rather than trying to shortcut certain shared combinations.

> 
>  Figure 9-14. CXL Link/Protocol Register Mapping in a CXL VH
>  Figure 9-15. CXL Link/Protocol Registers in a CXL Switch
>  Table 8-22. CXL_Capability_ID Assignment
> 
> In short, the CXL capabilities are wrongly assigned to the various
> component registers (component abbr. taken from Table 8-22):
> 
> ACPI CHBCR (RC): 
>  * includes RAS cap, misses HDM cap
>  * expected: no RAS cap, includes HDM cap

To check, do you have multiple RP under the RC? From info below
I see you only have 1.  You can override by providing pxb-cxl
with hdm_for_passthrough=true

If not default is a pass through decoder so no HDM cap should be
present (they kernel used to assume that - so we changed default in
QEMU to match - I think the kernel is now fine either way)

QEMU code indeed looks dodgy as it's initializing the host bridge
as if it were a root port. (oops) and as you've noticed then
adds HDM decoders to the RP which also makes no sense (but are
ignored).

I've just hacked in use of null capability (which was only defined
in CXL3.0 but is fine for old devices) so that we can screen
out the things that shouldn't be there by skipping initializing them.
Also added an enum entry for CXL2_RC so we can handle that one
correctly rather than combining it with CXL2_ROOT_PORT (that predates
me so no idea why it was like that).



> 
> RAS cap does not make sense here and is unused.
> 
> logs:
> 
> > [  468.239319] cxl port1: found RAS capability (0x80)
> > [  468.240186] cxl port1: Unknown CM cap ID: 4 (0xd8)
> > [  468.241083] cxl port1: Unknown CM cap ID: 0 (0x110)
> > [  468.241974] cxl port1: Unknown CM cap ID: 6 (0x260)
> > [  468.242876] cxl port1: Unknown CM cap ID: 8 (0xa84)  
> 
> CXL root port (R):
>  * includes RAS and HDM cap
>  * expected: includes RAS cap, misses HDM cap
> 
> This causes the HDM for the root ports being not properly initialized,
> kernel switching to fallback mode.

This confuses me a bit.  Why is it trying to initialize something
that shouldn't be there in the root ports? (which whilst a bug 'might'
occur on real hardware if that was also buggy ;)

> 
> logs:
> 
> > [  468.250182] pcieport 0000:34:00.0: Mapped CXL Memory Device resource 0x00000000fea00000
> > [  468.250258] pcieport 0000:34:00.0: found RAS capability (0x80)
> > [  468.250371] pcieport 0000:34:00.0: Unknown CM cap ID: 4 (0xd8)
> > [  468.250413] pcieport 0000:34:00.0: found HDM decoder capability (0x110)
> > [  468.256022] pcieport 0000:34:00.0: Unknown CM cap ID: 6 (0x260)
> > [  468.256058] pcieport 0000:34:00.0: Unknown CM cap ID: 8 (0xa84)  
> 
> CXL 2.0 device (D2)
>  * includes RAS and HDM cap
>  * works as expected
> 
> logs:
> 
> > [ 1396.813184] cxl_pci 0000:35:00.0: found RAS capability (0x80)
> > [ 1396.814133] cxl_pci 0000:35:00.0: Unknown CM cap ID: 4 (0xd8)
> > [ 1396.815081] cxl_pci 0000:35:00.0: found HDM decoder capability (0x110)  
> 
> No issues here.
> 
> Details below.
> 
> Kernel:
> 
> version: 0c0df63177e3 Merge branch 'for-6.5/cxl-rch-eh' into for-6.5/cxl
> cmdline: 
> [    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-6.4.0-rc6-rric-00055-g0c0df63177e3 root=UUID=8b447531-798d-41cb-a3df-ae1d9bb1eaf9 ro debug ignore_loglevel "dyndbg=file drivers/cxl/* +p" systemd.log_target=journal
> 
> Qemu:
> 
> version: QEMU emulator version 8.0.50 (v6.2.0-12087-g62c0c95799)
> cmdline:
> /usr/local/bin/qemu-system-x86_64 -smp 6 -machine type=q35,accel=kvm,nvdimm=on,cxl=on -hda debian.img -m 4096 -enable-kvm -object memory-backend-file,id=cxl-mem1,share=on,mem-path=cxl-window1,size=512M -object memory-backend-file,id=cxl-label1,share=on,mem-path=cxl-label1,size=512M -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 -device cxl-rp,id=rp0,bus=cxl.0,addr=0.0,chassis=0,slot=0,port=0 -device cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,lsa=cxl-label1 -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G -vnc :0 -nic none
> 
> Annotated qemu kernel boot log after loading the cxl_acpi module
> (including deps):
> 
> > [  468.229977] cxl_acpi ACPI0017:00: root0 added (root port)  
> 
> root0 added.
> 
> > [  468.231055] acpi ACPI0016:00: UID found: 52
> > [  468.231959]  pci0000:34: dport added to root0  
> 
> add_host_bridge_dport() found the CXL host bridge and added it as
> dport to root0..
> 
> > [  468.232867] Unknown target node for memory at 0x190000000, assuming node 0
> > [  468.233901] cxl_acpi ACPI0017:00: add: decoder0.0 node: 0 range [0x190000000 - 0x28fffffff]
> > [  468.235394] acpi ACPI0016:00: UID found: 52
> > [  468.236171] acpi ACPI0016:00: CHBCR found for UID 52: 0x0000000180000000
> > [  468.237193]  pci0000:34: host-bridge: pci0000:34
> > [  468.238132] cxl port1: Mapped CXL Memory Device resource 0x0000000180000000  
> 
> CHBCR for the host bridge was found at 0x0000000180000000.
> 
> In this kernel version it is bound to port1 (not the dport), some
> comments on this later.
> 
> Component registers and capabilities are extracted from the CHBCR:
> 
> > [  468.239319] cxl port1: found RAS capability (0x80)
> > [  468.240186] cxl port1: Unknown CM cap ID: 4 (0xd8)
> > [  468.241083] cxl port1: Unknown CM cap ID: 0 (0x110)
> > [  468.241974] cxl port1: Unknown CM cap ID: 6 (0x260)
> > [  468.242876] cxl port1: Unknown CM cap ID: 8 (0xa84)  
> 
> The CHBCR does not contain the HDM registers for the root ports.
> 
> It (wrongly) contains the RAS caps.
> 
> I would expect HDM here instead.

As above. No on HDM but agreed RAS is 'novel' and should go away.
In a root bridge with just one RP the HDM is optional.

> 
> > [  468.243767] cxl port1: Set up component registers
> > [  468.244959]  pci0000:34: port1 added to root0  
> 
> port1 added to root0 with the component regs from the CHBCR bound.

I think that's reasonable as the port1 is the Host bridge in this case.

> 
> > [  468.245807]  pci0000:34: host supports CXL
> > [  468.246711] cxl_acpi ACPI0017:00: root0: add: nvdimm-bridge0
> > [  468.247792] CXL bus rescan result: 0  
> 
> port 1 added to the cxl bus which triggers cxl_port_probe:
> 
> Running cxl_switch_port_probe() for port1 here:
> 
> > [  468.250149] pcieport 0000:34:00.0: Component Registers found for dport: 0x00000000fea00000  
> 
> The CXL root port was found by devm_cxl_port_enumerate_dports().
> 
> Its component regs are extracted:
> 
> > [  468.250182] pcieport 0000:34:00.0: Mapped CXL Memory Device resource 0x00000000fea00000
> > [  468.250258] pcieport 0000:34:00.0: found RAS capability (0x80)
> > [  468.250371] pcieport 0000:34:00.0: Unknown CM cap ID: 4 (0xd8)
> > [  468.250413] pcieport 0000:34:00.0: found HDM decoder capability (0x110)
> > [  468.256022] pcieport 0000:34:00.0: Unknown CM cap ID: 6 (0x260)
> > [  468.256058] pcieport 0000:34:00.0: Unknown CM cap ID: 8 (0xa84)  
> 
> The CXL root port's comp regs contain the RAS caps but also (wrongly)
> the HDM caps.

Agreed - that's garbage though kernel should probably just moan about it.

> 
> > [  468.256065] pcieport 0000:34:00.0: Set up component registers
> > [  468.256089] pcieport 0000:34:00.0: dport added to port1  
> 
> CXL root port added as dport to port1.
> 
> devm_cxl_setup_hdm() is run for port1:
> 
> > [  468.256157] cxl_port port1: found RAS capability (0x80)
> > [  468.260800] cxl_port port1: Unknown CM cap ID: 4 (0xd8)
> > [  468.260833] cxl_port port1: Unknown CM cap ID: 0 (0x110)
> > [  468.260865] cxl_port port1: Unknown CM cap ID: 6 (0x260)
> > [  468.260898] cxl_port port1: Unknown CM cap ID: 8 (0xa84)  
> 
> port1's comp regs from above extracted again for HDM setup, but...
> 
> > [  468.260904] cxl_port port1: HDM decoder registers not implemented  
> 
> ... the HDM decoder could not be found. map_hdm_decoder_regs() returns
> a -ENODEV.
> 
> > [  468.260915] cxl_port port1: Fallback to passthrough decoder  
> 
> cxl_switch_port_probe() continues with
> devm_cxl_add_passthrough_decoder() as there was only one dport found.

That's what I'd expect for the RC.

> 
> > [  468.261024] cxl decoder1.0: Added to port port1
> > [  468.267186] cxl_port port1: probe: 0  
> 
> Probe success.
> 
> > [  468.298485] nd_bus ndbus0: START: nd_bus.probe(ndbus0)
> > [  468.299859] nd_bus ndbus0: END: nd_bus.probe(ndbus0) = 0
> > [  468.300840] cxl_nvdimm_bridge nvdimm-bridge0: probe: 0  
> 
> End of log. Full log attached.
> 
> Loading the cxl_pci driver later succeeds, cxl list shows:
> 
> [
>   {
>     "memdev":"mem0",
>     "pmem_size":536870912,
>     "serial":0,
>     "host":"0000:35:00.0"
>   }
> ]
> 
> I am not sure if the issue is a qemu config or implementation issue.
> 
> Please take a look. Am I missing something here?

The RAS registers case is definitely a bug (and I expect there are similar
cases) so we should fix that in QEMU, but impact should be small (some
warnings at most) if we are providing registers the kernel doesn't
recognize.  In theory the spec could change in future to make them defined
for additional CXL components (unlikely though!)

Jonathan


> 
> Thanks,
> 
> -Robert
diff mbox series

Patch

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 76743016b64c..8aee1a42d9af 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -263,6 +263,7 @@  struct cxl_poison_state {
  *
  * @dev: The device associated with this CXL state
  * @cxlmd: The device representing the CXL.mem capabilities of @dev
+ * @comp_map: component register capability mappings
  * @regs: Parsed register blocks
  * @cxl_dvsec: Offset to the PCIe device DVSEC
  * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
@@ -286,7 +287,6 @@  struct cxl_poison_state {
  * @active_persistent_bytes: sum of hard + soft persistent
  * @next_volatile_bytes: volatile capacity change pending device reset
  * @next_persistent_bytes: persistent capacity change pending device reset
- * @component_reg_phys: register base of component registers
  * @info: Cached DVSEC information about the device.
  * @serial: PCIe Device Serial Number
  * @event: event log driver state
@@ -299,7 +299,7 @@  struct cxl_poison_state {
 struct cxl_dev_state {
 	struct device *dev;
 	struct cxl_memdev *cxlmd;
-
+	struct cxl_register_map comp_map;
 	struct cxl_regs regs;
 	int cxl_dvsec;
 
@@ -325,7 +325,6 @@  struct cxl_dev_state {
 	u64 next_volatile_bytes;
 	u64 next_persistent_bytes;
 
-	resource_size_t component_reg_phys;
 	u64 serial;
 
 	struct cxl_event_state event;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 205e2e280aed..92c6151b7206 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -49,7 +49,6 @@  static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
 				 struct cxl_dport *parent_dport)
 {
 	struct cxl_port *parent_port = parent_dport->port;
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *endpoint, *iter, *down;
 	int rc;
 
@@ -65,8 +64,9 @@  static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
 		ep->next = down;
 	}
 
+	/* The Endpoint's component regs are located in cxlds. */
 	endpoint = devm_cxl_add_port(host, &cxlmd->dev,
-				     cxlds->component_reg_phys,
+				     CXL_RESOURCE_NONE,
 				     parent_dport);
 	if (IS_ERR(endpoint))
 		return PTR_ERR(endpoint);
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 99a75c54ee39..ad4cfcd95e17 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -665,16 +665,14 @@  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	 * If the component registers can't be found, the cxl_pci driver may
 	 * still be useful for management functions so don't return an error.
 	 */
-	cxlds->component_reg_phys = CXL_RESOURCE_NONE;
-	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
+	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
+				&cxlds->comp_map);
 	if (rc)
 		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
-	else if (!map.component_map.ras.valid)
+	else if (!cxlds->comp_map.component_map.ras.valid)
 		dev_dbg(&pdev->dev, "RAS registers not found\n");
 
-	cxlds->component_reg_phys = map.resource;
-
-	rc = cxl_map_component_regs(&map, &cxlds->regs.component,
+	rc = cxl_map_component_regs(&cxlds->comp_map, &cxlds->regs.component,
 				    BIT(CXL_CM_CAP_CAP_ID_RAS));
 	if (rc)
 		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 34b48027b3de..fd562a5fa06f 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -1241,7 +1241,6 @@  static int cxl_mock_mem_probe(struct platform_device *pdev)
 	cxlds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
 	if (is_rcd(pdev)) {
 		cxlds->rcd = true;
-		cxlds->component_reg_phys = CXL_RESOURCE_NONE;
 	}
 
 	rc = cxl_enumerate_cmds(cxlds);