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.
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@intel.com>
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@lists.xensource.com
http://lists.xensource.com/xen-devel
On Tue, 5 May 2009 20:37:56 +0800 "Cui, Dexuan" <dexuan.cui@intel.com> 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.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_pc_read_condign. 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_CD_reg_write. Then you can get the proper value from reg_entry->date. 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@intel.com> > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, 5 May 2009 20:37:56 +0800 "Cui, Dexuan" <dexuan.cui@intel.com> 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@intel.com> > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
Yuji Shimada writes ("Re: [PATCH][ioemu] fix PCI bar
mapping"):> 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.
Should we be reverting Cui''s patch and fix the problem some other
way ?
> 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.
It would be nice to get this sorted for the 3.4 release, which is
imminent ...
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Hi Yuji, Thanks a lot for the comments!> 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.I prefer this approach. Attached is the patch. Could you help to have a review? Thanks, -- Dexuan -----Original Message----- From: Yuji Shimada [mailto:shimada-yxb@necst.nec.co.jp] Sent: 2009年5月7日 15:40 To: Cui, Dexuan; Ian Jackson Cc: Ke, Liping; Zhao, Yu; xen-devel Subject: Re: [PATCH][ioemu] fix PCI bar mapping On Tue, 5 May 2009 20:37:56 +0800 "Cui, Dexuan" <dexuan.cui@intel.com> 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@intel.com> > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson wrote:> Yuji Shimada writes ("Re: [PATCH][ioemu] fix PCI bar mapping"): >> 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. > > Should we be reverting Cui''s patch and fix the problem some other > way ?I have posted a new patch according to Yuji''s comment.> >> 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.Please help to review my newly-posted patch for this approach.> > It would be nice to get this sorted for the 3.4 release, which is > imminent ... > > Ian.Thanks, -- Dexuan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 7 May 2009 20:07:53 +0800 "Cui, Dexuan" <dexuan.cui@intel.com> wrote:> Hi Yuji, > Thanks a lot for the comments! > > > 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. > I prefer this approach. > Attached is the patch. Could you help to have a review? > > Thanks, > -- Dexuan >Hi Cui, Thanks for sending the patch. The patch seems to be good. Thanks, -- Yuji Shimada _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, May 05, 2009 at 08:37:56PM +0800, Cui, Dexuan 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. > > 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@intel.com>Hi, this patch also seems to resolve the problem that I was seeing where Intel NICs were not usable. Tested-by: Simon Horman <horms@verge.net.au> -- Simon Horman VA Linux Systems Japan K.K. Satellite Lab in Sydney, Australia H: www.vergenet.net/~horms/ W: www.valinux.co.jp/en _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Simon, thanks for helping testing it! Thanks, -- Dexuan -----Original Message----- From: Simon Horman [mailto:horms@verge.net.au] Sent: 2009年5月8日 10:06 To: Cui, Dexuan Cc: Ian Jackson; Yuji Shimada; Zhao, Yu; xen-devel; Ke, Liping Subject: Re: [Xen-devel] [PATCH][ioemu] fix PCI bar mapping On Tue, May 05, 2009 at 08:37:56PM +0800, Cui, Dexuan 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. > > 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@intel.com>Hi, this patch also seems to resolve the problem that I was seeing where Intel NICs were not usable. Tested-by: Simon Horman <horms@verge.net.au> -- Simon Horman VA Linux Systems Japan K.K. Satellite Lab in Sydney, Australia H: www.vergenet.net/~horms/ W: www.valinux.co.jp/en _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Thanks a lot for Yuji''s review! Anyway, looks this new patch doesn''t handle properly the case of SR/IOV VF. I''m improving this and will send out a new patch ASAP. Sorry. Thanks, -- Dexuan -----Original Message----- From: Yuji Shimada [mailto:shimada-yxb@necst.nec.co.jp] Sent: 2009年5月8日 8:47 To: Cui, Dexuan; Ian Jackson Cc: Ke, Liping; Zhao, Yu; xen-devel Subject: Re: [PATCH][ioemu] fix PCI bar mapping On Thu, 7 May 2009 20:07:53 +0800 "Cui, Dexuan" <dexuan.cui@intel.com> wrote:> Hi Yuji, > Thanks a lot for the comments! > > > 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. > I prefer this approach. > Attached is the patch. Could you help to have a review? > > Thanks, > -- Dexuan >Hi Cui, Thanks for sending the patch. The patch seems to be good. Thanks, -- Yuji Shimada _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Cui, Dexuan wrote:> Thanks a lot for Yuji''s review! > > Anyway, looks this new patch doesn''t handle properly the case of > SR/IOV VF. I''m improving this and will send out a new patch ASAP. > Sorry. > > Thanks, > -- Dexuan > > -----Original Message----- > From: Yuji Shimada [mailto:shimada-yxb@necst.nec.co.jp] > Sent: 2009年5月8日 8:47 > To: Cui, Dexuan; Ian Jackson > Cc: Ke, Liping; Zhao, Yu; xen-devel > Subject: Re: [PATCH][ioemu] fix PCI bar mapping > > On Thu, 7 May 2009 20:07:53 +0800 > "Cui, Dexuan" <dexuan.cui@intel.com> wrote: > >> Hi Yuji, >> Thanks a lot for the comments! >> >>> 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. >> I prefer this approach. >> Attached is the patch. Could you help to have a review? >> >> Thanks, >> -- Dexuan >> > > Hi Cui, > > Thanks for sending the patch. > > The patch seems to be good. > > Thanks,Hi all, Sorry, my previous mail turns out to be a false alarm. :-) Actually SR-IOV VF can still work fine with the patch. I also make some other tests and it works fine. So, Ian, please apply the pach. Let me paste the patch below for your convenience: -------------------------------------- passthrough: pt_bar_mapping: use a better way to get the CMD value The pt_pci_read_config(&ptdev->dev, PCI_COMMAND, 2) in 5d767b7b3fac52336f59e5b40d8befa6b1909937 is not proper as Yuji Shimada points out: "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, we can "remove emu_mask from writable_mask in pt_cmd_reg_write and then we can get the proper value from reg_entry->data". Thanks for Yuji''s review and Simon Horman''s test. Signed-off-by: Dexuan Cui <dexuan.cui@intel.com> diff --git a/hw/pass-through.c b/hw/pass-through.c index d2bed51..51a39db 100644 --- a/hw/pass-through.c +++ b/hw/pass-through.c @@ -1796,6 +1796,8 @@ static void pt_bar_mapping_one(struct pt_dev *ptdev, int bar, int io_enable, { PCIDevice *dev = (PCIDevice *)&ptdev->dev; PCIIORegion *r; + struct pt_reg_grp_tbl *reg_grp_entry = NULL; + struct pt_reg_tbl *reg_entry = NULL; struct pt_region *base = NULL; uint32_t r_size = 0, r_addr = -1; int ret = 0; @@ -1821,10 +1823,13 @@ static void pt_bar_mapping_one(struct pt_dev *ptdev, int bar, int io_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; + reg_grp_entry = pt_find_reg_grp(ptdev, PCI_ROM_ADDRESS); + if (reg_grp_entry) + { + reg_entry = pt_find_reg(reg_grp_entry, PCI_ROM_ADDRESS); + if (reg_entry && !(reg_entry->data & PCI_ROM_ADDRESS_ENABLE)) + r_addr = -1; + } } /* prevent guest software mapping memory resource to 00000000h */ @@ -3011,7 +3016,7 @@ static int pt_cmd_reg_write(struct pt_dev *ptdev, emu_mask |= PCI_COMMAND_MEMORY; /* modify emulate register */ - writable_mask = emu_mask & ~reg->ro_mask & valid_mask; + writable_mask = ~reg->ro_mask & valid_mask; cfg_entry->data = PT_MERGE_VALUE(*value, cfg_entry->data, writable_mask); /* create value for writing to I/O device register */ @@ -3061,7 +3066,6 @@ 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); @@ -3181,9 +3185,14 @@ exit: *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, index, cmd & PCI_COMMAND_IO, - cmd & PCI_COMMAND_MEMORY); + 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_one(ptdev, index, reg_entry->data & PCI_COMMAND_IO, + reg_entry->data & PCI_COMMAND_MEMORY); + } return 0; } @@ -3194,6 +3203,8 @@ static int pt_exp_rom_bar_reg_write(struct pt_dev *ptdev, uint32_t *value, uint32_t dev_value, uint32_t valid_mask) { struct pt_reg_info_tbl *reg = cfg_entry->reg; + struct pt_reg_grp_tbl *reg_grp_entry = NULL; + struct pt_reg_tbl *reg_entry = NULL; struct pt_region *base = NULL; PCIDevice *d = (PCIDevice *)&ptdev->dev; PCIIORegion *r; @@ -3202,7 +3213,6 @@ 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; @@ -3212,10 +3222,10 @@ static int pt_exp_rom_bar_reg_write(struct pt_dev *ptdev, /* set emulate mask and read-only mask */ bar_emu_mask = reg->emu_mask; - bar_ro_mask = reg->ro_mask | (r_size - 1); + bar_ro_mask = (reg->ro_mask | (r_size - 1)) & ~PCI_ROM_ADDRESS_ENABLE; /* modify emulate register */ - writable_mask = bar_emu_mask & ~bar_ro_mask & valid_mask; + writable_mask = ~bar_ro_mask & valid_mask; cfg_entry->data = PT_MERGE_VALUE(*value, cfg_entry->data, writable_mask); /* update the corresponding virtual region address */ @@ -3226,9 +3236,15 @@ static int pt_exp_rom_bar_reg_write(struct pt_dev *ptdev, *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); + 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_one(ptdev, PCI_ROM_SLOT, + reg_entry->data & PCI_COMMAND_IO, + reg_entry->data & PCI_COMMAND_MEMORY); + } return 0; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel