|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] RE: [PATCH][ioemu] fix PCI bar mapping
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@xxxxxxxxxxxxxxx] 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@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; > } > Attachment:
fix_writable_mask.patch _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |