[PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[Malcolm Crossley]

Upstream commit 1c84d046735102e02d2df454ab07f14ac51f235d (XSA-60 fix)
Stopped tracking of setting/unsetting of uncached mode in an HVM guest.
(This was later fixed by the EPT misconfig changes in Xen 4.5.)

On hosts which do not have forced IOMMU snooping then Xen allows for uncached
P2M mappings to be created otherwise cache issues would occur.

When hvmloader relocates RAM out of the HVM guests MMIO hole, Xen use's the
default MTRR setting to determine the cache setting for the destination RAM.

hvmloader has not configured the default MTRR to enable write back caching at
the point the RAM is relocated and so the destination RAM ends up being
configured as uncached.

Before the XSA-60 patches, the RAM would have it's cachability setting changed
to write back when the default MTRR was configured to write back.

After XSA-60 patches, this does not happen and the RAM remains as uncached.

This patch sets the default MTRR to be write back before relocating the RAM
and restores the default MTRR after relocating the RAM.

The patch was designed to be be minimally invasive so it could be backported
easily.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>

diff -r dfb21287aac1 -r 4c9ba9c07a76 tools/firmware/hvmloader/pci.c
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -27,6 +27,7 @@
 
 #include <xen/memory.h>
 #include <xen/hvm/ioreq.h>
+#define MSR_MTRRdefType      0x02ff
 
 unsigned long pci_mem_start = PCI_MEM_START;
 unsigned long pci_mem_end = PCI_MEM_END;
@@ -197,25 +198,34 @@ void pci_setup(void)
             ((pci_mem_start << 1) != 0) )
         pci_mem_start <<= 1;
 
-    /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
-    while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
+    /* Workaround XENMEM_add_to_physmap creating uncached mappings
+     * by setting default MTRR type to write back mapping */
+    if ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
     {
-        struct xen_add_to_physmap xatp;
-        unsigned int nr_pages = min_t(
-            unsigned int,
-            hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
-            (1u << 16) - 1);
-        if ( hvm_info->high_mem_pgend == 0 )
-            hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
-        hvm_info->low_mem_pgend -= nr_pages;
-        xatp.domid = DOMID_SELF;
-        xatp.space = XENMAPSPACE_gmfn_range;
-        xatp.idx   = hvm_info->low_mem_pgend;
-        xatp.gpfn  = hvm_info->high_mem_pgend;
-        xatp.size  = nr_pages;
-        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
-            BUG();
-        hvm_info->high_mem_pgend += nr_pages;
+        uint64_t mtrr_def_type = rdmsr(MSR_MTRRdefType);
+        wrmsr(MSR_MTRRdefType, (1u << 11) | 6);
+        /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
+        while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
+        {
+            struct xen_add_to_physmap xatp;
+            unsigned int nr_pages = min_t(
+                unsigned int,
+                hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
+                (1u << 16) - 1);
+            if ( hvm_info->high_mem_pgend == 0 )
+                hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
+            hvm_info->low_mem_pgend -= nr_pages;
+            xatp.domid = DOMID_SELF;
+            xatp.space = XENMAPSPACE_gmfn_range;
+            xatp.idx   = hvm_info->low_mem_pgend;
+            xatp.gpfn  = hvm_info->high_mem_pgend;
+            xatp.size  = nr_pages;
+            if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
+                BUG();
+            hvm_info->high_mem_pgend += nr_pages;
+        }
+        /* Restore previous default MTRR value */
+        wrmsr(MSR_MTRRdefType, mtrr_def_type);
     }
 
     mem_resource.base = pci_mem_start;

