On Tue, 5 May 2009 20:37:56 +0800
"Cui, Dexuan" <dexuan.cui@xxxxxxxxx> wrote:
> dbb8aafa702b8b4f5568e08641d98471fd04e0f8 has a bug:
> The virtual CMD value we get from reg_entry->data is not the proper value
> because reg_entry->data only holds the emulated bits and the
> PCI_COMMAND_IO/PCI_COMMAND_MEMORY bits are not in it.
> Instead, we can use pt_pci_read_config(&ptdev->dev, PCI_COMMAND, 2) to get
> the proper value.
* There were some typo in my comment. I resend it.
Hi Cui,
pt_pci_read_config should not be used to read configuration registers.
pt_pci_read_config emulates access to read the registers from guest
software. Many functions which are not relevant are executed in
pt_pci_read_config. So side effects may occur. Instead, you can use
pc_read_word of libpci just to read configuration registers.
Or, there is another approach. It is that you remove emu_mask from
writable_mask in pt_cmd_reg_write. Then you can get the proper value
from reg_entry->data.
Thanks,
--
Yuji Shimada
>
> We should only update the mapping of the related BAR, NOT the mappings of ALL
> BARs.
>
> In pt_exp_rom_bar_reg_write(), we should also update the mapping. And for
> PCI_ROM_SLOT, when the PCI_ROM_ADDRESS_ENABLE bit is 0, we should not have
> the mapping.
>
> Signed-off-by: Dexuan Cui <dexuan.cui@xxxxxxxxx>
>
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 6a53137..d2bed51 100644
> --- a/hw/pass-through.c
> +++ b/hw/pass-through.c
> @@ -1791,64 +1791,74 @@ out:
> }
>
> /* mapping BAR */
> -static void pt_bar_mapping(struct pt_dev *ptdev, int io_enable, int
> mem_enable)
> +static void pt_bar_mapping_one(struct pt_dev *ptdev, int bar, int io_enable,
> + int mem_enable)
> {
> PCIDevice *dev = (PCIDevice *)&ptdev->dev;
> PCIIORegion *r;
> struct pt_region *base = NULL;
> uint32_t r_size = 0, r_addr = -1;
> int ret = 0;
> - int i;
>
> - for (i=0; i<PCI_NUM_REGIONS; i++)
> - {
> - r = &dev->io_regions[i];
> + r = &dev->io_regions[bar];
>
> - /* check valid region */
> - if (!r->size)
> - continue;
> + /* check valid region */
> + if (!r->size)
> + return;
>
> - base = &ptdev->bases[i];
> - /* skip unused BAR or upper 64bit BAR */
> - if ((base->bar_flag == PT_BAR_FLAG_UNUSED) ||
> - (base->bar_flag == PT_BAR_FLAG_UPPER))
> - continue;
> + base = &ptdev->bases[bar];
> + /* skip unused BAR or upper 64bit BAR */
> + if ((base->bar_flag == PT_BAR_FLAG_UNUSED) ||
> + (base->bar_flag == PT_BAR_FLAG_UPPER))
> + return;
>
> - /* copy region address to temporary */
> - r_addr = r->addr;
> + /* copy region address to temporary */
> + r_addr = r->addr;
>
> - /* need unmapping in case I/O Space or Memory Space disable */
> - if (((base->bar_flag == PT_BAR_FLAG_IO) && !io_enable ) ||
> - ((base->bar_flag == PT_BAR_FLAG_MEM) && !mem_enable ))
> + /* need unmapping in case I/O Space or Memory Space disable */
> + if (((base->bar_flag == PT_BAR_FLAG_IO) && !io_enable ) ||
> + ((base->bar_flag == PT_BAR_FLAG_MEM) && !mem_enable ))
> + r_addr = -1;
> + if ( (bar == PCI_ROM_SLOT) && (r_addr != -1) )
> + {
> + uint32_t rom_reg;
> + rom_reg = pt_pci_read_config(&ptdev->dev, PCI_ROM_ADDRESS, 4);
> + if ( !(rom_reg & PCI_ROM_ADDRESS_ENABLE) )
> r_addr = -1;
> + }
>
> - /* prevent guest software mapping memory resource to 00000000h */
> - if ((base->bar_flag == PT_BAR_FLAG_MEM) && (r_addr == 0))
> - r_addr = -1;
> + /* prevent guest software mapping memory resource to 00000000h */
> + if ((base->bar_flag == PT_BAR_FLAG_MEM) && (r_addr == 0))
> + r_addr = -1;
>
> - /* align resource size (memory type only) */
> - r_size = r->size;
> - PT_GET_EMUL_SIZE(base->bar_flag, r_size);
> -
> - /* check overlapped address */
> - ret = pt_chk_bar_overlap(dev->bus, dev->devfn,
> - r_addr, r_size, r->type);
> - if (ret > 0)
> - PT_LOG("Warning: ptdev[%02x:%02x.%x][Region:%d][Address:%08xh]"
> - "[Size:%08xh] is overlapped.\n", pci_bus_num(dev->bus),
> - (dev->devfn >> 3) & 0x1F, (dev->devfn & 0x7),
> - i, r_addr, r_size);
> -
> - /* check whether we need to update the mapping or not */
> - if (r_addr != ptdev->bases[i].e_physbase)
> - {
> - /* mapping BAR */
> - r->map_func((PCIDevice *)ptdev, i, r_addr,
> - r_size, r->type);
> - }
> + /* align resource size (memory type only) */
> + r_size = r->size;
> + PT_GET_EMUL_SIZE(base->bar_flag, r_size);
> +
> + /* check overlapped address */
> + ret = pt_chk_bar_overlap(dev->bus, dev->devfn,
> + r_addr, r_size, r->type);
> + if (ret > 0)
> + PT_LOG("Warning: ptdev[%02x:%02x.%x][Region:%d][Address:%08xh]"
> + "[Size:%08xh] is overlapped.\n", pci_bus_num(dev->bus),
> + (dev->devfn >> 3) & 0x1F, (dev->devfn & 0x7),
> + bar, r_addr, r_size);
> +
> + /* check whether we need to update the mapping or not */
> + if (r_addr != ptdev->bases[bar].e_physbase)
> + {
> + /* mapping BAR */
> + r->map_func((PCIDevice *)ptdev, bar, r_addr,
> + r_size, r->type);
> }
> +}
>
> - return;
> +static void pt_bar_mapping(struct pt_dev *ptdev, int io_enable, int
> mem_enable)
> +{
> + int i;
> +
> + for (i=0; i<PCI_NUM_REGIONS; i++)
> + pt_bar_mapping_one(ptdev, i, io_enable, mem_enable);
> }
>
> /* check power state transition */
> @@ -3051,6 +3061,7 @@ static int pt_bar_reg_write(struct pt_dev *ptdev,
> uint32_t prev_offset;
> uint32_t r_size = 0;
> int index = 0;
> + uint16_t cmd;
>
> /* get BAR index */
> index = pt_bar_offset_to_index(reg->offset);
> @@ -3170,14 +3181,10 @@ exit:
> *value = PT_MERGE_VALUE(*value, dev_value, throughable_mask);
>
> /* After BAR reg update, we need to remap BAR*/
> - reg_grp_entry = pt_find_reg_grp(ptdev, PCI_COMMAND);
> - if (reg_grp_entry)
> - {
> - reg_entry = pt_find_reg(reg_grp_entry, PCI_COMMAND);
> - if (reg_entry)
> - pt_bar_mapping(ptdev, reg_entry->data & PCI_COMMAND_IO,
> - reg_entry->data & PCI_COMMAND_MEMORY);
> - }
> + cmd = pt_pci_read_config(&ptdev->dev, PCI_COMMAND, 2);
> + pt_bar_mapping_one(ptdev, index, cmd & PCI_COMMAND_IO,
> + cmd & PCI_COMMAND_MEMORY);
> +
> return 0;
> }
>
> @@ -3195,6 +3202,7 @@ static int pt_exp_rom_bar_reg_write(struct pt_dev
> *ptdev,
> uint32_t r_size = 0;
> uint32_t bar_emu_mask = 0;
> uint32_t bar_ro_mask = 0;
> + uint16_t cmd;
>
> r = &d->io_regions[PCI_ROM_SLOT];
> r_size = r->size;
> @@ -3217,6 +3225,11 @@ static int pt_exp_rom_bar_reg_write(struct pt_dev
> *ptdev,
> throughable_mask = ~bar_emu_mask & valid_mask;
> *value = PT_MERGE_VALUE(*value, dev_value, throughable_mask);
>
> + /* After BAR reg update, we need to remap BAR*/
> + cmd = pt_pci_read_config(&ptdev->dev, PCI_COMMAND, 2);
> + pt_bar_mapping_one(ptdev, PCI_ROM_SLOT, cmd & PCI_COMMAND_IO,
> + cmd & PCI_COMMAND_MEMORY);
> +
> return 0;
> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home