| Message ID | 20220516204913.542894-36-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,v2,01/86] virtio: fix feature negotiation for ACCESS_PLATFORM | expand |
On Mon, 16 May 2022 at 21:52, Michael S. Tsirkin <mst@redhat.com> wrote: > > From: Jonathan Cameron <jonathan.cameron@huawei.com> > > These memops perform interleave decoding, walking down the > CXL topology from CFMWS described host interleave > decoder via CXL host bridge HDM decoders, through the CXL > root ports and finally call CXL type 3 specific read and write > functions. > > Note that, whilst functional the current implementation does > not support: > * switches > * multiple HDM decoders at a given level. > * unaligned accesses across the interleave boundaries Hi; Coverity reports a bug in this code (CID 1488873): > +/* TODO: support, multiple hdm decoders */ > +static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr, > + uint8_t *target) > +{ > + uint32_t ctrl; > + uint32_t ig_enc; > + uint32_t iw_enc; > + uint32_t target_reg; target_reg is 32 bits... > + uint32_t target_idx; > + > + ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL]; > + if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) { > + return false; > + } > + > + ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG); > + iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW); > + target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc); > + > + if (target_idx > 4) { ...in this half of the if() target_idx is at least 5... > + target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO]; > + target_reg >>= target_idx * 8; ...but 5 * 8 is 40, so this shift will always be undefined behaviour. Was the if() condition intended to be "< 4" ? > + } else { > + target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO]; Was this (or the other one) intended to be ...LIST_HI ? > + target_reg >>= (target_idx - 4) * 8; Not noticed by Coverity, but in this half of the if(), target_idx is 4 or less, so (target_idx - 4) is in most cases going to be negative, which isn't a valid shift amount. This also suggests the if() condition is wrong. > + } > + *target = target_reg & 0xff; > + > + return true; > +} What's the intended behaviour here ? The code appears to be attempting to extract a particular subfield from one or other of the cache_mem[] values. I would recommend using extract32() for this rather than raw shift and mask operations -- it's generally easier to write and to review. thanks -- PMM
On Wed, 20 Jul 2022 13:23:10 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 16 May 2022 at 21:52, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > From: Jonathan Cameron <jonathan.cameron@huawei.com> > > > > These memops perform interleave decoding, walking down the > > CXL topology from CFMWS described host interleave > > decoder via CXL host bridge HDM decoders, through the CXL > > root ports and finally call CXL type 3 specific read and write > > functions. > > > > Note that, whilst functional the current implementation does > > not support: > > * switches > > * multiple HDM decoders at a given level. > > * unaligned accesses across the interleave boundaries > > Hi; Coverity reports a bug in this code (CID 1488873): Huh. This looks to be one of those cases where a bunch of different bugs ended up with something that appears to work. > > > +/* TODO: support, multiple hdm decoders */ > > +static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr, > > + uint8_t *target) > > +{ > > + uint32_t ctrl; > > + uint32_t ig_enc; > > + uint32_t iw_enc; > > + uint32_t target_reg; > > target_reg is 32 bits... > > > + uint32_t target_idx; > > + > > + ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL]; > > + if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) { > > + return false; > > + } > > + > > + ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG); > > + iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW); > > + target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc); > > + > > + if (target_idx > 4) { > > ...in this half of the if() target_idx is at least 5... > > > + target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO]; > > + target_reg >>= target_idx * 8; > > ...but 5 * 8 is 40, so this shift will always be undefined > behaviour. Was the if() condition intended to be "< 4" ? Spot on. > > > + } else { > > + target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO]; > > Was this (or the other one) intended to be ...LIST_HI ? > Also correct. > > + target_reg >>= (target_idx - 4) * 8; > > Not noticed by Coverity, but in this half of the if(), > target_idx is 4 or less, so (target_idx - 4) is in most > cases going to be negative, which isn't a valid shift amount. > This also suggests the if() condition is wrong. > > > + } > > + *target = target_reg & 0xff; > > + > > + return true; > > +} > > What's the intended behaviour here ? > > The code appears to be attempting to extract a particular > subfield from one or other of the cache_mem[] values. The two registers each have 4 target port numbers stored in them. This was supposed to get the right one. Tests weren't exercising greater than 4 as that requires a very flat hierarchy and doesn't exercise some other paths as can't interleave so wide at multiple levels. Oh well, clearly need an extra test. > I would > recommend using extract32() for this rather than raw shift > and mask operations -- it's generally easier to write and > to review. Will do. Unfortunately I've run into an issue testing with a single host bridge and 8 root ports. Need to pin down if it is a kernel or qemu issue before sending out this fix. I'm away for the next week though so may have to get back to this after returning. Sorry for the delay. Thanks, Jonathan > > thanks > -- PMM
diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h index dce38124db..21d28ca110 100644 --- a/include/hw/cxl/cxl.h +++ b/include/hw/cxl/cxl.h @@ -56,4 +56,6 @@ void cxl_fixed_memory_window_config(MachineState *ms, Error **errp); void cxl_fixed_memory_window_link_targets(Error **errp); +extern const MemoryRegionOps cfmws_ops; + #endif diff --git a/hw/cxl/cxl-host-stubs.c b/hw/cxl/cxl-host-stubs.c index f8fd278d5d..24465a52ab 100644 --- a/hw/cxl/cxl-host-stubs.c +++ b/hw/cxl/cxl-host-stubs.c @@ -12,3 +12,5 @@ void cxl_fixed_memory_window_config(MachineState *ms, Error **errp) {}; void cxl_fixed_memory_window_link_targets(Error **errp) {}; + +const MemoryRegionOps cfmws_ops; diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c index ec5a75cbf5..469b3c4ced 100644 --- a/hw/cxl/cxl-host.c +++ b/hw/cxl/cxl-host.c @@ -15,6 +15,10 @@ #include "qapi/qapi-visit-machine.h" #include "hw/cxl/cxl.h" +#include "hw/pci/pci_bus.h" +#include "hw/pci/pci_bridge.h" +#include "hw/pci/pci_host.h" +#include "hw/pci/pcie_port.h" void cxl_fixed_memory_window_config(MachineState *ms, CXLFixedMemoryWindowOptions *object, @@ -92,3 +96,127 @@ void cxl_fixed_memory_window_link_targets(Error **errp) } } } + +/* TODO: support, multiple hdm decoders */ +static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr, + uint8_t *target) +{ + uint32_t ctrl; + uint32_t ig_enc; + uint32_t iw_enc; + uint32_t target_reg; + uint32_t target_idx; + + ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL]; + if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) { + return false; + } + + ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG); + iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW); + target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc); + + if (target_idx > 4) { + target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO]; + target_reg >>= target_idx * 8; + } else { + target_reg = cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO]; + target_reg >>= (target_idx - 4) * 8; + } + *target = target_reg & 0xff; + + return true; +} + +static PCIDevice *cxl_cfmws_find_device(CXLFixedWindow *fw, hwaddr addr) +{ + CXLComponentState *hb_cstate; + PCIHostState *hb; + int rb_index; + uint32_t *cache_mem; + uint8_t target; + bool target_found; + PCIDevice *rp, *d; + + /* Address is relative to memory region. Convert to HPA */ + addr += fw->base; + + rb_index = (addr / cxl_decode_ig(fw->enc_int_gran)) % fw->num_targets; + hb = PCI_HOST_BRIDGE(fw->target_hbs[rb_index]->cxl.cxl_host_bridge); + if (!hb || !hb->bus || !pci_bus_is_cxl(hb->bus)) { + return NULL; + } + + hb_cstate = cxl_get_hb_cstate(hb); + if (!hb_cstate) { + return NULL; + } + + cache_mem = hb_cstate->crb.cache_mem_registers; + + target_found = cxl_hdm_find_target(cache_mem, addr, &target); + if (!target_found) { + return NULL; + } + + rp = pcie_find_port_by_pn(hb->bus, target); + if (!rp) { + return NULL; + } + + d = pci_bridge_get_sec_bus(PCI_BRIDGE(rp))->devices[0]; + + if (!d || !object_dynamic_cast(OBJECT(d), TYPE_CXL_TYPE3)) { + return NULL; + } + + return d; +} + +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; + /* Reads to invalid address return poison */ + return MEMTX_ERROR; + } + + return cxl_type3_read(d, addr + fw->base, data, size, attrs); +} + +static MemTxResult cxl_write_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) { + /* Writes to invalid address are silent */ + return MEMTX_OK; + } + + return cxl_type3_write(d, addr + fw->base, data, size, attrs); +} + +const MemoryRegionOps cfmws_ops = { + .read_with_attrs = cxl_read_cfmws, + .write_with_attrs = cxl_write_cfmws, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 8, + .unaligned = true, + }, + .impl = { + .min_access_size = 1, + .max_access_size = 8, + .unaligned = true, + }, +};