On Tue, Aug 9, 2016 at 8:44 AM, Peter Xu <pet...@redhat.com> wrote: > On Tue, Aug 02, 2016 at 11:39:06AM +0300, David Kiarie wrote: > > [...] > > Hi Peter.
Most of your comments are valid thought some are subjective :-). I'm covering most if not all of them on next version (should coming shortly). > +/* invalidate internal caches for devid */ > > +typedef struct QEMU_PACKED { > > +#ifdef HOST_WORDS_BIGENDIAN > > + uint64_t devid; /* device to invalidate */ > > + uint64_t reserved_1:44; > > + uint64_t type:4; /* command type */ > > +#else > > + uint64_t devid; > > + uint64_t reserved_1:44; > > + uint64_t type:4; > > +#endif /* __BIG_ENDIAN_BITFIELD */ > > Guess you forgot to reverse the order of fields in one of above block. > > [...] > > > +/* load adddress translation info for devid into translation cache */ > > +typedef struct QEMU_PACKED { > > +#ifdef HOST_WORDS_BIGENDIAN > > + uint64_t type:4; /* command type */ > > + uint64_t reserved_2:8; > > + uint64_t pasid_19_0:20; > > + uint64_t pfcount_7_0:8; > > + uint64_t reserved_1:8; > > + uint64_t devid; /* related devid */ > > +#else > > + uint64_t devid; > > + uint64_t reserved_1:8; > > + uint64_t pfcount_7_0:8; > > + uint64_t pasid_19_0:20; > > + uint64_t reserved_2:8; > > + uint64_t type:4; > > +#endif /* __BIG_ENDIAN_BITFIELD */ > > For this one, "devid" looks like a 16 bits field? > > [...] > > > +/* issue a PCIe completion packet for devid */ > > +typedef struct QEMU_PACKED { > > +#ifdef HOST_WORDS_BIGENDIAN > > + uint32_t devid; /* related devid */ > > + uint32_t reserved_1; > > +#else > > + uint32_t reserved_1; > > + uint32_t devid; > > +#endif /* __BIG_ENDIAN_BITFIELD */ > > Here I am not sure we need this "#ifdef". > > [...] > > > +/* external write */ > > +static void amdvi_writew(AMDVIState *s, hwaddr addr, uint16_t val) > > +{ > > + uint16_t romask = lduw_le_p(&s->romask[addr]); > > + uint16_t w1cmask = lduw_le_p(&s->w1cmask[addr]); > > + uint16_t oldval = lduw_le_p(&s->mmior[addr]); > > + stw_le_p(&s->mmior[addr], (val & ~(val & w1cmask)) | (romask & > oldval)); > > I think the above is problematic, e.g., what if we write 1 to one of > the romask while it's 0 originally? In that case, the RO bit will be > written to 1. > > Maybe we need: > > stw_le_p(&s->mmior[addr], ((oldval & romask) | (val & ~romask)) & \ > (val & w1cmask)); > > Same question to the below two functions. > > > +} > > + > > +static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val) > > +{ > > + uint32_t romask = ldl_le_p(&s->romask[addr]); > > + uint32_t w1cmask = ldl_le_p(&s->w1cmask[addr]); > > + uint32_t oldval = ldl_le_p(&s->mmior[addr]); > > + stl_le_p(&s->mmior[addr], (val & ~(val & w1cmask)) | (romask & > oldval)); > > +} > > + > > +static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val) > > +{ > > + uint64_t romask = ldq_le_p(&s->romask[addr]); > > + uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]); > > + uint32_t oldval = ldq_le_p(&s->mmior[addr]); > > + stq_le_p(&s->mmior[addr], (val & ~(val & w1cmask)) | (romask & > oldval)); > > +} > > + > > +/* OR a 64-bit register with a 64-bit value */ > > +static bool amdvi_orq(AMDVIState *s, hwaddr addr, uint64_t val) > > Nit: This function name gives me an illusion that it's a write op, not > read. IMHO it'll be better we directly use amdvi_readq() for all the > callers of this function, which is more clear to me. > > > +{ > > + return amdvi_readq(s, addr) | val; > > +} > > + > > +/* OR a 64-bit register with a 64-bit value storing result in the > register */ > > +static void amdvi_orassignq(AMDVIState *s, hwaddr addr, uint64_t val) > > +{ > > + amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) | val); > > +} > > + > > +/* AND a 64-bit register with a 64-bit value storing result in the > register */ > > +static void amdvi_and_assignq(AMDVIState *s, hwaddr addr, uint64_t val) > > Nit: the name is not matched with above: > > amdvi_{or|and}assign[qw] > > Though I would prefer: > > amdvi_assign_[qw]_{or|and} > > [...] > > > +static void amdvi_log_event(AMDVIState *s, uint64_t *evt) > > +{ > > + /* event logging not enabled */ > > + if (!s->evtlog_enabled || amdvi_orq(s, AMDVI_MMIO_STATUS, > > + AMDVI_MMIO_STATUS_EVT_OVF)) { > > + return; > > + } > > + > > + /* event log buffer full */ > > + if (s->evtlog_tail >= s->evtlog_len) { > > + amdvi_orassignq(s, AMDVI_MMIO_STATUS, > AMDVI_MMIO_STATUS_EVT_OVF); > > + /* generate interrupt */ > > + amdvi_generate_msi_interrupt(s); > > + return; > > + } > > + > > + if (dma_memory_write(&address_space_memory, s->evtlog_len + > s->evtlog_tail, > > + &evt, AMDVI_EVENT_LEN)) { > > Check with MEMTX_OK? > > [...] > > > +/* > > + * AMDVi event structure > > + * 0:15 -> DeviceID > > + * 55:63 -> event type + miscellaneous info > > + * 64:127 -> related address > > + */ > > +static void amdvi_encode_event(uint64_t *evt, uint16_t devid, uint64_t > addr, > > + uint16_t info) > > +{ > > + amdvi_setevent_bits(evt, devid, 0, 16); > > + amdvi_setevent_bits(evt, info, 55, 8); > > + amdvi_setevent_bits(evt, addr, 63, 64); > ^^ > should here be 64? > > Also, I am not sure whether we need this amdvi_setevent_bits() if it's > only used in this function. Though not a big problem for me. > > > +} > > +/* log an error encountered page-walking > > "during page-walking" > > > + * > > + * @addr: virtual address in translation request > > + */ > > +static void amdvi_page_fault(AMDVIState *s, uint16_t devid, > > + hwaddr addr, uint16_t info) > > +{ > > + uint64_t evt[4]; > > + > > + info |= AMDVI_EVENT_IOPF_I | AMDVI_EVENT_IOPF; > > + amdvi_encode_event(evt, devid, addr, info); > > + amdvi_log_event(s, evt); > > + pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS, > > + PCI_STATUS_SIG_TARGET_ABORT); > > Nit: maybe we can provide a function for setting this bit. > > [...] > > > +static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid, > > + uint64_t gpa, IOMMUTLBEntry to_cache, > > + uint16_t domid) > > +{ > > + AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry)); > > + uint64_t *key = g_malloc(sizeof(key)); > > + uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K; > > + > > + /* don't cache erroneous translations */ > > + if (to_cache.perm != IOMMU_NONE) { > > + trace_amdvi_cache_update(domid, PCI_BUS_NUM(devid), > PCI_SLOT(devid), > > + PCI_FUNC(devid), gpa, to_cache.translated_addr); > > + > > + if (g_hash_table_size(s->iotlb) >= AMDVI_IOTLB_MAX_SIZE) { > > + trace_amdvi_iotlb_reset(); > > We'd better put this trace into amdvi_iotlb_reset(). > > > + amdvi_iotlb_reset(s); > > + } > > + > > + entry->gfn = gfn; > > + entry->domid = domid; > > + entry->perms = to_cache.perm; > > + entry->translated_addr = to_cache.translated_addr; > > + entry->page_mask = to_cache.addr_mask; > > + *key = gfn | ((uint64_t)(devid) << AMDVI_DEVID_SHIFT); > > + g_hash_table_replace(s->iotlb, key, entry); > > + } > > +} > > + > > +static void amdvi_completion_wait(AMDVIState *s, CMDCompletionWait > *wait) > > +{ > > + /* pad the last 3 bits */ > > + hwaddr addr = cpu_to_le64(wait->store_addr << 3); > > Is this correct? IMO it should be: > > hwaddr addr = le64_to_cpu(wait->store_addr) << 3; > > > + uint64_t data = cpu_to_le64(wait->store_data); > > Maybe: > > uint64_t data = le64_to_cpu(wait->store_data); > > ? > > > + > > + if (wait->reserved) { > > + amdvi_log_illegalcom_error(s, wait->type, s->cmdbuf + > s->cmdbuf_head); > > + } > > + > > + if (wait->completion_store) { > > + if (dma_memory_write(&address_space_memory, addr, &data, > > + AMDVI_COMPLETION_DATA_SIZE)) > > + { > > Left bracket is better moved upward to follow the coding style. > > > + trace_amdvi_completion_wait_fail(addr); > > + } > > + } > > Thanks, > > -- peterx >