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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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

Thanks, Roger.