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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.02.22 13:11, Roger Pau Monné wrote:
> On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote:
>>
>> 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?
> The newly introduced handler ends up calling the existing one,
But before doing so it implements guest specific logic which will be
extended as we add more bits of emulation
>   so in
> this case it might make sense to expand cmd_write to also cater for
> the domU case?
So, from the above I thought is was ok to have a dedicated handler
>
> I think we need to be sensible here in that we don't want to end up
> with handlers like:
>
> register_read(...)
> {
>     if ( is_hardware_domain() )
>         ....
>     else
>         ...
> }
>
> If there's shared code it's IMO better to not create as guest specific
> handler.
>
> It's also more risky to use the same handlers for dom0 and domU, as a
> change intended to dom0 only might end up leaking in the domU path and
> that could easily become a security issue.
So, just for your justification: BARs. Is this something we also want
to be kept separate or we want if (is_hwdom)?
I guess the former.
>
> Thanks, Roger.
Thank you,
Oleksandr