[v6,10/12] hw/mem/cxl_type3: Add dpa range validation for accesses to DC regions

Commit Message

Fan Ni March 25, 2024, 7:02 p.m. UTC

From: Fan Ni <[e-mail]>

All dpa ranges in the DC regions are invalid to access until an extent
covering the range has been added. Add a bitmap for each region to
record whether a DC block in the region has been backed by DC extent.
For the bitmap, a bit in the bitmap represents a DC block. When a DC
extent is added, all the bits of the blocks in the extent will be set,
which will be cleared when the extent is released.

Reviewed-by: Jonathan Cameron <[e-mail]>
Signed-off-by: Fan Ni <[e-mail]>
---
 hw/cxl/cxl-mailbox-utils.c  |  6 +++
 hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
 include/hw/cxl/cxl_device.h |  7 ++++
 3 files changed, 89 insertions(+)

Comments

Jonathan Cameron April 5, 2024, 12:29 p.m. UTC | #1

On Mon, 25 Mar 2024 12:02:28 -0700
[e-mail] wrote:

> From: Fan Ni <[e-mail]>
> 
> All dpa ranges in the DC regions are invalid to access until an extent

Let's be more consistent for commit logs and use DPA DC HPA etc all
caps. It's a bit of a mixture in this series at the moment.

> covering the range has been added.
I'd expand that to 'has been successfully accepted by the host.'

> Add a bitmap for each region to
> record whether a DC block in the region has been backed by DC extent.
> For the bitmap, a bit in the bitmap represents a DC block. When a DC
> extent is added, all the bits of the blocks in the extent will be set,
> which will be cleared when the extent is released.
> 
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Fan Ni <[e-mail]>

Gregory Price April 12, 2024, 10:54 p.m. UTC | #2

On Mon, Mar 25, 2024 at 12:02:28PM -0700, [e-mail] wrote:
> From: Fan Ni <[e-mail]>
> 
> All dpa ranges in the DC regions are invalid to access until an extent
> covering the range has been added. Add a bitmap for each region to
> record whether a DC block in the region has been backed by DC extent.
> For the bitmap, a bit in the bitmap represents a DC block. When a DC
> extent is added, all the bits of the blocks in the extent will be set,
> which will be cleared when the extent is released.
> 
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Fan Ni <[e-mail]>
> ---
>  hw/cxl/cxl-mailbox-utils.c  |  6 +++
>  hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h |  7 ++++
>  3 files changed, 89 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 7094e007b9..a0d2239176 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1620,6 +1620,7 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
>  
>          cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
>          ct3d->dc.total_extent_count += 1;
> +        ct3_set_region_block_backed(ct3d, dpa, len);
>  
>          ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
>          cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);

while looking at the MHD code, we had decided to "reserve" the blocks in
the bitmap in the call to `qmp_cxl_process_dynamic_capacity` in order to
prevent a potential double-allocation (basically we need to sanity check
that two hosts aren't reserving the region PRIOR to the host being
notified).

I did not see any checks in the `qmp_cxl_process_dynamic_capacity` path
to prevent pending extents from being double-allocated.  Is this an
explicit choice?

I can see, for example, why you may want to allow the following in the
pending list: [Add X, Remove X, Add X].  I just want to know if this is
intentional or not. If not, you may consider adding a pending check
during the sanity check phase of `qmp_cxl_process_dynamic_capacity`

~Gregory

Fan Ni April 15, 2024, 5:37 p.m. UTC | #3

On Fri, Apr 12, 2024 at 06:54:42PM -0400, Gregory Price wrote:
> On Mon, Mar 25, 2024 at 12:02:28PM -0700, [e-mail] wrote:
> > From: Fan Ni <[e-mail]>
> > 
> > All dpa ranges in the DC regions are invalid to access until an extent
> > covering the range has been added. Add a bitmap for each region to
> > record whether a DC block in the region has been backed by DC extent.
> > For the bitmap, a bit in the bitmap represents a DC block. When a DC
> > extent is added, all the bits of the blocks in the extent will be set,
> > which will be cleared when the extent is released.
> > 
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Signed-off-by: Fan Ni <[e-mail]>
> > ---
> >  hw/cxl/cxl-mailbox-utils.c  |  6 +++
> >  hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
> >  include/hw/cxl/cxl_device.h |  7 ++++
> >  3 files changed, 89 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 7094e007b9..a0d2239176 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -1620,6 +1620,7 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> >  
> >          cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> >          ct3d->dc.total_extent_count += 1;
> > +        ct3_set_region_block_backed(ct3d, dpa, len);
> >  
> >          ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
> >          cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);
> 
> while looking at the MHD code, we had decided to "reserve" the blocks in
> the bitmap in the call to `qmp_cxl_process_dynamic_capacity` in order to
> prevent a potential double-allocation (basically we need to sanity check
> that two hosts aren't reserving the region PRIOR to the host being
> notified).
> 
> I did not see any checks in the `qmp_cxl_process_dynamic_capacity` path
> to prevent pending extents from being double-allocated.  Is this an
> explicit choice?
> 
> I can see, for example, why you may want to allow the following in the
> pending list: [Add X, Remove X, Add X].  I just want to know if this is
> intentional or not. If not, you may consider adding a pending check
> during the sanity check phase of `qmp_cxl_process_dynamic_capacity`
> 
> ~Gregory

First, for remove request, pending list is not involved. See cxl r3.1,
9.13.3.3. Pending basically means "pending to add". 
So for the above example, in the pending list, you can see [Add x, add x] if the
event is not processed in time.
Second, from the spec, I cannot find any text saying we cannot issue
another add extent X if it is still pending.
From the kernel side, if the first one is accepted, the second one will
get rejected, and there is no issue there.
If the first is reject for some reason, the second one can get
accepted or rejected and do not need to worry about the first one.


Fan

Jonathan Cameron April 16, 2024, 3 p.m. UTC | #4

On Mon, 15 Apr 2024 10:37:00 -0700
fan <[e-mail]> wrote:

> On Fri, Apr 12, 2024 at 06:54:42PM -0400, Gregory Price wrote:
> > On Mon, Mar 25, 2024 at 12:02:28PM -0700, [e-mail] wrote:  
> > > From: Fan Ni <[e-mail]>
> > > 
> > > All dpa ranges in the DC regions are invalid to access until an extent
> > > covering the range has been added. Add a bitmap for each region to
> > > record whether a DC block in the region has been backed by DC extent.
> > > For the bitmap, a bit in the bitmap represents a DC block. When a DC
> > > extent is added, all the bits of the blocks in the extent will be set,
> > > which will be cleared when the extent is released.
> > > 
> > > Reviewed-by: Jonathan Cameron <[e-mail]>
> > > Signed-off-by: Fan Ni <[e-mail]>
> > > ---
> > >  hw/cxl/cxl-mailbox-utils.c  |  6 +++
> > >  hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
> > >  include/hw/cxl/cxl_device.h |  7 ++++
> > >  3 files changed, 89 insertions(+)
> > > 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index 7094e007b9..a0d2239176 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -1620,6 +1620,7 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> > >  
> > >          cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> > >          ct3d->dc.total_extent_count += 1;
> > > +        ct3_set_region_block_backed(ct3d, dpa, len);
> > >  
> > >          ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
> > >          cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);  
> > 
> > while looking at the MHD code, we had decided to "reserve" the blocks in
> > the bitmap in the call to `qmp_cxl_process_dynamic_capacity` in order to
> > prevent a potential double-allocation (basically we need to sanity check
> > that two hosts aren't reserving the region PRIOR to the host being
> > notified).
> > 
> > I did not see any checks in the `qmp_cxl_process_dynamic_capacity` path
> > to prevent pending extents from being double-allocated.  Is this an
> > explicit choice?
> > 
> > I can see, for example, why you may want to allow the following in the
> > pending list: [Add X, Remove X, Add X].  I just want to know if this is
> > intentional or not. If not, you may consider adding a pending check
> > during the sanity check phase of `qmp_cxl_process_dynamic_capacity`
> > 
> > ~Gregory  
> 
> First, for remove request, pending list is not involved. See cxl r3.1,
> 9.13.3.3. Pending basically means "pending to add". 
> So for the above example, in the pending list, you can see [Add x, add x] if the
> event is not processed in time.
> Second, from the spec, I cannot find any text saying we cannot issue
> another add extent X if it is still pending.

