[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Is: qemu-xen mishandling upper 64-bit BAR compared to qemu-tradWas:Re: Dom0 linux 4.0 + devel/for-linus-4.1 branch: p2m.c:884:d0v0 gfn_to_mfn failed! gfn=ffffffff001ed type:4
On June 10, 2015 8:02:52 AM EDT, Jan Beulich <JBeulich@xxxxxxxx> wrote: >>>> On 10.06.15 at 13:13, <stefano.stabellini@xxxxxxxxxxxxx> wrote: >> On Wed, 10 Jun 2015, Jan Beulich wrote: >>> >>> On 10.06.15 at 03:02, <konrad.wilk@xxxxxxxxxx> 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. >>> >>> 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? > >Why ~PCI_COMMAND_MEMORY? Just like in the Xen specific data, >this field should start out as zero. It should fix it. I will send out a patch later tonight. > >>> 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)... >> >> 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. > >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? > Let me cobble a patch to this effect and see what falls out. Thank you for the feedback! >Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |