Re: [PATCH v5 4/9] xen/x86: populate PVHv2 Dom0 physical memory map

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.01.17 at 17:04, <roger.pau@citrix.com> wrote:
> On Fri, Jan 27, 2017 at 08:11:56AM -0700, Jan Beulich wrote:
>> >>> On 27.01.17 at 13:23, <roger.pau@citrix.com> wrote:
>> > On Thu, Jan 26, 2017 at 05:41:58AM -0700, Jan Beulich wrote:
>> >> >>> On 19.01.17 at 18:29, <roger.pau@citrix.com> wrote:
>> >> > @@ -43,6 +44,9 @@ static long __initdata dom0_nrpages;
>> >> >  static long __initdata dom0_min_nrpages;
>> >> >  static long __initdata dom0_max_nrpages = LONG_MAX;
>> >> >  
>> >> > +/* Size of the VM86 TSS for virtual 8086 mode to use. */
>> >> > +#define HVM_VM86_TSS_SIZE   128
>> >> 
>> >> I continue to be puzzled by this value. Why 128? I think this really
>> >> needs to be clarified in the comment.
>> > 
>> > Given the recent comments by Tim, and that this is starting to look like a can
>> > of worms, I would like to leave this as-is for the moment, on the grounds that
>> > it's what hvmloader does (I'm not saying it's right), and that this issue
>> > should be treated independently from this patch series.
>> 
>> Well, for the purpose of this patch it would be sufficient if the
>> comment referred to hvmloader. But then I think I saw you set the
>> TSS limit to 0x67, which is neither in line with the value above nor
> 
> Hm, no, I'm not setting the limit anywhere here, this is done in
> vmx_set_segment_register,

Well, you do, in patch 8 (in pvh_setup_cpus()). But that's a different
TSS, so the limits are independent. It's just what I had in mind here.

> and it's indeed set to 0xff which is wrong for
> hvmloader too according to the conversation that's going on related to this
> HVM_VM86_TSS_SIZE param.

Right.

>> - according to what Tim said (but I didn't check myself yet) - the
>> 255 used in hvmloader. I.e. if you clone hvmloader code, all
>> aspects of it should match.
>> 
>> > Alternatively, I can just remove setting HVM_PARAM_VM86_TSS for a PVHv2 Dom0.
>> > IIRC I've tried that before (without unrestricted mode support) and it was
>> > working fine.
>> 
>> Now if that's the case, then why bother with the TSS?
> 
> It seems like it working was just luck, but I don't know all the details. Maybe
> the emulator is somehow fixing this up when the TSS is corrupted/incorrect?

I don't think so. Btw, why is the kernel dropping back into real mode
anyway? It's being started in protected mode after all.

>> > Given the change that you requested in pvh_steal_ram, now the start of the
>> > memory area returned by it it's not going to be page-aligned, so I will have to
>> > perform the TSS setup first, and then the identity page tables.
>> 
>> Or simply pass the required alignment.
> 
> Passing an alignment here would mean that pvh_steal_ram would have to return 2
> pages in order to meet this alignment, and we would end up wasting memory.
> Also, this is the only caller of pvh_steal_ram that requires alignment. This is
> what I have after changing pvh_steal_ram to remove RAM from the end of the
> region:
> 
> static int __init pvh_setup_vmx_realmode_helpers(struct domain *d)
> {
>     p2m_type_t p2mt;
>     uint32_t rc, *ident_pt;
>     uint8_t *tss;
>     mfn_t mfn;
>     paddr_t gaddr;
> 
>     /*
>      * Steal some space from the last found RAM region. One page will be
>      * used for the identity page tables, and the remaining space for the
>      * VM86 TSS. Note that after this not all e820 regions will be aligned
>      * to PAGE_SIZE.
>      */
>     if ( pvh_steal_ram(d, PAGE_SIZE + HVM_VM86_TSS_SIZE, GB(4), &gaddr) )
>     {
>         printk("Unable to find memory to stash the identity map and TSS\n");
>         return -ENOMEM;
>     }
> 
>     tss = map_domain_gfn(p2m_get_hostp2m(d), _gfn(PFN_DOWN(gaddr)),
>                          &mfn, &p2mt, 0, &rc);
>     if ( tss )
>     {
>         memset(tss, 0, HVM_VM86_TSS_SIZE);
>         unmap_domain_page(tss);
>         put_page(mfn_to_page(mfn_x(mfn)));
>         d->arch.hvm_domain.params[HVM_PARAM_VM86_TSS] = gaddr;
>     }
>     else
>         printk("Unable to map VM86 TSS area\n");
> 
>     gaddr += HVM_VM86_TSS_SIZE;
>     ASSERT(IS_ALIGNED(gaddr, PAGE_SIZE));

And this assert holds merely because, prior to this function running,
all E820 entries are page aligned? That's rather fragile then.
Considering that getting into here is going to be increasingly unlikely
going forward, I don't think we should be afraid of wasting a little
bit of memory here.

Jan

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