Re: [RESEND] hw/cxl: fix the determination of illegal physical addresses

[peng guo <engguopeng@buaa.edu.cn>]

On Wed, Sep 04, 2024 at 01:01:55PM -0700, Davidlohr Bueso wrote:
> On Fri, 23 Aug 2024, Jonathan Cameron wrote:\n
> > On Mon, 19 Aug 2024 20:03:17 +0800
> > peng guo <engguopeng@buaa.edu.cn> wrote:
> > 
> > > When physical address range in the input payload of scan media command
> > > exceeds static_mem_size but does not exceed the sum of static and dynamic
> > > memory, the scan media mailbox command unexpectedly returns an error code
> > > which is CXL_MBOX_INVALID_PA.
> > > 
> > > This patch determines whether the physical address is valid in two cases.
> > > If dynamic memory exists, check whether the address range of the request
> > > exceeds the range of static memory and dynamic memory.If dynamic memory
> 							 ^ space for a new sentence
> > > does not exist, then check whether the address range of the request
> > > exceeds the static memory size.
> > > 
> > > Fixes: d61cc5b6a8d3 ("hw/cxl: Add get scan media capabilities cmd support")
> > Is that the right one, this code is affecting cmd_media_scan_media()
> > not the capabilities one which always limits to static_mem_size and
> > hence also looks wrong.
> 
> Yeah it is the right one - both commands were merged together (and
> yes, they both need to be updated for dcd).
> 
> Maybe have a helper to consolidate all cases where such ranges are used?
> 
> uint64_t ct3d_get_total_size(CXLDeviceState *cxl_dstate, CXLType3Dev *ct3d)
> {
> 
> 	return cxl_dstate->static_mem_size + ct3d->dc.total_capacity;
> }
> 
> > 
> > > Signed-off-by: peng guo <engguopeng@buaa.edu.cn>
> > 
> > As with the other patch, this needs to go to qemu-devel list
> > + both should have gone to Davidlohr as author the patch you
> > are fixing (sort of it, it's mostly down to what order patches
> > landed in I think).
> > 
> > Fan, Davidlohr, do we want to just cover the DCD regions as
> > well with all the scan_media commands?
> 
> Yeah, I think so - particularly since poison management already
> considers dcd for CXL_MBOX_INVALID_PA.
> 
> Thanks,
> Davidlohr

Hi Jonathan, Davidlohr,

Sorry for the late reply.

I’m following up on the discussion about the `scan media` command behavior 
when dynamic memory is present. Based on our earlier exchange, it seems 
that the command currently returns `CXL_MBOX_INVALID_PA` if the requested 
address falls within the dynamic memory region — even though this should 
be a valid case when DCD is enabled.

Just to confirm: should this be treated as a bug that needs to be fixed with 
an updated patch?

If so, I’d be happy to prepare a new version and send it to the proper recipients.

Thanks again for your feedback.

Best regards,
Peng Guo

> 
> > 
> > 
> > > ---
> > >  hw/cxl/cxl-mailbox-utils.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > index 3ebbd32e10..b23c6b9b0b 100644
> > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > +++ b/hw/cxl/cxl-mailbox-utils.c
> > > @@ -1943,11 +1943,12 @@ static CXLRetCode cmd_media_scan_media(const struct cxl_cmd *cmd,
> > >      }
> > >      query_length = ldq_le_p(&in->length) * CXL_CACHE_LINE_SIZE;
> > > 
> > > -    if (query_start + query_length > cxl_dstate->static_mem_size) {
> > > -        return CXL_MBOX_INVALID_PA;
> > > -    }
> > > -    if (ct3d->dc.num_regions && query_start + query_length >=
> > > +    if (ct3d->dc.num_regions) {
> > > +        if (query_start + query_length >=
> > >              cxl_dstate->static_mem_size + ct3d->dc.total_capacity) {
> > > +                return CXL_MBOX_INVALID_PA;
> > > +            }
> > > +    } else if (query_start + query_length > cxl_dstate->static_mem_size) {
> > >          return CXL_MBOX_INVALID_PA;
> > >      }
> > Can we not rely on dc.total_capacity == 0 if num_regions == 0/
> > 
> > > 
> >