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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 10 Jun 2015, Jan Beulich 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 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?

Yes, I think we could substitute the data field in XenPTReg with a
pointer to the right place in d->config. Or we could just get rid of it
and introduce a macro to access d->config + reg->offset.

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