[PATCH] passthru: Fix pci bar remapping for passthru devices

[PATCH] passthru: Fix pci bar remapping for passthru devices

[Wei Wang2]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

Hi, 
When guest code tries to get the block size of mmio, it will write all "1"s 
into pci bar register and then qemu will return all "0"s to the don't care 
bits in the emulated bar register to indicate the block size to guest code. 
In this case, we should not create p2m mapping in  pt_bar_reg_write() and
pt_exp_rom_bar_reg_write(). Attached patch fixes this issue, additional 
comment can be found in the patch.

Thanks,
Wei

Signed-off-by: Wei Wang <wei.wang2@amd.com>
--
 AMD GmbH, Germany
 Operating System Research Center
 
 Legal Information:
 Advanced Micro Devices GmbH
 Karl-Hammerschmidt-Str. 34
 85609 Dornach b. München
 
 Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
 Sitz: Dornach, Gemeinde Aschheim, Landkreis München
 Registergericht München, HRB Nr. 43632

[-- Attachment #2: mmio.patch --]
[-- Type: text/x-diff, Size: 1710 bytes --]

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 51a39db..009b902 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -3177,7 +3177,16 @@ static int pt_bar_reg_write(struct pt_dev *ptdev,
     }
 
     /* update the corresponding virtual region address */
-    r->addr = cfg_entry->data;
+    /*
+     * When guest code tries to get block size of mmio, it will write all "1"s
+     * into pci bar register. In this case, cfg_entry->data == writable_mask.
+     * Especially for devices with large mmio, the value of writable_mask
+     * is likely to be a guest physical address that has been mapped to ram
+     * rather than mmio. Remapping this value to mmio should be prevented.
+     */
+
+    if ( cfg_entry->data != writable_mask )
+        r->addr = cfg_entry->data;
 
 exit:
     /* create value for writing to I/O device register */
@@ -3229,7 +3238,16 @@ static int pt_exp_rom_bar_reg_write(struct pt_dev *ptdev,
     cfg_entry->data = PT_MERGE_VALUE(*value, cfg_entry->data, writable_mask);
 
     /* update the corresponding virtual region address */
-    r->addr = cfg_entry->data;
+    /*
+     * When guest code tries to get block size of mmio, it will write all "1"s
+     * into pci bar register. In this case, cfg_entry->data == writable_mask.
+     * Especially for devices with large mmio, the value of writable_mask
+     * is likely to be a guest physical address that has been mapped to ram
+     * rather than mmio. Remapping this value to mmio should be prevented.
+     */
+
+    if ( cfg_entry->data != writable_mask )
+        r->addr = cfg_entry->data;
 
     /* create value for writing to I/O device register */
     throughable_mask = ~bar_emu_mask & valid_mask;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Jiang, Yunhong]

I assume guest should disable the MMIO in PCI_COMMAND before writing all "1"s to bar register. Otherwise, what will happen on native if guest try to access 0xFFFFFFF0? 
And if we do update the P2M, will it cause trouble to Xen HV?

Thanks
Yunhong Jiang

xen-devel-bounces@lists.xensource.com wrote:
> Hi,
> When guest code tries to get the block size of mmio, it will write
> all "1"s into pci bar register and then qemu will return all "0"s to
> the don't care
> bits in the emulated bar register to indicate the block size
> to guest code.
> In this case, we should not create p2m mapping in
> pt_bar_reg_write() and
> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
> additional comment can be found in the patch.
> 
> Thanks,
> Wei
> 
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> --
> AMD GmbH, Germany
> Operating System Research Center
> 
> Legal Information:
> Advanced Micro Devices GmbH
> Karl-Hammerschmidt-Str. 34
> 85609 Dornach b. München
> 
> Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
> Sitz: Dornach, Gemeinde Aschheim, Landkreis München
> Registergericht München, HRB Nr. 43632

Re: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Wei Wang2]

Hi Yunhong
My testing shows that Linux guests will try to probe pci bar with MMIO being 
enabled. When I assign a broadcom NIC with 32MB MMIO a Linux guest, guest 
will hang after remapping a guest address "0xfe000000" to physical mmio. 
However, windows and BSD guests do not have this issue. They alway probe mmio 
size after disabling mmio. 
Thanks,
Wei

On Monday 20 July 2009 16:26:00 Jiang, Yunhong wrote:
> I assume guest should disable the MMIO in PCI_COMMAND before writing all
> "1"s to bar register. Otherwise, what will happen on native if guest try to
> access 0xFFFFFFF0? And if we do update the P2M, will it cause trouble to
> Xen HV?
>
> Thanks
> Yunhong Jiang
>
> xen-devel-bounces@lists.xensource.com wrote:
> > Hi,
> > When guest code tries to get the block size of mmio, it will write
> > all "1"s into pci bar register and then qemu will return all "0"s to
> > the don't care
> > bits in the emulated bar register to indicate the block size
> > to guest code.
> > In this case, we should not create p2m mapping in
> > pt_bar_reg_write() and
> > pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
> > additional comment can be found in the patch.
> >
> > Thanks,
> > Wei
> >
> > Signed-off-by: Wei Wang <wei.wang2@amd.com>
> > --
> > AMD GmbH, Germany
> > Operating System Research Center
> >
> > Legal Information:
> > Advanced Micro Devices GmbH
> > Karl-Hammerschmidt-Str. 34
> > 85609 Dornach b. München
> >
> > Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
> > Sitz: Dornach, Gemeinde Aschheim, Landkreis München
> > Registergericht München, HRB Nr. 43632

RE: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Jiang, Yunhong]

Wei Wang2 wrote:
> Hi Yunhong
> My testing shows that Linux guests will try to probe pci bar with
> MMIO being enabled. When I assign a broadcom NIC with 32MB MMIO a
> Linux guest, guest will hang after remapping a guest address

Yes, I remember I saw this bug in kernel before.

I suspect the issue here is the local APIC address, which should not be intercepted before MMIO/RAM address in native. i.e. the p2m table should not cover the local APIC address as RAM, but I think your change in the qemu side is more straightforward (otherwise, we may need consider IOAPIC, HPET etc, which is complex).

One thing left is, why it will hang, after all, guest will try to restore the BAR address later, and at that time, the local apic access can be intercepted again.

--jyh

> "0xfe000000" to physical mmio. However, windows and BSD guests do not
> have this issue. They alway probe mmio size after disabling mmio.
> Thanks,
> Wei
> 
> On Monday 20 July 2009 16:26:00 Jiang, Yunhong wrote:
>> I assume guest should disable the MMIO in PCI_COMMAND before writing
>> all "1"s to bar register. Otherwise, what will happen on native if
>> guest try to access 0xFFFFFFF0? And if we do update the P2M, will it
>> cause trouble to Xen HV? 
>> 
>> Thanks
>> Yunhong Jiang
>> 
>> xen-devel-bounces@lists.xensource.com wrote:
>>> Hi,
>>> When guest code tries to get the block size of mmio, it will write
>>> all "1"s into pci bar register and then qemu will return all "0"s
>>> to the don't care bits in the emulated bar register to indicate the
>>> block size 
>>> to guest code.
>>> In this case, we should not create p2m mapping in
>>> pt_bar_reg_write() and
>>> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
>>> additional comment can be found in the patch.
>>> 
>>> Thanks,
>>> Wei
>>> 
>>> Signed-off-by: Wei Wang <wei.wang2@amd.com>
>>> --
>>> AMD GmbH, Germany
>>> Operating System Research Center
>>> 
>>> Legal Information:
>>> Advanced Micro Devices GmbH
>>> Karl-Hammerschmidt-Str. 34
>>> 85609 Dornach b. München
>>> 
>>> Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
>>> Sitz: Dornach, Gemeinde Aschheim, Landkreis München
>>> Registergericht München, HRB Nr. 43632

Re: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Wei Wang2]

Hi Yunhong
Thanks for the comment. Regarding the reason of guest hang, my guess is: guest 
OS might be trying to access guest memory at "0xfe000000" after remapping 
this address to mmio and guest memory may be corrupted after this access. 
For devices with large mmio, the complement code of mmio block size is more 
likely to be a real guest RAM. I saw the same issue when I assigned a graphic 
device with 256MB mmio to Linux guest. But devices with small mmio seems to 
work well. I did not see the problem on a broadcom NIC with 64K mmio.
Thanks,
Wei

On Monday 20 July 2009 17:48:46 Jiang, Yunhong wrote:
> Wei Wang2 wrote:
> > Hi Yunhong
> > My testing shows that Linux guests will try to probe pci bar with
> > MMIO being enabled. When I assign a broadcom NIC with 32MB MMIO a
> > Linux guest, guest will hang after remapping a guest address
>
> Yes, I remember I saw this bug in kernel before.
>
> I suspect the issue here is the local APIC address, which should not be
> intercepted before MMIO/RAM address in native. i.e. the p2m table should
> not cover the local APIC address as RAM, but I think your change in the
> qemu side is more straightforward (otherwise, we may need consider IOAPIC,
> HPET etc, which is complex).
>
> One thing left is, why it will hang, after all, guest will try to restore
> the BAR address later, and at that time, the local apic access can be
> intercepted again.
>
> --jyh
>
> > "0xfe000000" to physical mmio. However, windows and BSD guests do not
> > have this issue. They alway probe mmio size after disabling mmio.
> > Thanks,
> > Wei
> >
> > On Monday 20 July 2009 16:26:00 Jiang, Yunhong wrote:
> >> I assume guest should disable the MMIO in PCI_COMMAND before writing
> >> all "1"s to bar register. Otherwise, what will happen on native if
> >> guest try to access 0xFFFFFFF0? And if we do update the P2M, will it
> >> cause trouble to Xen HV?
> >>
> >> Thanks
> >> Yunhong Jiang
> >>
> >> xen-devel-bounces@lists.xensource.com wrote:
> >>> Hi,
> >>> When guest code tries to get the block size of mmio, it will write
> >>> all "1"s into pci bar register and then qemu will return all "0"s
> >>> to the don't care bits in the emulated bar register to indicate the
> >>> block size
> >>> to guest code.
> >>> In this case, we should not create p2m mapping in
> >>> pt_bar_reg_write() and
> >>> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
> >>> additional comment can be found in the patch.
> >>>
> >>> Thanks,
> >>> Wei
> >>>
> >>> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> >>> --
> >>> AMD GmbH, Germany
> >>> Operating System Research Center
> >>>
> >>> Legal Information:
> >>> Advanced Micro Devices GmbH
> >>> Karl-Hammerschmidt-Str. 34
> >>> 85609 Dornach b. München
> >>>
> >>> Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
> >>> Sitz: Dornach, Gemeinde Aschheim, Landkreis München
> >>> Registergericht München, HRB Nr. 43632

RE: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Jiang, Yunhong]

Thanks for your reply very much. BTW, I'm not object the patch , the idea of the patch is quite straightforward. 

In kernel side, the issue has been discussed a lot ( e.g http://kerneltrap.org/mailarchive/linux-kernel/2007/8/28/165632). The main challenge in kernel side is MMCFG (maybe because the MMIO for display is intercepted before MMCFG?? I'm not sure). So if the issue is caused by geust memory access, it may cause issue on native also.
I think the 64K mmio is ok because it will change mapping to 0xFFxxxxx, which is higher than APIC/HPET range. I suspect 32M is the minimal size that will cause problem.
Anyway, this is only for some clarification and should not matter much.

Thanks
Yunhong Jiang

Wei Wang2 wrote:
> Hi Yunhong
> Thanks for the comment. Regarding the reason of guest hang, my
> guess is: guest
> OS might be trying to access guest memory at "0xfe000000"
> after remapping
> this address to mmio and guest memory may be corrupted after
> this access.
> For devices with large mmio, the complement code of mmio block
> size is more
> likely to be a real guest RAM. I saw the same issue when I
> assigned a graphic
> device with 256MB mmio to Linux guest. But devices with small
> mmio seems to
> work well. I did not see the problem on a broadcom NIC with 64K mmio.
> Thanks, Wei
> 
> On Monday 20 July 2009 17:48:46 Jiang, Yunhong wrote:
>> Wei Wang2 wrote:
>>> Hi Yunhong
>>> My testing shows that Linux guests will try to probe pci bar with
>>> MMIO being enabled. When I assign a broadcom NIC with 32MB MMIO a
>>> Linux guest, guest will hang after remapping a guest address
>> 
>> Yes, I remember I saw this bug in kernel before.
>> 
>> I suspect the issue here is the local APIC address, which should not
>> be intercepted before MMIO/RAM address in native. i.e. the p2m table
>> should not cover the local APIC address as RAM, but I think your
>> change in the qemu side is more straightforward (otherwise, we may
>> need consider IOAPIC, HPET etc, which is complex). 
>> 
>> One thing left is, why it will hang, after all, guest will try to
>> restore the BAR address later, and at that time, the local apic
>> access can be intercepted again. 
>> 
>> --jyh
>> 
>>> "0xfe000000" to physical mmio. However, windows and BSD guests do
>>> not have this issue. They alway probe mmio size after disabling
>>> mmio. Thanks, Wei
>>> 
>>> On Monday 20 July 2009 16:26:00 Jiang, Yunhong wrote:
>>>> I assume guest should disable the MMIO in PCI_COMMAND before
>>>> writing all "1"s to bar register. Otherwise, what will happen on
>>>> native if guest try to access 0xFFFFFFF0? And if we do update the
>>>> P2M, will it cause trouble to Xen HV? 
>>>> 
>>>> Thanks
>>>> Yunhong Jiang
>>>> 
>>>> xen-devel-bounces@lists.xensource.com wrote:
>>>>> Hi,
>>>>> When guest code tries to get the block size of mmio, it will write
>>>>> all "1"s into pci bar register and then qemu will return all "0"s
>>>>> to the don't care bits in the emulated bar register to indicate
>>>>> the block size to guest code.
>>>>> In this case, we should not create p2m mapping in
>>>>> pt_bar_reg_write() and
>>>>> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
>>>>> additional comment can be found in the patch.
>>>>> 
>>>>> Thanks,
>>>>> Wei
>>>>> 
>>>>> Signed-off-by: Wei Wang <wei.wang2@amd.com>
>>>>> --
>>>>> AMD GmbH, Germany
>>>>> Operating System Research Center
>>>>> 
>>>>> Legal Information:
>>>>> Advanced Micro Devices GmbH
>>>>> Karl-Hammerschmidt-Str. 34
>>>>> 85609 Dornach b. München
>>>>> 
>>>>> Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
>>>>> Sitz: Dornach, Gemeinde Aschheim, Landkreis München
>>>>> Registergericht München, HRB Nr. 43632

RE: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Ian Jackson]

Jiang, Yunhong writes ("RE: [Xen-devel] [PATCH] passthru: Fix pci bar remapping for passthru	devices"):
> Thanks for your reply very much. BTW, I'm not object the patch , the
> idea of the patch is quite straightforward.

Right, good, I'll take that as an Ack and apply it.

Thanks,
Ian.

Re: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Keir Fraser]

Are you questioning whether this patch should be applied?

 -- Keir

On 20/07/2009 15:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:

> I assume guest should disable the MMIO in PCI_COMMAND before writing all "1"s
> to bar register. Otherwise, what will happen on native if guest try to access
> 0xFFFFFFF0? 
> And if we do update the P2M, will it cause trouble to Xen HV?
> 
> Thanks
> Yunhong Jiang
> 
> xen-devel-bounces@lists.xensource.com wrote:
>> Hi,
>> When guest code tries to get the block size of mmio, it will write
>> all "1"s into pci bar register and then qemu will return all "0"s to
>> the don't care
>> bits in the emulated bar register to indicate the block size
>> to guest code.
>> In this case, we should not create p2m mapping in
>> pt_bar_reg_write() and
>> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
>> additional comment can be found in the patch.
>> 
>> Thanks,
>> Wei
>> 
>> Signed-off-by: Wei Wang <wei.wang2@amd.com>
>> --
>> AMD GmbH, Germany
>> Operating System Research Center
>> 
>> Legal Information:
>> Advanced Micro Devices GmbH
>> Karl-Hammerschmidt-Str. 34
>> 85609 Dornach b. München
>> 
>> Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
>> Sitz: Dornach, Gemeinde Aschheim, Landkreis München
>> Registergericht München, HRB Nr. 43632
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Jiang, Yunhong]

I'm just wondering if this is really needed. See my response to Wei Wang.

Thanks
--jyh

Keir Fraser wrote:
> Are you questioning whether this patch should be applied?
> 
> -- Keir
> 
> On 20/07/2009 15:26, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:
> 
>> I assume guest should disable the MMIO in PCI_COMMAND before writing
>> all "1"s to bar register. Otherwise, what will happen on native if
>> guest try to access 0xFFFFFFF0? And if we do update the P2M, will it
>> cause trouble to Xen HV? 
>> 
>> Thanks
>> Yunhong Jiang
>> 
>> xen-devel-bounces@lists.xensource.com wrote:
>>> Hi,
>>> When guest code tries to get the block size of mmio, it will write
>>> all "1"s into pci bar register and then qemu will return all "0"s
>>> to the don't care bits in the emulated bar register to indicate the
>>> block size 
>>> to guest code.
>>> In this case, we should not create p2m mapping in
>>> pt_bar_reg_write() and
>>> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue,
>>> additional comment can be found in the patch.
>>> 
>>> Thanks,
>>> Wei
>>> 
>>> Signed-off-by: Wei Wang <wei.wang2@amd.com>
>>> --
>>> AMD GmbH, Germany
>>> Operating System Research Center
>>> 
>>> Legal Information:
>>> Advanced Micro Devices GmbH
>>> Karl-Hammerschmidt-Str. 34
>>> 85609 Dornach b. München
>>> 
>>> Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
>>> Sitz: Dornach, Gemeinde Aschheim, Landkreis München
>>> Registergericht München, HRB Nr. 43632
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel

RE: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Kay, Allen M]

Hi Wei,

I thought the originally code already handles sizing of the BAR operations (writing all 1's and reading the size back). How did it work before? Did it work because we are relying on the a well behaved guest to restore the original value of the BAR after the sizing operation?

By reading the code, it is not obvious to me how does not doing r->addr = cfg_entry->data operation prevents calling of pt_bar_mapping_one() to create p2m mapping.

Allen

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2
Sent: Monday, July 20, 2009 7:04 AM
To: xen-devel@lists.xensource.com
Subject: [Xen-devel] [PATCH] passthru: Fix pci bar remapping for passthru devices

Hi, 
When guest code tries to get the block size of mmio, it will write all "1"s 
into pci bar register and then qemu will return all "0"s to the don't care 
bits in the emulated bar register to indicate the block size to guest code. 
In this case, we should not create p2m mapping in  pt_bar_reg_write() and
pt_exp_rom_bar_reg_write(). Attached patch fixes this issue, additional 
comment can be found in the patch.

Thanks,
Wei

Signed-off-by: Wei Wang <wei.wang2@amd.com>
--
 AMD GmbH, Germany
 Operating System Research Center
 
 Legal Information:
 Advanced Micro Devices GmbH
 Karl-Hammerschmidt-Str. 34
 85609 Dornach b. München
 
 Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
 Sitz: Dornach, Gemeinde Aschheim, Landkreis München
 Registergericht München, HRB Nr. 43632

Re: [PATCH] passthru: Fix pci bar remapping for passthru devices

[Wei Wang2]

Hi Allen,
Pls find my explanations inline.
> I thought the originally code already handles sizing of the BAR operations
> (writing all 1's and reading the size back). How did it work before? Did it
> work because we are relying on the a well behaved guest to restore the
> original value of the BAR after the sizing operation?
In early qemu, remapping mmio was only allowed by pt_cmd_reg_write(). But 
currently, guest code can also trigger mmio remapping from pt_bar_reg_write() 
and pt_exp_rom_bar_reg_write() and this will cause the problem.

> By reading the code, it is not obvious to me how does not doing r->addr =
> cfg_entry->data operation prevents calling of pt_bar_mapping_one() to
> create p2m mapping.

Guest OS probes pci bar after guest bios doing this, so r->addr will still 
have the old mmio base address assigned by guest bios before guest OS 
writing '1's. If we prevent r->addr from being updated by cfg_entry->data, 
pt_bar_mapping_one() will not trigger any actual p2m updates because it will 
check whether r->addr has already been changed before calling r->map_func().

Thanks,
Wei

> Allen
>
> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2 Sent:
> Monday, July 20, 2009 7:04 AM
> To: xen-devel@lists.xensource.com
> Subject: [Xen-devel] [PATCH] passthru: Fix pci bar remapping for passthru
> devices
>
> Hi,
> When guest code tries to get the block size of mmio, it will write all "1"s
> into pci bar register and then qemu will return all "0"s to the don't care
> bits in the emulated bar register to indicate the block size to guest code.
> In this case, we should not create p2m mapping in  pt_bar_reg_write() and
> pt_exp_rom_bar_reg_write(). Attached patch fixes this issue, additional
> comment can be found in the patch.
>
> Thanks,
> Wei
>
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> --
>  AMD GmbH, Germany
>  Operating System Research Center
>
>  Legal Information:
>  Advanced Micro Devices GmbH
>  Karl-Hammerschmidt-Str. 34
>  85609 Dornach b. München
>
>  Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
>  Sitz: Dornach, Gemeinde Aschheim, Landkreis München
>  Registergericht München, HRB Nr. 43632
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel