Re: [PATCH v3 08/11] vpci/header: Emulate PCI_COMMAND register for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 26.10.21 13:52, Roger Pau Monné wrote:
> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> Add basic emulation support for guests. At the moment only emulate
>> PCI_COMMAND_INTX_DISABLE bit, the rest is not emulated yet and left
>> as TODO.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> New in v2
>> ---
>>   xen/drivers/vpci/header.c | 35 ++++++++++++++++++++++++++++++++---
>>   1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index f23c956cde6c..754aeb5a584f 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -451,6 +451,32 @@ static void cmd_write(const struct pci_dev *pdev, 
>> unsigned int reg,
>>           pci_conf_write16(pdev->sbdf, reg, cmd);
>>   }
>>   
>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
>> +                            uint32_t cmd, void *data)
>> +{
>> +    /* TODO: Add proper emulation for all bits of the command register. */
>> +
>> +    if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 )
>> +    {
>> +        /*
>> +         * Guest wants to enable INTx. It can't be enabled if:
>> +         *  - host has INTx disabled
>> +         *  - MSI/MSI-X enabled
>> +         */
>> +        if ( pdev->vpci->msi->enabled )
>> +            cmd |= PCI_COMMAND_INTX_DISABLE;
>> +        else
>> +        {
>> +            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
>> +
>> +            if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
>> +                cmd |= PCI_COMMAND_INTX_DISABLE;
>> +        }
> This last part should be Arm specific. On other architectures we
> likely want the guest to modify INTx disable in order to select the
> interrupt delivery mode for the device.
This is not arch specific as we just do not allow INTx to be enabled
if MSI/MSI-X has been enabled before. This was discussed previously
(Jan) and this was pointed as an acceptable approach to limit the
guest from having inconsistent configuration
>
> I really wonder if we should allow the guest to play with any other
> bit apart from INTx disable and memory and IO decoding on the command
> register.
This needs to be implemented one day when we understand what
this emulation should look like. This is why I have a "TODO" above.
>
> Thanks, Roger.
Thank you,
Oleksandr