[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers
On 31.01.2022 14:41, Oleksandr Andrushchenko wrote: > On 31.01.22 15:36, Jan Beulich wrote: >> On 31.01.2022 14:30, Oleksandr Andrushchenko wrote: >>> On 31.01.22 13:39, Jan Beulich wrote: >>>> On 31.01.2022 12:23, Oleksandr Andrushchenko wrote: >>>>> On 31.01.22 13:10, Roger Pau Monné wrote: >>>>>> Right (see my previous reply to this comment). I think it would be >>>>>> easier (and cleaner) if you switched the default behavior regarding >>>>>> unhandled register access for domUs at the start of the series (drop >>>>>> writes, reads returns ~0), and then you won't need to add all those >>>>>> dummy handler to drop writes and return ~0 for reads. >>>>>> >>>>>> It's going to be more work initially as you would need to support >>>>>> passthrough of more registers, but it's the right approach that we >>>>>> need implementation wise. >>>>> While I agree in general, this effectively means that I'll need to provide >>>>> handling for all PCIe registers and capabilities from the very start. >>>>> Otherwise no guest be able to properly initialize a PCI device without >>>>> that. >>>>> Of course, we may want starting from stubs instead of proper emulation, >>>>> which will direct the access to real HW and later on we add proper >>>>> emulation. >>>>> But, again, this is going to be a rather big piece of code where we need >>>>> to explicitly handle every possible capability. >>>> Since the two sub-threads are now about exactly the same topic, I'm >>>> answering here instead of there. >>>> >>>> No, you are not going to need to emulate all possible capabilities. >>>> We (or really qemu) don't do this on x86 either. Certain capabilities >>>> may be a must, but not everything. There are also device specific >>>> registers not covered by any capability structures - what to do with >>>> those is even more of a question. >>>> >>>> Furthermore for some of the fields justification why access to the >>>> raw hardware value is fine is going to be easy: r/o fields like >>>> vendor and device ID, for example. But every bit you allow direct >>>> access to needs to come with justification. >>>> >>>>> At the moment we are not going to claim that vPCI provides all means to >>>>> pass through a PCI device safely with this respect and this is why the >>>>> feature >>>>> itself won't even be a tech preview yet. For that reason I think we can >>>>> still >>>>> have implemented only crucial set of handlers and still allow the rest to >>>>> be read/write directly without emulation. >>>> I think you need to separate what you need for development from what >>>> goes upstream: For dev purposes you can very well invert the policy >>>> from white- to black-listing. But if we accepted the latter into the >>>> main tree, the risk would be there that something gets missed at the >>>> time where the permission model gets changed around. >>>> >>>> You could even have a non-default mode operating the way you want it >>>> (along the lines of pciback's permissive mode), allowing you to get >>>> away without needing to carry private patches. Things may also >>>> initially only work in that mode. But the default should be a mode >>>> which is secure (and which perhaps initially offers only very limited >>>> functionality). >>> Ok, so to make it clear: >>> 1. We do not allow unhandled access for guests: for that I will create a >>> dedicated patch which will implement such restrictions. Something like >>> the below (for both vPCI read and write): >>> >>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >>> index c5e67491c24f..9ef2a1b5af58 100644 >>> --- a/xen/drivers/vpci/vpci.c >>> +++ b/xen/drivers/vpci/vpci.c >>> @@ -347,6 +347,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, >>> unsigned int size) >>> const struct vpci_register *r; >>> unsigned int data_offset = 0; >>> uint32_t data = ~(uint32_t)0; >>> + bool handled = false; >>> >>> if ( !size ) >>> { >>> @@ -405,6 +406,8 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, >>> unsigned int size) >>> if ( cmp > 0 ) >>> continue; >>> >>> + handled = true; /* Found the handler for this access. */ >>> + >>> if ( emu.offset < r->offset ) >>> { >>> /* Heading gap, read partial content from hardware. */ >>> @@ -432,6 +435,10 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, >>> unsigned int size) >>> } >>> spin_unlock(&pdev->vpci_lock); >>> >>> + /* All unhandled guest requests return all 1's. */ >>> + if ( !is_hardware_domain(d) && !handled ) >>> + return ~(uint32_t)0; >>> + >>> if ( data_offset < size ) >>> { >>> /* Tailing gap, read the remaining. */ >> Except that like for the "tailing gap" you also need to avoid the >> "heading gap" ending up in a read of the underlying hardware >> register. Effectively you want to deal properly with all >> vpci_read_hw() invocations (including the one when no pdev was >> found, which for a DomU may simply mean domain_crash()). > Yes. And with the above patch I can now remove the "TODO patch" then? > Because it is saying that we allow access to the registers, but it is not > safe. > And now, if we disable that access, then TODO should be about the need to > implement emulation for all the registers which are not yet handled which is > obvious. Yes, I think that other patch then should have no use anymore. (To be honest I don't recall such a patch anyway.) Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |