Re: [PATCH 2/2] hw/cxl/cxl-mailbox-utils: Add support for Media operations Sanitize and Write Zeros commands (8.2.9.9.5.3)

[Adam Manzanares <a.manzanares@samsung.com>]

On Mon, Feb 03, 2025 at 11:33:54AM +0000, Jonathan Cameron wrote:
> 
> > >   
> > > > +    int dpa_range_count = san_info->dpa_range_count;
> > > > +    int rc = 0;
> > > > +
> > > > +    for (int i = 0; i < dpa_range_count; i++) {
> > > > +        rc = sanitize_range(ct3d, san_info->dpa_range_list[i].starting_dpa,
> > > > +                san_info->dpa_range_list[i].length, san_info->fill_value);
> > > > +        if (rc) {
> > > > +            goto exit;
> > > > +        }
> > > > +    }
> > > > +    cxl_discard_all_event_records(&ct3d->cxl_dstate);  
> > > 
> > > Add a comment on why we are deleting event records when sanitizing a small
> > > part of memory?
> > >   
> > 
> > See response below for disabling the media state. Same section referenced
> > below, 8.2.10.9.5.1 states all event logs should be deleted. Outcome
> > depends on how we interpret "follow the method described in 8.2.10.9.5.1".
> > 
> 
> This also sounds like reading too much into that comment.
>

Agreed, Vinayak let's drop the discard of all event records
from this patch.

> > > > +    }
> > > > +
> > > > +start_bg:
> > > > +    /* EBUSY other bg cmds as of now */
> > > > +    cci->bg.runtime = secs * 1000UL;
> > > > +    *len_out = 0;
> > > > +    /* sanitize when done */
> > > > +    cxl_dev_disable_media(&ct3d->cxl_dstate);  
> > > Why?  This is santizing part of the device. As I undestand it the
> > > main aim is to offload cleanup when the device is in use. Definitely
> > > don't want to disable media.  If I'm missing a reason please give
> > > a spec reference.  
> > 
> > Table 8-164, sanitize description mentions to follow method
> > in 8.2.10.9.5.1, which does call out placing device in disabled
> > state, but I'm not sure if method refers to all text in 8.2.10.9.5.1
> > or the method devices uses to sanitize internally.
> 
> I think it is meant to just be the method of sanitizing. 
> 

Ok, let's use this interpretation. Vinayak, can you remove this as well
and then we put a comment in the patch that media op sanitize is targeted 
so no need to disable media or clear event logs.


> > 
> > I would imagine since sanitize is destructive we would not want to return 
> > any data from device ranges impacted by sanitize. I believe a simple
> > way to achieve this is to disable entire device. 
> 
> Hmm.  That rather destroys the main use case I'm aware of for this
> (unlike the general santize commands from earlier CXL versions)/
> Superficially sounds like we need a spec clarification as
> clearly not super clear!
> 

For this series, let's drive the work with the use case you have in
mind. We will start a thread with the consortium, but I don't think
that should delay this work.

> > 
> 
> Jonathan
> 
>