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

["Cui, Dexuan" <dexuan.cui@xxxxxxxxx>]

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@xxxxxxxxxxxxxxx]
> 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@xxxxxxxxx> 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@xxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel