Dom0 linux 4.0 + devel/for-linus-4.1 branch: p2m.c:884:d0v0 gfn_to_mfn failed! gfn=ffffffff001ed type:4

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

I now get a very large number of these at guest with passthrough start:
(XEN) [2015-04-12 14:55:20.226] p2m.c:884:d0v0 gfn_to_mfn failed! gfn=ffffffff001ed type:4

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

I think this is qemu incorrectly trying to map GFNs.

David

Quoted 2 lines

I now get a very large number of these at guest with passthrough start:
(XEN) [2015-04-12 14:55:20.226] p2m.c:884:d0v0 gfn_to_mfn failed! gfn=ffffffff001ed type:4

Quoted 12 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

Quoted 3 lines

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

Quoted 18 lines · Sander Eikelenboom

Quoted 14 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

I think this is qemu incorrectly trying to map GFNs.

Quoted 3 lines

Quoted 1 line

David

Reverted that specific one, but still get those messages.

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

David

Quoted 22 lines · you

Quoted 21 lines · Sander Eikelenboom

Quoted 9 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Quoted 7 lines

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

Quoted 4 lines

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

David

Ok .. hmm first candidate of the bisect also looks interessting:
[628c28eefd6f2cef03b212081b466ae43fd093a3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

Quoted 29 lines · Sander Eikelenboom

Quoted 25 lines · you

Quoted 16 lines · Sander Eikelenboom

Quoted 12 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

Quoted 5 lines

Quoted 3 lines

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

Quoted 4 lines

Quoted 1 line

David

Ok .. hmm first candidate of the bisect also looks interessting:
[628c28eefd6f2cef03b212081b466ae43fd093a3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

Unless your dom0 is PVH, no.

David

Quoted 29 lines · Sander Eikelenboom

Quoted 25 lines · you

Quoted 16 lines · Sander Eikelenboom

Quoted 12 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

Quoted 5 lines

Quoted 3 lines

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

Quoted 4 lines

Quoted 1 line

David

Ok .. hmm first candidate of the bisect also looks interessting:
[628c28eefd6f2cef03b212081b466ae43fd093a3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

Unless your dom0 is PVH, no.

David

Quoted 30 lines · you

Quoted 29 lines · Sander Eikelenboom

Quoted 25 lines · you

Quoted 16 lines · Sander Eikelenboom

Quoted 12 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

Quoted 5 lines

Quoted 3 lines

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

David

Ok .. hmm first candidate of the bisect also looks interessting:
[628c28eefd6f2cef03b212081b466ae43fd093a3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

Unless your dom0 is PVH, no.

David

Bisection came back with:

22d8a8938407cb1342af763e937fdf9ee8daf24a is the first bad commit commit 22d8a8938407cb1342af763e937fdf9ee8daf24a

Author: Konrad Rzeszutek Wilk <[email protected]>
Date:   Fri Apr 3 10:28:08 2015 -0400

xen/pciback: Don't disable PCI_COMMAND on PCI device reset.

Quoted 30 lines · you

Quoted 29 lines · Sander Eikelenboom

Quoted 25 lines · you

Quoted 16 lines · Sander Eikelenboom

Quoted 12 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

Quoted 5 lines

Quoted 3 lines

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

David

Ok .. hmm first candidate of the bisect also looks interessting:
[628c28eefd6f2cef03b212081b466ae43fd093a3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

Unless your dom0 is PVH, no.

David

Bisection came back with:

22d8a8938407cb1342af763e937fdf9ee8daf24a is the first bad commit commit 22d8a8938407cb1342af763e937fdf9ee8daf24a

Author: Konrad Rzeszutek Wilk <[email protected]>
Date:   Fri Apr 3 10:28:08 2015 -0400

xen/pciback: Don't disable PCI_COMMAND on PCI device reset.

There is no need for this at all. Worst it means that if the guest tries to write to BARs it could lead (on certain platforms) to PCI SERR errors.

Please note that with af6fc858a35b90e89ea7a7ee58e66628c55c776b "xen-pciback: limit guest control of command register"
a guest is still allowed to enable those control bits (safely), but is not allowed to disable them and that therefore a well behaved frontend which enables things before using them will still function correctly.

This is done via an write to the configuration register 0x4 which triggers on the backend side:
command_write
\- pci_enable_device
\- pci_enable_device_flags
\- do_pci_enable_device
\- pcibios_enable_device
\-pci_enable_resourcess
[which enables the PCI_COMMAND_MEMORY|PCI_COMMAND_IO]

However guests (and drivers) which don't do this could cause problems, including the security issues which XSA-120 sought to address.

CC [email protected]

Reported-by Jan Beulich <[email protected]>

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by David Vrabel <[email protected]>

Could it be due to the kind of "split brain" relation pciback and qemu have ? (qemu+libxl not setting up things like pcifront does (with event channel and conf space access going through pciback instead of qemu doing things by itself, which also leads to pciback not installing a interrupt handler in dom0 for the devices since that in pciback relies on:
- proper state signaling via xenbus
- setting up an event channel
- doing conf space acces via pciback
)

Is there actually a technical reason why the xen pci passthrough parts in qemu don't act more like pci-front for PV does:
setting up things properly via xenbus, event channel etc, so it would be more similar ?

Or was this more a kind of an oversight when HVM's were introduced ?

--
Sander

Quoted 92 lines · Sander Eikelenboom

Quoted 30 lines · you

Quoted 29 lines · Sander Eikelenboom

Quoted 25 lines · you

Quoted 16 lines · Sander Eikelenboom

Quoted 12 lines · you

Quoted 7 lines · Sander Eikelenboom

Hi David,

I seem to have spotted some trouble with a 4.0 dom0 kernel with the devel/for-linus-4.1 branch pulled on top.

Does this remind you of any specific commits in the devel/for-linus-4.1 branch that could likely be involved that i could try to revert ?

Yes. This will probably be 4e8c0c8c4bf3a (xen/privcmd: improve performance of MMAPBATCH_V2) which makes the kernel try harder to map all GFNs instead of failing on the first one.

Quoted 5 lines

Quoted 3 lines

I think this is qemu incorrectly trying to map GFNs.

David

Reverted that specific one, but still get those messages.

You'll have to bisect it then. Because I don't see any other relevant commits in devel/for-linus-4.1

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

David

Ok .. hmm first candidate of the bisect also looks interessting:
[628c28eefd6f2cef03b212081b466ae43fd093a3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

Unless your dom0 is PVH, no.

David

Bisection came back with:

22d8a8938407cb1342af763e937fdf9ee8daf24a is the first bad commit commit 22d8a8938407cb1342af763e937fdf9ee8daf24a

Author: Konrad Rzeszutek Wilk <[email protected]>
Date:   Fri Apr 3 10:28:08 2015 -0400

xen/pciback: Don't disable PCI_COMMAND on PCI device reset.

There is no need for this at all. Worst it means that if the guest tries to write to BARs it could lead (on certain platforms) to PCI SERR errors.

Please note that with af6fc858a35b90e89ea7a7ee58e66628c55c776b "xen-pciback: limit guest control of command register"
a guest is still allowed to enable those control bits (safely), but is not allowed to disable them and that therefore a well behaved frontend which enables things before using them will still function correctly.

This is done via an write to the configuration register 0x4 which triggers on the backend side:
command_write
\- pci_enable_device
\- pci_enable_device_flags
\- do_pci_enable_device
\- pcibios_enable_device
\-pci_enable_resourcess
[which enables the PCI_COMMAND_MEMORY|PCI_COMMAND_IO]

However guests (and drivers) which don't do this could cause problems, including the security issues which XSA-120 sought to address.

CC [email protected]

Reported-by Jan Beulich <[email protected]>

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

Signed-off-by David Vrabel <[email protected]>

Could it be due to the kind of "split brain" relation pciback and qemu have ? (qemu+libxl not setting up things like pcifront does (with event channel and conf space access going through pciback instead of qemu doing things by itself, which also leads to pciback not installing a interrupt handler in dom0 for the devices since that in pciback relies on:
- proper state signaling via xenbus
- setting up an event channel
- doing conf space acces via pciback
)

Quoted 3 lines

Is there actually a technical reason why the xen pci passthrough parts in qemu don't act more like pci-front for PV does:
setting up things properly via xenbus, event channel etc, so it would be more similar ?

What is your qemu-xen commit id?

With this patch reverted, what is lspci -vvv before you assign to a guest and when hvmloader started?

Thanks!

Quoted 2 lines

Or was this more a kind of an oversight when HVM's were introduced ?

Not sure I understand the question? The XSA126 (http://xenbits.xen.org/xsa/xsa126-qemuu.patch)
has a fix for enabling the proper bits in PCI_COMMAND which xen-pciback
had disabled (which the above Linux patch fixes).

If you have the QEMU patch in you should see a similar issue (I think). Unless the xen-pciback fix causes an previously unused path in libxl (or qemu) to be used (as before when it read the bars from SysFS they were zero). Hmmm.

Could you also attach 'xl -vvvv -d create <g.cfg>' with and without the kernel patch if it is not too much trouble?

If it is - I will do it tomorrow and see if I can see an disreprancy.

Quoted 4 lines

--
Sander

Hi Konrad,

xen version is at last changeset 3a28f760508fb35c430edac17a9efde5aff6d1d5 (previous unstable master before the force push which includes 1aeb1156fa43fe2cd2b5003995b20466cd19a622 which causes the trouble reported here:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01336.html )

qemu-xen is at last changeset 727b998448e852a5e8eb570ac3a259ef62fbdacb plus the revert of 7665d6ba98e20fb05c420de947c1750fd47e5c07 (due to other problem reported here:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01121.html )

Quoted 7 lines

Hi Konrad,

xen version is at last changeset 3a28f760508fb35c430edac17a9efde5aff6d1d5 (previous unstable master before the force push which includes 1aeb1156fa43fe2cd2b5003995b20466cd19a622 which causes the trouble reported here:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01336.html )

Quoted 4 lines

qemu-xen is at last changeset 727b998448e852a5e8eb570ac3a259ef62fbdacb plus the revert of 7665d6ba98e20fb05c420de947c1750fd47e5c07 (due to other problem reported here:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01121.html )

You sure? I thought Andrew had an three line patch to fix that:

xen/drivers/passthrough/x86/iommu.c +3 -0

+        if ( (mfn_to_gmfn(d, page_to_mfn(page)) != INVALID_MFN) &&
+            (has_hvm_container_domain(d) ||
+             ((page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page)) )
         {
             BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page))));
             rc = hd->platform_ops->map_page(

?

Quoted 7 lines · you

Quoted 7 lines

Hi Konrad,

xen version is at last changeset 3a28f760508fb35c430edac17a9efde5aff6d1d5 (previous unstable master before the force push which includes 1aeb1156fa43fe2cd2b5003995b20466cd19a622 which causes the trouble reported here:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01336.html )

Quoted 4 lines

Quoted 4 lines

qemu-xen is at last changeset 727b998448e852a5e8eb570ac3a259ef62fbdacb plus the revert of 7665d6ba98e20fb05c420de947c1750fd47e5c07 (due to other problem reported here:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01121.html )

Quoted 1 line

Quoted 1 line · On 10.06.15 at 03:02, <[email protected]> wrote:

The problem is that the XSA120-addendm patch (which does not clear the PCI_COMMAND register anymore), causes an missing functionality in qemu-xen to be exposed. This missing functionality is implemented in qemu-traditional which is why it works there.

The problem is that qemu-xen for any write to the BAR regions updates them to the hypervisor - but only if the real hardware has them enabled (see pci_update_mappings in pci_default_write_config which is called by xen_pt_pci_write_config). Specifically pci_bar_address checks PCI_COMMAND register for PCI_COMMAND_MEMORY. If it is disabled (so no XSA-120 addendum patch), it returns -1 (default value resulting in no changes in the internal structures). If it is enabled, then it updates the d->config space with the value written by the guest. Once xen_pt_pci_write_config is done it syncs up the changes (if there are any) which results in the QEMU calling hypervisor to update the P2M mapping.

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 03:02, <[email protected]> wrote:

The problem is that the XSA120-addendm patch (which does not clear the PCI_COMMAND register anymore), causes an missing functionality in qemu-xen to be exposed. This missing functionality is implemented in qemu-traditional which is why it works there.

Quoted 11 lines

The problem is that qemu-xen for any write to the BAR regions updates them to the hypervisor - but only if the real hardware has them enabled (see pci_update_mappings in pci_default_write_config which is called by xen_pt_pci_write_config). Specifically pci_bar_address checks PCI_COMMAND register for PCI_COMMAND_MEMORY. If it is disabled (so no XSA-120 addendum patch), it returns -1 (default value resulting in no changes in the internal structures). If it is enabled, then it updates the d->config space with the value written by the guest. Once xen_pt_pci_write_config is done it syncs up the changes (if there are any) which results in the QEMU calling hypervisor to update the P2M mapping.

There's one fundamental aspect I'm not understanding here:
pci_update_mappings() / pci_bar_address() look at the guest view (or at least they ought to be), and the virtual command register starts out as zero. Is the bug perhaps that xen_pt_initfn(), after having initialized d->config[] via xen_host_pci_get_block(), leaves the command register at its host view value (rather than updating it alongside reg_entry->data in xen_pt_config_reg_init(), called via xen_pt_config_init()), which would have happened to be zero without that XSA-120 addendum?

It is of course concerning that
there are two (now clearly mismatching) guest views within qemu (and along those lines I also wonder whether the apparent duplicate maintenance of MSI and MSI-X state, due to pci_default_write_config() calling msi{,x}_write_config(), can do any good, or why the code uses pci_default_write_config() but not pci_default_read_config()).

It looks to me as if there was a halfhearted attempt to utilize infrastructure already available in qemu when these Xen pieces got added, leading to hard to understand issues like the one here. I.e. even if we addressed the initialization value issue above, there would still be two competing emulation layers potentially (and I suppose quite likely) leading to differing register state later on. Hence I wonder how many more issues there are (to come)...

Quoted 1 line

Quoted 1 line · On 10.06.15 at 13:13, <[email protected]> wrote:

Quoted 18 lines · Jan Beulich

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 03:02, <[email protected]> wrote:

The problem is that the XSA120-addendm patch (which does not clear the PCI_COMMAND register anymore), causes an missing functionality in qemu-xen to be exposed. This missing functionality is implemented in qemu-traditional which is why it works there.

Quoted 11 lines

The problem is that qemu-xen for any write to the BAR regions updates them to the hypervisor - but only if the real hardware has them enabled (see pci_update_mappings in pci_default_write_config which is called by xen_pt_pci_write_config). Specifically pci_bar_address checks PCI_COMMAND register for PCI_COMMAND_MEMORY. If it is disabled (so no XSA-120 addendum patch), it returns -1 (default value resulting in no changes in the internal structures). If it is enabled, then it updates the d->config space with the value written by the guest. Once xen_pt_pci_write_config is done it syncs up the changes (if there are any) which results in the QEMU calling hypervisor to update the P2M mapping.

Quoted 9 lines

There's one fundamental aspect I'm not understanding here:
pci_update_mappings() / pci_bar_address() look at the guest view (or at least they ought to be), and the virtual command register starts out as zero. Is the bug perhaps that xen_pt_initfn(), after having initialized d->config[] via xen_host_pci_get_block(), leaves the command register at its host view value (rather than updating it alongside reg_entry->data in xen_pt_config_reg_init(), called via xen_pt_config_init()), which would have happened to be zero without that XSA-120 addendum?

It seems to me that Jan is right: setting the PCI_COMMAND register to ~PCI_COMMAND_MEMORY could be done at initialization time. Would that fix the bug?

Quoted 8 lines

It is of course concerning that
there are two (now clearly mismatching) guest views within qemu (and along those lines I also wonder whether the apparent duplicate maintenance of MSI and MSI-X state, due to pci_default_write_config() calling msi{,x}_write_config(), can do any good, or why the code uses pci_default_write_config() but not pci_default_read_config()).

Quoted 8 lines

It looks to me as if there was a halfhearted attempt to utilize infrastructure already available in qemu when these Xen pieces got added, leading to hard to understand issues like the one here. I.e. even if we addressed the initialization value issue above, there would still be two competing emulation layers potentially (and I suppose quite likely) leading to differing register state later on. Hence I wonder how many more issues there are (to come)...

The integration between the existing qemu-traditional code and the upstream QEMU code was hard. I am ready to believe there are more than just a few gaps and I would be happy to take further patches to improve the situation.

In this specific instance, are you referring to d->config, part of PCIDevice, and all the XenPTRegInfo instances? If so, I think the reason for having the latter, is that we need more flexibility, we need individual masks and read and write functions. At the same time we cannot really get rid of d->config.

Quoted 5 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 13:13, <[email protected]> wrote:

Quoted 3 lines · Jan Beulich

Quoted 3 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 03:02, <[email protected]> wrote:

The problem is that the XSA120-addendm patch (which does not clear the PCI_COMMAND register anymore), causes an missing functionality

in

Quoted 1 line

Quoted 1 line

Quoted 1 line

qemu-xen to be exposed. This missing functionality is implemented

in

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

qemu-traditional which is why it works there.

The problem is that qemu-xen for any write to the BAR regions updates them to the hypervisor - but only if the real hardware has them enabled (see pci_update_mappings in pci_default_write_config

which

Quoted 1 line

Quoted 1 line

Quoted 1 line

is called by xen_pt_pci_write_config). Specifically

pci_bar_address

Quoted 1 line

Quoted 1 line

Quoted 1 line

checks PCI_COMMAND register for PCI_COMMAND_MEMORY. If it is

disabled

Quoted 1 line

Quoted 1 line

Quoted 1 line

(so no XSA-120 addendum patch), it returns -1 (default value

resulting

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

in no changes in the internal structures). If it is enabled, then it updates the d->config space with the value written by the

guest.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Once xen_pt_pci_write_config is done it syncs up the changes (if

there

Quoted 1 line

Quoted 1 line

Quoted 1 line

are any) which results in the QEMU calling hypervisor to update

the P2M

Quoted 12 lines

Quoted 11 lines

Quoted 1 line

mapping.

There's one fundamental aspect I'm not understanding here:
pci_update_mappings() / pci_bar_address() look at the guest view (or at least they ought to be), and the virtual command register starts out as zero. Is the bug perhaps that xen_pt_initfn(), after having initialized d->config[] via xen_host_pci_get_block(), leaves the command register at its host view value (rather than updating it alongside reg_entry->data in xen_pt_config_reg_init(), called via xen_pt_config_init()), which would have happened to be zero without that XSA-120 addendum?

Quoted 3 lines

It seems to me that Jan is right: setting the PCI_COMMAND register to ~PCI_COMMAND_MEMORY could be done at initialization time. Would that fix the bug?

Why ~PCI_COMMAND_MEMORY? Just like in the Xen specific data, this field should start out as zero.

Quoted 18 lines

Quoted 9 lines

It is of course concerning that
there are two (now clearly mismatching) guest views within qemu (and along those lines I also wonder whether the apparent duplicate maintenance of MSI and MSI-X state, due to pci_default_write_config() calling msi{,x}_write_config(), can do any good, or why the code uses pci_default_write_config() but not pci_default_read_config()).

Quoted 8 lines

It looks to me as if there was a halfhearted attempt to utilize infrastructure already available in qemu when these Xen pieces got added, leading to hard to understand issues like the one here. I.e. even if we addressed the initialization value issue above, there would still be two competing emulation layers potentially (and I suppose quite likely) leading to differing register state later on. Hence I wonder how many more issues there are (to come)...

Quoted 2 lines

The integration between the existing qemu-traditional code and the upstream QEMU code was hard. I am ready to believe there are more

than

Quoted 1 line

just a few gaps and I would be happy to take further patches to

improve

Quoted 4 lines

the situation.

In this specific instance, are you referring to d->config, part of PCIDevice, and all the XenPTRegInfo instances? If so, I think the

reason

Quoted 3 lines

for having the latter, is that we need more flexibility, we need individual masks and read and write functions. At the same time we cannot really get rid of d->config.

I guessed as much, but in that case we should keep the two in sync (i.e. where we apply custom logic we should sync back what we do do d->config[], and at init time we should merge host and emulated state according to ->emu_mask; or maybe XenPTReg shouldn't even have a data field, and instead modifications should go straight to d->config[]). Perhaps a first step ought to be to log all cases where they differ?

Jan

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

hw/xen/xen_pt.c out: · line 793

Quoted 10 lines

hw/xen/xen_pt.c +3 -1

@@ -785,7 +785,9 @@ out:
         xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
                               pci_get_word(d->config + PCI_COMMAND) | cmd);
     }
-
+    /* Until the guest enables the device use d->config values which will
+     * inhibit pci_bar_address & pci_update_mappings from triggering updates.*/
+    pci_set_word(d->config + PCI_COMMAND, 0);
     memory_listener_register(&s->memory_listener, &address_space_memory);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,

Well, I can see this as something to be tried out as an experiment,

but it looks like you mean this to be a proper submission for inclusion upstream? Or maybe not, considering that qemu-devel wasn't even Cc-ed? In any case - what we need here is a general solution to at least the initialization part of the problem, i.e. all fields we emulate some or all bits for need to have d->config[] updated accordingly (i.e. you need to merge d->config[] and XenPTReg's data field based on the respective XenPTRegInfo's emu_mask, but perhaps simply copying the data field to

yet another problem). For the command register this for example means that it is in no way guaranteed that it would end up being zero; its emu_mask however guarantees that the memory and I/O decode bits would start out as zero (which is what you're after).

Jan

Quoted 10 lines · you

Quoted 10 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

hw/xen/xen_pt.c +2 -1

@@ -785,7 +785,9 @@ out:
         xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
                               pci_get_word(d->config + PCI_COMMAND) | cmd);
     }
-
+    /* Until the guest enables the device use d->config values which will
+     * inhibit pci_bar_address & pci_update_mappings from triggering updates.*/

Quoted 8 lines

hw/xen/xen_pt.c out: · line 793

Quoted 4 lines

hw/xen/xen_pt.c +1 -0

+    pci_set_word(d->config + PCI_COMMAND, 0);
     memory_listener_register(&s->memory_listener, &address_space_memory);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,

Well, I can see this as something to be tried out as an experiment, but it looks like you mean this to be a proper submission for inclusion upstream? Or maybe not, considering that qemu-devel

Quoted 6 lines

wasn't even Cc-ed? In any case - what we need here is a general solution to at least the initialization part of the problem, i.e. all fields we emulate some or all bits for need to have d->config[] updated accordingly (i.e. you need to merge d->config[] and XenPTReg's data field based on the respective XenPTRegInfo's emu_mask, but perhaps simply copying the data field to

d->>config[] would have the same effect; if it doesn't, we have

Quoted 4 lines

yet another problem). For the command register this for example means that it is in no way guaranteed that it would end up being zero; its emu_mask however guarantees that the memory and I/O decode bits would start out as zero (which is what you're after).

Just tested this patch together with a dom0 kernel which has the dropped kernel patch from Konrad:
22d8a8938407cb1342af763e937fdf9ee8daf24a xen/pciback: Don't disable PCI_COMMAND on PCI device reset.

And this patch does help to prevent the issue from happening.

--
Sander

Quoted 1 line

Jan

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

hw/xen/xen_pt.c out: · line 793

Quoted 10 lines

hw/xen/xen_pt.c +3 -1

@@ -785,7 +785,9 @@ out:
         xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
                               pci_get_word(d->config + PCI_COMMAND) | cmd);
     }
-
+    /* Until the guest enables the device use d->config values which will
+     * inhibit pci_bar_address & pci_update_mappings from triggering updates.*/
+    pci_set_word(d->config + PCI_COMMAND, 0);
     memory_listener_register(&s->memory_listener, &address_space_memory);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,

Well, I can see this as something to be tried out as an experiment, but it looks like you mean this to be a proper submission for inclusion upstream? Or maybe not, considering that qemu-devel wasn't even Cc-ed? In any case - what we need here is a general solution to at least the initialization part of the problem, i.e. all fields we emulate some or all bits for need to have d->config[]

updated accordingly (i.e. you need to merge d->config[] and XenPTReg's data field based on the respective XenPTRegInfo's emu_mask, but perhaps simply copying the data field to d->config[] would have the same effect; if it doesn't, we have yet another problem). For the command register this for example means that it is in no way guaranteed that it would end up being zero; its emu_mask however guarantees that the memory and I/O decode bits would start out as zero (which is what you're after).

Quoted 21 lines · Jan Beulich

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

hw/xen/xen_pt.c out: · line 793

Quoted 10 lines

hw/xen/xen_pt.c +3 -1

@@ -785,7 +785,9 @@ out:
         xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
                               pci_get_word(d->config + PCI_COMMAND) | cmd);
     }
-
+    /* Until the guest enables the device use d->config values which will
+     * inhibit pci_bar_address & pci_update_mappings from triggering updates.*/
+    pci_set_word(d->config + PCI_COMMAND, 0);
     memory_listener_register(&s->memory_listener, &address_space_memory);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,

Well, I can see this as something to be tried out as an experiment, but it looks like you mean this to be a proper submission for inclusion upstream? Or maybe not, considering that qemu-devel wasn't even Cc-ed? In any case - what we need here is a general solution to at least the initialization part of the problem, i.e. all fields we emulate some or all bits for need to have d->config[]

Quoted 8 lines

updated accordingly (i.e. you need to merge d->config[] and XenPTReg's data field based on the respective XenPTRegInfo's emu_mask, but perhaps simply copying the data field to d->config[] would have the same effect; if it doesn't, we have yet another problem). For the command register this for example means that it is in no way guaranteed that it would end up being zero; its emu_mask however guarantees that the memory and I/O decode bits would start out as zero (which is what you're after).

I am not sure I can ask Konrad to come up with a larger general solution when his intent was just to fix this issue. This one liner is OK for that.

Of course it would be great if Konrad wanted to volunteer to submit a patch that updates d->config for all fields, or gets rid of reg->data entirely.

Konrad, what do you think?

Quoted 1 line

Quoted 1 line · On 15.06.15 at 18:19, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

hw/xen/xen_pt.c out: · line 793

Quoted 10 lines

hw/xen/xen_pt.c +3 -1

@@ -785,7 +785,9 @@ out:
         xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
                               pci_get_word(d->config + PCI_COMMAND) | cmd);
     }
-
+    /* Until the guest enables the device use d->config values which will
+     * inhibit pci_bar_address & pci_update_mappings from triggering updates.*/
+    pci_set_word(d->config + PCI_COMMAND, 0);
     memory_listener_register(&s->memory_listener, &address_space_memory);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,

Well, I can see this as something to be tried out as an experiment, but it looks like you mean this to be a proper submission for inclusion upstream? Or maybe not, considering that qemu-devel wasn't even Cc-ed? In any case - what we need here is a general solution to at least the initialization part of the problem, i.e. all fields we emulate some or all bits for need to have d->config[]

Quoted 8 lines

updated accordingly (i.e. you need to merge d->config[] and XenPTReg's data field based on the respective XenPTRegInfo's emu_mask, but perhaps simply copying the data field to d->config[] would have the same effect; if it doesn't, we have yet another problem). For the command register this for example means that it is in no way guaranteed that it would end up being zero; its emu_mask however guarantees that the memory and I/O decode bits would start out as zero (which is what you're after).

I am not sure I can ask Konrad to come up with a larger general solution when his intent was just to fix this issue. This one liner is OK for that.

Quoted 32 lines

Quoted 1 line

Quoted 1 line · On 15.06.15 at 18:19, <[email protected]> wrote:

Quoted 21 lines · Jan Beulich

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 22:53, <[email protected]> wrote:

