On 2018-01-23 01:33, Gustavo Lima Chaves wrote:
> * Jan Kiszka <[email protected]> [2018-01-22 23:18:32 +0100]:
>
>> On 2018-01-22 22:35, Gustavo Lima Chaves wrote:
>>> * Jan Kiszka <[email protected]> [2018-01-20 12:06:53 +0100]:
>>>
>>> [...]
>>>
>>>>> + cap = pci_get_cap(device, PCI_CAP_PCIE | JAILHOUSE_PCI_EXT_CAP);
>>>>> + if (!cap)
>>>>> + return;
>>>>> +
>>>>> + val = pci_read_config(device->info->bdf,
>>>>
>>>> Let's cache bdf in local var, will give us shorter statements.
>>>
>>> True, sorry for that.
>>>
>>>>
>>>>> + cap->start + PCIE_DEV_CAPS_REG, 4);
>>>>> +
>>>>> + if (!(val & PCI_DEV_CAPS_FLR)) {
>>>>> + printk("Skipping function level reset for "
>>>>> + "device %02x:%02x.%x: no support\n",
>>>>> + PCI_BDF_PARAMS(device->info->bdf));
>>>>
>>>> Why warn here, but not if the device isn't PCIe-capable?
>>>
>>> Because these days it is kinda unlikely to fall in that case, I guess.
>>> Should I log on both, then?
>>
>> Yes, that's better. If you pre-init val to 0, you can keep the test and
>> output above for both cases.
>
> OK. But now that you mention support for non-pcie devices, below, I
> guess we'll have to do something in the lines Linux does
> (__pci_reset_function_locked())? It checks for a (ancient?)
> conventional PCI capability for non-pcie devices and acts on it:
> Advanced Features
> (https://pcisig.com/sites/default/files/specification_documents/ECN_Conventional_Adv_Caps_27Jul06.pdf).
> I guess this might be a more correc way of acting on conventional
> ones, since pci_disable_device() (writing zeros to the command
> register, like we did in JH previously) seems unrelated to FLR (and
> the conventional PCI specs are not dictating any form of FLR receipt
> on the base text as well, just on that addendum). What do we do, ugh?
Simply what I suggested:
- use FLR where available
- warn if it's not
- unconditionally clear the command register and try to set INTx-off
>
>>
>>>
>>>>
>>>>> + return;
>>>>
>>>> If FLR is not supported, we should still do the silencing of the device
>>>> via the command register below. Just move the command reg update before
>>>> all the FLR-related logic.
>>>
>>> Have you tried that on devices without the PCI_DEV_CAPS_FLR bit set?
>>> At least here you get a Unsupported Request right away (and thus an
>>> AER). At least I understand from the spec that if reset is not
>>> supported, you should do nothing in that regard.
>>
>> I'm also talking about devices without PCIe capability. All those
>> devices should at least get their DMA turned off (via the command
>> register) when we do a "reset" on them.
>>
>>>
>>> [...]
>>>
>>>>> }
>>>>> }
>>>>> diff --git a/include/jailhouse/cell-config.h
>>>>> b/include/jailhouse/cell-config.h
>>>>> index cf50ef84..72c4facc 100644
>>>>> --- a/include/jailhouse/cell-config.h
>>>>> +++ b/include/jailhouse/cell-config.h
>>>>> @@ -141,6 +141,8 @@ struct jailhouse_pci_device {
>>>>> __u16 domain;
>>>>> __u16 bdf;
>>>>> __u32 bar_mask[6];
>>>>> + /** A cycle-count based value that Jailhouse will wait for the device
>>>>> to terminate any PCI transactions during function-level reset. This
>>>>> *won't* be filled up by jailhouse-config-create tool automatically, so
>>>>> tailor this to your devices and system. */
>>>>
>>>> Overlong line.
>>>
>>> I know, but is that a doxygen style comment? From my time playing with
>>> it, struct field documentation in Doxygen did not go well with new
>>> lines... or are there no Doxygen intentions there?
>
> Speaking of a default value for the cycles count (on the FLR), let's
> say we do a rough instruction cycle count based on, say,
> http://www.agner.org/optimize/instruction_tables.pdf. I used Atom for
> reference just because and from the disassembly of the code of the
> loop we get roughly 100 cycles, maximum.
>
> So what do we do for a default? This is hard enough to estimate
> already considering caching times, parallel execution, etc. Do we
> take, say, 1.2GHz as a reasonable average CPU clock and make the
> constant 1200000 to make that take 100ms (quick math here, there could
> be mistakes)?
Doesn't the PCI config access give us some more or less stable delay
that is much longer that the individual instruction cycles? Try to
measure that across some recent platforms, then derive from the fastest,
I would say.
Jan
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
