Re: [PATCH v1 05/10] xen/pt: Remove XenPTReg->data field.

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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 <konrad.wilk@oracle.com>
> ---
>  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
>