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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.02.22 11:52, Jan Beulich wrote:
> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
>>
>> On 08.02.22 11:33, Jan Beulich wrote:
>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
>>>> 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
>>> Why "will"? There's nothing conceptually wrong with putting all the
>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
>>> If and when we gain CET-IBT support on the x86 side (and I'm told
>>> there's an Arm equivalent of this), then to make this as useful as
>>> possible it is going to be desirable to limit the number of functions
>>> called through function pointers. You may have seen Andrew's huge
>>> "x86: Support for CET Indirect Branch Tracking" series. We want to
>>> keep down the number of such annotations; the vast part of the series
>>> is about adding of such.
>> Well, while I see nothing bad with that, from the code organization
>> it would look a bit strange: we don't differentiate hwdom in vpci
>> handlers, but instead provide one for hwdom and one for guests.
>> While I understand your concern I still think that at the moment
>> it will be more in line with the existing code if we provide a dedicated
>> handler.
> The existing code only deals with Dom0, and hence doesn't have any
> pairs of handlers.
This is fair
>   FTAOD what I said above applies equally to other
> separate guest read/write handlers you may be introducing. The
> exception being when e.g. a hardware access handler is put in place
> for Dom0 (for obvious reasons, I think).
@Roger, what's your preference here?
>
> Jan
>
Thank you,
Oleksandr