| Message ID | 20221020094649.28667-7-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | (v)pci: fixes related to memory decoding handling | expand |
On 20.10.2022 11:46, Roger Pau Monne wrote: > Writes to the BARs are ignored if memory decoding is enabled for the > device, and the same happen with ROM BARs if the write is an attempt > to change the position of the BAR without disabling it first. > > The reason of ignoring such writes is a limitation in Xen, as it would > need to unmap the BAR, change the address, and remap the BAR at the > new position, which the current logic doesn't support. > > Some devices however seem to have the memory decoding bit hardcoded to > enabled, and attempts to disable it don't get reflected on the > command register. This isn't compliant with the spec, is it? It looks to contradict both "When a 0 is written to this register, the device is logically disconnected from the PCI bus for all accesses except configuration accesses" and "Devices typically power up with all 0's in this register, but Section 6.6 explains some exceptions" (quoting from the old 3.0 spec, which I have readily to hand). The referenced section then says "Such devices are required to support the Command register disabling function described in Section 6.2.2". How does any arbitrary OS go about sizing the BARs of such a device? > This causes issues for well behaved guests that disable memory > decoding and then try to size the BARs, as vPCI will think memory > decoding is still enabled and ignore the write. > > Since vPCI doesn't explicitly care about whether the memory decoding > bit is disabled as long as the BAR is not mapped in the guest p2m use > the information in the vpci_bar to check whether the BAR is mapped, > and refuse writes only based on that information. From purely a vPCI pov this looks to be a plausible solution (or should I better say workaround). I guess the two pieces of code that you alter would benefit from a comment as to it being intentional to _not_ check the command register (anymore). > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -388,7 +388,7 @@ static void cf_check bar_write( > else > val &= PCI_BASE_ADDRESS_MEM_MASK; > > - if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY ) > + if ( bar->enabled ) > { > /* If the value written is the current one avoid printing a warning. */ > if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) > @@ -425,7 +425,7 @@ static void cf_check rom_write( > uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND); > bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE; > > - if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled ) > + if ( rom->enabled && new_enabled ) > { > gprintk(XENLOG_WARNING, > "%pp: ignored ROM BAR write with memory decoding enabled\n", The log message wording then wants adjustment, I guess? What about if ( !(cmd & PCI_COMMAND_MEMORY) || header->rom_enabled == new_enabled ) a few lines down from here? Besides still using the command register value here not looking very consistent, wouldn't header->rom_enabled here an in the intermediate if() also better be converted to rom->enabled for consistency? Then again - is you also dropping the check of header->rom_enabled actually correct? While both are written to the same value by modify_decoding(), both rom_write() and init_bars() can bring the two booleans out of sync afaics. Jan
On Mon, Oct 24, 2022 at 01:51:03PM +0200, Jan Beulich wrote: > On 20.10.2022 11:46, Roger Pau Monne wrote: > > Writes to the BARs are ignored if memory decoding is enabled for the > > device, and the same happen with ROM BARs if the write is an attempt > > to change the position of the BAR without disabling it first. > > > > The reason of ignoring such writes is a limitation in Xen, as it would > > need to unmap the BAR, change the address, and remap the BAR at the > > new position, which the current logic doesn't support. > > > > Some devices however seem to have the memory decoding bit hardcoded to > > enabled, and attempts to disable it don't get reflected on the > > command register. > > This isn't compliant with the spec, is it? It looks to contradict both > "When a 0 is written to this register, the device is logically > disconnected from the PCI bus for all accesses except configuration > accesses" and "Devices typically power up with all 0's in this > register, but Section 6.6 explains some exceptions" (quoting from the > old 3.0 spec, which I have readily to hand). The referenced section > then says "Such devices are required to support the Command register > disabling function described in Section 6.2.2". It's unclear to me whether setting the bit to 0 is plain ignored, or just fails to be reflected on the command register. > How does any arbitrary OS go about sizing the BARs of such a device? AFAICT from Linux behavior, an OS would just set to 0 the memory decoding command register bit and write the value, but there's no check afterwards that the returned value from a read of the register still has memory decoding disabled. Xen does exactly the same: attempt to toggle the bit but don't check the value written. > > This causes issues for well behaved guests that disable memory > > decoding and then try to size the BARs, as vPCI will think memory > > decoding is still enabled and ignore the write. > > > > Since vPCI doesn't explicitly care about whether the memory decoding > > bit is disabled as long as the BAR is not mapped in the guest p2m use > > the information in the vpci_bar to check whether the BAR is mapped, > > and refuse writes only based on that information. > > From purely a vPCI pov this looks to be a plausible solution (or > should I better say workaround). I guess the two pieces of code that > you alter would benefit from a comment as to it being intentional to > _not_ check the command register (anymore). Ack. > > --- a/xen/drivers/vpci/header.c > > +++ b/xen/drivers/vpci/header.c > > @@ -388,7 +388,7 @@ static void cf_check bar_write( > > else > > val &= PCI_BASE_ADDRESS_MEM_MASK; > > > > - if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY ) > > + if ( bar->enabled ) > > { > > /* If the value written is the current one avoid printing a warning. */ > > if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) > > @@ -425,7 +425,7 @@ static void cf_check rom_write( > > uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND); > > bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE; > > > > - if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled ) > > + if ( rom->enabled && new_enabled ) > > { > > gprintk(XENLOG_WARNING, > > "%pp: ignored ROM BAR write with memory decoding enabled\n", > > The log message wording then wants adjustment, I guess? Hm, I think the message is fine for the purposes here, vPCI is indeed ignoring a write with memory decoding enabled, or else rom->enabled would be false. Or are you arguing that the message is already wrong in the current context and should instead be: "ignored ROM BAR write with memory decoding and ROM enabled" > What about > > if ( !(cmd & PCI_COMMAND_MEMORY) || header->rom_enabled == new_enabled ) > > a few lines down from here? Besides still using the command register > value here not looking very consistent, wouldn't header->rom_enabled > here an in the intermediate if() also better be converted to > rom->enabled for consistency? > > Then again - is you also dropping the check of header->rom_enabled > actually correct? rom->enabled should only be set when both the ROM is enabled and memory decoding is also enabled for the device. > While both are written to the same value by > modify_decoding(), both rom_write() and init_bars() can bring the > two booleans out of sync afaics. Right, bar->enabled is not a clone of header->rom_enabled, as the later only caches the ROM BAR enable bit, while the former caches both header->rom_enabled and whether memory decoding is also enabled (and the BAR is mapped). The usage of command register value in the checks below is indeed dubious, as the purpose of the change is tono trust the value of the memory decoding bit in the command register. I think the only way is to cache the Xen intended value of the memory decoding command register bit for it's usage in rom_write(). Thanks, Roger.
On 24.10.2022 17:04, Roger Pau Monné wrote: > On Mon, Oct 24, 2022 at 01:51:03PM +0200, Jan Beulich wrote: >> On 20.10.2022 11:46, Roger Pau Monne wrote: >>> Writes to the BARs are ignored if memory decoding is enabled for the >>> device, and the same happen with ROM BARs if the write is an attempt >>> to change the position of the BAR without disabling it first. >>> >>> The reason of ignoring such writes is a limitation in Xen, as it would >>> need to unmap the BAR, change the address, and remap the BAR at the >>> new position, which the current logic doesn't support. >>> >>> Some devices however seem to have the memory decoding bit hardcoded to >>> enabled, and attempts to disable it don't get reflected on the >>> command register. >> >> This isn't compliant with the spec, is it? It looks to contradict both >> "When a 0 is written to this register, the device is logically >> disconnected from the PCI bus for all accesses except configuration >> accesses" and "Devices typically power up with all 0's in this >> register, but Section 6.6 explains some exceptions" (quoting from the >> old 3.0 spec, which I have readily to hand). The referenced section >> then says "Such devices are required to support the Command register >> disabling function described in Section 6.2.2". > > It's unclear to me whether setting the bit to 0 is plain ignored, or > just fails to be reflected on the command register. > >> How does any arbitrary OS go about sizing the BARs of such a device? > > AFAICT from Linux behavior, an OS would just set to 0 the memory > decoding command register bit and write the value, but there's no > check afterwards that the returned value from a read of the register > still has memory decoding disabled. Xen does exactly the same: > attempt to toggle the bit but don't check the value written. Sure - both assume that the bit is writable in the first place. Yet altering the BARs when the write had no effect is not really correct. >>> --- a/xen/drivers/vpci/header.c >>> +++ b/xen/drivers/vpci/header.c >>> @@ -388,7 +388,7 @@ static void cf_check bar_write( >>> else >>> val &= PCI_BASE_ADDRESS_MEM_MASK; >>> >>> - if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY ) >>> + if ( bar->enabled ) >>> { >>> /* If the value written is the current one avoid printing a warning. */ >>> if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) >>> @@ -425,7 +425,7 @@ static void cf_check rom_write( >>> uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND); >>> bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE; >>> >>> - if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled ) >>> + if ( rom->enabled && new_enabled ) >>> { >>> gprintk(XENLOG_WARNING, >>> "%pp: ignored ROM BAR write with memory decoding enabled\n", >> >> The log message wording then wants adjustment, I guess? > > Hm, I think the message is fine for the purposes here, vPCI is indeed > ignoring a write with memory decoding enabled, or else rom->enabled > would be false. > > Or are you arguing that the message is already wrong in the current > context and should instead be: > > "ignored ROM BAR write with memory decoding and ROM enabled" No, my point rather was that we're no longer checking for memory decoding to be disabled. Aiui we still require that the guest has cleared its view of the bit, so I guess the messages could still be viewed as applicable. Personally I'd prefer disambiguation. Jan
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 4d7f8f4a30..4b39737b76 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -388,7 +388,7 @@ static void cf_check bar_write( else val &= PCI_BASE_ADDRESS_MEM_MASK; - if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY ) + if ( bar->enabled ) { /* If the value written is the current one avoid printing a warning. */ if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) ) @@ -425,7 +425,7 @@ static void cf_check rom_write( uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND); bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE; - if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled ) + if ( rom->enabled && new_enabled ) { gprintk(XENLOG_WARNING, "%pp: ignored ROM BAR write with memory decoding enabled\n",
Writes to the BARs are ignored if memory decoding is enabled for the device, and the same happen with ROM BARs if the write is an attempt to change the position of the BAR without disabling it first. The reason of ignoring such writes is a limitation in Xen, as it would need to unmap the BAR, change the address, and remap the BAR at the new position, which the current logic doesn't support. Some devices however seem to have the memory decoding bit hardcoded to enabled, and attempts to disable it don't get reflected on the command register. This causes issues for well behaved guests that disable memory decoding and then try to size the BARs, as vPCI will think memory decoding is still enabled and ignore the write. Since vPCI doesn't explicitly care about whether the memory decoding bit is disabled as long as the BAR is not mapped in the guest p2m use the information in the vpci_bar to check whether the BAR is mapped, and refuse writes only based on that information. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/drivers/vpci/header.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)