Re: [PATCH v6 10/13] vpci/header: reset the command register when adding devices

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 07.02.22 16:31, Jan Beulich wrote:
> On 07.02.2022 15:17, Oleksandr Andrushchenko wrote:
>>
>> On 07.02.22 14:54, Jan Beulich wrote:
>>> On 07.02.2022 13:51, Oleksandr Andrushchenko wrote:
>>>> On 07.02.22 14:38, Jan Beulich wrote:
>>>>> On 07.02.2022 12:27, Oleksandr Andrushchenko wrote:
>>>>>> On 07.02.22 09:29, Jan Beulich wrote:
>>>>>>> On 04.02.2022 15:37, Oleksandr Andrushchenko wrote:
>>>>>>>> On 04.02.22 16:30, Jan Beulich wrote:
>>>>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>>>>>> 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.
>>>>>>>>> It's not entirely clear to me whether setting the hardware register to
>>>>>>>>> zero is okay. What wants to be zero is the value the guest observes
>>>>>>>>> initially.
>>>>>>>> "the PCI spec says the PCI_COMMAND register is typically all 0's after 
>>>>>>>> reset."
>>>>>>>> Why wouldn't it be ok? What is the exact concern here?
>>>>>>> The concern is - as voiced is similar ways before, perhaps in other
>>>>>>> contexts - that you need to consider bit-by-bit whether overwriting
>>>>>>> with 0 what is currently there is okay. Xen and/or Dom0 may have put
>>>>>>> values there which they expect to remain unaltered. I guess
>>>>>>> PCI_COMMAND_SERR is a good example: While the guest's view of this
>>>>>>> will want to be zero initially, the host having set it to 1 may not
>>>>>>> easily be overwritten with 0, or else you'd effectively imply giving
>>>>>>> the guest control of the bit.
>>>>>> We have already discussed in great detail PCI_COMMAND emulation [1].
>>>>>> At the end you wrote [1]:
>>>>>> "Well, in order for the whole thing to be security supported it needs to
>>>>>> be explained for every bit why it is safe to allow the guest to drive it.
>>>>>> Until you mean vPCI to reach that state, leaving TODO notes in the code
>>>>>> for anything not investigated may indeed be good enough.
>>>>>>
>>>>>> Jan"
>>>>>>
>>>>>> So, this is why I left a TODO in the PCI_COMMAND emulation for now and 
>>>>>> only
>>>>>> care about INTx which is honored with the code in this patch.
>>>>> Right. The issue I see is that the description does not have any
>>>>> mention of this, but instead talks about simply writing zero.
>>>> How do you want that mentioned? Extended commit message or
>>>> just a link to the thread [1]?
>>> What I'd like you to describe is what the change does without
>>> fundamentally implying it'll end up being zero which gets written
>>> to the register. Stating as a conclusion that for the time being
>>> this means writing zero is certainly fine (and likely helpful if
>>> made explicit).
>> Xen and/or Dom0 may have put values in PCI_COMMAND which they expect
>> to remain unaltered. PCI_COMMAND_SERR bit is a good example: while the
>> guest's view of this will want to be zero initially, the host having set
>> it to 1 may not easily be overwritten with 0, or else we'd effectively
>> imply giving the guest control of the bit. Thus, PCI_COMMAND register needs
>> proper emulation in order to honor host's settings.
>>
>> There are examples of emulators [1], [2] which already deal with PCI_COMMAND
>> register emulation and it seems that at most they care about the only INTX
>> bit (besides IO/memory enable and bus muster which are write through).
>> It could be because in order to properly emulate the PCI_COMMAND register
>> we need to know about the whole PCI topology, e.g. if any setting in device's
>> command register is aligned with the upstream port etc.
>> This makes me think that because of this complexity others just ignore that.
>> Neither I think this can be easily done in Xen case.
>>
>> According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0", section "6.2.2
>> Device Control" says that the reset state of the command register is
>> typically 0, so reset the command register when assigning a PCI device
>> to a guest t all 0's and for now only make sure INTx bit is set according
>> to if MSI/MSI-X enabled.
> "... is typically 0, so when assigning a PCI device reset the guest view of
>   the command register to all 0's. For now our emulation only makes sure INTx
>   is set according to host requirements, i.e. depending on MSI/MSI-X enabled
>   state."
I'll put this description into PCI_COMMAND emulation patch
>
> Maybe? (Obviously a fresh device given to a guest will have MSI/MSI-X
> disabled, so I'm not sure that aspect really needs mentioning.)
>
> But: What's still missing here then is the separation of guest and host
> views. When we set INTx behind the guest's back, it shouldn't observe the
> bit set. Or is this meant to be another (big) TODO?
>
> Jan
>
Thank you,
Oleksandr