Re: [Xen-devel] [v7][RFC][PATCH 05/13] hvmloader/mmio: reconcile guest mmio with reserved device memory

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

On 2014/10/24 22:42, Jan Beulich wrote:
> > > > On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> wrote:
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -37,6 +37,44 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
> >   enum virtual_vga virtual_vga = VGA_none;
> >   unsigned long igd_opregion_pgbase = 0;
> >
> > +unsigned int need_skip_rmrr = 0;
> Static (and without initializer)?
static unsigned int need_skip_rmrr;

> > +
> > +/*
> > + * Check whether there exists mmio hole in the specified memory range.
> > + * Returns 1 if exists, else returns 0.
> > + */
> > +static int check_mmio_hole_confliction(uint64_t start, uint64_t memsize,
> I don't think the word "confliction" exists. "conflict" please.
s/check_mmio_hole_confliction/check_mmio_hole_conflict/g

> > +                           uint64_t mmio_start, uint64_t mmio_size)
> > +{
> > +    if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
> > +        return 0;
> > +    else
> > +        return 1;
> Make this a simple single return statement?
static int check_mmio_hole_conflict(uint64_t start, uint64_t memsize,
                                    uint64_t mmio_start, uint64_t 
mmio_size)
{
    if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
        return 0;

    return 1;
}

> > +}
> > +
> > +static int check_reserved_device_memory_map(uint64_t mmio_base,
> > +                                            uint64_t mmio_max)
> > +{
> > +    uint32_t i = 0;
> Pointless initializer.
>
> > +    uint64_t rdm_start, rdm_end;
> > +    int nr_entries = -1;
> And again.
>
> > +
> > +    nr_entries = hvm_get_reserved_device_memory_map();
> It's completely unclear why this can't be the variable's initializer.
    uint32_t i;
    uint64_t rdm_start, rdm_end;
    int nr_rdm_entries = hvm_get_reserved_device_memory_map();

> > +
> > +    for ( i = 0; i < nr_entries; i++ )
> > +    {
> > +        rdm_start = rdm_map[i].start_pfn << PAGE_SHIFT;
> > +        rdm_end = rdm_start + (rdm_map[i].nr_pages << PAGE_SHIFT);
> I'm pretty certain I pointed out before that you can't simply shift
> these fields - you risk losing significant bits.
I tried to go back looking into something but just found you were saying 
I shouldn't use PAGE_SHIFT and PAGE_SIZE at the same time. If I'm still 
missing could you show me what you expect?
> > +        if ( check_mmio_hole_confliction(rdm_start, (rdm_end - rdm_start),
> Pointless parentheses.
        if ( check_mmio_hole_conflict(rdm_start, rdm_end - rdm_start,

> > +                                         mmio_base, mmio_max - mmio_base) )
> > +        {
> > +            need_skip_rmrr++;
> > +        }
> > +    }
> > +
> > +    return nr_entries;
> > +}
> > +
> >   void pci_setup(void)
> >   {
> >       uint8_t is_64bar, using_64bar, bar64_relocate = 0;
> > @@ -58,7 +96,9 @@ void pci_setup(void)
> >           uint32_t bar_reg;
> >           uint64_t bar_sz;
> >       } *bars = (struct bars *)scratch_start;
> > -    unsigned int i, nr_bars = 0;
> > +    unsigned int i, j, nr_bars = 0;
> > +    int nr_entries = 0;
> And another pointless initializer. Plus as a count of something this
int nr_rdm_entries;

> surely wants to be "unsigned int". Also I guess the variable name
nr_rdm_entries should be literally unsigned int but this value always be 
set from  hvm_get_reserved_device_memory_map(),
nr_rdm_entries = hvm_get_reserved_device_memory_map()

I hope that return value can be negative value in some failed case

> is too generic - nr_rdm_entries perhaps?
Okay, s/nr_entries/nr_rdm_entries/g

> > @@ -363,11 +411,29 @@ void pci_setup(void)
> >               bar_data &= ~PCI_BASE_ADDRESS_IO_MASK;
> >           }
> >
> > + reallocate_mmio:
> >           base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
> >           bar_data |= (uint32_t)base;
> >           bar_data_upper = (uint32_t)(base >> 32);
> >           base += bar_sz;
> >
> > +        if ( need_skip_rmrr )
> > +        {
> > +            for ( j = 0; j < nr_entries; j++ )
> > +            {
> > +                rdm_start = rdm_map[j].start_pfn << PAGE_SHIFT;
> > +                rdm_end = rdm_start + (rdm_map[j].nr_pages << PAGE_SHIFT);
> > +                if ( check_mmio_hole_confliction(rdm_start,
> > +                                                 (rdm_end - rdm_start),
> > +                                                 base, bar_sz) )
> "base" was already updated by this point.
Looks I should insert these code fragments between

bar_data_upper = (uint32_t)(base >> 32);

and

base += bar_sz;

So (See below)

> > +                {
> > +                    resource->base = rdm_end;
> > +                    need_skip_rmrr--;
> > +                    goto reallocate_mmio;
> If you ever get here, the earlier determination of whether the MMIO
> hole is large enough may get invalidated. I.e. I'm afraid this approach
> is still to simplistic. Also to way you do the retry, the resulting bar_data
Here move 'reallocate_mmio' downward one line, and

s/resource->base = rdm_end;/base = (rdm_end  + bar_sz - 1) & 
~(uint64_t)(bar_sz - 1);
Then,
        ...
        base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
 reallocate_mmio:
        bar_data |= (uint32_t)base;
        bar_data_upper = (uint32_t)(base >> 32);

        if ( need_skip_rmrr )
        {
            for ( j = 0; j < nr_rdm_entries; j++ )
            {
                rdm_start = rdm_map[j].start_pfn << PAGE_SHIFT;
                rdm_end = rdm_start + (rdm_map[j].nr_pages << PAGE_SHIFT);
                if ( check_mmio_hole_conflict(rdm_start, rdm_end - 
rdm_start,
                                              base, bar_sz) )
                {
                    base = (rdm_end  + bar_sz - 1) & ~(uint64_t)(bar_sz 
- 1);
                    need_skip_rmrr--;
                    goto reallocate_mmio;
                }
            }
        }

        base += bar_sz;

Additionally, actually there are some original codes just following my 
codes:
        if ( need_skip_rmrr )
        {
                ...
        }

        base += bar_sz;

        if ( (base < resource->base) || (base > resource->max) )
        {
            printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
                   "resource!\n", devfn>>3, devfn&7, bar_reg,
                   PRIllx_arg(bar_sz));
            continue;
        }

This can guarantee we don't overwhelm the previous mmio range.

> and bar_data_upper will likely end up being garbage.
>
> Did you actually _test_ this code?
Actually in my real case those RMRR ranges are always below MMIO.

Thanks
Tiejun

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