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.
> 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
