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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> With the above done, do you think that writing 0's is an acceptable
> approach as of now?

Well, yes, provided we have a sufficiently similar understanding
of what "acceptable" here means.

Jan