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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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?
>>> --- 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?

Thank you,
Oleksandr