Re: [PATCH v5 3/3] hw/cxl: Add a performant (and correct) path for the non interleaved cases

[Jonathan Cameron via qemu development <qemu-devel@nongnu.org>]

On Thu, 26 Feb 2026 00:14:05 +0000
Alireza Sanaee <alireza.sanaee@huawei.com> wrote:

> On Wed, 25 Feb 2026 15:34:27 -0500
> Gregory Price <gourry@gourry.net> wrote:
> 
> Hi Gregory,
> 
> > On Wed, Feb 25, 2026 at 06:07:18PM +0000, Alireza Sanaee via qemu development wrote:  
> > > +static void cxl_fmws_direct_passthrough_remove(CXLType3Dev *ct3d,
> > > +                                               uint64_t decoder_base,
> > > +                                               unsigned int idx)
> > > +{
> > > +    CXLFixedWindow *owner_fw = ct3d->direct_mr_fw[idx];
> > > +    MemoryRegion *direct_mr = &ct3d->direct_mr[idx];
> > > +
> > > +    if (!owner_fw) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (!memory_region_is_mapped(direct_mr)) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (cxl_cfmws_find_device(owner_fw, decoder_base, false)) {
> > > +        return;
> > > +    }
> > > +
> > > +    memory_region_del_subregion(&owner_fw->mr, direct_mr);
> > > +    ct3d->direct_mr_fw[idx] = NULL;
> > > +}
> > > +    
> > 
> > kreview flagged this, seems reasonable given other examples in the tree
> > 
> > The other commits looked ok, I'll try to get around to testing this soon.
> > 
> > ~Gregory
> > 
> > ---
> > 
> > Reported-by: kreview-8842242
> > 
> > The alias parameters (target MR, offset, size) are all guest-controlled
> > and can change between cycles -- the guest can reprogram DPA skip, decoder
> > size, or decoder base before recommitting.  There is no API to change the
> > alias target (mr->alias) after init, so an init-once approach would not
> > work here.
> > 
> > The VGA chain4_alias in hw/display/vga.c handles the same situation
> > (embedded MR alias rebuilt with different parameters on guest register
> > writes) using object_unparent():
> > 
> >     vga_update_memory_access() {
> >         memory_region_del_subregion(s->legacy_address_space, &s->chain4_alias);
> >         object_unparent(OBJECT(&s->chain4_alias));
> >         ...
> >         memory_region_init_alias(&s->chain4_alias, ...);
> >         memory_region_add_subregion_overlap(..., &s->chain4_alias, 2);
> >     }
> > 
> > Adding object_unparent() in remove() should fix this:
> > 
> >     memory_region_del_subregion(&owner_fw->mr, direct_mr);
> >     object_unparent(OBJECT(direct_mr));
> >     ct3d->direct_mr_fw[idx] = NULL;
> >   
> 
> This was actually my question as well, but I tested with a scenario in which I 
> created a region with its alias. Then I torn down that region so that the alias 
> goes away, and finally reinitialized the same region to reuse the deleted alias 
> object. It worked, but still it does not mean it is correct. I can send another 
> revision using unparent API.

It is more elegant (and keeps the introspection stuff reporting correctly)
to unparent the object on delete. To test this you'd need to bring it up in a different
cfmws so the parent changes.  So this is good feedback.

There is a secondary issue that around the HBM decoder registers being modified at random
points that don't make sense from an OS flow point of view, but which could lead to
mismatch.  For that one we'd need to snapshot the decoders at commit time and use
that snapshot as the valid settings.

Note that form a spec viewpoint I think it's undefined what happens if the OS messes
with these registers at inappropriate moments.  Snap shot should be a valid 
implementation choice though.

Given we aren't currently that defensive against the OS doing crazy things I don't
mind pushing that snapshot thing to a follow up patch.  Maybe merge into this once
it's ready.

Jonathan