Re: [PATCH v8 09/13] vpci/header: emulate PCI_COMMAND register for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.07.2023 15:32, Roger Pau Monné wrote:
> On Thu, Jul 20, 2023 at 12:32:33AM +0000, Volodymyr Babchuk wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> 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.
> 
> You speak about SERR here, yet in the code all bits are togglable by
> domUs.

I think this paragraph is meant to describe only what would need doing,
as per what's said ...

>> There are examples of emulators [1], [2] which already deal with PCI_COMMAND
>> register emulation and it seems that at most they care about is the only INTx
>                                                                       ^ stray?
>> bit (besides IO/memory enable and bus master 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 easily be done in Xen case.
>>
>> According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0", section "6.2.2
>> Device Control" the reset state of the command register is typically 0,
>> so when assigning a PCI device use 0 as the initial state for the guest's 
>> view
>> of the command register.
>>
>> For now our emulation only makes sure INTx is set according to the host
>> requirements, i.e. depending on MSI/MSI-X enabled state.
>>
>> This implementation and the decision to only emulate INTx bit for now
>> is based on the previous discussion at [3].

... through to down here. Yet I agree the title suggests otherwise, and
hence that initial paragraph is further misleading.

>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -486,11 +486,27 @@ static int modify_bars(const struct pci_dev *pdev, 
>> uint16_t cmd, bool rom_only)
>>      return 0;
>>  }
>>  
>> +/* TODO: Add proper emulation for all bits of the command register. */
>>  static void cf_check cmd_write(
>>      const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
>>  {

Note also the TODO being added here. Which course will need resolving
before any of this can become supported.

Jan