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.
