Re: [PATCH v2 1/1] vpci: Add resizable bar support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2024 12: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;
> }
> 
> Note this requires that the data parameter points to the BAR that
> matches the ReBAR control register, this needs adjusting in
> init_rebar().

SGTM.

>>>>>>> +        if ( rc )
>>>>>>> +        {
>>>>>>> +            printk("%pp: add register for PCI_REBAR_CAP failed 
>>>>>>> (rc=%d)\n",
>>>>>>> +                   &pdev->sbdf, rc);
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>> +
>>>>>>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, 
>>>>>>> rebar_ctrl_write,
>>>>>>> +                               rebar_offset + PCI_REBAR_CTRL, 4,
>>>>>>> +                               pdev->vpci->header.bars);
>>>>>>> +        if ( rc )
>>>>>>> +        {
>>>>>>> +            printk("%pp: add register for PCI_REBAR_CTRL failed %d\n",
>>>>>>> +                   &pdev->sbdf, rc);
>>>>>>> +            break;
>>>>>>
>>>>>> Is it correct to keep the other handler installed? After all ...
>>>>> Will change to "return rc;" here and above in next version.
>>>>
>>>> I'm not convinced this is what we want, as per ...
>>>>
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>
>>>>>> ... you - imo sensibly - aren't communicating the error back up (to allow
>>>>>> the device to be used without BAR resizing.
>>>>
>>>> ... what I said here.
>>> Sorry, I didn’t understand.
>>> Do you mean it is not enough to return error code once a handler failed to 
>>> be installed, I need to remove the already installed handlers?
>>
>> No, if you return an error here, nothing else needs doing. However, I
>> question that returning an error here is good or even necessary. In
>> the event of an error, the device ought to still be usable, just
>> without the BAR-resizing capability.
> 
> So you suggest that the capability should be hidden in that case?

Yes.

>  We
> have logic to hide capabilities, just not used for the hardware
> domain.  It would need some extra wiring to be capable of hiding
> failed capabilities.

Indeed.

Jan