Re: [PATCH v5 06/13] hw/mem/cxl_type3: Add host backend and address space handling for DC regions

[fan <nifan.cxl@gmail.com>]

On Thu, Mar 07, 2024 at 12:16:05PM +0000, Jonathan Cameron wrote:
> > > > @@ -868,16 +974,24 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> > > >                                         AddressSpace **as,
> > > >                                         uint64_t *dpa_offset)
> > > >  {
> > > > -    MemoryRegion *vmr = NULL, *pmr = NULL;
> > > > +    MemoryRegion *vmr = NULL, *pmr = NULL, *dc_mr = NULL;
> > > > +    uint64_t vmr_size = 0, pmr_size = 0, dc_size = 0;
> > > >  
> > > >      if (ct3d->hostvmem) {
> > > >          vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> > > > +        vmr_size = memory_region_size(vmr);
> > > >      }
> > > >      if (ct3d->hostpmem) {
> > > >          pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> > > > +        pmr_size = memory_region_size(pmr);
> > > > +    }
> > > > +    if (ct3d->dc.host_dc) {
> > > > +        dc_mr = host_memory_backend_get_memory(ct3d->dc.host_dc);
> > > > +        /* Do we want dc_size to be dc_mr->size or not?? */  
> > > 
> > > Maybe - definitely don't want to leave this comment here
> > > unanswered and I think you enforce it above anyway.
> > > 
> > > So if we get here ct3d->dc.total_capacity == memory_region_size(ct3d->dc.host_dc);
> > > As such we could just not stash total_capacity at all?  
> > 
> > I cannot identify a case where these two will be different. But
> > total_capacity is referenced at quite some places, it may be nice to have
> > it so we do not need to call the function to get the value every time?
> 
> I kind of like having it via one path so that there is no confusion
> for the reader, but up to you on this one.  The function called is trivial
> (other than some magic to handle very large memory regions) so
> this is just a readability question, not a perf one.
> 
> Whatever, don't leave the question behind.  Find to have something
> that says they are always the same size if you don't get rid
> of the total_capacity representation.
> 
I will fix it.

For static capability, we have a variable static_mem_size, although we
can calculate it from volatile and non-volatile memory region size.
There are quite some places need to get the dynamic capacity, it is much
more convenient to have a variable ready to use, I will keep it for
now.

Fan
> 
> Jonathan