Re: [Xen-devel] [PATCH v4 7/8] libxc/pvh: set default MTRR type to write-back

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/10/18 16:52, Roger Pau Monné wrote:
> On Wed, Sep 26, 2018 at 01:29:27PM +0100, Andrew Cooper wrote:
>> On 08/06/18 10:59, Roger Pau Monne wrote:
>>> @@ -1014,6 +1034,30 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>>      if ( dom->start_info_seg.pfn )
>>>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>>>  
>>> +    /* Set the MTRR. */
>>> +    bsp_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
>>> +    bsp_ctx.mtrr_d.instance = 0;
>>> +    bsp_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
>>> +
>>> +    mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
>>> +    if ( !mtrr_record )
>>> +    {
>>> +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>>> +                     "%s: unable to get MTRR save record", __func__);
>>> +        goto out;
>>> +    }
>>> +
>>> +    memcpy(&bsp_ctx.mtrr, mtrr_record, sizeof(bsp_ctx.mtrr));
>>> +
>>> +    /* TODO: maybe this should be a firmware option instead? */
>>> +    if ( !dom->device_model )
>>> +        /*
>>> +         * Enable MTRR, set default type to WB.
>>> +         * TODO: add MMIO areas as UC when passthrough is supported.
>>> +         */
>>> +        bsp_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK |
>>> +                                         MTRR_DEF_TYPE_ENABLE;
>> This is buggy.  MTRRs are per-vcpu and expected to match.  This only
>> works by chance in the HVM case because all settings are still 0 at this
>> point.
> Yes, I will look into it ASAP, but I'm not sure if the current
> save/restore trick that's done here will be possible with vcpus that
> are down or I will need to expand the save/restore logic to allow
> setting the state of vcpus that are down.

This gets back into "what is an offline vcpu" argument which happened
recently on the Introspection side.

As far as I'm concerned, once the vcpus have been allocated
XEN_DOMCTL_max_vcpus, they are present and "around".

Xen should be responsible for putting them into the correct INIT state
(which is one of the many bugs I've yet to get fully fixed from the
debug handling side of things), and the domain builder is responsible
for making any changes that firmware would normally expect to make.

Whether the vcpus are scheduled or not is unrelated to the state (and
indeed set-ability) of their registers.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel