Re: [PATCH v6 09/12] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents

[Jonathan Cameron]

> > >  
> > >      ret = cxl_detect_malformed_extent_list(ct3d, in);
> > >      if (ret != CXL_MBOX_SUCCESS) {
> > > +        cxl_extent_group_list_delete_front(&ct3d->dc.extents_pending);  
> > 
> > If it's a bad message from the host, I don't think the device is supposed to
> > do anything with pending extents.  
> 
> It is not clear to me here. 
> 
> In the spec r3.1 8.2.9.9.9.3, Add Dynamic Capacity Response (Opcode 4802h),
> there is text like "After this command is received, the device is free to
> reclaim capacity that the host does not utilize.", that seems to imply
> as long as the response is received, we need to update the pending list
> so the capacity unused can be reclaimed. But of course, we can say if
> there is error, we cannot tell whether the host accepts the extents or
> not so not update the pending list.

Can try and get a clarification as I agree 'is received' is unclear,
but in general any command that gets an error response should have no
affect on device state. If it does, then what affect it has must be stated
in the specification.

> >   
> > > +
> > > +#define REMOVAL_POLICY_MASK 0xf
> > > +#define FORCED_REMOVAL_BIT BIT(4)
> > > +
> > > +void qmp_cxl_release_dynamic_capacity(const char *path, uint16_t hid,
> > > +                                      uint8_t flags, uint8_t region_id,
> > > +                                      const char *tag,
> > > +                                      CXLDCExtentRecordList  *records,
> > > +                                      Error **errp)
> > > +{
> > > +    CXLDCEventType type = DC_EVENT_RELEASE_CAPACITY;
> > > +
> > > +    if (flags & FORCED_REMOVAL_BIT) {
> > > +        /* TODO: enable forced removal in the future */
> > > +        type = DC_EVENT_FORCED_RELEASE_CAPACITY;
> > > +        error_setg(errp, "Forced removal not supported yet");
> > > +        return;
> > > +    }
> > > +
> > > +    switch (flags & REMOVAL_POLICY_MASK) {
> > > +    case 1:  
> > Probably benefit form a suitable define.
> >   
> > > +        qmp_cxl_process_dynamic_capacity_prescriptive(path, hid, type,
> > > +                                                      region_id, 
> > > records, errp);
> > > +        break;  
> > 
> > I'd not noticed before but might as well return from these case blocks.  
> 
> Sorry. I do not follow here. What do you mean by "return from these case
> blocks", are you referring the check above about the forced removal case?

No, what I meant was much simpler - just a code refactoring thing.
        case 1:
            qmp_cxl_process_dynamic_capacity_prescriptive(path, hid, type,
                                                          region_id, records, 
errp);

            //break;
            return;
> 
> Fan
> 
> >   
> > > +    default:
> > > +        error_setg(errp, "Removal policy not supported");
> > > +        break;
               return;
> > > +    }
> > > +}