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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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