Re: [Qemu-devel] [PATCH] vfio/pci-quirks.c: Make stolen memory size adjustable for igd VFIO

2018-04-16 Thread Zhang, Xiong Y
> On Wed, 11 Apr 2018 13:24:10 +0800
> Xiong Zhang  wrote:
> 
> > Currenly linux guest with kernel above 3.19 couldn't boot up on igd
> > passthrough env. The root case is i915 driver use stolen memory, but
> > qemu vfio doesn't support it.
> >
> > This patch set stolen memory size to zero default, so guest i915 won't
> > use it. But this breaks old windows igd driver which will be unloaded
> > once it see zero stolen memory size. Then this patch also use
> > x-igd-gms option to adjust stolen memory size. New windows igd driver
> > fixes this and could work with zero stolen memory size.
> >
> > Finally with this patch, Linux guest and windows guest with igd driver
> > above 15.45.4860 could work successfully, windows guest with igd
> > driver below 15.45.4860 should add x-igd-gms=* option.
> 
> Sorry, I can't understand the above commit log in comparison to what this
> patch is actually doing.
> 
> > Signed-off-by: Xiong Zhang 
> > ---
> >  hw/vfio/pci-quirks.c | 95
> > 
> >  1 file changed, 52 insertions(+), 43 deletions(-)
> >
> > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index
> > 60ad5fb..6e9ed7f 100644
> > --- a/hw/vfio/pci-quirks.c
> > +++ b/hw/vfio/pci-quirks.c
> > @@ -1372,14 +1372,60 @@ static void
> vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> >  uint16_t cmd_orig, cmd;
> >  Error *err = NULL;
> >
> > +/* This must be an Intel VGA device. */
> > +if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> > +!vfio_is_vga(vdev) || nr != 4) {
> > +return;
> > +}
> > +
> >  /*
> > - * This must be an Intel VGA device at address 00:02.0 for us to even
> > - * consider enabling legacy mode.  The vBIOS has dependencies on
> the
> > - * PCI bus address.
> > + * IGD is not a standard, they like to change their specs often.  We
> > + * only attempt to support back to SandBridge and we hope that
> newer
> > + * devices maintain compatibility with generation 8.
> >   */
> > -if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> > -!vfio_is_vga(vdev) || nr != 4 ||
> > -&vdev->pdev !=
> pci_find_device(pci_device_root_bus(&vdev->pdev),
> > +gen = igd_gen(vdev);
> > +if (gen != 6 && gen != 8) {
> > +error_report("IGD device %s is unsupported by IGD quirks, "
> > + "try SandyBridge or newer",
> vdev->vbasedev.name);
> > +return;
> > +}
> > +
> > +/*
> > + * Regardless of running in UPT or legacy mode, the guest graphics
> > + * driver may attempt to use stolen memory, however only legacy
> mode
> > + * has BIOS support for reserving stolen memory in the guest VM.
> > + * Emulate the GMCH register in all cases and zero out the stolen
> > + * memory size here.
> > + */
> > +gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
> > +gmch &= ~((gen < 8 ? 0x1f : 0xff) << (gen < 8 ? 3 : 8));
> 
> Aha!  You want to enable stolen memory in either UPT or legacy mode.  I
> could not possibly have determined that from the commit log.  Please
> rewrite the commit log to describe what is actually changing here.
[Zhang, Xiong Y] Here, I want to disable stolen memory in UPT and legacy mode.
Sure, I will rewrite the commit log.
> 
> > +
> > +/*
> > + * Assume we have no GMS memory, but allow it to be overrided by
> device
> > + * option (experimental). Old windows igd driver must see non-zero
> stolen
> > + * memory size, otherwise it will be unloaded. New igd windows
> driver fix
> > + * this issue and could load with zero stolen memory size.
> > + */
> > +if (vdev->igd_gms) {
> > +if (vdev->igd_gms <= 0x10) {
> > +gms_mb = vdev->igd_gms * 32;
> > +gmch |= vdev->igd_gms << (gen < 8 ? 3 : 8);
> > +} else {
> > +error_report("Unsupported IGD GMS value 0x%x",
> vdev->igd_gms);
> > +vdev->igd_gms = 0;
> > +}
> > +}
> 
> You state in the previous code block that only legacy mode has BIOS support
> for reserving stolen memory, so why should UPT mode be allowed to specify
> stolen memory?  Where is it allocated?
[Zhang, Xiong Y] As the previous code disable stolen memory and set the size of 
stolen memory to zero, this cause 

Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Set non-zero GMS memory size for IGD

2017-07-27 Thread Zhang, Xiong Y
> > On 26 Jul 2017, at 08:22 AM, Zhang, Xiong Y 
> wrote:
> >
> > Sorry, we indeed found Intel windows guest graphic driver couldn't be bind
> when GMS memory size is zero. And we have fixed it and the next intel
> windows driver release will contain this fix.
> > So currently please use x-igd-gms in legacy mode to work around this.
> >
> > BTW, how did you know window driver allocate extra ~4G memory when
> GMS size set to 0 ?
> 
> We noticed that with new IGD driver memory usage on VMs raised by around
> 4G.
> 
> Then we examined memory allocations with poolmon
> (https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/poolm
> on)
> and saw that around 4G of memory is allocated with IGD driver pool tag.
> Additionally we noticed that on VMs with less than 5G of memory IGD driver
> does not load and system does fallback to standard VGA driver.
> 
[Zhang, Xiong Y]  I tried our internal driver and didn't found the above two 
issues, please wait for the next intel graphic driver release.

thanks



Re: [Qemu-devel] [PATCH] vfio/pci-quirks: Set non-zero GMS memory size for IGD

2017-07-25 Thread Zhang, Xiong Y
Sorry, we indeed found Intel windows guest graphic driver couldn't be bind when 
GMS memory size is zero. And we have fixed it and the next intel windows driver 
release will contain this fix.
So currently please use x-igd-gms in legacy mode to work around this. 

BTW, how did you know window driver allocate extra ~4G memory when GMS size set 
to 0 ?

thanks
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, July 25, 2017 11:32 PM
> To: Dmitry Fleytman 
> Cc: qemu-devel@nongnu.org; Zhang, Xiong Y 
> Subject: Re: [PATCH] vfio/pci-quirks: Set non-zero GMS memory size for IGD
> 
> [cc +Zhang, Xiong Y]
> 
> 
> Intel, any comments?
> 
> 
> On Mon, 24 Jul 2017 15:07:29 +0300
> Dmitry Fleytman  wrote:
> 
> > There is a claim that GMS memory is unused however
> > Intel Windows 10 drivers starting from V.4534 (10/7/2016)
> > allocate extra ~4G memory when GMS size set to 0.
> >
> > This patch fixes this issue by seting IGD GMS memory
> > size to minimum by changing default value of x-igd-gms
> > device parameter.
> >
> > Signed-off-by: Dmitry Fleytman 
> > ---
> >  hw/vfio/pci-quirks.c | 13 +
> >  hw/vfio/pci.c|  2 +-
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > index 349085e..197cb90 100644
> > --- a/hw/vfio/pci-quirks.c
> > +++ b/hw/vfio/pci-quirks.c
> > @@ -1527,10 +1527,15 @@ static void
> vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> >  }
> >
> >  /*
> > - * Assume we have no GMS memory, but allow it to be overrided by
> device
> > - * option (experimental).  The spec doesn't actually allow zero GMS
> when
> > - * when IVD (IGD VGA Disable) is clear, but the claim is that it's 
> > unused,
> > - * so let's not waste VM memory for it.
> > + * There is a claim that GMS memory is unused and we want to waste
> for it
> > + * as less VM memory as possible, however Intel Windows 10 drivers
> starting
> > + * from V.4534 (10/7/2016) allocate extra ~4G memory when GMS
> size set to 0.
> > + * The spec as well doesn't actually allow zero GMS when IVD
> > + * (IGD VGA Disable) is clear.
> > + *
> > + * Therefore we set GMS memory size to minimal by default via device
> > + * option x-igd-gms (experimental) and allow further tweaking of this
> > + * parameter.
> >   */
> >  gmch &= ~((gen < 8 ? 0x1f : 0xff) << (gen < 8 ? 3 : 8));
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index d4051cb..bda07b7 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2982,7 +2982,7 @@ static Property vfio_pci_dev_properties[] = {
> > sub_vendor_id, PCI_ANY_ID),
> >  DEFINE_PROP_UINT32("x-pci-sub-device-id", VFIOPCIDevice,
> > sub_device_id, PCI_ANY_ID),
> > -DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, igd_gms, 0),
> > +DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, igd_gms, 1),
> >  /*
> >   * TODO - support passed fds... is this necessary?
> >   * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),




Re: [Qemu-devel] [PATCH] Revert "vfio/pci-quirks.c: Disable stolen memory for igd VFIO"

2017-03-31 Thread Zhang, Xiong Y
> On Fri, 31 Mar 2017 19:03:49 +0200
> Igor Mammedov  wrote:
> 
> > On Thu, 30 Mar 2017 20:55:11 -0600
> > Alex Williamson  wrote:
> >
> > > On Fri, 31 Mar 2017 02:27:11 +
> > > "Zhang, Xiong Y"  wrote:
> > >
> > > > > On Thu, 30 Mar 2017 18:27:21 +0800
> > > > > Xiong Zhang  wrote:
> > > > >
> > > > > > This reverts commit
> c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > > > > >
> > > > > > The original patch intend to prevent linux i915 driver from using
> > > > > > stolen meory. But this patch breaks windows IGD driver loading on
> > > > > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows
> IGD
> > > > > > driver see zero size stolen memory, it will unload.
> > > > > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > > > > a guest.
> > > > >
> > > > > Does this mean that legacy mode IGD assignment is not going to work
> > > > > on Gen9+ with Windows?  Will it continue to work with Gen8-?
> > > > [Zhang, Xiong Y] I try to use the following qemu command to enable
> legacy mode on SKyLake, but It seems the entry point of wins IGD driver isn't
> called(I couldn't confirm this as I don't have the source code, but I didn't 
> see
> any IGD driver info from windbg while I could see many info in upt mode), so
> driver doesn't bind to IGD after win 8.1 boot up.
> > > >   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none
> -nographic -cpu host -hda "$IMAGE" -device
> vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> > > > Is this the right method to enable legacy mode ?
> > >
> > > Yeah, that should do it.  x-vga should not be necessary, but shouldn't
> > > hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
> > > trouble with the ROM if the host is booted in UEFI mode.
> >
> > 1.
> > here is dmesg messages when host is booted with CSM mode enabled
> > and host's bios load option rom on boot:
> >
> > [165041.359929] vfio-pci :00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> > [165073.898940] vfio-pci :00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> > [165074.687515] vfio-pci :00:02.0: enabling device (0400 -> 0403)
> > [165074.791598] vfio_ecap_init: :00:02.0 hiding ecap 0x1b@0x100
> >
> > have output on monitor connected via HDMI
> >
> > 2.
> > and here is dmesg in pure UEFI mode (where host's bios doesn't load option
> rom):
> >
> > [   21.034983] vfio-pci :00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> > [   22.025361] vfio_ecap_init: :00:02.0 hiding ecap 0x1b@0x100
> > [   22.030970] vfio-pci :00:02.0: Invalid PCI ROM header signature:
> expecting 0xaa55, got 0x
> > [   22.031036] vfio-pci :00:02.0: Invalid PCI ROM header signature:
> expecting 0xaa55, got 0x
> > [   24.738793] vfio-pci :00:02.0: Invalid PCI ROM header signature:
> expecting 0xaa55, got 0x
> > [   27.776904] vfio-pci :00:02.0: vgaarb: changed VGA decodes:
> olddecodes=io+mem,decodes=io+mem:owns=io+mem
> >
> > no output on monitor connected via HDMI when using Kabylake CPU
> (HD630)
> > but pure UEFI mode used to work (external output) with Skylake CPU
> (HD510)
> 
> In my limited experience, I've never been able to use IGD assignment on
> a pure UEFI host unless I provide the ROM via file.  For instance on my
> laptop I switched to CSM and booted a live CD to dump the ROM, which I
> can then use regardless of the host BIOS mode.
> 
> I just updated my rom-parser to include a version to fixup ROMs for
> vendor/device ID and checksum which should help with this:
> 
> https://github.com/awilliam/rom-parser
> 
> Update the device ID to match your ROM and let it fix the checksum.
> Maybe Xiong has some insight into why the VGA ROM often doesn't seem to
> exist on native pure UEFI hosts.
> 
[Zhang, Xiong Y] According to my limited UEFI knowledge, I could explain this, 
but it may be wrong.
VGA ROM seems a graphic driver in bios, and supply graphic service to grub and 
the early phase of OS.
The content of IGD VGA ROM is filled by system bios.
1. turn on csm, bios supply legacy bios intxx service to grub and OS. VGA ROM 
contain a 16 bit image as a vga bios. 
This image is what we want in vfio legacy mode. 
2. turn off csm, i

Re: [Qemu-devel] [PATCH] Revert "vfio/pci-quirks.c: Disable stolen memory for igd VFIO"

2017-03-31 Thread Zhang, Xiong Y
> On Fri, 31 Mar 2017 02:27:11 +
> "Zhang, Xiong Y"  wrote:
> 
> > > On Thu, 30 Mar 2017 18:27:21 +0800
> > > Xiong Zhang  wrote:
> > >
> > > > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> > > >
> > > > The original patch intend to prevent linux i915 driver from using
> > > > stolen meory. But this patch breaks windows IGD driver loading on
> > > > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > > > driver see zero size stolen memory, it will unload.
> > > > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > > > a guest.
> > >
> > > Does this mean that legacy mode IGD assignment is not going to work
> > > on Gen9+ with Windows?  Will it continue to work with Gen8-?
> > [Zhang, Xiong Y] I try to use the following qemu command to enable legacy
> mode on SKyLake, but It seems the entry point of wins IGD driver isn't 
> called(I
> couldn't confirm this as I don't have the source code, but I didn't see any 
> IGD
> driver info from windbg while I could see many info in upt mode), so driver
> doesn't bind to IGD after win 8.1 boot up.
> >   #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none
> -nographic -cpu host -hda "$IMAGE" -device
> vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
> > Is this the right method to enable legacy mode ?
> 
> Yeah, that should do it.  x-vga should not be necessary, but shouldn't
> hurt IIRC.  Any dmesg errors regarding the ROM?  I think we have
> trouble with the ROM if the host is booted in UEFI mode.
[Zhang, Xiong Y] My host boot in legacy bios mode. After adding x-igd-gms in 
legacy mode, win 8.1 IGD driver could bind to IGD, and win 8.1 runs good. 
thanks
> 
> >
> > > Please clarify Gen9+, is this Kaby Lake?
> > [Zhang, Xiong Y] Gen 9+ is SkyLake and later.
> 
> Ok, then I cannot test since I only have access to BDW.  We do have
> users that might start complaining if this is a new change in the
> Windows driver for SKL+.
> 
> > > I assume this patch is intended for QEMU 2.9, it's helpful to make that
> > > explicit during the rc freeze.  Thanks,
> > [Zhang, Xiong Y] Yes, as the original patch has entered into Qemu 2.9 rc1. 
> > So
> this reverted patch should be entered into the later 2.9 rc.
> > Sorry for the troubles.
> 
> Ok, no problem.  Thanks,
> 
> Alex




Re: [Qemu-devel] [PATCH] Revert "vfio/pci-quirks.c: Disable stolen memory for igd VFIO"

2017-03-30 Thread Zhang, Xiong Y
> On Thu, 30 Mar 2017 18:27:21 +0800
> Xiong Zhang  wrote:
> 
> > This reverts commit c2b2e158cc7b1cb431bd6039824ec13c3184a775.
> >
> > The original patch intend to prevent linux i915 driver from using
> > stolen meory. But this patch breaks windows IGD driver loading on
> > Gen9+, as IGD HW will use stolen memory on Gen9+, once windows IGD
> > driver see zero size stolen memory, it will unload.
> > Meanwhile stolen memory will be disabled in 915 when i915 run as
> > a guest.
> 
> Does this mean that legacy mode IGD assignment is not going to work
> on Gen9+ with Windows?  Will it continue to work with Gen8-?
[Zhang, Xiong Y] I try to use the following qemu command to enable legacy mode 
on SKyLake, but It seems the entry point of wins IGD driver isn't called(I 
couldn't confirm this as I don't have the source code, but I didn't see any IGD 
driver info from windbg while I could see many info in upt mode), so driver 
doesn't bind to IGD after win 8.1 boot up.
  #qemu-system-x86_64 -M pc -enable-kvm -smp 2 -m 2G  -vga none -nographic -cpu 
host -hda "$IMAGE" -device 
vfio-pci,host=00:02.0,x-vga=true,id=hostdev0,bus=pci.0,addr=0x2
Is this the right method to enable legacy mode ?

> Please clarify Gen9+, is this Kaby Lake?
[Zhang, Xiong Y] Gen 9+ is SkyLake and later.

> I assume this patch is intended for QEMU 2.9, it's helpful to make that
> explicit during the rc freeze.  Thanks,
[Zhang, Xiong Y] Yes, as the original patch has entered into Qemu 2.9 rc1. So 
this reverted patch should be entered into the later 2.9 rc.
Sorry for the troubles.




Re: [Qemu-devel] [PATCH] vfio/pci-quirks.c: Disable stolen memory for igd VFIO

2017-02-20 Thread Zhang, Xiong Y
> On Mon, 20 Feb 2017 19:42:54 +0800
> Xiong Zhang  wrote:
> 
> > From: XiongZhang 
> >
> > If IGD isn't assigned at 00:02.0 in UPT and host bios enable stolen
> > memory, seabios won't reseave stolen memory in E820 for guest. Then
> > both Intel graphic driver and others in guest could use stolen
> > memory, this will generate system hang. So we should disable stolen
> > memory in this case.
> 
> Wasn't the intent of UPT mode that it removed all of the BIOS and
> chipset dependencies of IGD such that it could be assigned as just
> another PCI device?  Does this mean that the drivers fail to meet that
> promise by evaluating the size and location of stolen memory as
> programmed on the physical device even in UPT mode?
[Zhang, Xiong Y] The intent of UPT mode is correct. Driver also evaluate
the size and location of stolen memory correctly.
The current problem is: when IGD isn't at 00:02.0, seabios don't create memory
region and reserve memory resource in E820 for stolen memory.
So guest OS maybe assign stolen memory MMIO to other devices, when IGD driver
access stolen memory, it access the wrong device and cause system error. 
If guest OS don't assign stolen memory MMIO to other devices, then there
isn't gpa to hpa translate for stolen memory, guest IGD driver couldn't
access it.
> 
> I'm a little confused by the use of the term "others" here and in the
> comment below.  Can you be more specific what other software beyond the
> graphics driver is evaluating the size or location of stolen memory?
> 
> > Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=99028
> >  https://bugs.freedesktop.org/show_bug.cgi?id=99025
> >
> > Signed-off-by: Xiong Zhang 
> > Tested-by: Terrence Xu 
> > ---
> >  hw/vfio/pci-quirks.c | 63
> ++--
> >  1 file changed, 36 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > index 1e97bc4..015d0c2 100644
> > --- a/hw/vfio/pci-quirks.c
> > +++ b/hw/vfio/pci-quirks.c
> > @@ -1364,14 +1364,43 @@ static void
> vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> >  uint32_t gmch;
> >  uint16_t cmd_orig, cmd;
> >
> > +/* This must be an Intel VGA device. */
> > +if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> > +!vfio_is_vga(vdev) || nr != 4) {
> > +return;
> > +}
> > +
> >  /*
> > - * This must be an Intel VGA device at address 00:02.0 for us to even
> > - * consider enabling legacy mode.  The vBIOS has dependencies on
> the
> > - * PCI bus address.
> > + * IGD is not a standard, they like to change their specs often.  We
> > + * only attempt to support back to SandBridge and we hope that
> newer
> > + * devices maintain compatibility with generation 8.
> >   */
> > -if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> > -!vfio_is_vga(vdev) || nr != 4 ||
> > -&vdev->pdev !=
> pci_find_device(pci_device_root_bus(&vdev->pdev),
> > +gen = igd_gen(vdev);
> > +if (gen != 6 && gen != 8) {
> > +error_report("IGD device %s is unsupported in legacy mode, "
> > + "try SandyBridge or newer",
> vdev->vbasedev.name);
> 
> This is a little bit misleading now since this is no longer exclusively
> a legacy mode path, a user trying to use UPT mode might disregard this
> as noise.  Perhaps...
> 
> error_report("IGD device %s is unsupported by IGD quirks, "
>  "try SandyBridge or newer", vdev->vbasedev.name);
> 
[Zhang, Xiong Y] yes, I will follow it.
> 
> > +return;
> > +}
> > +/*
> > + * If this isn't at address 00:02.0, bios won't reserv stolen
> 
> s/reserv/reserve/
> 
> > + * memory in E820, then others could use stolen memory. If guest
> > + * graphic driver still use stolen memory, system maybe hang.
> > + * so we set stolen memory size to 0 and guest graphic driver won't
> > + * use stolen memory.
> 
> Based on my understanding of the bug, I might suggest:
> 
>   Regardless of running in UPT or legacy mode, the guest graphics
>   driver may attempt to use stolen memory, however only legacy mode has
>   BIOS support for reserving stolen memory in the guest VM.  Emulate
>   the GMCH register in all cases and zero out the stolen memory size
>   here.  Legacy mode may request allocation and re-write this below.
> 
[Zhang, Xion

[Qemu-devel] Recall: [PATCH] vfio/pci-quirks.c: Disable stolen memory for igd VFIO

2017-02-20 Thread Zhang, Xiong Y
Zhang, Xiong Y would like to recall the message, "[Qemu-devel] [PATCH] 
vfio/pci-quirks.c: Disable stolen memory for igd VFIO".


Re: [Qemu-devel] [PATCH] vfio/pci-quirks.c: Disable stolen memory for igd VFIO

2017-02-20 Thread Zhang, Xiong Y
> 
> On Mon, 20 Feb 2017 19:42:54 +0800
> Xiong Zhang  wrote:
> 
> > From: XiongZhang 
> >
> > If IGD isn't assigned at 00:02.0 in UPT and host bios enable stolen
> > memory, seabios won't reseave stolen memory in E820 for guest. Then
> > both Intel graphic driver and others in guest could use stolen
> > memory, this will generate system hang. So we should disable stolen
> > memory in this case.
> 
> Wasn't the intent of UPT mode that it removed all of the BIOS and
> chipset dependencies of IGD such that it could be assigned as just
> another PCI device?  Does this mean that the drivers fail to meet that
> promise by evaluating the size and location of stolen memory as
> programmed on the physical device even in UPT mode?
[Zhang, Xiong Y] The intent of UPT mode is correct. Driver also evaluate
the size and location of stolen memory correctly.
The current problem is: when IGD isn't at 00:02.0, seabios don't create memory
region and reserve memory resource in E820 for stolen memory.
So guest OS maybe assign stolen memory MMIO to other devices, when IGD driver
access stolen memory, it access the wrong device and cause system error. 
If guest OS don't assign stolen memory MMIO to other devices, then there
isn't gpa to hpa translate for stolen memory, guest IGD driver couldn't
access it. 
> 
> I'm a little confused by the use of the term "others" here and in the
> comment below.  Can you be more specific what other software beyond the
> graphics driver is evaluating the size or location of stolen memory?
> 
> > Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=99028
> >  https://bugs.freedesktop.org/show_bug.cgi?id=99025
> >
> > Signed-off-by: Xiong Zhang 
> > Tested-by: Terrence Xu 
> > ---
> >  hw/vfio/pci-quirks.c | 63
> ++--
> >  1 file changed, 36 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > index 1e97bc4..015d0c2 100644
> > --- a/hw/vfio/pci-quirks.c
> > +++ b/hw/vfio/pci-quirks.c
> > @@ -1364,14 +1364,43 @@ static void
> vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> >  uint32_t gmch;
> >  uint16_t cmd_orig, cmd;
> >
> > +/* This must be an Intel VGA device. */
> > +if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> > +!vfio_is_vga(vdev) || nr != 4) {
> > +return;
> > +}
> > +
> >  /*
> > - * This must be an Intel VGA device at address 00:02.0 for us to even
> > - * consider enabling legacy mode.  The vBIOS has dependencies on
> the
> > - * PCI bus address.
> > + * IGD is not a standard, they like to change their specs often.  We
> > + * only attempt to support back to SandBridge and we hope that
> newer
> > + * devices maintain compatibility with generation 8.
> >   */
> > -if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> > -!vfio_is_vga(vdev) || nr != 4 ||
> > -&vdev->pdev !=
> pci_find_device(pci_device_root_bus(&vdev->pdev),
> > +gen = igd_gen(vdev);
> > +if (gen != 6 && gen != 8) {
> > +error_report("IGD device %s is unsupported in legacy mode, "
> > + "try SandyBridge or newer",
> vdev->vbasedev.name);
> 
> This is a little bit misleading now since this is no longer exclusively
> a legacy mode path, a user trying to use UPT mode might disregard this
> as noise.  Perhaps...
> 
> error_report("IGD device %s is unsupported by IGD quirks, "
>  "try SandyBridge or newer", vdev->vbasedev.name);
> 
> 
> > +return;
> > +}
> > +/*
> > + * If this isn't at address 00:02.0, bios won't reserv stolen
> 
> s/reserv/reserve/
> 
> > + * memory in E820, then others could use stolen memory. If guest
> > + * graphic driver still use stolen memory, system maybe hang.
> > + * so we set stolen memory size to 0 and guest graphic driver won't
> > + * use stolen memory.
> 
> Based on my understanding of the bug, I might suggest:
> 
>   Regardless of running in UPT or legacy mode, the guest graphics
>   driver may attempt to use stolen memory, however only legacy mode has
>   BIOS support for reserving stolen memory in the guest VM.  Emulate
>   the GMCH register in all cases and zero out the stolen memory size
>   here.  Legacy mode may request allocation and re-write this below.
> 
> > + */
> > +gmch = vf