Re: [Xen-devel] PVH and mtrr/PAT.........

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Tue, Dec 3, 2013 at 7:20 AM, Xu, Dongxiao <dongxiao.xu@xxxxxxxxx> wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@xxxxxxxxxxxxx
>> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Jan Beulich
>> Sent: Friday, November 22, 2013 6:29 PM
>> To: George Dunlap; Dong, Eddie; Nakajima, Jun
>> Cc: xen-devel; Tim Deegan
>> Subject: Re: [Xen-devel] PVH and mtrr/PAT.........
>>
>> >>> On 21.11.13 at 16:47, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> > On Wed, Nov 20, 2013 at 10:24 PM, Mukesh Rathor
>> > <mukesh.rathor@xxxxxxxxxx> wrote:
>> >> On Wed, 20 Nov 2013 18:12:13 +0000
>> >> George Dunlap <dunlapg@xxxxxxxxx> wrote:
>> >>
>> >>> On Wed, Nov 20, 2013 at 8:42 AM, Jan Beulich <JBeulich@xxxxxxxx>
>> >>> wrote:
>> >>> >>>> On 20.11.13 at 03:11, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> >>>> wrote:
>> >>> >> After rebasing my dom0 on latest, it didn't boot. After debugging
>> >>> >> couple days, it turned out to be :
>> >>> >>
>> >>> >> +    if ( is_pvh_domain(d) )
>> >>> >> +    {
>> >>> >> +        if ( direct_mmio )
>> >>> >> +            return MTRR_TYPE_UNCACHABLE;
>> >>> >> +        return MTRR_TYPE_WRBACK;
>> >>> >> +    }
>> >>> >> +
>> >>> >>
>> >>> >> I had in my patches, missing in epte_get_entry_emt() in latest.
>> >>> >>
>> >>> >> So, since I don't know much about this, is an HVM guest setting
>> >>> >> MTRR range types? Looking for suggestions on best way to do this
>> >>> >> for PVH.
>> >>> >
>> >>> > A HVM guest is permitted to write to (virtual) MTRRs, whereas a PV
>> >>> > guest isn't. I'm inclined to prefer PV behavior here to be used for
>> >>> > PVH (since, as explained by Dongxiao, MTRRs don't really matter
>> >>> > for VMX guests anyway, i.e. the setting of (virtual) MTRRs needs to
>> >>> > get translated to EPT memory types anyway, hence a PVH guest
>> >>> > ought to be fine ignoring the MTRRs altogether and handling memory
>> >>> > types exclusively via PAT mechanisms).
>> >>>
>> >>> Mukesh,
>> >>>
>> >>> Do you know why this line is having this effect?  For one, is it a
>> >>> matter of direct_mmio pages being given something other than
>> >>> UNCACHEABLE, or a matter of non-direct_mmio pages given something
>> >>> other than WRBACK?
>> >>>
>> >>> And is the problem that the guest is *not* setting MTRRs, or that the
>> >>> guest *is* setting MTRRs?
>> >>>
>> >>> I'd prefer to avoid having a special case for PVH in this path if
>> >>> possible.
>> >>
>> >> Without any changes to epte_get_entry_emt(), all types are being returned
>> >> as MTRR_TYPE_WRBACK for PVH because of:
>> >>
>> >>     if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
>> >>             return MTRR_TYPE_WRBACK;
>> >>
>> >> This is problem for low level non-ram pages being accessed in dom0,
>> >> (which interesting gave MCE errors). non-ram IO pages have to be
>> >> MTRR_TYPE_UNCACHABLE.
>> >
>> > Hmm -- that looks like a bug in the logic there.  AFAICT, there's no
>> > reason for the lack of IDENT_PT to change the memory type like that.
>> >
>> > Unfortunately the changeset that introduced this (77283249) has
>> > neither comments nor a proper description of what's going on, so it's
>> > hard to tell where this came from.
>>
>> While a commit without any description at all is clearly bogus (even
>> more so in the light that this is the very change that also caused
>> XSA-60), in the case here it introduces the function as a whole, and
>> hence would not have been likely to comment on why the function
>> was written the way it is.
>>
>> I take it that this goes alongside the other check immediately previous
>> to it: When not set yet, assume WB (for the sake of the tool stack).
>> But I think this tool stack requirement should be expressed without
>> looking at this HVM param.
>>
>> Sadly the person having written that code doesn't appear to be
>> working on Xen anymore (and my not be at Intel anymore either),
>> so I'm afraid we'll have to sort this out ourselves. Nevertheless -
>> Intel folks, can you comment on this please?
>
> Hi Jan,
>
> This is really a piece of old code, and we can't recall why this judgment 
> (IDENT_PT) is added in epte_get_entry_emt().
>
> From current view, these two lines are buggy and not necessary, and I will 
> make a patch to remove them.

Thanks, Dongxiao.  No rush with the patch -- it isn't causing any
functional issues at the moment (just code a bit uglier than
necessary), so I think I'd probably advise holding off applying it
until the 4.5 development window opens (hopefully sometime end of
January / beginning of February).  (You can of course get reviews and
acks in the mean time.)

 -George

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