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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

We should check to make sure that this conditional is actually
necessary; since the HVM builder unconditionally sets IDENT_PT, I
don't think this conditional has ever actually been tested.
(Obviously too late to change this for 4.4.)

In the mean time, adding "is_pvh_vcpu()" here should be fine.  If the
conditional turns out to be necessary, we can either change this to a
"can_disable_paging" flag at some point in the future, or, if we want
to allow the guest to disable paging, actually set up an IDENT_PT for
PVH guests.

>
> After changing this to,
>     if ( !is_pvh_vcpu(v) &&
>          !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
>
> I started hitting if ( direct_mmio ), and getting proper UNCACHABLE
> for io pages, but RAM pages started being returned as UNCACHABLE also
> thru get_mtrr_type() which I've not investigated.

Presumably because the PVH guests aren't actually setting up their
MTRRs, whereas HVM guests do?

One solution of course would be to add MTRRs to the PVH interface.  A
more flexible option might be to set up default MTRRs in the domain
builder (which the guest can change if they want).

Or, we can change this line:
     gmtrr_mtype = get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT));

to this:
     gmtrr_mtype = is_hvm_domain(v) ?
get_mtrr_type(&v->arch.hvm_vcpu.mtrr, (gfn << PAGE_SHIFT)) :
MTRR_TYPE_WRBACK;

(Line-wrapped obviously.)

>
> For domU, it's incorrect, but happens to work because of:
>
>     if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
>             return MTRR_TYPE_WRBACK;
>
> as domU only has RAM pages, and thus WRBACK is correct for all.
>
> My quick fix while we come up with better solution was:
> -----------
> +    /* PVH fixme: Add support for more memory types. */
> +    if ( is_pvh_domain(d) )
> +    {
> +        if ( direct_mmio )
> +            return MTRR_TYPE_UNCACHABLE;
> +        return MTRR_TYPE_WRBACK;
> +    }
> +
>      if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
> ---------------
>
> It appears you didn't check all places where params was being used
> before adding it for PVH.

Given that there was no comment explaining why you were adding in the
special case above, and that it was not actually necessary for the
domU case (and so it passed my testing), it's not too surprising that
I missed it.  I figured this was another example of the "disable
everything and enable it as you need it" philosophy.

I am sorry that it took you so long to track down -- I thought I had
mentioned it in my cover letter, but apparently only indirectly.

 -George

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