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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jul 16, 2015 at 12:52 PM, Chen, Tiejun <tiejun.chen@xxxxxxxxx> wrote:
>>> +            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.

I know what you're trying to do; what I'm saying is I don't think it
does what you want it to do.

You have loops nested 3 deep:
1. for each dev
  2.  for each bar
    3. for each memory range

This conditional is in loop 3; you want it to be in loop 2.

(In fact, when you set is_conflict, you then break out of loop 3 back
into loop 2; so this code will never actually be run.)

 >> 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;
>                     }

I'm not a fan of hard-coding the loop continuing condition like this;
if I were going to do a goto, I'd want to go to the end of the loop.

Anyway, the code is OK as it is; I'd rather spend time working on
something that's more of a blocker.

 -George

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