From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v1 05/10] xen/pt: Remove XenPTReg->data field. Date: Fri, 17 Jul 2015 17:30:39 +0100 Message-ID: References: <1435866681-18468-1-git-send-email-konrad.wilk@oracle.com> <1435866681-18468-6-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZG8Y8-0005MA-M4 for xen-devel@lists.xenproject.org; Fri, 17 Jul 2015 16:31:48 +0000 In-Reply-To: <1435866681-18468-6-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: xen-devel@lists.xenproject.org, qemu-devel@nongnu.org, JBeulich@suse.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote: > We do not want to have two entries to cache the guest configuration > registers: XenPTReg->data and dev.config. Instead we want to use > only the dev.config. > > To do without much complications we rip out the ->data field > and replace it with an pointer to the dev.config. This way we > have the type-checking (uint8_t, uint16_t, etc) and as well > and pre-computed location. > > Alternatively we could compute the offset in dev.config by > using the XenPTRRegInfo and XenPTRegGroup every time but > this way we have the pre-computed values. > > Signed-off-by: Konrad Rzeszutek Wilk > --- > hw/xen/xen_pt.h | 6 +++- > hw/xen/xen_pt_config_init.c | 73 +++++++++++++++++++++++++++------------------ > 2 files changed, 49 insertions(+), 30 deletions(-) > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h > index 09358b1..586d055 100644 > --- a/hw/xen/xen_pt.h > +++ b/hw/xen/xen_pt.h > @@ -134,7 +134,11 @@ struct XenPTRegInfo { > struct XenPTReg { > QLIST_ENTRY(XenPTReg) entries; > XenPTRegInfo *reg; > - uint32_t data; /* emulated value */ > + union { > + uint8_t *byte; > + uint16_t *word; > + uint32_t *dbword; > + } ptr; /* pointer to dev.config. */ Nice (nasty?) trick. I would probably have just introduced a single pointer, but this might turn out to be better as it avoids the risk of merging bits that should be ignored. However it should be half-word (uint16_t) and word (uint32_t). > }; > > typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo; > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c > index e597993..0a710a2 100644 > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -128,10 +128,11 @@ static int xen_pt_byte_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > { > XenPTRegInfo *reg = cfg_entry->reg; > uint8_t valid_emu_mask = 0; > + uint8_t *data = cfg_entry->ptr.byte; > > /* emulate byte register */ > valid_emu_mask = reg->emu_mask & valid_mask; > - *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask); > + *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask); > > return 0; > } > @@ -140,10 +141,11 @@ static int xen_pt_word_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > { > XenPTRegInfo *reg = cfg_entry->reg; > uint16_t valid_emu_mask = 0; > + uint16_t *data = cfg_entry->ptr.word; > > /* emulate word register */ > valid_emu_mask = reg->emu_mask & valid_mask; > - *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask); > + *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask); > > return 0; > } > @@ -152,10 +154,11 @@ static int xen_pt_long_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > { > XenPTRegInfo *reg = cfg_entry->reg; > uint32_t valid_emu_mask = 0; > + uint32_t *data = cfg_entry->ptr.dbword; > > /* emulate long register */ > valid_emu_mask = reg->emu_mask & valid_mask; > - *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask); > + *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask); > > return 0; > } > @@ -169,10 +172,11 @@ static int xen_pt_byte_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > XenPTRegInfo *reg = cfg_entry->reg; > uint8_t writable_mask = 0; > uint8_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > + uint8_t *data = cfg_entry->ptr.byte; > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask); > @@ -186,10 +190,11 @@ static int xen_pt_word_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > XenPTRegInfo *reg = cfg_entry->reg; > uint16_t writable_mask = 0; > uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > + uint16_t *data = cfg_entry->ptr.word; > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask); > @@ -203,10 +208,11 @@ static int xen_pt_long_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > XenPTRegInfo *reg = cfg_entry->reg; > uint32_t writable_mask = 0; > uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > + uint32_t *data = cfg_entry->ptr.dbword; > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask); > @@ -255,7 +261,7 @@ static int xen_pt_status_reg_init(XenPCIPassthroughState *s, > reg_entry = xen_pt_find_reg(reg_grp_entry, PCI_CAPABILITY_LIST); > if (reg_entry) { > /* check Capabilities Pointer register */ > - if (reg_entry->data) { > + if (*reg_entry->ptr.word) { > reg_field |= PCI_STATUS_CAP_LIST; > } else { > reg_field &= ~PCI_STATUS_CAP_LIST; > @@ -301,10 +307,11 @@ static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > XenPTRegInfo *reg = cfg_entry->reg; > uint16_t writable_mask = 0; > uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > + uint16_t *data = cfg_entry->ptr.word; > > /* modify emulate register */ > writable_mask = ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > if (*val & PCI_COMMAND_INTX_DISABLE) { > @@ -447,7 +454,7 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > > /* emulate BAR */ > valid_emu_mask = bar_emu_mask & valid_mask; > - *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask); > + *value = XEN_PT_MERGE_VALUE(*value, *cfg_entry->ptr.dbword, ~valid_emu_mask); > > return 0; > } > @@ -464,6 +471,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > uint32_t bar_ro_mask = 0; > uint32_t r_size = 0; > int index = 0; > + uint32_t *data = cfg_entry->ptr.dbword; > > index = xen_pt_bar_offset_to_index(reg->offset); > if (index < 0 || index >= PCI_NUM_REGIONS) { > @@ -500,7 +508,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry, > > /* modify emulate register */ > writable_mask = bar_emu_mask & ~bar_ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* check whether we need to update the virtual region address or not */ > switch (s->bases[index].bar_flag) { > @@ -533,6 +541,7 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s, > uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > pcibus_t r_size = 0; > uint32_t bar_ro_mask = 0; > + uint32_t *data = cfg_entry->ptr.dbword; > > r_size = d->io_regions[PCI_ROM_SLOT].size; > base = &s->bases[PCI_ROM_SLOT]; > @@ -544,7 +553,7 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s, > > /* modify emulate register */ > writable_mask = ~bar_ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask); > @@ -983,10 +992,11 @@ static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s, > XenPTRegInfo *reg = cfg_entry->reg; > uint16_t writable_mask = 0; > uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > + uint16_t *data = cfg_entry->ptr.word; > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS, > @@ -1081,6 +1091,7 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s, > XenPTMSI *msi = s->msi; > uint16_t writable_mask = 0; > uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > + uint16_t *data = cfg_entry->ptr.word; > > /* Currently no support for multi-vector */ > if (*val & PCI_MSI_FLAGS_QSIZE) { > @@ -1089,8 +1100,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s, > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > - msi->flags |= cfg_entry->data & ~PCI_MSI_FLAGS_ENABLE; > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > + msi->flags |= *data & ~PCI_MSI_FLAGS_ENABLE; > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask); > @@ -1204,18 +1215,19 @@ static int xen_pt_msgaddr32_reg_write(XenPCIPassthroughState *s, > { > XenPTRegInfo *reg = cfg_entry->reg; > uint32_t writable_mask = 0; > - uint32_t old_addr = cfg_entry->data; > + uint32_t old_addr = *cfg_entry->ptr.dbword; > + uint32_t *data = cfg_entry->ptr.dbword; > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > - s->msi->addr_lo = cfg_entry->data; > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > + s->msi->addr_lo = *data; > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0); > > /* update MSI */ > - if (cfg_entry->data != old_addr) { > + if (*data != old_addr) { > if (s->msi->mapped) { > xen_pt_msi_update(s); > } > @@ -1230,7 +1242,8 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s, > { > XenPTRegInfo *reg = cfg_entry->reg; > uint32_t writable_mask = 0; > - uint32_t old_addr = cfg_entry->data; > + uint32_t old_addr = *cfg_entry->ptr.dbword; > + uint32_t *data = cfg_entry->ptr.dbword; > > /* check whether the type is 64 bit or not */ > if (!(s->msi->flags & PCI_MSI_FLAGS_64BIT)) { > @@ -1241,15 +1254,15 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s, > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > /* update the msi_info too */ > - s->msi->addr_hi = cfg_entry->data; > + s->msi->addr_hi = *data; > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0); > > /* update MSI */ > - if (cfg_entry->data != old_addr) { > + if (*data != old_addr) { > if (s->msi->mapped) { > xen_pt_msi_update(s); > } > @@ -1268,8 +1281,9 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s, > XenPTRegInfo *reg = cfg_entry->reg; > XenPTMSI *msi = s->msi; > uint16_t writable_mask = 0; > - uint16_t old_data = cfg_entry->data; > + uint16_t old_data = *cfg_entry->ptr.dbword; > uint32_t offset = reg->offset; > + uint32_t *data = cfg_entry->ptr.dbword; > > /* check the offset whether matches the type or not */ > if (!xen_pt_msi_check_type(offset, msi->flags, DATA)) { > @@ -1280,15 +1294,15 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s, > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > /* update the msi_info too */ > - msi->data = cfg_entry->data; > + msi->data = *data; > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0); > > /* update MSI */ > - if (cfg_entry->data != old_data) { > + if (*data != old_data) { > if (msi->mapped) { > xen_pt_msi_update(s); > } > @@ -1452,10 +1466,11 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s, > uint16_t writable_mask = 0; > uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask); > int debug_msix_enabled_old; > + uint16_t *data = cfg_entry->ptr.word; > > /* modify emulate register */ > writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask; > - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask); > + *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask); > > /* create value for writing to I/O device register */ > *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask); > @@ -1924,8 +1939,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s, > case 4: pci_set_long(s->dev.config + offset, val); break; > default: assert(1); > } > - /* set register value */ > - reg_entry->data = val; > + /* set register value pointer to the data. */ > + reg_entry->ptr.byte = s->dev.config + offset; > > } > /* list add register entry */ > -- > 2.1.0 >