hw/xen/xen_pt.c out: · line 793

Quoted 10 lines

hw/xen/xen_pt.c +3 -1

@@ -785,7 +785,9 @@ out:
         xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
                               pci_get_word(d->config + PCI_COMMAND) | cmd);
     }
-
+    /* Until the guest enables the device use d->config values which will
+     * inhibit pci_bar_address & pci_update_mappings from triggering updates.*/
+    pci_set_word(d->config + PCI_COMMAND, 0);
     memory_listener_register(&s->memory_listener, &address_space_memory);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,

Well, I can see this as something to be tried out as an experiment, but it looks like you mean this to be a proper submission for inclusion upstream? Or maybe not, considering that qemu-devel wasn't even Cc-ed? In any case - what we need here is a general solution to at least the initialization part of the problem, i.e. all fields we emulate some or all bits for need to have d->config[]

Quoted 8 lines

updated accordingly (i.e. you need to merge d->config[] and XenPTReg's data field based on the respective XenPTRegInfo's emu_mask, but perhaps simply copying the data field to d->config[] would have the same effect; if it doesn't, we have yet another problem). For the command register this for example means that it is in no way guaranteed that it would end up being zero; its emu_mask however guarantees that the memory and I/O decode bits would start out as zero (which is what you're after).

Quoted 3 lines

