[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers
Hi, Roger! On 12.01.22 19:34, Roger Pau Monné wrote: > A couple more comments I realized while walking the dog. > > On Thu, Nov 25, 2021 at 01:02:43PM +0200, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >> >> Add relevant vpci register handlers when assigning PCI device to a domain >> and remove those when de-assigning. This allows having different >> handlers for different domains, e.g. hwdom and other guests. >> >> Emulate guest BAR register values: this allows creating a guest view >> of the registers and emulates size and properties probe as it is done >> during PCI device enumeration by the guest. >> >> ROM BAR is only handled for the hardware domain and for guest domains >> there is a stub: at the moment PCI expansion ROM handling is supported >> for x86 only and it might not be used by other architectures without >> emulating x86. Other use-cases may include using that expansion ROM before >> Xen boots, hence no emulation is needed in Xen itself. Or when a guest >> wants to use the ROM code which seems to be rare. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >> --- >> Since v4: >> - updated commit message >> - s/guest_addr/guest_reg >> Since v3: >> - squashed two patches: dynamic add/remove handlers and guest BAR >> handler implementation >> - fix guest BAR read of the high part of a 64bit BAR (Roger) >> - add error handling to vpci_assign_device >> - s/dom%pd/%pd >> - blank line before return >> Since v2: >> - remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code >> has been eliminated from being built on x86 >> Since v1: >> - constify struct pci_dev where possible >> - do not open code is_system_domain() >> - simplify some code3. simplify >> - use gdprintk + error code instead of gprintk >> - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT, >> so these do not get compiled for x86 >> - removed unneeded is_system_domain check >> - re-work guest read/write to be much simpler and do more work on write >> than read which is expected to be called more frequently >> - removed one too obvious comment >> --- >> xen/drivers/vpci/header.c | 72 +++++++++++++++++++++++++++++++++++---- >> xen/include/xen/vpci.h | 3 ++ >> 2 files changed, 69 insertions(+), 6 deletions(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index ba333fb2f9b0..8880d34ebf8e 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -433,6 +433,48 @@ static void bar_write(const struct pci_dev *pdev, >> unsigned int reg, >> pci_conf_write32(pdev->sbdf, reg, val); >> } >> >> +static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg, >> + uint32_t val, void *data) >> +{ >> + struct vpci_bar *bar = data; >> + bool hi = false; >> + >> + if ( bar->type == VPCI_BAR_MEM64_HI ) >> + { >> + ASSERT(reg > PCI_BASE_ADDRESS_0); >> + bar--; >> + hi = true; >> + } >> + else >> + { >> + val &= PCI_BASE_ADDRESS_MEM_MASK; >> + val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32 >> + : PCI_BASE_ADDRESS_MEM_TYPE_64; >> + val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0; >> + } >> + >> + bar->guest_reg &= ~(0xffffffffull << (hi ? 32 : 0)); >> + bar->guest_reg |= (uint64_t)val << (hi ? 32 : 0); >> + >> + bar->guest_reg &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK; > You need to assert that the guest set address has the same page offset > as the physical address on the host, or otherwise things won't work as > expected. Ie: guest_addr & ~PAGE_MASK == addr & ~PAGE_MASK. Good catch, thank you > >> +} >> + >> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg, >> + void *data) >> +{ >> + const struct vpci_bar *bar = data; >> + bool hi = false; >> + >> + if ( bar->type == VPCI_BAR_MEM64_HI ) >> + { >> + ASSERT(reg > PCI_BASE_ADDRESS_0); >> + bar--; >> + hi = true; >> + } >> + >> + return bar->guest_reg >> (hi ? 32 : 0); >> +} >> + >> static void rom_write(const struct pci_dev *pdev, unsigned int reg, >> uint32_t val, void *data) >> { >> @@ -481,6 +523,17 @@ static void rom_write(const struct pci_dev *pdev, >> unsigned int reg, >> rom->addr = val & PCI_ROM_ADDRESS_MASK; >> } >> >> +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; >> +} >> + >> static int init_bars(struct pci_dev *pdev) >> { >> uint16_t cmd; >> @@ -489,6 +542,7 @@ static int init_bars(struct pci_dev *pdev) >> struct vpci_header *header = &pdev->vpci->header; >> struct vpci_bar *bars = header->bars; >> int rc; >> + bool is_hwdom = is_hardware_domain(pdev->domain); >> >> switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f ) >> { >> @@ -528,8 +582,10 @@ static int init_bars(struct pci_dev *pdev) >> if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO ) >> { >> bars[i].type = VPCI_BAR_MEM64_HI; >> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, >> reg, >> - 4, &bars[i]); >> + rc = vpci_add_register(pdev->vpci, >> + is_hwdom ? vpci_hw_read32 : >> guest_bar_read, >> + is_hwdom ? bar_write : guest_bar_write, >> + reg, 4, &bars[i]); >> if ( rc ) >> { >> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); >> @@ -569,8 +625,10 @@ static int init_bars(struct pci_dev *pdev) >> bars[i].size = size; >> bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH; >> >> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, >> 4, >> - &bars[i]); >> + rc = vpci_add_register(pdev->vpci, >> + is_hwdom ? vpci_hw_read32 : guest_bar_read, >> + is_hwdom ? bar_write : guest_bar_write, >> + reg, 4, &bars[i]); > You need to initialize guest_reg to the physical host value also. But why? There was a concern that exposing host's value to a guest may be a security issue. And wouldn't it be possible for a guest to decide that the firmware has setup the BAR and it doesn't need to care of it and hence use a wrong value instead of setting it up by itself? I had an issue with that if I'm not mistaken that guest's Linux didn't set the BAR properly and used what was programmed > >> if ( rc ) >> { >> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); >> @@ -590,8 +648,10 @@ static int init_bars(struct pci_dev *pdev) >> header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) & >> PCI_ROM_ADDRESS_ENABLE; >> >> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, >> rom_reg, >> - 4, rom); >> + rc = vpci_add_register(pdev->vpci, >> + is_hwdom ? vpci_hw_read32 : guest_rom_read, >> + is_hwdom ? rom_write : guest_rom_write, >> + rom_reg, 4, rom); >> if ( rc ) >> rom->type = VPCI_BAR_EMPTY; > Also memory decoding needs to be initially disabled when used by > guests, in order to prevent the BAR being placed on top of a RAM > region. The guest physmap will be different from the host one, so it's > possible for BARs to end up placed on top of RAM regions initially > until the firmware or OS places them at a suitable address. Agree, memory decoding must be disabled > > Thanks, Roger. Thank you, Oleksandr
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |