Xen devel - Dec 2012 - Inconsistent MSI flags for pure HVM guest in 4.1.3 with the back-ported 4.2.0 msi_retranslate patch. (Was: Issue about domU missing interrupt)

Sorry, but I have to resend this in a separate thread for better visibility.
Back ground:
After backporting this patch to fix my PVHVM interrupt missing issue in
4.1.3:
http://lists.xen.org/archives/html/xen-devel/2012-06/msg01909.html
I find side effect for pure HVM guest.
http://lists.xen.org/archives/html/xen-devel/2012-12/msg00208.html

I had a follow up thread to analysis the issue and post it again here:

Hi, it seems that the patch has some side effect on pure HVM
guest.
>> For openelec 2.0 guest, which is based on linux 3.2.x with PVOP
disabled,
>> I see such syndrome in qemu-dm-xxx.log:
>>
>> pt_msgaddr64_reg_write: Error: why comes to Upper Address without 64
bit
>> support??
>> pt_pci_write_config: Internal error: Invalid write emulation return
>> value[-1]. I/O emulator exit.
>>
>> The guest dies immediately after the log, I have no way to check guest
>> kernel log.
>> Without the patch, this guest can boot without obvious error log even
the
>> VGA passthrough does not quite work.
>> I''ll check the code to see what does these log mean...
>>
>
> I did some analysis and it really looks like a bug to me.
> Since this is a patch back-ported from 4.2.0, I would like to ask is there
> any follow up patch that would fix this issue?
> Please see my analysis below:
>
> Here is part of the qemu-dm log, with debug log enabled at compile time:
>
> dm-command: hot insert pass-through pci dev
> register_real_device: Assigning real physical device 00:1b.0 ...
> pt_iomul_init: Error: pt_iomul_init can''t open file
/dev/xen/pci_iomul: No
> such file or directory: 0x0:0x1b.0x0
> pt_register_regions: IO region registered (size=0x00004000
> base_addr=0xf7c10004)
> pt_msi_setup: msi mapped with pirq 36
> pci_intx: intx=1
> register_real_device: Real physical device 00:1b.0 registered successfuly!
> IRQ type = MSI-INTx
> ...
> pt_pci_read_config: [00:06.0]: address=0000 val=0x0000*8086* len=2
> pt_pci_read_config: [00:06.0]: address=0002 val=0x0000*1e20* len=2
> ...
> *pt_pci_write_config: [00:06.0]: address=0068* val=0x00000000 len=4
> ...
> pt_pci_write_config: [00:06.0]: address=0062 val=0x00000081 len=2
> *pt_msgctrl_reg_write: guest enabling MSI, disable MSI-INTx translation*
> pci_intx: intx=1
> *pt_msi_disable: Unmap msi with pirq 36*
> pt_msgctrl_reg_write: setup msi for dev 30
> pt_msi_setup: msi mapped with pirq 36
> pt_msi_update: Update msi with pirq 36 gvec 51 gflags 1303
> pt_pci_read_config: [00:06.0]: address=0062 val=0x00000081 len=2
> pt_pci_write_config: [00:06.0]: address=0062 val=0x00000081 len=2
> pt_pci_write_config: [00:06.0]: address=0064 val=0xfee0300c len=4
> *pt_pci_write_config: [00:06.0]: address=0068* val=0x00000000 len=4
>
> pt_msgaddr64_reg_write: Error: why comes to Upper Address without 64 bit
> support??
> pt_pci_write_config: Internal error: Invalid write emulation return
> value[-1]. I/O emulator exit.
>
>
> Here the device in question should is the audio controller, 00:1b.0 in the
> host, which is 64 bit capable:
> 00:1b.0 Audio device: Intel Corporation 7 Series/C210 Series Chipset
> Family High Definition Audio Controller (rev 04)
>     Capabilities: [60] MSI: Enable+ Count=1/1 Maskable- 64bit+
>         Address: 00000000fee00378  Data: 0000
>
> And there is also a successful write to offset 0x68 above, while the
> second write fails the I/O emulator.
>
>
> The patch added pt_msi_disable() call into *pt_msgctrl_reg_write()
*function,
> And the pt_msi_disable() function has these lines:
> out:
>     /* clear msi info */
>     dev->msi->flags = 0;
>     dev->msi->pirq = -1;
>     dev->msi_trans_en = 0;
>
> As a result, the flags are cleared -- this is new to the patch.
> And I believe this change caused the failure in pt_msgaddr64_write():
>
> 3882     /* check whether the type is 64 bit or not */
> 3883     if (!(ptdev->msi->flags & PCI_MSI_FLAGS_64BIT))
> 3884     {
> 3885         /* exit I/O emulator */
> 3886         PT_LOG("Error: why comes to Upper Address without 64 bit
> support??\n");
> 3887         return -1;
> 3888     }
>
>
> I only see flags setup in  pt_msgctrl_reg_init() function. I guess this is
> not called this time.
>
> Thanks,
> Timothy
>

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

Hi, could anybody take a look please?


On Thu, Dec 6, 2012 at 11:42 AM, G.R.
<firemeteor@users.sourceforge.net>wrote:
> Sorry, but I have to resend this in a separate thread for better
> visibility.
> Back ground:
> After backporting this patch to fix my PVHVM interrupt missing issue in
> 4.1.3:
> http://lists.xen.org/archives/html/xen-devel/2012-06/msg01909.html
> I find side effect for pure HVM guest.
> http://lists.xen.org/archives/html/xen-devel/2012-12/msg00208.html
>
> I had a follow up thread to analysis the issue and post it again here:
>
> Hi, it seems that the patch has some side effect on pure HVM guest.
>>> For openelec 2.0 guest, which is based on linux 3.2.x with PVOP
>>> disabled, I see such syndrome in qemu-dm-xxx.log:
>>>
>>> pt_msgaddr64_reg_write: Error: why comes to Upper Address without
64 bit
>>> support??
>>> pt_pci_write_config: Internal error: Invalid write emulation return
>>> value[-1]. I/O emulator exit.
>>>
>>> The guest dies immediately after the log, I have no way to check
guest
>>> kernel log.
>>> Without the patch, this guest can boot without obvious error log
even
>>> the VGA passthrough does not quite work.
>>> I''ll check the code to see what does these log mean...
>>>
>>
>> I did some analysis and it really looks like a bug to me.
>> Since this is a patch back-ported from 4.2.0, I would like to ask is
>> there any follow up patch that would fix this issue?
>> Please see my analysis below:
>>
>> Here is part of the qemu-dm log, with debug log enabled at compile
time:
>>
>> dm-command: hot insert pass-through pci dev
>> register_real_device: Assigning real physical device 00:1b.0 ...
>> pt_iomul_init: Error: pt_iomul_init can''t open file
/dev/xen/pci_iomul:
>> No such file or directory: 0x0:0x1b.0x0
>> pt_register_regions: IO region registered (size=0x00004000
>> base_addr=0xf7c10004)
>> pt_msi_setup: msi mapped with pirq 36
>> pci_intx: intx=1
>> register_real_device: Real physical device 00:1b.0 registered
successfuly!
>> IRQ type = MSI-INTx
>> ...
>> pt_pci_read_config: [00:06.0]: address=0000 val=0x0000*8086* len=2
>> pt_pci_read_config: [00:06.0]: address=0002 val=0x0000*1e20* len=2
>> ...
>> *pt_pci_write_config: [00:06.0]: address=0068* val=0x00000000 len=4
>> ...
>> pt_pci_write_config: [00:06.0]: address=0062 val=0x00000081 len=2
>> *pt_msgctrl_reg_write: guest enabling MSI, disable MSI-INTx
translation*
>> pci_intx: intx=1
>> *pt_msi_disable: Unmap msi with pirq 36*
>> pt_msgctrl_reg_write: setup msi for dev 30
>> pt_msi_setup: msi mapped with pirq 36
>> pt_msi_update: Update msi with pirq 36 gvec 51 gflags 1303
>> pt_pci_read_config: [00:06.0]: address=0062 val=0x00000081 len=2
>> pt_pci_write_config: [00:06.0]: address=0062 val=0x00000081 len=2
>> pt_pci_write_config: [00:06.0]: address=0064 val=0xfee0300c len=4
>> *pt_pci_write_config: [00:06.0]: address=0068* val=0x00000000 len=4
>>
>> pt_msgaddr64_reg_write: Error: why comes to Upper Address without 64
bit
>> support??
>> pt_pci_write_config: Internal error: Invalid write emulation return
>> value[-1]. I/O emulator exit.
>>
>>
>> Here the device in question should is the audio controller, 00:1b.0 in
>> the host, which is 64 bit capable:
>> 00:1b.0 Audio device: Intel Corporation 7 Series/C210 Series Chipset
>> Family High Definition Audio Controller (rev 04)
>>     Capabilities: [60] MSI: Enable+ Count=1/1 Maskable- 64bit+
>>         Address: 00000000fee00378  Data: 0000
>>
>> And there is also a successful write to offset 0x68 above, while the
>> second write fails the I/O emulator.
>>
>>
>> The patch added pt_msi_disable() call into *pt_msgctrl_reg_write()
*function,
>> And the pt_msi_disable() function has these lines:
>> out:
>>     /* clear msi info */
>>     dev->msi->flags = 0;
>>     dev->msi->pirq = -1;
>>     dev->msi_trans_en = 0;
>>
>> As a result, the flags are cleared -- this is new to the patch.
>> And I believe this change caused the failure in pt_msgaddr64_write():
>>
>> 3882     /* check whether the type is 64 bit or not */
>> 3883     if (!(ptdev->msi->flags & PCI_MSI_FLAGS_64BIT))
>> 3884     {
>> 3885         /* exit I/O emulator */
>> 3886         PT_LOG("Error: why comes to Upper Address without 64
bit
>> support??\n");
>> 3887         return -1;
>> 3888     }
>>
>>
>> I only see flags setup in  pt_msgctrl_reg_init() function. I guess this
>> is not called this time.
>>
>> Thanks,
>> Timothy
>>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

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

Sorry for the delay in the reply

On Thu, 6 Dec 2012, G.R. wrote:
> Sorry, but I have to resend this in a separate thread for better
visibility.
> Back ground:
> After backporting this patch to fix my PVHVM interrupt missing issue in
4.1.3:
> http://lists.xen.org/archives/html/xen-devel/2012-06/msg01909.html
> I find side effect for pure HVM guest.
> http://lists.xen.org/archives/html/xen-devel/2012-12/msg00208.html
> 
> I had a follow up thread to analysis the issue and post it again here:
> 
>             Hi, it seems that the patch has some side effect on pure HVM
guest.
>             For openelec 2.0 guest, which is based on linux 3.2.x with PVOP
disabled, I see such syndrome in
>             qemu-dm-xxx.log:
> 
>             pt_msgaddr64_reg_write: Error: why comes to Upper Address
without 64 bit support??
>             pt_pci_write_config: Internal error: Invalid write emulation
return value[-1]. I/O emulator exit.
> 
>             The guest dies immediately after the log, I have no way to
check guest kernel log.
>             Without the patch, this guest can boot without obvious error
log even the VGA passthrough does not
>             quite work.
>             I''ll check the code to see what does these log mean...
> 
> 
> I did some analysis and it really looks like a bug to me.
> Since this is a patch back-ported from 4.2.0, I would like to ask is there
any follow up patch that would fix this
> issue?
> Please see my analysis below:
> 
> Here is part of the qemu-dm log, with debug log enabled at compile time:
> 
> dm-command: hot insert pass-through pci dev
> register_real_device: Assigning real physical device 00:1b.0 ...
> pt_iomul_init: Error: pt_iomul_init can''t open file
/dev/xen/pci_iomul: No such file or directory: 0x0:0x1b.0x0
> pt_register_regions: IO region registered (size=0x00004000
base_addr=0xf7c10004)
> pt_msi_setup: msi mapped with pirq 36
> pci_intx: intx=1
> register_real_device: Real physical device 00:1b.0 registered successfuly!
> IRQ type = MSI-INTx
> ...
> pt_pci_read_config: [00:06.0]: address=0000 val=0x00008086 len=2
> pt_pci_read_config: [00:06.0]: address=0002 val=0x00001e20 len=2
> ...
> pt_pci_write_config: [00:06.0]: address=0068 val=0x00000000 len=4
> ...
> pt_pci_write_config: [00:06.0]: address=0062 val=0x00000081 len=2
> pt_msgctrl_reg_write: guest enabling MSI, disable MSI-INTx translation
> pci_intx: intx=1
> pt_msi_disable: Unmap msi with pirq 36
> pt_msgctrl_reg_write: setup msi for dev 30
> pt_msi_setup: msi mapped with pirq 36
> pt_msi_update: Update msi with pirq 36 gvec 51 gflags 1303
> pt_pci_read_config: [00:06.0]: address=0062 val=0x00000081 len=2
> pt_pci_write_config: [00:06.0]: address=0062 val=0x00000081 len=2
> pt_pci_write_config: [00:06.0]: address=0064 val=0xfee0300c len=4
> pt_pci_write_config: [00:06.0]: address=0068 val=0x00000000 len=4
> pt_msgaddr64_reg_write: Error: why comes to Upper Address without 64 bit
support??
> pt_pci_write_config: Internal error: Invalid write emulation return
value[-1]. I/O emulator exit.
> 
> 
> Here the device in question should is the audio controller, 00:1b.0 in the
host, which is 64 bit capable:
> 00:1b.0 Audio device: Intel Corporation 7 Series/C210 Series Chipset Family
High Definition Audio Controller (rev 04)
>     Capabilities: [60] MSI: Enable+ Count=1/1 Maskable- 64bit+
>         Address: 00000000fee00378  Data: 0000
I won''t be able to reproduce the bug, so I thank you in advance for any
help you can give me with testing a fix

> And there is also a successful write to offset 0x68 above, while the second
write fails the I/O emulator.
> 
> 
> The patch added pt_msi_disable() call into pt_msgctrl_reg_write() function,
And the pt_msi_disable() function has these
> lines:
> out:
>     /* clear msi info */
>     dev->msi->flags = 0;
>     dev->msi->pirq = -1;
>     dev->msi_trans_en = 0;
> 
> As a result, the flags are cleared -- this is new to the patch.
> And I believe this change caused the failure in pt_msgaddr64_write():
> 
> 3882     /* check whether the type is 64 bit or not */
> 3883     if (!(ptdev->msi->flags & PCI_MSI_FLAGS_64BIT))
> 3884     {
> 3885         /* exit I/O emulator */
> 3886         PT_LOG("Error: why comes to Upper Address without 64 bit
support??\n");
> 3887         return -1;
> 3888     }
> 
> 
> I only see flags setup in  pt_msgctrl_reg_init() function. I guess this is
not called this time.
I think you might be right, thanks for the detailed analysis!
If this is really the cause of the problem (as it looks like it is),
then the following should fix it:



diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 73f737d..b03b989 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
 
 out:
     /* clear msi info */
-    dev->msi->flags = 0;
+    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
PCI_MSI_FLAGS_ENABLE);
     dev->msi->pirq = -1;
     dev->msi_trans_en = 0;
 }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>
>
> I won''t be able to reproduce the bug, so I thank you in advance
for any
> help you can give me with testing a fix
>
> Thanks for your fix, I''ll try it out tomorrow and post the result
-- it''s
too late today.
But I''m wondering if it is specific to 4.1.3, or have been fixed in 4.2
already.

Thanks,
Timothy


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

On Fri, 7 Dec 2012, G.R. wrote:
> I won''t be able to reproduce the bug, so I thank you in advance
for any
> help you can give me with testing a fix
> 
> Thanks for your fix, I''ll try it out tomorrow and post the result
-- it''s too late today.
> But I''m wondering if it is specific to 4.1.3, or have been fixed
in 4.2 already.
I don''t think it has been fixes in 4.2

On Sat, Dec 8, 2012 at 2:11 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 7 Dec 2012, G.R. wrote:
> > I won''t be able to reproduce the bug, so I thank you in
advance for any
> > help you can give me with testing a fix
> >
> > Thanks for your fix, I''ll try it out tomorrow and post the
result --
> it''s too late today.
> > But I''m wondering if it is specific to 4.1.3, or have been
fixed in 4.2
> already.
>
> I don''t think it has been fixes in 4.2
>
Thanks Stefano. The fix solved the regression caused by the mistranslate
patch.
I guess you can submit this patch for further verification.

But unfortunately it does not help to make the IGD work in pure HVM guest.
I''ve enabled drm.debug log and dumped the IGD register value for both
passing && failing case.
They do have some differences. I''m contacting the driver developer to
identify the fatal ones.

I hope this effort can finally make win7 work also.
Currently it simply BSOD at early booting stage after the driver is
installed.
I have no way to debug directly -- if you have any suggestion, please share
with me.

Thanks,
Timothy


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

"qemu-xen-trad: fix msi_translate with PV event delivery" added a
pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
as a consequence. MSIs get enabled again soon after by calling
pt_msi_setup.

However the MSI flags are only setup once in the pt_msgctrl_reg_init
function, so from the QEMU point of view the device has lost some
important properties, like for example PCI_MSI_FLAGS_64BIT.

This patch fixes the bug by clearing only the MSI
enabled/mapped/initialized flags in pt_msi_disable.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Tested-by: G.R. <firemeteor@users.sourceforge.net>
Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075

diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 73f737d..b03b989 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
 
 out:
     /* clear msi info */
-    dev->msi->flags = 0;
+    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
PCI_MSI_FLAGS_ENABLE);
     dev->msi->pirq = -1;
     dev->msi_trans_en = 0;
 }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Monday, December 10, 2012, 1:36:41 PM, you wrote:
> "qemu-xen-trad: fix msi_translate with PV event delivery" added a
> pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
> as a consequence. MSIs get enabled again soon after by calling
> pt_msi_setup.
> However the MSI flags are only setup once in the pt_msgctrl_reg_init
> function, so from the QEMU point of view the device has lost some
> important properties, like for example PCI_MSI_FLAGS_64BIT.
> This patch fixes the bug by clearing only the MSI
> enabled/mapped/initialized flags in pt_msi_disable.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Tested-by: G.R. <firemeteor@users.sourceforge.net>
> Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> index 73f737d..b03b989 100644
> --- a/hw/pt-msi.c
> +++ b/hw/pt-msi.c
> @@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
>  
>  out:
>      /* clear msi info */
-    dev->>msi->flags = 0;
+    dev->>msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
PCI_MSI_FLAGS_ENABLE);
>      dev->msi->pirq = -1;
>      dev->msi_trans_en = 0;
>  }

Seems this should be fixed for qemu-upstream as well ?
I think since switching to qemu-upstream as default for xen-unstable / 4.3 seems
around the corner,
it's perhaps wise for all patches to qemu-traditional, to also check if
qemu-upstream needs the same fix (to prevent regressions after the switch) ?


--
Sander


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

On Mon, 10 Dec 2012, Sander Eikelenboom wrote:
> Monday, December 10, 2012, 1:36:41 PM, you wrote:
> 
> > "qemu-xen-trad: fix msi_translate with PV event delivery"
added a
> > pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI
flags
> > as a consequence. MSIs get enabled again soon after by calling
> > pt_msi_setup.
> 
> > However the MSI flags are only setup once in the pt_msgctrl_reg_init
> > function, so from the QEMU point of view the device has lost some
> > important properties, like for example PCI_MSI_FLAGS_64BIT.
> 
> > This patch fixes the bug by clearing only the MSI
> > enabled/mapped/initialized flags in pt_msi_disable.
> 
> 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > Tested-by: G.R. <firemeteor@users.sourceforge.net>
> > Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
> 
> > diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> > index 73f737d..b03b989 100644
> > --- a/hw/pt-msi.c
> > +++ b/hw/pt-msi.c
> > @@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
> >  
> >  out:
> >      /* clear msi info */
> -    dev->>msi->flags = 0;
> +    dev->>msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
PCI_MSI_FLAGS_ENABLE);
> >      dev->msi->pirq = -1;
> >      dev->msi_trans_en = 0;
> >  }
> 
> 
> Seems this should be fixed for qemu-upstream as well ?
> I think since switching to qemu-upstream as default for xen-unstable / 4.3
seems around the corner,
> it''s perhaps wise for all patches to qemu-traditional, to also
check if qemu-upstream needs the same fix (to prevent regressions after the
switch) ?
> 
That is true and you do right to remind me, but this is one of the few
cases of PCI passthrough code where upstream QEMU and
qemu-xen-traditional differ: MSI translation is not implemented at all
in upstream QEMU so there is no need for this patch there.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Stefano Stabellini writes ("[Xen-devel] [PATCH]
qemu-xen-trad/pt_msi_disable: do not clear all MSI
flags"):
> "qemu-xen-trad: fix msi_translate with PV event delivery" added a
> pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
> as a consequence. MSIs get enabled again soon after by calling
> pt_msi_setup.
> 
> However the MSI flags are only setup once in the pt_msgctrl_reg_init
> function, so from the QEMU point of view the device has lost some
> important properties, like for example PCI_MSI_FLAGS_64BIT.
> 
> This patch fixes the bug by clearing only the MSI
> enabled/mapped/initialized flags in pt_msi_disable.
> 
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Tested-by: G.R. <firemeteor@users.sourceforge.net>
> Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
Applied, thanks.  Sorry for the delay.

Ian.