Re: [PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[Malcolm Crossley]

On 19/02/15 13:01, Malcolm Crossley wrote:
> Upstream commit 1c84d046735102e02d2df454ab07f14ac51f235d (XSA-60 fix)
> Stopped tracking of setting/unsetting of uncached mode in an HVM guest.
> (This was later fixed by the EPT misconfig changes in Xen 4.5.)
> 
This issue affects any Xen with XSA-60 fixes applied, prior to Xen 4.5.

Re: [PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[Jan Beulich]

>>> On 19.02.15 at 14:01, <malcolm.crossley@citrix.com> wrote:
> Upstream commit 1c84d046735102e02d2df454ab07f14ac51f235d (XSA-60 fix)
> Stopped tracking of setting/unsetting of uncached mode in an HVM guest.
> (This was later fixed by the EPT misconfig changes in Xen 4.5.)
> 
> On hosts which do not have forced IOMMU snooping then Xen allows for 
> uncached
> P2M mappings to be created otherwise cache issues would occur.
> 
> When hvmloader relocates RAM out of the HVM guests MMIO hole, Xen use's the
> default MTRR setting to determine the cache setting for the destination RAM.
> 
> hvmloader has not configured the default MTRR to enable write back caching 
> at
> the point the RAM is relocated and so the destination RAM ends up being
> configured as uncached.
> 
> Before the XSA-60 patches, the RAM would have it's cachability setting 
> changed
> to write back when the default MTRR was configured to write back.
> 
> After XSA-60 patches, this does not happen and the RAM remains as uncached.
> 
> This patch sets the default MTRR to be write back before relocating the RAM
> and restores the default MTRR after relocating the RAM.
> 
> The patch was designed to be be minimally invasive so it could be backported
> easily.

So why is this tagged XEN-4.2 then? The EPT misconfig handling got
introduced with 4.5, i.e. if there was such a (general) problem, it
ought to affect 4.3 and 4.4 as much. Of course, for the still
maintained trees backporting the EPT misconfig handling would be an
option. Tim and I agreed we wouldn't want to do this immediately,
but considered doing it eventually.

> @@ -197,25 +198,34 @@ void pci_setup(void)
>              ((pci_mem_start << 1) != 0) )
>          pci_mem_start <<= 1;
>  
> -    /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
> -    while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
> +    /* Workaround XENMEM_add_to_physmap creating uncached mappings
> +     * by setting default MTRR type to write back mapping */
> +    if ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>      {
> -        struct xen_add_to_physmap xatp;
> -        unsigned int nr_pages = min_t(
> -            unsigned int,
> -            hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
> -            (1u << 16) - 1);
> -        if ( hvm_info->high_mem_pgend == 0 )
> -            hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
> -        hvm_info->low_mem_pgend -= nr_pages;
> -        xatp.domid = DOMID_SELF;
> -        xatp.space = XENMAPSPACE_gmfn_range;
> -        xatp.idx   = hvm_info->low_mem_pgend;
> -        xatp.gpfn  = hvm_info->high_mem_pgend;
> -        xatp.size  = nr_pages;
> -        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
> -            BUG();
> -        hvm_info->high_mem_pgend += nr_pages;
> +        uint64_t mtrr_def_type = rdmsr(MSR_MTRRdefType);
> +        wrmsr(MSR_MTRRdefType, (1u << 11) | 6);
> +        /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
> +        while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
> +        {
> +            struct xen_add_to_physmap xatp;
> +            unsigned int nr_pages = min_t(
> +                unsigned int,
> +                hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
> +                (1u << 16) - 1);
> +            if ( hvm_info->high_mem_pgend == 0 )
> +                hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
> +            hvm_info->low_mem_pgend -= nr_pages;
> +            xatp.domid = DOMID_SELF;
> +            xatp.space = XENMAPSPACE_gmfn_range;
> +            xatp.idx   = hvm_info->low_mem_pgend;
> +            xatp.gpfn  = hvm_info->high_mem_pgend;
> +            xatp.size  = nr_pages;
> +            if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
> +                BUG();
> +            hvm_info->high_mem_pgend += nr_pages;
> +        }
> +        /* Restore previous default MTRR value */
> +        wrmsr(MSR_MTRRdefType, mtrr_def_type);

So the previous if() has become a while(), but I can't see why, nor
why the whole block needed its indentation changed (making it quite
a bit more difficult to spot any eventual change in that code block).

Also please make sure there are blank lines between declarations
and statements.

Jan

Re: [PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[Malcolm Crossley]

On 19/02/15 14:28, Jan Beulich wrote:
>>>> On 19.02.15 at 14:01, <malcolm.crossley@citrix.com> wrote:
>> Upstream commit 1c84d046735102e02d2df454ab07f14ac51f235d (XSA-60 fix)
>> Stopped tracking of setting/unsetting of uncached mode in an HVM guest.
>> (This was later fixed by the EPT misconfig changes in Xen 4.5.)
>>
>> On hosts which do not have forced IOMMU snooping then Xen allows for 
>> uncached
>> P2M mappings to be created otherwise cache issues would occur.
>>
>> When hvmloader relocates RAM out of the HVM guests MMIO hole, Xen use's the
>> default MTRR setting to determine the cache setting for the destination RAM.
>>
>> hvmloader has not configured the default MTRR to enable write back caching 
>> at
>> the point the RAM is relocated and so the destination RAM ends up being
>> configured as uncached.
>>
>> Before the XSA-60 patches, the RAM would have it's cachability setting 
>> changed
>> to write back when the default MTRR was configured to write back.
>>
>> After XSA-60 patches, this does not happen and the RAM remains as uncached.
>>
>> This patch sets the default MTRR to be write back before relocating the RAM
>> and restores the default MTRR after relocating the RAM.
>>
>> The patch was designed to be be minimally invasive so it could be backported
>> easily.
> 
> So why is this tagged XEN-4.2 then? The EPT misconfig handling got
> introduced with 4.5, i.e. if there was such a (general) problem, it
> ought to affect 4.3 and 4.4 as much. Of course, for the still
> maintained trees backporting the EPT misconfig handling would be an
> option. Tim and I agreed we wouldn't want to do this immediately,
> but considered doing it eventually.

I made a mistake and was intending to reply inline to Andreas, he was
using Xen 4.2 so I made the backport patch for his testing.

I wasn't sure on how to flag the backport for multi versions so I
replied to my own post stating that Xen 4.4 and older was affected.

Tim suggested posting this patch to see what general opinion was to this
style of backport fix. I attempted to backport the EPT misconfig to Xen
4.4 but I ended up with quite a large number of patches and got
concerned there would be a subtle error in my backport.

> 
>> @@ -197,25 +198,34 @@ void pci_setup(void)
>>              ((pci_mem_start << 1) != 0) )
>>          pci_mem_start <<= 1;
>>  
>> -    /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
>> -    while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>> +    /* Workaround XENMEM_add_to_physmap creating uncached mappings
>> +     * by setting default MTRR type to write back mapping */
>> +    if ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>>      {
>> -        struct xen_add_to_physmap xatp;
>> -        unsigned int nr_pages = min_t(
>> -            unsigned int,
>> -            hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
>> -            (1u << 16) - 1);
>> -        if ( hvm_info->high_mem_pgend == 0 )
>> -            hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
>> -        hvm_info->low_mem_pgend -= nr_pages;
>> -        xatp.domid = DOMID_SELF;
>> -        xatp.space = XENMAPSPACE_gmfn_range;
>> -        xatp.idx   = hvm_info->low_mem_pgend;
>> -        xatp.gpfn  = hvm_info->high_mem_pgend;
>> -        xatp.size  = nr_pages;
>> -        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>> -            BUG();
>> -        hvm_info->high_mem_pgend += nr_pages;
>> +        uint64_t mtrr_def_type = rdmsr(MSR_MTRRdefType);
>> +        wrmsr(MSR_MTRRdefType, (1u << 11) | 6);
>> +        /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
>> +        while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>> +        {
>> +            struct xen_add_to_physmap xatp;
>> +            unsigned int nr_pages = min_t(
>> +                unsigned int,
>> +                hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
>> +                (1u << 16) - 1);
>> +            if ( hvm_info->high_mem_pgend == 0 )
>> +                hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
>> +            hvm_info->low_mem_pgend -= nr_pages;
>> +            xatp.domid = DOMID_SELF;
>> +            xatp.space = XENMAPSPACE_gmfn_range;
>> +            xatp.idx   = hvm_info->low_mem_pgend;
>> +            xatp.gpfn  = hvm_info->high_mem_pgend;
>> +            xatp.size  = nr_pages;
>> +            if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>> +                BUG();
>> +            hvm_info->high_mem_pgend += nr_pages;
>> +        }
>> +        /* Restore previous default MTRR value */
>> +        wrmsr(MSR_MTRRdefType, mtrr_def_type);
> 
> So the previous if() has become a while(), but I can't see why, nor
> why the whole block needed its indentation changed (making it quite
> a bit more difficult to spot any eventual change in that code block).

I admit the patch diff does not read easily, I've added an extra outer
if conditional so that we are not rewriting the default MTRR unless we
are going to relocate RAM.

> Also please make sure there are blank lines between declarations
> and statements.

Thanks for the style feedback. Is it worth respinning this patch with
style fixes or do you wish to consider a different fix strategy for the
backports?

Re: [PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[Jan Beulich]

>>> On 19.02.15 at 15:47, <malcolm.crossley@citrix.com> wrote:
> On 19/02/15 14:28, Jan Beulich wrote:
>>>>> On 19.02.15 at 14:01, <malcolm.crossley@citrix.com> wrote:
>>> The patch was designed to be be minimally invasive so it could be backported
>>> easily.
>> 
>> So why is this tagged XEN-4.2 then? The EPT misconfig handling got
>> introduced with 4.5, i.e. if there was such a (general) problem, it
>> ought to affect 4.3 and 4.4 as much. Of course, for the still
>> maintained trees backporting the EPT misconfig handling would be an
>> option. Tim and I agreed we wouldn't want to do this immediately,
>> but considered doing it eventually.
> 
> I made a mistake and was intending to reply inline to Andreas, he was
> using Xen 4.2 so I made the backport patch for his testing.
> 
> I wasn't sure on how to flag the backport for multi versions so I
> replied to my own post stating that Xen 4.4 and older was affected.

Saying 4.4 in the subject and making clear that older versions are
also affected would probably have been best for an old-version-
only submission. Since we don't update 4.2 anymore anyway (other
than for security fixes), using that or older in the subject is really a
little odd (and risks being deleted without looking at).

> Tim suggested posting this patch to see what general opinion was to this
> style of backport fix. I attempted to backport the EPT misconfig to Xen
> 4.4 but I ended up with quite a large number of patches and got
> concerned there would be a subtle error in my backport.

Yeah, your change is certainly much smaller, so I'm not opposed
to going this route. I've Cc-ed Tim now so he could chime in if at
this point he thinks differently.

>>> @@ -197,25 +198,34 @@ void pci_setup(void)
>>>              ((pci_mem_start << 1) != 0) )
>>>          pci_mem_start <<= 1;
>>>  
>>> -    /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
>>> -    while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>>> +    /* Workaround XENMEM_add_to_physmap creating uncached mappings
>>> +     * by setting default MTRR type to write back mapping */
>>> +    if ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>>>      {
>>> -        struct xen_add_to_physmap xatp;
>>> -        unsigned int nr_pages = min_t(
>>> -            unsigned int,
>>> -            hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
>>> -            (1u << 16) - 1);
>>> -        if ( hvm_info->high_mem_pgend == 0 )
>>> -            hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
>>> -        hvm_info->low_mem_pgend -= nr_pages;
>>> -        xatp.domid = DOMID_SELF;
>>> -        xatp.space = XENMAPSPACE_gmfn_range;
>>> -        xatp.idx   = hvm_info->low_mem_pgend;
>>> -        xatp.gpfn  = hvm_info->high_mem_pgend;
>>> -        xatp.size  = nr_pages;
>>> -        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>>> -            BUG();
>>> -        hvm_info->high_mem_pgend += nr_pages;
>>> +        uint64_t mtrr_def_type = rdmsr(MSR_MTRRdefType);
>>> +        wrmsr(MSR_MTRRdefType, (1u << 11) | 6);
>>> +        /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
>>> +        while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
>>> +        {
>>> +            struct xen_add_to_physmap xatp;
>>> +            unsigned int nr_pages = min_t(
>>> +                unsigned int,
>>> +                hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
>>> +                (1u << 16) - 1);
>>> +            if ( hvm_info->high_mem_pgend == 0 )
>>> +                hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
>>> +            hvm_info->low_mem_pgend -= nr_pages;
>>> +            xatp.domid = DOMID_SELF;
>>> +            xatp.space = XENMAPSPACE_gmfn_range;
>>> +            xatp.idx   = hvm_info->low_mem_pgend;
>>> +            xatp.gpfn  = hvm_info->high_mem_pgend;
>>> +            xatp.size  = nr_pages;
>>> +            if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>>> +                BUG();
>>> +            hvm_info->high_mem_pgend += nr_pages;
>>> +        }
>>> +        /* Restore previous default MTRR value */
>>> +        wrmsr(MSR_MTRRdefType, mtrr_def_type);
>> 
>> So the previous if() has become a while(), but I can't see why, nor
>> why the whole block needed its indentation changed (making it quite
>> a bit more difficult to spot any eventual change in that code block).
> 
> I admit the patch diff does not read easily, I've added an extra outer
> if conditional so that we are not rewriting the default MTRR unless we
> are going to relocate RAM.

But afaict all you would need is putting the MTRR adjustment
inside the new if, keeping the old loop, and (perhaps using a
flag) undo the MTRR adjustment after the loop. Especially with
the goal of a non-invasive change this would seem much easier
to verify and backport to versions older than 4.4 (and, to whom
it may apply, even older than 4.2).

>> Also please make sure there are blank lines between declarations
>> and statements.
> 
> Thanks for the style feedback. Is it worth respinning this patch with
> style fixes or do you wish to consider a different fix strategy for the
> backports?

I'd really like the patch to be posted against 4.4 with the main code
change done in the above outlined less intrusive way. I'd expect
that the backport to even older versions then turns out relatively
straightforward. As you go you could then also adjust your additions
style wise.

Jan

Re: [PATCH] [XEN-4.2] hvmloader: Workaround uncached P2M mappings being created when relocating RAM

[Tim Deegan]

At 15:12 +0000 on 19 Feb (1424355171), Jan Beulich wrote:
> >>> On 19.02.15 at 15:47, <malcolm.crossley@citrix.com> wrote:
> > Tim suggested posting this patch to see what general opinion was to this
> > style of backport fix. I attempted to backport the EPT misconfig to Xen
> > 4.4 but I ended up with quite a large number of patches and got
> > concerned there would be a subtle error in my backport.
> 
> Yeah, your change is certainly much smaller, so I'm not opposed
> to going this route. I've Cc-ed Tim now so he could chime in if at
> this point he thinks differently.

I still think this is the better path - especially once it's been
adjusted as Jan suggests to make the patch even smaller and clearer.
One other comment, if you're respinning:

> >>> +        uint64_t mtrr_def_type = rdmsr(MSR_MTRRdefType);
> >>> +        wrmsr(MSR_MTRRdefType, (1u << 11) | 6);

Can this magic constant have a comment?  When it's used in cacheattr.c
it's commented as '/* E, default type WB */'.

Cheers,

Tim.