Re: [Qemu-devel] [iGVT-g] [vfio-users] [PATCH v3 00/11] igd passthrough chipset tweaks

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Gerd Hoffmann <kraxel@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>
Cc: "igvt-g@ml01.01.org" <igvt-g@ml01.01.org>,
	"xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Cao jin <caoj.fnst@cn.fujitsu.com>,
	"vfio-users@redhat.com" <vfio-users@redhat.com>
Subject: Re: [Qemu-devel] [iGVT-g] [vfio-users] [PATCH v3 00/11] igd passthrough chipset	tweaks
Date: Tue, 2 Feb 2016 07:01:46 +0000	[thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D15F793FFA@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <1454330962.10168.34.camel@redhat.com>

> From: Gerd Hoffmann
> Sent: Monday, February 01, 2016 8:49 PM
> 
>   Hi,
> 
> > Thanks for the tip that seabios allocated pages automatically become
> > e820 reserved, that simplifies things a bit.
> 
> It's common practice for all firmware.  The e820 table from qemu is just
> a starting point, it is not passed on to the guest os as-is.  All
> permanent allocations (acpi tables, smbios tables, seabios driver data
> such as virtio rings, ...) are taken away from RAM and added to
> RESERVED, and IIRC seabios also takes care to reserve the bios and
> option rom regions in real mode address space.

Agree. It's cleaner to have seabios do allocation otherwise it's prune to
cause conflict if vfio-pci randomly picks up an address (though we can
do special tweaks within seabios to skip that one by reading 0xFC).

> 
> > > Maybe we should define the interface as "guest writes 0xfc to pick
> > > address, qemu takes care to place opregion there".  That gives us the
> > > freedom to change the qemu implementation (either copy host opregion or
> > > map the host opregion) without breaking things.
> >
> > Ok, so seabios allocates two pages, writes the base address of those
> > pages to 0xfc and looks to see whether the signature appears at that
> > address due to qemu mapping.  It verifies the size and does a
> > free/realloc if not the right size.
> 
> I think seabios first needs to reserve something big enough for a
> temporary mapping, to check signature + size, otherwise the opregion
> might scratch data structures beyond opregion in case it happens to be
> larger than 8k.
> 
> How likely is it that the opregion size ever changes?  Should we better
> be prepared to handle it?  Or would it be ok to have a ...
> 
>    if (opregion_size > 8k)
>       panic();
> 
> ... style sanity check?

Above sanity check should be enough. We use 8k in KVMGT too.

> 
> > If the graphics signature does not
> > appear, free those pages and assume no opregion support.
> 
> Yes.
> 
> > If we later
> > decide to use a copy, we'd need to disable the 0xfc automagic mapping
> > and probably pass the data via fw_cfg.  Sound right?
> 
> I'd have qemu copy the data on 0xfc write then, so things continue to
> work without updating seabios.  So, the firmware has to allocate space,
> reserve it etc.,  and programming the 0xfc register.  Qemu has to make
> sure the opregion appears at the address written by the firmware, by
> whatever method it prefers.

Yup. It's Qemu's responsibility to expose opregion content. 

btw, prefer to do copying here. It's pointless to allow write from guest
side. One write example is SWSCI mailbox, thru which gfx driver can
trigger some SCI event to communicate with BIOS (specifically ACPI
methods here), mostly for some monitor operations. However it's 
not a right thing for guest to trigger host SCI and thus kick host 
ACPI methods.

> 
> > > lpc bridge is no problem, only pci id fields are copied over and
> > > unprivileged access is allowed for them.
> > >
> > > Copying the gfx registers of the host bridge is a problem indeed.
> >
> > I would argue that both are really a problem, libvirt wants to put QEMU
> > in a container that prevents access to any host system files other than
> > those explicitly allowed.  Therefore libvirt needs to grant the process
> > access to the lpc sysfs config file even though it only needs user
> > visible register values.
> 
> Yes, correct.  We want svirt be as strict as possible.
> 

That is the most tricky part in IGD pass-thru, which is why Intel decides
to remove those dependencies to make pass-thru easier.

Thanks
Kevin