I think there is text saying that the capacity is not released for reuse
by the device until it receives a response from the host.   Whilst
it's not explicit on offers to the same host, I'm not sure that matters.
So I don't think it is suppose to queue multiple extents...


> From the kernel side, if the first one is accepted, the second one will
> get rejected, and there is no issue there.
> If the first is reject for some reason, the second one can get
> accepted or rejected and do not need to worry about the first one.
> 
> 
> Fan
>

Fan Ni April 16, 2024, 4:37 p.m. UTC | #5

On Tue, Apr 16, 2024 at 04:00:56PM +0100, Jonathan Cameron wrote:
> On Mon, 15 Apr 2024 10:37:00 -0700
> fan <[e-mail]> wrote:
> 
> > On Fri, Apr 12, 2024 at 06:54:42PM -0400, Gregory Price wrote:
> > > On Mon, Mar 25, 2024 at 12:02:28PM -0700, [e-mail] wrote:  
> > > > From: Fan Ni <[e-mail]>
> > > > 
> > > > All dpa ranges in the DC regions are invalid to access until an extent
> > > > covering the range has been added. Add a bitmap for each region to
> > > > record whether a DC block in the region has been backed by DC extent.
> > > > For the bitmap, a bit in the bitmap represents a DC block. When a DC
> > > > extent is added, all the bits of the blocks in the extent will be set,
> > > > which will be cleared when the extent is released.
> > > > 
> > > > Reviewed-by: Jonathan Cameron <[e-mail]>
> > > > Signed-off-by: Fan Ni <[e-mail]>
> > > > ---
> > > >  hw/cxl/cxl-mailbox-utils.c  |  6 +++
> > > >  hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
> > > >  include/hw/cxl/cxl_device.h |  7 ++++
> > > >  3 files changed, 89 insertions(+)
> > > > 
> > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > index 7094e007b9..a0d2239176 100644
> > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > > @@ -1620,6 +1620,7 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> > > >  
> > > >          cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> > > >          ct3d->dc.total_extent_count += 1;
> > > > +        ct3_set_region_block_backed(ct3d, dpa, len);
> > > >  
> > > >          ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
> > > >          cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);  
> > > 
> > > while looking at the MHD code, we had decided to "reserve" the blocks in
> > > the bitmap in the call to `qmp_cxl_process_dynamic_capacity` in order to
> > > prevent a potential double-allocation (basically we need to sanity check
> > > that two hosts aren't reserving the region PRIOR to the host being
> > > notified).
> > > 
> > > I did not see any checks in the `qmp_cxl_process_dynamic_capacity` path
> > > to prevent pending extents from being double-allocated.  Is this an
> > > explicit choice?
> > > 
> > > I can see, for example, why you may want to allow the following in the
> > > pending list: [Add X, Remove X, Add X].  I just want to know if this is
> > > intentional or not. If not, you may consider adding a pending check
> > > during the sanity check phase of `qmp_cxl_process_dynamic_capacity`
> > > 
> > > ~Gregory  
> > 
> > First, for remove request, pending list is not involved. See cxl r3.1,
> > 9.13.3.3. Pending basically means "pending to add". 
> > So for the above example, in the pending list, you can see [Add x, add x] if the
> > event is not processed in time.
> > Second, from the spec, I cannot find any text saying we cannot issue
> > another add extent X if it is still pending.
> 
> I think there is text saying that the capacity is not released for reuse
> by the device until it receives a response from the host.   Whilst
> it's not explicit on offers to the same host, I'm not sure that matters.
> So I don't think it is suppose to queue multiple extents...

Are you suggesting we add a check here to reject the second add when the
first one is still pending?

Currently, we do not allow releasing an extent when it is still pending,
which aligns with the case you mentioned above "not release for reuse", I
think.
Can the second add mean a retry instead of reuse? 

Fan

> 
> 
> > From the kernel side, if the first one is accepted, the second one will
> > get rejected, and there is no issue there.
> > If the first is reject for some reason, the second one can get
> > accepted or rejected and do not need to worry about the first one.
> > 
> > 
> > Fan
> > 
>

Gregory Price April 16, 2024, 5:15 p.m. UTC | #6

On Tue, Apr 16, 2024 at 04:00:56PM +0100, Jonathan Cameron wrote:
> On Mon, 15 Apr 2024 10:37:00 -0700
> fan <[e-mail]> wrote:
> 
> > On Fri, Apr 12, 2024 at 06:54:42PM -0400, Gregory Price wrote:
> > > On Mon, Mar 25, 2024 at 12:02:28PM -0700, [e-mail] wrote:  
> > > > From: Fan Ni <[e-mail]>
> > > > 
> > > > All dpa ranges in the DC regions are invalid to access until an extent
> > > > covering the range has been added. Add a bitmap for each region to
> > > > record whether a DC block in the region has been backed by DC extent.
> > > > For the bitmap, a bit in the bitmap represents a DC block. When a DC
> > > > extent is added, all the bits of the blocks in the extent will be set,
> > > > which will be cleared when the extent is released.
> > > > 
> > > > Reviewed-by: Jonathan Cameron <[e-mail]>
> > > > Signed-off-by: Fan Ni <[e-mail]>
> > > > ---
> > > >  hw/cxl/cxl-mailbox-utils.c  |  6 +++
> > > >  hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
> > > >  include/hw/cxl/cxl_device.h |  7 ++++
> > > >  3 files changed, 89 insertions(+)
> > > > 
> > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > index 7094e007b9..a0d2239176 100644
> > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > > @@ -1620,6 +1620,7 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> > > >  
> > > >          cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> > > >          ct3d->dc.total_extent_count += 1;
> > > > +        ct3_set_region_block_backed(ct3d, dpa, len);
> > > >  
> > > >          ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
> > > >          cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);  
> > > 
> > > while looking at the MHD code, we had decided to "reserve" the blocks in
> > > the bitmap in the call to `qmp_cxl_process_dynamic_capacity` in order to
> > > prevent a potential double-allocation (basically we need to sanity check
> > > that two hosts aren't reserving the region PRIOR to the host being
> > > notified).
> > > 
> > > I did not see any checks in the `qmp_cxl_process_dynamic_capacity` path
> > > to prevent pending extents from being double-allocated.  Is this an
> > > explicit choice?
> > > 
> > > I can see, for example, why you may want to allow the following in the
> > > pending list: [Add X, Remove X, Add X].  I just want to know if this is
> > > intentional or not. If not, you may consider adding a pending check
> > > during the sanity check phase of `qmp_cxl_process_dynamic_capacity`
> > > 
> > > ~Gregory  
> > 
> > First, for remove request, pending list is not involved. See cxl r3.1,
> > 9.13.3.3. Pending basically means "pending to add". 
> > So for the above example, in the pending list, you can see [Add x, add x] if the
> > event is not processed in time.
> > Second, from the spec, I cannot find any text saying we cannot issue
> > another add extent X if it is still pending.
> 
> I think there is text saying that the capacity is not released for reuse
> by the device until it receives a response from the host.   Whilst
> it's not explicit on offers to the same host, I'm not sure that matters.
> So I don't think it is suppose to queue multiple extents...
> 
> 

It definitely should not release capacity until it receives a response,
because the host could tell the device to kick rocks (which would be
reasonable under a variety of circumstances).

~Gregory

Jonathan Cameron April 17, 2024, 11:59 a.m. UTC | #7

On Tue, 16 Apr 2024 09:37:09 -0700
fan <[e-mail]> wrote:

> On Tue, Apr 16, 2024 at 04:00:56PM +0100, Jonathan Cameron wrote:
> > On Mon, 15 Apr 2024 10:37:00 -0700
> > fan <[e-mail]> wrote:
> >   
> > > On Fri, Apr 12, 2024 at 06:54:42PM -0400, Gregory Price wrote:  
> > > > On Mon, Mar 25, 2024 at 12:02:28PM -0700, [e-mail] wrote:    
> > > > > From: Fan Ni <[e-mail]>
> > > > > 
> > > > > All dpa ranges in the DC regions are invalid to access until an extent
> > > > > covering the range has been added. Add a bitmap for each region to
> > > > > record whether a DC block in the region has been backed by DC extent.
> > > > > For the bitmap, a bit in the bitmap represents a DC block. When a DC
> > > > > extent is added, all the bits of the blocks in the extent will be set,
> > > > > which will be cleared when the extent is released.
> > > > > 
> > > > > Reviewed-by: Jonathan Cameron <[e-mail]>
> > > > > Signed-off-by: Fan Ni <[e-mail]>
> > > > > ---
> > > > >  hw/cxl/cxl-mailbox-utils.c  |  6 +++
> > > > >  hw/mem/cxl_type3.c          | 76 +++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/cxl/cxl_device.h |  7 ++++
> > > > >  3 files changed, 89 insertions(+)
> > > > > 
> > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > > index 7094e007b9..a0d2239176 100644
> > > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > > > @@ -1620,6 +1620,7 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
> > > > >  
> > > > >          cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
> > > > >          ct3d->dc.total_extent_count += 1;
> > > > > +        ct3_set_region_block_backed(ct3d, dpa, len);
> > > > >  
> > > > >          ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
> > > > >          cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);    
> > > > 
> > > > while looking at the MHD code, we had decided to "reserve" the blocks in
> > > > the bitmap in the call to `qmp_cxl_process_dynamic_capacity` in order to
> > > > prevent a potential double-allocation (basically we need to sanity check
> > > > that two hosts aren't reserving the region PRIOR to the host being
> > > > notified).
> > > > 
> > > > I did not see any checks in the `qmp_cxl_process_dynamic_capacity` path
> > > > to prevent pending extents from being double-allocated.  Is this an
> > > > explicit choice?
> > > > 
> > > > I can see, for example, why you may want to allow the following in the
> > > > pending list: [Add X, Remove X, Add X].  I just want to know if this is
> > > > intentional or not. If not, you may consider adding a pending check
> > > > during the sanity check phase of `qmp_cxl_process_dynamic_capacity`
> > > > 
> > > > ~Gregory    
> > > 
> > > First, for remove request, pending list is not involved. See cxl r3.1,
> > > 9.13.3.3. Pending basically means "pending to add". 
> > > So for the above example, in the pending list, you can see [Add x, add x] if the
> > > event is not processed in time.
> > > Second, from the spec, I cannot find any text saying we cannot issue
> > > another add extent X if it is still pending.  
> > 
> > I think there is text saying that the capacity is not released for reuse
> > by the device until it receives a response from the host.   Whilst
> > it's not explicit on offers to the same host, I'm not sure that matters.
> > So I don't think it is suppose to queue multiple extents...  
> 
> Are you suggesting we add a check here to reject the second add when the
> first one is still pending?

Yes.  The capacity is not back with the device to reissue.
On an MH-MLD/SLD we'd need to prevent it being added (not shared) to multiple hosts,
this is kind of the temporal equivalent of that.

> 
> Currently, we do not allow releasing an extent when it is still pending,
> which aligns with the case you mentioned above "not release for reuse", I
> think.
> Can the second add mean a retry instead of reuse? 
No - or at least the device should not be doing that.  The FM might try
again, but only once it knows try 1 failed. For reasons of this aligning
with MHD case where you definitely can't offer it to more than one host,
I think we should not do it.  Whether we should put any effort into blocking
it is a different question.  User error :)

Note, the host must not remove a log entry until it has dealt with it
(sent a response) so there is no obvious reason to bother with a retry.
Maybe a booting host would reject all offered extents (because it's not ready
for them yet), but then I'd want the FM to explicitly decide to tell the device
to offer gain.

Whilst this is a custom interface, the equivalent FM API does say.

"The command, with selection policy Enable Shared Access, shall also fail with Invalid
Input under the following conditions:
• When the specified region is not Sharable
• When the tagged capacity is already mapped to any Host ID via a non-Sharable
region
• When the tagged capacity cannot be added to the requested region due to deviceimposed
restrictions
• When the same tagged capacity is currently accessible by the same LD"

Little fuzzy because of the whole pending vs 'mapped / accessible' wording but
I think intent is you can't send again until first one is dealt with.

