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

[Malcolm Crossley <malcolm.crossley@xxxxxxxxxx>]

On 19/02/15 14:28, Jan Beulich wrote:
>>>> On 19.02.15 at 14:01, <malcolm.crossley@xxxxxxxxxx> 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?


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