Re: [PATCH v6 7/8] vpci/msi: Free MSI resources when init_msi() fails

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/6/25 18:09, Jan Beulich wrote:
> On 25.06.2025 11:47, Chen, Jiqian wrote:
>> On 2025/6/25 17:15, Jan Beulich wrote:
>>> On 25.06.2025 09:16, Chen, Jiqian wrote:
>>>> On 2025/6/24 18:17, Jan Beulich wrote:
>>>>> On 24.06.2025 11:49, Chen, Jiqian wrote:
>>>>>> On 2025/6/18 22:45, Jan Beulich wrote:
>>>>>>> On 12.06.2025 11:29, Jiqian Chen wrote:
>>>>>>>> --- a/xen/drivers/vpci/msi.c
>>>>>>>> +++ b/xen/drivers/vpci/msi.c
>>>>>>>> @@ -193,6 +193,33 @@ static void cf_check mask_write(
>>>>>>>>      msi->mask = val;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static int cf_check cleanup_msi(struct pci_dev *pdev)
>>>>>>>> +{
>>>>>>>> +    int rc;
>>>>>>>> +    unsigned int end, size;
>>>>>>>> +    struct vpci *vpci = pdev->vpci;
>>>>>>>> +    const unsigned int msi_pos = pdev->msi_pos;
>>>>>>>> +    const unsigned int ctrl = msi_control_reg(msi_pos);
>>>>>>>> +
>>>>>>>> +    if ( !msi_pos || !vpci->msi )
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    if ( vpci->msi->masking )
>>>>>>>> +        end = msi_pending_bits_reg(msi_pos, vpci->msi->address64);
>>>>>>>> +    else
>>>>>>>> +        end = msi_mask_bits_reg(msi_pos, vpci->msi->address64) - 2;
>>>>>>>> +
>>>>>>>> +    size = end - ctrl;
>>>>>>>> +
>>>>>>>> +    rc = vpci_remove_registers(vpci, ctrl, size);
>>>>>>>> +    if ( rc )
>>>>>>>> +        return rc;
>>>>>>>
>>>>>>> This is a difficult one: It's not a good idea to simply return here, yet
>>>>>>> at the same time the handling of the register we're unable to remove may
>>>>>>> still require e.g. ...
>>>>>>>
>>>>>>>> +    XFREE(vpci->msi);
>>>>>>>
>>>>>>> ... this. There may therefore be more work required, such that in the
>>>>>>> end we're able to ...
>>>>>>>
>>>>>>>> +    return vpci_add_register(pdev->vpci, vpci_hw_read16, NULL, ctrl, 
>>>>>>>> 2, NULL);
>>>>>>>
>>>>>>> ... try this at least on a best effort basis.
>>>>>>>
>>>>>>> More generally: I don't think failure here (or in other .cleanup hook
>>>>>>> functions) may go entirely silently.
>>>>>> Does below meet your modification expectations?
>>>>>
>>>>> Not sure, sorry. By "more" I really meant "more" (which may just be code
>>>>> auditing, results of which would need writing down, but which may also
>>>>> involve further code changes; see below).
>>>>>
>>>>>>     rc = vpci_remove_registers(vpci, ctrl, size);
>>>>>>     if ( rc )
>>>>>>         printk(XENLOG_ERR "%pd %pp: remove msi handlers fail rc=%d\n",
>>>>>>                pdev->domain, &pdev->sbdf, rc);
>>>>>>
>>>>>>     XFREE(vpci->msi);
>>>>>
>>>>> As I tried to indicate in my earlier reply, the freeing of this struct is
>>>>> safe only if the failure above would not leave any register handlers in
>>>>> place which still (without appropriate checking) use this struct.
>>>> Hmm, but all handlers added in init_msi() use this struct.
>>>> So it doesn't exist the case that when above unable to remove all handlers 
>>>> and still require xfree this struct.
>>>
>>> Well, in the end you say in different words what I did say, if I understand
>>> correctly. There are several options how to deal with that. One might be to
>>> have those handlers recognize the lack of that pointer, and behave like ...
>>>
>>>>>>     /*
>>>>>>      * The driver may not traverse the capability list and think device
>>>>>>      * supports MSI by default. So here let the control register of MSI
>>>>>>      * be Read-Only is to ensure MSI disabled.
>>>>>>      */
>>>>>>     rc = vpci_add_register(vpci, vpci_hw_read16, NULL, ctrl, 2, NULL);
>>>
>>> ... what is tried to be put in place here (and like "no handler installed"
>>> for other registers).
>> According to your suggest. What I can think of is when 
>> vpci_remove_registers() fails,
>> then lookup the MSI related handlers,
> 
> What exactly does this mean? Aiui if vpci_remove_registers() ends up failing,
> it may be the lookup which is the problem. Which isn't why this wasn't what
> I suggested. Instead I suggested to make the respective handlers deal with
> the case of vpci->msi being NULL. Which of course in turn would require
> passing pdev->vpci to the respective vpci_add_register(), not pdev->vpci->msi.

Like below?

@@ -37,7 +42,13 @@ static uint32_t cf_check control_read(
 static void cf_check control_write(
     const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
 {
-    struct vpci_msi *msi = data;
+    const struct vpci *vpci = data;
+
+    if ( !vpci->msi )
+        return;
+
+    const struct vpci_msi *msi = vpci->msi;
+
     unsigned int vectors = min_t(uint8_t,
                                  1u << MASK_EXTR(val, PCI_MSI_FLAGS_QSIZE),
                                  pdev->msi_maxvec);
@@ -239,7 +250,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
         return -ENOMEM;

     ret = vpci_add_register(pdev->vpci, control_read, control_write,
-                            msi_control_reg(pos), 2, pdev->vpci->msi);
+                            msi_control_reg(pos), 2, pdev->vpci);

> 
>> and set the read/write hook to be vpci_ignored_read()/vpci_ignored_write(),
> 
> But vpci_hw_read16() != vpci_ignored_read().
> 
>> and set the private data to be NULL.
>> Is it acceptable?
> 
> Altering already registered handler properties feels pretty fragile to me.
> 
> Jan

-- 
Best regards,
Jiqian Chen.