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)

[Vinayak Holikatti <vinayak.kh@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

On 03/02/25 05:02PM, Adam Manzanares wrote:
>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.

Ok Adam will drop the discard of all event records

>
>> > > > +    }
>> > > > +
>> > > > +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.

ok Adam, will remove this as well and comment as needed

>
>
>> >
>> > 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
>>
>>
Vinayak Holikatti


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]