Re: [Xen-devel] [v6][PATCH 6/7] hvmloader: check to reserved device memory maps in e820

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/9/11 23:57, Jan Beulich wrote:
> > > > On 10.09.14 at 07:49, <tiejun.chen@xxxxxxxxx> wrote:
> > +static unsigned int construct_rdm_e820_maps(unsigned int nr,
> > +                                            uint32_t nr_map,
> > +                                            struct 
> > xen_mem_reserved_device_memory *map,
> > +                                            struct e820entry *e820)
> > +{
> > +    unsigned int i = 0, j = 0, m = 0, sum_nr = nr + nr_map;
> Some or all of the initializers here are pointless.
    unsigned int i, j, m, sum_nr = nr + nr_map;
    uint64_t start, end, rdm_start, rdm_end;
    unsigned int insert, do_insert;

> > +    uint64_t start, end, rdm_start, rdm_end;
> > +    unsigned int insert = 0, do_insert = 0;
> > +
> > + do_real_construct:
> > +    for ( i = 0; i < nr_map; i++ )
> > +    {
> > +        rdm_start = map[i].start_pfn << PAGE_SHIFT;
> > +        rdm_end = rdm_start + map[i].nr_pages * PAGE_SIZE;
> Same comment as for an earlier patch.
        rdm_start = map[i].start_pfn << XC_PAGE_SHIFT;
        rdm_end = rdm_start + map[i].nr_pages << XC_PAGE_SHIFT;

But there's no such definition in tools/firmware/, and this is defined 
in xenctl.h. But if I include xenctl.h here, something else is 
redefined. So here I have to define this, XC_PAGE_SHIFTseparately.
> > +
> > +        for ( j = 0; j < nr - 1; j++ )
> > +        {
> > +            end = e820[j].addr + e820[j].size;
> > +            start = e820[j+1].addr;
> So now you don't check the last region at all? You just need to make
> this assignment conditional upon whether you're in the last iteration.
I remember we discussed this previously.

"
> And this would skip the last region - I'm not sure that's correct.
>

No. Here we just want to know where we should insert a entry, so

    for ( j = 0; j < nr - 1; j++ )
    {
        end = e820[j].addr + e820[j].size;
        start = e820[j+1].addr;

For example, nr = 3,

    for ( j = 0; j < 2; j++ )

So
#1,
        end = e820[0].addr + e820[0].size;
        start = e820[1].addr;

#2,
        end = e820[1].addr + e820[1].size;
        start = e820[2].addr;
"

I didn't you have further comments there.

> > +
> > +            /* Between those existing e820 entries. */
> > +            if ( (rdm_start > end) && (rdm_end < start) )
> > +            {
> > +                if ( do_insert )
> > +                {
> > +                    /* Move to free this entry. */
> > +                    for ( m = sum_nr - 1; m > j; m-- )
> > +                    {
> > +                        e820[m].addr = e820[m-1].addr;
> > +                        e820[m].size = e820[m-1].size;
> > +                        e820[m].type = e820[m-1].type;
> > +                    }
> You know of the memmove() function, don't you?
I will try this.

> > +
> > +                    /* Then fill RMRR into that entry. */
> > +                    e820[j+1].addr = rdm_start;
> > +                    e820[j+1].size = rdm_end - rdm_start;
> > +                    e820[j+1].type = E820_RESERVED;
> > +                    nr++;
> > +                }
> > +                insert++;
> > +            }
> > +            /* Already at the end. */
> > +            else if ( (rdm_start > end) && !start )
> How would !start represent the end of anything?
end = e820[j].addr + e820[j].size;
start = e820[j+1].addr;

so if end = 0xXXXXXXXX and start = 0, does this mean we already are at 
the end of all valid e820 entries?
> > +            {
> > +                if ( do_insert )
> > +                {
> > +                    e820[nr].addr = rdm_start;
> > +                    e820[nr].size = rdm_end - rdm_start;
> > +                    e820[nr].type = E820_RESERVED;
> > +                    nr++;
> > +                }
> > +                insert++;
> > +            }
> > +        }
> > +    }
> > +
> > +    /* Just return if done. */
> > +    if ( do_insert )
> > +        return nr;
> > +
> > +    /* Fine to construct RDM mappings into e820. */
> > +    if ( insert == nr_map )
> > +    {
> > +        do_insert = 1;
> > +        goto do_real_construct;
> > +    }
> > +    /* Overlap. */
> > +    else
> > +    {
> > +        printf("RDM overlap with those existing e820 entries!\n");
> > +        printf("So we don't construct RDM mapping in e820!\n");
> > +    }
> > +
> > +    return nr;
> > +}
> >   /* Create an E820 table based on memory parameters provided in hvm_info. */
> Blank line missing above.
Will fixed.

Thanks
Tiejun

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