Jonathan

> 
> Fan
> 
> > 
> >   
> > > From the kernel side, if the first one is accepted, the second one will
> > > get rejected, and there is no issue there.
> > > If the first is reject for some reason, the second one can get
> > > accepted or rejected and do not need to worry about the first one.
> > > 
> > > 
> > > Fan
> > >   
> >

Gregory Price April 18, 2024, 5:58 p.m. UTC | #8

On Wed, Apr 17, 2024 at 12:59:51PM +0100, Jonathan Cameron wrote:
> On Tue, 16 Apr 2024 09:37:09 -0700
> fan <[e-mail]> wrote:
> 
> > 
> > Currently, we do not allow releasing an extent when it is still pending,
> > which aligns with the case you mentioned above "not release for reuse", I
> > think.
> > Can the second add mean a retry instead of reuse? 
> No - or at least the device should not be doing that.  The FM might try
> again, but only once it knows try 1 failed. For reasons of this aligning
> with MHD case where you definitely can't offer it to more than one host,
> I think we should not do it.  Whether we should put any effort into blocking
> it is a different question.  User error :)
> 
> Note, the host must not remove a log entry until it has dealt with it
> (sent a response) so there is no obvious reason to bother with a retry.
> Maybe a booting host would reject all offered extents (because it's not ready
> for them yet), but then I'd want the FM to explicitly decide to tell the device
> to offer gain.
> 

This might be the only time a forced-removal makes sense, considering
that removal of a pending add could be potentially catestrophic, but if
the FM knows the host is not coming up and never coming up, an
allocation stuck in pending would not be recoverable unless you
force-removed it.

> Whilst this is a custom interface, the equivalent FM API does say.
> 
> "The command, with selection policy Enable Shared Access, shall also fail with Invalid
> Input under the following conditions:
> • When the specified region is not Sharable
> • When the tagged capacity is already mapped to any Host ID via a non-Sharable
> region
> • When the tagged capacity cannot be added to the requested region due to deviceimposed
> restrictions
> • When the same tagged capacity is currently accessible by the same LD"
> 
> Little fuzzy because of the whole pending vs 'mapped / accessible' wording but
> I think intent is you can't send again until first one is dealt with.
> 
> Jonathan
> 
> > 
> > Fan
> > 
> > > 
> > >   
> > > > From the kernel side, if the first one is accepted, the second one will
> > > > get rejected, and there is no issue there.
> > > > If the first is reject for some reason, the second one can get
> > > > accepted or rejected and do not need to worry about the first one.
> > > > 
> > > > 
> > > > Fan
> > > >   
> > >   
>

Patch

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 7094e007b9..a0d2239176 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1620,6 +1620,7 @@  static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
 
         cxl_insert_extent_to_extent_list(extent_list, dpa, len, NULL, 0);
         ct3d->dc.total_extent_count += 1;
+        ct3_set_region_block_backed(ct3d, dpa, len);
 
         ent = QTAILQ_FIRST(&ct3d->dc.extents_pending);
         cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending, ent);
@@ -1798,18 +1799,23 @@  static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
 
                     cxl_remove_extent_from_extent_list(extent_list, ent);
                     ct3d->dc.total_extent_count -= 1;
+                    ct3_clear_region_block_backed(ct3d, ent_start_dpa,
+                                                  ent_len);
 
                     if (len1) {
                         cxl_insert_extent_to_extent_list(extent_list,
                                                          ent_start_dpa,
                                                          len1, NULL, 0);
                         ct3d->dc.total_extent_count += 1;
+                        ct3_set_region_block_backed(ct3d, ent_start_dpa,
+                                                    len1);
                     }
                     if (len2) {
                         cxl_insert_extent_to_extent_list(extent_list,
                                                          dpa + len,
                                                          len2, NULL, 0);
                         ct3d->dc.total_extent_count += 1;
+                        ct3_set_region_block_backed(ct3d, dpa + len, len2);
                     }
 
                     len -= len_done;
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 74cb64e843..2628a6f50f 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -672,6 +672,7 @@  static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
             .flags = 0,
         };
         ct3d->dc.total_capacity += region->len;
+        region->blk_bitmap = bitmap_new(region->len / region->block_size);
     }
     QTAILQ_INIT(&ct3d->dc.extents);
     QTAILQ_INIT(&ct3d->dc.extents_pending);
@@ -682,6 +683,8 @@  static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
 static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
 {
     CXLDCExtent *ent, *ent_next;
+    int i;
+    CXLDCRegion *region;
 
     QTAILQ_FOREACH_SAFE(ent, &ct3d->dc.extents, node, ent_next) {
         cxl_remove_extent_from_extent_list(&ct3d->dc.extents, ent);
@@ -690,6 +693,11 @@  static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
         cxl_remove_extent_from_extent_list(&ct3d->dc.extents_pending,
                                            ent);
     }
+
+    for (i = 0; i < ct3d->dc.num_regions; i++) {
+        region = &ct3d->dc.regions[i];
+        g_free(region->blk_bitmap);
+    }
 }
 
 static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
@@ -921,6 +929,70 @@  static void ct3_exit(PCIDevice *pci_dev)
     }
 }
 
+/*
+ * Mark the DPA range [dpa, dap + len - 1] to be backed and accessible. This
+ * happens when a DC extent is added and accepted by the host.
+ */
+void ct3_set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                 uint64_t len)
+{
+    CXLDCRegion *region;
+
+    region = cxl_find_dc_region(ct3d, dpa, len);
+    if (!region) {
+        return;
+    }
+
+    bitmap_set(region->blk_bitmap, (dpa - region->base) / region->block_size,
+               len / region->block_size);
+}
+
+/*
+ * Check whether the DPA range [dpa, dpa + len - 1] is backed with DC extents.
+ * Used when validating read/write to dc regions
+ */
+bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                  uint64_t len)
+{
+    CXLDCRegion *region;
+    uint64_t nbits;
+    long nr;
+
+    region = cxl_find_dc_region(ct3d, dpa, len);
+    if (!region) {
+        return false;
+    }
+
+    nr = (dpa - region->base) / region->block_size;
+    nbits = DIV_ROUND_UP(len, region->block_size);
+    /*
+     * if bits between [dpa, dpa + len) are all 1s, meaning the DPA range is
+     * backed with DC extents, return true; else return false.
+     */
+    return find_next_zero_bit(region->blk_bitmap, nr + nbits, nr) == nr + nbits;
+}
+
+/*
+ * Mark the DPA range [dpa, dap + len -1] to be unbacked and inaccessible.
+ * This happens when a dc extent is released by the host.
+ */
+void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                   uint64_t len)
+{
+    CXLDCRegion *region;
+    uint64_t nbits;
+    long nr;
+
+    region = cxl_find_dc_region(ct3d, dpa, len);
+    if (!region) {
+        return;
+    }
+
+    nr = (dpa - region->base) / region->block_size;
+    nbits = len / region->block_size;
+    bitmap_clear(region->blk_bitmap, nr, nbits);
+}
+
 static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
 {
     int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
@@ -1025,6 +1097,10 @@  static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
         *as = &ct3d->hostpmem_as;
         *dpa_offset -= vmr_size;
     } else {
+        if (!ct3_test_region_block_backed(ct3d, *dpa_offset, size)) {
+            return -ENODEV;
+        }
+
         *as = &ct3d->dc.host_dc_as;
         *dpa_offset -= (vmr_size + pmr_size);
     }
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index b84063d9f4..bc90da2ca2 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -450,6 +450,7 @@  typedef struct CXLDCRegion {
     uint64_t block_size;
     uint32_t dsmadhandle;
     uint8_t flags;
+    unsigned long *blk_bitmap;
 } CXLDCRegion;
 
 struct CXLType3Dev {
@@ -563,4 +564,10 @@  bool test_any_bits_set(const unsigned long *addr, unsigned long nr,
                        unsigned long size);
 bool cxl_extents_contains_dpa_range(CXLDCExtentList *list,
                                     uint64_t dpa, uint64_t len);
+void ct3_set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                 uint64_t len);
+void ct3_clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                   uint64_t len);
+bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
+                                  uint64_t len);
 #endif