WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH][ioemu] fix PCI bar mapping

To: "Cui, Dexuan" <dexuan.cui@xxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH][ioemu] fix PCI bar mapping
From: Yuji Shimada <shimada-yxb@xxxxxxxxxxxxxxx>
Date: Thu, 07 May 2009 16:40:02 +0900
Cc: "Zhao, Yu" <yu.zhao@xxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, "Ke, Liping" <liping.ke@xxxxxxxxx>
Delivery-date: Thu, 07 May 2009 00:40:41 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <EADF0A36011179459010BDF5142A45751555F260@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <EADF0A36011179459010BDF5142A45751555F260@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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