hvmloader: fix memory type handling

xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed

* hvmloader: fix memory type handling
@ 2014-05-19 13:02 Jan Beulich
  2014-05-19 13:12 ` [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges Jan Beulich
  2014-05-19 13:13 ` [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC Jan Beulich
  0 siblings, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2014-05-19 13:02 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Campbell, Ian Jackson, Stefano Stabellini

While in the end Tomasz'es problem description in the thread starting at
http://lists.xenproject.org/archives/html/xen-devel/2014-05/msg01902.html
turned out to not be BAR assignment related, looking into it nevertheless
revealed a couple of issues with hvmloader's handling of memory types
here.

1: also cover PCI MMIO ranges above 4G with UC MTRR ranges
2: PA range 0xfc000000-0xffffffff should be UC

Signed-off-by: Jan Beulich <jbeulich@suse.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges
  2014-05-19 13:02 hvmloader: fix memory type handling Jan Beulich
@ 2014-05-19 13:12 ` Jan Beulich
  2014-05-21 12:24   ` Ian Campbell
  2014-05-19 13:13 ` [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC Jan Beulich
  1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2014-05-19 13:12 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Campbell, Ian Jackson, Stefano Stabellini

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

When adding support for BAR assignments to addresses above 4G, the MTRR
side of things was left out.

Additionally the MMIO ranges in the DSDT's \_SB.PCI0._CRS were having
memory types not matching the ones put into MTRRs: The legacy VGA range
is supposed to be WC, and the other ones should be UC.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -53,6 +53,7 @@ struct acpi_info {
     uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
     uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
     uint32_t vm_gid_addr;       /* 20   - Address of VM generation id buffer */
+    uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
 };
 
 /* Number of processor objects in the chosen DSDT. */
@@ -541,6 +542,11 @@ void acpi_build_tables(struct acpi_confi
     acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
     acpi_info->pci_min = pci_mem_start;
     acpi_info->pci_len = pci_mem_end - pci_mem_start;
+    if ( pci_hi_mem_end > pci_hi_mem_start )
+    {
+        acpi_info->pci_hi_min = pci_hi_mem_start;
+        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
+    }
 
     return;
 
--- a/tools/firmware/hvmloader/acpi/dsdt.asl
+++ b/tools/firmware/hvmloader/acpi/dsdt.asl
@@ -45,7 +45,7 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
     Scope (\_SB)
     {
        /* ACPI_INFO_PHYSICAL_ADDRESS == 0xFC000000 */
-       OperationRegion(BIOS, SystemMemory, 0xFC000000, 24)
+       OperationRegion(BIOS, SystemMemory, 0xFC000000, 40)
        Field(BIOS, ByteAcc, NoLock, Preserve) {
            UAR1, 1,
            UAR2, 1,
@@ -56,7 +56,9 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
            PLEN, 32,
            MSUA, 32, /* MADT checksum address */
            MAPA, 32, /* MADT LAPIC0 address */
-           VGIA, 32  /* VM generation id address */
+           VGIA, 32, /* VM generation id address */
+           HMIN, 64,
+           HLEN, 64
        }
 
         /* Fix HCT test for 0x400 pci memory:
@@ -136,7 +138,7 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
                     /* reserve memory for pci devices */
                     DWordMemory(
                         ResourceProducer, PosDecode, MinFixed, MaxFixed,
-                        Cacheable, ReadWrite,
+                        WriteCombining, ReadWrite,
                         0x00000000,
                         0x000A0000,
                         0x000BFFFF,
@@ -145,13 +147,24 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
 
                     DWordMemory(
                         ResourceProducer, PosDecode, MinFixed, MaxFixed,
-                        Cacheable, ReadWrite,
+                        NonCacheable, ReadWrite,
                         0x00000000,
                         0xF0000000,
                         0xF4FFFFFF,
                         0x00000000,
                         0x05000000,
                         ,, _Y01)
+
+                    QWordMemory (
+                        ResourceProducer, PosDecode, MinFixed, MaxFixed,
+                        NonCacheable, ReadWrite,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        ,, _Y02)
+
                 })
 
                 CreateDWordField(PRT0, \_SB.PCI0._CRS._Y01._MIN, MMIN)
@@ -163,6 +176,17 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
                 Add(MMIN, MLEN, MMAX)
                 Subtract(MMAX, One, MMAX)
 
+                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MIN, HMIN)
+                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MAX, HMAX)
+                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._LEN, HLEN)
+
+                Store(\_SB.HMIN, HMIN)
+                Store(\_SB.HLEN, HLEN)
+                Add(HMIN, HLEN, HMAX)
+                If(LOr(HMIN, HLEN)) {
+                    Subtract(HMAX, One, HMAX)
+                }
+
                 Return (PRT0)
             }
 
--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -97,8 +97,7 @@ void cacheattr_init(void)
     nr_var_ranges = (uint8_t)mtrr_cap;
     if ( nr_var_ranges != 0 )
     {
-        unsigned long base = pci_mem_start, size;
-        int i;
+        uint64_t base = pci_mem_start, size;
 
         for ( i = 0; (base != pci_mem_end) && (i < nr_var_ranges); i++ )
         {
@@ -109,8 +108,22 @@ void cacheattr_init(void)
                 size >>= 1;
 
             wrmsr(MSR_MTRRphysBase(i), base);
-            wrmsr(MSR_MTRRphysMask(i),
-                  (~(uint64_t)(size-1) & addr_mask) | (1u << 11));
+            wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
+
+            base += size;
+        }
+
+        for ( base = pci_hi_mem_start;
+              (base != pci_hi_mem_end) && (i < nr_var_ranges); i++ )
+        {
+            size = PAGE_SIZE;
+            while ( !(base & size) )
+                size <<= 1;
+            while ( (base + size < base) || (base + size > pci_hi_mem_end) )
+                size >>= 1;
+
+            wrmsr(MSR_MTRRphysBase(i), base);
+            wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
 
             base += size;
         }
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -57,7 +57,7 @@ extern struct bios_config ovmf_config;
 #define PCI_MEM_END         0xfc000000
 
 extern unsigned long pci_mem_start, pci_mem_end;
-
+extern uint64_t pci_hi_mem_start, pci_hi_mem_end;
 
 /* Memory map. */
 #define SCRATCH_PHYSICAL_ADDRESS      0x00010000
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -32,6 +32,7 @@
 
 unsigned long pci_mem_start = PCI_MEM_START;
 unsigned long pci_mem_end = PCI_MEM_END;
+uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
 
 enum virtual_vga virtual_vga = VGA_none;
 unsigned long igd_opregion_pgbase = 0;
@@ -345,9 +346,8 @@ void pci_setup(void)
                 if ( high_mem_resource.base & (bar_sz - 1) )
                     high_mem_resource.base = high_mem_resource.base - 
                         (high_mem_resource.base & (bar_sz - 1)) + bar_sz;
-                else
-                    high_mem_resource.base = high_mem_resource.base - 
-                        (high_mem_resource.base & (bar_sz - 1));
+                if ( !pci_hi_mem_start )
+                    pci_hi_mem_start = high_mem_resource.base;
                 resource = &high_mem_resource;
                 bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
             } 
@@ -398,6 +398,16 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }
 
+    if ( pci_hi_mem_start )
+    {
+        /*
+         * Make end address alignment match the start address one's so that
+         * fewer variable range MTRRs are needed to cover the range.
+         */
+        pci_hi_mem_end = ((high_mem_resource.base - 1) |
+                          ((pci_hi_mem_start & -pci_hi_mem_start) - 1)) + 1;
+    }
+
     if ( vga_devfn != 256 )
     {
         /*



[-- Attachment #2: hvmloader-PCI-above-4G.patch --]
[-- Type: text/plain, Size: 7497 bytes --]

hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges

When adding support for BAR assignments to addresses above 4G, the MTRR
side of things was left out.

Additionally the MMIO ranges in the DSDT's \_SB.PCI0._CRS were having
memory types not matching the ones put into MTRRs: The legacy VGA range
is supposed to be WC, and the other ones should be UC.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -53,6 +53,7 @@ struct acpi_info {
     uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
     uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
     uint32_t vm_gid_addr;       /* 20   - Address of VM generation id buffer */
+    uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
 };
 
 /* Number of processor objects in the chosen DSDT. */
@@ -541,6 +542,11 @@ void acpi_build_tables(struct acpi_confi
     acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
     acpi_info->pci_min = pci_mem_start;
     acpi_info->pci_len = pci_mem_end - pci_mem_start;
+    if ( pci_hi_mem_end > pci_hi_mem_start )
+    {
+        acpi_info->pci_hi_min = pci_hi_mem_start;
+        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
+    }
 
     return;
 
--- a/tools/firmware/hvmloader/acpi/dsdt.asl
+++ b/tools/firmware/hvmloader/acpi/dsdt.asl
@@ -45,7 +45,7 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
     Scope (\_SB)
     {
        /* ACPI_INFO_PHYSICAL_ADDRESS == 0xFC000000 */
-       OperationRegion(BIOS, SystemMemory, 0xFC000000, 24)
+       OperationRegion(BIOS, SystemMemory, 0xFC000000, 40)
        Field(BIOS, ByteAcc, NoLock, Preserve) {
            UAR1, 1,
            UAR2, 1,
@@ -56,7 +56,9 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
            PLEN, 32,
            MSUA, 32, /* MADT checksum address */
            MAPA, 32, /* MADT LAPIC0 address */
-           VGIA, 32  /* VM generation id address */
+           VGIA, 32, /* VM generation id address */
+           HMIN, 64,
+           HLEN, 64
        }
 
         /* Fix HCT test for 0x400 pci memory:
@@ -136,7 +138,7 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
                     /* reserve memory for pci devices */
                     DWordMemory(
                         ResourceProducer, PosDecode, MinFixed, MaxFixed,
-                        Cacheable, ReadWrite,
+                        WriteCombining, ReadWrite,
                         0x00000000,
                         0x000A0000,
                         0x000BFFFF,
@@ -145,13 +147,24 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
 
                     DWordMemory(
                         ResourceProducer, PosDecode, MinFixed, MaxFixed,
-                        Cacheable, ReadWrite,
+                        NonCacheable, ReadWrite,
                         0x00000000,
                         0xF0000000,
                         0xF4FFFFFF,
                         0x00000000,
                         0x05000000,
                         ,, _Y01)
+
+                    QWordMemory (
+                        ResourceProducer, PosDecode, MinFixed, MaxFixed,
+                        NonCacheable, ReadWrite,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        0x0000000000000000,
+                        ,, _Y02)
+
                 })
 
                 CreateDWordField(PRT0, \_SB.PCI0._CRS._Y01._MIN, MMIN)
@@ -163,6 +176,17 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
                 Add(MMIN, MLEN, MMAX)
                 Subtract(MMAX, One, MMAX)
 
+                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MIN, HMIN)
+                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MAX, HMAX)
+                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._LEN, HLEN)
+
+                Store(\_SB.HMIN, HMIN)
+                Store(\_SB.HLEN, HLEN)
+                Add(HMIN, HLEN, HMAX)
+                If(LOr(HMIN, HLEN)) {
+                    Subtract(HMAX, One, HMAX)
+                }
+
                 Return (PRT0)
             }
 
--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -97,8 +97,7 @@ void cacheattr_init(void)
     nr_var_ranges = (uint8_t)mtrr_cap;
     if ( nr_var_ranges != 0 )
     {
-        unsigned long base = pci_mem_start, size;
-        int i;
+        uint64_t base = pci_mem_start, size;
 
         for ( i = 0; (base != pci_mem_end) && (i < nr_var_ranges); i++ )
         {
@@ -109,8 +108,22 @@ void cacheattr_init(void)
                 size >>= 1;
 
             wrmsr(MSR_MTRRphysBase(i), base);
-            wrmsr(MSR_MTRRphysMask(i),
-                  (~(uint64_t)(size-1) & addr_mask) | (1u << 11));
+            wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
+
+            base += size;
+        }
+
+        for ( base = pci_hi_mem_start;
+              (base != pci_hi_mem_end) && (i < nr_var_ranges); i++ )
+        {
+            size = PAGE_SIZE;
+            while ( !(base & size) )
+                size <<= 1;
+            while ( (base + size < base) || (base + size > pci_hi_mem_end) )
+                size >>= 1;
+
+            wrmsr(MSR_MTRRphysBase(i), base);
+            wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
 
             base += size;
         }
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -57,7 +57,7 @@ extern struct bios_config ovmf_config;
 #define PCI_MEM_END         0xfc000000
 
 extern unsigned long pci_mem_start, pci_mem_end;
-
+extern uint64_t pci_hi_mem_start, pci_hi_mem_end;
 
 /* Memory map. */
 #define SCRATCH_PHYSICAL_ADDRESS      0x00010000
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -32,6 +32,7 @@
 
 unsigned long pci_mem_start = PCI_MEM_START;
 unsigned long pci_mem_end = PCI_MEM_END;
+uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
 
 enum virtual_vga virtual_vga = VGA_none;
 unsigned long igd_opregion_pgbase = 0;
@@ -345,9 +346,8 @@ void pci_setup(void)
                 if ( high_mem_resource.base & (bar_sz - 1) )
                     high_mem_resource.base = high_mem_resource.base - 
                         (high_mem_resource.base & (bar_sz - 1)) + bar_sz;
-                else
-                    high_mem_resource.base = high_mem_resource.base - 
-                        (high_mem_resource.base & (bar_sz - 1));
+                if ( !pci_hi_mem_start )
+                    pci_hi_mem_start = high_mem_resource.base;
                 resource = &high_mem_resource;
                 bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
             } 
@@ -398,6 +398,16 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }
 
+    if ( pci_hi_mem_start )
+    {
+        /*
+         * Make end address alignment match the start address one's so that
+         * fewer variable range MTRRs are needed to cover the range.
+         */
+        pci_hi_mem_end = ((high_mem_resource.base - 1) |
+                          ((pci_hi_mem_start & -pci_hi_mem_start) - 1)) + 1;
+    }
+
     if ( vga_devfn != 256 )
     {
         /*

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges
  2014-05-19 13:12 ` [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges Jan Beulich
@ 2014-05-21 12:24   ` Ian Campbell
  2014-05-21 14:05     ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2014-05-21 12:24 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Ian Jackson, Stefano Stabellini

On Mon, 2014-05-19 at 14:12 +0100, Jan Beulich wrote:
> @@ -163,6 +176,17 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
>                  Add(MMIN, MLEN, MMAX)
>                  Subtract(MMAX, One, MMAX)
>  
> +                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MIN, HMIN)
> +                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MAX, HMAX)
> +                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._LEN, HLEN)
> +
> +                Store(\_SB.HMIN, HMIN)
> +                Store(\_SB.HLEN, HLEN)
> +                Add(HMIN, HLEN, HMAX)
> +                If(LOr(HMIN, HLEN)) {
> +                    Subtract(HMAX, One, HMAX)
> +                }

This is (unlike the low version) conditional because HLEN might be 0? Is
the fact that _Y02 might continue to contain all zeroes OK?

I expect so, in which case

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(I expect you will commit this yourself when you are ready)

Ian.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges
  2014-05-21 12:24   ` Ian Campbell
@ 2014-05-21 14:05     ` Jan Beulich
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2014-05-21 14:05 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel, Ian Jackson, Stefano Stabellini

>>> On 21.05.14 at 14:24, <Ian.Campbell@eu.citrix.com> wrote:
> On Mon, 2014-05-19 at 14:12 +0100, Jan Beulich wrote:
>> @@ -163,6 +176,17 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, 
>>                  Add(MMIN, MLEN, MMAX)
>>                  Subtract(MMAX, One, MMAX)
>>  
>> +                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MIN, HMIN)
>> +                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._MAX, HMAX)
>> +                CreateQWordField(PRT0, \_SB.PCI0._CRS._Y02._LEN, HLEN)
>> +
>> +                Store(\_SB.HMIN, HMIN)
>> +                Store(\_SB.HLEN, HLEN)
>> +                Add(HMIN, HLEN, HMAX)
>> +                If(LOr(HMIN, HLEN)) {
>> +                    Subtract(HMAX, One, HMAX)
>> +                }
> 
> This is (unlike the low version) conditional because HLEN might be 0?

Yes.

> Is the fact that _Y02 might continue to contain all zeroes OK?

Yes, it is - for Linux at least things end up being slightly wrong
(in that the region then gets treated as disabled rather than absent)
when doing the subtraction of 1 unconditionally.

Additionally I have a physical system where such an empty range (all
zeros) exists, which I take as additional proof that this is okay.

Jan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC
  2014-05-19 13:02 hvmloader: fix memory type handling Jan Beulich
  2014-05-19 13:12 ` [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges Jan Beulich
@ 2014-05-19 13:13 ` Jan Beulich
  2014-05-21 12:26   ` Ian Campbell
  1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2014-05-19 13:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Campbell, Ian Jackson, Stefano Stabellini

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

Rather than leaving the range from PCI_MEM_END (0xfc000000) to 4G
uncovered, we should include this in the UC range created for the (low)
PCI range. Besides being more correct, this also has the advantage that
with the way pci_setup() currently works the range will always be
mappable with a single variable range MTRR (rather than from 2 to 5
depending on how much the lower boundary gets shifted down to
accommodate all devices).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -99,12 +99,12 @@ void cacheattr_init(void)
     {
         uint64_t base = pci_mem_start, size;
 
-        for ( i = 0; (base != pci_mem_end) && (i < nr_var_ranges); i++ )
+        for ( i = 0; !(base >> 32) && (i < nr_var_ranges); i++ )
         {
             size = PAGE_SIZE;
             while ( !(base & size) )
                 size <<= 1;
-            while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
+            while ( ((base + size) < base) || ((base + size - 1) >> 32) )
                 size >>= 1;
 
             wrmsr(MSR_MTRRphysBase(i), base);




[-- Attachment #2: hvmloader-UC-up-to-4G.patch --]
[-- Type: text/plain, Size: 1234 bytes --]

hvmloader: PA range 0xfc000000-0xffffffff should be UC

Rather than leaving the range from PCI_MEM_END (0xfc000000) to 4G
uncovered, we should include this in the UC range created for the (low)
PCI range. Besides being more correct, this also has the advantage that
with the way pci_setup() currently works the range will always be
mappable with a single variable range MTRR (rather than from 2 to 5
depending on how much the lower boundary gets shifted down to
accommodate all devices).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -99,12 +99,12 @@ void cacheattr_init(void)
     {
         uint64_t base = pci_mem_start, size;
 
-        for ( i = 0; (base != pci_mem_end) && (i < nr_var_ranges); i++ )
+        for ( i = 0; !(base >> 32) && (i < nr_var_ranges); i++ )
         {
             size = PAGE_SIZE;
             while ( !(base & size) )
                 size <<= 1;
-            while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
+            while ( ((base + size) < base) || ((base + size - 1) >> 32) )
                 size >>= 1;
 
             wrmsr(MSR_MTRRphysBase(i), base);

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC
  2014-05-19 13:13 ` [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC Jan Beulich
@ 2014-05-21 12:26   ` Ian Campbell
  2014-05-21 14:02     ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2014-05-21 12:26 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Ian Jackson, Stefano Stabellini

On Mon, 2014-05-19 at 14:13 +0100, Jan Beulich wrote:
> Rather than leaving the range from PCI_MEM_END (0xfc000000) to 4G
> uncovered, we should include this in the UC range created for the (low)
> PCI range. Besides being more correct,

OOI why is this more correct? What is in here, APIC registers and stuff
I think? (that may have answered my question...)

>  this also has the advantage that
> with the way pci_setup() currently works the range will always be
> mappable with a single variable range MTRR (rather than from 2 to 5
> depending on how much the lower boundary gets shifted down to
> accommodate all devices).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Regardless of the above:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> --- a/tools/firmware/hvmloader/cacheattr.c
> +++ b/tools/firmware/hvmloader/cacheattr.c
> @@ -99,12 +99,12 @@ void cacheattr_init(void)
>      {
>          uint64_t base = pci_mem_start, size;
>  
> -        for ( i = 0; (base != pci_mem_end) && (i < nr_var_ranges); i++ )
> +        for ( i = 0; !(base >> 32) && (i < nr_var_ranges); i++ )
>          {
>              size = PAGE_SIZE;
>              while ( !(base & size) )
>                  size <<= 1;
> -            while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
> +            while ( ((base + size) < base) || ((base + size - 1) >> 32) )
>                  size >>= 1;
>  
>              wrmsr(MSR_MTRRphysBase(i), base);
> 
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC
  2014-05-21 12:26   ` Ian Campbell
@ 2014-05-21 14:02     ` Jan Beulich
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2014-05-21 14:02 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel, Ian Jackson, Stefano Stabellini

>>> On 21.05.14 at 14:26, <Ian.Campbell@eu.citrix.com> wrote:
> On Mon, 2014-05-19 at 14:13 +0100, Jan Beulich wrote:
>> Rather than leaving the range from PCI_MEM_END (0xfc000000) to 4G
>> uncovered, we should include this in the UC range created for the (low)
>> PCI range. Besides being more correct,
> 
> OOI why is this more correct? What is in here, APIC registers and stuff
> I think? (that may have answered my question...)

Right - LAPIC, IO-APIC, HPET, ...

Jan

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-05-21 14:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-19 13:02 hvmloader: fix memory type handling Jan Beulich
2014-05-19 13:12 ` [PATCH 1/2] hvmloader: also cover PCI MMIO ranges above 4G with UC MTRR ranges Jan Beulich
2014-05-21 12:24   ` Ian Campbell
2014-05-21 14:05     ` Jan Beulich
2014-05-19 13:13 ` [PATCH 2/2] hvmloader: PA range 0xfc000000-0xffffffff should be UC Jan Beulich
2014-05-21 12:26   ` Ian Campbell
2014-05-21 14:02     ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).