RE: [PATCH v5 02/27] cxl/regs: Let a BAR-owning driver own the component register block

[Manish Honap]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Tuesday, September 22, 2026 7:06 AM
> To: Manish Honap <mhonap@xxxxxxxxxx>
> Cc: alex@xxxxxxxxxxx; jgg@xxxxxxxx; Ankit Agrawal <ankita@xxxxxxxxxx>;
> dave.jiang@xxxxxxxxx; alejandro.lucero-palau@xxxxxxx; Srirangan
> Madhavan <smadhavan@xxxxxxxxxx>; corbet@xxxxxxx;
> skhan@xxxxxxxxxxxxxxxxxxx; dave@xxxxxxxxxxxx; alison.schofield@xxxxxxxxx;
> vishal.l.verma@xxxxxxxxx; iweiny@xxxxxxxxxx; ming.li@xxxxxxxxxxxx; Yishai
> Hadas <yishaih@xxxxxxxxxx>; Shameer Kolothum Thodi
> <skolothumtho@xxxxxxxxxx>; kevin.tian@xxxxxxxxx; bhelgaas@xxxxxxxxxx;
> dmatlack@xxxxxxxxxx; kees@xxxxxxxxxx; gustavoars@xxxxxxxxxx; Neo Jia
> <cjia@xxxxxxxxxx>; Krishnakant Jaju <kjaju@xxxxxxxxxx>; Vikram Sethi
> <vsethi@xxxxxxxxxx>; Zhi Wang <zhiw@xxxxxxxxxx>; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> linux-cxl@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kselftest@xxxxxxxxxxxxxxx; linux-hardening@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 02/27] cxl/regs: Let a BAR-owning driver own the
> component register block
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 17 Sep 2026 00:05:15 +0530
> <mhonap@xxxxxxxxxx> wrote:
> 
> > From: Manish Honap <mhonap@xxxxxxxxxx>
> >
> > cxl_map_component_regs() claims each mapped sub-block with
> > devm_request_mem_region(). A driver that already requested the whole
> > component register BAR, such as vfio-cxl, would then collide with that
> > claim and fail to map the HDM decoder and RAS blocks.
> >
> > Add cxl_reg_map_add_owned_resource() so such a driver records the
> > resource it already owns on the register map, and skip the sub-block
> > request when the block falls within an owned resource.
> 
> This seems a slightly odd intermediate point.
> 
> The assumption is that if owned the whole bar is owned - hence only handle
> one owned region (no check if there is already one set?) yet the code will try to
> map it if we get a request for something outside of the region mapped (which
> can't exist).
> 
> This all makes me a little nervous.
> 

Agreed; current model is to have one owned resource - the whole component register
BAR and I will enforce this in v6.
1. Helper is going to be renamed to cxl_reg_map_set_owned_resource and it will
   return -EBUSY when resource is already set.
2. cxl_map_component_regs will fail with -ENXIO when a block of an owned map lies
  outside the owned resource.

> >
> > Assisted-by: LLM
> > Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> > ---
> >  drivers/cxl/core/regs.c | 17 +++++++++++++++--
> >  include/cxl/cxl.h       |  2 ++
> >  include/cxl/pci.h       |  3 +++
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index
> > d79550dbb484..58a7c5cafb45 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -216,6 +216,13 @@ void __iomem *devm_cxl_iomap_block(struct
> device
> > *dev, resource_size_t addr,  }
> > EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, "CXL");
> >
> > +void cxl_reg_map_add_owned_resource(struct cxl_register_map *map,
> > +                                 struct resource *res) {
> > +     map->owned = res;
> 
> As above, I think this needs a sanity check that a future driver author doesn't
> think they can just add lots of these.
> Absolute minimum is add some Docs.

Yes, I will add the documentation.

> 
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_reg_map_add_owned_resource, "CXL");
> > +
> >  int cxl_map_component_regs(const struct cxl_register_map *map,
> >                          struct cxl_component_regs *regs,
> >                          unsigned long map_mask) @@ -234,6 +241,7 @@
> > int cxl_map_component_regs(const struct cxl_register_map *map,
> >               struct mapinfo *mi = &mapinfo[i];
> >               resource_size_t addr;
> >               resource_size_t length;
> > +             struct resource res;
> >
> >               if (!mi->rmap->valid)
> >                       continue;
> > @@ -241,8 +249,13 @@ int cxl_map_component_regs(const struct
> cxl_register_map *map,
> >                       continue;
> >               addr = map->resource + mi->rmap->offset;
> >               length = mi->rmap->size;
> > -             *(mi->addr) = devm_cxl_iomap_block(host, addr, length);
> > -             if (!*(mi->addr))
> > +             res = DEFINE_RES_MEM(addr, length);
> > +
> > +             if (map->owned && resource_contains(map->owned, &res))
> 
> If assumption is whole bar or none, should we just fail if map->owned is true
> but the resource contains fails?

Yes, agreed; I will also add this.

> 
> > +                     *mi->addr = devm_cxl_ioremap_block(host, addr, length);
> > +             else
> > +                     *mi->addr = devm_cxl_iomap_block(host, addr, length);
> > +             if (!*mi->addr)
> >                       return -ENOMEM;
> >       }
> >