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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 04.02.22 16:25, Jan Beulich wrote:
> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -454,6 +454,22 @@ 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. */
>> +
>> +#ifdef CONFIG_HAS_PCI_MSI
>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>> +    {
>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X 
>> enabled. */
>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>> +    }
>> +#endif
>> +
>> +    cmd_write(pdev, reg, cmd, data);
>> +}
> It's not really clear to me whether the TODO warrants this being a
> separate function. Personally I'd find it preferable if the logic
> was folded into cmd_write().
Not sure cmd_write needs to have guest's logic. And what's the
profit? Later on, when we decide how PCI_COMMAND can be emulated
this code will live in guest_cmd_write anyways
>
> With this and ...
>
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -70,6 +70,10 @@ static void control_write(const struct pci_dev *pdev, 
>> unsigned int reg,
>>   
>>           if ( vpci_msi_arch_enable(msi, pdev, vectors) )
>>               return;
>> +
>> +        /* Make sure guest doesn't enable INTx while enabling MSI. */
>> +        if ( !is_hardware_domain(pdev->domain) )
>> +            pci_intx(pdev, false);
>>       }
>>       else
>>           vpci_msi_arch_disable(msi, pdev);
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -92,6 +92,10 @@ static void control_write(const struct pci_dev *pdev, 
>> unsigned int reg,
>>           for ( i = 0; i < msix->max_entries; i++ )
>>               if ( !msix->entries[i].masked && msix->entries[i].updated )
>>                   update_entry(&msix->entries[i], pdev, i);
>> +
>> +        /* Make sure guest doesn't enable INTx while enabling MSI-X. */
>> +        if ( !is_hardware_domain(pdev->domain) )
>> +            pci_intx(pdev, false);
>>       }
>>       else if ( !new_enabled && msix->enabled )
>>       {
> ... this done (as requested) behind the back of the guest, what's the
> idea wrt the guest reading the command register? That continues to be
> wired to vpci_hw_read16() (and hence accesses the underlying hardware
> value irrespective of what patch 4 did).
Yes, good point. We need to add guest_cmd_read counterpart,
so we can also implement the same logic as in guest_cmd_write
wrt to INTx bit.
>
> Jan
>
Thank you,
Oleksandr