|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 04/10] xen/pt: Remove XenPTReg->data field.
On Tue, 8 Sep 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.
>
> This change also exposes some mis-use:
> - In 'xen_pt_status_reg_init' we used u32 for the Capabilities Pointer
> register, but said register is an an u16.
> - In 'xen_pt_msgdata_reg_write' we used u32 but should have only use u16.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Reviewed-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 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..4bf9e80 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 *half_word;
> + uint32_t *word;
> + } ptr; /* pointer to dev.config. */
> };
>
> typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 55be4ee..727f814 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.half_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.word;
>
> /* 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.half_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.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);
> @@ -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.half_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.half_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.word,
> ~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.word;
>
> 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.word;
>
> 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.half_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.half_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.word;
> + uint32_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);
> - 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.word;
> + uint32_t *data = cfg_entry->ptr.word;
>
> /* 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.half_word;
> uint32_t offset = reg->offset;
> + uint16_t *data = cfg_entry->ptr.half_word;
>
> /* 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.half_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);
> @@ -1930,8 +1945,8 @@ static int
> xen_pt_config_reg_init(XenPCIPassthroughState *s,
> 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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |