diff mbox series

[V7,08/11] vpci/header: reset the command register when adding devices

Message ID 20220719174253.541965-9-olekstysh@gmail.com (mailing list archive)
State New, archived
Headers show
Series PCI devices passthrough on Arm, part 3 | expand

Commit Message

Oleksandr Tyshchenko July 19, 2022, 5:42 p.m. UTC 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reset the command register when assigning a PCI device to a guest:
according to the PCI spec the PCI_COMMAND register is typically all 0's
after reset, but this might not be true for the guest as it needs
to respect host's settings.
For that reason, do not write 0 to the PCI_COMMAND register directly,
but go through the corresponding emulation layer (cmd_write), which
will take care about the actual bits written.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v6:
- use cmd_write directly without introducing emulate_cmd_reg
- update commit message with more description on all 0's in PCI_COMMAND
Since v5:
- updated commit message
Since v1:
 - do not write 0 to the command register, but respect host settings.
---
 xen/drivers/vpci/header.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Rahul Singh July 26, 2022, 3:09 p.m. UTC | #1 
HI Oleksandr,

> On 19 Jul 2022, at 6:42 pm, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote:
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Reset the command register when assigning a PCI device to a guest:
> according to the PCI spec the PCI_COMMAND register is typically all 0's
> after reset, but this might not be true for the guest as it needs
> to respect host's settings.
> For that reason, do not write 0 to the PCI_COMMAND register directly,
> but go through the corresponding emulation layer (cmd_write), which
> will take care about the actual bits written.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> 

Reviewed-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
Jan Beulich July 26, 2022, 3:23 p.m. UTC | #2 
On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Reset the command register when assigning a PCI device to a guest:
> according to the PCI spec the PCI_COMMAND register is typically all 0's
> after reset, but this might not be true for the guest as it needs
> to respect host's settings.
> For that reason, do not write 0 to the PCI_COMMAND register directly,
> but go through the corresponding emulation layer (cmd_write), which
> will take care about the actual bits written.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> Since v6:
> - use cmd_write directly without introducing emulate_cmd_reg
> - update commit message with more description on all 0's in PCI_COMMAND

I agree with the change, but it's imo enough that you also need to sign
off on the patch (and this likely also applies elsewhere in the series).

Jan
Oleksandr Tyshchenko July 27, 2022, 8:58 a.m. UTC | #3 
On 26.07.22 18:23, Jan Beulich wrote:

Hello Jan

> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Reset the command register when assigning a PCI device to a guest:
>> according to the PCI spec the PCI_COMMAND register is typically all 0's
>> after reset, but this might not be true for the guest as it needs
>> to respect host's settings.
>> For that reason, do not write 0 to the PCI_COMMAND register directly,
>> but go through the corresponding emulation layer (cmd_write), which
>> will take care about the actual bits written.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> ---
>> Since v6:
>> - use cmd_write directly without introducing emulate_cmd_reg
>> - update commit message with more description on all 0's in PCI_COMMAND
> I agree with the change,

thanks, may I please ask can this be converted to some tag?


>   but it's imo enough that you also need to sign
> off on the patch (and this likely also applies elsewhere in the series).


I got your point. Regarding the current patch, I didn't make any changes 
to it. As I mentioned in the cover letter [1] after "!!! OT: please note,"

Oleksandr Andrushchenko has addressed almost all review comments given 
for v6 by himself. For the patches which I had to touch I added "OT:" to 
the change log. For example, like here [2].

I will consider signing off these patches.



[1] 
https://lore.kernel.org/xen-devel/20220719174253.541965-1-olekstysh@gmail.com/

[2] 
https://lore.kernel.org/xen-devel/20220719174253.541965-8-olekstysh@gmail.com/


>
> Jan
Jan Beulich July 27, 2022, 9:46 a.m. UTC | #4 
On 27.07.2022 10:58, Oleksandr wrote:
> On 26.07.22 18:23, Jan Beulich wrote:
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> Reset the command register when assigning a PCI device to a guest:
>>> according to the PCI spec the PCI_COMMAND register is typically all 0's
>>> after reset, but this might not be true for the guest as it needs
>>> to respect host's settings.
>>> For that reason, do not write 0 to the PCI_COMMAND register directly,
>>> but go through the corresponding emulation layer (cmd_write), which
>>> will take care about the actual bits written.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>> ---
>>> Since v6:
>>> - use cmd_write directly without introducing emulate_cmd_reg
>>> - update commit message with more description on all 0's in PCI_COMMAND
>> I agree with the change,
> 
> thanks, may I please ask can this be converted to some tag?

I could offer a R-b, but you've got one from Rahul already, so mine
won't buy you anything further. What you will need is a maintainer
ack, which imo isn't a priority since this is only patch 8 in a
series which itself depends on a further series likely continuing
to be controversial (didn't get there yet).

Jan
Oleksandr Tyshchenko July 27, 2022, 4:53 p.m. UTC | #5 
On 27.07.22 12:46, Jan Beulich wrote:

Hello Jan

> On 27.07.2022 10:58, Oleksandr wrote:
>> On 26.07.22 18:23, Jan Beulich wrote:
>>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>
>>>> Reset the command register when assigning a PCI device to a guest:
>>>> according to the PCI spec the PCI_COMMAND register is typically all 0's
>>>> after reset, but this might not be true for the guest as it needs
>>>> to respect host's settings.
>>>> For that reason, do not write 0 to the PCI_COMMAND register directly,
>>>> but go through the corresponding emulation layer (cmd_write), which
>>>> will take care about the actual bits written.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>> ---
>>>> Since v6:
>>>> - use cmd_write directly without introducing emulate_cmd_reg
>>>> - update commit message with more description on all 0's in PCI_COMMAND
>>> I agree with the change,
>> thanks, may I please ask can this be converted to some tag?
> I could offer a R-b, but you've got one from Rahul already, so mine
> won't buy you anything further. What you will need is a maintainer
> ack, which imo isn't a priority since this is only patch 8 in a
> series which itself depends on a further series likely continuing
> to be controversial (didn't get there yet).


ok, fair enough.


>
> Jan
diff mbox series

Patch

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 2ce69a63a2..1be9775dda 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -701,6 +701,10 @@  static int cf_check init_bars(struct pci_dev *pdev)
      */
     ASSERT(header->guest_cmd == 0);
 
+    /* Reset the command register for guests. */
+    if ( !is_hwdom )
+        cmd_write(pdev, PCI_COMMAND, 0, header);
+
     /* Setup a handler for the command register. */
     rc = vpci_add_register(pdev->vpci, cmd_read, cmd_write, PCI_COMMAND,
                            2, header);