Re: [PATCH 2/2] vpci: use named rangeset for BARs

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 22.11.21 13:08, Jan Beulich wrote:
> On 22.11.2021 11:59, Oleksandr Andrushchenko wrote:
>> On 22.11.21 12:54, Jan Beulich wrote:
>>> On 22.11.2021 11:50, Oleksandr Andrushchenko wrote:
>>>> On 22.11.21 12:43, Jan Beulich wrote:
>>>>> On 22.11.2021 11:27, Roger Pau Monné wrote:
>>>>>> On Mon, Nov 22, 2021 at 11:28:25AM +0200, Oleksandr Andrushchenko wrote:
>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>> @@ -206,12 +206,16 @@ static void defer_map(struct domain *d, struct 
>>>>>>> pci_dev *pdev,
>>>>>>>     static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, 
>>>>>>> bool rom_only)
>>>>>>>     {
>>>>>>>         struct vpci_header *header = &pdev->vpci->header;
>>>>>>> -    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>>>> +    struct rangeset *mem;
>>>>>>> +    char str[32];
>>>>>>>         struct pci_dev *tmp, *dev = NULL;
>>>>>>>         const struct vpci_msix *msix = pdev->vpci->msix;
>>>>>>>         unsigned int i;
>>>>>>>         int rc;
>>>>>>>     
>>>>>>> +    snprintf(str, sizeof(str), "%pp", &pdev->sbdf);
>>>>>>> +    mem = rangeset_new(NULL, str, RANGESETF_no_print);
>>>>>> You are still not adding the rangeset to the domain list, as the first
>>>>>> parameter passed here in NULL instead of a domain struct.
>>>>>>
>>>>>> Given the current short living of the rangesets I'm not sure it makes
>>>>>> much sense to link them to the domain ATM, but I guess this is kind of
>>>>>> a preparatory change as other patches you have will have the
>>>>>> rangesets permanent as long as the device is assigned to a domain.
>>>>>>
>>>>>> Likely the above reasoning (or the appropriate one) should be added to
>>>>>> the commit message.
>>>> If I fold then there is no reason to add the comment, right?
>>> I'd say you still want to justify the change from "anonymous" to "named and
>>> accounted".
>> "Make the range sets permanent, e.g. create them when a PCI device is
>> added and destroy when it is removed. Also make the range sets named
>> and accounted."
> Making them permanent is a requirement of your change iirc, so I'd start with
> "Because the range sets now become permanent, make them ...".
"As the range sets are now created when a PCI device is added and destroyed
when it is removed so make them named and accounted."
>
> Jan
>
>
Thank you,
Oleksandr