Re: [PATCH v6 04/13] vpci: restrict unhandled read/write operations for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 08.02.22 11:05, Roger Pau Monné wrote:
> On Tue, Feb 08, 2022 at 08:00:28AM +0000, Oleksandr Andrushchenko wrote:
>> On 04.02.22 16:24, Oleksandr Andrushchenko wrote:
>>> On 04.02.22 16:11, Jan Beulich wrote:
>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>> A guest can read and write those registers which are not emulated and
>>>>> have no respective vPCI handlers, so it can access the HW directly.
>>>> I don't think this describes the present situation. Or did I miss where
>>>> devices can actually be exposed to guests already, despite much of the
>>>> support logic still missing?
>>> No, they are not exposed yet and you know that.
>>> I will update the commit message
>> BTW, all this work is about adding vpci for guests and of course this
>> is not going to be enabled right away.
>> I would like to hear the common acceptable way of documenting such
>> things: either we just say something like "A guest can read and write"
>> elsewhere or we need to invent something neutral not directly mentioning
>> what the change does. With the later it all seems a bit confusing IMO
>> as we do know what we are doing and for what reason: enable vpci for guests
>>>>> In order to prevent a guest from reads and writes from/to the unhandled
>>>>> registers make sure only hardware domain can access HW directly and restrict
>>>>> guests from doing so.
>>>> Tangential question: Going over the titles of the remaining patches I
>>>> notice patch 6 is going to deal with BAR accesses. But (going just
>>>> from the titles) I can't spot anywhere that vendor and device IDs
>>>> would be exposed to guests. Yet that's the first thing guests will need
>>>> in order to actually recognize devices. As said before, allowing guests
>>>> access to such r/o fields is quite likely going to be fine.
>>> Agree, I was thinking about adding such a patch to allow IDs,
>>> but finally decided not to add more to this series.
>>> Again, the whole thing is not working yet and for the development
>>> this patch can/needs to be reverted. So, either we implement IDs
>>> or not this doesn't change anything with this respect
>> Roger, do you want an additional patch with IDs in v7?
> I would expect a lot more work to be required, you need IDs and the
> Header type as a minimum I would say. And then in order to have
> something functional you will also need to handle the capabilities
> pointer.
>
> I'm fine for this to be added in a followup series. I think it's clear
> the status after this series is not going to be functional.
Ok, so let's first have something and then we can extend guest's support
This can go in parallel with other work on Arm which still waits
for this series to be accepted
>
>>>>> --- a/xen/drivers/vpci/vpci.c
>>>>> +++ b/xen/drivers/vpci/vpci.c
>>>>> @@ -215,11 +215,15 @@ int vpci_remove_register(struct vpci *vpci, unsigned int offset,
>>>>>     }
>>>>>     
>>>>>     /* Wrappers for performing reads/writes to the underlying hardware. */
>>>>> -static uint32_t vpci_read_hw(pci_sbdf_t sbdf, unsigned int reg,
>>>>> +static uint32_t vpci_read_hw(bool is_hwdom, pci_sbdf_t sbdf, unsigned int reg,
>>>>>                                  unsigned int size)
>>>> Was the passing around of a boolean the consensus which was reached?
>>> Was this patch committed yet?
>>>> Personally I'd fine it more natural if the two functions checked
>>>> current->domain themselves.
>>> This is also possible, but I would like to hear Roger's view on this as well
>>> I am fine either way
>> Roger, what's your maintainer's preference here? Additional argument
>> to vpci_read_hw of make it use current->domain internally?
> My recommendation would be to use current->domain. Handlers will
> always be executed in guest context, so there's no need to pass a
> parameter around.
ok, I'll use current->domain
>
> Thanks, Roger.
>
Thank you,
Oleksandr