Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

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

> > +            for ( i = 0; i < memory_map.nr_map ; i++ )
> > +            {
> > +                if ( memory_map.map[i].type != E820_RAM )
> Here we're assuming that any region not marked as RAM is an RMRR.  Is that true?
>
> In any case, it would be just as strange to have a device BAR overlap
> with guest RAM as with an RMRR, wouldn't it?
OOPS! Actually I should take this,

if ( memory_map.map[i].type == E820_RESERVED )

This is same as when I check [RESERVED_MEMORY_DYNAMIC_START, 
RESERVED_MEMORY_DYNAMIC_END).
> > +                {
> > +                    uint64_t reserved_start, reserved_size;
> > +                    reserved_start = memory_map.map[i].addr;
> > +                    reserved_size = memory_map.map[i].size;
> > +                    if ( check_overlap(bar_data , bar_sz,
> > +                                   reserved_start, reserved_size) )
> > +                    {
> > +                        is_conflict = true;
> > +                        /* Now disable the memory or I/O mapping. */
> > +                        printf("pci dev %02x:%x bar %02x : 0x%08x : conflicts "
> > +                               "reserved resource so disable this device.!\n",
> > +                               devfn>>3, devfn&7, bar_reg, bar_data);
> > +                        cmd = pci_readw(devfn, PCI_COMMAND);
> > +                        pci_writew(devfn, PCI_COMMAND, ~cmd);
> > +                        break;
> > +                    }
> > +                }
> > +
> > +                /* Jump next device. */
> > +                if ( is_conflict )
> > +                {
> > +                    is_conflict = false;
> > +                    break;
> > +                }
> This conditional is still inside the memory_map loop; you want it one
> loop futher out, in the bar loop, don't you?
Here what I intended to do is if one of all bars specific to one given 
device already conflicts with RDM, its not necessary to continue check 
other remaining bars of this device and other RDM regions, we just 
disable this device simply then check next device.
> Also, if you declare is_conflict inside the devfn loop, rather than in
> the main function, then you don't need this "is_conflict=false" here.
>
> It might also be more sensible to use a goto instead; but this is one
This can work for me so it may be as follows:

    for ( devfn = 0; devfn < 256; devfn++ )
    {
 check_next_device:
        vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
        device_id = pci_readw(devfn, PCI_DEVICE_ID);
        if ( (vendor_id == 0xffff) && (device_id == 0xffff) )
            continue;
    ...
                    if ( check_overlap(bar_data , bar_sz,
                                   reserved_start, reserved_size) )
                    {
                        ...
                        /* Jump next device. */
                        devfn++;
                        goto check_next_device;
                    }


> where Jan will have a better idea what standard practice will be.
I can follow that again if Jan has any good implementation.

Thanks
Tiejun

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