I am not sure I can ask Konrad to come up with a larger general solution when his intent was just to fix this issue. This one liner is OK for that.

But apart from leaving the same issue around for all other fields the change above is just a hack anyway, going from one incorrect value (the host one) to another incorrect one (constant zero). What I'd consider acceptable as a partial solution would be if at least proper merging was done for this particular field. Yet I suppose once doing that, it's not going to be that much more work to do at least proper init-value merging for all fields. Doing the wider change of eliminating/replacing the data field would indeed seem to go too far for an immediate fix of the issue here.

Quoted 1 line

Quoted 1 line · On 24.06.15 at 17:59, <[email protected]> wrote:

While digging in this I realized that some shortcuts and assumptions had been taken (I think I am restating what you two have already realized).

1) The dev.config is (by Xen code) used as the cache of the host configuration devices (which is OK at init right now).

However to sync up the ->data with dev.config (and apply emu_mask) means that it cannot be used as such (the semantics of that have
changed).

2). The dev.config is (by the generic code) used as a view of what
the guest should see (cache of guest values).

To make this work, the plan would be:

1). Remove all the dev.config accesses to check host values:

	@@ -728,7 +729,7 @@ static int xen_pt_initfn(PCIDevice *d)
	     }
	 
	     /* Bind interrupt */
	-    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
	+    if (xen_host_pci_get_byte(PCI_INTERRUPT_PIN)) {
		 XEN_PT_LOG(d, "no pin interrupt\n");
		 goto out;
	     }
     And replace them with calls to get the actual host value.

