On 2024/12/11 16:16, Roger Pau Monné wrote:
> On Wed, Dec 11, 2024 at 06:37:30AM +0000, Chen, Jiqian wrote:
>> On 2024/12/10 19:25, Roger Pau Monné wrote:
>>> On Tue, Dec 10, 2024 at 10:54:43AM +0100, Jan Beulich wrote:
>>>> On 10.12.2024 08:57, Chen, Jiqian wrote:
>>>>> On 2024/12/10 15:17, Jan Beulich wrote:
>>>>>> On 10.12.2024 08:07, Chen, Jiqian wrote:
>>>>>>> On 2024/12/9 21:59, Jan Beulich wrote:
>>>>>>>> On 02.12.2024 07:09, Jiqian Chen wrote:
>>>>>>>>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>>>>>>>>> + unsigned int reg,
>>>>>>>>> + uint32_t val,
>>>>>>>>> + void *data)
>>>>>>>>> +{
>>>>>>>>> + uint64_t size;
>>>>>>>>> + unsigned int index;
>>>>>>>>> + struct vpci_bar *bars = data;
>>>>>>>>> +
>>>>>>>>> + if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) &
>>>>>>>>> PCI_COMMAND_MEMORY )
>>>>>>>>> + return;
>>>>>>>>
>>>>>>>> I don't think something like this can go uncommented. I don't think the
>>>>>>>> spec mandates to drop writes in this situation?
>>>>>>> Spec says: Software must clear the Memory Space Enable bit in the
>>>>>>> Command register before writing the BAR Size field.
>>>>>>> This check is suggested by Roger and it really helps to prevent
>>>>>>> erroneous writes in this case,
>>>>>>> such as the result of debugging with Roger in the previous version.
>>>>>>> I will add the spec's sentences as comments here in next version.
>>>>>>
>>>>>> What you quote from the spec may not be enough as a comment here. There's
>>>>>> no direct implication that the write would simply be dropped on the floor
>>>>>> if the bit is still set. So I think you want to go a little beyond just
>>>>>> quoting from the spec.
>>>>> How about quoting Roger's previous words: " The memory decoding must be
>>>>> disabled before writing the BAR size field.
>>>>> Otherwise changing the BAR size will lead to the active p2m mappings
>>>>> getting out of sync w.r.t. the new BAR size." ?
>>>>
>>>> That'll be better, but imo still not enough to explain the outright
>>>> ignoring
>>>> of the write.
>>>
>>> I think we might want to do something along the lines of:
>>>
>>> uint64_t size = PCI_REBAR_CTRL_SIZE(val);
>>> struct vpci_bar *bar = data;
>>>
>>> if ( bar->enabled )
>>> {
>>> if ( size == bar->size )
>>> return;
>>>
>>> /*
>>> * Refuse to resize a BAR while memory decoding is enabled, as
>>> * otherwise the size of the mapped region in the p2m would become
>>> * stale with the newly set BAR size, and the position of the BAR
>>> * would be reset to undefined. Note the PCIe specification also
>>> * forbids resizing a BAR with memory decoding enabled.
>>> */
>>> gprintk(XENLOG_ERR,
>>> "%pp: refuse to resize BAR with memory decoding enabled\n",
>>> &pci->sbdf);
>>> return;
>>> }
>> Thank you very much!
>>
>>>
>>> Note this requires that the data parameter points to the BAR that
>>> matches the ReBAR control register, this needs adjusting in
>>> init_rebar().
>> I think I can keep current implementation of init_rebar() and use
>> bars[index] to get the corresponding BAR.
>
> IMO it would be best if you can pass the corresponding bar struct into
> the handler directly, as that will avoid having to do a PCI read just
> to get the BAR index from PCI_REBAR_CTRL. It should also avoid the
> need for the index and BAR type checks in rebar_ctrl_write().
OK, if so, then I need to move the logic of getting index from PCI_REBAR_CTRL
register and checking the BAR type into init_rebar(), right?
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
