[PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

This is hvmloader part of the change that gets rid of the resource
conflict warning in the guest kernel.
The OpRegion may not always be page aligned.
As a result one extra page is required to fully accommodate the
OpRegion in that case.
Just reserve one more page here.

Signed-off-by: Timothy Guo <firemeteor@users.sourceforge.net>

diff -r 11b4bc743b1f tools/firmware/hvmloader/e820.c
--- a/tools/firmware/hvmloader/e820.c    Mon Dec 17 14:59:11 2012 +0000
+++ b/tools/firmware/hvmloader/e820.c    Thu Dec 20 00:07:40 2012 +0800
@@ -142,11 +142,11 @@ int build_e820_table(struct e820entry *e
         nr++;

         e820[nr].addr = igd_opregion_base;
-        e820[nr].size = 2 * PAGE_SIZE;
+        e820[nr].size = 3 * PAGE_SIZE;
         e820[nr].type = E820_NVS;
         nr++;

-        e820[nr].addr = igd_opregion_base + 2 * PAGE_SIZE;
+        e820[nr].addr = igd_opregion_base + 3 * PAGE_SIZE;
         e820[nr].size = (uint32_t)-e820[nr].addr;
         e820[nr].type = E820_RESERVED;
         nr++;
diff -r 11b4bc743b1f tools/firmware/hvmloader/pci.c
--- a/tools/firmware/hvmloader/pci.c    Mon Dec 17 14:59:11 2012 +0000
+++ b/tools/firmware/hvmloader/pci.c    Thu Dec 20 00:07:40 2012 +0800
@@ -98,7 +98,7 @@ void pci_setup(void)
                 virtual_vga = VGA_pt;
                 if ( vendor_id == 0x8086 )
                 {
-                    igd_opregion_pgbase = mem_hole_alloc(2);
+                    igd_opregion_pgbase = mem_hole_alloc(3);
                     /*
                      * Write the the OpRegion offset to give the opregion
                      * address to the device model. The device model will trap


This is qemu-xen part of the change that gets rid of the resource
conflict warning in the guest kernel.
If the host OpRegion is page aligned, two pages will be sufficient.
Otherwise, we need to map one more page to have it fully accommodated
-- the guest kernel would map this extra page and complain about
resource conflict.

Signed-off-by: Timothy Guo <firemeteor@users.sourceforge.net>

diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index c6f8869..3f2b285 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -81,6 +81,7 @@ uint32_t igd_read_opregion(struct pt_dev *pci_dev)
 void igd_write_opregion(struct pt_dev *real_dev, uint32_t val)
 {
     uint32_t host_opregion = 0;
+    uint32_t map_size = 2;
     int ret;

     if ( igd_guest_opregion )
@@ -74,11 +93,13 @@ void igd_write_opregion(struct pt_dev *real_dev,
uint32_t val)
     host_opregion = pt_pci_host_read(real_dev->pci_dev, PCI_INTEL_OPREGION, 4);
     igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);
     PT_LOG("Map OpRegion: %x -> %x\n", host_opregion, igd_guest_opregion);
+    //If the opregion is not page-aligned, map one more page to fit
the entire region.
+    map_size += (host_opregion & 0xfff) != 0;

     ret = xc_domain_memory_mapping(xc_handle, domid,
             igd_guest_opregion >> XC_PAGE_SHIFT,
             host_opregion >> XC_PAGE_SHIFT,
-            2,
+            map_size,
             DPCI_ADD_MAPPING);

     if ( ret != 0 )

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

Switch to a new address that can reach to Jean.

On Thu, Dec 20, 2012 at 11:52 AM, G.R. <firemeteor@users.sourceforge.net> wrote:
> This is hvmloader part of the change that gets rid of the resource
> conflict warning in the guest kernel.
> The OpRegion may not always be page aligned.
> As a result one extra page is required to fully accommodate the
> OpRegion in that case.
> Just reserve one more page here.
>
> Signed-off-by: Timothy Guo <firemeteor@users.sourceforge.net>
>
> diff -r 11b4bc743b1f tools/firmware/hvmloader/e820.c
> --- a/tools/firmware/hvmloader/e820.c    Mon Dec 17 14:59:11 2012 +0000
> +++ b/tools/firmware/hvmloader/e820.c    Thu Dec 20 00:07:40 2012 +0800
> @@ -142,11 +142,11 @@ int build_e820_table(struct e820entry *e
>          nr++;
>
>          e820[nr].addr = igd_opregion_base;
> -        e820[nr].size = 2 * PAGE_SIZE;
> +        e820[nr].size = 3 * PAGE_SIZE;
>          e820[nr].type = E820_NVS;
>          nr++;
>
> -        e820[nr].addr = igd_opregion_base + 2 * PAGE_SIZE;
> +        e820[nr].addr = igd_opregion_base + 3 * PAGE_SIZE;
>          e820[nr].size = (uint32_t)-e820[nr].addr;
>          e820[nr].type = E820_RESERVED;
>          nr++;
> diff -r 11b4bc743b1f tools/firmware/hvmloader/pci.c
> --- a/tools/firmware/hvmloader/pci.c    Mon Dec 17 14:59:11 2012 +0000
> +++ b/tools/firmware/hvmloader/pci.c    Thu Dec 20 00:07:40 2012 +0800
> @@ -98,7 +98,7 @@ void pci_setup(void)
>                  virtual_vga = VGA_pt;
>                  if ( vendor_id == 0x8086 )
>                  {
> -                    igd_opregion_pgbase = mem_hole_alloc(2);
> +                    igd_opregion_pgbase = mem_hole_alloc(3);
>                      /*
>                       * Write the the OpRegion offset to give the opregion
>                       * address to the device model. The device model will trap
>
>
> This is qemu-xen part of the change that gets rid of the resource
> conflict warning in the guest kernel.
> If the host OpRegion is page aligned, two pages will be sufficient.
> Otherwise, we need to map one more page to have it fully accommodated
> -- the guest kernel would map this extra page and complain about
> resource conflict.
>
> Signed-off-by: Timothy Guo <firemeteor@users.sourceforge.net>
>
> diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
> index c6f8869..3f2b285 100644
> --- a/hw/pt-graphics.c
> +++ b/hw/pt-graphics.c
> @@ -81,6 +81,7 @@ uint32_t igd_read_opregion(struct pt_dev *pci_dev)
>  void igd_write_opregion(struct pt_dev *real_dev, uint32_t val)
>  {
>      uint32_t host_opregion = 0;
> +    uint32_t map_size = 2;
>      int ret;
>
>      if ( igd_guest_opregion )
> @@ -74,11 +93,13 @@ void igd_write_opregion(struct pt_dev *real_dev,
> uint32_t val)
>      host_opregion = pt_pci_host_read(real_dev->pci_dev, PCI_INTEL_OPREGION, 4);
>      igd_guest_opregion = (val & ~0xfff) | (host_opregion & 0xfff);
>      PT_LOG("Map OpRegion: %x -> %x\n", host_opregion, igd_guest_opregion);
> +    //If the opregion is not page-aligned, map one more page to fit
> the entire region.
> +    map_size += (host_opregion & 0xfff) != 0;
>
>      ret = xc_domain_memory_mapping(xc_handle, domid,
>              igd_guest_opregion >> XC_PAGE_SHIFT,
>              host_opregion >> XC_PAGE_SHIFT,
> -            2,
> +            map_size,
>              DPCI_ADD_MAPPING);
>
>      if ( ret != 0 )

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ian Campbell]

Adding our qemu maintainer.

On Thu, 2012-12-20 at 03:56 +0000, G.R. wrote:
> Switch to a new address that can reach to Jean.
> 
> On Thu, Dec 20, 2012 at 11:52 AM, G.R. <firemeteor@users.sourceforge.net> wrote:
> > This is hvmloader part of the change that gets rid of the resource
> > conflict warning in the guest kernel.
> > The OpRegion may not always be page aligned.

Is it worth detecting this and allocating 2 or 3 pages as required?

The OpRegion is always 8096 bytes? (two pages, but not necessarily
aligned)?

Do we need to worry about what is in the "slop" at either end of a 3
page region containing this? If they are sensitive registers then we may
have a problem.

Ian.

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Keir Fraser]

On 20/12/2012 10:41, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:

> Adding our qemu maintainer.
> 
> On Thu, 2012-12-20 at 03:56 +0000, G.R. wrote:
>> Switch to a new address that can reach to Jean.
>> 
>> On Thu, Dec 20, 2012 at 11:52 AM, G.R. <firemeteor@users.sourceforge.net>
>> wrote:
>>> This is hvmloader part of the change that gets rid of the resource
>>> conflict warning in the guest kernel.
>>> The OpRegion may not always be page aligned.
> 
> Is it worth detecting this and allocating 2 or 3 pages as required?
> 
> The OpRegion is always 8096 bytes? (two pages, but not necessarily
> aligned)?
> 
> Do we need to worry about what is in the "slop" at either end of a 3
> page region containing this? If they are sensitive registers then we may
> have a problem.

In the hvmloader patch it is not worth it I think, one extra page of memory
hole is hardly a scarce resource.

I don't know whether the qemu side is accurate enough. If the region is 8096
bytes then it is not necessarily the case that an unaligned start address
means we need three pages mapped.

 -- Keir

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

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

On Thu, Dec 20, 2012 at 9:03 PM, Keir Fraser <keir@xen.org> wrote:
> On 20/12/2012 10:41, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
>
>> Adding our qemu maintainer.
>>
>> On Thu, 2012-12-20 at 03:56 +0000, G.R. wrote:
>>> Switch to a new address that can reach to Jean.
>>>
>>> On Thu, Dec 20, 2012 at 11:52 AM, G.R. <firemeteor@users.sourceforge.net>
>>> wrote:
>>>> This is hvmloader part of the change that gets rid of the resource
>>>> conflict warning in the guest kernel.
>>>> The OpRegion may not always be page aligned.
>>
>> Is it worth detecting this and allocating 2 or 3 pages as required?
>>
>> The OpRegion is always 8096 bytes? (two pages, but not necessarily
>> aligned)?
>>
>> Do we need to worry about what is in the "slop" at either end of a 3
>> page region containing this? If they are sensitive registers then we may
>> have a problem.
>
> In the hvmloader patch it is not worth it I think, one extra page of memory
> hole is hardly a scarce resource.
>
> I don't know whether the qemu side is accurate enough. If the region is 8096
> bytes then it is not necessarily the case that an unaligned start address
> means we need three pages mapped.
>

According to the spec table 2.1, the length is fixed to 8096 bytes.
http://intellinuxgraphics.org/ACPI_IGD_OpRegion_%20Spec.pdf

768 (0x300) byte from the tail is reserved.
If the page offset larger than 0x300, the domU may not work correctly.
Otherwise, it's only a warning in the domU kernel log, which looks a
little scary. (And the hvmloader patch should be enough to suppress
it)

If concern is about security, the same argument should apply to the
first page (the portion before the page offset).
The problem is that I have no idea what is around the mapped page. Not
sure who has the knowledge.

What's the standard flow to handle such map with offset?
I expect this to be a common case, since the ioremap function in linux
kernel accept this.

In my case, the host opregion is reported at 0xcd996018, which comes
from a much larger 'ACPI NVS' region as shown below:

(XEN) Xen-e820 RAM map:
(XEN)  0000000000000000 - 000000000009ec00 (usable)
(XEN)  000000000009ec00 - 00000000000a0000 (reserved)
(XEN)  00000000000e0000 - 0000000000100000 (reserved)
(XEN)  0000000000100000 - 0000000020000000 (usable)
(XEN)  0000000020000000 - 0000000020200000 (reserved)
(XEN)  0000000020200000 - 0000000040004000 (usable)
(XEN)  0000000040004000 - 0000000040005000 (reserved)
(XEN)  0000000040005000 - 00000000cd103000 (usable)
(XEN)  00000000cd103000 - 00000000cd87b000 (reserved)
(XEN)  00000000cd87b000 - 00000000cd907000 (usable)
(XEN)  00000000cd907000 - 00000000cd9a8000 (ACPI NVS)      <======
(XEN)  00000000cd9a8000 - 00000000ce150000 (reserved)
(XEN)  00000000ce150000 - 00000000ce151000 (usable)
(XEN)  00000000ce151000 - 00000000ce194000 (ACPI NVS)
(XEN)  00000000ce194000 - 00000000cec15000 (usable)
(XEN)  00000000cec15000 - 00000000ceff2000 (reserved)
(XEN)  00000000ceff2000 - 00000000cf000000 (usable)
(XEN)  00000000cf800000 - 00000000dfa00000 (reserved)
(XEN)  00000000f8000000 - 00000000fc000000 (reserved)
(XEN)  00000000fec00000 - 00000000fec01000 (reserved)
(XEN)  00000000fed00000 - 00000000fed04000 (reserved)
(XEN)  00000000fed1c000 - 00000000fed20000 (reserved)
(XEN)  00000000fee00000 - 00000000fee01000 (reserved)
(XEN)  00000000ff000000 - 0000000100000000 (reserved)
(XEN)  0000000100000000 - 000000021f600000 (usable)
(XEN) ACPI: RSDP 000F0490, 0024 (r2 ALASKA)
(XEN) ACPI: XSDT CD999080, 0084 (r1 ALASKA    A M I  1072009 AMI     10013)
(XEN) ACPI: FACP CD9A28C0, 010C (r5 ALASKA    A M I  1072009 AMI     10013)
(XEN) ACPI Warning (tbfadt-0232): FADT (revision 5) is longer than
ACPI 2.0 version, truncating length 0x10C to 0xF4 [20070126]
(XEN) ACPI: DSDT CD9991A0, 971F (r2 ALASKA    A M I       22 INTL 20051117)
(XEN) ACPI: FACS CD9A6080, 0040
(XEN) ACPI: APIC CD9A29D0, 0092 (r3 ALASKA    A M I  1072009 AMI     10013)
(XEN) ACPI: FPDT CD9A2A68, 0044 (r1 ALASKA    A M I  1072009 AMI     10013)
(XEN) ACPI: ASF! CD9A2AB0, 00A5 (r32 INTEL       HCG        1 TFSM    F4240)
(XEN) ACPI: MCFG CD9A2B58, 003C (r1 ALASKA    A M I  1072009 MSFT       97)
(XEN) ACPI: AAFT CD9A2B98, 00EA (r1 ALASKA OEMAAFT   1072009 MSFT       97)
(XEN) ACPI: HPET CD9A2C88, 0038 (r1 ALASKA    A M I  1072009 AMI.        5)
(XEN) ACPI: SSDT CD9A2CC0, 036D (r1 SataRe SataTabl     1000 INTL 20091112)
(XEN) ACPI: SSDT CD9A3030, 09AA (r1  PmRef  Cpu0Ist     3000 INTL 20051117)
(XEN) ACPI: SSDT CD9A39E0, 0A92 (r1  PmRef    CpuPm     3000 INTL 20051117)
(XEN) ACPI: DMAR CD9A4478, 00B8 (r1 INTEL      SNB         1 INTL        1)
(XEN) ACPI: BGRT CD9A4530, 0038 (r0 ALASKA    A M I  1072009 AMI     10013)
(XEN) System RAM: 7887MB (8077040kB)

Thanks,
Timothy

>  -- Keir
>
>> Ian.
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Keir Fraser]

On 20/12/2012 13:31, "G.R." <firemeteor@users.sourceforge.net> wrote:

> If concern is about security, the same argument should apply to the
> first page (the portion before the page offset).
> The problem is that I have no idea what is around the mapped page. Not
> sure who has the knowledge.

Well we can't do better than mapping some whole number of pages, really.
Unless we trap to qemu on every access. I don't think we'd go there unless
there really were a known security issue. But mapping only the exact number
of pages we definitely need is a good principle.

> What's the standard flow to handle such map with offset?
> I expect this to be a common case, since the ioremap function in linux
> kernel accept this.

map_size = ((host_opregion & 0xfff) + 8096 + 0xfff) >> 12

Possibly with suitable macros used instead of magic numbers (e.g., XC_PAGE_*
and a macro for the opregion size).

 -- Keir

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

On Thu, Dec 20, 2012 at 10:19 PM, Keir Fraser <keir@xen.org> wrote:
> On 20/12/2012 13:31, "G.R." <firemeteor@users.sourceforge.net> wrote:
>
>> If concern is about security, the same argument should apply to the
>> first page (the portion before the page offset).
>> The problem is that I have no idea what is around the mapped page. Not
>> sure who has the knowledge.
>
> Well we can't do better than mapping some whole number of pages, really.
> Unless we trap to qemu on every access. I don't think we'd go there unless
> there really were a known security issue. But mapping only the exact number
> of pages we definitely need is a good principle.
>
>> What's the standard flow to handle such map with offset?
>> I expect this to be a common case, since the ioremap function in linux
>> kernel accept this.
>
> map_size = ((host_opregion & 0xfff) + 8096 + 0xfff) >> 12
>
Keir, I believe this expression should give the same result.
First of all, 8096 should be 8192 :-), and this part should result in
an constant number 2 after right shift.
The remaining part is ((host_opregion & 0xfff) + 0xfff) >> 12
As long as the first sub-expression is non zero, the result of the add
should range from [0x1000, 0x1ffe].
And this will yield a result '1' after the right shift.
So as long as there is known security risk (which I'm not sure about),
the patch should be fine.

> Possibly with suitable macros used instead of magic numbers (e.g., XC_PAGE_*
> and a macro for the opregion size).

I guess there is no predefined macro for OpRegion size. And I guess I
need to define it twice for both code?

>  -- Keir
>
>

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of G.R.
> Sent: Thursday, December 20, 2012 10:06 AM
> To: Keir (Xen.org)
> Cc: Stefano Stabellini; Ian Campbell; Jean.guyader@gmail.com; xen-devel
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> On Thu, Dec 20, 2012 at 10:19 PM, Keir Fraser <keir@xen.org> wrote:
> > On 20/12/2012 13:31, "G.R." <firemeteor@users.sourceforge.net> wrote:
> >
> >> If concern is about security, the same argument should apply to the
> >> first page (the portion before the page offset).
> >> The problem is that I have no idea what is around the mapped page.
> >> Not sure who has the knowledge.
> >
> > Well we can't do better than mapping some whole number of pages,
> really.
> > Unless we trap to qemu on every access. I don't think we'd go there
> > unless there really were a known security issue. But mapping only the
> > exact number of pages we definitely need is a good principle.
> >
> >> What's the standard flow to handle such map with offset?
> >> I expect this to be a common case, since the ioremap function in
> >> linux kernel accept this.
> >
> > map_size = ((host_opregion & 0xfff) + 8096 + 0xfff) >> 12
> >
> Keir, I believe this expression should give the same result.
> First of all, 8096 should be 8192 :-), and this part should result in an
> constant number 2 after right shift.
> The remaining part is ((host_opregion & 0xfff) + 0xfff) >> 12 As long as
> the first sub-expression is non zero, the result of the add should range
> from [0x1000, 0x1ffe].
> And this will yield a result '1' after the right shift.
> So as long as there is known security risk (which I'm not sure about),
> the patch should be fine.
> 
> > Possibly with suitable macros used instead of magic numbers (e.g.,
> > XC_PAGE_* and a macro for the opregion size).
> 
> I guess there is no predefined macro for OpRegion size. And I guess I
> need to define it twice for both code?

In addition we should think about defining the IGD OpRegion in ACPI per the spec (cited earlier). Guest drivers seem to find the region just by reading the ASLS register in the gfx device's config space but it would be more correct to define it in ACPI too. Just a thought.

Ross

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

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

On Fri, Dec 21, 2012 at 2:27 AM, Ross Philipson
<Ross.Philipson@citrix.com> wrote:


>> > Possibly with suitable macros used instead of magic numbers (e.g.,
>> > XC_PAGE_* and a macro for the opregion size).
>>
>> I guess there is no predefined macro for OpRegion size. And I guess I
>> need to define it twice for both code?
>
> In addition we should think about defining the IGD OpRegion in ACPI per the spec (cited earlier). Guest drivers seem to find the region just by reading the ASLS register in the gfx device's config space but it would be more correct to define it in ACPI too. Just a thought.

Is it a requirement for the patch to be accepted? Or, are you saying
that this should not be IGD passthrough specific?
I'm not sure what you refer to by 'ACPI' here. Is it the spec itself
or header in your source code?
I'm sorry to ask but I'm just a unlucky user trying to fix my box.

The ASLS register is just the documented way to communicate the
OpRegion you can find in the spec.
There are a lot of details in the spec. But as long as we are not
going to emulate it, only the size is relevant here, I believe.

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> -----Original Message-----
> From: firemeteor.guo@gmail.com [mailto:firemeteor.guo@gmail.com] On
> Behalf Of G.R.
> Sent: Thursday, December 20, 2012 11:00 PM
> To: Ross Philipson
> Cc: Keir (Xen.org); Stefano Stabellini; Ian Campbell;
> Jean.guyader@gmail.com; xen-devel
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> On Fri, Dec 21, 2012 at 2:27 AM, Ross Philipson
> <Ross.Philipson@citrix.com> wrote:
> 
> 
> >> > Possibly with suitable macros used instead of magic numbers (e.g.,
> >> > XC_PAGE_* and a macro for the opregion size).
> >>
> >> I guess there is no predefined macro for OpRegion size. And I guess I
> >> need to define it twice for both code?
> >
> > In addition we should think about defining the IGD OpRegion in ACPI
> per the spec (cited earlier). Guest drivers seem to find the region just
> by reading the ASLS register in the gfx device's config space but it
> would be more correct to define it in ACPI too. Just a thought.
> 
> Is it a requirement for the patch to be accepted? Or, are you saying
> that this should not be IGD passthrough specific?
> I'm not sure what you refer to by 'ACPI' here. Is it the spec itself or
> header in your source code?
> I'm sorry to ask but I'm just a unlucky user trying to fix my box.
> 
> The ASLS register is just the documented way to communicate the OpRegion
> you can find in the spec.
> There are a lot of details in the spec. But as long as we are not going
> to emulate it, only the size is relevant here, I believe.

I guess all I am pointing out is that the IGD OpRegion is supposed to be defined in ACPI (that is actually why it is called an OpRegion). On all they systems I have seen it is in the DSDT (look for IDGP and IGDM). The OpRegion declaration actually tells you how big the region is and where its base is. It should probably only be there in the case where you are passing in the igfx device but this could be done by loading an auxiliary SSDT table. In addition to the IGD definition, the BIOSes on these systems also define other NVS regions related to graphics which might be useful, at least in determining their size and layout.

I have been thinking about this in relationship to our igfx passthrough support. We see some odd behaviors here and there with igfx pt and the guest drivers (which we have no control over in say the Windows case). I don't currently have hard evidence that these missing BIOS bits are causing any specific problems but they are an inconsistency wrt to the spec and on my list of suspects.

Thanks
Ross

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

>> >> I guess there is no predefined macro for OpRegion size. And I guess I
>> >> need to define it twice for both code?
>> >
>> > In addition we should think about defining the IGD OpRegion in ACPI
>> per the spec (cited earlier). Guest drivers seem to find the region just
>> by reading the ASLS register in the gfx device's config space but it
>> would be more correct to define it in ACPI too. Just a thought.
>>
>> Is it a requirement for the patch to be accepted? Or, are you saying
>> that this should not be IGD passthrough specific?
>> I'm not sure what you refer to by 'ACPI' here. Is it the spec itself or
>> header in your source code?
>> I'm sorry to ask but I'm just a unlucky user trying to fix my box.
>>
>> The ASLS register is just the documented way to communicate the OpRegion
>> you can find in the spec.
>> There are a lot of details in the spec. But as long as we are not going
>> to emulate it, only the size is relevant here, I believe.
>
> I guess all I am pointing out is that the IGD OpRegion is supposed to be defined in ACPI (that is actually why it is called an OpRegion). On all they systems I have seen it is in the DSDT (look for IDGP and IGDM). The OpRegion declaration actually tells you how big the region is and where its base is. It should probably only be there in the case where you are passing in the igfx device but this could be done by loading an auxiliary SSDT table. In addition to the IGD definition, the BIOSes on these systems also define other NVS regions related to graphics which might be useful, at least in determining their size and layout.

Great! So you are the expert about these firmwares. I also tried to
grep in these ACPI tables but did not found anything useful.
I can see that IGDM has a matching size, but the base refers to an
entry called 'ASLB', which is not directly visible from the dump.

The intel opregion spec gives me the impression that it's part of the
ACPI spec. And you are saying that is is not! What a astonishing news
to me.

> I have been thinking about this in relationship to our igfx passthrough support. We see some odd behaviors here and there with igfx pt and the guest drivers (which we have no control over in say the Windows case). I don't currently have hard evidence that these missing BIOS bits are causing any specific problems but they are an inconsistency wrt to the spec and on my list of suspects.

Yes, my win 7 guest is totally broken with IGD passthrough (much worse
than linux status).
Before I bought my current build, sources like wikis seems to mention
that IGD is the first card that works.
And now, it seems the AMD cards are the best choice for pass-through.
Sad news for me.

Thanks,
Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> -----Original Message-----
> From: firemeteor.guo@gmail.com [mailto:firemeteor.guo@gmail.com] On
> Behalf Of G.R.
> Sent: Friday, December 21, 2012 11:49 AM
> To: Ross Philipson
> Cc: Keir (Xen.org); Stefano Stabellini; Ian Campbell;
> Jean.guyader@gmail.com; xen-devel
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> >> >> I guess there is no predefined macro for OpRegion size. And I
> >> >> guess I need to define it twice for both code?
> >> >
> >> > In addition we should think about defining the IGD OpRegion in ACPI
> >> per the spec (cited earlier). Guest drivers seem to find the region
> >> just by reading the ASLS register in the gfx device's config space
> >> but it would be more correct to define it in ACPI too. Just a
> thought.
> >>
> >> Is it a requirement for the patch to be accepted? Or, are you saying
> >> that this should not be IGD passthrough specific?
> >> I'm not sure what you refer to by 'ACPI' here. Is it the spec itself
> >> or header in your source code?
> >> I'm sorry to ask but I'm just a unlucky user trying to fix my box.
> >>
> >> The ASLS register is just the documented way to communicate the
> >> OpRegion you can find in the spec.
> >> There are a lot of details in the spec. But as long as we are not
> >> going to emulate it, only the size is relevant here, I believe.
> >
> > I guess all I am pointing out is that the IGD OpRegion is supposed to
> be defined in ACPI (that is actually why it is called an OpRegion). On
> all they systems I have seen it is in the DSDT (look for IDGP and IGDM).
> The OpRegion declaration actually tells you how big the region is and
> where its base is. It should probably only be there in the case where
> you are passing in the igfx device but this could be done by loading an
> auxiliary SSDT table. In addition to the IGD definition, the BIOSes on
> these systems also define other NVS regions related to graphics which
> might be useful, at least in determining their size and layout.
> 
> Great! So you are the expert about these firmwares. I also tried to grep
> in these ACPI tables but did not found anything useful.
> I can see that IGDM has a matching size, but the base refers to an entry
> called 'ASLB', which is not directly visible from the dump.

ASLB is a field in another OpRegion that defines other areas of the gfx
related NVS regions. So the value will be present at runtime and you can
query it via the ACPI object. But the same value should be reported in
the ASLS register in the PCI config space for the igfx device which is a
lot easier to get at.

> 
> The intel opregion spec gives me the impression that it's part of the
> ACPI spec. And you are saying that is is not! What a astonishing news to
> me.

I think the spec is saying that the IGD OpRegion should be defined in ACPI
using standard ACPI entities like OpRegions and Fields.

> 
> > I have been thinking about this in relationship to our igfx
> passthrough support. We see some odd behaviors here and there with igfx
> pt and the guest drivers (which we have no control over in say the
> Windows case). I don't currently have hard evidence that these missing
> BIOS bits are causing any specific problems but they are an
> inconsistency wrt to the spec and on my list of suspects.
> 
> Yes, my win 7 guest is totally broken with IGD passthrough (much worse
> than linux status).
> Before I bought my current build, sources like wikis seems to mention
> that IGD is the first card that works.
> And now, it seems the AMD cards are the best choice for pass-through.
> Sad news for me.

Let me just clarify that up to now we have been successful in passing in
igfx cards without having to surface any of these ACPI bits. I was just
mentioning that this is an inconsistency and might be worth investigating
at some point. More importantly I am pointing out that if you are trying
to find out information like the location/size/layout of the IGD OpRegion,
you can get that information from the host BIOS. That sounded like what
your original issues centered around. Sorry if I confused things.

> 
> Thanks,
> Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> > Yes, my win 7 guest is totally broken with IGD passthrough (much worse
> > than linux status).
> > Before I bought my current build, sources like wikis seems to mention
> > that IGD is the first card that works.
> > And now, it seems the AMD cards are the best choice for pass-through.
> > Sad news for me.
> 
> Let me just clarify that up to now we have been successful in passing in
> igfx cards without having to surface any of these ACPI bits. I was just
> mentioning that this is an inconsistency and might be worth
> investigating at some point. More importantly I am pointing out that if
> you are trying to find out information like the location/size/layout of
> the IGD OpRegion, you can get that information from the host BIOS. That
> sounded like what your original issues centered around. Sorry if I
> confused things.

Oh and I forgot to add. In addition there are other OpRegions defined like
GNVS that can give you an idea of what might be just before and after the
IGD regions when it is not page aligned.

> 
> >
> > Thanks,
> > Timothy
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

On Sat, Dec 22, 2012 at 1:26 AM, Ross Philipson
<Ross.Philipson@citrix.com> wrote:
>> > Yes, my win 7 guest is totally broken with IGD passthrough (much worse
>> > than linux status).
>> > Before I bought my current build, sources like wikis seems to mention
>> > that IGD is the first card that works.
>> > And now, it seems the AMD cards are the best choice for pass-through.
>> > Sad news for me.
>>
>> Let me just clarify that up to now we have been successful in passing in
>> igfx cards without having to surface any of these ACPI bits. I was just
>> mentioning that this is an inconsistency and might be worth
>> investigating at some point.

So you are able to get a working win7 domU with IGD passthrough? That's amazing.
Currently I just have a working linux domU with IGD passthrough. (just
solve the last known functionality issue)
But the win7 domU keeps BSOD during early boot stage with IGD passed through.
The BSOD varies time to time, with or without intel gfx driver installed.
But all the BSODs are more or less related to memory corruption.
I begun to suspect this may have something to do with the bios / firmware.
(Your working system are based on intel board, right? Mine are Asrock H77m-itx)
But I don't have enough knowledge to triage the issue (all I can do so
far is to analyze core dump with KDB).

I'll start a separate thread about this and keep this thread focused.
Help you could give me some hint in that thread.

>> More importantly I am pointing out that if
>> you are trying to find out information like the location/size/layout of
>> the IGD OpRegion, you can get that information from the host BIOS. That
>> sounded like what your original issues centered around. Sorry if I
>> confused things.
>

Ross, your help is highly appreciated. I think it's not you that
confused things.
The problem comes from my side, I'm far from familiar with all these
ACPI / BIOS related stuff.
I dumped && disassembled the ACPI table. But have no idea how to read
the output...
I attached the DSDT.dsl dumped from my system, in case you would like
to take a quick check.

Just one more question -- is the layout specific to the bios, or common?
I wonder how can we judge the security risk if the layout is not constant.


> Oh and I forgot to add. In addition there are other OpRegions defined like
> GNVS that can give you an idea of what might be just before and after the
> IGD regions when it is not page aligned.

Thanks for your hint, I've seen this -- many of these.
The only issue is that I don't understand what do they mean. :-(
I think I need to dig into specs when I got spare time.

[-- Attachment #2: DSDT.dsl.gz --]
[-- Type: application/x-gzip, Size: 25895 bytes --]

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

 -----Original Message-----
> From: firemeteor.guo@gmail.com [mailto:firemeteor.guo@gmail.com] On
> Behalf Of G.R.
> Sent: Sunday, December 23, 2012 1:11 AM
> To: Ross Philipson
> Cc: xen-devel; Keir (Xen.org); Ian Campbell; Jean.guyader@gmail.com;
> Stefano Stabellini
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> On Sat, Dec 22, 2012 at 1:26 AM, Ross Philipson
> <Ross.Philipson@citrix.com> wrote:
> >> > Yes, my win 7 guest is totally broken with IGD passthrough (much
> worse
> >> > than linux status).
> >> > Before I bought my current build, sources like wikis seems to
> mention
> >> > that IGD is the first card that works.
> >> > And now, it seems the AMD cards are the best choice for pass-
> through.
> >> > Sad news for me.
> >>
> >> Let me just clarify that up to now we have been successful in passing
> in
> >> igfx cards without having to surface any of these ACPI bits. I was
> just
> >> mentioning that this is an inconsistency and might be worth
> >> investigating at some point.
> 
> So you are able to get a working win7 domU with IGD passthrough? That's
> amazing.
> Currently I just have a working linux domU with IGD passthrough. (just
> solve the last known functionality issue)
> But the win7 domU keeps BSOD during early boot stage with IGD passed
> through.
> The BSOD varies time to time, with or without intel gfx driver
> installed.
> But all the BSODs are more or less related to memory corruption.
> I begun to suspect this may have something to do with the bios /
> firmware.
> (Your working system are based on intel board, right? Mine are Asrock
> H77m-itx)
> But I don't have enough knowledge to triage the issue (all I can do so
> far is to analyze core dump with KDB).

Hmm, I found a similar issue with BSODs like this and I tacked it down to the Windows igfx drivers expecting to find vendor capabilities in the GMCH device (the root device at 00:00.0). It was actually looking there for capabilities specific to the gfx card. We fixed this by patching qemu to pass in the vendor capabilities on that device. I am not sure if that made it upstream - I will have to check.

> 
> I'll start a separate thread about this and keep this thread focused.
> Help you could give me some hint in that thread.

What is the separate thread going to be for? I did not see it.

> 
> >> More importantly I am pointing out that if
> >> you are trying to find out information like the location/size/layout
> of
> >> the IGD OpRegion, you can get that information from the host BIOS.
> That
> >> sounded like what your original issues centered around. Sorry if I
> >> confused things.
> >
> 
> Ross, your help is highly appreciated. I think it's not you that
> confused things.
> The problem comes from my side, I'm far from familiar with all these
> ACPI / BIOS related stuff.
> I dumped && disassembled the ACPI table. But have no idea how to read
> the output...
> I attached the DSDT.dsl dumped from my system, in case you would like
> to take a quick check.
> 
> Just one more question -- is the layout specific to the bios, or common?
> I wonder how can we judge the security risk if the layout is not
> constant.

I would say the layout of the IGD stuff is common per the Intel spec for it and the OEMs have to follow that spec when writing the BIOS. But it could change for newer hardware (with a new rev of the spec for example that extended the IGD functionality). 

> 
> 
> > Oh and I forgot to add. In addition there are other OpRegions defined
> like
> > GNVS that can give you an idea of what might be just before and after
> the
> > IGD regions when it is not page aligned.
> 
> Thanks for your hint, I've seen this -- many of these.
> The only issue is that I don't understand what do they mean. :-(
> I think I need to dig into specs when I got spare time.

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

>> >> Let me just clarify that up to now we have been successful in passing
>> in
>> >> igfx cards without having to surface any of these ACPI bits. I was
>> just
>> >> mentioning that this is an inconsistency and might be worth
>> >> investigating at some point.
>>
>> So you are able to get a working win7 domU with IGD passthrough? That's
>> amazing.
>> Currently I just have a working linux domU with IGD passthrough. (just
>> solve the last known functionality issue)
>> But the win7 domU keeps BSOD during early boot stage with IGD passed
>> through.
>> The BSOD varies time to time, with or without intel gfx driver
>> installed.
>> But all the BSODs are more or less related to memory corruption.
>> I begun to suspect this may have something to do with the bios /
>> firmware.
>> (Your working system are based on intel board, right? Mine are Asrock
>> H77m-itx)
>> But I don't have enough knowledge to triage the issue (all I can do so
>> far is to analyze core dump with KDB).
>
> Hmm, I found a similar issue with BSODs like this and I tacked it down to the Windows igfx drivers expecting to find vendor capabilities in the GMCH device (the root device at 00:00.0). It was actually looking there for capabilities specific to the gfx card. We fixed this by patching qemu to pass in the vendor capabilities on that device. I am not sure if that made it upstream - I will have to check.

Great news to hear. It'll be great if you can locate that patch for me.
I'm not sure which upstream you are referring to?
I'm using the qemu-xen 4.2.1 version. I remember the upstream qemu
does not support gfx-passthrough.

>
>>
>> I'll start a separate thread about this and keep this thread focused.
>> Help you could give me some hint in that thread.
>
> What is the separate thread going to be for? I did not see it.
>
Sorry, I was just back from a vacation, will pick this up soon.

>> Ross, your help is highly appreciated. I think it's not you that
>> confused things.
>> The problem comes from my side, I'm far from familiar with all these
>> ACPI / BIOS related stuff.
>> I dumped && disassembled the ACPI table. But have no idea how to read
>> the output...
>> I attached the DSDT.dsl dumped from my system, in case you would like
>> to take a quick check.
>>
>> Just one more question -- is the layout specific to the bios, or common?
>> I wonder how can we judge the security risk if the layout is not
>> constant.
>
> I would say the layout of the IGD stuff is common per the Intel spec for it and the OEMs have to follow that spec when writing the BIOS. But it could change for newer hardware (with a new rev of the spec for example that extended the IGD functionality).
>

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

>> >> More importantly I am pointing out that if
>> >> you are trying to find out information like the location/size/layout
>> of
>> >> the IGD OpRegion, you can get that information from the host BIOS.
>> That
>> >> sounded like what your original issues centered around. Sorry if I
>> >> confused things.
>> >
>>
>> Ross, your help is highly appreciated. I think it's not you that
>> confused things.
>> The problem comes from my side, I'm far from familiar with all these
>> ACPI / BIOS related stuff.
>> I dumped && disassembled the ACPI table. But have no idea how to read
>> the output...
>> I attached the DSDT.dsl dumped from my system, in case you would like
>> to take a quick check.
>>

>> Just one more question -- is the layout specific to the bios, or common?
>> I wonder how can we judge the security risk if the layout is not
>> constant.
>
> I would say the layout of the IGD stuff is common per the Intel spec for it and the OEMs have to follow that spec when writing the BIOS. But it could change for newer hardware (with a new rev of the spec for example that extended the IGD functionality).

I understand that the layout within OpRegion  is defined by the spec
and should be common.
But I don't see any words (may be I missed something) about the layout
of the outside world (e.g. what region is adjacent to OpRegion?).
So my question is actually can we expect a constant layout with regard
to OpRegion's neighbors?

My impression from Ian's feedback is that the patch cannot be accepted
before the concern being resolved.
(Let alone the existing code in the tree has already open a hole
(first 18 bytes in my example))
And you mentioned that such info can be obtained from my host bios.
I've dumped the ACPI table from my host system and had it disassembled by iasl.
But I lack of the knowledge to interpret the content.
Could you lend me a hand in case that's a trivial task for you?
You can find the DSDT.dsl file attached in my previous mail (@2012-12-23).

If you don't have time to help, could you point me to a proper spec /
documentation to start with?

Thanks,
Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> -----Original Message-----
> From: firemeteor.guo@gmail.com [mailto:firemeteor.guo@gmail.com] On
> Behalf Of G.R.
> Sent: Wednesday, January 09, 2013 10:34 AM
> To: Ross Philipson; Ian Campbell
> Cc: xen-devel; Keir (Xen.org); Jean.guyader@gmail.com; Stefano
> Stabellini
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> >> >> More importantly I am pointing out that if
> >> >> you are trying to find out information like the
> location/size/layout
> >> of
> >> >> the IGD OpRegion, you can get that information from the host BIOS.
> >> That
> >> >> sounded like what your original issues centered around. Sorry if I
> >> >> confused things.
> >> >
> >>
> >> Ross, your help is highly appreciated. I think it's not you that
> >> confused things.
> >> The problem comes from my side, I'm far from familiar with all these
> >> ACPI / BIOS related stuff.
> >> I dumped && disassembled the ACPI table. But have no idea how to read
> >> the output...
> >> I attached the DSDT.dsl dumped from my system, in case you would like
> >> to take a quick check.
> >>
> 
> >> Just one more question -- is the layout specific to the bios, or
> common?
> >> I wonder how can we judge the security risk if the layout is not
> >> constant.
> >
> > I would say the layout of the IGD stuff is common per the Intel spec
> for it and the OEMs have to follow that spec when writing the BIOS. But
> it could change for newer hardware (with a new rev of the spec for
> example that extended the IGD functionality).
> 
> 
> I understand that the layout within OpRegion  is defined by the spec
> and should be common.
> But I don't see any words (may be I missed something) about the layout
> of the outside world (e.g. what region is adjacent to OpRegion?).
> So my question is actually can we expect a constant layout with regard
> to OpRegion's neighbors?

Ok sorry, I did not understand the question fully. We may be able to sort
out what is in those NVS areas.

> 
> My impression from Ian's feedback is that the patch cannot be accepted
> before the concern being resolved.
> (Let alone the existing code in the tree has already open a hole
> (first 18 bytes in my example))
> And you mentioned that such info can be obtained from my host bios.
> I've dumped the ACPI table from my host system and had it disassembled
> by iasl.
> But I lack of the knowledge to interpret the content.
> Could you lend me a hand in case that's a trivial task for you?
> You can find the DSDT.dsl file attached in my previous mail (@2012-12-
> 23).

Sure I can take a look. Can you send me a dump of your e820 map on the
system in question (and an lspci dump while you are at it)? Also I think
you may have mentioned it but what base address is the ASLS register
reporting?

Thanks

> 
> If you don't have time to help, could you point me to a proper spec /
> documentation to start with?
> 
> Thanks,
> Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

>> >> Just one more question -- is the layout specific to the bios, or
>> common?
>> >> I wonder how can we judge the security risk if the layout is not
>> >> constant.
>> >
>> > I would say the layout of the IGD stuff is common per the Intel spec
>> for it and the OEMs have to follow that spec when writing the BIOS. But
>> it could change for newer hardware (with a new rev of the spec for
>> example that extended the IGD functionality).
>>
>>
>> I understand that the layout within OpRegion  is defined by the spec
>> and should be common.
>> But I don't see any words (may be I missed something) about the layout
>> of the outside world (e.g. what region is adjacent to OpRegion?).
>> So my question is actually can we expect a constant layout with regard
>> to OpRegion's neighbors?
>
> Ok sorry, I did not understand the question fully. We may be able to sort
> out what is in those NVS areas.
>

If we need to sort out ourselves, I guess we need more samples from
different vendors / bios versions.
Do we need to dump them from a live system, or this can be easily
extracted from the bios rom file?

>>
>> My impression from Ian's feedback is that the patch cannot be accepted
>> before the concern being resolved.
>> (Let alone the existing code in the tree has already open a hole
>> (first 18 bytes in my example))
>> And you mentioned that such info can be obtained from my host bios.
>> I've dumped the ACPI table from my host system and had it disassembled
>> by iasl.
>> But I lack of the knowledge to interpret the content.
>> Could you lend me a hand in case that's a trivial task for you?
>> You can find the DSDT.dsl file attached in my previous mail (@2012-12-
>> 23).
>
> Sure I can take a look. Can you send me a dump of your e820 map on the
> system in question (and an lspci dump while you are at it)? Also I think
> you may have mentioned it but what base address is the ASLS register
> reporting?

First of all, are you asking for the guest or the host? Host, I guess?
I'm not sure how to dump the e820 map.
Previously I used acpidump to get acpitables and use iasl to disassemble them.
Are you talking about the same work flow?

Thanks,
Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> -----Original Message-----
> From: firemeteor.guo@gmail.com [mailto:firemeteor.guo@gmail.com] On
> Behalf Of G.R.
> Sent: Thursday, January 10, 2013 5:27 AM
> To: Ross Philipson
> Cc: Ian Campbell; xen-devel; Keir (Xen.org); Jean.guyader@gmail.com;
> Stefano Stabellini
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> >> >> Just one more question -- is the layout specific to the bios, or
> >> common?
> >> >> I wonder how can we judge the security risk if the layout is not
> >> >> constant.
> >> >
> >> > I would say the layout of the IGD stuff is common per the Intel
> spec
> >> for it and the OEMs have to follow that spec when writing the BIOS.
> But
> >> it could change for newer hardware (with a new rev of the spec for
> >> example that extended the IGD functionality).
> >>
> >>
> >> I understand that the layout within OpRegion  is defined by the spec
> >> and should be common.
> >> But I don't see any words (may be I missed something) about the
> layout
> >> of the outside world (e.g. what region is adjacent to OpRegion?).
> >> So my question is actually can we expect a constant layout with
> regard
> >> to OpRegion's neighbors?
> >
> > Ok sorry, I did not understand the question fully. We may be able to
> sort
> > out what is in those NVS areas.
> >
> 
> If we need to sort out ourselves, I guess we need more samples from
> different vendors / bios versions.
> Do we need to dump them from a live system, or this can be easily
> extracted from the bios rom file?

I am not sure yet. I was hoping we could correlate those regions to
something concrete - e.g. in a spec. 

> 
> >>
> >> My impression from Ian's feedback is that the patch cannot be
> accepted
> >> before the concern being resolved.
> >> (Let alone the existing code in the tree has already open a hole
> >> (first 18 bytes in my example))
> >> And you mentioned that such info can be obtained from my host bios.
> >> I've dumped the ACPI table from my host system and had it
> disassembled
> >> by iasl.
> >> But I lack of the knowledge to interpret the content.
> >> Could you lend me a hand in case that's a trivial task for you?
> >> You can find the DSDT.dsl file attached in my previous mail (@2012-
> 12-
> >> 23).
> >
> > Sure I can take a look. Can you send me a dump of your e820 map on the
> > system in question (and an lspci dump while you are at it)? Also I
> think
> > you may have mentioned it but what base address is the ASLS register
> > reporting?
> 
> First of all, are you asking for the guest or the host? Host, I guess?
> I'm not sure how to dump the e820 map.
> Previously I used acpidump to get acpitables and use iasl to disassemble
> them.
> Are you talking about the same work flow?

Host. The easiest way to get it is if you can get serial output from Xen
which should report it early on. Or your kernel may report it; mine does
and I can get it via dmesg (traced as "Xen-provided memory map").

For PCI I just wanted the output from lspci (e.g. lspci -v and lspci -xxx).



> 
> Thanks,
> Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]

>> >> My impression from Ian's feedback is that the patch cannot be
>> accepted
>> >> before the concern being resolved.
>> >> (Let alone the existing code in the tree has already open a hole
>> >> (first 18 bytes in my example))
>> >> And you mentioned that such info can be obtained from my host bios.
>> >> I've dumped the ACPI table from my host system and had it
>> disassembled
>> >> by iasl.
>> >> But I lack of the knowledge to interpret the content.
>> >> Could you lend me a hand in case that's a trivial task for you?
>> >> You can find the DSDT.dsl file attached in my previous mail (@2012-
>> 12-
>> >> 23).
>> >
>> > Sure I can take a look. Can you send me a dump of your e820 map on the
>> > system in question (and an lspci dump while you are at it)? Also I
>> think
>> > you may have mentioned it but what base address is the ASLS register
>> > reporting?
>>
>> First of all, are you asking for the guest or the host? Host, I guess?
>> I'm not sure how to dump the e820 map.
>> Previously I used acpidump to get acpitables and use iasl to disassemble
>> them.
>> Are you talking about the same work flow?
>
> Host. The easiest way to get it is if you can get serial output from Xen
> which should report it early on. Or your kernel may report it; mine does
> and I can get it via dmesg (traced as "Xen-provided memory map").
>
> For PCI I just wanted the output from lspci (e.g. lspci -v and lspci -xxx).
>

This shoud be the OpRegion base: cd996018 is the host address.
igd_write_opregion: Map OpRegion: cd996018 -> feff4018

Other info are available in the attachment, including pci info && the
xl dmesg log (for e820 map).

Thanks,
Timothy

[-- Attachment #2: opregion.dbg.tar.gz --]
[-- Type: application/x-gzip, Size: 8239 bytes --]

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ross Philipson]

> -----Original Message-----
> From: firemeteor.guo@gmail.com [mailto:firemeteor.guo@gmail.com] On
> Behalf Of G.R.
> Sent: Thursday, January 10, 2013 11:29 AM
> To: Ross Philipson
> Cc: Ian Campbell; xen-devel; Keir (Xen.org); Jean.guyader@gmail.com;
> Stefano Stabellini
> Subject: Re: [Xen-devel] [PATCH] hvmloader / qemu-xen: Getting rid of
> resource conflict for OpRegion.
> 
> >> >> My impression from Ian's feedback is that the patch cannot be
> >> accepted
> >> >> before the concern being resolved.
> >> >> (Let alone the existing code in the tree has already open a hole
> >> >> (first 18 bytes in my example))
> >> >> And you mentioned that such info can be obtained from my host
> bios.
> >> >> I've dumped the ACPI table from my host system and had it
> >> disassembled
> >> >> by iasl.
> >> >> But I lack of the knowledge to interpret the content.
> >> >> Could you lend me a hand in case that's a trivial task for you?
> >> >> You can find the DSDT.dsl file attached in my previous mail
> (@2012-
> >> 12-
> >> >> 23).
> >> >
> >> > Sure I can take a look. Can you send me a dump of your e820 map on
> the
> >> > system in question (and an lspci dump while you are at it)? Also I
> >> think
> >> > you may have mentioned it but what base address is the ASLS
> register
> >> > reporting?
> >>
> >> First of all, are you asking for the guest or the host? Host, I
> guess?
> >> I'm not sure how to dump the e820 map.
> >> Previously I used acpidump to get acpitables and use iasl to
> disassemble
> >> them.
> >> Are you talking about the same work flow?
> >
> > Host. The easiest way to get it is if you can get serial output from
> Xen
> > which should report it early on. Or your kernel may report it; mine
> does
> > and I can get it via dmesg (traced as "Xen-provided memory map").
> >
> > For PCI I just wanted the output from lspci (e.g. lspci -v and lspci -
> xxx).
> >
> 
> This shoud be the OpRegion base: cd996018 is the host address.
> igd_write_opregion: Map OpRegion: cd996018 -> feff4018
> 
> Other info are available in the attachment, including pci info && the
> xl dmesg log (for e820 map).

Well unfortunately not much else in that ACPI NVS region is defined. There
is the GNVS region that is defined but it is tiny and at the tail end of
it. So it is not clear what else is in that region or around the IGD
region (frankly it could be unused or contain other bits not related the
graphics at all). Some of the values in GNVS could be pointers to other
regions in that NVS too.

I guess the only truly 100% secure way to deal with it is to trap to qemu
(as was suggested) and block access to the area around IGD. It didn't
sound like this was a popular option. Anyway sorry I could not be
more helpful but it looks like your patch to map the extra page made
it in unstable.

> 
> Thanks,
> Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

>>
>> This shoud be the OpRegion base: cd996018 is the host address.
>> igd_write_opregion: Map OpRegion: cd996018 -> feff4018
>>
>> Other info are available in the attachment, including pci info && the
>> xl dmesg log (for e820 map).
>
> Well unfortunately not much else in that ACPI NVS region is defined. There
> is the GNVS region that is defined but it is tiny and at the tail end of
> it. So it is not clear what else is in that region or around the IGD
> region (frankly it could be unused or contain other bits not related the
> graphics at all). Some of the values in GNVS could be pointers to other
> regions in that NVS too.
>
> I guess the only truly 100% secure way to deal with it is to trap to qemu
> (as was suggested) and block access to the area around IGD. It didn't
> sound like this was a popular option. Anyway sorry I could not be
> more helpful but it looks like your patch to map the extra page made
> it in unstable.
>

Anyway, thanks for your help, Ross.

Hi Ian,
Unfortunately, our effort does not lead to clear result.
We still have no idea what is around the OpRegion, or even whether it
is supposed to be
a fixed layout. I not sure how to judge the security risk in this case.
So if you believe this is not acceptable, I'll stop wasting time on this...

Thanks,
Timothy

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Jean Guyader]

On Thu, Dec 20, 2012 at 7:06 AM, G.R. <firemeteor@users.sourceforge.net> wrote:
> On Thu, Dec 20, 2012 at 10:19 PM, Keir Fraser <keir@xen.org> wrote:
>> On 20/12/2012 13:31, "G.R." <firemeteor@users.sourceforge.net> wrote:
>>
>>> If concern is about security, the same argument should apply to the
>>> first page (the portion before the page offset).
>>> The problem is that I have no idea what is around the mapped page. Not
>>> sure who has the knowledge.
>>
>> Well we can't do better than mapping some whole number of pages, really.
>> Unless we trap to qemu on every access. I don't think we'd go there unless
>> there really were a known security issue. But mapping only the exact number
>> of pages we definitely need is a good principle.
>>
>>> What's the standard flow to handle such map with offset?
>>> I expect this to be a common case, since the ioremap function in linux
>>> kernel accept this.
>>
>> map_size = ((host_opregion & 0xfff) + 8096 + 0xfff) >> 12
>>
> Keir, I believe this expression should give the same result.
> First of all, 8096 should be 8192 :-), and this part should result in
> an constant number 2 after right shift.
> The remaining part is ((host_opregion & 0xfff) + 0xfff) >> 12
> As long as the first sub-expression is non zero, the result of the add
> should range from [0x1000, 0x1ffe].
> And this will yield a result '1' after the right shift.
> So as long as there is known security risk (which I'm not sure about),
> the patch should be fine.
>
>> Possibly with suitable macros used instead of magic numbers (e.g., XC_PAGE_*
>> and a macro for the opregion size).
>
> I guess there is no predefined macro for OpRegion size. And I guess I
> need to define it twice for both code?
>

Changing the OpRegion mapping to 3 pages if probably the right thing to do here.
If down the road we found some scary security problem we can emulate the device
in Qemu.

Thanks for looking into this.

Jean

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[Ian Campbell]

On Thu, 2012-12-20 at 13:03 +0000, Keir Fraser wrote:
> On 20/12/2012 10:41, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
> > Do we need to worry about what is in the "slop" at either end of a 3
> > page region containing this? If they are sensitive registers then we may
> > have a problem.
> 
> In the hvmloader patch it is not worth it I think, one extra page of memory
> hole is hardly a scarce resource.

I didn't mean the wastage, I meant the contents (registers) at the
physical addresses either immediately before or after the OpRegion.

Ian.

Re: [PATCH] hvmloader / qemu-xen: Getting rid of resource conflict for OpRegion.

[G.R.]

On Fri, Dec 21, 2012 at 3:50 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2012-12-20 at 13:03 +0000, Keir Fraser wrote:
>> On 20/12/2012 10:41, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
>> > Do we need to worry about what is in the "slop" at either end of a 3
>> > page region containing this? If they are sensitive registers then we may
>> > have a problem.
>>
>> In the hvmloader patch it is not worth it I think, one extra page of memory
>> hole is hardly a scarce resource.
>
> I didn't mean the wastage, I meant the contents (registers) at the
> physical addresses either immediately before or after the OpRegion.
>
The bad news is that we have no idea.
In section 2.3, the spec mentions that the firmware is responsible to
create the OpRegion upon boot.
So the layout may be firmware specific.

But I think we need to get confirmation from ACPI / firmware
experts.Who will that be?

Please remember that even the bundle potentially worsen the case, the
potential hole is already open
in the current code-base.
I think you should take action to evaluate the risk whether this patch
is accepted or not.

Thanks,
Timothy