2). Replace all the pci_get_[byte,word,long] (which are wrappers
around dev.config) in the init routines (see get_capability_version and xen_pt_linkctrl_reg_init) with calls to xen_host_pci_[byte,word,long].

In short - replace the calls to get the actual host values.

That would untangle a lot of this and make it a bit saner (I hope).

And after that I can:

3). Rip out ->data and use pci_set_[byte,word,long] or
pci_get_[byte,word,long]
to get one unified view of what the guest is suppose to have.

4). Tackle bugs that will creep up because of this. I am not sure what
they are, but surely will hit some. I expect that some of these patches will add debug/more logging to help me tackle this.

5). Reinstate an host cache (if needed) for configuration access to lessen
the amount of reads we do. 'host_cache_config' perhaps?

Quoted 30 lines

Quoted 1 line

Quoted 1 line · On 24.06.15 at 17:59, <[email protected]> wrote:

While digging in this I realized that some shortcuts and assumptions had been taken (I think I am restating what you two have already realized).

1) The dev.config is (by Xen code) used as the cache of the host configuration devices (which is OK at init right now).

However to sync up the ->data with dev.config (and apply emu_mask) means that it cannot be used as such (the semantics of that have
changed).

2). The dev.config is (by the generic code) used as a view of what
the guest should see (cache of guest values).

To make this work, the plan would be:

1). Remove all the dev.config accesses to check host values:

	@@ -728,7 +729,7 @@ static int xen_pt_initfn(PCIDevice *d)
	     }
	 
	     /* Bind interrupt */
	-    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
	+    if (xen_host_pci_get_byte(PCI_INTERRUPT_PIN)) {
		 XEN_PT_LOG(d, "no pin interrupt\n");
		 goto out;
	     }
     And replace them with calls to get the actual host value.

Quoted 7 lines

2). Replace all the pci_get_[byte,word,long] (which are wrappers
around dev.config) in the init routines (see get_capability_version and xen_pt_linkctrl_reg_init) with calls to xen_host_pci_[byte,word,long].

In short - replace the calls to get the actual host values.

That would untangle a lot of this and make it a bit saner (I hope).

Except that I don't think changing the init-time uses would really be necessary.

Quoted 10 lines

And after that I can:

3). Rip out ->data and use pci_set_[byte,word,long] or
pci_get_[byte,word,long]
to get one unified view of what the guest is suppose to have.

4). Tackle bugs that will creep up because of this. I am not sure what
they are, but surely will hit some. I expect that some of these patches will add debug/more logging to help me tackle this.

Quoted 2 lines

5). Reinstate an host cache (if needed) for configuration access to lessen
the amount of reads we do. 'host_cache_config' perhaps?

Not sure it's a good idea to cache registers that can change behind our back.

Quoted 34 lines

Quoted 1 line

Quoted 1 line · On 24.06.15 at 17:59, <[email protected]> wrote:

While digging in this I realized that some shortcuts and assumptions had been taken (I think I am restating what you two have already realized).

1) The dev.config is (by Xen code) used as the cache of the host configuration devices (which is OK at init right now).

However to sync up the ->data with dev.config (and apply emu_mask) means that it cannot be used as such (the semantics of that have
changed).

2). The dev.config is (by the generic code) used as a view of what
the guest should see (cache of guest values).

To make this work, the plan would be:

1). Remove all the dev.config accesses to check host values:

	@@ -728,7 +729,7 @@ static int xen_pt_initfn(PCIDevice *d)
	     }
	 
	     /* Bind interrupt */
	-    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
	+    if (xen_host_pci_get_byte(PCI_INTERRUPT_PIN)) {
		 XEN_PT_LOG(d, "no pin interrupt\n");
		 goto out;
	     }
     And replace them with calls to get the actual host value.

2). Replace all the pci_get_[byte,word,long] (which are wrappers
around dev.config) in the init routines (see get_capability_version and xen_pt_linkctrl_reg_init) with calls to xen_host_pci_[byte,word,long].

Quoted 3 lines

In short - replace the calls to get the actual host values.

That would untangle a lot of this and make it a bit saner (I hope).

Except that I don't think changing the init-time uses would really be necessary.

Quoted 7 lines

And after that I can:

3). Rip out ->data and use pci_set_[byte,word,long] or
pci_get_[byte,word,long]
to get one unified view of what the guest is suppose to have.

Quoted 6 lines

4). Tackle bugs that will creep up because of this. I am not sure what
they are, but surely will hit some. I expect that some of these patches will add debug/more logging to help me tackle this.

5). Reinstate an host cache (if needed) for configuration access to lessen
the amount of reads we do. 'host_cache_config' perhaps?

Not sure it's a good idea to cache registers that can change behind our back.

Jan

Quoted 1 line

Quoted 1 line · On 25.06.15 at 19:23, <[email protected]> wrote:

Quoted 36 lines · Jan Beulich

Quoted 36 lines

Quoted 1 line

Quoted 1 line · On 24.06.15 at 17:59, <[email protected]> wrote:

While digging in this I realized that some shortcuts and assumptions had been taken (I think I am restating what you two have already realized).

1) The dev.config is (by Xen code) used as the cache of the host configuration devices (which is OK at init right now).

However to sync up the ->data with dev.config (and apply emu_mask) means that it cannot be used as such (the semantics of that have
changed).

2). The dev.config is (by the generic code) used as a view of what
the guest should see (cache of guest values).

To make this work, the plan would be:

1). Remove all the dev.config accesses to check host values:

	@@ -728,7 +729,7 @@ static int xen_pt_initfn(PCIDevice *d)
	     }
	 
	     /* Bind interrupt */
	-    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
	+    if (xen_host_pci_get_byte(PCI_INTERRUPT_PIN)) {
		 XEN_PT_LOG(d, "no pin interrupt\n");
		 goto out;
	     }
     And replace them with calls to get the actual host value.

2). Replace all the pci_get_[byte,word,long] (which are wrappers
around dev.config) in the init routines (see get_capability_version and xen_pt_linkctrl_reg_init) with calls to xen_host_pci_[byte,word,long].

In short - replace the calls to get the actual host values.

Quoted 4 lines

Quoted 1 line

That would untangle a lot of this and make it a bit saner (I hope).

Except that I don't think changing the init-time uses would really be necessary.

Ah, but how would we go forward when dev.config is all we have - without any 'data'? We have to stash the init guest values somewhere.

Quoted 30 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 13:13, <[email protected]> wrote:

Quoted 18 lines · Jan Beulich

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 10.06.15 at 03:02, <[email protected]> wrote:

The problem is that the XSA120-addendm patch (which does not clear the PCI_COMMAND register anymore), causes an missing functionality in qemu-xen to be exposed. This missing functionality is implemented in qemu-traditional which is why it works there.

Quoted 11 lines

The problem is that qemu-xen for any write to the BAR regions updates them to the hypervisor - but only if the real hardware has them enabled (see pci_update_mappings in pci_default_write_config which is called by xen_pt_pci_write_config). Specifically pci_bar_address checks PCI_COMMAND register for PCI_COMMAND_MEMORY. If it is disabled (so no XSA-120 addendum patch), it returns -1 (default value resulting in no changes in the internal structures). If it is enabled, then it updates the d->config space with the value written by the guest. Once xen_pt_pci_write_config is done it syncs up the changes (if there are any) which results in the QEMU calling hypervisor to update the P2M mapping.

Quoted 9 lines

There's one fundamental aspect I'm not understanding here:
pci_update_mappings() / pci_bar_address() look at the guest view (or at least they ought to be), and the virtual command register starts out as zero. Is the bug perhaps that xen_pt_initfn(), after having initialized d->config[] via xen_host_pci_get_block(), leaves the command register at its host view value (rather than updating it alongside reg_entry->data in xen_pt_config_reg_init(), called via xen_pt_config_init()), which would have happened to be zero without that XSA-120 addendum?

Quoted 3 lines

It seems to me that Jan is right: setting the PCI_COMMAND register to ~PCI_COMMAND_MEMORY could be done at initialization time. Would that fix the bug?

Why ~PCI_COMMAND_MEMORY? Just like in the Xen specific data, this field should start out as zero.

Quoted 22 lines

Quoted 8 lines

It is of course concerning that
there are two (now clearly mismatching) guest views within qemu (and along those lines I also wonder whether the apparent duplicate maintenance of MSI and MSI-X state, due to pci_default_write_config() calling msi{,x}_write_config(), can do any good, or why the code uses pci_default_write_config() but not pci_default_read_config()).

Quoted 8 lines

It looks to me as if there was a halfhearted attempt to utilize infrastructure already available in qemu when these Xen pieces got added, leading to hard to understand issues like the one here. I.e. even if we addressed the initialization value issue above, there would still be two competing emulation layers potentially (and I suppose quite likely) leading to differing register state later on. Hence I wonder how many more issues there are (to come)...

The integration between the existing qemu-traditional code and the upstream QEMU code was hard. I am ready to believe there are more than just a few gaps and I would be happy to take further patches to improve the situation.

Quoted 5 lines

In this specific instance, are you referring to d->config, part of PCIDevice, and all the XenPTRegInfo instances? If so, I think the reason for having the latter, is that we need more flexibility, we need individual masks and read and write functions. At the same time we cannot really get rid of d->config.

I guessed as much, but in that case we should keep the two in sync (i.e. where we apply custom logic we should sync back what we do do d->config[], and at init time we should merge host and emulated state according to ->emu_mask; or maybe XenPTReg shouldn't even have a data field, and instead modifications should go straight to d->config[]). Perhaps a first step ought to be to log all cases where they differ?