[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.22 13:27, Roger Pau Monné wrote: > On Mon, Jan 31, 2022 at 11:04:29AM +0000, Oleksandr Andrushchenko wrote: >> Hi, Jan! >> >> On 31.01.22 12:54, Jan Beulich wrote: >>> On 31.01.2022 11:40, Oleksandr Andrushchenko wrote: >>>> On 31.01.22 11:47, Oleksandr Andrushchenko wrote: >>>>> Hi, Roger! >>>>> >>>>> On 12.01.22 14:35, Roger Pau Monné wrote: >>>>>>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int >>>>>>> reg, >>>>>>> + uint32_t val, void *data) >>>>>>> +{ >>>>>>> +} >>>>>>> + >>>>>>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned >>>>>>> int reg, >>>>>>> + void *data) >>>>>>> +{ >>>>>>> + return 0xffffffff; >>>>>>> +} >>>>>> There should be no need for those handlers. As said elsewhere: for >>>>>> guests registers not explicitly handled should return ~0 for reads and >>>>>> drop writes, which is what you are proposing here. >>>>> Yes, you are right: I can see in vpci_read that we end up reading ~0 if no >>>>> handler exists (which is what I do here with guest_rom_read). But I am >>>>> not that >>>>> sure about the dropped writes: >>>>> >>>>> void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, >>>>> uint32_t data) >>>>> { >>>>> unsigned int data_offset = 0; >>>>> >>>>> [snip] >>>>> >>>>> if ( data_offset < size ) >>>>> /* Tailing gap, write the remaining. */ >>>>> vpci_write_hw(sbdf, reg + data_offset, size - data_offset, >>>>> data >> (data_offset * 8)); >>>>> >>>>> so it looks like for the un-handled writes we still reach the HW register. >>>>> Could you please tell if the code above needs improvement (like checking >>>>> if the write was handled) or I still need to provide a write handler, e.g. >>>>> guest_rom_write here? >>>> Hm, but the same applies to the reads as well... And this is no surprise, >>>> as for the guests I can see that it accesses all the configuration space >>>> registers that I don't handle. Without that I would have guests unable >>>> to properly setup a PCI device being passed through... And this is why >>>> I have a big TODO in this series describing unhandled registers. >>>> So, it seems that I do need to provide those handlers which I need to >>>> drop writes and return ~0 on reads. >> Replying to myself: it is still possible to have vpci_ignored_{read|write} >> to handle defaults if, when vpci_add_register is called, the handler >> provided is NULL >>> It feels like we had been there before: For your initial purposes it may >>> be fine to do as you suggest, but any such patches should carry RFC tags >>> or alike to indicate they're not considered ready. Once you're aiming >>> for things to go in, I think there's no good way around white-listing >>> what guests may access. You may know that we've been bitten by starting >>> out with black-listing in the past, first and foremost with x86'es MSRs. >> I already have a big TODO patch describing the issue. Do you want >> it to have a list of handlers that we support as of now? What sort of >> while/black list would you expect? >> I do understand that we do need proper handling for all the PCI registers >> and capabilities long term, but this can't be done at the moment when >> we have nothing working at all. Requesting proper handling now will >> turn this series into a huge amount of code and undefined time frame. > We should at least make sure the code added now doesn't need to be > changed in the future when the default is switched. If you don't > want to switch the default handling for domUs to ignore writes and > return ~0 from reads to unhandled registers right now you should keep > the patches that add the ignore handlers to the end of the series and > mark them as 'HACK' or some such in order to notice they are just > used for testing purposes. Or for all the registers that I do want the writes to be rejected and reads return ~0 I can pass NULL while calling vpci_add_register, so the following works: int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler, vpci_write_t *write_handler, unsigned int offset, unsigned int size, void *data) { [snip] r->read = read_handler ?: vpci_ignored_read; r->write = write_handler ?: vpci_ignored_write; which does what we want. > Thanks, Roger. Thank you, Oleksandr
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |