Re: [PATCH v9 33/45] cxl/cxl-host: Add memops for CFMWS region.

[Jonathan Cameron]

On Thu, 7 Apr 2022 21:07:06 +0000
Tong Zhang <t.zhang2@samsung.com> wrote:

> On 4/4/22 08:14, Jonathan Cameron wrote:
> > From: Jonathan Cameron <jonathan.cameron@huawei.com>
> >
> >
> > +static MemTxResult cxl_read_cfmws(void *opaque, hwaddr addr, uint64_t 
> > *data,
> > +                                  unsigned size, MemTxAttrs attrs)
> > +{
> > +    CXLFixedWindow *fw = opaque;
> > +    PCIDevice *d;
> > +
> > +    d = cxl_cfmws_find_device(fw, addr);
> > +    if (d == NULL) {
> > +        *data = 0;  
> 
> I'm looking at this code and comparing it to CXL2.0 spec 8.2.5.12.2 CXL HDM
> 
> Decoder Global Control Register (Offset 04h) table. It seems that we should
> 
> check POSION_ON_ERR_EN bit, if this bit is set, we return poison, otherwise
> 
> should return all 1's data.

Good point.  Takes a bit of searching to find the statements on that, but
it should indeed by all 1s not all 0s. I'll fix that up.

> 
> Also, from the spec, this bit is implementation specific and hard 
> wired(RO) to either 1 or 0,

My temptation is to set that to 0 and not return poison, because the handling
of that in the host is horribly implementation specific.

> 
> but for type3 device looks like we are currently allowing it to be 
> overwritten in ct3d_reg_write()
> 
> function. We probably also need more sanitation in ct3d_reg_write. (Also 
> for HDM
> 
> range/interleaving settings.)

Absolutely agree. Generally my plan was to tighten up write restrictions
as a follow on series because it tends to require quite a lot of code and
makes it much harder to see the overall flow.

So far I've done most of the PCI config space santization (see the gitlab
tree) but not much yet on the memory mapped register space.

I'll add it to the todo list. If it turns out this particular case is
reasonably clean I might add it within this series.

Jonathan

> 
> > +        /* Reads to invalid address return poison */
> > +        return MEMTX_ERROR;
> > +    }
> > +
> > +    return cxl_type3_read(d, addr + fw->base, data, size, attrs);
> > +}
> > +  
> 
> - Tong
>