On 25.06.2025 12:26, Chen, Jiqian wrote:
> 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);
>
Yes, with control_read() also suitably adjusted. And assuming there are no other
caveats.
Jan
